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:
signature.asc
Description: This is a digitally signed message part.
