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

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

On Thu, Jun 30, 2022 at 01:11:56PM +0000, Vladimir Oltean wrote:
> On Wed, Jun 29, 2022 at 04:54:35PM -0700, Colin Foster wrote:
> > > > In that case, "name" would either be hard-coded to match what is in
> > > > drivers/mfd/ocelot-core.c. The other option is to fall back to
> > > > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in
> > > > resource->name. I'll be able to deal with that when I try it. (hopefully
> > > > this evening)
> > > 
> > > I'm not exactly clear on what you'd do with the REG resource once you
> > > get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;"
> > > from the device tree, what next, who's going to set up the SPI regmap
> > > for you?
> > 
> > The REG resource would only get the resource name, while the MFD core
> > driver would set up the regmaps.
> > 
> > e.g. drivers/mfd/ocelot-core.c has (annotated):
> > static const struct resource vsc7512_sgpio_resources[] = {
> >     DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") };
> > 
> > Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the
> > gpio resource, and gets the resource by index.
> > 
> > So for this there seem to be two options:
> > Option 1:
> > drivers/pinctrl/pinctrl-ocelot.c:
> > res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> > map = dev_get_regmap(dev->parent, res->name);
> > 
> > 
> > OR Option 2:
> > include/linux/mfd/ocelot.h has something like:
> > #define GCB_GPIO_REGMAP_NAME "gcb_gpio"
> > 
> > and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to:
> > map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME);
> > 
> > (With error checking, macro reuse, etc.)
> > 
> > 
> > I like option 1, since it then makes ocelot-pinctrl.c have no reliance
> > on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are
> > set up in advance during the start of ocelot_core_init, just before
> > devm_mfd_add_devices is called.
> > 
> > 
> > I should be able to test this all tonight.
> 
> I see what you mean now with the named resources from drivers/mfd/ocelot-core.c.
> I don't particularly like the platform_get_resource(0) option, because
> it's not as obvious/searchable what resource the pinctrl driver is
> asking for.
> 
> I suppose a compromise variant might be to combine the 2 options.
> Put enum ocelot_target in a header included by both drivers/mfd/ocelot-core.c,
> then create a _single_ resource table in the MFD driver, indexed by enum
> ocelot_target:
> 
> static const struct resource vsc7512_resources[TARGET_MAX] = {
> 	[ANA] = DEFINE_RES_REG_NAMED(start, end, "ana"),
> 	...
> };
> 
> then provide the exact same resource table to all children.
> 
> In the pinctrl driver you can then do:
> 	res = platform_get_resource(pdev, IORESOURCE_REG, GPIO);
> 	map = dev_get_regmap(dev->parent, res->name);
> 
> and you get both the benefit of not hardcoding the string twice, and the
> benefit of having some obvious keyword which can be used to link the mfd
> driver to the child driver via grep, for those trying to understand what
> goes on.
> 
> In addition, if there's a single resource table used for all peripherals,
> theoretically you need to modify less code in mfd/ocelot-core.c in case
> one driver or another needs access to one more regmap, if that regmap
> happened to be needed by some other driver already. Plus fewer tables to
> lug around, in general.

Ok... so I haven't yet changed any of the pinctrl / mdio drivers yet,
but I'm liking this:

static inline struct regmap *
ocelot_regmap_from_resource(struct platform_device *pdev, unsigned int index,
                            const struct regmap_config *config)
{
        struct device *dev = &pdev->dev;
        struct resource *res;
        u32 __iomem *regs;

        res = platform_get_resource(pdev, IORESOURCE_MEM, index);
        if (res) {
                regs = devm_ioremap_resource(dev, res);
                if (IS_ERR(regs))
                        return ERR_CAST(regs);
                return devm_regmap_init_mmio(dev, regs, config);
        }

        /*
         * Fall back to using REG and getting the resource from the parent
         * device, which is possible in an MFD configuration
         */
        res = platform_get_resource(pdev, IORESOURCE_REG, index);
        if (!res)
                return ERR_PTR(-ENOENT);

        return (dev_get_regmap(dev->parent, res->name));
}

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]);

        return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
                                    ndevs, NULL, 0, NULL);
}
EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);

we're good! (sorry about spaces / tabs... I have to up my mutt/vim/tmux
game still)


I like the enum / macro idea for cleanup, but I think that's a different
problem I can address. The main question I have now is this:

The ocelot_regmap_from_resource now has nothing to do with the ocelot
MFD system. It is generic. (If you listen carefully, you might hear me
cheering)

I can keep this in linux/mfd/ocelot.h, but is this actually something
that belongs elsewhere? platform? device? mfd-core?


And yes, I like the idea of changing the driver to
"ocelot_regmap_from_resource(pdev, GPIO, config);" from
"ocelot_regmap_from_resource(pdev, 0, config);"