| Message ID | 20230804104602.395892-6-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ef75e767167a8f30c7690bc4689dba76329ee06e |
| Headers | show |
| Series | Add cs42l43 PC focused SoundWire CODEC | expand |
Fri, Aug 04, 2023 at 11:46:01AM +0100, Charles Keepax kirjoitti: > From: Lucas Tanure <tanureal@opensource.cirrus.com> > > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed > for portable applications. It provides a high dynamic range, stereo > DAC for headphone output, two integrated Class D amplifiers for > loudspeakers, and two ADCs for wired headset microphone input or > stereo line input. PDM inputs are provided for digital microphones. > > The SPI component incorporates a SPI controller interface for > communication with other peripheral components. ... > + while (buf < block) { > + const u8 *word = min(buf + sizeof(u32), block); > + int pad = (buf + sizeof(u32)) - word; > + > + while (buf < word) { > + val >>= BITS_PER_BYTE; > + val |= FIELD_PREP(GENMASK(31, 24), *buf); > + > + buf++; > + } Is this a reinvented way of get_unaligned_*() APIs? > + val >>= pad * BITS_PER_BYTE; > + > + regmap_write(regmap, CS42L43_TX_DATA, val); > + } ... > + while (buf < word) { > + *buf = FIELD_GET(GENMASK(7, 0), val); > + > + val >>= BITS_PER_BYTE; > + buf++; > + } put_unaligned_*() ? ... > + /* select another internal CS, which doesn't exist, so CS 0 is not used */ > + if (spi_get_csgpiod(spi, 0)) > + spi_config1 |= 1 << CS42L43_SPI_SS_SEL_SHIFT; BIT() ? > + if (spi->mode & SPI_CPOL) > + spi_config1 |= CS42L43_SPI_CPOL_MASK; > + if (spi->mode & SPI_CPHA) > + spi_config1 |= CS42L43_SPI_CPHA_MASK; > + if (spi->mode & SPI_3WIRE) > + spi_config1 |= CS42L43_SPI_THREE_WIRE_MASK; ... > + if (is_of_node(fwnode)) > + fwnode = fwnode_get_named_child_node(fwnode, "spi"); You can actually drop these is_of_node() tests and use another variable. In ACPI there can't be child node in small letters. But main problem here (and in another driver where the similar is used) that you bumped reference count for fwnode. I haven't seen where you drop it back. Have you tested rmmod/modprobe in a loop? ... > + devm_pm_runtime_enable(priv->dev); No error check? Why? ... > + ret = devm_spi_register_controller(priv->dev, priv->ctlr); > + if (ret) { > + pm_runtime_disable(priv->dev); Ah! Are you sure you properly simulated faults when testing this code? > + dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret); > + } > + > + return ret;
On Thu, Jan 18, 2024 at 07:06:13PM +0200, andy.shevchenko@gmail.com wrote: > Fri, Aug 04, 2023 at 11:46:01AM +0100, Charles Keepax kirjoitti: > > + while (buf < block) { > > + const u8 *word = min(buf + sizeof(u32), block); > > + int pad = (buf + sizeof(u32)) - word; > > + > > + while (buf < word) { > > + val >>= BITS_PER_BYTE; > > + val |= FIELD_PREP(GENMASK(31, 24), *buf); > > + > > + buf++; > > + } > > Is this a reinvented way of get_unaligned_*() APIs? > > > + val >>= pad * BITS_PER_BYTE; > > + > > + regmap_write(regmap, CS42L43_TX_DATA, val); > > + } > > ... > > > + while (buf < word) { > > + *buf = FIELD_GET(GENMASK(7, 0), val); > > + > > + val >>= BITS_PER_BYTE; > > + buf++; > > + } > > put_unaligned_*() ? > Alas as it has been a while I have forgetten the exact context here and this one will take a little more time. I will try to find some spare time to work out if that would actual do the same thing, I have a vague feeling there was something here. > ... > > > + if (is_of_node(fwnode)) > > + fwnode = fwnode_get_named_child_node(fwnode, "spi"); > > You can actually drop these is_of_node() tests and use another variable. In > ACPI there can't be child node in small letters. > is_of_node feels pretty clear what the intent is, rather than relying on nodes not existing etc. > But main problem here (and in another driver where the similar is used) that > you bumped reference count for fwnode. I haven't seen where you drop it back. > Have you tested rmmod/modprobe in a loop? > Yeah it should drop the reference will add that. > > + devm_pm_runtime_enable(priv->dev); > > No error check? Why? Happy to add one. > > + ret = devm_spi_register_controller(priv->dev, priv->ctlr); > > + if (ret) { > > + pm_runtime_disable(priv->dev); > > Ah! Are you sure you properly simulated faults when testing this code? This one has already been fixed. Thanks, Charles
On Fri, Jan 19, 2024 at 1:49 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > On Thu, Jan 18, 2024 at 07:06:13PM +0200, andy.shevchenko@gmail.com wrote: ... > > > + if (is_of_node(fwnode)) > > > + fwnode = fwnode_get_named_child_node(fwnode, "spi"); > > > > You can actually drop these is_of_node() tests and use another variable. In > > ACPI there can't be child node in small letters. > > is_of_node feels pretty clear what the intent is, rather than > relying on nodes not existing etc. > > > But main problem here (and in another driver where the similar is used) that > > you bumped reference count for fwnode. I haven't seen where you drop it back. > > Have you tested rmmod/modprobe in a loop? > > Yeah it should drop the reference will add that. Note, this will require an additional variable anyway (as in the infamous `x = realloc(x...)` mistake).
On Fri, Jan 19, 2024 at 11:49:17AM +0000, Charles Keepax wrote: > On Thu, Jan 18, 2024 at 07:06:13PM +0200, andy.shevchenko@gmail.com wrote: > > Fri, Aug 04, 2023 at 11:46:01AM +0100, Charles Keepax kirjoitti: > > > + while (buf < block) { > > > + const u8 *word = min(buf + sizeof(u32), block); > > > + int pad = (buf + sizeof(u32)) - word; > > > + > > > + while (buf < word) { > > > + val >>= BITS_PER_BYTE; > > > + val |= FIELD_PREP(GENMASK(31, 24), *buf); > > > + > > > + buf++; > > > + } > > > > Is this a reinvented way of get_unaligned_*() APIs? > > > > > + val >>= pad * BITS_PER_BYTE; > > > + > > > + regmap_write(regmap, CS42L43_TX_DATA, val); > > > + } > > > > ... > > > > > + while (buf < word) { > > > + *buf = FIELD_GET(GENMASK(7, 0), val); > > > + > > > + val >>= BITS_PER_BYTE; > > > + buf++; > > > + } > > > > put_unaligned_*() ? > > > > Alas as it has been a while I have forgetten the exact context > here and this one will take a little more time. I will try to > find some spare time to work out if that would actual do the same > thing, I have a vague feeling there was something here. > Ok found some time to refresh my memory on this. The main issue here was as this is processing raw data for the SPI we shouldn't assume the data is a multiple of 4-bytes. You could write the code such that you used put_unaligned_le32 for most of the data and then special case the remainder, which would probably be slightly faster. But for the remainder you either end with the code here anyway or a special case for each of the cases 8,16,24 bits. So the code ends up looking much messier. Personally I am inclined to leave this unless performance on the SPI becomes a major issue. There is perhaps an argument for a comment in the code to explain this given it took me time to remember what was going on. Thanks, Charles
On Wed, Jan 31, 2024 at 5:41 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > On Fri, Jan 19, 2024 at 11:49:17AM +0000, Charles Keepax wrote: > > On Thu, Jan 18, 2024 at 07:06:13PM +0200, andy.shevchenko@gmail.com wrote: > > > Fri, Aug 04, 2023 at 11:46:01AM +0100, Charles Keepax kirjoitti: > > > > + while (buf < block) { > > > > + const u8 *word = min(buf + sizeof(u32), block); > > > > + int pad = (buf + sizeof(u32)) - word; > > > > + > > > > + while (buf < word) { > > > > + val >>= BITS_PER_BYTE; > > > > + val |= FIELD_PREP(GENMASK(31, 24), *buf); > > > > + > > > > + buf++; > > > > + } > > > > > > Is this a reinvented way of get_unaligned_*() APIs? > > > > > > > + val >>= pad * BITS_PER_BYTE; > > > > + > > > > + regmap_write(regmap, CS42L43_TX_DATA, val); > > > > + } > > > > > > ... > > > > > > > + while (buf < word) { > > > > + *buf = FIELD_GET(GENMASK(7, 0), val); > > > > + > > > > + val >>= BITS_PER_BYTE; > > > > + buf++; > > > > + } > > > > > > put_unaligned_*() ? > > > > > > > Alas as it has been a while I have forgetten the exact context > > here and this one will take a little more time. I will try to > > find some spare time to work out if that would actual do the same > > thing, I have a vague feeling there was something here. > > > > Ok found some time to refresh my memory on this. > > The main issue here was as this is processing raw data for the > SPI we shouldn't assume the data is a multiple of 4-bytes. You > could write the code such that you used put_unaligned_le32 for > most of the data and then special case the remainder, which would > probably be slightly faster. But for the remainder you either end > with the code here anyway or a special case for each of the cases > 8,16,24 bits. So the code ends up looking much messier. > Personally I am inclined to leave this unless performance on the > SPI becomes a major issue. Yes, the problem with the code is that it is a NIH existing API or patterns. We have already in the IIO subsystem a pattern where there is a switch case and put/get unaligned APIs per case. Perhaps this is what needs to be factored out for everybody. https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad4130.c#L472 (some shorter variants) https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ltc2497.c#L66 https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad4130.c#L472 Here is the abstraction for cameras, perhaps that's what ASoC might need. drivers/media/v4l2-core/v4l2-cci.c. > There is perhaps an argument for a comment in the code to explain > this given it took me time to remember what was going on. That's for sure.
diff --git a/MAINTAINERS b/MAINTAINERS index 4e35a4880cf84..5a581e846537d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4881,6 +4881,7 @@ S: Maintained F: Documentation/devicetree/bindings/sound/cirrus,cs* F: drivers/mfd/cs42l43* F: drivers/pinctrl/cirrus/pinctrl-cs42l43* +F: drivers/spi/spi-cs42l43* F: include/dt-bindings/sound/cs* F: include/linux/mfd/cs42l43* F: include/sound/cs* diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 8962b25576156..083f71cf4829f 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -281,6 +281,13 @@ config SPI_COLDFIRE_QSPI This enables support for the Coldfire QSPI controller in master mode. +config SPI_CS42L43 + tristate "Cirrus Logic CS42L43 SPI controller" + depends on MFD_CS42L43 && PINCTRL_CS42L43 + help + This enables support for the SPI controller inside the Cirrus Logic + CS42L43 audio codec. + config SPI_DAVINCI tristate "Texas Instruments DaVinci/DA8x/OMAP-L/AM1x SoC SPI controller" depends on ARCH_DAVINCI || ARCH_KEYSTONE || COMPILE_TEST diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 080c2c1b3ec19..c537640bed701 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -40,6 +40,7 @@ obj-$(CONFIG_SPI_CADENCE_QUADSPI) += spi-cadence-quadspi.o obj-$(CONFIG_SPI_CADENCE_XSPI) += spi-cadence-xspi.o obj-$(CONFIG_SPI_CLPS711X) += spi-clps711x.o obj-$(CONFIG_SPI_COLDFIRE_QSPI) += spi-coldfire-qspi.o +obj-$(CONFIG_SPI_CS42L43) += spi-cs42l43.o obj-$(CONFIG_SPI_DAVINCI) += spi-davinci.o obj-$(CONFIG_SPI_DLN2) += spi-dln2.o obj-$(CONFIG_SPI_DESIGNWARE) += spi-dw.o diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c new file mode 100644 index 0000000000000..453a9b37ce785 --- /dev/null +++ b/drivers/spi/spi-cs42l43.c @@ -0,0 +1,284 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// CS42L43 SPI Controller Driver +// +// Copyright (C) 2022-2023 Cirrus Logic, Inc. and +// Cirrus Logic International Semiconductor Ltd. + +#include <linux/bits.h> +#include <linux/bitfield.h> +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/mfd/cs42l43.h> +#include <linux/mfd/cs42l43-regs.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/spi/spi.h> +#include <linux/units.h> + +#define CS42L43_FIFO_SIZE 16 +#define CS42L43_SPI_ROOT_HZ (40 * HZ_PER_MHZ) +#define CS42L43_SPI_MAX_LENGTH 65532 + +enum cs42l43_spi_cmd { + CS42L43_WRITE, + CS42L43_READ +}; + +struct cs42l43_spi { + struct device *dev; + struct regmap *regmap; + struct spi_controller *ctlr; +}; + +static const unsigned int cs42l43_clock_divs[] = { + 2, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30 +}; + +static int cs42l43_spi_tx(struct regmap *regmap, const u8 *buf, unsigned int len) +{ + const u8 *end = buf + len; + u32 val = 0; + int ret; + + while (buf < end) { + const u8 *block = min(buf + CS42L43_FIFO_SIZE, end); + + while (buf < block) { + const u8 *word = min(buf + sizeof(u32), block); + int pad = (buf + sizeof(u32)) - word; + + while (buf < word) { + val >>= BITS_PER_BYTE; + val |= FIELD_PREP(GENMASK(31, 24), *buf); + + buf++; + } + + val >>= pad * BITS_PER_BYTE; + + regmap_write(regmap, CS42L43_TX_DATA, val); + } + + regmap_write(regmap, CS42L43_TRAN_CONFIG8, CS42L43_SPI_TX_DONE_MASK); + + ret = regmap_read_poll_timeout(regmap, CS42L43_TRAN_STATUS1, + val, (val & CS42L43_SPI_TX_REQUEST_MASK), + 1000, 5000); + if (ret) + return ret; + } + + return 0; +} + +static int cs42l43_spi_rx(struct regmap *regmap, u8 *buf, unsigned int len) +{ + u8 *end = buf + len; + u32 val; + int ret; + + while (buf < end) { + u8 *block = min(buf + CS42L43_FIFO_SIZE, end); + + ret = regmap_read_poll_timeout(regmap, CS42L43_TRAN_STATUS1, + val, (val & CS42L43_SPI_RX_REQUEST_MASK), + 1000, 5000); + if (ret) + return ret; + + while (buf < block) { + u8 *word = min(buf + sizeof(u32), block); + + ret = regmap_read(regmap, CS42L43_RX_DATA, &val); + if (ret) + return ret; + + while (buf < word) { + *buf = FIELD_GET(GENMASK(7, 0), val); + + val >>= BITS_PER_BYTE; + buf++; + } + } + + regmap_write(regmap, CS42L43_TRAN_CONFIG8, CS42L43_SPI_RX_DONE_MASK); + } + + return 0; +} + +static int cs42l43_transfer_one(struct spi_controller *ctlr, struct spi_device *spi, + struct spi_transfer *tfr) +{ + struct cs42l43_spi *priv = spi_controller_get_devdata(spi->controller); + int i, ret = -EINVAL; + + for (i = 0; i < ARRAY_SIZE(cs42l43_clock_divs); i++) { + if (CS42L43_SPI_ROOT_HZ / cs42l43_clock_divs[i] <= tfr->speed_hz) + break; + } + + if (i == ARRAY_SIZE(cs42l43_clock_divs)) + return -EINVAL; + + regmap_write(priv->regmap, CS42L43_SPI_CLK_CONFIG1, i); + + if (tfr->tx_buf) { + regmap_write(priv->regmap, CS42L43_TRAN_CONFIG3, CS42L43_WRITE); + regmap_write(priv->regmap, CS42L43_TRAN_CONFIG4, tfr->len - 1); + } else if (tfr->rx_buf) { + regmap_write(priv->regmap, CS42L43_TRAN_CONFIG3, CS42L43_READ); + regmap_write(priv->regmap, CS42L43_TRAN_CONFIG5, tfr->len - 1); + } + + regmap_write(priv->regmap, CS42L43_TRAN_CONFIG1, CS42L43_SPI_START_MASK); + + if (tfr->tx_buf) + ret = cs42l43_spi_tx(priv->regmap, (const u8 *)tfr->tx_buf, tfr->len); + else if (tfr->rx_buf) + ret = cs42l43_spi_rx(priv->regmap, (u8 *)tfr->rx_buf, tfr->len); + + return ret; +} + +static void cs42l43_set_cs(struct spi_device *spi, bool is_high) +{ + struct cs42l43_spi *priv = spi_controller_get_devdata(spi->controller); + + if (spi_get_chipselect(spi, 0) == 0) + regmap_write(priv->regmap, CS42L43_SPI_CONFIG2, !is_high); +} + +static int cs42l43_prepare_message(struct spi_controller *ctlr, struct spi_message *msg) +{ + struct cs42l43_spi *priv = spi_controller_get_devdata(ctlr); + struct spi_device *spi = msg->spi; + unsigned int spi_config1 = 0; + + /* select another internal CS, which doesn't exist, so CS 0 is not used */ + if (spi_get_csgpiod(spi, 0)) + spi_config1 |= 1 << CS42L43_SPI_SS_SEL_SHIFT; + if (spi->mode & SPI_CPOL) + spi_config1 |= CS42L43_SPI_CPOL_MASK; + if (spi->mode & SPI_CPHA) + spi_config1 |= CS42L43_SPI_CPHA_MASK; + if (spi->mode & SPI_3WIRE) + spi_config1 |= CS42L43_SPI_THREE_WIRE_MASK; + + regmap_write(priv->regmap, CS42L43_SPI_CONFIG1, spi_config1); + + return 0; +} + +static int cs42l43_prepare_transfer_hardware(struct spi_controller *ctlr) +{ + struct cs42l43_spi *priv = spi_controller_get_devdata(ctlr); + int ret; + + ret = regmap_write(priv->regmap, CS42L43_BLOCK_EN2, CS42L43_SPI_MSTR_EN_MASK); + if (ret) + dev_err(priv->dev, "Failed to enable SPI controller: %d\n", ret); + + return ret; +} + +static int cs42l43_unprepare_transfer_hardware(struct spi_controller *ctlr) +{ + struct cs42l43_spi *priv = spi_controller_get_devdata(ctlr); + int ret; + + ret = regmap_write(priv->regmap, CS42L43_BLOCK_EN2, 0); + if (ret) + dev_err(priv->dev, "Failed to disable SPI controller: %d\n", ret); + + return ret; +} + +static size_t cs42l43_spi_max_length(struct spi_device *spi) +{ + return CS42L43_SPI_MAX_LENGTH; +} + +static int cs42l43_spi_probe(struct platform_device *pdev) +{ + struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent); + struct cs42l43_spi *priv; + struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev); + int ret; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(*priv->ctlr)); + if (!priv->ctlr) + return -ENOMEM; + + spi_controller_set_devdata(priv->ctlr, priv); + + priv->dev = &pdev->dev; + priv->regmap = cs42l43->regmap; + + priv->ctlr->prepare_message = cs42l43_prepare_message; + priv->ctlr->prepare_transfer_hardware = cs42l43_prepare_transfer_hardware; + priv->ctlr->unprepare_transfer_hardware = cs42l43_unprepare_transfer_hardware; + priv->ctlr->transfer_one = cs42l43_transfer_one; + priv->ctlr->set_cs = cs42l43_set_cs; + priv->ctlr->max_transfer_size = cs42l43_spi_max_length; + + if (is_of_node(fwnode)) + fwnode = fwnode_get_named_child_node(fwnode, "spi"); + + device_set_node(&priv->ctlr->dev, fwnode); + + priv->ctlr->mode_bits = SPI_3WIRE | SPI_MODE_X_MASK; + priv->ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX; + priv->ctlr->bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16) | + SPI_BPW_MASK(32); + priv->ctlr->min_speed_hz = CS42L43_SPI_ROOT_HZ / + cs42l43_clock_divs[ARRAY_SIZE(cs42l43_clock_divs) - 1]; + priv->ctlr->max_speed_hz = CS42L43_SPI_ROOT_HZ / cs42l43_clock_divs[0]; + priv->ctlr->use_gpio_descriptors = true; + priv->ctlr->auto_runtime_pm = true; + + devm_pm_runtime_enable(priv->dev); + pm_runtime_idle(priv->dev); + + regmap_write(priv->regmap, CS42L43_TRAN_CONFIG6, CS42L43_FIFO_SIZE - 1); + regmap_write(priv->regmap, CS42L43_TRAN_CONFIG7, CS42L43_FIFO_SIZE - 1); + + // Disable Watchdog timer and enable stall + regmap_write(priv->regmap, CS42L43_SPI_CONFIG3, 0); + regmap_write(priv->regmap, CS42L43_SPI_CONFIG4, CS42L43_SPI_STALL_ENA_MASK); + + ret = devm_spi_register_controller(priv->dev, priv->ctlr); + if (ret) { + pm_runtime_disable(priv->dev); + dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret); + } + + return ret; +} + +static const struct platform_device_id cs42l43_spi_id_table[] = { + { "cs42l43-spi", }, + {} +}; +MODULE_DEVICE_TABLE(platform, cs42l43_spi_id_table); + +static struct platform_driver cs42l43_spi_driver = { + .driver = { + .name = "cs42l43-spi", + }, + .probe = cs42l43_spi_probe, + .id_table = cs42l43_spi_id_table, +}; +module_platform_driver(cs42l43_spi_driver); + +MODULE_DESCRIPTION("CS42L43 SPI Driver"); +MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>"); +MODULE_AUTHOR("Maciej Strozek <mstrozek@opensource.cirrus.com>"); +MODULE_LICENSE("GPL");