Messages in this thread | | | Subject | Re: [PATCH] HID: logitech-hidpp: add support for Logitech G533 headset | From | Kamil Domański <> | Date | Sun, 17 May 2020 18:23:14 +0200 |
| |
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
| |