On 13/10/23 22:34, Andi Shyti wrote:
> Hi Chris,
>
> ...
>
>> static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>> @@ -1083,6 +1084,10 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>> if (drv_data->irq < 0)
>> return drv_data->irq;
>>
>> + drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
>> + if (IS_ERR(drv_data->reset_gpio))
>> + return PTR_ERR(drv_data->reset_gpio);
>>
>> if this optional why are we returning in case of error?
>>
>> gpiod_get_optional() will return NULL if the property is not present. The main
>> error I care about here is -EPROBE_DEFER but I figure other errors are also
>> relevant. This same kind of pattern is used in other drivers.
> we already discussed about this, I don't have a strong opinion,
> you can leave it as it is... I recon this is a matter of pure
> taste.
I think in this case it would actually make things uglier because I'd
have to check for -EPROBE_DEFER. So something like
drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset",
GPIOD_OUT_HIGH);
if (IS_ERR(drv_data->reset_gpio) && PTR_ERR(drv_data->reset_gpio)
== -EPROBE_DEFER)
return PTR_ERR(drv_data->reset_gpio);
else
drv_data->reset_gpio = NULL;
I could probably come up with something less ugly with a local variable
or two but nothing as tidy as just returning on error.
>
> Would you just mind adding an error message using
> dev_err_probe()?
Yep sure. Will include in the next round.
