Now that we're in 2022, and the majority of desktop environments can and
should support touchpad gestures through libinput, remove the legacy
module parameter that made it possible to use gestures implemented in
firmware.
This will eventually allow simplifying the driver's initialisation code.
This reverts commit 9188dbaed68a4b23dc96eba165265c08caa7dc2a.
---
drivers/hid/hid-logitech-hidpp.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 08ad19097e9e..7f9187201913 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -32,11 +32,6 @@ MODULE_LICENSE("GPL");
MODULE_AUTHOR("Benjamin Tissoires <[email protected]>");
MODULE_AUTHOR("Nestor Lopez Casado <[email protected]>");
-static bool disable_raw_mode;
-module_param(disable_raw_mode, bool, 0644);
-MODULE_PARM_DESC(disable_raw_mode,
- "Disable Raw mode reporting for touchpads and keep firmware gestures.");
-
static bool disable_tap_to_click;
module_param(disable_tap_to_click, bool, 0644);
MODULE_PARM_DESC(disable_tap_to_click,
@@ -4355,11 +4350,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hidpp_application_equals(hdev, HID_GD_KEYBOARD))
hidpp->quirks |= HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS;
- if (disable_raw_mode) {
- hidpp->quirks &= ~HIDPP_QUIRK_CLASS_WTP;
- hidpp->quirks &= ~HIDPP_QUIRK_NO_HIDINPUT;
- }
-
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
ret = wtp_allocate(hdev, id);
if (ret)
--
2.38.1
Don't stop and restart communication with the device unless we need to
modify the connect flags used because of a device quirk.
---
drivers/hid/hid-logitech-hidpp.c | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 7f9187201913..b4e4a8c79c75 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4310,6 +4310,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
bool connected;
unsigned int connect_mask = HID_CONNECT_DEFAULT;
struct hidpp_ff_private_data data;
+ bool will_restart = false;
/* report_fixup needs drvdata to be set before we call hid_parse */
hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL);
@@ -4360,6 +4361,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
+ if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
+ will_restart = true;
+
INIT_WORK(&hidpp->work, delayed_work_cb);
mutex_init(&hidpp->send_mutex);
init_waitqueue_head(&hidpp->wait);
@@ -4374,7 +4378,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
* Plain USB connections need to actually call start and open
* on the transport driver to allow incoming data.
*/
- ret = hid_hw_start(hdev, 0);
+ ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
if (ret) {
hid_err(hdev, "hw start failed\n");
goto hid_hw_start_fail;
@@ -4411,6 +4415,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hidpp->wireless_feature_index = 0;
else if (ret)
goto hid_hw_init_fail;
+ ret = 0;
}
if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
@@ -4425,19 +4430,21 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hidpp_connect_event(hidpp);
- /* Reset the HID node state */
- hid_device_io_stop(hdev);
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
+ if (will_restart) {
+ /* Reset the HID node state */
+ hid_device_io_stop(hdev);
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
- if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
- connect_mask &= ~HID_CONNECT_HIDINPUT;
+ if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
+ connect_mask &= ~HID_CONNECT_HIDINPUT;
- /* Now export the actual inputs and hidraw nodes to the world */
- ret = hid_hw_start(hdev, connect_mask);
- if (ret) {
- hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
- goto hid_hw_start_fail;
+ /* Now export the actual inputs and hidraw nodes to the world */
+ ret = hid_hw_start(hdev, connect_mask);
+ if (ret) {
+ hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
+ goto hid_hw_start_fail;
+ }
}
if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
--
2.38.1
On Tue, 2022-12-20 at 10:22 +0100, Bastien Nocera wrote:
> Now that we're in 2022, and the majority of desktop environments can
> and
> should support touchpad gestures through libinput, remove the legacy
> module parameter that made it possible to use gestures implemented in
> firmware.
>
> This will eventually allow simplifying the driver's initialisation
> code.
>
> This reverts commit 9188dbaed68a4b23dc96eba165265c08caa7dc2a.
Forgot the signed-off-by, resent as v2.
> ---
> drivers/hid/hid-logitech-hidpp.c | 10 ----------
> 1 file changed, 10 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> logitech-hidpp.c
> index 08ad19097e9e..7f9187201913 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -32,11 +32,6 @@ MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Benjamin Tissoires <[email protected]>");
> MODULE_AUTHOR("Nestor Lopez Casado <[email protected]>");
>
> -static bool disable_raw_mode;
> -module_param(disable_raw_mode, bool, 0644);
> -MODULE_PARM_DESC(disable_raw_mode,
> - "Disable Raw mode reporting for touchpads and keep firmware
> gestures.");
> -
> static bool disable_tap_to_click;
> module_param(disable_tap_to_click, bool, 0644);
> MODULE_PARM_DESC(disable_tap_to_click,
> @@ -4355,11 +4350,6 @@ static int hidpp_probe(struct hid_device
> *hdev, const struct hid_device_id *id)
> hidpp_application_equals(hdev, HID_GD_KEYBOARD))
> hidpp->quirks |=
> HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS;
>
> - if (disable_raw_mode) {
> - hidpp->quirks &= ~HIDPP_QUIRK_CLASS_WTP;
> - hidpp->quirks &= ~HIDPP_QUIRK_NO_HIDINPUT;
> - }
> -
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> ret = wtp_allocate(hdev, id);
> if (ret)
On Tue, 2022-12-20 at 10:22 +0100, Bastien Nocera wrote:
> Don't stop and restart communication with the device unless we need
> to
> modify the connect flags used because of a device quirk.
FIWW, Andreas Bergmeier told me off-list that this fixed their problem
with the Litra Glow not connecting properly.
Would be great to have reviews on this and my other HID++ patches.
Cheers
> ---
> drivers/hid/hid-logitech-hidpp.c | 31 +++++++++++++++++++-----------
> -
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> logitech-hidpp.c
> index 7f9187201913..b4e4a8c79c75 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -4310,6 +4310,7 @@ static int hidpp_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> bool connected;
> unsigned int connect_mask = HID_CONNECT_DEFAULT;
> struct hidpp_ff_private_data data;
> + bool will_restart = false;
>
> /* report_fixup needs drvdata to be set before we call
> hid_parse */
> hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL);
> @@ -4360,6 +4361,9 @@ static int hidpp_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> return ret;
> }
>
> + if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
> + will_restart = true;
> +
> INIT_WORK(&hidpp->work, delayed_work_cb);
> mutex_init(&hidpp->send_mutex);
> init_waitqueue_head(&hidpp->wait);
> @@ -4374,7 +4378,7 @@ static int hidpp_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> * Plain USB connections need to actually call start and open
> * on the transport driver to allow incoming data.
> */
> - ret = hid_hw_start(hdev, 0);
> + ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
> if (ret) {
> hid_err(hdev, "hw start failed\n");
> goto hid_hw_start_fail;
> @@ -4411,6 +4415,7 @@ static int hidpp_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> hidpp->wireless_feature_index = 0;
> else if (ret)
> goto hid_hw_init_fail;
> + ret = 0;
> }
>
> if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
> @@ -4425,19 +4430,21 @@ static int hidpp_probe(struct hid_device
> *hdev, const struct hid_device_id *id)
>
> hidpp_connect_event(hidpp);
>
> - /* Reset the HID node state */
> - hid_device_io_stop(hdev);
> - hid_hw_close(hdev);
> - hid_hw_stop(hdev);
> + if (will_restart) {
> + /* Reset the HID node state */
> + hid_device_io_stop(hdev);
> + hid_hw_close(hdev);
> + hid_hw_stop(hdev);
>
> - if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> - connect_mask &= ~HID_CONNECT_HIDINPUT;
> + if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> + connect_mask &= ~HID_CONNECT_HIDINPUT;
>
> - /* Now export the actual inputs and hidraw nodes to the world
> */
> - ret = hid_hw_start(hdev, connect_mask);
> - if (ret) {
> - hid_err(hdev, "%s:hid_hw_start returned error\n",
> __func__);
> - goto hid_hw_start_fail;
> + /* Now export the actual inputs and hidraw nodes to
> the world */
> + ret = hid_hw_start(hdev, connect_mask);
> + if (ret) {
> + hid_err(hdev, "%s:hid_hw_start returned
> error\n", __func__);
> + goto hid_hw_start_fail;
> + }
> }
>
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
On Tue, Jan 24, 2023 at 6:20 PM Bastien Nocera <[email protected]> wrote:
>
> On Tue, 2022-12-20 at 10:22 +0100, Bastien Nocera wrote:
> > Don't stop and restart communication with the device unless we need
> > to
> > modify the connect flags used because of a device quirk.
>
> FIWW, Andreas Bergmeier told me off-list that this fixed their problem
> with the Litra Glow not connecting properly.
>
> Would be great to have reviews on this and my other HID++ patches.
Sigh. I reviewed the patches just now (well, v2 at least), and thought
I better give a shot at it before merging, and it turns out that this
patch breaks the Unifying receivers.
Without it, each device presented to the user space has a proper name:
logitech-hidpp-device 0003:046D:4041.001C: input,hidraw15: USB HID
v1.11 Keyboard [Logitech MX Master] on usb-0000:01:00.0-4/input2:5
But with it, I get:
logitech-hidpp-device 0003:046D:4041.0024: input,hidraw8: USB HID
v1.11 Keyboard [Logitech Wireless Device PID:4041] on
usb-0000:00:14.0-8.2.4/input2:5
This is because we present the device to the userspace before being
able to fetch the name from the receiver.
I think we should make that connect/disconnect a special case of the
receivers too. Or maybe if the bus is not Bluetooth or USB, do the
disconnect/reconnect.
Cheers,
Benjamin
>
> Cheers
>
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 31 +++++++++++++++++++-----------
> > -
> > 1 file changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > logitech-hidpp.c
> > index 7f9187201913..b4e4a8c79c75 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -4310,6 +4310,7 @@ static int hidpp_probe(struct hid_device *hdev,
> > const struct hid_device_id *id)
> > bool connected;
> > unsigned int connect_mask = HID_CONNECT_DEFAULT;
> > struct hidpp_ff_private_data data;
> > + bool will_restart = false;
> >
> > /* report_fixup needs drvdata to be set before we call
> > hid_parse */
> > hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL);
> > @@ -4360,6 +4361,9 @@ static int hidpp_probe(struct hid_device *hdev,
> > const struct hid_device_id *id)
> > return ret;
> > }
> >
> > + if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
> > + will_restart = true;
> > +
> > INIT_WORK(&hidpp->work, delayed_work_cb);
> > mutex_init(&hidpp->send_mutex);
> > init_waitqueue_head(&hidpp->wait);
> > @@ -4374,7 +4378,7 @@ static int hidpp_probe(struct hid_device *hdev,
> > const struct hid_device_id *id)
> > * Plain USB connections need to actually call start and open
> > * on the transport driver to allow incoming data.
> > */
> > - ret = hid_hw_start(hdev, 0);
> > + ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
> > if (ret) {
> > hid_err(hdev, "hw start failed\n");
> > goto hid_hw_start_fail;
> > @@ -4411,6 +4415,7 @@ static int hidpp_probe(struct hid_device *hdev,
> > const struct hid_device_id *id)
> > hidpp->wireless_feature_index = 0;
> > else if (ret)
> > goto hid_hw_init_fail;
> > + ret = 0;
> > }
> >
> > if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
> > @@ -4425,19 +4430,21 @@ static int hidpp_probe(struct hid_device
> > *hdev, const struct hid_device_id *id)
> >
> > hidpp_connect_event(hidpp);
> >
> > - /* Reset the HID node state */
> > - hid_device_io_stop(hdev);
> > - hid_hw_close(hdev);
> > - hid_hw_stop(hdev);
> > + if (will_restart) {
> > + /* Reset the HID node state */
> > + hid_device_io_stop(hdev);
> > + hid_hw_close(hdev);
> > + hid_hw_stop(hdev);
> >
> > - if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> > - connect_mask &= ~HID_CONNECT_HIDINPUT;
> > + if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> > + connect_mask &= ~HID_CONNECT_HIDINPUT;
> >
> > - /* Now export the actual inputs and hidraw nodes to the world
> > */
> > - ret = hid_hw_start(hdev, connect_mask);
> > - if (ret) {
> > - hid_err(hdev, "%s:hid_hw_start returned error\n",
> > __func__);
> > - goto hid_hw_start_fail;
> > + /* Now export the actual inputs and hidraw nodes to
> > the world */
> > + ret = hid_hw_start(hdev, connect_mask);
> > + if (ret) {
> > + hid_err(hdev, "%s:hid_hw_start returned
> > error\n", __func__);
> > + goto hid_hw_start_fail;
> > + }
> > }
> >
> > if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
>
On Wed, 2023-01-25 at 11:18 +0100, Benjamin Tissoires wrote:
> On Tue, Jan 24, 2023 at 6:20 PM Bastien Nocera <[email protected]>
> wrote:
> >
> > On Tue, 2022-12-20 at 10:22 +0100, Bastien Nocera wrote:
> > > Don't stop and restart communication with the device unless we
> > > need
> > > to
> > > modify the connect flags used because of a device quirk.
> >
> > FIWW, Andreas Bergmeier told me off-list that this fixed their
> > problem
> > with the Litra Glow not connecting properly.
> >
> > Would be great to have reviews on this and my other HID++ patches.
>
> Sigh. I reviewed the patches just now (well, v2 at least), and
> thought
> I better give a shot at it before merging, and it turns out that this
> patch breaks the Unifying receivers.
>
> Without it, each device presented to the user space has a proper
> name:
>
> logitech-hidpp-device 0003:046D:4041.001C: input,hidraw15: USB HID
> v1.11 Keyboard [Logitech MX Master] on usb-0000:01:00.0-4/input2:5
>
> But with it, I get:
>
> logitech-hidpp-device 0003:046D:4041.0024: input,hidraw8: USB HID
> v1.11 Keyboard [Logitech Wireless Device PID:4041] on
> usb-0000:00:14.0-8.2.4/input2:5
>
> This is because we present the device to the userspace before being
> able to fetch the name from the receiver.
>
> I think we should make that connect/disconnect a special case of the
> receivers too. Or maybe if the bus is not Bluetooth or USB, do the
> disconnect/reconnect.
From what I can tell, this would mean restarting the connection in case
hidpp_unifying_init() did anything, right?
I'll test this out and update the patch.
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4547e9580101..e0c28257f598 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4392,8 +4392,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
/* Allow incoming packets */
hid_device_io_start(hdev);
- if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
- hidpp_unifying_init(hidpp);
+ if (hidpp->quirks & HIDPP_QUIRK_UNIFYING) {
+ if (hidpp_unifying_init(hidpp) == 0)
+ will_restart = true;
+ }
connected = hidpp_root_get_protocol_version(hidpp) == 0;
atomic_set(&hidpp->connected, connected);