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

From: Tudor Ambarus
Date: Fri Feb 14 2025 - 01:49:56 EST




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.