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

[PATCH v2 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.

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/

Signed-off-by: Dmitry Torokhov <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 | 117 ++++++++++++++++++++++++++++++++-------------
 drivers/mfd/rohm-bd718x7.c | 117 ++++++++++++++++++++++++++++++++-------------
 2 files changed, 168 insertions(+), 66 deletions(-)
---
base-commit: 7109a2155340cc7b21f27e832ece6df03592f2e8
change-id: 20260313-rohm-software-nodes-0b4a3d36128c

Thanks.

-- 
Dmitry

[PATCH v2 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 | 117 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 85 insertions(+), 32 deletions(-)

diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
index a79f354bf5cb..20b7910e7f63 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,75 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap,
 				  OUT32K_MODE_CMOS);
 }
 
+static void bd71828_i2c_unregister_swnodes(void *data)
+{
+	const struct software_node *nodes = data;
+
+	software_node_unregister_node_group((const struct software_node *[]){
+		&nodes[0],
+		&nodes[1],
+		NULL
+	});
+}
+
+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"),
+		{ }
+	};
+	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 = software_node_register_node_group((const struct software_node *[]){
+		&nodes[0],
+		&nodes[1],
+		NULL
+	});
+	if (error)
+		return error;
+
+	error = devm_add_action_or_reset(dev, bd71828_i2c_unregister_swnodes, nodes);
+	if (error)
+		return error;
+
+	const struct mfd_cell gpio_keys_cell = {
+		.name = "gpio-keys",
+		.resources = (const struct resource[]) {
+			DEFINE_RES_IRQ_NAMED(button_irq, "bd71828-pwrkey"),
+		},
+		.num_resources = 1,
+		.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 +981,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;
@@ -1008,6 +1061,8 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
 	dev_dbg(&i2c->dev, "Registered %d IRQs for chip\n",
 		irqchip->num_irqs);
 
+	irq_domain = regmap_irq_get_domain(irq_data);
+
 	/*
 	 * On some ICs the main IRQ register has corresponding mask register.
 	 * This is not handled by the regmap IRQ. Let's enable all the main
@@ -1022,23 +1077,21 @@ 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;
 
 	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.53.0.959.g497ff81fa9-goog

[PATCH v2 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.
---
 drivers/mfd/rohm-bd718x7.c | 117 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 83 insertions(+), 34 deletions(-)

diff --git a/drivers/mfd/rohm-bd718x7.c b/drivers/mfd/rohm-bd718x7.c
index ff714fd4f54d..6f677be9a5d7 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,81 @@ static int bd718xx_init_press_duration(struct regmap *regmap,
 	return 0;
 }
 
+static void bd718xx_i2c_unregister_swnodes(void *data)
+{
+	const struct software_node *nodes = data;
+
+	software_node_unregister_node_group((const struct software_node *[]){
+		&nodes[0],
+		&nodes[1],
+		NULL
+	});
+}
+
+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),
+		{ }
+	};
+	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 = software_node_register_node_group((const struct software_node *[]){
+		&nodes[0],
+		&nodes[1],
+		NULL
+	});
+	if (error)
+		return error;
+
+	error = devm_add_action_or_reset(dev, bd718xx_i2c_unregister_swnodes,
+					 nodes);
+	if (error)
+		return error;
+
+	struct mfd_cell gpio_keys_cell = {
+		.name = "gpio-keys",
+		.resources = (const struct resource[]){
+			DEFINE_RES_IRQ_NAMED(BD718XX_INT_PWRBTN_S, "bd718xx-pwrkey"),
+		},
+		.num_resources = 1,
+		.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;
@@ -165,24 +216,22 @@ static int bd718xx_i2c_probe(struct i2c_client *i2c)
 	if (ret)
 		return dev_err_probe(&i2c->dev, ret, "Failed to add irq_chip\n");
 
+	irq_domain = regmap_irq_get_domain(irq_data);
+
 	ret = bd718xx_init_press_duration(regmap, &i2c->dev);
 	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;
-
 	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.53.0.959.g497ff81fa9-goog

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

[Matti Vaittinen]

On 23/03/2026 03:37, 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>

Thanks a lot Dmitry for converting this to the swnodes. I like the idea 
very much :) A few minor, (mostly styling related as I am a bit 
old-fashioned) comments.

> ---
>   drivers/mfd/rohm-bd71828.c | 117 ++++++++++++++++++++++++++++++++-------------
>   1 file changed, 85 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> index a79f354bf5cb..20b7910e7f63 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,75 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap,
>   				  OUT32K_MODE_CMOS);
>   }
>   
> +static void bd71828_i2c_unregister_swnodes(void *data)
> +{
> +	const struct software_node *nodes = data;
> +
> +	software_node_unregister_node_group((const struct software_node *[]){
> +		&nodes[0],
> +		&nodes[1],
> +		NULL
> +	});

Perhaps it was possible to use a temporary variable for the 
software_node pointer array? It would allow also old-school fellows like 
me to pick the meaning by a glance :)

> +}
> +
> +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"),
> +		{ }
> +	};
> +	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 = software_node_register_node_group((const struct software_node *[]){
> +		&nodes[0],
> +		&nodes[1],
> +		NULL
> +	});

I think having a temporary variable might make this to look more familiar.

> +	if (error)
> +		return error;
> +
> +	error = devm_add_action_or_reset(dev, bd71828_i2c_unregister_swnodes, nodes);
> +	if (error)
> +		return error;
> +
> +	const struct mfd_cell gpio_keys_cell = {
> +		.name = "gpio-keys",
> +		.resources = (const struct resource[]) {
> +			DEFINE_RES_IRQ_NAMED(button_irq, "bd71828-pwrkey"),
> +		},
> +		.num_resources = 1,
> +		.swnode = &nodes[0],
> +	};

I don't really love seeing variables declared in the middle of a block. 
Perhaps consider splitting the software-node preparation in own function 
and doing something like (completely untested thought):

ret = alloc_and_prepare_the_swnode_stuff(...);
if (!ret) {
	const struct software_node *nodes[] = { ... };
	const struct mfd_cell gpio_keys_cell = { ... };
	
	...
}

or alternatively, split the software-node and MFD device registration 
into own function? Do you think it would work?

> +	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 +981,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;
> @@ -1008,6 +1061,8 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
>   	dev_dbg(&i2c->dev, "Registered %d IRQs for chip\n",
>   		irqchip->num_irqs);
>   
> +	irq_domain = regmap_irq_get_domain(irq_data);

nit: Maybe move this call closer to where the irq_domain is actually 
needed for the first time.

> +
>   	/*
>   	 * On some ICs the main IRQ register has corresponding mask register.
>   	 * This is not handled by the regmap IRQ. Let's enable all the main
> @@ -1022,23 +1077,21 @@ 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;
>   
>   	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) {
> 


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

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

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

[Matti Vaittinen]

On 23/03/2026 03:37, 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.

I think we miss Signed-off-by -line here. Other than that, similar 
comments as I had for the 1/2.


Yours,
-- Matti

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

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

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

[Dmitry Torokhov]

On Tue, Mar 24, 2026 at 08:45:15AM +0200, Matti Vaittinen wrote:
> On 23/03/2026 03:37, 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>
> 
> Thanks a lot Dmitry for converting this to the swnodes. I like the idea very
> much :) A few minor, (mostly styling related as I am a bit old-fashioned)
> comments.
> 
> > ---
> >   drivers/mfd/rohm-bd71828.c | 117 ++++++++++++++++++++++++++++++++-------------
> >   1 file changed, 85 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> > index a79f354bf5cb..20b7910e7f63 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,75 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap,
> >   				  OUT32K_MODE_CMOS);
> >   }
> > +static void bd71828_i2c_unregister_swnodes(void *data)
> > +{
> > +	const struct software_node *nodes = data;
> > +
> > +	software_node_unregister_node_group((const struct software_node *[]){
> > +		&nodes[0],
> > +		&nodes[1],
> > +		NULL
> > +	});
> 
> Perhaps it was possible to use a temporary variable for the software_node
> pointer array? It would allow also old-school fellows like me to pick the
> meaning by a glance :)

Done.

> 
> > +}
> > +
> > +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"),
> > +		{ }
> > +	};
> > +	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 = software_node_register_node_group((const struct software_node *[]){
> > +		&nodes[0],
> > +		&nodes[1],
> > +		NULL
> > +	});
> 
> I think having a temporary variable might make this to look more familiar.

Done.

> 
> > +	if (error)
> > +		return error;
> > +
> > +	error = devm_add_action_or_reset(dev, bd71828_i2c_unregister_swnodes, nodes);
> > +	if (error)
> > +		return error;
> > +
> > +	const struct mfd_cell gpio_keys_cell = {
> > +		.name = "gpio-keys",
> > +		.resources = (const struct resource[]) {
> > +			DEFINE_RES_IRQ_NAMED(button_irq, "bd71828-pwrkey"),
> > +		},
> > +		.num_resources = 1,
> > +		.swnode = &nodes[0],
> > +	};
> 
> I don't really love seeing variables declared in the middle of a block.

I normally try not to declare in the middle of the code but in this case
I opted for it because swnode is not known so we have to split
initialization. 

> Perhaps consider splitting the software-node preparation in own function and
> doing something like (completely untested thought):
> 
> ret = alloc_and_prepare_the_swnode_stuff(...);
> if (!ret) {
> 	const struct software_node *nodes[] = { ... };
> 	const struct mfd_cell gpio_keys_cell = { ... };
> 	
> 	...
> }

That is an option but then spreads dealing with nodes across multiple
functions. I moved the declaration and partial initialization to the
beginning of the function, and assign swnode before calling
devm_mfd_add_devices().

> 
> or alternatively, split the software-node and MFD device registration into
> own function? Do you think it would work?
> 
> > +	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 +981,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;
> > @@ -1008,6 +1061,8 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
> >   	dev_dbg(&i2c->dev, "Registered %d IRQs for chip\n",
> >   		irqchip->num_irqs);
> > +	irq_domain = regmap_irq_get_domain(irq_data);
> 
> nit: Maybe move this call closer to where the irq_domain is actually needed
> for the first time.

Done.

Thanks.

-- 
Dmitry