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

[Vinod Koul]

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

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



-- 
~Vinod