linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] HID: revert the Logitech High Resolution wheel support
@ 2018-11-21 15:27 Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 1/7] Revert "HID: input: simplify/fix high-res scroll event handling" Benjamin Tissoires
                   ` (9 more replies)
  0 siblings, 10 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2018-11-21 15:27 UTC (permalink / raw)
  To: Dmitry Torokhov, Jiri Kosina, Harry Cutts, Peter Hutterer, torvalds
  Cc: Nestor Lopez Casado, linux-input, linux-kernel, Benjamin Tissoires

It turns out that the implementation of the high resolution support of
Logitech wheels is rather incompatible with the mice from Microsoft.

We had a lengthy discussion off-list and the summary is quoted in 7/7.

The TL;DR, we need to revert the current series before it gets out in
a released kernel and work on a better approach for 4.21.

This patch series has informally been acked by Dmitry, Harry, Jiri, Nestor
and Peter, but I wouldn't mind a public ack before I push this to
the for-linus branch.

Dmitry, I chose to also revert "Input: Add the `REL_WHEEL_HI_RES` event code"
as the documentation needs to be updated.
I would understand if you rather keep the patch that way and we just update
the doc. This would help synchronizing the trees. So please tell me if you
want 7/7 in the series or not (I'll reshuffle the commit message to have
the summary from Peter).

Cheers,
Benjamin

Benjamin Tissoires (7):
  Revert "HID: input: simplify/fix high-res scroll event handling"
  Revert "HID: logitech: fix a used uninitialized GCC warning"
  Revert "HID: logitech: Use LDJ_DEVICE macro for existing Logitech
    mice"
  Revert "HID: logitech: Enable high-resolution scrolling on Logitech
    mice"
  Revert "HID: logitech: Add function to enable HID++ 1.0 "scrolling
    acceleration""
  Revert "HID: input: Create a utility class for counting scroll events"
  Revert "Input: Add the `REL_WHEEL_HI_RES` event code"

 Documentation/input/event-codes.rst    |  11 +-
 drivers/hid/hid-input.c                |  44 ----
 drivers/hid/hid-logitech-hidpp.c       | 309 +++----------------------
 include/linux/hid.h                    |  28 ---
 include/uapi/linux/input-event-codes.h |  10 -
 5 files changed, 28 insertions(+), 374 deletions(-)

-- 
2.19.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/7] Revert "HID: input: simplify/fix high-res scroll event handling"
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
@ 2018-11-21 15:27 ` Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 2/7] Revert "HID: logitech: fix a used uninitialized GCC warning" Benjamin Tissoires
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2018-11-21 15:27 UTC (permalink / raw)
  To: Dmitry Torokhov, Jiri Kosina, Harry Cutts, Peter Hutterer, torvalds
  Cc: Nestor Lopez Casado, linux-input, linux-kernel, Benjamin Tissoires

This reverts commit 044ee890286153a1aefb40cb8b6659921aecb38b.

It turns out the current API is not that compatible with
some Microsoft mice, so better start again from scratch.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 43 +++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 44ea8e7c71a9..28ee2ed88a1a 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1858,30 +1858,31 @@ EXPORT_SYMBOL_GPL(hidinput_disconnect);
 void hid_scroll_counter_handle_scroll(struct hid_scroll_counter *counter,
 				      int hi_res_value)
 {
-	int low_res_value, remainder, multiplier;
+	int low_res_scroll_amount;
+	/* Some wheels will rest 7/8ths of a notch from the previous notch
+	 * after slow movement, so we want the threshold for low-res events to
+	 * be in the middle of the notches (e.g. after 4/8ths) as opposed to on
+	 * the notches themselves (8/8ths).
+	 */
+	int threshold = counter->resolution_multiplier / 2;
 
 	input_report_rel(counter->dev, REL_WHEEL_HI_RES,
 			 hi_res_value * counter->microns_per_hi_res_unit);
 
-	/*
-	 * Update the low-res remainder with the high-res value,
-	 * but reset if the direction has changed.
-	 */
-	remainder = counter->remainder;
-	if ((remainder ^ hi_res_value) < 0)
-		remainder = 0;
-	remainder += hi_res_value;
-
-	/*
-	 * Then just use the resolution multiplier to see if
-	 * we should send a low-res (aka regular wheel) event.
-	 */
-	multiplier = counter->resolution_multiplier;
-	low_res_value = remainder / multiplier;
-	remainder -= low_res_value * multiplier;
-	counter->remainder = remainder;
-
-	if (low_res_value)
-		input_report_rel(counter->dev, REL_WHEEL, low_res_value);
+	counter->remainder += hi_res_value;
+	if (abs(counter->remainder) >= threshold) {
+		/* Add (or subtract) 1 because we want to trigger when the wheel
+		 * is half-way to the next notch (i.e. scroll 1 notch after a
+		 * 1/2 notch movement, 2 notches after a 1 1/2 notch movement,
+		 * etc.).
+		 */
+		low_res_scroll_amount =
+			counter->remainder / counter->resolution_multiplier
+			+ (hi_res_value > 0 ? 1 : -1);
+		input_report_rel(counter->dev, REL_WHEEL,
+				 low_res_scroll_amount);
+		counter->remainder -=
+			low_res_scroll_amount * counter->resolution_multiplier;
+	}
 }
 EXPORT_SYMBOL_GPL(hid_scroll_counter_handle_scroll);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/7] Revert "HID: logitech: fix a used uninitialized GCC warning"
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 1/7] Revert "HID: input: simplify/fix high-res scroll event handling" Benjamin Tissoires
@ 2018-11-21 15:27 ` Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 3/7] Revert "HID: logitech: Use LDJ_DEVICE macro for existing Logitech mice" Benjamin Tissoires
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2018-11-21 15:27 UTC (permalink / raw)
  To: Dmitry Torokhov, Jiri Kosina, Harry Cutts, Peter Hutterer, torvalds
  Cc: Nestor Lopez Casado, linux-input, linux-kernel, Benjamin Tissoires

This reverts commit 5fe2ccbef9d7aecf5c4402c753444f1a12096cfd.

It turns out the current API is not that compatible with
some Microsoft mice, so better start again from scratch.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index f01280898b24..5f0c080059c6 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1231,6 +1231,7 @@ static int hidpp_hrw_get_wheel_capability(struct hidpp_device *hidpp,
 	*multiplier = response.fap.params[0];
 	return 0;
 return_default:
+	*multiplier = 8;
 	hid_warn(hidpp->hid_dev,
 		 "Couldn't get wheel multiplier (error %d), assuming %d.\n",
 		 ret, *multiplier);
@@ -2695,7 +2696,7 @@ static int hi_res_scroll_look_up_microns(__u32 product_id)
 static int hi_res_scroll_enable(struct hidpp_device *hidpp)
 {
 	int ret;
-	u8 multiplier = 8;
+	u8 multiplier;
 
 	if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_X2121) {
 		ret = hidpp_hrw_set_wheel_mode(hidpp, false, true, false);
@@ -2703,9 +2704,10 @@ static int hi_res_scroll_enable(struct hidpp_device *hidpp)
 	} else if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_X2120) {
 		ret = hidpp_hrs_set_highres_scrolling_mode(hidpp, true,
 							   &multiplier);
-	} else /* if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_1P0) */
+	} else /* if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_1P0) */ {
 		ret = hidpp10_enable_scrolling_acceleration(hidpp);
-
+		multiplier = 8;
+	}
 	if (ret)
 		return ret;
 
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/7] Revert "HID: logitech: Use LDJ_DEVICE macro for existing Logitech mice"
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 1/7] Revert "HID: input: simplify/fix high-res scroll event handling" Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 2/7] Revert "HID: logitech: fix a used uninitialized GCC warning" Benjamin Tissoires
@ 2018-11-21 15:27 ` Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 4/7] Revert "HID: logitech: Enable high-resolution scrolling on " Benjamin Tissoires
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2018-11-21 15:27 UTC (permalink / raw)
  To: Dmitry Torokhov, Jiri Kosina, Harry Cutts, Peter Hutterer, torvalds
  Cc: Nestor Lopez Casado, linux-input, linux-kernel, Benjamin Tissoires

This reverts commit 3fe1d6bbcd16f384d2c7dab2caf8e4b2df9ea7e6.

It turns out the current API is not that compatible with
some Microsoft mice, so better start again from scratch.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5f0c080059c6..fd6a8c325fa0 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3314,11 +3314,13 @@ static void hidpp_remove(struct hid_device *hdev)
 
 static const struct hid_device_id hidpp_devices[] = {
 	{ /* wireless touchpad */
-	  LDJ_DEVICE(0x4011),
+	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
+		USB_VENDOR_ID_LOGITECH, 0x4011),
 	  .driver_data = HIDPP_QUIRK_CLASS_WTP | HIDPP_QUIRK_DELAYED_INIT |
 			 HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS },
 	{ /* wireless touchpad T650 */
-	  LDJ_DEVICE(0x4101),
+	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
+		USB_VENDOR_ID_LOGITECH, 0x4101),
 	  .driver_data = HIDPP_QUIRK_CLASS_WTP | HIDPP_QUIRK_DELAYED_INIT },
 	{ /* wireless touchpad T651 */
 	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
@@ -3358,13 +3360,16 @@ static const struct hid_device_id hidpp_devices[] = {
 	{ /* Mouse Logitech Performance MX */
 	  LDJ_DEVICE(0x101a), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_1P0 },
 	{ /* Keyboard logitech K400 */
-	  LDJ_DEVICE(0x4024),
+	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
+		USB_VENDOR_ID_LOGITECH, 0x4024),
 	  .driver_data = HIDPP_QUIRK_CLASS_K400 },
 	{ /* Solar Keyboard Logitech K750 */
-	  LDJ_DEVICE(0x4002),
+	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
+		USB_VENDOR_ID_LOGITECH, 0x4002),
 	  .driver_data = HIDPP_QUIRK_CLASS_K750 },
 
-	{ LDJ_DEVICE(HID_ANY_ID) },
+	{ HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
+		USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},
 
 	{ 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.19.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/7] Revert "HID: logitech: Enable high-resolution scrolling on Logitech mice"
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
                   ` (2 preceding siblings ...)
  2018-11-21 15:27 ` [PATCH 3/7] Revert "HID: logitech: Use LDJ_DEVICE macro for existing Logitech mice" Benjamin Tissoires
@ 2018-11-21 15:27 ` Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 5/7] Revert "HID: logitech: Add function to enable HID++ 1.0 "scrolling acceleration"" Benjamin Tissoires
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2018-11-21 15:27 UTC (permalink / raw)
  To: Dmitry Torokhov, Jiri Kosina, Harry Cutts, Peter Hutterer, torvalds
  Cc: Nestor Lopez Casado, linux-input, linux-kernel, Benjamin Tissoires

This reverts commit d56ca9855bf924f3bc9807a3e42f38539df3f41f.

It turns out the current API is not that compatible with
some Microsoft mice, so better start again from scratch.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index fd6a8c325fa0..7f8218f6ff56 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -64,14 +64,6 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_NO_HIDINPUT			BIT(23)
 #define HIDPP_QUIRK_FORCE_OUTPUT_REPORTS	BIT(24)
 #define HIDPP_QUIRK_UNIFYING			BIT(25)
-#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)
-
-/* Convenience constant to check for any high-res support. */
-#define HIDPP_QUIRK_HI_RES_SCROLL	(HIDPP_QUIRK_HI_RES_SCROLL_1P0 | \
-					 HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
-					 HIDPP_QUIRK_HI_RES_SCROLL_X2121)
 
 #define HIDPP_QUIRK_DELAYED_INIT		HIDPP_QUIRK_NO_HIDINPUT
 
@@ -157,7 +149,6 @@ struct hidpp_device {
 	unsigned long capabilities;
 
 	struct hidpp_battery battery;
-	struct hid_scroll_counter vertical_wheel_counter;
 };
 
 /* HID++ 1.0 error codes */
@@ -1166,101 +1157,6 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 	return ret;
 }
 
-/* -------------------------------------------------------------------------- */
-/* 0x2120: Hi-resolution scrolling                                            */
-/* -------------------------------------------------------------------------- */
-
-#define HIDPP_PAGE_HI_RESOLUTION_SCROLLING			0x2120
-
-#define CMD_HI_RESOLUTION_SCROLLING_SET_HIGHRES_SCROLLING_MODE	0x10
-
-static int hidpp_hrs_set_highres_scrolling_mode(struct hidpp_device *hidpp,
-	bool enabled, u8 *multiplier)
-{
-	u8 feature_index;
-	u8 feature_type;
-	int ret;
-	u8 params[1];
-	struct hidpp_report response;
-
-	ret = hidpp_root_get_feature(hidpp,
-				     HIDPP_PAGE_HI_RESOLUTION_SCROLLING,
-				     &feature_index,
-				     &feature_type);
-	if (ret)
-		return ret;
-
-	params[0] = enabled ? BIT(0) : 0;
-	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
-					  CMD_HI_RESOLUTION_SCROLLING_SET_HIGHRES_SCROLLING_MODE,
-					  params, sizeof(params), &response);
-	if (ret)
-		return ret;
-	*multiplier = response.fap.params[1];
-	return 0;
-}
-
-/* -------------------------------------------------------------------------- */
-/* 0x2121: HiRes Wheel                                                        */
-/* -------------------------------------------------------------------------- */
-
-#define HIDPP_PAGE_HIRES_WHEEL		0x2121
-
-#define CMD_HIRES_WHEEL_GET_WHEEL_CAPABILITY	0x00
-#define CMD_HIRES_WHEEL_SET_WHEEL_MODE		0x20
-
-static int hidpp_hrw_get_wheel_capability(struct hidpp_device *hidpp,
-	u8 *multiplier)
-{
-	u8 feature_index;
-	u8 feature_type;
-	int ret;
-	struct hidpp_report response;
-
-	ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_HIRES_WHEEL,
-				     &feature_index, &feature_type);
-	if (ret)
-		goto return_default;
-
-	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
-					  CMD_HIRES_WHEEL_GET_WHEEL_CAPABILITY,
-					  NULL, 0, &response);
-	if (ret)
-		goto return_default;
-
-	*multiplier = response.fap.params[0];
-	return 0;
-return_default:
-	*multiplier = 8;
-	hid_warn(hidpp->hid_dev,
-		 "Couldn't get wheel multiplier (error %d), assuming %d.\n",
-		 ret, *multiplier);
-	return ret;
-}
-
-static int hidpp_hrw_set_wheel_mode(struct hidpp_device *hidpp, bool invert,
-	bool high_resolution, bool use_hidpp)
-{
-	u8 feature_index;
-	u8 feature_type;
-	int ret;
-	u8 params[1];
-	struct hidpp_report response;
-
-	ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_HIRES_WHEEL,
-				     &feature_index, &feature_type);
-	if (ret)
-		return ret;
-
-	params[0] = (invert          ? BIT(2) : 0) |
-		    (high_resolution ? BIT(1) : 0) |
-		    (use_hidpp       ? BIT(0) : 0);
-
-	return hidpp_send_fap_command_sync(hidpp, feature_index,
-					   CMD_HIRES_WHEEL_SET_WHEEL_MODE,
-					   params, sizeof(params), &response);
-}
-
 /* -------------------------------------------------------------------------- */
 /* 0x4301: Solar Keyboard                                                     */
 /* -------------------------------------------------------------------------- */
@@ -2524,8 +2420,7 @@ static int m560_raw_event(struct hid_device *hdev, u8 *data, int size)
 		input_report_rel(mydata->input, REL_Y, v);
 
 		v = hid_snto32(data[6], 8);
-		hid_scroll_counter_handle_scroll(
-				&hidpp->vertical_wheel_counter, v);
+		input_report_rel(mydata->input, REL_WHEEL, v);
 
 		input_sync(mydata->input);
 	}
@@ -2653,73 +2548,6 @@ static int g920_get_config(struct hidpp_device *hidpp)
 	return 0;
 }
 
-/* -------------------------------------------------------------------------- */
-/* High-resolution scroll wheels                                              */
-/* -------------------------------------------------------------------------- */
-
-/**
- * struct hi_res_scroll_info - Stores info on a device's high-res scroll wheel.
- * @product_id: the HID product ID of the device being described.
- * @microns_per_hi_res_unit: the distance moved by the user's finger for each
- *                         high-resolution unit reported by the device, in
- *                         256ths of a millimetre.
- */
-struct hi_res_scroll_info {
-	__u32 product_id;
-	int microns_per_hi_res_unit;
-};
-
-static struct hi_res_scroll_info hi_res_scroll_devices[] = {
-	{ /* Anywhere MX */
-	  .product_id = 0x1017, .microns_per_hi_res_unit = 445 },
-	{ /* Performance MX */
-	  .product_id = 0x101a, .microns_per_hi_res_unit = 406 },
-	{ /* M560 */
-	  .product_id = 0x402d, .microns_per_hi_res_unit = 435 },
-	{ /* MX Master 2S */
-	  .product_id = 0x4069, .microns_per_hi_res_unit = 406 },
-};
-
-static int hi_res_scroll_look_up_microns(__u32 product_id)
-{
-	int i;
-	int num_devices = sizeof(hi_res_scroll_devices)
-			  / sizeof(hi_res_scroll_devices[0]);
-	for (i = 0; i < num_devices; i++) {
-		if (hi_res_scroll_devices[i].product_id == product_id)
-			return hi_res_scroll_devices[i].microns_per_hi_res_unit;
-	}
-	/* We don't have a value for this device, so use a sensible default. */
-	return 406;
-}
-
-static int hi_res_scroll_enable(struct hidpp_device *hidpp)
-{
-	int ret;
-	u8 multiplier;
-
-	if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_X2121) {
-		ret = hidpp_hrw_set_wheel_mode(hidpp, false, true, false);
-		hidpp_hrw_get_wheel_capability(hidpp, &multiplier);
-	} else if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_X2120) {
-		ret = hidpp_hrs_set_highres_scrolling_mode(hidpp, true,
-							   &multiplier);
-	} else /* if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_1P0) */ {
-		ret = hidpp10_enable_scrolling_acceleration(hidpp);
-		multiplier = 8;
-	}
-	if (ret)
-		return ret;
-
-	hidpp->vertical_wheel_counter.resolution_multiplier = multiplier;
-	hidpp->vertical_wheel_counter.microns_per_hi_res_unit =
-		hi_res_scroll_look_up_microns(hidpp->hid_dev->product);
-	hid_info(hidpp->hid_dev, "multiplier = %d, microns = %d\n",
-		 multiplier,
-		 hidpp->vertical_wheel_counter.microns_per_hi_res_unit);
-	return 0;
-}
-
 /* -------------------------------------------------------------------------- */
 /* Generic HID++ devices                                                      */
 /* -------------------------------------------------------------------------- */
@@ -2765,11 +2593,6 @@ static void hidpp_populate_input(struct hidpp_device *hidpp,
 		wtp_populate_input(hidpp, input, origin_is_hid_core);
 	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560)
 		m560_populate_input(hidpp, input, origin_is_hid_core);
-
-	if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL) {
-		input_set_capability(input, EV_REL, REL_WHEEL_HI_RES);
-		hidpp->vertical_wheel_counter.dev = input;
-	}
 }
 
 static int hidpp_input_configured(struct hid_device *hdev,
@@ -2888,27 +2711,6 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 	return 0;
 }
 
-static int hidpp_event(struct hid_device *hdev, struct hid_field *field,
-	struct hid_usage *usage, __s32 value)
-{
-	/* This function will only be called for scroll events, due to the
-	 * restriction imposed in hidpp_usages.
-	 */
-	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
-	struct hid_scroll_counter *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
-	 * avoid a crash in hid_scroll_counter_handle_scroll.
-	 */
-	if (!(hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL) || value == 0
-	    || counter->dev == NULL || counter->resolution_multiplier == 0)
-		return 0;
-
-	hid_scroll_counter_handle_scroll(counter, value);
-	return 1;
-}
-
 static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 {
 	static atomic_t battery_no = ATOMIC_INIT(0);
@@ -3120,9 +2922,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 	if (hidpp->battery.ps)
 		power_supply_changed(hidpp->battery.ps);
 
-	if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL)
-		hi_res_scroll_enable(hidpp);
-
 	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) || hidpp->delayed_input)
 		/* if the input nodes are already created, we can stop now */
 		return;
@@ -3308,10 +3107,6 @@ static void hidpp_remove(struct hid_device *hdev)
 	mutex_destroy(&hidpp->send_mutex);
 }
 
-#define LDJ_DEVICE(product) \
-	HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE, \
-		   USB_VENDOR_ID_LOGITECH, (product))
-
 static const struct hid_device_id hidpp_devices[] = {
 	{ /* wireless touchpad */
 	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
@@ -3326,39 +3121,10 @@ static const struct hid_device_id hidpp_devices[] = {
 	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
 		USB_DEVICE_ID_LOGITECH_T651),
 	  .driver_data = HIDPP_QUIRK_CLASS_WTP },
-	{ /* Mouse Logitech Anywhere MX */
-	  LDJ_DEVICE(0x1017), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_1P0 },
-	{ /* Mouse Logitech Cube */
-	  LDJ_DEVICE(0x4010), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2120 },
-	{ /* Mouse Logitech M335 */
-	  LDJ_DEVICE(0x4050), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ /* Mouse Logitech M515 */
-	  LDJ_DEVICE(0x4007), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2120 },
 	{ /* Mouse logitech M560 */
-	  LDJ_DEVICE(0x402d),
-	  .driver_data = HIDPP_QUIRK_DELAYED_INIT | HIDPP_QUIRK_CLASS_M560
-		| HIDPP_QUIRK_HI_RES_SCROLL_X2120 },
-	{ /* Mouse Logitech M705 (firmware RQM17) */
-	  LDJ_DEVICE(0x101b), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_1P0 },
-	{ /* Mouse Logitech M705 (firmware RQM67) */
-	  LDJ_DEVICE(0x406d), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ /* Mouse Logitech M720 */
-	  LDJ_DEVICE(0x405e), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ /* Mouse Logitech MX Anywhere 2 */
-	  LDJ_DEVICE(0x404a), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ LDJ_DEVICE(0xb013), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ LDJ_DEVICE(0xb018), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ LDJ_DEVICE(0xb01f), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ /* Mouse Logitech MX Anywhere 2S */
-	  LDJ_DEVICE(0x406a), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ /* Mouse Logitech MX Master */
-	  LDJ_DEVICE(0x4041), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ LDJ_DEVICE(0x4060), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ LDJ_DEVICE(0x4071), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ /* Mouse Logitech MX Master 2S */
-	  LDJ_DEVICE(0x4069), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
-	{ /* Mouse Logitech Performance MX */
-	  LDJ_DEVICE(0x101a), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_1P0 },
+	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
+		USB_VENDOR_ID_LOGITECH, 0x402d),
+	  .driver_data = HIDPP_QUIRK_DELAYED_INIT | HIDPP_QUIRK_CLASS_M560 },
 	{ /* Keyboard logitech K400 */
 	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
 		USB_VENDOR_ID_LOGITECH, 0x4024),
@@ -3378,19 +3144,12 @@ static const struct hid_device_id hidpp_devices[] = {
 
 MODULE_DEVICE_TABLE(hid, hidpp_devices);
 
-static const struct hid_usage_id hidpp_usages[] = {
-	{ HID_GD_WHEEL, EV_REL, REL_WHEEL },
-	{ HID_ANY_ID - 1, HID_ANY_ID - 1, HID_ANY_ID - 1}
-};
-
 static struct hid_driver hidpp_driver = {
 	.name = "logitech-hidpp-device",
 	.id_table = hidpp_devices,
 	.probe = hidpp_probe,
 	.remove = hidpp_remove,
 	.raw_event = hidpp_raw_event,
-	.usage_table = hidpp_usages,
-	.event = hidpp_event,
 	.input_configured = hidpp_input_configured,
 	.input_mapping = hidpp_input_mapping,
 	.input_mapped = hidpp_input_mapped,
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/7] Revert "HID: logitech: Add function to enable HID++ 1.0 "scrolling acceleration""
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
                   ` (3 preceding siblings ...)
  2018-11-21 15:27 ` [PATCH 4/7] Revert "HID: logitech: Enable high-resolution scrolling on " Benjamin Tissoires
@ 2018-11-21 15:27 ` Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 6/7] Revert "HID: input: Create a utility class for counting scroll events" Benjamin Tissoires
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2018-11-21 15:27 UTC (permalink / raw)
  To: Dmitry Torokhov, Jiri Kosina, Harry Cutts, Peter Hutterer, torvalds
  Cc: Nestor Lopez Casado, linux-input, linux-kernel, Benjamin Tissoires

This reverts commit 051dc9b0579602bd63e9df74d0879b5293e71581.

It turns out the current API is not that compatible with
some Microsoft mice, so better start again from scratch.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 7f8218f6ff56..19cc980eebce 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -400,53 +400,32 @@ static void hidpp_prefix_name(char **name, int name_length)
 #define HIDPP_SET_LONG_REGISTER				0x82
 #define HIDPP_GET_LONG_REGISTER				0x83
 
-/**
- * hidpp10_set_register_bit() - Sets a single bit in a HID++ 1.0 register.
- * @hidpp_dev: the device to set the register on.
- * @register_address: the address of the register to modify.
- * @byte: the byte of the register to modify. Should be less than 3.
- * Return: 0 if successful, otherwise a negative error code.
- */
-static int hidpp10_set_register_bit(struct hidpp_device *hidpp_dev,
-	u8 register_address, u8 byte, u8 bit)
+#define HIDPP_REG_GENERAL				0x00
+
+static int hidpp10_enable_battery_reporting(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,
-					  register_address,
-					  NULL, 0, &response);
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_GET_REGISTER,
+					HIDPP_REG_GENERAL,
+					NULL, 0, &response);
 	if (ret)
 		return ret;
 
 	memcpy(params, response.rap.params, 3);
 
-	params[byte] |= BIT(bit);
+	/* Set the battery bit */
+	params[0] |= BIT(4);
 
 	return hidpp_send_rap_command_sync(hidpp_dev,
-					   REPORT_ID_HIDPP_SHORT,
-					   HIDPP_SET_REGISTER,
-					   register_address,
-					   params, 3, &response);
-}
-
-
-#define HIDPP_REG_GENERAL				0x00
-
-static int hidpp10_enable_battery_reporting(struct hidpp_device *hidpp_dev)
-{
-	return hidpp10_set_register_bit(hidpp_dev, HIDPP_REG_GENERAL, 0, 4);
-}
-
-#define HIDPP_REG_FEATURES				0x01
-
-/* On HID++ 1.0 devices, high-res scroll was called "scrolling acceleration". */
-static int hidpp10_enable_scrolling_acceleration(struct hidpp_device *hidpp_dev)
-{
-	return hidpp10_set_register_bit(hidpp_dev, HIDPP_REG_FEATURES, 0, 6);
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_SET_REGISTER,
+					HIDPP_REG_GENERAL,
+					params, 3, &response);
 }
 
 #define HIDPP_REG_BATTERY_STATUS			0x07
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 6/7] Revert "HID: input: Create a utility class for counting scroll events"
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
                   ` (4 preceding siblings ...)
  2018-11-21 15:27 ` [PATCH 5/7] Revert "HID: logitech: Add function to enable HID++ 1.0 "scrolling acceleration"" Benjamin Tissoires
@ 2018-11-21 15:27 ` Benjamin Tissoires
  2018-11-21 15:27 ` [PATCH 7/7] Revert "Input: Add the `REL_WHEEL_HI_RES` event code" Benjamin Tissoires
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2018-11-21 15:27 UTC (permalink / raw)
  To: Dmitry Torokhov, Jiri Kosina, Harry Cutts, Peter Hutterer, torvalds
  Cc: Nestor Lopez Casado, linux-input, linux-kernel, Benjamin Tissoires

This reverts commit 1ff2e1a44e02d4bdbb9be67c7d9acc240a67141f.

It turns out the current API is not that compatible with
some Microsoft mice, so better start again from scratch.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 45 -----------------------------------------
 include/linux/hid.h     | 28 -------------------------
 2 files changed, 73 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 28ee2ed88a1a..d6fab5798487 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1841,48 +1841,3 @@ void hidinput_disconnect(struct hid_device *hid)
 }
 EXPORT_SYMBOL_GPL(hidinput_disconnect);
 
-/**
- * hid_scroll_counter_handle_scroll() - Send high- and low-resolution scroll
- *                                      events given a high-resolution wheel
- *                                      movement.
- * @counter: a hid_scroll_counter struct describing the wheel.
- * @hi_res_value: the movement of the wheel, in the mouse's high-resolution
- *                units.
- *
- * Given a high-resolution movement, this function converts the movement into
- * microns and emits high-resolution scroll events for the input device. It also
- * uses the multiplier from &struct hid_scroll_counter to emit low-resolution
- * scroll events when appropriate for backwards-compatibility with userspace
- * input libraries.
- */
-void hid_scroll_counter_handle_scroll(struct hid_scroll_counter *counter,
-				      int hi_res_value)
-{
-	int low_res_scroll_amount;
-	/* Some wheels will rest 7/8ths of a notch from the previous notch
-	 * after slow movement, so we want the threshold for low-res events to
-	 * be in the middle of the notches (e.g. after 4/8ths) as opposed to on
-	 * the notches themselves (8/8ths).
-	 */
-	int threshold = counter->resolution_multiplier / 2;
-
-	input_report_rel(counter->dev, REL_WHEEL_HI_RES,
-			 hi_res_value * counter->microns_per_hi_res_unit);
-
-	counter->remainder += hi_res_value;
-	if (abs(counter->remainder) >= threshold) {
-		/* Add (or subtract) 1 because we want to trigger when the wheel
-		 * is half-way to the next notch (i.e. scroll 1 notch after a
-		 * 1/2 notch movement, 2 notches after a 1 1/2 notch movement,
-		 * etc.).
-		 */
-		low_res_scroll_amount =
-			counter->remainder / counter->resolution_multiplier
-			+ (hi_res_value > 0 ? 1 : -1);
-		input_report_rel(counter->dev, REL_WHEEL,
-				 low_res_scroll_amount);
-		counter->remainder -=
-			low_res_scroll_amount * counter->resolution_multiplier;
-	}
-}
-EXPORT_SYMBOL_GPL(hid_scroll_counter_handle_scroll);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 387c70df6f29..a355d61940f2 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1139,34 +1139,6 @@ static inline u32 hid_report_len(struct hid_report *report)
 int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
 		int interrupt);
 
-
-/**
- * struct hid_scroll_counter - Utility class for processing high-resolution
- *                             scroll events.
- * @dev: the input device for which events should be reported.
- * @microns_per_hi_res_unit: the amount moved by the user's finger for each
- *                           high-resolution unit reported by the mouse, in
- *                           microns.
- * @resolution_multiplier: the wheel's resolution in high-resolution mode as a
- *                         multiple of its lower resolution. For example, if
- *                         moving the wheel by one "notch" would result in a
- *                         value of 1 in low-resolution mode but 8 in
- *                         high-resolution, the multiplier is 8.
- * @remainder: counts the number of high-resolution units moved since the last
- *             low-resolution event (REL_WHEEL or REL_HWHEEL) was sent. Should
- *             only be used by class methods.
- */
-struct hid_scroll_counter {
-	struct input_dev *dev;
-	int microns_per_hi_res_unit;
-	int resolution_multiplier;
-
-	int remainder;
-};
-
-void hid_scroll_counter_handle_scroll(struct hid_scroll_counter *counter,
-				      int hi_res_value);
-
 /* HID quirks API */
 unsigned long hid_lookup_quirk(const struct hid_device *hdev);
 int hid_quirks_init(char **quirks_param, __u16 bus, int count);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 7/7] Revert "Input: Add the `REL_WHEEL_HI_RES` event code"
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
                   ` (5 preceding siblings ...)
  2018-11-21 15:27 ` [PATCH 6/7] Revert "HID: input: Create a utility class for counting scroll events" Benjamin Tissoires
@ 2018-11-21 15:27 ` Benjamin Tissoires
  2018-11-21 18:37 ` [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Harry Cutts
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2018-11-21 15:27 UTC (permalink / raw)
  To: Dmitry Torokhov, Jiri Kosina, Harry Cutts, Peter Hutterer, torvalds
  Cc: Nestor Lopez Casado, linux-input, linux-kernel, Benjamin Tissoires

This reverts commit aaf9978c3c0291ef3beaa97610bc9c3084656a85.

Quoting Peter:

There is a HID feature report called "Resolution Multiplier"
Described in the "Enhanced Wheel Support in Windows" doc and
the "USB HID Usage Tables" page 30.

http://download.microsoft.com/download/b/d/1/bd1f7ef4-7d72-419e-bc5c-9f79ad7bb66e/wheel.docx
https://www.usb.org/sites/default/files/documents/hut1_12v2.pdf

This was new for Windows Vista, so we're only a decade behind here. I only
accidentally found this a few days ago while debugging a stuck button on a
Microsoft mouse.

The docs above describe it like this: a wheel control by default sends
value 1 per notch. If the resolution multiplier is active, the wheel is
expected to send a value of $multiplier per notch (e.g. MS Sculpt mouse) or
just send events more often, i.e. for less physical motion (e.g. MS Comfort
mouse).

For the latter, you need the right HW of course. The Sculpt mouse has
tactile wheel clicks, so nothing really changes. The Comfort mouse has
continuous motion with no tactile clicks. Similar to the free-wheeling
Logitech mice but without any inertia.

Note that the doc also says that Vista and onwards *always* enable this
feature where available.

An example HID definition looks like this:

       Usage Page Generic Desktop (0x01)
       Usage Resolution Multiplier (0x48)
       Logical Minimum 0
       Logical Maximum 1
       Physical Minimum 1
       Physical Maximum 16
       Report Size 2 # in bits
       Report Count 1
       Feature (Data, Var, Abs)

So the actual bits have values 0 or 1 and that reflects real values 1 or 16.
We've only seen single-bits so far, so there's low-res and hi-res, but
nothing in between.

The multiplier is available for HID usages "Wheel" and "AC Pan" (horiz wheel).
Microsoft suggests that

> Vendors should ship their devices with smooth scrolling disabled and allow
> Windows to enable it. This ensures that the device works like a regular HID
> device on legacy operating systems that do not support smooth scrolling.
(see the wheel doc linked above)

The mice that we tested so far do reset on unplug.

Device Support looks to be all (?) Microsoft mice but nothing else

Not supported:
- Logitech G500s, G303
- Roccat Kone XTD
- all the cheap Lenovo, HP, Dell, Logitech USB mice that come with a
  workstation that I could find don't have it.
- Etekcity something something
- Razer Imperator

Supported:
- Microsoft Comfort Optical Mouse 3000 - yes, physical: 1:4
- Microsoft Sculpt Ergonomic Mouse - yes, physical: 1:12
- Microsoft Surface mouse - yes, physical: 1:4

So again, I think this is really just available on Microsoft mice, but
probably all decent MS mice released over the last decade.

Looking at the hardware itself:

- no noticeable notches in the weel
- low-res: 18 events per 360deg rotation (click angle 20 deg)
- high-res: 72 events per 360deg → matches multiplier of 4

- I can feel the notches during wheel turns
- low-res: 24 events per 360 deg rotation (click angle 15 deg)
  - horiz wheel is tilt-based, continuous output value 1
- high-res: 24 events per 360deg with value 12 → matches multiplier of 12
  - horiz wheel output rate doubles/triples?, values is 3

- It's a touch strip, not a wheel so no notches
- high-res: events have value 4 instead of 1
  a bit strange given that it doesn't actually have notches.

Ok, why is this an issue for the current API? First, because the logitech
multiplier used in Harry's patches looks suspiciously like the Resolution
Multiplier so I think we should assume it's the same thing. Nestor, can you
shed some light on that?

- `REL_WHEEL` is defined as the number of notches, emulated where needed.
- `REL_WHEEL_HI_RES` is the movement of the user's finger in microns.
- `WM_MOUSEWHEEL` (Windows) is is a multiple of 120, defined as "the threshold
  for action to be taken and one such action"
  https://docs.microsoft.com/en-us/windows/desktop/inputdev/wm-mousewheel

If the multiplier is set to M, this means we need an accumulated value of M
until we can claim there was a wheel click. So after enabling the multiplier
and setting it to the maximum (like Windows):
- M units are 15deg rotation → 1 unit is 2620/M micron (see below). This is
  the `REL_WHEEL_HI_RES` value.
  - wheel diameter 20mm: 15 deg rotation is 2.62mm, 2620 micron (pi * 20mm /
    (360deg/15deg))
- For every M units accumulated, send one `REL_WHEEL` event

The problem here is that we've now hardcoded 20mm/15 deg into the kernel and
we have no way of getting the size of the wheel or the click angle into the
kernel.

In userspace we now have to undo the kernel's calculation. If our click angle
is e.g. 20 degree we have to undo the (lossy) calculation from the kernel and
calculate the correct angle instead. This also means the 15 is a hardcoded
option forever and cannot be changed.

In hid-logitech-hidpp.c, the microns per unit is hardcoded per device.
Harry, did you measure those by hand? We'd need to update the kernel for
every device and there are 10 years worth of devices from MS alone.

The multiplier default is 8 which is in the right ballpark, so I'm pretty
sure this is the same as the Resolution Multiplier, just in HID++ lingo. And
given that the 120 magic factor is what Windows uses in the end, I can't
imagine Logitech rolling their own thing here. Nestor?

And we're already fairly inaccurate with the microns anyway. The MX Anywhere
2S has a click angle of 20 degrees (18 stops) and a 17mm wheel, so a wheel
notch is approximately 2.67mm, one event at multiplier 8 (1/8 of a notch)
would be 334 micron. That's only 80% of the fallback value of 406 in the
kernel. Multiplier 6 gives us 445micron (10% off). I'm assuming multiplier 7
doesn't exist because it's not a factor of 120.

Summary:

Best option may be to simply do what Windows is doing, all the HW manufacturers
have to use that approach after all. Switch `REL_WHEEL_HI_RES` to report in
fractions of 120, with 120 being one notch and divide that by the multiplier
for the actual events. So e.g. the Logitech multiplier 8 would send value 15
for each event in hi-res mode. This can be converted in userspace to
whatever userspace needs (combined with a hwdb there that tells you wheel
size/click angle/...).

Conflicts:
	include/uapi/linux/input-event-codes.h -> I kept the new
         reserved event in the code, so I had to adapt the revert
         slightly

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 Documentation/input/event-codes.rst    | 11 +----------
 include/uapi/linux/input-event-codes.h | 10 ----------
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
index cef220c176a4..a8c0873beb95 100644
--- a/Documentation/input/event-codes.rst
+++ b/Documentation/input/event-codes.rst
@@ -190,16 +190,7 @@ A few EV_REL codes have special meanings:
 * REL_WHEEL, REL_HWHEEL:
 
   - These codes are used for vertical and horizontal scroll wheels,
-    respectively. The value is the number of "notches" moved on the wheel, the
-    physical size of which varies by device. For high-resolution wheels (which
-    report multiple events for each notch of movement, or do not have notches)
-    this may be an approximation based on the high-resolution scroll events.
-
-* REL_WHEEL_HI_RES:
-
-  - If a vertical scroll wheel supports high-resolution scrolling, this code
-    will be emitted in addition to REL_WHEEL. The value is the (approximate)
-    distance travelled by the user's finger, in microns.
+    respectively.
 
 EV_ABS
 ------
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 6d180cc60a5d..3eb5a4c3d60a 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -716,7 +716,6 @@
  * the situation described above.
  */
 #define REL_RESERVED		0x0a
-#define REL_WHEEL_HI_RES	0x0b
 #define REL_MAX			0x0f
 #define REL_CNT			(REL_MAX+1)
 
@@ -753,15 +752,6 @@
 
 #define ABS_MISC		0x28
 
-/*
- * 0x2e is reserved and should not be used in input drivers.
- * It was used by HID as ABS_MISC+6 and userspace needs to detect if
- * the next ABS_* event is correct or is just ABS_MISC + n.
- * We define here ABS_RESERVED so userspace can rely on it and detect
- * the situation described above.
- */
-#define ABS_RESERVED		0x2e
-
 #define ABS_MT_SLOT		0x2f	/* MT slot being modified */
 #define ABS_MT_TOUCH_MAJOR	0x30	/* Major axis of touching ellipse */
 #define ABS_MT_TOUCH_MINOR	0x31	/* Minor axis (omit if circular) */
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/7] HID: revert the Logitech High Resolution wheel support
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
                   ` (6 preceding siblings ...)
  2018-11-21 15:27 ` [PATCH 7/7] Revert "Input: Add the `REL_WHEEL_HI_RES` event code" Benjamin Tissoires
@ 2018-11-21 18:37 ` Harry Cutts
  2018-11-21 18:44   ` Dmitry Torokhov
  2018-11-21 18:46 ` Jiri Kosina
  2018-11-22  8:01 ` Benjamin Tissoires
  9 siblings, 1 reply; 12+ messages in thread
From: Harry Cutts @ 2018-11-21 18:37 UTC (permalink / raw)
  To: benjamin.tissoires
  Cc: Dmitry Torokhov, jikos, Peter Hutterer, torvalds,
	Nestor Lopez Casado, linux-input, linux-kernel

On Wed, 21 Nov 2018 at 07:27, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> The TL;DR, we need to revert the current series before it gets out in
> a released kernel and work on a better approach for 4.21.
>
> This patch series has informally been acked by Dmitry, Harry, Jiri, Nestor
> and Peter, but I wouldn't mind a public ack before I push this to
> the for-linus branch.

Thanks Benjamin!

For the series:
Acked-by: Harry Cutts <hcutts@chromium.org>

Harry Cutts
Chrome OS Touch/Input team

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/7] HID: revert the Logitech High Resolution wheel support
  2018-11-21 18:37 ` [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Harry Cutts
@ 2018-11-21 18:44   ` Dmitry Torokhov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2018-11-21 18:44 UTC (permalink / raw)
  To: Harry Cutts
  Cc: benjamin.tissoires, jikos, Peter Hutterer, torvalds,
	Nestor Lopez Casado, linux-input, linux-kernel

On Wed, Nov 21, 2018 at 10:37:13AM -0800, Harry Cutts wrote:
> On Wed, 21 Nov 2018 at 07:27, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > The TL;DR, we need to revert the current series before it gets out in
> > a released kernel and work on a better approach for 4.21.
> >
> > This patch series has informally been acked by Dmitry, Harry, Jiri, Nestor
> > and Peter, but I wouldn't mind a public ack before I push this to
> > the for-linus branch.
> 
> Thanks Benjamin!
> 
> For the series:
> Acked-by: Harry Cutts <hcutts@chromium.org>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks!

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/7] HID: revert the Logitech High Resolution wheel support
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
                   ` (7 preceding siblings ...)
  2018-11-21 18:37 ` [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Harry Cutts
@ 2018-11-21 18:46 ` Jiri Kosina
  2018-11-22  8:01 ` Benjamin Tissoires
  9 siblings, 0 replies; 12+ messages in thread
From: Jiri Kosina @ 2018-11-21 18:46 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Dmitry Torokhov, Harry Cutts, Peter Hutterer, torvalds,
	Nestor Lopez Casado, linux-input, linux-kernel

On Wed, 21 Nov 2018, Benjamin Tissoires wrote:

> It turns out that the implementation of the high resolution support of
> Logitech wheels is rather incompatible with the mice from Microsoft.
> 
> We had a lengthy discussion off-list and the summary is quoted in 7/7.
> 
> The TL;DR, we need to revert the current series before it gets out in
> a released kernel and work on a better approach for 4.21.
> 
> This patch series has informally been acked by Dmitry, Harry, Jiri, Nestor
> and Peter, but I wouldn't mind a public ack before I push this to
> the for-linus branch.

As discussed previously

	Acked-by: Jiri Kosina <jkosina@suse.cz>

Thanks,

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/7] HID: revert the Logitech High Resolution wheel support
  2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
                   ` (8 preceding siblings ...)
  2018-11-21 18:46 ` Jiri Kosina
@ 2018-11-22  8:01 ` Benjamin Tissoires
  9 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2018-11-22  8:01 UTC (permalink / raw)
  To: Dmitry Torokhov, Jiri Kosina, hcutts, Peter Hutterer, Linus Torvalds
  Cc: Nestor Lopez Casado, open list:HID CORE LAYER, lkml

On Wed, Nov 21, 2018 at 4:27 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> It turns out that the implementation of the high resolution support of
> Logitech wheels is rather incompatible with the mice from Microsoft.
>
> We had a lengthy discussion off-list and the summary is quoted in 7/7.
>
> The TL;DR, we need to revert the current series before it gets out in
> a released kernel and work on a better approach for 4.21.
>
> This patch series has informally been acked by Dmitry, Harry, Jiri, Nestor
> and Peter, but I wouldn't mind a public ack before I push this to
> the for-linus branch.
>
> Dmitry, I chose to also revert "Input: Add the `REL_WHEEL_HI_RES` event code"
> as the documentation needs to be updated.
> I would understand if you rather keep the patch that way and we just update
> the doc. This would help synchronizing the trees. So please tell me if you
> want 7/7 in the series or not (I'll reshuffle the commit message to have
> the summary from Peter).
>
> Cheers,
> Benjamin
>
> Benjamin Tissoires (7):
>   Revert "HID: input: simplify/fix high-res scroll event handling"
>   Revert "HID: logitech: fix a used uninitialized GCC warning"
>   Revert "HID: logitech: Use LDJ_DEVICE macro for existing Logitech
>     mice"
>   Revert "HID: logitech: Enable high-resolution scrolling on Logitech
>     mice"
>   Revert "HID: logitech: Add function to enable HID++ 1.0 "scrolling
>     acceleration""
>   Revert "HID: input: Create a utility class for counting scroll events"
>   Revert "Input: Add the `REL_WHEEL_HI_RES` event code"
>
>  Documentation/input/event-codes.rst    |  11 +-
>  drivers/hid/hid-input.c                |  44 ----
>  drivers/hid/hid-logitech-hidpp.c       | 309 +++----------------------
>  include/linux/hid.h                    |  28 ---
>  include/uapi/linux/input-event-codes.h |  10 -
>  5 files changed, 28 insertions(+), 374 deletions(-)
>
> --
> 2.19.1
>

The series is now applied. Thanks everybody for the quick answers.

Cheers,
Benjamin

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-11-22  8:02 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-21 15:27 [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Benjamin Tissoires
2018-11-21 15:27 ` [PATCH 1/7] Revert "HID: input: simplify/fix high-res scroll event handling" Benjamin Tissoires
2018-11-21 15:27 ` [PATCH 2/7] Revert "HID: logitech: fix a used uninitialized GCC warning" Benjamin Tissoires
2018-11-21 15:27 ` [PATCH 3/7] Revert "HID: logitech: Use LDJ_DEVICE macro for existing Logitech mice" Benjamin Tissoires
2018-11-21 15:27 ` [PATCH 4/7] Revert "HID: logitech: Enable high-resolution scrolling on " Benjamin Tissoires
2018-11-21 15:27 ` [PATCH 5/7] Revert "HID: logitech: Add function to enable HID++ 1.0 "scrolling acceleration"" Benjamin Tissoires
2018-11-21 15:27 ` [PATCH 6/7] Revert "HID: input: Create a utility class for counting scroll events" Benjamin Tissoires
2018-11-21 15:27 ` [PATCH 7/7] Revert "Input: Add the `REL_WHEEL_HI_RES` event code" Benjamin Tissoires
2018-11-21 18:37 ` [PATCH 0/7] HID: revert the Logitech High Resolution wheel support Harry Cutts
2018-11-21 18:44   ` Dmitry Torokhov
2018-11-21 18:46 ` Jiri Kosina
2018-11-22  8:01 ` Benjamin Tissoires

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).