On Wed, 21 Jun 2017 abdulahhadi2@xxxxxxxxx wrote:
> Hi,
>
> Thanks for the reply! I'll be sure to send in another patch
> with fixes to all the formatting errors. But before that
> just a couple of points on your feedback.
>
> > > struct usb_request *req;
> > > unsigned long flags;
> > > ssize_t status = -ENOMEM;
> > >
> > > + usb_gadget_wakeup(cdev->gadget);
> >
> > this seems unrelated to implemeting GET/SET_PROTOCOL. Why do you need this?
>
> Right, I intially suspected support for these two features
> were dependant on one another. However, that does not seem to be the case.
> I'll send in a seperate patch to handle remote wake up support in
> the driver later.
>
> >
> > > @@ -528,6 +532,9 @@ static int hidg_setup(struct usb_function *f,
> > > | HID_REQ_GET_PROTOCOL):
> > > VDBG(cdev, "get_protocol\n");
> > > goto stall;
> > > + length = min_t(unsigned, length, 1);
> > > + ((u8 *) req->buf)[0] = hidg->protocol_is_report;
> > > + goto respond;
> > > break;
Felipe didn't mention this, but the lines you added here are dead code
because you forgot to remove the "goto stall".
Also, you have a number of excess space characters before the second
'=' sign.
> > >
> > > case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
> > > @@ -539,6 +546,17 @@ static int hidg_setup(struct usb_function *f,
> > > case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
> > > | HID_REQ_SET_PROTOCOL):
> > > VDBG(cdev, "set_protocol\n");
> > > + if (value > 1)
> >
> > why 1 here? Seems like this should be
> > USB_INTERFACE_PROTOCOL_KEYBOARD. And are you sure there's nobody using
> > this to emulate a mouse?
>
> According to 7.2.6 of the HID spec. For the wValue field, 0 indicates Boot
> Protocol and 1 indicates Report Protocol, which applies to both mice
> and keyboards. So the use of that macro wouldn't make sense.
>
> > > + goto stall;
> > > + length = 0;
> > > + /*
> > > + * We assume that programs implementing the Boot protocol
> > > + * are also compatible with the Report protocol.
> > > + */
> >
> > Why is this a safe assumption?
>
> Any device in the Boot subclass supports both Report and Boot protocols by
> definition according to the HID spec. Although the implementations of
> these two procotols maybe the same in some cases, there is a possbility
> that they are different. In this case it would pose a problem to the
> current driver, which offers no switching capabiliy via SET_PROTOCOL while
> in BIOS.
Also, if the assumption turns out to be wrong, there's nothing you can
do about it. :-)
> > > + if(hidg->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) {
> > > + hidg->protocol_is_report = value;
> > > + goto respond;
> >
> > wrong indentation.
> >
> > > @@ -768,6 +786,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
> > > /* set descriptor dynamic values */
> > > hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
> > > hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
> > > + hidg->protocol_is_report = 1;
> >
> > no idea why you called this "protocol_is_report" when "protocol"
> > would've been better.
>
> True, that name would probably make it more concise.
I originally called it "protocol_is_report" to improve readability for
things like:
if (protocol_is_report) ...
as opposed to:
if (protocol == 1)
But with new macros for HID_BOOT_PROTOCOL and HID_REPORT_PROTOCOL,
either way would be acceptable.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
