* [patch 18/25] ds1620: replace schedule_timeout() with msleep()
@ 2004-09-01 20:57 janitor
2004-11-28 17:39 ` Jesper Juhl
0 siblings, 1 reply; 6+ messages in thread
From: janitor @ 2004-09-01 20:57 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, janitor
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 <nacc@us.ibm.com>
Signed-off-by: Maximilian Attems <janitor@sternwelten.at>
---
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
--- linux-2.6.9-rc1-bk7/drivers/char/ds1620.c~msleep-drivers_char_ds1620 2004-09-01 19:34:43.000000000 +0200
+++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200
@@ -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);
ds1620_write_state(&th);
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 18/25] ds1620: replace schedule_timeout() with msleep()
2004-09-01 20:57 [patch 18/25] ds1620: replace schedule_timeout() with msleep() janitor
@ 2004-11-28 17:39 ` Jesper Juhl
2004-11-29 14:09 ` Domen Puncer
0 siblings, 1 reply; 6+ messages in thread
From: Jesper Juhl @ 2004-11-28 17:39 UTC (permalink / raw)
To: janitor; +Cc: linux-kernel, akpm
On Wed, 1 Sep 2004 janitor@sternwelten.at wrote:
>
> 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 <nacc@us.ibm.com>
> Signed-off-by: Maximilian Attems <janitor@sternwelten.at>
>
>
>
> ---
>
> 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
> --- linux-2.6.9-rc1-bk7/drivers/char/ds1620.c~msleep-drivers_char_ds1620 2004-09-01 19:34:43.000000000 +0200
> +++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200
> @@ -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);
>
> 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?
--
Jesper Juhl <juhl-lkml@dif.dk>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ds1620: replace schedule_timeout() with msleep()
2004-11-28 17:39 ` Jesper Juhl
@ 2004-11-29 14:09 ` Domen Puncer
2004-11-29 22:37 ` Jesper Juhl
0 siblings, 1 reply; 6+ messages in thread
From: Domen Puncer @ 2004-11-29 14:09 UTC (permalink / raw)
To: Jesper Juhl; +Cc: janitor, linux-kernel, akpm
On 28/11/04 18:39 +0100, Jesper Juhl wrote:
> > +++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200
> > @@ -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);
> >
> > 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.
Domen
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ds1620: replace schedule_timeout() with msleep()
2004-11-29 14:09 ` Domen Puncer
@ 2004-11-29 22:37 ` Jesper Juhl
2004-11-29 22:42 ` Russell King
0 siblings, 1 reply; 6+ messages in thread
From: Jesper Juhl @ 2004-11-29 22:37 UTC (permalink / raw)
To: Domen Puncer; +Cc: janitor, linux-kernel, akpm
On Mon, 29 Nov 2004, Domen Puncer wrote:
> On 28/11/04 18:39 +0100, Jesper Juhl wrote:
> > > +++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200
> > > @@ -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);
> > >
> > > 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.
>
It seems I didn't understand schedule_timeout() properly, thank you for
the clarification.
--
Jesper
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ds1620: replace schedule_timeout() with msleep()
2004-11-29 22:37 ` Jesper Juhl
@ 2004-11-29 22:42 ` Russell King
2004-11-30 0:10 ` Jesper Juhl
0 siblings, 1 reply; 6+ messages in thread
From: Russell King @ 2004-11-29 22:42 UTC (permalink / raw)
To: Jesper Juhl; +Cc: Domen Puncer, janitor, linux-kernel, akpm
On Mon, Nov 29, 2004 at 11:37:48PM +0100, Jesper Juhl wrote:
> On Mon, 29 Nov 2004, Domen Puncer wrote:
> > 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.
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.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: ds1620: replace schedule_timeout() with msleep()
2004-11-29 22:42 ` Russell King
@ 2004-11-30 0:10 ` Jesper Juhl
0 siblings, 0 replies; 6+ messages in thread
From: Jesper Juhl @ 2004-11-30 0:10 UTC (permalink / raw)
To: Russell King; +Cc: Domen Puncer, janitor, linux-kernel, akpm
On Mon, 29 Nov 2004, Russell King wrote:
> On Mon, Nov 29, 2004 at 11:37:48PM +0100, Jesper Juhl wrote:
> > On Mon, 29 Nov 2004, Domen Puncer wrote:
> > > 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.
>
> 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.
>
That makes sense - thanks.
--
Jesper
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-11-30 0:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-09-01 20:57 [patch 18/25] ds1620: replace schedule_timeout() with msleep() janitor
2004-11-28 17:39 ` Jesper Juhl
2004-11-29 14:09 ` Domen Puncer
2004-11-29 22:37 ` Jesper Juhl
2004-11-29 22:42 ` Russell King
2004-11-30 0:10 ` Jesper Juhl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox