Re: [RFC v1] mfd: pinctrl: RFC only: add and utilze mfd option in pinctrl-ocelot

[Colin Foster]

On Mon, Dec 06, 2021 at 08:55:25AM +0000, Lee Jones wrote:
> On Fri, 03 Dec 2021, Colin Foster wrote:
> 
> > This is a psuedo-commit, but one that tells the complete story of what I'm
> > looking at. During an actual submission this'll be broken up into two
> > commits, but I'd like to get some feedback on whether this is the correct
> > path for me to be going down.
> > 
> > Background:
> > 
> > Microchip has a family of chips - the VSC7511, 7512, 7513, and 7514. The
> > last two have an internal MIPS processor, which are supported by
> > drivers/net/ethernet/mscc/ocelot_*. The former two lack this processor.
> > 
> > All four chips can be configured externally via a number of interfaces:
> > SPI, I2C, PCIe... This is currently not supported and is my end goal.
> > 
> > The networking portion of these chips have been reused in other products as
> > well. These utilize the common code by way of mscc_ocelot_switch_lib and
> > net/dsa/ocelot/*. Specifically the "Felix" driver.
> > 
> > Current status:
> > 
> > I've put out a few RFCs on the "ocelot_spi" driver. It utilizes Felix and
> > invokes much of the network portion of the hardware (VSC7512). It works
> > great! Thanks community :)
> > 
> > There's more hardware that needs to get configured, however. Currently that
> > includes general pin configuration, and an optional serial GPIO expander.
> > The former is supported by drivers/pinctrl/pinctrl-ocelot.c and the latter
> > by drivers/pinctrl/pinctrl-microchip-sgpio.c.
> > 
> > These drivers have been updated to use regmap instead of iomem, but that
> > isn't the complete story. There are two options I know about, and maybe
> > others I don't.
> > 
> > Option 1 - directly hook into the driver:
> > 
> > This was the path that was done in
> > commit b99658452355 ("net: dsa: ocelot: felix: utilize shared mscc-miim
> > driver for indirect MDIO access").
> > This is in the net-next tree. In this case the Seville driver passes in its
> > regmap to the mscc_miim_setup function, which bypasses mscc_miim_probe but
> > allows the same driver to be used.
> > 
> > This was my initial implementation to hook into pinctrl-ocelot and
> > pinctrl-microchip-sgpio. The good thing about this implementation is I have
> > direct control over the order of things happening. For instance, pinctrl
> > might need to be configured before the MDIO bus gets probed.
> > 
> > The bad thing about this implementation is... it doesn't work yet. My
> > memory is fuzzy on this, but I recall noticing that the application of a
> > devicetree pinctrl function happens in the driver probe. I ventured down
> > this path of walking the devicetree, applying pincfg, etc. That was a path
> > to darkness that I have abandoned.
> > 
> > What is functioning is I have debugfs / sysfs control, so I do have the
> > ability to do some runtime testing and verification.
> > 
> > Option 2 - MFD (this "patch")
> > 
> > It really seems like anything in drivers/net/dsa/ should avoid
> > drivers/pinctl, and that MFD is the answer. This adds some complexity to
> > pinctrl-ocelot, and I'm not sure whether that breaks the concept of MFD. So
> > it seems like I'm either doing something unique, or I'm doing something
> > wrong.
> > 
> > I err on the assumption that I'm doing something wrong.
> > 
> > pinctrl-ocelot gets its resources the device tree by way of
> > platform_get_and_ioremap_resource. This driver has been updated to support
> > regmap in the pinctrl tree:
> > commit 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
> > 
> > The problem comes about when this driver is probed by way of
> > "mfd_add_devices". In an ideal world it seems like this would be handled by
> > resources. MFD adds resources to the device before it gets probed. The
> > probe happens and the driver is happy because platform_get_resource
> > succeeds.
> > 
> > In this scenario the device gets probed, but needs to know how to get its
> > regmap... not its resource. In the "I'm running from an internal chip"
> > scenario, the existing process of "devm_regmap_init_mmio" will suffice. But
> > in the "I'm running from an externally controlled setup via {SPI,I2C,PCIe}"
> > the process needs to be "get me this regmap from my parent". It seems like
> > dev_get_regmap is a perfect candidate for this.
> > 
> > Perhaps there is something I'm missing in the concept of resources /
> > regmaps. But it seems like pinctrl-ocelot needs to know whether it is in an
> > MFD scenario, and that concept didn't exist. Hence the addition of
> > device_is_mfd as part of this patch. Since "device_is_mfd" didn't exist, it
> > feels like I might be breaking the concept of MFD.
> > 
> > I think this would lend itself to a pretty elegant architecture for the
> > VSC751X externally controlled chips. In a manner similar to
> > drivers/mfd/madera* there would be small drivers handling the prococol
> > layers for SPI, I2C... A core driver would handle the register mappings,
> > and could be gotten by dev_get_regmap. Every sub-device (DSA, pinctrl,
> > other pinctrl, other things I haven't considered yet) could either rely on
> > dev->parent directly, or in this case adjust. I can't imagine a scenario
> > where someone would want pinctrl for the VSC7512 without the DSA side of
> > things... but that would be possible in this architecture that would
> > otherwise not.
> > 
> > Signed-off-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/mfd/mfd-core.c           | 6 ++++++
> >  drivers/pinctrl/pinctrl-ocelot.c | 7 ++++++-
> >  include/linux/mfd/core.h         | 2 ++
> >  3 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > index 684a011a6396..2ba6a692499b 100644
> > --- a/drivers/mfd/mfd-core.c
> > +++ b/drivers/mfd/mfd-core.c
> > @@ -33,6 +33,12 @@ static struct device_type mfd_dev_type = {
> >  	.name	= "mfd_device",
> >  };
> >  
> > +int device_is_mfd(struct platform_device *pdev)
> > +{
> > +	return (!strcmp(pdev->dev.type->name, mfd_dev_type.name));
> > +}
> > +EXPORT_SYMBOL(device_is_mfd);
> > +
> >  int mfd_cell_enable(struct platform_device *pdev)
> >  {
> >  	const struct mfd_cell *cell = mfd_get_cell(pdev);
> > diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
> > index 0a36ec8775a3..758fbc225244 100644
> > --- a/drivers/pinctrl/pinctrl-ocelot.c
> > +++ b/drivers/pinctrl/pinctrl-ocelot.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/gpio/driver.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > +#include <linux/mfd/core.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> > @@ -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, &regmap_config);
> > +	if (device_is_mfd(pdev))
> > +		info->map = dev_get_regmap(dev->parent, "GCB");
> > +	else
> > +		info->map = devm_regmap_init_mmio(dev, base, &regmap_config);
> 
> What happens if you were to call the wrong API in either scenario?
> 
> If the answer is 'the call would fail', then why not call one and if
> it fails, call the other?  With provided commits describing the
> reason for the stacked calls of course.

Hi Lee, 

As I said in the other thread, sorry for missing this response. My email
blocked it.

That's a good idea. I'll keep this in mind when the final
"get_mfd_regamp" implementation shakes out.

Thanks for the feedback!

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