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

[John Madieu]

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh at kernel.org>
> Sent: Monday, December 16, 2024 6:40 PM
> To: John Madieu <john.madieu.xa at bp.renesas.com>
> Cc: arnd at arndb.de; heiko at sntech.de; krzysztof.kozlowski at linaro.org;
> lee at kernel.org; linux-arm-kernel at lists.infradead.org; linux-
> kernel at vger.kernel.org; liviu.dudau at arm.com; lpieralisi at kernel.org;
> pankaj.dubey at samsung.com; peter.griffin at linaro.org; sudeep.holla at arm.com;
> willmcvicker at google.com; Biju Das <biju.das.jz at bp.renesas.com>
> Subject: Re: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a
> "syscon" compatible
> 
> On Sun, Dec 15, 2024 at 2:34 PM John Madieu
> <john.madieu.xa at 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?

Did not see Will's answer while doing my tests. I however tested it and it
works. I did also test your v2 series, which worked with me as well.

I'll then drop syscon compatible string in my series and send the v2.

Thanks,
John

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