| Message ID | 20190911075215.78047-5-dmitry.torokhov@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for software nodes to gpiolib | expand |
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.
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.
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.
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".
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.
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.
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.
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!
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
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!
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.
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;
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(-)