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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <AM4PR04MB21301A33C5EC5D57D122E189896B0@AM4PR04MB2130.eurprd04.prod.outlook.com>
Date:	Mon, 18 Apr 2016 08:27:52 +0000
From:	Jun Li <jun.li@....com>
To:	Baolin Wang <baolin.wang@...aro.org>
CC:	"balbi@...nel.org" <balbi@...nel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"sre@...nel.org" <sre@...nel.org>,
	"dbaryshkov@...il.com" <dbaryshkov@...il.com>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"peter.chen@...escale.com" <peter.chen@...escale.com>,
	"stern@...land.harvard.edu" <stern@...land.harvard.edu>,
	"r.baldyga@...sung.com" <r.baldyga@...sung.com>,
	"yoshihiro.shimoda.uh@...esas.com" <yoshihiro.shimoda.uh@...esas.com>,
	"lee.jones@...aro.org" <lee.jones@...aro.org>,
	"broonie@...nel.org" <broonie@...nel.org>,
	"ckeepax@...nsource.wolfsonmicro.com" 
	<ckeepax@...nsource.wolfsonmicro.com>,
	"patches@...nsource.wolfsonmicro.com" 
	<patches@...nsource.wolfsonmicro.com>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"device-mainlining@...ts.linuxfoundation.org" 
	<device-mainlining@...ts.linuxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v10 1/4] gadget: Introduce the usb charger framework

Hi

> -----Original Message-----
> From: Baolin Wang [mailto:baolin.wang@...aro.org]
> Sent: Monday, April 11, 2016 7:15 PM
> To: Jun Li <jun.li@....com>
> Cc: balbi@...nel.org; gregkh@...uxfoundation.org; sre@...nel.org;
> dbaryshkov@...il.com; dwmw2@...radead.org; peter.chen@...escale.com;
> stern@...land.harvard.edu; r.baldyga@...sung.com;
> yoshihiro.shimoda.uh@...esas.com; lee.jones@...aro.org; broonie@...nel.org;
> ckeepax@...nsource.wolfsonmicro.com; patches@...nsource.wolfsonmicro.com;
> linux-pm@...r.kernel.org; linux-usb@...r.kernel.org; device-
> mainlining@...ts.linuxfoundation.org; linux-kernel@...r.kernel.org
> 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