Re: [PATCH 2/4] i2c: iproc: Add Broadcom iProc I2C Driver

On Wed, Dec 10, 2014 at 7:11 AM, Ray Jui [off-list ref] wrote:

On 12/9/2014 5:33 PM, Varka Bhadram wrote:

quoted

On Wednesday 10 December 2014 06:24 AM, Ray Jui wrote:

quoted

Add initial support to the Broadcom iProc I2C controller found in the
iProc family of SoCs.

The iProc I2C controller has separate internal TX and RX FIFOs, each has
a size of 64 bytes. The iProc I2C controller supports two bus speeds
including standard mode (100kHz) and fast mode (400kHz)

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
  drivers/i2c/busses/Kconfig         |    9 +
  drivers/i2c/busses/Makefile        |    1 +
  drivers/i2c/busses/i2c-bcm-iproc.c |  503
++++++++++++++++++++++++++++++++++++
  3 files changed, 513 insertions(+)
  create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c

(...)

+static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *dev,

quoted

quoted

+        struct i2c_msg *msg)
+{

dto...

 One more indent? Sure.

Yes...

 static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *dev,
                                          struct i2c_msg *msg)

 +    int ret, i;

quoted

quoted

+    u8 addr;
+    u32 val;
+    unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MESC);
+
+    if (msg->len < 1 || msg->len > M_TX_RX_FIFO_SIZE - 1) {
+        dev_err(dev->device,
+            "supported data length is 1 - %u bytes\n",
+                M_TX_RX_FIFO_SIZE - 1);
+        return -EINVAL;
+    }
+
+    dev->msg = msg;
+    ret = __wait_for_bus_idle(dev);
+    if (ret)
+        return ret;
+
+    ret = bcm_iproc_i2c_format_addr(dev, msg, &addr);
+    if (ret)
+        return ret;
+
+    /* load slave address into the TX FIFO */
+    writel(addr, dev->base + M_TX_OFFSET);
+
+    /* for a write transaction, load data into the TX FIFO */
+    if (!(msg->flags & I2C_M_RD)) {
+        for (i = 0; i < msg->len; i++) {
+            val = msg->buf[i];
+
+            /* mark the last byte */
+            if (i == msg->len - 1)
+                val |= 1 << M_TX_WR_STATUS_SHIFT;
+
+            writel(val, dev->base + M_TX_OFFSET);
+        }
+    }
+
+    /* mark as incomplete before starting the transaction */
+    reinit_completion(&dev->done);
+
+    /*
+     * Enable the "start busy" interrupt, which will be triggered after
+     * the transaction is done
+     */
+    writel(1 << IE_M_START_BUSY_SHIFT, dev->base + IE_OFFSET);
+
+    /*
+     * Now we can activate the transfer. For a read operation,
specify the
+     * number of bytes to read
+     */
+    val = 1 << M_CMD_START_BUSY_SHIFT;
+    if (msg->flags & I2C_M_RD) {
+        val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) |
+            (msg->len << M_CMD_RD_CNT_SHIFT);
+    } else {
+        val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
+    }
+    writel(val, dev->base + M_CMD_OFFSET);
+
+    time_left = wait_for_completion_timeout(&dev->done, time_left);
+
+    /* disable all interrupts */
+    writel(0, dev->base + IE_OFFSET);
+
+    if (!time_left) {
+        dev_err(dev->device, "transaction times out\n");
+
+        /* flush FIFOs */
+        val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+            (1 << M_FIFO_TX_FLUSH_SHIFT);
+        writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+        return -EREMOTEIO;
+    }
+
+    ret = bcm_iproc_i2c_check_status(dev);
+    if (ret) {
+        /* flush both TX/RX FIFOs */
+        val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+            (1 << M_FIFO_TX_FLUSH_SHIFT);
+        writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+        return ret;
+    }
+
+    /*
+     * For a read operation, we now need to load the data from FIFO
+     * into the memory buffer
+     */
+    if (msg->flags & I2C_M_RD) {
+        for (i = 0; i < msg->len; i++) {
+            msg->buf[i] = (readl(dev->base + M_RX_OFFSET) >>
+                    M_RX_DATA_SHIFT) & M_RX_DATA_MASK;
+        }
+    }
+
+    dev_dbg(dev->device, "xfer %c, addr=0x%02x, len=%d\n",
+            (msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr,
+            msg->len);
+    dev_dbg(dev->device, "**** data start ****\n");
+    for (i = 0; i < msg->len; i++)
+        dev_dbg(dev->device, "0x%02x ",  msg->buf[i]);
+    dev_dbg(dev->device, "**** data end ****\n");
+
+    return 0;
+}
+
+static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
+                 struct i2c_msg msgs[], int num)
+{
+    struct bcm_iproc_i2c_dev *dev = i2c_get_adapdata(adapter);
+    int ret, i;
+
+    /* go through all messages */
+    for (i = 0; i < num; i++) {
+        ret = bcm_iproc_i2c_xfer_single_msg(dev, &msgs[i]);
+        if (ret) {
+            dev_err(dev->device, "xfer failed\n");
+            return ret;
+        }
+    }
+
+    return num;
+}
+
+static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
+{
+    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm bcm_iproc_algo = {
+    .master_xfer = bcm_iproc_i2c_xfer,
+    .functionality = bcm_iproc_i2c_functionality,
+};
+
+static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *dev)
+{
+    unsigned int bus_speed, speed_bit;
+    u32 val;
+    int ret = of_property_read_u32(dev->device->of_node,
"clock-frequency",
+                       &bus_speed);
+    if (ret < 0) {
+        dev_err(dev->device, "missing clock-frequency property\n");
+        return -ENODEV;
+    }
+
+    switch (bus_speed) {
+    case 100000:
+        speed_bit = 0;
+        break;
+    case 400000:
+        speed_bit = 1;
+        break;
+    default:
+        dev_err(dev->device, "%d Hz bus speed not supported\n",
+                bus_speed);
+        dev_err(dev->device, "valid speeds are 100khz and 400khz\n");
+        return -EINVAL;
+    }
+
+    val = readl(dev->base + TIM_CFG_OFFSET);
+    val &= ~(1 << TIME_CFG_MODE_400_SHIFT);
+    val |= speed_bit << TIME_CFG_MODE_400_SHIFT;
+    writel(val, dev->base + TIM_CFG_OFFSET);
+
+    dev_info(dev->device, "bus set to %u Hz\n", bus_speed);
+
+    return 0;
+}
+
+static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    /* put controller in reset */
+    val = readl(dev->base + CFG_OFFSET);
+    val |= 1 << CFG_RESET_SHIFT;
+    val &= ~(1 << CFG_EN_SHIFT);
+    writel(val, dev->base + CFG_OFFSET);
+
+    /* wait 100 usec per spec */
+    udelay(100);
+
+    /* bring controller out of reset */
+    val = readl(dev->base + CFG_OFFSET);
+    val &= ~(1 << CFG_RESET_SHIFT);
+    writel(val, dev->base + CFG_OFFSET);
+
+    /* flush TX/RX FIFOs and set RX FIFO threshold to zero */
+    val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
+    writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+
+    /* disable all interrupts */
+    val = 0;
+    writel(val, dev->base + IE_OFFSET);
+
+    /* clear all pending interrupts */
+    val = readl(dev->base + IS_OFFSET);
+    writel(val, dev->base + IS_OFFSET);
+
+    return 0;
+}
+
+static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    val = readl(dev->base + CFG_OFFSET);
+    val |= 1 << CFG_EN_SHIFT;
+    writel(val, dev->base + CFG_OFFSET);
+}
+
+static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    val = readl(dev->base + CFG_OFFSET);
+    val &= ~(1 << CFG_EN_SHIFT);
+    writel(val, dev->base + CFG_OFFSET);
+}
+
+static int bcm_iproc_i2c_probe(struct platform_device *pdev)
+{
+    int irq, ret = 0;
+    struct bcm_iproc_i2c_dev *dev;
+    struct i2c_adapter *adap;
+    struct resource *res;
+
+    dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+    if (!dev)
+        return -ENOMEM;
+
+    platform_set_drvdata(pdev, dev);
+    dev->device = &pdev->dev;
+    init_completion(&dev->done);
+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res)
+        return -ENODEV;

We can remove this resource check. This checking will happen with
devm_ioremap_resource()

 Don't you need to obtain a valid resource and pass it into

devm_ioremap_resource? Without 'res' being assigned a valid resource,
devm_ioremap_resource will reject with "invalid resource".

  platform_get_resource() will return a resource, checking on this

resource is happening at
http://lxr.free-electrons.com/source/lib/devres.c#L115. So no need to check
it explicitly.

If you check here it will be duplication of check with resource. Two times
we are checking on
the resource. No point of doing like that.

Thanks.