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
Attachment:
signature.asc
Description: This is a digitally signed message part
