Re: [PATCH] i2c-designware: add i2c gpio recovery option

From: Andy Shevchenko
Date: Thu May 11 2017 - 09:53:21 EST


On Thu, 2017-05-11 at 09:24 +0800, Phil Reid wrote:
> G'day Andy,
>
> Thanks for the review.

You're welcome, just don't forget to remove the parts that are out of
scope and/or you agree with.

> On 10/05/2017 21:13, Andy Shevchenko wrote:
> > On Wed, 2017-05-10 at 13:57 +0200, Tim Sander wrote:

> > > +static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct i2c_adapter *adap)
> > > +{
> > > +ÂÂÂÂÂÂÂstruct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> > > +
> > > +ÂÂÂÂÂÂÂdev->gpio_scl = devm_gpiod_get_optional(dev->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"scl",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂGPIOD_OUT_HIGH);
> > > +ÂÂÂÂÂÂÂif (IS_ERR_OR_NULL(dev->gpio_scl))
> >
> > This is wrong. You should not use this macro in most cases. And
> > especially it breaks the logic behind _optional().
>
> My logic here was that if the gpio is optional return null we return
> 0.

Why?!

_optional() *implies* that all rest calls will go fine and do nothing.

> which is an okay status.
> But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've
> never
> quite wrapped my head around why that's the case.
>
> But the probe function only bails out if this returns EPROBE_DEFER.
> Not sure that's the best approach

You need something like

desc = devm_gpiod_get_optional(...);
if (IS_ERR(desc))
return PTR_ERR(desc);

> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn PTR_ERR(dev->gpio_sda);
> > > +ÂÂÂÂÂÂÂrinfo->scl_gpio = desc_to_gpio(dev->gpio_scl);
> > > +ÂÂÂÂÂÂÂrinfo->sda_gpio = desc_to_gpio(dev->gpio_sda);
> >
> > Why?!
>
> ÂFrom my first attempt, didn't remove it from the example I sent.
>
> We could change i2c_init_recovery to something like the following
> then the gpio set / getter could use the default functions.
> Not sure the code is completely correct but hopefully you get the
> concept.
>
> static void i2c_init_recovery(struct i2c_adapter *adap)
> {
> struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> char *err_str;
>
> if (!bri)
> return;
>
> if (!bri->recover_bus) {
> err_str = "no recover_bus() found";
> goto err;
> }
>
> /* bail out if either no gpio or no set/get callback. */
> if (!gpio_is_valid(bri->scl_gpio) && (!bri->set_scl || !bri-
> >get_scl)) {
> if (!gpio_is_valid(bri->scl_gpio))
> err_str = "invalid SCL gpio";
> else
> err_str = "no {get|set}_scl() found";
> goto err;
> }
>
> if (gpio_is_valid(bri->sda_gpio))
> bri->get_sda = get_sda_gpio_value;
>
> if (gpio_is_valid(bri->scl_gpio)) {
> bri->get_scl = get_scl_gpio_value;
> bri->set_scl = set_scl_gpio_value;
> }
>
> return;
> Â err:
> dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
> adap->bus_recovery_info = NULL;
> }


I have briefly looked at the current code.
So, my suggestion is to switch to gpio descriptors in current code and
then rebase your stuff on top.

I wouldn't encourage people to continue using legacy GPIO API.

--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy