Patchwork [8/9] ARM i.MX53: Add pwms to dtsi

login
register
mail settings
Submitter Sascha Hauer
Date Aug. 28, 2012, 11:48 a.m.
Message ID <1346154504-5623-9-git-send-email-s.hauer@pengutronix.de>
Download mbox | patch
Permalink /patch/180446/
State New
Headers show

Comments

Sascha Hauer - Aug. 28, 2012, 11:48 a.m.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Conflicts:
	arch/arm/mach-imx/clk-imx51-imx53.c
---
 arch/arm/boot/dts/imx53.dtsi        |   14 ++++++++++++++
 arch/arm/mach-imx/clk-imx51-imx53.c |    4 ++++
 2 files changed, 18 insertions(+)
Shawn Guo - Aug. 30, 2012, 10:32 p.m.
On Tue, Aug 28, 2012 at 01:48:23PM +0200, Sascha Hauer wrote:
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Conflicts:
> 	arch/arm/mach-imx/clk-imx51-imx53.c

Yeah, I know you have sorted out conflicts :)

> ---
>  arch/arm/boot/dts/imx53.dtsi        |   14 ++++++++++++++
>  arch/arm/mach-imx/clk-imx51-imx53.c |    4 ++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
> index cd37165..7ec17e4 100644
> --- a/arch/arm/boot/dts/imx53.dtsi
> +++ b/arch/arm/boot/dts/imx53.dtsi
> @@ -189,6 +189,20 @@
>  				status = "disabled";
>  			};
>  
> +			pwm1: pwm@53fb4000 {
> +				#pwm-cells = <3>;

pwm-cells should be 2?

> +				compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> +				reg = <0x53fb4000 0x4000>;
> +				interrupts = <61>;
> +			};
> +
> +			pwm2: pwm@53fb8000 {
> +				#pwm-cells = <3>;
> +				compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> +				reg = <0x53fb8000 0x4000>;
> +				interrupts = <94>;
> +			};
> +
>  			uart1: serial@53fbc000 {
>  				compatible = "fsl,imx53-uart", "fsl,imx21-uart";
>  				reg = <0x53fbc000 0x4000>;
> diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
> index 4bdcaa9..b522411 100644
> --- a/arch/arm/mach-imx/clk-imx51-imx53.c
> +++ b/arch/arm/mach-imx/clk-imx51-imx53.c
> @@ -455,6 +455,10 @@ int __init mx53_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
>  	clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "63fcc000.ssi");
>  	clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "50014000.ssi");
>  	clk_register_clkdev(clk[ssi3_ipg_gate], NULL, "63fd0000.ssi");
> +	clk_register_clkdev(clk[pwm1_ipg_gate], "ipg", "53fb4000.pwm");
> +	clk_register_clkdev(clk[pwm1_hf_gate], "per", "53fb4000.pwm");
> +	clk_register_clkdev(clk[pwm2_ipg_gate], "ipg", "53fb8000.pwm");
> +	clk_register_clkdev(clk[pwm2_hf_gate], "per", "53fb8000.pwm");

It should be in a separate patch?

>  
>  	/* set SDHC root clock to 200MHZ*/
>  	clk_set_rate(clk[esdhc_a_podf], 200000000);
> -- 
> 1.7.10.4
>
Shawn Guo - Aug. 31, 2012, 12:16 a.m.
On Fri, Aug 31, 2012 at 03:07:23PM +0200, Sascha Hauer wrote:
> > >  	clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "63fcc000.ssi");
> > >  	clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "50014000.ssi");
> > >  	clk_register_clkdev(clk[ssi3_ipg_gate], NULL, "63fd0000.ssi");
> > > +	clk_register_clkdev(clk[pwm1_ipg_gate], "ipg", "53fb4000.pwm");
> > > +	clk_register_clkdev(clk[pwm1_hf_gate], "per", "53fb4000.pwm");
> > > +	clk_register_clkdev(clk[pwm2_ipg_gate], "ipg", "53fb8000.pwm");
> > > +	clk_register_clkdev(clk[pwm2_hf_gate], "per", "53fb8000.pwm");
> > 
> > It should be in a separate patch?
> 
> Should it? Surely I can do this, I had the feeling though that it
> belongs together.
> 
>From patch subject, I do not expect these changes in the patch.  It's
okay to have it but we need a more proper patch subject then.
Sascha Hauer - Aug. 31, 2012, 1:07 p.m.
On Fri, Aug 31, 2012 at 06:32:20AM +0800, Shawn Guo wrote:
> On Tue, Aug 28, 2012 at 01:48:23PM +0200, Sascha Hauer wrote:
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > Conflicts:
> > 	arch/arm/mach-imx/clk-imx51-imx53.c
> 
> Yeah, I know you have sorted out conflicts :)

:)

> 
> > ---
> >  arch/arm/boot/dts/imx53.dtsi        |   14 ++++++++++++++
> >  arch/arm/mach-imx/clk-imx51-imx53.c |    4 ++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
> > index cd37165..7ec17e4 100644
> > --- a/arch/arm/boot/dts/imx53.dtsi
> > +++ b/arch/arm/boot/dts/imx53.dtsi
> > @@ -189,6 +189,20 @@
> >  				status = "disabled";
> >  			};
> >  
> > +			pwm1: pwm@53fb4000 {
> > +				#pwm-cells = <3>;
> 
> pwm-cells should be 2?

Yes, right. We have a patch internally that allows us to pass a
'inverted' flag to the pwm, hence I accidently have 3 here.

> >  	clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "63fcc000.ssi");
> >  	clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "50014000.ssi");
> >  	clk_register_clkdev(clk[ssi3_ipg_gate], NULL, "63fd0000.ssi");
> > +	clk_register_clkdev(clk[pwm1_ipg_gate], "ipg", "53fb4000.pwm");
> > +	clk_register_clkdev(clk[pwm1_hf_gate], "per", "53fb4000.pwm");
> > +	clk_register_clkdev(clk[pwm2_ipg_gate], "ipg", "53fb8000.pwm");
> > +	clk_register_clkdev(clk[pwm2_hf_gate], "per", "53fb8000.pwm");
> 
> It should be in a separate patch?

Should it? Surely I can do this, I had the feeling though that it
belongs together.

Sascha
Thierry Reding - Sept. 7, 2012, 1:29 p.m.
On Fri, Aug 31, 2012 at 03:07:23PM +0200, Sascha Hauer wrote:
> On Fri, Aug 31, 2012 at 06:32:20AM +0800, Shawn Guo wrote:
> > On Tue, Aug 28, 2012 at 01:48:23PM +0200, Sascha Hauer wrote:
[...]
> > >  arch/arm/boot/dts/imx53.dtsi        |   14 ++++++++++++++
> > >  arch/arm/mach-imx/clk-imx51-imx53.c |    4 ++++
> > >  2 files changed, 18 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
> > > index cd37165..7ec17e4 100644
> > > --- a/arch/arm/boot/dts/imx53.dtsi
> > > +++ b/arch/arm/boot/dts/imx53.dtsi
> > > @@ -189,6 +189,20 @@
> > >  				status = "disabled";
> > >  			};
> > >  
> > > +			pwm1: pwm@53fb4000 {
> > > +				#pwm-cells = <3>;
> > 
> > pwm-cells should be 2?
> 
> Yes, right. We have a patch internally that allows us to pass a
> 'inverted' flag to the pwm, hence I accidently have 3 here.

There are patches in for-next that add support for setting the PWM
polarity, though there's currently no support for specifying it via a
third cell in the specifier. Would you mind sharing the patches that add
this?

Thierry
Sascha Hauer - Sept. 7, 2012, 5:26 p.m.
On Fri, Sep 07, 2012 at 03:29:55PM +0200, Thierry Reding wrote:
> On Fri, Aug 31, 2012 at 03:07:23PM +0200, Sascha Hauer wrote:
> > On Fri, Aug 31, 2012 at 06:32:20AM +0800, Shawn Guo wrote:
> > > On Tue, Aug 28, 2012 at 01:48:23PM +0200, Sascha Hauer wrote:
> [...]
> > > >  arch/arm/boot/dts/imx53.dtsi        |   14 ++++++++++++++
> > > >  arch/arm/mach-imx/clk-imx51-imx53.c |    4 ++++
> > > >  2 files changed, 18 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
> > > > index cd37165..7ec17e4 100644
> > > > --- a/arch/arm/boot/dts/imx53.dtsi
> > > > +++ b/arch/arm/boot/dts/imx53.dtsi
> > > > @@ -189,6 +189,20 @@
> > > >  				status = "disabled";
> > > >  			};
> > > >  
> > > > +			pwm1: pwm@53fb4000 {
> > > > +				#pwm-cells = <3>;
> > > 
> > > pwm-cells should be 2?
> > 
> > Yes, right. We have a patch internally that allows us to pass a
> > 'inverted' flag to the pwm, hence I accidently have 3 here.
> 
> There are patches in for-next that add support for setting the PWM
> polarity, though there's currently no support for specifying it via a
> third cell in the specifier. Would you mind sharing the patches that add
> this?

Yes, will do. I was afraid this leads to some discussion, so I skipped
them so far.

The basic idea was that the third cell is for flags from which bit0 set
means 'inverted'. We currently implemented this i.MX specific, but if
you think this is acceptable it's propably a good idea to implement this
in a generic manner.

Sascha
Thierry Reding - Sept. 7, 2012, 8:10 p.m.
On Fri, Sep 07, 2012 at 07:26:12PM +0200, Sascha Hauer wrote:
> On Fri, Sep 07, 2012 at 03:29:55PM +0200, Thierry Reding wrote:
> > On Fri, Aug 31, 2012 at 03:07:23PM +0200, Sascha Hauer wrote:
> > > On Fri, Aug 31, 2012 at 06:32:20AM +0800, Shawn Guo wrote:
> > > > On Tue, Aug 28, 2012 at 01:48:23PM +0200, Sascha Hauer wrote:
> > [...]
> > > > >  arch/arm/boot/dts/imx53.dtsi        |   14 ++++++++++++++
> > > > >  arch/arm/mach-imx/clk-imx51-imx53.c |    4 ++++
> > > > >  2 files changed, 18 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
> > > > > index cd37165..7ec17e4 100644
> > > > > --- a/arch/arm/boot/dts/imx53.dtsi
> > > > > +++ b/arch/arm/boot/dts/imx53.dtsi
> > > > > @@ -189,6 +189,20 @@
> > > > >  				status = "disabled";
> > > > >  			};
> > > > >  
> > > > > +			pwm1: pwm@53fb4000 {
> > > > > +				#pwm-cells = <3>;
> > > > 
> > > > pwm-cells should be 2?
> > > 
> > > Yes, right. We have a patch internally that allows us to pass a
> > > 'inverted' flag to the pwm, hence I accidently have 3 here.
> > 
> > There are patches in for-next that add support for setting the PWM
> > polarity, though there's currently no support for specifying it via a
> > third cell in the specifier. Would you mind sharing the patches that add
> > this?
> 
> Yes, will do. I was afraid this leads to some discussion, so I skipped
> them so far.
> 
> The basic idea was that the third cell is for flags from which bit0 set
> means 'inverted'. We currently implemented this i.MX specific, but if
> you think this is acceptable it's propably a good idea to implement this
> in a generic manner.

Absolutely. That's exactly what I had in mind as well, so if you have
the code ready I don't have to write it myself.

Thierry

Patch

diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index cd37165..7ec17e4 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -189,6 +189,20 @@ 
 				status = "disabled";
 			};
 
+			pwm1: pwm@53fb4000 {
+				#pwm-cells = <3>;
+				compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
+				reg = <0x53fb4000 0x4000>;
+				interrupts = <61>;
+			};
+
+			pwm2: pwm@53fb8000 {
+				#pwm-cells = <3>;
+				compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
+				reg = <0x53fb8000 0x4000>;
+				interrupts = <94>;
+			};
+
 			uart1: serial@53fbc000 {
 				compatible = "fsl,imx53-uart", "fsl,imx21-uart";
 				reg = <0x53fbc000 0x4000>;
diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
index 4bdcaa9..b522411 100644
--- a/arch/arm/mach-imx/clk-imx51-imx53.c
+++ b/arch/arm/mach-imx/clk-imx51-imx53.c
@@ -455,6 +455,10 @@  int __init mx53_clocks_init(unsigned long rate_ckil, unsigned long rate_osc,
 	clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "63fcc000.ssi");
 	clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "50014000.ssi");
 	clk_register_clkdev(clk[ssi3_ipg_gate], NULL, "63fd0000.ssi");
+	clk_register_clkdev(clk[pwm1_ipg_gate], "ipg", "53fb4000.pwm");
+	clk_register_clkdev(clk[pwm1_hf_gate], "per", "53fb4000.pwm");
+	clk_register_clkdev(clk[pwm2_ipg_gate], "ipg", "53fb8000.pwm");
+	clk_register_clkdev(clk[pwm2_hf_gate], "per", "53fb8000.pwm");
 
 	/* set SDHC root clock to 200MHZ*/
 	clk_set_rate(clk[esdhc_a_podf], 200000000);