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

[Vaishnav Achath]

Hi Rob,

On 17/12/24 23:41, 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@xxxxxxxxxx>
> ---
> 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?

Thanks and Regards,
Vaishnav

>   	mutex_unlock(&syscon_list_lock);
>   
>   	if (IS_ERR(syscon))
> @@ -238,9 +241,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);