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

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

[Nicholas Mc Guire]

As the overall delay is in range of seconds and the wait granularity
is 10ms msleep() seems more reasonable than to put load on the highres
timer subsystem.

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

This does throw a checkpatch warning with:
WARNING: msleep < 20ms can sleep for up to 20ms;"
but since this is in a loop and the intended granularity is
10ms with up to 2 seconds total delay this seems ok.

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 | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/rt5663.c b/sound/soc/codecs/rt5663.c
index a32508d..3bdb62b 100644
--- a/sound/soc/codecs/rt5663.c
+++ b/sound/soc/codecs/rt5663.c
@@ -2982,7 +2982,8 @@ static void rt5663_v2_calibrate(struct rt5663_priv *rt5663)
 
 static void rt5663_calibrate(struct rt5663_priv *rt5663)
 {
-	int value, count;
+	int value;
+	unsigned long timeout;
 
 	regmap_write(rt5663->regmap, RT5663_RC_CLK, 0x0280);
 	regmap_write(rt5663->regmap, RT5663_GLB_CLK, 0x8000);
@@ -3025,17 +3026,15 @@ static void rt5663_calibrate(struct rt5663_priv *rt5663)
 	regmap_write(rt5663->regmap, RT5663_HP_CALIB_3, 0x06c2);
 	regmap_write(rt5663->regmap, RT5663_HP_CALIB_1_1, 0x7b00);
 	regmap_write(rt5663->regmap, RT5663_HP_CALIB_1_1, 0xfb00);
-	count = 0;
-	while (true) {
+
+	/* allow the write completion to take up to 2s */
+	timeout = jiffies + msecs_to_jiffies(2000);
+	while (time_is_after_jiffies(timeout)) {
 		regmap_read(rt5663->regmap, RT5663_HP_CALIB_1_1, &value);
 		if (value & 0x8000)
-			usleep_range(10000, 10005);
+			msleep(10);
 		else
 			break;
-
-		if (count > 200)
-			return;
-		count++;
 	}
 }
 
-- 
2.1.4

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

[Mark Brown]

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

On Wed, Jan 11, 2017 at 04:03:13PM +0100, Nicholas Mc Guire wrote:
> As the overall delay is in range of seconds and the wait granularity
> is 10ms msleep() seems more reasonable than to put load on the highres
> timer subsystem.

Thia also contains a whole bunch of other refactoring that's not
mentioned in the changelog...

> This does throw a checkpatch warning with:
> WARNING: msleep < 20ms can sleep for up to 20ms;"
> but since this is in a loop and the intended granularity is
> 10ms with up to 2 seconds total delay this seems ok.

That does depend on how quickly the operation is expected to complete,
the total delay is likely to be a massive overestimate.

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

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

[Nicholas Mc Guire]

On Tue, Jan 17, 2017 at 07:34:34PM +0000, Mark Brown wrote:
> On Wed, Jan 11, 2017 at 04:03:13PM +0100, Nicholas Mc Guire wrote:
> > As the overall delay is in range of seconds and the wait granularity
> > is 10ms msleep() seems more reasonable than to put load on the highres
> > timer subsystem.
> 
> Thia also contains a whole bunch of other refactoring that's not
> mentioned in the changelog...
>

true - initially it was the transition to msleep and then I 
switched from counter based to time-based loop. Im actually not sure
if switching to time-based loop vs counter based loop is desired. My
assumption is that if the system was under high load then this loop
could take very long to actually timeout - which is probably not
what is desired and in the good case the behavior should not
actually be significantly different.
 
> > This does throw a checkpatch warning with:
> > WARNING: msleep < 20ms can sleep for up to 20ms;"
> > but since this is in a loop and the intended granularity is
> > 10ms with up to 2 seconds total delay this seems ok.
> 
> That does depend on how quickly the operation is expected to complete,
> the total delay is likely to be a massive overestimate.

the initial granularity of checking is 10ms (usleep_range(10000,10005))
and the loop is probing in this 10ms interval. Now msleep can overrun
a call by 10ms (in case HZ=100) but not in a loop, in the loop case
consecutive msleep(10) would not each overrun as the condition is that
you start the sleep just after jiffies was updated and then have to wait
until it changes by 2 (msleep() internally adding 1) so the effective
delays are comparable but dont use highresolution timers here.
The behavior is thus basically unchanged independent of the expected 
time of operation as it imediately probes (regmap_read()) and then
will wait at least 10ms in any case - and that behavior does not change.

Will go fix up the commit log and resend.