Messages in this thread | | | Subject | Re: [PATCH] HID: logitech-hidpp: add support for Unified Battery (1004) feature | From | Filipe Laíns <> | Date | Fri, 08 Jan 2021 15:01:53 +0000 |
| |
On Fri, 2021-01-08 at 15:55 +0100, Jiri Kosina wrote: > On Fri, 8 Jan 2021, Filipe Laíns wrote: > > 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 */ /* -------------------------------------------------------------------------- */
> > > Could you please use standard kernel commenting style here? > > > > Oops, sorry. Will do :) > > Thanks, >
Cheers, Filipe Laíns [unhandled content-type:application/pgp-signature] | |