On Fri, 2021-01-08 at 14:44 +0100, Jiri Kosina wrote:
> On Mon, 4 Jan 2021, lains@xxxxxxxxxxxxx wrote:
>
> > From: Filipe Laíns <lains@xxxxxxxxxxxxx>
> >
> > 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:
>
> > Signed-off-by: Filipe Laíns <lains@xxxxxxxxxxxxx>
> > ---
> > 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 ... ]
> > + /* 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
Attachment:
signature.asc
Description: This is a digitally signed message part
