Message-ID: <Yfji9Yy/VtrVv+Js@google.com>
Date:   Tue, 1 Feb 2022 07:36:21 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Colin Foster <colin.foster@...advantage.com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Russell King <linux@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>, UNGLinuxDriver@...rochip.com,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        katie.morris@...advantage.com
Subject: Re: [RFC v6 net-next 6/9] mfd: ocelot: add support for external mfd
 control over SPI for the VSC7512

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

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

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

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

> 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. :)

[...]

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

[...]

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

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

Powered by blists - more mailing lists