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

[John Madieu]

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@xxxxxxxxxx>
> Sent: Monday, December 16, 2024 6:40 PM
> To: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> Cc: arnd@xxxxxxxx; heiko@xxxxxxxxx; krzysztof.kozlowski@xxxxxxxxxx;
> lee@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; liviu.dudau@xxxxxxx; lpieralisi@xxxxxxxxxx;
> pankaj.dubey@xxxxxxxxxxx; peter.griffin@xxxxxxxxxx; sudeep.holla@xxxxxxx;
> willmcvicker@xxxxxxxxxx; Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 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@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?

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