From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E37C73F166D for ; Thu, 7 May 2026 14:42:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778164972; cv=none; b=pIO3Rji2zqASeK2HeJvZ2eITI80kFaQ+g7h6lGVL68qdSdNLXXSypPPe8SVH3aOqn00u0j623TK2KqX1Lp8BwXeF7WDGCZyXJ2L+KIn+napLG/YFqenenLyWhlV2EoOkpM6J1VK9d/ygRX+gUaNe/RCWVt+htxc3KPjNwB60e6k= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778164972; c=relaxed/simple; bh=0OOwJQlnvfruAiuZEBYmdoXFdJwyjwfZWdaBixJDTN4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=XY1uPHUIibn92ICvAtHXAkBy90/ZaH+jQhcCUoS0aojI19IZEhMMpqTyTwxS0upXUKepRinTkYrg7X7CrI/asyLQvrqIdEQn4nCMdLq+J5Kfr643FgZHEcu74fEx2P2CDBWQ6eQccuO+WVNkvk8y6B2zDdkfnco/rkJhVj4qaFY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=jaEoS5GE; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="jaEoS5GE" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A959DC2BCC9; Thu, 7 May 2026 14:42:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1778164971; bh=0OOwJQlnvfruAiuZEBYmdoXFdJwyjwfZWdaBixJDTN4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=jaEoS5GEcmhy61l8IuOE9ruVqMAflWXE6UXXItRUTtCSqRwP0j87DI592b5lRSGq7 tkmqziAoNxrhsXyRxZVYFROXaSot7Ye2yJ6t5MFd7ak5Mk3S5GivFstYMkBt+6Lrz6 Nrm1kjDmKuFgBqYCYL8JaFu08rByu3OE/GDMDVxp2csbewxoKhFb4MkeZvqZL14Gh4 HqmV34RIAc2H4oOY5tg0GTvdoz3Zm4O/z4bdpcHS+i49rDPyE+nm+LUdkAvqPQHIFa jnklJvrU6/sJTuIRWG6u7LOu8Ame3GrGyrTh3BaICsYQ1ZG9orvhDpDNOP7DU/1df6 9K5F4/JHezmkA== Date: Thu, 7 May 2026 15:42:47 +0100 From: Lee Jones To: Dmitry Torokhov Cc: Matti Vaittinen , Arnd Bergmann , linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 1/2] mfd: rohm-bd71828: Use software nodes for gpio-keys Message-ID: <20260507144247.GQ305027@google.com> References: <20260427-rohm-software-nodes-v4-0-ffeb5b0c4774@gmail.com> <20260427-rohm-software-nodes-v4-1-ffeb5b0c4774@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260427-rohm-software-nodes-v4-1-ffeb5b0c4774@gmail.com> 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 > --- > 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 > +#include > +#include > #include > #include > #include > @@ -18,6 +19,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -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