| Message ID | 20231106095205.231210-2-alexander.stein@ew.tq-group.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | imx8qm/imx8qxp: Support for inverted PWM | expand |
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
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
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
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
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 --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>
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(-)