Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework

[Baolin Wang]

On 6 April 2016 at 16:26, Jun Li <jun.li@xxxxxxx> wrote:
> Hi
>
>> + */
>> +static enum usb_charger_type
>> +usb_charger_get_type_by_others(struct usb_charger *uchger) {
>> +     if (uchger->type != UNKNOWN_TYPE)
>> +             return uchger->type;
>> +
>> +     if (uchger->psy) {
>> +             union power_supply_propval val;
>> +
>> +             power_supply_get_property(uchger->psy,
>> +                                       POWER_SUPPLY_PROP_CHARGE_TYPE,
>> +                                       &val);
>> +             switch (val.intval) {
>> +             case POWER_SUPPLY_TYPE_USB:
>> +                     uchger->type = SDP_TYPE;
>> +                     break;
>> +             case POWER_SUPPLY_TYPE_USB_DCP:
>> +                     uchger->type = DCP_TYPE;
>> +                     break;
>> +             case POWER_SUPPLY_TYPE_USB_CDP:
>> +                     uchger->type = CDP_TYPE;
>> +                     break;
>> +             case POWER_SUPPLY_TYPE_USB_ACA:
>> +                     uchger->type = ACA_TYPE;
>> +                     break;
>> +             default:
>> +                     uchger->type = UNKNOWN_TYPE;
>> +                     break;
>> +             }
>> +     } else if (uchger->get_charger_type) {
>> +             uchger->type = uchger->get_charger_type(uchger);
>> +     } else {
>> +             uchger->type = UNKNOWN_TYPE;
>> +     }
>> +
>> +     return uchger->type;
>> +}
>> +
>
> I think we may don't need this usb_charger_get_type_by_others().
> "uchger->type" is set in one place is enough, that is: by
> uchger->charger_detect() in usb_charger_detect_type(), then
> usb_charger_get_type_by_others() is replaced by usb_charger_get_type().
>
> uchger->charger_detect() can have diff implementations no matter
> what kind of mechanism is used, for your PMIC case, you can just
> directly get the type value by power_supply_get_property();
> with that, we can have one central place to set uchger->type.
> After uchger->type is set, charger type detection is no need to be
> involved until charger type changes.
>
> Then next question is where is to call usb_charger_detect_type(),
> We need make sure it finished before usb gadget connect.

Yeah, that's the point: where? It is hard for usb charger framework to
control, which will make it more complicated. The
'usb_charger_detect_type()' is used for detecting the charger type
manually on user's platform, and user should call it at the right time
to avoid affecting gadget enumeration. Otherwise user can implement
some callbacks showed in 'usb_charger_get_type_by_others()' function
to get charger type. I think this is controllable and simple for the
usb charger framework.

>
> Ideal is with your framework, diff users only need implement
> uchger->charger_detect(). :)
>

-- 
Baolin.wang
Best Regards