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;
Message-ID: <d65b0829d31ed6eb15f69b8771718d38a56a2502.camel@archlinux.org>
Subject: Re: [PATCH] HID: logitech-hidpp: add support for Unified Battery
 (1004) feature
From:   Filipe =?ISO-8859-1?Q?La=EDns?= <lains@archlinux.org>
To:     Jiri Kosina <jikos@kernel.org>
Cc:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <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>
         <nycvar.YFH.7.76.2101081554190.13752@cbobk.fhfr.pm>
Organization: Archlinux
Content-Type: multipart/signed; micalg="pgp-sha256";
        protocol="application/pgp-signature"; boundary="=-nUFrcBmRjwz1bi32mfxn"
Date:   Fri, 08 Jan 2021 15:01:53 +0000
MIME-Version: 1.0
User-Agent: Evolution 3.38.2 
Precedence: bulk


--=-nUFrcBmRjwz1bi32mfxn
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, 2021-01-08 at 15:55 +0100, Jiri Kosina wrote:
> On Fri, 8 Jan 2021, Filipe La=C3=ADns wrote:
> > The problem here is that hidpp20_query_battery_info_1004() does not set=
=20
> > battery voltage, it is also the battery level. The best alternative Ica=
n
> > 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 tha=
t
> > could
> > get confusing quickly.
> >=20
> > hidpp20_batterylevel_query_battery_info()
> > hidpp20_unifiedbattery_query_battery_info()
> >=20
> > 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?
>=20
> Alright, what a mess :) Would it perhaps help if there is at least a shor=
t=20
> comment preceding the function definition, noting what the constants=20
> actually are?

Yeah :head_scratch:
There is a header comment at the start of each feature section, which I thi=
nk
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?
> >=20
> > Oops, sorry. Will do :)
>=20
> Thanks,
>=20

Cheers,
Filipe La=C3=ADns

--=-nUFrcBmRjwz1bi32mfxn
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEE0jW0leqs33gyftiw+JPGdIFqqV0FAl/4c+AACgkQ+JPGdIFq
qV3xFg/+OI7Wrp9cLE9aXS7NbuQybltIktsp3S2oO9YD/GMYyVC9MXRPNtYfkqHQ
7JTUL8k/inqjsBBLMJhLqyehueWGmxYH0la1QgYpiO0Zlx/HocPG81pCG17F3QmQ
JrkNwH3w+0V2n8wWrVauwts+QTLDa4eIDJkuGtSn22XIEJlda0A95JLTtaKDYtRe
A2ElqrpfDRJoadnMQdeBzZ8M8kAeEifkamTCWshPpIthICceWvj9p/3SKbuwuAHy
mIH0DrzTdeUVlX4Z7+kOKXP3LEyTkhCnNcZ7PdFtUkWTXLaev6CfXKpNbW5Zt0iM
JMoI0+02XJVAgPe0tsIOsCl3JQ+jIb8fxvJ1Cum1PS6EWrFpgAQbxu4DQio/aiBb
5H0YBcp/UAVr5yrzVJQUUIv3cEFH/6PcE5YvOF7qNZs+i2xuwqFcAsXtasfQnTsT
txa4uBC9+osIrBYKhJNYO2KfJt/ifqR7XnpYYI5f0EktUjMgPQZnMbnTpxMKuFp9
+kaQ/5pQPWu6raDtUm+GFeNJoqQ78XVAxdz1Fb4oVw9aXYiUbYi6jKvAlVFO2qxk
EPbmyivjt5QswopB23LgFcIODusJGvubnSAOB8TbEGaanPdY+6RGcJKd2Ewank1B
9SuWGyePvG8JkWMnC3XioXpkpkfsc2MO6vhULxQh6CtFoz6qZ/c=
=i9Y+
-----END PGP SIGNATURE-----

--=-nUFrcBmRjwz1bi32mfxn--