LinuxLists.cc - [PATCH v3 0/8] HID: MS and Logitech high-resolution scroll wheel support

[permalink] [raw]

Subject: [PATCH v3 3/8] HID: core: process the Resolution Multiplier

The Resolution Multiplier is a feature report that modifies the value of
Usages within the same Logical Collection. If the multiplier is set to
anything but 1, the hardware reports (value * multiplier) for the same amount
of physical movement, i.e. the value we receive in the kernel is
pre-multiplied.

The hardware may either send a single (value * multiplier), or by sending
multiplier as many reports with the same value, or a combination of these two
options. For example, when the Microsoft Sculpt Ergonomic mouse Resolution
Multiplier is set to 12, the Wheel sends out 12 for every detent but AC Pan
sends out a value of 3 at 4 times the frequency.

The effective multiplier is based on the physical min/max of the multiplier
field, a logical min/max of [0,1] with a physical min/max of [1,8] means the
multiplier is either 1 or 8.

The Resolution Multiplier was introduced for high-resolution scrolling in
Windows Vista and is commonly used on Microsoft mice.

The recommendation for the Resolution Multiplier is to default to 1 for
backwards compatibility. This patch adds an arbitrary upper limit at 255. The
only known use case for the Resolution Multiplier is for scroll wheels where the
multiplier has to be a fraction of 120 to work with Windows.

Signed-off-by: Peter Hutterer <[email protected]>
---
Changes since v1, v2:
- expanded the commit message with more detail

drivers/hid/hid-core.c | 170 +++++++++++++++++++++++++++++++++++++++++
include/linux/hid.h | 5 ++
2 files changed, 175 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 43d488a45120..f41d5fe51abe 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -294,6 +294,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
field->usage[i].collection_index =
parser->local.collection_index[j];
field->usage[i].usage_index = i;
+ field->usage[i].resolution_multiplier = 1;
}

field->maxusage = usages;
@@ -947,6 +948,167 @@ struct hid_report *hid_validate_values(struct hid_device *hid,
}
EXPORT_SYMBOL_GPL(hid_validate_values);

+static int hid_calculate_multiplier(struct hid_device *hid,
+ struct hid_field *multiplier)
+{
+ int m;
+ __s32 v = *multiplier->value;
+ __s32 lmin = multiplier->logical_minimum;
+ __s32 lmax = multiplier->logical_maximum;
+ __s32 pmin = multiplier->physical_minimum;
+ __s32 pmax = multiplier->physical_maximum;
+
+ /*
+ * "Because OS implementations will generally divide the control's
+ * reported count by the Effective Resolution Multiplier, designers
+ * should take care not to establish a potential Effective
+ * Resolution Multiplier of zero."
+ * HID Usage Table, v1.12, Section 4.3.1, p31
+ */
+ if (lmax - lmin == 0)
+ return 1;
+ /*
+ * Handling the unit exponent is left as an exercise to whoever
+ * finds a device where that exponent is not 0.
+ */
+ m = ((v - lmin)/(lmax - lmin) * (pmax - pmin) + pmin);
+ if (unlikely(multiplier->unit_exponent != 0)) {
+ hid_warn(hid,
+ "unsupported Resolution Multiplier unit exponent %d\n",
+ multiplier->unit_exponent);
+ }
+
+ /* There are no devices with an effective multiplier > 255 */
+ if (unlikely(m == 0 || m > 255 || m < -255)) {
+ hid_warn(hid, "unsupported Resolution Multiplier %d\n", m);
+ m = 1;
+ }
+
+ return m;
+}
+
+static void hid_apply_multiplier_to_field(struct hid_device *hid,
+ struct hid_field *field,
+ struct hid_collection *multiplier_collection,
+ int effective_multiplier)
+{
+ struct hid_collection *collection;
+ struct hid_usage *usage;
+ int i;
+
+ /*
+ * If multiplier_collection is NULL, the multiplier applies
+ * to all fields in the report.
+ * Otherwise, it is the Logical Collection the multiplier applies to
+ * but our field may be in a subcollection of that collection.
+ */
+ for (i = 0; i < field->maxusage; i++) {
+ usage = &field->usage[i];
+
+ collection = &hid->collection[usage->collection_index];
+ while (collection && collection != multiplier_collection)
+ collection = collection->parent;
+
+ if (collection || multiplier_collection == NULL)
+ usage->resolution_multiplier = effective_multiplier;
+
+ }
+}
+
+static void hid_apply_multiplier(struct hid_device *hid,
+ struct hid_field *multiplier)
+{
+ struct hid_report_enum *rep_enum;
+ struct hid_report *rep;
+ struct hid_field *field;
+ struct hid_collection *multiplier_collection;
+ int effective_multiplier;
+ int i;
+
+ /*
+ * "The Resolution Multiplier control must be contained in the same
+ * Logical Collection as the control(s) to which it is to be applied.
+ * If no Resolution Multiplier is defined, then the Resolution
+ * Multiplier defaults to 1. If more than one control exists in a
+ * Logical Collection, the Resolution Multiplier is associated with
+ * all controls in the collection. If no Logical Collection is
+ * defined, the Resolution Multiplier is associated with all
+ * controls in the report."
+ * HID Usage Table, v1.12, Section 4.3.1, p30
+ *
+ * Thus, search from the current collection upwards until we find a
+ * logical collection. Then search all fields for that same parent
+ * collection. Those are the fields the multiplier applies to.
+ *
+ * If we have more than one multiplier, it will overwrite the
+ * applicable fields later.
+ */
+ multiplier_collection = &hid->collection[multiplier->usage->collection_index];
+ while (multiplier_collection &&
+ multiplier_collection->type != HID_COLLECTION_LOGICAL)
+ multiplier_collection = multiplier_collection->parent;
+
+ effective_multiplier = hid_calculate_multiplier(hid, multiplier);
+
+ rep_enum = &hid->report_enum[HID_INPUT_REPORT];
+ list_for_each_entry(rep, &rep_enum->report_list, list) {
+ for (i = 0; i < rep->maxfield; i++) {
+ field = rep->field[i];
+ hid_apply_multiplier_to_field(hid, field,
+ multiplier_collection,
+ effective_multiplier);
+ }
+ }
+}
+
+/*
+ * hid_setup_resolution_multiplier - set up all resolution multipliers
+ *
+ * @device: hid device
+ *
+ * Search for all Resolution Multiplier Feature Reports and apply their
+ * value to all matching Input items. This only updates the internal struct
+ * fields.
+ *
+ * The Resolution Multiplier is applied by the hardware. If the multiplier
+ * is anything other than 1, the hardware will send pre-multiplied events
+ * so that the same physical interaction generates an accumulated
+ * accumulated_value = value * * multiplier
+ * This may be achieved by sending
+ * - "value * multiplier" for each event, or
+ * - "value" but "multiplier" times as frequently, or
+ * - a combination of the above
+ * The only guarantee is that the same physical interaction always generates
+ * an accumulated 'value * multiplier'.
+ *
+ * This function must be called before any event processing and after
+ * any SetRequest to the Resolution Multiplier.
+ */
+void hid_setup_resolution_multiplier(struct hid_device *hid)
+{
+ struct hid_report_enum *rep_enum;
+ struct hid_report *rep;
+ struct hid_usage *usage;
+ int i, j;
+
+ rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
+ list_for_each_entry(rep, &rep_enum->report_list, list) {
+ for (i = 0; i < rep->maxfield; i++) {
+ /* Ignore if report count is out of bounds. */
+ if (rep->field[i]->report_count < 1)
+ continue;
+
+ for (j = 0; j < rep->field[i]->maxusage; j++) {
+ usage = &rep->field[i]->usage[j];
+ if (usage->hid == HID_GD_RESOLUTION_MULTIPLIER)
+ hid_apply_multiplier(hid,
+ rep->field[i]);
+ }
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(hid_setup_resolution_multiplier);
+
/**
* hid_open_report - open a driver-specific device report
*
@@ -1043,9 +1205,17 @@ int hid_open_report(struct hid_device *device)
hid_err(device, "unbalanced delimiter at end of report description\n");
goto err;
}
+
+ /*
+ * fetch initial values in case the device's
+ * default multiplier isn't the recommended 1
+ */
+ hid_setup_resolution_multiplier(device);
+
kfree(parser->collection_stack);
vfree(parser);
device->status |= HID_STAT_PARSED;
+
return 0;
}
}
diff --git a/include/linux/hid.h b/include/linux/hid.h
index fdfda898656c..fd8d860365a4 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -219,6 +219,7 @@ struct hid_item {
#define HID_GD_VBRZ 0x00010045
#define HID_GD_VNO 0x00010046
#define HID_GD_FEATURE 0x00010047
+#define HID_GD_RESOLUTION_MULTIPLIER 0x00010048
#define HID_GD_SYSTEM_CONTROL 0x00010080
#define HID_GD_UP 0x00010090
#define HID_GD_DOWN 0x00010091
@@ -437,6 +438,8 @@ struct hid_usage {
unsigned hid; /* hid usage code */
unsigned collection_index; /* index into collection array */
unsigned usage_index; /* index into usage array */
+ __s8 resolution_multiplier;/* Effective Resolution Multiplier
+ (HUT v1.12, 4.3.1), default: 1 */
/* hidinput data */
__u16 code; /* input driver code */
__u8 type; /* input driver type */
@@ -894,6 +897,8 @@ struct hid_report *hid_validate_values(struct hid_device *hid,
unsigned int type, unsigned int id,
unsigned int field_index,
unsigned int report_counts);
+
+void hid_setup_resolution_multiplier(struct hid_device *hid);
int hid_open_report(struct hid_device *device);
int hid_check_keys_pressed(struct hid_device *hid);
int hid_connect(struct hid_device *hid, unsigned int connect_mask);
--
2.19.2

2018-12-05 00:44:10

[permalink] [raw]

Subject: [PATCH v3 4/8] HID: input: use the Resolution Multiplier for high-resolution scrolling

Windows uses a magic number of 120 for a wheel click. High-resolution
scroll wheels are supposed to use a fraction of 120 to signal smaller
scroll steps. This is implemented by the Resolution Multiplier in the
device itself.

If the multiplier is present in the report descriptor, set it to the
logical max and then use the resolution multiplier to calculate the
high-resolution events. This is the recommendation by Microsoft, see
http://msdn.microsoft.com/en-us/windows/hardware/gg487477.aspx

Note that all mice encountered so far have a logical min/max of 0/1, so
it's a binary "yes or no" to high-res scrolling anyway.

To make userspace simpler, always enable the REL_WHEEL_HI_RES bit. Where
the device doesn't support high-resolution scrolling, the value for the
high-res data will simply be a multiple of 120 every time. For userspace,
if REL_WHEEL_HI_RES is available that is the one to be used.

Potential side-effect: a device with a Resolution Multiplier applying to
other Input items will have those items set to the logical max as well.
This cannot easily be worked around but it is doubtful such devices exist.

Signed-off-by: Peter Hutterer <[email protected]>
---
Changes since v1:
- drop the wheel factor and calculate the hi-res value as the event comes
in. This fixes the issue with a multiplier of 16, makes the code simpler
too because we don't have to refresh anything after setting the
multiplier.

Changes since v2:
- unchanged

drivers/hid/hid-input.c | 108 ++++++++++++++++++++++++++++++++++++++--
include/linux/hid.h | 3 ++
2 files changed, 108 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index d6fab5798487..59a5608b8dc0 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -712,7 +712,15 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
map_abs_clear(usage->hid & 0xf);
break;

- case HID_GD_SLIDER: case HID_GD_DIAL: case HID_GD_WHEEL:
+ case HID_GD_WHEEL:
+ if (field->flags & HID_MAIN_ITEM_RELATIVE) {
+ set_bit(REL_WHEEL, input->relbit);
+ map_rel(REL_WHEEL_HI_RES);
+ } else {
+ map_abs(usage->hid & 0xf);
+ }
+ break;
+ case HID_GD_SLIDER: case HID_GD_DIAL:
if (field->flags & HID_MAIN_ITEM_RELATIVE)
map_rel(usage->hid & 0xf);
else
@@ -1012,7 +1020,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
case 0x22f: map_key_clear(KEY_ZOOMRESET); break;
case 0x233: map_key_clear(KEY_SCROLLUP); break;
case 0x234: map_key_clear(KEY_SCROLLDOWN); break;
- case 0x238: map_rel(REL_HWHEEL); break;
+ case 0x238: /* AC Pan */
+ set_bit(REL_HWHEEL, input->relbit);
+ map_rel(REL_HWHEEL_HI_RES);
+ break;
case 0x23d: map_key_clear(KEY_EDIT); break;
case 0x25f: map_key_clear(KEY_CANCEL); break;
case 0x269: map_key_clear(KEY_INSERT); break;
@@ -1200,6 +1211,38 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel

}

+static void hidinput_handle_scroll(struct hid_usage *usage,
+ struct input_dev *input,
+ __s32 value)
+{
+ int code;
+ int hi_res, lo_res;
+
+ if (value == 0)
+ return;
+
+ if (usage->code == REL_WHEEL_HI_RES)
+ code = REL_WHEEL;
+ else
+ code = REL_HWHEEL;
+
+ /*
+ * Windows reports one wheel click as value 120. Where a high-res
+ * scroll wheel is present, a fraction of 120 is reported instead.
+ * Our REL_WHEEL_HI_RES axis does the same because all HW must
+ * adhere to the 120 expectation.
+ */
+ hi_res = value * 120/usage->resolution_multiplier;
+
+ usage->wheel_accumulated += hi_res;
+ lo_res = usage->wheel_accumulated/120;
+ if (lo_res)
+ usage->wheel_accumulated -= lo_res * 120;
+
+ input_event(input, EV_REL, code, lo_res);
+ input_event(input, EV_REL, usage->code, hi_res);
+}
+
void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, __s32 value)
{
struct input_dev *input;
@@ -1262,6 +1305,12 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
if ((usage->type == EV_KEY) && (usage->code == 0)) /* Key 0 is "unassigned", not KEY_UNKNOWN */
return;

+ if ((usage->type == EV_REL) && (usage->code == REL_WHEEL_HI_RES ||
+ usage->code == REL_HWHEEL_HI_RES)) {
+ hidinput_handle_scroll(usage, input, value);
+ return;
+ }
+
if ((usage->type == EV_ABS) && (field->flags & HID_MAIN_ITEM_RELATIVE) &&
(usage->code == ABS_VOLUME)) {
int count = abs(value);
@@ -1489,6 +1538,58 @@ static void hidinput_close(struct input_dev *dev)
hid_hw_close(hid);
}

+static void hidinput_change_resolution_multipliers(struct hid_device *hid)
+{
+ struct hid_report_enum *rep_enum;
+ struct hid_report *rep;
+ struct hid_usage *usage;
+ int i, j;
+
+ rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
+ list_for_each_entry(rep, &rep_enum->report_list, list) {
+ bool update_needed = false;
+
+ if (rep->maxfield == 0)
+ continue;
+
+ /*
+ * If we have more than one feature within this report we
+ * need to fill in the bits from the others before we can
+ * overwrite the ones for the Resolution Multiplier.
+ */
+ if (rep->maxfield > 1) {
+ hid_hw_request(hid, rep, HID_REQ_GET_REPORT);
+ hid_hw_wait(hid);
+ }
+
+ for (i = 0; i < rep->maxfield; i++) {
+ __s32 logical_max = rep->field[i]->logical_maximum;
+
+ /* There is no good reason for a Resolution
+ * Multiplier to have a count other than 1.
+ * Ignore that case.
+ */
+ if (rep->field[i]->report_count != 1)
+ continue;
+
+ for (j = 0; j < rep->field[i]->maxusage; j++) {
+ usage = &rep->field[i]->usage[j];
+
+ if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
+ continue;
+
+ *rep->field[i]->value = logical_max;
+ update_needed = true;
+ }
+ }
+ if (update_needed)
+ hid_hw_request(hid, rep, HID_REQ_SET_REPORT);
+ }
+
+ /* refresh our structs */
+ hid_setup_resolution_multiplier(hid);
+}
+
static void report_features(struct hid_device *hid)
{
struct hid_driver *drv = hid->driver;
@@ -1782,6 +1883,8 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
}
}

+ hidinput_change_resolution_multipliers(hid);
+
list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
if (drv->input_configured &&
drv->input_configured(hid, hidinput))
@@ -1840,4 +1943,3 @@ void hidinput_disconnect(struct hid_device *hid)
cancel_work_sync(&hid->led_work);
}
EXPORT_SYMBOL_GPL(hidinput_disconnect);
-
diff --git a/include/linux/hid.h b/include/linux/hid.h
index fd8d860365a4..93db548f8761 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -233,6 +233,7 @@ struct hid_item {
#define HID_DC_BATTERYSTRENGTH 0x00060020

#define HID_CP_CONSUMER_CONTROL 0x000c0001
+#define HID_CP_AC_PAN 0x000c0238

#define HID_DG_DIGITIZER 0x000d0001
#define HID_DG_PEN 0x000d0002
@@ -441,11 +442,13 @@ struct hid_usage {
__s8 resolution_multiplier;/* Effective Resolution Multiplier
(HUT v1.12, 4.3.1), default: 1 */
/* hidinput data */
+ __s8 wheel_factor; /* 120/resolution_multiplier */
__u16 code; /* input driver code */
__u8 type; /* input driver type */
__s8 hat_min; /* hat switch fun */
__s8 hat_max; /* ditto */
__s8 hat_dir; /* ditto */
+ __s16 wheel_accumulated; /* hi-res wheel */
};

struct hid_input;
--
2.19.2

2018-12-05 00:44:15

[permalink] [raw]

Subject: [PATCH v3 6/8] HID: logitech: Add function to enable HID++ 1.0 "scrolling acceleration"

From: Harry Cutts <[email protected]>

"Scrolling acceleration" is a bit of a misnomer: it doesn't deal with
acceleration at all. However, that's the name used in Logitech's spec,
so I used it here.

Signed-off-by: Harry Cutts <[email protected]>
Reviewed-by: Benjamin Tissoires <[email protected]>
Signed-off-by: Peter Hutterer <[email protected]>
---
Unchanged since v1

drivers/hid/hid-logitech-hidpp.c | 47 +++++++++++++++++++++++---------
1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 22b37a3844d1..95ba9db6e613 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -400,32 +400,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 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);
}

#define HIDPP_REG_BATTERY_STATUS 0x07
--
2.19.2

2018-12-05 00:44:18

[permalink] [raw]

Subject: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

From: Harry Cutts <[email protected]>

There are three features used by various Logitech mice for
high-resolution scrolling: the scrolling acceleration 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 (using the Unifying receiver):

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

This patch is a combinations of the now-reverted commits 1ff2e1a44e0,
d56ca9855bf9, 5fe2ccbef9d, 044ee89028 together with some extra bits for the
directional and timeout-based reset.
The previous patch series was in hid-input, it appears this remainder
handling is logitech-specific and was moved to hid-logitech-hidpp.c and
renamed accordingly.

Signed-off-by: Harry Cutts <[email protected]>
Signed-off-by: Peter Hutterer <[email protected]>
---
Changes to v1:
- get rid of the timer in favour of sched_clock()
- drop storing the multiplier and calculate value on the fly

Changes to v2:
- m560 now has REL_HWHEEL_HI_RES (untested, I don't have the device)

drivers/hid/hid-logitech-hidpp.c | 301 ++++++++++++++++++++++++++++++-
1 file changed, 295 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 95ba9db6e613..a66daf8acd87 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -21,6 +21,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/sched.h>
+#include <linux/sched/clock.h>
#include <linux/kfifo.h>
#include <linux/input/mt.h>
#include <linux/workqueue.h>
@@ -64,6 +65,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

@@ -128,6 +137,25 @@ struct hidpp_battery {
bool online;
};

+/**
+ * struct hidpp_scroll_counter - Utility class for processing high-resolution
+ * scroll events.
+ * @dev: the input device for which events should be reported.
+ * @wheel_multiplier: the scalar multiplier to be applied to each wheel event
+ * @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.
+ * @direction: direction of last movement (1 or -1)
+ * @last_time: last event time, used to reset remainder after inactivity
+ */
+struct hidpp_scroll_counter {
+ struct input_dev *dev;
+ int wheel_multiplier;
+ int remainder;
+ int direction;
+ unsigned long long last_time;
+};
+
struct hidpp_device {
struct hid_device *hid_dev;
struct mutex send_mutex;
@@ -149,6 +177,7 @@ struct hidpp_device {
unsigned long capabilities;

struct hidpp_battery battery;
+ struct hidpp_scroll_counter vertical_wheel_counter;
};

/* HID++ 1.0 error codes */
@@ -391,6 +420,67 @@ static void hidpp_prefix_name(char **name, int name_length)
*name = new_name;
}

+/**
+ * hidpp_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
+ * fractions of 120 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.
+ */
+static void hidpp_scroll_counter_handle_scroll(struct hidpp_scroll_counter *counter,
+ int hi_res_value)
+{
+ int low_res_value, remainder, direction;
+ unsigned long long now, previous;
+
+ hi_res_value = hi_res_value * 120/counter->wheel_multiplier;
+ input_report_rel(counter->dev, REL_WHEEL_HI_RES, hi_res_value);
+
+ remainder = counter->remainder;
+ direction = hi_res_value > 0 ? 1 : -1;
+
+ now = sched_clock();
+ previous = counter->last_time;
+ counter->last_time = now;
+ /*
+ * Reset the remainder after a period of inactivity or when the
+ * direction changes. This prevents the REL_WHEEL emulation point
+ * from sliding for devices that don't always provide the same
+ * number of movements per detent.
+ */
+ if (now - previous > 1000000000 || direction != counter->direction)
+ remainder = 0;
+
+ counter->direction = direction;
+ remainder += hi_res_value;
+
+ /* Some wheels will rest 7/8ths of a detent from the previous detent
+ * after slow movement, so we want the threshold for low-res events to
+ * be in the middle between two detents (e.g. after 4/8ths) as
+ * opposed to on the detents themselves (8/8ths).
+ */
+ if (abs(remainder) >= 60) {
+ /* Add (or subtract) 1 because we want to trigger when the wheel
+ * is half-way to the next detent (i.e. scroll 1 detent after a
+ * 1/2 detent movement, 2 detents after a 1 1/2 detent movement,
+ * etc.).
+ */
+ low_res_value = remainder / 120;
+ if (low_res_value == 0)
+ low_res_value = (hi_res_value > 0 ? 1 : -1);
+ input_report_rel(counter->dev, REL_WHEEL, low_res_value);
+ remainder -= low_res_value * 120;
+ }
+ counter->remainder = remainder;
+}
+
/* -------------------------------------------------------------------------- */
/* HIDP++ 1.0 commands */
/* -------------------------------------------------------------------------- */
@@ -1157,6 +1247,99 @@ 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:
+ hid_warn(hidpp->hid_dev,
+ "Couldn't get wheel multiplier (error %d)\n", ret);
+ 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 */
/* -------------------------------------------------------------------------- */
@@ -2408,10 +2591,15 @@ static int m560_raw_event(struct hid_device *hdev, u8 *data, int size)
input_report_key(mydata->input, BTN_RIGHT,
!!(data[1] & M560_MOUSE_BTN_RIGHT));

- if (data[1] & M560_MOUSE_BTN_WHEEL_LEFT)
+ if (data[1] & M560_MOUSE_BTN_WHEEL_LEFT) {
input_report_rel(mydata->input, REL_HWHEEL, -1);
- else if (data[1] & M560_MOUSE_BTN_WHEEL_RIGHT)
+ input_report_rel(mydata->input, REL_HWHEEL_HI_RES,
+ -120);
+ } else if (data[1] & M560_MOUSE_BTN_WHEEL_RIGHT) {
input_report_rel(mydata->input, REL_HWHEEL, 1);
+ input_report_rel(mydata->input, REL_HWHEEL_HI_RES,
+ 120);
+ }

v = hid_snto32(hid_field_extract(hdev, data+3, 0, 12), 12);
input_report_rel(mydata->input, REL_X, v);
@@ -2420,7 +2608,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);
+ hidpp_scroll_counter_handle_scroll(
+ &hidpp->vertical_wheel_counter, v);

input_sync(mydata->input);
}
@@ -2447,6 +2636,8 @@ static void m560_populate_input(struct hidpp_device *hidpp,
__set_bit(REL_Y, mydata->input->relbit);
__set_bit(REL_WHEEL, mydata->input->relbit);
__set_bit(REL_HWHEEL, mydata->input->relbit);
+ __set_bit(REL_WHEEL_HI_RES, mydata->input->relbit);
+ __set_bit(REL_HWHEEL_HI_RES, mydata->input->relbit);
}

static int m560_input_mapping(struct hid_device *hdev, struct hid_input *hi,
@@ -2548,6 +2739,37 @@ static int g920_get_config(struct hidpp_device *hidpp)
return 0;
}

+/* -------------------------------------------------------------------------- */
+/* High-resolution scroll wheels */
+/* -------------------------------------------------------------------------- */
+
+static int hi_res_scroll_enable(struct hidpp_device *hidpp)
+{
+ int ret;
+ u8 multiplier = 1;
+
+ if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_X2121) {
+ ret = hidpp_hrw_set_wheel_mode(hidpp, false, true, false);
+ if (ret == 0)
+ ret = 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;
+
+ if (multiplier == 0)
+ multiplier = 1;
+
+ hidpp->vertical_wheel_counter.wheel_multiplier = multiplier;
+ hid_info(hidpp->hid_dev, "multiplier = %d\n", multiplier);
+ return 0;
+}
+
/* -------------------------------------------------------------------------- */
/* Generic HID++ devices */
/* -------------------------------------------------------------------------- */
@@ -2593,6 +2815,9 @@ 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)
+ hidpp->vertical_wheel_counter.dev = input;
}

static int hidpp_input_configured(struct hid_device *hdev,
@@ -2711,6 +2936,27 @@ 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 hidpp_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 hidpp_scroll_counter_handle_scroll.
+ */
+ if (!(hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL) || value == 0
+ || counter->dev == NULL || counter->wheel_multiplier == 0)
+ return 0;
+
+ hidpp_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);
@@ -2922,6 +3168,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;
@@ -3107,6 +3356,10 @@ 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,
@@ -3121,10 +3374,39 @@ 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 */
- 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 },
+ { /* 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 },
{ /* Keyboard logitech K400 */
HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
USB_VENDOR_ID_LOGITECH, 0x4024),
@@ -3144,12 +3426,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_HI_RES },
+ { 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.2

2018-12-05 00:44:25

[permalink] [raw]

Subject: [PATCH v3 8/8] HID: logitech: Use LDJ_DEVICE macro for existing Logitech mice

From: Harry Cutts <[email protected]>

Signed-off-by: Harry Cutts <[email protected]>
Reviewed-by: Benjamin Tissoires <[email protected]>
Signed-off-by: Peter Hutterer <[email protected]>
---
Changes to v1, v2:
- reordered from 6/8 to 8/8 so the intermediate steps all build

drivers/hid/hid-logitech-hidpp.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index a66daf8acd87..15ed6177a7a3 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3362,13 +3362,11 @@ static void hidpp_remove(struct hid_device *hdev)

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,
@@ -3408,16 +3406,13 @@ 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 */
- 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},
--
2.19.2

2018-12-05 00:44:46

[permalink] [raw]

Subject: [PATCH v3 5/8] HID: logitech-hidpp: fix typo, hiddpp to hidpp

Signed-off-by: Peter Hutterer <[email protected]>
---
Unchanged since v1

drivers/hid/hid-logitech-hidpp.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 19cc980eebce..22b37a3844d1 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1465,7 +1465,7 @@ struct hidpp_ff_work_data {
u8 size;
};

-static const signed short hiddpp_ff_effects[] = {
+static const signed short hidpp_ff_effects[] = {
FF_CONSTANT,
FF_PERIODIC,
FF_SINE,
@@ -1480,7 +1480,7 @@ static const signed short hiddpp_ff_effects[] = {
-1
};

-static const signed short hiddpp_ff_effects_v2[] = {
+static const signed short hidpp_ff_effects_v2[] = {
FF_RAMP,
FF_FRICTION,
FF_INERTIA,
@@ -1873,11 +1873,11 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
version = bcdDevice & 255;

/* Set supported force feedback capabilities */
- for (j = 0; hiddpp_ff_effects[j] >= 0; j++)
- set_bit(hiddpp_ff_effects[j], dev->ffbit);
+ for (j = 0; hidpp_ff_effects[j] >= 0; j++)
+ set_bit(hidpp_ff_effects[j], dev->ffbit);
if (version > 1)
- for (j = 0; hiddpp_ff_effects_v2[j] >= 0; j++)
- set_bit(hiddpp_ff_effects_v2[j], dev->ffbit);
+ for (j = 0; hidpp_ff_effects_v2[j] >= 0; j++)
+ set_bit(hidpp_ff_effects_v2[j], dev->ffbit);

/* Read number of slots available in device */
error = hidpp_send_fap_command_sync(hidpp, feature_index,
--
2.19.2

2018-12-05 00:45:18

[permalink] [raw]

Subject: [PATCH v3 1/8] Input: add `REL_WHEEL_HI_RES` and `REL_HWHEEL_HI_RES`

This event code represents scroll reports from high-resolution wheels and
is modelled after the approach Windows uses. The value 120 is one detent
(wheel click) of movement. Mice with higher-resolution scrolling can send
fractions of 120 which must be accumulated in userspace. Userspace can either
wait for a full 120 to accumulate or scroll by fractions of one logical scroll
movement as the events come in. 120 was picked as magic number because it has
a high number of integer fractions that can be used by high-resolution wheels.

For more information see
https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn613912(v=vs.85)

These new axes obsolete REL_WHEEL and REL_HWHEEL. The legacy axes are emulated
by the kernel but the most accurate (and most granular) data is available
through the new axes.

Signed-off-by: Peter Hutterer <[email protected]>
---
No changes since v1

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

diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
index a8c0873beb95..b24b5343f5eb 100644
--- a/Documentation/input/event-codes.rst
+++ b/Documentation/input/event-codes.rst
@@ -190,7 +190,26 @@ 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 detents moved on the wheel, the
+ physical size of which varies by device. For high-resolution wheels
+ this may be an approximation based on the high-resolution scroll events,
+ see REL_WHEEL_HI_RES. These event codes are legacy codes and
+ REL_WHEEL_HI_RES and REL_HWHEEL_HI_RES should be preferred where
+ available.
+
+* REL_WHEEL_HI_RES, REL_HWHEEL_HI_RES:
+
+ - High-resolution scroll wheel data. The accumulated value 120 represents
+ movement by one detent. For devices that do not provide high-resolution
+ scrolling, the value is always a multiple of 120. For devices with
+ high-resolution scrolling, the value may be a fraction of 120.
+
+ If a vertical scroll wheel supports high-resolution scrolling, this code
+ will be emitted in addition to REL_WHEEL or REL_HWHEEL. The REL_WHEEL
+ and REL_HWHEEL may be an approximation based on the high-resolution
+ scroll events. There is no guarantee that the high-resolution data
+ is a multiple of 120 at the time of an emulated REL_WHEEL or REL_HWHEEL
+ event.

EV_ABS
------
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 3eb5a4c3d60a..265ef2028660 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -716,6 +716,8 @@
* the situation described above.
*/
#define REL_RESERVED 0x0a
+#define REL_WHEEL_HI_RES 0x0b
+#define REL_HWHEEL_HI_RES 0x0c
#define REL_MAX 0x0f
#define REL_CNT (REL_MAX+1)

--
2.19.2

2018-12-05 00:46:21

[permalink] [raw]

Subject: [PATCH v3 2/8] HID: core: store the collections as a basic tree

For each collection parsed, store a pointer to the parent collection
(if any). This makes it a lot easier to look up which collection(s)
any given item is part of

Signed-off-by: Peter Hutterer <[email protected]>
---
No changes since v1

drivers/hid/hid-core.c | 4 ++++
include/linux/hid.h | 2 ++
2 files changed, 6 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 5bec9244c45b..43d488a45120 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -172,6 +172,8 @@ static int open_collection(struct hid_parser *parser, unsigned type)
collection->type = type;
collection->usage = usage;
collection->level = parser->collection_stack_ptr - 1;
+ collection->parent = parser->active_collection;
+ parser->active_collection = collection;

if (type == HID_COLLECTION_APPLICATION)
parser->device->maxapplication++;
@@ -190,6 +192,8 @@ static int close_collection(struct hid_parser *parser)
return -EINVAL;
}
parser->collection_stack_ptr--;
+ if (parser->active_collection)
+ parser->active_collection = parser->active_collection->parent;
return 0;
}

diff --git a/include/linux/hid.h b/include/linux/hid.h
index a355d61940f2..fdfda898656c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -427,6 +427,7 @@ struct hid_local {
*/

struct hid_collection {
+ struct hid_collection *parent;
unsigned type;
unsigned usage;
unsigned level;
@@ -650,6 +651,7 @@ struct hid_parser {
unsigned int *collection_stack;
unsigned int collection_stack_ptr;
unsigned int collection_stack_size;
+ struct hid_collection *active_collection;
struct hid_device *device;
unsigned int scan_flags;
};
--
2.19.2

2018-12-05 21:54:12

by Harry Cutts

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

Hi Peter,

On Tue, 4 Dec 2018 at 16:43, Peter Hutterer <[email protected]> wrote:
> Changes to v2:
> - m560 now has REL_HWHEEL_HI_RES (untested, I don't have the device)

I just tested with my M560, and it now reports REL_HWHEEL_HI_RES correctly.

Verified-by: Harry Cutts <[email protected]>

Thanks,

Harry Cutts
Chrome OS Touch/Input team

2018-12-05 21:58:22

by Harry Cutts

[permalink] [raw]

Subject: Re: [PATCH v3 0/8] HID: MS and Logitech high-resolution scroll wheel support

On Tue, 4 Dec 2018 at 16:42, Peter Hutterer <[email protected]> wrote:
>
> A full explanation of why and what is in the v1, v2 patch thread here:
> https://lkml.org/lkml/2018/11/22/625
>
> v3 adds a better commit messages, m560 REL_HWHEEL_HI_RES support and a patch
> moved in the ordering. This is a full patch sequence because Benjamin's
> magic scripts struggle with singular updates ;)

I've retested with the same Logitech mice as before, except for the MX
Anywhere 2S, which is currently not available to me. So, for
reference, that's the MX Master 2S, Performance MX, M560, Anywhere MX,
and the M325 (to check low-resolution scrolling).

Verified-by: Harry Cutts <[email protected]>

Harry Cutts
Chrome OS Touch/Input team

2018-12-06 22:59:27

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH v3 1/8] Input: add `REL_WHEEL_HI_RES` and `REL_HWHEEL_HI_RES`

On Wed, Dec 05, 2018 at 10:42:21AM +1000, Peter Hutterer wrote:
> This event code represents scroll reports from high-resolution wheels and
> is modelled after the approach Windows uses. The value 120 is one detent
> (wheel click) of movement. Mice with higher-resolution scrolling can send
> fractions of 120 which must be accumulated in userspace. Userspace can either
> wait for a full 120 to accumulate or scroll by fractions of one logical scroll
> movement as the events come in. 120 was picked as magic number because it has
> a high number of integer fractions that can be used by high-resolution wheels.
>
> For more information see
> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn613912(v=vs.85)
>
> These new axes obsolete REL_WHEEL and REL_HWHEEL. The legacy axes are emulated
> by the kernel but the most accurate (and most granular) data is available
> through the new axes.
>
> Signed-off-by: Peter Hutterer <[email protected]>

Acked-by: Dmitry Torokhov <[email protected]>

> ---
> No changes since v1
>
> Documentation/input/event-codes.rst | 21 ++++++++++++++++++++-
> include/uapi/linux/input-event-codes.h | 2 ++
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
> index a8c0873beb95..b24b5343f5eb 100644
> --- a/Documentation/input/event-codes.rst
> +++ b/Documentation/input/event-codes.rst
> @@ -190,7 +190,26 @@ 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 detents moved on the wheel, the
> + physical size of which varies by device. For high-resolution wheels
> + this may be an approximation based on the high-resolution scroll events,
> + see REL_WHEEL_HI_RES. These event codes are legacy codes and
> + REL_WHEEL_HI_RES and REL_HWHEEL_HI_RES should be preferred where
> + available.
> +
> +* REL_WHEEL_HI_RES, REL_HWHEEL_HI_RES:
> +
> + - High-resolution scroll wheel data. The accumulated value 120 represents
> + movement by one detent. For devices that do not provide high-resolution
> + scrolling, the value is always a multiple of 120. For devices with
> + high-resolution scrolling, the value may be a fraction of 120.
> +
> + If a vertical scroll wheel supports high-resolution scrolling, this code
> + will be emitted in addition to REL_WHEEL or REL_HWHEEL. The REL_WHEEL
> + and REL_HWHEEL may be an approximation based on the high-resolution
> + scroll events. There is no guarantee that the high-resolution data
> + is a multiple of 120 at the time of an emulated REL_WHEEL or REL_HWHEEL
> + event.
>
> EV_ABS
> ------
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 3eb5a4c3d60a..265ef2028660 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -716,6 +716,8 @@
> * the situation described above.
> */
> #define REL_RESERVED 0x0a
> +#define REL_WHEEL_HI_RES 0x0b
> +#define REL_HWHEEL_HI_RES 0x0c
> #define REL_MAX 0x0f
> #define REL_CNT (REL_MAX+1)
>
> --
> 2.19.2
>

--
Dmitry

2018-12-07 16:21:03

[permalink] [raw]

Subject: Re: [PATCH v3 0/8] HID: MS and Logitech high-resolution scroll wheel support

On Wed, Dec 5, 2018 at 10:57 PM Harry Cutts <[email protected]> wrote:
>
> On Tue, 4 Dec 2018 at 16:42, Peter Hutterer <[email protected]> wrote:
> >
> > A full explanation of why and what is in the v1, v2 patch thread here:
> > https://lkml.org/lkml/2018/11/22/625
> >
> > v3 adds a better commit messages, m560 REL_HWHEEL_HI_RES support and a patch
> > moved in the ordering. This is a full patch sequence because Benjamin's
> > magic scripts struggle with singular updates ;)
>
> I've retested with the same Logitech mice as before, except for the MX
> Anywhere 2S, which is currently not available to me. So, for
> reference, that's the MX Master 2S, Performance MX, M560, Anywhere MX,
> and the M325 (to check low-resolution scrolling).
>
> Verified-by: Harry Cutts <[email protected]>

Thanks everybody, especially Peter for respinning and re-writing the
series, and writing the tests in hid-tools.

I was a little bit afraid when I checked `libinput debug-gui` as the
low res cursor was not moving if I were to scroll back and forth of
just one notch. However, checking debug-events of a non high-res aware
libinput showed that the low res wheel events were correctly emitted.
So I guess this is a debug-gui issue.

I have now queued this for 4.21. I do hope this will be smoother this time.

Cheers,
Benjamin

2018-12-14 13:48:56

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

Le mer. 5 déc. 2018 à 01:44, Peter Hutterer <[email protected]> a écrit :
>
> From: Harry Cutts <[email protected]>
>
> There are three features used by various Logitech mice for
> high-resolution scrolling: the scrolling acceleration 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.

Hi, The G500s (and the G500 too, I think) does support the "scrolling
acceleration" bit. If I set it, I get around 8 events for each wheel
"click", this is what this driver expects, right? If I understood
correctly, I should try this patch with the
HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.

2018-12-14 18:39:29

by Harry Cutts

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

Hi Clement,

On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
<[email protected]> wrote:
> Hi, The G500s (and the G500 too, I think) does support the "scrolling
> acceleration" bit. If I set it, I get around 8 events for each wheel
> "click", this is what this driver expects, right? If I understood
> correctly, I should try this patch with the
> HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.

Thanks for the info! Yes, that should work.

Harry Cutts
Chrome OS Touch/Input team

2018-12-15 14:46:39

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

Le ven. 14 déc. 2018 à 19:37, Harry Cutts <[email protected]> a écrit :
>
> Hi Clement,
>
> On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> <[email protected]> wrote:
> > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > acceleration" bit. If I set it, I get around 8 events for each wheel
> > "click", this is what this driver expects, right? If I understood
> > correctly, I should try this patch with the
> > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
>
> Thanks for the info! Yes, that should work.

Well, it is not that simple. I get "Device not connected" errors for
both interfaces of the mouse.

logitech-hidpp-device 0003:046D:C24E.000E: Device not connected
logitech-hidpp-device 0003:046D:C24E.000F: Device not connected

Here is the usbmon trace when connecting a G500s:

0cd42cc0 3.313741 C Ii:3:001:1 0:2048 1 =
04
0cd42cc0 3.313750 S Ii:3:001:1 -:2048 4 <
17b49a80 3.313764 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b49a80 3.313775 C Ci:3:001:0 0 4 =
01010100
17b49a80 3.313781 S Co:3:001:0 s 23 01 0010 0002 0000 0
17b49a80 3.313789 C Co:3:001:0 0 0
17b49a80 3.313792 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b49a80 3.313797 C Ci:3:001:0 0 4 =
01010000
17b49a80 3.340415 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b49a80 3.340438 C Ci:3:001:0 0 4 =
01010000
17b49a80 3.367434 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b49a80 3.367448 C Ci:3:001:0 0 4 =
01010000
17b49a80 3.394434 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b49a80 3.394449 C Ci:3:001:0 0 4 =
01010000
17b49a80 3.421416 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b49a80 3.421431 C Ci:3:001:0 0 4 =
01010000
17b49a80 3.421690 S Co:3:001:0 s 23 03 0004 0002 0000 0
17b49a80 3.421707 C Co:3:001:0 0 0
17b49a80 3.483421 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b49a80 3.483436 C Ci:3:001:0 0 4 =
11010000
17b499c0 3.545429 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b499c0 3.545442 C Ci:3:001:0 0 4 =
03011000
17b499c0 3.545448 S Co:3:001:0 s 23 01 0014 0002 0000 0
17b499c0 3.545456 C Co:3:001:0 0 0
17b499c0 3.597659 S Ci:3:000:0 s 80 06 0100 0000 0040 64 <
17b499c0 3.597851 C Ci:3:000:0 0 8 =
12010002 00000008
17b499c0 3.597870 S Co:3:001:0 s 23 03 0004 0002 0000 0
17b499c0 3.597884 C Co:3:001:0 0 0
17b499c0 3.659419 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b499c0 3.659434 C Ci:3:001:0 0 4 =
11010000
17b499c0 3.721421 S Ci:3:001:0 s a3 00 0000 0002 0004 4 <
17b499c0 3.721435 C Ci:3:001:0 0 4 =
03011000
17b499c0 3.721442 S Co:3:001:0 s 23 01 0014 0002 0000 0
17b499c0 3.721451 C Co:3:001:0 0 0
17b49600 3.788420 S Ci:3:007:0 s 80 06 0100 0000 0012 18 <
17b49600 3.788897 C Ci:3:007:0 0 18 =
12010002 00000008 6d044ec2 01840102 0301
. . . . . . . . m . N . . . . . . .
17b49600 3.788920 S Ci:3:007:0 s 80 06 0600 0000 000a 10 <
17b49600 3.789057 C Ci:3:007:0 -32 0
17b49600 3.789092 S Ci:3:007:0 s 80 06 0600 0000 000a 10 <
17b49600 3.789438 C Ci:3:007:0 -32 0
17b49600 3.789459 S Ci:3:007:0 s 80 06 0600 0000 000a 10 <
17b49600 3.789827 C Ci:3:007:0 -32 0
17b49600 3.789850 S Ci:3:007:0 s 80 06 0200 0000 0009 9 <
17b49600 3.790272 C Ci:3:007:0 0 9 =
09023b00 020104a0 31
. . ; . . . . . 1
17b49600 3.790285 S Ci:3:007:0 s 80 06 0200 0000 003b 59 <
17b49600 3.791004 C Ci:3:007:0 0 59 =
09023b00 020104a0 31090400 00010301 02000921 11010001 22430007 05810308
. . ; . . . . . 1 . . . . . . . . . . ! . . . . " C . . . . . .
17b49600 3.791021 S Ci:3:007:0 s 80 06 0300 0000 00ff 255 <
17b49600 3.791203 C Ci:3:007:0 0 4 =
04030904
17b49600 3.791212 S Ci:3:007:0 s 80 06 0302 0409 00ff 255 <
17b49600 3.791829 C Ci:3:007:0 0 50 =
32034700 35003000 30007300 20004c00 61007300 65007200 20004700 61006d00
2 . G . 5 . 0 . 0 . s . . L . a . s . e . r . . G . a . m .
17b49600 3.791844 S Ci:3:007:0 s 80 06 0301 0409 00ff 255 <
17b49600 3.792141 C Ci:3:007:0 0 18 =
12034c00 6f006700 69007400 65006300 6800
. . L . o . g . i . t . e . c . h .
17b49600 3.792154 S Ci:3:007:0 s 80 06 0303 0409 00ff 255 <
17b49600 3.792563 C Ci:3:007:0 0 30 =
1e033300 34004500 31003300 38003500 30003800 36003000 30003000 3900
. . 3 . 4 . E . 1 . 3 . 8 . 5 . 0 . 8 . 6 . 0 . 0 . 0 . 9 .
17b49300 3.795944 S Co:3:007:0 s 00 09 0001 0000 0000 0
17b49300 3.795998 C Co:3:007:0 0 0
17b49300 3.796025 S Ci:3:007:0 s 80 06 0304 0409 00ff 255 <
17b49300 3.796392 C Ci:3:007:0 0 26 =
1a035500 38003400 2e003000 31005f00 42003000 30003000 3900
. . U . 8 . 4 . . . 0 . 1 . _ . B . 0 . 0 . 0 . 9 .
17b49900 3.796473 S Ci:3:007:0 s 80 06 0303 0409 00ff 255 <
17b49900 3.796883 C Ci:3:007:0 0 30 =
1e033300 34004500 31003300 38003500 30003800 36003000 30003000 3900
. . 3 . 4 . E . 1 . 3 . 8 . 5 . 0 . 8 . 6 . 0 . 0 . 0 . 9 .
17b49900 3.796914 S Co:3:007:0 s 21 0a 0000 0000 0000 0
17b49900 3.797027 C Co:3:007:0 -32 0
17b49900 3.797056 S Ci:3:007:0 s 81 06 2200 0000 0043 67 <
17b49900 3.798055 C Ci:3:007:0 0 67 =
05010902 a1010901 a1000509 19012910 15002501 95107501 81020501 16018026
. . . . . . . . . . . . . . ) . . . % . . . u . . . . . . . . &
17b49840 3.798350 S Co:3:007:0 s 21 09 0211 0000 0014 20 =
11ff0011 00000000 00000000 00000000 00000000
17b49840 3.798500 C Co:3:007:0 0 20 >
17b49240 9.289560 S Ci:3:007:0 s 80 06 0303 0409 00ff 255 <
17b49240 9.289999 C Ci:3:007:0 0 30 =
1e033300 34004500 31003300 38003500 30003800 36003000 30003000 3900
. . 3 . 4 . E . 1 . 3 . 8 . 5 . 0 . 8 . 6 . 0 . 0 . 0 . 9 .
17b49240 9.290040 S Co:3:007:0 s 21 0a 0000 0001 0000 0
17b49240 9.290145 C Co:3:007:0 -32 0
17b49240 9.290155 S Ci:3:007:0 s 81 06 2200 0001 007a 122 <
17b49240 9.291756 C Ci:3:007:0 0 122 =
05010906 a1018501 050719e0 29e71500 25017501 95088102 95057508 150026a4
. . . . . . . . . . . . ) . . . % . u . . . . . . . u . . . & .
17b49c00 9.292167 S Co:3:007:0 s 21 09 0211 0001 0014 20 =
11ff0011 00000000 00000000 00000000 00000000
17b49c00 9.292628 C Co:3:007:0 0 20 >

It looks like the mouse is not responding to the
root_get_protocol_version packet. I don't know why, but even if it
did, it would not work correctly. The HID++ reports will only work
with one of the interfaces, the other should be ignored by the driver
instead of being disabled because it failed to respond to the HID++
report. The G500s and G500 HID++ interface also use the device index 0
instead of 0xff.

For comparison, if I use my distribution kernel
(4.19.7-200.fc28.x86_64) and send reports from user-space (using
hidraw) instead of for-4.21/highres-wheel kernel. I get this kind of
trace:

592193c0 0.253975 S Co:3:003:0 s 21 09 0211 0001 0014 20 =
11ff0011 00000000 00000000 00000000 00000000
592193c0 0.254426 C Co:3:003:0 0 20 >
9ee5ff00 0.255859 C Ii:3:003:2 0:1 7 =
10ff8f00 110800
9ee5ff00 0.255867 S Ii:3:003:2 -:1 20 <
592193c0 11.116794 S Co:3:003:0 s 21 09 0211 0001 0014 20 =
11000011 00000000 00000000 00000000 00000000
592193c0 11.117258 C Co:3:003:0 0 20 >
9ee5ff00 11.118023 C Ii:3:003:2 0:1 7 =
10008f00 110100
9ee5ff00 11.118033 S Ii:3:003:2 -:1 20 <

When using device index 0xff the mouse responds with a
ERR_UNKNOWN_DEVICE (0x08) error, when using device index 0 it responds
ERR_INVALID_SUBID (0x01), the expected error for a HID++1.0 device.

Here is the diff from the for-4.21/highres-wheel branch, I only added
the devices to the device list with the required quirk.

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index ed35c9a9a110..a1c431e7841c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -724,6 +724,7 @@
#define USB_DEVICE_ID_LOGITECH_KEYBOARD_G710_PLUS 0xc24d
#define USB_DEVICE_ID_LOGITECH_MOUSE_C01A 0xc01a
#define USB_DEVICE_ID_LOGITECH_MOUSE_C05A 0xc05a
+#define USB_DEVICE_ID_LOGITECH_MOUSE_G500 0xc068
#define USB_DEVICE_ID_LOGITECH_MOUSE_C06A 0xc06a
#define USB_DEVICE_ID_LOGITECH_RUMBLEPAD_CORD 0xc20a
#define USB_DEVICE_ID_LOGITECH_RUMBLEPAD 0xc211
@@ -731,6 +732,7 @@
#define USB_DEVICE_ID_LOGITECH_DUAL_ACTION 0xc216
#define USB_DEVICE_ID_LOGITECH_RUMBLEPAD2 0xc218
#define USB_DEVICE_ID_LOGITECH_RUMBLEPAD2_2 0xc219
+#define USB_DEVICE_ID_LOGITECH_MOUSE_G500S 0xc24e
#define USB_DEVICE_ID_LOGITECH_G29_WHEEL 0xc24f
#define USB_DEVICE_ID_LOGITECH_G920_WHEEL 0xc262
#define USB_DEVICE_ID_LOGITECH_WINGMAN_F3D 0xc283
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 15ed6177a7a3..b9b34ce455a4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3416,6 +3416,10 @@ static const struct hid_device_id hidpp_devices[] = {

{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_G920_WHEEL),
.driver_data = HIDPP_QUIRK_CLASS_G920 |
HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
+ { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_MOUSE_G500),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_1P0 },
+ { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_MOUSE_G500S),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_1P0 },
{}
};

2018-12-19 12:42:19

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
<[email protected]> a écrit :
>
> Le ven. 14 déc. 2018 à 19:37, Harry Cutts <[email protected]> a écrit :
> >
> > Hi Clement,
> >
> > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > <[email protected]> wrote:
> > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > "click", this is what this driver expects, right? If I understood
> > > correctly, I should try this patch with the
> > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> >
> > Thanks for the info! Yes, that should work.
>
> Well, it is not that simple. I get "Device not connected" errors for
> both interfaces of the mouse.

I suspect the device is not responding because the hid device is not
started. When is hid_hw_start supposed to be called? It is called
early for HID_QUIRK_CLASS_G920 but later for other device. So the
device is not started when hidpp_is_connected is called. Is this
because most of the device in this driver are not real HID devices but
DJ devices? How should non-DJ devices be treated?

2018-12-19 22:12:24

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
<[email protected]> wrote:
>
> Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> <[email protected]> a écrit :
> >
> > Le ven. 14 déc. 2018 à 19:37, Harry Cutts <[email protected]> a écrit :
> > >
> > > Hi Clement,
> > >
> > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > <[email protected]> wrote:
> > > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > > "click", this is what this driver expects, right? If I understood
> > > > correctly, I should try this patch with the
> > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > >
> > > Thanks for the info! Yes, that should work.
> >
> > Well, it is not that simple. I get "Device not connected" errors for
> > both interfaces of the mouse.
>
> I suspect the device is not responding because the hid device is not
> started. When is hid_hw_start supposed to be called? It is called
> early for HID_QUIRK_CLASS_G920 but later for other device. So the
> device is not started when hidpp_is_connected is called. Is this
> because most of the device in this driver are not real HID devices but
> DJ devices? How should non-DJ devices be treated?

Hi Clement,

I have a series I sent last September that allows to support non DJ
devices on logitech-hidpp
(https://patchwork.kernel.org/project/linux-input/list/?series=16359).

In its current form, with the latest upstream kernel, the series will
oops during the .event() callback, which is easy enough to fix.
However, I am currently trying to make it better as a second or third
reading made me realized that there was a bunch of non-sense in it and
a proper support would require slightly more work for the non unifying
receiver case.

I hope I'll be able to send out something by the end of the week.

Cheers,
Benjamin

2019-04-24 21:48:47

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

Hi Benjamin,

I tried again to add hi-res wheel support for the G500 with Hans de
Goede's latest patch series you've just merged in for-5.2/logitech, it
is much better but there is still some issues.

The first one is the device index, I need to use device index 0
instead 0xff. I added a quick and dirty quirk (stealing in the
QUIRK_CLASS range since the normal quirk range looks full) to change
the device index assigned in __hidpp_send_report. After that the
device is correctly initialized and the wheel multiplier is set.

The second issue is that wheel values are not actually scaled
according to the multiplier. I get 7/8 full scroll event for each
wheel step. I think it happens because the mouse is split in two
devices. The first device has the wheel events, and the second device
has the HID++ reports. The wheel multiplier is only set on the second
device (where the hi-res mode is enabled) and does not affect the
wheel events from the first one.

Le mer. 19 déc. 2018 à 21:35, Benjamin Tissoires
<[email protected]> a écrit :
>
> On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
> <[email protected]> wrote:
> >
> > Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> > <[email protected]> a écrit :
> > >
> > > Le ven. 14 déc. 2018 à 19:37, Harry Cutts <[email protected]> a écrit :
> > > >
> > > > Hi Clement,
> > > >
> > > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > > <[email protected]> wrote:
> > > > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > > > "click", this is what this driver expects, right? If I understood
> > > > > correctly, I should try this patch with the
> > > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > > >
> > > > Thanks for the info! Yes, that should work.
> > >
> > > Well, it is not that simple. I get "Device not connected" errors for
> > > both interfaces of the mouse.
> >
> > I suspect the device is not responding because the hid device is not
> > started. When is hid_hw_start supposed to be called? It is called
> > early for HID_QUIRK_CLASS_G920 but later for other device. So the
> > device is not started when hidpp_is_connected is called. Is this
> > because most of the device in this driver are not real HID devices but
> > DJ devices? How should non-DJ devices be treated?
>
> Hi Clement,
>
> I have a series I sent last September that allows to support non DJ
> devices on logitech-hidpp
> (https://patchwork.kernel.org/project/linux-input/list/?series=16359).
>
> In its current form, with the latest upstream kernel, the series will
> oops during the .event() callback, which is easy enough to fix.
> However, I am currently trying to make it better as a second or third
> reading made me realized that there was a bunch of non-sense in it and
> a proper support would require slightly more work for the non unifying
> receiver case.
>
> I hope I'll be able to send out something by the end of the week.
>
> Cheers,
> Benjamin

2019-04-25 08:52:59

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

Le jeu. 25 avr. 2019 à 09:40, Benjamin Tissoires
<[email protected]> a écrit :
>
> Hi Clément,
>
> On Wed, Apr 24, 2019 at 5:34 PM Clément VUCHENER
> <[email protected]> wrote:
> >
> > Hi Benjamin,
> >
> > I tried again to add hi-res wheel support for the G500 with Hans de
> > Goede's latest patch series you've just merged in for-5.2/logitech, it
> > is much better but there is still some issues.
> >
> > The first one is the device index, I need to use device index 0
> > instead 0xff. I added a quick and dirty quirk (stealing in the
> > QUIRK_CLASS range since the normal quirk range looks full) to change
> > the device index assigned in __hidpp_send_report. After that the
> > device is correctly initialized and the wheel multiplier is set.
>
> Hmm, maybe we should restrain a little bit the reserved quirks...
> But actually, .driver_data and .quirks are both unsigned long, so you
> should be able to use the 64 bits.

Only on 64 bits architectures, or is the kernel forcing long integers
to be 64 bits everywhere?

>
> >
> > The second issue is that wheel values are not actually scaled
> > according to the multiplier. I get 7/8 full scroll event for each
> > wheel step. I think it happens because the mouse is split in two
> > devices. The first device has the wheel events, and the second device
> > has the HID++ reports. The wheel multiplier is only set on the second
> > device (where the hi-res mode is enabled) and does not affect the
> > wheel events from the first one.
>
> I would think this have to do with the device not accepting the
> command instead. Can you share some raw logs of the events (ideally
> with hid-recorder from
> https://gitlab.freedesktop.org/libevdev/hid-tools)?

I already checked with usbmon and double-checked by querying the
register. The flag is set and accepted by the device and it behaves
accordingly: it sends about 8 wheel events per step.

hid-recorder takes hidraw nodes as parameters, how do I use it to
record the initialization by the driver?

>
> Cheers,
> Benjamin
>
> >
> > Le mer. 19 déc. 2018 à 21:35, Benjamin Tissoires
> > <[email protected]> a écrit :
> > >
> > > On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
> > > <[email protected]> wrote:
> > > >
> > > > Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> > > > <[email protected]> a écrit :
> > > > >
> > > > > Le ven. 14 déc. 2018 à 19:37, Harry Cutts <[email protected]> a écrit :
> > > > > >
> > > > > > Hi Clement,
> > > > > >
> > > > > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > > > > <[email protected]> wrote:
> > > > > > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > > > > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > > > > > "click", this is what this driver expects, right? If I understood
> > > > > > > correctly, I should try this patch with the
> > > > > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > > > > >
> > > > > > Thanks for the info! Yes, that should work.
> > > > >
> > > > > Well, it is not that simple. I get "Device not connected" errors for
> > > > > both interfaces of the mouse.
> > > >
> > > > I suspect the device is not responding because the hid device is not
> > > > started. When is hid_hw_start supposed to be called? It is called
> > > > early for HID_QUIRK_CLASS_G920 but later for other device. So the
> > > > device is not started when hidpp_is_connected is called. Is this
> > > > because most of the device in this driver are not real HID devices but
> > > > DJ devices? How should non-DJ devices be treated?
> > >
> > > Hi Clement,
> > >
> > > I have a series I sent last September that allows to support non DJ
> > > devices on logitech-hidpp
> > > (https://patchwork.kernel.org/project/linux-input/list/?series=16359).
> > >
> > > In its current form, with the latest upstream kernel, the series will
> > > oops during the .event() callback, which is easy enough to fix.
> > > However, I am currently trying to make it better as a second or third
> > > reading made me realized that there was a bunch of non-sense in it and
> > > a proper support would require slightly more work for the non unifying
> > > receiver case.
> > >
> > > I hope I'll be able to send out something by the end of the week.
> > >
> > > Cheers,
> > > Benjamin

2019-04-25 09:31:59

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

Le jeu. 25 avr. 2019 à 10:45, Benjamin Tissoires
<[email protected]> a écrit :
>
> On Thu, Apr 25, 2019 at 10:25 AM Clément VUCHENER
> <[email protected]> wrote:
> >
> > Le jeu. 25 avr. 2019 à 09:40, Benjamin Tissoires
> > <[email protected]> a écrit :
> > >
> > > Hi Clément,
> > >
> > > On Wed, Apr 24, 2019 at 5:34 PM Clément VUCHENER
> > > <[email protected]> wrote:
> > > >
> > > > Hi Benjamin,
> > > >
> > > > I tried again to add hi-res wheel support for the G500 with Hans de
> > > > Goede's latest patch series you've just merged in for-5.2/logitech, it
> > > > is much better but there is still some issues.
> > > >
> > > > The first one is the device index, I need to use device index 0
> > > > instead 0xff. I added a quick and dirty quirk (stealing in the
> > > > QUIRK_CLASS range since the normal quirk range looks full) to change
> > > > the device index assigned in __hidpp_send_report. After that the
> > > > device is correctly initialized and the wheel multiplier is set.
> > >
> > > Hmm, maybe we should restrain a little bit the reserved quirks...
> > > But actually, .driver_data and .quirks are both unsigned long, so you
> > > should be able to use the 64 bits.
> >
> > Only on 64 bits architectures, or is the kernel forcing long integers
> > to be 64 bits everywhere?
>
> Damnit, you are correct, there is no such enforcement :/
>
> >
> > >
> > > >
> > > > The second issue is that wheel values are not actually scaled
> > > > according to the multiplier. I get 7/8 full scroll event for each
> > > > wheel step. I think it happens because the mouse is split in two
> > > > devices. The first device has the wheel events, and the second device
> > > > has the HID++ reports. The wheel multiplier is only set on the second
> > > > device (where the hi-res mode is enabled) and does not affect the
> > > > wheel events from the first one.
> > >
> > > I would think this have to do with the device not accepting the
> > > command instead. Can you share some raw logs of the events (ideally
> > > with hid-recorder from
> > > https://gitlab.freedesktop.org/libevdev/hid-tools)?
> >
> > I already checked with usbmon and double-checked by querying the
> > register. The flag is set and accepted by the device and it behaves
> > accordingly: it sends about 8 wheel events per step.
>
> OK, that's what I wanted to see.
>
> >
> > hid-recorder takes hidraw nodes as parameters, how do I use it to
> > record the initialization by the driver?
>
> You can't. But it doesn't really matter here because I wanted to check
> what your mouse is actually sending after the initialization.
>
> So if I read correctly: the mouse is sending high res data but
> evemu-recorder shows one REL_WHEEL event every 7/8 clicks?

It is HID++ 1.0, there is no special high res data report, it sends
standard HID mouse wheel report, but more of them.

To be clear, here is a recording using the modified kernel. I moved
the wheel one step up (8 events are generated), then one step down (8
events again + a 2-event bump).

# EVEMU 1.3
# Kernel: 5.0.0logitech+
# DMI: dmi:bvnAmericanMegatrendsInc.:bvr1.40:bd01/17/2019:svnMicro-StarInternationalCo.,Ltd.:pnMS-7B98:pvr1.0:rvnMicro-StarInternationalCo.,Ltd.:rnZ390-APRO(MS-7B98):rvr1.0:cvnMicro-StarInternationalCo.,Ltd.:ct3:cvr1.0:
# Input device name: "Logitech G500"
# Input device ID: bus 0x03 vendor 0x46d product 0xc068 version 0x111
# Supported events:
# Event type 0 (EV_SYN)
# Event code 0 (SYN_REPORT)
# Event code 1 (SYN_CONFIG)
# Event code 2 (SYN_MT_REPORT)
# Event code 3 (SYN_DROPPED)
# Event code 4 ((null))
# Event code 5 ((null))
# Event code 6 ((null))
# Event code 7 ((null))
# Event code 8 ((null))
# Event code 9 ((null))
# Event code 10 ((null))
# Event code 11 ((null))
# Event code 12 ((null))
# Event code 13 ((null))
# Event code 14 ((null))
# Event code 15 (SYN_MAX)
# Event type 1 (EV_KEY)
# Event code 272 (BTN_LEFT)
# Event code 273 (BTN_RIGHT)
# Event code 274 (BTN_MIDDLE)
# Event code 275 (BTN_SIDE)
# Event code 276 (BTN_EXTRA)
# Event code 277 (BTN_FORWARD)
# Event code 278 (BTN_BACK)
# Event code 279 (BTN_TASK)
# Event code 280 ((null))
# Event code 281 ((null))
# Event code 282 ((null))
# Event code 283 ((null))
# Event code 284 ((null))
# Event code 285 ((null))
# Event code 286 ((null))
# Event code 287 ((null))
# Event type 2 (EV_REL)
# Event code 0 (REL_X)
# Event code 1 (REL_Y)
# Event code 6 (REL_HWHEEL)
# Event code 8 (REL_WHEEL)
# Event code 11 ((null))
# Event code 12 ((null))
# Event type 4 (EV_MSC)
# Event code 4 (MSC_SCAN)
# Properties:
N: Logitech G500
I: 0003 046d c068 0111
P: 00 00 00 00 00 00 00 00
B: 00 0b 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 ff ff 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 01 00 00 00 00 00 00 00 00
B: 02 43 19 00 00 00 00 00 00
B: 03 00 00 00 00 00 00 00 00
B: 04 10 00 00 00 00 00 00 00
B: 05 00 00 00 00 00 00 00 00
B: 11 00 00 00 00 00 00 00 00
B: 12 00 00 00 00 00 00 00 00
B: 14 00 00 00 00 00 00 00 00
B: 15 00 00 00 00 00 00 00 00
B: 15 00 00 00 00 00 00 00 00
################################
# Waiting for events #
################################
E: 0.000001 0002 0008 0001 # EV_REL / REL_WHEEL 1
E: 0.000001 0002 000b 0120 # EV_REL / (null) 120
E: 0.000001 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +0ms
E: 0.042009 0002 0008 0001 # EV_REL / REL_WHEEL 1
E: 0.042009 0002 000b 0120 # EV_REL / (null) 120
E: 0.042009 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +42ms
E: 0.075016 0002 0008 0001 # EV_REL / REL_WHEEL 1
E: 0.075016 0002 000b 0120 # EV_REL / (null) 120
E: 0.075016 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +33ms
E: 0.107977 0002 0008 0001 # EV_REL / REL_WHEEL 1
E: 0.107977 0002 000b 0120 # EV_REL / (null) 120
E: 0.107977 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +32ms
E: 0.124979 0002 0008 0001 # EV_REL / REL_WHEEL 1
E: 0.124979 0002 000b 0120 # EV_REL / (null) 120
E: 0.124979 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +17ms
E: 0.141950 0002 0008 0001 # EV_REL / REL_WHEEL 1
E: 0.141950 0002 000b 0120 # EV_REL / (null) 120
E: 0.141950 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +17ms
E: 0.152950 0002 0008 0001 # EV_REL / REL_WHEEL 1
E: 0.152950 0002 000b 0120 # EV_REL / (null) 120
E: 0.152950 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +11ms
E: 0.170943 0002 0008 0001 # EV_REL / REL_WHEEL 1
E: 0.170943 0002 000b 0120 # EV_REL / (null) 120
E: 0.170943 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +18ms
E: 1.452035 0002 0008 -001 # EV_REL / REL_WHEEL -1
E: 1.452035 0002 000b -120 # EV_REL / (null) -120
E: 1.452035 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +1282ms
E: 1.535031 0002 0008 -001 # EV_REL / REL_WHEEL -1
E: 1.535031 0002 000b -120 # EV_REL / (null) -120
E: 1.535031 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +83ms
E: 1.574981 0002 0008 -001 # EV_REL / REL_WHEEL -1
E: 1.574981 0002 000b -120 # EV_REL / (null) -120
E: 1.574981 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +39ms
E: 1.702039 0002 0008 -001 # EV_REL / REL_WHEEL -1
E: 1.702039 0002 000b -120 # EV_REL / (null) -120
E: 1.702039 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +128ms
E: 1.713031 0002 0008 -001 # EV_REL / REL_WHEEL -1
E: 1.713031 0002 000b -120 # EV_REL / (null) -120
E: 1.713031 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +11ms
E: 1.724036 0002 0008 -001 # EV_REL / REL_WHEEL -1
E: 1.724036 0002 000b -120 # EV_REL / (null) -120
E: 1.724036 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +11ms
E: 1.731006 0002 0008 -001 # EV_REL / REL_WHEEL -1
E: 1.731006 0002 000b -120 # EV_REL / (null) -120
E: 1.731006 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +7ms
E: 1.740043 0002 0008 -001 # EV_REL / REL_WHEEL -1
E: 1.740043 0002 000b -120 # EV_REL / (null) -120
E: 1.740043 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +9ms
E: 1.765013 0002 0008 -001 # EV_REL / REL_WHEEL -1
E: 1.765013 0002 000b -120 # EV_REL / (null) -120
E: 1.765013 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +25ms
E: 1.975044 0002 0008 0001 # EV_REL / REL_WHEEL 1
E: 1.975044 0002 000b 0120 # EV_REL / (null) 120
E: 1.975044 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- +210ms

Here is the corresponding hid-recorder:

D: 0
R: 67 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 10 15 00 25 01 95
10 75 01 81 02 05 01 16 01 80 26 ff 7f 75 10 95 02 09 30 09 31 81 06
15 81 25 7f 75 08 95 01 09 38 81 06 05 0c 0a 38 02 95 01 81 06 c0 c0
N: Logitech G500
P: usb-0000:00:14.0-2/input0
I: 3 046d c068
D: 0
E: 0.000000 8 00 00 00 00 00 00 01 00
E: 0.041957 8 00 00 00 00 00 00 01 00
E: 0.074966 8 00 00 00 00 00 00 01 00
E: 0.107932 8 00 00 00 00 00 00 01 00
E: 0.124933 8 00 00 00 00 00 00 01 00
E: 0.141897 8 00 00 00 00 00 00 01 00
E: 0.152900 8 00 00 00 00 00 00 01 00
E: 0.170892 8 00 00 00 00 00 00 01 00
E: 1.452000 8 00 00 00 00 00 00 ff 00
E: 1.535009 8 00 00 00 00 00 00 ff 00
E: 1.574928 8 00 00 00 00 00 00 ff 00
E: 1.702009 8 00 00 00 00 00 00 ff 00
E: 1.713009 8 00 00 00 00 00 00 ff 00
E: 1.724001 8 00 00 00 00 00 00 ff 00
E: 1.730963 8 00 00 00 00 00 00 ff 00
E: 1.740005 8 00 00 00 00 00 00 ff 00
E: 1.764977 8 00 00 00 00 00 00 ff 00
E: 1.975014 8 00 00 00 00 00 00 01 00

The hi-res events should +/-15 instead of +/-120, and less REL_WHEEL
events should be generated. This looks like the multiplier is set to 1
instead of 8.

kernel log shows the multiplier is set but only for one of the two devices:

input: Logitech G500 as
/devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.0/0003:046D:C068.0006/input/input25
hid-generic 0003:046D:C068.0006: input,hidraw5: USB HID v1.11 Mouse
[Logitech G500] on usb-0000:00:14.0-2/input0
input: Logitech G500 Keyboard as
/devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/0003:046D:C068.0007/input/input26
input: Logitech G500 Consumer Control as
/devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/0003:046D:C068.0007/input/input27
hid-generic 0003:046D:C068.0007: input,hiddev97,hidraw6: USB HID v1.11
Keyboard [Logitech G500] on usb-0000:00:14.0-2/input1
input: Logitech G500 as
/devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.0/0003:046D:C068.0006/input/input30
logitech-hidpp-device 0003:046D:C068.0006: input,hidraw5: USB HID
v1.11 Mouse [Logitech G500] on usb-0000:00:14.0-2/input0
logitech-hidpp-device 0003:046D:C068.0007: HID++ 1.0 device connected.
logitech-hidpp-device 0003:046D:C068.0007: multiplier = 8
input: Logitech G500 as
/devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/0003:046D:C068.0007/input/input31
logitech-hidpp-device 0003:046D:C068.0007: input,hiddev97,hidraw6: USB
HID v1.11 Keyboard [Logitech G500] on usb-0000:00:14.0-2/input1

>
> Cheers,
> Benjamin
>
> > > > Le mer. 19 déc. 2018 à 21:35, Benjamin Tissoires
> > > > <[email protected]> a écrit :
> > > > >
> > > > > On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> > > > > > <[email protected]> a écrit :
> > > > > > >
> > > > > > > Le ven. 14 déc. 2018 à 19:37, Harry Cutts <[email protected]> a écrit :
> > > > > > > >
> > > > > > > > Hi Clement,
> > > > > > > >
> > > > > > > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > > > > > > <[email protected]> wrote:
> > > > > > > > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > > > > > > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > > > > > > > "click", this is what this driver expects, right? If I understood
> > > > > > > > > correctly, I should try this patch with the
> > > > > > > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > > > > > > >
> > > > > > > > Thanks for the info! Yes, that should work.
> > > > > > >
> > > > > > > Well, it is not that simple. I get "Device not connected" errors for
> > > > > > > both interfaces of the mouse.
> > > > > >
> > > > > > I suspect the device is not responding because the hid device is not
> > > > > > started. When is hid_hw_start supposed to be called? It is called
> > > > > > early for HID_QUIRK_CLASS_G920 but later for other device. So the
> > > > > > device is not started when hidpp_is_connected is called. Is this
> > > > > > because most of the device in this driver are not real HID devices but
> > > > > > DJ devices? How should non-DJ devices be treated?
> > > > >
> > > > > Hi Clement,
> > > > >
> > > > > I have a series I sent last September that allows to support non DJ
> > > > > devices on logitech-hidpp
> > > > > (https://patchwork.kernel.org/project/linux-input/list/?series=16359).
> > > > >
> > > > > In its current form, with the latest upstream kernel, the series will
> > > > > oops during the .event() callback, which is easy enough to fix.
> > > > > However, I am currently trying to make it better as a second or third
> > > > > reading made me realized that there was a bunch of non-sense in it and
> > > > > a proper support would require slightly more work for the non unifying
> > > > > receiver case.
> > > > >
> > > > > I hope I'll be able to send out something by the end of the week.
> > > > >
> > > > > Cheers,
> > > > > Benjamin

2019-04-25 09:38:09

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

On Thu, Apr 25, 2019 at 11:29 AM Clément VUCHENER
<[email protected]> wrote:
>
> Le jeu. 25 avr. 2019 à 10:45, Benjamin Tissoires
> <[email protected]> a écrit :
> >
> > On Thu, Apr 25, 2019 at 10:25 AM Clément VUCHENER
> > <[email protected]> wrote:
> > >
> > > Le jeu. 25 avr. 2019 à 09:40, Benjamin Tissoires
> > > <[email protected]> a écrit :
> > > >
> > > > Hi Clément,
> > > >
> > > > On Wed, Apr 24, 2019 at 5:34 PM Clément VUCHENER
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Benjamin,
> > > > >
> > > > > I tried again to add hi-res wheel support for the G500 with Hans de
> > > > > Goede's latest patch series you've just merged in for-5.2/logitech, it
> > > > > is much better but there is still some issues.
> > > > >
> > > > > The first one is the device index, I need to use device index 0
> > > > > instead 0xff. I added a quick and dirty quirk (stealing in the
> > > > > QUIRK_CLASS range since the normal quirk range looks full) to change
> > > > > the device index assigned in __hidpp_send_report. After that the
> > > > > device is correctly initialized and the wheel multiplier is set.
> > > >
> > > > Hmm, maybe we should restrain a little bit the reserved quirks...
> > > > But actually, .driver_data and .quirks are both unsigned long, so you
> > > > should be able to use the 64 bits.
> > >
> > > Only on 64 bits architectures, or is the kernel forcing long integers
> > > to be 64 bits everywhere?
> >
> > Damnit, you are correct, there is no such enforcement :/
> >
> > >
> > > >
> > > > >
> > > > > The second issue is that wheel values are not actually scaled
> > > > > according to the multiplier. I get 7/8 full scroll event for each
> > > > > wheel step. I think it happens because the mouse is split in two
> > > > > devices. The first device has the wheel events, and the second device
> > > > > has the HID++ reports. The wheel multiplier is only set on the second
> > > > > device (where the hi-res mode is enabled) and does not affect the
> > > > > wheel events from the first one.
> > > >
> > > > I would think this have to do with the device not accepting the
> > > > command instead. Can you share some raw logs of the events (ideally
> > > > with hid-recorder from
> > > > https://gitlab.freedesktop.org/libevdev/hid-tools)?
> > >
> > > I already checked with usbmon and double-checked by querying the
> > > register. The flag is set and accepted by the device and it behaves
> > > accordingly: it sends about 8 wheel events per step.
> >
> > OK, that's what I wanted to see.
> >
> > >
> > > hid-recorder takes hidraw nodes as parameters, how do I use it to
> > > record the initialization by the driver?
> >
> > You can't. But it doesn't really matter here because I wanted to check
> > what your mouse is actually sending after the initialization.
> >
> > So if I read correctly: the mouse is sending high res data but
> > evemu-recorder shows one REL_WHEEL event every 7/8 clicks?
>
> It is HID++ 1.0, there is no special high res data report, it sends
> standard HID mouse wheel report, but more of them.
>
> To be clear, here is a recording using the modified kernel. I moved
> the wheel one step up (8 events are generated), then one step down (8
> events again + a 2-event bump).
>
> # EVEMU 1.3
[snipped]
> E: 1.975014 8 00 00 00 00 00 00 01 00
>
> The hi-res events should +/-15 instead of +/-120, and less REL_WHEEL
> events should be generated. This looks like the multiplier is set to 1
> instead of 8.
>
> kernel log shows the multiplier is set but only for one of the two devices:
>
> input: Logitech G500 as
> /devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.0/0003:046D:C068.0006/input/input25
> hid-generic 0003:046D:C068.0006: input,hidraw5: USB HID v1.11 Mouse
> [Logitech G500] on usb-0000:00:14.0-2/input0
> input: Logitech G500 Keyboard as
> /devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/0003:046D:C068.0007/input/input26
> input: Logitech G500 Consumer Control as
> /devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/0003:046D:C068.0007/input/input27
> hid-generic 0003:046D:C068.0007: input,hiddev97,hidraw6: USB HID v1.11
> Keyboard [Logitech G500] on usb-0000:00:14.0-2/input1
> input: Logitech G500 as
> /devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.0/0003:046D:C068.0006/input/input30
> logitech-hidpp-device 0003:046D:C068.0006: input,hidraw5: USB HID
> v1.11 Mouse [Logitech G500] on usb-0000:00:14.0-2/input0
> logitech-hidpp-device 0003:046D:C068.0007: HID++ 1.0 device connected.
> logitech-hidpp-device 0003:046D:C068.0007: multiplier = 8
> input: Logitech G500 as
> /devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/0003:046D:C068.0007/input/input31
> logitech-hidpp-device 0003:046D:C068.0007: input,hiddev97,hidraw6: USB
> HID v1.11 Keyboard [Logitech G500] on usb-0000:00:14.0-2/input1
>

Oh, now I see. And yes you are correct.

I wonder if we should not consider the mouse in hid-logitech-dj then.
And let hid-logitech-dj merge the 2 nodes together into one hidpp
device, and so we are facing a "regular" HID++ device which is
consistent.

Unfortunately, I am not sure I'll have the time to work on it this week :/

Cheers,
Benjamin

> >
> > > > > Le mer. 19 déc. 2018 à 21:35, Benjamin Tissoires
> > > > > <[email protected]> a écrit :
> > > > > >
> > > > > > On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> > > > > > > <[email protected]> a écrit :
> > > > > > > >
> > > > > > > > Le ven. 14 déc. 2018 à 19:37, Harry Cutts <[email protected]> a écrit :
> > > > > > > > >
> > > > > > > > > Hi Clement,
> > > > > > > > >
> > > > > > > > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > > > > > > > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > > > > > > > > "click", this is what this driver expects, right? If I understood
> > > > > > > > > > correctly, I should try this patch with the
> > > > > > > > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > > > > > > > >
> > > > > > > > > Thanks for the info! Yes, that should work.
> > > > > > > >
> > > > > > > > Well, it is not that simple. I get "Device not connected" errors for
> > > > > > > > both interfaces of the mouse.
> > > > > > >
> > > > > > > I suspect the device is not responding because the hid device is not
> > > > > > > started. When is hid_hw_start supposed to be called? It is called
> > > > > > > early for HID_QUIRK_CLASS_G920 but later for other device. So the
> > > > > > > device is not started when hidpp_is_connected is called. Is this
> > > > > > > because most of the device in this driver are not real HID devices but
> > > > > > > DJ devices? How should non-DJ devices be treated?
> > > > > >
> > > > > > Hi Clement,
> > > > > >
> > > > > > I have a series I sent last September that allows to support non DJ
> > > > > > devices on logitech-hidpp
> > > > > > (https://patchwork.kernel.org/project/linux-input/list/?series=16359).
> > > > > >
> > > > > > In its current form, with the latest upstream kernel, the series will
> > > > > > oops during the .event() callback, which is easy enough to fix.
> > > > > > However, I am currently trying to make it better as a second or third
> > > > > > reading made me realized that there was a bunch of non-sense in it and
> > > > > > a proper support would require slightly more work for the non unifying
> > > > > > receiver case.
> > > > > >
> > > > > > I hope I'll be able to send out something by the end of the week.
> > > > > >
> > > > > > Cheers,
> > > > > > Benjamin

2019-04-25 12:44:54

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

Hi Clément,

On Wed, Apr 24, 2019 at 5:34 PM Clément VUCHENER
<[email protected]> wrote:
>
> Hi Benjamin,
>
> I tried again to add hi-res wheel support for the G500 with Hans de
> Goede's latest patch series you've just merged in for-5.2/logitech, it
> is much better but there is still some issues.
>
> The first one is the device index, I need to use device index 0
> instead 0xff. I added a quick and dirty quirk (stealing in the
> QUIRK_CLASS range since the normal quirk range looks full) to change
> the device index assigned in __hidpp_send_report. After that the
> device is correctly initialized and the wheel multiplier is set.

Hmm, maybe we should restrain a little bit the reserved quirks...
But actually, .driver_data and .quirks are both unsigned long, so you
should be able to use the 64 bits.

>
> The second issue is that wheel values are not actually scaled
> according to the multiplier. I get 7/8 full scroll event for each
> wheel step. I think it happens because the mouse is split in two
> devices. The first device has the wheel events, and the second device
> has the HID++ reports. The wheel multiplier is only set on the second
> device (where the hi-res mode is enabled) and does not affect the
> wheel events from the first one.

I would think this have to do with the device not accepting the
command instead. Can you share some raw logs of the events (ideally
with hid-recorder from
https://gitlab.freedesktop.org/libevdev/hid-tools)?

Cheers,
Benjamin

>
> Le mer. 19 déc. 2018 à 21:35, Benjamin Tissoires
> <[email protected]> a écrit :
> >
> > On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
> > <[email protected]> wrote:
> > >
> > > Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> > > <[email protected]> a écrit :
> > > >
> > > > Le ven. 14 déc. 2018 à 19:37, Harry Cutts <[email protected]> a écrit :
> > > > >
> > > > > Hi Clement,
> > > > >
> > > > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > > > <[email protected]> wrote:
> > > > > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > > > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > > > > "click", this is what this driver expects, right? If I understood
> > > > > > correctly, I should try this patch with the
> > > > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > > > >
> > > > > Thanks for the info! Yes, that should work.
> > > >
> > > > Well, it is not that simple. I get "Device not connected" errors for
> > > > both interfaces of the mouse.
> > >
> > > I suspect the device is not responding because the hid device is not
> > > started. When is hid_hw_start supposed to be called? It is called
> > > early for HID_QUIRK_CLASS_G920 but later for other device. So the
> > > device is not started when hidpp_is_connected is called. Is this
> > > because most of the device in this driver are not real HID devices but
> > > DJ devices? How should non-DJ devices be treated?
> >
> > Hi Clement,
> >
> > I have a series I sent last September that allows to support non DJ
> > devices on logitech-hidpp
> > (https://patchwork.kernel.org/project/linux-input/list/?series=16359).
> >
> > In its current form, with the latest upstream kernel, the series will
> > oops during the .event() callback, which is easy enough to fix.
> > However, I am currently trying to make it better as a second or third
> > reading made me realized that there was a bunch of non-sense in it and
> > a proper support would require slightly more work for the non unifying
> > receiver case.
> >
> > I hope I'll be able to send out something by the end of the week.
> >
> > Cheers,
> > Benjamin

2019-04-25 12:45:39

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice

On Thu, Apr 25, 2019 at 10:25 AM Clément VUCHENER
<[email protected]> wrote:
>
> Le jeu. 25 avr. 2019 à 09:40, Benjamin Tissoires
> <[email protected]> a écrit :
> >
> > Hi Clément,
> >
> > On Wed, Apr 24, 2019 at 5:34 PM Clément VUCHENER
> > <[email protected]> wrote:
> > >
> > > Hi Benjamin,
> > >
> > > I tried again to add hi-res wheel support for the G500 with Hans de
> > > Goede's latest patch series you've just merged in for-5.2/logitech, it
> > > is much better but there is still some issues.
> > >
> > > The first one is the device index, I need to use device index 0
> > > instead 0xff. I added a quick and dirty quirk (stealing in the
> > > QUIRK_CLASS range since the normal quirk range looks full) to change
> > > the device index assigned in __hidpp_send_report. After that the
> > > device is correctly initialized and the wheel multiplier is set.
> >
> > Hmm, maybe we should restrain a little bit the reserved quirks...
> > But actually, .driver_data and .quirks are both unsigned long, so you
> > should be able to use the 64 bits.
>
> Only on 64 bits architectures, or is the kernel forcing long integers
> to be 64 bits everywhere?

Damnit, you are correct, there is no such enforcement :/

>
> >
> > >
> > > The second issue is that wheel values are not actually scaled
> > > according to the multiplier. I get 7/8 full scroll event for each
> > > wheel step. I think it happens because the mouse is split in two
> > > devices. The first device has the wheel events, and the second device
> > > has the HID++ reports. The wheel multiplier is only set on the second
> > > device (where the hi-res mode is enabled) and does not affect the
> > > wheel events from the first one.
> >
> > I would think this have to do with the device not accepting the
> > command instead. Can you share some raw logs of the events (ideally
> > with hid-recorder from
> > https://gitlab.freedesktop.org/libevdev/hid-tools)?
>
> I already checked with usbmon and double-checked by querying the
> register. The flag is set and accepted by the device and it behaves
> accordingly: it sends about 8 wheel events per step.

OK, that's what I wanted to see.

>
> hid-recorder takes hidraw nodes as parameters, how do I use it to
> record the initialization by the driver?

You can't. But it doesn't really matter here because I wanted to check
what your mouse is actually sending after the initialization.

So if I read correctly: the mouse is sending high res data but
evemu-recorder shows one REL_WHEEL event every 7/8 clicks?

Cheers,
Benjamin

> > > Le mer. 19 déc. 2018 à 21:35, Benjamin Tissoires
> > > <[email protected]> a écrit :
> > > >
> > > > On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
> > > > <[email protected]> wrote:
> > > > >
> > > > > Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> > > > > <[email protected]> a écrit :
> > > > > >
> > > > > > Le ven. 14 déc. 2018 à 19:37, Harry Cutts <[email protected]> a écrit :
> > > > > > >
> > > > > > > Hi Clement,
> > > > > > >
> > > > > > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > > > > > <[email protected]> wrote:
> > > > > > > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > > > > > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > > > > > > "click", this is what this driver expects, right? If I understood
> > > > > > > > correctly, I should try this patch with the
> > > > > > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > > > > > >
> > > > > > > Thanks for the info! Yes, that should work.
> > > > > >
> > > > > > Well, it is not that simple. I get "Device not connected" errors for
> > > > > > both interfaces of the mouse.
> > > > >
> > > > > I suspect the device is not responding because the hid device is not
> > > > > started. When is hid_hw_start supposed to be called? It is called
> > > > > early for HID_QUIRK_CLASS_G920 but later for other device. So the
> > > > > device is not started when hidpp_is_connected is called. Is this
> > > > > because most of the device in this driver are not real HID devices but
> > > > > DJ devices? How should non-DJ devices be treated?
> > > >
> > > > Hi Clement,
> > > >
> > > > I have a series I sent last September that allows to support non DJ
> > > > devices on logitech-hidpp
> > > > (https://patchwork.kernel.org/project/linux-input/list/?series=16359).
> > > >
> > > > In its current form, with the latest upstream kernel, the series will
> > > > oops during the .event() callback, which is easy enough to fix.
> > > > However, I am currently trying to make it better as a second or third
> > > > reading made me realized that there was a bunch of non-sense in it and
> > > > a proper support would require slightly more work for the non unifying
> > > > receiver case.
> > > >
> > > > I hope I'll be able to send out something by the end of the week.
> > > >
> > > > Cheers,
> > > > Benjamin

2019-04-25 14:01:31