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

[Andy Shevchenko]

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