Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

On Tue, Jan 11, 2022 at 10:28:15AM -0800, Colin Foster wrote:
> Hi Lee,
> 
> On Tue, Jan 11, 2022 at 05:00:11PM +0000, Lee Jones wrote:
> > On Tue, 11 Jan 2022, Colin Foster wrote:
> > 
> > > Hi Mark and Lee,
> > > 
> > > > 
> > > > > However, even if that is required, I still think we can come up with
> > > > > something cleaner than creating a whole API based around creating
> > > > > and fetching different regmap configurations depending on how the
> > > > > system was initialised.
> > > > 
> > > > Yeah, I'd expect the usual pattern is to have wrapper drivers that
> > > > instantiate a regmap then have the bulk of the driver be a library that
> > > > they call into should work.

I'm re-reading this with a fresh set of eyes. I've been jumping back and
forth between the relatively small drivers (sgpio, pinctrl) and more
complex ones (felix). I was looking at this from the narrow scope of the
smaller drivers, which typically only handle a small regmap... only a
handful of registers each. For those, there's no problem, and is pretty
much what I've done.

The Felix driver is different. Currently the VSC7512 has to allocate 20
different regmaps. Nine for different features, some optional. 11 for
each of the different ports. The VSC7511 will likely have 19 different
ones because their ranges might not be identical. Same with the 7513,
7514.

In this example code, the resources are getting defined and allocated
entirely within the felix system itself:

(drivers/net/dsa/ocelot/felix_vsc9959.c):
static const struct resource vsc9959_target_io_res[TARGET_MAX] = {
        [ANA] = {
                .start  = 0x0280000,
                .end    = 0x028ffff,
                .name   = "ana",
        },
        [QS] = {
                .start  = 0x0080000,
                .end    = 0x00800ff,
                .name   = "qs",
        },
        ...
};

(drivers/net/dsa/ocelot/felix.c):
for (i = 0; i < TARGET_MAX; i++) {
        struct regmap *target;

        if (!felix->info->target_io_res[i].name)
                continue;

        memcpy(&res, &felix->info->target_io_res[i], sizeof(res));
        res.flags = IORESOURCE_MEM;
        res.start += felix->switch_base;
        res.end += felix->switch_base;

        target = felix->info->init_regmap(ocelot, &res);
        if (IS_ERR(target)) {
                dev_err(ocelot->dev,
                        "Failed to map device memory space\n");
                kfree(port_phy_modes);
                return PTR_ERR(target);
        }

        ocelot->targets[i] = target;
}

So Felix will say "give me regmaps from this array of resources."
Resources have been added as development of Felix has progressed - in
this type of scenario they should be able to do exactly that without
having to "pre-register" with MFD. More specifically: why should adding
precision time protocol to drivers/net/dsa/felix/ocelot_ext.c have any
effect on drivers/mfd/ocelot-core.c?

The patch that I submitted utilized the function
ocelot_get_regmap_from_resource. Does this qualify as a "wrapper driver
that instantates a regmap"? The more I think about it, the more I think
that's exacly what the current implementation is. It just creates
regmaps for all the child devices... and it creates a lot of regmaps...
and it will have a lot of child devices...

Maybe something will come to me in the next week or two - clearly I'm
prone to changing my mind. But in the meantime I'll focus on cleaning up
the rest of the changes that were suggested and prepare a new RFC.

Thanks, and I'm looking forward to continuing work on this for
(hopefully) 5.18!

Colin Foster