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@xxxxxxxxxxx>
> ---
>  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@xxxxxxxxx>");
> +MODULE_AUTHOR("Kamil Domański <kamil@xxxxxxxxxxx>");
>  MODULE_AUTHOR("Nestor Lopez Casado <nlopezcasad@xxxxxxxxxxxx>");
>  
>  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