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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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! ~~