[PATCH] Support for Netlogic XLR/XLS I2C controller

[PATCH] Support for Netlogic XLR/XLS I2C controller

[Jayachandran C.]

From: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

This patch adds support to the I2C controller on Netlogic XLR/XLS
multi-core MIPS SoCs.

The patch has been sent out a many times already, and all the comments
we have seen are already taken care of.

It is a polling driver, but that is not an issue in our current usage
(and we have many CPUs on-chip).

Ganesan Ramalingam (1):
  i2c: Support for Netlogic XLR/XLS I2C controller.

 drivers/i2c/busses/Kconfig   |   14 ++
 drivers/i2c/busses/Makefile  |    1 +
 drivers/i2c/busses/i2c-xlr.c |  307 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 322 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-xlr.c

-- 
1.7.5.4

[PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Jayachandran C.]

From: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

Add support for the intergrated I2C controller on Netlogic
XLR/XLS MIPS SoC.

The changes are to add a new file i2c/buses/i2c-xlr.c, containing the
i2c bus implementation, and to update i2c/buses/{Kconfig,Makefile} to
add the CONFIG_I2C_XLR option.

Signed-off-by: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
---
 drivers/i2c/busses/Kconfig   |   14 ++
 drivers/i2c/busses/Makefile  |    1 +
 drivers/i2c/busses/i2c-xlr.c |  307 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 322 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-xlr.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index cbe7a2f..7052628 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -308,6 +308,20 @@ config I2C_AU1550
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-au1550.
 
+config I2C_XLR
+	tristate "XLR I2C support"
+	depends on CPU_XLR
+	help
+	  This driver enables support for the on-chip I2C interface of
+	  the Netlogic XLR/XLS MIPS processors.
+
+	  Say yes to this option if you have a Netlogic XLR/XLS based
+	  board and you need to access the I2C devices (typically the
+	  RTC, sensors, EEPROM) connected to this interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-au1550.
+
 config I2C_BLACKFIN_TWI
 	tristate "Blackfin TWI I2C support"
 	depends on BLACKFIN
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index fba6da6..4372dee 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
 obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
 obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
+obj-$(CONFIG_I2C_XLR)           += i2c-xlr.o
 obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
 
 # External I2C/SMBus adapter drivers
diff --git a/drivers/i2c/busses/i2c-xlr.c b/drivers/i2c/busses/i2c-xlr.c
new file mode 100644
index 0000000..6da0fa9
--- /dev/null
+++ b/drivers/i2c/busses/i2c-xlr.c
@@ -0,0 +1,307 @@
+/*
+ * Copyright 2011, Netlogic Microsystems Inc.
+ * Copyright 2004, Matt Porter <mporter-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+/* XLR I2C REGISTERS */
+#define XLR_I2C_CFG		0x00
+#define XLR_I2C_CLKDIV		0x01
+#define XLR_I2C_DEVADDR		0x02
+#define XLR_I2C_ADDR		0x03
+#define XLR_I2C_DATAOUT		0x04
+#define XLR_I2C_DATAIN		0x05
+#define XLR_I2C_STATUS		0x06
+#define XLR_I2C_STARTXFR	0x07
+#define XLR_I2C_BYTECNT		0x08
+#define XLR_I2C_HDSTATIM	0x09
+
+/* XLR I2C REGISTERS FLAGS */
+#define XLR_I2C_BUS_BUSY	0x01
+#define XLR_I2C_SDOEMPTY	0x02
+#define XLR_I2C_RXRDY		0x04
+#define XLR_I2C_ACK_ERR		0x08
+#define XLR_I2C_ARB_STARTERR	0x30
+
+/* Register Values */
+#define XLR_I2C_CFG_ADDR	0xF8
+#define XLR_I2C_CFG_NOADDR	0xFA
+#define XLR_I2C_STARTXFR_ND	0x02    /* No Data */
+#define XLR_I2C_STARTXFR_RD	0x01    /* Read */
+#define XLR_I2C_STARTXFR_WR	0x00    /* Write */
+
+#define XLR_I2C_IO_SIZE		0x1000
+
+#define MAX_RETRIES		10000 /* max retries per byte */
+/*
+ * Need un-swapped IO for the SoC I2C registers, use __raw_ IO
+ */
+static inline void xlr_i2c_wreg(u32 __iomem *base, unsigned int reg, u32 val)
+{
+	__raw_writel(val, base + reg);
+}
+
+static inline u32 xlr_i2c_rdreg(u32 __iomem *base, unsigned int reg)
+{
+	return __raw_readl(base + reg);
+}
+
+struct xlr_i2c_private {
+	struct i2c_adapter adap;
+	u32 __iomem *iobase;
+};
+
+static int xlr_i2c_tx(struct xlr_i2c_private *priv,  u16 len,
+		u8 *buf, u16 addr)
+{
+	struct i2c_adapter *adap = &priv->adap;
+	u32 i2c_status;
+	int pos, retries;
+	u8 offset, nb;
+
+	offset = buf[0];
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_ADDR, offset);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_ADDR);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len - 1);
+
+	retries = 0;
+retry:
+	pos = 1;
+	if (len == 1) {
+		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
+				XLR_I2C_STARTXFR_ND);
+	} else {
+		xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, buf[pos]);
+		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
+				XLR_I2C_STARTXFR_WR);
+	}
+
+	while (1) {
+		if (retries++ > MAX_RETRIES) {
+			dev_err(&adap->dev, "I2C transmit timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
+
+		if (i2c_status & XLR_I2C_SDOEMPTY) {
+			pos++;
+			retries = 0;
+			nb = (pos < len) ? buf[pos] : 0;
+			xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, nb);
+		}
+
+		if (i2c_status & XLR_I2C_ARB_STARTERR)
+			goto retry;
+
+		if (i2c_status & XLR_I2C_ACK_ERR)
+			return -EIO;
+
+		if (i2c_status & XLR_I2C_BUS_BUSY)
+			continue;
+
+		if (pos >= len)
+			break;
+	}
+
+	return 0;
+}
+
+static int xlr_i2c_rx(struct xlr_i2c_private *priv, u16 len, u8 *buf, u16 addr)
+{
+	struct i2c_adapter *adap = &priv->adap;
+	u32 i2c_status;
+	int pos, retries;
+
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_NOADDR);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
+
+	retries = 0;
+	pos = 0;
+retry:
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR, XLR_I2C_STARTXFR_RD);
+
+	while (1) {
+		if (retries++ > MAX_RETRIES) {
+			dev_err(&adap->dev, "I2C receive timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
+		if (i2c_status & XLR_I2C_RXRDY) {
+			buf[pos++] = (u8)xlr_i2c_rdreg(priv->iobase,
+					XLR_I2C_DATAIN);
+			retries = 0;
+		}
+
+		if (i2c_status & XLR_I2C_ARB_STARTERR)
+			goto retry;
+
+		if (i2c_status & XLR_I2C_ACK_ERR) {
+			dev_err(&adap->dev, "I2C receive ACK error\n");
+			return -EIO;
+		}
+
+		if ((i2c_status & XLR_I2C_BUS_BUSY) == 0)
+			break;
+	}
+	return 0;
+}
+
+static int xlr_i2c_xfer(struct i2c_adapter *adap,
+		struct i2c_msg *msgs, int num)
+{
+	struct i2c_msg *msg;
+	int i;
+	int ret = 0;
+	struct xlr_i2c_private *priv = i2c_get_adapdata(adap);
+
+	for (i = 0; ret == 0 && i < num; i++) {
+		msg = &msgs[i];
+		if (msg->flags & I2C_M_RD)
+			ret = xlr_i2c_rx(priv, msg->len, &msg->buf[0],
+					msg->addr);
+		else
+			ret = xlr_i2c_tx(priv, msg->len, &msg->buf[0],
+					msg->addr);
+	}
+
+	return (ret != 0) ? ret : num;
+}
+
+static u32 xlr_func(struct i2c_adapter *adap)
+{
+	/* Emulate SMBUS over I2C */
+	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C;
+}
+
+static struct i2c_algorithm xlr_i2c_algo = {
+	.master_xfer	= xlr_i2c_xfer,
+	.functionality	= xlr_func,
+};
+
+static int xlr_i2c_add_bus(struct xlr_i2c_private *priv)
+{
+	priv->adap.owner	= THIS_MODULE;
+	priv->adap.algo_data	= priv;
+	priv->adap.nr		= 1;
+	priv->adap.algo		= &xlr_i2c_algo;
+	priv->adap.class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;
+	snprintf(priv->adap.name, sizeof(priv->adap.name),
+			"SMBus XLR I2C Adapter");
+	i2c_set_adapdata(&priv->adap, priv);
+	/* register new adapter to i2c module... */
+	if (i2c_add_numbered_adapter(&priv->adap))
+		return -1;
+
+	return 0;
+}
+
+static int __devinit xlr_i2c_probe(struct platform_device *pdev)
+{
+	struct xlr_i2c_private  *priv;
+	struct resource *res;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (!res) {
+		ret = -ENXIO;
+		goto err1;
+	}
+
+	if (!request_mem_region(res->start, XLR_I2C_IO_SIZE, pdev->name)) {
+		dev_err(&pdev->dev, "request_mem_region failed\n");
+		ret = -ENOMEM;
+		goto err1;
+	}
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto err2;
+	}
+
+	priv->adap.dev.parent = &pdev->dev;
+	priv->iobase = ioremap(res->start, XLR_I2C_IO_SIZE);
+	if (!priv->iobase) {
+		ret = -ENOMEM;
+		goto err3;
+	}
+
+	platform_set_drvdata(pdev, priv);
+	ret = xlr_i2c_add_bus(priv);
+
+	if (ret < 0) {
+		dev_err(&priv->adap.dev, "Failed to add i2c bus.\n");
+		ret = -ENXIO;
+		goto err4;
+	} else
+		dev_info(&priv->adap.dev, "Added I2C Bus.\n");
+
+	return 0;
+err4:
+	iounmap(priv->iobase);
+	platform_set_drvdata(pdev, NULL);
+err3:
+	kfree(priv);
+err2:
+	release_mem_region(res->start, IORESOURCE_MEM);
+err1:
+	return ret;
+}
+
+static int __devexit xlr_i2c_remove(struct platform_device *pdev)
+{
+	struct xlr_i2c_private *priv = platform_get_drvdata(pdev);
+	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	i2c_del_adapter(&priv->adap);
+	iounmap(priv->iobase);
+	kfree(priv);
+	release_mem_region(res->start, IORESOURCE_MEM);
+	platform_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+static struct platform_driver xlr_i2c_driver = {
+	.probe  = xlr_i2c_probe,
+	.remove = __devexit_p(xlr_i2c_remove),
+	.driver = {
+		.owner  = THIS_MODULE,
+		.name   = "xlr-i2cbus",
+	},
+};
+
+static int __init xlr_i2c_init(void)
+{
+	return platform_driver_register(&xlr_i2c_driver);
+}
+
+static void __exit xlr_i2c_exit(void)
+{
+	platform_driver_unregister(&xlr_i2c_driver);
+}
+
+MODULE_AUTHOR("Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>");
+MODULE_DESCRIPTION("XLR I2C SMBus driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:xlr-i2cbus");
+
+module_init(xlr_i2c_init);
+module_exit(xlr_i2c_exit);
-- 
1.7.5.4

Re: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Ben Dooks]

On Tue, Jan 17, 2012 at 07:18:20PM +0530, Jayachandran C. wrote:
> From: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> 
> Add support for the intergrated I2C controller on Netlogic
> XLR/XLS MIPS SoC.
> 
> The changes are to add a new file i2c/buses/i2c-xlr.c, containing the
> i2c bus implementation, and to update i2c/buses/{Kconfig,Makefile} to
> add the CONFIG_I2C_XLR option.
> 
> Signed-off-by: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig   |   14 ++
>  drivers/i2c/busses/Makefile  |    1 +
>  drivers/i2c/busses/i2c-xlr.c |  307 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 322 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-xlr.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index cbe7a2f..7052628 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -308,6 +308,20 @@ config I2C_AU1550
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-au1550.
>  
> +config I2C_XLR
> +	tristate "XLR I2C support"
> +	depends on CPU_XLR
> +	help
> +	  This driver enables support for the on-chip I2C interface of
> +	  the Netlogic XLR/XLS MIPS processors.
> +
> +	  Say yes to this option if you have a Netlogic XLR/XLS based
> +	  board and you need to access the I2C devices (typically the
> +	  RTC, sensors, EEPROM) connected to this interface.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-au1550.
> +
>  config I2C_BLACKFIN_TWI
>  	tristate "Blackfin TWI I2C support"
>  	depends on BLACKFIN
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index fba6da6..4372dee 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
>  obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
>  obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
>  obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
> +obj-$(CONFIG_I2C_XLR)           += i2c-xlr.o
>  obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
>  
>  # External I2C/SMBus adapter drivers
> diff --git a/drivers/i2c/busses/i2c-xlr.c b/drivers/i2c/busses/i2c-xlr.c
> new file mode 100644
> index 0000000..6da0fa9
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-xlr.c
> @@ -0,0 +1,307 @@
> +/*
> + * Copyright 2011, Netlogic Microsystems Inc.
> + * Copyright 2004, Matt Porter <mporter-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/ioport.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +
> +/* XLR I2C REGISTERS */
> +#define XLR_I2C_CFG		0x00
> +#define XLR_I2C_CLKDIV		0x01
> +#define XLR_I2C_DEVADDR		0x02
> +#define XLR_I2C_ADDR		0x03
> +#define XLR_I2C_DATAOUT		0x04
> +#define XLR_I2C_DATAIN		0x05
> +#define XLR_I2C_STATUS		0x06
> +#define XLR_I2C_STARTXFR	0x07
> +#define XLR_I2C_BYTECNT		0x08
> +#define XLR_I2C_HDSTATIM	0x09
> +
> +/* XLR I2C REGISTERS FLAGS */
> +#define XLR_I2C_BUS_BUSY	0x01
> +#define XLR_I2C_SDOEMPTY	0x02
> +#define XLR_I2C_RXRDY		0x04
> +#define XLR_I2C_ACK_ERR		0x08
> +#define XLR_I2C_ARB_STARTERR	0x30
> +
> +/* Register Values */
> +#define XLR_I2C_CFG_ADDR	0xF8
> +#define XLR_I2C_CFG_NOADDR	0xFA
> +#define XLR_I2C_STARTXFR_ND	0x02    /* No Data */
> +#define XLR_I2C_STARTXFR_RD	0x01    /* Read */
> +#define XLR_I2C_STARTXFR_WR	0x00    /* Write */
> +
> +#define XLR_I2C_IO_SIZE		0x1000
> +
> +#define MAX_RETRIES		10000 /* max retries per byte */
> +/*
> + * Need un-swapped IO for the SoC I2C registers, use __raw_ IO
> + */
> +static inline void xlr_i2c_wreg(u32 __iomem *base, unsigned int reg, u32 val)
> +{
> +	__raw_writel(val, base + reg);
> +}
> +
> +static inline u32 xlr_i2c_rdreg(u32 __iomem *base, unsigned int reg)
> +{
> +	return __raw_readl(base + reg);
> +}

Here, there's the use of __raw accessors with memory returned from
ioremap(). Please use readl/writel here, or provide a _good_ reason
for not using them.

> +struct xlr_i2c_private {
> +	struct i2c_adapter adap;
> +	u32 __iomem *iobase;
> +};
> +
> +static int xlr_i2c_tx(struct xlr_i2c_private *priv,  u16 len,
> +		u8 *buf, u16 addr)
> +{
> +	struct i2c_adapter *adap = &priv->adap;
> +	u32 i2c_status;
> +	int pos, retries;
> +	u8 offset, nb;
> +
> +	offset = buf[0];
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_ADDR, offset);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_ADDR);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len - 1);
> +
> +	retries = 0;
> +retry:
> +	pos = 1;
> +	if (len == 1) {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_ND);
> +	} else {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, buf[pos]);
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_WR);
> +	}
> +
> +	while (1) {
> +		if (retries++ > MAX_RETRIES) {
> +			dev_err(&adap->dev, "I2C transmit timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
> +
> +		if (i2c_status & XLR_I2C_SDOEMPTY) {
> +			pos++;
> +			retries = 0;
> +			nb = (pos < len) ? buf[pos] : 0;
> +			xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, nb);
> +		}
> +
> +		if (i2c_status & XLR_I2C_ARB_STARTERR)
> +			goto retry;
> +
> +		if (i2c_status & XLR_I2C_ACK_ERR)
> +			return -EIO;
> +
> +		if (i2c_status & XLR_I2C_BUS_BUSY)
> +			continue;
> +
> +		if (pos >= len)
> +			break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xlr_i2c_rx(struct xlr_i2c_private *priv, u16 len, u8 *buf, u16 addr)
> +{
> +	struct i2c_adapter *adap = &priv->adap;
> +	u32 i2c_status;
> +	int pos, retries;
> +
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_NOADDR);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
> +
> +	retries = 0;
> +	pos = 0;
> +retry:
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR, XLR_I2C_STARTXFR_RD);
> +
> +	while (1) {
> +		if (retries++ > MAX_RETRIES) {
> +			dev_err(&adap->dev, "I2C receive timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
> +		if (i2c_status & XLR_I2C_RXRDY) {
> +			buf[pos++] = (u8)xlr_i2c_rdreg(priv->iobase,
> +					XLR_I2C_DATAIN);
> +			retries = 0;
> +		}
> +
> +		if (i2c_status & XLR_I2C_ARB_STARTERR)
> +			goto retry;
> +
> +		if (i2c_status & XLR_I2C_ACK_ERR) {
> +			dev_err(&adap->dev, "I2C receive ACK error\n");
> +			return -EIO;
> +		}
> +
> +		if ((i2c_status & XLR_I2C_BUS_BUSY) == 0)
> +			break;
> +	}
> +	return 0;
> +}
> +
> +static int xlr_i2c_xfer(struct i2c_adapter *adap,
> +		struct i2c_msg *msgs, int num)
> +{
> +	struct i2c_msg *msg;
> +	int i;
> +	int ret = 0;
> +	struct xlr_i2c_private *priv = i2c_get_adapdata(adap);
> +
> +	for (i = 0; ret == 0 && i < num; i++) {
> +		msg = &msgs[i];
> +		if (msg->flags & I2C_M_RD)
> +			ret = xlr_i2c_rx(priv, msg->len, &msg->buf[0],
> +					msg->addr);
> +		else
> +			ret = xlr_i2c_tx(priv, msg->len, &msg->buf[0],
> +					msg->addr);
> +	}
> +
> +	return (ret != 0) ? ret : num;
> +}
> +
> +static u32 xlr_func(struct i2c_adapter *adap)
> +{
> +	/* Emulate SMBUS over I2C */
> +	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C;
> +}
> +
> +static struct i2c_algorithm xlr_i2c_algo = {
> +	.master_xfer	= xlr_i2c_xfer,
> +	.functionality	= xlr_func,
> +};
> +
> +static int xlr_i2c_add_bus(struct xlr_i2c_private *priv)
> +{
> +	priv->adap.owner	= THIS_MODULE;
> +	priv->adap.algo_data	= priv;
> +	priv->adap.nr		= 1;
> +	priv->adap.algo		= &xlr_i2c_algo;
> +	priv->adap.class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;
> +	snprintf(priv->adap.name, sizeof(priv->adap.name),
> +			"SMBus XLR I2C Adapter");
> +	i2c_set_adapdata(&priv->adap, priv);
> +	/* register new adapter to i2c module... */
> +	if (i2c_add_numbered_adapter(&priv->adap))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int __devinit xlr_i2c_probe(struct platform_device *pdev)
> +{
> +	struct xlr_i2c_private  *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	if (!res) {
> +		ret = -ENXIO;
> +		goto err1;
> +	}
> +
> +	if (!request_mem_region(res->start, XLR_I2C_IO_SIZE, pdev->name)) {
> +		dev_err(&pdev->dev, "request_mem_region failed\n");
> +		ret = -ENOMEM;
> +		goto err1;
> +	}
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto err2;
> +	}
> +
> +	priv->adap.dev.parent = &pdev->dev;
> +	priv->iobase = ioremap(res->start, XLR_I2C_IO_SIZE);
> +	if (!priv->iobase) {
> +		ret = -ENOMEM;
> +		goto err3;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +	ret = xlr_i2c_add_bus(priv);
> +
> +	if (ret < 0) {
> +		dev_err(&priv->adap.dev, "Failed to add i2c bus.\n");
> +		ret = -ENXIO;

Hmm, not sure if -ENXIO is the best return code form here. I would probably
leave the 'ret' alone, and pass it to the upper layer.

> +		goto err4;
> +	} else
> +		dev_info(&priv->adap.dev, "Added I2C Bus.\n");
> +
> +	return 0;
> +err4:
> +	iounmap(priv->iobase);
> +	platform_set_drvdata(pdev, NULL);
> +err3:
> +	kfree(priv);
> +err2:
> +	release_mem_region(res->start, IORESOURCE_MEM);
> +err1:
> +	return ret;
> +}
> +
> +static int __devexit xlr_i2c_remove(struct platform_device *pdev)
> +{
> +	struct xlr_i2c_private *priv = platform_get_drvdata(pdev);
> +	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	i2c_del_adapter(&priv->adap);
> +	iounmap(priv->iobase);
> +	kfree(priv);
> +	release_mem_region(res->start, IORESOURCE_MEM);
> +	platform_set_drvdata(pdev, NULL);
> +	return 0;
> +}
> +
> +static struct platform_driver xlr_i2c_driver = {
> +	.probe  = xlr_i2c_probe,
> +	.remove = __devexit_p(xlr_i2c_remove),
> +	.driver = {
> +		.owner  = THIS_MODULE,
> +		.name   = "xlr-i2cbus",
> +	},
> +};
> +
> +static int __init xlr_i2c_init(void)
> +{
> +	return platform_driver_register(&xlr_i2c_driver);
> +}
> +
> +static void __exit xlr_i2c_exit(void)
> +{
> +	platform_driver_unregister(&xlr_i2c_driver);
> +}
> +
> +MODULE_AUTHOR("Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>");
> +MODULE_DESCRIPTION("XLR I2C SMBus driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:xlr-i2cbus");
> +
> +module_init(xlr_i2c_init);
> +module_exit(xlr_i2c_exit);
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Jayachandran C]

Ben Dooks [ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org] wrote:
> On Tue, Jan 17, 2012 at 07:18:20PM +0530, Jayachandran C. wrote:
>> From: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
>>
>> Add support for the intergrated I2C controller on Netlogic
>> XLR/XLS MIPS SoC.
>>
>> +/*
>> + * Need un-swapped IO for the SoC I2C registers, use __raw_ IO
>> + */
>> +static inline void xlr_i2c_wreg(u32 __iomem *base, unsigned int reg, u32 val)
>> +{
>> +     __raw_writel(val, base + reg);
>> +}
>> +
>> +static inline u32 xlr_i2c_rdreg(u32 __iomem *base, unsigned int reg)
>> +{
>> +     return __raw_readl(base + reg);
>> +}
> 
> Here, there's the use of __raw accessors with memory returned from
> ioremap(). Please use readl/writel here, or provide a _good_ reason
> for not using them.

We had tried to explain it in the comment above the block.  The I2C registers on 
this SoC are in an MMIO area that is big-endian.  The PCI memory and IO space of
this SoC is  little-endian, and we have kept a byteswapping readl/writel so that the
generic PCI drivers will work.

>> +
>> +     platform_set_drvdata(pdev, priv);
>> +     ret = xlr_i2c_add_bus(priv);
>> +
>> +     if (ret < 0) {
>> +             dev_err(&priv->adap.dev, "Failed to add i2c bus.\n");
>> +             ret = -ENXIO;

> Hmm, not sure if -ENXIO is the best return code form here. I would probably
> leave the 'ret' alone, and pass it to the upper layer.

This can be fixed up.

Thanks for the review, will post an updated version.
JC.

Re: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Wolfram Sang]

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

On Tue, Jan 17, 2012 at 07:18:20PM +0530, Jayachandran C. wrote:
> From: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> 
> Add support for the intergrated I2C controller on Netlogic
> XLR/XLS MIPS SoC.
> 
> The changes are to add a new file i2c/buses/i2c-xlr.c, containing the
> i2c bus implementation, and to update i2c/buses/{Kconfig,Makefile} to
> add the CONFIG_I2C_XLR option.
> 
> Signed-off-by: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig   |   14 ++
>  drivers/i2c/busses/Makefile  |    1 +
>  drivers/i2c/busses/i2c-xlr.c |  307 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 322 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-xlr.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index cbe7a2f..7052628 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -308,6 +308,20 @@ config I2C_AU1550
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-au1550.
>  
> +config I2C_XLR
> +	tristate "XLR I2C support"
> +	depends on CPU_XLR
> +	help
> +	  This driver enables support for the on-chip I2C interface of
> +	  the Netlogic XLR/XLS MIPS processors.
> +
> +	  Say yes to this option if you have a Netlogic XLR/XLS based
> +	  board and you need to access the I2C devices (typically the
> +	  RTC, sensors, EEPROM) connected to this interface.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-au1550.
> +

sorting broken

>  config I2C_BLACKFIN_TWI
>  	tristate "Blackfin TWI I2C support"
>  	depends on BLACKFIN
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index fba6da6..4372dee 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
>  obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
>  obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
>  obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
> +obj-$(CONFIG_I2C_XLR)           += i2c-xlr.o
>  obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
>  
>  # External I2C/SMBus adapter drivers
> diff --git a/drivers/i2c/busses/i2c-xlr.c b/drivers/i2c/busses/i2c-xlr.c
> new file mode 100644
> index 0000000..6da0fa9
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-xlr.c
> @@ -0,0 +1,307 @@
> +/*
> + * Copyright 2011, Netlogic Microsystems Inc.
> + * Copyright 2004, Matt Porter <mporter-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/ioport.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +
> +/* XLR I2C REGISTERS */
> +#define XLR_I2C_CFG		0x00
> +#define XLR_I2C_CLKDIV		0x01
> +#define XLR_I2C_DEVADDR		0x02
> +#define XLR_I2C_ADDR		0x03
> +#define XLR_I2C_DATAOUT		0x04
> +#define XLR_I2C_DATAIN		0x05
> +#define XLR_I2C_STATUS		0x06
> +#define XLR_I2C_STARTXFR	0x07
> +#define XLR_I2C_BYTECNT		0x08
> +#define XLR_I2C_HDSTATIM	0x09
> +
> +/* XLR I2C REGISTERS FLAGS */
> +#define XLR_I2C_BUS_BUSY	0x01
> +#define XLR_I2C_SDOEMPTY	0x02
> +#define XLR_I2C_RXRDY		0x04
> +#define XLR_I2C_ACK_ERR		0x08
> +#define XLR_I2C_ARB_STARTERR	0x30
> +
> +/* Register Values */
> +#define XLR_I2C_CFG_ADDR	0xF8
> +#define XLR_I2C_CFG_NOADDR	0xFA
> +#define XLR_I2C_STARTXFR_ND	0x02    /* No Data */
> +#define XLR_I2C_STARTXFR_RD	0x01    /* Read */
> +#define XLR_I2C_STARTXFR_WR	0x00    /* Write */
> +
> +#define XLR_I2C_IO_SIZE		0x1000
> +
> +#define MAX_RETRIES		10000 /* max retries per byte */
> +/*
> + * Need un-swapped IO for the SoC I2C registers, use __raw_ IO
> + */
> +static inline void xlr_i2c_wreg(u32 __iomem *base, unsigned int reg, u32 val)
> +{
> +	__raw_writel(val, base + reg);
> +}
> +
> +static inline u32 xlr_i2c_rdreg(u32 __iomem *base, unsigned int reg)
> +{
> +	return __raw_readl(base + reg);
> +}
> +
> +struct xlr_i2c_private {
> +	struct i2c_adapter adap;
> +	u32 __iomem *iobase;
> +};
> +
> +static int xlr_i2c_tx(struct xlr_i2c_private *priv,  u16 len,
> +		u8 *buf, u16 addr)
> +{
> +	struct i2c_adapter *adap = &priv->adap;
> +	u32 i2c_status;
> +	int pos, retries;
> +	u8 offset, nb;
> +
> +	offset = buf[0];
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_ADDR, offset);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_ADDR);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len - 1);
> +
> +	retries = 0;
> +retry:
> +	pos = 1;
> +	if (len == 1) {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_ND);
> +	} else {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, buf[pos]);
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_WR);
> +	}
> +
> +	while (1) {
> +		if (retries++ > MAX_RETRIES) {
> +			dev_err(&adap->dev, "I2C transmit timeout\n");
> +			return -ETIMEDOUT;
> +		}

A timeout should be in us or ms, but not in loop counts.

> +
> +		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
> +
> +		if (i2c_status & XLR_I2C_SDOEMPTY) {
> +			pos++;
> +			retries = 0;
> +			nb = (pos < len) ? buf[pos] : 0;
> +			xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, nb);
> +		}
> +
> +		if (i2c_status & XLR_I2C_ARB_STARTERR)
> +			goto retry;
> +
> +		if (i2c_status & XLR_I2C_ACK_ERR)
> +			return -EIO;
> +
> +		if (i2c_status & XLR_I2C_BUS_BUSY)
> +			continue;
> +
> +		if (pos >= len)
> +			break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xlr_i2c_rx(struct xlr_i2c_private *priv, u16 len, u8 *buf, u16 addr)
> +{
> +	struct i2c_adapter *adap = &priv->adap;
> +	u32 i2c_status;
> +	int pos, retries;
> +
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_NOADDR);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
> +
> +	retries = 0;
> +	pos = 0;
> +retry:
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR, XLR_I2C_STARTXFR_RD);
> +
> +	while (1) {
> +		if (retries++ > MAX_RETRIES) {
> +			dev_err(&adap->dev, "I2C receive timeout\n");
> +			return -ETIMEDOUT;
> +		}

ditto.

> +
> +		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
> +		if (i2c_status & XLR_I2C_RXRDY) {
> +			buf[pos++] = (u8)xlr_i2c_rdreg(priv->iobase,
> +					XLR_I2C_DATAIN);
> +			retries = 0;
> +		}
> +
> +		if (i2c_status & XLR_I2C_ARB_STARTERR)
> +			goto retry;
> +
> +		if (i2c_status & XLR_I2C_ACK_ERR) {
> +			dev_err(&adap->dev, "I2C receive ACK error\n");
> +			return -EIO;
> +		}
> +
> +		if ((i2c_status & XLR_I2C_BUS_BUSY) == 0)
> +			break;
> +	}
> +	return 0;
> +}
> +
> +static int xlr_i2c_xfer(struct i2c_adapter *adap,
> +		struct i2c_msg *msgs, int num)
> +{
> +	struct i2c_msg *msg;
> +	int i;
> +	int ret = 0;
> +	struct xlr_i2c_private *priv = i2c_get_adapdata(adap);
> +
> +	for (i = 0; ret == 0 && i < num; i++) {
> +		msg = &msgs[i];
> +		if (msg->flags & I2C_M_RD)
> +			ret = xlr_i2c_rx(priv, msg->len, &msg->buf[0],
> +					msg->addr);
> +		else
> +			ret = xlr_i2c_tx(priv, msg->len, &msg->buf[0],
> +					msg->addr);
> +	}
> +
> +	return (ret != 0) ? ret : num;
> +}
> +
> +static u32 xlr_func(struct i2c_adapter *adap)
> +{
> +	/* Emulate SMBUS over I2C */
> +	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C;
> +}
> +
> +static struct i2c_algorithm xlr_i2c_algo = {
> +	.master_xfer	= xlr_i2c_xfer,
> +	.functionality	= xlr_func,
> +};
> +
> +static int xlr_i2c_add_bus(struct xlr_i2c_private *priv)
> +{
> +	priv->adap.owner	= THIS_MODULE;
> +	priv->adap.algo_data	= priv;
> +	priv->adap.nr		= 1;

Fixed number? And if somebody adds some extra external busses?

> +	priv->adap.algo		= &xlr_i2c_algo;
> +	priv->adap.class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;

Are you sure you need the class?

> +	snprintf(priv->adap.name, sizeof(priv->adap.name),
> +			"SMBus XLR I2C Adapter");

really "SMBus"?

> +	i2c_set_adapdata(&priv->adap, priv);
> +	/* register new adapter to i2c module... */
> +	if (i2c_add_numbered_adapter(&priv->adap))
> +		return -1;

return the retval instead of -1?

> +
> +	return 0;
> +}
> +
> +static int __devinit xlr_i2c_probe(struct platform_device *pdev)
> +{
> +	struct xlr_i2c_private  *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Initialization looks okay, but you could consider using
devm_request_and_ioremap() and devm_kzalloc. Will need a lot less code.

> +
> +	if (!res) {
> +		ret = -ENXIO;
> +		goto err1;
> +	}
> +
> +	if (!request_mem_region(res->start, XLR_I2C_IO_SIZE, pdev->name)) {
> +		dev_err(&pdev->dev, "request_mem_region failed\n");
> +		ret = -ENOMEM;
> +		goto err1;
> +	}
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto err2;
> +	}
> +
> +	priv->adap.dev.parent = &pdev->dev;
> +	priv->iobase = ioremap(res->start, XLR_I2C_IO_SIZE);
> +	if (!priv->iobase) {
> +		ret = -ENOMEM;
> +		goto err3;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +	ret = xlr_i2c_add_bus(priv);
> +
> +	if (ret < 0) {
> +		dev_err(&priv->adap.dev, "Failed to add i2c bus.\n");
> +		ret = -ENXIO;

return ret instead of overwriting it?

> +		goto err4;
> +	} else
> +		dev_info(&priv->adap.dev, "Added I2C Bus.\n");
> +
> +	return 0;
> +err4:
> +	iounmap(priv->iobase);
> +	platform_set_drvdata(pdev, NULL);
> +err3:
> +	kfree(priv);
> +err2:
> +	release_mem_region(res->start, IORESOURCE_MEM);
> +err1:
> +	return ret;
> +}
> +
> +static int __devexit xlr_i2c_remove(struct platform_device *pdev)
> +{
> +	struct xlr_i2c_private *priv = platform_get_drvdata(pdev);
> +	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	i2c_del_adapter(&priv->adap);
> +	iounmap(priv->iobase);
> +	kfree(priv);
> +	release_mem_region(res->start, IORESOURCE_MEM);
> +	platform_set_drvdata(pdev, NULL);
> +	return 0;
> +}
> +
> +static struct platform_driver xlr_i2c_driver = {
> +	.probe  = xlr_i2c_probe,
> +	.remove = __devexit_p(xlr_i2c_remove),
> +	.driver = {
> +		.owner  = THIS_MODULE,
> +		.name   = "xlr-i2cbus",
> +	},
> +};
> +
> +static int __init xlr_i2c_init(void)
> +{
> +	return platform_driver_register(&xlr_i2c_driver);
> +}
> +
> +static void __exit xlr_i2c_exit(void)
> +{
> +	platform_driver_unregister(&xlr_i2c_driver);
> +}

Use the macro module_platform_driver, please.

> +
> +MODULE_AUTHOR("Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>");
> +MODULE_DESCRIPTION("XLR I2C SMBus driver");
> +MODULE_LICENSE("GPL");

"GPL v2" according to the header.

> +MODULE_ALIAS("platform:xlr-i2cbus");
> +
> +module_init(xlr_i2c_init);
> +module_exit(xlr_i2c_exit);
> -- 
> 1.7.5.4
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH UPDATED] Support for Netlogic XLR/XLS I2C controller

[Jayachandran C.]

From: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

This version addresses the review comments by Wolfram and Ben to the
previous version (BTW, thanks Wolfram for the detailed review).

The adap.class is set to I2C_CLASS_HWMON, because we have a temparature
sensor on the I2C bus, and I have added a udelay in both the tx and rx
loops so that we are not completely relying on number of retries.

Also as I noted earlier, it is a polling driver, but that is not an issue
in our current usage (and we have many CPUs on-chip).

Ganesan Ramalingam (1):
  i2c: Support for Netlogic XLR/XLS I2C controller.

 drivers/i2c/busses/Kconfig   |   14 ++
 drivers/i2c/busses/Makefile  |    1 +
 drivers/i2c/busses/i2c-xlr.c |  285 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 300 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-xlr.c

-- 
1.7.5.4

[PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Jayachandran C.]

From: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

Add support for the intergrated I2C controller on Netlogic
XLR/XLS MIPS SoC.

The changes are to add a new file i2c/buses/i2c-xlr.c, containing the
i2c bus implementation, and to update i2c/buses/{Kconfig,Makefile} to
add the CONFIG_I2C_XLR option.

Signed-off-by: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
---
 drivers/i2c/busses/Kconfig   |   14 ++
 drivers/i2c/busses/Makefile  |    1 +
 drivers/i2c/busses/i2c-xlr.c |  285 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 300 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-xlr.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index cbe7a2f..54b0d7d 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -681,6 +681,20 @@ config I2C_XILINX
 	  This driver can also be built as a module.  If so, the module
 	  will be called xilinx_i2c.
 
+config I2C_XLR
+	tristate "XLR I2C support"
+	depends on CPU_XLR
+	help
+	  This driver enables support for the on-chip I2C interface of
+	  the Netlogic XLR/XLS MIPS processors.
+
+	  Say yes to this option if you have a Netlogic XLR/XLS based
+	  board and you need to access the I2C devices (typically the
+	  RTC, sensors, EEPROM) connected to this interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-xlr.
+
 config I2C_EG20T
 	tristate "Intel EG20T PCH / OKI SEMICONDUCTOR IOH(ML7213/ML7223)"
 	depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index fba6da6..4372dee 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
 obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
 obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
+obj-$(CONFIG_I2C_XLR)           += i2c-xlr.o
 obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
 
 # External I2C/SMBus adapter drivers
diff --git a/drivers/i2c/busses/i2c-xlr.c b/drivers/i2c/busses/i2c-xlr.c
new file mode 100644
index 0000000..62307ba
--- /dev/null
+++ b/drivers/i2c/busses/i2c-xlr.c
@@ -0,0 +1,285 @@
+/*
+ * Copyright 2011, Netlogic Microsystems Inc.
+ * Copyright 2004, Matt Porter <mporter-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+/* XLR I2C REGISTERS */
+#define XLR_I2C_CFG		0x00
+#define XLR_I2C_CLKDIV		0x01
+#define XLR_I2C_DEVADDR		0x02
+#define XLR_I2C_ADDR		0x03
+#define XLR_I2C_DATAOUT		0x04
+#define XLR_I2C_DATAIN		0x05
+#define XLR_I2C_STATUS		0x06
+#define XLR_I2C_STARTXFR	0x07
+#define XLR_I2C_BYTECNT		0x08
+#define XLR_I2C_HDSTATIM	0x09
+
+/* XLR I2C REGISTERS FLAGS */
+#define XLR_I2C_BUS_BUSY	0x01
+#define XLR_I2C_SDOEMPTY	0x02
+#define XLR_I2C_RXRDY		0x04
+#define XLR_I2C_ACK_ERR		0x08
+#define XLR_I2C_ARB_STARTERR	0x30
+
+/* Register Values */
+#define XLR_I2C_CFG_ADDR	0xF8
+#define XLR_I2C_CFG_NOADDR	0xFA
+#define XLR_I2C_STARTXFR_ND	0x02    /* No Data */
+#define XLR_I2C_STARTXFR_RD	0x01    /* Read */
+#define XLR_I2C_STARTXFR_WR	0x00    /* Write */
+
+#define MAX_RETRIES		5000 /* max retries per byte */
+
+/*
+ * On XLR/XLS, we need to use __raw_ IO to read the I2C registers
+ * because they are in the big-endian MMIO area on the SoC.
+ *
+ * The readl/writel implementation on XLR/XLS byteswaps, beacause
+ * those are for its little-endian PCI space (see arch/mips/Kconfig).
+ */
+static inline void xlr_i2c_wreg(u32 __iomem *base, unsigned int reg, u32 val)
+{
+	__raw_writel(val, base + reg);
+}
+
+static inline u32 xlr_i2c_rdreg(u32 __iomem *base, unsigned int reg)
+{
+	return __raw_readl(base + reg);
+}
+
+struct xlr_i2c_private {
+	struct i2c_adapter adap;
+	u32 __iomem *iobase;
+};
+
+static int xlr_i2c_tx(struct xlr_i2c_private *priv,  u16 len,
+	u8 *buf, u16 addr)
+{
+	struct i2c_adapter *adap = &priv->adap;
+	u32 i2c_status;
+	int pos, retries;
+	u8 offset, nb;
+
+	offset = buf[0];
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_ADDR, offset);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_ADDR);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len - 1);
+
+	retries = 0;
+retry:
+	pos = 1;
+	if (len == 1) {
+		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
+				XLR_I2C_STARTXFR_ND);
+	} else {
+		xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, buf[pos]);
+		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
+				XLR_I2C_STARTXFR_WR);
+	}
+
+	while (1) {
+		if (retries++ > MAX_RETRIES) {
+			dev_err(&adap->dev, "I2C transmit timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
+
+		if (i2c_status & XLR_I2C_SDOEMPTY) {
+			pos++;
+			retries = 0;
+			nb = (pos < len) ? buf[pos] : 0;
+			xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, nb);
+		}
+
+		if (i2c_status & XLR_I2C_ARB_STARTERR)
+			goto retry;
+
+		if (i2c_status & XLR_I2C_ACK_ERR)
+			return -EIO;
+
+		if (i2c_status & XLR_I2C_BUS_BUSY) {
+			udelay(1);
+			continue;
+		}
+
+		if (pos >= len)
+			break;
+	}
+
+	return 0;
+}
+
+static int xlr_i2c_rx(struct xlr_i2c_private *priv, u16 len, u8 *buf, u16 addr)
+{
+	struct i2c_adapter *adap = &priv->adap;
+	u32 i2c_status;
+	int pos, retries;
+
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_NOADDR);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
+
+	retries = 0;
+	pos = 0;
+retry:
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR, XLR_I2C_STARTXFR_RD);
+
+	while (1) {
+		if (retries++ > MAX_RETRIES) {
+			dev_err(&adap->dev, "I2C receive timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
+		if (i2c_status & XLR_I2C_RXRDY) {
+			buf[pos++] = (u8)xlr_i2c_rdreg(priv->iobase,
+					XLR_I2C_DATAIN);
+			retries = 0;
+		}
+
+		if (i2c_status & XLR_I2C_ARB_STARTERR)
+			goto retry;
+
+		if (i2c_status & XLR_I2C_ACK_ERR) {
+			dev_err(&adap->dev, "I2C receive ACK error\n");
+			return -EIO;
+		}
+
+		if ((i2c_status & XLR_I2C_BUS_BUSY) == 0)
+			break;
+		udelay(1);
+	}
+	return 0;
+}
+
+static int xlr_i2c_xfer(struct i2c_adapter *adap,
+	struct i2c_msg *msgs, int num)
+{
+	struct i2c_msg *msg;
+	int i;
+	int ret = 0;
+	struct xlr_i2c_private *priv = i2c_get_adapdata(adap);
+
+	for (i = 0; ret == 0 && i < num; i++) {
+		msg = &msgs[i];
+		if (msg->flags & I2C_M_RD)
+			ret = xlr_i2c_rx(priv, msg->len, &msg->buf[0],
+					msg->addr);
+		else
+			ret = xlr_i2c_tx(priv, msg->len, &msg->buf[0],
+					msg->addr);
+	}
+
+	return (ret != 0) ? ret : num;
+}
+
+static u32 xlr_func(struct i2c_adapter *adap)
+{
+	/* Emulate SMBUS over I2C */
+	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C;
+}
+
+static struct i2c_algorithm xlr_i2c_algo = {
+	.master_xfer	= xlr_i2c_xfer,
+	.functionality	= xlr_func,
+};
+
+static int __devinit xlr_i2c_probe(struct platform_device *pdev)
+{
+	struct xlr_i2c_private  *priv;
+	struct resource *res;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENXIO;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->iobase = devm_request_and_ioremap(&pdev->dev, res);
+	if (!priv->iobase) {
+		dev_err(&pdev->dev, "devm_request_and_ioremap failed\n");
+		ret = -EBUSY;
+		goto err1;
+	}
+
+	priv->adap.dev.parent = &pdev->dev;
+	priv->adap.owner	= THIS_MODULE;
+	priv->adap.algo_data	= priv;
+	priv->adap.algo		= &xlr_i2c_algo;
+	priv->adap.nr		= pdev->id;
+	priv->adap.class	= I2C_CLASS_HWMON;
+	snprintf(priv->adap.name, sizeof(priv->adap.name), "xlr-i2c");
+
+	platform_set_drvdata(pdev, priv);
+	i2c_set_adapdata(&priv->adap, priv);
+	ret = i2c_add_numbered_adapter(&priv->adap);
+
+	if (ret < 0) {
+		dev_err(&priv->adap.dev, "Failed to add i2c bus.\n");
+		goto err2;
+	}
+
+	dev_info(&priv->adap.dev, "Added I2C Bus.\n");
+	return 0;
+err2:
+	devm_release_mem_region(&pdev->dev, res->start, resource_size(res));
+	devm_iounmap(&pdev->dev, priv->iobase);
+	platform_set_drvdata(pdev, NULL);
+err1:
+	devm_kfree(&pdev->dev, priv);
+	return ret;
+}
+
+static int __devexit xlr_i2c_remove(struct platform_device *pdev)
+{
+	struct xlr_i2c_private *priv;
+	struct resource *res;
+
+	priv = platform_get_drvdata(pdev);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	i2c_del_adapter(&priv->adap);
+	devm_release_mem_region(&pdev->dev, res->start, resource_size(res));
+	devm_iounmap(&pdev->dev, priv->iobase);
+	devm_kfree(&pdev->dev, priv);
+
+	platform_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+static struct platform_driver xlr_i2c_driver = {
+	.probe  = xlr_i2c_probe,
+	.remove = __devexit_p(xlr_i2c_remove),
+	.driver = {
+		.name   = "xlr-i2cbus",
+		.owner  = THIS_MODULE,
+	},
+};
+
+module_platform_driver(xlr_i2c_driver);
+
+MODULE_AUTHOR("Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>");
+MODULE_DESCRIPTION("XLR/XLS SoC I2C Controller driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:xlr-i2cbus");
-- 
1.7.5.4

Re: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Wolfram Sang]

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

> +config I2C_XLR
> +	tristate "XLR I2C support"
> +	depends on CPU_XLR
> +	help
> +	  This driver enables support for the on-chip I2C interface of
> +	  the Netlogic XLR/XLS MIPS processors.
> +
> +	  Say yes to this option if you have a Netlogic XLR/XLS based
> +	  board and you need to access the I2C devices (typically the
> +	  RTC, sensors, EEPROM) connected to this interface.

Do you insist on this paragraph?

> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-xlr.
> +
>  config I2C_EG20T
>  	tristate "Intel EG20T PCH / OKI SEMICONDUCTOR IOH(ML7213/ML7223)"
>  	depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index fba6da6..4372dee 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
>  obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
>  obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
>  obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
> +obj-$(CONFIG_I2C_XLR)           += i2c-xlr.o

Use tabs here.

>  obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
>  
>  # External I2C/SMBus adapter drivers
> diff --git a/drivers/i2c/busses/i2c-xlr.c b/drivers/i2c/busses/i2c-xlr.c
> new file mode 100644
> index 0000000..62307ba
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-xlr.c
> @@ -0,0 +1,285 @@
> +/*
> + * Copyright 2011, Netlogic Microsystems Inc.
> + * Copyright 2004, Matt Porter <mporter-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/ioport.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +
> +/* XLR I2C REGISTERS */
> +#define XLR_I2C_CFG		0x00
> +#define XLR_I2C_CLKDIV		0x01
> +#define XLR_I2C_DEVADDR		0x02
> +#define XLR_I2C_ADDR		0x03
> +#define XLR_I2C_DATAOUT		0x04
> +#define XLR_I2C_DATAIN		0x05
> +#define XLR_I2C_STATUS		0x06
> +#define XLR_I2C_STARTXFR	0x07
> +#define XLR_I2C_BYTECNT		0x08
> +#define XLR_I2C_HDSTATIM	0x09
> +
> +/* XLR I2C REGISTERS FLAGS */
> +#define XLR_I2C_BUS_BUSY	0x01
> +#define XLR_I2C_SDOEMPTY	0x02
> +#define XLR_I2C_RXRDY		0x04
> +#define XLR_I2C_ACK_ERR		0x08
> +#define XLR_I2C_ARB_STARTERR	0x30
> +
> +/* Register Values */
> +#define XLR_I2C_CFG_ADDR	0xF8
> +#define XLR_I2C_CFG_NOADDR	0xFA
> +#define XLR_I2C_STARTXFR_ND	0x02    /* No Data */
> +#define XLR_I2C_STARTXFR_RD	0x01    /* Read */
> +#define XLR_I2C_STARTXFR_WR	0x00    /* Write */
> +
> +#define MAX_RETRIES		5000 /* max retries per byte */
> +
> +/*
> + * On XLR/XLS, we need to use __raw_ IO to read the I2C registers
> + * because they are in the big-endian MMIO area on the SoC.
> + *
> + * The readl/writel implementation on XLR/XLS byteswaps, beacause
> + * those are for its little-endian PCI space (see arch/mips/Kconfig).
> + */

Good, thanks.

> +static inline void xlr_i2c_wreg(u32 __iomem *base, unsigned int reg, u32 val)
> +{
> +	__raw_writel(val, base + reg);
> +}
> +
> +static inline u32 xlr_i2c_rdreg(u32 __iomem *base, unsigned int reg)
> +{
> +	return __raw_readl(base + reg);
> +}
> +
> +struct xlr_i2c_private {
> +	struct i2c_adapter adap;
> +	u32 __iomem *iobase;
> +};
> +
> +static int xlr_i2c_tx(struct xlr_i2c_private *priv,  u16 len,
> +	u8 *buf, u16 addr)
> +{
> +	struct i2c_adapter *adap = &priv->adap;
> +	u32 i2c_status;
> +	int pos, retries;
> +	u8 offset, nb;
> +
> +	offset = buf[0];
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_ADDR, offset);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_ADDR);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len - 1);
> +
> +	retries = 0;
> +retry:
> +	pos = 1;
> +	if (len == 1) {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_ND);
> +	} else {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, buf[pos]);
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_WR);
> +	}
> +
> +	while (1) {
> +		if (retries++ > MAX_RETRIES) {
> +			dev_err(&adap->dev, "I2C transmit timeout\n");
> +			return -ETIMEDOUT;
> +		}

Still no. A device usually fails after trying for a specific period of time,
not after a number of retries. You need to work with jiffies here and
time_after/time_before. Check drivers/misc/eeprom/at24.c for an example.
Or check the second part of my talk from Prague :)
http://free-electrons.com/blog/elce-2011-videos/

> +
> +		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
> +
> +		if (i2c_status & XLR_I2C_SDOEMPTY) {
> +			pos++;
> +			retries = 0;
> +			nb = (pos < len) ? buf[pos] : 0;
> +			xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, nb);
> +		}
> +
> +		if (i2c_status & XLR_I2C_ARB_STARTERR)
> +			goto retry;

This will be an endless loop if the STARTERR condition remains? Also this goto
jumping out of the while loop is not nice. It might help to rearrange the
loops.

> +
> +		if (i2c_status & XLR_I2C_ACK_ERR)
> +			return -EIO;
> +
> +		if (i2c_status & XLR_I2C_BUS_BUSY) {
> +			udelay(1);
> +			continue;
> +		}
> +
> +		if (pos >= len)
> +			break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xlr_i2c_rx(struct xlr_i2c_private *priv, u16 len, u8 *buf, u16 addr)
> +{
> +	struct i2c_adapter *adap = &priv->adap;
> +	u32 i2c_status;
> +	int pos, retries;
> +
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_NOADDR);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
> +
> +	retries = 0;
> +	pos = 0;
> +retry:
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR, XLR_I2C_STARTXFR_RD);
> +
> +	while (1) {
> +		if (retries++ > MAX_RETRIES) {
> +			dev_err(&adap->dev, "I2C receive timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
> +		if (i2c_status & XLR_I2C_RXRDY) {
> +			buf[pos++] = (u8)xlr_i2c_rdreg(priv->iobase,
> +					XLR_I2C_DATAIN);
> +			retries = 0;
> +		}

comments from above also here.

> +
> +		if (i2c_status & XLR_I2C_ARB_STARTERR)
> +			goto retry;
> +
> +		if (i2c_status & XLR_I2C_ACK_ERR) {
> +			dev_err(&adap->dev, "I2C receive ACK error\n");
> +			return -EIO;
> +		}
> +
> +		if ((i2c_status & XLR_I2C_BUS_BUSY) == 0)
> +			break;
> +		udelay(1);
> +	}
> +	return 0;
> +}
> +
> +static int xlr_i2c_xfer(struct i2c_adapter *adap,
> +	struct i2c_msg *msgs, int num)
> +{
> +	struct i2c_msg *msg;
> +	int i;
> +	int ret = 0;
> +	struct xlr_i2c_private *priv = i2c_get_adapdata(adap);
> +
> +	for (i = 0; ret == 0 && i < num; i++) {
> +		msg = &msgs[i];
> +		if (msg->flags & I2C_M_RD)
> +			ret = xlr_i2c_rx(priv, msg->len, &msg->buf[0],
> +					msg->addr);
> +		else
> +			ret = xlr_i2c_tx(priv, msg->len, &msg->buf[0],
> +					msg->addr);
> +	}
> +
> +	return (ret != 0) ? ret : num;
> +}
> +
> +static u32 xlr_func(struct i2c_adapter *adap)
> +{
> +	/* Emulate SMBUS over I2C */
> +	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C;
> +}
> +
> +static struct i2c_algorithm xlr_i2c_algo = {
> +	.master_xfer	= xlr_i2c_xfer,
> +	.functionality	= xlr_func,
> +};
> +
> +static int __devinit xlr_i2c_probe(struct platform_device *pdev)
> +{
> +	struct xlr_i2c_private  *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENXIO;

You don't have to check res, devm_request_and_ioremap() will do it for you.

> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->iobase = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!priv->iobase) {
> +		dev_err(&pdev->dev, "devm_request_and_ioremap failed\n");
> +		ret = -EBUSY;
> +		goto err1;
> +	}
> +
> +	priv->adap.dev.parent = &pdev->dev;
> +	priv->adap.owner	= THIS_MODULE;
> +	priv->adap.algo_data	= priv;
> +	priv->adap.algo		= &xlr_i2c_algo;
> +	priv->adap.nr		= pdev->id;
> +	priv->adap.class	= I2C_CLASS_HWMON;
> +	snprintf(priv->adap.name, sizeof(priv->adap.name), "xlr-i2c");
> +
> +	platform_set_drvdata(pdev, priv);
> +	i2c_set_adapdata(&priv->adap, priv);
> +	ret = i2c_add_numbered_adapter(&priv->adap);
> +
> +	if (ret < 0) {
> +		dev_err(&priv->adap.dev, "Failed to add i2c bus.\n");
> +		goto err2;
> +	}
> +
> +	dev_info(&priv->adap.dev, "Added I2C Bus.\n");
> +	return 0;
> +err2:
> +	devm_release_mem_region(&pdev->dev, res->start, resource_size(res));
> +	devm_iounmap(&pdev->dev, priv->iobase);

You don't need to free devm_* resources. That's the main point of it :)

> +	platform_set_drvdata(pdev, NULL);
> +err1:
> +	devm_kfree(&pdev->dev, priv);
> +	return ret;
> +}
> +
> +static int __devexit xlr_i2c_remove(struct platform_device *pdev)
> +{
> +	struct xlr_i2c_private *priv;
> +	struct resource *res;
> +
> +	priv = platform_get_drvdata(pdev);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	i2c_del_adapter(&priv->adap);
> +	devm_release_mem_region(&pdev->dev, res->start, resource_size(res));
> +	devm_iounmap(&pdev->dev, priv->iobase);
> +	devm_kfree(&pdev->dev, priv);
> +
> +	platform_set_drvdata(pdev, NULL);
> +	return 0;
> +}
> +
> +static struct platform_driver xlr_i2c_driver = {
> +	.probe  = xlr_i2c_probe,
> +	.remove = __devexit_p(xlr_i2c_remove),
> +	.driver = {
> +		.name   = "xlr-i2cbus",
> +		.owner  = THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(xlr_i2c_driver);
> +
> +MODULE_AUTHOR("Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>");
> +MODULE_DESCRIPTION("XLR/XLS SoC I2C Controller driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:xlr-i2cbus");
> -- 
> 1.7.5.4
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Jayachandran C]

Wolfram Sang [w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org] wrote:
>> +config I2C_XLR
>> +     tristate "XLR I2C support"
>> +     depends on CPU_XLR
>> +     help
>> +       This driver enables support for the on-chip I2C interface of
>> +       the Netlogic XLR/XLS MIPS processors.
>> +
>> +       Say yes to this option if you have a Netlogic XLR/XLS based
>> +       board and you need to access the I2C devices (typically the
>> +       RTC, sensors, EEPROM) connected to this interface.
>
> Do you insist on this paragraph?

Not really :)
I did not see what is wrong here, but taken out anyway.

>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -68,6 +68,7 @@ obj-$(CONFIG_I2C_TEGRA)             += i2c-tegra.o
>>  obj-$(CONFIG_I2C_VERSATILE)  += i2c-versatile.o
>>  obj-$(CONFIG_I2C_OCTEON)     += i2c-octeon.o
>>  obj-$(CONFIG_I2C_XILINX)     += i2c-xiic.o
>> +obj-$(CONFIG_I2C_XLR)           += i2c-xlr.o
>
> Use tabs here.

Done, BTW, CONFIG_I2C_EG20T is also un-sorted and uses spaces.

>> +     retries = 0;
>> +retry:
>> +     pos = 1;
>> +     if (len == 1) {
>> +             xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
>> +                             XLR_I2C_STARTXFR_ND);
>> +     } else {
>> +             xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, buf[pos]);
>> +             xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
>> +                             XLR_I2C_STARTXFR_WR);
>> +     }
>> +
>> +     while (1) {
>> +             if (retries++ > MAX_RETRIES) {
>> +                     dev_err(&adap->dev, "I2C transmit timeout\n");
>> +                     return -ETIMEDOUT;
>> +             }
>
> Still no. A device usually fails after trying for a specific period of time,
> not after a number of retries. You need to work with jiffies here and
> time_after/time_before. Check drivers/misc/eeprom/at24.c for an example.
> Or check the second part of my talk from Prague :)
> http://free-electrons.com/blog/elce-2011-videos/

The timeout is only for the case where there is a serious issue, all the
normal cases including errors are handled in the code already. The retries
was just a mechanism not to hang the CPU when the hardware is dead. 

The normal transaction take about hundreds of microseconds and the
timeout is 10 times that, which will workout to about a jiffy...

Anyway, I have replaced it with jiffies based timeout.

>> +
>> +             i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
>> +
>> +             if (i2c_status & XLR_I2C_SDOEMPTY) {
>> +                     pos++;
>> +                     retries = 0;
>> +                     nb = (pos < len) ? buf[pos] : 0;
>> +                     xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, nb);
>> +             }
>> +
>> +             if (i2c_status & XLR_I2C_ARB_STARTERR)
>> +                     goto retry;
>
> This will be an endless loop if the STARTERR condition remains? Also this goto
> jumping out of the while loop is not nice. It might help to rearrange the
> loops.

No, the retry count is only reset when there is a successful byte transfer, so the
logic is okay.

[...]
>> +             i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
>> +             if (i2c_status & XLR_I2C_RXRDY) {
>> +                     buf[pos++] = (u8)xlr_i2c_rdreg(priv->iobase,
>> +                                     XLR_I2C_DATAIN);
>> +                     retries = 0;
>> +             }
>
> comments from above also here.

This was okay, but I have fixed up an unrelated issue in the code,
i.e. buf[len] could be written in some cases.

[...]
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     if (!res)
>> +             return -ENXIO;
>
> You don't have to check res, devm_request_and_ioremap() will do it for you.

Fixed. Thanks.

>> +     devm_release_mem_region(&pdev->dev, res->start, resource_size(res));
>> +     devm_iounmap(&pdev->dev, priv->iobase);
>
>You don't need to free devm_* resources. That's the main point of it :)

Thanks, fixed this here and in xlr_i2c_remove.  I will follow-up with the
updated patch.

JC.

[PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Jayachandran C.]

From: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

Add support for the intergrated I2C controller on Netlogic
XLR/XLS MIPS SoC.

The changes are to add a new file i2c/buses/i2c-xlr.c, containing the
i2c bus implementation, and to update i2c/buses/{Kconfig,Makefile} to
add the CONFIG_I2C_XLR option.

Signed-off-by: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
---
 drivers/i2c/busses/Kconfig   |   10 ++
 drivers/i2c/busses/Makefile  |    1 +
 drivers/i2c/busses/i2c-xlr.c |  279 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 290 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-xlr.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index cbe7a2f..40b7c18 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -681,6 +681,16 @@ config I2C_XILINX
 	  This driver can also be built as a module.  If so, the module
 	  will be called xilinx_i2c.
 
+config I2C_XLR
+	tristate "XLR I2C support"
+	depends on CPU_XLR
+	help
+	  This driver enables support for the on-chip I2C interface of
+	  the Netlogic XLR/XLS MIPS processors.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-xlr.
+
 config I2C_EG20T
 	tristate "Intel EG20T PCH / OKI SEMICONDUCTOR IOH(ML7213/ML7223)"
 	depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index fba6da6..a0c644d 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
 obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
 obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
+obj-$(CONFIG_I2C_XLR)		+= i2c-xlr.o
 obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
 
 # External I2C/SMBus adapter drivers
diff --git a/drivers/i2c/busses/i2c-xlr.c b/drivers/i2c/busses/i2c-xlr.c
new file mode 100644
index 0000000..ecf3e9e
--- /dev/null
+++ b/drivers/i2c/busses/i2c-xlr.c
@@ -0,0 +1,279 @@
+/*
+ * Copyright 2011, Netlogic Microsystems Inc.
+ * Copyright 2004, Matt Porter <mporter-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+/* XLR I2C REGISTERS */
+#define XLR_I2C_CFG		0x00
+#define XLR_I2C_CLKDIV		0x01
+#define XLR_I2C_DEVADDR		0x02
+#define XLR_I2C_ADDR		0x03
+#define XLR_I2C_DATAOUT		0x04
+#define XLR_I2C_DATAIN		0x05
+#define XLR_I2C_STATUS		0x06
+#define XLR_I2C_STARTXFR	0x07
+#define XLR_I2C_BYTECNT		0x08
+#define XLR_I2C_HDSTATIM	0x09
+
+/* XLR I2C REGISTERS FLAGS */
+#define XLR_I2C_BUS_BUSY	0x01
+#define XLR_I2C_SDOEMPTY	0x02
+#define XLR_I2C_RXRDY		0x04
+#define XLR_I2C_ACK_ERR		0x08
+#define XLR_I2C_ARB_STARTERR	0x30
+
+/* Register Values */
+#define XLR_I2C_CFG_ADDR	0xF8
+#define XLR_I2C_CFG_NOADDR	0xFA
+#define XLR_I2C_STARTXFR_ND	0x02    /* No Data */
+#define XLR_I2C_STARTXFR_RD	0x01    /* Read */
+#define XLR_I2C_STARTXFR_WR	0x00    /* Write */
+
+#define XLR_I2C_TIMEOUT		5000	/* timeout per byte in usec*/
+
+/*
+ * On XLR/XLS, we need to use __raw_ IO to read the I2C registers
+ * because they are in the big-endian MMIO area on the SoC.
+ *
+ * The readl/writel implementation on XLR/XLS byteswaps, because
+ * those are for its little-endian PCI space (see arch/mips/Kconfig).
+ */
+static inline void xlr_i2c_wreg(u32 __iomem *base, unsigned int reg, u32 val)
+{
+	__raw_writel(val, base + reg);
+}
+
+static inline u32 xlr_i2c_rdreg(u32 __iomem *base, unsigned int reg)
+{
+	return __raw_readl(base + reg);
+}
+
+struct xlr_i2c_private {
+	struct i2c_adapter adap;
+	u32 __iomem *iobase;
+};
+
+static int xlr_i2c_tx(struct xlr_i2c_private *priv,  u16 len,
+	u8 *buf, u16 addr)
+{
+	struct i2c_adapter *adap = &priv->adap;
+	u32 i2c_status;
+	unsigned long timeout, stoptime;
+	int pos;
+	u8 offset, byte;
+
+	offset = buf[0];
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_ADDR, offset);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_ADDR);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len - 1);
+
+	timeout = usecs_to_jiffies(XLR_I2C_TIMEOUT);
+	stoptime = jiffies + timeout;
+	pos = 1;
+retry:
+	if (len == 1) {
+		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
+				XLR_I2C_STARTXFR_ND);
+	} else {
+		xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, buf[pos]);
+		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
+				XLR_I2C_STARTXFR_WR);
+	}
+
+	while (1) {
+		if (time_after(jiffies, stoptime)) {
+			dev_err(&adap->dev, "I2C transmit timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
+
+		if (i2c_status & XLR_I2C_SDOEMPTY) {
+			pos++;
+			/* need to do a empty dataout after the last byte */
+			byte = (pos < len) ? buf[pos] : 0;
+			xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, byte);
+
+			/* reset timeout on successful xmit */
+			stoptime = jiffies + timeout;
+		}
+
+		if (i2c_status & XLR_I2C_ARB_STARTERR)
+			goto retry;
+
+		if (i2c_status & XLR_I2C_ACK_ERR)
+			return -EIO;
+
+		if (i2c_status & XLR_I2C_BUS_BUSY)
+			continue;
+
+		if (pos >= len)
+			break;
+	}
+
+	return 0;
+}
+
+static int xlr_i2c_rx(struct xlr_i2c_private *priv, u16 len, u8 *buf, u16 addr)
+{
+	struct i2c_adapter *adap = &priv->adap;
+	u32 i2c_status;
+	unsigned long timeout, stoptime;
+	int nbytes;
+	u8 byte;
+
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_NOADDR);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len);
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
+
+	timeout = usecs_to_jiffies(XLR_I2C_TIMEOUT);
+	stoptime = jiffies + timeout;
+	nbytes = 0;
+retry:
+	xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR, XLR_I2C_STARTXFR_RD);
+
+	while (1) {
+		if (time_after(jiffies, stoptime)) {
+			dev_err(&adap->dev, "I2C receive timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
+		if (i2c_status & XLR_I2C_RXRDY) {
+			if (nbytes > len)
+				return -EIO;	/* should not happen */
+
+			/* we need to do a dummy datain when nbytes == len */
+			byte = xlr_i2c_rdreg(priv->iobase, XLR_I2C_DATAIN);
+			if (nbytes < len)
+				buf[nbytes] = byte;
+			nbytes++;
+
+			/* reset timeout on successful read */
+			stoptime = jiffies + timeout;
+		}
+
+		if (i2c_status & XLR_I2C_ARB_STARTERR)
+			goto retry;
+
+		if (i2c_status & XLR_I2C_ACK_ERR) {
+			dev_err(&adap->dev, "I2C receive ACK error\n");
+			return -EIO;
+		}
+
+		if ((i2c_status & XLR_I2C_BUS_BUSY) == 0)
+			break;
+	}
+	return 0;
+}
+
+static int xlr_i2c_xfer(struct i2c_adapter *adap,
+	struct i2c_msg *msgs, int num)
+{
+	struct i2c_msg *msg;
+	int i;
+	int ret = 0;
+	struct xlr_i2c_private *priv = i2c_get_adapdata(adap);
+
+	for (i = 0; ret == 0 && i < num; i++) {
+		msg = &msgs[i];
+		if (msg->flags & I2C_M_RD)
+			ret = xlr_i2c_rx(priv, msg->len, &msg->buf[0],
+					msg->addr);
+		else
+			ret = xlr_i2c_tx(priv, msg->len, &msg->buf[0],
+					msg->addr);
+	}
+
+	return (ret != 0) ? ret : num;
+}
+
+static u32 xlr_func(struct i2c_adapter *adap)
+{
+	/* Emulate SMBUS over I2C */
+	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C;
+}
+
+static struct i2c_algorithm xlr_i2c_algo = {
+	.master_xfer	= xlr_i2c_xfer,
+	.functionality	= xlr_func,
+};
+
+static int __devinit xlr_i2c_probe(struct platform_device *pdev)
+{
+	struct xlr_i2c_private  *priv;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->iobase = devm_request_and_ioremap(&pdev->dev, res);
+	if (!priv->iobase) {
+		dev_err(&pdev->dev, "devm_request_and_ioremap failed\n");
+		return -EBUSY;
+	}
+
+	priv->adap.dev.parent = &pdev->dev;
+	priv->adap.owner	= THIS_MODULE;
+	priv->adap.algo_data	= priv;
+	priv->adap.algo		= &xlr_i2c_algo;
+	priv->adap.nr		= pdev->id;
+	priv->adap.class	= I2C_CLASS_HWMON;
+	snprintf(priv->adap.name, sizeof(priv->adap.name), "xlr-i2c");
+
+	i2c_set_adapdata(&priv->adap, priv);
+	ret = i2c_add_numbered_adapter(&priv->adap);
+	if (ret < 0) {
+		dev_err(&priv->adap.dev, "Failed to add i2c bus.\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, priv);
+	dev_info(&priv->adap.dev, "Added I2C Bus.\n");
+	return 0;
+}
+
+static int __devexit xlr_i2c_remove(struct platform_device *pdev)
+{
+	struct xlr_i2c_private *priv;
+
+	priv = platform_get_drvdata(pdev);
+	i2c_del_adapter(&priv->adap);
+	platform_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+static struct platform_driver xlr_i2c_driver = {
+	.probe  = xlr_i2c_probe,
+	.remove = __devexit_p(xlr_i2c_remove),
+	.driver = {
+		.name   = "xlr-i2cbus",
+		.owner  = THIS_MODULE,
+	},
+};
+
+module_platform_driver(xlr_i2c_driver);
+
+MODULE_AUTHOR("Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>");
+MODULE_DESCRIPTION("XLR/XLS SoC I2C Controller driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:xlr-i2cbus");
-- 
1.7.5.4

Re: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Wolfram Sang]

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

On Thu, Jan 19, 2012 at 08:12:51PM +0530, Jayachandran C. wrote:
> From: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> 
> Add support for the intergrated I2C controller on Netlogic
> XLR/XLS MIPS SoC.
> 
> The changes are to add a new file i2c/buses/i2c-xlr.c, containing the
> i2c bus implementation, and to update i2c/buses/{Kconfig,Makefile} to
> add the CONFIG_I2C_XLR option.
> 
> Signed-off-by: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

Please say [PATCH V3] instead of just [PATCH], this helps reviewing.
Also, start a new mail-thread, it might get too messy if 10 different
versions get comments.

> +static int xlr_i2c_tx(struct xlr_i2c_private *priv,  u16 len,
> +	u8 *buf, u16 addr)
> +{
> +	struct i2c_adapter *adap = &priv->adap;
> +	u32 i2c_status;
> +	unsigned long timeout, stoptime;
> +	int pos;
> +	u8 offset, byte;
> +
> +	offset = buf[0];
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_ADDR, offset);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_ADDR);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len - 1);
> +
> +	timeout = usecs_to_jiffies(XLR_I2C_TIMEOUT);
> +	stoptime = jiffies + timeout;
> +	pos = 1;
> +retry:
> +	if (len == 1) {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_ND);
> +	} else {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, buf[pos]);
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_WR);
> +	}
> +
> +	while (1) {

If you get scheduled away here and return after stoptime, you won't have
checked the status even once. Do something like

		checktime = jiffies;
		i2c_status = ...
		/* check error bits */
		if time_after(checktime, stoptime)
			return -ETIMEDOUT;

An interrupt would be much better, of course.

> +		if (time_after(jiffies, stoptime)) {
> +			dev_err(&adap->dev, "I2C transmit timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
> +
> +		if (i2c_status & XLR_I2C_SDOEMPTY) {
> +			pos++;
> +			/* need to do a empty dataout after the last byte */
> +			byte = (pos < len) ? buf[pos] : 0;
> +			xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, byte);
> +
> +			/* reset timeout on successful xmit */
> +			stoptime = jiffies + timeout;
> +		}
> +
> +		if (i2c_status & XLR_I2C_ARB_STARTERR)
> +			goto retry;
> +
> +		if (i2c_status & XLR_I2C_ACK_ERR)
> +			return -EIO;
> +
> +		if (i2c_status & XLR_I2C_BUS_BUSY)
> +			continue;
> +
> +		if (pos >= len)
> +			break;
> +	}
> +
> +	return 0;
> +}

Rest looks okay to me.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Mark Brown]

On Wed, Jan 25, 2012 at 02:40:55PM +0100, Wolfram Sang wrote:

> Please say [PATCH V3] instead of just [PATCH], this helps reviewing.
> Also, start a new mail-thread, it might get too messy if 10 different
> versions get comments.

...but note that opinions vary on bothering the versioning (it's not
likely to actually annoy anyone though).  The bit about starting a new
thread is important though.

Re: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Wolfram Sang]

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

On Wed, Jan 25, 2012 at 01:45:15PM +0000, Mark Brown wrote:
> On Wed, Jan 25, 2012 at 02:40:55PM +0100, Wolfram Sang wrote:
> 
> > Please say [PATCH V3] instead of just [PATCH], this helps reviewing.
> > Also, start a new mail-thread, it might get too messy if 10 different
> > versions get comments.
> 
> ...but note that opinions vary on bothering the versioning (it's not
> likely to actually annoy anyone though).

Huh, never heard of those. When digging old/lost/forgotten patches via
search engines, the versioning does help a lot, for example. Even when
going through huge mailboxes, I'd think this may help to prevent that
old patches are picked? What is the reasoning against versioning?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Jean Delvare]

On Wed, 25 Jan 2012 14:52:36 +0100, Wolfram Sang wrote:
> On Wed, Jan 25, 2012 at 01:45:15PM +0000, Mark Brown wrote:
> > On Wed, Jan 25, 2012 at 02:40:55PM +0100, Wolfram Sang wrote:
> > 
> > > Please say [PATCH V3] instead of just [PATCH], this helps reviewing.
> > > Also, start a new mail-thread, it might get too messy if 10 different
> > > versions get comments.
> > 
> > ...but note that opinions vary on bothering the versioning (it's not
> > likely to actually annoy anyone though).
> 
> Huh, never heard of those. When digging old/lost/forgotten patches via
> search engines, the versioning does help a lot, for example. Even when
> going through huge mailboxes, I'd think this may help to prevent that
> old patches are picked? What is the reasoning against versioning?

Versioning is good, I don't think there's anything to argue about.

-- 
Jean Delvare

Re: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Mark Brown]

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

On Wed, Jan 25, 2012 at 02:52:36PM +0100, Wolfram Sang wrote:
> On Wed, Jan 25, 2012 at 01:45:15PM +0000, Mark Brown wrote:

> > ...but note that opinions vary on bothering the versioning (it's not
> > likely to actually annoy anyone though).

> Huh, never heard of those. When digging old/lost/forgotten patches via
> search engines, the versioning does help a lot, for example. Even when
> going through huge mailboxes, I'd think this may help to prevent that

Personally I've never found the version number added anything for any of
that - the fact that it's vX doesn't say anything about vX+1 existing.

> old patches are picked? What is the reasoning against versioning?

I've not seen anyone actively objecting to it but it's certainly not
universal that people want it.  Mostly just due it not being worth the
effort maintaining it, and the occasional confusion it can generate with
new patches having old versions (eg, adding a new patch to an existing
series).  There's also sometimes the depression that comes from seeing
large versions on what ought to be trivial patches.

What does seem to be more of an issue is that tracking the versions of
patches pushes people towards other behaviours which aren't great.  The
one I've really noticed is that quite a few new contributors seem to get
the idea that both the version numbering and the series numbering get
attached to a patch the first time it's posted so even when a series
should be split (eg, because the core bit of the series which everything
depends on got merged) so you see an individual patch posted as patch
3/4 or whatever.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]