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