Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560647 > unrolled thread

[PATCH 00/17] Report power supply from hid-logitech-dj and others

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/17] Report power supply from hid-logitech-dj and others Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 12/17] HID: logitech-hidpp: allow non HID++ devices to be handled by this module Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
      Re: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable Benjamin Tissoires <benjamin.tissoires@gmail.com> - 2017-01-18 10:30 +0100
        Re: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable Jiri Kosina <jikos@kernel.org> - 2017-01-19 12:00 +0100
          Re: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable Benjamin Tissoires <benjamin.tissoires@gmail.com> - 2017-01-19 12:20 +0100
    [PATCH 14/17] HID: logitech-hidpp: do not query the name through HID++ for 1.0 devices Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 06/17] HID: logitech-hidpp: create the battery for all types of HID++ devices Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 11/17] HID: logitech-hidpp: add a sysfs file to tell we support power_supply Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 03/17] HID: logitech-hidpp: make sure we only register one battery per device Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
      Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Bastien Nocera <hadess@hadess.net> - 2017-01-18 12:40 +0100
      Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Jiri Kosina <jikos@kernel.org> - 2017-01-20 14:50 +0100
        Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Jiri Kosina <jikos@kernel.org> - 2017-01-20 15:30 +0100
        Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-20 15:30 +0100
    [PATCH 15/17] HID: logitech-hidpp: rework probe path for unifying devices Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 16/17] HID: logitech-hidpp: report battery for the G700 over wireless Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 10/17] HID: logitech-hidpp: notify battery on connect Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 04/17] HID: logitech-hidpp: battery: remove overloads and provide ONLINE Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 16:00 +0100
    [PATCH 07/17] HID: logitech-hidpp: return an error if the feature is not present Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 16:00 +0100
    [PATCH 01/17] HID: logitech-dj: allow devices to request full pairing information Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 16:00 +0100
    [PATCH 05/17] HID: logitech-hidpp: forward device info in power_supply Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 16:00 +0100
    Re: [PATCH 00/17] Report power supply from hid-logitech-dj and  others Bastien Nocera <hadess@hadess.net> - 2017-01-23 15:40 +0100
      Re: [PATCH 00/17] Report power supply from hid-logitech-dj and others Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-23 16:30 +0100
      Re: [PATCH 00/17] Report power supply from hid-logitech-dj and  others Bastien Nocera <hadess@hadess.net> - 2017-01-24 18:20 +0100

Page 1 of 2  [1] 2  Next page →

#1560647 — [PATCH 00/17] Report power supply from hid-logitech-dj and others

Hey guys,

I tried to revive the in-kernel battery support for HID++ devices.
I was thinking of doing just a few patches, but in the end I had to do
cleanups and some more tweaks...

So, the final result is that now hid-logitech-hidpp should allow to
handle any HID++ device, no matter which connection it uses.
I was able to test it on some unifying devices, some USB and Bluetooth,
but I'd like to get the confirmation from Simon that I did not break
the G920.

Other than that, I implemented most features asked by Bastien during the
last round:
- have a sysfs file to indicate we are capable of power_supply
- use ONLINE capability (not sure if I mess something up or if Gnome handles
  it correctly)
- report product, serial and manufacturer
- report K750 battery info (not Lux, sorry)
- report HID++ 1.0 battery info

The interesting bit also is that now devices that behaves likes unifying
receivers can be handled with hid-logitech-hidpp (for the gaming mice, mostly),
which allows to have the real name and serial of the connected device, not just
"Logitech USB Receiver".

Nestor, Olivier, I can't include the G900 in the series. It seems the G900 is
using a different feature (0x1001?) for the battery. Could you sent me the
specs so I can had this one too?

Cheers,
Benjamin

Bastien Nocera (1):
  HID: logitech-hidpp: Add scope to battery

Benjamin Tissoires (16):
  HID: logitech-dj: allow devices to request full pairing information
  HID: logitech-hidpp: make sure we only register one battery per device
  HID: logitech-hidpp: battery: remove overloads and provide ONLINE
  HID: logitech-hidpp: forward device info in power_supply
  HID: logitech-hidpp: create the battery for all types of HID++ devices
  HID: logitech-hidpp: return an error if the feature is not present
  HID: logitech-hidpp: add support for battery status for the K750
  HID: logitech-hidpp: enable HID++ 1.0 battery reporting
  HID: logitech-hidpp: notify battery on connect
  HID: logitech-hidpp: add a sysfs file to tell we support power_supply
  HID: logitech-hidpp: allow non HID++ devices to be handled by this
    module
  HID: logitech-hidpp: make .probe usbhid capable
  HID: logitech-hidpp: do not query the name through HID++ for 1.0
    devices
  HID: logitech-hidpp: rework probe path for unifying devices
  HID: logitech-hidpp: report battery for the G700 over wireless
  HID: logitech-hidpp: retrieve the name of the gaming mice over
    wireless

 drivers/hid/hid-core.c           |   1 +
 drivers/hid/hid-ids.h            |   1 +
 drivers/hid/hid-logitech-dj.c    |  17 +-
 drivers/hid/hid-logitech-hidpp.c | 835 +++++++++++++++++++++++++++++++++------
 4 files changed, 736 insertions(+), 118 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1560648 — [PATCH 12/17] HID: logitech-hidpp: allow non HID++ devices to be handled by this module

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

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 033ef4c..a5d37a4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2380,6 +2380,9 @@ static int hidpp_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 
+	if (!hidpp)
+		return 0;
+
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
 		return wtp_input_mapping(hdev, hi, field, usage, bit, max);
 	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560 &&
@@ -2395,6 +2398,9 @@ static int hidpp_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 
+	if (!hidpp)
+		return 0;
+
 	/* Ensure that Logitech G920 is not given a default fuzz/flat value */
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
 		if (usage->type == EV_ABS && (usage->code == ABS_X ||
@@ -2423,6 +2429,9 @@ static int hidpp_input_configured(struct hid_device *hdev,
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	struct input_dev *input = hidinput->input;
 
+	if (!hidpp)
+		return 0;
+
 	hidpp_populate_input(hidpp, input, true);
 
 	return 0;
@@ -2476,6 +2485,9 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	int ret = 0;
 
+	if (!hidpp)
+		return 0;
+
 	/* Generic HID++ processing. */
 	switch (data[0]) {
 	case REPORT_ID_HIDPP_VERY_LONG:
@@ -2747,6 +2759,41 @@ static struct attribute_group ps_attribute_group = {
 	.attrs = sysfs_attrs
 };
 
+static bool hidpp_validate_report(struct hid_device *hdev, int id, int size,
+		bool optional)
+{
+	struct hid_report_enum *re;
+	struct hid_report *report;
+
+	if (id >= HID_MAX_IDS || id < 0) {
+		hid_err(hdev, "invalid HID report id %u\n", id);
+		return false;
+	}
+
+	re = &(hdev->report_enum[HID_OUTPUT_REPORT]);
+	report = re->report_id_hash[id];
+
+	if (!report)
+		return optional;
+
+	if (report->field[0]->report_count < size) {
+		hid_warn(hdev, "not enough values in hidpp report %d\n", id);
+		return false;
+	}
+
+	return true;
+}
+
+static bool hidpp_validate_device(struct hid_device *hdev)
+{
+	return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
+				     HIDPP_REPORT_SHORT_LENGTH - 1, false) &&
+	       hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
+				     HIDPP_REPORT_LONG_LENGTH - 1, true) &&
+	       hidpp_validate_report(hdev, REPORT_ID_HIDPP_VERY_LONG,
+				     HIDPP_REPORT_VERY_LONG_LENGTH - 1, true);
+}
+
 static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct hidpp_device *hidpp;
@@ -2754,6 +2801,18 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	bool connected;
 	unsigned int connect_mask = HID_CONNECT_DEFAULT;
 
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "%s:parse failed\n", __func__);
+		return ret;
+	}
+
+	/*
+	 * Make sure the device is HID++ capable, otherwise treat as generic HID
+	 */
+	if (!hidpp_validate_device(hdev))
+		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+
 	hidpp = devm_kzalloc(&hdev->dev, sizeof(struct hidpp_device),
 			GFP_KERNEL);
 	if (!hidpp)
@@ -2794,12 +2853,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hid_warn(hdev, "Cannot allocate sysfs group for %s\n",
 			 hdev->name);
 
-	ret = hid_parse(hdev);
-	if (ret) {
-		hid_err(hdev, "%s:parse failed\n", __func__);
-		goto hid_parse_fail;
-	}
-
 	if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
 		connect_mask &= ~HID_CONNECT_HIDINPUT;
 
@@ -2872,7 +2925,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hid_hw_stop(hdev);
 	}
 hid_hw_start_fail:
-hid_parse_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 	cancel_work_sync(&hidpp->work);
 	mutex_destroy(&hidpp->send_mutex);
@@ -2885,6 +2937,9 @@ static void hidpp_remove(struct hid_device *hdev)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 
+	if (!hidpp)
+		return hid_hw_stop(hdev);
+
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560649 — [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable

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

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

Tested with a G502 (plain USB), a T650 and many other unifying devices,
and the T651 over Bluetooth.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index a5d37a4..f5889ff 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2813,6 +2813,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (!hidpp_validate_device(hdev))
 		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 
+	/*
+	 * HID++ needs to read incoming report while in .probe().
+	 * However, this doesn't work for plain USB devices until the hdev
+	 * status is set with HID_STAT_ADDED (device fully registered once
+	 * with HID).
+	 * So we ask for it to be reprobed later.
+	 */
+	if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE &&
+	    !(hdev->status & HID_STAT_ADDED))
+		return -EPROBE_DEFER;
+
 	hidpp = devm_kzalloc(&hdev->dev, sizeof(struct hidpp_device),
 			GFP_KERNEL);
 	if (!hidpp)
@@ -2853,24 +2864,23 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hid_warn(hdev, "Cannot allocate sysfs group for %s\n",
 			 hdev->name);
 
-	if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
-		connect_mask &= ~HID_CONNECT_HIDINPUT;
-
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-		ret = hid_hw_start(hdev, connect_mask);
-		if (ret) {
-			hid_err(hdev, "hw start failed\n");
-			goto hid_hw_start_fail;
-		}
-		ret = hid_hw_open(hdev);
-		if (ret < 0) {
-			dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
-				__func__, ret);
-			hid_hw_stop(hdev);
-			goto hid_hw_start_fail;
-		}
+	/*
+	 * Plain USB connections need to actually call start and open
+	 * on the transport driver to allow incoming data.
+	 */
+	ret = hid_hw_start(hdev, 0);
+	if (ret) {
+		hid_err(hdev, "hw start failed\n");
+		goto hid_hw_start_fail;
 	}
 
+	ret = hid_hw_open(hdev);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
+			__func__, ret);
+		hid_hw_stop(hdev);
+		goto hid_hw_open_fail;
+	}
 
 	/* Allow incoming packets */
 	hid_device_io_start(hdev);
@@ -2880,7 +2890,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		if (!connected) {
 			ret = -ENODEV;
 			hid_err(hdev, "Device not connected");
-			goto hid_hw_open_failed;
+			goto hid_hw_init_fail;
 		}
 
 		hid_info(hdev, "HID++ %u.%u device connected.\n",
@@ -2893,37 +2903,36 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
 		ret = wtp_get_config(hidpp);
 		if (ret)
-			goto hid_hw_open_failed;
+			goto hid_hw_init_fail;
 	} else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
 		ret = g920_get_config(hidpp);
 		if (ret)
-			goto hid_hw_open_failed;
+			goto hid_hw_init_fail;
 	}
 
-	/* Block incoming packets */
-	hid_device_io_stop(hdev);
+	hidpp_connect_event(hidpp);
 
-	if (!(hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
-		ret = hid_hw_start(hdev, connect_mask);
-		if (ret) {
-			hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
-			goto hid_hw_start_fail;
-		}
-	}
+	/* Reset the HID node state */
+	hid_device_io_stop(hdev);
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
 
-	/* Allow incoming packets */
-	hid_device_io_start(hdev);
+	if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
+		connect_mask &= ~HID_CONNECT_HIDINPUT;
 
-	hidpp_connect_event(hidpp);
+	/* Now export the actual inputs and hidraw nodes to the world */
+	ret = hid_hw_start(hdev, connect_mask);
+	if (ret) {
+		hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
+		goto hid_hw_start_fail;
+	}
 
 	return ret;
 
-hid_hw_open_failed:
-	hid_device_io_stop(hdev);
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-		hid_hw_close(hdev);
-		hid_hw_stop(hdev);
-	}
+hid_hw_init_fail:
+	hid_hw_close(hdev);
+hid_hw_open_fail:
+	hid_hw_stop(hdev);
 hid_hw_start_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 	cancel_work_sync(&hidpp->work);
@@ -2942,10 +2951,9 @@ static void hidpp_remove(struct hid_device *hdev)
 
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
+	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
 		hidpp_ff_deinit(hdev);
-		hid_hw_close(hdev);
-	}
+
 	hid_hw_stop(hdev);
 	cancel_work_sync(&hidpp->work);
 	mutex_destroy(&hidpp->send_mutex);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1561383 — Re: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable

On Tue, Jan 17, 2017 at 3:35 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> The current custom solution for the G920 is not the best because
> hid_hw_start() is not called at the end of the .probe().
> It means that any configuration retrieved after the initial hid_hw_start
> would not be exposed to user space without races.
>
> We can simply force hid_hw_start to just enable the transport layer by
> not using a connect_mask. This way, we can have a common path between
> USB, Unifying and Bluetooth devices.
>
> Tested with a G502 (plain USB), a T650 and many other unifying devices,
> and the T651 over Bluetooth.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-logitech-hidpp.c | 88 ++++++++++++++++++++++------------------
>  1 file changed, 48 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index a5d37a4..f5889ff 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -2813,6 +2813,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         if (!hidpp_validate_device(hdev))
>                 return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>
> +       /*
> +        * HID++ needs to read incoming report while in .probe().
> +        * However, this doesn't work for plain USB devices until the hdev
> +        * status is set with HID_STAT_ADDED (device fully registered once
> +        * with HID).
> +        * So we ask for it to be reprobed later.
> +        */
> +       if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE &&
> +           !(hdev->status & HID_STAT_ADDED))

Looks like this test breaks the T651 (bluetooth) after all. I seem to
have better success with:
    if (id->bus == BUS_USB && !(hdev->status & HID_STAT_ADDED))

But that also means that the solution will not work if there is only
one USB interface in the device :/

Cheers,
Benjamin

> +               return -EPROBE_DEFER;
> +
>         hidpp = devm_kzalloc(&hdev->dev, sizeof(struct hidpp_device),
>                         GFP_KERNEL);
>         if (!hidpp)
> @@ -2853,24 +2864,23 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 hid_warn(hdev, "Cannot allocate sysfs group for %s\n",
>                          hdev->name);
>
> -       if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> -               connect_mask &= ~HID_CONNECT_HIDINPUT;
> -
> -       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
> -               ret = hid_hw_start(hdev, connect_mask);
> -               if (ret) {
> -                       hid_err(hdev, "hw start failed\n");
> -                       goto hid_hw_start_fail;
> -               }
> -               ret = hid_hw_open(hdev);
> -               if (ret < 0) {
> -                       dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
> -                               __func__, ret);
> -                       hid_hw_stop(hdev);
> -                       goto hid_hw_start_fail;
> -               }
> +       /*
> +        * Plain USB connections need to actually call start and open
> +        * on the transport driver to allow incoming data.
> +        */
> +       ret = hid_hw_start(hdev, 0);
> +       if (ret) {
> +               hid_err(hdev, "hw start failed\n");
> +               goto hid_hw_start_fail;
>         }
>
> +       ret = hid_hw_open(hdev);
> +       if (ret < 0) {
> +               dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
> +                       __func__, ret);
> +               hid_hw_stop(hdev);
> +               goto hid_hw_open_fail;
> +       }
>
>         /* Allow incoming packets */
>         hid_device_io_start(hdev);
> @@ -2880,7 +2890,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 if (!connected) {
>                         ret = -ENODEV;
>                         hid_err(hdev, "Device not connected");
> -                       goto hid_hw_open_failed;
> +                       goto hid_hw_init_fail;
>                 }
>
>                 hid_info(hdev, "HID++ %u.%u device connected.\n",
> @@ -2893,37 +2903,36 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
>                 ret = wtp_get_config(hidpp);
>                 if (ret)
> -                       goto hid_hw_open_failed;
> +                       goto hid_hw_init_fail;
>         } else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
>                 ret = g920_get_config(hidpp);
>                 if (ret)
> -                       goto hid_hw_open_failed;
> +                       goto hid_hw_init_fail;
>         }
>
> -       /* Block incoming packets */
> -       hid_device_io_stop(hdev);
> +       hidpp_connect_event(hidpp);
>
> -       if (!(hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
> -               ret = hid_hw_start(hdev, connect_mask);
> -               if (ret) {
> -                       hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
> -                       goto hid_hw_start_fail;
> -               }
> -       }
> +       /* Reset the HID node state */
> +       hid_device_io_stop(hdev);
> +       hid_hw_close(hdev);
> +       hid_hw_stop(hdev);
>
> -       /* Allow incoming packets */
> -       hid_device_io_start(hdev);
> +       if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> +               connect_mask &= ~HID_CONNECT_HIDINPUT;
>
> -       hidpp_connect_event(hidpp);
> +       /* Now export the actual inputs and hidraw nodes to the world */
> +       ret = hid_hw_start(hdev, connect_mask);
> +       if (ret) {
> +               hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
> +               goto hid_hw_start_fail;
> +       }
>
>         return ret;
>
> -hid_hw_open_failed:
> -       hid_device_io_stop(hdev);
> -       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
> -               hid_hw_close(hdev);
> -               hid_hw_stop(hdev);
> -       }
> +hid_hw_init_fail:
> +       hid_hw_close(hdev);
> +hid_hw_open_fail:
> +       hid_hw_stop(hdev);
>  hid_hw_start_fail:
>         sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
>         cancel_work_sync(&hidpp->work);
> @@ -2942,10 +2951,9 @@ static void hidpp_remove(struct hid_device *hdev)
>
>         sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
>
> -       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
> +       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
>                 hidpp_ff_deinit(hdev);
> -               hid_hw_close(hdev);
> -       }
> +
>         hid_hw_stop(hdev);
>         cancel_work_sync(&hidpp->work);
>         mutex_destroy(&hidpp->send_mutex);
> --
> 2.9.3
>

[toc] | [prev] | [next] | [standalone]

#1562607 — Re: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable

On Wed, 18 Jan 2017, Benjamin Tissoires wrote:

> > The current custom solution for the G920 is not the best because
> > hid_hw_start() is not called at the end of the .probe().
> > It means that any configuration retrieved after the initial hid_hw_start
> > would not be exposed to user space without races.
> >
> > We can simply force hid_hw_start to just enable the transport layer by
> > not using a connect_mask. This way, we can have a common path between
> > USB, Unifying and Bluetooth devices.
> >
> > Tested with a G502 (plain USB), a T650 and many other unifying devices,
> > and the T651 over Bluetooth.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 88 ++++++++++++++++++++++------------------
> >  1 file changed, 48 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index a5d37a4..f5889ff 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -2813,6 +2813,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >         if (!hidpp_validate_device(hdev))
> >                 return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> >
> > +       /*
> > +        * HID++ needs to read incoming report while in .probe().
> > +        * However, this doesn't work for plain USB devices until the hdev
> > +        * status is set with HID_STAT_ADDED (device fully registered once
> > +        * with HID).
> > +        * So we ask for it to be reprobed later.
> > +        */
> > +       if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE &&
> > +           !(hdev->status & HID_STAT_ADDED))
> 
> Looks like this test breaks the T651 (bluetooth) after all. I seem to
> have better success with:
>     if (id->bus == BUS_USB && !(hdev->status & HID_STAT_ADDED))
> 
> But that also means that the solution will not work if there is only
> one USB interface in the device :/

Benjamin,

do you want at least a subset of this patchset to be queued before you 
figure this out, or should I put the whole thing on hold? (not gone 
through it fully yet).

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1562620 — Re: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable

On Thu, Jan 19, 2017 at 11:56 AM, Jiri Kosina <jikos@kernel.org> wrote:
> On Wed, 18 Jan 2017, Benjamin Tissoires wrote:
>
>> > The current custom solution for the G920 is not the best because
>> > hid_hw_start() is not called at the end of the .probe().
>> > It means that any configuration retrieved after the initial hid_hw_start
>> > would not be exposed to user space without races.
>> >
>> > We can simply force hid_hw_start to just enable the transport layer by
>> > not using a connect_mask. This way, we can have a common path between
>> > USB, Unifying and Bluetooth devices.
>> >
>> > Tested with a G502 (plain USB), a T650 and many other unifying devices,
>> > and the T651 over Bluetooth.
>> >
>> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> > ---
>> >  drivers/hid/hid-logitech-hidpp.c | 88 ++++++++++++++++++++++------------------
>> >  1 file changed, 48 insertions(+), 40 deletions(-)
>> >
>> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> > index a5d37a4..f5889ff 100644
>> > --- a/drivers/hid/hid-logitech-hidpp.c
>> > +++ b/drivers/hid/hid-logitech-hidpp.c
>> > @@ -2813,6 +2813,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> >         if (!hidpp_validate_device(hdev))
>> >                 return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> >
>> > +       /*
>> > +        * HID++ needs to read incoming report while in .probe().
>> > +        * However, this doesn't work for plain USB devices until the hdev
>> > +        * status is set with HID_STAT_ADDED (device fully registered once
>> > +        * with HID).
>> > +        * So we ask for it to be reprobed later.
>> > +        */
>> > +       if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE &&
>> > +           !(hdev->status & HID_STAT_ADDED))
>>
>> Looks like this test breaks the T651 (bluetooth) after all. I seem to
>> have better success with:
>>     if (id->bus == BUS_USB && !(hdev->status & HID_STAT_ADDED))
>>
>> But that also means that the solution will not work if there is only
>> one USB interface in the device :/
>
> Benjamin,
>
> do you want at least a subset of this patchset to be queued before you
> figure this out, or should I put the whole thing on hold? (not gone
> through it fully yet).
>

I think the first 11 patches (until "HID: logitech-hidpp: add a sysfs
file to tell we support power_supply" - included) should be good to go
while we get the test results and confirmation from others. The last
part of the series is really for non-unifying devices, but they are
not handled currently by upower, so it won't be a conflict to have
them now or later.

However, I'd like to get inputs from Bastien to see if that works for
him (I provided a test kenrel build for him to do the tests). I have
some doubts with the T650 as it appears in the main section of the
battery info instead of a separate device if you connect it while the
power panel is opened...

Cheers,
Benjamin

[toc] | [prev] | [next] | [standalone]

#1560650 — [PATCH 14/17] HID: logitech-hidpp: do not query the name through HID++ for 1.0 devices

Unless they are connected through unifying, they don't support it,
so remove one error in the logs.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index f5889ff..17dd569 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2612,6 +2612,8 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
 		 * Ask the receiver for its name.
 		 */
 		name = hidpp_get_unifying_name(hidpp);
+	else if (hidpp->protocol_major < 2)
+		return;
 	else
 		name = hidpp_get_device_name(hidpp);
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560652 — [PATCH 06/17] HID: logitech-hidpp: create the battery for all types of HID++ devices

The creation of the power_supply should not be in a HID++ 2.0 specific
function.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 77bfb65..0c7144f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -803,57 +803,6 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 	return ret;
 }
 
-static int hidpp20_initialize_battery(struct hidpp_device *hidpp)
-{
-	static atomic_t battery_no = ATOMIC_INIT(0);
-	struct power_supply_config cfg = { .drv_data = hidpp };
-	struct power_supply_desc *desc = &hidpp->battery.desc;
-	struct hidpp_battery *battery;
-	unsigned long n;
-	int ret;
-
-	ret = hidpp20_query_battery_info(hidpp);
-	if (ret)
-		return ret;
-
-	battery = &hidpp->battery;
-
-	n = atomic_inc_return(&battery_no) - 1;
-	desc->properties = hidpp_battery_props;
-	desc->num_properties = ARRAY_SIZE(hidpp_battery_props);
-	desc->get_property = hidpp_battery_get_property;
-	sprintf(battery->name, "hidpp_battery_%ld", n);
-	desc->name = battery->name;
-	desc->type = POWER_SUPPLY_TYPE_BATTERY;
-	desc->use_for_apm = 0;
-
-	battery->ps = devm_power_supply_register(&hidpp->hid_dev->dev,
-						 &battery->desc,
-						 &cfg);
-	if (IS_ERR(battery->ps))
-		return PTR_ERR(battery->ps);
-
-	power_supply_powers(battery->ps, &hidpp->hid_dev->dev);
-
-	return 0;
-}
-
-static int hidpp_initialize_battery(struct hidpp_device *hidpp)
-{
-	int ret;
-
-	if (hidpp->battery.ps)
-		return 0;
-
-	if (hidpp->protocol_major >= 2) {
-		ret = hidpp20_initialize_battery(hidpp);
-		if (ret == 0)
-			hidpp->quirks |= HIDPP_QUIRK_HIDPP20_BATTERY;
-	}
-
-	return ret;
-}
-
 /* -------------------------------------------------------------------------- */
 /* 0x6010: Touchpad FW items                                                  */
 /* -------------------------------------------------------------------------- */
@@ -2311,6 +2260,49 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 	return 0;
 }
 
+static int hidpp_initialize_battery(struct hidpp_device *hidpp)
+{
+	static atomic_t battery_no = ATOMIC_INIT(0);
+	struct power_supply_config cfg = { .drv_data = hidpp };
+	struct power_supply_desc *desc = &hidpp->battery.desc;
+	struct hidpp_battery *battery;
+	unsigned long n;
+	int ret;
+
+	if (hidpp->battery.ps)
+		return 0;
+
+	if (hidpp->protocol_major >= 2) {
+		ret = hidpp20_query_battery_info(hidpp);
+		if (ret)
+			return ret;
+		hidpp->quirks |= HIDPP_QUIRK_HIDPP20_BATTERY;
+	} else {
+		return -ENOENT;
+	}
+
+	battery = &hidpp->battery;
+
+	n = atomic_inc_return(&battery_no) - 1;
+	desc->properties = hidpp_battery_props;
+	desc->num_properties = ARRAY_SIZE(hidpp_battery_props);
+	desc->get_property = hidpp_battery_get_property;
+	sprintf(battery->name, "hidpp_battery_%ld", n);
+	desc->name = battery->name;
+	desc->type = POWER_SUPPLY_TYPE_BATTERY;
+	desc->use_for_apm = 0;
+
+	battery->ps = devm_power_supply_register(&hidpp->hid_dev->dev,
+						 &battery->desc,
+						 &cfg);
+	if (IS_ERR(battery->ps))
+		return PTR_ERR(battery->ps);
+
+	power_supply_powers(battery->ps, &hidpp->hid_dev->dev);
+
+	return ret;
+}
+
 static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560653 — [PATCH 11/17] HID: logitech-hidpp: add a sysfs file to tell we support power_supply

This way, upower can add a simple udev rule to decide whether or not
it should use the internal unifying support or just the generic kernel
one.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3ae2f41..033ef4c 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2736,6 +2736,17 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 	hidpp->delayed_input = input;
 }
 
+static DEVICE_ATTR(builtin_power_supply, 0000, NULL, NULL);
+
+static struct attribute *sysfs_attrs[] = {
+	&dev_attr_builtin_power_supply.attr,
+	NULL
+};
+
+static struct attribute_group ps_attribute_group = {
+	.attrs = sysfs_attrs
+};
+
 static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct hidpp_device *hidpp;
@@ -2777,6 +2788,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	mutex_init(&hidpp->send_mutex);
 	init_waitqueue_head(&hidpp->wait);
 
+	/* indicates we are handling the battery properties in the kernel */
+	ret = sysfs_create_group(&hdev->dev.kobj, &ps_attribute_group);
+	if (ret)
+		hid_warn(hdev, "Cannot allocate sysfs group for %s\n",
+			 hdev->name);
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "%s:parse failed\n", __func__);
@@ -2856,6 +2873,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 hid_hw_start_fail:
 hid_parse_fail:
+	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 	cancel_work_sync(&hidpp->work);
 	mutex_destroy(&hidpp->send_mutex);
 allocate_fail:
@@ -2867,6 +2885,8 @@ static void hidpp_remove(struct hid_device *hdev)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 
+	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
+
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
 		hidpp_ff_deinit(hdev);
 		hid_hw_close(hdev);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560655 — [PATCH 03/17] HID: logitech-hidpp: make sure we only register one battery per device

Simple check to add, huge improvement :)

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4aaf237..1cda29e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -828,6 +828,9 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 {
 	int ret;
 
+	if (hidpp->battery.ps)
+		return 0;
+
 	if (hidpp->protocol_major >= 2) {
 		ret = hidpp20_initialize_battery(hidpp);
 		if (ret == 0)
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560656 — [PATCH 02/17] HID: logitech-hidpp: Add scope to battery

From: Bastien Nocera <hadess@hadess.net>

Without a scope defined, UPower assumes that the battery is provide
power to the computer it's connected to, like a laptop battery or a UPS.

Tested-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Bastien Nocera <hadess@hadess.net>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4eeb550..4aaf237 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -761,6 +761,7 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
 static enum power_supply_property hidpp_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 static int hidpp_battery_get_property(struct power_supply *psy,
@@ -777,6 +778,9 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 		case POWER_SUPPLY_PROP_CAPACITY:
 			val->intval = hidpp->battery.level;
 			break;
+		case POWER_SUPPLY_PROP_SCOPE:
+			val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+			break;
 		default:
 			ret = -EINVAL;
 			break;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1561662 — Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery

On Tue, 2017-01-17 at 15:35 +0100, Benjamin Tissoires wrote:
> From: Bastien Nocera <hadess@hadess.net>
> 
> Without a scope defined, UPower assumes that the battery is provide
> power to the computer it's connected to

I wrote that? s/is provide/provides/

[toc] | [prev] | [next] | [standalone]

#1563591 — Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery

On Tue, 17 Jan 2017, Benjamin Tissoires wrote:

> From: Bastien Nocera <hadess@hadess.net>
> 
> Without a scope defined, UPower assumes that the battery is provide
> power to the computer it's connected to, like a laptop battery or a UPS.
> 
> Tested-by: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-logitech-hidpp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 4eeb550..4aaf237 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -761,6 +761,7 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
>  static enum power_supply_property hidpp_battery_props[] = {
>  	POWER_SUPPLY_PROP_STATUS,
>  	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_SCOPE,

Actually, what is the code baseline for this patchset please? I don't 
think I've ever seen hidpp_battery_props[] before.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1563642 — Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery

On Fri, 20 Jan 2017, Benjamin Tissoires wrote:

> > > Without a scope defined, UPower assumes that the battery is provide
> > > power to the computer it's connected to, like a laptop battery or a UPS.
> > > 
> > > Tested-by: Peter Hutterer <peter.hutterer@who-t.net>
> > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index 4eeb550..4aaf237 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -761,6 +761,7 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
> > >  static enum power_supply_property hidpp_battery_props[] = {
> > >  	POWER_SUPPLY_PROP_STATUS,
> > >  	POWER_SUPPLY_PROP_CAPACITY,
> > > +	POWER_SUPPLY_PROP_SCOPE,
> > 
> > Actually, what is the code baseline for this patchset please? I don't 
> > think I've ever seen hidpp_battery_props[] before.
> > 
> 
> Sorry, I should have mentioned this was on top of your
> for-4.8/logitech-hidpp-battery (merged with for-next, but which was put
> on hold since last Summer).

Ah, so I *did* see it, but forgot :) Thanks.

I'll look at the patchset in proper context now.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1563655 — Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery

On Jan 20 2017 or thereabouts, Jiri Kosina wrote:
> On Tue, 17 Jan 2017, Benjamin Tissoires wrote:
> 
> > From: Bastien Nocera <hadess@hadess.net>
> > 
> > Without a scope defined, UPower assumes that the battery is provide
> > power to the computer it's connected to, like a laptop battery or a UPS.
> > 
> > Tested-by: Peter Hutterer <peter.hutterer@who-t.net>
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 4eeb550..4aaf237 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -761,6 +761,7 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
> >  static enum power_supply_property hidpp_battery_props[] = {
> >  	POWER_SUPPLY_PROP_STATUS,
> >  	POWER_SUPPLY_PROP_CAPACITY,
> > +	POWER_SUPPLY_PROP_SCOPE,
> 
> Actually, what is the code baseline for this patchset please? I don't 
> think I've ever seen hidpp_battery_props[] before.
> 

Sorry, I should have mentioned this was on top of your
for-4.8/logitech-hidpp-battery (merged with for-next, but which was put
on hold since last Summer).

Cheers,
Benjamin

> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

[toc] | [prev] | [next] | [standalone]

#1560658 — [PATCH 15/17] HID: logitech-hidpp: rework probe path for unifying devices

Unifying devices are different from others because they can probed
while not connected. So we need to talk to the receiver to get some
extra information like the device name and the serial.

Instead of having conditionals while attempting to read the device name
from HID++ 2.0, have a special init path for them.

Store the retrieved serial in hdev->uniq.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 17dd569..2293898 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -65,6 +65,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_FORCE_OUTPUT_REPORTS	BIT(24)
 #define HIDPP_QUIRK_HIDPP20_BATTERY		BIT(25)
 #define HIDPP_QUIRK_HIDPP10_BATTERY		BIT(26)
+#define HIDPP_QUIRK_UNIFYING			BIT(27)
 
 #define HIDPP_QUIRK_DELAYED_INIT		HIDPP_QUIRK_NO_HIDINPUT
 
@@ -586,14 +587,14 @@ static int hidpp10_battery_event(struct hidpp_device *hidpp, u8 *data, int size)
 }
 
 #define HIDPP_REG_PAIRING_INFORMATION			0xB5
-#define DEVICE_NAME					0x40
+#define HIDPP_EXTENDED_PAIRING				0x30
+#define HIDPP_DEVICE_NAME				0x40
 
-static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
+static char *hidpp_unifying_get_name(struct hidpp_device *hidpp_dev)
 {
 	struct hidpp_report response;
 	int ret;
-	/* hid-logitech-dj is in charge of setting the right device index */
-	u8 params[1] = { DEVICE_NAME };
+	u8 params[1] = { HIDPP_DEVICE_NAME };
 	char *name;
 	int len;
 
@@ -622,6 +623,52 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
 	return name;
 }
 
+static u32 hidpp_unifying_get_serial(struct hidpp_device *hidpp)
+{
+	struct hidpp_report response;
+	int ret;
+	u8 params[1] = { HIDPP_EXTENDED_PAIRING };
+
+	ret = hidpp_send_rap_command_sync(hidpp,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_GET_LONG_REGISTER,
+					HIDPP_REG_PAIRING_INFORMATION,
+					params, 1, &response);
+	if (ret)
+		return 0;
+
+	/*
+	 * We don't care about LE or BE, we will output it as a string
+	 * with %4phD, so we need to keep the order.
+	 */
+	return *((u32 *)&response.rap.params[1]);
+}
+
+static int hidpp_unifying_init(struct hidpp_device *hidpp)
+{
+	struct hid_device *hdev = hidpp->hid_dev;
+	const char *name;
+	u32 serial;
+
+	serial = hidpp_unifying_get_serial(hidpp);
+	if (serial == 0)
+		return -EIO;
+
+	name = hidpp_unifying_get_name(hidpp);
+	if (!name)
+		return -EIO;
+
+	snprintf(hdev->name, sizeof(hdev->name), "%s", name);
+	dbg_hid("HID++ Unifying: Got name: %s\n", name);
+
+	snprintf(hdev->uniq, sizeof(hdev->uniq), "%04x-%4phD",
+		 hdev->product, &serial);
+	dbg_hid("HID++ Unifying: Got serial: %s\n", hdev->uniq);
+
+	kfree(name);
+	return 0;
+}
+
 /* -------------------------------------------------------------------------- */
 /* 0x0000: Root                                                               */
 /* -------------------------------------------------------------------------- */
@@ -2600,22 +2647,15 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 	return ret;
 }
 
-static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
+static void hidpp_overwrite_name(struct hid_device *hdev)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	char *name;
 
-	if (use_unifying)
-		/*
-		 * the device is connected through an Unifying receiver, and
-		 * might not be already connected.
-		 * Ask the receiver for its name.
-		 */
-		name = hidpp_get_unifying_name(hidpp);
-	else if (hidpp->protocol_major < 2)
+	if (hidpp->protocol_major < 2)
 		return;
-	else
-		name = hidpp_get_device_name(hidpp);
+
+	name = hidpp_get_device_name(hidpp);
 
 	if (!name) {
 		hid_err(hdev, "unable to retrieve the name of the device");
@@ -2837,6 +2877,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	hidpp->quirks = id->driver_data;
 
+	if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE)
+		hidpp->quirks |= HIDPP_QUIRK_UNIFYING;
+
 	if (disable_raw_mode) {
 		hidpp->quirks &= ~HIDPP_QUIRK_CLASS_WTP;
 		hidpp->quirks &= ~HIDPP_QUIRK_NO_HIDINPUT;
@@ -2887,8 +2930,11 @@ 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);
+
 	connected = hidpp_is_connected(hidpp);
-	if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
+	if (!(hidpp->quirks & HIDPP_QUIRK_UNIFYING)) {
 		if (!connected) {
 			ret = -ENODEV;
 			hid_err(hdev, "Device not connected");
@@ -2899,7 +2945,8 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			 hidpp->protocol_major, hidpp->protocol_minor);
 	}
 
-	hidpp_overwrite_name(hdev, id->group == HID_GROUP_LOGITECH_DJ_DEVICE);
+	if (connected)
+		hidpp_overwrite_name(hdev);
 	atomic_set(&hidpp->connected, connected);
 
 	if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560659 — [PATCH 16/17] HID: logitech-hidpp: report battery for the G700 over wireless

The receiver of the G700 is similar to a Unifying one, but not entirely.
It doesn't come with the DJ collections and thus can't be handled by
hid-logitech-dj.

To enable connection notifications, we need to instruct the receiver (0xff)
that we can handle those. And the actual device will be connected through
device index 0x01.

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

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9905455..55123ea 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1967,6 +1967,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2) },
 #endif
+	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G700_RECEIVER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WII_WHEEL) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACETRAVELLER) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 58c365f..195ba8a 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -670,6 +670,7 @@
 #define USB_DEVICE_ID_LOGITECH_CORDLESS_DESKTOP_LX500	0xc512
 #define USB_DEVICE_ID_MX3000_RECEIVER	0xc513
 #define USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER	0xc52b
+#define USB_DEVICE_ID_LOGITECH_G700_RECEIVER		0xc531
 #define USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2	0xc532
 #define USB_DEVICE_ID_SPACETRAVELLER	0xc623
 #define USB_DEVICE_ID_SPACENAVIGATOR	0xc626
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2293898..99caec4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -132,6 +132,7 @@ struct hidpp_device {
 	bool answer_available;
 	u8 protocol_major;
 	u8 protocol_minor;
+	u8 device_index;
 
 	void *private_data;
 
@@ -187,11 +188,7 @@ static int __hidpp_send_report(struct hid_device *hdev,
 		return -ENODEV;
 	}
 
-	/*
-	 * set the device_index as the receiver, it will be overwritten by
-	 * hid_hw_request if needed
-	 */
-	hidpp_report->device_index = 0xff;
+	hidpp_report->device_index = hidpp->device_index;
 
 	if (hidpp->quirks & HIDPP_QUIRK_FORCE_OUTPUT_REPORTS) {
 		ret = hid_hw_output_report(hdev, (u8 *)hidpp_report, fields_count);
@@ -422,6 +419,36 @@ static int hidpp10_enable_battery_reporting(struct hidpp_device *hidpp_dev)
 					params, 3, &response);
 }
 
+/* Must be called with 0xff as device index */
+static int hidpp_unifying_enable_notifications(struct hidpp_device *hidpp_dev)
+{
+	struct hidpp_report response;
+	int ret;
+	u8 params[3] = { 0 };
+
+	ret = hidpp_send_rap_command_sync(hidpp_dev,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_GET_REGISTER,
+					HIDPP_REG_GENERAL,
+					NULL, 0, &response);
+	if (ret)
+		return ret;
+
+	memcpy(params, response.rap.params, 3);
+
+	/* Set the wireless notification bit */
+	params[1] |= BIT(0);
+	params[1] |= BIT(3);
+
+	ret = hidpp_send_rap_command_sync(hidpp_dev,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_SET_REGISTER,
+					HIDPP_REG_GENERAL,
+					params, 3, &response);
+
+	return ret;
+}
+
 #define HIDPP_REG_BATTERY_STATUS			0x07
 
 static int hidpp10_battery_status_map_level(u8 param)
@@ -587,9 +614,32 @@ static int hidpp10_battery_event(struct hidpp_device *hidpp, u8 *data, int size)
 }
 
 #define HIDPP_REG_PAIRING_INFORMATION			0xB5
+#define HIDPP_PAIRING_INFORMATION			0x20
 #define HIDPP_EXTENDED_PAIRING				0x30
 #define HIDPP_DEVICE_NAME				0x40
 
+static u16 hidpp_unifying_get_quadid(struct hidpp_device *hidpp_dev, int *index)
+{
+	struct hidpp_report response;
+	int ret;
+	u8 params[1] = { HIDPP_PAIRING_INFORMATION };
+	__be16 *quadid;
+
+	ret = hidpp_send_rap_command_sync(hidpp_dev,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_GET_LONG_REGISTER,
+					HIDPP_REG_PAIRING_INFORMATION,
+					params, 1, &response);
+	if (ret)
+		return 0;
+
+	quadid = (__be16 *)&response.rap.params[3];
+	/* the device index goes from 1 to 6 */
+	*index = (response.rap.params[0] & 0x0F) + 1;
+
+	return be16_to_cpup(quadid);
+}
+
 static char *hidpp_unifying_get_name(struct hidpp_device *hidpp_dev)
 {
 	struct hidpp_report response;
@@ -649,6 +699,12 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
 	struct hid_device *hdev = hidpp->hid_dev;
 	const char *name;
 	u32 serial;
+	u16 quadid;
+	int device_index;
+
+	quadid = hidpp_unifying_get_quadid(hidpp, &device_index);
+	if (quadid == 0)
+		return -EIO;
 
 	serial = hidpp_unifying_get_serial(hidpp);
 	if (serial == 0)
@@ -661,10 +717,13 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
 	snprintf(hdev->name, sizeof(hdev->name), "%s", name);
 	dbg_hid("HID++ Unifying: Got name: %s\n", name);
 
-	snprintf(hdev->uniq, sizeof(hdev->uniq), "%04x-%4phD",
-		 hdev->product, &serial);
+	snprintf(hdev->uniq, sizeof(hdev->uniq), "%04x-%4phD", quadid, &serial);
 	dbg_hid("HID++ Unifying: Got serial: %s\n", hdev->uniq);
 
+	if (quadid != hdev->product)
+		hidpp_unifying_enable_notifications(hidpp);
+	hidpp->device_index = device_index;
+
 	kfree(name);
 	return 0;
 }
@@ -2880,6 +2939,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE)
 		hidpp->quirks |= HIDPP_QUIRK_UNIFYING;
 
+	/*
+	 * set the device_index as the receiver, it will be overwritten by
+	 * hid_hw_request if needed
+	 */
+	hidpp->device_index = 0xFF;
+
 	if (disable_raw_mode) {
 		hidpp->quirks &= ~HIDPP_QUIRK_CLASS_WTP;
 		hidpp->quirks &= ~HIDPP_QUIRK_NO_HIDINPUT;
@@ -3034,6 +3099,9 @@ static const struct hid_device_id hidpp_devices[] = {
 	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
 		USB_VENDOR_ID_LOGITECH, 0x4002),
 	  .driver_data = HIDPP_QUIRK_CLASS_K750 },
+	{ /* G700 over Wireless */
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G700_RECEIVER),
+	  .driver_data = HIDPP_QUIRK_UNIFYING },
 
 	{ HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
 		USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560660 — [PATCH 10/17] HID: logitech-hidpp: notify battery on connect

When a device reconnects, there is a high chance its power supply has
been changed (for a battery replacement for instance). Just forward
the battery state here.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1fd95c2..3ae2f41 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2557,9 +2557,6 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 			return ret;
 		hidpp->quirks |= HIDPP_QUIRK_HIDPP20_BATTERY;
 	} else {
-		ret = hidpp10_enable_battery_reporting(hidpp);
-		if (ret)
-			return -ENOENT;
 		ret = hidpp10_query_battery_status(hidpp);
 		if (ret) {
 			ret = hidpp10_query_battery_mileage(hidpp);
@@ -2693,6 +2690,17 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 
 	hidpp_initialize_battery(hidpp);
 
+	/* forward current battery state */
+	if (hidpp->quirks & HIDPP_QUIRK_HIDPP10_BATTERY) {
+		hidpp10_enable_battery_reporting(hidpp);
+		hidpp10_query_battery_status(hidpp);
+		hidpp10_query_battery_mileage(hidpp);
+	} else if (hidpp->quirks & HIDPP_QUIRK_HIDPP20_BATTERY) {
+		hidpp20_query_battery_info(hidpp);
+	}
+	if (hidpp->battery.ps)
+		power_supply_changed(hidpp->battery.ps);
+
 	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT))
 		/* if HID created the input nodes for us, we can stop now */
 		return;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560670 — [PATCH 04/17] HID: logitech-hidpp: battery: remove overloads and provide ONLINE

When ONLINE isn't set, upower should ignore the battery capacity, so there
is no need to overload it with some random values.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1cda29e..3c57886 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -632,48 +632,39 @@ static int hidpp20_batterylevel_map_status_level(u8 data[3], int *level,
 						 int *next_level)
 {
 	int status;
-	int level_override;
 
-	*level = data[0];
+	if (data[0])
+		*level = data[0];
 	*next_level = data[1];
 
 	/* When discharging, we can rely on the device reported level.
-	 * For all other states the device reports level 0 (unknown). Make up
-	 * a number instead
+	 * For all other states the device reports level 0 (unknown).
 	 */
 	switch (data[2]) {
 		case 0: /* discharging (in use) */
 			status = POWER_SUPPLY_STATUS_DISCHARGING;
-			level_override = 0;
 			break;
 		case 1: /* recharging */
 			status = POWER_SUPPLY_STATUS_CHARGING;
-			level_override = 80;
 			break;
 		case 2: /* charge in final stage */
 			status = POWER_SUPPLY_STATUS_CHARGING;
-			level_override = 90;
 			break;
 		case 3: /* charge complete */
 			status = POWER_SUPPLY_STATUS_FULL;
-			level_override = 100;
+			*level = 100;
 			break;
 		case 4: /* recharging below optimal speed */
 			status = POWER_SUPPLY_STATUS_CHARGING;
-			level_override = 50;
 			break;
 		/* 5 = invalid battery type
 		   6 = thermal error
 		   7 = other charging error */
 		default:
 			status = POWER_SUPPLY_STATUS_NOT_CHARGING;
-			level_override = 0;
 			break;
 	}
 
-	if (level_override != 0 && *level == 0)
-		*level = level_override;
-
 	return status;
 }
 
@@ -719,6 +710,8 @@ static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
 			return ret;
 	}
 
+	level = 0;
+	next_level = 0;
 	ret = hidpp20_batterylevel_get_battery_level(hidpp,
 						     hidpp->battery.feature_index,
 						     &status, &level, &next_level);
@@ -742,6 +735,8 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
 	    report->fap.funcindex_clientid != EVENT_BATTERY_LEVEL_STATUS_BROADCAST)
 		return 0;
 
+	level = hidpp->battery.level;
+
 	status = hidpp20_batterylevel_map_status_level(report->fap.params,
 						       &level, &next_level);
 
@@ -759,6 +754,7 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
 }
 
 static enum power_supply_property hidpp_battery_props[] = {
+	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_SCOPE,
@@ -781,6 +777,12 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 		case POWER_SUPPLY_PROP_SCOPE:
 			val->intval = POWER_SUPPLY_SCOPE_DEVICE;
 			break;
+		case POWER_SUPPLY_PROP_ONLINE:
+			val->intval = hidpp->battery.status ==
+					      POWER_SUPPLY_STATUS_DISCHARGING ||
+					hidpp->battery.status ==
+					      POWER_SUPPLY_STATUS_FULL;
+			break;
 		default:
 			ret = -EINVAL;
 			break;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560680 — [PATCH 07/17] HID: logitech-hidpp: return an error if the feature is not present

Or the device just answers a valid feature '0'.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0c7144f..404b3b8 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -452,6 +452,9 @@ static int hidpp_root_get_feature(struct hidpp_device *hidpp, u16 feature,
 	if (ret)
 		return ret;
 
+	if (response.fap.params[0] == 0)
+		return -ENOENT;
+
 	*feature_index = response.fap.params[0];
 	*feature_type = response.fap.params[1];
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web