* Re: [PATCH] HID: logitech-hidpp: add support for Logitech G533 headset
2020-05-17 15:52 ` [PATCH] HID: logitech-hidpp: add support for Logitech G533 headset Filipe Laíns
@ 2020-05-17 16:23 ` Kamil Domański
0 siblings, 0 replies; 2+ messages in thread
From: Kamil Domański @ 2020-05-17 16:23 UTC (permalink / raw)
To: Filipe Laíns, linux-kernel
Cc: Jiri Kosina, Benjamin Tissoires, Nestor Lopez Casado
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
^ permalink raw reply [flat|nested] 2+ messages in thread