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

[Lee Jones <lee@kernel.org>]

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