Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1433549 > unrolled thread

Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v12 2/4] gadget: Support for the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-06-29 10:30 +0200
    Re: [PATCH v12 2/4] gadget: Support for the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-06-29 10:40 +0200
      Re: [PATCH v12 2/4] gadget: Support for the usb charger framework Baolin Wang <baolin.wang@linaro.org> - 2016-06-29 10:40 +0200
        Re: [PATCH v12 2/4] gadget: Support for the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-06-29 14:10 +0200
          Re: [PATCH v12 2/4] gadget: Support for the usb charger framework Baolin Wang <baolin.wang@linaro.org> - 2016-06-29 14:20 +0200
            Re: [PATCH v12 2/4] gadget: Support for the usb charger framework Felipe Balbi <balbi@kernel.org> - 2016-06-29 14:40 +0200
    Re: [PATCH v12 2/4] gadget: Support for the usb charger framework Baolin Wang <baolin.wang@linaro.org> - 2016-06-29 10:40 +0200

#1433549 — Re: [PATCH v12 2/4] gadget: Support for the usb charger framework

[Multipart message — attachments visible in raw view] — view raw

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> For supporting the usb charger, it adds the usb_charger_init() and
>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>
>>> It will report to the usb charger when the gadget state is changed,
>>> then the usb charger can do the power things.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>> Tested-by: Li Jun <jun.li@nxp.com>
>>
>> Before anything, I must say that I really liked this patch. It's
>> minimaly invasive to udc core and does all the necessary changes. If it
>> wasn't for the extra charger class, this would've been perfect.
>>
>> Can't you just tie a charger to a UDC and avoid the charger class
>> completely?
>>
>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>  {
>>> +     if (gadget->charger)
>>
>> I guess you could do this check inside
>> usb_gadget_set_cur_limit_by_type() itself.
>
> We will access the 'gadget->charger->type' member when issuing
> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
> check here in next new version.

Here's what I mean:

int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
{
	struct usb_charger *charger;
        enum usb_charger_type type;

	if (!gadget->charger)
        	return 0;

        charger = gadget->charger;
        type = charger->type;

	return __usb_charger_set_cur_limit(charger, type, mA);
}

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
{
	usb_charger_set_cur_limit(gadget,  mA);

 	if (!gadget->ops->vbus_draw)
 		return -EOPNOTSUPP;
 	return gadget->ops->vbus_draw(gadget, mA);
}

-- 
balbi

[toc] | [next] | [standalone]

#1433558

[Multipart message — attachments visible in raw view] — view raw

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>
>>>>> It will report to the usb charger when the gadget state is changed,
>>>>> then the usb charger can do the power things.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>
>>>> Before anything, I must say that I really liked this patch. It's
>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>> wasn't for the extra charger class, this would've been perfect.
>>>>
>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>> completely?
>>>>
>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>  {
>>>>> +     if (gadget->charger)
>>>>
>>>> I guess you could do this check inside
>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>
>>> We will access the 'gadget->charger->type' member when issuing
>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>> check here in next new version.
>>
>> Here's what I mean:
>>
>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>> {
>>         struct usb_charger *charger;
>>         enum usb_charger_type type;
>>
>>         if (!gadget->charger)
>>                 return 0;
>>
>>         charger = gadget->charger;
>>         type = charger->type;
>>
>>         return __usb_charger_set_cur_limit(charger, type, mA);
>> }
>
> But that means we need to export  both 'usb_charger_set_cur_limit()'
> function and '__usb_charger_set_cur_limit()' function in charger.c
> file. Cause some user may want to set the current limitation by one
> charger type parameter (may be not from charger->type), like by
> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
> you think about this situation? Thanks.

if we have that requirement, that's totally fine. Just rename
__usb_charger_set_cur_limit() back to
_usb_charger_set_cur_limit_by_type() and expose both. But
set_cur_limit_by_type can assume its arguments are valid at all times.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1433559

On 29 June 2016 at 16:34, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>>
>>>>>> It will report to the usb charger when the gadget state is changed,
>>>>>> then the usb charger can do the power things.
>>>>>>
>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>>
>>>>> Before anything, I must say that I really liked this patch. It's
>>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>>> wasn't for the extra charger class, this would've been perfect.
>>>>>
>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>> completely?
>>>>>
>>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>>  {
>>>>>> +     if (gadget->charger)
>>>>>
>>>>> I guess you could do this check inside
>>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>>
>>>> We will access the 'gadget->charger->type' member when issuing
>>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>>> check here in next new version.
>>>
>>> Here's what I mean:
>>>
>>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>>> {
>>>         struct usb_charger *charger;
>>>         enum usb_charger_type type;
>>>
>>>         if (!gadget->charger)
>>>                 return 0;
>>>
>>>         charger = gadget->charger;
>>>         type = charger->type;
>>>
>>>         return __usb_charger_set_cur_limit(charger, type, mA);
>>> }
>>
>> But that means we need to export  both 'usb_charger_set_cur_limit()'
>> function and '__usb_charger_set_cur_limit()' function in charger.c
>> file. Cause some user may want to set the current limitation by one
>> charger type parameter (may be not from charger->type), like by
>> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
>> you think about this situation? Thanks.
>
> if we have that requirement, that's totally fine. Just rename
> __usb_charger_set_cur_limit() back to
> _usb_charger_set_cur_limit_by_type() and expose both. But
> set_cur_limit_by_type can assume its arguments are valid at all times.

Make sense. I'll fix this issue in v14 version. Thanks.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1433698

[Multipart message — attachments visible in raw view] — view raw

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>>>
>>>>>>> It will report to the usb charger when the gadget state is changed,
>>>>>>> then the usb charger can do the power things.
>>>>>>>
>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>>>
>>>>>> Before anything, I must say that I really liked this patch. It's
>>>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>>>> wasn't for the extra charger class, this would've been perfect.
>>>>>>
>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>> completely?
>>>>>>
>>>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>>>  {
>>>>>>> +     if (gadget->charger)
>>>>>>
>>>>>> I guess you could do this check inside
>>>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>>>
>>>>> We will access the 'gadget->charger->type' member when issuing
>>>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>>>> check here in next new version.
>>>>
>>>> Here's what I mean:
>>>>
>>>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>>>> {
>>>>         struct usb_charger *charger;
>>>>         enum usb_charger_type type;
>>>>
>>>>         if (!gadget->charger)
>>>>                 return 0;
>>>>
>>>>         charger = gadget->charger;
>>>>         type = charger->type;
>>>>
>>>>         return __usb_charger_set_cur_limit(charger, type, mA);
>>>> }
>>>
>>> But that means we need to export  both 'usb_charger_set_cur_limit()'
>>> function and '__usb_charger_set_cur_limit()' function in charger.c
>>> file. Cause some user may want to set the current limitation by one
>>> charger type parameter (may be not from charger->type), like by
>>> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
>>> you think about this situation? Thanks.
>>
>> if we have that requirement, that's totally fine. Just rename
>> __usb_charger_set_cur_limit() back to
>> _usb_charger_set_cur_limit_by_type() and expose both. But
>> set_cur_limit_by_type can assume its arguments are valid at all times.
>
> Make sense. I'll fix this issue in v14 version. Thanks.

there's one thing bothering me though:

gadget->charger is supposed to be "current detected charger" right? If
we have a single port tied to a single charger, in which case would we
*ever* need to change current limit of any charger type other than
"current charger" ?

IOW, why do we need _set_cur_limit_by_type() exported at all?

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1433700

Hi Felipe,

On 29 June 2016 at 20:06, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>>>>
>>>>>>>> It will report to the usb charger when the gadget state is changed,
>>>>>>>> then the usb charger can do the power things.
>>>>>>>>
>>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>>>>
>>>>>>> Before anything, I must say that I really liked this patch. It's
>>>>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>>>>> wasn't for the extra charger class, this would've been perfect.
>>>>>>>
>>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>>> completely?
>>>>>>>
>>>>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>>>>  {
>>>>>>>> +     if (gadget->charger)
>>>>>>>
>>>>>>> I guess you could do this check inside
>>>>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>>>>
>>>>>> We will access the 'gadget->charger->type' member when issuing
>>>>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>>>>> check here in next new version.
>>>>>
>>>>> Here's what I mean:
>>>>>
>>>>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>>>>> {
>>>>>         struct usb_charger *charger;
>>>>>         enum usb_charger_type type;
>>>>>
>>>>>         if (!gadget->charger)
>>>>>                 return 0;
>>>>>
>>>>>         charger = gadget->charger;
>>>>>         type = charger->type;
>>>>>
>>>>>         return __usb_charger_set_cur_limit(charger, type, mA);
>>>>> }
>>>>
>>>> But that means we need to export  both 'usb_charger_set_cur_limit()'
>>>> function and '__usb_charger_set_cur_limit()' function in charger.c
>>>> file. Cause some user may want to set the current limitation by one
>>>> charger type parameter (may be not from charger->type), like by
>>>> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
>>>> you think about this situation? Thanks.
>>>
>>> if we have that requirement, that's totally fine. Just rename
>>> __usb_charger_set_cur_limit() back to
>>> _usb_charger_set_cur_limit_by_type() and expose both. But
>>> set_cur_limit_by_type can assume its arguments are valid at all times.
>>
>> Make sense. I'll fix this issue in v14 version. Thanks.
>
> there's one thing bothering me though:
>
> gadget->charger is supposed to be "current detected charger" right? If
> we have a single port tied to a single charger, in which case would we
> *ever* need to change current limit of any charger type other than
> "current charger" ?

What I mean is user can set the current limit by charger type as what
they want at initial stage. As we know the default current of SDP (not
super speed) is 500 mA, but user can set the current limit of SDP as
450 mA if there are some special on their own platform by issuing
'__usb_charger_set_cur_limit(charger, SDP_TYPE, 450)'.

>
> IOW, why do we need _set_cur_limit_by_type() exported at all?
>
> --
> balbi



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1433707

[Multipart message — attachments visible in raw view] — view raw

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>>>>>
>>>>>>>>> It will report to the usb charger when the gadget state is changed,
>>>>>>>>> then the usb charger can do the power things.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>>>>>
>>>>>>>> Before anything, I must say that I really liked this patch. It's
>>>>>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>>>>>> wasn't for the extra charger class, this would've been perfect.
>>>>>>>>
>>>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>>>> completely?
>>>>>>>>
>>>>>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>>>>>  {
>>>>>>>>> +     if (gadget->charger)
>>>>>>>>
>>>>>>>> I guess you could do this check inside
>>>>>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>>>>>
>>>>>>> We will access the 'gadget->charger->type' member when issuing
>>>>>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>>>>>> check here in next new version.
>>>>>>
>>>>>> Here's what I mean:
>>>>>>
>>>>>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>>>>>> {
>>>>>>         struct usb_charger *charger;
>>>>>>         enum usb_charger_type type;
>>>>>>
>>>>>>         if (!gadget->charger)
>>>>>>                 return 0;
>>>>>>
>>>>>>         charger = gadget->charger;
>>>>>>         type = charger->type;
>>>>>>
>>>>>>         return __usb_charger_set_cur_limit(charger, type, mA);
>>>>>> }
>>>>>
>>>>> But that means we need to export  both 'usb_charger_set_cur_limit()'
>>>>> function and '__usb_charger_set_cur_limit()' function in charger.c
>>>>> file. Cause some user may want to set the current limitation by one
>>>>> charger type parameter (may be not from charger->type), like by
>>>>> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
>>>>> you think about this situation? Thanks.
>>>>
>>>> if we have that requirement, that's totally fine. Just rename
>>>> __usb_charger_set_cur_limit() back to
>>>> _usb_charger_set_cur_limit_by_type() and expose both. But
>>>> set_cur_limit_by_type can assume its arguments are valid at all times.
>>>
>>> Make sense. I'll fix this issue in v14 version. Thanks.
>>
>> there's one thing bothering me though:
>>
>> gadget->charger is supposed to be "current detected charger" right? If
>> we have a single port tied to a single charger, in which case would we
>> *ever* need to change current limit of any charger type other than
>> "current charger" ?
>
> What I mean is user can set the current limit by charger type as what
> they want at initial stage. As we know the default current of SDP (not
> super speed) is 500 mA, but user can set the current limit of SDP as
> 450 mA if there are some special on their own platform by issuing
> '__usb_charger_set_cur_limit(charger, SDP_TYPE, 450)'.

Oh I see. Odd, but okay

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1433566

Hi Felipe,

On 29 June 2016 at 16:20, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>
>>>> It will report to the usb charger when the gadget state is changed,
>>>> then the usb charger can do the power things.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>
>>> Before anything, I must say that I really liked this patch. It's
>>> minimaly invasive to udc core and does all the necessary changes. If it
>>> wasn't for the extra charger class, this would've been perfect.
>>>
>>> Can't you just tie a charger to a UDC and avoid the charger class
>>> completely?
>>>
>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>  {
>>>> +     if (gadget->charger)
>>>
>>> I guess you could do this check inside
>>> usb_gadget_set_cur_limit_by_type() itself.
>>
>> We will access the 'gadget->charger->type' member when issuing
>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>> check here in next new version.
>
> Here's what I mean:
>
> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
> {
>         struct usb_charger *charger;
>         enum usb_charger_type type;
>
>         if (!gadget->charger)
>                 return 0;
>
>         charger = gadget->charger;
>         type = charger->type;
>
>         return __usb_charger_set_cur_limit(charger, type, mA);
> }

But that means we need to export  both 'usb_charger_set_cur_limit()'
function and '__usb_charger_set_cur_limit()' function in charger.c
file. Cause some user may want to set the current limitation by one
charger type parameter (may be not from charger->type), like by
issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
you think about this situation? Thanks.

>
> static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
> {
>         usb_charger_set_cur_limit(gadget,  mA);
>
>         if (!gadget->ops->vbus_draw)
>                 return -EOPNOTSUPP;
>         return gadget->ops->vbus_draw(gadget, mA);
> }
>
> --
> balbi



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web