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 <m...@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
