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;
> >
> > 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.
>
> > + 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.
- Abdulhadi Mohamed
--
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
