From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nishanth Aravamudan Date: Mon, 26 Jul 2004 22:30:39 +0000 Subject: [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout() Message-Id: <20040726223039.GG1897@us.ibm.com> MIME-Version: 1 Content-Type: multipart/mixed; boundary="===============28241050696293613==" List-Id: References: <20040723220936.GQ2675@us.ibm.com> In-Reply-To: <20040723220936.GQ2675@us.ibm.com> To: kernel-janitors@vger.kernel.org --===============28241050696293613== Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. > 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 --- 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 #include -/* 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) --===============28241050696293613== Content-Type: text/plain; charset="iso-8859-1" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline _______________________________________________ Kernel-janitors mailing list Kernel-janitors@lists.osdl.org http://lists.osdl.org/mailman/listinfo/kernel-janitors --===============28241050696293613==--