| 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 >
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
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 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
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
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(-)