[PATCH v4 0/2] rohm-bdi718x7/71828: Use software nodes for gpio-keys

[PATCH v4 0/2] rohm-bdi718x7/71828: Use software nodes for gpio-keys

[Dmitry Torokhov]

Now that gpio-keys can use platform resources to identify interrupts
assigned to buttons we can convert ROHM power buttons to use software
nodes and device properties for configuration, removing the need to use
platform data.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Changes in v4:
- avoid using compound literals in assignments
- Link to v3: https://patch.msgid.link/20260324-rohm-software-nodes-v3-0-edde5a0324d5@gmail.com

Changes in v3:
- Stopped mixing code and variable declarations, use more temps
- Moved assignment to irq_domain closer to where is is being used
- Added missing SOB
- Link to v2: https://patch.msgid.link/20260322-rohm-software-nodes-v2-0-3c7d21336d37@gmail.com

v2:
- dropped patch to gpio-keys as it is in the mainline now
- reworked the both drivers to dynamically allocate per-device software
  nodes

v1:
https://lore.kernel.org/r/20250817224731.1911207-1-dmitry.torokhov@gmail.com/

---
Dmitry Torokhov (2):
      mfd: rohm-bd71828: Use software nodes for gpio-keys
      mfd: rohm-bd718x7: Use software nodes for gpio-keys

 drivers/mfd/rohm-bd71828.c | 122 +++++++++++++++++++++++++++++++++------------
 drivers/mfd/rohm-bd718x7.c | 120 ++++++++++++++++++++++++++++++++------------
 2 files changed, 177 insertions(+), 65 deletions(-)
---
base-commit: 39704f00f747aba3144289870b5fd8ac230a9aaf
change-id: 20260313-rohm-software-nodes-0b4a3d36128c

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

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
+	};
+
+	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),
+	};
+	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");
+
+	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);
+
 	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");
+		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

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

[Dmitry Torokhov]

Refactor the rohm-bd7182x7 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-bd718x7.c | 120 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 87 insertions(+), 33 deletions(-)

diff --git a/drivers/mfd/rohm-bd718x7.c b/drivers/mfd/rohm-bd718x7.c
index ff714fd4f54d..4d8de19e5c34 100644
--- a/drivers/mfd/rohm-bd718x7.c
+++ b/drivers/mfd/rohm-bd718x7.c
@@ -7,7 +7,8 @@
 // Datasheet for BD71837MWV available from
 // https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
 
-#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>
@@ -15,37 +16,16 @@
 #include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/types.h>
 
-static struct gpio_keys_button button = {
-	.code = KEY_POWER,
-	.gpio = -1,
-	.type = EV_KEY,
-};
-
-static struct gpio_keys_platform_data bd718xx_powerkey_data = {
-	.buttons = &button,
-	.nbuttons = 1,
-	.name = "bd718xx-pwrkey",
-};
-
 static struct mfd_cell bd71837_mfd_cells[] = {
-	{
-		.name = "gpio-keys",
-		.platform_data = &bd718xx_powerkey_data,
-		.pdata_size = sizeof(bd718xx_powerkey_data),
-	},
 	{ .name = "bd71837-clk", },
 	{ .name = "bd71837-pmic", },
 };
 
 static struct mfd_cell bd71847_mfd_cells[] = {
-	{
-		.name = "gpio-keys",
-		.platform_data = &bd718xx_powerkey_data,
-		.pdata_size = sizeof(bd718xx_powerkey_data),
-	},
 	{ .name = "bd71847-clk", },
 	{ .name = "bd71847-pmic", },
 };
@@ -125,10 +105,86 @@ static int bd718xx_init_press_duration(struct regmap *regmap,
 	return 0;
 }
 
+static int bd718xx_i2c_register_swnodes(const struct software_node *nodes)
+{
+	const struct software_node * const node_group[] = {
+		&nodes[0], &nodes[1], NULL
+	};
+
+	return software_node_register_node_group(node_group);
+}
+
+static void bd718xx_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 bd718xx_i2c_register_pwrbutton(struct device *dev,
+					  struct irq_domain *irq_domain)
+{
+	static const struct property_entry bd718xx_powerkey_parent_props[] = {
+		PROPERTY_ENTRY_STRING("label", "bd718xx-pwrkey"),
+		{ }
+	};
+	static const struct property_entry bd718xx_powerkey_props[] = {
+		PROPERTY_ENTRY_U32("linux,code", KEY_POWER),
+		{ }
+	};
+	const struct resource res[] = {
+		DEFINE_RES_IRQ_NAMED(BD718XX_INT_PWRBTN_S, "bd718xx-pwrkey"),
+	};
+	struct mfd_cell gpio_keys_cell = {
+		.name = "gpio-keys",
+		.resources = res,
+		.num_resources = ARRAY_SIZE(res),
+	};
+	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 = bd718xx_powerkey_parent_props;
+
+	/* Node representing power button within gpio-keys device */
+	nodes[1].parent = &nodes[0];
+	nodes[1].properties = bd718xx_powerkey_props;
+
+	error = bd718xx_i2c_register_swnodes(nodes);
+	if (error)
+		return error;
+
+	error = devm_add_action_or_reset(dev, bd718xx_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");
+
+	return 0;
+}
+
 static int bd718xx_i2c_probe(struct i2c_client *i2c)
 {
 	struct regmap *regmap;
 	struct regmap_irq_chip_data *irq_data;
+	struct irq_domain *irq_domain;
 	int ret;
 	unsigned int chip_type;
 	struct mfd_cell *mfd;
@@ -169,20 +225,18 @@ static int bd718xx_i2c_probe(struct i2c_client *i2c)
 	if (ret)
 		return ret;
 
-	ret = regmap_irq_get_virq(irq_data, BD718XX_INT_PWRBTN_S);
-
-	if (ret < 0)
-		return dev_err_probe(&i2c->dev, ret, "Failed to get the IRQ\n");
-
-	button.irq = ret;
+	irq_domain = regmap_irq_get_domain(irq_data);
 
 	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
-				   mfd, cells, NULL, 0,
-				   regmap_irq_get_domain(irq_data));
+				   mfd, cells, NULL, 0, irq_domain);
 	if (ret)
-		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
+		return dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
 
-	return ret;
+	ret = bd718xx_i2c_register_pwrbutton(&i2c->dev, irq_domain);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 static const struct of_device_id bd718xx_of_match[] = {

-- 
2.54.0.545.g6539524ca2-goog

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

[Matti Vaittinen]

Hi Dee Ho,

Thanks a ton Dmitry! This is looking very good to me now. I only have 
one question below.

On 28/04/2026 07:13, 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

//snip

> +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),
> +	};
> +	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;

Do we have any guidance/rules for naming the swnodes similar to 
devicetree nodes? Do they need to be unique, and are they used for anything?

I am wondering if the dev_name() is needed or if we should have some 
'numbering'? I am not sure if the node names can be used for anything, 
but in some cases adding IC-type to names will hurt the "generic 
usability". On the other hand, if names need to be unique, then some 
numbering might be needed (although, this is not critical for this 
driver as it is very unlikely there is a system with more than one of 
these PMICs).

Huh. I feel the "generic usability" is not too accurately said... As one 
example, I have added IC-type in IRQ names declared in MFD driver. 
Later, when same RTC driver was used to support multiple ICs, getting 
IRQ resources in RTC had to be done separately for each IC-variant just 
because the IRQ names contained the IC-type. No idea if there is any use 
for swnode names, so I don't know if this makes sense for them.

This is a 'nit'. I am fine with a simple reply that the naming is not a 
concern here - just thought that perhaps it matters :)

With that clarified:
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

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

[Matti Vaittinen]

On 29/04/2026 08:53, Matti Vaittinen wrote:
> Hi Dee Ho,
> 
> Thanks a ton Dmitry! This is looking very good to me now. I only have 
> one question below.
> 
> On 28/04/2026 07:13, 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
>> +    /* 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;
> 
> Do we have any guidance/rules for naming the swnodes similar to 
> devicetree nodes? Do they need to be unique, and are they used for 
> anything?
> 
> I am wondering if the dev_name() is needed or if we should have some 
> 'numbering'? I am not sure if the node names can be used for anything, 
> but in some cases adding IC-type to names will hurt the "generic 
> usability". On the other hand, if names need to be unique, then some 
> numbering might be needed (although, this is not critical for this 
> driver as it is very unlikely there is a system with more than one of 
> these PMICs).

After thinking 2 seconds more... I suppose the dev_name() guarantees the 
name is unique :) So, please forget my babblings about numbering.


-- 
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

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

[Matti Vaittinen]

Thanks!

On 28/04/2026 07:13, Dmitry Torokhov wrote:
> Refactor the rohm-bd7182x7 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>

If swnode naming is ok [see my mail(s) for 1/2]:
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

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

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

[Lee Jones]

On Thu, 14 May 2026, Dmitry Torokhov wrote:

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

Let me go back to the original patch and make slightly different
recommendations.

-- 
Lee Jones

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 */
>  };
>  
>  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.

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

> +	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'? We should aim to
avoid local copies for dynamic amendments unless it is absolutely unavoidable.

> +	struct software_node *nodes;
> +	int error;

Nit: This is almost always 'ret'.

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.

> +	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");
> +
> +	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);
> +
>  	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");
> +		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]

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