Re: [PATCH] arm64: dts: exynos: gs101: define all PERIC USI nodes

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Tudor Ambarus <tudor.ambarus@linaro.org>,
	peter.griffin@linaro.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org
Cc: alim.akhtar@samsung.com, linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, andre.draszik@linaro.org,
	willmcvicker@google.com, kernel-team@android.com
Subject: Re: [PATCH] arm64: dts: exynos: gs101: define all PERIC USI nodes
Date: Fri, 8 Mar 2024 17:45:02 +0100	[thread overview]
Message-ID: <073e5ef5-2a2e-4300-93d6-e25552276e13@linaro.org> (raw)
In-Reply-To: <20240307135912.163996-1-tudor.ambarus@linaro.org>

On 07/03/2024 14:59, Tudor Ambarus wrote:
> Universal Serial Interface (USI) supports three types of serial
> interface such as UART, SPI and I2C. Each protocol works independently.
> USI can be configured to work as one of these protocols. Define all the
> USI nodes from the PERIC blocks (USI0-14), in all their possible
> configurations. These blocks have the TX/RX FIFO depth of 64 bytes.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
> Please note that:
> - google,gs101-spi compatible was queued through the SPI tree:
>   https://lore.kernel.org/linux-arm-kernel/170742731537.2266792.3851016361229293846.b4-ty@kernel.org/
> - SPI dma properties were marked as not requiered. Queued through the
>   SPI tree:
>   https://lore.kernel.org/linux-spi/170967132774.228925.1759895846287455970.b4-ty@kernel.org/
> 
>  arch/arm64/boot/dts/exynos/google/gs101.dtsi | 782 +++++++++++++++++++
>  1 file changed, 782 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> index ee65ed9d2cfc..d7ecfbc7e440 100644
> --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> @@ -373,6 +373,398 @@ pinctrl_peric0: pinctrl@10840000 {
>  			interrupts = <GIC_SPI 625 IRQ_TYPE_LEVEL_HIGH 0>;
>  		};
>  
> +		usi1: usi@109000c0 {
> +			compatible = "google,gs101-usi",
> +				     "samsung,exynos850-usi";

This should fit within 81 characters, which is pretty close to 80, so if
there is going to be any resend/new version, please join the lines.

> +			reg = <0x109000c0 0x20>;
> +			ranges;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			clocks = <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_0>,
> +				 <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_0>;
> +			clock-names = "pclk", "ipclk";
> +			samsung,sysreg = <&sysreg_peric0 0x1000>;
> +			status = "disabled";
> +
> +			hsi2c_1: i2c@10900000 {
> +				compatible = "google,gs101-hsi2c",
> +					     "samsung,exynosautov9-hsi2c";
> +				reg = <0x10900000 0xc0>;
> +				interrupts = <GIC_SPI 635 IRQ_TYPE_LEVEL_HIGH 0>;

I wonder why we use four cells in GIC...

> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				pinctrl-names = "default";
> +				pinctrl-0 = <&hsi2c1_bus>;

Please reverse two lines, first pinctrl-0 then pinctrl-names. I know we
did not follow this convention till now, but at least new code can be
correct. Also clocks should be before pinctrl, so we keep some sort of
alphabetical order.

It is anyway too late in the cycle for me to pick it up, so no need to
hurry with resend.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel