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

John Madieu
2024-12-18T10:50:22Z

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Monday, December 16, 2024 6:40 PM
> To: John Madieu <john.madieu.xa@bp.renesas.com>
> Cc: arnd@arndb.de; heiko@sntech.de; krzysztof.kozlowski@linaro.org;
> lee@kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; liviu.dudau@arm.com; lpieralisi@kernel.org;
> pankaj.dubey@samsung.com; peter.griffin@linaro.org; sudeep.holla@arm.com;
> willmcvicker@google.com; Biju Das <biju.das.jz@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@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