Re: [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset

["Kamil Domański" <kamil@domanski.co>]

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