[PATCH 0/3] mfd: syscon: Cleanup, fix race condition and remove platform driver

[PATCH 0/3] mfd: syscon: Cleanup, fix race condition and remove platform driver

[Rob Herring (Arm)]

Questions from Krzysztof and a syscon related binding review got me 
looking at the syscon "driver". This series is the result.

This short series drops the stale platform driver part of syscon 
support, fixes a race condition in device_node_get_regmap()
which is used by all the lookup functions, and allows for registering 
nodes without "syscon" compatibles.

Compile tested only. Testing on Tensor, the one user of 
of_syscon_register_regmap(), would be helpful.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
Rob Herring (Arm) (3):
      mfd: syscon: Fix race in device_node_get_regmap()
      mfd: syscon: Remove the platform driver support
      mfd: syscon: Allow syscon nodes without a "syscon" compatible

 drivers/mfd/syscon.c                 | 92 +++++-------------------------------
 drivers/mfd/vexpress-sysreg.c        |  1 -
 include/linux/platform_data/syscon.h |  9 ----
 3 files changed, 13 insertions(+), 89 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241211-syscon-fixes-9f35ecb9bfbe

Best regards,
-- 
Rob Herring (Arm) <robh@kernel.org>

[PATCH 1/3] mfd: syscon: Fix race in device_node_get_regmap()

[Rob Herring (Arm)]

It is possible for multiple, simultaneous callers calling
device_node_get_regmap() with the same node to fail to find an entry in
the syscon_list. There is a period of time while the first caller is
calling of_syscon_register() that subsequent callers also fail to find
an entry in the syscon_list and then call of_syscon_register() a second
time.

Fix this by keeping the lock held until after of_syscon_register()
completes and adds the node to syscon_list. Convert the spinlock to a
mutex as many of the functions called in of_syscon_register() may sleep.

Fixes: bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/mfd/syscon.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 3e1d699ba934..72f20de9652d 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -15,6 +15,7 @@
 #include <linux/io.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
@@ -27,7 +28,7 @@
 
 static struct platform_driver syscon_driver;
 
-static DEFINE_SPINLOCK(syscon_list_slock);
+static DEFINE_MUTEX(syscon_list_lock);
 static LIST_HEAD(syscon_list);
 
 struct syscon {
@@ -54,6 +55,8 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 	struct resource res;
 	struct reset_control *reset;
 
+	WARN_ON(!mutex_is_locked(&syscon_list_lock));
+
 	struct syscon *syscon __free(kfree) = kzalloc(sizeof(*syscon), GFP_KERNEL);
 	if (!syscon)
 		return ERR_PTR(-ENOMEM);
@@ -146,9 +149,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 	syscon->regmap = regmap;
 	syscon->np = np;
 
-	spin_lock(&syscon_list_slock);
 	list_add_tail(&syscon->list, &syscon_list);
-	spin_unlock(&syscon_list_slock);
 
 	return_ptr(syscon);
 
@@ -169,7 +170,7 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 {
 	struct syscon *entry, *syscon = NULL;
 
-	spin_lock(&syscon_list_slock);
+	mutex_lock(&syscon_list_lock);
 
 	list_for_each_entry(entry, &syscon_list, list)
 		if (entry->np == np) {
@@ -177,11 +178,11 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 			break;
 		}
 
-	spin_unlock(&syscon_list_slock);
-
 	if (!syscon)
 		syscon = of_syscon_register(np, check_res);
 
+	mutex_unlock(&syscon_list_lock);
+
 	if (IS_ERR(syscon))
 		return ERR_CAST(syscon);
 
@@ -212,7 +213,7 @@ int of_syscon_register_regmap(struct device_node *np, struct regmap *regmap)
 		return -ENOMEM;
 
 	/* check if syscon entry already exists */
-	spin_lock(&syscon_list_slock);
+	mutex_lock(&syscon_list_lock);
 
 	list_for_each_entry(entry, &syscon_list, list)
 		if (entry->np == np) {
@@ -225,12 +226,12 @@ int of_syscon_register_regmap(struct device_node *np, struct regmap *regmap)
 
 	/* register the regmap in syscon list */
 	list_add_tail(&syscon->list, &syscon_list);
-	spin_unlock(&syscon_list_slock);
+	mutex_unlock(&syscon_list_lock);
 
 	return 0;
 
 err_unlock:
-	spin_unlock(&syscon_list_slock);
+	mutex_unlock(&syscon_list_lock);
 	kfree(syscon);
 	return ret;
 }

-- 
2.45.2

[PATCH 2/3] mfd: syscon: Remove the platform driver support

[Rob Herring (Arm)]

The platform driver is dead code. It is not used by DT platforms since
commit bdb0066df96e ("mfd: syscon: Decouple syscon interface from
platform devices") which said:

    For non-DT based platforms, this patch keeps syscon platform driver
    structure so that syscon can be probed and such non-DT based drivers
    can use syscon_regmap_lookup_by_pdev API and access regmap handles.
    Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
    we can completely remove platform driver of syscon, and keep only helper
    functions to get regmap handles.

The last user of syscon_regmap_lookup_by_pdevname() was removed in 2018.
syscon_regmap_lookup_by_pdevname() was then removed in 2019, but that
commit failed to remove the rest of the platform driver.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/mfd/syscon.c                 | 66 ------------------------------------
 drivers/mfd/vexpress-sysreg.c        |  1 -
 include/linux/platform_data/syscon.h |  9 -----
 3 files changed, 76 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 72f20de9652d..bfb1f69fcff1 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -12,22 +12,15 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/hwspinlock.h>
-#include <linux/io.h>
-#include <linux/init.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/of_platform.h>
-#include <linux/platform_data/syscon.h>
-#include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
 #include <linux/mfd/syscon.h>
 #include <linux/slab.h>
 
-static struct platform_driver syscon_driver;
-
 static DEFINE_MUTEX(syscon_list_lock);
 static LIST_HEAD(syscon_list);
 
@@ -337,62 +330,3 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
 	return regmap;
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
-
-static int syscon_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct syscon_platform_data *pdata = dev_get_platdata(dev);
-	struct syscon *syscon;
-	struct regmap_config syscon_config = syscon_regmap_config;
-	struct resource *res;
-	void __iomem *base;
-
-	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
-	if (!syscon)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENOENT;
-
-	base = devm_ioremap(dev, res->start, resource_size(res));
-	if (!base)
-		return -ENOMEM;
-
-	syscon_config.max_register = resource_size(res) - 4;
-	if (!syscon_config.max_register)
-		syscon_config.max_register_is_0 = true;
-
-	if (pdata)
-		syscon_config.name = pdata->label;
-	syscon->regmap = devm_regmap_init_mmio(dev, base, &syscon_config);
-	if (IS_ERR(syscon->regmap)) {
-		dev_err(dev, "regmap init failed\n");
-		return PTR_ERR(syscon->regmap);
-	}
-
-	platform_set_drvdata(pdev, syscon);
-
-	dev_dbg(dev, "regmap %pR registered\n", res);
-
-	return 0;
-}
-
-static const struct platform_device_id syscon_ids[] = {
-	{ "syscon", },
-	{ }
-};
-
-static struct platform_driver syscon_driver = {
-	.driver = {
-		.name = "syscon",
-	},
-	.probe		= syscon_probe,
-	.id_table	= syscon_ids,
-};
-
-static int __init syscon_init(void)
-{
-	return platform_driver_register(&syscon_driver);
-}
-postcore_initcall(syscon_init);
diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
index d34d58ce46db..ef03d6cec9ff 100644
--- a/drivers/mfd/vexpress-sysreg.c
+++ b/drivers/mfd/vexpress-sysreg.c
@@ -10,7 +10,6 @@
 #include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
-#include <linux/platform_data/syscon.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/stat.h>
diff --git a/include/linux/platform_data/syscon.h b/include/linux/platform_data/syscon.h
deleted file mode 100644
index 2c089dd3e2bd..000000000000
--- a/include/linux/platform_data/syscon.h
+++ /dev/null
@@ -1,9 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef PLATFORM_DATA_SYSCON_H
-#define PLATFORM_DATA_SYSCON_H
-
-struct syscon_platform_data {
-	const char *label;
-};
-
-#endif

-- 
2.45.2

[PATCH 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[Rob Herring (Arm)]

of_syscon_register_regmap() was added for nodes which need a custom
regmap setup. It's not really correct for those nodes to claim they are
compatible with "syscon" as the default handling likely doesn't work in
those cases. If device_node_get_regmap() happens to be called first,
then of_syscon_register() will be called and an incorrect regmap will be
created (barring some other error). That may lead to unknown results in
the worst case. In the best case, of_syscon_register_regmap() will fail
with -EEXIST. This problem remains unless these cases drop "syscon" (an
ABI issue) or we exclude them using their specific compatible. ATM,
there is only one user: "google,gs101-pmu"

There are also cases of adding "syscon" compatible to existing nodes
after the fact in order to register the syscon. That presents a
potential DT ABI problem. Instead, if there's a kernel change needing a
syscon for a node, then it should be possible to allow the kernel to
register a syscon without a DT change. That's only possible by using
of_syscon_register_regmap() currently, but in the future we may want to
support a match list for cases which don't need a custom regmap.

With this change, the lookup functions will succeed for any node
registered by of_syscon_register_regmap() regardless of whether the node
compatible contains "syscon".

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/mfd/syscon.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

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,
 			break;
 		}
 
-	if (!syscon)
+	if (!syscon && of_device_is_compatible(np, "syscon"))
 		syscon = of_syscon_register(np, check_res);
+	else
+		syscon = ERR_PTR(-EINVAL);
 
 	mutex_unlock(&syscon_list_lock);
 
@@ -238,9 +240,6 @@ EXPORT_SYMBOL_GPL(device_node_to_regmap);
 
 struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
-	if (!of_device_is_compatible(np, "syscon"))
-		return ERR_PTR(-EINVAL);
-
 	return device_node_get_regmap(np, true);
 }
 EXPORT_SYMBOL_GPL(syscon_node_to_regmap);

-- 
2.45.2

Re: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[William McVicker]

Hi Rob,

Thanks for working on this!

On 12/11/2024, Rob Herring (Arm) wrote:
> of_syscon_register_regmap() was added for nodes which need a custom
> regmap setup. It's not really correct for those nodes to claim they are
> compatible with "syscon" as the default handling likely doesn't work in
> those cases. If device_node_get_regmap() happens to be called first,
> then of_syscon_register() will be called and an incorrect regmap will be
> created (barring some other error). That may lead to unknown results in
> the worst case. In the best case, of_syscon_register_regmap() will fail
> with -EEXIST. This problem remains unless these cases drop "syscon" (an
> ABI issue) or we exclude them using their specific compatible. ATM,
> there is only one user: "google,gs101-pmu"
> 
> There are also cases of adding "syscon" compatible to existing nodes
> after the fact in order to register the syscon. That presents a
> potential DT ABI problem. Instead, if there's a kernel change needing a
> syscon for a node, then it should be possible to allow the kernel to
> register a syscon without a DT change. That's only possible by using
> of_syscon_register_regmap() currently, but in the future we may want to
> support a match list for cases which don't need a custom regmap.
> 
> With this change, the lookup functions will succeed for any node
> registered by of_syscon_register_regmap() regardless of whether the node
> compatible contains "syscon".
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/mfd/syscon.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> 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,
>  			break;
>  		}
>  
> -	if (!syscon)
> +	if (!syscon && of_device_is_compatible(np, "syscon"))
>  		syscon = of_syscon_register(np, check_res);
> +	else
> +		syscon = ERR_PTR(-EINVAL);

This else case actually breaks Pixel 6 (Tensor) since you are now returning
-EINVAL when the syscon is created by the exynos-pmu driver and present in the
list. Instead you should only return -EINVAL if the syscon doesn't exist and
the device node is not a compatible syscon device. If you still want to check
for `of_device_is_compatible(np, "syscon")` even when the syscon is found in
the list, that should be okay , but it's probably best to check that before
inserting the regmap in the list to begin with.

This worked for me on my Pixel 6 device:

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

Thanks,
Will

Re: [PATCH 1/3] mfd: syscon: Fix race in device_node_get_regmap()

[Krzysztof Kozlowski]

On 11/12/2024 21:57, Rob Herring (Arm) wrote:
> It is possible for multiple, simultaneous callers calling
> device_node_get_regmap() with the same node to fail to find an entry in
> the syscon_list. There is a period of time while the first caller is
> calling of_syscon_register() that subsequent callers also fail to find
> an entry in the syscon_list and then call of_syscon_register() a second
> time.
> 
> Fix this by keeping the lock held until after of_syscon_register()
> completes and adds the node to syscon_list. Convert the spinlock to a
> mutex as many of the functions called in of_syscon_register() may sleep.

Maybe mention the reason for conversion to mutex: lock will be now
containing non-atomic kzalloc and regmap_init_mmio, where the latter
calls clk_get and clk_prepare, which both are sleeping, I believe.

Otherwise it feels you are converting to mutex just to make code better,
which would be then a separate commit.

Everything else looks good.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 1/3] mfd: syscon: Fix race in device_node_get_regmap()

[Krzysztof Kozlowski]

On 11/12/2024 21:57, Rob Herring (Arm) wrote:
> It is possible for multiple, simultaneous callers calling
> device_node_get_regmap() with the same node to fail to find an entry in
> the syscon_list. There is a period of time while the first caller is
> calling of_syscon_register() that subsequent callers also fail to find
> an entry in the syscon_list and then call of_syscon_register() a second
> time.
> 
> Fix this by keeping the lock held until after of_syscon_register()
> completes and adds the node to syscon_list. Convert the spinlock to a
> mutex as many of the functions called in of_syscon_register() may sleep.
> 
> Fixes: bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
Tested on arm64: Qualcomm SM8450
Tested on armv7: Exynos5422

Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 2/3] mfd: syscon: Remove the platform driver support

[Krzysztof Kozlowski]

On 11/12/2024 21:57, Rob Herring (Arm) wrote:
> The platform driver is dead code. It is not used by DT platforms since
> commit bdb0066df96e ("mfd: syscon: Decouple syscon interface from
> platform devices") which said:
> 
>     For non-DT based platforms, this patch keeps syscon platform driver
>     structure so that syscon can be probed and such non-DT based drivers
>     can use syscon_regmap_lookup_by_pdev API and access regmap handles.
>     Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
>     we can completely remove platform driver of syscon, and keep only helper
>     functions to get regmap handles.
> 
> The last user of syscon_regmap_lookup_by_pdevname() was removed in 2018.
> syscon_regmap_lookup_by_pdevname() was then removed in 2019, but that
> commit failed to remove the rest of the platform driver.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
FWIW:

Tested on arm64: Qualcomm SM8450
Tested on armv7: Samsung Exynos5422

Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[John Madieu]

Hi Rob,

On Wed, 11 Dec 2024 14:57:14 -0600 Rob Herring wrote:
> 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,
> 			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.

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;
		mutex_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);
	return ERR_PTR(-EINVAL);
}

The above is the resulting working function I've obtained while testing with
different scenarios. This ensures that:

 1. Already registered syscons are found and returned correctly
 2. New syscons with "syscon" compatible are registered as before
 3. Nodes without "syscon" compatible return -EINVAL only if not found in the list

This has been tested with my v1 syscon work [1] and with v2, where I remove the
"syscon" compatible string for custom regmap initialization, aligning with your
goals for this series.

Let me know if I should add this series as a dependency of my v2 or how I should proceed.

Thanks,

John Madieu

[1] https://lore.kernel.org/all/20241206212559.192705-2-john.madieu.xa@bp.renesas.com

Re: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[Rob Herring]

On Sun, Dec 15, 2024 at 2:34 PM John Madieu
<john.madieu.xa@bp.renesas.com> wrote:
>
> Hi Rob,
>
> On Wed, 11 Dec 2024 14:57:14 -0600 Rob Herring wrote:
> > 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,
> >                       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?

>
> 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

RE: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[John Madieu]

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 <biju.das.jz@bp.renesas.com>
> 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
> <john.madieu.xa@bp.renesas.com> wrote:
> >
> > Hi Rob,
> >
> > On Wed, 11 Dec 2024 14:57:14 -0600 Rob Herring wrote:
> > > 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,
> > >                       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

> 
> >
> > 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