Re: [PATCH] i2c: add driver for Freescale i.MX28

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

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

Hi Ben,

thanks for the review.

> > +#include <mach/common.h>
> 
> do we actually use anything from the above?

Yes, mxs_reset_block() is from there. The original driver resets the
whole core after some error conditions. I cannot verify if it is
needed or not, because I sadly cannot create these error conditions
here.

> > +static void mxs_i2c_pioq_setup_write(struct mxs_i2c_dev *i2c,
> > +				    u8 addr, u8 *buf, int len, int flags)
> > +{
> > +	u32 data;
> > +	int i, shifts_left;
> > +
> > +	data = MXS_CMD_I2C_WRITE | MXS_I2C_CTRL0_XFER_COUNT(len + 1) | flags;
> > +	__raw_writel(data, i2c->regs + MXS_I2C_QUEUECMD);
> > +
> > +	/* Start with address, then append buffer */
> > +	data = ((addr << 1) | I2C_SMBUS_WRITE) << 24;
> > +
> > +	for (i = 0; i < len; i++) {
> > +		data >>= 8;
> > +		data |= buf[i] << 24;
> > +		if ((i & 3) == 2)
> > +			__raw_writel(data, i2c->regs + MXS_I2C_DATA);
> > +	}
> > +
> > +	shifts_left = 24 - (i & 3) * 8;
> > +	if (shifts_left)
> > +		__raw_writel(data >> shifts_left, i2c->regs + MXS_I2C_DATA);
> > +}
> 
> I'm going to have a think about the above, since it looks rather
> hard to understand. Any chance of re-writing this?

Will proper comments do? Like

/*
 * We have to copy the slave address (u8) and buffer (lots of u8) into
 * the data register (u32). To achieve that, the u8 are put into the
 * MSBs of 'data' which is then shifted for the next u8. When
 * apropriate, 'data' is written to MXS_I2C_DATA.
 */

? I tried a few versions and liked this best so far...

> 
> > +static int mxs_i2c_wait_for_data(struct mxs_i2c_dev *i2c)
> > +{
> > +	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
> > +
> > +	while (__raw_readl(i2c->regs + MXS_I2C_QUEUESTAT)
> > +			& MXS_I2C_QUEUESTAT_RD_QUEUE_EMPTY) {
> > +			if (time_after(jiffies, timeout))
> > +				return -ETIMEDOUT;
> > +			cond_resched();
> > +	}
> > +
> > +	return 0;
> > +}
> 
> surely there's a relevant wait_event here?

Have to check...

> > +static int __devinit mxs_i2c_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct mxs_i2c_dev *i2c;
> > +	struct i2c_adapter *adap;
> > +	struct resource *res;
> > +	resource_size_t res_size;
> > +	int err, irq;
> > +
> > +	i2c = devm_kzalloc(dev, sizeof(struct mxs_i2c_dev), GFP_KERNEL);
> > +	if (!i2c)
> > +		return -ENOMEM;
> 
> Possibly be worth printing an error.

Do you insist on it? I'd think that

[    0.580000] mxs-i2c: probe of mxs-i2c.0 failed with error -12

is telling enough?


> > +	i2c->regs = devm_ioremap_nocache(dev, res->start, res_size);
> > +	if (!i2c->regs)
> > +		return -EBUSY;
> 
> Maybe there should be a devm request to do all of the above in one go.

"should" == would be nice if you make one

  or

"should" == have a look for one (which I couldn't find)


The rest you mentioned is already fixed here.

Regards,

   Wolfram

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

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