Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Uwe Kleine-König]

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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |