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

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

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

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