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

["Filipe Laíns" <lains@archlinux.org>]

[-- Attachment #1: Type: text/plain, Size: 2784 bytes --]

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]