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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <OS0PR01MB592218C1636537D396A8E08E86569@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Date:   Fri, 30 Sep 2022 06:13:49 +0000
From:   Biju Das <biju.das.jz@...renesas.com>
To:     Thierry Reding <thierry.reding@...il.com>,
        Johan Jonker <jbx6244@...il.com>
CC:     Rob Herring <robh@...nel.org>,
        "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
        "linux-rockchip@...ts.infradead.org" 
        <linux-rockchip@...ts.infradead.org>,
        "philipp.tomsich@...ll.eu" <philipp.tomsich@...ll.eu>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "kever.yang@...k-chips.com" <kever.yang@...k-chips.com>,
        "zhangqing@...k-chips.com" <zhangqing@...k-chips.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "heiko@...ech.de" <heiko@...ech.de>,
        Geert Uytterhoeven <geert+renesas@...der.be>
Subject: RE: [PATCH v1 03/11] dt-bindings: pwm: rockchip: add
 rockchip,rk3128-pwm

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@...40000 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040000 0x10>;
> > 		};
> >
> > 		pwm1: pwm@...40010 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040010 0x10>;
> > 		};
> >
> > 		pwm2: pwm@...40020 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040020 0x10>;
> > 		};
> >
> > 		pwm3: pwm@...40030 {
> > 			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@...80000 {
> > 		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@...40000 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040000 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > 	pwm1: pwm@...40010 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040010 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > 	pwm2: pwm@...40020 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040020 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > 	pwm3: pwm@...40030 {
> > 		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@...40030 {
> 		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@...40030 {
		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