On Fri, 2021-01-08 at 14:44 +0100, Jiri Kosina wrote:
On Mon, 4 Jan 2021, lains@archlinux.org wrote:
quoted
From: Filipe Laíns <redacted>
This new feature present in new devices replaces the old Battery Level
Status (0x1000) feature. It keeps essentially the same information for
levels (reporting critical, low, good and full) but makes these levels
optional, the device exports a capability setting which describes which
levels it supports. In addition to this, there is an optional
state_of_charge paramenter that exports the battery percentage.
This patch adds support for this new feature. There were some
implementation choices, as described below and in the code.
If the device supports the state_of_charge parameter, we will just
export the battery percentage and not the levels, which the device might
still support.
Since this feature can co-exist with the Battery Voltage (0x1001)
feature and we currently only support one battery feature, I changed the
battery feature discovery to try to use 0x1000 and 0x1004 first and only
then 0x1001, the battery voltage feature.
In the future we could uncouple this and make the battery feature
co-exists with 0x1000 and 0x1004, allowing the device to export voltage
information in addition to the battery percentage or level.
I tested this patch with a MX Anywhere 3, which supports the new
feature. Since I don't have any device that doesn't support the
state_of_charge parameter of this feature, I forced the MX Anywhere 3 to
use the level information, instead of battery percentage, to test that
part of the implementation.
I also tested with a MX Master 3, which supports the Battery Level
Status (0x1000) feature, and a G703 Hero, which supports the Battery
Voltage (0x1001) feature, to make sure nothing broke there.
Thanks a lot for the patch, Filipe. Minor details:
quoted
Signed-off-by: Filipe Laíns <redacted>
---
drivers/hid/hid-logitech-hidpp.c | 244 ++++++++++++++++++++++++++++++-
1 file changed, 237 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-
hidpp.c
index f85781464807..291c6b4d26b7 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -92,6 +92,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_CAPABILITY_BATTERY_MILEAGE BIT(2)
#define HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS BIT(3)
#define HIDPP_CAPABILITY_BATTERY_VOLTAGE BIT(4)
+#define HIDPP_CAPABILITY_BATTERY_PERCENTAGE BIT(5)
+#define HIDPP_CAPABILITY_UNIFIED_BATTERY BIT(6)
#define lg_map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max,
EV_KEY, (c))
@@ -152,6 +154,7 @@ struct hidpp_battery { int voltage;
int charge_type;
bool online;
+ u8 supported_levels_1004;
};
/**
@@ -1171,7 +1174,7 @@ static int
hidpp20_batterylevel_get_battery_info(struct hidpp_device *hidpp,
return 0;
}
-static int hidpp20_query_battery_info(struct hidpp_device *hidpp)
+static int hidpp20_query_battery_info_1000(struct hidpp_device *hidpp)
That '_1000' suffix looks strange to me, as it's not completely obvious
just from looking at the code what it actually means. Would it perhaps be
more readable to call it something like hidpp20_query_battery_level(), and
symmentrically change hidpp20_query_battery_info_1004() to e.g.
hidpp20_query_battery_voltage() ?
The problem here is that hidpp20_query_battery_info_1004() does not set the
battery voltage, it is also the battery level. The best alternative I can think
of is replacing the 1000/1004 with slightly mangled HID++ feature names, like we
do on the other feature function. The drawback here is that I think that could
get confusing quickly.
hidpp20_batterylevel_query_battery_info()
hidpp20_unifiedbattery_query_battery_info()
Note that this does not provide *that* much more information than the feature
number, though it is probably the best option. What do you think?
[ ... snip ... ]
quoted
+ /* if the device supports state of charge (battery percentage) we
won't
+ export the battery level information. there are 4 possible
battery
+ levels and they all are optional, this means that the device
might
+ not support any of them, we are just better off with the battery
+ percentage. */
Could you please use standard kernel commenting style here?
Oops, sorry. Will do :)
Cheers,
Filipe Laíns