[PATCH 0/3] Add support for high-resolution scrolling on Logitech mice

[PATCH 0/3] Add support for high-resolution scrolling on Logitech mice

[Harry Cutts]

Hi everyone,

This set of patches adds support for high-resolution scroll wheels on
Logitech mice. See the linux-input "Reporting high-resolution scroll
events" thread [0] for previous discussion of the evdev changes. I would
love to hear your feedback.

Thanks,

Harry Cutts
Chrome OS Touch/Input team

[0]: https://www.spinics.net/lists/linux-input/msg57380.html



Harry Cutts (3):
  Add the `REL_WHEEL_HI_RES` event code
  Create a utility class for counting scroll events
  Enable high-resolution scrolling on Logitech mice

 Documentation/input/event-codes.rst    |  11 +-
 drivers/hid/hid-ids.h                  |  15 ++
 drivers/hid/hid-input.c                |  44 ++++
 drivers/hid/hid-logitech-hidpp.c       | 341 +++++++++++++++++++++++--
 drivers/hid/hid-quirks.c               |  11 +
 include/linux/hid.h                    |  28 ++
 include/uapi/linux/input-event-codes.h |   1 +
 7 files changed, 423 insertions(+), 28 deletions(-)

-- 
2.18.0.1017.ga543ac7ca45-goog

[PATCH 1/3] Add the `REL_WHEEL_HI_RES` event code

[Harry Cutts]

This event code represents scroll reports from high-resolution wheels,
and will be used by future patches in this series. See the linux-input
"Reporting high-resolution scroll events" thread [0] for more details.

[0]: https://www.spinics.net/lists/linux-input/msg57380.html

Signed-off-by: Harry Cutts <hcutts@chromium.org>
---

 Documentation/input/event-codes.rst    | 11 ++++++++++-
 include/uapi/linux/input-event-codes.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
index a8c0873beb95..df024cb82829 100644
--- a/Documentation/input/event-codes.rst
+++ b/Documentation/input/event-codes.rst
@@ -190,7 +190,16 @@ A few EV_REL codes have special meanings:
 * REL_WHEEL, REL_HWHEEL:
 
   - These codes are used for vertical and horizontal scroll wheels,
-    respectively.
+    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 256ths of a millimeter.
 
 EV_ABS
 ------
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 53fbae27b280..dad8d3890a3a 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -708,6 +708,7 @@
 #define REL_DIAL		0x07
 #define REL_WHEEL		0x08
 #define REL_MISC		0x09
+#define REL_WHEEL_HI_RES	0x0a
 #define REL_MAX			0x0f
 #define REL_CNT			(REL_MAX+1)
 
-- 
2.18.0.1017.ga543ac7ca45-goog

Re: [PATCH 1/3] Add the `REL_WHEEL_HI_RES` event code

[Dmitry Torokhov]

On Thu, Aug 23, 2018 at 11:30:55AM -0700, Harry Cutts wrote:
> This event code represents scroll reports from high-resolution wheels,
> and will be used by future patches in this series. See the linux-input
> "Reporting high-resolution scroll events" thread [0] for more details.
> 
> [0]: https://www.spinics.net/lists/linux-input/msg57380.html
> 
> Signed-off-by: Harry Cutts <hcutts@chromium.org>

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

Jiri, Benjamin, feel free to merge through HID tree if the rest is
acceptable.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Add the `REL_WHEEL_HI_RES` event code

[Matthew Wilcox]

On Thu, Aug 23, 2018 at 11:30:55AM -0700, Harry Cutts wrote:
> +  - 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 256ths of a millimeter.

Is it too late to change this to an actual unit like micrometres?

Re: [PATCH 1/3] Add the `REL_WHEEL_HI_RES` event code

[Dmitry Torokhov]

On Thu, Aug 23, 2018 at 11:57:22AM -0700, Matthew Wilcox wrote:
> On Thu, Aug 23, 2018 at 11:30:55AM -0700, Harry Cutts wrote:
> > +  - 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 256ths of a millimeter.
> 
> Is it too late to change this to an actual unit like micrometres?

No love for imperial roots? Sure, micrometers would work too I think.

-- 
Dmitry

Re: [PATCH 1/3] Add the `REL_WHEEL_HI_RES` event code

[Jiri Kosina]

On Thu, 23 Aug 2018, Dmitry Torokhov wrote:

> > > +  - 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 256ths of a millimeter.
> > 
> > Is it too late to change this to an actual unit like micrometres?
> 
> No love for imperial roots? Sure, micrometers would work too I think.

I don't really care strongly :) Is either of you going to update the 
patch? Otherwise I'll just apply as-is.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/3] Add the `REL_WHEEL_HI_RES` event code

[Harry Cutts]

Benjamin says he prefers micrometres, and I think I do to now that I
think about it (sticking with SI seems like a good idea), so I'll
update it in a V2.

Harry Cutts
Chrome OS Touch/Input team
Harry Cutts
Chrome OS Touch/Input team


On Tue, 28 Aug 2018 at 02:02, Jiri Kosina <jikos@kernel.org> wrote:
>
> On Thu, 23 Aug 2018, Dmitry Torokhov wrote:
>
> > > > +  - 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 256ths of a millimeter.
> > >
> > > Is it too late to change this to an actual unit like micrometres?
> >
> > No love for imperial roots? Sure, micrometers would work too I think.
>
> I don't really care strongly :) Is either of you going to update the
> patch? Otherwise I'll just apply as-is.
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

[PATCH 2/3] Create a utility class for counting scroll events

[Harry Cutts]

To avoid code duplication, this class counts high-resolution scroll
movements and emits the legacy low-resolution events when appropriate.
Drivers should create one instance for each scroll wheel that they
need to handle.

Signed-off-by: Harry Cutts <hcutts@chromium.org>
---

 drivers/hid/hid-input.c | 44 +++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h     | 28 ++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 4e94ea3e280a..4ee23b297472 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1826,3 +1826,47 @@ 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
+ * 256ths of a millimeter 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 often rest 7/8ths of a notch from the previous notch
+	 * after slow movement, so we want the threshold for low-res events in
+	 * between 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->mm256_per_hi_res_unit);
+
+	counter->remainder += hi_res_value;
+	if (abs(counter->remainder) >= threshold) {
+		/* Add (or subtract) 1 because we want to trigger when 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 834e6461a690..b7bc0b2faf4f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1138,6 +1138,34 @@ 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.
+ * @mm256_per_hi_res_unit: the amount moved by the user's finger for each
+ *                         high-resolution unit reported by the mouse, in 256ths
+ *                         of a millimetre.
+ * @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 mm256_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.18.0.1017.ga543ac7ca45-goog

[PATCH 3/3] Enable high-resolution scrolling on Logitech mice

[Harry Cutts]

There are three features used by various Logitech mice for
high-resolution scrolling: the fast scrolling bit in HID++ 1.0, and the
x2120 and x2121 features in HID++ 2.0 and above. This patch supports
all three, and uses the multiplier reported by the mouse for the HID++
2.0+ features.

The full list of product IDs of mice which support high-resolution
scrolling was provided by Logitech, but the patch was tested using the
following mice (over both Bluetooth and Unifying where applicable):

* HID++ 1.0: Anywhere MX, Performance MX
* x2120: M560
* x2121: MX Anywhere 2, MX Master 2S

Signed-off-by: Harry Cutts <hcutts@chromium.org>
---

 drivers/hid/hid-ids.h            |  15 ++
 drivers/hid/hid-logitech-hidpp.c | 341 ++++++++++++++++++++++++++++---
 drivers/hid/hid-quirks.c         |  11 +
 3 files changed, 340 insertions(+), 27 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 79bdf0c7e351..64fbe6174189 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -717,6 +717,21 @@
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C01A	0xc01a
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C05A	0xc05a
 #define USB_DEVICE_ID_LOGITECH_MOUSE_C06A	0xc06a
+/*
+ * The following mice have different IDs over Bluetooth than Logitech Unifying
+ * protocol, hence the _BT suffix.
+ */
+#define USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT1	0xb014
+#define USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT2	0xb016
+#define USB_DEVICE_ID_LOGITECH_MOUSE_M720_BT		0xb015
+#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT1	0xb013
+#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT2	0xb018
+#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT3	0xb01f
+#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2S_BT	0xb01a
+#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT1	0xb012
+#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT2	0xb017
+#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT3	0xb01e
+#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT	0xb019
 #define USB_DEVICE_ID_LOGITECH_RUMBLEPAD_CORD	0xc20a
 #define USB_DEVICE_ID_LOGITECH_RUMBLEPAD	0xc211
 #define USB_DEVICE_ID_LOGITECH_EXTREME_3D	0xc215
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 19cc980eebce..17598b87f1b7 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -64,6 +64,14 @@ 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
 
@@ -149,6 +157,7 @@ struct hidpp_device {
 	unsigned long capabilities;
 
 	struct hidpp_battery battery;
+	struct hid_scroll_counter vertical_wheel_counter;
 };
 
 /* HID++ 1.0 error codes */
@@ -400,32 +409,53 @@ static void hidpp_prefix_name(char **name, int name_length)
 #define HIDPP_SET_LONG_REGISTER				0x82
 #define HIDPP_GET_LONG_REGISTER				0x83
 
-#define HIDPP_REG_GENERAL				0x00
-
-static int hidpp10_enable_battery_reporting(struct hidpp_device *hidpp_dev)
+/**
+ * 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)
 {
 	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);
+					  REPORT_ID_HIDPP_SHORT,
+					  HIDPP_GET_REGISTER,
+					  register_address,
+					  NULL, 0, &response);
 	if (ret)
 		return ret;
 
 	memcpy(params, response.rap.params, 3);
 
-	/* Set the battery bit */
-	params[0] |= BIT(4);
+	params[byte] |= BIT(bit);
 
 	return hidpp_send_rap_command_sync(hidpp_dev,
-					REPORT_ID_HIDPP_SHORT,
-					HIDPP_SET_REGISTER,
-					HIDPP_REG_GENERAL,
-					params, 3, &response);
+					   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 scrolling was called "fast scrolling". */
+static int hidpp10_enable_fast_scrolling(struct hidpp_device *hidpp_dev)
+{
+	return hidpp10_set_register_bit(hidpp_dev, HIDPP_REG_FEATURES, 0, 6);
 }
 
 #define HIDPP_REG_BATTERY_STATUS			0x07
@@ -1136,6 +1166,101 @@ 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                                                     */
 /* -------------------------------------------------------------------------- */
@@ -2399,7 +2524,8 @@ 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);
-		input_report_rel(mydata->input, REL_WHEEL, v);
+		hid_scroll_counter_handle_scroll(
+				&hidpp->vertical_wheel_counter, v);
 
 		input_sync(mydata->input);
 	}
@@ -2527,6 +2653,71 @@ 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.
+ * @mm256_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 mm256_per_hi_res_unit;
+};
+
+static struct hi_res_scroll_info hi_res_scroll_devices[] = {
+	{ /* Anywhere MX */
+	  .product_id = 0x1017, .mm256_per_hi_res_unit = 114 },
+	{ /* Performance MX */
+	  .product_id = 0x101a, .mm256_per_hi_res_unit = 104 },
+	{ /* M560 */
+	  .product_id = 0x402d, .mm256_per_hi_res_unit = 111 },
+	{ /* MX Master 2S */
+	  .product_id = 0x4069, .mm256_per_hi_res_unit = 104 },
+	{ .product_id = USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT,
+	  .mm256_per_hi_res_unit = 104 },
+};
+
+static int hi_res_scroll_look_up_mm256(__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].mm256_per_hi_res_unit;
+	}
+	return 104;
+}
+
+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_fast_scrolling(hidpp);
+		multiplier = 8;
+	}
+	if (ret)
+		return ret;
+
+	hidpp->vertical_wheel_counter.resolution_multiplier = multiplier;
+	hidpp->vertical_wheel_counter.mm256_per_hi_res_unit =
+		hi_res_scroll_look_up_mm256(hidpp->hid_dev->product);
+	return 0;
+}
+
 /* -------------------------------------------------------------------------- */
 /* Generic HID++ devices                                                      */
 /* -------------------------------------------------------------------------- */
@@ -2572,6 +2763,11 @@ 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,
@@ -2690,6 +2886,25 @@ 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)
+{
+	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);
@@ -2901,6 +3116,9 @@ 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;
@@ -3086,35 +3304,97 @@ 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,
-		USB_VENDOR_ID_LOGITECH, 0x4011),
+	  LDJ_DEVICE(0x4011),
 	  .driver_data = HIDPP_QUIRK_CLASS_WTP | HIDPP_QUIRK_DELAYED_INIT |
 			 HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS },
 	{ /* wireless touchpad T650 */
-	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
-		USB_VENDOR_ID_LOGITECH, 0x4101),
+	  LDJ_DEVICE(0x4101),
 	  .driver_data = HIDPP_QUIRK_CLASS_WTP | HIDPP_QUIRK_DELAYED_INIT },
 	{ /* wireless touchpad T651 */
 	  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 M336, M337, and M535 */
+	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT1),
+	  .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT2),
+	  .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 */
-	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
-		USB_VENDOR_ID_LOGITECH, 0x402d),
-	  .driver_data = HIDPP_QUIRK_DELAYED_INIT | HIDPP_QUIRK_CLASS_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 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_M720_BT),
+	  .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 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT1),
+	  .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT2),
+	  .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT3),
+	  .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ /* Mouse Logitech MX Anywhere 2S */
+	  LDJ_DEVICE(0x406a), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2S_BT),
+	  .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 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT1),
+	  .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT2),
+	  .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT3),
+	  .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ /* Mouse Logitech MX Master 2S */
+	  LDJ_DEVICE(0x4069), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
+		USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT),
+	  .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+	{ /* Mouse Logitech Performance MX */
+	  LDJ_DEVICE(0x101a), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_1P0 },
 	{ /* Keyboard logitech K400 */
-	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
-		USB_VENDOR_ID_LOGITECH, 0x4024),
+	  LDJ_DEVICE(0x4024),
 	  .driver_data = HIDPP_QUIRK_CLASS_K400 },
 	{ /* Solar Keyboard Logitech K750 */
-	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
-		USB_VENDOR_ID_LOGITECH, 0x4002),
+	  LDJ_DEVICE(0x4002),
 	  .driver_data = HIDPP_QUIRK_CLASS_K750 },
 
-	{ HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
-		USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},
+	{ LDJ_DEVICE(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},
@@ -3123,12 +3403,19 @@ 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,
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 249d49b6b16c..7926c275f258 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -463,6 +463,17 @@ static const struct hid_device_id hid_have_special_driver[] = {
 #endif
 #if IS_ENABLED(CONFIG_HID_LOGITECH_HIDPP)
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_T651) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT1) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT2) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_M720_BT) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT1) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT2) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT3) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2S_BT) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT1) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT2) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT3) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL) },
 #endif
 #if IS_ENABLED(CONFIG_HID_LOGITECH_DJ)
-- 
2.18.0.1017.ga543ac7ca45-goog

Re: [PATCH 3/3] Enable high-resolution scrolling on Logitech mice

[Benjamin Tissoires]

Hi Harry,

On Thu, Aug 23, 2018 at 8:31 PM Harry Cutts <hcutts@chromium.org> wrote:
>
> There are three features used by various Logitech mice for
> high-resolution scrolling: the fast scrolling bit in HID++ 1.0, and the
> x2120 and x2121 features in HID++ 2.0 and above. This patch supports
> all three, and uses the multiplier reported by the mouse for the HID++
> 2.0+ features.
>
> The full list of product IDs of mice which support high-resolution
> scrolling was provided by Logitech, but the patch was tested using the
> following mice (over both Bluetooth and Unifying where applicable):
>
> * HID++ 1.0: Anywhere MX, Performance MX
> * x2120: M560
> * x2121: MX Anywhere 2, MX Master 2S
>
> Signed-off-by: Harry Cutts <hcutts@chromium.org>

Patches 1 and 2 look fine (I'd rather have the micrometers too).
I have more concerns about this one.
My main issue is that this patch both reshuffle existing parts and add
new features, which makes it hard to review.

> ---
>
>  drivers/hid/hid-ids.h            |  15 ++
>  drivers/hid/hid-logitech-hidpp.c | 341 ++++++++++++++++++++++++++++---
>  drivers/hid/hid-quirks.c         |  11 +
>  3 files changed, 340 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 79bdf0c7e351..64fbe6174189 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -717,6 +717,21 @@
>  #define USB_DEVICE_ID_LOGITECH_MOUSE_C01A      0xc01a
>  #define USB_DEVICE_ID_LOGITECH_MOUSE_C05A      0xc05a
>  #define USB_DEVICE_ID_LOGITECH_MOUSE_C06A      0xc06a
> +/*
> + * The following mice have different IDs over Bluetooth than Logitech Unifying
> + * protocol, hence the _BT suffix.
> + */
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT1  0xb014
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT2  0xb016
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_M720_BT           0xb015
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT1 0xb013
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT2 0xb018
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT3 0xb01f
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2S_BT 0xb01a
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT1     0xb012
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT2     0xb017
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT3     0xb01e
> +#define USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT   0xb019
>  #define USB_DEVICE_ID_LOGITECH_RUMBLEPAD_CORD  0xc20a
>  #define USB_DEVICE_ID_LOGITECH_RUMBLEPAD       0xc211
>  #define USB_DEVICE_ID_LOGITECH_EXTREME_3D      0xc215
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 19cc980eebce..17598b87f1b7 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -64,6 +64,14 @@ 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
>
> @@ -149,6 +157,7 @@ struct hidpp_device {
>         unsigned long capabilities;
>
>         struct hidpp_battery battery;
> +       struct hid_scroll_counter vertical_wheel_counter;
>  };
>
>  /* HID++ 1.0 error codes */
> @@ -400,32 +409,53 @@ static void hidpp_prefix_name(char **name, int name_length)
>  #define HIDPP_SET_LONG_REGISTER                                0x82
>  #define HIDPP_GET_LONG_REGISTER                                0x83
>
> -#define HIDPP_REG_GENERAL                              0x00
> -
> -static int hidpp10_enable_battery_reporting(struct hidpp_device *hidpp_dev)
> +/**
> + * 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)
>  {
>         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);
> +                                         REPORT_ID_HIDPP_SHORT,
> +                                         HIDPP_GET_REGISTER,
> +                                         register_address,
> +                                         NULL, 0, &response);
>         if (ret)
>                 return ret;
>
>         memcpy(params, response.rap.params, 3);
>
> -       /* Set the battery bit */
> -       params[0] |= BIT(4);
> +       params[byte] |= BIT(bit);
>
>         return hidpp_send_rap_command_sync(hidpp_dev,
> -                                       REPORT_ID_HIDPP_SHORT,
> -                                       HIDPP_SET_REGISTER,
> -                                       HIDPP_REG_GENERAL,
> -                                       params, 3, &response);
> +                                          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);
> +}

This hunk should be dealt in a separate patch (including the one function below)

> +
> +#define HIDPP_REG_FEATURES                             0x01
> +
> +/* On HID++ 1.0 devices, high-res scrolling was called "fast scrolling". */
> +static int hidpp10_enable_fast_scrolling(struct hidpp_device *hidpp_dev)
> +{
> +       return hidpp10_set_register_bit(hidpp_dev, HIDPP_REG_FEATURES, 0, 6);
>  }
>
>  #define HIDPP_REG_BATTERY_STATUS                       0x07
> @@ -1136,6 +1166,101 @@ 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                                                     */
>  /* -------------------------------------------------------------------------- */
> @@ -2399,7 +2524,8 @@ 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);
> -               input_report_rel(mydata->input, REL_WHEEL, v);
> +               hid_scroll_counter_handle_scroll(
> +                               &hidpp->vertical_wheel_counter, v);

The conversion input_report_rel(... REL_WHEEL,...) to
hid_scroll_counter_handle_scroll() should be dealt in a separate
patch.

>
>                 input_sync(mydata->input);
>         }
> @@ -2527,6 +2653,71 @@ 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.
> + * @mm256_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 mm256_per_hi_res_unit;
> +};
> +
> +static struct hi_res_scroll_info hi_res_scroll_devices[] = {
> +       { /* Anywhere MX */
> +         .product_id = 0x1017, .mm256_per_hi_res_unit = 114 },
> +       { /* Performance MX */
> +         .product_id = 0x101a, .mm256_per_hi_res_unit = 104 },
> +       { /* M560 */
> +         .product_id = 0x402d, .mm256_per_hi_res_unit = 111 },
> +       { /* MX Master 2S */
> +         .product_id = 0x4069, .mm256_per_hi_res_unit = 104 },
> +       { .product_id = USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT,
> +         .mm256_per_hi_res_unit = 104 },
> +};
> +
> +static int hi_res_scroll_look_up_mm256(__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].mm256_per_hi_res_unit;
> +       }
> +       return 104;

104?

> +}
> +
> +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_fast_scrolling(hidpp);
> +               multiplier = 8;
> +       }
> +       if (ret)
> +               return ret;
> +
> +       hidpp->vertical_wheel_counter.resolution_multiplier = multiplier;
> +       hidpp->vertical_wheel_counter.mm256_per_hi_res_unit =
> +               hi_res_scroll_look_up_mm256(hidpp->hid_dev->product);
> +       return 0;
> +}
> +
>  /* -------------------------------------------------------------------------- */
>  /* Generic HID++ devices                                                      */
>  /* -------------------------------------------------------------------------- */
> @@ -2572,6 +2763,11 @@ 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,
> @@ -2690,6 +2886,25 @@ 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)
> +{
> +       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;

You are using usage_table to force the .event function to be called
only on the WHEEL events. This is correct, but I have a feeling this
will be harder to understand when we are going to extend the .event()
function for other events.
If you rather keep the cde that way, please add a comment at the
beginning of the function stating that we are only called against
WHEEL events because of usage_table.

> +
> +       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);
> @@ -2901,6 +3116,9 @@ 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;
> @@ -3086,35 +3304,97 @@ 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,
> -               USB_VENDOR_ID_LOGITECH, 0x4011),
> +         LDJ_DEVICE(0x4011),
>           .driver_data = HIDPP_QUIRK_CLASS_WTP | HIDPP_QUIRK_DELAYED_INIT |
>                          HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS },
>         { /* wireless touchpad T650 */
> -         HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
> -               USB_VENDOR_ID_LOGITECH, 0x4101),
> +         LDJ_DEVICE(0x4101),

The rewrite of the existing supported devices should be in a separate patch too.

>           .driver_data = HIDPP_QUIRK_CLASS_WTP | HIDPP_QUIRK_DELAYED_INIT },
>         { /* wireless touchpad T651 */
>           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 M336, M337, and M535 */
> +         HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT1),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT2),
> +         .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 */
> -         HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
> -               USB_VENDOR_ID_LOGITECH, 0x402d),
> -         .driver_data = HIDPP_QUIRK_DELAYED_INIT | HIDPP_QUIRK_CLASS_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 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_M720_BT),
> +         .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 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT1),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT2),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT3),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { /* Mouse Logitech MX Anywhere 2S */
> +         LDJ_DEVICE(0x406a), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2S_BT),
> +         .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 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT1),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT2),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT3),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { /* Mouse Logitech MX Master 2S */
> +         LDJ_DEVICE(0x4069), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> +               USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> +       { /* Mouse Logitech Performance MX */
> +         LDJ_DEVICE(0x101a), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_1P0 },
>         { /* Keyboard logitech K400 */
> -         HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
> -               USB_VENDOR_ID_LOGITECH, 0x4024),
> +         LDJ_DEVICE(0x4024),
>           .driver_data = HIDPP_QUIRK_CLASS_K400 },
>         { /* Solar Keyboard Logitech K750 */
> -         HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
> -               USB_VENDOR_ID_LOGITECH, 0x4002),
> +         LDJ_DEVICE(0x4002),
>           .driver_data = HIDPP_QUIRK_CLASS_K750 },
>
> -       { HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
> -               USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},
> +       { LDJ_DEVICE(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},
> @@ -3123,12 +3403,19 @@ 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,
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index 249d49b6b16c..7926c275f258 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -463,6 +463,17 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  #endif
>  #if IS_ENABLED(CONFIG_HID_LOGITECH_HIDPP)
>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_T651) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT1) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT2) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_M720_BT) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT1) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT2) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT3) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2S_BT) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT1) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT2) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT3) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT) },

Since v4.16, this should not be required anymore. Please drop the hunk
if I am correct.

Cheers,
Benjamin

>         { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL) },
>  #endif
>  #if IS_ENABLED(CONFIG_HID_LOGITECH_DJ)
> --
> 2.18.0.1017.ga543ac7ca45-goog
>

Re: [PATCH 3/3] Enable high-resolution scrolling on Logitech mice

[Benjamin Tissoires]

Hi Harry,

On Thu, Aug 30, 2018 at 1:06 AM Harry Cutts <hcutts@chromium.org> wrote:
>
> Hi Benjamin,
>
> On Tue, 28 Aug 2018 at 01:47, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > On Thu, Aug 23, 2018 at 8:31 PM Harry Cutts <hcutts@chromium.org> wrote:
> > > [snip]
> > > @@ -400,32 +409,53 @@ static void hidpp_prefix_name(char **name, int name_length)
> > >  #define HIDPP_SET_LONG_REGISTER                                0x82
> > >  #define HIDPP_GET_LONG_REGISTER                                0x83
> > >
> > > -#define HIDPP_REG_GENERAL                              0x00
> > > -
> > > -static int hidpp10_enable_battery_reporting(struct hidpp_device *hidpp_dev)
> > > +/**
> > > + * 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)
> > >  {
> > >         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);
> > > +                                         REPORT_ID_HIDPP_SHORT,
> > > +                                         HIDPP_GET_REGISTER,
> > > +                                         register_address,
> > > +                                         NULL, 0, &response);
> > >         if (ret)
> > >                 return ret;
> > >
> > >         memcpy(params, response.rap.params, 3);
> > >
> > > -       /* Set the battery bit */
> > > -       params[0] |= BIT(4);
> > > +       params[byte] |= BIT(bit);
> > >
> > >         return hidpp_send_rap_command_sync(hidpp_dev,
> > > -                                       REPORT_ID_HIDPP_SHORT,
> > > -                                       HIDPP_SET_REGISTER,
> > > -                                       HIDPP_REG_GENERAL,
> > > -                                       params, 3, &response);
> > > +                                          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);
> > > +}
> >
> > This hunk should be dealt in a separate patch (including the one function below)
>
> OK, will do in v2.
>
> > > [snip]
> > > @@ -2399,7 +2524,8 @@ 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);
> > > -               input_report_rel(mydata->input, REL_WHEEL, v);
> > > +               hid_scroll_counter_handle_scroll(
> > > +                               &hidpp->vertical_wheel_counter, v);
> >
> > The conversion input_report_rel(... REL_WHEEL,...) to
> > hid_scroll_counter_handle_scroll() should be dealt in a separate
> > patch.
>
> OK, I'll do that in v2, but might I ask why? I don't see how this
> particular hunk is special.

I tend to consider that existing code rewrite need to be in their
special commit, especially if the change isn't supposed to change the
behaviour. This is my personal taste in case something goes wrong and
(in this case) a m560 suddenly starts complaining about an issue with
this mouse.
I wouldn't mind too much if you rather keep the
hid_scroll_counter_handle_scroll() introduction to this commit though.

>
> >
> > >
> > >                 input_sync(mydata->input);
> > >         }
> > > @@ -2527,6 +2653,71 @@ 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.
> > > + * @mm256_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 mm256_per_hi_res_unit;
> > > +};
> > > +
> > > +static struct hi_res_scroll_info hi_res_scroll_devices[] = {
> > > +       { /* Anywhere MX */
> > > +         .product_id = 0x1017, .mm256_per_hi_res_unit = 114 },
> > > +       { /* Performance MX */
> > > +         .product_id = 0x101a, .mm256_per_hi_res_unit = 104 },
> > > +       { /* M560 */
> > > +         .product_id = 0x402d, .mm256_per_hi_res_unit = 111 },
> > > +       { /* MX Master 2S */
> > > +         .product_id = 0x4069, .mm256_per_hi_res_unit = 104 },
> > > +       { .product_id = USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT,
> > > +         .mm256_per_hi_res_unit = 104 },
> > > +};
> > > +
> > > +static int hi_res_scroll_look_up_mm256(__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].mm256_per_hi_res_unit;
> > > +       }
> > > +       return 104;
> >
> > 104?
>
> This seems like a sensible default value in case we don't have a value
> for this mouse in hi_res_scroll_devices. I'll add a comment explaining
> this in v2.
>
> >
> > > [snip]
> > > +static int hidpp_event(struct hid_device *hdev, struct hid_field *field,
> > > +       struct hid_usage *usage, __s32 value)
> > > +{
> > > +       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;
> >
> > You are using usage_table to force the .event function to be called
> > only on the WHEEL events. This is correct, but I have a feeling this
> > will be harder to understand when we are going to extend the .event()
> > function for other events.
> > If you rather keep the cde that way, please add a comment at the
> > beginning of the function stating that we are only called against
> > WHEEL events because of usage_table.
>
> OK, I'll add the comment in v2.
>
> > > [snip]
> > >
> > > +#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,
> > > -               USB_VENDOR_ID_LOGITECH, 0x4011),
> > > +         LDJ_DEVICE(0x4011),
> > >           .driver_data = HIDPP_QUIRK_CLASS_WTP | HIDPP_QUIRK_DELAYED_INIT |
> > >                          HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS },
> > >         { /* wireless touchpad T650 */
> > > -         HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
> > > -               USB_VENDOR_ID_LOGITECH, 0x4101),
> > > +         LDJ_DEVICE(0x4101),
> >
> > The rewrite of the existing supported devices should be in a separate patch too.
>
> OK, will do.
>
> > > [snip]
> > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> > > index 249d49b6b16c..7926c275f258 100644
> > > --- a/drivers/hid/hid-quirks.c
> > > +++ b/drivers/hid/hid-quirks.c
> > > @@ -463,6 +463,17 @@ static const struct hid_device_id hid_have_special_driver[] = {
> > >  #endif
> > >  #if IS_ENABLED(CONFIG_HID_LOGITECH_HIDPP)
> > >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_T651) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT1) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_M336_337_535_BT2) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_M720_BT) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT1) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT2) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2_BT3) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_ANYWHERE_2S_BT) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT1) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT2) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_BT3) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_MX_MASTER_2S_BT) },
> >
> > Since v4.16, this should not be required anymore. Please drop the hunk
> > if I am correct.
>
> Yes, it seems to work fine without it (at least for the MX Master 2S).
> Unfortunately, while testing this I encountered a bug with high-res
> scrolling over Bluetooth. (It seems that if you turn off the MX Master
> 2S while it's connected over Bluetooth, we don't detect that and
> remove the input device, meaning that when it reconnects the driver
> thinks it's in high-res mode but the mouse is in low-res.) I'm
> investigating, but in the meantime I'll remove the Bluetooth support
> from v2 and add it back in later.

As far as I can see, the MX Master 2S is connected over BLE. Bluez
keeps the uhid node opened (and thus the existing bluetooth HID
device) to be able to reconnect faster.
I would suppose you should get notified in the connect event of a
reconnection, but it doesn't seem to be the case.

Nestor, is there any event emitted by the mouse when it gets
reconnected over BLE or is that a bluez issue?

Cheers,
Benjamin

>
> >
> > Cheers,
> > Benjamin
> >
> > >         { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL) },
> > >  #endif
> > >  #if IS_ENABLED(CONFIG_HID_LOGITECH_DJ)
> > > --
> > > 2.18.0.1017.ga543ac7ca45-goog
> > >
>
> Thanks,
>
> Harry Cutts
> Chrome OS Touch/Input team

Re: [PATCH 3/3] Enable high-resolution scrolling on Logitech mice

[Harry Cutts]

Hi Benjamin,

On Thu, 30 Aug 2018 at 00:18, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>> On Thu, Aug 30, 2018 at 1:06 AM Harry Cutts <hcutts@chromium.org> wrote:
> > > The conversion input_report_rel(... REL_WHEEL,...) to
> > > hid_scroll_counter_handle_scroll() should be dealt in a separate
> > > patch.
> >
> > OK, I'll do that in v2, but might I ask why? I don't see how this
> > particular hunk is special.
>
> I tend to consider that existing code rewrite need to be in their
> special commit, especially if the change isn't supposed to change the
> behaviour. This is my personal taste in case something goes wrong and
> (in this case) a m560 suddenly starts complaining about an issue with
> this mouse.
> I wouldn't mind too much if you rather keep the
> hid_scroll_counter_handle_scroll() introduction to this commit though.

Yes, I see the reasoning for that, but this hunk is pretty tied to the
main change in that scrolling on the M560 would be 8x too fast without
it. I'll keep it in the same one, if you don't mind.

> > [snip]
> > Yes, it seems to work fine without it (at least for the MX Master 2S).
> > Unfortunately, while testing this I encountered a bug with high-res
> > scrolling over Bluetooth. (It seems that if you turn off the MX Master
> > 2S while it's connected over Bluetooth, we don't detect that and
> > remove the input device, meaning that when it reconnects the driver
> > thinks it's in high-res mode but the mouse is in low-res.) I'm
> > investigating, but in the meantime I'll remove the Bluetooth support
> > from v2 and add it back in later.
>
> As far as I can see, the MX Master 2S is connected over BLE. Bluez
> keeps the uhid node opened (and thus the existing bluetooth HID
> device) to be able to reconnect faster.
> I would suppose you should get notified in the connect event of a
> reconnection, but it doesn't seem to be the case.
>
> Nestor, is there any event emitted by the mouse when it gets
> reconnected over BLE or is that a bluez issue?

Ah, interesting. The MX Master 2S is indeed using BLE, and testing
with another Logitech BLE mouse (the M585) also leaves the input
device around. I think this is something Logitech-specific, though, as
the Microsoft Surface Precision mouse (also BLE) does have its input
device removed when it turns off. I notice that btmon does show
"device disconnected" and "device connected" events when I turn the
M585 on and off; maybe we need to plumb those through to the driver.
We've decided to delay Bluetooth support for high-res scrolling until
the Chrome OS Bluetooth stack is more stable.

Harry Cutts
Chrome OS Touch/Input team

Re: [PATCH 3/3] Enable high-resolution scrolling on Logitech mice

[Nestor Lopez Casado]

On Fri, Aug 31, 2018 at 11:11 AM Nestor Lopez Casado
<nlopezcasad@logitech.com> wrote:
>
>
>
> On Thu, Aug 30, 2018 at 10:38 PM Harry Cutts <hcutts@chromium.org> wrote:
>>
>> Hi Benjamin,
>>
>> On Thu, 30 Aug 2018 at 00:18, Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:
>> >> On Thu, Aug 30, 2018 at 1:06 AM Harry Cutts <hcutts@chromium.org> wrote:
>> > > > The conversion input_report_rel(... REL_WHEEL,...) to
>> > > > hid_scroll_counter_handle_scroll() should be dealt in a separate
>> > > > patch.
>> > >
>> > > OK, I'll do that in v2, but might I ask why? I don't see how this
>> > > particular hunk is special.
>> >
>> > I tend to consider that existing code rewrite need to be in their
>> > special commit, especially if the change isn't supposed to change the
>> > behaviour. This is my personal taste in case something goes wrong and
>> > (in this case) a m560 suddenly starts complaining about an issue with
>> > this mouse.
>> > I wouldn't mind too much if you rather keep the
>> > hid_scroll_counter_handle_scroll() introduction to this commit though.
>>
>> Yes, I see the reasoning for that, but this hunk is pretty tied to the
>> main change in that scrolling on the M560 would be 8x too fast without
>> it. I'll keep it in the same one, if you don't mind.
>>
>> > > [snip]
>> > > Yes, it seems to work fine without it (at least for the MX Master 2S).
>> > > Unfortunately, while testing this I encountered a bug with high-res
>> > > scrolling over Bluetooth. (It seems that if you turn off the MX Master
>> > > 2S while it's connected over Bluetooth, we don't detect that and
>> > > remove the input device, meaning that when it reconnects the driver
>> > > thinks it's in high-res mode but the mouse is in low-res.) I'm
>> > > investigating, but in the meantime I'll remove the Bluetooth support
>> > > from v2 and add it back in later.
>> >
>> > As far as I can see, the MX Master 2S is connected over BLE. Bluez
>> > keeps the uhid node opened (and thus the existing bluetooth HID
>> > device) to be able to reconnect faster.
>> > I would suppose you should get notified in the connect event of a
>> > reconnection, but it doesn't seem to be the case.
>> >
>> > Nestor, is there any event emitted by the mouse when it gets
>> > reconnected over BLE or is that a bluez issue?
>>
>> Ah, interesting. The MX Master 2S is indeed using BLE, and testing
>> with another Logitech BLE mouse (the M585) also leaves the input
>> device around. I think this is something Logitech-specific, though, as
>> the Microsoft Surface Precision mouse (also BLE) does have its input
>> device removed when it turns off. I notice that btmon does show
>> "device disconnected" and "device connected" events when I turn the
>> M585 on and off; maybe we need to plumb those through to the driver.
>> We've decided to delay Bluetooth support for high-res scrolling until
>> the Chrome OS Bluetooth stack is more stable.

-----Ooops, no html now...
This might be related to how the device disconnects from the host.
Sometimes a disconnection comes from the device not responding anymore
and it is the BT supervision timeout that kicks in (host side). In
some cases (hw support required on the device side) a device will send
a disconnect request when switched off. Maybe these different
disconnect flavors are the root cause behind the input device being
removed or not.
--nestor
>>
>> Harry Cutts
>> Chrome OS Touch/Input team