From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 42FE238D404 for ; Thu, 11 Jun 2026 08:53:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781168005; cv=none; b=QqYyhF52Ja2U2JN8TsZqaA3v4npK97zroTSpBGDwIpbWOPd+1ETMPPqanrFNvPSCRPYz+RVRdWTFOQFdK0z8Pc0M5NJXi9RKZ5GKTT7hKFFWLCmPYwyaPq/8IMysT1fT8fVNhtmRt43zi9s96DrxMJsvF+rtIGaEvltCbybqV4Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781168005; c=relaxed/simple; bh=mw4pY+pum5CZPA+k1zry2U5Z53+dpa8SdZRuEil5V8c=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=rg8TmJCEltkUnvmOgKUNKf5ekHUt0zUIPMkkG++nTYQ84VDj+I4tMP21yuhnjAAK+K7TS0MLhBvjfuGDBNm9KW0+pXphg1/OEI2gVjXe+06d9KuvZhej2WjE8TU4ff3XvBEaWSUWwCcoTqzihMWUeR1sX4FFCcls87v3fFww1T0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=dx4z6YCQ; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="dx4z6YCQ" Received: by smtp.kernel.org (Postfix) with ESMTPSA id DFBB71F00893; Thu, 11 Jun 2026 08:53:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1781168003; bh=E+qcgIjzv5puCzdfQyhF+Ukk/ij4QuThJx8KpbGlfaw=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=dx4z6YCQQyA+CrpuF04gHfgJUDakjYerfWdTd4Nrn9E7LzFCIMDTI6bdnJuOhwill XuZ2o4xknwl3KO2VKLCC8IYAmSlQxyukfWx+n+FEsUWs7ATfhQj4LCgUSmwFyDCRNG C3iRaeAcLaLrxF8ye0yrazOrsphY51hKHfxEHRud4gfy/GwHxt3okJpgCK44mRqwIJ bCFeZj+AVZDoSJehhQ6PdaLlQH2encVD7g2ECeyoB2+3KIg8kdhIXEWll/WyMPKf+r c4OHozjsKSeaUKnrOWW0k3wt4wbtz+u+oET/kQsoCazl5dIIQoeFk+qmso0s7UId1P /XZ79oUBcvIcg== Date: Thu, 11 Jun 2026 09:53:20 +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: <20260611085320.GM4151951@google.com> References: <20260427-rohm-software-nodes-v4-0-ffeb5b0c4774@gmail.com> <20260427-rohm-software-nodes-v4-1-ffeb5b0c4774@gmail.com> <20260521122046.GF2921053@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 Wed, 27 May 2026, Dmitry Torokhov wrote: > Hi Lee, > > On Thu, May 21, 2026 at 01:20:46PM +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 */ > > > }; > > > > > > 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 > > > > Tell us what these nodes represent - defines are nicer that indexes for readers: > > > > #define GPIO_KEYS 0 > > #define PWRON_KEY 1 > > > > This also eradicates for the need for the comments during allocation. > > OK, however here (in both register and especially unregister) we do not > really care what nodes represent, we just want to register (or > unregister) all of them. Purely for code readability. > I could also write: > > #define BD71828_NUM_SWNODES 2 > > struct software_node * const node_group[BD71828_NUM_SWNODES + 1] = {}; > > for (int i = 0; i < BD71828_NUM_SWNODES; i++) > node_group[i] = &nodes[i]; > ... > > but for just 2 nodes it is a bit of overkill. Agreed. Just a couple of defines that describe the indexes would be a good addition IMHO. > > > > > + }; > > > + > > > + 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"), > > > + { } > > > + }; > > > > Break these 'static consts' out just under the 'static const struct resource's. > > OK. > > > > > > + 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), > > > + }; > > > > Could we keep this 'mfd_cell' structure as 'static const'? > > Hmm... there are 2 variable items: the button irq and the software node > associated with the device. While it is possible to create 2 separate > resource structures/arrays to represent variants with different IRQs, > the per-device software nodes still require individual, non constant, > mfd_cells to instantiate the gpio-keys device(s). > > Unless you want to go to singleton model and only allow instantiating 1 > device it is not really possible to have static const mfd_cell for the > keys. Okay, drop this suggestion, I must have missed that it was being edited. > > We should aim to > > avoid local copies for dynamic amendments unless it is absolutely unavoidable. > > Why though? Being per-device and used from probe() they can not be > marked __initconst and discarded, so having temporaries on stack seems > fine to me? I think the code this was referring to has been snipped. Please submit your next version. -- Lee Jones