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.
