Re: [PATCH] ASoC: sta32x: add workaround for ESD reset issue

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Nov 09, 2011 at 06:34:19PM +0100, Johannes Stezenbach wrote:
> On Wed, Nov 09, 2011 at 02:36:47PM +0000, Mark Brown wrote:

> > So, clearly a constant poll isn't going to do power consumption or
> > anything any favours so we shouldn't be doing this by default.  The most
> > obvious idea is to only check while audio is active (since it doesn't
> > really matter if the device is reset while not playing audio anyway)
> > which would get rid of most of the problem.

> Um, well, the sta32x is a power amplifier, it draws 3mA
> even when in power down.  I think a poll once per second
> won't add measurable power consumption, especially since
> there are other wakeup sources present in the system with
> higher wakeup frequency.  I acknowledge that your are right
> in principle, but since the sta32x won't be used in battery-powered
> mobile devices I think it is not crucial to improve this.  Do you agree?

I'd still rather avoid it.

> > Given that the driver already supports powering the device down it may
> > also be sufficient to simply enable idle_bias_off and assume that if the
> > device resets the application will get sufficiently upset to restart
> > things anyway.

> Unfortunately the app won't notice, the PXA I2S interface will
> happily push the audio data to the codec which will be muted
> without the app knowing it since the register cache will tell
> something different.

Yeah, but if it suddenly stops in the middle of playback then that'll
tend to register with users and if you keep it powered off at all other
times.

> > If we do need to poll or anything else invasive I'd also expect this to
> > be optional as if the device has ESD weaknesses then it'd be likely that
> > boards would add external ESD protection which 

> OK, I'll add a module parameter.  Raumfeld engineers claim their
> hw design is fine as is, of course...

Platform data seems more sensible, it's board level breakage from the
sounds of it.

> > > +	for (i = 0; i < numcoef && (index + (i + 1) * 3 < STA32X_COEF_COUNT); i++)
> > > +		sta32x->coef_shadow[index + i] =
> > > +			  (ucontrol->value.bytes.data[3 * i    ] << 16)
> > > +			| (ucontrol->value.bytes.data[3 * i + 1] << 8)
> > > +			| (ucontrol->value.bytes.data[3 * i + 2]);

> > Does this need to be done when restoring to _STANDBY as well?

> No, all registers and coefs are preserved until hw reset or power loss.

Right, but the bias level management is disabling the regulators for the
device so power loss may happen then and _STANDBY is also used to resume
the device after suspend when power loss may also occur.

> > > +	/* mute during restore */
> > > +	snd_soc_cache_read(codec, STA32X_MMUTE, &mute);
> > > +	snd_soc_write(codec, STA32X_MMUTE, mute | STA32X_MMUTE_MMUTE);

> > Just use snd_soc_read() and let the cache do the right thing.

> hm, I think snd_soc_cache_read() is more explicit; but OK, will change

It's not particularly important that you read the cache here, you just
want to update the value in the register.  If this happens to not
involve hardware I/O that's nice but not essential.