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

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello,

On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote:
> +#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.

> +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.

> +	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.

> +}
> +
> +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.

> +		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().

> +	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.

> +	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?

> +	/*
> +	 * 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/$/./
> +	 */
> +	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/$/./

> +	 */
> +	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.
> +
> +	/* 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/

> +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.

> +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?

> +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.

> +	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/  |