Re: [PATCH v4 1/2] mfd: rohm-bd71828: Use software nodes for gpio-keys

Re: [PATCH v4 1/2] mfd: rohm-bd71828: Use software nodes for gpio-keys

[Lee Jones]

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.

>  };
>  
>  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?

> +	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.
+
> +	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"

> +
> +	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?

> +
>  	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
> 

-- 
Lee Jones

Re: [PATCH v4 1/2] mfd: rohm-bd71828: Use software nodes for gpio-keys

[Dmitry Torokhov]

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

Re: [PATCH v4 1/2] mfd: rohm-bd71828: Use software nodes for gpio-keys

[Lee Jones]

On Thu, 07 May 2026, Dmitry Torokhov wrote:

> 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.

If the handling has to stay the same, then we need to at least present
it in a nicer way.  Manually playing with nameless array indexes looks
super hacky.

> > > +	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.

They don't need to be locals, they can be !const and you can fill them in.

Again, this is common practice.

-- 
Lee Jones

Re: [PATCH v4 1/2] mfd: rohm-bd71828: Use software nodes for gpio-keys

[Dmitry Torokhov]

On Thu, May 14, 2026 at 05:00:10PM +0100, Lee Jones wrote:
> On Thu, 07 May 2026, Dmitry Torokhov wrote:
> 
> > 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.
> 
> If the handling has to stay the same, then we need to at least present
> it in a nicer way.  Manually playing with nameless array indexes looks
> super hacky.

I am all ears as to how you want it be presented.

> 
> > > > +	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.
> 
> They don't need to be locals, they can be !const and you can fill them in.
> 
> Again, this is common practice.

This is a per-device data. Some if it can be shared but some can not. If
you want to use module-globals then we need to introduce locking to make
sure we are not probing multiple devices at once and have them stomp on
each other.

Is the new lock really worth it so that we can move variable
declarations out of the function scope?

Thanks.

-- 
Dmitry