| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7d356834-5795-4979-9f51-0ffcec52ae1d@kernel.org>
Date: Sun, 31 Aug 2025 15:01:12 +0200
From: Hans de Goede <hansg@...nel.org>
To: "Leo L. Schwab" <ewhac@...ac.org>
Cc: Kate Hsuan <hpa@...hat.com>, Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <bentiss@...nel.org>, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.
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@...ac.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@...nel.org>
to the next version.
Regards,
Hans
Powered by blists - more mailing lists