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

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

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