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
