Date:   Wed, 29 Jun 2022 17:53:06 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Colin Foster <colin.foster@...advantage.com>
CC:     Andy Shevchenko <andy.shevchenko@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Wolfram Sang <wsa@...nel.org>,
        Terry Bowman <terry.bowman@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Arnd Bergmann <arnd@...nel.org>
Subject: Re: [PATCH v11 net-next 1/9] mfd: ocelot: add helper to get regmap
 from a resource

On Tue, Jun 28, 2022 at 12:56:54PM -0700, Colin Foster wrote:
> On Tue, Jun 28, 2022 at 09:04:21PM +0200, Andy Shevchenko wrote:
> > On Tue, Jun 28, 2022 at 8:56 PM Vladimir Oltean <vladimir.oltean@....com> wrote:
> > >
> > > On Tue, Jun 28, 2022 at 09:46:59PM +0300, Vladimir Oltean wrote:
> > > > I searched for 5 minutes or so, and I noticed regmap_attach_dev() and
> > > > dev_get_regmap(), maybe those could be of help?
> > >
> > > ah, I see I haven't really brought anything of value to the table,
> > > dev_get_regmap() was discussed around v1 or so. I'll read the
> > > discussions again in a couple of hours to remember what was wrong with
> > > it such that you aren't using it anymore.
> > 
> > It would be nice if you can comment after here to clarify that.
> > Because in another series (not related to this anyhow) somebody
> > insisted to use dev_get_regmap().
> 
> To add some info: The main issue at that time was "how do I get a spi
> regmap instead of an mmio regmap from the device". V1 was very early,
> and was before I knew about the pinctrl / mdio drivers that were to
> come, so that led to the existing MFD implementation.
> 
> I came across the IORESOURCE_REG, which seems to be created for exactly
> this purpose. Seemingly I'm pretty unique though, since IORESOURCE_REG
> doesn't get used much compared to IORESOURCE_MEM.
> 
> Though I'll revisit this again. The switch portion of this driver (no
> longer included in this patch set) is actually quite different from the
> rest of the MFD in how it allocates regmaps, and that might have been
> a major contributor at the time. So maybe I dismissed it at the time,
> but it actually makes sense for the MFD portion now.

I'm sorry, I can't actually understand what went wrong, I'll need some
help from you, Colin.

So during your RFC v1 and then v1 proper (Nov. 19, 2021), you talked
about dev_get_regmap(dev->parent, res->name) yourself and proposed a
relatively simple interface where the mfd child drivers would just
request a regmap by its name:
https://patchwork.kernel.org/project/netdevbpf/patch/20211119224313.2803941-4-colin.foster@in-advantage.com/

In fact you implemented just this (Dec. 6, 2021):
https://patchwork.kernel.org/project/netdevbpf/patch/20211203211611.946658-1-colin.foster@in-advantage.com/#24637477
it's just that the pattern went something like:

@@ -1368,7 +1369,11 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
 	regmap_config.max_register = OCELOT_GPIO_SD_MAP * info->stride + 15 * 4;

-	info->map = devm_regmap_init_mmio(dev, base, &regmap_config);
+	if (device_is_mfd(pdev))
+		info->map = dev_get_regmap(dev->parent, "GCB");
+	else
+		info->map = devm_regmap_init_mmio(dev, base, &regmap_config);

where Lee Jones (rightfully) commented asking why can't you just first
check whether dev->parent has any GCB regmap to give you, and only then
resort to call devm_regmap_init_mmio? A small comment with a small
and pretty actionable change to be made.


As best as I can tell, RFC v5 (Dec. 18, 2021) is the first version after
v1 where you came back with proposed mfd patches:
https://patchwork.kernel.org/project/netdevbpf/patch/20211218214954.109755-2-colin.foster@in-advantage.com/

And while dev_get_regmap() was technically still there, its usage
pattern changed radically. It was now just used as a sort of
optimization in ocelot_mfd_regmap_init() to not create twice a regmap
that already existed.
What you introduced in RFC v5 instead was this "interface for MFD
children to get regmaps":
https://patchwork.kernel.org/project/netdevbpf/patch/20211218214954.109755-3-colin.foster@in-advantage.com/

to which Lee replied that "This is almost certainly not the right way to
do whatever it is you're trying to do!"

And rightfully so. What happened to dev_get_regmap() as the "interface
for MFD children to get regmaps" I wonder?
dev_get_regmap() just wants a name, not a full blown resource.
When you're a mfd child, you don't have a full resource, you just know
the name of the regmap you want to use. Only the top dog needs to have
access to the resources. And DSA as a MFD child is not top dog.
So of course I expect it to change. Otherwise said, what is currently
done in felix_init_structs() needs to be more or less replicated in its
entirety in drivers/mfd/ocelot-core.c.
All the regmaps of the switch SoC, created at mfd parent probe time, not
on demand, and attached via devres to the mfd parent device, so that
children can get them via dev_get_regmap.

Next thing would be to modify the felix DSA driver so that it only
attempts to create regmaps if it can do so (if it has the resource
structures). If it doesn't have the resource structures, it calls
dev_get_regmap(ocelot->dev->parent, target->name) and tries to get the
regmaps that way. If that fails => so sad, too bad, fail to probe, bye.

The point is that the ocelot-ext driver you're trying to introduce
should have no resources in struct resource *target_io_res, *port_io_res,
*imdio_res, etc. I really don't know why you're so fixated on this
"regmap from resource" thing when the resource shouldn't even be of
concern to the driver when used as a mfd child.

The contract is _not_ "here's the resource, give me the regmap".
The contract is "I want the regmap named XXX". And in order to fulfill
that contract, a mfd child driver should _not_ call a symbol exported by
the ocelot parent driver directly (for the builtin vs module dependency
reasons you've mentioned yourself), but get the regmap from the list of
regmaps managed by devres using devm_regmap_init().

Yes, there would need to exist a split between the target strings and
their start and end offsets, because the ocelot-ext driver would still
call dev_get_regmap() by the name. But that's a fairly minor rework, and
by the way, it turns out that the introduction of felix->info->init_regmap()
was indeed not the right thing to do, so you'll need to change that again.

I am really not sure what went with the plan above. You said a while ago
that you don't like the fact that regmaps exist in the parent
independently of the drivers that use them and continue to do so even
after the children unbind, and that feels "wrong". Yes, because?

Powered by blists - more mailing lists