From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>, Lee Jones <lee@kernel.org>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mfd: rohm: Factor out power button registration
Date: Thu, 25 Jun 2026 11:00:08 +0300 [thread overview]
Message-ID: <a867d076-55cf-4686-a46f-6cb9fd198b90@gmail.com> (raw)
In-Reply-To: <ajQ-CtU131FAJ9ES@google.com>
On 18/06/2026 21:58, Dmitry Torokhov wrote:
> Factor out the power button registration logic using software nodes
> from rohm-bd718x7 and rohm-bd71828 drivers into a shared module
> rohm-pwrbutton.
>
> This reduces duplication and makes it easier to support other ROHM
> PMICs with similar power button configurations.
>
> Suggested-by: Lee Jones <lee@kernel.org>
> Assisted-by: Antigravity:gemini-3.5-flash
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks a lot Dmitry. I like the idea of having generic helpers for
crearting the swnodes for the power-button!
> ---
> MAINTAINERS | 2 +
> drivers/mfd/Kconfig | 6 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/rohm-bd71828.c | 84 ++------------------------
> drivers/mfd/rohm-bd718x7.c | 84 ++------------------------
> drivers/mfd/rohm-pwrbutton.c | 112 +++++++++++++++++++++++++++++++++++
> drivers/mfd/rohm-pwrbutton.h | 12 ++++
> 7 files changed, 141 insertions(+), 160 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f1caa6e5198b..40c46a7363fb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23524,6 +23524,8 @@ F: drivers/mfd/rohm-bd71828.c
> F: drivers/mfd/rohm-bd718x7.c
> F: drivers/mfd/rohm-bd9576.c
> F: drivers/mfd/rohm-bd96801.c
> +F: drivers/mfd/rohm-pwrbutton.c
> +F: drivers/mfd/rohm-pwrbutton.h
> F: drivers/regulator/bd71815-regulator.c
> F: drivers/regulator/bd71828-regulator.c
> F: drivers/regulator/bd718x7-regulator.c
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 763ce6a34782..8d04e1b1f8c8 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2208,6 +2208,10 @@ config MFD_STW481X
> in various ST Microelectronics and ST-Ericsson embedded
> Nomadik series.
>
> +config MFD_ROHM_PWRBUTTON
> + tristate
> + select MFD_CORE
I would drop the MFD dependency from the helper. It'd be nice to be able
to add the SW-nodes representing power-button(s) outside MFD as well.
Furthermore, I would drop the _ROHM_ from it as it should be useful for
other vendors as well.
I might then add another, "MFD-helper", which includes the MFD-specific
bits. If there are no other users but ROHM drivers in MFD, then that
could carry the ROHM in the name.
Finally, I am unsure if we should select the GPIO_KEYS? How do you see it?
> +
> config MFD_ROHM_BD718XX
> tristate "ROHM BD71837 Power Management IC"
> depends on I2C=y
> @@ -2215,6 +2219,7 @@ config MFD_ROHM_BD718XX
> select REGMAP_I2C
> select REGMAP_IRQ
> select MFD_CORE
> + select MFD_ROHM_PWRBUTTON
> help
> Select this option to get support for the ROHM BD71837
> Power Management ICs. BD71837 is designed to power processors like
> @@ -2228,6 +2233,7 @@ config MFD_ROHM_BD71828
> select REGMAP_I2C
> select REGMAP_IRQ
> select MFD_CORE
> + select MFD_ROHM_PWRBUTTON
> help
> Select this option to get support for the ROHM BD71815, BD71828,
> BD71879, BD72720 and BD73900 Power Management ICs (PMICs). These are
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index dd4bb7e77c33..72d3944b0ad8 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -273,6 +273,7 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
> obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> +obj-$(CONFIG_MFD_ROHM_PWRBUTTON) += rohm-pwrbutton.o
> obj-$(CONFIG_MFD_ROHM_BD71828) += rohm-bd71828.o
> obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
> obj-$(CONFIG_MFD_ROHM_BD957XMUF) += rohm-bd9576.o
> diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> index 5fb6142cf087..dfc23cd13ef2 100644
> --- a/drivers/mfd/rohm-bd71828.c
> +++ b/drivers/mfd/rohm-bd71828.c
> @@ -5,8 +5,6 @@
> * ROHM BD718[15/28/79] and BD72720 PMIC driver
> */
>
> -#include <linux/device/devres.h>
> -#include <linux/gfp_types.h>
> #include <linux/i2c.h>
> #include <linux/input.h>
> #include <linux/interrupt.h>
> @@ -19,10 +17,11 @@
> #include <linux/mfd/rohm-generic.h>
> #include <linux/module.h>
> #include <linux/of.h>
> -#include <linux/property.h>
> #include <linux/regmap.h>
> #include <linux/types.h>
>
> +#include "rohm-pwrbutton.h"
> +
> #define BD72720_TYPED_IRQ_REG(_irq, _stat_offset, _mask, _type_offset) \
> [_irq] = { \
> .reg_offset = (_stat_offset), \
> @@ -859,83 +858,7 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap,
> OUT32K_MODE_CMOS);
> }
>
> -static const struct property_entry bd71828_powerkey_parent_props[] = {
> - PROPERTY_ENTRY_STRING("label", "bd71828-pwrkey"),
> - { }
> -};
> -
> -static const struct property_entry bd71828_powerkey_props[] = {
> - PROPERTY_ENTRY_U32("linux,code", KEY_POWER),
> - PROPERTY_ENTRY_BOOL("wakeup-source"),
> - { }
> -};
> -
> -#define GPIO_KEYS 0 /* Node corresponding to gpio-keys device itself */
> -#define PWRON_KEY 1 /* Node describing power button in gpio-keys */
> -
> -static int bd71828_i2c_register_swnodes(const struct software_node *nodes)
> -{
> - const struct software_node * const node_group[] = {
> - &nodes[GPIO_KEYS], &nodes[PWRON_KEY], NULL
> - };
> -
> - return software_node_register_node_group(node_group);
> -}
> -
> -static void bd71828_i2c_unregister_swnodes(void *data)
> -{
> - const struct software_node *nodes = data;
> - const struct software_node * const node_group[] = {
> - &nodes[GPIO_KEYS], &nodes[PWRON_KEY], NULL
> - };
> -
> - software_node_unregister_node_group(node_group);
> -}
> -
> -static int bd71828_i2c_register_pwrbutton(struct device *dev, int button_irq,
> - struct irq_domain *irq_domain)
> -{
> - const struct resource res[] = {
> - DEFINE_RES_IRQ_NAMED(button_irq, "bd71828-pwrkey"),
> - };
> - struct mfd_cell gpio_keys_cell = {
> - .name = "gpio-keys",
> - .resources = res,
> - .num_resources = ARRAY_SIZE(res),
> - };
> - struct software_node *nodes;
> - int ret;
>
> - nodes = devm_kcalloc(dev, 2, sizeof(*nodes), GFP_KERNEL);
> - if (!nodes)
> - return -ENOMEM;
> -
> - nodes[GPIO_KEYS].name = devm_kasprintf(dev, GFP_KERNEL, "%s-power-key", dev_name(dev));
> - if (!nodes[GPIO_KEYS].name)
> - return -ENOMEM;
> -
> - nodes[GPIO_KEYS].properties = bd71828_powerkey_parent_props;
> -
> - nodes[PWRON_KEY].parent = &nodes[GPIO_KEYS];
> - nodes[PWRON_KEY].properties = bd71828_powerkey_props;
> -
> - ret = bd71828_i2c_register_swnodes(nodes);
> - if (ret)
> - return ret;
> -
> - ret = devm_add_action_or_reset(dev, bd71828_i2c_unregister_swnodes, nodes);
> - if (ret)
> - return ret;
> -
> - gpio_keys_cell.swnode = &nodes[GPIO_KEYS];
> -
> - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, &gpio_keys_cell, 1,
> - NULL, 0, irq_domain);
> - if (ret)
> - return dev_err_probe(dev, ret, "Failed to register power-button");
> -
> - return 0;
> -}
>
> static struct i2c_client *bd71828_dev;
> static void bd71828_power_off(void)
> @@ -1096,7 +1019,8 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
> return dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>
> if (button_irq) {
> - ret = bd71828_i2c_register_pwrbutton(&i2c->dev, button_irq, irq_domain);
> + ret = rohm_mfd_register_pwrbutton(&i2c->dev, button_irq,
> + "bd71828-pwrkey", true, irq_domain);
> if (ret)
> return ret;
> }
> diff --git a/drivers/mfd/rohm-bd718x7.c b/drivers/mfd/rohm-bd718x7.c
> index be2acc429fe3..bab3b1c2445b 100644
> --- a/drivers/mfd/rohm-bd718x7.c
> +++ b/drivers/mfd/rohm-bd718x7.c
> @@ -7,8 +7,6 @@
> // Datasheet for BD71837MWV available from
> // https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
>
> -#include <linux/device/devres.h>
> -#include <linux/gfp_types.h>
> #include <linux/i2c.h>
> #include <linux/input.h>
> #include <linux/interrupt.h>
> @@ -16,10 +14,11 @@
> #include <linux/mfd/core.h>
> #include <linux/module.h>
> #include <linux/of.h>
> -#include <linux/property.h>
> #include <linux/regmap.h>
> #include <linux/types.h>
>
> +#include "rohm-pwrbutton.h"
> +
> static struct mfd_cell bd71837_mfd_cells[] = {
> { .name = "bd71837-clk", },
> { .name = "bd71837-pmic", },
> @@ -105,83 +104,7 @@ static int bd718xx_init_press_duration(struct regmap *regmap,
> return 0;
> }
>
> -static const struct property_entry bd718xx_powerkey_parent_props[] = {
> - PROPERTY_ENTRY_STRING("label", "bd718xx-pwrkey"),
> - { }
> -};
> -
> -static const struct property_entry bd718xx_powerkey_props[] = {
> - PROPERTY_ENTRY_U32("linux,code", KEY_POWER),
> - { }
> -};
> -
> -static const struct resource bd718xx_powerkey_resources[] = {
> - DEFINE_RES_IRQ_NAMED(BD718XX_INT_PWRBTN_S, "bd718xx-pwrkey"),
> -};
> -
> -#define GPIO_KEYS 0 /* Node corresponding to gpio-keys device itself */
> -#define PWRON_KEY 1 /* Node describing power button in gpio-keys */
> -
> -static int bd718xx_i2c_register_swnodes(const struct software_node *nodes)
> -{
> - const struct software_node * const node_group[] = {
> - &nodes[GPIO_KEYS], &nodes[PWRON_KEY], NULL
> - };
> -
> - return software_node_register_node_group(node_group);
> -}
> -
> -static void bd718xx_i2c_unregister_swnodes(void *data)
> -{
> - const struct software_node *nodes = data;
> - const struct software_node * const node_group[] = {
> - &nodes[GPIO_KEYS], &nodes[PWRON_KEY], NULL
> - };
> -
> - software_node_unregister_node_group(node_group);
> -}
>
> -static int bd718xx_i2c_register_pwrbutton(struct device *dev,
> - struct irq_domain *irq_domain)
> -{
> - struct mfd_cell gpio_keys_cell = {
> - .name = "gpio-keys",
> - .resources = bd718xx_powerkey_resources,
> - .num_resources = ARRAY_SIZE(bd718xx_powerkey_resources),
> - };
> - struct software_node *nodes;
> - int ret;
> -
> - nodes = devm_kcalloc(dev, 2, sizeof(*nodes), GFP_KERNEL);
> - if (!nodes)
> - return -ENOMEM;
> -
> - nodes[GPIO_KEYS].name = devm_kasprintf(dev, GFP_KERNEL, "%s-power-key", dev_name(dev));
> - if (!nodes[GPIO_KEYS].name)
> - return -ENOMEM;
> -
> - nodes[GPIO_KEYS].properties = bd718xx_powerkey_parent_props;
> -
> - nodes[PWRON_KEY].parent = &nodes[GPIO_KEYS];
> - nodes[PWRON_KEY].properties = bd718xx_powerkey_props;
> -
> - ret = bd718xx_i2c_register_swnodes(nodes);
> - if (ret)
> - return ret;
> -
> - ret = devm_add_action_or_reset(dev, bd718xx_i2c_unregister_swnodes, nodes);
> - if (ret)
> - return ret;
> -
> - gpio_keys_cell.swnode = &nodes[GPIO_KEYS];
> -
> - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, &gpio_keys_cell, 1,
> - NULL, 0, irq_domain);
> - if (ret)
> - return dev_err_probe(dev, ret, "Failed to register power-button");
> -
> - return 0;
> -}
>
> static int bd718xx_i2c_probe(struct i2c_client *i2c)
> {
> @@ -235,7 +158,8 @@ static int bd718xx_i2c_probe(struct i2c_client *i2c)
> if (ret)
> return dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>
> - ret = bd718xx_i2c_register_pwrbutton(&i2c->dev, irq_domain);
> + ret = rohm_mfd_register_pwrbutton(&i2c->dev, BD718XX_INT_PWRBTN_S,
> + "bd718xx-pwrkey", false, irq_domain);
> if (ret)
> return ret;
>
> diff --git a/drivers/mfd/rohm-pwrbutton.c b/drivers/mfd/rohm-pwrbutton.c
> new file mode 100644
> index 000000000000..e0cb74fdd73c
> --- /dev/null
> +++ b/drivers/mfd/rohm-pwrbutton.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Shared helper for ROHM PMIC power button registration
> + *
> + * Copyright 2018, 2019 ROHM Semiconductors
> + * Copyright 2026 Google LLC
> + */
> +
> +#include <linux/device/devres.h>
> +#include <linux/gfp_types.h>
> +#include <linux/input.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +#include "rohm-pwrbutton.h"
> +
> +#define GPIO_KEYS 0 /* Node corresponding to gpio-keys device itself */
> +#define PWRON_KEY 1 /* Node describing power button in gpio-keys */
> +
> +static int rohm_mfd_pwrbutton_register_swnodes(const struct software_node *nodes)
> +{
> + const struct software_node * const node_group[] = {
> + &nodes[GPIO_KEYS], &nodes[PWRON_KEY], NULL
> + };
> +
> + return software_node_register_node_group(node_group);
> +}
> +
> +static void rohm_mfd_pwrbutton_unregister_swnodes(void *data)
> +{
> + const struct software_node *nodes = data;
> + const struct software_node * const node_group[] = {
> + &nodes[GPIO_KEYS], &nodes[PWRON_KEY], NULL
> + };
> +
> + software_node_unregister_node_group(node_group);
> +}
> +
> +int rohm_mfd_register_pwrbutton(struct device *dev, int irq, const char *name,
> + bool wakeup, struct irq_domain *irq_domain)
> +{
> + const struct resource res[] = {
> + DEFINE_RES_IRQ_NAMED(irq, name),
> + };
> + struct mfd_cell gpio_keys_cell = {
> + .name = "gpio-keys",
> + .resources = res,
> + .num_resources = ARRAY_SIZE(res),
> + };
> + struct property_entry *parent_props;
> + struct property_entry *child_props;
> + struct software_node *nodes;
> + int n_props;
> + int ret;
> +
> + if (irq <= 0)
> + return -EINVAL;
> +
> + nodes = devm_kcalloc(dev, 2, sizeof(*nodes), GFP_KERNEL);
> + if (!nodes)
> + return -ENOMEM;
> +
> + nodes[GPIO_KEYS].name = devm_kasprintf(dev, GFP_KERNEL, "%s-power-key", dev_name(dev));
> + if (!nodes[GPIO_KEYS].name)
> + return -ENOMEM;
> +
> + parent_props = devm_kcalloc(dev, 2, sizeof(*parent_props), GFP_KERNEL);
> + if (!parent_props)
> + return -ENOMEM;
> +
> + parent_props[0] = PROPERTY_ENTRY_STRING("label", name);
> + nodes[GPIO_KEYS].properties = parent_props;
> +
> + n_props = 2; /* linux,code and terminator */
> + if (wakeup)
> + n_props++;
> +
> + child_props = devm_kcalloc(dev, n_props, sizeof(*child_props), GFP_KERNEL);
> + if (!child_props)
> + return -ENOMEM;
> +
> + child_props[0] = PROPERTY_ENTRY_U32("linux,code", KEY_POWER);
> + if (wakeup)
> + child_props[1] = PROPERTY_ENTRY_BOOL("wakeup-source");
> +
> + nodes[PWRON_KEY].parent = &nodes[GPIO_KEYS];
> + nodes[PWRON_KEY].properties = child_props;
> +
> + ret = rohm_mfd_pwrbutton_register_swnodes(nodes);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev, rohm_mfd_pwrbutton_unregister_swnodes, nodes);
> + if (ret)
> + return ret;
> +
> + gpio_keys_cell.swnode = &nodes[GPIO_KEYS];
> +
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, &gpio_keys_cell, 1,
> + NULL, 0, irq_domain);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register power-button");
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(rohm_mfd_register_pwrbutton);
As explained, I would split this to something like:
pwrbutton_create_swnodes() // Shouldn't depend on MFD. Allocates and
registers the swnodes for power-button + registers the devm-release.
(rohm)_register_pwrbutton() // Should call pwrbutton_create_swnodes() to
allocate and populate the swnodes, and then fill-in the MFC-cell data +
call the devm_mfd_add_devices().
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Dmitry Torokhov <dmitry.torokhov@gmail.com>");
> +MODULE_DESCRIPTION("Shared helper for ROHM PMIC power button registration");
> diff --git a/drivers/mfd/rohm-pwrbutton.h b/drivers/mfd/rohm-pwrbutton.h
> new file mode 100644
> index 000000000000..985bd37a4ad0
> --- /dev/null
> +++ b/drivers/mfd/rohm-pwrbutton.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __LINUX_MFD_ROHM_PWRBUTTON_H__
> +#define __LINUX_MFD_ROHM_PWRBUTTON_H__
> +
> +struct device;
> +struct irq_domain;
> +
> +int rohm_mfd_register_pwrbutton(struct device *dev, int irq, const char *name,
> + bool wakeup, struct irq_domain *irq_domain);
> +
> +#endif /* __LINUX_MFD_ROHM_PWRBUTTON_H__ */
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
next prev parent reply other threads:[~2026-06-25 8:00 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-18 18:58 Dmitry Torokhov
2026-06-19 9:02 ` Lee Jones
2026-06-25 8:00 ` Matti Vaittinen [this message]
2026-06-25 18:33 ` Dmitry Torokhov
2026-06-26 7:35 ` Matti Vaittinen
2026-07-06 22:27 ` Dmitry Torokhov
2026-07-12 16:11 ` Matti Vaittinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a867d076-55cf-4686-a46f-6cb9fd198b90@gmail.com \
--to=mazziesaccount@gmail.com \
--cc=dmitry.torokhov@gmail.com \
--cc=lee@kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome