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

[Andrzej Pietrasiewicz]

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/