[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aLSntMknSv3lMarZ@ewhac.org>
Date: Sun, 31 Aug 2025 12:51:16 -0700
From: "Leo L. Schwab" <ewhac@...ac.org>
To: Hans de Goede <hansg@...nel.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.
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@...nel.org>
>
> to the next version.
>
Thank you very kindly, sir!
Schwab
Powered by blists - more mailing lists