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

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

[Michael Hunold]

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

Hello Nishanth,

I'm sorry for the long delay. As I already explained in the mail I sent 
to linux-dvb-maintainer (yes, I'm working on both projects) some mails 
were "caught" by my spam filter and then we had a power failure in our 
company which resulted in a huge mail overload.

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

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

I admit that this "my_wait" functions is totally bogus. It's obsolete 
and it's obviously not doing what's expected (ie. wait some milliseconds).

It's only used in the time-uncritical i2c code of my saa7146 driver. 
When I wrote that code, I obviously wanted to wait a certain amount of ms.

So please do me a favour and nuke that function completely and simply 
replace it with msleep(). It has worked with the broken code before, now 
only the waits will be 10 times longer for some error paths. The normal 
code paths are not affected because either irq based i2c transactions 
are used or tight udelay() loops are performed.

Is msleep() already available in 2.6.x?
How will these patches be included in 2.6?
Who is going to post patches to whom?
Do you need me to sign-off the saa7146 patches or should I prepare and 
send them instead?

CU
Michael.

[-- 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] Re: [PATCH] saa7146_i2c: replace

[Nishanth Aravamudan]

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

On Tue, Jul 27, 2004 at 04:44:36PM +0200, Michael Hunold wrote:
> Hello Nishanth,
> 
> I'm sorry for the long delay. As I already explained in the mail I sent 
> to linux-dvb-maintainer (yes, I'm working on both projects) some mails 
> were "caught" by my spam filter and then we had a power failure in our 
> company which resulted in a huge mail overload.
> 
> >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.
> 
> >--- 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);
> > }
> 
> I admit that this "my_wait" functions is totally bogus. It's obsolete 
> and it's obviously not doing what's expected (ie. wait some milliseconds).
> 
> It's only used in the time-uncritical i2c code of my saa7146 driver. 
> When I wrote that code, I obviously wanted to wait a certain amount of ms.
> 
> So please do me a favour and nuke that function completely and simply 
> replace it with msleep(). It has worked with the broken code before, now 
> only the waits will be 10 times longer for some error paths. The normal 
> code paths are not affected because either irq based i2c transactions 
> are used or tight udelay() loops are performed.

Here is this patch:

Applys-to: 2.6.7

Description: Uses msleep() instead of my_wait() to guarantee the time
delay. Removes definition 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-27 09:07:15.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(dev,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--);
 


> 
> Is msleep() already available in 2.6.x?
> How will these patches be included in 2.6?
> Who is going to post patches to whom?
> Do you need me to sign-off the saa7146 patches or should I prepare and 
> send them instead?
> 
> CU
> Michael.

[-- 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] Re: [PATCH] saa7146_i2c: replace

[Nishanth Aravamudan]

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

On Tue, Jul 27, 2004 at 04:44:36PM +0200, Michael Hunold wrote:
> Hello Nishanth,
> 
> I'm sorry for the long delay. As I already explained in the mail I sent 
> to linux-dvb-maintainer (yes, I'm working on both projects) some mails 
> were "caught" by my spam filter and then we had a power failure in our 
> company which resulted in a huge mail overload.
> 
> >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.
> 
> >--- 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);
> > }

<snip> 

> Is msleep() already available in 2.6.x?

I am not sure when it was added, but it definitely is in 2.6.7, as this
is what the patch is applied against.

> How will these patches be included in 2.6?
> Who is going to post patches to whom?
> Do you need me to sign-off the saa7146 patches or should I prepare and 
> send them instead?

I will have to defer to Randy Dunlap, who posted about this a bit ago:

The module/subsystem maintainer can elect to merge it on his/her own
(is that you?) or the KJ maintainer (Max Attems) will forward them
if they "pass".

If ther are better answer(er)s out there, hopefully they will post soon.

-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

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

[Nishanth Aravamudan]

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

On Tue, Jul 27, 2004 at 09:10:52AM -0700, Nishanth Aravamudan wrote:

<snip>

> Here is this patch:
> 
> Applys-to: 2.6.7
> 
> Description: Uses msleep() instead of my_wait() to guarantee the time
> delay. Removes definition of my_wait().
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

Sorry, this previous patch will not compile due to a typo. Find the
correct version below, thanks.


--- 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-28 10:42:41.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