| Message ID | f9d2c7cb01cbf31bf75c4160611fa1d37d99f355.1565703607.git.baolin.wang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation | expand |
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
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.
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
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.
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
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.
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
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.
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
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>; + };
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