Re: [RFC v6 net-next 6/9] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

Hello Lee,

On Tue, Feb 01, 2022 at 07:36:21AM +0000, Lee Jones wrote:
> On Mon, 31 Jan 2022, Colin Foster wrote:
> 
> > Hi Lee,
> > 
> > Thank you very much for your time / feedback.
> > 
> > On Mon, Jan 31, 2022 at 09:29:34AM +0000, Lee Jones wrote:
> > > On Sat, 29 Jan 2022, Colin Foster wrote:
> > > 
> > > > Create a single SPI MFD ocelot device that manages the SPI bus on the
> > > > external chip and can handle requests for regmaps. This should allow any
> > > > ocelot driver (pinctrl, miim, etc.) to be used externally, provided they
> > > > utilize regmaps.
> > > > 
> > > > Signed-off-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/mfd/Kconfig                       |  19 ++
> > > >  drivers/mfd/Makefile                      |   3 +
> > > >  drivers/mfd/ocelot-core.c                 | 165 +++++++++++
> > > >  drivers/mfd/ocelot-spi.c                  | 325 ++++++++++++++++++++++
> > > >  drivers/mfd/ocelot.h                      |  36 +++
> > > 
> > > >  drivers/net/mdio/mdio-mscc-miim.c         |  21 +-
> > > >  drivers/pinctrl/pinctrl-microchip-sgpio.c |  22 +-
> > > >  drivers/pinctrl/pinctrl-ocelot.c          |  29 +-
> > > >  include/soc/mscc/ocelot.h                 |  11 +
> > > 
> > > Please avoid mixing subsystems in patches if at all avoidable.
> > > 
> > > If there are not build time dependencies/breakages, I'd suggest
> > > firstly applying support for this into MFD *then* utilising that
> > > support in subsequent patches.
> > 
> > My last RFC did this, and you had suggested to squash the commits. To
> > clarify, are you suggesting the MFD / Pinctrl get applied in a single
> > patch, then the MIIM get applied in a separate one? Because I had
> > started with what sounds like you're describing - an "empty" MFD with
> > subsequent patches rolling in each subsystem.
> > 
> > Perhaps I misinterpreted your initial feedback.
> 
> I want you to add all device support into the MFD driver at once.
> 
> The associated drivers, the ones that live in other subsystems, should
> be applied as separate patches.  There seldom exist any *build time*
> dependencies between the device side and the driver side.

The sub-devices are modified to use ocelot_get_regmap_from_resource. I
suppose I can add the inline stub function in drivers/mfd/ocelot.h,
which wouldn't break functionality. I'll do that in the next RFC.
Thanks for clarifying!

> 
> > > >  9 files changed, 614 insertions(+), 17 deletions(-)
> > > >  create mode 100644 drivers/mfd/ocelot-core.c
> > > >  create mode 100644 drivers/mfd/ocelot-spi.c
> > > >  create mode 100644 drivers/mfd/ocelot.h
> > > > 
> > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > index ba0b3eb131f1..57bbf2d11324 100644
> > > > --- a/drivers/mfd/Kconfig
> > > > +++ b/drivers/mfd/Kconfig
> > > > @@ -948,6 +948,25 @@ config MFD_MENF21BMC
> > > >  	  This driver can also be built as a module. If so the module
> > > >  	  will be called menf21bmc.
> > > >  
> > > > +config MFD_OCELOT
> > > > +	tristate "Microsemi Ocelot External Control Support"
> > > 
> > > Please explain exactly what an ECS is in the help below.
> > 
> > I thought I had by way of the second paragraph below. I'm trying to
> > think of what extra information could be of use at this point... 
> > 
> > I could describe how they have internal processors and using this level
> > of control would basically bypass that functionality.
> 
> Yes please.
> 
> Also provide details about what the device actually does.

Got it.

> 
> > > > +static struct regmap *ocelot_devm_regmap_init(struct ocelot_core *core,
> > > > +					      struct device *dev,
> > > > +					      const struct resource *res)
> > > > +{
> > > > +	struct regmap *regmap;
> > > > +
> > > > +	regmap = dev_get_regmap(dev, res->name);
> > > > +	if (!regmap)
> > > > +		regmap = ocelot_spi_devm_get_regmap(core, dev, res);
> > > 
> > > Why are you making SPI specific calls from the Core driver?
> > 
> > This was my interpretation of your initial feedback. It was initially
> > implemented as a config->get_regmap() function pointer so that core
> > didn't need to know anything about ocelot_spi.
> > 
> > If function pointers aren't used, it seems like core would have to know
> > about all possible bus types... Maybe my naming led to some
> > misunderstandings. Specifically I'd used "init_bus" which was intended
> > to be "set up the chip to be able to properly communicate via SPI" but
> > could have been interpreted as "tell the user of this driver that the
> > bus is being initialized by way of a callback"?
> 
> Okay, I see what's happening now.
> 
> Please add a comment to describe why you're calling one helper, what
> failure means in the first instance and what you hope to achieve by
> calling the subsequent one.

Will do.

> 
> > > > +	return regmap;
> > > > +}
> > > > +
> > > > +struct regmap *ocelot_get_regmap_from_resource(struct device *dev,
> > > > +					       const struct resource *res)
> > > > +{
> > > > +	struct ocelot_core *core = dev_get_drvdata(dev);
> > > > +
> > > > +	return ocelot_devm_regmap_init(core, dev, res);
> > > > +}
> > > > +EXPORT_SYMBOL(ocelot_get_regmap_from_resource);
> > > 
> > > Why don't you always call ocelot_devm_regmap_init() with the 'core'
> > > parameter dropped and just do dev_get_drvdata() inside of there?
> > > 
> > > You're passing 'dev' anyway.
> > 
> > This might be an error. I'll look into this, but I changed the intended
> > behavior of this between v5 and v6.
> > 
> > In v5 I had intended to attach all regmaps to the spi_device. This way
> > they could be shared amongst child devices of spi->dev. I think that was
> > a bad design decision on my part, so I abandoned it. If the child
> > devices are to share regmaps, they should explicitly do so by way of
> > syscon, not implicitly by name.
> > 
> > In v6 my intent is to have every regmap be devm-linked to the children.
> > This way the regmap would be destroyed and recreated by rmmod / insmod,
> > of the sub-modules, instead of being kept around the MFD module.
> 
> What's the reason for using an MFD to handle the Regmap(s) if you're
> going to have per-device ones anyway?  Why not handle them in the
> children?

Also addressing the suggestion below:

ocelot_core is the MFD "regmap-giver". It knows how to get a regmap from
Ocelot SPI and hand it to the child.

In order to do this, ocelot_core needs to know information about
ocelot-spi. As you pointed out, there's a cleaner way to do this without
jumping between core, spi, and dev. I agree.

However, the ocelot-spi priv data is tied to ocelot_core->dev. In order
to recover that information, ocelot-core needs to know about a child
device's "dev->parent".

So in v5, I had only used this "core->dev", which eventually burrowed
down into devm_regmap_init().

What this would mean to the user is if they ran "modprobe
ocelot-pinctrl; rmmod ocelot-pinctrl" there would be a debugfs interface
left at /sys/kernel/debug/regmap/spi0.0-gcb_gpio that was created for
ocelot-pinctrl, but abandoned.

I feel like that is incorrect behavior. "rmmod ocelot-pinctrl" should
destroy the regmap, since it is unused. In fact, it would probably break
upon subsequent "modprobe" commands, since it would try to register a
regmap of the same name but to a different device instance.

In order to achieve this, _two_ devices are required to pass around: the
"core->dev" (the same as child->parent) to get all information needed
about SPI, and the "child->dev" to get attached in devm_regmap_init().


As an example: ocelot_core needs a regmap for resetting the chip. It
would call:

ocelot_devm_regmap_init(dev, dev, res);

The first "dev" is used to get core information, the second is used to
in devm_*

A child device like ocelot-pinctrl would use something like:

ocelot_devm_regmap_init(dev->parent, dev, res);


This second "dev" argument wasn't in v5 because I believe I tried to
implicitly share regmaps. That would've led to the stale debugfs
reference I mentioned above, which I think is pretty undesireable.

> 
> > So perhaps to clear this up I should rename "dev" to "child" because it
> > seems that the naming has already gotten too confusing. What I intended
> > to do was:
> > 
> > struct regmap *ocelot_get_regmap_from_resource(struct device *parent,
> > 					       struct device *child,
> > 					       const struct resource *res)
> > {
> > 	struct ocelot_core *core = dev_get_drvdata(parent);
> > 
> > 	return ocelot_devm_regmap_init(core, child, res);
> > }
> > 
> > Or maybe even:
> > struct regmap *ocelot_get_regmap_from_resource(struct device *child,
> > 					       const struct resource *res)
> > {
> > 	struct ocelot_core *core = dev_get_drvdata(child->parent);
> > 
> > 	return ocelot_devm_regmap_init(core, child, res);
> > }
> 
> Or just call:
> 
>   ocelot_devm_regmap_init(core, dev->parent, res);
> 
> ... from the original call-site?
> 
> Or, as I previously suggested:
> 
>   ocelot_devm_regmap_init(dev->parent, res);
> 
> [...]
> 
> > > > +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, vsc7512_devs,
> > > 
> > > Why NONE?
> > 
> > I dont know the implication here. Example taken from
> > drivers/mfd/madera-core.c. I imagine PLATFORM_DEVID_AUTO is the correct
> > macro to use here?
> 
> That's why I asked.  Please read-up on the differences and use the
> correct one for your device instead of just blindly copy/pasting from
> other sources. :)

Will do! It is easy to miss these details when everything is new, so
thanks for pointing this out.

> 
> [...]
> 
> > > > +	WARN_ON(!val);
> > > 
> > > Is this possible?
> > 
> > Hmm... I don't know if regmap_read guards against val == NULL. It
> > doesn't look like it does. It is very much a "this should never happen"
> > moment...
> > 
> > I can remove it, or change this to return an error if !val, which is
> > what I probably should have done in the first place. Thoughts?
> 
> Not really.  Just make sure whatever you decide to do is informed.

Understood. I'll look more into it and verify.

> 
> [...]
> 
> > > > -	regs = devm_platform_ioremap_resource(pdev, 0);
> > > > -	if (IS_ERR(regs))
> > > > -		return PTR_ERR(regs);
> > > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > +
> > > > +	if (!device_is_mfd(pdev)) {
> > > > +		regs = devm_ioremap_resource(dev, res);
> > > 
> > > What happens if you call this if the device was registered via MFD?
> > 
> > I don't recall if it was your suggestion, but I tried this.
> > devm_ioremap_resource on the MFD triggered a kernel crash. I didn't look
> > much more into things than that, but if trying devm_ioremap_resource and
> > falling back to ocelot_get_regmap_from_resource is the desired path, I
> > can investigate further.
> 
> Yes please.  It should never crash.  That's probably a bug.

Can do.


And thanks again for the feedback! I do really appreciate it.

> 
> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog