Hi,
On 18/11/21 10:00, Lee Jones wrote:
> 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.
>
As the author of the code to blame, I wrote this patch, but just needed
a little time to test it before sending:
lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
GPIOD_OUT_LOW);
if (IS_ERR(lp87565->reset_gpio))
return dev_err_probe(lp87565->dev, PTR_ERR(lp87565->reset_gpio),
"Failed getting reset GPIO");
if (lp87565->reset_gpio) {
...
I prefer to exit on any error as it would be either -EPROBE_DEFER of a
_real_ error (e.g. GPIO already in use). If there's no GPIO specified,
then devm_gpiod_get_optional() returns NULL and libgpio ignores NULL
pointers gracefully.
Would that work?
--
Luca
