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

[Dan Carpenter <dan.carpenter@oracle.com>]

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.

There are two options.  The first is to just "return ret;" for all
errors.  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;
	}

regards,
dan carpenter