Messages in this thread | | | Subject | Re: [PATCH] i2c-designware: add i2c gpio recovery option | From | Phil Reid <> | Date | Fri, 12 May 2017 09:49:49 +0800 |
| |
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.
-- Regards Phil Reid
| |