From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/panel: add LXD M9189A panel driver Date: Wed, 11 Feb 2026 16:26:36 +1000 Message-ID: In-Reply-To: <20260210-drm-panel-ek79007ad3-v3-3-cd2974d56937@pengutronix.de> References: <20260210-drm-panel-ek79007ad3-v3-0-cd2974d56937@pengutronix.de> <20260210-drm-panel-ek79007ad3-v3-3-cd2974d56937@pengutronix.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Mailer: Claude Code Patch Reviewer Patch Review **Subject:** `[PATCH v3 3/3] drm/panel: add LXD M9189A panel driver` **Review of driver implementation:** #### 1. **File header copyright mismatch** (lines 147-150): ```c +// SPDX-License-Identifier: GPL-2.0-only +/* + * Generated with linux-mdss-dsi-panel-driver-generator from vendor device tree. + * Copyright (c) 2024 Luca Weiss + */ ``` **Issue:** The copyright attributes the code to Luca Weiss, but the patch is signed off by Rouven Czerwinski and Michael Tretter. This appears to be boilerplate from a code generator that wasn't updated. Either: - Update copyright to reflect actual authors, or - Add a note explaining this was generated from tooling with that copyright #### 2. **Naming inconsistency** (throughout file): The driver uses mixed naming: - File: `panel-lxd-m9189a.c` (line 88, 140) - Kconfig: `DRM_PANEL_LXD_M9189A` (line 116) - Functions: `m9189_*` prefix (e.g., `m9189_reset`, `m9189_on`) - Structure: `m9189_panel` - Driver name: `panel-lxa-m9189a` (line 381) ← **TYPO!** ```c + .driver = { + .name = "panel-lxa-m9189a", + .of_match_table = lxd_m9189_of_match, + }, ``` **Critical bug:** The driver name is `"panel-lxa-m9189a"` (lxa) but should be `"panel-lxd-m9189a"` (lxd) to match the vendor prefix and file naming. #### 3. **Magic numbers in timing** (line 281): ```c +static const struct drm_display_mode m9189_mode = { + .clock = (1024 + 160 + 160 + 10) * (600 + 12 + 23 + 1) * 60 / 1000, + .hdisplay = 1024, + .hsync_start = 1024 + 160, + .hsync_end = 1024 + 160 + 160, + .htotal = 1024 + 160 + 160 + 10, + .vdisplay = 600, + .vsync_start = 600 + 12, + .vsync_end = 600 + 12 + 23, + .vtotal = 600 + 12 + 23 + 1, ``` **Suggestion:** While the formula for clock calculation is clear, consider using #defines for the porch/sync values to make them more readable: ```c #define M9189_HFRONT_PORCH 160 #define M9189_HSYNC 160 #define M9189_HBACK_PORCH 10 #define M9189_VFRONT_PORCH 12 #define M9189_VSYNC 23 #define M9189_VBACK_PORCH 1 ``` This is a minor style issue but improves maintainability. #### 4. **Missing mode flags** (line 281-292): The mode structure doesn't specify any flags (hsync/vsync polarity, etc.): ```c +static const struct drm_display_mode m9189_mode = { + .clock = (1024 + 160 + 160 + 10) * (600 + 12 + 23 + 1) * 60 / 1000, + ... + .width_mm = 154, + .height_mm = 86, +}; ``` **Question:** Are the default sync polarities (positive hsync/vsync) correct for this panel? Typically this should be documented or explicitly set. #### 5. **Error handling in probe** (line 317-320): ```c + m9189->supply = devm_regulator_get(dev, "power"); + if (IS_ERR(m9189->supply)) + return dev_err_probe(dev, PTR_ERR(m9189->supply), + "Failed to get power-supply\n"); ``` **Good:** The v3 changelog mentions "Fix error code if regulator is missing" and this looks correct. Using `dev_err_probe()` with `PTR_ERR()` is the right pattern. #### 6. **Regulator enable/disable asymmetry** (lines 246-250 vs 275): In `m9189_prepare()`: ```c + ret = regulator_enable(m9189->supply); + if (ret < 0) { + dev_err(dev, "Failed to enable regulators: %d\n", ret); + return ret; + } ``` In error path (lines 259-260): ```c + gpiod_set_value_cansleep(m9189->reset_gpio, 1); + regulator_disable(m9189->supply); ``` In `m9189_unprepare()` (line 275): ```c + regulator_disable(m9189->supply); ``` **Analysis:** The regulator is enabled in `prepare()` and disabled in `unprepare()`, which is correct. Error paths also properly disable it. However, there's a potential issue: In `m9189_disable()` (lines 235-236): ```c + gpiod_set_value_cansleep(m9189->standby_gpio, 1); + + return ctx.accum_err; ``` The `disable()` callback sets standby but doesn't touch the regulator. This is probably correct since `unprepare()` handles power, but the ordering should be verified: - `prepare()` enables power, then initializes panel - `disable()` puts panel in sleep mode - `unprepare()` cuts power This appears correct per DRM panel API conventions. #### 7. **GPIO active state confusion** (lines 189-194): ```c +static void m9189_reset(struct m9189_panel *m9189) +{ + gpiod_set_value_cansleep(m9189->reset_gpio, 0); + msleep(20); + gpiod_set_value_cansleep(m9189->reset_gpio, 1); + msleep(30); + gpiod_set_value_cansleep(m9189->reset_gpio, 0); + msleep(55); +} ``` The reset sequence is: LOW → HIGH → LOW. Combined with: ```c + m9189->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); ``` And the binding example: ```yaml + reset-gpios = <&gpio3 25 GPIO_ACTIVE_LOW>; ``` **Analysis:** - GPIO is acquired with `GPIOD_OUT_LOW` (logical 0) - With `GPIO_ACTIVE_LOW` in DT, this means physical HIGH - Reset sequence: physical HIGH → LOW → HIGH - Assuming reset is active-low (common), this means: inactive → active → inactive This seems inverted. Typically reset should start inactive, go active, then inactive: - Start: logical 0 (inactive, physical HIGH with ACTIVE_LOW) ✓ - Assert reset: logical 1 (active, physical LOW with ACTIVE_LOW) ✓ - Release: logical 0 (inactive, physical HIGH) ✓ Wait, the sequence is `0 → 1 → 0`, which IS correct! The reset line is asserted (logical 1) in the middle. The code is actually correct. However, in the error path and unprepare (lines 259, 274): ```c + gpiod_set_value_cansleep(m9189->reset_gpio, 1); ``` This sets reset to logical 1 (active/asserted) to keep the panel in reset while powered off, which makes sense. **No issue here**, but the logic is a bit hard to follow. A comment might help. #### 8. **Manufacturer command definitions** (lines 164-172): ```c +/* Manufacturer specific DSI commands */ +#define EK79007AD3_GAMMA1 0x80 +#define EK79007AD3_GAMMA2 0x81 +#define EK79007AD3_GAMMA3 0x82 +#define EK79007AD3_GAMMA4 0x83 +#define EK79007AD3_GAMMA5 0x84 +#define EK79007AD3_GAMMA6 0x85 +#define EK79007AD3_GAMMA7 0x86 +#define EK79007AD3_PANEL_CTRL3 0xB2 ``` **Good:** Commands are well-documented with the controller name prefix. #### 9. **Hardcoded gamma values** (lines 203-211): ```c + /* Gamma 2.2 */ + mipi_dsi_dcs_write_seq_multi(&ctx, EK79007AD3_GAMMA1, 0x48); + mipi_dsi_dcs_write_seq_multi(&ctx, EK79007AD3_GAMMA2, 0xB8); + mipi_dsi_dcs_write_seq_multi(&ctx, EK79007AD3_GAMMA3, 0x88); + mipi_dsi_dcs_write_seq_multi(&ctx, EK79007AD3_GAMMA4, 0x88); + mipi_dsi_dcs_write_seq_multi(&ctx, EK79007AD3_GAMMA5, 0x58); + mipi_dsi_dcs_write_seq_multi(&ctx, EK79007AD3_GAMMA6, 0xD2); + mipi_dsi_dcs_write_seq_multi(&ctx, EK79007AD3_GAMMA7, 0x88); + mipi_dsi_msleep(&ctx, 50); ``` **Acceptable:** These are panel-specific gamma calibration values, likely from vendor specification. Hardcoding them is normal for panel drivers. #### 10. **Generic write for panel control** (line 214): ```c + /* 4 Lanes */ + mipi_dsi_generic_write_multi(&ctx, (u8[]){ EK79007AD3_PANEL_CTRL3, 0x70 }, 2); ``` **Question:** Why is this using `mipi_dsi_generic_write_multi()` instead of `mipi_dsi_dcs_write_seq_multi()` like the gamma commands? Is `0xB2` not a DCS command? If it's a manufacturer-specific command, `generic_write` is correct, but then the gamma commands above should probably also use generic_write. The comment says the panel supports "only 4 lane operation" (line 65), but the code doesn't validate that `dsi->lanes == 4` anywhere. Consider adding validation in probe: ```c if (dsi->lanes != 4) { dev_err(dev, "This panel only supports 4-lane operation\n"); return -EINVAL; } ``` Or if the panel could theoretically support other lane counts, remove the "only" from the commit message. #### 11. **MAINTAINERS entry** (lines 99-104): ``` +DRM DRIVER FOR LXD M9189A PANELS +M: Michael Tretter +S: Maintained +F: Documentation/devicetree/bindings/display/panel/lxd,m9189a.yaml +F: drivers/gpu/drm/panel/panel-lxd-m9189a.c ``` **Good:** Proper MAINTAINERS entry with status and file patterns. Alphabetical ordering looks correct (between LVDS and MANTIX entries). #### 12. **Module metadata** (lines 387-388): ```c +MODULE_DESCRIPTION("DRM driver for LXD M9189A MIPI-DSI panels"); +MODULE_LICENSE("GPL"); ``` **Issue:** License mismatch. The SPDX header says `GPL-2.0-only` but `MODULE_LICENSE("GPL")` can mean GPL v2 or later. Should be: ```c MODULE_LICENSE("GPL v2"); ``` to match the SPDX identifier. --- ## SUMMARY OF ISSUES ### Critical: 1. **Driver name typo** (panel-lxa vs panel-lxd) - MUST FIX 2. **MODULE_LICENSE mismatch** - should be "GPL v2" not "GPL" ### Important: 3. **Copyright header** - needs updating or explanation 4. **Backlight requirement** - unclear if optional or required in binding 5. **Lane count validation** - driver doesn't validate 4-lane requirement ### Minor/Suggestions: 6. Magic numbers in display mode could use #defines 7. Missing mode flags documentation 8. Inconsistent use of DCS vs generic write 9. Standby GPIO needs better documentation in binding ### Good practices observed: - Use of modern multi-context helpers - Proper error handling with dev_err_probe - Correct panel lifecycle (prepare/unprepare/disable) - Devm resource management - prepare_prev_first flag set appropriately **Overall verdict:** The series needs at least the critical fixes before merge. The rest are improvements that could be addressed or explained in discussion with maintainers. --- Generated by Claude Code Patch Reviewer