Re: [PATCH 02/10] [v3] input: gpio-keys: make legacy gpiolib optional

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying the legacy GPIO validation to check `!button->irq` breaks legacy devices that legitimately provide both a GPIO and an IRQ.
- [Medium] Conditionally removing the `gpio` field from `struct gpio_keys_button` causes build failures in drivers that assign to this field when `CONFIG_GPIOLIB_LEGACY` is disabled.
--

commit cb399daff5ceabcf54d5ea611fd309f5c0600ab4
Author: Arnd Bergmann <arnd@arndb.de>

input: gpio-keys: make legacy gpiolib optional

This commit makes the legacy gpiolib interface optional for gpio-keys and
gpio-keys-polled by adding ifdef blocks. It also modifies the gpio-keys
driver to ignore the gpio number if an IRQ is passed to keep Rohm PMIC
drivers working with and without CONFIG_GPIOLIB_LEGACY.

Link: https://lore.kernel.org/all/b3c94552-c104-42e3-be15-7e8362e8039e@gmail.com/
Link: https://lore.kernel.org/all/afJXG4_rtaj3l2Dk@google.com/

> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
[ ... ]
> @@ -528,7 +528,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  			 */
>  			bdata->gpiod = NULL;
>  		}
> -	} else if (gpio_is_valid(button->gpio)) {
> +#ifdef CONFIG_GPIOLIB_LEGACY
> +	} else if (!button->irq && gpio_is_valid(button->gpio)) {

[Severity: High]
Does this change break legacy devices that legitimately provide both a valid
GPIO and a valid IRQ?

If a legacy platform provides both a valid GPIO and an IRQ, this condition
now evaluates to false.

By skipping the GPIO request entirely, the driver falls back to treating the
device as a pure IRQ button. This forces the driver to lose hardware
debouncing and rely on a simulated release timer instead of checking the
physical GPIO line.

[ ... ]
> diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
> --- a/include/linux/gpio_keys.h
> +++ b/include/linux/gpio_keys.h
[ ... ]
> @@ -25,7 +25,9 @@ struct device;
>   */
>  struct gpio_keys_button {
>  	unsigned int code;
> +#ifdef CONFIG_GPIOLIB_LEGACY
>  	int gpio;
> +#endif
>  	int active_low;

[Severity: Medium]
Will this cause build failures for drivers that assign to this field when
CONFIG_GPIOLIB_LEGACY is disabled?

Other modern drivers dynamically create the platform device and pass legacy
GPIO numbers by directly assigning to this field:

drivers/input/misc/soc_button_array.c:soc_button_device_create() {
    ...
    gpio_keys[n_buttons].gpio = gpio;
    ...
}

When CONFIG_GPIOLIB_LEGACY is disabled, this results in a compilation failure
since struct gpio_keys_button no longer has the gpio member.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=2