On Thu, Jan 19, 2012 at 08:12:51PM +0530, Jayachandran C. wrote:
> From: Ganesan Ramalingam <ganesanr@xxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Jayachandran C <jayachandranc@xxxxxxxxxxxxxxxxx>
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:
signature.asc
Description: Digital signature
