diff mbox series

[v1,2/2] usb: gadget: composite: Draw 100mA current if not configured

Message ID 1677129510-10283-3-git-send-email-quic_prashk@quicinc.com
State New
Headers
Series Fix vbus draw of dwc3 gadget |

Commit Message

Prashanth K Feb. 23, 2023, 5:18 a.m. UTC 
  Currently we don't change the current value if device isn't in
configured state. But the battery charging specification says,
the device can draw upto 100mA of current if its in unconfigured
state. Hence add a Vbus_draw work in composite_resume to draw
100mA if the device isn't configured.

Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
---
 drivers/usb/gadget/composite.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Jack Pham Feb. 23, 2023, 7:33 a.m. UTC | #1 
Hi Prashanth,

On Thu, Feb 23, 2023 at 10:48:30AM +0530, Prashanth K wrote:
> Currently we don't change the current value if device isn't in
> configured state. But the battery charging specification says,
> the device can draw upto 100mA of current if its in unconfigured

Here you say spec says "up to" (BTW you have a typo) 100mA...

> state. Hence add a Vbus_draw work in composite_resume to draw
> 100mA if the device isn't configured.

But here and in the patch you are calling the function to draw exactly
100mA.  Consider the possibility that a gadget could be configured to
draw less current than that or not anything at all, we should make sure
to honor that as an absolute maximum.

> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> ---
>  drivers/usb/gadget/composite.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index fa7dd6c..147d278 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2531,6 +2531,8 @@ void composite_resume(struct usb_gadget *gadget)
>  			usb_gadget_clear_selfpowered(gadget);
>  
>  		usb_gadget_vbus_draw(gadget, maxpower);
> +	} else {
> +		usb_gadget_vbus_draw(gadget, 100);

Similar to the configured case, maybe you can perform a min()
calculation against either or both the config->MaxPower or
CONFIG_USB_GADGET_VBUS_DRAW.

Thanks,
Jack
Prashanth K Feb. 23, 2023, 8:22 a.m. UTC | #2 
On 23-02-23 01:03 pm, Jack Pham wrote:
> Hi Prashanth,
> 
> On Thu, Feb 23, 2023 at 10:48:30AM +0530, Prashanth K wrote:
>> Currently we don't change the current value if device isn't in
>> configured state. But the battery charging specification says,
>> the device can draw upto 100mA of current if its in unconfigured
> 
> Here you say spec says "up to" (BTW you have a typo) 100mA...
> 
Will fix it in v2
>> state. Hence add a Vbus_draw work in composite_resume to draw
>> 100mA if the device isn't configured.
> 
> But here and in the patch you are calling the function to draw exactly
> 100mA.  Consider the possibility that a gadget could be configured to
> draw less current than that or not anything at all, we should make sure
> to honor that as an absolute maximum.
That's right
> 
>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
>> ---
>>   drivers/usb/gadget/composite.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index fa7dd6c..147d278 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -2531,6 +2531,8 @@ void composite_resume(struct usb_gadget *gadget)
>>   			usb_gadget_clear_selfpowered(gadget);
>>   
>>   		usb_gadget_vbus_draw(gadget, maxpower);
>> +	} else {
>> +		usb_gadget_vbus_draw(gadget, 100);
> 
> Similar to the configured case, maybe you can perform a min()
> calculation against either or both the config->MaxPower or
> CONFIG_USB_GADGET_VBUS_DRAW.
> 
Thanks for the suggestion, will update it in v2 patch
> Thanks,
> Jack
diff mbox series

Patch

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index fa7dd6c..147d278 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2531,6 +2531,8 @@  void composite_resume(struct usb_gadget *gadget)
 			usb_gadget_clear_selfpowered(gadget);
 
 		usb_gadget_vbus_draw(gadget, maxpower);
+	} else {
+		usb_gadget_vbus_draw(gadget, 100);
 	}
 
 	cdev->suspended = 0;