From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-dy1-f174.google.com (mail-dy1-f174.google.com [74.125.82.174]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B0A543BF66E for ; Thu, 14 May 2026 20:57:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.82.174 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778792258; cv=none; b=nJwz3HZxDdLWZcf/798tJ/pXnnhjpPqsq38J4Vlt3Hj7WfmLQYW2+LZEiK9rToZR6SgyEwBiISQjV4zqs/0+nEMu0zsB10sLmjqHnBwibYgVOPcVYi84rJcyUTQ8tpy9Z7yNqQrqpt025xMnR6oyyzBWPp6SWW89/by6Ln9V7QY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778792258; c=relaxed/simple; bh=DOU45JalOxCkunm7IfJ3OQo6xIS0uGv+ja/x62aQNbQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=BI70TJBxcgA90nfSxH7cq6rFu5srRPNr7NprFue7B/+jEAHfUoldQnechBUzvnWs+ByzEqm7RwZXYyGOe5FoU0I+NINSPf34+jMmkM451rjKxzbKlxQSmv3Uj5e514O/+rntk4WLu6Z1TSnZyw24pyJcN4cpfnXNGtinFvuBN+Q= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=YqiI1EPC; arc=none smtp.client-ip=74.125.82.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="YqiI1EPC" Received: by mail-dy1-f174.google.com with SMTP id 5a478bee46e88-2f33ae12f97so4442223eec.1 for ; Thu, 14 May 2026 13:57:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1778792256; x=1779397056; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=gQVNtuKWxzwmISnCn6b6Jfq3RS/c/dVc+AIvviUb0HI=; b=YqiI1EPCvRg7XkzrPjXD8sFDIlJNSmi4swBDXHaO720zeWFisdgMWYzqE7Bl7qYoii vBkFJ4jgBADSLxEF7WgnY4N/SmlPV4RvbZNGafxFUXVq31I+G9Tue3RDg6rokt9ofNqd OpK3FARm6dgAeVPTyRXqDmt0nzcISCkL0cUS8s6k6gIcdInU5TRH9S3B1sPrPHLHMrZI LKQpBlO1CvECZ+IdvDz3wg0KtY77Fi+m24qWhBg9HJ70UappxW/D2/4XkRV9nuc9A1sZ l+jZkg4n2Vl7DOrz2vJCx1pTGe8Om6EUQuWnU581cdhWhzu50u+FRoO+GqodeR/UWd0r aaIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778792256; x=1779397056; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=gQVNtuKWxzwmISnCn6b6Jfq3RS/c/dVc+AIvviUb0HI=; b=Vu3oHcQxdFNIKb380WIv6DflncrZ3FiQD7Qrs8r5s80ZxWbN0S0Vm+ju2HHX8Filld dbb5fHP8lq6hmeXq0cECU/XHQAkdNNURFyBn2bhSQe1q/dNCDeKJcuh0yDIe2MuBTWd1 bQltRxiForiQJKAuMssz9MKbl26bNY/+Y3p7h1LU6zk4Theg5X2bp/g5x4S2ID7ug3ik apOJDtEXgfjM0DtnH413rg7dDMeyLcuewNRJS7WPLix/1XihPLfXQEKZc9GCPI1lz5ut YFqcQf8irBsYIzLDMPYR5e3hZKdWzy+4wtgpN3GYJzGQzAtEuOIhvGXB4IdONd+7YM+X JyZQ== X-Forwarded-Encrypted: i=1; AFNElJ8ESg4uk1u1q0sIp7QeaCUkOiOWHCU9YrZJjDEC1I6lwS+Jktf0YbaCWCUJfTmmZJ0CeyX9VAb91c51tkA=@vger.kernel.org X-Gm-Message-State: AOJu0YzfzcHKFgPmOlq9cYfZcOaGAKfBjfdYW1bmqZIBgrn861NhIshU 9gscYozxJix34E/B392J3HHgfsCZKPwsjauqnSQhJ7lgGCaDgQFzaMPo X-Gm-Gg: Acq92OGEJOWlj9/X5PsA9E21cc+s4nLFy0uCfm+npP6dcDLjWWyQjNdxc2XKhdhs9Ce rSTOGGJr2C7ejg1Ou5IxOBZH/JM/hPcXrSDPJ/w9h4Hwrnh6spfIVHYPeBtsijoybFnbbcBiN+Q Kd76BGZAOgGnYaYEIfaL4lLdGevc6znIQXvTPJFtp1VInC4nfLzGOyodiySKNKL0g0ezRIl8Wjh A82OsWp7TU+JnWZczrMtfh6QauYrzrYUUyE5H3Z731mPcrKbFdtxxjLGahF+cMsfVbm6alrblHH 9gcjJuyJpSPwXlhFsnye1yACJdUGOFocZlTBcbCAiNt0lLSUPgqvpfC9/KnIWV5axWwfPkcsCpS iDINq4Sc2JE1nnkjm5FPfUqxkbel141PdEMzyFu22rev1yI389Q6wvSwBJxdbbEOol0e55sJliW UEhRtOZAGVDLWgJiblNYUHEC/96OtgNKiy+PezbV+f4F5xO7Ciofnu+S8yAN6hWHFF X-Received: by 2002:a05:7301:688:b0:2ed:e16:6b4a with SMTP id 5a478bee46e88-3039869b963mr721004eec.32.1778792255549; Thu, 14 May 2026 13:57:35 -0700 (PDT) Received: from google.com ([2a00:79e0:2ebe:8:8210:f61b:62be:85eb]) by smtp.gmail.com with ESMTPSA id 5a478bee46e88-302944ffdf0sm4614252eec.8.2026.05.14.13.57.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 May 2026 13:57:34 -0700 (PDT) Date: Thu, 14 May 2026 13:57:31 -0700 From: Dmitry Torokhov To: Lee Jones 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: References: <20260427-rohm-software-nodes-v4-0-ffeb5b0c4774@gmail.com> <20260427-rohm-software-nodes-v4-1-ffeb5b0c4774@gmail.com> <20260507144247.GQ305027@google.com> <20260514160010.GP305027@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: <20260514160010.GP305027@google.com> On Thu, May 14, 2026 at 05:00:10PM +0100, Lee Jones wrote: > 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. I am all ears as to how you want it be presented. > > > > > + 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. This is a per-device data. Some if it can be shared but some can not. If you want to use module-globals then we need to introduce locking to make sure we are not probing multiple devices at once and have them stomp on each other. Is the new lock really worth it so that we can move variable declarations out of the function scope? Thanks. -- Dmitry