RE: [PATCH] usb/gadget: make composite gadget meet usb compliance for vbus draw

[Du, Changbin]

Thanks for explanation. I am agree with you that separate changes into two patches.
Will send out new patch soon.

Regards
Du, Changbin

> From: Andrzej Pietrasiewicz [mailto:andrzej.p@xxxxxxxxxxx]
> Hi,
> 
> W dniu 24.07.2015 o 06:11, Du, Changbin pisze:
> > Thanks, Pietrasiewicz.
> >> From: Andrzej Pietrasiewicz [mailto:andrzej.p@xxxxxxxxxxx]
> >> W dniu 23.07.2015 o 14:34, Du, Changbin pisze:
> >>> >From 0a8e0d63a9887735c6782d7b0c15c2c1fdf1952a Mon Sep 17
> 00:00:00
> >>>    void composite_disconnect(struct usb_gadget *gadget)
> >>>    {
> >>>    	struct usb_composite_dev	*cdev = get_gadget_data(gadget);
> >>> @@ -2095,7 +2119,7 @@ void composite_suspend(struct usb_gadget
> >> *gadget)
> >>>
> >>>    	cdev->suspended = 1;
> >>>
> >>> -	usb_gadget_vbus_draw(gadget, 2);
> >>> +	usb_gadget_vbus_draw(gadget, USB_VBUS_DRAW_SUSPEND);
> >>>    }
> >>
> >> This looks like an unrelated change. I think it should go first
> >> in a separate patch which eliminates usage of "magic" numbers.
> >>
> > This change does make sense. As you know, when device is reset, it is in a
> 'unconfigured'
> > state. Compliance Test equipment may also measure vbus current at this
> moment.
> 
> I am not questioning the change itself.
> 
> What I mean is that in my opinion it should be done in a separate patch,
> because the newly introduced USB_VBUS_DRAW_SUSPEND is not used
> anywhere else in your patch. The meaning of this change is "use a symbolic
> name rather than an explicit number" and it is unrelated to
> making composite gadget meet usb compliance for vbus draw. In other
> words,
> if you don't do this change at all the compliance is still maintained,
> because the value of USB_VBUS_DRAW_SUSPEND is 2 anyway, so what the
> compiler eventually sees is the same whether the change is made or not.
> 
> My idea:
> 
> [PATCHv2 0/2] usb gadget vbus draw compilance
>    [PATCHv2 1/2] usb: gadget: composite: avoid using a magic number
>        >> substituting an explicit "2" with USB_VBUS_DRAW_SUSPEND goes
> here <<
>    [PATCHv2 2/2] usb: gadget: composite: meet usb compliance for vbus draw
>        >> the rest of your changes go here <<
> 
> >
> >>>    }
> >>> @@ -2132,7 +2157,7 @@ static const struct usb_gadget_driver
> >> composite_driver_template = {
> >>>    	.unbind		= composite_unbind,
> >>>
> >>>    	.setup		= composite_setup,
> >>> -	.reset		= composite_disconnect,
> >>> +	.reset		= composite_reset,
> >>>    	.disconnect	= composite_disconnect,
> >>>
> >>
> >> A similar "template" is in drivers/usb/gadget/configfs.c. Shouldn't the
> "reset"
> >> method be changed there as well?
> >>
> > Yes, it also need to change. I will change it as well.
> >
> >>
> >>>
> >>> +/* USB2 compliance requires that un-configured current draw <=
> 100mA,
> >>> + * USB3 requires it <= 150mA, OTG requires it <= 2.5mA.
> >>> + */
> >>> +#define USB2_VBUS_DRAW_UNCONF		100
> >>> +#define USB3_VBUS_DRAW_UNCONF		150
> >>> +#define USB_OTG_VBUS_DRAW_UNCONF	2
> >>
> >>
> >>> +#define USB_VBUS_DRAW_SUSPEND		2
> >>
> >> separate patch
> >>
> > Sorry, I didn't get your idea. Why separate these macros definition?
> 
> Please see above.
> 
> Andrzej

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/