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
