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

Rob Herring
2024-12-16T17:40:07Z

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