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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Lee,

On Thu, May 21, 2026 at 01:20:46PM +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 */
> >  };
> >  
> >  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
> 
> Tell us what these nodes represent - defines are nicer that indexes for readers:
> 
> #define GPIO_KEYS  0
> #define PWRON_KEY  1
> 
> This also eradicates for the need for the comments during allocation.

OK, however here (in both register and especially unregister) we do not
really care what nodes represent, we just want to register (or
unregister) all of them.

I could also write:

#define BD71828_NUM_SWNODES	2

	struct software_node * const node_group[BD71828_NUM_SWNODES + 1] = {};

	for (int i = 0; i < BD71828_NUM_SWNODES; i++)
		node_group[i] = &nodes[i];
	...

but for just 2 nodes it is a bit of overkill.

> 
> > +	};
> > +
> > +	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"),
> > +		{ }
> > +	};
> 
> Break these 'static consts' out just under the 'static const struct resource's.

OK.

> 
> > +	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),
> > +	};
> 
> Could we keep this 'mfd_cell' structure as 'static const'?

Hmm... there are 2 variable items: the button irq and the software node
associated with the device. While it is possible to create 2 separate
resource structures/arrays to represent variants with different IRQs,
the per-device software nodes still require individual, non constant,
mfd_cells to instantiate the gpio-keys device(s).

Unless you want to go to singleton model and only allow instantiating 1
device it is not really possible to have static const mfd_cell for the
keys.

> We should aim to
> avoid local copies for dynamic amendments unless it is absolutely unavoidable.

Why though? Being per-device and used from probe() they can not be
marked __initconst and discarded, so having temporaries on stack seems
fine to me?

> 
> > +	struct software_node *nodes;
> > +	int error;
> 
> Nit: This is almost always 'ret'.

OK.

> 
> Please be consistent with the reset of the subsystem.
> 
> % git grep "int error" | wc -l
> 4402
> % git grep "int err" | wc -l
> 37211
> % git grep "int ret" | wc -l
> 83075
> 
> > +
> > +	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];
> 
> Nit: Blank line between these unrelated blocks.

OK. I was trying to co-locate final mfd cell structure adjustment with
its use in registration.

> 
> > +	error = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, &gpio_keys_cell, 1,
> > +				     NULL, 0, irq_domain);
	

Thanks.

-- 
Dmitry