Message-ID: <CAMz4kuKGLor6t74TuwuS7ZCXH=Mu+UuC_6s5MUCVQB+8RPFafQ@mail.gmail.com>
Date:	Mon, 11 Apr 2016 19:15:02 +0800
From:	Baolin Wang <baolin.wang@...aro.org>
To:	Jun Li <jun.li@....com>
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 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.

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

Powered by blists - more mailing lists