Three drivers/i2c/ patches

Three drivers/i2c/ patches

[Jan Dittmer]

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

Hi Greg,

these are 2 patches to the drivers/i2c subdirectory.

The first patch removes a warning from i2c-algo-bit.c, which I get 
regularly with my tv cards. It's more an info than a failure and 
spamming the logs at a rate of about 1/min. It indicates a send failure 
on the i2c bus or a miss of the ACK. Technically I think, it should 
resend the message, shouldn't it?

The second patch removes the cli/sti usage from i2c-elektor. It's only 
used to protect pcf_pending from changing. I replaced it (hopefully 
correct) with a spinlock.

The third patch is actually a resend and converts w83781d to use milli 
Celsius instead of centi Celsius.

Thanks,

Jan



-- 
Linux rubicon 2.5.75-mm1-jd10 #1 SMP Sat Jul 12 19:40:28 CEST 2003 i686

[-- Attachment #2: i2c-algo-bit.remove.warning2 --]
[-- Type: text/plain, Size: 392 bytes --]

--- linux-bk/drivers/i2c/i2c-algo-bit.c	Mon May  5 01:53:31 2003
+++ 2.5.75-bk1-jd2/drivers/i2c/i2c-algo-bit.c	Sat Jul 12 23:22:01 2003
@@ -347,7 +347,6 @@
 			temp++;
 			wrcount++;
 		} else { /* arbitration or no acknowledge */
-			dev_err(&i2c_adap->dev, "sendbytes: error - bailout.\n");
 			i2c_stop(adap);
 			return (retval<0)? retval : -EFAULT;
 			        /* got a better one ?? */

[-- Attachment #3: i2c_elektor_remove_cli_sti --]
[-- Type: text/plain, Size: 1253 bytes --]

This removes cli/sti from i2c-elektor which protected pcf_pending from changing.
Replaced by a spinlock instead.

Jan

diff -urN -X exclude linux-bk/drivers/i2c/i2c-elektor.c 2.5.75-bk1/drivers/i2c/i2c-elektor.c
--- linux-bk/drivers/i2c/i2c-elektor.c	Sun Jun 15 14:04:13 2003
+++ 2.5.75-bk1/drivers/i2c/i2c-elektor.c	Sat Jul 12 11:51:34 2003
@@ -59,6 +59,8 @@
   need to be rewriten - but for now just remove this for simpler reading */
 
 static wait_queue_head_t pcf_wait;
+
+spinlock_t pcf_pending_lock = SPIN_LOCK_UNLOCKED;
 static int pcf_pending;
 
 /* ----- global defines -----------------------------------------------	*/
@@ -120,12 +122,12 @@
 	int timeout = 2;
 
 	if (irq > 0) {
-		cli();
+		spin_lock_irq(&pcf_pending_lock);
 		if (pcf_pending == 0) {
 			interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ );
 		} else
 			pcf_pending = 0;
-		sti();
+		spin_unlock_irq(&pcf_pending_lock);
 	} else {
 		udelay(100);
 	}
@@ -133,8 +135,11 @@
 
 
 static irqreturn_t pcf_isa_handler(int this_irq, void *dev_id, struct pt_regs *regs) {
+	unsigned long flags;
+	spin_lock_irqsave(&pcf_pending_lock, flags);
 	pcf_pending = 1;
 	wake_up_interruptible(&pcf_wait);
+	spin_unlock_irqrestore(&pcf_pending_lock, flags);
 	return IRQ_HANDLED;
 }
 

[-- Attachment #4: i2c.w83781d.temp --]
[-- Type: text/plain, Size: 1042 bytes --]

--- linux-mm/drivers/i2c/chips/w83781d.c	2003-07-03 15:17:37.000000000 +0200
+++ 2.5.73-mm3/drivers/i2c/chips/w83781d.c	2003-07-10 13:43:49.000000000 +0200
@@ -496,13 +496,13 @@
 	if (nr >= 2) {	/* TEMP2 and TEMP3 */ \
 		if (data->type == as99127f) { \
 			return sprintf(buf,"%ld\n", \
-				(long)AS99127_TEMP_ADD_FROM_REG(data->reg##_add[nr-2])); \
+				(long)AS99127_TEMP_ADD_FROM_REG(data->reg##_add[nr-2])*10); \
 		} else { \
 			return sprintf(buf,"%ld\n", \
-				(long)TEMP_ADD_FROM_REG(data->reg##_add[nr-2])); \
+				(long)TEMP_ADD_FROM_REG(data->reg##_add[nr-2])*10); \
 		} \
 	} else {	/* TEMP1 */ \
-		return sprintf(buf,"%ld\n", (long)TEMP_FROM_REG(data->reg)); \
+		return sprintf(buf,"%ld\n", (long)TEMP_FROM_REG(data->reg)*10); \
 	} \
 }
 show_temp_reg(temp);
@@ -516,7 +516,7 @@
 	struct w83781d_data *data = i2c_get_clientdata(client); \
 	u32 val; \
 	 \
-	val = simple_strtoul(buf, NULL, 10); \
+	val = simple_strtoul(buf, NULL, 10)/10; \
 	 \
 	if (nr >= 2) {	/* TEMP2 and TEMP3 */ \
 		if (data->type == as99127f) \

Re: Three drivers/i2c/ patches

[Christoph Hellwig]

> diff -urN -X exclude linux-bk/drivers/i2c/i2c-elektor.c 2.5.75-bk1/drivers/i2c/i2c-elektor.c
> --- linux-bk/drivers/i2c/i2c-elektor.c	Sun Jun 15 14:04:13 2003
> +++ 2.5.75-bk1/drivers/i2c/i2c-elektor.c	Sat Jul 12 11:51:34 2003
> @@ -59,6 +59,8 @@
>    need to be rewriten - but for now just remove this for simpler reading */
>  
>  static wait_queue_head_t pcf_wait;
> +
> +spinlock_t pcf_pending_lock = SPIN_LOCK_UNLOCKED;
>  static int pcf_pending;
>  
>  /* ----- global defines -----------------------------------------------	*/
> @@ -120,12 +122,12 @@
>  	int timeout = 2;
>  
>  	if (irq > 0) {
> -		cli();
> +		spin_lock_irq(&pcf_pending_lock);
>  		if (pcf_pending == 0) {
>  			interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ );
>  		} else
>  			pcf_pending = 0;
> -		sti();
> +		spin_unlock_irq(&pcf_pending_lock);

Sleeping with interrupts disabled and a spinlock held still isn't exactly
a good idea.  As is sleep_on..

Re: Three drivers/i2c/ patches

[Jan Dittmer]

Christoph Hellwig wrote:
> 
> Sleeping with interrupts disabled and a spinlock held still isn't exactly a
> good idea.  As is sleep_on..

So something like the following does make more sense?
I don't quite understand, how that code worked before - I suppose
interruptible_sleep_on_timeout activates irqs again, otherwise the interrupt 
handler would have never been called? But then, the sti() doesn't make much 
sense and should have been moved to the else path?

Thanks,

Jan

--- 2.5.75/drivers/i2c/i2c-elektor.c    2003-07-11 09:35:37.000000000 +0200
+++ 2.5.75-bk1/drivers/i2c/i2c-elektor.c        2003-07-13 12:06:06.000000000
+0200
@@ -59,6 +59,8 @@
    need to be rewriten - but for now just remove this for simpler reading */

  static wait_queue_head_t pcf_wait;
+
+spinlock_t pcf_pending_lock = SPIN_LOCK_UNLOCKED;
  static int pcf_pending;

  /* ----- global defines ----------------------------------------------- 
    */
@@ -120,12 +122,14 @@
         int timeout = 2;

         if (irq > 0) {
-               cli();
+               spin_lock_irq(&pcf_pending_lock);
                 if (pcf_pending == 0) {
+                       spin_unlock_irq(&pcf_pending_lock);
                         interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ );
-               } else
+               } else {
                         pcf_pending = 0;
-               sti();
+                       spin_unlock_irq(&pcf_pending_lock);
+               }
         } else {
                 udelay(100);
         }
@@ -133,7 +137,10 @@


  static irqreturn_t pcf_isa_handler(int this_irq, void *dev_id, struct pt_regs
*regs) {
+       unsigned long flags;
+       spin_lock_irqsave(&pcf_pending_lock, flags);
         pcf_pending = 1;
+       spin_unlock_irqrestore(&pcf_pending_lock, flags);
         wake_up_interruptible(&pcf_wait);
         return IRQ_HANDLED;
  }


-- 
Linux rubicon 2.5.75-mm1-jd10 #1 SMP Sat Jul 12 19:40:28 CEST 2003 i686

Re: Three drivers/i2c/ patches

[Greg KH]

On Sat, Jul 12, 2003 at 11:35:11PM +0200, Jan Dittmer wrote:
> Hi Greg,
> 
> these are 2 patches to the drivers/i2c subdirectory.
> 
> The first patch removes a warning from i2c-algo-bit.c, which I get 
> regularly with my tv cards. It's more an info than a failure and 
> spamming the logs at a rate of about 1/min. It indicates a send failure 
> on the i2c bus or a miss of the ACK. Technically I think, it should 
> resend the message, shouldn't it?

Can you just change this to dev_dbg() for people to still see it if they
want to?

> The second patch removes the cli/sti usage from i2c-elektor. It's only 
> used to protect pcf_pending from changing. I replaced it (hopefully 
> correct) with a spinlock.

Like Christoph said, this isn't correct.

> The third patch is actually a resend and converts w83781d to use milli 
> Celsius instead of centi Celsius.

Sorry for not getting to this previously, I have applied it and will
send it on later this evening.

thanks,

greg k-h