Re: [PATCH V6 1/2] i2c/adapter: Add bus recovery infrastructure

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

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

On Sun, Nov 25, 2012 at 09:34:46AM +0530, Viresh Kumar wrote:
> Hi Wolfram,
> 
> Thanks for sharing your opinion.
> 
> On 25 November 2012 02:29, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > On Thu, Oct 04, 2012 at 04:34:53PM +0530, Viresh Kumar wrote:
> 
> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> 
> >> +static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
> >> +{
> >> +     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> >> +     struct device *dev = &adap->dev;
> >> +     int ret = 0;
> >> +
> >> +     if (bri->get_gpio) {
> >> +             ret = bri->get_gpio(bri->scl_gpio);
> >> +             if (ret) {
> >> +                     dev_warn(dev, "scl get_gpio: %d\n", bri->scl_gpio);
> >
> > This warning is probably not very helpful to a user.
> 
> This is what i thought about it: This is a platform specific routine and
> most probably it will fail the first time this code is ever hit and so a
> warning would be very helpful for them.
> 
> But, now i think platforms should have these prints in their get_gpio
> implementations. And we can make it have return type void.

The warning could be made helpful if properly rephrased. Actually, I
think returning an err is OK here, but if there is a warning it should
be a proper human readable sentence.

> >> +             if (unlikely(ret ||
> >
> > Since the unlikely() are not in hot-paths, you probably better skip
> > them.
> 
> It will be removed as get_gpio() would have return type void.

Keep in mind that it was not the only one. I think V7 had still one
left.

> >> + * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
> >> + *   it became high or not. Only required if recover_bus == NULL.
> >
> > Does a user really need to set this?
> 
> This was done for platforms and i2c controllers which don't have configuration
> to control scl line. So i still feel we need it.

It might be needed internally, still not sure if a user needs to set it.
If so, there should be more documentation when she wants to use this.

> >> + * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio and
> >> + *   scl type recovery.
> >
> > Does a user really need this? We could probably use something close to
> > 100kHz always?
> 
> Not sure. Doesn't it depend on the current clk rate of controller ?
> If not then yes it can be 100 KHz

It should be OK to drive the bus at x kHz and recover from it at y kHz
(as long as y < 100).

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 --]