[net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Calvin Johnson]

Following functions are defined:
  phylink_fwnode_phy_connect()
  phylink_device_phy_connect()
  fwnode_phy_find_device()
  device_phy_find_device()
  fwnode_get_phy_node()

First two help in connecting phy to phylink instance.
Next two help in finding a phy on a mdiobus.
Last one helps in getting phy_node from a fwnode.

Changes in v2:
  move phy code from base/property.c to net/phy/phy_device.c
  replace acpi & of code to get phy-handle with fwnode_find_reference
  replace of_ and acpi_ code with generic fwnode to get phy-handle.

Calvin Johnson (3):
  device property: Introduce phy related fwnode functions
  net: phy: alphabetically sort header includes
  phylink: Introduce phylink_fwnode_phy_connect()

 drivers/net/phy/phy_device.c | 83 ++++++++++++++++++++++++++++++------
 drivers/net/phy/phylink.c    | 68 +++++++++++++++++++++++++++++
 include/linux/phy.h          |  3 ++
 include/linux/phylink.h      |  6 +++
 4 files changed, 146 insertions(+), 14 deletions(-)

-- 
2.17.1

[net-next PATCH v2 1/3] device property: Introduce phy related fwnode functions

[Calvin Johnson]

Define fwnode_phy_find_device() to iterate an mdiobus and find the
phy device of the provided phy fwnode. Additionally define
device_phy_find_device() to find phy device of provided device.

Define fwnode_get_phy_node() to get phy_node using named reference.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2:
  move phy code from base/property.c to net/phy/phy_device.c
  replace acpi & of code to get phy-handle with fwnode_find_reference

 drivers/net/phy/phy_device.c | 55 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  3 ++
 2 files changed, 58 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7e1ddd5745d2..a2f3dbba8a3c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -31,6 +31,7 @@
 #include <linux/mdio.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
+#include <linux/property.h>
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
@@ -2436,6 +2437,60 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->ack_interrupt;
 }
 
+/**
+ * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
+ * phy_fwnode.
+ * @phy_fwnode: Pointer to the phy's fwnode.
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ */
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	struct device *d;
+	struct mdio_device *mdiodev;
+
+	if (!phy_fwnode)
+		return NULL;
+
+	d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
+	if (d) {
+		mdiodev = to_mdio_device(d);
+		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+			return to_phy_device(d);
+		put_device(d);
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(fwnode_phy_find_device);
+
+/**
+ * device_phy_find_device - For the given device, get the phy_device
+ * @dev: Pointer to the given device
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ */
+struct phy_device *device_phy_find_device(struct device *dev)
+{
+	return fwnode_phy_find_device(dev_fwnode(dev));
+}
+EXPORT_SYMBOL_GPL(device_phy_find_device);
+
+/**
+ * fwnode_get_phy_node - Get the phy_node using the named reference.
+ * @fwnode: Pointer to fwnode from which phy_node has to be obtained.
+ *
+ * Returns pointer to the phy fwnode, or ERR_PTR. Caller is responsible to
+ * call fwnode_handle_put() on the returned phy fwnode pointer.
+ */
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+	return fwnode_find_reference(fwnode, "phy-handle", 0);
+}
+EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
+
 /**
  * phy_probe - probe and init a PHY device
  * @dev: device to probe and init
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e2bfb9240587..f0450ef2dc9b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1328,6 +1328,9 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
 bool phy_validate_pause(struct phy_device *phydev,
 			struct ethtool_pauseparam *pp);
 void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
+struct phy_device *device_phy_find_device(struct device *dev);
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
 void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
 		       bool *tx_pause, bool *rx_pause);
 
-- 
2.17.1

Re: [net-next PATCH v2 1/3] device property: Introduce phy related fwnode functions

[Russell King - ARM Linux admin]

On Mon, Apr 27, 2020 at 06:54:07PM +0530, Calvin Johnson wrote:
> Define fwnode_phy_find_device() to iterate an mdiobus and find the
> phy device of the provided phy fwnode. Additionally define
> device_phy_find_device() to find phy device of provided device.
> 
> Define fwnode_get_phy_node() to get phy_node using named reference.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
> Changes in v2:
>   move phy code from base/property.c to net/phy/phy_device.c
>   replace acpi & of code to get phy-handle with fwnode_find_reference
> 
>  drivers/net/phy/phy_device.c | 55 ++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          |  3 ++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 7e1ddd5745d2..a2f3dbba8a3c 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -31,6 +31,7 @@
>  #include <linux/mdio.h>
>  #include <linux/io.h>
>  #include <linux/uaccess.h>
> +#include <linux/property.h>
>  
>  MODULE_DESCRIPTION("PHY library");
>  MODULE_AUTHOR("Andy Fleming");
> @@ -2436,6 +2437,60 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->ack_interrupt;
>  }
>  
> +/**
> + * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
> + * phy_fwnode.
> + * @phy_fwnode: Pointer to the phy's fwnode.
> + *
> + * If successful, returns a pointer to the phy_device with the embedded
> + * struct device refcount incremented by one, or NULL on failure.
> + */
> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> +	struct device *d;
> +	struct mdio_device *mdiodev;
> +
> +	if (!phy_fwnode)
> +		return NULL;
> +
> +	d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
> +	if (d) {
> +		mdiodev = to_mdio_device(d);
> +		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> +			return to_phy_device(d);
> +		put_device(d);
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(fwnode_phy_find_device);
> +
> +/**
> + * device_phy_find_device - For the given device, get the phy_device
> + * @dev: Pointer to the given device
> + *
> + * If successful, returns a pointer to the phy_device with the embedded
> + * struct device refcount incremented by one, or NULL on failure.
> + */
> +struct phy_device *device_phy_find_device(struct device *dev)
> +{
> +	return fwnode_phy_find_device(dev_fwnode(dev));
> +}
> +EXPORT_SYMBOL_GPL(device_phy_find_device);
> +
> +/**
> + * fwnode_get_phy_node - Get the phy_node using the named reference.
> + * @fwnode: Pointer to fwnode from which phy_node has to be obtained.
> + *
> + * Returns pointer to the phy fwnode, or ERR_PTR. Caller is responsible to
> + * call fwnode_handle_put() on the returned phy fwnode pointer.
> + */
> +struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> +{
> +	return fwnode_find_reference(fwnode, "phy-handle", 0);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
> +
>  /**
>   * phy_probe - probe and init a PHY device
>   * @dev: device to probe and init
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index e2bfb9240587..f0450ef2dc9b 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1328,6 +1328,9 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
>  bool phy_validate_pause(struct phy_device *phydev,
>  			struct ethtool_pauseparam *pp);
>  void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
> +struct phy_device *device_phy_find_device(struct device *dev);
> +struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
>  void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
>  		       bool *tx_pause, bool *rx_pause);
>  

I think you can find a better location in this file to places these
function prototypes - putting them in the middle of a set of prototypes
for pause-related functions looks like you printed the file out, and
played "pin the tail on the donkey" to select somewhere to put them!

I think putting them near the get_phy_device() prototype would be
sensible.  Also, consider the case where CONFIG_PHYLIB is not set.
Do we need stub implementations of these functions to avoid ifdefs
in C code?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v2 1/3] device property: Introduce phy related fwnode functions

[Russell King - ARM Linux admin]

On Mon, Apr 27, 2020 at 06:54:07PM +0530, Calvin Johnson wrote:
> +/**
> + * device_phy_find_device - For the given device, get the phy_device
> + * @dev: Pointer to the given device
> + *
> + * If successful, returns a pointer to the phy_device with the embedded
> + * struct device refcount incremented by one, or NULL on failure.

It probably makes sense to refer the reader to the return conditions
for fwnode_phy_find_device() so that should fwnode_phy_find_device()
be updated, there's no need to modify multiple locations.

> + */
> +struct phy_device *device_phy_find_device(struct device *dev)
> +{
> +	return fwnode_phy_find_device(dev_fwnode(dev));
> +}
> +EXPORT_SYMBOL_GPL(device_phy_find_device);
> +
> +/**
> + * fwnode_get_phy_node - Get the phy_node using the named reference.
> + * @fwnode: Pointer to fwnode from which phy_node has to be obtained.
> + *
> + * Returns pointer to the phy fwnode, or ERR_PTR. Caller is responsible to
> + * call fwnode_handle_put() on the returned phy fwnode pointer.

This one even more so - fwnode_find_reference() is in an entirely
separate file, so the comment could well be missed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

[net-next PATCH v2 2/3] net: phy: alphabetically sort header includes

[Calvin Johnson]

Header includes are not sorted. Alphabetically sort them.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/phy/phy_device.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a2f3dbba8a3c..b8326bfc7c2a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,29 +9,29 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/kernel.h>
-#include <linux/string.h>
-#include <linux/errno.h>
-#include <linux/unistd.h>
-#include <linux/slab.h>
-#include <linux/interrupt.h>
-#include <linux/init.h>
+#include <linux/bitmap.h>
 #include <linux/delay.h>
-#include <linux/netdevice.h>
+#include <linux/errno.h>
 #include <linux/etherdevice.h>
-#include <linux/skbuff.h>
+#include <linux/ethtool.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mdio.h>
+#include <linux/mii.h>
 #include <linux/mm.h>
 #include <linux/module.h>
-#include <linux/mii.h>
-#include <linux/ethtool.h>
-#include <linux/bitmap.h>
+#include <linux/netdevice.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
+#include <linux/property.h>
 #include <linux/sfp.h>
-#include <linux/mdio.h>
-#include <linux/io.h>
+#include <linux/skbuff.h>
+#include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/uaccess.h>
-#include <linux/property.h>
+#include <linux/unistd.h>
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
-- 
2.17.1

[net-next PATCH v2 3/3] phylink: Introduce phylink_fwnode_phy_connect()

[Calvin Johnson]

Define phylink_fwnode_phy_connect() to connect phy specified by
a fwnode to a phylink instance. Additionally,
phylink_device_phy_connect() is defined to connect phy specified
by a device to a phylink instance.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2:
  replace of_ and acpi_ code with generic fwnode to get phy-handle.

 drivers/net/phy/phylink.c | 68 +++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  6 ++++
 2 files changed, 74 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0f23bec431c1..5eab1eadded7 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -961,6 +961,74 @@ 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()
+ * @fwnode: a pointer to a &struct fwnode_handle.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
+ *
+ * Connect the phy specified @fwnode 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_fwnode;
+	struct phy_device *phy_dev;
+	int ret = 0;
+
+	/* Fixed links and 802.3z are handled without needing a PHY */
+	if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
+	    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
+	     phy_interface_mode_is_8023z(pl->link_interface)))
+		return 0;
+
+	phy_fwnode = fwnode_get_phy_node(fwnode);
+	if ((IS_ERR_OR_NULL(phy_fwnode)) && (pl->cfg_link_an_mode == MLO_AN_PHY))
+		return -ENODEV;
+
+	phy_dev = fwnode_phy_find_device(phy_fwnode);
+	fwnode_handle_put(phy_fwnode);
+	if (!phy_dev)
+		return -ENODEV;
+
+	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
+				pl->link_interface);
+	if (ret)
+		return ret;
+
+	ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
+	if (ret)
+		phy_detach(phy_dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);
+
+/**
+ * phylink_device_phy_connect() - connect the PHY specified by the device.
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @dev: a pointer to a &struct device.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
+ *
+ * Connect the phy specified by the device 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_device_phy_connect(struct phylink *pl,
+			       struct device *dev,
+			       u32 flags)
+{
+	return phylink_fwnode_phy_connect(pl, dev_fwnode(dev), flags);
+}
+EXPORT_SYMBOL_GPL(phylink_device_phy_connect);
+
 /**
  * phylink_of_phy_connect() - connect the PHY specified in the DT mode.
  * @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index cc5b452a184e..c2bd0ee9dd9c 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -367,6 +367,12 @@ void phylink_add_pcs(struct phylink *, const struct phylink_pcs_ops *ops);
 void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags);
+int phylink_device_phy_connect(struct phylink *pl,
+			       struct device *dev,
+			       u32 flags);
 int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
 void phylink_disconnect_phy(struct phylink *);
 
-- 
2.17.1

Re: [net-next PATCH v2 3/3] phylink: Introduce phylink_fwnode_phy_connect()

[Russell King - ARM Linux admin]

On Mon, Apr 27, 2020 at 06:54:09PM +0530, Calvin Johnson wrote:
> Define phylink_fwnode_phy_connect() to connect phy specified by
> a fwnode to a phylink instance. Additionally,
> phylink_device_phy_connect() is defined to connect phy specified
> by a device to a phylink instance.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
> Changes in v2:
>   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> 
>  drivers/net/phy/phylink.c | 68 +++++++++++++++++++++++++++++++++++++++
>  include/linux/phylink.h   |  6 ++++
>  2 files changed, 74 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 0f23bec431c1..5eab1eadded7 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -961,6 +961,74 @@ 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()
> + * @fwnode: a pointer to a &struct fwnode_handle.
> + * @flags: PHY-specific flags to communicate to the PHY device driver
> + *
> + * Connect the phy specified @fwnode 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_fwnode;
> +	struct phy_device *phy_dev;
> +	int ret = 0;
> +
> +	/* Fixed links and 802.3z are handled without needing a PHY */
> +	if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
> +	    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
> +	     phy_interface_mode_is_8023z(pl->link_interface)))
> +		return 0;
> +
> +	phy_fwnode = fwnode_get_phy_node(fwnode);
> +	if ((IS_ERR_OR_NULL(phy_fwnode)) && (pl->cfg_link_an_mode == MLO_AN_PHY))

According to your documentation for fwnode_get_phy_node(), it can't
return NULL.  So, use of IS_ERR_OR_NULL() is incorrect here.  Please
also eliminate the unnecessary parens to match the style in the rest
of this file.

> +		return -ENODEV;

If fwnode_get_phy_node() returns an error pointer, shouldn't you be
propagating that error here?

> +
> +	phy_dev = fwnode_phy_find_device(phy_fwnode);
> +	fwnode_handle_put(phy_fwnode);
> +	if (!phy_dev)
> +		return -ENODEV;
> +
> +	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
> +				pl->link_interface);
> +	if (ret)
> +		return ret;
> +
> +	ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
> +	if (ret)
> +		phy_detach(phy_dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);
> +
> +/**
> + * phylink_device_phy_connect() - connect the PHY specified by the device.
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + * @dev: a pointer to a &struct device.
> + * @flags: PHY-specific flags to communicate to the PHY device driver
> + *
> + * Connect the phy specified by the device 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_device_phy_connect(struct phylink *pl,
> +			       struct device *dev,
> +			       u32 flags)
> +{
> +	return phylink_fwnode_phy_connect(pl, dev_fwnode(dev), flags);
> +}
> +EXPORT_SYMBOL_GPL(phylink_device_phy_connect);

If this has any users, I think this should be an inline function in
phylink.h - it's just a helper after all.  If it doesn't then it should
just be dropped.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Russell King - ARM Linux admin]

On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote:
> Following functions are defined:
>   phylink_fwnode_phy_connect()
>   phylink_device_phy_connect()
>   fwnode_phy_find_device()
>   device_phy_find_device()
>   fwnode_get_phy_node()
> 
> First two help in connecting phy to phylink instance.
> Next two help in finding a phy on a mdiobus.
> Last one helps in getting phy_node from a fwnode.
> 
> Changes in v2:
>   move phy code from base/property.c to net/phy/phy_device.c
>   replace acpi & of code to get phy-handle with fwnode_find_reference
>   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> 
> Calvin Johnson (3):
>   device property: Introduce phy related fwnode functions
>   net: phy: alphabetically sort header includes
>   phylink: Introduce phylink_fwnode_phy_connect()

Thanks for this, but there's more work that needs to be done here.  I
also think that we must have an ack from ACPI people before this can be
accepted - you are in effect proposing a new way for representing PHYs
in ACPI.

> 
>  drivers/net/phy/phy_device.c | 83 ++++++++++++++++++++++++++++++------
>  drivers/net/phy/phylink.c    | 68 +++++++++++++++++++++++++++++
>  include/linux/phy.h          |  3 ++
>  include/linux/phylink.h      |  6 +++
>  4 files changed, 146 insertions(+), 14 deletions(-)
> 
> -- 
> 2.17.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Calvin Johnson]

On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote:
> > Following functions are defined:
> >   phylink_fwnode_phy_connect()
> >   phylink_device_phy_connect()
> >   fwnode_phy_find_device()
> >   device_phy_find_device()
> >   fwnode_get_phy_node()
> > 
> > First two help in connecting phy to phylink instance.
> > Next two help in finding a phy on a mdiobus.
> > Last one helps in getting phy_node from a fwnode.
> > 
> > Changes in v2:
> >   move phy code from base/property.c to net/phy/phy_device.c
> >   replace acpi & of code to get phy-handle with fwnode_find_reference
> >   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> > 
> > Calvin Johnson (3):
> >   device property: Introduce phy related fwnode functions
> >   net: phy: alphabetically sort header includes
> >   phylink: Introduce phylink_fwnode_phy_connect()
> 
> Thanks for this, but there's more work that needs to be done here.  I
> also think that we must have an ack from ACPI people before this can be
> accepted - you are in effect proposing a new way for representing PHYs
> in ACPI.

Thanks for your review.

Agree that we need an ack from ACPI people.
However, I don't think it is a completely new way as similar acpi approach to
get phy-handle is already in place.
Please see this:
https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832

Please let me know, if you see more work than the ones you pointed out in your
review comments on previous patches.

Thanks
Calvin

Re: [net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Russell King - ARM Linux admin]

On Mon, Apr 27, 2020 at 08:02:38PM +0530, Calvin Johnson wrote:
> On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote:
> > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote:
> > > Following functions are defined:
> > >   phylink_fwnode_phy_connect()
> > >   phylink_device_phy_connect()
> > >   fwnode_phy_find_device()
> > >   device_phy_find_device()
> > >   fwnode_get_phy_node()
> > > 
> > > First two help in connecting phy to phylink instance.
> > > Next two help in finding a phy on a mdiobus.
> > > Last one helps in getting phy_node from a fwnode.
> > > 
> > > Changes in v2:
> > >   move phy code from base/property.c to net/phy/phy_device.c
> > >   replace acpi & of code to get phy-handle with fwnode_find_reference
> > >   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> > > 
> > > Calvin Johnson (3):
> > >   device property: Introduce phy related fwnode functions
> > >   net: phy: alphabetically sort header includes
> > >   phylink: Introduce phylink_fwnode_phy_connect()
> > 
> > Thanks for this, but there's more work that needs to be done here.  I
> > also think that we must have an ack from ACPI people before this can be
> > accepted - you are in effect proposing a new way for representing PHYs
> > in ACPI.
> 
> Thanks for your review.
> 
> Agree that we need an ack from ACPI people.
> However, I don't think it is a completely new way as similar acpi approach to
> get phy-handle is already in place.
> Please see this:
> https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832

That was added by:

commit 8089a96f601bdfe3e1b41d14bb703aafaf1b8f34
Author: Iyappan Subramanian <isubramanian@apm.com>
Date:   Mon Jul 25 17:12:41 2016 -0700

    drivers: net: xgene: Add backward compatibility

    This patch adds xgene_enet_check_phy_hanlde() function that checks whether
    MDIO driver is probed successfully and sets pdata->mdio_driver to true.
    If MDIO driver is not probed, ethernet driver falls back to backward
    compatibility mode.

    Since enum xgene_enet_cmd is used by MDIO driver, removing this from
    ethernet driver.

    Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
    Tested-by: Fushen Chen <fchen@apm.com>
    Tested-by: Toan Le <toanle@apm.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

The commit message says nothing about adding ACPI stuff, and searching
the 'net for the posting of this patch seems to suggest that it wasn't
obviously copied to any ACPI people:

    https://lists.openwall.net/netdev/2016/07/26/11

Annoyingly, searching for:

    "drivers: net: xgene: Add backward compatibility" site:lore.kernel.org

doesn't find it on lore, so can't get the full headers and therefore
addresses.

So, yes, there's another driver using it, but the ACPI folk probably
never got a look-in on that instance.  Even if they had been copied,
the patch description is probably sufficiently poor that they wouldn't
have read the patch.

I'd say there's questions over whether ACPI people will find this an
acceptable approach.

Given that your patch moves this from one driver to a subsystem thing,
it needs to be ratified by ACPI people, because it's effectively
becoming a standardised way to represent a PHY in ACPI.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Calvin Johnson]

On Mon, Apr 27, 2020 at 03:48:07PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Apr 27, 2020 at 08:02:38PM +0530, Calvin Johnson wrote:
> > On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote:
> > > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote:
> > > > Following functions are defined:
> > > >   phylink_fwnode_phy_connect()
> > > >   phylink_device_phy_connect()
> > > >   fwnode_phy_find_device()
> > > >   device_phy_find_device()
> > > >   fwnode_get_phy_node()
> > > > 
> > > > First two help in connecting phy to phylink instance.
> > > > Next two help in finding a phy on a mdiobus.
> > > > Last one helps in getting phy_node from a fwnode.
> > > > 
> > > > Changes in v2:
> > > >   move phy code from base/property.c to net/phy/phy_device.c
> > > >   replace acpi & of code to get phy-handle with fwnode_find_reference
> > > >   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> > > > 
> > > > Calvin Johnson (3):
> > > >   device property: Introduce phy related fwnode functions
> > > >   net: phy: alphabetically sort header includes
> > > >   phylink: Introduce phylink_fwnode_phy_connect()
> > > 
> > > Thanks for this, but there's more work that needs to be done here.  I
> > > also think that we must have an ack from ACPI people before this can be
> > > accepted - you are in effect proposing a new way for representing PHYs
> > > in ACPI.
> > 
> > Thanks for your review.
> > 
> > Agree that we need an ack from ACPI people.
> > However, I don't think it is a completely new way as similar acpi approach to
> > get phy-handle is already in place.
> > Please see this:
> > https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832
> 
> That was added by:
> 
> commit 8089a96f601bdfe3e1b41d14bb703aafaf1b8f34
> Author: Iyappan Subramanian <isubramanian@apm.com>
> Date:   Mon Jul 25 17:12:41 2016 -0700
> 
>     drivers: net: xgene: Add backward compatibility
> 
>     This patch adds xgene_enet_check_phy_hanlde() function that checks whether
>     MDIO driver is probed successfully and sets pdata->mdio_driver to true.
>     If MDIO driver is not probed, ethernet driver falls back to backward
>     compatibility mode.
> 
>     Since enum xgene_enet_cmd is used by MDIO driver, removing this from
>     ethernet driver.
> 
>     Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
>     Tested-by: Fushen Chen <fchen@apm.com>
>     Tested-by: Toan Le <toanle@apm.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> The commit message says nothing about adding ACPI stuff, and searching
> the 'net for the posting of this patch seems to suggest that it wasn't
> obviously copied to any ACPI people:
> 
>     https://lists.openwall.net/netdev/2016/07/26/11
> 
> Annoyingly, searching for:
> 
>     "drivers: net: xgene: Add backward compatibility" site:lore.kernel.org
> 
> doesn't find it on lore, so can't get the full headers and therefore
> addresses.
> 
> So, yes, there's another driver using it, but the ACPI folk probably
> never got a look-in on that instance.  Even if they had been copied,
> the patch description is probably sufficiently poor that they wouldn't
> have read the patch.
> 
> I'd say there's questions over whether ACPI people will find this an
> acceptable approach.
> 
> Given that your patch moves this from one driver to a subsystem thing,
> it needs to be ratified by ACPI people, because it's effectively
> becoming a standardised way to represent a PHY in ACPI.
> 
Thanks for digging deep. Makes sense to me.
Will wait for ACPI response.

Regards
Calvin

Re: [net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Calvin Johnson]

On Mon, Apr 27, 2020 at 03:48:07PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Apr 27, 2020 at 08:02:38PM +0530, Calvin Johnson wrote:
> > On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote:
> > > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote:
> > > > Following functions are defined:
> > > >   phylink_fwnode_phy_connect()
> > > >   phylink_device_phy_connect()
> > > >   fwnode_phy_find_device()
> > > >   device_phy_find_device()
> > > >   fwnode_get_phy_node()
> > > > 
> > > > First two help in connecting phy to phylink instance.
> > > > Next two help in finding a phy on a mdiobus.
> > > > Last one helps in getting phy_node from a fwnode.
> > > > 
> > > > Changes in v2:
> > > >   move phy code from base/property.c to net/phy/phy_device.c
> > > >   replace acpi & of code to get phy-handle with fwnode_find_reference
> > > >   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> > > > 
> > > > Calvin Johnson (3):
> > > >   device property: Introduce phy related fwnode functions
> > > >   net: phy: alphabetically sort header includes
> > > >   phylink: Introduce phylink_fwnode_phy_connect()
> > > 
> > > Thanks for this, but there's more work that needs to be done here.  I
> > > also think that we must have an ack from ACPI people before this can be
> > > accepted - you are in effect proposing a new way for representing PHYs
> > > in ACPI.
> > 
> > Thanks for your review.
> > 
> > Agree that we need an ack from ACPI people.
> > However, I don't think it is a completely new way as similar acpi approach to
> > get phy-handle is already in place.
> > Please see this:
> > https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832
> 
> That was added by:
> 
> commit 8089a96f601bdfe3e1b41d14bb703aafaf1b8f34
> Author: Iyappan Subramanian <isubramanian@apm.com>
> Date:   Mon Jul 25 17:12:41 2016 -0700
> 
>     drivers: net: xgene: Add backward compatibility
> 
>     This patch adds xgene_enet_check_phy_hanlde() function that checks whether
>     MDIO driver is probed successfully and sets pdata->mdio_driver to true.
>     If MDIO driver is not probed, ethernet driver falls back to backward
>     compatibility mode.
> 
>     Since enum xgene_enet_cmd is used by MDIO driver, removing this from
>     ethernet driver.
> 
>     Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
>     Tested-by: Fushen Chen <fchen@apm.com>
>     Tested-by: Toan Le <toanle@apm.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> The commit message says nothing about adding ACPI stuff, and searching
> the 'net for the posting of this patch seems to suggest that it wasn't
> obviously copied to any ACPI people:
> 
>     https://lists.openwall.net/netdev/2016/07/26/11
> 
> Annoyingly, searching for:
> 
>     "drivers: net: xgene: Add backward compatibility" site:lore.kernel.org
> 
> doesn't find it on lore, so can't get the full headers and therefore
> addresses.
> 
> So, yes, there's another driver using it, but the ACPI folk probably
> never got a look-in on that instance.  Even if they had been copied,
> the patch description is probably sufficiently poor that they wouldn't
> have read the patch.
> 
> I'd say there's questions over whether ACPI people will find this an
> acceptable approach.
> 
> Given that your patch moves this from one driver to a subsystem thing,
> it needs to be ratified by ACPI people, because it's effectively
> becoming a standardised way to represent a PHY in ACPI.

How can we get attention/response from ACPI people? I've now added ACPI 
maintainers in the To list.

Thanks
Calvin

Re: [net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Rafael J. Wysocki]

On Wed, Apr 29, 2020 at 7:38 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> On Mon, Apr 27, 2020 at 03:48:07PM +0100, Russell King - ARM Linux admin wrote:
> > On Mon, Apr 27, 2020 at 08:02:38PM +0530, Calvin Johnson wrote:
> > > On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote:
> > > > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote:
> > > > > Following functions are defined:
> > > > >   phylink_fwnode_phy_connect()
> > > > >   phylink_device_phy_connect()
> > > > >   fwnode_phy_find_device()
> > > > >   device_phy_find_device()
> > > > >   fwnode_get_phy_node()
> > > > >
> > > > > First two help in connecting phy to phylink instance.
> > > > > Next two help in finding a phy on a mdiobus.
> > > > > Last one helps in getting phy_node from a fwnode.
> > > > >
> > > > > Changes in v2:
> > > > >   move phy code from base/property.c to net/phy/phy_device.c
> > > > >   replace acpi & of code to get phy-handle with fwnode_find_reference
> > > > >   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> > > > >
> > > > > Calvin Johnson (3):
> > > > >   device property: Introduce phy related fwnode functions
> > > > >   net: phy: alphabetically sort header includes
> > > > >   phylink: Introduce phylink_fwnode_phy_connect()
> > > >
> > > > Thanks for this, but there's more work that needs to be done here.  I
> > > > also think that we must have an ack from ACPI people before this can be
> > > > accepted - you are in effect proposing a new way for representing PHYs
> > > > in ACPI.
> > >
> > > Thanks for your review.
> > >
> > > Agree that we need an ack from ACPI people.
> > > However, I don't think it is a completely new way as similar acpi approach to
> > > get phy-handle is already in place.
> > > Please see this:
> > > https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832
> >
> > That was added by:
> >
> > commit 8089a96f601bdfe3e1b41d14bb703aafaf1b8f34
> > Author: Iyappan Subramanian <isubramanian@apm.com>
> > Date:   Mon Jul 25 17:12:41 2016 -0700
> >
> >     drivers: net: xgene: Add backward compatibility
> >
> >     This patch adds xgene_enet_check_phy_hanlde() function that checks whether
> >     MDIO driver is probed successfully and sets pdata->mdio_driver to true.
> >     If MDIO driver is not probed, ethernet driver falls back to backward
> >     compatibility mode.
> >
> >     Since enum xgene_enet_cmd is used by MDIO driver, removing this from
> >     ethernet driver.
> >
> >     Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
> >     Tested-by: Fushen Chen <fchen@apm.com>
> >     Tested-by: Toan Le <toanle@apm.com>
> >     Signed-off-by: David S. Miller <davem@davemloft.net>
> >
> > The commit message says nothing about adding ACPI stuff, and searching
> > the 'net for the posting of this patch seems to suggest that it wasn't
> > obviously copied to any ACPI people:
> >
> >     https://lists.openwall.net/netdev/2016/07/26/11
> >
> > Annoyingly, searching for:
> >
> >     "drivers: net: xgene: Add backward compatibility" site:lore.kernel.org
> >
> > doesn't find it on lore, so can't get the full headers and therefore
> > addresses.
> >
> > So, yes, there's another driver using it, but the ACPI folk probably
> > never got a look-in on that instance.  Even if they had been copied,
> > the patch description is probably sufficiently poor that they wouldn't
> > have read the patch.
> >
> > I'd say there's questions over whether ACPI people will find this an
> > acceptable approach.
> >
> > Given that your patch moves this from one driver to a subsystem thing,
> > it needs to be ratified by ACPI people, because it's effectively
> > becoming a standardised way to represent a PHY in ACPI.
>
> How can we get attention/response from ACPI people?

This is in my queue, but the processing of this has been slow for a
while, sorry about that.

If you have a new version of the series, please submit it, otherwise
ping me in a couple of days if I don't respond to the patches in the
meantime.

Thanks!

Re: [net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Calvin Johnson]

Hi Rafael,

On Wed, Apr 29, 2020 at 12:26:12PM +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 29, 2020 at 7:38 AM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
<snip>
> > > So, yes, there's another driver using it, but the ACPI folk probably
> > > never got a look-in on that instance.  Even if they had been copied,
> > > the patch description is probably sufficiently poor that they wouldn't
> > > have read the patch.
> > >
> > > I'd say there's questions over whether ACPI people will find this an
> > > acceptable approach.
> > >
> > > Given that your patch moves this from one driver to a subsystem thing,
> > > it needs to be ratified by ACPI people, because it's effectively
> > > becoming a standardised way to represent a PHY in ACPI.
> >
> > How can we get attention/response from ACPI people?
> 
> This is in my queue, but the processing of this has been slow for a
> while, sorry about that.
> 
> If you have a new version of the series, please submit it, otherwise
> ping me in a couple of days if I don't respond to the patches in the
> meantime.

I've posted v3 of the patchset. Can you please review?

Thanks
Calvin

Re: [net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Calvin Johnson]

On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote:

Hi Russell, others,

> Following functions are defined:
>   phylink_fwnode_phy_connect()
>   phylink_device_phy_connect()
>   fwnode_phy_find_device()
>   device_phy_find_device()
>   fwnode_get_phy_node()
> 
> First two help in connecting phy to phylink instance.
> Next two help in finding a phy on a mdiobus.
> Last one helps in getting phy_node from a fwnode.
> 
> Changes in v2:
>   move phy code from base/property.c to net/phy/phy_device.c
>   replace acpi & of code to get phy-handle with fwnode_find_reference
>   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> 
> Calvin Johnson (3):
>   device property: Introduce phy related fwnode functions
>   net: phy: alphabetically sort header includes
>   phylink: Introduce phylink_fwnode_phy_connect()
> 
>  drivers/net/phy/phy_device.c | 83 ++++++++++++++++++++++++++++++------
>  drivers/net/phy/phylink.c    | 68 +++++++++++++++++++++++++++++
>  include/linux/phy.h          |  3 ++
>  include/linux/phylink.h      |  6 +++
>  4 files changed, 146 insertions(+), 14 deletions(-)

I've a new patch introducing fwnode_mdiobus_register_phy and fwnode_get_phy_id.
Can I introduce it in v3 of this patchset or do I need to send it separately?
Please advice.

Thanks
Calvin