Re: [PATCH v11 net-next 1/9] mfd: ocelot: add helper to get regmap from a resource

[Vladimir Oltean <vladimir.oltean@xxxxxxx>]

On Fri, Jul 01, 2022 at 10:18:31AM -0700, Colin Foster wrote:
> While I have your ear: do I need to check for dev->parent == NULL before
> calling dev_get_regmap? I see find_dr will call
> (dev->parent)->devres_head... but specifically "does every device have a
> valid parent?"

While the technical answer is "no", the practical answer is "pretty much".
Platform devices sit at least on the "platform" bus created in drivers/base/platform.c,
and they are reparented to the "platform_bus" struct device named "platform"
within platform_device_add(), if they don't have a parent.

Additionally, for MMIO-controlled platform devices in Ocelot, these have
as parent a platform device probed by the drivers/bus/simple-pm-bus.c
driver on the "ahb@70000000" simple-bus OF node. That simple-bus
platform device has as parent the "platform_bus" device mentioned above.

So it's a pretty long way to the top in the device hierarchy, I wouldn't
concern myself too much with checking for NULL, unless you intend to
call dev_get_regmap() on a parent's parent's parent, or things like that.

> > > }
> > > 
> > > So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain
> > > an inline helper function. And so long as ocelot_core_init does this:
> > > 
> > > static void ocelot_core_try_add_regmap(struct device *dev,
> > >                                        const struct resource *res)
> > > {
> > >         if (!dev_get_regmap(dev, res->name)) {
> > >                 ocelot_spi_init_regmap(dev, res);
> > >         }
> > > }
> > > 
> > > static void ocelot_core_try_add_regmaps(struct device *dev,
> > >                                         const struct mfd_cell *cell)
> > > {
> > >         int i;
> > > 
> > >         for (i = 0; i < cell->num_resources; i++) {
> > >                 ocelot_core_try_add_regmap(dev, &cell->resources[i]);
> > >         }
> > > }
> > > 
> > > int ocelot_core_init(struct device *dev)
> > > {
> > >         int i, ndevs;
> > > 
> > >         ndevs = ARRAY_SIZE(vsc7512_devs);
> > > 
> > >         for (i = 0; i < ndevs; i++)
> > >                 ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
> > 
> > Dumb question, why just "try"?
> 
> Because of this conditional:
> > >         if (!dev_get_regmap(dev, res->name)) {
> Don't add it if it is already there.

Hmm. So that's because you add regmaps iterating by the resource table
of each device. What if you keep a single resource table for regmap
creation purposes, and the device resource tables as separate?

> This might get interesting... The soc uses the HSIO regmap by way of
> syscon. Among other things, drivers/phy/mscc/phy-ocelot-serdes.c. If
> dev->parent has all the regmaps, what role does syscon play?
> 
> But that's a problem for another day...

Interesting question. I think part of the reason why syscon exists is to
not have OF nodes with overlapping address regions. In that sense, its
need does not go away here - I expect the layout of OF nodes beneath the
ocelot SPI device to be the same as their AHB variants. But in terms of
driver implementation, I don't know. Even if the OF nodes for your MFD
functions will contain all the regs that their AHB variants do, I'd
personally still be inclined to also hardcode those as resources in the
ocelot mfd parent driver and use those - case in which the OF regs will
more or less exist just as a formality. Maybe because the HSIO syscon is
already compatible with "simple-mfd", devices beneath it should just
probe. I haven't studied how syscon_node_to_regmap() behaves when the
syscon itself is probed as a MFD function. If that "just works", then
the phy-ocelot-serdes.c driver might not need to be modified.