Re: [PATCH 2/3] mfd: rohm-bd71828: Use software nodes for gpio-keys
From: Matti Vaittinen
Date: Tue Aug 19 2025 - 06:50:09 EST
On 18/08/2025 20:11, Dmitry Torokhov wrote:
On Mon, Aug 18, 2025 at 09:56:07AM +0300, Matti Vaittinen wrote:
On 18/08/2025 09:54, Matti Vaittinen wrote:
On 18/08/2025 01:47, 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.
Thanks for doing this Dmitry! I believe I didn't understand how
providing the IRQs via swnode works... :)
If I visit the ROHM office this week, then I will try to test this using
the PMIC HW. (Next week I'll be in ELCE, and after it I have probably
already forgotten this...)
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 <dmitry.torokhov@xxxxxxxxx>
---
drivers/mfd/rohm-bd71828.c | 81 +++++++++++++++++++++++++++-----------
1 file changed, 58 insertions(+), 23 deletions(-)
diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
index a14b7aa69c3c..c29dde9996b7 100644
--- a/drivers/mfd/rohm-bd71828.c
+++ b/drivers/mfd/rohm-bd71828.c
@@ -4,7 +4,6 @@
// ...snip
+static int bd71828_reg_cnt;
+
+static int bd71828_i2c_register_swnodes(void)
+{
+ int error;
+
+ if (bd71828_reg_cnt == 0) {
Isn't this check racy...
+ error = software_node_register_node_group(bd71828_swnodes);
+ if (error)
+ return error;
+ }
+
+ bd71828_reg_cnt++;
... with this...
+ return 0;
+}
+
+static void bd71828_i2c_unregister_swnodes(void *dummy)
+{
+ if (bd71828_reg_cnt != 0) {
...this...
+ software_node_unregister_node_group(bd71828_swnodes);
+ bd71828_reg_cnt--;
...and this? Perhaps add a mutex or use atomics?
Also, shouldn't the software_node_unregister_node_group() be only called
for the last instance to exit (Eg, "if (bd71828_reg_cnt == 0)" instead
of the "if (bd71828_reg_cnt != 0) {")?
Oh. Probably "if (bd71828_reg_cnt == 1)".
You are right, I am not sure what I was thinking when I wrote this.
I actually doubt that sharing of nodes between devices would work well.
But I believe these devices are singletons, it should not be possible to
have several of them in a single system, right?
I can't say for sure. I have seen more and more setups where more than
one PMIC is used to power-up a system. Thus I nowadays try to use
solutions which don't limit the amount of instances.
The BD718[37,47,50] regulator driver seems to be written in a way it
doesn't properly support multiple driver instances. (It uses global
data, with a comment that if multiple instances need to be supported the
data should be copied):
https://elixir.bootlin.com/linux/v6.11-rc2/source/drivers/regulator/bd718x7-regulator.c#L1558
For BD71828 and BD71815 I don't see existing limitations on how many
instances there can be...
...except that I do :)
The current MFD driver uses single static global for the gpio_keys
platform data. I assume that wouldn't be race-free if we had multiple
instances.
So, I am unsure what to say. I know that for example the BD9680x PMIC
series is intended to be used with multi-PMIC configurations, and I
believe these setups are getting more common. Hence I would like to see
the bd718XX code to work on multi-PMIC systems too, so the gpio_keys
swnode example could be copied over to new PMICs ;)
But yeah, I am not insisting on it. The existing solution does not
support multiple instances, so if you think it gets too cumbersome to
add such support, then I am happy with supporting just one chip/system.
So maybe the best way is
to simply instantiate them in probe and bail out if they are already
registered.
Well, I wouldn't say best (as explained above), but yes, sufficient for
these PMICs AFAICS.
Thanks for doing this!
Yours,
-- Matti