Re: Litra Glow on Linux

Re: Litra Glow on Linux

[Benjamin Tissoires]

[adding linux-leds in Cc]

On Sat, Oct 29, 2022 at 9:21 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
> Sorry, another set of questions - seems like I am a bit dense.
>
> On Thu, 27 Oct 2022 at 11:44, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > It's just Logitech's common HID protocol. The advantage is that if
> > Logitech reuses the feature on a different hardware, we won't have to
> > implement anything new in the kernel.
> Started implementing some illumination code but will take a while
> until I figure out the driver I think.
>
> > But from where you are now, you should probably be able to implement
> > the basic on/off feature by looking at the function 0x1000 in the
> > hid-logitech-hidpp code:
> > - you need define a few macros for your functionality (the class, the
> > commands, the events)
> So my approach would be to identify the GLOW device and then at some
> later point create the
> illumination state and from there only handle common illumination.

For identifying the GLOW device you should be adding an id in the
table of hid-logitech-hidpp, with a driver data that tells the driver
to look for 0x1990.

>
> > - you need to add a hook in connect_event to register the led class
> > device that will hook on to the actual LED of the device
> I did read all the LED specs/headers that I could find and from what I
> have seen all you can currently do with this interface is control
> brightness. There seems to be no way of controlling the Color
> temperature, though.

Leds can be multicolor. See drivers/hid/hid-playstation.c for an example.

So I think you should be able to give a color to the LED that can be
controlled as a separate channel, different from the brightness. The
LEDs folks will know better.

> So either this then would have to be exposed as a special device or
> get handled entirely in userspace.
> The latter seems to work against "implementing illumination handling
> once in driver and reusing it".

I would rather have it handled as a standard LED class, a colored LED
one. There might be a special structure for colored LEDs, and if not
you should probably be able to use a multi-color led with just one
color channel.

>
> > [0] https://pwr-solaar.github.io/Solaar
> > [1] https://github.com/pwr-Solaar/Solaar/blob/master/docs/hidpp-documentation.txt
> Thanks. Never would have found the specs on my own.
> That said - I read x1990 spec and tried to access getIllumination from
> userspace.

Oh, good point, Nestor added that spec back in May :)
Thanks, Nestor!

> The spec seems a bit vague for my limited experience level.
> For example I have not yet figured out what the communication (bytes)
> difference between _getIllumination()_ and _illuminationChangedEvent_
> is.
> What seems to work is accessing events:
>
> So I tried:
> ```c
>
> #define LONG_REPORT_ID 0x11
>
>     struct hiddev_usage_ref_multi multi;
>     memset(&multi, 0, sizeof(multi));
>     multi.uref.report_type = HID_REPORT_TYPE_INPUT;
>     multi.uref.report_id = LONG_REPORT_ID;
>     multi.uref.field_index = 0x0;
>     multi.uref.usage_index = 0x03;
>     multi.uref.usage_code = 0xff430002;
>     multi.num_values = 1;
>
>     if (ioctl(fd, HIDIOCGUSAGES, &multi) < 0)
>     {
>         perror("HIDIOCGUSAGES getIllumination");
>         return -11;
>     }
>
>     printf("VALUE: %0x\n", multi.values[0]);
>
> ```
> Which seems to report the illumination state until I press another
> hardware button. So this seems to access the last event, which seems
> to be _illuminationChangedEvent_ in my case.
> No idea currently how to get _getIllumination_ to work.

IIRC, HIDIOCGUSAGES doesn't do anything with the device, you are
querying the kernel of its current state. Which explains why you are
not getting the current state, but the previous known state.

What you need to do, is actually emit a command to the device
(completely untested, of course):
---
#define APPLICATION_ID 0x06 // can be anything between 0x01  and 0xF,
0x1 being the ID from the kernel

#define HIDPP_1990_GET_ILLUMINATION 0x00
#define HIDPP_1990_SET_ILLUMINATION 0x10
#define ....

unsigned char cmd = HIDPP_1990_GET_ILLUMINATION | APPLICATION_ID;
unsigned char buf[20] = {0x11, 0xff, 0x04}; // HIDPP ID, DEVICE INDEX,
FEATURE INDEX in the feature table

buf[3] = cmd;
write(fd, buf, sizeof(buf));
memset(buf, sizeof(buf), 0);
while (buf[3] != cmd)
  read(fd, buf, sizeof(buf));

printf("VALUE: %0x\n", buf[4]);
---

It is important to set an application id so you can differentiate your
requests from the events (the application ID from an event is always
0).

With that in mind, you should now be able to understand why you had to
send {0x11, 0xff, 0x04, 0x10, 0x01} to set the illumination. And
again, for being sure you can get the feedback you should use 0x16 as
the fourth byte here, so you can detect your answer.

I hope it makes more sense now.

Cheers,
Benjamin

Re: Litra Glow on Linux

[Andreas Bergmeier]

See my first stab at the problem below.

On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Sat, Oct 29, 2022 at 9:21 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
> >
> > Sorry, another set of questions - seems like I am a bit dense.
> >
> > On Thu, 27 Oct 2022 at 11:44, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > It's just Logitech's common HID protocol. The advantage is that if
> > > Logitech reuses the feature on a different hardware, we won't have to
> > > implement anything new in the kernel.
> > Started implementing some illumination code but will take a while
> > until I figure out the driver I think.
> >
> > > But from where you are now, you should probably be able to implement
> > > the basic on/off feature by looking at the function 0x1000 in the
> > > hid-logitech-hidpp code:
> > > - you need define a few macros for your functionality (the class, the
> > > commands, the events)
> > So my approach would be to identify the GLOW device and then at some
> > later point create the
> > illumination state and from there only handle common illumination.
>
> For identifying the GLOW device you should be adding an id in the
> table of hid-logitech-hidpp, with a driver data that tells the driver
> to look for 0x1990.
Currently compiles and should have all handling that seemed necessary to me.
Will now try to get the compiled kernel running on an Ubuntu - that
might take a while.
RFC:
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 71a9c258a20b..bdc2949d1be5 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>
@@ -99,6 +100,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))
@@ -206,7 +208,10 @@ struct hidpp_device {
struct hidpp_battery battery;
struct hidpp_scroll_counter vertical_wheel_counter;
- u8 wireless_feature_index;
+ union {
+ u8 wireless_feature_index;
+ u8 illumination_feature_index;
+ };
};
/* HID++ 1.0 error codes */
@@ -862,6 +867,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)
{
@@ -1729,6 +1736,294 @@ 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
+
+#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_NON_LINEAR_LEVELS 0x04
+#define HIDPP_ILLUMINATION_CAP_LINEAR_LEVELS 0x02
+#define HIDPP_ILLUMINATION_CAP_EVENTS 0x01
+
+struct led_data {
+ struct led_classdev cdev;
+ struct hidpp_device *drv_data;
+ struct hid_device *hdev;
+ struct work_struct work;
+ u16 feature_index;
+ bool on;
+ unsigned int brightness;
+ unsigned int color_temperature;
+ bool removed;
+};
+
+static enum led_brightness led_brightness_get(struct led_classdev *led_cdev) {
+ struct led_data *led = container_of(led_cdev, struct led_data,
+ cdev);
+
+ if (!led->on) {
+ return LED_OFF;
+ }
+
+ return led->brightness + 1;
+}
+
+static void led_brightness_set(struct led_classdev *led_cdev, enum
led_brightness brightness) {
+ struct led_data *led = container_of(led_cdev, struct led_data,
+ cdev);
+ led->on = brightness != 0;
+ if (led->on) {
+ led->brightness = brightness - 1;
+ }
+
+ schedule_work(&led->work);
+}
+
+static unsigned int led_color_temperature_get(struct led_classdev *led_cdev) {
+ struct led_data *led = container_of(led_cdev, struct led_data,
+ cdev);
+ return led->color_temperature;
+}
+
+static void led_color_temperature_set(struct led_classdev *led_cdev,
unsigned int color_temperature) {
+ struct led_data *led = container_of(led_cdev, struct led_data,
+ cdev);
+
+ led->color_temperature = color_temperature;
+ schedule_work(&led->work);
+}
+
+static void led_work(struct work_struct *work)
+{
+ struct hidpp_device *hidpp;
+ u8 params[20];
+ int ret;
+ struct hidpp_report report;
+ struct led_data *led = container_of(work, struct led_data, work);
+
+ if (led->removed)
+ return;
+
+ hidpp = led->drv_data;
+
+ memset(&params, 0, sizeof(params)/sizeof(*params));
+ if (!led->on) {
+ ret = hidpp_send_fap_command_sync(hidpp,
hidpp->illumination_feature_index, HIDPP_ILLUMINATION_FUNC_SET,
params, 20, &report);
+ if (ret) {
+ return;
+ }
+ }
+
+ ret = hidpp_send_fap_command_sync(hidpp,
hidpp->illumination_feature_index,
HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS, params, 20, &report);
+ if (ret) {
+ return;
+ }
+
+ ret = hidpp_send_fap_command_sync(hidpp,
hidpp->illumination_feature_index,
HIDPP_ILLUMINATION_FUNC_SET_COLOR_TEMPERATURE, params, 20, &report);
+ if (ret) {
+ return;
+ }
+}
+
+struct control_info{
+ u16 min;
+ u16 max;
+ u16 res;
+ u8 capabilities;
+ u8 max_levels;
+};
+
+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)
+ return ret;
+
+ info->capabilities = resp.fap.params[0];
+ info->min = be16_to_cpu(resp.fap.params[1] << 8 | resp.fap.params[2] << 0);
+ info->max = be16_to_cpu(resp.fap.params[3] << 8 | resp.fap.params[4] << 0);
+ info->res = be16_to_cpu(resp.fap.params[5] << 8 | resp.fap.params[6] << 0);
+ info->max_levels = resp.fap.params[7];
+ 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)
+ return ret;
+
+ info->capabilities = resp.fap.params[0];
+ info->min = be16_to_cpu(resp.fap.params[1] << 8 | resp.fap.params[2] << 0);
+ info->max = be16_to_cpu(resp.fap.params[3] << 8 | resp.fap.params[4] << 0);
+ info->res = be16_to_cpu(resp.fap.params[5] << 8 | resp.fap.params[6] << 0);
+ info->max_levels = resp.fap.params[7];
+ return 0;
+}
+
+static int register_led(struct hidpp_device *hidpp)
+{
+ struct control_info bi, cti;
+ struct led_data *ld;
+ int ret = get_brightness_info_sync(hidpp, &bi);
+ if (ret)
+ return ret;
+
+ if (bi.res != 1) {
+ // FAIL - not supported
+ return -1;
+ }
+
+ ret = get_color_temperature_info_sync(hidpp, &cti);
+ if (ret)
+ return ret;
+
+ if (cti.res != 1) {
+ // FAIL - not supported
+ return -1;
+ }
+
+ ld = devm_kzalloc(&hidpp->hid_dev->dev,
+ sizeof(struct led_data),
+ GFP_KERNEL);
+ if (!ld)
+ return -ENOMEM;
+
+ ld->drv_data = hidpp;
+ ld->removed = false;
+ ld->cdev.name = hidpp->name;
+ ld->cdev.flags = LED_HW_PLUGGABLE | LED_BRIGHT_HW_CHANGED |
LED_COLOR_TEMP_HW_CHANGED;
+ /* 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
+ */
+ ld->cdev.max_brightness = bi.max - bi.min + 1;
+ if (bi.max == bi.min) {
+ /* According to docs set value is not supported under these
+ * conditions
+ */
+ ld->cdev.brightness_set = NULL;
+ } else {
+ ld->cdev.brightness_set = led_brightness_set;
+ }
+ ld->cdev.brightness_get = led_brightness_get;
+
+ ld->cdev.min_color_temperature = cti.min;
+ ld->cdev.max_color_temperature = cti.max;
+ if (bi.max == bi.min) {
+ /* According to docs set value is not supported under these
+ * conditions
+ */
+ ld->cdev.color_temperature_set = NULL;
+ } else {
+ ld->cdev.color_temperature_set = led_color_temperature_set;
+ }
+ ld->cdev.color_temperature_get = led_color_temperature_get;
+ INIT_WORK(&ld->work, led_work);
+ ret = devm_led_classdev_register(&hidpp->hid_dev->dev, &ld->cdev);
+ if (ret < 0) {
+ /* No need to have this still around */
+ devm_kfree(&hidpp->hid_dev->dev, ld);
+ }
+ hidpp->private_data = ld;
+ return 0;
+}
+
+/* TODO: Do we need this
+static int unregister_led(struct hidpp_device *hidpp,) {
+ devm_led_classdev_unregister(&hdev->dev, &ld->cdev);
+}
+*/
+
+static int hidpp20_illumination_raw_event(struct hidpp_device *hidpp,
u8 *data, int size) {
+
+ struct led_data *led;
+ 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:
+ return 0;
+ }
+
+ if (report->fap.feature_index != hidpp->illumination_feature_index) {
+ return 0;
+ }
+
+ led = (struct led_data*)hidpp->private_data;
+
+ if (report->fap.funcindex_clientid == HIDPP_ILLUMINATION_EVENT_CHANGE) {
+ led->on = report->fap.params[0] & 0x1;
+ if (led->on )
+ led_classdev_notify_brightness_hw_changed(&led->cdev, led->brightness + 1);
+ else
+ led_classdev_notify_brightness_hw_changed(&led->cdev, LED_OFF);
+ return 0;
+ }
+
+ if (report->fap.funcindex_clientid ==
HIDPP_ILLUMINATION_EVENT_BRIGHTNESS_CHANGE) {
+ u16 brightness = be16_to_cpu(report->fap.params[0] << 8 |
report->fap.params[1] << 0);
+ led->brightness = brightness;
+ led_classdev_notify_brightness_hw_changed(&led->cdev, led->brightness + 1);
+ return 0;
+ }
+
+ if (report->fap.funcindex_clientid ==
HIDPP_ILLUMINATION_EVENT_COLOR_TEMPERATURE_CHANGE) {
+ u16 color_temperature = be16_to_cpu(report->fap.params[0] << 8 |
report->fap.params[1] << 0);
+ led->color_temperature = color_temperature;
+ led_classdev_notify_color_temperature_hw_changed(&led->cdev,
led->color_temperature);
+ return 0;
+ }
+
+ return 0;
+}
+
+static int hidpp20_illumination_connect(struct hidpp_device *hidpp) {
+
+ u8 feature_index, feature_type;
+ int ret = hidpp_root_get_feature(hidpp,
+ HIDPP_PAGE_ILLUMINATION_LIGHT,
+ &feature_index,
+ &feature_type);
+ if (ret)
+ return ret;
+
+ if (feature_type & HIDPP_FEATURE_TYPE_HIDDEN) {
+ /* According to docs the host should ignore this feature */
+ return -ENOENT;
+ }
+
+ hidpp->illumination_feature_index = feature_index;
+
+ ret = register_led(hidpp);
+ if (!ret)
+ hidpp->capabilities |= HIDPP_CAPABILITY_ILLUMINATION_LIGHT;
+
+ return ret;
+}
+
/* -------------------------------------------------------------------------- */
/* 0x2120: Hi-resolution scrolling */
/* -------------------------------------------------------------------------- */
@@ -3648,6 +3943,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)
@@ -4013,6 +4314,8 @@ static void hidpp_connect_event(struct
hidpp_device *hidpp)
}
hidpp->delayed_input = input;
+
+ ret = hidpp20_illumination_connect(hidpp);
}
static DEVICE_ATTR(builtin_power_supply, 0000, NULL, NULL);


>
> >
> > > - you need to add a hook in connect_event to register the led class
> > > device that will hook on to the actual LED of the device
> > I did read all the LED specs/headers that I could find and from what I
> > have seen all you can currently do with this interface is control
> > brightness. There seems to be no way of controlling the Color
> > temperature, though.
>
> Leds can be multicolor. See drivers/hid/hid-playstation.c for an example.
>
> So I think you should be able to give a color to the LED that can be
> controlled as a separate channel, different from the brightness. The
> LEDs folks will know better.
>
> > So either this then would have to be exposed as a special device or
> > get handled entirely in userspace.
> > The latter seems to work against "implementing illumination handling
> > once in driver and reusing it".
>
> I would rather have it handled as a standard LED class, a colored LED
> one. There might be a special structure for colored LEDs, and if not
> you should probably be able to use a multi-color led with just one
> color channel.
>
> >
> > > [0] https://pwr-solaar.github.io/Solaar
> > > [1] https://github.com/pwr-Solaar/Solaar/blob/master/docs/hidpp-documentation.txt
> > Thanks. Never would have found the specs on my own.
> > That said - I read x1990 spec and tried to access getIllumination from
> > userspace.
>
> Oh, good point, Nestor added that spec back in May :)
> Thanks, Nestor!
>
> > The spec seems a bit vague for my limited experience level.
> > For example I have not yet figured out what the communication (bytes)
> > difference between _getIllumination()_ and _illuminationChangedEvent_
> > is.
> > What seems to work is accessing events:
> >
> > So I tried:
> > ```c
> >
> > #define LONG_REPORT_ID 0x11
> >
> >     struct hiddev_usage_ref_multi multi;
> >     memset(&multi, 0, sizeof(multi));
> >     multi.uref.report_type = HID_REPORT_TYPE_INPUT;
> >     multi.uref.report_id = LONG_REPORT_ID;
> >     multi.uref.field_index = 0x0;
> >     multi.uref.usage_index = 0x03;
> >     multi.uref.usage_code = 0xff430002;
> >     multi.num_values = 1;
> >
> >     if (ioctl(fd, HIDIOCGUSAGES, &multi) < 0)
> >     {
> >         perror("HIDIOCGUSAGES getIllumination");
> >         return -11;
> >     }
> >
> >     printf("VALUE: %0x\n", multi.values[0]);
> >
> > ```
> > Which seems to report the illumination state until I press another
> > hardware button. So this seems to access the last event, which seems
> > to be _illuminationChangedEvent_ in my case.
> > No idea currently how to get _getIllumination_ to work.
>
> IIRC, HIDIOCGUSAGES doesn't do anything with the device, you are
> querying the kernel of its current state. Which explains why you are
> not getting the current state, but the previous known state.
>
> What you need to do, is actually emit a command to the device
> (completely untested, of course):
> ---
> #define APPLICATION_ID 0x06 // can be anything between 0x01  and 0xF,
> 0x1 being the ID from the kernel
>
> #define HIDPP_1990_GET_ILLUMINATION 0x00
> #define HIDPP_1990_SET_ILLUMINATION 0x10
> #define ....
>
> unsigned char cmd = HIDPP_1990_GET_ILLUMINATION | APPLICATION_ID;
> unsigned char buf[20] = {0x11, 0xff, 0x04}; // HIDPP ID, DEVICE INDEX,
> FEATURE INDEX in the feature table
>
> buf[3] = cmd;
> write(fd, buf, sizeof(buf));
> memset(buf, sizeof(buf), 0);
> while (buf[3] != cmd)
>   read(fd, buf, sizeof(buf));
>
> printf("VALUE: %0x\n", buf[4]);
> ---
>
> It is important to set an application id so you can differentiate your
> requests from the events (the application ID from an event is always
> 0).
>
> With that in mind, you should now be able to understand why you had to
> send {0x11, 0xff, 0x04, 0x10, 0x01} to set the illumination. And
> again, for being sure you can get the feedback you should use 0x16 as
> the fourth byte here, so you can detect your answer.
>
> I hope it makes more sense now.
>
> Cheers,
> Benjamin
>

Re: Litra Glow on Linux

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

Hi!

> See my first stab at the problem below.

> index 71a9c258a20b..bdc2949d1be5 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>

> @@ -206,7 +208,10 @@ struct hidpp_device {
> struct hidpp_battery battery;
> struct hidpp_scroll_counter vertical_wheel_counter;
> - u8 wireless_feature_index;
> + union {
> + u8 wireless_feature_index;
> + u8 illumination_feature_index;
> + };
> };
> /* HID++ 1.0 error codes */

Something very wrong is going on with your email -- all spaces are
deleted or something. You'll need to adjust your mail settings.

Best regards,
								Pavel
								
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Litra Glow on Linux

[Andreas Bergmeier]

Finally I have an environment where I can test my kernel code.

On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> For identifying the GLOW device you should be adding an id in the
> table of hid-logitech-hidpp, with a driver data that tells the driver
> to look for 0x1990.
>
> >
> > > - you need to add a hook in connect_event to register the led class
> > > device that will hook on to the actual LED of the device
Sadly my tests did not go very far. The code fails already when
calling the `probe` callback (`hidpp_probe`).
When it calls into `hidpp_root_get_protocol_version` it seems to
receive `HIDPP_ERROR_RESOURCE_ERROR`.
Which then leads to an error message: Device not connected
Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
documentation what it means in code.
From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
devices. Thus I am wondering how the code knows
that it is a problem with connectivity. Couldn't it also mean that the
device is not supporting getting the protocol version?
And why is protocol version only enforced for non unifying devices?

Re: Litra Glow on Linux

[Andreas Bergmeier]

On Wed, 9 Nov 2022 at 21:27, Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
> Finally I have an environment where I can test my kernel code.
>
> On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > For identifying the GLOW device you should be adding an id in the
> > table of hid-logitech-hidpp, with a driver data that tells the driver
> > to look for 0x1990.
> >
> > >
> > > > - you need to add a hook in connect_event to register the led class
> > > > device that will hook on to the actual LED of the device
> Sadly my tests did not go very far. The code fails already when
> calling the `probe` callback (`hidpp_probe`).
> When it calls into `hidpp_root_get_protocol_version` it seems to
> receive `HIDPP_ERROR_RESOURCE_ERROR`.
> Which then leads to an error message: Device not connected
> Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
> documentation what it means in code.
> From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
> devices. Thus I am wondering how the code knows
> that it is a problem with connectivity. Couldn't it also mean that the
> device is not supporting getting the protocol version?
> And why is protocol version only enforced for non unifying devices?
Also, looking into `supported_reports` turned out to be 2 (very long).
Inside of `hidpp_root_get_protocol_version` it does upgrade SHORT to
LONG if the former is not supported.
On a whim I then added upgrade of LONG to VERY LONG if the former is
not supported. Sadly, the results stayed the same.

Re: Litra Glow on Linux

[Benjamin Tissoires]

On Thu, Nov 10, 2022 at 4:29 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
> On Wed, 9 Nov 2022 at 21:27, Andreas Bergmeier <abergmeier@gmx.net> wrote:
> >
> > Finally I have an environment where I can test my kernel code.
> >
> > On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > For identifying the GLOW device you should be adding an id in the
> > > table of hid-logitech-hidpp, with a driver data that tells the driver
> > > to look for 0x1990.
> > >
> > > >
> > > > > - you need to add a hook in connect_event to register the led class
> > > > > device that will hook on to the actual LED of the device
> > Sadly my tests did not go very far. The code fails already when
> > calling the `probe` callback (`hidpp_probe`).
> > When it calls into `hidpp_root_get_protocol_version` it seems to
> > receive `HIDPP_ERROR_RESOURCE_ERROR`.
> > Which then leads to an error message: Device not connected
> > Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
> > documentation what it means in code.
> > From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
> > devices. Thus I am wondering how the code knows
> > that it is a problem with connectivity.

From the top of my memory, this was told to us that this is the way we
detect if the device was connected or not in the unifying case. Though
in your case, it's a USB device, so there is no such thing as "not
connected"...

> > Couldn't it also mean that the
> > device is not supporting getting the protocol version?

Probably. What happens if you comment out that protocol version
request and force connected to be true?

> > And why is protocol version only enforced for non unifying devices?

Unifying devices are wireless, and when we probe the device, we are
actually talking to the receiver. So The device might not be
connected, and we should wait for the device to be present and not
reject it. On non unifying devices, if the device is not connected,
this likely means that the device is not behaving properly, and so we
can not handle it in the driver.

In your case though, it would be interesting to know if we should
bypass that verification.

> Also, looking into `supported_reports` turned out to be 2 (very long).

Oops, you mistook the bit definition with the value:
#define HIDPP_REPORT_SHORT_SUPPORTED  BIT(0)  -> value of 1
#define HIDPP_REPORT_LONG_SUPPORTED  BIT(1)  -> value of 2
#define HIDPP_REPORT_VERY_LONG_SUPPORTED  BIT(2)  -> value of 4

Which is coherent with what your device exports: only one report ID of
value 0x11, HIDPP_REPORT_LONG.

> Inside of `hidpp_root_get_protocol_version` it does upgrade SHORT to
> LONG if the former is not supported.

Yep, this should be good for your device.

> On a whim I then added upgrade of LONG to VERY LONG if the former is
> not supported. Sadly, the results stayed the same.
>

And this is expected because you don't have VERY_LONG support on your device.

Cheers,
Benjamin

Re: Litra Glow on Linux

[Andreas Bergmeier]

On Thu, 10 Nov 2022, Benjamin Tissoires wrote:

> On Thu, Nov 10, 2022 at 4:29 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
> >
> > On Wed, 9 Nov 2022 at 21:27, Andreas Bergmeier <abergmeier@gmx.net> wrote:
> > >
> > > Finally I have an environment where I can test my kernel code.
> > >
> > > On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > > For identifying the GLOW device you should be adding an id in the
> > > > table of hid-logitech-hidpp, with a driver data that tells the driver
> > > > to look for 0x1990.
> > > >
> > > > >
> > > > > > - you need to add a hook in connect_event to register the led class
> > > > > > device that will hook on to the actual LED of the device
> > > Sadly my tests did not go very far. The code fails already when
> > > calling the `probe` callback (`hidpp_probe`).
> > > When it calls into `hidpp_root_get_protocol_version` it seems to
> > > receive `HIDPP_ERROR_RESOURCE_ERROR`.
> > > Which then leads to an error message: Device not connected
> > > Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
> > > documentation what it means in code.
> > > From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
> > > devices. Thus I am wondering how the code knows
> > > that it is a problem with connectivity.
>
> From the top of my memory, this was told to us that this is the way we
> detect if the device was connected or not in the unifying case. Though
> in your case, it's a USB device, so there is no such thing as "not
> connected"...
So isn't the current error handling at a minimum misleading?


> > > Couldn't it also mean that the
> > > device is not supporting getting the protocol version?
>
> Probably. What happens if you comment out that protocol version
> request and force connected to be true?
Well I went the other way around. I had a look at the hidpp utility
sources:
https://github.com/cvuchener/hidpp/blob/057407fbb7248bbc6cefcfaa860758d0711c01b9/src/libhidpp/hidpp/Device.cpp#L82
Which seems to do a similar thing. From the top of my head the only
difference seems to be that they are sending `0x1` as a ping value instead
of `0x5a`. Might give that a shot next.
Anyway hidpp-list-features successfully reads the protocol version in
userspace (4, 2) as seen here:
https://github.com/abergmeier/litra_glow_linux/blob/main/hidpp-list-features

> In your case though, it would be interesting to know if we should
> bypass that verification.
Since reading the protocol version seems generally possible I think we
should understand what logitech-hidpp does wrong first.


> > Also, looking into `supported_reports` turned out to be 2 (very long).
>
> Oops, you mistook the bit definition with the value:
> #define HIDPP_REPORT_SHORT_SUPPORTED  BIT(0)  -> value of 1
> #define HIDPP_REPORT_LONG_SUPPORTED  BIT(1)  -> value of 2
> #define HIDPP_REPORT_VERY_LONG_SUPPORTED  BIT(2)  -> value of 4
Ah indeed, thx.

> And this is expected because you don't have VERY_LONG support on your device.
True. The question remains whether the upgrade from LONG to VERY_LONG
could be needed should a device only support VERY_LONG.

Re: Litra Glow on Linux

[Benjamin Tissoires]

On Thu, Nov 10, 2022 at 1:24 PM Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
>
>
> On Thu, 10 Nov 2022, Benjamin Tissoires wrote:
>
> > On Thu, Nov 10, 2022 at 4:29 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
> > >
> > > On Wed, 9 Nov 2022 at 21:27, Andreas Bergmeier <abergmeier@gmx.net> wrote:
> > > >
> > > > Finally I have an environment where I can test my kernel code.
> > > >
> > > > On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
> > > > <benjamin.tissoires@redhat.com> wrote:
> > > > > For identifying the GLOW device you should be adding an id in the
> > > > > table of hid-logitech-hidpp, with a driver data that tells the driver
> > > > > to look for 0x1990.
> > > > >
> > > > > >
> > > > > > > - you need to add a hook in connect_event to register the led class
> > > > > > > device that will hook on to the actual LED of the device
> > > > Sadly my tests did not go very far. The code fails already when
> > > > calling the `probe` callback (`hidpp_probe`).
> > > > When it calls into `hidpp_root_get_protocol_version` it seems to
> > > > receive `HIDPP_ERROR_RESOURCE_ERROR`.
> > > > Which then leads to an error message: Device not connected
> > > > Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
> > > > documentation what it means in code.
> > > > From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
> > > > devices. Thus I am wondering how the code knows
> > > > that it is a problem with connectivity.
> >
> > From the top of my memory, this was told to us that this is the way we
> > detect if the device was connected or not in the unifying case. Though
> > in your case, it's a USB device, so there is no such thing as "not
> > connected"...
> So isn't the current error handling at a minimum misleading?
>

Maybe, but this is the first time we have that error on a non wireless
device... so it is not for the supported devices :)

>
> > > > Couldn't it also mean that the
> > > > device is not supporting getting the protocol version?
> >
> > Probably. What happens if you comment out that protocol version
> > request and force connected to be true?
> Well I went the other way around. I had a look at the hidpp utility
> sources:
> https://github.com/cvuchener/hidpp/blob/057407fbb7248bbc6cefcfaa860758d0711c01b9/src/libhidpp/hidpp/Device.cpp#L82
> Which seems to do a similar thing. From the top of my head the only
> difference seems to be that they are sending `0x1` as a ping value instead
> of `0x5a`. Might give that a shot next.
> Anyway hidpp-list-features successfully reads the protocol version in
> userspace (4, 2) as seen here:
> https://github.com/abergmeier/litra_glow_linux/blob/main/hidpp-list-features

Hmm... It would seem wrong for me if the firmware suddenly expects to
have a specific ping value.
If it works in userspace, it might also mean that the timing is not
right and we are talking to the device too early, and it can't answer
yet.

>
> > In your case though, it would be interesting to know if we should
> > bypass that verification.
> Since reading the protocol version seems generally possible I think we
> should understand what logitech-hidpp does wrong first.
>
>
> > > Also, looking into `supported_reports` turned out to be 2 (very long).
> >
> > Oops, you mistook the bit definition with the value:
> > #define HIDPP_REPORT_SHORT_SUPPORTED  BIT(0)  -> value of 1
> > #define HIDPP_REPORT_LONG_SUPPORTED  BIT(1)  -> value of 2
> > #define HIDPP_REPORT_VERY_LONG_SUPPORTED  BIT(2)  -> value of 4
> Ah indeed, thx.
>
> > And this is expected because you don't have VERY_LONG support on your device.
> True. The question remains whether the upgrade from LONG to VERY_LONG
> could be needed should a device only support VERY_LONG.
>

I don't think we want that. AFAICT, devices supporting VERY_LONG are
not very common, and also the length of the report makes it not
convenient for the generic protocol to be used there. I am not in the
head of Logitech's engineers, but removing the 20 bytes long report in
favor of 64 bytes long seems a little bit overkill (the 7 bytes long
is a different story).

And given that we generally add manual support of new devices, I think
we are safe not implementing something we haven't seen in the wild.

Cheers,
Benjamin

Re: Litra Glow on Linux

[Andreas Bergmeier]

So after some tinkering I have code now that succeeds in retrieving state
via sending reports once. After that all following sends time out.
I am at a loss what I am doing wrong, tbh. RFC below.

On Thu, 10 Nov 2022, Benjamin Tissoires wrote:

>
> >
> > I had a look at the hidpp utility
> > sources:
> > https://github.com/cvuchener/hidpp/blob/057407fbb7248bbc6cefcfaa860758d0711c01b9/src/libhidpp/hidpp/Device.cpp#L82
> > Which seems to do a similar thing. From the top of my head the only
> > difference seems to be that they are sending `0x1` as a ping value instead
> > of `0x5a`. Might give that a shot next.
> > Anyway hidpp-list-features successfully reads the protocol version in
> > userspace (4, 2) as seen here:
> > https://github.com/abergmeier/litra_glow_linux/blob/main/hidpp-list-features
>
> Hmm... It would seem wrong for me if the firmware suddenly expects to
> have a specific ping value.
> If it works in userspace, it might also mean that the timing is not
> right and we are talking to the device too early, and it can't answer
> yet.
I needed to set some specific quirk flags to make communication work. See
below.

RFC on current PATCH:
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index dad953f66996..78265f7235ce 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -856,6 +856,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 71a9c258a20b..949fd09d2b43 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>
@@ -99,6 +100,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))

@@ -206,7 +208,10 @@ struct hidpp_device {
 	struct hidpp_battery battery;
 	struct hidpp_scroll_counter vertical_wheel_counter;

-	u8 wireless_feature_index;
+	union {
+		u8 wireless_feature_index;
+		u8 illumination_feature_index;
+	};
 };

 /* HID++ 1.0 error codes */
@@ -355,15 +360,16 @@ static int hidpp_send_fap_command_sync(struct
hidpp_device *hidpp,
 }

 static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
-	u8 report_id, u8 sub_id, u8 reg_address, u8 *params, int
param_count,
+	u8 sub_id, u8 reg_address, u8 *params, int param_count,
 	struct hidpp_report *response)
 {
 	struct hidpp_report *message;
 	int ret, max_count;
+	u8 report_id;

-	/* Send as long report if short reports are not supported. */
-	if (report_id == REPORT_ID_HIDPP_SHORT &&
-	    !(hidpp_dev->supported_reports &
HIDPP_REPORT_SHORT_SUPPORTED))
+	if (hidpp_dev->supported_reports & HIDPP_REPORT_SHORT_SUPPORTED)
+		report_id = REPORT_ID_HIDPP_SHORT;
+	else
 		report_id = REPORT_ID_HIDPP_LONG;

 	switch (report_id) {
@@ -544,7 +550,6 @@ static int hidpp10_set_register(struct hidpp_device
*hidpp_dev,
 	u8 params[3] = { 0 };

 	ret = hidpp_send_rap_command_sync(hidpp_dev,
-					  REPORT_ID_HIDPP_SHORT,
 					  HIDPP_GET_REGISTER,
 					  register_address,
 					  NULL, 0, &response);
@@ -557,7 +562,6 @@ static int hidpp10_set_register(struct hidpp_device
*hidpp_dev,
 	params[byte] |= value & mask;

 	return hidpp_send_rap_command_sync(hidpp_dev,
-					   REPORT_ID_HIDPP_SHORT,
 					   HIDPP_SET_REGISTER,
 					   register_address,
 					   params, 3, &response);
@@ -653,7 +657,6 @@ static int hidpp10_query_battery_status(struct
hidpp_device *hidpp)
 	int ret, status;

 	ret = hidpp_send_rap_command_sync(hidpp,
-					REPORT_ID_HIDPP_SHORT,
 					HIDPP_GET_REGISTER,
 					HIDPP_REG_BATTERY_STATUS,
 					NULL, 0, &response);
@@ -705,7 +708,6 @@ static int hidpp10_query_battery_mileage(struct
hidpp_device *hidpp)
 	int ret, status;

 	ret = hidpp_send_rap_command_sync(hidpp,
-					REPORT_ID_HIDPP_SHORT,
 					HIDPP_GET_REGISTER,
 					HIDPP_REG_BATTERY_MILEAGE,
 					NULL, 0, &response);
@@ -777,7 +779,6 @@ static char *hidpp_unifying_get_name(struct
hidpp_device *hidpp_dev)
 	int len;

 	ret = hidpp_send_rap_command_sync(hidpp_dev,
-					REPORT_ID_HIDPP_SHORT,
 					HIDPP_GET_LONG_REGISTER,
 					HIDPP_REG_PAIRING_INFORMATION,
 					params, 1, &response);
@@ -811,7 +812,6 @@ static int hidpp_unifying_get_serial(struct
hidpp_device *hidpp, u32 *serial)
 	u8 params[1] = { HIDPP_EXTENDED_PAIRING };

 	ret = hidpp_send_rap_command_sync(hidpp,
-					REPORT_ID_HIDPP_SHORT,
 					HIDPP_GET_LONG_REGISTER,
 					HIDPP_REG_PAIRING_INFORMATION,
 					params, 1, &response);
@@ -862,6 +862,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)
 {
@@ -893,9 +895,8 @@ static int hidpp_root_get_protocol_version(struct
hidpp_device *hidpp)
 	int ret;

 	ret = hidpp_send_rap_command_sync(hidpp,
-			REPORT_ID_HIDPP_SHORT,
 			HIDPP_PAGE_ROOT_IDX,
-			CMD_ROOT_GET_PROTOCOL_VERSION,
+			CMD_ROOT_GET_PROTOCOL_VERSION |
LINUX_KERNEL_SW_ID,
 			ping_data, sizeof(ping_data), &response);

 	if (ret == HIDPP_ERROR_INVALID_SUBID) {
@@ -1729,6 +1730,361 @@ 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;
+	bool on;
+	u16 brightness;
+	struct control_info brightness_info;
+	struct control_info color_temperature_info;
+	char dirname[256];
+	bool removed;
+};
+
+/* 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 device_to_led_brightness_value(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 unsigned device_to_led_brightness(struct led_data* led) {
+	return device_to_led_brightness_value(led, led->brightness);
+}
+
+static u16 led_to_device_brightness_value(struct led_data* led, unsigned
led_brightness) {
+	unsigned step = led_brightness - 1;
+	unsigned relative = step * led->brightness_info.res;
+	return led->brightness_info.min + relative;
+}
+
+static enum led_brightness led_brightness_get(struct led_classdev
*led_cdev)
+{
+	struct led_data *led = container_of(led_cdev, struct led_data,
cdev);
+	struct hidpp_device *hidpp = led->drv_data;
+	u8 params[1] = { 0 };
+	struct hidpp_report report;
+	int ret;
+	u16 be_brightness;
+
+
+	ret = hidpp_send_fap_command_sync(hidpp,
+
hidpp->illumination_feature_index,
+					  HIDPP_ILLUMINATION_FUNC_GET,
params,
+					  0, &report);
+	if (ret) {
+		hid_err(hidpp->hid_dev, "Getting Illumination failed\n");
+		goto exit;
+	}
+
+
+	led->on = report.fap.params[0] & 0x01;
+	if (!led->on) {
+		return LED_OFF;
+	}
+
+	ret = hidpp_send_fap_command_sync(
+		hidpp, hidpp->illumination_feature_index,
+		HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS, params, 0,
&report);
+	if (ret) {
+		hid_err(hidpp->hid_dev,
+			"Getting Illumination Brightness failed\n");
+		goto exit;
+	}
+
+	be_brightness = (report.fap.params[0] << 8) |
+			(report.fap.params[1] << 0);
+	led->brightness = be16_to_cpu(be_brightness);
+exit:
+	return device_to_led_brightness(led);
+}
+
+static void led_brightness_set_dummy(struct led_classdev *led_cdev,
+			enum led_brightness brightness) {
+}
+
+static int led_brightness_set_sync(struct led_classdev *led_cdev,
+			       enum led_brightness brightness)
+{
+	struct led_data *led = container_of(led_cdev, struct led_data,
cdev);
+	struct hidpp_device *hidpp = led->drv_data;
+	u16 be_brightness;
+	struct hidpp_report report;
+	u8 params[2];
+	int params_count = sizeof(params) / sizeof(*params);
+	int ret;
+
+
+	be_brightness = cpu_to_be16(led->brightness);
+	led->on = brightness != 0;
+	if (led->on) {
+		led->brightness = led_to_device_brightness_value(led,
brightness);
+	}
+
+	memzero_explicit(params, params_count);
+	params[0] = led->on ? 0x01 : 0x00;
+	ret = hidpp_send_fap_command_sync(hidpp,
+
hidpp->illumination_feature_index,
+					  HIDPP_ILLUMINATION_FUNC_SET,
params,
+					  params_count, &report);
+	if (ret) {
+		hid_err(hidpp->hid_dev, "Setting Illumination failed\n");
+		return ret;
+	}
+
+	if (!led->on)
+		return 0;
+	params[0] = (be_brightness & 0xFF00) >> 8;
+	params[1] = (be_brightness & 0x00FF) >> 0;
+	ret = hidpp_send_fap_command_sync(
+		hidpp, hidpp->illumination_feature_index,
+		HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS, params,
params_count,
+		&report);
+	if (ret) {
+		hid_err(hidpp->hid_dev,
+			"Setting Illumination Brightness failed\n");
+		return ret;
+	}
+	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,
+			"get_brightness_info_sync failed with %d\n", ret);
+		return ret;
+	}
+
+	info->capabilities = resp.fap.params[0];
+	info->min =
+		be16_to_cpu(resp.fap.params[1] << 8 | resp.fap.params[2]
<< 0);
+	info->max =
+		be16_to_cpu(resp.fap.params[3] << 8 | resp.fap.params[4]
<< 0);
+	info->res =
+		be16_to_cpu(resp.fap.params[5] << 8 | resp.fap.params[6]
<< 0);
+	info->max_levels = resp.fap.params[7];
+	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,
+			"get_color_temperature_info_sync failed with
%d\n",
+			ret);
+		return ret;
+	}
+
+	info->capabilities = resp.fap.params[0];
+	info->min =
+		be16_to_cpu(resp.fap.params[1] << 8 | resp.fap.params[2]
<< 0);
+	info->max =
+		be16_to_cpu(resp.fap.params[3] << 8 | resp.fap.params[4]
<< 0);
+	info->res =
+		be16_to_cpu(resp.fap.params[5] << 8 | resp.fap.params[6]
<< 0);
+	info->max_levels = resp.fap.params[7];
+	return 0;
+}
+
+static int register_led(struct hidpp_device *hidpp)
+{
+	char buf[256];
+	int ret;
+	unsigned brightness_range, r = 0, w = 0;
+	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;
+	led->removed = false;
+	memzero_explicit(buf, 256);
+	strscpy(buf, hidpp->name, 256);
+	while (w != 256) {
+		char c = buf[r];
+		if (c == '\'' || c == '\"') {
+			if (r != 255) {
+				r++;
+			}
+			continue;
+		}
+		buf[w] = buf[r];
+		w++;
+		if (r != 255) {
+			r++;
+		}
+	}
+	strreplace(buf, ' ', '_');
+	snprintf(led->dirname, sizeof(led->dirname) /
sizeof(*led->dirname),
+		 "%s::illumination", buf);
+	led->cdev.name = led->dirname;
+	led->cdev.flags = LED_HW_PLUGGABLE | LED_BRIGHT_HW_CHANGED;
+	led->cdev.max_brightness = device_to_led_brightness_value(led,
led->brightness_info.max);
+	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 cleanup;
+	}
+	hidpp->private_data = led;
+	return 0;
+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 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:
+		return 0;
+	}
+
+	if (report->fap.feature_index !=
hidpp->illumination_feature_index) {
+		return 0;
+	}
+
+
+	if (report->fap.funcindex_clientid ==
HIDPP_ILLUMINATION_EVENT_CHANGE) {
+		led->on = report->fap.params[0] & 0x1;
+		if (led->on) {
+			unsigned led_brightness =
device_to_led_brightness(led);
+			led_classdev_notify_brightness_hw_changed(
+				&led->cdev, led_brightness);
+		} else
+
led_classdev_notify_brightness_hw_changed(&led->cdev,
+
LED_OFF);
+		return 0;
+	}
+
+	if (report->fap.funcindex_clientid ==
+	    HIDPP_ILLUMINATION_EVENT_BRIGHTNESS_CHANGE) {
+		unsigned led_brightness;
+		u16 brightness = be16_to_cpu(report->fap.params[0] << 8 |
+					     report->fap.params[1] << 0);
+		led->brightness = brightness;
+		led_brightness = device_to_led_brightness(led);
+		led_classdev_notify_brightness_hw_changed(&led->cdev,
+							  led_brightness);
+		return 0;
+	}
+
+	return 0;
+}
+
 /*
--------------------------------------------------------------------------
*/
 /* 0x2120: Hi-resolution scrolling
*/
 /*
--------------------------------------------------------------------------
*/
@@ -2929,7 +3285,6 @@ static int m560_send_config_command(struct
hid_device *hdev, bool connected)

 	return hidpp_send_rap_command_sync(
 		hidpp_dev,
-		REPORT_ID_HIDPP_SHORT,
 		M560_SUB_ID,
 		M560_BUTTON_MODE_REGISTER,
 		(u8 *)m560_config_parameter,
@@ -3468,7 +3823,6 @@ static int hidpp_initialize_hires_scroll(struct
hidpp_device *hidpp)
 		struct hidpp_report response;

 		ret = hidpp_send_rap_command_sync(hidpp,
-						  REPORT_ID_HIDPP_SHORT,
 						  HIDPP_GET_REGISTER,

HIDPP_ENABLE_FAST_SCROLL,
 						  NULL, 0, &response);
@@ -3648,6 +4002,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)
@@ -3972,6 +4332,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) {
@@ -3994,6 +4355,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;
@@ -4187,12 +4556,16 @@ static int hidpp_probe(struct hid_device *hdev,
const struct hid_device_id *id)
 	if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
 		hidpp_unifying_init(hidpp);

-	connected = hidpp_root_get_protocol_version(hidpp) == 0;
+	ret = hidpp_root_get_protocol_version(hidpp);
+	connected = ret == 0;
 	atomic_set(&hidpp->connected, connected);
 	if (!(hidpp->quirks & HIDPP_QUIRK_UNIFYING)) {
 		if (!connected) {
+			if (ret == -ETIMEDOUT)
+				hid_err(hdev, "Device connection timed
out");
+			else
+				hid_err(hdev, "Device not connected");
 			ret = -ENODEV;
-			hid_err(hdev, "Device not connected");
 			goto hid_hw_init_fail;
 		}

@@ -4357,6 +4730,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_DELAYED_INIT |
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 50e1c717fc0a..0332662692d2 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -491,6 +491,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) },

Re: Litra Glow on Linux

[Andreas Bergmeier]

On Sat, 19 Nov 2022, Andreas Bergmeier wrote:

> So after some tinkering I have code now that succeeds in retrieving state
> via sending reports once. After that all following sends time out.
> I am at a loss what I am doing wrong, tbh. RFC below.
I noticed that by default hidraw, hiddev and hid-input are loaded for
Glow. Probably because it exposes a Consumer Control page in Reports.
Is it possible that hid-input interferes with hid-logitech-hidpp?
I was trying to disable hid-input using
- HIDPP_QUIRK_NO_HIDINPUT
- hid_ignore_list

For the former it still seems to load hid-input.
For the latter it doesn't seem to load hid-logitech-hidpp.
I couldn't yet untangle all the hid driver indirections and loading.