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 <ka...@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
signature.asc
Description: This is a digitally signed message part
