Re: [PATCH 2/3] mfd: rohm-bd71828: Use software nodes for gpio-keys

[Matti Vaittinen <mazziesaccount@gmail.com>]

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@gmail.com>
>>>> ---
>>>>    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