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:
signature.asc
Description: Digital signature
