| Message ID | cd685d8e4d6754c384acfc1796065d539a2c3ea8.1674036164.git.geert+renesas@glider.be |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: Add devm_of_phy_optional_get() helper | expand |
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 >
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
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
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
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
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)
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(-)