From: Arnd Bergmann <arnd@arndb.de>
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@arndb.de>
---
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;
--
2.39.5Hi dee Ho peeps,
On 08/08/2025 18:17, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> 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@arndb.de>
> ---
> 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
On Mon, Aug 11, 2025 at 01:34:43PM +0300, Matti Vaittinen wrote: > On 08/08/2025 18:17, Arnd Bergmann wrote: ... > 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? FWIW, on Intel platforms that use power button by PMIC we add a special driver for each of such cases. -- With Best Regards, Andy Shevchenko
On Mon, Aug 11, 2025 at 03:52:19PM +0300, Andy Shevchenko wrote: > On Mon, Aug 11, 2025 at 01:34:43PM +0300, Matti Vaittinen wrote: > > On 08/08/2025 18:17, Arnd Bergmann wrote: > > ... > > > 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? > > FWIW, on Intel platforms that use power button by PMIC we add a special driver > for each of such cases. If we can make gpio-keys work for various power buttons that would be great IMO. The MFD drivers in question already are using device tree, but they do not define/expect nodes for the power buttons. If the nodes were there then I think gpio-keys would work out of the box? Thanks. -- Dmitry
On Mon, Aug 11, 2025 at 12:21:51PM -0700, Dmitry Torokhov wrote: > On Mon, Aug 11, 2025 at 03:52:19PM +0300, Andy Shevchenko wrote: > > On Mon, Aug 11, 2025 at 01:34:43PM +0300, Matti Vaittinen wrote: > > > On 08/08/2025 18:17, Arnd Bergmann wrote: ... > > > 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? > > > > FWIW, on Intel platforms that use power button by PMIC we add a special driver > > for each of such cases. > > If we can make gpio-keys work for various power buttons that would be > great IMO. The MFD drivers in question already are using device tree, > but they do not define/expect nodes for the power buttons. If the nodes > were there then I think gpio-keys would work out of the box? Looking at the, e.g., https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/x86/intel/mrfld_pwrbtn.c, I am not sure it's as simply as it sounds. Basically it's an IRQ, which requires IRQ handling and proper acking/masking/etc. -- With Best Regards, Andy Shevchenko
On 11/08/2025 23:09, Andy Shevchenko wrote: > On Mon, Aug 11, 2025 at 12:21:51PM -0700, Dmitry Torokhov wrote: >> On Mon, Aug 11, 2025 at 03:52:19PM +0300, Andy Shevchenko wrote: >>> On Mon, Aug 11, 2025 at 01:34:43PM +0300, Matti Vaittinen wrote: >>>> On 08/08/2025 18:17, Arnd Bergmann wrote: > > ... > >>>> 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? >>> >>> FWIW, on Intel platforms that use power button by PMIC we add a special driver >>> for each of such cases. >> >> If we can make gpio-keys work for various power buttons that would be >> great IMO. The MFD drivers in question already are using device tree, >> but they do not define/expect nodes for the power buttons. If the nodes >> were there then I think gpio-keys would work out of the box? > > Looking at the, e.g., https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/x86/intel/mrfld_pwrbtn.c, > I am not sure it's as simply as it sounds. Basically it's an IRQ, which > requires IRQ handling and proper acking/masking/etc. In some (many?) cases the interrupts (acking/masking) are handled by an irqchip code. When this is the case, the gpio-keys (or any other power-button code) does not need to care about IRQ-specifics. (I don't know about the Intel driver though.) Problem with many of the bd718* (and probably some other MFD drivers) is, that the interrupts are really relevant only for the drivers specific to this one device (like PMIC in ROHM case). When this is the case, the device is not really (from the HW perspective) an interrupt-controller, which means it shouldn't probably be marked as one in the device-tree either. It will then also mean that there can't be meaningful interrupt specification for the button IRQ in the device-tree, right? Additionally, we have devices where most of the interrupts are internal to the PMIC, but then the PMIC also has some pins usable as GPIO, which can be used as interrupt sources. Eg, someone can connect another device to these pins - which makes the PMIC an interrupt-controller. For these PMICs the power-button IRQ can be provided via device-tree node (but the IRQ spec may become a bit hairy, since most of the IRQs are meant to be internal). Hence, for me, providing the IRQ number in platform data seems still to be the right thing to do :) TLDR; I agree with Dmitry. It's nice to have an easily re-usable power-button handler, which requires no IC-specific code. Gpio-keys works for simple IRQ based power-buttons where IRQ controller takes care of the acks/masks. I just wanted to know if it is for some reason discouraged, or if I've used it in a wrong way (because the grep resulted so few results). Yours, -- Matti
© 2016 - 2025 Red Hat, Inc.