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]

Message-ID: <d65b0829d31ed6eb15f69b8771718d38a56a2502.camel@archlinux.org>
Date:   Fri, 08 Jan 2021 15:01:53 +0000
From:   Filipe Laíns <lains@...hlinux.org>
To:     Jiri Kosina <jikos@...nel.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, 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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)