Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

[-- Attachment #1: Type: text/plain, Size: 5505 bytes --]

Hi Thierry,

On Thursday 11 July 2013 08:36:00 Thierry Reding wrote:
> On Thu, Jul 11, 2013 at 04:37:48PM +0200, Laurent Pinchart wrote:
> [...]
> 
> > diff --git a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
> > b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt index
> > de0eaed..be09be4 100644
> > --- a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
> > +++ b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
> > 
> > @@ -4,9 +4,9 @@ Required properties:
> >  - compatible: should be "atmel,tcb-pwm"
> >  - #pwm-cells: Should be 3.  The first cell specifies the per-chip index
> >    of the PWM to use, the second cell is the period in nanoseconds and
> > -  bit 0 in the third cell is used to encode the polarity of PWM output.
> > -  Set bit 0 of the third cell in PWM specifier to 1 for inverse polarity
> > &
> > -  set to 0 for normal polarity.
> > +  the third cell is used to encode the polarity of PWM output. Set the
> > +  PWM_POLARITY_NORMAL flag for normal polarity or the
> > PWM_POLARITY_INVERSED
> > +  flag for inverted polarity. PWM flags are defined in <dt-
bindings/pwm/pwm.h>.
> >  - tc-block: The Timer Counter block to use as a PWM chip.
> 
> >  Example:
>
> I'd prefer for the original text to stay in place and the reference to the
> dt-bindings/pwm/pwm.h file to go below that block. The reason is that
> perhaps somebody will look at an older version of a given DT (with the
> numerical value) and not have access to the include and therefore not know
> which flag was being set by just looking at the documentation. I'm also not
> sure about what the plans are with respect to shipping device trees in a
> separate repository and if they are, whether that repository would ship the
> includes as well.
> 
> Another issue might be that people without access to recent versions of
> DTC won't be able to use the new #include feature, so keeping the
> documentation backwards compatible seems like a good idea.
> 
> Perhaps the include file should even only be mentioned in the general
> PWM binding documentation.
> 
> Perhaps Grant and Rob (Cc'ed) can comment on how they want to handle
> this.

I'll comment on this in a reply to Stephen.

> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-samsung.txt
> > b/Documentation/devicetree/bindings/pwm/pwm-samsung.txt index
> > ac67c68..bece18b 100644
> > --- a/Documentation/devicetree/bindings/pwm/pwm-samsung.txt
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-samsung.txt
> > 
> > @@ -24,8 +24,9 @@ Required properties:
> >       - phandle to PWM controller node
> >       - index of PWM channel (from 0 to 4)
> >       - PWM signal period in nanoseconds
> > 
> > -     - bitmask of optional PWM flags:
> > -        0x1 - invert PWM signal
> > +     - bitmask of optional PWM flags as defined in
> > <dt-bindings/pwm/pwm.h>: +        PWM_POLARITY_NORMAL - normal polarity
> > +	PWM_POLARITY_INVERSED - inverted polarity
> 
> This part mixes spaces and tabs for indentation. Please stick to spaces.

OK.

> > diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt
> > b/Documentation/devicetree/bindings/pwm/pwm.txt index 06e6724..38c357a
> > 100644
> > --- a/Documentation/devicetree/bindings/pwm/pwm.txt
> > +++ b/Documentation/devicetree/bindings/pwm/pwm.txt
> > @@ -43,13 +43,15 @@ because the name "backlight" would be used as fallback
> > anyway.> 
> >  pwm-specifier typically encodes the chip-relative PWM number and the PWM
> >  period in nanoseconds.
> > 
> > -Optionally, the pwm-specifier can encode a number of flags in a third
> > cell:
> > -- bit 0: PWM signal polarity (0: normal polarity, 1: inverse polarity)
> > +Optionally, the pwm-specifier can encode a number of flags (defined in
> > +<dt-bindings/gpio/gpio.h>) in a third cell:
> > +- PWM_POLARITY_NORMAL: use the normal PWM signal polarity
> > +- PWM_POLARITY_INVERSED: invert the PWM signal polarity
> 
> You'd have a hard time finding those in the GPIO header. =)

Oops :-)

Will fix.

> > diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h
> > new file mode 100644
> > index 0000000..f82be30
> > --- /dev/null
> > +++ b/include/dt-bindings/pwm/pwm.h
> > @@ -0,0 +1,15 @@
> > +/*
> > + * This header provides constants for most PWM bindings.
> > + *
> > + * Most PWM bindings can include a flags cell as part of the PWM 
> > specifier.
> > + * In most cases, the format of the flags cell uses the standard values
> > + * defined in this header.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_PWM_PWM_H
> > +#define _DT_BINDINGS_PWM_PWM_H
> > +
> > +#define PWM_POLARITY_NORMAL			(0 << 0)
> > +#define PWM_POLARITY_INVERSED			(1 << 0)
> > +
> > +#endif
> 
> I think this should go into a patch separate from the DT changes above
> because they'll likely go in via different trees. Or maybe they won't,
> but it'd still be good to introduce the header first and only then
> change the DTS files.

I'll fix that as well. Please see my reply to Stephen for details.

> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> 
> [...]
> 
> >  enum pwm_polarity {
> > 
> > -	PWM_POLARITY_NORMAL,
> > -	PWM_POLARITY_INVERSED,
> > +	PWM_POLARITY_NORMAL = 0,
> > +	PWM_POLARITY_INVERSED = 1,
> 
> That's what the enum values will be by default, so there's no need to be
> explicit here.

Please see my reply to Stephen as well. I'll at least replace this change with 
a comment, or remove enum pwm_polarity completely if that's the preferred 
solution.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]