On Fri, 18 Oct 2013, Nikita Kiryanov wrote:
> >> + for (i = 0; i < PXA_UHC_MAX_PORTNUM; i++) {
> >> + retval = 0;
> >> + if (ohci->ext_regulator[i])
> >> + retval = regulator_enable(ohci->ext_regulator[i]);
> >> +
> >> + if (retval)
> >> + pr_err("%s: regulator enable failed: port%d, err=%d\n",
> >> + __func__, i, retval);
> >
> > You could get rid of the "retval = 0;" line if you wrote this as:
> >
> > if (ohci->ext_regulator[i]) {
> > retval = regulator_enable(...);
> > if (retval)
> > dev_err(dev, ...)
> > }
> >
> > Note: dev_err, not pr_err.
>
> Good point.
>
> >
> > What happens to ext_regulator[0] if you fail to enable
> > ext_regulator[1]? Does it remain permanently enabled?
>
> It will be turned off when pxa27x_stop_hc is called.
Ah, I see. You enable regulators only in pxa27x_start_hc() and disable
them only in pxa27x_stop_hc(). The second routine is always called
if the first one succeeds.
Which leads to the question: What happens to usb_regulator if
pxa27x_start_hc() returns early because of a failure in inf->init()?
> >> +static void ohci_regulator_put_all(struct pxa27x_ohci *ohci)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < PXA_UHC_MAX_PORTNUM; i++) {
> >> + if (ohci->ext_regulator[i])
> >> + regulator_put(ohci->ext_regulator[i]);
> >> + }
> >> +
> >> + /* usb regulator should be present if we get here */
> >> + regulator_put(ohci->usb_regulator);
> >> +}
> >
> > What happens if you call regulator_put() for an enabled regulator?
>
> It'll remain enabled, but there's not much that can be done about this:
> The problem with regulators is that there's no way to tell *why*
> they're enabled. The same regulator can supply more than one device,
> and thus be turned on by multiple drivers. Because of this there's no
> way to tell if corrective measures should be taken before a
> regulator_put.
A better question would be: What happens if you call
regulator_disable() after regulator_enable() fails? Does this end up
not mattering?
> > How come you have an ohci_regulator_put_all() routine but not an
> > ohci_regulator_disable_all() routine?
> >
> > And likewise for _get_ and _enable_?
>
> The code that would've been ohci_regulator_[get|enable|disable]_all is
> used only once in each case, so I didn't feel the need to encapsulate
> it into a function. I can make the change if you want me to.
No, it's okay the way you did it. I just wanted to make sure.
> >> @@ -427,6 +459,13 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_device
> >> goto err3;
> >> }
> >>
> >> + for (i = 0; i < PXA_UHC_MAX_PORTNUM; i++) {
> >> + snprintf(supply, sizeof(supply), "fsusb%d", i);
> >> + ohci->ext_regulator[i] = regulator_get(&pdev->dev, supply);
> >> + if (IS_ERR(ohci->ext_regulator[i]))
> >> + ohci->ext_regulator[i] = NULL;
> >
> > Shouldn't this be a fatal error?
>
> No, and it's not even necessarily an error. This can happen for valid
> reasons, for example if there is no regulator powered device on that
> port. In this case the appropriate fsusb%d regulator wouldn't have been
> registered, and regulator_get would fail.
> If it happened due to some other error, it's still not fatal because
> the subsystem and the rest of the regulator powered usb devices can
> continue to work normally.
And for the same reason, you don't consider an error in
regulator_enable() to be fatal. All right.
Alan Stern
--
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
