[PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

From: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>

This is a rewrite of large parts of the driver mainly so that it uses
SMBus interrupts to offload the CPU from busy-waiting on status inputs.
As a part of the overhaul of the init and exit calls, all accesses to the
hardware got converted to use accessory functions via an ioremap() cookie.

[mattst88] Added BCM1480 interrupts and rebased minimally.

Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
---
This is the second version of this patch that I've sent. This version
fixes the problem with the ENXIO return.

 drivers/i2c/busses/i2c-sibyte.c |  296 +++++++++++++++++++++++++++++---------
 1 files changed, 226 insertions(+), 70 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sibyte.c b/drivers/i2c/busses/i2c-sibyte.c
index 0fe505d..d2f1cf1 100644
--- a/drivers/i2c/busses/i2c-sibyte.c
+++ b/drivers/i2c/busses/i2c-sibyte.c
@@ -2,6 +2,7 @@
  * Copyright (C) 2004 Steven J. Hill
  * Copyright (C) 2001,2002,2003 Broadcom Corporation
  * Copyright (C) 1995-2000 Simon G. Vogl
+ * Copyright (C) 2008 Maciej W. Rozycki
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -18,104 +19,164 @@
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
  */
 
+#include <linux/errno.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/param.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/wait.h>
 #include <linux/io.h>
+#include <asm/sibyte/sb1250_int.h>
 #include <asm/sibyte/sb1250_regs.h>
 #include <asm/sibyte/sb1250_smbus.h>
+#include <asm/sibyte/bcm1480_int.h>
 
 
 struct i2c_algo_sibyte_data {
-	void *data;		/* private data */
-	int   bus;		/* which bus */
-	void *reg_base;		/* CSR base */
+	wait_queue_head_t	wait;		/* IRQ queue */
+	void __iomem		*csr;		/* mapped CSR handle */
+	phys_t			base;		/* physical CSR base */
+	char			*name;		/* IRQ handler name */
+	spinlock_t		lock;		/* atomiser */
+	int			irq;		/* IRQ line */
+	int			status;		/* IRQ status */
 };
 
-/* ----- global defines ----------------------------------------------- */
-#define SMB_CSR(a,r) ((long)(a->reg_base + r))
 
+static irqreturn_t i2c_sibyte_interrupt(int irq, void *dev_id)
+{
+	struct i2c_adapter *i2c_adap = dev_id;
+	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+	u8 status;
+
+	/*
+	 * Ugh, no way to detect the finish interrupt,
+	 * but if busy it is obviously not one.
+	 */
+	status = __raw_readq(csr + R_SMB_STATUS);
+	if ((status & (M_SMB_ERROR | M_SMB_BUSY)) == M_SMB_BUSY)
+		return IRQ_NONE;
+
+	/*
+	 * Clear the error interrupt (write 1 to clear);
+	 * the finish interrupt was cleared by the read above.
+	 */
+	__raw_writeq(status, csr + R_SMB_STATUS);
+
+	/* Post the status. */
+	spin_lock(&adap->lock);
+	adap->status = status & (M_SMB_ERROR_TYPE | M_SMB_ERROR | M_SMB_BUSY);
+	wake_up(&adap->wait);
+	spin_unlock(&adap->lock);
+
+	return IRQ_HANDLED;
+}
 
-static int smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
-		      unsigned short flags, char read_write,
-		      u8 command, int size, union i2c_smbus_data * data)
+static s32 i2c_sibyte_smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
+				 unsigned short cflags,
+				 char read_write, u8 command, int size,
+				 union i2c_smbus_data *data)
 {
 	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+	unsigned long flags;
 	int data_bytes = 0;
 	int error;
 
-	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
-		;
+	spin_lock_irqsave(&adap->lock, flags);
+
+	if (adap->status < 0) {
+		error = -EIO;
+		goto out_unlock;
+	}
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		csr_out32((V_SMB_ADDR(addr) |
-			   (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
-			   V_SMB_TT_QUICKCMD), SMB_CSR(adap, R_SMB_START));
+		__raw_writeq(V_SMB_ADDR(addr) |
+			     (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
+			     V_SMB_TT_QUICKCMD,
+			     csr + R_SMB_START);
 		break;
 	case I2C_SMBUS_BYTE:
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 1;
 		} else {
-			csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
+		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 1;
 		} else {
-			csr_out32(V_SMB_LB(data->byte),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_LB(data->byte), csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	case I2C_SMBUS_WORD_DATA:
-		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
+		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 2;
 		} else {
-			csr_out32(V_SMB_LB(data->word & 0xff),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32(V_SMB_MB(data->word >> 8),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_LB(data->word & 0xff),
+				     csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_MB(data->word >> 8),
+				     csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	default:
-		return -EOPNOTSUPP;
+		error = -EOPNOTSUPP;
+		goto out_unlock;
 	}
+	mmiowb();
+	__raw_readq(csr + R_SMB_START);
+	adap->status = -1;
+
+	spin_unlock_irqrestore(&adap->lock, flags);
 
-	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
-		;
+	wait_event_timeout(adap->wait, (adap->status >= 0), HZ);
 
-	error = csr_in32(SMB_CSR(adap, R_SMB_STATUS));
-	if (error & M_SMB_ERROR) {
-		/* Clear error bit by writing a 1 */
-		csr_out32(M_SMB_ERROR, SMB_CSR(adap, R_SMB_STATUS));
-		return (error & M_SMB_ERROR_TYPE) ? -EIO : -ENXIO;
+	spin_lock_irqsave(&adap->lock, flags);
+
+	if (adap->status > 0 && ((adap->status & (M_SMB_ERROR | M_SMB_ERROR_TYPE)) == M_SMB_ERROR)) {
+		error = -ENXIO;
+		goto out_unlock;
+	}
+	if (adap->status < 0 || (adap->status & (M_SMB_ERROR | M_SMB_BUSY))) {
+		error = -EIO;
+		goto out_unlock;
 	}
 
 	if (data_bytes == 1)
-		data->byte = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xff;
+		data->byte = __raw_readq(csr + R_SMB_DATA) & 0xff;
 	if (data_bytes == 2)
-		data->word = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xffff;
+		data->word = __raw_readq(csr + R_SMB_DATA) & 0xffff;
 
-	return 0;
+	error = 0;
+
+out_unlock:
+	spin_unlock_irqrestore(&adap->lock, flags);
+
+	return error;
 }
 
-static u32 bit_func(struct i2c_adapter *adap)
+static u32 i2c_sibyte_bit_func(struct i2c_adapter *adap)
 {
 	return (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA);
@@ -125,8 +186,8 @@ static u32 bit_func(struct i2c_adapter *adap)
 /* -----exported algorithm data: -------------------------------------	*/
 
 static const struct i2c_algorithm i2c_sibyte_algo = {
-	.smbus_xfer	= smbus_xfer,
-	.functionality	= bit_func,
+	.smbus_xfer	= i2c_sibyte_smbus_xfer,
+	.functionality	= i2c_sibyte_bit_func,
 };
 
 /*
@@ -135,37 +196,121 @@ static const struct i2c_algorithm i2c_sibyte_algo = {
 static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
 {
 	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr;
+	int err;
 
-	/* Register new adapter to i2c module... */
-	i2c_adap->algo = &i2c_sibyte_algo;
+	adap->status = 0;
+	init_waitqueue_head(&adap->wait);
+	spin_lock_init(&adap->lock);
+
+	csr = ioremap(adap->base, R_SMB_PEC + SMB_REGISTER_SPACING);
+	if (!csr) {
+		err = -ENOMEM;
+		goto out;
+	}
+	adap->csr = csr;
 
 	/* Set the requested frequency. */
-	csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
-	csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
+	__raw_writeq(speed, csr + R_SMB_FREQ);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	err = request_irq(adap->irq, i2c_sibyte_interrupt, IRQF_SHARED,
+			  adap->name, i2c_adap);
+	if (err < 0)
+		goto out_unmap;
+
+	/* Enable finish and error interrupts. */
+	__raw_writeq(M_SMB_FINISH_INTR | M_SMB_ERR_INTR, csr + R_SMB_CONTROL);
+
+	/* Register new adapter to i2c module... */
+	err = i2c_add_numbered_adapter(i2c_adap);
+	if (err < 0)
+		goto out_unirq;
+
+	return 0;
 
-	return i2c_add_numbered_adapter(i2c_adap);
+out_unirq:
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	free_irq(adap->irq, i2c_adap);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+out_unmap:
+	iounmap(csr);
+out:
+	return err;
 }
 
+static void i2c_sibyte_remove_bus(struct i2c_adapter *i2c_adap)
+{
+	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+
+	i2c_del_adapter(i2c_adap);
+
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
 
-static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
-	{ NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
-	{ NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
+	free_irq(adap->irq, i2c_adap);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+
+	iounmap(csr);
+}
+
+static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
+#ifdef CONFIG_SIBYTE_SB1250
+	{
+		.name	= "sb1250-smbus-0",
+		.base	= A_SMB_0,
+		.irq	= K_INT_SMB_0,
+	},
+	{
+		.name	= "sb1250-smbus-1",
+		.base	= A_SMB_1,
+		.irq	= K_INT_SMB_1,
+	}
+#else
+	{
+		.name	= "bcm1480-smbus-0",
+		.base	= A_SMB_0,
+		.irq	= K_BCM1480_INT_SMB_0,
+	},
+	{
+		.name	= "bcm1480-smbus-1",
+		.base	= A_SMB_1,
+		.irq	= K_BCM1480_INT_SMB_1,
+	}
+#endif
 };
 
-static struct i2c_adapter sibyte_board_adapter[2] = {
+static struct i2c_adapter i2c_sibyte_board_adapter[2] = {
 	{
 		.owner		= THIS_MODULE,
 		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
-		.algo		= NULL,
-		.algo_data	= &sibyte_board_data[0],
+		.algo		= &i2c_sibyte_algo,
+		.algo_data	= &i2c_sibyte_board_data[0],
 		.nr		= 0,
 		.name		= "SiByte SMBus 0",
 	},
 	{
 		.owner		= THIS_MODULE,
 		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
-		.algo		= NULL,
-		.algo_data	= &sibyte_board_data[1],
+		.algo		= &i2c_sibyte_algo,
+		.algo_data	= &i2c_sibyte_board_data[1],
 		.nr		= 1,
 		.name		= "SiByte SMBus 1",
 	},
@@ -173,21 +318,32 @@ static struct i2c_adapter sibyte_board_adapter[2] = {
 
 static int __init i2c_sibyte_init(void)
 {
+	int err;
+
 	pr_info("i2c-sibyte: i2c SMBus adapter module for SiByte board\n");
-	if (i2c_sibyte_add_bus(&sibyte_board_adapter[0], K_SMB_FREQ_100KHZ) < 0)
-		return -ENODEV;
-	if (i2c_sibyte_add_bus(&sibyte_board_adapter[1],
-			       K_SMB_FREQ_400KHZ) < 0) {
-		i2c_del_adapter(&sibyte_board_adapter[0]);
-		return -ENODEV;
-	}
+
+	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[0],
+				 K_SMB_FREQ_100KHZ);
+	if (err < 0)
+		goto out;
+
+	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[1],
+				 K_SMB_FREQ_400KHZ);
+	if (err < 0)
+		goto out_remove;
+
 	return 0;
+
+out_remove:
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
+out:
+	return err;
 }
 
 static void __exit i2c_sibyte_exit(void)
 {
-	i2c_del_adapter(&sibyte_board_adapter[0]);
-	i2c_del_adapter(&sibyte_board_adapter[1]);
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[1]);
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
 }
 
 module_init(i2c_sibyte_init);
-- 
1.7.3.4

[PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

From: Maciej W. Rozycki <macro@linux-mips.org>

This is a rewrite of large parts of the driver mainly so that it uses
SMBus interrupts to offload the CPU from busy-waiting on status inputs.
As a part of the overhaul of the init and exit calls, all accesses to the
hardware got converted to use accessory functions via an ioremap() cookie.

[mattst88] Added BCM1480 interrupts and rebased minimally.

Signed-off-by: Matt Turner <mattst88@gmail.com>
Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
This is the second version of this patch that I've sent. This version
fixes the problem with the ENXIO return.

 drivers/i2c/busses/i2c-sibyte.c |  296 +++++++++++++++++++++++++++++---------
 1 files changed, 226 insertions(+), 70 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sibyte.c b/drivers/i2c/busses/i2c-sibyte.c
index 0fe505d..d2f1cf1 100644
--- a/drivers/i2c/busses/i2c-sibyte.c
+++ b/drivers/i2c/busses/i2c-sibyte.c
@@ -2,6 +2,7 @@
  * Copyright (C) 2004 Steven J. Hill
  * Copyright (C) 2001,2002,2003 Broadcom Corporation
  * Copyright (C) 1995-2000 Simon G. Vogl
+ * Copyright (C) 2008 Maciej W. Rozycki
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -18,104 +19,164 @@
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
  */
 
+#include <linux/errno.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/param.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/wait.h>
 #include <linux/io.h>
+#include <asm/sibyte/sb1250_int.h>
 #include <asm/sibyte/sb1250_regs.h>
 #include <asm/sibyte/sb1250_smbus.h>
+#include <asm/sibyte/bcm1480_int.h>
 
 
 struct i2c_algo_sibyte_data {
-	void *data;		/* private data */
-	int   bus;		/* which bus */
-	void *reg_base;		/* CSR base */
+	wait_queue_head_t	wait;		/* IRQ queue */
+	void __iomem		*csr;		/* mapped CSR handle */
+	phys_t			base;		/* physical CSR base */
+	char			*name;		/* IRQ handler name */
+	spinlock_t		lock;		/* atomiser */
+	int			irq;		/* IRQ line */
+	int			status;		/* IRQ status */
 };
 
-/* ----- global defines ----------------------------------------------- */
-#define SMB_CSR(a,r) ((long)(a->reg_base + r))
 
+static irqreturn_t i2c_sibyte_interrupt(int irq, void *dev_id)
+{
+	struct i2c_adapter *i2c_adap = dev_id;
+	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+	u8 status;
+
+	/*
+	 * Ugh, no way to detect the finish interrupt,
+	 * but if busy it is obviously not one.
+	 */
+	status = __raw_readq(csr + R_SMB_STATUS);
+	if ((status & (M_SMB_ERROR | M_SMB_BUSY)) == M_SMB_BUSY)
+		return IRQ_NONE;
+
+	/*
+	 * Clear the error interrupt (write 1 to clear);
+	 * the finish interrupt was cleared by the read above.
+	 */
+	__raw_writeq(status, csr + R_SMB_STATUS);
+
+	/* Post the status. */
+	spin_lock(&adap->lock);
+	adap->status = status & (M_SMB_ERROR_TYPE | M_SMB_ERROR | M_SMB_BUSY);
+	wake_up(&adap->wait);
+	spin_unlock(&adap->lock);
+
+	return IRQ_HANDLED;
+}
 
-static int smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
-		      unsigned short flags, char read_write,
-		      u8 command, int size, union i2c_smbus_data * data)
+static s32 i2c_sibyte_smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
+				 unsigned short cflags,
+				 char read_write, u8 command, int size,
+				 union i2c_smbus_data *data)
 {
 	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+	unsigned long flags;
 	int data_bytes = 0;
 	int error;
 
-	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
-		;
+	spin_lock_irqsave(&adap->lock, flags);
+
+	if (adap->status < 0) {
+		error = -EIO;
+		goto out_unlock;
+	}
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		csr_out32((V_SMB_ADDR(addr) |
-			   (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
-			   V_SMB_TT_QUICKCMD), SMB_CSR(adap, R_SMB_START));
+		__raw_writeq(V_SMB_ADDR(addr) |
+			     (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
+			     V_SMB_TT_QUICKCMD,
+			     csr + R_SMB_START);
 		break;
 	case I2C_SMBUS_BYTE:
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 1;
 		} else {
-			csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
+		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 1;
 		} else {
-			csr_out32(V_SMB_LB(data->byte),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_LB(data->byte), csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	case I2C_SMBUS_WORD_DATA:
-		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
+		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 2;
 		} else {
-			csr_out32(V_SMB_LB(data->word & 0xff),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32(V_SMB_MB(data->word >> 8),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_LB(data->word & 0xff),
+				     csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_MB(data->word >> 8),
+				     csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	default:
-		return -EOPNOTSUPP;
+		error = -EOPNOTSUPP;
+		goto out_unlock;
 	}
+	mmiowb();
+	__raw_readq(csr + R_SMB_START);
+	adap->status = -1;
+
+	spin_unlock_irqrestore(&adap->lock, flags);
 
-	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
-		;
+	wait_event_timeout(adap->wait, (adap->status >= 0), HZ);
 
-	error = csr_in32(SMB_CSR(adap, R_SMB_STATUS));
-	if (error & M_SMB_ERROR) {
-		/* Clear error bit by writing a 1 */
-		csr_out32(M_SMB_ERROR, SMB_CSR(adap, R_SMB_STATUS));
-		return (error & M_SMB_ERROR_TYPE) ? -EIO : -ENXIO;
+	spin_lock_irqsave(&adap->lock, flags);
+
+	if (adap->status > 0 && ((adap->status & (M_SMB_ERROR | M_SMB_ERROR_TYPE)) == M_SMB_ERROR)) {
+		error = -ENXIO;
+		goto out_unlock;
+	}
+	if (adap->status < 0 || (adap->status & (M_SMB_ERROR | M_SMB_BUSY))) {
+		error = -EIO;
+		goto out_unlock;
 	}
 
 	if (data_bytes == 1)
-		data->byte = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xff;
+		data->byte = __raw_readq(csr + R_SMB_DATA) & 0xff;
 	if (data_bytes == 2)
-		data->word = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xffff;
+		data->word = __raw_readq(csr + R_SMB_DATA) & 0xffff;
 
-	return 0;
+	error = 0;
+
+out_unlock:
+	spin_unlock_irqrestore(&adap->lock, flags);
+
+	return error;
 }
 
-static u32 bit_func(struct i2c_adapter *adap)
+static u32 i2c_sibyte_bit_func(struct i2c_adapter *adap)
 {
 	return (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA);
@@ -125,8 +186,8 @@ static u32 bit_func(struct i2c_adapter *adap)
 /* -----exported algorithm data: -------------------------------------	*/
 
 static const struct i2c_algorithm i2c_sibyte_algo = {
-	.smbus_xfer	= smbus_xfer,
-	.functionality	= bit_func,
+	.smbus_xfer	= i2c_sibyte_smbus_xfer,
+	.functionality	= i2c_sibyte_bit_func,
 };
 
 /*
@@ -135,37 +196,121 @@ static const struct i2c_algorithm i2c_sibyte_algo = {
 static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
 {
 	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr;
+	int err;
 
-	/* Register new adapter to i2c module... */
-	i2c_adap->algo = &i2c_sibyte_algo;
+	adap->status = 0;
+	init_waitqueue_head(&adap->wait);
+	spin_lock_init(&adap->lock);
+
+	csr = ioremap(adap->base, R_SMB_PEC + SMB_REGISTER_SPACING);
+	if (!csr) {
+		err = -ENOMEM;
+		goto out;
+	}
+	adap->csr = csr;
 
 	/* Set the requested frequency. */
-	csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
-	csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
+	__raw_writeq(speed, csr + R_SMB_FREQ);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	err = request_irq(adap->irq, i2c_sibyte_interrupt, IRQF_SHARED,
+			  adap->name, i2c_adap);
+	if (err < 0)
+		goto out_unmap;
+
+	/* Enable finish and error interrupts. */
+	__raw_writeq(M_SMB_FINISH_INTR | M_SMB_ERR_INTR, csr + R_SMB_CONTROL);
+
+	/* Register new adapter to i2c module... */
+	err = i2c_add_numbered_adapter(i2c_adap);
+	if (err < 0)
+		goto out_unirq;
+
+	return 0;
 
-	return i2c_add_numbered_adapter(i2c_adap);
+out_unirq:
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	free_irq(adap->irq, i2c_adap);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+out_unmap:
+	iounmap(csr);
+out:
+	return err;
 }
 
+static void i2c_sibyte_remove_bus(struct i2c_adapter *i2c_adap)
+{
+	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+
+	i2c_del_adapter(i2c_adap);
+
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
 
-static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
-	{ NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
-	{ NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
+	free_irq(adap->irq, i2c_adap);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+
+	iounmap(csr);
+}
+
+static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
+#ifdef CONFIG_SIBYTE_SB1250
+	{
+		.name	= "sb1250-smbus-0",
+		.base	= A_SMB_0,
+		.irq	= K_INT_SMB_0,
+	},
+	{
+		.name	= "sb1250-smbus-1",
+		.base	= A_SMB_1,
+		.irq	= K_INT_SMB_1,
+	}
+#else
+	{
+		.name	= "bcm1480-smbus-0",
+		.base	= A_SMB_0,
+		.irq	= K_BCM1480_INT_SMB_0,
+	},
+	{
+		.name	= "bcm1480-smbus-1",
+		.base	= A_SMB_1,
+		.irq	= K_BCM1480_INT_SMB_1,
+	}
+#endif
 };
 
-static struct i2c_adapter sibyte_board_adapter[2] = {
+static struct i2c_adapter i2c_sibyte_board_adapter[2] = {
 	{
 		.owner		= THIS_MODULE,
 		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
-		.algo		= NULL,
-		.algo_data	= &sibyte_board_data[0],
+		.algo		= &i2c_sibyte_algo,
+		.algo_data	= &i2c_sibyte_board_data[0],
 		.nr		= 0,
 		.name		= "SiByte SMBus 0",
 	},
 	{
 		.owner		= THIS_MODULE,
 		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
-		.algo		= NULL,
-		.algo_data	= &sibyte_board_data[1],
+		.algo		= &i2c_sibyte_algo,
+		.algo_data	= &i2c_sibyte_board_data[1],
 		.nr		= 1,
 		.name		= "SiByte SMBus 1",
 	},
@@ -173,21 +318,32 @@ static struct i2c_adapter sibyte_board_adapter[2] = {
 
 static int __init i2c_sibyte_init(void)
 {
+	int err;
+
 	pr_info("i2c-sibyte: i2c SMBus adapter module for SiByte board\n");
-	if (i2c_sibyte_add_bus(&sibyte_board_adapter[0], K_SMB_FREQ_100KHZ) < 0)
-		return -ENODEV;
-	if (i2c_sibyte_add_bus(&sibyte_board_adapter[1],
-			       K_SMB_FREQ_400KHZ) < 0) {
-		i2c_del_adapter(&sibyte_board_adapter[0]);
-		return -ENODEV;
-	}
+
+	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[0],
+				 K_SMB_FREQ_100KHZ);
+	if (err < 0)
+		goto out;
+
+	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[1],
+				 K_SMB_FREQ_400KHZ);
+	if (err < 0)
+		goto out_remove;
+
 	return 0;
+
+out_remove:
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
+out:
+	return err;
 }
 
 static void __exit i2c_sibyte_exit(void)
 {
-	i2c_del_adapter(&sibyte_board_adapter[0]);
-	i2c_del_adapter(&sibyte_board_adapter[1]);
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[1]);
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
 }
 
 module_init(i2c_sibyte_init);
-- 
1.7.3.4

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Thu, 2011-08-18 at 19:43 -0400, Matt Turner wrote:
> From: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.
> 
> [mattst88] Added BCM1480 interrupts and rebased minimally.
> 
> Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>

Patch works fine on our target, and shows significant speed improvements
for i2c accesses.

Linux version 3.0.3-428-g17c1f3f (groeck@rbos-pc-13) (gcc version 4.4.1
(Debian 4.4.1-1) ) #2 SMP Mon Aug 22 12:56:41 PDT 2011
bootconsole [early0] enabled
CPU revision is: 01041100 (SiByte SB1A)
FPU revision is: 000f0103
Checking for the multiply/shift bug... no.
Checking for the daddiu bug... no.
Broadcom SiByte BCM1480 B1 (pass2) @ 900 MHz (SB-1A rev 0)

Tested-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>

Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Thu, 2011-08-18 at 19:43 -0400, Matt Turner wrote:
> From: Maciej W. Rozycki <macro@linux-mips.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.
> 
> [mattst88] Added BCM1480 interrupts and rebased minimally.
> 
> Signed-off-by: Matt Turner <mattst88@gmail.com>
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>

Patch works fine on our target, and shows significant speed improvements
for i2c accesses.

Linux version 3.0.3-428-g17c1f3f (groeck@rbos-pc-13) (gcc version 4.4.1
(Debian 4.4.1-1) ) #2 SMP Mon Aug 22 12:56:41 PDT 2011
bootconsole [early0] enabled
CPU revision is: 01041100 (SiByte SB1A)
FPU revision is: 000f0103
Checking for the multiply/shift bug... no.
Checking for the daddiu bug... no.
Broadcom SiByte BCM1480 B1 (pass2) @ 900 MHz (SB-1A rev 0)

Tested-by: Guenter Roeck <guenter.roeck@ericsson.com>

Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Thu, Aug 18, 2011 at 7:43 PM, Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>

Jean,
Do you want to take this patch, or should Ralf through his tree?

Thanks,
Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Thu, Aug 18, 2011 at 7:43 PM, Matt Turner <mattst88@gmail.com> wrote:
> Signed-off-by: Matt Turner <mattst88@gmail.com>
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>

Jean,
Do you want to take this patch, or should Ralf through his tree?

Thanks,
Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Wed, 24 Aug 2011 11:36:48 -0400, Matt Turner wrote:
> On Thu, Aug 18, 2011 at 7:43 PM, Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> 
> Do you want to take this patch, or should Ralf through his tree?

My tree is fine, unless Ralf has a specific reason to pick it. Just
give me a couple hours to review the patch.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Wed, 24 Aug 2011 11:36:48 -0400, Matt Turner wrote:
> On Thu, Aug 18, 2011 at 7:43 PM, Matt Turner <mattst88@gmail.com> wrote:
> > Signed-off-by: Matt Turner <mattst88@gmail.com>
> > Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
> 
> Do you want to take this patch, or should Ralf through his tree?

My tree is fine, unless Ralf has a specific reason to pick it. Just
give me a couple hours to review the patch.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Fri, 2 Sep 2011, Jean Delvare wrote:

> > > Signed-off-by: Matt Turner <mattst88@gmail.com>
> > > Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
> > 
> > Do you want to take this patch, or should Ralf through his tree?
> 
> My tree is fine, unless Ralf has a specific reason to pick it. Just
> give me a couple hours to review the patch.

 As a side note I do really need to get back to this series and see if 
there's anything else that needs reviving.  Hopefully within the next 
couple of weeks.

 Thanks, Matt, for taking care of this patch.  I reckon I've got some 
other old SWARM stuff that needs digging up too.

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

Hi Matt,

On Thu, 18 Aug 2011 19:43:11 -0400, Matt Turner wrote:
> From: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.

This could have been split into incremental patches, to make review
easier.

> [mattst88] Added BCM1480 interrupts and rebased minimally.

Ditto.

checkpatch complains about this, please fix:

WARNING: line over 80 characters
#257: FILE: drivers/i2c/busses/i2c-sibyte.c:157:
+	if (adap->status > 0 && ((adap->status & (M_SMB_ERROR | M_SMB_ERROR_TYPE)) == M_SMB_ERROR)) {

Very nice patch overall, I only have minor comments, see below inline.

> 
> Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> ---
> This is the second version of this patch that I've sent. This version
> fixes the problem with the ENXIO return.
> 
>  drivers/i2c/busses/i2c-sibyte.c |  296 +++++++++++++++++++++++++++++---------
>  1 files changed, 226 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sibyte.c b/drivers/i2c/busses/i2c-sibyte.c
> index 0fe505d..d2f1cf1 100644
> --- a/drivers/i2c/busses/i2c-sibyte.c
> +++ b/drivers/i2c/busses/i2c-sibyte.c
> @@ -2,6 +2,7 @@
>   * Copyright (C) 2004 Steven J. Hill
>   * Copyright (C) 2001,2002,2003 Broadcom Corporation
>   * Copyright (C) 1995-2000 Simon G. Vogl
> + * Copyright (C) 2008 Maciej W. Rozycki

Wow, looks like this patch has been sleeping for quite some time...

>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -18,104 +19,164 @@
>   * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
>   */
>  
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
> +#include <linux/param.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
>  #include <linux/io.h>
> +#include <asm/sibyte/sb1250_int.h>
>  #include <asm/sibyte/sb1250_regs.h>
>  #include <asm/sibyte/sb1250_smbus.h>
> +#include <asm/sibyte/bcm1480_int.h>
>  
>  
>  struct i2c_algo_sibyte_data {
> -	void *data;		/* private data */
> -	int   bus;		/* which bus */
> -	void *reg_base;		/* CSR base */
> +	wait_queue_head_t	wait;		/* IRQ queue */
> +	void __iomem		*csr;		/* mapped CSR handle */
> +	phys_t			base;		/* physical CSR base */
> +	char			*name;		/* IRQ handler name */

Should be a const pointer. Also, if the name is only for the IRQ, then
irq_name would be a better name.

> +	spinlock_t		lock;		/* atomiser */

A more useful comment would explain what exactly is being protected by
the lock.

> +	int			irq;		/* IRQ line */
> +	int			status;		/* IRQ status */

You could document than -1 means error.

>  };
>  
> -/* ----- global defines ----------------------------------------------- */
> -#define SMB_CSR(a,r) ((long)(a->reg_base + r))
>  
> +static irqreturn_t i2c_sibyte_interrupt(int irq, void *dev_id)
> +{
> +	struct i2c_adapter *i2c_adap = dev_id;
> +	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +	void __iomem *csr = adap->csr;
> +	u8 status;
> +
> +	/*
> +	 * Ugh, no way to detect the finish interrupt,
> +	 * but if busy it is obviously not one.
> +	 */
> +	status = __raw_readq(csr + R_SMB_STATUS);
> +	if ((status & (M_SMB_ERROR | M_SMB_BUSY)) == M_SMB_BUSY)
> +		return IRQ_NONE;
> +
> +	/*
> +	 * Clear the error interrupt (write 1 to clear);
> +	 * the finish interrupt was cleared by the read above.
> +	 */
> +	__raw_writeq(status, csr + R_SMB_STATUS);
> +
> +	/* Post the status. */
> +	spin_lock(&adap->lock);
> +	adap->status = status & (M_SMB_ERROR_TYPE | M_SMB_ERROR | M_SMB_BUSY);
> +	wake_up(&adap->wait);
> +	spin_unlock(&adap->lock);
> +
> +	return IRQ_HANDLED;
> +}
>  
> -static int smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> -		      unsigned short flags, char read_write,
> -		      u8 command, int size, union i2c_smbus_data * data)
> +static s32 i2c_sibyte_smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> +				 unsigned short cflags,
> +				 char read_write, u8 command, int size,
> +				 union i2c_smbus_data *data)
>  {
>  	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +	void __iomem *csr = adap->csr;
> +	unsigned long flags;
>  	int data_bytes = 0;
>  	int error;
>  
> -	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -		;
> +	spin_lock_irqsave(&adap->lock, flags);
> +
> +	if (adap->status < 0) {
> +		error = -EIO;
> +		goto out_unlock;
> +	}

Well, this can only happen if the previous transaction ended up with a
failure, right? This means that a single error will break the SMBus
forever. Is there no way to reset the controller to a sane state if
this happens?

>  
>  	switch (size) {
>  	case I2C_SMBUS_QUICK:
> -		csr_out32((V_SMB_ADDR(addr) |
> -			   (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> -			   V_SMB_TT_QUICKCMD), SMB_CSR(adap, R_SMB_START));
> +		__raw_writeq(V_SMB_ADDR(addr) |
> +			     (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> +			     V_SMB_TT_QUICKCMD,
> +			     csr + R_SMB_START);
>  		break;
>  	case I2C_SMBUS_BYTE:
>  		if (read_write == I2C_SMBUS_READ) {
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE,
> +				     csr + R_SMB_START);
>  			data_bytes = 1;
>  		} else {
> -			csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE,
> +				     csr + R_SMB_START);
>  		}
>  		break;
>  	case I2C_SMBUS_BYTE_DATA:
> -		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>  		if (read_write == I2C_SMBUS_READ) {
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE,
> +				     csr + R_SMB_START);
>  			data_bytes = 1;
>  		} else {
> -			csr_out32(V_SMB_LB(data->byte),
> -				  SMB_CSR(adap, R_SMB_DATA));
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_LB(data->byte), csr + R_SMB_DATA);
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +				     csr + R_SMB_START);
>  		}
>  		break;
>  	case I2C_SMBUS_WORD_DATA:
> -		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>  		if (read_write == I2C_SMBUS_READ) {
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE,
> +				     csr + R_SMB_START);
>  			data_bytes = 2;
>  		} else {
> -			csr_out32(V_SMB_LB(data->word & 0xff),
> -				  SMB_CSR(adap, R_SMB_DATA));
> -			csr_out32(V_SMB_MB(data->word >> 8),
> -				  SMB_CSR(adap, R_SMB_DATA));
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_LB(data->word & 0xff),
> +				     csr + R_SMB_DATA);
> +			__raw_writeq(V_SMB_MB(data->word >> 8),
> +				     csr + R_SMB_DATA);
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +				     csr + R_SMB_START);
>  		}
>  		break;
>  	default:
> -		return -EOPNOTSUPP;
> +		error = -EOPNOTSUPP;
> +		goto out_unlock;
>  	}
> +	mmiowb();
> +	__raw_readq(csr + R_SMB_START);
> +	adap->status = -1;
> +
> +	spin_unlock_irqrestore(&adap->lock, flags);
>  
> -	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -		;
> +	wait_event_timeout(adap->wait, (adap->status >= 0), HZ);

1 second is a rather long timeout. The driver only supports small
transactions, so even if a slave slows down the clock, I can hardly
imagine a transaction lasting more that, say, 10 ms. So I think you can
safely lower the timeout to HZ / 5 or even HZ / 10.

Also, shouldn't you check the return value? This would let you return
the right error code (-ETIMEDOUT according to
Documentation/i2c/fault-codes) and you would no longer have to check
for adap->status sign in the rest of the function.

>  
> -	error = csr_in32(SMB_CSR(adap, R_SMB_STATUS));
> -	if (error & M_SMB_ERROR) {
> -		/* Clear error bit by writing a 1 */
> -		csr_out32(M_SMB_ERROR, SMB_CSR(adap, R_SMB_STATUS));
> -		return (error & M_SMB_ERROR_TYPE) ? -EIO : -ENXIO;
> +	spin_lock_irqsave(&adap->lock, flags);
> +
> +	if (adap->status > 0 && ((adap->status & (M_SMB_ERROR | M_SMB_ERROR_TYPE)) == M_SMB_ERROR)) {
> +		error = -ENXIO;
> +		goto out_unlock;
> +	}
> +	if (adap->status < 0 || (adap->status & (M_SMB_ERROR | M_SMB_BUSY))) {
> +		error = -EIO;
> +		goto out_unlock;
>  	}
>  
>  	if (data_bytes == 1)
> -		data->byte = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xff;
> +		data->byte = __raw_readq(csr + R_SMB_DATA) & 0xff;
>  	if (data_bytes == 2)
> -		data->word = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xffff;
> +		data->word = __raw_readq(csr + R_SMB_DATA) & 0xffff;
>  
> -	return 0;
> +	error = 0;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&adap->lock, flags);
> +
> +	return error;
>  }
>  
> -static u32 bit_func(struct i2c_adapter *adap)
> +static u32 i2c_sibyte_bit_func(struct i2c_adapter *adap)

If you're renaming this then please drop the "bit" part in it, it's
most likely coming from a copy-and-paste from i2c-algo-bit and has no
meaning in the sibyte driver.

>  {
>  	return (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>  		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA);
> @@ -125,8 +186,8 @@ static u32 bit_func(struct i2c_adapter *adap)
>  /* -----exported algorithm data: -------------------------------------	*/
>  
>  static const struct i2c_algorithm i2c_sibyte_algo = {
> -	.smbus_xfer	= smbus_xfer,
> -	.functionality	= bit_func,
> +	.smbus_xfer	= i2c_sibyte_smbus_xfer,
> +	.functionality	= i2c_sibyte_bit_func,
>  };
>  
>  /*
> @@ -135,37 +196,121 @@ static const struct i2c_algorithm i2c_sibyte_algo = {
>  static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
>  {
>  	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +	void __iomem *csr;
> +	int err;
>  
> -	/* Register new adapter to i2c module... */
> -	i2c_adap->algo = &i2c_sibyte_algo;
> +	adap->status = 0;
> +	init_waitqueue_head(&adap->wait);
> +	spin_lock_init(&adap->lock);
> +
> +	csr = ioremap(adap->base, R_SMB_PEC + SMB_REGISTER_SPACING);
> +	if (!csr) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +	adap->csr = csr;
>  
>  	/* Set the requested frequency. */
> -	csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
> -	csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
> +	__raw_writeq(speed, csr + R_SMB_FREQ);
> +
> +	/* Clear any pending error interrupt. */
> +	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +	/* Disable interrupts. */
> +	__raw_writeq(0, csr + R_SMB_CONTROL);
> +	mmiowb();
> +	__raw_readq(csr + R_SMB_CONTROL);

Shouldn't it be the other way around, disable interrupts first and then
clear any pending one? Looks racy otherwise, but maybe it makes no
difference in practice.

> +
> +	err = request_irq(adap->irq, i2c_sibyte_interrupt, IRQF_SHARED,
> +			  adap->name, i2c_adap);
> +	if (err < 0)
> +		goto out_unmap;
> +
> +	/* Enable finish and error interrupts. */
> +	__raw_writeq(M_SMB_FINISH_INTR | M_SMB_ERR_INTR, csr + R_SMB_CONTROL);
> +
> +	/* Register new adapter to i2c module... */
> +	err = i2c_add_numbered_adapter(i2c_adap);
> +	if (err < 0)
> +		goto out_unirq;
> +
> +	return 0;
>  
> -	return i2c_add_numbered_adapter(i2c_adap);
> +out_unirq:
> +	/* Disable interrupts. */
> +	__raw_writeq(0, csr + R_SMB_CONTROL);
> +	mmiowb();
> +	__raw_readq(csr + R_SMB_CONTROL);
> +
> +	free_irq(adap->irq, i2c_adap);
> +
> +	/* Clear any pending error interrupt. */
> +	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);

You may consider moving this block to a separate function, as it is
duplicated in the i2c_sibyte_remove_bus() function below.

> +out_unmap:
> +	iounmap(csr);
> +out:
> +	return err;
>  }
>  
> +static void i2c_sibyte_remove_bus(struct i2c_adapter *i2c_adap)
> +{
> +	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +	void __iomem *csr = adap->csr;
> +
> +	i2c_del_adapter(i2c_adap);
> +
> +	/* Disable interrupts. */
> +	__raw_writeq(0, csr + R_SMB_CONTROL);
> +	mmiowb();
> +	__raw_readq(csr + R_SMB_CONTROL);
>  
> -static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
> -	{ NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
> -	{ NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
> +	free_irq(adap->irq, i2c_adap);
> +
> +	/* Clear any pending error interrupt. */
> +	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +
> +	iounmap(csr);
> +}
> +
> +static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
> +#ifdef CONFIG_SIBYTE_SB1250
> +	{
> +		.name	= "sb1250-smbus-0",
> +		.base	= A_SMB_0,
> +		.irq	= K_INT_SMB_0,
> +	},
> +	{
> +		.name	= "sb1250-smbus-1",
> +		.base	= A_SMB_1,
> +		.irq	= K_INT_SMB_1,
> +	}
> +#else
> +	{
> +		.name	= "bcm1480-smbus-0",
> +		.base	= A_SMB_0,
> +		.irq	= K_BCM1480_INT_SMB_0,
> +	},
> +	{
> +		.name	= "bcm1480-smbus-1",
> +		.base	= A_SMB_1,
> +		.irq	= K_BCM1480_INT_SMB_1,
> +	}
> +#endif
>  };
>  
> -static struct i2c_adapter sibyte_board_adapter[2] = {
> +static struct i2c_adapter i2c_sibyte_board_adapter[2] = {
>  	{
>  		.owner		= THIS_MODULE,
>  		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -		.algo		= NULL,
> -		.algo_data	= &sibyte_board_data[0],
> +		.algo		= &i2c_sibyte_algo,
> +		.algo_data	= &i2c_sibyte_board_data[0],
>  		.nr		= 0,
>  		.name		= "SiByte SMBus 0",
>  	},
>  	{
>  		.owner		= THIS_MODULE,
>  		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -		.algo		= NULL,
> -		.algo_data	= &sibyte_board_data[1],
> +		.algo		= &i2c_sibyte_algo,
> +		.algo_data	= &i2c_sibyte_board_data[1],
>  		.nr		= 1,
>  		.name		= "SiByte SMBus 1",
>  	},
> @@ -173,21 +318,32 @@ static struct i2c_adapter sibyte_board_adapter[2] = {
>  
>  static int __init i2c_sibyte_init(void)
>  {
> +	int err;
> +
>  	pr_info("i2c-sibyte: i2c SMBus adapter module for SiByte board\n");
> -	if (i2c_sibyte_add_bus(&sibyte_board_adapter[0], K_SMB_FREQ_100KHZ) < 0)
> -		return -ENODEV;
> -	if (i2c_sibyte_add_bus(&sibyte_board_adapter[1],
> -			       K_SMB_FREQ_400KHZ) < 0) {
> -		i2c_del_adapter(&sibyte_board_adapter[0]);
> -		return -ENODEV;
> -	}
> +
> +	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[0],
> +				 K_SMB_FREQ_100KHZ);
> +	if (err < 0)
> +		goto out;
> +
> +	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[1],
> +				 K_SMB_FREQ_400KHZ);
> +	if (err < 0)
> +		goto out_remove;
> +
>  	return 0;
> +
> +out_remove:
> +	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
> +out:
> +	return err;
>  }
>  
>  static void __exit i2c_sibyte_exit(void)
>  {
> -	i2c_del_adapter(&sibyte_board_adapter[0]);
> -	i2c_del_adapter(&sibyte_board_adapter[1]);
> +	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[1]);
> +	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
>  }
>  
>  module_init(i2c_sibyte_init);

Please address my concerns where you agree and send an updated patch.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

Hi Matt,

On Thu, 18 Aug 2011 19:43:11 -0400, Matt Turner wrote:
> From: Maciej W. Rozycki <macro@linux-mips.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.

This could have been split into incremental patches, to make review
easier.

> [mattst88] Added BCM1480 interrupts and rebased minimally.

Ditto.

checkpatch complains about this, please fix:

WARNING: line over 80 characters
#257: FILE: drivers/i2c/busses/i2c-sibyte.c:157:
+	if (adap->status > 0 && ((adap->status & (M_SMB_ERROR | M_SMB_ERROR_TYPE)) == M_SMB_ERROR)) {

Very nice patch overall, I only have minor comments, see below inline.

> 
> Signed-off-by: Matt Turner <mattst88@gmail.com>
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
> ---
> This is the second version of this patch that I've sent. This version
> fixes the problem with the ENXIO return.
> 
>  drivers/i2c/busses/i2c-sibyte.c |  296 +++++++++++++++++++++++++++++---------
>  1 files changed, 226 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sibyte.c b/drivers/i2c/busses/i2c-sibyte.c
> index 0fe505d..d2f1cf1 100644
> --- a/drivers/i2c/busses/i2c-sibyte.c
> +++ b/drivers/i2c/busses/i2c-sibyte.c
> @@ -2,6 +2,7 @@
>   * Copyright (C) 2004 Steven J. Hill
>   * Copyright (C) 2001,2002,2003 Broadcom Corporation
>   * Copyright (C) 1995-2000 Simon G. Vogl
> + * Copyright (C) 2008 Maciej W. Rozycki

Wow, looks like this patch has been sleeping for quite some time...

>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -18,104 +19,164 @@
>   * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
>   */
>  
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
> +#include <linux/param.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
>  #include <linux/io.h>
> +#include <asm/sibyte/sb1250_int.h>
>  #include <asm/sibyte/sb1250_regs.h>
>  #include <asm/sibyte/sb1250_smbus.h>
> +#include <asm/sibyte/bcm1480_int.h>
>  
>  
>  struct i2c_algo_sibyte_data {
> -	void *data;		/* private data */
> -	int   bus;		/* which bus */
> -	void *reg_base;		/* CSR base */
> +	wait_queue_head_t	wait;		/* IRQ queue */
> +	void __iomem		*csr;		/* mapped CSR handle */
> +	phys_t			base;		/* physical CSR base */
> +	char			*name;		/* IRQ handler name */

Should be a const pointer. Also, if the name is only for the IRQ, then
irq_name would be a better name.

> +	spinlock_t		lock;		/* atomiser */

A more useful comment would explain what exactly is being protected by
the lock.

> +	int			irq;		/* IRQ line */
> +	int			status;		/* IRQ status */

You could document than -1 means error.

>  };
>  
> -/* ----- global defines ----------------------------------------------- */
> -#define SMB_CSR(a,r) ((long)(a->reg_base + r))
>  
> +static irqreturn_t i2c_sibyte_interrupt(int irq, void *dev_id)
> +{
> +	struct i2c_adapter *i2c_adap = dev_id;
> +	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +	void __iomem *csr = adap->csr;
> +	u8 status;
> +
> +	/*
> +	 * Ugh, no way to detect the finish interrupt,
> +	 * but if busy it is obviously not one.
> +	 */
> +	status = __raw_readq(csr + R_SMB_STATUS);
> +	if ((status & (M_SMB_ERROR | M_SMB_BUSY)) == M_SMB_BUSY)
> +		return IRQ_NONE;
> +
> +	/*
> +	 * Clear the error interrupt (write 1 to clear);
> +	 * the finish interrupt was cleared by the read above.
> +	 */
> +	__raw_writeq(status, csr + R_SMB_STATUS);
> +
> +	/* Post the status. */
> +	spin_lock(&adap->lock);
> +	adap->status = status & (M_SMB_ERROR_TYPE | M_SMB_ERROR | M_SMB_BUSY);
> +	wake_up(&adap->wait);
> +	spin_unlock(&adap->lock);
> +
> +	return IRQ_HANDLED;
> +}
>  
> -static int smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> -		      unsigned short flags, char read_write,
> -		      u8 command, int size, union i2c_smbus_data * data)
> +static s32 i2c_sibyte_smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> +				 unsigned short cflags,
> +				 char read_write, u8 command, int size,
> +				 union i2c_smbus_data *data)
>  {
>  	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +	void __iomem *csr = adap->csr;
> +	unsigned long flags;
>  	int data_bytes = 0;
>  	int error;
>  
> -	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -		;
> +	spin_lock_irqsave(&adap->lock, flags);
> +
> +	if (adap->status < 0) {
> +		error = -EIO;
> +		goto out_unlock;
> +	}

Well, this can only happen if the previous transaction ended up with a
failure, right? This means that a single error will break the SMBus
forever. Is there no way to reset the controller to a sane state if
this happens?

>  
>  	switch (size) {
>  	case I2C_SMBUS_QUICK:
> -		csr_out32((V_SMB_ADDR(addr) |
> -			   (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> -			   V_SMB_TT_QUICKCMD), SMB_CSR(adap, R_SMB_START));
> +		__raw_writeq(V_SMB_ADDR(addr) |
> +			     (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> +			     V_SMB_TT_QUICKCMD,
> +			     csr + R_SMB_START);
>  		break;
>  	case I2C_SMBUS_BYTE:
>  		if (read_write == I2C_SMBUS_READ) {
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE,
> +				     csr + R_SMB_START);
>  			data_bytes = 1;
>  		} else {
> -			csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE,
> +				     csr + R_SMB_START);
>  		}
>  		break;
>  	case I2C_SMBUS_BYTE_DATA:
> -		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>  		if (read_write == I2C_SMBUS_READ) {
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE,
> +				     csr + R_SMB_START);
>  			data_bytes = 1;
>  		} else {
> -			csr_out32(V_SMB_LB(data->byte),
> -				  SMB_CSR(adap, R_SMB_DATA));
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_LB(data->byte), csr + R_SMB_DATA);
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +				     csr + R_SMB_START);
>  		}
>  		break;
>  	case I2C_SMBUS_WORD_DATA:
> -		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>  		if (read_write == I2C_SMBUS_READ) {
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE,
> +				     csr + R_SMB_START);
>  			data_bytes = 2;
>  		} else {
> -			csr_out32(V_SMB_LB(data->word & 0xff),
> -				  SMB_CSR(adap, R_SMB_DATA));
> -			csr_out32(V_SMB_MB(data->word >> 8),
> -				  SMB_CSR(adap, R_SMB_DATA));
> -			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -				  SMB_CSR(adap, R_SMB_START));
> +			__raw_writeq(V_SMB_LB(data->word & 0xff),
> +				     csr + R_SMB_DATA);
> +			__raw_writeq(V_SMB_MB(data->word >> 8),
> +				     csr + R_SMB_DATA);
> +			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +				     csr + R_SMB_START);
>  		}
>  		break;
>  	default:
> -		return -EOPNOTSUPP;
> +		error = -EOPNOTSUPP;
> +		goto out_unlock;
>  	}
> +	mmiowb();
> +	__raw_readq(csr + R_SMB_START);
> +	adap->status = -1;
> +
> +	spin_unlock_irqrestore(&adap->lock, flags);
>  
> -	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -		;
> +	wait_event_timeout(adap->wait, (adap->status >= 0), HZ);

1 second is a rather long timeout. The driver only supports small
transactions, so even if a slave slows down the clock, I can hardly
imagine a transaction lasting more that, say, 10 ms. So I think you can
safely lower the timeout to HZ / 5 or even HZ / 10.

Also, shouldn't you check the return value? This would let you return
the right error code (-ETIMEDOUT according to
Documentation/i2c/fault-codes) and you would no longer have to check
for adap->status sign in the rest of the function.

>  
> -	error = csr_in32(SMB_CSR(adap, R_SMB_STATUS));
> -	if (error & M_SMB_ERROR) {
> -		/* Clear error bit by writing a 1 */
> -		csr_out32(M_SMB_ERROR, SMB_CSR(adap, R_SMB_STATUS));
> -		return (error & M_SMB_ERROR_TYPE) ? -EIO : -ENXIO;
> +	spin_lock_irqsave(&adap->lock, flags);
> +
> +	if (adap->status > 0 && ((adap->status & (M_SMB_ERROR | M_SMB_ERROR_TYPE)) == M_SMB_ERROR)) {
> +		error = -ENXIO;
> +		goto out_unlock;
> +	}
> +	if (adap->status < 0 || (adap->status & (M_SMB_ERROR | M_SMB_BUSY))) {
> +		error = -EIO;
> +		goto out_unlock;
>  	}
>  
>  	if (data_bytes == 1)
> -		data->byte = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xff;
> +		data->byte = __raw_readq(csr + R_SMB_DATA) & 0xff;
>  	if (data_bytes == 2)
> -		data->word = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xffff;
> +		data->word = __raw_readq(csr + R_SMB_DATA) & 0xffff;
>  
> -	return 0;
> +	error = 0;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&adap->lock, flags);
> +
> +	return error;
>  }
>  
> -static u32 bit_func(struct i2c_adapter *adap)
> +static u32 i2c_sibyte_bit_func(struct i2c_adapter *adap)

If you're renaming this then please drop the "bit" part in it, it's
most likely coming from a copy-and-paste from i2c-algo-bit and has no
meaning in the sibyte driver.

>  {
>  	return (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>  		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA);
> @@ -125,8 +186,8 @@ static u32 bit_func(struct i2c_adapter *adap)
>  /* -----exported algorithm data: -------------------------------------	*/
>  
>  static const struct i2c_algorithm i2c_sibyte_algo = {
> -	.smbus_xfer	= smbus_xfer,
> -	.functionality	= bit_func,
> +	.smbus_xfer	= i2c_sibyte_smbus_xfer,
> +	.functionality	= i2c_sibyte_bit_func,
>  };
>  
>  /*
> @@ -135,37 +196,121 @@ static const struct i2c_algorithm i2c_sibyte_algo = {
>  static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
>  {
>  	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +	void __iomem *csr;
> +	int err;
>  
> -	/* Register new adapter to i2c module... */
> -	i2c_adap->algo = &i2c_sibyte_algo;
> +	adap->status = 0;
> +	init_waitqueue_head(&adap->wait);
> +	spin_lock_init(&adap->lock);
> +
> +	csr = ioremap(adap->base, R_SMB_PEC + SMB_REGISTER_SPACING);
> +	if (!csr) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +	adap->csr = csr;
>  
>  	/* Set the requested frequency. */
> -	csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
> -	csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
> +	__raw_writeq(speed, csr + R_SMB_FREQ);
> +
> +	/* Clear any pending error interrupt. */
> +	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +	/* Disable interrupts. */
> +	__raw_writeq(0, csr + R_SMB_CONTROL);
> +	mmiowb();
> +	__raw_readq(csr + R_SMB_CONTROL);

Shouldn't it be the other way around, disable interrupts first and then
clear any pending one? Looks racy otherwise, but maybe it makes no
difference in practice.

> +
> +	err = request_irq(adap->irq, i2c_sibyte_interrupt, IRQF_SHARED,
> +			  adap->name, i2c_adap);
> +	if (err < 0)
> +		goto out_unmap;
> +
> +	/* Enable finish and error interrupts. */
> +	__raw_writeq(M_SMB_FINISH_INTR | M_SMB_ERR_INTR, csr + R_SMB_CONTROL);
> +
> +	/* Register new adapter to i2c module... */
> +	err = i2c_add_numbered_adapter(i2c_adap);
> +	if (err < 0)
> +		goto out_unirq;
> +
> +	return 0;
>  
> -	return i2c_add_numbered_adapter(i2c_adap);
> +out_unirq:
> +	/* Disable interrupts. */
> +	__raw_writeq(0, csr + R_SMB_CONTROL);
> +	mmiowb();
> +	__raw_readq(csr + R_SMB_CONTROL);
> +
> +	free_irq(adap->irq, i2c_adap);
> +
> +	/* Clear any pending error interrupt. */
> +	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);

You may consider moving this block to a separate function, as it is
duplicated in the i2c_sibyte_remove_bus() function below.

> +out_unmap:
> +	iounmap(csr);
> +out:
> +	return err;
>  }
>  
> +static void i2c_sibyte_remove_bus(struct i2c_adapter *i2c_adap)
> +{
> +	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +	void __iomem *csr = adap->csr;
> +
> +	i2c_del_adapter(i2c_adap);
> +
> +	/* Disable interrupts. */
> +	__raw_writeq(0, csr + R_SMB_CONTROL);
> +	mmiowb();
> +	__raw_readq(csr + R_SMB_CONTROL);
>  
> -static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
> -	{ NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
> -	{ NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
> +	free_irq(adap->irq, i2c_adap);
> +
> +	/* Clear any pending error interrupt. */
> +	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +
> +	iounmap(csr);
> +}
> +
> +static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
> +#ifdef CONFIG_SIBYTE_SB1250
> +	{
> +		.name	= "sb1250-smbus-0",
> +		.base	= A_SMB_0,
> +		.irq	= K_INT_SMB_0,
> +	},
> +	{
> +		.name	= "sb1250-smbus-1",
> +		.base	= A_SMB_1,
> +		.irq	= K_INT_SMB_1,
> +	}
> +#else
> +	{
> +		.name	= "bcm1480-smbus-0",
> +		.base	= A_SMB_0,
> +		.irq	= K_BCM1480_INT_SMB_0,
> +	},
> +	{
> +		.name	= "bcm1480-smbus-1",
> +		.base	= A_SMB_1,
> +		.irq	= K_BCM1480_INT_SMB_1,
> +	}
> +#endif
>  };
>  
> -static struct i2c_adapter sibyte_board_adapter[2] = {
> +static struct i2c_adapter i2c_sibyte_board_adapter[2] = {
>  	{
>  		.owner		= THIS_MODULE,
>  		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -		.algo		= NULL,
> -		.algo_data	= &sibyte_board_data[0],
> +		.algo		= &i2c_sibyte_algo,
> +		.algo_data	= &i2c_sibyte_board_data[0],
>  		.nr		= 0,
>  		.name		= "SiByte SMBus 0",
>  	},
>  	{
>  		.owner		= THIS_MODULE,
>  		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -		.algo		= NULL,
> -		.algo_data	= &sibyte_board_data[1],
> +		.algo		= &i2c_sibyte_algo,
> +		.algo_data	= &i2c_sibyte_board_data[1],
>  		.nr		= 1,
>  		.name		= "SiByte SMBus 1",
>  	},
> @@ -173,21 +318,32 @@ static struct i2c_adapter sibyte_board_adapter[2] = {
>  
>  static int __init i2c_sibyte_init(void)
>  {
> +	int err;
> +
>  	pr_info("i2c-sibyte: i2c SMBus adapter module for SiByte board\n");
> -	if (i2c_sibyte_add_bus(&sibyte_board_adapter[0], K_SMB_FREQ_100KHZ) < 0)
> -		return -ENODEV;
> -	if (i2c_sibyte_add_bus(&sibyte_board_adapter[1],
> -			       K_SMB_FREQ_400KHZ) < 0) {
> -		i2c_del_adapter(&sibyte_board_adapter[0]);
> -		return -ENODEV;
> -	}
> +
> +	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[0],
> +				 K_SMB_FREQ_100KHZ);
> +	if (err < 0)
> +		goto out;
> +
> +	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[1],
> +				 K_SMB_FREQ_400KHZ);
> +	if (err < 0)
> +		goto out_remove;
> +
>  	return 0;
> +
> +out_remove:
> +	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
> +out:
> +	return err;
>  }
>  
>  static void __exit i2c_sibyte_exit(void)
>  {
> -	i2c_del_adapter(&sibyte_board_adapter[0]);
> -	i2c_del_adapter(&sibyte_board_adapter[1]);
> +	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[1]);
> +	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
>  }
>  
>  module_init(i2c_sibyte_init);

Please address my concerns where you agree and send an updated patch.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
> Please address my concerns where you agree and send an updated patch.

Matt, care to send an updated patch addressing my concerns? Otherwise
it will be lost again.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
> Please address my concerns where you agree and send an updated patch.

Matt, care to send an updated patch addressing my concerns? Otherwise
it will be lost again.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
> Please address my concerns where you agree and send an updated patch.

Matt, care to send an updated patch addressing my concerns? Otherwise
it will be lost again.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
> > Please address my concerns where you agree and send an updated patch.
> 
> Matt, care to send an updated patch addressing my concerns? Otherwise
> it will be lost again.

It's been 3 more months. Matt (or anyone else who cares and has access
to the hardware), please send an updated patch or I'll have to drop it.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
> > Please address my concerns where you agree and send an updated patch.
> 
> Matt, care to send an updated patch addressing my concerns? Otherwise
> it will be lost again.

It's been 3 more months. Matt (or anyone else who cares and has access
to the hardware), please send an updated patch or I'll have to drop it.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Tue, 10 Jan 2012, Jean Delvare wrote:

> > > Please address my concerns where you agree and send an updated patch.
> > 
> > Matt, care to send an updated patch addressing my concerns? Otherwise
> > it will be lost again.
> 
> It's been 3 more months. Matt (or anyone else who cares and has access
> to the hardware), please send an updated patch or I'll have to drop it.

 Thanks for nagging -- it seems unlikely I'll be able to have a look at it 
before February.  If Matt or anyone else cannot get at it before myself 
and you have to drop the change, then just do what you have to and drop 
it.  I'll try to resync with the current kernel as soon as I can, recheck 
my queue of outstanding patches and resend both this change and any other 
ones I'll consider necessary -- I reckon there were more than just this 
one.

 Thanks for your understanding and sorry about my recent lack of activity 
in this area.  All the best in the New Year! :)

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Tue, Jan 10, 2012 at 9:38 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
>> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
>> > Please address my concerns where you agree and send an updated patch.
>>
>> Matt, care to send an updated patch addressing my concerns? Otherwise
>> it will be lost again.
>
> It's been 3 more months. Matt (or anyone else who cares and has access
> to the hardware), please send an updated patch or I'll have to drop it.
>
> --
> Jean Delvare

I'll fix it up and resend the next time I'm working on the related mips stuff.

It's hard to prioritize volunteer work for hardware you and two other
people have. :)

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Tue, Jan 10, 2012 at 9:38 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
>> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
>> > Please address my concerns where you agree and send an updated patch.
>>
>> Matt, care to send an updated patch addressing my concerns? Otherwise
>> it will be lost again.
>
> It's been 3 more months. Matt (or anyone else who cares and has access
> to the hardware), please send an updated patch or I'll have to drop it.
>
> --
> Jean Delvare

I'll fix it up and resend the next time I'm working on the related mips stuff.

It's hard to prioritize volunteer work for hardware you and two other
people have. :)

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Thu, 12 Jan 2012 16:19:49 -0500, Matt Turner wrote:
> On Tue, Jan 10, 2012 at 9:38 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
> >> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
> >> > Please address my concerns where you agree and send an updated patch.
> >>
> >> Matt, care to send an updated patch addressing my concerns? Otherwise
> >> it will be lost again.
> >
> > It's been 3 more months. Matt (or anyone else who cares and has access
> > to the hardware), please send an updated patch or I'll have to drop it.
> 
> I'll fix it up and resend the next time I'm working on the related mips stuff.
> 
> It's hard to prioritize volunteer work for hardware you and two other
> people have. :)

Matt, Maciej, does any of you have an updated patch ready by now? If I
don't receive anything by the end of April I'll just drop it.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Thu, 12 Jan 2012 16:19:49 -0500, Matt Turner wrote:
> On Tue, Jan 10, 2012 at 9:38 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
> >> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
> >> > Please address my concerns where you agree and send an updated patch.
> >>
> >> Matt, care to send an updated patch addressing my concerns? Otherwise
> >> it will be lost again.
> >
> > It's been 3 more months. Matt (or anyone else who cares and has access
> > to the hardware), please send an updated patch or I'll have to drop it.
> 
> I'll fix it up and resend the next time I'm working on the related mips stuff.
> 
> It's hard to prioritize volunteer work for hardware you and two other
> people have. :)

Matt, Maciej, does any of you have an updated patch ready by now? If I
don't receive anything by the end of April I'll just drop it.

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Sat, Mar 31, 2012 at 2:23 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Thu, 12 Jan 2012 16:19:49 -0500, Matt Turner wrote:
>> On Tue, Jan 10, 2012 at 9:38 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>> > On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
>> >> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
>> >> > Please address my concerns where you agree and send an updated patch.
>> >>
>> >> Matt, care to send an updated patch addressing my concerns? Otherwise
>> >> it will be lost again.
>> >
>> > It's been 3 more months. Matt (or anyone else who cares and has access
>> > to the hardware), please send an updated patch or I'll have to drop it.
>>
>> I'll fix it up and resend the next time I'm working on the related mips stuff.
>>
>> It's hard to prioritize volunteer work for hardware you and two other
>> people have. :)
>
> Matt, Maciej, does any of you have an updated patch ready by now? If I
> don't receive anything by the end of April I'll just drop it.

I'll do my best to get you an updated patch.

Thanks for keeping after me.

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Sat, Mar 31, 2012 at 2:23 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Thu, 12 Jan 2012 16:19:49 -0500, Matt Turner wrote:
>> On Tue, Jan 10, 2012 at 9:38 AM, Jean Delvare <khali@linux-fr.org> wrote:
>> > On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
>> >> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
>> >> > Please address my concerns where you agree and send an updated patch.
>> >>
>> >> Matt, care to send an updated patch addressing my concerns? Otherwise
>> >> it will be lost again.
>> >
>> > It's been 3 more months. Matt (or anyone else who cares and has access
>> > to the hardware), please send an updated patch or I'll have to drop it.
>>
>> I'll fix it up and resend the next time I'm working on the related mips stuff.
>>
>> It's hard to prioritize volunteer work for hardware you and two other
>> people have. :)
>
> Matt, Maciej, does any of you have an updated patch ready by now? If I
> don't receive anything by the end of April I'll just drop it.

I'll do my best to get you an updated patch.

Thanks for keeping after me.

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Sat, 31 Mar 2012, Matt Turner wrote:

> >> It's hard to prioritize volunteer work for hardware you and two other
> >> people have. :)
> >
> > Matt, Maciej, does any of you have an updated patch ready by now? If I
> > don't receive anything by the end of April I'll just drop it.
> 
> I'll do my best to get you an updated patch.
> 
> Thanks for keeping after me.

 And thanks for looking after the patches -- regrettably I still have no 
ETA on updating my Linux sources to a version that would let me work on 
upstream patches, sigh...  I'll do my best to review or provide other 
feedback on these changes if needed though.

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Sat, 31 Mar 2012, Matt Turner wrote:

> >> It's hard to prioritize volunteer work for hardware you and two other
> >> people have. :)
> >
> > Matt, Maciej, does any of you have an updated patch ready by now? If I
> > don't receive anything by the end of April I'll just drop it.
> 
> I'll do my best to get you an updated patch.
> 
> Thanks for keeping after me.

 And thanks for looking after the patches -- regrettably I still have no 
ETA on updating my Linux sources to a version that would let me work on 
upstream patches, sigh...  I'll do my best to review or provide other 
feedback on these changes if needed though.

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Sat, Mar 31, 2012 at 8:11 AM, Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sat, Mar 31, 2012 at 2:23 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>> On Thu, 12 Jan 2012 16:19:49 -0500, Matt Turner wrote:
>>> On Tue, Jan 10, 2012 at 9:38 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>>> > On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
>>> >> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
>>> >> > Please address my concerns where you agree and send an updated patch.
>>> >>
>>> >> Matt, care to send an updated patch addressing my concerns? Otherwise
>>> >> it will be lost again.
>>> >
>>> > It's been 3 more months. Matt (or anyone else who cares and has access
>>> > to the hardware), please send an updated patch or I'll have to drop it.
>>>
>>> I'll fix it up and resend the next time I'm working on the related mips stuff.
>>>
>>> It's hard to prioritize volunteer work for hardware you and two other
>>> people have. :)
>>
>> Matt, Maciej, does any of you have an updated patch ready by now? If I
>> don't receive anything by the end of April I'll just drop it.
>
> I'll do my best to get you an updated patch.
>
> Thanks for keeping after me.
>
> Matt

Hi Jean,

I'm not going to have time to do this. :(

I had another look at the code, and I'm not sure I really understand
it well enough to address your concerns.

Good thing there are only about three users with this motherboard.

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Sat, Mar 31, 2012 at 8:11 AM, Matt Turner <mattst88@gmail.com> wrote:
> On Sat, Mar 31, 2012 at 2:23 AM, Jean Delvare <khali@linux-fr.org> wrote:
>> On Thu, 12 Jan 2012 16:19:49 -0500, Matt Turner wrote:
>>> On Tue, Jan 10, 2012 at 9:38 AM, Jean Delvare <khali@linux-fr.org> wrote:
>>> > On Mon, 31 Oct 2011 10:53:54 +0100, Jean Delvare wrote:
>>> >> On Sat, 3 Sep 2011 10:30:36 +0200, Jean Delvare wrote:
>>> >> > Please address my concerns where you agree and send an updated patch.
>>> >>
>>> >> Matt, care to send an updated patch addressing my concerns? Otherwise
>>> >> it will be lost again.
>>> >
>>> > It's been 3 more months. Matt (or anyone else who cares and has access
>>> > to the hardware), please send an updated patch or I'll have to drop it.
>>>
>>> I'll fix it up and resend the next time I'm working on the related mips stuff.
>>>
>>> It's hard to prioritize volunteer work for hardware you and two other
>>> people have. :)
>>
>> Matt, Maciej, does any of you have an updated patch ready by now? If I
>> don't receive anything by the end of April I'll just drop it.
>
> I'll do my best to get you an updated patch.
>
> Thanks for keeping after me.
>
> Matt

Hi Jean,

I'm not going to have time to do this. :(

I had another look at the code, and I'm not sure I really understand
it well enough to address your concerns.

Good thing there are only about three users with this motherboard.

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Sat, 30 Jun 2012, Matt Turner wrote:

> I'm not going to have time to do this. :(
> 
> I had another look at the code, and I'm not sure I really understand
> it well enough to address your concerns.

 I'll try then, as soon as I can.

> Good thing there are only about three users with this motherboard.

 Really?  I've thought Debian used them for MIPS distribution builds if 
nobody else.  AFAIK, ten years on and these systems (I mean the whole 
family) are still about the only ones reasonably widely available that 
provide performance decent enough for native use these days (yes, I did 
make serious use of an R3k DECstation natively once -- waiting for a GCC 
bootstrap to complete after the expected four weeks of computing time only 
to see it choke on a Makefile typo three weeks into was all but fun).

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Sat, 30 Jun 2012, Matt Turner wrote:

> I'm not going to have time to do this. :(
> 
> I had another look at the code, and I'm not sure I really understand
> it well enough to address your concerns.

 I'll try then, as soon as I can.

> Good thing there are only about three users with this motherboard.

 Really?  I've thought Debian used them for MIPS distribution builds if 
nobody else.  AFAIK, ten years on and these systems (I mean the whole 
family) are still about the only ones reasonably widely available that 
provide performance decent enough for native use these days (yes, I did 
make serious use of an R3k DECstation natively once -- waiting for a GCC 
bootstrap to complete after the expected four weeks of computing time only 
to see it choke on a Makefile typo three weeks into was all but fun).

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Thu, 19 Jul 2012 22:01:08 +0100 (BST), Maciej W. Rozycki wrote:
> On Sat, 30 Jun 2012, Matt Turner wrote:
> 
> > I'm not going to have time to do this. :(
> > 
> > I had another look at the code, and I'm not sure I really understand
> > it well enough to address your concerns.
> 
>  I'll try then, as soon as I can.

I'm giving up on this one, sorry. I've been waiting for an update for
over a year, this is simply too much. I just don't get why this patch
was ever submitted if there was no intent to actually get it included.
This has been a waste of many people's time :(

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Thu, 19 Jul 2012 22:01:08 +0100 (BST), Maciej W. Rozycki wrote:
> On Sat, 30 Jun 2012, Matt Turner wrote:
> 
> > I'm not going to have time to do this. :(
> > 
> > I had another look at the code, and I'm not sure I really understand
> > it well enough to address your concerns.
> 
>  I'll try then, as soon as I can.

I'm giving up on this one, sorry. I've been waiting for an update for
over a year, this is simply too much. I just don't get why this patch
was ever submitted if there was no intent to actually get it included.
This has been a waste of many people's time :(

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Jean Delvare]

On Thu, 19 Jul 2012 22:01:08 +0100 (BST), Maciej W. Rozycki wrote:
> On Sat, 30 Jun 2012, Matt Turner wrote:
> 
> > I'm not going to have time to do this. :(
> > 
> > I had another look at the code, and I'm not sure I really understand
> > it well enough to address your concerns.
> 
>  I'll try then, as soon as I can.

I'm giving up on this one, sorry. I've been waiting for an update for
over a year, this is simply too much. I just don't get why this patch
was ever submitted if there was no intent to actually get it included.
This has been a waste of many people's time :(

-- 
Jean Delvare

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Tue, Dec 07, 2010 at 09:30:27AM -0500, Maciej W. Rozycki wrote:
> On Mon, 6 Dec 2010, Guenter Roeck wrote:
> 
> > A quick look through sb1250 vs. sb1480 code shows that the 1480 uses different
> > interrupt numbers. The patch assigns the sb1250 interrupt numbers, so unless
> > I am missing something the code as written can not work for sb1480.
> 
>  That well could be -- I never had access to a BigSur board.  The 
> board-specific interrupt numbers should either be available from the board 
> manual (I haven't checked if one has been released; I certainly have one 
> for my SWARM) or quoted somewhere in our tree.  Otherwise figuring them 
> out by trial and error should be a trivial exercise for someone with 
> actual hardware at hand.
> 
I already sent a reply to the original patch - I confirmed that the interrupts are different.
Those are SOC interrupts, so they are CPU specific, not board specific. Code started working 
after I replaced the sb1250 interrupts with bcm1480 interrupts.

Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Tue, Dec 07, 2010 at 09:30:27AM -0500, Maciej W. Rozycki wrote:
> On Mon, 6 Dec 2010, Guenter Roeck wrote:
> 
> > A quick look through sb1250 vs. sb1480 code shows that the 1480 uses different
> > interrupt numbers. The patch assigns the sb1250 interrupt numbers, so unless
> > I am missing something the code as written can not work for sb1480.
> 
>  That well could be -- I never had access to a BigSur board.  The 
> board-specific interrupt numbers should either be available from the board 
> manual (I haven't checked if one has been released; I certainly have one 
> for my SWARM) or quoted somewhere in our tree.  Otherwise figuring them 
> out by trial and error should be a trivial exercise for someone with 
> actual hardware at hand.
> 
I already sent a reply to the original patch - I confirmed that the interrupts are different.
Those are SOC interrupts, so they are CPU specific, not board specific. Code started working 
after I replaced the sb1250 interrupts with bcm1480 interrupts.

Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Mon, 6 Dec 2010, Guenter Roeck wrote:

> A quick look through sb1250 vs. sb1480 code shows that the 1480 uses different
> interrupt numbers. The patch assigns the sb1250 interrupt numbers, so unless
> I am missing something the code as written can not work for sb1480.

 That well could be -- I never had access to a BigSur board.  The 
board-specific interrupt numbers should either be available from the board 
manual (I haven't checked if one has been released; I certainly have one 
for my SWARM) or quoted somewhere in our tree.  Otherwise figuring them 
out by trial and error should be a trivial exercise for someone with 
actual hardware at hand.

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Mon, 6 Dec 2010, Guenter Roeck wrote:

> A quick look through sb1250 vs. sb1480 code shows that the 1480 uses different
> interrupt numbers. The patch assigns the sb1250 interrupt numbers, so unless
> I am missing something the code as written can not work for sb1480.

 That well could be -- I never had access to a BigSur board.  The 
board-specific interrupt numbers should either be available from the board 
manual (I haven't checked if one has been released; I certainly have one 
for my SWARM) or quoted somewhere in our tree.  Otherwise figuring them 
out by trial and error should be a trivial exercise for someone with 
actual hardware at hand.

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
> From: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.
> 
> Minimally rebased by Matt Turner.
> 
> Tested-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

[ .. ] 
> 
> -static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
> -       { NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
> -       { NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
> +static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
> +       {
> +               .name   = "sb1250-smbus-0",
> +               .base   = A_SMB_0,
> +               .irq    = K_INT_SMB_0,
> +       },
> +       {
> +               .name   = "sb1250-smbus-1",
> +               .base   = A_SMB_1,
> +               .irq    = K_INT_SMB_1,

Found my problem. The .irq settings don't work for BCM1480.
It needs K_BCM1480_INT_SMB_0 and K_BCM1480_INT_SMB_1 from asm/sibyte/bcm1480_int.h.

For a clean fix, i2c_sibyte_board_data[] should probably be defined in a platform file, 
not in the i2c bus driver.

Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
> From: Maciej W. Rozycki <macro@linux-mips.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.
> 
> Minimally rebased by Matt Turner.
> 
> Tested-by: Matt Turner <mattst88@gmail.com>
> Signed-off-by: Matt Turner <mattst88@gmail.com>

[ .. ] 
> 
> -static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
> -       { NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
> -       { NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
> +static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
> +       {
> +               .name   = "sb1250-smbus-0",
> +               .base   = A_SMB_0,
> +               .irq    = K_INT_SMB_0,
> +       },
> +       {
> +               .name   = "sb1250-smbus-1",
> +               .base   = A_SMB_1,
> +               .irq    = K_INT_SMB_1,

Found my problem. The .irq settings don't work for BCM1480.
It needs K_BCM1480_INT_SMB_0 and K_BCM1480_INT_SMB_1 from asm/sibyte/bcm1480_int.h.

For a clean fix, i2c_sibyte_board_data[] should probably be defined in a platform file, 
not in the i2c bus driver.

Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 09:26:54PM -0500, Maciej W. Rozycki wrote:
[ ... ]
> 
>  Note that for IRQ support you may have to investigate dependencies in the 
> other two series as the patch (third above) was intended to apply on top 
> of the two series (select the date sort for easier identification of the 
> series).  I'd have to dig into that code for further details and I cannot 
> afford the time right now, sorry.
> 
A quick look through sb1250 vs. sb1480 code shows that the 1480 uses different
interrupt numbers. The patch assigns the sb1250 interrupt numbers, so unless
I am missing something the code as written can not work for sb1480.

Thanks,
Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 09:26:54PM -0500, Maciej W. Rozycki wrote:
[ ... ]
> 
>  Note that for IRQ support you may have to investigate dependencies in the 
> other two series as the patch (third above) was intended to apply on top 
> of the two series (select the date sort for easier identification of the 
> series).  I'd have to dig into that code for further details and I cannot 
> afford the time right now, sorry.
> 
A quick look through sb1250 vs. sb1480 code shows that the 1480 uses different
interrupt numbers. The patch assigns the sb1250 interrupt numbers, so unless
I am missing something the code as written can not work for sb1480.

Thanks,
Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Mon, 6 Dec 2010, Matt Turner wrote:

> >  Matt, thanks for keeping your eye on these bits and reviving them; I've
> > meant to do so for a long time now, but never came to it.  Please note
> > however, as I'm the original author, my original Signed-off-by markups
> > continue to apply and you should be quoting them with the submissions.
> > You should only add your own Signed-off-by annotation if you made any
> > changes and it would make sense to state what these changes were.
> 
> Sure thing. Will fix. For patches 2 and 3 of the other series, I don't
> think I was ever 100% sure that you were the author, since they were
> living on OpenWRT.org and I couldn't find them in any mailing list
> archives. Can you confirm that these 4 patches are all yours?

 All the relevant submissions should be present here:

http://www.linux-mips.org/archives/linux-mips/2008-05/threads.html

Specifically:

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=Pine.LNX.4.55.0805180447210.10067%40cliff.in.clinika.pl

(5th of a series of 6), and:

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=Pine.LNX.4.55.0805070054440.16173%40cliff.in.clinika.pl

(3rd of a series of 4), and:

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=Pine.LNX.4.55.0805130353250.535%40cliff.in.clinika.pl

(individual submission).

The "clean up SWARM RTC setup" change seems to be modified (lacking e.g. a 
proper read_persistent_clock() implementation) compared to my proposal 
(second above) and most likely came from someone else and the lm90 change 
definitely comes from someone else.

 Note that for IRQ support you may have to investigate dependencies in the 
other two series as the patch (third above) was intended to apply on top 
of the two series (select the date sort for easier identification of the 
series).  I'd have to dig into that code for further details and I cannot 
afford the time right now, sorry.

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Mon, 6 Dec 2010, Matt Turner wrote:

> >  Matt, thanks for keeping your eye on these bits and reviving them; I've
> > meant to do so for a long time now, but never came to it.  Please note
> > however, as I'm the original author, my original Signed-off-by markups
> > continue to apply and you should be quoting them with the submissions.
> > You should only add your own Signed-off-by annotation if you made any
> > changes and it would make sense to state what these changes were.
> 
> Sure thing. Will fix. For patches 2 and 3 of the other series, I don't
> think I was ever 100% sure that you were the author, since they were
> living on OpenWRT.org and I couldn't find them in any mailing list
> archives. Can you confirm that these 4 patches are all yours?

 All the relevant submissions should be present here:

http://www.linux-mips.org/archives/linux-mips/2008-05/threads.html

Specifically:

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=Pine.LNX.4.55.0805180447210.10067%40cliff.in.clinika.pl

(5th of a series of 6), and:

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=Pine.LNX.4.55.0805070054440.16173%40cliff.in.clinika.pl

(3rd of a series of 4), and:

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=Pine.LNX.4.55.0805130353250.535%40cliff.in.clinika.pl

(individual submission).

The "clean up SWARM RTC setup" change seems to be modified (lacking e.g. a 
proper read_persistent_clock() implementation) compared to my proposal 
(second above) and most likely came from someone else and the lm90 change 
definitely comes from someone else.

 Note that for IRQ support you may have to investigate dependencies in the 
other two series as the patch (third above) was intended to apply on top 
of the two series (select the date sort for easier identification of the 
series).  I'd have to dig into that code for further details and I cannot 
afford the time right now, sorry.

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Mon, Dec 6, 2010 at 6:02 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
> On Mon, Dec 06, 2010 at 12:40:15PM -0500, Matt Turner wrote:
>> On Mon, Dec 6, 2010 at 5:30 PM, Guenter Roeck
>> <guenter.roeck@ericsson.com> wrote:
>> > On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
>> >> From: Maciej W. Rozycki <macro@linux-mips.org>
>> >>
>> >> This is a rewrite of large parts of the driver mainly so that it uses
>> >> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
>> >> As a part of the overhaul of the init and exit calls, all accesses to the
>> >> hardware got converted to use accessory functions via an ioremap() cookie.
>> >>
>> >> Minimally rebased by Matt Turner.
>> >>
>> >> Tested-by: Matt Turner <mattst88@gmail.com>
>> >> Signed-off-by: Matt Turner <mattst88@gmail.com>
>> >
>> >
>> > I applied the patch to my 1480 tree. Unfortunately, it doesn't work with my system.
>> > As far as I can see, the driver does not get any interrupts.
>> >
>> > My tree is 2.6.32, though. Do you know if I might be missing some other relevant patch ?
>> >
>> > Thanks,
>> > Guenter
>>
>> I think this patch depends on
>> http://www.linux-mips.org/archives/linux-mips/2010-12/msg00030.html
>>
> I did apply the second patch as well, since you had mentioned it in your patch.
> That did not help, though. Frankly, I don't see the dependency in the first place - the other
> patch only affects drivers/rtc/rtc-m41t80.c, and I would hope that SMBus support does not depend
> on an rtc driver. Am I missing something ?
>
> Thanks,
> Guenter

Indeed that does not make much sense. I really don't know. Perhaps
Maciej can shed some light on this? It certainly might be the case
that these patches haven't ever been tested on a BCM91480 before.

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Mon, Dec 6, 2010 at 5:56 PM, Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org> wrote:
>  Matt, thanks for keeping your eye on these bits and reviving them; I've
> meant to do so for a long time now, but never came to it.  Please note
> however, as I'm the original author, my original Signed-off-by markups
> continue to apply and you should be quoting them with the submissions.
> You should only add your own Signed-off-by annotation if you made any
> changes and it would make sense to state what these changes were.

Sure thing. Will fix. For patches 2 and 3 of the other series, I don't
think I was ever 100% sure that you were the author, since they were
living on OpenWRT.org and I couldn't find them in any mailing list
archives. Can you confirm that these 4 patches are all yours?

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Mon, Dec 6, 2010 at 5:56 PM, Maciej W. Rozycki <macro@linux-mips.org> wrote:
>  Matt, thanks for keeping your eye on these bits and reviving them; I've
> meant to do so for a long time now, but never came to it.  Please note
> however, as I'm the original author, my original Signed-off-by markups
> continue to apply and you should be quoting them with the submissions.
> You should only add your own Signed-off-by annotation if you made any
> changes and it would make sense to state what these changes were.

Sure thing. Will fix. For patches 2 and 3 of the other series, I don't
think I was ever 100% sure that you were the author, since they were
living on OpenWRT.org and I couldn't find them in any mailing list
archives. Can you confirm that these 4 patches are all yours?

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 12:40:15PM -0500, Matt Turner wrote:
> On Mon, Dec 6, 2010 at 5:30 PM, Guenter Roeck
> <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> wrote:
> > On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
> >> From: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> >>
> >> This is a rewrite of large parts of the driver mainly so that it uses
> >> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> >> As a part of the overhaul of the init and exit calls, all accesses to the
> >> hardware got converted to use accessory functions via an ioremap() cookie.
> >>
> >> Minimally rebased by Matt Turner.
> >>
> >> Tested-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >
> >
> > I applied the patch to my 1480 tree. Unfortunately, it doesn't work with my system.
> > As far as I can see, the driver does not get any interrupts.
> >
> > My tree is 2.6.32, though. Do you know if I might be missing some other relevant patch ?
> >
> > Thanks,
> > Guenter
> 
> I think this patch depends on
> http://www.linux-mips.org/archives/linux-mips/2010-12/msg00030.html
> 
I did apply the second patch as well, since you had mentioned it in your patch.
That did not help, though. Frankly, I don't see the dependency in the first place - the other 
patch only affects drivers/rtc/rtc-m41t80.c, and I would hope that SMBus support does not depend
on an rtc driver. Am I missing something ?

Thanks,
Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 12:40:15PM -0500, Matt Turner wrote:
> On Mon, Dec 6, 2010 at 5:30 PM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> > On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
> >> From: Maciej W. Rozycki <macro@linux-mips.org>
> >>
> >> This is a rewrite of large parts of the driver mainly so that it uses
> >> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> >> As a part of the overhaul of the init and exit calls, all accesses to the
> >> hardware got converted to use accessory functions via an ioremap() cookie.
> >>
> >> Minimally rebased by Matt Turner.
> >>
> >> Tested-by: Matt Turner <mattst88@gmail.com>
> >> Signed-off-by: Matt Turner <mattst88@gmail.com>
> >
> >
> > I applied the patch to my 1480 tree. Unfortunately, it doesn't work with my system.
> > As far as I can see, the driver does not get any interrupts.
> >
> > My tree is 2.6.32, though. Do you know if I might be missing some other relevant patch ?
> >
> > Thanks,
> > Guenter
> 
> I think this patch depends on
> http://www.linux-mips.org/archives/linux-mips/2010-12/msg00030.html
> 
I did apply the second patch as well, since you had mentioned it in your patch.
That did not help, though. Frankly, I don't see the dependency in the first place - the other 
patch only affects drivers/rtc/rtc-m41t80.c, and I would hope that SMBus support does not depend
on an rtc driver. Am I missing something ?

Thanks,
Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

On Mon, 6 Dec 2010, Guenter Roeck wrote:

> > From: Maciej W. Rozycki <macro@linux-mips.org>
> > 
> > This is a rewrite of large parts of the driver mainly so that it uses
> > SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> > As a part of the overhaul of the init and exit calls, all accesses to the
> > hardware got converted to use accessory functions via an ioremap() cookie.
> > 
> > Minimally rebased by Matt Turner.
> > 
> > Tested-by: Matt Turner <mattst88@gmail.com>
> > Signed-off-by: Matt Turner <mattst88@gmail.com>
> 
> I applied the patch to my 1480 tree. Unfortunately, it doesn't work with 
> my system. As far as I can see, the driver does not get any interrupts.
> 
> My tree is 2.6.32, though. Do you know if I might be missing some other 
> relevant patch ?

 As the original author I apologise for the lack of response about these 
changes -- I've had a really, really hectic time recently and will 
continue to suffer from that for several weeks yet at the very least.

 As to the patches -- these I submitted originally back in 2008 as a 
series.  There may have been more than one series actually, but I can't 
recall the details offhand.  There were some discussions and concerns 
about some of the patches which in the end I did not fully address owing 
to various disruptions and the lack of time, which is why they did not go 
in.  I do remember some bits about interrupt handling as the original 
implementation of the I2C host interface used polling only and I saw it as 
a gross inefficiency.  Obviously with all the bits in place they used to 
work at least for me.

 Matt, thanks for keeping your eye on these bits and reviving them; I've 
meant to do so for a long time now, but never came to it.  Please note 
however, as I'm the original author, my original Signed-off-by markups 
continue to apply and you should be quoting them with the submissions.  
You should only add your own Signed-off-by annotation if you made any 
changes and it would make sense to state what these changes were.

 I'll do my best to provide some aid with these bits, but won't be able to 
do anything but plain code review up till January at the very least, and 
then maybe not even that.  My SWARM board has been stuck with 2.6.27-ish 
for a long while now.  Sorry.

  Maciej

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Mon, Dec 6, 2010 at 5:30 PM, Guenter Roeck
<guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> wrote:
> On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
>> From: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
>>
>> This is a rewrite of large parts of the driver mainly so that it uses
>> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
>> As a part of the overhaul of the init and exit calls, all accesses to the
>> hardware got converted to use accessory functions via an ioremap() cookie.
>>
>> Minimally rebased by Matt Turner.
>>
>> Tested-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
>
> I applied the patch to my 1480 tree. Unfortunately, it doesn't work with my system.
> As far as I can see, the driver does not get any interrupts.
>
> My tree is 2.6.32, though. Do you know if I might be missing some other relevant patch ?
>
> Thanks,
> Guenter

I think this patch depends on
http://www.linux-mips.org/archives/linux-mips/2010-12/msg00030.html

Thanks for testing and the suggestions! :)

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

On Mon, Dec 6, 2010 at 5:30 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
> On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
>> From: Maciej W. Rozycki <macro@linux-mips.org>
>>
>> This is a rewrite of large parts of the driver mainly so that it uses
>> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
>> As a part of the overhaul of the init and exit calls, all accesses to the
>> hardware got converted to use accessory functions via an ioremap() cookie.
>>
>> Minimally rebased by Matt Turner.
>>
>> Tested-by: Matt Turner <mattst88@gmail.com>
>> Signed-off-by: Matt Turner <mattst88@gmail.com>
>
>
> I applied the patch to my 1480 tree. Unfortunately, it doesn't work with my system.
> As far as I can see, the driver does not get any interrupts.
>
> My tree is 2.6.32, though. Do you know if I might be missing some other relevant patch ?
>
> Thanks,
> Guenter

I think this patch depends on
http://www.linux-mips.org/archives/linux-mips/2010-12/msg00030.html

Thanks for testing and the suggestions! :)

Matt

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
> From: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.
> 
> Minimally rebased by Matt Turner.
> 
> Tested-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


I applied the patch to my 1480 tree. Unfortunately, it doesn't work with my system.
As far as I can see, the driver does not get any interrupts.

My tree is 2.6.32, though. Do you know if I might be missing some other relevant patch ?

Thanks,
Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
> From: Maciej W. Rozycki <macro@linux-mips.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.
> 
> Minimally rebased by Matt Turner.
> 
> Tested-by: Matt Turner <mattst88@gmail.com>
> Signed-off-by: Matt Turner <mattst88@gmail.com>


I applied the patch to my 1480 tree. Unfortunately, it doesn't work with my system.
As far as I can see, the driver does not get any interrupts.

My tree is 2.6.32, though. Do you know if I might be missing some other relevant patch ?

Thanks,
Guenter

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
> From: Maciej W. Rozycki <macro-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.
> 
> Minimally rebased by Matt Turner.
> 
> Tested-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Matt Turner <mattst88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> This patch was originally sent in May 2008 [1], but appears to have been lost.
> 
> I believe this patch depends on
> [PATCH 1/3] RTC: SMBus support for the M41T80, etc. driver
> 
> Please review the change in return values (search for ENXIO). I wasn't entirely
> sure how this code should look. (The code the original patch was against just
> returned -1 on error. See 102b59c6d6d30fb6560177fd1ae8a34c4c163897). Please
> apply if acceptable.
> 
> [1] http://lists.lm-sensors.org/pipermail/i2c/2008-May/003638.html
> 
>  drivers/i2c/busses/i2c-sibyte.c |  278 +++++++++++++++++++++++++++++----------
>  1 files changed, 208 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sibyte.c b/drivers/i2c/busses/i2c-sibyte.c
> index 0fe505d..283747c 100644
> --- a/drivers/i2c/busses/i2c-sibyte.c
> +++ b/drivers/i2c/busses/i2c-sibyte.c
> @@ -2,6 +2,7 @@
>   * Copyright (C) 2004 Steven J. Hill
>   * Copyright (C) 2001,2002,2003 Broadcom Corporation
>   * Copyright (C) 1995-2000 Simon G. Vogl
> + * Copyright (C) 2008 Maciej W. Rozycki
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -18,104 +19,159 @@
>   * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
>   */
> 
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
> +#include <linux/param.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
>  #include <linux/io.h>
> +#include <asm/sibyte/sb1250_int.h>
>  #include <asm/sibyte/sb1250_regs.h>
>  #include <asm/sibyte/sb1250_smbus.h>
> 
> 
>  struct i2c_algo_sibyte_data {
> -       void *data;             /* private data */
> -       int   bus;              /* which bus */
> -       void *reg_base;         /* CSR base */
> +       wait_queue_head_t       wait;           /* IRQ queue */
> +       void __iomem            *csr;           /* mapped CSR handle */
> +       phys_t                  base;           /* physical CSR base */
> +       char                    *name;          /* IRQ handler name */
> +       spinlock_t              lock;           /* atomiser */
> +       int                     irq;            /* IRQ line */
> +       int                     status;         /* IRQ status */
>  };
> 
> -/* ----- global defines ----------------------------------------------- */
> -#define SMB_CSR(a,r) ((long)(a->reg_base + r))
> 
> +static irqreturn_t i2c_sibyte_interrupt(int irq, void *dev_id)
> +{
> +       struct i2c_adapter *i2c_adap = dev_id;
> +       struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr = adap->csr;
> +       u8 status;
> +
> +       /*
> +        * Ugh, no way to detect the finish interrupt,
> +        * but if busy it is obviously not one.
> +        */
> +       status = __raw_readq(csr + R_SMB_STATUS);
> +       if ((status & (M_SMB_ERROR | M_SMB_BUSY)) == M_SMB_BUSY)
> +               return IRQ_NONE;
> 
> -static int smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> -                     unsigned short flags, char read_write,
> -                     u8 command, int size, union i2c_smbus_data * data)
> +       /*
> +        * Clear the error interrupt (write 1 to clear);
> +        * the finish interrupt was cleared by the read above.
> +        */
> +       __raw_writeq(status, csr + R_SMB_STATUS);
> +
> +       /* Post the status. */
> +       spin_lock_irq(&adap->lock);
> +       adap->status = status & (M_SMB_ERROR_TYPE | M_SMB_ERROR | M_SMB_BUSY);
> +       wake_up(&adap->wait);
> +       spin_unlock_irq(&adap->lock);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static s32 i2c_sibyte_smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> +                                unsigned short cflags,
> +                                char read_write, u8 command, int size,
> +                                union i2c_smbus_data *data)
>  {
>         struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr = adap->csr;
> +       unsigned long flags;
>         int data_bytes = 0;
>         int error;
> 
> -       while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -               ;
> +       spin_lock_irqsave(&adap->lock, flags);
> +
> +       if (adap->status < 0) {
> +               error = -EIO;
> +               goto out_unlock;
> +       }

If a previous operation timed out, subsequent operations will fail forever. 
Is this a good idea ? Maybe it is - just asking.

> 
>         switch (size) {
>         case I2C_SMBUS_QUICK:
> -               csr_out32((V_SMB_ADDR(addr) |
> -                          (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> -                          V_SMB_TT_QUICKCMD), SMB_CSR(adap, R_SMB_START));
> +               __raw_writeq(V_SMB_ADDR(addr) |
> +                            (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> +                            V_SMB_TT_QUICKCMD,
> +                            csr + R_SMB_START);
>                 break;
>         case I2C_SMBUS_BYTE:
>                 if (read_write == I2C_SMBUS_READ) {
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE,
> +                                    csr + R_SMB_START);
>                         data_bytes = 1;
>                 } else {
> -                       csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE,
> +                                    csr + R_SMB_START);
>                 }
>                 break;
>         case I2C_SMBUS_BYTE_DATA:
> -               csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +               __raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>                 if (read_write == I2C_SMBUS_READ) {
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE,
> +                                    csr + R_SMB_START);
>                         data_bytes = 1;
>                 } else {
> -                       csr_out32(V_SMB_LB(data->byte),
> -                                 SMB_CSR(adap, R_SMB_DATA));
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_LB(data->byte), csr + R_SMB_DATA);
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +                                    csr + R_SMB_START);
>                 }
>                 break;
>         case I2C_SMBUS_WORD_DATA:
> -               csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +               __raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>                 if (read_write == I2C_SMBUS_READ) {
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE,
> +                                    csr + R_SMB_START);
>                         data_bytes = 2;
>                 } else {
> -                       csr_out32(V_SMB_LB(data->word & 0xff),
> -                                 SMB_CSR(adap, R_SMB_DATA));
> -                       csr_out32(V_SMB_MB(data->word >> 8),
> -                                 SMB_CSR(adap, R_SMB_DATA));
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_LB(data->word & 0xff),
> +                                    csr + R_SMB_DATA);
> +                       __raw_writeq(V_SMB_MB(data->word >> 8),
> +                                    csr + R_SMB_DATA);
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +                                    csr + R_SMB_START);
>                 }
>                 break;
>         default:
> -               return -EOPNOTSUPP;
> +               error = -EOPNOTSUPP;
> +               goto out_unlock;
>         }
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_START);
> +       adap->status = -1;
> +
> +       spin_unlock_irqrestore(&adap->lock, flags);
> +
> +       wait_event_timeout(adap->wait, (adap->status >= 0), HZ);
> 
> -       while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -               ;
> +       spin_lock_irqsave(&adap->lock, flags);
> 
> -       error = csr_in32(SMB_CSR(adap, R_SMB_STATUS));
> -       if (error & M_SMB_ERROR) {
> -               /* Clear error bit by writing a 1 */
> -               csr_out32(M_SMB_ERROR, SMB_CSR(adap, R_SMB_STATUS));
> -               return (error & M_SMB_ERROR_TYPE) ? -EIO : -ENXIO;
> +       if (adap->status < 0 || (adap->status & (M_SMB_ERROR | M_SMB_BUSY))) {
> +               error = -EIO;
> +               goto out_unlock;
>         }

The idea was to return -ENXIO if there was no ACK from a device (per Documentation/i2c/fault-codes).
With your change, this distinction gets lost. I think you should retain the original semantics,
ie return -ENXIO if status > 0 && ((status & (M_SMB_ERROR | M_SMB_ERROR_TYPE) == M_SMB_ERROR).

> 
>         if (data_bytes == 1)
> -               data->byte = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xff;
> +               data->byte = __raw_readq(csr + R_SMB_DATA) & 0xff;
>         if (data_bytes == 2)
> -               data->word = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xffff;
> +               data->word = __raw_readq(csr + R_SMB_DATA) & 0xffff;
> 
> -       return 0;
> +       error = 0;
> +
> +out_unlock:
> +       spin_unlock_irqrestore(&adap->lock, flags);
> +
> +       return error;
>  }
> 
> -static u32 bit_func(struct i2c_adapter *adap)
> +static u32 i2c_sibyte_bit_func(struct i2c_adapter *adap)
>  {
>         return (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>                 I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA);
> @@ -125,8 +181,8 @@ static u32 bit_func(struct i2c_adapter *adap)
>  /* -----exported algorithm data: ------------------------------------- */
> 
>  static const struct i2c_algorithm i2c_sibyte_algo = {
> -       .smbus_xfer     = smbus_xfer,
> -       .functionality  = bit_func,
> +       .smbus_xfer     = i2c_sibyte_smbus_xfer,
> +       .functionality  = i2c_sibyte_bit_func,
>  };
> 
>  /*
> @@ -135,37 +191,108 @@ static const struct i2c_algorithm i2c_sibyte_algo = {
>  static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
>  {
>         struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr;
> +       int err;
> 
> -       /* Register new adapter to i2c module... */
> -       i2c_adap->algo = &i2c_sibyte_algo;
> +       adap->status = 0;
> +       init_waitqueue_head(&adap->wait);
> +       spin_lock_init(&adap->lock);
> +
> +       csr = ioremap(adap->base, R_SMB_PEC + SMB_REGISTER_SPACING);
> +       if (!csr) {
> +               err = -ENOMEM;
> +               goto out;
> +       }
> +       adap->csr = csr;
> 
>         /* Set the requested frequency. */
> -       csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
> -       csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
> +       __raw_writeq(speed, csr + R_SMB_FREQ);
> +
> +       /* Clear any pending error interrupt. */
> +       __raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +       /* Disable interrupts. */
> +       __raw_writeq(0, csr + R_SMB_CONTROL);
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_CONTROL);
> +
> +       err = request_irq(adap->irq, i2c_sibyte_interrupt, IRQF_SHARED,
> +                         adap->name, i2c_adap);
> +       if (err < 0)
> +               goto out_unmap;
> +
> +       /* Enable finish and error interrupts. */
> +       __raw_writeq(M_SMB_FINISH_INTR | M_SMB_ERR_INTR, csr + R_SMB_CONTROL);
> +
> +       /* Register new adapter to i2c module... */
> +       err = i2c_add_numbered_adapter(i2c_adap);
> +       if (err < 0)
> +               goto out_unirq;
> 
> -       return i2c_add_numbered_adapter(i2c_adap);
> +       return 0;
> +
> +out_unirq:
> +       /* Disable interrupts. */
> +       __raw_writeq(0, csr + R_SMB_CONTROL);
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_CONTROL);
> +
> +       free_irq(adap->irq, i2c_adap);
> +
> +       /* Clear any pending error interrupt. */
> +       __raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +out_unmap:
> +       iounmap(csr);
> +out:
> +       return err;
>  }
> 
> +static void i2c_sibyte_remove_bus(struct i2c_adapter *i2c_adap)
> +{
> +       struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr = adap->csr;
> +
> +       i2c_del_adapter(i2c_adap);
> +
> +       /* Disable interrupts. */
> +       __raw_writeq(0, csr + R_SMB_CONTROL);
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_CONTROL);
> +
> +       free_irq(adap->irq, i2c_adap);
> +
> +       /* Clear any pending error interrupt. */
> +       __raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +
> +       iounmap(csr);
> +}
> 
> -static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
> -       { NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
> -       { NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
> +static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
> +       {
> +               .name   = "sb1250-smbus-0",
> +               .base   = A_SMB_0,
> +               .irq    = K_INT_SMB_0,
> +       },
> +       {
> +               .name   = "sb1250-smbus-1",
> +               .base   = A_SMB_1,
> +               .irq    = K_INT_SMB_1,
> +       }
>  };
> 
> -static struct i2c_adapter sibyte_board_adapter[2] = {
> +static struct i2c_adapter i2c_sibyte_board_adapter[2] = {
>         {
>                 .owner          = THIS_MODULE,
>                 .class          = I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -               .algo           = NULL,
> -               .algo_data      = &sibyte_board_data[0],
> +               .algo           = &i2c_sibyte_algo,
> +               .algo_data      = &i2c_sibyte_board_data[0],
>                 .nr             = 0,
>                 .name           = "SiByte SMBus 0",
>         },
>         {
>                 .owner          = THIS_MODULE,
>                 .class          = I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -               .algo           = NULL,
> -               .algo_data      = &sibyte_board_data[1],
> +               .algo           = &i2c_sibyte_algo,
> +               .algo_data      = &i2c_sibyte_board_data[1],
>                 .nr             = 1,
>                 .name           = "SiByte SMBus 1",
>         },
> @@ -173,21 +300,32 @@ static struct i2c_adapter sibyte_board_adapter[2] = {
> 
>  static int __init i2c_sibyte_init(void)
>  {
> +       int err;
> +
>         pr_info("i2c-sibyte: i2c SMBus adapter module for SiByte board\n");
> -       if (i2c_sibyte_add_bus(&sibyte_board_adapter[0], K_SMB_FREQ_100KHZ) < 0)
> -               return -ENODEV;
> -       if (i2c_sibyte_add_bus(&sibyte_board_adapter[1],
> -                              K_SMB_FREQ_400KHZ) < 0) {
> -               i2c_del_adapter(&sibyte_board_adapter[0]);
> -               return -ENODEV;
> -       }
> +
> +       err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[0],
> +                                K_SMB_FREQ_100KHZ);
> +       if (err < 0)
> +               goto out;
> +
> +       err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[1],
> +                                K_SMB_FREQ_400KHZ);
> +       if (err < 0)
> +               goto out_remove;
> +
>         return 0;
> +
> +out_remove:
> +       i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
> +out:
> +       return err;
>  }
> 
>  static void __exit i2c_sibyte_exit(void)
>  {
> -       i2c_del_adapter(&sibyte_board_adapter[0]);
> -       i2c_del_adapter(&sibyte_board_adapter[1]);
> +       i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[1]);
> +       i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
>  }
> 
>  module_init(i2c_sibyte_init);
> --
> 1.7.2.2
> 
> 

Re: [PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Guenter Roeck]

On Mon, Dec 06, 2010 at 01:38:14AM -0500, Matt Turner wrote:
> From: Maciej W. Rozycki <macro@linux-mips.org>
> 
> This is a rewrite of large parts of the driver mainly so that it uses
> SMBus interrupts to offload the CPU from busy-waiting on status inputs.
> As a part of the overhaul of the init and exit calls, all accesses to the
> hardware got converted to use accessory functions via an ioremap() cookie.
> 
> Minimally rebased by Matt Turner.
> 
> Tested-by: Matt Turner <mattst88@gmail.com>
> Signed-off-by: Matt Turner <mattst88@gmail.com>
> ---
> This patch was originally sent in May 2008 [1], but appears to have been lost.
> 
> I believe this patch depends on
> [PATCH 1/3] RTC: SMBus support for the M41T80, etc. driver
> 
> Please review the change in return values (search for ENXIO). I wasn't entirely
> sure how this code should look. (The code the original patch was against just
> returned -1 on error. See 102b59c6d6d30fb6560177fd1ae8a34c4c163897). Please
> apply if acceptable.
> 
> [1] http://lists.lm-sensors.org/pipermail/i2c/2008-May/003638.html
> 
>  drivers/i2c/busses/i2c-sibyte.c |  278 +++++++++++++++++++++++++++++----------
>  1 files changed, 208 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sibyte.c b/drivers/i2c/busses/i2c-sibyte.c
> index 0fe505d..283747c 100644
> --- a/drivers/i2c/busses/i2c-sibyte.c
> +++ b/drivers/i2c/busses/i2c-sibyte.c
> @@ -2,6 +2,7 @@
>   * Copyright (C) 2004 Steven J. Hill
>   * Copyright (C) 2001,2002,2003 Broadcom Corporation
>   * Copyright (C) 1995-2000 Simon G. Vogl
> + * Copyright (C) 2008 Maciej W. Rozycki
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -18,104 +19,159 @@
>   * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
>   */
> 
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
> +#include <linux/param.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
>  #include <linux/io.h>
> +#include <asm/sibyte/sb1250_int.h>
>  #include <asm/sibyte/sb1250_regs.h>
>  #include <asm/sibyte/sb1250_smbus.h>
> 
> 
>  struct i2c_algo_sibyte_data {
> -       void *data;             /* private data */
> -       int   bus;              /* which bus */
> -       void *reg_base;         /* CSR base */
> +       wait_queue_head_t       wait;           /* IRQ queue */
> +       void __iomem            *csr;           /* mapped CSR handle */
> +       phys_t                  base;           /* physical CSR base */
> +       char                    *name;          /* IRQ handler name */
> +       spinlock_t              lock;           /* atomiser */
> +       int                     irq;            /* IRQ line */
> +       int                     status;         /* IRQ status */
>  };
> 
> -/* ----- global defines ----------------------------------------------- */
> -#define SMB_CSR(a,r) ((long)(a->reg_base + r))
> 
> +static irqreturn_t i2c_sibyte_interrupt(int irq, void *dev_id)
> +{
> +       struct i2c_adapter *i2c_adap = dev_id;
> +       struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr = adap->csr;
> +       u8 status;
> +
> +       /*
> +        * Ugh, no way to detect the finish interrupt,
> +        * but if busy it is obviously not one.
> +        */
> +       status = __raw_readq(csr + R_SMB_STATUS);
> +       if ((status & (M_SMB_ERROR | M_SMB_BUSY)) == M_SMB_BUSY)
> +               return IRQ_NONE;
> 
> -static int smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> -                     unsigned short flags, char read_write,
> -                     u8 command, int size, union i2c_smbus_data * data)
> +       /*
> +        * Clear the error interrupt (write 1 to clear);
> +        * the finish interrupt was cleared by the read above.
> +        */
> +       __raw_writeq(status, csr + R_SMB_STATUS);
> +
> +       /* Post the status. */
> +       spin_lock_irq(&adap->lock);
> +       adap->status = status & (M_SMB_ERROR_TYPE | M_SMB_ERROR | M_SMB_BUSY);
> +       wake_up(&adap->wait);
> +       spin_unlock_irq(&adap->lock);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static s32 i2c_sibyte_smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
> +                                unsigned short cflags,
> +                                char read_write, u8 command, int size,
> +                                union i2c_smbus_data *data)
>  {
>         struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr = adap->csr;
> +       unsigned long flags;
>         int data_bytes = 0;
>         int error;
> 
> -       while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -               ;
> +       spin_lock_irqsave(&adap->lock, flags);
> +
> +       if (adap->status < 0) {
> +               error = -EIO;
> +               goto out_unlock;
> +       }

If a previous operation timed out, subsequent operations will fail forever. 
Is this a good idea ? Maybe it is - just asking.

> 
>         switch (size) {
>         case I2C_SMBUS_QUICK:
> -               csr_out32((V_SMB_ADDR(addr) |
> -                          (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> -                          V_SMB_TT_QUICKCMD), SMB_CSR(adap, R_SMB_START));
> +               __raw_writeq(V_SMB_ADDR(addr) |
> +                            (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
> +                            V_SMB_TT_QUICKCMD,
> +                            csr + R_SMB_START);
>                 break;
>         case I2C_SMBUS_BYTE:
>                 if (read_write == I2C_SMBUS_READ) {
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE,
> +                                    csr + R_SMB_START);
>                         data_bytes = 1;
>                 } else {
> -                       csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE,
> +                                    csr + R_SMB_START);
>                 }
>                 break;
>         case I2C_SMBUS_BYTE_DATA:
> -               csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +               __raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>                 if (read_write == I2C_SMBUS_READ) {
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE,
> +                                    csr + R_SMB_START);
>                         data_bytes = 1;
>                 } else {
> -                       csr_out32(V_SMB_LB(data->byte),
> -                                 SMB_CSR(adap, R_SMB_DATA));
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_LB(data->byte), csr + R_SMB_DATA);
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +                                    csr + R_SMB_START);
>                 }
>                 break;
>         case I2C_SMBUS_WORD_DATA:
> -               csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
> +               __raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
>                 if (read_write == I2C_SMBUS_READ) {
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE,
> +                                    csr + R_SMB_START);
>                         data_bytes = 2;
>                 } else {
> -                       csr_out32(V_SMB_LB(data->word & 0xff),
> -                                 SMB_CSR(adap, R_SMB_DATA));
> -                       csr_out32(V_SMB_MB(data->word >> 8),
> -                                 SMB_CSR(adap, R_SMB_DATA));
> -                       csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
> -                                 SMB_CSR(adap, R_SMB_START));
> +                       __raw_writeq(V_SMB_LB(data->word & 0xff),
> +                                    csr + R_SMB_DATA);
> +                       __raw_writeq(V_SMB_MB(data->word >> 8),
> +                                    csr + R_SMB_DATA);
> +                       __raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
> +                                    csr + R_SMB_START);
>                 }
>                 break;
>         default:
> -               return -EOPNOTSUPP;
> +               error = -EOPNOTSUPP;
> +               goto out_unlock;
>         }
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_START);
> +       adap->status = -1;
> +
> +       spin_unlock_irqrestore(&adap->lock, flags);
> +
> +       wait_event_timeout(adap->wait, (adap->status >= 0), HZ);
> 
> -       while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
> -               ;
> +       spin_lock_irqsave(&adap->lock, flags);
> 
> -       error = csr_in32(SMB_CSR(adap, R_SMB_STATUS));
> -       if (error & M_SMB_ERROR) {
> -               /* Clear error bit by writing a 1 */
> -               csr_out32(M_SMB_ERROR, SMB_CSR(adap, R_SMB_STATUS));
> -               return (error & M_SMB_ERROR_TYPE) ? -EIO : -ENXIO;
> +       if (adap->status < 0 || (adap->status & (M_SMB_ERROR | M_SMB_BUSY))) {
> +               error = -EIO;
> +               goto out_unlock;
>         }

The idea was to return -ENXIO if there was no ACK from a device (per Documentation/i2c/fault-codes).
With your change, this distinction gets lost. I think you should retain the original semantics,
ie return -ENXIO if status > 0 && ((status & (M_SMB_ERROR | M_SMB_ERROR_TYPE) == M_SMB_ERROR).

> 
>         if (data_bytes == 1)
> -               data->byte = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xff;
> +               data->byte = __raw_readq(csr + R_SMB_DATA) & 0xff;
>         if (data_bytes == 2)
> -               data->word = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xffff;
> +               data->word = __raw_readq(csr + R_SMB_DATA) & 0xffff;
> 
> -       return 0;
> +       error = 0;
> +
> +out_unlock:
> +       spin_unlock_irqrestore(&adap->lock, flags);
> +
> +       return error;
>  }
> 
> -static u32 bit_func(struct i2c_adapter *adap)
> +static u32 i2c_sibyte_bit_func(struct i2c_adapter *adap)
>  {
>         return (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>                 I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA);
> @@ -125,8 +181,8 @@ static u32 bit_func(struct i2c_adapter *adap)
>  /* -----exported algorithm data: ------------------------------------- */
> 
>  static const struct i2c_algorithm i2c_sibyte_algo = {
> -       .smbus_xfer     = smbus_xfer,
> -       .functionality  = bit_func,
> +       .smbus_xfer     = i2c_sibyte_smbus_xfer,
> +       .functionality  = i2c_sibyte_bit_func,
>  };
> 
>  /*
> @@ -135,37 +191,108 @@ static const struct i2c_algorithm i2c_sibyte_algo = {
>  static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
>  {
>         struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr;
> +       int err;
> 
> -       /* Register new adapter to i2c module... */
> -       i2c_adap->algo = &i2c_sibyte_algo;
> +       adap->status = 0;
> +       init_waitqueue_head(&adap->wait);
> +       spin_lock_init(&adap->lock);
> +
> +       csr = ioremap(adap->base, R_SMB_PEC + SMB_REGISTER_SPACING);
> +       if (!csr) {
> +               err = -ENOMEM;
> +               goto out;
> +       }
> +       adap->csr = csr;
> 
>         /* Set the requested frequency. */
> -       csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
> -       csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
> +       __raw_writeq(speed, csr + R_SMB_FREQ);
> +
> +       /* Clear any pending error interrupt. */
> +       __raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +       /* Disable interrupts. */
> +       __raw_writeq(0, csr + R_SMB_CONTROL);
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_CONTROL);
> +
> +       err = request_irq(adap->irq, i2c_sibyte_interrupt, IRQF_SHARED,
> +                         adap->name, i2c_adap);
> +       if (err < 0)
> +               goto out_unmap;
> +
> +       /* Enable finish and error interrupts. */
> +       __raw_writeq(M_SMB_FINISH_INTR | M_SMB_ERR_INTR, csr + R_SMB_CONTROL);
> +
> +       /* Register new adapter to i2c module... */
> +       err = i2c_add_numbered_adapter(i2c_adap);
> +       if (err < 0)
> +               goto out_unirq;
> 
> -       return i2c_add_numbered_adapter(i2c_adap);
> +       return 0;
> +
> +out_unirq:
> +       /* Disable interrupts. */
> +       __raw_writeq(0, csr + R_SMB_CONTROL);
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_CONTROL);
> +
> +       free_irq(adap->irq, i2c_adap);
> +
> +       /* Clear any pending error interrupt. */
> +       __raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +out_unmap:
> +       iounmap(csr);
> +out:
> +       return err;
>  }
> 
> +static void i2c_sibyte_remove_bus(struct i2c_adapter *i2c_adap)
> +{
> +       struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
> +       void __iomem *csr = adap->csr;
> +
> +       i2c_del_adapter(i2c_adap);
> +
> +       /* Disable interrupts. */
> +       __raw_writeq(0, csr + R_SMB_CONTROL);
> +       mmiowb();
> +       __raw_readq(csr + R_SMB_CONTROL);
> +
> +       free_irq(adap->irq, i2c_adap);
> +
> +       /* Clear any pending error interrupt. */
> +       __raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
> +
> +       iounmap(csr);
> +}
> 
> -static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
> -       { NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
> -       { NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
> +static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
> +       {
> +               .name   = "sb1250-smbus-0",
> +               .base   = A_SMB_0,
> +               .irq    = K_INT_SMB_0,
> +       },
> +       {
> +               .name   = "sb1250-smbus-1",
> +               .base   = A_SMB_1,
> +               .irq    = K_INT_SMB_1,
> +       }
>  };
> 
> -static struct i2c_adapter sibyte_board_adapter[2] = {
> +static struct i2c_adapter i2c_sibyte_board_adapter[2] = {
>         {
>                 .owner          = THIS_MODULE,
>                 .class          = I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -               .algo           = NULL,
> -               .algo_data      = &sibyte_board_data[0],
> +               .algo           = &i2c_sibyte_algo,
> +               .algo_data      = &i2c_sibyte_board_data[0],
>                 .nr             = 0,
>                 .name           = "SiByte SMBus 0",
>         },
>         {
>                 .owner          = THIS_MODULE,
>                 .class          = I2C_CLASS_HWMON | I2C_CLASS_SPD,
> -               .algo           = NULL,
> -               .algo_data      = &sibyte_board_data[1],
> +               .algo           = &i2c_sibyte_algo,
> +               .algo_data      = &i2c_sibyte_board_data[1],
>                 .nr             = 1,
>                 .name           = "SiByte SMBus 1",
>         },
> @@ -173,21 +300,32 @@ static struct i2c_adapter sibyte_board_adapter[2] = {
> 
>  static int __init i2c_sibyte_init(void)
>  {
> +       int err;
> +
>         pr_info("i2c-sibyte: i2c SMBus adapter module for SiByte board\n");
> -       if (i2c_sibyte_add_bus(&sibyte_board_adapter[0], K_SMB_FREQ_100KHZ) < 0)
> -               return -ENODEV;
> -       if (i2c_sibyte_add_bus(&sibyte_board_adapter[1],
> -                              K_SMB_FREQ_400KHZ) < 0) {
> -               i2c_del_adapter(&sibyte_board_adapter[0]);
> -               return -ENODEV;
> -       }
> +
> +       err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[0],
> +                                K_SMB_FREQ_100KHZ);
> +       if (err < 0)
> +               goto out;
> +
> +       err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[1],
> +                                K_SMB_FREQ_400KHZ);
> +       if (err < 0)
> +               goto out_remove;
> +
>         return 0;
> +
> +out_remove:
> +       i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
> +out:
> +       return err;
>  }
> 
>  static void __exit i2c_sibyte_exit(void)
>  {
> -       i2c_del_adapter(&sibyte_board_adapter[0]);
> -       i2c_del_adapter(&sibyte_board_adapter[1]);
> +       i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[1]);
> +       i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
>  }
> 
>  module_init(i2c_sibyte_init);
> --
> 1.7.2.2
> 
> 

[PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Matt Turner]

From: Maciej W. Rozycki <macro@linux-mips.org>

This is a rewrite of large parts of the driver mainly so that it uses
SMBus interrupts to offload the CPU from busy-waiting on status inputs.
As a part of the overhaul of the init and exit calls, all accesses to the
hardware got converted to use accessory functions via an ioremap() cookie.

Minimally rebased by Matt Turner.

Tested-by: Matt Turner <mattst88@gmail.com>
Signed-off-by: Matt Turner <mattst88@gmail.com>
---
This patch was originally sent in May 2008 [1], but appears to have been lost.

I believe this patch depends on
[PATCH 1/3] RTC: SMBus support for the M41T80, etc. driver

Please review the change in return values (search for ENXIO). I wasn't entirely
sure how this code should look. (The code the original patch was against just
returned -1 on error. See 102b59c6d6d30fb6560177fd1ae8a34c4c163897). Please
apply if acceptable.

[1] http://lists.lm-sensors.org/pipermail/i2c/2008-May/003638.html

 drivers/i2c/busses/i2c-sibyte.c |  278 +++++++++++++++++++++++++++++----------
 1 files changed, 208 insertions(+), 70 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sibyte.c b/drivers/i2c/busses/i2c-sibyte.c
index 0fe505d..283747c 100644
--- a/drivers/i2c/busses/i2c-sibyte.c
+++ b/drivers/i2c/busses/i2c-sibyte.c
@@ -2,6 +2,7 @@
  * Copyright (C) 2004 Steven J. Hill
  * Copyright (C) 2001,2002,2003 Broadcom Corporation
  * Copyright (C) 1995-2000 Simon G. Vogl
+ * Copyright (C) 2008 Maciej W. Rozycki
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -18,104 +19,159 @@
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
  */
 
+#include <linux/errno.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/param.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/wait.h>
 #include <linux/io.h>
+#include <asm/sibyte/sb1250_int.h>
 #include <asm/sibyte/sb1250_regs.h>
 #include <asm/sibyte/sb1250_smbus.h>
 
 
 struct i2c_algo_sibyte_data {
-	void *data;		/* private data */
-	int   bus;		/* which bus */
-	void *reg_base;		/* CSR base */
+	wait_queue_head_t	wait;		/* IRQ queue */
+	void __iomem		*csr;		/* mapped CSR handle */
+	phys_t			base;		/* physical CSR base */
+	char			*name;		/* IRQ handler name */
+	spinlock_t		lock;		/* atomiser */
+	int			irq;		/* IRQ line */
+	int			status;		/* IRQ status */
 };
 
-/* ----- global defines ----------------------------------------------- */
-#define SMB_CSR(a,r) ((long)(a->reg_base + r))
 
+static irqreturn_t i2c_sibyte_interrupt(int irq, void *dev_id)
+{
+	struct i2c_adapter *i2c_adap = dev_id;
+	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+	u8 status;
+
+	/*
+	 * Ugh, no way to detect the finish interrupt,
+	 * but if busy it is obviously not one.
+	 */
+	status = __raw_readq(csr + R_SMB_STATUS);
+	if ((status & (M_SMB_ERROR | M_SMB_BUSY)) == M_SMB_BUSY)
+		return IRQ_NONE;
 
-static int smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
-		      unsigned short flags, char read_write,
-		      u8 command, int size, union i2c_smbus_data * data)
+	/*
+	 * Clear the error interrupt (write 1 to clear);
+	 * the finish interrupt was cleared by the read above.
+	 */
+	__raw_writeq(status, csr + R_SMB_STATUS);
+
+	/* Post the status. */
+	spin_lock_irq(&adap->lock);
+	adap->status = status & (M_SMB_ERROR_TYPE | M_SMB_ERROR | M_SMB_BUSY);
+	wake_up(&adap->wait);
+	spin_unlock_irq(&adap->lock);
+
+	return IRQ_HANDLED;
+}
+
+static s32 i2c_sibyte_smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
+				 unsigned short cflags,
+				 char read_write, u8 command, int size,
+				 union i2c_smbus_data *data)
 {
 	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+	unsigned long flags;
 	int data_bytes = 0;
 	int error;
 
-	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
-		;
+	spin_lock_irqsave(&adap->lock, flags);
+
+	if (adap->status < 0) {
+		error = -EIO;
+		goto out_unlock;
+	}
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		csr_out32((V_SMB_ADDR(addr) |
-			   (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
-			   V_SMB_TT_QUICKCMD), SMB_CSR(adap, R_SMB_START));
+		__raw_writeq(V_SMB_ADDR(addr) |
+			     (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
+			     V_SMB_TT_QUICKCMD,
+			     csr + R_SMB_START);
 		break;
 	case I2C_SMBUS_BYTE:
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 1;
 		} else {
-			csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
+		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 1;
 		} else {
-			csr_out32(V_SMB_LB(data->byte),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_LB(data->byte), csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	case I2C_SMBUS_WORD_DATA:
-		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
+		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 2;
 		} else {
-			csr_out32(V_SMB_LB(data->word & 0xff),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32(V_SMB_MB(data->word >> 8),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_LB(data->word & 0xff),
+				     csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_MB(data->word >> 8),
+				     csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	default:
-		return -EOPNOTSUPP;
+		error = -EOPNOTSUPP;
+		goto out_unlock;
 	}
+	mmiowb();
+	__raw_readq(csr + R_SMB_START);
+	adap->status = -1;
+
+	spin_unlock_irqrestore(&adap->lock, flags);
+
+	wait_event_timeout(adap->wait, (adap->status >= 0), HZ);
 
-	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
-		;
+	spin_lock_irqsave(&adap->lock, flags);
 
-	error = csr_in32(SMB_CSR(adap, R_SMB_STATUS));
-	if (error & M_SMB_ERROR) {
-		/* Clear error bit by writing a 1 */
-		csr_out32(M_SMB_ERROR, SMB_CSR(adap, R_SMB_STATUS));
-		return (error & M_SMB_ERROR_TYPE) ? -EIO : -ENXIO;
+	if (adap->status < 0 || (adap->status & (M_SMB_ERROR | M_SMB_BUSY))) {
+		error = -EIO;
+		goto out_unlock;
 	}
 
 	if (data_bytes == 1)
-		data->byte = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xff;
+		data->byte = __raw_readq(csr + R_SMB_DATA) & 0xff;
 	if (data_bytes == 2)
-		data->word = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xffff;
+		data->word = __raw_readq(csr + R_SMB_DATA) & 0xffff;
 
-	return 0;
+	error = 0;
+
+out_unlock:
+	spin_unlock_irqrestore(&adap->lock, flags);
+
+	return error;
 }
 
-static u32 bit_func(struct i2c_adapter *adap)
+static u32 i2c_sibyte_bit_func(struct i2c_adapter *adap)
 {
 	return (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA);
@@ -125,8 +181,8 @@ static u32 bit_func(struct i2c_adapter *adap)
 /* -----exported algorithm data: -------------------------------------	*/
 
 static const struct i2c_algorithm i2c_sibyte_algo = {
-	.smbus_xfer	= smbus_xfer,
-	.functionality	= bit_func,
+	.smbus_xfer	= i2c_sibyte_smbus_xfer,
+	.functionality	= i2c_sibyte_bit_func,
 };
 
 /*
@@ -135,37 +191,108 @@ static const struct i2c_algorithm i2c_sibyte_algo = {
 static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
 {
 	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr;
+	int err;
 
-	/* Register new adapter to i2c module... */
-	i2c_adap->algo = &i2c_sibyte_algo;
+	adap->status = 0;
+	init_waitqueue_head(&adap->wait);
+	spin_lock_init(&adap->lock);
+
+	csr = ioremap(adap->base, R_SMB_PEC + SMB_REGISTER_SPACING);
+	if (!csr) {
+		err = -ENOMEM;
+		goto out;
+	}
+	adap->csr = csr;
 
 	/* Set the requested frequency. */
-	csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
-	csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
+	__raw_writeq(speed, csr + R_SMB_FREQ);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	err = request_irq(adap->irq, i2c_sibyte_interrupt, IRQF_SHARED,
+			  adap->name, i2c_adap);
+	if (err < 0)
+		goto out_unmap;
+
+	/* Enable finish and error interrupts. */
+	__raw_writeq(M_SMB_FINISH_INTR | M_SMB_ERR_INTR, csr + R_SMB_CONTROL);
+
+	/* Register new adapter to i2c module... */
+	err = i2c_add_numbered_adapter(i2c_adap);
+	if (err < 0)
+		goto out_unirq;
 
-	return i2c_add_numbered_adapter(i2c_adap);
+	return 0;
+
+out_unirq:
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	free_irq(adap->irq, i2c_adap);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+out_unmap:
+	iounmap(csr);
+out:
+	return err;
 }
 
+static void i2c_sibyte_remove_bus(struct i2c_adapter *i2c_adap)
+{
+	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+
+	i2c_del_adapter(i2c_adap);
+
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	free_irq(adap->irq, i2c_adap);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+
+	iounmap(csr);
+}
 
-static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
-	{ NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
-	{ NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
+static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
+	{
+		.name	= "sb1250-smbus-0",
+		.base	= A_SMB_0,
+		.irq	= K_INT_SMB_0,
+	},
+	{
+		.name	= "sb1250-smbus-1",
+		.base	= A_SMB_1,
+		.irq	= K_INT_SMB_1,
+	}
 };
 
-static struct i2c_adapter sibyte_board_adapter[2] = {
+static struct i2c_adapter i2c_sibyte_board_adapter[2] = {
 	{
 		.owner		= THIS_MODULE,
 		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
-		.algo		= NULL,
-		.algo_data	= &sibyte_board_data[0],
+		.algo		= &i2c_sibyte_algo,
+		.algo_data	= &i2c_sibyte_board_data[0],
 		.nr		= 0,
 		.name		= "SiByte SMBus 0",
 	},
 	{
 		.owner		= THIS_MODULE,
 		.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
-		.algo		= NULL,
-		.algo_data	= &sibyte_board_data[1],
+		.algo		= &i2c_sibyte_algo,
+		.algo_data	= &i2c_sibyte_board_data[1],
 		.nr		= 1,
 		.name		= "SiByte SMBus 1",
 	},
@@ -173,21 +300,32 @@ static struct i2c_adapter sibyte_board_adapter[2] = {
 
 static int __init i2c_sibyte_init(void)
 {
+	int err;
+
 	pr_info("i2c-sibyte: i2c SMBus adapter module for SiByte board\n");
-	if (i2c_sibyte_add_bus(&sibyte_board_adapter[0], K_SMB_FREQ_100KHZ) < 0)
-		return -ENODEV;
-	if (i2c_sibyte_add_bus(&sibyte_board_adapter[1],
-			       K_SMB_FREQ_400KHZ) < 0) {
-		i2c_del_adapter(&sibyte_board_adapter[0]);
-		return -ENODEV;
-	}
+
+	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[0],
+				 K_SMB_FREQ_100KHZ);
+	if (err < 0)
+		goto out;
+
+	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[1],
+				 K_SMB_FREQ_400KHZ);
+	if (err < 0)
+		goto out_remove;
+
 	return 0;
+
+out_remove:
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
+out:
+	return err;
 }
 
 static void __exit i2c_sibyte_exit(void)
 {
-	i2c_del_adapter(&sibyte_board_adapter[0]);
-	i2c_del_adapter(&sibyte_board_adapter[1]);
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[1]);
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
 }
 
 module_init(i2c_sibyte_init);
-- 
1.7.2.2

[PATCH] I2C: SiByte: Convert the driver to make use of interrupts

[Maciej W. Rozycki]

 This is a rewrite of large parts of the driver mainly so that it uses 
SMBus interrupts to offload the CPU from busy-waiting on status inputs.  
As a part of the overhaul of the init and exit calls, all accesses to the 
hardware got converted to use accessory functions via an ioremap() cookie.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
 This patch depends on patch-2.6.26-rc1-20080505-swarm-i2c-16 -- submitted 
as a part of the recent set of M41T80 RTC changes.

 Jean, please let me know if you prefer the ioremap() changes separately.  
Similarly, how about the -EINVAL/-EIO error codes?  Also please note how I
hesitated from adding second space after final full stops within comments.

  Maciej

patch-2.6.26-rc1-20080505-sibyte-i2c-irq-5
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/drivers/i2c/busses/i2c-sibyte.c linux-2.6.26-rc1-20080505/drivers/i2c/busses/i2c-sibyte.c
--- linux-2.6.26-rc1-20080505.macro/drivers/i2c/busses/i2c-sibyte.c	2008-05-11 02:25:56.000000000 +0000
+++ linux-2.6.26-rc1-20080505/drivers/i2c/busses/i2c-sibyte.c	2008-05-12 06:00:16.000000000 +0000
@@ -2,6 +2,7 @@
  * Copyright (C) 2004 Steven J. Hill
  * Copyright (C) 2001,2002,2003 Broadcom Corporation
  * Copyright (C) 1995-2000 Simon G. Vogl
+ * Copyright (C) 2008 Maciej W. Rozycki
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -18,104 +19,159 @@
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
  */
 
+#include <linux/errno.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/param.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/wait.h>
 #include <asm/io.h>
+#include <asm/sibyte/sb1250_int.h>
 #include <asm/sibyte/sb1250_regs.h>
 #include <asm/sibyte/sb1250_smbus.h>
 
 
 struct i2c_algo_sibyte_data {
-	void *data;		/* private data */
-	int   bus;		/* which bus */
-	void *reg_base;		/* CSR base */
+	wait_queue_head_t	wait;		/* IRQ queue */
+	void __iomem		*csr;		/* mapped CSR handle */
+	phys_t			base;		/* physical CSR base */
+	char			*name;		/* IRQ handler name */
+	spinlock_t		lock;		/* atomiser */
+	int			irq;		/* IRQ line */
+	int			status;		/* IRQ status */
 };
 
-/* ----- global defines ----------------------------------------------- */
-#define SMB_CSR(a,r) ((long)(a->reg_base + r))
 
+static irqreturn_t i2c_sibyte_interrupt(int irq, void *dev_id)
+{
+	struct i2c_adapter *i2c_adap = dev_id;
+	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+	u8 status;
+
+	/*
+	 * Ugh, no way to detect the finish interrupt,
+	 * but if busy it is obviously not one.
+	 */
+	status = __raw_readq(csr + R_SMB_STATUS);
+	if ((status & (M_SMB_ERROR | M_SMB_BUSY)) == M_SMB_BUSY)
+		return IRQ_NONE;
+
+	/*
+	 * Clear the error interrupt (write 1 to clear);
+	 * the finish interrupt was cleared by the read above.
+	 */
+	__raw_writeq(status, csr + R_SMB_STATUS);
+
+	/* Post the status. */
+	spin_lock_irq(&adap->lock);
+	adap->status = status & (M_SMB_ERROR_TYPE | M_SMB_ERROR | M_SMB_BUSY);
+	wake_up(&adap->wait);
+	spin_unlock_irq(&adap->lock);
+
+	return IRQ_HANDLED;
+}
 
-static int smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
-		      unsigned short flags, char read_write,
-		      u8 command, int size, union i2c_smbus_data * data)
+static s32 i2c_sibyte_smbus_xfer(struct i2c_adapter *i2c_adap, u16 addr,
+				 unsigned short cflags,
+				 char read_write, u8 command, int size,
+				 union i2c_smbus_data *data)
 {
 	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+	unsigned long flags;
 	int data_bytes = 0;
 	int error;
 
-	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
-		;
+	spin_lock_irqsave(&adap->lock, flags);
+
+	if (adap->status < 0) {
+		error = -EIO;
+		goto out_unlock;
+	}
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		csr_out32((V_SMB_ADDR(addr) |
-			   (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
-			   V_SMB_TT_QUICKCMD), SMB_CSR(adap, R_SMB_START));
+		__raw_writeq(V_SMB_ADDR(addr) |
+			     (read_write == I2C_SMBUS_READ ? M_SMB_QDATA : 0) |
+			     V_SMB_TT_QUICKCMD,
+			     csr + R_SMB_START);
 		break;
 	case I2C_SMBUS_BYTE:
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_RD1BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 1;
 		} else {
-			csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR1BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
+		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD1BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 1;
 		} else {
-			csr_out32(V_SMB_LB(data->byte),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_LB(data->byte), csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	case I2C_SMBUS_WORD_DATA:
-		csr_out32(V_SMB_CMD(command), SMB_CSR(adap, R_SMB_CMD));
+		__raw_writeq(V_SMB_CMD(command), csr + R_SMB_CMD);
 		if (read_write == I2C_SMBUS_READ) {
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_CMD_RD2BYTE,
+				     csr + R_SMB_START);
 			data_bytes = 2;
 		} else {
-			csr_out32(V_SMB_LB(data->word & 0xff),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32(V_SMB_MB(data->word >> 8),
-				  SMB_CSR(adap, R_SMB_DATA));
-			csr_out32((V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE),
-				  SMB_CSR(adap, R_SMB_START));
+			__raw_writeq(V_SMB_LB(data->word & 0xff),
+				     csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_MB(data->word >> 8),
+				     csr + R_SMB_DATA);
+			__raw_writeq(V_SMB_ADDR(addr) | V_SMB_TT_WR2BYTE,
+				     csr + R_SMB_START);
 		}
 		break;
 	default:
-		return -1;      /* XXXKW better error code? */
+		error = -EINVAL;
+		goto out_unlock;
 	}
+	mmiowb();
+	__raw_readq(csr + R_SMB_START);
+	adap->status = -1;
+
+	spin_unlock_irqrestore(&adap->lock, flags);
 
-	while (csr_in32(SMB_CSR(adap, R_SMB_STATUS)) & M_SMB_BUSY)
-		;
+	wait_event_timeout(adap->wait, (adap->status >= 0), HZ);
 
-	error = csr_in32(SMB_CSR(adap, R_SMB_STATUS));
-	if (error & M_SMB_ERROR) {
-		/* Clear error bit by writing a 1 */
-		csr_out32(M_SMB_ERROR, SMB_CSR(adap, R_SMB_STATUS));
-		return -1;      /* XXXKW better error code? */
+	spin_lock_irqsave(&adap->lock, flags);
+
+	if (adap->status < 0 || (adap->status & (M_SMB_ERROR | M_SMB_BUSY))) {
+		error = -EIO;
+		goto out_unlock;
 	}
 
 	if (data_bytes == 1)
-		data->byte = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xff;
+		data->byte = __raw_readq(csr + R_SMB_DATA) & 0xff;
 	if (data_bytes == 2)
-		data->word = csr_in32(SMB_CSR(adap, R_SMB_DATA)) & 0xffff;
+		data->word = __raw_readq(csr + R_SMB_DATA) & 0xffff;
 
-	return 0;
+	error = 0;
+
+out_unlock:
+	spin_unlock_irqrestore(&adap->lock, flags);
+
+	return error;
 }
 
-static u32 bit_func(struct i2c_adapter *adap)
+static u32 i2c_sibyte_bit_func(struct i2c_adapter *adap)
 {
 	return (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA);
@@ -125,8 +181,8 @@ static u32 bit_func(struct i2c_adapter *
 /* -----exported algorithm data: -------------------------------------	*/
 
 static const struct i2c_algorithm i2c_sibyte_algo = {
-	.smbus_xfer	= smbus_xfer,
-	.functionality	= bit_func,
+	.smbus_xfer	= i2c_sibyte_smbus_xfer,
+	.functionality	= i2c_sibyte_bit_func,
 };
 
 /*
@@ -135,30 +191,101 @@ static const struct i2c_algorithm i2c_si
 static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
 {
 	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr;
+	int err;
 
-	/* Register new adapter to i2c module... */
-	i2c_adap->algo = &i2c_sibyte_algo;
+	adap->status = 0;
+	init_waitqueue_head(&adap->wait);
+	spin_lock_init(&adap->lock);
+
+	csr = ioremap(adap->base, R_SMB_PEC + SMB_REGISTER_SPACING);
+	if (!csr) {
+		err = -ENOMEM;
+		goto out;
+	}
+	adap->csr = csr;
 
 	/* Set the requested frequency. */
-	csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
-	csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
+	__raw_writeq(speed, csr + R_SMB_FREQ);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	err = request_irq(adap->irq, i2c_sibyte_interrupt, IRQF_SHARED,
+			  adap->name, i2c_adap);
+	if (err < 0)
+		goto out_unmap;
 
-	return i2c_add_numbered_adapter(i2c_adap);
+	/* Enable finish and error interrupts. */
+	__raw_writeq(M_SMB_FINISH_INTR | M_SMB_ERR_INTR, csr + R_SMB_CONTROL);
+
+	/* Register new adapter to i2c module... */
+	err = i2c_add_numbered_adapter(i2c_adap);
+	if (err < 0)
+		goto out_unirq;
+
+	return 0;
+
+out_unirq:
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	free_irq(adap->irq, i2c_adap);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+out_unmap:
+	iounmap(csr);
+out:
+	return err;
 }
 
+static void i2c_sibyte_remove_bus(struct i2c_adapter *i2c_adap)
+{
+	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
+	void __iomem *csr = adap->csr;
+
+	i2c_del_adapter(i2c_adap);
+
+	/* Disable interrupts. */
+	__raw_writeq(0, csr + R_SMB_CONTROL);
+	mmiowb();
+	__raw_readq(csr + R_SMB_CONTROL);
+
+	free_irq(adap->irq, i2c_adap);
+
+	/* Clear any pending error interrupt. */
+	__raw_writeq(__raw_readq(csr + R_SMB_STATUS), csr + R_SMB_STATUS);
+
+	iounmap(csr);
+}
 
-static struct i2c_algo_sibyte_data sibyte_board_data[2] = {
-	{ NULL, 0, (void *) (CKSEG1+A_SMB_BASE(0)) },
-	{ NULL, 1, (void *) (CKSEG1+A_SMB_BASE(1)) }
+static struct i2c_algo_sibyte_data i2c_sibyte_board_data[2] = {
+	{
+		.name	= "sb1250-smbus-0",
+		.base	= A_SMB_0,
+		.irq	= K_INT_SMB_0,
+	},
+	{
+		.name	= "sb1250-smbus-1",
+		.base	= A_SMB_1,
+		.irq	= K_INT_SMB_1,
+	}
 };
 
-static struct i2c_adapter sibyte_board_adapter[2] = {
+static struct i2c_adapter i2c_sibyte_board_adapter[2] = {
 	{
 		.owner		= THIS_MODULE,
 		.id		= I2C_HW_SIBYTE,
 		.class		= I2C_CLASS_HWMON,
-		.algo		= NULL,
-		.algo_data	= &sibyte_board_data[0],
+		.algo		= &i2c_sibyte_algo,
+		.algo_data	= &i2c_sibyte_board_data[0],
 		.nr		= 0,
 		.name		= "SiByte SMBus 0",
 	},
@@ -166,8 +293,8 @@ static struct i2c_adapter sibyte_board_a
 		.owner		= THIS_MODULE,
 		.id		= I2C_HW_SIBYTE,
 		.class		= I2C_CLASS_HWMON,
-		.algo		= NULL,
-		.algo_data	= &sibyte_board_data[1],
+		.algo		= &i2c_sibyte_algo,
+		.algo_data	= &i2c_sibyte_board_data[1],
 		.nr		= 1,
 		.name		= "SiByte SMBus 1",
 	},
@@ -175,21 +302,32 @@ static struct i2c_adapter sibyte_board_a
 
 static int __init i2c_sibyte_init(void)
 {
+	int err;
+
 	pr_info("i2c-sibyte: i2c SMBus adapter module for SiByte board\n");
-	if (i2c_sibyte_add_bus(&sibyte_board_adapter[0], K_SMB_FREQ_100KHZ) < 0)
-		return -ENODEV;
-	if (i2c_sibyte_add_bus(&sibyte_board_adapter[1],
-			       K_SMB_FREQ_400KHZ) < 0) {
-		i2c_del_adapter(&sibyte_board_adapter[0]);
-		return -ENODEV;
-	}
+
+	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[0],
+				 K_SMB_FREQ_100KHZ);
+	if (err < 0)
+		goto out;
+
+	err = i2c_sibyte_add_bus(&i2c_sibyte_board_adapter[1],
+				 K_SMB_FREQ_400KHZ);
+	if (err < 0)
+		goto out_remove;
+
 	return 0;
+
+out_remove:
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
+out:
+	return err;
 }
 
 static void __exit i2c_sibyte_exit(void)
 {
-	i2c_del_adapter(&sibyte_board_adapter[0]);
-	i2c_del_adapter(&sibyte_board_adapter[1]);
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[1]);
+	i2c_sibyte_remove_bus(&i2c_sibyte_board_adapter[0]);
 }
 
 module_init(i2c_sibyte_init);