lkml.org 
[lkml]   [2020]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset
From
Date
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

\
 
 \ /
  Last update: 2020-07-04 02:38    [W:0.076 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site