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 F208C3C8C75 for ; Thu, 21 May 2026 12:20:50 +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=1779366052; cv=none; b=DbHW97l7MT1rOTqK67pBbggX5tFFCxObebVoGUQLXY/wrmz6rzTneYC4XnzT92jlF9X75p3ZG1FHeH+eVYvAoXz/isTuih1njVfLtJcnvk5Ya2Aob14xcFtlXdWYqEt/RqHoKw5w6y7FGIpJkvE7F6yqKlXo1WAjlw0XlHoDANs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779366052; c=relaxed/simple; bh=8L3GaTM6ou17OO6thGjWnhB3eJHoh7TfRMRIZchX+Bc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ediuNYPaLgQIJC7Fv45fkt/6ZKuISwFQ40hb7NLv8STEZZV9yufi64bIqVfROein0r2b5WmT0e243KJATlqYby8BCvcYEsKmhKq+4Ts05qBpuM7scUz8SLD3N6H7EfgA4IlUhPZRhOOybEfXZ9GRX/0/0tWQwzqemlVgy1u+Tfk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=SikY7GbY; 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="SikY7GbY" Received: by smtp.kernel.org (Postfix) with ESMTPSA id B77841F000E9; Thu, 21 May 2026 12:20:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1779366050; bh=B4/3n7ec9O8S62L0iNCT5lPeWb70GPBQgJvGqCprANY=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=SikY7GbY940prfDSg6QkOBOdJa0SagtfVdGKMBmruEg+u9L8N24AfBaGgo9Z8DU96 YAyJffgqG7P1nrXDBRjGF0qjbxxKpVTR49qXVFK+NX6tRZcCFh1xVz6EmUGnWk7zjZ sVRFYZ2N3NQgfhap+bN34qvsscJT4C6b5xbaeTkLU6XfVr6VxYc9kdTlwyiq9yQgAY /yFITqjhbo1X5uNfogyOSXihjGdYHSgTdOb7z9GK4UY995fzI0ezsFyleFEDIVsL8j TT5KlJOhig4kjuowvC7FaD+1oczHjZQeo3ahg6sUw4Xrr648tygG2p95ImOQa93Hme ol0/uii4qJWYQ== Date: Thu, 21 May 2026 13:20:46 +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: <20260521122046.GF2921053@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 */ > }; > > 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. > + }; > + > + 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. > + 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'? We should aim to avoid local copies for dynamic amendments unless it is absolutely unavoidable. > + struct software_node *nodes; > + int error; Nit: This is almost always 'ret'. Please be consistent with the reset of the subsystem. % git grep "int error" | wc -l 4402 % git grep "int err" | wc -l 37211 % git grep "int ret" | wc -l 83075 > + > + 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]; Nit: Blank line between these unrelated blocks. > + 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 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); > + > 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"); > + 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