[PATCH v2 1/2] i2c: xiic: Add OF support for Xilinx i2c bus interface

[PATCH v2 1/2] i2c: xiic: Add OF support for Xilinx i2c bus interface

[Michal Simek]

Add OF support to Xilinx i2c bus interface.
It is used on Microblaze systems.

Signed-off-by: Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
---
 drivers/i2c/busses/i2c-xiic.c |   46 ++++++++++++++++++++++++++++++++++------
 1 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index a9c419e..4166570 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -2,6 +2,8 @@
  * i2c-xiic.c
  * Copyright (c) 2002-2007 Xilinx Inc.
  * Copyright (c) 2009-2010 Intel Corporation
+ * Copyright (c) 2010-2011 Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
+ * Copyright (c) 2010-2011 PetaLogix
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -41,6 +43,13 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 
+#ifdef CONFIG_OF
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_i2c.h>
+#include <linux/of_address.h>
+#endif
+
 #define DRIVER_NAME "xiic-i2c"
 
 enum xilinx_i2c_state {
@@ -426,7 +435,7 @@ static void xiic_process(struct xiic_i2c *i2c)
 			xiic_wakeup(i2c, STATE_ERROR);
 
 	} else if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) {
-		/* Transmit register/FIFO is empty or ½ empty */
+		/* Transmit register/FIFO is empty or 1/2 empty */
 
 		clr = pend &
 			(XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK);
@@ -691,10 +700,12 @@ static struct i2c_adapter xiic_adapter = {
 static int __devinit xiic_i2c_probe(struct platform_device *pdev)
 {
 	struct xiic_i2c *i2c;
+#ifndef CONFIG_OF
 	struct xiic_i2c_platform_data *pdata;
+	u8 i;
+#endif
 	struct resource *res;
 	int ret, irq;
-	u8 i;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
@@ -704,10 +715,6 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
 	if (irq < 0)
 		goto resource_missing;
 
-	pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
-	if (!pdata)
-		return -EINVAL;
-
 	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
@@ -730,7 +737,9 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
 	i2c->adap = xiic_adapter;
 	i2c_set_adapdata(&i2c->adap, i2c);
 	i2c->adap.dev.parent = &pdev->dev;
-
+#ifdef CONFIG_OF
+	i2c->adap.dev.of_node = of_node_get(pdev->dev.of_node);
+#endif
 	xiic_reinit(i2c);
 
 	spin_lock_init(&i2c->lock);
@@ -748,9 +757,20 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
 		goto add_adapter_failed;
 	}
 
+	dev_info(&pdev->dev, "mapped to 0x%08X, irq=%d\n",
+					(unsigned int)i2c->base, irq);
+
+#ifndef CONFIG_OF
+	pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
+	if (!pdata)
+		return -EINVAL;
+
 	/* add in known devices to the bus */
 	for (i = 0; i < pdata->num_devices; i++)
 		i2c_new_device(&i2c->adap, pdata->devices + i);
+#else
+	of_i2c_register_devices(&i2c->adap);
+#endif
 
 	return 0;
 
@@ -799,12 +819,24 @@ static int __devexit xiic_i2c_remove(struct platform_device* pdev)
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:"DRIVER_NAME);
 
+#ifdef CONFIG_OF
+/* Match table for of_platform binding */
+static struct of_device_id __devinitdata xilinx_iic_of_match[] = {
+	{ .compatible = "xlnx,xps-iic-2.00.a", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xilinx_iic_of_match);
+#endif
+
 static struct platform_driver xiic_i2c_driver = {
 	.probe   = xiic_i2c_probe,
 	.remove  = __devexit_p(xiic_i2c_remove),
 	.driver  = {
 		.owner = THIS_MODULE,
 		.name = DRIVER_NAME,
+#ifdef CONFIG_OF
+		.of_match_table = xilinx_iic_of_match,
+#endif
 	},
 };
 
-- 
1.5.5.6

[PATCH v2 2/2] i2c: xiic: Use 32bit accesses only

[Michal Simek]

i2c driver is used for LE/BE that's why is useful to use
32bit accesses. Then it is not necessary to solve any
endian issues.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---
 drivers/i2c/busses/i2c-xiic.c |   66 +++++++++++++++++------------------------
 1 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 4166570..f6367c2 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -182,21 +182,6 @@ struct xiic_i2c {
 static void xiic_start_xfer(struct xiic_i2c *i2c);
 static void __xiic_start_xfer(struct xiic_i2c *i2c);
 
-static inline void xiic_setreg8(struct xiic_i2c *i2c, int reg, u8 value)
-{
-	iowrite8(value, i2c->base + reg);
-}
-
-static inline u8 xiic_getreg8(struct xiic_i2c *i2c, int reg)
-{
-	return ioread8(i2c->base + reg);
-}
-
-static inline void xiic_setreg16(struct xiic_i2c *i2c, int reg, u16 value)
-{
-	iowrite16(value, i2c->base + reg);
-}
-
 static inline void xiic_setreg32(struct xiic_i2c *i2c, int reg, int value)
 {
 	iowrite32(value, i2c->base + reg);
@@ -234,10 +219,11 @@ static inline void xiic_irq_clr_en(struct xiic_i2c *i2c, u32 mask)
 static void xiic_clear_rx_fifo(struct xiic_i2c *i2c)
 {
 	u8 sr;
-	for (sr = xiic_getreg8(i2c, XIIC_SR_REG_OFFSET);
+	for (sr = xiic_getreg32(i2c, XIIC_SR_REG_OFFSET);
 		!(sr & XIIC_SR_RX_FIFO_EMPTY_MASK);
-		sr = xiic_getreg8(i2c, XIIC_SR_REG_OFFSET))
-		xiic_getreg8(i2c, XIIC_DRR_REG_OFFSET);
+		sr = xiic_getreg32(i2c, XIIC_SR_REG_OFFSET)) {
+		xiic_getreg32(i2c, XIIC_DRR_REG_OFFSET);
+	}
 }
 
 static void xiic_reinit(struct xiic_i2c *i2c)
@@ -245,13 +231,13 @@ static void xiic_reinit(struct xiic_i2c *i2c)
 	xiic_setreg32(i2c, XIIC_RESETR_OFFSET, XIIC_RESET_MASK);
 
 	/* Set receive Fifo depth to maximum (zero based). */
-	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, IIC_RX_FIFO_DEPTH - 1);
+	xiic_setreg32(i2c, XIIC_RFD_REG_OFFSET, IIC_RX_FIFO_DEPTH - 1);
 
 	/* Reset Tx Fifo. */
-	xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK);
+	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK);
 
 	/* Enable IIC Device, remove Tx Fifo reset & disable general call. */
-	xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_ENABLE_DEVICE_MASK);
+	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_ENABLE_DEVICE_MASK);
 
 	/* make sure RX fifo is empty */
 	xiic_clear_rx_fifo(i2c);
@@ -269,8 +255,9 @@ static void xiic_deinit(struct xiic_i2c *i2c)
 	xiic_setreg32(i2c, XIIC_RESETR_OFFSET, XIIC_RESET_MASK);
 
 	/* Disable IIC Device. */
-	cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
-	xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr & ~XIIC_CR_ENABLE_DEVICE_MASK);
+	cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
+	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET,
+					cr & ~XIIC_CR_ENABLE_DEVICE_MASK);
 }
 
 static void xiic_read_rx(struct xiic_i2c *i2c)
@@ -278,22 +265,22 @@ static void xiic_read_rx(struct xiic_i2c *i2c)
 	u8 bytes_in_fifo;
 	int i;
 
-	bytes_in_fifo = xiic_getreg8(i2c, XIIC_RFO_REG_OFFSET) + 1;
+	bytes_in_fifo = xiic_getreg32(i2c, XIIC_RFO_REG_OFFSET) + 1;
 
 	dev_dbg(i2c->adap.dev.parent, "%s entry, bytes in fifo: %d, msg: %d"
 		", SR: 0x%x, CR: 0x%x\n",
 		__func__, bytes_in_fifo, xiic_rx_space(i2c),
-		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
-		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
+		xiic_getreg32(i2c, XIIC_SR_REG_OFFSET),
+		xiic_getreg32(i2c, XIIC_CR_REG_OFFSET));
 
 	if (bytes_in_fifo > xiic_rx_space(i2c))
 		bytes_in_fifo = xiic_rx_space(i2c);
 
 	for (i = 0; i < bytes_in_fifo; i++)
 		i2c->rx_msg->buf[i2c->rx_pos++] =
-			xiic_getreg8(i2c, XIIC_DRR_REG_OFFSET);
+			xiic_getreg32(i2c, XIIC_DRR_REG_OFFSET);
 
-	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET,
+	xiic_setreg32(i2c, XIIC_RFD_REG_OFFSET,
 		(xiic_rx_space(i2c) > IIC_RX_FIFO_DEPTH) ?
 		IIC_RX_FIFO_DEPTH - 1 :  xiic_rx_space(i2c) - 1);
 }
@@ -301,7 +288,7 @@ static void xiic_read_rx(struct xiic_i2c *i2c)
 static int xiic_tx_fifo_space(struct xiic_i2c *i2c)
 {
 	/* return the actual space left in the FIFO */
-	return IIC_TX_FIFO_DEPTH - xiic_getreg8(i2c, XIIC_TFO_REG_OFFSET) - 1;
+	return IIC_TX_FIFO_DEPTH - xiic_getreg32(i2c, XIIC_TFO_REG_OFFSET) - 1;
 }
 
 static void xiic_fill_tx_fifo(struct xiic_i2c *i2c)
@@ -321,9 +308,9 @@ static void xiic_fill_tx_fifo(struct xiic_i2c *i2c)
 			data |= XIIC_TX_DYN_STOP_MASK;
 			dev_dbg(i2c->adap.dev.parent, "%s TX STOP\n", __func__);
 
-			xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
+			xiic_setreg32(i2c, XIIC_DTR_REG_OFFSET, data);
 		} else
-			xiic_setreg8(i2c, XIIC_DTR_REG_OFFSET, data);
+			xiic_setreg32(i2c, XIIC_DTR_REG_OFFSET, data);
 	}
 }
 
@@ -352,7 +339,8 @@ static void xiic_process(struct xiic_i2c *i2c)
 
 	dev_dbg(i2c->adap.dev.parent, "%s entry, IER: 0x%x, ISR: 0x%x, "
 		"pend: 0x%x, SR: 0x%x, msg: %p, nmsgs: %d\n",
-		__func__, ier, isr, pend, xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
+		__func__, ier, isr, pend,
+		xiic_getreg32(i2c, XIIC_SR_REG_OFFSET),
 		i2c->tx_msg, i2c->nmsgs);
 
 	/* Do not processes a devices interrupts if the device has no
@@ -483,7 +471,7 @@ out:
 
 static int xiic_bus_busy(struct xiic_i2c *i2c)
 {
-	u8 sr = xiic_getreg8(i2c, XIIC_SR_REG_OFFSET);
+	u8 sr = xiic_getreg32(i2c, XIIC_SR_REG_OFFSET);
 
 	return (sr & XIIC_SR_BUS_BUSY_MASK) ? -EBUSY : 0;
 }
@@ -526,17 +514,17 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 	rx_watermark = msg->len;
 	if (rx_watermark > IIC_RX_FIFO_DEPTH)
 		rx_watermark = IIC_RX_FIFO_DEPTH;
-	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
+	xiic_setreg32(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
 
 	if (!(msg->flags & I2C_M_NOSTART))
 		/* write the address */
-		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
+		xiic_setreg32(i2c, XIIC_DTR_REG_OFFSET,
 			(msg->addr << 1) | XIIC_READ_OPERATION |
 			XIIC_TX_DYN_START_MASK);
 
 	xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
 
-	xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
+	xiic_setreg32(i2c, XIIC_DTR_REG_OFFSET,
 		msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
 	if (i2c->nmsgs == 1)
 		/* very last, enable bus not busy as well */
@@ -555,7 +543,7 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 	dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d, "
 		"ISR: 0x%x, CR: 0x%x\n",
 		__func__, msg, msg->len, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
-		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
+		xiic_getreg32(i2c, XIIC_CR_REG_OFFSET));
 
 	if (!(msg->flags & I2C_M_NOSTART)) {
 		/* write the address */
@@ -565,7 +553,7 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 			/* no data and last message -> add STOP */
 			data |= XIIC_TX_DYN_STOP_MASK;
 
-		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
+		xiic_setreg32(i2c, XIIC_DTR_REG_OFFSET, data);
 	}
 
 	xiic_fill_tx_fifo(i2c);
@@ -657,7 +645,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	int err;
 
 	dev_dbg(adap->dev.parent, "%s entry SR: 0x%x\n", __func__,
-		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET));
+		xiic_getreg32(i2c, XIIC_SR_REG_OFFSET));
 
 	err = xiic_busy(i2c);
 	if (err)
-- 
1.5.5.6

Re: [PATCH v2 1/2] i2c: xiic: Add OF support for Xilinx i2c bus interface

[Grant Likely]

On Tue, Feb 22, 2011 at 7:22 AM, Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org> wrote:
> Add OF support to Xilinx i2c bus interface.
> It is used on Microblaze systems.
>
> Signed-off-by: Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>

Hi Michal, comments below...

> ---
>  drivers/i2c/busses/i2c-xiic.c |   46 ++++++++++++++++++++++++++++++++++------
>  1 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index a9c419e..4166570 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -2,6 +2,8 @@
>  * i2c-xiic.c
>  * Copyright (c) 2002-2007 Xilinx Inc.
>  * Copyright (c) 2009-2010 Intel Corporation
> + * Copyright (c) 2010-2011 Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
> + * Copyright (c) 2010-2011 PetaLogix

This change is going to end up being pretty trivial.  It would be a
little odd to claim copyright on such a tiny change; particularly when
it just follows the pattern already established by other drivers.

>  *
>  * This program is free software; you can redistribute it and/or modify
>  * it under the terms of the GNU General Public License version 2 as
> @@ -41,6 +43,13 @@
>  #include <linux/io.h>
>  #include <linux/slab.h>
>
> +#ifdef CONFIG_OF
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_i2c.h>
> +#include <linux/of_address.h>
> +#endif

As previously mentioned; drop the #ifdef protection around these
includes. It should be safe to include them unconditionally, and if it
isn't then it is a bug in the headers which should be fixed.  These
includes should be part of the regular block of includes.

> +
>  #define DRIVER_NAME "xiic-i2c"
>
>  enum xilinx_i2c_state {
> @@ -426,7 +435,7 @@ static void xiic_process(struct xiic_i2c *i2c)
>                        xiic_wakeup(i2c, STATE_ERROR);
>
>        } else if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) {
> -               /* Transmit register/FIFO is empty or ½ empty */
> +               /* Transmit register/FIFO is empty or 1/2 empty */
>
>                clr = pend &
>                        (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK);
> @@ -691,10 +700,12 @@ static struct i2c_adapter xiic_adapter = {
>  static int __devinit xiic_i2c_probe(struct platform_device *pdev)
>  {
>        struct xiic_i2c *i2c;
> +#ifndef CONFIG_OF
>        struct xiic_i2c_platform_data *pdata;
> +       u8 i;
> +#endif
>        struct resource *res;
>        int ret, irq;
> -       u8 i;

You can drop this hunk (see below).

>
>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>        if (!res)
> @@ -704,10 +715,6 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
>        if (irq < 0)
>                goto resource_missing;
>
> -       pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
> -       if (!pdata)
> -               return -EINVAL;
> -
>        i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
>        if (!i2c)
>                return -ENOMEM;
> @@ -730,7 +737,9 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
>        i2c->adap = xiic_adapter;
>        i2c_set_adapdata(&i2c->adap, i2c);
>        i2c->adap.dev.parent = &pdev->dev;
> -
> +#ifdef CONFIG_OF
> +       i2c->adap.dev.of_node = of_node_get(pdev->dev.of_node);
> +#endif

Drop the #ifdef here.  In devicetree/next of_node is no longer
protected by a #ifdef.

>        xiic_reinit(i2c);
>
>        spin_lock_init(&i2c->lock);
> @@ -748,9 +757,20 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
>                goto add_adapter_failed;
>        }
>
> +       dev_info(&pdev->dev, "mapped to 0x%08X, irq=%d\n",
> +                                       (unsigned int)i2c->base, irq);
> +
> +#ifndef CONFIG_OF
> +       pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
> +       if (!pdata)
> +               return -EINVAL;
> +
>        /* add in known devices to the bus */
>        for (i = 0; i < pdata->num_devices; i++)
>                i2c_new_device(&i2c->adap, pdata->devices + i);
> +#else
> +       of_i2c_register_devices(&i2c->adap);
> +#endif

Do it this way instead:

	pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
	if (pdata) {
		/* add in known devices to the bus */
		for (i = 0; i < pdata->num_devices; i++)
		       i2c_new_device(&i2c->adap, pdata->devices + i);
	}

	of_i2c_register_devices(&i2c->adap);

of_i2c_register_devices() is safe to call regardless of if CONFIG_OF
is selected or not.  This driver should be written to handle both
device tree and platform_data use cases at run time.

>
>        return 0;
>
> @@ -799,12 +819,24 @@ static int __devexit xiic_i2c_remove(struct platform_device* pdev)
>  /* work with hotplug and coldplug */
>  MODULE_ALIAS("platform:"DRIVER_NAME);
>
> +#ifdef CONFIG_OF
> +/* Match table for of_platform binding */
> +static struct of_device_id __devinitdata xilinx_iic_of_match[] = {
> +       { .compatible = "xlnx,xps-iic-2.00.a", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, xilinx_iic_of_match);

If you add the following, then you can drop the #ifdef around
.of_match_table below...

#else
#define xilinx_iic_of_match NULL

> +#endif
> +
>  static struct platform_driver xiic_i2c_driver = {
>        .probe   = xiic_i2c_probe,
>        .remove  = __devexit_p(xiic_i2c_remove),
>        .driver  = {
>                .owner = THIS_MODULE,
>                .name = DRIVER_NAME,
> +#ifdef CONFIG_OF
> +               .of_match_table = xilinx_iic_of_match,
> +#endif

Drop the #ifdef and base this patch on devicetree/next

>        },
>  };
>
> --
> 1.5.5.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v2 1/2] i2c: xiic: Add OF support for Xilinx i2c bus interface

[Michal Simek]

Grant Likely wrote:
> On Tue, Feb 22, 2011 at 7:22 AM, Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org> wrote:
>> Add OF support to Xilinx i2c bus interface.
>> It is used on Microblaze systems.
>>
>> Signed-off-by: Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
> 
> Hi Michal, comments below...
> 
>> ---
>>  drivers/i2c/busses/i2c-xiic.c |   46 ++++++++++++++++++++++++++++++++++------
>>  1 files changed, 39 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
>> index a9c419e..4166570 100644
>> --- a/drivers/i2c/busses/i2c-xiic.c
>> +++ b/drivers/i2c/busses/i2c-xiic.c
>> @@ -2,6 +2,8 @@
>>  * i2c-xiic.c
>>  * Copyright (c) 2002-2007 Xilinx Inc.
>>  * Copyright (c) 2009-2010 Intel Corporation
>> + * Copyright (c) 2010-2011 Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
>> + * Copyright (c) 2010-2011 PetaLogix
> 
> This change is going to end up being pretty trivial.  It would be a
> little odd to claim copyright on such a tiny change; particularly when
> it just follows the pattern already established by other drivers.

agree - I forget to remove it - it was in my origin patch.

> 
>>  *
>>  * This program is free software; you can redistribute it and/or modify
>>  * it under the terms of the GNU General Public License version 2 as
>> @@ -41,6 +43,13 @@
>>  #include <linux/io.h>
>>  #include <linux/slab.h>
>>
>> +#ifdef CONFIG_OF
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_i2c.h>
>> +#include <linux/of_address.h>
>> +#endif
> 
> As previously mentioned; drop the #ifdef protection around these
> includes. It should be safe to include them unconditionally, and if it
> isn't then it is a bug in the headers which should be fixed.  These
> includes should be part of the regular block of includes.

remove done

> 
>> +
>>  #define DRIVER_NAME "xiic-i2c"
>>
>>  enum xilinx_i2c_state {
>> @@ -426,7 +435,7 @@ static void xiic_process(struct xiic_i2c *i2c)
>>                        xiic_wakeup(i2c, STATE_ERROR);
>>
>>        } else if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) {
>> -               /* Transmit register/FIFO is empty or ½ empty */
>> +               /* Transmit register/FIFO is empty or 1/2 empty */
>>
>>                clr = pend &
>>                        (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK);
>> @@ -691,10 +700,12 @@ static struct i2c_adapter xiic_adapter = {
>>  static int __devinit xiic_i2c_probe(struct platform_device *pdev)
>>  {
>>        struct xiic_i2c *i2c;
>> +#ifndef CONFIG_OF
>>        struct xiic_i2c_platform_data *pdata;
>> +       u8 i;
>> +#endif
>>        struct resource *res;
>>        int ret, irq;
>> -       u8 i;
> 
> You can drop this hunk (see below).

done

> 
>>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>        if (!res)
>> @@ -704,10 +715,6 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
>>        if (irq < 0)
>>                goto resource_missing;
>>
>> -       pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
>> -       if (!pdata)
>> -               return -EINVAL;
>> -
>>        i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
>>        if (!i2c)
>>                return -ENOMEM;
>> @@ -730,7 +737,9 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
>>        i2c->adap = xiic_adapter;
>>        i2c_set_adapdata(&i2c->adap, i2c);
>>        i2c->adap.dev.parent = &pdev->dev;
>> -
>> +#ifdef CONFIG_OF
>> +       i2c->adap.dev.of_node = of_node_get(pdev->dev.of_node);
>> +#endif
> 
> Drop the #ifdef here.  In devicetree/next of_node is no longer
> protected by a #ifdef.

ok, done

> 
>>        xiic_reinit(i2c);
>>
>>        spin_lock_init(&i2c->lock);
>> @@ -748,9 +757,20 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
>>                goto add_adapter_failed;
>>        }
>>
>> +       dev_info(&pdev->dev, "mapped to 0x%08X, irq=%d\n",
>> +                                       (unsigned int)i2c->base, irq);
>> +
>> +#ifndef CONFIG_OF
>> +       pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
>> +       if (!pdata)
>> +               return -EINVAL;
>> +
>>        /* add in known devices to the bus */
>>        for (i = 0; i < pdata->num_devices; i++)
>>                i2c_new_device(&i2c->adap, pdata->devices + i);
>> +#else
>> +       of_i2c_register_devices(&i2c->adap);
>> +#endif
> 
> Do it this way instead:
> 
> 	pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
> 	if (pdata) {
> 		/* add in known devices to the bus */
> 		for (i = 0; i < pdata->num_devices; i++)
> 		       i2c_new_device(&i2c->adap, pdata->devices + i);
> 	}
> 
> 	of_i2c_register_devices(&i2c->adap);
> 
> of_i2c_register_devices() is safe to call regardless of if CONFIG_OF
> is selected or not.  This driver should be written to handle both
> device tree and platform_data use cases at run time.

ok, done. I hope that it doesn't non OF version. I can't easily test it.

> 
>>        return 0;
>>
>> @@ -799,12 +819,24 @@ static int __devexit xiic_i2c_remove(struct platform_device* pdev)
>>  /* work with hotplug and coldplug */
>>  MODULE_ALIAS("platform:"DRIVER_NAME);
>>
>> +#ifdef CONFIG_OF
>> +/* Match table for of_platform binding */
>> +static struct of_device_id __devinitdata xilinx_iic_of_match[] = {
>> +       { .compatible = "xlnx,xps-iic-2.00.a", },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, xilinx_iic_of_match);
> 
> If you add the following, then you can drop the #ifdef around
> .of_match_table below...
> 
> #else
> #define xilinx_iic_of_match NULL
> 
>> +#endif
>> +
>>  static struct platform_driver xiic_i2c_driver = {
>>        .probe   = xiic_i2c_probe,
>>        .remove  = __devexit_p(xiic_i2c_remove),
>>        .driver  = {
>>                .owner = THIS_MODULE,
>>                .name = DRIVER_NAME,
>> +#ifdef CONFIG_OF
>> +               .of_match_table = xilinx_iic_of_match,
>> +#endif
> 
> Drop the #ifdef and base this patch on devicetree/next

specifically this I was checking in the Linus tree to know if there is any ifdef.
devicetree/next branch is fine.

I have sent v3 based on your devicetree/next branch.

Thanks for your comments,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian