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
