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

[Felipe Balbi]

Hi,

Baolin Wang  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 
> Reviewed-by: Li Jun 
> Tested-by: Li Jun 

 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


signature.asc
Description: PGP signature

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

[Baolin Wang]

Hi Felipe,

On 29 June 2016 at 20:06, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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 
 Reviewed-by: Li Jun 
 Tested-by: Li Jun 
>>>
>>> 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
--
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 v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  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 
>>> Reviewed-by: Li Jun 
>>> Tested-by: Li Jun 
>>
>> 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


signature.asc
Description: PGP signature

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

[Baolin Wang]

On 29 June 2016 at 16:34, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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 
>> Reviewed-by: Li Jun 
>> Tested-by: Li Jun 
>
> 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
--
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 v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  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 
> Reviewed-by: Li Jun 
> Tested-by: Li Jun 

 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


signature.asc
Description: PGP signature

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

[Baolin Wang]

Hi Felipe,

On 29 June 2016 at 16:20, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>>> Baolin Wang  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 
 Reviewed-by: Li Jun 
 Tested-by: Li Jun 
>>>
>>> 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
--
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 v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  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 
>>> Reviewed-by: Li Jun 
>>> Tested-by: Li Jun 
>>
>> 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


signature.asc
Description: PGP signature

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

[Baolin Wang]

Hi,

On 21 June 2016 at 18:27, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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 
>> Reviewed-by: Li Jun 
>> Tested-by: Li Jun 
>
> 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.

-- 
Baolin.wang
Best Regards
--
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 v12 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 21 June 2016 at 20:53, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
> Can't you just tie a charger to a UDC and avoid the charger class
> completely?

 Yeah, I also hope so. But we really want something to manage the
 charger devices, do you have any good suggestion to avoid the 'class'
 but also can manage the charger devices?
>>>
>>> manage in what way? It seems to me that they don't need to be real
>>> devices, just a handle as part of struct usb_gadget, no?
>>
>> Although charger device is not one real hardware device, we also use
>> one 'struct device' to describe it in charger.c file. So we should
>> manage the 'struct device' with one proper way.
>
> that's fine, but why do you think they need a struct device to start with?

 We can get/put usb charger and mange usb charger attributes with the
 device model if we use a struct device.
>>>
>>> We already have that as part of struct usb_udc. Why don't you just
>>> create a subdirectory called charger which will hold all your
>>> charger-related attributes. That directory will only be created if a
>>> valid ->charger pointer exists.
>>
>> That means we can remove all the device and class things in charger.c
>> file, right? OK, I try to do that. Thanks.
>
> right. Keep your charger.c file, because to conditionally compile and
> link that to udc-core.ko, but remove all the class initialization and
> all of that extra code.

Make sense.

-- 
Baolin.wang
Best Regards
--
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 v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Can't you just tie a charger to a UDC and avoid the charger class
>> completely?
>
> Yeah, I also hope so. But we really want something to manage the
> charger devices, do you have any good suggestion to avoid the 'class'
> but also can manage the charger devices?

manage in what way? It seems to me that they don't need to be real
devices, just a handle as part of struct usb_gadget, no?

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Felipe Balbi  writes:
> Can't you just tie a charger to a UDC and avoid the charger class
> completely?

 Yeah, I also hope so. But we really want something to manage the
 charger devices, do you have any good suggestion to avoid the 'class'
 but also can manage the charger devices?
>>>
>>> manage in what way? It seems to me that they don't need to be real
>>> devices, just a handle as part of struct usb_gadget, no?
>>
>> Although charger device is not one real hardware device, we also use
>> one 'struct device' to describe it in charger.c file. So we should
>> manage the 'struct device' with one proper way.
>
> that's fine, but why do you think they need a struct device to start with?

 We can get/put usb charger and mange usb charger attributes with the
 device model if we use a struct device.
>>>
>>> We already have that as part of struct usb_udc. Why don't you just
>>> create a subdirectory called charger which will hold all your
>>> charger-related attributes. That directory will only be created if a
>>> valid ->charger pointer exists.
>>
>> That means we can remove all the device and class things in charger.c
>> file, right? OK, I try to do that. Thanks.
>
> right. Keep your charger.c file, because to conditionally compile and
  ^
   we want   

> link that to udc-core.ko, but remove all the class initialization and
> all of that extra code.
>
> -- 
> balbi

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
 Can't you just tie a charger to a UDC and avoid the charger class
 completely?
>>>
>>> Yeah, I also hope so. But we really want something to manage the
>>> charger devices, do you have any good suggestion to avoid the 'class'
>>> but also can manage the charger devices?
>>
>> manage in what way? It seems to me that they don't need to be real
>> devices, just a handle as part of struct usb_gadget, no?
>
> Although charger device is not one real hardware device, we also use
> one 'struct device' to describe it in charger.c file. So we should
> manage the 'struct device' with one proper way.

 that's fine, but why do you think they need a struct device to start with?
>>>
>>> We can get/put usb charger and mange usb charger attributes with the
>>> device model if we use a struct device.
>>
>> We already have that as part of struct usb_udc. Why don't you just
>> create a subdirectory called charger which will hold all your
>> charger-related attributes. That directory will only be created if a
>> valid ->charger pointer exists.
>
> That means we can remove all the device and class things in charger.c
> file, right? OK, I try to do that. Thanks.

right. Keep your charger.c file, because to conditionally compile and
link that to udc-core.ko, but remove all the class initialization and
all of that extra code.

-- 
balbi


signature.asc
Description: PGP signature

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

[Baolin Wang]

On 21 June 2016 at 20:36, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>>> Baolin Wang  writes:
> Baolin Wang  writes:
>>> Can't you just tie a charger to a UDC and avoid the charger class
>>> completely?
>>
>> Yeah, I also hope so. But we really want something to manage the
>> charger devices, do you have any good suggestion to avoid the 'class'
>> but also can manage the charger devices?
>
> manage in what way? It seems to me that they don't need to be real
> devices, just a handle as part of struct usb_gadget, no?

 Although charger device is not one real hardware device, we also use
 one 'struct device' to describe it in charger.c file. So we should
 manage the 'struct device' with one proper way.
>>>
>>> that's fine, but why do you think they need a struct device to start with?
>>
>> We can get/put usb charger and mange usb charger attributes with the
>> device model if we use a struct device.
>
> We already have that as part of struct usb_udc. Why don't you just
> create a subdirectory called charger which will hold all your
> charger-related attributes. That directory will only be created if a
> valid ->charger pointer exists.

That means we can remove all the device and class things in charger.c
file, right? OK, I try to do that. Thanks.

>
> USB Charging is always tied to a peripheral side controller, anyway.
>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
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 v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  writes:
 Can't you just tie a charger to a UDC and avoid the charger class
 completely?
>>>
>>> Yeah, I also hope so. But we really want something to manage the
>>> charger devices, do you have any good suggestion to avoid the 'class'
>>> but also can manage the charger devices?
>>
>> manage in what way? It seems to me that they don't need to be real
>> devices, just a handle as part of struct usb_gadget, no?
>
> Although charger device is not one real hardware device, we also use
> one 'struct device' to describe it in charger.c file. So we should
> manage the 'struct device' with one proper way.

that's fine, but why do you think they need a struct device to start with?

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  writes:
 Baolin Wang  writes:
>> Can't you just tie a charger to a UDC and avoid the charger class
>> completely?
>
> Yeah, I also hope so. But we really want something to manage the
> charger devices, do you have any good suggestion to avoid the 'class'
> but also can manage the charger devices?

 manage in what way? It seems to me that they don't need to be real
 devices, just a handle as part of struct usb_gadget, no?
>>>
>>> Although charger device is not one real hardware device, we also use
>>> one 'struct device' to describe it in charger.c file. So we should
>>> manage the 'struct device' with one proper way.
>>
>> that's fine, but why do you think they need a struct device to start with?
>
> We can get/put usb charger and mange usb charger attributes with the
> device model if we use a struct device.

We already have that as part of struct usb_udc. Why don't you just
create a subdirectory called charger which will hold all your
charger-related attributes. That directory will only be created if a
valid ->charger pointer exists.

USB Charging is always tied to a peripheral side controller, anyway.

-- 
balbi


signature.asc
Description: PGP signature

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

[Baolin Wang]

On 21 June 2016 at 20:27, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>>> Baolin Wang  writes:
> Can't you just tie a charger to a UDC and avoid the charger class
> completely?

 Yeah, I also hope so. But we really want something to manage the
 charger devices, do you have any good suggestion to avoid the 'class'
 but also can manage the charger devices?
>>>
>>> manage in what way? It seems to me that they don't need to be real
>>> devices, just a handle as part of struct usb_gadget, no?
>>
>> Although charger device is not one real hardware device, we also use
>> one 'struct device' to describe it in charger.c file. So we should
>> manage the 'struct device' with one proper way.
>
> that's fine, but why do you think they need a struct device to start with?

We can get/put usb charger and mange usb charger attributes with the
device model if we use a struct device.

-- 
Baolin.wang
Best Regards
--
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 v12 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 21 June 2016 at 19:49, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>>> Can't you just tie a charger to a UDC and avoid the charger class
>>> completely?
>>
>> Yeah, I also hope so. But we really want something to manage the
>> charger devices, do you have any good suggestion to avoid the 'class'
>> but also can manage the charger devices?
>
> manage in what way? It seems to me that they don't need to be real
> devices, just a handle as part of struct usb_gadget, no?

Although charger device is not one real hardware device, we also use
one 'struct device' to describe it in charger.c file. So we should
manage the 'struct device' with one proper way.

-- 
Baolin.wang
Best Regards
--
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 v12 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 21 June 2016 at 18:27, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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 
>> Reviewed-by: Li Jun 
>> Tested-by: Li Jun 
>
> Before anything, I must say that I really liked this patch. It's

Thanks.

> 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?

Yeah, I also hope so. But we really want something to manage the
charger devices, do you have any good suggestion to avoid the 'class'
but also can manage the charger devices?

>
>>  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.

OK.

>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
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 v12 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang  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 
> Reviewed-by: Li Jun 
> Tested-by: Li Jun 

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.

-- 
balbi


signature.asc
Description: PGP signature

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

[Baolin Wang]

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 
Reviewed-by: Li Jun 
Tested-by: Li Jun 
---
 drivers/usb/gadget/udc/udc-core.c |   11 +++
 include/linux/usb/gadget.h|   11 +++
 2 files changed, 22 insertions(+)

diff --git a/drivers/usb/gadget/udc/udc-core.c 
b/drivers/usb/gadget/udc/udc-core.c
index 6e8300d..84c098c 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -242,6 +243,9 @@ static void usb_gadget_state_work(struct work_struct *work)
struct usb_gadget *gadget = work_to_gadget(work);
struct usb_udc *udc = gadget->udc;
 
+   /* when the gadget state is changed, then report to USB charger */
+   usb_charger_plug_by_gadget(gadget, gadget->state);
+
if (udc)
sysfs_notify(>dev.kobj, NULL, "state");
 }
@@ -411,6 +415,10 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
if (ret)
goto err4;
 
+   ret = usb_charger_init(gadget);
+   if (ret)
+   goto err5;
+
usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
udc->vbus = true;
 
@@ -431,6 +439,8 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
 
return 0;
 
+err5:
+   device_del(>dev);
 err4:
list_del(>list);
mutex_unlock(_lock);
@@ -539,6 +549,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
kobject_uevent(>dev.kobj, KOBJ_REMOVE);
flush_work(>work);
device_unregister(>dev);
+   usb_charger_exit(gadget);
device_unregister(>dev);
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 457651b..40390ec 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct usb_ep;
 
@@ -639,6 +640,8 @@ struct usb_gadget {
unsignedout_epnum;
unsignedin_epnum;
struct usb_otg_caps *otg_caps;
+   /* negotiate the power with the usb charger */
+   struct usb_charger  *charger;
 
unsignedsg_supported:1;
unsignedis_otg:1;
@@ -855,10 +858,18 @@ static inline int usb_gadget_vbus_connect(struct 
usb_gadget *gadget)
  * reporting how much power the device may consume.  For example, this
  * could affect how quickly batteries are recharged.
  *
+ * It will also notify the USB charger how much power the device may
+ * consume if there is a USB charger linking with the gadget.
+ *
  * Returns zero on success, else negative errno.
  */
 static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 {
+   if (gadget->charger)
+   usb_charger_set_cur_limit_by_type(gadget->charger,
+ gadget->charger->type,
+ mA);
+
if (!gadget->ops->vbus_draw)
return -EOPNOTSUPP;
return gadget->ops->vbus_draw(gadget, mA);
-- 
1.7.9.5

--
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