Hello Stephen,
On Wed, Oct 28, 2015 at 09:55:05PM -0600, Stephen Warren wrote:
> On 10/27/2015 12:18 PM, Uwe Kleine-König wrote:
> > 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.
>
> Then it's not actually optional. For the optional case, the driver
> doesn't care whether it's there, but rather simply wants to blindly call
> the phy APIs irrespective. The only error condition is if some problem
> occurs actually looking up the PHY.
>
> In this case, the driver cares whether the PHY exists, so it can take
> alternate action if it does not. The code should do something like:
>
> phy = devm_phy_get(); // not optional
> if (error) {
> phy = some_compatibility_api();
> if (error)
> return error;
> }
>
> (ignoring things like deferred probe, and real function names)
(and ignoring that the 2nd if (error) evaluates to true always)
I think it's not that easy. Assuming the source devm_phy_get uses is a
device tree: If the dt specifies a phy, but devm_phy_get failes because
of a memory allocation the compat path shouldn't be taken either. In
fact you want to only ignore the error value that is returned if there
is no phy specified in the dt.
For gpios gpiod_get_optional is supposed to help you here. If for phys
that's different (even though devm_phy_optional_get's implementation is
implemented in an analogous way) that's IMHO unlucky.
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
