[PATCH 0/4] Fix mxs audio regressions

[PATCH 0/4] Fix mxs audio regressions

[Shawn Guo]

The mxs audio got terribly broken during 3.11 development cycle due to
the following two commits.

  af8ee11 ASoC: sgtl5000: Fix driver probe after reset
  9e13f34 ASoC: sgtl5000: Let the codec acquire its clock

The series fixes the regressions and bring mxs audio back to work.

Mark,

Please consider to take the whole series as fixes through your tree
during 3.11-rc, so that we do not get a regression in the final v3.11
release.

Shawn

Shawn Guo (4):
  ASoC: sgtl5000: give it a ramping time before writting
  ASoC: sgtl5000: defer the probe if clock is not found
  ASoC: mxs: register saif mclk to clock framework
  ARM: mxs: saif0 is the clock provider to sgtl5000

 arch/arm/boot/dts/imx28-apx4devkit.dts |    2 +-
 arch/arm/boot/dts/imx28-evk.dts        |    2 +-
 arch/arm/boot/dts/imx28-m28evk.dts     |    2 +-
 arch/arm/boot/dts/imx28.dtsi           |    1 +
 sound/soc/codecs/sgtl5000.c            |   10 ++++++++-
 sound/soc/mxs/mxs-saif.c               |   35 ++++++++++++++++++++++++++++++++
 6 files changed, 48 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH 0/4] Fix mxs audio regressions

[Shawn Guo]

The mxs audio got terribly broken during 3.11 development cycle due to
the following two commits.

  af8ee11 ASoC: sgtl5000: Fix driver probe after reset
  9e13f34 ASoC: sgtl5000: Let the codec acquire its clock

The series fixes the regressions and bring mxs audio back to work.

Mark,

Please consider to take the whole series as fixes through your tree
during 3.11-rc, so that we do not get a regression in the final v3.11
release.

Shawn

Shawn Guo (4):
  ASoC: sgtl5000: give it a ramping time before writting
  ASoC: sgtl5000: defer the probe if clock is not found
  ASoC: mxs: register saif mclk to clock framework
  ARM: mxs: saif0 is the clock provider to sgtl5000

 arch/arm/boot/dts/imx28-apx4devkit.dts |    2 +-
 arch/arm/boot/dts/imx28-evk.dts        |    2 +-
 arch/arm/boot/dts/imx28-m28evk.dts     |    2 +-
 arch/arm/boot/dts/imx28.dtsi           |    1 +
 sound/soc/codecs/sgtl5000.c            |   10 ++++++++-
 sound/soc/mxs/mxs-saif.c               |   35 ++++++++++++++++++++++++++++++++
 6 files changed, 48 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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
+	 * the first write goes.
+	 */
+	msleep(50);
+
 	/* Ensure sgtl5000 will start with sane register values */
 	ret = sgtl5000_fill_defaults(sgtl5000);
 	if (ret)
-- 
1.7.9.5

[PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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
+	 * the first write goes.
+	 */
+	msleep(50);
+
 	/* Ensure sgtl5000 will start with sane register values */
 	ret = sgtl5000_fill_defaults(sgtl5000);
 	if (ret)
-- 
1.7.9.5

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 619 bytes --]

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" :)

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[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.

Also "bread" :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130701/38567814/attachment.sig>

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Marek Vasut]

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

[PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Marek Vasut]

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

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

[PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Fabio Estevam]

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

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Fabio Estevam]

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

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Fabio Estevam]

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.

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Fabio Estevam]

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.

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[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?

>
>>
>> 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.

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[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?

>
>>
>> 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.

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Marek Vasut]

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

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Marek Vasut]

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

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Fabio Estevam]

Hi Marek,

On Mon, Jul 1, 2013 at 2:43 PM, Marek Vasut <marex@denx.de> wrote:

> 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).

Yes, this is a good suggestion.

This is what I did on 3.10 kernel:

- Booted a 3.10 kernel, audio is probed correctly

- Enabled touchscreen:

--- a/arch/arm/boot/dts/imx28-evk.dts
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -181,6 +181,7 @@

                        lradc@80050000 {
                                status = "okay";
+                               fsl,lradc-touchscreen-wires = <4>;
                        };


Then audio fails to probe:

[    3.854263] sgtl5000 0-000a: Device with ID register 0 is not a sgtl5000
[    5.875286] sgtl5000 0-000a: ASoC: failed to probe CODEC -19
[    5.881522] mxs-sgtl5000 sound.12: ASoC: failed to instantiate card -19
[    5.891010] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-19)

- Removed PIO from i2c, then audio probes again

--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -494,7 +494,7 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter
*adap, struct i2c_msg *msg,
         * based on this empirical measurement and a lot of previous frobbing.
         */
        i2c->cmd_err = 0;
-       if (msg->len < 8) {
+       if (0) {
                ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);
                if (ret)
                        mxs_i2c_reset(i2c);

Really don't know why enabling the touchscreen triggers the i2c error
during the codec reading though.

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Fabio Estevam]

Hi Marek,

On Mon, Jul 1, 2013 at 2:43 PM, Marek Vasut <marex@denx.de> wrote:

> 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).

Yes, this is a good suggestion.

This is what I did on 3.10 kernel:

- Booted a 3.10 kernel, audio is probed correctly

- Enabled touchscreen:

--- a/arch/arm/boot/dts/imx28-evk.dts
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -181,6 +181,7 @@

                        lradc at 80050000 {
                                status = "okay";
+                               fsl,lradc-touchscreen-wires = <4>;
                        };


Then audio fails to probe:

[    3.854263] sgtl5000 0-000a: Device with ID register 0 is not a sgtl5000
[    5.875286] sgtl5000 0-000a: ASoC: failed to probe CODEC -19
[    5.881522] mxs-sgtl5000 sound.12: ASoC: failed to instantiate card -19
[    5.891010] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-19)

- Removed PIO from i2c, then audio probes again

--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -494,7 +494,7 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter
*adap, struct i2c_msg *msg,
         * based on this empirical measurement and a lot of previous frobbing.
         */
        i2c->cmd_err = 0;
-       if (msg->len < 8) {
+       if (0) {
                ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);
                if (ret)
                        mxs_i2c_reset(i2c);

Really don't know why enabling the touchscreen triggers the i2c error
during the codec reading though.

Re: [alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Alexandre Belloni]

Hi,

On 01/07/2013 21:14, Fabio Estevam wrote:
> Hi Marek,
> 
> On Mon, Jul 1, 2013 at 2:43 PM, Marek Vasut <marex@denx.de> wrote:
> 
>> 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).
> 
> Yes, this is a good suggestion.
> 
> This is what I did on 3.10 kernel:
> 
> - Booted a 3.10 kernel, audio is probed correctly
> 
> - Enabled touchscreen:
> 

It definitely seems to be an issue with PIO mode as the investigation
from Torsten Fleischer showed. I'll test that on my boards tonight. I'll
also try to remove the touchscreen support to see if I observe the same
thing as you do.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Alexandre Belloni]

Hi,

On 01/07/2013 21:14, Fabio Estevam wrote:
> Hi Marek,
> 
> On Mon, Jul 1, 2013 at 2:43 PM, Marek Vasut <marex@denx.de> wrote:
> 
>> 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).
> 
> Yes, this is a good suggestion.
> 
> This is what I did on 3.10 kernel:
> 
> - Booted a 3.10 kernel, audio is probed correctly
> 
> - Enabled touchscreen:
> 

It definitely seems to be an issue with PIO mode as the investigation
from Torsten Fleischer showed. I'll test that on my boards tonight. I'll
also try to remove the touchscreen support to see if I observe the same
thing as you do.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

Re: [alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Alexandre Belloni]

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.
> 


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[alsa-devel] [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Alexandre Belloni]

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.
> 


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

[PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Shawn Guo]

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

Re: [PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Lothar Waßmann]

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
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 1/4] ASoC: sgtl5000: give it a ramping time before writting

[Lothar Waßmann]

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
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 2/4] ASoC: sgtl5000: defer the probe if clock is not found

[Shawn Guo]

It's not always the case that clock is already available when sgtl5000
get probed at the first time, e.g. the clock is provided by CPU DAI
which may be probed after sgtl5000.  So let's defer the probe when
devm_clk_get() call fails and give it chance to try later.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 sound/soc/codecs/sgtl5000.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 20bca03..1b7003c 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1527,7 +1527,8 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	if (IS_ERR(sgtl5000->mclk)) {
 		ret = PTR_ERR(sgtl5000->mclk);
 		dev_err(&client->dev, "Failed to get mclock: %d\n", ret);
-		return ret;
+		/* Defer the probe to see if the clk will be provided later */
+		return ret == -ENOENT ? -EPROBE_DEFER : ret;
 	}
 
 	ret = clk_prepare_enable(sgtl5000->mclk);
-- 
1.7.9.5

[PATCH 2/4] ASoC: sgtl5000: defer the probe if clock is not found

[Shawn Guo]

It's not always the case that clock is already available when sgtl5000
get probed at the first time, e.g. the clock is provided by CPU DAI
which may be probed after sgtl5000.  So let's defer the probe when
devm_clk_get() call fails and give it chance to try later.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 sound/soc/codecs/sgtl5000.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 20bca03..1b7003c 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1527,7 +1527,8 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	if (IS_ERR(sgtl5000->mclk)) {
 		ret = PTR_ERR(sgtl5000->mclk);
 		dev_err(&client->dev, "Failed to get mclock: %d\n", ret);
-		return ret;
+		/* Defer the probe to see if the clk will be provided later */
+		return ret == -ENOENT ? -EPROBE_DEFER : ret;
 	}
 
 	ret = clk_prepare_enable(sgtl5000->mclk);
-- 
1.7.9.5

Re: [PATCH 2/4] ASoC: sgtl5000: defer the probe if clock is not found

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 435 bytes --]

On Mon, Jul 01, 2013 at 04:16:09PM +0800, Shawn Guo wrote:
> It's not always the case that clock is already available when sgtl5000
> get probed at the first time, e.g. the clock is provided by CPU DAI
> which may be probed after sgtl5000.  So let's defer the probe when
> devm_clk_get() call fails and give it chance to try later.

Why not fix this in the clock API - the same logic is going to apply to
a great proportion of clocks?

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 2/4] ASoC: sgtl5000: defer the probe if clock is not found

[Mark Brown]

On Mon, Jul 01, 2013 at 04:16:09PM +0800, Shawn Guo wrote:
> It's not always the case that clock is already available when sgtl5000
> get probed at the first time, e.g. the clock is provided by CPU DAI
> which may be probed after sgtl5000.  So let's defer the probe when
> devm_clk_get() call fails and give it chance to try later.

Why not fix this in the clock API - the same logic is going to apply to
a great proportion of clocks?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130701/6fdf4c7b/attachment.sig>

Re: [PATCH 2/4] ASoC: sgtl5000: defer the probe if clock is not found

[Russell King - ARM Linux]

On Mon, Jul 01, 2013 at 10:45:25AM +0100, Mark Brown wrote:
> On Mon, Jul 01, 2013 at 04:16:09PM +0800, Shawn Guo wrote:
> > It's not always the case that clock is already available when sgtl5000
> > get probed at the first time, e.g. the clock is provided by CPU DAI
> > which may be probed after sgtl5000.  So let's defer the probe when
> > devm_clk_get() call fails and give it chance to try later.
> 
> Why not fix this in the clock API - the same logic is going to apply to
> a great proportion of clocks?

It's not ever clear whether a missing clock is because it's just not
provided or whether it's because it hasn't been registered yet.  The
decision to defer on missing resources is something which a _driver_
using the resource should make, not the subsystem providing the
resource - because the driver should know better whether that resource
is optional, whether it can continue to initialise without it and maybe
try again later, or whether it does need to defer.

Take for instance a video driver. :)  It may be that it can't do all
resolutions, but it can do some without an external clock, so it may
decide that it can initialise without the external clock and allow one
of the possible modes to be set - and when the external clock does
appear, it can then allow the other modes.

That kind of information is unknown at the subsystem level, and so the
subsystem level should only ever return "I don't know about this" not
"defer probe".

[PATCH 2/4] ASoC: sgtl5000: defer the probe if clock is not found

[Russell King - ARM Linux]

On Mon, Jul 01, 2013 at 10:45:25AM +0100, Mark Brown wrote:
> On Mon, Jul 01, 2013 at 04:16:09PM +0800, Shawn Guo wrote:
> > It's not always the case that clock is already available when sgtl5000
> > get probed at the first time, e.g. the clock is provided by CPU DAI
> > which may be probed after sgtl5000.  So let's defer the probe when
> > devm_clk_get() call fails and give it chance to try later.
> 
> Why not fix this in the clock API - the same logic is going to apply to
> a great proportion of clocks?

It's not ever clear whether a missing clock is because it's just not
provided or whether it's because it hasn't been registered yet.  The
decision to defer on missing resources is something which a _driver_
using the resource should make, not the subsystem providing the
resource - because the driver should know better whether that resource
is optional, whether it can continue to initialise without it and maybe
try again later, or whether it does need to defer.

Take for instance a video driver. :)  It may be that it can't do all
resolutions, but it can do some without an external clock, so it may
decide that it can initialise without the external clock and allow one
of the possible modes to be set - and when the external clock does
appear, it can then allow the other modes.

That kind of information is unknown at the subsystem level, and so the
subsystem level should only ever return "I don't know about this" not
"defer probe".

Re: [PATCH 2/4] ASoC: sgtl5000: defer the probe if clock is not found

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2024 bytes --]

On Mon, Jul 01, 2013 at 12:33:10PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 01, 2013 at 10:45:25AM +0100, Mark Brown wrote:

> > Why not fix this in the clock API - the same logic is going to apply to
> > a great proportion of clocks?

> It's not ever clear whether a missing clock is because it's just not
> provided or whether it's because it hasn't been registered yet.  The
> decision to defer on missing resources is something which a _driver_
> using the resource should make, not the subsystem providing the
> resource - because the driver should know better whether that resource
> is optional, whether it can continue to initialise without it and maybe
> try again later, or whether it does need to defer.

Given how common it is to have a hard requirement for a clock I think we
should at least have a helper which implements deferral, even if it's
not part of the default request function, since so many drivers ought to
be using it.  Doing this in by default doesn't prevent the driver
overriding, though - the driver can choose to squash down the PROBE_DEFER
into a fatal error.

One nice thing you can sometimes do here in the DT case is to have the
subsystem tell the driver the difference between "this clock is mapped
but not yet registered" and "this clock is not physically present"
though there's never any guarantee that the provided resource will
actually load.  I don't think the idiom for the clock API is to provide
every single clock in DT though so that might not be doable.

> Take for instance a video driver. :)  It may be that it can't do all
> resolutions, but it can do some without an external clock, so it may
> decide that it can initialise without the external clock and allow one
> of the possible modes to be set - and when the external clock does
> appear, it can then allow the other modes.

Yes, that's the sort of case where you do definitely want to just carry
on (though there is then some vulnerability to changes in init ordering
with things like modular kernels).

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 2/4] ASoC: sgtl5000: defer the probe if clock is not found

[Mark Brown]

On Mon, Jul 01, 2013 at 12:33:10PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 01, 2013 at 10:45:25AM +0100, Mark Brown wrote:

> > Why not fix this in the clock API - the same logic is going to apply to
> > a great proportion of clocks?

> It's not ever clear whether a missing clock is because it's just not
> provided or whether it's because it hasn't been registered yet.  The
> decision to defer on missing resources is something which a _driver_
> using the resource should make, not the subsystem providing the
> resource - because the driver should know better whether that resource
> is optional, whether it can continue to initialise without it and maybe
> try again later, or whether it does need to defer.

Given how common it is to have a hard requirement for a clock I think we
should at least have a helper which implements deferral, even if it's
not part of the default request function, since so many drivers ought to
be using it.  Doing this in by default doesn't prevent the driver
overriding, though - the driver can choose to squash down the PROBE_DEFER
into a fatal error.

One nice thing you can sometimes do here in the DT case is to have the
subsystem tell the driver the difference between "this clock is mapped
but not yet registered" and "this clock is not physically present"
though there's never any guarantee that the provided resource will
actually load.  I don't think the idiom for the clock API is to provide
every single clock in DT though so that might not be doable.

> Take for instance a video driver. :)  It may be that it can't do all
> resolutions, but it can do some without an external clock, so it may
> decide that it can initialise without the external clock and allow one
> of the possible modes to be set - and when the external clock does
> appear, it can then allow the other modes.

Yes, that's the sort of case where you do definitely want to just carry
on (though there is then some vulnerability to changes in init ordering
with things like modular kernels).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130701/34be7eea/attachment.sig>

[PATCH 3/4] ASoC: mxs: register saif mclk to clock framework

[Shawn Guo]

Mostly the mxs system design uses saif0 mclk output as the clock source
of codec.  Since the mclk is implemented as a general divider with the
saif clk as the parent clock, let's register the mclk as a basic
clk-divider to common clock framework.  Then with it being a clock
provdier, clk_get() call in codec driver probe function will just work.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 sound/soc/mxs/mxs-saif.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
index 49d8700..54511c5 100644
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/time.h>
 #include <sound/core.h>
@@ -658,6 +659,33 @@ static irqreturn_t mxs_saif_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int mxs_saif_mclk_init(struct platform_device *pdev)
+{
+	struct mxs_saif *saif = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	struct clk *clk;
+	int ret;
+
+	clk = clk_register_divider(&pdev->dev, "mxs_saif_mclk",
+				   __clk_get_name(saif->clk), 0,
+				   saif->base + SAIF_CTRL,
+				   BP_SAIF_CTRL_BITCLK_MULT_RATE, 3,
+				   0, NULL);
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		if (ret == -EEXIST)
+			return 0;
+		dev_err(&pdev->dev, "failed to register mclk: %d\n", ret);
+		return PTR_ERR(clk);
+	}
+
+	ret = of_clk_add_provider(np, of_clk_src_simple_get, clk);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int mxs_saif_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -734,6 +762,13 @@ static int mxs_saif_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, saif);
 
+	/* We only support saif0 being tx and clock master */
+	if (saif->id == 0) {
+		ret = mxs_saif_mclk_init(pdev);
+		if (ret)
+			dev_warn(&pdev->dev, "failed to init clocks\n");
+	}
+
 	ret = snd_soc_register_component(&pdev->dev, &mxs_saif_component,
 					 &mxs_saif_dai, 1);
 	if (ret) {
-- 
1.7.9.5

[PATCH 3/4] ASoC: mxs: register saif mclk to clock framework

[Shawn Guo]

Mostly the mxs system design uses saif0 mclk output as the clock source
of codec.  Since the mclk is implemented as a general divider with the
saif clk as the parent clock, let's register the mclk as a basic
clk-divider to common clock framework.  Then with it being a clock
provdier, clk_get() call in codec driver probe function will just work.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 sound/soc/mxs/mxs-saif.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
index 49d8700..54511c5 100644
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/time.h>
 #include <sound/core.h>
@@ -658,6 +659,33 @@ static irqreturn_t mxs_saif_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int mxs_saif_mclk_init(struct platform_device *pdev)
+{
+	struct mxs_saif *saif = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	struct clk *clk;
+	int ret;
+
+	clk = clk_register_divider(&pdev->dev, "mxs_saif_mclk",
+				   __clk_get_name(saif->clk), 0,
+				   saif->base + SAIF_CTRL,
+				   BP_SAIF_CTRL_BITCLK_MULT_RATE, 3,
+				   0, NULL);
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		if (ret == -EEXIST)
+			return 0;
+		dev_err(&pdev->dev, "failed to register mclk: %d\n", ret);
+		return PTR_ERR(clk);
+	}
+
+	ret = of_clk_add_provider(np, of_clk_src_simple_get, clk);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int mxs_saif_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -734,6 +762,13 @@ static int mxs_saif_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, saif);
 
+	/* We only support saif0 being tx and clock master */
+	if (saif->id == 0) {
+		ret = mxs_saif_mclk_init(pdev);
+		if (ret)
+			dev_warn(&pdev->dev, "failed to init clocks\n");
+	}
+
 	ret = snd_soc_register_component(&pdev->dev, &mxs_saif_component,
 					 &mxs_saif_dai, 1);
 	if (ret) {
-- 
1.7.9.5

Re: [PATCH 3/4] ASoC: mxs: register saif mclk to clock framework

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 434 bytes --]

On Mon, Jul 01, 2013 at 04:16:10PM +0800, Shawn Guo wrote:
> Mostly the mxs system design uses saif0 mclk output as the clock source
> of codec.  Since the mclk is implemented as a general divider with the
> saif clk as the parent clock, let's register the mclk as a basic
> clk-divider to common clock framework.  Then with it being a clock
> provdier, clk_get() call in codec driver probe function will just work.

Applied, thanks.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 3/4] ASoC: mxs: register saif mclk to clock framework

[Mark Brown]

On Mon, Jul 01, 2013 at 04:16:10PM +0800, Shawn Guo wrote:
> Mostly the mxs system design uses saif0 mclk output as the clock source
> of codec.  Since the mclk is implemented as a general divider with the
> saif clk as the parent clock, let's register the mclk as a basic
> clk-divider to common clock framework.  Then with it being a clock
> provdier, clk_get() call in codec driver probe function will just work.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130701/353cc584/attachment.sig>

[PATCH 4/4] ARM: mxs: saif0 is the clock provider to sgtl5000

[Shawn Guo]

These systems all use saif0 as the mclock provider to codec sgtl5000.
Reflect that in device tree source, so that sgtl5000 can find the clock
by calling clk_get().

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/boot/dts/imx28-apx4devkit.dts |    2 +-
 arch/arm/boot/dts/imx28-evk.dts        |    2 +-
 arch/arm/boot/dts/imx28-m28evk.dts     |    2 +-
 arch/arm/boot/dts/imx28.dtsi           |    1 +
 4 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/imx28-apx4devkit.dts b/arch/arm/boot/dts/imx28-apx4devkit.dts
index 43bf3c7..0e7fed4 100644
--- a/arch/arm/boot/dts/imx28-apx4devkit.dts
+++ b/arch/arm/boot/dts/imx28-apx4devkit.dts
@@ -147,7 +147,7 @@
 					reg = <0x0a>;
 					VDDA-supply = <&reg_3p3v>;
 					VDDIO-supply = <&reg_3p3v>;
-
+					clocks = <&saif0>;
 				};
 
 				pcf8563: rtc@51 {
diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
index 3637bf3..dccb223 100644
--- a/arch/arm/boot/dts/imx28-evk.dts
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -193,7 +193,7 @@
 					reg = <0x0a>;
 					VDDA-supply = <&reg_3p3v>;
 					VDDIO-supply = <&reg_3p3v>;
-
+					clocks = <&saif0>;
 				};
 
 				at24@51 {
diff --git a/arch/arm/boot/dts/imx28-m28evk.dts b/arch/arm/boot/dts/imx28-m28evk.dts
index 880df2f..44d9da5 100644
--- a/arch/arm/boot/dts/imx28-m28evk.dts
+++ b/arch/arm/boot/dts/imx28-m28evk.dts
@@ -184,7 +184,7 @@
 					reg = <0x0a>;
 					VDDA-supply = <&reg_3p3v>;
 					VDDIO-supply = <&reg_3p3v>;
-
+					clocks = <&saif0>;
 				};
 
 				eeprom: eeprom@51 {
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 6a8acb0..9524a05 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -837,6 +837,7 @@
 				compatible = "fsl,imx28-saif";
 				reg = <0x80042000 0x2000>;
 				interrupts = <59 80>;
+				#clock-cells = <0>;
 				clocks = <&clks 53>;
 				dmas = <&dma_apbx 4>;
 				dma-names = "rx-tx";
-- 
1.7.9.5

[PATCH 4/4] ARM: mxs: saif0 is the clock provider to sgtl5000

[Shawn Guo]

These systems all use saif0 as the mclock provider to codec sgtl5000.
Reflect that in device tree source, so that sgtl5000 can find the clock
by calling clk_get().

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/boot/dts/imx28-apx4devkit.dts |    2 +-
 arch/arm/boot/dts/imx28-evk.dts        |    2 +-
 arch/arm/boot/dts/imx28-m28evk.dts     |    2 +-
 arch/arm/boot/dts/imx28.dtsi           |    1 +
 4 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/imx28-apx4devkit.dts b/arch/arm/boot/dts/imx28-apx4devkit.dts
index 43bf3c7..0e7fed4 100644
--- a/arch/arm/boot/dts/imx28-apx4devkit.dts
+++ b/arch/arm/boot/dts/imx28-apx4devkit.dts
@@ -147,7 +147,7 @@
 					reg = <0x0a>;
 					VDDA-supply = <&reg_3p3v>;
 					VDDIO-supply = <&reg_3p3v>;
-
+					clocks = <&saif0>;
 				};
 
 				pcf8563: rtc at 51 {
diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
index 3637bf3..dccb223 100644
--- a/arch/arm/boot/dts/imx28-evk.dts
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -193,7 +193,7 @@
 					reg = <0x0a>;
 					VDDA-supply = <&reg_3p3v>;
 					VDDIO-supply = <&reg_3p3v>;
-
+					clocks = <&saif0>;
 				};
 
 				at24 at 51 {
diff --git a/arch/arm/boot/dts/imx28-m28evk.dts b/arch/arm/boot/dts/imx28-m28evk.dts
index 880df2f..44d9da5 100644
--- a/arch/arm/boot/dts/imx28-m28evk.dts
+++ b/arch/arm/boot/dts/imx28-m28evk.dts
@@ -184,7 +184,7 @@
 					reg = <0x0a>;
 					VDDA-supply = <&reg_3p3v>;
 					VDDIO-supply = <&reg_3p3v>;
-
+					clocks = <&saif0>;
 				};
 
 				eeprom: eeprom at 51 {
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 6a8acb0..9524a05 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -837,6 +837,7 @@
 				compatible = "fsl,imx28-saif";
 				reg = <0x80042000 0x2000>;
 				interrupts = <59 80>;
+				#clock-cells = <0>;
 				clocks = <&clks 53>;
 				dmas = <&dma_apbx 4>;
 				dma-names = "rx-tx";
-- 
1.7.9.5