Re: [PATCH v3 3/3] arm64: dts: nuvoton: Add I2C nodes for MA35D1 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The I2C0 controller node is entirely missing from the SoC device tree.
- [Medium] Dangling device alias pointing to a disabled node while the active node is left unaliased.
- [Medium] Meaningful mismatch between commit message and implementation.
--

commit 872a73a723152a79752769d63796c54985979c16
Author: Zi-Yu Chen <zychennvt@gmail.com>

arm64: dts: nuvoton: Add I2C nodes for MA35D1 SoC

This commit adds I2C controller nodes to the MA35D1 SoC device tree. It
also enables an I2C interface on the MA35D1 SOM board and sets up the
corresponding pin control configurations.

> Add I2C controller nodes to the MA35D1 SoC dtsi.
> Also enable the I2C interfaces on the MA35D1 SOM board
> to allow communication with onboard peripherals.

The commit message mentions enabling multiple I2C interfaces and allowing
communication with onboard peripherals. Since only a single interface is
enabled in the device tree and no child nodes for peripherals are added,
is this description accurate, or were some parts of the patch omitted?

> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
> index f6f20a17e5018..a0f1d76d288e1 100644
> --- a/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
> @@ -13,6 +13,7 @@ / {
>  	compatible = "nuvoton,ma35d1-som", "nuvoton,ma35d1";
>  
>  	aliases {
> +		i2c0 = &i2c2;

Does this alias point to the correct node? Since the i2c2 node remains
disabled, the subsystem will not create this interface. Should this be
pointing to the newly enabled i2c1 node instead?

[ ... ]

> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> index e51b98f5bdce4..a88d1c2938e78 100644
> --- a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> @@ -372,6 +372,66 @@ uart15: serial@407f0000 {
>  			status = "disabled";
>  		};
>  
> +		i2c1: i2c@40810000 {

It looks like the i2c0 controller node is missing from the hardware
description. Should i2c0 be included here to fully describe the SoC, even
if it is marked as reserved for the secure world or a coprocessor?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512073953.564323-1-zychennvt@gmail.com?part=3