From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-dy1-f178.google.com (mail-dy1-f178.google.com [74.125.82.178]) (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 4DF1F64 for ; Thu, 28 May 2026 00:59:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.82.178 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779929964; cv=none; b=iXraVWjoO+3FZ5Csi+V4JjQuyLVT6CiYNNJOWo/0oiH7r6DOE34xDhyXwJEsA9JkQsYIKwYCN+HLuHV2NybNn4psQ+gMkNC+kqdVyIq5kn5sc2HG/FdGjGC8mm7QXkDjXiOah4B1ZhDGBUI+HD/oqLVACYeTL9P2M5CFgjcr7wM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779929964; c=relaxed/simple; bh=aF3FjW+h2NNH2Y8OLhc2GI+f7quanBLbFSDFsjH19F4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Wi3/o+pEROe33ASdHMVel1/Nqn0JbXd60sXnrXhb4JHr+Ol1ESvHnf6saG9Jn3iBsGUfm8WIzUgTVbCoQyAlFwNC417mkvDyHtq6jE2S3Xk2q5crn/4HT4qCKiSXgH19Mk/7nSe0nwjWXJkJcHRG1bON/EEUw/kIIVbcrIN0WsY= 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=Y3W6jLTx; arc=none smtp.client-ip=74.125.82.178 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="Y3W6jLTx" Received: by mail-dy1-f178.google.com with SMTP id 5a478bee46e88-3042a388168so4153314eec.1 for ; Wed, 27 May 2026 17:59:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1779929962; x=1780534762; 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=cFRWWAmYZIFJuImHjV4tbWqH2vxtOMOPUVIzwkesFqU=; b=Y3W6jLTxs9rsAGZxDFlP4vNIUocYNANlcaBWncS56AWtdOnTZXFlWZMJTrShQCb/fs q/X+Z0rYMtJCm1fOkbSFJIaFNJQxP17bERDJ6jkb+2qQWjGtP816xDWqMjKGrmJgRZaW zWXak6to9De7o+VpRHqlo3KBkBhm1hmj6obPccZSXuY9y3AD4iafhW/Ttl33U6JRfdPw PrXIVPIQ96qwi6TdCdoJfp2eL705iF029gpFO2S+dwdNCNwozxOx7hXVxdk7sPQAuIoR eJFKKE3QrxRMsOy6aMNLXPdKTjshqYHTYn7PjlVCw/Cmui2GUoEdCR/tH7VkXY4ujq/l eLMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779929962; x=1780534762; 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=cFRWWAmYZIFJuImHjV4tbWqH2vxtOMOPUVIzwkesFqU=; b=Ee91fWbJhQTw0ycigRlQThh7busyl0YN0ZjnJB5ojBipgMRiy5cNNqfuoe51THplzu pN28M9nCkxNpND1swpYiQfqLTXxam7ZrhSnPWx4Mlq5V1DUThWwzbqKL2tOR4Il/ypo4 gg7ECTyQ3nicH2MgV7ulDMDNNu9v6uySplzMrIArC28XzPvkYt+t34eETDVCNQ9TQfCL YmRzIe8DiGvD8mTlBhFSDjiuhARHhXUwX3fPs83CpOLoo4QYArBY0V1BJ3fj+YKVj7Og AMMnlL6RsvrFPuFuRzrqVA+/tBRgUcvEJHFTd27w+3HORz0obNQJLh2K5ytMOQGImlGO XSIw== X-Forwarded-Encrypted: i=1; AFNElJ94Ab3mRy4Z39kjPFjkkqcDl6QgtvCdKyedPN6M7JOiyU5Fe2GfKxtYKeXmE4F+4doM3b/wbvyDftISnDQ=@vger.kernel.org X-Gm-Message-State: AOJu0Yy1NilGbqhXy5WKIJfv2qPn8tMxsg2tLnlzbxoUhf9KEgBSIEfD AZj1qkx8HTAdUTHc6/s9ej22glTp0D/PjXtmczJ8CVOx5mVa/F9NIziiOTuieQ== X-Gm-Gg: Acq92OFy3vP0fLJWHjVttTU+2luknSiMli/5Fi67o8ZnwA1gv9MKQ/OuqTE2g1lQTB4 T3WKmsewxamOFMYUJL3dy2VU+j1cXFjmSti1D1FJ0QP/TaXN4ZGLXnIc6kIRTYfpnGXYZYXMpMy uQXqUhMNp7ti1oaZTWAFR4MNlNu6gHpuGnqgCAgwa3zmDOZYLGHAm3+rCfroTaAVh93PUDcB0Ux htyhDKigmxX7pIrVlhid6hbVtgyZJts4QoKhWcFA5QlvV09NfDcfWfUK21HUsWPePXfq68VQN+Q yGxKBeoM5NcbvkrWPpqn3ihvenk99fzY3UeerWSzSL3G9WTZ/x9OmY1sAGzb3W6R+PY+Hf5lY6D jif392LYwvQYkak3wRwYobkMwTgDw3ToHQ8w1o5/espivjvXjFdOHydhYj/NeA6hQ2+hQI2TTfr AkdUsdrGcavK9EmWV1RHaTdMl9IIoyTxPY8uejq9DuopRjqA54Dbzjl3ZJGD+2XSRWhjPIeZ58/ OE= X-Received: by 2002:a05:7300:2147:b0:2ca:bd22:6102 with SMTP id 5a478bee46e88-30430f5a25cmr12276938eec.14.1779929962288; Wed, 27 May 2026 17:59:22 -0700 (PDT) Received: from google.com ([2a00:79e0:2ebe:8:ca8d:7a6a:7fd3:5948]) by smtp.gmail.com with ESMTPSA id 5a478bee46e88-30451f3feadsm18251729eec.13.2026.05.27.17.59.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 May 2026 17:59:21 -0700 (PDT) Date: Wed, 27 May 2026 17:59:05 -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> <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: <20260521122046.GF2921053@google.com> 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. 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. > > > + }; > > + > > + 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. > 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? > > > + struct software_node *nodes; > > + int error; > > Nit: This is almost always 'ret'. OK. > > 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. OK. I was trying to co-locate final mfd cell structure adjustment with its use in registration. > > > + error = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, &gpio_keys_cell, 1, > > + NULL, 0, irq_domain); Thanks. -- Dmitry