[PATCH] unicore32: add i2c bus driver for PKUnity SoC

[PATCH] unicore32: add i2c bus driver for PKUnity SoC

[Guan Xuetao]

From: Guan Xuetao <gxt@mprc.pku.edu.cn>

This patch adds framebuffer driver for unigfx engine in PKUnity SoC.

Signed-off-by: Guan Xuetao <gxt@mprc.pku.edu.cn>
---
 drivers/i2c/busses/Kconfig    |    5 +
 drivers/i2c/busses/Makefile   |    1 +
 drivers/i2c/busses/puv3_i2c.c |  300 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 306 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 3a6321c..20d328c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -236,6 +236,11 @@ config I2C_VIAPRO
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-viapro.
 
+config I2C_PUV3
+	tristate "PKUnity v3 I2C bus support"
+	depends on I2C && UNICORE32 && ARCH_PUV3
+	select I2C_ALGOBIT
+
 if ACPI
 
 comment "ACPI drivers"
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 84cb16a..cb4a097 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.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_PUV3)		+= puv3_i2c.o
 
 # External I2C/SMBus adapter drivers
 obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
diff --git a/drivers/i2c/busses/puv3_i2c.c b/drivers/i2c/busses/puv3_i2c.c
new file mode 100644
index 0000000..fb4f1fd
--- /dev/null
+++ b/drivers/i2c/busses/puv3_i2c.c
@@ -0,0 +1,300 @@
+/*
+ * I2C driver for PKUnity-v3 SoC
+ * Code specific to PKUnity SoC and UniCore ISA
+ *
+ *	Maintained by GUAN Xue-tao <gxt@mprc.pku.edu.cn>
+ *	Copyright (C) 2001-2010 Guan Xuetao
+ *
+ * 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
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <mach/hardware.h>
+
+static int i2c_reg = -1;
+
+/*
+ * Poll the i2c status register until the specified bit is set.
+ * Returns 0 if timed out (100 msec).
+ */
+static short poll_status(unsigned long bit)
+{
+	int loop_cntr = 1000;
+
+	if (bit & I2C_STATUS_TFNF) {
+		do {
+			udelay(10);
+		} while (!(I2C_STATUS & bit) && (--loop_cntr > 0));
+	} else {
+		/* RXRDY handler */
+		do {
+			if (I2C_TAR == I2C_TAR_EEPROM)
+				msleep(20);
+			else
+				udelay(10);
+		} while (!(I2C_RXFLR & 0xf) && (--loop_cntr > 0));
+	}
+
+	return (loop_cntr > 0);
+}
+
+static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length)
+{
+	/* Read data */
+	while (length--) {
+		if (!poll_status(I2C_STATUS_TFNF)) {
+			dev_dbg(&adap->dev, "Tx FIFO Not Full timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		/* send addr */
+		I2C_DATACMD = i2c_reg | I2C_DATACMD_WRITE;
+
+		/* get ready to next write */
+		i2c_reg++;
+
+		/* send read CMD */
+		I2C_DATACMD = I2C_DATACMD_READ;
+
+		/* wait until the Rx FIFO have available */
+		if (!poll_status(I2C_STATUS_RFNE)) {
+			dev_dbg(&adap->dev, "RXRDY timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		/* read the data to buf */
+		*buf = (I2C_DATACMD & I2C_DATACMD_DAT_MASK);
+		buf++;
+	}
+
+	i2c_reg = -1;
+
+	return 0;
+}
+
+static int xfer_write(struct i2c_adapter *adap, unsigned char *buf, int length)
+{
+	/* Do nothing but storing the reg_num to a static variable */
+	i2c_reg = *buf;
+	if (i2c_reg == -1) {
+		printk(KERN_WARNING "Error i2c reg\n");
+		return -ETIMEDOUT;
+	}
+
+	if (length == 1)
+		return 0;
+
+	buf++;
+	length--;
+	while (length--) {
+		/* send addr */
+		I2C_DATACMD = i2c_reg | I2C_DATACMD_WRITE;
+
+		/* send write CMD */
+		I2C_DATACMD = *buf | I2C_DATACMD_WRITE;
+
+		/* wait until the Rx FIFO have available */
+		msleep(20);
+
+		/* read the data to buf */
+		i2c_reg++;
+		buf++;
+	}
+
+	i2c_reg = -1;
+
+	return 0;
+}
+
+/*
+ * Generic i2c master transfer entrypoint.
+ *
+ */
+static int puv3_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *pmsg,
+		int num)
+{
+	int i, ret;
+	unsigned char swap;
+
+	/* Disable i2c */
+	I2C_ENABLE = I2C_ENABLE_DISABLE;
+
+	/* Set the work mode and speed*/
+	I2C_CON = I2C_CON_MASTER | I2C_CON_SPEED_STD | I2C_CON_SLAVEDISABLE;
+
+	I2C_TAR = pmsg->addr;
+
+	/* Enable i2c */
+	I2C_ENABLE = I2C_ENABLE_ENABLE;
+
+	dev_dbg(&adap->dev, "puv3_i2c_xfer: processing %d messages:\n", num);
+
+	for (i = 0; i < num; i++) {
+		dev_dbg(&adap->dev, " #%d: %sing %d byte%s %s 0x%02x\n", i,
+			pmsg->flags & I2C_M_RD ? "read" : "writ",
+			pmsg->len, pmsg->len > 1 ? "s" : "",
+			pmsg->flags & I2C_M_RD ? "from" : "to",	pmsg->addr);
+
+		if (pmsg->len && pmsg->buf) {	/* sanity check */
+			if (pmsg->flags & I2C_M_RD)
+				ret = xfer_read(adap, pmsg->buf, pmsg->len);
+			else
+				ret = xfer_write(adap, pmsg->buf, pmsg->len);
+
+			if (ret)
+				return ret;
+
+		}
+		dev_dbg(&adap->dev, "transfer complete\n");
+		pmsg++;		/* next message */
+	}
+
+	/* XXX: fixup be16_to_cpu in bq27x00_battery.c */
+	if (pmsg->addr == I2C_TAR_PWIC) {
+		swap = pmsg->buf[0];
+		pmsg->buf[0] = pmsg->buf[1];
+		pmsg->buf[1] = swap;
+	}
+
+	return i;
+}
+
+/*
+ * Return list of supported functionality.
+ */
+static u32 puv3_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm puv3_i2c_algorithm = {
+	.master_xfer	= puv3_i2c_xfer,
+	.functionality	= puv3_i2c_func,
+};
+
+/*
+ * Main initialization routine.
+ */
+static int __devinit puv3_i2c_probe(struct platform_device *pdev)
+{
+	struct i2c_adapter *adapter;
+	struct resource *res;
+	int rc;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENXIO;
+
+	if (!request_mem_region(res->start, resource_size(res), "puv3_i2c"))
+		return -EBUSY;
+
+	adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
+	if (adapter == NULL) {
+		dev_err(&pdev->dev, "can't allocate inteface!\n");
+		rc = -ENOMEM;
+		goto fail2;
+	}
+	snprintf(adapter->name, sizeof(adapter->name), "PUV3");
+	adapter->algo = &puv3_i2c_algorithm;
+	adapter->class = I2C_CLASS_HWMON;
+	adapter->dev.parent = &pdev->dev;
+
+	platform_set_drvdata(pdev, adapter);
+
+	rc = i2c_add_numbered_adapter(adapter);
+	if (rc) {
+		dev_err(&pdev->dev, "Adapter %s registration failed\n",
+				adapter->name);
+		goto fail3;
+	}
+
+	dev_info(&pdev->dev, "PKUnity v3 i2c bus driver.\n");
+	return 0;
+
+fail3:
+	platform_set_drvdata(pdev, NULL);
+	kfree(adapter);
+fail2:
+	release_mem_region(res->start, resource_size(res));
+
+	return rc;
+}
+
+static int puv3_i2c_remove(struct platform_device *pdev)
+{
+	struct i2c_adapter *adapter = platform_get_drvdata(pdev);
+	struct resource *res;
+	int rc;
+
+	rc = i2c_del_adapter(adapter);
+	platform_set_drvdata(pdev, NULL);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(res->start, resource_size(res));
+
+	return rc;
+}
+
+#ifdef CONFIG_PM
+static int puv3_i2c_suspend(struct platform_device *dev, pm_message_t state)
+{
+	int poll_count;
+	/* Disable the IIC */
+	I2C_ENABLE = I2C_ENABLE_DISABLE;
+	for (poll_count = 0; poll_count < 50; poll_count++) {
+		if (I2C_ENSTATUS & I2C_ENSTATUS_ENABLE)
+			udelay(25);
+	}
+
+	return 0;
+}
+
+static int puv3_i2c_resume(struct platform_device *dev)
+{
+	return 0 ;
+}
+#else
+#define puv3_i2c_suspend NULL
+#define puv3_i2c_resume NULL
+#endif
+
+MODULE_ALIAS("platform:puv3_i2c");
+
+static struct platform_driver puv3_i2c_driver = {
+	.probe		= puv3_i2c_probe,
+	.remove		= __devexit_p(puv3_i2c_remove),
+	.suspend	= puv3_i2c_suspend,
+	.resume		= puv3_i2c_resume,
+	.driver		= {
+		.name	= "PKUnity-v3-I2C",
+		.owner	= THIS_MODULE,
+	}
+};
+
+static int __init puv3_i2c_init(void)
+{
+	return platform_driver_register(&puv3_i2c_driver);
+}
+
+static void __exit puv3_i2c_exit(void)
+{
+	platform_driver_unregister(&puv3_i2c_driver);
+}
+
+module_init(puv3_i2c_init);
+module_exit(puv3_i2c_exit);
+
+MODULE_DESCRIPTION("PKUnity v3 I2C driver");
+MODULE_LICENSE("GPL v2");

Re: [PATCH] unicore32: add i2c bus driver for PKUnity SoC

[Jean Delvare]

Hi Guan,

On Sat, 22 Jan 2011 18:55:09 +0800, Guan Xuetao wrote:
> From: Guan Xuetao <gxt-TG0Ac1+ktVePQbnJrJN+5g@public.gmane.org>
> 
> This patch adds framebuffer driver for unigfx engine in PKUnity SoC.

While I don't have the time for a full review, I would like to at least
sort out the integration issues. Framebuffer driver, really? I doubt
it. If this is a copy-and-paste mistake, and this is actually an I2C
system bus driver, you will have to change the driver name (see below.)
If what you actually meant is that this is a driver for the I2C/DDC
channels on a graphics chip, then you have to move this driver to the
same directory the framebuffer driver lives in, as all other
framebuffer drivers do.

> Signed-off-by: Guan Xuetao <gxt-TG0Ac1+ktVePQbnJrJN+5g@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig    |    5 +
>  drivers/i2c/busses/Makefile   |    1 +
>  drivers/i2c/busses/puv3_i2c.c |  300 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 306 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 3a6321c..20d328c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -236,6 +236,11 @@ config I2C_VIAPRO
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-viapro.
>  
> +config I2C_PUV3
> +	tristate "PKUnity v3 I2C bus support"
> +	depends on I2C && UNICORE32 && ARCH_PUV3

The whole menu depends on I2C, so you don't have to repeat it.

> +	select I2C_ALGOBIT

Where is the description of what the driver is for? All other entries
have it, yours is no exception.

> +
>  if ACPI
>  
>  comment "ACPI drivers"
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 84cb16a..cb4a097 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.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_PUV3)		+= puv3_i2c.o

Assuming that this is the right place for your driver, then the name is
NOT correct. As you can see, all drivers are names i2c-*, so your
driver should be too. And the it should be inserted in alphabetical
order.

See also my comments below related to the probe() and remove()
functions.

>  
>  # External I2C/SMBus adapter drivers
>  obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
> diff --git a/drivers/i2c/busses/puv3_i2c.c b/drivers/i2c/busses/puv3_i2c.c
> new file mode 100644
> index 0000000..fb4f1fd
> --- /dev/null
> +++ b/drivers/i2c/busses/puv3_i2c.c
> @@ -0,0 +1,300 @@
> +/*
> + * I2C driver for PKUnity-v3 SoC
> + * Code specific to PKUnity SoC and UniCore ISA
> + *
> + *	Maintained by GUAN Xue-tao <gxt-TG0Ac1+ktVePQbnJrJN+5g@public.gmane.org>
> + *	Copyright (C) 2001-2010 Guan Xuetao
> + *
> + * 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
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <mach/hardware.h>
> +
> +static int i2c_reg = -1;

Please allocate and use per-device data structure for device state
tracking. Global variables are evil (for this use case at least.)

> +
> +/*
> + * Poll the i2c status register until the specified bit is set.
> + * Returns 0 if timed out (100 msec).
> + */
> +static short poll_status(unsigned long bit)
> +{
> +	int loop_cntr = 1000;
> +
> +	if (bit & I2C_STATUS_TFNF) {
> +		do {
> +			udelay(10);
> +		} while (!(I2C_STATUS & bit) && (--loop_cntr > 0));
> +	} else {
> +		/* RXRDY handler */
> +		do {
> +			if (I2C_TAR == I2C_TAR_EEPROM)
> +				msleep(20);
> +			else
> +				udelay(10);
> +		} while (!(I2C_RXFLR & 0xf) && (--loop_cntr > 0));
> +	}
> +
> +	return (loop_cntr > 0);
> +}
> +
> +static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length)
> +{
> +	/* Read data */
> +	while (length--) {
> +		if (!poll_status(I2C_STATUS_TFNF)) {
> +			dev_dbg(&adap->dev, "Tx FIFO Not Full timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		/* send addr */
> +		I2C_DATACMD = i2c_reg | I2C_DATACMD_WRITE;
> +
> +		/* get ready to next write */
> +		i2c_reg++;
> +
> +		/* send read CMD */
> +		I2C_DATACMD = I2C_DATACMD_READ;
> +
> +		/* wait until the Rx FIFO have available */
> +		if (!poll_status(I2C_STATUS_RFNE)) {
> +			dev_dbg(&adap->dev, "RXRDY timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		/* read the data to buf */
> +		*buf = (I2C_DATACMD & I2C_DATACMD_DAT_MASK);
> +		buf++;
> +	}
> +
> +	i2c_reg = -1;
> +
> +	return 0;
> +}
> +
> +static int xfer_write(struct i2c_adapter *adap, unsigned char *buf, int length)
> +{
> +	/* Do nothing but storing the reg_num to a static variable */
> +	i2c_reg = *buf;
> +	if (i2c_reg == -1) {
> +		printk(KERN_WARNING "Error i2c reg\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (length == 1)
> +		return 0;
> +
> +	buf++;
> +	length--;
> +	while (length--) {
> +		/* send addr */
> +		I2C_DATACMD = i2c_reg | I2C_DATACMD_WRITE;
> +
> +		/* send write CMD */
> +		I2C_DATACMD = *buf | I2C_DATACMD_WRITE;
> +
> +		/* wait until the Rx FIFO have available */
> +		msleep(20);
> +
> +		/* read the data to buf */
> +		i2c_reg++;
> +		buf++;
> +	}
> +
> +	i2c_reg = -1;
> +
> +	return 0;
> +}
> +
> +/*
> + * Generic i2c master transfer entrypoint.
> + *
> + */
> +static int puv3_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *pmsg,
> +		int num)
> +{
> +	int i, ret;
> +	unsigned char swap;
> +
> +	/* Disable i2c */
> +	I2C_ENABLE = I2C_ENABLE_DISABLE;
> +
> +	/* Set the work mode and speed*/
> +	I2C_CON = I2C_CON_MASTER | I2C_CON_SPEED_STD | I2C_CON_SLAVEDISABLE;
> +
> +	I2C_TAR = pmsg->addr;
> +
> +	/* Enable i2c */
> +	I2C_ENABLE = I2C_ENABLE_ENABLE;
> +
> +	dev_dbg(&adap->dev, "puv3_i2c_xfer: processing %d messages:\n", num);
> +
> +	for (i = 0; i < num; i++) {
> +		dev_dbg(&adap->dev, " #%d: %sing %d byte%s %s 0x%02x\n", i,
> +			pmsg->flags & I2C_M_RD ? "read" : "writ",
> +			pmsg->len, pmsg->len > 1 ? "s" : "",
> +			pmsg->flags & I2C_M_RD ? "from" : "to",	pmsg->addr);
> +
> +		if (pmsg->len && pmsg->buf) {	/* sanity check */
> +			if (pmsg->flags & I2C_M_RD)
> +				ret = xfer_read(adap, pmsg->buf, pmsg->len);
> +			else
> +				ret = xfer_write(adap, pmsg->buf, pmsg->len);
> +
> +			if (ret)
> +				return ret;
> +
> +		}
> +		dev_dbg(&adap->dev, "transfer complete\n");
> +		pmsg++;		/* next message */
> +	}
> +
> +	/* XXX: fixup be16_to_cpu in bq27x00_battery.c */
> +	if (pmsg->addr == I2C_TAR_PWIC) {
> +		swap = pmsg->buf[0];
> +		pmsg->buf[0] = pmsg->buf[1];
> +		pmsg->buf[1] = swap;
> +	}
> +
> +	return i;
> +}
> +
> +/*
> + * Return list of supported functionality.
> + */
> +static u32 puv3_i2c_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm puv3_i2c_algorithm = {
> +	.master_xfer	= puv3_i2c_xfer,
> +	.functionality	= puv3_i2c_func,
> +};
> +
> +/*
> + * Main initialization routine.
> + */
> +static int __devinit puv3_i2c_probe(struct platform_device *pdev)
> +{
> +	struct i2c_adapter *adapter;
> +	struct resource *res;
> +	int rc;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENXIO;

-ENODEV would seem more appropriate.

> +
> +	if (!request_mem_region(res->start, resource_size(res), "puv3_i2c"))
> +		return -EBUSY;
> +
> +	adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> +	if (adapter == NULL) {
> +		dev_err(&pdev->dev, "can't allocate inteface!\n");
> +		rc = -ENOMEM;
> +		goto fail2;
> +	}
> +	snprintf(adapter->name, sizeof(adapter->name), "PUV3");

Please include some uniqueness in the name. The address of the mem
region would be a good candidate:

	snprintf(adapter->name, sizeof(adapter->name), "PUV3 at 0x%08x",
		 res->start);

This lets user-space differentiate between the adapters if there are
more than one.

> +	adapter->algo = &puv3_i2c_algorithm;
> +	adapter->class = I2C_CLASS_HWMON;
> +	adapter->dev.parent = &pdev->dev;
> +
> +	platform_set_drvdata(pdev, adapter);
> +
> +	rc = i2c_add_numbered_adapter(adapter);

You can't call i2c_add_numbered_adapter() if you haven't set
adapter->nr first.

> +	if (rc) {
> +		dev_err(&pdev->dev, "Adapter %s registration failed\n",
> +				adapter->name);

Quotes around %s would be welcome.

> +		goto fail3;
> +	}
> +
> +	dev_info(&pdev->dev, "PKUnity v3 i2c bus driver.\n");

Wrong message. You have just registered a device, not a driver.

> +	return 0;
> +
> +fail3:

Please use descriptive labels, otherwise it's easy to mess up and jump
to the wrong one.

> +	platform_set_drvdata(pdev, NULL);
> +	kfree(adapter);
> +fail2:
> +	release_mem_region(res->start, resource_size(res));
> +
> +	return rc;
> +}
> +
> +static int puv3_i2c_remove(struct platform_device *pdev)

Missing __devexit.

> +{
> +	struct i2c_adapter *adapter = platform_get_drvdata(pdev);
> +	struct resource *res;
> +	int rc;
> +
> +	rc = i2c_del_adapter(adapter);
> +	platform_set_drvdata(pdev, NULL);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(res->start, resource_size(res));
> +
> +	return rc;

The logic in this function doesn't make sense. If i2c_del_adapter()
fails, you must exit immediately. Freeing the resources while the
adapter is still registered can only lead to a crash.

Additionally, you are leaking memory in the success case, as adapter is
never freed.

> +}
> +
> +#ifdef CONFIG_PM
> +static int puv3_i2c_suspend(struct platform_device *dev, pm_message_t state)
> +{
> +	int poll_count;
> +	/* Disable the IIC */
> +	I2C_ENABLE = I2C_ENABLE_DISABLE;
> +	for (poll_count = 0; poll_count < 50; poll_count++) {
> +		if (I2C_ENSTATUS & I2C_ENSTATUS_ENABLE)
> +			udelay(25);
> +	}
> +
> +	return 0;
> +}
> +
> +static int puv3_i2c_resume(struct platform_device *dev)
> +{
> +	return 0 ;
> +}
> +#else
> +#define puv3_i2c_suspend NULL
> +#define puv3_i2c_resume NULL
> +#endif
> +
> +MODULE_ALIAS("platform:puv3_i2c");
> +
> +static struct platform_driver puv3_i2c_driver = {
> +	.probe		= puv3_i2c_probe,
> +	.remove		= __devexit_p(puv3_i2c_remove),
> +	.suspend	= puv3_i2c_suspend,
> +	.resume		= puv3_i2c_resume,
> +	.driver		= {
> +		.name	= "PKUnity-v3-I2C",
> +		.owner	= THIS_MODULE,
> +	}
> +};
> +
> +static int __init puv3_i2c_init(void)
> +{
> +	return platform_driver_register(&puv3_i2c_driver);
> +}
> +
> +static void __exit puv3_i2c_exit(void)
> +{
> +	platform_driver_unregister(&puv3_i2c_driver);
> +}
> +
> +module_init(puv3_i2c_init);
> +module_exit(puv3_i2c_exit);
> +
> +MODULE_DESCRIPTION("PKUnity v3 I2C driver");
> +MODULE_LICENSE("GPL v2");

-- 
Jean Delvare

RE: [PATCH] unicore32: add i2c bus driver for PKUnity SoC

[Guan Xuetao]

> -----Original Message-----
> From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org]
> Sent: Monday, January 24, 2011 5:19 PM
> To: Guan Xuetao
> Cc: ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] unicore32: add i2c bus driver for PKUnity SoC
> 
> Hi Guan,
> 
> On Sat, 22 Jan 2011 18:55:09 +0800, Guan Xuetao wrote:
> > From: Guan Xuetao <gxt-TG0Ac1+ktVePQbnJrJN+5g@public.gmane.org>
> >
> > This patch adds framebuffer driver for unigfx engine in PKUnity SoC.
> 
> While I don't have the time for a full review, I would like to at least
> sort out the integration issues. Framebuffer driver, really? I doubt
> it. If this is a copy-and-paste mistake, and this is actually an I2C
> system bus driver, you will have to change the driver name (see below.)
> If what you actually meant is that this is a driver for the I2C/DDC
> channels on a graphics chip, then you have to move this driver to the
> same directory the framebuffer driver lives in, as all other
> framebuffer drivers do.
Yes, it's an I2C system bus driver. Sorry for my mistake.

> 
> > Signed-off-by: Guan Xuetao <gxt-TG0Ac1+ktVePQbnJrJN+5g@public.gmane.org>
> > ---
> >  drivers/i2c/busses/Kconfig    |    5 +
> >  drivers/i2c/busses/Makefile   |    1 +
> >  drivers/i2c/busses/puv3_i2c.c |  300 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 306 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 3a6321c..20d328c 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -236,6 +236,11 @@ config I2C_VIAPRO
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called i2c-viapro.
> >
> > +config I2C_PUV3
> > +	tristate "PKUnity v3 I2C bus support"
> > +	depends on I2C && UNICORE32 && ARCH_PUV3
> 
> The whole menu depends on I2C, so you don't have to repeat it.
Ok.

> 
> > +	select I2C_ALGOBIT
> 
> Where is the description of what the driver is for? All other entries
> have it, yours is no exception.
A description block is added.

> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 84cb16a..cb4a097 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -61,6 +61,7 @@ obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.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_PUV3)		+= puv3_i2c.o
> 
> Assuming that this is the right place for your driver, then the name is
> NOT correct. As you can see, all drivers are names i2c-*, so your
> driver should be too. And the it should be inserted in alphabetical
> order.
Ok.

> 
> See also my comments below related to the probe() and remove()
> functions.
> 
> >
> >  # External I2C/SMBus adapter drivers
> >  obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
> > diff --git a/drivers/i2c/busses/puv3_i2c.c b/drivers/i2c/busses/puv3_i2c.c
> > +
> > +static int i2c_reg = -1;
> 
> Please allocate and use per-device data structure for device state
> tracking. Global variables are evil (for this use case at least.)
Yes, i2c_reg is replaced by local variables.

> > +
> > +/*
> > + * Main initialization routine.
> > + */
> > +static int __devinit puv3_i2c_probe(struct platform_device *pdev)
> > +{
> > +	struct i2c_adapter *adapter;
> > +	struct resource *res;
> > +	int rc;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		return -ENXIO;
> 
> -ENODEV would seem more appropriate.
Ok.

> > +	adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> > +	if (adapter == NULL) {
> > +		dev_err(&pdev->dev, "can't allocate inteface!\n");
> > +		rc = -ENOMEM;
> > +		goto fail2;
> > +	}
> > +	snprintf(adapter->name, sizeof(adapter->name), "PUV3");
> 
> Please include some uniqueness in the name. The address of the mem
> region would be a good candidate:
> 
> 	snprintf(adapter->name, sizeof(adapter->name), "PUV3 at 0x%08x",
> 		 res->start);
> 
> This lets user-space differentiate between the adapters if there are
> more than one.
Ok.

> > +
> > +	platform_set_drvdata(pdev, adapter);
> > +
> > +	rc = i2c_add_numbered_adapter(adapter);
> 
> You can't call i2c_add_numbered_adapter() if you haven't set
> adapter->nr first.
Corrected.

> 
> > +	if (rc) {
> > +		dev_err(&pdev->dev, "Adapter %s registration failed\n",
> > +				adapter->name);
> 
> Quotes around %s would be welcome.
Ok.

> 
> > +		goto fail3;
> > +	}
> > +
> > +	dev_info(&pdev->dev, "PKUnity v3 i2c bus driver.\n");
> 
> Wrong message. You have just registered a device, not a driver.
Corrected.

> 
> > +	return 0;
> > +
> > +fail3:
> 
> Please use descriptive labels, otherwise it's easy to mess up and jump
> to the wrong one.
Ok.

> 
> > +	platform_set_drvdata(pdev, NULL);
> > +	kfree(adapter);
> > +fail2:
> > +	release_mem_region(res->start, resource_size(res));
> > +
> > +	return rc;
> > +}
> > +
> > +static int puv3_i2c_remove(struct platform_device *pdev)
> 
> Missing __devexit.
Added.

> 
> > +{
> > +	struct i2c_adapter *adapter = platform_get_drvdata(pdev);
> > +	struct resource *res;
> > +	int rc;
> > +
> > +	rc = i2c_del_adapter(adapter);
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	release_mem_region(res->start, resource_size(res));
> > +
> > +	return rc;
> 
> The logic in this function doesn't make sense. If i2c_del_adapter()
> fails, you must exit immediately. Freeing the resources while the
> adapter is still registered can only lead to a crash.
> 
> Additionally, you are leaking memory in the success case, as adapter is
> never freed.
Above two are corrected.

> 
> --
> Jean Delvare

Thanks Jean.

Guan Xuetao