Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
On 7 April 2016 at 12:56, Felipe Balbi wrote:
>
> Hi,
>
> Peter Chen writes:
>> On Wed, Apr 06, 2016 at 01:25:06PM +0300, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Peter Chen writes:
>>> > On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
>>> >> Peter Chen writes:
>>> >> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
>>> >> >> Peter Chen writes:
>>> >> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>>> >> >> > +
>>> >> >> >> +static struct attribute *usb_charger_attrs[] = {
>>> >> >> >> + &dev_attr_sdp_current.attr,
>>> >> >> >> + &dev_attr_dcp_current.attr,
>>> >> >> >> + &dev_attr_cdp_current.attr,
>>> >> >> >> + &dev_attr_aca_current.attr,
>>> >> >> >> + &dev_attr_charger_type.attr,
>>> >> >> >> + &dev_attr_charger_state.attr,
>>> >> >> >> + NULL
>>> >> >> >> +};
>>> >> >> >
>>> >> >> > The user may only care about current limit, type and state, why they
>>> >> >> > need to care what type's current limit, it is the usb charger
>>> >> >> > framework handles, the framework judge the current according to
>>> >> >> > charger type and USB state (connect/configured/suspended).
>>> >> >>
>>> >> >> it might be useful if we want to know that $this charger doesn't
>>> >> >> really
>>> >> >> give us as much current as it advertises.
>>> >> >>
>>> >> >
>>> >> > As my understanding, the current limit is dynamic value, it should
>>> >> > report the value the charger supports now, eg, it connects SDP, but
>>> >> > the host is suspended now, then the value should be 2mA.
>>> >>
>>> >> yes, and that's the limit. Now consider we connect to DCP or CDP and
>>> >> limit is 2000mA but we're charging at 1000mA ;-)
>>> >>
>>> >
>>> > Does the user need to know the $this charger limit? Don't they only
>>> > care about the current charging value? I have a USB cable which can
>>>
>>> Why not ? UI might want to change the color of the battery charging icon
>>> if we're charging @ 2000mA or @ 1000mA to give some visual feedback as
>>> to "how fast" battery is supposed to be charged.
>>>
>>> > show charging current value, it changes from time to time, when it
>>> > connects to host pc, it shows 430mA; when it connects to dedicated
>>> > charger, it shows 1000mA.
>>>
>>> good for you, now what does that have to do with $subject ?
>>>
>>
>> +static struct attribute *usb_charger_attrs[] = {
>> + &dev_attr_sdp_current.attr,
>> + &dev_attr_dcp_current.attr,
>> + &dev_attr_cdp_current.attr,
>> + &dev_attr_aca_current.attr,
>> + &dev_attr_charger_type.attr,
>> + &dev_attr_charger_state.attr,
>> + NULL
>> +};
>>
>> Ok, even the users are interesting in current limit, we still have no
>> necessary to know all kinds of chargers limit and current value, since
>> we already have charger type user interface, the framework can show
>> limit according to charger type.
>
> Oh, now I get your comment and I totally agree. We already *know* the
> detected charger type, there's no point in showing them all.
>
>> I think below user interfaces are enough, who do you think?
>>
>> +static struct attribute *usb_charger_attrs[] = {
>> + &dev_attr_current.attr,
>> + &dev_attr_current_limit.attr,
>> + &dev_attr_charger_type.attr,
>> + &dev_attr_charger_state.attr,
>> + NULL
>> +};
>
> agreed, const though.
OK. Thanks for Felipe, Peter and Jun's suggestion. I'll modify it in
next version.
>
> --
> balbi
--
Baolin.wang
Best Regards
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
Hi,
Peter Chen writes:
> On Wed, Apr 06, 2016 at 01:25:06PM +0300, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Peter Chen writes:
>> > On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
>> >> Peter Chen writes:
>> >> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
>> >> >> Peter Chen writes:
>> >> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>> >> >> > +
>> >> >> >> +static struct attribute *usb_charger_attrs[] = {
>> >> >> >> + &dev_attr_sdp_current.attr,
>> >> >> >> + &dev_attr_dcp_current.attr,
>> >> >> >> + &dev_attr_cdp_current.attr,
>> >> >> >> + &dev_attr_aca_current.attr,
>> >> >> >> + &dev_attr_charger_type.attr,
>> >> >> >> + &dev_attr_charger_state.attr,
>> >> >> >> + NULL
>> >> >> >> +};
>> >> >> >
>> >> >> > The user may only care about current limit, type and state, why they
>> >> >> > need to care what type's current limit, it is the usb charger
>> >> >> > framework handles, the framework judge the current according to
>> >> >> > charger type and USB state (connect/configured/suspended).
>> >> >>
>> >> >> it might be useful if we want to know that $this charger doesn't really
>> >> >> give us as much current as it advertises.
>> >> >>
>> >> >
>> >> > As my understanding, the current limit is dynamic value, it should
>> >> > report the value the charger supports now, eg, it connects SDP, but
>> >> > the host is suspended now, then the value should be 2mA.
>> >>
>> >> yes, and that's the limit. Now consider we connect to DCP or CDP and
>> >> limit is 2000mA but we're charging at 1000mA ;-)
>> >>
>> >
>> > Does the user need to know the $this charger limit? Don't they only
>> > care about the current charging value? I have a USB cable which can
>>
>> Why not ? UI might want to change the color of the battery charging icon
>> if we're charging @ 2000mA or @ 1000mA to give some visual feedback as
>> to "how fast" battery is supposed to be charged.
>>
>> > show charging current value, it changes from time to time, when it
>> > connects to host pc, it shows 430mA; when it connects to dedicated
>> > charger, it shows 1000mA.
>>
>> good for you, now what does that have to do with $subject ?
>>
>
> +static struct attribute *usb_charger_attrs[] = {
> + &dev_attr_sdp_current.attr,
> + &dev_attr_dcp_current.attr,
> + &dev_attr_cdp_current.attr,
> + &dev_attr_aca_current.attr,
> + &dev_attr_charger_type.attr,
> + &dev_attr_charger_state.attr,
> + NULL
> +};
>
> Ok, even the users are interesting in current limit, we still have no
> necessary to know all kinds of chargers limit and current value, since
> we already have charger type user interface, the framework can show
> limit according to charger type.
Oh, now I get your comment and I totally agree. We already *know* the
detected charger type, there's no point in showing them all.
> I think below user interfaces are enough, who do you think?
>
> +static struct attribute *usb_charger_attrs[] = {
> + &dev_attr_current.attr,
> + &dev_attr_current_limit.attr,
> + &dev_attr_charger_type.attr,
> + &dev_attr_charger_state.attr,
> + NULL
> +};
agreed, const though.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
On Wed, Apr 06, 2016 at 01:25:06PM +0300, Felipe Balbi wrote:
>
> Hi,
>
> Peter Chen writes:
> > On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
> >> Peter Chen writes:
> >> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
> >> >> Peter Chen writes:
> >> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> >> >> > +
> >> >> >> +static struct attribute *usb_charger_attrs[] = {
> >> >> >> +&dev_attr_sdp_current.attr,
> >> >> >> +&dev_attr_dcp_current.attr,
> >> >> >> +&dev_attr_cdp_current.attr,
> >> >> >> +&dev_attr_aca_current.attr,
> >> >> >> +&dev_attr_charger_type.attr,
> >> >> >> +&dev_attr_charger_state.attr,
> >> >> >> +NULL
> >> >> >> +};
> >> >> >
> >> >> > The user may only care about current limit, type and state, why they
> >> >> > need to care what type's current limit, it is the usb charger
> >> >> > framework handles, the framework judge the current according to
> >> >> > charger type and USB state (connect/configured/suspended).
> >> >>
> >> >> it might be useful if we want to know that $this charger doesn't really
> >> >> give us as much current as it advertises.
> >> >>
> >> >
> >> > As my understanding, the current limit is dynamic value, it should
> >> > report the value the charger supports now, eg, it connects SDP, but
> >> > the host is suspended now, then the value should be 2mA.
> >>
> >> yes, and that's the limit. Now consider we connect to DCP or CDP and
> >> limit is 2000mA but we're charging at 1000mA ;-)
> >>
> >
> > Does the user need to know the $this charger limit? Don't they only
> > care about the current charging value? I have a USB cable which can
>
> Why not ? UI might want to change the color of the battery charging icon
> if we're charging @ 2000mA or @ 1000mA to give some visual feedback as
> to "how fast" battery is supposed to be charged.
>
> > show charging current value, it changes from time to time, when it
> > connects to host pc, it shows 430mA; when it connects to dedicated
> > charger, it shows 1000mA.
>
> good for you, now what does that have to do with $subject ?
>
+static struct attribute *usb_charger_attrs[] = {
+ &dev_attr_sdp_current.attr,
+ &dev_attr_dcp_current.attr,
+ &dev_attr_cdp_current.attr,
+ &dev_attr_aca_current.attr,
+ &dev_attr_charger_type.attr,
+ &dev_attr_charger_state.attr,
+ NULL
+};
Ok, even the users are interesting in current limit, we still have no
necessary to know all kinds of chargers limit and current value, since
we already have charger type user interface, the framework can show
limit according to charger type.
I think below user interfaces are enough, who do you think?
+static struct attribute *usb_charger_attrs[] = {
+ &dev_attr_current.attr,
+ &dev_attr_current_limit.attr,
+ &dev_attr_charger_type.attr,
+ &dev_attr_charger_state.attr,
+ NULL
+};
--
Best Regards,
Peter Chen
RE: [PATCH v9 1/4] gadget: Introduce the usb charger framework
Hi
> -Original Message-
> From: Baolin Wang [mailto:baolin.w...@linaro.org]
> Sent: Wednesday, April 06, 2016 7:31 PM
> To: Jun Li
> Cc: ba...@kernel.org; gre...@linuxfoundation.org; s...@kernel.org;
> dbarysh...@gmail.com; dw...@infradead.org; peter.c...@freescale.com;
> st...@rowland.harvard.edu; r.bald...@samsung.com;
> yoshihiro.shimoda...@renesas.com; lee.jo...@linaro.org; broo...@kernel.org;
> ckee...@opensource.wolfsonmicro.com; patc...@opensource.wolfsonmicro.com;
> linux...@vger.kernel.org; linux-...@vger.kernel.org; device-
> mainlin...@lists.linuxfoundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
>
> On 6 April 2016 at 16:26, Jun Li wrote:
> > Hi
> >
> >> + */
> >> +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;
> >> +}
> >> +
> >
> > I think we may don't need this usb_charger_get_type_by_others().
> > "uchger->type" is set in one place is enough, that is: by
> > uchger->charger_detect() in usb_charger_detect_type(), then
> > usb_charger_get_type_by_others() is replaced by usb_charger_get_type().
> >
> > uchger->charger_detect() can have diff implementations no matter
> > what kind of mechanism is used, for your PMIC case, you can just
> > directly get the type value by power_supply_get_property(); with that,
> > we can have one central place to set uchger->type.
> > After uchger->type is set, charger type detection is no need to be
> > involved until charger type changes.
> >
> > Then next question is where is to call usb_charger_detect_type(), We
> > need make sure it finished before usb gadget connect.
>
> Yeah, that's the point: where? It is hard for usb charger framework to
> control, which will make it more complicated. The
> 'usb_charger_detect_type()' is used for detecting the charger type
> manually on user's platform, and user should call it at the right time to
> avoid affecting gadget enumeration. Otherwise user can implement some
> callbacks showed in 'usb_charger_get_type_by_others()' function to get
> charger type. I think this is controllable and simple for the usb charger
> framework.
As my suggested, Can this detection be in usb_udc_vbus_handler(), and
before usb_udc_connect_control()? We should be able to find some central
place where it is between vbus detection and gadget connect.
>
> >
> > Ideal is with your framework, diff users only need implement
> > uchger->charger_detect(). :)
> >
>
> --
> Baolin.wang
> Best Regards
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
On 6 April 2016 at 16:26, Jun Li wrote:
> Hi
>
>> + */
>> +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;
>> +}
>> +
>
> I think we may don't need this usb_charger_get_type_by_others().
> "uchger->type" is set in one place is enough, that is: by
> uchger->charger_detect() in usb_charger_detect_type(), then
> usb_charger_get_type_by_others() is replaced by usb_charger_get_type().
>
> uchger->charger_detect() can have diff implementations no matter
> what kind of mechanism is used, for your PMIC case, you can just
> directly get the type value by power_supply_get_property();
> with that, we can have one central place to set uchger->type.
> After uchger->type is set, charger type detection is no need to be
> involved until charger type changes.
>
> Then next question is where is to call usb_charger_detect_type(),
> We need make sure it finished before usb gadget connect.
Yeah, that's the point: where? It is hard for usb charger framework to
control, which will make it more complicated. The
'usb_charger_detect_type()' is used for detecting the charger type
manually on user's platform, and user should call it at the right time
to avoid affecting gadget enumeration. Otherwise user can implement
some callbacks showed in 'usb_charger_get_type_by_others()' function
to get charger type. I think this is controllable and simple for the
usb charger framework.
>
> Ideal is with your framework, diff users only need implement
> uchger->charger_detect(). :)
>
--
Baolin.wang
Best Regards
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
Hi,
Peter Chen writes:
> On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
>> Peter Chen writes:
>> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
>> >> Peter Chen writes:
>> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>> >> > +
>> >> >> +static struct attribute *usb_charger_attrs[] = {
>> >> >> + &dev_attr_sdp_current.attr,
>> >> >> + &dev_attr_dcp_current.attr,
>> >> >> + &dev_attr_cdp_current.attr,
>> >> >> + &dev_attr_aca_current.attr,
>> >> >> + &dev_attr_charger_type.attr,
>> >> >> + &dev_attr_charger_state.attr,
>> >> >> + NULL
>> >> >> +};
>> >> >
>> >> > The user may only care about current limit, type and state, why they
>> >> > need to care what type's current limit, it is the usb charger
>> >> > framework handles, the framework judge the current according to
>> >> > charger type and USB state (connect/configured/suspended).
>> >>
>> >> it might be useful if we want to know that $this charger doesn't really
>> >> give us as much current as it advertises.
>> >>
>> >
>> > As my understanding, the current limit is dynamic value, it should
>> > report the value the charger supports now, eg, it connects SDP, but
>> > the host is suspended now, then the value should be 2mA.
>>
>> yes, and that's the limit. Now consider we connect to DCP or CDP and
>> limit is 2000mA but we're charging at 1000mA ;-)
>>
>
> Does the user need to know the $this charger limit? Don't they only
> care about the current charging value? I have a USB cable which can
Why not ? UI might want to change the color of the battery charging icon
if we're charging @ 2000mA or @ 1000mA to give some visual feedback as
to "how fast" battery is supposed to be charged.
> show charging current value, it changes from time to time, when it
> connects to host pc, it shows 430mA; when it connects to dedicated
> charger, it shows 1000mA.
good for you, now what does that have to do with $subject ?
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
Hi,
Peter Chen writes:
> On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
>> Peter Chen writes:
>> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
>> >> Peter Chen writes:
>> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>> >> > +
>> >> >> +static struct attribute *usb_charger_attrs[] = {
>> >> >> + &dev_attr_sdp_current.attr,
>> >> >> + &dev_attr_dcp_current.attr,
>> >> >> + &dev_attr_cdp_current.attr,
>> >> >> + &dev_attr_aca_current.attr,
>> >> >> + &dev_attr_charger_type.attr,
>> >> >> + &dev_attr_charger_state.attr,
>> >> >> + NULL
>> >> >> +};
>> >> >
>> >> > The user may only care about current limit, type and state, why they
>> >> > need to care what type's current limit, it is the usb charger
>> >> > framework handles, the framework judge the current according to
>> >> > charger type and USB state (connect/configured/suspended).
>> >>
>> >> it might be useful if we want to know that $this charger doesn't really
>> >> give us as much current as it advertises.
>> >>
>> >
>> > As my understanding, the current limit is dynamic value, it should
>> > report the value the charger supports now, eg, it connects SDP, but
>> > the host is suspended now, then the value should be 2mA.
>>
>> yes, and that's the limit. Now consider we connect to DCP or CDP and
>> limit is 2000mA but we're charging at 1000mA ;-)
>>
>
> The user doesn't need to know the value which spec designs.
because... ?
--
balbi
signature.asc
Description: PGP signature
RE: [PATCH v9 1/4] gadget: Introduce the usb charger framework
Hi
> -Original Message-
> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> ow...@vger.kernel.org] On Behalf Of Baolin Wang
> Sent: Friday, April 01, 2016 3:22 PM
> To: ba...@kernel.org; gre...@linuxfoundation.org; s...@kernel.org;
> dbarysh...@gmail.com; dw...@infradead.org
> Cc: peter.c...@freescale.com; st...@rowland.harvard.edu;
> r.bald...@samsung.com; yoshihiro.shimoda...@renesas.com;
> lee.jo...@linaro.org; broo...@kernel.org;
> ckee...@opensource.wolfsonmicro.com; patc...@opensource.wolfsonmicro.com;
> baolin.w...@linaro.org; linux...@vger.kernel.org; linux-
> u...@vger.kernel.org; device-mainlin...@lists.linuxfoundation.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH v9 1/4] gadget: Introduce the usb charger framework
>
...
> +/*
> + * usb_charger_get_type() - get the usb charger type with lock protection.
> + * @uchger - usb charger device
> + *
> + * Users can get the charger type by this safe API, rather than using
> +the
> + * usb_charger structure directly.
> + */
> +enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger)
> +{
> + enum usb_charger_type type;
> +
> + mutex_lock(&uchger->lock);
> + type = uchger->type;
> + mutex_unlock(&uchger->lock);
> +
> + return type;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get_type);
> +
> +/*
> + * 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);
> +
> +/*
> + * 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;
> +}
> +
I think we may don't need this usb_charger_get_type_by_others().
"uchger->type" is set in one place is enough, that is: by
uchger->charger_detect() in usb_charger_detect_type(), then
usb_charger_get_type_by_others() is replaced by usb_charger_get_type().
uchger->charger_detect() can have diff implementations no matter
what kind of mechanism is used, for your PMIC case, you can just
directly get the type value by power_supply_get_property();
with that, we can have one central place to set uchger->type.
After uchger->type is set, charger type detection is no need to be
involved until charger type changes.
Then next question is where is to call usb_charger_detect_type(),
We need make sure it finished before usb gadget connect.
Ideal is with your framework, diff users only need implement
uchger->charger_detect(). :)
Li Jun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
> Peter Chen writes:
> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
> >> Peter Chen writes:
> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> >> > +
> >> >> +static struct attribute *usb_charger_attrs[] = {
> >> >> + &dev_attr_sdp_current.attr,
> >> >> + &dev_attr_dcp_current.attr,
> >> >> + &dev_attr_cdp_current.attr,
> >> >> + &dev_attr_aca_current.attr,
> >> >> + &dev_attr_charger_type.attr,
> >> >> + &dev_attr_charger_state.attr,
> >> >> + NULL
> >> >> +};
> >> >
> >> > The user may only care about current limit, type and state, why they
> >> > need to care what type's current limit, it is the usb charger
> >> > framework handles, the framework judge the current according to
> >> > charger type and USB state (connect/configured/suspended).
> >>
> >> it might be useful if we want to know that $this charger doesn't really
> >> give us as much current as it advertises.
> >>
> >
> > As my understanding, the current limit is dynamic value, it should
> > report the value the charger supports now, eg, it connects SDP, but
> > the host is suspended now, then the value should be 2mA.
>
> yes, and that's the limit. Now consider we connect to DCP or CDP and
> limit is 2000mA but we're charging at 1000mA ;-)
>
The user doesn't need to know the value which spec designs.
--
Best Regards,
Peter Chen
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
> Peter Chen writes:
> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
> >> Peter Chen writes:
> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> >> > +
> >> >> +static struct attribute *usb_charger_attrs[] = {
> >> >> + &dev_attr_sdp_current.attr,
> >> >> + &dev_attr_dcp_current.attr,
> >> >> + &dev_attr_cdp_current.attr,
> >> >> + &dev_attr_aca_current.attr,
> >> >> + &dev_attr_charger_type.attr,
> >> >> + &dev_attr_charger_state.attr,
> >> >> + NULL
> >> >> +};
> >> >
> >> > The user may only care about current limit, type and state, why they
> >> > need to care what type's current limit, it is the usb charger
> >> > framework handles, the framework judge the current according to
> >> > charger type and USB state (connect/configured/suspended).
> >>
> >> it might be useful if we want to know that $this charger doesn't really
> >> give us as much current as it advertises.
> >>
> >
> > As my understanding, the current limit is dynamic value, it should
> > report the value the charger supports now, eg, it connects SDP, but
> > the host is suspended now, then the value should be 2mA.
>
> yes, and that's the limit. Now consider we connect to DCP or CDP and
> limit is 2000mA but we're charging at 1000mA ;-)
>
Does the user need to know the $this charger limit? Don't they only
care about the current charging value? I have a USB cable which can
show charging current value, it changes from time to time, when it
connects to host pc, it shows 430mA; when it connects to dedicated
charger, it shows 1000mA.
--
Best Regards,
Peter Chen
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
Peter Chen writes:
> On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
>> Peter Chen writes:
>> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>> > +
>> >> +static struct attribute *usb_charger_attrs[] = {
>> >> + &dev_attr_sdp_current.attr,
>> >> + &dev_attr_dcp_current.attr,
>> >> + &dev_attr_cdp_current.attr,
>> >> + &dev_attr_aca_current.attr,
>> >> + &dev_attr_charger_type.attr,
>> >> + &dev_attr_charger_state.attr,
>> >> + NULL
>> >> +};
>> >
>> > The user may only care about current limit, type and state, why they
>> > need to care what type's current limit, it is the usb charger
>> > framework handles, the framework judge the current according to
>> > charger type and USB state (connect/configured/suspended).
>>
>> it might be useful if we want to know that $this charger doesn't really
>> give us as much current as it advertises.
>>
>
> As my understanding, the current limit is dynamic value, it should
> report the value the charger supports now, eg, it connects SDP, but
> the host is suspended now, then the value should be 2mA.
yes, and that's the limit. Now consider we connect to DCP or CDP and
limit is 2000mA but we're charging at 1000mA ;-)
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
> Peter Chen writes:
> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> > +
> >> +static struct attribute *usb_charger_attrs[] = {
> >> + &dev_attr_sdp_current.attr,
> >> + &dev_attr_dcp_current.attr,
> >> + &dev_attr_cdp_current.attr,
> >> + &dev_attr_aca_current.attr,
> >> + &dev_attr_charger_type.attr,
> >> + &dev_attr_charger_state.attr,
> >> + NULL
> >> +};
> >
> > The user may only care about current limit, type and state, why they
> > need to care what type's current limit, it is the usb charger
> > framework handles, the framework judge the current according to
> > charger type and USB state (connect/configured/suspended).
>
> it might be useful if we want to know that $this charger doesn't really
> give us as much current as it advertises.
>
As my understanding, the current limit is dynamic value, it should
report the value the charger supports now, eg, it connects SDP, but
the host is suspended now, then the value should be 2mA.
--
Best Regards,
Peter Chen
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
Peter Chen writes:
> On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> +
>> +static struct attribute *usb_charger_attrs[] = {
>> +&dev_attr_sdp_current.attr,
>> +&dev_attr_dcp_current.attr,
>> +&dev_attr_cdp_current.attr,
>> +&dev_attr_aca_current.attr,
>> +&dev_attr_charger_type.attr,
>> +&dev_attr_charger_state.attr,
>> +NULL
>> +};
>
> The user may only care about current limit, type and state, why they
> need to care what type's current limit, it is the usb charger
> framework handles, the framework judge the current according to
> charger type and USB state (connect/configured/suspended).
it might be useful if we want to know that $this charger doesn't really
give us as much current as it advertises.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
+
> +static struct attribute *usb_charger_attrs[] = {
> + &dev_attr_sdp_current.attr,
> + &dev_attr_dcp_current.attr,
> + &dev_attr_cdp_current.attr,
> + &dev_attr_aca_current.attr,
> + &dev_attr_charger_type.attr,
> + &dev_attr_charger_state.attr,
> + NULL
> +};
The user may only care about current limit, type and state, why they
need to care what type's current limit, it is the usb charger
framework handles, the framework judge the current according to
charger type and USB state (connect/configured/suspended).
Peter
> +ATTRIBUTE_GROUPS(usb_charger);
> +
> +/*
> + * usb_charger_find_by_name() - Get the usb charger device by name.
> + * @name - usb charger device name.
> + *
> + * return the instance of usb charger device, the device must be
> + * released with usb_charger_put().
> + */
> +struct usb_charger *usb_charger_find_by_name(const char *name)
> +{
> + struct class_dev_iter iter;
> + struct device *dev;
> +
> + if (WARN(!name, "can't work with NULL name"))
> + return ERR_PTR(-EINVAL);
> +
> + /* Iterate all usb charger devices in the class */
> + class_dev_iter_init(&iter, usb_charger_class, NULL, NULL);
> + while ((dev = class_dev_iter_next(&iter))) {
> + if (!strcmp(dev_name(dev), name))
> + break;
> + }
> + class_dev_iter_exit(&iter);
> +
> + if (WARN(!dev, "can't find usb charger device"))
> + return ERR_PTR(-ENODEV);
> +
> + return dev_to_uchger(dev);
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_find_by_name);
> +
> +/*
> + * usb_charger_get() - Reference a usb charger.
> + * @uchger - usb charger
> + */
> +struct usb_charger *usb_charger_get(struct usb_charger *uchger)
> +{
> + return (uchger && get_device(&uchger->dev)) ? uchger : NULL;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get);
> +
> +/*
> + * usb_charger_put() - Dereference a usb charger.
> + * @uchger - charger to release
> + */
> +void usb_charger_put(struct usb_charger *uchger)
> +{
> + if (uchger)
> + put_device(&uchger->dev);
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_put);
> +
> +/*
> + * usb_charger_get_type() - get the usb charger type with lock protection.
> + * @uchger - usb charger device
> + *
> + * Users can get the charger type by this safe API, rather than using the
> + * usb_charger structure directly.
> + */
> +enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger)
> +{
> + enum usb_charger_type type;
> +
> + mutex_lock(&uchger->lock);
> + type = uchger->type;
> + mutex_unlock(&uchger->lock);
> +
> + return type;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get_type);
> +
> +/*
> + * 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);
> +
> +/*
> + * 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;
> + }
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
On 5 April 2016 at 15:56, Peter Chen wrote:
> On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>> +
>> +int devm_usb_charger_register(struct device *dev,
>> + struct usb_charger *uchger)
>> +{
>> + struct usb_charger **ptr;
>> + int ret;
>> +
>> + ptr = devres_alloc(devm_uchger_dev_unreg, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return -ENOMEM;
>> +
>> + ret = usb_charger_register(dev, uchger);
>> + if (ret) {
>> + devres_free(ptr);
>> + return ret;
>> + }
>> +
>> + *ptr = uchger;
>> + devres_add(dev, ptr);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_usb_charger_register);
>
> When the above API is expected to call? Can we use the USB charger
> without USB gadget?
I think this is open for user to do their specific initialization for
usb charger. That depends on how to initialize your usb charger
structure.
>
>> +
>> +int usb_charger_init(struct usb_gadget *ugadget)
>> +{
>> + struct usb_charger *uchger;
>> + struct extcon_dev *edev;
>> + struct power_supply *psy;
>> + int ret;
>> +
>> + uchger = kzalloc(sizeof(struct usb_charger), GFP_KERNEL);
>> + if (!uchger)
>> + return -ENOMEM;
>> +
>> + uchger->type = UNKNOWN_TYPE;
>> + uchger->state = USB_CHARGER_DEFAULT;
>> + uchger->id = -1;
>> +
>> + if (ugadget->speed >= USB_SPEED_SUPER)
>> + uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT_SS;
>> + else
>> + uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT;
>
> We still haven't known bus speed here, it is better do it after
> setting configuration has finished.
OK. Make sense.
--
Baolin.wang
Best Regards
Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> +
> +int devm_usb_charger_register(struct device *dev,
> + struct usb_charger *uchger)
> +{
> + struct usb_charger **ptr;
> + int ret;
> +
> + ptr = devres_alloc(devm_uchger_dev_unreg, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + ret = usb_charger_register(dev, uchger);
> + if (ret) {
> + devres_free(ptr);
> + return ret;
> + }
> +
> + *ptr = uchger;
> + devres_add(dev, ptr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_usb_charger_register);
When the above API is expected to call? Can we use the USB charger
without USB gadget?
> +
> +int usb_charger_init(struct usb_gadget *ugadget)
> +{
> + struct usb_charger *uchger;
> + struct extcon_dev *edev;
> + struct power_supply *psy;
> + int ret;
> +
> + uchger = kzalloc(sizeof(struct usb_charger), GFP_KERNEL);
> + if (!uchger)
> + return -ENOMEM;
> +
> + uchger->type = UNKNOWN_TYPE;
> + uchger->state = USB_CHARGER_DEFAULT;
> + uchger->id = -1;
> +
> + if (ugadget->speed >= USB_SPEED_SUPER)
> + uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT_SS;
> + else
> + uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT;
We still haven't known bus speed here, it is better do it after
setting configuration has finished.
Peter
> + uchger->cur_limit.dcp_cur_limit = DEFAULT_DCP_CUR_LIMIT;
> + uchger->cur_limit.cdp_cur_limit = DEFAULT_CDP_CUR_LIMIT;
> + uchger->cur_limit.aca_cur_limit = DEFAULT_ACA_CUR_LIMIT;
> +
> + mutex_init(&uchger->lock);
> + RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
> +
> + /* register a notifier on a extcon device if it is exsited */
> + edev = extcon_get_edev_by_phandle(ugadget->dev.parent, 0);
> + if (!IS_ERR_OR_NULL(edev)) {
> + uchger->extcon_dev = edev;
> + uchger->extcon_nb.nb.notifier_call = usb_charger_plug_by_extcon;
> + uchger->extcon_nb.uchger = uchger;
> + extcon_register_notifier(edev, EXTCON_USB,
> + &uchger->extcon_nb.nb);
> + }
> +
> + /* to check if the usb charger is link to a power supply */
> + psy = devm_power_supply_get_by_phandle(ugadget->dev.parent,
> +"power-supplies");
> + if (!IS_ERR_OR_NULL(psy))
> + uchger->psy = psy;
> + else
> + uchger->psy = NULL;
> +
> + /* register a notifier on a usb gadget device */
> + uchger->gadget = ugadget;
> + uchger->old_gadget_state = ugadget->state;
> +
> + /* register a new usb charger */
> + ret = usb_charger_register(&ugadget->dev, uchger);
> + if (ret)
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + if (uchger->extcon_dev)
> + extcon_unregister_notifier(uchger->extcon_dev,
> +EXTCON_USB, &uchger->extcon_nb.nb);
> +
> + kfree(uchger);
> + return ret;
> +}
> +
> +int usb_charger_exit(struct usb_gadget *ugadget)
> +{
> + return 0;
> +}
> +
> +static int __init usb_charger_class_init(void)
> +{
> + usb_charger_class = class_create(THIS_MODULE, "usb_charger");
> + if (IS_ERR(usb_charger_class)) {
> + pr_err("couldn't create class\n");
> + return PTR_ERR(usb_charger_class);
> + }
> +
> + return 0;
> +}
> +
> +static void __exit usb_charger_class_exit(void)
> +{
> + class_destroy(usb_charger_class);
> +}
> +subsys_initcall(usb_charger_class_init);
> +module_exit(usb_charger_class_exit);
> +
> +MODULE_AUTHOR("Baolin Wang ");
> +MODULE_DESCRIPTION("USB charger driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/usb/charger.h b/include/linux/usb/charger.h
> new file mode 100644
> index 000..1bf1d55
> --- /dev/null
> +++ b/include/linux/usb/charger.h
> @@ -0,0 +1,171 @@
> +#ifndef __LINUX_USB_CHARGER_H__
> +#define __LINUX_USB_CHARGER_H__
> +
> +#include
> +#include
> +
> +/* Current limitation by charger type */
> +struct usb_charger_cur_limit {
> + unsigned int sdp_cur_limit;
> + unsigned int dcp_cur_limit;
> + unsigned int cdp_cur_limit;
> + unsigned int aca_cur_limit;
> +};
> +
> +struct usb_charger_nb {
> + struct notifier_block nb;
> + struct usb_charger *uchger;
> +};
> +
> +struct usb_charger {
> + struct device dev;
> + struct raw_notifier_headuchger_nh;
> + /* protect the notifier head and charger */
> + struct mutexlock;
> + int id;
> + enum usb_charger_type type;
> + enum usb_charger_state state;
> +
> + /* for supporting extcon usb gpio */
> + struct extcon_dev *extcon_dev;
> + struct usb_charger_nb extcon_nb;
> +
> + /* for supporting usb gadget */
> + struct usb_gadget *gadget;
> +
