[RFC] ASoC: max98373: don't access volatile registers in bias level off

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul at kernel.org>
> Sent: Friday, November 20, 2020 1:59 PM
> To: Mark Brown <broonie at kernel.org>
> Cc: Bard Liao <yung-chuan.liao at linux.intel.com>; tiwai at suse.de; alsa-
> devel at alsa-project.org; pierre-louis.bossart at linux.intel.com;
> ryans.lee at maximintegrated.com; kai.vehmanen at linux.intel.com; Liao, Bard
> <bard.liao at intel.com>
> Subject: Re: [RFC] ASoC: max98373: don't access volatile registers in bias level
> off
> 
> On 19-11-20, 16:58, Mark Brown wrote:
> > On Mon, Nov 09, 2020 at 09:55:43PM +0800, Bard Liao wrote:
> > > We will set regcache_cache_only true in suspend. As a result,
> > > regmap_read will return error when we try to read volatile registers in
> suspend.
> > > Besides, it doesn't make sense to read feedback data when codec is
> > > not active. To avoid the regmap_read error, this patch try to return
> > > a fake value when kcontrol _get is called in suspend.
> >
> > > However, the question is what is the "correct" behavior when we try
> > > to read a volatile register when cache only is set.
> > > 1. return an error code to signal codec is not available as what we
> > > have now.
> > > 2. return a fake value like what this patch do.
> > > 3. wake-up the codec and always return a valid value.
> >
> > This depends a bit on what the value is, if a value obtained by waking
> > the device up is likely to be useful and what userspace is likely to
> > do if it gets an error.
> >
> > > -SOC_SINGLE("ADC PVDD",
> MAX98373_R2054_MEAS_ADC_PVDD_CH_READBACK, 0,
> > > 0xFF, 0), -SOC_SINGLE("ADC TEMP",
> > > MAX98373_R2055_MEAS_ADC_THERM_CH_READBACK, 0, 0xFF, 0),
> >
> > For things like voltage and temperature it seems like if we return
> > zero that's not much different from a userspace point of view than
> > returning an error, they're both clearly bad values so if userspace is
> > doing any kind of error checking based on looking at the values it's
> > likely to get upset by unexpected values - a clear indication that it
> > was the read that failed might be better than bogus data, stops
> > someone wondering why there's no power or the device is suddenly
> > freezing.  Or if it's important that we get a value for monitoring
> > purposes then waking the device up and reading a value might make more
> > sense though it'd burn power if done by some random ALSA UI on a
> > regular basis which wouldn't be desirable either, it'd be relatively slow too.
> >
> > Another option might be for the driver to cache the last value it got
> > when powering down, that way it can return something "sensible" even
> > if it's not up to date.
> 
> That would be better IMO. Also, do consider the nature of data, the
> monitoring data should be useful only when audio is active, not sure if
> anyone would care to look at this data when codec is suspended...

Thanks Mark and Vinod
Obviously, return the last value is better then zero.

> 
> > TBH I'd consider moving these to hwmon, it's possibly a bit more
> > idiomatic to have these there than in the ALSA API where things do
> > stuff like go through and read all the controls I think?  Not sure
> > that it'd be that much happier with values that are only
> > intermittently readable though so the underlying problem remains.

Indeed, the issue was reported when someone went through and read all the
controls. People are not happy to see errors.
Moving it to hwmon sounds reasonable to me, too. But it depends on how
userspace works. I don't know how userspace uses those controls or if
userspace needs those controls.
@Ryan what is your idea?

> 
> 
> 
> --
> ~Vinod