| Message ID | 1467988545.2317.4.camel@hadess.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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 -- 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
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.
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
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;