* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-03-23 17:08 [PATCH v3 0/2] " Aaron Kling via B4 Relay
2026-03-23 17:08 ` [PATCH v3 2/2] " Aaron Kling via B4 Relay
@ 2026-03-24 21:40 ` Claude Code Review Bot
1 sibling, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-03-24 21:40 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/panel: Add panel driver for ChipWealth CH13726A based panels
Author: Aaron Kling via B4 Relay <devnull+webgeek1234.gmail.com@kernel.org>
Patches: 5
Reviewed: 2026-03-25T07:40:21.203030
---
This is a v3 series adding a new MIPI-DSI panel driver for the ChipWealth CH13726A, used in the AYN Thor handheld's bottom panel. The series is 2 patches: a dt-binding YAML and the panel driver itself. The driver is straightforward and mostly follows current conventions, but there are several issues ranging from an incorrect Kconfig select to deprecated patterns that should be addressed.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-03-23 17:08 ` [PATCH v3 2/2] " Aaron Kling via B4 Relay
@ 2026-03-24 21:40 ` Claude Code Review Bot
0 siblings, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-03-24 21:40 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Several issues:
1. **Unnecessary and incorrect Kconfig selects.** The Kconfig entry selects `DRM_DISPLAY_DP_HELPER` and `DRM_DISPLAY_HELPER`:
```
+ select DRM_DISPLAY_DP_HELPER
+ select DRM_DISPLAY_HELPER
```
This is a MIPI-DSI panel, not a DisplayPort panel. The driver doesn't use any DP helpers. These selects should be removed entirely — no replacement is needed since `drm_mipi_dsi.h` and `drm_panel.h` don't depend on them.
2. **`prepared` flag is deprecated.** The driver manually tracks `prepared` state:
```c
+ bool prepared;
```
```c
+ if (ctx->prepared)
+ return 0;
```
The DRM panel framework has handled prepare/unprepare idempotency since commit work in recent kernels. Only 2 legacy drivers in the tree still use this pattern. Remove the `prepared` field and the guard checks in `ch13726a_prepare()` and `ch13726a_unprepare()`.
3. **`of_device_get_match_data()` cast drops const.** The probe function casts away const:
```c
+ ctx->desc = (struct ch13726a_desc *)of_device_get_match_data(dev);
```
The `desc` field in the struct should be `const struct ch13726a_desc *desc` and the cast removed. Similarly, `thor_bottom_desc` should be `static const`:
```c
+static struct ch13726a_desc thor_bottom_desc = {
```
Should be `static const struct ch13726a_desc thor_bottom_desc`.
4. **`uint8_t` is not kernel style.** The kernel uses `u8` not `uint8_t`:
```c
+ for (uint8_t i = 0; i < ctx->desc->num_modes; i++) {
```
Should use `unsigned int i` (since `num_modes` is `unsigned int` and the value is small, there's no reason to use `u8` here anyway).
5. **Reset GPIO polarity may be inverted.** The reset sequence does high-low-high:
```c
+ gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+ usleep_range(10000, 11000);
+ gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+ usleep_range(10000, 11000);
+ gpiod_set_value_cansleep(ctx->reset_gpio, 1);
```
The GPIO is obtained with `GPIOD_OUT_LOW` (meaning initially deasserted). Typically a reset is assert-deassert (1-0-1 asserts then deasserts), but the dt-binding example uses `GPIO_ACTIVE_HIGH`, which means `gpiod_set_value(1)` drives the pin high. This is plausible for an active-high reset, but worth confirming the hardware — most AMOLED panels use active-low reset where the sequence would be 0-1-0 (assert low, release high). If the GPIO is active-high reset, then the unprepare path sets reset to 0 (deasserted/released), which means the panel leaves reset deasserted during power-down — that's backwards. Usually you want to assert reset *before* cutting power.
6. **`ch13726a_disable()` clears `MIPI_DSI_MODE_LPM` but `ch13726a_on()` sets it.** In `ch13726a_disable()`:
```c
+ ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
```
This means the display-off and sleep-enter commands are sent in HS mode, but the init sequence sends commands in LP mode. This is fine if intentional, but the backlight handler also toggles LPM:
```c
+ dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
+ ...
+ dsi->mode_flags |= MIPI_DSI_MODE_LPM;
```
Toggling `mode_flags` like this is racy if backlight updates can happen concurrently with enable/disable. Consider setting the mode flags once in probe and not toggling them per-operation.
7. **Missing `#include <linux/of.h>` guard.** The driver includes `<linux/of.h>` but could use `<linux/mod_devicetable.h>` instead for `of_device_id`, matching newer driver conventions (see `panel-boe-td4320.c`). Minor style point.
8. **`ch13726a_remove()` error check on detach is unnecessary boilerplate.** `mipi_dsi_detach()` return value logging in remove is acceptable but some maintainers prefer to just ignore it since there's nothing useful to do with the error in the remove path.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-04-08 5:32 [PATCH v4 0/2] " Aaron Kling via B4 Relay
2026-04-08 5:32 ` [PATCH v4 2/2] " Aaron Kling via B4 Relay
@ 2026-04-12 3:10 ` Claude Code Review Bot
1 sibling, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-04-12 3:10 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/panel: Add panel driver for ChipWealth CH13726A based panels
Author: Aaron Kling via B4 Relay <devnull+webgeek1234.gmail.com@kernel.org>
Patches: 4
Reviewed: 2026-04-12T13:10:38.270464
---
This is a straightforward 2-patch series adding a new MIPI-DSI panel driver for the ChipWealth CH13726A AMOLED, used as the bottom panel on the AYN Thor handheld device. The dt-bindings patch is clean and well-structured. The driver patch is functional but has several issues ranging from unnecessary boilerplate (redundant `prepared` tracking that the core already handles) to a potentially incorrect Kconfig (selecting DP helpers for a pure DSI panel) and minor style issues. None are critical, but a v5 addressing them would be worthwhile.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-04-08 5:32 ` [PATCH v4 2/2] " Aaron Kling via B4 Relay
@ 2026-04-12 3:10 ` Claude Code Review Bot
0 siblings, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-04-12 3:10 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Several issues need attention:
**1. Unnecessary `select DRM_DISPLAY_DP_HELPER` and `select DRM_DISPLAY_HELPER` in Kconfig (important)**
```c
+ select DRM_DISPLAY_DP_HELPER
+ select DRM_DISPLAY_HELPER
```
This is a pure MIPI-DSI panel driver — it has no DisplayPort or DSC functionality. These selects are unnecessary and pull in code the driver doesn't use. Other pure MIPI-DSI panel drivers (e.g., `panel-boe-th101mb31ig002-28a`, `panel-himax-hx8394`) do not select these. Remove both lines.
**2. Redundant `prepared` flag tracking (important)**
```c
+struct ch13726a_panel {
+ ...
+ bool prepared;
+};
```
And in `ch13726a_prepare()`:
```c
+ if (ctx->prepared)
+ return 0;
+ ...
+ ctx->prepared = true;
```
And in `ch13726a_unprepare()`:
```c
+ if (!ctx->prepared)
+ return 0;
+ ...
+ ctx->prepared = false;
```
The DRM panel core already tracks `panel->prepared` and guards against double-prepare/unprepare in `drm_panel_prepare()`/`drm_panel_unprepare()` (see `drm_panel.c` lines 122-134 and 177-199). This driver-level tracking is redundant and should be removed. No current in-tree panel driver uses a private `prepared` flag anymore.
**3. `thor_bottom_desc` should be `const` (minor)**
```c
+static struct ch13726a_desc thor_bottom_desc = {
```
This should be `static const struct ch13726a_desc`. It's immutable data. The struct member should also be `const`:
```c
- struct ch13726a_desc *desc;
+ const struct ch13726a_desc *desc;
```
This would also eliminate the need for the cast in probe:
```c
+ ctx->desc = (struct ch13726a_desc *)of_device_get_match_data(dev);
```
With `const struct ch13726a_desc *desc`, this becomes simply:
```c
ctx->desc = of_device_get_match_data(dev);
```
**4. `uint8_t` loop variable (minor, style)**
```c
+ for (uint8_t i = 0; i < ctx->desc->num_modes; i++) {
```
Kernel code uses `u8` rather than `uint8_t`, but for a loop index iterating over a small count, a plain `unsigned int` or `int` is more idiomatic. Declaring the variable inside `for()` is a C99/C11 feature — while it works with modern kernel builds, many panel drivers still declare loop variables at function scope. Use `int i;` declared at the top for consistency with the prevailing kernel style.
**5. Reset GPIO polarity in error path (worth verifying)**
In `ch13726a_prepare()`:
```c
+ gpiod_set_value_cansleep(ctx->reset_gpio, 0);
```
The GPIO is obtained with `GPIOD_OUT_LOW` and the reset sequence asserts high (1), deasserts to low (0), then asserts high (1) again. In the error path after `ch13726a_on()` fails, the driver sets GPIO to 0 (deasserted/inactive), which keeps the panel out of reset. This is inconsistent — typically on error you want to put the panel *into* reset (assert reset) before disabling regulators. If active-high means "reset active", the error path should set the GPIO to 1. However, this depends on the actual hardware semantics. Since the binding says `GPIO_ACTIVE_HIGH` and `reset-gpios` is the name, `gpiod_set_value(1)` means "assert reset" which would be the safer state on error. Worth confirming with the hardware.
Similarly in `ch13726a_unprepare()`:
```c
+ gpiod_set_value_cansleep(ctx->reset_gpio, 0);
```
This deasserts reset before killing power, which could leave the panel in an undefined state. Most panel drivers assert reset (set to 1) before cutting regulators to ensure clean shutdown. Compare with how `ch13726a_reset()` works — it uses `1` as the active/working state. The unprepare should logically assert reset (1) then cut power, not the other way around.
**6. `mipi_dsi_detach` error checking in `remove()` is unnecessary noise (very minor)**
```c
+ ret = mipi_dsi_detach(dsi);
+ if (ret < 0)
+ dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
```
`mipi_dsi_detach()` never fails in practice (the host ops->detach can return an error, but no upstream host driver does). Newer panel drivers tend to not check this return. Not a blocker, but the error message adds no value.
**7. Missing `msleep`/delay between display-off and sleep-enter in `ch13726a_disable()` (minor)**
```c
+ mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+ mipi_dsi_msleep(&dsi_ctx, 50);
+ mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
```
There is a 50ms delay between display-off and sleep-enter, which is good. But there is no delay *after* entering sleep mode. Many MIPI panel datasheets require a delay (commonly 120ms) after entering sleep mode before cutting power in `unprepare()`. The `ch13726a_on()` path has a 28ms `msleep` at the end of `prepare()`, but the disable->unprepare transition has no such guard. If the panel requires a post-sleep delay per its datasheet, this could cause issues.
**8. Mode flags manipulation in backlight callback (worth noting)**
```c
+static int ch13726a_bl_update_status(struct backlight_device *bl)
+{
+ ...
+ dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
+ ...
+ dsi->mode_flags |= MIPI_DSI_MODE_LPM;
```
And in `ch13726a_on()`:
```c
+ ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
```
And `ch13726a_disable()`:
```c
+ ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
```
The LPM flag toggling is not protected by any locking. If backlight updates can race with disable/prepare calls, the mode_flags could be corrupted. This is a pre-existing pattern in many panel drivers, so not unique to this one, but worth noting.
**Summary of requested changes for v5:**
1. Remove `DRM_DISPLAY_DP_HELPER` and `DRM_DISPLAY_HELPER` from Kconfig
2. Remove the private `prepared` bool and the associated checks
3. Make `thor_bottom_desc` and the `desc` pointer `const`, drop the cast
4. Use `int` or `unsigned int` for the loop variable, not `uint8_t`
5. Verify GPIO reset polarity in error/unprepare paths (assert reset before cutting power)
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v5 0/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels
@ 2026-04-21 17:38 Aaron Kling via B4 Relay
2026-04-21 17:38 ` [PATCH v5 1/2] dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver Aaron Kling via B4 Relay
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Aaron Kling via B4 Relay @ 2026-04-21 17:38 UTC (permalink / raw)
To: Neil Armstrong, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-kernel, Teguh Sobirin, Aaron Kling
This driver is based on the one by Teguh Sobirin [0], cut down to only
support the AYN Thor bottom panel.
Due to [1], the AYN vendor description patch has been folded into the
AYN QCS8550 dt series. Which means this series now depends on said
series and it must be picked up before this.
[0] https://github.com/AYNTechnologies/linux/commit/d0ff75b09e66023c5f88992706dee4601aa7a437
[1] https://lore.kernel.org/linux-arm-msm/c7fb3f89-6574-4761-9ef2-2fdf6d4801b5@kernel.org
Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
Changes in v5:
- Don't duplicate prepared tracking
- Use devm_regulator_bulk_get_const() instead of setting names in probe
- Use devm_drm_panel_alloc() instead of drm_panel_init()
- Link to v4: https://lore.kernel.org/r/20260408-ch13726a-v4-0-9bb1a9b8f329@gmail.com
Changes in v4:
- Add fallback compatible to bindings doc in patch 1
- Link to v3: https://lore.kernel.org/r/20260323-ch13726a-v3-0-e28b6f97fe80@gmail.com
Changes in v3:
- Reword patch 1 commit message for clarity
- Order properties properly in patch 1
- Track vendor description dependency series change
- Link to v2: https://lore.kernel.org/r/20260317-ch13726a-v2-0-28aa46bcd6d0@gmail.com
Changes in v2:
- Add Neil Armstrong to binding maintainer list
- Add 120hz mode, which required a minor restructure of the driver
- Link to v1: https://lore.kernel.org/r/20260222-ch13726a-v1-0-e501d78e105a@gmail.com
---
Aaron Kling (1):
dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver
Teguh Sobirin (1):
drm/panel: Add panel driver for ChipWealth CH13726A based panels
.../display/panel/chipwealth,ch13726a.yaml | 67 +++++
drivers/gpu/drm/panel/Kconfig | 11 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-chipwealth-ch13726a.c | 333 +++++++++++++++++++++
4 files changed, 412 insertions(+)
---
base-commit: bee6ea30c48788e18348309f891ed8afbf7702ac
change-id: 20260220-ch13726a-59c6678d53d8
prerequisite-change-id: 20260217-ayn-qcs8550-16c07b63de26:v5
prerequisite-patch-id: 2b010637c46f5ea0e19a2fa87cc5ccb19bfd9204
prerequisite-patch-id: d0c633dc17f0aba726a8e8d21fee52b944bf67ff
prerequisite-patch-id: 76ac126273870a96f05e1bd87f4f31bdc3746440
prerequisite-patch-id: 76b61799dba520fc7fa3ff39b044e24103337af3
prerequisite-patch-id: 3844bef2eda3cf59031b1d131eb6ba9295629bb4
prerequisite-patch-id: 3331648bc6f4ac3bb156f0525aa1ede92bfc57f1
Best regards,
--
Aaron Kling <webgeek1234@gmail.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v5 1/2] dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver
2026-04-21 17:38 [PATCH v5 0/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels Aaron Kling via B4 Relay
@ 2026-04-21 17:38 ` Aaron Kling via B4 Relay
2026-04-22 7:05 ` Krzysztof Kozlowski
2026-04-22 22:16 ` Claude review: " Claude Code Review Bot
2026-04-21 17:38 ` [PATCH v5 2/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels Aaron Kling via B4 Relay
2026-04-22 22:16 ` Claude Code Review Bot
2 siblings, 2 replies; 16+ messages in thread
From: Aaron Kling via B4 Relay @ 2026-04-21 17:38 UTC (permalink / raw)
To: Neil Armstrong, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-kernel, Teguh Sobirin, Aaron Kling
From: Aaron Kling <webgeek1234@gmail.com>
The Chip Wealth Technology CH13726A AMOLED driver is a single chip
solution for MIPI-DSI. This is used for the AYN Thor bottom panel.
Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
.../display/panel/chipwealth,ch13726a.yaml | 67 ++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/panel/chipwealth,ch13726a.yaml b/Documentation/devicetree/bindings/display/panel/chipwealth,ch13726a.yaml
new file mode 100644
index 00000000000000..c8d7b61037e622
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/chipwealth,ch13726a.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/chipwealth,ch13726a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Chip Wealth Technology CH13726A AMOLED driver
+
+maintainers:
+ - Neil Armstrong <neil.armstrong@linaro.org>
+
+description:
+ Chip Wealth Technology CH13726A is a single-chip solution
+ for AMOLED connected using a MIPI-DSI video interface.
+
+allOf:
+ - $ref: panel-common.yaml#
+
+properties:
+ compatible:
+ items:
+ - const: ayntec,thor-panel-bottom
+ - const: chipwealth,ch13726a
+
+ reg:
+ maxItems: 1
+ description: DSI virtual channel
+
+ vdd-supply: true
+ vddio-supply: true
+ vdd1v2-supply: true
+ avdd-supply: true
+
+ port: true
+ reset-gpios: true
+ rotation: true
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+ - vddio-supply
+ - vdd1v2-supply
+ - avdd-supply
+ - reset-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ dsi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ panel@0 {
+ compatible = "ayntec,thor-panel-bottom", "chipwealth,ch13726a";
+ reg = <0>;
+ vdd1v2-supply = <&vreg_l11b_1p2>;
+ vddio-supply = <&vdd_disp_1v8>;
+ vdd-supply = <&vreg_l13b_3p0>;
+ avdd-supply = <&vdd_disp2_2v8>;
+ reset-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>;
+ };
+ };
+
+...
--
2.53.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v5 2/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-04-21 17:38 [PATCH v5 0/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels Aaron Kling via B4 Relay
2026-04-21 17:38 ` [PATCH v5 1/2] dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver Aaron Kling via B4 Relay
@ 2026-04-21 17:38 ` Aaron Kling via B4 Relay
2026-04-22 22:16 ` Claude review: " Claude Code Review Bot
2026-04-22 22:16 ` Claude Code Review Bot
2 siblings, 1 reply; 16+ messages in thread
From: Aaron Kling via B4 Relay @ 2026-04-21 17:38 UTC (permalink / raw)
To: Neil Armstrong, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-kernel, Teguh Sobirin, Aaron Kling
From: Teguh Sobirin <teguh@sobir.in>
This is used by the AYN Thor for the bottom panel.
Signed-off-by: Teguh Sobirin <teguh@sobir.in>
Co-developed-by: Aaron Kling <webgeek1234@gmail.com>
Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
drivers/gpu/drm/panel/Kconfig | 11 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-chipwealth-ch13726a.c | 333 ++++++++++++++++++++++
3 files changed, 345 insertions(+)
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index d6863b28ddc559..e2c00f08f4507d 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -105,6 +105,17 @@ config DRM_PANEL_BOE_TV101WUM_LL2
Say Y here if you want to support for BOE TV101WUM-LL2
WUXGA PANEL DSI Video Mode panel
+config DRM_PANEL_CHIPWEALTH_CH13726A
+ tristate "CHIPWEALTH CH13726A-based DSI panel"
+ depends on OF
+ depends on DRM_MIPI_DSI
+ depends on BACKLIGHT_CLASS_DEVICE
+ select DRM_DISPLAY_DP_HELPER
+ select DRM_DISPLAY_HELPER
+ help
+ Say Y here if you want to enable support for ChipWealth
+ CH13726A-based display panels.
+
config DRM_PANEL_EBBG_FT8719
tristate "EBBG FT8719 panel driver"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index a4291dc3905bed..343d283d1620fb 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_DRM_PANEL_BOE_TD4320) += panel-boe-td4320.o
obj-$(CONFIG_DRM_PANEL_BOE_TH101MB31UIG002_28A) += panel-boe-th101mb31ig002-28a.o
obj-$(CONFIG_DRM_PANEL_BOE_TV101WUM_LL2) += panel-boe-tv101wum-ll2.o
obj-$(CONFIG_DRM_PANEL_BOE_TV101WUM_NL6) += panel-boe-tv101wum-nl6.o
+obj-$(CONFIG_DRM_PANEL_CHIPWEALTH_CH13726A) += panel-chipwealth-ch13726a.o
obj-$(CONFIG_DRM_PANEL_DSI_CM) += panel-dsi-cm.o
obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
diff --git a/drivers/gpu/drm/panel/panel-chipwealth-ch13726a.c b/drivers/gpu/drm/panel/panel-chipwealth-ch13726a.c
new file mode 100644
index 00000000000000..175f40e752126f
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-chipwealth-ch13726a.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ChipWealth CH13726A MIPI-DSI panel driver
+ * Copyright (c) 2024, Teguh Sobirin <teguh@sobir.in>.
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+#include <video/mipi_display.h>
+
+static const struct regulator_bulk_data ch13726a_supplies[] = {
+ { .supply = "vdd1v2", },
+ { .supply = "vddio", },
+ { .supply = "vdd", },
+ { .supply = "avdd", },
+};
+
+struct ch13726a_panel {
+ struct drm_panel panel;
+ struct mipi_dsi_device *dsi;
+ struct regulator_bulk_data *supplies;
+ struct gpio_desc *reset_gpio;
+ struct ch13726a_desc *desc;
+ enum drm_panel_orientation orientation;
+};
+
+struct ch13726a_desc {
+ unsigned int width_mm;
+ unsigned int height_mm;
+ unsigned int bpc;
+
+ const struct drm_display_mode *modes;
+ unsigned int num_modes;
+};
+
+static inline struct ch13726a_panel *to_ch13726a_panel(struct drm_panel *panel)
+{
+ return container_of(panel, struct ch13726a_panel, panel);
+}
+
+static void ch13726a_reset(struct ch13726a_panel *ctx)
+{
+ gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+ usleep_range(10000, 11000);
+ gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+ usleep_range(10000, 11000);
+ gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+ usleep_range(10000, 11000);
+}
+
+static int ch13726a_on(struct ch13726a_panel *ctx)
+{
+ struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
+
+ ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+ mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xf0, 0x50);
+ mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb9, 0x00);
+
+ mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
+
+ mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
+
+ return dsi_ctx.accum_err;
+}
+
+static int ch13726a_disable(struct drm_panel *panel)
+{
+ struct ch13726a_panel *ctx = to_ch13726a_panel(panel);
+ struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
+
+ ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+ mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+ mipi_dsi_msleep(&dsi_ctx, 50);
+ mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
+
+ return dsi_ctx.accum_err;
+}
+
+static int ch13726a_prepare(struct drm_panel *panel)
+{
+ struct ch13726a_panel *ctx = to_ch13726a_panel(panel);
+ struct device *dev = &ctx->dsi->dev;
+ int ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(ch13726a_supplies), ctx->supplies);
+ if (ret < 0) {
+ dev_err(dev, "Failed to enable regulators: %d\n", ret);
+ return ret;
+ }
+
+ ch13726a_reset(ctx);
+
+ ret = ch13726a_on(ctx);
+ if (ret < 0) {
+ dev_err(dev, "Failed to initialize panel: %d\n", ret);
+ gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+ regulator_bulk_disable(ARRAY_SIZE(ch13726a_supplies), ctx->supplies);
+ return ret;
+ }
+
+ msleep(28);
+
+ return 0;
+}
+
+static int ch13726a_unprepare(struct drm_panel *panel)
+{
+ struct ch13726a_panel *ctx = to_ch13726a_panel(panel);
+
+ gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+ regulator_bulk_disable(ARRAY_SIZE(ch13726a_supplies), ctx->supplies);
+
+ return 0;
+}
+
+static const struct drm_display_mode thor_bottom_modes[] = {
+ {
+ /* 120Hz */
+ .clock = (1080 + 28 + 4 + 36) * (1240 + 16 + 4 + 8) * 120 / 1000,
+ .hdisplay = 1080,
+ .hsync_start = 1080 + 28,
+ .hsync_end = 1080 + 28 + 4,
+ .htotal = 1080 + 28 + 4 + 36,
+ .vdisplay = 1240,
+ .vsync_start = 1240 + 16,
+ .vsync_end = 1240 + 16 + 4,
+ .vtotal = 1240 + 16 + 4 + 8,
+ },
+ {
+ /* 60Hz */
+ .clock = (1080 + 28 + 4 + 36) * (1240 + 16 + 4 + 8) * 60 / 1000,
+ .hdisplay = 1080,
+ .hsync_start = 1080 + 28,
+ .hsync_end = 1080 + 28 + 4,
+ .htotal = 1080 + 28 + 4 + 36,
+ .vdisplay = 1240,
+ .vsync_start = 1240 + 16,
+ .vsync_end = 1240 + 16 + 4,
+ .vtotal = 1240 + 16 + 4 + 8,
+ }
+};
+
+static struct ch13726a_desc thor_bottom_desc = {
+ .modes = thor_bottom_modes,
+ .num_modes = ARRAY_SIZE(thor_bottom_modes),
+ .width_mm = 65,
+ .height_mm = 75,
+ .bpc = 8,
+};
+
+static int ch13726a_get_modes(struct drm_panel *panel,
+ struct drm_connector *connector)
+{
+ struct ch13726a_panel *ctx = to_ch13726a_panel(panel);
+
+ for (uint8_t i = 0; i < ctx->desc->num_modes; i++) {
+ const struct drm_display_mode *m = &ctx->desc->modes[i];
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_duplicate(connector->dev, m);
+ if (!mode) {
+ dev_err(&ctx->dsi->dev, "failed to add mode %ux%u@%u\n",
+ m->hdisplay, m->vdisplay, drm_mode_vrefresh(m));
+ return -ENOMEM;
+ }
+
+ mode->type = DRM_MODE_TYPE_DRIVER;
+ if (i == 0)
+ mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+ drm_mode_set_name(mode);
+ drm_mode_probed_add(connector, mode);
+ }
+
+ connector->display_info.width_mm = ctx->desc->width_mm;
+ connector->display_info.height_mm = ctx->desc->height_mm;
+ connector->display_info.bpc = ctx->desc->bpc;
+
+ return ctx->desc->num_modes;
+}
+
+static enum drm_panel_orientation ch13726a_get_orientation(struct drm_panel *panel)
+{
+ struct ch13726a_panel *ctx = to_ch13726a_panel(panel);
+
+ return ctx->orientation;
+}
+
+static const struct drm_panel_funcs ch13726a_panel_funcs = {
+ .prepare = ch13726a_prepare,
+ .unprepare = ch13726a_unprepare,
+ .disable = ch13726a_disable,
+ .get_modes = ch13726a_get_modes,
+ .get_orientation = ch13726a_get_orientation,
+};
+
+static int ch13726a_bl_update_status(struct backlight_device *bl)
+{
+ struct mipi_dsi_device *dsi = bl_get_data(bl);
+ u16 brightness = backlight_get_brightness(bl);
+ int ret;
+
+ dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
+ if (ret < 0)
+ return ret;
+
+ dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+ return 0;
+}
+
+static const struct backlight_ops ch13726a_bl_ops = {
+ .update_status = ch13726a_bl_update_status,
+};
+
+static struct backlight_device *
+ch13726a_create_backlight(struct mipi_dsi_device *dsi)
+{
+ struct device *dev = &dsi->dev;
+ const struct backlight_properties props = {
+ .type = BACKLIGHT_RAW,
+ .brightness = 255,
+ .max_brightness = 255,
+ };
+
+ return devm_backlight_device_register(dev, dev_name(dev), dev, dsi,
+ &ch13726a_bl_ops, &props);
+}
+
+static int ch13726a_probe(struct mipi_dsi_device *dsi)
+{
+ struct device *dev = &dsi->dev;
+ struct ch13726a_panel *ctx;
+ int ret;
+
+ ctx = devm_drm_panel_alloc(dev, __typeof(*ctx), panel,
+ &ch13726a_panel_funcs,
+ DRM_MODE_CONNECTOR_DSI);
+ if (IS_ERR(ctx))
+ return PTR_ERR(ctx);
+
+ ctx->desc = (struct ch13726a_desc *)of_device_get_match_data(dev);
+ if (!ctx->desc)
+ return -ENODEV;
+
+ ret = devm_regulator_bulk_get_const(dev,
+ ARRAY_SIZE(ch13726a_supplies),
+ ch13726a_supplies,
+ &ctx->supplies);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to get regulators\n");
+
+ ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(ctx->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
+ "Failed to get reset-gpios\n");
+
+ ret = of_drm_get_panel_orientation(dev->of_node, &ctx->orientation);
+ if (ret < 0) {
+ dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, ret);
+ return ret;
+ }
+
+ ctx->dsi = dsi;
+ mipi_dsi_set_drvdata(dsi, ctx);
+
+ dsi->lanes = 4;
+ dsi->format = MIPI_DSI_FMT_RGB888;
+ dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
+ MIPI_DSI_CLOCK_NON_CONTINUOUS;
+
+ ctx->panel.prepare_prev_first = true;
+
+ ctx->panel.backlight = ch13726a_create_backlight(dsi);
+ if (IS_ERR(ctx->panel.backlight))
+ return dev_err_probe(dev, PTR_ERR(ctx->panel.backlight),
+ "Failed to create backlight\n");
+
+ drm_panel_add(&ctx->panel);
+
+ ret = mipi_dsi_attach(dsi);
+ if (ret < 0) {
+ dev_err(dev, "Failed to attach to DSI host: %d\n", ret);
+ drm_panel_remove(&ctx->panel);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void ch13726a_remove(struct mipi_dsi_device *dsi)
+{
+ struct ch13726a_panel *ctx = mipi_dsi_get_drvdata(dsi);
+ int ret;
+
+ ret = mipi_dsi_detach(dsi);
+ if (ret < 0)
+ dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
+
+ drm_panel_remove(&ctx->panel);
+}
+
+static const struct of_device_id ch13726a_of_match[] = {
+ { .compatible = "ayntec,thor-panel-bottom", .data = &thor_bottom_desc },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ch13726a_of_match);
+
+static struct mipi_dsi_driver ch13726a_driver = {
+ .probe = ch13726a_probe,
+ .remove = ch13726a_remove,
+ .driver = {
+ .name = "panel-ch13726a-amoled",
+ .of_match_table = ch13726a_of_match,
+ },
+};
+module_mipi_dsi_driver(ch13726a_driver);
+
+MODULE_DESCRIPTION("DRM driver for CH13726A DSI panels");
+MODULE_LICENSE("GPL");
--
2.53.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver
2026-04-21 17:38 ` [PATCH v5 1/2] dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver Aaron Kling via B4 Relay
@ 2026-04-22 7:05 ` Krzysztof Kozlowski
2026-04-22 7:36 ` Aaron Kling
2026-04-22 22:16 ` Claude review: " Claude Code Review Bot
1 sibling, 1 reply; 16+ messages in thread
From: Krzysztof Kozlowski @ 2026-04-22 7:05 UTC (permalink / raw)
To: Aaron Kling
Cc: Neil Armstrong, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
linux-kernel, Teguh Sobirin
On Tue, Apr 21, 2026 at 12:38:37PM -0500, Aaron Kling wrote:
> The Chip Wealth Technology CH13726A AMOLED driver is a single chip
> solution for MIPI-DSI. This is used for the AYN Thor bottom panel.
>
> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> ---
> .../display/panel/chipwealth,ch13726a.yaml | 67 ++++++++++++++++++++++
> 1 file changed, 67 insertions(+)
>
I don't get it, you use b4 but still drop the tags (b4 gets them
automatically). Nothing explains reasons for dropping the tag, so I skip
the patch.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver
2026-04-22 7:05 ` Krzysztof Kozlowski
@ 2026-04-22 7:36 ` Aaron Kling
0 siblings, 0 replies; 16+ messages in thread
From: Aaron Kling @ 2026-04-22 7:36 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Neil Armstrong, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
linux-kernel, Teguh Sobirin
On Wed, Apr 22, 2026 at 2:05 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, Apr 21, 2026 at 12:38:37PM -0500, Aaron Kling wrote:
> > The Chip Wealth Technology CH13726A AMOLED driver is a single chip
> > solution for MIPI-DSI. This is used for the AYN Thor bottom panel.
> >
> > Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > ---
> > .../display/panel/chipwealth,ch13726a.yaml | 67 ++++++++++++++++++++++
> > 1 file changed, 67 insertions(+)
> >
>
> I don't get it, you use b4 but still drop the tags (b4 gets them
> automatically). Nothing explains reasons for dropping the tag, so I skip
> the patch.
It's not automatic, you have to run b4 trailers, which I forgot to do.
Will send another revision after running that.
Aaron
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-04-22 7:43 [PATCH v6 0/2] " Aaron Kling via B4 Relay
2026-04-22 7:43 ` [PATCH v6 2/2] " Aaron Kling via B4 Relay
@ 2026-04-22 22:00 ` Claude Code Review Bot
1 sibling, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 22:00 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/panel: Add panel driver for ChipWealth CH13726A based panels
Author: Aaron Kling via B4 Relay <devnull+webgeek1234.gmail.com@kernel.org>
Patches: 4
Reviewed: 2026-04-23T08:00:14.799825
---
This is a clean, straightforward 2-patch series adding a new MIPI-DSI panel driver for the ChipWealth CH13726A AMOLED controller, used in the AYN Thor handheld's bottom panel. The DT binding (patch 1) already has a Reviewed-by from Krzysztof Kozlowski, which is good. The driver (patch 2) follows modern patterns (`devm_drm_panel_alloc`, `devm_regulator_bulk_get_const`, `mipi_dsi_multi_context`) and is relatively small. This is v6, and has been through several rounds of review already.
There are a few issues remaining, one of which is a real bug (incorrect Kconfig selects pulling in DisplayPort helpers for a DSI panel), and several minor style/correctness nits.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-04-22 7:43 ` [PATCH v6 2/2] " Aaron Kling via B4 Relay
@ 2026-04-22 22:00 ` Claude Code Review Bot
0 siblings, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 22:00 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Issue 1 (bug): Incorrect Kconfig selects**
```
+config DRM_PANEL_CHIPWEALTH_CH13726A
+ tristate "CHIPWEALTH CH13726A-based DSI panel"
+ depends on OF
+ depends on DRM_MIPI_DSI
+ depends on BACKLIGHT_CLASS_DEVICE
+ select DRM_DISPLAY_DP_HELPER
+ select DRM_DISPLAY_HELPER
```
`DRM_DISPLAY_DP_HELPER` is for DisplayPort/eDP panels only. In the entire panel Kconfig, the only entries selecting it are `DRM_PANEL_SAMSUNG_ATNA33XC20` (an eDP panel) and `DRM_PANEL_EDP` (the generic eDP panel driver). This DSI panel driver does not use any DP or display helpers — it only uses `drm_mipi_dsi.h`, `drm_panel.h`, and `drm_modes.h`. Both `select` lines should be removed.
**Issue 2 (minor): `thor_bottom_desc` should be `const`**
```c
+static struct ch13726a_desc thor_bottom_desc = {
```
This should be `static const struct ch13726a_desc thor_bottom_desc`. The data is never modified at runtime, and the `of_match_table` `.data` field is `const void *`. This also ties into Issue 3 below.
**Issue 3 (minor): const-correctness for `desc` field**
```c
+struct ch13726a_panel {
+ ...
+ struct ch13726a_desc *desc;
+ ...
+};
```
The `desc` pointer should be `const struct ch13726a_desc *desc`, since the descriptor data pointed to (from the match table) should never be modified. The assignment in probe:
```c
+ ctx->desc = (struct ch13726a_desc *)of_device_get_match_data(dev);
```
currently casts away the `const` returned by `of_device_get_match_data()` (which returns `const void *`). With a `const` field, the explicit cast would be unnecessary:
```c
ctx->desc = of_device_get_match_data(dev);
```
**Issue 4 (style): `uint8_t` loop variable**
```c
+ for (uint8_t i = 0; i < ctx->desc->num_modes; i++) {
```
The kernel convention is to use `unsigned int` for loop counters, not fixed-width types like `uint8_t`. A `uint8_t` counter is also fragile — it would silently wrap if `num_modes` ever exceeded 255, though that's admittedly unlikely for display modes.
**Issue 5 (nit): `of_device_get_match_data` vs `device_get_match_data`**
```c
+ ctx->desc = (struct ch13726a_desc *)of_device_get_match_data(dev);
```
Newer panel drivers are moving to the bus-agnostic `device_get_match_data(dev)`. While `of_device_get_match_data` still works, reviewers may prefer the newer API since the driver already `depends on OF` anyway and the function is equivalent for OF-based matching.
**Issue 6 (observation): LPM toggling in disable**
```c
+static int ch13726a_disable(struct drm_panel *panel)
+{
+ ...
+ ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+ mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+ mipi_dsi_msleep(&dsi_ctx, 50);
+ mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
```
The disable path clears `MIPI_DSI_MODE_LPM` before sending DCS commands, meaning display-off and enter-sleep are sent in HS mode. Meanwhile `ch13726a_on()` sets `MIPI_DSI_MODE_LPM` to send init commands in LP mode. This asymmetry is present in some other drivers (e.g., `panel-jdi-fhd-r63452.c`), so it may be correct per the panel datasheet, but it's worth confirming that the CH13726A actually requires HS-mode for shutdown commands. If LP mode works for both init and shutdown, the mode_flags manipulation could be simplified.
**Non-issues / things done well:**
- Correct use of `devm_drm_panel_alloc()` (modern allocation pattern)
- Correct use of `devm_regulator_bulk_get_const()` with a static const supply array
- Proper use of `mipi_dsi_multi_context` for init sequence error accumulation
- `prepare_prev_first = true` is appropriate for a DSI video mode panel
- The 120Hz/60Hz mode clock calculations look correct: `(1080+28+4+36) * (1240+16+4+8) * rate / 1000` = `1148 * 1268 * 120 / 1000` = 174,614 for 120Hz
- Backlight via DCS brightness is standard
- Error cleanup paths in prepare and probe are correct
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-04-21 17:38 [PATCH v5 0/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels Aaron Kling via B4 Relay
2026-04-21 17:38 ` [PATCH v5 1/2] dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver Aaron Kling via B4 Relay
2026-04-21 17:38 ` [PATCH v5 2/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels Aaron Kling via B4 Relay
@ 2026-04-22 22:16 ` Claude Code Review Bot
2 siblings, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 22:16 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/panel: Add panel driver for ChipWealth CH13726A based panels
Author: Aaron Kling via B4 Relay <devnull+webgeek1234.gmail.com@kernel.org>
Patches: 5
Reviewed: 2026-04-23T08:16:54.090766
---
This is a clean, straightforward 2-patch series adding a new MIPI-DSI panel driver for the ChipWealth CH13726A AMOLED, used as the bottom panel in the AYN Thor handheld. The series is at v5 and has addressed previous review feedback (using `devm_drm_panel_alloc()`, `devm_regulator_bulk_get_const()`, removing duplicated prepared tracking). The dt-bindings patch is well-structured. The driver follows modern panel driver patterns. There are a few minor issues worth addressing.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver
2026-04-21 17:38 ` [PATCH v5 1/2] dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver Aaron Kling via B4 Relay
2026-04-22 7:05 ` Krzysztof Kozlowski
@ 2026-04-22 22:16 ` Claude Code Review Bot
1 sibling, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 22:16 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This binding looks correct and follows the standard panel-common pattern.
**Minor nit: missing `port` in the example.** The binding declares `port: true` as an optional property, which is fine. The example doesn't use it, which is acceptable since it's not required.
**Looks good overall.** The compatible uses the proper two-entry pattern with board-specific first and chip-level fallback. Supplies, reset-gpios, and reg are all properly required.
No issues found with this patch.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-04-21 17:38 ` [PATCH v5 2/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels Aaron Kling via B4 Relay
@ 2026-04-22 22:16 ` Claude Code Review Bot
0 siblings, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 22:16 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Issue 1 (minor): Unnecessary Kconfig selects.**
```c
+ select DRM_DISPLAY_DP_HELPER
+ select DRM_DISPLAY_HELPER
```
This is a MIPI-DSI panel driver. It has no DisplayPort functionality. The only other panel drivers that select `DRM_DISPLAY_DP_HELPER` are eDP panels (Samsung ATNA33XC20 and the eDP panel driver). These selects should be removed.
**Issue 2 (minor): `uint8_t` loop variable in `ch13726a_get_modes`.**
```c
+ for (uint8_t i = 0; i < ctx->desc->num_modes; i++) {
```
No other panel driver in the tree uses `uint8_t` for this kind of loop counter. The kernel convention is `unsigned int` or `int`. While functionally fine here (there will never be >255 modes), it should be `unsigned int i` for consistency. Also, declaring the variable in the `for` initializer is C99/C11 style — while the kernel now permits it, most existing panel drivers declare `i` at function scope. Using `unsigned int` declared in the loop is fine, but `uint8_t` is unusual.
**Issue 3 (minor): `thor_bottom_desc` should be `const`.**
```c
+static struct ch13726a_desc thor_bottom_desc = {
```
This data is never modified. It should be `static const struct ch13726a_desc thor_bottom_desc`. The `.data` pointer in the of_match_table already casts it to `const void *`. Correspondingly, the `desc` member in the panel struct should be `const struct ch13726a_desc *desc`.
**Issue 4 (very minor): Cast in `of_device_get_match_data`.**
```c
+ ctx->desc = (struct ch13726a_desc *)of_device_get_match_data(dev);
```
If the `desc` field is changed to `const struct ch13726a_desc *` per above, the cast can become `(const struct ch13726a_desc *)` or be dropped entirely since `of_device_get_match_data` returns `const void *`.
**Issue 5 (observation): Reset GPIO polarity.**
```c
+ gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+ usleep_range(10000, 11000);
+ gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+ usleep_range(10000, 11000);
+ gpiod_set_value_cansleep(ctx->reset_gpio, 1);
```
The GPIO is obtained with `GPIOD_OUT_LOW` (deasserted at probe), and the DT example uses `GPIO_ACTIVE_HIGH`. The reset sequence asserts (1), deasserts (0), then asserts (1) again — this means the panel enters reset, exits reset, and then enters reset once more as the "end" state of the reset function, staying in reset. But `ch13726a_prepare` calls `ch13726a_reset()` and then proceeds to `ch13726a_on()` which sends DSI commands. If the panel is still held in reset when DSI commands are sent, this may not work. Since the driver appears to be tested and working (v5), perhaps the logic value interpretation is inverted from the typical convention — the author should confirm that the reset sequence is intentional and correct. Typically reset functions end with the reset line deasserted (panel out of reset).
**Issue 6 (minor): `ch13726a_disable` clears LPM but `ch13726a_on` sets it.**
```c
+static int ch13726a_disable(struct drm_panel *panel)
+{
+ ...
+ ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
```
In `ch13726a_on`:
```c
+ ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
```
The disable path clears LPM mode, sends display-off and sleep commands without LPM. The backlight update callback also clears LPM, sends brightness, then sets it again. The on path sets LPM and sends initialization commands in LPM. This is internally consistent, but the disable path never restores LPM — so if there's a disable/re-prepare cycle, `ch13726a_on` will set it again which is fine. No bug, just worth noting.
**Summary of requested changes:**
1. Remove the `DRM_DISPLAY_DP_HELPER` and `DRM_DISPLAY_HELPER` selects from Kconfig — they are not needed for a DSI panel driver.
2. Change `uint8_t i` to `unsigned int i` in `ch13726a_get_modes`.
3. Make `thor_bottom_desc` and the `desc` struct member `const`.
4. Please confirm the reset sequence is correct (panel left in reset state before DSI init commands are sent).
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-04-27 4:39 [PATCH v7 0/2] " Aaron Kling via B4 Relay
2026-04-27 4:39 ` [PATCH v7 2/2] " Aaron Kling via B4 Relay
@ 2026-04-28 5:10 ` Claude Code Review Bot
1 sibling, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-04-28 5:10 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/panel: Add panel driver for ChipWealth CH13726A based panels
Author: Aaron Kling via B4 Relay <devnull+webgeek1234.gmail.com@kernel.org>
Patches: 3
Reviewed: 2026-04-28T15:10:45.632908
---
This is a clean, well-structured v7 series adding a MIPI-DSI panel driver for the ChipWealth CH13726A AMOLED controller, used as the bottom panel in the AYN Thor handheld. The series is at a good level of maturity — it already carries Reviewed-by tags from DT and DRM reviewers, and uses modern APIs (`devm_drm_panel_alloc`, `devm_regulator_bulk_get_const`, `mipi_dsi_multi_context`). The issues found are minor.
The main substantive issue is the Kconfig selecting DP helpers that a MIPI-DSI panel doesn't need. The remaining items are const-correctness and minor style.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels
2026-04-27 4:39 ` [PATCH v7 2/2] " Aaron Kling via B4 Relay
@ 2026-04-28 5:10 ` Claude Code Review Bot
0 siblings, 0 replies; 16+ messages in thread
From: Claude Code Review Bot @ 2026-04-28 5:10 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Issue 1 (Kconfig): Unnecessary DP helper selections**
```c
select DRM_DISPLAY_DP_HELPER
select DRM_DISPLAY_HELPER
```
This is a pure MIPI-DSI panel. The only other panel drivers that `select DRM_DISPLAY_DP_HELPER` are eDP panels (Samsung ATNA33XC20, simple eDP). Comparable MIPI-DSI panel drivers like `DRM_PANEL_BOE_TD4320` select neither of these. Both `select` lines should be removed — the driver doesn't use any display helper or DP helper APIs.
**Issue 2 (const-correctness): `desc` pointer and `thor_bottom_desc` should be const**
The descriptor struct is declared as:
```c
struct ch13726a_desc *desc;
```
and `thor_bottom_desc` is:
```c
static struct ch13726a_desc thor_bottom_desc = {
```
Since `of_device_get_match_data()` returns `const void *`, the current cast discards const:
```c
ctx->desc = (struct ch13726a_desc *)of_device_get_match_data(dev);
```
The field should be `const struct ch13726a_desc *desc`, the variable should be `static const struct ch13726a_desc thor_bottom_desc`, and the cast should be `(const struct ch13726a_desc *)` (or better, dropped entirely since C allows implicit conversion from `const void *` to `const T *`).
**Issue 3 (style): `uint8_t` loop variable in `get_modes`**
```c
for (uint8_t i = 0; i < ctx->desc->num_modes; i++) {
```
Kernel convention is to use `unsigned int` or `int` for loop counters, not fixed-width types. `unsigned int` would match the type of `num_modes`.
**Issue 4 (style): `__typeof(*ctx)` in `devm_drm_panel_alloc`**
```c
ctx = devm_drm_panel_alloc(dev, __typeof(*ctx), panel,
&ch13726a_panel_funcs,
DRM_MODE_CONNECTOR_DSI);
```
While functionally correct, all other panel drivers pass the struct name directly:
```c
ctx = devm_drm_panel_alloc(dev, struct ch13726a_panel, panel,
&ch13726a_panel_funcs,
DRM_MODE_CONNECTOR_DSI);
```
**Observation (not a blocker): `of_drm_get_panel_orientation` error path**
```c
ret = of_drm_get_panel_orientation(dev->of_node, &ctx->orientation);
if (ret < 0) {
dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, ret);
return ret;
}
```
This could use `dev_err_probe()` for consistency with the other error paths in `probe()`, which would also handle `-EPROBE_DEFER` cleanly:
```c
return dev_err_probe(dev, ret, "Failed to get orientation\n");
```
**Overall for patch 2:** The driver logic itself is solid — the reset sequence, init commands, backlight handling, and multi-rate mode support all look correct. The `mipi_dsi_multi_context` usage is clean, `prepare_prev_first = true` is properly set, and the remove path correctly detaches DSI before removing the panel. The issues above are all minor fixups.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2026-04-28 5:10 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-21 17:38 [PATCH v5 0/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels Aaron Kling via B4 Relay
2026-04-21 17:38 ` [PATCH v5 1/2] dt-bindings: display: panel: Add ChipWealth CH13726A AMOLED driver Aaron Kling via B4 Relay
2026-04-22 7:05 ` Krzysztof Kozlowski
2026-04-22 7:36 ` Aaron Kling
2026-04-22 22:16 ` Claude review: " Claude Code Review Bot
2026-04-21 17:38 ` [PATCH v5 2/2] drm/panel: Add panel driver for ChipWealth CH13726A based panels Aaron Kling via B4 Relay
2026-04-22 22:16 ` Claude review: " Claude Code Review Bot
2026-04-22 22:16 ` Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-04-27 4:39 [PATCH v7 0/2] " Aaron Kling via B4 Relay
2026-04-27 4:39 ` [PATCH v7 2/2] " Aaron Kling via B4 Relay
2026-04-28 5:10 ` Claude review: " Claude Code Review Bot
2026-04-28 5:10 ` Claude Code Review Bot
2026-04-22 7:43 [PATCH v6 0/2] " Aaron Kling via B4 Relay
2026-04-22 7:43 ` [PATCH v6 2/2] " Aaron Kling via B4 Relay
2026-04-22 22:00 ` Claude review: " Claude Code Review Bot
2026-04-22 22:00 ` Claude Code Review Bot
2026-04-08 5:32 [PATCH v4 0/2] " Aaron Kling via B4 Relay
2026-04-08 5:32 ` [PATCH v4 2/2] " Aaron Kling via B4 Relay
2026-04-12 3:10 ` Claude review: " Claude Code Review Bot
2026-04-12 3:10 ` Claude Code Review Bot
2026-03-23 17:08 [PATCH v3 0/2] " Aaron Kling via B4 Relay
2026-03-23 17:08 ` [PATCH v3 2/2] " Aaron Kling via B4 Relay
2026-03-24 21:40 ` Claude review: " Claude Code Review Bot
2026-03-24 21:40 ` Claude Code Review Bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox