diff mbox series

[1/2] pwm: sifive: Add DT documentation for SiFive PWM Controller

Message ID 1547194964-16718-2-git-send-email-yash.shah@sifive.com (mailing list archive)
State New, archived
Headers show
Series PWM support for HiFive Unleashed | expand

Commit Message

Yash Shah Jan. 11, 2019, 8:22 a.m. UTC 
DT documentation for PWM controller added with updated compatible
string.

Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
[Atish: Compatible string update]
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 .../devicetree/bindings/pwm/pwm-sifive.txt         | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt

Comments

Uwe Kleine-König Jan. 15, 2019, 8:11 p.m. UTC | #1 
Hello,

this is v3, right? It is helpful to point this out to ease reviewing.

On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote:
> DT documentation for PWM controller added with updated compatible
> string.

Not sure what was updated here. But assuming this is compared to v2 this
is not a helpful info in the commit log.

> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Compatible string update]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
>  .../devicetree/bindings/pwm/pwm-sifive.txt         | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> new file mode 100644
> index 0000000..e0fc22a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,37 @@
> +SiFive PWM controller
> +
> +Unlike most other PWM controllers, the SiFive PWM controller currently only
> +supports one period for all channels in the PWM. This is set globally in DTS.
> +The period also has significant restrictions on the values it can achieve,
> +which the driver rounds to the nearest achievable frequency.
> +
> +Required properties:
> +- compatible: Please refer to sifive-blocks-ip-versioning.txt

While the description was too verbose in v2, this is too short. You
should at least mention something like "sifive,pwmX" and
"sifive,$cpuname-pwm" (or how ever that scheme works).

> +- reg: physical base address and length of the controller's registers
> +- clocks: Should contain a clock identifier for the PWM's parent clock.
> +- #pwm-cells: Should be 2.
> +  The first cell is the PWM channel number
> +  The second cell is the PWM polarity

I'd drop these two lines and refer to bindings/pwm/pwm.txt instead.

> +- sifive,approx-period-ns: the driver will get as close to this period as it can

As already said for v2: I'd drop "approx-". 

> +- interrupts: one interrupt per PWM channel
> +
> +PWM RTL that corresponds to the IP block version numbers can be found
> +here:
> +
> +https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm
> +
> +Further information on the format of the IP
> +block-specific version numbers can be found in
> +Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt
> +
> +Examples:
> +
> +pwm:  pwm@10020000 {
> +	compatible = "sifive,fu540-c000-pwm","sifive,pwm0";
> +	reg = <0x0 0x10020000 0x0 0x1000>;
> +	clocks = <&tlclk>;
> +	interrupt-parent = <&plic>;
> +	interrupts = <42 43 44 45>;
> +	#pwm-cells = <2>;
> +	sifive,approx-period-ns = <1000000>;
> +};
> -- 
> 1.9.1
> 
>
Yash Shah Jan. 16, 2019, 9:21 a.m. UTC | #2 
On Wed, Jan 16, 2019 at 1:41 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> this is v3, right? It is helpful to point this out to ease reviewing.

Yes, it is v3. Will take care of this in v4.

>
> On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote:
> > DT documentation for PWM controller added with updated compatible
> > string.
>
> Not sure what was updated here. But assuming this is compared to v2 this
> is not a helpful info in the commit log.

Ok, will remove the 'updated compatible string' part.

>
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Compatible string update]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> >  .../devicetree/bindings/pwm/pwm-sifive.txt         | 37 ++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > new file mode 100644
> > index 0000000..e0fc22a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > @@ -0,0 +1,37 @@
> > +SiFive PWM controller
> > +
> > +Unlike most other PWM controllers, the SiFive PWM controller currently only
> > +supports one period for all channels in the PWM. This is set globally in DTS.
> > +The period also has significant restrictions on the values it can achieve,
> > +which the driver rounds to the nearest achievable frequency.
> > +
> > +Required properties:
> > +- compatible: Please refer to sifive-blocks-ip-versioning.txt
>
> While the description was too verbose in v2, this is too short. You
> should at least mention something like "sifive,pwmX" and
> "sifive,$cpuname-pwm" (or how ever that scheme works).

Will mention the above.

>
> > +- reg: physical base address and length of the controller's registers
> > +- clocks: Should contain a clock identifier for the PWM's parent clock.
> > +- #pwm-cells: Should be 2.
> > +  The first cell is the PWM channel number
> > +  The second cell is the PWM polarity
>
> I'd drop these two lines and refer to bindings/pwm/pwm.txt instead.

Will be done.

>
> > +- sifive,approx-period-ns: the driver will get as close to this period as it can
>
> As already said for v2: I'd drop "approx-".

Sure, will be done.

>
> > +- interrupts: one interrupt per PWM channel
> > +
> > +PWM RTL that corresponds to the IP block version numbers can be found
> > +here:
> > +
> > +https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm
> > +
> > +Further information on the format of the IP
> > +block-specific version numbers can be found in
> > +Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt
> > +
> > +Examples:
> > +
> > +pwm:  pwm@10020000 {
> > +     compatible = "sifive,fu540-c000-pwm","sifive,pwm0";
> > +     reg = <0x0 0x10020000 0x0 0x1000>;
> > +     clocks = <&tlclk>;
> > +     interrupt-parent = <&plic>;
> > +     interrupts = <42 43 44 45>;
> > +     #pwm-cells = <2>;
> > +     sifive,approx-period-ns = <1000000>;
> > +};
> > --
> > 1.9.1
> >
> >
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Thanks for the comments.
Thierry Reding Jan. 21, 2019, 11:20 a.m. UTC | #3 
On Wed, Jan 16, 2019 at 02:51:31PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 1:41 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > this is v3, right? It is helpful to point this out to ease reviewing.
> 
> Yes, it is v3. Will take care of this in v4.
> 
> >
> > On Fri, Jan 11, 2019 at 01:52:43PM +0530, Yash Shah wrote:
> > > DT documentation for PWM controller added with updated compatible
> > > string.
> >
> > Not sure what was updated here. But assuming this is compared to v2 this
> > is not a helpful info in the commit log.
> 
> Ok, will remove the 'updated compatible string' part.
> 
> >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Compatible string update]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > >  .../devicetree/bindings/pwm/pwm-sifive.txt         | 37 ++++++++++++++++++++++
> > >  1 file changed, 37 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > > new file mode 100644
> > > index 0000000..e0fc22a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > > @@ -0,0 +1,37 @@
> > > +SiFive PWM controller
> > > +
> > > +Unlike most other PWM controllers, the SiFive PWM controller currently only
> > > +supports one period for all channels in the PWM. This is set globally in DTS.
> > > +The period also has significant restrictions on the values it can achieve,
> > > +which the driver rounds to the nearest achievable frequency.
> > > +
> > > +Required properties:
> > > +- compatible: Please refer to sifive-blocks-ip-versioning.txt
> >
> > While the description was too verbose in v2, this is too short. You
> > should at least mention something like "sifive,pwmX" and
> > "sifive,$cpuname-pwm" (or how ever that scheme works).
> 
> Will mention the above.
> 
> >
> > > +- reg: physical base address and length of the controller's registers
> > > +- clocks: Should contain a clock identifier for the PWM's parent clock.
> > > +- #pwm-cells: Should be 2.
> > > +  The first cell is the PWM channel number
> > > +  The second cell is the PWM polarity
> >
> > I'd drop these two lines and refer to bindings/pwm/pwm.txt instead.
> 
> Will be done.

I don't think you can do that. You're omitting the second cell
representing the period, so this is different from the standard binding
and therefore needs to be explicit.

That said, given the rest of the discussion in the other threads, maybe
it no longer makes sense to set the period on the whole block, but
rather just note in the binding that all PWMs need to run at the same
period. If the driver already refuses to apply incompatible periods,
your users are going to notice that they've got the DT wrong.

This would have the advantage that you can use the standard bindings.

Thierry
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
new file mode 100644
index 0000000..e0fc22a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,37 @@ 
+SiFive PWM controller
+
+Unlike most other PWM controllers, the SiFive PWM controller currently only
+supports one period for all channels in the PWM. This is set globally in DTS.
+The period also has significant restrictions on the values it can achieve,
+which the driver rounds to the nearest achievable frequency.
+
+Required properties:
+- compatible: Please refer to sifive-blocks-ip-versioning.txt
+- reg: physical base address and length of the controller's registers
+- clocks: Should contain a clock identifier for the PWM's parent clock.
+- #pwm-cells: Should be 2.
+  The first cell is the PWM channel number
+  The second cell is the PWM polarity
+- sifive,approx-period-ns: the driver will get as close to this period as it can
+- interrupts: one interrupt per PWM channel
+
+PWM RTL that corresponds to the IP block version numbers can be found
+here:
+
+https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/pwm
+
+Further information on the format of the IP
+block-specific version numbers can be found in
+Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt
+
+Examples:
+
+pwm:  pwm@10020000 {
+	compatible = "sifive,fu540-c000-pwm","sifive,pwm0";
+	reg = <0x0 0x10020000 0x0 0x1000>;
+	clocks = <&tlclk>;
+	interrupt-parent = <&plic>;
+	interrupts = <42 43 44 45>;
+	#pwm-cells = <2>;
+	sifive,approx-period-ns = <1000000>;
+};