diff mbox series

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

Message ID 20220903-gpiod_get_from_of_node-remove-v1-4-b29adfb27a6c@gmail.com (mailing list archive)
State New, archived
Headers show
Series Get rid of [devm_]gpiod_get_from_of_node() public APIs | expand

Commit Message

Dmitry Torokhov Sept. 5, 2022, 6:30 a.m. UTC
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>

Comments

Andy Shevchenko Sept. 5, 2022, 10:59 a.m. UTC | #1
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;
>                 }
Dmitry Torokhov Sept. 5, 2022, 7:39 p.m. UTC | #2
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.
Andy Shevchenko Sept. 5, 2022, 7:41 p.m. UTC | #3
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?
Dmitry Torokhov Sept. 5, 2022, 7:51 p.m. UTC | #4
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.
Andy Shevchenko Sept. 5, 2022, 7:55 p.m. UTC | #5
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.
Guenter Roeck Sept. 5, 2022, 10:07 p.m. UTC | #6
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
Andy Shevchenko Sept. 6, 2022, 12:27 p.m. UTC | #7
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.
diff mbox series

Patch

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,