Re: [PATCH v1 6/7] net: phylink: Introduce phylink_fwnode_phy_connect()

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Fri, Jan 31, 2020 at 09:04:39PM +0530, Calvin Johnson wrote:
> From: Calvin Johnson <calvin.johnson@oss.nxp.com>
> 
> Introduce phylink_fwnode_phy_connect API to connect the PHY using
> fwnode.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
>  drivers/net/phy/phylink.c | 64 +++++++++++++++++++++++++++++++++++++++
>  include/linux/phylink.h   |  2 ++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index ee7a718662c6..f211f62283b5 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -18,6 +18,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/timer.h>
>  #include <linux/workqueue.h>
> +#include <linux/acpi.h>
>  
>  #include "sfp.h"
>  #include "swphy.h"
> @@ -817,6 +818,69 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
>  }
>  EXPORT_SYMBOL_GPL(phylink_connect_phy);
>  
> +/**
> + * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode.
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + * @dn: a pointer to a &struct device_node.
> + * @flags: PHY-specific flags to communicate to the PHY device driver
> + *
> + * Connect the phy specified in the device node @dn to the phylink instance
> + * specified by @pl. Actions specified in phylink_connect_phy() will be
> + * performed.
> + *
> + * Returns 0 on success or a negative errno.
> + */
> +int phylink_fwnode_phy_connect(struct phylink *pl,
> +			       struct fwnode_handle *fwnode,
> +			       u32 flags)
> +{
> +	struct fwnode_handle *phy_node;
> +	struct phy_device *phy_dev;
> +	int ret;
> +	int status;
> +	struct fwnode_reference_args args;
> +
> +	/* Fixed links and 802.3z are handled without needing a PHY */
> +	if (pl->link_an_mode == MLO_AN_FIXED ||
> +	    (pl->link_an_mode == MLO_AN_INBAND &&
> +	     phy_interface_mode_is_8023z(pl->link_interface)))
> +		return 0;
> +
> +	status = acpi_node_get_property_reference(fwnode, "phy-handle", 0,
> +						  &args);
> +	if (ACPI_FAILURE(status) || !is_acpi_device_node(args.fwnode))
> +		status = acpi_node_get_property_reference(fwnode, "phy", 0,
> +							  &args);
> +	if (ACPI_FAILURE(status) || !is_acpi_device_node(args.fwnode))
> +		status = acpi_node_get_property_reference(fwnode,
> +							  "phy-device", 0,
> +							  &args);

This is a copy-and-paste of phylink_of_phy_connect() without much
thought.

There is no need to duplicate the legacy DT functionality of
phy/phy-device/phy-handle in ACPI - there is no legacy to support,
so it's pointless trying to find one of three properties here.

I'd prefer both the DT and ACPI variants to be more integrated, so
we don't have two almost identical functions except for the firmware
specific detail.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up