| Message ID | 20190206205106.11517-1-mdf@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c | expand |
On Wed, Feb 06, 2019 at 12:51:06PM -0800, Moritz Fischer wrote: > Move the DT based link GPIO parsing to of_mdio and let the places > that register a fixed_phy pass in a GPIO descriptor or NULL. > > This allows fixed_phy on non-DT platforms to have link GPIOs, too. > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > --- > drivers/net/dsa/dsa_loop.c | 2 +- > drivers/net/ethernet/broadcom/bgmac.c | 2 +- > drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +- > drivers/net/phy/fixed_phy.c | 48 ++------------------ > drivers/net/usb/lan78xx.c | 2 +- > drivers/of/of_mdio.c | 15 +++++- > include/linux/phy_fixed.h | 3 +- > 7 files changed, 23 insertions(+), 51 deletions(-) > > diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c > index 17482ae09aa5..7f124c620092 100644 > --- a/drivers/net/dsa/dsa_loop.c > +++ b/drivers/net/dsa/dsa_loop.c > @@ -343,7 +343,7 @@ static int __init dsa_loop_init(void) > unsigned int i; > > for (i = 0; i < NUM_FIXED_PHYS; i++) > - phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL); > + phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL, NULL); > > return mdio_driver_register(&dsa_loop_drv); > } > diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c > index 4632dd5dbad1..bce644dec5c2 100644 > --- a/drivers/net/ethernet/broadcom/bgmac.c > +++ b/drivers/net/ethernet/broadcom/bgmac.c > @@ -1446,7 +1446,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac) > struct phy_device *phy_dev; > int err; > > - phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); > + phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); > if (!phy_dev || IS_ERR(phy_dev)) { > dev_err(bgmac->dev, "Failed to register fixed PHY device\n"); > return -ENODEV; > diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c > index 51880d83131a..7cbd737aba80 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c > @@ -525,7 +525,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv) > .asym_pause = 0, > }; > > - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); > + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); > if (!phydev || IS_ERR(phydev)) { > dev_err(kdev, "failed to register fixed PHY device\n"); > return -ENODEV; > diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c > index d810f914aaa4..845bd7c2065a 100644 > --- a/drivers/net/phy/fixed_phy.c > +++ b/drivers/net/phy/fixed_phy.c > @@ -18,6 +18,7 @@ > #include <linux/err.h> > #include <linux/slab.h> > #include <linux/of.h> > +#include <linux/of_mdio.h> > #include <linux/gpio/consumer.h> > #include <linux/seqlock.h> > #include <linux/idr.h> > @@ -191,50 +192,12 @@ static void fixed_phy_del(int phy_addr) > } > } > > -#ifdef CONFIG_OF_GPIO > -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) > -{ > - struct device_node *fixed_link_node; > - struct gpio_desc *gpiod; > - > - if (!np) > - return NULL; > - > - fixed_link_node = of_get_child_by_name(np, "fixed-link"); > - if (!fixed_link_node) > - return NULL; > - > - /* > - * As the fixed link is just a device tree node without any > - * Linux device associated with it, we simply have obtain > - * the GPIO descriptor from the device tree like this. > - */ > - gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0, > - GPIOD_IN, "mdio"); > - of_node_put(fixed_link_node); > - if (IS_ERR(gpiod)) { > - if (PTR_ERR(gpiod) == -EPROBE_DEFER) > - return gpiod; > - pr_err("error getting GPIO for fixed link %pOF, proceed without\n", > - fixed_link_node); > - gpiod = NULL; > - } > - > - return gpiod; > -} > -#else > -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) > -{ > - return NULL; > -} > -#endif > - > struct phy_device *fixed_phy_register(unsigned int irq, > struct fixed_phy_status *status, > - struct device_node *np) > + struct device_node *np, > + struct gpio_desc *gpiod) > { > struct fixed_mdio_bus *fmb = &platform_fmb; > - struct gpio_desc *gpiod = NULL; > struct phy_device *phy; > int phy_addr; > int ret; > @@ -242,11 +205,6 @@ struct phy_device *fixed_phy_register(unsigned int irq, > if (!fmb->mii_bus || fmb->mii_bus->state != MDIOBUS_REGISTERED) > return ERR_PTR(-EPROBE_DEFER); > > - /* Check if we have a GPIO associated with this fixed phy */ > - gpiod = fixed_phy_get_gpiod(np); > - if (IS_ERR(gpiod)) > - return ERR_CAST(gpiod); > - > /* Get the next available PHY address, up to PHY_MAX_ADDR */ > phy_addr = ida_simple_get(&phy_fixed_ida, 0, PHY_MAX_ADDR, GFP_KERNEL); > if (phy_addr < 0) > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > index 3d92ea6fcc02..bd88f0aef2fa 100644 > --- a/drivers/net/usb/lan78xx.c > +++ b/drivers/net/usb/lan78xx.c > @@ -2051,7 +2051,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev) > phydev = phy_find_first(dev->mdiobus); > if (!phydev) { > netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n"); > - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); > + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); > if (IS_ERR(phydev)) { > netdev_err(dev->net, "No PHY/fixed_PHY found\n"); > return NULL; > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index de6157357e26..6be2120b5f03 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -20,6 +20,7 @@ > #include <linux/of_mdio.h> > #include <linux/of_net.h> > #include <linux/module.h> > +#include <linux/gpio/consumer.h> > > #define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */ > > @@ -460,6 +461,7 @@ int of_phy_register_fixed_link(struct device_node *np) > { > struct fixed_phy_status status = {}; > struct device_node *fixed_link_node; > + struct gpio_desc *gpiod = NULL; > u32 fixed_link_prop[5]; > const char *managed; > > @@ -483,7 +485,17 @@ int of_phy_register_fixed_link(struct device_node *np) > status.pause = of_property_read_bool(fixed_link_node, "pause"); > status.asym_pause = of_property_read_bool(fixed_link_node, > "asym-pause"); > + > + gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0, > + GPIOD_IN, "mdio"); > of_node_put(fixed_link_node); > + if (IS_ERR(gpiod)) { > + if (PTR_ERR(gpiod) == -EPROBE_DEFER) > + return PTR_ERR(gpiod); > + pr_err("error getting GPIO for fixed link %pOF, proceed without\n", > + fixed_link_node); > + gpiod = NULL; > extern struct phy_device *fixed_phy_register(unsigned int irq, > struct fixed_phy_status *status, > - struct device_node *np); > + struct device_node *np, > + struct gpio_desc *gpiod); Hi Moritz I think it would be better to add a extern struct phy_device *fixed_phy_register_gpiod(unsigned int irq, struct fixed_phy_status *status, struct gpio_desc *gpiod); If you are not using DT, the np is pointless. So lets keep the API simple. Andrew
Hi Andrew, thanks for your feedback. On Wed, Feb 06, 2019 at 10:53:22PM +0100, Andrew Lunn wrote: > On Wed, Feb 06, 2019 at 12:51:06PM -0800, Moritz Fischer wrote: > > Move the DT based link GPIO parsing to of_mdio and let the places > > that register a fixed_phy pass in a GPIO descriptor or NULL. > > > > This allows fixed_phy on non-DT platforms to have link GPIOs, too. > > > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > > --- > > drivers/net/dsa/dsa_loop.c | 2 +- > > drivers/net/ethernet/broadcom/bgmac.c | 2 +- > > drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +- > > drivers/net/phy/fixed_phy.c | 48 ++------------------ > > drivers/net/usb/lan78xx.c | 2 +- > > drivers/of/of_mdio.c | 15 +++++- > > include/linux/phy_fixed.h | 3 +- > > 7 files changed, 23 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c > > index 17482ae09aa5..7f124c620092 100644 > > --- a/drivers/net/dsa/dsa_loop.c > > +++ b/drivers/net/dsa/dsa_loop.c > > @@ -343,7 +343,7 @@ static int __init dsa_loop_init(void) > > unsigned int i; > > > > for (i = 0; i < NUM_FIXED_PHYS; i++) > > - phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL); > > + phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL, NULL); > > > > return mdio_driver_register(&dsa_loop_drv); > > } > > diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c > > index 4632dd5dbad1..bce644dec5c2 100644 > > --- a/drivers/net/ethernet/broadcom/bgmac.c > > +++ b/drivers/net/ethernet/broadcom/bgmac.c > > @@ -1446,7 +1446,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac) > > struct phy_device *phy_dev; > > int err; > > > > - phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); > > + phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); > > if (!phy_dev || IS_ERR(phy_dev)) { > > dev_err(bgmac->dev, "Failed to register fixed PHY device\n"); > > return -ENODEV; > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c > > index 51880d83131a..7cbd737aba80 100644 > > --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c > > +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c > > @@ -525,7 +525,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv) > > .asym_pause = 0, > > }; > > > > - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); > > + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); > > if (!phydev || IS_ERR(phydev)) { > > dev_err(kdev, "failed to register fixed PHY device\n"); > > return -ENODEV; > > diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c > > index d810f914aaa4..845bd7c2065a 100644 > > --- a/drivers/net/phy/fixed_phy.c > > +++ b/drivers/net/phy/fixed_phy.c > > @@ -18,6 +18,7 @@ > > #include <linux/err.h> > > #include <linux/slab.h> > > #include <linux/of.h> > > +#include <linux/of_mdio.h> > > #include <linux/gpio/consumer.h> > > #include <linux/seqlock.h> > > #include <linux/idr.h> > > @@ -191,50 +192,12 @@ static void fixed_phy_del(int phy_addr) > > } > > } > > > > -#ifdef CONFIG_OF_GPIO > > -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) > > -{ > > - struct device_node *fixed_link_node; > > - struct gpio_desc *gpiod; > > - > > - if (!np) > > - return NULL; > > - > > - fixed_link_node = of_get_child_by_name(np, "fixed-link"); > > - if (!fixed_link_node) > > - return NULL; > > - > > - /* > > - * As the fixed link is just a device tree node without any > > - * Linux device associated with it, we simply have obtain > > - * the GPIO descriptor from the device tree like this. > > - */ > > - gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0, > > - GPIOD_IN, "mdio"); > > - of_node_put(fixed_link_node); > > - if (IS_ERR(gpiod)) { > > - if (PTR_ERR(gpiod) == -EPROBE_DEFER) > > - return gpiod; > > - pr_err("error getting GPIO for fixed link %pOF, proceed without\n", > > - fixed_link_node); > > - gpiod = NULL; > > - } > > - > > - return gpiod; > > -} > > -#else > > -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) > > -{ > > - return NULL; > > -} > > -#endif > > - > > struct phy_device *fixed_phy_register(unsigned int irq, > > struct fixed_phy_status *status, > > - struct device_node *np) > > + struct device_node *np, > > + struct gpio_desc *gpiod) > > { > > struct fixed_mdio_bus *fmb = &platform_fmb; > > - struct gpio_desc *gpiod = NULL; > > struct phy_device *phy; > > int phy_addr; > > int ret; > > @@ -242,11 +205,6 @@ struct phy_device *fixed_phy_register(unsigned int irq, > > if (!fmb->mii_bus || fmb->mii_bus->state != MDIOBUS_REGISTERED) > > return ERR_PTR(-EPROBE_DEFER); > > > > - /* Check if we have a GPIO associated with this fixed phy */ > > - gpiod = fixed_phy_get_gpiod(np); > > - if (IS_ERR(gpiod)) > > - return ERR_CAST(gpiod); > > - > > /* Get the next available PHY address, up to PHY_MAX_ADDR */ > > phy_addr = ida_simple_get(&phy_fixed_ida, 0, PHY_MAX_ADDR, GFP_KERNEL); > > if (phy_addr < 0) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > > index 3d92ea6fcc02..bd88f0aef2fa 100644 > > --- a/drivers/net/usb/lan78xx.c > > +++ b/drivers/net/usb/lan78xx.c > > @@ -2051,7 +2051,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev) > > phydev = phy_find_first(dev->mdiobus); > > if (!phydev) { > > netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n"); > > - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); > > + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); > > if (IS_ERR(phydev)) { > > netdev_err(dev->net, "No PHY/fixed_PHY found\n"); > > return NULL; > > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > > index de6157357e26..6be2120b5f03 100644 > > --- a/drivers/of/of_mdio.c > > +++ b/drivers/of/of_mdio.c > > @@ -20,6 +20,7 @@ > > #include <linux/of_mdio.h> > > #include <linux/of_net.h> > > #include <linux/module.h> > > +#include <linux/gpio/consumer.h> > > > > #define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */ > > > > @@ -460,6 +461,7 @@ int of_phy_register_fixed_link(struct device_node *np) > > { > > struct fixed_phy_status status = {}; > > struct device_node *fixed_link_node; > > + struct gpio_desc *gpiod = NULL; > > u32 fixed_link_prop[5]; > > const char *managed; > > > > @@ -483,7 +485,17 @@ int of_phy_register_fixed_link(struct device_node *np) > > status.pause = of_property_read_bool(fixed_link_node, "pause"); > > status.asym_pause = of_property_read_bool(fixed_link_node, > > "asym-pause"); > > + > > + gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0, > > + GPIOD_IN, "mdio"); > > of_node_put(fixed_link_node); > > + if (IS_ERR(gpiod)) { > > + if (PTR_ERR(gpiod) == -EPROBE_DEFER) > > + return PTR_ERR(gpiod); > > + pr_err("error getting GPIO for fixed link %pOF, proceed without\n", > > + fixed_link_node); > > + gpiod = NULL; > > extern struct phy_device *fixed_phy_register(unsigned int irq, > > struct fixed_phy_status *status, > > - struct device_node *np); > > + struct device_node *np, > > + struct gpio_desc *gpiod); > > Hi Moritz > > I think it would be better to add a > > extern struct phy_device *fixed_phy_register_gpiod(unsigned int irq, > struct fixed_phy_status *status, > struct gpio_desc *gpiod); > > If you are not using DT, the np is pointless. So lets keep the API > simple. To clarify: Do you agree with moving the parsing to of_mdio, or do you want to keep the parsing as it was and just add an additional function. In the latter case you'd have a static struct phy_device *__fixed_phy_register(unsigned int irq, struct fixed_phy_status *status, struct device_node *np, struct gpio_desc *gpiod) { if (gpiod) use that else old behavior / scan dt? } That we then call from fixed_phy_register_gpiod() with a np of NULL, and from fixed_phy_register() with a gpiod of NULL? Thanks, Moritz
On 2/6/19 2:38 PM, Moritz Fischer wrote: > Hi Andrew, > > thanks for your feedback. > > On Wed, Feb 06, 2019 at 10:53:22PM +0100, Andrew Lunn wrote: >> On Wed, Feb 06, 2019 at 12:51:06PM -0800, Moritz Fischer wrote: >>> Move the DT based link GPIO parsing to of_mdio and let the places >>> that register a fixed_phy pass in a GPIO descriptor or NULL. >>> >>> This allows fixed_phy on non-DT platforms to have link GPIOs, too. >>> >>> Signed-off-by: Moritz Fischer <mdf@kernel.org> >>> --- >>> drivers/net/dsa/dsa_loop.c | 2 +- >>> drivers/net/ethernet/broadcom/bgmac.c | 2 +- >>> drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +- >>> drivers/net/phy/fixed_phy.c | 48 ++------------------ >>> drivers/net/usb/lan78xx.c | 2 +- >>> drivers/of/of_mdio.c | 15 +++++- >>> include/linux/phy_fixed.h | 3 +- >>> 7 files changed, 23 insertions(+), 51 deletions(-) >>> >>> diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c >>> index 17482ae09aa5..7f124c620092 100644 >>> --- a/drivers/net/dsa/dsa_loop.c >>> +++ b/drivers/net/dsa/dsa_loop.c >>> @@ -343,7 +343,7 @@ static int __init dsa_loop_init(void) >>> unsigned int i; >>> >>> for (i = 0; i < NUM_FIXED_PHYS; i++) >>> - phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL); >>> + phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL, NULL); >>> >>> return mdio_driver_register(&dsa_loop_drv); >>> } >>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c >>> index 4632dd5dbad1..bce644dec5c2 100644 >>> --- a/drivers/net/ethernet/broadcom/bgmac.c >>> +++ b/drivers/net/ethernet/broadcom/bgmac.c >>> @@ -1446,7 +1446,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac) >>> struct phy_device *phy_dev; >>> int err; >>> >>> - phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); >>> + phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); >>> if (!phy_dev || IS_ERR(phy_dev)) { >>> dev_err(bgmac->dev, "Failed to register fixed PHY device\n"); >>> return -ENODEV; >>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c >>> index 51880d83131a..7cbd737aba80 100644 >>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c >>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c >>> @@ -525,7 +525,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv) >>> .asym_pause = 0, >>> }; >>> >>> - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); >>> + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); >>> if (!phydev || IS_ERR(phydev)) { >>> dev_err(kdev, "failed to register fixed PHY device\n"); >>> return -ENODEV; >>> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c >>> index d810f914aaa4..845bd7c2065a 100644 >>> --- a/drivers/net/phy/fixed_phy.c >>> +++ b/drivers/net/phy/fixed_phy.c >>> @@ -18,6 +18,7 @@ >>> #include <linux/err.h> >>> #include <linux/slab.h> >>> #include <linux/of.h> >>> +#include <linux/of_mdio.h> >>> #include <linux/gpio/consumer.h> >>> #include <linux/seqlock.h> >>> #include <linux/idr.h> >>> @@ -191,50 +192,12 @@ static void fixed_phy_del(int phy_addr) >>> } >>> } >>> >>> -#ifdef CONFIG_OF_GPIO >>> -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) >>> -{ >>> - struct device_node *fixed_link_node; >>> - struct gpio_desc *gpiod; >>> - >>> - if (!np) >>> - return NULL; >>> - >>> - fixed_link_node = of_get_child_by_name(np, "fixed-link"); >>> - if (!fixed_link_node) >>> - return NULL; >>> - >>> - /* >>> - * As the fixed link is just a device tree node without any >>> - * Linux device associated with it, we simply have obtain >>> - * the GPIO descriptor from the device tree like this. >>> - */ >>> - gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0, >>> - GPIOD_IN, "mdio"); >>> - of_node_put(fixed_link_node); >>> - if (IS_ERR(gpiod)) { >>> - if (PTR_ERR(gpiod) == -EPROBE_DEFER) >>> - return gpiod; >>> - pr_err("error getting GPIO for fixed link %pOF, proceed without\n", >>> - fixed_link_node); >>> - gpiod = NULL; >>> - } >>> - >>> - return gpiod; >>> -} >>> -#else >>> -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) >>> -{ >>> - return NULL; >>> -} >>> -#endif >>> - >>> struct phy_device *fixed_phy_register(unsigned int irq, >>> struct fixed_phy_status *status, >>> - struct device_node *np) >>> + struct device_node *np, >>> + struct gpio_desc *gpiod) >>> { >>> struct fixed_mdio_bus *fmb = &platform_fmb; >>> - struct gpio_desc *gpiod = NULL; >>> struct phy_device *phy; >>> int phy_addr; >>> int ret; >>> @@ -242,11 +205,6 @@ struct phy_device *fixed_phy_register(unsigned int irq, >>> if (!fmb->mii_bus || fmb->mii_bus->state != MDIOBUS_REGISTERED) >>> return ERR_PTR(-EPROBE_DEFER); >>> >>> - /* Check if we have a GPIO associated with this fixed phy */ >>> - gpiod = fixed_phy_get_gpiod(np); >>> - if (IS_ERR(gpiod)) >>> - return ERR_CAST(gpiod); >>> - >>> /* Get the next available PHY address, up to PHY_MAX_ADDR */ >>> phy_addr = ida_simple_get(&phy_fixed_ida, 0, PHY_MAX_ADDR, GFP_KERNEL); >>> if (phy_addr < 0) >>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c >>> index 3d92ea6fcc02..bd88f0aef2fa 100644 >>> --- a/drivers/net/usb/lan78xx.c >>> +++ b/drivers/net/usb/lan78xx.c >>> @@ -2051,7 +2051,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev) >>> phydev = phy_find_first(dev->mdiobus); >>> if (!phydev) { >>> netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n"); >>> - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); >>> + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); >>> if (IS_ERR(phydev)) { >>> netdev_err(dev->net, "No PHY/fixed_PHY found\n"); >>> return NULL; >>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c >>> index de6157357e26..6be2120b5f03 100644 >>> --- a/drivers/of/of_mdio.c >>> +++ b/drivers/of/of_mdio.c >>> @@ -20,6 +20,7 @@ >>> #include <linux/of_mdio.h> >>> #include <linux/of_net.h> >>> #include <linux/module.h> >>> +#include <linux/gpio/consumer.h> >>> >>> #define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */ >>> >>> @@ -460,6 +461,7 @@ int of_phy_register_fixed_link(struct device_node *np) >>> { >>> struct fixed_phy_status status = {}; >>> struct device_node *fixed_link_node; >>> + struct gpio_desc *gpiod = NULL; >>> u32 fixed_link_prop[5]; >>> const char *managed; >>> >>> @@ -483,7 +485,17 @@ int of_phy_register_fixed_link(struct device_node *np) >>> status.pause = of_property_read_bool(fixed_link_node, "pause"); >>> status.asym_pause = of_property_read_bool(fixed_link_node, >>> "asym-pause"); >>> + >>> + gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0, >>> + GPIOD_IN, "mdio"); >>> of_node_put(fixed_link_node); >>> + if (IS_ERR(gpiod)) { >>> + if (PTR_ERR(gpiod) == -EPROBE_DEFER) >>> + return PTR_ERR(gpiod); >>> + pr_err("error getting GPIO for fixed link %pOF, proceed without\n", >>> + fixed_link_node); >>> + gpiod = NULL; >>> extern struct phy_device *fixed_phy_register(unsigned int irq, >>> struct fixed_phy_status *status, >>> - struct device_node *np); >>> + struct device_node *np, >>> + struct gpio_desc *gpiod); >> >> Hi Moritz >> >> I think it would be better to add a >> >> extern struct phy_device *fixed_phy_register_gpiod(unsigned int irq, >> struct fixed_phy_status *status, >> struct gpio_desc *gpiod); >> >> If you are not using DT, the np is pointless. So lets keep the API >> simple. > > To clarify: Do you agree with moving the parsing to of_mdio, or do you > want to keep the parsing as it was and just add an additional function. > > In the latter case you'd have a > static struct phy_device *__fixed_phy_register(unsigned int irq, > struct fixed_phy_status *status, > struct device_node *np, > struct gpio_desc *gpiod) > { > if (gpiod) > use that > else > old behavior / scan dt? > } > > That we then call from fixed_phy_register_gpiod() with a np of NULL, > and from fixed_phy_register() with a gpiod of NULL? If you have existing users in tree, of something like: foo(a, b, c) and you want to introduce a "d" argument, it's a good practice to do: foo(a, b, c) -> __foo(a, b, c, 0) foo_with_d(a, b, c, d) __foo(a, b, c, d) and have the body of foo() become __foo() while taking the new argument. Hope this is clear.
diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c index 17482ae09aa5..7f124c620092 100644 --- a/drivers/net/dsa/dsa_loop.c +++ b/drivers/net/dsa/dsa_loop.c @@ -343,7 +343,7 @@ static int __init dsa_loop_init(void) unsigned int i; for (i = 0; i < NUM_FIXED_PHYS; i++) - phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL); + phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL, NULL); return mdio_driver_register(&dsa_loop_drv); } diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c index 4632dd5dbad1..bce644dec5c2 100644 --- a/drivers/net/ethernet/broadcom/bgmac.c +++ b/drivers/net/ethernet/broadcom/bgmac.c @@ -1446,7 +1446,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac) struct phy_device *phy_dev; int err; - phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); + phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); if (!phy_dev || IS_ERR(phy_dev)) { dev_err(bgmac->dev, "Failed to register fixed PHY device\n"); return -ENODEV; diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index 51880d83131a..7cbd737aba80 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -525,7 +525,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv) .asym_pause = 0, }; - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); if (!phydev || IS_ERR(phydev)) { dev_err(kdev, "failed to register fixed PHY device\n"); return -ENODEV; diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c index d810f914aaa4..845bd7c2065a 100644 --- a/drivers/net/phy/fixed_phy.c +++ b/drivers/net/phy/fixed_phy.c @@ -18,6 +18,7 @@ #include <linux/err.h> #include <linux/slab.h> #include <linux/of.h> +#include <linux/of_mdio.h> #include <linux/gpio/consumer.h> #include <linux/seqlock.h> #include <linux/idr.h> @@ -191,50 +192,12 @@ static void fixed_phy_del(int phy_addr) } } -#ifdef CONFIG_OF_GPIO -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) -{ - struct device_node *fixed_link_node; - struct gpio_desc *gpiod; - - if (!np) - return NULL; - - fixed_link_node = of_get_child_by_name(np, "fixed-link"); - if (!fixed_link_node) - return NULL; - - /* - * As the fixed link is just a device tree node without any - * Linux device associated with it, we simply have obtain - * the GPIO descriptor from the device tree like this. - */ - gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0, - GPIOD_IN, "mdio"); - of_node_put(fixed_link_node); - if (IS_ERR(gpiod)) { - if (PTR_ERR(gpiod) == -EPROBE_DEFER) - return gpiod; - pr_err("error getting GPIO for fixed link %pOF, proceed without\n", - fixed_link_node); - gpiod = NULL; - } - - return gpiod; -} -#else -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) -{ - return NULL; -} -#endif - struct phy_device *fixed_phy_register(unsigned int irq, struct fixed_phy_status *status, - struct device_node *np) + struct device_node *np, + struct gpio_desc *gpiod) { struct fixed_mdio_bus *fmb = &platform_fmb; - struct gpio_desc *gpiod = NULL; struct phy_device *phy; int phy_addr; int ret; @@ -242,11 +205,6 @@ struct phy_device *fixed_phy_register(unsigned int irq, if (!fmb->mii_bus || fmb->mii_bus->state != MDIOBUS_REGISTERED) return ERR_PTR(-EPROBE_DEFER); - /* Check if we have a GPIO associated with this fixed phy */ - gpiod = fixed_phy_get_gpiod(np); - if (IS_ERR(gpiod)) - return ERR_CAST(gpiod); - /* Get the next available PHY address, up to PHY_MAX_ADDR */ phy_addr = ida_simple_get(&phy_fixed_ida, 0, PHY_MAX_ADDR, GFP_KERNEL); if (phy_addr < 0) diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 3d92ea6fcc02..bd88f0aef2fa 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -2051,7 +2051,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev) phydev = phy_find_first(dev->mdiobus); if (!phydev) { netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n"); - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL); if (IS_ERR(phydev)) { netdev_err(dev->net, "No PHY/fixed_PHY found\n"); return NULL; diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index de6157357e26..6be2120b5f03 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -20,6 +20,7 @@ #include <linux/of_mdio.h> #include <linux/of_net.h> #include <linux/module.h> +#include <linux/gpio/consumer.h> #define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */ @@ -460,6 +461,7 @@ int of_phy_register_fixed_link(struct device_node *np) { struct fixed_phy_status status = {}; struct device_node *fixed_link_node; + struct gpio_desc *gpiod = NULL; u32 fixed_link_prop[5]; const char *managed; @@ -483,7 +485,17 @@ int of_phy_register_fixed_link(struct device_node *np) status.pause = of_property_read_bool(fixed_link_node, "pause"); status.asym_pause = of_property_read_bool(fixed_link_node, "asym-pause"); + + gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0, + GPIOD_IN, "mdio"); of_node_put(fixed_link_node); + if (IS_ERR(gpiod)) { + if (PTR_ERR(gpiod) == -EPROBE_DEFER) + return PTR_ERR(gpiod); + pr_err("error getting GPIO for fixed link %pOF, proceed without\n", + fixed_link_node); + gpiod = NULL; + } goto register_phy; } @@ -502,7 +514,8 @@ int of_phy_register_fixed_link(struct device_node *np) return -ENODEV; register_phy: - return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, np)); + return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, np, + gpiod)); } EXPORT_SYMBOL(of_phy_register_fixed_link); diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h index c78fc203db43..69caa8cda767 100644 --- a/include/linux/phy_fixed.h +++ b/include/linux/phy_fixed.h @@ -18,7 +18,8 @@ extern int fixed_phy_add(unsigned int irq, int phy_id, struct fixed_phy_status *status); extern struct phy_device *fixed_phy_register(unsigned int irq, struct fixed_phy_status *status, - struct device_node *np); + struct device_node *np, + struct gpio_desc *gpiod); extern void fixed_phy_unregister(struct phy_device *phydev); extern int fixed_phy_set_link_update(struct phy_device *phydev, int (*link_update)(struct net_device *,
Move the DT based link GPIO parsing to of_mdio and let the places that register a fixed_phy pass in a GPIO descriptor or NULL. This allows fixed_phy on non-DT platforms to have link GPIOs, too. Signed-off-by: Moritz Fischer <mdf@kernel.org> --- drivers/net/dsa/dsa_loop.c | 2 +- drivers/net/ethernet/broadcom/bgmac.c | 2 +- drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +- drivers/net/phy/fixed_phy.c | 48 ++------------------ drivers/net/usb/lan78xx.c | 2 +- drivers/of/of_mdio.c | 15 +++++- include/linux/phy_fixed.h | 3 +- 7 files changed, 23 insertions(+), 51 deletions(-)