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

[Andy Shevchenko]

On Fri, 2017-05-12 at 09:49 +0800, Phil Reid wrote:
> On 11/05/2017 21:53, Andy Shevchenko 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);
> > 
> 
> I found that continuing without the check on null results in a kernel
> panic for a dereferenced null pointer.
> So something in the gpiolib doesn't treat a null desc as optional.
> 
> It was probably this code:
> int desc_to_gpio(const struct gpio_desc *desc)
> {
> 	return desc->gdev->base + (desc - &desc->gdev->descs[0]);
> }
> 
> So perhaps this should return an invalid gpio number when desc == null
> 
> I don't know what the intents are, so don't know if its a "bug" orÂÂby
> design.

No, it doesn't seem like a bug to me. If you don't use legacy API it
would be fine.

Summarize this discussion, I would rather go this way:
1) introduce gpiod_ based API in I2C core for recovering;
2) rebase your patch on top of that change.

It would be beneficial in a long term for everyone.

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