On Wed, Oct 28, 2015 at 09:34:33PM +0100, Robert Jarzmik wrote:
> Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxxxxxxxx> writes:
> >> /* 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 ?
Ah sorry this one is my bad, I was assuming these where the
runtime suspend/resume a closer look shows these are the system
suspend/resume. So yes it is pretty reasonable nothing will touch
the registers.
>
> > 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 :
Ah ok yes these are all volatile registers in which cause they
will bypass the naturally.
>
> 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);
Yes you are correct you can just drop this line.
>
> 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);
ditto.
>
> 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.
Yeah that solution looks a lot more like what I was expecting.
Thanks,
Charles
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
