[PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[Leo L. Schwab]

The Logitech G13 is a gaming keypad with general-purpose macro keys,
four LED-backlit macro preset keys, five "menu" keys, backlight toggle
key, an analog thumbstick, RGB LED backlight, and a monochrome LCD
display.

Support input event generation for all keys and the thumbstick, and
expose all LEDs.

Signed-off-by: Leo L. Schwab <ewhac@ewhac.org>
---
 drivers/hid/hid-ids.h    |   1 +
 drivers/hid/hid-lg-g15.c | 428 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 413 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 33cc5820f2be..7ed1e402b80a 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -870,6 +870,7 @@
 #define USB_DEVICE_ID_LOGITECH_DUAL_ACTION	0xc216
 #define USB_DEVICE_ID_LOGITECH_RUMBLEPAD2	0xc218
 #define USB_DEVICE_ID_LOGITECH_RUMBLEPAD2_2	0xc219
+#define USB_DEVICE_ID_LOGITECH_G13		0xc21c
 #define USB_DEVICE_ID_LOGITECH_G15_LCD		0xc222
 #define USB_DEVICE_ID_LOGITECH_G11		0xc225
 #define USB_DEVICE_ID_LOGITECH_G15_V2_LCD	0xc227
diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
index f8605656257b..6e1d79abfb23 100644
--- a/drivers/hid/hid-lg-g15.c
+++ b/drivers/hid/hid-lg-g15.c
@@ -26,7 +26,11 @@
 #define LG_G510_FEATURE_BACKLIGHT_RGB	0x05
 #define LG_G510_FEATURE_POWER_ON_RGB	0x06
 
+#define LG_G13_FEATURE_M_KEYS_LEDS	0x05
+#define LG_G13_FEATURE_BACKLIGHT_RGB	0x07
+
 enum lg_g15_model {
+	LG_G13,
 	LG_G15,
 	LG_G15_V2,
 	LG_G510,
@@ -45,6 +49,12 @@ enum lg_g15_led_type {
 	LG_G15_LED_MAX
 };
 
+struct g13_input_report {
+	u8 report_id;	/* Report ID is always set to 1. */
+	u8 joy_x, joy_y;
+	u8 keybits[5];
+};
+
 struct lg_g15_led {
 	union {
 		struct led_classdev cdev;
@@ -63,12 +73,172 @@ struct lg_g15_data {
 	struct mutex mutex;
 	struct work_struct work;
 	struct input_dev *input;
+	struct input_dev *input_js; /* Separate joystick device for G13. */
 	struct hid_device *hdev;
 	enum lg_g15_model model;
 	struct lg_g15_led leds[LG_G15_LED_MAX];
 	bool game_mode_enabled;
 };
 
+/********* G13 LED functions ***********/
+/*
+ * G13 retains no state across power cycles, and always powers up with the backlight on,
+ * color #5AFF6E, all macro key LEDs off.
+ */
+static int lg_g13_get_leds_state(struct lg_g15_data *g15)
+{
+	u8 * const tbuf = g15->transfer_buf;
+	int ret, high;
+
+	/* RGB backlight. */
+	ret = hid_hw_raw_request(g15->hdev, LG_G13_FEATURE_BACKLIGHT_RGB,
+				 tbuf, 5,
+				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret != 5) {
+		hid_err(g15->hdev, "Error getting backlight brightness: %d\n", ret);
+		return (ret < 0) ? ret : -EIO;
+	}
+
+	/* Normalize RGB intensities against the highest component. */
+	high = max3(tbuf[1], tbuf[2], tbuf[3]);
+	if (high) {
+		g15->leds[LG_G15_KBD_BRIGHTNESS].red =
+			DIV_ROUND_CLOSEST(tbuf[1] * 255, high);
+		g15->leds[LG_G15_KBD_BRIGHTNESS].green =
+			DIV_ROUND_CLOSEST(tbuf[2] * 255, high);
+		g15->leds[LG_G15_KBD_BRIGHTNESS].blue =
+			DIV_ROUND_CLOSEST(tbuf[3] * 255, high);
+		g15->leds[LG_G15_KBD_BRIGHTNESS].brightness = high;
+	} else {
+		g15->leds[LG_G15_KBD_BRIGHTNESS].red        = 255;
+		g15->leds[LG_G15_KBD_BRIGHTNESS].green      = 255;
+		g15->leds[LG_G15_KBD_BRIGHTNESS].blue       = 255;
+		g15->leds[LG_G15_KBD_BRIGHTNESS].brightness = 0;
+	}
+
+	/* Macro LEDs. */
+	ret = hid_hw_raw_request(g15->hdev, LG_G13_FEATURE_M_KEYS_LEDS,
+				 tbuf, 5,
+				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret != 5) {
+		hid_err(g15->hdev, "Error getting macro LED brightness: %d\n", ret);
+		return (ret < 0) ? ret : -EIO;
+	}
+
+	for (int i = LG_G15_MACRO_PRESET1; i < LG_G15_LED_MAX; ++i)
+		g15->leds[i].brightness = tbuf[1] & (1 << (i - LG_G15_MACRO_PRESET1));
+
+	return 0;
+}
+
+static int lg_g13_kbd_led_write(struct lg_g15_data *g15,
+				struct lg_g15_led *g15_led,
+				enum led_brightness brightness)
+{
+	struct mc_subled const * const subleds = g15_led->mcdev.subled_info;
+	u8 * const tbuf = g15->transfer_buf;
+	int ret;
+
+	guard(mutex)(&g15->mutex);
+
+	led_mc_calc_color_components(&g15_led->mcdev, brightness);
+
+	tbuf[0] = 5;
+	tbuf[1] = subleds[0].brightness;
+	tbuf[2] = subleds[1].brightness;
+	tbuf[3] = subleds[2].brightness;
+	tbuf[4] = 0;
+
+	ret = hid_hw_raw_request(g15->hdev, LG_G13_FEATURE_BACKLIGHT_RGB,
+				 tbuf, 5,
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	if (ret != 5) {
+		hid_err(g15->hdev, "Error setting backlight brightness: %d\n", ret);
+		return (ret < 0) ? ret : -EIO;
+	}
+
+	g15_led->brightness = brightness;
+	return 0;
+}
+
+static int lg_g13_kbd_led_set(struct led_classdev *led_cdev, enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc = lcdev_to_mccdev(led_cdev);
+	struct lg_g15_led *g15_led =
+		container_of(mc, struct lg_g15_led, mcdev);
+	struct lg_g15_data *g15 = dev_get_drvdata(led_cdev->dev->parent);
+
+	/* Ignore LED off on unregister / keyboard unplug */
+	if (led_cdev->flags & LED_UNREGISTERING)
+		return 0;
+
+	return lg_g13_kbd_led_write(g15, g15_led, brightness);
+}
+
+static enum led_brightness lg_g13_kbd_led_get(struct led_classdev *led_cdev)
+{
+	struct led_classdev_mc const * const mc = lcdev_to_mccdev(led_cdev);
+	struct lg_g15_led const *g15_led =
+		container_of(mc, struct lg_g15_led, mcdev);
+
+	return g15_led->brightness;
+}
+
+static int lg_g13_mkey_led_set(struct led_classdev *led_cdev, enum led_brightness brightness)
+{
+	struct lg_g15_led *g15_led =
+		container_of(led_cdev, struct lg_g15_led, cdev);
+	struct lg_g15_data *g15 = dev_get_drvdata(led_cdev->dev->parent);
+	int i, ret;
+	u8 * const tbuf = g15->transfer_buf;
+	u8 val, mask = 0;
+
+	/* Ignore LED off on unregister / keyboard unplug */
+	if (led_cdev->flags & LED_UNREGISTERING)
+		return 0;
+
+	guard(mutex)(&g15->mutex);
+
+	for (i = LG_G15_MACRO_PRESET1; i < LG_G15_LED_MAX; ++i) {
+		if (i == g15_led->led)
+			val = brightness;
+		else
+			val = g15->leds[i].brightness;
+
+		if (val)
+			mask |= 1 << (i - LG_G15_MACRO_PRESET1);
+	}
+
+	tbuf[0] = 5;
+	tbuf[1] = mask;
+	tbuf[2] = 0;
+	tbuf[3] = 0;
+	tbuf[4] = 0;
+
+	ret = hid_hw_raw_request(g15->hdev, LG_G13_FEATURE_M_KEYS_LEDS,
+				 tbuf, 5,
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	if (ret != 5) {
+		hid_err(g15->hdev, "Error setting LED brightness: %d\n", ret);
+		return (ret < 0) ? ret : -EIO;
+	}
+
+	g15_led->brightness = brightness;
+	return 0;
+}
+
+static enum led_brightness lg_g13_mkey_led_get(struct led_classdev *led_cdev)
+{
+	/*
+	 * G13 doesn't change macro key LEDs behind our back, so they're
+	 * whatever we last set them to.
+	 */
+	struct lg_g15_led *g15_led =
+		container_of(led_cdev, struct lg_g15_led, cdev);
+
+	return g15_led->brightness;
+}
+
 /******** G15 and G15 v2 LED functions ********/
 
 static int lg_g15_update_led_brightness(struct lg_g15_data *g15)
@@ -390,6 +560,8 @@ static int lg_g15_get_initial_led_brightness(struct lg_g15_data *g15)
 	int ret;
 
 	switch (g15->model) {
+	case LG_G13:
+		return lg_g13_get_leds_state(g15);
 	case LG_G15:
 	case LG_G15_V2:
 		return lg_g15_update_led_brightness(g15);
@@ -417,6 +589,116 @@ static int lg_g15_get_initial_led_brightness(struct lg_g15_data *g15)
 
 /******** Input functions ********/
 
+/**
+ * g13_input_report.keybits[] is not 32-bit aligned, so we can't use the bitops macros.
+ *
+ * @ary: Pointer to array of u8s
+ * @b: Bit index into ary, LSB first.  Not range checked.
+ */
+#define	TEST_BIT(ary, b)	((1 << ((b) & 7)) & (ary)[(b) >> 3])
+
+/* Table mapping keybits[] bit positions to event codes. */
+/* Note: Indices are discontinuous to aid readability. */
+static const u16 g13_keys_for_bits[] = {
+	/* Main keypad - keys G1 - G22 */
+	[0] = KEY_MACRO1,
+	[1] = KEY_MACRO2,
+	[2] = KEY_MACRO3,
+	[3] = KEY_MACRO4,
+	[4] = KEY_MACRO5,
+	[5] = KEY_MACRO6,
+	[6] = KEY_MACRO7,
+	[7] = KEY_MACRO8,
+	[8] = KEY_MACRO9,
+	[9] = KEY_MACRO10,
+	[10] = KEY_MACRO11,
+	[11] = KEY_MACRO12,
+	[12] = KEY_MACRO13,
+	[13] = KEY_MACRO14,
+	[14] = KEY_MACRO15,
+	[15] = KEY_MACRO16,
+	[16] = KEY_MACRO17,
+	[17] = KEY_MACRO18,
+	[18] = KEY_MACRO19,
+	[19] = KEY_MACRO20,
+	[20] = KEY_MACRO21,
+	[21] = KEY_MACRO22,
+
+	/* LCD menu buttons. */
+	[24] = KEY_KBD_LCD_MENU5,	/* "Next page" button */
+	[25] = KEY_KBD_LCD_MENU1,	/* Left-most */
+	[26] = KEY_KBD_LCD_MENU2,
+	[27] = KEY_KBD_LCD_MENU3,
+	[28] = KEY_KBD_LCD_MENU4,	/* Right-most */
+
+	/* Macro preset and record buttons; have red LEDs under them. */
+	[29] = KEY_MACRO_PRESET1,
+	[30] = KEY_MACRO_PRESET2,
+	[31] = KEY_MACRO_PRESET3,
+	[32] = KEY_MACRO_RECORD_START,
+
+	/* 33-35 handled by joystick device. */
+
+	/* Backlight toggle. */
+	[37] = KEY_LIGHTS_TOGGLE,
+};
+
+static const u16 g13_keys_for_bits_js[] = {
+	/* Joystick buttons */
+	/* These keybits are at bit indices 33, 34, and 35. */
+	BTN_BASE,	/* Left side */
+	BTN_BASE2,	/* Bottom side */
+	BTN_THUMB,	/* Stick depress */
+};
+
+static int lg_g13_event(struct lg_g15_data *g15, u8 const *data)
+{
+	struct g13_input_report const * const rep = (struct g13_input_report *) data;
+	int i, val;
+
+	/*
+	 * Main macropad and menu keys.
+	 * Emit key events defined for each bit position.
+	 */
+	for (i = 0; i < ARRAY_SIZE(g13_keys_for_bits); ++i) {
+		if (g13_keys_for_bits[i]) {
+			val = TEST_BIT(rep->keybits, i);
+			input_report_key(g15->input, g13_keys_for_bits[i], val);
+		}
+	}
+	input_sync(g15->input);
+
+	/*
+	 * Joystick.
+	 * Emit button and deflection events.
+	 */
+	for (i = 0; i < ARRAY_SIZE(g13_keys_for_bits_js); ++i) {
+		if (g13_keys_for_bits_js[i]) {
+			val = TEST_BIT(rep->keybits, i + 33);
+			input_report_key(g15->input_js, g13_keys_for_bits_js[i], val);
+		}
+	}
+	input_report_abs(g15->input_js, ABS_X, rep->joy_x);
+	input_report_abs(g15->input_js, ABS_Y, rep->joy_y);
+	input_sync(g15->input_js);
+
+	if (IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)) {
+		/*
+		 * Bit 23 of keybits[] reports the current backlight on/off
+		 * state.  If it has changed from the last cached value, apply
+		 * an update.
+		 */
+		bool hw_brightness_changed = (!!TEST_BIT(rep->keybits, 23))
+					   ^ (g15->leds[0].cdev.brightness_hw_changed > 0);
+		if (hw_brightness_changed)
+			led_classdev_notify_brightness_hw_changed(
+				&g15->leds[0].cdev,
+				TEST_BIT(rep->keybits, 23) ? LED_FULL : LED_OFF);
+	}
+
+	return 0;
+}
+
 /* On the G15 Mark I Logitech has been quite creative with which bit is what */
 static void lg_g15_handle_lcd_menu_keys(struct lg_g15_data *g15, u8 *data)
 {
@@ -572,6 +854,10 @@ static int lg_g15_raw_event(struct hid_device *hdev, struct hid_report *report,
 		return 0;
 
 	switch (g15->model) {
+	case LG_G13:
+		if (data[0] == 0x01 && size == sizeof(struct g13_input_report))
+			return lg_g13_event(g15, data);
+		break;
 	case LG_G15:
 		if (data[0] == 0x02 && size == 9)
 			return lg_g15_event(g15, data);
@@ -616,13 +902,22 @@ static void lg_g15_setup_led_rgb(struct lg_g15_data *g15, int index)
 {
 	int i;
 	struct mc_subled *subled_info;
-
-	g15->leds[index].mcdev.led_cdev.brightness_set_blocking =
-		lg_g510_kbd_led_set;
-	g15->leds[index].mcdev.led_cdev.brightness_get =
-		lg_g510_kbd_led_get;
-	g15->leds[index].mcdev.led_cdev.max_brightness = 255;
-	g15->leds[index].mcdev.num_colors = 3;
+	struct lg_g15_led * const gled = &g15->leds[index];
+
+	if (g15->model == LG_G13) {
+		gled->mcdev.led_cdev.brightness_set_blocking =
+			lg_g13_kbd_led_set;
+		gled->mcdev.led_cdev.brightness_get =
+			lg_g13_kbd_led_get;
+		gled->mcdev.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
+	} else {
+		gled->mcdev.led_cdev.brightness_set_blocking =
+			lg_g510_kbd_led_set;
+		gled->mcdev.led_cdev.brightness_get =
+			lg_g510_kbd_led_get;
+	}
+	gled->mcdev.led_cdev.max_brightness = 255;
+	gled->mcdev.num_colors = 3;
 
 	subled_info = devm_kcalloc(&g15->hdev->dev, 3, sizeof(*subled_info), GFP_KERNEL);
 	if (!subled_info)
@@ -632,20 +927,20 @@ static void lg_g15_setup_led_rgb(struct lg_g15_data *g15, int index)
 		switch (i + 1) {
 		case LED_COLOR_ID_RED:
 			subled_info[i].color_index = LED_COLOR_ID_RED;
-			subled_info[i].intensity = g15->leds[index].red;
+			subled_info[i].intensity = gled->red;
 			break;
 		case LED_COLOR_ID_GREEN:
 			subled_info[i].color_index = LED_COLOR_ID_GREEN;
-			subled_info[i].intensity = g15->leds[index].green;
+			subled_info[i].intensity = gled->green;
 			break;
 		case LED_COLOR_ID_BLUE:
 			subled_info[i].color_index = LED_COLOR_ID_BLUE;
-			subled_info[i].intensity = g15->leds[index].blue;
+			subled_info[i].intensity = gled->blue;
 			break;
 		}
 		subled_info[i].channel = i;
 	}
-	g15->leds[index].mcdev.subled_info = subled_info;
+	gled->mcdev.subled_info = subled_info;
 }
 
 static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
@@ -656,6 +951,23 @@ static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
 	g15->leds[i].cdev.name = name;
 
 	switch (g15->model) {
+	case LG_G13:
+		if (i < LG_G15_BRIGHTNESS_MAX) {
+			/* RGB backlight. */
+			lg_g15_setup_led_rgb(g15, i);
+			ret = devm_led_classdev_multicolor_register_ext(&g15->hdev->dev,
+									&g15->leds[i].mcdev,
+									NULL);
+		} else {
+			/* Macro keys */
+			g15->leds[i].cdev.brightness_set_blocking = lg_g13_mkey_led_set;
+			g15->leds[i].cdev.brightness_get = lg_g13_mkey_led_get;
+			g15->leds[i].cdev.max_brightness = 1;
+
+			ret = devm_led_classdev_register(&g15->hdev->dev,
+							 &g15->leds[i].cdev);
+		}
+		break;
 	case LG_G15:
 	case LG_G15_V2:
 		g15->leds[i].cdev.brightness_get = lg_g15_led_get;
@@ -702,11 +1014,9 @@ static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
 }
 
 /* Common input device init code shared between keyboards and Z-10 speaker handling */
-static void lg_g15_init_input_dev(struct hid_device *hdev, struct input_dev *input,
-				  const char *name)
+static void lg_g15_init_input_dev_core(struct hid_device *hdev, struct input_dev *input,
+				       char const *name)
 {
-	int i;
-
 	input->name = name;
 	input->phys = hdev->phys;
 	input->uniq = hdev->uniq;
@@ -717,12 +1027,44 @@ static void lg_g15_init_input_dev(struct hid_device *hdev, struct input_dev *inp
 	input->dev.parent = &hdev->dev;
 	input->open = lg_g15_input_open;
 	input->close = lg_g15_input_close;
+}
+
+static void lg_g15_init_input_dev(struct hid_device *hdev, struct input_dev *input,
+				  const char *name)
+{
+	int i;
+
+	lg_g15_init_input_dev_core(hdev, input, name);
 
 	/* Keys below the LCD, intended for controlling a menu on the LCD */
 	for (i = 0; i < 5; i++)
 		input_set_capability(input, EV_KEY, KEY_KBD_LCD_MENU1 + i);
 }
 
+static void lg_g13_init_input_dev(struct hid_device *hdev,
+				  struct input_dev *input, const char *name,
+				  struct input_dev *input_js, const char *name_js)
+{
+	/* Macropad. */
+	lg_g15_init_input_dev_core(hdev, input, name);
+	for (int i = 0; i < ARRAY_SIZE(g13_keys_for_bits); ++i) {
+		if (g13_keys_for_bits[i])
+			input_set_capability(input, EV_KEY, g13_keys_for_bits[i]);
+	}
+
+	/* OBTW, we're a joystick, too... */
+	lg_g15_init_input_dev_core(hdev, input_js, name_js);
+	for (int i = 0; i < ARRAY_SIZE(g13_keys_for_bits_js); ++i) {
+		if (g13_keys_for_bits_js[i])
+			input_set_capability(input_js, EV_KEY, g13_keys_for_bits_js[i]);
+	}
+
+	input_set_capability(input_js, EV_ABS, ABS_X);
+	input_set_abs_params(input_js, ABS_X, 0, 255, 0, 0);
+	input_set_capability(input_js, EV_ABS, ABS_Y);
+	input_set_abs_params(input_js, ABS_Y, 0, 255, 0, 0);
+}
+
 static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	static const char * const led_names[] = {
@@ -739,7 +1081,7 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	unsigned int connect_mask = 0;
 	bool has_ff000000 = false;
 	struct lg_g15_data *g15;
-	struct input_dev *input;
+	struct input_dev *input, *input_js;
 	struct hid_report *rep;
 	int ret, i, gkeys = 0;
 
@@ -778,6 +1120,25 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	hid_set_drvdata(hdev, (void *)g15);
 
 	switch (g15->model) {
+	case LG_G13:
+		/*
+		 * The G13 has an analog thumbstick with nearby buttons.  Some
+		 * libraries and applications are known to ignore devices that
+		 * don't "look like" a joystick, and a device with two ABS axes
+		 * and 25+ macro keys would confuse them.
+		 *
+		 * Create an additional input device dedicated to appear as a
+		 * simplified joystick (two ABS axes, three BTN buttons).
+		 */
+		input_js = devm_input_allocate_device(&hdev->dev);
+		if (!input_js)
+			return -ENOMEM;
+		g15->input_js = input_js;
+		input_set_drvdata(input_js, hdev);
+
+		connect_mask = HID_CONNECT_HIDRAW;
+		gkeys = 25;
+		break;
 	case LG_G15:
 		INIT_WORK(&g15->work, lg_g15_leds_changed_work);
 		/*
@@ -859,6 +1220,34 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			goto error_hw_stop;
 
 		return 0; /* All done */
+	} else if (g15->model == LG_G13) {
+		static char const * const g13_led_names[] = {
+			/* Backlight is shared between LCD and keys. */
+			"g13:rgb:kbd_backlight",
+			NULL,	/* Keep in sync with led_type enum */
+			"g13:red:macro_preset_1",
+			"g13:red:macro_preset_2",
+			"g13:red:macro_preset_3",
+			"g13:red:macro_record",
+		};
+		lg_g13_init_input_dev(hdev,
+				      input, "Logitech G13 Gaming Keypad",
+				      input_js, "Logitech G13 Thumbstick");
+		ret = input_register_device(input);
+		if (ret)
+			goto error_hw_stop;
+		ret = input_register_device(input_js);
+		if (ret)
+			goto error_hw_stop;
+
+		for (i = 0; i < ARRAY_SIZE(g13_led_names); ++i) {
+			if (g13_led_names[i]) {
+				ret = lg_g15_register_led(g15, i, g13_led_names[i]);
+				if (ret)
+					goto error_hw_stop;
+			}
+		}
+		return 0;
 	}
 
 	/* Setup and register input device */
@@ -903,6 +1292,13 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
 }
 
 static const struct hid_device_id lg_g15_devices[] = {
+	/*
+	 * The G13 is a macropad-only device with an LCD, LED backlighing,
+	 * and joystick.
+	 */
+	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+			 USB_DEVICE_ID_LOGITECH_G13),
+		.driver_data = LG_G13 },
 	/* The G11 is a G15 without the LCD, treat it as a G15 */
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
 		USB_DEVICE_ID_LOGITECH_G11),
-- 
2.50.1

Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[Hans de Goede]

Hi Leo,

On 14-Aug-25 11:26 PM, Leo L. Schwab wrote:
> The Logitech G13 is a gaming keypad with general-purpose macro keys,
> four LED-backlit macro preset keys, five "menu" keys, backlight toggle
> key, an analog thumbstick, RGB LED backlight, and a monochrome LCD
> display.
> 
> Support input event generation for all keys and the thumbstick, and
> expose all LEDs.
> 
> Signed-off-by: Leo L. Schwab <ewhac@ewhac.org>

Thank you for your patch overal this looks good to me,
some remarks inline below.

...

> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index f8605656257b..6e1d79abfb23 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c

...

> +/**
> + * g13_input_report.keybits[] is not 32-bit aligned, so we can't use the bitops macros.
> + *
> + * @ary: Pointer to array of u8s
> + * @b: Bit index into ary, LSB first.  Not range checked.
> + */
> +#define	TEST_BIT(ary, b)	((1 << ((b) & 7)) & (ary)[(b) >> 3])
> +
> +/* Table mapping keybits[] bit positions to event codes. */
> +/* Note: Indices are discontinuous to aid readability. */
> +static const u16 g13_keys_for_bits[] = {
> +	/* Main keypad - keys G1 - G22 */
> +	[0] = KEY_MACRO1,
> +	[1] = KEY_MACRO2,
> +	[2] = KEY_MACRO3,
> +	[3] = KEY_MACRO4,
> +	[4] = KEY_MACRO5,
> +	[5] = KEY_MACRO6,
> +	[6] = KEY_MACRO7,
> +	[7] = KEY_MACRO8,
> +	[8] = KEY_MACRO9,
> +	[9] = KEY_MACRO10,
> +	[10] = KEY_MACRO11,
> +	[11] = KEY_MACRO12,
> +	[12] = KEY_MACRO13,
> +	[13] = KEY_MACRO14,
> +	[14] = KEY_MACRO15,
> +	[15] = KEY_MACRO16,
> +	[16] = KEY_MACRO17,
> +	[17] = KEY_MACRO18,
> +	[18] = KEY_MACRO19,
> +	[19] = KEY_MACRO20,
> +	[20] = KEY_MACRO21,
> +	[21] = KEY_MACRO22,
> +
> +	/* LCD menu buttons. */
> +	[24] = KEY_KBD_LCD_MENU5,	/* "Next page" button */
> +	[25] = KEY_KBD_LCD_MENU1,	/* Left-most */
> +	[26] = KEY_KBD_LCD_MENU2,
> +	[27] = KEY_KBD_LCD_MENU3,
> +	[28] = KEY_KBD_LCD_MENU4,	/* Right-most */
> +
> +	/* Macro preset and record buttons; have red LEDs under them. */
> +	[29] = KEY_MACRO_PRESET1,
> +	[30] = KEY_MACRO_PRESET2,
> +	[31] = KEY_MACRO_PRESET3,
> +	[32] = KEY_MACRO_RECORD_START,
> +
> +	/* 33-35 handled by joystick device. */
> +
> +	/* Backlight toggle. */
> +	[37] = KEY_LIGHTS_TOGGLE,
> +};
> +
> +static const u16 g13_keys_for_bits_js[] = {
> +	/* Joystick buttons */
> +	/* These keybits are at bit indices 33, 34, and 35. */
> +	BTN_BASE,	/* Left side */
> +	BTN_BASE2,	/* Bottom side */
> +	BTN_THUMB,	/* Stick depress */
> +};

You are using this 33 offset hardcoded below, maybe
at a #define for this, e.g. :

#define G13_JS_KEYBITS_OFFSET	33

> +
> +static int lg_g13_event(struct lg_g15_data *g15, u8 const *data)
> +{
> +	struct g13_input_report const * const rep = (struct g13_input_report *) data;
> +	int i, val;
> +
> +	/*
> +	 * Main macropad and menu keys.
> +	 * Emit key events defined for each bit position.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(g13_keys_for_bits); ++i) {
> +		if (g13_keys_for_bits[i]) {
> +			val = TEST_BIT(rep->keybits, i);
> +			input_report_key(g15->input, g13_keys_for_bits[i], val);
> +		}
> +	}
> +	input_sync(g15->input);
> +
> +	/*
> +	 * Joystick.
> +	 * Emit button and deflection events.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(g13_keys_for_bits_js); ++i) {
> +		if (g13_keys_for_bits_js[i]) {

g13_keys_for_bits_js[] is contiguous so no need
for this if (g13_keys_for_bits_js[i]) test.

> +			val = TEST_BIT(rep->keybits, i + 33);
> +			input_report_key(g15->input_js, g13_keys_for_bits_js[i], val);
> +		}
> +	}
> +	input_report_abs(g15->input_js, ABS_X, rep->joy_x);
> +	input_report_abs(g15->input_js, ABS_Y, rep->joy_y);
> +	input_sync(g15->input_js);
> +
> +	if (IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)) {

I do not believe that this IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)
is necessary, led_classdev_notify_brightness_hw_changed() has a static
inline replacement when CONFIG_LEDS_BRIGHTNESS_HW_CHANGED is not set,
so you can just call it unconditionally.

This is already called unconditionally in other places of the code.

> +		/*
> +		 * Bit 23 of keybits[] reports the current backlight on/off
> +		 * state.  If it has changed from the last cached value, apply
> +		 * an update.
> +		 */
> +		bool hw_brightness_changed = (!!TEST_BIT(rep->keybits, 23))
> +					   ^ (g15->leds[0].cdev.brightness_hw_changed > 0);
> +		if (hw_brightness_changed)
> +			led_classdev_notify_brightness_hw_changed(
> +				&g15->leds[0].cdev,
> +				TEST_BIT(rep->keybits, 23) ? LED_FULL : LED_OFF);
> +	}
> +
> +	return 0;
> +}
> +

...

> +static void lg_g13_init_input_dev(struct hid_device *hdev,
> +				  struct input_dev *input, const char *name,
> +				  struct input_dev *input_js, const char *name_js)
> +{
> +	/* Macropad. */
> +	lg_g15_init_input_dev_core(hdev, input, name);
> +	for (int i = 0; i < ARRAY_SIZE(g13_keys_for_bits); ++i) {
> +		if (g13_keys_for_bits[i])
> +			input_set_capability(input, EV_KEY, g13_keys_for_bits[i]);
> +	}
> +
> +	/* OBTW, we're a joystick, too... */
> +	lg_g15_init_input_dev_core(hdev, input_js, name_js);
> +	for (int i = 0; i < ARRAY_SIZE(g13_keys_for_bits_js); ++i) {
> +		if (g13_keys_for_bits_js[i])
> +			input_set_capability(input_js, EV_KEY, g13_keys_for_bits_js[i]);

g13_keys_for_bits_js[] is contiguous so no need
for this if (g13_keys_for_bits_js[i]) test.

This will also allow you to drop the {} from the for loop.

> +	}
> +
> +	input_set_capability(input_js, EV_ABS, ABS_X);
> +	input_set_abs_params(input_js, ABS_X, 0, 255, 0, 0);
> +	input_set_capability(input_js, EV_ABS, ABS_Y);
> +	input_set_abs_params(input_js, ABS_Y, 0, 255, 0, 0);
> +}
> +
>  static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>  	static const char * const led_names[] = {
...

Besides from my few small remarks this looks good to me,
feel free to add:

Reviewed-by: Hans de Goede <hansg@kernel.org>

to the next version.

Regards,

Hans

Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[Leo L. Schwab]

On Sun, Aug 31, 2025 at 03:01:12PM +0200, Hans de Goede wrote:
> > +static const u16 g13_keys_for_bits_js[] = {
> > +	/* Joystick buttons */
> > +	/* These keybits are at bit indices 33, 34, and 35. */
> > +	BTN_BASE,	/* Left side */
> > +	BTN_BASE2,	/* Bottom side */
> > +	BTN_THUMB,	/* Stick depress */
> > +};
> 
> You are using this 33 offset hardcoded below, maybe
> at a #define for this, e.g. :
> 
> #define G13_JS_KEYBITS_OFFSET	33
>
	Noted.

> g13_keys_for_bits_js[] is contiguous so no need
> for this if (g13_keys_for_bits_js[i]) test.
>
	Noted.

> > +	if (IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)) {
> 
> I do not believe that this IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)
> is necessary, led_classdev_notify_brightness_hw_changed() has a static
> inline replacement when CONFIG_LEDS_BRIGHTNESS_HW_CHANGED is not set,
> so you can just call it unconditionally.
> 
> This is already called unconditionally in other places of the code.
>
	I was actually bit by this in the first two revs by the build bot.
If CONFIG_LEDS_BRIGHTNESS_HW_CHANGED is not enabled, the field
`cdev.brightness_hw_changed`, referenced a bit further down, does not exist,
and causes a build failure.

	My first attempt at #ifdef-ing around it led to another build bot
warning about `hw_brightness_changed` being defined but not used.  Then I
leanred about `IS_ENABLED()`, which is evidently now preferred over `#ifdef
CONFIG_`, and nicely isolated the whole block, so I went with that.

> g13_keys_for_bits_js[] is contiguous so no need
> for this if (g13_keys_for_bits_js[i]) test.
> 
> This will also allow you to drop the {} from the for loop.
>
	Noted.

	I've been mucking around with Rust a bit lately (and badly indented
JavaScript rather more), and am now in the habit putting braces around
single-line `if` clauses.

> Besides from my few small remarks this looks good to me,
> feel free to add:
> 
> Reviewed-by: Hans de Goede <hansg@kernel.org>
> 
> to the next version.
>
	Thank you very kindly, sir!

					Schwab

Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[Hans de Goede]

Hi Leo,

On 31-Aug-25 9:51 PM, Leo L. Schwab wrote:
> On Sun, Aug 31, 2025 at 03:01:12PM +0200, Hans de Goede wrote:
>>> +static const u16 g13_keys_for_bits_js[] = {
>>> +	/* Joystick buttons */
>>> +	/* These keybits are at bit indices 33, 34, and 35. */
>>> +	BTN_BASE,	/* Left side */
>>> +	BTN_BASE2,	/* Bottom side */
>>> +	BTN_THUMB,	/* Stick depress */
>>> +};
>>
>> You are using this 33 offset hardcoded below, maybe
>> at a #define for this, e.g. :
>>
>> #define G13_JS_KEYBITS_OFFSET	33
>>
> 	Noted.
> 
>> g13_keys_for_bits_js[] is contiguous so no need
>> for this if (g13_keys_for_bits_js[i]) test.
>>
> 	Noted.
> 
>>> +	if (IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)) {
>>
>> I do not believe that this IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)
>> is necessary, led_classdev_notify_brightness_hw_changed() has a static
>> inline replacement when CONFIG_LEDS_BRIGHTNESS_HW_CHANGED is not set,
>> so you can just call it unconditionally.
>>
>> This is already called unconditionally in other places of the code.
>>
> 	I was actually bit by this in the first two revs by the build bot.
> If CONFIG_LEDS_BRIGHTNESS_HW_CHANGED is not enabled, the field
> `cdev.brightness_hw_changed`, referenced a bit further down, does not exist,
> and causes a build failure.
> 
> 	My first attempt at #ifdef-ing around it led to another build bot
> warning about `hw_brightness_changed` being defined but not used.  Then I
> leanred about `IS_ENABLED()`, which is evidently now preferred over `#ifdef
> CONFIG_`, and nicely isolated the whole block, so I went with that.

Ah I see. Yes if you do need to do a CONFIG check then using IS_ENABLED()
is good.

But I'm afraid that the underlying problem here is the use of
cdev.brightness_hw_changed this is really only meant for led-class.c
internal use.

The idea of cdev.brightness_hw_changed is that it stores the last
value set by the hw.

But in the mean time that value may have been overwritten by software.

I think that you will fail to call led_classdev_notify_brightness_hw_changed()
(you can add a debug print to check) if the following happens:

1. Brightness set to 255 (RGB 255,255,255) through sysfs
2. State toggled to off by backlight control button, brightness is now 0
3. Brightness set to 255 (RGB 255,255,255) through sysfs
4. State toggled to off by backlight control button, brightness is now 0

In this case the second hw-button toggle will not call
led_classdev_notify_brightness_hw_changed(), since cdev.brightness_hw_changed
still has the 0 value from last time.

I also see that you use TEST_BIT(rep->keybits, 23) ? LED_FULL : LED_OFF
for the brightness value send to led_classdev_notify_brightness_hw_changed()
but I would expect the hw to restore the previous brightness on a toggle
from off -> on through the button? So then that should be send.

And you also never update cdev.brightness and use the cached 
struct lg_g15_led.brightness in lg_g13_kbd_led_get(). This means that
after a hw toggle of the backlight reading the brightness from sysfs
will show the wrong (old) value.

I think that instead what you need to do is create a
lg_g13_leds_changed_work() mirroring lg_g15_leds_changed_work()
but then only for the leds[0] instead of using the for loops.

Combined with caching the keybit 23 value (1) and then when
keybit 23 changes value queue the work.

This will also allow you to drop the:
	
	if (IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED))

but that is just a side effect. The most important thing here
is to:

1. Correctly update the cached brightness after hitting the toggle button
2. Compare the new brightness against the previous cached brightness to
   determine if led_classdev_notify_brightness_hw_changed() should be called,
   rather then using the possible stale cdev.brightness_hw_changed
3. Pass the actual new brightness to
   led_classdev_notify_brightness_hw_changed() and not LED_FULL.

Note I see that lg_g13_get_leds_state() does 2 USB transfers,
when running from the work we only need to get the backlight
state and not the macro keys state. So either give it a parameter
whether it should update the macro-keys or not; or split it into
2 functions, calling both functions from 
lg_g15_get_initial_led_brightness() and only calling the one
for the backlight from lg_g13_leds_changed_work().

Regards,

Hans



1) You can just init the keybit23 cache at 1 since I think the bl always
starts on, eating the cost of possibly running the work one time too many
on the first key press if the bl was turned off before the driver probes.

Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[Hans de Goede]

On 2-Sep-25 11:07 AM, Hans de Goede wrote:
> Hi Leo,
> 
> On 31-Aug-25 9:51 PM, Leo L. Schwab wrote:
>> On Sun, Aug 31, 2025 at 03:01:12PM +0200, Hans de Goede wrote:
>>>> +static const u16 g13_keys_for_bits_js[] = {
>>>> +	/* Joystick buttons */
>>>> +	/* These keybits are at bit indices 33, 34, and 35. */
>>>> +	BTN_BASE,	/* Left side */
>>>> +	BTN_BASE2,	/* Bottom side */
>>>> +	BTN_THUMB,	/* Stick depress */
>>>> +};
>>>
>>> You are using this 33 offset hardcoded below, maybe
>>> at a #define for this, e.g. :
>>>
>>> #define G13_JS_KEYBITS_OFFSET	33
>>>
>> 	Noted.
>>
>>> g13_keys_for_bits_js[] is contiguous so no need
>>> for this if (g13_keys_for_bits_js[i]) test.
>>>
>> 	Noted.
>>
>>>> +	if (IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)) {
>>>
>>> I do not believe that this IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)
>>> is necessary, led_classdev_notify_brightness_hw_changed() has a static
>>> inline replacement when CONFIG_LEDS_BRIGHTNESS_HW_CHANGED is not set,
>>> so you can just call it unconditionally.
>>>
>>> This is already called unconditionally in other places of the code.
>>>
>> 	I was actually bit by this in the first two revs by the build bot.
>> If CONFIG_LEDS_BRIGHTNESS_HW_CHANGED is not enabled, the field
>> `cdev.brightness_hw_changed`, referenced a bit further down, does not exist,
>> and causes a build failure.
>>
>> 	My first attempt at #ifdef-ing around it led to another build bot
>> warning about `hw_brightness_changed` being defined but not used.  Then I
>> leanred about `IS_ENABLED()`, which is evidently now preferred over `#ifdef
>> CONFIG_`, and nicely isolated the whole block, so I went with that.
> 
> Ah I see. Yes if you do need to do a CONFIG check then using IS_ENABLED()
> is good.
> 
> But I'm afraid that the underlying problem here is the use of
> cdev.brightness_hw_changed this is really only meant for led-class.c
> internal use.
> 
> The idea of cdev.brightness_hw_changed is that it stores the last
> value set by the hw.
> 
> But in the mean time that value may have been overwritten by software.
> 
> I think that you will fail to call led_classdev_notify_brightness_hw_changed()
> (you can add a debug print to check) if the following happens:
> 
> 1. Brightness set to 255 (RGB 255,255,255) through sysfs
> 2. State toggled to off by backlight control button, brightness is now 0
> 3. Brightness set to 255 (RGB 255,255,255) through sysfs
> 4. State toggled to off by backlight control button, brightness is now 0
> 
> In this case the second hw-button toggle will not call
> led_classdev_notify_brightness_hw_changed(), since cdev.brightness_hw_changed
> still has the 0 value from last time.
> 
> I also see that you use TEST_BIT(rep->keybits, 23) ? LED_FULL : LED_OFF
> for the brightness value send to led_classdev_notify_brightness_hw_changed()
> but I would expect the hw to restore the previous brightness on a toggle
> from off -> on through the button? So then that should be send.
> 
> And you also never update cdev.brightness and use the cached 
> struct lg_g15_led.brightness in lg_g13_kbd_led_get(). This means that
> after a hw toggle of the backlight reading the brightness from sysfs
> will show the wrong (old) value.
> 
> I think that instead what you need to do is create a
> lg_g13_leds_changed_work() mirroring lg_g15_leds_changed_work()
> but then only for the leds[0] instead of using the for loops.
> 
> Combined with caching the keybit 23 value (1) and then when
> keybit 23 changes value queue the work.
> 
> This will also allow you to drop the:
> 	
> 	if (IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED))
> 
> but that is just a side effect. The most important thing here
> is to:
> 
> 1. Correctly update the cached brightness after hitting the toggle button
> 2. Compare the new brightness against the previous cached brightness to
>    determine if led_classdev_notify_brightness_hw_changed() should be called,
>    rather then using the possible stale cdev.brightness_hw_changed
> 3. Pass the actual new brightness to
>    led_classdev_notify_brightness_hw_changed() and not LED_FULL.
> 
> Note I see that lg_g13_get_leds_state() does 2 USB transfers,
> when running from the work we only need to get the backlight
> state and not the macro keys state. So either give it a parameter
> whether it should update the macro-keys or not; or split it into
> 2 functions, calling both functions from 
> lg_g15_get_initial_led_brightness() and only calling the one
> for the backlight from lg_g13_leds_changed_work().
> 
> Regards,
> 
> Hans
> 
> 
> 
> 1) You can just init the keybit23 cache at 1 since I think the bl always
> starts on, eating the cost of possibly running the work one time too many
> on the first key press if the bl was turned off before the driver probes.

p.s.

Hmm, I wonder if this device is maybe more like the G510, where
once the BL is turned off it simply ignores any updates send
from the PC? See lg_g510_leds_event() where we restore the cached
values when the bl is toggled on so that any changes done in sysfs
get applied when the bl is turned back on through the button on
the keyboard. In this G510 case no brightness_hw_changed events
are send. Although I'm not entirely sure why, we could still do
that and get notifications on the on/off press.

Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[Leo L. Schwab]

	Didn't directly receive the intermediate reply:

On Tue, Sep 02, 2025 at 11:14:09AM +0200, Hans de Goede wrote:
> On 2-Sep-25 11:07 AM, Hans de Goede wrote:
> > Ah I see. Yes if you do need to do a CONFIG check then using IS_ENABLED()
> > is good.
> > 
> > But I'm afraid that the underlying problem here is the use of
> > cdev.brightness_hw_changed this is really only meant for led-class.c
> > internal use.
> > 
	Then there should be a comment in the include file to that effect.

> > The idea of cdev.brightness_hw_changed is that it stores the last
> > value set by the hw.
> > 
> > But in the mean time that value may have been overwritten by software.
> > 
> > I think that you will fail to call led_classdev_notify_brightness_hw_changed()
> > (you can add a debug print to check) if the following happens:
> > 
> > 1. Brightness set to 255 (RGB 255,255,255) through sysfs
> > 2. State toggled to off by backlight control button, brightness is now 0
> > 3. Brightness set to 255 (RGB 255,255,255) through sysfs
> > 4. State toggled to off by backlight control button, brightness is now 0
> > 
	This does not happen.  The G13 accepts and remembers backlight color
settings even when the LEDs have been toggled off locally.

```
#### Initial state: Backlight on, full green:
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
255
0 255 0
root:/sys/class/leds/g13:rgb:kbd_backlight# echo 255 0 0 > multi_intensity 
#### Backlight is on, full red.
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
255
255 0 0
#### Backlight toggle button pressed; backlight is now off.
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
0
255 0 0
root:/sys/class/leds/g13:rgb:kbd_backlight# echo 0 0 255 > multi_intensity 
#### Backlight color set to full blue, but is still off.
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
0
0 0 255
#### Backlight toggle button pressed; backlight is now on, and blue.
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
255
0 0 255
```

	This also works if you alter `brightness` while the backlight is
toggled off.  IMHO, this is correct, principle-of-least-surprise behavior.

	Further (at least on my machine), `brightness_hw_changed` is
read-only in sysfs, and therefore can't be altered by host software.
Therefore, it would seem that using `cdev.brightness_hw_changed` as a cache
value is valid.  Otherwise, as you ouline below, we'd have to go through all
the workqueue gymnastics.

> > I also see that you use TEST_BIT(rep->keybits, 23) ? LED_FULL : LED_OFF
> > for the brightness value send to led_classdev_notify_brightness_hw_changed()
> > but I would expect the hw to restore the previous brightness on a toggle
> > from off -> on through the button? So then that should be send.
> > 
> > And you also never update cdev.brightness and use the cached 
> > struct lg_g15_led.brightness in lg_g13_kbd_led_get(). This means that
> > after a hw toggle of the backlight reading the brightness from sysfs
> > will show the wrong (old) value.
> > 
	This prompts the question:  What is the full intensity calculation
formula intended to be?  The docs appear to be rather quiet on this point.
If we assume all intensity/brightness values (0-255) are essentially mapped
to the range [0.0, 1.0], then it seems to me the calculation is:

	out = intensity * brightness * brightness_hw_changed

	i.e., turning either brightness value down to zero will turn the LED
"off" without affecting the other values, so when you turn it back on again,
you'll get back the color/other brightness you started with.  (Corollary:
For software to know the current output value, it must consider all three
input values.)

	I'm further assuming that brightness_hw_changed should have the same
range as brightness, as there is no separate `max_brightness_hw_changed`
sysfs value.

> > I think that instead what you need to do is create a
> > lg_g13_leds_changed_work() mirroring lg_g15_leds_changed_work()

	I dissent.  But then it's entirely possible I'm still missing
something...

	The only edge case I'm immediately aware of is:

	* Plug in G13.
	* Toggle backlight off.
	* Unload kernel module.
	* Reload kernel module.

	The backlight is now toggled off, but the newly loaded driver
doesn't know this.  Attempting to read `brightness_hw_changed` from sysfs at
this point will result in ENODATA (essentially reporting, "I don't know").
AFAIK, there is no way to probe the G13 for the current state of the
backlight HW toggle.  However, the moment the user generates any event on
the G13, the correct state will be obtained, and `brightness_hw_changed`
will be updated accordingly.  Not ideal, but seemed the most honest
approach.

> p.s.
> 
> Hmm, I wonder if this device is maybe more like the G510, where
> once the BL is turned off it simply ignores any updates send
> from the PC?  [ ... ]

	Nope.  As per my experiments above, the G13 accepts and remembers
backlight color updates even when the backlight is locally toggled off.

					Schwab

Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[Hans de Goede]

Hi,

On 2-Sep-25 22:41, Leo L. Schwab wrote:
> 	Didn't directly receive the intermediate reply:
> 
> On Tue, Sep 02, 2025 at 11:14:09AM +0200, Hans de Goede wrote:
>> On 2-Sep-25 11:07 AM, Hans de Goede wrote:
>>> Ah I see. Yes if you do need to do a CONFIG check then using IS_ENABLED()
>>> is good.
>>>
>>> But I'm afraid that the underlying problem here is the use of
>>> cdev.brightness_hw_changed this is really only meant for led-class.c
>>> internal use.
>>>
> 	Then there should be a comment in the include file to that effect.
> 
>>> The idea of cdev.brightness_hw_changed is that it stores the last
>>> value set by the hw.
>>>
>>> But in the mean time that value may have been overwritten by software.
>>>
>>> I think that you will fail to call led_classdev_notify_brightness_hw_changed()
>>> (you can add a debug print to check) if the following happens:
>>>
>>> 1. Brightness set to 255 (RGB 255,255,255) through sysfs
>>> 2. State toggled to off by backlight control button, brightness is now 0
>>> 3. Brightness set to 255 (RGB 255,255,255) through sysfs
>>> 4. State toggled to off by backlight control button, brightness is now 0
>>>
> 	This does not happen.  The G13 accepts and remembers backlight color
> settings even when the LEDs have been toggled off locally.
> 
> ```
> #### Initial state: Backlight on, full green:
> root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
> 255
> 255
> 0 255 0
> root:/sys/class/leds/g13:rgb:kbd_backlight# echo 255 0 0 > multi_intensity 
> #### Backlight is on, full red.
> root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
> 255
> 255
> 255 0 0
> #### Backlight toggle button pressed; backlight is now off.
> root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
> 255
> 0
> 255 0 0
> root:/sys/class/leds/g13:rgb:kbd_backlight# echo 0 0 255 > multi_intensity 
> #### Backlight color set to full blue, but is still off.
> root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
> 255
> 0
> 0 0 255
> #### Backlight toggle button pressed; backlight is now on, and blue.
> root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
> 255
> 255
> 0 0 255
> ```
> 
> 	This also works if you alter `brightness` while the backlight is
> toggled off.  IMHO, this is correct, principle-of-least-surprise behavior.

I see, interesting.

So what happens if you turn off the backlight with the toggle button on the G13
and then write 0 to brightness in sysfs and then press the toggle button again?

> 	Further (at least on my machine), `brightness_hw_changed` is
> read-only in sysfs, and therefore can't be altered by host software.
> Therefore, it would seem that using `cdev.brightness_hw_changed` as a cache
> value is valid.

Right it does seem that using cdev.brightness_hw_changed is valid in
this case.

But the LED API is supposed to have the brightness attribute present
the actual current brightness of the device.

I'm not sure how upower will react if the poll() on brightness_hw_changed
wakes upower up and then the reported brightness is unchanged...

I need to think about this a bit and check the upower code, let me
get back to you on this in a day or 2 ...

>>> I also see that you use TEST_BIT(rep->keybits, 23) ? LED_FULL : LED_OFF
>>> for the brightness value send to led_classdev_notify_brightness_hw_changed()
>>> but I would expect the hw to restore the previous brightness on a toggle
>>> from off -> on through the button? So then that should be send.
>>>
>>> And you also never update cdev.brightness and use the cached 
>>> struct lg_g15_led.brightness in lg_g13_kbd_led_get(). This means that
>>> after a hw toggle of the backlight reading the brightness from sysfs
>>> will show the wrong (old) value.
>>>
> 	This prompts the question:  What is the full intensity calculation
> formula intended to be?  The docs appear to be rather quiet on this point.
> If we assume all intensity/brightness values (0-255) are essentially mapped
> to the range [0.0, 1.0], then it seems to me the calculation is:
> 
> 	out = intensity * brightness * brightness_hw_changed

The way the API is intended to work is that after a hw-brightness-changes
event brightness == brightness_hw_changed in sysfs.

brightness_hw_changed only purpose is to cache the last hw set value so
that if userspace writing a new value to brightness races with
a brightness_hw_changed event a userspace process listening can still
read which value the hw actually send. TBH this is of little value to
userspace but the LED subsys maintainer insisted on this.

IOW the formula should always be:

 	out = intensity * brightness

The problem is that in the G13 case the real formula is:

 	out = intensity * brightness * internal-g13-toggle-bool

And there is no standard userspace API for how to deal with
the device having an internal-g13-toggle-bool

The G510 has something similar which is why I left out
brightness_hw_changed event generation in the G510 code.

As mentioned before I need to think a bit about how to handle
this. I have an idea howto handle this and I can try and
prototype this on the G510 which has more or less the same
problem, except that it simply throws away brightness writes
while toggled off with the button.


> 
>>> I think that instead what you need to do is create a
>>> lg_g13_leds_changed_work() mirroring lg_g15_leds_changed_work()
> 
> 	I dissent.  But then it's entirely possible I'm still missing
> something...
> 
> 	The only edge case I'm immediately aware of is:
> 
> 	* Plug in G13.
> 	* Toggle backlight off.
> 	* Unload kernel module.
> 	* Reload kernel module.
> 
> 	The backlight is now toggled off, but the newly loaded driver
> doesn't know this.  Attempting to read `brightness_hw_changed` from sysfs at
> this point will result in ENODATA (essentially reporting, "I don't know").
> AFAIK, there is no way to probe the G13 for the current state of the
> backlight HW toggle.  However, the moment the user generates any event on
> the G13, the correct state will be obtained, and `brightness_hw_changed`
> will be updated accordingly.  Not ideal, but seemed the most honest
> approach.

Ack.

Regards,

Hans

Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[Leo L. Schwab]

	For some reason, your replies aren't making it to me directly -- I
had to find and scrape your reply off the LKML web site:

On Tue, 2 Sep 2025 23:05:06 +0200, Hans de Goede wrote:
> On 2-Sep-25 22:41, Leo L. Schwab wrote:
>> 	This does not happen.  The G13 accepts and remembers backlight color
>> settings even when the LEDs have been toggled off locally.
>> [ ... ]
>
> I see, interesting.
>
> So what happens if you turn off the backlight with the toggle button on the G13
> and then write 0 to brightness in sysfs and then press the toggle button again?
>
	It's a little difficult to see, but the backlight turns back on with
minimal brightness.  To my eye, it looks like it's displaying #000001.

> Right it does seem that using cdev.brightness_hw_changed is valid in
> this case.
>
> But the LED API is supposed to have the brightness attribute present
> the actual current brightness of the device.
>
> I'm not sure how upower will react if the poll() on brightness_hw_changed
> wakes upower up and then the reported brightness is unchanged...
>
> I need to think about this a bit and check the upower code, let me
> get back to you on this in a day or 2 ...
>
	Certainly.

>> 	This prompts the question:  What is the full intensity calculation
>> formula intended to be?  The docs appear to be rather quiet on this point.
>> If we assume all intensity/brightness values (0-255) are essentially mapped
>> to the range [0.0, 1.0], then it seems to me the calculation is:
>>
>> 	out = intensity * brightness * brightness_hw_changed
>
> The way the API is intended to work is that after a hw-brightness-changes
> event brightness == brightness_hw_changed in sysfs.
> [ ... ]
> IOW the formula should always be:
>
>  	out = intensity * brightness
>
	Then this should be written down somewhere.  A quick ripgrep through
the 6.16 source tree shows brightness_hw_changed is used in *very* few
places so far, so it'd be good to get this clarified before too many other
people start having competing interpretations.

> As mentioned before I need to think a bit about how to handle
> this. [ ... ]

	Fair enough.  I'll hold off on spinning a v6 until I hear from you.

					Schwab