On Fri, 2021-01-08 at 15:55 +0100, Jiri Kosina wrote:
On Fri, 8 Jan 2021, Filipe Laíns wrote:
quoted
The problem here is that hidpp20_query_battery_info_1004() does not set
battery voltage, it is also the battery level. The best alternative Ican
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?
Yeah :head_scratch:
There is a header comment at the start of each feature section, which I think
does a good job pointing this out. IMO the problem with the naming is more for
people who see its usage in other parts of the code, but I guess that is C for
you right? Names don't scale well with code quantity :P
/* -------------------------------------------------------------------------- */
/* 0x1000: Battery level status */
/* -------------------------------------------------------------------------- */
/* -------------------------------------------------------------------------- */
/* 0x1004: Unified battery */
/* -------------------------------------------------------------------------- */
quoted
quoted
Could you please use standard kernel commenting style here?
Oops, sorry. Will do :)
Thanks,
Cheers,
Filipe Laíns