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 | 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
+ };
+
+ error = software_node_register_node_group(node_group);
+ 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 +223,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.53.0.1018.g2bb0e51243-googOn 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@gmail.com>
> ---
> 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! ~~
On Fri, Mar 27, 2026 at 12:55:55PM +0200, Matti Vaittinen wrote:
> On 25/03/2026 02:54, Dmitry Torokhov wrote:
> > +
> > + 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.
This is simply a compound literal, part of the C standard since C99. It
allows skip explicitly declaring the dimensions of the node_group[]
array (which is "far" away from where we initialize it and it
potentially may get out of sync).
We have quite a few in the kernel, DEFINE_RES_IRQ() and others for
example are compound literals under the hood.
Thanks.
--
Dmitry
On 27/03/2026 18:10, Dmitry Torokhov wrote:
> On Fri, Mar 27, 2026 at 12:55:55PM +0200, Matti Vaittinen wrote:
>> On 25/03/2026 02:54, Dmitry Torokhov wrote:
>>> +
>>> + 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.
>
> This is simply a compound literal, part of the C standard since C99. It
> allows skip explicitly declaring the dimensions of the node_group[]
> array (which is "far" away from where we initialize it and it
> potentially may get out of sync).
>
> We have quite a few in the kernel, DEFINE_RES_IRQ() and others for
> example are compound literals under the hood.
Yes. But ones I've seen have been wrapped in macros. I don't think I've
seen open-coded one written directly to a call-site (although that's
what the macros end up). Hence, this is not something I see typically
when reading drivers.
But as I said, if this is fine with Lee, I can live with this too :)
--
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Wed, 01 Apr 2026, Matti Vaittinen wrote:
> On 27/03/2026 18:10, Dmitry Torokhov wrote:
> > On Fri, Mar 27, 2026 at 12:55:55PM +0200, Matti Vaittinen wrote:
> > > On 25/03/2026 02:54, Dmitry Torokhov wrote:
> > > > +
> > > > + 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.
> >
> > This is simply a compound literal, part of the C standard since C99. It
> > allows skip explicitly declaring the dimensions of the node_group[]
> > array (which is "far" away from where we initialize it and it
> > potentially may get out of sync).
> >
> > We have quite a few in the kernel, DEFINE_RES_IRQ() and others for
> > example are compound literals under the hood.
>
> Yes. But ones I've seen have been wrapped in macros. I don't think I've seen
> open-coded one written directly to a call-site (although that's what the
> macros end up). Hence, this is not something I see typically when reading
> drivers.
>
> But as I said, if this is fine with Lee, I can live with this too :)
My personal preference is to avoid the use of compound literals for
structs inside functions.
--
Lee Jones [李琼斯]
On Wed, Apr 01, 2026 at 11:59:30AM +0100, Lee Jones wrote:
> On Wed, 01 Apr 2026, Matti Vaittinen wrote:
>
> > On 27/03/2026 18:10, Dmitry Torokhov wrote:
> > > On Fri, Mar 27, 2026 at 12:55:55PM +0200, Matti Vaittinen wrote:
> > > > On 25/03/2026 02:54, Dmitry Torokhov wrote:
> > > > > +
> > > > > + 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.
> > >
> > > This is simply a compound literal, part of the C standard since C99. It
> > > allows skip explicitly declaring the dimensions of the node_group[]
> > > array (which is "far" away from where we initialize it and it
> > > potentially may get out of sync).
> > >
> > > We have quite a few in the kernel, DEFINE_RES_IRQ() and others for
> > > example are compound literals under the hood.
> >
> > Yes. But ones I've seen have been wrapped in macros. I don't think I've seen
> > open-coded one written directly to a call-site (although that's what the
> > macros end up). Hence, this is not something I see typically when reading
> > drivers.
> >
> > But as I said, if this is fine with Lee, I can live with this too :)
>
> My personal preference is to avoid the use of compound literals for
> structs inside functions.
>
I assume use of macros like DEFINE_RES_IRQ() is still OK?
Thanks.
--
Dmitry
© 2016 - 2026 Red Hat, Inc.