| Message ID | 1372666571-17276-2-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 01, 2013 at 04:16:08PM +0800, Shawn Guo wrote: > + /* > + * It seems that sgtl5000 takes a longer time to get the registers > + * ready for write than bread. Let's give it a ramping time before > + * the first write goes. > + */ > + msleep(50); > + > /* Ensure sgtl5000 will start with sane register values */ > ret = sgtl5000_fill_defaults(sgtl5000); > if (ret) This seems like a really odd place to add the sleep - I'd have expected this to be a part of or just after the reset operation. It's a *really* long sleep too, though if that's what you need that's what you need. Also "bread" :)
Dear Mark Brown, > On Mon, Jul 01, 2013 at 04:16:08PM +0800, Shawn Guo wrote: > > + /* > > + * It seems that sgtl5000 takes a longer time to get the registers > > + * ready for write than bread. Let's give it a ramping time before > > + * the first write goes. > > + */ > > + msleep(50); > > + > > > > /* Ensure sgtl5000 will start with sane register values */ > > ret = sgtl5000_fill_defaults(sgtl5000); > > if (ret) > > This seems like a really odd place to add the sleep - I'd have expected > this to be a part of or just after the reset operation. It's a *really* > long sleep too, though if that's what you need that's what you need. The delay might also be caused by the I2C driver, I have this in my ToDo to check, but I didn't get to it yet. On the other hand, if subsequent IO does work, then it's unlikely to be the case. > Also "bread" :) Of course, sometimes you want to put a lot of things on it, like bacon and such. Best regards, Marek Vasut
Shawn Guo writes: > Since commit af8ee11 (ASoC: sgtl5000: Fix driver probe after reset), > it's very ofen to run into the following probe error on imx28. It is > caused by the regmap_write() failure in sgtl5000_fill_defaults(). > However, the regmap_read() before this has already been working. It > seems that sgtl5000 takes a longer ramping time to get the registers > ready for write than read. > > [ 1.991579] sgtl5000 0-000a: sgtl5000 revision 0x11 > [ 2.021655] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered > [ 2.039087] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) > [ 2.046299] platform sound.12: Driver mxs-sgtl5000 requests probe deferral > > Fix the regression by giving it a ramping time before the first write. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > sound/soc/codecs/sgtl5000.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c > index d441559..20bca03 100644 > --- a/sound/soc/codecs/sgtl5000.c > +++ b/sound/soc/codecs/sgtl5000.c > @@ -1552,6 +1552,13 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, > > i2c_set_clientdata(client, sgtl5000); > > + /* > + * It seems that sgtl5000 takes a longer time to get the registers > + * ready for write than bread. Let's give it a ramping time before ^^^^^ > What sort of bread? ;-) Lothar Waßmann
On Mon, Jul 01, 2013 at 12:54:18PM +0200, Marek Vasut wrote: > Dear Mark Brown, > > > On Mon, Jul 01, 2013 at 04:16:08PM +0800, Shawn Guo wrote: > > > + /* > > > + * It seems that sgtl5000 takes a longer time to get the registers > > > + * ready for write than bread. Let's give it a ramping time before > > > + * the first write goes. > > > + */ > > > + msleep(50); > > > + > > > > > > /* Ensure sgtl5000 will start with sane register values */ > > > ret = sgtl5000_fill_defaults(sgtl5000); > > > if (ret) > > > > This seems like a really odd place to add the sleep - I'd have expected > > this to be a part of or just after the reset operation. It's a *really* > > long sleep too, though if that's what you need that's what you need. > > The delay might also be caused by the I2C driver, I have this in my ToDo to > check, but I didn't get to it yet. On the other hand, if subsequent IO does > work, then it's unlikely to be the case. Right, since read is already working there. Shawn
On Mon, Jul 01, 2013 at 11:07:10AM +0100, Mark Brown wrote: > On Mon, Jul 01, 2013 at 04:16:08PM +0800, Shawn Guo wrote: > > > + /* > > + * It seems that sgtl5000 takes a longer time to get the registers > > + * ready for write than bread. Let's give it a ramping time before > > + * the first write goes. > > + */ > > + msleep(50); > > + > > /* Ensure sgtl5000 will start with sane register values */ > > ret = sgtl5000_fill_defaults(sgtl5000); > > if (ret) > > This seems like a really odd place to add the sleep - I'd have expected > this to be a part of or just after the reset operation. Since I do not see any reset operation between clk_prepare_enable() and sgtl5000_fill_defaults(), so I will move the sleep into sgtl5000_fill_defaults() call. > It's a *really* > long sleep too, though if that's what you need that's what you need. > Yeah, from my testing it's what we need. > Also "bread" :) Yeah, I was really hungry when writing the comment. Shawn
On Mon, Jul 1, 2013 at 7:54 AM, Marek Vasut <marex@denx.de> wrote:
> The delay might also be caused by the I2C driver, I have this in my ToDo to
Yes, this extra delay could be related to the mxs i2c driver, as we do
not need to add this delay when using sgtl5000 on other i.mx devices.
Other people have also reported mxs i2c issues currently:
http://marc.info/?l=linux-i2c&m=137241339917261&w=2
,so this may be related some how.
Regards,
Fabio Estevam
On Mon, Jul 01, 2013 at 11:12:48AM -0300, Fabio Estevam wrote: > On Mon, Jul 1, 2013 at 7:54 AM, Marek Vasut <marex@denx.de> wrote: > > > The delay might also be caused by the I2C driver, I have this in my ToDo to > > Yes, this extra delay could be related to the mxs i2c driver, as we do > not need to add this delay when using sgtl5000 on other i.mx devices. > > Other people have also reported mxs i2c issues currently: > http://marc.info/?l=linux-i2c&m=137241339917261&w=2 > > ,so this may be related some how. How does it explain the fact that sgtl5000 read has been working there? The sgtl5000 revision has been successfully read out at that point. Shawn
On Mon, Jul 01, 2013 at 10:26:12PM +0800, Shawn Guo wrote: > On Mon, Jul 01, 2013 at 11:12:48AM -0300, Fabio Estevam wrote: > > On Mon, Jul 1, 2013 at 7:54 AM, Marek Vasut <marex@denx.de> wrote: > > > > > The delay might also be caused by the I2C driver, I have this in my ToDo to > > > > Yes, this extra delay could be related to the mxs i2c driver, as we do > > not need to add this delay when using sgtl5000 on other i.mx devices. > > > > Other people have also reported mxs i2c issues currently: > > http://marc.info/?l=linux-i2c&m=137241339917261&w=2 > > > > ,so this may be related some how. > > How does it explain the fact that sgtl5000 read has been working there? > The sgtl5000 revision has been successfully read out at that point. And also why we do not see the impact of i2c issue on sgtl5000 before the offending commit af8ee11 (ASoC: sgtl5000: Fix driver probe after reset)? Shawn
On Mon, Jul 1, 2013 at 11:34 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > And also why we do not see the impact of i2c issue on sgtl5000 before > the offending commit af8ee11 (ASoC: sgtl5000: Fix driver probe after > reset)? Probably because the mxs i2c was working before this commit. Commit af8ee11 does fix the reset probe on mx51/mx53/mx6. It's only on mx28 that we have this issue. Also, please see this patch from Alexandre Belloni: https://lkml.org/lkml/2013/6/24/430 He can only connect the ADC chip if he uses I2C bitbang mode on mx28.
On Mon, Jul 01, 2013 at 12:11:58PM -0300, Fabio Estevam wrote: > On Mon, Jul 1, 2013 at 11:34 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > > And also why we do not see the impact of i2c issue on sgtl5000 before > > the offending commit af8ee11 (ASoC: sgtl5000: Fix driver probe after > > reset)? > > Probably because the mxs i2c was working before this commit. Do you have a commit id at which mxs i2c is known good? I would like to find out it's a mxs i2c issue or sgtl5000 problem. > > Commit af8ee11 does fix the reset probe on mx51/mx53/mx6. > > It's only on mx28 that we have this issue. > > Also, please see this patch from Alexandre Belloni: > https://lkml.org/lkml/2013/6/24/430 > > He can only connect the ADC chip if he uses I2C bitbang mode on mx28. Does I2C bitbang mode help fix the sgtl5000 issue here? Shawn
On Mon, Jul 1, 2013 at 12:33 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Mon, Jul 01, 2013 at 12:11:58PM -0300, Fabio Estevam wrote: >> On Mon, Jul 1, 2013 at 11:34 AM, Shawn Guo <shawn.guo@linaro.org> wrote: >> >> > And also why we do not see the impact of i2c issue on sgtl5000 before >> > the offending commit af8ee11 (ASoC: sgtl5000: Fix driver probe after >> > reset)? >> >> Probably because the mxs i2c was working before this commit. > > Do you have a commit id at which mxs i2c is known good? I would like to > find out it's a mxs i2c issue or sgtl5000 problem. I don't have it, but I am adding Alexandre in case he knows. Alexandre, Do you happen to know a commit id which does not show the mxs i2c timeouts you have been observing? > >> >> Commit af8ee11 does fix the reset probe on mx51/mx53/mx6. >> >> It's only on mx28 that we have this issue. >> >> Also, please see this patch from Alexandre Belloni: >> https://lkml.org/lkml/2013/6/24/430 >> >> He can only connect the ADC chip if he uses I2C bitbang mode on mx28. > > Does I2C bitbang mode help fix the sgtl5000 issue here? I haven't made this test.
Dear Fabio Estevam, > On Mon, Jul 1, 2013 at 12:33 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > > On Mon, Jul 01, 2013 at 12:11:58PM -0300, Fabio Estevam wrote: > >> On Mon, Jul 1, 2013 at 11:34 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > >> > And also why we do not see the impact of i2c issue on sgtl5000 before > >> > the offending commit af8ee11 (ASoC: sgtl5000: Fix driver probe after > >> > reset)? > >> > >> Probably because the mxs i2c was working before this commit. > > > > Do you have a commit id at which mxs i2c is known good? I would like to > > find out it's a mxs i2c issue or sgtl5000 problem. > > I don't have it, but I am adding Alexandre in case he knows. > > Alexandre, > > Do you happen to know a commit id which does not show the mxs i2c > timeouts you have been observing? I think you can just disable the PIO mode altogether (around line 500 ... if (msg->len < 8) ... replace this with if (0) ) and then the problem should not be there (if it's a PIO problem). Best regards, Marek Vasut
On 01/07/2013 17:42, Fabio Estevam wrote: > On Mon, Jul 1, 2013 at 12:33 PM, Shawn Guo <shawn.guo@linaro.org> wrote: >> On Mon, Jul 01, 2013 at 12:11:58PM -0300, Fabio Estevam wrote: >>> On Mon, Jul 1, 2013 at 11:34 AM, Shawn Guo <shawn.guo@linaro.org> wrote: >>> >>>> And also why we do not see the impact of i2c issue on sgtl5000 before >>>> the offending commit af8ee11 (ASoC: sgtl5000: Fix driver probe after >>>> reset)? >>> >>> Probably because the mxs i2c was working before this commit. >> >> Do you have a commit id at which mxs i2c is known good? I would like to >> find out it's a mxs i2c issue or sgtl5000 problem. > > I don't have it, but I am adding Alexandre in case he knows. > > Alexandre, > > Do you happen to know a commit id which does not show the mxs i2c > timeouts you have been observing? > the closest I have is that 3.7 was ok but 3.9 is broken. In the meant time, Marek did rewrite the whole stuff ;) >> >>> >>> Commit af8ee11 does fix the reset probe on mx51/mx53/mx6. >>> >>> It's only on mx28 that we have this issue. >>> >>> Also, please see this patch from Alexandre Belloni: >>> https://lkml.org/lkml/2013/6/24/430 >>> >>> He can only connect the ADC chip if he uses I2C bitbang mode on mx28. >> >> Does I2C bitbang mode help fix the sgtl5000 issue here? > > I haven't made this test. >
On Mon, Jul 01, 2013 at 07:43:27PM +0200, Marek Vasut wrote: > Dear Fabio Estevam, > > > On Mon, Jul 1, 2013 at 12:33 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > > > On Mon, Jul 01, 2013 at 12:11:58PM -0300, Fabio Estevam wrote: > > >> On Mon, Jul 1, 2013 at 11:34 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > >> > And also why we do not see the impact of i2c issue on sgtl5000 before > > >> > the offending commit af8ee11 (ASoC: sgtl5000: Fix driver probe after > > >> > reset)? > > >> > > >> Probably because the mxs i2c was working before this commit. > > > > > > Do you have a commit id at which mxs i2c is known good? I would like to > > > find out it's a mxs i2c issue or sgtl5000 problem. > > > > I don't have it, but I am adding Alexandre in case he knows. > > > > Alexandre, > > > > Do you happen to know a commit id which does not show the mxs i2c > > timeouts you have been observing? > > I think you can just disable the PIO mode altogether (around line 500 ... if > (msg->len < 8) ... replace this with if (0) ) and then the problem should not be > there (if it's a PIO problem). Ok, Fabio is right. With the change, sgtl5000 write works even without that msleep(50). So, Mark, please disregard the patch, and we should fix mxs i2c driver instead. Shawn
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index d441559..20bca03 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -1552,6 +1552,13 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, i2c_set_clientdata(client, sgtl5000); + /* + * It seems that sgtl5000 takes a longer time to get the registers + * ready for write than bread. Let's give it a ramping time before + * the first write goes. + */ + msleep(50); + /* Ensure sgtl5000 will start with sane register values */ ret = sgtl5000_fill_defaults(sgtl5000); if (ret)
Since commit af8ee11 (ASoC: sgtl5000: Fix driver probe after reset), it's very ofen to run into the following probe error on imx28. It is caused by the regmap_write() failure in sgtl5000_fill_defaults(). However, the regmap_read() before this has already been working. It seems that sgtl5000 takes a longer ramping time to get the registers ready for write than read. [ 1.991579] sgtl5000 0-000a: sgtl5000 revision 0x11 [ 2.021655] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered [ 2.039087] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) [ 2.046299] platform sound.12: Driver mxs-sgtl5000 requests probe deferral Fix the regression by giving it a ramping time before the first write. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- sound/soc/codecs/sgtl5000.c | 7 +++++++ 1 file changed, 7 insertions(+)