Hi Benjamin, Nestor, On Tue, 2018-03-06 at 00:31 +0100, Florent Flament wrote: > On Mon, 2018-03-05 at 18:26 +0100, Benjamin Tissoires wrote: > > Hi Florent, > > Hi Benjamin, > > > > > On Mon, Mar 5, 2018 at 10:31 AM, Nestor Lopez Casado > > <nlopezca...@logitech.com> wrote: > > > Hello Florent, > > > > > > In my view, this driver may not be a good idea. The default > > > behaviour > > > of K290 is 'send multimedia keycodes' with the user given the > > > choice > > > to change that behaviour via vendor commands. Putting a driver > > > that > > > will unconditionally change that behaviour without the user's > > > consent > > > might bother other users that prefer the multimedia keycodes by > > > default. > > > > > > Besides, I'd argue that instead of a kernel module this would be > > > best > > > achieved from a user space application. Something in the lines of > > > Solaar (github pwr/solaar) or libratbag (there's an issue open to > > > support keyboards) or even a specific application built for the > > > purpose. Anyways, please collect the input from Benjamin and Jiri > > > as > > > they as they best placed to advise than myself. > > > > On top of what Nestor said, this type of functionality, if we want > > to > > have them in the kernel should probably be integrated in > > hid-logitech-hidpp, in order not having some magic reports to send. > > > > Things like reconnect of the device would be handled far more > > easily > > in hid-logitech-hidpp while you would be reinventing the wheel > > here. > > > > One other thing I do not like in this submission of the driver is > > the > > direct use of USB while we have a full transport agnostic layer > > called > > HID. > > Fair enough, I didn't have a look at how hid-logitech-hidpp is > working > yet. I'll dig into that to see if this driver can me implemented more > elegantly.
I had a closer look at how the HID layer is interacting with the USB layer. And as far as I understand, the only way to send a message to the USB control endpoint from the HID layer is through the hid_submit_ctrl function in drivers/hid/usbhid/hid-core.c, which does this: usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir; usbhid->cr->bRequest = (dir == USB_DIR_OUT) ? HID_REQ_SET_REPORT : HID_REQ_GET_REPORT; usbhid->cr->wValue = cpu_to_le16(((report->type + 1) << 8) | report->id); usbhid->cr->wIndex = cpu_to_le16(usbhid->ifnum); usbhid->cr->wLength = cpu_to_le16(len); dbg_hid("submitting ctrl urb: %s wValue=0x%04x wIndex=0x%04x wLength=%u\n", usbhid->cr->bRequest == HID_REQ_SET_REPORT ? "Set_Report" : "Get_Report", usbhid->cr->wValue, usbhid->cr->wIndex, usbhid->cr->wLength); r = usb_submit_urb(usbhid->urbctrl, GFP_ATOMIC); While this is probably fine for most HID devices, some devices (like the Logitech K290) need to receive a vendor specific request directly adressed to the device (i.e bRequestType = USB_TYPE_VENDOR | USB_RECIPE_DEVICE). While in the hid_submit_ctrl function, the bRequestType is hardcoded to USB_TYPE_CLASS | USB_RECIP_INTERFACE. So it looks like the mechanism used by Logitech to allow switching its K290 keyboard behavior is not HID compliant and requires to forge a custom USB request. Apparently this keyboard is not the only device that requires the same kind of custom USB requests. If we look at the hid-elo driver, the same usb_control_msg calls are performed in elo_smartset_send_get: return usb_control_msg(dev, pipe, command, dir | USB_TYPE_VENDOR | USB_RECIP_DEVICE, 0, 0, data, ELO_SMARTSET_PACKET_SIZE, ELO_SMARTSET_CMD_TIMEOUT); and in elo_flush_smartset_responses: return usb_control_msg(dev, usb_sndctrlpipe(dev, 0), ELO_FLUSH_SMARTSET_RESPONSES, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, 0, 0, NULL, 0, USB_CTRL_SET_TIMEOUT); So far, I don't think that it's feasible to send the control message required to toggle the keyboard behavior from the HID layer, though I'd be glad to have your thoughts. Regards, Florent Flament