Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxxxxxxxx> writes:
>> @@ -1158,6 +1199,9 @@ static int wm9713_soc_suspend(struct snd_soc_codec *codec)
>> {
>> u16 reg;
>>
>> + snd_soc_cache_sync(codec);
>
> There doesn't seem to be much point in syncing the cache at the
> start of a suspend, in theory I would expect the cache to be in
> sync at this point anyway.
> I think you are thinking of this wrong, cache_sync does not
> ensure all previous writes have been commited, as I explained
> in my last email it literally writes the cache to the hardware,
> suspend usually turns the hardware off. So why write all the
> registers to the hardware just before turning it off.
Indeed, I'll remove it.
>> + regcache_cache_bypass(codec->component.regmap, true);
>
> Why is the necessary? I can't see an obvious sign that these
> writes bypass the cache in the non-regmap version, am I missing
> something?
This is because to mimic the previous behavior, the next 4 writes
(AC97_EXTENDED_MID, AC97_EXTENDED_MSTATUS, AC97_POWERDOWN * 2) should hit the
hardware but not the cache (hence the bypass), but see below for the follow-up
of my thinking.
> Also if this is necessary I would quite like it to be
> accompanied by a comment in the code to explain why it is safe to
> do this here. Regarding the inherent dangers of cache bypass I
> explained in my last email.
Well, once suspended I supposed no access should be done before the resume
function is called, hence the "safe" part.
>> /* Disable everything except touchpanel - that will be handled
>> * by the touch driver and left disabled if touch is not in
>> * use. */
>> @@ -1173,14 +1217,14 @@ static int wm9713_soc_suspend(struct snd_soc_codec *codec)
>
> I would have expected to see the cache being put into cache only
> mode at some point during suspend, am I missing something here as
> well?
Why ? Once suspended, why would you expect an access to be done to the regmap ?
What is the case this "cache only" protects us from ?
> Again this feels like you are getting confused on the
> functionality of the API, bypassing the cache makes all
> reads/writes go to the hardware, suspend normally turns the
> hardware off. Directing all reads/writes to go to the hardware in
> a function that normally turns the hardware off looks odd.
Yes, I must certainly misunderstand something.
Once again I must understand first why you expect accesses to be done after a
suspend function was called ...
>> + if (ret == 0)
>> + regcache_mark_dirty(codec->component.regmap);
>> +
>> + snd_soc_cache_sync(codec);
>
> Probably best to have both the mark_dirty and the cache_sync in
> the if. Whilst the cache sync is a no-op if it hasn't been marked
> as dirty, will just be a bit clearer this is indentical to the
> pre-regmap code and more likely to remain that way under future
> changes.
I must admit I was expecting that the 4 registers I wrote directly to hardware
in bypass mode were marked as "dirty", and this sync() would restore them ...
Anyway, I have another idea to simplify the code greatly, it's only I'm not sure
if my thinking is right. The idea is that these 3 registers (AC97_EXTENDED_MID,
AC97_EXTENDED_MSTATUS, AC97_POWERDOWN) should never land in the regmap
cache. What I think is that because they are in regmap_ac97_default_volatile(),
they already have this property. Therefore, there is no need to do the bypass
thing, and I could end up with :
static int wm9713_soc_suspend(struct snd_soc_codec *codec)
{
/* Disable everything except touchpanel - that will be handled
* by the touch driver and left disabled if touch is not in
* use. */
snd_soc_update_bits(codec, AC97_EXTENDED_MID, 0x7fff,
0x7fff);
snd_soc_write(codec, AC97_EXTENDED_MSTATUS, 0xffff);
snd_soc_write(codec, AC97_POWERDOWN, 0x6f00);
snd_soc_write(codec, AC97_POWERDOWN, 0xffff);
/*
* RJK: still need to be convinced why this is necessary for this
* next line
*/
regcache_cache_only(codec->regmap, true);
return 0;
}
static int wm9713_soc_resume(struct snd_soc_codec *codec)
{
struct wm9713_priv *wm9713 = snd_soc_codec_get_drvdata(codec);
int ret;
/*
* RJK: still need to be convinced why this is necessary for this
* next line
*/
regcache_cache_only(codec->regmap, false);
ret = snd_ac97_reset(wm9713->ac97, true, WM9713_VENDOR_ID,
WM9713_VENDOR_ID_MASK);
if (ret < 0)
return ret;
snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_STANDBY);
/* do we need to re-start the PLL ? */
if (wm9713->pll_in)
wm9713_set_pll(codec, 0, wm9713->pll_in, 0);
/* only synchronise the codec if warm reset failed */
if (ret == 0) {
regcache_mark_dirty(codec->component.regmap);
snd_soc_cache_sync(codec);
}
return ret;
}
Thanks for your reviews.
--
Robert
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
