[Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()

[Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()

[Nishanth Aravamudan]

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

I would appreciate any comments from the janitors 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. 

Thanks,
Nish



Applys-to: 2.6.7

Description: Replace my_wait() with msleep() and remove defintion of
my_wait().

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>


--- linux-vanilla/drivers/media/common/saa7146_i2c.c	2004-06-15 22:20:04.000000000 -0700
+++ linux-dev/drivers/media/common/saa7146_i2c.c	2004-07-23 15:07:45.000000000 -0700
@@ -1,13 +1,6 @@
 #include <linux/version.h>
 #include <media/saa7146_vv.h>
 
-/* helper function */
-static void my_wait(struct saa7146_dev *dev, long ms)
-{
-	set_current_state(TASK_INTERRUPTIBLE);
-	schedule_timeout((((ms+10)/10)*HZ)/1000);
-}
-
 u32 saa7146_i2c_func(struct i2c_adapter *adapter)
 {
 //fm	DEB_I2C(("'%s'.\n", adapter->name));
@@ -136,12 +129,12 @@ static int saa7146_i2c_reset(struct saa7
 		/* set "ABORT-OPERATION"-bit (bit 7)*/
 		saa7146_write(dev, I2C_STATUS, (dev->i2c_bitrate | MASK_07));
 		saa7146_write(dev, MC2, (MASK_00 | MASK_16));
-		my_wait(dev,SAA7146_I2C_DELAY);
+		msleep(SAA7146_I2C_DELAY);
 
 		/* clear all error-bits pending; this is needed because p.123, note 1 */
 		saa7146_write(dev, I2C_STATUS, dev->i2c_bitrate);
 		saa7146_write(dev, MC2, (MASK_00 | MASK_16));
-		my_wait(dev,SAA7146_I2C_DELAY);
+		msleep(SAA7146_I2C_DELAY);
  	}
 
 	/* check if any error is (still) present. (this can be necessary because p.123, note 1) */
@@ -155,18 +148,18 @@ static int saa7146_i2c_reset(struct saa7
 		   after serious protocol errors caused by e.g. the SAA7740 */
 		saa7146_write(dev, I2C_STATUS, (dev->i2c_bitrate | MASK_07));
 		saa7146_write(dev, MC2, (MASK_00 | MASK_16));
-		my_wait(dev,SAA7146_I2C_DELAY);
+		msleep(SAA7146_I2C_DELAY);
 
 		/* clear all error-bits pending */
 		saa7146_write(dev, I2C_STATUS, dev->i2c_bitrate);
 		saa7146_write(dev, MC2, (MASK_00 | MASK_16));
-		my_wait(dev,SAA7146_I2C_DELAY);
+		msleep(SAA7146_I2C_DELAY);
 
 		/* the data sheet says it might be necessary to clear the status
 		   twice after an abort */
 		saa7146_write(dev, I2C_STATUS, dev->i2c_bitrate);
 		saa7146_write(dev, MC2, (MASK_00 | MASK_16));
-		my_wait(dev,SAA7146_I2C_DELAY);
+		msleep(SAA7146_I2C_DELAY);
      	}
 
 	/* if any error is still present, a fatal error has occured ... */
@@ -243,7 +236,7 @@ static int saa7146_i2c_writeout(struct s
 			if ((++trial < 20) && short_delay)
 				udelay(10);
 			else
-			my_wait(dev,1);
+			msleep(1);
 		}
 	}
 
@@ -345,7 +338,7 @@ int saa7146_i2c_transfer(struct saa7146_
 		}
 	        
 	        /* delay a bit before retrying */
-	        my_wait(dev, 10);
+	        msleep(10);
 		
 	} while (err != num && retries--);
 

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()

[Mark Hollomon]

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

I think this patch will result in delays 10 times too long.


Nishanth Aravamudan wrote:
>  
> -/* helper function */
> -static void my_wait(struct saa7146_dev *dev, long ms)
> -{
> -	set_current_state(TASK_INTERRUPTIBLE);
> -	schedule_timeout((((ms+10)/10)*HZ)/1000);
> -}

ms seems to be a number that expresses an interval in units of 1/10 of a 
millisecond.

> -
>  u32 saa7146_i2c_func(struct i2c_adapter *adapter)
>  {
>  //fm	DEB_I2C(("'%s'.\n", adapter->name));
> @@ -136,12 +129,12 @@ static int saa7146_i2c_reset(struct saa7
>  		/* set "ABORT-OPERATION"-bit (bit 7)*/
>  		saa7146_write(dev, I2C_STATUS, (dev->i2c_bitrate | MASK_07));
>  		saa7146_write(dev, MC2, (MASK_00 | MASK_16));
> -		my_wait(dev,SAA7146_I2C_DELAY);
> +		msleep(SAA7146_I2C_DELAY);

But this would use it as a delay in milliseconds.



-- 
--------------------------
Mark Hollomon

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: Re: [Kernel-janitors] [PATCH] saa7146_i2c: replace

[Nish Aravamudan]

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

On Fri, 23 Jul 2004 21:12:25 -0400, Mark Hollomon
<markhollomon@comcast.net> wrote:
> I think this patch will result in delays 10 times too long.
> 
> 
> Nishanth Aravamudan wrote:
> >
<snip> 
>
> ms seems to be a number that expresses an interval in units of 1/10 of a
> millisecond.
> 
<snip>
> 
> But this would use it as a delay in milliseconds.

Ok, thanks, I wasn't sure about this - I had posted a while ago about
the weird conversion because I couldn't quite decipher what it was
intended to achieve. Looking over the math, I think I agree with you.
I will re-submit this patch soon. BTW, doesn't that seem kind of
confusing? - The delays are in terms of 1, 5 and 10 tenths of a
millisecond? If that's the case, then this patch is pretty much
useless (it makes sense to use msleep() with long delays, things
countable in msecs). I suppose I could re-patch so that the value of
SAA7146_I2C_DELAY is in msecs and then use msecs_to_jiffies() in
my_wait(). Thanks again!

-Nish

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()

[Mark Hollomon]

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

Nish Aravamudan wrote:

> 
> Ok, thanks, I wasn't sure about this - I had posted a while ago about
> the weird conversion because I couldn't quite decipher what it was
> intended to achieve. Looking over the math, I think I agree with you.
> I will re-submit this patch soon. BTW, doesn't that seem kind of
> confusing? - The delays are in terms of 1, 5 and 10 tenths of a
> millisecond? If that's the case, then this patch is pretty much
> useless (it makes sense to use msleep() with long delays, things
> countable in msecs). I suppose I could re-patch so that the value of
> SAA7146_I2C_DELAY is in msecs and then use msecs_to_jiffies() in
> my_wait(). Thanks again!

Very confusing. Also, a millisecond is added to the delay. So, if you 
pass in 10, the expression will partially evaluate to 2 * HZ / 1000 
which is effectively 2 milliseconds.


But I suspect the +10 in the original code was simply to ensure that the 
delay was never zero. I don't remember if gcc does all the constant 
folding in float and truncate at the end or if it truncates as it goes. 
But if it truncates partial results then ms/10 will be zero if  ms < 10.

Alright, a simple compile test proves that the evaluation is done 
completely as an integer. [ eg ((5/10)*10) == 0 ].

So passing either 1 or 5 to my_wait will get you a millisecond delay. 
Passing 10 in will get you a 2 millisecond delay.

If you wanted to be cheap, you could make the guts of my_wait be simply
	msleep((ms+10)/10)
and let somebody else figure out what was really meant.






-- 
--------------------------
Mark Hollomon

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()

[Nishanth Aravamudan]

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

On Sat, Jul 24, 2004 at 09:10:27PM -0400, Mark Hollomon wrote:
> Nish Aravamudan wrote:
> 
> >
> >Ok, thanks, I wasn't sure about this - I had posted a while ago about
> >the weird conversion because I couldn't quite decipher what it was
> >intended to achieve. Looking over the math, I think I agree with you.
> >I will re-submit this patch soon. BTW, doesn't that seem kind of
> >confusing? - The delays are in terms of 1, 5 and 10 tenths of a
> >millisecond? If that's the case, then this patch is pretty much
> >useless (it makes sense to use msleep() with long delays, things
> >countable in msecs). I suppose I could re-patch so that the value of
> >SAA7146_I2C_DELAY is in msecs and then use msecs_to_jiffies() in
> >my_wait(). Thanks again!
> 
> Very confusing. Also, a millisecond is added to the delay. So, if you 
> pass in 10, the expression will partially evaluate to 2 * HZ / 1000 
> which is effectively 2 milliseconds.
> 
> 
> But I suspect the +10 in the original code was simply to ensure that the 
> delay was never zero. I don't remember if gcc does all the constant 
> folding in float and truncate at the end or if it truncates as it goes. 
> But if it truncates partial results then ms/10 will be zero if  ms < 10.

<snip>

> If you wanted to be cheap, you could make the guts of my_wait be simply
> 	msleep((ms+10)/10)
> and let somebody else figure out what was really meant.

Ok, I think I understand the math now, after doing a bit of messing with
it myself. I agree that the +10 is just a rounding fix, which is pretty
common in schedule_timeout() calls. I have used msecs_to_jiffies() in
the corrected patch below to avoid some of this confusion, as it will
take care of the rounding. Thus, I am invoking msleep() on (ms/10),
which should correctly express the parameter as tenths of a millisecond.
Thanks for your help, Mark. 

-Nish

Note: This patch supersedes the previous saa7146_i2c one. Thanks.

Applys-to: 2.6.7

Description: Uses msleep() instead of schedule_timeout() to guarantee
the task delays the desired time. Also replaces the conversion between
msecs and jiffies with msecs_to_jiffies(). 

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>


--- linux-vanilla/drivers/media/common/saa7146_i2c.c	2004-06-15 22:20:04.000000000 -0700
+++ linux-dev/drivers/media/common/saa7146_i2c.c	2004-07-26 10:27:26.000000000 -0700
@@ -1,11 +1,11 @@
 #include <linux/version.h>
 #include <media/saa7146_vv.h>
 
-/* helper function */
+/* helper function
+   Note: ms is in terms of tenths of a msec */
 static void my_wait(struct saa7146_dev *dev, long ms)
 {
-	set_current_state(TASK_INTERRUPTIBLE);
-	schedule_timeout((((ms+10)/10)*HZ)/1000);
+	msleep(msecs_to_jiffies(ms/10));
 }
 
 u32 saa7146_i2c_func(struct i2c_adapter *adapter)

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()

[Nishanth Aravamudan]

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

Not quite corrected enough. Please see below. Thanks again, Domen.

On Sat, Jul 24, 2004 at 09:10:27PM -0400, Mark Hollomon wrote:
> Nish Aravamudan wrote:
> 
> >
> >Ok, thanks, I wasn't sure about this - I had posted a while ago about
> >the weird conversion because I couldn't quite decipher what it was
> >intended to achieve. Looking over the math, I think I agree with you.
> >I will re-submit this patch soon. BTW, doesn't that seem kind of
> >confusing? - The delays are in terms of 1, 5 and 10 tenths of a
> >millisecond? If that's the case, then this patch is pretty much
> >useless (it makes sense to use msleep() with long delays, things
> >countable in msecs). I suppose I could re-patch so that the value of
> >SAA7146_I2C_DELAY is in msecs and then use msecs_to_jiffies() in
> >my_wait(). Thanks again!
> 
> Very confusing. Also, a millisecond is added to the delay. So, if you 
> pass in 10, the expression will partially evaluate to 2 * HZ / 1000 
> which is effectively 2 milliseconds.
> 
> 
> But I suspect the +10 in the original code was simply to ensure that the 
> delay was never zero. I don't remember if gcc does all the constant 
> folding in float and truncate at the end or if it truncates as it goes. 
> But if it truncates partial results then ms/10 will be zero if  ms < 10.

<snip>

> If you wanted to be cheap, you could make the guts of my_wait be simply
> 	msleep((ms+10)/10)
> and let somebody else figure out what was really meant.

Ok, I think I understand the math now, after doing a bit of messing with
it myself. I agree that the +10 is just a rounding fix, which is pretty
common in schedule_timeout() calls. I have used msecs_to_jiffies() in
the corrected patch below to avoid some of this confusion, as it will
take care of the rounding. Thus, I am invoking msleep() on (ms/10),
which should correctly express the parameter as tenths of a millisecond.
Thanks for your help, Mark. 

-Nish

Note: This patch supersedes the previous saa7146_i2c one. Thanks.

Applys-to: 2.6.7

Description: Uses msleep() instead of schedule_timeout() to guarantee
the task delays the desired time. Also replaces the conversion between
msecs and jiffies with msecs_to_jiffies(). 

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>


--- linux-vanilla/drivers/media/common/saa7146_i2c.c	2004-06-15 22:20:04.000000000 -0700
+++ linux-dev/drivers/media/common/saa7146_i2c.c	2004-07-26 10:27:26.000000000 -0700
@@ -1,11 +1,11 @@
 #include <linux/version.h>
 #include <media/saa7146_vv.h>
 
-/* helper function */
+/* helper function
+   Note: ms is in terms of tenths of a msec */
 static void my_wait(struct saa7146_dev *dev, long ms)
 {
-	set_current_state(TASK_INTERRUPTIBLE);
-	schedule_timeout((((ms+10)/10)*HZ)/1000);
+	msleep(ms/10);
 }
 
 u32 saa7146_i2c_func(struct i2c_adapter *adapter)

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()

[Nishanth Aravamudan]

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

Another fix thanks to Mark Hollomon. The call

msleep(ms/10);

will result in rounding errors due to the integer evaluation of ms/10
before the msleep() call. This leads to 0ms timeouts. Therefore, I have
reinserted the 1msec offset. Please find the correction below and sorry
for the repeated repeated patches.

Not quite corrected enough. Please see below. Thanks again, Domen.

On Sat, Jul 24, 2004 at 09:10:27PM -0400, Mark Hollomon wrote:
> Nish Aravamudan wrote:
> 
> >
> >Ok, thanks, I wasn't sure about this - I had posted a while ago about
> >the weird conversion because I couldn't quite decipher what it was
> >intended to achieve. Looking over the math, I think I agree with you.
> >I will re-submit this patch soon. BTW, doesn't that seem kind of
> >confusing? - The delays are in terms of 1, 5 and 10 tenths of a
> >millisecond? If that's the case, then this patch is pretty much
> >useless (it makes sense to use msleep() with long delays, things
> >countable in msecs). I suppose I could re-patch so that the value of
> >SAA7146_I2C_DELAY is in msecs and then use msecs_to_jiffies() in
> >my_wait(). Thanks again!
> 
> Very confusing. Also, a millisecond is added to the delay. So, if you 
> pass in 10, the expression will partially evaluate to 2 * HZ / 1000 
> which is effectively 2 milliseconds.
> 
> 
> But I suspect the +10 in the original code was simply to ensure that the 
> delay was never zero. I don't remember if gcc does all the constant 
> folding in float and truncate at the end or if it truncates as it goes. 
> But if it truncates partial results then ms/10 will be zero if  ms < 10.

<snip>

> If you wanted to be cheap, you could make the guts of my_wait be simply
> 	msleep((ms+10)/10)
> and let somebody else figure out what was really meant.

Ok, I think I understand the math now, after doing a bit of messing with
it myself. I agree that the +10 is just a rounding fix, which is pretty
common in schedule_timeout() calls. I have used msecs_to_jiffies() in
the corrected patch below to avoid some of this confusion, as it will
take care of the rounding. Thus, I am invoking msleep() on (ms/10),
which should correctly express the parameter as tenths of a millisecond.
Thanks for your help, Mark. 

-Nish

Note: This patch supersedes the previous saa7146_i2c one. Thanks.

Applys-to: 2.6.7

Description: Uses msleep() instead of schedule_timeout() to guarantee
the task delays the desired time. Also replaces the conversion between
msecs and jiffies with msecs_to_jiffies(). 

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>


--- linux-vanilla/drivers/media/common/saa7146_i2c.c	2004-06-15 22:20:04.000000000 -0700
+++ linux-dev/drivers/media/common/saa7146_i2c.c	2004-07-26 10:27:26.000000000 -0700
@@ -1,11 +1,11 @@
 #include <linux/version.h>
 #include <media/saa7146_vv.h>
 
-/* helper function */
+/* helper function
+   Note: ms is in terms of tenths of a msec */
 static void my_wait(struct saa7146_dev *dev, long ms)
 {
-	set_current_state(TASK_INTERRUPTIBLE);
-	schedule_timeout((((ms+10)/10)*HZ)/1000);
+	msleep((ms+10)/10);
 }
 
 u32 saa7146_i2c_func(struct i2c_adapter *adapter)

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors