Re: [PATCH v5 10/10] arm64: dts: Add support for LX2160 Twins board in single configuration

Am 12.05.26 um 00:29 schrieb [email protected]:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Undocumented and unrelated SoC revision change for the Clearfog CX board.
> - [High] The PCI-E OCP card connector is not enabled in the device tree.
> - [Low] Duplicate `function-enumerator` value in the SFP LED definitions.
> - [Low] The `dpmac` nodes referenced as LED `trigger-sources` lack the mandatory `#trigger-source-cells` property.
> - [Low] The `gpio-mux` configured for PWM fan monitoring has no label and no consumer node.
> --
>
> commit e131b341e534ec21c95289bd4bedcef8bb6fcf54
> Author: Josua Mayer <[email protected]>
>
> arm64: dts: Add support for LX2160 Twins board in single configuration
>     
> This commit adds support for the SolidRun LX2160A Twins board in its
> single CPU configuration. The board features several connectors including
> SFP, PCIe OCP, USB-3.0, and microSD.
>
>> - PCI-E OCP card connector
> The commit message explicitly lists the PCI-E OCP card connector as a
> supported feature. However, looking at the device tree, are there any PCIe
> host controller nodes enabled?
>
> All PCIe host controller nodes default to disabled in the base SoC device
> trees, and this device tree doesn't seem to override their status to "okay".
>
>> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-clearfog-cx.dts b/arch/arm64/boot/dts/freescale/fsl-lx2160a-clearfog-cx.dts
>> index 802d7611c6479..6078ce47fabf1 100644
>> --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-clearfog-cx.dts
>> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-clearfog-cx.dts
>> @@ -6,7 +6,7 @@
>>  
>>  /dts-v1/;
>>  
>> -#include "fsl-lx2160a.dtsi"
>> +#include "fsl-lx2160a-rev2.dtsi"
> Does this commit intend to change the SoC revision for the Clearfog CX board?
Unintentional, to be dropped in v6.
>
> This hardware definition change is not documented in the commit message.
>
>> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-half-twins.dts b/arch/arm64/boot/dts/freescale/fsl-lx2160a-half-twins.dts
>> new file mode 100644
>> index 0000000000000..434b3f4873008
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-half-twins.dts
> [ ... ]
>
>> +	leds {
>> +		compatible = "gpio-leds";
> [ ... ]
>
>> +		led_ht_c3_bb: led-sfp-2 {
>> +			color = <LED_COLOR_ID_GREEN>;
>> +			default-state = "off";
>> +			function = LED_FUNCTION_LAN;
>> +			function-enumerator = <2>;
>> +			gpios = <&expander3 13 GPIO_ACTIVE_LOW>;
>> +			trigger-sources = <&dpmac15>;
>> +			linux,default-trigger = "netdev";
>> +		};
>> +
>> +		led_ht_c3_at: led-sfp-3 {
>> +			color = <LED_COLOR_ID_GREEN>;
>> +			default-state = "off";
>> +			function = LED_FUNCTION_LAN;
>> +			function-enumerator = <2>;
> Does this node have a duplicate function-enumerator?
Queued for v6.
>
> It shares the same value as the preceding led_ht_c3_bb (led-sfp-2) node.
>
>> +			gpios = <&expander3 11 GPIO_ACTIVE_LOW>;
>> +			trigger-sources = <&dpmac6>;
> Do the referenced dpmac nodes lack the #trigger-source-cells property?
They do, and possibly they need to keep lacking it.
MACs do not (currently) provide LED triggers, only linux netdevs do.

This does not fail dtbs_check as of v6.1-c1.

>
> According to the standard LED trigger-source binding, any provider node must
> define this property. The base dpmac nodes in fsl-lx2160a.dtsi do not appear
> to define it.
>
> [ ... ]
>
>> +	mux-controller {
>> +		compatible = "gpio-mux";
>> +		#mux-control-cells = <0>;
>> +		/*
>> +		 * This gpio controlled mux can route the tacho signals of 6 PWM FAN connectors
>> +		 * to the tacho inputs of both CEX-7 modules (twins).
>> +		 *
>> +		 * The first twin controls this mux and monitors four fan connectors, two intended
>> +		 * for itself, and two for the OCP card.
>> +		 *
>> +		 * The second twin monitors only two fan connectors intended for itself.
>> +		 *
>> +		 * The table below maps selector GPIO states to monitored fan connector per twin:
>> +		 *
>> +		 * | SEL1 | SEL0 | Twin 1 | Twin 2 |
>> +		 * | ---: | ---: | :------| ------ |
>> +		 * |    0 |    0 | J10    |  J5024 |
>> +		 * |    0 |    1 | J5016  |  J5024 |
>> +		 * |    1 |    0 | J5026  |  J5025 |
>> +		 * |    1 |    1 | J5013  |  J5025 |
>> +		 */
>> +		mux-gpios = <&expander0 8 GPIO_ACTIVE_HIGH>, /* SEL0 */
>> +			    <&expander0 15 GPIO_ACTIVE_HIGH>; /* SEL1 */
>> +	};
> Is this mux-controller missing a label and a consumer node?
>
> There are no hwmon, fan, or other consumer nodes defined that reference it.
This mux is impossibly complex to use from hwmon driver.
Hence there is no consumer.