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

[Hans de Goede <hansg@kernel.org>]

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