Hello,
On Tue, Oct 27, 2015 at 09:08:27AM -0600, Stephen Warren wrote:
> On 10/27/2015 01:41 AM, Uwe Kleine-König wrote:
> >Hello,
> >
> >On Mon, Oct 26, 2015 at 08:08:58PM -0600, Stephen Warren wrote:
> >>On 10/26/2015 03:28 AM, Alexander Aring wrote:
> >>>This patch changes devm_phy_get to devm_phy_optional_get. Which fixes the
> >>>error handling when trying to get the phy. Currently on all errors we
> >>>try to look for an old style USB PHY. With this patch we try to look for
> >>>an old USB PHY when devm_phy_get returns -ENOENT only. Other values like
> >>>-EPROBE_DEFER need to be returned from the probe function, because it
> >>>could be that a phy is specified but not probed before.
> >>
> >>>diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> >>
> >>>@@ -222,12 +222,15 @@ static int dwc2_driver_probe(struct platform_device *dev)
> >>>
> >>> hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);
> >>>
> >>>+ phy = devm_phy_optional_get(&dev->dev, "usb2-phy");
> >>>+ if (IS_ERR(phy))
> >>>+ return PTR_ERR(phy);
> >>
> >>Here, phy is checked using IS_ERR(), ...
> >
> >IS_ERR(phy) is true, if something went wrong getting a phy (probably
> >from dt).
> >
> >>> /*
> >>> * Attempt to find a generic PHY, then look for an old style
> >>> * USB PHY
> >>> */
> >>>- phy = devm_phy_get(&dev->dev, "usb2-phy");
> >>>- if (IS_ERR(phy)) {
> >>>+ if (!phy) {
> >>
> >>... and here the same phy value is checked against NULL. Hopefully,
> >
> >and phy is NULL if dt didn't specify a phy. That's the semantic of
> >devm_phy_optional_get (which is identically to what
> >devm_gpiod_get_optional) does.
> >
> >>devm_phy_get() doesn't actually return either an ERR value or a NULL
> >>pointer as errors, but restricts itself to either an ERR value or
> >
> >The relevant part is that for a function with "optinal" in it's name not
> >finding the respective resource isn't an error. So checking for NULL
> >isn't error handling but handling the 2nd valid case.
>
> If devm_phy_optional_get() uses IS_ERR to check for errors, then ANY other
> (non-IS_ERR) value is an opaque handle to a PHY. The values should be
> meaningless outside of the PHY subsystem. Any other set of return values is
> broken.
>
> The whole point about optional APIs is that they return one of:
>
> a) An error value that can be checked for. Any error is a hard error.
>
> b) A valid PHY value that can be passed to all other PHY-related API.
>
> (b) can be further divided into two cases:
>
> b1) An actual PHY handle if the PHY exists
>
> b2) A dummy PHY value that the PHY APIs internally map to no-ops.
>
> However, the distinction between (b1) and (b2) is something that clients
> should not know about, and hence assuming "NULL" is that dummy value is
> wrong.
So for the case where there is an "optional" phy handle in the dt (in
the sense that the driver can handle both the presence and the absence
of such a handle) what do you suggest? If not using *_optional the only
alternative is to use plain devm_phy_get and ignore -ESOMETHING
(whatever signals the absence of the handle). Not pretty. I like using
devm_phy_optional_get better here.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
