[04/11] net: phylink: switch to using fwnode_gpiod_get_index()
diff mbox series

Message ID 20190911075215.78047-5-dmitry.torokhov@gmail.com
State New
Headers show
Series
  • Add support for software nodes to gpiolib
Related show

Commit Message

Dmitry Torokhov Sept. 11, 2019, 7:52 a.m. UTC
Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use
the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit
works with arbitrary firmware node.

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

 drivers/net/phy/phylink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko Sept. 11, 2019, 9:25 a.m. UTC | #1
On Wed, Sep 11, 2019 at 12:52:08AM -0700, Dmitry Torokhov wrote:
> Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use
> the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit
> works with arbitrary firmware node.

I'm wondering if it's possible to step forward and replace
fwnode_get_gpiod_index by gpiod_get() / gpiod_get_index() here and
in other cases in this series.
Russell King - ARM Linux admin Sept. 11, 2019, 9:39 a.m. UTC | #2
On Wed, Sep 11, 2019 at 12:25:14PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 11, 2019 at 12:52:08AM -0700, Dmitry Torokhov wrote:
> > Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use
> > the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit
> > works with arbitrary firmware node.
> 
> I'm wondering if it's possible to step forward and replace
> fwnode_get_gpiod_index by gpiod_get() / gpiod_get_index() here and
> in other cases in this series.

No, those require a struct device, but we have none.  There are network
drivers where there is a struct device for the network complex, but only
DT nodes for the individual network interfaces.  So no, gpiod_* really
doesn't work.
Andy Shevchenko Sept. 11, 2019, 9:46 a.m. UTC | #3
On Wed, Sep 11, 2019 at 10:39:14AM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Sep 11, 2019 at 12:25:14PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 11, 2019 at 12:52:08AM -0700, Dmitry Torokhov wrote:
> > > Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use
> > > the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit
> > > works with arbitrary firmware node.
> > 
> > I'm wondering if it's possible to step forward and replace
> > fwnode_get_gpiod_index by gpiod_get() / gpiod_get_index() here and
> > in other cases in this series.
> 
> No, those require a struct device, but we have none.  There are network
> drivers where there is a struct device for the network complex, but only
> DT nodes for the individual network interfaces.  So no, gpiod_* really
> doesn't work.

In the following patch the node is derived from struct device. So, I believe
some cases can be handled differently.
Russell King - ARM Linux admin Sept. 11, 2019, 9:49 a.m. UTC | #4
On Wed, Sep 11, 2019 at 12:46:19PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 11, 2019 at 10:39:14AM +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Sep 11, 2019 at 12:25:14PM +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 11, 2019 at 12:52:08AM -0700, Dmitry Torokhov wrote:
> > > > Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use
> > > > the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit
> > > > works with arbitrary firmware node.
> > > 
> > > I'm wondering if it's possible to step forward and replace
> > > fwnode_get_gpiod_index by gpiod_get() / gpiod_get_index() here and
> > > in other cases in this series.
> > 
> > No, those require a struct device, but we have none.  There are network
> > drivers where there is a struct device for the network complex, but only
> > DT nodes for the individual network interfaces.  So no, gpiod_* really
> > doesn't work.
> 
> In the following patch the node is derived from struct device. So, I believe
> some cases can be handled differently.

phylink is not passed a struct device - it has no knowledge what the
parent device is.

In any case, I do not have "the following patch".
Dmitry Torokhov Sept. 11, 2019, 9:51 a.m. UTC | #5
On Wed, Sep 11, 2019 at 12:46:19PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 11, 2019 at 10:39:14AM +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Sep 11, 2019 at 12:25:14PM +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 11, 2019 at 12:52:08AM -0700, Dmitry Torokhov wrote:
> > > > Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use
> > > > the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit
> > > > works with arbitrary firmware node.
> > > 
> > > I'm wondering if it's possible to step forward and replace
> > > fwnode_get_gpiod_index by gpiod_get() / gpiod_get_index() here and
> > > in other cases in this series.
> > 
> > No, those require a struct device, but we have none.  There are network
> > drivers where there is a struct device for the network complex, but only
> > DT nodes for the individual network interfaces.  So no, gpiod_* really
> > doesn't work.
> 
> In the following patch the node is derived from struct device. So, I believe
> some cases can be handled differently.

If we are willing to sacrifice the custom label for the GPIO that
fwnode_gpiod_get_index() allows us to set, then there are several
drivers that could actually use gpiod_get() API.

This is up to the dirver's maintainers...

Thanks.
Dmitry Torokhov Sept. 11, 2019, 9:55 a.m. UTC | #6
On Wed, Sep 11, 2019 at 10:49:29AM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Sep 11, 2019 at 12:46:19PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 11, 2019 at 10:39:14AM +0100, Russell King - ARM Linux admin wrote:
> > > On Wed, Sep 11, 2019 at 12:25:14PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Sep 11, 2019 at 12:52:08AM -0700, Dmitry Torokhov wrote:
> > > > > Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use
> > > > > the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit
> > > > > works with arbitrary firmware node.
> e > > 
> > > > I'm wondering if it's possible to step forward and replace
> > > > fwnode_get_gpiod_index by gpiod_get() / gpiod_get_index() here and
> > > > in other cases in this series.
> > > 
> > > No, those require a struct device, but we have none.  There are network
> > > drivers where there is a struct device for the network complex, but only
> > > DT nodes for the individual network interfaces.  So no, gpiod_* really
> > > doesn't work.
> > 
> > In the following patch the node is derived from struct device. So, I believe
> > some cases can be handled differently.
> 
> phylink is not passed a struct device - it has no knowledge what the
> parent device is.
> 
> In any case, I do not have "the following patch".

Andy is talking about this one:

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index ce940871331e..9ca51d678123 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -46,8 +46,8 @@ static int mdiobus_register_gpiod(struct mdio_device *mdiodev)

        /* Deassert the optional reset signal */
        if (mdiodev->dev.of_node)
-               gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
-                                              "reset-gpios", 0,
                                               GPIOD_OUT_LOW,
+               gpiod = fwnode_gpiod_get_index(&mdiodev->dev.of_node->fwnode,
+                                              "reset", 0, GPIOD_OUT_LOW,
                                               "PHY reset");
Here if we do not care about "PHY reset" label, we could use
gpiod_get(&mdiodev->dev, "reset", GPIOD_OUT_LOW).

Thanks.
Russell King - ARM Linux admin Sept. 11, 2019, 10:10 a.m. UTC | #7
On Wed, Sep 11, 2019 at 02:55:11AM -0700, Dmitry Torokhov wrote:
> On Wed, Sep 11, 2019 at 10:49:29AM +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Sep 11, 2019 at 12:46:19PM +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 11, 2019 at 10:39:14AM +0100, Russell King - ARM Linux admin wrote:
> > > > On Wed, Sep 11, 2019 at 12:25:14PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, Sep 11, 2019 at 12:52:08AM -0700, Dmitry Torokhov wrote:
> > > > > > Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use
> > > > > > the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit
> > > > > > works with arbitrary firmware node.
> > e > > 
> > > > > I'm wondering if it's possible to step forward and replace
> > > > > fwnode_get_gpiod_index by gpiod_get() / gpiod_get_index() here and
> > > > > in other cases in this series.
> > > > 
> > > > No, those require a struct device, but we have none.  There are network
> > > > drivers where there is a struct device for the network complex, but only
> > > > DT nodes for the individual network interfaces.  So no, gpiod_* really
> > > > doesn't work.
> > > 
> > > In the following patch the node is derived from struct device. So, I believe
> > > some cases can be handled differently.
> > 
> > phylink is not passed a struct device - it has no knowledge what the
> > parent device is.
> > 
> > In any case, I do not have "the following patch".
> 
> Andy is talking about this one:
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index ce940871331e..9ca51d678123 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -46,8 +46,8 @@ static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
> 
>         /* Deassert the optional reset signal */
>         if (mdiodev->dev.of_node)
> -               gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
> -                                              "reset-gpios", 0,
>                                                GPIOD_OUT_LOW,
> +               gpiod = fwnode_gpiod_get_index(&mdiodev->dev.of_node->fwnode,
> +                                              "reset", 0, GPIOD_OUT_LOW,
>                                                "PHY reset");
> Here if we do not care about "PHY reset" label, we could use
> gpiod_get(&mdiodev->dev, "reset", GPIOD_OUT_LOW).

Here, you have a struct device, so yes, it's possible.

Referring back to my comment, notice that I said we have none for the
phylink case, so it's not possible there.

I'm not sure why Andy replied the way he did, unless he mis-read my
comment.
Andy Shevchenko Sept. 11, 2019, 4:52 p.m. UTC | #8
On Wed, Sep 11, 2019 at 11:10:16AM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Sep 11, 2019 at 02:55:11AM -0700, Dmitry Torokhov wrote:
> > On Wed, Sep 11, 2019 at 10:49:29AM +0100, Russell King - ARM Linux admin wrote:
> > > On Wed, Sep 11, 2019 at 12:46:19PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Sep 11, 2019 at 10:39:14AM +0100, Russell King - ARM Linux admin wrote:
> > > > > On Wed, Sep 11, 2019 at 12:25:14PM +0300, Andy Shevchenko wrote:
> > > > > > On Wed, Sep 11, 2019 at 12:52:08AM -0700, Dmitry Torokhov wrote:
> > > > > > > Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use
> > > > > > > the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit
> > > > > > > works with arbitrary firmware node.
> > > e > > 
> > > > > > I'm wondering if it's possible to step forward and replace
> > > > > > fwnode_get_gpiod_index by gpiod_get() / gpiod_get_index() here and
> > > > > > in other cases in this series.
> > > > > 
> > > > > No, those require a struct device, but we have none.  There are network
> > > > > drivers where there is a struct device for the network complex, but only
> > > > > DT nodes for the individual network interfaces.  So no, gpiod_* really
> > > > > doesn't work.
> > > > 
> > > > In the following patch the node is derived from struct device. So, I believe
> > > > some cases can be handled differently.

> Referring back to my comment, notice that I said we have none for the
> phylink case, so it's not possible there.
> 
> I'm not sure why Andy replied the way he did, unless he mis-read my
> comment.

It is a first patch which does the change. Mostly my reply was to Dmitry and
your comment clarifies the case with this patch, thanks!
Linus Walleij Sept. 12, 2019, 9:41 a.m. UTC | #9
On Wed, Sep 11, 2019 at 10:51 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> If we are willing to sacrifice the custom label for the GPIO that
> fwnode_gpiod_get_index() allows us to set, then there are several
> drivers that could actually use gpiod_get() API.

We have:
gpiod_set_consumer_name(gpiod, "name");
to deal with that so no sacrifice is needed.

Yours,
Linus Walleij
Andy Shevchenko Sept. 12, 2019, 1:44 p.m. UTC | #10
On Thu, Sep 12, 2019 at 10:41:43AM +0100, Linus Walleij wrote:
> On Wed, Sep 11, 2019 at 10:51 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > If we are willing to sacrifice the custom label for the GPIO that
> > fwnode_gpiod_get_index() allows us to set, then there are several
> > drivers that could actually use gpiod_get() API.
> 
> We have:
> gpiod_set_consumer_name(gpiod, "name");
> to deal with that so no sacrifice is needed.

Thank for this hint!
Russell King - ARM Linux admin Sept. 12, 2019, 1:52 p.m. UTC | #11
On Thu, Sep 12, 2019 at 04:44:29PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 12, 2019 at 10:41:43AM +0100, Linus Walleij wrote:
> > On Wed, Sep 11, 2019 at 10:51 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > 
> > > If we are willing to sacrifice the custom label for the GPIO that
> > > fwnode_gpiod_get_index() allows us to set, then there are several
> > > drivers that could actually use gpiod_get() API.
> > 
> > We have:
> > gpiod_set_consumer_name(gpiod, "name");
> > to deal with that so no sacrifice is needed.
> 
> Thank for this hint!

Would it be possible to improve your email etiquette, and move this
discussion to a more appropriate subject line, so I don't have to keep
checking these emails, in case you _do_ talk about something relevent
to the original patch that the subject line refers to?

Thanks.

Patch
diff mbox series

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a45c5de96ab1..14b608991445 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -168,8 +168,8 @@  static int phylink_parse_fixedlink(struct phylink *pl,
 			pl->link_config.pause |= MLO_PAUSE_ASYM;
 
 		if (ret == 0) {
-			desc = fwnode_get_named_gpiod(fixed_node, "link-gpios",
-						      0, GPIOD_IN, "?");
+			desc = fwnode_gpiod_get_index(fixed_node, "link", 0,
+						      GPIOD_IN, "?");
 
 			if (!IS_ERR(desc))
 				pl->link_gpio = desc;