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

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

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

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.

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.

	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

Hi Leo,

On 3-Sep-25 9:39 PM, Leo L. Schwab wrote:
> 	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.

Ok.

>> 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.

Thank you for waiting. After looking at the upower code + running some
tests with a G510 I think that your hw_brightness_changed support
is pretty good as is.

There are 2 improvements which I would like to see:

1. When the backlight is turned on through the button, you
should pass g15_led->brightness to the notify() call rather
then LED_FULL. GNOME will show an OSD with the new brightness
value shown as a mini progress bar similar to how it shows
speaker volume when doing mute/unmute. This mini progress
bar should show the actual brightness being restored, not
always full brightness.

2. ATM if the backlight is turned off on the G13 when
the driver loads and then one of the buttons gets pressed
then a notify() will happen because the led_cdev.hw_brightness_changed
value of -1 will be different from the value of 0 in the
input-report. This notify will lead to an unwanted OSD
notification in GNOME, so this needs to be fixed.
IMHO the best fix would be to use:

	hid_hw_raw_request(..., HID_INPUT_REPORT, HID_REQ_GET_REPORT);

at probe to get the input-report so that the driver will
actually now the backlight state at probe() time without
needing to wait for the first time the input-report is send.

You have inspired me to add hw_brightness_changed support
to the G510 code, see the attached patch. This patch can
also be used as an example how to get the input report
on the G13 during probe().

Note this also adds a variable at the driver level to
track the backlight state also fixing the compile issue
you hit without needing to use #ifdef-ery.

I'll wait for your G13 support to land first and then
rebase the G510 patch on top.

Regards,

Hans

On Mon, Sep 08, 2025 at 11:08:29PM +0200, Hans de Goede wrote:
> There are 2 improvements which I would like to see:
> 
> 1. When the backlight is turned on through the button, you
> should pass g15_led->brightness to the notify() call rather
> then LED_FULL. GNOME will show an OSD with the new brightness
> value shown as a mini progress bar similar to how it shows
> speaker volume when doing mute/unmute. This mini progress
> bar should show the actual brightness being restored, not
> always full brightness.
>
	If g15_led->brightness is subsequently changed, should a new
notify() call also be made with that new brightness, i.e. should
`hw_brightness_changed` be made to track `brightness`?  Indeed, it looks
like you do this in `lg_g15_leds_changed_work()`.

> 2. ATM if the backlight is turned off on the G13 when
> the driver loads and then one of the buttons gets pressed
> then a notify() will happen because the led_cdev.hw_brightness_changed
> value of -1 will be different from the value of 0 in the
> input-report. This notify will lead to an unwanted OSD
> notification in GNOME, so this needs to be fixed.
> IMHO the best fix would be to use:
> 
> 	hid_hw_raw_request(..., HID_INPUT_REPORT, HID_REQ_GET_REPORT);
> 
> at probe to get the input-report so that the driver will
> actually now the backlight state at probe() time without
> needing to wait for the first time the input-report is send.
>
	Will give this a try.

> I'll wait for your G13 support to land first and then
> rebase the G510 patch on top.
> 
	Roger that.

					Schwab

Hi,

On 10-Sep-25 7:52 AM, Leo L. Schwab wrote:
> On Mon, Sep 08, 2025 at 11:08:29PM +0200, Hans de Goede wrote:
>> There are 2 improvements which I would like to see:
>>
>> 1. When the backlight is turned on through the button, you
>> should pass g15_led->brightness to the notify() call rather
>> then LED_FULL. GNOME will show an OSD with the new brightness
>> value shown as a mini progress bar similar to how it shows
>> speaker volume when doing mute/unmute. This mini progress
>> bar should show the actual brightness being restored, not
>> always full brightness.
>>
> 	If g15_led->brightness is subsequently changed, should a new
> notify() call also be made with that new brightness, i.e. should
> `hw_brightness_changed` be made to track `brightness`?

No, hw_brightness_changed only track changes done independently
by the hw. sysfs writes should not call notify().

> Indeed, it looks
> like you do this in `lg_g15_leds_changed_work()`.

That is for the original G15 and G15-v2, where the buttion
cycles through a couple of backlight levels (IIRC). That work
only gets queued when we receive a button press notification
and then it *reads* the new brightness from the keyboard and
uses that for the notify().

That work does not get queued/used for normal sysfs writes.

>> 2. ATM if the backlight is turned off on the G13 when
>> the driver loads and then one of the buttons gets pressed
>> then a notify() will happen because the led_cdev.hw_brightness_changed
>> value of -1 will be different from the value of 0 in the
>> input-report. This notify will lead to an unwanted OSD
>> notification in GNOME, so this needs to be fixed.
>> IMHO the best fix would be to use:
>>
>> 	hid_hw_raw_request(..., HID_INPUT_REPORT, HID_REQ_GET_REPORT);
>>
>> at probe to get the input-report so that the driver will
>> actually now the backlight state at probe() time without
>> needing to wait for the first time the input-report is send.
>>
> 	Will give this a try.
> 
>> I'll wait for your G13 support to land first and then
>> rebase the G510 patch on top.
>>
> 	Roger that.

Regards,

Hans

On Wed, Sep 10, 2025 at 01:09:10PM +0200, Hans de Goede wrote:
> On 10-Sep-25 7:52 AM, Leo L. Schwab wrote:
> > On Mon, Sep 08, 2025 at 11:08:29PM +0200, Hans de Goede wrote:
> >> There are 2 improvements which I would like to see:
> >>
> >> 1. When the backlight is turned on through the button, you
> >> should pass g15_led->brightness to the notify() call rather
> >> then LED_FULL. GNOME will show an OSD with the new brightness
> >> value shown as a mini progress bar similar to how it shows
> >> speaker volume when doing mute/unmute. This mini progress
> >> bar should show the actual brightness being restored, not
> >> always full brightness.
> >>
> > 	If g15_led->brightness is subsequently changed, should a new
> > notify() call also be made with that new brightness, i.e. should
> > `hw_brightness_changed` be made to track `brightness`?
> 
> No, hw_brightness_changed only track changes done independently
> by the hw. sysfs writes should not call notify().
>
	Erm...  So brightness_hw_changed should only sample
g15_led->brightness on first probe?

	What should happen in this case:

	* Driver loads, probes G13 backlight's current color, calculates
	  brightness to be 50, sets both `brightness` and
	  `brightness_hw_changed` sysfs values to 50.
	* User presses toggle key; backlight is now off.
	  `brightness_hw_changed` now set to 0.
	  `brightness` and RGB values remain unchanged.
	* User writes to `brightness` sysfs value, setting it to 255.  This
	  does *not* turn the backlight back on; `hw_brightness_changed`
	  remains unchanged.
	* User presses toggle key; backlight is back on, showing the
	  original color, but brighter.

	What should brightness_hw_changed be updated to, if anything?

> >> IMHO the best fix would be to use:
> >>
> >> 	hid_hw_raw_request(..., HID_INPUT_REPORT, HID_REQ_GET_REPORT);
> >> [ ... ]
> > 
> > 	Will give this a try.
> > 
	Got this part working.

					Schwab

Hi,

On 10-Sep-25 8:02 PM, Leo L. Schwab wrote:
> On Wed, Sep 10, 2025 at 01:09:10PM +0200, Hans de Goede wrote:
>> On 10-Sep-25 7:52 AM, Leo L. Schwab wrote:
>>> On Mon, Sep 08, 2025 at 11:08:29PM +0200, Hans de Goede wrote:
>>>> There are 2 improvements which I would like to see:
>>>>
>>>> 1. When the backlight is turned on through the button, you
>>>> should pass g15_led->brightness to the notify() call rather
>>>> then LED_FULL. GNOME will show an OSD with the new brightness
>>>> value shown as a mini progress bar similar to how it shows
>>>> speaker volume when doing mute/unmute. This mini progress
>>>> bar should show the actual brightness being restored, not
>>>> always full brightness.
>>>>
>>> 	If g15_led->brightness is subsequently changed, should a new
>>> notify() call also be made with that new brightness, i.e. should
>>> `hw_brightness_changed` be made to track `brightness`?
>>
>> No, hw_brightness_changed only track changes done independently
>> by the hw. sysfs writes should not call notify().
>>
> 	Erm...  So brightness_hw_changed should only sample
> g15_led->brightness on first probe?
> 
> 	What should happen in this case:
> 
> 	* Driver loads, probes G13 backlight's current color, calculates
> 	  brightness to be 50, sets both `brightness` and
> 	  `brightness_hw_changed` sysfs values to 50.
> 	* User presses toggle key; backlight is now off.
> 	  `brightness_hw_changed` now set to 0.
> 	  `brightness` and RGB values remain unchanged.
> 	* User writes to `brightness` sysfs value, setting it to 255.  This
> 	  does *not* turn the backlight back on; `hw_brightness_changed`
> 	  remains unchanged.
> 	* User presses toggle key; backlight is back on, showing the
> 	  original color, but brighter.
> 
> 	What should brightness_hw_changed be updated to, if anything?

Since the driver writes any new values to the G13 and the G13 accepts
those and remembers them even when the backlight is off,
the notify() should be passed g15_led->brightness when an
off -> on transition happens (and 0 or LED_OFF for the on -> off
transition).

Since g15_led->brightness gets initialized by reading the actual
setting from the G13 at probe time and then gets updated on
any successful completion if writing a new brightness value
to the G13, it should always reflect the value which the backlight
will be set at by the G13 after an off -> on transition.

Or am I missing something ?

In case of your example step above then the notify() should
be passed 255 as brightness and that should also be the value
in g15_led->brightness since g15_led->brightness gets set
to the brightness send to the G13 hw on a successful setting
of the report, right ?

Regards,

Hans

On Wed, Sep 10, 2025 at 09:16:45PM +0200, Hans de Goede wrote:
> Since the driver writes any new values to the G13 and the G13 accepts
> those and remembers them even when the backlight is off,
> the notify() should be passed g15_led->brightness when an
> off -> on transition happens (and 0 or LED_OFF for the on -> off
> transition).
> 
> Since g15_led->brightness gets initialized by reading the actual
> setting from the G13 at probe time and then gets updated on
> any successful completion if writing a new brightness value
> to the G13, it should always reflect the value which the backlight
> will be set at by the G13 after an off -> on transition.
> 
> Or am I missing something ?
>
	If I'm understanding you correctly:

	You want `brightness` to be copied to `brightness_hw_changed` on
probe, and on every backlight off->on transition (cool so far).

	What do you want to happen to `brightness_hw_changed` when
`brightness` is changed in sysfs while the backlight is on?  As it stands,
the current behavior is:
	* Driver loads and probes; `brightness` and `brightness_hw_changed`
	  both set to 255.
	* sysfs `brightness` changed to 128.  `brightness_hw_changed`
	  remains at 255.
	* Toggle backilght off.  `brightness_hw_changed` changed to 0.
	  `brightness` remains at 128.
	* Toggle backlight back on.  `brightness_hw_changed` gets a copy of
	  `brightness`, and both are now 128.

	This seems inconsistent to me.  Hence my earlier suggestion that
`brightness_hw_changed` should track all changes to `brightness`, except
when the backlight is toggled off.

					Schwab

Hi Leo,

On 16-Sep-25 00:18, Leo L. Schwab wrote:
> On Wed, Sep 10, 2025 at 09:16:45PM +0200, Hans de Goede wrote:
>> Since the driver writes any new values to the G13 and the G13 accepts
>> those and remembers them even when the backlight is off,
>> the notify() should be passed g15_led->brightness when an
>> off -> on transition happens (and 0 or LED_OFF for the on -> off
>> transition).
>>
>> Since g15_led->brightness gets initialized by reading the actual
>> setting from the G13 at probe time and then gets updated on
>> any successful completion if writing a new brightness value
>> to the G13, it should always reflect the value which the backlight
>> will be set at by the G13 after an off -> on transition.
>>
>> Or am I missing something ?
>>
> 	If I'm understanding you correctly:


> 	You want `brightness` to be copied to `brightness_hw_changed` on
> probe, and on every backlight off->on transition (cool so far).

Just to clarify, there are 2 things here

1: brightness as in the actual rgb/brightness values the backlight will
   be lit with when it is on
2. A backlight_disabled flag to indicate if the backlight is disabled
   in hw by the button on the G13

I would suggest to track those separately by adding a backlight_disabled
(or backlight_enabled) flag to struct lg_g15_data like I do in the
g510 patch I send earlier in the thread.

So wrt copying things on probe() I would copy both the brightness
to g15_led->brightness which is already done in v3 as well as 
use something like:

        ret = hid_hw_raw_request(g15->hdev, LG_G510_INPUT_KBD_BACKLIGHT,
                                 g15->transfer_buf, 2,
                                 HID_INPUT_REPORT, HID_REQ_GET_REPORT);

to get the input-report with the backlight_enabled/disabled flag and
initialize backlight_disabled based on that.

I would not touch mcled.cdev.brightness_hw_changed directly,
not touching this will also avoid the build issues when
support for it is disabled.

Ack to on detecting a backlight off->on transition based on comparing
the input-report bit to the cached backlight_disabled flag pass
the cached g15_led->brightness to notify()

> 	What do you want to happen to `brightness_hw_changed` when
> `brightness` is changed in sysfs while the backlight is on?  As it stands,
> the current behavior is:
> 	* Driver loads and probes; `brightness` and `brightness_hw_changed`
> 	  both set to 255.

Ack, except that as mentioned above I would not touch brightness_hw_changed
and just leave it at -1.

> 	* sysfs `brightness` changed to 128.  `brightness_hw_changed`
> 	  remains at 255.
> 	* Toggle backilght off.  `brightness_hw_changed` changed to 0.
> 	  `brightness` remains at 128.
> 	* Toggle backlight back on.  `brightness_hw_changed` gets a copy of
> 	  `brightness`, and both are now 128.

Ack this is all correct.

> 	This seems inconsistent to me.

This is working as intended / how the API was designed as
Documentation/ABI/testing/sysfs-class-led says:

                Reading this file will return the last brightness level set
                by the hardware, this may be different from the current
                brightness. Reading this file when no hw brightness change
                event has happened will return an ENODATA error.

>  Hence my earlier suggestion that
> `brightness_hw_changed` should track all changes to `brightness`, except
> when the backlight is toggled off.

Then it also would be reporting values coming from sysfs writes,
which it explicitly should not do.

Summarizing in my view the following changes are necessary on v4:

1. Add backlight_disabled (or backlight_enabled) flag to struct lg_g15_data
2. Init that flag from prope()
3. Use that flag on receiving input reports to see if notify()
   should be called
4. Replace the LED_FULL passed to notify() (for off->on)
   with g15_led->brightness

and that is it, with those changes I believe that we should be
good to go.

Regards,

Hans

On Wed, Sep 17, 2025 at 12:33:36PM +0200, Hans de Goede wrote:
> On 16-Sep-25 00:18, Leo L. Schwab wrote:
> > 	What do you want to happen to `brightness_hw_changed` when
> > `brightness` is changed in sysfs while the backlight is on?  As it stands,
> > the current behavior is:
> > 	* Driver loads and probes; `brightness` and `brightness_hw_changed`
> > 	  both set to 255.
> 
> Ack, except that as mentioned above I would not touch brightness_hw_changed
> and just leave it at -1.
> 
> > 	* sysfs `brightness` changed to 128.  `brightness_hw_changed`
> > 	  remains at 255.
> > 	* Toggle backilght off.  `brightness_hw_changed` changed to 0.
> > 	  `brightness` remains at 128.
> > 	* Toggle backlight back on.  `brightness_hw_changed` gets a copy of
> > 	  `brightness`, and both are now 128.
> 
> Ack this is all correct.
>
	...Oy.

	Okay, I can give you that.

> > 	This seems inconsistent to me.
> 
> This is working as intended / how the API was designed as
> Documentation/ABI/testing/sysfs-class-led says:
> 
>                 Reading this file will return the last brightness level set
>                 by the hardware, this may be different from the current
>                 brightness. Reading this file when no hw brightness change
>                 event has happened will return an ENODATA error.
>
	First: Why isn't this mentioned in Documentation/leds/leds_class.rst?

	And second: This doesn't really clarify anything.  That paragraph
may be legitimately interpreted to mean that `brightness_hw_changed` is
completely independent of `brightness`, as it was in my original
implementation.

> >  Hence my earlier suggestion that
> > `brightness_hw_changed` should track all changes to `brightness`, except
> > when the backlight is toggled off.
> 
> Then it also would be reporting values coming from sysfs writes,
> which it explicitly should not do.
>
	Okay, fair, but having `brightness_hw_changed` read as 255, then
later as 128 after hitting the toggle button a couple of times strikes me as
inconsistent behavior.

> Summarizing in my view the following changes are necessary on v4:
> 
> 1. Add backlight_disabled (or backlight_enabled) flag to struct lg_g15_data
> 2. Init that flag from prope()
> 3. Use that flag on receiving input reports to see if notify()
>    should be called
> 4. Replace the LED_FULL passed to notify() (for off->on)
>    with g15_led->brightness
> 
	Will do; will post v6 shortly.  And someone should update the docs
describing the expected interaction between `brightness` and
`brightness_hw_changed`.

					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

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