Hi Guan, On Sat, 22 Jan 2011 18:55:09 +0800, Guan Xuetao wrote: > From: Guan Xuetao <gxt@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxx> > + * 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html