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

[Peter Rosin]

Hi!

2023-10-15 at 22:20, Chris Packham wrote:
> 
> 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.

I disagree with this, in my opinion it should just be:

      drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
      if (IS_ERR(drv_data->reset_gpio))
          return dev_err_probe(&pd->dev, PTR_ERR(drv_data->reset_gpio), ...);

My take is that optional gpios (or whatever) are optional because it is
optional to specify them in the devicetree (or whereever), but if the
optional item is indeed specified, it is just like any other error if
it is not working.

$0.02

Cheers,
Peter

>>
>> Would you just mind adding an error message using
>> dev_err_probe()?
> 
> Yep sure. Will include in the next round.
>