[PATCH 0/2] Add PWM polarity flag macros for DT

[PATCH 0/2] Add PWM polarity flag macros for DT

[Laurent Pinchart]

Hello,

Here's a small patch set that replaces PWM polarity numerical constants with
macros in DT. The series starts with a fix for an error in the i.MX53 mba53 DT,
and the second patch then introduces the macros and update both bindings
documentation and existing DT.

Laurent Pinchart (2):
  ARM i.MX53: mba53: Fix PWM backlight DT node
  pwm: Add PWM polarity flag macros for DT

 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/imx53-mba53.dts                       |  2 +-
 arch/arm/boot/dts/wm8850-w70v2.dts                      |  3 ++-
 include/dt-bindings/pwm/pwm.h                           | 15 +++++++++++++++
 include/linux/pwm.h                                     |  4 ++--
 11 files changed, 41 insertions(+), 18 deletions(-)
 create mode 100644 include/dt-bindings/pwm/pwm.h

-- 
Regards,

Laurent Pinchart

[PATCH 1/2] ARM i.MX53: mba53: Fix PWM backlight DT node

[Laurent Pinchart]

The i.MX53 PWM controller uses two cells to describe the PWM specifier.
Remove the extra unused values from the backlight DT node pwms property.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm/boot/dts/imx53-mba53.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx53-mba53.dts b/arch/arm/boot/dts/imx53-mba53.dts
index aaa33bc..a630902 100644
--- a/arch/arm/boot/dts/imx53-mba53.dts
+++ b/arch/arm/boot/dts/imx53-mba53.dts
@@ -27,7 +27,7 @@
 
 	backlight {
 		compatible = "pwm-backlight";
-		pwms = <&pwm2 0 50000 0 0>;
+		pwms = <&pwm2 0 50000>;
 		brightness-levels = <0 24 28 32 36 40 44 48 52 56 60 64 68 72 76 80 84 88 92 96 100>;
 		default-brightness-level = <10>;
 		enable-gpios = <&gpio7 7 0>;
-- 
1.8.1.5

Re: [PATCH 1/2] ARM i.MX53: mba53: Fix PWM backlight DT node

[Shawn Guo]

On Thu, Jul 11, 2013 at 04:37:47PM +0200, Laurent Pinchart wrote:
> The i.MX53 PWM controller uses two cells to describe the PWM specifier.
> Remove the extra unused values from the backlight DT node pwms property.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Applied, thanks.

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

[Laurent Pinchart]

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

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 {
-- 
1.8.1.5

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

[Thierry Reding]

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Stephen Warren]

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.

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

[Thierry Reding]

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Stephen Warren]

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.

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

[Laurent Pinchart]

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

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

[Stephen Warren]

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.

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

[Laurent Pinchart]

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

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

[Stephen Warren]

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.

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

[Laurent Pinchart]

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

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

[Stephen Warren]

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.

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

[Thierry Reding]

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Laurent Pinchart]

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

Hi Thierry,

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

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

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

OK.

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

Oops :-)

Will fix.

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

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

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

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

-- 
Regards,

Laurent Pinchart

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

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

[Stephen Warren]

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?

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

[Laurent Pinchart]

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

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

[Stephen Warren]

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.

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

[Thierry Reding]

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Stephen Warren]

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...

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

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart