[PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Benjamin Tissoires]

Hi,

this is finally a rework of the series that provides kernel power_supply
for hidpp devices.

This will allow upower to not handle those devices anymore and to have more
immediate reportng of the device to the system.

I have splitted the series to not support non unifying receivers here. I got
the non Unifying (DJ) receivers working locally, but given I am not so sure
it will not break the G920, I'd rather send a shorter series later (this one
is big enough already).

Cheers,
Benjamin

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

Benjamin Tissoires (18):
  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: do not query the name through HID++ for 1.0
    devices
  HID: logitech-hidpp: create a capabilities bits field
  HID: logitech-hidpp: rework probe path for unifying devices
  HID: logitech-hidpp: retrieve the HID++ device name when available
  HID: logitech-hidpp: rework hidpp_connect_event()
  HID: logitech-hidpp: handle battery events in hidpp_raw_hidpp_event()
  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 queried feature is not
    present
  HID: logitech-hidpp: notify battery on connect
  HID: logitech-hidpp: battery: provide ONLINE property
  HID: logitech-hidpp: rename battery level into capacity
  HID: logitech-hidpp: battery: provide CAPACITY_LEVEL
  HID: logitech-hidpp: add support for battery status for the K750
  HID: logitech-hidpp: enable HID++ 1.0 battery reporting
  HID: logitech-hidpp: add a sysfs file to tell we support power_supply

 drivers/hid/hid-logitech-dj.c    |  19 +-
 drivers/hid/hid-logitech-hidpp.c | 741 +++++++++++++++++++++++++++++++++------
 2 files changed, 641 insertions(+), 119 deletions(-)

-- 
2.9.3

[PATCH v3 01/19] 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>

---

changes in v3:
- fixed 0xb5 to handle any incoming parameters

no changes in v2
---
 drivers/hid/hid-logitech-dj.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 5bc6d80..826fa1e 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,9 +716,9 @@ 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)))
-			buf[4] |= djdev->device_index - 1;
+		if (count == 7 && !memcmp(buf, unifying_pairing_query,
+					  sizeof(unifying_pairing_query)))
+			buf[4] = (buf[4] & 0xf0) | (djdev->device_index - 1);
 		else
 			buf[1] = djdev->device_index;
 		return hid_hw_raw_request(djrcv_dev->hdev, reportnum, buf,
@@ -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 v3 02/19] HID: logitech-hidpp: Add scope to battery

[Benjamin Tissoires]

From: Bastien Nocera <hadess@hadess.net>

Without a scope defined, UPower assumes that the battery provides
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>

---

no changes in v3

changes in v2:
* fixed typo in commit message
---
 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 v3 02/19] HID: logitech-hidpp: Add scope to battery

[Jiri Kosina]

On Mon, 27 Mar 2017, Benjamin Tissoires wrote:

> From: Bastien Nocera <hadess@hadess.net>
> 
> Without a scope defined, UPower assumes that the battery provides
> 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>
> 
> ---
> 
> no changes in v3
> 
> changes in v2:
> * fixed typo in commit message
> ---
>  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,

This certainly assumes some other patchset to be already applied, right?

Which tree is this patchset based on, please?

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Benjamin Tissoires]

On Apr 06 2017 or thereabouts, Jiri Kosina wrote:
> On Mon, 27 Mar 2017, Benjamin Tissoires wrote:
> 
> > From: Bastien Nocera <hadess@hadess.net>
> > 
> > Without a scope defined, UPower assumes that the battery provides
> > 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>
> > 
> > ---
> > 
> > no changes in v3
> > 
> > changes in v2:
> > * fixed typo in commit message
> > ---
> >  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,
> 
> This certainly assumes some other patchset to be already applied, right?

Hehe, once again you forgot about it, and me to make a note about it
too ;-) 

> 
> Which tree is this patchset based on, please?

Your for-4.8/logitech-hidpp-battery branch :)

Cheers,
Benjamin

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

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

[Jiri Kosina]

On Thu, 6 Apr 2017, Benjamin Tissoires wrote:

> > > Without a scope defined, UPower assumes that the battery provides
> > > 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>
> > > 
> > > ---
> > > 
> > > no changes in v3
> > > 
> > > changes in v2:
> > > * fixed typo in commit message
> > > ---
> > >  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,
> > 
> > This certainly assumes some other patchset to be already applied, right?
> 
> Hehe, once again you forgot about it, and me to make a note about it
> too ;-) 

Bah, right; I was pretty sure we've discussed this, but was not able to 
find in anywhere in the history :)

Now queued in for-4.12/logitech-hidpp-battery-power-supply. Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH v3 03/19] 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>

---

no changes in v3

no changes in v2
---
 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 v3 04/19] 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>

---

changes in v3:
- moved up in the series

no changes in v2
---
 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 1cda29e..4031405 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2309,6 +2309,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 v3 05/19] HID: logitech-hidpp: create a capabilities bits field

[Benjamin Tissoires]

Do not pollute the quirks bits field which is public API
with elements that are queried from the device.

Move the 2 battery capabilities into the new field.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

new in v3
---
 drivers/hid/hid-logitech-hidpp.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4031405..4ee466c 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -62,11 +62,12 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS	BIT(22)
 #define HIDPP_QUIRK_NO_HIDINPUT			BIT(23)
 #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_DELAYED_INIT		HIDPP_QUIRK_NO_HIDINPUT
 
+#define HIDPP_CAPABILITY_HIDPP10_BATTERY	BIT(0)
+#define HIDPP_CAPABILITY_HIDPP20_BATTERY	BIT(1)
+
 /*
  * There are two hidpp protocols in use, the first version hidpp10 is known
  * as register access protocol or RAP, the second version hidpp20 is known as
@@ -138,6 +139,7 @@ struct hidpp_device {
 	struct input_dev *delayed_input;
 
 	unsigned long quirks;
+	unsigned long capabilities;
 
 	struct hidpp_battery battery;
 };
@@ -834,7 +836,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 	if (hidpp->protocol_major >= 2) {
 		ret = hidpp20_initialize_battery(hidpp);
 		if (ret == 0)
-			hidpp->quirks |= HIDPP_QUIRK_HIDPP20_BATTERY;
+			hidpp->capabilities |= HIDPP_CAPABILITY_HIDPP20_BATTERY;
 	}
 
 	return ret;
@@ -2283,7 +2285,7 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 	if (ret != 0)
 		return ret;
 
-	if (hidpp->quirks & HIDPP_QUIRK_HIDPP20_BATTERY) {
+	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
 		ret = hidpp20_battery_event(hidpp, data, size);
 		if (ret != 0)
 			return ret;
-- 
2.9.3

[PATCH v3 06/19] 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>

---

changes in v3:
- moved up in the series

changes in v2:
* only overwrite the name when the input is delayed (was causing different
  name depending of the connect stat of the device while probing)
---
 drivers/hid/hid-logitech-hidpp.c | 86 +++++++++++++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 19 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4ee466c..db15cfc 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -62,6 +62,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS	BIT(22)
 #define HIDPP_QUIRK_NO_HIDINPUT			BIT(23)
 #define HIDPP_QUIRK_FORCE_OUTPUT_REPORTS	BIT(24)
+#define HIDPP_QUIRK_UNIFYING			BIT(25)
 
 #define HIDPP_QUIRK_DELAYED_INIT		HIDPP_QUIRK_NO_HIDINPUT
 
@@ -394,14 +395,14 @@ static void hidpp_prefix_name(char **name, int name_length)
 #define HIDPP_GET_LONG_REGISTER				0x83
 
 #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;
 
@@ -430,6 +431,54 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
 	return name;
 }
 
+static int hidpp_unifying_get_serial(struct hidpp_device *hidpp, u32 *serial)
+{
+	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 ret;
+
+	/*
+	 * We don't care about LE or BE, we will output it as a string
+	 * with %4phD, so we need to keep the order.
+	 */
+	*serial = *((u32 *)&response.rap.params[1]);
+	return 0;
+}
+
+static int hidpp_unifying_init(struct hidpp_device *hidpp)
+{
+	struct hid_device *hdev = hidpp->hid_dev;
+	const char *name;
+	u32 serial;
+	int ret;
+
+	ret = hidpp_unifying_get_serial(hidpp, &serial);
+	if (ret)
+		return ret;
+
+	snprintf(hdev->uniq, sizeof(hdev->uniq), "%04x-%4phD",
+		 hdev->product, &serial);
+	dbg_hid("HID++ Unifying: Got serial: %s\n", hdev->uniq);
+
+	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);
+
+	kfree(name);
+	return 0;
+}
+
 /* -------------------------------------------------------------------------- */
 /* 0x0000: Root                                                               */
 /* -------------------------------------------------------------------------- */
@@ -2299,22 +2348,15 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 	return 0;
 }
 
-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");
@@ -2456,6 +2498,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;
@@ -2507,8 +2552,12 @@ 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) {
+	atomic_set(&hidpp->connected, connected);
+	if (!(hidpp->quirks & HIDPP_QUIRK_UNIFYING)) {
 		if (!connected) {
 			ret = -ENODEV;
 			hid_err(hdev, "Device not connected");
@@ -2517,10 +2566,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 		hid_info(hdev, "HID++ %u.%u device connected.\n",
 			 hidpp->protocol_major, hidpp->protocol_minor);
-	}
 
-	hidpp_overwrite_name(hdev, id->group == HID_GROUP_LOGITECH_DJ_DEVICE);
-	atomic_set(&hidpp->connected, connected);
+		hidpp_overwrite_name(hdev);
+	}
 
 	if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
 		ret = wtp_get_config(hidpp);
-- 
2.9.3

[PATCH v3 07/19] HID: logitech-hidpp: retrieve the HID++ device name when available

[Benjamin Tissoires]

hidpp->name can't be null.
Only HID++ 2.0 and above device supports the query.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

changes in v3:
- moved up in the series

new in v2
---
 drivers/hid/hid-logitech-hidpp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index db15cfc..b0d2fea 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2443,13 +2443,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 			 hidpp->protocol_major, hidpp->protocol_minor);
 	}
 
-	hidpp_initialize_battery(hidpp);
-
-	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT))
-		/* if HID created the input nodes for us, we can stop now */
-		return;
-
-	if (!hidpp->name || hidpp->name == hdev->name) {
+	if (hidpp->name == hdev->name && hidpp->protocol_major >= 2) {
 		name = hidpp_get_device_name(hidpp);
 		if (!name) {
 			hid_err(hdev,
@@ -2465,6 +2459,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 		hidpp->name = devm_name;
 	}
 
+	hidpp_initialize_battery(hidpp);
+
+	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT))
+		/* if HID created the input nodes for us, we can stop now */
+		return;
+
 	input = hidpp_allocate_input(hdev);
 	if (!input) {
 		hid_err(hdev, "cannot allocate new input device: %d\n", ret);
-- 
2.9.3

[PATCH v3 08/19] HID: logitech-hidpp: rework hidpp_connect_event()

[Benjamin Tissoires]

Looks like all users don't care about a disconnect.
Simplify the various variant_connect() and put the connect state check
at the beginning.

For delayed input devices, make sure we go through all other connect
values (protocol, battery) before bailing out.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

changes in v3:
- moved up in the series

new in v2
---
 drivers/hid/hid-logitech-hidpp.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index b0d2fea..81ebded 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1884,9 +1884,6 @@ static int wtp_connect(struct hid_device *hdev, bool connected)
 	struct wtp_data *wd = hidpp->private_data;
 	int ret;
 
-	if (!connected)
-		return 0;
-
 	if (!wd->x_size) {
 		ret = wtp_get_config(hidpp);
 		if (ret) {
@@ -1954,9 +1951,6 @@ static int m560_send_config_command(struct hid_device *hdev, bool connected)
 
 	hidpp_dev = hid_get_drvdata(hdev);
 
-	if (!connected)
-		return -ENODEV;
-
 	return hidpp_send_rap_command_sync(
 		hidpp_dev,
 		REPORT_ID_HIDPP_SHORT,
@@ -2160,9 +2154,6 @@ static int k400_connect(struct hid_device *hdev, bool connected)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 
-	if (!connected)
-		return 0;
-
 	if (!disable_tap_to_click)
 		return 0;
 
@@ -2414,6 +2405,9 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 	struct input_dev *input;
 	char *name, *devm_name;
 
+	if (!connected)
+		return;
+
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
 		ret = wtp_connect(hdev, connected);
 		if (ret)
@@ -2428,9 +2422,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 			return;
 	}
 
-	if (!connected || hidpp->delayed_input)
-		return;
-
 	/* the device is already connected, we can ask for its name and
 	 * protocol */
 	if (!hidpp->protocol_major) {
@@ -2461,8 +2452,8 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 
 	hidpp_initialize_battery(hidpp);
 
-	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT))
-		/* if HID created the input nodes for us, we can stop now */
+	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) || hidpp->delayed_input)
+		/* if the input nodes are already created, we can stop now */
 		return;
 
 	input = hidpp_allocate_input(hdev);
-- 
2.9.3

[PATCH v3 09/19] HID: logitech-hidpp: handle battery events in hidpp_raw_hidpp_event()

[Benjamin Tissoires]

Battery events are reported through HID++, so we need to be sure
the report ID is the HID++ one.

Without this, we might receive keyboard events that looks just like
battery events with wrong data and which will confuse user space.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

new in v3
---
 drivers/hid/hid-logitech-hidpp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 81ebded..3e2f716 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2250,6 +2250,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 	struct hidpp_report *question = hidpp->send_receive_buf;
 	struct hidpp_report *answer = hidpp->send_receive_buf;
 	struct hidpp_report *report = (struct hidpp_report *)data;
+	int ret;
 
 	/*
 	 * If the mutex is locked then we have a pending answer from a
@@ -2283,6 +2284,12 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 		return 1;
 	}
 
+	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
+		ret = hidpp20_battery_event(hidpp, data, size);
+		if (ret != 0)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -2325,12 +2332,6 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 	if (ret != 0)
 		return ret;
 
-	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
-		ret = hidpp20_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)
-- 
2.9.3

[PATCH v3 10/19] 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>

---

changes in v3:
- moved up in the series

changes in v2:
* model stripped of Logitech
* vendor Logitech -> Logitech, Inc.
* use hidpp->name for the power_supply name (so that it can be overloaded)
---
 drivers/hid/hid-logitech-hidpp.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3e2f716..421c374 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -813,6 +813,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,
@@ -832,6 +835,18 @@ 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_MODEL_NAME:
+			if (!strncmp(hidpp->name, "Logitech ", 9))
+				val->strval = hidpp->name + 9;
+			else
+				val->strval = hidpp->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 v3 11/19] 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>

---

changes in v3:
- moved up in the series

no changes in v2
---
 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 421c374..9a9771a 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -855,57 +855,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->capabilities |= HIDPP_CAPABILITY_HIDPP20_BATTERY;
-	}
-
-	return ret;
-}
-
 /* -------------------------------------------------------------------------- */
 /* 0x6010: Touchpad FW items                                                  */
 /* -------------------------------------------------------------------------- */
@@ -2355,6 +2304,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->capabilities |= HIDPP_CAPABILITY_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)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
-- 
2.9.3

[PATCH v3 12/19] HID: logitech-hidpp: return an error if the queried feature is not present

[Benjamin Tissoires]

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

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

changes in v3:
- moved up in the series

no changes in v2
---
 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 9a9771a..22129dd 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -503,6 +503,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 v3 13/19] 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>

---

changes in v3:
- moved up in the series

no changes in v2
---
 drivers/hid/hid-logitech-hidpp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 22129dd..0781d2b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2463,6 +2463,13 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 
 	hidpp_initialize_battery(hidpp);
 
+	/* forward current battery state */
+	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
+		hidpp20_query_battery_info(hidpp);
+		if (hidpp->battery.ps)
+			power_supply_changed(hidpp->battery.ps);
+	}
+
 	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) || hidpp->delayed_input)
 		/* if the input nodes are already created, we can stop now */
 		return;
-- 
2.9.3

[PATCH v3 14/19] HID: logitech-hidpp: battery: provide ONLINE property

[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>

---

completely reworked in v3:
- store a online field in hidpp->battery to be able to fine control
  the value
- provide POWER_SUPPLY_STATUS_UNKNOWN to the status when the device
  is disconnected
---
 drivers/hid/hid-logitech-hidpp.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0781d2b..7e4445a 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -120,6 +120,7 @@ struct hidpp_battery {
 	char name[64];
 	int status;
 	int level;
+	bool online;
 };
 
 struct hidpp_device {
@@ -686,7 +687,6 @@ static int hidpp20_batterylevel_map_status_level(u8 data[3], int *level,
 						 int *next_level)
 {
 	int status;
-	int level_override;
 
 	*level = data[0];
 	*next_level = data[1];
@@ -698,36 +698,28 @@ static int hidpp20_batterylevel_map_status_level(u8 data[3], int *level,
 	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;
 }
 
@@ -781,6 +773,9 @@ static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
 
 	hidpp->battery.status = status;
 	hidpp->battery.level = level;
+	/* the capacity is only available when discharging or full */
+	hidpp->battery.online = status == POWER_SUPPLY_STATUS_DISCHARGING ||
+				status == POWER_SUPPLY_STATUS_FULL;
 
 	return 0;
 }
@@ -799,6 +794,10 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
 	status = hidpp20_batterylevel_map_status_level(report->fap.params,
 						       &level, &next_level);
 
+	/* the capacity is only available when discharging or full */
+	hidpp->battery.online = status == POWER_SUPPLY_STATUS_DISCHARGING ||
+				status == POWER_SUPPLY_STATUS_FULL;
+
 	changed = level != hidpp->battery.level ||
 		  status != hidpp->battery.status;
 
@@ -813,6 +812,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,
@@ -838,6 +838,9 @@ 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.online;
+			break;
 		case POWER_SUPPLY_PROP_MODEL_NAME:
 			if (!strncmp(hidpp->name, "Logitech ", 9))
 				val->strval = hidpp->name + 9;
@@ -2416,8 +2419,14 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 	struct input_dev *input;
 	char *name, *devm_name;
 
-	if (!connected)
+	if (!connected) {
+		if (hidpp->battery.ps) {
+			hidpp->battery.online = false;
+			hidpp->battery.status = POWER_SUPPLY_STATUS_UNKNOWN;
+			power_supply_changed(hidpp->battery.ps);
+		}
 		return;
+	}
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
 		ret = wtp_connect(hdev, connected);
-- 
2.9.3

[PATCH v3 15/19] HID: logitech-hidpp: rename battery level into capacity

[Benjamin Tissoires]

The power_supply term for the percentage is capacity. Capacity level
can be given when non accurate mileage is provided by the device, so
better stick to the terms used in power_supply.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

new in v3
---
 drivers/hid/hid-logitech-hidpp.c | 55 ++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 7e4445a..c59f7e5e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -119,7 +119,7 @@ struct hidpp_battery {
 	struct power_supply *ps;
 	char name[64];
 	int status;
-	int level;
+	int capacity;
 	bool online;
 };
 
@@ -683,17 +683,16 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
 
 #define EVENT_BATTERY_LEVEL_STATUS_BROADCAST			0x00
 
-static int hidpp20_batterylevel_map_status_level(u8 data[3], int *level,
-						 int *next_level)
+static int hidpp20_batterylevel_map_status_capacity(u8 data[3], int *capacity,
+						    int *next_capacity)
 {
 	int status;
 
-	*level = data[0];
-	*next_level = data[1];
+	*capacity = data[0];
+	*next_capacity = 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
+	/* When discharging, we can rely on the device reported capacity.
+	 * For all other states the device reports 0 (unknown).
 	 */
 	switch (data[2]) {
 		case 0: /* discharging (in use) */
@@ -707,7 +706,7 @@ static int hidpp20_batterylevel_map_status_level(u8 data[3], int *level,
 			break;
 		case 3: /* charge complete */
 			status = POWER_SUPPLY_STATUS_FULL;
-			*level = 100;
+			*capacity = 100;
 			break;
 		case 4: /* recharging below optimal speed */
 			status = POWER_SUPPLY_STATUS_CHARGING;
@@ -723,11 +722,11 @@ static int hidpp20_batterylevel_map_status_level(u8 data[3], int *level,
 	return status;
 }
 
-static int hidpp20_batterylevel_get_battery_level(struct hidpp_device *hidpp,
-						  u8 feature_index,
-						  int *status,
-						  int *level,
-						  int *next_level)
+static int hidpp20_batterylevel_get_battery_capacity(struct hidpp_device *hidpp,
+						     u8 feature_index,
+						     int *status,
+						     int *capacity,
+						     int *next_capacity)
 {
 	struct hidpp_report response;
 	int ret;
@@ -744,8 +743,8 @@ static int hidpp20_batterylevel_get_battery_level(struct hidpp_device *hidpp,
 	if (ret)
 		return ret;
 
-	*status = hidpp20_batterylevel_map_status_level(params, level,
-							next_level);
+	*status = hidpp20_batterylevel_map_status_capacity(params, capacity,
+							   next_capacity);
 
 	return 0;
 }
@@ -754,7 +753,7 @@ static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
 {
 	u8 feature_type;
 	int ret;
-	int status, level, next_level;
+	int status, capacity, next_capacity;
 
 	if (hidpp->battery.feature_index == 0) {
 		ret = hidpp_root_get_feature(hidpp,
@@ -765,14 +764,15 @@ static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
 			return ret;
 	}
 
-	ret = hidpp20_batterylevel_get_battery_level(hidpp,
-						     hidpp->battery.feature_index,
-						     &status, &level, &next_level);
+	ret = hidpp20_batterylevel_get_battery_capacity(hidpp,
+						hidpp->battery.feature_index,
+						&status, &capacity,
+						&next_capacity);
 	if (ret)
 		return ret;
 
 	hidpp->battery.status = status;
-	hidpp->battery.level = level;
+	hidpp->battery.capacity = capacity;
 	/* the capacity is only available when discharging or full */
 	hidpp->battery.online = status == POWER_SUPPLY_STATUS_DISCHARGING ||
 				status == POWER_SUPPLY_STATUS_FULL;
@@ -784,25 +784,26 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
 				 u8 *data, int size)
 {
 	struct hidpp_report *report = (struct hidpp_report *)data;
-	int status, level, next_level;
+	int status, capacity, next_capacity;
 	bool changed;
 
 	if (report->fap.feature_index != hidpp->battery.feature_index ||
 	    report->fap.funcindex_clientid != EVENT_BATTERY_LEVEL_STATUS_BROADCAST)
 		return 0;
 
-	status = hidpp20_batterylevel_map_status_level(report->fap.params,
-						       &level, &next_level);
+	status = hidpp20_batterylevel_map_status_capacity(report->fap.params,
+							  &capacity,
+							  &next_capacity);
 
 	/* the capacity is only available when discharging or full */
 	hidpp->battery.online = status == POWER_SUPPLY_STATUS_DISCHARGING ||
 				status == POWER_SUPPLY_STATUS_FULL;
 
-	changed = level != hidpp->battery.level ||
+	changed = capacity != hidpp->battery.capacity ||
 		  status != hidpp->battery.status;
 
 	if (changed) {
-		hidpp->battery.level = level;
+		hidpp->battery.capacity = capacity;
 		hidpp->battery.status = status;
 		if (hidpp->battery.ps)
 			power_supply_changed(hidpp->battery.ps);
@@ -833,7 +834,7 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 			val->intval = hidpp->battery.status;
 			break;
 		case POWER_SUPPLY_PROP_CAPACITY:
-			val->intval = hidpp->battery.level;
+			val->intval = hidpp->battery.capacity;
 			break;
 		case POWER_SUPPLY_PROP_SCOPE:
 			val->intval = POWER_SUPPLY_SCOPE_DEVICE;
-- 
2.9.3

[PATCH v3 16/19] HID: logitech-hidpp: battery: provide CAPACITY_LEVEL

[Benjamin Tissoires]

CAPACITY LEVEL allows to forward rough information on the battery mileage.
HID++ 2.0 devices will either report percentage or levels, so better
forwarding this information to the user space.

The M325 supports only 2 levels: 'Full' and 'Critical'. With mileage,
it will report either 90% or 5%, which might confuse users. With this
change the battery will either report "Full" or "Critical".

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

new in v3
---
 drivers/hid/hid-logitech-hidpp.c | 104 +++++++++++++++++++++++++++++++++++----
 1 file changed, 94 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index c59f7e5e..d7dc458 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -68,6 +68,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
 
 #define HIDPP_CAPABILITY_HIDPP10_BATTERY	BIT(0)
 #define HIDPP_CAPABILITY_HIDPP20_BATTERY	BIT(1)
+#define HIDPP_CAPABILITY_BATTERY_MILEAGE	BIT(2)
+#define HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS	BIT(3)
 
 /*
  * There are two hidpp protocols in use, the first version hidpp10 is known
@@ -120,6 +122,7 @@ struct hidpp_battery {
 	char name[64];
 	int status;
 	int capacity;
+	int level;
 	bool online;
 };
 
@@ -683,13 +686,30 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
 
 #define EVENT_BATTERY_LEVEL_STATUS_BROADCAST			0x00
 
+#define FLAG_BATTERY_LEVEL_DISABLE_OSD				BIT(0)
+#define FLAG_BATTERY_LEVEL_MILEAGE				BIT(1)
+#define FLAG_BATTERY_LEVEL_RECHARGEABLE				BIT(2)
+
+static int hidpp_map_battery_level(int capacity)
+{
+	if (capacity < 11)
+		return POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
+	else if (capacity < 31)
+		return POWER_SUPPLY_CAPACITY_LEVEL_LOW;
+	else if (capacity < 81)
+		return POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
+	return POWER_SUPPLY_CAPACITY_LEVEL_FULL;
+}
+
 static int hidpp20_batterylevel_map_status_capacity(u8 data[3], int *capacity,
-						    int *next_capacity)
+						    int *next_capacity,
+						    int *level)
 {
 	int status;
 
 	*capacity = data[0];
 	*next_capacity = data[1];
+	*level = POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN;
 
 	/* When discharging, we can rely on the device reported capacity.
 	 * For all other states the device reports 0 (unknown).
@@ -697,6 +717,7 @@ static int hidpp20_batterylevel_map_status_capacity(u8 data[3], int *capacity,
 	switch (data[2]) {
 		case 0: /* discharging (in use) */
 			status = POWER_SUPPLY_STATUS_DISCHARGING;
+			*level = hidpp_map_battery_level(*capacity);
 			break;
 		case 1: /* recharging */
 			status = POWER_SUPPLY_STATUS_CHARGING;
@@ -706,6 +727,7 @@ static int hidpp20_batterylevel_map_status_capacity(u8 data[3], int *capacity,
 			break;
 		case 3: /* charge complete */
 			status = POWER_SUPPLY_STATUS_FULL;
+			*level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
 			*capacity = 100;
 			break;
 		case 4: /* recharging below optimal speed */
@@ -726,7 +748,8 @@ static int hidpp20_batterylevel_get_battery_capacity(struct hidpp_device *hidpp,
 						     u8 feature_index,
 						     int *status,
 						     int *capacity,
-						     int *next_capacity)
+						     int *next_capacity,
+						     int *level)
 {
 	struct hidpp_report response;
 	int ret;
@@ -744,7 +767,38 @@ static int hidpp20_batterylevel_get_battery_capacity(struct hidpp_device *hidpp,
 		return ret;
 
 	*status = hidpp20_batterylevel_map_status_capacity(params, capacity,
-							   next_capacity);
+							   next_capacity,
+							   level);
+
+	return 0;
+}
+
+static int hidpp20_batterylevel_get_battery_info(struct hidpp_device *hidpp,
+						  u8 feature_index)
+{
+	struct hidpp_report response;
+	int ret;
+	u8 *params = (u8 *)response.fap.params;
+	unsigned int level_count, flags;
+
+	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
+					  CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_CAPABILITY,
+					  NULL, 0, &response);
+	if (ret > 0) {
+		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+			__func__, ret);
+		return -EPROTO;
+	}
+	if (ret)
+		return ret;
+
+	level_count = params[0];
+	flags = params[1];
+
+	if (level_count < 10 || !(flags & FLAG_BATTERY_LEVEL_MILEAGE))
+		hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS;
+	else
+		hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_MILEAGE;
 
 	return 0;
 }
@@ -753,7 +807,7 @@ static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
 {
 	u8 feature_type;
 	int ret;
-	int status, capacity, next_capacity;
+	int status, capacity, next_capacity, level;
 
 	if (hidpp->battery.feature_index == 0) {
 		ret = hidpp_root_get_feature(hidpp,
@@ -767,12 +821,18 @@ static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
 	ret = hidpp20_batterylevel_get_battery_capacity(hidpp,
 						hidpp->battery.feature_index,
 						&status, &capacity,
-						&next_capacity);
+						&next_capacity, &level);
+	if (ret)
+		return ret;
+
+	ret = hidpp20_batterylevel_get_battery_info(hidpp,
+						hidpp->battery.feature_index);
 	if (ret)
 		return ret;
 
 	hidpp->battery.status = status;
 	hidpp->battery.capacity = capacity;
+	hidpp->battery.level = level;
 	/* the capacity is only available when discharging or full */
 	hidpp->battery.online = status == POWER_SUPPLY_STATUS_DISCHARGING ||
 				status == POWER_SUPPLY_STATUS_FULL;
@@ -784,7 +844,7 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
 				 u8 *data, int size)
 {
 	struct hidpp_report *report = (struct hidpp_report *)data;
-	int status, capacity, next_capacity;
+	int status, capacity, next_capacity, level;
 	bool changed;
 
 	if (report->fap.feature_index != hidpp->battery.feature_index ||
@@ -793,16 +853,19 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
 
 	status = hidpp20_batterylevel_map_status_capacity(report->fap.params,
 							  &capacity,
-							  &next_capacity);
+							  &next_capacity,
+							  &level);
 
 	/* the capacity is only available when discharging or full */
 	hidpp->battery.online = status == POWER_SUPPLY_STATUS_DISCHARGING ||
 				status == POWER_SUPPLY_STATUS_FULL;
 
 	changed = capacity != hidpp->battery.capacity ||
+		  level != hidpp->battery.level ||
 		  status != hidpp->battery.status;
 
 	if (changed) {
+		hidpp->battery.level = level;
 		hidpp->battery.capacity = capacity;
 		hidpp->battery.status = status;
 		if (hidpp->battery.ps)
@@ -815,11 +878,12 @@ 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,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+	0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY, */
+	0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY_LEVEL, */
 };
 
 static int hidpp_battery_get_property(struct power_supply *psy,
@@ -836,6 +900,9 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 		case POWER_SUPPLY_PROP_CAPACITY:
 			val->intval = hidpp->battery.capacity;
 			break;
+		case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
+			val->intval = hidpp->battery.level;
+			break;
 		case POWER_SUPPLY_PROP_SCOPE:
 			val->intval = POWER_SUPPLY_SCOPE_DEVICE;
 			break;
@@ -2316,7 +2383,9 @@ 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;
+	enum power_supply_property *battery_props;
 	struct hidpp_battery *battery;
+	unsigned int num_battery_props;
 	unsigned long n;
 	int ret;
 
@@ -2332,11 +2401,25 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 		return -ENOENT;
 	}
 
+	battery_props = devm_kmemdup(&hidpp->hid_dev->dev,
+				     hidpp_battery_props,
+				     sizeof(hidpp_battery_props),
+				     GFP_KERNEL);
+	num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 2;
+
+	if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE)
+		battery_props[num_battery_props++] =
+				POWER_SUPPLY_PROP_CAPACITY;
+
+	if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS)
+		battery_props[num_battery_props++] =
+				POWER_SUPPLY_PROP_CAPACITY_LEVEL;
+
 	battery = &hidpp->battery;
 
 	n = atomic_inc_return(&battery_no) - 1;
-	desc->properties = hidpp_battery_props;
-	desc->num_properties = ARRAY_SIZE(hidpp_battery_props);
+	desc->properties = battery_props;
+	desc->num_properties = num_battery_props;
 	desc->get_property = hidpp_battery_get_property;
 	sprintf(battery->name, "hidpp_battery_%ld", n);
 	desc->name = battery->name;
@@ -2424,6 +2507,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 		if (hidpp->battery.ps) {
 			hidpp->battery.online = false;
 			hidpp->battery.status = POWER_SUPPLY_STATUS_UNKNOWN;
+			hidpp->battery.level = POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN;
 			power_supply_changed(hidpp->battery.ps);
 		}
 		return;
-- 
2.9.3

[PATCH v3 17/19] 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>

---

changes in v3:
- fixed online status

changes in v2:
* update state according to lux information
* do not update Lux if the event does not contain lux information
---
 drivers/hid/hid-logitech-hidpp.c | 115 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 113 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index d7dc458..bb40e5d 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 */
@@ -117,6 +118,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];
@@ -809,7 +811,7 @@ static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
 	int ret;
 	int status, capacity, next_capacity, 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,
@@ -930,6 +932,101 @@ 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] = { 1, 1 };
+	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;
+
+	hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_MILEAGE;
+
+	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 capacity, lux, status;
+	u8 function;
+
+	function = report->fap.funcindex_clientid;
+
+
+	if (report->fap.feature_index != hidpp->battery.solar_feature_index ||
+	    !(function == EVENT_SOLAR_BATTERY_BROADCAST ||
+	      function == EVENT_SOLAR_BATTERY_LIGHT_MEASURE ||
+	      function == EVENT_SOLAR_CHECK_LIGHT_BUTTON))
+		return 0;
+
+	capacity = report->fap.params[0];
+
+	switch (function) {
+	case EVENT_SOLAR_BATTERY_LIGHT_MEASURE:
+		lux = (report->fap.params[1] << 8) | report->fap.params[2];
+		if (lux > 200)
+			status = POWER_SUPPLY_STATUS_CHARGING;
+		else
+			status = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+	case EVENT_SOLAR_CHECK_LIGHT_BUTTON:
+	default:
+		if (capacity < hidpp->battery.capacity)
+			status = POWER_SUPPLY_STATUS_DISCHARGING;
+		else
+			status = POWER_SUPPLY_STATUS_CHARGING;
+
+	}
+
+	if (capacity == 100)
+		status = POWER_SUPPLY_STATUS_FULL;
+
+	hidpp->battery.online = true;
+	if (capacity != hidpp->battery.capacity ||
+	    status != hidpp->battery.status) {
+		hidpp->battery.capacity = capacity;
+		hidpp->battery.status = status;
+		if (hidpp->battery.ps)
+			power_supply_changed(hidpp->battery.ps);
+	}
+
+	return 0;
+}
+
+/* -------------------------------------------------------------------------- */
 /* 0x6010: Touchpad FW items                                                  */
 /* -------------------------------------------------------------------------- */
 
@@ -2326,6 +2423,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 		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;
 	}
 
 	return 0;
@@ -2392,8 +2492,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->capabilities |= HIDPP_CAPABILITY_HIDPP20_BATTERY;
@@ -2751,6 +2858,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 v3 18/19] 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>

---

changes in v3:
- differentiate between level and mileage
- better online reporting

no changes in v2
---
 drivers/hid/hid-logitech-hidpp.c | 234 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 230 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index bb40e5d..eb4339e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -400,6 +400,211 @@ 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 = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
+		break;
+	case 3 ... 4:
+		level = POWER_SUPPLY_CAPACITY_LEVEL_LOW;
+		break;
+	case 5 ... 6:
+		level = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
+		break;
+	case 7:
+		level = POWER_SUPPLY_CAPACITY_LEVEL_HIGH;
+		break;
+	default:
+		level = POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN;
+	}
+
+	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;
+	case 0x20: /* unknown */
+		status = POWER_SUPPLY_STATUS_UNKNOWN;
+		break;
+	/*
+	 * 0x01...0x1F = reserved (not charging)
+	 * 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, status;
+
+	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]);
+	status = hidpp10_battery_status_map_status(response.rap.params[1]);
+	hidpp->battery.status = status;
+	/* the capacity is only available when discharging or full */
+	hidpp->battery.online = status == POWER_SUPPLY_STATUS_DISCHARGING ||
+				status == POWER_SUPPLY_STATUS_FULL;
+
+	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, status;
+
+	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.capacity = response.rap.params[0];
+	status = hidpp10_battery_mileage_map_status(response.rap.params[2]);
+	hidpp->battery.status = status;
+	/* the capacity is only available when discharging or full */
+	hidpp->battery.online = status == POWER_SUPPLY_STATUS_DISCHARGING ||
+				status == POWER_SUPPLY_STATUS_FULL;
+
+	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, capacity, level;
+	bool changed;
+
+	if (report->report_id != REPORT_ID_HIDPP_SHORT)
+		return 0;
+
+	switch (report->rap.sub_id) {
+	case HIDPP_REG_BATTERY_STATUS:
+		capacity = hidpp->battery.capacity;
+		level = hidpp10_battery_status_map_level(report->rawbytes[1]);
+		status = hidpp10_battery_status_map_status(report->rawbytes[2]);
+		break;
+	case HIDPP_REG_BATTERY_MILEAGE:
+		capacity = report->rap.params[0];
+		level = hidpp->battery.level;
+		status = hidpp10_battery_mileage_map_status(report->rawbytes[3]);
+		break;
+	default:
+		return 0;
+	}
+
+	changed = capacity != hidpp->battery.capacity ||
+		  level != hidpp->battery.level ||
+		  status != hidpp->battery.status;
+
+	/* the capacity is only available when discharging or full */
+	hidpp->battery.online = status == POWER_SUPPLY_STATUS_DISCHARGING ||
+				status == POWER_SUPPLY_STATUS_FULL;
+
+	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 HIDPP_EXTENDED_PAIRING				0x30
 #define HIDPP_DEVICE_NAME				0x40
@@ -2428,6 +2633,12 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 			return ret;
 	}
 
+	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
+		ret = hidpp10_battery_event(hidpp, data, size);
+		if (ret != 0)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -2505,7 +2716,16 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 			return ret;
 		hidpp->capabilities |= HIDPP_CAPABILITY_HIDPP20_BATTERY;
 	} else {
-		return -ENOENT;
+		ret = hidpp10_query_battery_status(hidpp);
+		if (ret) {
+			ret = hidpp10_query_battery_mileage(hidpp);
+			if (ret)
+				return -ENOENT;
+			hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_MILEAGE;
+		} else {
+			hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS;
+		}
+		hidpp->capabilities |= HIDPP_CAPABILITY_HIDPP10_BATTERY;
 	}
 
 	battery_props = devm_kmemdup(&hidpp->hid_dev->dev,
@@ -2665,11 +2885,17 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 	hidpp_initialize_battery(hidpp);
 
 	/* forward current battery state */
-	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
+	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
+		hidpp10_enable_battery_reporting(hidpp);
+		if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE)
+			hidpp10_query_battery_mileage(hidpp);
+		else
+			hidpp10_query_battery_status(hidpp);
+	} else if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
 		hidpp20_query_battery_info(hidpp);
-		if (hidpp->battery.ps)
-			power_supply_changed(hidpp->battery.ps);
 	}
+	if (hidpp->battery.ps)
+		power_supply_changed(hidpp->battery.ps);
 
 	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) || hidpp->delayed_input)
 		/* if the input nodes are already created, we can stop now */
-- 
2.9.3

[PATCH v3 19/19] 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>

---

no changes in v3

no changes in v2
---
 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 eb4339e..41b3946 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2916,6 +2916,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;
@@ -2960,6 +2971,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__);
@@ -3042,6 +3059,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:
@@ -3053,6 +3071,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

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Bastien Nocera]

On Mon, 2017-03-27 at 16:59 +0200, Benjamin Tissoires wrote:
> Hi,
> 
> this is finally a rework of the series that provides kernel
> power_supply
> for hidpp devices.
> 
> This will allow upower to not handle those devices anymore and to
> have more
> immediate reportng of the device to the system.

You can add my Tested-by to all the commits in this patchset, using a
K750 solar keyboard and a T650 touchpad.

Note that user-space support work is still ongoing, on top of the
current upower master:
https://bugs.freedesktop.org/show_bug.cgi?id=100359

Cheers

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Dave Hansen]

On 03/27/2017 07:59 AM, Benjamin Tissoires wrote:
> this is finally a rework of the series that provides kernel power_supply
> for hidpp devices.
> 
> This will allow upower to not handle those devices anymore and to have more
> immediate reportng of the device to the system.

FWIW, I'm on Ubuntu 14.04, and upower *is* reporting my mouse battery as
if it were a laptop battery.  It's mostly garbage, and always reports
0%, which makes upower always tell me my laptop is 2/3 charged (I have 2
real batteries).

Is this expected?

> $ upower --dump
> Device: /org/freedesktop/UPower/devices/battery_hidpp_battery_0
>   native-path:          hidpp_battery_0
>   vendor:               Logitech
>   model:                Performance MX
>   serial:               101a-92-ca-86-1e
>   power supply:         no
>   updated:              Thu 01 Jun 2017 11:04:02 AM PDT (22 seconds ago)
>   has history:          yes
>   has statistics:       yes
>   battery
>     present:             yes
>     rechargeable:        yes
>     state:               discharging
>     energy:              0 Wh
>     energy-empty:        0 Wh
>     energy-full:         0 Wh
>     energy-full-design:  0 Wh
>     energy-rate:         0 W
>     percentage:          0%
>     capacity:            100%
>   History (charge):
>     1496340177	0.000	unknown
>   History (rate):
>     1496340177	0.000	unknown

$ upower --version
UPower client version 0.9.23
UPower daemon version 0.9.23

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Bastien Nocera]

On Thu, 2017-06-01 at 11:06 -0700, Dave Hansen wrote:
> On 03/27/2017 07:59 AM, Benjamin Tissoires wrote:
> > this is finally a rework of the series that provides kernel
> > power_supply
> > for hidpp devices.
> > 
> > This will allow upower to not handle those devices anymore and to
> > have more
> > immediate reportng of the device to the system.
> 
> FWIW, I'm on Ubuntu 14.04, and upower *is* reporting my mouse battery
> as
> if it were a laptop battery.  It's mostly garbage, and always reports
> 0%, which makes upower always tell me my laptop is 2/3 charged (I
> have 2
> real batteries).
> 
> Is this expected?

You need to either disable the feature for the kernel, or upgrade to
the latest git master of UPower. Mixing both won't work. Use the devkit
mailing-list if you need a release of UPower.

Cheers

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Benjamin Tissoires]

On Jun 01 2017 or thereabouts, Bastien Nocera wrote:
> On Thu, 2017-06-01 at 11:06 -0700, Dave Hansen wrote:
> > On 03/27/2017 07:59 AM, Benjamin Tissoires wrote:
> > > this is finally a rework of the series that provides kernel
> > > power_supply
> > > for hidpp devices.
> > > 
> > > This will allow upower to not handle those devices anymore and to
> > > have more
> > > immediate reportng of the device to the system.
> > 
> > FWIW, I'm on Ubuntu 14.04, and upower *is* reporting my mouse battery
> > as
> > if it were a laptop battery.  It's mostly garbage, and always reports
> > 0%, which makes upower always tell me my laptop is 2/3 charged (I
> > have 2
> > real batteries).

Well, the exported battery might be sending levels instead of
pourcentages. And upower needs to be upgraded to handle those :(

> > 
> > Is this expected?
> 
> You need to either disable the feature for the kernel, or upgrade to
> the latest git master of UPower. Mixing both won't work. Use the devkit
> mailing-list if you need a release of UPower.
> 

Bastien, is is possible to have a simple udev rule that tells upower to
ignore the battery device if builtin_power_supply is there?
That way we can tell people running old upower to use this to be sure to
ignore the kernel device and just rely on the upower hidpp support?

Cheers,
Benjamin

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Bastien Nocera]

On Fri, 2017-06-02 at 09:29 +0200, Benjamin Tissoires wrote:
> 
<snip>
> Bastien, is is possible to have a simple udev rule that tells upower
> to
> ignore the battery device if builtin_power_supply is there?
> That way we can tell people running old upower to use this to be sure
> to
> ignore the kernel device and just rely on the upower hidpp support?

I'm not sure where we'd ship that in a way that would make it work
differently based on the version of UPower. In UPower? Well, UPower
0.99.x is the stable track for UPower, and the git master already has
support for this feature.

I'm not sure what else would need to be done.

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Benjamin Tissoires]

On Jun 02 2017 or thereabouts, Bastien Nocera wrote:
> On Fri, 2017-06-02 at 09:29 +0200, Benjamin Tissoires wrote:
> > 
> <snip>
> > Bastien, is is possible to have a simple udev rule that tells upower
> > to
> > ignore the battery device if builtin_power_supply is there?
> > That way we can tell people running old upower to use this to be sure
> > to
> > ignore the kernel device and just rely on the upower hidpp support?
> 
> I'm not sure where we'd ship that in a way that would make it work
> differently based on the version of UPower. In UPower? Well, UPower
> 0.99.x is the stable track for UPower, and the git master already has
> support for this feature.
> 
> I'm not sure what else would need to be done.

I was more thinking at a snippet that we drop on this mailing list
(here) that users can carefully copy/paste in their /etc and remove
themself once they upgrade the system.
Though OTOH, that's relying on users remembering that they have a
special conf that need to be removed in a later update.

Cheers,
Benjamin

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Dave Hansen]

On 06/02/2017 12:29 AM, Benjamin Tissoires wrote:
> On Jun 01 2017 or thereabouts, Bastien Nocera wrote:
>> On Thu, 2017-06-01 at 11:06 -0700, Dave Hansen wrote:
>>> On 03/27/2017 07:59 AM, Benjamin Tissoires wrote:
>>>> this is finally a rework of the series that provides kernel
>>>> power_supply
>>>> for hidpp devices.
>>>>
>>>> This will allow upower to not handle those devices anymore and to
>>>> have more
>>>> immediate reportng of the device to the system.
>>> FWIW, I'm on Ubuntu 14.04, and upower *is* reporting my mouse battery
>>> as
>>> if it were a laptop battery.  It's mostly garbage, and always reports
>>> 0%, which makes upower always tell me my laptop is 2/3 charged (I
>>> have 2
>>> real batteries).
> Well, the exported battery might be sending levels instead of
> pourcentages. And upower needs to be upgraded to handle those :(

It sounds like there are a number of things here where newer kernels are
breaking older userspace.  It's also causing some very end-user visible
effects, like having folks' systems auto shut down because upower thinks
their batteries are dead.

Old versions of upower are obviously confused here.  It would be really
nice to ensure that newer kernels don't break it like this.

IOW, it would be really nice if this were treated like a regression,
because it's tantalizingly close.

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Jiri Kosina]

On Fri, 2 Jun 2017, Dave Hansen wrote:

> >>>> This will allow upower to not handle those devices anymore and to
> >>>> have more
> >>>> immediate reportng of the device to the system.
> >>> FWIW, I'm on Ubuntu 14.04, and upower *is* reporting my mouse battery
> >>> as
> >>> if it were a laptop battery.  It's mostly garbage, and always reports
> >>> 0%, which makes upower always tell me my laptop is 2/3 charged (I
> >>> have 2
> >>> real batteries).
> > Well, the exported battery might be sending levels instead of
> > pourcentages. And upower needs to be upgraded to handle those :(
> 
> It sounds like there are a number of things here where newer kernels are
> breaking older userspace.  It's also causing some very end-user visible
> effects, like having folks' systems auto shut down because upower thinks
> their batteries are dead.
> 
> Old versions of upower are obviously confused here.  It would be really
> nice to ensure that newer kernels don't break it like this.
> 
> IOW, it would be really nice if this were treated like a regression,
> because it's tantalizingly close.

I agree with Dave. If there is no solution found in time for -rc5, 
reverting to previous state would be the proper way to go.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Bastien Nocera]

On Mon, 2017-06-05 at 10:01 +0200, Jiri Kosina wrote:
> On Fri, 2 Jun 2017, Dave Hansen wrote:
> 
> > > > > > This will allow upower to not handle those devices anymore
> > > > > > and to
> > > > > > have more
> > > > > > immediate reportng of the device to the system.
> > > > > 
> > > > > FWIW, I'm on Ubuntu 14.04, and upower *is* reporting my mouse
> > > > > battery
> > > > > as
> > > > > if it were a laptop battery.  It's mostly garbage, and always
> > > > > reports
> > > > > 0%, which makes upower always tell me my laptop is 2/3
> > > > > charged (I
> > > > > have 2
> > > > > real batteries).
> > > 
> > > Well, the exported battery might be sending levels instead of
> > > pourcentages. And upower needs to be upgraded to handle those :(
> > 
> > It sounds like there are a number of things here where newer
> > kernels are
> > breaking older userspace.  It's also causing some very end-user
> > visible
> > effects, like having folks' systems auto shut down because upower
> > thinks
> > their batteries are dead.
> > 
> > Old versions of upower are obviously confused here.  It would be
> > really
> > nice to ensure that newer kernels don't break it like this.
> > 
> > IOW, it would be really nice if this were treated like a
> > regression,
> > because it's tantalizingly close.
> 
> I agree with Dave. If there is no solution found in time for -rc5, 
> reverting to previous state would be the proper way to go.

I don't see how it's possible to retroactively fix user-space. The best
(but oh so backwards) way to fix this might be to wrap the
functionality in a config option, for user-spaces that can't update
their version of UPower.

I still think it's counter-productive, the only consumer for this
information is UPower, and it's been modified to work-around the fact
that the kernel didn't export this battery information. It now does.

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Dave Hansen]

On 06/05/2017 06:09 AM, Bastien Nocera wrote:
>> I agree with Dave. If there is no solution found in time for -rc5, 
>> reverting to previous state would be the proper way to go.
> I don't see how it's possible to retroactively fix user-space.

It's not possible to retroactively change userspace.  That why the
kernel tries so hard not to break it in the first place.  Although this
is in "minor annoyance" territory for me at the moment, this patch
causes a clear, user-visible issue with new kernels.

The right way to do this is to have the kernel export the data in a way
that does not confuse old userspace.  Perhaps we should separate out
"power supplies that run the system" from "power supplies in a perihperal".

And, no, a config option isn't the right thing either.

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Bastien Nocera]

On Mon, 2017-06-05 at 07:53 -0700, Dave Hansen wrote:
> On 06/05/2017 06:09 AM, Bastien Nocera wrote:
> > > I agree with Dave. If there is no solution found in time for
> > > -rc5, 
> > > reverting to previous state would be the proper way to go.
> > 
> > I don't see how it's possible to retroactively fix user-space.
> 
> It's not possible to retroactively change userspace.  That why the
> kernel tries so hard not to break it in the first place.  Although
> this
> is in "minor annoyance" territory for me at the moment, this patch
> causes a clear, user-visible issue with new kernels.
> 
> The right way to do this is to have the kernel export the data in a
> way
> that does not confuse old userspace.  Perhaps we should separate out
> "power supplies that run the system" from "power supplies in a
> perihperal".

There's already such a property for it, "scope". I think that you don't
realise that it's this version of UPower you're using (one major API
version behind the current one) is buggy when it comes to handling
kernel-created "power_supply".

It's just that UPower used to do this itself, in user-space, and that
it gets thoroughly confused when it accesses both the battery from
user-space and kernel-space.

> And, no, a config option isn't the right thing either.

Because...? It's the best way to avoid exposing the feature for ancient
user-spaces. The battery information will be gathered from user-space.
It doesn't regress either.

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Benjamin Tissoires]

[Sorry for the delay, public holiday yesterday and I wasn't home]

On Jun 05 2017 or thereabouts, Jiri Kosina wrote:
> On Fri, 2 Jun 2017, Dave Hansen wrote:
> 
> > >>>> This will allow upower to not handle those devices anymore and to
> > >>>> have more
> > >>>> immediate reportng of the device to the system.
> > >>> FWIW, I'm on Ubuntu 14.04, and upower *is* reporting my mouse battery
> > >>> as
> > >>> if it were a laptop battery.  It's mostly garbage, and always reports
> > >>> 0%, which makes upower always tell me my laptop is 2/3 charged (I
> > >>> have 2
> > >>> real batteries).
> > > Well, the exported battery might be sending levels instead of
> > > pourcentages. And upower needs to be upgraded to handle those :(
> > 
> > It sounds like there are a number of things here where newer kernels are
> > breaking older userspace.  It's also causing some very end-user visible
> > effects, like having folks' systems auto shut down because upower thinks
> > their batteries are dead.
> > 
> > Old versions of upower are obviously confused here.  It would be really
> > nice to ensure that newer kernels don't break it like this.
> > 
> > IOW, it would be really nice if this were treated like a regression,
> > because it's tantalizingly close.

Believe me, I really try to avoid any regression. However, in this
situation, it's a user space bug and the only solution to not hit the
user space bug from the kernel is to not export the extra device, or
teach your upower daemon to ignore this particular device through a
udev rule (if that's possible, I'll check that today).


> 
> I agree with Dave. If there is no solution found in time for -rc5, 
> reverting to previous state would be the proper way to go.
>

Well, as Bastien said, the issue is that old user space is buggy, and
even if we postpone the switch to 4.13, there will always be someone who
did not updated upower and who will complain.

As soon as I started the development of this series, Bastien upgraded
upower with the required changes, and usually the development cycle of
the kernel gives plenty of time for users to upgrade their user space
tools before the kernel hits mainline.

[...after a little bit of digging...]

I tried today with a Fedora 25 and the shipped upower that doesn't have
the bits Bastien worked on last March.

I couldn't expose the bug as reported here. The reason being what
Bastien said, there is a "scope" property exported by the kernel device
which is set to "Device" telling upower to ignore the device completely
in this version.

A git blame game gives me 28c8653ed8d43 being the original addition of
the "scope" handling and this commit is dated "Wed Apr 18 16:46:41 2012
+0100". It was shipped in UPOWER_0_9_16, and there has been 7 minor
releases of upower since, and there has been the final 1.0 branch since
too.

So, no, I don't think this is a regression if you are running a 5 year
old user space that doesn't handle properties introduced in kernel v3.4.
Any HID keyboard you plug in that exports a battery will show the very
same upower bug, and there has been countless since 2012.

Cheers,
Benjamin

Re: [PATCH v3 00/19] Report power supply from hid-logitech-hidpp

[Bastien Nocera]

On Tue, 2017-06-06 at 09:25 +0200, Benjamin Tissoires wrote:
> 
<snip>
> Well, as Bastien said, the issue is that old user space is buggy, and
> even if we postpone the switch to 4.13, there will always be someone
> who
> did not updated upower and who will complain.
> 
> As soon as I started the development of this series, Bastien upgraded
> upower with the required changes, and usually the development cycle
> of
> the kernel gives plenty of time for users to upgrade their user space
> tools before the kernel hits mainline.
> 
> [...after a little bit of digging...]
> 
> I tried today with a Fedora 25 and the shipped upower that doesn't
> have
> the bits Bastien worked on last March.

Richard should be able to cut a release, UPower hasn't had one since
early last year, so it's time in any case :)

> I couldn't expose the bug as reported here. The reason being what
> Bastien said, there is a "scope" property exported by the kernel
> device
> which is set to "Device" telling upower to ignore the device
> completely
> in this version.

Unless we added code to the obsolete version of UPower, we don't have a
way to tell UPower to not use a particular device as a generic
"power_supply" device.