linux-kernel - Re: [PATCH] HID: logitech-hidpp: add support for Unified Battery (1004) feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Jan 2021 15:55:11 +0100 (CET)
From:   Jiri Kosina <jikos@...nel.org>
To:     Filipe Laíns <lains@...hlinux.org>
cc:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: logitech-hidpp: add support for Unified Battery
 (1004) feature

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