[alsa-devel] [PATCH RFC] ASoC: rt5663: use msleep() for long delay loop
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++; } }
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.
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.
participants (3)
-
Mark Brown -
Nicholas Mc Guire -
Nicholas Mc Guire