From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f42.google.com (mail-lf1-f42.google.com [209.85.167.42]) (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 7C3423BD638 for ; Tue, 24 Mar 2026 06:45:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774334728; cv=none; b=VlybdqK05/X0JL8vH0nyfifvNJodXS7kX+bxvexa33OeHatmk2RpJm0h6YGUfTFSp0s2sUTVe2Ao1azWoSJnCE0CweXfem5EQs+ZlgIqtixyaaVWwAaBZ+B26WdmO/lcELbUavgMPeLl8QP2WJgMlgPpGjrta0y+UPDRlJVsVdE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774334728; c=relaxed/simple; bh=Ojn6dVfyKfstwYmcJmSSZClMCMWjrHsqa1LwfOjeSDY=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=im9bIRvk8avwuuZKR7ImcvpE/q2yDU8VGit8iIgvrB4uR+e+anz+lO2mG0GzHU844hYZDVaGZuxxk1oSebYcDBYjYROeqYOiA6AuNjAH/0WDq3s8JXARGklkncMoaGFExA80Ocd6Mh7i2THC3EJzKFqLTGOksL4Zn9roftHrrtw= 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=f8vsrrxg; arc=none smtp.client-ip=209.85.167.42 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="f8vsrrxg" Received: by mail-lf1-f42.google.com with SMTP id 2adb3069b0e04-5a27bfadca9so6000684e87.3 for ; Mon, 23 Mar 2026 23:45:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1774334717; x=1774939517; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=s/RZtI7qzK9OVSW3KKAviyLf5Tj6jUu1h6pcHF8nU2E=; b=f8vsrrxgYMW0EBYSuT13UDSCLKS520tdfJzOGM3sNOo1eiX9Xli0O1lAH7vA1DcqNW t/1cEnrAc/N8rD4xP+r2pAg0TsY4mYhGmwU0KF+AmXxb2HS6mBDR/OL8tZLp+ivDU+Yf ho5vBp83dto71+L5BMWju9+X5nHubMhE6RMXSCVQLzVk3FPJIPhKMCXWD327YMuMoY50 lUaYHlLMrJmS+HevJVJ+DGqLqHeVjPJ1dcJeQNaSeZjD+nw60FsBmnXK5nXIrFfMq3qf DTihJXi9KFkvNVh/K6vKk7jIMP3xCe0U4vQ2YeRCd8Ah3L3r+96SgDzDxRTqoONczxpV krow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774334717; x=1774939517; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=s/RZtI7qzK9OVSW3KKAviyLf5Tj6jUu1h6pcHF8nU2E=; b=eMtpZWBZyZnSLUCHw5c+hTg9OKynzZjz5K0rN6wfQflJuLX5CB8LSmejLqUpP9Vms3 Wi0EkfKr8OmGQ5QLWFAYZYcvelqHG9hCw7RA9w6sUPNM3wWV6915N4EhdYZuVVHM0yhO MWIQSsMErr1ESG1ISSaDthb19BY4Cf7eewh/9p8yzfTYZbAAvk9xJBvy9eSRIbOo7NuQ n76gYgEBVBSNT5JMlm7IZDc0FFkntZQHbVzt4Z5rl4YPBdJ+KU+ibKNvz3EI3pEvSBNM bhQujTmatnx2kcmBWlhOzQjVQYneYcCPPjKjFxREuYLXZUwf493Zil0li2k+Nfob8o/V M+qQ== X-Gm-Message-State: AOJu0YxHA1NCwe8z9oddHKcHn93QHJJWRiHy33ZzMC9mhayWpgW7kHyn 584OpAjgK2ajZF8mdGtjseAj8tSQNqcj03mI7Zxx4djh26XZJNy3lyuV X-Gm-Gg: ATEYQzwYUr2/mxlr6/SGDzkZCWVqJf4EZxIlABqVSEbwG6nl3M6hvl21ELP8V800BsA LuRS8IFpvZfkUtqJjgB2xAhatnOrYJypgsXese1OEB/Go4L80Wl0IR/7HTdd+qqf2WSPbvUisfv UEqm68k9BW9+1W1HBObrqXJnfhK01TFySV9dM+Ut0X1Canzvfddc4ZOaIlRjLwWvhcYiR+G+zy3 wyoLdVsoIO3tMen+XXPgG1pOI6Ma/AjvBGvCKl83/Q9Sia0cc+1TrBvlwSrEtWH/mvV/JmvAgy1 3PCK3l4BkLpTLwqLIptB9fabTMQIryXXJ3jKyOYvjEUtWjEBIaFFSLaTRJnHVDnj4q2byj1rbjV JgsYN5rMlrgi6/5ozo6d4W1zd+7A5/rrM76jKU06jQg+lDehFBtC3pGMdldiRj/yKtxVvKLSdJC 8Y0I0JBTEXHCcq5DYHYPSI5Mukah/00duFCrKrjI1Kql+e+zgQQf3j4Wrprpw0UR9+LxUJb9CRh Y4jmbZJ X-Received: by 2002:a05:6512:3e28:b0:5a2:961a:f7c7 with SMTP id 2adb3069b0e04-5a2961af86emr811573e87.21.1774334717010; Mon, 23 Mar 2026 23:45:17 -0700 (PDT) Received: from ?IPV6:2a10:a5c0:800d:dd00:8fdf:935a:2c85:d703? ([2a10:a5c0:800d:dd00:8fdf:935a:2c85:d703]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-5a2851a113csm2982964e87.23.2026.03.23.23.45.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 23 Mar 2026 23:45:16 -0700 (PDT) Message-ID: <80feeb8f-faad-44f5-827a-def3952037f8@gmail.com> Date: Tue, 24 Mar 2026 08:45:15 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/2] mfd: rohm-bd71828: Use software nodes for gpio-keys To: Dmitry Torokhov , Lee Jones Cc: linux-kernel@vger.kernel.org References: <20260322-rohm-software-nodes-v2-0-3c7d21336d37@gmail.com> <20260322-rohm-software-nodes-v2-1-3c7d21336d37@gmail.com> Content-Language: en-US, en-AU, en-GB, en-BW From: Matti Vaittinen In-Reply-To: <20260322-rohm-software-nodes-v2-1-3c7d21336d37@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 23/03/2026 03:37, 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 Thanks a lot Dmitry for converting this to the swnodes. I like the idea very much :) A few minor, (mostly styling related as I am a bit old-fashioned) comments. > --- > drivers/mfd/rohm-bd71828.c | 117 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 85 insertions(+), 32 deletions(-) > > diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c > index a79f354bf5cb..20b7910e7f63 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,75 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap, > OUT32K_MODE_CMOS); > } > > +static void bd71828_i2c_unregister_swnodes(void *data) > +{ > + const struct software_node *nodes = data; > + > + software_node_unregister_node_group((const struct software_node *[]){ > + &nodes[0], > + &nodes[1], > + NULL > + }); Perhaps it was possible to use a temporary variable for the software_node pointer array? It would allow also old-school fellows like me to pick the meaning by a glance :) > +} > + > +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"), > + { } > + }; > + 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 = software_node_register_node_group((const struct software_node *[]){ > + &nodes[0], > + &nodes[1], > + NULL > + }); I think having a temporary variable might make this to look more familiar. > + if (error) > + return error; > + > + error = devm_add_action_or_reset(dev, bd71828_i2c_unregister_swnodes, nodes); > + if (error) > + return error; > + > + const struct mfd_cell gpio_keys_cell = { > + .name = "gpio-keys", > + .resources = (const struct resource[]) { > + DEFINE_RES_IRQ_NAMED(button_irq, "bd71828-pwrkey"), > + }, > + .num_resources = 1, > + .swnode = &nodes[0], > + }; I don't really love seeing variables declared in the middle of a block. Perhaps consider splitting the software-node preparation in own function and doing something like (completely untested thought): ret = alloc_and_prepare_the_swnode_stuff(...); if (!ret) { const struct software_node *nodes[] = { ... }; const struct mfd_cell gpio_keys_cell = { ... }; ... } or alternatively, split the software-node and MFD device registration into own function? Do you think it would work? > + 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 +981,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; > @@ -1008,6 +1061,8 @@ static int bd71828_i2c_probe(struct i2c_client *i2c) > dev_dbg(&i2c->dev, "Registered %d IRQs for chip\n", > irqchip->num_irqs); > > + irq_domain = regmap_irq_get_domain(irq_data); nit: Maybe move this call closer to where the irq_domain is actually needed for the first time. > + > /* > * On some ICs the main IRQ register has corresponding mask register. > * This is not handled by the regmap IRQ. Let's enable all the main > @@ -1022,23 +1077,21 @@ 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; > > 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) { > -- --- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~