| Message ID | 1370642870-8051-1-git-send-email-festevam@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 07, 2013 at 07:07:50PM -0300, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > In order sgtl5000 driver to probe successfully, we need to read its ID via I2C > ,which requires that MCLK is driven prior to the I2C access. > > Otherwise we get the following probe error: > > sgtl5000: probe of 0-000a failed with error -5 > imx-sgtl5000 sound.13: ASoC: CODEC (null) not registered > imx-sgtl5000 sound.13: snd_soc_register_card failed (-517) > platform sound.13: Driver imx-sgtl5000 requests probe deferral > > Turn on MCLK sooner so that the probe can succeed. Umm... doesn't that mean that imx-sgtl5000 should be getting this clock and turning it on itself, and disabling it on device removal?
On 6/7/2013 4:16 PM, Fabio Estevam wrote: > Hi Russell, > > On Fri, Jun 7, 2013 at 7:20 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > >> Umm... doesn't that mean that imx-sgtl5000 should be getting this clock >> and turning it on itself, and disabling it on device removal? > Yes, you are right. > > Actually imx-sgtl500 does get this clock and turn it on itself. > > The problem is that imx-sgtl5000 is getting called after the codec > sgtl500 codec is probed. > > ***** reading codec ID > sgtl5000: probe of 0-000a failed with error -5 > ***** enabling the codec clock > imx-sgtl5000 sound.13: ASoC: CODEC (null) not registered > imx-sgtl5000 sound.13: snd_soc_register_card failed (-517) > platform sound.13: Driver imx-sgtl5000 requests probe deferral > > > And then If I do: > > --- a/sound/soc/codecs/sgtl5000.c > +++ b/sound/soc/codecs/sgtl5000.c > @@ -1525,7 +1525,7 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, > /* read chip information */ > ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, ®); > if (ret) > - return ret; > + return -EPROBE_DEFER; > > if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) != > SGTL5000_PARTID_PART_ID) { > > Then the probe is succesful on the second attemp. > > I will post this to the alsa-devel list. > > Thanks, > > Fabio Estevam > Perhaps a delay is needed after the clock is turned on, to when you can read a register. Using -EPROBE_DEFER seems like an abuse. Shouldn't it turn the clock back off before it returns anyway? Troy
Hi Troy, On Fri, Jun 7, 2013 at 8:43 PM, Troy Kisky <troy.kisky@boundarydevices.com> wrote: > Perhaps a delay is needed after the clock is turned on, to when you can read > a register. The problem is that in linux-next the reading of the codec happens before the clock is turned on and I was not able to understand yet why this is happening . On Linus' tree the codec reading only happens after the codec clock is on. > Using -EPROBE_DEFER seems like an abuse. Shouldn't it turn the clock back > off before it returns anyway? That was done just for testing purpose. Regards, Fabio Estevam
On 6/7/2013 4:43 PM, Troy Kisky wrote: > On 6/7/2013 4:16 PM, Fabio Estevam wrote: >> Hi Russell, >> >> On Fri, Jun 7, 2013 at 7:20 PM, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> >>> Umm... doesn't that mean that imx-sgtl5000 should be getting this clock >>> and turning it on itself, and disabling it on device removal? >> Yes, you are right. >> >> Actually imx-sgtl500 does get this clock and turn it on itself. >> >> The problem is that imx-sgtl5000 is getting called after the codec >> sgtl500 codec is probed. >> >> ***** reading codec ID >> sgtl5000: probe of 0-000a failed with error -5 >> ***** enabling the codec clock >> imx-sgtl5000 sound.13: ASoC: CODEC (null) not registered >> imx-sgtl5000 sound.13: snd_soc_register_card failed (-517) >> platform sound.13: Driver imx-sgtl5000 requests probe deferral >> >> >> And then If I do: >> >> --- a/sound/soc/codecs/sgtl5000.c >> +++ b/sound/soc/codecs/sgtl5000.c >> @@ -1525,7 +1525,7 @@ static int sgtl5000_i2c_probe(struct i2c_client >> *client, >> /* read chip information */ >> ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, ®); >> if (ret) >> - return ret; >> + return -EPROBE_DEFER; >> >> if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) != >> SGTL5000_PARTID_PART_ID) { >> >> Then the probe is succesful on the second attemp. >> >> I will post this to the alsa-devel list. >> >> Thanks, >> >> Fabio Estevam >> > > Perhaps a delay is needed after the clock is turned on, to when you > can read a register. > > Using -EPROBE_DEFER seems like an abuse. Shouldn't it turn the clock > back off before it returns anyway? > > Troy > Sorry, I read that wrong. Maybe sgtl5000 needs to get the clock, as well as imx-sgtl5000. Troy
On Fri, Jun 07, 2013 at 04:43:12PM -0700, Troy Kisky wrote: > Using -EPROBE_DEFER seems like an abuse. Shouldn't it turn the clock > back off before it returns anyway? Yes.
On Fri, Jun 07, 2013 at 04:49:17PM -0700, Troy Kisky wrote: > Sorry, I read that wrong. Maybe sgtl5000 needs to get the clock, as > well as imx-sgtl5000. Yes, in general we should be able to use the clock API more but it's not present on most platforms so it's hard to use in generic code like CODEC drivers. This is why I keep pushing to get the generic clock API enabled on all platforms (which unfortunately nobody seems really interested in).
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index 045e5e3..034f4d2 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -118,6 +118,7 @@ static void __init imx6q_sabrelite_cko1_setup(void) { struct clk *cko1_sel, *ahb, *cko1; unsigned long rate; + int ret; cko1_sel = clk_get_sys(NULL, "cko1_sel"); ahb = clk_get_sys(NULL, "ahb"); @@ -129,6 +130,9 @@ static void __init imx6q_sabrelite_cko1_setup(void) clk_set_parent(cko1_sel, ahb); rate = clk_round_rate(cko1, 16000000); clk_set_rate(cko1, rate); + ret = clk_prepare_enable(cko1); + if (ret) + pr_err("enabling clko1 failed: %d", ret); put_clk: if (!IS_ERR(cko1_sel)) clk_put(cko1_sel);