Hi Thierry Reding,

Subject: Re: [PATCH v1 03/11] dt-bindings: pwm: rockchip: add
rockchip,rk3128-pwm

On Tue, Sep 13, 2022 at 04:38:32PM +0200, Johan Jonker wrote:

quoted

On 9/12/22 18:21, Rob Herring wrote:

quoted

On Sat, Sep 10, 2022 at 09:48:04PM +0200, Johan Jonker wrote:

quoted

Reduced CC.

Hi Rob,

Seemed like a simple enough warning to fix...

Some examples for comment.
Let us know what would be the better solution?

======================================================================

quoted

=====

option1:

	combpwm0: combpwm0 {
		compatible = "rockchip,rv1108-combpwm";
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
		#address-cells = <2>;
		#size-cells = <2>;

		pwm0: pwm@20040000 {
			compatible = "rockchip,rv1108-pwm";
			reg = <0x20040000 0x10>;
		};

		pwm1: pwm@20040010 {
			compatible = "rockchip,rv1108-pwm";
			reg = <0x20040010 0x10>;
		};

		pwm2: pwm@20040020 {
			compatible = "rockchip,rv1108-pwm";
			reg = <0x20040020 0x10>;
		};

		pwm3: pwm@20040030 {
			compatible = "rockchip,rv1108-pwm";
			reg = <0x20040030 0x10>;
		};
	};

PRO:
- Existing driver might still work.
CON:
- New compatible needed to service the combined interrupts.
- Driver change needed.

======================================================================

quoted

=====
option 2:

	combpwm0: pwm@10280000 {
		compatible = "rockchip,rv1108-pwm";
		reg = <0x10280000 0x40>;
		interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
		#address-cells = <1>;
		#size-cells = <0>;

		pwm4: pwm-4@0 {
			reg = <0x0>;
		};

		pwm5: pwm-5@10 {
			reg = <0x10>;
		};

		pwm6: pwm-6@20 {
			reg = <0x20>;
		};

		pwm7: pwm-7@30 {
			reg = <0x30>;
		};
	};

CON:
- Driver change needed.
- Not compatible with current drivers.

======================================================================

quoted

=====

Current situation:

	pwm0: pwm@20040000 {
		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
		reg = <0x20040000 0x10>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

	pwm1: pwm@20040010 {
		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
		reg = <0x20040010 0x10>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

	pwm2: pwm@20040020 {
		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
		reg = <0x20040020 0x10>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

	pwm3: pwm@20040030 {
		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
		reg = <0x20040030 0x10>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

CON:
- The property "interrupts 39" can only be claimed ones by one probe

function at the time.

quoted

- Has a fall-back string for rk3288, but unknown identical behavior

for interrupts ???

To be honest, all three descriptions look wrong to me. From the above
it looks like this is simply one PWM controller with four channels, so
it should really be described as such, i.e.:

	pwm@20040030 {
		compatible = "rockchip,rv1108-pwm";
		reg = <0x20040030 0x40>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

Sorry to jump in.

Renesas GPT has also similar case where we have large PWM IP block
having 8 pwm channels. Each channel has it's Own pinctrl, unique registers, interrupts
for each channel. But there are 4 sharable external trigger input pins for all the channels.

If it is a single block like this, how will you associate pinctrl 
with each channel?

At board level if you specify <pin4 enabled>, without pwm channel
specific information how will you configure channel4?

Maybe something like this will help. Is it acceptable?

	pwm@20040030 {
		compatible = "rockchip,rv1108-pwm";
		reg = <0x20040030 0x40>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;

		pwm4: pwm-4@0 {
			reg = <0x0>;
		};

		pwm5: pwm-5@10 {
			reg = <0x10>;
		};

		pwm6: pwm-6@20 {
			reg = <0x20>;
		};

		pwm7: pwm-7@30 {
			reg = <0x30>;
		};
	};


Cheers,
Biju

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