Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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 signature.asc Description: Digital signature
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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 000..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,
+
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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
signature.asc
Description: Digital signature
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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...
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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+rene...@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
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 500 1;
+ pwms = pwm 0 500 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
+++
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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 000..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
signature.asc
Description: Digital signature
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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 signature.asc Description: Digital signature
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
