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

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

On Fri, Jul 01, 2022 at 04:21:28PM +0000, Vladimir Oltean wrote:
> On Thu, Jun 30, 2022 at 01:09:51PM -0700, Colin Foster wrote:
> > 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));
> 
> parentheses not needed around dev_get_regmap.

Oops.

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

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


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

> 
> > 
> >         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?
> 
> Sounds like something which could be named devm_platform_get_regmap_from_resource_or_parent(),
> but I'm not 100% clear where it should sit. Platform devices are independent
> of regmap, regmap is independent of platform devices, device core of both.
> 
> FWIW platform devices are always built-in and have no config option;
> regmap is bool and is selected by others.
> 
> Logically, the melting pot of regmaps and platform devices is mfd.
> However, it seems that include/linux/mfd/core.h only provides API for
> mfd parent drivers, not children. So a new header would be needed?
> 
> Alternatively, you could just duplicate this logic in the drivers
> (by the way, only spelling out the function name takes up half of the
> implementation). How many times would it be needed? Felix DSA would roll
> its own thing, as mentioned. I'm thinking, let it be open coded for now,
> let's agree on the entire solution in terms of operations that are
> actually being done, and we can revisit proper placement for this later.

I came to the same conclusion. Hopefully I'll button up v12 today.

> 
> > 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);"
> 
> Sorry, I just realized we need to junk this idea with GPIO instead of 0.
> Presenting the entire resource table to all peripherals implies that
> there is no more than one single peripheral of each kind. This is not
> true for the MDIO controllers, where the driver would need to know it
> has to request the region corresponding to MIIM1 or MIIM2 according to
> some crystal ball.