From: thierry.reding@gmail•com (Thierry Reding)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings
Date: Mon, 5 Dec 2016 12:23:47 +0100 [thread overview]
Message-ID: <20161205112347.GF19891@ulmo.ba.sec> (raw)
In-Reply-To: <CA+M3ks4-JQNGYtdiXNj1J701x0njKD0_bf4yN1mLSjEcT2Z=6A@mail.gmail.com>
On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 7:53 GMT+01:00 Thierry Reding <thierry.reding@gmail•com>:
> > 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 <benjamin.gaignard@st•com>
> >> ---
> >> .../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 0000000..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: Some channels have a break input,
whose polarity will be active low by default. If this
property is present, the channel will be configured with an
active high polarity for the break input.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161205/85093ef8/attachment.sig>
next prev parent reply other threads:[~2016-12-05 11:23 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-02 10:17 [PATCH v3 0/7] Add pwm and IIO timer drivers for stm32 Benjamin Gaignard
2016-12-02 10:17 ` [PATCH v3 1/7] MFD: add bindings for stm32 general purpose timer driver Benjamin Gaignard
2016-12-02 10:17 ` [PATCH v3 2/7] MFD: add " Benjamin Gaignard
2016-12-02 14:29 ` Lee Jones
2016-12-02 10:17 ` [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings Benjamin Gaignard
2016-12-02 14:23 ` Lee Jones
2016-12-05 6:53 ` Thierry Reding
2016-12-05 8:35 ` Lee Jones
2016-12-05 10:50 ` Thierry Reding
2016-12-05 11:08 ` Benjamin Gaignard
2016-12-05 11:23 ` Thierry Reding [this message]
2016-12-05 12:12 ` Benjamin Gaignard
2016-12-05 12:21 ` Thierry Reding
2016-12-02 10:17 ` [PATCH v3 4/7] PWM: add pwm driver for stm32 plaftorm Benjamin Gaignard
2016-12-05 7:23 ` Thierry Reding
2016-12-05 8:31 ` Lee Jones
2016-12-05 11:02 ` Benjamin Gaignard
2016-12-05 11:30 ` Thierry Reding
2016-12-02 10:17 ` [PATCH v3 5/7] IIO: add bindings for stm32 timer trigger driver Benjamin Gaignard
2016-12-02 13:59 ` Lee Jones
2016-12-02 14:23 ` Benjamin Gaignard
2016-12-03 9:35 ` Jonathan Cameron
2016-12-02 10:17 ` [PATCH v3 6/7] IIO: add STM32 " Benjamin Gaignard
2016-12-02 13:57 ` Lee Jones
2016-12-02 10:17 ` [PATCH v3 7/7] ARM: dts: stm32: add stm32 general purpose timer driver in DT Benjamin Gaignard
2016-12-02 13:22 ` Lee Jones
2016-12-03 9:42 ` Jonathan Cameron
2016-12-05 8:54 ` Lee Jones
2016-12-05 16:09 ` Alexandre Torgue
2016-12-06 9:48 ` Lee Jones
2016-12-06 9:56 ` Alexandre Torgue
2016-12-06 12:54 ` Lee Jones
2016-12-02 13:41 ` Alexandre Torgue
2016-12-03 9:45 ` Jonathan Cameron
2016-12-03 9:32 ` [PATCH v3 0/7] Add pwm and IIO timer drivers for stm32 Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161205112347.GF19891@ulmo.ba.sec \
--to=thierry.reding@gmail$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox