On Wed, Jan 22, 2025 at 3:44 AM Vaishnav Achath [off-list ref] wrote:

Hi Rob,

On 17/12/24 23:41, Rob Herring (Arm) wrote:

quoted

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>
---
v2:
  - Fix logic when a syscon is found in list to not return an error
---
  drivers/mfd/syscon.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79..226915ca3c93dcaf47bdd46b58e00e10e155f952 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c

@@ -171,9 +171,12 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
                      break;
              }

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

Earlier the above check was only in syscon_node_to_regmap() , but now
the users of device_node_to_regmap() also undergoes this check and there
are few drivers in tree which uses device_node_to_regmap() without
having "syscon" in compatible, likely those drivers need to be fixed,
but this patch breaks all those drivers now, atleast the below drivers
are affected and all TI platforms fail to boot due to this:

drivers/soc/ti/k3-socinfo.c
drivers/mux/mmio.c ("reg-mux" users)
drivers/clk/keystone/syscon-clk.c

All the above drivers fail due to the above check for syscon compatible.

What is the recommended solution to fix this?

I've sent a fix[1]. That's not to say I don't agree with fixes to
drivers removing device_node_to_regmap() calls. If you just need a
regmap for within a single driver, then just use regmap APIs directly.
What's in syscon.c is for (surprise!) syscon's, and those have the
characteristic of being shared.

Rob

[1] https://lore.kernel.org/all/20250124191644.2309790-1-robh@kernel.org/ (local)