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

[Biju Das]

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:
> >
> >
> > On 9/12/22 18:21, Rob Herring wrote:
> > > On Sat, Sep 10, 2022 at 09:48:04PM +0200, Johan Jonker wrote:
> > >> 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?
> >
> >
> ======================================================================
> > =====
> >
> > option1:
> >
> > 	combpwm0: combpwm0 {
> > 		compatible = "rockchip,rv1108-combpwm";
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 		#address-cells = <2>;
> > 		#size-cells = <2>;
> >
> > 		pwm0: pwm at 20040000 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040000 0x10>;
> > 		};
> >
> > 		pwm1: pwm at 20040010 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040010 0x10>;
> > 		};
> >
> > 		pwm2: pwm at 20040020 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040020 0x10>;
> > 		};
> >
> > 		pwm3: pwm at 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.
> >
> >
> ======================================================================
> > =====
> > option 2:
> >
> > 	combpwm0: pwm at 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 at 0 {
> > 			reg = <0x0>;
> > 		};
> >
> > 		pwm5: pwm-5 at 10 {
> > 			reg = <0x10>;
> > 		};
> >
> > 		pwm6: pwm-6 at 20 {
> > 			reg = <0x20>;
> > 		};
> >
> > 		pwm7: pwm-7 at 30 {
> > 			reg = <0x30>;
> > 		};
> > 	};
> >
> > CON:
> > - Driver change needed.
> > - Not compatible with current drivers.
> >
> >
> ======================================================================
> > =====
> >
> > Current situation:
> >
> > 	pwm0: pwm at 20040000 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040000 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > 	pwm1: pwm at 20040010 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040010 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > 	pwm2: pwm at 20040020 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040020 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > 	pwm3: pwm at 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.
> > - 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 at 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 at 20040030 {
		compatible = "rockchip,rv1108-pwm";
		reg = <0x20040030 0x40>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;

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

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

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

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


Cheers,
Biju