Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Thierry Reding]

On Mon, Dec 05, 2016 at 01:12:25PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 12:23 GMT+01:00 Thierry Reding :
> > On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
> >> 2016-12-05 7:53 GMT+01:00 Thierry Reding :
> >> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> >> >> Define bindings for pwm-stm32
> >> >>
> >> >> version 2:
> >> >> - use parameters instead of compatible of handle the hardware 
> >> >> configuration
> >> >>
> >> >> Signed-off-by: Benjamin Gaignard 
> >> >> ---
> >> >>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> >> >> ++
> >> >>  1 file changed, 38 insertions(+)
> >> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> >> >> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >> new file mode 100644
> >> >> index 000..575b9fb
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >> @@ -0,0 +1,38 @@
> >> >> +STMicroelectronics PWM driver bindings for STM32
> >> >
> >> > Technically this bindings describe devices, so "driver binding" is a
> >> > somewhat odd wording. Perhaps:
> >> >
> >> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >> >
> >> > ?
> >>  done
> >>
> >> >
> >> >> +
> >> >> +Must be a sub-node of STM32 general purpose timer driver
> >> >> +Parent node properties are describe in 
> >> >> ../mfd/stm32-general-purpose-timer.txt
> >> >
> >> > Again, "driver parent node" is odd. Perhaps:
> >> >
> >> > Must be a sub-node of an STM32 General Purpose Timer device tree
> >> > node. See ../mfd/stm32-general-purpose-timer.txt for details 
> >> > about
> >> > the parent node.
> >> >
> >> > ?
> >>
> >> done
> >>
> >> >
> >> >> +Required parameters:
> >> >> +- compatible:Must be "st,stm32-pwm"
> >> >> +- pinctrl-names: Set to "default".
> >> >> +- pinctrl-0: List of phandles pointing to pin 
> >> >> configuration nodes
> >> >> + for PWM module.
> >> >> + For Pinctrl properties, please refer to [1].
> >> >
> >> > Your indentation and capitalization are inconsistent. Also, please refer
> >> > to the pinctrl bindings by relative path and inline, rather than as a
> >> > footnote reference.
> >>
> >> OK
> >>
> >> >
> >> >> +
> >> >> +Optional parameters:
> >> >> +- st,breakinput: Set if the hardware have break input capabilities
> >> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
> >> >> +  The value define the active polarity:
> >> >> +   - 0 (active LOW)
> >> >> +   - 1 (active HIGH)
> >> >
> >> > Could we fold these into a single property? If st,breakinput-polarity is
> >> > not present it could simply mean that there is no break input, and if it
> >> > is present you don't have to rely on a default.
> >>
> >> I need to know if I have to activate breakinput feature and on which level
> >> I will rewrite it like that:
> >> Optional parameters:
> >> - st,breakinput-polarity-high: Set if break input polarity is active
> >> on high level.
> >> - st,breakinput-polarity-high: Set if break input polarity is active
> >> on low level.
> >
> > How is that different from a single property:
> >
> > Optional properties:
> > - st,breakinput-polarity: If present, a break input is available
> > for the channel. In that case the property value denotes the
> > polarity of the break input:
> > - 0: active low
> > - 1: active high
> >
> > ?
> 
> For break input feature I need two information: do I have to active it
> and if activated
> on which level.
> I have two solutions:
> - one parameter with a value (0 or 1) -> st,breakinput-polarity
> - two parameters without value -> st,breakinput-active-high and
> st,breakinput-active-low
> 
> By default break input feature is disabled

Right, what I was suggesting is that you use the first solution because
it's the typical way to use for tristate options. It's also much easier
to test for in practice because for the second solution you have to
parse two properties before you know whether it is active or not.

The second is typically the solution for required properties where only
one of the properties is used to override the default.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Thierry Reding]

On Mon, Dec 05, 2016 at 01:12:25PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 12:23 GMT+01:00 Thierry Reding :
> > On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
> >> 2016-12-05 7:53 GMT+01:00 Thierry Reding :
> >> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> >> >> Define bindings for pwm-stm32
> >> >>
> >> >> version 2:
> >> >> - use parameters instead of compatible of handle the hardware 
> >> >> configuration
> >> >>
> >> >> Signed-off-by: Benjamin Gaignard 
> >> >> ---
> >> >>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> >> >> ++
> >> >>  1 file changed, 38 insertions(+)
> >> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> >> >> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >> new file mode 100644
> >> >> index 000..575b9fb
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >> @@ -0,0 +1,38 @@
> >> >> +STMicroelectronics PWM driver bindings for STM32
> >> >
> >> > Technically this bindings describe devices, so "driver binding" is a
> >> > somewhat odd wording. Perhaps:
> >> >
> >> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >> >
> >> > ?
> >>  done
> >>
> >> >
> >> >> +
> >> >> +Must be a sub-node of STM32 general purpose timer driver
> >> >> +Parent node properties are describe in 
> >> >> ../mfd/stm32-general-purpose-timer.txt
> >> >
> >> > Again, "driver parent node" is odd. Perhaps:
> >> >
> >> > Must be a sub-node of an STM32 General Purpose Timer device tree
> >> > node. See ../mfd/stm32-general-purpose-timer.txt for details 
> >> > about
> >> > the parent node.
> >> >
> >> > ?
> >>
> >> done
> >>
> >> >
> >> >> +Required parameters:
> >> >> +- compatible:Must be "st,stm32-pwm"
> >> >> +- pinctrl-names: Set to "default".
> >> >> +- pinctrl-0: List of phandles pointing to pin 
> >> >> configuration nodes
> >> >> + for PWM module.
> >> >> + For Pinctrl properties, please refer to [1].
> >> >
> >> > Your indentation and capitalization are inconsistent. Also, please refer
> >> > to the pinctrl bindings by relative path and inline, rather than as a
> >> > footnote reference.
> >>
> >> OK
> >>
> >> >
> >> >> +
> >> >> +Optional parameters:
> >> >> +- st,breakinput: Set if the hardware have break input capabilities
> >> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
> >> >> +  The value define the active polarity:
> >> >> +   - 0 (active LOW)
> >> >> +   - 1 (active HIGH)
> >> >
> >> > Could we fold these into a single property? If st,breakinput-polarity is
> >> > not present it could simply mean that there is no break input, and if it
> >> > is present you don't have to rely on a default.
> >>
> >> I need to know if I have to activate breakinput feature and on which level
> >> I will rewrite it like that:
> >> Optional parameters:
> >> - st,breakinput-polarity-high: Set if break input polarity is active
> >> on high level.
> >> - st,breakinput-polarity-high: Set if break input polarity is active
> >> on low level.
> >
> > How is that different from a single property:
> >
> > Optional properties:
> > - st,breakinput-polarity: If present, a break input is available
> > for the channel. In that case the property value denotes the
> > polarity of the break input:
> > - 0: active low
> > - 1: active high
> >
> > ?
> 
> For break input feature I need two information: do I have to active it
> and if activated
> on which level.
> I have two solutions:
> - one parameter with a value (0 or 1) -> st,breakinput-polarity
> - two parameters without value -> st,breakinput-active-high and
> st,breakinput-active-low
> 
> By default break input feature is disabled

Right, what I was suggesting is that you use the first solution because
it's the typical way to use for tristate options. It's also much easier
to test for in practice because for the second solution you have to
parse two properties before you know whether it is active or not.

The second is typically the solution for required properties where only
one of the properties is used to override the default.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Benjamin Gaignard]

2016-12-05 12:23 GMT+01:00 Thierry Reding :
> On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
>> 2016-12-05 7:53 GMT+01:00 Thierry Reding :
>> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
>> >> Define bindings for pwm-stm32
>> >>
>> >> version 2:
>> >> - use parameters instead of compatible of handle the hardware 
>> >> configuration
>> >>
>> >> Signed-off-by: Benjamin Gaignard 
>> >> ---
>> >>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
>> >> ++
>> >>  1 file changed, 38 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
>> >> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >> new file mode 100644
>> >> index 000..575b9fb
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >> @@ -0,0 +1,38 @@
>> >> +STMicroelectronics PWM driver bindings for STM32
>> >
>> > Technically this bindings describe devices, so "driver binding" is a
>> > somewhat odd wording. Perhaps:
>> >
>> > STMicroelectronics STM32 General Purpose Timer PWM bindings
>> >
>> > ?
>>  done
>>
>> >
>> >> +
>> >> +Must be a sub-node of STM32 general purpose timer driver
>> >> +Parent node properties are describe in 
>> >> ../mfd/stm32-general-purpose-timer.txt
>> >
>> > Again, "driver parent node" is odd. Perhaps:
>> >
>> > Must be a sub-node of an STM32 General Purpose Timer device tree
>> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
>> > the parent node.
>> >
>> > ?
>>
>> done
>>
>> >
>> >> +Required parameters:
>> >> +- compatible:Must be "st,stm32-pwm"
>> >> +- pinctrl-names: Set to "default".
>> >> +- pinctrl-0: List of phandles pointing to pin 
>> >> configuration nodes
>> >> + for PWM module.
>> >> + For Pinctrl properties, please refer to [1].
>> >
>> > Your indentation and capitalization are inconsistent. Also, please refer
>> > to the pinctrl bindings by relative path and inline, rather than as a
>> > footnote reference.
>>
>> OK
>>
>> >
>> >> +
>> >> +Optional parameters:
>> >> +- st,breakinput: Set if the hardware have break input capabilities
>> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
>> >> +  The value define the active polarity:
>> >> +   - 0 (active LOW)
>> >> +   - 1 (active HIGH)
>> >
>> > Could we fold these into a single property? If st,breakinput-polarity is
>> > not present it could simply mean that there is no break input, and if it
>> > is present you don't have to rely on a default.
>>
>> I need to know if I have to activate breakinput feature and on which level
>> I will rewrite it like that:
>> Optional parameters:
>> - st,breakinput-polarity-high: Set if break input polarity is active
>> on high level.
>> - st,breakinput-polarity-high: Set if break input polarity is active
>> on low level.
>
> How is that different from a single property:
>
> Optional properties:
> - st,breakinput-polarity: If present, a break input is available
> for the channel. In that case the property value denotes the
> polarity of the break input:
> - 0: active low
> - 1: active high
>
> ?

For break input feature I need two information: do I have to active it
and if activated
on which level.
I have two solutions:
- one parameter with a value (0 or 1) -> st,breakinput-polarity
- two parameters without value -> st,breakinput-active-high and
st,breakinput-active-low

By default break input feature is disabled

>
>> > The pwm- prefix is rather redundant since the node is already named pwm.
>> > Why not simply st,channels? Or simply channels, since it's not really
>> > anything specific to this hardware.
>> >
>> > Come to think of it, might be worth having a discussion with our DT
>> > gurus about what their stance is on using the # as prefix for numbers
>> > (such as in #address-cells or #size-cells). This could be #channels to
>> > mark it more explicitly as representing a count.
>> >
>> >> +- st,32bits-counter: Set if the hardware have a 32 bits counter
>> >> +- st,complementary:  Set if the hardware have complementary output 
>> >> channels
>> >
>> > "hardware has" and also maybe mention explicitly that this is a boolean
>> > property. Otherwise people might be left wondering what it should be set
>> > to. Or maybe word this differently to imply that it's boolean:
>> >
>> > - st,32bits-counter: if present, the hardware has a 32 bit counter
>> > - st,complementary: if present, the hardware has a complementary
>> > output channel
>>
>> I found a way to detect, at probe time, the number of channels, 

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Benjamin Gaignard]

2016-12-05 12:23 GMT+01:00 Thierry Reding :
> On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
>> 2016-12-05 7:53 GMT+01:00 Thierry Reding :
>> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
>> >> Define bindings for pwm-stm32
>> >>
>> >> version 2:
>> >> - use parameters instead of compatible of handle the hardware 
>> >> configuration
>> >>
>> >> Signed-off-by: Benjamin Gaignard 
>> >> ---
>> >>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
>> >> ++
>> >>  1 file changed, 38 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
>> >> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >> new file mode 100644
>> >> index 000..575b9fb
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >> @@ -0,0 +1,38 @@
>> >> +STMicroelectronics PWM driver bindings for STM32
>> >
>> > Technically this bindings describe devices, so "driver binding" is a
>> > somewhat odd wording. Perhaps:
>> >
>> > STMicroelectronics STM32 General Purpose Timer PWM bindings
>> >
>> > ?
>>  done
>>
>> >
>> >> +
>> >> +Must be a sub-node of STM32 general purpose timer driver
>> >> +Parent node properties are describe in 
>> >> ../mfd/stm32-general-purpose-timer.txt
>> >
>> > Again, "driver parent node" is odd. Perhaps:
>> >
>> > Must be a sub-node of an STM32 General Purpose Timer device tree
>> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
>> > the parent node.
>> >
>> > ?
>>
>> done
>>
>> >
>> >> +Required parameters:
>> >> +- compatible:Must be "st,stm32-pwm"
>> >> +- pinctrl-names: Set to "default".
>> >> +- pinctrl-0: List of phandles pointing to pin 
>> >> configuration nodes
>> >> + for PWM module.
>> >> + For Pinctrl properties, please refer to [1].
>> >
>> > Your indentation and capitalization are inconsistent. Also, please refer
>> > to the pinctrl bindings by relative path and inline, rather than as a
>> > footnote reference.
>>
>> OK
>>
>> >
>> >> +
>> >> +Optional parameters:
>> >> +- st,breakinput: Set if the hardware have break input capabilities
>> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
>> >> +  The value define the active polarity:
>> >> +   - 0 (active LOW)
>> >> +   - 1 (active HIGH)
>> >
>> > Could we fold these into a single property? If st,breakinput-polarity is
>> > not present it could simply mean that there is no break input, and if it
>> > is present you don't have to rely on a default.
>>
>> I need to know if I have to activate breakinput feature and on which level
>> I will rewrite it like that:
>> Optional parameters:
>> - st,breakinput-polarity-high: Set if break input polarity is active
>> on high level.
>> - st,breakinput-polarity-high: Set if break input polarity is active
>> on low level.
>
> How is that different from a single property:
>
> Optional properties:
> - st,breakinput-polarity: If present, a break input is available
> for the channel. In that case the property value denotes the
> polarity of the break input:
> - 0: active low
> - 1: active high
>
> ?

For break input feature I need two information: do I have to active it
and if activated
on which level.
I have two solutions:
- one parameter with a value (0 or 1) -> st,breakinput-polarity
- two parameters without value -> st,breakinput-active-high and
st,breakinput-active-low

By default break input feature is disabled

>
>> > The pwm- prefix is rather redundant since the node is already named pwm.
>> > Why not simply st,channels? Or simply channels, since it's not really
>> > anything specific to this hardware.
>> >
>> > Come to think of it, might be worth having a discussion with our DT
>> > gurus about what their stance is on using the # as prefix for numbers
>> > (such as in #address-cells or #size-cells). This could be #channels to
>> > mark it more explicitly as representing a count.
>> >
>> >> +- st,32bits-counter: Set if the hardware have a 32 bits counter
>> >> +- st,complementary:  Set if the hardware have complementary output 
>> >> channels
>> >
>> > "hardware has" and also maybe mention explicitly that this is a boolean
>> > property. Otherwise people might be left wondering what it should be set
>> > to. Or maybe word this differently to imply that it's boolean:
>> >
>> > - st,32bits-counter: if present, the hardware has a 32 bit counter
>> > - st,complementary: if present, the hardware has a complementary
>> > output channel
>>
>> I found a way to detect, at probe time, the number of channels, counter size,
>> break input capability and complementary channels so I will remove

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Thierry Reding]

On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 7:53 GMT+01:00 Thierry Reding :
> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> >> Define bindings for pwm-stm32
> >>
> >> version 2:
> >> - use parameters instead of compatible of handle the hardware configuration
> >>
> >> Signed-off-by: Benjamin Gaignard 
> >> ---
> >>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> >> ++
> >>  1 file changed, 38 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> >> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> new file mode 100644
> >> index 000..575b9fb
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> @@ -0,0 +1,38 @@
> >> +STMicroelectronics PWM driver bindings for STM32
> >
> > Technically this bindings describe devices, so "driver binding" is a
> > somewhat odd wording. Perhaps:
> >
> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >
> > ?
>  done
> 
> >
> >> +
> >> +Must be a sub-node of STM32 general purpose timer driver
> >> +Parent node properties are describe in 
> >> ../mfd/stm32-general-purpose-timer.txt
> >
> > Again, "driver parent node" is odd. Perhaps:
> >
> > Must be a sub-node of an STM32 General Purpose Timer device tree
> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> > the parent node.
> >
> > ?
> 
> done
> 
> >
> >> +Required parameters:
> >> +- compatible:Must be "st,stm32-pwm"
> >> +- pinctrl-names: Set to "default".
> >> +- pinctrl-0: List of phandles pointing to pin 
> >> configuration nodes
> >> + for PWM module.
> >> + For Pinctrl properties, please refer to [1].
> >
> > Your indentation and capitalization are inconsistent. Also, please refer
> > to the pinctrl bindings by relative path and inline, rather than as a
> > footnote reference.
> 
> OK
> 
> >
> >> +
> >> +Optional parameters:
> >> +- st,breakinput: Set if the hardware have break input capabilities
> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
> >> +  The value define the active polarity:
> >> +   - 0 (active LOW)
> >> +   - 1 (active HIGH)
> >
> > Could we fold these into a single property? If st,breakinput-polarity is
> > not present it could simply mean that there is no break input, and if it
> > is present you don't have to rely on a default.
> 
> I need to know if I have to activate breakinput feature and on which level
> I will rewrite it like that:
> Optional parameters:
> - st,breakinput-polarity-high: Set if break input polarity is active
> on high level.
> - st,breakinput-polarity-high: Set if break input polarity is active
> on low level.

How is that different from a single property:

Optional properties:
- st,breakinput-polarity: If present, a break input is available
for the channel. In that case the property value denotes the
polarity of the break input:
- 0: active low
- 1: active high

?

> > The pwm- prefix is rather redundant since the node is already named pwm.
> > Why not simply st,channels? Or simply channels, since it's not really
> > anything specific to this hardware.
> >
> > Come to think of it, might be worth having a discussion with our DT
> > gurus about what their stance is on using the # as prefix for numbers
> > (such as in #address-cells or #size-cells). This could be #channels to
> > mark it more explicitly as representing a count.
> >
> >> +- st,32bits-counter: Set if the hardware have a 32 bits counter
> >> +- st,complementary:  Set if the hardware have complementary output 
> >> channels
> >
> > "hardware has" and also maybe mention explicitly that this is a boolean
> > property. Otherwise people might be left wondering what it should be set
> > to. Or maybe word this differently to imply that it's boolean:
> >
> > - st,32bits-counter: if present, the hardware has a 32 bit counter
> > - st,complementary: if present, the hardware has a complementary
> > output channel
> 
> I found a way to detect, at probe time, the number of channels, counter size,
> break input capability and complementary channels so I will remove
> "st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
> parameters

Oh hey, that's very neat. I suppose in that case my comment above about
the break input polarity is somewhat obsoleted. Still I think you won't
need two properties. Instead you can follow what other similar
properties have done: choose a default (often low-active) and have a
single optional property to override the default (often high-active).

In 

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Thierry Reding]

On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 7:53 GMT+01:00 Thierry Reding :
> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> >> Define bindings for pwm-stm32
> >>
> >> version 2:
> >> - use parameters instead of compatible of handle the hardware configuration
> >>
> >> Signed-off-by: Benjamin Gaignard 
> >> ---
> >>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> >> ++
> >>  1 file changed, 38 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> >> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> new file mode 100644
> >> index 000..575b9fb
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> @@ -0,0 +1,38 @@
> >> +STMicroelectronics PWM driver bindings for STM32
> >
> > Technically this bindings describe devices, so "driver binding" is a
> > somewhat odd wording. Perhaps:
> >
> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >
> > ?
>  done
> 
> >
> >> +
> >> +Must be a sub-node of STM32 general purpose timer driver
> >> +Parent node properties are describe in 
> >> ../mfd/stm32-general-purpose-timer.txt
> >
> > Again, "driver parent node" is odd. Perhaps:
> >
> > Must be a sub-node of an STM32 General Purpose Timer device tree
> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> > the parent node.
> >
> > ?
> 
> done
> 
> >
> >> +Required parameters:
> >> +- compatible:Must be "st,stm32-pwm"
> >> +- pinctrl-names: Set to "default".
> >> +- pinctrl-0: List of phandles pointing to pin 
> >> configuration nodes
> >> + for PWM module.
> >> + For Pinctrl properties, please refer to [1].
> >
> > Your indentation and capitalization are inconsistent. Also, please refer
> > to the pinctrl bindings by relative path and inline, rather than as a
> > footnote reference.
> 
> OK
> 
> >
> >> +
> >> +Optional parameters:
> >> +- st,breakinput: Set if the hardware have break input capabilities
> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
> >> +  The value define the active polarity:
> >> +   - 0 (active LOW)
> >> +   - 1 (active HIGH)
> >
> > Could we fold these into a single property? If st,breakinput-polarity is
> > not present it could simply mean that there is no break input, and if it
> > is present you don't have to rely on a default.
> 
> I need to know if I have to activate breakinput feature and on which level
> I will rewrite it like that:
> Optional parameters:
> - st,breakinput-polarity-high: Set if break input polarity is active
> on high level.
> - st,breakinput-polarity-high: Set if break input polarity is active
> on low level.

How is that different from a single property:

Optional properties:
- st,breakinput-polarity: If present, a break input is available
for the channel. In that case the property value denotes the
polarity of the break input:
- 0: active low
- 1: active high

?

> > The pwm- prefix is rather redundant since the node is already named pwm.
> > Why not simply st,channels? Or simply channels, since it's not really
> > anything specific to this hardware.
> >
> > Come to think of it, might be worth having a discussion with our DT
> > gurus about what their stance is on using the # as prefix for numbers
> > (such as in #address-cells or #size-cells). This could be #channels to
> > mark it more explicitly as representing a count.
> >
> >> +- st,32bits-counter: Set if the hardware have a 32 bits counter
> >> +- st,complementary:  Set if the hardware have complementary output 
> >> channels
> >
> > "hardware has" and also maybe mention explicitly that this is a boolean
> > property. Otherwise people might be left wondering what it should be set
> > to. Or maybe word this differently to imply that it's boolean:
> >
> > - st,32bits-counter: if present, the hardware has a 32 bit counter
> > - st,complementary: if present, the hardware has a complementary
> > output channel
> 
> I found a way to detect, at probe time, the number of channels, counter size,
> break input capability and complementary channels so I will remove
> "st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
> parameters

Oh hey, that's very neat. I suppose in that case my comment above about
the break input polarity is somewhat obsoleted. Still I think you won't
need two properties. Instead you can follow what other similar
properties have done: choose a default (often low-active) and have a
single optional property to override the default (often high-active).

In your case:

- st,breakinput-active-high: 

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Benjamin Gaignard]

2016-12-05 7:53 GMT+01:00 Thierry Reding :
> On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
>> Define bindings for pwm-stm32
>>
>> version 2:
>> - use parameters instead of compatible of handle the hardware configuration
>>
>> Signed-off-by: Benjamin Gaignard 
>> ---
>>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
>> ++
>>  1 file changed, 38 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
>> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> new file mode 100644
>> index 000..575b9fb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> @@ -0,0 +1,38 @@
>> +STMicroelectronics PWM driver bindings for STM32
>
> Technically this bindings describe devices, so "driver binding" is a
> somewhat odd wording. Perhaps:
>
> STMicroelectronics STM32 General Purpose Timer PWM bindings
>
> ?
 done

>
>> +
>> +Must be a sub-node of STM32 general purpose timer driver
>> +Parent node properties are describe in 
>> ../mfd/stm32-general-purpose-timer.txt
>
> Again, "driver parent node" is odd. Perhaps:
>
> Must be a sub-node of an STM32 General Purpose Timer device tree
> node. See ../mfd/stm32-general-purpose-timer.txt for details about
> the parent node.
>
> ?

done

>
>> +Required parameters:
>> +- compatible:Must be "st,stm32-pwm"
>> +- pinctrl-names: Set to "default".
>> +- pinctrl-0: List of phandles pointing to pin configuration 
>> nodes
>> + for PWM module.
>> + For Pinctrl properties, please refer to [1].
>
> Your indentation and capitalization are inconsistent. Also, please refer
> to the pinctrl bindings by relative path and inline, rather than as a
> footnote reference.

OK

>
>> +
>> +Optional parameters:
>> +- st,breakinput: Set if the hardware have break input capabilities
>> +- st,breakinput-polarity: Set break input polarity. Default is 0
>> +  The value define the active polarity:
>> +   - 0 (active LOW)
>> +   - 1 (active HIGH)
>
> Could we fold these into a single property? If st,breakinput-polarity is
> not present it could simply mean that there is no break input, and if it
> is present you don't have to rely on a default.

I need to know if I have to activate breakinput feature and on which level
I will rewrite it like that:
Optional parameters:
- st,breakinput-polarity-high: Set if break input polarity is active
on high level.
- st,breakinput-polarity-high: Set if break input polarity is active
on low level.

>> +- st,pwm-num-chan:   Number of available PWM channels.  Default is 0.
>
> The pwm- prefix is rather redundant since the node is already named pwm.
> Why not simply st,channels? Or simply channels, since it's not really
> anything specific to this hardware.
>
> Come to think of it, might be worth having a discussion with our DT
> gurus about what their stance is on using the # as prefix for numbers
> (such as in #address-cells or #size-cells). This could be #channels to
> mark it more explicitly as representing a count.
>
>> +- st,32bits-counter: Set if the hardware have a 32 bits counter
>> +- st,complementary:  Set if the hardware have complementary output channels
>
> "hardware has" and also maybe mention explicitly that this is a boolean
> property. Otherwise people might be left wondering what it should be set
> to. Or maybe word this differently to imply that it's boolean:
>
> - st,32bits-counter: if present, the hardware has a 32 bit counter
> - st,complementary: if present, the hardware has a complementary
> output channel

I found a way to detect, at probe time, the number of channels, counter size,
break input capability and complementary channels so I will remove
"st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
parameters

>
> Thierry

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Benjamin Gaignard]

2016-12-05 7:53 GMT+01:00 Thierry Reding :
> On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
>> Define bindings for pwm-stm32
>>
>> version 2:
>> - use parameters instead of compatible of handle the hardware configuration
>>
>> Signed-off-by: Benjamin Gaignard 
>> ---
>>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
>> ++
>>  1 file changed, 38 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
>> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> new file mode 100644
>> index 000..575b9fb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> @@ -0,0 +1,38 @@
>> +STMicroelectronics PWM driver bindings for STM32
>
> Technically this bindings describe devices, so "driver binding" is a
> somewhat odd wording. Perhaps:
>
> STMicroelectronics STM32 General Purpose Timer PWM bindings
>
> ?
 done

>
>> +
>> +Must be a sub-node of STM32 general purpose timer driver
>> +Parent node properties are describe in 
>> ../mfd/stm32-general-purpose-timer.txt
>
> Again, "driver parent node" is odd. Perhaps:
>
> Must be a sub-node of an STM32 General Purpose Timer device tree
> node. See ../mfd/stm32-general-purpose-timer.txt for details about
> the parent node.
>
> ?

done

>
>> +Required parameters:
>> +- compatible:Must be "st,stm32-pwm"
>> +- pinctrl-names: Set to "default".
>> +- pinctrl-0: List of phandles pointing to pin configuration 
>> nodes
>> + for PWM module.
>> + For Pinctrl properties, please refer to [1].
>
> Your indentation and capitalization are inconsistent. Also, please refer
> to the pinctrl bindings by relative path and inline, rather than as a
> footnote reference.

OK

>
>> +
>> +Optional parameters:
>> +- st,breakinput: Set if the hardware have break input capabilities
>> +- st,breakinput-polarity: Set break input polarity. Default is 0
>> +  The value define the active polarity:
>> +   - 0 (active LOW)
>> +   - 1 (active HIGH)
>
> Could we fold these into a single property? If st,breakinput-polarity is
> not present it could simply mean that there is no break input, and if it
> is present you don't have to rely on a default.

I need to know if I have to activate breakinput feature and on which level
I will rewrite it like that:
Optional parameters:
- st,breakinput-polarity-high: Set if break input polarity is active
on high level.
- st,breakinput-polarity-high: Set if break input polarity is active
on low level.

>> +- st,pwm-num-chan:   Number of available PWM channels.  Default is 0.
>
> The pwm- prefix is rather redundant since the node is already named pwm.
> Why not simply st,channels? Or simply channels, since it's not really
> anything specific to this hardware.
>
> Come to think of it, might be worth having a discussion with our DT
> gurus about what their stance is on using the # as prefix for numbers
> (such as in #address-cells or #size-cells). This could be #channels to
> mark it more explicitly as representing a count.
>
>> +- st,32bits-counter: Set if the hardware have a 32 bits counter
>> +- st,complementary:  Set if the hardware have complementary output channels
>
> "hardware has" and also maybe mention explicitly that this is a boolean
> property. Otherwise people might be left wondering what it should be set
> to. Or maybe word this differently to imply that it's boolean:
>
> - st,32bits-counter: if present, the hardware has a 32 bit counter
> - st,complementary: if present, the hardware has a complementary
> output channel

I found a way to detect, at probe time, the number of channels, counter size,
break input capability and complementary channels so I will remove
"st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
parameters

>
> Thierry

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Thierry Reding]

On Mon, Dec 05, 2016 at 08:35:35AM +, Lee Jones wrote:
> On Mon, 05 Dec 2016, Thierry Reding wrote:
> 
> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> > > Define bindings for pwm-stm32
> > > 
> > > version 2:
> > > - use parameters instead of compatible of handle the hardware 
> > > configuration
> > > 
> > > Signed-off-by: Benjamin Gaignard 
> > > ---
> > >  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> > > ++
> > >  1 file changed, 38 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> > > b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > new file mode 100644
> > > index 000..575b9fb
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > @@ -0,0 +1,38 @@
> > > +STMicroelectronics PWM driver bindings for STM32
> > 
> > Technically this bindings describe devices, so "driver binding" is a
> > somewhat odd wording. Perhaps:
> > 
> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> > 
> > ?
> > 
> > > +
> > > +Must be a sub-node of STM32 general purpose timer driver
> > > +Parent node properties are describe in 
> > > ../mfd/stm32-general-purpose-timer.txt
> > 
> > Again, "driver parent node" is odd. Perhaps:
> > 
> > Must be a sub-node of an STM32 General Purpose Timer device tree
> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> > the parent node.
> > 
> > ?
> > 
> > > +Required parameters:
> > > +- compatible:Must be "st,stm32-pwm"
> > > +- pinctrl-names: Set to "default".
> > > +- pinctrl-0: List of phandles pointing to pin configuration 
> > > nodes
> > > + for PWM module.
> > > + For Pinctrl properties, please refer to [1].
> > 
> > Your indentation and capitalization are inconsistent. Also, please refer
> > to the pinctrl bindings by relative path and inline, rather than as a
> > footnote reference.
> > 
> > > +
> > > +Optional parameters:
> > > +- st,breakinput: Set if the hardware have break input capabilities
> > > +- st,breakinput-polarity: Set break input polarity. Default is 0
> > > +  The value define the active polarity:
> > > +   - 0 (active LOW)
> > > +   - 1 (active HIGH)
> > 
> > Could we fold these into a single property? If st,breakinput-polarity is
> > not present it could simply mean that there is no break input, and if it
> > is present you don't have to rely on a default.
> > 
> > > +- st,pwm-num-chan:   Number of available PWM channels.  Default is 0.
> > 
> > The pwm- prefix is rather redundant since the node is already named pwm.
> > Why not simply st,channels? Or simply channels, since it's not really
> > anything specific to this hardware.
> > 
> > Come to think of it, might be worth having a discussion with our DT
> > gurus about what their stance is on using the # as prefix for numbers
> > (such as in #address-cells or #size-cells). This could be #channels to
> > mark it more explicitly as representing a count.
> 
> Unfortunately that ship has sailed.
> 
> st,pwm-num-chan already exists (with your blessing).  It's usually

I think I did at the time object, though very mildly. The property here
is somewhat different, though. For one this is a PWM specific node, so
the pwm- prefix is completely redundant. Also for pwm-sti where you had
introduced st,pwm-num-chan, the property denoted how many PWM channels
vs. capture channels (st,capture-num-chan) the device was supposed to
use. Here there are only one type of channels.

> suggested to reuse exiting properties when writing new bindings.

Given the above I think this case is different. Further my understanding
is that the desire to reuse existing properties is primarily for generic
properties. Vendor specific properties are always going to have to be
defined in the specific bindings, so it doesn't matter very much whether
they are reused or not.

Lastly, I think st,pwm-num-chan is not optimal, and while it isn't very
bad either, I do believe that when we see ways of improving things then
we should do so, regardless of whether existing ways to describe things
already exist. Especially if it comes at no additional cost.

All of that said, this is my opinion and if everybody thinks that the
st,pwm-num-chan is the better choice I'll merge it. Anyway, we'll need
the Acked-by from one of the device tree bindings maintainers and I'd
like to see at least an attempt at a discussion.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Thierry Reding]

On Mon, Dec 05, 2016 at 08:35:35AM +, Lee Jones wrote:
> On Mon, 05 Dec 2016, Thierry Reding wrote:
> 
> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> > > Define bindings for pwm-stm32
> > > 
> > > version 2:
> > > - use parameters instead of compatible of handle the hardware 
> > > configuration
> > > 
> > > Signed-off-by: Benjamin Gaignard 
> > > ---
> > >  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> > > ++
> > >  1 file changed, 38 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> > > b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > new file mode 100644
> > > index 000..575b9fb
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > @@ -0,0 +1,38 @@
> > > +STMicroelectronics PWM driver bindings for STM32
> > 
> > Technically this bindings describe devices, so "driver binding" is a
> > somewhat odd wording. Perhaps:
> > 
> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> > 
> > ?
> > 
> > > +
> > > +Must be a sub-node of STM32 general purpose timer driver
> > > +Parent node properties are describe in 
> > > ../mfd/stm32-general-purpose-timer.txt
> > 
> > Again, "driver parent node" is odd. Perhaps:
> > 
> > Must be a sub-node of an STM32 General Purpose Timer device tree
> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> > the parent node.
> > 
> > ?
> > 
> > > +Required parameters:
> > > +- compatible:Must be "st,stm32-pwm"
> > > +- pinctrl-names: Set to "default".
> > > +- pinctrl-0: List of phandles pointing to pin configuration 
> > > nodes
> > > + for PWM module.
> > > + For Pinctrl properties, please refer to [1].
> > 
> > Your indentation and capitalization are inconsistent. Also, please refer
> > to the pinctrl bindings by relative path and inline, rather than as a
> > footnote reference.
> > 
> > > +
> > > +Optional parameters:
> > > +- st,breakinput: Set if the hardware have break input capabilities
> > > +- st,breakinput-polarity: Set break input polarity. Default is 0
> > > +  The value define the active polarity:
> > > +   - 0 (active LOW)
> > > +   - 1 (active HIGH)
> > 
> > Could we fold these into a single property? If st,breakinput-polarity is
> > not present it could simply mean that there is no break input, and if it
> > is present you don't have to rely on a default.
> > 
> > > +- st,pwm-num-chan:   Number of available PWM channels.  Default is 0.
> > 
> > The pwm- prefix is rather redundant since the node is already named pwm.
> > Why not simply st,channels? Or simply channels, since it's not really
> > anything specific to this hardware.
> > 
> > Come to think of it, might be worth having a discussion with our DT
> > gurus about what their stance is on using the # as prefix for numbers
> > (such as in #address-cells or #size-cells). This could be #channels to
> > mark it more explicitly as representing a count.
> 
> Unfortunately that ship has sailed.
> 
> st,pwm-num-chan already exists (with your blessing).  It's usually

I think I did at the time object, though very mildly. The property here
is somewhat different, though. For one this is a PWM specific node, so
the pwm- prefix is completely redundant. Also for pwm-sti where you had
introduced st,pwm-num-chan, the property denoted how many PWM channels
vs. capture channels (st,capture-num-chan) the device was supposed to
use. Here there are only one type of channels.

> suggested to reuse exiting properties when writing new bindings.

Given the above I think this case is different. Further my understanding
is that the desire to reuse existing properties is primarily for generic
properties. Vendor specific properties are always going to have to be
defined in the specific bindings, so it doesn't matter very much whether
they are reused or not.

Lastly, I think st,pwm-num-chan is not optimal, and while it isn't very
bad either, I do believe that when we see ways of improving things then
we should do so, regardless of whether existing ways to describe things
already exist. Especially if it comes at no additional cost.

All of that said, this is my opinion and if everybody thinks that the
st,pwm-num-chan is the better choice I'll merge it. Anyway, we'll need
the Acked-by from one of the device tree bindings maintainers and I'd
like to see at least an attempt at a discussion.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Lee Jones]

On Mon, 05 Dec 2016, Thierry Reding wrote:

> On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> > Define bindings for pwm-stm32
> > 
> > version 2:
> > - use parameters instead of compatible of handle the hardware configuration
> > 
> > Signed-off-by: Benjamin Gaignard 
> > ---
> >  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> > ++
> >  1 file changed, 38 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> > b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > new file mode 100644
> > index 000..575b9fb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > @@ -0,0 +1,38 @@
> > +STMicroelectronics PWM driver bindings for STM32
> 
> Technically this bindings describe devices, so "driver binding" is a
> somewhat odd wording. Perhaps:
> 
>   STMicroelectronics STM32 General Purpose Timer PWM bindings
> 
> ?
> 
> > +
> > +Must be a sub-node of STM32 general purpose timer driver
> > +Parent node properties are describe in 
> > ../mfd/stm32-general-purpose-timer.txt
> 
> Again, "driver parent node" is odd. Perhaps:
> 
>   Must be a sub-node of an STM32 General Purpose Timer device tree
>   node. See ../mfd/stm32-general-purpose-timer.txt for details about
>   the parent node.
> 
> ?
> 
> > +Required parameters:
> > +- compatible:  Must be "st,stm32-pwm"
> > +- pinctrl-names:   Set to "default".
> > +- pinctrl-0:   List of phandles pointing to pin configuration 
> > nodes
> > +   for PWM module.
> > +   For Pinctrl properties, please refer to [1].
> 
> Your indentation and capitalization are inconsistent. Also, please refer
> to the pinctrl bindings by relative path and inline, rather than as a
> footnote reference.
> 
> > +
> > +Optional parameters:
> > +- st,breakinput:   Set if the hardware have break input capabilities
> > +- st,breakinput-polarity: Set break input polarity. Default is 0
> > +The value define the active polarity:
> > + - 0 (active LOW)
> > + - 1 (active HIGH)
> 
> Could we fold these into a single property? If st,breakinput-polarity is
> not present it could simply mean that there is no break input, and if it
> is present you don't have to rely on a default.
> 
> > +- st,pwm-num-chan: Number of available PWM channels.  Default is 0.
> 
> The pwm- prefix is rather redundant since the node is already named pwm.
> Why not simply st,channels? Or simply channels, since it's not really
> anything specific to this hardware.
> 
> Come to think of it, might be worth having a discussion with our DT
> gurus about what their stance is on using the # as prefix for numbers
> (such as in #address-cells or #size-cells). This could be #channels to
> mark it more explicitly as representing a count.

Unfortunately that ship has sailed.

st,pwm-num-chan already exists (with your blessing).  It's usually
suggested to reuse exiting properties when writing new bindings.

> > +- st,32bits-counter:   Set if the hardware have a 32 bits counter
> > +- st,complementary:Set if the hardware have complementary output 
> > channels
> 
> "hardware has" and also maybe mention explicitly that this is a boolean
> property. Otherwise people might be left wondering what it should be set
> to. Or maybe word this differently to imply that it's boolean:
> 
>   - st,32bits-counter: if present, the hardware has a 32 bit counter
>   - st,complementary: if present, the hardware has a complementary
>   output channel
> 
> Thierry



-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Lee Jones]

On Mon, 05 Dec 2016, Thierry Reding wrote:

> On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> > Define bindings for pwm-stm32
> > 
> > version 2:
> > - use parameters instead of compatible of handle the hardware configuration
> > 
> > Signed-off-by: Benjamin Gaignard 
> > ---
> >  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> > ++
> >  1 file changed, 38 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> > b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > new file mode 100644
> > index 000..575b9fb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > @@ -0,0 +1,38 @@
> > +STMicroelectronics PWM driver bindings for STM32
> 
> Technically this bindings describe devices, so "driver binding" is a
> somewhat odd wording. Perhaps:
> 
>   STMicroelectronics STM32 General Purpose Timer PWM bindings
> 
> ?
> 
> > +
> > +Must be a sub-node of STM32 general purpose timer driver
> > +Parent node properties are describe in 
> > ../mfd/stm32-general-purpose-timer.txt
> 
> Again, "driver parent node" is odd. Perhaps:
> 
>   Must be a sub-node of an STM32 General Purpose Timer device tree
>   node. See ../mfd/stm32-general-purpose-timer.txt for details about
>   the parent node.
> 
> ?
> 
> > +Required parameters:
> > +- compatible:  Must be "st,stm32-pwm"
> > +- pinctrl-names:   Set to "default".
> > +- pinctrl-0:   List of phandles pointing to pin configuration 
> > nodes
> > +   for PWM module.
> > +   For Pinctrl properties, please refer to [1].
> 
> Your indentation and capitalization are inconsistent. Also, please refer
> to the pinctrl bindings by relative path and inline, rather than as a
> footnote reference.
> 
> > +
> > +Optional parameters:
> > +- st,breakinput:   Set if the hardware have break input capabilities
> > +- st,breakinput-polarity: Set break input polarity. Default is 0
> > +The value define the active polarity:
> > + - 0 (active LOW)
> > + - 1 (active HIGH)
> 
> Could we fold these into a single property? If st,breakinput-polarity is
> not present it could simply mean that there is no break input, and if it
> is present you don't have to rely on a default.
> 
> > +- st,pwm-num-chan: Number of available PWM channels.  Default is 0.
> 
> The pwm- prefix is rather redundant since the node is already named pwm.
> Why not simply st,channels? Or simply channels, since it's not really
> anything specific to this hardware.
> 
> Come to think of it, might be worth having a discussion with our DT
> gurus about what their stance is on using the # as prefix for numbers
> (such as in #address-cells or #size-cells). This could be #channels to
> mark it more explicitly as representing a count.

Unfortunately that ship has sailed.

st,pwm-num-chan already exists (with your blessing).  It's usually
suggested to reuse exiting properties when writing new bindings.

> > +- st,32bits-counter:   Set if the hardware have a 32 bits counter
> > +- st,complementary:Set if the hardware have complementary output 
> > channels
> 
> "hardware has" and also maybe mention explicitly that this is a boolean
> property. Otherwise people might be left wondering what it should be set
> to. Or maybe word this differently to imply that it's boolean:
> 
>   - st,32bits-counter: if present, the hardware has a 32 bit counter
>   - st,complementary: if present, the hardware has a complementary
>   output channel
> 
> Thierry



-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Thierry Reding]

On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> Define bindings for pwm-stm32
> 
> version 2:
> - use parameters instead of compatible of handle the hardware configuration
> 
> Signed-off-by: Benjamin Gaignard 
> ---
>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> ++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> new file mode 100644
> index 000..575b9fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> @@ -0,0 +1,38 @@
> +STMicroelectronics PWM driver bindings for STM32

Technically this bindings describe devices, so "driver binding" is a
somewhat odd wording. Perhaps:

STMicroelectronics STM32 General Purpose Timer PWM bindings

?

> +
> +Must be a sub-node of STM32 general purpose timer driver
> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt

Again, "driver parent node" is odd. Perhaps:

Must be a sub-node of an STM32 General Purpose Timer device tree
node. See ../mfd/stm32-general-purpose-timer.txt for details about
the parent node.

?

> +Required parameters:
> +- compatible:Must be "st,stm32-pwm"
> +- pinctrl-names: Set to "default".
> +- pinctrl-0: List of phandles pointing to pin configuration 
> nodes
> + for PWM module.
> + For Pinctrl properties, please refer to [1].

Your indentation and capitalization are inconsistent. Also, please refer
to the pinctrl bindings by relative path and inline, rather than as a
footnote reference.

> +
> +Optional parameters:
> +- st,breakinput: Set if the hardware have break input capabilities
> +- st,breakinput-polarity: Set break input polarity. Default is 0
> +  The value define the active polarity:
> +   - 0 (active LOW)
> +   - 1 (active HIGH)

Could we fold these into a single property? If st,breakinput-polarity is
not present it could simply mean that there is no break input, and if it
is present you don't have to rely on a default.

> +- st,pwm-num-chan:   Number of available PWM channels.  Default is 0.

The pwm- prefix is rather redundant since the node is already named pwm.
Why not simply st,channels? Or simply channels, since it's not really
anything specific to this hardware.

Come to think of it, might be worth having a discussion with our DT
gurus about what their stance is on using the # as prefix for numbers
(such as in #address-cells or #size-cells). This could be #channels to
mark it more explicitly as representing a count.

> +- st,32bits-counter: Set if the hardware have a 32 bits counter
> +- st,complementary:  Set if the hardware have complementary output channels

"hardware has" and also maybe mention explicitly that this is a boolean
property. Otherwise people might be left wondering what it should be set
to. Or maybe word this differently to imply that it's boolean:

- st,32bits-counter: if present, the hardware has a 32 bit counter
- st,complementary: if present, the hardware has a complementary
output channel

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Thierry Reding]

On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> Define bindings for pwm-stm32
> 
> version 2:
> - use parameters instead of compatible of handle the hardware configuration
> 
> Signed-off-by: Benjamin Gaignard 
> ---
>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> ++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> new file mode 100644
> index 000..575b9fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> @@ -0,0 +1,38 @@
> +STMicroelectronics PWM driver bindings for STM32

Technically this bindings describe devices, so "driver binding" is a
somewhat odd wording. Perhaps:

STMicroelectronics STM32 General Purpose Timer PWM bindings

?

> +
> +Must be a sub-node of STM32 general purpose timer driver
> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt

Again, "driver parent node" is odd. Perhaps:

Must be a sub-node of an STM32 General Purpose Timer device tree
node. See ../mfd/stm32-general-purpose-timer.txt for details about
the parent node.

?

> +Required parameters:
> +- compatible:Must be "st,stm32-pwm"
> +- pinctrl-names: Set to "default".
> +- pinctrl-0: List of phandles pointing to pin configuration 
> nodes
> + for PWM module.
> + For Pinctrl properties, please refer to [1].

Your indentation and capitalization are inconsistent. Also, please refer
to the pinctrl bindings by relative path and inline, rather than as a
footnote reference.

> +
> +Optional parameters:
> +- st,breakinput: Set if the hardware have break input capabilities
> +- st,breakinput-polarity: Set break input polarity. Default is 0
> +  The value define the active polarity:
> +   - 0 (active LOW)
> +   - 1 (active HIGH)

Could we fold these into a single property? If st,breakinput-polarity is
not present it could simply mean that there is no break input, and if it
is present you don't have to rely on a default.

> +- st,pwm-num-chan:   Number of available PWM channels.  Default is 0.

The pwm- prefix is rather redundant since the node is already named pwm.
Why not simply st,channels? Or simply channels, since it's not really
anything specific to this hardware.

Come to think of it, might be worth having a discussion with our DT
gurus about what their stance is on using the # as prefix for numbers
(such as in #address-cells or #size-cells). This could be #channels to
mark it more explicitly as representing a count.

> +- st,32bits-counter: Set if the hardware have a 32 bits counter
> +- st,complementary:  Set if the hardware have complementary output channels

"hardware has" and also maybe mention explicitly that this is a boolean
property. Otherwise people might be left wondering what it should be set
to. Or maybe word this differently to imply that it's boolean:

- st,32bits-counter: if present, the hardware has a 32 bit counter
- st,complementary: if present, the hardware has a complementary
output channel

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Lee Jones]

On Fri, 02 Dec 2016, Benjamin Gaignard wrote:

> Define bindings for pwm-stm32
> 
> version 2:
> - use parameters instead of compatible of handle the hardware configuration
> 
> Signed-off-by: Benjamin Gaignard 
> ---
>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> ++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> new file mode 100644
> index 000..575b9fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> @@ -0,0 +1,38 @@
> +STMicroelectronics PWM driver bindings for STM32
> +
> +Must be a sub-node of STM32 general purpose timer driver
> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
> +
> +Required parameters:
> +- compatible:Must be "st,stm32-pwm"
> +- pinctrl-names: Set to "default".
> +- pinctrl-0: List of phandles pointing to pin configuration 
> nodes
> + for PWM module.
> + For Pinctrl properties, please refer to [1].
> +
> +Optional parameters:
> +- st,breakinput: Set if the hardware have break input capabilities
> +- st,breakinput-polarity: Set break input polarity. Default is 0
> +  The value define the active polarity:
> +   - 0 (active LOW)
> +   - 1 (active HIGH)

> +- st,breakinput-polarity-high

Then assume the default if the property is not present.

> +- st,pwm-num-chan:   Number of available PWM channels.  Default is 0.

What's the point in having a PWM device with  no channels?

Best to make this a compulsory property.

> +- st,32bits-counter: Set if the hardware have a 32 bits counter
> +- st,complementary:  Set if the hardware have complementary output channels
> +
> +[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Use relative path.

../pinctrl/pinctrl-bindings.txt

> +Example:
> + gptimer1: gptimer1@4001 {
> + compatible = "st,stm32-gptimer";
> + reg = <0x4001 0x400>;
> + clocks = < 0 160>;
> + clock-names = "clk_int";
> +
> + pwm1@0 {

Don't number the node name.

pwm@xx

> + compatible = "st,stm32-pwm";
> + st,pwm-num-chan = <4>;
> + st,breakinput;
> + st,complementary;
> + };
> + };

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Lee Jones]

On Fri, 02 Dec 2016, Benjamin Gaignard wrote:

> Define bindings for pwm-stm32
> 
> version 2:
> - use parameters instead of compatible of handle the hardware configuration
> 
> Signed-off-by: Benjamin Gaignard 
> ---
>  .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 
> ++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
> b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> new file mode 100644
> index 000..575b9fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> @@ -0,0 +1,38 @@
> +STMicroelectronics PWM driver bindings for STM32
> +
> +Must be a sub-node of STM32 general purpose timer driver
> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
> +
> +Required parameters:
> +- compatible:Must be "st,stm32-pwm"
> +- pinctrl-names: Set to "default".
> +- pinctrl-0: List of phandles pointing to pin configuration 
> nodes
> + for PWM module.
> + For Pinctrl properties, please refer to [1].
> +
> +Optional parameters:
> +- st,breakinput: Set if the hardware have break input capabilities
> +- st,breakinput-polarity: Set break input polarity. Default is 0
> +  The value define the active polarity:
> +   - 0 (active LOW)
> +   - 1 (active HIGH)

> +- st,breakinput-polarity-high

Then assume the default if the property is not present.

> +- st,pwm-num-chan:   Number of available PWM channels.  Default is 0.

What's the point in having a PWM device with  no channels?

Best to make this a compulsory property.

> +- st,32bits-counter: Set if the hardware have a 32 bits counter
> +- st,complementary:  Set if the hardware have complementary output channels
> +
> +[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Use relative path.

../pinctrl/pinctrl-bindings.txt

> +Example:
> + gptimer1: gptimer1@4001 {
> + compatible = "st,stm32-gptimer";
> + reg = <0x4001 0x400>;
> + clocks = < 0 160>;
> + clock-names = "clk_int";
> +
> + pwm1@0 {

Don't number the node name.

pwm@xx

> + compatible = "st,stm32-pwm";
> + st,pwm-num-chan = <4>;
> + st,breakinput;
> + st,complementary;
> + };
> + };

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Benjamin Gaignard]

Define bindings for pwm-stm32

version 2:
- use parameters instead of compatible of handle the hardware configuration

Signed-off-by: Benjamin Gaignard 
---
 .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 ++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
new file mode 100644
index 000..575b9fb
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
@@ -0,0 +1,38 @@
+STMicroelectronics PWM driver bindings for STM32
+
+Must be a sub-node of STM32 general purpose timer driver
+Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
+
+Required parameters:
+- compatible:  Must be "st,stm32-pwm"
+- pinctrl-names:   Set to "default".
+- pinctrl-0:   List of phandles pointing to pin configuration nodes
+   for PWM module.
+   For Pinctrl properties, please refer to [1].
+
+Optional parameters:
+- st,breakinput:   Set if the hardware have break input capabilities
+- st,breakinput-polarity: Set break input polarity. Default is 0
+The value define the active polarity:
+ - 0 (active LOW)
+ - 1 (active HIGH)
+- st,pwm-num-chan: Number of available PWM channels.  Default is 0.
+- st,32bits-counter:   Set if the hardware have a 32 bits counter
+- st,complementary:Set if the hardware have complementary output channels
+
+[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+
+Example:
+   gptimer1: gptimer1@4001 {
+   compatible = "st,stm32-gptimer";
+   reg = <0x4001 0x400>;
+   clocks = < 0 160>;
+   clock-names = "clk_int";
+
+   pwm1@0 {
+   compatible = "st,stm32-pwm";
+   st,pwm-num-chan = <4>;
+   st,breakinput;
+   st,complementary;
+   };
+   };
-- 
1.9.1

[PATCH v3 3/7] PWM: add pwm-stm32 DT bindings

[Benjamin Gaignard]

Define bindings for pwm-stm32

version 2:
- use parameters instead of compatible of handle the hardware configuration

Signed-off-by: Benjamin Gaignard 
---
 .../devicetree/bindings/pwm/pwm-stm32.txt  | 38 ++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt 
b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
new file mode 100644
index 000..575b9fb
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
@@ -0,0 +1,38 @@
+STMicroelectronics PWM driver bindings for STM32
+
+Must be a sub-node of STM32 general purpose timer driver
+Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
+
+Required parameters:
+- compatible:  Must be "st,stm32-pwm"
+- pinctrl-names:   Set to "default".
+- pinctrl-0:   List of phandles pointing to pin configuration nodes
+   for PWM module.
+   For Pinctrl properties, please refer to [1].
+
+Optional parameters:
+- st,breakinput:   Set if the hardware have break input capabilities
+- st,breakinput-polarity: Set break input polarity. Default is 0
+The value define the active polarity:
+ - 0 (active LOW)
+ - 1 (active HIGH)
+- st,pwm-num-chan: Number of available PWM channels.  Default is 0.
+- st,32bits-counter:   Set if the hardware have a 32 bits counter
+- st,complementary:Set if the hardware have complementary output channels
+
+[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+
+Example:
+   gptimer1: gptimer1@4001 {
+   compatible = "st,stm32-gptimer";
+   reg = <0x4001 0x400>;
+   clocks = < 0 160>;
+   clock-names = "clk_int";
+
+   pwm1@0 {
+   compatible = "st,stm32-pwm";
+   st,pwm-num-chan = <4>;
+   st,breakinput;
+   st,complementary;
+   };
+   };
-- 
1.9.1