Hi Rob,

-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Monday, December 16, 2024 6:40 PM
To: John Madieu <john.madieu.xa@bp.renesas.com>
Cc: arnd@arndb.de; heiko@sntech.de; krzysztof.kozlowski@linaro.org;
lee@kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org; liviu.dudau@arm.com; lpieralisi@kernel.org;
pankaj.dubey@samsung.com; peter.griffin@linaro.org; sudeep.holla@arm.com;
willmcvicker@google.com; Biju Das [off-list ref]
Subject: Re: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a
"syscon" compatible

On Sun, Dec 15, 2024 at 2:34 PM John Madieu
[off-list ref] wrote:

quoted

Hi Rob,

On Wed, 11 Dec 2024 14:57:14 -0600 Rob Herring wrote:

quoted

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index

bfb1f69fcff1..e6df2825c14d 100644

--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c

@@ -171,8 +171,10 @@ static struct regmap

*device_node_get_regmap(struct device_node *np,

quoted

quoted

                      break;
              }

-     if (!syscon)
+     if (!syscon && of_device_is_compatible(np, "syscon"))
              syscon = of_syscon_register(np, check_res);
+     else
+             syscon = ERR_PTR(-EINVAL);

The current modification will make device_node_get_regmap() return
-EINVAL even for syscons that were already found in the syscon_list,
which I believe is not the intended behavior.

Yes, it is. Doesn't Will's fix work for you?

Did not see Will's answer while doing my tests. I however tested it and it
works. I did also test your v2 series, which worked with me as well.

I'll then drop syscon compatible string in my series and send the v2.

Thanks,
John

quoted

I suggest modifying it this way to maintain lookup functionality for
registered syscons while implementing your intended changes:

static struct regmap *device_node_get_regmap(struct device_node *np,
                                             bool check_res) {
        struct syscon *entry, *syscon = NULL;
        struct regmap *regmap;

        mutex_lock(&syscon_list_lock);

        list_for_each_entry(entry, &syscon_list, list)
                if (entry->np == np) {
                        syscon = entry;
                        break;
                }

        if (syscon) {
                regmap = syscon->regmap;
                mut ix_unlock(&syscon_list_lock);
                return regmap;
        }

        if (of_device_is_compatible(np, "syscon")) {
                syscon = of_syscon_register(np, check_res);
                mutex_unlock(&syscon_list_lock);
                if (IS_ERR(syscon))
                        return ERR_CAST(syscon);
                return syscon->regmap;
        }

        mutex_unlock(&syscon_list_lock);

3 unlock calls is a sign the code structure could be improved. A goto or a
guard() for example. However, I think this is the same logic as what Will
suggested.

Rob