ds1620: replace schedule_timeout() with msleep()

I would appreciate any comments from the janitor@sternweltens list. This is one (of many) cases where I made a decision about replacing

set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(some_time);

with

msleep(jiffies_to_msecs(some_time));

msleep() is not exactly the same as the previous code, but I only did this replacement where I thought long delays were *desired*. If this is not the case here, then just disregard this patch.

Note: I looked for the appropriate maintainer of this driver, but I did not find anyone. If someone could tell me who that would be, I would appreciate it.

Thanks,
Nish

Description: Uses msleep() instead of schedule_timeout() to guarantee the task delays at least the desired time amount.

Signed-off-by Nishanth Aravamudan <[email protected]>

Signed-off-by Maximilian Attems <[email protected]>

---

 linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c |    3 +--
 1 files changed, 1 insertion(+), 2 deletions(-)

diff -puN drivers/char/ds1620.c~msleep-drivers_char_ds1620 drivers/char/ds1620.c

Quoted 9 lines

+++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200

drivers/char/ds1620.c renamed from drivers/char/ds1620.c~msleep-drivers_char_ds1620 +1 -2

@@ -373,8 +373,7 @@ static int __init ds1620_init(void)
 	th_start.hi = 1;
 	ds1620_write_state(&th_start);
 
-	set_current_state(TASK_INTERRUPTIBLE);
-	schedule_timeout(2*HZ);
+	msleep(2000);
 

drivers/char/ds1620.c static int __init ds1620_init(void) · line 379

Quoted 2 lines

drivers/char/ds1620.c renamed from drivers/char/ds1620.c~msleep-drivers_char_ds1620 +0 -0

 	ds1620_write_state(&th);
 

I'm wondering if 2000 is really the value we want here. As far as I can see, the schedule_timeout(2*HZ); line has been there as long back as since HZ was 100, so back then the delay would have been 200. if 200 is all it needs, then we are now sleeping 10 times as long as really needed. What is the argument behind the value used?

Quoted 9 lines · Jesper Juhl

Quoted 9 lines

+++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200

drivers/char/ds1620.c renamed from drivers/char/ds1620.c~msleep-drivers_char_ds1620 +1 -2

@@ -373,8 +373,7 @@ static int __init ds1620_init(void)
 	th_start.hi = 1;
 	ds1620_write_state(&th_start);
 
-	set_current_state(TASK_INTERRUPTIBLE);
-	schedule_timeout(2*HZ);
+	msleep(2000);
 

Quoted 7 lines

drivers/char/ds1620.c static int __init ds1620_init(void) · line 379

Quoted 2 lines

drivers/char/ds1620.c renamed from drivers/char/ds1620.c~msleep-drivers_char_ds1620 +0 -0

 	ds1620_write_state(&th);
 

I'm wondering if 2000 is really the value we want here. As far as I can see, the schedule_timeout(2*HZ); line has been there as long back as since HZ was 100, so back then the delay would have been 200. if 200 is all it needs, then we are now sleeping 10 times as long as really needed. What is the argument behind the value used?

It's right:
schedule_timeout(2*HZ) sleeps for 2 seconds;
msleep(2000) sleeps for 2000 miliseconds, and does not depend on what HZ is.

Quoted 4 lines · Domen Puncer

It's right:
schedule_timeout(2*HZ) sleeps for 2 seconds;
msleep(2000) sleeps for 2000 miliseconds, and does not depend on what HZ is.

It seems I didn't understand schedule_timeout() properly, thank you for the clarification.

Quoted 6 lines · Jesper Juhl

Quoted 4 lines · Domen Puncer

It's right:
schedule_timeout(2*HZ) sleeps for 2 seconds;
msleep(2000) sleeps for 2000 miliseconds, and does not depend on what HZ is.

Quoted 2 lines

It seems I didn't understand schedule_timeout() properly, thank you for the clarification.

As part-author of this driver, and actually of this particular bit of code, a 2 second delay is intented here. The fan needs to be run at full power in order to start running, so the idea here is to give it full power for 2 seconds and then to restore the temperature trip points to the configured values.