* Re: [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset
[not found] <023148f582cc20bef7079508ce417c8ebeb0febb.camel@domanski.co>
@ 2020-06-05 15:15 ` Filipe Laíns
2020-07-04 0:37 ` Kamil Domański
2020-06-19 7:19 ` Jiri Kosina
1 sibling, 1 reply; 4+ messages in thread
From: Filipe Laíns @ 2020-06-05 15:15 UTC (permalink / raw)
To: Kamil Domański, linux-kernel
Cc: Jiri Kosina, Benjamin Tissoires, Nestor Lopez Casado
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] HID: logitech-hidpp: add support for Logitech G533 headset
[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-06-19 7:19 ` Jiri Kosina
1 sibling, 0 replies; 4+ messages in thread
From: Jiri Kosina @ 2020-06-19 7:19 UTC (permalink / raw)
To: Kamil Domański
Cc: linux-kernel, Benjamin Tissoires, Nestor Lopez Casado, Filipe Laíns
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
^ permalink raw reply [flat|nested] 4+ messages in thread