tlv320aic3x audio driver -- possible bug

tlv320aic3x audio driver -- possible bug

[Frederick Gotham]

In the C source file "tlv320aic3x" in the function "aic3x_set_power",
when the argument for power is 0, the hardware registers are saved to
the Linux computer as follows:

    regcache_cache_only(aic3x->regmap, true);

And then later when "aic3x_set_power" is called with power set to 1,
the previous values are then restored back to their hardware
registers:

     regcache_cache_only(aic3x->regmap, false);
     regcache_sync(aic3x->regmap);

When the device is powered back on, the previous setting for
microphone bias (MICBIAS) is lost. If it was previously set to 2 volts
or 2.5 volts, then when it's powered back on it becomes Off.

I'm not very familiar with audio codec chips and how they work, but I
think maybe the order of function calls in the "else" branch might be
incorrect. Right now the code looks like this:

		/*
		 * Do soft reset to this codec instance in order to clear
		 * possible VDD leakage currents in case the supply regulators
		 * remain on
		 */
		snd_soc_component_write(component, AIC3X_RESET, SOFT_RESET);
		regcache_mark_dirty(aic3x->regmap);
		aic3x->power = 0;
		/* HW writes are needless when bias is off */
		regcache_cache_only(aic3x->regmap, true);
		ret = regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies),
					     aic3x->supplies);

However I think that the call to "regcache_cache_only" should come
before the soft reset takeS place. The setting for MICBIAS is lost at
this line:

    snd_soc_component_write(component, AIC3X_RESET, SOFT_RESET);

And so I think that the call to "regcache_cache_only" should be moved
up above "snd_soc_component_write", as follows:


		/*
		 * Do soft reset to this codec instance in order to clear
		 * possible VDD leakage currents in case the supply regulators
		 * remain on
		 */
		regcache_cache_only(aic3x->regmap, true);
		snd_soc_component_write(component, AIC3X_RESET, SOFT_RESET);
		regcache_mark_dirty(aic3x->regmap);
		aic3x->power = 0;
		ret = regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies),
					     aic3x->supplies);

Re: tlv320aic3x audio driver -- possible bug

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]

On Fri, Jul 23, 2021 at 02:29:51PM +0100, Frederick Gotham wrote:

> When the device is powered back on, the previous setting for
> microphone bias (MICBIAS) is lost. If it was previously set to 2 volts
> or 2.5 volts, then when it's powered back on it becomes Off.

If this is configured in a volatile register (I don't know off hand)
then it needs to be specifically saved and restored separately, I know
this is an issue with the accessory detection - I've got a patch from
talking to someone else about an issue they were having there.

> However I think that the call to "regcache_cache_only" should come
> before the soft reset takeS place. The setting for MICBIAS is lost at
> this line:

>     snd_soc_component_write(component, AIC3X_RESET, SOFT_RESET);

> And so I think that the call to "regcache_cache_only" should be moved
> up above "snd_soc_component_write", as follows:

> 		/*
> 		 * Do soft reset to this codec instance in order to clear
> 		 * possible VDD leakage currents in case the supply regulators
> 		 * remain on
> 		 */
> 		regcache_cache_only(aic3x->regmap, true);
> 		snd_soc_component_write(component, AIC3X_RESET, SOFT_RESET);

I don't understand how you think this will fix things - we're still
powering off the regulators (if they're controllable or if that happens
over suspend) so we'll still loose all the register state and exactly
the same thing will happen when we try to restore, at best this will
mean the reset that's being done there doesn't actually happens so we've
got the risk of having higher power consumption over suspend if the
supplies are maintained over suspend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]