[PATCH] i2c: Renesas Highlander FPGA SMBus support.

[PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Paul Mundt]

This adds support for the SMBus adapter found in the various FPGAs on
the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
R0P7785LC0011RL FPGAs.

Functionality is fairly restricted, in that only byte and block data
transfers are supported. Normal/fast mode and IRQ/polling are also
supported. Primarily used for various RTCs and thermal sensors.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

---

 drivers/i2c/busses/Kconfig          |   12 
 drivers/i2c/busses/Makefile         |    1 
 drivers/i2c/busses/i2c-highlander.c |  500 ++++++++++++++++++++++++++++++++++++
 3 files changed, 513 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 476b0bb..3376d53 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -146,6 +146,18 @@ config I2C_GPIO
 	  This is a very simple bitbanging I2C driver utilizing the
 	  arch-neutral GPIO API to control the SCL and SDA lines.
 
+config I2C_HIGHLANDER
+	tristate "Highlander FPGA SMBus interface"
+	depends on SH_HIGHLANDER
+	help
+	  If you say yes to this option, support will be included for
+	  the SMBus interface located in the FPGA on various Highlander
+	  boards, particularly the R0P7780LC0011RL and R0P7785LC0011RL
+	  FPGAs. This is wholly unrelated to the SoC I2C.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-highlander.
+
 config I2C_HYDRA
 	tristate "CHRP Apple Hydra Mac I/O I2C interface"
 	depends on PCI && PPC_CHRP && EXPERIMENTAL
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index ea7068f..d179bce 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
+obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
 obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_I801)		+= i2c-i801.o
 obj-$(CONFIG_I2C_I810)		+= i2c-i810.o
diff --git a/drivers/i2c/busses/i2c-highlander.c b/drivers/i2c/busses/i2c-highlander.c
new file mode 100644
index 0000000..f6b92d2
--- /dev/null
+++ b/drivers/i2c/busses/i2c-highlander.c
@@ -0,0 +1,500 @@
+/*
+ * Renesas Solutions Highlander FPGA I2C/SMBus support.
+ *
+ * Supported devices: R0P7780LC0011RL, R0P7785LC0011RL
+ *
+ * Copyright (C) 2008  Paul Mundt
+ * Copyright (C) 2008  Renesas Solutions Corp.
+ * Copyright (C) 2008  Atom Create Engineering Co., Ltd.
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License version 2. See the file "COPYING" in the main directory
+ * of this archive for more details.
+ */
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+#include <linux/completion.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+
+#define SMCR		0x00
+#define SMCR_START	(1 << 0)
+#define SMCR_IRIC	(1 << 1)
+#define SMCR_BBSY	(1 << 2)
+#define SMCR_ACKE	(1 << 3)
+#define SMCR_RST	(1 << 4)
+#define SMCR_IEIC	(1 << 6)
+
+#define SMSMADR		0x02
+
+#define SMMR		0x04
+#define SMMR_MODE0	(1 << 0)
+#define SMMR_MODE1	(1 << 1)
+#define SMMR_CAP	(1 << 3)
+#define SMMR_TMMD	(1 << 4)
+#define SMMR_SP		(1 << 7)
+
+#define SMSADR		0x06
+#define SMTRDR		0x46
+
+struct highlander_i2c_dev {
+	struct device		*dev;
+	void __iomem		*base;
+	struct i2c_adapter	adapter;
+	struct completion	cmd_complete;
+	int			irq;
+	u8			*buf;
+	size_t			buf_len;
+};
+
+static int iic_force_poll, iic_force_normal, iic_timeout = 1000;
+
+static inline void highlander_i2c_irq_enable(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) | SMCR_IEIC, dev->base + SMCR);
+}
+
+static inline void highlander_i2c_irq_disable(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) & ~SMCR_IEIC, dev->base + SMCR);
+}
+
+static inline void highlander_i2c_start(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) | SMCR_START, dev->base + SMCR);
+}
+
+static inline void highlander_i2c_done(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) | SMCR_IRIC, dev->base + SMCR);
+}
+
+static inline void highlander_i2c_setup(struct highlander_i2c_dev *dev)
+{
+	u16 smmr;
+
+	smmr = ioread16(dev->base + SMMR);
+	smmr |= SMMR_TMMD;
+
+	if (iic_force_normal)
+		smmr &= ~SMMR_SP;
+	else
+		smmr |= SMMR_SP;
+
+	iowrite16(smmr, dev->base + SMMR);
+}
+
+static void smbus_write_data(u8 *src, u16 *dst, int len)
+{
+	int i, j;
+
+	if (len = 1)
+		*dst = *src << 8;
+	else {
+		j = 0;
+		for (i = 0; i < len; i += 2) {
+			*(dst + j) = *(src + i) << 8 | *(src + i + 1);
+			j++;
+		}
+	}
+}
+
+static void smbus_read_data(u16 *src, u8 *dst, int len)
+{
+	int i, j;
+
+	if (len = 1)
+		*dst = *src >> 8;
+	else {
+		j = 0;
+		for (i = 0; i < len; i += 2) {
+			*(dst + i) = *(src + j) >> 8;
+			*(dst + i + 1) = *(src + j) & 0x00ff;
+			j++;
+		}
+	}
+}
+
+static void highlander_i2c_command(struct highlander_i2c_dev *dev, u8 command, int len)
+{
+	u16 cmd[32];
+	int i, j;
+
+	j = 0;
+	if (len = 1)
+		cmd[j++] = (command << 8);
+	else
+		for (i = 0; i < len; i += 2)
+			cmd[j++] = (command << 8) | command;
+
+	for (i = 0; i < j; i++) {
+		iowrite16(cmd[i], dev->base + SMSADR + (i * sizeof(u16)));
+		dev_dbg(dev->dev, "command data[%x] 0x%04x\n", i, cmd[i]);
+	}
+}
+
+static int highlander_i2c_wait_for_bbsy(struct highlander_i2c_dev *dev)
+{
+	unsigned long timeout;
+
+	timeout = jiffies + msecs_to_jiffies(iic_timeout);
+	while (ioread16(dev->base + SMCR) & SMCR_BBSY) {
+		if (time_after(jiffies, timeout)) {
+			dev_warn(dev->dev, "timeout waiting for bus ready\n");
+			return -ETIMEDOUT;
+		}
+
+		msleep(1);
+	}
+
+	return 0;
+}
+
+static int highlander_i2c_reset(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) | SMCR_RST, dev->base + SMCR);
+	return highlander_i2c_wait_for_bbsy(dev);
+}
+
+static int highlander_i2c_wait_for_ack(struct highlander_i2c_dev *dev)
+{
+	u16 tmp = ioread16(dev->base + SMCR);
+
+	if ((tmp & (SMCR_IRIC | SMCR_ACKE)) = SMCR_ACKE) {
+		dev_warn(dev->dev, "ack abnormality\n");
+		return highlander_i2c_reset(dev);
+	}
+
+	return 0;
+}
+
+static irqreturn_t highlander_i2c_irq(int irq, void *dev_id)
+{
+	struct highlander_i2c_dev *dev = dev_id;
+
+	highlander_i2c_done(dev);
+	complete(&dev->cmd_complete);
+
+	return IRQ_HANDLED;
+}
+
+static void highlander_i2c_poll(struct highlander_i2c_dev *dev)
+{
+	unsigned long timeout;
+	u16 smcr;
+
+	timeout = jiffies + msecs_to_jiffies(iic_timeout);
+	for (;;) {
+		smcr = ioread16(dev->base + SMCR);
+
+		/*
+		 * Don't bother checking ACKE here, this and the reset
+		 * are handled in highlander_i2c_wait_xfer_done() when
+		 * waiting for the ACK.
+		 */
+
+		if (smcr & SMCR_IRIC)
+			return;
+		if (time_after(jiffies, timeout))
+			break;
+
+		cpu_relax();
+		cond_resched();
+	}
+
+	dev_err(dev->dev, "polling timed out\n");
+}
+
+static inline int highlander_i2c_wait_xfer_done(struct highlander_i2c_dev *dev)
+{
+	if (dev->irq)
+		wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+					  msecs_to_jiffies(iic_timeout));
+	else
+		/* busy looping, the IRQ of champions */
+		highlander_i2c_poll(dev);
+
+	return highlander_i2c_wait_for_ack(dev);
+}
+
+static int highlander_i2c_read(struct highlander_i2c_dev *dev,
+			       u8 *buf, unsigned int len)
+{
+	int i, cnt;
+	u16 data[16];
+
+	if (highlander_i2c_wait_for_bbsy(dev)) {
+		dev_warn(dev->dev, "timed out\n");
+		return -EAGAIN;
+	}
+
+	highlander_i2c_start(dev);
+
+	if (highlander_i2c_wait_xfer_done(dev)) {
+		dev_err(dev->dev, "Arbitration loss\n");
+		return -EIO;
+	}
+
+	cnt = (len + 1) >> 1;
+	for (i = 0; i < cnt; i++) {
+		data[i] = ioread16(dev->base + SMTRDR + (i * sizeof(u16)));
+		dev_dbg(dev->dev, "read data[%x] 0x%04x\n", i, data[i]);
+	}
+
+	smbus_read_data(data, buf, len);
+
+	/*
+	 * The R0P7780LC0011RL FPGA on the SH7780-based Highlanders
+	 * needs a significant delay in the read path. SH7785 Highlanders
+	 * don't have this issue, so restrict it entirely to those.
+	 */
+	if (mach_is_r7780rp() || mach_is_r7780mp())
+		mdelay(1000);
+
+	return 0;
+}
+
+static int highlander_i2c_write(struct highlander_i2c_dev *dev,
+				u8 *buf, unsigned int len)
+{
+	int i, cnt;
+	u16 data[16];
+
+	smbus_write_data(buf, data, len);
+
+	cnt = (len + 1) >> 1;
+	for (i = 0; i < cnt; i++) {
+		iowrite16(data[i], dev->base + SMTRDR + (i * sizeof(u16)));
+		dev_dbg(dev->dev, "write data[%x] 0x%04x\n", i, data[i]);
+	}
+
+	if (highlander_i2c_wait_for_bbsy(dev)) {
+		dev_warn(dev->dev, "timed out\n");
+		return -EAGAIN;
+	}
+
+	highlander_i2c_start(dev);
+
+	return highlander_i2c_wait_xfer_done(dev);
+}
+
+static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+				  unsigned short flags, char read_write,
+				  u8 command, int size,
+				  union i2c_smbus_data *data)
+{
+	struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
+	int read = read_write & I2C_SMBUS_READ;
+	u16 tmp;
+
+	init_completion(&dev->cmd_complete);
+
+	dev_dbg(dev->dev, "addr %04x, command %02x, read_write %d, size %d\n",
+		addr, command, read_write, size);
+
+	/* Defaults */
+	dev->buf = NULL;
+	dev->buf_len = 0;
+
+	/*
+	 * Set up the buffer and transfer size
+	 */
+	switch (size) {
+	case I2C_SMBUS_BYTE_DATA:
+		if (data)
+			dev->buf = &data->byte;
+		dev->buf_len = 1;
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		dev->buf = &data->block[1];
+		dev->buf_len = data->block[0];
+		break;
+	default:
+		dev_err(dev->dev, "bogus command %d\n", size);
+		return -EINVAL;
+	}
+
+	/*
+	 * Encode the mode setting
+	 */
+	tmp = ioread16(dev->base + SMMR);
+	tmp &= ~(SMMR_MODE0 | SMMR_MODE1);
+
+	switch (dev->buf_len) {
+	case 1:
+		/* default */
+		break;
+	case 8:
+		tmp |= SMMR_MODE0;
+		break;
+	case 16:
+		tmp |= SMMR_MODE1;
+		break;
+	case 32:
+		tmp |= (SMMR_MODE0 | SMMR_MODE1);
+		break;
+	default:
+		dev_err(dev->dev, "bogus xfer size %d\n", dev->buf_len);
+		return -EINVAL;
+	}
+
+	iowrite16(tmp, dev->base + SMMR);
+
+	/* Ensure we're in a sane state */
+	highlander_i2c_done(dev);
+
+	/* Set slave address */
+	iowrite16(((addr & 0x7f) << 1) | read, dev->base + SMSMADR);
+
+	highlander_i2c_command(dev, command, dev->buf_len);
+
+	if (read)
+		return highlander_i2c_read(dev, dev->buf, dev->buf_len);
+	else
+		return highlander_i2c_write(dev, dev->buf, dev->buf_len);
+}
+
+static u32 highlander_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm highlander_i2c_algo = {
+	.smbus_xfer	= highlander_i2c_smbus_xfer,
+	.functionality	= highlander_i2c_func,
+};
+
+static int highlander_i2c_probe(struct platform_device *pdev)
+{
+	struct highlander_i2c_dev *dev;
+	struct i2c_adapter *adap;
+	struct resource *res;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (unlikely(!res)) {
+		dev_err(&pdev->dev, "no mem resource\n");
+		return -ENODEV;
+	}
+
+	dev = kzalloc(sizeof(struct highlander_i2c_dev), GFP_KERNEL);
+	if (unlikely(!dev))
+		return -ENOMEM;
+
+	dev->base = ioremap_nocache(res->start, res->end - res ->start + 1);
+	if (unlikely(!dev->base)) {
+		ret = -ENXIO;
+		goto err;
+	}
+
+	dev->dev = &pdev->dev;
+	platform_set_drvdata(pdev, dev);
+
+	dev->irq = platform_get_irq(pdev, 0);
+	if (dev->irq && !iic_force_poll) {
+		ret = request_irq(dev->irq, highlander_i2c_irq, IRQF_DISABLED,
+				  pdev->name, dev);
+		if (unlikely(ret))
+			goto err_unmap;
+
+		highlander_i2c_irq_enable(dev);
+	} else {
+		dev_notice(&pdev->dev, "no IRQ, using polling mode\n");
+		highlander_i2c_irq_disable(dev);
+	}
+
+	highlander_i2c_setup(dev);
+
+	adap = &dev->adapter;
+	i2c_set_adapdata(adap, dev);
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON;
+	strncpy(adap->name, "HL FPGA I2C adapter", sizeof(adap->name));
+	adap->algo = &highlander_i2c_algo;
+	adap->dev.parent = &pdev->dev;
+
+	adap->nr = pdev->id;
+	ret = i2c_add_numbered_adapter(adap);
+	if (unlikely(ret)) {
+		dev_err(&pdev->dev, "failure adding adapter\n");
+		goto err_free_irq;
+	}
+
+	/*
+	 * Reset the adapter
+	 */
+	ret = highlander_i2c_reset(dev);
+	if (unlikely(ret)) {
+		dev_err(&pdev->dev, "controller didn't come up\n");
+		goto err_free_irq;
+	}
+
+	return 0;
+
+err_free_irq:
+	free_irq(dev->irq, dev);
+err_unmap:
+	iounmap(dev->base);
+err:
+	kfree(dev);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return ret;
+}
+
+static int highlander_i2c_remove(struct platform_device *pdev)
+{
+	struct highlander_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dev->adapter);
+
+	if (dev->irq)
+		free_irq(dev->irq, dev);
+
+	iounmap(dev->base);
+	kfree(dev);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver highlander_i2c_driver = {
+	.driver		= {
+		.name	= "i2c-highlander",
+		.owner	= THIS_MODULE,
+	},
+
+	.probe		= highlander_i2c_probe,
+	.remove		= highlander_i2c_remove,
+};
+
+static int __init highlander_i2c_init(void)
+{
+	return platform_driver_register(&highlander_i2c_driver);
+}
+
+static void __exit highlander_i2c_exit(void)
+{
+	platform_driver_unregister(&highlander_i2c_driver);
+}
+
+module_init(highlander_i2c_init);
+module_exit(highlander_i2c_exit);
+
+MODULE_AUTHOR("Paul Mundt");
+MODULE_DESCRIPTION("Renesas Highlander FPGA I2C/SMBus adapter");
+MODULE_LICENSE("GPL v2");
+
+module_param(iic_force_poll, bool, 0);
+module_param(iic_force_normal, bool, 0);
+module_param(iic_timeout, int, 0);
+
+MODULE_PARM_DESC(iic_force_poll, "Force polling mode");
+MODULE_PARM_DESC(iic_force_normal, "Force normal mode (100 kHz), default is fast mode (400 kHz)");
+MODULE_PARM_DESC(iic_timeout, "Force timeout value in msecs (default 1000 ms)");

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Jean Delvare]

On Tue, 25 Mar 2008 15:32:36 +0900, Paul Mundt wrote:
> This adds support for the SMBus adapter found in the various FPGAs on
> the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
> R0P7785LC0011RL FPGAs.
> 
> Functionality is fairly restricted, in that only byte and block data
> transfers are supported. Normal/fast mode and IRQ/polling are also
> supported. Primarily used for various RTCs and thermal sensors.
> 
> Signed-off-by: Paul Mundt <lethal@linux-sh.org>

No volunteer to review this driver? Manuel or Magnus maybe?

Thanks,
-- 
Jean Delvare

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Manuel Lauss]

Hi Jean,

On Wed, Apr 23, 2008 at 01:41:56PM +0200, Jean Delvare wrote:
> On Tue, 25 Mar 2008 15:32:36 +0900, Paul Mundt wrote:
> > This adds support for the SMBus adapter found in the various FPGAs on
> > the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
> > R0P7785LC0011RL FPGAs.
> > 
> > Functionality is fairly restricted, in that only byte and block data
> > transfers are supported. Normal/fast mode and IRQ/polling are also
> > supported. Primarily used for various RTCs and thermal sensors.
> > 
> > Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> 
> No volunteer to review this driver? Manuel or Magnus maybe?

I don't think I'm qualified to review other peoples' code (it looks
fine to me).  The other thing is Renesas demoboards are impossible to
get outside of Japan so I think Paul is the only user of this hardware
who is also interested in linux support. And I'm convinced he knows
what he's doing ;-)

Thanks,
	Manuel Lauss

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Jean Delvare]

On Wed, 23 Apr 2008 20:11:01 +0200, Manuel Lauss wrote:
> Hi Jean,
> 
> On Wed, Apr 23, 2008 at 01:41:56PM +0200, Jean Delvare wrote:
> > On Tue, 25 Mar 2008 15:32:36 +0900, Paul Mundt wrote:
> > > This adds support for the SMBus adapter found in the various FPGAs on
> > > the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
> > > R0P7785LC0011RL FPGAs.
> > > 
> > > Functionality is fairly restricted, in that only byte and block data
> > > transfers are supported. Normal/fast mode and IRQ/polling are also
> > > supported. Primarily used for various RTCs and thermal sensors.
> > > 
> > > Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> > 
> > No volunteer to review this driver? Manuel or Magnus maybe?
> 
> I don't think I'm qualified to review other peoples' code (it looks
> fine to me).  The other thing is Renesas demoboards are impossible to
> get outside of Japan so I think Paul is the only user of this hardware
> who is also interested in linux support. And I'm convinced he knows
> what he's doing ;-)

I guess that there are more users than just Paul, otherwise he wouldn't
bother pushing his driver upstream, would he?

-- 
Jean Delvare

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Paul Mundt]

On Wed, Apr 23, 2008 at 08:31:04PM +0200, Jean Delvare wrote:
> On Wed, 23 Apr 2008 20:11:01 +0200, Manuel Lauss wrote:
> > On Wed, Apr 23, 2008 at 01:41:56PM +0200, Jean Delvare wrote:
> > > On Tue, 25 Mar 2008 15:32:36 +0900, Paul Mundt wrote:
> > > > This adds support for the SMBus adapter found in the various FPGAs on
> > > > the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
> > > > R0P7785LC0011RL FPGAs.
> > > > 
> > > > Functionality is fairly restricted, in that only byte and block data
> > > > transfers are supported. Normal/fast mode and IRQ/polling are also
> > > > supported. Primarily used for various RTCs and thermal sensors.
> > > > 
> > > > Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> > > 
> > > No volunteer to review this driver? Manuel or Magnus maybe?
> > 
> > I don't think I'm qualified to review other peoples' code (it looks
> > fine to me).  The other thing is Renesas demoboards are impossible to
> > get outside of Japan so I think Paul is the only user of this hardware
> > who is also interested in linux support. And I'm convinced he knows
> > what he's doing ;-)
> 
> I guess that there are more users than just Paul, otherwise he wouldn't
> bother pushing his driver upstream, would he?
> 
Even if I were the only user, I would still push it upstream. Though in
this case, there are plenty of other users. This is a fairly common
reference board in Japan, which we've supported in the kernel in various
forms for a few years already. The FPGA SMBus implementation is one of
the outstanding bits of support for the platform.

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Andrew Morton]

> On Fri, 25 Apr 2008 10:30:11 +0900 Paul Mundt <lethal@linux-sh.org> wrote:
> On Wed, Apr 23, 2008 at 08:31:04PM +0200, Jean Delvare wrote:
> > On Wed, 23 Apr 2008 20:11:01 +0200, Manuel Lauss wrote:
> > > On Wed, Apr 23, 2008 at 01:41:56PM +0200, Jean Delvare wrote:
> > > > On Tue, 25 Mar 2008 15:32:36 +0900, Paul Mundt wrote:
> > > > > This adds support for the SMBus adapter found in the various FPGAs on
> > > > > the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
> > > > > R0P7785LC0011RL FPGAs.
> > > > > 
> > > > > Functionality is fairly restricted, in that only byte and block data
> > > > > transfers are supported. Normal/fast mode and IRQ/polling are also
> > > > > supported. Primarily used for various RTCs and thermal sensors.
> > > > > 
> > > > > Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> > > > 
> > > > No volunteer to review this driver? Manuel or Magnus maybe?
> > > 
> > > I don't think I'm qualified to review other peoples' code (it looks
> > > fine to me).

I looked through it when I merged it - believe it or not, I always do
(well, except for some dopey mechanical code transformation patches where
I'll just believe the changelog).  I saw nothing worth commenting on.  As
is always the case when I don't comment ;)

So here's a
Reviewed-by: Andrew Morton <akpm@linux-foundation.org>

Although that is of course of limited use, coming from a person
who isn't terribly sure what an i2c is.

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Paul Mundt]

On Thu, Apr 24, 2008 at 11:12:07PM -0700, Andrew Morton wrote:
> > On Fri, 25 Apr 2008 10:30:11 +0900 Paul Mundt <lethal@linux-sh.org> wrote:
> > On Wed, Apr 23, 2008 at 08:31:04PM +0200, Jean Delvare wrote:
> > > On Wed, 23 Apr 2008 20:11:01 +0200, Manuel Lauss wrote:
> > > > I don't think I'm qualified to review other peoples' code (it looks
> > > > fine to me).
> 
> I looked through it when I merged it - believe it or not, I always do
> (well, except for some dopey mechanical code transformation patches where
> I'll just believe the changelog).  I saw nothing worth commenting on.  As
> is always the case when I don't comment ;)
> 
> So here's a
> Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
> 
> Although that is of course of limited use, coming from a person
> who isn't terribly sure what an i2c is.

This is the root of the issue, none of the people asked to review the
code are i2c people either. This is a pretty sad state for the subsystem
if the subsystem maintainer needs to defer to people with little to no
knowledge of the subsystem to "review" a driver before it can be merged.

While Manuel, Magnus, and I can easily review and ack our patches, none
of this changes the fact that outside of the platform and architecture
specific bits in the driver, there's very little we can generally comment
on. The reason for soliciting feedback from the i2c list in the first
place was to get review and comments on the subsystem-specific bits from
the people who are obviously far more familiar with these things. I
understand that Jean isn't an embedded person and therefore isn't
comfortable reviewing those sorts of drivers, but in these cases it's the
bus-specific stuff where the review really matters, which obviously the
rest of us aren't in the best position to self-review.

If it's not possible to get a subsystem maintainer to review a patch,
what's the point of having a centralized subsystem in the first place?

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Jean Delvare]

Hi Paul,

On Tue, 25 Mar 2008 15:32:36 +0900, Paul Mundt wrote:
> This adds support for the SMBus adapter found in the various FPGAs on
> the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
> R0P7785LC0011RL FPGAs.
> 
> Functionality is fairly restricted, in that only byte and block data
> transfers are supported. Normal/fast mode and IRQ/polling are also
> supported. Primarily used for various RTCs and thermal sensors.
> 
> Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> 

Review:

> ---
> 
>  drivers/i2c/busses/Kconfig          |   12 
>  drivers/i2c/busses/Makefile         |    1 
>  drivers/i2c/busses/i2c-highlander.c |  500 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 513 insertions(+)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 476b0bb..3376d53 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -146,6 +146,18 @@ config I2C_GPIO
>  	  This is a very simple bitbanging I2C driver utilizing the
>  	  arch-neutral GPIO API to control the SCL and SDA lines.
>  
> +config I2C_HIGHLANDER
> +	tristate "Highlander FPGA SMBus interface"
> +	depends on SH_HIGHLANDER
> +	help
> +	  If you say yes to this option, support will be included for
> +	  the SMBus interface located in the FPGA on various Highlander
> +	  boards, particularly the R0P7780LC0011RL and R0P7785LC0011RL
> +	  FPGAs. This is wholly unrelated to the SoC I2C.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-highlander.
> +
>  config I2C_HYDRA
>  	tristate "CHRP Apple Hydra Mac I/O I2C interface"
>  	depends on PCI && PPC_CHRP && EXPERIMENTAL
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index ea7068f..d179bce 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
>  obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
>  obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
>  obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
> +obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
>  obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
>  obj-$(CONFIG_I2C_I801)		+= i2c-i801.o
>  obj-$(CONFIG_I2C_I810)		+= i2c-i810.o
> diff --git a/drivers/i2c/busses/i2c-highlander.c b/drivers/i2c/busses/i2c-highlander.c
> new file mode 100644
> index 0000000..f6b92d2
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-highlander.c
> @@ -0,0 +1,500 @@
> +/*
> + * Renesas Solutions Highlander FPGA I2C/SMBus support.
> + *
> + * Supported devices: R0P7780LC0011RL, R0P7785LC0011RL
> + *
> + * Copyright (C) 2008  Paul Mundt
> + * Copyright (C) 2008  Renesas Solutions Corp.
> + * Copyright (C) 2008  Atom Create Engineering Co., Ltd.
> + *
> + * This file is subject to the terms and conditions of the GNU General
> + * Public License version 2. See the file "COPYING" in the main directory
> + * of this archive for more details.
> + */
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_device.h>
> +#include <linux/completion.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +
> +#define SMCR		0x00
> +#define SMCR_START	(1 << 0)
> +#define SMCR_IRIC	(1 << 1)
> +#define SMCR_BBSY	(1 << 2)
> +#define SMCR_ACKE	(1 << 3)
> +#define SMCR_RST	(1 << 4)
> +#define SMCR_IEIC	(1 << 6)
> +
> +#define SMSMADR		0x02
> +
> +#define SMMR		0x04
> +#define SMMR_MODE0	(1 << 0)
> +#define SMMR_MODE1	(1 << 1)
> +#define SMMR_CAP	(1 << 3)
> +#define SMMR_TMMD	(1 << 4)
> +#define SMMR_SP		(1 << 7)
> +
> +#define SMSADR		0x06
> +#define SMTRDR		0x46
> +
> +struct highlander_i2c_dev {
> +	struct device		*dev;
> +	void __iomem		*base;
> +	struct i2c_adapter	adapter;
> +	struct completion	cmd_complete;
> +	int			irq;
> +	u8			*buf;
> +	size_t			buf_len;

Your driver is a bit inconsistent with these two last struct members.
You only access them in highlander_i2c_smbus_xfer(), the rest of the
time you pass them to functions as extra parameters (while you are
_also_ passing a pointer to the struct highlander_i2c_dev.) Please
choose a strategy and stick to it: either use these struct members to
carry the values around, or drop these members and use local variables
instead. Mixing both is inefficient and confusing.

> +};
> +
> +static int iic_force_poll, iic_force_normal, iic_timeout = 1000;
> +
> +static inline void highlander_i2c_irq_enable(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) | SMCR_IEIC, dev->base + SMCR);
> +}
> +
> +static inline void highlander_i2c_irq_disable(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) & ~SMCR_IEIC, dev->base + SMCR);
> +}
> +
> +static inline void highlander_i2c_start(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) | SMCR_START, dev->base + SMCR);
> +}
> +
> +static inline void highlander_i2c_done(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) | SMCR_IRIC, dev->base + SMCR);
> +}
> +
> +static inline void highlander_i2c_setup(struct highlander_i2c_dev *dev)

There's no point in forcing this function to be inlined, it's definitely
not performance critical.

> +{
> +	u16 smmr;
> +
> +	smmr = ioread16(dev->base + SMMR);
> +	smmr |= SMMR_TMMD;
> +
> +	if (iic_force_normal)
> +		smmr &= ~SMMR_SP;
> +	else
> +		smmr |= SMMR_SP;
> +
> +	iowrite16(smmr, dev->base + SMMR);
> +}
> +

A comment about why the following 2 functions are needed and/or what
they are doing would be nice to have. Also, the order of the parameters
is rather error-prone, most C functions copying things from one buffer
to another have the destination as first parameter, for example memcpy.

These functions seem to only support even values for len (except for
1), this should be mentioned.

> +static void smbus_write_data(u8 *src, u16 *dst, int len)
> +{
> +	int i, j;
> +
> +	if (len = 1)
> +		*dst = *src << 8;
> +	else {
> +		j = 0;
> +		for (i = 0; i < len; i += 2) {
> +			*(dst + j) = *(src + i) << 8 | *(src + i + 1);
> +			j++;
> +		}
> +	}
> +}
> +
> +static void smbus_read_data(u16 *src, u8 *dst, int len)
> +{
> +	int i, j;
> +
> +	if (len = 1)
> +		*dst = *src >> 8;
> +	else {
> +		j = 0;
> +		for (i = 0; i < len; i += 2) {
> +			*(dst + i) = *(src + j) >> 8;
> +			*(dst + i + 1) = *(src + j) & 0x00ff;
> +			j++;
> +		}
> +	}
> +}

If I read the code above correctly, you are merely converting 16-bit
words from cpu-endian to long-endian and back, so using be16_to_cpu and
cpu_to_be16 would perform better. If the Highlander is big-endian, you
should be able to let the compiler optimize out most of the code.

> +
> +static void highlander_i2c_command(struct highlander_i2c_dev *dev, u8 command, int len)
> +{
> +	u16 cmd[32];
> +	int i, j;
> +
> +	j = 0;
> +	if (len = 1)
> +		cmd[j++] = (command << 8);
> +	else
> +		for (i = 0; i < len; i += 2)
> +			cmd[j++] = (command << 8) | command;
> +
> +	for (i = 0; i < j; i++) {
> +		iowrite16(cmd[i], dev->base + SMSADR + (i * sizeof(u16)));
> +		dev_dbg(dev->dev, "command data[%x] 0x%04x\n", i, cmd[i]);
> +	}
> +}
> +
> +static int highlander_i2c_wait_for_bbsy(struct highlander_i2c_dev *dev)
> +{
> +	unsigned long timeout;
> +
> +	timeout = jiffies + msecs_to_jiffies(iic_timeout);
> +	while (ioread16(dev->base + SMCR) & SMCR_BBSY) {
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(dev->dev, "timeout waiting for bus ready\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		msleep(1);
> +	}
> +
> +	return 0;
> +}
> +
> +static int highlander_i2c_reset(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) | SMCR_RST, dev->base + SMCR);
> +	return highlander_i2c_wait_for_bbsy(dev);
> +}
> +
> +static int highlander_i2c_wait_for_ack(struct highlander_i2c_dev *dev)
> +{
> +	u16 tmp = ioread16(dev->base + SMCR);
> +
> +	if ((tmp & (SMCR_IRIC | SMCR_ACKE)) = SMCR_ACKE) {
> +		dev_warn(dev->dev, "ack abnormality\n");
> +		return highlander_i2c_reset(dev);
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t highlander_i2c_irq(int irq, void *dev_id)
> +{
> +	struct highlander_i2c_dev *dev = dev_id;
> +
> +	highlander_i2c_done(dev);
> +	complete(&dev->cmd_complete);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void highlander_i2c_poll(struct highlander_i2c_dev *dev)
> +{
> +	unsigned long timeout;
> +	u16 smcr;
> +
> +	timeout = jiffies + msecs_to_jiffies(iic_timeout);
> +	for (;;) {
> +		smcr = ioread16(dev->base + SMCR);
> +
> +		/*
> +		 * Don't bother checking ACKE here, this and the reset
> +		 * are handled in highlander_i2c_wait_xfer_done() when
> +		 * waiting for the ACK.
> +		 */
> +
> +		if (smcr & SMCR_IRIC)
> +			return;
> +		if (time_after(jiffies, timeout))
> +			break;
> +
> +		cpu_relax();
> +		cond_resched();
> +	}
> +
> +	dev_err(dev->dev, "polling timed out\n");
> +}
> +
> +static inline int highlander_i2c_wait_xfer_done(struct highlander_i2c_dev *dev)
> +{
> +	if (dev->irq)
> +		wait_for_completion_interruptible_timeout(&dev->cmd_complete,
> +					  msecs_to_jiffies(iic_timeout));
> +	else
> +		/* busy looping, the IRQ of champions */
> +		highlander_i2c_poll(dev);
> +
> +	return highlander_i2c_wait_for_ack(dev);
> +}
> +
> +static int highlander_i2c_read(struct highlander_i2c_dev *dev,
> +			       u8 *buf, unsigned int len)
> +{
> +	int i, cnt;
> +	u16 data[16];
> +
> +	if (highlander_i2c_wait_for_bbsy(dev)) {
> +		dev_warn(dev->dev, "timed out\n");

highlander_i2c_wait_for_bbsy already prints a warning message.

> +		return -EAGAIN;
> +	}
> +
> +	highlander_i2c_start(dev);
> +
> +	if (highlander_i2c_wait_xfer_done(dev)) {
> +		dev_err(dev->dev, "Arbitration loss\n");
> +		return -EIO;

I think you should return -EAGAIN here as well... On arbitration loss
the caller will certainly want to retry later, same as if the bus was
busy?

> +	}
> +
> +	cnt = (len + 1) >> 1;
> +	for (i = 0; i < cnt; i++) {
> +		data[i] = ioread16(dev->base + SMTRDR + (i * sizeof(u16)));
> +		dev_dbg(dev->dev, "read data[%x] 0x%04x\n", i, data[i]);
> +	}
> +
> +	smbus_read_data(data, buf, len);
> +
> +	/*
> +	 * The R0P7780LC0011RL FPGA on the SH7780-based Highlanders
> +	 * needs a significant delay in the read path. SH7785 Highlanders
> +	 * don't have this issue, so restrict it entirely to those.
> +	 */
> +	if (mach_is_r7780rp() || mach_is_r7780mp())
> +		mdelay(1000);

A one second busy-wait is nasty. Can't you use msleep here instead?

Having to wait for 1 second after each read makes this driver pretty
unusable on these machines anyway, doesn't it? :(

> +
> +	return 0;
> +}
> +
> +static int highlander_i2c_write(struct highlander_i2c_dev *dev,
> +				u8 *buf, unsigned int len)
> +{
> +	int i, cnt;
> +	u16 data[16];
> +
> +	smbus_write_data(buf, data, len);
> +
> +	cnt = (len + 1) >> 1;
> +	for (i = 0; i < cnt; i++) {
> +		iowrite16(data[i], dev->base + SMTRDR + (i * sizeof(u16)));
> +		dev_dbg(dev->dev, "write data[%x] 0x%04x\n", i, data[i]);
> +	}
> +
> +	if (highlander_i2c_wait_for_bbsy(dev)) {
> +		dev_warn(dev->dev, "timed out\n");

highlander_i2c_wait_for_bbsy already prints a warning message.

> +		return -EAGAIN;
> +	}
> +
> +	highlander_i2c_start(dev);
> +
> +	return highlander_i2c_wait_xfer_done(dev);
> +}
> +
> +static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> +				  unsigned short flags, char read_write,
> +				  u8 command, int size,
> +				  union i2c_smbus_data *data)
> +{
> +	struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int read = read_write & I2C_SMBUS_READ;
> +	u16 tmp;
> +
> +	init_completion(&dev->cmd_complete);
> +
> +	dev_dbg(dev->dev, "addr %04x, command %02x, read_write %d, size %d\n",
> +		addr, command, read_write, size);
> +
> +	/* Defaults */
> +	dev->buf = NULL;
> +	dev->buf_len = 0;

These initializations shouldn't be needed.

> +
> +	/*
> +	 * Set up the buffer and transfer size
> +	 */
> +	switch (size) {
> +	case I2C_SMBUS_BYTE_DATA:
> +		if (data)
> +			dev->buf = &data->byte;

It is not valid for data to be null for I2C_SMBUS_BYTE_DATA, so this
test isn't needed.

> +		dev->buf_len = 1;
> +		break;
> +	case I2C_SMBUS_BLOCK_DATA:
> +	case I2C_SMBUS_I2C_BLOCK_DATA:
> +		dev->buf = &data->block[1];
> +		dev->buf_len = data->block[0];
> +		break;

That's definitely not correct. Given that you don't differentiate
between I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_I2C_BLOCK_DATA anywhere in
the code, I take it that the device implements either I2C block
transactions or SMBus block transactions but not both. Which one is it?

> +	default:
> +		dev_err(dev->dev, "bogus command %d\n", size);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Encode the mode setting
> +	 */
> +	tmp = ioread16(dev->base + SMMR);
> +	tmp &= ~(SMMR_MODE0 | SMMR_MODE1);
> +
> +	switch (dev->buf_len) {
> +	case 1:
> +		/* default */
> +		break;
> +	case 8:
> +		tmp |= SMMR_MODE0;
> +		break;
> +	case 16:
> +		tmp |= SMMR_MODE1;
> +		break;
> +	case 32:
> +		tmp |= (SMMR_MODE0 | SMMR_MODE1);
> +		break;
> +	default:
> +		dev_err(dev->dev, "bogus xfer size %d\n", dev->buf_len);
> +		return -EINVAL;
> +	}

Wait, do you mean that this device only supports block sizes of 8, 16
and 32. Oh my. At least please change "bogus" to "unsupported" in the
message above - if something is bogus here, that's the hardware.

> +
> +	iowrite16(tmp, dev->base + SMMR);
> +
> +	/* Ensure we're in a sane state */
> +	highlander_i2c_done(dev);
> +
> +	/* Set slave address */
> +	iowrite16(((addr & 0x7f) << 1) | read, dev->base + SMSMADR);

Masking is not needed, addr is a 7-bit value to start with.

> +
> +	highlander_i2c_command(dev, command, dev->buf_len);
> +
> +	if (read)
> +		return highlander_i2c_read(dev, dev->buf, dev->buf_len);
> +	else
> +		return highlander_i2c_write(dev, dev->buf, dev->buf_len);
> +}
> +
> +static u32 highlander_i2c_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_BLOCK_DATA;

Doesn't match the cases handled in highlander_i2c_smbus_xfer.

> +}
> +
> +static const struct i2c_algorithm highlander_i2c_algo = {
> +	.smbus_xfer	= highlander_i2c_smbus_xfer,
> +	.functionality	= highlander_i2c_func,
> +};
> +
> +static int highlander_i2c_probe(struct platform_device *pdev)
> +{
> +	struct highlander_i2c_dev *dev;
> +	struct i2c_adapter *adap;
> +	struct resource *res;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (unlikely(!res)) {
> +		dev_err(&pdev->dev, "no mem resource\n");
> +		return -ENODEV;
> +	}
> +
> +	dev = kzalloc(sizeof(struct highlander_i2c_dev), GFP_KERNEL);
> +	if (unlikely(!dev))
> +		return -ENOMEM;
> +
> +	dev->base = ioremap_nocache(res->start, res->end - res ->start + 1);

No spaces around ->.

> +	if (unlikely(!dev->base)) {
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +
> +	dev->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, dev);
> +
> +	dev->irq = platform_get_irq(pdev, 0);
> +	if (dev->irq && !iic_force_poll) {
> +		ret = request_irq(dev->irq, highlander_i2c_irq, IRQF_DISABLED,
> +				  pdev->name, dev);
> +		if (unlikely(ret))
> +			goto err_unmap;
> +
> +		highlander_i2c_irq_enable(dev);
> +	} else {
> +		dev_notice(&pdev->dev, "no IRQ, using polling mode\n");
> +		highlander_i2c_irq_disable(dev);
> +	}
> +
> +	highlander_i2c_setup(dev);
> +
> +	adap = &dev->adapter;
> +	i2c_set_adapdata(adap, dev);
> +	adap->owner = THIS_MODULE;
> +	adap->class = I2C_CLASS_HWMON;
> +	strncpy(adap->name, "HL FPGA I2C adapter", sizeof(adap->name));

strncpy is broken, please use strlcpy instead.

> +	adap->algo = &highlander_i2c_algo;
> +	adap->dev.parent = &pdev->dev;
> +
> +	adap->nr = pdev->id;
> +	ret = i2c_add_numbered_adapter(adap);
> +	if (unlikely(ret)) {
> +		dev_err(&pdev->dev, "failure adding adapter\n");
> +		goto err_free_irq;
> +	}
> +
> +	/*
> +	 * Reset the adapter
> +	 */
> +	ret = highlander_i2c_reset(dev);
> +	if (unlikely(ret)) {
> +		dev_err(&pdev->dev, "controller didn't come up\n");
> +		goto err_free_irq;

The error path is broken here, you return with an i2c adapter
registered but all its resources freed.

In fact, you should reset the adapter before registering it, otherwise
there's a race where someone could use the adapter before it's
operational.

> +	}
> +
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(dev->irq, dev);
> +err_unmap:
> +	iounmap(dev->base);
> +err:
> +	kfree(dev);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return ret;
> +}
> +
> +static int highlander_i2c_remove(struct platform_device *pdev)
> +{
> +	struct highlander_i2c_dev *dev = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&dev->adapter);
> +
> +	if (dev->irq)
> +		free_irq(dev->irq, dev);

Here you test for dev->irq before calling free_irq(), but in the error
path above you didn't?

> +
> +	iounmap(dev->base);
> +	kfree(dev);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver highlander_i2c_driver = {
> +	.driver		= {
> +		.name	= "i2c-highlander",
> +		.owner	= THIS_MODULE,
> +	},
> +
> +	.probe		= highlander_i2c_probe,
> +	.remove		= highlander_i2c_remove,

I guess that the remove function could be made __devexit or even __exit?

> +};
> +
> +static int __init highlander_i2c_init(void)
> +{
> +	return platform_driver_register(&highlander_i2c_driver);
> +}
> +
> +static void __exit highlander_i2c_exit(void)
> +{
> +	platform_driver_unregister(&highlander_i2c_driver);
> +}
> +
> +module_init(highlander_i2c_init);
> +module_exit(highlander_i2c_exit);
> +
> +MODULE_AUTHOR("Paul Mundt");
> +MODULE_DESCRIPTION("Renesas Highlander FPGA I2C/SMBus adapter");
> +MODULE_LICENSE("GPL v2");
> +
> +module_param(iic_force_poll, bool, 0);
> +module_param(iic_force_normal, bool, 0);
> +module_param(iic_timeout, int, 0);
> +
> +MODULE_PARM_DESC(iic_force_poll, "Force polling mode");
> +MODULE_PARM_DESC(iic_force_normal, "Force normal mode (100 kHz), default is fast mode (400 kHz)");
> +MODULE_PARM_DESC(iic_timeout, "Force timeout value in msecs (default 1000 ms)");

This is just setting the timeout value, not "forcing" it.

-- 
Jean Delvare

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Jean Delvare]

On Fri, 25 Apr 2008 15:22:08 +0900, Paul Mundt wrote:
> On Thu, Apr 24, 2008 at 11:12:07PM -0700, Andrew Morton wrote:
> > > On Fri, 25 Apr 2008 10:30:11 +0900 Paul Mundt <lethal@linux-sh.org> wrote:
> > > On Wed, Apr 23, 2008 at 08:31:04PM +0200, Jean Delvare wrote:
> > > > On Wed, 23 Apr 2008 20:11:01 +0200, Manuel Lauss wrote:
> > > > > I don't think I'm qualified to review other peoples' code (it looks
> > > > > fine to me).
> > 
> > I looked through it when I merged it - believe it or not, I always do
> > (well, except for some dopey mechanical code transformation patches where
> > I'll just believe the changelog).  I saw nothing worth commenting on.  As
> > is always the case when I don't comment ;)
> > 
> > So here's a
> > Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
> > 
> > Although that is of course of limited use, coming from a person
> > who isn't terribly sure what an i2c is.
> 
> This is the root of the issue, none of the people asked to review the
> code are i2c people either. This is a pretty sad state for the subsystem
> if the subsystem maintainer needs to defer to people with little to no
> knowledge of the subsystem to "review" a driver before it can be merged.
>
> While Manuel, Magnus, and I can easily review and ack our patches, none
> of this changes the fact that outside of the platform and architecture
> specific bits in the driver, there's very little we can generally comment
> on. The reason for soliciting feedback from the i2c list in the first
> place was to get review and comments on the subsystem-specific bits from
> the people who are obviously far more familiar with these things. I
> understand that Jean isn't an embedded person and therefore isn't
> comfortable reviewing those sorts of drivers, but in these cases it's the
> bus-specific stuff where the review really matters, which obviously the
> rest of us aren't in the best position to self-review.

OK, I just reviewed your driver. I had 20 comments, only 6 of them
required knowledge about the i2c subsystem. The 14 remaining comments
could have been made by about anybody with some experience with Linux
kernel development.

Given the limited time I have to review new i2c drivers, my hope was
some other people could take care of the first review catching most of
the non-i2c-related issues, and then I could just focus on the i2c side
of things. But I guess I'm asking for too much.

> If it's not possible to get a subsystem maintainer to review a patch,
> what's the point of having a centralized subsystem in the first place?

I don't even understand your question, but I doubt it deserves an
answer anyway.

-- 
Jean Delvare

Re: [i2c] [PATCH] i2c: Renesas Highlander FPGA SMBus support.

[Trent Piepho]

On Fri, 25 Apr 2008, Jean Delvare wrote:
> > +static void smbus_write_data(u8 *src, u16 *dst, int len)
> > +{
> > +	int i, j;
> > +
> > +	if (len = 1)
> > +		*dst = *src << 8;
> > +	else {
> > +		j = 0;
> > +		for (i = 0; i < len; i += 2) {
> > +			*(dst + j) = *(src + i) << 8 | *(src + i + 1);
> > +			j++;
> > +		}
> > +	}
> > +}

	for (; len > 1; len -= 2) {
		*dst++ = be16_to_cpup((u16*)src);
		src += 2;
	}
	if (len)
		*dst = *src << 8;

> > +static void smbus_read_data(u16 *src, u8 *dst, int len)
> > +{
> > +	int i, j;
> > +
> > +	if (len = 1)
> > +		*dst = *src >> 8;
> > +	else {
> > +		j = 0;
> > +		for (i = 0; i < len; i += 2) {
> > +			*(dst + i) = *(src + j) >> 8;
> > +			*(dst + i + 1) = *(src + j) & 0x00ff;
> > +			j++;
> > +		}
> > +	}
> > +}
>
> If I read the code above correctly, you are merely converting 16-bit
> words from cpu-endian to long-endian and back, so using be16_to_cpu and
> cpu_to_be16 would perform better. If the Highlander is big-endian, you
> should be able to let the compiler optimize out most of the code.

	for (; len > 1; len -= 2) {
		*(u16*)dst = cpu_to_be16p(src++);
		dst += 2;
	}
	if (len)
		*dst = *src >> 8;

> > +static void highlander_i2c_command(struct highlander_i2c_dev *dev, u8 command, int len)
> > +{
> > +	u16 cmd[32];
> > +	int i, j;
> > +
> > +	j = 0;
> > +	if (len = 1)
> > +		cmd[j++] = (command << 8);
> > +	else
> > +		for (i = 0; i < len; i += 2)
> > +			cmd[j++] = (command << 8) | command;
> > +
> > +	for (i = 0; i < j; i++) {
> > +		iowrite16(cmd[i], dev->base + SMSADR + (i * sizeof(u16)));
> > +		dev_dbg(dev->dev, "command data[%x] 0x%04x\n", i, cmd[i]);
> > +	}
> > +}

Is there a reason to have a 32 element array for cmd?  Each element is the
same.

	unsigned int i;
	u16 cmd = (command << 8) | command;
	for (i = 0; i < len; i += 2) {
		if (len - i = 1)
			cmd = command << 8;
		iowrite16(cmd, dev->base + SMSADR + i);
		dev_dbg(dev->dev, "command data[%x] 0x%04x\n", i/2, cmd);
	}

These versions will work for odd values of len.  If the hardware can't
handle this, except when len = 1, it would probably make more sense to
catch the error sooner and never call them with unsupported values of len.

> > +static int highlander_i2c_wait_for_bbsy(struct highlander_i2c_dev *dev)
> > +{
> > +	unsigned long timeout;
> > +
> > +	timeout = jiffies + msecs_to_jiffies(iic_timeout);
> > +	while (ioread16(dev->base + SMCR) & SMCR_BBSY) {

If there is some delay here (interrupt or kernel preemption for example),
then the timeout could be triggered incorrectly.

> > +		if (time_after(jiffies, timeout)) {
> > +			dev_warn(dev->dev, "timeout waiting for bus ready\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +
> > +		msleep(1);
> > +	}
> > +
> > +	return 0;
> > +}

> > +	/*
> > +	 * The R0P7780LC0011RL FPGA on the SH7780-based Highlanders
> > +	 * needs a significant delay in the read path. SH7785 Highlanders
> > +	 * don't have this issue, so restrict it entirely to those.
> > +	 */
> > +	if (mach_is_r7780rp() || mach_is_r7780mp())
> > +		mdelay(1000);
>
> A one second busy-wait is nasty. Can't you use msleep here instead?
>
> Having to wait for 1 second after each read makes this driver pretty
> unusable on these machines anyway, doesn't it? :(

Does it need to wait 1 sec after each read, or does it really need a 1 sec
delay _between_ reads?  If it's the later, you would be much better off
storing the time of the last read, and delaying one second from this time
before starting a new read.  If you're only trying to poll a sensor chip
every second, you won't need to busy wait at all this way.

[PATCH] i2c: Renesas Highlander FPGA SMBus support, v2.

[Paul Mundt]

This adds support for the SMBus adapter found in the various FPGAs on
the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
R0P7785LC0011RL FPGAs.

Functionality is fairly restricted, in that only byte and block data
transfers are supported. Normal/fast mode and IRQ/polling are also
supported. Primarily used for various RTCs and thermal sensors.

This version incorporates all of the comments from Jean Delvare and
Trent Piepho.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

---

 drivers/i2c/busses/Kconfig          |   12 
 drivers/i2c/busses/Makefile         |    1 
 drivers/i2c/busses/i2c-highlander.c |  495 ++++++++++++++++++++++++++++++++++++
 3 files changed, 508 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 6ee997b..e54b9fe 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -330,6 +330,18 @@ config I2C_GPIO
 	  This is a very simple bitbanging I2C driver utilizing the
 	  arch-neutral GPIO API to control the SCL and SDA lines.
 
+config I2C_HIGHLANDER
+	tristate "Highlander FPGA SMBus interface"
+	depends on SH_HIGHLANDER
+	help
+	  If you say yes to this option, support will be included for
+	  the SMBus interface located in the FPGA on various Highlander
+	  boards, particularly the R0P7780LC0011RL and R0P7785LC0011RL
+	  FPGAs. This is wholly unrelated to the SoC I2C.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-highlander.
+
 config I2C_IBM_IIC
 	tristate "IBM PPC 4xx on-chip I2C interface"
 	depends on 4xx
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 97dbfa2..0c2c4b2 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
+obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
 obj-$(CONFIG_I2C_IBM_IIC)	+= i2c-ibm_iic.o
 obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
 obj-$(CONFIG_I2C_IXP2000)	+= i2c-ixp2000.o
--- /dev/null	2008-06-05 00:27:47.457594308 +0900
+++ b/drivers/i2c/busses/i2c-highlander.c	2008-07-31 19:35:26.000000000 +0900
@@ -0,0 +1,495 @@
+/*
+ * Renesas Solutions Highlander FPGA I2C/SMBus support.
+ *
+ * Supported devices: R0P7780LC0011RL, R0P7785LC0011RL
+ *
+ * Copyright (C) 2008  Paul Mundt
+ * Copyright (C) 2008  Renesas Solutions Corp.
+ * Copyright (C) 2008  Atom Create Engineering Co., Ltd.
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License version 2. See the file "COPYING" in the main directory
+ * of this archive for more details.
+ */
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+#include <linux/completion.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+
+#define SMCR		0x00
+#define SMCR_START	(1 << 0)
+#define SMCR_IRIC	(1 << 1)
+#define SMCR_BBSY	(1 << 2)
+#define SMCR_ACKE	(1 << 3)
+#define SMCR_RST	(1 << 4)
+#define SMCR_IEIC	(1 << 6)
+
+#define SMSMADR		0x02
+
+#define SMMR		0x04
+#define SMMR_MODE0	(1 << 0)
+#define SMMR_MODE1	(1 << 1)
+#define SMMR_CAP	(1 << 3)
+#define SMMR_TMMD	(1 << 4)
+#define SMMR_SP		(1 << 7)
+
+#define SMSADR		0x06
+#define SMTRDR		0x46
+
+struct highlander_i2c_dev {
+	struct device		*dev;
+	void __iomem		*base;
+	struct i2c_adapter	adapter;
+	struct completion	cmd_complete;
+	unsigned long		last_read_time;
+	int			irq;
+	u8			*buf;
+	size_t			buf_len;
+};
+
+static int iic_force_poll, iic_force_normal;
+static int iic_timeout = 1000, iic_read_delay = 0;
+
+static inline void highlander_i2c_irq_enable(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) | SMCR_IEIC, dev->base + SMCR);
+}
+
+static inline void highlander_i2c_irq_disable(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) & ~SMCR_IEIC, dev->base + SMCR);
+}
+
+static inline void highlander_i2c_start(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) | SMCR_START, dev->base + SMCR);
+}
+
+static inline void highlander_i2c_done(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) | SMCR_IRIC, dev->base + SMCR);
+}
+
+static void highlander_i2c_setup(struct highlander_i2c_dev *dev)
+{
+	u16 smmr;
+
+	smmr = ioread16(dev->base + SMMR);
+	smmr |= SMMR_TMMD;
+
+	if (iic_force_normal)
+		smmr &= ~SMMR_SP;
+	else
+		smmr |= SMMR_SP;
+
+	iowrite16(smmr, dev->base + SMMR);
+}
+
+static void smbus_write_data(u8 *src, u16 *dst, int len)
+{
+	for (; len > 1; len -= 2) {
+		*dst++ = be16_to_cpup((u16 *)src);
+		src += 2;
+	}
+
+	if (len)
+		*dst = *src << 8;
+}
+
+static void smbus_read_data(u16 *src, u8 *dst, int len)
+{
+	for (; len > 1; len -= 2) {
+		*(u16 *)dst = cpu_to_be16p(src++);
+		dst += 2;
+	}
+
+	if (len)
+		*dst = *src >> 8;
+}
+
+static void highlander_i2c_command(struct highlander_i2c_dev *dev, u8 command, int len)
+{
+	unsigned int i;
+	u16 cmd = (command << 8) | command;
+
+	for (i = 0; i < len; i += 2) {
+		if (len - i = 1)
+			cmd = command << 8;
+		iowrite16(cmd, dev->base + SMSADR + i);
+		dev_dbg(dev->dev, "command data[%x] 0x%04x\n", i/2, cmd);
+	}
+}
+
+static int highlander_i2c_wait_for_bbsy(struct highlander_i2c_dev *dev)
+{
+	unsigned long timeout;
+
+	timeout = jiffies + msecs_to_jiffies(iic_timeout);
+	while (ioread16(dev->base + SMCR) & SMCR_BBSY) {
+		if (time_after(jiffies, timeout)) {
+			dev_warn(dev->dev, "timeout waiting for bus ready\n");
+			return -ETIMEDOUT;
+		}
+
+		msleep(1);
+	}
+
+	return 0;
+}
+
+static int highlander_i2c_reset(struct highlander_i2c_dev *dev)
+{
+	iowrite16(ioread16(dev->base + SMCR) | SMCR_RST, dev->base + SMCR);
+	return highlander_i2c_wait_for_bbsy(dev);
+}
+
+static int highlander_i2c_wait_for_ack(struct highlander_i2c_dev *dev)
+{
+	u16 tmp = ioread16(dev->base + SMCR);
+
+	if ((tmp & (SMCR_IRIC | SMCR_ACKE)) = SMCR_ACKE) {
+		dev_warn(dev->dev, "ack abnormality\n");
+		return highlander_i2c_reset(dev);
+	}
+
+	return 0;
+}
+
+static irqreturn_t highlander_i2c_irq(int irq, void *dev_id)
+{
+	struct highlander_i2c_dev *dev = dev_id;
+
+	highlander_i2c_done(dev);
+	complete(&dev->cmd_complete);
+
+	return IRQ_HANDLED;
+}
+
+static void highlander_i2c_poll(struct highlander_i2c_dev *dev)
+{
+	unsigned long timeout;
+	u16 smcr;
+
+	timeout = jiffies + msecs_to_jiffies(iic_timeout);
+	for (;;) {
+		smcr = ioread16(dev->base + SMCR);
+
+		/*
+		 * Don't bother checking ACKE here, this and the reset
+		 * are handled in highlander_i2c_wait_xfer_done() when
+		 * waiting for the ACK.
+		 */
+
+		if (smcr & SMCR_IRIC)
+			return;
+		if (time_after(jiffies, timeout))
+			break;
+
+		cpu_relax();
+		cond_resched();
+	}
+
+	dev_err(dev->dev, "polling timed out\n");
+}
+
+static inline int highlander_i2c_wait_xfer_done(struct highlander_i2c_dev *dev)
+{
+	if (dev->irq)
+		wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+					  msecs_to_jiffies(iic_timeout));
+	else
+		/* busy looping, the IRQ of champions */
+		highlander_i2c_poll(dev);
+
+	return highlander_i2c_wait_for_ack(dev);
+}
+
+static int highlander_i2c_read(struct highlander_i2c_dev *dev)
+{
+	int i, cnt;
+	u16 data[16];
+
+	if (highlander_i2c_wait_for_bbsy(dev))
+		return -EAGAIN;
+
+	highlander_i2c_start(dev);
+
+	if (highlander_i2c_wait_xfer_done(dev)) {
+		dev_err(dev->dev, "Arbitration loss\n");
+		return -EAGAIN;
+	}
+
+	/*
+	 * The R0P7780LC0011RL FPGA needs a significant delay between
+	 * data read cycles, otherwise the transciever gets confused and
+	 * garbage is returned when the read is subsequently aborted.
+	 *
+	 * It is not sufficient to wait for BBSY.
+	 *
+	 * While this generally only applies to the older SH7780-based
+	 * Highlanders, the same issue can be observed on SH7785 ones,
+	 * albeit less frequently. SH7780-based Highlanders may need
+	 * this to be as high as 1000 ms.
+	 */
+	if (iic_read_delay && time_before(jiffies, dev->last_read_time +
+				 msecs_to_jiffies(iic_read_delay)))
+		msleep_interruptible(jiffies_to_msecs((dev->last_read_time +
+				msecs_to_jiffies(iic_read_delay)) - jiffies));
+
+	cnt = (dev->buf_len + 1) >> 1;
+	for (i = 0; i < cnt; i++) {
+		data[i] = ioread16(dev->base + SMTRDR + (i * sizeof(u16)));
+		dev_dbg(dev->dev, "read data[%x] 0x%04x\n", i, data[i]);
+	}
+
+	smbus_read_data(data, dev->buf, dev->buf_len);
+
+	dev->last_read_time = jiffies;
+
+	return 0;
+}
+
+static int highlander_i2c_write(struct highlander_i2c_dev *dev)
+{
+	int i, cnt;
+	u16 data[16];
+
+	smbus_write_data(dev->buf, data, dev->buf_len);
+
+	cnt = (dev->buf_len + 1) >> 1;
+	for (i = 0; i < cnt; i++) {
+		iowrite16(data[i], dev->base + SMTRDR + (i * sizeof(u16)));
+		dev_dbg(dev->dev, "write data[%x] 0x%04x\n", i, data[i]);
+	}
+
+	if (highlander_i2c_wait_for_bbsy(dev))
+		return -EAGAIN;
+
+	highlander_i2c_start(dev);
+
+	return highlander_i2c_wait_xfer_done(dev);
+}
+
+static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+				  unsigned short flags, char read_write,
+				  u8 command, int size,
+				  union i2c_smbus_data *data)
+{
+	struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
+	int read = read_write & I2C_SMBUS_READ;
+	u16 tmp;
+
+	init_completion(&dev->cmd_complete);
+
+	dev_dbg(dev->dev, "addr %04x, command %02x, read_write %d, size %d\n",
+		addr, command, read_write, size);
+
+	/*
+	 * Set up the buffer and transfer size
+	 */
+	switch (size) {
+	case I2C_SMBUS_BYTE_DATA:
+		dev->buf = &data->byte;
+		dev->buf_len = 1;
+		break;
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		dev->buf = &data->block[1];
+		dev->buf_len = data->block[0];
+		break;
+	default:
+		dev_err(dev->dev, "unsupported command %d\n", size);
+		return -EINVAL;
+	}
+
+	/*
+	 * Encode the mode setting
+	 */
+	tmp = ioread16(dev->base + SMMR);
+	tmp &= ~(SMMR_MODE0 | SMMR_MODE1);
+
+	switch (dev->buf_len) {
+	case 1:
+		/* default */
+		break;
+	case 8:
+		tmp |= SMMR_MODE0;
+		break;
+	case 16:
+		tmp |= SMMR_MODE1;
+		break;
+	case 32:
+		tmp |= (SMMR_MODE0 | SMMR_MODE1);
+		break;
+	default:
+		dev_err(dev->dev, "unsupported xfer size %d\n", dev->buf_len);
+		return -EINVAL;
+	}
+
+	iowrite16(tmp, dev->base + SMMR);
+
+	/* Ensure we're in a sane state */
+	highlander_i2c_done(dev);
+
+	/* Set slave address */
+	iowrite16((addr << 1) | read, dev->base + SMSMADR);
+
+	highlander_i2c_command(dev, command, dev->buf_len);
+
+	if (read)
+		return highlander_i2c_read(dev);
+	else
+		return highlander_i2c_write(dev);
+}
+
+static u32 highlander_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_I2C_BLOCK;
+}
+
+static const struct i2c_algorithm highlander_i2c_algo = {
+	.smbus_xfer	= highlander_i2c_smbus_xfer,
+	.functionality	= highlander_i2c_func,
+};
+
+static int __devinit highlander_i2c_probe(struct platform_device *pdev)
+{
+	struct highlander_i2c_dev *dev;
+	struct i2c_adapter *adap;
+	struct resource *res;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (unlikely(!res)) {
+		dev_err(&pdev->dev, "no mem resource\n");
+		return -ENODEV;
+	}
+
+	dev = kzalloc(sizeof(struct highlander_i2c_dev), GFP_KERNEL);
+	if (unlikely(!dev))
+		return -ENOMEM;
+
+	dev->base = ioremap_nocache(res->start, res->end - res->start + 1);
+	if (unlikely(!dev->base)) {
+		ret = -ENXIO;
+		goto err;
+	}
+
+	dev->dev = &pdev->dev;
+	platform_set_drvdata(pdev, dev);
+
+	dev->irq = platform_get_irq(pdev, 0);
+	if (iic_force_poll)
+		dev->irq = 0;
+
+	if (dev->irq) {
+		ret = request_irq(dev->irq, highlander_i2c_irq, IRQF_DISABLED,
+				  pdev->name, dev);
+		if (unlikely(ret))
+			goto err_unmap;
+
+		highlander_i2c_irq_enable(dev);
+	} else {
+		dev_notice(&pdev->dev, "no IRQ, using polling mode\n");
+		highlander_i2c_irq_disable(dev);
+	}
+
+	dev->last_read_time = jiffies;	/* initial read jiffies */
+
+	highlander_i2c_setup(dev);
+
+	adap = &dev->adapter;
+	i2c_set_adapdata(adap, dev);
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON;
+	strlcpy(adap->name, "HL FPGA I2C adapter", I2C_NAME_SIZE);
+	adap->algo = &highlander_i2c_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->nr = pdev->id;
+
+	/*
+	 * Reset the adapter
+	 */
+	ret = highlander_i2c_reset(dev);
+	if (unlikely(ret)) {
+		dev_err(&pdev->dev, "controller didn't come up\n");
+		goto err_free_irq;
+	}
+
+	ret = i2c_add_numbered_adapter(adap);
+	if (unlikely(ret)) {
+		dev_err(&pdev->dev, "failure adding adapter\n");
+		goto err_free_irq;
+	}
+
+	return 0;
+
+err_free_irq:
+	if (dev->irq)
+		free_irq(dev->irq, dev);
+err_unmap:
+	iounmap(dev->base);
+err:
+	kfree(dev);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return ret;
+}
+
+static int __devexit highlander_i2c_remove(struct platform_device *pdev)
+{
+	struct highlander_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dev->adapter);
+
+	if (dev->irq)
+		free_irq(dev->irq, dev);
+
+	iounmap(dev->base);
+	kfree(dev);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver highlander_i2c_driver = {
+	.driver		= {
+		.name	= "i2c-highlander",
+		.owner	= THIS_MODULE,
+	},
+
+	.probe		= highlander_i2c_probe,
+	.remove		= __devexit_p(highlander_i2c_remove),
+};
+
+static int __init highlander_i2c_init(void)
+{
+	return platform_driver_register(&highlander_i2c_driver);
+}
+
+static void __exit highlander_i2c_exit(void)
+{
+	platform_driver_unregister(&highlander_i2c_driver);
+}
+
+module_init(highlander_i2c_init);
+module_exit(highlander_i2c_exit);
+
+MODULE_AUTHOR("Paul Mundt");
+MODULE_DESCRIPTION("Renesas Highlander FPGA I2C/SMBus adapter");
+MODULE_LICENSE("GPL v2");
+
+module_param(iic_force_poll, bool, 0);
+module_param(iic_force_normal, bool, 0);
+module_param(iic_timeout, int, 0);
+module_param(iic_read_delay, int, 0);
+
+MODULE_PARM_DESC(iic_force_poll, "Force polling mode");
+MODULE_PARM_DESC(iic_force_normal, "Force normal mode (100 kHz), default is fast mode (400 kHz)");
+MODULE_PARM_DESC(iic_timeout, "Set timeout value in msecs (default 1000 ms)");
+MODULE_PARM_DESC(iic_read_delay, "Delay between data read cycles (default 0 ms)");

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support, v2.

[Andrew Morton]

On Thu, 31 Jul 2008 19:43:54 +0900
Paul Mundt <lethal@linux-sh.org> wrote:

> This adds support for the SMBus adapter found in the various FPGAs on
> the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
> R0P7785LC0011RL FPGAs.
> 
> Functionality is fairly restricted, in that only byte and block data
> transfers are supported. Normal/fast mode and IRQ/polling are also
> supported. Primarily used for various RTCs and thermal sensors.
> 
> This version incorporates all of the comments from Jean Delvare and
> Trent Piepho.
>
> ...
>
> +static int highlander_i2c_read(struct highlander_i2c_dev *dev)
> +{
> +	int i, cnt;
> +	u16 data[16];
> +
> +	if (highlander_i2c_wait_for_bbsy(dev))
> +		return -EAGAIN;
> +
> +	highlander_i2c_start(dev);
> +
> +	if (highlander_i2c_wait_xfer_done(dev)) {
> +		dev_err(dev->dev, "Arbitration loss\n");
> +		return -EAGAIN;
> +	}
> +
> +	/*
> +	 * The R0P7780LC0011RL FPGA needs a significant delay between
> +	 * data read cycles, otherwise the transciever gets confused and
> +	 * garbage is returned when the read is subsequently aborted.
> +	 *
> +	 * It is not sufficient to wait for BBSY.
> +	 *
> +	 * While this generally only applies to the older SH7780-based
> +	 * Highlanders, the same issue can be observed on SH7785 ones,
> +	 * albeit less frequently. SH7780-based Highlanders may need
> +	 * this to be as high as 1000 ms.
> +	 */
> +	if (iic_read_delay && time_before(jiffies, dev->last_read_time +
> +				 msecs_to_jiffies(iic_read_delay)))
> +		msleep_interruptible(jiffies_to_msecs((dev->last_read_time +
> +				msecs_to_jiffies(iic_read_delay)) - jiffies));

If this task has signal_pending(), msleep_interruptible() will
immediately return.

> +	cnt = (dev->buf_len + 1) >> 1;
> +	for (i = 0; i < cnt; i++) {
> +		data[i] = ioread16(dev->base + SMTRDR + (i * sizeof(u16)));
> +		dev_dbg(dev->dev, "read data[%x] 0x%04x\n", i, data[i]);
> +	}
> +
> +	smbus_read_data(data, dev->buf, dev->buf_len);
> +
> +	dev->last_read_time = jiffies;

In which case I assume that the above will screw up.

> +	return 0;
> +}
>
> ...

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support, v2.

[Paul Mundt]

On Tue, Aug 05, 2008 at 02:37:51PM -0700, Andrew Morton wrote:
> On Thu, 31 Jul 2008 19:43:54 +0900
> Paul Mundt <lethal@linux-sh.org> wrote:
> > +	/*
> > +	 * The R0P7780LC0011RL FPGA needs a significant delay between
> > +	 * data read cycles, otherwise the transciever gets confused and
> > +	 * garbage is returned when the read is subsequently aborted.
> > +	 *
> > +	 * It is not sufficient to wait for BBSY.
> > +	 *
> > +	 * While this generally only applies to the older SH7780-based
> > +	 * Highlanders, the same issue can be observed on SH7785 ones,
> > +	 * albeit less frequently. SH7780-based Highlanders may need
> > +	 * this to be as high as 1000 ms.
> > +	 */
> > +	if (iic_read_delay && time_before(jiffies, dev->last_read_time +
> > +				 msecs_to_jiffies(iic_read_delay)))
> > +		msleep_interruptible(jiffies_to_msecs((dev->last_read_time +
> > +				msecs_to_jiffies(iic_read_delay)) - jiffies));
> 
> If this task has signal_pending(), msleep_interruptible() will
> immediately return.
> 
> > +	cnt = (dev->buf_len + 1) >> 1;
> > +	for (i = 0; i < cnt; i++) {
> > +		data[i] = ioread16(dev->base + SMTRDR + (i * sizeof(u16)));
> > +		dev_dbg(dev->dev, "read data[%x] 0x%04x\n", i, data[i]);
> > +	}
> > +
> > +	smbus_read_data(data, dev->buf, dev->buf_len);
> > +
> > +	dev->last_read_time = jiffies;
> 
> In which case I assume that the above will screw up.
> 
Ah, right. Yes, that should be switched to msleep instead. Feel free to
drop the _interruptible part from the patch, or I can send an update if
you prefer.

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support, v2.

[Andrew Morton]

On Wed, 6 Aug 2008 11:42:33 +0900 Paul Mundt <lethal@linux-sh.org> wrote:

> On Tue, Aug 05, 2008 at 02:37:51PM -0700, Andrew Morton wrote:
> > On Thu, 31 Jul 2008 19:43:54 +0900
> > Paul Mundt <lethal@linux-sh.org> wrote:
> > > +	/*
> > > +	 * The R0P7780LC0011RL FPGA needs a significant delay between
> > > +	 * data read cycles, otherwise the transciever gets confused and
> > > +	 * garbage is returned when the read is subsequently aborted.
> > > +	 *
> > > +	 * It is not sufficient to wait for BBSY.
> > > +	 *
> > > +	 * While this generally only applies to the older SH7780-based
> > > +	 * Highlanders, the same issue can be observed on SH7785 ones,
> > > +	 * albeit less frequently. SH7780-based Highlanders may need
> > > +	 * this to be as high as 1000 ms.
> > > +	 */
> > > +	if (iic_read_delay && time_before(jiffies, dev->last_read_time +
> > > +				 msecs_to_jiffies(iic_read_delay)))
> > > +		msleep_interruptible(jiffies_to_msecs((dev->last_read_time +
> > > +				msecs_to_jiffies(iic_read_delay)) - jiffies));
> > 
> > If this task has signal_pending(), msleep_interruptible() will
> > immediately return.
> > 
> > > +	cnt = (dev->buf_len + 1) >> 1;
> > > +	for (i = 0; i < cnt; i++) {
> > > +		data[i] = ioread16(dev->base + SMTRDR + (i * sizeof(u16)));
> > > +		dev_dbg(dev->dev, "read data[%x] 0x%04x\n", i, data[i]);
> > > +	}
> > > +
> > > +	smbus_read_data(data, dev->buf, dev->buf_len);
> > > +
> > > +	dev->last_read_time = jiffies;
> > 
> > In which case I assume that the above will screw up.
> > 
> Ah, right. Yes, that should be switched to msleep instead. Feel free to
> drop the _interruptible part from the patch, or I can send an update if
> you prefer.

I did that.

highlander_i2c_wait_xfer_done() might have the same problem?

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support, v2.

[Paul Mundt]

On Tue, Aug 05, 2008 at 08:41:42PM -0700, Andrew Morton wrote:
> On Wed, 6 Aug 2008 11:42:33 +0900 Paul Mundt <lethal@linux-sh.org> wrote:
> 
> > On Tue, Aug 05, 2008 at 02:37:51PM -0700, Andrew Morton wrote:
> > > On Thu, 31 Jul 2008 19:43:54 +0900
> > > Paul Mundt <lethal@linux-sh.org> wrote:
> > > > +	/*
> > > > +	 * The R0P7780LC0011RL FPGA needs a significant delay between
> > > > +	 * data read cycles, otherwise the transciever gets confused and
> > > > +	 * garbage is returned when the read is subsequently aborted.
> > > > +	 *
> > > > +	 * It is not sufficient to wait for BBSY.
> > > > +	 *
> > > > +	 * While this generally only applies to the older SH7780-based
> > > > +	 * Highlanders, the same issue can be observed on SH7785 ones,
> > > > +	 * albeit less frequently. SH7780-based Highlanders may need
> > > > +	 * this to be as high as 1000 ms.
> > > > +	 */
> > > > +	if (iic_read_delay && time_before(jiffies, dev->last_read_time +
> > > > +				 msecs_to_jiffies(iic_read_delay)))
> > > > +		msleep_interruptible(jiffies_to_msecs((dev->last_read_time +
> > > > +				msecs_to_jiffies(iic_read_delay)) - jiffies));
> > > 
> > > If this task has signal_pending(), msleep_interruptible() will
> > > immediately return.
> > > 
> > > > +	cnt = (dev->buf_len + 1) >> 1;
> > > > +	for (i = 0; i < cnt; i++) {
> > > > +		data[i] = ioread16(dev->base + SMTRDR + (i * sizeof(u16)));
> > > > +		dev_dbg(dev->dev, "read data[%x] 0x%04x\n", i, data[i]);
> > > > +	}
> > > > +
> > > > +	smbus_read_data(data, dev->buf, dev->buf_len);
> > > > +
> > > > +	dev->last_read_time = jiffies;
> > > 
> > > In which case I assume that the above will screw up.
> > > 
> > Ah, right. Yes, that should be switched to msleep instead. Feel free to
> > drop the _interruptible part from the patch, or I can send an update if
> > you prefer.
> 
> I did that.
> 
> highlander_i2c_wait_xfer_done() might have the same problem?

Yes, that should also not be interruptible, we should be using
wait_for_completion_timeout() there instead, or we risk checking for ACK
too early and potentially resetting the device on abnormality. The
-EAGAIN handling should do the correct thing in the interrupted case, but
obviously it's undesirable to reset the controller in such a situation.

Please drop the interruptible part from there as well.

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support, v2.

[Jean Delvare]

Hi Paul,

On Thu, 31 Jul 2008 19:43:54 +0900, Paul Mundt wrote:
> This adds support for the SMBus adapter found in the various FPGAs on
> the Renesas Highlander platforms. Particularly the R0P7780LC0011RL and
> R0P7785LC0011RL FPGAs.
> 
> Functionality is fairly restricted, in that only byte and block data
> transfers are supported. Normal/fast mode and IRQ/polling are also
> supported. Primarily used for various RTCs and thermal sensors.
> 
> This version incorporates all of the comments from Jean Delvare and
> Trent Piepho.
> 
> Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> 
> ---
> 
>  drivers/i2c/busses/Kconfig          |   12 
>  drivers/i2c/busses/Makefile         |    1 
>  drivers/i2c/busses/i2c-highlander.c |  495 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 508 insertions(+)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 6ee997b..e54b9fe 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -330,6 +330,18 @@ config I2C_GPIO
>  	  This is a very simple bitbanging I2C driver utilizing the
>  	  arch-neutral GPIO API to control the SCL and SDA lines.
>  
> +config I2C_HIGHLANDER
> +	tristate "Highlander FPGA SMBus interface"
> +	depends on SH_HIGHLANDER
> +	help
> +	  If you say yes to this option, support will be included for
> +	  the SMBus interface located in the FPGA on various Highlander
> +	  boards, particularly the R0P7780LC0011RL and R0P7785LC0011RL
> +	  FPGAs. This is wholly unrelated to the SoC I2C.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-highlander.
> +
>  config I2C_IBM_IIC
>  	tristate "IBM PPC 4xx on-chip I2C interface"
>  	depends on 4xx
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 97dbfa2..0c2c4b2 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
>  obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
>  obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
>  obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
> +obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
>  obj-$(CONFIG_I2C_IBM_IIC)	+= i2c-ibm_iic.o
>  obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
>  obj-$(CONFIG_I2C_IXP2000)	+= i2c-ixp2000.o
> --- /dev/null	2008-06-05 00:27:47.457594308 +0900
> +++ b/drivers/i2c/busses/i2c-highlander.c	2008-07-31 19:35:26.000000000 +0900
> @@ -0,0 +1,495 @@
> +/*
> + * Renesas Solutions Highlander FPGA I2C/SMBus support.
> + *
> + * Supported devices: R0P7780LC0011RL, R0P7785LC0011RL
> + *
> + * Copyright (C) 2008  Paul Mundt
> + * Copyright (C) 2008  Renesas Solutions Corp.
> + * Copyright (C) 2008  Atom Create Engineering Co., Ltd.
> + *
> + * This file is subject to the terms and conditions of the GNU General
> + * Public License version 2. See the file "COPYING" in the main directory
> + * of this archive for more details.
> + */
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_device.h>
> +#include <linux/completion.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +
> +#define SMCR		0x00
> +#define SMCR_START	(1 << 0)
> +#define SMCR_IRIC	(1 << 1)
> +#define SMCR_BBSY	(1 << 2)
> +#define SMCR_ACKE	(1 << 3)
> +#define SMCR_RST	(1 << 4)
> +#define SMCR_IEIC	(1 << 6)
> +
> +#define SMSMADR		0x02
> +
> +#define SMMR		0x04
> +#define SMMR_MODE0	(1 << 0)
> +#define SMMR_MODE1	(1 << 1)
> +#define SMMR_CAP	(1 << 3)
> +#define SMMR_TMMD	(1 << 4)
> +#define SMMR_SP		(1 << 7)
> +
> +#define SMSADR		0x06
> +#define SMTRDR		0x46
> +
> +struct highlander_i2c_dev {
> +	struct device		*dev;
> +	void __iomem		*base;
> +	struct i2c_adapter	adapter;
> +	struct completion	cmd_complete;
> +	unsigned long		last_read_time;
> +	int			irq;
> +	u8			*buf;
> +	size_t			buf_len;
> +};
> +
> +static int iic_force_poll, iic_force_normal;
> +static int iic_timeout = 1000, iic_read_delay = 0;

There's a comment in the driver that says that this delay is needed for
several devices. So I'm a bit surprised that the default value is 0.
Shouldn't the default be such that the driver works on all devices by
default?

> +
> +static inline void highlander_i2c_irq_enable(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) | SMCR_IEIC, dev->base + SMCR);
> +}
> +
> +static inline void highlander_i2c_irq_disable(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) & ~SMCR_IEIC, dev->base + SMCR);
> +}
> +
> +static inline void highlander_i2c_start(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) | SMCR_START, dev->base + SMCR);
> +}
> +
> +static inline void highlander_i2c_done(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) | SMCR_IRIC, dev->base + SMCR);
> +}
> +
> +static void highlander_i2c_setup(struct highlander_i2c_dev *dev)
> +{
> +	u16 smmr;
> +
> +	smmr = ioread16(dev->base + SMMR);
> +	smmr |= SMMR_TMMD;
> +
> +	if (iic_force_normal)
> +		smmr &= ~SMMR_SP;
> +	else
> +		smmr |= SMMR_SP;
> +
> +	iowrite16(smmr, dev->base + SMMR);
> +}
> +
> +static void smbus_write_data(u8 *src, u16 *dst, int len)
> +{
> +	for (; len > 1; len -= 2) {
> +		*dst++ = be16_to_cpup((u16 *)src);
> +		src += 2;
> +	}
> +
> +	if (len)
> +		*dst = *src << 8;
> +}
> +
> +static void smbus_read_data(u16 *src, u8 *dst, int len)
> +{
> +	for (; len > 1; len -= 2) {
> +		*(u16 *)dst = cpu_to_be16p(src++);
> +		dst += 2;
> +	}
> +
> +	if (len)
> +		*dst = *src >> 8;
> +}
> +
> +static void highlander_i2c_command(struct highlander_i2c_dev *dev, u8 command, int len)
> +{
> +	unsigned int i;
> +	u16 cmd = (command << 8) | command;
> +
> +	for (i = 0; i < len; i += 2) {
> +		if (len - i = 1)
> +			cmd = command << 8;
> +		iowrite16(cmd, dev->base + SMSADR + i);
> +		dev_dbg(dev->dev, "command data[%x] 0x%04x\n", i/2, cmd);
> +	}
> +}
> +
> +static int highlander_i2c_wait_for_bbsy(struct highlander_i2c_dev *dev)
> +{
> +	unsigned long timeout;
> +
> +	timeout = jiffies + msecs_to_jiffies(iic_timeout);
> +	while (ioread16(dev->base + SMCR) & SMCR_BBSY) {
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(dev->dev, "timeout waiting for bus ready\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		msleep(1);
> +	}
> +
> +	return 0;
> +}
> +
> +static int highlander_i2c_reset(struct highlander_i2c_dev *dev)
> +{
> +	iowrite16(ioread16(dev->base + SMCR) | SMCR_RST, dev->base + SMCR);
> +	return highlander_i2c_wait_for_bbsy(dev);
> +}
> +
> +static int highlander_i2c_wait_for_ack(struct highlander_i2c_dev *dev)
> +{
> +	u16 tmp = ioread16(dev->base + SMCR);
> +
> +	if ((tmp & (SMCR_IRIC | SMCR_ACKE)) = SMCR_ACKE) {
> +		dev_warn(dev->dev, "ack abnormality\n");
> +		return highlander_i2c_reset(dev);
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t highlander_i2c_irq(int irq, void *dev_id)
> +{
> +	struct highlander_i2c_dev *dev = dev_id;
> +
> +	highlander_i2c_done(dev);
> +	complete(&dev->cmd_complete);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void highlander_i2c_poll(struct highlander_i2c_dev *dev)
> +{
> +	unsigned long timeout;
> +	u16 smcr;
> +
> +	timeout = jiffies + msecs_to_jiffies(iic_timeout);
> +	for (;;) {
> +		smcr = ioread16(dev->base + SMCR);
> +
> +		/*
> +		 * Don't bother checking ACKE here, this and the reset
> +		 * are handled in highlander_i2c_wait_xfer_done() when
> +		 * waiting for the ACK.
> +		 */
> +
> +		if (smcr & SMCR_IRIC)
> +			return;
> +		if (time_after(jiffies, timeout))
> +			break;
> +
> +		cpu_relax();
> +		cond_resched();
> +	}
> +
> +	dev_err(dev->dev, "polling timed out\n");
> +}
> +
> +static inline int highlander_i2c_wait_xfer_done(struct highlander_i2c_dev *dev)
> +{
> +	if (dev->irq)
> +		wait_for_completion_interruptible_timeout(&dev->cmd_complete,
> +					  msecs_to_jiffies(iic_timeout));
> +	else
> +		/* busy looping, the IRQ of champions */
> +		highlander_i2c_poll(dev);
> +
> +	return highlander_i2c_wait_for_ack(dev);
> +}
> +
> +static int highlander_i2c_read(struct highlander_i2c_dev *dev)
> +{
> +	int i, cnt;
> +	u16 data[16];
> +
> +	if (highlander_i2c_wait_for_bbsy(dev))
> +		return -EAGAIN;
> +
> +	highlander_i2c_start(dev);
> +
> +	if (highlander_i2c_wait_xfer_done(dev)) {
> +		dev_err(dev->dev, "Arbitration loss\n");
> +		return -EAGAIN;
> +	}
> +
> +	/*
> +	 * The R0P7780LC0011RL FPGA needs a significant delay between
> +	 * data read cycles, otherwise the transciever gets confused and
> +	 * garbage is returned when the read is subsequently aborted.
> +	 *
> +	 * It is not sufficient to wait for BBSY.
> +	 *
> +	 * While this generally only applies to the older SH7780-based
> +	 * Highlanders, the same issue can be observed on SH7785 ones,
> +	 * albeit less frequently. SH7780-based Highlanders may need
> +	 * this to be as high as 1000 ms.
> +	 */
> +	if (iic_read_delay && time_before(jiffies, dev->last_read_time +
> +				 msecs_to_jiffies(iic_read_delay)))
> +		msleep_interruptible(jiffies_to_msecs((dev->last_read_time +
> +				msecs_to_jiffies(iic_read_delay)) - jiffies));
> +
> +	cnt = (dev->buf_len + 1) >> 1;
> +	for (i = 0; i < cnt; i++) {
> +		data[i] = ioread16(dev->base + SMTRDR + (i * sizeof(u16)));
> +		dev_dbg(dev->dev, "read data[%x] 0x%04x\n", i, data[i]);
> +	}
> +
> +	smbus_read_data(data, dev->buf, dev->buf_len);
> +
> +	dev->last_read_time = jiffies;
> +
> +	return 0;
> +}
> +
> +static int highlander_i2c_write(struct highlander_i2c_dev *dev)
> +{
> +	int i, cnt;
> +	u16 data[16];
> +
> +	smbus_write_data(dev->buf, data, dev->buf_len);
> +
> +	cnt = (dev->buf_len + 1) >> 1;
> +	for (i = 0; i < cnt; i++) {
> +		iowrite16(data[i], dev->base + SMTRDR + (i * sizeof(u16)));
> +		dev_dbg(dev->dev, "write data[%x] 0x%04x\n", i, data[i]);
> +	}
> +
> +	if (highlander_i2c_wait_for_bbsy(dev))
> +		return -EAGAIN;
> +
> +	highlander_i2c_start(dev);
> +
> +	return highlander_i2c_wait_xfer_done(dev);
> +}
> +
> +static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> +				  unsigned short flags, char read_write,
> +				  u8 command, int size,
> +				  union i2c_smbus_data *data)
> +{
> +	struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int read = read_write & I2C_SMBUS_READ;
> +	u16 tmp;
> +
> +	init_completion(&dev->cmd_complete);
> +
> +	dev_dbg(dev->dev, "addr %04x, command %02x, read_write %d, size %d\n",
> +		addr, command, read_write, size);
> +
> +	/*
> +	 * Set up the buffer and transfer size
> +	 */
> +	switch (size) {
> +	case I2C_SMBUS_BYTE_DATA:
> +		dev->buf = &data->byte;
> +		dev->buf_len = 1;
> +		break;
> +	case I2C_SMBUS_I2C_BLOCK_DATA:
> +		dev->buf = &data->block[1];
> +		dev->buf_len = data->block[0];
> +		break;
> +	default:
> +		dev_err(dev->dev, "unsupported command %d\n", size);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Encode the mode setting
> +	 */
> +	tmp = ioread16(dev->base + SMMR);
> +	tmp &= ~(SMMR_MODE0 | SMMR_MODE1);
> +
> +	switch (dev->buf_len) {
> +	case 1:
> +		/* default */
> +		break;
> +	case 8:
> +		tmp |= SMMR_MODE0;
> +		break;
> +	case 16:
> +		tmp |= SMMR_MODE1;
> +		break;
> +	case 32:
> +		tmp |= (SMMR_MODE0 | SMMR_MODE1);
> +		break;
> +	default:
> +		dev_err(dev->dev, "unsupported xfer size %d\n", dev->buf_len);
> +		return -EINVAL;
> +	}
> +
> +	iowrite16(tmp, dev->base + SMMR);
> +
> +	/* Ensure we're in a sane state */
> +	highlander_i2c_done(dev);
> +
> +	/* Set slave address */
> +	iowrite16((addr << 1) | read, dev->base + SMSMADR);
> +
> +	highlander_i2c_command(dev, command, dev->buf_len);
> +
> +	if (read)
> +		return highlander_i2c_read(dev);
> +	else
> +		return highlander_i2c_write(dev);
> +}
> +
> +static u32 highlander_i2c_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_I2C_BLOCK;
> +}
> +
> +static const struct i2c_algorithm highlander_i2c_algo = {
> +	.smbus_xfer	= highlander_i2c_smbus_xfer,
> +	.functionality	= highlander_i2c_func,
> +};
> +
> +static int __devinit highlander_i2c_probe(struct platform_device *pdev)
> +{
> +	struct highlander_i2c_dev *dev;
> +	struct i2c_adapter *adap;
> +	struct resource *res;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (unlikely(!res)) {
> +		dev_err(&pdev->dev, "no mem resource\n");
> +		return -ENODEV;
> +	}
> +
> +	dev = kzalloc(sizeof(struct highlander_i2c_dev), GFP_KERNEL);
> +	if (unlikely(!dev))
> +		return -ENOMEM;
> +
> +	dev->base = ioremap_nocache(res->start, res->end - res->start + 1);
> +	if (unlikely(!dev->base)) {
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +
> +	dev->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, dev);
> +
> +	dev->irq = platform_get_irq(pdev, 0);
> +	if (iic_force_poll)
> +		dev->irq = 0;
> +
> +	if (dev->irq) {
> +		ret = request_irq(dev->irq, highlander_i2c_irq, IRQF_DISABLED,
> +				  pdev->name, dev);
> +		if (unlikely(ret))
> +			goto err_unmap;
> +
> +		highlander_i2c_irq_enable(dev);
> +	} else {
> +		dev_notice(&pdev->dev, "no IRQ, using polling mode\n");
> +		highlander_i2c_irq_disable(dev);
> +	}
> +
> +	dev->last_read_time = jiffies;	/* initial read jiffies */
> +
> +	highlander_i2c_setup(dev);
> +
> +	adap = &dev->adapter;
> +	i2c_set_adapdata(adap, dev);
> +	adap->owner = THIS_MODULE;
> +	adap->class = I2C_CLASS_HWMON;
> +	strlcpy(adap->name, "HL FPGA I2C adapter", I2C_NAME_SIZE);

I2C_NAME_SIZE isn't the size of i2c_adapter.name. Use
sizeof(adap->name) instead (as your original code was doing.)

> +	adap->algo = &highlander_i2c_algo;
> +	adap->dev.parent = &pdev->dev;
> +	adap->nr = pdev->id;
> +
> +	/*
> +	 * Reset the adapter
> +	 */
> +	ret = highlander_i2c_reset(dev);
> +	if (unlikely(ret)) {
> +		dev_err(&pdev->dev, "controller didn't come up\n");
> +		goto err_free_irq;
> +	}
> +
> +	ret = i2c_add_numbered_adapter(adap);
> +	if (unlikely(ret)) {
> +		dev_err(&pdev->dev, "failure adding adapter\n");
> +		goto err_free_irq;
> +	}
> +
> +	return 0;
> +
> +err_free_irq:
> +	if (dev->irq)
> +		free_irq(dev->irq, dev);
> +err_unmap:
> +	iounmap(dev->base);
> +err:
> +	kfree(dev);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return ret;
> +}
> +
> +static int __devexit highlander_i2c_remove(struct platform_device *pdev)
> +{
> +	struct highlander_i2c_dev *dev = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&dev->adapter);
> +
> +	if (dev->irq)
> +		free_irq(dev->irq, dev);
> +
> +	iounmap(dev->base);
> +	kfree(dev);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver highlander_i2c_driver = {
> +	.driver		= {
> +		.name	= "i2c-highlander",
> +		.owner	= THIS_MODULE,
> +	},
> +
> +	.probe		= highlander_i2c_probe,
> +	.remove		= __devexit_p(highlander_i2c_remove),
> +};
> +
> +static int __init highlander_i2c_init(void)
> +{
> +	return platform_driver_register(&highlander_i2c_driver);
> +}
> +
> +static void __exit highlander_i2c_exit(void)
> +{
> +	platform_driver_unregister(&highlander_i2c_driver);
> +}
> +
> +module_init(highlander_i2c_init);
> +module_exit(highlander_i2c_exit);
> +
> +MODULE_AUTHOR("Paul Mundt");
> +MODULE_DESCRIPTION("Renesas Highlander FPGA I2C/SMBus adapter");
> +MODULE_LICENSE("GPL v2");
> +
> +module_param(iic_force_poll, bool, 0);
> +module_param(iic_force_normal, bool, 0);
> +module_param(iic_timeout, int, 0);
> +module_param(iic_read_delay, int, 0);
> +
> +MODULE_PARM_DESC(iic_force_poll, "Force polling mode");
> +MODULE_PARM_DESC(iic_force_normal, "Force normal mode (100 kHz), default is fast mode (400 kHz)");
> +MODULE_PARM_DESC(iic_timeout, "Set timeout value in msecs (default 1000 ms)");
> +MODULE_PARM_DESC(iic_read_delay, "Delay between data read cycles (default 0 ms)");

In general I am curious why all these are module parameters instead of
platform device attributes. Shouldn't the platform code know the
correct values? If so, that would be less error prone than leaving it
on the user to pass the right parameters to the module.

-- 
Jean Delvare

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support, v2.

[Paul Mundt]

On Sun, Aug 10, 2008 at 04:29:31PM +0200, Jean Delvare wrote:
> On Thu, 31 Jul 2008 19:43:54 +0900, Paul Mundt wrote:
> > +static int iic_force_poll, iic_force_normal;
> > +static int iic_timeout = 1000, iic_read_delay = 0;
> 
> There's a comment in the driver that says that this delay is needed for
> several devices. So I'm a bit surprised that the default value is 0.
> Shouldn't the default be such that the driver works on all devices by
> default?
> 
The main issue is that it's highly device specific, not just platform
specific. On most of the references boards we've shipped to customers,
there's no need for a delay, so the default is simply to leave that off,
and to allow people to extend it if they start witnessing problems.

Internally we have many different variations both of the board and the
FPGA, a lot of which have very differing timing characteristics. For the
general case a 0 or close to 0 read delay should be sufficient.
Originally I was going to just leave the delay at 250 or something like
that, but that failed to work for the boards that had problems, and
needlessly penalized the ones that didn't (and we have no way of
detecting from the platform if we're on a troublesome board or not). For
customer references that use a silicon option for the same controller,
there's no need for the delay.

Obviously one thing we could try to do is do a few dummy reads and see if
any of them are aborted to try and auto-calculate a meaningful default
delay, but in general it's pretty much all over the place. Given that, I
think just leaving it configurable and documenting the problem cases
should be fine. I'll be the first person to get an email when there's a
problem anyways :-)

> > +	strlcpy(adap->name, "HL FPGA I2C adapter", I2C_NAME_SIZE);
> 
> I2C_NAME_SIZE isn't the size of i2c_adapter.name. Use
> sizeof(adap->name) instead (as your original code was doing.)
> 
Thanks, I'm not sure why that got changed, I'll switch it back.

> > +MODULE_PARM_DESC(iic_force_poll, "Force polling mode");
> > +MODULE_PARM_DESC(iic_force_normal, "Force normal mode (100 kHz), default is fast mode (400 kHz)");
> > +MODULE_PARM_DESC(iic_timeout, "Set timeout value in msecs (default 1000 ms)");
> > +MODULE_PARM_DESC(iic_read_delay, "Delay between data read cycles (default 0 ms)");
> 
> In general I am curious why all these are module parameters instead of
> platform device attributes. Shouldn't the platform code know the
> correct values? If so, that would be less error prone than leaving it
> on the user to pass the right parameters to the module.
> 
Hopefully that's answered above. Let me know if you have any other
concerns or suggestions.

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support, v2.

[Jean Delvare]

On Sun, 10 Aug 2008 23:50:54 +0900, Paul Mundt wrote:
> On Sun, Aug 10, 2008 at 04:29:31PM +0200, Jean Delvare wrote:
> > On Thu, 31 Jul 2008 19:43:54 +0900, Paul Mundt wrote:
> > > +static int iic_force_poll, iic_force_normal;
> > > +static int iic_timeout = 1000, iic_read_delay = 0;
> > 
> > There's a comment in the driver that says that this delay is needed for
> > several devices. So I'm a bit surprised that the default value is 0.
> > Shouldn't the default be such that the driver works on all devices by
> > default?
> > 
> The main issue is that it's highly device specific, not just platform
> specific. On most of the references boards we've shipped to customers,
> there's no need for a delay, so the default is simply to leave that off,
> and to allow people to extend it if they start witnessing problems.
> 
> Internally we have many different variations both of the board and the
> FPGA, a lot of which have very differing timing characteristics. For the
> general case a 0 or close to 0 read delay should be sufficient.
> Originally I was going to just leave the delay at 250 or something like
> that, but that failed to work for the boards that had problems, and
> needlessly penalized the ones that didn't (and we have no way of
> detecting from the platform if we're on a troublesome board or not). For
> customer references that use a silicon option for the same controller,
> there's no need for the delay.
> 
> Obviously one thing we could try to do is do a few dummy reads and see if
> any of them are aborted to try and auto-calculate a meaningful default
> delay, but in general it's pretty much all over the place. Given that, I
> think just leaving it configurable and documenting the problem cases
> should be fine. I'll be the first person to get an email when there's a
> problem anyways :-)
> 
> > > +	strlcpy(adap->name, "HL FPGA I2C adapter", I2C_NAME_SIZE);
> > 
> > I2C_NAME_SIZE isn't the size of i2c_adapter.name. Use
> > sizeof(adap->name) instead (as your original code was doing.)
> > 
> Thanks, I'm not sure why that got changed, I'll switch it back.
> 
> > > +MODULE_PARM_DESC(iic_force_poll, "Force polling mode");
> > > +MODULE_PARM_DESC(iic_force_normal, "Force normal mode (100 kHz), default is fast mode (400 kHz)");
> > > +MODULE_PARM_DESC(iic_timeout, "Set timeout value in msecs (default 1000 ms)");
> > > +MODULE_PARM_DESC(iic_read_delay, "Delay between data read cycles (default 0 ms)");
> > 
> > In general I am curious why all these are module parameters instead of
> > platform device attributes. Shouldn't the platform code know the
> > correct values? If so, that would be less error prone than leaving it
> > on the user to pass the right parameters to the module.
> > 
> Hopefully that's answered above. Let me know if you have any other
> concerns or suggestions.

OK. I've fixed the strlcpy size issue myself, together with a couple
checkpatch warnings. The resulting patch is there:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-renesas-highlander-fpga-smbus-support.patch
It is queued for 2.6.28.

-- 
Jean Delvare

Re: [PATCH] i2c: Renesas Highlander FPGA SMBus support, v2.

[Paul Mundt]

On Sun, Aug 10, 2008 at 05:13:42PM +0200, Jean Delvare wrote:
> On Sun, 10 Aug 2008 23:50:54 +0900, Paul Mundt wrote:
> > Hopefully that's answered above. Let me know if you have any other
> > concerns or suggestions.
> 
> OK. I've fixed the strlcpy size issue myself, together with a couple
> checkpatch warnings. The resulting patch is there:
> http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-renesas-highlander-fpga-smbus-support.patch
> It is queued for 2.6.28.
> 
Looks good, thanks Jean.