On Monday 05 September 2022 15:54:53 Dmitry Torokhov wrote:
> On Mon, Sep 05, 2022 at 09:00:46AM +0200, Pali Rohár wrote:
> > On Sunday 04 September 2022 23:30:58 Dmitry Torokhov wrote:
> > > I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> > > so that gpiolib can be cleaned a bit, so let's switch to the generic
> > > device property API.
> > >
> > > I believe that the only reason the driver, instead of the standard
> > > devm_gpiod_get_optional(), used devm_gpiod_get_from_of_node() is
> > > because it wanted to set up a pretty consumer name for the GPIO,
> >
> > IIRC consumer name is not used at all.
> >
> > The reason was to specify full name of DTS property, for easier
> > identification of the code. DTS property is "reset-gpios" but API
> > specify only "reset".
>
> I see. Do you want me to reset the patch with updated desctiption as to
> the reason devm_gpiod_get_from_of_node() was used?
I think it is fine. So add my:
Acked-by: Pali Rohár <pali@xxxxxxxxxx>
Anyway as another improvement for future I would suggest some API
function with _optional_ logic, so it could be used for more PCIe
controller drivers (e.g. also tegra) without need to reimplement
-ENOENT handling. It is really strange if for acquiring same PERST#
line via GPIO ("reset-gpios" DT property) are used more API functions
in different PCIe drivers.
> >
> > > and we now have a special API for that.
> > >
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > >
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index 4834198cc86b..4a8a4a8522cb 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -1856,20 +1856,19 @@ static int advk_pcie_probe(struct platform_device *pdev)
> > > return ret;
> > > }
> > >
> > > - pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
> > > - "reset-gpios", 0,
> > > - GPIOD_OUT_LOW,
> > > - "pcie1-reset");
> > > + pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > > 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;
> > > - }
> > > + if (ret != -EPROBE_DEFER)
> > > + dev_err(dev, "Failed to get reset-gpio: %i\n",
> > > + ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = gpiod_set_consumer_name(pcie->reset_gpio, "pcie1-reset");
> > > + if (ret) {
> > > + dev_err(dev, "Failed to set reset gpio name: %d\n", ret);
> > > + return ret;
> > > }
> > >
> > > ret = of_pci_get_max_link_speed(dev->of_node);
> > >
> > > --
> > > b4 0.10.0-dev-fc921
>
> Thanks.
>
> --
> Dmitry
