mbox series

[0/2] i2c: mv64xxx: reset-gpios

Message ID 20231012035838.2804064-1-chris.packham@alliedtelesis.co.nz
Headers show
Series i2c: mv64xxx: reset-gpios | expand

Message

Chris Packham Oct. 12, 2023, 3:58 a.m. UTC 
This series adds the ability to associate a gpio with an I2C bus so that
downstream devices can be brought out of reset when the host controller is
probed.

Chris Packham (2):
  dt-bindings: i2c: mv64xxx: add reset-gpios property
  i2c: mv64xxx: add an optional reset-gpios property

 .../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml  |  3 +++
 drivers/i2c/busses/i2c-mv64xxx.c                      | 11 +++++++++++
 2 files changed, 14 insertions(+)

Comments

Andi Shyti Oct. 13, 2023, 9:34 a.m. UTC | #1 
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.

Would you just mind adding an error message using
dev_err_probe()?

Thanks,
Andi
Chris Packham Oct. 15, 2023, 8:20 p.m. UTC | #2 
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.