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
>

2016-03-14 09:22:12

by Jan Glauber

[permalink] [raw]

Subject: Re: [PATCH v3 03/14] i2c-octeon: Cleanup resource allocation code

On Sat, Mar 12, 2016 at 04:37:15PM +0100, Wolfram Sang wrote:
> On Mon, Mar 07, 2016 at 04:10:46PM +0100, Jan Glauber wrote:
> > From: David Daney <[email protected]>
> >
> > Use resource start and size directly.
> >
> > Signed-off-by: David Daney <[email protected]>
> > Signed-off-by: Jan Glauber <[email protected]>
>
> What about converting to devm_ioremap_resource()?
>

Nice. I'll use that.

2016-03-14 12:45:28

by Jan Glauber

[permalink] [raw]

Subject: Re: [PATCH v3 05/14] i2c-octeon: Make adapter timeout tunable

On Sat, Mar 12, 2016 at 04:46:12PM +0100, Wolfram Sang wrote:
> On Mon, Mar 07, 2016 at 04:10:48PM +0100, Jan Glauber wrote:
> > From: Peter Swain <[email protected]>
> >
> > Make the i2c adapter timeout a module parameter to allow upper-level
> > target device drivers to retry with their own logic before their own
> > timeouts abort operations.
> >
> > For example, at24 eeprom driver retries for 25ms when -EAGAIN
> > indicates that an eeprom has gone unresponsive while committing
> > a newly written page (5ms on typical devices).
> >
> > Signed-off-by: Peter Swain <[email protected]>
> > Signed-off-by: Jan Glauber <[email protected]>
>
> i2c-dev has IOCTLs for timeout and retries. Can't you use those?
>

Yes, with these IOCTLs we don't need to add a module parameter
so I'll drop that.

2016-03-14 17:37:00

by Swain, Peter

[permalink] [raw]

Subject: Re: [PATCH v3 05/14] i2c-octeon: Make adapter timeout tunable

Agreed, this timeout modparam can go, it serves no purpose which can't be achieved with existing ioctl.

It was introduced during debug to reduce spurious error count while hunting an issue finally resolved by the 80uS IFLG re-poll. But the same effect is achieved by (for example) the at24 driver's retry at 25mS timeout. The issue there is not timeout _within_ command, but failure to respect a 25mS post-write recovery time before next access to the at24, and the existing at24-level retry works well (though delaying the presentation of the next at24 transaction _within_ the at24 driver would have lead to less bus-level churn).
Leaving this adapter tuning out affects nothing but the (normally invisible) adapter-level retry count, which is better addressed within the target device's code, like at24 does. Default is ioctl-tweakable, so users wanting fine tuning have a mechanism.

-pete

________________________________________
From: Jan Glauber <[email protected]>
Sent: Monday, March 14, 2016 5:45 AM
To: Wolfram Sang
Cc: [email protected]; [email protected]; Daney, David; Peter Swain
Subject: Re: [PATCH v3 05/14] i2c-octeon: Make adapter timeout tunable

On Sat, Mar 12, 2016 at 04:46:12PM +0100, Wolfram Sang wrote:
> On Mon, Mar 07, 2016 at 04:10:48PM +0100, Jan Glauber wrote:
> > From: Peter Swain <[email protected]>
> >
> > Make the i2c adapter timeout a module parameter to allow upper-level
> > target device drivers to retry with their own logic before their own
> > timeouts abort operations.
> >
> > For example, at24 eeprom driver retries for 25ms when -EAGAIN
> > indicates that an eeprom has gone unresponsive while committing
> > a newly written page (5ms on typical devices).
> >
> > Signed-off-by: Peter Swain <[email protected]>
> > Signed-off-by: Jan Glauber <[email protected]>
>
> i2c-dev has IOCTLs for timeout and retries. Can't you use those?
>

Yes, with these IOCTLs we don't need to add a module parameter
so I'll drop that.