Re: [PATCH 2/2] arm64: dts: cix: add FCH(S0)/S5 GPIO controllers for sky1

[Krzysztof Kozlowski]

On Fri, Mar 06, 2026 at 05:32:38PM +0800, Zichar Zhang wrote:
> From: "Zichar.Zhang" <zichar.zhang@xxxxxxxxxxx>
> 
> Add Cadence GPIO controller nodes for Sky1 FCH(S0) and S5 domains in
> sky1.dtsi, and enable those controllers on sky1-orion-o6.
> 
> Signed-off-by: Zichar Zhang <zichar.zhang@xxxxxxxxxxx>
> ---
>  arch/arm64/boot/dts/cix/sky1-orion-o6.dts |  28 +++++
>  arch/arm64/boot/dts/cix/sky1.dtsi         | 123 ++++++++++++++++++++++
>  2 files changed, 151 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> index 4dee8cd0b86d..4dc76e0135ee 100644
> --- a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> +++ b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> @@ -89,3 +89,31 @@ &pcie_x1_1_rc {
>  &uart2 {
>  	status = "okay";
>  };
> +
> +&s5_gpio0 {

I already asked cixtech contributors to read DTS coding style. More than
once. Does it mean I need to ask EACH contributor that? Maybe create
internal guideline to avoid trivial mistakes?

> +	status = "okay";
> +};
> +
> +&s5_gpio1 {
> +	status = "okay";
> +};
> +
> +&s5_gpio2 {
> +	status = "okay";
> +};
> +
> +&fch_gpio0 {
> +	status = "okay";
> +};
> +
> +&fch_gpio1 {
> +	status = "okay";
> +};
> +
> +&fch_gpio2 {
> +	status = "okay";
> +};
> +
> +&fch_gpio3 {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index 72f3b195a927..9ceaf8f68e83 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -185,6 +185,13 @@ psci {
>  		method = "smc";
>  	};
>  
> +	s5_gpio_apb_clk: s5-gpio-apb-clk {

Please use name for all fixed clocks which matches current format
recommendation: 'clock-<freq>' (see also the pattern in the binding for
any other options).
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +		clock-output-names = "s5_gpio_apb_clk";
> +	};
> +
>  	soc@0 {
>  		compatible = "simple-bus";
>  		ranges = <0 0 0 0 0x20 0>;
> @@ -354,6 +361,74 @@ syscon: syscon@4160000 {
>  			#reset-cells = <1>;
>  		};
>  
> +		fch_gpio0: gpio-controller@4120000 {

Again, read DTS coding style.

> +			compatible = "cdns,gpio-r1p02";
> +			reg = <0x0 0x4120000 0x0 0x1000>;
> +			clocks = <&scmi_clk CLK_TREE_FCH_GPIO_APB>;
> +			clock-names = "fch_gpio_apb_clk";

This is pointless name. GPIO block does not take some "fch" input. You
just called the input clock based on clock output which is completely
misunderstanding of the DTS.

Best regards,
Krzysztof