[v5,07/13] dt-bindings: pwm: add a property "num-pwms"
diff mbox series

Message ID 1566457123-20791-8-git-send-email-sam.shih@mediatek.com
State Superseded
Headers show
Series
  • Add mt7629 and fix mt7628 pwm
Related show

Commit Message

Sam Shih Aug. 22, 2019, 6:58 a.m. UTC
From: Ryder Lee <ryder.lee@mediatek.com>

This adds a property "num-pwms" in example so that we could
specify the number of PWM channels via device tree.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
Signed-off-by: Sam Shih <sam.shih@mediatek.com>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Changes since v5:
- Add an Acked-by tag
- This file is original v4 patch 5/10
(https://patchwork.kernel.org/patch/11102577/)

Change-Id: I429048afeffa96f3f14533910efe242f88776043
---
 Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Rob Herring Aug. 27, 2019, 6:39 p.m. UTC | #1
On Thu, Aug 22, 2019 at 02:58:37PM +0800, Sam Shih wrote:
> From: Ryder Lee <ryder.lee@mediatek.com>

The subject should indicate this is for Mediatek.

> 
> This adds a property "num-pwms" in example so that we could
> specify the number of PWM channels via device tree.
> 
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> Signed-off-by: Sam Shih <sam.shih@mediatek.com>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Changes since v5:
> - Add an Acked-by tag
> - This file is original v4 patch 5/10
> (https://patchwork.kernel.org/patch/11102577/)
> 
> Change-Id: I429048afeffa96f3f14533910efe242f88776043
> ---
>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> index 991728cb46cb..ea95b490a913 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> @@ -14,12 +14,12 @@ Required properties:
>                  has no clocks
>     - "top": the top clock generator
>     - "main": clock used by the PWM core
> -   - "pwm1-8": the eight per PWM clocks for mt2712
> -   - "pwm1-6": the six per PWM clocks for mt7622
> -   - "pwm1-5": the five per PWM clocks for mt7623
> +   - "pwm1-N": the PWM clocks for each channel
> +   where N starting from 1 to the maximum number of PWM channels

Once converted to schema, you are going to be back to listing them out.

>   - pinctrl-names: Must contain a "default" entry.
>   - pinctrl-0: One property must exist for each entry in pinctrl-names.
>     See pinctrl/pinctrl-bindings.txt for details of the property values.
> + - num-pwms: the number of PWM channels.

You can't add new required properties without breaking compatibility. 

You already have to imply the number of channels from the compatible (or 
number of clocks) and you have to keep doing so to maintain 
compatibility, so why not just keep doing that for new chips?

Rob
Uwe Kleine-König Sept. 2, 2019, 4:04 p.m. UTC | #2
On Tue, Aug 27, 2019 at 01:39:24PM -0500, Rob Herring wrote:
> On Thu, Aug 22, 2019 at 02:58:37PM +0800, Sam Shih wrote:
> > From: Ryder Lee <ryder.lee@mediatek.com>
> 
> The subject should indicate this is for Mediatek.
> 
> > 
> > This adds a property "num-pwms" in example so that we could
> > specify the number of PWM channels via device tree.
> > 
> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > Signed-off-by: Sam Shih <sam.shih@mediatek.com>
> > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Changes since v5:
> > - Add an Acked-by tag
> > - This file is original v4 patch 5/10
> > (https://patchwork.kernel.org/patch/11102577/)
> > 
> > Change-Id: I429048afeffa96f3f14533910efe242f88776043
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > index 991728cb46cb..ea95b490a913 100644
> > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > @@ -14,12 +14,12 @@ Required properties:
> >                  has no clocks
> >     - "top": the top clock generator
> >     - "main": clock used by the PWM core
> > -   - "pwm1-8": the eight per PWM clocks for mt2712
> > -   - "pwm1-6": the six per PWM clocks for mt7622
> > -   - "pwm1-5": the five per PWM clocks for mt7623
> > +   - "pwm1-N": the PWM clocks for each channel
> > +   where N starting from 1 to the maximum number of PWM channels
> 
> Once converted to schema, you are going to be back to listing them out.
> 
> >   - pinctrl-names: Must contain a "default" entry.
> >   - pinctrl-0: One property must exist for each entry in pinctrl-names.
> >     See pinctrl/pinctrl-bindings.txt for details of the property values.
> > + - num-pwms: the number of PWM channels.
> 
> You can't add new required properties without breaking compatibility. 
> 
> You already have to imply the number of channels from the compatible (or 
> number of clocks) and you have to keep doing so to maintain 
> compatibility, so why not just keep doing that for new chips?

This was a suggestion by me. The driver still handles compatibility
(i.e. falls back to the number of PWMs that was implied by the
compatible before). Given that there are various drivers that all solve
the same problem (i.e. different variants with different number of PWMs)
I thought it would be a good idea to introduce a property in the device
tree that specifies this number.

Only for newly introduced compatibles the num-pwms property is really
required. Differentiating the ones that need it and the ones that don't
seems over-engineered to me.

(BTW, using the number of clks doesn't really work because there are
also some variants without clocks. It is still under discussion if in
this case dummy-clocks should be provided IIRC.)

Best regards
Uwe
Sam Shih Sept. 5, 2019, 3:27 a.m. UTC | #3
On Mon, 2019-09-02 at 18:04 +0200, Uwe Kleine-König wrote:
> On Tue, Aug 27, 2019 at 01:39:24PM -0500, Rob Herring wrote:
> > On Thu, Aug 22, 2019 at 02:58:37PM +0800, Sam Shih wrote:
> > > From: Ryder Lee <ryder.lee@mediatek.com>
> > 
> > The subject should indicate this is for Mediatek.
> > 
> > > 
> > > This adds a property "num-pwms" in example so that we could
> > > specify the number of PWM channels via device tree.
> > > 
> > > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > > Signed-off-by: Sam Shih <sam.shih@mediatek.com>
> > > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Changes since v5:
> > > - Add an Acked-by tag
> > > - This file is original v4 patch 5/10
> > > (https://patchwork.kernel.org/patch/11102577/)
> > > 
> > > Change-Id: I429048afeffa96f3f14533910efe242f88776043
> > > ---
> > >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > > index 991728cb46cb..ea95b490a913 100644
> > > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > > @@ -14,12 +14,12 @@ Required properties:
> > >                  has no clocks
> > >     - "top": the top clock generator
> > >     - "main": clock used by the PWM core
> > > -   - "pwm1-8": the eight per PWM clocks for mt2712
> > > -   - "pwm1-6": the six per PWM clocks for mt7622
> > > -   - "pwm1-5": the five per PWM clocks for mt7623
> > > +   - "pwm1-N": the PWM clocks for each channel
> > > +   where N starting from 1 to the maximum number of PWM channels
> > 
> > Once converted to schema, you are going to be back to listing them out.
> > 
> > >   - pinctrl-names: Must contain a "default" entry.
> > >   - pinctrl-0: One property must exist for each entry in pinctrl-names.
> > >     See pinctrl/pinctrl-bindings.txt for details of the property values.
> > > + - num-pwms: the number of PWM channels.
> > 
> > You can't add new required properties without breaking compatibility. 
> > 
> > You already have to imply the number of channels from the compatible (or 
> > number of clocks) and you have to keep doing so to maintain 
> > compatibility, so why not just keep doing that for new chips?
> 
> This was a suggestion by me. The driver still handles compatibility
> (i.e. falls back to the number of PWMs that was implied by the
> compatible before). Given that there are various drivers that all solve
> the same problem (i.e. different variants with different number of PWMs)
> I thought it would be a good idea to introduce a property in the device
> tree that specifies this number.
> Only for newly introduced compatibles the num-pwms property is really
> required. Differentiating the ones that need it and the ones that don't
> seems over-engineered to me.

I'm fine with both.

num-pwms and pwm1-N is required properties for new chip, but it still
have backward compatibility for old dt without num-pwms properties.


> (BTW, using the number of clks doesn't really work because there are
> also some variants without clocks. It is still under discussion if in
> this case dummy-clocks should be provided IIRC.)

Yes, the dummy-clocks of "top", "main" is needed in old MIPS dt.
We also need fixed-clock for period calculation.



Best Regards,
Sam
Sam Shih Sept. 12, 2019, 1:33 a.m. UTC | #4
On Mon, 2019-09-02 at 18:04 +0200, Uwe Kleine-König wrote:
> On Tue, Aug 27, 2019 at 01:39:24PM -0500, Rob Herring wrote:
> > On Thu, Aug 22, 2019 at 02:58:37PM +0800, Sam Shih wrote:
> > > From: Ryder Lee <ryder.lee@mediatek.com>
> > 
> > The subject should indicate this is for Mediatek.
> > 
> > > 
> > > This adds a property "num-pwms" in example so that we could
> > > specify the number of PWM channels via device tree.
> > > 
> > > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > > Signed-off-by: Sam Shih <sam.shih@mediatek.com>
> > > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Changes since v5:
> > > - Add an Acked-by tag
> > > - This file is original v4 patch 5/10
> > > (https://patchwork.kernel.org/patch/11102577/)
> > > 
> > > Change-Id: I429048afeffa96f3f14533910efe242f88776043
> > > ---
> > >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > > index 991728cb46cb..ea95b490a913 100644
> > > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > > @@ -14,12 +14,12 @@ Required properties:
> > >                  has no clocks
> > >     - "top": the top clock generator
> > >     - "main": clock used by the PWM core
> > > -   - "pwm1-8": the eight per PWM clocks for mt2712
> > > -   - "pwm1-6": the six per PWM clocks for mt7622
> > > -   - "pwm1-5": the five per PWM clocks for mt7623
> > > +   - "pwm1-N": the PWM clocks for each channel
> > > +   where N starting from 1 to the maximum number of PWM channels
> > 
> > Once converted to schema, you are going to be back to listing them out.
> > 
> > >   - pinctrl-names: Must contain a "default" entry.
> > >   - pinctrl-0: One property must exist for each entry in pinctrl-names.
> > >     See pinctrl/pinctrl-bindings.txt for details of the property values.
> > > + - num-pwms: the number of PWM channels.
> > 
> > You can't add new required properties without breaking compatibility. 
> > 
> > You already have to imply the number of channels from the compatible (or 
> > number of clocks) and you have to keep doing so to maintain 
> > compatibility, so why not just keep doing that for new chips?
> 
> This was a suggestion by me. The driver still handles compatibility
> (i.e. falls back to the number of PWMs that was implied by the
> compatible before). Given that there are various drivers that all solve
> the same problem (i.e. different variants with different number of PWMs)
> I thought it would be a good idea to introduce a property in the device
> tree that specifies this number.
> 
> Only for newly introduced compatibles the num-pwms property is really
> required. Differentiating the ones that need it and the ones that don't
> seems over-engineered to me.
> 
> (BTW, using the number of clks doesn't really work because there are
> also some variants without clocks. It is still under discussion if in
> this case dummy-clocks should be provided IIRC.)
> 
> Best regards
> Uwe
> 


Any conclusions ? 

just a friendly reminder :)


regards Sam

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
index 991728cb46cb..ea95b490a913 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
@@ -14,12 +14,12 @@  Required properties:
                 has no clocks
    - "top": the top clock generator
    - "main": clock used by the PWM core
-   - "pwm1-8": the eight per PWM clocks for mt2712
-   - "pwm1-6": the six per PWM clocks for mt7622
-   - "pwm1-5": the five per PWM clocks for mt7623
+   - "pwm1-N": the PWM clocks for each channel
+   where N starting from 1 to the maximum number of PWM channels
  - pinctrl-names: Must contain a "default" entry.
  - pinctrl-0: One property must exist for each entry in pinctrl-names.
    See pinctrl/pinctrl-bindings.txt for details of the property values.
+ - num-pwms: the number of PWM channels.
 
 Example:
 	pwm0: pwm@11006000 {
@@ -37,4 +37,5 @@  Example:
 			      "pwm3", "pwm4", "pwm5";
 		pinctrl-names = "default";
 		pinctrl-0 = <&pwm0_pins>;
+		num-pwms = <5>;
 	};