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

[Jiri Kosina]

On Fri, 8 Jan 2021, Filipe Laíns wrote:

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

Alright, what a mess :) Would it perhaps help if there is at least a short 
comment preceding the function definition, noting what the constants 
actually are?

> > [ ... 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 :)

Thanks,

-- 
Jiri Kosina
SUSE Labs