Re: [PATCH] riscv: dts: spacemit: k3: Add pwm support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Device tree node name collision causes silent overwrite of PWM0 pin configuration.
- [Medium] PWM nodes specify two clocks, violating the schema for the fallback compatible and leaving the bus clock unmanaged.
- [Low] Widespread copy-paste errors in pinctrl child node names.
--

commit 9a4ae96f46c2dc497874c74f2a45fcc9d68ce685
Author: Yixun Lan <dlan@kernel.org>
riscv: dts: spacemit: k3: Add pwm support

This commit populates the PWM device tree nodes for the SpacemiT K3 SoC.
It also adds the corresponding pinctrl configurations to enable PWM support
in the future.

> diff --git a/arch/riscv/boot/dts/spacemit/k3-pinctrl.dtsi b/arch/riscv/boot/dts/spacemit/k3-pinctrl.dtsi
> index 23899d3f308a0..1fd39502071be 100644
> --- a/arch/riscv/boot/dts/spacemit/k3-pinctrl.dtsi
> +++ b/arch/riscv/boot/dts/spacemit/k3-pinctrl.dtsi
> @@ -56,6 +56,596 @@ i2c8-pins {
>  		};
>  	};
>  
> +	/omit-if-no-ref/
> +	pwm0_0_cfg: pwm0-0-cfg {
> +		pwm0-0-pins {
> +			pinmux = <K3_PADCONF(0, 3)>;	/* pwm0 */
> +
> +			bias-pull-up = <0>;
> +			drive-strength = <25>;
> +		};
> +	};

[ ... ]

> +	/omit-if-no-ref/
> +	pwm1_0_cfg: pwm1-0-cfg {
> +		pwm1-0-pins {
> +			pinmux = <K3_PADCONF(1, 3)>;	/* pwm1 */
> +
> +			bias-pull-up = <0>;
> +			drive-strength = <25>;
> +		};
> +	};
> +
> +	/omit-if-no-ref/
> +	pwm1_1_cfg: pwm0-0-cfg {

Will this cause a node name collision that silently overwrites the PWM0 pin
configuration?

The Device Tree Compiler merges nodes with identical names at the same level.
Because this is named pwm0-0-cfg instead of pwm1-1-cfg, the pinmux property
of pwm0_0_cfg will be overwritten by this node. If a board device tree
references pwm0_0_cfg, will it receive the pin configuration for PWM1 instead
of PWM0?

> +		pwm0-0-pins {

This isn't a bug, but there appear to be widespread copy-paste errors in the
pinctrl child node names.

For example, pwm1_1_cfg uses pwm0-0-pins, pwm1_2_cfg uses pwm1-0-pins, and
pwm2_1_cfg uses pwm2-0-pins. While this does not cause functional issues, it
creates a confusing device tree structure.

> +			pinmux = <K3_PADCONF(43, 6)>;	/* pwm1 */
> +
> +			bias-pull-up = <0>;
> +			drive-strength = <25>;
> +		};
> +	};

[ ... ]

> diff --git a/arch/riscv/boot/dts/spacemit/k3.dtsi b/arch/riscv/boot/dts/spacemit/k3.dtsi
> index 815debd16409b..07e43b6b256ec 100644
> --- a/arch/riscv/boot/dts/spacemit/k3.dtsi
> +++ b/arch/riscv/boot/dts/spacemit/k3.dtsi
> @@ -797,6 +797,226 @@ i2c8: i2c@d401d800 {
>  			status = "disabled";
>  		};
>  
> +		pwm0: pwm@d401a000 {
> +			compatible = "spacemit,k3-pwm", "marvell,pxa910-pwm";
> +			reg = <0x0 0xd401a000 0x0 0x10>;
> +			clocks = <&syscon_apbc CLK_APBC_PWM0>,
> +				 <&syscon_apbc CLK_APBC_PWM0_BUS>;
> +			clock-names = "func", "bus";

Could these clock definitions violate the schema for the fallback compatible
and leave the bus clock unmanaged?

The dt-binding for marvell,pxa-pwm.yaml restricts clocks to maxItems: 1 and
prohibits clock-names.

Additionally, the fallback driver (drivers/pwm/pwm-pxa.c) calls
devm_clk_get(dev, NULL), which will only acquire the first func clock. If the
SoC requires the APB bus clock to be explicitly enabled for register access,
could this result in a bus fault when the fallback driver probes?

> +			resets = <&syscon_apbc RESET_APBC_PWM0>;
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-04-k3-pwm-dts-v1-1-81fcde1871f8@kernel.org?part=1