Re: [PATCH v5] i2c: add driver for Rockchip RK3xxx SoC I2C adapter

[Wolfram Sang]

Hi,

On Sat, Jun 07, 2014 at 07:36:19PM +0200, Max Schwarz wrote:
> Driver for the native I2C adapter found in Rockchip RK3xxx SoCs.
> 
> Configuration is only possible through devicetree. The driver is
> interrupt driven and supports the I2C_M_IGNORE_NAK mangling bit.
> 
> Signed-off-by: Max Schwarz <max.schwarz@xxxxxxxxx>

Checking if the spinlock is needed would be nice, but we can also fix
this later. Besides this, my code-checkers say:

    CHECKPATCH
CHECK: Logical continuations should be on the previous line
#615: FILE: drivers/i2c/busses/i2c-rk3x.c:470:
+	if (num >= 2 && msgs[0].len < 4
+	    && !(msgs[0].flags & I2C_M_RD)

CHECK: Logical continuations should be on the previous line
#616: FILE: drivers/i2c/busses/i2c-rk3x.c:471:
+	    && !(msgs[0].flags & I2C_M_RD)
+	    && (msgs[1].flags & I2C_M_RD)) {

    SPARSE
drivers/i2c/busses/i2c-rk3x.c:173:20: warning: Using plain integer as NULL pointer
drivers/i2c/busses/i2c-rk3x.c:664:45: warning: Using plain integer as NULL pointer
drivers/i2c/busses/i2c-rk3x.c:735:9: warning: cast removes address space of expression
    SMATCH
drivers/i2c/busses/i2c-rk3x.c:592 rk3x_i2c_xfer() error: double unlock 'spin_lock:&i2c->lock'
    SPATCH
drivers/i2c/busses/i2c-rk3x.c:366:1-10: WARNING: Assignment of bool to 0/1
drivers/i2c/busses/i2c-rk3x.c:187:2-11: WARNING: Assignment of bool to 0/1
  CC      drivers/i2c/busses/i2c-rk3x.o
drivers/i2c/busses/i2c-rk3x.c: In function 'rk3x_i2c_irq':
drivers/i2c/busses/i2c-rk3x.c:336:15: warning: 'val' may be used uninitialized in this function [-Wuninitialized]
drivers/i2c/busses/i2c-rk3x.c:321:15: note: 'val' was declared here

The smatch warning may be a false positive, please check. The gcc warning is a
false positive but to prevent people from sending fixup patches, please do
something like commit a55b44ac3fe0.

> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> new file mode 100644
> index 0000000..7a430f4
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -0,0 +1,769 @@
> +/*
> + * Driver for I2C adapter in Rockchip RK3xxx SoC
> + *
> + * Max Schwarz <max.schwarz@xxxxxxxxx>
> + * based on the patches by Rockchip Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +

No empty line here.

> +#include <linux/i2c.h>
> +#include <linux/time.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/spinlock.h>
> +#include <linux/clk.h>
> +#include <linux/wait.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>

Do you really need delay.h and time.h? Look like leftovers to me.

> +/**
> + * Generate a START condition, which triggers a REG_INT_START interrupt.
> + */
> +static void rk3x_i2c_start(struct rk3x_i2c *i2c)
> +{
> +	u32 val;
> +
> +	dev_dbg(i2c->dev, "start\n");

I think this debug could go now, or?

> +
> +	rk3x_i2c_clean_ipd(i2c);
> +	i2c_writel(i2c, REG_INT_START, REG_IEN);
> +
> +	/* enable adapter with correct mode, send START condition */
> +	val = REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
> +
> +	/* if we want to react to NACK, set ACTACK bit */
> +	if (!(i2c->msg->flags & I2C_M_IGNORE_NAK))
> +		val |= REG_CON_ACTACK;
> +
> +	i2c_writel(i2c, val, REG_CON);
> +}

...

> +
> +static void rk3x_i2c_prepare_read(struct rk3x_i2c *i2c)
> +{
> +	unsigned int len = i2c->msg->len - i2c->processed;
> +	u32 con;
> +
> +	con = i2c_readl(i2c, REG_CON);
> +
> +	/*
> +	 * The hw can read up to 32 bytes at a time. If we need more than one
> +	 * chunk, send an ACK after the last byte of the current chunk.
> +	 */
> +	if (unlikely(len > 32)) {
> +		len = 32;
> +		con &= ~REG_CON_LASTACK;
> +	} else {
> +		con |= REG_CON_LASTACK;
> +	}

This is a bit confusing also due to the description of this bit:
"/* 1: do not send ACK after last receive */"

It means not only don't send ACK, but merely do send NACK (to signal end of
read). I though "not send ack" means clock stretching so I was wondering. Maybe
needs some rewording.

> +
> +	/* make sure we are in plain RX mode if we read a second chunk */
> +	if (i2c->processed != 0) {
> +		con &= ~REG_CON_MOD_MASK;
> +		con |= REG_CON_MOD(REG_CON_MOD_RX);
> +	}
> +
> +	i2c_writel(i2c, con, REG_CON);
> +	i2c_writel(i2c, len, REG_MRXCNT);
> +}
> +

Thanks, I think we're very close to go...

   Wolfram

Attachment: signature.asc
Description: Digital signature