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

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

On Sat, Jul 02, 2022 at 12:42:06PM +0000, Vladimir Oltean wrote:
> 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.

Thanks for the info. I have the NULL check in there, since I followed
the code and didn't see anything in device initialization that always
initializes parent. Maybe a default initializer would be
dev->parent = dev;

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

That would work - though it seems like it might be adding extra info
that isn't necessary. I'll take a look.

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

That'd be nice! When I looked into it a few months ago I came to the
conclusion that I'd need to implement "mscc,ocelot-hsio" but maybe
there's something I missed.