| Message ID | 20220903-gpiod_get_from_of_node-remove-v1-6-b29adfb27a6c@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Get rid of [devm_]gpiod_get_from_of_node() public APIs | expand |
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". > and we now have a special API for that. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > 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
On Mon, Sep 5, 2022 at 10:02 AM Pali Rohár <pali@kernel.org> 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. It's. The user space tools use it as a label. So, GPIO line can have "name" (this is provider specific) and "label" (which is consumer specific, i.o.w. how we use this line). ... > > + if (ret != -EPROBE_DEFER) > > + dev_err(dev, "Failed to get reset-gpio: %i\n", > > + ret); > > + return ret; I understand that in the input subsystem maintainer's hat you don't like dev_err_probe(), but it's a good case to have it here. ... > > + 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; > > } Ditto.
On Mon, Sep 05, 2022 at 01:47:41PM +0300, Andy Shevchenko wrote: > On Mon, Sep 5, 2022 at 10:02 AM Pali Rohár <pali@kernel.org> 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. > > It's. The user space tools use it as a label. So, GPIO line can have > "name" (this is provider specific) and "label" (which is consumer > specific, i.o.w. how we use this line). > > ... > > > > + if (ret != -EPROBE_DEFER) > > > + dev_err(dev, "Failed to get reset-gpio: %i\n", > > > + ret); > > > + return ret; > > I understand that in the input subsystem maintainer's hat you don't > like dev_err_probe(), but it's a good case to have it here. The driver currently does not use this API, so I elected not to introduce it in this series. Thanks,
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? > > > and we now have a special API for that. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > 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.
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@kernel.org> 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@gmail.com> > > > > > > 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
On Tue, Sep 06, 2022 at 01:10:10AM +0200, Pali Rohár wrote: > 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@kernel.org> > > Anyway as another improvement for future I would suggest some API > function with _optional_ logic, so it could be used for more PCIe I think we need to see how many are attaching reset lines to subnodes. If there are multiple then I agree we could add _optional. So far I see: dtor@dtor-ws:~/kernel/linux-next (gpiod_get_from_of_node-remove)$ git grep '"reset"' -- drivers/pci/controller/ drivers/pci/controller/cadence/pci-j721e.c: gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/dwc/pci-keystone.c: gpiod = devm_gpiod_get_optional(dev, "reset", drivers/pci/controller/dwc/pci-meson.c: mp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/dwc/pcie-dw-rockchip.c: rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", drivers/pci/controller/dwc/pcie-fu740.c: afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/dwc/pcie-intel-gw.c: pcie->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/dwc/pcie-keembay.c: pcie->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); drivers/pci/controller/dwc/pcie-qcom-ep.c: pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN); drivers/pci/controller/dwc/pcie-tegra194.c: pcie->pex_rst_gpiod = devm_gpiod_get(pcie->dev, "reset", GPIOD_IN); drivers/pci/controller/pci-aardvark.c: pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/pci-tegra.c: "reset", drivers/pci/controller/pcie-apple.c: "reset", 0, GPIOD_OUT_LOW, "PERST#"); drivers/pci/controller/pcie-mt7621.c: port->gpio_rst = devm_gpiod_get_index_optional(dev, "reset", slot, So majority have reset lines attached to the "main" node and thus can use devm_gpiod_get_optional(). Thanks.
On Mon, Sep 5, 2022 at 8:31 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> 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, > and we now have a special API for that. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
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);
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, and we now have a special API for that. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>