| Message ID | 1393210853-32246-1-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 75306820248e26d15d84acf4e297b9fb27dd3bb2 |
| Headers | show |
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.
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.
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.
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.
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.
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?
> 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>
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,