RE: [PATCH v10 1/4] gadget: Introduce the usb charger framework

[Jun Li]

Hi

> -----Original Message-----
> From: Baolin Wang [mailto:baolin.wang@xxxxxxxxxx]
> Sent: Friday, April 08, 2016 5:56 PM
> To: Jun Li <jun.li@xxxxxxx>
> Cc: balbi@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; sre@xxxxxxxxxx;
> dbaryshkov@xxxxxxxxx; dwmw2@xxxxxxxxxxxxx; peter.chen@xxxxxxxxxxxxx;
> stern@xxxxxxxxxxxxxxxxxxx; r.baldyga@xxxxxxxxxxx;
> yoshihiro.shimoda.uh@xxxxxxxxxxx; lee.jones@xxxxxxxxxx; broonie@xxxxxxxxxx;
> ckeepax@xxxxxxxxxxxxxxxxxxxxxxxxxxx; patches@xxxxxxxxxxxxxxxxxxxxxxxxxxx;
> linux-pm@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; device-
> mainlining@xxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v10 1/4] gadget: Introduce the usb charger framework
> 
> Hi Jun,
> 
> On 8 April 2016 at 16:04, Jun Li <jun.li@xxxxxxx> wrote:
> > Hi, Baolin
> >
> >> +/*
> >> + * usb_charger_detect_type() - detect the charger type manually.
> >> + * @uchger - usb charger device
> >> + *
> >> + * Note: You should ensure you need to detect the charger type
> >> +manually on your
> >> + * platform.
> >> + * You should call it at the right gadget state to avoid affecting
> >> +gadget
> >> + * enumeration.
> >> + */
> >> +int usb_charger_detect_type(struct usb_charger *uchger) {
> >> +     enum usb_charger_type type;
> >> +
> >> +     if (WARN(!uchger->charger_detect,
> >> +              "charger detection method should not be NULL"))
> >> +             return -EINVAL;
> >> +
> >> +     type = uchger->charger_detect(uchger);
> >> +
> >> +     mutex_lock(&uchger->lock);
> >> +     uchger->type = type;
> >> +     mutex_unlock(&uchger->lock);
> >> +
> >> +     return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_charger_detect_type);
> >
> > I still recommend have a central place to call
> > usb_charger_detect_type() to cover real charger detection instead of
> > leaving this question open to diff users. This can be done after
> > vbus-on is detected and before do gadget connect. I don't think this
> will make your framework complicated.
> 
> This API is used for detecting the charger type manually (software charger
> detection), so if user's udc driver is needed to do this, then they can
> issue it at their right place to make it more flexible. But let us see
> other people's suggestion. Thanks.

Ok, actually this API can be used for what ever charger detection
type, user can implement any method(manual detection, directly read
result from some HW...what ever) in uchger->charger_detect(), target is
to have the charger type and set uchger->type, then you no need to add the comments/description to limit it usage. Also I do see there is possible
central place to do this.

> 
> >
> > Hi Felipe, what do you think of this?
> >
> > Li Jun
> >
> >> +
> >> +/*
> >> + * usb_charger_get_type_by_others() - Get the usb charger type by
> >> +the callback
> >> + * which is implemented by users.
> >> + * @uchger - the usb charger device.
> >> + *
> >> + * Note: This function is just used for getting the charger type,
> >> +not for
> >> + * detecting charger type which might affect the DP/DM line when
> >> +gadget is on
> >> + * enumeration state.
> >> + */
> >> +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;
> >> +}
> >> +
> 
> 
> 
> --
> Baolin.wang
> Best Regards