diff mbox series

[1/3] dt-bindings: pwm: imx-pwm: Add constraints for #pwm-cells

Message ID 20231106095205.231210-2-alexander.stein@ew.tq-group.com (mailing list archive)
State New, archived
Headers show
Series imx8qm/imx8qxp: Support for inverted PWM | expand

Commit Message

Alexander Stein Nov. 6, 2023, 9:52 a.m. UTC
Only fsl,imx1-pwm comptabile devices use #pwm-cells = <2>. Newer SoCs
supportinverted PWM output, thus #pwm-cells needs to be set to 3.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 .../devicetree/bindings/pwm/imx-pwm.yaml      | 20 ++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

Uwe Kleine-König Nov. 6, 2023, 10:36 a.m. UTC | #1
Hello,

[dropped Philippe Schenker from Cc as his email bounced in the past]

On Mon, Nov 06, 2023 at 10:52:03AM +0100, Alexander Stein wrote:
> Only fsl,imx1-pwm comptabile devices use #pwm-cells = <2>. Newer SoCs
> supportinverted PWM output, thus #pwm-cells needs to be set to 3.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  .../devicetree/bindings/pwm/imx-pwm.yaml      | 20 ++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> index c01dff3b7f843..59a981c0f39ab 100644
> --- a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> @@ -9,9 +9,6 @@ title: Freescale i.MX PWM controller
>  maintainers:
>    - Philipp Zabel <p.zabel@pengutronix.de>
>  
> -allOf:
> -  - $ref: pwm.yaml#
> -
>  properties:
>    "#pwm-cells":
>      description: |
> @@ -74,6 +71,23 @@ required:
>  
>  additionalProperties: false
>  
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: fsl,imx1-pwm
> +    then:
> +      properties:
> +        "#pwm-cells":
> +          enum: [2]

Given that arch/arm/boot/dts/nxp/imx/imx1.dtsi has:

	pwm: pwm@208000 {
		#pwm-cells = <3>;
		compatible = "fsl,imx1-pwm";
		...

this looks wrong.

Best regards
Uwe
Alexander Stein Nov. 6, 2023, 10:49 a.m. UTC | #2
Hello Uwe,

Am Montag, 6. November 2023, 11:36:21 CET schrieb Uwe Kleine-König:
> Hello,
> 
> [dropped Philippe Schenker from Cc as his email bounced in the past]
> 
> On Mon, Nov 06, 2023 at 10:52:03AM +0100, Alexander Stein wrote:
> > Only fsl,imx1-pwm comptabile devices use #pwm-cells = <2>. Newer SoCs
> > supportinverted PWM output, thus #pwm-cells needs to be set to 3.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 
> >  .../devicetree/bindings/pwm/imx-pwm.yaml      | 20 ++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> > b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml index
> > c01dff3b7f843..59a981c0f39ab 100644
> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> > @@ -9,9 +9,6 @@ title: Freescale i.MX PWM controller
> > 
> >  maintainers:
> >    - Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > -allOf:
> > -  - $ref: pwm.yaml#
> > -
> > 
> >  properties:
> >    "#pwm-cells":
> >      description: |
> > 
> > @@ -74,6 +71,23 @@ required:
> >  additionalProperties: false
> > 
> > +allOf:
> > +  - $ref: pwm.yaml#
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: fsl,imx1-pwm
> > +    then:
> > +      properties:
> > +        "#pwm-cells":
> > +          enum: [2]
> 
> Given that arch/arm/boot/dts/nxp/imx/imx1.dtsi has:
> 
> 	pwm: pwm@208000 {
> 		#pwm-cells = <3>;
> 		compatible = "fsl,imx1-pwm";
> 		...
> 
> this looks wrong.

Indeed, something i doesn't match. Checking with [1] section 22.4.1 there are 
no bits regarding output inversion. Also pwm_imx1_apply returns -EINVAL if 
state->polarity != PWM_POLARITY_NORMAL.
So IMO "#pwm-cells = <3>" is wrong for imx1. If fixed to 2, this also matches 
the description for the value of #pwm-cells in imx-pwm.yaml.

Best regards,
Alexander

[1] https://www.nxp.com/docs/en/reference-manual/MC9328MX1RM.pdf

> 
> Best regards
> Uwe
Uwe Kleine-König Nov. 6, 2023, 11:02 a.m. UTC | #3
On Mon, Nov 06, 2023 at 11:49:19AM +0100, Alexander Stein wrote:
> Hello Uwe,
> 
> Am Montag, 6. November 2023, 11:36:21 CET schrieb Uwe Kleine-König:
> > Hello,
> > 
> > [dropped Philippe Schenker from Cc as his email bounced in the past]
> > 
> > On Mon, Nov 06, 2023 at 10:52:03AM +0100, Alexander Stein wrote:
> > > Only fsl,imx1-pwm comptabile devices use #pwm-cells = <2>. Newer SoCs
> > > supportinverted PWM output, thus #pwm-cells needs to be set to 3.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > 
> > >  .../devicetree/bindings/pwm/imx-pwm.yaml      | 20 ++++++++++++++++---
> > >  1 file changed, 17 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> > > b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml index
> > > c01dff3b7f843..59a981c0f39ab 100644
> > > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> > > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> > > @@ -9,9 +9,6 @@ title: Freescale i.MX PWM controller
> > > 
> > >  maintainers:
> > >    - Philipp Zabel <p.zabel@pengutronix.de>
> > > 
> > > -allOf:
> > > -  - $ref: pwm.yaml#
> > > -
> > > 
> > >  properties:
> > >    "#pwm-cells":
> > >      description: |
> > > 
> > > @@ -74,6 +71,23 @@ required:
> > >  additionalProperties: false
> > > 
> > > +allOf:
> > > +  - $ref: pwm.yaml#
> > > +
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: fsl,imx1-pwm
> > > +    then:
> > > +      properties:
> > > +        "#pwm-cells":
> > > +          enum: [2]
> > 
> > Given that arch/arm/boot/dts/nxp/imx/imx1.dtsi has:
> > 
> > 	pwm: pwm@208000 {
> > 		#pwm-cells = <3>;
> > 		compatible = "fsl,imx1-pwm";
> > 		...
> > 
> > this looks wrong.
> 
> Indeed, something i doesn't match. Checking with [1] section 22.4.1 there are 
> no bits regarding output inversion. Also pwm_imx1_apply returns -EINVAL if 
> state->polarity != PWM_POLARITY_NORMAL.
> So IMO "#pwm-cells = <3>" is wrong for imx1. If fixed to 2, this also matches 
> the description for the value of #pwm-cells in imx-pwm.yaml.

The reasoning only works in one direction, i.e. if your PWM supports
inversed polarity you need #pwm-cells = <3>. But there no problem in
using 3 cells for a PWM that doesn't support inversed output. Some might
call it unnecessary verboseness, other might call it consistency.

I'd say the right thing to do here is to stick to = 3 and adapt this
patch and the already existing comment in imx-pwm.yaml.

Best regards
Uwe
Shawn Guo Dec. 5, 2023, 12:36 p.m. UTC | #4
On Mon, Nov 06, 2023 at 10:52:03AM +0100, Alexander Stein wrote:
> Only fsl,imx1-pwm comptabile devices use #pwm-cells = <2>. Newer SoCs
> supportinverted PWM output, thus #pwm-cells needs to be set to 3.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

As suggested by Uwe, I picked this up too.

Shawn
Shawn Guo Dec. 5, 2023, 12:37 p.m. UTC | #5
On Tue, Dec 5, 2023 at 8:36 PM Shawn Guo <shawnguo@kernel.org> wrote:
>
> On Mon, Nov 06, 2023 at 10:52:03AM +0100, Alexander Stein wrote:
> > Only fsl,imx1-pwm comptabile devices use #pwm-cells = <2>. Newer SoCs
> > supportinverted PWM output, thus #pwm-cells needs to be set to 3.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>
> As suggested by Uwe, I picked this up too.

Sorry, I picked v2 up actually.

Shawn
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
index c01dff3b7f843..59a981c0f39ab 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
@@ -9,9 +9,6 @@  title: Freescale i.MX PWM controller
 maintainers:
   - Philipp Zabel <p.zabel@pengutronix.de>
 
-allOf:
-  - $ref: pwm.yaml#
-
 properties:
   "#pwm-cells":
     description: |
@@ -74,6 +71,23 @@  required:
 
 additionalProperties: false
 
+allOf:
+  - $ref: pwm.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: fsl,imx1-pwm
+    then:
+      properties:
+        "#pwm-cells":
+          enum: [2]
+    else:
+      properties:
+        "#pwm-cells":
+          enum: [3]
+
 examples:
   - |
     #include <dt-bindings/clock/imx5-clock.h>