Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[NeilBrown]

On Fri, Sep 09 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Fri, Sep 09, 2016 at 09:13:31AM +1000, NeilBrown wrote:
>
>> Conceptually, the PHY is separate from the power manager and a solution
>> which recognises that will be more universal.
>
> The wm831x driver in the patch series is an example of such hardware -
> it is purely a power manager, it has no USB PHY hardware at all.  It's a
> current limiter intended to sit in line with the USB power lines to
> ensure the system doesn't go over the maximum current draw (and also
> integrates with the power source selection logic the chip has to pick
> the best available power source for the system).

It might be instructive to look at exactly what happens with this
device.
The "probe" routine calls

 +			usb_charger_find_by_name(wm831x_pdata->usb_gadget);

Presumably wm831x_pdata is initialised by a board file?
I strongly suspect it is initialized to  "usb-charger.0" because the
names given to usb chargers are "usb-charger.%d" in discovery order.
I don't see this being at all useful if there is ever more than one
usb-charger.
Do you?

The probe function then registers for charger notifications.  When they
arrive, wm831x_usb_limit_change() will set the highest supported limit
which is less than the notified "limit".  So presumably that "limit"
must be the minimum guaranteed by the charger type.

Let's see when the notification is called...
->uchgr_nh is sent a notification from usb_charger_notify_others()
with (in the "charger is present" case) the value of
   __usb_charger_get_cur_limit(uchger)
which just pulls values out of the cur_limit structure, based on the
type, reported by
		usb_charger_get_type_by_others(uchger);
(The default values in this structure are not the minimums guaranteed
by the charger types, they are generally higher.  So this could easily
result in the charger shutting down).

usb_charger_get_type_by_others()  has two ways to get the charger
type, which it then caches in uchger->type until the charger is removed.

If there is a uchger->psy power supply, then the
  POWER_SUPPLY_PROP_CHARGE_TYPE
property is used...  Oh, that is weird.
The valid values for that property are:

enum {
        POWER_SUPPLY_CHARGE_TYPE_UNKNOWN = 0,
        POWER_SUPPLY_CHARGE_TYPE_NONE,
        POWER_SUPPLY_CHARGE_TYPE_TRICKLE,
        POWER_SUPPLY_CHARGE_TYPE_FAST,
};

but the code in usb_charger_get_type_by_others() compares it against:
 +		case POWER_SUPPLY_TYPE_USB:
 +		case POWER_SUPPLY_TYPE_USB_DCP:
 +		case POWER_SUPPLY_TYPE_USB_CDP:
 +		case POWER_SUPPLY_TYPE_USB_ACA:

Presumably that it just a bug and it was meant to request the
  POWER_SUPPLY_PROP_TYPE ??
I wonder how much testing was done on this code?

Anyway, assuming it is meant to request the TYPE, where is that set?
The new code doesn't set it at all.
Only three existing power supplies set any of
  POWER_SUPPLY_TYPE_USB_*
axp288_charger.c  gpio-charger.c isp1704_charger.c
As I wrote in https://lwn.net/Articles/694062/
axp288_charger.c is broken and cannot possibly work.
gpio-charger.c configures the type at boot-time so it cannot sensibly
detect a newly plugged in charger (how does that make any sense)
and isp1704_charger.c peeks in the usb registers (ULPI) to work out
the charger type.

None of these set the  POWER_SUPPLY_PROP_TYPE in a useful way, so why
would usb_charger_get_type_by_others() want to use that property?

Maybe it really does want to use POWER_SUPPLY_PROP_CHARGE_TYPE?
Quite a few chargers set that.  It would be a challenge to map names
like "TRICKLE" and "FAST" into mA values for a current limiter though.
My hardware knowledge is running out here.. I see wm8350_power.c reports
that property, but I don't know how that device fits into the picture.
With the patch, that driver would request that property from somewhere
else(?) and also report it.  That is kind-a strange.

The other possible source for the charger type is a call to
   uchger->get_charger_type()

There is no get_charger_type() function anywhere in the patchset.  No code
ever sets that field in the uchger.

So how can this wm831x driver actually find out what sort of charger is
connected and so set the power limit?  I just don't see this working *at*
*all*.

NeilBrown

Attachment: signature.asc
Description: PGP signature