[PATCH] ASoC: da732x: Mark DC offset control registers volatile

[PATCH] ASoC: da732x: Mark DC offset control registers volatile

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

The driver reads from the DC offset control registers during callibration
but since the registers are marked as volatile and there is a register
cache the values will not be read from the hardware after the first reading
rendering the callibration ineffective.

It appears that the driver was originally written for the ASoC level
register I/O code but converted to regmap prior to merge and this issue
was missed during the conversion as the framework level volatile register
functionality was not being used.

Signed-off-by: Mark Brown <broonie@linaro.org>
Cc: stable@vger.kernel.org
---
 sound/soc/codecs/da732x.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/soc/codecs/da732x.c b/sound/soc/codecs/da732x.c
index 8e2c74d..798eb43 100644
--- a/sound/soc/codecs/da732x.c
+++ b/sound/soc/codecs/da732x.c
@@ -1268,11 +1268,23 @@ static struct snd_soc_dai_driver da732x_dai[] = {
 	},
 };
 
+static bool da732x_volatile(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case DA732X_REG_HPL_DAC_OFF_CNTL:
+	case DA732X_REG_HPR_DAC_OFF_CNTL:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static const struct regmap_config da732x_regmap = {
 	.reg_bits		= 8,
 	.val_bits		= 8,
 
 	.max_register		= DA732X_MAX_REG,
+	.volatile_reg		= da732x_volatile,
 	.reg_defaults		= da732x_reg_cache,
 	.num_reg_defaults	= ARRAY_SIZE(da732x_reg_cache),
 	.cache_type		= REGCACHE_RBTREE,
-- 
1.9.0.rc3

Re: [PATCH] ASoC: da732x: Mark DC offset control registers volatile

[Opensource [Adam Thomson]]

On Mon, 24 Feb 2014 03:01:37 +0000, Mark Brown wrote:

> From: Mark Brown <broonie@linaro.org>
> 
> The driver reads from the DC offset control registers during callibration
> but since the registers are marked as volatile and there is a register
> cache the values will not be read from the hardware after the first reading
> rendering the callibration ineffective.
 
Guessing that should read 'not marked as volatile'.

> It appears that the driver was originally written for the ASoC level
> register I/O code but converted to regmap prior to merge and this issue
> was missed during the conversion as the framework level volatile register
> functionality was not being used.

Yes you're correct here, unfortunately. A good spot.

> +static bool da732x_volatile(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case DA732X_REG_HPL_DAC_OFF_CNTL:
> +	case DA732X_REG_HPR_DAC_OFF_CNTL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +

Having looked over the driver again, can you include the following
registers as well: 

DA732X_REG_HPL
DA732X_REG_HPR

Also, if we're doing this then you can move to using snd_soc_read()
instead of hw_read(), I guess.

Re: [PATCH] ASoC: da732x: Mark DC offset control registers volatile

[Mark Brown]

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

On Mon, Feb 24, 2014 at 11:31:25AM +0000, Opensource [Adam Thomson] wrote:
> On Mon, 24 Feb 2014 03:01:37 +0000, Mark Brown wrote:

> > but since the registers are marked as volatile and there is a register

> Guessing that should read 'not marked as volatile'.

Yes.

> Having looked over the driver again, can you include the following
> registers as well: 

> DA732X_REG_HPL
> DA732X_REG_HPR

I did notice those.  However they are a bit more fun since they have
some non-volatile fields in them which are also used for control.  I
*suspect* that a lot of the time it'll be possible to get away with just
caching the first read but it needs further study - often with these
things the basic offset is constant in a given system so the sign may
well be right all the time.  That's just a guess, though, and may not
actually hold at which point a bit more attention might be needed.

> Also, if we're doing this then you can move to using snd_soc_read()
> instead of hw_read(), I guess.

Indeed, in fact it was while doing an audit of drivers to kill off
direct users of hw_read() that I noticed what was happening here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ASoC: da732x: Mark DC offset control registers volatile

[Opensource [Adam Thomson]]

On Mon, 24 Feb 2014 12:43:51 +0000, Mark Brown wrote:

> > Having looked over the driver again, can you include the following
> > registers as well:
> 
> > DA732X_REG_HPL
> > DA732X_REG_HPR
> 
> I did notice those.  However they are a bit more fun since they have
> some non-volatile fields in them which are also used for control.  I
> *suspect* that a lot of the time it'll be possible to get away with just
> caching the first read but it needs further study - often with these
> things the basic offset is constant in a given system so the sign may
> well be right all the time.  That's just a guess, though, and may not
> actually hold at which point a bit more attention might be needed.

The field DA732X_HP_OUT_COMPO is the only volatile field for those two
registers. Having looked at this some more, I believe the offset_cancellation
need only be done at Codec initialisation, and doesn't need to be done every time
the system resumes. Moving to a one time run may make dealing with this easier,
if we're to do something clever. However, In terms of control, really only the
MUTE, OUT_EN and OUT_HIZ_EN fields will be used with some frequency, in these
registers, and generally they will be writes which will likely cause I2C traffic
anyway. I'd be tempted to keep it simple and just make those registers volatile,
then you know it will work as expected, at least for the time being until a
better solution presents itself.

Re: [PATCH] ASoC: da732x: Mark DC offset control registers volatile

[Mark Brown]

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

On Mon, Feb 24, 2014 at 01:57:53PM +0000, Opensource [Adam Thomson] wrote:

> anyway. I'd be tempted to keep it simple and just make those registers volatile,
> then you know it will work as expected, at least for the time being until a

That breaks suspend and resume without further work - for suspend and
resume we generally rely on restoring the register cache to restore the
current settings but if a register is volatile it won't be cached so
will go back to the hardware defaults after suspend.  The ability to
avoid I2C traffic is partly just a nice side effect (though it was
needed on devices that don't have readback), the main thing these days
is that controls get efficient suspend and resume handling for free.

Refactoring the offset correction to happen once on startup would solve
the issue since the cache could just be bypassed, though you are likely
to find that there is some run to run variation for the callibration due
to effects like thermal variation and simple measurement errors.  Still,
the effects are typically very small.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ASoC: da732x: Mark DC offset control registers volatile

[Opensource [Adam Thomson]]

On Tue, 25 Feb 2014 01:22:04 +0000, Mark Brown wrote:

> That breaks suspend and resume without further work - for suspend and
> resume we generally rely on restoring the register cache to restore the
> current settings but if a register is volatile it won't be cached so
> will go back to the hardware defaults after suspend.  The ability to
> avoid I2C traffic is partly just a nice side effect (though it was
> needed on devices that don't have readback), the main thing these days
> is that controls get efficient suspend and resume handling for free.

Yes, true. Good point. 

> 
> Refactoring the offset correction to happen once on startup would solve
> the issue since the cache could just be bypassed, though you are likely
> to find that there is some run to run variation for the callibration due
> to effects like thermal variation and simple measurement errors.  Still,
> the effects are typically very small.

I have to agree, the variation won't be great. If it were then you'd see
problems for example when keeping a device awake for prolonged periods and
during fluctuations in temperature. As far as I'm aware issues like this have
not been experienced with this device so I think it's safe to make this a one
time thing at startup.

Re: [PATCH] ASoC: da732x: Mark DC offset control registers volatile

[Mark Brown]

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

On Tue, Feb 25, 2014 at 04:13:02PM +0000, Opensource [Adam Thomson] wrote:
> On Tue, 25 Feb 2014 01:22:04 +0000, Mark Brown wrote:

> > Refactoring the offset correction to happen once on startup would solve
> > the issue since the cache could just be bypassed, though you are likely
> > to find that there is some run to run variation for the callibration due
> > to effects like thermal variation and simple measurement errors.  Still,
> > the effects are typically very small.

> I have to agree, the variation won't be great. If it were then you'd see
> problems for example when keeping a device awake for prolonged periods and
> during fluctuations in temperature. As far as I'm aware issues like this have
> not been experienced with this device so I think it's safe to make this a one
> time thing at startup.

OK, so in that case how about applying the patch I sent for now (since
it should make things better for stable users) and then refactoring to
do the callibration at startup with cache bypass enabled for a proper
fix?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH] ASoC: da732x: Mark DC offset control registers volatile

[Opensource [Adam Thomson]]

> OK, so in that case how about applying the patch I sent for now (since
> it should make things better for stable users) and then refactoring to
> do the callibration at startup with cache bypass enabled for a proper
> fix?

Ok, fine.

Acked-by: Adam Thomson <Adam.Thomson.Opensource at diasemi.com>