[PATCH] timer clean up for i2c-keywest.c

[PATCH] timer clean up for i2c-keywest.c

[Paul Mackerras]

This patch changes i2c-keywest.c to use mod_timer instead of a
two-line sequence to compute .expires and call add_timer in 3 places.
Without this patch I get a BUG from time to time in add_timer.

Paul.

diff -urN linux-2.5/drivers/i2c/i2c-keywest.c pmac-2.5/drivers/i2c/i2c-keywest.c
--- linux-2.5/drivers/i2c/i2c-keywest.c	2003-05-07 14:25:43.000000000 +1000
+++ pmac-2.5/drivers/i2c/i2c-keywest.c	2003-06-15 13:26:59.000000000 +1000
@@ -220,10 +220,8 @@
 	spin_lock(&iface->lock);
 	del_timer(&iface->timeout_timer);
 	handle_interrupt(iface, read_reg(reg_isr));
-	if (iface->state != state_idle) {
-		iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
-		add_timer(&iface->timeout_timer);
-	}
+	if (iface->state != state_idle)
+		mod_timer(&iface->timeout_timer, jiffies + POLL_TIMEOUT);
 	spin_unlock(&iface->lock);
 	return IRQ_HANDLED;
 }
@@ -331,8 +329,7 @@
 		write_reg(reg_subaddr, command);
 
 	/* Arm timeout */
-	iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
-	add_timer(&iface->timeout_timer);
+	mod_timer(&iface->timeout_timer, jiffies + POLL_TIMEOUT);
 
 	/* Start sending address & enable interrupt*/
 	write_reg(reg_control, read_reg(reg_control) | KW_I2C_CTL_XADDR);
@@ -421,8 +418,7 @@
 			((iface->read_write == I2C_SMBUS_READ) ? 0x01 : 0x00));
 
 		/* Arm timeout */
-		iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
-		add_timer(&iface->timeout_timer);
+		mod_timer(&iface->timeout_timer, jiffies + POLL_TIMEOUT);
 
 		/* Start sending address & enable interrupt*/
 		write_reg(reg_control, read_reg(reg_control) | KW_I2C_CTL_XADDR);

Re: [PATCH] timer clean up for i2c-keywest.c

[Benjamin Herrenschmidt]

On Mon, 2003-07-07 at 09:03, Paul Mackerras wrote:
> This patch changes i2c-keywest.c to use mod_timer instead of a
> two-line sequence to compute .expires and call add_timer in 3 places.
> Without this patch I get a BUG from time to time in add_timer.

Hrm... I did a different fix for this problem in 2.4, patch
against 2.5 coming in a few minutes...

Ben.

Re: [PATCH] timer clean up for i2c-keywest.c

[Benjamin Herrenschmidt]

On Mon, 2003-07-07 at 09:03, Paul Mackerras wrote:
> This patch changes i2c-keywest.c to use mod_timer instead of a
> two-line sequence to compute .expires and call add_timer in 3 places.
> Without this patch I get a BUG from time to time in add_timer.

Ok, here it is. It also remove the never used "polled" mode. The
driver is now in sync with the more up-to-date 2.4 version ;)

Sorry for not sending that earlier, I forgot about it and didn't
notice it was out of sync.

Ben.

===== drivers/i2c/i2c-keywest.c 1.2 vs edited =====
--- 1.2/drivers/i2c/i2c-keywest.c	Wed Apr 23 13:32:32 2003
+++ edited/drivers/i2c/i2c-keywest.c	Tue Jul  8 12:16:51 2003
@@ -66,8 +66,6 @@
 
 #include "i2c-keywest.h"
 
-#undef POLLED_MODE
-
 #define DBG(x...) do {\
 	if (debug > 0) \
 		printk(KERN_DEBUG "KW:" x); \
@@ -85,27 +83,6 @@
 
 static struct keywest_iface *ifaces = NULL;
 
-#ifdef POLLED_MODE
-/* This isn't fast, but will go once I implement interrupt with
- * proper timeout
- */
-static u8
-wait_interrupt(struct keywest_iface* iface)
-{
-	int i;
-	u8 isr;
-	
-	for (i = 0; i < POLL_TIMEOUT; i++) {
-		isr = read_reg(reg_isr) & KW_I2C_IRQ_MASK;
-		if (isr != 0)
-			return isr;
-		current->state = TASK_UNINTERRUPTIBLE;
-		schedule_timeout(1);
-	}
-	return isr;
-}
-#endif /* POLLED_MODE */
-
 
 static void
 do_stop(struct keywest_iface* iface, int result)
@@ -116,16 +93,17 @@
 }
 
 /* Main state machine for standard & standard sub mode */
-static void
+static int
 handle_interrupt(struct keywest_iface *iface, u8 isr)
 {
 	int ack;
+	int rearm_timer = 1;
 	
 	DBG("handle_interrupt(), got: %x, status: %x, state: %d\n",
 		isr, read_reg(reg_status), iface->state);
 	if (isr == 0 && iface->state != state_stop) {
 		do_stop(iface, -1);
-		return;
+		return rearm_timer;
 	}
 	if (isr & KW_I2C_IRQ_STOP && iface->state != state_stop) {
 		iface->result = -1;
@@ -196,20 +174,19 @@
 		if (!(isr & KW_I2C_IRQ_STOP) && (++iface->stopretry) < 10)
 			do_stop(iface, -1);
 		else {
+			rearm_timer = 0;
 			iface->state = state_idle;
 			write_reg(reg_control, 0x00);
 			write_reg(reg_ier, 0x00);
-#ifndef POLLED_MODE
 			complete(&iface->complete);
-#endif /* POLLED_MODE */			
 		}
 		break;
 	}
 	
 	write_reg(reg_isr, isr);
-}
 
-#ifndef POLLED_MODE
+	return rearm_timer;
+}
 
 /* Interrupt handler */
 static irqreturn_t
@@ -219,8 +196,7 @@
 
 	spin_lock(&iface->lock);
 	del_timer(&iface->timeout_timer);
-	handle_interrupt(iface, read_reg(reg_isr));
-	if (iface->state != state_idle) {
+	if (handle_interrupt(iface, read_reg(reg_isr))) {
 		iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
 		add_timer(&iface->timeout_timer);
 	}
@@ -235,16 +211,13 @@
 
 	DBG("timeout !\n");
 	spin_lock_irq(&iface->lock);
-	handle_interrupt(iface, read_reg(reg_isr));
-	if (iface->state != state_idle) {
+	if (handle_interrupt(iface, read_reg(reg_isr))) {
 		iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
 		add_timer(&iface->timeout_timer);
 	}
 	spin_unlock(&iface->lock);
 }
 
-#endif /* POLLED_MODE */
-
 /*
  * SMBUS-type transfer entrypoint
  */
@@ -338,17 +311,7 @@
 	write_reg(reg_control, read_reg(reg_control) | KW_I2C_CTL_XADDR);
 	write_reg(reg_ier, KW_I2C_IRQ_MASK);
 
-#ifdef POLLED_MODE
-	DBG("using polled mode...\n");
-	/* State machine, to turn into an interrupt handler */
-	while(iface->state != state_idle) {
-		u8 isr = wait_interrupt(iface);
-		handle_interrupt(iface, isr);
-	}
-#else /* POLLED_MODE */
-	DBG("using interrupt mode...\n");
 	wait_for_completion(&iface->complete);	
-#endif /* POLLED_MODE */	
 
 	rc = iface->result;	
 	DBG("transfer done, result: %d\n", rc);
@@ -428,17 +391,7 @@
 		write_reg(reg_control, read_reg(reg_control) | KW_I2C_CTL_XADDR);
 		write_reg(reg_ier, KW_I2C_IRQ_MASK);
 
-#ifdef POLLED_MODE
-		DBG("using polled mode...\n");
-		/* State machine, to turn into an interrupt handler */
-		while(iface->state != state_idle) {
-			u8 isr = wait_interrupt(iface);
-			handle_interrupt(iface, isr);
-		}
-#else /* POLLED_MODE */
-		DBG("using interrupt mode...\n");
 		wait_for_completion(&iface->complete);	
-#endif /* POLLED_MODE */	
 
 		rc = iface->result;
 		if (rc == 0)
@@ -540,8 +493,8 @@
 			*prate);
 	}
 	
-	/* Select standard sub mode */
-	iface->cur_mode |= KW_I2C_MODE_STANDARDSUB;
+	/* Select standard mode by default */
+	iface->cur_mode |= KW_I2C_MODE_STANDARD;
 	
 	/* Write mode */
 	write_reg(reg_mode, iface->cur_mode);
@@ -550,7 +503,6 @@
 	write_reg(reg_ier, 0x00);
 	write_reg(reg_isr, KW_I2C_IRQ_MASK);
 
-#ifndef POLLED_MODE
 	/* Request chip interrupt */	
 	rc = request_irq(iface->irq, keywest_irq, 0, "keywest i2c", iface);
 	if (rc) {
@@ -559,7 +511,6 @@
 		kfree(iface);
 		return -ENODEV;
 	}
-#endif /* POLLED_MODE */
 
 	for (i=0; i<nchan; i++) {
 		struct keywest_chan* chan = &iface->channels[i];
@@ -609,19 +560,16 @@
 
 	/* Make sure we stop all activity */
 	down(&iface->sem);
-#ifndef POLLED_MODE
 	spin_lock_irq(&iface->lock);
 	while (iface->state != state_idle) {
 		spin_unlock_irq(&iface->lock);
-		schedule();
+		set_task_state(current,TASK_UNINTERRUPTIBLE);
+		schedule_timeout(HZ/10);
 		spin_lock_irq(&iface->lock);
 	}
-#endif /* POLLED_MODE */
 	iface->state = state_dead;
-#ifndef POLLED_MODE
 	spin_unlock_irq(&iface->lock);
 	free_irq(iface->irq, iface);
-#endif /* POLLED_MODE */
 	up(&iface->sem);
 
 	/* Release all channels */

Re: [PATCH] timer clean up for i2c-keywest.c

[Benjamin Herrenschmidt]

On Tue, 2003-07-08 at 12:26, Benjamin Herrenschmidt wrote:
> On Mon, 2003-07-07 at 09:03, Paul Mackerras wrote:
> > This patch changes i2c-keywest.c to use mod_timer instead of a
> > two-line sequence to compute .expires and call add_timer in 3 places.
> > Without this patch I get a BUG from time to time in add_timer.
> 
> Ok, here it is. It also remove the never used "polled" mode. The
> driver is now in sync with the more up-to-date 2.4 version ;)
> 
> Sorry for not sending that earlier, I forgot about it and didn't
> notice it was out of sync.

And just in case you didn't merge it yet... Here's a version changing
the timer->expire ; add_timer() pairs into calls to mod_timer, makes
the code slighly cleaner.

Ben.

===== drivers/i2c/i2c-keywest.c 1.2 vs edited =====
--- 1.2/drivers/i2c/i2c-keywest.c	Wed Apr 23 13:32:32 2003
+++ edited/drivers/i2c/i2c-keywest.c	Wed Jul  9 11:45:34 2003
@@ -66,8 +66,6 @@
 
 #include "i2c-keywest.h"
 
-#undef POLLED_MODE
-
 #define DBG(x...) do {\
 	if (debug > 0) \
 		printk(KERN_DEBUG "KW:" x); \
@@ -85,27 +83,6 @@
 
 static struct keywest_iface *ifaces = NULL;
 
-#ifdef POLLED_MODE
-/* This isn't fast, but will go once I implement interrupt with
- * proper timeout
- */
-static u8
-wait_interrupt(struct keywest_iface* iface)
-{
-	int i;
-	u8 isr;
-	
-	for (i = 0; i < POLL_TIMEOUT; i++) {
-		isr = read_reg(reg_isr) & KW_I2C_IRQ_MASK;
-		if (isr != 0)
-			return isr;
-		current->state = TASK_UNINTERRUPTIBLE;
-		schedule_timeout(1);
-	}
-	return isr;
-}
-#endif /* POLLED_MODE */
-
 
 static void
 do_stop(struct keywest_iface* iface, int result)
@@ -116,16 +93,17 @@
 }
 
 /* Main state machine for standard & standard sub mode */
-static void
+static int
 handle_interrupt(struct keywest_iface *iface, u8 isr)
 {
 	int ack;
+	int rearm_timer = 1;
 	
 	DBG("handle_interrupt(), got: %x, status: %x, state: %d\n",
 		isr, read_reg(reg_status), iface->state);
 	if (isr == 0 && iface->state != state_stop) {
 		do_stop(iface, -1);
-		return;
+		return rearm_timer;
 	}
 	if (isr & KW_I2C_IRQ_STOP && iface->state != state_stop) {
 		iface->result = -1;
@@ -196,20 +174,19 @@
 		if (!(isr & KW_I2C_IRQ_STOP) && (++iface->stopretry) < 10)
 			do_stop(iface, -1);
 		else {
+			rearm_timer = 0;
 			iface->state = state_idle;
 			write_reg(reg_control, 0x00);
 			write_reg(reg_ier, 0x00);
-#ifndef POLLED_MODE
 			complete(&iface->complete);
-#endif /* POLLED_MODE */			
 		}
 		break;
 	}
 	
 	write_reg(reg_isr, isr);
-}
 
-#ifndef POLLED_MODE
+	return rearm_timer;
+}
 
 /* Interrupt handler */
 static irqreturn_t
@@ -219,11 +196,8 @@
 
 	spin_lock(&iface->lock);
 	del_timer(&iface->timeout_timer);
-	handle_interrupt(iface, read_reg(reg_isr));
-	if (iface->state != state_idle) {
-		iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
-		add_timer(&iface->timeout_timer);
-	}
+	if (handle_interrupt(iface, read_reg(reg_isr)))
+		mod_timer(&iface->timeout_timer, jiffies + POLL_TIMEOUT);
 	spin_unlock(&iface->lock);
 	return IRQ_HANDLED;
 }
@@ -235,16 +209,11 @@
 
 	DBG("timeout !\n");
 	spin_lock_irq(&iface->lock);
-	handle_interrupt(iface, read_reg(reg_isr));
-	if (iface->state != state_idle) {
-		iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
-		add_timer(&iface->timeout_timer);
-	}
+	if (handle_interrupt(iface, read_reg(reg_isr)))
+		mod_timer(&iface->timeout_timer, jiffies + POLL_TIMEOUT);
 	spin_unlock(&iface->lock);
 }
 
-#endif /* POLLED_MODE */
-
 /*
  * SMBUS-type transfer entrypoint
  */
@@ -331,24 +300,13 @@
 		write_reg(reg_subaddr, command);
 
 	/* Arm timeout */
-	iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
-	add_timer(&iface->timeout_timer);
+	mod_timer(&iface->timeout_timer, jiffies + POLL_TIMEOUT);
 
 	/* Start sending address & enable interrupt*/
 	write_reg(reg_control, read_reg(reg_control) | KW_I2C_CTL_XADDR);
 	write_reg(reg_ier, KW_I2C_IRQ_MASK);
 
-#ifdef POLLED_MODE
-	DBG("using polled mode...\n");
-	/* State machine, to turn into an interrupt handler */
-	while(iface->state != state_idle) {
-		u8 isr = wait_interrupt(iface);
-		handle_interrupt(iface, isr);
-	}
-#else /* POLLED_MODE */
-	DBG("using interrupt mode...\n");
 	wait_for_completion(&iface->complete);	
-#endif /* POLLED_MODE */	
 
 	rc = iface->result;	
 	DBG("transfer done, result: %d\n", rc);
@@ -421,24 +379,13 @@
 			((iface->read_write == I2C_SMBUS_READ) ? 0x01 : 0x00));
 
 		/* Arm timeout */
-		iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
-		add_timer(&iface->timeout_timer);
+		mod_timer(&iface->timeout_timer, jiffies + POLL_TIMEOUT);
 
 		/* Start sending address & enable interrupt*/
 		write_reg(reg_control, read_reg(reg_control) | KW_I2C_CTL_XADDR);
 		write_reg(reg_ier, KW_I2C_IRQ_MASK);
 
-#ifdef POLLED_MODE
-		DBG("using polled mode...\n");
-		/* State machine, to turn into an interrupt handler */
-		while(iface->state != state_idle) {
-			u8 isr = wait_interrupt(iface);
-			handle_interrupt(iface, isr);
-		}
-#else /* POLLED_MODE */
-		DBG("using interrupt mode...\n");
 		wait_for_completion(&iface->complete);	
-#endif /* POLLED_MODE */	
 
 		rc = iface->result;
 		if (rc == 0)
@@ -540,8 +487,8 @@
 			*prate);
 	}
 	
-	/* Select standard sub mode */
-	iface->cur_mode |= KW_I2C_MODE_STANDARDSUB;
+	/* Select standard mode by default */
+	iface->cur_mode |= KW_I2C_MODE_STANDARD;
 	
 	/* Write mode */
 	write_reg(reg_mode, iface->cur_mode);
@@ -550,7 +497,6 @@
 	write_reg(reg_ier, 0x00);
 	write_reg(reg_isr, KW_I2C_IRQ_MASK);
 
-#ifndef POLLED_MODE
 	/* Request chip interrupt */	
 	rc = request_irq(iface->irq, keywest_irq, 0, "keywest i2c", iface);
 	if (rc) {
@@ -559,7 +505,6 @@
 		kfree(iface);
 		return -ENODEV;
 	}
-#endif /* POLLED_MODE */
 
 	for (i=0; i<nchan; i++) {
 		struct keywest_chan* chan = &iface->channels[i];
@@ -609,19 +554,16 @@
 
 	/* Make sure we stop all activity */
 	down(&iface->sem);
-#ifndef POLLED_MODE
 	spin_lock_irq(&iface->lock);
 	while (iface->state != state_idle) {
 		spin_unlock_irq(&iface->lock);
-		schedule();
+		set_task_state(current,TASK_UNINTERRUPTIBLE);
+		schedule_timeout(HZ/10);
 		spin_lock_irq(&iface->lock);
 	}
-#endif /* POLLED_MODE */
 	iface->state = state_dead;
-#ifndef POLLED_MODE
 	spin_unlock_irq(&iface->lock);
 	free_irq(iface->irq, iface);
-#endif /* POLLED_MODE */
 	up(&iface->sem);
 
 	/* Release all channels */

Re: [PATCH] timer clean up for i2c-keywest.c

[Greg KH]

On Wed, Jul 09, 2003 at 11:48:08AM +0200, Benjamin Herrenschmidt wrote:
> On Tue, 2003-07-08 at 12:26, Benjamin Herrenschmidt wrote:
> > On Mon, 2003-07-07 at 09:03, Paul Mackerras wrote:
> > > This patch changes i2c-keywest.c to use mod_timer instead of a
> > > two-line sequence to compute .expires and call add_timer in 3 places.
> > > Without this patch I get a BUG from time to time in add_timer.
> > 
> > Ok, here it is. It also remove the never used "polled" mode. The
> > driver is now in sync with the more up-to-date 2.4 version ;)
> > 
> > Sorry for not sending that earlier, I forgot about it and didn't
> > notice it was out of sync.
> 
> And just in case you didn't merge it yet... Here's a version changing
> the timer->expire ; add_timer() pairs into calls to mod_timer, makes
> the code slighly cleaner.

Thanks, I've applied this one.

greg k-h