lkml.org 
[lkml]   [2016]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFT PATCH 1/3] usb: misc: usb3503: Fix HUB mode after bootloader initialization
From
Date
On 04/29/2016 01:30 PM, Mark Brown wrote:
> On Fri, Apr 29, 2016 at 12:59:49PM +0200, Krzysztof Kozlowski wrote:
>
>> +++ b/Documentation/devicetree/bindings/usb/usb3503.txt
>> @@ -24,6 +24,7 @@ Optional properties:
>> pins (optional, if not provided, driver will not set rate of the
>> REFCLK signal and assume that a value from the primary reference
>> clock frequencies table is used)
>> +- vdd33-supply: Optional supply for VDD 3.3 V power source.
>
> Supplies are only optional if they may be physically absent. In this
> case it's possible that on device regulators may be used instead, a
> pattern more like that used for arizona-ldo1 where we represent those
> regulators might be better as it's more clearly describing the
> situation. I'm just wondering if the supply lookup stuff there should
> be factored out as this is not an uncommon pattern..
>
> It should at least be clearly stated what's going on, ignoring failure
> to get supplies is generally a bug and people will tend to blindly cut
> and paste things (witness all the breakage in graphics drivers with
> this).

The device has four power input lines (called VBAT, VDD33, VDD_CORE and
VDD_12). Datasheet describes 4 valid configurations... but impression of
the Odroid U3 board schematics is that they used another (custom?)
configuration.

I did not add rest of regulators on purpose:
1. I don't have other configurations to test.
2. It is rather old device, so I don't expect active development.

The VDD33 is really optional. The device can work in different
configuration, e.g. only on VBAT. How the reset logic would work then? I
don't know... I would suspect that it could be exactly the same (just
replace VDD33 with VBAT) but I am not sure.

>> static int usb3503_reset(struct usb3503 *hub, int state)
>> {
>> + int err;
>> +
>> + err = usb3503_regulator(hub, state);
>> + if (err) {
>> + dev_err(hub->dev, "unable to %s VDD33 regulator to (%d)\n",
>> + (state ? "enable" : "disable"), err);
>> + }
>
> Are we sure that the callers all balance enables and disables and we
> don't ever end up going through reset more than once on the way down?

I double checked the code and there might be in-balance if DT or
platform data sets initial mode to suspend. Otherwise it should be balanced.

I'll re-think the patch and fix this.

>
>> + hub->vdd_reg = devm_regulator_get_optional(dev, "vdd33");
>> + if (IS_ERR(hub->vdd_reg)) {
>> + if (PTR_ERR(hub->vdd_reg) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>
> This should explicitly check for -ENODEV and return the error if it gets
> anything else, that will mean that if the supply is needed but lookup
> fails somehow due to a non-deferral error we'll handle it properly.

I must admit I wasn't sure about handling the ENODEV and some other
examples (drivers) were handling this just like that.

Thanks for pointing this out.

>
>> + err = usb3503_regulator(hub, true);
>
> The naming on this function is very obscure (and there's also a couple
> of other supplies). I'd suggest just folding this into the reset
> function, or at least renaming so the reader can tell what these calls
> do.

Okay.

Thanks for feedback!

Best regards,
Krzysztof

\
 
 \ /
  Last update: 2016-04-29 14:21    [W:0.102 / U:0.920 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site