Re: [PATCH_V2 2/2] i2c: jz4780: Add i2c bus controller driver for Ingenic JZ4780

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 4315 bytes --]

Hi,

mostly looking good. checkpatch.pl --strict has some whitespaces and
continuation issues, please fix them.

And sparse rightfully says:

drivers/i2c/busses/i2c-jz4780.c:510:51: warning: cast truncates bits from constant value (100 becomes 0)

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 22da9c2..50b0c91 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -909,6 +909,15 @@ config I2C_RCAR
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-rcar.
>  
> +config I2C_JZ4780
> +	tristate "JZ4780 I2C controller interface support"
> +	depends on MACH_JZ4780

|| COMPILE_TEST ?

> +	help
> +	 If you say yes to this option, support will be included for the
> +	 Ingenic JZ4780 I2C controller.
> +
> +	 If you don't know what to do here, say N.
> +

Sorting looks broken.

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/time.h>
> +#include <linux/sched.h>
> +#include <linux/errno.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/bitops.h>
> +#include <linux/completion.h>

Please sort the includes, that prevents duplicates.

> +static inline unsigned short jz4780_i2c_readw(struct jz4780_i2c *i2c,
> +					unsigned long offset)
> +{
> +	return readw(i2c->iomem + offset);
> +}
> +
> +static inline void jz4780_i2c_writew(struct jz4780_i2c *i2c,
> +				     unsigned long offset, unsigned short val)
> +{
> +	writew(val, i2c->iomem + offset);
> +}

I don't think these functions add much compared to readw/writew, but you
can keep them if you really want.

> +			do {
> +				i2c_sta = jz4780_i2c_readw(i2c, JZ4780_I2C_STA);
> +				if ((i2c_sta & JZ4780_I2C_STA_TFNF)
> +					      && (i2c->wt_len > 0)) {
> +					data = *i2c->wbuf;
> +					data &= (~JZ4780_I2C_DC_READ);
> +					jz4780_i2c_writew(i2c, JZ4780_I2C_DC,
> +							  data);
> +					i2c->wbuf++;
> +					i2c->wt_len--;
> +				} else {
> +					break;
> +				}
> +			} while (1);

do/while(1) with else break;

That can be simplified with a while loop?

> +static void jz4780_i2c_txabrt(struct jz4780_i2c *i2c, int src)
> +{
> +	int i;
> +
> +	dev_err(&i2c->adap.dev, "txabrt: 0x%08x\n", src);
> +	dev_err(&i2c->adap.dev, "device addr=%x\n",
> +				  jz4780_i2c_readw(i2c, JZ4780_I2C_TAR));
> +	dev_err(&i2c->adap.dev, "send cmd count:%d  %d\n",
> +				  i2c->cmd, i2c->cmd_buf[i2c->cmd]);
> +	dev_err(&i2c->adap.dev, "receive data count:%d  %d\n",
> +				  i2c->cmd, i2c->data_buf[i2c->cmd]);
> +
> +	for (i = 0; i < 16; i++) {
> +		if (src & BIT(i))
> +			dev_info(&i2c->adap.dev, "I2C TXABRT[%d]=%s\n",
> +						 i, jz4780_i2c_abrt_src[i]);
> +	}

Isn't that more dev_dbg?

> +	if (!timeout) {
> +		dev_err(&i2c->adap.dev, "irq read timeout\n");
> +		dev_err(&i2c->adap.dev, "send cmd count:%d  %d\n",
> +					  i2c->cmd, i2c->cmd_buf[i2c->cmd]);
> +		dev_err(&i2c->adap.dev, "receive data count:%d  %d\n",
> +					  i2c->cmd, i2c->data_buf[i2c->cmd]);

Same here with the last two messages?

> +	if (msg->addr != jz4780_i2c_readw(i2c, JZ4780_I2C_TAR)) {
> +		ret = jz4780_i2c_set_target(i2c, msg->addr);
> +		if (ret) {
> +			dev_err(&i2c->adap.dev, "I2C set target failed\n");
> +			goto out;
> +		}

set_target already has an error message. And set_target should return
some errno, not -1.

> +	}
> +	for (i = 0; i < count; i++, msg++) {
> +		if (msg->flags & I2C_M_RD)
> +			ret = jz4780_i2c_xfer_read(i2c, msg->buf, msg->len,
> +						   count, i);
> +		else
> +			ret = jz4780_i2c_xfer_write(i2c, msg->buf, msg->len,
> +						    count, i);
> +
> +		if (ret) {
> +			dev_err(&i2c->adap.dev, "I2C xfer failed\n");
> +			ret = -EAGAIN;

No use error codes like described in Documentation/i2c/fault-codes.
-EAGAIN has only one well defined meaning.

> +	i2c->adap.owner		= THIS_MODULE;
> +	i2c->adap.algo		= &jz4780_i2c_algorithm;
> +	i2c->adap.algo_data	= i2c;
> +	i2c->adap.timeout	= 5;

5 jiffies? That's probably not what you meant. Why not leave the
default?

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]