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

[Baolin Wang]

Hi Neil,

Sorry for late reply due yo my holiday.

On 10 September 2016 at 05:19, NeilBrown <neilb@xxxxxxxx> wrote:
> On Fri, Sep 09 2016, Baolin Wang wrote:
>
>>>
>>> In practice, the USB PHY and the Power manager will often be in the same
>>> IC (the PMIC) so the driver for one could look at the registers for the
>>> other.
>>> But there is no guarantee that the hardware works like that.  It is
>>> best to create a generally design.
>>
>> Yes, we hope to create one generally design, so we need to consider
>> this situation: the power supply getting the charger type by accessing
>> PMIC registers. The registers which save the charger type are not
>> always belong to the USB PHY, may be just some registers on PMIC.
>
> If the power_supply can directly detect the type of charger, then it
> doesn't need any usb-charger-infrastructure to tell it.  It can handle
> current selection entirely internally.
> Surely the only interesting case for a framework to address is the one
> where the power_supply cannot directly detect the charger type.

But power supply also need one plugged or unplugged event to set
current from USB charger framework. Another hand, considering one
generic framework, since power supply support API (e.g.:
power_supply_get_property()) to get charger type for others, we should
integrate power supply into USB charger framework.

>
>>
>> Now in mainline kernel, there are 3 methods can get the charger type
>> which need to integrate with USB charger framework:
>> 1. power supply
>> 2. extcon (need to add as you suggested)
>> 3. others (by 'get_charger_type' callback of USB charger)
>
> There is no "get_charger_type" now in the mainline kernel.

We want to create one generic framework, thus we should consider some
users want to implement the function to get charger type by accessing
PMIC registers or else.

>
>>>>
>>>> Suppose the USB configuration requests 100mA, then we should set the
>>>> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
>>>> funtion, then notify this to power driver.
>>>
>>> ahh.... I had missed something there.  It's a while since I looked
>>> closely at these patches.
>>>
>>> Only.... this usage of usb_charger_set_cur_limit_by_type() is really
>>> nonsensical.
>>>
>>> If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
>>> the number negotiated with the USB configuration is not relevant and
>>> should be ignored.  There is a guaranteed minimum which is at least the
>>> maximum that *can* be negotiated.
>>
>> Yes. If it is not relevant, we will no't set the current from USB
>> configuration. Just when your charger type is SDP and the USB
>> enumeration is done, we can get the USB configuration from host to set
>> current.
>
> But you do!
> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
> Your patch passes that to usb_charger_set_cur_limit_by_type()
> which calls __usb_charger_set_cur_limit_by_type() which will set the
> cur_limit for whichever type uchger->type currently is.
>
> So when it is not relevant, your code *does* set some current limit.

Suppose the charger type is DCP(it is not relevant to the mA number
from the USB configuration ), it will not do the USB enumeration, then
no USB configuration from host to set current.

-- 
Baolin.wang
Best Regards