* Re: [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()
2004-07-23 22:09 [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout() Nishanth Aravamudan
@ 2004-07-24 1:12 ` Mark Hollomon
2004-07-24 1:41 ` Re: [Kernel-janitors] [PATCH] saa7146_i2c: replace Nish Aravamudan
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Mark Hollomon @ 2004-07-24 1:12 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: Re: [Kernel-janitors] [PATCH] saa7146_i2c: replace
2004-07-23 22:09 [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout() Nishanth Aravamudan
2004-07-24 1:12 ` Mark Hollomon
@ 2004-07-24 1:41 ` Nish Aravamudan
2004-07-25 1:10 ` [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout() Mark Hollomon
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Nish Aravamudan @ 2004-07-24 1:41 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()
2004-07-23 22:09 [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout() Nishanth Aravamudan
2004-07-24 1:12 ` Mark Hollomon
2004-07-24 1:41 ` Re: [Kernel-janitors] [PATCH] saa7146_i2c: replace Nish Aravamudan
@ 2004-07-25 1:10 ` Mark Hollomon
2004-07-26 17:29 ` Nishanth Aravamudan
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Mark Hollomon @ 2004-07-25 1:10 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()
2004-07-23 22:09 [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout() Nishanth Aravamudan
` (2 preceding siblings ...)
2004-07-25 1:10 ` [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout() Mark Hollomon
@ 2004-07-26 17:29 ` Nishanth Aravamudan
2004-07-26 20:41 ` Nishanth Aravamudan
2004-07-26 22:30 ` Nishanth Aravamudan
5 siblings, 0 replies; 7+ messages in thread
From: Nishanth Aravamudan @ 2004-07-26 17:29 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()
2004-07-23 22:09 [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout() Nishanth Aravamudan
` (3 preceding siblings ...)
2004-07-26 17:29 ` Nishanth Aravamudan
@ 2004-07-26 20:41 ` Nishanth Aravamudan
2004-07-26 22:30 ` Nishanth Aravamudan
5 siblings, 0 replies; 7+ messages in thread
From: Nishanth Aravamudan @ 2004-07-26 20:41 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout()
2004-07-23 22:09 [Kernel-janitors] [PATCH] saa7146_i2c: replace schedule_timeout() Nishanth Aravamudan
` (4 preceding siblings ...)
2004-07-26 20:41 ` Nishanth Aravamudan
@ 2004-07-26 22:30 ` Nishanth Aravamudan
5 siblings, 0 replies; 7+ messages in thread
From: Nishanth Aravamudan @ 2004-07-26 22:30 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread