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