saa7146_i2c: replace schedule_timeout()

PATCH In review Nishanth Aravamudan 3 participants 7 messages Jul 23, 2004 – Jul 26, 2004

Subsystems (2)

Media (V4L/DVB) Kernel janitors

lore

Share & tools

lore

Nishanth Aravamudan

author

2004-07-23 22:09 UTC #0 lore raw

PATCH 1/1 saa7146_i2c: replace schedule_timeout()

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 <[email protected]>

drivers/media/common/saa7146_i2c.c +7 ~~-14~~

@@ -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--);

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Mark Hollomon

2004-07-24 01:12 UTC #1 lore raw

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

drivers/media/common/saa7146_i2c.c line 9

Quoted 7 lines · Nishanth Aravamudan

drivers/media/common/saa7146_i2c.c +0 -6

-/* 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.

drivers/media/common/saa7146_i2c.c static int saa7146_i2c_reset(struct saa7 · line 132

Quoted 10 lines

drivers/media/common/saa7146_i2c.c +1 -2

-
 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.

Signature · 2 lines

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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Nish Aravamudan

2004-07-24 01:41 UTC #2 lore raw

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

In reply to Nishanth Aravamudan

Quoted 5 lines · Mark Hollomon

I think this patch will result in delays 10 times too long.
Quoted 1 line · Nishanth Aravamudan

<snip>

Quoted 4 lines

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

<snip>

Quoted 2 lines

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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Mark Hollomon

2004-07-25 01:10 UTC #3 lore raw

In reply to Nishanth Aravamudan

Quoted 11 lines · Nish Aravamudan

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.

Signature · 2 lines

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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

author

2004-07-26 17:29 UTC #4 lore raw

PATCH 1/1 saa7146_i2c: replace schedule_timeout()

In reply to Nishanth Aravamudan

Quoted 23 lines · Mark Hollomon

Quoted 11 lines · Nish Aravamudan
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>

Quoted 3 lines

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 <[email protected]>

drivers/media/common/saa7146_i2c.c +3 -3

@@ -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)

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

author

2004-07-26 20:41 UTC #5 lore raw

PATCH 1/1 saa7146_i2c: replace schedule_timeout()

In reply to Nishanth Aravamudan

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

Quoted 23 lines · Mark Hollomon

Quoted 11 lines · Nish Aravamudan
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>

Quoted 3 lines

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.

-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 <[email protected]>

drivers/media/common/saa7146_i2c.c +3 -3

@@ -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)

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

author

2004-07-26 22:30 UTC #6 lore raw

PATCH 1/1 saa7146_i2c: replace schedule_timeout()

In reply to Nishanth Aravamudan

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.

Quoted 23 lines · Mark Hollomon

Quoted 11 lines · Nish Aravamudan
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>

Quoted 3 lines

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.

-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 <[email protected]>

drivers/media/common/saa7146_i2c.c +3 -3

@@ -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)

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors