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

[Kamil DomaÅski]

On Sun, 2020-05-17 at 16:52 +0100, Filipe LaÃns wrote:
> 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.

Will fix, thanks.

> > +
> > +	long flags = (long) data[2];
> 
> Use u8 instead. Why are we even using a variable for this?

I is analogous to how it's done in existing code for feature 0x1001 (voltage).
Besides that IMHO it greatly improves readability and the asignment
will be optimized out anyways.

> > +
> > +	*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.

IMHO it improves readability.
The asignments will be optimized out anyways.

> Here I would add a #define for each bit:
> 
> #define FLAG_ADC_MAP_STATUS_CONNECTED 0
> ...
> if (data[2] & FLAG_ADC_MAP_STATUS_CONNECTED)

Again, I did it analogous to existing code. I'll be glad to change
it (and perhaps in the existing code as well) if the others concur.

> > +
> > +	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;
> > +}
> > 

Thanks for pointing it out. I'll fix it.

> > @@ -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.

I frankly don't think it supports anything else since it self-identifies
as "Logitech, Inc. [G533 Wireless Headset Dongle]" and it only returns
feature indexes for this one headset, whether it's paired or not.
It also registers a USB audio device immediately, even if the headset
is not connected.

Thanks for all the help.

Best regards,
Kamil DomaÅski