Re: [RFC net-next] net: fixed_phy: Move the DT based link GPIO parsing to of_mdio.c

[Florian Fainelli]

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@xxxxxxxxxx>
>>> ---
>>>  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