[PATCH v1 00/11] Get rid of [devm_]gpiod_get_from_of_node() public APIs

[PATCH v1 00/11] Get rid of [devm_]gpiod_get_from_of_node() public APIs

[Dmitry Torokhov]

I would like to stop exporting OF-specific [devm_]gpiod_get_from_of_node()
so that gpiolib can be cleaned a bit. We can do that by switching drivers
to use generic fwnode API ([devm_]fwnode_gpiod_get()). By doing so we open
the door to augmenting device tree and ACPI information through secondary
software properties (once we teach gpiolib how to handle those).

I hope that relevant maintainers will take patches through their trees and
then we could merge the last one some time after -rc1.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

---
Dmitry Torokhov (11):
      PCI: tegra: switch to using devm_fwnode_gpiod_get
      drm/tegra: switch to using devm_fwnode_gpiod_get
      mtd: rawnand: stm32_fmc2: switch to using devm_fwnode_gpiod_get()
      usb: phy: tegra: switch to using devm_gpiod_get()
      usb: gadget: udc: at91: switch to using fwnode_gpiod_get_index()
      PCI: aardvark: switch to using devm_gpiod_get_optional()
      PCI: apple: switch to using fwnode_gpiod_get_index()
      regulator: bd71815: switch to using devm_fwnode_gpiod_get()
      regulator: bd9576: switch to using devm_fwnode_gpiod_get()
      watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()
      gpiolib: of: remove [devm_]gpiod_get_from_of_node() APIs

 drivers/gpio/gpiolib-devres.c          | 55 ----------------------------------
 drivers/gpio/gpiolib-of.c              |  1 -
 drivers/gpio/gpiolib-of.h              | 13 ++++++++
 drivers/gpu/drm/tegra/output.c         | 10 +++----
 drivers/mtd/nand/raw/stm32_fmc2_nand.c |  5 ++--
 drivers/pci/controller/pci-aardvark.c  | 23 +++++++-------
 drivers/pci/controller/pci-tegra.c     |  9 +++---
 drivers/pci/controller/pcie-apple.c    |  4 +--
 drivers/regulator/bd71815-regulator.c  |  7 ++---
 drivers/regulator/bd9576-regulator.c   | 17 ++++++-----
 drivers/usb/gadget/udc/at91_udc.c      |  8 +++--
 drivers/usb/phy/phy-tegra-usb.c        | 14 ++++++---
 drivers/watchdog/bd9576_wdt.c          | 51 ++++++++++++++++++-------------
 include/linux/gpio/consumer.h          | 48 -----------------------------
 14 files changed, 96 insertions(+), 169 deletions(-)
---
base-commit: 7fd22855300e693668c3397771b3a2b3948f827a
change-id: 20220903-gpiod_get_from_of_node-remove-de3032fc01de

Best regards,
-- 
Dmitry

[PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get

[Dmitry Torokhov]

I would like to limit (or maybe even remove) use of
[devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned
a bit, so let's switch to the generic device property API. It may even
help with handling secondary fwnodes when gpiolib is taught to handle
gpios described by swnodes.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 8e323e93be91..929f9363e94b 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -2202,10 +2202,11 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		 * and in this case fall back to using AFI per port register
 		 * to toggle PERST# SFIO line.
 		 */
-		rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
-							     "reset-gpios", 0,
-							     GPIOD_OUT_LOW,
-							     label);
+		rp->reset_gpio = devm_fwnode_gpiod_get(dev,
+						       of_fwnode_handle(port),
+						       "reset",
+						       GPIOD_OUT_LOW,
+						       label);
 		if (IS_ERR(rp->reset_gpio)) {
 			if (PTR_ERR(rp->reset_gpio) == -ENOENT) {
 				rp->reset_gpio = NULL;

-- 
b4 0.10.0-dev-fc921

Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get

[Pali Rohár]

On Sunday 04 September 2022 23:30:53 Dmitry Torokhov wrote:
> I would like to limit (or maybe even remove) use of
> [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned
> a bit, so let's switch to the generic device property API. It may even
> help with handling secondary fwnodes when gpiolib is taught to handle
> gpios described by swnodes.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 8e323e93be91..929f9363e94b 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -2202,10 +2202,11 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  		 * and in this case fall back to using AFI per port register
>  		 * to toggle PERST# SFIO line.
>  		 */
> -		rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
> -							     "reset-gpios", 0,
> -							     GPIOD_OUT_LOW,
> -							     label);
> +		rp->reset_gpio = devm_fwnode_gpiod_get(dev,
> +						       of_fwnode_handle(port),
> +						       "reset",
> +						       GPIOD_OUT_LOW,
> +						       label);

Why in pci-aardvark.c for PERST# reset-gpio you have used
devm_gpiod_get_optional() and here in pci-tegra.c you have used
devm_fwnode_gpiod_get()? I think that PERST# logic is same in both
drivers.

>  		if (IS_ERR(rp->reset_gpio)) {
>  			if (PTR_ERR(rp->reset_gpio) == -ENOENT) {
>  				rp->reset_gpio = NULL;
> 
> -- 
> b4 0.10.0-dev-fc921

Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 10:23 AM Pali Rohár <pali@kernel.org> wrote:
> On Sunday 04 September 2022 23:30:53 Dmitry Torokhov wrote:

...

> > -             rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
> > -                                                          "reset-gpios", 0,
> > -                                                          GPIOD_OUT_LOW,
> > -                                                          label);
> > +             rp->reset_gpio = devm_fwnode_gpiod_get(dev,
> > +                                                    of_fwnode_handle(port),
> > +                                                    "reset",
> > +                                                    GPIOD_OUT_LOW,
> > +                                                    label);
>
> Why in pci-aardvark.c for PERST# reset-gpio you have used
> devm_gpiod_get_optional() and here in pci-tegra.c you have used
> devm_fwnode_gpiod_get()? I think that PERST# logic is same in both
> drivers.

It's not the same dev and its node in this case. There is one reset
for _all_ ports, here is the reset on _per port_ basis.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get

[Pali Rohár]

On Monday 05 September 2022 13:49:21 Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 10:23 AM Pali Rohár <pali@kernel.org> wrote:
> > On Sunday 04 September 2022 23:30:53 Dmitry Torokhov wrote:
> 
> ...
> 
> > > -             rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
> > > -                                                          "reset-gpios", 0,
> > > -                                                          GPIOD_OUT_LOW,
> > > -                                                          label);
> > > +             rp->reset_gpio = devm_fwnode_gpiod_get(dev,
> > > +                                                    of_fwnode_handle(port),
> > > +                                                    "reset",
> > > +                                                    GPIOD_OUT_LOW,
> > > +                                                    label);
> >
> > Why in pci-aardvark.c for PERST# reset-gpio you have used
> > devm_gpiod_get_optional() and here in pci-tegra.c you have used
> > devm_fwnode_gpiod_get()? I think that PERST# logic is same in both
> > drivers.
> 
> It's not the same dev and its node in this case. There is one reset
> for _all_ ports, here is the reset on _per port_ basis.

aardvark is single port controller. So it is basically same.

Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 1:53 PM Pali Rohár <pali@kernel.org> wrote:
> On Monday 05 September 2022 13:49:21 Andy Shevchenko wrote:

...

> > It's not the same dev and its node in this case. There is one reset
> > for _all_ ports, here is the reset on _per port_ basis.
>
> aardvark is single port controller. So it is basically same.

Yep, just replied to my message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 1:49 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Sep 5, 2022 at 10:23 AM Pali Rohár <pali@kernel.org> wrote:
> > On Sunday 04 September 2022 23:30:53 Dmitry Torokhov wrote:
>
> ...
>
> > > -             rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
> > > -                                                          "reset-gpios", 0,
> > > -                                                          GPIOD_OUT_LOW,
> > > -                                                          label);
> > > +             rp->reset_gpio = devm_fwnode_gpiod_get(dev,
> > > +                                                    of_fwnode_handle(port),
> > > +                                                    "reset",
> > > +                                                    GPIOD_OUT_LOW,
> > > +                                                    label);
> >
> > Why in pci-aardvark.c for PERST# reset-gpio you have used
> > devm_gpiod_get_optional() and here in pci-tegra.c you have used
> > devm_fwnode_gpiod_get()? I think that PERST# logic is same in both
> > drivers.
>
> It's not the same dev and its node in this case. There is one reset
> for _all_ ports, here is the reset on _per port_ basis.

Actually I'm wrong, the aardvark has only one port (?) to serve there.
In any case, here dev == dev->of_node, here dev != port.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get

[Dmitry Torokhov]

On Mon, Sep 05, 2022 at 09:19:02AM +0200, Pali Rohár wrote:
> On Sunday 04 September 2022 23:30:53 Dmitry Torokhov wrote:
> > I would like to limit (or maybe even remove) use of
> > [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned
> > a bit, so let's switch to the generic device property API. It may even
> > help with handling secondary fwnodes when gpiolib is taught to handle
> > gpios described by swnodes.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> > index 8e323e93be91..929f9363e94b 100644
> > --- a/drivers/pci/controller/pci-tegra.c
> > +++ b/drivers/pci/controller/pci-tegra.c
> > @@ -2202,10 +2202,11 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
> >  		 * and in this case fall back to using AFI per port register
> >  		 * to toggle PERST# SFIO line.
> >  		 */
> > -		rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
> > -							     "reset-gpios", 0,
> > -							     GPIOD_OUT_LOW,
> > -							     label);
> > +		rp->reset_gpio = devm_fwnode_gpiod_get(dev,
> > +						       of_fwnode_handle(port),
> > +						       "reset",
> > +						       GPIOD_OUT_LOW,
> > +						       label);
> 
> Why in pci-aardvark.c for PERST# reset-gpio you have used
> devm_gpiod_get_optional() and here in pci-tegra.c you have used
> devm_fwnode_gpiod_get()? I think that PERST# logic is same in both
> drivers.

I believe Andy already answered that, but in this driver we can have
several root ports described via subnodes of dev->of_node, and reset
GPIOs are attached to those subnodes. We are forced to use
devm_fwnode_gpiod_get() instead of devm_gpiod_get_optional() as we need
to supply the exact fwnode we need to look up GPIO in, and can not infer
it from the 'dev' parameter of devm_gpiod_get().

Thanks.

-- 
Dmitry

Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get

[Linus Walleij]

On Mon, Sep 5, 2022 at 8:31 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> I would like to limit (or maybe even remove) use of
> [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned
> a bit, so let's switch to the generic device property API. It may even
> help with handling secondary fwnodes when gpiolib is taught to handle
> gpios described by swnodes.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 02/11] drm/tegra: switch to using devm_fwnode_gpiod_get

[Dmitry Torokhov]

I would like to limit (or maybe even remove) use of
[devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned
a bit, so let's switch to the generic device property API. It may even
help with handling secondary fwnodes when gpiolib is taught to handle
gpios described by swnodes.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 47d26b5d9945..a8925dcd7edd 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -133,11 +133,11 @@ int tegra_output_probe(struct tegra_output *output)
 		}
 	}
 
-	output->hpd_gpio = devm_gpiod_get_from_of_node(output->dev,
-						       output->of_node,
-						       "nvidia,hpd-gpio", 0,
-						       GPIOD_IN,
-						       "HDMI hotplug detect");
+	output->hpd_gpio = devm_fwnode_gpiod_get(output->dev,
+					of_fwnode_handle(output->of_node),
+					"nvidia,hpd",
+					GPIOD_IN,
+					"HDMI hotplug detect");
 	if (IS_ERR(output->hpd_gpio)) {
 		if (PTR_ERR(output->hpd_gpio) != -ENOENT)
 			return PTR_ERR(output->hpd_gpio);

-- 
b4 0.10.0-dev-fc921

Re: [PATCH v1 02/11] drm/tegra: switch to using devm_fwnode_gpiod_get

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> I would like to limit (or maybe even remove) use of
> [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned
> a bit, so let's switch to the generic device property API.

> It may even
> help with handling secondary fwnodes when gpiolib is taught to handle
> gpios described by swnodes.

I would remove this sentence from all commit messages since it's a
debatable thing and might even not happen, so the above is a pure
speculation.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 02/11] drm/tegra: switch to using devm_fwnode_gpiod_get

[Dmitry Torokhov]

On Mon, Sep 05, 2022 at 01:57:01PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > I would like to limit (or maybe even remove) use of
> > [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned
> > a bit, so let's switch to the generic device property API.
> 
> > It may even
> > help with handling secondary fwnodes when gpiolib is taught to handle
> > gpios described by swnodes.
> 
> I would remove this sentence from all commit messages since it's a
> debatable thing and might even not happen, so the above is a pure
> speculation.

I have the patches. Granted, I had them since '19 ;) but I'm rebasing
them and going to push them. I need them to convert bunch of input
drivers away from platform data.

Thanks.

-- 
Dmitry

Re: [PATCH v1 02/11] drm/tegra: switch to using devm_fwnode_gpiod_get

[Linus Walleij]

On Mon, Sep 5, 2022 at 9:37 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Sep 05, 2022 at 01:57:01PM +0300, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > I would like to limit (or maybe even remove) use of
> > > [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned
> > > a bit, so let's switch to the generic device property API.
> >
> > > It may even
> > > help with handling secondary fwnodes when gpiolib is taught to handle
> > > gpios described by swnodes.
> >
> > I would remove this sentence from all commit messages since it's a
> > debatable thing and might even not happen, so the above is a pure
> > speculation.
>
> I have the patches. Granted, I had them since '19 ;) but I'm rebasing
> them and going to push them. I need them to convert bunch of input
> drivers away from platform data.

That's good news!

Are you referring to this patch set mentioned in a discussion
from 2017 thru 2020?
https://lore.kernel.org/linux-input/20200826161222.GA1665100@dtor-ws/

I put aside GPIO descriptor conversion for input devices (keys, buttons)
in board files anticipating a swnode mechanism.

Yours,
Linus Walleij

Re: [PATCH v1 02/11] drm/tegra: switch to using devm_fwnode_gpiod_get

[Dmitry Torokhov]

On Mon, Sep 05, 2022 at 11:03:38PM +0200, Linus Walleij wrote:
> On Mon, Sep 5, 2022 at 9:37 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Sep 05, 2022 at 01:57:01PM +0300, Andy Shevchenko wrote:
> > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > I would like to limit (or maybe even remove) use of
> > > > [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned
> > > > a bit, so let's switch to the generic device property API.
> > >
> > > > It may even
> > > > help with handling secondary fwnodes when gpiolib is taught to handle
> > > > gpios described by swnodes.
> > >
> > > I would remove this sentence from all commit messages since it's a
> > > debatable thing and might even not happen, so the above is a pure
> > > speculation.
> >
> > I have the patches. Granted, I had them since '19 ;) but I'm rebasing
> > them and going to push them. I need them to convert bunch of input
> > drivers away from platform data.
> 
> That's good news!
> 
> Are you referring to this patch set mentioned in a discussion
> from 2017 thru 2020?
> https://lore.kernel.org/linux-input/20200826161222.GA1665100@dtor-ws/
> 
> I put aside GPIO descriptor conversion for input devices (keys, buttons)
> in board files anticipating a swnode mechanism.

Yep, that one, exactly. It is taking a bit longer than I anticipated ;)

Thanks.

-- 
Dmitry

[PATCH v1 03/11] mtd: rawnand: stm32_fmc2: switch to using devm_fwnode_gpiod_get()

[Dmitry Torokhov]

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
fwnode property API.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
index 87c1c7dd97eb..7e466b840368 100644
--- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
+++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
@@ -1799,9 +1799,8 @@ static int stm32_fmc2_nfc_parse_child(struct stm32_fmc2_nfc *nfc,
 		nand->cs_used[i] = cs;
 	}
 
-	nand->wp_gpio = devm_gpiod_get_from_of_node(nfc->dev, dn,
-						    "wp-gpios", 0,
-						    GPIOD_OUT_HIGH, "wp");
+	nand->wp_gpio = devm_fwnode_gpiod_get(nfc->dev, of_fwnode_handle(dn),
+					      "wp", GPIOD_OUT_HIGH, "wp");
 	if (IS_ERR(nand->wp_gpio)) {
 		ret = PTR_ERR(nand->wp_gpio);
 		if (ret != -ENOENT)

-- 
b4 0.10.0-dev-fc921

Re: [PATCH v1 03/11] mtd: rawnand: stm32_fmc2: switch to using devm_fwnode_gpiod_get()

[Miquel Raynal]

On Mon, 2022-09-05 at 06:30:55 UTC, 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
> fwnode property API.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

[PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()

[Dmitry Torokhov]

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(), 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>

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index 68cd4b68e3a2..f0240107edb1 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -1440,16 +1440,22 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 			return err;
 		}
 
-		gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np,
-						    "nvidia,phy-reset-gpio",
-						    0, GPIOD_OUT_HIGH,
-						    "ulpi_phy_reset_b");
+		gpiod = devm_gpiod_get(&pdev->dev, "nvidia,phy-reset",
+				       GPIOD_OUT_HIGH);
 		err = PTR_ERR_OR_ZERO(gpiod);
 		if (err) {
 			dev_err(&pdev->dev,
 				"Request failed for reset GPIO: %d\n", err);
 			return err;
 		}
+
+		err = gpiod_set_consumer_name(gpiod, "ulpi_phy_reset_b");
+		if (err) {
+			dev_err(&pdev->dev,
+				"Failed to set up reset GPIO name: %d\n", err);
+			return err;
+		}
+
 		tegra_phy->reset_gpio = gpiod;
 
 		phy = devm_otg_ulpi_create(&pdev->dev,

-- 
b4 0.10.0-dev-fc921

Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 9:32 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(), 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.

...

> -               gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np,
> -                                                   "nvidia,phy-reset-gpio",
> -                                                   0, GPIOD_OUT_HIGH,
> -                                                   "ulpi_phy_reset_b");
> +               gpiod = devm_gpiod_get(&pdev->dev, "nvidia,phy-reset",
> +                                      GPIOD_OUT_HIGH);
>                 err = PTR_ERR_OR_ZERO(gpiod);

What does _OR_ZERO mean now?

>                 if (err) {
>                         dev_err(&pdev->dev,
>                                 "Request failed for reset GPIO: %d\n", err);
>                         return err;
>                 }



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()

[Dmitry Torokhov]

On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 9:32 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(), 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.
> 
> ...
> 
> > -               gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np,
> > -                                                   "nvidia,phy-reset-gpio",
> > -                                                   0, GPIOD_OUT_HIGH,
> > -                                                   "ulpi_phy_reset_b");
> > +               gpiod = devm_gpiod_get(&pdev->dev, "nvidia,phy-reset",
> > +                                      GPIOD_OUT_HIGH);
> >                 err = PTR_ERR_OR_ZERO(gpiod);
> 
> What does _OR_ZERO mean now?

This converts a pointer to an error code if a pointer represents
ERR_PTR() encoded error, or 0 to indicate success.

static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr)
{
	if (IS_ERR(ptr))
		return PTR_ERR(ptr);
	else
		return 0;
}

Thanks.

-- 
Dmitry

Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:

...

> > > -               gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np,
> > > -                                                   "nvidia,phy-reset-gpio",
> > > -                                                   0, GPIOD_OUT_HIGH,
> > > -                                                   "ulpi_phy_reset_b");
> > > +               gpiod = devm_gpiod_get(&pdev->dev, "nvidia,phy-reset",
> > > +                                      GPIOD_OUT_HIGH);
> > >                 err = PTR_ERR_OR_ZERO(gpiod);
> >
> > What does _OR_ZERO mean now?
>
> This converts a pointer to an error code if a pointer represents
> ERR_PTR() encoded error, or 0 to indicate success.

Yes, I know that. My point is, how is it useful now (or even before)?
I mean that devm_gpio_get() never returns NULL, right?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()

[Dmitry Torokhov]

On Mon, Sep 05, 2022 at 10:41:40PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote:
> > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> 
> ...
> 
> > > > -               gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np,
> > > > -                                                   "nvidia,phy-reset-gpio",
> > > > -                                                   0, GPIOD_OUT_HIGH,
> > > > -                                                   "ulpi_phy_reset_b");
> > > > +               gpiod = devm_gpiod_get(&pdev->dev, "nvidia,phy-reset",
> > > > +                                      GPIOD_OUT_HIGH);
> > > >                 err = PTR_ERR_OR_ZERO(gpiod);
> > >
> > > What does _OR_ZERO mean now?
> >
> > This converts a pointer to an error code if a pointer represents
> > ERR_PTR() encoded error, or 0 to indicate success.
> 
> Yes, I know that. My point is, how is it useful now (or even before)?
> I mean that devm_gpio_get() never returns NULL, right?

What does returning NULL have to do with anything. It converts a pointer
to a "classic" return code, with negative errors and 0 on success.

It allows to not use multiple IS_ERR/PTR_ERR in the code (I'd need 1
IS_ERR and 2 PTR_ERR, one in dev_err() and another to return).

Thanks.

-- 
Dmitry

Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 10:51 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Sep 05, 2022 at 10:41:40PM +0300, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:

...

> > > > > -               gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np,
> > > > > -                                                   "nvidia,phy-reset-gpio",
> > > > > -                                                   0, GPIOD_OUT_HIGH,
> > > > > -                                                   "ulpi_phy_reset_b");
> > > > > +               gpiod = devm_gpiod_get(&pdev->dev, "nvidia,phy-reset",
> > > > > +                                      GPIOD_OUT_HIGH);
> > > > >                 err = PTR_ERR_OR_ZERO(gpiod);
> > > >
> > > > What does _OR_ZERO mean now?
> > >
> > > This converts a pointer to an error code if a pointer represents
> > > ERR_PTR() encoded error, or 0 to indicate success.
> >
> > Yes, I know that. My point is, how is it useful now (or even before)?
> > I mean that devm_gpio_get() never returns NULL, right?
>
> What does returning NULL have to do with anything.

It has to do with a dead code. If defm_gpiod_get() does not return
NULL, then why do we even bother to check?

> It converts a pointer
> to a "classic" return code, with negative errors and 0 on success.
>
> It allows to not use multiple IS_ERR/PTR_ERR in the code (I'd need 1
> IS_ERR and 2 PTR_ERR, one in dev_err() and another to return).

I don't see how this is relevant.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()

[Guenter Roeck]

On 9/5/22 12:55, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 10:51 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Mon, Sep 05, 2022 at 10:41:40PM +0300, Andy Shevchenko wrote:
>>> On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>>> On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote:
>>>>> On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
>>>>> <dmitry.torokhov@gmail.com> wrote:
> 
> ...
> 
>>>>>> -               gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np,
>>>>>> -                                                   "nvidia,phy-reset-gpio",
>>>>>> -                                                   0, GPIOD_OUT_HIGH,
>>>>>> -                                                   "ulpi_phy_reset_b");
>>>>>> +               gpiod = devm_gpiod_get(&pdev->dev, "nvidia,phy-reset",
>>>>>> +                                      GPIOD_OUT_HIGH);
>>>>>>                  err = PTR_ERR_OR_ZERO(gpiod);
>>>>>
>>>>> What does _OR_ZERO mean now?
>>>>
>>>> This converts a pointer to an error code if a pointer represents
>>>> ERR_PTR() encoded error, or 0 to indicate success.
>>>
>>> Yes, I know that. My point is, how is it useful now (or even before)?
>>> I mean that devm_gpio_get() never returns NULL, right?
>>
>> What does returning NULL have to do with anything.
> 
> It has to do with a dead code. If defm_gpiod_get() does not return
> NULL, then why do we even bother to check?
> 

PTR_ERR_OR_ZERO() converts into an error code (if the pointer is an
ERR_PTR) or 0 if it is a real pointer. Its purpose is not to convert
NULL into 0, its purpose is to convert a pointer either into an error
code or 0. That is what is done here, and it is done all over the place
in the kernel. I don't see your problem with it. Care to explain ?

>> It converts a pointer
>> to a "classic" return code, with negative errors and 0 on success.
>>
>> It allows to not use multiple IS_ERR/PTR_ERR in the code (I'd need 1
>> IS_ERR and 2 PTR_ERR, one in dev_err() and another to return).
> 
> I don't see how this is relevant.
> 

You lost me. Really, please explain your problem with PTR_ERR_OR_ZERO().

Thanks,
Guenter

Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()

[Andy Shevchenko]

On Mon, Sep 05, 2022 at 03:07:48PM -0700, Guenter Roeck wrote:
> On 9/5/22 12:55, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 10:51 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Mon, Sep 05, 2022 at 10:41:40PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote:
> > > > > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
> > > > > > <dmitry.torokhov@gmail.com> wrote:

...

> > > > > > > +               gpiod = devm_gpiod_get(&pdev->dev, "nvidia,phy-reset",
> > > > > > > +                                      GPIOD_OUT_HIGH);
> > > > > > >                  err = PTR_ERR_OR_ZERO(gpiod);
> > > > > > 
> > > > > > What does _OR_ZERO mean now?
> > > > > 
> > > > > This converts a pointer to an error code if a pointer represents
> > > > > ERR_PTR() encoded error, or 0 to indicate success.
> > > > 
> > > > Yes, I know that. My point is, how is it useful now (or even before)?
> > > > I mean that devm_gpio_get() never returns NULL, right?
> > > 
> > > What does returning NULL have to do with anything.
> > 
> > It has to do with a dead code. If defm_gpiod_get() does not return
> > NULL, then why do we even bother to check?
> 
> PTR_ERR_OR_ZERO() converts into an error code (if the pointer is an
> ERR_PTR) or 0 if it is a real pointer. Its purpose is not to convert
> NULL into 0, its purpose is to convert a pointer either into an error
> code or 0. That is what is done here, and it is done all over the place
> in the kernel. I don't see your problem with it. Care to explain ?
> 
> > > It converts a pointer
> > > to a "classic" return code, with negative errors and 0 on success.
> > > 
> > > It allows to not use multiple IS_ERR/PTR_ERR in the code (I'd need 1
> > > IS_ERR and 2 PTR_ERR, one in dev_err() and another to return).
> > 
> > I don't see how this is relevant.
> 
> You lost me. Really, please explain your problem with PTR_ERR_OR_ZERO().

I don't know what I was thinking about... You, guys, are right, sorry for
my noise.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 05/11] usb: gadget: udc: at91: switch to using fwnode_gpiod_get_index()

[Dmitry Torokhov]

I would like to stop exporting OF-specific gpiod_get_from_of_node()
so that gpiolib can be cleaned a bit, so let's switch to the generic
fwnode property API.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

diff --git a/drivers/usb/gadget/udc/at91_udc.c b/drivers/usb/gadget/udc/at91_udc.c
index 728987280373..1db1dbbab79a 100644
--- a/drivers/usb/gadget/udc/at91_udc.c
+++ b/drivers/usb/gadget/udc/at91_udc.c
@@ -1779,12 +1779,14 @@ static void at91udc_of_init(struct at91_udc *udc, struct device_node *np)
 	if (of_property_read_u32(np, "atmel,vbus-polled", &val) == 0)
 		board->vbus_polled = 1;
 
-	board->vbus_pin = gpiod_get_from_of_node(np, "atmel,vbus-gpio", 0,
-						 GPIOD_IN, "udc_vbus");
+	board->vbus_pin = fwnode_gpiod_get_index(of_fwnode_handle(np),
+						 "atmel,vbus", 0, GPIOD_IN,
+						 "udc_vbus");
 	if (IS_ERR(board->vbus_pin))
 		board->vbus_pin = NULL;
 
-	board->pullup_pin = gpiod_get_from_of_node(np, "atmel,pullup-gpio", 0,
+	board->pullup_pin = fwnode_gpiod_get_index(of_fwnode_handle(np),
+						   "atmel,pullup", 0,
 						   GPIOD_ASIS, "udc_pullup");
 	if (IS_ERR(board->pullup_pin))
 		board->pullup_pin = NULL;

-- 
b4 0.10.0-dev-fc921

[PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()

[Dmitry Torokhov]

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>

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

Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()

[Pali Rohár]

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

Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()

[Andy Shevchenko]

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.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()

[Dmitry Torokhov]

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,

-- 
Dmitry

Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()

[Pali Rohár]

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

Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()

[Linus Walleij]

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

[PATCH v1 07/11] PCI: apple: switch to using fwnode_gpiod_get_index()

[Dmitry Torokhov]

I would like to stop exporting OF-specific gpiod_get_from_of_node()
so that gpiolib can be cleaned a bit, so let's switch to the generic
fwnode property API.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index a2c3c207a04b..d83817d3ff86 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -516,8 +516,8 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	u32 stat, idx;
 	int ret, i;
 
-	reset = gpiod_get_from_of_node(np, "reset-gpios", 0,
-				       GPIOD_OUT_LOW, "PERST#");
+	reset = fwnode_gpiod_get_index(of_fwnode_handle(np),
+				       "reset", 0, GPIOD_OUT_LOW, "PERST#");
 	if (IS_ERR(reset))
 		return PTR_ERR(reset);
 

-- 
b4 0.10.0-dev-fc921

Re: [PATCH v1 07/11] PCI: apple: switch to using fwnode_gpiod_get_index()

[Dmitry Torokhov]

On Sun, Sep 04, 2022 at 11:30:59PM -0700, Dmitry Torokhov wrote:
> I would like to stop exporting OF-specific gpiod_get_from_of_node()
> so that gpiolib can be cleaned a bit, so let's switch to the generic
> fwnode property API.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index a2c3c207a04b..d83817d3ff86 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -516,8 +516,8 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  	u32 stat, idx;
>  	int ret, i;
>  
> -	reset = gpiod_get_from_of_node(np, "reset-gpios", 0,
> -				       GPIOD_OUT_LOW, "PERST#");
> +	reset = fwnode_gpiod_get_index(of_fwnode_handle(np),
> +				       "reset", 0, GPIOD_OUT_LOW, "PERST#");

Hmm, I am looking at the driver and it leaks the reset gpio on
unbind/unload. I guess it does not matter in practice, but still nice
not to leak. Thankfully it is easy to cure by switching to devm option:
devm_fwnode_gpiod_get().

I'll send and updated patch with a new justification.

Thanks.

-- 
Dmitry

Re: [PATCH v1 07/11] PCI: apple: switch to using fwnode_gpiod_get_index()

[Linus Walleij]

On Tue, Sep 6, 2022 at 2:19 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> On Sun, Sep 04, 2022 at 11:30:59PM -0700, Dmitry Torokhov wrote:
> > I would like to stop exporting OF-specific gpiod_get_from_of_node()
> > so that gpiolib can be cleaned a bit, so let's switch to the generic
> > fwnode property API.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > index a2c3c207a04b..d83817d3ff86 100644
> > --- a/drivers/pci/controller/pcie-apple.c
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -516,8 +516,8 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
> >       u32 stat, idx;
> >       int ret, i;
> >
> > -     reset = gpiod_get_from_of_node(np, "reset-gpios", 0,
> > -                                    GPIOD_OUT_LOW, "PERST#");
> > +     reset = fwnode_gpiod_get_index(of_fwnode_handle(np),
> > +                                    "reset", 0, GPIOD_OUT_LOW, "PERST#");
>
> Hmm, I am looking at the driver and it leaks the reset gpio on
> unbind/unload. I guess it does not matter in practice, but still nice
> not to leak. Thankfully it is easy to cure by switching to devm option:
> devm_fwnode_gpiod_get().
>
> I'll send and updated patch with a new justification.

With that change:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 08/11] regulator: bd71815: switch to using devm_fwnode_gpiod_get()

[Dmitry Torokhov]

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
fwnode property API.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

diff --git a/drivers/regulator/bd71815-regulator.c b/drivers/regulator/bd71815-regulator.c
index acaa6607898e..c2b8b8be7824 100644
--- a/drivers/regulator/bd71815-regulator.c
+++ b/drivers/regulator/bd71815-regulator.c
@@ -571,11 +571,10 @@ static int bd7181x_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "No parent regmap\n");
 		return -ENODEV;
 	}
-	ldo4_en = devm_gpiod_get_from_of_node(&pdev->dev,
-					      pdev->dev.parent->of_node,
-						 "rohm,vsel-gpios", 0,
-						 GPIOD_ASIS, "ldo4-en");
 
+	ldo4_en = devm_fwnode_gpiod_get(&pdev->dev,
+					dev_fwnode(pdev->dev.parent),
+					"rohm,vsel", GPIOD_ASIS, "ldo4-en");
 	if (IS_ERR(ldo4_en)) {
 		ret = PTR_ERR(ldo4_en);
 		if (ret != -ENOENT)

-- 
b4 0.10.0-dev-fc921

Re: [PATCH v1 08/11] regulator: bd71815: switch to using devm_fwnode_gpiod_get()

[Matti Vaittinen]

On 9/5/22 09:31, 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
> fwnode property API.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> 
> diff --git a/drivers/regulator/bd71815-regulator.c b/drivers/regulator/bd71815-regulator.c
> index acaa6607898e..c2b8b8be7824 100644
> --- a/drivers/regulator/bd71815-regulator.c
> +++ b/drivers/regulator/bd71815-regulator.c
> @@ -571,11 +571,10 @@ static int bd7181x_probe(struct platform_device *pdev)
>   		dev_err(&pdev->dev, "No parent regmap\n");
>   		return -ENODEV;
>   	}
> -	ldo4_en = devm_gpiod_get_from_of_node(&pdev->dev,
> -					      pdev->dev.parent->of_node,
> -						 "rohm,vsel-gpios", 0,
> -						 GPIOD_ASIS, "ldo4-en");
>   
> +	ldo4_en = devm_fwnode_gpiod_get(&pdev->dev,
> +					dev_fwnode(pdev->dev.parent),
> +					"rohm,vsel", GPIOD_ASIS, "ldo4-en");
>   	if (IS_ERR(ldo4_en)) {
>   		ret = PTR_ERR(ldo4_en);
>   		if (ret != -ENOENT)
> 


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v1 08/11] regulator: bd71815: switch to using devm_fwnode_gpiod_get()

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 9:33 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
> fwnode property API.

Special thanks for using dev_fwnode().
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
(Dunno if my suggestion about parent applies here)

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> diff --git a/drivers/regulator/bd71815-regulator.c b/drivers/regulator/bd71815-regulator.c
> index acaa6607898e..c2b8b8be7824 100644
> --- a/drivers/regulator/bd71815-regulator.c
> +++ b/drivers/regulator/bd71815-regulator.c
> @@ -571,11 +571,10 @@ static int bd7181x_probe(struct platform_device *pdev)
>                 dev_err(&pdev->dev, "No parent regmap\n");
>                 return -ENODEV;
>         }
> -       ldo4_en = devm_gpiod_get_from_of_node(&pdev->dev,
> -                                             pdev->dev.parent->of_node,
> -                                                "rohm,vsel-gpios", 0,
> -                                                GPIOD_ASIS, "ldo4-en");
>
> +       ldo4_en = devm_fwnode_gpiod_get(&pdev->dev,
> +                                       dev_fwnode(pdev->dev.parent),
> +                                       "rohm,vsel", GPIOD_ASIS, "ldo4-en");
>         if (IS_ERR(ldo4_en)) {
>                 ret = PTR_ERR(ldo4_en);
>                 if (ret != -ENOENT)
>
> --
> b4 0.10.0-dev-fc921



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 08/11] regulator: bd71815: switch to using devm_fwnode_gpiod_get()

[Linus Walleij]

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
> fwnode property API.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 09/11] regulator: bd9576: switch to using devm_fwnode_gpiod_get()

[Dmitry Torokhov]

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
fwnode property API.

While at it switch the rest of the calls to read properties in
bd957x_probe() to the generic device property API as well.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

diff --git a/drivers/regulator/bd9576-regulator.c b/drivers/regulator/bd9576-regulator.c
index aa42da4d141e..393c8693b327 100644
--- a/drivers/regulator/bd9576-regulator.c
+++ b/drivers/regulator/bd9576-regulator.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/regulator/of_regulator.h>
@@ -939,8 +940,8 @@ static int bd957x_probe(struct platform_device *pdev)
 	}
 
 	ic_data->regmap = regmap;
-	vout_mode = of_property_read_bool(pdev->dev.parent->of_node,
-					 "rohm,vout1-en-low");
+	vout_mode = device_property_read_bool(pdev->dev.parent,
+					      "rohm,vout1-en-low");
 	if (vout_mode) {
 		struct gpio_desc *en;
 
@@ -948,10 +949,10 @@ static int bd957x_probe(struct platform_device *pdev)
 
 		/* VOUT1 enable state judged by VOUT1_EN pin */
 		/* See if we have GPIO defined */
-		en = devm_gpiod_get_from_of_node(&pdev->dev,
-						 pdev->dev.parent->of_node,
-						 "rohm,vout1-en-gpios", 0,
-						 GPIOD_OUT_LOW, "vout1-en");
+		en = devm_fwnode_gpiod_get(&pdev->dev,
+					   dev_fwnode(pdev->dev.parent),
+					   "rohm,vout1-en", GPIOD_OUT_LOW,
+					   "vout1-en");
 		if (!IS_ERR(en)) {
 			/* VOUT1_OPS gpio ctrl */
 			/*
@@ -986,8 +987,8 @@ static int bd957x_probe(struct platform_device *pdev)
 	 * like DDR voltage selection.
 	 */
 	platform_set_drvdata(pdev, ic_data);
-	ddr_sel =  of_property_read_bool(pdev->dev.parent->of_node,
-					 "rohm,ddr-sel-low");
+	ddr_sel = device_property_read_bool(pdev->dev.parent,
+					    "rohm,ddr-sel-low");
 	if (ddr_sel)
 		ic_data->regulator_data[2].desc.fixed_uV = 1350000;
 	else

-- 
b4 0.10.0-dev-fc921

Re: [PATCH v1 09/11] regulator: bd9576: switch to using devm_fwnode_gpiod_get()

[Matti Vaittinen]

On 9/5/22 09:31, 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
> fwnode property API.
> 
> While at it switch the rest of the calls to read properties in
> bd957x_probe() to the generic device property API as well.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>


Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

Thanks!

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v1 09/11] regulator: bd9576: switch to using devm_fwnode_gpiod_get()

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 9:33 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
> fwnode property API.
>
> While at it switch the rest of the calls to read properties in
> bd957x_probe() to the generic device property API as well.

With or without below addressed,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> diff --git a/drivers/regulator/bd9576-regulator.c b/drivers/regulator/bd9576-regulator.c
> index aa42da4d141e..393c8693b327 100644
> --- a/drivers/regulator/bd9576-regulator.c
> +++ b/drivers/regulator/bd9576-regulator.c
> @@ -12,6 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/regulator/driver.h>
>  #include <linux/regulator/machine.h>
>  #include <linux/regulator/of_regulator.h>
> @@ -939,8 +940,8 @@ static int bd957x_probe(struct platform_device *pdev)
>         }
>
>         ic_data->regmap = regmap;
> -       vout_mode = of_property_read_bool(pdev->dev.parent->of_node,
> -                                        "rohm,vout1-en-low");
> +       vout_mode = device_property_read_bool(pdev->dev.parent,
> +                                             "rohm,vout1-en-low");

They all using parent device and you may make code neater by adding

  struct device *parent = pdev->dev.parent;

at the definition block of the probe function.

>         if (vout_mode) {
>                 struct gpio_desc *en;
>
> @@ -948,10 +949,10 @@ static int bd957x_probe(struct platform_device *pdev)
>
>                 /* VOUT1 enable state judged by VOUT1_EN pin */
>                 /* See if we have GPIO defined */
> -               en = devm_gpiod_get_from_of_node(&pdev->dev,
> -                                                pdev->dev.parent->of_node,
> -                                                "rohm,vout1-en-gpios", 0,
> -                                                GPIOD_OUT_LOW, "vout1-en");
> +               en = devm_fwnode_gpiod_get(&pdev->dev,
> +                                          dev_fwnode(pdev->dev.parent),
> +                                          "rohm,vout1-en", GPIOD_OUT_LOW,
> +                                          "vout1-en");
>                 if (!IS_ERR(en)) {
>                         /* VOUT1_OPS gpio ctrl */
>                         /*
> @@ -986,8 +987,8 @@ static int bd957x_probe(struct platform_device *pdev)
>          * like DDR voltage selection.
>          */
>         platform_set_drvdata(pdev, ic_data);
> -       ddr_sel =  of_property_read_bool(pdev->dev.parent->of_node,
> -                                        "rohm,ddr-sel-low");
> +       ddr_sel = device_property_read_bool(pdev->dev.parent,
> +                                           "rohm,ddr-sel-low");
>         if (ddr_sel)
>                 ic_data->regulator_data[2].desc.fixed_uV = 1350000;
>         else
>
> --
> b4 0.10.0-dev-fc921



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 09/11] regulator: bd9576: switch to using devm_fwnode_gpiod_get()

[Matti Vaittinen]

On 9/5/22 13:40, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 9:33 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
>> fwnode property API.
>>
>> While at it switch the rest of the calls to read properties in
>> bd957x_probe() to the generic device property API as well.
> 
> With or without below addressed,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> diff --git a/drivers/regulator/bd9576-regulator.c b/drivers/regulator/bd9576-regulator.c
>> index aa42da4d141e..393c8693b327 100644
>> --- a/drivers/regulator/bd9576-regulator.c
>> +++ b/drivers/regulator/bd9576-regulator.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/property.h>
>>   #include <linux/regulator/driver.h>
>>   #include <linux/regulator/machine.h>
>>   #include <linux/regulator/of_regulator.h>
>> @@ -939,8 +940,8 @@ static int bd957x_probe(struct platform_device *pdev)
>>          }
>>
>>          ic_data->regmap = regmap;
>> -       vout_mode = of_property_read_bool(pdev->dev.parent->of_node,
>> -                                        "rohm,vout1-en-low");
>> +       vout_mode = device_property_read_bool(pdev->dev.parent,
>> +                                             "rohm,vout1-en-low");
> 
> They all using parent device and you may make code neater by adding
> 
>    struct device *parent = pdev->dev.parent;

This is a matter of personal preference. I prefer seeing 
pdev->dev.parent - as it is more obvious (to me) what the 'pdev' is than 
what 'parent' would be.

I'd use the local variable only when it shortens at least one of the 
lines so that we avoid splitting it. After that being said - I'm not 
going to argue over this change either if one who is improving the 
driver wants to use the "helper" variable here.

Best Regards
	-- Matti


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v1 09/11] regulator: bd9576: switch to using devm_fwnode_gpiod_get()

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 4:19 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> On 9/5/22 13:40, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:

...

> >> +       vout_mode = device_property_read_bool(pdev->dev.parent,
> >> +                                             "rohm,vout1-en-low");
> >
> > They all using parent device and you may make code neater by adding
> >
> >    struct device *parent = pdev->dev.parent;
>
> This is a matter of personal preference. I prefer seeing
> pdev->dev.parent - as it is more obvious (to me) what the 'pdev' is than
> what 'parent' would be.
>
> I'd use the local variable only when it shortens at least one of the
> lines so that we avoid splitting it. After that being said - I'm not
> going to argue over this change either if one who is improving the
> driver wants to use the "helper" variable here.

And I believe the quoted one is exactly the case of what you are saying above.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

[Dmitry Torokhov]

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
fwnode property API.

While at it switch the rest of the calls to read properties in
bd9576_wdt_probe() to the generic device property API as well.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

diff --git a/drivers/watchdog/bd9576_wdt.c b/drivers/watchdog/bd9576_wdt.c
index 0b6999f3b6e8..4a20e07fbb69 100644
--- a/drivers/watchdog/bd9576_wdt.c
+++ b/drivers/watchdog/bd9576_wdt.c
@@ -9,8 +9,8 @@
 #include <linux/gpio/consumer.h>
 #include <linux/mfd/rohm-bd957x.h>
 #include <linux/module.h>
-#include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/watchdog.h>
 
@@ -202,10 +202,10 @@ static int bd957x_set_wdt_mode(struct bd9576_wdt_priv *priv, int hw_margin,
 static int bd9576_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->parent->of_node;
 	struct bd9576_wdt_priv *priv;
 	u32 hw_margin[2];
 	u32 hw_margin_max = BD957X_WDT_DEFAULT_MARGIN, hw_margin_min = 0;
+	int count;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -221,40 +221,51 @@ static int bd9576_wdt_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	priv->gpiod_en = devm_gpiod_get_from_of_node(dev, dev->parent->of_node,
-						     "rohm,watchdog-enable-gpios",
-						     0, GPIOD_OUT_LOW,
-						     "watchdog-enable");
+	priv->gpiod_en = devm_fwnode_gpiod_get(dev, dev_fwnode(dev->parent),
+					       "rohm,watchdog-enable",
+					       GPIOD_OUT_LOW,
+					       "watchdog-enable");
 	if (IS_ERR(priv->gpiod_en))
 		return dev_err_probe(dev, PTR_ERR(priv->gpiod_en),
 			      "getting watchdog-enable GPIO failed\n");
 
-	priv->gpiod_ping = devm_gpiod_get_from_of_node(dev, dev->parent->of_node,
-						     "rohm,watchdog-ping-gpios",
-						     0, GPIOD_OUT_LOW,
-						     "watchdog-ping");
+	priv->gpiod_ping = devm_fwnode_gpiod_get(dev, dev_fwnode(dev->parent),
+						 "rohm,watchdog-ping",
+						 GPIOD_OUT_LOW,
+						 "watchdog-ping");
 	if (IS_ERR(priv->gpiod_ping))
 		return dev_err_probe(dev, PTR_ERR(priv->gpiod_ping),
 				     "getting watchdog-ping GPIO failed\n");
 
-	ret = of_property_read_variable_u32_array(np, "rohm,hw-timeout-ms",
-						  &hw_margin[0], 1, 2);
-	if (ret < 0 && ret != -EINVAL)
-		return ret;
+	count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
+	if (count < 0 && count != -EINVAL)
+		return count;
+
+	if (count > 0) {
+		if (count > ARRAY_SIZE(hw_margin))
+			return -EINVAL;
 
-	if (ret == 1)
-		hw_margin_max = hw_margin[0];
+		ret = device_property_read_u32_array(dev->parent,
+						     "rohm,hw-timeout-ms",
+						     hw_margin, count);
+		if (ret < 0)
+			return ret;
 
-	if (ret == 2) {
-		hw_margin_max = hw_margin[1];
-		hw_margin_min = hw_margin[0];
+		if (count == 1)
+			hw_margin_max = hw_margin[0];
+
+		if (count == 2) {
+			hw_margin_max = hw_margin[1];
+			hw_margin_min = hw_margin[0];
+		}
 	}
 
 	ret = bd957x_set_wdt_mode(priv, hw_margin_max, hw_margin_min);
 	if (ret)
 		return ret;
 
-	priv->always_running = of_property_read_bool(np, "always-running");
+	priv->always_running = device_property_read_bool(dev->parent,
+							 "always-running");
 
 	watchdog_set_drvdata(&priv->wdd, priv);
 

-- 
b4 0.10.0-dev-fc921

Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 9:33 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
> fwnode property API.
>
> While at it switch the rest of the calls to read properties in

it, switch

> bd9576_wdt_probe() to the generic device property API as well.

...

>         struct device *dev = &pdev->dev;

struct device *parent = dev->parent;

can make your code slightly neater.

...

> +       count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
> +       if (count < 0 && count != -EINVAL)
> +               return count;
> +
> +       if (count > 0) {

> +               if (count > ARRAY_SIZE(hw_margin))
> +                       return -EINVAL;

Why double check? You may move it out of the (count > 0).

...

> -       if (ret == 1)
> -               hw_margin_max = hw_margin[0];

> +               ret = device_property_read_u32_array(dev->parent,
> +                                                    "rohm,hw-timeout-ms",
> +                                                    hw_margin, count);
> +               if (ret < 0)
> +                       return ret;

So, only this needs the count > 0 check since below already has it implicitly.

> -       if (ret == 2) {
> -               hw_margin_max = hw_margin[1];
> -               hw_margin_min = hw_margin[0];
> +               if (count == 1)
> +                       hw_margin_max = hw_margin[0];
> +
> +               if (count == 2) {
> +                       hw_margin_max = hw_margin[1];
> +                       hw_margin_min = hw_margin[0];
> +               }
>         }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

[Guenter Roeck]

On 9/5/22 04:09, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 9:33 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
>> fwnode property API.
>>
>> While at it switch the rest of the calls to read properties in
> 
> it, switch
> 
>> bd9576_wdt_probe() to the generic device property API as well.
> 
> ...
> 
>>          struct device *dev = &pdev->dev;
> 
> struct device *parent = dev->parent;
> 
> can make your code slightly neater.
> 
> ...
> 
>> +       count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
>> +       if (count < 0 && count != -EINVAL)
>> +               return count;
>> +
>> +       if (count > 0) {
> 
>> +               if (count > ARRAY_SIZE(hw_margin))
>> +                       return -EINVAL;
> 
> Why double check? You may move it out of the (count > 0).
> 

Two checks will always be needed, so I don't entirely see
how that would be better.

> ...
> 
>> -       if (ret == 1)
>> -               hw_margin_max = hw_margin[0];
> 
>> +               ret = device_property_read_u32_array(dev->parent,
>> +                                                    "rohm,hw-timeout-ms",
>> +                                                    hw_margin, count);
>> +               if (ret < 0)
>> +                       return ret;
> 
> So, only this needs the count > 0 check since below already has it implicitly.
> 
Sorry, I don't understand this comment.

Guenter

>> -       if (ret == 2) {
>> -               hw_margin_max = hw_margin[1];
>> -               hw_margin_min = hw_margin[0];
>> +               if (count == 1)
>> +                       hw_margin_max = hw_margin[0];
>> +
>> +               if (count == 2) {
>> +                       hw_margin_max = hw_margin[1];
>> +                       hw_margin_min = hw_margin[0];
>> +               }
>>          }
> 

Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On 9/5/22 04:09, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:

...

> >> +       count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
> >> +       if (count < 0 && count != -EINVAL)
> >> +               return count;
> >> +
> >> +       if (count > 0) {
> >
> >> +               if (count > ARRAY_SIZE(hw_margin))
> >> +                       return -EINVAL;
> >
> > Why double check? You may move it out of the (count > 0).
>
> Two checks will always be needed, so I don't entirely see
> how that would be better.

But not nested. That's my point:

if (count > ARRAY_SIZE())
  return ...
if (count > 0)
  ...

> >> -       if (ret == 1)
> >> -               hw_margin_max = hw_margin[0];
> >
> >> +               ret = device_property_read_u32_array(dev->parent,
> >> +                                                    "rohm,hw-timeout-ms",
> >> +                                                    hw_margin, count);
> >> +               if (ret < 0)
> >> +                       return ret;
> >
> > So, only this needs the count > 0 check since below already has it implicitly.
> >
> Sorry, I don't understand this comment.

if (count > 0) {
  ret = device_property_read_u32_array(...);
  ...
}
if (count == 1)
 ...
if (count == 2)
 ...

But here it might be better to have the nested conditionals.

> >> -       if (ret == 2) {
> >> -               hw_margin_max = hw_margin[1];
> >> -               hw_margin_min = hw_margin[0];
> >> +               if (count == 1)
> >> +                       hw_margin_max = hw_margin[0];
> >> +
> >> +               if (count == 2) {
> >> +                       hw_margin_max = hw_margin[1];
> >> +                       hw_margin_min = hw_margin[0];
> >> +               }
> >>          }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

[Guenter Roeck]

On 9/5/22 08:21, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck <linux@roeck-us.net> wrote:
>> On 9/5/22 04:09, Andy Shevchenko wrote:
>>> On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
> 
> ...
> 
>>>> +       count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
>>>> +       if (count < 0 && count != -EINVAL)
>>>> +               return count;
>>>> +
>>>> +       if (count > 0) {
>>>
>>>> +               if (count > ARRAY_SIZE(hw_margin))
>>>> +                       return -EINVAL;
>>>
>>> Why double check? You may move it out of the (count > 0).
>>
>> Two checks will always be needed, so I don't entirely see
>> how that would be better.
> 
> But not nested. That's my point:
> 
> if (count > ARRAY_SIZE())
>    return ...
> if (count > 0)
>    ...
> 

The old code has either 1 or two checks if there is no error.
Your suggested code has always two checks. I don't see how that
is an improvement.

>>>> -       if (ret == 1)
>>>> -               hw_margin_max = hw_margin[0];
>>>
>>>> +               ret = device_property_read_u32_array(dev->parent,
>>>> +                                                    "rohm,hw-timeout-ms",
>>>> +                                                    hw_margin, count);
>>>> +               if (ret < 0)
>>>> +                       return ret;
>>>
>>> So, only this needs the count > 0 check since below already has it implicitly.
>>>
>> Sorry, I don't understand this comment.
> 
> if (count > 0) {
>    ret = device_property_read_u32_array(...);
>    ...
> }
> if (count == 1)
>   ...
> if (count == 2)
>   ...
> 
> But here it might be better to have the nested conditionals.
> 

We know that count is either 1 or 2 here, so strictly speaking
	if (count == 1) {
	} else {
	}
would be sufficient. On the other side, that depends on ARRAY_SIZE() being
exactly 2, so
	if (count == 1) {
	} else if (count == 2) {
	}
would also make sense. Either way is fine with me. I'll leave it up
to Dmitry to decide what he wants to do.

Thanks,
Guenter

>>>> -       if (ret == 2) {
>>>> -               hw_margin_max = hw_margin[1];
>>>> -               hw_margin_min = hw_margin[0];
>>>> +               if (count == 1)
>>>> +                       hw_margin_max = hw_margin[0];
>>>> +
>>>> +               if (count == 2) {
>>>> +                       hw_margin_max = hw_margin[1];
>>>> +                       hw_margin_min = hw_margin[0];
>>>> +               }
>>>>           }
> 

Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

[Dmitry Torokhov]

On Mon, Sep 05, 2022 at 08:49:58AM -0700, Guenter Roeck wrote:
> On 9/5/22 08:21, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > On 9/5/22 04:09, Andy Shevchenko wrote:
> > > > On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > 
> > ...
> > 
> > > > > +       count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
> > > > > +       if (count < 0 && count != -EINVAL)
> > > > > +               return count;
> > > > > +
> > > > > +       if (count > 0) {
> > > > 
> > > > > +               if (count > ARRAY_SIZE(hw_margin))
> > > > > +                       return -EINVAL;
> > > > 
> > > > Why double check? You may move it out of the (count > 0).
> >
> > > Two checks will always be needed, so I don't entirely see
> > > how that would be better.
> > 
> > But not nested. That's my point:
> > 
> > if (count > ARRAY_SIZE())
> >    return ...
> > if (count > 0)
> >    ...
> > 
> 
> The old code has either 1 or two checks if there is no error.
> Your suggested code has always two checks. I don't see how that
> is an improvement.
> 
> > > > > -       if (ret == 1)
> > > > > -               hw_margin_max = hw_margin[0];
> > > > 
> > > > > +               ret = device_property_read_u32_array(dev->parent,
> > > > > +                                                    "rohm,hw-timeout-ms",
> > > > > +                                                    hw_margin, count);
> > > > > +               if (ret < 0)
> > > > > +                       return ret;
> > > > 
> > > > So, only this needs the count > 0 check since below already has it implicitly.
> > > > 
> > > Sorry, I don't understand this comment.
> > 
> > if (count > 0) {
> >    ret = device_property_read_u32_array(...);
> >    ...
> > }
> > if (count == 1)
> >   ...
> > if (count == 2)
> >   ...
> > 
> > But here it might be better to have the nested conditionals.
> > 
> 
> We know that count is either 1 or 2 here, so strictly speaking
> 	if (count == 1) {
> 	} else {
> 	}
> would be sufficient. On the other side, that depends on ARRAY_SIZE() being
> exactly 2, so
> 	if (count == 1) {
> 	} else if (count == 2) {
> 	}
> would also make sense. Either way is fine with me. I'll leave it up
> to Dmitry to decide what he wants to do.

My goal is to drop usage of devm_gpiod_get_from_of_node(), beyond that I
do not have strong preferences either way really. It is probing code, so
performance is not critical, but I'm obviously satisfied with how the
code looks now, or I would not have sent it.

Thanks.

-- 
Dmitry

Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

[Guenter Roeck]

On 9/5/22 12:47, Dmitry Torokhov wrote:
[ ... ]
>> We know that count is either 1 or 2 here, so strictly speaking
>> 	if (count == 1) {
>> 	} else {
>> 	}
>> would be sufficient. On the other side, that depends on ARRAY_SIZE() being
>> exactly 2, so
>> 	if (count == 1) {
>> 	} else if (count == 2) {
>> 	}
>> would also make sense. Either way is fine with me. I'll leave it up
>> to Dmitry to decide what he wants to do.
> 
> My goal is to drop usage of devm_gpiod_get_from_of_node(), beyond that I
> do not have strong preferences either way really. It is probing code, so
> performance is not critical, but I'm obviously satisfied with how the
> code looks now, or I would not have sent it.
> 

Good point.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

[Dmitry Torokhov]

On Mon, Sep 05, 2022 at 03:09:05PM -0700, Guenter Roeck wrote:
> On 9/5/22 12:47, Dmitry Torokhov wrote:
> [ ... ]
> > > We know that count is either 1 or 2 here, so strictly speaking
> > > 	if (count == 1) {
> > > 	} else {
> > > 	}
> > > would be sufficient. On the other side, that depends on ARRAY_SIZE() being
> > > exactly 2, so
> > > 	if (count == 1) {
> > > 	} else if (count == 2) {
> > > 	}
> > > would also make sense. Either way is fine with me. I'll leave it up
> > > to Dmitry to decide what he wants to do.
> > 
> > My goal is to drop usage of devm_gpiod_get_from_of_node(), beyond that I
> > do not have strong preferences either way really. It is probing code, so
> > performance is not critical, but I'm obviously satisfied with how the
> > code looks now, or I would not have sent it.
> > 
> 
> Good point.
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter, individual patches are going through maintainer's trees, will
you take this one?

Thanks.

-- 
Dmitry

Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

[Linus Walleij]

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
> fwnode property API.
>
> While at it switch the rest of the calls to read properties in
> bd9576_wdt_probe() to the generic device property API as well.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 11/11] gpiolib: of: remove [devm_]gpiod_get_from_of_node() APIs

[Dmitry Torokhov]

Now that everyone is using [devm_]fwnode_gpiod_get[_index]() APIs, we no
longer need to expose OF-specific [devm_]gpiod_get_from_of_node().

Note that we are keeping gpiod_get_from_of_node() but only as a private
to gpiolib function.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 16a696249229..fe9ce6b19f15 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -129,61 +129,6 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_gpiod_get_index);
 
-/**
- * devm_gpiod_get_from_of_node() - obtain a GPIO from an OF node
- * @dev:	device for lifecycle management
- * @node:	handle of the OF node
- * @propname:	name of the DT property representing the GPIO
- * @index:	index of the GPIO to obtain for the consumer
- * @dflags:	GPIO initialization flags
- * @label:	label to attach to the requested GPIO
- *
- * Returns:
- * On successful request the GPIO pin is configured in accordance with
- * provided @dflags.
- *
- * In case of error an ERR_PTR() is returned.
- */
-struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev,
-					      const struct device_node *node,
-					      const char *propname, int index,
-					      enum gpiod_flags dflags,
-					      const char *label)
-{
-	struct gpio_desc **dr;
-	struct gpio_desc *desc;
-
-	desc = gpiod_get_from_of_node(node, propname, index, dflags, label);
-	if (IS_ERR(desc))
-		return desc;
-
-	/*
-	 * For non-exclusive GPIO descriptors, check if this descriptor is
-	 * already under resource management by this device.
-	 */
-	if (dflags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
-		struct devres *dres;
-
-		dres = devres_find(dev, devm_gpiod_release,
-				   devm_gpiod_match, &desc);
-		if (dres)
-			return desc;
-	}
-
-	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
-			  GFP_KERNEL);
-	if (!dr) {
-		gpiod_put(desc);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	*dr = desc;
-	devres_add(dev, dr);
-
-	return desc;
-}
-EXPORT_SYMBOL_GPL(devm_gpiod_get_from_of_node);
-
 /**
  * devm_fwnode_gpiod_get_index - get a GPIO descriptor from a given node
  * @dev:	GPIO consumer
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index a037b50bef33..b0e0723b12ab 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -365,7 +365,6 @@ struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
 
 	return desc;
 }
-EXPORT_SYMBOL_GPL(gpiod_get_from_of_node);
 
 /*
  * The SPI GPIO bindings happened before we managed to establish that GPIO
diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
index 8af2bc899aab..e0da568d6da3 100644
--- a/drivers/gpio/gpiolib-of.h
+++ b/drivers/gpio/gpiolib-of.h
@@ -3,6 +3,7 @@
 #ifndef GPIOLIB_OF_H
 #define GPIOLIB_OF_H
 
+struct device_node;
 struct gpio_chip;
 enum of_gpio_flags;
 
@@ -16,6 +17,10 @@ void of_gpiochip_remove(struct gpio_chip *gc);
 int of_gpio_get_count(struct device *dev, const char *con_id);
 bool of_gpio_need_valid_mask(const struct gpio_chip *gc);
 void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev);
+struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
+					 const char *propname, int index,
+					 enum gpiod_flags dflags,
+					 const char *label);
 #else
 static inline struct gpio_desc *of_find_gpio(struct device *dev,
 					     const char *con_id,
@@ -38,6 +43,14 @@ static inline void of_gpio_dev_init(struct gpio_chip *gc,
 				    struct gpio_device *gdev)
 {
 }
+static inline
+struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
+					 const char *propname, int index,
+					 enum gpiod_flags dflags,
+					 const char *label)
+{
+	return ERR_PTR(-ENOSYS);
+}
 #endif /* CONFIG_OF_GPIO */
 
 extern struct notifier_block gpio_of_notifier;
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index fe0f460d9a3b..37448ee17e81 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -615,54 +615,6 @@ struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev,
 	return devm_fwnode_gpiod_get_index(dev, child, con_id, 0, flags, label);
 }
 
-#if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_OF_GPIO)
-struct device_node;
-
-struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
-					 const char *propname, int index,
-					 enum gpiod_flags dflags,
-					 const char *label);
-
-#else  /* CONFIG_GPIOLIB && CONFIG_OF_GPIO */
-
-struct device_node;
-
-static inline
-struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
-					 const char *propname, int index,
-					 enum gpiod_flags dflags,
-					 const char *label)
-{
-	return ERR_PTR(-ENOSYS);
-}
-
-#endif /* CONFIG_GPIOLIB && CONFIG_OF_GPIO */
-
-#ifdef CONFIG_GPIOLIB
-struct device_node;
-
-struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev,
-					      const struct device_node *node,
-					      const char *propname, int index,
-					      enum gpiod_flags dflags,
-					      const char *label);
-
-#else  /* CONFIG_GPIOLIB */
-
-struct device_node;
-
-static inline
-struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev,
-					      const struct device_node *node,
-					      const char *propname, int index,
-					      enum gpiod_flags dflags,
-					      const char *label)
-{
-	return ERR_PTR(-ENOSYS);
-}
-
-#endif /* CONFIG_GPIOLIB */
-
 struct acpi_gpio_params {
 	unsigned int crs_entry_index;
 	unsigned int line_index;

-- 
b4 0.10.0-dev-fc921

Re: [PATCH v1 11/11] gpiolib: of: remove [devm_]gpiod_get_from_of_node() APIs

[Linus Walleij]

On Mon, Sep 5, 2022 at 8:31 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> Now that everyone is using [devm_]fwnode_gpiod_get[_index]() APIs, we no
> longer need to expose OF-specific [devm_]gpiod_get_from_of_node().
>
> Note that we are keeping gpiod_get_from_of_node() but only as a private
> to gpiolib function.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v1 00/11] Get rid of [devm_]gpiod_get_from_of_node() public APIs

[Greg Kroah-Hartman]

On Sun, Sep 04, 2022 at 11:30:52PM -0700, 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. We can do that by switching drivers
> to use generic fwnode API ([devm_]fwnode_gpiod_get()). By doing so we open
> the door to augmenting device tree and ACPI information through secondary
> software properties (once we teach gpiolib how to handle those).
> 
> I hope that relevant maintainers will take patches through their trees and
> then we could merge the last one some time after -rc1.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Nice work, I'll go add the USB ones to my tree now, thanks!

greg k-h

Re: [PATCH v1 00/11] Get rid of [devm_]gpiod_get_from_of_node() public APIs

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 9:32 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. We can do that by switching drivers
> to use generic fwnode API ([devm_]fwnode_gpiod_get()). By doing so we open
> the door to augmenting device tree and ACPI information through secondary
> software properties (once we teach gpiolib how to handle those).
>
> I hope that relevant maintainers will take patches through their trees and
> then we could merge the last one some time after -rc1.

I'm in favour of the series, but some comments would be good to be addressed.

-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH v1 00/11] Get rid of [devm_]gpiod_get_from_of_node() public APIs

[Lorenzo Pieralisi]

On Sun, 4 Sep 2022 23:30:52 -0700, 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. We can do that by switching drivers
> to use generic fwnode API ([devm_]fwnode_gpiod_get()). By doing so we open
> the door to augmenting device tree and ACPI information through secondary
> software properties (once we teach gpiolib how to handle those).
> 
> I hope that relevant maintainers will take patches through their trees and
> then we could merge the last one some time after -rc1.
> 
> [...]

Applied to pci/tegra, thanks!

[01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get
        https://git.kernel.org/lpieralisi/pci/c/16e3f4077965

Thanks,
Lorenzo

Re: (subset) [PATCH v1 00/11] Get rid of [devm_]gpiod_get_from_of_node() public APIs

[Dmitry Torokhov]

Hi Lorenzo,

On Thu, Oct 27, 2022 at 03:38:11PM +0200, Lorenzo Pieralisi wrote:
> On Sun, 4 Sep 2022 23:30:52 -0700, 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. We can do that by switching drivers
> > to use generic fwnode API ([devm_]fwnode_gpiod_get()). By doing so we open
> > the door to augmenting device tree and ACPI information through secondary
> > software properties (once we teach gpiolib how to handle those).
> > 
> > I hope that relevant maintainers will take patches through their trees and
> > then we could merge the last one some time after -rc1.
> > 
> > [...]
> 
> Applied to pci/tegra, thanks!
> 
> [01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get
>         https://git.kernel.org/lpieralisi/pci/c/16e3f4077965

Any chance you could also pick up

 [06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()
 (20220903-gpiod_get_from_of_node-remove-v1-6-b29adfb27a6c@gmail.com)

 - Pali Rohár has acked it.

Thanks!

-- 
Dmitry