Re: ASoC: bug report for ssm2602

Re: ASoC: bug report for ssm2602

[Lars-Peter Clausen]

On 08/22/2014 11:21 AM, Scott Jiang wrote:
> Hi Mark and Lars,
>
> commit 9d863b88ec371491e926e0828dbe3d36ead0f6f9
> Author: Lars-Peter Clausen <lars@metafoo.de>
> Date:   Sat Aug 31 18:15:23 2013 +0200
>
>      ASoC: ssm2602: Fix cache sync
>
>      The ssm2602 uses regmap for caching not soc-cache, so we need to use
>      regcache_sync() instead of snd_soc_cache_sync().
>
> According my test, regcache_sync() can't recover register values when
> resume. While snd_soc_cache_sync() works fine. I used regmap_read() to
> read registers in regmap layer and found it's the same as suspend. But
> regcache_sync() didn't sync these registers correctly to hardware.
>

That does not make too much sense. snd_soc_cache_sync() is definitely a 
no-op when using regmap. Which kernel version are you on? Are there any 
additional changes to this driver in your tree?

- Lars

Re: ASoC: bug report for ssm2602

[Scott Jiang]

2014-08-22 17:47 GMT+08:00 Lars-Peter Clausen <lars@metafoo.de>:
> On 08/22/2014 11:21 AM, Scott Jiang wrote:
>>
>> Hi Mark and Lars,
>>
>> commit 9d863b88ec371491e926e0828dbe3d36ead0f6f9
>> Author: Lars-Peter Clausen <lars@metafoo.de>
>> Date:   Sat Aug 31 18:15:23 2013 +0200
>>
>>      ASoC: ssm2602: Fix cache sync
>>
>>      The ssm2602 uses regmap for caching not soc-cache, so we need to use
>>      regcache_sync() instead of snd_soc_cache_sync().
>>
>> According my test, regcache_sync() can't recover register values when
>> resume. While snd_soc_cache_sync() works fine. I used regmap_read() to
>> read registers in regmap layer and found it's the same as suspend. But
>> regcache_sync() didn't sync these registers correctly to hardware.
>>
>
> That does not make too much sense. snd_soc_cache_sync() is definitely a
> no-op when using regmap. Which kernel version are you on? Are there any
> additional changes to this driver in your tree?

My version is 3.16. I compared with the latest 3.17 rc2 kernel, there
is only one patch.

commit 00a37032a099572302ae89c893bd5c90b924c537
Author: Mark Brown <broonie@linaro.org>
Date:   Thu Jul 31 12:47:13 2014 +0100

    ASoC: ssm2602: Convert to params_width()

    The CODEC doesn't care how data is laid out in memory.

    Signed-off-by: Mark Brown <broonie@linaro.org>
    Acked-by: Lars-Peter Clausen <lars@metafoo.de>

I believe this patch isn't related to the pm bug here.
By the way, have you ever tested your patches on any blackfin board?
Your patches for adau1761, ssm2602 and bf5xx-i2s all failed to run.

Scott

Re: ASoC: bug report for ssm2602

[Mark Brown]

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

On Tue, Aug 26, 2014 at 02:11:20PM +0800, Scott Jiang wrote:
> 2014-08-22 17:47 GMT+08:00 Lars-Peter Clausen <lars@metafoo.de>:

> >>      ASoC: ssm2602: Fix cache sync

> >>      The ssm2602 uses regmap for caching not soc-cache, so we need to use
> >>      regcache_sync() instead of snd_soc_cache_sync().

> >> According my test, regcache_sync() can't recover register values when
> >> resume. While snd_soc_cache_sync() works fine. I used regmap_read() to
> >> read registers in regmap layer and found it's the same as suspend. But
> >> regcache_sync() didn't sync these registers correctly to hardware.

> > That does not make too much sense. snd_soc_cache_sync() is definitely a
> > no-op when using regmap. Which kernel version are you on? Are there any
> > additional changes to this driver in your tree?

If it is the above patch is the problem perhaps that the register map
isn't getting marked as dirty so the cache sync is being suppressed?

> I believe this patch isn't related to the pm bug here.
> By the way, have you ever tested your patches on any blackfin board?
> Your patches for adau1761, ssm2602 and bf5xx-i2s all failed to run.

I guess this is addressed to Lars-Peter but could you be more specific
about the problems you are seeing?

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

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

Re: ASoC: bug report for ssm2602

[Lars-Peter Clausen]

On 08/26/2014 08:11 AM, Scott Jiang wrote:
> 2014-08-22 17:47 GMT+08:00 Lars-Peter Clausen <lars@metafoo.de>:
>> On 08/22/2014 11:21 AM, Scott Jiang wrote:
>>>
>>> Hi Mark and Lars,
>>>
>>> commit 9d863b88ec371491e926e0828dbe3d36ead0f6f9
>>> Author: Lars-Peter Clausen <lars@metafoo.de>
>>> Date:   Sat Aug 31 18:15:23 2013 +0200
>>>
>>>       ASoC: ssm2602: Fix cache sync
>>>
>>>       The ssm2602 uses regmap for caching not soc-cache, so we need to use
>>>       regcache_sync() instead of snd_soc_cache_sync().
>>>
>>> According my test, regcache_sync() can't recover register values when
>>> resume. While snd_soc_cache_sync() works fine. I used regmap_read() to
>>> read registers in regmap layer and found it's the same as suspend. But
>>> regcache_sync() didn't sync these registers correctly to hardware.
>>>
>>
>> That does not make too much sense. snd_soc_cache_sync() is definitely a
>> no-op when using regmap. Which kernel version are you on? Are there any
>> additional changes to this driver in your tree?
>
> My version is 3.16. I compared with the latest 3.17 rc2 kernel, there
> is only one patch.

And it is vanilla v3.16, no patches to the driver or the ASoC core? 
snd_soc_cache_sync() will definitely do nothing for this driver on v3.16 
since reg_cache_size is 0 for this driver's snd_soc_codec_driver.

[...]
> By the way, have you ever tested your patches on any blackfin board?
> Your patches for adau1761, ssm2602 and bf5xx-i2s all failed to run.

Yes, I've tested them before submitting them upstream on a ezkit board.
What kind of errors are you seeing?

- Lars

Re: ASoC: bug report for ssm2602

[Lars-Peter Clausen]

On 08/26/2014 08:28 AM, Mark Brown wrote:
> On Tue, Aug 26, 2014 at 02:11:20PM +0800, Scott Jiang wrote:
>> 2014-08-22 17:47 GMT+08:00 Lars-Peter Clausen <lars@metafoo.de>:
>
>>>>       ASoC: ssm2602: Fix cache sync
>
>>>>       The ssm2602 uses regmap for caching not soc-cache, so we need to use
>>>>       regcache_sync() instead of snd_soc_cache_sync().
>
>>>> According my test, regcache_sync() can't recover register values when
>>>> resume. While snd_soc_cache_sync() works fine. I used regmap_read() to
>>>> read registers in regmap layer and found it's the same as suspend. But
>>>> regcache_sync() didn't sync these registers correctly to hardware.
>
>>> That does not make too much sense. snd_soc_cache_sync() is definitely a
>>> no-op when using regmap. Which kernel version are you on? Are there any
>>> additional changes to this driver in your tree?
>
> If it is the above patch is the problem perhaps that the register map
> isn't getting marked as dirty so the cache sync is being suppressed?

The core should mark it as dirty after calling the suspend callback. But 
there might be bugs preventing this. Try to make sure that 
regmap_mark_dirty() gets called for this device and also try to check what 
goes wrong in regcache_sync() and why it does not sync the registers.

- Lars

Re: ASoC: bug report for ssm2602

[Mark Brown]

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

On Tue, Aug 26, 2014 at 08:43:14AM +0200, Lars-Peter Clausen wrote:
> On 08/26/2014 08:28 AM, Mark Brown wrote:

> >If it is the above patch is the problem perhaps that the register map
> >isn't getting marked as dirty so the cache sync is being suppressed?

> The core should mark it as dirty after calling the suspend callback. But
> there might be bugs preventing this. Try to make sure that
> regmap_mark_dirty() gets called for this device and also try to check what
> goes wrong in regcache_sync() and why it does not sync the registers.

Indeed, I can't see any obvious breakage there and don't remember any
fixes though I've not checked the old code properly.

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

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

Re: ASoC: bug report for ssm2602

[Scott Jiang]

Hi Mark and Lars,

>
> If it is the above patch is the problem perhaps that the register map
> isn't getting marked as dirty so the cache sync is being suppressed?
>

I print some info in regcache_sync_block_single() and found some registers
are skipped because the regmap thinks it's the same as default. I guess it's
a bug here. When I use regmap_read(), these values are not equal to default.

>> I believe this patch isn't related to the pm bug here.
>> By the way, have you ever tested your patches on any blackfin board?
>> Your patches for adau1761, ssm2602 and bf5xx-i2s all failed to run.
>
> I guess this is addressed to Lars-Peter but could you be more specific
> about the problems you are seeing?

bf5xx-i2s-pcm, I have sent a patch to Mark for this.

commit 30443408fd7201fd1911b09daccf92fae3cc700d
Author: Scott Jiang <scott.jiang.linux@gmail.com>
Date:   Fri Jul 18 16:14:57 2014 +0800

    ASoC: blackfin: use samples to set silence

    The third parameter for snd_pcm_format_set_silence needs the number
    of samples instead of sample bytes.

In fact, test cases still report bugs for this driver after I fix this
bug. There are
many Xrun for TDM codecs. I guess you change the parameters in
bf5xx_pcm_hardware
for TDM to I2S.

ssm2602, can't recover registers after resume.
adau1761, I'm not sure what the problem is. I'll take a look later.

Thanks,
Scott