Re: [PATCH] i2c: Support for Netlogic XLR/XLS I2C controller.

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

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

On Thu, Jan 19, 2012 at 08:12:51PM +0530, Jayachandran C. wrote:
> From: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> 
> Add support for the intergrated I2C controller on Netlogic
> XLR/XLS MIPS SoC.
> 
> The changes are to add a new file i2c/buses/i2c-xlr.c, containing the
> i2c bus implementation, and to update i2c/buses/{Kconfig,Makefile} to
> add the CONFIG_I2C_XLR option.
> 
> Signed-off-by: Ganesan Ramalingam <ganesanr-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

Please say [PATCH V3] instead of just [PATCH], this helps reviewing.
Also, start a new mail-thread, it might get too messy if 10 different
versions get comments.

> +static int xlr_i2c_tx(struct xlr_i2c_private *priv,  u16 len,
> +	u8 *buf, u16 addr)
> +{
> +	struct i2c_adapter *adap = &priv->adap;
> +	u32 i2c_status;
> +	unsigned long timeout, stoptime;
> +	int pos;
> +	u8 offset, byte;
> +
> +	offset = buf[0];
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_ADDR, offset);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_DEVADDR, addr);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_CFG, XLR_I2C_CFG_ADDR);
> +	xlr_i2c_wreg(priv->iobase, XLR_I2C_BYTECNT, len - 1);
> +
> +	timeout = usecs_to_jiffies(XLR_I2C_TIMEOUT);
> +	stoptime = jiffies + timeout;
> +	pos = 1;
> +retry:
> +	if (len == 1) {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_ND);
> +	} else {
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, buf[pos]);
> +		xlr_i2c_wreg(priv->iobase, XLR_I2C_STARTXFR,
> +				XLR_I2C_STARTXFR_WR);
> +	}
> +
> +	while (1) {

If you get scheduled away here and return after stoptime, you won't have
checked the status even once. Do something like

		checktime = jiffies;
		i2c_status = ...
		/* check error bits */
		if time_after(checktime, stoptime)
			return -ETIMEDOUT;

An interrupt would be much better, of course.

> +		if (time_after(jiffies, stoptime)) {
> +			dev_err(&adap->dev, "I2C transmit timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		i2c_status = xlr_i2c_rdreg(priv->iobase, XLR_I2C_STATUS);
> +
> +		if (i2c_status & XLR_I2C_SDOEMPTY) {
> +			pos++;
> +			/* need to do a empty dataout after the last byte */
> +			byte = (pos < len) ? buf[pos] : 0;
> +			xlr_i2c_wreg(priv->iobase, XLR_I2C_DATAOUT, byte);
> +
> +			/* reset timeout on successful xmit */
> +			stoptime = jiffies + timeout;
> +		}
> +
> +		if (i2c_status & XLR_I2C_ARB_STARTERR)
> +			goto retry;
> +
> +		if (i2c_status & XLR_I2C_ACK_ERR)
> +			return -EIO;
> +
> +		if (i2c_status & XLR_I2C_BUS_BUSY)
> +			continue;
> +
> +		if (pos >= len)
> +			break;
> +	}
> +
> +	return 0;
> +}

Rest looks okay to me.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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