On Sat, Apr 20, 2019 at 12:34 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 4/19/19 5:54 PM, Benjamin Tissoires wrote:
> > On Wed, Apr 10, 2019 at 4:55 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Most Logitech wireless keyboard and mice using the 27 MHz are hidpp10
> >> devices, add support to logitech-dj for their receivers.
> >>
> >> Doing so leads to 2 improvements:
> >>
> >> 1) All these devices share the same USB product-id for their receiver,
> >> making it impossible to properly map some special keys / buttons
> >> which differ from device to device. Adding support to logitech-dj to
> >> see these as hidpp10 devices allows us to get the actual device-id
> >> from the keyboard / mouse.
> >>
> >> 2) It enables battery-monitoring of these devices
> >>
> >> This patch uses a new HID group for 27Mhz devices, since the logitech-hidpp
> >> code needs to be able to differentiate them from other devices instantiated
> >> by the logitech-dj code.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> Changes in v2:
> >> -Remove chunk to pick a better name for hidpp devices, this is replaced
> >> with a more generic fix in a separate patch
> >> -Add support for mouse on index 2, numeric-keypad on index 4
> >> -Use a new group for 27MHz, so that logitech-hidpp.c can check for this to
> >> enable 27MHz specific behavior, rather then have it guess based on the
> >> descriptors. This also allows removing some 27MHz descriptor hacks from
> >> logitech-dj.c, so it simplifies things on both ends
> >> ---
> >> drivers/hid/hid-lg.c | 2 -
> >> drivers/hid/hid-logitech-dj.c | 94 ++++++++++++++++++++++++++++++++++-
> >> drivers/hid/hid-quirks.c | 1 -
> >> include/linux/hid.h | 1 +
> >> 4 files changed, 94 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
> >> index 5d419a95b6c2..36d725fdb199 100644
> >> --- a/drivers/hid/hid-lg.c
> >> +++ b/drivers/hid/hid-lg.c
> >> @@ -876,8 +876,6 @@ static const struct hid_device_id lg_devices[] = {
> >> .driver_data = LG_RDESC | LG_WIRELESS },
> >> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER),
> >> .driver_data = LG_RDESC | LG_WIRELESS },
> >> - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2),
> >> - .driver_data = LG_RDESC | LG_WIRELESS },
> >>
> >> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RECEIVER),
> >> .driver_data = LG_BAD_RELATIVE_KEYS },
> >> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> >> index a994d2e3dd9e..9d5db242e326 100644
> >> --- a/drivers/hid/hid-logitech-dj.c
> >> +++ b/drivers/hid/hid-logitech-dj.c
> >> @@ -106,6 +106,7 @@
> >> #define HIDPP_PARAM_DEVICE_INFO 0x01
> >> #define HIDPP_PARAM_EQUAD_LSB 0x02
> >> #define HIDPP_PARAM_EQUAD_MSB 0x03
> >> +#define HIDPP_PARAM_27MHZ_DEVID 0x03
> >> #define HIDPP_DEVICE_TYPE_MASK GENMASK(3, 0)
> >> #define HIDPP_LINK_STATUS_MASK BIT(6)
> >>
> >> @@ -120,6 +121,7 @@ enum recvr_type {
> >> recvr_type_dj,
> >> recvr_type_hidpp,
> >> recvr_type_gaming_hidpp,
> >> + recvr_type_27mhz,
> >> };
> >>
> >> struct dj_report {
> >> @@ -248,6 +250,44 @@ static const char mse_descriptor[] = {
> >> 0xC0, /* END_COLLECTION */
> >> };
> >>
> >> +/* Mouse descriptor (2) for 27 MHz receiver, only 8 buttons */
> >> +static const char mse_27mhz_descriptor[] = {
> >> + 0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */
> >> + 0x09, 0x02, /* USAGE (Mouse) */
> >> + 0xA1, 0x01, /* COLLECTION (Application) */
> >> + 0x85, 0x02, /* REPORT_ID = 2 */
> >> + 0x09, 0x01, /* USAGE (pointer) */
> >> + 0xA1, 0x00, /* COLLECTION (physical) */
> >> + 0x05, 0x09, /* USAGE_PAGE (buttons) */
> >> + 0x19, 0x01, /* USAGE_MIN (1) */
> >> + 0x29, 0x08, /* USAGE_MAX (8) */
> >> + 0x15, 0x00, /* LOGICAL_MIN (0) */
> >> + 0x25, 0x01, /* LOGICAL_MAX (1) */
> >> + 0x95, 0x08, /* REPORT_COUNT (8) */
> >> + 0x75, 0x01, /* REPORT_SIZE (1) */
> >> + 0x81, 0x02, /* INPUT (data var abs) */
> >> + 0x05, 0x01, /* USAGE_PAGE (generic desktop) */
> >> + 0x16, 0x01, 0xF8, /* LOGICAL_MIN (-2047) */
> >> + 0x26, 0xFF, 0x07, /* LOGICAL_MAX (2047) */
> >> + 0x75, 0x0C, /* REPORT_SIZE (12) */
> >> + 0x95, 0x02, /* REPORT_COUNT (2) */
> >> + 0x09, 0x30, /* USAGE (X) */
> >> + 0x09, 0x31, /* USAGE (Y) */
> >> + 0x81, 0x06, /* INPUT */
> >> + 0x15, 0x81, /* LOGICAL_MIN (-127) */
> >> + 0x25, 0x7F, /* LOGICAL_MAX (127) */
> >> + 0x75, 0x08, /* REPORT_SIZE (8) */
> >> + 0x95, 0x01, /* REPORT_COUNT (1) */
> >> + 0x09, 0x38, /* USAGE (wheel) */
> >> + 0x81, 0x06, /* INPUT */
> >> + 0x05, 0x0C, /* USAGE_PAGE(consumer) */
> >> + 0x0A, 0x38, 0x02, /* USAGE(AC Pan) */
> >> + 0x95, 0x01, /* REPORT_COUNT (1) */
> >> + 0x81, 0x06, /* INPUT */
> >> + 0xC0, /* END_COLLECTION */
> >> + 0xC0, /* END_COLLECTION */
> >> +};
> >> +
> >> /* Gaming Mouse descriptor (2) */
> >> static const char mse_high_res_descriptor[] = {
> >> 0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */
> >> @@ -596,7 +636,10 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
> >> "Logitech Unifying Device. Wireless PID:%04x",
> >> dj_hiddev->product);
> >>
> >> - dj_hiddev->group = HID_GROUP_LOGITECH_DJ_DEVICE;
> >> + if (djrcv_dev->type == recvr_type_27mhz)
> >> + dj_hiddev->group = HID_GROUP_LOGITECH_27MHZ_DEVICE;
> >> + else
> >> + dj_hiddev->group = HID_GROUP_LOGITECH_DJ_DEVICE;
> >>
> >> memcpy(dj_hiddev->phys, djrcv_hdev->phys, sizeof(djrcv_hdev->phys));
> >> snprintf(tmpstr, sizeof(tmpstr), ":%d", device_index);
> >> @@ -782,6 +825,28 @@ static void logi_hidpp_dev_conn_notif_equad(struct hidpp_event *hidpp_report,
> >> }
> >> }
> >>
> >> +static void logi_hidpp_dev_conn_notif_27mhz(struct hid_device *hdev,
> >> + struct hidpp_event *hidpp_report,
> >> + struct dj_workitem *workitem)
> >> +{
> >> + workitem->type = WORKITEM_TYPE_PAIRED;
> >> + workitem->quad_id_lsb = hidpp_report->params[HIDPP_PARAM_27MHZ_DEVID];
> >> + switch (hidpp_report->device_index) {
> >> + case 1: /* Index 1 is always a mouse */
> >
> > IIRC we are supposed to mark the fall through statements as such. Not
> > sure why checkpatch doesn't complain here.
>
> Using multiple labels behind one each other without any statements in
> between is a common designpattern and -Wimplicit-fallthrough allows this
> without warning. I've added a patch to my personal tree to enable
> -Wimplicit-fallthrough to make sure that I'm not introducing new warnings:
> https://github.com/jwrdegoede/linux-sunxi/commit/c10090f844bbcb8c646ca9afec269c8242526255
>
> So adding fall through comments to consecutive case labels without
> any statements in between is not necessary and is quite ugly IMHO.
OK, that's good news. I also thought this would be ugly but I do not
like receiving a message from a bot telling me we need this right
after I applied a patch.
Cheers,
Benjamin
>
> Regards,
>
> Hans
>
>
>
> >
> >> + case 2: /* Index 2 is always a mouse */
> >> + workitem->reports_supported |= STD_MOUSE;
> >> + break;
> >> + case 3: /* Index 3 is always the keyboard */
> >
> > Same here, please mark this as a fall-through.
> >
> > Cheers,
> > Benjamin
> >
> >> + case 4: /* Index 4 is used for an optional separate numpad */
> >> + workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA |
> >> + POWER_KEYS;
> >> + break;
> >> + default:
> >> + hid_warn(hdev, "%s: unexpected device-index %d", __func__,
> >> + hidpp_report->device_index);
> >> + }
> >> +}
> >> +
> >> static void logi_hidpp_recv_queue_notif(struct hid_device *hdev,
> >> struct hidpp_event *hidpp_report)
> >> {
> >> @@ -799,6 +864,7 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev,
> >> break;
> >> case 0x02:
> >> device_type = "27 Mhz";
> >> + logi_hidpp_dev_conn_notif_27mhz(hdev, hidpp_report, &workitem);
> >> break;
> >> case 0x03:
> >> device_type = "QUAD or eQUAD";
> >> @@ -1186,6 +1252,9 @@ static int logi_dj_ll_parse(struct hid_device *hid)
> >> if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp)
> >> rdcat(rdesc, &rsize, mse_high_res_descriptor,
> >> sizeof(mse_high_res_descriptor));
> >> + else if (djdev->dj_receiver_dev->type == recvr_type_27mhz)
> >> + rdcat(rdesc, &rsize, mse_27mhz_descriptor,
> >> + sizeof(mse_27mhz_descriptor));
> >> else
> >> rdcat(rdesc, &rsize, mse_descriptor,
> >> sizeof(mse_descriptor));
> >> @@ -1357,6 +1426,25 @@ static int logi_dj_hidpp_event(struct hid_device *hdev,
> >> spin_lock_irqsave(&djrcv_dev->lock, flags);
> >>
> >> dj_dev = djrcv_dev->paired_dj_devices[device_index];
> >> +
> >> + /*
> >> + * With 27 MHz receivers, we do not get an explicit unpair event,
> >> + * remove the old device if the user has paired a *different* device.
> >> + */
> >> + if (djrcv_dev->type == recvr_type_27mhz && dj_dev &&
> >> + hidpp_report->sub_id == REPORT_TYPE_NOTIF_DEVICE_CONNECTED &&
> >> + hidpp_report->params[HIDPP_PARAM_PROTO_TYPE] == 0x02 &&
> >> + hidpp_report->params[HIDPP_PARAM_27MHZ_DEVID] !=
> >> + dj_dev->hdev->product) {
> >> + struct dj_workitem workitem = {
> >> + .device_index = hidpp_report->device_index,
> >> + .type = WORKITEM_TYPE_UNPAIRED,
> >> + };
> >> + kfifo_in(&djrcv_dev->notif_fifo, &workitem, sizeof(workitem));
> >> + /* logi_hidpp_recv_queue_notif will queue the work */
> >> + dj_dev = NULL;
> >> + }
> >> +
> >> if (dj_dev) {
> >> logi_dj_recv_forward_report(dj_dev, data, size);
> >> } else {
> >> @@ -1624,6 +1712,10 @@ static const struct hid_device_id logi_dj_receivers[] = {
> >> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> >> USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_GAMING),
> >> .driver_data = recvr_type_gaming_hidpp},
> >> + { /* Logitech 27 MHz HID++ 1.0 receiver (0xc517) */
> >> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> >> + USB_DEVICE_ID_S510_RECEIVER_2),
> >> + .driver_data = recvr_type_27mhz},
> >> {}
> >> };
> >>
> >> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> >> index 56559f2b8334..5decf16aeb4b 100644
> >> --- a/drivers/hid/hid-quirks.c
> >> +++ b/drivers/hid/hid-quirks.c
> >> @@ -432,7 +432,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
> >> #if IS_ENABLED(CONFIG_HID_LOGITECH)
> >> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
> >> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
> >> - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2) },
> >> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RECEIVER) },
> >> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_DINOVO_DESKTOP) },
> >> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_DINOVO_EDGE) },
> >> diff --git a/include/linux/hid.h b/include/linux/hid.h
> >> index f9707d1dcb58..9f161fa5cbd4 100644
> >> --- a/include/linux/hid.h
> >> +++ b/include/linux/hid.h
> >> @@ -382,6 +382,7 @@ struct hid_item {
> >> #define HID_GROUP_WACOM 0x0101
> >> #define HID_GROUP_LOGITECH_DJ_DEVICE 0x0102
> >> #define HID_GROUP_STEAM 0x0103
> >> +#define HID_GROUP_LOGITECH_27MHZ_DEVICE 0x0104
> >>
> >> /*
> >> * HID protocol status
> >> --
> >> 2.21.0
> >>
