Messages in this thread | | | Subject | Re: [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset | From | Kamil Domański <> | Date | Sat, 4 Jul 2020 02:37:36 +0200 |
| |
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
| |