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

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello,

On Fri, Sep 28, 2012 at 09:11:34AM +0530, viresh kumar wrote:
> >> +             ndelay(delay);
> >> +             gpio_set_value(bri->scl_gpio, val);
> >> +
> >> +             /* break if sda got high, check only when scl line is high */
> >> +             if (!bri->skip_sda_polling && val)
> >> +                     if (gpio_get_value(bri->sda_gpio))
> >> +                             break;
> > I'm not sure it's worth to conditionally shortcut here. Either always or
> > never shortcut.
> 
> This was done, because few platforms may not have configuration bits to read
> status of sda line.. For them skip_sda_polling was required.
> 
> Whereas, others would need this to see if we can return early.
What is the upside of returning early? I'd say, just don't do it.

> >> + * @put_gpio: called after recover_bus() to get padmux configured for scl line
> >> + *   as scl. Only required if is_gpio_recovery == true.
> >> + * @scl_gpio: gpio number of the scl line. Only required if is_gpio_recovery ==
> >> + *   true.
> >> + * @sda_gpio: gpio number of the sda line. Only required if is_gpio_recovery ==
> >> + *   true and skip_sda_polling == false.
> >> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
> >> + *   GPIOF_OUT_INIT_LOW.
> > IMHO you should not make this configurable but use
> >
> >         GPIOF_OUT_INIT_HIGH | GPIOF_OPEN_DRAIN
> 
> Discussed here:
> http://www.spinics.net/lists/linux-i2c/msg07325.html
Why do you default to GPIOF_OUT_INIT_LOW? The idle state of scl is high.
Using low here introduces an additional clk pulse.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |