| Message ID | 1373553468-6564-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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. > 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. =) > 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. > 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. Thierry
On 07/11/2013 08:37 AM, Laurent Pinchart wrote: > Define PWM_POLARITY_NORMAL and PWM_POLARITY_INVERTED macros in > include/dt-bindings/pwm/pwm.h to be used by device tree sources. > Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt | 6 +++--- > Documentation/devicetree/bindings/pwm/pwm-samsung.txt | 5 +++-- > Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 5 +++-- > Documentation/devicetree/bindings/pwm/pwm.txt | 8 +++++--- > Documentation/devicetree/bindings/pwm/vt8500-pwm.txt | 5 +++-- > arch/arm/boot/dts/am335x-evm.dts | 3 ++- > arch/arm/boot/dts/am335x-evmsk.dts | 3 ++- > arch/arm/boot/dts/wm8850-w70v2.dts | 3 ++- > include/dt-bindings/pwm/pwm.h | 15 +++++++++++++++ > include/linux/pwm.h | 4 ++-- I think this needs to be separate patches; at least the new pwm.h should be introduced separately to the board-specific *.dts edits, and perhaps further split up? That way, the one patch that introduces <dt-bindings/pwm.h> would be available to be merged into any other tree that wanted to take patches to use the new defines. > 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, > }; Rather than manually editing that to ensure the enum matches the DT bindings header, the whole point of making a separate <dt-bindings/...> directory was that drivers could include the binding header files directly to avoid having to duplicate the constant definitions. Can't <linux/pwm.h> include <dt-bindings/pwm.h> and remove that enum?
On 07/11/2013 09:36 AM, 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. I disagree here. The whole point of creating header files for the constants in binding definitions was so that you wouldn't have to duplicate all the values into the binding definitions. Rather, you'd simply say "see <dt-bindings/xxx.h>". A comment on the original patch: You're creating <dt-bindings/pwm/pwm.h>. That filename sounds entirely generic, as if the values are defining something PWM-wide rather than specific to individual PWM bindings. As such, I think the text in each individual binding should simply say something like: ----- the third cell encodes standard PWM flags, as defined in <dt-bindings/pwm/pwm.h>. ----- Similarly, pwm.txt should mention that standard flags exist, and refer to that same file for definitions. > 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. It's pretty trivial to find the header that defines the values; just grab the latest source. If you're looking at an old version of the .dts file, it's almost certain that's within the context of an old Linux kernel soruce tree, in which case you trivially have access to the old version of the binding document that spelled out the values. > 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. It would have to. That separate repository would be upstream for <dt-bindings/> files, which would be imported into the kernel periodically as drivers wanted to make use of any new headers/values defined there. > 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. The dtc source tree is duplicated into the kernel source tree, so that isn't an issue for now. Besides, the dtc version is an entirely unrelated issue to how the documentation is written. > Perhaps the include file should even only be mentioned in the > general PWM binding documentation. I wondered about that too. It's slightly indirect in that you'd read foo-pwm.txt which would say something like: ----- the third cell encodes standard PWM flags, as defined by the core PWM binding in pwm.txt in this directory. ----- and pwm.txt would say: ----- The PWM specifier should include a flags cell to contain standard PWM flags. Values for that cell are defined in <dt-bindings/pwm/pwm.h>. ----- Which is somewhat like following a lot of symlinks. Still, I agree that's arguably the correct thing to do.
On Thu, Jul 11, 2013 at 11:50:48AM -0600, Stephen Warren wrote: > On 07/11/2013 09:36 AM, 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. > > I disagree here. The whole point of creating header files for the > constants in binding definitions was so that you wouldn't have to > duplicate all the values into the binding definitions. Rather, you'd > simply say "see <dt-bindings/xxx.h>". But that's not something that this patch solves. And it could be solved even in the absence of the header file defining the symbolic constants. If all the standard flags that dt-bindings/pwm/pwm.txt now specifies were to be listed in pwm.txt (they actually are) then referring to that document as the canonical source works equally well. > > 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. > > It's pretty trivial to find the header that defines the values; just > grab the latest source. If you're looking at an old version of the > .dts file, it's almost certain that's within the context of an old > Linux kernel soruce tree, in which case you trivially have access to > the old version of the binding document that spelled out the values. > > > 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. > > It would have to. That separate repository would be upstream for > <dt-bindings/> files, which would be imported into the kernel > periodically as drivers wanted to make use of any new headers/values > defined there. If we can take both of the above for granted, then sure, let's refer to the header from within the generic pwm.txt file and add a reference to that in bindings for drivers that use the standard flags. > > 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. > > The dtc source tree is duplicated into the kernel source tree, so that > isn't an issue for now. > > Besides, the dtc version is an entirely unrelated issue to how the > documentation is written. Well, not really. If the documentation specifies the binding in a way that the DTC can't handle that's still a problem. People will end up with a DTS that their DTC can't compile. I guess that can be resolved by adding a note to the upstream device tree repository about the minimum required version of DTC. > > Perhaps the include file should even only be mentioned in the > > general PWM binding documentation. > > I wondered about that too. It's slightly indirect in that you'd read > foo-pwm.txt which would say something like: > > ----- > the third cell encodes standard PWM flags, as defined by the core PWM > binding in pwm.txt in this directory. > ----- > > and pwm.txt would say: > > ----- > The PWM specifier should include a flags cell to contain standard PWM > flags. Values for that cell are defined in <dt-bindings/pwm/pwm.h>. > ----- > > Which is somewhat like following a lot of symlinks. Still, I agree > that's arguably the correct thing to do. Yes, I like this a whole lot better than the current situation. It gets us almost automatic consistency, which is harder to do with the current approach. Thierry
On 07/11/2013 01:32 PM, Thierry Reding wrote: > On Thu, Jul 11, 2013 at 11:50:48AM -0600, Stephen Warren wrote: >> On 07/11/2013 09:36 AM, 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. >> >> I disagree here. The whole point of creating header files for >> the constants in binding definitions was so that you wouldn't >> have to duplicate all the values into the binding definitions. >> Rather, you'd simply say "see <dt-bindings/xxx.h>". > > But that's not something that this patch solves. Well, if the comments I made on the patch re: that <linux/pwm.h> should simply #include <dt-bindings/pwm/pwm.h> instead of duplicating the constants, then yet this patch will solve that. There will be a single place where the constants are defined. > And it could be solved even in the absence of the header file > defining the symbolic constants. If all the standard flags that > dt-bindings/pwm/pwm.txt now specifies were to be listed in pwm.txt > (they actually are) then referring to that document as the > canonical source works equally well. If that's all the happens, then there will still be duplication between pwm.txt and <linux/pwm.h>. > If we can take both of the above for granted, then sure, let's > refer to the header from within the generic pwm.txt file and add a > reference to that in bindings for drivers that use the standard > flags. > >>> 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. >> >> The dtc source tree is duplicated into the kernel source tree, so >> that isn't an issue for now. >> >> Besides, the dtc version is an entirely unrelated issue to how >> the documentation is written. > > Well, not really. If the documentation specifies the binding in a > way that the DTC can't handle that's still a problem. People will > end up with a DTS that their DTC can't compile. I guess that can be > resolved by adding a note to the upstream device tree repository > about the minimum required version of DTC. Yes, the separate repository would obviously require a version of dtc that's able to compile the files there; i.e. a version equivalent to what's already in the kernel tree (upstream 1.4.0 specifically). Again, right now, all of the binding docs, the *.dts files, and the dtc required to use them are part of the kernel; a single package, so there's no scope for issues re: using dtc features that aren't supported. If those components get separated later, obviously there will be a requirement to install a specific version of dtc to use with the separated *.dts and binding files.
Hi Stephen, On Thursday 11 July 2013 11:40:37 Stephen Warren wrote: > On 07/11/2013 08:37 AM, Laurent Pinchart wrote: > > Define PWM_POLARITY_NORMAL and PWM_POLARITY_INVERTED macros in > > include/dt-bindings/pwm/pwm.h to be used by device tree sources. > > > > Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt | 6 +++--- > > Documentation/devicetree/bindings/pwm/pwm-samsung.txt | 5 +++-- > > Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 5 +++-- > > Documentation/devicetree/bindings/pwm/pwm.txt | 8 +++++--- > > Documentation/devicetree/bindings/pwm/vt8500-pwm.txt | 5 +++-- > > arch/arm/boot/dts/am335x-evm.dts | 3 ++- > > arch/arm/boot/dts/am335x-evmsk.dts | 3 ++- > > arch/arm/boot/dts/wm8850-w70v2.dts | 3 ++- > > include/dt-bindings/pwm/pwm.h | 15 ++++++++++++ > > include/linux/pwm.h | 4 ++-- > > I think this needs to be separate patches; at least the new pwm.h should > be introduced separately to the board-specific *.dts edits, and perhaps > further split up? What about splitting it in three patches that - add the include/dt-bindings/pwm/pwm.h header, and update include/linux/pwm.h and Documentation/devicetree/bindings/pwm/pwm.txt - update the rest of the documentation - update the .dts files > That way, the one patch that introduces <dt-bindings/pwm.h> would be > available to be merged into any other tree that wanted to take patches > to use the new defines. > > > 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, > > > > }; > > Rather than manually editing that to ensure the enum matches the DT bindings > header, the whole point of making a separate <dt-bindings/...> directory was > that drivers could include the binding header files directly to avoid having > to duplicate the constant definitions. Can't <linux/pwm.h> include <dt- > bindings/pwm.h> and remove that enum? We could do that, but we would then need to modify all drivers to replace enum_pwm_polarity with unsigned int. Thierry, what's your opinion on this ? Replying to a comment from another e-mail, I know that the above change to include/linux/pwm.h is not strictly needed as the enum values are already correct. The point of specifying the enum values explicitly was to hint that the values matter and should not be changed. A comment in the source would probably be more appropriate though.
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.
Hi, On Thursday 11 July 2013 14:06:44 Stephen Warren wrote: > On 07/11/2013 01:32 PM, Thierry Reding wrote: > > On Thu, Jul 11, 2013 at 11:50:48AM -0600, Stephen Warren wrote: > >> On 07/11/2013 09:36 AM, 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. > >> > >> I disagree here. The whole point of creating header files for the > >> constants in binding definitions was so that you wouldn't have to > >> duplicate all the values into the binding definitions. Rather, you'd > >> simply say "see <dt-bindings/xxx.h>". > > > > But that's not something that this patch solves. > > Well, if the comments I made on the patch re: that <linux/pwm.h> should > simply #include <dt-bindings/pwm/pwm.h> instead of duplicating the > constants, then yet this patch will solve that. There will be a single place > where the constants are defined. As explained in another reply, this would require replacing the enum with an unsigned int. I can write a patch if we agree on this. > > And it could be solved even in the absence of the header file defining the > > symbolic constants. If all the standard flags that dt-bindings/pwm/pwm.txt > > now specifies were to be listed in pwm.txt (they actually are) then > > referring to that document as the canonical source works equally well. > > If that's all the happens, then there will still be duplication > between pwm.txt and <linux/pwm.h>. I've explicitly mentioned the flags in individual DT bindings to ease adding new flags in the future. At the moment the defined flags are either all supported or not used at all by drivers. If we later add a new flag supported by a subset of drivers only the driver bindings should list supported flags for each driver. I'm fine with removing the explicit mentions of individual flags right now and adding it back when needed if you think that's better. > > If we can take both of the above for granted, then sure, let's refer to > > the header from within the generic pwm.txt file and add a reference to > > that in bindings for drivers that use the standard flags. > > > >>> 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. > >> > >> The dtc source tree is duplicated into the kernel source tree, so that > >> isn't an issue for now. > >> > >> Besides, the dtc version is an entirely unrelated issue to how the > >> documentation is written. > > > > Well, not really. If the documentation specifies the binding in a way that > > the DTC can't handle that's still a problem. People will end up with a DTS > > that their DTC can't compile. I guess that can be resolved by adding a > > note to the upstream device tree repository about the minimum required > > version of DTC. > > Yes, the separate repository would obviously require a version of dtc that's > able to compile the files there; i.e. a version equivalent to what's already > in the kernel tree (upstream 1.4.0 specifically). > > Again, right now, all of the binding docs, the *.dts files, and the dtc > required to use them are part of the kernel; a single package, so there's no > scope for issues re: using dtc features that aren't supported. If those > components get separated later, obviously there will be a requirement to > install a specific version of dtc to use with the separated *.dts and > binding files.
On 07/12/2013 04:41 AM, Laurent Pinchart wrote: > Hi Stephen, > > On Thursday 11 July 2013 11:40:37 Stephen Warren wrote: >> On 07/11/2013 08:37 AM, Laurent Pinchart wrote: >>> Define PWM_POLARITY_NORMAL and PWM_POLARITY_INVERTED macros in >>> include/dt-bindings/pwm/pwm.h to be used by device tree sources. >>> >>> Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt | 6 +++--- >>> Documentation/devicetree/bindings/pwm/pwm-samsung.txt | 5 +++-- >>> Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 5 +++-- >>> Documentation/devicetree/bindings/pwm/pwm.txt | 8 +++++--- >>> Documentation/devicetree/bindings/pwm/vt8500-pwm.txt | 5 +++-- >>> arch/arm/boot/dts/am335x-evm.dts | 3 ++- >>> arch/arm/boot/dts/am335x-evmsk.dts | 3 ++- >>> arch/arm/boot/dts/wm8850-w70v2.dts | 3 ++- >>> include/dt-bindings/pwm/pwm.h | 15 ++++++++++++ >>> include/linux/pwm.h | 4 ++-- >> >> I think this needs to be separate patches; at least the new pwm.h should >> be introduced separately to the board-specific *.dts edits, and perhaps >> further split up? > > What about splitting it in three patches that > > - add the include/dt-bindings/pwm/pwm.h header, and update include/linux/pwm.h > and Documentation/devicetree/bindings/pwm/pwm.txt > > - update the rest of the documentation > > - update the .dts files I think that sounds reasonable. >> That way, the one patch that introduces <dt-bindings/pwm.h> would be >> available to be merged into any other tree that wanted to take patches >> to use the new defines. >> >>> 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, >>> >>> }; >> >> Rather than manually editing that to ensure the enum matches the DT bindings >> header, the whole point of making a separate <dt-bindings/...> directory was >> that drivers could include the binding header files directly to avoid having >> to duplicate the constant definitions. Can't <linux/pwm.h> include <dt- >> bindings/pwm.h> and remove that enum? > > We could do that, but we would then need to modify all drivers to replace > enum_pwm_polarity with unsigned int. Thierry, what's your opinion on this ? Or perhaps we could keep the enums around, but force the values to match the DT constants: enum pwm_polarity { PWM_POLARITY_NORMAL = PWM_POLARITY_NORMAL, PWM_POLARITY_INVERTED = PWM_POLARITY_INVERTED, }; (although obviously you'd need to avoid the enum and DT constants having the same name). Although this brings up one point: let's say we support ACPI/.. bindings in the future. The enum possibly can't match the binding values from every different kind of binding definition (DT, ACPI, ...) so perhaps rather than changing the enum definition in <linux/pwm.h>, what we should be doing is mapping between the different name-spaces in whatever of_xlate function exists for the PWM flags cell. That would be more flexible.
On 07/12/2013 05:01 AM, Laurent Pinchart wrote: > Hi, > > On Thursday 11 July 2013 14:06:44 Stephen Warren wrote: >> On 07/11/2013 01:32 PM, Thierry Reding wrote: >>> On Thu, Jul 11, 2013 at 11:50:48AM -0600, Stephen Warren wrote: >>>> On 07/11/2013 09:36 AM, 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. >>>> >>>> I disagree here. The whole point of creating header files for the >>>> constants in binding definitions was so that you wouldn't have to >>>> duplicate all the values into the binding definitions. Rather, you'd >>>> simply say "see <dt-bindings/xxx.h>". >>> >>> But that's not something that this patch solves. >> >> Well, if the comments I made on the patch re: that <linux/pwm.h> should >> simply #include <dt-bindings/pwm/pwm.h> instead of duplicating the >> constants, then yet this patch will solve that. There will be a single place >> where the constants are defined. > > As explained in another reply, this would require replacing the enum with an > unsigned int. I can write a patch if we agree on this. > >>> And it could be solved even in the absence of the header file defining the >>> symbolic constants. If all the standard flags that dt-bindings/pwm/pwm.txt >>> now specifies were to be listed in pwm.txt (they actually are) then >>> referring to that document as the canonical source works equally well. >> >> If that's all the happens, then there will still be duplication >> between pwm.txt and <linux/pwm.h>. > > I've explicitly mentioned the flags in individual DT bindings to ease adding > new flags in the future. At the moment the defined flags are either all > supported or not used at all by drivers. If we later add a new flag supported > by a subset of drivers only the driver bindings should list supported flags > for each driver. > > I'm fine with removing the explicit mentions of individual flags right now and > adding it back when needed if you think that's better. I think the values for any common system-wide flags should be defined once in some system-wide place, and the values for any HW-specific values should be defined only in the documentation for that specific HW. You could try and avoid conflicts by either: a) Allocating system-wide flags from bit 0 up, and HW-specific flags from bit 31 down. or: b) Using 1 cell for standard flags, and a separate cell for any HW-specific flags. Drivers can quite easily adapt to adding extra cells to #pwm-cells, thus making adding a HW-specific cell later backwards-compatible.
On Fri, Jul 12, 2013 at 08:40:07AM -0600, Stephen Warren wrote: > On 07/12/2013 04:41 AM, Laurent Pinchart wrote: > > Hi Stephen, [...] > > What about splitting it in three patches that > > > > - add the include/dt-bindings/pwm/pwm.h header, and update include/linux/pwm.h > > and Documentation/devicetree/bindings/pwm/pwm.txt > > > > - update the rest of the documentation > > > > - update the .dts files > > I think that sounds reasonable. Shouldn't the addition of include/dt-bindings/pwm/pwm.h be separate from its inclusion in include/linux/pwm.h so that it can be moved more easily (cherry-picked) to a separate repository? > >>> 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, > >>> > >>> }; > >> > >> Rather than manually editing that to ensure the enum matches the DT bindings > >> header, the whole point of making a separate <dt-bindings/...> directory was > >> that drivers could include the binding header files directly to avoid having > >> to duplicate the constant definitions. Can't <linux/pwm.h> include <dt- > >> bindings/pwm.h> and remove that enum? > > > > We could do that, but we would then need to modify all drivers to replace > > enum_pwm_polarity with unsigned int. Thierry, what's your opinion on this ? > > Or perhaps we could keep the enums around, but force the values to match > the DT constants: > > enum pwm_polarity { > PWM_POLARITY_NORMAL = PWM_POLARITY_NORMAL, > PWM_POLARITY_INVERTED = PWM_POLARITY_INVERTED, > }; > > (although obviously you'd need to avoid the enum and DT constants having > the same name). I think I've seen stuff like the following done in a few header files to keep compatibility between enums and defines. enum foo { BAR, #define BAR BAR BAZ, #define BAZ BAZ }; Which, as I understand it, won't work in this case because DTC can only cope with plain cpp files? > Although this brings up one point: let's say we support ACPI/.. bindings > in the future. The enum possibly can't match the binding values from > every different kind of binding definition (DT, ACPI, ...) so perhaps > rather than changing the enum definition in <linux/pwm.h>, what we > should be doing is mapping between the different name-spaces in whatever > of_xlate function exists for the PWM flags cell. That would be more > flexible. I'm not quite sure what exactly you are suggesting here. Can you elaborate? Thierry
On 07/12/2013 11:24 AM, Thierry Reding wrote: > On Fri, Jul 12, 2013 at 08:40:07AM -0600, Stephen Warren wrote: >> On 07/12/2013 04:41 AM, Laurent Pinchart wrote: >>> Hi Stephen, > [...] >>> What about splitting it in three patches that >>> >>> - add the include/dt-bindings/pwm/pwm.h header, and update >>> include/linux/pwm.h and >>> Documentation/devicetree/bindings/pwm/pwm.txt >>> >>> - update the rest of the documentation >>> >>> - update the .dts files >> >> I think that sounds reasonable. > > Shouldn't the addition of include/dt-bindings/pwm/pwm.h be separate > from its inclusion in include/linux/pwm.h so that it can be moved > more easily (cherry-picked) to a separate repository? I'm fine with that being another separate patch. However, I doubt cherry-picking is an issue here; when the separate DT repo is created, it seems likely that someone will simply copy the latest files from the latest Linux kernel in order to populate the tree. cherry-picking probably won't work because: a) I doubt that the DT binding/header additions have always been kept separate from kernel code changes in all of Linux's history. b) I wouldn't be remotely surprised if the layout of the new repo was entirely different to the kernel source tree layout, so direct cherry-pick wouldn't work. c) Not having a common git history would make adding a kernel remote into the DT repo rather odd. (b) and (c) would at leat require some kind of git filter operation rather than cherry-pick, and this issue could be solve in that filter definition. >>>>> 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, >>>>> >>>>> }; >>>> >>>> Rather than manually editing that to ensure the enum matches >>>> the DT bindings header, the whole point of making a separate >>>> <dt-bindings/...> directory was that drivers could include >>>> the binding header files directly to avoid having to >>>> duplicate the constant definitions. Can't <linux/pwm.h> >>>> include <dt- bindings/pwm.h> and remove that enum? >>> >>> We could do that, but we would then need to modify all drivers >>> to replace enum_pwm_polarity with unsigned int. Thierry, what's >>> your opinion on this ? >> >> Or perhaps we could keep the enums around, but force the values >> to match the DT constants: >> >> enum pwm_polarity { PWM_POLARITY_NORMAL = PWM_POLARITY_NORMAL, >> PWM_POLARITY_INVERTED = PWM_POLARITY_INVERTED, }; >> >> (although obviously you'd need to avoid the enum and DT constants >> having the same name). > > I think I've seen stuff like the following done in a few header > files to keep compatibility between enums and defines. > > enum foo { BAR, #define BAR BAR BAZ, #define BAZ BAZ }; > > Which, as I understand it, won't work in this case because DTC can > only cope with plain cpp files? Yeah, dtc doesn't understand "enum", so you can't include an enum definition in a DT file. You have to share simple #define headers between DT and kernel code. >> Although this brings up one point: let's say we support ACPI/.. >> bindings in the future. The enum possibly can't match the binding >> values from every different kind of binding definition (DT, ACPI, >> ...) so perhaps rather than changing the enum definition in >> <linux/pwm.h>, what we should be doing is mapping between the >> different name-spaces in whatever of_xlate function exists for >> the PWM flags cell. That would be more flexible. > > I'm not quite sure what exactly you are suggesting here. Can you > elaborate? Suppose ACPI (or whatever else) starts representing PWM devices. Suppose the author isn't aware that DT exists, represents PWM devices and/or has already defined some PWM-related flags. So, ACPI picks bit 5 in some data value to represent inverted, rather than bit 0. Then, there is no way that all of [ (a) DT binding PWM flags (b) ACPI PWM flags (c) Linux's enum foo ] can use the same values. Hence, some mapping is required between them. The typical way to do this is to define an "of_xlate" function which converts from DT cell values to Linux-internal representation. Presumably if we added an ACPI parser, there'd be some equivalent for that. So, what I'm arguing for is that of_pwm_simple_xlate() (or any other custom xlate function) should include both <dt-bindings/pwm/pwm.h> and <linux/pwm.h>, and contain explicit code to convert between the two name-spaces of flags definitions. Since those two name-spaces currently are 100% identical, presumably if the code is written in the right way, the compiler will actually just optimize it all away...
Hi Stephen, On Friday 12 July 2013 08:42:41 Stephen Warren wrote: > On 07/12/2013 05:01 AM, Laurent Pinchart wrote: > > On Thursday 11 July 2013 14:06:44 Stephen Warren wrote: > >> On 07/11/2013 01:32 PM, Thierry Reding wrote: > >>> On Thu, Jul 11, 2013 at 11:50:48AM -0600, Stephen Warren wrote: > >>>> On 07/11/2013 09:36 AM, 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. > >>>> > >>>> I disagree here. The whole point of creating header files for the > >>>> constants in binding definitions was so that you wouldn't have to > >>>> duplicate all the values into the binding definitions. Rather, you'd > >>>> simply say "see <dt-bindings/xxx.h>". > >>> > >>> But that's not something that this patch solves. > >> > >> Well, if the comments I made on the patch re: that <linux/pwm.h> should > >> simply #include <dt-bindings/pwm/pwm.h> instead of duplicating the > >> constants, then yet this patch will solve that. There will be a single > >> place where the constants are defined. > > > > As explained in another reply, this would require replacing the enum with > > an unsigned int. I can write a patch if we agree on this. > > > >>> And it could be solved even in the absence of the header file defining > >>> the symbolic constants. If all the standard flags that > >>> dt-bindings/pwm/pwm.txt now specifies were to be listed in pwm.txt (they > >>> actually are) then referring to that document as the canonical source > >>> works equally well. > >> > >> If that's all the happens, then there will still be duplication > >> between pwm.txt and <linux/pwm.h>. > > > > I've explicitly mentioned the flags in individual DT bindings to ease > > adding new flags in the future. At the moment the defined flags are > > either all supported or not used at all by drivers. If we later add a new > > flag supported by a subset of drivers only the driver bindings should > > list supported flags for each driver. > > > > I'm fine with removing the explicit mentions of individual flags right now > > and adding it back when needed if you think that's better. > > I think the values for any common system-wide flags should be defined > once in some system-wide place, and the values for any HW-specific > values should be defined only in the documentation for that specific HW. > You could try and avoid conflicts by either: > > a) Allocating system-wide flags from bit 0 up, and HW-specific flags > from bit 31 down. > > or: > > b) Using 1 cell for standard flags, and a separate cell for any > HW-specific flags. Drivers can quite easily adapt to adding extra cells > to #pwm-cells, thus making adding a HW-specific cell later > backwards-compatible. I wasn't referring to HW-specific flags, but rather to system-wide flags that might not be supported by all drivers. If we later add new system-wide flags I think the individual DT bindings should explicitly document which flags they support.
Hello, On Friday 12 July 2013 11:40:44 Stephen Warren wrote: > On 07/12/2013 11:24 AM, Thierry Reding wrote: > > On Fri, Jul 12, 2013 at 08:40:07AM -0600, Stephen Warren wrote: > >> On 07/12/2013 04:41 AM, Laurent Pinchart wrote: > >>> Hi Stephen, > > > > [...] > > > >>> What about splitting it in three patches that > >>> > >>> - add the include/dt-bindings/pwm/pwm.h header, and update > >>> include/linux/pwm.h and > >>> Documentation/devicetree/bindings/pwm/pwm.txt > >>> > >>> - update the rest of the documentation > >>> > >>> - update the .dts files > >> > >> I think that sounds reasonable. > > > > Shouldn't the addition of include/dt-bindings/pwm/pwm.h be separate from > > its inclusion in include/linux/pwm.h so that it can be moved more easily > > (cherry-picked) to a separate repository? > > I'm fine with that being another separate patch. However, I doubt cherry- > picking is an issue here; when the separate DT repo is created, it seems > likely that someone will simply copy the latest files from the latest Linux > kernel in order to populate the tree. cherry-picking probably won't work > because: > > a) I doubt that the DT binding/header additions have always been kept > separate from kernel code changes in all of Linux's history. > > b) I wouldn't be remotely surprised if the layout of the new repo was > entirely different to the kernel source tree layout, so direct cherry-pick > wouldn't work. > > c) Not having a common git history would make adding a kernel remote into > the DT repo rather odd. > > (b) and (c) would at leat require some kind of git filter operation rather > than cherry-pick, and this issue could be solve in that filter definition. I agree with the reasoning, so I'll implement the patch split scheme described above unless someone objects. > >>>>> 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, > >>>>> > >>>>> }; > >>>> > >>>> Rather than manually editing that to ensure the enum matches the DT > >>>> bindings header, the whole point of making a separate <dt-bindings/...> > >>>> directory was that drivers could include the binding header files > >>>> directly to avoid having to duplicate the constant definitions. Can't > >>>> <linux/pwm.h> include <dt- bindings/pwm.h> and remove that enum? > >>> > >>> We could do that, but we would then need to modify all drivers to > >>> replace enum_pwm_polarity with unsigned int. Thierry, what's your > >>> opinion on this ? > >> > >> Or perhaps we could keep the enums around, but force the values to match > >> the DT constants: > >> > >> enum pwm_polarity { PWM_POLARITY_NORMAL = PWM_POLARITY_NORMAL, > >> PWM_POLARITY_INVERTED = PWM_POLARITY_INVERTED, }; > >> > >> (although obviously you'd need to avoid the enum and DT constants having > >> the same name). > > > > I think I've seen stuff like the following done in a few header files to > > keep compatibility between enums and defines. > > > > enum foo { BAR, #define BAR BAR BAZ, #define BAZ BAZ }; > > > > Which, as I understand it, won't work in this case because DTC can only > > cope with plain cpp files? > > Yeah, dtc doesn't understand "enum", so you can't include an enum definition > in a DT file. You have to share simple #define headers between DT and kernel > code. > > >> Although this brings up one point: let's say we support ACPI/.. > >> bindings in the future. The enum possibly can't match the binding > >> values from every different kind of binding definition (DT, ACPI, > >> ...) so perhaps rather than changing the enum definition in > >> <linux/pwm.h>, what we should be doing is mapping between the > >> different name-spaces in whatever of_xlate function exists for > >> the PWM flags cell. That would be more flexible. > > > > I'm not quite sure what exactly you are suggesting here. Can you > > elaborate? > > Suppose ACPI (or whatever else) starts representing PWM devices. Suppose the > author isn't aware that DT exists, represents PWM devices and/or has already > defined some PWM-related flags. So, ACPI picks bit 5 in some data value to > represent inverted, rather than bit 0. Then, there is no way that all of [ > (a) DT binding PWM flags (b) ACPI PWM flags (c) Linux's enum foo ] can use > the same values. Hence, some mapping is required between them. > > The typical way to do this is to define an "of_xlate" function which > converts from DT cell values to Linux-internal representation. Presumably if > we added an ACPI parser, there'd be some equivalent for that. > > So, what I'm arguing for is that of_pwm_simple_xlate() (or any other custom > xlate function) should include both <dt-bindings/pwm/pwm.h> and > <linux/pwm.h>, and contain explicit code to convert between the two name- > spaces of flags definitions. Since those two name-spaces currently are 100% > identical, presumably if the code is written in the right way, the compiler > will actually just optimize it all away... If ACPI (or any other source of device information) uses different numerical values for the flags we will need an xlate function. In the DT case we're designing bindings so we're free to pick the numerical values currently used by the kernel, and even use the same symbolic names. I'm however fine with renamed the #define's in dt-bindings/pwm/pwm.h. Does anyone have a preference regarding the names I should use ?
On 07/15/2013 07:10 PM, Laurent Pinchart wrote: > On Friday 12 July 2013 08:42:41 Stephen Warren wrote: ... >> I think the values for any common system-wide flags should be defined >> once in some system-wide place, and the values for any HW-specific >> values should be defined only in the documentation for that specific HW. >> You could try and avoid conflicts by either: >> >> a) Allocating system-wide flags from bit 0 up, and HW-specific flags >> from bit 31 down. >> >> or: >> >> b) Using 1 cell for standard flags, and a separate cell for any >> HW-specific flags. Drivers can quite easily adapt to adding extra cells >> to #pwm-cells, thus making adding a HW-specific cell later >> backwards-compatible. > > I wasn't referring to HW-specific flags, but rather to system-wide flags that > might not be supported by all drivers. If we later add new system-wide flags I > think the individual DT bindings should explicitly document which flags they > support. Shouldn't all system-wide flags be supported by all HW, perhaps being implemented by the core subsystem rather than individual drivers to ensure that? Consider the case of the GPIO active-low flag which is actually implemented in SW, hence can work with any GPIO controller. Perhaps that's not possible in all cases, in which case, yes, it does make sense to define which of the common flags a particular HW module supports. But then there's a problem where people assume that the common flags are always available, and somewhere they aren't... Care is needed in the choice of which common flags to define and/or how they're used.
Hi Stephen, On Monday 15 July 2013 21:39:31 Stephen Warren wrote: > On 07/15/2013 07:10 PM, Laurent Pinchart wrote: > > On Friday 12 July 2013 08:42:41 Stephen Warren wrote: > ... > > >> I think the values for any common system-wide flags should be defined > >> once in some system-wide place, and the values for any HW-specific > >> values should be defined only in the documentation for that specific HW. > >> You could try and avoid conflicts by either: > >> > >> a) Allocating system-wide flags from bit 0 up, and HW-specific flags > >> from bit 31 down. > >> > >> or: > >> > >> b) Using 1 cell for standard flags, and a separate cell for any > >> HW-specific flags. Drivers can quite easily adapt to adding extra cells > >> to #pwm-cells, thus making adding a HW-specific cell later > >> backwards-compatible. > > > > I wasn't referring to HW-specific flags, but rather to system-wide flags > > that might not be supported by all drivers. If we later add new > > system-wide flags I think the individual DT bindings should explicitly > > document which flags they support. > > Shouldn't all system-wide flags be supported by all HW, perhaps being > implemented by the core subsystem rather than individual drivers to > ensure that? Consider the case of the GPIO active-low flag which is > actually implemented in SW, hence can work with any GPIO controller. > > Perhaps that's not possible in all cases, in which case, yes, it does > make sense to define which of the common flags a particular HW module > supports. It largely depends on what we consider as system flags. We should probably be talking about common flags instead of system flags. I usually try to standardize DT properties that are common to multiple devices. Some of those properties can apply to all devices of the same class (possibly implemented/emulated in software when the hardware doesn't support them), but others are just commonly seen properties that don't make sense for all the devices. One example I'm familiar with can be found in V4L2 DT bindings. We've standardized properties to specify what kind of video synchronization signals devices support/use. Not all synchronization signals are supported by a given video transmitter, so DT bindings for such chips list (or at least should list) the common properties supported by a given device. The definitions of the properties should of course not be duplicated to all individual DT bindings. > But then there's a problem where people assume that the common flags are > always available, and somewhere they aren't... Care is needed in the > choice of which common flags to define and/or how they're used. Exactly. That's why I think listing the supported common flags in individual bindings makes sense when some of the flags are not supported by all devices. As the only PWM flags currently used are common to all PWM devices I can leave this out now. I have no strong preference, I'll follow your opinion on this.
On 07/17/2013 05:00 AM, Laurent Pinchart wrote: > On Monday 15 July 2013 21:39:31 Stephen Warren wrote: ... >> But then there's a problem where people assume that the common flags are >> always available, and somewhere they aren't... Care is needed in the >> choice of which common flags to define and/or how they're used. > > Exactly. That's why I think listing the supported common flags in individual > bindings makes sense when some of the flags are not supported by all devices. > As the only PWM flags currently used are common to all PWM devices I can leave > this out now. I have no strong preference, I'll follow your opinion on this. Yes, I guess separating the concept of defining common flags and which devices use them is good. And then indeed individual devices need to define which of the common flags they support. I'd still like to see the *definition* of those common flags in some central place (i.e. pwm.txt or a header that defines constants for it), and the other device bindings simply reference that for the actual definitions.
On Wed, Jul 17, 2013 at 11:11:19AM -0600, Stephen Warren wrote: > On 07/17/2013 05:00 AM, Laurent Pinchart wrote: > > On Monday 15 July 2013 21:39:31 Stephen Warren wrote: > ... > >> But then there's a problem where people assume that the common flags are > >> always available, and somewhere they aren't... Care is needed in the > >> choice of which common flags to define and/or how they're used. > > > > Exactly. That's why I think listing the supported common flags in individual > > bindings makes sense when some of the flags are not supported by all devices. > > As the only PWM flags currently used are common to all PWM devices I can leave > > this out now. I have no strong preference, I'll follow your opinion on this. > > Yes, I guess separating the concept of defining common flags and which > devices use them is good. And then indeed individual devices need to > define which of the common flags they support. I'd still like to see the > *definition* of those common flags in some central place (i.e. pwm.txt > or a header that defines constants for it), and the other device > bindings simply reference that for the actual definitions. That sounds reasonable to me. Thierry
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: 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 Optional properties: - samsung,pwm-outputs: list of PWM channels used as PWM outputs on particular diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt index 337c6fc..9007d92 100644 --- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt @@ -7,8 +7,9 @@ Required properties: - #pwm-cells: Should be 3. Number of cells being used to specify PWM property. 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 in PWM specifier - to 1 for inverse polarity & set to 0 for normal polarity. + 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>. - reg: physical base address and size of the registers map. Optional properties: 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 Example with optional PWM specifier for inverse polarity bl: backlight { - pwms = <&pwm 0 5000000 1>; + pwms = <&pwm 0 5000000 PWM_POLARITY_INVERSED>; pwm-names = "backlight"; }; diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt index d21d82d..5b1b21f 100644 --- a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt +++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt @@ -6,8 +6,9 @@ Required properties: - #pwm-cells: Should be 3. Number of cells being used to specify PWM property. 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 in PWM specifier - to 1 for inverse polarity & set to 0 for normal polarity. + 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>. - clocks: phandle to the PWM source clock Example: diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts index 3aee1a4..c9da673 100644 --- a/arch/arm/boot/dts/am335x-evm.dts +++ b/arch/arm/boot/dts/am335x-evm.dts @@ -8,6 +8,7 @@ /dts-v1/; #include "am33xx.dtsi" +#include <dt-bindings/pwm/pwm.h> / { model = "TI AM335x EVM"; @@ -370,7 +371,7 @@ backlight { compatible = "pwm-backlight"; - pwms = <&ecap0 0 50000 0>; + pwms = <&ecap0 0 50000 PWM_POLARITY_NORMAL>; brightness-levels = <0 51 53 56 62 75 101 152 255>; default-brightness-level = <8>; }; diff --git a/arch/arm/boot/dts/am335x-evmsk.dts b/arch/arm/boot/dts/am335x-evmsk.dts index 0c8ad17..5d4ec91 100644 --- a/arch/arm/boot/dts/am335x-evmsk.dts +++ b/arch/arm/boot/dts/am335x-evmsk.dts @@ -14,6 +14,7 @@ /dts-v1/; #include "am33xx.dtsi" +#include <dt-bindings/pwm/pwm.h> / { model = "TI AM335x EVM-SK"; @@ -298,7 +299,7 @@ backlight { compatible = "pwm-backlight"; - pwms = <&ecap2 0 50000 1>; + pwms = <&ecap2 0 50000 PWM_POLARITY_INVERSED>; brightness-levels = <0 58 61 66 75 90 125 170 255>; default-brightness-level = <8>; }; diff --git a/arch/arm/boot/dts/wm8850-w70v2.dts b/arch/arm/boot/dts/wm8850-w70v2.dts index 90e913f..171930a 100644 --- a/arch/arm/boot/dts/wm8850-w70v2.dts +++ b/arch/arm/boot/dts/wm8850-w70v2.dts @@ -11,13 +11,14 @@ /dts-v1/; /include/ "wm8850.dtsi" +#include <dt-bindings/pwm/pwm.h> / { model = "Wondermedia WM8850-W70v2 Tablet"; backlight { compatible = "pwm-backlight"; - pwms = <&pwm 0 50000 1>; /* duty inverted */ + pwms = <&pwm 0 50000 PWM_POLARITY_INVERSED>; brightness-levels = <0 40 60 80 100 130 190 255>; default-brightness-level = <5>; 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 diff --git a/include/linux/pwm.h b/include/linux/pwm.h index f0feafd..ffde99b 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -69,8 +69,8 @@ struct pwm_chip; * period */ enum pwm_polarity { - PWM_POLARITY_NORMAL, - PWM_POLARITY_INVERSED, + PWM_POLARITY_NORMAL = 0, + PWM_POLARITY_INVERSED = 1, }; enum {
Define PWM_POLARITY_NORMAL and PWM_POLARITY_INVERTED macros in include/dt-bindings/pwm/pwm.h to be used by device tree sources. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt | 6 +++--- Documentation/devicetree/bindings/pwm/pwm-samsung.txt | 5 +++-- Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 5 +++-- Documentation/devicetree/bindings/pwm/pwm.txt | 8 +++++--- Documentation/devicetree/bindings/pwm/vt8500-pwm.txt | 5 +++-- arch/arm/boot/dts/am335x-evm.dts | 3 ++- arch/arm/boot/dts/am335x-evmsk.dts | 3 ++- arch/arm/boot/dts/wm8850-w70v2.dts | 3 ++- include/dt-bindings/pwm/pwm.h | 15 +++++++++++++++ include/linux/pwm.h | 4 ++-- 10 files changed, 40 insertions(+), 17 deletions(-) create mode 100644 include/dt-bindings/pwm/pwm.h