Date:   Mon, 6 Dec 2021 08:55:25 +0000
From:   Lee Jones <lee.jones@linaro.org>
To:     Colin Foster <colin.foster@in-advantage.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-gpio@vger.kernel.org,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        katie.morris@in-advantage.com
Subject: Re: [RFC v1] mfd: pinctrl: RFC only: add and utilze mfd option in
 pinctrl-ocelot
Message-ID: <Ya3P/Z3jbMpV1Fso@google.com>
References: <20211203211611.946658-1-colin.foster@in-advantage.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211203211611.946658-1-colin.foster@in-advantage.com>
Precedence: bulk

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

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