i2c-i801: Add basic interrupt support

This patch adds basic interrupt support to the i2c-i801 driver.

Signed-off-by Ivo Manca <[email protected]>

---

i2c-i801.c renamed from i2c-i801.c.git +16 -0

 #define I801_BYTE		0x04
 #define I801_BYTE_DATA		0x08
 #define I801_WORD_DATA		0x0C

@@ -121,23 +123,65 @@
 #define SMBHSTSTS_INTR		0x02
 #define SMBHSTSTS_HOST_BUSY	0x01
 
+/* mask for events we normally handle */
+#define I801_HST_STS_MASK_NORM ( \
+		SMBHSTSTS_FAILED | \
+		SMBHSTSTS_BUS_ERR | \
+		SMBHSTSTS_DEV_ERR | \
+		SMBHSTSTS_INTR)
+
+/* mask for all events */
+#define I801_HST_STS_MASK_ALL ( \
+		SMBHSTSTS_BYTE_DONE | \
+		SMBHSTSTS_SMBALERT_STS | \
+		SMBHSTSTS_FAILED | \
+		SMBHSTSTS_BUS_ERR | \
+		SMBHSTSTS_DEV_ERR | \
+		SMBHSTSTS_INTR)
+
 static unsigned long i801_smba;

i2c-i801.c renamed from i2c-i801.c.git +20 -0

+};
+
+static struct i2c_i801_algo_data i801_algo_data;
+
 #define FEATURE_SMBUS_PEC	(1 << 0)
 #define FEATURE_BLOCK_BUFFER	(1 << 1)
 #define FEATURE_BLOCK_PROC	(1 << 2)
 #define FEATURE_I2C_BLOCK_READ	(1 << 3)
 static unsigned int i801_features;
 
+/* fetch & consume host status out of algo_data */
+static inline int i801_get_status(struct i2c_i801_algo_data *algo_data)
+{
+	unsigned long flags;
+	int status;
+
+	spin_lock_irqsave(&algo_data->lock, flags);
+	status = algo_data->status;
+	algo_data->status = 0;
+	spin_unlock_irqrestore(&algo_data->lock, flags);
+
+	return status;
+}
+
 static int i801_transaction(int xact)
 {
 	int temp;
 	int result = 0;
 	int timeout = 0;
 
+	struct i2c_i801_algo_data *algo_data = i801_adapter.algo_data;
+
 	dev_dbg(&I801_dev->dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
 		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),
 		inb_p(SMBHSTCMD), inb_p(SMBHSTADD), inb_p(SMBHSTDAT0),

i2c-i801.c renamed from i2c-i801.c.git +2 -0

+	} else {
 	outb_p(xact | I801_START, SMBHSTCNT);
 
 	/* We will always wait for a fraction of a second! */

@@ -177,6 +228,7 @@ static int i801_transaction(int xact)
 		msleep(1);
 		outb_p(inb_p(SMBHSTCNT) & (~SMBHSTCNT_KILL), SMBHSTCNT);
 	}
+	}
 
 	if (temp & SMBHSTSTS_FAILED) {
 		result = -1;

i2c-i801.c renamed from i2c-i801.c.git +30 -0

 };
 
 static struct pci_device_id i801_ids[] = {

@@ -597,8 +650,38 @@ static struct pci_device_id i801_ids[] =
 
 MODULE_DEVICE_TABLE (pci, i801_ids);
 
+static irqreturn_t i801_isr(int irq, void *dev_id)
+{
+	u8 status = inb(SMBHSTSTS);
+
+	/* bail if it's not ours */
+	if (!(status & I801_HST_STS_MASK_ALL)) {
+		dev_dbg(&I801_dev->dev, "BAILING interrupt\n");
+		return IRQ_NONE;
+	}
+
+	dev_dbg(&I801_dev->dev, "INTERRUPT: IRQ status: 0x%02x\n", status);
+
+	/* ACK */
+	outb((status & I801_HST_STS_MASK_ALL), SMBHSTSTS);
+
+	if (status & I801_HST_STS_MASK_NORM) {
+		struct i2c_adapter *adap = dev_id;
+		struct i2c_i801_algo_data *algo_data = adap->algo_data;
+		unsigned long flags;
+
+		spin_lock_irqsave(&algo_data->lock, flags);
+		algo_data->status = status;
+		spin_unlock_irqrestore(&algo_data->lock, flags);
+		wake_up_interruptible(&algo_data->waitq);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int __devinit i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
+	struct i2c_i801_algo_data *algo_data = i801_adapter.algo_data;
 	unsigned char temp;
 	int err;
 

@@ -656,10 +739,27 @@ static int __devinit i801_probe(struct p
 	}
 	pci_write_config_byte(I801_dev, SMBHSTCFG, temp);

i2c-i801.c renamed from i2c-i801.c.git +14 -0

+			dev_err(&dev->dev,
+				"request irq %d failed!\n", I801_dev->irq);
+			algo_data->irq = -1;
+		} else {
+			dev_dbg(&dev->dev,
+				"SMBus base address: 0x%04lx, IRQ: %d\n",
+				i801_smba, I801_dev->irq);
+
+			algo_data->irq = I801_dev->irq;
+			algo_data->status = 0;
+			init_waitqueue_head(&algo_data->waitq);
+			spin_lock_init(&algo_data->lock);
+		}
+	}
 
 	/* Clear special mode bits */
 	if (i801_features & (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER))

Please provide patches that can be applied with patch -p1 from the root of the kernel source tree.

Having this here is a bit confusing, you should leave all the transaction types (from I801_QUICK to I801_I2C_BLOCK_LAST) as a block.

I801_HST_CNT_INTREN (which could probably be renamed to just I801_INTREN for brevity) would go first.

Also note that the drivers already had a define for this flag, named ENABLE_INT9, although it was disabled. I agree that I801_INTREN is a better name, but it would probably be a good idea to delete ENABLE_INT9 and all references thereto first, otherwise the code becomes a little confusing.

You never really use the irq value, only whether it is -1 or not. So it might make more sense to turn it into a flag and rename it to use_irq?

Please make this HZ/2 a define so that it's easy to change.

Ideally this define would replace MAX_TIMEOUT (in a separate patch) so that both
the poll-based and the interrupt-driven paths have the same timeout value. Right now,
the timeout handling of the poll-based path is rather ugly (the actual timeout depends on
the value of HZ.)

Not sure why you make this wait interruptible. The poll-based path isn't interruptible, and you do not handle the interrupted case anyway.

The polled-based path has additional logic to handle the timeout case (by resetting the controller.) I would guess you want the same for the interrupt-driven path?

Please add a trailing comma.

i801_adapter.name is a rather long string, I think I'd rather use i801_driver.name to register the IRQ.

I think you have a race here. You free the irq before removing the i2c adapter. What will happen if someone initiates an I2C transaction after the IRQ has been freed? It would probably be better to remove the i2c adapter first.

Other than that, it looks good to me, good work. Note though that I am in no way an expert of interrupt handling, as this is the first PC-style SMBus controller driver which gets converted to use interrupts instead of polling.

I'm not an expert, but I think this is the right way to use it. The interupt handler i801_isr catches the interrupt, saves the status to algo_data and calls wake_up_interruptable. This will wake up the wait.

I'm not sure whether this is the correct way or not. Any input from an interrupt expert is very much welcome.
I did test it without the event_interruptable, which produces nonsense, like:

bla bla

Hey Jean,

Quoted 7 lines

Ideally this define would replace MAX_TIMEOUT (in a separate patch) so that both
the poll-based and the interrupt-driven paths have the same timeout value. Right now,
the timeout handling of the poll-based path is rather ugly (the actual timeout depends on
the value of HZ.)

Hm, seems like a very sensible thing to do. However, I have no idea how to implement that, since you don't really know how long msleep slept, or not? I sadly don't really have time to look into this right now, sorry.

(...)
I'll send you an updated patch later this evening. I don't know anything about config files though, so that'll have to wait a bit... The module parameter will be added though.