linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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;
as well as URLs for NNTP newsgroup(s).