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

From: Michal Simek
Date: Tue Feb 22 2011 - 12:52:22 EST

Grant Likely wrote:
On Tue, Feb 22, 2011 at 7:22 AM, Michal Simek <monstr@xxxxxxxxx> wrote:
Add OF support to Xilinx i2c bus interface.
It is used on Microblaze systems.

Signed-off-by: Michal Simek <monstr@xxxxxxxxx>

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@xxxxxxxxx>
+ * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/