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

[Vladimir Oltean <vladimir.oltean@xxxxxxx>]

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.

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

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

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