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 7:51 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
> 
> On 8 April 2016 at 19:27, Jun Li <jun.li@xxxxxxx> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Baolin Wang [mailto:baolin.wang@xxxxxxxxxx]
> >> Sent: Friday, April 08, 2016 7:00 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,
> >>
> >> >> >> +/*
> >> >> >> + * 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();
	...
}

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