Re: [PATCH v6 4/4] arm64: dts: nuvoton: npcm845-evb: Add peripheral nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect type for PWM fan cooling-levels property causes intermediate cooling states to turn fans off.
- [Medium] The `&fiux` node is missing the `status = "okay";` property, so the controller will remain disabled.
- [Medium] The FIU aliases intentionally violate device tree bindings to work around a hardcoded driver array, improperly swapping physical mappings.
- [Medium] The `ipmb@10` and `ipmb@11` nodes lack the `I2C_OWN_SLAVE_ADDRESS` bitmask in their `reg` property.
--

commit cf191e9e7fae85040690a2905adeff456af8e03b
Author: Tomer Maimon <tmaimon77@gmail.com>

arm64: dts: nuvoton: npcm845-evb: Add peripheral nodes

Enable peripheral support for the npcm845 evaluation board by wiring up the
soc controller nodes to the board design. Add board aliases, gpio-backed
mdio buses, reserved-memory, and flash partitions. Enable various peripheral
controllers and interfaces with board-specific pinctrl and phy connections.

> diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts
> index 5edf5d13342da..1092dc5bb7aba 100644
> --- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts
> +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts

[ ... ]

> @@ -10,6 +10,42 @@ / {
>  
>  	aliases {
>  		serial0 = &serial0;
> +		ethernet1 = &gmac1;
> +		ethernet2 = &gmac2;
> +		ethernet3 = &gmac3;
> +		mdio-gpio0 = &mdio0;
> +		mdio-gpio1 = &mdio1;
> +		fiu0 = &fiu0;
> +		fiu1 = &fiu3;
> +		fiu2 = &fiux;
> +		fiu3 = &fiu1;

[Severity: Medium]
Do these aliases intentionally violate the device tree bindings for the soc?

The device tree bindings explicitly map the fiu1 node to the fiu1 hardware,
fiu2 to fiu3, and fiu3 to fiux.

Mapping fiu1 to &fiu3, fiu2 to &fiux, and fiu3 to &fiu1 appears to swap
the physical mappings to match the hardcoded npxm8xx_fiu_info array in
spi-npcm-fiu.c.

Should the driver be fixed instead of working around it by conflicting with
the physical hardware intent here?

[ ... ]

> +&fiux {
> +	nuvoton,spix-mode;
> +};

[Severity: Medium]
Will the fiux controller remain disabled without setting its status?

The commit message mentions enabling the controllers, but the base soc
dtsi file defaults the fiux node to status = "disabled".

Is a status = "okay" property needed here to ensure the kernel actually
probes it?

[ ... ]

> +&pwm_fan {
> +	status = "okay";
> +	pinctrl-names = "default";

[ ... ]

> +	fan@0 {
> +		reg = <0x00>;
> +		fan-tach-ch = /bits/ 8 <0x0 0x1>;
> +		cooling-levels = <127 255>;
> +	};

[Severity: High]
Does this cooling-levels definition cause intermediate thermal states to
turn the fans completely off?

The npcm750-pwm-fan.c driver parses cooling-levels strictly as an array of
8-bit unsigned integers via of_property_read_u8_array(). Because the
/bits/ 8 directive is missing, the device tree compiler generates two
32-bit big-endian integers (00 00 00 7f 00 00 00 ff).

When the driver counts the elements using of_property_count_u8_elems(), it
will find 8 states instead of 2. The parsed levels become:
[0, 0, 0, 127, 0, 0, 0, 255]

Could this result in a 0% duty cycle when requesting intermediate states
like state 0, 1, or 2, risking hardware overheating? Should this be written
as cooling-levels = /bits/ 8 <127 255>; instead?

[ ... ]

> +&i2c2 {
> +	status = "okay";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	ipmb@10 {
> +		compatible = "ipmb-dev";
> +		reg = <0x10>;
> +		i2c-protocol;
> +	};
> +};
> +
> +&i2c3 {
> +	status = "okay";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	ipmb@11 {
> +		compatible = "ipmb-dev";
> +		reg = <0x11>;
> +		i2c-protocol;
> +	};
> +};

[Severity: Medium]
Are the I2C_OWN_SLAVE_ADDRESS bitmasks missing from these reg properties?

According to the ipmb-dev device tree bindings, ipmb devices must operate
as i2c slaves. Passing a raw address like reg = <0x10> tells the i2c core
to treat it as a master-side client target.

This can trigger a warning in i2c_slave_register() and might cause address
collisions or break reception. Should this be defined as
reg = <(0x10 | I2C_OWN_SLAVE_ADDRESS)>; to instruct the underlying
controller to listen in slave mode?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617133212.287768-1-tmaimon77@gmail.com?part=4