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

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

[Filipe Laíns]

[-- Attachment #1: Type: text/plain, Size: 2705 bytes --]

Hi Kamil,

On v2 patches it s usual to add a changelog (could be small) to help
keep track of what changed.

On Fri, 2020-06-05 at 16:59 +0200, Kamil Domański wrote:
> Signed-off-by: Kamil Domański <kamil@domanski.co>

*snip*

> +/**
> + * hidpp20_adc_map_status_voltage() - convert HID++ code to power supply status
> + * @hidpp: HID++ device struct.
> + * @data: ADC report data.
> + * @voltage: Pointer to variable where the ADC voltage shall be written.
> + *
> + * This function decodes the ADC voltage and charge status
> + * of the device's battery.
> + *
> + * Return: Returns the power supply charge status code.
> + */
> +static int hidpp20_adc_map_status_voltage(struct hidpp_device *hidpp,
> +						u8 data[3], int *voltage)
> +{
> +	bool isConnected;
> +	bool isCharging;
> +	bool chargingComplete;
> +	bool chargingFault;

From my initial comments:

> We use snake case.

> +
> +	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.

> +
> +	*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)

> +
> +	if (!isConnected)
> +		return POWER_SUPPLY_STATUS_UNKNOWN;
> +
> +	if (isCharging) {
> +		if (chargingFault)
> +			return POWER_SUPPLY_STATUS_NOT_CHARGING;
> +
> +		if (chargingComplete)
> +			return POWER_SUPPLY_STATUS_FULL;
> +
> +		return POWER_SUPPLY_STATUS_CHARGING;
> +	}
> +
> +	return POWER_SUPPLY_STATUS_DISCHARGING;
> +}

The algorithm is now correct, thanks!

*snip*

> @@ -3994,6 +4190,8 @@ static const struct hid_device_id hidpp_devices[] = {
>  
>  	{ /* Logitech G403 Wireless Gaming Mouse over USB */
>  	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082) },
> +	{ /* Logitech G533 Wireless Headset over USB */
> +	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0A66) },
>  	{ /* Logitech G703 Gaming Mouse over USB */
>  	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) },
>  	{ /* Logitech G703 Hero Gaming Mouse over USB */

Same thing here. We should see if the device supports the DJ protocol
and add it in hid-logitech-dj instead.

Most of my comments here are nitpicks, I am not sure how strict
Benjamin/Jiri are about that.

Cheers,
Filipe Laíns

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jiri Kosina]

Hi Kamil,

thanks a lot for your patch. I have a couple of comments, in addition to 
Felipe's review.

On Fri, 5 Jun 2020, Kamil Domański wrote:

Changelog is missing here.

> Signed-off-by: Kamil Domański <kamil@domanski.co>
> ---
>  drivers/hid/hid-logitech-hidpp.c | 200 ++++++++++++++++++++++++++++++-
>  1 file changed, 199 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 094f4f1b6555..b898ad4ceac5 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -29,6 +29,7 @@
>  
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
> +MODULE_AUTHOR("Kamil Domański <kamil@domanski.co>");
>  MODULE_AUTHOR("Nestor Lopez Casado <nlopezcasad@logitech.com>");
>  
>  static bool disable_raw_mode;
> @@ -92,6 +93,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
>  #define HIDPP_CAPABILITY_BATTERY_MILEAGE	BIT(2)
>  #define HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS	BIT(3)
>  #define HIDPP_CAPABILITY_BATTERY_VOLTAGE	BIT(4)
> +#define HIDPP_CAPABILITY_ADC_MEASUREMENT	BIT(5)
>  
>  /*
>   * There are two hidpp protocols in use, the first version hidpp10 is known
> @@ -141,6 +143,7 @@ struct hidpp_battery {
>  	u8 feature_index;
>  	u8 solar_feature_index;
>  	u8 voltage_feature_index;
> +	u8 adc_measurement_feature_index;
>  	struct power_supply_desc desc;
>  	struct power_supply *ps;
>  	char name[64];
> @@ -215,6 +218,7 @@ struct hidpp_device {
>  #define HIDPP_ERROR_INVALID_PARAM_VALUE		0x0b
>  #define HIDPP_ERROR_WRONG_PIN_CODE		0x0c
>  /* HID++ 2.0 error codes */
> +#define HIDPP20_ERROR_DISCONNECTED	0x05
>  #define HIDPP20_ERROR				0xff
>  
>  static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
> @@ -1378,6 +1382,184 @@ static int hidpp20_battery_voltage_event(struct hidpp_device *hidpp,
>  	return 0;
>  }
>  
> +/* -------------------------------------------------------------------------- */
> +/* 0x1F20: Analog-digital converter measurement                               */
> +/* -------------------------------------------------------------------------- */
> +
> +#define HIDPP_PAGE_ADC_MEASUREMENT 0x1F20
> +
> +#define CMD_ADC_MEASUREMENT_GET_VOLTAGE 0x01
> +
> +/**
> + * hidpp20_adc_map_status_voltage() - convert HID++ code to power supply status
> + * @hidpp: HID++ device struct.
> + * @data: ADC report data.
> + * @voltage: Pointer to variable where the ADC voltage shall be written.
> + *
> + * This function decodes the ADC voltage and charge status
> + * of the device's battery.
> + *
> + * Return: Returns the power supply charge status code.
> + */
> +static int hidpp20_adc_map_status_voltage(struct hidpp_device *hidpp,
> +						u8 data[3], int *voltage)
> +{
> +	bool isConnected;
> +	bool isCharging;
> +	bool chargingComplete;
> +	bool chargingFault;

We are not using CamelCase in the kernel. Could you please convert it to 
something that'd be in line with the kernel coding style, e.g. 
'is_connected', etc?

[ ... snip ... ]
>  static enum power_supply_property hidpp_battery_props[] = {
>  	POWER_SUPPLY_PROP_ONLINE,
>  	POWER_SUPPLY_PROP_STATUS,
> @@ -1426,6 +1608,11 @@ static int hidpp_battery_get_property(struct power_supply *psy,
>  			val->strval = hidpp->hid_dev->uniq;
>  			break;
>  		case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +			/* ADC feature doesn't automatically report the voltage
> +			   so we poll it explicitly when the property is read. */

Please use

	/*
	 * this style
	 * of multi-line comments
	 */


Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Kamil Domański]

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

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

[Filipe Laíns]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]