2022-12-15 21:17:09

by Andreas Bergmeier

[permalink] [raw]
Subject: [PATCH] hid: Support for Litra Glow

Tries to implement as general support for Illumination Light as
possible. Note that it is singular, which means by Logitech spec we are
fine off with just handling one capability/device.

Implementation currently only exposes Brightness and On/Off controls.
Does currently not expose Color Temperature because LEDs does not
support it.

Introduces HIDPP_QUIRK_CLASS_SIMPLE_START to prevent reconnect on
startup. Could not get Glow to work with that.

Signed-off-by: Andreas Bergmeier <[email protected]>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-logitech-hidpp.c | 435 ++++++++++++++++++++++++++++++-
drivers/hid/hid-quirks.c | 1 +
3 files changed, 432 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 8f58c3c1bec3..728dede997d3 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -857,6 +857,7 @@
#define USB_DEVICE_ID_MX5500_RECEIVER_MOUSE_DEV 0xc71c
#define USB_DEVICE_ID_DINOVO_MINI_RECEIVER_KBD_DEV 0xc71e
#define USB_DEVICE_ID_DINOVO_MINI_RECEIVER_MOUSE_DEV 0xc71f
+#define USB_DEVICE_ID_LOGITECH_LITRA_GLOW 0xc900
#define USB_DEVICE_ID_LOGITECH_MOMO_WHEEL2 0xca03
#define USB_DEVICE_ID_LOGITECH_VIBRATION_WHEEL 0xca04

diff --git a/drivers/hid/hid-logitech-hidpp.c
b/drivers/hid/hid-logitech-hidpp.c
index 6d8d933efe18..c495484e9765 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -11,6 +11,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

#include <linux/device.h>
+#include <linux/dmi.h>
#include <linux/input.h>
#include <linux/usb.h>
#include <linux/hid.h>
@@ -77,6 +78,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_QUIRK_HIDPP_WHEELS BIT(26)
#define HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS BIT(27)
#define HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS BIT(28)
+#define HIDPP_QUIRK_CLASS_SIMPLE_START BIT(29)

/* These are just aliases for now */
#define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS
@@ -99,6 +101,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL BIT(7)
#define HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL BIT(8)
#define HIDPP_CAPABILITY_HIDPP10_FAST_SCROLL BIT(9)
+#define HIDPP_CAPABILITY_ILLUMINATION_LIGHT BIT(10)

#define lg_map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max,
EV_KEY, (c))

@@ -207,6 +210,7 @@ struct hidpp_device {
struct hidpp_scroll_counter vertical_wheel_counter;

u8 wireless_feature_index;
+ u8 illumination_feature_index;
};

/* HID++ 1.0 error codes */
@@ -228,6 +232,7 @@ struct hidpp_device {
#define HIDPP20_ERROR 0xff

static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
+static void hidpp_illumination_defered_event(struct hidpp_device *hidpp);

static int __hidpp_send_report(struct hid_device *hdev,
struct hidpp_report *hidpp_report)
@@ -402,6 +407,9 @@ static void delayed_work_cb(struct work_struct *work)
struct hidpp_device *hidpp = container_of(work, struct
hidpp_device,
work);
hidpp_connect_event(hidpp);
+ if (hidpp->capabilities & HIDPP_CAPABILITY_ILLUMINATION_LIGHT) {
+ hidpp_illumination_defered_event(hidpp);
+ }
}

static inline bool hidpp_match_answer(struct hidpp_report *question,
@@ -857,6 +865,8 @@ static int hidpp_unifying_init(struct hidpp_device
*hidpp)
#define CMD_ROOT_GET_FEATURE 0x00
#define CMD_ROOT_GET_PROTOCOL_VERSION 0x10

+#define HIDPP_FEATURE_TYPE_HIDDEN 0x70
+
static int hidpp_root_get_feature(struct hidpp_device *hidpp, u16
feature,
u8 *feature_index, u8 *feature_type)
{
@@ -1723,6 +1733,392 @@ static int hidpp_set_wireless_feature_index(struct
hidpp_device *hidpp)
return ret;
}

+/*
--------------------------------------------------------------------------
*/
+/* 0x1990: Illumination Light
*/
+/*
--------------------------------------------------------------------------
*/
+
+#define HIDPP_PAGE_ILLUMINATION_LIGHT 0x1990
+
+#define HIDPP_ILLUMINATION_FUNC_GET 0x00
+#define HIDPP_ILLUMINATION_FUNC_SET 0x10
+#define HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS_INFO 0x20
+#define HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS 0x30
+#define HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS 0x40
+
+/* Not yet supported
+ * #define HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS_LEVELS 0x50
+ * #define HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS_LEVELS 0x60
+ */
+
+#define HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE_INFO 0x70
+#define HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE 0x80
+#define HIDPP_ILLUMINATION_FUNC_SET_COLOR_TEMPERATURE 0x90
+
+/* Not yet supported
+ * #define HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE_LEVELS 0xA0
+ * #define HIDPP_ILLUMINATION_FUNC_SET_COLOR_TEMPERATURE_LEVELS 0xB0
+ */
+
+#define HIDPP_ILLUMINATION_EVENT_CHANGE 0x00
+#define HIDPP_ILLUMINATION_EVENT_BRIGHTNESS_CHANGE 0x10
+#define HIDPP_ILLUMINATION_EVENT_COLOR_TEMPERATURE_CHANGE 0x20
+
+#define HIDPP_ILLUMINATION_CAP_EVENTS BIT(0)
+#define HIDPP_ILLUMINATION_CAP_LINEAR_LEVELS BIT(1)
+#define HIDPP_ILLUMINATION_CAP_NON_LINEAR_LEVELS BIT(2)
+
+struct control_info {
+ u16 min;
+ u16 max;
+ u16 res;
+ u8 capabilities;
+ u8 max_levels;
+};
+
+struct led_data {
+ struct led_classdev cdev;
+ struct hidpp_device *drv_data;
+ struct hid_device *hdev;
+ u16 feature_index;
+ struct control_info brightness_info;
+ struct control_info color_temperature_info;
+ struct {
+ struct mutex mutex;
+ int on;
+ int brightness;
+ } hw_change;
+ char dirname[256];
+};
+
+/* kernel led interface designates 0 as off. To not lose the ability to
chose
+ * minimal brightness, we thus need to increase the reported range by 1
+ */
+static unsigned int device_to_led_brightness(struct led_data *led, u16
device_brightness)
+{
+ u16 relative = device_brightness - led->brightness_info.min;
+ u16 step = relative / led->brightness_info.res;
+
+ return step + 1;
+}
+
+static u16 led_to_device_brightness(struct led_data *led, unsigned int
led_brightness)
+{
+ unsigned int step = led_brightness - 1;
+ unsigned int relative = step * led->brightness_info.res;
+
+ return led->brightness_info.min + relative;
+}
+
+static int request_led_on(struct hidpp_device *hidpp, struct led_data
*led, bool *on)
+{
+
+ struct hidpp_report report;
+
+ int ret = hidpp_send_fap_command_sync(hidpp,
+ hidpp->illumination_feature_index,
+ HIDPP_ILLUMINATION_FUNC_GET, NULL,
+ 0, &report);
+ if (ret) {
+ hid_err(hidpp->hid_dev, "Getting Illumination failed\n");
+ return ret;
+ }
+
+ *on = report.fap.params[0] & BIT(0);
+ return 0;
+}
+
+static int request_led_brightness(struct hidpp_device *hidpp, struct
led_data *led,
+ unsigned int *led_brightness)
+{
+ u16 device_brightness;
+ struct hidpp_report report;
+
+ int ret = hidpp_send_fap_command_sync(
+ hidpp, hidpp->illumination_feature_index,
+ HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS, NULL, 0, &report);
+ if (ret) {
+ hid_err(hidpp->hid_dev,
+ "Getting Illumination Brightness failed\n");
+ return ret;
+ }
+
+ device_brightness = get_unaligned_be16(&report.fap.params[0]);
+ *led_brightness = device_to_led_brightness(led,
device_brightness);
+ return 0;
+}
+
+static enum led_brightness led_brightness_get(struct led_classdev
*led_cdev)
+{
+ bool on;
+ unsigned int brightness;
+ struct led_data *led = container_of(led_cdev, struct led_data,
cdev);
+ struct hidpp_device *hidpp = led->drv_data;
+
+ int ret = request_led_on(hidpp, led, &on);
+
+ if (ret || !on)
+ return LED_OFF;
+
+ ret = request_led_brightness(hidpp, led, &brightness);
+ if (ret)
+ return LED_OFF;
+
+ return brightness;
+}
+
+
+static void led_brightness_set_dummy(struct led_classdev *led_cdev,
+ enum led_brightness led_brightness)
+{
+}
+
+static int led_brightness_set_sync(struct led_classdev *led_cdev,
+ enum led_brightness led_brightness)
+{
+ struct hidpp_report report;
+ struct led_data *led = container_of(led_cdev, struct led_data,
cdev);
+ struct hidpp_device *hidpp = led->drv_data;
+ u16 device_brightness = led_to_device_brightness(led,
led_brightness);
+
+ bool on = led_brightness != 0;
+ u8 params[2] = { on ? BIT(0) : 0, 0 };
+
+ int ret = hidpp_send_fap_command_sync(hidpp,
+
hidpp->illumination_feature_index,
+ HIDPP_ILLUMINATION_FUNC_SET,
params,
+ 1, &report);
+ if (ret) {
+ hid_err(hidpp->hid_dev, "Setting Illumination failed\n");
+ return ret;
+ }
+
+ if (!on)
+ return 0;
+
+ put_unaligned_be16(device_brightness, params);
+ ret = hidpp_send_fap_command_sync(
+ hidpp, hidpp->illumination_feature_index,
+ HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS, params, 2,
+ &report);
+ if (ret) {
+ hid_err(hidpp->hid_dev,
+ "Setting Illumination Brightness failed\n");
+ }
+ return ret;
+}
+
+static int get_brightness_info_sync(struct hidpp_device *hidpp,
+ struct control_info *info)
+{
+ struct hidpp_report resp;
+ int ret = hidpp_send_fap_command_sync(
+ hidpp, hidpp->illumination_feature_index,
+ HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS_INFO, NULL, 0,
&resp);
+ if (ret) {
+ hid_err(hidpp->hid_dev,
+ "%s: failed with %d\n", __func__, ret);
+ return ret;
+ }
+
+ info->capabilities = resp.fap.params[0];
+ info->min = get_unaligned_be16(&resp.fap.params[1]);
+ info->max = get_unaligned_be16(&resp.fap.params[3]);
+ info->res = get_unaligned_be16(&resp.fap.params[5]);
+ info->max_levels = resp.fap.params[7] & 0x0F;
+ return 0;
+}
+
+static int get_color_temperature_info_sync(struct hidpp_device *hidpp,
+ struct control_info *info)
+{
+ struct hidpp_report resp;
+ int ret = hidpp_send_fap_command_sync(
+ hidpp, hidpp->illumination_feature_index,
+ HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE_INFO, NULL,
0,
+ &resp);
+ if (ret) {
+ hid_err(hidpp->hid_dev,
+ "%s: failed with %d\n", __func__, ret);
+ return ret;
+ }
+
+ info->capabilities = resp.fap.params[0];
+ info->min = get_unaligned_be16(&resp.fap.params[1]);
+ info->max = get_unaligned_be16(&resp.fap.params[3]);
+ info->res = get_unaligned_be16(&resp.fap.params[5]);
+ info->max_levels = resp.fap.params[7];
+ return 0;
+}
+
+static int register_led(struct hidpp_device *hidpp)
+{
+ int ret;
+ unsigned int brightness_range;
+ struct led_data *led = devm_kzalloc(&hidpp->hid_dev->dev,
sizeof(struct led_data),
+ GFP_KERNEL);
+
+ if (!led)
+ return -ENOMEM;
+
+ ret = get_brightness_info_sync(hidpp, &led->brightness_info);
+ if (ret)
+ goto cleanup;
+
+ ret = get_color_temperature_info_sync(hidpp,
+
&led->color_temperature_info);
+ if (ret)
+ goto cleanup;
+
+ led->drv_data = hidpp;
+ mutex_init(&led->hw_change.mutex);
+ led->hw_change.on = -1;
+ led->hw_change.brightness = -1;
+ led->cdev.name = "logitech::illumination";
+ led->cdev.flags = LED_BRIGHT_HW_CHANGED | LED_HW_PLUGGABLE;
+ led->cdev.max_brightness = device_to_led_brightness(led,
led->brightness_info.max);
+ brightness_range = led->brightness_info.max -
led->brightness_info.min;
+ if (brightness_range == 0) {
+ /* According to docs set value is not supported under
these
+ * conditions.
+ * LED interface enforces a set function.
+ */
+ led->cdev.brightness_set = led_brightness_set_dummy;
+ } else {
+ led->cdev.brightness_set_blocking =
led_brightness_set_sync;
+ }
+ led->cdev.brightness_get = led_brightness_get;
+
+ ret = devm_led_classdev_register(&hidpp->hid_dev->dev,
&led->cdev);
+ if (ret < 0)
+ goto register_fail;
+
+ hidpp->private_data = led;
+ return 0;
+register_fail:
+ mutex_destroy(&led->hw_change.mutex);
+cleanup:
+ devm_kfree(&hidpp->hid_dev->dev, led);
+ return ret;
+}
+
+static int hidpp_initialize_illumination(struct hidpp_device *hidpp)
+{
+ int ret;
+ unsigned long capabilities = hidpp->capabilities;
+
+ if (hidpp->protocol_major >= 2) {
+ u8 feature_index;
+ u8 feature_type;
+
+ ret = hidpp_root_get_feature(hidpp,
+
HIDPP_PAGE_ILLUMINATION_LIGHT,
+ &feature_index,
&feature_type);
+ if (!ret && !(feature_type & HIDPP_FEATURE_TYPE_HIDDEN)) {
+ hidpp->capabilities |=
+ HIDPP_CAPABILITY_ILLUMINATION_LIGHT;
+ hidpp->illumination_feature_index = feature_index;
+ hid_dbg(hidpp->hid_dev,
+ "Detected HID++ 2.0 Illumination
Light\n");
+ return 0;
+ }
+ }
+
+ if (hidpp->capabilities == capabilities)
+ hid_dbg(hidpp->hid_dev,
+ "Did not detect HID++ Illumination Light hardware
support\n");
+ return 0;
+}
+
+static void hidpp_remove_illumination(struct hidpp_device *hidpp)
+{
+ struct led_data *led = (struct led_data *)hidpp->private_data;
+
+ mutex_destroy(&led->hw_change.mutex);
+}
+
+static void hidpp_illumination_defered_event(struct hidpp_device *hidpp)
+{
+ bool has_hw_change, on;
+ int ret;
+ unsigned int led_brightness;
+ struct led_data *led = (struct led_data *)hidpp->private_data;
+
+ mutex_lock(&led->hw_change.mutex);
+
+ has_hw_change = led->hw_change.on == -1 &&
led->hw_change.brightness == -1;
+
+ if (!has_hw_change)
+ goto unlock;
+
+ if (led->hw_change.on == -1) {
+ ret = request_led_on(hidpp, led, &on);
+ if (ret)
+ goto reset_hw_change;
+ } else {
+ on = led->hw_change.on;
+ }
+
+ if (!on) {
+ led_brightness = LED_OFF;
+ goto notify_changed;
+ }
+
+ if (led->hw_change.brightness == -1) {
+ ret = request_led_brightness(hidpp, led, &led_brightness);
+ if (ret)
+ goto reset_hw_change;
+ } else {
+ led_brightness = device_to_led_brightness(led,
led->hw_change.brightness);
+ }
+
+notify_changed:
+ led_classdev_notify_brightness_hw_changed(
+ &led->cdev, led_brightness);
+reset_hw_change:
+ led->hw_change.on = -1;
+ led->hw_change.brightness = -1;
+unlock:
+ mutex_unlock(&led->hw_change.mutex);
+}
+
+static int hidpp20_illumination_raw_event(struct hidpp_device *hidpp, u8
*data,
+ int size)
+{
+ struct led_data *led = (struct led_data *)hidpp->private_data;
+ struct hidpp_report *report = (struct hidpp_report *)data;
+
+ switch (report->report_id) {
+ case REPORT_ID_HIDPP_LONG:
+ /* size is already checked in hidpp_raw_event.
+ * only leave long through
+ */
+ break;
+ default:
+ hid_err(hidpp->hid_dev, "%s:Unhandled report_id %u\n",
__func__, report->report_id);
+ return 0;
+ }
+
+ if (report->fap.feature_index !=
hidpp->illumination_feature_index)
+ return 0;
+
+ if (report->fap.funcindex_clientid ==
HIDPP_ILLUMINATION_EVENT_CHANGE) {
+ mutex_lock(&led->hw_change.mutex);
+ led->hw_change.on = report->fap.params[0] & BIT(0);
+ mutex_unlock(&led->hw_change.mutex);
+ return 0;
+ }
+
+ if (report->fap.funcindex_clientid ==
+ HIDPP_ILLUMINATION_EVENT_BRIGHTNESS_CHANGE) {
+ mutex_lock(&led->hw_change.mutex);
+ led->hw_change.brightness =
get_unaligned_be16(&report->fap.params[0]);
+ mutex_unlock(&led->hw_change.mutex);
+ return 0;
+ }
+
+ return 0;
+}
+
/*
--------------------------------------------------------------------------
*/
/* 0x2120: Hi-resolution scrolling
*/
/*
--------------------------------------------------------------------------
*/
@@ -3640,6 +4036,12 @@ static int hidpp_raw_hidpp_event(struct
hidpp_device *hidpp, u8 *data,
return ret;
}

+ if (hidpp->capabilities & HIDPP_CAPABILITY_ILLUMINATION_LIGHT) {
+ ret = hidpp20_illumination_raw_event(hidpp, data, size);
+ if (ret != 0)
+ return ret;
+ }
+
if (hidpp->quirks & HIDPP_QUIRK_HIDPP_WHEELS) {
ret = hidpp10_wheel_raw_event(hidpp, data, size);
if (ret != 0)
@@ -3964,6 +4366,7 @@ static void hidpp_connect_event(struct hidpp_device
*hidpp)

hidpp_initialize_battery(hidpp);
hidpp_initialize_hires_scroll(hidpp);
+ hidpp_initialize_illumination(hidpp);

/* forward current battery state */
if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
@@ -3986,6 +4389,14 @@ static void hidpp_connect_event(struct hidpp_device
*hidpp)
if (hidpp->capabilities & HIDPP_CAPABILITY_HI_RES_SCROLL)
hi_res_scroll_enable(hidpp);

+ if (hidpp->capabilities & HIDPP_CAPABILITY_ILLUMINATION_LIGHT) {
+ ret = register_led(hidpp);
+ if (ret) {
+ hid_err(hdev, "Registering leds failed.\n");
+ return;
+ }
+ }
+
if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) ||
hidpp->delayed_input)
/* if the input nodes are already created, we can stop now
*/
return;
@@ -4156,11 +4567,15 @@ static int hidpp_probe(struct hid_device *hdev,
const struct hid_device_id *id)
hid_warn(hdev, "Cannot allocate sysfs group for %s\n",
hdev->name);

- /*
- * Plain USB connections need to actually call start and open
- * on the transport driver to allow incoming data.
- */
- ret = hid_hw_start(hdev, 0);
+ if (hidpp->quirks & HIDPP_QUIRK_CLASS_SIMPLE_START) {
+ ret = hid_hw_start(hdev, HID_CLAIMED_DRIVER);
+ } else {
+ /*
+ * Plain USB connections need to actually call start and
open
+ * on the transport driver to allow incoming data.
+ */
+ ret = hid_hw_start(hdev, 0);
+ }
if (ret) {
hid_err(hdev, "hw start failed\n");
goto hid_hw_start_fail;
@@ -4211,6 +4626,10 @@ static int hidpp_probe(struct hid_device *hdev,
const struct hid_device_id *id)

hidpp_connect_event(hidpp);

+ /* Resetting HID node made communication with Glow break down*/
+ if (hidpp->quirks & HIDPP_QUIRK_CLASS_SIMPLE_START)
+ return 0;
+
/* Reset the HID node state */
hid_device_io_stop(hdev);
hid_hw_close(hdev);
@@ -4254,6 +4673,9 @@ static void hidpp_remove(struct hid_device *hdev)
if (!hidpp)
return hid_hw_stop(hdev);

+ if (hidpp->capabilities & HIDPP_CAPABILITY_ILLUMINATION_LIGHT)
+ hidpp_remove_illumination(hidpp);
+
sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);

hid_hw_stop(hdev);
@@ -4334,6 +4756,9 @@ static const struct hid_device_id hidpp_devices[] =
{
.driver_data = HIDPP_QUIRK_CLASS_G920 |
HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
{ /* Logitech G Pro Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
+ { /* Logitech Litra Glow over USB*/
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_LITRA_GLOW),
+ .driver_data = HIDPP_QUIRK_CLASS_SIMPLE_START |
HIDPP_QUIRK_FORCE_OUTPUT_REPORTS },

{ /* MX5000 keyboard over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb305),
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 0e9702c7f7d6..64c176b0ba88 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -494,6 +494,7 @@ static const struct hid_device_id
hid_have_special_driver[] = {
#endif
#if IS_ENABLED(CONFIG_HID_LOGITECH_HIDPP)
{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_G920_WHEEL) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_LITRA_GLOW) },
#endif
#if IS_ENABLED(CONFIG_HID_MAGICMOUSE)
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_MAGICMOUSE) },
--
2.34.1


2022-12-16 10:11:33

by Bastien Nocera

[permalink] [raw]
Subject: Re: [PATCH] hid: Support for Litra Glow

On Thu, 2022-12-15 at 22:09 +0100, Andreas Bergmeier wrote:
> Tries to implement as general support for Illumination Light as
> possible. Note that it is singular, which means by Logitech spec we
> are
> fine off with just handling one capability/device.

Your email client absolutely trashed the patch's indentation, it's
unreadable as-is.

> Implementation currently only exposes Brightness and On/Off controls.
> Does currently not expose Color Temperature because LEDs does not
> support it.
>
> Introduces HIDPP_QUIRK_CLASS_SIMPLE_START to prevent reconnect on
> startup. Could not get Glow to work with that.

I'd really rather we didn't introduce a new quirk, but instead fixed
the fact that we need to restart the HID transport to support 3 (!)
devices.

Would something like the attached patch work? I haven't tested it yet,
but if it works for you, I'll test it on the devices I have here.

Cheers


Attachments:
0001-HID-logitech-hidpp-Don-t-restart-communication-if-no.patch (2.76 kB)

2022-12-16 15:00:53

by Bastien Nocera

[permalink] [raw]
Subject: Re: [PATCH] hid: Support for Litra Glow

On Fri, 2022-12-16 at 10:53 +0100, Bastien Nocera wrote:
> On Thu, 2022-12-15 at 22:09 +0100, Andreas Bergmeier wrote:
> > Tries to implement as general support for Illumination Light as
> > possible. Note that it is singular, which means by Logitech spec we
> > are
> > fine off with just handling one capability/device.
>
> Your email client absolutely trashed the patch's indentation, it's
> unreadable as-is.
>
> > Implementation currently only exposes Brightness and On/Off
> > controls.
> > Does currently not expose Color Temperature because LEDs does not
> > support it.
> >
> > Introduces HIDPP_QUIRK_CLASS_SIMPLE_START to prevent reconnect on
> > startup. Could not get Glow to work with that.
>
> I'd really rather we didn't introduce a new quirk, but instead fixed
> the fact that we need to restart the HID transport to support 3 (!)
> devices.
>
> Would something like the attached patch work? I haven't tested it
> yet,
> but if it works for you, I'll test it on the devices I have here.

A tested version attached. I'll need to test it against a T650 before
sending it for review.


Attachments:
0001-HID-logitech-hidpp-Don-t-restart-communication-if-no.patch (3.02 kB)

2022-12-20 10:00:33

by Bastien Nocera

[permalink] [raw]
Subject: Re: [PATCH] hid: Support for Litra Glow

On Fri, 2022-12-16 at 15:27 +0100, Bastien Nocera wrote:
> On Fri, 2022-12-16 at 10:53 +0100, Bastien Nocera wrote:
> > On Thu, 2022-12-15 at 22:09 +0100, Andreas Bergmeier wrote:
> > > Tries to implement as general support for Illumination Light as
> > > possible. Note that it is singular, which means by Logitech spec
> > > we
> > > are
> > > fine off with just handling one capability/device.
> >
> > Your email client absolutely trashed the patch's indentation, it's
> > unreadable as-is.
> >
> > > Implementation currently only exposes Brightness and On/Off
> > > controls.
> > > Does currently not expose Color Temperature because LEDs does not
> > > support it.
> > >
> > > Introduces HIDPP_QUIRK_CLASS_SIMPLE_START to prevent reconnect on
> > > startup. Could not get Glow to work with that.
> >
> > I'd really rather we didn't introduce a new quirk, but instead
> > fixed
> > the fact that we need to restart the HID transport to support 3 (!)
> > devices.
> >
> > Would something like the attached patch work? I haven't tested it
> > yet,
> > but if it works for you, I'll test it on the devices I have here.
>
> A tested version attached. I'll need to test it against a T650 before
> sending it for review.

I've tested it with a T650, and it works, so I sent it for review.

Would be great to know if it helps simplify your driver.

Cheers