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 <[email protected]>
*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
Hi Filipe,
>> + bool isConnected;
>> + bool isCharging;
>> + bool chargingComplete;
>> + bool chargingFault;
>
> From my initial comments:
>
>> We use snake case.
Will be fixed in v3.
>> +
>> + 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.
I used long, because the test_bit macro accepts long and the similar
function for voltage reading already used long too.
That will be changed in v3 - see next paragraph.
>> +
>> + *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)
Yeah, I it will do exactly that for v3, which allows to drop the flag
variables and avoid using a long.
> Same thing here. We should see if the device supports the DJ protocol
> and add it in hid-logitech-dj instead.
It doesn't seem to be a DJ device. The DJ driver just detects the extra interfaces
and skips directly to hid_hw_start.
Regards,
Kamil
On Sat, 2020-07-04 at 02:37 +0200, Kamil Domański wrote:
> Hi Filipe,
>
> > 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.
>
> I used long, because the test_bit macro accepts long and the similar
> function for voltage reading already used long too.
> That will be changed in v3 - see next paragraph.
The compiler can handle these conversions. Explicit is generally better
than implicit, and in cases where the implicit assumptions might break,
explicit is definitely better. We know the data size, so let's use it
:D
> > > +
> > > + *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)
>
> Yeah, I it will do exactly that for v3, which allows to drop the flag
> variables and avoid using a long.
>
>
> > Same thing here. We should see if the device supports the DJ protocol
> > and add it in hid-logitech-dj instead.
>
> It doesn't seem to be a DJ device. The DJ driver just detects the extra interfaces
> and skips directly to hid_hw_start.
Thanks for looking into this!
Cheers,
Filipe Laíns