[PATCH v4 2/3] i2c: iproc: Add Broadcom iProc I2C Driver
flat view
From: Uwe Kleine-König <hidden>
Date: 2015-01-15 08:42:13
Also in:
linux-devicetree, linux-i2c, lkml
Hello, On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote:
quoted hunk ↗ jump to hunk
+#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/sched.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/delay.h>
some of them are not needed. I tested on amd64 and efm32 and could drop linux/device.h, linux/sched.h, linux/clk.h. (BTW, I wonder that you don't need clk handling.)
+#define TIM_CFG_OFFSET 0x04 +#define TIME_CFG_MODE_400_SHIFT 31
Is the register name and the bit name prefix really different or is this a typo?
+static int __wait_for_bus_idle(struct bcm_iproc_i2c_dev *iproc_i2c)
A bcm_iproc_i2c prefix would be nice here.
quoted hunk ↗ jump to hunk
+static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *iproc_i2c, + struct i2c_msg *msg, u8 *addr) +{ + + if (msg->flags & I2C_M_TEN) { + dev_err(iproc_i2c->device, "no support for 10-bit address\n"); + return -EINVAL; + } + + *addr = (msg->addr << 1);
You can also drop the parentheses.
quoted hunk ↗ jump to hunk
+ switch (val) { + case M_CMD_STATUS_SUCCESS: + return 0; + + case M_CMD_STATUS_LOST_ARB: + dev_err(iproc_i2c->device, "lost bus arbitration\n"); + return -EREMOTEIO; + + case M_CMD_STATUS_NACK_ADDR: + dev_err(iproc_i2c->device, "NAK addr:0x%02x\n", + iproc_i2c->msg->addr); + return -EREMOTEIO; + + case M_CMD_STATUS_NACK_DATA: + dev_err(iproc_i2c->device, "NAK data\n"); + return -EREMOTEIO; + + case M_CMD_STATUS_TIMEOUT: + dev_err(iproc_i2c->device, "bus timeout\n"); + return -ETIMEDOUT; + + default: + dev_err(iproc_i2c->device, "unknown error code=%d\n", val); + return -EREMOTEIO; + } + + return -EREMOTEIO;
This is not reached.
quoted hunk ↗ jump to hunk
+} + +static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, + struct i2c_msg *msg) +{ + int ret, i; + 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) {
Is the < 1 a hardware or a software limitation? That means your driver doesn't support I2C_SMBUS_QUICK which is used for example by i2cdetect.
quoted hunk ↗ jump to hunk
+ dev_err(iproc_i2c->device, + "supported data length is 1 - %u bytes\n", + M_TX_RX_FIFO_SIZE - 1); + return -EINVAL; + } + + iproc_i2c->msg = msg;
Can it happen that iproc_i2c->msg still holds an uncompleted message here or is this serialized by the core? Wolfram? Either here something like: if (iproc_i2c->msg) return -EBUSY; and iproc_i2c->msg = NULL; when a transfer is completed is needed, or the respective code can be dropped from other drivers (e.g. i2c-efm32). On the other hand .msg is only used in bcm_iproc_i2c_check_status() to give a diagnostic message. Maybe you can drop .msg and instead give it as an additional parameter to bcm_iproc_i2c_check_status().
quoted hunk ↗ jump to hunk
+ ret = __wait_for_bus_idle(iproc_i2c); + if (ret) + return ret;
I would still prefer to have something like: if (bcm_iproc_i2c_bus_busy()) return -EBUSY; instead of a tight loop here.
quoted hunk ↗ jump to hunk
+ ret = bcm_iproc_i2c_format_addr(iproc_i2c, msg, &addr); + if (ret) + return ret; + + /* load slave address into the TX FIFO */ + writel(addr, iproc_i2c->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;
What happens if you don't mark this last byte? Could this be used to support transfers bigger than the fifo size?
quoted hunk ↗ jump to hunk
+ /* + * Enable the "start busy" interrupt, which will be triggered after + * the transaction is done, i.e., the internal start_busy bit
s/\.,/./ I think
+ * transitions from 1 to 0
s/$/./
quoted hunk ↗ jump to hunk
+ */ + writel(1 << IE_M_START_BUSY_SHIFT, iproc_i2c->base + IE_OFFSET); + + /* + * Now we can activate the transfer. For a read operation, specify the + * number of bytes to read
s/$/./
quoted hunk ↗ jump to hunk
+ */ + 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, iproc_i2c->base + M_CMD_OFFSET); + + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left);
When the interrupt fires here after the complete timed out and before you disable the irq you still throw the result away.
quoted hunk ↗ jump to hunk
+ + /* disable all interrupts */ + writel(0, iproc_i2c->base + IE_OFFSET); + + if (!time_left) { + dev_err(iproc_i2c->device, "transaction times out\n");
s/times/timed/
quoted hunk ↗ jump to hunk
+static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
Note that I2C_FUNC_SMBUS_EMUL includes I2C_FUNC_SMBUS_QUICK, so your driver claims to support transfers of length 0.
quoted hunk ↗ jump to hunk
+static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) +{ + unsigned int bus_speed, speed_bit; + u32 val; + int ret = of_property_read_u32(iproc_i2c->device->of_node, + "clock-frequency", &bus_speed); + if (ret < 0) { + dev_err(iproc_i2c->device, + "missing clock-frequency property\n"); + return -ENODEV;
Is a missing property the only situation where of_property_read_u32 returns an error? Would it be sane to default to 100 kHz?
quoted hunk ↗ jump to hunk
+static int bcm_iproc_i2c_remove(struct platform_device *pdev) +{ + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev); + + i2c_del_adapter(&iproc_i2c->adapter);
You need to free the irq before i2c_del_adapter.
quoted hunk ↗ jump to hunk
+ free_irq(iproc_i2c->irq, iproc_i2c); + bcm_iproc_i2c_disable(iproc_i2c); + + return 0; +} + +static const struct of_device_id bcm_iproc_i2c_of_match[] = { + {.compatible = "brcm,iproc-i2c",},
Not sure this is specified to be a must, but I'd add spaces after { and
before }.
+ {},It's a good habit to write this as
{ /* sentinel */ }
without trailing comma.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |