[v4,3/5] dt-bindings: hwmon: Document adt7475 invert-pwm property
diff mbox series

Message ID 20200221041631.10960-4-chris.packham@alliedtelesis.co.nz
State Superseded
Headers show
Series
  • hwmon: (adt7475) attenuator bypass and pwm invert
Related show

Commit Message

Chris Packham Feb. 21, 2020, 4:16 a.m. UTC
Add binding information for the invert-pwm property.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v4:
    - use $ref uint32 and enum
    - add adi vendor prefix
    
    Cahnges in v3:
    - new

 Documentation/devicetree/bindings/hwmon/adt7475.yaml | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Rob Herring Feb. 21, 2020, 3:40 p.m. UTC | #1
On Thu, Feb 20, 2020 at 10:16 PM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
>
> Add binding information for the invert-pwm property.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Notes:
>     Changes in v4:
>     - use $ref uint32 and enum
>     - add adi vendor prefix
>
>     Cahnges in v3:
>     - new
>
>  Documentation/devicetree/bindings/hwmon/adt7475.yaml | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> index e40612ee075f..6a358b30586c 100644
> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> @@ -50,6 +50,17 @@ patternProperties:
>       - $ref: /schemas/types.yaml#/definitions/uint32
>       - enum: [0, 1]
>
> +  "^adi,invert-pwm[1-3]$":
> +    description: |
> +      Configures the pwm output to use inverted logic. If set to 1
> +      the pwm uses a logic low output for 100% duty cycle. If set
> +      to 0 the pwm uses a logic high output for 100% duty cycle.
> +      If the property is absent the pwm retains it's configuration
> +      from the bios/bootloader.

I believe we already have an inverted flag for consumers. That doesn't
work if you don't have a consumer described in DT, but then the
question is should you? Or is this something the user will want to
configure from userspace.

The problem with 'invert' properties is they assume you know what the
not inverted state is. I would also make this an array:

adi,pwm-active-state = <1 0 0 1>; // PWM1 and PWM2 active low

And not present means <1 1 1 1>.

Rob
Guenter Roeck Feb. 21, 2020, 3:52 p.m. UTC | #2
On Fri, Feb 21, 2020 at 09:40:00AM -0600, Rob Herring wrote:
> On Thu, Feb 20, 2020 at 10:16 PM Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
> >
> > Add binding information for the invert-pwm property.
> >
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> >
> > Notes:
> >     Changes in v4:
> >     - use $ref uint32 and enum
> >     - add adi vendor prefix
> >
> >     Cahnges in v3:
> >     - new
> >
> >  Documentation/devicetree/bindings/hwmon/adt7475.yaml | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > index e40612ee075f..6a358b30586c 100644
> > --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > @@ -50,6 +50,17 @@ patternProperties:
> >       - $ref: /schemas/types.yaml#/definitions/uint32
> >       - enum: [0, 1]
> >
> > +  "^adi,invert-pwm[1-3]$":
> > +    description: |
> > +      Configures the pwm output to use inverted logic. If set to 1
> > +      the pwm uses a logic low output for 100% duty cycle. If set
> > +      to 0 the pwm uses a logic high output for 100% duty cycle.
> > +      If the property is absent the pwm retains it's configuration
> > +      from the bios/bootloader.
> 
> I believe we already have an inverted flag for consumers. That doesn't
> work if you don't have a consumer described in DT, but then the
> question is should you? Or is this something the user will want to
> configure from userspace.
> 

Normally that is a system property. It is difficult to imagine
that it would ever have to be configured from userspace at runtime.
Most of the time users won't have any idea, and the board datasheet
(if avaible) won't list such information.

Guenter
Rob Herring Feb. 21, 2020, 10:04 p.m. UTC | #3
On Fri, Feb 21, 2020 at 9:53 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Feb 21, 2020 at 09:40:00AM -0600, Rob Herring wrote:
> > On Thu, Feb 20, 2020 at 10:16 PM Chris Packham
> > <chris.packham@alliedtelesis.co.nz> wrote:
> > >
> > > Add binding information for the invert-pwm property.
> > >
> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > ---
> > >
> > > Notes:
> > >     Changes in v4:
> > >     - use $ref uint32 and enum
> > >     - add adi vendor prefix
> > >
> > >     Cahnges in v3:
> > >     - new
> > >
> > >  Documentation/devicetree/bindings/hwmon/adt7475.yaml | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > index e40612ee075f..6a358b30586c 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > @@ -50,6 +50,17 @@ patternProperties:
> > >       - $ref: /schemas/types.yaml#/definitions/uint32
> > >       - enum: [0, 1]
> > >
> > > +  "^adi,invert-pwm[1-3]$":
> > > +    description: |
> > > +      Configures the pwm output to use inverted logic. If set to 1
> > > +      the pwm uses a logic low output for 100% duty cycle. If set
> > > +      to 0 the pwm uses a logic high output for 100% duty cycle.
> > > +      If the property is absent the pwm retains it's configuration
> > > +      from the bios/bootloader.
> >
> > I believe we already have an inverted flag for consumers. That doesn't
> > work if you don't have a consumer described in DT, but then the
> > question is should you? Or is this something the user will want to
> > configure from userspace.
> >
>
> Normally that is a system property. It is difficult to imagine
> that it would ever have to be configured from userspace at runtime.
> Most of the time users won't have any idea, and the board datasheet
> (if avaible) won't list such information.

Yes, I agree and for those cases I'd expect the consumer is described
in DT too. I read and refreshed my memory on this binding after
sending this. I believe this is for a fan which I'd expect to be in DT
as fans need power and there's different types.

The userspace case I was thinking of was more the hobbyist boards with
PWM to a connector.

Rob

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
index e40612ee075f..6a358b30586c 100644
--- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
@@ -50,6 +50,17 @@  patternProperties:
      - $ref: /schemas/types.yaml#/definitions/uint32
      - enum: [0, 1]
 
+  "^adi,invert-pwm[1-3]$":
+    description: |
+      Configures the pwm output to use inverted logic. If set to 1
+      the pwm uses a logic low output for 100% duty cycle. If set
+      to 0 the pwm uses a logic high output for 100% duty cycle.
+      If the property is absent the pwm retains it's configuration
+      from the bios/bootloader.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum: [0, 1]
+
 required:
   - compatible
   - reg
@@ -65,6 +76,7 @@  examples:
         reg = <0x2e>;
         adi,bypass-attenuator-in0 = <1>;
         adi,bypass-attenuator-in1 = <0>;
+        adi,invert-pwm1 = <1>;
       };
     };