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.shevche...@linux.intel.com>
Intel Finland Oy
