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(®map_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, > > + ®map_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, ®map_config); It could be: devm_regmap_init(dev, &bus, dev, ®map_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
