[net-next PATCH v3 00/15] ACPI support for dpaa2 driver

[net-next PATCH v3 00/15] ACPI support for dpaa2 driver

[Calvin Johnson]

This patch set provides ACPI support to DPAA2 network drivers.

It also introduces new fwnode based APIs to support phylink and phy
layers
    Following functions are defined:
      phylink_fwnode_phy_connect()
      fwnode_mdiobus_register_phy()
      fwnode_mdiobus_register()
      fwnode_get_phy_id()
      fwnode_phy_find_device()
      device_phy_find_device()
      fwnode_get_phy_node()
      fwnode_mdio_find_device()
      fwnode_get_id()

    First one helps in connecting phy to phylink instance.
    Next three helps in getting phy_id and registering phy to mdiobus
    Next two help in finding a phy on a mdiobus.
    Next one helps in getting phy_node from a fwnode.
    Last one is used to get fwnode ID.

    Corresponding OF functions are refactored.
    END


Changes in v3:
- Add more info on legacy DT properties "phy" and "phy-device"
- Redefine fwnode_phy_find_device() to follow of_phy_find_device()
- Use traditional comparison pattern
- Use GENMASK
- Modified to retrieve reg property value for ACPI as well
- Resolved compilation issue with CONFIG_ACPI = n
- Added more info into documentation
- Use acpi_mdiobus_register()
- Avoid unnecessary line removal
- Remove unused inclusion of acpi.h

Changes in v2:
- Updated with more description in document
- use reverse christmas tree ordering for local variables
- Refactor OF functions to use fwnode functions

Calvin Johnson (15):
  Documentation: ACPI: DSD: Document MDIO PHY
  net: phy: Introduce fwnode_mdio_find_device()
  net: phy: Introduce phy related fwnode functions
  of: mdio: Refactor of_phy_find_device()
  net: phy: Introduce fwnode_get_phy_id()
  of: mdio: Refactor of_get_phy_id()
  net: mdiobus: Introduce fwnode_mdiobus_register_phy()
  of: mdio: Refactor of_mdiobus_register_phy()
  device property: Introduce fwnode_get_id()
  net: mdio: Add ACPI support code for mdio
  net: mdiobus: Introduce fwnode_mdiobus_register()
  net/fsl: Use fwnode_mdiobus_register()
  phylink: introduce phylink_fwnode_phy_connect()
  net: phylink: Refactor phylink_of_phy_connect()
  net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

 Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/base/property.c                       |  33 +++++
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  |  87 +++++++-----
 drivers/net/ethernet/freescale/xgmac_mdio.c   |  12 +-
 drivers/net/mdio/Kconfig                      |   7 +
 drivers/net/mdio/Makefile                     |   1 +
 drivers/net/mdio/acpi_mdio.c                  |  49 +++++++
 drivers/net/mdio/of_mdio.c                    |  79 +----------
 drivers/net/phy/mdio_bus.c                    |  87 ++++++++++++
 drivers/net/phy/phy_device.c                  | 106 ++++++++++++++
 drivers/net/phy/phylink.c                     |  49 ++++---
 include/linux/acpi_mdio.h                     |  27 ++++
 include/linux/mdio.h                          |   2 +
 include/linux/of_mdio.h                       |   6 +-
 include/linux/phy.h                           |  32 +++++
 include/linux/phylink.h                       |   3 +
 include/linux/property.h                      |   1 +
 18 files changed, 579 insertions(+), 132 deletions(-)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
 create mode 100644 drivers/net/mdio/acpi_mdio.c
 create mode 100644 include/linux/acpi_mdio.h

-- 
2.17.1

[net-next PATCH v3 01/15] Documentation: ACPI: DSD: Document MDIO PHY

[Calvin Johnson]

Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
provide them to be connected to MAC.

Describe properties "phy-handle" and "phy-mode".

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

Changes in v3: None
Changes in v2:
- Updated with more description in document

 Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++
 1 file changed, 129 insertions(+)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst

diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
new file mode 100644
index 000000000000..a2e4fdcdbf53
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
@@ -0,0 +1,129 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+MDIO bus and PHYs in ACPI
+=========================
+
+The PHYs on an MDIO bus [1] are probed and registered using
+fwnode_mdiobus_register_phy().
+Later, for connecting these PHYs to MAC, the PHYs registered on the
+mdiobus have to be referenced.
+
+UUID given below should be used as mentioned in the "Device Properties
+UUID For _DSD" [2] document.
+   - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301
+
+This document introduces two _DSD properties that are to be used
+for PHYs on the MDIO bus.[3]
+
+phy-handle
+----------
+For each MAC node, a device property "phy-handle" is used to reference
+the PHY that is registered on an MDIO bus. This is mandatory for
+network interfaces that have PHYs connected to MAC via MDIO bus.
+
+During the MDIO bus driver initialization, PHYs on this bus are probed
+using the _ADR object as shown below and are registered on the mdio bus.
+
+::
+      Scope(\_SB.MDI0)
+      {
+        Device(PHY1) {
+          Name (_ADR, 0x1)
+        } // end of PHY1
+
+        Device(PHY2) {
+          Name (_ADR, 0x2)
+        } // end of PHY2
+      }
+
+Later, during the MAC driver initialization, the registered PHY devices
+have to be retrieved from the mdio bus. For this, MAC driver needs
+reference to the previously registered PHYs which are provided
+using reference to the device as {\_SB.MDI0.PHY1}.
+
+phy-mode
+--------
+The "phy-mode" _DSD property is used to describe the connection to
+the PHY. The valid values for "phy-mode" are defined in [4].
+
+
+An ASL example of this is shown below.
+
+DSDT entry for MDIO node
+------------------------
+The MDIO bus has an SoC component(mdio controller) and a platform
+component(PHYs on the mdiobus).
+
+a) Silicon Component
+This node describes the MDIO controller,MDI0
+--------------------------------------------
+::
+	Scope(_SB)
+	{
+	  Device(MDI0) {
+	    Name(_HID, "NXP0006")
+	    Name(_CCA, 1)
+	    Name(_UID, 0)
+	    Name(_CRS, ResourceTemplate() {
+	      Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
+	      Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
+	       {
+		 MDI0_IT
+	       }
+	    }) // end of _CRS for MDI0
+	  } // end of MDI0
+	}
+
+b) Platform Component
+This node defines the PHYs that are connected to the MDIO bus, MDI0
+-------------------------------------------------------------------
+::
+	Scope(\_SB.MDI0)
+	{
+	  Device(PHY1) {
+	    Name (_ADR, 0x1)
+	  } // end of PHY1
+
+	  Device(PHY2) {
+	    Name (_ADR, 0x2)
+	  } // end of PHY2
+	}
+
+
+Below are the MAC nodes where PHY nodes are referenced.
+phy-mode and phy-handle are used as explained earlier.
+------------------------------------------------------
+::
+	Scope(\_SB.MCE0.PR17)
+	{
+	  Name (_DSD, Package () {
+	     ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package (2) {"phy-mode", "rgmii-id"},
+		     Package (2) {"phy-handle", \_SB.MDI0.PHY1}
+	      }
+	   })
+	}
+
+	Scope(\_SB.MCE0.PR18)
+	{
+	  Name (_DSD, Package () {
+	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		Package () {
+		    Package (2) {"phy-mode", "rgmii-id"},
+		    Package (2) {"phy-handle", \_SB.MDI0.PHY2}}
+	    }
+	  })
+	}
+
+References
+==========
+
+[1] Documentation/networking/phy.rst
+
+[2] https://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
+
+[3] Documentation/firmware-guide/acpi/DSD-properties-rules.rst
+
+[4] Documentation/devicetree/bindings/net/ethernet-controller.yaml
-- 
2.17.1

Re: [net-next PATCH v3 01/15] Documentation: ACPI: DSD: Document MDIO PHY

[Randy Dunlap]

Hi,

On 1/12/21 5:40 AM, Calvin Johnson wrote:
> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> provide them to be connected to MAC.
> 
> Describe properties "phy-handle" and "phy-mode".
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
> Changes in v3: None
> Changes in v2:
> - Updated with more description in document
> 
>  Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++
>  1 file changed, 129 insertions(+)
>  create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
> 
> diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
> new file mode 100644
> index 000000000000..a2e4fdcdbf53
> --- /dev/null
> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
> @@ -0,0 +1,129 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================
> +MDIO bus and PHYs in ACPI
> +=========================
> +
> +The PHYs on an MDIO bus [1] are probed and registered using
> +fwnode_mdiobus_register_phy().
> +Later, for connecting these PHYs to MAC, the PHYs registered on the
> +mdiobus have to be referenced.
> +
> +UUID given below should be used as mentioned in the "Device Properties

   The UUID given below

> +UUID For _DSD" [2] document.
> +   - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301
> +
> +This document introduces two _DSD properties that are to be used
> +for PHYs on the MDIO bus.[3]
> +
> +phy-handle
> +----------
> +For each MAC node, a device property "phy-handle" is used to reference
> +the PHY that is registered on an MDIO bus. This is mandatory for
> +network interfaces that have PHYs connected to MAC via MDIO bus.
> +
> +During the MDIO bus driver initialization, PHYs on this bus are probed
> +using the _ADR object as shown below and are registered on the mdio bus.

s/mdio/MDIO/  (please be consistent, as 3 lines above)
> +
> +::
> +      Scope(\_SB.MDI0)
> +      {
> +        Device(PHY1) {
> +          Name (_ADR, 0x1)
> +        } // end of PHY1
> +
> +        Device(PHY2) {
> +          Name (_ADR, 0x2)
> +        } // end of PHY2
> +      }
> +
> +Later, during the MAC driver initialization, the registered PHY devices
> +have to be retrieved from the mdio bus. For this, MAC driver needs

ditto.

> +reference to the previously registered PHYs which are provided
> +using reference to the device as {\_SB.MDI0.PHY1}.
> +
> +phy-mode
> +--------
> +The "phy-mode" _DSD property is used to describe the connection to
> +the PHY. The valid values for "phy-mode" are defined in [4].
> +
> +
> +An ASL example of this is shown below.
> +
> +DSDT entry for MDIO node
> +------------------------
> +The MDIO bus has an SoC component(mdio controller) and a platform

                           component (MDIO controller)

> +component(PHYs on the mdiobus).

   component (PHYs

> +
> +a) Silicon Component
> +This node describes the MDIO controller,MDI0

                                controller, MDI0

> +--------------------------------------------

and then one more '-', please.

> +::
> +	Scope(_SB)
> +	{
> +	  Device(MDI0) {
> +	    Name(_HID, "NXP0006")
> +	    Name(_CCA, 1)
> +	    Name(_UID, 0)
> +	    Name(_CRS, ResourceTemplate() {
> +	      Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
> +	      Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
> +	       {
> +		 MDI0_IT
> +	       }
> +	    }) // end of _CRS for MDI0
> +	  } // end of MDI0
> +	}
> +
> +b) Platform Component
> +This node defines the PHYs that are connected to the MDIO bus, MDI0
> +-------------------------------------------------------------------

[deletia]


thanks.
-- 
~Randy
You can't do anything without having to do something else first.
-- Belefant's Law

[net-next PATCH v3 02/15] net: phy: Introduce fwnode_mdio_find_device()

[Calvin Johnson]

Define fwnode_mdio_find_device() to get a pointer to the
mdio_device from fwnode passed to the function.

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

Changes in v3: None
Changes in v2: None

 drivers/net/mdio/of_mdio.c   | 11 +----------
 drivers/net/phy/phy_device.c | 23 +++++++++++++++++++++++
 include/linux/phy.h          |  6 ++++++
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 4daf94bb56a5..7bd33b930116 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -347,16 +347,7 @@ EXPORT_SYMBOL(of_mdiobus_register);
  */
 struct mdio_device *of_mdio_find_device(struct device_node *np)
 {
-	struct device *d;
-
-	if (!np)
-		return NULL;
-
-	d = bus_find_device_by_of_node(&mdio_bus_type, np);
-	if (!d)
-		return NULL;
-
-	return to_mdio_device(d);
+	return fwnode_mdio_find_device(of_fwnode_handle(np));
 }
 EXPORT_SYMBOL(of_mdio_find_device);
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8447e56ba572..06e0ddcca8c9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2829,6 +2829,29 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->handle_interrupt;
 }
 
+/**
+ * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
+ * @np: pointer to the mdio_device's fwnode
+ *
+ * If successful, returns a pointer to the mdio_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ * The caller should call put_device() on the mdio_device after its use
+ */
+struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)
+{
+	struct device *d;
+
+	if (!fwnode)
+		return NULL;
+
+	d = bus_find_device_by_fwnode(&mdio_bus_type, fwnode);
+	if (!d)
+		return NULL;
+
+	return to_mdio_device(d);
+}
+EXPORT_SYMBOL(fwnode_mdio_find_device);
+
 /**
  * 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 9effb511acde..ce3987e4e615 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1342,11 +1342,17 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     bool is_c45,
 				     struct phy_c45_device_ids *c45_ids);
 #if IS_ENABLED(CONFIG_PHYLIB)
+struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode);
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
 #else
 static inline
+struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)
+{
+	return 0;
+}
+static inline
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
 	return NULL;
-- 
2.17.1

[net-next PATCH v3 03/15] net: phy: 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 v3:
- Add more info on legacy DT properties "phy" and "phy-device"
- Redefine fwnode_phy_find_device() to follow of_phy_find_device()

Changes in v2:
- use reverse christmas tree ordering for local variables

 drivers/net/phy/phy_device.c | 62 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 20 ++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 06e0ddcca8c9..66e779cd905a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi.h>
 #include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -2852,6 +2853,67 @@ struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL(fwnode_mdio_find_device);
 
+/**
+ * fwnode_phy_find_device - For provided phy_fwnode, find phy_device.
+ *
+ * @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 mdio_device *mdiodev;
+
+	mdiodev = fwnode_mdio_find_device(phy_fwnode);
+	if (!mdiodev)
+		return NULL;
+
+	if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+		return to_phy_device(&mdiodev->dev);
+
+	put_device(&mdiodev->dev);
+
+	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
+ *
+ * Refer return conditions of fwnode_phy_find_device().
+ */
+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.
+ *
+ * Refer return conditions of fwnode_find_reference().
+ * For ACPI, only "phy-handle" is supported. Legacy DT properties "phy"
+ * and "phy-device" are not supported in ACPI. DT supports all the three
+ * named references to the phy node.
+ */
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *phy_node;
+
+	/* Only phy-handle is used for ACPI */
+	phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
+	if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
+		return phy_node;
+	phy_node = fwnode_find_reference(fwnode, "phy", 0);
+	if (IS_ERR(phy_node))
+		phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
+	return phy_node;
+}
+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 ce3987e4e615..46b9637dc27e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1343,6 +1343,9 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     struct phy_c45_device_ids *c45_ids);
 #if IS_ENABLED(CONFIG_PHYLIB)
 struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode);
+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);
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
@@ -1352,6 +1355,23 @@ struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)
 {
 	return 0;
 }
+static inline
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	return NULL;
+}
+
+static inline struct phy_device *device_phy_find_device(struct device *dev)
+{
+	return NULL;
+}
+
+static inline
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
 static inline
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
-- 
2.17.1

[net-next PATCH v3 04/15] of: mdio: Refactor of_phy_find_device()

[Calvin Johnson]

Refactor of_phy_find_device() to use fwnode_phy_find_device().

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

Changes in v3: None
Changes in v2: None

 drivers/net/mdio/of_mdio.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 7bd33b930116..94ec421dd91b 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -360,18 +360,7 @@ EXPORT_SYMBOL(of_mdio_find_device);
  */
 struct phy_device *of_phy_find_device(struct device_node *phy_np)
 {
-	struct mdio_device *mdiodev;
-
-	mdiodev = of_mdio_find_device(phy_np);
-	if (!mdiodev)
-		return NULL;
-
-	if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
-		return to_phy_device(&mdiodev->dev);
-
-	put_device(&mdiodev->dev);
-
-	return NULL;
+	return fwnode_phy_find_device(of_fwnode_handle(phy_np));
 }
 EXPORT_SYMBOL(of_phy_find_device);
 
-- 
2.17.1

[net-next PATCH v3 05/15] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

Extract phy_id from compatible string. This will be used by
fwnode_mdiobus_register_phy() to create phy device using the
phy_id.

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

Changes in v3:
- Use traditional comparison pattern
- Use GENMASK

Changes in v2: None

 drivers/net/phy/phy_device.c | 21 +++++++++++++++++++++
 include/linux/phy.h          |  5 +++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 66e779cd905a..6ebb67a19e69 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -846,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
 	return 0;
 }
 
+/* Extract the phy ID from the compatible string of the form
+ * ethernet-phy-idAAAA.BBBB.
+ */
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+	unsigned int upper, lower;
+	const char *cp;
+	int ret;
+
+	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
+	if (ret)
+		return ret;
+
+	if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) != 2)
+		return -EINVAL;
+
+	*phy_id = ((upper & GENMASK(15, 0)) << 16) | (lower & GENMASK(15, 0));
+	return 0;
+}
+EXPORT_SYMBOL(fwnode_get_phy_id);
+
 /**
  * get_phy_device - reads the specified PHY device and returns its @phy_device
  *		    struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 46b9637dc27e..28cd111f1b09 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1342,6 +1342,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     bool is_c45,
 				     struct phy_c45_device_ids *c45_ids);
 #if IS_ENABLED(CONFIG_PHYLIB)
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
 struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode);
 struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
 struct phy_device *device_phy_find_device(struct device *dev);
@@ -1350,6 +1351,10 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
 #else
+static inline int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+	return 0;
+}
 static inline
 struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)
 {
-- 
2.17.1

[net-next PATCH v3 06/15] of: mdio: Refactor of_get_phy_id()

[Calvin Johnson]

With the introduction of fwnode_get_phy_id(), refactor of_get_phy_id()
to use fwnode equivalent.

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

Changes in v3: None
Changes in v2: None

 drivers/net/mdio/of_mdio.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 94ec421dd91b..d4cc293358f7 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -29,17 +29,7 @@ MODULE_LICENSE("GPL");
  * ethernet-phy-idAAAA.BBBB */
 static int of_get_phy_id(struct device_node *device, u32 *phy_id)
 {
-	struct property *prop;
-	const char *cp;
-	unsigned int upper, lower;
-
-	of_property_for_each_string(device, "compatible", prop, cp) {
-		if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
-			*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
-			return 0;
-		}
-	}
-	return -EINVAL;
+	return fwnode_get_phy_id(of_fwnode_handle(device), phy_id);
 }
 
 static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
-- 
2.17.1

[net-next PATCH v3 07/15] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Calvin Johnson]

Introduce fwnode_mdiobus_register_phy() to register PHYs on the
mdiobus. From the compatible string, identify whether the PHY is
c45 and based on this create a PHY device instance which is
registered on the mdiobus.

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

Changes in v3: None
Changes in v2: None

 drivers/net/mdio/of_mdio.c |  3 +-
 drivers/net/phy/mdio_bus.c | 67 ++++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h       |  2 ++
 include/linux/of_mdio.h    |  6 +++-
 4 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index d4cc293358f7..cd7da38ae763 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -32,7 +32,7 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
 	return fwnode_get_phy_id(of_fwnode_handle(device), phy_id);
 }
 
-static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
+struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
 {
 	struct of_phandle_args arg;
 	int err;
@@ -49,6 +49,7 @@ static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
 
 	return register_mii_timestamper(arg.np, arg.args[0]);
 }
+EXPORT_SYMBOL(of_find_mii_timestamper);
 
 int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
 			      struct device_node *child, u32 addr)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 040509b81f02..44ddfb0ba99f 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -8,6 +8,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/errno.h>
@@ -106,6 +107,72 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev)
 }
 EXPORT_SYMBOL(mdiobus_unregister_device);
 
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+				struct fwnode_handle *child, u32 addr)
+{
+	struct mii_timestamper *mii_ts;
+	struct phy_device *phy;
+	bool is_c45 = false;
+	u32 phy_id;
+	int rc;
+
+	if (is_of_node(child)) {
+		mii_ts = of_find_mii_timestamper(to_of_node(child));
+		if (IS_ERR(mii_ts))
+			return PTR_ERR(mii_ts);
+	}
+
+	rc = fwnode_property_match_string(child, "compatible", "ethernet-phy-ieee802.3-c45");
+	if (rc >= 0)
+		is_c45 = true;
+
+	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
+		phy = get_phy_device(bus, addr, is_c45);
+	else
+		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+	if (IS_ERR(phy)) {
+		if (mii_ts && is_of_node(child))
+			unregister_mii_timestamper(mii_ts);
+		return PTR_ERR(phy);
+	}
+
+	if (is_acpi_node(child)) {
+		phy->irq = bus->irq[addr];
+
+		/* Associate the fwnode with the device structure so it
+		 * can be looked up later.
+		 */
+		phy->mdio.dev.fwnode = child;
+
+		/* All data is now stored in the phy struct, so register it */
+		rc = phy_device_register(phy);
+		if (rc) {
+			phy_device_free(phy);
+			fwnode_handle_put(phy->mdio.dev.fwnode);
+			return rc;
+		}
+
+		dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
+	} else if (is_of_node(child)) {
+		rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr);
+		if (rc) {
+			if (mii_ts)
+				unregister_mii_timestamper(mii_ts);
+			phy_device_free(phy);
+			return rc;
+		}
+
+		/* phy->mii_ts may already be defined by the PHY driver. A
+		 * mii_timestamper probed via the device tree will still have
+		 * precedence.
+		 */
+		if (mii_ts)
+			phy->mii_ts = mii_ts;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
+
 struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
 {
 	struct mdio_device *mdiodev = bus->mdio_map[addr];
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index dbd69b3d170b..f138ec333725 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -368,6 +368,8 @@ int mdiobus_register_device(struct mdio_device *mdiodev);
 int mdiobus_unregister_device(struct mdio_device *mdiodev);
 bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
 struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+				      struct fwnode_handle *child, u32 addr);
 
 /**
  * mdio_module_driver() - Helper macro for registering mdio drivers
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index cfe8c607a628..3b66016f18aa 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -34,6 +34,7 @@ struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
 int of_phy_register_fixed_link(struct device_node *np);
 void of_phy_deregister_fixed_link(struct device_node *np);
 bool of_phy_is_fixed_link(struct device_node *np);
+struct mii_timestamper *of_find_mii_timestamper(struct device_node *np);
 int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
 				   struct device_node *child, u32 addr);
 
@@ -128,7 +129,10 @@ static inline bool of_phy_is_fixed_link(struct device_node *np)
 {
 	return false;
 }
-
+static inline struct mii_timestamper *of_find_mii_timestamper(struct device_node *np)
+{
+	return NULL;
+}
 static inline int of_mdiobus_phy_device_register(struct mii_bus *mdio,
 					    struct phy_device *phy,
 					    struct device_node *child, u32 addr)
-- 
2.17.1

[net-next PATCH v3 08/15] of: mdio: Refactor of_mdiobus_register_phy()

[Calvin Johnson]

Refactor of_mdiobus_register_phy() to use fwnode_mdiobus_register_phy().

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

Changes in v3: None
Changes in v2: None

 drivers/net/mdio/of_mdio.c | 40 +-------------------------------------
 1 file changed, 1 insertion(+), 39 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index cd7da38ae763..1b561849269e 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -98,45 +98,7 @@ EXPORT_SYMBOL(of_mdiobus_phy_device_register);
 static int of_mdiobus_register_phy(struct mii_bus *mdio,
 				    struct device_node *child, u32 addr)
 {
-	struct mii_timestamper *mii_ts;
-	struct phy_device *phy;
-	bool is_c45;
-	int rc;
-	u32 phy_id;
-
-	mii_ts = of_find_mii_timestamper(child);
-	if (IS_ERR(mii_ts))
-		return PTR_ERR(mii_ts);
-
-	is_c45 = of_device_is_compatible(child,
-					 "ethernet-phy-ieee802.3-c45");
-
-	if (!is_c45 && !of_get_phy_id(child, &phy_id))
-		phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
-	else
-		phy = get_phy_device(mdio, addr, is_c45);
-	if (IS_ERR(phy)) {
-		if (mii_ts)
-			unregister_mii_timestamper(mii_ts);
-		return PTR_ERR(phy);
-	}
-
-	rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);
-	if (rc) {
-		if (mii_ts)
-			unregister_mii_timestamper(mii_ts);
-		phy_device_free(phy);
-		return rc;
-	}
-
-	/* phy->mii_ts may already be defined by the PHY driver. A
-	 * mii_timestamper probed via the device tree will still have
-	 * precedence.
-	 */
-	if (mii_ts)
-		phy->mii_ts = mii_ts;
-
-	return 0;
+	return fwnode_mdiobus_register_phy(mdio, of_fwnode_handle(child), addr);
 }
 
 static int of_mdiobus_register_device(struct mii_bus *mdio,
-- 
2.17.1

[net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Calvin Johnson]

Using fwnode_get_id(), get the reg property value for DT node
or get the _ADR object value for ACPI node.

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

Changes in v3:
- Modified to retrieve reg property value for ACPI as well
- Resolved compilation issue with CONFIG_ACPI = n
- Added more info into documentation

Changes in v2: None

 drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
 include/linux/property.h |  1 +
 2 files changed, 34 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 35b95c6ac0c6..2d51108cb936 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
 	return fwnode_call_ptr_op(fwnode, get_name_prefix);
 }
 
+/**
+ * fwnode_get_id - Get the id of a fwnode.
+ * @fwnode: firmware node
+ * @id: id of the fwnode
+ *
+ * This function provides the id of a fwnode which can be either
+ * DT or ACPI node. For ACPI, "reg" property value, if present will
+ * be provided or else _ADR value will be provided.
+ * Returns 0 on success or a negative errno.
+ */
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
+{
+#ifdef CONFIG_ACPI
+	unsigned long long adr;
+	acpi_status status;
+#endif
+	int ret;
+
+	ret = fwnode_property_read_u32(fwnode, "reg", id);
+	if (!(ret && is_acpi_node(fwnode)))
+		return ret;
+
+#ifdef CONFIG_ACPI
+	status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+				       METHOD_NAME__ADR, NULL, &adr);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+	*id = (u32)adr;
+#endif
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_id);
+
 /**
  * fwnode_get_parent - Return parent firwmare node
  * @fwnode: Firmware whose parent is retrieved
diff --git a/include/linux/property.h b/include/linux/property.h
index 0a9001fe7aea..3f41475f010b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
 
 const char *fwnode_get_name(const struct fwnode_handle *fwnode);
 const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id);
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
-- 
2.17.1

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Andy Shevchenko]

On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Using fwnode_get_id(), get the reg property value for DT node
> or get the _ADR object value for ACPI node.
>
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
>
> Changes in v3:
> - Modified to retrieve reg property value for ACPI as well
> - Resolved compilation issue with CONFIG_ACPI = n
> - Added more info into documentation
>
> Changes in v2: None
>
>  drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
>  include/linux/property.h |  1 +
>  2 files changed, 34 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 35b95c6ac0c6..2d51108cb936 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
>         return fwnode_call_ptr_op(fwnode, get_name_prefix);
>  }
>
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
> + * This function provides the id of a fwnode which can be either
> + * DT or ACPI node. For ACPI, "reg" property value, if present will
> + * be provided or else _ADR value will be provided.
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> +#ifdef CONFIG_ACPI
> +       unsigned long long adr;
> +       acpi_status status;
> +#endif
> +       int ret;
> +
> +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> +       if (!(ret && is_acpi_node(fwnode)))
> +               return ret;
> +
> +#ifdef CONFIG_ACPI
> +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> +                                      METHOD_NAME__ADR, NULL, &adr);
> +       if (ACPI_FAILURE(status))
> +               return -EINVAL;
> +       *id = (u32)adr;

Shouldn't be

       return 0;
#else
       return -EINVAL;
#endif

?

Yes, it's a theoretical case when is_acpi_node() returns true when
CONFIG_ACPI=n.

> +#endif
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_id);
> +
>  /**
>   * fwnode_get_parent - Return parent firwmare node
>   * @fwnode: Firmware whose parent is retrieved
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 0a9001fe7aea..3f41475f010b 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
>
>  const char *fwnode_get_name(const struct fwnode_handle *fwnode);
>  const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id);
>  struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_get_next_parent(
>         struct fwnode_handle *fwnode);
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Rafael J. Wysocki]

On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > or get the _ADR object value for ACPI node.
> >
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
> >
> > Changes in v3:
> > - Modified to retrieve reg property value for ACPI as well
> > - Resolved compilation issue with CONFIG_ACPI = n
> > - Added more info into documentation
> >
> > Changes in v2: None
> >
> >  drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
> >  include/linux/property.h |  1 +
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 35b95c6ac0c6..2d51108cb936 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
> >         return fwnode_call_ptr_op(fwnode, get_name_prefix);
> >  }
> >
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * This function provides the id of a fwnode which can be either
> > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > + * be provided or else _ADR value will be provided.
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +#ifdef CONFIG_ACPI
> > +       unsigned long long adr;
> > +       acpi_status status;
> > +#endif
> > +       int ret;
> > +
> > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > +       if (!(ret && is_acpi_node(fwnode)))
> > +               return ret;
> > +
> > +#ifdef CONFIG_ACPI
> > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +                                      METHOD_NAME__ADR, NULL, &adr);
> > +       if (ACPI_FAILURE(status))
> > +               return -EINVAL;
> > +       *id = (u32)adr;
>
> Shouldn't be
>
>        return 0;
> #else
>        return -EINVAL;
> #endif
>
> ?
>
> Yes, it's a theoretical case when is_acpi_node() returns true when
> CONFIG_ACPI=n.

How so?  is_acpi_node() is defined as a static inline returning false then.

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Andy Shevchenko]

On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:

...

> > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > > +{
> > > +#ifdef CONFIG_ACPI
> > > +       unsigned long long adr;
> > > +       acpi_status status;
> > > +#endif
> > > +       int ret;
> > > +
> > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > +       if (!(ret && is_acpi_node(fwnode)))
> > > +               return ret;
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > +       if (ACPI_FAILURE(status))
> > > +               return -EINVAL;
> > > +       *id = (u32)adr;
> >
> > Shouldn't be
> >
> >        return 0;
> > #else
> >        return -EINVAL;
> > #endif
> >
> > ?
> >
> > Yes, it's a theoretical case when is_acpi_node() returns true when
> > CONFIG_ACPI=n.
>
> How so?  is_acpi_node() is defined as a static inline returning false then.

I understand that, that's why it's pure theoretical when, for example,
the semantics is changed. But I believe it's unlucky to happen.

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Rafael J. Wysocki]

On Wed, Jan 20, 2021 at 7:44 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
> > > <calvin.johnson@oss.nxp.com> wrote:
>
> ...
>
> > > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > > > +{
> > > > +#ifdef CONFIG_ACPI
> > > > +       unsigned long long adr;
> > > > +       acpi_status status;
> > > > +#endif
> > > > +       int ret;
> > > > +
> > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > +               return ret;
> > > > +
> > > > +#ifdef CONFIG_ACPI
> > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > +       if (ACPI_FAILURE(status))
> > > > +               return -EINVAL;
> > > > +       *id = (u32)adr;
> > >
> > > Shouldn't be
> > >
> > >        return 0;
> > > #else
> > >        return -EINVAL;
> > > #endif
> > >
> > > ?
> > >
> > > Yes, it's a theoretical case when is_acpi_node() returns true when
> > > CONFIG_ACPI=n.
> >
> > How so?  is_acpi_node() is defined as a static inline returning false then.
>
> I understand that, that's why it's pure theoretical when, for example,
> the semantics is changed. But I believe it's unlucky to happen.

Changing the definition of it for CONFIG_ACPI=n would be a regression
given the current usage of it.

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Saravana Kannan]

On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Using fwnode_get_id(), get the reg property value for DT node
> or get the _ADR object value for ACPI node.
>
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
>
> Changes in v3:
> - Modified to retrieve reg property value for ACPI as well
> - Resolved compilation issue with CONFIG_ACPI = n
> - Added more info into documentation
>
> Changes in v2: None
>
>  drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
>  include/linux/property.h |  1 +
>  2 files changed, 34 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 35b95c6ac0c6..2d51108cb936 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
>         return fwnode_call_ptr_op(fwnode, get_name_prefix);
>  }
>
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
> + * This function provides the id of a fwnode which can be either
> + * DT or ACPI node. For ACPI, "reg" property value, if present will
> + * be provided or else _ADR value will be provided.
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> +#ifdef CONFIG_ACPI
> +       unsigned long long adr;
> +       acpi_status status;
> +#endif
> +       int ret;
> +
> +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> +       if (!(ret && is_acpi_node(fwnode)))
> +               return ret;
> +
> +#ifdef CONFIG_ACPI
> +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> +                                      METHOD_NAME__ADR, NULL, &adr);
> +       if (ACPI_FAILURE(status))
> +               return -EINVAL;
> +       *id = (u32)adr;
> +#endif
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_id);

Please don't do it this way. The whole point of fwnode_operations is
to avoid conditional stuff at the fwnode level. Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.

Also, can you CC me on the entire series please? I want to reply to
some of your other patches too. Most of the fwnode changes don't seem
right. fwnode is lower level that the device-driver framework. Making
it aware of busses like mdio, etc doesn't sound right. Also, there's
already get_dev_from_fwnode() which is a much more efficient way to
look up/get a device from a fwnode instead of looping through a bus.

-Saravana

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Andy Shevchenko]

On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > or get the _ADR object value for ACPI node.

...

> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * This function provides the id of a fwnode which can be either
> > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > + * be provided or else _ADR value will be provided.
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +#ifdef CONFIG_ACPI
> > +       unsigned long long adr;
> > +       acpi_status status;
> > +#endif
> > +       int ret;
> > +
> > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > +       if (!(ret && is_acpi_node(fwnode)))
> > +               return ret;
> > +
> > +#ifdef CONFIG_ACPI
> > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +                                      METHOD_NAME__ADR, NULL, &adr);
> > +       if (ACPI_FAILURE(status))
> > +               return -EINVAL;
> > +       *id = (u32)adr;
> > +#endif
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(fwnode_get_id);

> Please don't do it this way. The whole point of fwnode_operations is
> to avoid conditional stuff at the fwnode level.

Not fully true. We have non-POD getters that are conditional. Moreover,
we have additional layer of Primary / Secondary fwnodes on top of that.

The caller of fwnode API is indeed agnostic, but under the hood it differs by
the definition (obviously due to natural differences between ACPI and DT and
whatever else might come in the future.

> Also ACPI and DT
> aren't mutually exclusive if I'm not mistaken.

That's why we try 'reg' property for both cases first.

is_acpi_fwnode() conditional is that what I don't like though.

...

> fwnode is lower level that the device-driver framework.

Agree.

> Making
> it aware of busses like mdio, etc doesn't sound right.

Disagree. Conceptually resource providers can be quite different and fwnode API
*is* LCM for them.

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Rafael J. Wysocki]

On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:
> > >
> > > Using fwnode_get_id(), get the reg property value for DT node
> > > or get the _ADR object value for ACPI node.
>
> ...
>
> > > +/**
> > > + * fwnode_get_id - Get the id of a fwnode.
> > > + * @fwnode: firmware node
> > > + * @id: id of the fwnode
> > > + *
> > > + * This function provides the id of a fwnode which can be either
> > > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > > + * be provided or else _ADR value will be provided.
> > > + * Returns 0 on success or a negative errno.
> > > + */
> > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > > +{
> > > +#ifdef CONFIG_ACPI
> > > +       unsigned long long adr;
> > > +       acpi_status status;
> > > +#endif
> > > +       int ret;
> > > +
> > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > +       if (!(ret && is_acpi_node(fwnode)))
> > > +               return ret;
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > +       if (ACPI_FAILURE(status))
> > > +               return -EINVAL;
> > > +       *id = (u32)adr;
> > > +#endif
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(fwnode_get_id);
>
> > Please don't do it this way. The whole point of fwnode_operations is
> > to avoid conditional stuff at the fwnode level.
>
> Not fully true. We have non-POD getters that are conditional. Moreover,
> we have additional layer of Primary / Secondary fwnodes on top of that.
>
> The caller of fwnode API is indeed agnostic, but under the hood it differs by
> the definition (obviously due to natural differences between ACPI and DT and
> whatever else might come in the future.
>
> > Also ACPI and DT
> > aren't mutually exclusive if I'm not mistaken.
>
> That's why we try 'reg' property for both cases first.
>
> is_acpi_fwnode() conditional is that what I don't like though.

I'm not sure what you mean here, care to elaborate?

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Andy Shevchenko]

On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > <calvin.johnson@oss.nxp.com> wrote:

...

> > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > +               return ret;
> > > > +
> > > > +#ifdef CONFIG_ACPI
> > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > +       if (ACPI_FAILURE(status))
> > > > +               return -EINVAL;
> > > > +       *id = (u32)adr;
> > > > +#endif
> > > > +       return 0;

> > > Also ACPI and DT
> > > aren't mutually exclusive if I'm not mistaken.
> >
> > That's why we try 'reg' property for both cases first.
> >
> > is_acpi_fwnode() conditional is that what I don't like though.
>
> I'm not sure what you mean here, care to elaborate?

I meant is_acpi_node(fwnode) in the conditional.

I think it's redundant and we can simple do something like this:

  if (ret) {
#ifdef ACPI
    ...
#else
    return ret;
#endif
  }
  return 0;

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Rafael J. Wysocki]

On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > > <calvin.johnson@oss.nxp.com> wrote:
>
> ...
>
> > > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > > +               return ret;
> > > > > +
> > > > > +#ifdef CONFIG_ACPI
> > > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > > +       if (ACPI_FAILURE(status))
> > > > > +               return -EINVAL;
> > > > > +       *id = (u32)adr;
> > > > > +#endif
> > > > > +       return 0;
>
> > > > Also ACPI and DT
> > > > aren't mutually exclusive if I'm not mistaken.
> > >
> > > That's why we try 'reg' property for both cases first.
> > >
> > > is_acpi_fwnode() conditional is that what I don't like though.
> >
> > I'm not sure what you mean here, care to elaborate?
>
> I meant is_acpi_node(fwnode) in the conditional.
>
> I think it's redundant and we can simple do something like this:
>
>   if (ret) {
> #ifdef ACPI
>     ...
> #else
>     return ret;
> #endif
>   }
>   return 0;
>
> --

Right, that should work.  And I'd prefer it too.

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Saravana Kannan]

On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > > > <calvin.johnson@oss.nxp.com> wrote:
> >
> > ...
> >
> > > > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > > > +               return ret;
> > > > > > +
> > > > > > +#ifdef CONFIG_ACPI
> > > > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > > > +       if (ACPI_FAILURE(status))
> > > > > > +               return -EINVAL;
> > > > > > +       *id = (u32)adr;
> > > > > > +#endif
> > > > > > +       return 0;
> >
> > > > > Also ACPI and DT
> > > > > aren't mutually exclusive if I'm not mistaken.
> > > >
> > > > That's why we try 'reg' property for both cases first.
> > > >
> > > > is_acpi_fwnode() conditional is that what I don't like though.
> > >
> > > I'm not sure what you mean here, care to elaborate?
> >
> > I meant is_acpi_node(fwnode) in the conditional.
> >
> > I think it's redundant and we can simple do something like this:
> >
> >   if (ret) {
> > #ifdef ACPI
> >     ...
> > #else
> >     return ret;
> > #endif
> >   }
> >   return 0;
> >
> > --
>
> Right, that should work.  And I'd prefer it too.

Rafael,

I'd rather this new function be an ops instead of a bunch of #ifdef or
if (acpi) checks. Thoughts?

-Saravana

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Rafael J. Wysocki]

On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > > > > <calvin.johnson@oss.nxp.com> wrote:
> > >
> > > ...
> > >
> > > > > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > > > > +               return ret;
> > > > > > > +
> > > > > > > +#ifdef CONFIG_ACPI
> > > > > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > > > > +       if (ACPI_FAILURE(status))
> > > > > > > +               return -EINVAL;
> > > > > > > +       *id = (u32)adr;
> > > > > > > +#endif
> > > > > > > +       return 0;
> > >
> > > > > > Also ACPI and DT
> > > > > > aren't mutually exclusive if I'm not mistaken.
> > > > >
> > > > > That's why we try 'reg' property for both cases first.
> > > > >
> > > > > is_acpi_fwnode() conditional is that what I don't like though.
> > > >
> > > > I'm not sure what you mean here, care to elaborate?
> > >
> > > I meant is_acpi_node(fwnode) in the conditional.
> > >
> > > I think it's redundant and we can simple do something like this:
> > >
> > >   if (ret) {
> > > #ifdef ACPI
> > >     ...
> > > #else
> > >     return ret;
> > > #endif
> > >   }
> > >   return 0;
> > >
> > > --
> >
> > Right, that should work.  And I'd prefer it too.
>
> Rafael,
>
> I'd rather this new function be an ops instead of a bunch of #ifdef or
> if (acpi) checks. Thoughts?

Well, it looks more like a helper function than like an op and I'm not
even sure how many potential users of it will expect that _ADR should
be evaluated in the absence of the "reg" property.

It's just that the "reg" property happens to be kind of an _ADR
equivalent in this particular binding AFAICS.

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Saravana Kannan]

On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > > > > > <calvin.johnson@oss.nxp.com> wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > > > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > > > > > +               return ret;
> > > > > > > > +
> > > > > > > > +#ifdef CONFIG_ACPI
> > > > > > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > > > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > > > > > +       if (ACPI_FAILURE(status))
> > > > > > > > +               return -EINVAL;
> > > > > > > > +       *id = (u32)adr;
> > > > > > > > +#endif
> > > > > > > > +       return 0;
> > > >
> > > > > > > Also ACPI and DT
> > > > > > > aren't mutually exclusive if I'm not mistaken.
> > > > > >
> > > > > > That's why we try 'reg' property for both cases first.
> > > > > >
> > > > > > is_acpi_fwnode() conditional is that what I don't like though.
> > > > >
> > > > > I'm not sure what you mean here, care to elaborate?
> > > >
> > > > I meant is_acpi_node(fwnode) in the conditional.
> > > >
> > > > I think it's redundant and we can simple do something like this:
> > > >
> > > >   if (ret) {
> > > > #ifdef ACPI
> > > >     ...
> > > > #else
> > > >     return ret;
> > > > #endif
> > > >   }
> > > >   return 0;
> > > >
> > > > --
> > >
> > > Right, that should work.  And I'd prefer it too.
> >
> > Rafael,
> >
> > I'd rather this new function be an ops instead of a bunch of #ifdef or
> > if (acpi) checks. Thoughts?
>
> Well, it looks more like a helper function than like an op and I'm not
> even sure how many potential users of it will expect that _ADR should
> be evaluated in the absence of the "reg" property.
>
> It's just that the "reg" property happens to be kind of an _ADR
> equivalent in this particular binding AFAICS.

I agree it is not clear how useful this helper function is going to be.

But in general, to me, any time the wrapper/helper functions in
drivers/base/property.c need to do something like this:

if (ACPI)
   ACPI specific code
if (OF)
   OF specific code

I think the code should be pushed to the fwnode ops. That's one of the
main point of fwnode. So that firmware specific stuff is done by
firmware specific code. Also, when adding support for new firmware,
it's pretty clear what support the firmware needs to implement.
Instead of having to go fix up a bunch of code all over the place.

So fwnode_ops->get_id() would be the OP ACPI and OF would implement.
And then we can have a wrapper in drivers/base/property.c.

-Saravana

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Andy Shevchenko]

On Fri, Jan 22, 2021 at 10:59 PM Saravana Kannan <saravanak@google.com> wrote:
> On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote:
> > > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:


> > > I'd rather this new function be an ops instead of a bunch of #ifdef or
> > > if (acpi) checks. Thoughts?
> >
> > Well, it looks more like a helper function than like an op and I'm not
> > even sure how many potential users of it will expect that _ADR should
> > be evaluated in the absence of the "reg" property.
> >
> > It's just that the "reg" property happens to be kind of an _ADR
> > equivalent in this particular binding AFAICS.
>
> I agree it is not clear how useful this helper function is going to be.
>
> But in general, to me, any time the wrapper/helper functions in
> drivers/base/property.c need to do something like this:
>
> if (ACPI)
>    ACPI specific code
> if (OF)
>    OF specific code
>
> I think the code should be pushed to the fwnode ops. That's one of the
> main point of fwnode. So that firmware specific stuff is done by
> firmware specific code. Also, when adding support for new firmware,
> it's pretty clear what support the firmware needs to implement.
> Instead of having to go fix up a bunch of code all over the place.

Wishful thinking.
In the very case of GPIO it's related to framework using headers local
to framework. Are you suggesting to open its guts to the entire wild
world?
I don't think it's a good idea. You see, here we have different
layering POD types, which are natural and quite low level that ops
suits best for them and quite different resource types like GPIO. And
the latter is closer to certain framework rather than to POD handling
cases.

> So fwnode_ops->get_id() would be the OP ACPI and OF would implement.
> And then we can have a wrapper in drivers/base/property.c.


-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Saravana Kannan]

On Fri, Jan 22, 2021 at 1:05 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Jan 22, 2021 at 10:59 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
>
> > > > I'd rather this new function be an ops instead of a bunch of #ifdef or
> > > > if (acpi) checks. Thoughts?
> > >
> > > Well, it looks more like a helper function than like an op and I'm not
> > > even sure how many potential users of it will expect that _ADR should
> > > be evaluated in the absence of the "reg" property.
> > >
> > > It's just that the "reg" property happens to be kind of an _ADR
> > > equivalent in this particular binding AFAICS.
> >
> > I agree it is not clear how useful this helper function is going to be.
> >
> > But in general, to me, any time the wrapper/helper functions in
> > drivers/base/property.c need to do something like this:
> >
> > if (ACPI)
> >    ACPI specific code
> > if (OF)
> >    OF specific code
> >
> > I think the code should be pushed to the fwnode ops. That's one of the
> > main point of fwnode. So that firmware specific stuff is done by
> > firmware specific code. Also, when adding support for new firmware,
> > it's pretty clear what support the firmware needs to implement.
> > Instead of having to go fix up a bunch of code all over the place.
>
> Wishful thinking.
> In the very case of GPIO it's related to framework using headers local
> to framework. Are you suggesting to open its guts to the entire wild
> world?

What are you even talking about? How is whatever you are saying
remotely related to getting an "id" for a fwnode?

> I don't think it's a good idea. You see, here we have different
> layering POD types,

POD?

> which are natural and quite low level that ops
> suits best for them and quite different resource types like GPIO. And
> the latter is closer to certain framework rather than to POD handling
> cases.
>

-Saravana

Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

[Calvin Johnson]

Hi,

On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > or get the _ADR object value for ACPI node.
> >
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
> >
> > Changes in v3:
> > - Modified to retrieve reg property value for ACPI as well
> > - Resolved compilation issue with CONFIG_ACPI = n
> > - Added more info into documentation
> >
> > Changes in v2: None
> >
> >  drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
> >  include/linux/property.h |  1 +
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 35b95c6ac0c6..2d51108cb936 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
> >         return fwnode_call_ptr_op(fwnode, get_name_prefix);
> >  }
> >
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * This function provides the id of a fwnode which can be either
> > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > + * be provided or else _ADR value will be provided.
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +#ifdef CONFIG_ACPI
> > +       unsigned long long adr;
> > +       acpi_status status;
> > +#endif
> > +       int ret;
> > +
> > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > +       if (!(ret && is_acpi_node(fwnode)))
> > +               return ret;
> > +
> > +#ifdef CONFIG_ACPI
> > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +                                      METHOD_NAME__ADR, NULL, &adr);
> > +       if (ACPI_FAILURE(status))
> > +               return -EINVAL;
> > +       *id = (u32)adr;
> > +#endif
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(fwnode_get_id);
> 
> Please don't do it this way. The whole point of fwnode_operations is
> to avoid conditional stuff at the fwnode level. Also ACPI and DT
> aren't mutually exclusive if I'm not mistaken.
> 
> Also, can you CC me on the entire series please? I want to reply to
> some of your other patches too. Most of the fwnode changes don't seem
> right. fwnode is lower level that the device-driver framework. Making
> it aware of busses like mdio, etc doesn't sound right. Also, there's
> already get_dev_from_fwnode() which is a much more efficient way to
> look up/get a device from a fwnode instead of looping through a bus.

Thanks for reviewing the patch. I'll add you in the upcoming v4 series.
There is lot of history into patch series. It would be helpful, if you can
search for related submissions in the past and get some background.

Regards
Calvin

[net-next PATCH v3 10/15] net: mdio: Add ACPI support code for mdio

[Calvin Johnson]

Define acpi_mdiobus_register() to Register mii_bus and create PHYs for
each ACPI child node.

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

Changes in v3: None
Changes in v2: None

 MAINTAINERS                  |  1 +
 drivers/net/mdio/Kconfig     |  7 ++++++
 drivers/net/mdio/Makefile    |  1 +
 drivers/net/mdio/acpi_mdio.c | 49 ++++++++++++++++++++++++++++++++++++
 include/linux/acpi_mdio.h    | 27 ++++++++++++++++++++
 5 files changed, 85 insertions(+)
 create mode 100644 drivers/net/mdio/acpi_mdio.c
 create mode 100644 include/linux/acpi_mdio.h

diff --git a/MAINTAINERS b/MAINTAINERS
index bfd13ae685d4..efb4bae8d622 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6658,6 +6658,7 @@ F:	Documentation/devicetree/bindings/net/mdio*
 F:	Documentation/devicetree/bindings/net/qca,ar803x.yaml
 F:	Documentation/networking/phy.rst
 F:	drivers/net/mdio/
+F:	drivers/net/mdio/acpi_mdio.c
 F:	drivers/net/mdio/of_mdio.c
 F:	drivers/net/pcs/
 F:	drivers/net/phy/
diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index a10cc460d7cf..df6bb7837d6a 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -27,6 +27,13 @@ config OF_MDIO
 	help
 	  OpenFirmware MDIO bus (Ethernet PHY) accessors
 
+config ACPI_MDIO
+	def_tristate PHYLIB
+	depends on ACPI
+	depends on PHYLIB
+	help
+	  ACPI MDIO bus (Ethernet PHY) accessors
+
 if MDIO_BUS
 
 config MDIO_DEVRES
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 5c498dde463f..2373ade8af13 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -2,6 +2,7 @@
 # Makefile for Linux MDIO bus drivers
 
 obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
+obj-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
 
 obj-$(CONFIG_MDIO_ASPEED)		+= mdio-aspeed.o
 obj-$(CONFIG_MDIO_BCM_IPROC)		+= mdio-bcm-iproc.o
diff --git a/drivers/net/mdio/acpi_mdio.c b/drivers/net/mdio/acpi_mdio.c
new file mode 100644
index 000000000000..adab5dc9d911
--- /dev/null
+++ b/drivers/net/mdio/acpi_mdio.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ACPI helpers for the MDIO (Ethernet PHY) API
+ *
+ * This file provides helper functions for extracting PHY device information
+ * out of the ACPI ASL and using it to populate an mii_bus.
+ */
+
+#include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
+
+/**
+ * acpi_mdiobus_register - Register mii_bus and create PHYs from the ACPI ASL.
+ *
+ * @mdio: pointer to mii_bus structure
+ * @fwnode: pointer to fwnode of MDIO bus.
+ *
+ * This function registers the mii_bus structure and registers a phy_device
+ * for each child node of @fwnode.
+ */
+int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *child;
+	u32 addr;
+	int ret;
+
+	/* Mask out all PHYs from auto probing. */
+	mdio->phy_mask = ~0;
+	ret = mdiobus_register(mdio);
+	if (ret)
+		return ret;
+
+	mdio->dev.fwnode = fwnode;
+/* Loop over the child nodes and register a phy_device for each PHY */
+	fwnode_for_each_child_node(fwnode, child) {
+		ret = fwnode_get_id(child, &addr);
+
+		if (addr >= PHY_MAX_ADDR)
+			continue;
+
+		ret = fwnode_mdiobus_register_phy(mdio, child, addr);
+		if (ret == -ENODEV)
+			dev_err(&mdio->dev,
+				"MDIO device at address %d is missing.\n",
+				addr);
+	}
+	return 0;
+}
+EXPORT_SYMBOL(acpi_mdiobus_register);
diff --git a/include/linux/acpi_mdio.h b/include/linux/acpi_mdio.h
new file mode 100644
index 000000000000..9be6f63cde8f
--- /dev/null
+++ b/include/linux/acpi_mdio.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * ACPI helpers for the MDIO (Ethernet PHY) API
+ *
+ */
+
+#ifndef __LINUX_ACPI_MDIO_H
+#define __LINUX_ACPI_MDIO_H
+
+#include <linux/device.h>
+#include <linux/phy.h>
+
+#if IS_ENABLED(CONFIG_ACPI_MDIO)
+int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode);
+#else /* CONFIG_ACPI_MDIO */
+static inline int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
+{
+	/*
+	 * Fall back to mdiobus_register() function to register a bus.
+	 * This way, we don't have to keep compat bits around in drivers.
+	 */
+
+	return mdiobus_register(mdio);
+}
+#endif
+
+#endif /* __LINUX_ACPI_MDIO_H */
-- 
2.17.1

[net-next PATCH v3 11/15] net: mdiobus: Introduce fwnode_mdiobus_register()

[Calvin Johnson]

Introduce fwnode_mdiobus_register() to register PHYs on the  mdiobus.
If the fwnode is DT node, then call of_mdiobus_register().
If it is an ACPI node, then call acpi_mdiobus_register().

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

Changes in v3:
- Use acpi_mdiobus_register()

Changes in v2: None

 drivers/net/phy/mdio_bus.c | 20 ++++++++++++++++++++
 include/linux/phy.h        |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 44ddfb0ba99f..01bee3c46d12 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -9,6 +9,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/errno.h>
@@ -568,6 +569,25 @@ static int mdiobus_create_device(struct mii_bus *bus,
 	return ret;
 }
 
+/**
+ * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
+ * @mdio: pointer to mii_bus structure
+ * @fwnode: pointer to fwnode of MDIO bus.
+ *
+ * This function returns of_mdiobus_register() for DT and
+ * acpi_mdiobus_register() for ACPI.
+ */
+int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
+{
+	if (is_of_node(fwnode))
+		return of_mdiobus_register(mdio, to_of_node(fwnode));
+	else if (is_acpi_node(fwnode))
+		return acpi_mdiobus_register(mdio, fwnode);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register);
+
 /**
  * __mdiobus_register - bring up all the PHYs on a given bus and attach them to bus
  * @bus: target mii_bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 28cd111f1b09..0a5c68eab53a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -383,6 +383,7 @@ static inline struct mii_bus *mdiobus_alloc(void)
 	return mdiobus_alloc_size(0);
 }
 
+int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode);
 int __mdiobus_register(struct mii_bus *bus, struct module *owner);
 int __devm_mdiobus_register(struct device *dev, struct mii_bus *bus,
 			    struct module *owner);
-- 
2.17.1

Re: [net-next PATCH v3 11/15] net: mdiobus: Introduce fwnode_mdiobus_register()

[Andy Shevchenko]

On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Introduce fwnode_mdiobus_register() to register PHYs on the  mdiobus.
> If the fwnode is DT node, then call of_mdiobus_register().
> If it is an ACPI node, then call acpi_mdiobus_register().

...

> +/**
> + * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
> + * @mdio: pointer to mii_bus structure
> + * @fwnode: pointer to fwnode of MDIO bus.
> + *
> + * This function returns of_mdiobus_register() for DT and
> + * acpi_mdiobus_register() for ACPI.
> + */
> +int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
> +{
> +       if (is_of_node(fwnode))
> +               return of_mdiobus_register(mdio, to_of_node(fwnode));
> +       else if (is_acpi_node(fwnode))

Redundant 'else'

> +               return acpi_mdiobus_register(mdio, fwnode);
> +
> +       return -EINVAL;
> +}

-- 
With Best Regards,
Andy Shevchenko

[net-next PATCH v3 12/15] net/fsl: Use fwnode_mdiobus_register()

[Calvin Johnson]

fwnode_mdiobus_register() internally takes care of both DT
and ACPI cases to register mdiobus. Replace existing
of_mdiobus_register() with fwnode_mdiobus_register().

Note: For both ACPI and DT cases, endianness of MDIO controller
need to be specified using "little-endian" property.

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

Changes in v3:
- Avoid unnecessary line removal
- Remove unused inclusion of acpi.h

Changes in v2: None

 drivers/net/ethernet/freescale/xgmac_mdio.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index bfa2826c5545..d609c08b445a 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -2,6 +2,7 @@
  * QorIQ 10G MDIO Controller
  *
  * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2020 NXP
  *
  * Authors: Andy Fleming <afleming@freescale.com>
  *          Timur Tabi <timur@freescale.com>
@@ -243,10 +244,9 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 
 static int xgmac_mdio_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct mii_bus *bus;
-	struct resource *res;
 	struct mdio_fsl_priv *priv;
+	struct resource *res;
+	struct mii_bus *bus;
 	int ret;
 
 	/* In DPAA-1, MDIO is one of the many FMan sub-devices. The FMan
@@ -279,13 +279,15 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 		goto err_ioremap;
 	}
 
+	/* For both ACPI and DT cases, endianness of MDIO controller
+	 *  need to be specified using "little-endian" property.
+	 */
 	priv->is_little_endian = device_property_read_bool(&pdev->dev,
 							   "little-endian");
 
 	priv->has_a011043 = device_property_read_bool(&pdev->dev,
 						      "fsl,erratum-a011043");
-
-	ret = of_mdiobus_register(bus, np);
+	ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode);
 	if (ret) {
 		dev_err(&pdev->dev, "cannot register MDIO bus\n");
 		goto err_registration;
-- 
2.17.1

Re: [net-next PATCH v3 12/15] net/fsl: Use fwnode_mdiobus_register()

[Andy Shevchenko]

On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> fwnode_mdiobus_register() internally takes care of both DT
> and ACPI cases to register mdiobus. Replace existing
> of_mdiobus_register() with fwnode_mdiobus_register().
>
> Note: For both ACPI and DT cases, endianness of MDIO controller
> need to be specified using "little-endian" property.


> +       /* For both ACPI and DT cases, endianness of MDIO controller
> +        *  need to be specified using "little-endian" property.

needs

> +        */

...

>         priv->has_a011043 = device_property_read_bool(&pdev->dev,
>                                                       "fsl,erratum-a011043");
> -

Unrelated change.

-- 
With Best Regards,
Andy Shevchenko

[net-next PATCH v3 13/15] phylink: introduce phylink_fwnode_phy_connect()

[Calvin Johnson]

Define phylink_fwnode_phy_connect() to connect phy specified by
a fwnode to a phylink instance.

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

Changes in v3: None
Changes in v2: None

 drivers/net/phy/phylink.c | 54 +++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 57 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 84f6e197f965..389dc3ec165e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -5,6 +5,7 @@
  *
  * Copyright (C) 2015 Russell King
  */
+#include <linux/acpi.h>
 #include <linux/ethtool.h>
 #include <linux/export.h>
 #include <linux/gpio/consumer.h>
@@ -1120,6 +1121,59 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
 }
 EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
 
+/**
+ * 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.
+ *
+ * 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;
+
+	if (is_of_node(fwnode)) {
+		/* 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(phy_fwnode)) {
+		if (pl->cfg_link_an_mode == MLO_AN_PHY)
+			return -ENODEV;
+		return 0;
+	}
+
+	phy_dev = fwnode_phy_find_device(phy_fwnode);
+	/* We're done with the phy_node handle */
+	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_disconnect_phy() - disconnect any PHY attached to the phylink
  *   instance.
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index d81a714cfbbd..75d4f99090fd 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -439,6 +439,9 @@ void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
 int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags);
 void phylink_disconnect_phy(struct phylink *);
 
 void phylink_mac_change(struct phylink *, bool up);
-- 
2.17.1

Re: [net-next PATCH v3 13/15] phylink: introduce phylink_fwnode_phy_connect()

[Andy Shevchenko]

On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Define phylink_fwnode_phy_connect() to connect phy specified by
> a fwnode to a phylink instance.

...

> +       phy_dev = fwnode_phy_find_device(phy_fwnode);
> +       /* We're done with the phy_node handle */
> +       fwnode_handle_put(phy_fwnode);
> +       if (!phy_dev)
> +               return -ENODEV;
> +
> +       ret = phy_attach_direct(pl->netdev, phy_dev, flags,
> +                               pl->link_interface);
> +       if (ret)

Hmm... Shouldn't you put phy_dev here?

> +               return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 13/15] phylink: introduce phylink_fwnode_phy_connect()

[Calvin Johnson]

On Tue, Jan 12, 2021 at 05:55:54PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Define phylink_fwnode_phy_connect() to connect phy specified by
> > a fwnode to a phylink instance.
> 
> ...
> 
> > +       phy_dev = fwnode_phy_find_device(phy_fwnode);
> > +       /* We're done with the phy_node handle */
> > +       fwnode_handle_put(phy_fwnode);
> > +       if (!phy_dev)
> > +               return -ENODEV;
> > +
> > +       ret = phy_attach_direct(pl->netdev, phy_dev, flags,
> > +                               pl->link_interface);
> > +       if (ret)
> 
> Hmm... Shouldn't you put phy_dev here?
I think you are right. We may have to add
		put_device(&phydev->mdio.dev);
It is missing in phylink_of_phy_connect() as well.
> 
> > +               return ret;
> 
> -- 
Thanks
Calvin

[net-next PATCH v3 14/15] net: phylink: Refactor phylink_of_phy_connect()

[Calvin Johnson]

Refactor phylink_of_phy_connect() to use phylink_fwnode_phy_connect().

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

Changes in v3: None
Changes in v2: None

 drivers/net/phy/phylink.c | 39 +--------------------------------------
 1 file changed, 1 insertion(+), 38 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 389dc3ec165e..26f014f0ad42 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1080,44 +1080,7 @@ EXPORT_SYMBOL_GPL(phylink_connect_phy);
 int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
 			   u32 flags)
 {
-	struct device_node *phy_node;
-	struct phy_device *phy_dev;
-	int ret;
-
-	/* 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_node = of_parse_phandle(dn, "phy-handle", 0);
-	if (!phy_node)
-		phy_node = of_parse_phandle(dn, "phy", 0);
-	if (!phy_node)
-		phy_node = of_parse_phandle(dn, "phy-device", 0);
-
-	if (!phy_node) {
-		if (pl->cfg_link_an_mode == MLO_AN_PHY)
-			return -ENODEV;
-		return 0;
-	}
-
-	phy_dev = of_phy_find_device(phy_node);
-	/* We're done with the phy_node handle */
-	of_node_put(phy_node);
-	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;
+	return phylink_fwnode_phy_connect(pl, of_fwnode_handle(dn), flags);
 }
 EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
 
-- 
2.17.1

Re: [net-next PATCH v3 14/15] net: phylink: Refactor phylink_of_phy_connect()

[Andy Shevchenko]

On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Refactor phylink_of_phy_connect() to use phylink_fwnode_phy_connect().

Same Q as per previous patch. If it's indeed a bug in the existing
code, should be fixed in a separate patch

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 14/15] net: phylink: Refactor phylink_of_phy_connect()

[Calvin Johnson]

On Tue, Jan 12, 2021 at 05:57:01PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Refactor phylink_of_phy_connect() to use phylink_fwnode_phy_connect().
> 
> Same Q as per previous patch. If it's indeed a bug in the existing
> code, should be fixed in a separate patch

Sorry, I didn't get what you meant. This patch just refactors
phylink_of_phy_connect(). There is no bug fixing.

Regards
Calvin

[net-next PATCH v3 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Calvin Johnson]

Modify dpaa2_mac_connect() to support ACPI along with DT.
Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
DT or ACPI.

Replace of_get_phy_mode with fwnode_get_phy_mode to get
phy-mode for a dpmac_node.

Use helper function phylink_fwnode_phy_connect() to find phy_dev and
connect to mac->phylink.

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

Changes in v3: None
Changes in v2:
- Refactor OF functions to use fwnode functions

 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 87 +++++++++++--------
 1 file changed, 50 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 69ad869446cf..2aa320fd84ce 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
 /* Copyright 2019 NXP */
 
+#include <linux/acpi.h>
+#include <linux/property.h>
+
 #include "dpaa2-eth.h"
 #include "dpaa2-mac.h"
 
@@ -34,39 +37,47 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
 	return 0;
 }
 
-/* Caller must call of_node_put on the returned value */
-static struct device_node *dpaa2_mac_get_node(u16 dpmac_id)
+static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
+						u16 dpmac_id)
 {
-	struct device_node *dpmacs, *dpmac = NULL;
-	u32 id;
+	struct device_node *dpmacs = NULL;
+	struct fwnode_handle *parent, *child  = NULL;
 	int err;
+	u32 id;
 
-	dpmacs = of_find_node_by_name(NULL, "dpmacs");
-	if (!dpmacs)
-		return NULL;
+	if (is_of_node(dev->parent->fwnode)) {
+		dpmacs = of_find_node_by_name(NULL, "dpmacs");
+		if (!dpmacs)
+			return NULL;
+		parent = of_fwnode_handle(dpmacs);
+	} else if (is_acpi_node(dev->parent->fwnode)) {
+		parent = dev->parent->fwnode;
+	}
 
-	while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
-		err = of_property_read_u32(dpmac, "reg", &id);
-		if (err)
+	fwnode_for_each_child_node(parent, child) {
+		err = fwnode_get_id(child, &id);
+		if (err) {
 			continue;
-		if (id == dpmac_id)
-			break;
+		} else if (id == dpmac_id) {
+			if (is_of_node(dev->parent->fwnode))
+				of_node_put(dpmacs);
+			return child;
+		}
 	}
-
-	of_node_put(dpmacs);
-
-	return dpmac;
+	if (is_of_node(dev->parent->fwnode))
+		of_node_put(dpmacs);
+	return NULL;
 }
 
-static int dpaa2_mac_get_if_mode(struct device_node *node,
+static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node,
 				 struct dpmac_attr attr)
 {
 	phy_interface_t if_mode;
 	int err;
 
-	err = of_get_phy_mode(node, &if_mode);
-	if (!err)
-		return if_mode;
+	err = fwnode_get_phy_mode(dpmac_node);
+	if (err > 0)
+		return err;
 
 	err = phy_mode(attr.eth_if, &if_mode);
 	if (!err)
@@ -221,26 +232,27 @@ static const struct phylink_mac_ops dpaa2_mac_phylink_ops = {
 };
 
 static int dpaa2_pcs_create(struct dpaa2_mac *mac,
-			    struct device_node *dpmac_node, int id)
+			    struct fwnode_handle *dpmac_node,
+			    int id)
 {
 	struct mdio_device *mdiodev;
-	struct device_node *node;
+	struct fwnode_handle *node;
 
-	node = of_parse_phandle(dpmac_node, "pcs-handle", 0);
-	if (!node) {
+	node = fwnode_find_reference(dpmac_node, "pcs-handle", 0);
+	if (IS_ERR(node)) {
 		/* do not error out on old DTS files */
 		netdev_warn(mac->net_dev, "pcs-handle node not found\n");
 		return 0;
 	}
 
-	if (!of_device_is_available(node)) {
+	if (!of_device_is_available(to_of_node(node))) {
 		netdev_err(mac->net_dev, "pcs-handle node not available\n");
-		of_node_put(node);
+		of_node_put(to_of_node(node));
 		return -ENODEV;
 	}
 
-	mdiodev = of_mdio_find_device(node);
-	of_node_put(node);
+	mdiodev = fwnode_mdio_find_device(node);
+	fwnode_handle_put(node);
 	if (!mdiodev)
 		return -EPROBE_DEFER;
 
@@ -269,13 +281,12 @@ static void dpaa2_pcs_destroy(struct dpaa2_mac *mac)
 int dpaa2_mac_connect(struct dpaa2_mac *mac)
 {
 	struct net_device *net_dev = mac->net_dev;
-	struct device_node *dpmac_node;
+	struct fwnode_handle *dpmac_node = NULL;
 	struct phylink *phylink;
 	int err;
 
 	mac->if_link_type = mac->attr.link_type;
-
-	dpmac_node = dpaa2_mac_get_node(mac->attr.id);
+	dpmac_node = dpaa2_mac_get_node(&mac->mc_dev->dev, mac->attr.id);
 	if (!dpmac_node) {
 		netdev_err(net_dev, "No dpmac@%d node found.\n", mac->attr.id);
 		return -ENODEV;
@@ -292,7 +303,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	 * error out if the interface mode requests them and there is no PHY
 	 * to act upon them
 	 */
-	if (of_phy_is_fixed_link(dpmac_node) &&
+	if (of_phy_is_fixed_link(to_of_node(dpmac_node)) &&
 	    (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID ||
 	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
 	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) {
@@ -312,7 +323,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	mac->phylink_config.type = PHYLINK_NETDEV;
 
 	phylink = phylink_create(&mac->phylink_config,
-				 of_fwnode_handle(dpmac_node), mac->if_mode,
+				 dpmac_node, mac->if_mode,
 				 &dpaa2_mac_phylink_ops);
 	if (IS_ERR(phylink)) {
 		err = PTR_ERR(phylink);
@@ -323,13 +334,14 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	if (mac->pcs)
 		phylink_set_pcs(mac->phylink, &mac->pcs->pcs);
 
-	err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0);
+	err = phylink_fwnode_phy_connect(mac->phylink, dpmac_node, 0);
 	if (err) {
-		netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err);
+		netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err);
 		goto err_phylink_destroy;
 	}
 
-	of_node_put(dpmac_node);
+	if (is_of_node(dpmac_node))
+		fwnode_handle_put(dpmac_node);
 
 	return 0;
 
@@ -338,7 +350,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 err_pcs_destroy:
 	dpaa2_pcs_destroy(mac);
 err_put_node:
-	of_node_put(dpmac_node);
+	if (is_of_node(dpmac_node))
+		fwnode_handle_put(dpmac_node);
 
 	return err;
 }
-- 
2.17.1