Re: [PATCH] HID: logitech-hidpp: add support for Logitech G533 headset

From: Filipe Laíns
Date: Sun May 17 2020 - 11:58:20 EST


On Sun, 2020-04-26 at 17:18 +0200, Kamil DomaÅski wrote:

*snip*

> +/* -------------------------------------------------------------------------- */
> +/* 0x1F20: Analog-digital converter measurement */
> +/* -------------------------------------------------------------------------- */
> +
> +#define HIDPP_PAGE_ADC_MEASUREMENT 0x1F20
> +
> +#define CMD_ADC_MEASUREMENT_GET_VOLTAGE 0x01
> +
> +/**
> + * 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;

We use snake case.

> +
> + long flags = (long) data[2];

Use u8 instead. Why are we even using a variable for this?

> +
> + *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 (chargingFault)
> + return POWER_SUPPLY_STATUS_NOT_CHARGING;

From the spec:
> Only valid if 'isCharging' is 1.

> +
> + if (chargingComplete)
> + return POWER_SUPPLY_STATUS_FULL;

From the spec:
> Only valid if 'isCharging' is 1.

> +
> + if (isCharging)
> + return POWER_SUPPLY_STATUS_CHARGING;

Put the two previous checks inside this if.

> + return POWER_SUPPLY_STATUS_DISCHARGING;
> +}
>

*snip*

> @@ -3994,6 +4189,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) },

Is this a receiver? If so, we need to know if it is used by other
devices. Or better, see if it supports the DJ protocol and add it to
hid-logitech-dj instead.

> { /* Logitech G703 Gaming Mouse over USB */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) },
> { /* Logitech G703 Hero Gaming Mouse over USB */

Cheers,
Filipe LaÃns

Attachment: signature.asc
Description: This is a digitally signed message part