Re: [PATCH v3 05/12] PCI: aardvark: Issue PERST via GPIO

["Pali Rohár" <pali@kernel.org>]

On Friday 24 April 2020 12:05:29 Rob Herring wrote:
> On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> > @@ -1046,6 +1071,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
> >         }
> >         pcie->root_bus_nr = bus->start;
> >
> > +       pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
> > +                                                      "reset-gpios", 0,
> > +                                                      GPIOD_OUT_LOW,
> > +                                                      "pcie1-reset");
> > +       ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);
> > +       if (ret) {
> > +               if (ret == -ENOENT) {
> > +                       pcie->reset_gpio = NULL;
> > +               } else {
> > +                       if (ret != -EPROBE_DEFER)
> > +                               dev_err(dev, "Failed to get reset-gpio: %i\n",
> > +                                       ret);
> > +                       return ret;
> > +               }
> > +       }
> 
> I believe all this can be replaced with devm_gpiod_get_optional.

I'm looking at the devm_gpiod_get_optional() code and it calls
fwnode_gpiod_get_index() which mangle passed gpio name by appending
suffix from array gpio_suffixes[]. And then mangled name is passed to
fwnode_get_named_gpiod(). At the end devm_gpiod_get_optional() handles
-ENOENT and transform it to NULL.

So via devm_gpiod_get_optional() I cannot request for "reset-gpios" DT
property directly, but I need to specify implicit name "reset" due to
appending suffix from gpio_suffixes[] array. And the only benefit is
that I do not need to handle ret == -ENOENT state.

Handling of -EPROBE_DEFER is still needed in aardvark driver.

For me it makes usage of devm_gpiod_get_optional() in this case harder
to debug code in future. As I would not be able to find a place which
reads 'reset-gpios' DT property via 'git grep reset-gpios'.

So it is really useful for this particular case to use
devm_gpiod_get_optional() instead of devm_gpiod_get_from_of_node().