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

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Thierry Reding <thierry.reding@gmail.com>,
	Johan Jonker <jbx6244@gmail.com>
Cc: Rob Herring <robh@kernel.org>,
	"u.kleine-koenig@pengutronix.de" <u.kleine-koenig@pengutronix.de>,
	"linux-rockchip@lists.infradead.org" 
	<linux-rockchip@lists.infradead.org>,
	"philipp.tomsich@vrull.eu" <philipp.tomsich@vrull.eu>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"krzysztof.kozlowski+dt@linaro.org" 
	<krzysztof.kozlowski+dt@linaro.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
	"kever.yang@rock-chips.com" <kever.yang@rock-chips.com>,
	"zhangqing@rock-chips.com" <zhangqing@rock-chips.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"heiko@sntech.de" <heiko@sntech.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>
Subject: RE: [PATCH v1 03/11] dt-bindings: pwm: rockchip: add rockchip,rk3128-pwm
Date: Fri, 30 Sep 2022 06:13:49 +0000	[thread overview]
Message-ID: <OS0PR01MB592218C1636537D396A8E08E86569@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <YzQ3He2wyD2bgxz1@orome>

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@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.
> >
> >
> ======================================================================
> > =====
> > 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.
> >
> >
> ======================================================================
> > =====
> >
> > 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.
> > - 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