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

[zychen <zychennvt@gmail.com>]

Hi Krzysztof,
	Thanks for your review.

Krzysztof Kozlowski 於 2026/3/2 下午 03:25 寫道:
> On Mon, Mar 02, 2026 at 02:08:22AM +0000, Zi-Yu Chen wrote:
>> 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.
>>
>> Signed-off-by: Zi-Yu Chen <zychennvt@gmail.com>
>> ---
>>  .../boot/dts/nuvoton/ma35d1-som-256m.dts      | 14 ++++
>>  arch/arm64/boot/dts/nuvoton/ma35d1.dtsi       | 65 +++++++++++++++++++
>>  2 files changed, 79 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
>> index f6f20a17e501..2a8f0fd90ded 100644
>> --- a/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
>> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
>> @@ -98,6 +98,14 @@ pinctrl_uart16: uart16-pins {
>>  			power-source = <1>;
>>  		};
>>  	};
> 
> Missing blank line
Will fix in v2.
> 
>> +	i2c-grp {
>> +		pinctrl_i2c1: i2c1-pins {
>> +			nuvoton,pins = <1 10 12>,
>> +				       <1 11 12>;
>> +			bias-disable;
>> +		};
>> +
>> +	};
>>  };
>>  
>>  &uart0 {
>> @@ -129,3 +137,9 @@ &uart16 {
>>  	pinctrl-0 = <&pinctrl_uart16>;
>>  	status = "okay";
>>  };
>> +
>> +&i2c1 {
> 
> Why 'i' is after 'u'? Please read DTS coding style.
Will move &i2c1 after &clk and before &pinctrl in v2 to follow alphabetical order.
> 
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c1>;
>> +	status = "okay";
>> +};
>> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
>> index e51b98f5bdce..36bd19e37b57 100644
>> --- a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
>> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
>> @@ -17,6 +17,10 @@ / {
>>  	#address-cells = <2>;
>>  	#size-cells = <2>;
>>  
>> +	aliases {
>> +		i2c0 = &i2c2;
> 
> Not a property of DTSI, but DTS.
Will move to DTSI in v2.
> 
>> +	};
>> +
>>  	cpus {
>>  		#address-cells = <2>;
>>  		#size-cells = <0>;
>> @@ -372,6 +376,66 @@ uart15: serial@407f0000 {
>>  			status = "disabled";
>>  		};
>>  
>> +		i2c1: i2c@40810000 {
>> +			compatible = "nuvoton,ma35d1-i2c";
>> +			reg = <0x0 0x40810000 0x0 0x1000>;
>> +			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&clk I2C1_GATE>;
>> +			clock-frequency = <100000>;
>> +			resets = <&sys MA35D1_RESET_I2C1>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +		};
>> +
>> +		i2c2: i2c@40820000 {
>> +			compatible = "nuvoton,ma35d1-i2c";
>> +			reg = <0x0 0x40820000 0x0 0x1000>;
>> +			interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&clk I2C2_GATE>;
>> +			clock-frequency = <100000>;
>> +			resets = <&sys MA35D1_RESET_I2C2>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +		};
>> +
>> +		i2c3: i2c@40830000 {
>> +			compatible = "nuvoton,ma35d1-i2c";
>> +			reg = <0x0 0x40830000 0x0 0x1000>;
>> +			interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&clk I2C3_GATE>;
>> +			clock-frequency = <100000>;
>> +			resets = <&sys MA35D1_RESET_I2C3>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +		};
>> +
>> +		i2c4: i2c@40840000 {
>> +			compatible = "nuvoton,ma35d1-i2c";
>> +			reg = <0x0 0x40840000 0x0 0x1000>;
>> +			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&clk I2C4_GATE>;
>> +			clock-frequency = <100000>;
>> +			resets = <&sys MA35D1_RESET_I2C4>;
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			status = "disabled";
>> +		};
>> +
>> +		i2c5: i2c@40850000 {
>> +			compatible = "nuvoton,ma35d1-i2c";
>> +			reg = <0x0 0x40850000 0x0 0x1000>;
>> +			interrupts = <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&clk I2C5_GATE>;
>> +			clock-frequency = <100000>;
>> +			resets = <&sys MA35D1_RESET_I2C5>;
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			status = "disabled";
>> +		};
>> +
>>  		uart16: serial@40880000 {
>>  			compatible = "nuvoton,ma35d1-uart";
>>  			reg = <0x0 0x40880000 0x0 0x100>;
>> @@ -379,5 +443,6 @@ uart16: serial@40880000 {
>>  			clocks = <&clk UART16_GATE>;
>>  			status = "disabled";
>>  		};
>> +
> 
> Why? Do not introduce random changes.
Will fix in v2.
> 
> Best regards,
> Krzysztof
>