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

[Vladimir Oltean <vladimir.oltean@xxxxxxx>]

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.