Re: [PATCH] HID: logitech-hidpp: add support for Unified Battery (1004) feature

[Filipe Laíns]

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