[PATCH treewide 0/7] phy: Add devm_of_phy_optional_get() helper

[PATCH treewide 0/7] phy: Add devm_of_phy_optional_get() helper

[Geert Uytterhoeven]

	Hi all,

While there exist several optional_get() PHY helper functions, there is
no optional variant of devm_of_phy_get(), leading to several drivers
implementing this theirselves, sometimes in buggy ways.

Hence this series introduces a devm_of_phy_optional_get() helper(), and
converts existing users of devm_of_phy_get() where appropriate.

This series been compile-tested only, but the new helper itself has been
tested with a new user I am about to submit.

Thanks for your comments!

Geert Uytterhoeven (7):
  phy: Add devm_of_phy_optional_get() helper
  net: fman: memac: Convert to devm_of_phy_optional_get()
  net: lan966x: Convert to devm_of_phy_optional_get()
  net: ethernet: ti: am65-cpsw: Convert to devm_of_phy_optional_get()
  PCI: tegra: Convert to devm_of_phy_optional_get()
  usb: host: ehci-exynos: Convert to devm_of_phy_optional_get()
  usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

 .../net/ethernet/freescale/fman/fman_memac.c  |  8 +++---
 .../ethernet/microchip/lan966x/lan966x_main.c |  5 ++--
 drivers/net/ethernet/ti/am65-cpsw-nuss.c      |  6 ++---
 drivers/pci/controller/pci-tegra.c            |  5 +---
 drivers/phy/phy-core.c                        | 26 +++++++++++++++++++
 drivers/usb/host/ehci-exynos.c                | 24 +++++------------
 drivers/usb/host/ohci-exynos.c                | 24 +++++------------
 include/linux/phy/phy.h                       |  9 ++++++
 8 files changed, 59 insertions(+), 48 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/7] phy: Add devm_of_phy_optional_get() helper

[Geert Uytterhoeven]

Add an optional variant of devm_of_phy_get(), so drivers no longer have
to open-code this operation.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/phy/phy-core.c  | 26 ++++++++++++++++++++++++++
 include/linux/phy/phy.h |  9 ++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index d93ddf1262c5178b..ea009a611e19c705 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -879,6 +879,32 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(devm_of_phy_get);
 
+/**
+ * devm_of_phy_optional_get() - lookup and obtain a reference to an optional
+ * phy.
+ * @dev: device that requests this phy
+ * @np: node containing the phy
+ * @con_id: name of the phy from device's point of view
+ *
+ * Gets the phy using of_phy_get(), and associates a device with it using
+ * devres. On driver detach, release function is invoked on the devres data,
+ * then, devres data is freed.  This differs to devm_of_phy_get() in
+ * that if the phy does not exist, it is not considered an error and
+ * -ENODEV will not be returned. Instead the NULL phy is returned,
+ * which can be passed to all other phy consumer calls.
+ */
+struct phy *devm_of_phy_optional_get(struct device *dev, struct device_node *np,
+				     const char *con_id)
+{
+	struct phy *phy = devm_of_phy_get(dev, np, con_id);
+
+	if (PTR_ERR(phy) == -ENODEV)
+		phy = NULL;
+
+	return phy;
+}
+EXPORT_SYMBOL_GPL(devm_of_phy_optional_get);
+
 /**
  * devm_of_phy_get_by_index() - lookup and obtain a reference to a phy by index.
  * @dev: device that requests this phy
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 559c3da515073697..5f6e669b616da0b0 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -255,6 +255,8 @@ struct phy *devm_phy_get(struct device *dev, const char *string);
 struct phy *devm_phy_optional_get(struct device *dev, const char *string);
 struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
 			    const char *con_id);
+struct phy *devm_of_phy_optional_get(struct device *dev, struct device_node *np,
+				     const char *con_id);
 struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
 				     int index);
 void of_phy_put(struct phy *phy);
@@ -450,6 +452,13 @@ static inline struct phy *devm_of_phy_get(struct device *dev,
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct phy *devm_of_phy_optional_get(struct device *dev,
+						   struct device_node *np,
+						   const char *con_id)
+{
+	return NULL;
+}
+
 static inline struct phy *devm_of_phy_get_by_index(struct device *dev,
 						   struct device_node *np,
 						   int index)
-- 
2.34.1

[PATCH 2/7] net: fman: memac: Convert to devm_of_phy_optional_get()

[Geert Uytterhoeven]

Use the new devm_of_phy_optional_get() helper instead of open-coding the
same operation.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/ethernet/freescale/fman/fman_memac.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/net/ethernet/freescale/fman/fman_memac.c
index 9349f841bd0645a0..892277f13048660d 100644
--- a/drivers/net/ethernet/freescale/fman/fman_memac.c
+++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
@@ -1152,12 +1152,12 @@ int memac_initialization(struct mac_device *mac_dev,
 	else
 		memac->sgmii_pcs = pcs;
 
-	memac->serdes = devm_of_phy_get(mac_dev->dev, mac_node, "serdes");
-	err = PTR_ERR(memac->serdes);
-	if (err == -ENODEV || err == -ENOSYS) {
+	memac->serdes = devm_of_phy_optional_get(mac_dev->dev, mac_node,
+						 "serdes");
+	if (!memac->serdes) {
 		dev_dbg(mac_dev->dev, "could not get (optional) serdes\n");
-		memac->serdes = NULL;
 	} else if (IS_ERR(memac->serdes)) {
+		err = PTR_ERR(memac->serdes);
 		dev_err_probe(mac_dev->dev, err, "could not get serdes\n");
 		goto _return_fm_mac_free;
 	}
-- 
2.34.1

[PATCH 3/7] net: lan966x: Convert to devm_of_phy_optional_get()

[Geert Uytterhoeven]

Use the new devm_of_phy_optional_get() helper instead of open-coding the
same operation.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index cadde20505ba0689..d64a525cdc9ea18b 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -1147,9 +1147,8 @@ static int lan966x_probe(struct platform_device *pdev)
 		lan966x->ports[p]->config.portmode = phy_mode;
 		lan966x->ports[p]->fwnode = fwnode_handle_get(portnp);
 
-		serdes = devm_of_phy_get(lan966x->dev, to_of_node(portnp), NULL);
-		if (PTR_ERR(serdes) == -ENODEV)
-			serdes = NULL;
+		serdes = devm_of_phy_optional_get(lan966x->dev,
+						  to_of_node(portnp), NULL);
 		if (IS_ERR(serdes)) {
 			err = PTR_ERR(serdes);
 			goto cleanup_ports;
-- 
2.34.1

[PATCH 4/7] net: ethernet: ti: am65-cpsw: Convert to devm_of_phy_optional_get()

[Geert Uytterhoeven]

Use the new devm_of_phy_optional_get() helper instead of open-coding the
same operation.

While at it, fix handling of other error codes (e.g. -EPROBE_DEFER).

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 5cac982841845369..794f228c8d632f7a 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -1460,9 +1460,9 @@ static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *por
 	struct phy *phy;
 	int ret;
 
-	phy = devm_of_phy_get(dev, port_np, name);
-	if (PTR_ERR(phy) == -ENODEV)
-		return 0;
+	phy = devm_of_phy_optional_get(dev, port_np, name);
+	if (IS_ERR_OR_NULL(phy))
+		return PTR_ERR_OR_ZERO(phy);
 
 	/* Serdes PHY exists. Store it. */
 	port->slave.serdes_phy = phy;
-- 
2.34.1

[PATCH 5/7] PCI: tegra: Convert to devm_of_phy_optional_get()

[Geert Uytterhoeven]

Use the new devm_of_phy_optional_get() helper instead of open-coding the
same operation.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pci/controller/pci-tegra.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 929f9363e94bec71..5b8907c663e516ad 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -1330,12 +1330,9 @@ static struct phy *devm_of_phy_optional_get_index(struct device *dev,
 	if (!name)
 		return ERR_PTR(-ENOMEM);
 
-	phy = devm_of_phy_get(dev, np, name);
+	phy = devm_of_phy_optional_get(dev, np, name);
 	kfree(name);
 
-	if (PTR_ERR(phy) == -ENODEV)
-		phy = NULL;
-
 	return phy;
 }
 
-- 
2.34.1

[PATCH 6/7] usb: host: ehci-exynos: Convert to devm_of_phy_optional_get()

[Geert Uytterhoeven]

Use the new devm_of_phy_optional_get() helper instead of open-coding the
same operation.

This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
handle NULL parameters fine.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/usb/host/ehci-exynos.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index a333231616f437b8..0f8d7df4937b2da4 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -80,19 +80,12 @@ static int exynos_ehci_get_phy(struct device *dev,
 			return -EINVAL;
 		}
 
-		phy = devm_of_phy_get(dev, child, NULL);
+		phy = devm_of_phy_optional_get(dev, child, NULL);
 		exynos_ehci->phy[phy_number] = phy;
 		if (IS_ERR(phy)) {
-			ret = PTR_ERR(phy);
-			if (ret == -EPROBE_DEFER) {
-				of_node_put(child);
-				return ret;
-			} else if (ret != -ENOSYS && ret != -ENODEV) {
-				dev_err(dev,
-					"Error retrieving usb2 phy: %d\n", ret);
-				of_node_put(child);
-				return ret;
-			}
+			of_node_put(child);
+			return dev_err_probe(dev, PTR_ERR(phy),
+					     "Error retrieving usb2 phy\n");
 		}
 	}
 
@@ -108,12 +101,10 @@ static int exynos_ehci_phy_enable(struct device *dev)
 	int ret = 0;
 
 	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
-		if (!IS_ERR(exynos_ehci->phy[i]))
-			ret = phy_power_on(exynos_ehci->phy[i]);
+		ret = phy_power_on(exynos_ehci->phy[i]);
 	if (ret)
 		for (i--; i >= 0; i--)
-			if (!IS_ERR(exynos_ehci->phy[i]))
-				phy_power_off(exynos_ehci->phy[i]);
+			phy_power_off(exynos_ehci->phy[i]);
 
 	return ret;
 }
@@ -125,8 +116,7 @@ static void exynos_ehci_phy_disable(struct device *dev)
 	int i;
 
 	for (i = 0; i < PHY_NUMBER; i++)
-		if (!IS_ERR(exynos_ehci->phy[i]))
-			phy_power_off(exynos_ehci->phy[i]);
+		phy_power_off(exynos_ehci->phy[i]);
 }
 
 static void exynos_setup_vbus_gpio(struct device *dev)
-- 
2.34.1

[PATCH 7/7] usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

[Geert Uytterhoeven]

Use the new devm_of_phy_optional_get() helper instead of open-coding the
same operation.

This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
handle NULL parameters fine.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/usb/host/ohci-exynos.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 8d7977fd5d3bd502..8dd9c3b2411c383f 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -69,19 +69,12 @@ static int exynos_ohci_get_phy(struct device *dev,
 			return -EINVAL;
 		}
 
-		phy = devm_of_phy_get(dev, child, NULL);
+		phy = devm_of_phy_optional_get(dev, child, NULL);
 		exynos_ohci->phy[phy_number] = phy;
 		if (IS_ERR(phy)) {
-			ret = PTR_ERR(phy);
-			if (ret == -EPROBE_DEFER) {
-				of_node_put(child);
-				return ret;
-			} else if (ret != -ENOSYS && ret != -ENODEV) {
-				dev_err(dev,
-					"Error retrieving usb2 phy: %d\n", ret);
-				of_node_put(child);
-				return ret;
-			}
+			of_node_put(child);
+			return dev_err_probe(dev, PTR_ERR(phy),
+					     "Error retrieving usb2 phy\n");
 		}
 	}
 
@@ -97,12 +90,10 @@ static int exynos_ohci_phy_enable(struct device *dev)
 	int ret = 0;
 
 	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
-		if (!IS_ERR(exynos_ohci->phy[i]))
-			ret = phy_power_on(exynos_ohci->phy[i]);
+		ret = phy_power_on(exynos_ohci->phy[i]);
 	if (ret)
 		for (i--; i >= 0; i--)
-			if (!IS_ERR(exynos_ohci->phy[i]))
-				phy_power_off(exynos_ohci->phy[i]);
+			phy_power_off(exynos_ohci->phy[i]);
 
 	return ret;
 }
@@ -114,8 +105,7 @@ static void exynos_ohci_phy_disable(struct device *dev)
 	int i;
 
 	for (i = 0; i < PHY_NUMBER; i++)
-		if (!IS_ERR(exynos_ohci->phy[i]))
-			phy_power_off(exynos_ohci->phy[i]);
+		phy_power_off(exynos_ohci->phy[i]);
 }
 
 static int exynos_ohci_probe(struct platform_device *pdev)
-- 
2.34.1

Re: [PATCH 7/7] usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

[Alan Stern]

On Wed, Jan 18, 2023 at 11:15:20AM +0100, Geert Uytterhoeven wrote:
> Use the new devm_of_phy_optional_get() helper instead of open-coding the
> same operation.
> 
> This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
> handle NULL parameters fine.

The patch ignores a possible -ENOSYS error return.  Is it known that 
this will never happen?

Alan Stern

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/usb/host/ohci-exynos.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> index 8d7977fd5d3bd502..8dd9c3b2411c383f 100644
> --- a/drivers/usb/host/ohci-exynos.c
> +++ b/drivers/usb/host/ohci-exynos.c
> @@ -69,19 +69,12 @@ static int exynos_ohci_get_phy(struct device *dev,
>  			return -EINVAL;
>  		}
>  
> -		phy = devm_of_phy_get(dev, child, NULL);
> +		phy = devm_of_phy_optional_get(dev, child, NULL);
>  		exynos_ohci->phy[phy_number] = phy;
>  		if (IS_ERR(phy)) {
> -			ret = PTR_ERR(phy);
> -			if (ret == -EPROBE_DEFER) {
> -				of_node_put(child);
> -				return ret;
> -			} else if (ret != -ENOSYS && ret != -ENODEV) {
> -				dev_err(dev,
> -					"Error retrieving usb2 phy: %d\n", ret);
> -				of_node_put(child);
> -				return ret;
> -			}
> +			of_node_put(child);
> +			return dev_err_probe(dev, PTR_ERR(phy),
> +					     "Error retrieving usb2 phy\n");
>  		}
>  	}
>  
> @@ -97,12 +90,10 @@ static int exynos_ohci_phy_enable(struct device *dev)
>  	int ret = 0;
>  
>  	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
> -		if (!IS_ERR(exynos_ohci->phy[i]))
> -			ret = phy_power_on(exynos_ohci->phy[i]);
> +		ret = phy_power_on(exynos_ohci->phy[i]);
>  	if (ret)
>  		for (i--; i >= 0; i--)
> -			if (!IS_ERR(exynos_ohci->phy[i]))
> -				phy_power_off(exynos_ohci->phy[i]);
> +			phy_power_off(exynos_ohci->phy[i]);
>  
>  	return ret;
>  }
> @@ -114,8 +105,7 @@ static void exynos_ohci_phy_disable(struct device *dev)
>  	int i;
>  
>  	for (i = 0; i < PHY_NUMBER; i++)
> -		if (!IS_ERR(exynos_ohci->phy[i]))
> -			phy_power_off(exynos_ohci->phy[i]);
> +		phy_power_off(exynos_ohci->phy[i]);
>  }
>  
>  static int exynos_ohci_probe(struct platform_device *pdev)
> -- 
> 2.34.1
> 

Re: [PATCH 7/7] usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

[Geert Uytterhoeven]

Hi Alan,

On Wed, Jan 18, 2023 at 5:26 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, Jan 18, 2023 at 11:15:20AM +0100, Geert Uytterhoeven wrote:
> > Use the new devm_of_phy_optional_get() helper instead of open-coding the
> > same operation.
> >
> > This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
> > handle NULL parameters fine.
>
> The patch ignores a possible -ENOSYS error return.  Is it known that
> this will never happen?

While some phy_*() dummies in include/linux/phy/phy.h do return -ENOSYS
if CONFIG_GENERIC_PHY is not enabled, devm_of_phy_optional_get()
returns NULL in that case, so I think this cannot happen.

> > --- a/drivers/usb/host/ohci-exynos.c
> > +++ b/drivers/usb/host/ohci-exynos.c
> > @@ -69,19 +69,12 @@ static int exynos_ohci_get_phy(struct device *dev,
> >                       return -EINVAL;
> >               }
> >
> > -             phy = devm_of_phy_get(dev, child, NULL);
> > +             phy = devm_of_phy_optional_get(dev, child, NULL);
> >               exynos_ohci->phy[phy_number] = phy;
> >               if (IS_ERR(phy)) {
> > -                     ret = PTR_ERR(phy);
> > -                     if (ret == -EPROBE_DEFER) {
> > -                             of_node_put(child);
> > -                             return ret;
> > -                     } else if (ret != -ENOSYS && ret != -ENODEV) {
> > -                             dev_err(dev,
> > -                                     "Error retrieving usb2 phy: %d\n", ret);
> > -                             of_node_put(child);
> > -                             return ret;
> > -                     }
> > +                     of_node_put(child);
> > +                     return dev_err_probe(dev, PTR_ERR(phy),
> > +                                          "Error retrieving usb2 phy\n");
> >               }
> >       }
> >

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 7/7] usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

[Alan Stern]

On Wed, Jan 18, 2023 at 05:50:00PM +0100, Geert Uytterhoeven wrote:
> Hi Alan,
> 
> On Wed, Jan 18, 2023 at 5:26 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, Jan 18, 2023 at 11:15:20AM +0100, Geert Uytterhoeven wrote:
> > > Use the new devm_of_phy_optional_get() helper instead of open-coding the
> > > same operation.
> > >
> > > This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
> > > handle NULL parameters fine.
> >
> > The patch ignores a possible -ENOSYS error return.  Is it known that
> > this will never happen?
> 
> While some phy_*() dummies in include/linux/phy/phy.h do return -ENOSYS
> if CONFIG_GENERIC_PHY is not enabled, devm_of_phy_optional_get()
> returns NULL in that case, so I think this cannot happen.

You could mention that in the patch description.  Apart from this one 
issue:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [PATCH 7/7] usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

[Rob Herring]

On Wed, Jan 18, 2023 at 4:15 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> Use the new devm_of_phy_optional_get() helper instead of open-coding the
> same operation.
>
> This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
> handle NULL parameters fine.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/usb/host/ohci-exynos.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> index 8d7977fd5d3bd502..8dd9c3b2411c383f 100644
> --- a/drivers/usb/host/ohci-exynos.c
> +++ b/drivers/usb/host/ohci-exynos.c
> @@ -69,19 +69,12 @@ static int exynos_ohci_get_phy(struct device *dev,
>                         return -EINVAL;
>                 }
>
> -               phy = devm_of_phy_get(dev, child, NULL);
> +               phy = devm_of_phy_optional_get(dev, child, NULL);
>                 exynos_ohci->phy[phy_number] = phy;
>                 if (IS_ERR(phy)) {
> -                       ret = PTR_ERR(phy);
> -                       if (ret == -EPROBE_DEFER) {
> -                               of_node_put(child);
> -                               return ret;
> -                       } else if (ret != -ENOSYS && ret != -ENODEV) {
> -                               dev_err(dev,
> -                                       "Error retrieving usb2 phy: %d\n", ret);
> -                               of_node_put(child);
> -                               return ret;
> -                       }
> +                       of_node_put(child);
> +                       return dev_err_probe(dev, PTR_ERR(phy),
> +                                            "Error retrieving usb2 phy\n");

Optional is really the only reason for the caller to decide whether to
print an error message or not. If we have both flavors of 'get', then
really the 'get' functions should print an error message.

Rob

Re: [PATCH 7/7] usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

[Geert Uytterhoeven]

Hi Rob,

On Wed, Jan 18, 2023 at 6:30 PM Rob Herring <robh@kernel.org> wrote:
> On Wed, Jan 18, 2023 at 4:15 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > Use the new devm_of_phy_optional_get() helper instead of open-coding the
> > same operation.
> >
> > This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
> > handle NULL parameters fine.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  drivers/usb/host/ohci-exynos.c | 24 +++++++-----------------
> >  1 file changed, 7 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> > index 8d7977fd5d3bd502..8dd9c3b2411c383f 100644
> > --- a/drivers/usb/host/ohci-exynos.c
> > +++ b/drivers/usb/host/ohci-exynos.c
> > @@ -69,19 +69,12 @@ static int exynos_ohci_get_phy(struct device *dev,
> >                         return -EINVAL;
> >                 }
> >
> > -               phy = devm_of_phy_get(dev, child, NULL);
> > +               phy = devm_of_phy_optional_get(dev, child, NULL);
> >                 exynos_ohci->phy[phy_number] = phy;
> >                 if (IS_ERR(phy)) {
> > -                       ret = PTR_ERR(phy);
> > -                       if (ret == -EPROBE_DEFER) {
> > -                               of_node_put(child);
> > -                               return ret;
> > -                       } else if (ret != -ENOSYS && ret != -ENODEV) {
> > -                               dev_err(dev,
> > -                                       "Error retrieving usb2 phy: %d\n", ret);
> > -                               of_node_put(child);
> > -                               return ret;
> > -                       }
> > +                       of_node_put(child);
> > +                       return dev_err_probe(dev, PTR_ERR(phy),
> > +                                            "Error retrieving usb2 phy\n");
>
> Optional is really the only reason for the caller to decide whether to
> print an error message or not. If we have both flavors of 'get', then
> really the 'get' functions should print an error message.

In case of a real error, both should print an error message, right?

Anyway, I understand that's a three step operation:
  1. Introduce and convert to the _optional variant,
  2. Add error printing to callees.
  3. Remove error printing from callers.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 7/7] usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

[Rob Herring]

On Wed, Jan 18, 2023 at 12:28 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Wed, Jan 18, 2023 at 6:30 PM Rob Herring <robh@kernel.org> wrote:
> > On Wed, Jan 18, 2023 at 4:15 AM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> > > Use the new devm_of_phy_optional_get() helper instead of open-coding the
> > > same operation.
> > >
> > > This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
> > > handle NULL parameters fine.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > >  drivers/usb/host/ohci-exynos.c | 24 +++++++-----------------
> > >  1 file changed, 7 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> > > index 8d7977fd5d3bd502..8dd9c3b2411c383f 100644
> > > --- a/drivers/usb/host/ohci-exynos.c
> > > +++ b/drivers/usb/host/ohci-exynos.c
> > > @@ -69,19 +69,12 @@ static int exynos_ohci_get_phy(struct device *dev,
> > >                         return -EINVAL;
> > >                 }
> > >
> > > -               phy = devm_of_phy_get(dev, child, NULL);
> > > +               phy = devm_of_phy_optional_get(dev, child, NULL);
> > >                 exynos_ohci->phy[phy_number] = phy;
> > >                 if (IS_ERR(phy)) {
> > > -                       ret = PTR_ERR(phy);
> > > -                       if (ret == -EPROBE_DEFER) {
> > > -                               of_node_put(child);
> > > -                               return ret;
> > > -                       } else if (ret != -ENOSYS && ret != -ENODEV) {
> > > -                               dev_err(dev,
> > > -                                       "Error retrieving usb2 phy: %d\n", ret);
> > > -                               of_node_put(child);
> > > -                               return ret;
> > > -                       }
> > > +                       of_node_put(child);
> > > +                       return dev_err_probe(dev, PTR_ERR(phy),
> > > +                                            "Error retrieving usb2 phy\n");
> >
> > Optional is really the only reason for the caller to decide whether to
> > print an error message or not. If we have both flavors of 'get', then
> > really the 'get' functions should print an error message.
>
> In case of a real error, both should print an error message, right?
>
> Anyway, I understand that's a three step operation:
>   1. Introduce and convert to the _optional variant,
>   2. Add error printing to callees.
>   3. Remove error printing from callers.

I think you only need 2 out of 3 steps depending on the situation. In
this case, you can add error printing in the _optional variant when
you introduce it and then convert callers to it.

Where we already have an optional variant, then you need steps 2 and 3.

Rob

Re: [PATCH 1/7] phy: Add devm_of_phy_optional_get() helper

[Jakub Kicinski]

On Wed, 18 Jan 2023 11:15:14 +0100 Geert Uytterhoeven wrote:
> Add an optional variant of devm_of_phy_get(), so drivers no longer have
> to open-code this operation.

For merging could you put this one on an immutable branch and then
everyone can pull + apply patches for callers from their section?

Re: [PATCH 1/7] phy: Add devm_of_phy_optional_get() helper

[Vinod Koul]

On 18-01-23, 19:28, Jakub Kicinski wrote:
> On Wed, 18 Jan 2023 11:15:14 +0100 Geert Uytterhoeven wrote:
> > Add an optional variant of devm_of_phy_get(), so drivers no longer have
> > to open-code this operation.
> 
> For merging could you put this one on an immutable branch and then
> everyone can pull + apply patches for callers from their section?

Since this is phy, I can do that and everyone else can merge that in or
all changes can go thru phy tree

Thanks
-- 
~Vinod

Re: [PATCH 6/7] usb: host: ehci-exynos: Convert to devm_of_phy_optional_get()

[Greg Kroah-Hartman]

On Wed, Jan 18, 2023 at 11:15:19AM +0100, Geert Uytterhoeven wrote:
> Use the new devm_of_phy_optional_get() helper instead of open-coding the
> same operation.
> 
> This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
> handle NULL parameters fine.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/usb/host/ehci-exynos.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/7] phy: Add devm_of_phy_optional_get() helper

[Jakub Kicinski]

On Thu, 19 Jan 2023 16:44:35 +0530 Vinod Koul wrote:
> > For merging could you put this one on an immutable branch and then
> > everyone can pull + apply patches for callers from their section?  
> 
> Since this is phy, I can do that and everyone else can merge that in or
> all changes can go thru phy tree

Great! The microchip and ti drivers are relatively actively developed, 
I reckon it's worth going the branch way.

Re: [PATCH 2/7] net: fman: memac: Convert to devm_of_phy_optional_get()

[Sean Anderson]

On 1/18/23 05:15, Geert Uytterhoeven wrote:
> Use the new devm_of_phy_optional_get() helper instead of open-coding the
> same operation.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/net/ethernet/freescale/fman/fman_memac.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/net/ethernet/freescale/fman/fman_memac.c
> index 9349f841bd0645a0..892277f13048660d 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_memac.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
> @@ -1152,12 +1152,12 @@ int memac_initialization(struct mac_device *mac_dev,
>  	else
>  		memac->sgmii_pcs = pcs;
>  
> -	memac->serdes = devm_of_phy_get(mac_dev->dev, mac_node, "serdes");
> -	err = PTR_ERR(memac->serdes);
> -	if (err == -ENODEV || err == -ENOSYS) {
> +	memac->serdes = devm_of_phy_optional_get(mac_dev->dev, mac_node,
> +						 "serdes");
> +	if (!memac->serdes) {
>  		dev_dbg(mac_dev->dev, "could not get (optional) serdes\n");
> -		memac->serdes = NULL;
>  	} else if (IS_ERR(memac->serdes)) {
> +		err = PTR_ERR(memac->serdes);
>  		dev_err_probe(mac_dev->dev, err, "could not get serdes\n");
>  		goto _return_fm_mac_free;
>  	}

Reviewed-by: Sean Anderson <sean.anderson@seco.com>

Re: [PATCH 7/7] usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

[Geert Uytterhoeven]

Hi Rob,

On Wed, Jan 18, 2023 at 8:49 PM Rob Herring <robh@kernel.org> wrote:
> On Wed, Jan 18, 2023 at 12:28 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Wed, Jan 18, 2023 at 6:30 PM Rob Herring <robh@kernel.org> wrote:
> > > On Wed, Jan 18, 2023 at 4:15 AM Geert Uytterhoeven
> > > <geert+renesas@glider.be> wrote:
> > > > Use the new devm_of_phy_optional_get() helper instead of open-coding the
> > > > same operation.
> > > >
> > > > This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
> > > > handle NULL parameters fine.
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > ---
> > > >  drivers/usb/host/ohci-exynos.c | 24 +++++++-----------------
> > > >  1 file changed, 7 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> > > > index 8d7977fd5d3bd502..8dd9c3b2411c383f 100644
> > > > --- a/drivers/usb/host/ohci-exynos.c
> > > > +++ b/drivers/usb/host/ohci-exynos.c
> > > > @@ -69,19 +69,12 @@ static int exynos_ohci_get_phy(struct device *dev,
> > > >                         return -EINVAL;
> > > >                 }
> > > >
> > > > -               phy = devm_of_phy_get(dev, child, NULL);
> > > > +               phy = devm_of_phy_optional_get(dev, child, NULL);
> > > >                 exynos_ohci->phy[phy_number] = phy;
> > > >                 if (IS_ERR(phy)) {
> > > > -                       ret = PTR_ERR(phy);
> > > > -                       if (ret == -EPROBE_DEFER) {
> > > > -                               of_node_put(child);
> > > > -                               return ret;
> > > > -                       } else if (ret != -ENOSYS && ret != -ENODEV) {
> > > > -                               dev_err(dev,
> > > > -                                       "Error retrieving usb2 phy: %d\n", ret);
> > > > -                               of_node_put(child);
> > > > -                               return ret;
> > > > -                       }
> > > > +                       of_node_put(child);
> > > > +                       return dev_err_probe(dev, PTR_ERR(phy),
> > > > +                                            "Error retrieving usb2 phy\n");
> > >
> > > Optional is really the only reason for the caller to decide whether to
> > > print an error message or not. If we have both flavors of 'get', then
> > > really the 'get' functions should print an error message.
> >
> > In case of a real error, both should print an error message, right?
> >
> > Anyway, I understand that's a three step operation:
> >   1. Introduce and convert to the _optional variant,
> >   2. Add error printing to callees.
> >   3. Remove error printing from callers.
>
> I think you only need 2 out of 3 steps depending on the situation. In
> this case, you can add error printing in the _optional variant when
> you introduce it and then convert callers to it.
>
> Where we already have an optional variant, then you need steps 2 and 3.

Right, so the error printing can be done now, while introducing
devm_of_phy_optional_get().

Vinod: Do you agree?
If yes, I can respin with that change.
If not, I'll have to respin anyway, as the bug in
am65_cpsw_init_serdes_phy() has been fixed in the meantime.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 7/7] usb: host: ohci-exynos: Convert to devm_of_phy_optional_get()

[Vinod Koul]

On 20-01-23, 08:56, Geert Uytterhoeven wrote:
> Hi Rob,
> 
> On Wed, Jan 18, 2023 at 8:49 PM Rob Herring <robh@kernel.org> wrote:
> > On Wed, Jan 18, 2023 at 12:28 PM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Wed, Jan 18, 2023 at 6:30 PM Rob Herring <robh@kernel.org> wrote:
> > > > On Wed, Jan 18, 2023 at 4:15 AM Geert Uytterhoeven
> > > > <geert+renesas@glider.be> wrote:
> > > > > Use the new devm_of_phy_optional_get() helper instead of open-coding the
> > > > > same operation.
> > > > >
> > > > > This lets us drop several checks for IS_ERR(), as phy_power_{on,off}()
> > > > > handle NULL parameters fine.
> > > > >
> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > ---
> > > > >  drivers/usb/host/ohci-exynos.c | 24 +++++++-----------------
> > > > >  1 file changed, 7 insertions(+), 17 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> > > > > index 8d7977fd5d3bd502..8dd9c3b2411c383f 100644
> > > > > --- a/drivers/usb/host/ohci-exynos.c
> > > > > +++ b/drivers/usb/host/ohci-exynos.c
> > > > > @@ -69,19 +69,12 @@ static int exynos_ohci_get_phy(struct device *dev,
> > > > >                         return -EINVAL;
> > > > >                 }
> > > > >
> > > > > -               phy = devm_of_phy_get(dev, child, NULL);
> > > > > +               phy = devm_of_phy_optional_get(dev, child, NULL);
> > > > >                 exynos_ohci->phy[phy_number] = phy;
> > > > >                 if (IS_ERR(phy)) {
> > > > > -                       ret = PTR_ERR(phy);
> > > > > -                       if (ret == -EPROBE_DEFER) {
> > > > > -                               of_node_put(child);
> > > > > -                               return ret;
> > > > > -                       } else if (ret != -ENOSYS && ret != -ENODEV) {
> > > > > -                               dev_err(dev,
> > > > > -                                       "Error retrieving usb2 phy: %d\n", ret);
> > > > > -                               of_node_put(child);
> > > > > -                               return ret;
> > > > > -                       }
> > > > > +                       of_node_put(child);
> > > > > +                       return dev_err_probe(dev, PTR_ERR(phy),
> > > > > +                                            "Error retrieving usb2 phy\n");
> > > >
> > > > Optional is really the only reason for the caller to decide whether to
> > > > print an error message or not. If we have both flavors of 'get', then
> > > > really the 'get' functions should print an error message.
> > >
> > > In case of a real error, both should print an error message, right?
> > >
> > > Anyway, I understand that's a three step operation:
> > >   1. Introduce and convert to the _optional variant,
> > >   2. Add error printing to callees.
> > >   3. Remove error printing from callers.
> >
> > I think you only need 2 out of 3 steps depending on the situation. In
> > this case, you can add error printing in the _optional variant when
> > you introduce it and then convert callers to it.
> >
> > Where we already have an optional variant, then you need steps 2 and 3.
> 
> Right, so the error printing can be done now, while introducing
> devm_of_phy_optional_get().
> 
> Vinod: Do you agree?

Ack.. IMO makes it better that way

> If yes, I can respin with that change.

ok

> If not, I'll have to respin anyway, as the bug in
> am65_cpsw_init_serdes_phy() has been fixed in the meantime.
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
~Vinod

Re: [PATCH 3/7] net: lan966x: Convert to devm_of_phy_optional_get()

[Steen Hegelund]

Hi Geert,

This looks good to me.

BR
Steen

Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>

On Wed, 2023-01-18 at 11:15 +0100, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Use the new devm_of_phy_optional_get() helper instead of open-coding the
> same operation.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index cadde20505ba0689..d64a525cdc9ea18b 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -1147,9 +1147,8 @@ static int lan966x_probe(struct platform_device *pdev)
>                 lan966x->ports[p]->config.portmode = phy_mode;
>                 lan966x->ports[p]->fwnode = fwnode_handle_get(portnp);
> 
> -               serdes = devm_of_phy_get(lan966x->dev, to_of_node(portnp),
> NULL);
> -               if (PTR_ERR(serdes) == -ENODEV)
> -                       serdes = NULL;
> +               serdes = devm_of_phy_optional_get(lan966x->dev,
> +                                                 to_of_node(portnp), NULL);
>                 if (IS_ERR(serdes)) {
>                         err = PTR_ERR(serdes);
>                         goto cleanup_ports;
> --
> 2.34.1
> 

Re: [PATCH 5/7] PCI: tegra: Convert to devm_of_phy_optional_get()

[Bjorn Helgaas]

On Wed, Jan 18, 2023 at 11:15:18AM +0100, Geert Uytterhoeven wrote:
> Use the new devm_of_phy_optional_get() helper instead of open-coding the
> same operation.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Let me know if you want me to apply; otherwise I'll assume you will
merge along with the [1/7] patch.

> ---
>  drivers/pci/controller/pci-tegra.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 929f9363e94bec71..5b8907c663e516ad 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -1330,12 +1330,9 @@ static struct phy *devm_of_phy_optional_get_index(struct device *dev,
>  	if (!name)
>  		return ERR_PTR(-ENOMEM);
>  
> -	phy = devm_of_phy_get(dev, np, name);
> +	phy = devm_of_phy_optional_get(dev, np, name);
>  	kfree(name);
>  
> -	if (PTR_ERR(phy) == -ENODEV)
> -		phy = NULL;
> -
>  	return phy;
>  }
>  
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel