Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller

[Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>]

Hi Linus,

On Wed, May 27, 2009 at 09:23:22PM +0200, Linus Walleij wrote:
> Just some comments since I've been reading a lot of drivers lately...

Thank you very much.

> 2009/5/19 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:
> >
> > Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > ---
> >  drivers/i2c/busses/Kconfig          |    9 +
> >  drivers/i2c/busses/Makefile         |    1 +
> >  drivers/i2c/busses/i2c-designware.c |  587 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 597 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/i2c/busses/i2c-designware.c
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index f1c6ca7..cef1567 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -326,6 +326,15 @@ config I2C_DAVINCI
> >          devices such as DaVinci NIC.
> >          For details please see http://www.ti.com/davinci
> >
> > +config I2C_DESIGNWARE
> > +       tristate "Synopsys DesignWare"
> > +       help
> > +         If you say yes to this option, support will be included for the
> > +         Synopsys DesignWare I2C adapter. Only master mode is supported.
> > +
> > +         This driver can also be built as a module.  If so, the module
> > +         will be called i2c-designware.
> > +
> >  config I2C_GPIO
> >        tristate "GPIO-based bitbanging I2C"
> >        depends on GENERIC_GPIO
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 776acb6..c2be11e 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_AU1550)      += i2c-au1550.o
> >  obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
> >  obj-$(CONFIG_I2C_CPM)          += i2c-cpm.o
> >  obj-$(CONFIG_I2C_DAVINCI)      += i2c-davinci.o
> > +obj-$(CONFIG_I2C_DESIGNWARE)   += i2c-designware.o
> >  obj-$(CONFIG_I2C_GPIO)         += i2c-gpio.o
> >  obj-$(CONFIG_I2C_HIGHLANDER)   += i2c-highlander.o
> >  obj-$(CONFIG_I2C_IBM_IIC)      += i2c-ibm_iic.o
> > diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
> > new file mode 100644
> > index 0000000..0138fea
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-designware.c
> > @@ -0,0 +1,587 @@
> > +/*
> > + * Synopsys Designware I2C adapter driver (master only).
> > + *
> > + * Based on the TI DAVINCI I2C adapter driver.
> > + *
> > + * Copyright (C) 2006 Texas Instruments.
> > + * Copyright (C) 2007 MontaVista Software Inc.
> > + * Copyright (C) 2009 Provigent Ltd.
> > + *
> > + * ----------------------------------------------------------------------------
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + * ----------------------------------------------------------------------------
> > + *
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/clk.h>
> > +#include <linux/errno.h>
> > +#include <linux/sched.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +
> > +/*
> > + * Registers offset
> > + */
> > +#define DW_IC_CON              0x0
> > +#define DW_IC_TAR              0x4
> > +#define DW_IC_DATA_CMD         0x10
> > +#define DW_IC_SS_SCL_HCNT      0x14
> > +#define DW_IC_SS_SCL_LCNT      0x18
> > +#define DW_IC_FS_SCL_HCNT      0x1c
> > +#define DW_IC_FS_SCL_LCNT      0x20
> > +#define DW_IC_INTR_STAT                0x2c
> > +#define DW_IC_INTR_MASK                0x30
> > +#define DW_IC_CLR_INTR         0x40
> > +#define DW_IC_ENABLE           0x6c
> > +#define DW_IC_STATUS           0x70
> > +#define DW_IC_TXFLR            0x74
> > +#define DW_IC_RXFLR            0x78
> > +#define DW_IC_COMP_PARAM_1     0xf4
> > +#define DW_IC_TX_ABRT_SOURCE   0x80
> > +
> > +#define DW_IC_CON_MASTER               0x1
> > +#define DW_IC_CON_SPEED_STD            0x2
> > +#define DW_IC_CON_SPEED_FAST           0x4
> > +#define DW_IC_CON_10BITADDR_MASTER     0x10
> > +#define DW_IC_CON_RESTART_EN           0x20
> > +#define DW_IC_CON_SLAVE_DISABLE                0x40
> > +
> > +#define DW_IC_INTR_TX_EMPTY    0x10
> > +#define DW_IC_INTR_TX_ABRT     0x40
> > +#define DW_IC_INTR_STOP_DET    0x200
> > +
> > +#define DW_IC_STATUS_ACTIVITY  0x1
> > +
> > +#define DW_IC_ERR_TX_ABRT      0x1
> > +
> > +/*
> > + * status codes
> > + */
> > +#define STATUS_IDLE                    0x0
> > +#define STATUS_WRITE_IN_PROGRESS       0x1
> > +#define STATUS_READ_IN_PROGRESS                0x2
> > +
> > +/*
> > + * hardware abort codes
> > + *
> > + * only expected abort codes are listed here
> > + * refer to the datasheet for the full list
> 
> Write here that this is the bit number in register DW_IC_TX_ABRT_SOURCE
> that's good to know!

OK.

> Makes me curious why the rest of the codes are unexpected, usually
> error paths handle the unexpected but hey, I can live with degrees
> of unexpectedness..

Aborts no. 6 and 8 are related to high seed which is not supported by this 
driver. Aborts no. 13-15 are only relevant for hardware operating as slave, 
which is not supported as well.

> > + */
> > +#define ABRT_7B_ADDR_NOACK     0
> > +#define ABRT_10ADDR1_NOACK     1
> > +#define ABRT_10ADDR2_NOACK     2
> > +#define ABRT_TXDATA_NOACK      3
> > +#define ABRT_GCALL_NOACK       4
> > +#define ABRT_GCALL_READ                5
> > +#define ABRT_SBYTE_ACKDET      7
> > +#define ABRT_SBYTE_NORSTRT     9
> > +#define ABRT_10B_RD_NORSTRT    10
> > +#define ARB_MASTER_DIS         11
> > +#define ARB_LOST               12
> > +
> > +static char *abort_sources[] = {
> > +       [ABRT_7B_ADDR_NOACK]    =
> > +               "slave address not acknowledged (7bit mode)",
> > +       [ABRT_10ADDR1_NOACK]    =
> > +               "first address byte not acknowledged (10bit mode)",
> > +       [ABRT_10ADDR2_NOACK]    =
> > +               "second address byte not acknowledged (10bit mode)",
> > +       [ABRT_TXDATA_NOACK]             =
> > +               "data not acknowledged",
> > +       [ABRT_GCALL_NOACK]              =
> > +               "no acknowledgement for a general call",
> > +       [ABRT_GCALL_READ]               =
> > +               "read after general call",
> > +       [ABRT_SBYTE_ACKDET]             =
> > +               "start byte acknowledged",
> > +       [ABRT_SBYTE_NORSTRT]    =
> > +               "trying to send start byte when restart is disabled",
> > +       [ABRT_10B_RD_NORSTRT]   =
> > +               "trying to read when restart is disabled (10bit mode)",
> > +       [ARB_MASTER_DIS]                =
> > +               "trying to use disabled adapter",
> > +       [ARB_LOST]                      =
> > +               "lost arbitration",
> > +};
> > +
> > +struct dw_i2c_dev {
> > +       struct device           *dev;
> > +       void __iomem            *base;
> > +       struct completion       cmd_complete;
> > +       struct tasklet_struct   pump_msg;
> > +       struct mutex            lock;
> > +       struct clk              *clk;
> > +       int                     cmd_err;
> > +       struct i2c_msg          *msgs;
> > +       int                     msgs_num;
> > +       int                     msg_write_idx;
> > +       int                     msg_read_idx;
> > +       int                     msg_err;
> > +       unsigned int            status;
> > +       u16                     abort_source;
> > +       int                     irq;
> > +       struct i2c_adapter      adapter;
> > +       unsigned int            tx_fifo_depth;
> > +       unsigned int            rx_fifo_depth;
> > +};
> > +
> > +/*
> > + * This functions configures I2C and enables I2C.
> > + * This function is called during I2C init function.
> > + */
> > +static int i2c_dw_init(struct dw_i2c_dev *dev)
> 
> Can this be tagged with __init?

No. It's called from i2c_dw_xfer() at run time in case of timeout.

> 
> > +{
> > +       u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
> > +       u16 ic_con;
> > +
> > +       /* Disable the adapter */
> > +       writeb(0, dev->base + DW_IC_ENABLE);
> > +
> > +       /* set standard and fast speed deviders for high/low periods */
> > +       writew((input_clock_khz * 40 / 10000)+1, /* std speed high, 4us */
> > +                       dev->base + DW_IC_SS_SCL_HCNT);
> > +       writew((input_clock_khz * 47 / 10000)+1, /* std speed low, 4.7us */
> > +                       dev->base + DW_IC_SS_SCL_LCNT);
> > +       writew((input_clock_khz *  6 / 10000)+1, /* fast speed high, 0.6us */
> > +                       dev->base + DW_IC_FS_SCL_HCNT);
> > +       writew((input_clock_khz * 13 / 10000)+1, /* fast speed low, 1.3us */
> > +                       dev->base + DW_IC_FS_SCL_LCNT);
> > +
> > +       /* configure the i2c master */
> > +       ic_con = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
> > +               DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
> > +       writew(ic_con, dev->base + DW_IC_CON);
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * Waiting for bus not busy
> > + */
> > +static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
> > +{
> > +       unsigned long timeout;
> > +
> > +       timeout = jiffies + HZ;
> 
> Is this timeout really valid if this device is clocked differently than
> the CPU, as indicated when supplying a clk? Well, should work...

This really looks wrong. I'll look for some better way to do this.

> > +       while (readb(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
> > +               if (time_after(jiffies, timeout)) {
> > +                       dev_warn(dev->dev,
> > +                                "timeout waiting for bus ready\n");
> > +                       return -ETIMEDOUT;
> > +               }
> > +               schedule_timeout(1);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * Initiate low level master read/write transaction.
> > + * This function is called from i2c_dw_xfer when starting a transfer.
> > + * This function is also called from dw_i2c_pump_msg to continue a transfer
> > + * that is longer than the size of the TX FIFO.
> > + */
> > +static void
> > +i2c_dw_xfer_msg(struct i2c_adapter *adap)
> > +{
> > +       struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > +       struct i2c_msg *msgs = dev->msgs;
> > +       int num = dev->msgs_num;
> > +       u16 ic_con, intr_mask;
> > +       int tx_valid_bytes = readb(dev->base + DW_IC_TXFLR);
> > +       int tx_limit = dev->tx_fifo_depth - tx_valid_bytes;
> > +       int rx_limit = dev->rx_fifo_depth - readb(dev->base + DW_IC_RXFLR)
> > +               - tx_valid_bytes;
> > +       static u16 buf_len = 0;
> > +       static u16 addr;
> > +
> > +       if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> > +               /* Disable the adapter */
> > +               writeb(0, dev->base + DW_IC_ENABLE);
> > +
> > +               /* set the slave (target) address */
> > +               writew(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
> > +
> > +               /* if the slave address is ten bit address, enable 10BITADDR */
> > +               ic_con = readw(dev->base + DW_IC_CON);
> > +               if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
> > +                       ic_con |= DW_IC_CON_10BITADDR_MASTER;
> > +               else
> > +                       ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
> > +               writew(ic_con, dev->base + DW_IC_CON);
> > +
> > +               /* Enable the adapter */
> > +               writeb(1, dev->base + DW_IC_ENABLE);
> > +
> > +               addr = msgs[dev->msg_write_idx].addr;
> > +       }
> > +
> > +       for (; dev->msg_write_idx < num; dev->msg_write_idx++) {
> > +               static u8 *buf;
> > +
> > +               /* if target address has changed, we need to
> > +                * reprogram the target address in the i2c
> > +                * adapter when we are done with this transfer
> > +                */
> > +               if (msgs[dev->msg_write_idx].addr != addr)
> > +                       return;
> > +
> > +               if (msgs[dev->msg_write_idx].len == 0) {
> > +                       dev_err(dev->dev,
> > +                               "%s: invalid message length\n", __func__);
> > +                       dev->msg_err = -EINVAL;
> > +                       return;
> > +               }
> > +
> > +               if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> > +                       /* new i2c_msg */
> > +                       buf = msgs[dev->msg_write_idx].buf;
> > +                       buf_len = msgs[dev->msg_write_idx].len;
> > +               }
> > +
> > +               for (; buf_len > 0 && tx_limit > 0 && rx_limit > 0; buf_len--)
> > +                       if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
> > +                               writew(0x100, dev->base + DW_IC_DATA_CMD);
> > +                               rx_limit--; tx_limit--;
> > +                       } else {
> > +                               writew(*buf++, dev->base + DW_IC_DATA_CMD);
> > +                               tx_limit--;
> > +                       }
> > +       }
> > +
> > +       intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
> > +       if (buf_len > 0) { /* more bytes to be written */
> > +               intr_mask |= DW_IC_INTR_TX_EMPTY;
> > +               dev->status |= STATUS_WRITE_IN_PROGRESS;
> > +       } else
> > +               dev->status &= ~STATUS_WRITE_IN_PROGRESS;
> > +       writew(intr_mask, dev->base + DW_IC_INTR_MASK);
> > +}
> > +
> > +static void
> > +i2c_dw_read(struct i2c_adapter *adap)
> > +{
> > +       struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > +       struct i2c_msg *msgs = dev->msgs;
> > +       int num = dev->msgs_num;
> > +       u16 addr = msgs[dev->msg_read_idx].addr;
> > +       int rx_valid = readw(dev->base + DW_IC_RXFLR);
> > +
> > +       for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
> > +               static int len;
> > +               static u8 *buf;
> > +
> > +               if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))
> > +                       continue;
> > +
> > +               /* different i2c client, reprogram the i2c adapter */
> > +               if (msgs[dev->msg_read_idx].addr != addr)
> > +                       return;
> > +
> > +               if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
> > +                       len = msgs[dev->msg_read_idx].len;
> > +                       buf = msgs[dev->msg_read_idx].buf;
> > +               }
> > +
> > +               for (; len > 0 && rx_valid > 0; len--, rx_valid--)
> > +                       *buf++ = readb(dev->base + DW_IC_DATA_CMD);
> > +
> > +               if (len > 0) {
> > +                       dev->status |= STATUS_READ_IN_PROGRESS;
> > +                       return;
> > +               } else
> > +                       dev->status &= ~STATUS_READ_IN_PROGRESS;
> > +       }
> > +}
> > +
> > +/*
> > + * Prepare controller for a transaction and call i2c_dw_xfer_msg
> > + */
> > +static int
> > +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > +{
> > +       struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > +       int ret;
> > +
> > +       dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> > +
> > +       mutex_lock(&dev->lock);
> > +
> > +       INIT_COMPLETION(dev->cmd_complete);
> > +       dev->msgs = msgs;
> > +       dev->msgs_num = num;
> > +       dev->cmd_err = 0;
> > +       dev->msg_write_idx = 0;
> > +       dev->msg_read_idx = 0;
> > +       dev->msg_err = 0;
> > +       dev->status = STATUS_IDLE;
> > +
> > +       ret = i2c_dw_wait_bus_not_busy(dev);
> > +       if (ret < 0)
> > +               goto done;
> > +
> > +       /* start the transfers */
> > +       i2c_dw_xfer_msg(adap);
> > +
> > +       /* wait for tx to complete */
> > +       ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
> > +       if (ret == 0) {
> > +               dev_err(dev->dev, "controller timed out\n");
> > +               i2c_dw_init(dev);
> > +               ret = -ETIMEDOUT;
> > +               goto done;
> > +       } else if (ret < 0)
> > +               goto done;
> > +
> > +       if (dev->msg_err) {
> > +               ret = dev->msg_err;
> > +               goto done;
> > +       }
> > +
> > +       /* no error */
> > +       if (likely(!dev->cmd_err)) {
> > +               /* read rx fifo, and disable the adapter */
> > +               do {
> > +                       i2c_dw_read(adap);
> > +               } while (dev->status & STATUS_READ_IN_PROGRESS);
> > +               writeb(0, dev->base + DW_IC_ENABLE);
> > +               ret = num;
> > +               goto done;
> > +       }
> > +
> > +       /* We have an error */
> > +       if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> > +               unsigned long abort_source = dev->abort_source;
> > +               int i;
> > +
> > +               for_each_bit(i, &abort_source, ARRAY_SIZE(abort_sources)) {
> > +                   dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
> > +               }
> > +       }
> > +       ret = -EIO;
> > +
> > +done:
> > +       mutex_unlock(&dev->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +static u32 i2c_dw_func(struct i2c_adapter *adap)
> > +{
> > +       return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
> > +}
> > +
> > +static void dw_i2c_pump_msg(unsigned long data)
> > +{
> > +       struct dw_i2c_dev *dev = (struct dw_i2c_dev *) data;
> > +       u16 intr_mask;
> > +
> > +       i2c_dw_read(&dev->adapter);
> > +       i2c_dw_xfer_msg(&dev->adapter);
> > +
> > +       intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
> > +       if (dev->status & STATUS_WRITE_IN_PROGRESS)
> > +               intr_mask |= DW_IC_INTR_TX_EMPTY;
> > +       writew(intr_mask, dev->base + DW_IC_INTR_MASK);
> > +}
> > +
> > +/*
> > + * Interrupt service routine. This gets called whenever an I2C interrupt
> > + * occurs.
> > + */
> > +static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> > +{
> > +       struct dw_i2c_dev *dev = dev_id;
> > +       u16 stat;
> > +
> > +       stat = readw(dev->base + DW_IC_INTR_STAT);
> > +       dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
> > +       if (stat & DW_IC_INTR_TX_ABRT) {
> > +               dev->abort_source = readw(dev->base + DW_IC_TX_ABRT_SOURCE);
> > +               dev->cmd_err |= DW_IC_ERR_TX_ABRT;
> > +               dev->status = STATUS_IDLE;
> > +       } else if (stat & DW_IC_INTR_TX_EMPTY)
> > +               tasklet_schedule(&dev->pump_msg);
> > +
> > +       readb(dev->base + DW_IC_CLR_INTR);      /* clear interrupts */
> > +       writew(0, dev->base + DW_IC_INTR_MASK); /* disable interrupts */
> > +       if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
> > +               complete(&dev->cmd_complete);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static struct i2c_algorithm i2c_dw_algo = {
> > +       .master_xfer    = i2c_dw_xfer,
> > +       .functionality  = i2c_dw_func,
> > +};
> > +
> > +static int dw_i2c_probe(struct platform_device *pdev)
> 
> Can you tag this with __init?

OK. __devinit is probably better.

> > +{
> > +       struct dw_i2c_dev *dev;
> > +       struct i2c_adapter *adap;
> > +       struct resource *mem, *irq, *ioarea;
> > +       int r;
> > +
> > +       /* NOTE: driver uses the static register mapping */
> > +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!mem) {
> > +               dev_err(&pdev->dev, "no mem resource?\n");
> > +               return -ENODEV;
> 
> I've seen other drivers use -ENOENT which makes sense.

OK.

> > +       }
> > +
> > +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +       if (!irq) {
> > +               dev_err(&pdev->dev, "no irq resource?\n");
> > +               return -ENODEV;
> 
> -ENOENT

OK.

> > +       }
> > +
> > +       ioarea = request_mem_region(mem->start, (mem->end - mem->start) + 1,
> 
> Use resource_size(mem) instead of the last argument.

OK.

> > +                                   pdev->name);
> > +       if (!ioarea) {
> > +               dev_err(&pdev->dev, "I2C region already claimed\n");
> > +               return -EBUSY;
> > +       }
> > +
> > +       dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
> > +       if (!dev) {
> > +               r = -ENOMEM;
> > +               goto err_release_region;
> > +       }
> > +
> > +       init_completion(&dev->cmd_complete);
> > +       tasklet_init(&dev->pump_msg, dw_i2c_pump_msg, (unsigned long) dev);
> > +       mutex_init(&dev->lock);
> > +       dev->dev = get_device(&pdev->dev);
> > +       dev->irq = irq->start;
> > +       platform_set_drvdata(pdev, dev);
> > +
> > +       dev->clk = clk_get(&pdev->dev, "I2CCLK");
> > +       if (IS_ERR(dev->clk)) {
> > +               r = -ENODEV;
> > +               goto err_free_mem;
> > +       }
> > +       clk_enable(dev->clk);
> > +
> > +       dev->base = ioremap(mem->start, mem->end - mem->start + 1);
> 
> resource_size()

OK.

> > +       if (dev->base == NULL) {
> > +               dev_err(&pdev->dev, "failure mapping io resources\n");
> > +               r = -EBUSY;
> > +               goto err_unuse_clocks;
> > +       }
> > +       dev->tx_fifo_depth =
> > +               ((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x00ff0000) >> 16) + 1;
> > +       dev->rx_fifo_depth =
> > +               ((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x0000ff00) >> 8) + 1;
> > +       i2c_dw_init(dev);
> > +
> > +       writew(0, dev->base + DW_IC_INTR_MASK); /* disable IRQ */
> > +       r = request_irq(dev->irq, i2c_dw_isr, 0, pdev->name, dev);
> > +       if (r) {
> > +               dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
> > +               goto err_iounmap;
> > +       }
> > +
> > +       adap = &dev->adapter;
> > +       i2c_set_adapdata(adap, dev);
> > +       adap->owner = THIS_MODULE;
> > +       adap->class = I2C_CLASS_HWMON;
> > +       strlcpy(adap->name, "Synopsys DesignWare I2C adapter",
> > +                       sizeof(adap->name));
> > +       adap->algo = &i2c_dw_algo;
> > +       adap->dev.parent = &pdev->dev;
> > +
> > +       adap->nr = pdev->id;
> > +       r = i2c_add_numbered_adapter(adap);
> > +       if (r) {
> > +               dev_err(&pdev->dev, "failure adding adapter\n");
> > +               goto err_free_irq;
> > +       }
> > +
> > +       return 0;
> > +
> > +err_free_irq:
> > +       free_irq(dev->irq, dev);
> > +err_iounmap:
> > +       iounmap(dev->base);
> > +err_unuse_clocks:
> > +       clk_disable(dev->clk);
> > +       clk_put(dev->clk);
> > +       dev->clk = NULL;
> > +err_free_mem:
> > +       platform_set_drvdata(pdev, NULL);
> > +       put_device(&pdev->dev);
> > +       kfree(dev);
> > +err_release_region:
> > +       release_mem_region(mem->start, (mem->end - mem->start) + 1);
> 
> resource_size()

OK.

> > +
> > +       return r;
> > +}
> > +
> > +static int dw_i2c_remove(struct platform_device *pdev)
> 
> Can you tag this with __exit?

Or __devexit. OK.

> > +{
> > +       struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> > +       struct resource *mem;
> > +
> > +       platform_set_drvdata(pdev, NULL);
> > +       i2c_del_adapter(&dev->adapter);
> > +       put_device(&pdev->dev);
> > +
> > +       clk_disable(dev->clk);
> > +       clk_put(dev->clk);
> > +       dev->clk = NULL;
> > +
> > +       writeb(0, dev->base + DW_IC_ENABLE);
> > +       free_irq(dev->irq, dev);
> > +       kfree(dev);
> > +
> > +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       release_mem_region(mem->start, (mem->end - mem->start) + 1);
> 
> resource_size()
> But I usually cache important resources in the driver private struct
> (struct dw_i2c_dev in this case)

OK.

> > +       return 0;
> > +}
> > +
> > +/* work with hotplug and coldplug */
> > +MODULE_ALIAS("platform:i2c_designware");
> > +
> > +static struct platform_driver dw_i2c_driver = {
> > +       .probe          = dw_i2c_probe,
> 
> Can you remove this and use platform_driver_probe() as detaile below instead?

OK.

> > +       .remove         = dw_i2c_remove,
> 
> If the function can be tagges __exit, this can be tagged
> __exit_p(dw_i2c_remove);

OK.

> > +       .driver         = {
> > +               .name   = "i2c_designware",
> > +               .owner  = THIS_MODULE,
> > +       },
> > +};
> > +
> > +static int __init dw_i2c_init_driver(void)
> > +{
> > +       return platform_driver_register(&dw_i2c_driver);
> 
> return platform_driver_probe(&dw_i2c_driver, dw_i2c_probe);
> and remove the .probe field in the driver.

OK.

> > +}
> > +module_init(dw_i2c_init_driver);
> > +
> > +static void __exit dw_i2c_exit_driver(void)
> > +{
> > +       platform_driver_unregister(&dw_i2c_driver);
> > +}
> > +module_exit(dw_i2c_exit_driver);
> > +
> > +MODULE_AUTHOR("Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>");
> > +MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.6.2.4
> >
> 
> 
> Yours,
> Linus Walleij

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -