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