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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html