| Message ID | 1566457123-20791-8-git-send-email-sam.shih@mediatek.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add mt7629 and fix mt7628 pwm | expand |
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
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
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
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
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>; };