Messages in this thread | | | Subject | Re: [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset | From | Filipe Laíns <> | Date | Fri, 05 Jun 2020 16:15:43 +0100 |
| |
Hi Kamil,
On v2 patches it s usual to add a changelog (could be small) to help keep track of what changed.
On Fri, 2020-06-05 at 16:59 +0200, Kamil Domański wrote: > Signed-off-by: Kamil Domański <kamil@domanski.co>
*snip*
> +/** > + * 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;
From my initial comments:
> We use snake case.
> + > + long flags = (long) data[2];
> Use u8 instead. Why are we even using a variable for this?
My main point here is that long means different things in different architectures, and we only want one byte so I would go for u8.
> + > + *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 (isCharging) { > + if (chargingFault) > + return POWER_SUPPLY_STATUS_NOT_CHARGING; > + > + if (chargingComplete) > + return POWER_SUPPLY_STATUS_FULL; > + > + return POWER_SUPPLY_STATUS_CHARGING; > + } > + > + return POWER_SUPPLY_STATUS_DISCHARGING; > +}
The algorithm is now correct, thanks!
*snip*
> @@ -3994,6 +4190,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) }, > { /* Logitech G703 Gaming Mouse over USB */ > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) }, > { /* Logitech G703 Hero Gaming Mouse over USB */
Same thing here. We should see if the device supports the DJ protocol and add it in hid-logitech-dj instead.
Most of my comments here are nitpicks, I am not sure how strict Benjamin/Jiri are about that.
Cheers, Filipe Laíns [unhandled content-type:application/pgp-signature] | |