[v2,1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation
diff mbox series

Message ID f9d2c7cb01cbf31bf75c4160611fa1d37d99f355.1565703607.git.baolin.wang@linaro.org
State New
Headers show
Series
  • [v2,1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation
Related show

Commit Message

Baolin Wang Aug. 13, 2019, 1:46 p.m. UTC
Add Spreadtrum PWM controller documentation.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v1:
 - Use assigned-clock-parents and assigned-clocks to set PWM clock parent.
---
 Documentation/devicetree/bindings/pwm/pwm-sprd.txt |   38 ++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt

Comments

Uwe Kleine-König Aug. 13, 2019, 2:12 p.m. UTC | #1
On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> Add Spreadtrum PWM controller documentation.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - Use assigned-clock-parents and assigned-clocks to set PWM clock parent.
> ---
>  Documentation/devicetree/bindings/pwm/pwm-sprd.txt |   38 ++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> new file mode 100644
> index 0000000..e6cf312
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> @@ -0,0 +1,38 @@
> +Spreadtrum PWM controller
> +
> +Spreadtrum SoCs PWM controller provides 4 PWM channels.
> +
> +Required porperties:

s/porperties/properties/

> +- compatible : Should be "sprd,ums512-pwm".
> +- reg: Physical base address and length of the controller's registers.
> +- clocks: The phandle and specifier referencing the controller's clocks.
> +- clock-names: Should contain following entries:
> +  "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
> +  "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
> +- assigned-clocks: Reference to the PWM clock entroes.

s/entroes/entries/

> +- assigned-clock-parents: The phandle of the parent clock of PWM clock.

I'm not sure you need to point out assigned-clocks and
assigned-clock-parents as this is general clk stuff. Also I wonder if
these should be "required properties".

> +- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of
> +  the cells format.

Best regards
Uwe
Baolin Wang Aug. 14, 2019, 1:51 a.m. UTC | #2
Hi Uwe,

On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > Add Spreadtrum PWM controller documentation.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > Changes from v1:
> >  - Use assigned-clock-parents and assigned-clocks to set PWM clock parent.
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-sprd.txt |   38 ++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> > new file mode 100644
> > index 0000000..e6cf312
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> > @@ -0,0 +1,38 @@
> > +Spreadtrum PWM controller
> > +
> > +Spreadtrum SoCs PWM controller provides 4 PWM channels.
> > +
> > +Required porperties:
>
> s/porperties/properties/

Sorry for typos, will fix in next version.

>
> > +- compatible : Should be "sprd,ums512-pwm".
> > +- reg: Physical base address and length of the controller's registers.
> > +- clocks: The phandle and specifier referencing the controller's clocks.
> > +- clock-names: Should contain following entries:
> > +  "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
> > +  "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
> > +- assigned-clocks: Reference to the PWM clock entroes.
>
> s/entroes/entries/

Sure.

>
> > +- assigned-clock-parents: The phandle of the parent clock of PWM clock.
>
> I'm not sure you need to point out assigned-clocks and
> assigned-clock-parents as this is general clk stuff. Also I wonder if
> these should be "required properties".

I think I should describe any properties used by PWM node, like
'clocks' and 'clock-names' properties, though they are common clock
properties.
Yes, they are required. Thanks for your comments.
Uwe Kleine-König Aug. 14, 2019, 7:01 a.m. UTC | #3
Hello Baolin,

On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > > +- assigned-clock-parents: The phandle of the parent clock of PWM clock.
> >
> > I'm not sure you need to point out assigned-clocks and
> > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > these should be "required properties".
> 
> I think I should describe any properties used by PWM node, like
> 'clocks' and 'clock-names' properties, though they are common clock
> properties.

Then you might want to describe also "status", "assigned-clock-rates",
"pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names" and
probably another dozen I'm not aware of.

> Yes, they are required. Thanks for your comments.

required in which sense? Why can a Spreadtrum PWM not work when the
clock parents are unspecified?

Best regards
Uwe
Baolin Wang Aug. 14, 2019, 7:25 a.m. UTC | #4
Hi Uwe,

On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > > > +- assigned-clock-parents: The phandle of the parent clock of PWM clock.
> > >
> > > I'm not sure you need to point out assigned-clocks and
> > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > > these should be "required properties".
> >
> > I think I should describe any properties used by PWM node, like
> > 'clocks' and 'clock-names' properties, though they are common clock
> > properties.
>
> Then you might want to describe also "status", "assigned-clock-rates",
> "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names" and
> probably another dozen I'm not aware of.

We usually do not describe 'status', but if your device node used
"pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
describe them to let users know what is the purpose of these
properties. That's also asked by DT maintainer Rob.

>
> > Yes, they are required. Thanks for your comments.
>
> required in which sense? Why can a Spreadtrum PWM not work when the
> clock parents are unspecified?

On some Spreadtrum platforms, the default source clock of PWM may not
be enabled, so we should force users to select one available source
clock for PWM output clock.
Uwe Kleine-König Aug. 14, 2019, 7:39 a.m. UTC | #5
On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
> Hi Uwe,
> 
> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello Baolin,
> >
> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM clock.
> > > >
> > > > I'm not sure you need to point out assigned-clocks and
> > > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > > > these should be "required properties".
> > >
> > > I think I should describe any properties used by PWM node, like
> > > 'clocks' and 'clock-names' properties, though they are common clock
> > > properties.
> >
> > Then you might want to describe also "status", "assigned-clock-rates",
> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names" and
> > probably another dozen I'm not aware of.
> 
> We usually do not describe 'status', but if your device node used
> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
> describe them to let users know what is the purpose of these
> properties. That's also asked by DT maintainer Rob.

Does this convince you that you should also describe "pinctrl-$n" and
the others? If not, why not? We also usually don't describe
assigned-clock-parents. For me these are all in the same catagory:
Common properties supported for each devicetree node that represents a
device. The only difference is that on your board you make use of some
but not some others.

> > > Yes, they are required. Thanks for your comments.
> >
> > required in which sense? Why can a Spreadtrum PWM not work when the
> > clock parents are unspecified?
> 
> On some Spreadtrum platforms, the default source clock of PWM may not
> be enabled, so we should force users to select one available source
> clock for PWM output clock.

Sounds like a bug in the clk tree of your SoC that shouldn't affect how
the PWM is described in the device tree. After all a parent of a clock
is supposed to become enabled when the clock gets enabled.

Best regards
Uwe
Baolin Wang Aug. 14, 2019, 7:52 a.m. UTC | #6
Hi Uwe,

On 14/08/2019, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
>> Hi Uwe,
>>
>> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> >
>> > Hello Baolin,
>> >
>> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
>> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
>> > > <u.kleine-koenig@pengutronix.de> wrote:
>> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
>> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM
>> > > > > clock.
>> > > >
>> > > > I'm not sure you need to point out assigned-clocks and
>> > > > assigned-clock-parents as this is general clk stuff. Also I wonder
>> > > > if
>> > > > these should be "required properties".
>> > >
>> > > I think I should describe any properties used by PWM node, like
>> > > 'clocks' and 'clock-names' properties, though they are common clock
>> > > properties.
>> >
>> > Then you might want to describe also "status", "assigned-clock-rates",
>> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names"
>> > and
>> > probably another dozen I'm not aware of.
>>
>> We usually do not describe 'status', but if your device node used
>> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
>> describe them to let users know what is the purpose of these
>> properties. That's also asked by DT maintainer Rob.
>
> Does this convince you that you should also describe "pinctrl-$n" and
> the others? If not, why not? We also usually don't describe

Our PWM device node did not use "pinctrl-$n" things, why I should
describe "pinctrl-$n"?
If some devices use pinctrl, yes, they should describe what is the
purpose of pinctrl, see:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mmc/sdhci-sprd.txt

> assigned-clock-parents. For me these are all in the same catagory:

Lots of dt bindings describe 'assigned-clock-parents',:
./Documentation/devicetree/bindings/display/msm/dsi.txt
./Documentation/devicetree/bindings/display/msm/dsi.txt
./Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.txt
./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
./Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt
./Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
......

> Common properties supported for each devicetree node that represents a
> device. The only difference is that on your board you make use of some
> but not some others.

Fine, let's decide this by PWM maintainer or DT maintainer Rob.

>
>> > > Yes, they are required. Thanks for your comments.
>> >
>> > required in which sense? Why can a Spreadtrum PWM not work when the
>> > clock parents are unspecified?
>>
>> On some Spreadtrum platforms, the default source clock of PWM may not
>> be enabled, so we should force users to select one available source
>> clock for PWM output clock.
>
> Sounds like a bug in the clk tree of your SoC that shouldn't affect how
> the PWM is described in the device tree. After all a parent of a clock
> is supposed to become enabled when the clock gets enabled.

That's not a bug, that's like a MUX, the default source clock of PWM
can be disabled, since we usually do not use the default source clock.
Then we can select a available source clock by the MUX.
Uwe Kleine-König Aug. 14, 2019, 8:49 a.m. UTC | #7
Hello,

On Wed, Aug 14, 2019 at 03:52:08PM +0800, Baolin Wang wrote:
> On 14/08/2019, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
> >> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
> >> <u.kleine-koenig@pengutronix.de> wrote:
> >> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> >> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> >> > > <u.kleine-koenig@pengutronix.de> wrote:
> >> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> >> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM
> >> > > > > clock.
> >> > > >
> >> > > > I'm not sure you need to point out assigned-clocks and
> >> > > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> >> > > > these should be "required properties".
> >> > >
> >> > > I think I should describe any properties used by PWM node, like
> >> > > 'clocks' and 'clock-names' properties, though they are common clock
> >> > > properties.
> >> >
> >> > Then you might want to describe also "status", "assigned-clock-rates",
> >> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names"
> >> > and
> >> > probably another dozen I'm not aware of.
> >>
> >> We usually do not describe 'status', but if your device node used
> >> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
> >> describe them to let users know what is the purpose of these
> >> properties. That's also asked by DT maintainer Rob.
> >
> > Does this convince you that you should also describe "pinctrl-$n" and
> > the others? If not, why not? We also usually don't describe
> 
> Our PWM device node did not use "pinctrl-$n" things, why I should
> describe "pinctrl-$n"?

The binding you implemented supports "pinctrl-$n". And this is described
somewhere in the generic pinctrl binding docs. The same applies to
"assigned-clock-parents".

That you happen to use assigned-clock-parents but not pinctrl-$n on the
board you used to develop your driver is a detail that IMHO shouldn't
decide about being listed in the binding doc for your PWM type.

> If some devices use pinctrl, yes, they should describe what is the
> purpose of pinctrl, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mmc/sdhci-sprd.txt

I agree that if the driver assumes special pinctrl names this is worth
mentioning. If however there is nothing special and just some generic
stuff is used that is described in some other location then I'd chose to
not add this redundant information to the binding document. So if I
reviewed the patch that created
Documentation/devicetree/bindings/mmc/sdhci-sprd.txt I would have
suggested to drop "assigned-clocks" and "assigned-clock-parents" there,
too.

> > assigned-clock-parents. For me these are all in the same catagory:
> 
> Lots of dt bindings describe 'assigned-clock-parents',:
> ./Documentation/devicetree/bindings/display/msm/dsi.txt
> ./Documentation/devicetree/bindings/display/msm/dsi.txt
> ./Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.txt
> ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> ./Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt
> ./Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
> ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> ......

I didn't check each of them, but I assume the same applies here, too.
Please don't copy blindly but think before using other people's stuff as
reference. Even in the Linux kernel where reviews seem and are told to
catch non-optimal stuff, there are places where this doesn't work. IMHO
the key question is: Does it add value to describe "assigned-clocks" in
the binding for your PWM device given that you're only using this
generic and well documented construct?

> > Common properties supported for each devicetree node that represents a
> > device. The only difference is that on your board you make use of some
> > but not some others.
> 
> Fine, let's decide this by PWM maintainer or DT maintainer Rob.

Fine for me.

> >> > > Yes, they are required. Thanks for your comments.
> >> >
> >> > required in which sense? Why can a Spreadtrum PWM not work when the
> >> > clock parents are unspecified?
> >>
> >> On some Spreadtrum platforms, the default source clock of PWM may not
> >> be enabled, so we should force users to select one available source
> >> clock for PWM output clock.
> >
> > Sounds like a bug in the clk tree of your SoC that shouldn't affect how
> > the PWM is described in the device tree. After all a parent of a clock
> > is supposed to become enabled when the clock gets enabled.
> 
> That's not a bug, that's like a MUX, the default source clock of PWM
> can be disabled, since we usually do not use the default source clock.
> Then we can select a available source clock by the MUX.

In my eyes there is a difference between a) The way the clocks are
implemented in the XZ SoC implies that to actually use the PWM you need
to reparent some clock; and b) Each "sprd,ums512-pwm" device really
needs an "assigned-clock" property, otherwise it cannot work.

If you write "required" in the binding doc the semantic should be b) but
the motivation here seems to be a). Legal questions aside someone could
implement a PWM that has the same register layout and behaviour as the
PWM in your SoC but with a different clock tree. Should they use a
different compatible just because they don't need "assigned-clock"?

Best regards
Uwe
Baolin Wang Aug. 14, 2019, 9:33 a.m. UTC | #8
On Wed, 14 Aug 2019 at 16:49, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Wed, Aug 14, 2019 at 03:52:08PM +0800, Baolin Wang wrote:
> > On 14/08/2019, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > > On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
> > >> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
> > >> <u.kleine-koenig@pengutronix.de> wrote:
> > >> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> > >> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> > >> > > <u.kleine-koenig@pengutronix.de> wrote:
> > >> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > >> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM
> > >> > > > > clock.
> > >> > > >
> > >> > > > I'm not sure you need to point out assigned-clocks and
> > >> > > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > >> > > > these should be "required properties".
> > >> > >
> > >> > > I think I should describe any properties used by PWM node, like
> > >> > > 'clocks' and 'clock-names' properties, though they are common clock
> > >> > > properties.
> > >> >
> > >> > Then you might want to describe also "status", "assigned-clock-rates",
> > >> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names"
> > >> > and
> > >> > probably another dozen I'm not aware of.
> > >>
> > >> We usually do not describe 'status', but if your device node used
> > >> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
> > >> describe them to let users know what is the purpose of these
> > >> properties. That's also asked by DT maintainer Rob.
> > >
> > > Does this convince you that you should also describe "pinctrl-$n" and
> > > the others? If not, why not? We also usually don't describe
> >
> > Our PWM device node did not use "pinctrl-$n" things, why I should
> > describe "pinctrl-$n"?
>
> The binding you implemented supports "pinctrl-$n". And this is described
> somewhere in the generic pinctrl binding docs. The same applies to
> "assigned-clock-parents".
>
> That you happen to use assigned-clock-parents but not pinctrl-$n on the
> board you used to develop your driver is a detail that IMHO shouldn't
> decide about being listed in the binding doc for your PWM type.
>
> > If some devices use pinctrl, yes, they should describe what is the
> > purpose of pinctrl, see:
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mmc/sdhci-sprd.txt
>
> I agree that if the driver assumes special pinctrl names this is worth
> mentioning. If however there is nothing special and just some generic
> stuff is used that is described in some other location then I'd chose to
> not add this redundant information to the binding document. So if I
> reviewed the patch that created
> Documentation/devicetree/bindings/mmc/sdhci-sprd.txt I would have
> suggested to drop "assigned-clocks" and "assigned-clock-parents" there,
> too.
>
> > > assigned-clock-parents. For me these are all in the same catagory:
> >
> > Lots of dt bindings describe 'assigned-clock-parents',:
> > ./Documentation/devicetree/bindings/display/msm/dsi.txt
> > ./Documentation/devicetree/bindings/display/msm/dsi.txt
> > ./Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.txt
> > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > ./Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt
> > ./Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
> > ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> > ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> > ......
>
> I didn't check each of them, but I assume the same applies here, too.
> Please don't copy blindly but think before using other people's stuff as

I did not  copy blindly.

> reference. Even in the Linux kernel where reviews seem and are told to
> catch non-optimal stuff, there are places where this doesn't work. IMHO
> the key question is: Does it add value to describe "assigned-clocks" in
> the binding for your PWM device given that you're only using this
> generic and well documented construct?

I just want to remind users that they should set the parent clock for
PWMs, otherwise the PWM source clock can be not available.

>
> > > Common properties supported for each devicetree node that represents a
> > > device. The only difference is that on your board you make use of some
> > > but not some others.
> >
> > Fine, let's decide this by PWM maintainer or DT maintainer Rob.
>
> Fine for me.
>
> > >> > > Yes, they are required. Thanks for your comments.
> > >> >
> > >> > required in which sense? Why can a Spreadtrum PWM not work when the
> > >> > clock parents are unspecified?
> > >>
> > >> On some Spreadtrum platforms, the default source clock of PWM may not
> > >> be enabled, so we should force users to select one available source
> > >> clock for PWM output clock.
> > >
> > > Sounds like a bug in the clk tree of your SoC that shouldn't affect how
> > > the PWM is described in the device tree. After all a parent of a clock
> > > is supposed to become enabled when the clock gets enabled.
> >
> > That's not a bug, that's like a MUX, the default source clock of PWM
> > can be disabled, since we usually do not use the default source clock.
> > Then we can select a available source clock by the MUX.
>
> In my eyes there is a difference between a) The way the clocks are
> implemented in the XZ SoC implies that to actually use the PWM you need
> to reparent some clock; and b) Each "sprd,ums512-pwm" device really
> needs an "assigned-clock" property, otherwise it cannot work.
>
> If you write "required" in the binding doc the semantic should be b) but
> the motivation here seems to be a). Legal questions aside someone could
> implement a PWM that has the same register layout and behaviour as the
> PWM in your SoC but with a different clock tree. Should they use a
> different compatible just because they don't need "assigned-clock"?

Fair enough, I move them to be optional.
Uwe Kleine-König Aug. 14, 2019, 9:46 a.m. UTC | #9
Hello,

On Wed, Aug 14, 2019 at 05:33:25PM +0800, Baolin Wang wrote:
> On Wed, 14 Aug 2019 at 16:49, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Aug 14, 2019 at 03:52:08PM +0800, Baolin Wang wrote:
> > > On 14/08/2019, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
> > > >> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
> > > >> <u.kleine-koenig@pengutronix.de> wrote:
> > > >> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> > > >> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> > > >> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > > >> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM
> > > >> > > > > clock.
> > > >> > > >
> > > >> > > > I'm not sure you need to point out assigned-clocks and
> > > >> > > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > > >> > > > these should be "required properties".
> > > >> > >
> > > >> > > I think I should describe any properties used by PWM node, like
> > > >> > > 'clocks' and 'clock-names' properties, though they are common clock
> > > >> > > properties.
> > > >> >
> > > >> > Then you might want to describe also "status", "assigned-clock-rates",
> > > >> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names"
> > > >> > and
> > > >> > probably another dozen I'm not aware of.
> > > >>
> > > >> We usually do not describe 'status', but if your device node used
> > > >> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
> > > >> describe them to let users know what is the purpose of these
> > > >> properties. That's also asked by DT maintainer Rob.
> > > >
> > > > Does this convince you that you should also describe "pinctrl-$n" and
> > > > the others? If not, why not? We also usually don't describe
> > >
> > > Our PWM device node did not use "pinctrl-$n" things, why I should
> > > describe "pinctrl-$n"?
> >
> > The binding you implemented supports "pinctrl-$n". And this is described
> > somewhere in the generic pinctrl binding docs. The same applies to
> > "assigned-clock-parents".
> >
> > That you happen to use assigned-clock-parents but not pinctrl-$n on the
> > board you used to develop your driver is a detail that IMHO shouldn't
> > decide about being listed in the binding doc for your PWM type.
> >
> > > If some devices use pinctrl, yes, they should describe what is the
> > > purpose of pinctrl, see:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mmc/sdhci-sprd.txt
> >
> > I agree that if the driver assumes special pinctrl names this is worth
> > mentioning. If however there is nothing special and just some generic
> > stuff is used that is described in some other location then I'd chose to
> > not add this redundant information to the binding document. So if I
> > reviewed the patch that created
> > Documentation/devicetree/bindings/mmc/sdhci-sprd.txt I would have
> > suggested to drop "assigned-clocks" and "assigned-clock-parents" there,
> > too.
> >
> > > > assigned-clock-parents. For me these are all in the same catagory:
> > >
> > > Lots of dt bindings describe 'assigned-clock-parents',:
> > > ./Documentation/devicetree/bindings/display/msm/dsi.txt
> > > ./Documentation/devicetree/bindings/display/msm/dsi.txt
> > > ./Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.txt
> > > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > > ./Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt
> > > ./Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
> > > ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> > > ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> > > ......
> >
> > I didn't check each of them, but I assume the same applies here, too.
> > Please don't copy blindly but think before using other people's stuff as
> 
> I did not  copy blindly.

OK, there was no offence intended.

> > reference. Even in the Linux kernel where reviews seem and are told to
> > catch non-optimal stuff, there are places where this doesn't work. IMHO
> > the key question is: Does it add value to describe "assigned-clocks" in
> > the binding for your PWM device given that you're only using this
> > generic and well documented construct?
> 
> I just want to remind users that they should set the parent clock for
> PWMs, otherwise the PWM source clock can be not available.

Probably it is just subjective where to draw the line here. There are a
thousand and one things that can go wrong when the PWM should be used.
To me it seems artificial to pick one of these and mention it in a
document that is supposed to describe how to formalize such a device.

But given that we're going in cycles, I will stop trying to convince you
now.

Best regards
Uwe

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
new file mode 100644
index 0000000..e6cf312
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
@@ -0,0 +1,38 @@ 
+Spreadtrum PWM controller
+
+Spreadtrum SoCs PWM controller provides 4 PWM channels.
+
+Required porperties:
+- compatible : Should be "sprd,ums512-pwm".
+- reg: Physical base address and length of the controller's registers.
+- clocks: The phandle and specifier referencing the controller's clocks.
+- clock-names: Should contain following entries:
+  "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
+  "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
+- assigned-clocks: Reference to the PWM clock entroes.
+- assigned-clock-parents: The phandle of the parent clock of PWM clock.
+- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of
+  the cells format.
+
+Example:
+	pwms: pwm@32260000 {
+		compatible = "sprd,ums512-pwm";
+		reg = <0 0x32260000 0 0x10000>;
+		clock-names = "pwm0", "enable0",
+			"pwm1", "enable1",
+			"pwm2", "enable2",
+			"pwm3", "enable3";
+		clocks = <&aon_clk CLK_PWM0>, <&aonapb_gate CLK_PWM0_EB>,
+		       <&aon_clk CLK_PWM1>, <&aonapb_gate CLK_PWM1_EB>,
+		       <&aon_clk CLK_PWM2>, <&aonapb_gate CLK_PWM2_EB>,
+		       <&aon_clk CLK_PWM3>, <&aonapb_gate CLK_PWM3_EB>;
+		assigned-clocks = <&aon_clk CLK_PWM0>,
+			<&aon_clk CLK_PWM1>,
+			<&aon_clk CLK_PWM2>,
+			<&aon_clk CLK_PWM3>;
+		assigned-clock-parents = <&ext_26m>,
+			<&ext_26m>,
+			<&ext_26m>,
+			<&ext_26m>;
+		#pwm-cells = <2>;
+	};