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

["Leo L. Schwab" <ewhac@ewhac.org>]

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