On Fri, 8 Jan 2021, Filipe Laíns wrote:
quoted
quoted
-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?
quoted
[ ... 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 :)
Thanks,
--
Jiri Kosina
SUSE Labs