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

[Lee Jones]

On Wed, 27 May 2026, Dmitry Torokhov wrote:

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

Purely for code readability.

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

Agreed.  Just a couple of defines that describe the indexes would be a
good addition IMHO.

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

Okay, drop this suggestion, I must have missed that it was being edited.

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

I think the code this was referring to has been snipped.

Please submit your next version.

-- 
Lee Jones