From: Jiri Kosina <jikos@kernel.org>
To: "Kamil Domański" <kamil@domanski.co>
Cc: linux-kernel@vger.kernel.org,
"Benjamin Tissoires" <benjamin.tissoires@redhat.com>,
"Nestor Lopez Casado" <nlopezcasad@logitech.com>,
"Filipe Laíns" <lains@archlinux.org>
Subject: Re: [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset
Date: Fri, 19 Jun 2020 09:19:55 +0200 (CEST) [thread overview]
Message-ID: <nycvar.YFH.7.76.2006190916230.13242@cbobk.fhfr.pm> (raw)
In-Reply-To: <023148f582cc20bef7079508ce417c8ebeb0febb.camel@domanski.co>
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
prev parent reply other threads:[~2020-06-19 7:20 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <023148f582cc20bef7079508ce417c8ebeb0febb.camel@domanski.co>
2020-06-05 15:15 ` [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset Filipe Laíns
2020-07-04 0:37 ` Kamil Domański
2020-07-04 14:48 ` Filipe Laíns
2020-06-19 7:19 ` Jiri Kosina [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.YFH.7.76.2006190916230.13242@cbobk.fhfr.pm \
--to=jikos@kernel.org \
--cc=benjamin.tissoires@redhat.com \
--cc=kamil@domanski.co \
--cc=lains@archlinux.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nlopezcasad@logitech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).