From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: ARM: dts: imx6ul-var-som: add support for EC configuration option (ENET1) Date: Tue, 03 Mar 2026 12:58:07 +1000 Message-ID: In-Reply-To: <20260302190953.669325-12-hugo@hugovil.com> References: <20260302190953.669325-1-hugo@hugovil.com> <20260302190953.669325-12-hugo@hugovil.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Issue 1 (Critical, same as patch 9)**: The `pinctrl_enet1_gpio` in `imx6u= ll-var-som.dtsi` uses `MX6ULL_PAD_SNVS_TAMPER0__GPIO5_IO00` under `&iomuxc`= =E2=80=94 this must be `&iomuxc_snvs`. **Issue 2 (Significant)**: There is a duplicate definition of `ethphy0: eth= ernet-phy@1` =E2=80=94 it appears in both `imx6ul-var-som-enet2.dtsi` (adde= d by this patch) and `imx6ul-var-som-enet1.dtsi`. When both are included in= the `-full` boards, the same node is defined twice with the same label at = the same path. While DTS merge semantics allow this (properties get overwri= tten with identical values), it is confusing and error-prone. A cleaner approach would be to have `ethphy0` defined only in `enet1.dtsi` = (since it's the ENET1 PHY), and use the `mdio_enet2` label from enet2.dtsi = to place it. Don't duplicate it in enet2.dtsi. **Issue 3 (Significant)**: The author acknowledges the design problem in th= e `---` notes: > In order for this to work, imx6ul-var-som-enet2.dtsi must be included fir= st, and thus enabled, even if not used. Maybe there is a better way to supp= ort both independently, but I'm not sure how. This forced dependency on enet2.dtsi for enet1 to work is fragile. Consider= defining the shared MDIO bus in the common dtsi or in a separate shared fi= le, rather than making enet1 depend on enet2's internal implementation. **Issue 4**: The `pinctrl_enet1_mdio` group from the original code was drop= ped. The original had: ```c pinctrl-0 =3D <&pinctrl_enet1>, <&pinctrl_enet1_gpio>, <&pinctrl_enet1_mdio= >; ``` The new version has: ```c pinctrl-0 =3D <&pinctrl_enet1>, <&pinctrl_enet1_gpio>; ``` This seems intentional since fec1 now uses fec2's MDIO bus, but should be n= oted in the commit message. --- Generated by Claude Code Patch Reviewer