Re: [PATCH v13 net-next 9/9] mfd: ocelot: add support for the vsc7512 chip via spi

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

On Mon, Jul 18, 2022 at 03:18:28PM +0100, Lee Jones wrote:
> On Tue, 05 Jul 2022, Colin Foster wrote:
> 
> > +MODULE_IMPORT_NS(MFD_OCELOT_SPI);
> > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > new file mode 100644
> > index 000000000000..0c1c5215c706
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot-spi.c
> > @@ -0,0 +1,317 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * SPI core driver for the Ocelot chip family.
> > + *
> > + * This driver will handle everything necessary to allow for communication over
> > + * SPI to the VSC7511, VSC7512, VSC7513 and VSC7514 chips. The main functions
> > + * are to prepare the chip's SPI interface for a specific bus speed, and a host
> > + * processor's endianness. This will create and distribute regmaps for any
> > + * children.
> > + *
> > + * Copyright 2021, 2022 Innovative Advantage Inc.
> > + *
> > + * Author: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/ioport.h>
> > +#include <linux/kconfig.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <asm/byteorder.h>
> > +
> > +#include "ocelot.h"
> > +
> > +#define REG_DEV_CPUORG_IF_CTRL		0x0000
> > +#define REG_DEV_CPUORG_IF_CFGSTAT	0x0004
> > +
> > +#define CFGSTAT_IF_NUM_VCORE		(0 << 24)
> > +#define CFGSTAT_IF_NUM_VRAP		(1 << 24)
> > +#define CFGSTAT_IF_NUM_SI		(2 << 24)
> > +#define CFGSTAT_IF_NUM_MIIM		(3 << 24)
> > +
> > +#define VSC7512_DEVCPU_ORG_RES_START	0x71000000
> > +#define VSC7512_DEVCPU_ORG_RES_SIZE	0x38
> > +
> > +#define VSC7512_CHIP_REGS_RES_START	0x71070000
> > +#define VSC7512_CHIP_REGS_RES_SIZE	0x14
> > +
> > +struct spi_device;
> 
> Why not just #include?

I mis-understood this to mean drivers/mfd/ocelot-spi.c when it meant
drivers/mfd/ocelot.h. Thanks.

https://patchwork.kernel.org/project/netdevbpf/patch/20220701192609.3970317-10-colin.foster@xxxxxxxxxxxxxxxx/#24921057

"""
You missed a lot of forward declarations that are used in this file.

Like

struct spi_device;
"""


> > +static int ocelot_spi_regmap_bus_read(void *context,
> > +				      const void *reg, size_t reg_size,
> > +				      void *val, size_t val_size)
> > +{
> > +	struct ocelot_ddata *ddata = context;
> > +	struct spi_transfer tx, padding, rx;
> > +	struct spi_device *spi = ddata->spi;
> > +	struct spi_message msg;
> > +
> > +	spi = ddata->spi;
> 
> Drop this line.

Yes - and actually since I'm removing ddata->spi altogether it'll become
to_spi_device(ddata->dev)  (obviously without the double-assignment that
you're pointing out here)

> 
> > +	spi_message_init(&msg);
> > +
> > +	memset(&tx, 0, sizeof(tx));
> > +
> > +	tx.tx_buf = reg;
> > +	tx.len = reg_size;
> > +
> > +	spi_message_add_tail(&tx, &msg);
> > +
> > +	if (ddata->spi_padding_bytes) {
> > +		memset(&padding, 0, sizeof(padding));
> > +
> > +		padding.len = ddata->spi_padding_bytes;
> > +		padding.tx_buf = ddata->dummy_buf;
> > +		padding.dummy_data = 1;
> > +
> > +		spi_message_add_tail(&padding, &msg);
> > +	}
> > +
> > +	memset(&rx, 0, sizeof(rx));
> > +	rx.rx_buf = val;
> > +	rx.len = val_size;
> > +
> > +	spi_message_add_tail(&rx, &msg);
> > +
> > +	return spi_sync(spi, &msg);
> > +}
> > +
> > +static int ocelot_spi_regmap_bus_write(void *context, const void *data,
> > +				       size_t count)
> > +{
> > +	struct ocelot_ddata *ddata = context;
> > +	struct spi_device *spi = ddata->spi;

As above, I'm changing to to_spi_device(ddata->dev)

> > +
> > +	return spi_write(spi, data, count);
> > +}
> > +
> > +static const struct regmap_bus ocelot_spi_regmap_bus = {
> > +	.write = ocelot_spi_regmap_bus_write,
> > +	.read = ocelot_spi_regmap_bus_read,
> > +};
> > +
> > +struct regmap *
> > +ocelot_spi_init_regmap(struct device *dev, const struct resource *res)
> 
> One line, along with all the others.
> 
> > +{
> > +	struct ocelot_ddata *ddata = dev_get_drvdata(dev);
> > +	struct regmap_config regmap_config;
> > +
> > +	memcpy(&regmap_config, &ocelot_spi_regmap_config,
> > +	       sizeof(regmap_config));
> > +
> > +	regmap_config.name = res->name;
> > +	regmap_config.max_register = res->end - res->start;
> > +	regmap_config.reg_base = res->start;
> > +
> > +	return devm_regmap_init(dev, &ocelot_spi_regmap_bus, ddata,
> > +				&regmap_config);
> > +}
> > +EXPORT_SYMBOL_NS(ocelot_spi_init_regmap, MFD_OCELOT_SPI);
> > +
> > +static int ocelot_spi_probe(struct spi_device *spi)
> > +{
> > +	struct device *dev = &spi->dev;
> > +	struct ocelot_ddata *ddata;
> > +	struct regmap *r;
> > +	int err;
> > +
> > +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > +	if (!ddata)
> > +		return -ENOMEM;
> > +
> > +	ddata->dev = dev;
> 
> How are you fetching ddata if you don't already have 'dev'?

I don't think I fully understand this question...


Are you saying ddata doesn't need a dev instance? So instead of:

devm_regmap_init(dev, &bus, ddata, &regmap_config);

It could be:

devm_regmap_init(dev, &bus, dev, &regmap_config);


In that case, the context into ocelot_spi_regmap_bus_{read,write} would
be dev, instead of ddata.

Then I get ddata from device via:

static int ocelot_spi_regmap_bus_write(void *context,...)
{
    struct device *dev = context;
    struct ocelot_ddata *ddata = dev_get_drvdata(dev);
    struct spi_device *spi = to_spi_device(dev);

    /* ddata isn't actually needed for bus_write, just making a point */
    ...
}


I haven't tested this yet, but I think this is what you're suggesting.
So now I've removed both spi and dev from the ddata struct (as I mention
below). Cool.

> 
> > +	dev_set_drvdata(dev, ddata);
> 
> This should use the spi_* variant.

Agreed.

> 
> > +	if (spi->max_speed_hz <= 500000) {
> > +		ddata->spi_padding_bytes = 0;
> > +	} else {
> > +		/*
> > +		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG.
> > +		 * Register access time is 1us, so we need to configure and send
> > +		 * out enough padding bytes between the read request and data
> > +		 * transmission that lasts at least 1 microsecond.
> > +		 */
> > +		ddata->spi_padding_bytes = 1 +
> > +			(spi->max_speed_hz / 1000000 + 2) / 8;
> > +
> > +		ddata->dummy_buf = devm_kzalloc(dev, ddata->spi_padding_bytes,
> > +						GFP_KERNEL);
> > +		if (!ddata->dummy_buf)
> > +			return -ENOMEM;
> > +	}
> > +
> > +	ddata->spi = spi;
> 
> If you have 'spi' you definitely do not need 'dev'.
> 
> You can derive one from the other.

Good point. As I implied above, I'm dropping "spi" from the ddata struct
and will recover spi from to_spi_device(dev)

That does some nice things like removes "struct spi_device" from
drivers/mfd/ocelot.h

> > +	spi->bits_per_word = 8;
> > +
> > +	err = spi_setup(spi);
> > +	if (err < 0)
> > +		return dev_err_probe(&spi->dev, err,
> > +				     "Error performing SPI setup\n");
> > +
> > +	r = ocelot_spi_init_regmap(dev, &vsc7512_dev_cpuorg_resource);
> > +	if (IS_ERR(r))
> > +		return PTR_ERR(r);
> > +
> > +	ddata->cpuorg_regmap = r;
> > +
> > +	r = ocelot_spi_init_regmap(dev, &vsc7512_gcb_resource);
> > +	if (IS_ERR(r))
> > +		return PTR_ERR(r);
> > +
> > +	ddata->gcb_regmap = r;
> > +
> > +	/*
> > +	 * The chip must be set up for SPI before it gets initialized and reset.
> > +	 * This must be done before calling init, and after a chip reset is
> > +	 * performed.
> > +	 */
> > +	err = ocelot_spi_initialize(dev);
> > +	if (err)
> > +		return dev_err_probe(dev, err, "Error initializing SPI bus\n");
> > +
> > +	err = ocelot_chip_reset(dev);
> > +	if (err)
> > +		return dev_err_probe(dev, err, "Error resetting device\n");
> > +
> > +	/*
> > +	 * A chip reset will clear the SPI configuration, so it needs to be done
> > +	 * again before we can access any registers
> > +	 */
> > +	err = ocelot_spi_initialize(dev);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "Error initializing SPI bus after reset\n");
> > +
> > +	err = ocelot_core_init(dev);
> > +	if (err < 0)
> > +		return dev_err_probe(dev, err,
> > +				     "Error initializing Ocelot core\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct spi_device_id ocelot_spi_ids[] = {
> > +	{ "vsc7512", 0 },
> > +	{ }
> > +};
> > +
> > +static const struct of_device_id ocelot_spi_of_match[] = {
> > +	{ .compatible = "mscc,vsc7512" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ocelot_spi_of_match);
> > +
> > +static struct spi_driver ocelot_spi_driver = {
> > +	.driver = {
> > +		.name = "ocelot-soc",
> > +		.of_match_table = ocelot_spi_of_match,
> > +	},
> > +	.id_table = ocelot_spi_ids,
> > +	.probe = ocelot_spi_probe,
> > +};
> > +module_spi_driver(ocelot_spi_driver);
> > +
> > +MODULE_DESCRIPTION("SPI Controlled Ocelot Chip Driver");
> > +MODULE_AUTHOR("Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>");
> > +MODULE_LICENSE("Dual MIT/GPL");
> > +MODULE_IMPORT_NS(MFD_OCELOT);
> > diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
> > new file mode 100644
> > index 000000000000..c86bd6990a3c
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/* Copyright 2021, 2022 Innovative Advantage Inc. */
> > +
> > +#include <asm/byteorder.h>
> > +
> > +struct device;
> > +struct spi_device;
> > +struct regmap;
> > +struct resource;
> > +
> > +struct ocelot_ddata {
> > +	struct device *dev;
> > +	struct regmap *gcb_regmap;
> > +	struct regmap *cpuorg_regmap;
> > +	int spi_padding_bytes;
> > +	struct spi_device *spi;
> > +	void *dummy_buf;
> > +};
> 
> This looks like it deserves a doc header.

Will do!

> 
> > +int ocelot_chip_reset(struct device *dev);
> > +int ocelot_core_init(struct device *dev);
> > +
> > +/* SPI-specific routines that won't be necessary for other interfaces */
> > +struct regmap *ocelot_spi_init_regmap(struct device *dev,
> > +				      const struct resource *res);
> > +
> > +#define OCELOT_SPI_BYTE_ORDER_LE 0x00000000
> > +#define OCELOT_SPI_BYTE_ORDER_BE 0x81818181
> > +
> > +#ifdef __LITTLE_ENDIAN
> > +#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_LE
> > +#else
> > +#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_BE
> > +#endif
> 
> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog