[PATCH 1/7] ARM: stih407: Add DT nodes for for PWM

[PATCH 1/7] ARM: stih407: Add DT nodes for for PWM

[Lee Jones]

Supply top level nodes for the STiH407 based development boards.
The Pinctrl configuration has already been applied, so the only
missing piece of the DT puzzle is for a board's DTB to enable
the nodes.

Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/stih407.dtsi | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/stih407.dtsi b/arch/arm/boot/dts/stih407.dtsi
index 4f9024f..6b43ba2 100644
--- a/arch/arm/boot/dts/stih407.dtsi
+++ b/arch/arm/boot/dts/stih407.dtsi
@@ -259,5 +259,32 @@
 
 			status = "disabled";
 		};
+
+		/* COMMS PWM Module */
+		pwm0: pwm@9810000 {
+			compatible      = "st,sti-pwm";
+			status		= "disabled";
+			#pwm-cells	= <2>;
+			reg		= <0x9810000 0x68>;
+			pinctrl-names	= "default";
+                        pinctrl-0	= <&pinctrl_pwm0_chan0_default>;
+			clock-names	= "pwm";
+			clocks          = <&clk_sysin>;
+		};
+
+		/* SBC PWM Module */
+		pwm1: pwm@9510000 {
+			compatible	= "st,sti-pwm";
+			status		= "disabled";
+			#pwm-cells	= <2>;
+			reg		= <0x9510000 0x68>;
+			pinctrl-names	= "default";
+		        pinctrl-0	= <&pinctrl_pwm1_chan0_default
+					&pinctrl_pwm1_chan1_default
+					&pinctrl_pwm1_chan2_default
+					&pinctrl_pwm1_chan3_default>;
+			clock-names	= "pwm";
+			clocks          = <&clk_sysin>;
+		};
 	};
 };
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/7] ARM: stih416: Add Pinctrl settings for PWM

[Lee Jones]

Supply the Pinctrl configuration to enable PWM{0,1} lines on STiH416
based development boards.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416-pinctrl.dtsi | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
index ee6c119..04e61d6 100644
--- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
@@ -207,6 +207,29 @@
 					};
 				};
 			};
+
+			pwm1 {
+				pinctrl_pwm1_chan0_default: pwm1-0-default {
+					st,pins {
+						pwm-out    = <&PIO3 0 ALT1 OUT>;
+					};
+				};
+				pinctrl_pwm1_chan1_default: pwm1-1-default {
+					st,pins {
+						pwm-out    = <&PIO4 4 ALT1 OUT>;
+					};
+				};
+				pinctrl_pwm1_chan2_default: pwm1-2-default {
+					st,pins {
+						pwm-out    = <&PIO4 6 ALT3 OUT>;
+					};
+				};
+				pinctrl_pwm1_chan3_default: pwm1-3-default {
+					st,pins {
+						pwm-out    = <&PIO4 7 ALT3 OUT>;
+					};
+				};
+			};
 		};
 
 		pin-controller-front {
@@ -301,6 +324,14 @@
 				st,bank-name	= "PIO31";
 			};
 
+			pwm0 {
+				pinctrl_pwm0_chan0_default: pwm0-0-default {
+					st,pins {
+						pwm-out    = <&PIO9 7 ALT2 OUT>;
+					};
+				};
+			};
+
 			serial2-oe {
 				pinctrl_serial2_oe: serial2-1 {
 					st,pins {
@@ -467,6 +498,25 @@
 					};
 				};
 			};
+
+			pwm0 {
+				pinctrl_pwm0_chan1_default: pwm0-1-default {
+					st,pins {
+						pwm-out    = <&PIO13 2 ALT2 OUT>;
+					};
+				};
+				pinctrl_pwm0_chan2_default: pwm0-2-default {
+					st,pins {
+						pwm-out    = <&PIO15 2 ALT4 OUT>;
+					};
+				};
+				pinctrl_pwm0_chan3_default: pwm0-3-default {
+					st,pins {
+						pwm-out    = <&PIO17 4 ALT1 OUT>;
+					};
+				};
+			};
+
 		};
 
 		pin-controller-fvdp-fe {
-- 
1.8.3.2

[PATCH 3/7] ARM: stih416: Add DT nodes for PWM

[Lee Jones]

Supply top level nodes for the STiH416 based development boards.
The Pinctrl configuration has already been applied, so the only
missing piece of the DT puzzle is for a board's DTB to enable
the nodes.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416.dtsi | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 06473c5..8af159e 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -236,5 +236,45 @@
 			resets	= <&powerdown STIH416_KEYSCAN_POWERDOWN>,
 				  <&softreset STIH416_KEYSCAN_SOFTRESET>;
 		};
+
+		/* SAS PWM Module */
+		pwm0: pwm@fed10000 {
+			compatible      = "st,sti-pwm";
+			status		= "disabled";
+			#pwm-cells	= <2>;
+			reg		= <0xfed10000 0x68>;
+
+			pinctrl-names	= "default";
+                        pinctrl-0 = 	<&pinctrl_pwm0_chan0_default
+					&pinctrl_pwm0_chan1_default
+					&pinctrl_pwm0_chan2_default
+					&pinctrl_pwm0_chan3_default>;
+
+			clock-names	= "pwm";
+			clocks          = <&clk_sysin>;
+		};
+
+		/* SBC PWM Module */
+		pwm1: pwm@fe510000 {
+			compatible	= "st,sti-pwm";
+			status		= "disabled";
+			#pwm-cells	= <2>;
+			reg		= <0xfe510000 0x68>;
+
+			pinctrl-names	= "default";
+		        pinctrl-0 =	<&pinctrl_pwm1_chan0_default
+					/*
+					 * Shared with SBC_OBS_NOTRST.  Don't
+					 * enable unless you really know what
+					 * you're doing.
+					 *
+					 * &pinctrl_pwm1_chan1_default
+					 */
+					&pinctrl_pwm1_chan2_default
+					&pinctrl_pwm1_chan3_default>;
+
+			clock-names	= "pwm";
+			clocks          = <&clk_sysin>;
+		};
 	};
 };
-- 
1.8.3.2

Re: [PATCH 3/7] ARM: stih416: Add DT nodes for PWM

[Gabriel Fernandez]

Hi Lee,
minor remark below..

On 18 June 2014 16:52, Lee Jones <lee.jones@linaro.org> wrote:
> Supply top level nodes for the STiH416 based development boards.
> The Pinctrl configuration has already been applied, so the only
> missing piece of the DT puzzle is for a board's DTB to enable
> the nodes.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/boot/dts/stih416.dtsi | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
> index 06473c5..8af159e 100644
> --- a/arch/arm/boot/dts/stih416.dtsi
> +++ b/arch/arm/boot/dts/stih416.dtsi
> @@ -236,5 +236,45 @@
>                         resets  = <&powerdown STIH416_KEYSCAN_POWERDOWN>,
>                                   <&softreset STIH416_KEYSCAN_SOFTRESET>;
>                 };
> +
> +               /* SAS PWM Module */
> +               pwm0: pwm@fed10000 {
> +                       compatible      = "st,sti-pwm";
> +                       status          = "disabled";
> +                       #pwm-cells      = <2>;
> +                       reg             = <0xfed10000 0x68>;
> +
> +                       pinctrl-names   = "default";
> +                        pinctrl-0 =    <&pinctrl_pwm0_chan0_default
del space

> +                                       &pinctrl_pwm0_chan1_default
> +                                       &pinctrl_pwm0_chan2_default
> +                                       &pinctrl_pwm0_chan3_default>;
> +
> +                       clock-names     = "pwm";
> +                       clocks          = <&clk_sysin>;
> +               };
> +
> +               /* SBC PWM Module */
> +               pwm1: pwm@fe510000 {
> +                       compatible      = "st,sti-pwm";
> +                       status          = "disabled";
> +                       #pwm-cells      = <2>;
> +                       reg             = <0xfe510000 0x68>;
> +
> +                       pinctrl-names   = "default";
> +                       pinctrl-0 =     <&pinctrl_pwm1_chan0_default
> +                                       /*
> +                                        * Shared with SBC_OBS_NOTRST.  Don't
> +                                        * enable unless you really know what
> +                                        * you're doing.
> +                                        *
> +                                        * &pinctrl_pwm1_chan1_default
> +                                        */
> +                                       &pinctrl_pwm1_chan2_default
> +                                       &pinctrl_pwm1_chan3_default>;
> +
> +                       clock-names     = "pwm";
> +                       clocks          = <&clk_sysin>;
> +               };
>         };
>  };
> --
> 1.8.3.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ARM: stih416: Add DT nodes for PWM

[Lee Jones]

On Wed, 18 Jun 2014, Gabriel Fernandez wrote:

> Hi Lee,
> minor remark below..
> 
> On 18 June 2014 16:52, Lee Jones <lee.jones@linaro.org> wrote:
> > Supply top level nodes for the STiH416 based development boards.
> > The Pinctrl configuration has already been applied, so the only
> > missing piece of the DT puzzle is for a board's DTB to enable
> > the nodes.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/boot/dts/stih416.dtsi | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
> > index 06473c5..8af159e 100644
> > --- a/arch/arm/boot/dts/stih416.dtsi
> > +++ b/arch/arm/boot/dts/stih416.dtsi
> > @@ -236,5 +236,45 @@
> >                         resets  = <&powerdown STIH416_KEYSCAN_POWERDOWN>,
> >                                   <&softreset STIH416_KEYSCAN_SOFTRESET>;
> >                 };
> > +
> > +               /* SAS PWM Module */
> > +               pwm0: pwm@fed10000 {
> > +                       compatible      = "st,sti-pwm";
> > +                       status          = "disabled";
> > +                       #pwm-cells      = <2>;
> > +                       reg             = <0xfed10000 0x68>;
> > +
> > +                       pinctrl-names   = "default";
> > +                        pinctrl-0 =    <&pinctrl_pwm0_chan0_default
> del space

Thanks.

> > +                                       &pinctrl_pwm0_chan1_default
> > +                                       &pinctrl_pwm0_chan2_default
> > +                                       &pinctrl_pwm0_chan3_default>;
> > +
> > +                       clock-names     = "pwm";
> > +                       clocks          = <&clk_sysin>;
> > +               };
> > +
> > +               /* SBC PWM Module */
> > +               pwm1: pwm@fe510000 {
> > +                       compatible      = "st,sti-pwm";
> > +                       status          = "disabled";
> > +                       #pwm-cells      = <2>;
> > +                       reg             = <0xfe510000 0x68>;
> > +
> > +                       pinctrl-names   = "default";
> > +                       pinctrl-0 =     <&pinctrl_pwm1_chan0_default
> > +                                       /*
> > +                                        * Shared with SBC_OBS_NOTRST.  Don't
> > +                                        * enable unless you really know what
> > +                                        * you're doing.
> > +                                        *
> > +                                        * &pinctrl_pwm1_chan1_default
> > +                                        */
> > +                                       &pinctrl_pwm1_chan2_default
> > +                                       &pinctrl_pwm1_chan3_default>;
> > +
> > +                       clock-names     = "pwm";
> > +                       clocks          = <&clk_sysin>;
> > +               };
> >         };
> >  };
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 3/7] ARM: stih416: Add DT nodes for PWM

[Lee Jones]

ARM: stih416: Add DT nodes for PWM

Supply top level nodes for the STiH416 based development boards.
The Pinctrl configuration has already been applied, so the only
missing piece of the DT puzzle is for a board's DTB to enable
the nodes.

Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 06473c5..f95e469 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -102,7 +102,7 @@
 			interrupts	= <0 210 0>;
 			pinctrl-names 	= "default";
 			pinctrl-0 	= <&pinctrl_sbc_serial1>;
-			clocks          = <&clk_sysin>;
+			clocks		= <&clk_sysin>;
 		};
 
 		i2c@fed40000 {
@@ -236,5 +236,45 @@
 			resets	= <&powerdown STIH416_KEYSCAN_POWERDOWN>,
 				  <&softreset STIH416_KEYSCAN_SOFTRESET>;
 		};
+
+		/* SAS PWM Module */
+		pwm0: pwm@fed10000 {
+			compatible	= "st,sti-pwm";
+			status		= "disabled";
+			#pwm-cells	= <2>;
+			reg		= <0xfed10000 0x68>;
+
+			pinctrl-names	= "default";
+			pinctrl-0 = 	<&pinctrl_pwm0_chan0_default
+					&pinctrl_pwm0_chan1_default
+					&pinctrl_pwm0_chan2_default
+					&pinctrl_pwm0_chan3_default>;
+
+			clock-names	= "pwm";
+			clocks		= <&clk_sysin>;
+		};
+
+		/* SBC PWM Module */
+		pwm1: pwm@fe510000 {
+			compatible	= "st,sti-pwm";
+			status		= "disabled";
+			#pwm-cells	= <2>;
+			reg		= <0xfe510000 0x68>;
+
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_pwm1_chan0_default
+					/*
+					 * Shared with SBC_OBS_NOTRST.  Don't
+					 * enable unless you really know what
+					 * you're doing.
+					 *
+					 * &pinctrl_pwm1_chan1_default
+					 */
+					&pinctrl_pwm1_chan2_default
+					&pinctrl_pwm1_chan3_default>;
+
+			clock-names	= "pwm";
+			clocks		= <&clk_sysin>;
+		};
 	};
 };

[PATCH 4/7] ARM: stih416-b2020e: Enable PWM on the B2020 Rev-E

[Lee Jones]

All the infrastructure is now in place for ST's PWM controller.  This
patch takes the final step and enables the IP on the 2020 Rev-E
development platform.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416-b2020e.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/stih416-b2020e.dts b/arch/arm/boot/dts/stih416-b2020e.dts
index ba0fa2c..07cbf5b 100644
--- a/arch/arm/boot/dts/stih416-b2020e.dts
+++ b/arch/arm/boot/dts/stih416-b2020e.dts
@@ -31,5 +31,15 @@
 		ethernet1: dwmac@fef08000 {
 			snps,reset-gpio = <&PIO0 7>;
 		};
+
+		/* SAS PWM Module */
+		pwm0: pwm@fed10000 {
+			status		= "okay";
+		};
+
+		/* SBC PWM Module */
+		pwm1: pwm@fe510000 {
+			status		= "okay";
+		};
 	};
 };
-- 
1.8.3.2

Re: [STLinux Kernel] [PATCH 4/7] ARM: stih416-b2020e: Enable PWM on the B2020 Rev-E

[Peter Griffin]

Hi Lee,

> All the infrastructure is now in place for ST's PWM controller.  This
> patch takes the final step and enables the IP on the 2020 Rev-E
> development platform.

Why are you only enabling this for rev-E board?

If you make the stih415 DT PWM nodes part of this series as well, then 
assuming there is no good reason to the contary, the PWM could 
be enabled for both stih415/6 in the common stih41x-b2020 DTS file which
is also included by rev-E (assuming that PWM wiring is not some rev-E specific
thing).

Also Ajit Signed-off-by needs adding to this series.

After that:
Acked-by: Peter Griffin <peter.griffin@linaro.org>

regards,

Peter.

Re: [STLinux Kernel] [PATCH 4/7] ARM: stih416-b2020e: Enable PWM on the B2020 Rev-E

[Lee Jones]

> > All the infrastructure is now in place for ST's PWM controller.  This
> > patch takes the final step and enables the IP on the 2020 Rev-E
> > development platform.
> 
> Why are you only enabling this for rev-E board?

Purely for testing purposes, as it's the only board I have.  If
anyone has the other revisions and would be happy to test the driver
for me then great.

> If you make the stih415 DT PWM nodes part of this series as well, then 
> assuming there is no good reason to the contary, the PWM could 
> be enabled for both stih415/6 in the common stih41x-b2020 DTS file which
> is also included by rev-E (assuming that PWM wiring is not some rev-E specific
> thing).
> 
> Also Ajit Signed-off-by needs adding to this series.

As you know, Thierry already picked this up.

Will fix for v2.

> After that:
> Acked-by: Peter Griffin <peter.griffin@linaro.org>

Thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 4/7] ARM: stih416-b2020e: Enable PWM on the B2020 Rev-E

[Peter Griffin]

Hi Lee,

> > Why are you only enabling this for rev-E board?
> 
> Purely for testing purposes, as it's the only board I have.  If
> anyone has the other revisions and would be happy to test the driver
> for me then great.

Ok that's what I suspected.

If you tell me how, I can test on a Rev C board. But as there are several other 
revisions I think checking the board schematics would be the quickest way forward.

> > If you make the stih415 DT PWM nodes part of this series as well, then 
> > assuming there is no good reason to the contary, the PWM could 
> > be enabled for both stih415/6 in the common stih41x-b2020 DTS file which
> > is also included by rev-E (assuming that PWM wiring is not some rev-E specific
> > thing).

Can you add the stih415 bindings in V2 as well?

regards,

Peter

Re: [STLinux Kernel] [PATCH 4/7] ARM: stih416-b2020e: Enable PWM on the B2020 Rev-E

[Maxime Coquelin]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

Hi Peter,

On 06/19/2014 12:12 PM, Peter Griffin wrote:
> Hi Lee,
> 
>>> Why are you only enabling this for rev-E board?
>>
>> Purely for testing purposes, as it's the only board I have.  If
>> anyone has the other revisions and would be happy to test the driver
>> for me then great.
> 
> Ok that's what I suspected.
> 
> If you tell me how, I can test on a Rev C board. But as there are several other 
> revisions I think checking the board schematics would be the quickest way forward.

Only Rev C & E are in the wild now, so checking these two boards will
be enough.

Thanks,
Maxime

> 
>>> If you make the stih415 DT PWM nodes part of this series as well, then 
>>> assuming there is no good reason to the contary, the PWM could 
>>> be enabled for both stih415/6 in the common stih41x-b2020 DTS file which
>>> is also included by rev-E (assuming that PWM wiring is not some rev-E specific
>>> thing).
> 
> Can you add the stih415 bindings in V2 as well?
> 
> regards,
> 
> Peter
> 
> _______________________________________________
> Kernel mailing list
> Kernel@stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [STLinux Kernel] [PATCH 4/7] ARM: stih416-b2020e: Enable PWM on the B2020 Rev-E

[Lee Jones]

> > > Why are you only enabling this for rev-E board?
> > 
> > Purely for testing purposes, as it's the only board I have.  If
> > anyone has the other revisions and would be happy to test the driver
> > for me then great.
> 
> Ok that's what I suspected.
> 
> If you tell me how, I can test on a Rev C board. But as there are several other 
> revisions I think checking the board schematics would be the quickest way forward.

Great.  Do you have a oscilloscope/logic analyser?

> > > If you make the stih415 DT PWM nodes part of this series as well, then 
> > > assuming there is no good reason to the contary, the PWM could 
> > > be enabled for both stih415/6 in the common stih41x-b2020 DTS file which
> > > is also included by rev-E (assuming that PWM wiring is not some rev-E specific
> > > thing).
> 
> Can you add the stih415 bindings in V2 as well?

Who's using Orly1?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 4/7] ARM: stih416-b2020e: Enable PWM on the B2020 Rev-E

[Peter Griffin]

Hi Lee,

> > > Purely for testing purposes, as it's the only board I have.  If
> > > anyone has the other revisions and would be happy to test the driver
> > > for me then great.
> > 
> > Ok that's what I suspected.
> > 
> > If you tell me how, I can test on a Rev C board. But as there are several other 
> > revisions I think checking the board schematics would be the quickest way forward.
> 
> Great.  Do you have a oscilloscope/logic analyser?

I have an old scope, and I've been wanting to buy a decent logic analyser ;-)
Also can you email me the rev E schematic? I currently only have rev C.

> > > > If you make the stih415 DT PWM nodes part of this series as well, then 
> > > > assuming there is no good reason to the contary, the PWM could 
> > > > be enabled for both stih415/6 in the common stih41x-b2020 DTS file which
> > > > is also included by rev-E (assuming that PWM wiring is not some rev-E specific
> > > > thing).
> > 
> > Can you add the stih415 bindings in V2 as well?
> 
> Who's using Orly1?

I'll let ST answer that.

AFAIK drivers upstreamed so far for Orly based hardware have generally been enabled for both
SoCs, hence querying why the stih415 bindings are missing from this series.

Being pinout compatible, a b2020 board can be populated with a stih415, so it seems strange 
for it to then be missing PWM support given that the SoC, board and pwm driver all support it.

regards,

Peter.

[PATCH 5/7] ARM: multi_v7_defconfig: Enable ST's PWM driver

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/configs/multi_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index e2d6204..287b569 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -364,6 +364,7 @@ CONFIG_AK8975=y
 CONFIG_PWM=y
 CONFIG_PWM_TEGRA=y
 CONFIG_PWM_VT8500=y
+CONFIG_PWM_ST=y
 CONFIG_OMAP_USB2=y
 CONFIG_PHY_SUN4I_USB=y
 CONFIG_EXT4_FS=y
-- 
1.8.3.2

[PATCH 6/7] pwm: st: Add new driver for ST's PWM IP

[Lee Jones]

This driver supports all current STi platforms' PWM IPs.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/Kconfig  |   9 ++
 drivers/pwm/Makefile |   1 +
 drivers/pwm/pwm-st.c | 378 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 388 insertions(+)
 create mode 100644 drivers/pwm/pwm-st.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4ad7b89..98a7bbc 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -292,4 +292,13 @@ config PWM_VT8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-vt8500.
 
+config PWM_ST
+	tristate "STiH4xx PWM support"
+	depends on ARCH_STI
+	help
+	  Generic PWM framework driver for STiH4xx SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-st.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 5c86a19..2c846a6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
+obj-$(CONFIG_PWM_ST)		+= pwm-st.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
diff --git a/drivers/pwm/pwm-st.c b/drivers/pwm/pwm-st.c
new file mode 100644
index 0000000..4dea616
--- /dev/null
+++ b/drivers/pwm/pwm-st.c
@@ -0,0 +1,378 @@
+/*
+ * PWM device driver for ST SoCs.
+ * Author: Ajit Pal Singh <ajitpal.singh@st.com>
+ *
+ * Copyright (C) 2003-2014 STMicroelectronics (R&D) Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+#include <linux/bsearch.h>
+#include <linux/clk.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/mfd/syscon.h>
+#include <linux/time.h>
+
+#define ST_PWMVAL(x)	(4 * (x))	/* Value used to define duty cycle */
+#define ST_PWMCR	0x50		/* Control/Config reg */
+#define ST_INTEN	0x54		/* Interrupt Enable/Disable reg */
+#define ST_CNT		0x60		/* PWMCounter */
+
+#define MAX_PWM_CNT_DEFAULT	255
+#define MAX_PRESCALE_DEFAULT	0xff
+#define NUM_CHAN_DEFAULT	1
+
+/* Regfield IDs */
+enum {
+	PWMCLK_PRESCALE = 0,
+	PWM_EN,
+	PWM_INT_EN,
+	/* keep last */
+	MAX_REGFIELDS
+};
+
+struct st_pwm_chip {
+	struct device *dev;
+	struct clk *clk;
+	unsigned long clk_rate;
+	struct regmap *regmap;
+	struct st_pwm_compat_data *cdata;
+	struct regmap_field *prescale;
+	struct regmap_field *pwm_en;
+	struct regmap_field *pwm_int_en;
+	unsigned long *pwm_periods;
+	struct pwm_chip chip;
+	void __iomem *mmio_base;
+};
+
+struct st_pwm_compat_data {
+	const struct reg_field *reg_fields;
+	int num_chan;
+	int max_pwm_cnt;
+	int max_prescale;
+};
+
+static const struct reg_field st_pwm_regfields[MAX_REGFIELDS] = {
+	[PWMCLK_PRESCALE]	= REG_FIELD(ST_PWMCR, 0, 3),
+	[PWM_EN]		= REG_FIELD(ST_PWMCR, 9, 9),
+	[PWM_INT_EN]		= REG_FIELD(ST_INTEN, 0, 0),
+};
+
+static inline struct st_pwm_chip *to_st_pwmchip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct st_pwm_chip, chip);
+}
+
+/*
+ * Calculate the period values supported by the PWM for the
+ * current clock rate.
+ */
+static void st_pwm_calc_periods(struct st_pwm_chip *pc)
+{
+	struct st_pwm_compat_data *cdata = pc->cdata;
+	struct device *dev = pc->dev;
+	unsigned long val;
+	int i;
+
+	/*
+	 * period_ns = (10^9 * (prescaler + 1) * (MAX_PWM_COUNT + 1)) / CLK_RATE
+	 */
+	val = NSEC_PER_SEC / pc->clk_rate;
+	val *= cdata->max_pwm_cnt + 1;
+
+	dev_dbg(dev, "possible periods for clkrate[HZ]:%lu\n", pc->clk_rate);
+
+	for (i = 0; i <= cdata->max_prescale; i++) {
+		pc->pwm_periods[i] = val * (i + 1);
+		dev_dbg(dev, "prescale:%d, period[ns]:%lu\n",
+			i, pc->pwm_periods[i]);
+	}
+}
+
+static int st_pwm_cmp_periods(const void *key, const void *elt)
+{
+	unsigned long i = *(unsigned long *)key;
+	unsigned long j = *(unsigned long *)elt;
+
+	if (i < j)
+		return -1;
+	else
+		return i == j ? 0 : 1;
+}
+
+/*
+ * For STiH4xx PWM IP, the pwm period is fixed to 256 local clock cycles.
+ * The only way to change the period (apart from changing the pwm input clock)
+ * is to change the pwm clock prescaler.
+ * The prescaler is of 4 bits, so only 16 prescaler values and hence only
+ * 16 possible period values are supported (for a particular clock rate).
+ * The requested period will be applied only if it matches one of these
+ * 16 values.
+ */
+static int st_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			 int duty_ns, int period_ns)
+{
+	struct st_pwm_chip *pc = to_st_pwmchip(chip);
+	struct device *dev = pc->dev;
+	struct st_pwm_compat_data *cdata = pc->cdata;
+	unsigned int prescale, pwmvalx;
+	unsigned long *found;
+	int ret;
+
+	/*
+	 * Search for matching period value. The corresponding index is our
+	 * prescale value
+	 */
+	found = bsearch(&period_ns, &pc->pwm_periods[0],
+			cdata->max_prescale + 1, sizeof(unsigned long),
+			st_pwm_cmp_periods);
+	if (!found) {
+		dev_err(dev, "failed to find matching period\n");
+		return -EINVAL;
+	}
+
+	prescale = found - &pc->pwm_periods[0];
+
+	/*
+	 * When PWMVal == 0, PWM pulse = 1 local clock cycle.
+	 * When PWMVal == max_pwm_count,
+	 * PWM pulse = (max_pwm_count + 1) local cycles,
+	 * that is continuous pulse: signal never goes low.
+	 */
+	pwmvalx = cdata->max_pwm_cnt * duty_ns / period_ns;
+
+	dev_dbg(dev, "prescale:%u, period:%i, duty:%i, pwmvalx:%u\n",
+		prescale, period_ns, duty_ns, pwmvalx);
+
+	/* Enable clock before writing to PWM registers */
+	ret = clk_enable(pc->clk);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(pc->prescale, prescale);
+	if (ret)
+		goto clk_dis;
+
+	ret = regmap_write(pc->regmap, ST_PWMVAL(pwm->hwpwm), pwmvalx);
+	if (ret)
+		goto clk_dis;
+
+	ret = regmap_field_write(pc->pwm_int_en, 0);
+
+clk_dis:
+	clk_disable(pc->clk);
+	return ret;
+}
+
+static int st_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct st_pwm_chip *pc = to_st_pwmchip(chip);
+	struct device *dev = pc->dev;
+	int ret;
+
+	ret = clk_enable(pc->clk);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(pc->pwm_en, 1);
+	if (ret)
+		dev_err(dev, "%s,pwm_en write failed\n", __func__);
+
+	return ret;
+}
+
+static void st_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct st_pwm_chip *pc = to_st_pwmchip(chip);
+	struct device *dev = pc->dev;
+	unsigned int val;
+
+	regmap_field_write(pc->pwm_en, 0);
+
+	regmap_read(pc->regmap, ST_CNT, &val);
+
+	dev_dbg(dev, "pwm counter :%u\n", val);
+
+	clk_disable(pc->clk);
+}
+
+static const struct pwm_ops st_pwm_ops = {
+	.config = st_pwm_config,
+	.enable = st_pwm_enable,
+	.disable = st_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int st_pwm_probe_dt(struct st_pwm_chip *pc)
+{
+	struct device *dev = pc->dev;
+	const struct reg_field *reg_fields;
+	struct device_node *np = dev->of_node;
+	struct st_pwm_compat_data *cdata = pc->cdata;
+	u32 num_chan;
+
+	of_property_read_u32(np, "st,pwm-num-chan", &num_chan);
+	if (num_chan)
+		cdata->num_chan = num_chan;
+
+	reg_fields = cdata->reg_fields;
+
+	pc->prescale = devm_regmap_field_alloc(dev, pc->regmap,
+					       reg_fields[PWMCLK_PRESCALE]);
+	pc->pwm_en = devm_regmap_field_alloc(dev, pc->regmap,
+					     reg_fields[PWM_EN]);
+	pc->pwm_int_en = devm_regmap_field_alloc(dev, pc->regmap,
+						 reg_fields[PWM_INT_EN]);
+
+	if (IS_ERR(pc->prescale) ||
+	    IS_ERR(pc->pwm_en)   ||
+	    IS_ERR(pc->pwm_int_en)) {
+		dev_err(dev, "unable to allocate reg_field(s)\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static struct regmap_config st_pwm_regmap_config = {
+	.reg_bits   = 32,
+	.val_bits   = 32,
+	.reg_stride = 4,
+};
+
+static int st_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct st_pwm_compat_data *cdata;
+	struct st_pwm_chip *pc;
+	struct resource *res;
+	int ret;
+
+	if (!np) {
+		dev_err(dev, "failed to find device node\n");
+		return -EINVAL;
+	}
+
+	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
+	if (!pc)
+		return -ENOMEM;
+
+	cdata = devm_kzalloc(dev, sizeof(*cdata), GFP_KERNEL);
+	if (!cdata)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	pc->mmio_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pc->mmio_base)) {
+		dev_err(dev, "failed to find and map memory resources\n");
+		return PTR_ERR(pc->mmio_base);
+	}
+
+	pc->regmap = devm_regmap_init_mmio(dev, pc->mmio_base,
+					   &st_pwm_regmap_config);
+	if (IS_ERR(pc->regmap))
+		return PTR_ERR(pc->regmap);
+
+	/*
+	 * Setup PWM data with default values: some values could be replaced
+	 * with specific ones provided from device tree
+	 */
+	cdata->reg_fields   = &st_pwm_regfields[0];
+	cdata->max_pwm_cnt  = MAX_PWM_CNT_DEFAULT;
+	cdata->max_prescale = MAX_PRESCALE_DEFAULT;
+	cdata->num_chan     = NUM_CHAN_DEFAULT;
+
+	pc->cdata = cdata;
+	pc->dev = dev;
+
+	ret = st_pwm_probe_dt(pc);
+	if (ret)
+		return ret;
+
+	pc->pwm_periods = devm_kzalloc(dev,
+			sizeof(unsigned long) * (pc->cdata->max_prescale + 1),
+			GFP_KERNEL);
+	if (!pc->pwm_periods)
+		return -ENOMEM;
+
+	pc->clk = of_clk_get_by_name(np, "pwm");
+	if (IS_ERR(pc->clk)) {
+		dev_err(dev, "failed to get pwm clock\n");
+		return PTR_ERR(pc->clk);
+	}
+
+	pc->clk_rate = clk_get_rate(pc->clk);
+	if (!pc->clk_rate) {
+		dev_err(dev, "failed to get clk rate\n");
+		return -EINVAL;
+	}
+
+	ret = clk_prepare(pc->clk);
+	if (ret) {
+		dev_err(dev, "failed to prepare clk\n");
+		return ret;
+	}
+
+	st_pwm_calc_periods(pc);
+
+	pc->chip.dev = dev;
+	pc->chip.ops = &st_pwm_ops;
+	pc->chip.base = -1;
+	pc->chip.npwm = pc->cdata->num_chan;
+	pc->chip.can_sleep = true;
+
+	ret = pwmchip_add(&pc->chip);
+	if (ret < 0) {
+		clk_unprepare(pc->clk);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pc);
+
+	return 0;
+}
+
+static int st_pwm_remove(struct platform_device *pdev)
+{
+	struct st_pwm_chip *pc = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < pc->cdata->num_chan; i++)
+		pwm_disable(&pc->chip.pwms[i]);
+
+	clk_unprepare(pc->clk);
+
+	return pwmchip_remove(&pc->chip);
+}
+
+static struct of_device_id st_pwm_of_match[] = {
+	{ .compatible = "st,sti-pwm", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, st_pwm_of_match);
+
+static struct platform_driver st_pwm_driver = {
+	.driver = {
+		.name	= "st-pwm",
+		.owner  = THIS_MODULE,
+		.of_match_table = st_pwm_of_match,
+	},
+	.probe		= st_pwm_probe,
+	.remove		= st_pwm_remove,
+};
+module_platform_driver(st_pwm_driver);
+
+MODULE_AUTHOR("STMicroelectronics (R&D) Limited <ajitpal.singh@st.com>");
+MODULE_DESCRIPTION("STMicroelectronics ST PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.3.2

Re: [PATCH 6/7] pwm: st: Add new driver for ST's PWM IP

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 15562 bytes --]

On Wed, Jun 18, 2014 at 03:52:51PM +0100, Lee Jones wrote:
> This driver supports all current STi platforms' PWM IPs.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/pwm/Kconfig  |   9 ++
>  drivers/pwm/Makefile |   1 +
>  drivers/pwm/pwm-st.c | 378 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 388 insertions(+)
>  create mode 100644 drivers/pwm/pwm-st.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4ad7b89..98a7bbc 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -292,4 +292,13 @@ config PWM_VT8500
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-vt8500.
>  
> +config PWM_ST

PWM_ST is awfully generic, perhaps PWM_STI would be a better choice?
Even that's very generic. Maybe PWM_STI_H4XX? There's nothing wrong with
supporting STiH{5,6,7,...}xx SoCs with such a driver. I'm just trying to
think ahead what will happen if at some point a new SoC family is
released that requires a different driver.

> diff --git a/drivers/pwm/pwm-st.c b/drivers/pwm/pwm-st.c
[...]
> +/*
> + * PWM device driver for ST SoCs.
> + * Author: Ajit Pal Singh <ajitpal.singh@st.com>

Given this line, shouldn't the commit contain Ajit Pal Singh's
Signed-off-by?

> + *
> + * Copyright (C) 2003-2014 STMicroelectronics (R&D) Limited

Was this driver really developed over a period of 11 years?

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *

Nit: no need for this extra blank line at the end of the comment.

> + */
> +#include <linux/bsearch.h>

I prefer a blank line between the above two

> +#include <linux/clk.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/time.h>

These should be sorted alphabetically.

> +
> +#define ST_PWMVAL(x)	(4 * (x))	/* Value used to define duty cycle */

"x" seems to designate the channel number, so perhaps make that clearer
by naming the variable "ch"? Also in that case the comment is somewhat
misleading.

> +#define ST_PWMCR	0x50		/* Control/Config reg */
> +#define ST_INTEN	0x54		/* Interrupt Enable/Disable reg */
> +#define ST_CNT		0x60		/* PWMCounter */

I'd prefer s/reg/register/ and also use it consistently for the ST_CNT
as well.

> +
> +#define MAX_PWM_CNT_DEFAULT	255
> +#define MAX_PRESCALE_DEFAULT	0xff
> +#define NUM_CHAN_DEFAULT	1

These are only used in one place and their meaning is fairly obvious, so
I'd just drop them.

> +/* Regfield IDs */
> +enum {
> +	PWMCLK_PRESCALE = 0,

No need for "= 0".

> +	PWM_EN,
> +	PWM_INT_EN,
> +	/* keep last */
> +	MAX_REGFIELDS
> +};
> +
> +struct st_pwm_chip {
> +	struct device *dev;
> +	struct clk *clk;
> +	unsigned long clk_rate;
> +	struct regmap *regmap;
> +	struct st_pwm_compat_data *cdata;

Doesn't this require a predeclaration of struct st_pwm_compat_data? Or
maybe just move struct st_pwm_compat_data before this.

> +	struct regmap_field *prescale;
> +	struct regmap_field *pwm_en;
> +	struct regmap_field *pwm_int_en;
> +	unsigned long *pwm_periods;
> +	struct pwm_chip chip;
> +	void __iomem *mmio_base;

mmio_base is somewhat long, maybe "mmio" or "base" would work equally
well?

> +};
> +
> +struct st_pwm_compat_data {
> +	const struct reg_field *reg_fields;
> +	int num_chan;
> +	int max_pwm_cnt;
> +	int max_prescale;

Can't these three be unsigned?

> +};
> +
> +static const struct reg_field st_pwm_regfields[MAX_REGFIELDS] = {
> +	[PWMCLK_PRESCALE]	= REG_FIELD(ST_PWMCR, 0, 3),
> +	[PWM_EN]		= REG_FIELD(ST_PWMCR, 9, 9),
> +	[PWM_INT_EN]		= REG_FIELD(ST_INTEN, 0, 0),
> +};
> +
> +static inline struct st_pwm_chip *to_st_pwmchip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct st_pwm_chip, chip);
> +}
> +
> +/*
> + * Calculate the period values supported by the PWM for the
> + * current clock rate.
> + */
> +static void st_pwm_calc_periods(struct st_pwm_chip *pc)
> +{
> +	struct st_pwm_compat_data *cdata = pc->cdata;
> +	struct device *dev = pc->dev;
> +	unsigned long val;
> +	int i;

unsigned?

> +
> +	/*
> +	 * period_ns = (10^9 * (prescaler + 1) * (MAX_PWM_COUNT + 1)) / CLK_RATE
> +	 */
> +	val = NSEC_PER_SEC / pc->clk_rate;
> +	val *= cdata->max_pwm_cnt + 1;
> +
> +	dev_dbg(dev, "possible periods for clkrate[HZ]:%lu\n", pc->clk_rate);
> +
> +	for (i = 0; i <= cdata->max_prescale; i++) {
> +		pc->pwm_periods[i] = val * (i + 1);
> +		dev_dbg(dev, "prescale:%d, period[ns]:%lu\n",
> +			i, pc->pwm_periods[i]);
> +	}
> +}
> +
> +static int st_pwm_cmp_periods(const void *key, const void *elt)
> +{
> +	unsigned long i = *(unsigned long *)key;
> +	unsigned long j = *(unsigned long *)elt;
> +
> +	if (i < j)
> +		return -1;
> +	else
> +		return i == j ? 0 : 1;
> +}
> +
> +/*
> + * For STiH4xx PWM IP, the pwm period is fixed to 256 local clock cycles.

s/pwm/PWM/ in prose. There are probably other occurrences of this
throughout the driver.

> + * The only way to change the period (apart from changing the pwm input clock)
> + * is to change the pwm clock prescaler.
> + * The prescaler is of 4 bits, so only 16 prescaler values and hence only

I'm confused. This says there are 16 prescaler values, but at the same
time the default maximum number of prescaler values is set to 255. Am I
missing something?

> + * 16 possible period values are supported (for a particular clock rate).
> + * The requested period will be applied only if it matches one of these
> + * 16 values.
> + */
> +static int st_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 int duty_ns, int period_ns)
> +{
> +	struct st_pwm_chip *pc = to_st_pwmchip(chip);
> +	struct device *dev = pc->dev;
> +	struct st_pwm_compat_data *cdata = pc->cdata;
> +	unsigned int prescale, pwmvalx;
> +	unsigned long *found;
> +	int ret;
> +
> +	/*
> +	 * Search for matching period value. The corresponding index is our
> +	 * prescale value
> +	 */
> +	found = bsearch(&period_ns, &pc->pwm_periods[0],

Technically doesn't period_ns need to be converted to an unsigned long
here? Otherwise this won't be compatible with 64-bit architectures.
Admittedly that's maybe not something relevant for this family of SoCs,
but you never know where this driver will be used eventually.

> +			cdata->max_prescale + 1, sizeof(unsigned long),
> +			st_pwm_cmp_periods);
> +	if (!found) {
> +		dev_err(dev, "failed to find matching period\n");
> +		return -EINVAL;
> +	}
> +
> +	prescale = found - &pc->pwm_periods[0];

This is somewhat unconventional. None of the other drivers precompute
possible periods and I'm not convinced that it's an advantage. Setting
the period (and configuring the PWM in general) is a fairly uncommon
operation.

> +	/*
> +	 * When PWMVal == 0, PWM pulse = 1 local clock cycle.
> +	 * When PWMVal == max_pwm_count,
> +	 * PWM pulse = (max_pwm_count + 1) local cycles,
> +	 * that is continuous pulse: signal never goes low.
> +	 */
> +	pwmvalx = cdata->max_pwm_cnt * duty_ns / period_ns;
> +
> +	dev_dbg(dev, "prescale:%u, period:%i, duty:%i, pwmvalx:%u\n",
> +		prescale, period_ns, duty_ns, pwmvalx);
> +
> +	/* Enable clock before writing to PWM registers */
> +	ret = clk_enable(pc->clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_write(pc->prescale, prescale);
> +	if (ret)
> +		goto clk_dis;

So the prescaler is shared between all channels? In that case I think
you should check for conflicting settings to prevent one channel from
stomping on the other.

> +
> +	ret = regmap_write(pc->regmap, ST_PWMVAL(pwm->hwpwm), pwmvalx);
> +	if (ret)
> +		goto clk_dis;
> +
> +	ret = regmap_field_write(pc->pwm_int_en, 0);

This seems to be never set to any other value, so perhaps it should be
set at .probe() time?

> +
> +clk_dis:
> +	clk_disable(pc->clk);
> +	return ret;
> +}
> +
> +static int st_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct st_pwm_chip *pc = to_st_pwmchip(chip);
> +	struct device *dev = pc->dev;
> +	int ret;
> +
> +	ret = clk_enable(pc->clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_write(pc->pwm_en, 1);
> +	if (ret)
> +		dev_err(dev, "%s,pwm_en write failed\n", __func__);

This error message is somewhat cryptic, perhaps:

	"failed to enable PWM"

? Also what implications does this have on controllers with multiple
channels?

> +
> +	return ret;
> +}
> +
> +static void st_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct st_pwm_chip *pc = to_st_pwmchip(chip);
> +	struct device *dev = pc->dev;
> +	unsigned int val;
> +
> +	regmap_field_write(pc->pwm_en, 0);

Does this turn off the second channel as well?

> +
> +	regmap_read(pc->regmap, ST_CNT, &val);

Does this read have any other use beyond the debugging output below?

> +
> +	dev_dbg(dev, "pwm counter :%u\n", val);
> +
> +	clk_disable(pc->clk);
> +}
> +
> +static const struct pwm_ops st_pwm_ops = {
> +	.config = st_pwm_config,
> +	.enable = st_pwm_enable,
> +	.disable = st_pwm_disable,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int st_pwm_probe_dt(struct st_pwm_chip *pc)
> +{
> +	struct device *dev = pc->dev;
> +	const struct reg_field *reg_fields;
> +	struct device_node *np = dev->of_node;
> +	struct st_pwm_compat_data *cdata = pc->cdata;
> +	u32 num_chan;
> +
> +	of_property_read_u32(np, "st,pwm-num-chan", &num_chan);
> +	if (num_chan)
> +		cdata->num_chan = num_chan;

I don't like this very much. What influences the number of channels? Is
it that specific SoC revisions have one and others have two?

> +
> +	reg_fields = cdata->reg_fields;
> +
> +	pc->prescale = devm_regmap_field_alloc(dev, pc->regmap,
> +					       reg_fields[PWMCLK_PRESCALE]);
> +	pc->pwm_en = devm_regmap_field_alloc(dev, pc->regmap,
> +					     reg_fields[PWM_EN]);
> +	pc->pwm_int_en = devm_regmap_field_alloc(dev, pc->regmap,
> +						 reg_fields[PWM_INT_EN]);
> +
> +	if (IS_ERR(pc->prescale) ||
> +	    IS_ERR(pc->pwm_en)   ||
> +	    IS_ERR(pc->pwm_int_en)) {
> +		dev_err(dev, "unable to allocate reg_field(s)\n");
> +		return -EINVAL;
> +	}

You're obfuscating error codes here. A better approach would be to check
each of these individually and return PTR_ERR(pc->...) to report the
most accurate error code.

> +
> +	return 0;
> +}
> +
> +static struct regmap_config st_pwm_regmap_config = {

static const

> +	.reg_bits   = 32,
> +	.val_bits   = 32,
> +	.reg_stride = 4,
> +};

Please drop the artificial padding. A single space on each side of '='
will do just fine.

> +
> +static int st_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct st_pwm_compat_data *cdata;
> +	struct st_pwm_chip *pc;
> +	struct resource *res;
> +	int ret;
> +
> +	if (!np) {
> +		dev_err(dev, "failed to find device node\n");
> +		return -EINVAL;
> +	}

I have difficulty imagining how this condition would ever happen. Can
this check not simply be removed?

> +
> +	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> +	if (!pc)
> +		return -ENOMEM;
> +
> +	cdata = devm_kzalloc(dev, sizeof(*cdata), GFP_KERNEL);
> +	if (!cdata)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	pc->mmio_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pc->mmio_base)) {
> +		dev_err(dev, "failed to find and map memory resources\n");

No need for this error message. devm_ioremap_resource() will provide one
for you.

> +		return PTR_ERR(pc->mmio_base);
> +	}
> +
> +	pc->regmap = devm_regmap_init_mmio(dev, pc->mmio_base,
> +					   &st_pwm_regmap_config);
> +	if (IS_ERR(pc->regmap))
> +		return PTR_ERR(pc->regmap);
> +
> +	/*
> +	 * Setup PWM data with default values: some values could be replaced
> +	 * with specific ones provided from device tree

Nit: this is a sentence and therefore should be terminated with a full
stop.

> +	 */
> +	cdata->reg_fields   = &st_pwm_regfields[0];

Why not simply "= st_pwm_regfields;"?

> +	cdata->max_pwm_cnt  = MAX_PWM_CNT_DEFAULT;
> +	cdata->max_prescale = MAX_PRESCALE_DEFAULT;
> +	cdata->num_chan     = NUM_CHAN_DEFAULT;
> +
> +	pc->cdata = cdata;
> +	pc->dev = dev;
> +
> +	ret = st_pwm_probe_dt(pc);
> +	if (ret)
> +		return ret;
> +
> +	pc->pwm_periods = devm_kzalloc(dev,
> +			sizeof(unsigned long) * (pc->cdata->max_prescale + 1),

This could use a temporary variable to make this shorter. I'd still urge
you to consider dropping the cache here, in which case you don't need
this allocation in the first place.

> +			GFP_KERNEL);
> +	if (!pc->pwm_periods)
> +		return -ENOMEM;
> +
> +	pc->clk = of_clk_get_by_name(np, "pwm");

devm_clk_get(&pdev->dev, "pwm")?

> +	if (IS_ERR(pc->clk)) {
> +		dev_err(dev, "failed to get pwm clock\n");
> +		return PTR_ERR(pc->clk);
> +	}
> +
> +	pc->clk_rate = clk_get_rate(pc->clk);
> +	if (!pc->clk_rate) {
> +		dev_err(dev, "failed to get clk rate\n");

"... clock rate\n"

> +		return -EINVAL;
> +	}
> +
> +	ret = clk_prepare(pc->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to prepare clk\n");

"... prepare clock\n"

> +		return ret;
> +	}
> +
> +	st_pwm_calc_periods(pc);
> +
> +	pc->chip.dev = dev;
> +	pc->chip.ops = &st_pwm_ops;
> +	pc->chip.base = -1;
> +	pc->chip.npwm = pc->cdata->num_chan;
> +	pc->chip.can_sleep = true;
> +
> +	ret = pwmchip_add(&pc->chip);
> +	if (ret < 0) {
> +		clk_unprepare(pc->clk);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pc);
> +
> +	return 0;
> +}
> +
> +static int st_pwm_remove(struct platform_device *pdev)
> +{
> +	struct st_pwm_chip *pc = platform_get_drvdata(pdev);
> +	int i;

unsigned

> +
> +	for (i = 0; i < pc->cdata->num_chan; i++)
> +		pwm_disable(&pc->chip.pwms[i]);
> +
> +	clk_unprepare(pc->clk);
> +
> +	return pwmchip_remove(&pc->chip);
> +}
> +
> +static struct of_device_id st_pwm_of_match[] = {

static const

> +	{ .compatible = "st,sti-pwm", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, st_pwm_of_match);
> +
> +static struct platform_driver st_pwm_driver = {
> +	.driver = {
> +		.name	= "st-pwm",
> +		.owner  = THIS_MODULE,

No need for this, module_platform_driver() will set it for you. Also
please drop the artificial padding above and below.

> +		.of_match_table = st_pwm_of_match,
> +	},
> +	.probe		= st_pwm_probe,
> +	.remove		= st_pwm_remove,
> +};
> +module_platform_driver(st_pwm_driver);
> +
> +MODULE_AUTHOR("STMicroelectronics (R&D) Limited <ajitpal.singh@st.com>");

This doesn't look right. Shouldn't the author be the same as in the
header comment here? The email address matches that, but the company
name is usually not a good fit for the MODULE_AUTHOR field.

> +MODULE_DESCRIPTION("STMicroelectronics ST PWM driver");
> +MODULE_LICENSE("GPL v2");

According to the file header comment this should be simply "GPL".

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 6/7] pwm: st: Add new driver for ST's PWM IP

[Lee Jones]

I'll comment on some of the more fluffy topics, I'll let Ajit reply to
the more technical details of the patch.

On Thu, 19 Jun 2014, Thierry Reding wrote:
> On Wed, Jun 18, 2014 at 03:52:51PM +0100, Lee Jones wrote:
> > This driver supports all current STi platforms' PWM IPs.
> > 
> > Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/pwm/Kconfig  |   9 ++
> >  drivers/pwm/Makefile |   1 +
> >  drivers/pwm/pwm-st.c | 378 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 388 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-st.c
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 4ad7b89..98a7bbc 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -292,4 +292,13 @@ config PWM_VT8500
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-vt8500.
> >  
> > +config PWM_ST
> 
> PWM_ST is awfully generic, perhaps PWM_STI would be a better choice?
> Even that's very generic. Maybe PWM_STI_H4XX? There's nothing wrong with
> supporting STiH{5,6,7,...}xx SoCs with such a driver. I'm just trying to
> think ahead what will happen if at some point a new SoC family is
> released that requires a different driver.

I'm inclined to agree with you, but as it stands, this driver supports
all ST h/w, so it's correct for it to be generic.  If some new IP
comes into fuition, at worst we'll have to change the name of the
driver.  I'm happy to put myself on the line for that if the time
comes.

> > diff --git a/drivers/pwm/pwm-st.c b/drivers/pwm/pwm-st.c
> [...]
> > +/*
> > + * PWM device driver for ST SoCs.
> > + * Author: Ajit Pal Singh <ajitpal.singh-qxv4g6HH51o@public.gmane.org>
> 
> Given this line, shouldn't the commit contain Ajit Pal Singh's
> Signed-off-by?

Absolutely, this is a failing on my part - will be applied in v2.

> > + *
> > + * Copyright (C) 2003-2014 STMicroelectronics (R&D) Limited
> 
> Was this driver really developed over a period of 11 years?

Probably, in one incarnation or other, but I'll speak to Ajit and see
if we can narrow this down to just this version.

> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> 
> Nit: no need for this extra blank line at the end of the comment.

Very well.

> > + */
> > +#include <linux/bsearch.h>
> 
> I prefer a blank line between the above two

Me too.  Will change.

> > +#include <linux/clk.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/time.h>
> 
> These should be sorted alphabetically.

Really? :)

> > +
> > +#define ST_PWMVAL(x)	(4 * (x))	/* Value used to define duty cycle */
> 
> "x" seems to designate the channel number, so perhaps make that clearer
> by naming the variable "ch"? Also in that case the comment is somewhat
> misleading.

Sure, will redefine.

> > +#define ST_PWMCR	0x50		/* Control/Config reg */
> > +#define ST_INTEN	0x54		/* Interrupt Enable/Disable reg */
> > +#define ST_CNT		0x60		/* PWMCounter */
> 
> I'd prefer s/reg/register/ and also use it consistently for the ST_CNT
> as well.

Okay.

> > +
> > +#define MAX_PWM_CNT_DEFAULT	255
> > +#define MAX_PRESCALE_DEFAULT	0xff
> > +#define NUM_CHAN_DEFAULT	1
> 
> These are only used in one place and their meaning is fairly obvious, so
> I'd just drop them.

I _always_ prefer defines over magic numbers, but as you wish - will fix.

> > +/* Regfield IDs */
> > +enum {
> > +	PWMCLK_PRESCALE = 0,
> 
> No need for "= 0".

It's more for clarity rather than technical requirement, but will remove.

> > +	PWM_EN,
> > +	PWM_INT_EN,
> > +	/* keep last */
> > +	MAX_REGFIELDS
> > +};
> > +
> > +struct st_pwm_chip {
> > +	struct device *dev;
> > +	struct clk *clk;
> > +	unsigned long clk_rate;
> > +	struct regmap *regmap;
> > +	struct st_pwm_compat_data *cdata;
> 
> Doesn't this require a predeclaration of struct st_pwm_compat_data? Or
> maybe just move struct st_pwm_compat_data before this.

You're right, will fix.

I think I would have expected at least a compiler warning about that?

> > +	struct regmap_field *prescale;
> > +	struct regmap_field *pwm_en;
> > +	struct regmap_field *pwm_int_en;
> > +	unsigned long *pwm_periods;
> > +	struct pwm_chip chip;
> > +	void __iomem *mmio_base;
> 
> mmio_base is somewhat long, maybe "mmio" or "base" would work equally
> well?

Okay.

> > +};
> > +
> > +struct st_pwm_compat_data {
> > +	const struct reg_field *reg_fields;
> > +	int num_chan;
> > +	int max_pwm_cnt;
> > +	int max_prescale;
> 
> Can't these three be unsigned?

I see no reason why not.  They can also be signed. :)

> > +};
> > +
> > +static const struct reg_field st_pwm_regfields[MAX_REGFIELDS] = {
> > +	[PWMCLK_PRESCALE]	= REG_FIELD(ST_PWMCR, 0, 3),
> > +	[PWM_EN]		= REG_FIELD(ST_PWMCR, 9, 9),
> > +	[PWM_INT_EN]		= REG_FIELD(ST_INTEN, 0, 0),
> > +};
> > +
> > +static inline struct st_pwm_chip *to_st_pwmchip(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct st_pwm_chip, chip);
> > +}
> > +
> > +/*
> > + * Calculate the period values supported by the PWM for the
> > + * current clock rate.
> > + */
> > +static void st_pwm_calc_periods(struct st_pwm_chip *pc)
> > +{
> > +	struct st_pwm_compat_data *cdata = pc->cdata;
> > +	struct device *dev = pc->dev;
> > +	unsigned long val;
> > +	int i;
> 
> unsigned?

Why?

It's much more common this way:

$ git grep $'\t'"int i;" | wc -l
17018
$ git grep $'\t'"unsigned int i;" | wc -l
2033

> > +
> > +	/*
> > +	 * period_ns = (10^9 * (prescaler + 1) * (MAX_PWM_COUNT + 1)) / CLK_RATE
> > +	 */
> > +	val = NSEC_PER_SEC / pc->clk_rate;
> > +	val *= cdata->max_pwm_cnt + 1;
> > +
> > +	dev_dbg(dev, "possible periods for clkrate[HZ]:%lu\n", pc->clk_rate);
> > +
> > +	for (i = 0; i <= cdata->max_prescale; i++) {
> > +		pc->pwm_periods[i] = val * (i + 1);
> > +		dev_dbg(dev, "prescale:%d, period[ns]:%lu\n",
> > +			i, pc->pwm_periods[i]);
> > +	}
> > +}
> > +
> > +static int st_pwm_cmp_periods(const void *key, const void *elt)
> > +{
> > +	unsigned long i = *(unsigned long *)key;
> > +	unsigned long j = *(unsigned long *)elt;
> > +
> > +	if (i < j)
> > +		return -1;
> > +	else
> > +		return i == j ? 0 : 1;
> > +}
> > +
> > +/*
> > + * For STiH4xx PWM IP, the pwm period is fixed to 256 local clock cycles.
> 
> s/pwm/PWM/ in prose. There are probably other occurrences of this
> throughout the driver.

Will change.

> > + * The only way to change the period (apart from changing the pwm input clock)
> > + * is to change the pwm clock prescaler.
> > + * The prescaler is of 4 bits, so only 16 prescaler values and hence only
> 
> I'm confused. This says there are 16 prescaler values, but at the same
> time the default maximum number of prescaler values is set to 255. Am I
> missing something?

I'll let Ajit answer this one, as he holds the technical documentation.

> > + * 16 possible period values are supported (for a particular clock rate).
> > + * The requested period will be applied only if it matches one of these
> > + * 16 values.
> > + */
> > +static int st_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			 int duty_ns, int period_ns)
> > +{
> > +	struct st_pwm_chip *pc = to_st_pwmchip(chip);
> > +	struct device *dev = pc->dev;
> > +	struct st_pwm_compat_data *cdata = pc->cdata;
> > +	unsigned int prescale, pwmvalx;
> > +	unsigned long *found;
> > +	int ret;
> > +
> > +	/*
> > +	 * Search for matching period value. The corresponding index is our
> > +	 * prescale value
> > +	 */
> > +	found = bsearch(&period_ns, &pc->pwm_periods[0],
> 
> Technically doesn't period_ns need to be converted to an unsigned long
> here? Otherwise this won't be compatible with 64-bit architectures.
> Admittedly that's maybe not something relevant for this family of SoCs,
> but you never know where this driver will be used eventually.

This driver depends on ARCH_STI which only supports 32bit.

> > +			cdata->max_prescale + 1, sizeof(unsigned long),
> > +			st_pwm_cmp_periods);
> > +	if (!found) {
> > +		dev_err(dev, "failed to find matching period\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	prescale = found - &pc->pwm_periods[0];
> 
> This is somewhat unconventional. None of the other drivers precompute
> possible periods and I'm not convinced that it's an advantage. Setting
> the period (and configuring the PWM in general) is a fairly uncommon
> operation.

Another one for Ajit I feel.

> > +	/*
> > +	 * When PWMVal == 0, PWM pulse = 1 local clock cycle.
> > +	 * When PWMVal == max_pwm_count,
> > +	 * PWM pulse = (max_pwm_count + 1) local cycles,
> > +	 * that is continuous pulse: signal never goes low.
> > +	 */
> > +	pwmvalx = cdata->max_pwm_cnt * duty_ns / period_ns;
> > +
> > +	dev_dbg(dev, "prescale:%u, period:%i, duty:%i, pwmvalx:%u\n",
> > +		prescale, period_ns, duty_ns, pwmvalx);
> > +
> > +	/* Enable clock before writing to PWM registers */
> > +	ret = clk_enable(pc->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_field_write(pc->prescale, prescale);
> > +	if (ret)
> > +		goto clk_dis;
> 
> So the prescaler is shared between all channels? In that case I think
> you should check for conflicting settings to prevent one channel from
> stomping on the other.

Ajit, if you'd be so kind.

> > +
> > +	ret = regmap_write(pc->regmap, ST_PWMVAL(pwm->hwpwm), pwmvalx);
> > +	if (ret)
> > +		goto clk_dis;
> > +
> > +	ret = regmap_field_write(pc->pwm_int_en, 0);
> 
> This seems to be never set to any other value, so perhaps it should be
> set at .probe() time?

Unfortunately not, as the clk needs to be enabled whilst setting IRQ
state.  Moving it into .probe() would mean wrapping this command
between clk_enable() and clk_disable(), which I think it a waste.

> > +
> > +clk_dis:
> > +	clk_disable(pc->clk);
> > +	return ret;
> > +}
> > +
> > +static int st_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct st_pwm_chip *pc = to_st_pwmchip(chip);
> > +	struct device *dev = pc->dev;
> > +	int ret;
> > +
> > +	ret = clk_enable(pc->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_field_write(pc->pwm_en, 1);
> > +	if (ret)
> > +		dev_err(dev, "%s,pwm_en write failed\n", __func__);
> 
> This error message is somewhat cryptic, perhaps:
> 
> 	"failed to enable PWM"

Agreed.  I also can't believe I missed that nasty __func__ too.

> ? Also what implications does this have on controllers with multiple
> channels?

I believe this enables both channels, but I'm sure Ajit will correct
me if I'm wrong.

> > +
> > +	return ret;
> > +}
> > +
> > +static void st_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct st_pwm_chip *pc = to_st_pwmchip(chip);
> > +	struct device *dev = pc->dev;
> > +	unsigned int val;
> > +
> > +	regmap_field_write(pc->pwm_en, 0);
> 
> Does this turn off the second channel as well?

Ajit, what happens if the other channel is still in use?

> > +
> > +	regmap_read(pc->regmap, ST_CNT, &val);
> 
> Does this read have any other use beyond the debugging output below?

Doesn't look like it does it.  Ajit, can this be removed?

> > +	dev_dbg(dev, "pwm counter :%u\n", val);
> > +
> > +	clk_disable(pc->clk);
> > +}
> > +
> > +static const struct pwm_ops st_pwm_ops = {
> > +	.config = st_pwm_config,
> > +	.enable = st_pwm_enable,
> > +	.disable = st_pwm_disable,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int st_pwm_probe_dt(struct st_pwm_chip *pc)
> > +{
> > +	struct device *dev = pc->dev;
> > +	const struct reg_field *reg_fields;
> > +	struct device_node *np = dev->of_node;
> > +	struct st_pwm_compat_data *cdata = pc->cdata;
> > +	u32 num_chan;
> > +
> > +	of_property_read_u32(np, "st,pwm-num-chan", &num_chan);
> > +	if (num_chan)
> > +		cdata->num_chan = num_chan;
> 
> I don't like this very much. What influences the number of channels? Is
> it that specific SoC revisions have one and others have two?

Ajit?

> > +	reg_fields = cdata->reg_fields;
> > +
> > +	pc->prescale = devm_regmap_field_alloc(dev, pc->regmap,
> > +					       reg_fields[PWMCLK_PRESCALE]);
> > +	pc->pwm_en = devm_regmap_field_alloc(dev, pc->regmap,
> > +					     reg_fields[PWM_EN]);
> > +	pc->pwm_int_en = devm_regmap_field_alloc(dev, pc->regmap,
> > +						 reg_fields[PWM_INT_EN]);
> > +
> > +	if (IS_ERR(pc->prescale) ||
> > +	    IS_ERR(pc->pwm_en)   ||
> > +	    IS_ERR(pc->pwm_int_en)) {
> > +		dev_err(dev, "unable to allocate reg_field(s)\n");
> > +		return -EINVAL;
> > +	}
> 
> You're obfuscating error codes here. A better approach would be to check
> each of these individually and return PTR_ERR(pc->...) to report the
> most accurate error code.

Agreed, will change.

> > +
> > +	return 0;
> > +}
> > +
> > +static struct regmap_config st_pwm_regmap_config = {
> 
> static const

Good catch.

> > +	.reg_bits   = 32,
> > +	.val_bits   = 32,
> > +	.reg_stride = 4,
> > +};
> 
> Please drop the artificial padding. A single space on each side of '='
> will do just fine.

/me likes straight lines!

... but as you wish.

> > +static int st_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct st_pwm_compat_data *cdata;
> > +	struct st_pwm_chip *pc;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	if (!np) {
> > +		dev_err(dev, "failed to find device node\n");
> > +		return -EINVAL;
> > +	}
> 
> I have difficulty imagining how this condition would ever happen. Can
> this check not simply be removed?

Although true, this check is very common.  I wonder if they can _all_
be removed from OF only drivers?  Probably something worth bringing up
with the DT guys.

> > +	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> > +	if (!pc)
> > +		return -ENOMEM;
> > +
> > +	cdata = devm_kzalloc(dev, sizeof(*cdata), GFP_KERNEL);
> > +	if (!cdata)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +	pc->mmio_base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(pc->mmio_base)) {
> > +		dev_err(dev, "failed to find and map memory resources\n");
> 
> No need for this error message. devm_ioremap_resource() will provide one
> for you.
> 
> > +		return PTR_ERR(pc->mmio_base);
> > +	}
> > +
> > +	pc->regmap = devm_regmap_init_mmio(dev, pc->mmio_base,
> > +					   &st_pwm_regmap_config);
> > +	if (IS_ERR(pc->regmap))
> > +		return PTR_ERR(pc->regmap);
> > +
> > +	/*
> > +	 * Setup PWM data with default values: some values could be replaced
> > +	 * with specific ones provided from device tree
> 
> Nit: this is a sentence and therefore should be terminated with a full
> stop.

:)

> > +	 */
> > +	cdata->reg_fields   = &st_pwm_regfields[0];
> 
> Why not simply "= st_pwm_regfields;"?

Although semantically the same, I think what we're trying to achieve
is more obvious at first glance in the current format.

But will fix if you are insistent.

> > +	cdata->max_pwm_cnt  = MAX_PWM_CNT_DEFAULT;
> > +	cdata->max_prescale = MAX_PRESCALE_DEFAULT;
> > +	cdata->num_chan     = NUM_CHAN_DEFAULT;

Look at those lovely straight lines.  Are you sure you want me to
unalign the regmap_config above?

> > +	pc->cdata = cdata;
> > +	pc->dev = dev;
> > +
> > +	ret = st_pwm_probe_dt(pc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pc->pwm_periods = devm_kzalloc(dev,
> > +			sizeof(unsigned long) * (pc->cdata->max_prescale + 1),
> 
> This could use a temporary variable to make this shorter. I'd still urge
> you to consider dropping the cache here, in which case you don't need
> this allocation in the first place.

Ajit, what would you like to do?

> > +			GFP_KERNEL);
> > +	if (!pc->pwm_periods)
> > +		return -ENOMEM;
> > +
> > +	pc->clk = of_clk_get_by_name(np, "pwm");
> 
> devm_clk_get(&pdev->dev, "pwm")?

Sure.

> > +	if (IS_ERR(pc->clk)) {
> > +		dev_err(dev, "failed to get pwm clock\n");
> > +		return PTR_ERR(pc->clk);
> > +	}
> > +
> > +	pc->clk_rate = clk_get_rate(pc->clk);
> > +	if (!pc->clk_rate) {
> > +		dev_err(dev, "failed to get clk rate\n");
> 
> "... clock rate\n"

clk is a well known synonym for clock in Linux and can be found
throughout many bootlogs, but again, happy to change if it's causing
issues.

> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = clk_prepare(pc->clk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to prepare clk\n");
> 
> "... prepare clock\n"

As above.

> > +		return ret;
> > +	}
> > +
> > +	st_pwm_calc_periods(pc);
> > +
> > +	pc->chip.dev = dev;
> > +	pc->chip.ops = &st_pwm_ops;
> > +	pc->chip.base = -1;
> > +	pc->chip.npwm = pc->cdata->num_chan;
> > +	pc->chip.can_sleep = true;
> > +
> > +	ret = pwmchip_add(&pc->chip);
> > +	if (ret < 0) {
> > +		clk_unprepare(pc->clk);
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, pc);
> > +
> > +	return 0;
> > +}
> > +
> > +static int st_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct st_pwm_chip *pc = platform_get_drvdata(pdev);
> > +	int i;
> 
> unsigned

As above.

> > +
> > +	for (i = 0; i < pc->cdata->num_chan; i++)
> > +		pwm_disable(&pc->chip.pwms[i]);
> > +
> > +	clk_unprepare(pc->clk);
> > +
> > +	return pwmchip_remove(&pc->chip);
> > +}
> > +
> > +static struct of_device_id st_pwm_of_match[] = {
> 
> static const

Good catch.

> > +	{ .compatible = "st,sti-pwm", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, st_pwm_of_match);
> > +
> > +static struct platform_driver st_pwm_driver = {
> > +	.driver = {
> > +		.name	= "st-pwm",
> > +		.owner  = THIS_MODULE,
> 
> No need for this, module_platform_driver() will set it for you. Also
> please drop the artificial padding above and below.

Will remove.

> > +		.of_match_table = st_pwm_of_match,
> > +	},
> > +	.probe		= st_pwm_probe,
> > +	.remove		= st_pwm_remove,
> > +};
> > +module_platform_driver(st_pwm_driver);
> > +
> > +MODULE_AUTHOR("STMicroelectronics (R&D) Limited <ajitpal.singh@st.com>");
> 
> This doesn't look right. Shouldn't the author be the same as in the
> header comment here? The email address matches that, but the company
> name is usually not a good fit for the MODULE_AUTHOR field.

Agree, will change.

> > +MODULE_DESCRIPTION("STMicroelectronics ST PWM driver");
> > +MODULE_LICENSE("GPL v2");
> 
> According to the file header comment this should be simply "GPL".

Will check with ST.

> Thierry

Thanks Thierry.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] pwm: st: Add new driver for ST's PWM IP

[Russell King - ARM Linux]

On Thu, Jun 19, 2014 at 09:44:04AM +0100, Lee Jones wrote:
> On Thu, 19 Jun 2014, Thierry Reding wrote:
> > On Wed, Jun 18, 2014 at 03:52:51PM +0100, Lee Jones wrote:
> > > +#include <linux/clk.h>
> > > +#include <linux/math64.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/time.h>
> > 
> > These should be sorted alphabetically.
> 
> Really? :)

Yes, as a general rule, for two reasons:

- It helps to reduce conflicts when two people add two different header
  files at a later date (provided they don't ignore the sorting and add
  them both at the end of the list.)

- It helps reduce the chance of having the same include listed twice
  (sometimes silently caused at git merge time) since it means if two
  people independently add the same include in the same place in the
  list, git will cope.  Eg, if one adds an include in one random place
  in the list, and someone else adds the same in another random place in
  the list, the result will be two includes of the same file.  If they're
  sufficiently far apart, git will not flag a conflict.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH 6/7] pwm: st: Add new driver for ST's PWM IP

[Ajit Pal]

Hi Thierry,

Thanks for your review. Please see my replies inline.

Regards
Ajitpal Singh

On Thursday 19 June 2014 02:14 PM, Lee Jones wrote:
> I'll comment on some of the more fluffy topics, I'll let Ajit reply to
> the more technical details of the patch.
>
> On Thu, 19 Jun 2014, Thierry Reding wrote:
>> On Wed, Jun 18, 2014 at 03:52:51PM +0100, Lee Jones wrote:
>>> This driver supports all current STi platforms' PWM IPs.
>>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>>   drivers/pwm/Kconfig  |   9 ++
>>>   drivers/pwm/Makefile |   1 +
>>>   drivers/pwm/pwm-st.c | 378 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 388 insertions(+)
>>>   create mode 100644 drivers/pwm/pwm-st.c
>>>
>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> index 4ad7b89..98a7bbc 100644
>>> --- a/drivers/pwm/Kconfig
>>> +++ b/drivers/pwm/Kconfig
>>> @@ -292,4 +292,13 @@ config PWM_VT8500
>>>        To compile this driver as a module, choose M here: the module
>>>        will be called pwm-vt8500.
>>>
>>> +config PWM_ST
>>
>> PWM_ST is awfully generic, perhaps PWM_STI would be a better choice?
>> Even that's very generic. Maybe PWM_STI_H4XX? There's nothing wrong with
>> supporting STiH{5,6,7,...}xx SoCs with such a driver. I'm just trying to
>> think ahead what will happen if at some point a new SoC family is
>> released that requires a different driver.
>
> I'm inclined to agree with you, but as it stands, this driver supports
> all ST h/w, so it's correct for it to be generic.  If some new IP
> comes into fuition, at worst we'll have to change the name of the
> driver.  I'm happy to put myself on the line for that if the time
> comes.
>
>>> diff --git a/drivers/pwm/pwm-st.c b/drivers/pwm/pwm-st.c
>> [...]
>>> +/*
>>> + * PWM device driver for ST SoCs.
>>> + * Author: Ajit Pal Singh <ajitpal.singh@st.com>
>>
>> Given this line, shouldn't the commit contain Ajit Pal Singh's
>> Signed-off-by?
>
> Absolutely, this is a failing on my part - will be applied in v2.
>
>>> + *
>>> + * Copyright (C) 2003-2014 STMicroelectronics (R&D) Limited
>>
>> Was this driver really developed over a period of 11 years?
>
> Probably, in one incarnation or other, but I'll speak to Ajit and see
> if we can narrow this down to just this version.
>
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>
>> Nit: no need for this extra blank line at the end of the comment.
>
> Very well.
>
>>> + */
>>> +#include <linux/bsearch.h>
>>
>> I prefer a blank line between the above two
>
> Me too.  Will change.
>
>>> +#include <linux/clk.h>
>>> +#include <linux/math64.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pwm.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/time.h>
>>
>> These should be sorted alphabetically.
>
> Really? :)
>
>>> +
>>> +#define ST_PWMVAL(x)       (4 * (x))       /* Value used to define duty cycle */
>>
>> "x" seems to designate the channel number, so perhaps make that clearer
>> by naming the variable "ch"? Also in that case the comment is somewhat
>> misleading.
>
> Sure, will redefine.
>
>>> +#define ST_PWMCR   0x50            /* Control/Config reg */
>>> +#define ST_INTEN   0x54            /* Interrupt Enable/Disable reg */
>>> +#define ST_CNT             0x60            /* PWMCounter */
>>
>> I'd prefer s/reg/register/ and also use it consistently for the ST_CNT
>> as well.
>
> Okay.
>
>>> +
>>> +#define MAX_PWM_CNT_DEFAULT        255
>>> +#define MAX_PRESCALE_DEFAULT       0xff
>>> +#define NUM_CHAN_DEFAULT   1
>>
>> These are only used in one place and their meaning is fairly obvious, so
>> I'd just drop them.
>
> I _always_ prefer defines over magic numbers, but as you wish - will fix.
>
>>> +/* Regfield IDs */
>>> +enum {
>>> +   PWMCLK_PRESCALE = 0,
>>
>> No need for "= 0".
>
> It's more for clarity rather than technical requirement, but will remove.
>
>>> +   PWM_EN,
>>> +   PWM_INT_EN,
>>> +   /* keep last */
>>> +   MAX_REGFIELDS
>>> +};
>>> +
>>> +struct st_pwm_chip {
>>> +   struct device *dev;
>>> +   struct clk *clk;
>>> +   unsigned long clk_rate;
>>> +   struct regmap *regmap;
>>> +   struct st_pwm_compat_data *cdata;
>>
>> Doesn't this require a predeclaration of struct st_pwm_compat_data? Or
>> maybe just move struct st_pwm_compat_data before this.
>
> You're right, will fix.
>
> I think I would have expected at least a compiler warning about that?
>
>>> +   struct regmap_field *prescale;
>>> +   struct regmap_field *pwm_en;
>>> +   struct regmap_field *pwm_int_en;
>>> +   unsigned long *pwm_periods;
>>> +   struct pwm_chip chip;
>>> +   void __iomem *mmio_base;
>>
>> mmio_base is somewhat long, maybe "mmio" or "base" would work equally
>> well?
>
> Okay.
>
>>> +};
>>> +
>>> +struct st_pwm_compat_data {
>>> +   const struct reg_field *reg_fields;
>>> +   int num_chan;
>>> +   int max_pwm_cnt;
>>> +   int max_prescale;
>>
>> Can't these three be unsigned?
>
> I see no reason why not.  They can also be signed. :)
>
>>> +};
>>> +
>>> +static const struct reg_field st_pwm_regfields[MAX_REGFIELDS] = {
>>> +   [PWMCLK_PRESCALE]       = REG_FIELD(ST_PWMCR, 0, 3),
>>> +   [PWM_EN]                = REG_FIELD(ST_PWMCR, 9, 9),
>>> +   [PWM_INT_EN]            = REG_FIELD(ST_INTEN, 0, 0),
>>> +};
>>> +
>>> +static inline struct st_pwm_chip *to_st_pwmchip(struct pwm_chip *chip)
>>> +{
>>> +   return container_of(chip, struct st_pwm_chip, chip);
>>> +}
>>> +
>>> +/*
>>> + * Calculate the period values supported by the PWM for the
>>> + * current clock rate.
>>> + */
>>> +static void st_pwm_calc_periods(struct st_pwm_chip *pc)
>>> +{
>>> +   struct st_pwm_compat_data *cdata = pc->cdata;
>>> +   struct device *dev = pc->dev;
>>> +   unsigned long val;
>>> +   int i;
>>
>> unsigned?
>
> Why?
>
> It's much more common this way:
>
> $ git grep $'\t'"int i;" | wc -l
> 17018
> $ git grep $'\t'"unsigned int i;" | wc -l
> 2033
>
>>> +
>>> +   /*
>>> +    * period_ns = (10^9 * (prescaler + 1) * (MAX_PWM_COUNT + 1)) / CLK_RATE
>>> +    */
>>> +   val = NSEC_PER_SEC / pc->clk_rate;
>>> +   val *= cdata->max_pwm_cnt + 1;
>>> +
>>> +   dev_dbg(dev, "possible periods for clkrate[HZ]:%lu\n", pc->clk_rate);
>>> +
>>> +   for (i = 0; i <= cdata->max_prescale; i++) {
>>> +           pc->pwm_periods[i] = val * (i + 1);
>>> +           dev_dbg(dev, "prescale:%d, period[ns]:%lu\n",
>>> +                   i, pc->pwm_periods[i]);
>>> +   }
>>> +}
>>> +
>>> +static int st_pwm_cmp_periods(const void *key, const void *elt)
>>> +{
>>> +   unsigned long i = *(unsigned long *)key;
>>> +   unsigned long j = *(unsigned long *)elt;
>>> +
>>> +   if (i < j)
>>> +           return -1;
>>> +   else
>>> +           return i == j ? 0 : 1;
>>> +}
>>> +
>>> +/*
>>> + * For STiH4xx PWM IP, the pwm period is fixed to 256 local clock cycles.
>>
>> s/pwm/PWM/ in prose. There are probably other occurrences of this
>> throughout the driver.
>
> Will change.
>
>>> + * The only way to change the period (apart from changing the pwm input clock)
>>> + * is to change the pwm clock prescaler.
>>> + * The prescaler is of 4 bits, so only 16 prescaler values and hence only
>>
>> I'm confused. This says there are 16 prescaler values, but at the same
>> time the default maximum number of prescaler values is set to 255. Am I
>> missing something?
>
> I'll let Ajit answer this one, as he holds the technical documentation.

Will correct this in the next patch.
>
>>> + * 16 possible period values are supported (for a particular clock rate).
>>> + * The requested period will be applied only if it matches one of these
>>> + * 16 values.
>>> + */
>>> +static int st_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>> +                    int duty_ns, int period_ns)
>>> +{
>>> +   struct st_pwm_chip *pc = to_st_pwmchip(chip);
>>> +   struct device *dev = pc->dev;
>>> +   struct st_pwm_compat_data *cdata = pc->cdata;
>>> +   unsigned int prescale, pwmvalx;
>>> +   unsigned long *found;
>>> +   int ret;
>>> +
>>> +   /*
>>> +    * Search for matching period value. The corresponding index is our
>>> +    * prescale value
>>> +    */
>>> +   found = bsearch(&period_ns, &pc->pwm_periods[0],
>>
>> Technically doesn't period_ns need to be converted to an unsigned long
>> here? Otherwise this won't be compatible with 64-bit architectures.
>> Admittedly that's maybe not something relevant for this family of SoCs,
>> but you never know where this driver will be used eventually.
>
> This driver depends on ARCH_STI which only supports 32bit.
>
>>> +                   cdata->max_prescale + 1, sizeof(unsigned long),
>>> +                   st_pwm_cmp_periods);
>>> +   if (!found) {
>>> +           dev_err(dev, "failed to find matching period\n");
>>> +           return -EINVAL;
>>> +   }
>>> +
>>> +   prescale = found - &pc->pwm_periods[0];
>>
>> This is somewhat unconventional. None of the other drivers precompute
>> possible periods and I'm not convinced that it's an advantage. Setting
>> the period (and configuring the PWM in general) is a fairly uncommon
>> operation.
>
> Another one for Ajit I feel.

For ST PWM IP, the PWM period is fixed to 256 local clock pulses.There 
is no register interface to select PWM periods.To change the period we 
have to change the prescaler.
We precompute the possible periods, so as to avoid the calculations 
everytime the .config function is called. Based upon a matching period 
we then select the prescaler.
Sorry but why do you think precomputing is not helpful ?
>
>>> +   /*
>>> +    * When PWMVal == 0, PWM pulse = 1 local clock cycle.
>>> +    * When PWMVal == max_pwm_count,
>>> +    * PWM pulse = (max_pwm_count + 1) local cycles,
>>> +    * that is continuous pulse: signal never goes low.
>>> +    */
>>> +   pwmvalx = cdata->max_pwm_cnt * duty_ns / period_ns;
>>> +
>>> +   dev_dbg(dev, "prescale:%u, period:%i, duty:%i, pwmvalx:%u\n",
>>> +           prescale, period_ns, duty_ns, pwmvalx);
>>> +
>>> +   /* Enable clock before writing to PWM registers */
>>> +   ret = clk_enable(pc->clk);
>>> +   if (ret)
>>> +           return ret;
>>> +
>>> +   ret = regmap_field_write(pc->prescale, prescale);
>>> +   if (ret)
>>> +           goto clk_dis;
>>
>> So the prescaler is shared between all channels? In that case I think
>> you should check for conflicting settings to prevent one channel from
>> stomping on the other.
>
> Ajit, if you'd be so kind.
>
Yes prescaler is shared between all the channels.Till now we have had 
only one user of PWM on the ST boards which I have used.
Anyway will add such a check in next version.

>>> +
>>> +   ret = regmap_write(pc->regmap, ST_PWMVAL(pwm->hwpwm), pwmvalx);
>>> +   if (ret)
>>> +           goto clk_dis;
>>> +
>>> +   ret = regmap_field_write(pc->pwm_int_en, 0);
>>
>> This seems to be never set to any other value, so perhaps it should be
>> set at .probe() time?
>
> Unfortunately not, as the clk needs to be enabled whilst setting IRQ
> state.  Moving it into .probe() would mean wrapping this command
> between clk_enable() and clk_disable(), which I think it a waste.
>
>>> +
>>> +clk_dis:
>>> +   clk_disable(pc->clk);
>>> +   return ret;
>>> +}
>>> +
>>> +static int st_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>>> +{
>>> +   struct st_pwm_chip *pc = to_st_pwmchip(chip);
>>> +   struct device *dev = pc->dev;
>>> +   int ret;
>>> +
>>> +   ret = clk_enable(pc->clk);
>>> +   if (ret)
>>> +           return ret;
>>> +
>>> +   ret = regmap_field_write(pc->pwm_en, 1);
>>> +   if (ret)
>>> +           dev_err(dev, "%s,pwm_en write failed\n", __func__);

>>
>> This error message is somewhat cryptic, perhaps:
>>
>>        "failed to enable PWM"
>
> Agreed.  I also can't believe I missed that nasty __func__ too.
>
>> ? Also what implications does this have on controllers with multiple
>> channels?
>
> I believe this enables both channels, but I'm sure Ajit will correct
> me if I'm wrong.

Yes it enables all channels.Unfortunately we do not have the facility to
enable/disable individual channels on the ST PWM IP.
>
>>> +
>>> +   return ret;
>>> +}
>>> +
>>> +static void st_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>> +{
>>> +   struct st_pwm_chip *pc = to_st_pwmchip(chip);
>>> +   struct device *dev = pc->dev;
>>> +   unsigned int val;
>>> +
>>> +   regmap_field_write(pc->pwm_en, 0);
>>
>> Does this turn off the second channel as well?
>
> Ajit, what happens if the other channel is still in use?
>
Yes it turns of all channels.
>>> +
>>> +   regmap_read(pc->regmap, ST_CNT, &val);
>>
>> Does this read have any other use beyond the debugging output below?
>
> Doesn't look like it does it.  Ajit, can this be removed?
>

Yes it can be removed.
>>> +   dev_dbg(dev, "pwm counter :%u\n", val);
>>> +
>>> +   clk_disable(pc->clk);
>>> +}
>>> +
>>> +static const struct pwm_ops st_pwm_ops = {
>>> +   .config = st_pwm_config,
>>> +   .enable = st_pwm_enable,
>>> +   .disable = st_pwm_disable,
>>> +   .owner = THIS_MODULE,
>>> +};
>>> +
>>> +static int st_pwm_probe_dt(struct st_pwm_chip *pc)
>>> +{
>>> +   struct device *dev = pc->dev;
>>> +   const struct reg_field *reg_fields;
>>> +   struct device_node *np = dev->of_node;
>>> +   struct st_pwm_compat_data *cdata = pc->cdata;
>>> +   u32 num_chan;
>>> +
>>> +   of_property_read_u32(np, "st,pwm-num-chan", &num_chan);
>>> +   if (num_chan)
>>> +           cdata->num_chan = num_chan;
>>
>> I don't like this very much. What influences the number of channels? Is
>> it that specific SoC revisions have one and others have two?
>
> Ajit?
>
Depends on the board type on which the SoC is used.
>>> +   reg_fields = cdata->reg_fields;
>>> +
>>> +   pc->prescale = devm_regmap_field_alloc(dev, pc->regmap,
>>> +                                          reg_fields[PWMCLK_PRESCALE]);
>>> +   pc->pwm_en = devm_regmap_field_alloc(dev, pc->regmap,
>>> +                                        reg_fields[PWM_EN]);
>>> +   pc->pwm_int_en = devm_regmap_field_alloc(dev, pc->regmap,
>>> +                                            reg_fields[PWM_INT_EN]);
>>> +
>>> +   if (IS_ERR(pc->prescale) ||
>>> +       IS_ERR(pc->pwm_en)   ||
>>> +       IS_ERR(pc->pwm_int_en)) {
>>> +           dev_err(dev, "unable to allocate reg_field(s)\n");
>>> +           return -EINVAL;
>>> +   }
>>
>> You're obfuscating error codes here. A better approach would be to check
>> each of these individually and return PTR_ERR(pc->...) to report the
>> most accurate error code.
>
> Agreed, will change.
>
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static struct regmap_config st_pwm_regmap_config = {
>>
>> static const
>
> Good catch.
>
>>> +   .reg_bits   = 32,
>>> +   .val_bits   = 32,
>>> +   .reg_stride = 4,
>>> +};
>>
>> Please drop the artificial padding. A single space on each side of '='
>> will do just fine.
>
> /me likes straight lines!
>
> ... but as you wish.
>
>>> +static int st_pwm_probe(struct platform_device *pdev)
>>> +{
>>> +   struct device *dev = &pdev->dev;
>>> +   struct device_node *np = dev->of_node;
>>> +   struct st_pwm_compat_data *cdata;
>>> +   struct st_pwm_chip *pc;
>>> +   struct resource *res;
>>> +   int ret;
>>> +
>>> +   if (!np) {
>>> +           dev_err(dev, "failed to find device node\n");
>>> +           return -EINVAL;
>>> +   }
>>
>> I have difficulty imagining how this condition would ever happen. Can
>> this check not simply be removed?
>
> Although true, this check is very common.  I wonder if they can _all_
> be removed from OF only drivers?  Probably something worth bringing up
> with the DT guys.
>
>>> +   pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
>>> +   if (!pc)
>>> +           return -ENOMEM;
>>> +
>>> +   cdata = devm_kzalloc(dev, sizeof(*cdata), GFP_KERNEL);
>>> +   if (!cdata)
>>> +           return -ENOMEM;
>>> +
>>> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +
>>> +   pc->mmio_base = devm_ioremap_resource(dev, res);
>>> +   if (IS_ERR(pc->mmio_base)) {
>>> +           dev_err(dev, "failed to find and map memory resources\n");
>>
>> No need for this error message. devm_ioremap_resource() will provide one
>> for you.
>>
>>> +           return PTR_ERR(pc->mmio_base);
>>> +   }
>>> +
>>> +   pc->regmap = devm_regmap_init_mmio(dev, pc->mmio_base,
>>> +                                      &st_pwm_regmap_config);
>>> +   if (IS_ERR(pc->regmap))
>>> +           return PTR_ERR(pc->regmap);
>>> +
>>> +   /*
>>> +    * Setup PWM data with default values: some values could be replaced
>>> +    * with specific ones provided from device tree
>>
>> Nit: this is a sentence and therefore should be terminated with a full
>> stop.
>
> :)
>
>>> +    */
>>> +   cdata->reg_fields   = &st_pwm_regfields[0];
>>
>> Why not simply "= st_pwm_regfields;"?
>
> Although semantically the same, I think what we're trying to achieve
> is more obvious at first glance in the current format.
>
> But will fix if you are insistent.
>
>>> +   cdata->max_pwm_cnt  = MAX_PWM_CNT_DEFAULT;
>>> +   cdata->max_prescale = MAX_PRESCALE_DEFAULT;
>>> +   cdata->num_chan     = NUM_CHAN_DEFAULT;
>
> Look at those lovely straight lines.  Are you sure you want me to
> unalign the regmap_config above?
>
>>> +   pc->cdata = cdata;
>>> +   pc->dev = dev;
>>> +
>>> +   ret = st_pwm_probe_dt(pc);
>>> +   if (ret)
>>> +           return ret;
>>> +
>>> +   pc->pwm_periods = devm_kzalloc(dev,
>>> +                   sizeof(unsigned long) * (pc->cdata->max_prescale + 1),
>>
>> This could use a temporary variable to make this shorter. I'd still urge
>> you to consider dropping the cache here, in which case you don't need
>> this allocation in the first place.
>
> Ajit, what would you like to do?

Discussed above

>
>>> +                   GFP_KERNEL);
>>> +   if (!pc->pwm_periods)
>>> +           return -ENOMEM;
>>> +
>>> +   pc->clk = of_clk_get_by_name(np, "pwm");
>>
>> devm_clk_get(&pdev->dev, "pwm")?
>
> Sure.
>
>>> +   if (IS_ERR(pc->clk)) {
>>> +           dev_err(dev, "failed to get pwm clock\n");
>>> +           return PTR_ERR(pc->clk);
>>> +   }
>>> +
>>> +   pc->clk_rate = clk_get_rate(pc->clk);
>>> +   if (!pc->clk_rate) {
>>> +           dev_err(dev, "failed to get clk rate\n");
>>
>> "... clock rate\n"
>
> clk is a well known synonym for clock in Linux and can be found
> throughout many bootlogs, but again, happy to change if it's causing
> issues.
>
>>> +           return -EINVAL;
>>> +   }
>>> +
>>> +   ret = clk_prepare(pc->clk);
>>> +   if (ret) {
>>> +           dev_err(dev, "failed to prepare clk\n");
>>
>> "... prepare clock\n"
>
> As above.
>
>>> +           return ret;
>>> +   }
>>> +
>>> +   st_pwm_calc_periods(pc);
>>> +
>>> +   pc->chip.dev = dev;
>>> +   pc->chip.ops = &st_pwm_ops;
>>> +   pc->chip.base = -1;
>>> +   pc->chip.npwm = pc->cdata->num_chan;
>>> +   pc->chip.can_sleep = true;
>>> +
>>> +   ret = pwmchip_add(&pc->chip);
>>> +   if (ret < 0) {
>>> +           clk_unprepare(pc->clk);
>>> +           return ret;
>>> +   }
>>> +
>>> +   platform_set_drvdata(pdev, pc);
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static int st_pwm_remove(struct platform_device *pdev)
>>> +{
>>> +   struct st_pwm_chip *pc = platform_get_drvdata(pdev);
>>> +   int i;
>>
>> unsigned
>
> As above.
>
>>> +
>>> +   for (i = 0; i < pc->cdata->num_chan; i++)
>>> +           pwm_disable(&pc->chip.pwms[i]);
>>> +
>>> +   clk_unprepare(pc->clk);
>>> +
>>> +   return pwmchip_remove(&pc->chip);
>>> +}
>>> +
>>> +static struct of_device_id st_pwm_of_match[] = {
>>
>> static const
>
> Good catch.
>
>>> +   { .compatible = "st,sti-pwm", },
>>> +   { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, st_pwm_of_match);
>>> +
>>> +static struct platform_driver st_pwm_driver = {
>>> +   .driver = {
>>> +           .name   = "st-pwm",
>>> +           .owner  = THIS_MODULE,
>>
>> No need for this, module_platform_driver() will set it for you. Also
>> please drop the artificial padding above and below.
>
> Will remove.
>
>>> +           .of_match_table = st_pwm_of_match,
>>> +   },
>>> +   .probe          = st_pwm_probe,
>>> +   .remove         = st_pwm_remove,
>>> +};
>>> +module_platform_driver(st_pwm_driver);
>>> +
>>> +MODULE_AUTHOR("STMicroelectronics (R&D) Limited <ajitpal.singh@st.com>");
>>
>> This doesn't look right. Shouldn't the author be the same as in the
>> header comment here? The email address matches that, but the company
>> name is usually not a good fit for the MODULE_AUTHOR field.
>
> Agree, will change.
>
>>> +MODULE_DESCRIPTION("STMicroelectronics ST PWM driver");
>>> +MODULE_LICENSE("GPL v2");
>>
>> According to the file header comment this should be simply "GPL".
>
> Will check with ST.
>
>> Thierry
>
> Thanks Thierry.
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
>

Re: [PATCH 6/7] pwm: st: Add new driver for ST's PWM IP

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4076 bytes --]

On Thu, Jun 19, 2014 at 07:57:14PM +0530, Ajit Pal wrote:
> On Thursday 19 June 2014 02:14 PM, Lee Jones wrote:
> >On Thu, 19 Jun 2014, Thierry Reding wrote:
> >>On Wed, Jun 18, 2014 at 03:52:51PM +0100, Lee Jones wrote:
[...]
> >>>+                   cdata->max_prescale + 1, sizeof(unsigned long),
> >>>+                   st_pwm_cmp_periods);
> >>>+   if (!found) {
> >>>+           dev_err(dev, "failed to find matching period\n");
> >>>+           return -EINVAL;
> >>>+   }
> >>>+
> >>>+   prescale = found - &pc->pwm_periods[0];
> >>
> >>This is somewhat unconventional. None of the other drivers precompute
> >>possible periods and I'm not convinced that it's an advantage. Setting
> >>the period (and configuring the PWM in general) is a fairly uncommon
> >>operation.
> >
> >Another one for Ajit I feel.
> 
> For ST PWM IP, the PWM period is fixed to 256 local clock pulses.There is no
> register interface to select PWM periods.To change the period we have to
> change the prescaler.
> We precompute the possible periods, so as to avoid the calculations
> everytime the .config function is called. Based upon a matching period we
> then select the prescaler.
> Sorry but why do you think precomputing is not helpful ?

Mostly I dislike it here because it sticks out as nobody else is doing
it. Secondly I'm not convinced that it gives you much of a performance
gain since the computations aren't that involved and typically the
period isn't changed all that often.

Also computing the value directly in .config() makes the code much
easier to follow.

> >>>+static int st_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >>>+{
> >>>+   struct st_pwm_chip *pc = to_st_pwmchip(chip);
> >>>+   struct device *dev = pc->dev;
> >>>+   int ret;
> >>>+
> >>>+   ret = clk_enable(pc->clk);
> >>>+   if (ret)
> >>>+           return ret;
> >>>+
> >>>+   ret = regmap_field_write(pc->pwm_en, 1);
> >>>+   if (ret)
> >>>+           dev_err(dev, "%s,pwm_en write failed\n", __func__);
> 
> >>
> >>This error message is somewhat cryptic, perhaps:
> >>
> >>       "failed to enable PWM"
> >
> >Agreed.  I also can't believe I missed that nasty __func__ too.
> >
> >>? Also what implications does this have on controllers with multiple
> >>channels?
> >
> >I believe this enables both channels, but I'm sure Ajit will correct
> >me if I'm wrong.
> 
> Yes it enables all channels.Unfortunately we do not have the facility to
> enable/disable individual channels on the ST PWM IP.

That's bad. If you can't control them separately then there's no way you
can guarantee the semantics of the PWM framework.

> >>>+   dev_dbg(dev, "pwm counter :%u\n", val);
> >>>+
> >>>+   clk_disable(pc->clk);
> >>>+}
> >>>+
> >>>+static const struct pwm_ops st_pwm_ops = {
> >>>+   .config = st_pwm_config,
> >>>+   .enable = st_pwm_enable,
> >>>+   .disable = st_pwm_disable,
> >>>+   .owner = THIS_MODULE,
> >>>+};
> >>>+
> >>>+static int st_pwm_probe_dt(struct st_pwm_chip *pc)
> >>>+{
> >>>+   struct device *dev = pc->dev;
> >>>+   const struct reg_field *reg_fields;
> >>>+   struct device_node *np = dev->of_node;
> >>>+   struct st_pwm_compat_data *cdata = pc->cdata;
> >>>+   u32 num_chan;
> >>>+
> >>>+   of_property_read_u32(np, "st,pwm-num-chan", &num_chan);
> >>>+   if (num_chan)
> >>>+           cdata->num_chan = num_chan;
> >>
> >>I don't like this very much. What influences the number of channels? Is
> >>it that specific SoC revisions have one and others have two?
> >
> >Ajit?
> >
> Depends on the board type on which the SoC is used.

I don't understand. How can the board influence the number of PWM
channels that the SoC supports? It does make sense for a board to define
how many of them are actually *used*, but that's nothing that DT should
contain nor that the driver should care about. The driver (and DT for
that matter) should expose the hardware block's full capabilities. The
use-case is what should determine what's used and what not.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 6/7] pwm: st: Add new driver for ST's PWM IP

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8645 bytes --]

On Thu, Jun 19, 2014 at 09:44:04AM +0100, Lee Jones wrote:
> I'll comment on some of the more fluffy topics, I'll let Ajit reply to
> the more technical details of the patch.
> 
> On Thu, 19 Jun 2014, Thierry Reding wrote:
> > On Wed, Jun 18, 2014 at 03:52:51PM +0100, Lee Jones wrote:
> > > This driver supports all current STi platforms' PWM IPs.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/pwm/Kconfig  |   9 ++
> > >  drivers/pwm/Makefile |   1 +
> > >  drivers/pwm/pwm-st.c | 378 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 388 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-st.c
> > > 
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 4ad7b89..98a7bbc 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -292,4 +292,13 @@ config PWM_VT8500
> > >  	  To compile this driver as a module, choose M here: the module
> > >  	  will be called pwm-vt8500.
> > >  
> > > +config PWM_ST
> > 
> > PWM_ST is awfully generic, perhaps PWM_STI would be a better choice?
> > Even that's very generic. Maybe PWM_STI_H4XX? There's nothing wrong with
> > supporting STiH{5,6,7,...}xx SoCs with such a driver. I'm just trying to
> > think ahead what will happen if at some point a new SoC family is
> > released that requires a different driver.
> 
> I'm inclined to agree with you, but as it stands, this driver supports
> all ST h/w, so it's correct for it to be generic.  If some new IP
> comes into fuition, at worst we'll have to change the name of the
> driver.  I'm happy to put myself on the line for that if the time
> comes.

Renaming a driver isn't a trivial matter. People may be using the name
in blacklists or scripts and renaming will likely annoy them. Like I
said, there's nothing wrong with the driver name being less generic, we
have other ways to identify what hardware it will run on.

> > > diff --git a/drivers/pwm/pwm-st.c b/drivers/pwm/pwm-st.c
[...]
> > > +#define MAX_PWM_CNT_DEFAULT	255
> > > +#define MAX_PRESCALE_DEFAULT	0xff
> > > +#define NUM_CHAN_DEFAULT	1
> > 
> > These are only used in one place and their meaning is fairly obvious, so
> > I'd just drop them.
> 
> I _always_ prefer defines over magic numbers, but as you wish - will fix.

In general I agree, but there are cases where in my opinion the defines
obfuscate rather than help. This is one of those. These aren't really
magic numbers, since they are used in a context where their meaning is
crystal clear.

> > > +	PWM_EN,
> > > +	PWM_INT_EN,
> > > +	/* keep last */
> > > +	MAX_REGFIELDS
> > > +};
> > > +
> > > +struct st_pwm_chip {
> > > +	struct device *dev;
> > > +	struct clk *clk;
> > > +	unsigned long clk_rate;
> > > +	struct regmap *regmap;
> > > +	struct st_pwm_compat_data *cdata;
> > 
> > Doesn't this require a predeclaration of struct st_pwm_compat_data? Or
> > maybe just move struct st_pwm_compat_data before this.
> 
> You're right, will fix.
> 
> I think I would have expected at least a compiler warning about that?

Me too. Perhaps one of the includes has a forward declaration? I'd hope
not.

> > > +};
> > > +
> > > +struct st_pwm_compat_data {
> > > +	const struct reg_field *reg_fields;
> > > +	int num_chan;
> > > +	int max_pwm_cnt;
> > > +	int max_prescale;
> > 
> > Can't these three be unsigned?
> 
> I see no reason why not.  They can also be signed. :)

I prefer if variables use the strictest type possible.

> > > +static void st_pwm_calc_periods(struct st_pwm_chip *pc)
> > > +{
> > > +	struct st_pwm_compat_data *cdata = pc->cdata;
> > > +	struct device *dev = pc->dev;
> > > +	unsigned long val;
> > > +	int i;
> > 
> > unsigned?
> 
> Why?
> 
> It's much more common this way:
> 
> $ git grep $'\t'"int i;" | wc -l
> 17018
> $ git grep $'\t'"unsigned int i;" | wc -l
> 2033

That just means that not everybody is as pedantic as I am. The reason
why it should be unsigned int is that it's used in a loop and compared
to a value which should also be unsigned (cdata->max_prescale). There
just isn't a reasonable scenario where they would need to be negative.

> > > + * 16 possible period values are supported (for a particular clock rate).
> > > + * The requested period will be applied only if it matches one of these
> > > + * 16 values.
> > > + */
> > > +static int st_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			 int duty_ns, int period_ns)
> > > +{
> > > +	struct st_pwm_chip *pc = to_st_pwmchip(chip);
> > > +	struct device *dev = pc->dev;
> > > +	struct st_pwm_compat_data *cdata = pc->cdata;
> > > +	unsigned int prescale, pwmvalx;
> > > +	unsigned long *found;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * Search for matching period value. The corresponding index is our
> > > +	 * prescale value
> > > +	 */
> > > +	found = bsearch(&period_ns, &pc->pwm_periods[0],
> > 
> > Technically doesn't period_ns need to be converted to an unsigned long
> > here? Otherwise this won't be compatible with 64-bit architectures.
> > Admittedly that's maybe not something relevant for this family of SoCs,
> > but you never know where this driver will be used eventually.
> 
> This driver depends on ARCH_STI which only supports 32bit.

That's true now. It may not be forever. Also there's always a chance
that somebody will look at your code for inspiration and will copy the
same non-portability.

> > > +	ret = regmap_write(pc->regmap, ST_PWMVAL(pwm->hwpwm), pwmvalx);
> > > +	if (ret)
> > > +		goto clk_dis;
> > > +
> > > +	ret = regmap_field_write(pc->pwm_int_en, 0);
> > 
> > This seems to be never set to any other value, so perhaps it should be
> > set at .probe() time?
> 
> Unfortunately not, as the clk needs to be enabled whilst setting IRQ
> state.  Moving it into .probe() would mean wrapping this command
> between clk_enable() and clk_disable(), which I think it a waste.

That's a one-time thing nevertheless. If you keep it here the register
will keep being written with the same value over and over again.

> > > +	.reg_bits   = 32,
> > > +	.val_bits   = 32,
> > > +	.reg_stride = 4,
> > > +};
> > 
> > Please drop the artificial padding. A single space on each side of '='
> > will do just fine.
> 
> /me likes straight lines!
> 
> ... but as you wish.

And at some point you need to add a field that exceeds the current
padding and then you'll have one line that stands out. Trust me, that's
a whole lot worse than making each of them use a single space around '='
consistently from the beginning.

Or you'd need to update the whole block at the same time, which is just
needless churn.

> > > +static int st_pwm_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct device_node *np = dev->of_node;
> > > +	struct st_pwm_compat_data *cdata;
> > > +	struct st_pwm_chip *pc;
> > > +	struct resource *res;
> > > +	int ret;
> > > +
> > > +	if (!np) {
> > > +		dev_err(dev, "failed to find device node\n");
> > > +		return -EINVAL;
> > > +	}
> > 
> > I have difficulty imagining how this condition would ever happen. Can
> > this check not simply be removed?
> 
> Although true, this check is very common.  I wonder if they can _all_
> be removed from OF only drivers?  Probably something worth bringing up
> with the DT guys.

Yes, it's completely unnecessary for OF-only drivers.

> > > +	 */
> > > +	cdata->reg_fields   = &st_pwm_regfields[0];
> > 
> > Why not simply "= st_pwm_regfields;"?
> 
> Although semantically the same, I think what we're trying to achieve
> is more obvious at first glance in the current format.
> 
> But will fix if you are insistent.

Yes, please.

> Look at those lovely straight lines.  Are you sure you want me to
> unalign the regmap_config above?

	cdata->max_pwm_cnt  = MAX_PWM_CNT_DEFAULT;
	cdata->max_prescale = MAX_PRESCALE_DEFAULT;
	cdata->num_chan     = NUM_CHAN_DEFAULT;
	cdata->some_other_param = SOME_OTHER_PARAM_DEFAULT;

Not very pretty, is it?

> > > +	if (IS_ERR(pc->clk)) {
> > > +		dev_err(dev, "failed to get pwm clock\n");
> > > +		return PTR_ERR(pc->clk);
> > > +	}
> > > +
> > > +	pc->clk_rate = clk_get_rate(pc->clk);
> > > +	if (!pc->clk_rate) {
> > > +		dev_err(dev, "failed to get clk rate\n");
> > 
> > "... clock rate\n"
> 
> clk is a well known synonym for clock in Linux and can be found
> throughout many bootlogs, but again, happy to change if it's causing
> issues.

Yes, please.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 7/7] pwm: st: Supply Device Tree binding documentation for ST's PWM IP

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/pwm/pwm-st.txt | 41 ++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-st.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
new file mode 100644
index 0000000..ea88c5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
@@ -0,0 +1,41 @@
+STMicroelectronics PWM driver bindings
+--------------------------------------
+
+Required parameters:
+- compatible :		"st,pwm"
+- #pwm-cells : 		Number of cells used to specify a PWM. First cell
+			specifies the per-chip index of the PWM to use and the
+			second cell is the period in nanoseconds - fixed to 2
+			for STiH41x.
+- reg :        		Physical base address and length of the controller's
+			registers.
+- pinctrl-names: 	Set to "default".
+- pinctrl-0: 		List of phandles pointing to pin configuration nodes
+			for PWM module.
+			For Pinctrl properties, please refer to [1].
+- clock-names: 		Set to "pwm".
+- clocks: 		phandle of the clock used by the PWM module.
+			For Clk properties, please refer to [2].
+
+Optional properties:
+- st,pwm-num-chan:	Number of available channels. If not passed, the driver
+			will consider single channel by default.
+
+[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+[2] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Example:
+
+pwm1: pwm@fe510000 {
+	compatible = "st,pwm";
+	reg = <0xfe510000 0x68>;
+	#pwm-cells = <2>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pwm1_chan0_default
+		     &pinctrl_pwm1_chan1_default
+		     &pinctrl_pwm1_chan2_default
+		     &pinctrl_pwm1_chan3_default>;
+	clocks = <&CLK_SYSIN>;
+	clock-names = "pwm";
+	st,pwm-num-chan = <4>;
+};
-- 
1.8.3.2

Re: [PATCH 7/7] pwm: st: Supply Device Tree binding documentation for ST's PWM IP

[Gabriel Fernandez]

Hi Lee,
minor remark below..

On 18 June 2014 16:52, Lee Jones <lee.jones@linaro.org> wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-st.txt | 41 ++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-st.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> new file mode 100644
> index 0000000..ea88c5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> @@ -0,0 +1,41 @@
> +STMicroelectronics PWM driver bindings
> +--------------------------------------

> +Example:
> +
> +pwm1: pwm@fe510000 {
> +       compatible = "st,pwm";
> +       reg = <0xfe510000 0x68>;
> +       #pwm-cells = <2>;
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_pwm1_chan0_default
> +                    &pinctrl_pwm1_chan1_default
> +                    &pinctrl_pwm1_chan2_default
> +                    &pinctrl_pwm1_chan3_default>;
> +       clocks = <&CLK_SYSIN>;
clock name in lower case

> +       clock-names = "pwm";
> +       st,pwm-num-chan = <4>;
> +};
> --
> 1.8.3.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/7] pwm: st: Supply Device Tree binding documentation for ST's PWM IP

[Lee Jones]

On Wed, 18 Jun 2014, Gabriel Fernandez wrote:

> Hi Lee,
> minor remark below..
> 
> On 18 June 2014 16:52, Lee Jones <lee.jones@linaro.org> wrote:
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-st.txt | 41 ++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-st.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> > new file mode 100644
> > index 0000000..ea88c5c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> > @@ -0,0 +1,41 @@
> > +STMicroelectronics PWM driver bindings
> > +--------------------------------------
> 
> > +Example:
> > +
> > +pwm1: pwm@fe510000 {
> > +       compatible = "st,pwm";
> > +       reg = <0xfe510000 0x68>;
> > +       #pwm-cells = <2>;
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_pwm1_chan0_default
> > +                    &pinctrl_pwm1_chan1_default
> > +                    &pinctrl_pwm1_chan2_default
> > +                    &pinctrl_pwm1_chan3_default>;
> > +       clocks = <&CLK_SYSIN>;
> clock name in lower case

Good catch.  Will fix.

> > +       clock-names = "pwm";
> > +       st,pwm-num-chan = <4>;
> > +};
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 7/7] pwm: st: Supply Device Tree binding documentation for ST's PWM IP

[Lee Jones]

pwm: st: Supply Device Tree binding documentation for ST's PWM IP
    
Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
new file mode 100644
index 0000000..84d2fb8
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
@@ -0,0 +1,41 @@
+STMicroelectronics PWM driver bindings
+--------------------------------------
+
+Required parameters:
+- compatible :		"st,pwm"
+- #pwm-cells : 		Number of cells used to specify a PWM. First cell
+			specifies the per-chip index of the PWM to use and the
+			second cell is the period in nanoseconds - fixed to 2
+			for STiH41x.
+- reg :			Physical base address and length of the controller's
+			registers.
+- pinctrl-names: 	Set to "default".
+- pinctrl-0: 		List of phandles pointing to pin configuration nodes
+			for PWM module.
+			For Pinctrl properties, please refer to [1].
+- clock-names: 		Set to "pwm".
+- clocks: 		phandle of the clock used by the PWM module.
+			For Clk properties, please refer to [2].
+
+Optional properties:
+- st,pwm-num-chan:	Number of available channels. If not passed, the driver
+			will consider single channel by default.
+
+[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+[2] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Example:
+
+pwm1: pwm@fe510000 {
+	compatible = "st,pwm";
+	reg = <0xfe510000 0x68>;
+	#pwm-cells = <2>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pwm1_chan0_default
+		     &pinctrl_pwm1_chan1_default
+		     &pinctrl_pwm1_chan2_default
+		     &pinctrl_pwm1_chan3_default>;
+	clocks = <&clk_sysin>;
+	clock-names = "pwm";
+	st,pwm-num-chan = <4>;
+};

Re: [PATCH 1/7] ARM: stih407: Add DT nodes for for PWM

[Gabriel Fernandez]

Hi Lee,
minor remark below..

On 18 June 2014 16:52, Lee Jones <lee.jones@linaro.org> wrote:
> Supply top level nodes for the STiH407 based development boards.
> The Pinctrl configuration has already been applied, so the only
> missing piece of the DT puzzle is for a board's DTB to enable
> the nodes.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/boot/dts/stih407.dtsi | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih407.dtsi b/arch/arm/boot/dts/stih407.dtsi
> index 4f9024f..6b43ba2 100644
> --- a/arch/arm/boot/dts/stih407.dtsi
> +++ b/arch/arm/boot/dts/stih407.dtsi
> @@ -259,5 +259,32 @@
>
>                         status = "disabled";
>                 };
> +
> +               /* COMMS PWM Module */
> +               pwm0: pwm@9810000 {
> +                       compatible      = "st,sti-pwm";
> +                       status          = "disabled";
> +                       #pwm-cells      = <2>;
> +                       reg             = <0x9810000 0x68>;
> +                       pinctrl-names   = "default";
> +                        pinctrl-0      = <&pinctrl_pwm0_chan0_default>;
del space

> +                       clock-names     = "pwm";
> +                       clocks          = <&clk_sysin>;
> +               };
> +
> +               /* SBC PWM Module */
> +               pwm1: pwm@9510000 {
> +                       compatible      = "st,sti-pwm";
> +                       status          = "disabled";
> +                       #pwm-cells      = <2>;
> +                       reg             = <0x9510000 0x68>;
> +                       pinctrl-names   = "default";
> +                       pinctrl-0       = <&pinctrl_pwm1_chan0_default
> +                                       &pinctrl_pwm1_chan1_default
> +                                       &pinctrl_pwm1_chan2_default
> +                                       &pinctrl_pwm1_chan3_default>;
> +                       clock-names     = "pwm";
> +                       clocks          = <&clk_sysin>;
> +               };
>         };
>  };
> --
> 1.8.3.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/7] ARM: stih407: Add DT nodes for for PWM

[Lee Jones]

On Wed, 18 Jun 2014, Gabriel Fernandez wrote:

> Hi Lee,
> minor remark below..
> 
> On 18 June 2014 16:52, Lee Jones <lee.jones@linaro.org> wrote:
> > Supply top level nodes for the STiH407 based development boards.
> > The Pinctrl configuration has already been applied, so the only
> > missing piece of the DT puzzle is for a board's DTB to enable
> > the nodes.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/boot/dts/stih407.dtsi | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/stih407.dtsi b/arch/arm/boot/dts/stih407.dtsi
> > index 4f9024f..6b43ba2 100644
> > --- a/arch/arm/boot/dts/stih407.dtsi
> > +++ b/arch/arm/boot/dts/stih407.dtsi
> > @@ -259,5 +259,32 @@
> >
> >                         status = "disabled";
> >                 };
> > +
> > +               /* COMMS PWM Module */
> > +               pwm0: pwm@9810000 {
> > +                       compatible      = "st,sti-pwm";
> > +                       status          = "disabled";
> > +                       #pwm-cells      = <2>;
> > +                       reg             = <0x9810000 0x68>;
> > +                       pinctrl-names   = "default";
> > +                        pinctrl-0      = <&pinctrl_pwm0_chan0_default>;
> del space

Fixing, thanks.

> > +                       clock-names     = "pwm";
> > +                       clocks          = <&clk_sysin>;
> > +               };
> > +
> > +               /* SBC PWM Module */
> > +               pwm1: pwm@9510000 {
> > +                       compatible      = "st,sti-pwm";
> > +                       status          = "disabled";
> > +                       #pwm-cells      = <2>;
> > +                       reg             = <0x9510000 0x68>;
> > +                       pinctrl-names   = "default";
> > +                       pinctrl-0       = <&pinctrl_pwm1_chan0_default
> > +                                       &pinctrl_pwm1_chan1_default
> > +                                       &pinctrl_pwm1_chan2_default
> > +                                       &pinctrl_pwm1_chan3_default>;
> > +                       clock-names     = "pwm";
> > +                       clocks          = <&clk_sysin>;
> > +               };
> >         };
> >  };
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 1/7] ARM: stih407: Add DT nodes for for PWM

[Lee Jones]

ARM: stih407: Add DT nodes for for PWM

Supply top level nodes for the STiH407 based development boards.
The Pinctrl configuration has already been applied, so the only
missing piece of the DT puzzle is for a board's DTB to enable
the nodes.

Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/arch/arm/boot/dts/stih407.dtsi b/arch/arm/boot/dts/stih407.dtsi
index 4f9024f..e9766a4 100644
--- a/arch/arm/boot/dts/stih407.dtsi
+++ b/arch/arm/boot/dts/stih407.dtsi
@@ -259,5 +259,32 @@
 
 			status = "disabled";
 		};
+
+		/* COMMS PWM Module */
+		pwm0: pwm@9810000 {
+			compatible	= "st,sti-pwm";
+			status		= "disabled";
+			#pwm-cells	= <2>;
+			reg		= <0x9810000 0x68>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_pwm0_chan0_default>;
+			clock-names	= "pwm";
+			clocks		= <&clk_sysin>;
+		};
+
+		/* SBC PWM Module */
+		pwm1: pwm@9510000 {
+			compatible	= "st,sti-pwm";
+			status		= "disabled";
+			#pwm-cells	= <2>;
+			reg		= <0x9510000 0x68>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_pwm1_chan0_default
+					&pinctrl_pwm1_chan1_default
+					&pinctrl_pwm1_chan2_default
+					&pinctrl_pwm1_chan3_default>;
+			clock-names	= "pwm";
+			clocks		= <&clk_sysin>;
+		};
 	};
 };

Re: [STLinux Kernel] [PATCH 1/7] ARM: stih407: Add DT nodes for for PWM

[Peter Griffin]

Hi Lee,

> +		/* SBC PWM Module */
> +		pwm1: pwm@9510000 {
> +			compatible	= "st,sti-pwm";
> +			status		= "disabled";
> +			#pwm-cells	= <2>;
> +			reg		= <0x9510000 0x68>;
> +			pinctrl-names	= "default";
> +		        pinctrl-0	= <&pinctrl_pwm1_chan0_default
> +					&pinctrl_pwm1_chan1_default
> +					&pinctrl_pwm1_chan2_default
> +					&pinctrl_pwm1_chan3_default>;

If you're passing pin config for 4 channels, I think you should also be using the 
'st,pwm-num-chan = <4>' property like the doc example in patch 7.

After that: 
Acked-by: Peter Griffin <peter.griffin@linaro.org>

regards,

Peter.