[PATCH 0/7] HID: logitech-hidpp: support non DJ devices

[PATCH 0/7] HID: logitech-hidpp: support non DJ devices

[Benjamin Tissoires]

Hi Jiri, Nestor,

I have this series in my tree for a while now (first commit was in January).
I never sent it (as far as I can remember*) because I was requesting
feedback regarding the G920 wheel. I think I haven't break it, but
mistakes can happen.

Anyway, I think it's been too long, so I am just sending it now, as
it will fix the names of the non-dj devices that share the same USB receiver
(G900 and G403 for instance). Also, rebasing on top of new code is a pain
I'd rather not do every 3 months.

Cheers,
Benjamin

* looking into my archives, I actually sent those as a part of the
battery support in hid-logitech-hidpp. Consider it as a fresh start though :)

Benjamin Tissoires (7):
  HID: logitech-hidpp: allow non HID++ devices to be handled by this
    module
  HID: logitech-hidpp: make .probe usbhid capable
  HID: logitech-hidpp: support non-DJ receivers
  HID: logitech-hidpp: get the name and serial of the other non-HID++
    node
  HID: logitech-hidpp: create a name based on the type if non available
  HID: logitech-hidpp: support the G700 over wireless
  HID: logitech-hidpp: support the G900 over wireless

 drivers/hid/hid-ids.h            |   4 +
 drivers/hid/hid-logitech-hidpp.c | 470 +++++++++++++++++++++++++++++++++------
 2 files changed, 407 insertions(+), 67 deletions(-)

-- 
2.14.3

[PATCH 1/7] HID: logitech-hidpp: allow non HID++ devices to be handled by this module

[Benjamin Tissoires]

On the gaming mice, there are 2 interfaces, one for the mouse and one
for the macros. Better allow everybody to go through hid-logitech-hidpp
than trying to be smarter.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 77 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5f0c080059c6..37abae63bb1a 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2730,6 +2730,9 @@ static int hidpp_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 
+	if (!hidpp)
+		return 0;
+
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
 		return wtp_input_mapping(hdev, hi, field, usage, bit, max);
 	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560 &&
@@ -2745,6 +2748,9 @@ static int hidpp_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 
+	if (!hidpp)
+		return 0;
+
 	/* Ensure that Logitech G920 is not given a default fuzz/flat value */
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
 		if (usage->type == EV_ABS && (usage->code == ABS_X ||
@@ -2778,6 +2784,9 @@ static int hidpp_input_configured(struct hid_device *hdev,
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	struct input_dev *input = hidinput->input;
 
+	if (!hidpp)
+		return 0;
+
 	hidpp_populate_input(hidpp, input, true);
 
 	return 0;
@@ -2847,6 +2856,9 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	int ret = 0;
 
+	if (!hidpp)
+		return 0;
+
 	/* Generic HID++ processing. */
 	switch (data[0]) {
 	case REPORT_ID_HIDPP_VERY_LONG:
@@ -2895,7 +2907,13 @@ static int hidpp_event(struct hid_device *hdev, struct hid_field *field,
 	 * restriction imposed in hidpp_usages.
 	 */
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
-	struct hid_scroll_counter *counter = &hidpp->vertical_wheel_counter;
+	struct hid_scroll_counter *counter;
+
+	if (!hidpp)
+		return 0;
+
+	counter = &hidpp->vertical_wheel_counter;
+
 	/* A scroll event may occur before the multiplier has been retrieved or
 	 * the input device set, or high-res scroll enabling may fail. In such
 	 * cases we must return early (falling back to default behaviour) to
@@ -3153,6 +3171,41 @@ static const struct attribute_group ps_attribute_group = {
 	.attrs = sysfs_attrs
 };
 
+static bool hidpp_validate_report(struct hid_device *hdev, int id, int size,
+		bool optional)
+{
+	struct hid_report_enum *re;
+	struct hid_report *report;
+
+	if (id >= HID_MAX_IDS || id < 0) {
+		hid_err(hdev, "invalid HID report id %u\n", id);
+		return false;
+	}
+
+	re = &(hdev->report_enum[HID_OUTPUT_REPORT]);
+	report = re->report_id_hash[id];
+
+	if (!report)
+		return optional;
+
+	if (report->field[0]->report_count < size) {
+		hid_warn(hdev, "not enough values in hidpp report %d\n", id);
+		return false;
+	}
+
+	return true;
+}
+
+static bool hidpp_validate_device(struct hid_device *hdev)
+{
+	return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
+				     HIDPP_REPORT_SHORT_LENGTH - 1, false) &&
+	       hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
+				     HIDPP_REPORT_LONG_LENGTH - 1, true) &&
+	       hidpp_validate_report(hdev, REPORT_ID_HIDPP_VERY_LONG,
+				     HIDPP_REPORT_VERY_LONG_LENGTH - 1, true);
+}
+
 static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct hidpp_device *hidpp;
@@ -3160,6 +3213,18 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	bool connected;
 	unsigned int connect_mask = HID_CONNECT_DEFAULT;
 
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "%s:parse failed\n", __func__);
+		return ret;
+	}
+
+	/*
+	 * Make sure the device is HID++ capable, otherwise treat as generic HID
+	 */
+	if (!hidpp_validate_device(hdev))
+		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+
 	hidpp = devm_kzalloc(&hdev->dev, sizeof(struct hidpp_device),
 			GFP_KERNEL);
 	if (!hidpp)
@@ -3203,12 +3268,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hid_warn(hdev, "Cannot allocate sysfs group for %s\n",
 			 hdev->name);
 
-	ret = hid_parse(hdev);
-	if (ret) {
-		hid_err(hdev, "%s:parse failed\n", __func__);
-		goto hid_parse_fail;
-	}
-
 	if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
 		connect_mask &= ~HID_CONNECT_HIDINPUT;
 
@@ -3284,7 +3343,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hid_hw_stop(hdev);
 	}
 hid_hw_start_fail:
-hid_parse_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 	cancel_work_sync(&hidpp->work);
 	mutex_destroy(&hidpp->send_mutex);
@@ -3297,6 +3355,9 @@ static void hidpp_remove(struct hid_device *hdev)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 
+	if (!hidpp)
+		return hid_hw_stop(hdev);
+
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-- 
2.14.3

[PATCH 2/7] HID: logitech-hidpp: make .probe usbhid capable

[Benjamin Tissoires]

The current custom solution for the G920 is not the best because
hid_hw_start() is not called at the end of the .probe().
It means that any configuration retrieved after the initial hid_hw_start
would not be exposed to user space without races.

We can simply force hid_hw_start to just enable the transport layer by
not using a connect_mask. This way, we can have a common path between
USB, Unifying and Bluetooth devices.

Tested with a M185 with the non unifying receiver, a T650 and many other
unifying devices, and the T651 over Bluetooth.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-ids.h            |  2 +
 drivers/hid/hid-logitech-hidpp.c | 85 +++++++++++++++++++++-------------------
 2 files changed, 47 insertions(+), 40 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index e254ae802688..537223086d6b 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -741,7 +741,9 @@
 #define USB_DEVICE_ID_LOGITECH_CORDLESS_DESKTOP_LX500	0xc512
 #define USB_DEVICE_ID_MX3000_RECEIVER	0xc513
 #define USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER	0xc52b
+#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER		0xc52f
 #define USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2	0xc532
+#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2		0xc534
 #define USB_DEVICE_ID_SPACETRAVELLER	0xc623
 #define USB_DEVICE_ID_SPACENAVIGATOR	0xc626
 #define USB_DEVICE_ID_DINOVO_DESKTOP	0xc704
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 37abae63bb1a..d635e9685581 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3268,24 +3268,23 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hid_warn(hdev, "Cannot allocate sysfs group for %s\n",
 			 hdev->name);
 
-	if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
-		connect_mask &= ~HID_CONNECT_HIDINPUT;
-
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-		ret = hid_hw_start(hdev, connect_mask);
-		if (ret) {
-			hid_err(hdev, "hw start failed\n");
-			goto hid_hw_start_fail;
-		}
-		ret = hid_hw_open(hdev);
-		if (ret < 0) {
-			dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
-				__func__, ret);
-			hid_hw_stop(hdev);
-			goto hid_hw_start_fail;
-		}
+	/*
+	 * Plain USB connections need to actually call start and open
+	 * on the transport driver to allow incoming data.
+	 */
+	ret = hid_hw_start(hdev, 0);
+	if (ret) {
+		hid_err(hdev, "hw start failed\n");
+		goto hid_hw_start_fail;
 	}
 
+	ret = hid_hw_open(hdev);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
+			__func__, ret);
+		hid_hw_stop(hdev);
+		goto hid_hw_open_fail;
+	}
 
 	/* Allow incoming packets */
 	hid_device_io_start(hdev);
@@ -3299,7 +3298,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		if (!connected) {
 			ret = -ENODEV;
 			hid_err(hdev, "Device not connected");
-			goto hid_hw_open_failed;
+			goto hid_hw_init_fail;
 		}
 
 		hid_info(hdev, "HID++ %u.%u device connected.\n",
@@ -3311,37 +3310,36 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
 		ret = wtp_get_config(hidpp);
 		if (ret)
-			goto hid_hw_open_failed;
+			goto hid_hw_init_fail;
 	} else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
 		ret = g920_get_config(hidpp);
 		if (ret)
-			goto hid_hw_open_failed;
+			goto hid_hw_init_fail;
 	}
 
-	/* Block incoming packets */
-	hid_device_io_stop(hdev);
+	hidpp_connect_event(hidpp);
 
-	if (!(hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
-		ret = hid_hw_start(hdev, connect_mask);
-		if (ret) {
-			hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
-			goto hid_hw_start_fail;
-		}
-	}
+	/* Reset the HID node state */
+	hid_device_io_stop(hdev);
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
 
-	/* Allow incoming packets */
-	hid_device_io_start(hdev);
+	if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
+		connect_mask &= ~HID_CONNECT_HIDINPUT;
 
-	hidpp_connect_event(hidpp);
+	/* Now export the actual inputs and hidraw nodes to the world */
+	ret = hid_hw_start(hdev, connect_mask);
+	if (ret) {
+		hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
+		goto hid_hw_start_fail;
+	}
 
 	return ret;
 
-hid_hw_open_failed:
-	hid_device_io_stop(hdev);
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-		hid_hw_close(hdev);
-		hid_hw_stop(hdev);
-	}
+hid_hw_init_fail:
+	hid_hw_close(hdev);
+hid_hw_open_fail:
+	hid_hw_stop(hdev);
 hid_hw_start_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 	cancel_work_sync(&hidpp->work);
@@ -3360,10 +3358,9 @@ static void hidpp_remove(struct hid_device *hdev)
 
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
+	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
 		hidpp_ff_deinit(hdev);
-		hid_hw_close(hdev);
-	}
+
 	hid_hw_stop(hdev);
 	cancel_work_sync(&hidpp->work);
 	mutex_destroy(&hidpp->send_mutex);
@@ -3427,6 +3424,14 @@ static const struct hid_device_id hidpp_devices[] = {
 
 	{ LDJ_DEVICE(HID_ANY_ID) },
 
+	{ /* Logitech Nano (non DJ) receiver */
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+			 USB_DEVICE_ID_LOGITECH_NANO_RECEIVER) },
+
+	{ /* Logitech Nano (non DJ) receiver */
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+			 USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2) },
+
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL),
 		.driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
 	{}
-- 
2.14.3

[PATCH 3/7] HID: logitech-hidpp: support non-DJ receivers

[Benjamin Tissoires]

The non unifying receivers are similar to a Unifying one, but not
entirely. They don't come with the DJ collections and thus can't be
handled by hid-logitech-dj.

To enable connection notifications, we need to instruct the receiver (0xff)
that we can handle those. Then, we need to retrieve the device index(es)
of the connected devices by manually enumerating the 6 available slots.
The two non DJ can actually handle two devices, one keyboard in slot
1 and one mouse in slot 2. The HID++ collection is in the second
interface, so we should be sure not to assign the attached keyboard
to the mouse input node. The proper keyboard matching will be added in
a following patch.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 119 +++++++++++++++++++++++++++++++++------
 1 file changed, 103 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index d635e9685581..315ef209a0c4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -67,6 +67,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_HI_RES_SCROLL_1P0		BIT(26)
 #define HIDPP_QUIRK_HI_RES_SCROLL_X2120		BIT(27)
 #define HIDPP_QUIRK_HI_RES_SCROLL_X2121		BIT(28)
+#define HIDPP_QUIRK_RECEIVER			BIT(29)
 
 /* Convenience constant to check for any high-res support. */
 #define HIDPP_QUIRK_HI_RES_SCROLL	(HIDPP_QUIRK_HI_RES_SCROLL_1P0 | \
@@ -145,6 +146,7 @@ struct hidpp_device {
 	bool answer_available;
 	u8 protocol_major;
 	u8 protocol_minor;
+	u8 device_index;
 
 	void *private_data;
 
@@ -202,11 +204,7 @@ static int __hidpp_send_report(struct hid_device *hdev,
 		return -ENODEV;
 	}
 
-	/*
-	 * set the device_index as the receiver, it will be overwritten by
-	 * hid_hw_request if needed
-	 */
-	hidpp_report->device_index = 0xff;
+	hidpp_report->device_index = hidpp->device_index;
 
 	if (hidpp->quirks & HIDPP_QUIRK_FORCE_OUTPUT_REPORTS) {
 		ret = hid_hw_output_report(hdev, (u8 *)hidpp_report, fields_count);
@@ -458,6 +456,36 @@ static int hidpp10_enable_scrolling_acceleration(struct hidpp_device *hidpp_dev)
 	return hidpp10_set_register_bit(hidpp_dev, HIDPP_REG_FEATURES, 0, 6);
 }
 
+/* Must be called with 0xff as device index */
+static int hidpp_unifying_enable_notifications(struct hidpp_device *hidpp_dev)
+{
+	struct hidpp_report response;
+	int ret;
+	u8 params[3] = { 0 };
+
+	ret = hidpp_send_rap_command_sync(hidpp_dev,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_GET_REGISTER,
+					HIDPP_REG_GENERAL,
+					NULL, 0, &response);
+	if (ret)
+		return ret;
+
+	memcpy(params, response.rap.params, 3);
+
+	/* Set the wireless notification bit */
+	params[1] |= BIT(0);
+	params[1] |= BIT(3);
+
+	ret = hidpp_send_rap_command_sync(hidpp_dev,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_SET_REGISTER,
+					HIDPP_REG_GENERAL,
+					params, 3, &response);
+
+	return ret;
+}
+
 #define HIDPP_REG_BATTERY_STATUS			0x07
 
 static int hidpp10_battery_status_map_level(u8 param)
@@ -636,14 +664,41 @@ static int hidpp10_battery_event(struct hidpp_device *hidpp, u8 *data, int size)
 }
 
 #define HIDPP_REG_PAIRING_INFORMATION			0xB5
+#define HIDPP_PAIRING_INFORMATION			0x20
 #define HIDPP_EXTENDED_PAIRING				0x30
 #define HIDPP_DEVICE_NAME				0x40
 
-static char *hidpp_unifying_get_name(struct hidpp_device *hidpp_dev)
+static inline bool hidpp_unifying_type_is_keyboard(u8 type)
+{
+	return type == 0x01;
+}
+
+static int hidpp_unifying_get_quadid(struct hidpp_device *hidpp_dev, int index,
+				     __be16 *quadid, u8 *type)
+{
+	struct hidpp_report response;
+	int ret;
+	u8 params[1] = { HIDPP_PAIRING_INFORMATION | ((index - 1) & 0x0F) };
+
+	ret = hidpp_send_rap_command_sync(hidpp_dev,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_GET_LONG_REGISTER,
+					HIDPP_REG_PAIRING_INFORMATION,
+					params, 1, &response);
+	if (ret)
+		return ret;
+
+	*quadid = get_unaligned_be16(&response.rap.params[3]);
+	*type = response.rap.params[7];
+
+	return ret;
+}
+
+static char *hidpp_unifying_get_name(struct hidpp_device *hidpp_dev, int index)
 {
 	struct hidpp_report response;
 	int ret;
-	u8 params[1] = { HIDPP_DEVICE_NAME };
+	u8 params[1] = { HIDPP_DEVICE_NAME | ((index - 1) & 0x0F) };
 	char *name;
 	int len;
 
@@ -672,11 +727,12 @@ static char *hidpp_unifying_get_name(struct hidpp_device *hidpp_dev)
 	return name;
 }
 
-static int hidpp_unifying_get_serial(struct hidpp_device *hidpp, u32 *serial)
+static int hidpp_unifying_get_serial(struct hidpp_device *hidpp, int index,
+				     u32 *serial)
 {
 	struct hidpp_report response;
 	int ret;
-	u8 params[1] = { HIDPP_EXTENDED_PAIRING };
+	u8 params[1] = { HIDPP_EXTENDED_PAIRING | ((index - 1) & 0x0F) };
 
 	ret = hidpp_send_rap_command_sync(hidpp,
 					REPORT_ID_HIDPP_SHORT,
@@ -699,23 +755,42 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
 	struct hid_device *hdev = hidpp->hid_dev;
 	const char *name;
 	u32 serial;
-	int ret;
+	u16 quadid, q;
+	int device_index, ret;
+	u8 type = 0;
+
+	device_index = 0;
+	quadid = hdev->product;
+
+	if (hidpp->quirks & HIDPP_QUIRK_RECEIVER) {
+		/*
+		 * On the receiver, the HID++ collection is on the mouse
+		 * interface, so match only there.
+		 */
+		ret = hidpp_unifying_get_quadid(hidpp, 2, &q, &type);
+		if (!ret && !hidpp_unifying_type_is_keyboard(type)) {
+			device_index = 2;
+			quadid = q;
+		}
+		hidpp_unifying_enable_notifications(hidpp);
+	}
 
-	ret = hidpp_unifying_get_serial(hidpp, &serial);
+	ret = hidpp_unifying_get_serial(hidpp, device_index, &serial);
 	if (ret)
 		return ret;
 
-	snprintf(hdev->uniq, sizeof(hdev->uniq), "%04x-%4phD",
-		 hdev->product, &serial);
+	snprintf(hdev->uniq, sizeof(hdev->uniq), "%04x-%4phD", quadid, &serial);
 	dbg_hid("HID++ Unifying: Got serial: %s\n", hdev->uniq);
 
-	name = hidpp_unifying_get_name(hidpp);
+	name = hidpp_unifying_get_name(hidpp, device_index);
 	if (!name)
 		return -EIO;
 
 	snprintf(hdev->name, sizeof(hdev->name), "%s", name);
 	dbg_hid("HID++ Unifying: Got name: %s\n", name);
 
+	hidpp->device_index = device_index;
+
 	kfree(name);
 	return 0;
 }
@@ -2800,6 +2875,10 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 	struct hidpp_report *report = (struct hidpp_report *)data;
 	int ret;
 
+	if ((hidpp->quirks & HIDPP_QUIRK_RECEIVER) &&
+	    data[1] != hidpp->device_index)
+		return 0;
+
 	/*
 	 * If the mutex is locked then we have a pending answer from a
 	 * previously sent command.
@@ -3239,6 +3318,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE)
 		hidpp->quirks |= HIDPP_QUIRK_UNIFYING;
 
+	/*
+	 * set the device_index as the receiver, it will be overwritten by
+	 * hid_hw_request if needed
+	 */
+	hidpp->device_index = 0xFF;
+
 	if (disable_raw_mode) {
 		hidpp->quirks &= ~HIDPP_QUIRK_CLASS_WTP;
 		hidpp->quirks &= ~HIDPP_QUIRK_NO_HIDINPUT;
@@ -3426,11 +3511,13 @@ static const struct hid_device_id hidpp_devices[] = {
 
 	{ /* Logitech Nano (non DJ) receiver */
 	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
-			 USB_DEVICE_ID_LOGITECH_NANO_RECEIVER) },
+			 USB_DEVICE_ID_LOGITECH_NANO_RECEIVER),
+	  .driver_data = HIDPP_QUIRK_RECEIVER | HIDPP_QUIRK_UNIFYING },
 
 	{ /* Logitech Nano (non DJ) receiver */
 	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
-			 USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2) },
+			 USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2),
+	  .driver_data = HIDPP_QUIRK_RECEIVER | HIDPP_QUIRK_UNIFYING },
 
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL),
 		.driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
-- 
2.14.3

[PATCH 4/7] HID: logitech-hidpp: get the name and serial of the other non-HID++ node

[Benjamin Tissoires]

The non-Unifying receivers forward the input events over one HID++
capable node (the second one) and one which is not HID++ capable (the
first one, the keyboard). We can use the HID++ information to know
which device name and serial is connected to the first node. For that,
delay the probe of the non-HID++ node, retrieve the information when
available and then authorize the non-HID++ node to appear.

Bonus point, this allows gaming mice to have both input nodes named
after the mouse, and not one called "Logitech USB Receiver".

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 193 +++++++++++++++++++++++++++++++++------
 1 file changed, 166 insertions(+), 27 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 315ef209a0c4..5f96e63dbd87 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -137,6 +137,16 @@ struct hidpp_battery {
 	bool online;
 };
 
+struct hidpp_shared {
+	struct list_head list;
+	char name[128];		/* Device name */
+	char phys[64];		/* Device physical location */
+	char uniq[64];		/* Device unique identifier (serial #) */
+};
+
+static LIST_HEAD(hidpp_shared_list);
+static DEFINE_MUTEX(hidpp_shared_list_lock);
+
 struct hidpp_device {
 	struct hid_device *hid_dev;
 	struct mutex send_mutex;
@@ -160,6 +170,8 @@ struct hidpp_device {
 
 	struct hidpp_battery battery;
 	struct hid_scroll_counter vertical_wheel_counter;
+
+	struct hidpp_shared *shared;
 };
 
 /* HID++ 1.0 error codes */
@@ -398,6 +410,81 @@ static void hidpp_prefix_name(char **name, int name_length)
 	*name = new_name;
 }
 
+static void hidpp_remove_shared_data(void *res)
+{
+	struct hidpp_device *hidpp = res;
+
+	mutex_lock(&hidpp_shared_list_lock);
+
+	if (hidpp->shared) {
+		list_del(&hidpp->shared->list);
+		kfree(hidpp->shared);
+	}
+	hidpp->shared = NULL;
+
+	mutex_unlock(&hidpp_shared_list_lock);
+}
+
+static bool hidpp_compare_device_paths(const char *hdev_a, const char *hdev_b)
+{
+	const char separator = '/';
+	int n1 = strrchr(hdev_a, separator) - hdev_a;
+	int n2 = strrchr(hdev_b, separator) - hdev_b;
+
+	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+		return false;
+
+	return !strncmp(hdev_a, hdev_b, n1);
+}
+
+static int hidpp_allocate_shared_data(struct hidpp_device *hidpp)
+{
+	struct hid_device *hdev = hidpp->hid_dev;
+	struct hidpp_shared *shared = NULL;
+
+	mutex_lock(&hidpp_shared_list_lock);
+
+	shared = kzalloc(sizeof(struct hidpp_shared), GFP_KERNEL);
+	if (!shared) {
+		mutex_unlock(&hidpp_shared_list_lock);
+		return -ENOMEM;
+	}
+	memcpy(shared->name, hdev->name, sizeof(shared->name));
+	memcpy(shared->uniq, hdev->uniq, sizeof(shared->uniq));
+	memcpy(shared->phys, hdev->phys, sizeof(shared->phys));
+	list_add_tail(&shared->list, &hidpp_shared_list);
+
+	hidpp->shared = shared;
+
+	mutex_unlock(&hidpp_shared_list_lock);
+
+	return devm_add_action_or_reset(&hidpp->hid_dev->dev,
+					hidpp_remove_shared_data, hidpp);
+}
+
+static int hidpp_get_shared_data(struct hid_device *hdev)
+{
+	struct hidpp_shared *s, *shared = NULL;
+
+	mutex_lock(&hidpp_shared_list_lock);
+
+	list_for_each_entry(s, &hidpp_shared_list, list) {
+		if (hidpp_compare_device_paths(s->phys, hdev->phys))
+			shared = s;
+	}
+
+	if (!shared) {
+		mutex_unlock(&hidpp_shared_list_lock);
+		return -EPROBE_DEFER;
+	}
+
+	memcpy(hdev->name, shared->name, sizeof(hdev->name));
+	memcpy(hdev->uniq, shared->uniq, sizeof(hdev->uniq));
+
+	mutex_unlock(&hidpp_shared_list_lock);
+	return 0;
+}
+
 /* -------------------------------------------------------------------------- */
 /* HIDP++ 1.0 commands                                                        */
 /* -------------------------------------------------------------------------- */
@@ -750,48 +837,90 @@ static int hidpp_unifying_get_serial(struct hidpp_device *hidpp, int index,
 	return 0;
 }
 
+static int hidpp_unifying_get_name_and_uniq(struct hidpp_device *hidpp,
+		int device_index, u16 quadid, char name[128], char uniq[64])
+{
+	const char *hidpp_name;
+	u32 serial;
+	int ret;
+
+	ret = hidpp_unifying_get_serial(hidpp, device_index, &serial);
+	if (ret)
+		return ret;
+
+	snprintf(uniq, 64, "%04x-%4phD", quadid, &serial);
+	dbg_hid("HID++ Unifying: Got serial: '%s' for device %d\n", uniq,
+		device_index);
+
+	hidpp_name = hidpp_unifying_get_name(hidpp, device_index);
+	if (!hidpp_name)
+		return -EIO;
+
+	snprintf(name, 128, "%s", hidpp_name);
+	dbg_hid("HID++ Unifying: Got name: '%s' for device %d\n", name,
+		device_index);
+
+	kfree(hidpp_name);
+	return 0;
+}
+
 static int hidpp_unifying_init(struct hidpp_device *hidpp)
 {
 	struct hid_device *hdev = hidpp->hid_dev;
-	const char *name;
-	u32 serial;
 	u16 quadid, q;
-	int device_index, ret;
+	int device_index, i, ret;
 	u8 type = 0;
 
 	device_index = 0;
 	quadid = hdev->product;
 
 	if (hidpp->quirks & HIDPP_QUIRK_RECEIVER) {
-		/*
-		 * On the receiver, the HID++ collection is on the mouse
-		 * interface, so match only there.
-		 */
-		ret = hidpp_unifying_get_quadid(hidpp, 2, &q, &type);
-		if (!ret && !hidpp_unifying_type_is_keyboard(type)) {
-			device_index = 2;
-			quadid = q;
+		ret = hidpp_allocate_shared_data(hidpp);
+		if (ret)
+			return ret;
+
+		/* The device indexes go from 1 to 2 */
+		for (i = 1; i <= 2; i++) {
+			ret = hidpp_unifying_get_quadid(hidpp, i, &q, &type);
+			if (ret)
+				continue;
+			/*
+			 * On the receiver, the HID++ collection is on the mouse
+			 * interface, so match only there.
+			 */
+			if (!hidpp_unifying_type_is_keyboard(type)) {
+				device_index = i;
+				quadid = q;
+			}
+
+			/*
+			 * The first device will be reported through the
+			 * first hidraw node (the non HID++ one).
+			 */
+			if (i == 1) {
+				ret = hidpp_unifying_get_name_and_uniq(hidpp, i,
+							q, hidpp->shared->name,
+							hidpp->shared->uniq);
+				if (ret)
+					return ret;
+			}
 		}
 		hidpp_unifying_enable_notifications(hidpp);
 	}
 
-	ret = hidpp_unifying_get_serial(hidpp, device_index, &serial);
-	if (ret)
-		return ret;
-
-	snprintf(hdev->uniq, sizeof(hdev->uniq), "%04x-%4phD", quadid, &serial);
-	dbg_hid("HID++ Unifying: Got serial: %s\n", hdev->uniq);
-
-	name = hidpp_unifying_get_name(hidpp, device_index);
-	if (!name)
-		return -EIO;
-
-	snprintf(hdev->name, sizeof(hdev->name), "%s", name);
-	dbg_hid("HID++ Unifying: Got name: %s\n", name);
+	if (device_index != 1) {
+		ret = hidpp_unifying_get_name_and_uniq(hidpp, device_index,
+						       quadid, hdev->name,
+						       hdev->uniq);
+		if (ret)
+			return ret;
+	} else {
+		memcpy(hdev->name, hidpp->shared->name, sizeof(hdev->name));
+		memcpy(hdev->uniq, hidpp->shared->uniq, sizeof(hdev->uniq));
+	}
 
 	hidpp->device_index = device_index;
 
-	kfree(name);
 	return 0;
 }
 
@@ -3299,10 +3428,20 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 
 	/*
-	 * Make sure the device is HID++ capable, otherwise treat as generic HID
+	 * Make sure the device is HID++ capable, otherwise treat as generic
+	 * HID.
+	 * Special case for manually affected Non Unifying receiver where we
+	 * want to link the interfaces together to have a proper name instead
+	 * of "Logitech USB Receiver".
 	 */
-	if (!hidpp_validate_device(hdev))
+	if (!hidpp_validate_device(hdev)) {
+		if (id->driver_data & HIDPP_QUIRK_RECEIVER) {
+			ret = hidpp_get_shared_data(hdev);
+			if (ret)
+				return ret;
+		}
 		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	}
 
 	hidpp = devm_kzalloc(&hdev->dev, sizeof(struct hidpp_device),
 			GFP_KERNEL);
-- 
2.14.3

[PATCH 5/7] HID: logitech-hidpp: create a name based on the type if non available

[Benjamin Tissoires]

Some low cost devices don't even have a name (nor a serial) registered
in the receiver. In those case, create a better name that "Logitech
USB Receiver" or even "Logitech  " if we try to get their name.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5f96e63dbd87..a09509fdb746 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -802,6 +802,9 @@ static char *hidpp_unifying_get_name(struct hidpp_device *hidpp_dev, int index)
 	if (2 + len > sizeof(response.rap.params))
 		return NULL;
 
+	if (len < 4) /* logitech devices are usually at least Xddd */
+		return NULL;
+
 	name = kzalloc(len + 1, GFP_KERNEL);
 	if (!name)
 		return NULL;
@@ -838,7 +841,8 @@ static int hidpp_unifying_get_serial(struct hidpp_device *hidpp, int index,
 }
 
 static int hidpp_unifying_get_name_and_uniq(struct hidpp_device *hidpp,
-		int device_index, u16 quadid, char name[128], char uniq[64])
+		int device_index, u16 quadid, u8 type, char name[128],
+		char uniq[64])
 {
 	const char *hidpp_name;
 	u32 serial;
@@ -853,8 +857,29 @@ static int hidpp_unifying_get_name_and_uniq(struct hidpp_device *hidpp,
 		device_index);
 
 	hidpp_name = hidpp_unifying_get_name(hidpp, device_index);
-	if (!hidpp_name)
-		return -EIO;
+	if (!hidpp_name) {
+		switch (type) {
+		case 0x01:
+			snprintf(name, 128, "Logitech Wireless Keyboard");
+			break;
+		case 0x02:
+			snprintf(name, 128, "Logitech Wireless Mouse");
+			break;
+		case 0x03:
+			snprintf(name, 128, "Logitech Wireless Numpad");
+			break;
+		case 0x04:
+			snprintf(name, 128, "Logitech Wireless Presenter");
+			break;
+		case 0x08:
+			snprintf(name, 128, "Logitech Wireless Trackball");
+			break;
+		case 0x09:
+			snprintf(name, 128, "Logitech Wireless Touchpad");
+			break;
+		}
+		return 0;
+	}
 
 	snprintf(name, 128, "%s", hidpp_name);
 	dbg_hid("HID++ Unifying: Got name: '%s' for device %d\n", name,
@@ -899,7 +924,8 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
 			 */
 			if (i == 1) {
 				ret = hidpp_unifying_get_name_and_uniq(hidpp, i,
-							q, hidpp->shared->name,
+							q, type,
+							hidpp->shared->name,
 							hidpp->shared->uniq);
 				if (ret)
 					return ret;
@@ -910,7 +936,7 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
 
 	if (device_index != 1) {
 		ret = hidpp_unifying_get_name_and_uniq(hidpp, device_index,
-						       quadid, hdev->name,
+						       quadid, type, hdev->name,
 						       hdev->uniq);
 		if (ret)
 			return ret;
-- 
2.14.3

[PATCH 6/7] HID: logitech-hidpp: support the G700 over wireless

[Benjamin Tissoires]

The G700 is using a non unifying receiver, so it's easy to add its support
in hid-logitech-hidpp now.

With this, the 2 input nodes attached to the mouse will be enumerated
as "Logitech G700" and the battery power_supply will be exported (HID++
1.0 battery type).

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-ids.h            | 1 +
 drivers/hid/hid-logitech-hidpp.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 537223086d6b..b366976e928c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -742,6 +742,7 @@
 #define USB_DEVICE_ID_MX3000_RECEIVER	0xc513
 #define USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER	0xc52b
 #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER		0xc52f
+#define USB_DEVICE_ID_LOGITECH_G700_RECEIVER		0xc531
 #define USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2	0xc532
 #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2		0xc534
 #define USB_DEVICE_ID_SPACETRAVELLER	0xc623
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index a09509fdb746..0ab7f8b66b83 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3671,6 +3671,9 @@ static const struct hid_device_id hidpp_devices[] = {
 	{ /* Solar Keyboard Logitech K750 */
 	  LDJ_DEVICE(0x4002),
 	  .driver_data = HIDPP_QUIRK_CLASS_K750 },
+	{ /* G700 over Wireless */
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G700_RECEIVER),
+	  .driver_data = HIDPP_QUIRK_RECEIVER | HIDPP_QUIRK_UNIFYING },
 
 	{ LDJ_DEVICE(HID_ANY_ID) },
 
-- 
2.14.3

[PATCH 7/7] HID: logitech-hidpp: support the G900 over wireless

[Benjamin Tissoires]

The receiver of the G900 is similar to a Unifying one. However,
if we switch the receiver into the DJ mode through hid-logitech-dj,
the reports emitted by the mouse are from a new kind. Instead of finding
out what they are, and given that only one device should be connected
to the receiver, just use hid-logitech-hidpp.

The battery is not yet supported in the kernel, but at least the
input nodes are now named after the mouse.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-ids.h            |  1 +
 drivers/hid/hid-logitech-hidpp.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index b366976e928c..f3de8562a92b 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -745,6 +745,7 @@
 #define USB_DEVICE_ID_LOGITECH_G700_RECEIVER		0xc531
 #define USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2	0xc532
 #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2		0xc534
+#define USB_DEVICE_ID_LOGITECH_G900_RECEIVER		0xc539
 #define USB_DEVICE_ID_SPACETRAVELLER	0xc623
 #define USB_DEVICE_ID_SPACENAVIGATOR	0xc626
 #define USB_DEVICE_ID_DINOVO_DESKTOP	0xc704
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0ab7f8b66b83..2e264869b096 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -68,6 +68,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_HI_RES_SCROLL_X2120		BIT(27)
 #define HIDPP_QUIRK_HI_RES_SCROLL_X2121		BIT(28)
 #define HIDPP_QUIRK_RECEIVER			BIT(29)
+#define HIDPP_QUIRK_FORCE_OPEN			BIT(30)
 
 /* Convenience constant to check for any high-res support. */
 #define HIDPP_QUIRK_HI_RES_SCROLL	(HIDPP_QUIRK_HI_RES_SCROLL_1P0 | \
@@ -3584,6 +3585,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto hid_hw_start_fail;
 	}
 
+	if (hidpp->quirks & HIDPP_QUIRK_FORCE_OPEN) {
+		ret = hid_hw_open(hdev);
+		if (ret)
+			goto hid_hw_open_fail;
+	}
+
 	return ret;
 
 hid_hw_init_fail:
@@ -3608,6 +3615,9 @@ static void hidpp_remove(struct hid_device *hdev)
 
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 
+	if (hidpp->quirks & HIDPP_QUIRK_FORCE_OPEN)
+		hid_hw_close(hdev);
+
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
 		hidpp_ff_deinit(hdev);
 
@@ -3674,6 +3684,11 @@ static const struct hid_device_id hidpp_devices[] = {
 	{ /* G700 over Wireless */
 	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G700_RECEIVER),
 	  .driver_data = HIDPP_QUIRK_RECEIVER | HIDPP_QUIRK_UNIFYING },
+	{ /* G900 over Wireless */
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_G900_RECEIVER),
+	  .driver_data = HIDPP_QUIRK_RECEIVER | HIDPP_QUIRK_UNIFYING |
+			 HIDPP_QUIRK_FORCE_OPEN},
 
 	{ LDJ_DEVICE(HID_ANY_ID) },
 
-- 
2.14.3

[PATCH 8/7] HID: quirks: do not blacklist Logitech devices

[Benjamin Tissoires]

I am actually suggesting people to not populate this list, and I should
probably start to apply my advices to myself.

The end result means that if your initrd is lacking hid-logitech-dj
or hid-logitech-hidpp, but still contains hid-generic, then your
keyboard will work during pre-init.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

Oops, looks like I forgot to append this one to the series.

Cheers,
Benjamin

 drivers/hid/hid-quirks.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 249d49b6b16c..996febdf21ee 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -462,13 +462,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACENAVIGATOR) },
 #endif
 #if IS_ENABLED(CONFIG_HID_LOGITECH_HIDPP)
-	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_T651) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL) },
 #endif
-#if IS_ENABLED(CONFIG_HID_LOGITECH_DJ)
-	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2) },
-#endif
 #if IS_ENABLED(CONFIG_HID_MAGICMOUSE)
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE) },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD) },
-- 
2.14.3

Re: [PATCH 6/7] HID: logitech-hidpp: support the G700 over wireless

[Harry Cutts]

Hi Benjamin,

On Fri, 7 Sep 2018 at 03:35, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> The G700 is using a non unifying receiver, so it's easy to add its support
> in hid-logitech-hidpp now.
> [snip]
> @@ -3671,6 +3671,9 @@ static const struct hid_device_id hidpp_devices[] = {
>         { /* Solar Keyboard Logitech K750 */
>           LDJ_DEVICE(0x4002),
>           .driver_data = HIDPP_QUIRK_CLASS_K750 },
> +       { /* G700 over Wireless */
> +         HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G700_RECEIVER),
> +         .driver_data = HIDPP_QUIRK_RECEIVER | HIDPP_QUIRK_UNIFYING },

As someone who's new to the codebase, it seems rather confusing to me
that HIDPP_QUIRK_UNIFYING would be present here for a device that
doesn't use a Unifying receiver. Am I misunderstanding, or should we
consider renaming the quirk or adding some clarifying comment?
(Similarly for the G900 in the next patch.)

>
>         { LDJ_DEVICE(HID_ANY_ID) },
>
> --
> 2.14.3
>

Thanks,

Harry Cutts
Chrome OS Touch/Input team

Re: [PATCH 0/7] HID: logitech-hidpp: support non DJ devices

[Benjamin Tissoires]

Hello,

On Fri, Sep 7, 2018 at 12:34 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi Jiri, Nestor,
>
> I have this series in my tree for a while now (first commit was in January).
> I never sent it (as far as I can remember*) because I was requesting
> feedback regarding the G920 wheel. I think I haven't break it, but
> mistakes can happen.
>
> Anyway, I think it's been too long, so I am just sending it now, as
> it will fix the names of the non-dj devices that share the same USB receiver
> (G900 and G403 for instance). Also, rebasing on top of new code is a pain
> I'd rather not do every 3 months.
>

Can anyone review this series?
It's been in my local tree for too long and feels too lonely there :)

Cheers,
Benjamin

>
> * looking into my archives, I actually sent those as a part of the
> battery support in hid-logitech-hidpp. Consider it as a fresh start though :)
>
> Benjamin Tissoires (7):
>   HID: logitech-hidpp: allow non HID++ devices to be handled by this
>     module
>   HID: logitech-hidpp: make .probe usbhid capable
>   HID: logitech-hidpp: support non-DJ receivers
>   HID: logitech-hidpp: get the name and serial of the other non-HID++
>     node
>   HID: logitech-hidpp: create a name based on the type if non available
>   HID: logitech-hidpp: support the G700 over wireless
>   HID: logitech-hidpp: support the G900 over wireless
>
>  drivers/hid/hid-ids.h            |   4 +
>  drivers/hid/hid-logitech-hidpp.c | 470 +++++++++++++++++++++++++++++++++------
>  2 files changed, 407 insertions(+), 67 deletions(-)
>
> --
> 2.14.3
>

Re: [PATCH 6/7] HID: logitech-hidpp: support the G700 over wireless

[Benjamin Tissoires]

Hi Harry,

[now that the series has been reverted and re-inserted, I am starting
to have a look at this again]

On Fri, Sep 7, 2018 at 7:33 PM Harry Cutts <hcutts@chromium.org> wrote:
>
> Hi Benjamin,
>
> On Fri, 7 Sep 2018 at 03:35, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > The G700 is using a non unifying receiver, so it's easy to add its support
> > in hid-logitech-hidpp now.
> > [snip]
> > @@ -3671,6 +3671,9 @@ static const struct hid_device_id hidpp_devices[] = {
> >         { /* Solar Keyboard Logitech K750 */
> >           LDJ_DEVICE(0x4002),
> >           .driver_data = HIDPP_QUIRK_CLASS_K750 },
> > +       { /* G700 over Wireless */
> > +         HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G700_RECEIVER),
> > +         .driver_data = HIDPP_QUIRK_RECEIVER | HIDPP_QUIRK_UNIFYING },
>
> As someone who's new to the codebase, it seems rather confusing to me
> that HIDPP_QUIRK_UNIFYING would be present here for a device that
> doesn't use a Unifying receiver. Am I misunderstanding, or should we
> consider renaming the quirk or adding some clarifying comment?
> (Similarly for the G900 in the next patch.)

The initial reason is that the gaming receivers are Unifying but that
are not normally allowed to connect to more that one device at a time.
The reason is that those receiver are using a higher frequency and
need all of the bandwidth to operate (so they can't multiplex the
devices).

But as I re-read the series, it is clear that HIDPP_QUIRK_RECEIVER and
HIDPP_QUIRK_UNIFYING are never used separately, so it doesn't make
sense to have 2 quirks. We should probably rename UNIFYING into
RECEIVER for consistency and we will be good.

Cheers,
Benjamin

>
> >
> >         { LDJ_DEVICE(HID_ANY_ID) },
> >
> > --
> > 2.14.3
> >
>
> Thanks,
>
> Harry Cutts
> Chrome OS Touch/Input team