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

[Matti Vaittinen]

On 25/03/2026 02:54, 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@xxxxxxxxx>
> ---
>   drivers/mfd/rohm-bd718x7.c | 118 ++++++++++++++++++++++++++++++++-------------
>   1 file changed, 85 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/mfd/rohm-bd718x7.c b/drivers/mfd/rohm-bd718x7.c
> index ff714fd4f54d..39961177a17f 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,84 @@ 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;
> +	const struct software_node *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),
> +	};
> +	const struct software_node * const *node_group;
> +	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;
> +
> +	node_group = (const struct software_node *[]){
> +		&nodes[0],
> +		&nodes[1],
> +		NULL
> +	};

Hmm. I suppose I was not explaining myself well. When I asked for a 
temporary variable, I was hoping to get rid of this syntax. Something like:
	const struct software_node *node_group[3];

        node_group[0] = &nodes[0];
        node_group[1] = &nodes[1];
        node_group[2] = NULL;

would look more familiar to me. Well, I suppose I can live with this if 
it is Ok to Lee though. Let's see if he has an opinion.

Other than this the change looks very good to me! Thanks.

Yours,
-- Matti

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

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