[v4,0/4] Add support for CS40L50

From: James Ogletree <james.ogletree@cirrus.com>

This patch series adds support for Cirrus Logic CS40L50, a haptic driver.

While I2S streaming to the device will need to be supported in the future,
no codec driver is included in this submission and therefore this MFD
driver has just one component. A bare bones codec driver can be created
and included if maintainers prefer.

Changes in v4:
- Move from Input to MFD
- Move common Cirrus haptic functions to a library
- Incorporate runtime PM framework
- Coding style related improvements

Changes in v3:
- YAML formatting corrections
- Fix typo in MAINTAINERS
- Use generic node name "haptic-driver"
- Fix probe error code paths
- Use sizeof(*)
- Remove tree reference in MAINTAINERS

Changes in v2:
- Fix checkpatch warnings

James Ogletree (4):
  dt-bindings: input: cirrus,cs40l50: Add initial DT binding
  Input: cs40l50 - Add cirrus haptics base support
  mfd: cs40l50: Add support for CS40L50 core driver
  Input: cs40l50 - Add support for the CS40L50 haptic driver

 .../bindings/input/cirrus,cs40l50.yaml        |  70 +++
 MAINTAINERS                                   |  13 +
 drivers/input/misc/Kconfig                    |  10 +
 drivers/input/misc/Makefile                   |   1 +
 drivers/input/misc/cirrus_haptics.c           | 586 ++++++++++++++++++
 drivers/input/misc/cs40l50-vibra.c            | 353 +++++++++++
 drivers/mfd/Kconfig                           |  30 +
 drivers/mfd/Makefile                          |   4 +
 drivers/mfd/cs40l50-core.c                    | 443 +++++++++++++
 drivers/mfd/cs40l50-i2c.c                     |  69 +++
 drivers/mfd/cs40l50-spi.c                     |  68 ++
 include/linux/input/cirrus_haptics.h          | 121 ++++
 include/linux/mfd/cs40l50.h                   | 198 ++++++
 13 files changed, 1966 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
 create mode 100644 drivers/input/misc/cirrus_haptics.c
 create mode 100644 drivers/input/misc/cs40l50-vibra.c
 create mode 100644 drivers/mfd/cs40l50-core.c
 create mode 100644 drivers/mfd/cs40l50-i2c.c
 create mode 100644 drivers/mfd/cs40l50-spi.c
 create mode 100644 include/linux/input/cirrus_haptics.h
 create mode 100644 include/linux/mfd/cs40l50.h

On Wed, 18 Oct 2023, James Ogletree wrote:

> From: James Ogletree <james.ogletree@cirrus.com>
> 
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
> 
> The MFD component registers and initializes the device.
> 
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
>  MAINTAINERS                 |   2 +
>  drivers/mfd/Kconfig         |  30 +++
>  drivers/mfd/Makefile        |   4 +
>  drivers/mfd/cs40l50-core.c  | 443 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/cs40l50-i2c.c   |  69 ++++++
>  drivers/mfd/cs40l50-spi.c   |  68 ++++++
>  include/linux/mfd/cs40l50.h | 198 ++++++++++++++++
>  7 files changed, 814 insertions(+)
>  create mode 100644 drivers/mfd/cs40l50-core.c
>  create mode 100644 drivers/mfd/cs40l50-i2c.c
>  create mode 100644 drivers/mfd/cs40l50-spi.c
>  create mode 100644 include/linux/mfd/cs40l50.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 57daf77bf550..08e1e9695d49 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4971,7 +4971,9 @@ L:	patches@opensource.cirrus.com
>  S:	Supported
>  F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
>  F:	drivers/input/misc/cirrus*
> +F:	drivers/mfd/cs40l*
>  F:	include/linux/input/cirrus*
> +F:	include/linux/mfd/cs40l*
>  
>  CIRRUS LOGIC DSP FIRMWARE DRIVER
>  M:	Simon Trimmer <simont@opensource.cirrus.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b93856de432..a133d04a7859 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2187,6 +2187,36 @@ config MCP_UCB1200_TS
>  
>  endmenu
>  
> +config MFD_CS40L50_CORE
> +	tristate
> +	select MFD_CORE
> +	select CS_DSP
> +	select REGMAP_IRQ
> +
> +config MFD_CS40L50_I2C
> +	tristate "Cirrus Logic CS40L50 (I2C)"
> +	select REGMAP_I2C
> +	select MFD_CS40L50_CORE
> +	depends on I2C
> +	help
> +	  Select this to support the Cirrus Logic CS40L50 Haptic
> +	  Driver over I2C.
> +
> +	  This driver can be built as a module. If built as a module it will be
> +	  called "cs40l50-i2c".
> +
> +config MFD_CS40L50_SPI
> +	tristate "Cirrus Logic CS40L50 (SPI)"
> +	select REGMAP_SPI
> +	select MFD_CS40L50_CORE
> +	depends on SPI
> +	help
> +	  Select this to support the Cirrus Logic CS40L50 Haptic
> +	  Driver over SPI.
> +
> +	  This driver can be built as a module. If built as a module it will be
> +	  called "cs40l50-spi".
> +
>  config MFD_VEXPRESS_SYSREG
>  	tristate "Versatile Express System Registers"
>  	depends on VEXPRESS_CONFIG && GPIOLIB
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..3b1a43b3acaf 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -95,6 +95,10 @@ obj-$(CONFIG_MFD_MADERA)	+= madera.o
>  obj-$(CONFIG_MFD_MADERA_I2C)	+= madera-i2c.o
>  obj-$(CONFIG_MFD_MADERA_SPI)	+= madera-spi.o
>  
> +obj-$(CONFIG_MFD_CS40L50_CORE)	+= cs40l50-core.o
> +obj-$(CONFIG_MFD_CS40L50_I2C)	+= cs40l50-i2c.o
> +obj-$(CONFIG_MFD_CS40L50_SPI)	+= cs40l50-spi.o
> +
>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> diff --git a/drivers/mfd/cs40l50-core.c b/drivers/mfd/cs40l50-core.c
> new file mode 100644
> index 000000000000..f1eadd80203a
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-core.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,

s/Driver/device/

> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *

Remove this line.

No Author?

> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +static const struct mfd_cell cs40l50_devs[] = {
> +	{
> +		.name = "cs40l50-vibra",
> +	},

This should be on one line.

Where are the other devices?  Without them, it's not an MFD.

> +};
> +
> +const struct regmap_config cs40l50_regmap = {
> +	.reg_bits =		32,
> +	.reg_stride =		4,
> +	.val_bits =		32,
> +	.reg_format_endian =	REGMAP_ENDIAN_BIG,
> +	.val_format_endian =	REGMAP_ENDIAN_BIG,
> +};
> +EXPORT_SYMBOL_GPL(cs40l50_regmap);
> +
> +static struct regulator_bulk_data cs40l50_supplies[] = {
> +	{
> +		.supply = "vp",
> +	},
> +	{
> +		.supply = "vio",
> +	},

One line each please.

> +};
> +
> +static int cs40l50_handle_f0_est_done(struct cs40l50_private *cs40l50)
> +{
> +	u32 f_zero;
> +	int error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_F0_ESTIMATION, &f_zero);
> +	if (error)
> +		return error;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_F0_STORED, f_zero);
> +}

I have no idea what this is doing (probably goes for the following
functions too.  Either give the function a friendly name or provide
commentary so people can read it.

> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> +{
> +	int error, fractional, integer, stored;

err or ret is traditional.

The other variables need better nomenclature.

> +	u32 redc;

This one too.

I won't mention this again, but is likely to apply throughout.

> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_RE_EST_STATUS, &redc);
> +	if (error)
> +		return error;
> +
> +	error = regmap_write(cs40l50->regmap, CS40L50_REDC_ESTIMATION, redc);
> +	if (error)
> +		return error;
> +
> +	/* Convert from Q8.15 to (Q7.17 * 29/240) */

I have no idea what this is supposed to be telling me.

> +	integer = ((redc >> 15) & 0xFF) << 17;
> +	fractional = (redc & 0x7FFF) * 4;
> +	stored = (integer | fractional) * 29 / 240;

No magic numbers.  Define them all please.

> +	return regmap_write(cs40l50->regmap, CS40L50_REDC_STORED, stored);
> +}
> +
> +static int cs40l50_error_release(struct cs40l50_private *cs40l50)
> +{
> +	int error;
> +
> +	error = regmap_write(cs40l50->regmap, CS40L50_ERR_RLS,
> +			     CS40L50_GLOBAL_ERR_RLS);
> +	if (error)
> +		return error;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_ERR_RLS, 0);
> +}
> +
> +static int cs40l50_mailbox_read_next(struct cs40l50_private *cs40l50, u32 *val)
> +{
> +	u32 rd_ptr, wt_ptr;
> +	int error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_WT, &wt_ptr);
> +	if (error)
> +		return error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, &rd_ptr);
> +	if (error)
> +		return error;
> +
> +	if (wt_ptr == rd_ptr) {
> +		*val = 0;
> +		return 0;
> +	}
> +
> +	error = regmap_read(cs40l50->regmap, rd_ptr, val);
> +	if (error)
> +		return error;
> +
> +	rd_ptr += sizeof(u32);
> +	if (rd_ptr > CS40L50_MBOX_QUEUE_END)
> +		rd_ptr = CS40L50_MBOX_QUEUE_BASE;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, rd_ptr);
> +}
> +
> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +	int error = 0;
> +	u32 val;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
> +		switch (val) {
> +		case 0:
> +			mutex_unlock(&cs40l50->lock);
> +			dev_dbg(cs40l50->dev, "Reached end of queue\n");
> +			return IRQ_HANDLED;
> +		case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");

These all appear to be no-ops?

> +			break;
> +		case CS40L50_MBOX_HAPTIC_TRIGGER_MBOX:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_MBOX\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_TRIGGER_I2S:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_I2S\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_COMPLETE_MBOX:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_MBOX\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_COMPLETE_GPIO:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_GPIO\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_COMPLETE_I2S:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_I2S\n");
> +			break;
> +		case CS40L50_MBOX_F0_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_START\n");
> +			break;
> +		case CS40L50_MBOX_F0_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_DONE\n");
> +			error = cs40l50_handle_f0_est_done(cs40l50);
> +			if (error)
> +				goto out_mutex;
> +			break;
> +		case CS40L50_MBOX_REDC_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_START\n");
> +			break;
> +		case CS40L50_MBOX_REDC_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_DONE\n");
> +			error = cs40l50_handle_redc_est_done(cs40l50);
> +			if (error)
> +				goto out_mutex;
> +			break;
> +		case CS40L50_MBOX_LE_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_START\n");
> +			break;
> +		case CS40L50_MBOX_LE_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_DONE\n");
> +			break;
> +		case CS40L50_MBOX_AWAKE:
> +			dev_dbg(cs40l50->dev, "Mailbox: AWAKE\n");
> +			break;
> +		case CS40L50_MBOX_INIT:
> +			dev_dbg(cs40l50->dev, "Mailbox: INIT\n");
> +			break;
> +		case CS40L50_MBOX_ACK:
> +			dev_dbg(cs40l50->dev, "Mailbox: ACK\n");
> +			break;
> +		case CS40L50_MBOX_ERR_EVENT_UNMAPPED:
> +			dev_err(cs40l50->dev, "Unmapped event\n");
> +			break;
> +		case CS40L50_MBOX_ERR_EVENT_MODIFY:
> +			dev_err(cs40l50->dev, "Failed to modify event index\n");
> +			break;
> +		case CS40L50_MBOX_ERR_NULLPTR:
> +			dev_err(cs40l50->dev, "Null pointer\n");
> +			break;
> +		case CS40L50_MBOX_ERR_BRAKING:
> +			dev_err(cs40l50->dev, "Braking not in progress\n");
> +			break;
> +		case CS40L50_MBOX_ERR_INVAL_SRC:
> +			dev_err(cs40l50->dev, "Suspend/resume invalid source\n");
> +			break;
> +		case CS40L50_MBOX_ERR_ENABLE_RANGE:
> +			dev_err(cs40l50->dev, "GPIO enable out of range\n");
> +			break;
> +		case CS40L50_MBOX_ERR_GPIO_UNMAPPED:
> +			dev_err(cs40l50->dev, "GPIO not mapped\n");
> +			break;
> +		case CS40L50_MBOX_ERR_ISR_RANGE:
> +			dev_err(cs40l50->dev, "GPIO ISR out of range\n");
> +			break;
> +		case CS40L50_MBOX_PERMANENT_SHORT:
> +			dev_crit(cs40l50->dev, "Permanent short detected\n");
> +			break;
> +		case CS40L50_MBOX_RUNTIME_SHORT:
> +			dev_err(cs40l50->dev, "Runtime short detected\n");
> +			error = cs40l50_error_release(cs40l50);
> +			if (error)
> +				goto out_mutex;
> +			break;
> +		default:
> +			dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
> +			error = -EINVAL;
> +			goto out_mutex;
> +		}
> +	}
> +
> +	error = -EIO;
> +
> +out_mutex:
> +	mutex_unlock(&cs40l50->lock);
> +
> +	return IRQ_RETVAL(!error);
> +}

Should the last two drivers live in drivers/mailbox?

> +static irqreturn_t cs40l50_error(int irq, void *data);

Why is this being forward declared?

> +static const struct cs40l50_irq cs40l50_irqs[] = {
> +	CS40L50_IRQ(AMP_SHORT,		"Amp short",		error),

I assume that last parameter is half of a function name.

Better to have 2 different structures and do 2 requests I feel.

> +	CS40L50_IRQ(VIRT2_MBOX,		"Mailbox",		process_mbox),
> +	CS40L50_IRQ(TEMP_ERR,		"Overtemperature",	error),
> +	CS40L50_IRQ(BST_UVP,		"Boost undervoltage",	error),
> +	CS40L50_IRQ(BST_SHORT,		"Boost short",		error),
> +	CS40L50_IRQ(BST_ILIMIT,		"Boost current limit",	error),
> +	CS40L50_IRQ(UVLO_VDDBATT,	"Boost UVLO",		error),
> +	CS40L50_IRQ(GLOBAL_ERROR,	"Global",		error),
> +};
> +
> +static irqreturn_t cs40l50_error(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);

> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));

Please break the function out of the parentheses.

We don't tend to put functions in if()s either.

> +}
> +
> +static const struct regmap_irq cs40l50_reg_irqs[] = {
> +	CS40L50_REG_IRQ(IRQ1_INT_1,	AMP_SHORT),

I commented on these where you defined them - see below.

> +	CS40L50_REG_IRQ(IRQ1_INT_2,	VIRT2_MBOX),
> +	CS40L50_REG_IRQ(IRQ1_INT_8,	TEMP_ERR),
> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_UVP),
> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_SHORT),
> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_ILIMIT),
> +	CS40L50_REG_IRQ(IRQ1_INT_10,	UVLO_VDDBATT),
> +	CS40L50_REG_IRQ(IRQ1_INT_18,	GLOBAL_ERROR),
> +};
> +
> +static struct regmap_irq_chip cs40l50_irq_chip = {
> +	.name =			"CS40L50 IRQ Controller",
> +
> +	.status_base =		CS40L50_IRQ1_INT_1,
> +	.mask_base =		CS40L50_IRQ1_MASK_1,
> +	.ack_base =		CS40L50_IRQ1_INT_1,
> +	.num_regs =		22,
> +
> +	.irqs =			cs40l50_reg_irqs,
> +	.num_irqs =		ARRAY_SIZE(cs40l50_reg_irqs),
> +
> +	.runtime_pm =		true,
> +};
> +
> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
> +{
> +	struct device *dev = cs40l50->dev;
> +	int error, i, irq;
> +
> +	error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
> +					 IRQF_ONESHOT | IRQF_SHARED, 0,
> +					 &cs40l50_irq_chip, &cs40l50->irq_data);
> +	if (error)
> +		return error;
> +
> +	for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
> +		irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
> +		if (irq < 0) {
> +			dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
> +			return irq;
> +		}
> +
> +		error = devm_request_threaded_irq(dev, irq, NULL,
> +						  cs40l50_irqs[i].handler,
> +						  IRQF_ONESHOT | IRQF_SHARED,
> +						  cs40l50_irqs[i].name, cs40l50);
> +		if (error) {
> +			dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
> +			return error;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int cs40l50_part_num_resolve(struct cs40l50_private *cs40l50)
> +{
> +	struct device *dev = cs40l50->dev;
> +	int error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_DEVID, &cs40l50->devid);
> +	if (error)
> +		return error;
> +
> +	if (cs40l50->devid != CS40L50_DEVID_A) {
> +		dev_err(dev, "Invalid device ID: %#010X\n", cs40l50->devid);
> +		return -EINVAL;
> +	}
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_REVID, &cs40l50->revid);
> +	if (error)
> +		return error;
> +
> +	if (cs40l50->revid != CS40L50_REVID_B0) {
> +		dev_err(dev, "Invalid revision: %#04X\n", cs40l50->revid);
> +		return -EINVAL;
> +	}
> +
> +	dev_info(dev, "Cirrus Logic CS40L50 revision %02X\n", cs40l50->revid);
> +
> +	return 0;
> +}
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50)

Previous Cirrus drivers have omitted the "_private" part, which I think
is better.  "_ddata" is also common and acceptable.

> +{
> +	struct device *dev = cs40l50->dev;
> +	int error;
> +
> +	mutex_init(&cs40l50->lock);

Don't you need to destroy this in the error path?

> +	cs40l50->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(cs40l50->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(cs40l50->reset_gpio),
> +				     "Failed getting reset GPIO\n");
> +
> +	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(cs40l50_supplies),
> +					cs40l50_supplies);
> +	if (error)
> +		goto err_reset;
> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies),
> +				      cs40l50_supplies);
> +	if (error)
> +		goto err_reset;
> +
> +	usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 100);

A comment for why this is required please.

And why 100us is appropriate.

> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
> +
> +	usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 1000);

As above.

> +	pm_runtime_set_autosuspend_delay(cs40l50->dev, CS40L50_AUTOSUSPEND_MS);
> +	pm_runtime_use_autosuspend(cs40l50->dev);
> +	pm_runtime_set_active(cs40l50->dev);
> +	pm_runtime_get_noresume(cs40l50->dev);
> +	devm_pm_runtime_enable(cs40l50->dev);
> +
> +	error = cs40l50_part_num_resolve(cs40l50);

*_get_model()?

> +	if (error)
> +		goto err_supplies;
> +
> +	error = cs40l50_irq_init(cs40l50);
> +	if (error)
> +		goto err_supplies;
> +
> +	error = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cs40l50_devs,
> +				     ARRAY_SIZE(cs40l50_devs), NULL, 0, NULL);
> +	if (error)
> +		goto err_supplies;
> +
> +	pm_runtime_mark_last_busy(cs40l50->dev);
> +	pm_runtime_put_autosuspend(cs40l50->dev);
> +
> +	return 0;
> +
> +err_supplies:
> +	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +err_reset:
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_probe);
> +
> +int cs40l50_remove(struct cs40l50_private *cs40l50)
> +{
> +	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);

mutex_destroy()?

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_remove);
> +
> +static int cs40l50_runtime_suspend(struct device *dev)
> +{
> +	struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX, CS40L50_ALLOW_HIBER);
> +}
> +
> +static int cs40l50_runtime_resume(struct device *dev)
> +{
> +	struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> +	int error, i;
> +	u32 val;
> +
> +	/* Device NAKs when exiting hibernation, so optionally retry here. */

A comment - hoorah!

> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX,
> +				     CS40L50_PREVENT_HIBER);
> +		if (!error)
> +			break;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	for (; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {

So, the amount of time this section is given is entirely based on how
well the previous section did.  Is that intentional?

Perhaps some comments to help straighten out your intentions.

> +		error = regmap_read(cs40l50->regmap, CS40L50_DSP_MBOX, &val);
> +		if (!error && val == 0)
> +			return 0;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	return error ? error : -ETIMEDOUT;

return error ?: -ETIMEDOUT;

> +}
> +
> +EXPORT_GPL_DEV_PM_OPS(cs40l50_pm_ops) = {
> +	RUNTIME_PM_OPS(cs40l50_runtime_suspend, cs40l50_runtime_resume, NULL)
> +};
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-i2c.c b/drivers/mfd/cs40l50-i2c.c
> new file mode 100644
> index 000000000000..be1b130eb94b
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-i2c.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 I2C Driver

This is not an I2C driver.

Best to describe the hardware rather that the "driver".

> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *

Remove please.

> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/cs40l50.h>
> +
> +static int cs40l50_i2c_probe(struct i2c_client *client,
> +			     const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct cs40l50_private *cs40l50;
> +
> +	cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> +	if (!cs40l50)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, cs40l50);
> +
> +	cs40l50->regmap = devm_regmap_init_i2c(client, &cs40l50_regmap);
> +	if (IS_ERR(cs40l50->regmap))
> +		return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> +				     "Failed to initialize register map\n");
> +
> +	cs40l50->dev = dev;
> +	cs40l50->irq = client->irq;
> +
> +	return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_i2c_remove(struct i2c_client *client)
> +{
> +	struct cs40l50_private *cs40l50 = i2c_get_clientdata(client);
> +
> +	cs40l50_remove(cs40l50);
> +}
> +
> +static const struct i2c_device_id cs40l50_id_i2c[] = {
> +	{"cs40l50", 0},

This second parameter shouldn't be required now.

> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, cs40l50_id_i2c);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> +	{ .compatible = "cirrus,cs40l50" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct i2c_driver cs40l50_i2c_driver = {
> +	.driver = {
> +		.name = "cs40l50",
> +		.of_match_table = cs40l50_of_match,
> +		.pm = pm_ptr(&cs40l50_pm_ops),
> +	},
> +	.id_table = cs40l50_id_i2c,
> +	.probe = cs40l50_i2c_probe,
> +	.remove = cs40l50_i2c_remove,
> +};
> +

Remove this line.

> +module_i2c_driver(cs40l50_i2c_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 I2C Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-spi.c b/drivers/mfd/cs40l50-spi.c
> new file mode 100644
> index 000000000000..8311d48efedf
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-spi.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 SPI Driver
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *

Same comments as before.

> + */
> +
> +#include <linux/input/cs40l50.h>
> +#include <linux/mfd/spi.h>
> +
> +static int cs40l50_spi_probe(struct spi_device *spi)
> +{
> +	struct cs40l50_private *cs40l50;
> +	struct device *dev = &spi->dev;
> +
> +	cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> +	if (!cs40l50)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, cs40l50);
> +
> +	cs40l50->regmap = devm_regmap_init_spi(spi, &cs40l50_regmap);
> +	if (IS_ERR(cs40l50->regmap))
> +		return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> +				     "Failed to initialize register map\n");
> +
> +	cs40l50->dev = dev;
> +	cs40l50->irq = spi->irq;
> +
> +	return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_spi_remove(struct spi_device *spi)
> +{
> +	struct cs40l50_private *cs40l50 = spi_get_drvdata(spi);
> +
> +	cs40l50_remove(cs40l50);
> +}
> +
> +static const struct spi_device_id cs40l50_id_spi[] = {
> +	{"cs40l50", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, cs40l50_id_spi);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> +	{ .compatible = "cirrus,cs40l50" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct spi_driver cs40l50_spi_driver = {
> +	.driver = {
> +		.name = "cs40l50",
> +		.of_match_table = cs40l50_of_match,
> +		.pm = pm_ptr(&cs40l50_pm_ops),
> +	},
> +	.id_table = cs40l50_id_spi,
> +	.probe = cs40l50_spi_probe,
> +	.remove = cs40l50_spi_remove,
> +};
> +
> +module_spi_driver(cs40l50_spi_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 SPI Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/cs40l50.h b/include/linux/mfd/cs40l50.h
> new file mode 100644
> index 000000000000..c625a999a5ae
> --- /dev/null
> +++ b/include/linux/mfd/cs40l50.h
> @@ -0,0 +1,198 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#ifndef __CS40L50_H__
> +#define __CS40L50_H__
> +
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/input.h>
> +#include <linux/input/cirrus_haptics.h>
> +#include <linux/interrupt.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Power Supply Configuration */
> +#define CS40L50_BLOCK_ENABLES2			0x201C
> +#define CS40L50_ERR_RLS				0x2034
> +#define CS40L50_PWRMGT_CTL			0x2900
> +#define CS40L50_BST_LPMODE_SEL			0x3810
> +#define CS40L50_DCM_LOW_POWER		0x1
> +#define CS40L50_OVERTEMP_WARN		0x4000010
> +
> +/* Interrupts */
> +#define CS40L50_IRQ1_INT_1			0xE010
> +#define CS40L50_IRQ1_INT_2			0xE014
> +#define CS40L50_IRQ1_INT_8			0xE02C
> +#define CS40L50_IRQ1_INT_9			0xE030
> +#define CS40L50_IRQ1_INT_10			0xE034
> +#define CS40L50_IRQ1_INT_18			0xE054
> +#define CS40L50_IRQ1_MASK_1			0xE090
> +#define CS40L50_IRQ1_MASK_2			0xE094
> +#define CS40L50_IRQ1_MASK_20			0xE0DC
> +#define CS40L50_IRQ_MASK_2_OVERRIDE	0xFFDF7FFF
> +#define CS40L50_IRQ_MASK_20_OVERRIDE	0x15C01000
> +#define CS40L50_AMP_SHORT_MASK		BIT(31)
> +#define CS40L50_VIRT2_MBOX_MASK		BIT(21)
> +#define CS40L50_TEMP_ERR_MASK		BIT(31)
> +#define CS40L50_BST_UVP_MASK		BIT(6)
> +#define CS40L50_BST_SHORT_MASK		BIT(7)
> +#define CS40L50_BST_ILIMIT_MASK		BIT(8)
> +#define CS40L50_UVLO_VDDBATT_MASK	BIT(16)
> +#define CS40L50_GLOBAL_ERROR_MASK	BIT(15)
> +#define CS40L50_GLOBAL_ERR_RLS		BIT(11)
> +#define CS40L50_IRQ(_irq, _name, _hand)		\
> +	{					\
> +		.irq = CS40L50_ ## _irq ## _IRQ,\
> +		.name = _name,			\
> +		.handler = cs40l50_ ## _hand,	\
> +	}
> +#define CS40L50_REG_IRQ(_reg, _irq)					\

Please don't reinvent the wheel:

  REGMAP_IRQ_REG_LINE()

> +	[CS40L50_ ## _irq ## _IRQ] = {					\
> +		.reg_offset = (CS40L50_ ## _reg) - CS40L50_IRQ1_INT_1,	\
> +		.mask = CS40L50_ ## _irq ## _MASK			\
> +	}
> +
> +/* Mailbox Inbound Commands */
> +#define CS40L50_RAM_BANK_INDEX_START	0x1000000
> +#define CS40L50_RTH_INDEX_START		0x1400000
> +#define CS40L50_RTH_INDEX_END		0x1400001
> +#define CS40L50_ROM_BANK_INDEX_START	0x1800000
> +#define CS40L50_ROM_BANK_INDEX_END	0x180001A
> +#define CS40L50_PREVENT_HIBER		0x2000003
> +#define CS40L50_ALLOW_HIBER		0x2000004
> +#define CS40L50_OWT_PUSH		0x3000008
> +#define CS40L50_STOP_PLAYBACK		0x5000000
> +#define CS40L50_OWT_DELETE		0xD000000
> +
> +/* Mailbox Outbound Commands */
> +#define CS40L50_MBOX_QUEUE_BASE				0x11004
> +#define CS40L50_MBOX_QUEUE_END				0x1101C
> +#define CS40L50_DSP_MBOX				0x11020
> +#define CS40L50_MBOX_QUEUE_WT				0x28042C8
> +#define CS40L50_MBOX_QUEUE_RD				0x28042CC
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_MBOX	0x1000000
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_GPIO	0x1000001
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_I2S	0x1000002
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_MBOX	0x1000010
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_GPIO	0x1000011
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_I2S		0x1000012
> +#define CS40L50_MBOX_INIT			0x2000000
> +#define CS40L50_MBOX_AWAKE			0x2000002
> +#define CS40L50_MBOX_F0_EST_START		0x7000011
> +#define CS40L50_MBOX_F0_EST_DONE		0x7000021
> +#define CS40L50_MBOX_REDC_EST_START		0x7000012
> +#define CS40L50_MBOX_REDC_EST_DONE		0x7000022
> +#define CS40L50_MBOX_LE_EST_START		0x7000014
> +#define CS40L50_MBOX_LE_EST_DONE		0x7000024
> +#define CS40L50_MBOX_ACK			0xA000000
> +#define CS40L50_MBOX_PANIC			0xC000000
> +#define CS40L50_MBOX_WATERMARK			0xD000000
> +#define CS40L50_MBOX_ERR_EVENT_UNMAPPED		0xC0004B3
> +#define CS40L50_MBOX_ERR_EVENT_MODIFY		0xC0004B4
> +#define CS40L50_MBOX_ERR_NULLPTR		0xC0006A5
> +#define CS40L50_MBOX_ERR_BRAKING		0xC0006A8
> +#define CS40L50_MBOX_ERR_INVAL_SRC		0xC0006AC
> +#define CS40L50_MBOX_ERR_ENABLE_RANGE		0xC000836
> +#define CS40L50_MBOX_ERR_GPIO_UNMAPPED		0xC000837
> +#define CS40L50_MBOX_ERR_ISR_RANGE		0xC000838
> +#define CS40L50_MBOX_PERMANENT_SHORT		0xC000C1C
> +#define CS40L50_MBOX_RUNTIME_SHORT		0xC000C1D
> +
> +/* DSP */
> +#define CS40L50_DSP1_XMEM_PACKED_0		0x2000000
> +#define CS40L50_DSP1_XMEM_UNPACKED32_0		0x2400000
> +#define CS40L50_SYS_INFO_ID			0x25E0000
> +#define CS40L50_DSP1_XMEM_UNPACKED24_0		0x2800000
> +#define CS40L50_RAM_INIT			0x28021DC
> +#define CS40L50_POWER_ON_SEQ			0x2804320
> +#define CS40L50_OWT_BASE			0x2805C34
> +#define CS40L50_NUM_OF_WAVES			0x280CB4C
> +#define CS40L50_CORE_BASE			0x2B80000
> +#define CS40L50_CCM_CORE_CONTROL		0x2BC1000
> +#define CS40L50_DSP1_YMEM_PACKED_0		0x2C00000
> +#define CS40L50_DSP1_YMEM_UNPACKED32_0		0x3000000
> +#define CS40L50_DSP1_YMEM_UNPACKED24_0		0x3400000
> +#define CS40L50_DSP1_PMEM_0			0x3800000
> +#define CS40L50_DSP1_PMEM_5114			0x3804FE8
> +#define CS40L50_MEM_RDY_HW		0x2
> +#define CS40L50_RAM_INIT_FLAG		0x1
> +#define CS40L50_CLOCK_DISABLE		0x80
> +#define CS40L50_CLOCK_ENABLE		0x281
> +#define CS40L50_DSP_POLL_US		1000
> +#define CS40L50_DSP_TIMEOUT_COUNT	100
> +#define CS40L50_CP_READY_US		2200
> +#define CS40L50_PSEQ_SIZE		200
> +#define CS40L50_AUTOSUSPEND_MS		2000
> +
> +/* Firmware */
> +#define CS40L50_FW			"cs40l50.wmfw"
> +#define CS40L50_WT			"cs40l50.bin"
> +
> +/* Calibration */
> +#define CS40L50_REDC_ESTIMATION		0x2802F7C
> +#define CS40L50_F0_ESTIMATION		0x2802F84
> +#define CS40L50_F0_STORED		0x2805C00
> +#define CS40L50_REDC_STORED		0x2805C04
> +#define CS40L50_RE_EST_STATUS		0x3401B40
> +
> +/* Open wavetable */
> +#define CS40L50_OWT_SIZE		0x2805C38
> +#define CS40L50_OWT_NEXT		0x2805C3C
> +#define CS40L50_NUM_OF_OWT_WAVES	0x2805C40
> +
> +/* GPIO */
> +#define CS40L50_GPIO_BASE		0x2804140
> +
> +/* Device */
> +#define CS40L50_DEVID			0x0
> +#define CS40L50_REVID			0x4
> +#define CS40L50_DEVID_A		0x40A50
> +#define CS40L50_REVID_B0	0xB0
> +
> +enum cs40l50_irq_list {
> +	CS40L50_GLOBAL_ERROR_IRQ,
> +	CS40L50_UVLO_VDDBATT_IRQ,
> +	CS40L50_BST_ILIMIT_IRQ,
> +	CS40L50_BST_SHORT_IRQ,
> +	CS40L50_BST_UVP_IRQ,
> +	CS40L50_TEMP_ERR_IRQ,
> +	CS40L50_VIRT2_MBOX_IRQ,
> +	CS40L50_AMP_SHORT_IRQ,
> +};
> +
> +struct cs40l50_irq {
> +	const char *name;
> +	int irq;
> +	irqreturn_t (*handler)(int irq, void *data);
> +};
> +
> +struct cs40l50_private {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct cs_dsp dsp;
> +	struct mutex lock;
> +	struct gpio_desc *reset_gpio;
> +	struct regmap_irq_chip_data *irq_data;
> +	struct input_dev *input;

Where is this used?

> +	const struct firmware *wmfw;

Or this.

> +	struct cs_hap haptics;

Or this?

> +	u32 devid;
> +	u32 revid;

Are these used after they're set?

> +	int irq;
> +};
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50);
> +int cs40l50_remove(struct cs40l50_private *cs40l50);
> +
> +extern const struct regmap_config cs40l50_regmap;
> +extern const struct dev_pm_ops cs40l50_pm_ops;
> +
> +#endif /* __CS40L50_H__ */
> -- 
> 2.25.1
>

Thank you for your thorough review. Anything not replied to below will be
incorporated in the next version.

>> +/*
>> + * CS40L50 Advanced Haptic Driver with waveform memory,
> 
> s/Driver/device/

CS40L50 is a “haptic driver”, like a "motor driver" in a car. It is an
unfortunate name in this context, but it is straight from the datasheet.

>> +static const struct mfd_cell cs40l50_devs[] = {
>> + {
>> + .name = "cs40l50-vibra",
>> + },
> 
> 
> Where are the other devices?  Without them, it's not an MFD.

The driver will need to support I2S streaming to the device at some point
in the future, for which a codec driver will be added. I thought it better to
submit this as an MFD driver now, rather than as an Input driver, so as
not to have to move everything later.

Should I add the “cs40l50-codec” mfd_cell now, even though it does not
exist yet?

>> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
>> +{
>> + int error, fractional, integer, stored;
> 
> err or ret is traditional.

We received feedback to change from “ret” to “error” in the input
subsystem, and now the opposite in MFD. I have no problem adopting
“err” here, but is it understood that styles will be mixed across
components?

>> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
>> +{
>> + struct cs40l50_private *cs40l50 = data;
>> + int error = 0;
>> + u32 val;
>> +
>> + mutex_lock(&cs40l50->lock);
>> +
>> + while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
>> + switch (val) {
>> + case 0:
>> + mutex_unlock(&cs40l50->lock);
>> + dev_dbg(cs40l50->dev, "Reached end of queue\n");
>> + return IRQ_HANDLED;
>> + case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
>> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
> 
> These all appear to be no-ops?

Correct.

>> + case CS40L50_MBOX_RUNTIME_SHORT:
>> + dev_err(cs40l50->dev, "Runtime short detected\n");
>> + error = cs40l50_error_release(cs40l50);
>> + if (error)
>> + goto out_mutex;
>> + break;
>> + default:
>> + dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
>> + error = -EINVAL;
>> + goto out_mutex;
>> + }
>> + }
>> +
>> + error = -EIO;
>> +
>> +out_mutex:
>> + mutex_unlock(&cs40l50->lock);
>> +
>> + return IRQ_RETVAL(!error);
>> +}
> 
> Should the last two drivers live in drivers/mailbox?

Adopting the mailbox framework seems like an excessive amount
of overhead for our requirements.

>> +static irqreturn_t cs40l50_error(int irq, void *data);
> 
> Why is this being forward declared?
> 
>> +static const struct cs40l50_irq cs40l50_irqs[] = {
>> + CS40L50_IRQ(AMP_SHORT, "Amp short", error),
> 
> I assume that last parameter is half of a function name.
> 
> Better to have 2 different structures and do 2 requests I feel.

I think I will combine the two handler functions into one, so as not
to need the struct handler parameter, or the forward declaration.

>> +{
>> + struct device *dev = cs40l50->dev;
>> + int error;
>> +
>> + mutex_init(&cs40l50->lock);
> 
> Don't you need to destroy this in the error path?

My understanding based on past feedback is that mutex_destroy()
is an empty function unless mutex debugging is enabled, and there
is no need cleanup the mutex explicitly. I will change this if you
disagree with that feedback.

> 
>> +struct cs40l50_irq {
>> + const char *name;
>> + int irq;
>> + irqreturn_t (*handler)(int irq, void *data);
>> +};
>> +
>> +struct cs40l50_private {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct cs_dsp dsp;
>> + struct mutex lock;
>> + struct gpio_desc *reset_gpio;
>> + struct regmap_irq_chip_data *irq_data;
>> + struct input_dev *input;
> 
> Where is this used?
> 
>> + const struct firmware *wmfw;
> 
> Or this.
> 
>> + struct cs_hap haptics;
> 
> Or this?
> 
>> + u32 devid;
>> + u32 revid;
> 
> Are these used after they're set?

These are all used in the input driver, patch 4/4 of this series. If
this is not acceptable in some way, I will change it per your
suggestions.

Best,
James

On Fri, 20 Oct 2023, James Ogletree wrote:

> 
> Thank you for your thorough review. Anything not replied to below will be
> incorporated in the next version.
> 
> >> +/*
> >> + * CS40L50 Advanced Haptic Driver with waveform memory,
> > 
> > s/Driver/device/
> 
> CS40L50 is a “haptic driver”, like a "motor driver" in a car. It is an
> unfortunate name in this context, but it is straight from the datasheet.

Understood.  That's fine then.

> >> +static const struct mfd_cell cs40l50_devs[] = {
> >> + {
> >> + .name = "cs40l50-vibra",
> >> + },
> > 
> > 
> > Where are the other devices?  Without them, it's not an MFD.
> 
> The driver will need to support I2S streaming to the device at some point
> in the future, for which a codec driver will be added. I thought it better to
> submit this as an MFD driver now, rather than as an Input driver, so as
> not to have to move everything later.
> 
> Should I add the “cs40l50-codec” mfd_cell now, even though it does not
> exist yet?

What is your timeline for this to be authored?

Does the device function well without it?

> >> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> >> +{
> >> + int error, fractional, integer, stored;
> > 
> > err or ret is traditional.
> 
> We received feedback to change from “ret” to “error” in the input
> subsystem, and now the opposite in MFD. I have no problem adopting
> “err” here, but is it understood that styles will be mixed across
> components?

That surprises me:

% git grep "int .*error" | wc -l
6152

vs

% git grep "int .*err" | grep -v error | wc -l
34753
% git grep "int .*ret" | wc -l  
76584

> >> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
> >> +{
> >> + struct cs40l50_private *cs40l50 = data;
> >> + int error = 0;
> >> + u32 val;
> >> +
> >> + mutex_lock(&cs40l50->lock);
> >> +
> >> + while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
> >> + switch (val) {
> >> + case 0:
> >> + mutex_unlock(&cs40l50->lock);
> >> + dev_dbg(cs40l50->dev, "Reached end of queue\n");
> >> + return IRQ_HANDLED;
> >> + case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
> >> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
> > 
> > These all appear to be no-ops?
> 
> Correct.

Then why do the exist?

> >> + case CS40L50_MBOX_RUNTIME_SHORT:
> >> + dev_err(cs40l50->dev, "Runtime short detected\n");
> >> + error = cs40l50_error_release(cs40l50);
> >> + if (error)
> >> + goto out_mutex;
> >> + break;
> >> + default:
> >> + dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
> >> + error = -EINVAL;
> >> + goto out_mutex;
> >> + }
> >> + }
> >> +
> >> + error = -EIO;
> >> +
> >> +out_mutex:
> >> + mutex_unlock(&cs40l50->lock);
> >> +
> >> + return IRQ_RETVAL(!error);
> >> +}
> > 
> > Should the last two drivers live in drivers/mailbox?
> 
> Adopting the mailbox framework seems like an excessive amount
> of overhead for our requirements.

MFD isn't a dumping a ground for miscellaneous functionality.

MFD requests resources and registers devices.

Mailbox functionality should live in drivers/mailbox.

> >> +static irqreturn_t cs40l50_error(int irq, void *data);
> > 
> > Why is this being forward declared?
> > 
> >> +static const struct cs40l50_irq cs40l50_irqs[] = {
> >> + CS40L50_IRQ(AMP_SHORT, "Amp short", error),
> > 
> > I assume that last parameter is half of a function name.
> > 
> > Better to have 2 different structures and do 2 requests I feel.
> 
> I think I will combine the two handler functions into one, so as not
> to need the struct handler parameter, or the forward declaration.

Or the MACRO - win, win win.

> >> +{
> >> + struct device *dev = cs40l50->dev;
> >> + int error;
> >> +
> >> + mutex_init(&cs40l50->lock);
> > 
> > Don't you need to destroy this in the error path?
> 
> My understanding based on past feedback is that mutex_destroy()
> is an empty function unless mutex debugging is enabled, and there
> is no need cleanup the mutex explicitly. I will change this if you
> disagree with that feedback.

It just seems odd to create something and not tear it down.

> >> +struct cs40l50_irq {
> >> + const char *name;
> >> + int irq;
> >> + irqreturn_t (*handler)(int irq, void *data);
> >> +};
> >> +
> >> +struct cs40l50_private {
> >> + struct device *dev;
> >> + struct regmap *regmap;
> >> + struct cs_dsp dsp;
> >> + struct mutex lock;
> >> + struct gpio_desc *reset_gpio;
> >> + struct regmap_irq_chip_data *irq_data;
> >> + struct input_dev *input;
> > 
> > Where is this used?
> > 
> >> + const struct firmware *wmfw;
> > 
> > Or this.
> > 
> >> + struct cs_hap haptics;
> > 
> > Or this?
> > 
> >> + u32 devid;
> >> + u32 revid;
> > 
> > Are these used after they're set?
> 
> These are all used in the input driver, patch 4/4 of this series. If
> this is not acceptable in some way, I will change it per your
> suggestions.

Do they need to be shared with other devices?

If not, they should live where they are used.

Hi Lee and James,

On Mon, Oct 23, 2023 at 10:20:46AM +0100, Lee Jones wrote:

[...]

> > >> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> > >> +{
> > >> + int error, fractional, integer, stored;
> > > 
> > > err or ret is traditional.
> > 
> > We received feedback to change from “ret” to “error” in the input
> > subsystem, and now the opposite in MFD. I have no problem adopting
> > “err” here, but is it understood that styles will be mixed across
> > components?

FWIW, this is not an uncommon outcome for submissions that span multiple
subsystems.

> 
> That surprises me:
> 
> % git grep "int .*error" | wc -l
> 6152
> 
> vs
> 
> % git grep "int .*err" | grep -v error | wc -l
> 34753
> % git grep "int .*ret" | wc -l  
> 76584

Right, the history here is that this driver started its life in input,
where submitters have historically been asked to use 'error' for return
variables that return either zero or a negative error code. Since this
driver is no longer in input, this can easily be changed.

[...]

> > > Should the last two drivers live in drivers/mailbox?
> > 
> > Adopting the mailbox framework seems like an excessive amount
> > of overhead for our requirements.
> 
> MFD isn't a dumping a ground for miscellaneous functionality.
> 
> MFD requests resources and registers devices.
> 
> Mailbox functionality should live in drivers/mailbox.

I think this is just a misnomer; the code uses the terms "mailbox" and
"mbox" throughout because the relevant registers are named as such in
the datasheet.

Please correct me James, but I believe the datasheet uses this language
because both the host and the part itself have write access, meaning the
part may write a status code to the register after the host writes that
same register. There is no relation to IPC or the mbox subsystem.

That being said, some of the functions currently placed in this MFD,
namely those related to haptic motor properties (e.g. f0 and ReDC), do
seem more appropriate for the input/FF child device. My understanding
is that those functions serve only momentary haptic click effects and
not the I2S streaming case; please let me know if I have misunderstood.

I understand that no customer would ever build the to-be-added codec
driver _without_ the input driver, but the MFD must be generic enough
to support this case. Would a codec-only implementation use f0 and ReDC
estimation? If so, then these functions _do_ belong in the MFD, albeit
with some comments to explain their nature.

[...]

> > >> +{
> > >> + struct device *dev = cs40l50->dev;
> > >> + int error;
> > >> +
> > >> + mutex_init(&cs40l50->lock);
> > > 
> > > Don't you need to destroy this in the error path?
> > 
> > My understanding based on past feedback is that mutex_destroy()
> > is an empty function unless mutex debugging is enabled, and there
> > is no need cleanup the mutex explicitly. I will change this if you
> > disagree with that feedback.
> 
> It just seems odd to create something and not tear it down.

mutex_init() is not creating anything; the mutex struct is allocated as
part of the driver's private data, which is de-allocated as part of device
managed resources being freed when the module is unloaded.

mutex_destroy() is a NOP unless CONFIG_DEBUG_MUTEXES is set, and there are
roughly 4x fewer instances of it than mutex_init() in mainline. I recommend
not to add mutex_destroy() because it adds unnecessary tear-down paths and
remove() callbacks that wouldn't otherwise have to exist.

Kind regards,
Jeff LaBundy

Any comment that went un-replied will be adopted in the next version.

> On Oct 23, 2023, at 4:20 AM, Lee Jones <lee@kernel.org> wrote:
> 
> On Fri, 20 Oct 2023, James Ogletree wrote:
> 
>>>> +static const struct mfd_cell cs40l50_devs[] = {
>>>> + {
>>>> + .name = "cs40l50-vibra",
>>>> + },
>>> 
>>> 
>>> Where are the other devices?  Without them, it's not an MFD.
>> 
>> The driver will need to support I2S streaming to the device at some point
>> in the future, for which a codec driver will be added. I thought it better to
>> submit this as an MFD driver now, rather than as an Input driver, so as
>> not to have to move everything later.
>> 
>> Should I add the “cs40l50-codec” mfd_cell now, even though it does not
>> exist yet?
> 
> What is your timeline for this to be authored?
> 
> Does the device function well without it?

Without the codec component, one minor feature will be missing, but
the device will have no issues functioning.

The current timeline, as best as I can see it, is 3-6 months.

> 
>>>> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
>>>> +{
>>>> + int error, fractional, integer, stored;
>>> 
>>> err or ret is traditional.
>> 
>> We received feedback to change from “ret” to “error” in the input
>> subsystem, and now the opposite in MFD. I have no problem adopting
>> “err” here, but is it understood that styles will be mixed across
>> components?
> 
> That surprises me:
> 
> % git grep "int .*error" | wc -l
> 6152
> 
> vs
> 
> % git grep "int .*err" | grep -v error | wc -l
> 34753
> % git grep "int .*ret" | wc -l  
> 76584

Understood. Is it possible that “error” is a recent adoption?
Regardless, I will go ahead and use “err” for the MFD driver.

> 
>>>> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
>>>> +{
>>>> + struct cs40l50_private *cs40l50 = data;
>>>> + int error = 0;
>>>> + u32 val;
>>>> +
>>>> + mutex_lock(&cs40l50->lock);
>>>> +
>>>> + while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
>>>> + switch (val) {
>>>> + case 0:
>>>> + mutex_unlock(&cs40l50->lock);
>>>> + dev_dbg(cs40l50->dev, "Reached end of queue\n");
>>>> + return IRQ_HANDLED;
>>>> + case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
>>>> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
>>> 
>>> These all appear to be no-ops?
>> 
>> Correct.
> 
> Then why do the exist?

In my judgment it alerts the user or developer of an important
error, and in other cases it gives them insight that is useful for
understanding firmware behavior. Is this kind of visibility not
typical? Regardless, I will move it out of MFD for V5.

> 
>>>> + case CS40L50_MBOX_RUNTIME_SHORT:
>>>> + dev_err(cs40l50->dev, "Runtime short detected\n");
>>>> + error = cs40l50_error_release(cs40l50);
>>>> + if (error)
>>>> + goto out_mutex;
>>>> + break;
>>>> + default:
>>>> + dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
>>>> + error = -EINVAL;
>>>> + goto out_mutex;
>>>> + }
>>>> + }
>>>> +
>>>> + error = -EIO;
>>>> +
>>>> +out_mutex:
>>>> + mutex_unlock(&cs40l50->lock);
>>>> +
>>>> + return IRQ_RETVAL(!error);
>>>> +}
>>> 
>>> Should the last two drivers live in drivers/mailbox?
>> 
>> Adopting the mailbox framework seems like an excessive amount
>> of overhead for our requirements.
> 
> MFD isn't a dumping a ground for miscellaneous functionality.
> 

> MFD requests resources and registers devices.
> 
> Mailbox functionality should live in drivers/mailbox.

Roger that. Mailbox functionality will move out of MFD for V5.

> 
>>>> +struct cs40l50_irq {
>>>> + const char *name;
>>>> + int irq;
>>>> + irqreturn_t (*handler)(int irq, void *data);
>>>> +};
>>>> +
>>>> +struct cs40l50_private {
>>>> + struct device *dev;
>>>> + struct regmap *regmap;
>>>> + struct cs_dsp dsp;
>>>> + struct mutex lock;
>>>> + struct gpio_desc *reset_gpio;
>>>> + struct regmap_irq_chip_data *irq_data;
>>>> + struct input_dev *input;
>>> 
>>> Where is this used?
>>> 
>>>> + const struct firmware *wmfw;
>>> 
>>> Or this.
>>> 
>>>> + struct cs_hap haptics;
>>> 
>>> Or this?
>>> 
>>>> + u32 devid;
>>>> + u32 revid;
>>> 
>>> Are these used after they're set?
>> 
>> These are all used in the input driver, patch 4/4 of this series. If
>> this is not acceptable in some way, I will change it per your
>> suggestions.
> 
> Do they need to be shared with other devices?
> 
> If not, they should live where they are used.

devid and revid are shared, but the others are not. I will move them to
their proper home in V5.

Best,
James

> On Oct 23, 2023, at 8:08 PM, Jeff LaBundy <jeff@labundy.com> wrote:
> 
>>>> Should the last two drivers live in drivers/mailbox?
>>> 
>>> Adopting the mailbox framework seems like an excessive amount
>>> of overhead for our requirements.
>> 
>> MFD isn't a dumping a ground for miscellaneous functionality.
>> 
>> MFD requests resources and registers devices.
>> 
>> Mailbox functionality should live in drivers/mailbox.
> 
> I think this is just a misnomer; the code uses the terms "mailbox" and
> "mbox" throughout because the relevant registers are named as such in
> the datasheet.
> 
> Please correct me James, but I believe the datasheet uses this language
> because both the host and the part itself have write access, meaning the
> part may write a status code to the register after the host writes that
> same register. There is no relation to IPC or the mbox subsystem.
> 
> That being said, some of the functions currently placed in this MFD,
> namely those related to haptic motor properties (e.g. f0 and ReDC), do
> seem more appropriate for the input/FF child device. My understanding
> is that those functions serve only momentary haptic click effects and
> not the I2S streaming case; please let me know if I have misunderstood.
> 
> I understand that no customer would ever build the to-be-added codec
> driver _without_ the input driver, but the MFD must be generic enough
> to support this case. Would a codec-only implementation use f0 and ReDC
> estimation? If so, then these functions _do_ belong in the MFD, albeit
> with some comments to explain their nature.

Thank you for the clarifications, Jeff, and you are correct on all counts.
I see that I spoke before having a good enough grasp on the mailbox
framework. As regards the codec-only use case, they would not be used.
So those functions do belong in the input driver.

On Mon, 23 Oct 2023, Jeff LaBundy wrote:
> I understand that no customer would ever build the to-be-added codec
> driver _without_ the input driver, but the MFD must be generic enough
> to support this case. Would a codec-only implementation use f0 and ReDC
> estimation? If so, then these functions _do_ belong in the MFD, albeit
> with some comments to explain their nature.

I'm not going to be able to accept a single-function device into the
multi-function devices subsystem.  Please submit both once the codec is
ready.

> > > >> + struct device *dev = cs40l50->dev;
> > > >> + int error;
> > > >> +
> > > >> + mutex_init(&cs40l50->lock);
> > > > 
> > > > Don't you need to destroy this in the error path?
> > > 
> > > My understanding based on past feedback is that mutex_destroy()
> > > is an empty function unless mutex debugging is enabled, and there
> > > is no need cleanup the mutex explicitly. I will change this if you
> > > disagree with that feedback.
> > 
> > It just seems odd to create something and not tear it down.
> 
> mutex_init() is not creating anything; the mutex struct is allocated as
> part of the driver's private data, which is de-allocated as part of device
> managed resources being freed when the module is unloaded.
> 
> mutex_destroy() is a NOP unless CONFIG_DEBUG_MUTEXES is set, and there are
> roughly 4x fewer instances of it than mutex_init() in mainline. I recommend
> not to add mutex_destroy() because it adds unnecessary tear-down paths and
> remove() callbacks that wouldn't otherwise have to exist.

Fair enough.

Hi James,

Excellent work as always!

On Wed, Oct 18, 2023 at 05:57:23PM +0000, James Ogletree wrote:
> From: James Ogletree <james.ogletree@cirrus.com>
> 
> Introduce the cirrus haptics library which factors out
> common haptics operations used by Cirrus Logic Input
> drivers.
> 
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
>  MAINTAINERS                          |   2 +
>  drivers/input/misc/cirrus_haptics.c  | 586 +++++++++++++++++++++++++++
>  include/linux/input/cirrus_haptics.h | 121 ++++++
>  3 files changed, 709 insertions(+)
>  create mode 100644 drivers/input/misc/cirrus_haptics.c
>  create mode 100644 include/linux/input/cirrus_haptics.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 28f0ca9324b3..57daf77bf550 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4970,6 +4970,8 @@ M:	Ben Bright <ben.bright@cirrus.com>
>  L:	patches@opensource.cirrus.com
>  S:	Supported
>  F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> +F:	drivers/input/misc/cirrus*
> +F:	include/linux/input/cirrus*
>  
>  CIRRUS LOGIC DSP FIRMWARE DRIVER
>  M:	Simon Trimmer <simont@opensource.cirrus.com>
> diff --git a/drivers/input/misc/cirrus_haptics.c b/drivers/input/misc/cirrus_haptics.c
> new file mode 100644
> index 000000000000..7e539cd45167
> --- /dev/null
> +++ b/drivers/input/misc/cirrus_haptics.c
> @@ -0,0 +1,586 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Helper functions for dealing with wavetable
> + * formats and DSP interfaces used by Cirrus
> + * haptic drivers.
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + */
> +
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/input.h>
> +#include <linux/input/cirrus_haptics.h>
> +#include <linux/pm_runtime.h>
> +
> +static int cs_hap_pseq_init(struct cs_hap *haptics)
> +{
> +	struct cs_hap_pseq_op *op;
> +	int error, i, num_words;
> +	u8 operation;
> +	u32 *words;
> +
> +	if (!haptics->dsp.pseq_size || !haptics->dsp.pseq_reg)
> +		return 0;
> +
> +	INIT_LIST_HEAD(&haptics->pseq_head);

Anything that allocates or initializes an element that is normally held
in a driver's private data, like a list head or mutex, belongs in probe()
in my opinion. It's less of an issue here, but for more complex cases
where we may set something up in probe() and tear it down in remove(),
the driver is easier to maintain if helper functions such as cs_hap_pseq_init()
only manipulate or organize the data, rather than absorb additional work.

> +
> +	words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL);
> +	if (!words)
> +		return -ENOMEM;
> +
> +	error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg,
> +				 words, haptics->dsp.pseq_size);
> +	if (error)
> +		goto err_free;
> +
> +	for (i = 0; i < haptics->dsp.pseq_size; i += num_words) {
> +		operation = FIELD_GET(PSEQ_OP_MASK, words[i]);
> +		switch (operation) {
> +		case PSEQ_OP_END:
> +		case PSEQ_OP_WRITE_UNLOCK:
> +			num_words = PSEQ_OP_END_WORDS;
> +			break;
> +		case PSEQ_OP_WRITE_ADDR8:
> +		case PSEQ_OP_WRITE_H16:
> +		case PSEQ_OP_WRITE_L16:
> +			num_words = PSEQ_OP_WRITE_X16_WORDS;
> +			break;
> +		case PSEQ_OP_WRITE_FULL:
> +			num_words = PSEQ_OP_WRITE_FULL_WORDS;
> +			break;
> +		default:
> +			error = -EINVAL;
> +			dev_err(haptics->dev, "Unsupported op: %u\n", operation);
> +			goto err_free;
> +		}
> +
> +		op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL);
> +		if (!op) {
> +			error = -ENOMEM;
> +			goto err_free;
> +		}
> +
> +		op->size = num_words * sizeof(u32);
> +		memcpy(op->words, &words[i], op->size);
> +		op->offset = i * sizeof(u32);
> +		op->operation = operation;
> +		list_add(&op->list, &haptics->pseq_head);
> +
> +		if (operation == PSEQ_OP_END)
> +			break;
> +	}
> +
> +	if (operation != PSEQ_OP_END)
> +		error = -ENOENT;
> +
> +err_free:
> +	kfree(words);
> +
> +	return error;
> +}

My first thought as I reviewed this patch was that this and the lot
of pseq-related functions are not necessarily related to haptics, but
rather CS40L50 register access and housekeeping in general.

I seem to recall on L25 and friends that the the power-on sequencer,
i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory
that can play back a series of address/data pairs when the device
comes out of hibernation, and any registers written during runtime
must also be mirrored to the PSEQ for "playback" later. Is that still
the case here?

Assuming so, these functions seem like they belong in the MFD, not
an input-specific library, because they will presumably be used by
the codec driver as well, since that driver will write registers to
set BCLK/LRCK ratio, etc.

Therefore, I think it makes more sense for these functions to move to
the MFD, which can then export them for use by the input/FF and codec
children.

This leaves cirrus_haptics.* with only a few functions related to
starting and stopping work, which seem specific enough to just live
in cs40l50-vibra.c. Presumably many of those could be re-used by
the L30 down the road, but in that case I think we'd be looking to
actually re-use the L50 driver and simply add a compatible string
for L30.

I recall L30 has some overhead that L50 does not, which may make
it hairy for cs40l50-vibra.c to support both. But in that case,
you could always fork a cs40l30-vibra.c with its own compatible
string, then have the L50 MFD selectively load the correct child
based on device ID. To summarize, we should have:

* drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery,
  IRQ handling, exported PSEQ functions, etc.
* sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from
  the MFD.
* drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop
  work, also uses PSEQ library from the MFD.

And down the road, depending on complexity, maybe we also have:

* drivers/input/misc/cs40l30-vibra.c: another input/FF driver that
  includes other functionality that didn't really fit in cs40l50-vibra.c;
  also uses PSEQ library from, and is loaded by, the original L50 MFD.
  If this driver duplicates small bits of cs40l50-vibra.c, it's not the
  end of the world.

All of these files would #include include/linux/mfd/cs40l50.h. And
finally, cirrus_haptics.* simply go away. Same idea, just slightly
more scalable, and closer to common design patterns.

> +
> +static int cs_hap_pseq_find_end(struct cs_hap *haptics,
> +				struct cs_hap_pseq_op **op_end)
> +{
> +	u8 operation = PSEQ_OP_WRITE_FULL;
> +	struct cs_hap_pseq_op *op;
> +
> +	list_for_each_entry(op, &haptics->pseq_head, list) {
> +		operation = op->operation;
> +		if (operation == PSEQ_OP_END)
> +			break;
> +	}
> +
> +	if (operation != PSEQ_OP_END) {
> +		dev_err(haptics->dev, "Missing PSEQ list terminator\n");
> +		return -ENOENT;
> +	}
> +
> +	*op_end = op;
> +
> +	return 0;
> +}
> +
> +static struct cs_hap_pseq_op *cs_hap_pseq_find_op(struct cs_hap_pseq_op *match_op,
> +						  struct list_head *pseq_head)
> +{
> +	struct cs_hap_pseq_op *op;
> +
> +	list_for_each_entry(op, pseq_head, list) {
> +		if (op->operation == PSEQ_OP_END)
> +			break;

Nit: a line break here makes this easier to read IMO.

> +		if (op->operation != match_op->operation ||
> +		    op->words[0] != match_op->words[0])
> +			continue;

And here.

> +		switch (op->operation) {
> +		case PSEQ_OP_WRITE_FULL:
> +			if (FIELD_GET(GENMASK(23, 8), op->words[1]) ==
> +			    FIELD_GET(GENMASK(23, 8), match_op->words[1]))
> +				return op;
> +			break;
> +		case PSEQ_OP_WRITE_H16:
> +		case PSEQ_OP_WRITE_L16:
> +			if (FIELD_GET(GENMASK(23, 16), op->words[1]) ==
> +			    FIELD_GET(GENMASK(23, 16), match_op->words[1]))
> +				return op;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr,
> +		      u32 data, bool update, u8 op_code)
> +{
> +	struct cs_hap_pseq_op *op, *op_end, *op_new;
> +	struct cs_dsp_chunk ch;
> +	u32 pseq_bytes;
> +	int error;
> +
> +	op_new = devm_kzalloc(haptics->dev, sizeof(*op_new), GFP_KERNEL);
> +	if (!op_new)
> +		return -ENOMEM;
> +
> +	op_new->operation = op_code;
> +
> +	ch = cs_dsp_chunk((void *) op_new->words,
> +			  PSEQ_OP_WRITE_FULL_WORDS * sizeof(u32));
> +	cs_dsp_chunk_write(&ch, 8, op_code);
> +	switch (op_code) {
> +	case PSEQ_OP_WRITE_FULL:
> +		cs_dsp_chunk_write(&ch, 32, addr);
> +		cs_dsp_chunk_write(&ch, 32, data);
> +		break;
> +	case PSEQ_OP_WRITE_L16:
> +	case PSEQ_OP_WRITE_H16:
> +		cs_dsp_chunk_write(&ch, 24, addr);
> +		cs_dsp_chunk_write(&ch, 16, data);
> +		break;
> +	default:
> +		error = -EINVAL;
> +		goto op_new_free;
> +	}
> +
> +	op_new->size = cs_dsp_chunk_bytes(&ch);
> +
> +	if (update) {
> +		op = cs_hap_pseq_find_op(op_new, &haptics->pseq_head);
> +		if (!op) {
> +			error = -EINVAL;
> +			goto op_new_free;
> +		}
> +	}

It seems we are relying on the developer to remember if he or she has
already written 'addr' using a previous call to cs_hap_pseq_write(),
then set the update flag accordingly; is that accurate?

If so, that is a high risk for bugs to be introduced as the driver is
maintained. Can we not search for an existing address/data entry upon
any call to cs_hap_pseq_write() using cs_hap_pseq_find_op(), and add
or replace a new address/data entry automatically?

> +
> +	error = cs_hap_pseq_find_end(haptics, &op_end);
> +	if (error)
> +		goto op_new_free;
> +
> +	pseq_bytes = haptics->dsp.pseq_size * sizeof(u32);
> +
> +	if (pseq_bytes - op_end->offset < op_new->size) {
> +		error = -ENOMEM;
> +		goto op_new_free;
> +	}
> +
> +	if (update) {
> +		op_new->offset = op->offset;
> +	} else {
> +		op_new->offset = op_end->offset;
> +		op_end->offset += op_new->size;
> +	}
> +
> +	error = regmap_raw_write(haptics->regmap, haptics->dsp.pseq_reg +
> +				 op_new->offset, op_new->words, op_new->size);
> +	if (error)
> +		goto op_new_free;
> +
> +	if (update) {
> +		list_replace(&op->list, &op_new->list);
> +	} else {
> +		error = regmap_raw_write(haptics->regmap, haptics->dsp.pseq_reg +
> +					 op_end->offset, op_end->words,
> +					 op_end->size);
> +		if (error)
> +			goto op_new_free;
> +
> +		list_add(&op_new->list, &haptics->pseq_head);
> +	}
> +
> +	return 0;
> +
> +op_new_free:
> +	devm_kfree(haptics->dev, op_new);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(cs_hap_pseq_write);
> +
> +int cs_hap_pseq_multi_write(struct cs_hap *haptics,
> +			    const struct reg_sequence *reg_seq,
> +			    int num_regs, bool update, u8 op_code)
> +{
> +	int error, i;
> +
> +	for (i = 0; i < num_regs; i++) {
> +		error = cs_hap_pseq_write(haptics, reg_seq[i].reg,
> +					  reg_seq[i].def, update, op_code);
> +		if (error)
> +			return error;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs_hap_pseq_multi_write);
> +
> +static struct cs_hap_effect *cs_hap_find_effect(int id,
> +						struct list_head *effect_head)
> +{
> +	struct cs_hap_effect *effect;
> +
> +	list_for_each_entry(effect, effect_head, list)
> +		if (effect->id == id)
> +			return effect;
> +
> +	return NULL;
> +}
> +
> +static int cs_hap_effect_bank_set(struct cs_hap *haptics,
> +				  struct cs_hap_effect *effect,
> +				  struct ff_periodic_effect add_effect)
> +{
> +	s16 bank = add_effect.custom_data[0] & 0xffffu;
> +	unsigned int len = add_effect.custom_len;
> +
> +	if (bank >= WVFRM_BANK_NUM) {
> +		dev_err(haptics->dev, "Invalid waveform bank: %d\n", bank);
> +		return -EINVAL;
> +	}
> +
> +	effect->bank = len > CUSTOM_DATA_SIZE ? WVFRM_BANK_OWT : bank;
> +
> +	return 0;
> +}
> +
> +static int cs_hap_effect_mapping_set(struct cs_hap *haptics, u16 button,
> +				     struct cs_hap_effect *effect)
> +{
> +	u32 gpio_num, gpio_edge;
> +
> +	if (button) {
> +		gpio_num = FIELD_GET(BTN_NUM_MASK, button);
> +		gpio_edge = FIELD_GET(BTN_EDGE_MASK, button);
> +		effect->mapping = haptics->dsp.gpio_base_reg +
> +				  (gpio_num * 8) - gpio_edge;
> +
> +		return regmap_write(haptics->regmap, effect->mapping, button);
> +	}
> +
> +	effect->mapping = GPIO_MAPPING_INVALID;
> +
> +	return 0;
> +}
> +
> +static int cs_hap_effect_index_set(struct cs_hap *haptics,
> +				   struct cs_hap_effect *effect,
> +				   struct ff_periodic_effect add_effect)
> +{
> +	struct cs_hap_effect *owt_effect;
> +	u32 base_index, max_index;
> +
> +	base_index = haptics->banks[effect->bank].base_index;
> +	max_index = haptics->banks[effect->bank].max_index;
> +
> +	effect->index = base_index;
> +
> +	switch (effect->bank) {
> +	case WVFRM_BANK_OWT:
> +		list_for_each_entry(owt_effect, &haptics->effect_head, list)
> +			if (owt_effect->bank == WVFRM_BANK_OWT)
> +				effect->index++;
> +		break;
> +	case WVFRM_BANK_ROM:
> +	case WVFRM_BANK_RAM:
> +		effect->index += add_effect.custom_data[1] & 0xffffu;
> +		break;
> +	default:
> +		dev_err(haptics->dev, "Bank not supported: %d\n", effect->bank);
> +		return -EINVAL;
> +	}
> +
> +	if (effect->index > max_index || effect->index < base_index) {
> +		dev_err(haptics->dev, "Index out of bounds: %u\n", effect->index);
> +		return -ENOSPC;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cs_hap_upload_pwle(struct cs_hap *haptics,
> +			      struct cs_hap_effect *effect,
> +			      struct ff_periodic_effect add_effect)
> +{
> +	u32 len, wt_offset, wt_size_words;
> +	struct cs_hap_pwle_header header;
> +	u8 *out_data;
> +	int error;
> +
> +	error = regmap_read(haptics->regmap, haptics->dsp.owt_offset_reg,
> +			    &wt_offset);
> +	if (error)
> +		return error;
> +
> +	error = regmap_read(haptics->regmap, haptics->dsp.owt_size_reg,
> +			    &wt_size_words);
> +	if (error)
> +		return error;
> +
> +	len = 2 * add_effect.custom_len;
> +
> +	if ((wt_size_words * sizeof(u32)) < OWT_HEADER_SIZE + len)
> +		return -ENOSPC;
> +
> +	out_data = kzalloc(OWT_HEADER_SIZE + len, GFP_KERNEL);
> +	if (!out_data)
> +		return -ENOMEM;
> +
> +	header.type = add_effect.custom_data[0] == PCM_ID ? OWT_TYPE_PCM :
> +							    OWT_TYPE_PWLE;
> +	header.offset = OWT_HEADER_SIZE / sizeof(u32);
> +	header.data_words = len / sizeof(u32);
> +
> +	memcpy(out_data, &header, sizeof(header));
> +	memcpy(out_data + OWT_HEADER_SIZE, add_effect.custom_data, len);
> +
> +	error = regmap_bulk_write(haptics->regmap, haptics->dsp.owt_base_reg +
> +				  (wt_offset * sizeof(u32)), out_data,
> +				  OWT_HEADER_SIZE + len);
> +	if (error)
> +		goto err_free;
> +
> +	error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> +			     haptics->dsp.push_owt_cmd);
> +
> +err_free:
> +	kfree(out_data);
> +
> +	return error;
> +}
> +
> +static void cs_hap_add_worker(struct work_struct *work)
> +{
> +	struct cs_hap *haptics = container_of(work, struct cs_hap,
> +					      add_work);
> +	struct ff_effect add_effect = haptics->add_effect;
> +	bool is_new = false;
> +	struct cs_hap_effect *effect;
> +	int error;
> +
> +	if (haptics->runtime_pm) {
> +		error = pm_runtime_resume_and_get(haptics->dev);
> +		if (error < 0) {
> +			haptics->add_error = error;
> +			return;
> +		}
> +	}
> +
> +	mutex_lock(&haptics->lock);
> +
> +	effect = cs_hap_find_effect(add_effect.id, &haptics->effect_head);
> +	if (!effect) {
> +		effect = kzalloc(sizeof(*effect), GFP_KERNEL);
> +		if (!effect) {
> +			error = -ENOMEM;
> +			goto err_mutex;
> +		}
> +		effect->id = add_effect.id;
> +		is_new = true;
> +	}
> +
> +	error = cs_hap_effect_bank_set(haptics, effect, add_effect.u.periodic);
> +	if (error)
> +		goto err_free;
> +
> +	error = cs_hap_effect_index_set(haptics, effect, add_effect.u.periodic);
> +	if (error)
> +		goto err_free;
> +
> +	error = cs_hap_effect_mapping_set(haptics, add_effect.trigger.button,
> +					  effect);
> +	if (error)
> +		goto err_free;
> +
> +	if (effect->bank == WVFRM_BANK_OWT)
> +		error = cs_hap_upload_pwle(haptics, effect,
> +					   add_effect.u.periodic);
> +
> +err_free:
> +	if (is_new) {
> +		if (error)
> +			kfree(effect);
> +		else
> +			list_add(&effect->list, &haptics->effect_head);
> +	}
> +
> +err_mutex:
> +	mutex_unlock(&haptics->lock);
> +
> +	if (haptics->runtime_pm) {
> +		pm_runtime_mark_last_busy(haptics->dev);
> +		pm_runtime_put_autosuspend(haptics->dev);
> +	}
> +
> +	haptics->add_error = error;
> +}
> +
> +static void cs_hap_erase_worker(struct work_struct *work)
> +{
> +	struct cs_hap *haptics = container_of(work, struct cs_hap,
> +					      erase_work);
> +	int error = 0;
> +	struct cs_hap_effect *owt_effect, *erase_effect;
> +
> +	if (haptics->runtime_pm) {
> +		error = pm_runtime_resume_and_get(haptics->dev);
> +		if (error < 0) {
> +			haptics->erase_error = error;
> +			return;
> +		}
> +	}
> +
> +	mutex_lock(&haptics->lock);
> +
> +	erase_effect = cs_hap_find_effect(haptics->erase_effect->id,
> +					  &haptics->effect_head);
> +	if (!erase_effect) {
> +		dev_err(haptics->dev, "Effect to erase does not exist\n");
> +		error = -EINVAL;
> +		goto err_mutex;
> +	}
> +
> +	if (erase_effect->mapping != GPIO_MAPPING_INVALID) {
> +		error = regmap_write(haptics->regmap, erase_effect->mapping,
> +				     GPIO_DISABLE);
> +		if (error)
> +			goto err_mutex;
> +	}
> +
> +	if (erase_effect->bank == WVFRM_BANK_OWT) {
> +		error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> +				     haptics->dsp.delete_owt_cmd |
> +				     erase_effect->index);
> +		if (error)
> +			goto err_mutex;
> +
> +		list_for_each_entry(owt_effect, &haptics->effect_head, list)
> +			if (owt_effect->bank == WVFRM_BANK_OWT &&
> +			    owt_effect->index > erase_effect->index)
> +				owt_effect->index--;
> +	}
> +
> +	list_del(&erase_effect->list);
> +	kfree(erase_effect);
> +
> +err_mutex:
> +	mutex_unlock(&haptics->lock);
> +
> +	if (haptics->runtime_pm) {
> +		pm_runtime_mark_last_busy(haptics->dev);
> +		pm_runtime_put_autosuspend(haptics->dev);
> +	}
> +
> +	haptics->erase_error = error;
> +}
> +
> +static void cs_hap_vibe_start_worker(struct work_struct *work)
> +{
> +	struct cs_hap *haptics = container_of(work, struct cs_hap,
> +					      vibe_start_work);
> +	struct cs_hap_effect *effect;
> +	int error;
> +
> +	if (haptics->runtime_pm) {
> +		error = pm_runtime_resume_and_get(haptics->dev);
> +		if (error < 0) {
> +			haptics->start_error = error;
> +			return;
> +		}
> +	}
> +
> +	mutex_lock(&haptics->lock);
> +
> +	effect = cs_hap_find_effect(haptics->start_effect->id,
> +				    &haptics->effect_head);
> +	if (effect) {
> +		error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> +				     effect->index);
> +	} else {
> +		dev_err(haptics->dev, "Effect to start does not exist\n");
> +		error = -EINVAL;
> +	}
> +
> +	mutex_unlock(&haptics->lock);
> +
> +	if (haptics->runtime_pm) {
> +		pm_runtime_mark_last_busy(haptics->dev);
> +		pm_runtime_put_autosuspend(haptics->dev);
> +	}
> +
> +	haptics->start_error = error;
> +}
> +
> +static void cs_hap_vibe_stop_worker(struct work_struct *work)
> +{
> +	struct cs_hap *haptics = container_of(work, struct cs_hap,
> +					      vibe_stop_work);
> +	int error;
> +
> +	if (haptics->runtime_pm) {
> +		error = pm_runtime_resume_and_get(haptics->dev);
> +		if (error < 0) {
> +			haptics->stop_error = error;
> +			return;
> +		}
> +	}
> +
> +	mutex_lock(&haptics->lock);
> +	error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> +			     haptics->dsp.stop_cmd);
> +	mutex_unlock(&haptics->lock);
> +
> +	if (haptics->runtime_pm) {
> +		pm_runtime_mark_last_busy(haptics->dev);
> +		pm_runtime_put_autosuspend(haptics->dev);
> +	}
> +
> +	haptics->stop_error = error;

This seems like another argument for not separating the input/FF child
from the meat of the driver; it just seems messy to pass around error
codes within a driver's private data like this.

That being said, where are start_error and stop_error used? I didn't
see them in the input/FF child. We should only introduce code that has
at least one user.

> +}
> +
> +int cs_hap_init(struct cs_hap *haptics)
> +{
> +	haptics->vibe_wq = alloc_ordered_workqueue("vibe_wq", 0);
> +	if (!haptics->vibe_wq)
> +		return -ENOMEM;
> +
> +	mutex_init(&haptics->lock);
> +
> +	INIT_WORK(&haptics->vibe_start_work, cs_hap_vibe_start_worker);
> +	INIT_WORK(&haptics->vibe_stop_work, cs_hap_vibe_stop_worker);
> +	INIT_WORK(&haptics->erase_work, cs_hap_erase_worker);
> +	INIT_WORK(&haptics->add_work, cs_hap_add_worker);
> +
> +	return cs_hap_pseq_init(haptics);
> +}
> +EXPORT_SYMBOL_GPL(cs_hap_init);
> +
> +void cs_hap_remove(struct cs_hap *haptics)
> +{
> +	flush_workqueue(haptics->vibe_wq);
> +	destroy_workqueue(haptics->vibe_wq);
> +}
> +EXPORT_SYMBOL_GPL(cs_hap_remove);
> +
> +MODULE_DESCRIPTION("Cirrus Logic Haptics Support");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/input/cirrus_haptics.h b/include/linux/input/cirrus_haptics.h
> new file mode 100644
> index 000000000000..42f6afed7944
> --- /dev/null
> +++ b/include/linux/input/cirrus_haptics.h
> @@ -0,0 +1,121 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Helper functions for dealing with wavetable
> + * formats and DSP interfaces used by Cirrus
> + * haptic drivers.
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + */
> +
> +#ifndef __CIRRUS_HAPTICS_H
> +#define __CIRRUS_HAPTICS_H
> +
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Power-on write sequencer */
> +#define PSEQ_OP_MASK			GENMASK(23, 16)
> +#define PSEQ_OP_SHIFT			16
> +#define PSEQ_OP_WRITE_FULL_WORDS	3
> +#define PSEQ_OP_WRITE_X16_WORDS		2
> +#define PSEQ_OP_END_WORDS		1
> +#define PSEQ_OP_WRITE_FULL		0x00
> +#define PSEQ_OP_WRITE_ADDR8		0x02
> +#define PSEQ_OP_WRITE_L16		0x04
> +#define PSEQ_OP_WRITE_H16		0x05
> +#define PSEQ_OP_WRITE_UNLOCK		0xFD
> +#define PSEQ_OP_END			0xFF
> +
> +/* Open wavetable */
> +#define OWT_HEADER_SIZE		12
> +#define OWT_TYPE_PCM		8
> +#define OWT_TYPE_PWLE		12
> +#define PCM_ID			0x0
> +#define CUSTOM_DATA_SIZE	2
> +
> +/* GPIO */
> +#define BTN_NUM_MASK		GENMASK(14, 12)
> +#define BTN_EDGE_MASK		BIT(15)
> +#define GPIO_MAPPING_INVALID	0
> +#define GPIO_DISABLE		0x1FF
> +
> +enum cs_hap_bank_type {
> +	WVFRM_BANK_RAM,
> +	WVFRM_BANK_ROM,
> +	WVFRM_BANK_OWT,
> +	WVFRM_BANK_NUM,
> +};
> +
> +struct cs_hap_pseq_op {
> +	struct list_head list;
> +	u32 words[3];
> +	u16 offset;
> +	u8 operation;
> +	u8 size;
> +};
> +
> +struct cs_hap_effect {
> +	enum cs_hap_bank_type bank;
> +	struct list_head list;
> +	u32 mapping;
> +	u32 index;
> +	int id;
> +};
> +
> +struct cs_hap_pwle_header {
> +	u32 type;
> +	u32 data_words;
> +	u32 offset;
> +} __packed;
> +
> +struct cs_hap_bank {
> +	enum cs_hap_bank_type bank;
> +	u32 base_index;
> +	u32 max_index;
> +};
> +
> +struct cs_hap_dsp {
> +	u32 gpio_base_reg;
> +	u32 owt_offset_reg;
> +	u32 owt_size_reg;
> +	u32 owt_base_reg;
> +	u32 mailbox_reg;
> +	u32 pseq_reg;
> +	u32 push_owt_cmd;
> +	u32 delete_owt_cmd;
> +	u32 stop_cmd;
> +	u32 pseq_size;
> +};
> +
> +struct cs_hap {
> +	struct regmap *regmap;
> +	struct mutex lock;
> +	struct device *dev;
> +	struct list_head pseq_head;
> +	struct cs_hap_bank *banks;
> +	struct cs_hap_dsp dsp;
> +	struct workqueue_struct *vibe_wq;
> +	struct work_struct vibe_start_work;
> +	struct work_struct vibe_stop_work;
> +	struct work_struct erase_work;
> +	struct work_struct add_work;
> +	struct ff_effect *start_effect;
> +	struct ff_effect *erase_effect;
> +	struct ff_effect add_effect;
> +	struct list_head effect_head;
> +	int erase_error;
> +	int start_error;
> +	int stop_error;
> +	int add_error;
> +	bool runtime_pm;
> +};
> +
> +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr,
> +		      u32 data, bool update, u8 op_code);
> +int cs_hap_pseq_multi_write(struct cs_hap *haptics,
> +			    const struct reg_sequence *reg_seq,
> +			    int num_regs, bool update, u8 op_code);
> +int cs_hap_init(struct cs_hap *haptics);
> +void cs_hap_remove(struct cs_hap *haptics);
> +
> +#endif
> -- 
> 2.25.1
> 

Kind regards,
Jeff LaBundy

Hi James,

Just a few trailing comments on top of Lee's feedback.

On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
> From: James Ogletree <james.ogletree@cirrus.com>
> 
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
> 
> The MFD component registers and initializes the device.
> 
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
>  MAINTAINERS                 |   2 +
>  drivers/mfd/Kconfig         |  30 +++
>  drivers/mfd/Makefile        |   4 +
>  drivers/mfd/cs40l50-core.c  | 443 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/cs40l50-i2c.c   |  69 ++++++
>  drivers/mfd/cs40l50-spi.c   |  68 ++++++
>  include/linux/mfd/cs40l50.h | 198 ++++++++++++++++
>  7 files changed, 814 insertions(+)
>  create mode 100644 drivers/mfd/cs40l50-core.c
>  create mode 100644 drivers/mfd/cs40l50-i2c.c
>  create mode 100644 drivers/mfd/cs40l50-spi.c
>  create mode 100644 include/linux/mfd/cs40l50.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 57daf77bf550..08e1e9695d49 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4971,7 +4971,9 @@ L:	patches@opensource.cirrus.com
>  S:	Supported
>  F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
>  F:	drivers/input/misc/cirrus*
> +F:	drivers/mfd/cs40l*
>  F:	include/linux/input/cirrus*
> +F:	include/linux/mfd/cs40l*
>  
>  CIRRUS LOGIC DSP FIRMWARE DRIVER
>  M:	Simon Trimmer <simont@opensource.cirrus.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b93856de432..a133d04a7859 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2187,6 +2187,36 @@ config MCP_UCB1200_TS
>  
>  endmenu
>  
> +config MFD_CS40L50_CORE
> +	tristate
> +	select MFD_CORE
> +	select CS_DSP
> +	select REGMAP_IRQ
> +
> +config MFD_CS40L50_I2C
> +	tristate "Cirrus Logic CS40L50 (I2C)"
> +	select REGMAP_I2C
> +	select MFD_CS40L50_CORE
> +	depends on I2C
> +	help
> +	  Select this to support the Cirrus Logic CS40L50 Haptic
> +	  Driver over I2C.
> +
> +	  This driver can be built as a module. If built as a module it will be
> +	  called "cs40l50-i2c".
> +
> +config MFD_CS40L50_SPI
> +	tristate "Cirrus Logic CS40L50 (SPI)"
> +	select REGMAP_SPI
> +	select MFD_CS40L50_CORE
> +	depends on SPI
> +	help
> +	  Select this to support the Cirrus Logic CS40L50 Haptic
> +	  Driver over SPI.
> +
> +	  This driver can be built as a module. If built as a module it will be
> +	  called "cs40l50-spi".
> +
>  config MFD_VEXPRESS_SYSREG
>  	tristate "Versatile Express System Registers"
>  	depends on VEXPRESS_CONFIG && GPIOLIB
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..3b1a43b3acaf 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -95,6 +95,10 @@ obj-$(CONFIG_MFD_MADERA)	+= madera.o
>  obj-$(CONFIG_MFD_MADERA_I2C)	+= madera-i2c.o
>  obj-$(CONFIG_MFD_MADERA_SPI)	+= madera-spi.o
>  
> +obj-$(CONFIG_MFD_CS40L50_CORE)	+= cs40l50-core.o
> +obj-$(CONFIG_MFD_CS40L50_I2C)	+= cs40l50-i2c.o
> +obj-$(CONFIG_MFD_CS40L50_SPI)	+= cs40l50-spi.o
> +
>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> diff --git a/drivers/mfd/cs40l50-core.c b/drivers/mfd/cs40l50-core.c
> new file mode 100644
> index 000000000000..f1eadd80203a
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-core.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +static const struct mfd_cell cs40l50_devs[] = {
> +	{
> +		.name = "cs40l50-vibra",
> +	},
> +};

I also recommend including the codec driver, especially because it will
force you to think what core components belong in the MFD (e.g. PSEQ
housekeeping as per the other thread).

If L50 shares a die with an audio amp that has its own codec driver,
finding a way to make it work as the codec child of this MFD would be
a beautiful solution, since presumably that audio amp has to manage the
PSEQ as well?

I'm sure lining up the audio and haptic amps is a lot messier than it
sounds in real life; maybe something to think about for next gen though.
For now, even an extremely simple codec driver should suffice.

> +
> +const struct regmap_config cs40l50_regmap = {
> +	.reg_bits =		32,
> +	.reg_stride =		4,
> +	.val_bits =		32,
> +	.reg_format_endian =	REGMAP_ENDIAN_BIG,
> +	.val_format_endian =	REGMAP_ENDIAN_BIG,
> +};
> +EXPORT_SYMBOL_GPL(cs40l50_regmap);
> +
> +static struct regulator_bulk_data cs40l50_supplies[] = {
> +	{
> +		.supply = "vp",
> +	},
> +	{
> +		.supply = "vio",
> +	},
> +};
> +
> +static int cs40l50_handle_f0_est_done(struct cs40l50_private *cs40l50)
> +{
> +	u32 f_zero;
> +	int error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_F0_ESTIMATION, &f_zero);
> +	if (error)
> +		return error;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_F0_STORED, f_zero);
> +}
> +
> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> +{
> +	int error, fractional, integer, stored;
> +	u32 redc;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_RE_EST_STATUS, &redc);
> +	if (error)
> +		return error;
> +
> +	error = regmap_write(cs40l50->regmap, CS40L50_REDC_ESTIMATION, redc);
> +	if (error)
> +		return error;
> +
> +	/* Convert from Q8.15 to (Q7.17 * 29/240) */
> +	integer = ((redc >> 15) & 0xFF) << 17;
> +	fractional = (redc & 0x7FFF) * 4;
> +	stored = (integer | fractional) * 29 / 240;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_REDC_STORED, stored);
> +}
> +
> +static int cs40l50_error_release(struct cs40l50_private *cs40l50)
> +{
> +	int error;
> +
> +	error = regmap_write(cs40l50->regmap, CS40L50_ERR_RLS,
> +			     CS40L50_GLOBAL_ERR_RLS);
> +	if (error)
> +		return error;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_ERR_RLS, 0);
> +}
> +
> +static int cs40l50_mailbox_read_next(struct cs40l50_private *cs40l50, u32 *val)
> +{
> +	u32 rd_ptr, wt_ptr;
> +	int error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_WT, &wt_ptr);
> +	if (error)
> +		return error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, &rd_ptr);
> +	if (error)
> +		return error;
> +
> +	if (wt_ptr == rd_ptr) {
> +		*val = 0;
> +		return 0;
> +	}
> +
> +	error = regmap_read(cs40l50->regmap, rd_ptr, val);
> +	if (error)
> +		return error;
> +
> +	rd_ptr += sizeof(u32);
> +	if (rd_ptr > CS40L50_MBOX_QUEUE_END)
> +		rd_ptr = CS40L50_MBOX_QUEUE_BASE;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, rd_ptr);
> +}
> +
> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +	int error = 0;
> +	u32 val;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
> +		switch (val) {
> +		case 0:
> +			mutex_unlock(&cs40l50->lock);

Interleaving the mutex through the switch statement like this is dangerous;
it kind of looks like a zipper. It makes the code difficult to follow and
can lead to bugs in case the switch statement grows over time.

In general, we want to lock as little code as possible; maybe the mutex
needs to move inside the helper functions called from here.

> +			dev_dbg(cs40l50->dev, "Reached end of queue\n");
> +			return IRQ_HANDLED;
> +		case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_TRIGGER_MBOX:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_MBOX\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_TRIGGER_I2S:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_I2S\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_COMPLETE_MBOX:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_MBOX\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_COMPLETE_GPIO:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_GPIO\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_COMPLETE_I2S:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_I2S\n");
> +			break;
> +		case CS40L50_MBOX_F0_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_START\n");
> +			break;
> +		case CS40L50_MBOX_F0_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_DONE\n");
> +			error = cs40l50_handle_f0_est_done(cs40l50);
> +			if (error)
> +				goto out_mutex;
> +			break;
> +		case CS40L50_MBOX_REDC_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_START\n");
> +			break;
> +		case CS40L50_MBOX_REDC_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_DONE\n");
> +			error = cs40l50_handle_redc_est_done(cs40l50);
> +			if (error)
> +				goto out_mutex;
> +			break;
> +		case CS40L50_MBOX_LE_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_START\n");
> +			break;
> +		case CS40L50_MBOX_LE_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_DONE\n");
> +			break;
> +		case CS40L50_MBOX_AWAKE:
> +			dev_dbg(cs40l50->dev, "Mailbox: AWAKE\n");
> +			break;
> +		case CS40L50_MBOX_INIT:
> +			dev_dbg(cs40l50->dev, "Mailbox: INIT\n");
> +			break;
> +		case CS40L50_MBOX_ACK:
> +			dev_dbg(cs40l50->dev, "Mailbox: ACK\n");
> +			break;
> +		case CS40L50_MBOX_ERR_EVENT_UNMAPPED:
> +			dev_err(cs40l50->dev, "Unmapped event\n");
> +			break;
> +		case CS40L50_MBOX_ERR_EVENT_MODIFY:
> +			dev_err(cs40l50->dev, "Failed to modify event index\n");
> +			break;
> +		case CS40L50_MBOX_ERR_NULLPTR:
> +			dev_err(cs40l50->dev, "Null pointer\n");
> +			break;
> +		case CS40L50_MBOX_ERR_BRAKING:
> +			dev_err(cs40l50->dev, "Braking not in progress\n");
> +			break;
> +		case CS40L50_MBOX_ERR_INVAL_SRC:
> +			dev_err(cs40l50->dev, "Suspend/resume invalid source\n");
> +			break;
> +		case CS40L50_MBOX_ERR_ENABLE_RANGE:
> +			dev_err(cs40l50->dev, "GPIO enable out of range\n");
> +			break;
> +		case CS40L50_MBOX_ERR_GPIO_UNMAPPED:
> +			dev_err(cs40l50->dev, "GPIO not mapped\n");
> +			break;
> +		case CS40L50_MBOX_ERR_ISR_RANGE:
> +			dev_err(cs40l50->dev, "GPIO ISR out of range\n");
> +			break;
> +		case CS40L50_MBOX_PERMANENT_SHORT:
> +			dev_crit(cs40l50->dev, "Permanent short detected\n");
> +			break;
> +		case CS40L50_MBOX_RUNTIME_SHORT:
> +			dev_err(cs40l50->dev, "Runtime short detected\n");
> +			error = cs40l50_error_release(cs40l50);
> +			if (error)
> +				goto out_mutex;
> +			break;
> +		default:
> +			dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
> +			error = -EINVAL;
> +			goto out_mutex;
> +		}
> +	}

I don't think we need such a large chunk of code just to translate the mailbox
codes to human-readable strings; leave that to the datasheet. Just print the
numeric value.

> +
> +	error = -EIO;
> +
> +out_mutex:
> +	mutex_unlock(&cs40l50->lock);
> +
> +	return IRQ_RETVAL(!error);
> +}
> +
> +static irqreturn_t cs40l50_error(int irq, void *data);
> +
> +static const struct cs40l50_irq cs40l50_irqs[] = {
> +	CS40L50_IRQ(AMP_SHORT,		"Amp short",		error),
> +	CS40L50_IRQ(VIRT2_MBOX,		"Mailbox",		process_mbox),
> +	CS40L50_IRQ(TEMP_ERR,		"Overtemperature",	error),
> +	CS40L50_IRQ(BST_UVP,		"Boost undervoltage",	error),
> +	CS40L50_IRQ(BST_SHORT,		"Boost short",		error),
> +	CS40L50_IRQ(BST_ILIMIT,		"Boost current limit",	error),
> +	CS40L50_IRQ(UVLO_VDDBATT,	"Boost UVLO",		error),
> +	CS40L50_IRQ(GLOBAL_ERROR,	"Global",		error),
> +};
> +
> +static irqreturn_t cs40l50_error(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);
> +
> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static const struct regmap_irq cs40l50_reg_irqs[] = {
> +	CS40L50_REG_IRQ(IRQ1_INT_1,	AMP_SHORT),
> +	CS40L50_REG_IRQ(IRQ1_INT_2,	VIRT2_MBOX),
> +	CS40L50_REG_IRQ(IRQ1_INT_8,	TEMP_ERR),
> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_UVP),
> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_SHORT),
> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_ILIMIT),
> +	CS40L50_REG_IRQ(IRQ1_INT_10,	UVLO_VDDBATT),
> +	CS40L50_REG_IRQ(IRQ1_INT_18,	GLOBAL_ERROR),
> +};
> +
> +static struct regmap_irq_chip cs40l50_irq_chip = {
> +	.name =			"CS40L50 IRQ Controller",
> +
> +	.status_base =		CS40L50_IRQ1_INT_1,
> +	.mask_base =		CS40L50_IRQ1_MASK_1,
> +	.ack_base =		CS40L50_IRQ1_INT_1,
> +	.num_regs =		22,
> +
> +	.irqs =			cs40l50_reg_irqs,
> +	.num_irqs =		ARRAY_SIZE(cs40l50_reg_irqs),
> +
> +	.runtime_pm =		true,
> +};
> +
> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
> +{
> +	struct device *dev = cs40l50->dev;
> +	int error, i, irq;
> +
> +	error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
> +					 IRQF_ONESHOT | IRQF_SHARED, 0,
> +					 &cs40l50_irq_chip, &cs40l50->irq_data);
> +	if (error)
> +		return error;
> +
> +	for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
> +		irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
> +		if (irq < 0) {
> +			dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
> +			return irq;
> +		}
> +
> +		error = devm_request_threaded_irq(dev, irq, NULL,
> +						  cs40l50_irqs[i].handler,
> +						  IRQF_ONESHOT | IRQF_SHARED,
> +						  cs40l50_irqs[i].name, cs40l50);
> +		if (error) {
> +			dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
> +			return error;
> +		}
> +	}

This is kind of an uncommon design pattern; if anyone reads /proc/interrupts
on their system, it's going to be full of L50 interrupts. Normally we declare
a single IRQ, read the status register(s) from inside its handler and then
act accordingly.

What is the motivation for having one handler per interrupt status bit? If
multiple bits are set at once, does the register get read multiple times and
if so, does doing so clear any pending status? Or are the status registers
write-to-clear instead of read-to-clear?

> +
> +	return 0;
> +}
> +
> +static int cs40l50_part_num_resolve(struct cs40l50_private *cs40l50)
> +{
> +	struct device *dev = cs40l50->dev;
> +	int error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_DEVID, &cs40l50->devid);
> +	if (error)
> +		return error;
> +
> +	if (cs40l50->devid != CS40L50_DEVID_A) {
> +		dev_err(dev, "Invalid device ID: %#010X\n", cs40l50->devid);
> +		return -EINVAL;
> +	}
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_REVID, &cs40l50->revid);
> +	if (error)
> +		return error;
> +
> +	if (cs40l50->revid != CS40L50_REVID_B0) {
> +		dev_err(dev, "Invalid revision: %#04X\n", cs40l50->revid);
> +		return -EINVAL;
> +	}

I recommend this be '<' and not '!=' so that the driver isn't immediately
broken if a backwards-compatible metal fix hits the market (e.g. B1).

> +
> +	dev_info(dev, "Cirrus Logic CS40L50 revision %02X\n", cs40l50->revid);
> +
> +	return 0;
> +}
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50)
> +{
> +	struct device *dev = cs40l50->dev;
> +	int error;
> +
> +	mutex_init(&cs40l50->lock);
> +
> +	cs40l50->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(cs40l50->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(cs40l50->reset_gpio),
> +				     "Failed getting reset GPIO\n");
> +
> +	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(cs40l50_supplies),
> +					cs40l50_supplies);
> +	if (error)
> +		goto err_reset;

You shouldn't have to reset the device here; by initializing the GPIO as
GPIOD_OUT_HIGH, it is already asserted, which is required while the rails
are in an unknown state.

If GPIOD_OUT_HIGH is 1.8 V and not GND on your board, then the polarity
specified in your dts is backwards. gpiod is a logical API, not a physical
API. HIGH/1 = asserted (GND for active low when configured properly in
dts); LOW/0 = deasserted. Please check the flags in 'interrupts'.

> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies),
> +				      cs40l50_supplies);
> +	if (error)
> +		goto err_reset;

And here.

> +
> +	usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 100);
> +
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);

This confirms your dts is backwards. To drive an active-low reset high using
the gpiod API, you must write zero here. Fixing this will allow you to get
rid of the goto label.

> +
> +	usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 1000);
> +
> +	pm_runtime_set_autosuspend_delay(cs40l50->dev, CS40L50_AUTOSUSPEND_MS);
> +	pm_runtime_use_autosuspend(cs40l50->dev);
> +	pm_runtime_set_active(cs40l50->dev);
> +	pm_runtime_get_noresume(cs40l50->dev);
> +	devm_pm_runtime_enable(cs40l50->dev);
> +
> +	error = cs40l50_part_num_resolve(cs40l50);
> +	if (error)
> +		goto err_supplies;
> +
> +	error = cs40l50_irq_init(cs40l50);
> +	if (error)
> +		goto err_supplies;
> +
> +	error = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cs40l50_devs,
> +				     ARRAY_SIZE(cs40l50_devs), NULL, 0, NULL);
> +	if (error)
> +		goto err_supplies;
> +
> +	pm_runtime_mark_last_busy(cs40l50->dev);
> +	pm_runtime_put_autosuspend(cs40l50->dev);
> +
> +	return 0;
> +
> +err_supplies:
> +	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);

I recommend moving the disable call to a devm_add_action_or_reset callback;
this will save you the trouble of having to disable the regulators in the
error path or a remove callback, which can go away.

> +err_reset:
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);

This won't be necessary once you fix the polarity in your dts.

> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_probe);
> +
> +int cs40l50_remove(struct cs40l50_private *cs40l50)
> +{
> +	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_remove);
> +
> +static int cs40l50_runtime_suspend(struct device *dev)
> +{
> +	struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX, CS40L50_ALLOW_HIBER);
> +}
> +
> +static int cs40l50_runtime_resume(struct device *dev)
> +{
> +	struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> +	int error, i;
> +	u32 val;
> +
> +	/* Device NAKs when exiting hibernation, so optionally retry here. */
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX,
> +				     CS40L50_PREVENT_HIBER);
> +		if (!error)
> +			break;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	for (; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = regmap_read(cs40l50->regmap, CS40L50_DSP_MBOX, &val);
> +		if (!error && val == 0)
> +			return 0;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	return error ? error : -ETIMEDOUT;

This is a style preference, but it's perfectly legal to write 'return error ? : ...

> +}
> +
> +EXPORT_GPL_DEV_PM_OPS(cs40l50_pm_ops) = {
> +	RUNTIME_PM_OPS(cs40l50_runtime_suspend, cs40l50_runtime_resume, NULL)
> +};
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-i2c.c b/drivers/mfd/cs40l50-i2c.c
> new file mode 100644
> index 000000000000..be1b130eb94b
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-i2c.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 I2C Driver
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/cs40l50.h>
> +
> +static int cs40l50_i2c_probe(struct i2c_client *client,
> +			     const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct cs40l50_private *cs40l50;
> +
> +	cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> +	if (!cs40l50)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, cs40l50);
> +
> +	cs40l50->regmap = devm_regmap_init_i2c(client, &cs40l50_regmap);
> +	if (IS_ERR(cs40l50->regmap))
> +		return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> +				     "Failed to initialize register map\n");
> +
> +	cs40l50->dev = dev;
> +	cs40l50->irq = client->irq;
> +
> +	return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_i2c_remove(struct i2c_client *client)
> +{
> +	struct cs40l50_private *cs40l50 = i2c_get_clientdata(client);
> +
> +	cs40l50_remove(cs40l50);
> +}
> +
> +static const struct i2c_device_id cs40l50_id_i2c[] = {
> +	{"cs40l50", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, cs40l50_id_i2c);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> +	{ .compatible = "cirrus,cs40l50" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct i2c_driver cs40l50_i2c_driver = {
> +	.driver = {
> +		.name = "cs40l50",
> +		.of_match_table = cs40l50_of_match,
> +		.pm = pm_ptr(&cs40l50_pm_ops),
> +	},
> +	.id_table = cs40l50_id_i2c,
> +	.probe = cs40l50_i2c_probe,
> +	.remove = cs40l50_i2c_remove,
> +};
> +
> +module_i2c_driver(cs40l50_i2c_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 I2C Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-spi.c b/drivers/mfd/cs40l50-spi.c
> new file mode 100644
> index 000000000000..8311d48efedf
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-spi.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 SPI Driver
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/input/cs40l50.h>
> +#include <linux/mfd/spi.h>
> +
> +static int cs40l50_spi_probe(struct spi_device *spi)
> +{
> +	struct cs40l50_private *cs40l50;
> +	struct device *dev = &spi->dev;
> +
> +	cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> +	if (!cs40l50)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, cs40l50);
> +
> +	cs40l50->regmap = devm_regmap_init_spi(spi, &cs40l50_regmap);
> +	if (IS_ERR(cs40l50->regmap))
> +		return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> +				     "Failed to initialize register map\n");
> +
> +	cs40l50->dev = dev;
> +	cs40l50->irq = spi->irq;
> +
> +	return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_spi_remove(struct spi_device *spi)
> +{
> +	struct cs40l50_private *cs40l50 = spi_get_drvdata(spi);
> +
> +	cs40l50_remove(cs40l50);
> +}
> +
> +static const struct spi_device_id cs40l50_id_spi[] = {
> +	{"cs40l50", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, cs40l50_id_spi);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> +	{ .compatible = "cirrus,cs40l50" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct spi_driver cs40l50_spi_driver = {
> +	.driver = {
> +		.name = "cs40l50",
> +		.of_match_table = cs40l50_of_match,
> +		.pm = pm_ptr(&cs40l50_pm_ops),
> +	},
> +	.id_table = cs40l50_id_spi,
> +	.probe = cs40l50_spi_probe,
> +	.remove = cs40l50_spi_remove,
> +};
> +
> +module_spi_driver(cs40l50_spi_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 SPI Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/cs40l50.h b/include/linux/mfd/cs40l50.h
> new file mode 100644
> index 000000000000..c625a999a5ae
> --- /dev/null
> +++ b/include/linux/mfd/cs40l50.h
> @@ -0,0 +1,198 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#ifndef __CS40L50_H__
> +#define __CS40L50_H__
> +
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/input.h>
> +#include <linux/input/cirrus_haptics.h>
> +#include <linux/interrupt.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Power Supply Configuration */
> +#define CS40L50_BLOCK_ENABLES2			0x201C
> +#define CS40L50_ERR_RLS				0x2034
> +#define CS40L50_PWRMGT_CTL			0x2900
> +#define CS40L50_BST_LPMODE_SEL			0x3810
> +#define CS40L50_DCM_LOW_POWER		0x1
> +#define CS40L50_OVERTEMP_WARN		0x4000010
> +
> +/* Interrupts */
> +#define CS40L50_IRQ1_INT_1			0xE010
> +#define CS40L50_IRQ1_INT_2			0xE014
> +#define CS40L50_IRQ1_INT_8			0xE02C
> +#define CS40L50_IRQ1_INT_9			0xE030
> +#define CS40L50_IRQ1_INT_10			0xE034
> +#define CS40L50_IRQ1_INT_18			0xE054
> +#define CS40L50_IRQ1_MASK_1			0xE090
> +#define CS40L50_IRQ1_MASK_2			0xE094
> +#define CS40L50_IRQ1_MASK_20			0xE0DC
> +#define CS40L50_IRQ_MASK_2_OVERRIDE	0xFFDF7FFF
> +#define CS40L50_IRQ_MASK_20_OVERRIDE	0x15C01000
> +#define CS40L50_AMP_SHORT_MASK		BIT(31)
> +#define CS40L50_VIRT2_MBOX_MASK		BIT(21)
> +#define CS40L50_TEMP_ERR_MASK		BIT(31)
> +#define CS40L50_BST_UVP_MASK		BIT(6)
> +#define CS40L50_BST_SHORT_MASK		BIT(7)
> +#define CS40L50_BST_ILIMIT_MASK		BIT(8)
> +#define CS40L50_UVLO_VDDBATT_MASK	BIT(16)
> +#define CS40L50_GLOBAL_ERROR_MASK	BIT(15)
> +#define CS40L50_GLOBAL_ERR_RLS		BIT(11)
> +#define CS40L50_IRQ(_irq, _name, _hand)		\
> +	{					\
> +		.irq = CS40L50_ ## _irq ## _IRQ,\
> +		.name = _name,			\
> +		.handler = cs40l50_ ## _hand,	\
> +	}
> +#define CS40L50_REG_IRQ(_reg, _irq)					\
> +	[CS40L50_ ## _irq ## _IRQ] = {					\
> +		.reg_offset = (CS40L50_ ## _reg) - CS40L50_IRQ1_INT_1,	\
> +		.mask = CS40L50_ ## _irq ## _MASK			\
> +	}
> +
> +/* Mailbox Inbound Commands */
> +#define CS40L50_RAM_BANK_INDEX_START	0x1000000
> +#define CS40L50_RTH_INDEX_START		0x1400000
> +#define CS40L50_RTH_INDEX_END		0x1400001
> +#define CS40L50_ROM_BANK_INDEX_START	0x1800000
> +#define CS40L50_ROM_BANK_INDEX_END	0x180001A
> +#define CS40L50_PREVENT_HIBER		0x2000003
> +#define CS40L50_ALLOW_HIBER		0x2000004
> +#define CS40L50_OWT_PUSH		0x3000008
> +#define CS40L50_STOP_PLAYBACK		0x5000000
> +#define CS40L50_OWT_DELETE		0xD000000
> +
> +/* Mailbox Outbound Commands */
> +#define CS40L50_MBOX_QUEUE_BASE				0x11004
> +#define CS40L50_MBOX_QUEUE_END				0x1101C
> +#define CS40L50_DSP_MBOX				0x11020
> +#define CS40L50_MBOX_QUEUE_WT				0x28042C8
> +#define CS40L50_MBOX_QUEUE_RD				0x28042CC
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_MBOX	0x1000000
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_GPIO	0x1000001
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_I2S	0x1000002
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_MBOX	0x1000010
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_GPIO	0x1000011
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_I2S		0x1000012
> +#define CS40L50_MBOX_INIT			0x2000000
> +#define CS40L50_MBOX_AWAKE			0x2000002
> +#define CS40L50_MBOX_F0_EST_START		0x7000011
> +#define CS40L50_MBOX_F0_EST_DONE		0x7000021
> +#define CS40L50_MBOX_REDC_EST_START		0x7000012
> +#define CS40L50_MBOX_REDC_EST_DONE		0x7000022
> +#define CS40L50_MBOX_LE_EST_START		0x7000014
> +#define CS40L50_MBOX_LE_EST_DONE		0x7000024
> +#define CS40L50_MBOX_ACK			0xA000000
> +#define CS40L50_MBOX_PANIC			0xC000000
> +#define CS40L50_MBOX_WATERMARK			0xD000000
> +#define CS40L50_MBOX_ERR_EVENT_UNMAPPED		0xC0004B3
> +#define CS40L50_MBOX_ERR_EVENT_MODIFY		0xC0004B4
> +#define CS40L50_MBOX_ERR_NULLPTR		0xC0006A5
> +#define CS40L50_MBOX_ERR_BRAKING		0xC0006A8
> +#define CS40L50_MBOX_ERR_INVAL_SRC		0xC0006AC
> +#define CS40L50_MBOX_ERR_ENABLE_RANGE		0xC000836
> +#define CS40L50_MBOX_ERR_GPIO_UNMAPPED		0xC000837
> +#define CS40L50_MBOX_ERR_ISR_RANGE		0xC000838
> +#define CS40L50_MBOX_PERMANENT_SHORT		0xC000C1C
> +#define CS40L50_MBOX_RUNTIME_SHORT		0xC000C1D
> +
> +/* DSP */
> +#define CS40L50_DSP1_XMEM_PACKED_0		0x2000000
> +#define CS40L50_DSP1_XMEM_UNPACKED32_0		0x2400000
> +#define CS40L50_SYS_INFO_ID			0x25E0000
> +#define CS40L50_DSP1_XMEM_UNPACKED24_0		0x2800000
> +#define CS40L50_RAM_INIT			0x28021DC
> +#define CS40L50_POWER_ON_SEQ			0x2804320
> +#define CS40L50_OWT_BASE			0x2805C34
> +#define CS40L50_NUM_OF_WAVES			0x280CB4C
> +#define CS40L50_CORE_BASE			0x2B80000
> +#define CS40L50_CCM_CORE_CONTROL		0x2BC1000
> +#define CS40L50_DSP1_YMEM_PACKED_0		0x2C00000
> +#define CS40L50_DSP1_YMEM_UNPACKED32_0		0x3000000
> +#define CS40L50_DSP1_YMEM_UNPACKED24_0		0x3400000
> +#define CS40L50_DSP1_PMEM_0			0x3800000
> +#define CS40L50_DSP1_PMEM_5114			0x3804FE8
> +#define CS40L50_MEM_RDY_HW		0x2
> +#define CS40L50_RAM_INIT_FLAG		0x1
> +#define CS40L50_CLOCK_DISABLE		0x80
> +#define CS40L50_CLOCK_ENABLE		0x281
> +#define CS40L50_DSP_POLL_US		1000
> +#define CS40L50_DSP_TIMEOUT_COUNT	100
> +#define CS40L50_CP_READY_US		2200
> +#define CS40L50_PSEQ_SIZE		200
> +#define CS40L50_AUTOSUSPEND_MS		2000
> +
> +/* Firmware */
> +#define CS40L50_FW			"cs40l50.wmfw"
> +#define CS40L50_WT			"cs40l50.bin"
> +
> +/* Calibration */
> +#define CS40L50_REDC_ESTIMATION		0x2802F7C
> +#define CS40L50_F0_ESTIMATION		0x2802F84
> +#define CS40L50_F0_STORED		0x2805C00
> +#define CS40L50_REDC_STORED		0x2805C04
> +#define CS40L50_RE_EST_STATUS		0x3401B40
> +
> +/* Open wavetable */
> +#define CS40L50_OWT_SIZE		0x2805C38
> +#define CS40L50_OWT_NEXT		0x2805C3C
> +#define CS40L50_NUM_OF_OWT_WAVES	0x2805C40
> +
> +/* GPIO */
> +#define CS40L50_GPIO_BASE		0x2804140
> +
> +/* Device */
> +#define CS40L50_DEVID			0x0
> +#define CS40L50_REVID			0x4
> +#define CS40L50_DEVID_A		0x40A50
> +#define CS40L50_REVID_B0	0xB0
> +
> +enum cs40l50_irq_list {
> +	CS40L50_GLOBAL_ERROR_IRQ,
> +	CS40L50_UVLO_VDDBATT_IRQ,
> +	CS40L50_BST_ILIMIT_IRQ,
> +	CS40L50_BST_SHORT_IRQ,
> +	CS40L50_BST_UVP_IRQ,
> +	CS40L50_TEMP_ERR_IRQ,
> +	CS40L50_VIRT2_MBOX_IRQ,
> +	CS40L50_AMP_SHORT_IRQ,
> +};
> +
> +struct cs40l50_irq {
> +	const char *name;
> +	int irq;
> +	irqreturn_t (*handler)(int irq, void *data);
> +};
> +
> +struct cs40l50_private {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct cs_dsp dsp;
> +	struct mutex lock;
> +	struct gpio_desc *reset_gpio;
> +	struct regmap_irq_chip_data *irq_data;
> +	struct input_dev *input;
> +	const struct firmware *wmfw;
> +	struct cs_hap haptics;
> +	u32 devid;
> +	u32 revid;
> +	int irq;
> +};
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50);
> +int cs40l50_remove(struct cs40l50_private *cs40l50);
> +
> +extern const struct regmap_config cs40l50_regmap;
> +extern const struct dev_pm_ops cs40l50_pm_ops;
> +
> +#endif /* __CS40L50_H__ */
> -- 
> 2.25.1
> 

Kind regards,
Jeff LaBundy

Hi James,

On Wed, Oct 18, 2023 at 05:57:25PM +0000, James Ogletree wrote:
> From: James Ogletree <james.ogletree@cirrus.com>
> 
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
> 
> The input driver provides the interface for control of
> haptic effects through the device.
> 
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
>  MAINTAINERS                        |   1 +
>  drivers/input/misc/Kconfig         |  10 +
>  drivers/input/misc/Makefile        |   1 +
>  drivers/input/misc/cs40l50-vibra.c | 353 +++++++++++++++++++++++++++++
>  4 files changed, 365 insertions(+)
>  create mode 100644 drivers/input/misc/cs40l50-vibra.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 08e1e9695d49..24a00d8e5c1c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4971,6 +4971,7 @@ L:	patches@opensource.cirrus.com
>  S:	Supported
>  F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
>  F:	drivers/input/misc/cirrus*
> +F:	drivers/input/misc/cs40l*
>  F:	drivers/mfd/cs40l*
>  F:	include/linux/input/cirrus*
>  F:	include/linux/mfd/cs40l*
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 9f088900f863..938090648126 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -129,6 +129,16 @@ config INPUT_BMA150
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bma150.
>  
> +config INPUT_CS40L50_VIBRA
> +	tristate "CS40L50 Haptic Driver support"
> +	depends on MFD_CS40L50_CORE
> +	help
> +	  Say Y here to enable support for Cirrus Logic's CS40L50
> +	  haptic driver.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called cs40l50-vibra.
> +
>  config INPUT_E3X0_BUTTON
>  	tristate "NI Ettus Research USRP E3xx Button support."
>  	default n
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 6abefc41037b..6b653ed2124f 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_INPUT_CMA3000)		+= cma3000_d0x.o
>  obj-$(CONFIG_INPUT_CMA3000_I2C)		+= cma3000_d0x_i2c.o
>  obj-$(CONFIG_INPUT_COBALT_BTNS)		+= cobalt_btns.o
>  obj-$(CONFIG_INPUT_CPCAP_PWRBUTTON)	+= cpcap-pwrbutton.o
> +obj-$(CONFIG_INPUT_CS40L50_VIBRA)	+= cs40l50-vibra.o cirrus_haptics.o
>  obj-$(CONFIG_INPUT_DA7280_HAPTICS)	+= da7280.o
>  obj-$(CONFIG_INPUT_DA9052_ONKEY)	+= da9052_onkey.o
>  obj-$(CONFIG_INPUT_DA9055_ONKEY)	+= da9055_onkey.o
> diff --git a/drivers/input/misc/cs40l50-vibra.c b/drivers/input/misc/cs40l50-vibra.c
> new file mode 100644
> index 000000000000..3b3e4cb10de0
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50-vibra.c
> @@ -0,0 +1,353 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/firmware/cirrus/wmfw.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +static int cs40l50_add(struct input_dev *dev,
> +		       struct ff_effect *effect,
> +		       struct ff_effect *old)
> +{
> +	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +	u32 len = effect->u.periodic.custom_len;
> +
> +	if (effect->type != FF_PERIODIC || effect->u.periodic.waveform != FF_CUSTOM) {
> +		dev_err(cs40l50->dev, "Type (%#X) or waveform (%#X) unsupported\n",
> +			effect->type, effect->u.periodic.waveform);
> +		return -EINVAL;
> +	}
> +
> +	memcpy(&cs40l50->haptics.add_effect, effect, sizeof(struct ff_effect));
> +
> +	cs40l50->haptics.add_effect.u.periodic.custom_data = kcalloc(len,
> +								     sizeof(s16),
> +								     GFP_KERNEL);
> +	if (!cs40l50->haptics.add_effect.u.periodic.custom_data)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(cs40l50->haptics.add_effect.u.periodic.custom_data,
> +			   effect->u.periodic.custom_data, sizeof(s16) * len)) {
> +		cs40l50->haptics.add_error = -EFAULT;
> +		goto out_free;
> +	}
> +
> +	queue_work(cs40l50->haptics.vibe_wq, &cs40l50->haptics.add_work);
> +	flush_work(&cs40l50->haptics.add_work);
> +
> +out_free:
> +	kfree(cs40l50->haptics.add_effect.u.periodic.custom_data);
> +	cs40l50->haptics.add_effect.u.periodic.custom_data = NULL;
> +
> +	return cs40l50->haptics.add_error;
> +}
> +
> +static int cs40l50_playback(struct input_dev *dev, int effect_id, int val)
> +{
> +	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +
> +	if (val > 0) {
> +		cs40l50->haptics.start_effect = &dev->ff->effects[effect_id];
> +		queue_work(cs40l50->haptics.vibe_wq,
> +			   &cs40l50->haptics.vibe_start_work);
> +	} else {
> +		queue_work(cs40l50->haptics.vibe_wq,
> +			   &cs40l50->haptics.vibe_stop_work);
> +	}
> +
> +	return 0;
> +}
> +
> +static int cs40l50_erase(struct input_dev *dev, int effect_id)
> +{
> +	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +
> +	cs40l50->haptics.erase_effect = &dev->ff->effects[effect_id];
> +
> +	queue_work(cs40l50->haptics.vibe_wq, &cs40l50->haptics.erase_work);
> +	flush_work(&cs40l50->haptics.erase_work);
> +
> +	return cs40l50->haptics.erase_error;
> +}
> +
> +static const struct reg_sequence cs40l50_int_vamp_seq[] = {
> +	{ CS40L50_BST_LPMODE_SEL,	CS40L50_DCM_LOW_POWER },
> +	{ CS40L50_BLOCK_ENABLES2,	CS40L50_OVERTEMP_WARN },
> +};
> +
> +static const struct reg_sequence cs40l50_irq_mask_seq[] = {
> +	{ CS40L50_IRQ1_MASK_2,	CS40L50_IRQ_MASK_2_OVERRIDE },
> +	{ CS40L50_IRQ1_MASK_20,	CS40L50_IRQ_MASK_20_OVERRIDE },
> +};
> +
> +static int cs40l50_hw_init(struct cs40l50_private *cs40l50)
> +{
> +	int error;
> +
> +	error = regmap_multi_reg_write(cs40l50->regmap,
> +				       cs40l50_int_vamp_seq,
> +				       ARRAY_SIZE(cs40l50_int_vamp_seq));
> +	if (error)
> +		return error;
> +
> +	error = cs_hap_pseq_multi_write(&cs40l50->haptics,
> +					cs40l50_int_vamp_seq,
> +					ARRAY_SIZE(cs40l50_int_vamp_seq),
> +					false, PSEQ_OP_WRITE_FULL);
> +	if (error)
> +		return error;
> +
> +	error = regmap_multi_reg_write(cs40l50->regmap, cs40l50_irq_mask_seq,
> +				       ARRAY_SIZE(cs40l50_irq_mask_seq));
> +	if (error)
> +		return error;
> +
> +	return cs_hap_pseq_multi_write(&cs40l50->haptics, cs40l50_irq_mask_seq,
> +				       ARRAY_SIZE(cs40l50_irq_mask_seq), false,
> +				       PSEQ_OP_WRITE_FULL);
> +}
> +
> +static const struct cs_dsp_client_ops cs40l50_cs_dsp_client_ops;
> +
> +static const struct cs_dsp_region cs40l50_dsp_regions[] = {
> +	{
> +		.type = WMFW_HALO_PM_PACKED,
> +		.base = CS40L50_DSP1_PMEM_0
> +	},
> +	{
> +		.type = WMFW_HALO_XM_PACKED,
> +		.base = CS40L50_DSP1_XMEM_PACKED_0
> +	},
> +	{
> +		.type = WMFW_HALO_YM_PACKED,
> +		.base = CS40L50_DSP1_YMEM_PACKED_0
> +	},
> +	{
> +		.type = WMFW_ADSP2_XM,
> +		.base = CS40L50_DSP1_XMEM_UNPACKED24_0
> +	},
> +	{
> +		.type = WMFW_ADSP2_YM,
> +		.base = CS40L50_DSP1_YMEM_UNPACKED24_0
> +	},
> +};
> +
> +static int cs40l50_cs_dsp_init(struct cs40l50_private *cs40l50)
> +{
> +	cs40l50->dsp.num = 1;
> +	cs40l50->dsp.type = WMFW_HALO;
> +	cs40l50->dsp.dev = cs40l50->dev;
> +	cs40l50->dsp.regmap = cs40l50->regmap;
> +	cs40l50->dsp.base = CS40L50_CORE_BASE;
> +	cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
> +	cs40l50->dsp.mem = cs40l50_dsp_regions;
> +	cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
> +	cs40l50->dsp.no_core_startstop = true;
> +	cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
> +
> +	return cs_dsp_halo_init(&cs40l50->dsp);
> +}
> +
> +static struct cs_hap_bank cs40l50_banks[] = {
> +	{
> +		.bank =		WVFRM_BANK_RAM,
> +		.base_index =	CS40L50_RAM_BANK_INDEX_START,
> +		.max_index =	CS40L50_RAM_BANK_INDEX_START,
> +	},
> +	{
> +		.bank =		WVFRM_BANK_ROM,
> +		.base_index =	CS40L50_ROM_BANK_INDEX_START,
> +		.max_index =	CS40L50_ROM_BANK_INDEX_END,
> +	},
> +	{
> +		.bank =		WVFRM_BANK_OWT,
> +		.base_index =	CS40L50_RTH_INDEX_START,
> +		.max_index =	CS40L50_RTH_INDEX_END,
> +	},
> +};

These structs describe the DSP, and hence the silicon; they are not
specific to the input/FF device. Presumably the DSP could run algorithms
that support only the I2S streaming case as well (e.g. A2H); therefore,
these seem more appropriately placed in the MFD.

> +
> +static int cs40l50_cs_hap_init(struct cs40l50_private *cs40l50)
> +{
> +	cs40l50->haptics.regmap = cs40l50->regmap;
> +	cs40l50->haptics.dev = cs40l50->dev;
> +	cs40l50->haptics.banks = cs40l50_banks;
> +	cs40l50->haptics.dsp.gpio_base_reg = CS40L50_GPIO_BASE;
> +	cs40l50->haptics.dsp.owt_base_reg = CS40L50_OWT_BASE;
> +	cs40l50->haptics.dsp.owt_offset_reg = CS40L50_OWT_NEXT;
> +	cs40l50->haptics.dsp.owt_size_reg = CS40L50_OWT_SIZE;
> +	cs40l50->haptics.dsp.mailbox_reg = CS40L50_DSP_MBOX;
> +	cs40l50->haptics.dsp.pseq_reg = CS40L50_POWER_ON_SEQ;
> +	cs40l50->haptics.dsp.push_owt_cmd = CS40L50_OWT_PUSH;
> +	cs40l50->haptics.dsp.delete_owt_cmd = CS40L50_OWT_DELETE;
> +	cs40l50->haptics.dsp.stop_cmd = CS40L50_STOP_PLAYBACK;
> +	cs40l50->haptics.dsp.pseq_size = CS40L50_PSEQ_SIZE;
> +	cs40l50->haptics.runtime_pm = true;
> +
> +	return cs_hap_init(&cs40l50->haptics);
> +}
> +
> +static void cs40l50_upload_wt(const struct firmware *bin, void *context)
> +{
> +	struct cs40l50_private *cs40l50 = context;
> +	u32 nwaves;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	if (cs40l50->wmfw) {
> +		if (regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL,
> +				 CS40L50_CLOCK_DISABLE))
> +			goto err_mutex;
> +
> +		if (regmap_write(cs40l50->regmap, CS40L50_RAM_INIT,
> +				 CS40L50_RAM_INIT_FLAG))
> +			goto err_mutex;
> +
> +		if (regmap_write(cs40l50->regmap, CS40L50_PWRMGT_CTL,
> +				 CS40L50_MEM_RDY_HW))
> +			goto err_mutex;
> +	}
> +
> +	cs_dsp_power_up(&cs40l50->dsp, cs40l50->wmfw, "cs40l50.wmfw",
> +			bin, "cs40l50.bin", "cs40l50");
> +
> +	if (cs40l50->wmfw) {
> +		if (regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL,
> +				 CS40L50_CLOCK_ENABLE))
> +			goto err_mutex;
> +	}
> +
> +	if (regmap_read(cs40l50->regmap, CS40L50_NUM_OF_WAVES, &nwaves))
> +		goto err_mutex;
> +
> +	cs40l50->haptics.banks[WVFRM_BANK_RAM].max_index += (nwaves - 1);
> +
> +err_mutex:
> +	mutex_unlock(&cs40l50->lock);
> +	release_firmware(bin);
> +	release_firmware(cs40l50->wmfw);
> +}
> +
> +static void cs40l50_upload_patch(const struct firmware *wmfw, void *context)
> +{
> +	struct cs40l50_private *cs40l50 = context;
> +
> +	cs40l50->wmfw = wmfw;
> +
> +	if (request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT, CS40L50_WT,
> +				    cs40l50->dev, GFP_KERNEL,
> +				    cs40l50, cs40l50_upload_wt))
> +		release_firmware(cs40l50->wmfw);
> +}
> +
> +static int cs40l50_input_init(struct cs40l50_private *cs40l50)
> +{
> +	int error;
> +
> +	cs40l50->input = devm_input_allocate_device(cs40l50->dev);
> +	if (!cs40l50->input)
> +		return -ENOMEM;
> +
> +	cs40l50->input->id.product = cs40l50->devid & 0xFFFF;
> +	cs40l50->input->id.version = cs40l50->revid;
> +	cs40l50->input->name = "cs40l50_vibra";
> +
> +	input_set_drvdata(cs40l50->input, cs40l50);
> +	input_set_capability(cs40l50->input, EV_FF, FF_PERIODIC);
> +	input_set_capability(cs40l50->input, EV_FF, FF_CUSTOM);
> +
> +	error = input_ff_create(cs40l50->input, FF_MAX_EFFECTS);
> +	if (error)
> +		return error;
> +
> +	cs40l50->input->ff->upload = cs40l50_add;
> +	cs40l50->input->ff->playback = cs40l50_playback;
> +	cs40l50->input->ff->erase = cs40l50_erase;
> +
> +	INIT_LIST_HEAD(&cs40l50->haptics.effect_head);
> +
> +	error = input_register_device(cs40l50->input);
> +	if (error)
> +		goto err_free_dev;
> +
> +	return cs40l50_cs_hap_init(cs40l50);
> +
> +err_free_dev:
> +	input_free_device(cs40l50->input);
> +	return error;
> +}
> +static int cs40l50_vibra_probe(struct platform_device *pdev)
> +{
> +	struct cs40l50_private *cs40l50 = dev_get_drvdata(pdev->dev.parent);
> +	int error;
> +
> +	error = cs40l50_input_init(cs40l50);
> +	if (error)
> +		return error;
> +
> +	error = cs40l50_hw_init(cs40l50);
> +	if (error)
> +		goto err_input;
> +
> +	error = cs40l50_cs_dsp_init(cs40l50);
> +	if (error)
> +		goto err_input;
> +
> +	error = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
> +					CS40L50_FW, cs40l50->dev,
> +					GFP_KERNEL, cs40l50,
> +					cs40l50_upload_patch);
> +	if (error)
> +		goto err_dsp;
> +
> +	return 0;
> +
> +err_dsp:
> +	cs_dsp_remove(&cs40l50->dsp);
> +err_input:
> +	input_unregister_device(cs40l50->input);
> +	cs_hap_remove(&cs40l50->haptics);
> +
> +	return error;
> +}
> +
> +static int cs40l50_vibra_remove(struct platform_device *pdev)
> +{
> +	struct cs40l50_private *cs40l50 = dev_get_drvdata(pdev->dev.parent);
> +
> +	input_unregister_device(cs40l50->input);
> +	cs_hap_remove(&cs40l50->haptics);
> +
> +	if (cs40l50->dsp.booted)
> +		cs_dsp_power_down(&cs40l50->dsp);
> +	if (&cs40l50->dsp)
> +		cs_dsp_remove(&cs40l50->dsp);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id cs40l50_id_vibra[] = {
> +	{"cs40l50-vibra", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(platform, cs40l50_id_vibra);
> +
> +static struct platform_driver cs40l50_vibra_driver = {
> +	.probe		= cs40l50_vibra_probe,
> +	.remove		= cs40l50_vibra_remove,
> +	.id_table	= cs40l50_id_vibra,
> +	.driver		= {
> +		.name	= "cs40l50-vibra",
> +	},
> +};
> +module_platform_driver(cs40l50_vibra_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1
> 

Kind regards,
Jeff LaBundy

Hi James,

Just a few trailing comments on top of Lee's feedback.

On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
> From: James Ogletree <james.ogletree@cirrus.com>
> 
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
> 
> The MFD component registers and initializes the device.
> 
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
>  MAINTAINERS                 |   2 +
>  drivers/mfd/Kconfig         |  30 +++
>  drivers/mfd/Makefile        |   4 +
>  drivers/mfd/cs40l50-core.c  | 443 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/cs40l50-i2c.c   |  69 ++++++
>  drivers/mfd/cs40l50-spi.c   |  68 ++++++
>  include/linux/mfd/cs40l50.h | 198 ++++++++++++++++
>  7 files changed, 814 insertions(+)
>  create mode 100644 drivers/mfd/cs40l50-core.c
>  create mode 100644 drivers/mfd/cs40l50-i2c.c
>  create mode 100644 drivers/mfd/cs40l50-spi.c
>  create mode 100644 include/linux/mfd/cs40l50.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 57daf77bf550..08e1e9695d49 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4971,7 +4971,9 @@ L:	patches@opensource.cirrus.com
>  S:	Supported
>  F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
>  F:	drivers/input/misc/cirrus*
> +F:	drivers/mfd/cs40l*
>  F:	include/linux/input/cirrus*
> +F:	include/linux/mfd/cs40l*
>  
>  CIRRUS LOGIC DSP FIRMWARE DRIVER
>  M:	Simon Trimmer <simont@opensource.cirrus.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b93856de432..a133d04a7859 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2187,6 +2187,36 @@ config MCP_UCB1200_TS
>  
>  endmenu
>  
> +config MFD_CS40L50_CORE
> +	tristate
> +	select MFD_CORE
> +	select CS_DSP
> +	select REGMAP_IRQ
> +
> +config MFD_CS40L50_I2C
> +	tristate "Cirrus Logic CS40L50 (I2C)"
> +	select REGMAP_I2C
> +	select MFD_CS40L50_CORE
> +	depends on I2C
> +	help
> +	  Select this to support the Cirrus Logic CS40L50 Haptic
> +	  Driver over I2C.
> +
> +	  This driver can be built as a module. If built as a module it will be
> +	  called "cs40l50-i2c".
> +
> +config MFD_CS40L50_SPI
> +	tristate "Cirrus Logic CS40L50 (SPI)"
> +	select REGMAP_SPI
> +	select MFD_CS40L50_CORE
> +	depends on SPI
> +	help
> +	  Select this to support the Cirrus Logic CS40L50 Haptic
> +	  Driver over SPI.
> +
> +	  This driver can be built as a module. If built as a module it will be
> +	  called "cs40l50-spi".
> +
>  config MFD_VEXPRESS_SYSREG
>  	tristate "Versatile Express System Registers"
>  	depends on VEXPRESS_CONFIG && GPIOLIB
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..3b1a43b3acaf 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -95,6 +95,10 @@ obj-$(CONFIG_MFD_MADERA)	+= madera.o
>  obj-$(CONFIG_MFD_MADERA_I2C)	+= madera-i2c.o
>  obj-$(CONFIG_MFD_MADERA_SPI)	+= madera-spi.o
>  
> +obj-$(CONFIG_MFD_CS40L50_CORE)	+= cs40l50-core.o
> +obj-$(CONFIG_MFD_CS40L50_I2C)	+= cs40l50-i2c.o
> +obj-$(CONFIG_MFD_CS40L50_SPI)	+= cs40l50-spi.o
> +
>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> diff --git a/drivers/mfd/cs40l50-core.c b/drivers/mfd/cs40l50-core.c
> new file mode 100644
> index 000000000000..f1eadd80203a
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-core.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +static const struct mfd_cell cs40l50_devs[] = {
> +	{
> +		.name = "cs40l50-vibra",
> +	},
> +};

I also recommend including the codec driver, especially because it will
force you to think what core components belong in the MFD (e.g. PSEQ
housekeeping as per the other thread).

If L50 shares a die with an audio amp that has its own codec driver,
finding a way to make it work as the codec child of this MFD would be
a beautiful solution, since presumably that audio amp has to manage the
PSEQ as well?

I'm sure lining up the audio and haptic amps is a lot messier than it
sounds in real life; maybe something to think about for next gen though.
For now, even an extremely simple codec driver should suffice.

> +
> +const struct regmap_config cs40l50_regmap = {
> +	.reg_bits =		32,
> +	.reg_stride =		4,
> +	.val_bits =		32,
> +	.reg_format_endian =	REGMAP_ENDIAN_BIG,
> +	.val_format_endian =	REGMAP_ENDIAN_BIG,
> +};
> +EXPORT_SYMBOL_GPL(cs40l50_regmap);
> +
> +static struct regulator_bulk_data cs40l50_supplies[] = {
> +	{
> +		.supply = "vp",
> +	},
> +	{
> +		.supply = "vio",
> +	},
> +};
> +
> +static int cs40l50_handle_f0_est_done(struct cs40l50_private *cs40l50)
> +{
> +	u32 f_zero;
> +	int error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_F0_ESTIMATION, &f_zero);
> +	if (error)
> +		return error;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_F0_STORED, f_zero);
> +}
> +
> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> +{
> +	int error, fractional, integer, stored;
> +	u32 redc;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_RE_EST_STATUS, &redc);
> +	if (error)
> +		return error;
> +
> +	error = regmap_write(cs40l50->regmap, CS40L50_REDC_ESTIMATION, redc);
> +	if (error)
> +		return error;
> +
> +	/* Convert from Q8.15 to (Q7.17 * 29/240) */
> +	integer = ((redc >> 15) & 0xFF) << 17;
> +	fractional = (redc & 0x7FFF) * 4;
> +	stored = (integer | fractional) * 29 / 240;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_REDC_STORED, stored);
> +}
> +
> +static int cs40l50_error_release(struct cs40l50_private *cs40l50)
> +{
> +	int error;
> +
> +	error = regmap_write(cs40l50->regmap, CS40L50_ERR_RLS,
> +			     CS40L50_GLOBAL_ERR_RLS);
> +	if (error)
> +		return error;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_ERR_RLS, 0);
> +}
> +
> +static int cs40l50_mailbox_read_next(struct cs40l50_private *cs40l50, u32 *val)
> +{
> +	u32 rd_ptr, wt_ptr;
> +	int error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_WT, &wt_ptr);
> +	if (error)
> +		return error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, &rd_ptr);
> +	if (error)
> +		return error;
> +
> +	if (wt_ptr == rd_ptr) {
> +		*val = 0;
> +		return 0;
> +	}
> +
> +	error = regmap_read(cs40l50->regmap, rd_ptr, val);
> +	if (error)
> +		return error;
> +
> +	rd_ptr += sizeof(u32);
> +	if (rd_ptr > CS40L50_MBOX_QUEUE_END)
> +		rd_ptr = CS40L50_MBOX_QUEUE_BASE;
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, rd_ptr);
> +}
> +
> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +	int error = 0;
> +	u32 val;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
> +		switch (val) {
> +		case 0:
> +			mutex_unlock(&cs40l50->lock);

Interleaving the mutex through the switch statement like this is dangerous;
it kind of looks like a zipper. It makes the code difficult to follow and
can lead to bugs in case the switch statement grows over time.

In general, we want to lock as little code as possible; maybe the mutex
needs to move inside the helper functions called from here.

> +			dev_dbg(cs40l50->dev, "Reached end of queue\n");
> +			return IRQ_HANDLED;
> +		case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_TRIGGER_MBOX:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_MBOX\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_TRIGGER_I2S:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_I2S\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_COMPLETE_MBOX:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_MBOX\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_COMPLETE_GPIO:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_GPIO\n");
> +			break;
> +		case CS40L50_MBOX_HAPTIC_COMPLETE_I2S:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_I2S\n");
> +			break;
> +		case CS40L50_MBOX_F0_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_START\n");
> +			break;
> +		case CS40L50_MBOX_F0_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_DONE\n");
> +			error = cs40l50_handle_f0_est_done(cs40l50);
> +			if (error)
> +				goto out_mutex;
> +			break;
> +		case CS40L50_MBOX_REDC_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_START\n");
> +			break;
> +		case CS40L50_MBOX_REDC_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_DONE\n");
> +			error = cs40l50_handle_redc_est_done(cs40l50);
> +			if (error)
> +				goto out_mutex;
> +			break;
> +		case CS40L50_MBOX_LE_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_START\n");
> +			break;
> +		case CS40L50_MBOX_LE_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_DONE\n");
> +			break;
> +		case CS40L50_MBOX_AWAKE:
> +			dev_dbg(cs40l50->dev, "Mailbox: AWAKE\n");
> +			break;
> +		case CS40L50_MBOX_INIT:
> +			dev_dbg(cs40l50->dev, "Mailbox: INIT\n");
> +			break;
> +		case CS40L50_MBOX_ACK:
> +			dev_dbg(cs40l50->dev, "Mailbox: ACK\n");
> +			break;
> +		case CS40L50_MBOX_ERR_EVENT_UNMAPPED:
> +			dev_err(cs40l50->dev, "Unmapped event\n");
> +			break;
> +		case CS40L50_MBOX_ERR_EVENT_MODIFY:
> +			dev_err(cs40l50->dev, "Failed to modify event index\n");
> +			break;
> +		case CS40L50_MBOX_ERR_NULLPTR:
> +			dev_err(cs40l50->dev, "Null pointer\n");
> +			break;
> +		case CS40L50_MBOX_ERR_BRAKING:
> +			dev_err(cs40l50->dev, "Braking not in progress\n");
> +			break;
> +		case CS40L50_MBOX_ERR_INVAL_SRC:
> +			dev_err(cs40l50->dev, "Suspend/resume invalid source\n");
> +			break;
> +		case CS40L50_MBOX_ERR_ENABLE_RANGE:
> +			dev_err(cs40l50->dev, "GPIO enable out of range\n");
> +			break;
> +		case CS40L50_MBOX_ERR_GPIO_UNMAPPED:
> +			dev_err(cs40l50->dev, "GPIO not mapped\n");
> +			break;
> +		case CS40L50_MBOX_ERR_ISR_RANGE:
> +			dev_err(cs40l50->dev, "GPIO ISR out of range\n");
> +			break;
> +		case CS40L50_MBOX_PERMANENT_SHORT:
> +			dev_crit(cs40l50->dev, "Permanent short detected\n");
> +			break;
> +		case CS40L50_MBOX_RUNTIME_SHORT:
> +			dev_err(cs40l50->dev, "Runtime short detected\n");
> +			error = cs40l50_error_release(cs40l50);
> +			if (error)
> +				goto out_mutex;
> +			break;
> +		default:
> +			dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
> +			error = -EINVAL;
> +			goto out_mutex;
> +		}
> +	}

I don't think we need such a large chunk of code just to translate the mailbox
codes to human-readable strings; leave that to the datasheet. Just print the
numeric value.

> +
> +	error = -EIO;
> +
> +out_mutex:
> +	mutex_unlock(&cs40l50->lock);
> +
> +	return IRQ_RETVAL(!error);
> +}
> +
> +static irqreturn_t cs40l50_error(int irq, void *data);
> +
> +static const struct cs40l50_irq cs40l50_irqs[] = {
> +	CS40L50_IRQ(AMP_SHORT,		"Amp short",		error),
> +	CS40L50_IRQ(VIRT2_MBOX,		"Mailbox",		process_mbox),
> +	CS40L50_IRQ(TEMP_ERR,		"Overtemperature",	error),
> +	CS40L50_IRQ(BST_UVP,		"Boost undervoltage",	error),
> +	CS40L50_IRQ(BST_SHORT,		"Boost short",		error),
> +	CS40L50_IRQ(BST_ILIMIT,		"Boost current limit",	error),
> +	CS40L50_IRQ(UVLO_VDDBATT,	"Boost UVLO",		error),
> +	CS40L50_IRQ(GLOBAL_ERROR,	"Global",		error),
> +};
> +
> +static irqreturn_t cs40l50_error(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);
> +
> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static const struct regmap_irq cs40l50_reg_irqs[] = {
> +	CS40L50_REG_IRQ(IRQ1_INT_1,	AMP_SHORT),
> +	CS40L50_REG_IRQ(IRQ1_INT_2,	VIRT2_MBOX),
> +	CS40L50_REG_IRQ(IRQ1_INT_8,	TEMP_ERR),
> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_UVP),
> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_SHORT),
> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_ILIMIT),
> +	CS40L50_REG_IRQ(IRQ1_INT_10,	UVLO_VDDBATT),
> +	CS40L50_REG_IRQ(IRQ1_INT_18,	GLOBAL_ERROR),
> +};
> +
> +static struct regmap_irq_chip cs40l50_irq_chip = {
> +	.name =			"CS40L50 IRQ Controller",
> +
> +	.status_base =		CS40L50_IRQ1_INT_1,
> +	.mask_base =		CS40L50_IRQ1_MASK_1,
> +	.ack_base =		CS40L50_IRQ1_INT_1,
> +	.num_regs =		22,
> +
> +	.irqs =			cs40l50_reg_irqs,
> +	.num_irqs =		ARRAY_SIZE(cs40l50_reg_irqs),
> +
> +	.runtime_pm =		true,
> +};
> +
> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
> +{
> +	struct device *dev = cs40l50->dev;
> +	int error, i, irq;
> +
> +	error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
> +					 IRQF_ONESHOT | IRQF_SHARED, 0,
> +					 &cs40l50_irq_chip, &cs40l50->irq_data);
> +	if (error)
> +		return error;
> +
> +	for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
> +		irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
> +		if (irq < 0) {
> +			dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
> +			return irq;
> +		}
> +
> +		error = devm_request_threaded_irq(dev, irq, NULL,
> +						  cs40l50_irqs[i].handler,
> +						  IRQF_ONESHOT | IRQF_SHARED,
> +						  cs40l50_irqs[i].name, cs40l50);
> +		if (error) {
> +			dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
> +			return error;
> +		}
> +	}

This is kind of an uncommon design pattern; if anyone reads /proc/interrupts
on their system, it's going to be full of L50 interrupts. Normally we declare
a single IRQ, read the status register(s) from inside its handler and then
act accordingly.

What is the motivation for having one handler per interrupt status bit? If
multiple bits are set at once, does the register get read multiple times and
if so, does doing so clear any pending status? Or are the status registers
write-to-clear instead of read-to-clear?

> +
> +	return 0;
> +}
> +
> +static int cs40l50_part_num_resolve(struct cs40l50_private *cs40l50)
> +{
> +	struct device *dev = cs40l50->dev;
> +	int error;
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_DEVID, &cs40l50->devid);
> +	if (error)
> +		return error;
> +
> +	if (cs40l50->devid != CS40L50_DEVID_A) {
> +		dev_err(dev, "Invalid device ID: %#010X\n", cs40l50->devid);
> +		return -EINVAL;
> +	}
> +
> +	error = regmap_read(cs40l50->regmap, CS40L50_REVID, &cs40l50->revid);
> +	if (error)
> +		return error;
> +
> +	if (cs40l50->revid != CS40L50_REVID_B0) {
> +		dev_err(dev, "Invalid revision: %#04X\n", cs40l50->revid);
> +		return -EINVAL;
> +	}

I recommend this be '<' and not '!=' so that the driver isn't immediately
broken if a backwards-compatible metal fix hits the market (e.g. B1).

> +
> +	dev_info(dev, "Cirrus Logic CS40L50 revision %02X\n", cs40l50->revid);
> +
> +	return 0;
> +}
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50)
> +{
> +	struct device *dev = cs40l50->dev;
> +	int error;
> +
> +	mutex_init(&cs40l50->lock);
> +
> +	cs40l50->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(cs40l50->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(cs40l50->reset_gpio),
> +				     "Failed getting reset GPIO\n");
> +
> +	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(cs40l50_supplies),
> +					cs40l50_supplies);
> +	if (error)
> +		goto err_reset;

You shouldn't have to reset the device here; by initializing the GPIO as
GPIOD_OUT_HIGH, it is already asserted, which is required while the rails
are in an unknown state.

If GPIOD_OUT_HIGH is 1.8 V and not GND on your board, then the polarity
specified in your dts is backwards. gpiod is a logical API, not a physical
API. HIGH/1 = asserted (GND for active low when configured properly in
dts); LOW/0 = deasserted. Please check the flags in 'interrupts'.

> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies),
> +				      cs40l50_supplies);
> +	if (error)
> +		goto err_reset;

And here.

> +
> +	usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 100);
> +
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);

This confirms your dts is backwards. To drive an active-low reset high using
the gpiod API, you must write zero here. Fixing this will allow you to get
rid of the goto label.

> +
> +	usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 1000);
> +
> +	pm_runtime_set_autosuspend_delay(cs40l50->dev, CS40L50_AUTOSUSPEND_MS);
> +	pm_runtime_use_autosuspend(cs40l50->dev);
> +	pm_runtime_set_active(cs40l50->dev);
> +	pm_runtime_get_noresume(cs40l50->dev);
> +	devm_pm_runtime_enable(cs40l50->dev);
> +
> +	error = cs40l50_part_num_resolve(cs40l50);
> +	if (error)
> +		goto err_supplies;
> +
> +	error = cs40l50_irq_init(cs40l50);
> +	if (error)
> +		goto err_supplies;
> +
> +	error = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cs40l50_devs,
> +				     ARRAY_SIZE(cs40l50_devs), NULL, 0, NULL);
> +	if (error)
> +		goto err_supplies;
> +
> +	pm_runtime_mark_last_busy(cs40l50->dev);
> +	pm_runtime_put_autosuspend(cs40l50->dev);
> +
> +	return 0;
> +
> +err_supplies:
> +	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);

I recommend moving the disable call to a devm_add_action_or_reset callback;
this will save you the trouble of having to disable the regulators in the
error path or a remove callback, which can go away.

> +err_reset:
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);

This won't be necessary once you fix the polarity in your dts.

> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_probe);
> +
> +int cs40l50_remove(struct cs40l50_private *cs40l50)
> +{
> +	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_remove);
> +
> +static int cs40l50_runtime_suspend(struct device *dev)
> +{
> +	struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> +
> +	return regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX, CS40L50_ALLOW_HIBER);
> +}
> +
> +static int cs40l50_runtime_resume(struct device *dev)
> +{
> +	struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> +	int error, i;
> +	u32 val;
> +
> +	/* Device NAKs when exiting hibernation, so optionally retry here. */
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX,
> +				     CS40L50_PREVENT_HIBER);
> +		if (!error)
> +			break;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	for (; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = regmap_read(cs40l50->regmap, CS40L50_DSP_MBOX, &val);
> +		if (!error && val == 0)
> +			return 0;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	return error ? error : -ETIMEDOUT;

This is a style preference, but it's perfectly legal to write 'return error ? : ...

> +}
> +
> +EXPORT_GPL_DEV_PM_OPS(cs40l50_pm_ops) = {
> +	RUNTIME_PM_OPS(cs40l50_runtime_suspend, cs40l50_runtime_resume, NULL)
> +};
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-i2c.c b/drivers/mfd/cs40l50-i2c.c
> new file mode 100644
> index 000000000000..be1b130eb94b
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-i2c.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 I2C Driver
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/cs40l50.h>
> +
> +static int cs40l50_i2c_probe(struct i2c_client *client,
> +			     const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct cs40l50_private *cs40l50;
> +
> +	cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> +	if (!cs40l50)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, cs40l50);
> +
> +	cs40l50->regmap = devm_regmap_init_i2c(client, &cs40l50_regmap);
> +	if (IS_ERR(cs40l50->regmap))
> +		return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> +				     "Failed to initialize register map\n");
> +
> +	cs40l50->dev = dev;
> +	cs40l50->irq = client->irq;
> +
> +	return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_i2c_remove(struct i2c_client *client)
> +{
> +	struct cs40l50_private *cs40l50 = i2c_get_clientdata(client);
> +
> +	cs40l50_remove(cs40l50);
> +}
> +
> +static const struct i2c_device_id cs40l50_id_i2c[] = {
> +	{"cs40l50", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, cs40l50_id_i2c);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> +	{ .compatible = "cirrus,cs40l50" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct i2c_driver cs40l50_i2c_driver = {
> +	.driver = {
> +		.name = "cs40l50",
> +		.of_match_table = cs40l50_of_match,
> +		.pm = pm_ptr(&cs40l50_pm_ops),
> +	},
> +	.id_table = cs40l50_id_i2c,
> +	.probe = cs40l50_i2c_probe,
> +	.remove = cs40l50_i2c_remove,
> +};
> +
> +module_i2c_driver(cs40l50_i2c_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 I2C Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-spi.c b/drivers/mfd/cs40l50-spi.c
> new file mode 100644
> index 000000000000..8311d48efedf
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-spi.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 SPI Driver
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/input/cs40l50.h>
> +#include <linux/mfd/spi.h>
> +
> +static int cs40l50_spi_probe(struct spi_device *spi)
> +{
> +	struct cs40l50_private *cs40l50;
> +	struct device *dev = &spi->dev;
> +
> +	cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> +	if (!cs40l50)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, cs40l50);
> +
> +	cs40l50->regmap = devm_regmap_init_spi(spi, &cs40l50_regmap);
> +	if (IS_ERR(cs40l50->regmap))
> +		return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> +				     "Failed to initialize register map\n");
> +
> +	cs40l50->dev = dev;
> +	cs40l50->irq = spi->irq;
> +
> +	return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_spi_remove(struct spi_device *spi)
> +{
> +	struct cs40l50_private *cs40l50 = spi_get_drvdata(spi);
> +
> +	cs40l50_remove(cs40l50);
> +}
> +
> +static const struct spi_device_id cs40l50_id_spi[] = {
> +	{"cs40l50", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, cs40l50_id_spi);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> +	{ .compatible = "cirrus,cs40l50" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct spi_driver cs40l50_spi_driver = {
> +	.driver = {
> +		.name = "cs40l50",
> +		.of_match_table = cs40l50_of_match,
> +		.pm = pm_ptr(&cs40l50_pm_ops),
> +	},
> +	.id_table = cs40l50_id_spi,
> +	.probe = cs40l50_spi_probe,
> +	.remove = cs40l50_spi_remove,
> +};
> +
> +module_spi_driver(cs40l50_spi_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 SPI Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/cs40l50.h b/include/linux/mfd/cs40l50.h
> new file mode 100644
> index 000000000000..c625a999a5ae
> --- /dev/null
> +++ b/include/linux/mfd/cs40l50.h
> @@ -0,0 +1,198 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#ifndef __CS40L50_H__
> +#define __CS40L50_H__
> +
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/input.h>
> +#include <linux/input/cirrus_haptics.h>
> +#include <linux/interrupt.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Power Supply Configuration */
> +#define CS40L50_BLOCK_ENABLES2			0x201C
> +#define CS40L50_ERR_RLS				0x2034
> +#define CS40L50_PWRMGT_CTL			0x2900
> +#define CS40L50_BST_LPMODE_SEL			0x3810
> +#define CS40L50_DCM_LOW_POWER		0x1
> +#define CS40L50_OVERTEMP_WARN		0x4000010
> +
> +/* Interrupts */
> +#define CS40L50_IRQ1_INT_1			0xE010
> +#define CS40L50_IRQ1_INT_2			0xE014
> +#define CS40L50_IRQ1_INT_8			0xE02C
> +#define CS40L50_IRQ1_INT_9			0xE030
> +#define CS40L50_IRQ1_INT_10			0xE034
> +#define CS40L50_IRQ1_INT_18			0xE054
> +#define CS40L50_IRQ1_MASK_1			0xE090
> +#define CS40L50_IRQ1_MASK_2			0xE094
> +#define CS40L50_IRQ1_MASK_20			0xE0DC
> +#define CS40L50_IRQ_MASK_2_OVERRIDE	0xFFDF7FFF
> +#define CS40L50_IRQ_MASK_20_OVERRIDE	0x15C01000
> +#define CS40L50_AMP_SHORT_MASK		BIT(31)
> +#define CS40L50_VIRT2_MBOX_MASK		BIT(21)
> +#define CS40L50_TEMP_ERR_MASK		BIT(31)
> +#define CS40L50_BST_UVP_MASK		BIT(6)
> +#define CS40L50_BST_SHORT_MASK		BIT(7)
> +#define CS40L50_BST_ILIMIT_MASK		BIT(8)
> +#define CS40L50_UVLO_VDDBATT_MASK	BIT(16)
> +#define CS40L50_GLOBAL_ERROR_MASK	BIT(15)
> +#define CS40L50_GLOBAL_ERR_RLS		BIT(11)
> +#define CS40L50_IRQ(_irq, _name, _hand)		\
> +	{					\
> +		.irq = CS40L50_ ## _irq ## _IRQ,\
> +		.name = _name,			\
> +		.handler = cs40l50_ ## _hand,	\
> +	}
> +#define CS40L50_REG_IRQ(_reg, _irq)					\
> +	[CS40L50_ ## _irq ## _IRQ] = {					\
> +		.reg_offset = (CS40L50_ ## _reg) - CS40L50_IRQ1_INT_1,	\
> +		.mask = CS40L50_ ## _irq ## _MASK			\
> +	}
> +
> +/* Mailbox Inbound Commands */
> +#define CS40L50_RAM_BANK_INDEX_START	0x1000000
> +#define CS40L50_RTH_INDEX_START		0x1400000
> +#define CS40L50_RTH_INDEX_END		0x1400001
> +#define CS40L50_ROM_BANK_INDEX_START	0x1800000
> +#define CS40L50_ROM_BANK_INDEX_END	0x180001A
> +#define CS40L50_PREVENT_HIBER		0x2000003
> +#define CS40L50_ALLOW_HIBER		0x2000004
> +#define CS40L50_OWT_PUSH		0x3000008
> +#define CS40L50_STOP_PLAYBACK		0x5000000
> +#define CS40L50_OWT_DELETE		0xD000000
> +
> +/* Mailbox Outbound Commands */
> +#define CS40L50_MBOX_QUEUE_BASE				0x11004
> +#define CS40L50_MBOX_QUEUE_END				0x1101C
> +#define CS40L50_DSP_MBOX				0x11020
> +#define CS40L50_MBOX_QUEUE_WT				0x28042C8
> +#define CS40L50_MBOX_QUEUE_RD				0x28042CC
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_MBOX	0x1000000
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_GPIO	0x1000001
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_I2S	0x1000002
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_MBOX	0x1000010
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_GPIO	0x1000011
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_I2S		0x1000012
> +#define CS40L50_MBOX_INIT			0x2000000
> +#define CS40L50_MBOX_AWAKE			0x2000002
> +#define CS40L50_MBOX_F0_EST_START		0x7000011
> +#define CS40L50_MBOX_F0_EST_DONE		0x7000021
> +#define CS40L50_MBOX_REDC_EST_START		0x7000012
> +#define CS40L50_MBOX_REDC_EST_DONE		0x7000022
> +#define CS40L50_MBOX_LE_EST_START		0x7000014
> +#define CS40L50_MBOX_LE_EST_DONE		0x7000024
> +#define CS40L50_MBOX_ACK			0xA000000
> +#define CS40L50_MBOX_PANIC			0xC000000
> +#define CS40L50_MBOX_WATERMARK			0xD000000
> +#define CS40L50_MBOX_ERR_EVENT_UNMAPPED		0xC0004B3
> +#define CS40L50_MBOX_ERR_EVENT_MODIFY		0xC0004B4
> +#define CS40L50_MBOX_ERR_NULLPTR		0xC0006A5
> +#define CS40L50_MBOX_ERR_BRAKING		0xC0006A8
> +#define CS40L50_MBOX_ERR_INVAL_SRC		0xC0006AC
> +#define CS40L50_MBOX_ERR_ENABLE_RANGE		0xC000836
> +#define CS40L50_MBOX_ERR_GPIO_UNMAPPED		0xC000837
> +#define CS40L50_MBOX_ERR_ISR_RANGE		0xC000838
> +#define CS40L50_MBOX_PERMANENT_SHORT		0xC000C1C
> +#define CS40L50_MBOX_RUNTIME_SHORT		0xC000C1D
> +
> +/* DSP */
> +#define CS40L50_DSP1_XMEM_PACKED_0		0x2000000
> +#define CS40L50_DSP1_XMEM_UNPACKED32_0		0x2400000
> +#define CS40L50_SYS_INFO_ID			0x25E0000
> +#define CS40L50_DSP1_XMEM_UNPACKED24_0		0x2800000
> +#define CS40L50_RAM_INIT			0x28021DC
> +#define CS40L50_POWER_ON_SEQ			0x2804320
> +#define CS40L50_OWT_BASE			0x2805C34
> +#define CS40L50_NUM_OF_WAVES			0x280CB4C
> +#define CS40L50_CORE_BASE			0x2B80000
> +#define CS40L50_CCM_CORE_CONTROL		0x2BC1000
> +#define CS40L50_DSP1_YMEM_PACKED_0		0x2C00000
> +#define CS40L50_DSP1_YMEM_UNPACKED32_0		0x3000000
> +#define CS40L50_DSP1_YMEM_UNPACKED24_0		0x3400000
> +#define CS40L50_DSP1_PMEM_0			0x3800000
> +#define CS40L50_DSP1_PMEM_5114			0x3804FE8
> +#define CS40L50_MEM_RDY_HW		0x2
> +#define CS40L50_RAM_INIT_FLAG		0x1
> +#define CS40L50_CLOCK_DISABLE		0x80
> +#define CS40L50_CLOCK_ENABLE		0x281
> +#define CS40L50_DSP_POLL_US		1000
> +#define CS40L50_DSP_TIMEOUT_COUNT	100
> +#define CS40L50_CP_READY_US		2200
> +#define CS40L50_PSEQ_SIZE		200
> +#define CS40L50_AUTOSUSPEND_MS		2000
> +
> +/* Firmware */
> +#define CS40L50_FW			"cs40l50.wmfw"
> +#define CS40L50_WT			"cs40l50.bin"
> +
> +/* Calibration */
> +#define CS40L50_REDC_ESTIMATION		0x2802F7C
> +#define CS40L50_F0_ESTIMATION		0x2802F84
> +#define CS40L50_F0_STORED		0x2805C00
> +#define CS40L50_REDC_STORED		0x2805C04
> +#define CS40L50_RE_EST_STATUS		0x3401B40
> +
> +/* Open wavetable */
> +#define CS40L50_OWT_SIZE		0x2805C38
> +#define CS40L50_OWT_NEXT		0x2805C3C
> +#define CS40L50_NUM_OF_OWT_WAVES	0x2805C40
> +
> +/* GPIO */
> +#define CS40L50_GPIO_BASE		0x2804140
> +
> +/* Device */
> +#define CS40L50_DEVID			0x0
> +#define CS40L50_REVID			0x4
> +#define CS40L50_DEVID_A		0x40A50
> +#define CS40L50_REVID_B0	0xB0
> +
> +enum cs40l50_irq_list {
> +	CS40L50_GLOBAL_ERROR_IRQ,
> +	CS40L50_UVLO_VDDBATT_IRQ,
> +	CS40L50_BST_ILIMIT_IRQ,
> +	CS40L50_BST_SHORT_IRQ,
> +	CS40L50_BST_UVP_IRQ,
> +	CS40L50_TEMP_ERR_IRQ,
> +	CS40L50_VIRT2_MBOX_IRQ,
> +	CS40L50_AMP_SHORT_IRQ,
> +};
> +
> +struct cs40l50_irq {
> +	const char *name;
> +	int irq;
> +	irqreturn_t (*handler)(int irq, void *data);
> +};
> +
> +struct cs40l50_private {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct cs_dsp dsp;
> +	struct mutex lock;
> +	struct gpio_desc *reset_gpio;
> +	struct regmap_irq_chip_data *irq_data;
> +	struct input_dev *input;
> +	const struct firmware *wmfw;
> +	struct cs_hap haptics;
> +	u32 devid;
> +	u32 revid;
> +	int irq;
> +};
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50);
> +int cs40l50_remove(struct cs40l50_private *cs40l50);
> +
> +extern const struct regmap_config cs40l50_regmap;
> +extern const struct dev_pm_ops cs40l50_pm_ops;
> +
> +#endif /* __CS40L50_H__ */
> -- 
> 2.25.1
> 

Kind regards,
Jeff LaBundy

On Tue, 24 Oct 2023, Jeff LaBundy wrote:

> Hi James,
> 
> Just a few trailing comments on top of Lee's feedback.
> 
> On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
> > From: James Ogletree <james.ogletree@cirrus.com>
> > 
> > Introduce support for Cirrus Logic Device CS40L50: a
> > haptic driver with waveform memory, integrated DSP,
> > and closed-loop algorithms.
> > 
> > The MFD component registers and initializes the device.
> > 
> > Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> > ---
> >  MAINTAINERS                 |   2 +
> >  drivers/mfd/Kconfig         |  30 +++
> >  drivers/mfd/Makefile        |   4 +
> >  drivers/mfd/cs40l50-core.c  | 443 ++++++++++++++++++++++++++++++++++++
> >  drivers/mfd/cs40l50-i2c.c   |  69 ++++++
> >  drivers/mfd/cs40l50-spi.c   |  68 ++++++
> >  include/linux/mfd/cs40l50.h | 198 ++++++++++++++++
> >  7 files changed, 814 insertions(+)
> >  create mode 100644 drivers/mfd/cs40l50-core.c
> >  create mode 100644 drivers/mfd/cs40l50-i2c.c
> >  create mode 100644 drivers/mfd/cs40l50-spi.c
> >  create mode 100644 include/linux/mfd/cs40l50.h

Just popping along to say; these are excellent comments Jeff.

Thank you for your review.

Hi Jeff,

You’ve given such great feedback covering many aspects of the
driver, on this patch and the whole series, which has required very
careful consideration, so thank you for your patience.

> On Oct 24, 2023, at 9:04 PM, Jeff LaBundy <jeff@labundy.com> wrote:
> 
> On Wed, Oct 18, 2023 at 05:57:23PM +0000, James Ogletree wrote:
>> 
>> +static int cs_hap_pseq_init(struct cs_hap *haptics)
>> +{
>> + struct cs_hap_pseq_op *op;
>> + int error, i, num_words;
>> + u8 operation;
>> + u32 *words;
>> +
>> + if (!haptics->dsp.pseq_size || !haptics->dsp.pseq_reg)
>> + return 0;
>> +
>> + INIT_LIST_HEAD(&haptics->pseq_head);
> 
> Anything that allocates or initializes an element that is normally held
> in a driver's private data, like a list head or mutex, belongs in probe()
> in my opinion. It's less of an issue here, but for more complex cases
> where we may set something up in probe() and tear it down in remove(),
> the driver is easier to maintain if helper functions such as cs_hap_pseq_init()
> only manipulate or organize the data, rather than absorb additional work.

I agree with your reasoning, however, doesn’t this then turn on the question of
who the rightful owner of the write sequencer code is? If the pseq code
belongs in the MFD driver, it ought to be moved to the driver’s private data
structure there, however if it fits here, then not so. A counter example would
be cs_dsp.c, a library which contains within it some INIT_LIST_HEAD calls.
Perhaps the dust should settle on that dispute, and in regards to that, please
read my comments below.

>> +
>> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL);
>> + if (!words)
>> + return -ENOMEM;
>> +
>> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg,
>> +  words, haptics->dsp.pseq_size);
>> + if (error)
>> + goto err_free;
>> +
>> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) {
>> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]);
>> + switch (operation) {
>> + case PSEQ_OP_END:
>> + case PSEQ_OP_WRITE_UNLOCK:
>> + num_words = PSEQ_OP_END_WORDS;
>> + break;
>> + case PSEQ_OP_WRITE_ADDR8:
>> + case PSEQ_OP_WRITE_H16:
>> + case PSEQ_OP_WRITE_L16:
>> + num_words = PSEQ_OP_WRITE_X16_WORDS;
>> + break;
>> + case PSEQ_OP_WRITE_FULL:
>> + num_words = PSEQ_OP_WRITE_FULL_WORDS;
>> + break;
>> + default:
>> + error = -EINVAL;
>> + dev_err(haptics->dev, "Unsupported op: %u\n", operation);
>> + goto err_free;
>> + }
>> +
>> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL);
>> + if (!op) {
>> + error = -ENOMEM;
>> + goto err_free;
>> + }
>> +
>> + op->size = num_words * sizeof(u32);
>> + memcpy(op->words, &words[i], op->size);
>> + op->offset = i * sizeof(u32);
>> + op->operation = operation;
>> + list_add(&op->list, &haptics->pseq_head);
>> +
>> + if (operation == PSEQ_OP_END)
>> + break;
>> + }
>> +
>> + if (operation != PSEQ_OP_END)
>> + error = -ENOENT;
>> +
>> +err_free:
>> + kfree(words);
>> +
>> + return error;
>> +}
> 
> My first thought as I reviewed this patch was that this and the lot
> of pseq-related functions are not necessarily related to haptics, but
> rather CS40L50 register access and housekeeping in general.
> 
> I seem to recall on L25 and friends that the the power-on sequencer,
> i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory
> that can play back a series of address/data pairs when the device
> comes out of hibernation, and any registers written during runtime
> must also be mirrored to the PSEQ for "playback" later. Is that still
> the case here?
> 
> Assuming so, these functions seem like they belong in the MFD, not
> an input-specific library, because they will presumably be used by
> the codec driver as well, since that driver will write registers to
> set BCLK/LRCK ratio, etc.
> 
> Therefore, I think it makes more sense for these functions to move to
> the MFD, which can then export them for use by the input/FF and codec
> children.

I think the problem with moving the write sequencer code to the MFD
driver is that it would go unused in a codec-only environment. We only
need to write to the PSEQ when we want to maintain register settings
throughout hibernation cycles, and it isn’t possible to hibernate when
there is data streaming to the device. So the PSEQ will never be used
in the codec driver.

This leaves either the input driver or the library, and it makes more
sense to be in the library since it is shared code with L26. This was
my reasoning, let me know whether you think it is sound.

> This leaves cirrus_haptics.* with only a few functions related to
> starting and stopping work, which seem specific enough to just live
> in cs40l50-vibra.c. Presumably many of those could be re-used by
> the L30 down the road, but in that case I think we'd be looking to
> actually re-use the L50 driver and simply add a compatible string
> for L30.
> 
> I recall L30 has some overhead that L50 does not, which may make
> it hairy for cs40l50-vibra.c to support both. But in that case,
> you could always fork a cs40l30-vibra.c with its own compatible
> string, then have the L50 MFD selectively load the correct child
> based on device ID. To summarize, we should have:
> 
> * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery,
>  IRQ handling, exported PSEQ functions, etc.
> * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from
>  the MFD.
> * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop
>  work, also uses PSEQ library from the MFD.
> 
> And down the road, depending on complexity, maybe we also have:
> 
> * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that
>  includes other functionality that didn't really fit in cs40l50-vibra.c;
>  also uses PSEQ library from, and is loaded by, the original L50 MFD.
>  If this driver duplicates small bits of cs40l50-vibra.c, it's not the
>  end of the world.
> 
> All of these files would #include include/linux/mfd/cs40l50.h. And
> finally, cirrus_haptics.* simply go away. Same idea, just slightly
> more scalable, and closer to common design patterns.


I understand that it is a common design pattern to selectively load
devices from the MFD driver. If I could summarize my thoughts on why
that would not be fitting here, it’s that the L26 and L50 share a ton of
input FF related work, and not enough “MFD core” related work.
Between errata differences, power supply configurations, regmap
configurations, interrupt register differences, it would seem to make for
a very awkward, scrambled MFD driver. Moreover, I think I will be moving
firmware download to the MFD driver, and that alone constitutes a
significant incompatibility because firmware downloading is compulsory
on L26, not so on L50.

On the other hand, I want to emphasize the amount that L26 and
L50 share when it comes to the Input FF callbacks. The worker
functions in cirrus_haptics.c are bare-bones for this first
submission, but were designed to be totally generic and scalable to
the needs of L26 and all future Cirrus input drivers. While it might appear
too specific for L26, everything currently in cirrus_haptics is usable by
L26 as-is.

For the above reasons I favor the current approach.


>> +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr,
>> +       u32 data, bool update, u8 op_code)
>> +{
>> + struct cs_hap_pseq_op *op, *op_end, *op_new;
>> + struct cs_dsp_chunk ch;
>> + u32 pseq_bytes;
>> + int error;
>> +
>> + op_new = devm_kzalloc(haptics->dev, sizeof(*op_new), GFP_KERNEL);
>> + if (!op_new)
>> + return -ENOMEM;
>> +
>> + op_new->operation = op_code;
>> +
>> + ch = cs_dsp_chunk((void *) op_new->words,
>> +   PSEQ_OP_WRITE_FULL_WORDS * sizeof(u32));
>> + cs_dsp_chunk_write(&ch, 8, op_code);
>> + switch (op_code) {
>> + case PSEQ_OP_WRITE_FULL:
>> + cs_dsp_chunk_write(&ch, 32, addr);
>> + cs_dsp_chunk_write(&ch, 32, data);
>> + break;
>> + case PSEQ_OP_WRITE_L16:
>> + case PSEQ_OP_WRITE_H16:
>> + cs_dsp_chunk_write(&ch, 24, addr);
>> + cs_dsp_chunk_write(&ch, 16, data);
>> + break;
>> + default:
>> + error = -EINVAL;
>> + goto op_new_free;
>> + }
>> +
>> + op_new->size = cs_dsp_chunk_bytes(&ch);
>> +
>> + if (update) {
>> + op = cs_hap_pseq_find_op(op_new, &haptics->pseq_head);
>> + if (!op) {
>> + error = -EINVAL;
>> + goto op_new_free;
>> + }
>> + }
> 
> It seems we are relying on the developer to remember if he or she has
> already written 'addr' using a previous call to cs_hap_pseq_write(),
> then set the update flag accordingly; is that accurate?
> 
> If so, that is a high risk for bugs to be introduced as the driver is
> maintained. Can we not search for an existing address/data entry upon
> any call to cs_hap_pseq_write() using cs_hap_pseq_find_op(), and add
> or replace a new address/data entry automatically?

There are currently sequences on L26 where we want to write to the same
address twice. In such cases we wouldn't want the driver to automatically
look up and update the first entry during the second write call, hence the
need for the update flag. I think with this use case in mind, updating
the entries automatically wouldn't be feasible.

>> +static void cs_hap_vibe_stop_worker(struct work_struct *work)
>> +{
>> + struct cs_hap *haptics = container_of(work, struct cs_hap,
>> +       vibe_stop_work);
>> + int error;
>> +
>> + if (haptics->runtime_pm) {
>> + error = pm_runtime_resume_and_get(haptics->dev);
>> + if (error < 0) {
>> + haptics->stop_error = error;
>> + return;
>> + }
>> + }
>> +
>> + mutex_lock(&haptics->lock);
>> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
>> +      haptics->dsp.stop_cmd);
>> + mutex_unlock(&haptics->lock);
>> +
>> + if (haptics->runtime_pm) {
>> + pm_runtime_mark_last_busy(haptics->dev);
>> + pm_runtime_put_autosuspend(haptics->dev);
>> + }
>> +
>> + haptics->stop_error = error;
> 
> This seems like another argument for not separating the input/FF child
> from the meat of the driver; it just seems messy to pass around error
> codes within a driver's private data like this.

I removed the start_error and stop_error members. However I think the
erase_error and add_error need to stay. I think this is more of a symptom
of the Input FF layer requiring error reporting and having to use workqueues
for those Input FF callbacks, than it is to do with the separation of these
functions from the driver. Point being, even if these were moved, we would still
need these *_error members. Let me know if I understood you right here.

Best,
James

Hi Jeff,

> On Oct 24, 2023, at 10:03 PM, Jeff LaBundy <jeff@labundy.com> wrote:
>> 
>> +static const struct cs_dsp_client_ops cs40l50_cs_dsp_client_ops;
>> +
>> +static const struct cs_dsp_region cs40l50_dsp_regions[] = {
>> + {
>> + .type = WMFW_HALO_PM_PACKED,
>> + .base = CS40L50_DSP1_PMEM_0
>> + },
>> + {
>> + .type = WMFW_HALO_XM_PACKED,
>> + .base = CS40L50_DSP1_XMEM_PACKED_0
>> + },
>> + {
>> + .type = WMFW_HALO_YM_PACKED,
>> + .base = CS40L50_DSP1_YMEM_PACKED_0
>> + },
>> + {
>> + .type = WMFW_ADSP2_XM,
>> + .base = CS40L50_DSP1_XMEM_UNPACKED24_0
>> + },
>> + {
>> + .type = WMFW_ADSP2_YM,
>> + .base = CS40L50_DSP1_YMEM_UNPACKED24_0
>> + },
>> +};
>> +
>> +static int cs40l50_cs_dsp_init(struct cs40l50_private *cs40l50)
>> +{
>> + cs40l50->dsp.num = 1;
>> + cs40l50->dsp.type = WMFW_HALO;
>> + cs40l50->dsp.dev = cs40l50->dev;
>> + cs40l50->dsp.regmap = cs40l50->regmap;
>> + cs40l50->dsp.base = CS40L50_CORE_BASE;
>> + cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
>> + cs40l50->dsp.mem = cs40l50_dsp_regions;
>> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
>> + cs40l50->dsp.no_core_startstop = true;
>> + cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
>> +
>> + return cs_dsp_halo_init(&cs40l50->dsp);
>> +}
>> +
>> +static struct cs_hap_bank cs40l50_banks[] = {
>> + {
>> + .bank = WVFRM_BANK_RAM,
>> + .base_index = CS40L50_RAM_BANK_INDEX_START,
>> + .max_index = CS40L50_RAM_BANK_INDEX_START,
>> + },
>> + {
>> + .bank = WVFRM_BANK_ROM,
>> + .base_index = CS40L50_ROM_BANK_INDEX_START,
>> + .max_index = CS40L50_ROM_BANK_INDEX_END,
>> + },
>> + {
>> + .bank = WVFRM_BANK_OWT,
>> + .base_index = CS40L50_RTH_INDEX_START,
>> + .max_index = CS40L50_RTH_INDEX_END,
>> + },
>> +};
> 
> These structs describe the DSP, and hence the silicon; they are not
> specific to the input/FF device. Presumably the DSP could run algorithms
> that support only the I2S streaming case as well (e.g. A2H); therefore,
> these seem more appropriately placed in the MFD.

Acknowledged, but would you consider the last struct “cs40l50_banks” as
an exception? It would go unused in a codec-only setup.

Best,
James

Hi Jeff,

> On Oct 24, 2023, at 9:56 PM, Jeff LaBundy <jeff@labundy.com> wrote:
> 
> On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
>> 
>> +static irqreturn_t cs40l50_error(int irq, void *data);
>> +
>> +static const struct cs40l50_irq cs40l50_irqs[] = {
>> +	CS40L50_IRQ(AMP_SHORT,		"Amp short",		error),
>> +	CS40L50_IRQ(VIRT2_MBOX,		"Mailbox",		process_mbox),
>> +	CS40L50_IRQ(TEMP_ERR,		"Overtemperature",	error),
>> +	CS40L50_IRQ(BST_UVP,		"Boost undervoltage",	error),
>> +	CS40L50_IRQ(BST_SHORT,		"Boost short",		error),
>> +	CS40L50_IRQ(BST_ILIMIT,		"Boost current limit",	error),
>> +	CS40L50_IRQ(UVLO_VDDBATT,	"Boost UVLO",		error),
>> +	CS40L50_IRQ(GLOBAL_ERROR,	"Global",		error),
>> +};
>> +
>> +static irqreturn_t cs40l50_error(int irq, void *data)
>> +{
>> +	struct cs40l50_private *cs40l50 = data;
>> +
>> +	dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);
>> +
>> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
>> +}
>> +
>> +static const struct regmap_irq cs40l50_reg_irqs[] = {
>> +	CS40L50_REG_IRQ(IRQ1_INT_1,	AMP_SHORT),
>> +	CS40L50_REG_IRQ(IRQ1_INT_2,	VIRT2_MBOX),
>> +	CS40L50_REG_IRQ(IRQ1_INT_8,	TEMP_ERR),
>> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_UVP),
>> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_SHORT),
>> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_ILIMIT),
>> +	CS40L50_REG_IRQ(IRQ1_INT_10,	UVLO_VDDBATT),
>> +	CS40L50_REG_IRQ(IRQ1_INT_18,	GLOBAL_ERROR),
>> +};
>> +
>> +static struct regmap_irq_chip cs40l50_irq_chip = {
>> +	.name =			"CS40L50 IRQ Controller",
>> +
>> +	.status_base =		CS40L50_IRQ1_INT_1,
>> +	.mask_base =		CS40L50_IRQ1_MASK_1,
>> +	.ack_base =		CS40L50_IRQ1_INT_1,
>> +	.num_regs =		22,
>> +
>> +	.irqs =			cs40l50_reg_irqs,
>> +	.num_irqs =		ARRAY_SIZE(cs40l50_reg_irqs),
>> +
>> +	.runtime_pm =		true,
>> +};
>> +
>> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
>> +{
>> +	struct device *dev = cs40l50->dev;
>> +	int error, i, irq;
>> +
>> +	error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
>> +					 IRQF_ONESHOT | IRQF_SHARED, 0,
>> +					 &cs40l50_irq_chip, &cs40l50->irq_data);
>> +	if (error)
>> +		return error;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
>> +		irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
>> +		if (irq < 0) {
>> +			dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
>> +			return irq;
>> +		}
>> +
>> +		error = devm_request_threaded_irq(dev, irq, NULL,
>> +						  cs40l50_irqs[i].handler,
>> +						  IRQF_ONESHOT | IRQF_SHARED,
>> +						  cs40l50_irqs[i].name, cs40l50);
>> +		if (error) {
>> +			dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
>> +			return error;
>> +		}
>> +	}
> 
> This is kind of an uncommon design pattern; if anyone reads /proc/interrupts
> on their system, it's going to be full of L50 interrupts. Normally we declare
> a single IRQ, read the status register(s) from inside its handler and then
> act accordingly.
> 
> What is the motivation for having one handler per interrupt status bit? If
> multiple bits are set at once, does the register get read multiple times and
> if so, does doing so clear any pending status? Or are the status registers
> write-to-clear instead of read-to-clear?

The reason I used the regmap_irq framework is that it takes care of
the reading and clearing of the status register, and yes it handles the
situation of multiple bits getting set at once. I think I will merge the IRQ
handlers into one for the next version. The fact of /proc/interrupts filling
up with these interrupts is not great and was something I overlooked,
though I think I see instances of drivers with similar amount of interrupts
upstream.

Best,
James

Hi James,

On Wed, Nov 01, 2023 at 08:46:12PM +0000, James Ogletree wrote:
> Hi Jeff,
> 
> You’ve given such great feedback covering many aspects of the
> driver, on this patch and the whole series, which has required very
> careful consideration, so thank you for your patience.

Your high-quality submissions make the review process efficient and
productive :) Please excuse my delayed response.

> 
> > On Oct 24, 2023, at 9:04 PM, Jeff LaBundy <jeff@labundy.com> wrote:
> > 
> > On Wed, Oct 18, 2023 at 05:57:23PM +0000, James Ogletree wrote:
> >> 
> >> +static int cs_hap_pseq_init(struct cs_hap *haptics)
> >> +{
> >> + struct cs_hap_pseq_op *op;
> >> + int error, i, num_words;
> >> + u8 operation;
> >> + u32 *words;
> >> +
> >> + if (!haptics->dsp.pseq_size || !haptics->dsp.pseq_reg)
> >> + return 0;
> >> +
> >> + INIT_LIST_HEAD(&haptics->pseq_head);
> > 
> > Anything that allocates or initializes an element that is normally held
> > in a driver's private data, like a list head or mutex, belongs in probe()
> > in my opinion. It's less of an issue here, but for more complex cases
> > where we may set something up in probe() and tear it down in remove(),
> > the driver is easier to maintain if helper functions such as cs_hap_pseq_init()
> > only manipulate or organize the data, rather than absorb additional work.
> 
> I agree with your reasoning, however, doesn’t this then turn on the question of
> who the rightful owner of the write sequencer code is? If the pseq code
> belongs in the MFD driver, it ought to be moved to the driver’s private data
> structure there, however if it fits here, then not so. A counter example would
> be cs_dsp.c, a library which contains within it some INIT_LIST_HEAD calls.
> Perhaps the dust should settle on that dispute, and in regards to that, please
> read my comments below.

I think the list head belongs in the MFD's private data (cs40l50_private)
because the pseq code belongs in the MFD; more details as to why below.

> 
> >> +
> >> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL);
> >> + if (!words)
> >> + return -ENOMEM;
> >> +
> >> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg,
> >> +  words, haptics->dsp.pseq_size);
> >> + if (error)
> >> + goto err_free;
> >> +
> >> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) {
> >> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]);
> >> + switch (operation) {
> >> + case PSEQ_OP_END:
> >> + case PSEQ_OP_WRITE_UNLOCK:
> >> + num_words = PSEQ_OP_END_WORDS;
> >> + break;
> >> + case PSEQ_OP_WRITE_ADDR8:
> >> + case PSEQ_OP_WRITE_H16:
> >> + case PSEQ_OP_WRITE_L16:
> >> + num_words = PSEQ_OP_WRITE_X16_WORDS;
> >> + break;
> >> + case PSEQ_OP_WRITE_FULL:
> >> + num_words = PSEQ_OP_WRITE_FULL_WORDS;
> >> + break;
> >> + default:
> >> + error = -EINVAL;
> >> + dev_err(haptics->dev, "Unsupported op: %u\n", operation);
> >> + goto err_free;
> >> + }
> >> +
> >> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL);
> >> + if (!op) {
> >> + error = -ENOMEM;
> >> + goto err_free;
> >> + }
> >> +
> >> + op->size = num_words * sizeof(u32);
> >> + memcpy(op->words, &words[i], op->size);
> >> + op->offset = i * sizeof(u32);
> >> + op->operation = operation;
> >> + list_add(&op->list, &haptics->pseq_head);
> >> +
> >> + if (operation == PSEQ_OP_END)
> >> + break;
> >> + }
> >> +
> >> + if (operation != PSEQ_OP_END)
> >> + error = -ENOENT;
> >> +
> >> +err_free:
> >> + kfree(words);
> >> +
> >> + return error;
> >> +}
> > 
> > My first thought as I reviewed this patch was that this and the lot
> > of pseq-related functions are not necessarily related to haptics, but
> > rather CS40L50 register access and housekeeping in general.
> > 
> > I seem to recall on L25 and friends that the the power-on sequencer,
> > i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory
> > that can play back a series of address/data pairs when the device
> > comes out of hibernation, and any registers written during runtime
> > must also be mirrored to the PSEQ for "playback" later. Is that still
> > the case here?
> > 
> > Assuming so, these functions seem like they belong in the MFD, not
> > an input-specific library, because they will presumably be used by
> > the codec driver as well, since that driver will write registers to
> > set BCLK/LRCK ratio, etc.
> > 
> > Therefore, I think it makes more sense for these functions to move to
> > the MFD, which can then export them for use by the input/FF and codec
> > children.
> 
> I think the problem with moving the write sequencer code to the MFD
> driver is that it would go unused in a codec-only environment. We only
> need to write to the PSEQ when we want to maintain register settings
> throughout hibernation cycles, and it isn’t possible to hibernate when
> there is data streaming to the device. So the PSEQ will never be used
> in the codec driver.

I agree that in practice, the write sequencer would technically go unused
in a codec-only implementation. However, that is because the ASoC core
happens to write all pertinent registers ahead-of-time each time a stream
starts. That is a property of the ASoC core and not L50; my feeling is that
the driver should not be structured based on what one of the subsystems
associated with it happens to do, but rather the nature of the hardware.

Some specific reasons I think the MFD is a better home for the pseq code:

1. The write sequencer is a housekeeping function derived from the way
the hardware implements its power management; it doesn't have anything
to do with haptics. My opinion is that facilities supporting application-
agnostic functions belong in the MFD, for all children to access, even
if only one happens to do so today.

2. Over the course of the IC's life, you may be required to add errata
writes after the IC is taken out of reset; these presumably would need
to be "scheduled" in the write sequencer as well. These wouldn't make
logical sense to add in the input driver, and they would be missed in
the theoretical codec-only case.

3. This device has a programmable DSP; it wouldn't be unheard of for a
customer to ask for a new function down the road that begets a third
child device. Perhaps a customer asks for a special .wmfw file that
repurposes the SDOUT pin as a PWM output for an LED, and now you must
add a pwm child. That's a made-up example of course, but in general we
want to structure things in such a way that rip-up is minimal in case
requirements change in the future.

> This leaves either the input driver or the library, and it makes more
> sense to be in the library since it is shared code with L26. This was
> my reasoning, let me know whether you think it is sound.

I'm not sold on the input driver and the methods in what is now an FF
library living in separate files; more on that below.

> 
> > This leaves cirrus_haptics.* with only a few functions related to
> > starting and stopping work, which seem specific enough to just live
> > in cs40l50-vibra.c. Presumably many of those could be re-used by
> > the L30 down the road, but in that case I think we'd be looking to
> > actually re-use the L50 driver and simply add a compatible string
> > for L30.
> > 
> > I recall L30 has some overhead that L50 does not, which may make
> > it hairy for cs40l50-vibra.c to support both. But in that case,
> > you could always fork a cs40l30-vibra.c with its own compatible
> > string, then have the L50 MFD selectively load the correct child
> > based on device ID. To summarize, we should have:
> > 
> > * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery,
> >  IRQ handling, exported PSEQ functions, etc.
> > * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from
> >  the MFD.
> > * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop
> >  work, also uses PSEQ library from the MFD.
> > 
> > And down the road, depending on complexity, maybe we also have:
> > 
> > * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that
> >  includes other functionality that didn't really fit in cs40l50-vibra.c;
> >  also uses PSEQ library from, and is loaded by, the original L50 MFD.
> >  If this driver duplicates small bits of cs40l50-vibra.c, it's not the
> >  end of the world.
> > 
> > All of these files would #include include/linux/mfd/cs40l50.h. And
> > finally, cirrus_haptics.* simply go away. Same idea, just slightly
> > more scalable, and closer to common design patterns.
> 
> 
> I understand that it is a common design pattern to selectively load
> devices from the MFD driver. If I could summarize my thoughts on why
> that would not be fitting here, it’s that the L26 and L50 share a ton of
> input FF related work, and not enough “MFD core” related work.
> Between errata differences, power supply configurations, regmap
> configurations, interrupt register differences, it would seem to make for
> a very awkward, scrambled MFD driver. Moreover, I think I will be moving
> firmware download to the MFD driver, and that alone constitutes a
> significant incompatibility because firmware downloading is compulsory
> on L26, not so on L50.
> 
> On the other hand, I want to emphasize the amount that L26 and
> L50 share when it comes to the Input FF callbacks. The worker
> functions in cirrus_haptics.c are bare-bones for this first
> submission, but were designed to be totally generic and scalable to
> the needs of L26 and all future Cirrus input drivers. While it might appear
> too specific for L26, everything currently in cirrus_haptics is usable by
> L26 as-is.
> 
> For the above reasons I favor the current approach.
> 

If the core functions (e.g. firmware loading and interrupt handling) of L26
and L50 are drastically different, then it's perfectly acceptable to have
different MFD drivers for the two. There are plenty of examples of devices
throughout MFD who are close cousins to one another.

Likewise, if the input-related functions of L26 and L50 are nearly identical,
then it's also perfectly acceptable for both drivers/mfd/cs40l26.c and
drivers/mfd/cs40l50.c to load drivers/input/misc/cs40l50-vibra.c, which
supports both L26 and L50 haptics-related functions. You're already doing
something similar, but I disagree on the following:

1. Rather than have a library referenced by two drivers that support children
which are largely the same in a logcial sense, just have a single driver that
supports two children.

2. That common code encapsulates too many things (e.g. pseq facilities). A
library may make sense, but not restricted to input children.

For code that is common to either MFD (e.g. pseq), it is largely a judgement
call and maintainer preference as to where that code lives. One approach could
be to simply include all common code in drivers/mfd/cs40l50.c today. When
drivers/mfd/cs40l26.c lands, it can simply #include <linux/mfd/cs40l50.h>, and
its Kconfig can select MFD_CS40L50. Both modules would get built, but only one
would bind and probe.

Another approach could be to move the pseq code from the L50 MFD into a library
(e.g. drivers/mfs/cs40l50-common.c) selected by both L50 and L26 Kconfigs. That
is of course some rip-up, but at least the code is not jumping subsystems at this
point, which it otherwise might if you leave it in an input library for now.

That being said, you are the one who is responsible for supporting the driver in
the future, so the decision is ultimately yours. You have a much better sense of
the hardware variants and the relationships among them. It's a delicate balance
of preparing for what customers may request, and any decision will inevitably be
wrong in some way :) I think the important thing is to not restrict yourself too
much up front, and my humble opinion is that slightly redrawing the dotted lines
in your "block diagram" may ease some pain down the road.

> 
> >> +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr,
> >> +       u32 data, bool update, u8 op_code)
> >> +{
> >> + struct cs_hap_pseq_op *op, *op_end, *op_new;
> >> + struct cs_dsp_chunk ch;
> >> + u32 pseq_bytes;
> >> + int error;
> >> +
> >> + op_new = devm_kzalloc(haptics->dev, sizeof(*op_new), GFP_KERNEL);
> >> + if (!op_new)
> >> + return -ENOMEM;
> >> +
> >> + op_new->operation = op_code;
> >> +
> >> + ch = cs_dsp_chunk((void *) op_new->words,
> >> +   PSEQ_OP_WRITE_FULL_WORDS * sizeof(u32));
> >> + cs_dsp_chunk_write(&ch, 8, op_code);
> >> + switch (op_code) {
> >> + case PSEQ_OP_WRITE_FULL:
> >> + cs_dsp_chunk_write(&ch, 32, addr);
> >> + cs_dsp_chunk_write(&ch, 32, data);
> >> + break;
> >> + case PSEQ_OP_WRITE_L16:
> >> + case PSEQ_OP_WRITE_H16:
> >> + cs_dsp_chunk_write(&ch, 24, addr);
> >> + cs_dsp_chunk_write(&ch, 16, data);
> >> + break;
> >> + default:
> >> + error = -EINVAL;
> >> + goto op_new_free;
> >> + }
> >> +
> >> + op_new->size = cs_dsp_chunk_bytes(&ch);
> >> +
> >> + if (update) {
> >> + op = cs_hap_pseq_find_op(op_new, &haptics->pseq_head);
> >> + if (!op) {
> >> + error = -EINVAL;
> >> + goto op_new_free;
> >> + }
> >> + }
> > 
> > It seems we are relying on the developer to remember if he or she has
> > already written 'addr' using a previous call to cs_hap_pseq_write(),
> > then set the update flag accordingly; is that accurate?
> > 
> > If so, that is a high risk for bugs to be introduced as the driver is
> > maintained. Can we not search for an existing address/data entry upon
> > any call to cs_hap_pseq_write() using cs_hap_pseq_find_op(), and add
> > or replace a new address/data entry automatically?
> 
> There are currently sequences on L26 where we want to write to the same
> address twice. In such cases we wouldn't want the driver to automatically
> look up and update the first entry during the second write call, hence the
> need for the update flag. I think with this use case in mind, updating
> the entries automatically wouldn't be feasible.

ACK. Good call on returning -EINVAL if update is true yet the entry was
never added; that seems like a reasonable level of protection.

> 
> >> +static void cs_hap_vibe_stop_worker(struct work_struct *work)
> >> +{
> >> + struct cs_hap *haptics = container_of(work, struct cs_hap,
> >> +       vibe_stop_work);
> >> + int error;
> >> +
> >> + if (haptics->runtime_pm) {
> >> + error = pm_runtime_resume_and_get(haptics->dev);
> >> + if (error < 0) {
> >> + haptics->stop_error = error;
> >> + return;
> >> + }
> >> + }
> >> +
> >> + mutex_lock(&haptics->lock);
> >> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> >> +      haptics->dsp.stop_cmd);
> >> + mutex_unlock(&haptics->lock);
> >> +
> >> + if (haptics->runtime_pm) {
> >> + pm_runtime_mark_last_busy(haptics->dev);
> >> + pm_runtime_put_autosuspend(haptics->dev);
> >> + }
> >> +
> >> + haptics->stop_error = error;
> > 
> > This seems like another argument for not separating the input/FF child
> > from the meat of the driver; it just seems messy to pass around error
> > codes within a driver's private data like this.
> 
> I removed the start_error and stop_error members. However I think the
> erase_error and add_error need to stay. I think this is more of a symptom
> of the Input FF layer requiring error reporting and having to use workqueues
> for those Input FF callbacks, than it is to do with the separation of these
> functions from the driver. Point being, even if these were moved, we would still
> need these *_error members. Let me know if I understood you right here.

Sure, but why do adding and removing waveforms require workqueues? The DA7280
driver doesn't do this; what is different in this case? (That's a genuine
question, not an assertion that what you have is wrong, although it seems
unique based on my limited search).

> 
> Best,
> James
> 
> 

Kind regards,
Jeff LaBundy

Hi James,

On Wed, Nov 01, 2023 at 08:47:11PM +0000, James Ogletree wrote:
> Hi Jeff,
> 
> > On Oct 24, 2023, at 9:56 PM, Jeff LaBundy <jeff@labundy.com> wrote:
> > 
> > On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
> >> 
> >> +static irqreturn_t cs40l50_error(int irq, void *data);
> >> +
> >> +static const struct cs40l50_irq cs40l50_irqs[] = {
> >> +	CS40L50_IRQ(AMP_SHORT,		"Amp short",		error),
> >> +	CS40L50_IRQ(VIRT2_MBOX,		"Mailbox",		process_mbox),
> >> +	CS40L50_IRQ(TEMP_ERR,		"Overtemperature",	error),
> >> +	CS40L50_IRQ(BST_UVP,		"Boost undervoltage",	error),
> >> +	CS40L50_IRQ(BST_SHORT,		"Boost short",		error),
> >> +	CS40L50_IRQ(BST_ILIMIT,		"Boost current limit",	error),
> >> +	CS40L50_IRQ(UVLO_VDDBATT,	"Boost UVLO",		error),
> >> +	CS40L50_IRQ(GLOBAL_ERROR,	"Global",		error),
> >> +};
> >> +
> >> +static irqreturn_t cs40l50_error(int irq, void *data)
> >> +{
> >> +	struct cs40l50_private *cs40l50 = data;
> >> +
> >> +	dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);
> >> +
> >> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> >> +}
> >> +
> >> +static const struct regmap_irq cs40l50_reg_irqs[] = {
> >> +	CS40L50_REG_IRQ(IRQ1_INT_1,	AMP_SHORT),
> >> +	CS40L50_REG_IRQ(IRQ1_INT_2,	VIRT2_MBOX),
> >> +	CS40L50_REG_IRQ(IRQ1_INT_8,	TEMP_ERR),
> >> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_UVP),
> >> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_SHORT),
> >> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_ILIMIT),
> >> +	CS40L50_REG_IRQ(IRQ1_INT_10,	UVLO_VDDBATT),
> >> +	CS40L50_REG_IRQ(IRQ1_INT_18,	GLOBAL_ERROR),
> >> +};
> >> +
> >> +static struct regmap_irq_chip cs40l50_irq_chip = {
> >> +	.name =			"CS40L50 IRQ Controller",
> >> +
> >> +	.status_base =		CS40L50_IRQ1_INT_1,
> >> +	.mask_base =		CS40L50_IRQ1_MASK_1,
> >> +	.ack_base =		CS40L50_IRQ1_INT_1,
> >> +	.num_regs =		22,
> >> +
> >> +	.irqs =			cs40l50_reg_irqs,
> >> +	.num_irqs =		ARRAY_SIZE(cs40l50_reg_irqs),
> >> +
> >> +	.runtime_pm =		true,
> >> +};
> >> +
> >> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
> >> +{
> >> +	struct device *dev = cs40l50->dev;
> >> +	int error, i, irq;
> >> +
> >> +	error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
> >> +					 IRQF_ONESHOT | IRQF_SHARED, 0,
> >> +					 &cs40l50_irq_chip, &cs40l50->irq_data);
> >> +	if (error)
> >> +		return error;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
> >> +		irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
> >> +		if (irq < 0) {
> >> +			dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
> >> +			return irq;
> >> +		}
> >> +
> >> +		error = devm_request_threaded_irq(dev, irq, NULL,
> >> +						  cs40l50_irqs[i].handler,
> >> +						  IRQF_ONESHOT | IRQF_SHARED,
> >> +						  cs40l50_irqs[i].name, cs40l50);
> >> +		if (error) {
> >> +			dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
> >> +			return error;
> >> +		}
> >> +	}
> > 
> > This is kind of an uncommon design pattern; if anyone reads /proc/interrupts
> > on their system, it's going to be full of L50 interrupts. Normally we declare
> > a single IRQ, read the status register(s) from inside its handler and then
> > act accordingly.
> > 
> > What is the motivation for having one handler per interrupt status bit? If
> > multiple bits are set at once, does the register get read multiple times and
> > if so, does doing so clear any pending status? Or are the status registers
> > write-to-clear instead of read-to-clear?
> 
> The reason I used the regmap_irq framework is that it takes care of
> the reading and clearing of the status register, and yes it handles the
> situation of multiple bits getting set at once. I think I will merge the IRQ
> handlers into one for the next version. The fact of /proc/interrupts filling
> up with these interrupts is not great and was something I overlooked,
> though I think I see instances of drivers with similar amount of interrupts
> upstream.

I'm very much a proponent of using regmap_irq for a device whose register
map is organized in this way; my question was mostly why to map an entire
irq line to every possible interrupt source as opposed to reserving only
one line for L50 altogether.

I noted other such examples as well, and I think either method is functionally
equivalent. But considering many of these interrupts are related to events
that no customer would reasonably care about, and the ones that customers do
care about can easily be delineated by printing, a single handler is fine in
my opinion.

> 
> Best,
> James
> 

Kind regards,
Jeff LaBundy

Hi James,

On Wed, Nov 01, 2023 at 08:47:07PM +0000, James Ogletree wrote:
> Hi Jeff,
> 
> > On Oct 24, 2023, at 10:03 PM, Jeff LaBundy <jeff@labundy.com> wrote:
> >> 
> >> +static const struct cs_dsp_client_ops cs40l50_cs_dsp_client_ops;
> >> +
> >> +static const struct cs_dsp_region cs40l50_dsp_regions[] = {
> >> + {
> >> + .type = WMFW_HALO_PM_PACKED,
> >> + .base = CS40L50_DSP1_PMEM_0
> >> + },
> >> + {
> >> + .type = WMFW_HALO_XM_PACKED,
> >> + .base = CS40L50_DSP1_XMEM_PACKED_0
> >> + },
> >> + {
> >> + .type = WMFW_HALO_YM_PACKED,
> >> + .base = CS40L50_DSP1_YMEM_PACKED_0
> >> + },
> >> + {
> >> + .type = WMFW_ADSP2_XM,
> >> + .base = CS40L50_DSP1_XMEM_UNPACKED24_0
> >> + },
> >> + {
> >> + .type = WMFW_ADSP2_YM,
> >> + .base = CS40L50_DSP1_YMEM_UNPACKED24_0
> >> + },
> >> +};
> >> +
> >> +static int cs40l50_cs_dsp_init(struct cs40l50_private *cs40l50)
> >> +{
> >> + cs40l50->dsp.num = 1;
> >> + cs40l50->dsp.type = WMFW_HALO;
> >> + cs40l50->dsp.dev = cs40l50->dev;
> >> + cs40l50->dsp.regmap = cs40l50->regmap;
> >> + cs40l50->dsp.base = CS40L50_CORE_BASE;
> >> + cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
> >> + cs40l50->dsp.mem = cs40l50_dsp_regions;
> >> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
> >> + cs40l50->dsp.no_core_startstop = true;
> >> + cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
> >> +
> >> + return cs_dsp_halo_init(&cs40l50->dsp);
> >> +}
> >> +
> >> +static struct cs_hap_bank cs40l50_banks[] = {
> >> + {
> >> + .bank = WVFRM_BANK_RAM,
> >> + .base_index = CS40L50_RAM_BANK_INDEX_START,
> >> + .max_index = CS40L50_RAM_BANK_INDEX_START,
> >> + },
> >> + {
> >> + .bank = WVFRM_BANK_ROM,
> >> + .base_index = CS40L50_ROM_BANK_INDEX_START,
> >> + .max_index = CS40L50_ROM_BANK_INDEX_END,
> >> + },
> >> + {
> >> + .bank = WVFRM_BANK_OWT,
> >> + .base_index = CS40L50_RTH_INDEX_START,
> >> + .max_index = CS40L50_RTH_INDEX_END,
> >> + },
> >> +};
> > 
> > These structs describe the DSP, and hence the silicon; they are not
> > specific to the input/FF device. Presumably the DSP could run algorithms
> > that support only the I2S streaming case as well (e.g. A2H); therefore,
> > these seem more appropriately placed in the MFD.
> 
> Acknowledged, but would you consider the last struct “cs40l50_banks” as
> an exception? It would go unused in a codec-only setup.

I agree with you; I should have inserted my comment after cs40l50_cs_dsp_init()
and not cs40l50_banks[].

> 
> Best,
> James
> 
> 

Kind regards,
Jeff LaBundy

>>>> +
>>>> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL);
>>>> + if (!words)
>>>> + return -ENOMEM;
>>>> +
>>>> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg,
>>>> +  words, haptics->dsp.pseq_size);
>>>> + if (error)
>>>> + goto err_free;
>>>> +
>>>> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) {
>>>> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]);
>>>> + switch (operation) {
>>>> + case PSEQ_OP_END:
>>>> + case PSEQ_OP_WRITE_UNLOCK:
>>>> + num_words = PSEQ_OP_END_WORDS;
>>>> + break;
>>>> + case PSEQ_OP_WRITE_ADDR8:
>>>> + case PSEQ_OP_WRITE_H16:
>>>> + case PSEQ_OP_WRITE_L16:
>>>> + num_words = PSEQ_OP_WRITE_X16_WORDS;
>>>> + break;
>>>> + case PSEQ_OP_WRITE_FULL:
>>>> + num_words = PSEQ_OP_WRITE_FULL_WORDS;
>>>> + break;
>>>> + default:
>>>> + error = -EINVAL;
>>>> + dev_err(haptics->dev, "Unsupported op: %u\n", operation);
>>>> + goto err_free;
>>>> + }
>>>> +
>>>> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL);
>>>> + if (!op) {
>>>> + error = -ENOMEM;
>>>> + goto err_free;
>>>> + }
>>>> +
>>>> + op->size = num_words * sizeof(u32);
>>>> + memcpy(op->words, &words[i], op->size);
>>>> + op->offset = i * sizeof(u32);
>>>> + op->operation = operation;
>>>> + list_add(&op->list, &haptics->pseq_head);
>>>> +
>>>> + if (operation == PSEQ_OP_END)
>>>> + break;
>>>> + }
>>>> +
>>>> + if (operation != PSEQ_OP_END)
>>>> + error = -ENOENT;
>>>> +
>>>> +err_free:
>>>> + kfree(words);
>>>> +
>>>> + return error;
>>>> +}
>>> 
>>> My first thought as I reviewed this patch was that this and the lot
>>> of pseq-related functions are not necessarily related to haptics, but
>>> rather CS40L50 register access and housekeeping in general.
>>> 
>>> I seem to recall on L25 and friends that the the power-on sequencer,
>>> i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory
>>> that can play back a series of address/data pairs when the device
>>> comes out of hibernation, and any registers written during runtime
>>> must also be mirrored to the PSEQ for "playback" later. Is that still
>>> the case here?
>>> 
>>> Assuming so, these functions seem like they belong in the MFD, not
>>> an input-specific library, because they will presumably be used by
>>> the codec driver as well, since that driver will write registers to
>>> set BCLK/LRCK ratio, etc.
>>> 
>>> Therefore, I think it makes more sense for these functions to move to
>>> the MFD, which can then export them for use by the input/FF and codec
>>> children.
>> 
>> I think the problem with moving the write sequencer code to the MFD
>> driver is that it would go unused in a codec-only environment. We only
>> need to write to the PSEQ when we want to maintain register settings
>> throughout hibernation cycles, and it isn’t possible to hibernate when
>> there is data streaming to the device. So the PSEQ will never be used
>> in the codec driver.
> 
> I agree that in practice, the write sequencer would technically go unused
> in a codec-only implementation. However, that is because the ASoC core
> happens to write all pertinent registers ahead-of-time each time a stream
> starts. That is a property of the ASoC core and not L50; my feeling is that
> the driver should not be structured based on what one of the subsystems
> associated with it happens to do, but rather the nature of the hardware.
> 
> Some specific reasons I think the MFD is a better home for the pseq code:
> 
> 1. The write sequencer is a housekeeping function derived from the way
> the hardware implements its power management; it doesn't have anything
> to do with haptics. My opinion is that facilities supporting application-
> agnostic functions belong in the MFD, for all children to access, even
> if only one happens to do so today.
> 
> 2. Over the course of the IC's life, you may be required to add errata
> writes after the IC is taken out of reset; these presumably would need
> to be "scheduled" in the write sequencer as well. These wouldn't make
> logical sense to add in the input driver, and they would be missed in
> the theoretical codec-only case.
> 
> 3. This device has a programmable DSP; it wouldn't be unheard of for a
> customer to ask for a new function down the road that begets a third
> child device. Perhaps a customer asks for a special .wmfw file that
> repurposes the SDOUT pin as a PWM output for an LED, and now you must
> add a pwm child. That's a made-up example of course, but in general we
> want to structure things in such a way that rip-up is minimal in case
> requirements change in the future.

Great points. I agree now that the write sequencer code ought not to go in
cirrus_haptics.c. After talking it over with the internal team, I am considering
moving the write sequencer interface to cs_dsp.c. It’s an already existing
library with both Cirrus haptics and audio users. It seems to dodge your
concerns above and avoids a new common library as you suggested
below. Do you have any concerns on this approach over putting it in MFD?


>>> This leaves cirrus_haptics.* with only a few functions related to
>>> starting and stopping work, which seem specific enough to just live
>>> in cs40l50-vibra.c. Presumably many of those could be re-used by
>>> the L30 down the road, but in that case I think we'd be looking to
>>> actually re-use the L50 driver and simply add a compatible string
>>> for L30.
>>> 
>>> I recall L30 has some overhead that L50 does not, which may make
>>> it hairy for cs40l50-vibra.c to support both. But in that case,
>>> you could always fork a cs40l30-vibra.c with its own compatible
>>> string, then have the L50 MFD selectively load the correct child
>>> based on device ID. To summarize, we should have:
>>> 
>>> * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery,
>>> IRQ handling, exported PSEQ functions, etc.
>>> * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from
>>> the MFD.
>>> * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop
>>> work, also uses PSEQ library from the MFD.
>>> 
>>> And down the road, depending on complexity, maybe we also have:
>>> 
>>> * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that
>>> includes other functionality that didn't really fit in cs40l50-vibra.c;
>>> also uses PSEQ library from, and is loaded by, the original L50 MFD.
>>> If this driver duplicates small bits of cs40l50-vibra.c, it's not the
>>> end of the world.
>>> 
>>> All of these files would #include include/linux/mfd/cs40l50.h. And
>>> finally, cirrus_haptics.* simply go away. Same idea, just slightly
>>> more scalable, and closer to common design patterns.
>> 
>> 
>> I understand that it is a common design pattern to selectively load
>> devices from the MFD driver. If I could summarize my thoughts on why
>> that would not be fitting here, it’s that the L26 and L50 share a ton of
>> input FF related work, and not enough “MFD core” related work.
>> Between errata differences, power supply configurations, regmap
>> configurations, interrupt register differences, it would seem to make for
>> a very awkward, scrambled MFD driver. Moreover, I think I will be moving
>> firmware download to the MFD driver, and that alone constitutes a
>> significant incompatibility because firmware downloading is compulsory
>> on L26, not so on L50.
>> 
>> On the other hand, I want to emphasize the amount that L26 and
>> L50 share when it comes to the Input FF callbacks. The worker
>> functions in cirrus_haptics.c are bare-bones for this first
>> submission, but were designed to be totally generic and scalable to
>> the needs of L26 and all future Cirrus input drivers. While it might appear
>> too specific for L26, everything currently in cirrus_haptics is usable by
>> L26 as-is.
>> 
>> For the above reasons I favor the current approach.
>> 
> 
> Likewise, if the input-related functions of L26 and L50 are nearly identical,
> then it's also perfectly acceptable for both drivers/mfd/cs40l26.c and
> drivers/mfd/cs40l50.c to load drivers/input/misc/cs40l50-vibra.c, which
> supports both L26 and L50 haptics-related functions. You're already doing
> something similar, but I disagree on the following:
> 
> 1. Rather than have a library referenced by two drivers that support children
> which are largely the same in a logcial sense, just have a single driver that
> supports two children.

Your point here is clear and makes sense to me, especially now with the write
sequencer interface moving out. After considering the similarities and
differences closer, I am still a little wary. Maybe you can help me with these
concerns:

1. In the current implementation, drivers would be able to configure their own
input FF capabilities, and selectively register to input FF callbacks. L50 does
not register to the set_gain callback, whereas L26 does. I anticipate future
divergences, such as one driver supporting different effect types (see
the L50-specific error checking in cs40l50_add()). This would be exacerbated
by any future additional children.

2. This may be my lack of imagination, but in the current implementation it
seems much easier to develop new haptic features that don’t apply to all the
users of the library. One would simply wrap the feature in a boolean in
cirrus_haptics, which clients can take or leave. In the one driver
implementation, it seems you would have to find some clever, generalized
way of determining whether or not a feature can be used. This would also
seem to be exacerbated by any future additional children.

3. The current implementation provides for the individual drivers to setup
the haptics interface in whatever way peculiar to that device, whether that
interface be static (L50) or dependent on the loaded firmware (L26).

Since I am moving around a lot of code in and out of both -vibra.c and the
library for the next version, I think it would be helpful for me to wait until the
next version is submitted to decide on this. Would that be acceptable?

> 
> 
>> 
>>>> +static void cs_hap_vibe_stop_worker(struct work_struct *work)
>>>> +{
>>>> + struct cs_hap *haptics = container_of(work, struct cs_hap,
>>>> +       vibe_stop_work);
>>>> + int error;
>>>> +
>>>> + if (haptics->runtime_pm) {
>>>> + error = pm_runtime_resume_and_get(haptics->dev);
>>>> + if (error < 0) {
>>>> + haptics->stop_error = error;
>>>> + return;
>>>> + }
>>>> + }
>>>> +
>>>> + mutex_lock(&haptics->lock);
>>>> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
>>>> +      haptics->dsp.stop_cmd);
>>>> + mutex_unlock(&haptics->lock);
>>>> +
>>>> + if (haptics->runtime_pm) {
>>>> + pm_runtime_mark_last_busy(haptics->dev);
>>>> + pm_runtime_put_autosuspend(haptics->dev);
>>>> + }
>>>> +
>>>> + haptics->stop_error = error;
>>> 
>>> This seems like another argument for not separating the input/FF child
>>> from the meat of the driver; it just seems messy to pass around error
>>> codes within a driver's private data like this.
>> 
>> I removed the start_error and stop_error members. However I think the
>> erase_error and add_error need to stay. I think this is more of a symptom
>> of the Input FF layer requiring error reporting and having to use workqueues
>> for those Input FF callbacks, than it is to do with the separation of these
>> functions from the driver. Point being, even if these were moved, we would still
>> need these *_error members. Let me know if I understood you right here.
> 
> Sure, but why do adding and removing waveforms require workqueues? The DA7280
> driver doesn't do this; what is different in this case? (That's a genuine
> question, not an assertion that what you have is wrong, although it seems
> unique based on my limited search).

The reason why we have worker items for upload and erase input FF callbacks is
because we need to ensure their ordering with playback worker items, and we need
those worker items because the Input FF layer calls playbacks in atomic context.

Best,
James

Hi James,

Apologies for the delayed response.

On Wed, Nov 29, 2023 at 10:22:16PM +0000, James Ogletree wrote:
> 
> >>>> +
> >>>> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL);
> >>>> + if (!words)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg,
> >>>> +  words, haptics->dsp.pseq_size);
> >>>> + if (error)
> >>>> + goto err_free;
> >>>> +
> >>>> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) {
> >>>> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]);
> >>>> + switch (operation) {
> >>>> + case PSEQ_OP_END:
> >>>> + case PSEQ_OP_WRITE_UNLOCK:
> >>>> + num_words = PSEQ_OP_END_WORDS;
> >>>> + break;
> >>>> + case PSEQ_OP_WRITE_ADDR8:
> >>>> + case PSEQ_OP_WRITE_H16:
> >>>> + case PSEQ_OP_WRITE_L16:
> >>>> + num_words = PSEQ_OP_WRITE_X16_WORDS;
> >>>> + break;
> >>>> + case PSEQ_OP_WRITE_FULL:
> >>>> + num_words = PSEQ_OP_WRITE_FULL_WORDS;
> >>>> + break;
> >>>> + default:
> >>>> + error = -EINVAL;
> >>>> + dev_err(haptics->dev, "Unsupported op: %u\n", operation);
> >>>> + goto err_free;
> >>>> + }
> >>>> +
> >>>> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL);
> >>>> + if (!op) {
> >>>> + error = -ENOMEM;
> >>>> + goto err_free;
> >>>> + }
> >>>> +
> >>>> + op->size = num_words * sizeof(u32);
> >>>> + memcpy(op->words, &words[i], op->size);
> >>>> + op->offset = i * sizeof(u32);
> >>>> + op->operation = operation;
> >>>> + list_add(&op->list, &haptics->pseq_head);
> >>>> +
> >>>> + if (operation == PSEQ_OP_END)
> >>>> + break;
> >>>> + }
> >>>> +
> >>>> + if (operation != PSEQ_OP_END)
> >>>> + error = -ENOENT;
> >>>> +
> >>>> +err_free:
> >>>> + kfree(words);
> >>>> +
> >>>> + return error;
> >>>> +}
> >>> 
> >>> My first thought as I reviewed this patch was that this and the lot
> >>> of pseq-related functions are not necessarily related to haptics, but
> >>> rather CS40L50 register access and housekeeping in general.
> >>> 
> >>> I seem to recall on L25 and friends that the the power-on sequencer,
> >>> i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory
> >>> that can play back a series of address/data pairs when the device
> >>> comes out of hibernation, and any registers written during runtime
> >>> must also be mirrored to the PSEQ for "playback" later. Is that still
> >>> the case here?
> >>> 
> >>> Assuming so, these functions seem like they belong in the MFD, not
> >>> an input-specific library, because they will presumably be used by
> >>> the codec driver as well, since that driver will write registers to
> >>> set BCLK/LRCK ratio, etc.
> >>> 
> >>> Therefore, I think it makes more sense for these functions to move to
> >>> the MFD, which can then export them for use by the input/FF and codec
> >>> children.
> >> 
> >> I think the problem with moving the write sequencer code to the MFD
> >> driver is that it would go unused in a codec-only environment. We only
> >> need to write to the PSEQ when we want to maintain register settings
> >> throughout hibernation cycles, and it isn’t possible to hibernate when
> >> there is data streaming to the device. So the PSEQ will never be used
> >> in the codec driver.
> > 
> > I agree that in practice, the write sequencer would technically go unused
> > in a codec-only implementation. However, that is because the ASoC core
> > happens to write all pertinent registers ahead-of-time each time a stream
> > starts. That is a property of the ASoC core and not L50; my feeling is that
> > the driver should not be structured based on what one of the subsystems
> > associated with it happens to do, but rather the nature of the hardware.
> > 
> > Some specific reasons I think the MFD is a better home for the pseq code:
> > 
> > 1. The write sequencer is a housekeeping function derived from the way
> > the hardware implements its power management; it doesn't have anything
> > to do with haptics. My opinion is that facilities supporting application-
> > agnostic functions belong in the MFD, for all children to access, even
> > if only one happens to do so today.
> > 
> > 2. Over the course of the IC's life, you may be required to add errata
> > writes after the IC is taken out of reset; these presumably would need
> > to be "scheduled" in the write sequencer as well. These wouldn't make
> > logical sense to add in the input driver, and they would be missed in
> > the theoretical codec-only case.
> > 
> > 3. This device has a programmable DSP; it wouldn't be unheard of for a
> > customer to ask for a new function down the road that begets a third
> > child device. Perhaps a customer asks for a special .wmfw file that
> > repurposes the SDOUT pin as a PWM output for an LED, and now you must
> > add a pwm child. That's a made-up example of course, but in general we
> > want to structure things in such a way that rip-up is minimal in case
> > requirements change in the future.
> 
> Great points. I agree now that the write sequencer code ought not to go in
> cirrus_haptics.c. After talking it over with the internal team, I am considering
> moving the write sequencer interface to cs_dsp.c. It’s an already existing
> library with both Cirrus haptics and audio users. It seems to dodge your
> concerns above and avoids a new common library as you suggested
> below. Do you have any concerns on this approach over putting it in MFD?

I think that's a great idea. Not every DSP-equipped device has a write
sequencer, but most, if not all write-sequencer-equipped devices have
a DSP. Adding the write sequencer code to cs_dsp.c seems like a natural
union of the two facilities.

> 
> 
> >>> This leaves cirrus_haptics.* with only a few functions related to
> >>> starting and stopping work, which seem specific enough to just live
> >>> in cs40l50-vibra.c. Presumably many of those could be re-used by
> >>> the L30 down the road, but in that case I think we'd be looking to
> >>> actually re-use the L50 driver and simply add a compatible string
> >>> for L30.
> >>> 
> >>> I recall L30 has some overhead that L50 does not, which may make
> >>> it hairy for cs40l50-vibra.c to support both. But in that case,
> >>> you could always fork a cs40l30-vibra.c with its own compatible
> >>> string, then have the L50 MFD selectively load the correct child
> >>> based on device ID. To summarize, we should have:
> >>> 
> >>> * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery,
> >>> IRQ handling, exported PSEQ functions, etc.
> >>> * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from
> >>> the MFD.
> >>> * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop
> >>> work, also uses PSEQ library from the MFD.
> >>> 
> >>> And down the road, depending on complexity, maybe we also have:
> >>> 
> >>> * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that
> >>> includes other functionality that didn't really fit in cs40l50-vibra.c;
> >>> also uses PSEQ library from, and is loaded by, the original L50 MFD.
> >>> If this driver duplicates small bits of cs40l50-vibra.c, it's not the
> >>> end of the world.
> >>> 
> >>> All of these files would #include include/linux/mfd/cs40l50.h. And
> >>> finally, cirrus_haptics.* simply go away. Same idea, just slightly
> >>> more scalable, and closer to common design patterns.
> >> 
> >> 
> >> I understand that it is a common design pattern to selectively load
> >> devices from the MFD driver. If I could summarize my thoughts on why
> >> that would not be fitting here, it’s that the L26 and L50 share a ton of
> >> input FF related work, and not enough “MFD core” related work.
> >> Between errata differences, power supply configurations, regmap
> >> configurations, interrupt register differences, it would seem to make for
> >> a very awkward, scrambled MFD driver. Moreover, I think I will be moving
> >> firmware download to the MFD driver, and that alone constitutes a
> >> significant incompatibility because firmware downloading is compulsory
> >> on L26, not so on L50.
> >> 
> >> On the other hand, I want to emphasize the amount that L26 and
> >> L50 share when it comes to the Input FF callbacks. The worker
> >> functions in cirrus_haptics.c are bare-bones for this first
> >> submission, but were designed to be totally generic and scalable to
> >> the needs of L26 and all future Cirrus input drivers. While it might appear
> >> too specific for L26, everything currently in cirrus_haptics is usable by
> >> L26 as-is.
> >> 
> >> For the above reasons I favor the current approach.
> >> 
> > 
> > Likewise, if the input-related functions of L26 and L50 are nearly identical,
> > then it's also perfectly acceptable for both drivers/mfd/cs40l26.c and
> > drivers/mfd/cs40l50.c to load drivers/input/misc/cs40l50-vibra.c, which
> > supports both L26 and L50 haptics-related functions. You're already doing
> > something similar, but I disagree on the following:
> > 
> > 1. Rather than have a library referenced by two drivers that support children
> > which are largely the same in a logcial sense, just have a single driver that
> > supports two children.
> 
> Your point here is clear and makes sense to me, especially now with the write
> sequencer interface moving out. After considering the similarities and
> differences closer, I am still a little wary. Maybe you can help me with these
> concerns:
> 
> 1. In the current implementation, drivers would be able to configure their own
> input FF capabilities, and selectively register to input FF callbacks. L50 does
> not register to the set_gain callback, whereas L26 does. I anticipate future
> divergences, such as one driver supporting different effect types (see
> the L50-specific error checking in cs40l50_add()). This would be exacerbated
> by any future additional children.
> 
> 2. This may be my lack of imagination, but in the current implementation it
> seems much easier to develop new haptic features that don’t apply to all the
> users of the library. One would simply wrap the feature in a boolean in
> cirrus_haptics, which clients can take or leave. In the one driver
> implementation, it seems you would have to find some clever, generalized
> way of determining whether or not a feature can be used. This would also
> seem to be exacerbated by any future additional children.
> 
> 3. The current implementation provides for the individual drivers to setup
> the haptics interface in whatever way peculiar to that device, whether that
> interface be static (L50) or dependent on the loaded firmware (L26).
> 

Maybe the solution here is to define a "descriptor", that is a struct which
reserves members for all things that can vary based on device (FF callback
pointers, input device capabilities, etc.). You then define an array of such
structs, with L50 being the only member, and L26 and others to come. Then,
cs40l50_vibra_probe() would simply parse the array index based on the device
ID hinted to it by the parent MFD.

As the person not doing the actual work, my naive opinion is that you face the
same problems whether two different drivers reference a library, or a common
driver supports two devices. The problem is therefore cosmetic, and if so, we
should tend toward a design pattern that seems to be most common in the input
subsystem, which is to support as many devices as possible with a single driver.

> Since I am moving around a lot of code in and out of both -vibra.c and the
> library for the next version, I think it would be helpful for me to wait until the
> next version is submitted to decide on this. Would that be acceptable?

I very much support moving in incremental steps; if you still feel strongly
against my suggestion, let us by all means evaluate your approach in the
context of the other changes.

> 
> > 
> > 
> >> 
> >>>> +static void cs_hap_vibe_stop_worker(struct work_struct *work)
> >>>> +{
> >>>> + struct cs_hap *haptics = container_of(work, struct cs_hap,
> >>>> +       vibe_stop_work);
> >>>> + int error;
> >>>> +
> >>>> + if (haptics->runtime_pm) {
> >>>> + error = pm_runtime_resume_and_get(haptics->dev);
> >>>> + if (error < 0) {
> >>>> + haptics->stop_error = error;
> >>>> + return;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + mutex_lock(&haptics->lock);
> >>>> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> >>>> +      haptics->dsp.stop_cmd);
> >>>> + mutex_unlock(&haptics->lock);
> >>>> +
> >>>> + if (haptics->runtime_pm) {
> >>>> + pm_runtime_mark_last_busy(haptics->dev);
> >>>> + pm_runtime_put_autosuspend(haptics->dev);
> >>>> + }
> >>>> +
> >>>> + haptics->stop_error = error;
> >>> 
> >>> This seems like another argument for not separating the input/FF child
> >>> from the meat of the driver; it just seems messy to pass around error
> >>> codes within a driver's private data like this.
> >> 
> >> I removed the start_error and stop_error members. However I think the
> >> erase_error and add_error need to stay. I think this is more of a symptom
> >> of the Input FF layer requiring error reporting and having to use workqueues
> >> for those Input FF callbacks, than it is to do with the separation of these
> >> functions from the driver. Point being, even if these were moved, we would still
> >> need these *_error members. Let me know if I understood you right here.
> > 
> > Sure, but why do adding and removing waveforms require workqueues? The DA7280
> > driver doesn't do this; what is different in this case? (That's a genuine
> > question, not an assertion that what you have is wrong, although it seems
> > unique based on my limited search).
> 
> The reason why we have worker items for upload and erase input FF callbacks is
> because we need to ensure their ordering with playback worker items, and we need
> those worker items because the Input FF layer calls playbacks in atomic context.

Got it, that makes sense. Presumably, the act of adding or deleting waveforms
directly manipulates XMEM and YMEM regions of the DSP, and we cannot do so while
they are actively being read during playback. Acknowledged on all counts.

> 
> Best,
> James
> 
> 

Kind regards,
Jeff LaBundy