On Sat, Mar 12, 2016 at 04:35:00PM +0100, Wolfram Sang wrote:
> On Mon, Mar 07, 2016 at 04:10:45PM +0100, Jan Glauber wrote:
> > Cleanup only without functional change.
>
> I like most of the changes, but there are still some functional changes
> left.
>
> > -static int octeon_i2c_stop(struct octeon_i2c *i2c)
> > +/* send STOP to the bus */
> > +static void octeon_i2c_stop(struct octeon_i2c *i2c)
> > {
> > u8 data;
> >
> > @@ -266,11 +259,8 @@ static int octeon_i2c_stop(struct octeon_i2c *i2c)
> >
> > data = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
> >
> > - if (data != STAT_IDLE) {
> > + if (data != STAT_IDLE)
> > dev_err(i2c->dev, "%s: bad status(0x%x)\n", __func__, data);
> > - return -EIO;
> > - }
> > - return 0;
>
> Why this change? I don't know what SW_TWSI_EOP_TWSI_STAT tells, but this
> is surely not a cleanup.
It is no functional change because the return value of
octeon_i2c_stop() was ignored anyway. That said, the whole read-back of
the status and the dev_err looks like debug code to me and is removed
in a later patch anyway. I'll incoporate this in the cleanup,
so octeon_i2c_stop() will only do the write.
> > octeon_i2c_stop(i2c);
> >
> > - return (ret != 0) ? ret : num;
> > + return ret ? -EAGAIN : num;
>
> This is also not a cleanup and looks wrong. -EAGAIN is for lost
> arbitration only.
I agree, this looks like an over-simplification and drops
the EINVAL/ETIMEDOUT/EIO errors. I'll drop that completely.
> >
> > -static struct of_device_id octeon_i2c_match[] = {
> > - {
> > - .compatible = "cavium,octeon-3860-twsi",
> > - },
> > +static const struct of_device_id octeon_i2c_match[] = {
> > + { .compatible = "cavium,octeon-3860-twsi", },
>
> Nit: I'd prefer no tabs within the curly braces.
Agreed.
thanks, Jan
> Thanks,
>
> Wolfram
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
