Re: [PATCH 2/2] i2c: mv64xxx: add an optional reset-gpios property

[Chris Packham]

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.