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 0122131AA92 for ; Thu, 14 May 2026 16:00:15 +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=1778774415; cv=none; b=DWK6YGFAVslP+Qelggtxv9XYfB2ZlL03ZBwJ1E9A/BEl2VX5RJrXlD2fVCw64B54FDfwL6I2xmWqIIfv6zkcr5eg0vdZmrwt4n06xYN3DbXSbYozuz/TMKZ51byQ7vukXKxyTvga3hRqIUdmpoqFMfWLzKwLtMzuH2F+L+zZUx8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778774415; c=relaxed/simple; bh=AOLoDDnu5QPzW0tF34boL6QmHKP/zYAw5ddJsCTBwqg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=OpfuRwRMLisT0rYCZ8xh+bPdrMQPQWpOk62HUwuAsKNUvSkV9X/gz41sPD/hQsbFQF4VgD5Nh9reo8sJvAKtwlnkShXqkyIBpCr+wI0Z3Ama4ErkomEB7XI1MkuV5BaWkdESsN5oKCA4A/ViCcY+PAcvfdWwFdc7NJQGeHW12jI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=pbFThbe/; 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="pbFThbe/" Received: by smtp.kernel.org (Postfix) with ESMTPSA id D6FFFC2BCB7; Thu, 14 May 2026 16:00:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1778774414; bh=AOLoDDnu5QPzW0tF34boL6QmHKP/zYAw5ddJsCTBwqg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=pbFThbe/Yr74V71sYc3pjgoYbbH55tWxTS36ul806ufdDOU2TaMQAZpTIZTtIXw5T kfJJ+OdqpZpitC6vx32GIVrCAOoKslT/AaIgxFmr/CT1y9eVg2hxogpELL7euLo5GS Z3MAM2xC6z8e2wAaab+ObMJUDutWHBRqs3iFbXK/DCLvHVqzTwoKU5mNAl+Ht/HPiP Xjhmjtg41HnVbYtFL71r2Rnv41uZLicu6uVVG3biFYtwuL2xZTjTnDHlZXDqXJvTa0 O1FvC0DuDKC0BJ1tstkr7hTc03F+fUThBnP6njUnILxBBp/aoocXnqTFqptYXtnHhT hDCfRALkJJ9Vw== Date: Thu, 14 May 2026 17:00:10 +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: <20260514160010.GP305027@google.com> References: <20260427-rohm-software-nodes-v4-0-ffeb5b0c4774@gmail.com> <20260427-rohm-software-nodes-v4-1-ffeb5b0c4774@gmail.com> <20260507144247.GQ305027@google.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: On Thu, 07 May 2026, Dmitry Torokhov wrote: > On Thu, May 07, 2026 at 03:42:47PM +0100, Lee Jones wrote: > > 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. > > OK. > > > > > > }; > > > > > > 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? > > The nodes represent per-device data, so they can't be static/shared if > we want to continue using the non-singleton approach in the driver. If the handling has to stay the same, then we need to at least present it in a nicer way. Manually playing with nameless array indexes looks super hacky. > > > + 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. > > "button_irq" is not a constant, so we need to have "res[]" and therefore > gpio_keys_cell as locals. I can move out the properties, but I believe > there is a value in grouping them together. They don't need to be locals, they can be !const and you can fill them in. Again, this is common practice. -- Lee Jones