* Re: [PATCH v4 1/2] mfd: rohm-bd71828: Use software nodes for gpio-keys
2026-05-07 14:42 ` [PATCH v4 1/2] mfd: rohm-bd71828: Use software nodes for gpio-keys Lee Jones
@ 2026-05-07 15:05 ` Dmitry Torokhov
0 siblings, 0 replies; 2+ messages in thread
From: Dmitry Torokhov @ 2026-05-07 15:05 UTC (permalink / raw)
To: Lee Jones; +Cc: Matti Vaittinen, Arnd Bergmann, linux-kernel
On Thu, May 07, 2026 at 03:42:47PM +0100, Lee Jones wrote:
> On Mon, 27 Apr 2026, Dmitry Torokhov wrote:
>
> > Refactor the rohm-bd71828 MFD driver to use software nodes for
> > instantiating the gpio-keys child device, replacing the old
> > platform_data mechanism.
> >
> > The power key's properties are now defined using software nodes and
> > property entries. The IRQ is passed as a resource attached to the
> > platform device.
> >
> > This will allow dropping support for using platform data for configuring
> > gpio-keys in the future.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/mfd/rohm-bd71828.c | 122 +++++++++++++++++++++++++++++++++------------
> > 1 file changed, 90 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> > index a79f354bf5cb..a8bdb9c955a4 100644
> > --- a/drivers/mfd/rohm-bd71828.c
> > +++ b/drivers/mfd/rohm-bd71828.c
> > @@ -5,7 +5,8 @@
> > * ROHM BD718[15/28/79] and BD72720 PMIC driver
> > */
> >
> > -#include <linux/gpio_keys.h>
> > +#include <linux/device/devres.h>
> > +#include <linux/gfp_types.h>
> > #include <linux/i2c.h>
> > #include <linux/input.h>
> > #include <linux/interrupt.h>
> > @@ -18,6 +19,7 @@
> > #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>
> >
> > @@ -37,19 +39,6 @@
> > }, \
> > }
> >
> > -static struct gpio_keys_button button = {
> > - .code = KEY_POWER,
> > - .gpio = -1,
> > - .type = EV_KEY,
> > - .wakeup = 1,
> > -};
> > -
> > -static const struct gpio_keys_platform_data bd71828_powerkey_data = {
> > - .buttons = &button,
> > - .nbuttons = 1,
> > - .name = "bd71828-pwrkey",
> > -};
> > -
> > static const struct resource bd71815_rtc_irqs[] = {
> > DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC0, "bd70528-rtc-alm-0"),
> > DEFINE_RES_IRQ_NAMED(BD71815_INT_RTC1, "bd70528-rtc-alm-1"),
> > @@ -174,11 +163,8 @@ static struct mfd_cell bd71828_mfd_cells[] = {
> > .name = "bd71828-rtc",
> > .resources = bd71828_rtc_irqs,
> > .num_resources = ARRAY_SIZE(bd71828_rtc_irqs),
> > - }, {
> > - .name = "gpio-keys",
> > - .platform_data = &bd71828_powerkey_data,
> > - .pdata_size = sizeof(bd71828_powerkey_data),
> > },
> > + /* Power button is registered separately */
>
> This happens a lot in MFD - no need to call it out.
OK.
>
> > };
> >
> > static const struct resource bd72720_power_irqs[] = {
> > @@ -242,11 +228,8 @@ static const struct mfd_cell bd72720_mfd_cells[] = {
> > .name = "bd72720-rtc",
> > .resources = bd72720_rtc_irqs,
> > .num_resources = ARRAY_SIZE(bd72720_rtc_irqs),
> > - }, {
> > - .name = "gpio-keys",
> > - .platform_data = &bd71828_powerkey_data,
> > - .pdata_size = sizeof(bd71828_powerkey_data),
> > },
> > + /* Power button is registered separately */
> > };
> >
> > static const struct regmap_range bd71815_volatile_ranges[] = {
> > @@ -877,6 +860,80 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap,
> > OUT32K_MODE_CMOS);
> > }
> >
> > +static int bd71828_i2c_register_swnodes(const struct software_node *nodes)
> > +{
> > + const struct software_node * const node_group[] = {
> > + &nodes[0], &nodes[1], NULL
> > + };
>
> I only see handling like this in the testing infra.
>
> This is all very opaque and fiddly.
>
> Are you sure we can't do better with statically declared arrays?
The nodes represent per-device data, so they can't be static/shared if
we want to continue using the non-singleton approach in the driver.
>
> > + 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[0], &nodes[1], 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)
> > +{
> > + 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"),
> > + { }
> > + };
> > + 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),
> > + };
> > +
> +
> +Please break all 3 of these out of function context.
> +
> +We nearly always declare these externally unless they contain dynamic
> +values and even then we try and avoid it.
"button_irq" is not a constant, so we need to have "res[]" and therefore
gpio_keys_cell as locals. I can move out the properties, but I believe
there is a value in grouping them together.
> +
> > + struct software_node *nodes;
> > + int error;
> > +
> > + nodes = devm_kcalloc(dev, 2, sizeof(*nodes), GFP_KERNEL);
> > + if (!nodes)
> > + return -ENOMEM;
> > +
> > + /* Node corresponding to gpio-keys device itself */
> > + nodes[0].name = devm_kasprintf(dev, GFP_KERNEL, "%s-power-key", dev_name(dev));
> > + if (!nodes[0].name)
> > + return -ENOMEM;
> > +
> > + nodes[0].properties = bd71828_powerkey_parent_props;
> > +
> > + /* Node representing power button within gpio-keys device */
> > + nodes[1].parent = &nodes[0];
> > + nodes[1].properties = bd71828_powerkey_props;
> > +
> > + error = bd71828_i2c_register_swnodes(nodes);
> > + if (error)
> > + return error;
> > +
> > + error = devm_add_action_or_reset(dev, bd71828_i2c_unregister_swnodes, nodes);
> > + if (error)
> > + return error;
> > +
> > + gpio_keys_cell.swnode = &nodes[0];
> > + error = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, &gpio_keys_cell, 1,
> > + NULL, 0, irq_domain);
> > + if (error)
> > + return dev_err_probe(dev, error, "Failed to create power button subdevice");
>
> "Failed to register power-button"
OK.
>
> > +
> > + return 0;
> > +}
> > +
> > static struct i2c_client *bd71828_dev;
> > static void bd71828_power_off(void)
> > {
> > @@ -929,6 +986,7 @@ static struct regmap *bd72720_do_regmaps(struct i2c_client *i2c)
> > static int bd71828_i2c_probe(struct i2c_client *i2c)
> > {
> > struct regmap_irq_chip_data *irq_data;
> > + struct irq_domain *irq_domain;
> > int ret;
> > struct regmap *regmap = NULL;
> > const struct regmap_config *regmap_config;
> > @@ -1022,23 +1080,23 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
> > "Failed to enable main level IRQs\n");
> > }
> > }
> > - if (button_irq) {
> > - ret = regmap_irq_get_virq(irq_data, button_irq);
> > - if (ret < 0)
> > - return dev_err_probe(&i2c->dev, ret,
> > - "Failed to get the power-key IRQ\n");
> > -
> > - button.irq = ret;
> > - }
> >
> > ret = set_clk_mode(&i2c->dev, regmap, clkmode_reg);
> > if (ret)
> > return ret;
> >
> > + irq_domain = regmap_irq_get_domain(irq_data);
>
> This looks like an unrelated change?
It is not. Both devm_mfd_add_devices() and
bd71828_i2c_register_pwrbutton() use irq_domain argument o it makes
sense to have a temporary here. Making a separate preparatory patch
introducing a temporary just for one function call does not make sense:
each patch has to make sense on its own.
>
> > + ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd,
> > cells, - NULL, 0,
> > regmap_irq_get_domain(irq_data)); + NULL,
> > 0, irq_domain); if (ret) - return dev_err_probe(&i2c->dev,
> > ret, "Failed to create subdevices\n");
>
> I can close my eyes to this one!
>
> > + 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); +
> > if (ret) + return ret; + }
> >
> > if (of_device_is_system_power_controller(i2c->dev.of_node) &&
> > chip_type == ROHM_CHIP_TYPE_BD71828) {
> >
> > -- 2.54.0.545.g6539524ca2-goog
> >
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 2+ messages in thread