diff mbox series

[v9,07/11] dt-bindings: pwm: pwm-mediatek: add a property "num-pwms"

Message ID 1568933351-8584-8-git-send-email-sam.shih@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Add mt7629 and fix mt7628 pwm | expand

Commit Message

Sam Shih (史碩三) Sept. 19, 2019, 10:49 p.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 v6:
Follow reviewers's comments:
- The subject should indicate this is for Mediatek

Changes since v5:
- Add an Acked-by tag
- This file is original v4 patch 5/10
(https://patchwork.kernel.org/patch/11102577/)

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

Comments

Thierry Reding Sept. 21, 2019, 12:21 a.m. UTC | #1 
On Fri, Sep 20, 2019 at 06:49:07AM +0800, Sam Shih wrote:
> 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 v6:
> Follow reviewers's comments:
> - The subject should indicate this is for Mediatek
> 
> Changes since v5:
> - Add an Acked-by tag
> - This file is original v4 patch 5/10
> (https://patchwork.kernel.org/patch/11102577/)
> 
> ---
>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

You failed to address Rob's questions repeatedly and I agree with him
that you can just as easily derive the number of PWMs from the specific
compatible string. I won't be applying this and none of the patches that
depend on it.

Thierry
Sam Shih (史碩三) Sept. 23, 2019, 3:20 a.m. UTC | #2 
On Sat, 2019-09-21 at 02:21 +0200, Thierry Reding wrote:
> On Fri, Sep 20, 2019 at 06:49:07AM +0800, Sam Shih wrote:
> > 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 v6:
> > Follow reviewers's comments:
> > - The subject should indicate this is for Mediatek
> > 
> > Changes since v5:
> > - Add an Acked-by tag
> > - This file is original v4 patch 5/10
> > (https://patchwork.kernel.org/patch/11102577/)
> > 
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> You failed to address Rob's questions repeatedly and I agree with him
> that you can just as easily derive the number of PWMs from the specific
> compatible string. I won't be applying this and none of the patches that
> depend on it.
> 

Hi, 

Thanks for getting back to me.

New pwm driver (patch 04/11 : "pwm: mediatek: allocate the clks array
dynamically") can support different variants with different number of
PWMs by the new property <num-pwms>

For example:
1. Use "num-pwms" = <2> and assign clocks pwm1, pwm2 for mt7622
2. Use "num-pwms" = <6> and assign clocks pwm1, pwm2, pwm3, pwm4, pwm5,
pwm6 for mt7622.

If we just as easily derive the number of PWMs from the specific
compatible string in this document:

   - "pwm1-6": the six per PWM clocks for mt7622
   
This looks like all "pwm1", "pwm2", "pwm3", "pwm4", "pwm5", "pwm6" is
required property in DT, It doesn't make sense.
   
So we removed those descriptions and added  

   - "pwm1-N": the PWM clocks for each channel 
   
   
But the max number of clocks from the compatible string are still
important information that should be provide in this document.


What do you think of this?

   - "pwm1-N": per PWM clocks for mt2712, the max number of PWM channels
is 8

   - "pwm1-N": per PWM clocks for mt7622, the max number of PWM channels
is 6

   - "pwm1-N": per PWM clocks for mt7623, the max number of PWM channels
is 5

   where N starting from 1 to the maximum number of PWM channels
   - num-pwms: the number of PWM channels.

   
Thanks
Best Regards
Sam
Thierry Reding Sept. 23, 2019, 1:36 p.m. UTC | #3 
On Mon, Sep 23, 2019 at 11:20:57AM +0800, Sam Shih wrote:
> On Sat, 2019-09-21 at 02:21 +0200, Thierry Reding wrote:
> > On Fri, Sep 20, 2019 at 06:49:07AM +0800, Sam Shih wrote:
> > > 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 v6:
> > > Follow reviewers's comments:
> > > - The subject should indicate this is for Mediatek
> > > 
> > > Changes since v5:
> > > - Add an Acked-by tag
> > > - This file is original v4 patch 5/10
> > > (https://patchwork.kernel.org/patch/11102577/)
> > > 
> > > ---
> > >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > You failed to address Rob's questions repeatedly and I agree with him
> > that you can just as easily derive the number of PWMs from the specific
> > compatible string. I won't be applying this and none of the patches that
> > depend on it.
> > 
> 
> Hi, 
> 
> Thanks for getting back to me.
> 
> New pwm driver (patch 04/11 : "pwm: mediatek: allocate the clks array
> dynamically") can support different variants with different number of
> PWMs by the new property <num-pwms>
> 
> For example:
> 1. Use "num-pwms" = <2> and assign clocks pwm1, pwm2 for mt7622
> 2. Use "num-pwms" = <6> and assign clocks pwm1, pwm2, pwm3, pwm4, pwm5,
> pwm6 for mt7622.
> 
> If we just as easily derive the number of PWMs from the specific
> compatible string in this document:
> 
>    - "pwm1-6": the six per PWM clocks for mt7622
>    
> This looks like all "pwm1", "pwm2", "pwm3", "pwm4", "pwm5", "pwm6" is
> required property in DT, It doesn't make sense.

I don't understand. Why doesn't that make sense? If your hardware block
has 6 PWMs and each can be driven by its own clock, then you need to
provide references for each of those clocks, otherwise you won't be able
to use them.

>    
> So we removed those descriptions and added  
> 
>    - "pwm1-N": the PWM clocks for each channel 
>    
>    
> But the max number of clocks from the compatible string are still
> important information that should be provide in this document.
> 
> 
> What do you think of this?
> 
>    - "pwm1-N": per PWM clocks for mt2712, the max number of PWM channels
> is 8
> 
>    - "pwm1-N": per PWM clocks for mt7622, the max number of PWM channels
> is 6
> 
>    - "pwm1-N": per PWM clocks for mt7623, the max number of PWM channels
> is 5

That's what's in the bindings already, isn't it?

	 - clocks: phandle and clock specifier of the PWM reference clock.
	 - clock-names: must contain the following, except for MT7628 which
			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

Note that the description of the "clocks" property isn't quite accurate.
It should be something like:

	- clocks: One phandle and clock specifier for each entry in the
	          "clock-names" property.

In the above you clearly describe which PWMs you have to specify for
each generation of the hardware block.

> 
>    where N starting from 1 to the maximum number of PWM channels
>    - num-pwms: the number of PWM channels.

That's redundant information. The specific number of PWMs in already
implied by the compatible string, so you don't need to duplicate that
information here.

Thierry
Sam Shih (史碩三) Sept. 23, 2019, 3:11 p.m. UTC | #4 
On Mon, 2019-09-23 at 15:36 +0200, Thierry Reding wrote:
> On Mon, Sep 23, 2019 at 11:20:57AM +0800, Sam Shih wrote:
> > On Sat, 2019-09-21 at 02:21 +0200, Thierry Reding wrote:
> > > On Fri, Sep 20, 2019 at 06:49:07AM +0800, Sam Shih wrote:
> > > > 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 v6:
> > > > Follow reviewers's comments:
> > > > - The subject should indicate this is for Mediatek
> > > > 
> > > > Changes since v5:
> > > > - Add an Acked-by tag
> > > > - This file is original v4 patch 5/10
> > > > (https://patchwork.kernel.org/patch/11102577/)
> > > > 
> > > > ---
> > > >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > You failed to address Rob's questions repeatedly and I agree with him
> > > that you can just as easily derive the number of PWMs from the specific
> > > compatible string. I won't be applying this and none of the patches that
> > > depend on it.
> > > 
> > 
> > Hi, 
> > 
> > Thanks for getting back to me.
> > 
> > New pwm driver (patch 04/11 : "pwm: mediatek: allocate the clks array
> > dynamically") can support different variants with different number of
> > PWMs by the new property <num-pwms>
> > 
> > For example:
> > 1. Use "num-pwms" = <2> and assign clocks pwm1, pwm2 for mt7622
> > 2. Use "num-pwms" = <6> and assign clocks pwm1, pwm2, pwm3, pwm4, pwm5,
> > pwm6 for mt7622.
> > 
> > If we just as easily derive the number of PWMs from the specific
> > compatible string in this document:
> > 
> >    - "pwm1-6": the six per PWM clocks for mt7622
> >    
> > This looks like all "pwm1", "pwm2", "pwm3", "pwm4", "pwm5", "pwm6" is
> > required property in DT, It doesn't make sense.
> 
> I don't understand. Why doesn't that make sense? If your hardware block
> has 6 PWMs and each can be driven by its own clock, then you need to
> provide references for each of those clocks, otherwise you won't be able
> to use them.

Thank you for your instruction,
I will add all clock-names and clocks according to
hardware blocks instead of value of <num-pwms> in DT.

> >    
> > So we removed those descriptions and added  
> > 
> >    - "pwm1-N": the PWM clocks for each channel 
> >    
> >    
> > But the max number of clocks from the compatible string are still
> > important information that should be provide in this document.
> > 
> > 
> > What do you think of this?
> > 
> >    - "pwm1-N": per PWM clocks for mt2712, the max number of PWM channels
> > is 8
> > 
> >    - "pwm1-N": per PWM clocks for mt7622, the max number of PWM channels
> > is 6
> > 
> >    - "pwm1-N": per PWM clocks for mt7623, the max number of PWM channels
> > is 5
> 
> That's what's in the bindings already, isn't it?
> 
> 	 - clocks: phandle and clock specifier of the PWM reference clock.
> 	 - clock-names: must contain the following, except for MT7628 which
> 			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

Yes, You are right, 
I will keep original description and remove "pwm1-N" from this patch.
	   - "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       

Actually, MT7629 also use "mediatek,mt7622-pwm" as compatible string,
but it's hardware only support one pwm, so I was wrongly stick by
expecting "pwm1-N" in clock-names based on "num-pwms" in DT.
(that we can assign num-pwms to 1 and only provide pwm1 as clock-names)

Maybe added mt7629 description to this document can solve this simply.
	   - "pwm1": the PWM1 clock for mt7629    

> Note that the description of the "clocks" property isn't quite accurate.
> It should be something like:
> 
> 	- clocks: One phandle and clock specifier for each entry in the
> 	          "clock-names" property.
> 
> In the above you clearly describe which PWMs you have to specify for
> each generation of the hardware block.

Thanks for your advise, I will update this description
	- clocks: One phandle and clock specifier for each entry in the
	          "clock-names" property.

> > 
> >    where N starting from 1 to the maximum number of PWM channels
> >    - num-pwms: the number of PWM channels.
> 
> That's redundant information. The specific number of PWMs in already
> implied by the compatible string, so you don't need to duplicate that
> information here.

Okay, I will remove it.



Thanks,
Best Regards,
Sam
Thierry Reding Sept. 23, 2019, 3:20 p.m. UTC | #5 
On Mon, Sep 23, 2019 at 11:11:55PM +0800, Sam Shih wrote:
> On Mon, 2019-09-23 at 15:36 +0200, Thierry Reding wrote:
> > On Mon, Sep 23, 2019 at 11:20:57AM +0800, Sam Shih wrote:
> > > On Sat, 2019-09-21 at 02:21 +0200, Thierry Reding wrote:
> > > > On Fri, Sep 20, 2019 at 06:49:07AM +0800, Sam Shih wrote:
> > > > > 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 v6:
> > > > > Follow reviewers's comments:
> > > > > - The subject should indicate this is for Mediatek
> > > > > 
> > > > > Changes since v5:
> > > > > - Add an Acked-by tag
> > > > > - This file is original v4 patch 5/10
> > > > > (https://patchwork.kernel.org/patch/11102577/)
> > > > > 
> > > > > ---
> > > > >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 7 ++++---
> > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > You failed to address Rob's questions repeatedly and I agree with him
> > > > that you can just as easily derive the number of PWMs from the specific
> > > > compatible string. I won't be applying this and none of the patches that
> > > > depend on it.
> > > > 
> > > 
> > > Hi, 
> > > 
> > > Thanks for getting back to me.
> > > 
> > > New pwm driver (patch 04/11 : "pwm: mediatek: allocate the clks array
> > > dynamically") can support different variants with different number of
> > > PWMs by the new property <num-pwms>
> > > 
> > > For example:
> > > 1. Use "num-pwms" = <2> and assign clocks pwm1, pwm2 for mt7622
> > > 2. Use "num-pwms" = <6> and assign clocks pwm1, pwm2, pwm3, pwm4, pwm5,
> > > pwm6 for mt7622.
> > > 
> > > If we just as easily derive the number of PWMs from the specific
> > > compatible string in this document:
> > > 
> > >    - "pwm1-6": the six per PWM clocks for mt7622
> > >    
> > > This looks like all "pwm1", "pwm2", "pwm3", "pwm4", "pwm5", "pwm6" is
> > > required property in DT, It doesn't make sense.
> > 
> > I don't understand. Why doesn't that make sense? If your hardware block
> > has 6 PWMs and each can be driven by its own clock, then you need to
> > provide references for each of those clocks, otherwise you won't be able
> > to use them.
> 
> Thank you for your instruction,
> I will add all clock-names and clocks according to
> hardware blocks instead of value of <num-pwms> in DT.
> 
> > >    
> > > So we removed those descriptions and added  
> > > 
> > >    - "pwm1-N": the PWM clocks for each channel 
> > >    
> > >    
> > > But the max number of clocks from the compatible string are still
> > > important information that should be provide in this document.
> > > 
> > > 
> > > What do you think of this?
> > > 
> > >    - "pwm1-N": per PWM clocks for mt2712, the max number of PWM channels
> > > is 8
> > > 
> > >    - "pwm1-N": per PWM clocks for mt7622, the max number of PWM channels
> > > is 6
> > > 
> > >    - "pwm1-N": per PWM clocks for mt7623, the max number of PWM channels
> > > is 5
> > 
> > That's what's in the bindings already, isn't it?
> > 
> > 	 - clocks: phandle and clock specifier of the PWM reference clock.
> > 	 - clock-names: must contain the following, except for MT7628 which
> > 			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
> 
> Yes, You are right, 
> I will keep original description and remove "pwm1-N" from this patch.
> 	   - "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       
> 
> Actually, MT7629 also use "mediatek,mt7622-pwm" as compatible string,
> but it's hardware only support one pwm, so I was wrongly stick by
> expecting "pwm1-N" in clock-names based on "num-pwms" in DT.
> (that we can assign num-pwms to 1 and only provide pwm1 as clock-names)
> 
> Maybe added mt7629 description to this document can solve this simply.
> 	   - "pwm1": the PWM1 clock for mt7629    

Yeah, if mt7629 support only one PWM channel, you should list it's
compatible string separately and also update the driver to reflect the
number of channels associated with the hardware.

Thierry
diff mbox series

Patch

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>;
 	};