From: "Filipe Laíns" <lains@archlinux.org>
To: "Kamil Domański" <kamil@domanski.co>, linux-kernel@vger.kernel.org
Cc: Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
Nestor Lopez Casado <nlopezcasad@logitech.com>
Subject: Re: [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset
Date: Fri, 05 Jun 2020 16:15:43 +0100 [thread overview]
Message-ID: <137acb00c6ec098d7506901df05067e8d7207aa9.camel@archlinux.org> (raw)
In-Reply-To: <023148f582cc20bef7079508ce417c8ebeb0febb.camel@domanski.co>
[-- Attachment #1: Type: text/plain, Size: 2705 bytes --]
Hi Kamil,
On v2 patches it s usual to add a changelog (could be small) to help
keep track of what changed.
On Fri, 2020-06-05 at 16:59 +0200, Kamil Domański wrote:
> Signed-off-by: Kamil Domański <kamil@domanski.co>
*snip*
> +/**
> + * hidpp20_adc_map_status_voltage() - convert HID++ code to power supply status
> + * @hidpp: HID++ device struct.
> + * @data: ADC report data.
> + * @voltage: Pointer to variable where the ADC voltage shall be written.
> + *
> + * This function decodes the ADC voltage and charge status
> + * of the device's battery.
> + *
> + * Return: Returns the power supply charge status code.
> + */
> +static int hidpp20_adc_map_status_voltage(struct hidpp_device *hidpp,
> + u8 data[3], int *voltage)
> +{
> + bool isConnected;
> + bool isCharging;
> + bool chargingComplete;
> + bool chargingFault;
From my initial comments:
> We use snake case.
> +
> + long flags = (long) data[2];
> Use u8 instead. Why are we even using a variable for this?
My main point here is that long means different things in different
architectures, and we only want one byte so I would go for u8.
> +
> + *voltage = get_unaligned_be16(data);
> + isConnected = test_bit(0, &flags);
> + isCharging = test_bit(1, &flags);
> + chargingComplete = test_bit(2, &flags);
> + chargingFault = test_bit(3, &flags);
> I don't think this is needed, just do it in the ifs directly.
>
> Here I would add a #define for each bit:
>
> #define FLAG_ADC_MAP_STATUS_CONNECTED 0
> ...
> if (data[2] & FLAG_ADC_MAP_STATUS_CONNECTED)
> +
> + if (!isConnected)
> + return POWER_SUPPLY_STATUS_UNKNOWN;
> +
> + if (isCharging) {
> + if (chargingFault)
> + return POWER_SUPPLY_STATUS_NOT_CHARGING;
> +
> + if (chargingComplete)
> + return POWER_SUPPLY_STATUS_FULL;
> +
> + return POWER_SUPPLY_STATUS_CHARGING;
> + }
> +
> + return POWER_SUPPLY_STATUS_DISCHARGING;
> +}
The algorithm is now correct, thanks!
*snip*
> @@ -3994,6 +4190,8 @@ static const struct hid_device_id hidpp_devices[] = {
>
> { /* Logitech G403 Wireless Gaming Mouse over USB */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082) },
> + { /* Logitech G533 Wireless Headset over USB */
> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0A66) },
> { /* Logitech G703 Gaming Mouse over USB */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) },
> { /* Logitech G703 Hero Gaming Mouse over USB */
Same thing here. We should see if the device supports the DJ protocol
and add it in hid-logitech-dj instead.
Most of my comments here are nitpicks, I am not sure how strict
Benjamin/Jiri are about that.
Cheers,
Filipe Laíns
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next parent reply other threads:[~2020-06-05 15:15 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <023148f582cc20bef7079508ce417c8ebeb0febb.camel@domanski.co>
2020-06-05 15:15 ` Filipe Laíns [this message]
2020-07-04 0:37 ` [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset Kamil Domański
2020-07-04 14:48 ` Filipe Laíns
2020-06-19 7:19 ` Jiri Kosina
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=137acb00c6ec098d7506901df05067e8d7207aa9.camel@archlinux.org \
--to=lains@archlinux.org \
--cc=benjamin.tissoires@redhat.com \
--cc=jikos@kernel.org \
--cc=kamil@domanski.co \
--cc=linux-kernel@vger.kernel.org \
--cc=nlopezcasad@logitech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).