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

[Filipe Laíns]

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

Attachment: signature.asc
Description: This is a digitally signed message part