On Tue, Jun 28, 2022 at 12:56:54PM -0700, Colin Foster wrote: > On Tue, Jun 28, 2022 at 09:04:21PM +0200, Andy Shevchenko wrote: > > On Tue, Jun 28, 2022 at 8:56 PM Vladimir Oltean <vladimir.oltean@xxxxxxx> wrote: > > > > > > On Tue, Jun 28, 2022 at 09:46:59PM +0300, Vladimir Oltean wrote: > > > > I searched for 5 minutes or so, and I noticed regmap_attach_dev() and > > > > dev_get_regmap(), maybe those could be of help? > > > > > > ah, I see I haven't really brought anything of value to the table, > > > dev_get_regmap() was discussed around v1 or so. I'll read the > > > discussions again in a couple of hours to remember what was wrong with > > > it such that you aren't using it anymore. > > > > It would be nice if you can comment after here to clarify that. > > Because in another series (not related to this anyhow) somebody > > insisted to use dev_get_regmap(). > > To add some info: The main issue at that time was "how do I get a spi > regmap instead of an mmio regmap from the device". V1 was very early, > and was before I knew about the pinctrl / mdio drivers that were to > come, so that led to the existing MFD implementation. > > I came across the IORESOURCE_REG, which seems to be created for exactly > this purpose. Seemingly I'm pretty unique though, since IORESOURCE_REG > doesn't get used much compared to IORESOURCE_MEM. > > Though I'll revisit this again. The switch portion of this driver (no > longer included in this patch set) is actually quite different from the > rest of the MFD in how it allocates regmaps, and that might have been > a major contributor at the time. So maybe I dismissed it at the time, > but it actually makes sense for the MFD portion now. I'm sorry, I can't actually understand what went wrong, I'll need some help from you, Colin. So during your RFC v1 and then v1 proper (Nov. 19, 2021), you talked about dev_get_regmap(dev->parent, res->name) yourself and proposed a relatively simple interface where the mfd child drivers would just request a regmap by its name: https://patchwork.kernel.org/project/netdevbpf/patch/20211119224313.2803941-4-colin.foster@xxxxxxxxxxxxxxxx/ In fact you implemented just this (Dec. 6, 2021): https://patchwork.kernel.org/project/netdevbpf/patch/20211203211611.946658-1-colin.foster@xxxxxxxxxxxxxxxx/#24637477 it's just that the pattern went something like: @@ -1368,7 +1369,11 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev) regmap_config.max_register = OCELOT_GPIO_SD_MAP * info->stride + 15 * 4; - info->map = devm_regmap_init_mmio(dev, base, ®map_config); + if (device_is_mfd(pdev)) + info->map = dev_get_regmap(dev->parent, "GCB"); + else + info->map = devm_regmap_init_mmio(dev, base, ®map_config); where Lee Jones (rightfully) commented asking why can't you just first check whether dev->parent has any GCB regmap to give you, and only then resort to call devm_regmap_init_mmio? A small comment with a small and pretty actionable change to be made. As best as I can tell, RFC v5 (Dec. 18, 2021) is the first version after v1 where you came back with proposed mfd patches: https://patchwork.kernel.org/project/netdevbpf/patch/20211218214954.109755-2-colin.foster@xxxxxxxxxxxxxxxx/ And while dev_get_regmap() was technically still there, its usage pattern changed radically. It was now just used as a sort of optimization in ocelot_mfd_regmap_init() to not create twice a regmap that already existed. What you introduced in RFC v5 instead was this "interface for MFD children to get regmaps": https://patchwork.kernel.org/project/netdevbpf/patch/20211218214954.109755-3-colin.foster@xxxxxxxxxxxxxxxx/ to which Lee replied that "This is almost certainly not the right way to do whatever it is you're trying to do!" And rightfully so. What happened to dev_get_regmap() as the "interface for MFD children to get regmaps" I wonder? dev_get_regmap() just wants a name, not a full blown resource. When you're a mfd child, you don't have a full resource, you just know the name of the regmap you want to use. Only the top dog needs to have access to the resources. And DSA as a MFD child is not top dog. So of course I expect it to change. Otherwise said, what is currently done in felix_init_structs() needs to be more or less replicated in its entirety in drivers/mfd/ocelot-core.c. All the regmaps of the switch SoC, created at mfd parent probe time, not on demand, and attached via devres to the mfd parent device, so that children can get them via dev_get_regmap. Next thing would be to modify the felix DSA driver so that it only attempts to create regmaps if it can do so (if it has the resource structures). If it doesn't have the resource structures, it calls dev_get_regmap(ocelot->dev->parent, target->name) and tries to get the regmaps that way. If that fails => so sad, too bad, fail to probe, bye. The point is that the ocelot-ext driver you're trying to introduce should have no resources in struct resource *target_io_res, *port_io_res, *imdio_res, etc. I really don't know why you're so fixated on this "regmap from resource" thing when the resource shouldn't even be of concern to the driver when used as a mfd child. The contract is _not_ "here's the resource, give me the regmap". The contract is "I want the regmap named XXX". And in order to fulfill that contract, a mfd child driver should _not_ call a symbol exported by the ocelot parent driver directly (for the builtin vs module dependency reasons you've mentioned yourself), but get the regmap from the list of regmaps managed by devres using devm_regmap_init(). Yes, there would need to exist a split between the target strings and their start and end offsets, because the ocelot-ext driver would still call dev_get_regmap() by the name. But that's a fairly minor rework, and by the way, it turns out that the introduction of felix->info->init_regmap() was indeed not the right thing to do, so you'll need to change that again. I am really not sure what went with the plan above. You said a while ago that you don't like the fact that regmaps exist in the parent independently of the drivers that use them and continue to do so even after the children unbind, and that feels "wrong". Yes, because?
