Re: [PATCH 09/21] input: gpio-keys: make legacy gpiolib optional

[Matti Vaittinen]

Hi dee Ho peeps,

On 08/08/2025 18:17, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> Most users of gpio-keys and gpio-keys-polled use modern gpiolib
> interfaces, but there are still number of ancient sh, arm32 and x86
> machines that have never been converted.
>
> Add an #ifdef block for the parts of the driver that are only
> used on those legacy machines.
>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
>   drivers/input/keyboard/gpio_keys.c        | 5 +++--
>   drivers/input/keyboard/gpio_keys_polled.c | 2 ++
>   drivers/mfd/rohm-bd71828.c                | 2 ++
>   drivers/mfd/rohm-bd718x7.c                | 2 ++
>   include/linux/gpio_keys.h                 | 2 ++
>   5 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index f9db86da0818..984b20f773ed 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -528,6 +528,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>   			 */
>   			bdata->gpiod = NULL;
>   		}
> +#ifdef CONFIG_GPIOLIB_LEGACY
>   	} else if (gpio_is_valid(button->gpio)) {
>   		/*
>   		 * Legacy GPIO number, so request the GPIO here and
> @@ -546,6 +547,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>   
>   		if (button->active_low ^ gpiod_is_active_low(bdata->gpiod))
>   			gpiod_toggle_active_low(bdata->gpiod);
> +#endif
>   	}
>   
>   	if (bdata->gpiod) {
> @@ -583,8 +585,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>   			if (irq < 0) {
>   				error = irq;
>   				dev_err_probe(dev, error,
> -					      "Unable to get irq number for GPIO %d\n",
> -					      button->gpio);
> +					      "Unable to get irq number for GPIO\n");
>   				return error;
>   			}
>   			bdata->irq = irq;
> diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
> index e6707d72210e..0ae0e53910ea 100644
> --- a/drivers/input/keyboard/gpio_keys_polled.c
> +++ b/drivers/input/keyboard/gpio_keys_polled.c
> @@ -301,6 +301,7 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
>   				return dev_err_probe(dev, PTR_ERR(bdata->gpiod),
>   						     "failed to get gpio\n");
>   			}
> +#ifdef CONFIG_GPIOLIB_LEGACY
>   		} else if (gpio_is_valid(button->gpio)) {
>   			/*
>   			 * Legacy GPIO number so request the GPIO here and
> @@ -323,6 +324,7 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
>   
>   			if (button->active_low ^ gpiod_is_active_low(bdata->gpiod))
>   				gpiod_toggle_active_low(bdata->gpiod);
> +#endif
>   		}
>   
>   		bdata->last_state = -1;
> diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> index a14b7aa69c3c..fb68694fadca 100644
> --- a/drivers/mfd/rohm-bd71828.c
> +++ b/drivers/mfd/rohm-bd71828.c
> @@ -21,7 +21,9 @@
>   
>   static struct gpio_keys_button button = {
>   	.code = KEY_POWER,
> +#ifdef CONFIG_GPIOLIB_LEGACY
>   	.gpio = -1,
> +#endif
>   	.type = EV_KEY,
>   };
>   
> diff --git a/drivers/mfd/rohm-bd718x7.c b/drivers/mfd/rohm-bd718x7.c
> index 25e494a93d48..6c99ab62e31b 100644
> --- a/drivers/mfd/rohm-bd718x7.c
> +++ b/drivers/mfd/rohm-bd718x7.c
> @@ -20,7 +20,9 @@
>   
>   static struct gpio_keys_button button = {
>   	.code = KEY_POWER,
> +#ifdef CONFIG_GPIOLIB_LEGACY
>   	.gpio = -1,
> +#endif
>   	.type = EV_KEY,
>   };
>   
> diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
> index 80fa930b04c6..e8d6dc290efb 100644
> --- 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;
>   	const char *desc;
>   	unsigned int type;

AFAIR, these ROHM PMICs (bd718[15, 27, 28, 37, 47, 50, 78, 85]) all 
provide a 'button IRQ', from a power button. (Or, couple of IRQs but 
let's skip the details) The gpio-keys is used to send the KEY_POWER 
event when IRQ is detected.

The IRQ comes from the PMIC, and the regmap_irq chip provided by the MFD 
provides it. This IRQ information is delivered to the gpio-keys from the 
MFD driver via platform data. That's basically what these "button" 
structs are here for. No GPIO line information (only the IRQ number) is 
needed to be delivered to the gpio-keys. This problematic assignment:

> +#ifdef CONFIG_GPIOLIB_LEGACY
>   	.gpio = -1,
> +#endif

is only needed to invalidate the gpio information so that the gpio-keys 
wont use it, only the IRQ.

As such, this patch seems Ok to me, you can treat this as an ack :) 
This, however made me ponder following - is this the tight way to handle 
the power-button IRQ? I don't see any other MFD devices doing this in 
same way, although I am pretty sure there are other PMICs with similar 
power-button IRQ...

I see for example the "drivers/mfd/rt5120.c" to invoke 
"drivers/input/misc/rt5120-pwrkey.c" instead of using the gpio-keys. 
This, however, feels like code duplication to me. I'd rather kept using 
the gpio-keys, but seeing:

git grep KEY_POWER drivers/mfd/
drivers/mfd/rohm-bd71828.c:     .code = KEY_POWER,
drivers/mfd/rohm-bd718x7.c:     .code = KEY_POWER,

makes me wonder if there is more widely used (better) way?

Yours,
	-- Matti