| Message ID | 20181125161859.GA5277@arx-s1 |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PWM support for allwinner sun8i R40/T3/V40 SOCs. | expand |
On Mon, 26 Nov 2018 00:18:59 +0800, Hao Zhang wrote: > This patch adds Allwinner sun8i pwm binding document. > > Signed-off-by: Hao Zhang <hao5781286@gmail.com> > --- > .../devicetree/bindings/pwm/pwm-sun8i.txt | 24 ++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > Reviewed-by: Rob Herring <robh@kernel.org>
Hello, On Mon, Nov 26, 2018 at 12:18:59AM +0800, Hao Zhang wrote: > This patch adds Allwinner sun8i pwm binding document. > > Signed-off-by: Hao Zhang <hao5781286@gmail.com> > --- > .../devicetree/bindings/pwm/pwm-sun8i.txt | 24 ++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > new file mode 100644 > index 0000000..7531d85 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > @@ -0,0 +1,24 @@ > +Allwinner sun8i R40/V40/T3 SoC PWM controller > + > +Required properties: > + - compatible: Should be one of: > + - "allwinner,sun8i-r40-pwm" > + - reg: Physical base address and length of the controller's registers > + - interrupts: Should contain interrupt. > + - clocks: From common clock binding, handle to the parent clock. > + - clock-names: Must contain the clock names described just above. > + - pwm-channels: PWM channels of the controller. > + - #pwm-cells: Should be 3. See pwm.txt in this directory for a description of > + the cells format. I wonder why "interrupts" is needed here. I guess this is only needed for waveform capture? Is this only "optional"? The driver doesn't use it. Apart from this interrupts property this is all pretty standard and I wonder if we could merge several documents into one. For example Documentation/devicetree/bindings/pwm/pwm-st.txt looks identically apart from "pwm-channels" being called "st,pwm-num-chan" there. (It even has an interrupts property. Should the st driver move to "pwm-channels", too?) Best regards Uwe
On Mon, Nov 26, 2018 at 12:18:59AM +0800, Hao Zhang wrote: > This patch adds Allwinner sun8i pwm binding document. > > Signed-off-by: Hao Zhang <hao5781286@gmail.com> > --- > .../devicetree/bindings/pwm/pwm-sun8i.txt | 24 ++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > new file mode 100644 > index 0000000..7531d85 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > @@ -0,0 +1,24 @@ > +Allwinner sun8i R40/V40/T3 SoC PWM controller > + > +Required properties: > + - compatible: Should be one of: > + - "allwinner,sun8i-r40-pwm" > + - reg: Physical base address and length of the controller's registers > + - interrupts: Should contain interrupt. > + - clocks: From common clock binding, handle to the parent clock. > + - clock-names: Must contain the clock names described just above. You didn't describe those names in that document. You seem to have used mux-0 and mux-1 for the clock names. I guess we don't have to use a name there, we can simply use the position to find out (as long as it's documented in the binding) Maxime
Hello, On Tue, Nov 27, 2018 at 08:52:26AM +0100, Maxime Ripard wrote: > On Mon, Nov 26, 2018 at 12:18:59AM +0800, Hao Zhang wrote: > > + - clocks: From common clock binding, handle to the parent clock. > > + - clock-names: Must contain the clock names described just above. > > [...] > > You seem to have used mux-0 and mux-1 for the clock names. I guess we > don't have to use a name there, we can simply use the position to find > out (as long as it's documented in the binding) I also wondered if the driver relies on the fact that the second clock is the faster running one. Is this sensible? Best regards Uwe
On Tue, Nov 27, 2018 at 09:35:23AM +0100, Uwe Kleine-König wrote: > Hello, > > On Tue, Nov 27, 2018 at 08:52:26AM +0100, Maxime Ripard wrote: > > On Mon, Nov 26, 2018 at 12:18:59AM +0800, Hao Zhang wrote: > > > + - clocks: From common clock binding, handle to the parent clock. > > > + - clock-names: Must contain the clock names described just above. > > > > [...] > > > > You seem to have used mux-0 and mux-1 for the clock names. I guess we > > don't have to use a name there, we can simply use the position to find > > out (as long as it's documented in the binding) > > I also wondered if the driver relies on the fact that the second clock > is the faster running one. Is this sensible? Not really, I'm not sure we can make those expectations in the DT binding, especially since clock rate can change at runtime. Maxime
Hi! (Please keep all the recipiens in Cc) On Sun, Dec 02, 2018 at 12:13:21AM +0800, Hao Zhang wrote: > Maxime Ripard <maxime.ripard@bootlin.com> 于2018年11月27日周二 下午6:33写道： > > > > On Tue, Nov 27, 2018 at 09:35:23AM +0100, Uwe Kleine-König wrote: > > > Hello, > > > > > > On Tue, Nov 27, 2018 at 08:52:26AM +0100, Maxime Ripard wrote: > > > > On Mon, Nov 26, 2018 at 12:18:59AM +0800, Hao Zhang wrote: > > > > > + - clocks: From common clock binding, handle to the parent clock. > > > > > + - clock-names: Must contain the clock names described just above. > > > > > > > > [...] > > > > > > > > You seem to have used mux-0 and mux-1 for the clock names. I guess we > > > > don't have to use a name there, we can simply use the position to find > > > > out (as long as it's documented in the binding) > > > > > > I also wondered if the driver relies on the fact that the second clock > > > is the faster running one. Is this sensible? > > > > Not really, I'm not sure we can make those expectations in the DT > > binding, especially since clock rate can change at runtime. > > How about just add one clock on DT, most of the time, 24MHZ is enough > (apb1 is 100MHZ) > other one just use as a optional. > clock rate change at runtime would make the same pair pwm channel > uncontrollable, > because previous one would be change by the new one different setting. The DT is a hardware representation. If the hardware block can use both clocks, it should be described. Now, you can totally use only one clock of these 2 in your driver if that's easier / more reasonable. Maxime
On Mon, Nov 26, 2018 at 12:18:59AM +0800, Hao Zhang wrote: > This patch adds Allwinner sun8i pwm binding document. > > Signed-off-by: Hao Zhang <hao5781286@gmail.com> > --- > .../devicetree/bindings/pwm/pwm-sun8i.txt | 24 ++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > new file mode 100644 > index 0000000..7531d85 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > @@ -0,0 +1,24 @@ > +Allwinner sun8i R40/V40/T3 SoC PWM controller > + > +Required properties: > + - compatible: Should be one of: > + - "allwinner,sun8i-r40-pwm" > + - reg: Physical base address and length of the controller's registers > + - interrupts: Should contain interrupt. > + - clocks: From common clock binding, handle to the parent clock. > + - clock-names: Must contain the clock names described just above. > + - pwm-channels: PWM channels of the controller. Why do you need this? In the cover letter you say: "The sun8i R40/T3/V40 PWM has 8 PWM channals ..." Why does this need to be specified in the DT? Thierry
---------- Forwarded message --------- From: Hao Zhang <hao5781286@gmail.com> Date: 2019年3月12日周二 下午12:59 Subject: Re: [PATCH v3 1/6] Documentation: ARM: sunxi: pwm: add Allwinner sun8i. To: Thierry Reding <thierry.reding@gmail.com> Thierry Reding <thierry.reding@gmail.com> 于2018年12月21日周五 上午1:50写道： > > On Mon, Nov 26, 2018 at 12:18:59AM +0800, Hao Zhang wrote: > > This patch adds Allwinner sun8i pwm binding document. > > > > Signed-off-by: Hao Zhang <hao5781286@gmail.com> > > --- > > .../devicetree/bindings/pwm/pwm-sun8i.txt | 24 ++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > > new file mode 100644 > > index 0000000..7531d85 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt > > @@ -0,0 +1,24 @@ > > +Allwinner sun8i R40/V40/T3 SoC PWM controller > > + > > +Required properties: > > + - compatible: Should be one of: > > + - "allwinner,sun8i-r40-pwm" > > + - reg: Physical base address and length of the controller's registers > > + - interrupts: Should contain interrupt. > > + - clocks: From common clock binding, handle to the parent clock. > > + - clock-names: Must contain the clock names described just above. > > + - pwm-channels: PWM channels of the controller. > > Why do you need this? In the cover letter you say: > > "The sun8i R40/T3/V40 PWM has 8 PWM channals ..." > > Why does this need to be specified in the DT? T3 PWM has 8 channals, i think it is necessary to tell user how to specify it Instead of hardcode the channal myself :) Thanks for review :) > > Thierry > -----BEGIN PGP SIGNATURE----- > > iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAlwb1k4ACgkQ3SOs138+ > s6EOyA//auHjqjKwvjCLwWgHXdVr26cFUnFn/Ml6ZHHRe+oLCiYsatv4AZfGFvZ7 > CIGWN3zUu9c5YoDOd1isauQYgRtTsShWYC4gPxFFK9hWfb8f3o/wG60whkNDuvLL > 1SAQ/KJTC01LQIEXfHlb60EPvKCtt4YUQG4PkTGBGOHSO+MhWQHRLy5aLaq+d3yH > KHaDZ0PuZvYNnFWi7W/ggraiIlRToPH8HanFzGew+gUfPjClrczjrGqgn8u0bAL6 > MuKDMHLgjI/D8cs7XIaXc/OCPsp69B4JGrRJsxYh0KGKthaYDeKAUERpvsltDhGT > oTB55mJPZlriaiEOSwPrj+M0JQe9AnUIBVEiSIP2dSn8+rcSlWd10ysEjnCH/Ea7 > ARkamiRCk2hgOhZlDZcm+hjh7VxnJinaYahGFXMszpVgCScHT/fjZKexGqX8NJa8 > EWRJjeJaS1jLpLb7ZhM0iZrhzSC638G/5z3+1CWmyxwOvICb0FXzQDCTNSm1t8DO > NicV26tAWMIvDEW5PcTqrJaSvQmNrr4MiBiqocKs4N+ZA7Ey8JQW0oUFzwiwD6Ew > HaOuVXDlha1SZNK2tEnTDsTctXefl+eB7xeQ8MOHPp3yeKrpQlj4gHSyNOboEaXR > 8el/ZC1gGYHPeFGPSgXTbRNFwNY8/9GKPfP5cUgLc+e1B7oWAHg= > =9v1O > -----END PGP SIGNATURE-----
diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt new file mode 100644 index 0000000..7531d85 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt @@ -0,0 +1,24 @@ +Allwinner sun8i R40/V40/T3 SoC PWM controller + +Required properties: + - compatible: Should be one of: + - "allwinner,sun8i-r40-pwm" + - reg: Physical base address and length of the controller's registers + - interrupts: Should contain interrupt. + - clocks: From common clock binding, handle to the parent clock. + - clock-names: Must contain the clock names described just above. + - pwm-channels: PWM channels of the controller. + - #pwm-cells: Should be 3. See pwm.txt in this directory for a description of + the cells format. + +Example: + +pwm: pwm@1c23400 { + compatible = "allwinner,sun8i-r40-pwm"; + reg = <0x01c23400 0x400>; + interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&osc24M>, <&ccu CLK_APB1>; + clock-names = "mux-0", "mux-1"; + pwm-channels = <8>; + #pwm-cells = <3>; +};
This patch adds Allwinner sun8i pwm binding document. Signed-off-by: Hao Zhang <hao5781286@gmail.com> --- .../devicetree/bindings/pwm/pwm-sun8i.txt | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt