Re: [bug report] mfd: lp87565: Handle optional reset pin

[Lee Jones <lee.jones@linaro.org>]

On Thu, 18 Nov 2021, Dan Carpenter wrote:

> On Thu, Nov 18, 2021 at 08:38:12AM +0000, Lee Jones wrote:
> > On Wed, 17 Nov 2021, Dan Carpenter wrote:
> > 
> > > Hello Luca Ceresoli,
> > > 
> > > The patch 50e4d7a2a667: "mfd: lp87565: Handle optional reset pin"
> > > from Feb 26, 2021, leads to the following Smatch static checker
> > > warning:
> > > 
> > > 	drivers/mfd/lp87565.c:76 lp87565_probe()
> > > 	warn: 'lp87565->reset_gpio' could be an error pointer
> > > 
> > > drivers/mfd/lp87565.c
> > >     46 static int lp87565_probe(struct i2c_client *client,
> > >     47                          const struct i2c_device_id *ids)
> > >     48 {
> > >     49         struct lp87565 *lp87565;
> > >     50         const struct of_device_id *of_id;
> > >     51         int ret;
> > >     52         unsigned int otpid;
> > >     53 
> > >     54         lp87565 = devm_kzalloc(&client->dev, sizeof(*lp87565), GFP_KERNEL);
> > >     55         if (!lp87565)
> > >     56                 return -ENOMEM;
> > >     57 
> > >     58         lp87565->dev = &client->dev;
> > >     59 
> > >     60         lp87565->regmap = devm_regmap_init_i2c(client, &lp87565_regmap_config);
> > >     61         if (IS_ERR(lp87565->regmap)) {
> > >     62                 ret = PTR_ERR(lp87565->regmap);
> > >     63                 dev_err(lp87565->dev,
> > >     64                         "Failed to initialize register map: %d\n", ret);
> > >     65                 return ret;
> > >     66         }
> > >     67 
> > >     68         lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
> > >     69                                                       GPIOD_OUT_LOW);
> > >     70         if (IS_ERR(lp87565->reset_gpio)) {
> > >     71                 ret = PTR_ERR(lp87565->reset_gpio);
> > >     72                 if (ret == -EPROBE_DEFER)
> > >     73                         return ret;
> > > 
> > > Only "ret = -EPROBE_DEFER" is handled.  Other error pointer will lead to
> > > a crash.
> > > 
> > >     74         }
> > >     75 
> > > --> 76         if (lp87565->reset_gpio) {
> > 
> > I guess this should be:
> > 
> >     if (lp87565->reset_gpio >= 0)
> > 
> > If 0 is valid, or '>' if it's not.
> > 
> 
> lp87565->reset_gpio is a pointer so that's not going to work.

Ah, I assumed this was a GPIO #.  I missed the gpioD part.

> There are two options.  The first is to just "return ret;" for all
> errors.

No, don't do that.  This GPIO is optional.

> The second option is to say something like:
> 
> 	if (IS_ERR(lp87565->reset_gpio)) {
> 		ret = dev_err_probe(lp87565->dev, PTR_ERR(lp87565->reset_gpio),
> 				    "no GPIO error for ->reset_gpio");
> 		if (ret == -EPROBE_DEFER)
> 			return;
> 		lp87565->reset_gpio = NULL;
> 	}

Works for me.

Or:

    if (!IS_ERR(lp87565->reset_gpio))

Or stick it in the else.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog