Hello Lee,

On Tue, Feb 01, 2022 at 07:36:21AM +0000, Lee Jones wrote:

On Mon, 31 Jan 2022, Colin Foster wrote:

quoted

Hi Lee,

Thank you very much for your time / feedback.

On Mon, Jan 31, 2022 at 09:29:34AM +0000, Lee Jones wrote:

quoted

On Sat, 29 Jan 2022, Colin Foster wrote:

quoted

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@in-advantage.com>
---
 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 +++

quoted

 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!

quoted

quoted

quoted

 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.

quoted

quoted

quoted

+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.

quoted

quoted

quoted

+	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.

quoted

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

[...]

quoted

quoted

quoted

+	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.

[...]

quoted

quoted

quoted

+	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.

[...]

quoted

quoted

quoted

-	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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel