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

[Jun Li]

Hi

> -----Original Message-----
> From: Baolin Wang [mailto:baolin.wang@xxxxxxxxxx]
> Sent: Monday, April 11, 2016 7:15 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,
> 
> Sorry for late reply.
> 
> >> >> >> >> +/*
> >> >> >> >> + * 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
> >> >>
> >> >> But reading result from some HW dose not means *detect*, actually
> >> >> is
> >> *get*.
> >> >> We can not mix them together with the different semantics.
> >> >
> >> > It doesnât matter here, you already define the _get_ API for just
> >> > read the charger type from the saved value(uchger->type), so we can
> >> > think the API to make uchger->type from UNKNOW to ONE KNOWN type is
> >> > *detect*, because we don't need 2 separate API one for *get* type
> >> > from HW and another one for *detect* from HW, only one API involve
> >> > HW access
> >> is enough.
> >>
> >> But another issue is some users may need to get the charger type from
> >> power supply by "power_supply_get_property()" function, we need to
> >> integrate with the power supply things in the usb charger framework,
> >> not user to implement that.
> >
> > Why this user(your case) can't put the charger type get by
> > "power_supply_get_property()" in its uchger->charger_detect(), any
> > special reason prevent you doing it? I am not sure if this case is
> > very common, even if it is, you also can put it in
> > usb_charger_detect_type()
> >
> > usb_charger_detect_type()
> > {
> >         If can get from power_supply_get_property()
> >                 ...
> >         else if uchger->charger_detect
> >                 uchger->charger_detect();
> >         ...
> > }
> 
> If user want to implement above method, they need to get the
> 'power_supply' structure to do this action, which maybe can not get in
> some contexts. So we need to integrate with the power supply things to
> avoid this situation. IIRC, Felipe suggested me to flesh out the charger
> getting method.
> 

Okay, then I agree with you to let user do that with more flexibility,
I tried to enable usb charger detection on one NXP i.mx platform based on
this framework, works fine, thanks!
 
Li Jun
> Hi Felipe, what do you think of Jun's suggestion? Thanks.
> 
> >
> > I don't know if most design of usb charger detection now days is as
> > easy as your PMIC for software(HW does the whole detection process and
> > prevent the vbus generation until detection has completed), anyway if
> > your framework can guarantee the detection process finished before
> > gadget connect, then each user don't have to worry about the HW
> > detection process details and seek a proper place to do it.
> >
> > Li Jun
> >>
> >> >
> >> > - Detect: (write to and) read from HW to get the charger type,
> >> >   save to uchger->type;
> >> > - Get: read the charger type from uchger->type.
> >> >
> >> >>
> >> >> > 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.
> >> >> >
> >> >>
> >> >> --
> >> >> Baolin.wang
> >> >> Best Regards
> >>
> >>
> >>
> >> --
> >> Baolin.wang
> >> Best Regards
> 
> 
> 
> --
> Baolin.wang
> Best Regards