* [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
@ 2019-02-06 20:51 ` mdf
0 siblings, 0 replies; 8+ messages in thread
From: Moritz Fischer @ 2019-02-06 20:51 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, netdev, colin.king, linus.walleij, yuehaibing, mcgrof,
frowand.list, robh+dt, UNGLinuxDriver, woojung.huh, hkallweit1,
davem, f.fainelli, vivien.didelot, andrew, moritz, alex.williams,
Moritz Fischer
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;
+ }
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 *,
--
2.20.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [RFC,net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
@ 2019-02-06 20:51 ` mdf
0 siblings, 0 replies; 8+ messages in thread
From: mdf @ 2019-02-06 20:51 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, netdev, colin.king, linus.walleij, yuehaibing, mcgrof,
frowand.list, robh+dt, UNGLinuxDriver, woojung.huh, hkallweit1,
davem, f.fainelli, vivien.didelot, andrew, moritz, alex.williams,
Moritz Fischer
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;
+ }
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 *,
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
@ 2019-02-06 21:53 ` Andrew Lunn
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2019-02-06 21:53 UTC (permalink / raw)
To: Moritz Fischer
Cc: linux-kernel, linux-usb, netdev, colin.king, linus.walleij,
yuehaibing, mcgrof, frowand.list, robh+dt, UNGLinuxDriver,
woojung.huh, hkallweit1, davem, f.fainelli, vivien.didelot,
moritz, alex.williams
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC,net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
@ 2019-02-06 21:53 ` Andrew Lunn
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2019-02-06 21:53 UTC (permalink / raw)
To: Moritz Fischer
Cc: linux-kernel, linux-usb, netdev, colin.king, linus.walleij,
yuehaibing, mcgrof, frowand.list, robh+dt, UNGLinuxDriver,
woojung.huh, hkallweit1, davem, f.fainelli, vivien.didelot,
moritz, alex.williams
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
@ 2019-02-06 22:38 ` mdf
0 siblings, 0 replies; 8+ messages in thread
From: Moritz Fischer @ 2019-02-06 22:38 UTC (permalink / raw)
To: Andrew Lunn
Cc: Moritz Fischer, linux-kernel, linux-usb, netdev, colin.king,
linus.walleij, yuehaibing, mcgrof, frowand.list, robh+dt,
UNGLinuxDriver, woojung.huh, hkallweit1, davem, f.fainelli,
vivien.didelot, moritz, alex.williams
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC,net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
@ 2019-02-06 22:38 ` mdf
0 siblings, 0 replies; 8+ messages in thread
From: mdf @ 2019-02-06 22:38 UTC (permalink / raw)
To: Andrew Lunn
Cc: Moritz Fischer, linux-kernel, linux-usb, netdev, colin.king,
linus.walleij, yuehaibing, mcgrof, frowand.list, robh+dt,
UNGLinuxDriver, woojung.huh, hkallweit1, davem, f.fainelli,
vivien.didelot, moritz, alex.williams
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
@ 2019-02-06 22:41 ` Florian Fainelli
0 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2019-02-06 22:41 UTC (permalink / raw)
To: Moritz Fischer, Andrew Lunn
Cc: linux-kernel, linux-usb, netdev, colin.king, linus.walleij,
yuehaibing, mcgrof, frowand.list, robh+dt, UNGLinuxDriver,
woojung.huh, hkallweit1, davem, vivien.didelot, moritz,
alex.williams
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.
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC,net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c
@ 2019-02-06 22:41 ` Florian Fainelli
0 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2019-02-06 22:41 UTC (permalink / raw)
To: Moritz Fischer, Andrew Lunn
Cc: linux-kernel, linux-usb, netdev, colin.king, linus.walleij,
yuehaibing, mcgrof, frowand.list, robh+dt, UNGLinuxDriver,
woojung.huh, hkallweit1, davem, vivien.didelot, moritz,
alex.williams
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-02-06 22:41 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-06 20:51 [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c Moritz Fischer
2019-02-06 20:51 ` [RFC,net-next] " mdf
2019-02-06 21:53 ` [RFC net-next] " Andrew Lunn
2019-02-06 21:53 ` [RFC,net-next] " Andrew Lunn
2019-02-06 22:38 ` [RFC net-next] " Moritz Fischer
2019-02-06 22:38 ` [RFC,net-next] " mdf
2019-02-06 22:41 ` [RFC net-next] " Florian Fainelli
2019-02-06 22:41 ` [RFC,net-next] " Florian Fainelli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.