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