From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: ili9806e: Add Densitron DMT050WVNMCMI-1A to ILI9806E DSI driver Date: Fri, 13 Mar 2026 14:42:09 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Overall Series Review Subject: ili9806e: Add Densitron DMT050WVNMCMI-1A to ILI9806E DSI driver Author: =3D?iso-8859-1?Q?Bartholom=3DE4us_Steinmayr?=3D Patches: 2 Reviewed: 2026-03-13T14:42:09.867822 --- **Incomplete submission =E2=80=94 only the cover letter (0/2) is present in= the mbox. Patches 1/2 and 2/2 are missing**, so no code changes can be rev= iewed directly. The review below is based on the design described in the co= ver letter and the existing driver code. The series aims to add support for the Densitron DMT050WVNMCMI-1A panel to = the ILI9806E DSI driver. The panel initialization code part is standard and= should be straightforward. However, the proposed mechanism for handling th= e shared reset line with the Goodix GT911 touchscreen controller raises sig= nificant design concerns. **Key design concerns from the cover letter:** 1. **Invented `i2c-frag` DT property**: The cover letter describes adding a= n optional `i2c-frag` devicetree node to the ILI9806E driver that causes it= to defer probing until an I2C device (the Goodix touchscreen) has initiali= zed. This is a non-standard, ad-hoc probe-ordering mechanism. The DT bindin= gs maintainers will very likely reject this =E2=80=94 devicetree should des= cribe hardware, not encode driver probe ordering. The proper kernel mechani= sms for this are: - **`gpio-hog`** or shared GPIO with proper flags - Using the **component framework** or **device links** (`device_link_ad= d`) - Expressing the dependency through standard DT properties like `power-s= upply` phandles or similar that naturally create deferred probe chains - The reset controller subsystem (`reset-controller`) which is specifica= lly designed for shared reset lines 2. **Making `reset-gpios` optional**: The cover letter says the reset GPIO = is made optional so the ILI9806E driver won't touch the shared reset line. = This changes existing behavior for all panels using this driver =E2=80=94 c= urrently `reset-gpios` is required (line 184: `devm_gpiod_get(dev, "reset",= GPIOD_OUT_LOW)`). Making it optional without breaking existing panels need= s careful handling, but more fundamentally, a panel driver that can't contr= ol its own reset is fragile. 3. **Better alternatives exist**: The shared reset line between the GT911 a= nd ILI9806E should ideally be modeled using the **reset controller subsyste= m** (`drivers/reset/`), where a simple GPIO-based reset controller provides= the reset to both consumers. This is exactly the pattern the kernel has fo= r shared reset lines. Alternatively, the GT911 driver already handles the r= eset+I2C-address-selection dance =E2=80=94 the panel driver could simply de= clare the reset GPIO as `GPIOD_ASIS` (not asserting it) if it knows another= driver will handle it. --- Generated by Claude Code Patch Reviewer