Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 8 Jan 2021 15:55:11 +0100 (CET)
From:   Jiri Kosina <jikos@kernel.org>
To:     =?ISO-8859-15?Q?Filipe_La=EDns?= <lains@archlinux.org>
cc:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] HID: logitech-hidpp: add support for Unified Battery
 (1004) feature
In-Reply-To: <e832278f9021c0f71afc5f90261bd17aea45a336.camel@archlinux.org>
Message-ID: <nycvar.YFH.7.76.2101081554190.13752@cbobk.fhfr.pm>
References: <20210104182937.1472673-1-lains@archlinux.org>  <nycvar.YFH.7.76.2101081438530.13752@cbobk.fhfr.pm> <e832278f9021c0f71afc5f90261bd17aea45a336.camel@archlinux.org>
User-Agent: Alpine 2.21 (LSU 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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