-----Original Message----- From: Vinod Koul vkoul@kernel.org Sent: Friday, November 20, 2020 1:59 PM To: Mark Brown broonie@kernel.org Cc: Bard Liao yung-chuan.liao@linux.intel.com; tiwai@suse.de; alsa- devel@alsa-project.org; pierre-louis.bossart@linux.intel.com; ryans.lee@maximintegrated.com; kai.vehmanen@linux.intel.com; Liao, Bard bard.liao@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.
- 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