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 <[email protected]> > --- > 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 <[email protected]>"); > +MODULE_AUTHOR("Kamil Domański <[email protected]>"); > MODULE_AUTHOR("Nestor Lopez Casado <[email protected]>"); > > 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
