[PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Peter Hutterer]

If the 0x1000 Unified Battery Level Status feature exists, expose the battery
level.

The main drawback is that while a device is plugged in its battery level is 0.
To avoid exposing that as 0% charge we make up a number based on the charging
status.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2e2515a..1ead9f6 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -62,6 +62,8 @@ 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 | \
 						 HIDPP_QUIRK_CONNECT_EVENTS)
@@ -110,6 +112,15 @@ struct hidpp_report {
 	};
 } __packed;
 
+struct hidpp_battery {
+	u8 feature_index;
+	struct power_supply_desc desc;
+	struct power_supply *ps;
+	char name[64];
+	int status;
+	int level;
+};
+
 struct hidpp_device {
 	struct hid_device *hid_dev;
 	struct mutex send_mutex;
@@ -128,8 +139,9 @@ struct hidpp_device {
 	struct input_dev *delayed_input;
 
 	unsigned long quirks;
-};
 
+	struct hidpp_battery battery;
+};
 
 /* HID++ 1.0 error codes */
 #define HIDPP_ERROR				0x8f
@@ -607,6 +619,222 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
 }
 
 /* -------------------------------------------------------------------------- */
+/* 0x1000: Battery level status                                               */
+/* -------------------------------------------------------------------------- */
+
+#define HIDPP_PAGE_BATTERY_LEVEL_STATUS				0x1000
+
+#define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_LEVEL_STATUS	0x00
+#define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_CAPABILITY		0x10
+
+#define EVENT_BATTERY_LEVEL_STATUS_BROADCAST			0x00
+
+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];
+
+	/* 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
+	 */
+	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;
+			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;
+}
+
+static int hidpp20_batterylevel_get_battery_level(struct hidpp_device *hidpp,
+						  u8 feature_index,
+						  int *status,
+						  int *level,
+						  int *next_level)
+{
+	struct hidpp_report response;
+	int ret;
+	u8 *params = (u8 *)response.fap.params;
+
+	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
+					  CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_LEVEL_STATUS,
+					  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;
+
+	*status = hidpp20_batterylevel_map_status_level(params, level,
+							next_level);
+
+	return 0;
+}
+
+static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
+{
+	u8 feature_type;
+	int ret;
+	int status, level, next_level;
+
+	if (hidpp->battery.feature_index == 0) {
+		ret = hidpp_root_get_feature(hidpp,
+					     HIDPP_PAGE_BATTERY_LEVEL_STATUS,
+					     &hidpp->battery.feature_index,
+					     &feature_type);
+		if (ret)
+			return ret;
+	}
+
+	ret = hidpp20_batterylevel_get_battery_level(hidpp,
+						     hidpp->battery.feature_index,
+						     &status, &level, &next_level);
+	if (ret)
+		return ret;
+
+	hidpp->battery.status = status;
+	hidpp->battery.level = level;
+
+	return 0;
+}
+
+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;
+	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);
+
+	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;
+}
+
+static enum power_supply_property hidpp_battery_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CAPACITY,
+};
+
+static int hidpp_battery_get_property(struct power_supply *psy,
+				      enum power_supply_property psp,
+				      union power_supply_propval *val)
+{
+	struct hidpp_device *hidpp = power_supply_get_drvdata(psy);
+	int ret = 0;
+
+	switch(psp) {
+		case POWER_SUPPLY_PROP_STATUS:
+			val->intval = hidpp->battery.status;
+			break;
+		case POWER_SUPPLY_PROP_CAPACITY:
+			val->intval = hidpp->battery.level;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+	}
+
+	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->protocol_major >= 2) {
+		ret = hidpp20_initialize_battery(hidpp);
+		if (ret == 0)
+			hidpp->quirks |= HIDPP_QUIRK_HIDPP20_BATTERY;
+	}
+
+	return ret;
+}
+
+/* -------------------------------------------------------------------------- */
 /* 0x6010: Touchpad FW items                                                  */
 /* -------------------------------------------------------------------------- */
 
@@ -2050,6 +2278,12 @@ 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) {
+		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)
@@ -2158,6 +2392,8 @@ 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;
-- 
2.7.4

[PATCH 2/2] HID: logitech-hidpp: remove HIDPP_QUIRK_CONNECT_EVENTS

[Peter Hutterer]

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Now that we can create battery power_supply sources, it's better to enable
the connect_event callback unconditionally.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1ead9f6..4eeb550 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -58,15 +58,14 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_CLASS_G920			BIT(3)
 
 /* bits 2..20 are reserved for classes */
-#define HIDPP_QUIRK_CONNECT_EVENTS		BIT(21)
+/* #define HIDPP_QUIRK_CONNECT_EVENTS		BIT(21) disabled */
 #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 | \
-						 HIDPP_QUIRK_CONNECT_EVENTS)
+#define HIDPP_QUIRK_DELAYED_INIT		HIDPP_QUIRK_NO_HIDINPUT
 
 /*
  * There are two hidpp protocols in use, the first version hidpp10 is known
@@ -2230,8 +2229,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 	if (unlikely(hidpp_report_is_connect_event(report))) {
 		atomic_set(&hidpp->connected,
 				!(report->rap.params[0] & (1 << 6)));
-		if ((hidpp->quirks & HIDPP_QUIRK_CONNECT_EVENTS) &&
-		    (schedule_work(&hidpp->work) == 0))
+		if (schedule_work(&hidpp->work) == 0)
 			dbg_hid("%s: connect event already queued\n", __func__);
 		return 1;
 	}
@@ -2449,7 +2447,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	if (disable_raw_mode) {
 		hidpp->quirks &= ~HIDPP_QUIRK_CLASS_WTP;
-		hidpp->quirks &= ~HIDPP_QUIRK_CONNECT_EVENTS;
 		hidpp->quirks &= ~HIDPP_QUIRK_NO_HIDINPUT;
 	}
 
@@ -2535,12 +2532,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
-	if (hidpp->quirks & HIDPP_QUIRK_CONNECT_EVENTS) {
-		/* Allow incoming packets */
-		hid_device_io_start(hdev);
+	/* Allow incoming packets */
+	hid_device_io_start(hdev);
 
-		hidpp_connect_event(hidpp);
-	}
+	hidpp_connect_event(hidpp);
 
 	return ret;
 
@@ -2593,7 +2588,7 @@ static const struct hid_device_id hidpp_devices[] = {
 	{ /* Keyboard logitech K400 */
 	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
 		USB_VENDOR_ID_LOGITECH, 0x4024),
-	  .driver_data = HIDPP_QUIRK_CONNECT_EVENTS | HIDPP_QUIRK_CLASS_K400 },
+	  .driver_data = HIDPP_QUIRK_CLASS_K400 },
 
 	{ HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
 		USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},
-- 
2.7.4

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Jiri Kosina]

On Wed, 29 Jun 2016, Peter Hutterer wrote:

> If the 0x1000 Unified Battery Level Status feature exists, expose the battery
> level.
> 
> The main drawback is that while a device is plugged in its battery level is 0.
> To avoid exposing that as 0% charge we make up a number based on the charging
> status.
> 
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Merged both patches to hid.git#for-4.8/logitech-hidpp-battery. Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Bastien Nocera]

On Wed, 2016-06-29 at 19:28 +1000, Peter Hutterer wrote:
> If the 0x1000 Unified Battery Level Status feature exists, expose the
> battery
> level.
> 
> The main drawback is that while a device is plugged in its battery
> level is 0.
> To avoid exposing that as 0% charge we make up a number based on the
> charging
> status.

This will require changes in UPower, so that it doesn't try to access
the Logitech unifying devices via user-space, and uses the data from
the kernel. Did you already file a bug?

Note that this would also mean losing the "lux" information, but I
don't think that's something we're that interested in exposing.

For example, for a keyboard that recharges via solar panels, at night:

Device: /org/freedesktop/UPower/devices/keyboard_0003o046Do4002x0004
  native-path:          /sys/devices/pci0000:00/0000:00:14.0/usb3/3-10/3-10:1.2/0003:046D:C52B.0003/0003:046D:4002.0004
  vendor:               Logitech, Inc.
  model:                K750
  serial:               197F3F23
  power supply:         no
  updated:              Fri 08 Jul 2016 01:17:40 CEST (95 seconds ago)
  has history:          yes
  has statistics:       no
  keyboard
    present:             yes
    rechargeable:        yes
    state:               discharging
    warning-level:       none
    luminosity:          16 lx
    percentage:          89%
    icon-name:          'battery-full-symbolic'
--
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

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Peter Hutterer]

On Fri, Jul 08, 2016 at 01:21:08AM +0200, Bastien Nocera wrote:
> On Wed, 2016-06-29 at 19:28 +1000, Peter Hutterer wrote:
> > If the 0x1000 Unified Battery Level Status feature exists, expose the
> > battery
> > level.
> > 
> > The main drawback is that while a device is plugged in its battery
> > level is 0.
> > To avoid exposing that as 0% charge we make up a number based on the
> > charging
> > status.
> 
> This will require changes in UPower, so that it doesn't try to access
> the Logitech unifying devices via user-space, and uses the data from
> the kernel. Did you already file a bug?

filed now: https://bugs.freedesktop.org/show_bug.cgi?id=96857

> Note that this would also mean losing the "lux" information, but I
> don't think that's something we're that interested in exposing.

Adding that HID++ request to the kernel would be easy enough but I don't 
see anything in the power_supply_property that would match this, do you? 
Also, I don't have such a device so testing would be tricky.

Cheers,
   Peter


> 
> For example, for a keyboard that recharges via solar panels, at night:
> 
> Device: /org/freedesktop/UPower/devices/keyboard_0003o046Do4002x0004
>   native-path:          /sys/devices/pci0000:00/0000:00:14.0/usb3/3-10/3-10:1.2/0003:046D:C52B.0003/0003:046D:4002.0004
>   vendor:               Logitech, Inc.
>   model:                K750
>   serial:               197F3F23
>   power supply:         no
>   updated:              Fri 08 Jul 2016 01:17:40 CEST (95 seconds ago)
>   has history:          yes
>   has statistics:       no
>   keyboard
>     present:             yes
>     rechargeable:        yes
>     state:               discharging
>     warning-level:       none
>     luminosity:          16 lx
>     percentage:          89%
>     icon-name:          'battery-full-symbolic'
--
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

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Bastien Nocera]

On Wed, 2016-06-29 at 19:28 +1000, Peter Hutterer wrote:
> +static int hidpp_battery_get_property(struct power_supply *psy,
> +                                     enum power_supply_property psp,
> +                                     union power_supply_propval
> *val)
> +{
> +       struct hidpp_device *hidpp = power_supply_get_drvdata(psy);
> +       int ret = 0;
> +
> +       switch(psp) {
> +               case POWER_SUPPLY_PROP_STATUS:
> +                       val->intval = hidpp->battery.status;
> +                       break;
> +               case POWER_SUPPLY_PROP_CAPACITY:
> +                       val->intval = hidpp->battery.level;
> +                       break;
> +               default:

You forgot to handle POWER_SUPPLY_PROP_SCOPE. This means that UPower
thinks it's supplying power to the computer to which it is connected.

Should be set to "POWER_SUPPLY_SCOPE_DEVICE". This should fix it.

From 8fbfcfd411a4b2c55ec24adc8b8ecc0bca2db5e3 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Fri, 8 Jul 2016 16:34:18 +0200
Subject: [PATCH] HID: logitech-hidpp: Add scope to battery

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.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 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.7.4

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

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Bastien Nocera]

On Fri, 2016-07-08 at 16:35 +0200, Bastien Nocera wrote:
> On Wed, 2016-06-29 at 19:28 +1000, Peter Hutterer wrote:
> > +static int hidpp_battery_get_property(struct power_supply *psy,
> > +                                     enum power_supply_property
> > psp,
> > +                                     union power_supply_propval
> > *val)
> > +{
> > +       struct hidpp_device *hidpp = power_supply_get_drvdata(psy);
> > +       int ret = 0;
> > +
> > +       switch(psp) {
> > +               case POWER_SUPPLY_PROP_STATUS:
> > +                       val->intval = hidpp->battery.status;
> > +                       break;
> > +               case POWER_SUPPLY_PROP_CAPACITY:
> > +                       val->intval = hidpp->battery.level;
> > +                       break;
> > +               default:
> 
> You forgot to handle POWER_SUPPLY_PROP_SCOPE. This means that UPower
> thinks it's supplying power to the computer to which it is connected.
> 
> Should be set to "POWER_SUPPLY_SCOPE_DEVICE". This should fix it.

I've also noticed that my keyboard appears 4 times:
$ cat /sys/class/power_supply/*/device/input/input*/name
Logitech K750
Logitech K750
Logitech Rechargeable Touchpad T650
Logitech K750
Logitech K750

By the way, instead of giving fake values when starting up, you should
use the ONLINE property instead. That way, when the property changes to
1, UPower can start taking the capacity value into account. In the
meanwhile, it would be ignored.

Finally, things like the serial number and model name and manufacturer
should be replicated in the power_supply, so that UPower can use them
too.

Fixing UPower to not do its user-space poking when a device appears
would be quite complicated. First, the unifier device will show up,
then as a child, the power_supply subsystem device, so we cannot just
probe when devices appear, as it would be in the wrong order for us.

Would it be possible to add a sysfs file that's there in those newer
versions of the kernel with this patch included? That way, I'd change
the lines in:
http://cgit.freedesktop.org/upower/tree/rules/95-upower-csr.rules

From:
ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c52b", DRIVER=="logitech-
hidpp-device", ENV{UPOWER_BATTERY_TYPE}="unifying"
to:
ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c52b", DRIVER=="logitech-
hidpp-device", TEST!="builtin_power_supply",
ENV{UPOWER_BATTERY_TYPE}="unifying"

For example.

Sorry about not being able to test this before it was merged for
inclusion.

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

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Jiri Kosina]

On Fri, 8 Jul 2016, Bastien Nocera wrote:

> Sorry about not being able to test this before it was merged for 
> inclusion.

This is just in a topic branch of hid.git as of now, so we can still tweak 
things (including API) if/as needed.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Benjamin Tissoires]

On Jul 08 2016 or thereabouts, Bastien Nocera wrote:
> On Wed, 2016-06-29 at 19:28 +1000, Peter Hutterer wrote:
> > +static int hidpp_battery_get_property(struct power_supply *psy,
> > +                                     enum power_supply_property psp,
> > +                                     union power_supply_propval
> > *val)
> > +{
> > +       struct hidpp_device *hidpp = power_supply_get_drvdata(psy);
> > +       int ret = 0;
> > +
> > +       switch(psp) {
> > +               case POWER_SUPPLY_PROP_STATUS:
> > +                       val->intval = hidpp->battery.status;
> > +                       break;
> > +               case POWER_SUPPLY_PROP_CAPACITY:
> > +                       val->intval = hidpp->battery.level;
> > +                       break;
> > +               default:
> 
> You forgot to handle POWER_SUPPLY_PROP_SCOPE. This means that UPower
> thinks it's supplying power to the computer to which it is connected.
> 
> Should be set to "POWER_SUPPLY_SCOPE_DEVICE". This should fix it.
> 
> From 8fbfcfd411a4b2c55ec24adc8b8ecc0bca2db5e3 Mon Sep 17 00:00:00 2001
> From: Bastien Nocera <hadess@hadess.net>
> Date: Fri, 8 Jul 2016 16:34:18 +0200
> Subject: [PATCH] HID: logitech-hidpp: Add scope to battery
> 
> 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.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>

FWIW, the following patch is:
Reviewed-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.7.4
> 
--
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

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Benjamin Tissoires]

On Jul 08 2016 or thereabouts, Bastien Nocera wrote:
> On Fri, 2016-07-08 at 16:35 +0200, Bastien Nocera wrote:
> > On Wed, 2016-06-29 at 19:28 +1000, Peter Hutterer wrote:
> > > +static int hidpp_battery_get_property(struct power_supply *psy,
> > > +                                     enum power_supply_property
> > > psp,
> > > +                                     union power_supply_propval
> > > *val)
> > > +{
> > > +       struct hidpp_device *hidpp = power_supply_get_drvdata(psy);
> > > +       int ret = 0;
> > > +
> > > +       switch(psp) {
> > > +               case POWER_SUPPLY_PROP_STATUS:
> > > +                       val->intval = hidpp->battery.status;
> > > +                       break;
> > > +               case POWER_SUPPLY_PROP_CAPACITY:
> > > +                       val->intval = hidpp->battery.level;
> > > +                       break;
> > > +               default:
> > 
> > You forgot to handle POWER_SUPPLY_PROP_SCOPE. This means that UPower
> > thinks it's supplying power to the computer to which it is connected.
> > 
> > Should be set to "POWER_SUPPLY_SCOPE_DEVICE". This should fix it.
> 
> I've also noticed that my keyboard appears 4 times:
> $ cat /sys/class/power_supply/*/device/input/input*/name
> Logitech K750
> Logitech K750
> Logitech Rechargeable Touchpad T650
> Logitech K750
> Logitech K750

I just tested it again, and it appears we create a power_supply node at
each connect event. Switching the device off and on creates a lot of
those :)

That's easy to fix

> 
> By the way, instead of giving fake values when starting up, you should
> use the ONLINE property instead. That way, when the property changes to
> 1, UPower can start taking the capacity value into account. In the
> meanwhile, it would be ignored.

I couldn't managed to get this working. I played with ONLINE and
PRESENT, but each time the battery was reported with the fake values.

The problem we also see with the Logitech devices is that they do not
provide the battery capacity at all while charging. I think UPower
caches the last known value, and we should probably do the same (and use
the provided hints to give a rough idea depending on the charging
status).

> 
> Finally, things like the serial number and model name and manufacturer
> should be replicated in the power_supply, so that UPower can use them
> too.

That should be easy enough to do.

> 
> Fixing UPower to not do its user-space poking when a device appears
> would be quite complicated. First, the unifier device will show up,
> then as a child, the power_supply subsystem device, so we cannot just
> probe when devices appear, as it would be in the wrong order for us.
> 
> Would it be possible to add a sysfs file that's there in those newer
> versions of the kernel with this patch included? That way, I'd change
> the lines in:
> http://cgit.freedesktop.org/upower/tree/rules/95-upower-csr.rules
> 
> From:
> ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c52b", DRIVER=="logitech-
> hidpp-device", ENV{UPOWER_BATTERY_TYPE}="unifying"
> to:
> ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c52b", DRIVER=="logitech-
> hidpp-device", TEST!="builtin_power_supply",
> ENV{UPOWER_BATTERY_TYPE}="unifying"
> 
> For example.

Seems like a reasonable thing to do. Especially because the power_supply
node is created when the device first gets connected, which means that
you'll see the input node first and then the power supply at some point,
later, or maybe never.

Cheers,
Benjamin

> 
> Sorry about not being able to test this before it was merged for
> inclusion.
> 
> Cheers
--
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

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Benjamin Tissoires]

On Jul 08 2016 or thereabouts, Jiri Kosina wrote:
> On Fri, 8 Jul 2016, Bastien Nocera wrote:
> 
> > Sorry about not being able to test this before it was merged for 
> > inclusion.
> 
> This is just in a topic branch of hid.git as of now, so we can still tweak 
> things (including API) if/as needed.

Hmm, I think before this gets pushed to Linus' tree, we'll also need to
have battery support for HID++ 1.0 devices too (upower seems to be
handling them, so this would introduce a regression).
So depending on how much work we can do, it might be better to consider
this as v4.9 material (or at least keep in mind that this might not be
ready before the v4.8 merge window opens, and decide its fate at that
time).

Cheers,
Benjamin 

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Jiri Kosina]

On Mon, 11 Jul 2016, Benjamin Tissoires wrote:

> Hmm, I think before this gets pushed to Linus' tree, we'll also need to 
> have battery support for HID++ 1.0 devices too (upower seems to be 
> handling them, so this would introduce a regression). So depending on 
> how much work we can do, it might be better to consider this as v4.9 
> material (or at least keep in mind that this might not be ready before 
> the v4.8 merge window opens, and decide its fate at that time).

Makes sense, thanks Benjamin. I am putting 
hid.git#for-4.8/logitech-hidpp-battery on hold for now.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Peter Hutterer]

On Fri, Jul 08, 2016 at 04:35:45PM +0200, Bastien Nocera wrote:
> On Wed, 2016-06-29 at 19:28 +1000, Peter Hutterer wrote:
> > +static int hidpp_battery_get_property(struct power_supply *psy,
> > +                                     enum power_supply_property psp,
> > +                                     union power_supply_propval
> > *val)
> > +{
> > +       struct hidpp_device *hidpp = power_supply_get_drvdata(psy);
> > +       int ret = 0;
> > +
> > +       switch(psp) {
> > +               case POWER_SUPPLY_PROP_STATUS:
> > +                       val->intval = hidpp->battery.status;
> > +                       break;
> > +               case POWER_SUPPLY_PROP_CAPACITY:
> > +                       val->intval = hidpp->battery.level;
> > +                       break;
> > +               default:
> 
> You forgot to handle POWER_SUPPLY_PROP_SCOPE. This means that UPower
> thinks it's supplying power to the computer to which it is connected.
> 
> Should be set to "POWER_SUPPLY_SCOPE_DEVICE". This should fix it.
> 
> From 8fbfcfd411a4b2c55ec24adc8b8ecc0bca2db5e3 Mon Sep 17 00:00:00 2001
> From: Bastien Nocera <hadess@hadess.net>
> Date: Fri, 8 Jul 2016 16:34:18 +0200
> Subject: [PATCH] HID: logitech-hidpp: Add scope to battery
> 
> 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.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>

Tested-by: Peter Hutterer <peter.hutterer@who-t.net>

Cheers,
   Peter

> ---
>  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.7.4
> 
--
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

Re: [PATCH 1/2] HID: logitech-hidpp: add battery support for HID++ 2.0 devices

[Bastien Nocera]

On Wed, 2016-06-29 at 19:28 +1000, Peter Hutterer wrote:
> If the 0x1000 Unified Battery Level Status feature exists, expose the battery
> level.
> 
> The main drawback is that while a device is plugged in its battery level is 0.
> To avoid exposing that as 0% charge we make up a number based on the charging
> status.

The reason why you don't get a proper state for the K750 is because it
doesn't export a battery status of its own. The UPower code uses a
heuristic based on the status of the solar panel:
if (priv->lux > 200) {
	priv->batt_status = HIDPP_DEVICE_BATT_STATUS_CHARGING;
} else {
	priv->batt_status = HIDPP_DEVICE_BATT_STATUS_DISCHARGING;
}

This is the code in UPower:
https://cgit.freedesktop.org/upower/tree/src/linux/hidpp-device.c#n914

The heuristics seem to be fairly consistent with the light level button
on the keyboard, which shows green or red depending on whether the
device is charging or discharging.

This hunk of code is also the reason why the battery percentage doesn't
match:

kernel hidpp code:
Device: /org/freedesktop/UPower/devices/keyboard_hidpp_battery_4
  native-path:          hidpp_battery_4
    percentage:          1%

UPower's hidpp code:
Device: /org/freedesktop/UPower/devices/keyboard_0003o046Do4002x000F
  native-path:          /sys/devices/pci0000:00/0000:00:14.0/usb3/3-
10/3-10:1.2/0003:046D:C52B.000E/0003:046D:4002.000F
    percentage:          95%

Cheers