Re: [PATCH RFC 4/4] arm64: dts: freescale: Add chargebyte Charge Control Y

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The LAN8651 ethernet device is missing its hardware reset configuration, leaving the `X_nRST_LAN` pin unmanaged.
- [Medium] The SD card slot power/reset pin (`SD2_nRESET`) is named in `gpio-line-names` but is not associated with any regulator or reset control.
- [Low] The GPIO label `X_LAN_nCS` is incorrectly placed at index 0 of `&gpio2`'s `gpio-line-names` array, but the `lan8651` chip select is actually wired to index 8.
- [Low] The `ti,leds-polarity` property used in both Ethernet PHY nodes is undocumented, invalid, and will cause `dtbs_check` validation errors.
--

--- Patch [4]: [PATCH RFC 4/4] arm64: dts: freescale: Add chargebyte Charge Control Y ---
commit a177a5e0acba62156876ea6dc84457789786bfb8
Author: Stefan Wahren <[email protected]>

arm64: dts: freescale: Add chargebyte Charge Control Y

This commit adds support for the chargebyte Charge Control Y, an EVSE
for MCS charging stations based on the phyCORE-i.MX 93 ARM SoM. It
adds the devicetree for the board, including ethernet interfaces, CAN,
SPI, and SD card configurations.

> diff --git a/arch/arm64/boot/dts/freescale/imx93-charge-control-y.dts b/arch/arm64/boot/dts/freescale/imx93-charge-control-y.dts
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx93-charge-control-y.dts
> @@ -0,0 +1,387 @@
[ ... ]
> +/* Ethernet 1 PHY */
> +&ethphy1 {
> +	ti,leds-polarity = <1>; // active-high
> +};

[Severity: Low]
Is ti,leds-polarity supported by the devicetree bindings? It does not
appear to be documented in the schema and might cause dtbs_check validation
errors.

[ ... ]
> +&gpio2 {
> +	gpio-line-names = "X_LAN_nCS", "", "", "", "",
> +			  "", "", "X_LAN_nIRQ", "", "",

[Severity: Low]
Should the X_LAN_nCS label be placed at index 8 instead of index 0? The
lpspi3 node below configures the cs-gpios using index 8 on this bank.

[ ... ]
> +/* MCS Ethernet SPI */
> +&lpspi3 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_lpspi3>;
> +	num-cs = <1>;
> +	cs-gpios = <&gpio2 8 GPIO_ACTIVE_LOW>;
> +	status = "okay";
> +
> +	lan8651: ethernet@0 {
> +		compatible = "microchip,lan8651", "microchip,lan8650";
> +		reg = <0>;
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> +		spi-max-frequency = <25000000>;
> +
> +		nvmem-cells = <&eth2_mac_address 0>;
> +		nvmem-cell-names = "mac-address";
> +	};
> +};

[Severity: Medium]
The pin X_nRST_LAN is labeled in the gpio1 node, but this ethernet node
doesn't seem to configure a reset mechanism. Does the LAN8651 need a reset
control or gpio-hog to ensure the pin is driven and not left floating?

[ ... ]
> +/* Ethernet 2 PHY */
> +&mdio {
> +	ethphy2: ethernet-phy@2 {
> +		reg = <2>;
> +		compatible = "ethernet-phy-id2000.a140";
> +		clocks = <&clk IMX93_CLK_ENET_REF_PHY>;
> +		reset-gpios = <&gpio4 13 GPIO_ACTIVE_HIGH>;
> +		reset-assert-us = <30>;
> +		reset-deassert-us = <50000>;
> +		ti,leds-polarity = <1>; // active-high
> +	};
> +};

[Severity: Low]
Similar to the ethernet 1 PHY above, is this ti,leds-polarity property
supported by the devicetree bindings?

> +
> +/* SD */
> +&usdhc2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_usdhc2_default>, <&pinctrl_usdhc2_cd>;
> +	cd-gpios = <&gpio3 00 GPIO_ACTIVE_LOW>;
> +	bus-width = <4>;
> +	disable-wp;
> +	no-sdio;
> +	no-mmc;
> +	no-1-8-v;
> +	status = "okay";
> +};

[Severity: Medium]
The SD2_nRESET pin is labeled in the gpio3 node, but does this usdhc2 node
need a vmmc-supply using that pin as a fixed regulator to ensure the SD
card slot is reliably powered?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy