From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/panel: add driver for Waveshare 8.8" DSI TOUCH-A panel
Date: Sun, 12 Apr 2026 09:06:57 +1000
Message-ID: <review-patch19-20260411-waveshare-dsi-touch-v2-19-75cdbeac5156@oss.qualcomm.com>
In-Reply-To: <20260411-waveshare-dsi-touch-v2-19-75cdbeac5156@oss.qualcomm.com>
References: <20260411-waveshare-dsi-touch-v2-0-75cdbeac5156@oss.qualcomm.com> <20260411-waveshare-dsi-touch-v2-19-75cdbeac5156@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

New OTA7290B panel driver. Has several issues:

1. **Bug -- wrong ERR_PTR check**:
   ```c
   ctx = devm_drm_panel_alloc(&dsi->dev, struct ota7290b, panel, ...);
   if (!ctx)
       return -ENOMEM;
   ```
   `devm_drm_panel_alloc()` returns `ERR_PTR()` on failure, not NULL (confirmed in `drm_panel.c:416`). This should be `if (IS_ERR(ctx)) return PTR_ERR(ctx);`.

2. **Bug -- commas instead of semicolons**:
   ```c
   dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_HSE |
       MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS,
   dsi->format = MIPI_DSI_FMT_RGB888,
   dsi->lanes = 2;
   ```
   The first two lines end with commas. Due to C comma operator semantics, the values happen to be assigned correctly, but this is clearly unintentional and should be semicolons.

3. **Bug -- wrong variable in error message**:
   ```c
   if (dsi_ctx.accum_err < 0)
       dev_err(panel->dev, "failed to init panel: %d\n", ret);
   ```
   Prints `ret` (last regulator enable result) instead of `dsi_ctx.accum_err`.

4. **Regulator errors not propagated**: `ota7290b_prepare()` logs regulator enable failures but continues execution. If `regulator_enable()` fails, the function should return the error rather than proceeding to send DSI commands to an unpowered panel.

5. **No cleanup on prepare failure**: If DSI commands fail after regulators are enabled, the function returns the error without disabling regulators or re-asserting reset.

6. **Same `devm_regulator_get_optional()` issue** as patch 17 -- treats `-ENODEV` as fatal while guarding with NULL checks.

7. **Kconfig error**: Help text says "Waveshare 12.3" DSI TOUCH-A panel" but this driver is for the 8.8" panel.

---

---
Generated by Claude Code Patch Reviewer