Re: [PATCH v1 3/3] arm64: dts: exynos990: define all PERIC USI nodes

[Tudor Ambarus <tudor.ambarus@linaro.org>]

On 2/14/25 6:17 AM, Denzeel Oliva wrote:
> On Thu, Feb 13, 2025 at 07:38:35AM +0000, Tudor Ambarus wrote:

>> node properties shall be specified in a specific order. Follow similar
>> nodes that are already accepted, gs101 is one.
> 
> Not all Exynos SoCs will follow the same order

you an fix them then. Please follow
https://docs.kernel.org/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node

> 
>> <&cmu_peric0 CLK_GOUT_PERIC0_TOP0_IPCLK_4>;
> 
> Is
> 
> GATE(CLK_GOUT_PERIC0_TOP0_IPCLK_4, "gout_peric0_top0_ipclk_4",
>      "dout_peric0_uart_dbg",
>      CLK_CON_GAT_GOUT_BLK_PERIC0_UID_PERIC0_TOP0_IPCLKPORT_IPCLK_4,
>      21, 0, 0), [Mainline CLK]

I don't get this reasoning, sorry.
> 
> You can find it in the cmucal-node.c driver downstream of the kernel. [0]
> 
>>> +			};
>>> +		};
>>> +
>>> +		usi0: usi@105500c0 {

cut

>>> +			serial_2: serial@10550000 {
>>
>> why not serial_0 since you're in USI0.
> 
> Because it is simply displayed in the exynos9830-usi.dtsi [1]

I don't think it matters what downstream specifies for labels. Use what
common sense says.

> 
>>> +		};
>>> +
>>> +		usi_i2c_0: usi@105600c0 {
>>> +			compatible = "samsung,exynos990-usi", "samsung,exynos850-usi";
>>> +			reg = <0x105600c0 0x20>;
>>> +			samsung,sysreg = <&sysreg_peric0 0x1008>;
>>> +			samsung,mode = <USI_V2_I2C>;
>>> +			#address-cells = <1>;
>>> +			#size-cells = <1>;
>>> +			ranges;
>>> +			clocks = <&cmu_peric0 CLK_GOUT_PERIC0_TOP0_PCLK_6>,
>>> +				 <&cmu_peric0 CLK_GOUT_PERIC0_TOP0_IPCLK_6>;
>>> +			clock-names = "pclk", "ipclk";
>>> +			status = "disabled";
>>> +
>>> +			hsi2c_1: i2c@10560000 {
>>> +				compatible = "samsung,exynos990-hsi2c",
>>> +					     "samsung,exynosautov9-hsi2c";
>>> +				reg = <0x10560000 0xc0>;
>>> +				interrupts = <GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH>;
>>> +				pinctrl-names = "default";
>>> +				pinctrl-0 = <&hsi2c1_bus>;
>>> +				clocks = <&cmu_peric0 CLK_GOUT_PERIC0_TOP0_IPCLK_6>,
>>> +					 <&cmu_peric0 CLK_GOUT_PERIC0_TOP0_PCLK_6>;
>>> +				clock-names = "hsi2c", "hsi2c_pclk";
>>> +				#address-cells = <1>;
>>> +				#size-cells = <0>;
>>> +				status = "disabled";
>>> +			};
>>
>> shouldn't you define serial and SPI too?
> 
> As shown in the node it only uses i2c which
> corresponds to the exynos9830-usi.dts. [2]

If you can't specify all the protocol modes for all the USI nodes, then
make it clear in the commit message.