| Message ID | 20240602-pwm-gpio-v6-1-e8f6ec9cc783@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: Add GPIO PWM driver | expand |
On Sun, Jun 02, 2024 at 10:33:08PM +0200, Linus Walleij wrote: > From: Nicola Di Lieto <nicola.dilieto@gmail.com> > > Add bindings for PWM modulated by GPIO. > Shouldn't the bindings be added after the driver? > Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com> > Co-developed-by: Stefan Wahren <wahrenst@gmx.net> > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Reviewed-by: Dhruva Gole <d-gole@ti.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > .../devicetree/bindings/pwm/pwm-gpio.yaml | 42 ++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml b/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml > new file mode 100644 > index 000000000000..1a1281e0fbd7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml > @@ -0,0 +1,42 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/pwm-gpio.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Generic software PWM for modulating GPIOs > + > +maintainers: > + - Stefan Wahren <wahrenst@gmx.net> > + > +allOf: > + - $ref: pwm.yaml# > + > +properties: > + compatible: > + const: pwm-gpio > + > + "#pwm-cells": > + const: 3 > + What is the significance of the 3 here? Sorry if that is obvious, but it isn't to me. Cheers, Kent. > + gpios: > + description: > + GPIO to be modulated > + maxItems: 1 > + > +required: > + - compatible > + - "#pwm-cells" > + - gpios > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + pwm { > + #pwm-cells = <3>; > + compatible = "pwm-gpio"; > + gpios = <&gpio 1 GPIO_ACTIVE_HIGH>; > + }; > > -- > 2.45.1 >
On 04/06/2024 04:51, Kent Gibson wrote: > On Sun, Jun 02, 2024 at 10:33:08PM +0200, Linus Walleij wrote: >> From: Nicola Di Lieto <nicola.dilieto@gmail.com> >> >> Add bindings for PWM modulated by GPIO. >> > > Shouldn't the bindings be added after the driver? No. See submitting patches document. Best regards, Krzysztof
On Tue, Jun 04, 2024 at 08:21:32AM +0200, Krzysztof Kozlowski wrote: > On 04/06/2024 04:51, Kent Gibson wrote: > > On Sun, Jun 02, 2024 at 10:33:08PM +0200, Linus Walleij wrote: > >> From: Nicola Di Lieto <nicola.dilieto@gmail.com> > >> > >> Add bindings for PWM modulated by GPIO. > >> > > > > Shouldn't the bindings be added after the driver? > > No. See submitting patches document. > Hmmm, ok, so "5. The Documentation/ portion of the patch should come in the series before the code implementing the binding."[1]? It just seems odd that you document something that doesn't exist yet. Cheers, Kent. [1] https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html
On 04/06/2024 09:34, Kent Gibson wrote: > On Tue, Jun 04, 2024 at 08:21:32AM +0200, Krzysztof Kozlowski wrote: >> On 04/06/2024 04:51, Kent Gibson wrote: >>> On Sun, Jun 02, 2024 at 10:33:08PM +0200, Linus Walleij wrote: >>>> From: Nicola Di Lieto <nicola.dilieto@gmail.com> >>>> >>>> Add bindings for PWM modulated by GPIO. >>>> >>> >>> Shouldn't the bindings be added after the driver? >> >> No. See submitting patches document. >> > > Hmmm, ok, so "5. The Documentation/ portion of the patch should come in > the series before the code implementing the binding."[1]? > > It just seems odd that you document something that doesn't exist yet. It's logical. First you define the ABI for every user, then you implement the ABI. Do you first implement software and then design? Or first implement then write interface (API) for it? Best regards, Krzysztof
On Tue, Jun 04, 2024 at 09:42:25AM +0200, Krzysztof Kozlowski wrote: > On 04/06/2024 09:34, Kent Gibson wrote: > > On Tue, Jun 04, 2024 at 08:21:32AM +0200, Krzysztof Kozlowski wrote: > >> On 04/06/2024 04:51, Kent Gibson wrote: > >>> On Sun, Jun 02, 2024 at 10:33:08PM +0200, Linus Walleij wrote: > >>>> From: Nicola Di Lieto <nicola.dilieto@gmail.com> > >>>> > >>>> Add bindings for PWM modulated by GPIO. > >>>> > >>> > >>> Shouldn't the bindings be added after the driver? > >> > >> No. See submitting patches document. > >> > > > > Hmmm, ok, so "5. The Documentation/ portion of the patch should come in > > the series before the code implementing the binding."[1]? > > > > It just seems odd that you document something that doesn't exist yet. > > It's logical. First you define the ABI for every user, then you > implement the ABI. Do you first implement software and then design? Or > first implement then write interface (API) for it? > I don't see the relevance of your analogy. This isn't design, this is roll out, i.e. publishing. Not publishing the ABI until the implementation is available seems more logical to me. But as long as it is all in the same series then whatever. Cheers, Kent.
On 04/06/2024 10:16, Kent Gibson wrote: > On Tue, Jun 04, 2024 at 09:42:25AM +0200, Krzysztof Kozlowski wrote: >> On 04/06/2024 09:34, Kent Gibson wrote: >>> On Tue, Jun 04, 2024 at 08:21:32AM +0200, Krzysztof Kozlowski wrote: >>>> On 04/06/2024 04:51, Kent Gibson wrote: >>>>> On Sun, Jun 02, 2024 at 10:33:08PM +0200, Linus Walleij wrote: >>>>>> From: Nicola Di Lieto <nicola.dilieto@gmail.com> >>>>>> >>>>>> Add bindings for PWM modulated by GPIO. >>>>>> >>>>> >>>>> Shouldn't the bindings be added after the driver? >>>> >>>> No. See submitting patches document. >>>> >>> >>> Hmmm, ok, so "5. The Documentation/ portion of the patch should come in >>> the series before the code implementing the binding."[1]? >>> >>> It just seems odd that you document something that doesn't exist yet. >> >> It's logical. First you define the ABI for every user, then you >> implement the ABI. Do you first implement software and then design? Or >> first implement then write interface (API) for it? >> > > I don't see the relevance of your analogy. > This isn't design, this is roll out, i.e. publishing. > > Not publishing the ABI until the implementation is available seems more > logical to me. But as long as it is all in the same series then whatever. Anyway, that's how Bindings and kernel development works. First you document the ABI, then you implement it. Not the other way around. Best regards, Krzysztof
Some buzz around the patch made me notice this: On Sun, Jun 2, 2024 at 10:33 PM Linus Walleij <linus.walleij@linaro.org> wrote: > + "#pwm-cells": > + const: 3 I guess we should document these three cells: - First cell must be 0 - just the one PWM on the one GPIO pin - Second cell should be the default period that can be changed by software - Third cell is polarity, 0 or PWM_POLARITY_INVERTED I guess this is 3 not 2 because the maintainers previously said they wanted it like this? (I haven't read all old mail, nor do I remember...) The #pwm-cells are currently not properly specified in the bindings: for example pwm-tiecap.yaml says "See pwm.yaml in this directory for a description of the cells format." and that file says nothing about the cells and what they are for, should I send a separate patch for that? Yours, Linus Walleij
On Tue, Jun 04, 2024 at 03:38:41PM +0200, Linus Walleij wrote: > Some buzz around the patch made me notice this: > > On Sun, Jun 2, 2024 at 10:33 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > + "#pwm-cells": > > + const: 3 > > I guess we should document these three cells: > - First cell must be 0 - just the one PWM on the one GPIO pin > - Second cell should be the default period that can be changed by software > - Third cell is polarity, 0 or PWM_POLARITY_INVERTED > > I guess this is 3 not 2 because the maintainers previously said they wanted > it like this? (I haven't read all old mail, nor do I remember...) > > The #pwm-cells are currently not properly specified in the bindings: for example > pwm-tiecap.yaml says "See pwm.yaml in this directory for a description > of the cells format." > and that file says nothing about the cells and what they are for, should > I send a separate patch for that? Does this suffice? https://lore.kernel.org/linux-pwm/20240517-patient-stingily-30611f73e792@spud/
On Tue, Jun 4, 2024 at 4:14 PM Conor Dooley <conor@kernel.org> wrote: > > The #pwm-cells are currently not properly specified in the bindings: for example > > pwm-tiecap.yaml says "See pwm.yaml in this directory for a description > > of the cells format." > > and that file says nothing about the cells and what they are for, should > > I send a separate patch for that? > > Does this suffice? > https://lore.kernel.org/linux-pwm/20240517-patient-stingily-30611f73e792@spud/ Indeed. You can add: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> for the above patch! Yours, Linus Walleij
On Tue, Jun 04, 2024 at 10:54:26PM +0200, Linus Walleij wrote: > On Tue, Jun 4, 2024 at 4:14 PM Conor Dooley <conor@kernel.org> wrote: > > > > The #pwm-cells are currently not properly specified in the bindings: for example > > > pwm-tiecap.yaml says "See pwm.yaml in this directory for a description > > > of the cells format." > > > and that file says nothing about the cells and what they are for, should > > > I send a separate patch for that? > > > > Does this suffice? > > https://lore.kernel.org/linux-pwm/20240517-patient-stingily-30611f73e792@spud/ > > Indeed. You can add: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> for the above patch! Seemingly Uwe already queued it, so should end up in 6.11: https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git/commit/?h=pwm/for-next&id=603e1cf3b21a2451b99a5d06dca9e511dff0a294
diff --git a/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml b/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml new file mode 100644 index 000000000000..1a1281e0fbd7 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml @@ -0,0 +1,42 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/pwm-gpio.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Generic software PWM for modulating GPIOs + +maintainers: + - Stefan Wahren <wahrenst@gmx.net> + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + const: pwm-gpio + + "#pwm-cells": + const: 3 + + gpios: + description: + GPIO to be modulated + maxItems: 1 + +required: + - compatible + - "#pwm-cells" + - gpios + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + pwm { + #pwm-cells = <3>; + compatible = "pwm-gpio"; + gpios = <&gpio 1 GPIO_ACTIVE_HIGH>; + };