Patchwork [10/11] ARM: imx6q_sabrelite: clkdev_add cko1 for sgtl5000

login
register
mail settings
Submitter Richard Zhao
Date April 27, 2012, 7:03 a.m.
Message ID <1335510185-7906-11-git-send-email-richard.zhao@freescale.com>
Download mbox | patch
Permalink /patch/155393/
State New
Headers show

Comments

Richard Zhao - April 27, 2012, 7:03 a.m.
Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 arch/arm/mach-imx/clk-imx6q.c  |    3 +++
 arch/arm/mach-imx/mach-imx6q.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)
Shawn Guo - May 1, 2012, 12:47 p.m.
On Fri, Apr 27, 2012 at 03:03:04PM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  arch/arm/mach-imx/clk-imx6q.c  |    3 +++
>  arch/arm/mach-imx/mach-imx6q.c |   28 ++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
> index 9a03dcc..4ea0de0 100644
> --- a/arch/arm/mach-imx/clk-imx6q.c
> +++ b/arch/arm/mach-imx/clk-imx6q.c
> @@ -419,6 +419,9 @@ int __init mx6q_clocks_init(void)
>  	clk_register_clkdev(clk[dummy], NULL, "20bc000.wdog");
>  	clk_register_clkdev(clk[dummy], NULL, "20c0000.wdog");
>  	clk_register_clkdev(clk[ssi1], NULL, "2028000.ssi");
> +	clk_register_clkdev(clk[cko1_sel], "cko1_sel", NULL);
> +	clk_register_clkdev(clk[ahb], "ahb", NULL);
> +	clk_register_clkdev(clk[cko1], "cko1", NULL);
>  
>  	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++) {
>  		c = clk_get_sys(clks_init_on[i], NULL);
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index d25c5d8..e9b2522 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -10,6 +10,8 @@
>   * http://www.gnu.org/copyleft/gpl.html
>   */
>  
> +#include <linux/clkdev.h>
> +#include <linux/clk.h>

Nit: I would have <linux/clk.h> put before <linux/clkdev.h>.

>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> @@ -75,10 +77,36 @@ static int ksz9021rn_phy_fixup(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static void __init imx6q_cko1_setup(void)

I would say this is a sabrelite specific setup so far.

> +{
> +	struct clk *cko1_sel, *ahb, *cko1;
> +	unsigned long rate;
> +
> +	cko1_sel = clk_get_sys(NULL, "cko1_sel");
> +	ahb = clk_get_sys(NULL, "ahb");
> +	cko1 = clk_get_sys(NULL, "cko1");
> +	if (IS_ERR(cko1_sel) || IS_ERR(ahb) || IS_ERR(cko1)) {
> +		printk(KERN_ERR "cko1 setup failed!\n");

pr_err

> +		goto put_clk;
> +	}
> +	clk_set_parent(cko1_sel, ahb);
> +	rate = clk_round_rate(cko1, 16000000);
> +	clk_set_rate(cko1, rate);
> +	clk_register_clkdev(cko1, NULL, "0-000a");

This dev_id looks a little strange to me.  I understand that's the
consequence of having sgtl5000 clock managed in ASoC machine driver
imx-sgtl5000.  IMO, having sgtl5000 driver manages its clock and
looking up the clock with dev_id simply as "sgtl5000" makes more
sense to me.  Will put more comment on your imx-sgtl5000 clock patch.

Regards,
Shawn

> +put_clk:
> +	if (!IS_ERR(cko1_sel))
> +		clk_put(cko1_sel);
> +	if (!IS_ERR(ahb))
> +		clk_put(ahb);
> +	if (!IS_ERR(cko1))
> +		clk_put(cko1);
> +}
> +
>  static void __init imx6q_sabrelite_init(void)
>  {
>  	phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
>  				ksz9021rn_phy_fixup);
> +	imx6q_cko1_setup();
>  }
>  
>  static void __init imx6q_init_machine(void)
> -- 
> 1.7.5.4
> 
>
Shawn Guo - May 1, 2012, 11:39 p.m.
On Tue, May 01, 2012 at 08:47:10PM +0800, Shawn Guo wrote:
> > +	clk_register_clkdev(cko1, NULL, "0-000a");
> 
> This dev_id looks a little strange to me.  I understand that's the
> consequence of having sgtl5000 clock managed in ASoC machine driver
> imx-sgtl5000.
> 
Just realised it's not the case.  But shouldn't the name look like
"sgtl5000.0-000a"?
Fabio Estevam - May 2, 2012, 2:47 a.m.
On Tue, May 1, 2012 at 8:39 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, May 01, 2012 at 08:47:10PM +0800, Shawn Guo wrote:
>> > +   clk_register_clkdev(cko1, NULL, "0-000a");
>>
>> This dev_id looks a little strange to me.  I understand that's the
>> consequence of having sgtl5000 clock managed in ASoC machine driver
>> imx-sgtl5000.
>>
> Just realised it's not the case.  But shouldn't the name look like
> "sgtl5000.0-000a"?

Since this is common code, we should not hardcode the I2C bus as the
codec can be connected in a I2C bus different of 0.
Shawn Guo - May 2, 2012, 3:37 a.m.
On Tue, May 01, 2012 at 11:47:55PM -0300, Fabio Estevam wrote:
> On Tue, May 1, 2012 at 8:39 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > On Tue, May 01, 2012 at 08:47:10PM +0800, Shawn Guo wrote:
> >> > +   clk_register_clkdev(cko1, NULL, "0-000a");
> >>
> >> This dev_id looks a little strange to me.  I understand that's the
> >> consequence of having sgtl5000 clock managed in ASoC machine driver
> >> imx-sgtl5000.
> >>
> > Just realised it's not the case.  But shouldn't the name look like
> > "sgtl5000.0-000a"?
> 
> Since this is common code, we should not hardcode the I2C bus as the
> codec can be connected in a I2C bus different of 0.

No, it's not.  It's a sabrelite specific lookup which is done in
imx6q_sabrelite_init, while imx6q_cko1_setup needs a better naming,
as what I already commented.

In the end, this type of board specific clock lookup should really
go into device tree.
Richard Zhao - May 2, 2012, 11 a.m.
On Tue, May 01, 2012 at 08:47:10PM +0800, Shawn Guo wrote:
> On Fri, Apr 27, 2012 at 03:03:04PM +0800, Richard Zhao wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> >  arch/arm/mach-imx/clk-imx6q.c  |    3 +++
> >  arch/arm/mach-imx/mach-imx6q.c |   28 ++++++++++++++++++++++++++++
> >  2 files changed, 31 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
> > index 9a03dcc..4ea0de0 100644
> > --- a/arch/arm/mach-imx/clk-imx6q.c
> > +++ b/arch/arm/mach-imx/clk-imx6q.c
> > @@ -419,6 +419,9 @@ int __init mx6q_clocks_init(void)
> >  	clk_register_clkdev(clk[dummy], NULL, "20bc000.wdog");
> >  	clk_register_clkdev(clk[dummy], NULL, "20c0000.wdog");
> >  	clk_register_clkdev(clk[ssi1], NULL, "2028000.ssi");
> > +	clk_register_clkdev(clk[cko1_sel], "cko1_sel", NULL);
> > +	clk_register_clkdev(clk[ahb], "ahb", NULL);
> > +	clk_register_clkdev(clk[cko1], "cko1", NULL);
> >  
> >  	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++) {
> >  		c = clk_get_sys(clks_init_on[i], NULL);
> > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> > index d25c5d8..e9b2522 100644
> > --- a/arch/arm/mach-imx/mach-imx6q.c
> > +++ b/arch/arm/mach-imx/mach-imx6q.c
> > @@ -10,6 +10,8 @@
> >   * http://www.gnu.org/copyleft/gpl.html
> >   */
> >  
> > +#include <linux/clkdev.h>
> > +#include <linux/clk.h>
> 
> Nit: I would have <linux/clk.h> put before <linux/clkdev.h>.
ok
> 
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> >  #include <linux/io.h>
> > @@ -75,10 +77,36 @@ static int ksz9021rn_phy_fixup(struct phy_device *phydev)
> >  	return 0;
> >  }
> >  
> > +static void __init imx6q_cko1_setup(void)
> 
> I would say this is a sabrelite specific setup so far.
ok
> 
> > +{
> > +	struct clk *cko1_sel, *ahb, *cko1;
> > +	unsigned long rate;
> > +
> > +	cko1_sel = clk_get_sys(NULL, "cko1_sel");
> > +	ahb = clk_get_sys(NULL, "ahb");
> > +	cko1 = clk_get_sys(NULL, "cko1");
> > +	if (IS_ERR(cko1_sel) || IS_ERR(ahb) || IS_ERR(cko1)) {
> > +		printk(KERN_ERR "cko1 setup failed!\n");
> 
> pr_err
ok

Thanks
Richard
> 
> > +		goto put_clk;
> > +	}
> > +	clk_set_parent(cko1_sel, ahb);
> > +	rate = clk_round_rate(cko1, 16000000);
> > +	clk_set_rate(cko1, rate);
> > +	clk_register_clkdev(cko1, NULL, "0-000a");
> 
> This dev_id looks a little strange to me.  I understand that's the
> consequence of having sgtl5000 clock managed in ASoC machine driver
> imx-sgtl5000.  IMO, having sgtl5000 driver manages its clock and
> looking up the clock with dev_id simply as "sgtl5000" makes more
> sense to me.  Will put more comment on your imx-sgtl5000 clock patch.
> 
> Regards,
> Shawn
> 
> > +put_clk:
> > +	if (!IS_ERR(cko1_sel))
> > +		clk_put(cko1_sel);
> > +	if (!IS_ERR(ahb))
> > +		clk_put(ahb);
> > +	if (!IS_ERR(cko1))
> > +		clk_put(cko1);
> > +}
> > +
> >  static void __init imx6q_sabrelite_init(void)
> >  {
> >  	phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
> >  				ksz9021rn_phy_fixup);
> > +	imx6q_cko1_setup();
> >  }
> >  
> >  static void __init imx6q_init_machine(void)
> > -- 
> > 1.7.5.4
> > 
> > 
>

Patch

diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index 9a03dcc..4ea0de0 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -419,6 +419,9 @@  int __init mx6q_clocks_init(void)
 	clk_register_clkdev(clk[dummy], NULL, "20bc000.wdog");
 	clk_register_clkdev(clk[dummy], NULL, "20c0000.wdog");
 	clk_register_clkdev(clk[ssi1], NULL, "2028000.ssi");
+	clk_register_clkdev(clk[cko1_sel], "cko1_sel", NULL);
+	clk_register_clkdev(clk[ahb], "ahb", NULL);
+	clk_register_clkdev(clk[cko1], "cko1", NULL);
 
 	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++) {
 		c = clk_get_sys(clks_init_on[i], NULL);
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index d25c5d8..e9b2522 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -10,6 +10,8 @@ 
  * http://www.gnu.org/copyleft/gpl.html
  */
 
+#include <linux/clkdev.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -75,10 +77,36 @@  static int ksz9021rn_phy_fixup(struct phy_device *phydev)
 	return 0;
 }
 
+static void __init imx6q_cko1_setup(void)
+{
+	struct clk *cko1_sel, *ahb, *cko1;
+	unsigned long rate;
+
+	cko1_sel = clk_get_sys(NULL, "cko1_sel");
+	ahb = clk_get_sys(NULL, "ahb");
+	cko1 = clk_get_sys(NULL, "cko1");
+	if (IS_ERR(cko1_sel) || IS_ERR(ahb) || IS_ERR(cko1)) {
+		printk(KERN_ERR "cko1 setup failed!\n");
+		goto put_clk;
+	}
+	clk_set_parent(cko1_sel, ahb);
+	rate = clk_round_rate(cko1, 16000000);
+	clk_set_rate(cko1, rate);
+	clk_register_clkdev(cko1, NULL, "0-000a");
+put_clk:
+	if (!IS_ERR(cko1_sel))
+		clk_put(cko1_sel);
+	if (!IS_ERR(ahb))
+		clk_put(ahb);
+	if (!IS_ERR(cko1))
+		clk_put(cko1);
+}
+
 static void __init imx6q_sabrelite_init(void)
 {
 	phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
 				ksz9021rn_phy_fixup);
+	imx6q_cko1_setup();
 }
 
 static void __init imx6q_init_machine(void)