mbox series

[v2,0/6] Add cs42l43 PC focused SoundWire CODEC

Message ID 20230530122112.1314458-1-ckeepax@opensource.cirrus.com
Headers show
Series Add cs42l43 PC focused SoundWire CODEC | expand

Message

Charles Keepax May 30, 2023, 12:21 p.m. UTC
This patch chain adds support for the Cirrus Logic cs42l43 PC focused
SoundWire CODEC. The chain is currently based of Lee's for-mfd-next
branch.

Change notes are included with each patch, most of the changes are
trivial, the notable ones are moving the IRQs out of irqchip and into
the MFD, and moving the DT binding to sound.

Thanks,
Charles

Charles Keepax (4):
  dt-bindings: mfd: cirrus,cs42l43: Add initial DT binding
  mfd: cs42l43: Add support for cs42l43 core driver
  pinctrl: cs42l43: Add support for the cs42l43
  ASoC: cs42l43: Add support for the cs42l43

Lucas Tanure (2):
  soundwire: bus: Allow SoundWire peripherals to register IRQ handlers
  spi: cs42l43: Add SPI controller support

 .../bindings/sound/cirrus,cs42l43.yaml        |  320 +++
 MAINTAINERS                                   |    5 +
 drivers/mfd/Kconfig                           |   23 +
 drivers/mfd/Makefile                          |    3 +
 drivers/mfd/cs42l43-i2c.c                     |   86 +
 drivers/mfd/cs42l43-sdw.c                     |  213 ++
 drivers/mfd/cs42l43.c                         | 1141 +++++++++
 drivers/mfd/cs42l43.h                         |   23 +
 drivers/pinctrl/cirrus/Kconfig                |   11 +
 drivers/pinctrl/cirrus/Makefile               |    2 +
 drivers/pinctrl/cirrus/pinctrl-cs42l43.c      |  609 +++++
 drivers/soundwire/bus.c                       |   31 +
 drivers/soundwire/bus_type.c                  |   12 +
 drivers/spi/Kconfig                           |    7 +
 drivers/spi/Makefile                          |    1 +
 drivers/spi/spi-cs42l43.c                     |  279 ++
 include/linux/mfd/cs42l43-regs.h              | 1172 +++++++++
 include/linux/mfd/cs42l43.h                   |  102 +
 include/linux/soundwire/sdw.h                 |    9 +
 include/sound/cs42l43.h                       |   17 +
 sound/soc/codecs/Kconfig                      |   16 +
 sound/soc/codecs/Makefile                     |    4 +
 sound/soc/codecs/cs42l43-jack.c               |  951 +++++++
 sound/soc/codecs/cs42l43-sdw.c                |   75 +
 sound/soc/codecs/cs42l43.c                    | 2275 +++++++++++++++++
 sound/soc/codecs/cs42l43.h                    |  126 +
 26 files changed, 7513 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/cirrus,cs42l43.yaml
 create mode 100644 drivers/mfd/cs42l43-i2c.c
 create mode 100644 drivers/mfd/cs42l43-sdw.c
 create mode 100644 drivers/mfd/cs42l43.c
 create mode 100644 drivers/mfd/cs42l43.h
 create mode 100644 drivers/pinctrl/cirrus/pinctrl-cs42l43.c
 create mode 100644 drivers/spi/spi-cs42l43.c
 create mode 100644 include/linux/mfd/cs42l43-regs.h
 create mode 100644 include/linux/mfd/cs42l43.h
 create mode 100644 include/sound/cs42l43.h
 create mode 100644 sound/soc/codecs/cs42l43-jack.c
 create mode 100644 sound/soc/codecs/cs42l43-sdw.c
 create mode 100644 sound/soc/codecs/cs42l43.c
 create mode 100644 sound/soc/codecs/cs42l43.h

Comments

Mark Brown May 30, 2023, 9:30 p.m. UTC | #1
On Tue, May 30, 2023 at 01:21:11PM +0100, Charles Keepax wrote:

A couple of small things:

> +static unsigned int cs42l43_clock_divs[16] = {
> +	2, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30
> +};

Do we need to specify the size of the array?  I just had to count the
number of initialisers :(   Should probably also be const.

> +		for (; buf < block - (sizeof(u32) - 1); buf += sizeof(u32))
> +			regmap_write(regmap, CS42L43_TX_DATA, *(const u32 *)buf);

We're passing a byte stream through a u32 here - are you sure this is
endian safe?
Mahapatra, Amit Kumar May 31, 2023, 5:26 a.m. UTC | #2
Hello,


> -----Original Message-----
> From: Charles Keepax <ckeepax@opensource.cirrus.com>
> Sent: Tuesday, May 30, 2023 5:51 PM
> To: broonie@kernel.org; lee@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> linus.walleij@linaro.org; vkoul@kernel.org
> Cc: robh+dt@kernel.org; conor+dt@kernel.org; lgirdwood@gmail.com; yung-
> chuan.liao@linux.intel.com; sanyog.r.kale@intel.com; pierre-
> louis.bossart@linux.intel.com; alsa-devel@alsa-project.org;
> patches@opensource.cirrus.com; devicetree@vger.kernel.org; linux-
> gpio@vger.kernel.org; linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH v2 5/6] spi: cs42l43: Add SPI controller support
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> 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.
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> Signed-off-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> Changes since v1:
>  - Use HZ_PER_MHZ
>  - Return ret in un/prepare_transfer_hardware
>  - Use SPI_MODE_X_MASK
>  - Use devm_pm_runtime_enable
>  - Switch to using a platform_device_id table
>  - Use device_set_node
>  - Use min rather than min_t, and updating some consts to make it work
> 
> Thanks,
> Charles
> 
>  MAINTAINERS               |   1 +
>  drivers/spi/Kconfig       |   7 +
>  drivers/spi/Makefile      |   1 +
>  drivers/spi/spi-cs42l43.c | 279 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 288 insertions(+)
>  create mode 100644 drivers/spi/spi-cs42l43.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ed26f63087342..a30dfbeb4f57a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4930,6 +4930,7 @@ F:
> Documentation/devicetree/bindings/mfd/cirrus,cs*
>  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
> 3de2ebe8294aa..f6ce06de41051 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
> 28c4817a8a74a..49937ea0d73d0 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..5f86f4feb22c8
> --- /dev/null
> +++ b/drivers/spi/spi-cs42l43.c
> @@ -0,0 +1,279 @@
> +// 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/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            (4 * 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 unsigned int cs42l43_clock_divs[16] = {
> +       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;
> +       int ret;
> +
> +       while (buf < end) {
> +               const u8 *block = min(buf + CS42L43_FIFO_SIZE, end);
> +
> +               for (; buf < block - (sizeof(u32) - 1); buf += sizeof(u32))
> +                       regmap_write(regmap, CS42L43_TX_DATA, *(const
> + u32 *)buf);
> +
> +               if (buf < block) {
> +                       unsigned int residue = end - buf;
> +
> +                       memcpy(&val, buf, residue);
> +                       regmap_write(regmap, CS42L43_TX_DATA, val);
> +                       buf += residue;
> +               }
> +
> +               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) {
> +               const 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;
> +
> +               for (; buf < block - (sizeof(u32) - 1); buf += sizeof(u32)) {
> +                       ret = regmap_read(regmap, CS42L43_RX_DATA, (u32 *)buf);
> +                       if (ret)
> +                               return ret;
> +               }
> +
> +               if (buf < block) {
> +                       unsigned int residue = end - buf;
> +
> +                       ret = regmap_read(regmap, CS42L43_RX_DATA, &val);
> +                       if (ret)
> +                               return ret;
> +
> +                       memcpy(buf, &val, residue);
> +                       buf += residue;
> +               }
> +
> +               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->chip_select == 0)

New set/get APIs for accessing spi->chip_select were introduced by
https://github.com/torvalds/linux/commit/303feb3cc06ac0665d0ee9c1414941200e60e8a3
please use these APIs instead of accessing spi->chip_select directly.

> +               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->cs_gpiod)

Same here for spi->cs_gpio

Regards,
Amit
> +               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");
> --
> 2.30.2
Charles Keepax May 31, 2023, 8:34 a.m. UTC | #3
On Tue, May 30, 2023 at 10:30:46PM +0100, Mark Brown wrote:
> On Tue, May 30, 2023 at 01:21:11PM +0100, Charles Keepax wrote:
> 
> A couple of small things:
> 
> > +static unsigned int cs42l43_clock_divs[16] = {
> > +	2, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30
> > +};
> 
> Do we need to specify the size of the array?  I just had to count the
> number of initialisers :(   Should probably also be const.
> 
> > +		for (; buf < block - (sizeof(u32) - 1); buf += sizeof(u32))
> > +			regmap_write(regmap, CS42L43_TX_DATA, *(const u32 *)buf);
> 
> We're passing a byte stream through a u32 here - are you sure this is
> endian safe?

Ah shoot, yeah Andy made some comments on this that seem to have
got lost in my mass of fixups. I will fixup for a v3.

Thanks,
Charles
Charles Keepax May 31, 2023, 8:44 a.m. UTC | #4
On Wed, May 31, 2023 at 05:26:46AM +0000, Mahapatra, Amit Kumar wrote:
> > +       if (spi->chip_select == 0)
> 
> New set/get APIs for accessing spi->chip_select were introduced by
> https://github.com/torvalds/linux/commit/303feb3cc06ac0665d0ee9c1414941200e60e8a3
> please use these APIs instead of accessing spi->chip_select directly.
> 
> > +       /* select another internal CS, which doesn't exist, so CS 0 is not used */
> > +       if (spi->cs_gpiod)
> 
> Same here for spi->cs_gpio
> 

Thanks, I will get these fixed up for v3.

Thanks,
Charles
Charles Keepax May 31, 2023, 8:50 a.m. UTC | #5
On Tue, May 30, 2023 at 01:21:08PM +0100, Charles Keepax wrote:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7e0b87d5aa2e5..0db9f37eec258 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4926,6 +4926,7 @@ M:	Richard Fitzgerald <rf@opensource.cirrus.com>
>  L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
>  L:	patches@opensource.cirrus.com
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/mfd/cirrus,cs*

Just spotted this should be dropped for v3 as well, sorry.

Thanks,
Charles
Krzysztof Kozlowski May 31, 2023, 9:02 a.m. UTC | #6
On 30/05/2023 14:21, Charles Keepax wrote:
> 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.
> 
> Add a YAML DT binding document for this device.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thank you for your patch. There is something to discuss/improve.


> +  clocks:
> +    items:
> +      - description: Synchronous audio clock provided on mclk_in.
> +
> +  clock-names:
> +    const: mclk
> +
> +  cirrus,bias-low:
> +    type: boolean
> +    description:
> +      Select a 1.8V headset micbias rather than 2.8V.
> +
> +  cirrus,bias-sense-ua:

"ua" looks like microamp. If so, microamp instead:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Current at which the headset micbias sense clamp will engage, 0 to
> +      disable.
> +    enum: [ 0, 14, 23, 41, 50, 60, 68, 86, 95 ]
> +    default: 0
> +
> +  cirrus,bias-ramp-ms:
> +    description:
> +      Time in milliseconds the hardware allows for the headset micbias to
> +      ramp up.
> +    enum: [ 10, 40, 90, 170 ]
> +    default: 170
> +
> +  cirrus,detect-us:
> +    description:
> +      Time in microseconds the type detection will run for. Long values will
> +      cause more audible effects, but give more accurate detection.
> +    enum: [ 20, 100, 1000, 10000, 50000, 75000, 100000, 200000 ]
> +    default: 10000
> +
> +  cirrus,button-automute:
> +    type: boolean
> +    description:
> +      Enable the hardware automuting of decimator 1 when a headset button is
> +      pressed.
> +
> +  cirrus,buttons-ohms:
> +    description:
> +      Impedance in Ohms for each headset button, these should be listed in
> +      ascending order.
> +    minItems: 1
> +    maxItems: 6
> +
> +  cirrus,tip-debounce-ms:
> +    description:
> +      Software debounce on tip sense triggering in milliseconds.
> +    default: 0
> +
> +  cirrus,tip-invert:
> +    type: boolean
> +    description:
> +      Indicates tip detect polarity, inverted implies open-circuit whilst the
> +      jack is inserted.
> +
> +  cirrus,tip-disable-pullup:
> +    type: boolean
> +    description:
> +      Indicates if the internal pullup on the tip detect should be disabled.
> +
> +  cirrus,tip-fall-db-ms:
> +    description:
> +      Time in milliseconds a falling edge on the tip detect should be hardware
> +      debounced for. Note the falling edge is considered after the invert.
> +    enum: [ 0, 125, 250, 500, 750, 1000, 1250, 1500 ]
> +    default: 500
> +
> +  cirrus,tip-rise-db-ms:
> +    description:
> +      Time in milliseconds a rising edge on the tip detect should be hardware
> +      debounced for. Note the rising edge is considered after the invert.
> +    enum: [ 0, 125, 250, 500, 750, 1000, 1250, 1500 ]
> +    default: 500
> +
> +  cirrus,use-ring-sense:
> +    type: boolean
> +    description:
> +      Indicates if the ring sense should be used.
> +
> +  cirrus,ring-invert:
> +    type: boolean
> +    description:
> +      Indicates ring detect polarity, inverted implies open-circuit whilst the
> +      jack is inserted.
> +
> +  cirrus,ring-disable-pullup:
> +    type: boolean
> +    description:
> +      Indicates if the internal pullup on the ring detect should be disabled.
> +
> +  cirrus,ring-fall-db-ms:
> +    description:
> +      Time in milliseconds a falling edge on the ring detect should be hardware
> +      debounced for. Note the falling edge is considered after the invert.
> +    enum: [ 0, 125, 250, 500, 750, 1000, 1250, 1500 ]
> +    default: 500
> +
> +  cirrus,ring-rise-db-ms:
> +    description:
> +      Time in milliseconds a rising edge on the ring detect should be hardware
> +      debounced for. Note the rising edge is considered after the invert.
> +    enum: [ 0, 125, 250, 500, 750, 1000, 1250, 1500 ]
> +    default: 500
> +
> +  pinctrl:
> +    type: object
> +
> +    allOf:

Drop allOf, just "$ref: ......"

> +      - $ref: /schemas/pinctrl/pinctrl.yaml#
> +
> +    additionalProperties: false

Also drop blank lines between these three above.

> +
> +    properties:
> +      gpio-controller: true
> +
> +      '#gpio-cells':
> +        const: 2
> +
> +      gpio-ranges:
> +        items:
> +          - description: A phandle to the CODEC pinctrl node
> +            minimum: 0
> +          - const: 0
> +          - const: 0
> +          - const: 3
> +
> +    patternProperties:
> +      "-state$":

Use consistent quotes, either " or ' everywhere

> +        oneOf:
> +          - $ref: "#/$defs/cirrus-cs42l43-state"
> +          - patternProperties:
> +              "-pins$":
> +                $ref: "#/$defs/cirrus-cs42l43-state"
> +            additionalProperties: false
> +
> +  spi:
> +    type: object
> +
> +    allOf:
> +      - $ref: /schemas/spi/spi-controller.yaml#
> +
> +    unevaluatedProperties: false

Same comments here.

> +
> +$defs:
> +  cirrus-cs42l43-state:
> +    type: object
> +



Best regards,
Krzysztof
Charles Keepax May 31, 2023, 2:12 p.m. UTC | #7
On Wed, May 31, 2023 at 11:02:24AM +0200, Krzysztof Kozlowski wrote:
> On 30/05/2023 14:21, Charles Keepax wrote:
> > +  cirrus,bias-sense-ua:
> 
> "ua" looks like microamp. If so, microamp instead:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> > +  pinctrl:
> > +    type: object
> > +
> > +    allOf:
> 
> Drop allOf, just "$ref: ......"
> 
> > +      - $ref: /schemas/pinctrl/pinctrl.yaml#
> > +
> > +    additionalProperties: false
> 
> Also drop blank lines between these three above.
> 
> > +    patternProperties:
> > +      "-state$":
> 
> Use consistent quotes, either " or ' everywhere
> 

Thanks, will fix those all up for v3.

Thanks,
Charles
Linus Walleij June 1, 2023, 11:22 a.m. UTC | #8
On Tue, May 30, 2023 at 2:21 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:

> 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.
>
> Add a basic pinctrl driver which supports driver strength for the
> various pins, gpios, and pinmux for the 2 multi-function pins.
>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

This version looks acceptable to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I guess it will be merged with the rest.

Yours,
Linus Walleij