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

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

[Benjamin Tissoires]

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

[PATCH 01/17] HID: logitech-dj: allow devices to request full pairing information

[Benjamin Tissoires]

Register 0xB5 should be handled specially no matter what function is
used. This allows to retrieve the serial and the Quad ID from
hid-logitech-hidpp directly.

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

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 5bc6d80..71ce4ca 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -692,8 +692,12 @@ static void logi_dj_ll_close(struct hid_device *hid)
 	dbg_hid("%s:%s\n", __func__, hid->phys);
 }
 
-static u8 unifying_name_query[]  = {0x10, 0xff, 0x83, 0xb5, 0x40, 0x00, 0x00};
-static u8 unifying_name_answer[] = {0x11, 0xff, 0x83, 0xb5};
+/*
+ * Register 0xB5 is "pairing information". It is solely intended for the
+ * receiver, so do not overwrite the device index.
+ */
+static u8 unifying_pairing_query[]  = {0x10, 0xff, 0x83, 0xb5};
+static u8 unifying_pairing_answer[] = {0x11, 0xff, 0x83, 0xb5};
 
 static int logi_dj_ll_raw_request(struct hid_device *hid,
 				  unsigned char reportnum, __u8 *buf,
@@ -712,8 +716,8 @@ static int logi_dj_ll_raw_request(struct hid_device *hid,
 
 		/* special case where we should not overwrite
 		 * the device_index */
-		if (count == 7 && !memcmp(buf, unifying_name_query,
-					  sizeof(unifying_name_query)))
+		if (count == 7 && !memcmp(buf, unifying_pairing_query,
+					  sizeof(unifying_pairing_query)))
 			buf[4] |= djdev->device_index - 1;
 		else
 			buf[1] = djdev->device_index;
@@ -911,9 +915,8 @@ static int logi_dj_hidpp_event(struct hid_device *hdev,
 		/* special case were the device wants to know its unifying
 		 * name */
 		if (size == HIDPP_REPORT_LONG_LENGTH &&
-		    !memcmp(data, unifying_name_answer,
-			    sizeof(unifying_name_answer)) &&
-		    ((data[4] & 0xF0) == 0x40))
+		    !memcmp(data, unifying_pairing_answer,
+			    sizeof(unifying_pairing_answer)))
 			device_index = (data[4] & 0x0F) + 1;
 		else
 			return false;
-- 
2.9.3

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

[Benjamin Tissoires]

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

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

[Bastien Nocera]

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/

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

[Jiri Kosina]

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

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

[Benjamin Tissoires]

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
> 

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

[Jiri Kosina]

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

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

[Benjamin Tissoires]

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

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

[Benjamin Tissoires]

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

[PATCH 05/17] HID: logitech-hidpp: forward device info in power_supply

[Benjamin Tissoires]

Better forwarding the device name, manufacturer and serial to upower.
Note that serial is still empty, it will be filled in a later patch
in this series.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3c57886..77bfb65 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -758,6 +758,9 @@ static enum power_supply_property hidpp_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_SCOPE,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+	POWER_SUPPLY_PROP_SERIAL_NUMBER,
 };
 
 static int hidpp_battery_get_property(struct power_supply *psy,
@@ -783,6 +786,15 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 					hidpp->battery.status ==
 					      POWER_SUPPLY_STATUS_FULL;
 			break;
+		case POWER_SUPPLY_PROP_MODEL_NAME:
+			val->strval = hidpp->hid_dev->name;
+			break;
+		case POWER_SUPPLY_PROP_MANUFACTURER:
+			val->strval = "Logitech";
+			break;
+		case POWER_SUPPLY_PROP_SERIAL_NUMBER:
+			val->strval = hidpp->hid_dev->uniq;
+			break;
 		default:
 			ret = -EINVAL;
 			break;
-- 
2.9.3

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

[Benjamin Tissoires]

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

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

[Benjamin Tissoires]

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

[PATCH 08/17] HID: logitech-hidpp: add support for battery status for the K750

[Benjamin Tissoires]

The Solar Keyboard uses a different feature to report the battery level.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 404b3b8..91ea553 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -56,6 +56,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_CLASS_M560			BIT(1)
 #define HIDPP_QUIRK_CLASS_K400			BIT(2)
 #define HIDPP_QUIRK_CLASS_G920			BIT(3)
+#define HIDPP_QUIRK_CLASS_K750			BIT(4)
 
 /* bits 2..20 are reserved for classes */
 /* #define HIDPP_QUIRK_CONNECT_EVENTS		BIT(21) disabled */
@@ -113,6 +114,7 @@ struct hidpp_report {
 
 struct hidpp_battery {
 	u8 feature_index;
+	u8 solar_feature_index;
 	struct power_supply_desc desc;
 	struct power_supply *ps;
 	char name[64];
@@ -704,7 +706,7 @@ static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
 	int ret;
 	int status, level, next_level;
 
-	if (hidpp->battery.feature_index == 0) {
+	if (hidpp->battery.feature_index == 0xff) {
 		ret = hidpp_root_get_feature(hidpp,
 					     HIDPP_PAGE_BATTERY_LEVEL_STATUS,
 					     &hidpp->battery.feature_index,
@@ -807,6 +809,72 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 }
 
 /* -------------------------------------------------------------------------- */
+/* 0x4301: Solar Keyboard                                                     */
+/* -------------------------------------------------------------------------- */
+
+#define HIDPP_PAGE_SOLAR_KEYBOARD			0x4301
+
+#define CMD_SOLAR_SET_LIGHT_MEASURE			0x00
+
+#define EVENT_SOLAR_BATTERY_BROADCAST			0x00
+#define EVENT_SOLAR_BATTERY_LIGHT_MEASURE		0x10
+#define EVENT_SOLAR_CHECK_LIGHT_BUTTON			0x20
+
+static int hidpp_solar_request_battery_event(struct hidpp_device *hidpp)
+{
+	struct hidpp_report response;
+	u8 params[2] = { 3, 4 };
+	u8 feature_type;
+	int ret;
+
+	if (hidpp->battery.feature_index == 0xff) {
+		ret = hidpp_root_get_feature(hidpp,
+					     HIDPP_PAGE_SOLAR_KEYBOARD,
+					     &hidpp->battery.solar_feature_index,
+					     &feature_type);
+		if (ret)
+			return ret;
+	}
+
+	ret = hidpp_send_fap_command_sync(hidpp,
+					  hidpp->battery.solar_feature_index,
+					  CMD_SOLAR_SET_LIGHT_MEASURE,
+					  params, 2, &response);
+	if (ret > 0) {
+		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+			__func__, ret);
+		return -EPROTO;
+	}
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int hidpp_solar_battery_event(struct hidpp_device *hidpp,
+				     u8 *data, int size)
+{
+	struct hidpp_report *report = (struct hidpp_report *)data;
+	int level;
+
+	if (report->fap.feature_index != hidpp->battery.solar_feature_index ||
+	    !(report->fap.funcindex_clientid == EVENT_SOLAR_BATTERY_BROADCAST ||
+	      report->fap.funcindex_clientid == EVENT_SOLAR_BATTERY_LIGHT_MEASURE ||
+	      report->fap.funcindex_clientid == EVENT_SOLAR_CHECK_LIGHT_BUTTON))
+		return 0;
+
+	level = report->fap.params[0];
+
+	if (level != hidpp->battery.level) {
+		hidpp->battery.level = level;
+		if (hidpp->battery.ps)
+			power_supply_changed(hidpp->battery.ps);
+	}
+
+	return 0;
+}
+
+/* -------------------------------------------------------------------------- */
 /* 0x6010: Touchpad FW items                                                  */
 /* -------------------------------------------------------------------------- */
 
@@ -2253,6 +2321,9 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 		ret = hidpp20_battery_event(hidpp, data, size);
 		if (ret != 0)
 			return ret;
+		ret = hidpp_solar_battery_event(hidpp, data, size);
+		if (ret != 0)
+			return ret;
 	}
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
@@ -2275,8 +2346,15 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 	if (hidpp->battery.ps)
 		return 0;
 
+	hidpp->battery.feature_index = 0xff;
+	hidpp->battery.solar_feature_index = 0xff;
+
 	if (hidpp->protocol_major >= 2) {
-		ret = hidpp20_query_battery_info(hidpp);
+		if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750)
+			ret = hidpp_solar_request_battery_event(hidpp);
+		else
+			ret = hidpp20_query_battery_info(hidpp);
+
 		if (ret)
 			return ret;
 		hidpp->quirks |= HIDPP_QUIRK_HIDPP20_BATTERY;
@@ -2605,6 +2683,10 @@ static const struct hid_device_id hidpp_devices[] = {
 	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
 		USB_VENDOR_ID_LOGITECH, 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),
+	  .driver_data = HIDPP_QUIRK_CLASS_K750 },
 
 	{ HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
 		USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},
-- 
2.9.3

[PATCH 09/17] HID: logitech-hidpp: enable HID++ 1.0 battery reporting

[Benjamin Tissoires]

Also enable battery reporting for HID++ 1.0 devices through 2 registers:
0x07: battery status -> reports only 4 levels (critical, low, good, full)
0x0D: battery mileage -> reports true pourcentage

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 91ea553..1fd95c2 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -393,6 +393,198 @@ 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)
+{
+	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 battery bit */
+	params[0] |= BIT(4);
+
+	return hidpp_send_rap_command_sync(hidpp_dev,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_SET_REGISTER,
+					HIDPP_REG_GENERAL,
+					params, 3, &response);
+}
+
+#define HIDPP_REG_BATTERY_STATUS			0x07
+
+static int hidpp10_battery_status_map_level(u8 param)
+{
+	int level;
+
+	switch (param) {
+	case 1 ... 2:
+		level = 5;
+		break;
+	case 3 ... 4:
+		level = 20;
+		break;
+	case 5 ... 6:
+		level = 55;
+		break;
+	case 7:
+		level = 90;
+		break;
+	default:
+		level = 0;
+	}
+
+	return level;
+}
+
+static int hidpp10_battery_status_map_status(u8 param)
+{
+	int status;
+
+	switch (param) {
+	case 0x00:
+		/* discharging (in use) */
+		status = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+	case 0x21: /* (standard) charging */
+	case 0x24: /* fast charging */
+	case 0x25: /* slow charging */
+		status = POWER_SUPPLY_STATUS_CHARGING;
+		break;
+	case 0x26: /* topping charge */
+	case 0x22: /* charge complete */
+		status = POWER_SUPPLY_STATUS_FULL;
+		break;
+	/*
+	 * 0x01...0x1F = reserved (not charging)
+	 * 0x20 = unknown
+	 * 0x23 = charging error
+	 * 0x27..0xff = reserved
+	 */
+	default:
+		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	}
+
+	return status;
+}
+
+static int hidpp10_query_battery_status(struct hidpp_device *hidpp)
+{
+	struct hidpp_report response;
+	int ret;
+
+	ret = hidpp_send_rap_command_sync(hidpp,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_GET_REGISTER,
+					HIDPP_REG_BATTERY_STATUS,
+					NULL, 0, &response);
+	if (ret)
+		return ret;
+
+	hidpp->battery.level =
+		hidpp10_battery_status_map_level(response.rap.params[0]);
+
+	hidpp->battery.status =
+		hidpp10_battery_status_map_status(response.rap.params[1]);
+
+	return 0;
+}
+
+#define HIDPP_REG_BATTERY_MILEAGE			0x0D
+
+static int hidpp10_battery_mileage_map_status(u8 param)
+{
+	int status;
+
+	switch (param >> 6) {
+	case 0x00:
+		/* discharging (in use) */
+		status = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+	case 0x01: /* charging */
+		status = POWER_SUPPLY_STATUS_CHARGING;
+		break;
+	case 0x02: /* charge complete */
+		status = POWER_SUPPLY_STATUS_FULL;
+		break;
+	/*
+	 * 0x03 = charging error
+	 */
+	default:
+		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	}
+
+	return status;
+}
+
+static int hidpp10_query_battery_mileage(struct hidpp_device *hidpp)
+{
+	struct hidpp_report response;
+	int ret;
+
+	ret = hidpp_send_rap_command_sync(hidpp,
+					REPORT_ID_HIDPP_SHORT,
+					HIDPP_GET_REGISTER,
+					HIDPP_REG_BATTERY_MILEAGE,
+					NULL, 0, &response);
+	if (ret)
+		return ret;
+
+	hidpp->battery.level = response.rap.params[0];
+
+	hidpp->battery.status =
+		hidpp10_battery_mileage_map_status(response.rap.params[2]);
+
+	return 0;
+}
+
+static int hidpp10_battery_event(struct hidpp_device *hidpp, u8 *data, int size)
+{
+	struct hidpp_report *report = (struct hidpp_report *)data;
+	int status, level;
+	bool changed;
+
+	if (report->report_id != REPORT_ID_HIDPP_SHORT)
+		return 0;
+
+	switch (report->rap.reg_address) {
+	case HIDPP_REG_BATTERY_STATUS:
+		level = hidpp10_battery_status_map_level(report->rap.params[0]);
+		status = hidpp10_battery_status_map_status(report->rap.params[1]);
+		break;
+	case HIDPP_REG_BATTERY_MILEAGE:
+		level = report->rap.params[0];
+		status = hidpp10_battery_mileage_map_status(report->rap.params[2]);
+		break;
+	default:
+		return 0;
+	}
+
+	changed = level != hidpp->battery.level ||
+		  status != hidpp->battery.status;
+
+	if (changed) {
+		hidpp->battery.level = level;
+		hidpp->battery.status = status;
+		if (hidpp->battery.ps)
+			power_supply_changed(hidpp->battery.ps);
+	}
+
+	return 0;
+}
+
 #define HIDPP_REG_PAIRING_INFORMATION			0xB5
 #define DEVICE_NAME					0x40
 
@@ -2326,6 +2518,12 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 			return ret;
 	}
 
+	if (hidpp->quirks & HIDPP_QUIRK_HIDPP10_BATTERY) {
+		ret = hidpp10_battery_event(hidpp, data, size);
+		if (ret != 0)
+			return ret;
+	}
+
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
 		return wtp_raw_event(hdev, data, size);
 	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560)
@@ -2359,7 +2557,16 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 			return ret;
 		hidpp->quirks |= HIDPP_QUIRK_HIDPP20_BATTERY;
 	} else {
-		return -ENOENT;
+		ret = hidpp10_enable_battery_reporting(hidpp);
+		if (ret)
+			return -ENOENT;
+		ret = hidpp10_query_battery_status(hidpp);
+		if (ret) {
+			ret = hidpp10_query_battery_mileage(hidpp);
+			if (ret)
+				return -ENOENT;
+		}
+		hidpp->quirks |= HIDPP_QUIRK_HIDPP10_BATTERY;
 	}
 
 	battery = &hidpp->battery;
-- 
2.9.3

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

[Benjamin Tissoires]

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

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

[Benjamin Tissoires]

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

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

[Benjamin Tissoires]

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

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

[Benjamin Tissoires]

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

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

[Benjamin Tissoires]

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
>

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

[Jiri Kosina]

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

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

[Benjamin Tissoires]

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

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

[Benjamin Tissoires]

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

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

[Benjamin Tissoires]

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

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

[Benjamin Tissoires]

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

[PATCH 17/17] HID: logitech-hidpp: retrieve the name of the gaming mice over wireless

[Benjamin Tissoires]

The G700 and the G900 present both a wireless receiver with almost unifying
protocol. We handle them as bypass for the mouse and keyboard nodes, but
they appear in the system as "Logitech USB Receiver".

We can do better by retrieving the name from one of the nodes, the HID++
one of the receiver. To get the valid values in the non-HID++ nodes, we
delay their probing until the HID++ node has been found. This way, we
can retrieve the shared names and overwrite the USB provided ones.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 99caec4..3412101 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -123,6 +123,17 @@ struct hidpp_battery {
 	int level;
 };
 
+struct hidpp_shared {
+	struct list_head list;
+	int count;
+	char name[128];		/* Device name */
+	char phys[64];		/* Device physical location */
+	char uniq[64];		/* Device unique identifier (serial #) */
+};
+
+static LIST_HEAD(hidpp_shared_list);
+static DEFINE_MUTEX(hidpp_shared_list_lock);
+
 struct hidpp_device {
 	struct hid_device *hid_dev;
 	struct mutex send_mutex;
@@ -144,6 +155,8 @@ struct hidpp_device {
 	unsigned long quirks;
 
 	struct hidpp_battery battery;
+
+	struct hidpp_shared *shared;
 };
 
 /* HID++ 1.0 error codes */
@@ -2849,6 +2862,78 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 	hidpp->delayed_input = input;
 }
 
+static void hidpp_remove_shared_data(void *res)
+{
+	struct hidpp_device *hidpp = res;
+
+	mutex_lock(&hidpp_shared_list_lock);
+
+	if (hidpp->shared && !--hidpp->shared->count) {
+		list_del(&hidpp->shared->list);
+		kfree(hidpp->shared);
+	}
+	hidpp->shared = NULL;
+
+	mutex_unlock(&hidpp_shared_list_lock);
+}
+
+static bool hidpp_compare_device_paths(const char *hdev_a, const char *hdev_b)
+{
+	const char separator = '/';
+	int n1 = strrchr(hdev_a, separator) - hdev_a;
+	int n2 = strrchr(hdev_b, separator) - hdev_b;
+
+	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+		return false;
+
+	return !strncmp(hdev_a, hdev_b, n1);
+}
+
+static int hidpp_get_shared_data(struct hidpp_device *hidpp)
+{
+	struct hid_device *hdev = hidpp->hid_dev;
+	struct hidpp_shared *s, *shared = NULL;
+	int ret;
+
+	mutex_lock(&hidpp_shared_list_lock);
+
+	if (hidpp->device_index != 0xff) {
+		shared = kzalloc(sizeof(struct hidpp_shared), GFP_KERNEL);
+		if (!shared) {
+			mutex_unlock(&hidpp_shared_list_lock);
+			return -ENOMEM;
+		}
+		memcpy(shared->name, hdev->name, sizeof(shared->name));
+		memcpy(shared->uniq, hdev->uniq, sizeof(shared->uniq));
+		memcpy(shared->phys, hdev->phys, sizeof(shared->phys));
+		list_add_tail(&shared->list, &hidpp_shared_list);
+	} else {
+		list_for_each_entry(s, &hidpp_shared_list, list) {
+			if (hidpp_compare_device_paths(s->phys, hdev->phys))
+				shared = s;
+		}
+	}
+
+	if (!shared) {
+		mutex_unlock(&hidpp_shared_list_lock);
+		return -EPROBE_DEFER;
+	}
+
+	hidpp->shared = shared;
+	shared->count++;
+
+	mutex_unlock(&hidpp_shared_list_lock);
+
+	ret = devm_add_action_or_reset(&hidpp->hid_dev->dev,
+					hidpp_remove_shared_data, hidpp);
+	if (ret)
+		return ret;
+
+	memcpy(hdev->name, shared->name, sizeof(hdev->name));
+	memcpy(hdev->uniq, shared->uniq, sizeof(hdev->uniq));
+	return 0;
+}
+
 static DEVICE_ATTR(builtin_power_supply, 0000, NULL, NULL);
 
 static struct attribute *sysfs_attrs[] = {
@@ -2910,8 +2995,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	/*
 	 * Make sure the device is HID++ capable, otherwise treat as generic HID
+	 * Special case for manually affected Unifying receiver where we want to
+	 * link the interfaces together to have a proper name instead of
+	 * "Logitech USB Receiver".
 	 */
-	if (!hidpp_validate_device(hdev))
+	if (!(id->driver_data & HIDPP_QUIRK_UNIFYING) &&
+	    !hidpp_validate_device(hdev))
 		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 
 	/*
@@ -2995,8 +3084,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);
+	if (hidpp->quirks & HIDPP_QUIRK_UNIFYING) {
+		ret = hidpp_unifying_init(hidpp);
+		if (ret)
+			goto start;
+	}
 
 	connected = hidpp_is_connected(hidpp);
 	if (!(hidpp->quirks & HIDPP_QUIRK_UNIFYING)) {
@@ -3026,6 +3118,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	hidpp_connect_event(hidpp);
 
+start:
 	/* Reset the HID node state */
 	hid_device_io_stop(hdev);
 	hid_hw_close(hdev);
@@ -3034,6 +3127,13 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
 		connect_mask &= ~HID_CONNECT_HIDINPUT;
 
+	if ((hidpp->quirks & HIDPP_QUIRK_UNIFYING) &&
+	    id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
+		ret = hidpp_get_shared_data(hidpp);
+		if (ret)
+			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) {
-- 
2.9.3

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

[Bastien Nocera]

On Tue, 2017-01-17 at 15:35 +0100, Benjamin Tissoires wrote:
> 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
> 
> 
<snip>

I've tested your patches with the kernel build that you kindly
provided. The output of "upower -d", here[1], shows both a K750
keyboard (the one with the solar charging) and a T650 touchpad (which
was plugged in to a separate power supply when testing).

Here's a jumble of notes:
- UPower expects the serial number to be available when the device is
created. This wasn't the case for the keyboard here, and we end up with
no serial number, even though the serial_number sysfs file is now
populated
- the K750's battery state doesn't seem to match that found by the
UPower code, eg. it's stuck in "Unknown" when upower could detect that
it is charging (it's sunny here). That might also be why the icon is
stuck at "battery-missing-symbolic".
- the model names of the batteries seem to have manufacturer
information prepended, eg. vendor: Logitech model_name: Logitech K750
I'd have expected to only have "K750" there.
- the touchpad is detected as a random "battery", but that's likely due
to the slightly dodgy code in UPower (look for "try to detect using the
device type" and cringe)
- the serial number is in a different format than in UPower:
  kernel: 4101-6f-63-fd-39
  UPower: 6F63FD39

I'll look at updating the UPower code, thanks.

UPower's "power_supply_ class code:
https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c
and its HID++ support:
https://cgit.freedesktop.org/upower/tree/src/linux/up-device-unifying.c

[1]: https://paste.fedoraproject.org/535093/51785481

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

[Benjamin Tissoires]

On Jan 23 2017 or thereabouts, Bastien Nocera wrote:
> On Tue, 2017-01-17 at 15:35 +0100, Benjamin Tissoires wrote:
> > 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
> > 
> > 
> <snip>
> 
> I've tested your patches with the kernel build that you kindly
> provided. The output of "upower -d", here[1], shows both a K750
> keyboard (the one with the solar charging) and a T650 touchpad (which
> was plugged in to a separate power supply when testing).
> 
> Here's a jumble of notes:
> - UPower expects the serial number to be available when the device is
> created. This wasn't the case for the keyboard here, and we end up with
> no serial number, even though the serial_number sysfs file is now
> populated

I think I can fix that, but I think upower might need to get some tweaks
too. In the kernel, the way the sysfs files are created is decided by
power_supply core. We just set a list of properties and power_supply
creates the sysfs. I would say it creates the files with the order of
the property list we provide, thus the fixable state.

But this means that the order we declare the properties is rather
important because you are probably notified by one specific property,
which should be the last one in the list. I'd need to check into the
udev enumeration process, but we should probably enforce upower to not
handle the power_supply before it gets fully initialized (either in
power_supply core, or in udev, or in upower).

> - the K750's battery state doesn't seem to match that found by the
> UPower code, eg. it's stuck in "Unknown" when upower could detect that
> it is charging (it's sunny here). That might also be why the icon is
> stuck at "battery-missing-symbolic".

I'll look into that. But I tested it with 2 K750 here and both were
reporting charging... I wonder if this has to do with the enumeration
process too.

> - the model names of the batteries seem to have manufacturer
> information prepended, eg. vendor: Logitech model_name: Logitech K750
> I'd have expected to only have "K750" there.

That's fixable.

> - the touchpad is detected as a random "battery", but that's likely due
> to the slightly dodgy code in UPower (look for "try to detect using the
> device type" and cringe)

Yep, I'll cringe, for sure :)

> - the serial number is in a different format than in UPower:
>   kernel: 4101-6f-63-fd-39
>   UPower: 6F63FD39

Yes, the serial is the same format as the one reported on the Windows
application. The first 4 chars are the unifying PID, and the rest is the
same than yours. I like having the Quad ID (unifying PID): that way, you
are ensured to have a unique identifier across all Unifying devices.

Is it really an issue to change the serial?

> 
> I'll look at updating the UPower code, thanks.

Thank you for testing and reporting!

Cheers,
Benjamin

> 
> UPower's "power_supply_ class code:
> https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c
> and its HID++ support:
> https://cgit.freedesktop.org/upower/tree/src/linux/up-device-unifying.c
> 
> [1]: https://paste.fedoraproject.org/535093/51785481

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

[Bastien Nocera]

On Mon, 2017-01-23 at 15:35 +0100, Bastien Nocera wrote:
> On Tue, 2017-01-17 at 15:35 +0100, Benjamin Tissoires wrote:
> > 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
> > 
> > 
> 
> <snip>
> 
> I've tested your patches with the kernel build that you kindly
> provided. The output of "upower -d", here[1], shows both a K750
> keyboard (the one with the solar charging) and a T650 touchpad (which
> was plugged in to a separate power supply when testing).
> 
> Here's a jumble of notes:
> - UPower expects the serial number to be available when the device is
> created. This wasn't the case for the keyboard here, and we end up
> with
> no serial number, even though the serial_number sysfs file is now
> populated

I think I've fixed this, the code in UPower didn't even try to read the
serial_number attribute, for "devices" (as opposed to the batteries
that power the computer).

> - the K750's battery state doesn't seem to match that found by the
> UPower code, eg. it's stuck in "Unknown" when upower could detect
> that
> it is charging (it's sunny here). That might also be why the icon is
> stuck at "battery-missing-symbolic".
> - the model names of the batteries seem to have manufacturer
> information prepended, eg. vendor: Logitech model_name: Logitech K750
> I'd have expected to only have "K750" there.
> - the touchpad is detected as a random "battery", but that's likely
> due
> to the slightly dodgy code in UPower (look for "try to detect using
> the
> device type" and cringe)

This is fixed in UPower master, and it's much nicer.

> - the serial number is in a different format than in UPower:
>   kernel: 4101-6f-63-fd-39
>   UPower: 6F63FD39
> 
> I'll look at updating the UPower code, thanks.
> 
> UPower's "power_supply_ class code:
> https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c
> and its HID++ support:
> https://cgit.freedesktop.org/upower/tree/src/linux/up-device-unifying
> .c
> 
> [1]: https://paste.fedoraproject.org/535093/51785481
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html