[PATCH RFC] ASoC: rt5663: use msleep() for uncritical delay

[PATCH RFC] ASoC: rt5663: use msleep() for uncritical delay

[Nicholas Mc Guire]

The delay here does not seem to be critical with respect to longer
delays than 10ms as this delay is to ensure that the write took
effect before the next soc_update_bits/write call only, thus a 
high resolution timer makes little sense here - msleep() should do.

Fixes: commit df7c52168ee1 ("ASoC: add rt5663 codec driver")
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
Problem was found by cocinelle script.

msleep() of 10ms can take up to 20ms on HZ=100 systems but even this
does not seem critical here as this is not a very frequent operation
(that is not called in a loop)

In any case the delta of only 5 us seems unnecessarily small given that
jitter on almost all systems would be expected to be larger than 5us and
the microsecond precision really is not needed here.

This does throw a checkpatch warning with:
WARNING: msleep < 20ms can sleep for up to 20ms;"
but that seems ok in this case, presumably there are very few systems 
running audio subsystem that have HZ=100 set.

Patch was compile tested with: x86_64_defconfig + CONFIG_COMPILE_TEST=y
SND_SOC=m + SND_SOC_ALL_CODECS=m (implies SND_SOC_RT5663=m)

Patch is aginast 4.10-rc3 (localversion-next is next-20170111)

 sound/soc/codecs/rt5663.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/rt5663.c b/sound/soc/codecs/rt5663.c
index ed94830..2ecc600 100644
--- a/sound/soc/codecs/rt5663.c
+++ b/sound/soc/codecs/rt5663.c
@@ -2764,7 +2764,7 @@ static int rt5663_set_bias_level(struct snd_soc_codec *codec,
 			RT5663_PWR_FV1_MASK | RT5663_PWR_FV2_MASK |
 			RT5663_PWR_MB_MASK, RT5663_PWR_VREF1 |
 			RT5663_PWR_VREF2 | RT5663_PWR_MB);
-		usleep_range(10000, 10005);
+		msleep(10);
 		if (rt5663->codec_ver == CODEC_VER_1) {
 			snd_soc_update_bits(codec, RT5663_SIG_CLK_DET,
 				RT5663_EN_ANA_CLK_DET_MASK |
-- 
2.1.4

Re: [PATCH RFC] ASoC: rt5663: use msleep() for uncritical delay

[Mark Brown]

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

On Wed, Jan 11, 2017 at 04:03:51PM +0100, Nicholas Mc Guire wrote:
> The delay here does not seem to be critical with respect to longer
> delays than 10ms as this delay is to ensure that the write took
> effect before the next soc_update_bits/write call only, thus a 
> high resolution timer makes little sense here - msleep() should do.

No, that's not what the code is doing at all.

> +++ b/sound/soc/codecs/rt5663.c
> @@ -2764,7 +2764,7 @@ static int rt5663_set_bias_level(struct snd_soc_codec *codec,
>  			RT5663_PWR_FV1_MASK | RT5663_PWR_FV2_MASK |
>  			RT5663_PWR_MB_MASK, RT5663_PWR_VREF1 |
>  			RT5663_PWR_VREF2 | RT5663_PWR_MB);
> -		usleep_range(10000, 10005);
> +		msleep(10);

The write before is turning on a bunch of analogue power bits, the
enabled supplies will then take time to ramp up to their operating
state before we can proceed.  That's not just "make sure the change took
effect", there's a bit more to it than that, and power up sequences are
generally very latency sensitive as they tend to happen in response to
user input.  People are generally picking the minimum value they can
reliably get away with and a lot of effort goes into optimising the
power up procedures.

That doesn't mean that the change is bad but the analysis in the
changelog is and could cause confusion.

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

Re: [PATCH RFC] ASoC: rt5663: use msleep() for uncritical delay

[Nicholas Mc Guire]

On Tue, Jan 17, 2017 at 07:33:02PM +0000, Mark Brown wrote:
> On Wed, Jan 11, 2017 at 04:03:51PM +0100, Nicholas Mc Guire wrote:
> > The delay here does not seem to be critical with respect to longer
> > delays than 10ms as this delay is to ensure that the write took
> > effect before the next soc_update_bits/write call only, thus a 
> > high resolution timer makes little sense here - msleep() should do.
> 
> No, that's not what the code is doing at all.
> 
> > +++ b/sound/soc/codecs/rt5663.c
> > @@ -2764,7 +2764,7 @@ static int rt5663_set_bias_level(struct snd_soc_codec *codec,
> >  			RT5663_PWR_FV1_MASK | RT5663_PWR_FV2_MASK |
> >  			RT5663_PWR_MB_MASK, RT5663_PWR_VREF1 |
> >  			RT5663_PWR_VREF2 | RT5663_PWR_MB);
> > -		usleep_range(10000, 10005);
> > +		msleep(10);
> 
> The write before is turning on a bunch of analogue power bits, the
> enabled supplies will then take time to ramp up to their operating
> state before we can proceed.  That's not just "make sure the change took
> effect", there's a bit more to it than that, and power up sequences are
> generally very latency sensitive as they tend to happen in response to
> user input.  People are generally picking the minimum value they can
> reliably get away with and a lot of effort goes into optimising the
> power up procedures.
> 
> That doesn't mean that the change is bad but the analysis in the
> changelog is and could cause confusion.
ok - thanks for the explaination - you are right that I was not seeing
the actual intent of the delay here but simply took it as a I/O delay.

The change should stay valid as both msleep() and usleep_range() can
very significantly overrun their stated values and that should be safe here
(or the code has a deeper rooted problem) while the usage of the high-resolution
timers does not seem to bring any benefit here.

Will clean up the commit message and resend this as well.

thx!
hofrat