[PATCH v3 0/2] Add support for CS40L50

[PATCH v3 0/2] Add support for CS40L50

[James Ogletree]

Changes in v2:
- Fix checkpatch warnings

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

James Ogletree (2):
  dt-bindings: input: cirrus,cs40l50: Support for CS40L50
  Input: cs40l50 - Initial support for Cirrus Logic CS40L50

 .../bindings/input/cirrus,cs40l50.yaml        |   77 ++
 MAINTAINERS                                   |   10 +
 drivers/input/misc/Kconfig                    |   33 +
 drivers/input/misc/Makefile                   |    3 +
 drivers/input/misc/cs40l50-i2c.c              |   67 ++
 drivers/input/misc/cs40l50-spi.c              |   67 ++
 drivers/input/misc/cs40l50.c                  | 1008 +++++++++++++++++
 include/linux/input/cs40l50.h                 |  321 ++++++
 8 files changed, 1586 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
 create mode 100644 drivers/input/misc/cs40l50-i2c.c
 create mode 100644 drivers/input/misc/cs40l50-spi.c
 create mode 100644 drivers/input/misc/cs40l50.c
 create mode 100644 include/linux/input/cs40l50.h

-- 
2.25.1

[PATCH v3 1/2] dt-bindings: input: cirrus,cs40l50: Support for CS40L50

[James Ogletree]

Introduce device tree bindings for the initial commit
of the CS40L50 driver.

Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
---
 .../bindings/input/cirrus,cs40l50.yaml        | 77 +++++++++++++++++++
 MAINTAINERS                                   |  8 ++
 2 files changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml

diff --git a/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml b/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
new file mode 100644
index 000000000000..0da7211efdec
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/cirrus,cs40l50.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cirrus Logic CS40L50 Advanced Haptic Driver
+
+maintainers:
+  - James Ogletree <james.ogletree@cirrus.com>
+
+description:
+  CS40L50 is a Haptic Driver with Waveform Memory DSP
+  and Closed-Loop Algorithms
+
+properties:
+  compatible:
+    enum:
+      - cirrus,cs40l50
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  va-supply:
+    description: Regulator for analog voltage
+
+  vp-supply:
+    description: Regulator for peak voltage
+
+  vio-supply:
+    description: Regulator for digital IO voltage
+
+  cirrus,external-boost:
+    description:
+      Bypass the CS40L50 boost converter.
+
+      The amplifier on CS40L50 can be powered internally through the boost
+      converter, or else can be powered using an external supply. If an
+      external supply is used, the boost converter must be bypassed.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/input/input.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      haptic-driver@34 {
+        compatible = "cirrus,cs40l50";
+        reg = <0x34>;
+        interrupt-parent = <&gpio>;
+        interrupts = <113 IRQ_TYPE_LEVEL_LOW>;
+        reset-gpios = <&gpio 112 GPIO_ACTIVE_LOW>;
+        va-supply = <&vreg>;
+        vp-supply = <&vreg>;
+        vio-supply = <&vreg>;
+        cirrus,external-boost;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 54ef320bb689..2869dd2dbbcf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2081,6 +2081,14 @@ F:	arch/arm/boot/compressed/misc-ep93xx.h
 F:	arch/arm/mach-ep93xx/
 F:	drivers/iio/adc/ep93xx_adc.c
 
+CIRRUS LOGIC HAPTICS DRIVER
+M:	James Ogletree <james.ogletree@cirrus.com>
+M:	Fred Treven <fred.treven@cirrus.com>
+M:	Ben Bright <ben.bright@cirrus.com>
+L:	patches@opensource.cirrus.com
+S:	Supported
+F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
+
 ARM/CLKDEV SUPPORT
 M:	Russell King <linux@armlinux.org.uk>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
-- 
2.25.1

Re: [PATCH v3 1/2] dt-bindings: input: cirrus,cs40l50: Support for CS40L50

[Krzysztof Kozlowski]

On 09/08/2023 21:10, James Ogletree wrote:
> Introduce device tree bindings for the initial commit
> of the CS40L50 driver.
>

> +  va-supply:
> +    description: Regulator for analog voltage
> +
> +  vp-supply:
> +    description: Regulator for peak voltage
> +
> +  vio-supply:
> +    description: Regulator for digital IO voltage
> +
> +  cirrus,external-boost:
> +    description:
> +      Bypass the CS40L50 boost converter.
> +
> +      The amplifier on CS40L50 can be powered internally through the boost
> +      converter, or else can be powered using an external supply. If an
> +      external supply is used, the boost converter must be bypassed.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - reset-gpios

I would expect at least some supplies/regulators to be required. This
should match what the datasheet says. The rest looks good.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[James Ogletree]

Introduce support for Cirrus Logic Device CS40L50: a
haptics driver with waveform memory DSP and closed-loop
algorithms.

Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
---
 MAINTAINERS                      |    2 +
 drivers/input/misc/Kconfig       |   33 +
 drivers/input/misc/Makefile      |    3 +
 drivers/input/misc/cs40l50-i2c.c |   67 ++
 drivers/input/misc/cs40l50-spi.c |   67 ++
 drivers/input/misc/cs40l50.c     | 1008 ++++++++++++++++++++++++++++++
 include/linux/input/cs40l50.h    |  321 ++++++++++
 7 files changed, 1501 insertions(+)
 create mode 100644 drivers/input/misc/cs40l50-i2c.c
 create mode 100644 drivers/input/misc/cs40l50-spi.c
 create mode 100644 drivers/input/misc/cs40l50.c
 create mode 100644 include/linux/input/cs40l50.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 2869dd2dbbcf..e82d03293383 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2088,6 +2088,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/cs40l*
+F:	include/linux/input/cs40l*
 
 ARM/CLKDEV SUPPORT
 M:	Russell King <linux@armlinux.org.uk>
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index c47eecc6f83b..c1c1572ec39c 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -140,6 +140,39 @@ config INPUT_BMA150
 	  To compile this driver as a module, choose M here: the
 	  module will be called bma150.
 
+config INPUT_CS40L50
+	tristate "Cirrus Logic CS40L50 Haptic Driver support"
+	select CS_DSP
+	select REGMAP_IRQ
+	help
+	  Say Y here to enable support for CS40L50 boosted haptic
+	  driver.
+
+	  To compile the driver as a module choose M here: the module
+	  will be called cs40l50.
+
+config INPUT_CS40L50_I2C
+	tristate "Support I2C bus connection"
+	depends on INPUT_CS40L50 && I2C
+	select REGMAP_I2C
+	help
+	  Say Y here if you have Cirrus Logic's CS40L50 connected
+	  to an I2C bus.
+
+	  To compile the driver as a module choose M here: the
+	  module will be called cs40l50_i2c.
+
+config INPUT_CS40L50_SPI
+	tristate "Support SPI bus connection"
+	depends on INPUT_CS40L50 && SPI
+	select REGMAP_SPI
+	help
+	  Say Y here if you have Cirrus Logic's CS40L50 connected
+	  to a SPI bus.
+
+	  To compile the driver as a module choose M here: the
+	  module will be called cs40l50_spi.
+
 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 04296a4abe8e..77f34a33c364 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -28,6 +28,9 @@ 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)		+= cs40l50.o
+obj-$(CONFIG_INPUT_CS40L50_I2C)		+= cs40l50-i2c.o
+obj-$(CONFIG_INPUT_CS40L50_SPI)		+= cs40l50-spi.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-i2c.c b/drivers/input/misc/cs40l50-i2c.c
new file mode 100644
index 000000000000..6ebe1b51990b
--- /dev/null
+++ b/drivers/input/misc/cs40l50-i2c.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
+ * and Closed-Loop Algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ */
+#include <linux/i2c.h>
+#include <linux/input/cs40l50.h>
+
+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 int cs40l50_i2c_probe(struct i2c_client *client)
+{
+	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 struct i2c_driver cs40l50_i2c_driver = {
+	.driver = {
+		.name = "cs40l50",
+		.of_match_table = cs40l50_of_match,
+	},
+	.id_table = cs40l50_id_i2c,
+	.probe_new = 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/input/misc/cs40l50-spi.c b/drivers/input/misc/cs40l50-spi.c
new file mode 100644
index 000000000000..5fcf3ba137f0
--- /dev/null
+++ b/drivers/input/misc/cs40l50-spi.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
+ * and Closed-Loop Algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ */
+#include <linux/spi/spi.h>
+#include <linux/input/cs40l50.h>
+
+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 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 struct spi_driver cs40l50_spi_driver = {
+	.driver = {
+		.name = "cs40l50",
+		.of_match_table = cs40l50_of_match,
+	},
+	.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/drivers/input/misc/cs40l50.c b/drivers/input/misc/cs40l50.c
new file mode 100644
index 000000000000..9bb2881683c0
--- /dev/null
+++ b/drivers/input/misc/cs40l50.c
@@ -0,0 +1,1008 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
+ * and Closed-Loop Algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ */
+#include <linux/firmware.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/bitfield.h>
+#include <linux/input/cs40l50.h>
+
+const struct regmap_config cs40l50_regmap = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.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" },
+	{ .supply = "va" },
+};
+
+static int cs40l50_dsp_write(struct cs40l50_private *cs40l50, u32 write_reg, u32 write_val)
+{
+	int error = 0, i;
+
+	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
+		error = regmap_write(cs40l50->regmap, write_reg, write_val);
+		if (!error)
+			break;
+
+		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
+	}
+
+	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
+		dev_err(cs40l50->dev, "Error writing %#X to %#X: %d", write_val, write_reg, error);
+		return error;
+	}
+
+	return 0;
+}
+
+static int cs40l50_dsp_read(struct cs40l50_private *cs40l50, u32 read_reg, u32 *read_val)
+{
+	int error = 0, i;
+
+	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
+		error = regmap_read(cs40l50->regmap, read_reg, read_val);
+		if (!error)
+			break;
+
+		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
+	}
+
+	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
+		dev_err(cs40l50->dev, "Error reading %#X: %d", read_reg, error);
+		return error;
+	}
+
+	return 0;
+}
+
+static int cs40l50_pseq_write(struct cs40l50_private *cs40l50, u32 addr, u32 data)
+{
+	u8 operation = 0;
+	struct cs40l50_pseq_op *op_new, *op_end;
+	int error;
+
+	op_new = devm_kzalloc(cs40l50->dev, sizeof(*op_new), GFP_KERNEL);
+	if (!op_new)
+		return -ENOMEM;
+
+	op_new->words[0] = (addr >> CS40L50_PSEQ_WRITE_FULL_UPPER_ADDR_SHIFT);
+	op_new->words[1] = ((addr & CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_MASK) <<
+			CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_SHIFT) |
+			(data >> CS40L50_PSEQ_WRITE_FULL_UPPER_DATA_SHIFT);
+	op_new->words[2] = data & CS40L50_PSEQ_WRITE_FULL_LOWER_DATA_MASK;
+
+	list_for_each_entry(op_end, &cs40l50->pseq_op_head, list) {
+		operation = op_end->operation;
+		if (operation == CS40L50_PSEQ_OP_END)
+			break;
+	}
+
+	if (operation != CS40L50_PSEQ_OP_END) {
+		error = -ENOENT;
+		goto op_new_free;
+	}
+
+	if ((CS40L50_PSEQ_MAX_WORDS - op_end->offset) < ARRAY_SIZE(op_new->words)) {
+		error = -ENOSPC;
+		goto op_new_free;
+	}
+
+	op_new->offset = op_end->offset;
+	op_end->offset += (ARRAY_SIZE(op_new->words) * sizeof(u32));
+
+	error = regmap_bulk_write(cs40l50->regmap, CS40L50_DSP1_POWER_ON_SEQ + op_new->offset,
+			op_new->words, ARRAY_SIZE(op_new->words));
+	if (error)
+		goto op_new_free;
+
+	error = cs40l50_dsp_write(cs40l50, CS40L50_DSP1_POWER_ON_SEQ + op_end->offset,
+			op_end->words[0]);
+	if (error)
+		goto op_new_free;
+
+	list_add(&op_new->list, &cs40l50->pseq_op_head);
+
+	return 0;
+
+op_new_free:
+	devm_kfree(cs40l50->dev, op_new);
+
+	return error;
+}
+
+static struct cs40l50_effect *cs40l50_find_effect(struct cs40l50_private *cs40l50, int id)
+{
+	struct cs40l50_effect *effect;
+
+	if (!list_empty(&cs40l50->effect_head)) {
+		list_for_each_entry(effect, &cs40l50->effect_head, list) {
+			if (effect->id == id)
+				return effect;
+		}
+	}
+
+	return NULL;
+}
+
+static int cs40l50_owt_upload(struct cs40l50_private *cs40l50, s16 *in_data, u32 in_data_nibbles)
+{
+	size_t in_data_bytes = 2 * in_data_nibbles;
+	struct cs40l50_owt_header header;
+	u32 wt_offset, wt_size_words;
+	int error;
+	u8 *data;
+
+	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_OWT_NEXT, &wt_offset);
+	if (error)
+		return error;
+
+	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_OWT_SIZE, &wt_size_words);
+	if (error)
+		return error;
+
+	if ((wt_size_words * sizeof(u32)) < CS40L50_WT_HEADER_PWLE_SIZE + in_data_bytes)
+		return -ENOSPC;
+
+	data = kcalloc(CS40L50_WT_HEADER_PWLE_SIZE + in_data_bytes, sizeof(u8), GFP_KERNEL);
+	if (IS_ERR_OR_NULL(data))
+		return -ENOMEM;
+
+	header.wvfrm_type = in_data[0] == CS40L50_PCM_ID ? CS40L50_WT_TYPE_PCM :
+			CS40L50_WT_TYPE_PWLE;
+	header.offset = CS40L50_WT_HEADER_OFFSET;
+	header.data_size = in_data_bytes / sizeof(u32);
+
+	memcpy(data, &header, sizeof(header));
+	memcpy(data + (CS40L50_WT_HEADER_OFFSET * sizeof(u32)), in_data, in_data_bytes);
+
+	error = regmap_bulk_write(cs40l50->regmap, CS40L50_DSP1_OWT_BASE +
+			(wt_offset * sizeof(u32)), data,
+			CS40L50_WT_HEADER_PWLE_SIZE + in_data_bytes);
+	if (error)
+		goto err_free;
+
+	error = cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1, CS40L50_MBOX_CMD_OWT_PUSH);
+err_free:
+	kfree(data);
+
+	return error;
+}
+
+static int cs40l50_add_effect(struct input_dev *dev, struct ff_effect *effect,
+		struct ff_effect *old)
+{
+	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
+	bool is_new = false;
+	struct cs40l50_effect *new_effect;
+	u32 nwaves, base_index, max_index;
+	s16 *raw_custom_data;
+	int error;
+
+	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;
+	}
+
+	raw_custom_data = kcalloc(effect->u.periodic.custom_len, sizeof(s16), GFP_KERNEL);
+	if (IS_ERR_OR_NULL(raw_custom_data))
+		return -ENOMEM;
+
+	error = copy_from_user(raw_custom_data, effect->u.periodic.custom_data,
+			sizeof(s16) * effect->u.periodic.custom_len);
+	if (error)
+		goto out_free;
+
+	mutex_lock(&cs40l50->lock);
+
+	new_effect = cs40l50_find_effect(cs40l50, effect->id);
+	if (!new_effect) {
+		is_new = true;
+		new_effect = kzalloc(sizeof(*new_effect), GFP_KERNEL);
+		if (IS_ERR_OR_NULL(new_effect)) {
+			error = -ENOMEM;
+			goto err_mutex;
+		}
+	}
+
+	if (effect->u.periodic.custom_len > CS40L50_CUSTOM_DATA_SIZE) {
+		error = cs40l50_dsp_read(cs40l50, CS40L50_NUM_OF_OWT_WAVES,
+				&new_effect->trigger_index);
+		if (error)
+			goto err_free;
+
+		new_effect->wvfrm_bank = CS40L50_WVFRM_BANK_OWT;
+		base_index = CS40L50_OWT_INDEX_START;
+		max_index = CS40L50_OWT_INDEX_END;
+	} else {
+		new_effect->trigger_index = (raw_custom_data[1] & 0xffffu);
+		new_effect->wvfrm_bank = (raw_custom_data[0] & 0xffffu);
+
+		if (new_effect->wvfrm_bank == CS40L50_WVFRM_BANK_ROM) {
+			base_index = CS40L50_ROM_BANK_INDEX_START;
+			max_index = CS40L50_ROM_BANK_INDEX_END;
+		} else if (new_effect->wvfrm_bank == CS40L50_WVFRM_BANK_RAM) {
+			error = cs40l50_dsp_read(cs40l50, CS40L50_NUM_OF_WAVES, &nwaves);
+			if (error)
+				goto err_free;
+
+			base_index = CS40L50_RAM_BANK_INDEX_START;
+			max_index = base_index + nwaves - 1;
+		} else {
+			dev_err(cs40l50->dev, "Invalid waveform bank\n");
+			error = -EINVAL;
+			goto err_free;
+		}
+	}
+
+	new_effect->trigger_index += base_index;
+
+	if (new_effect->trigger_index > max_index) {
+		dev_err(cs40l50->dev, "Index %#X out of bounds\n", new_effect->trigger_index);
+		error = -EINVAL;
+		goto err_free;
+	}
+
+	if (effect->trigger.button) {
+		new_effect->mapping = CS40L50_DSP1_GPIO_BASE + sizeof(u32) *
+				FIELD_GET(CS40L50_BTN_NUM_MASK, effect->trigger.button) * 2 -
+				FIELD_GET(CS40L50_BTN_EDGE_MASK, effect->trigger.button);
+
+		error = cs40l50_dsp_write(cs40l50, new_effect->mapping, effect->trigger.button);
+		if (error)
+			goto err_free;
+	}
+
+	if (new_effect->wvfrm_bank == CS40L50_WVFRM_BANK_OWT) {
+		error = cs40l50_owt_upload(cs40l50, raw_custom_data,
+				effect->u.periodic.custom_len);
+		if (error)
+			goto err_free;
+	}
+
+err_free:
+	if (is_new)
+		error ? kfree(new_effect) : list_add(&new_effect->list, &cs40l50->effect_head);
+err_mutex:
+	mutex_unlock(&cs40l50->lock);
+out_free:
+	kfree(raw_custom_data);
+
+	return error ? -EFAULT : 0;
+}
+
+
+static void cs40l50_vibe_start_worker(struct work_struct *work)
+{
+	struct cs40l50_private *cs40l50 =
+			container_of(work, struct cs40l50_private, vibe_start_work);
+	struct cs40l50_effect *effect;
+
+	mutex_lock(&cs40l50->lock);
+
+	effect = cs40l50_find_effect(cs40l50, cs40l50->trigger_effect->id);
+	if (effect)
+		cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1, effect->trigger_index);
+
+	mutex_unlock(&cs40l50->lock);
+}
+
+static void cs40l50_vibe_stop_worker(struct work_struct *work)
+{
+	struct cs40l50_private *cs40l50 =
+			container_of(work, struct cs40l50_private, vibe_stop_work);
+
+	mutex_lock(&cs40l50->lock);
+
+	cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1, CS40L50_MBOX_CMD_STOP_PLAYBACK);
+
+	mutex_unlock(&cs40l50->lock);
+}
+
+static int cs40l50_playback_effect(struct input_dev *dev, int effect_id, int val)
+{
+	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
+
+	cs40l50->trigger_effect = &dev->ff->effects[effect_id];
+
+	if (val > 0)
+		queue_work(cs40l50->vibe_workqueue, &cs40l50->vibe_start_work);
+	else
+		queue_work(cs40l50->vibe_workqueue, &cs40l50->vibe_stop_work);
+
+	return 0;
+}
+
+static int cs40l50_erase_effect(struct input_dev *dev, int effect_id)
+{
+	int error = 0;
+	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
+	struct cs40l50_effect *effect, *effect_owt;
+
+	mutex_lock(&cs40l50->lock);
+
+	effect = cs40l50_find_effect(cs40l50, dev->ff->effects[effect_id].id);
+	if (!effect) {
+		error = -EINVAL;
+		goto err_mutex;
+	}
+
+	if (effect->mapping != CS40L50_GPIO_MAPPING_INVALID) {
+		error = cs40l50_dsp_write(cs40l50, effect->mapping, CS40L50_GPI_DISABLE);
+		if (error)
+			goto err_mutex;
+	}
+
+	if (effect->wvfrm_bank == CS40L50_WVFRM_BANK_OWT) {
+		error = cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1,
+				CS40L50_MBOX_CMD_OWT_DELETE | effect->trigger_index);
+		if (error)
+			goto err_mutex;
+
+		list_for_each_entry(effect_owt, &cs40l50->effect_head, list) {
+			if (effect_owt->wvfrm_bank == CS40L50_WVFRM_BANK_OWT &&
+					effect_owt->trigger_index > effect->trigger_index)
+				effect_owt->trigger_index--;
+		}
+	}
+
+	list_del(&effect->list);
+	kfree(effect);
+err_mutex:
+	mutex_unlock(&cs40l50->lock);
+
+	return error;
+}
+
+static int cs40l50_mailbox_read_next(struct cs40l50_private *cs40l50, u32 *val)
+{
+	u32 wt_ptr, rd_ptr;
+	int error = 0;
+
+	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_MBOX_QUEUE_WT, &wt_ptr);
+	if (error)
+		return error;
+
+	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_MBOX_QUEUE_RD, &rd_ptr);
+	if (error)
+		return error;
+
+	if (wt_ptr == rd_ptr) {
+		*val = 0;
+		return 0;
+	}
+
+	error = cs40l50_dsp_read(cs40l50, 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 cs40l50_dsp_write(cs40l50, CS40L50_DSP1_MBOX_QUEUE_RD, rd_ptr);
+}
+
+static int cs40l50_handle_f0_est_done(struct cs40l50_private *cs40l50)
+{
+	int error;
+	u32 f_zero;
+
+	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_F0_ESTIMATION, &f_zero);
+	if (error)
+		return error;
+
+	return cs40l50_dsp_write(cs40l50, CS40L50_DSP1_F0_STORED, f_zero);
+}
+
+static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
+{
+	int integer, fractional, error;
+	u32 redc;
+
+	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_RE_EST_STATUS, &redc);
+	if (error)
+		return error;
+
+	error = cs40l50_dsp_write(cs40l50, CS40L50_DSP1_REDC_ESTIMATION, redc);
+	if (error)
+		return error;
+
+	/* Convert from Q8.15 to (Q7.17 * 29/240) */
+	integer = (redc >> 15) & 0xFF;
+	fractional = (redc & 0x7FFF) * 4;
+
+	return cs40l50_dsp_write(cs40l50, CS40L50_DSP1_REDC_STORED,
+		(((integer << 17) | fractional) * 29 / 240) & GENMASK(23, 0));
+}
+
+static int cs40l50_error_release(struct cs40l50_private *cs40l50)
+{
+	int error;
+
+	error = cs40l50_dsp_write(cs40l50, CS40L50_ERROR_RELEASE, CS40L50_ERROR_RELEASE_MASK);
+	if (error)
+		return error;
+
+	return cs40l50_dsp_write(cs40l50, CS40L50_ERROR_RELEASE, 0);
+}
+
+static irqreturn_t cs40l50_handle_mbox_buffer(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:
+			goto out_irq;
+		case CS40L50_DSP_MBOX_HAPTIC_TRIGGER_GPIO:
+			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
+			break;
+		case CS40L50_DSP_MBOX_HAPTIC_TRIGGER_MBOX:
+			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_MBOX\n");
+			break;
+		case CS40L50_DSP_MBOX_HAPTIC_TRIGGER_I2S:
+			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_I2S\n");
+			break;
+		case CS40L50_DSP_MBOX_HAPTIC_COMPLETE_MBOX:
+			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_MBOX\n");
+			break;
+		case CS40L50_DSP_MBOX_HAPTIC_COMPLETE_GPIO:
+			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_GPIO\n");
+			break;
+		case CS40L50_DSP_MBOX_HAPTIC_COMPLETE_I2S:
+			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_I2S\n");
+			break;
+		case CS40L50_DSP_MBOX_F0_EST_START:
+			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_START\n");
+			break;
+		case CS40L50_DSP_MBOX_F0_EST_DONE:
+			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_DONE\n");
+			error = cs40l50_handle_f0_est_done(cs40l50);
+			if (error)
+				goto out_irq;
+			break;
+		case CS40L50_DSP_MBOX_REDC_EST_START:
+			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_START\n");
+			break;
+		case CS40L50_DSP_MBOX_REDC_EST_DONE:
+			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_DONE\n");
+			error = cs40l50_handle_redc_est_done(cs40l50);
+			if (error)
+				goto out_irq;
+			break;
+		case CS40L50_DSP_MBOX_LE_EST_START:
+			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_START\n");
+			break;
+		case CS40L50_DSP_MBOX_LE_EST_DONE:
+			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_DONE\n");
+			break;
+		case CS40L50_DSP_MBOX_AWAKE:
+			dev_dbg(cs40l50->dev, "Mailbox: AWAKE\n");
+			break;
+		case CS40L50_DSP_MBOX_INIT:
+			dev_dbg(cs40l50->dev, "Mailbox: INIT\n");
+			break;
+		case CS40L50_DSP_MBOX_ACK:
+			dev_dbg(cs40l50->dev, "Mailbox: ACK\n");
+			break;
+		case CS40L50_DSP_MBOX_ERR_EVENT_UNMAPPED:
+			dev_err(cs40l50->dev, "Unmapped event\n");
+			break;
+		case CS40L50_DSP_MBOX_ERR_EVENT_MODIFY:
+			dev_err(cs40l50->dev, "Attempt to modify event index failed\n");
+			break;
+		case CS40L50_DSP_MBOX_ERR_NULLPTR:
+			dev_err(cs40l50->dev, "Null pointer\n");
+			break;
+		case CS40L50_DSP_MBOX_ERR_BRAKING:
+			dev_err(cs40l50->dev, "Braking not in progress\n");
+			break;
+		case CS40L50_DSP_MBOX_ERR_INVAL_SRC:
+			dev_err(cs40l50->dev, "Suspend/resume invalid source\n");
+			break;
+		case CS40L50_DSP_MBOX_ERR_ENABLE_RANGE:
+			dev_err(cs40l50->dev, "GPIO enable out of range\n");
+			break;
+		case CS40L50_DSP_MBOX_ERR_GPIO_UNMAPPED:
+			dev_err(cs40l50->dev, "GPIO not mapped\n");
+			break;
+		case CS40L50_DSP_MBOX_ERR_ISR_RANGE:
+			dev_err(cs40l50->dev, "GPIO ISR out of range\n");
+			break;
+		case CS40L50_DSP_MBOX_PERMANENT_SHORT:
+			dev_crit(cs40l50->dev, "Permanent short detected\n");
+			break;
+		case CS40L50_DSP_MBOX_RUNTIME_SHORT:
+			dev_err(cs40l50->dev, "Runtime short detected\n");
+			error = cs40l50_error_release(cs40l50);
+			if (error)
+				goto out_irq;
+			break;
+		default:
+			dev_err(cs40l50->dev, "Mailbox value %#X not recognized\n", val);
+			error = -EINVAL;
+			goto out_irq;
+		}
+	}
+
+	error = -EIO;
+
+out_irq:
+	mutex_unlock(&cs40l50->lock);
+
+	return IRQ_RETVAL(!error);
+}
+
+
+static irqreturn_t cs40l50_vddb_uv(int irq, void *data)
+{
+	struct cs40l50_private *cs40l50 = data;
+
+	dev_err(cs40l50->dev, "VDD_B undervoltage error\n");
+
+	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
+}
+
+static irqreturn_t cs40l50_boost_current_limit(int irq, void *data)
+{
+	struct cs40l50_private *cs40l50 = data;
+
+	dev_err(cs40l50->dev, "Boost current limit engagement\n");
+
+	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
+}
+
+static irqreturn_t cs40l50_boost_short(int irq, void *data)
+{
+	struct cs40l50_private *cs40l50 = data;
+
+	dev_err(cs40l50->dev, "Boost inductor short error\n");
+
+	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
+}
+
+static irqreturn_t cs40l50_boost_uv(int irq, void *data)
+{
+	struct cs40l50_private *cs40l50 = data;
+
+	dev_err(cs40l50->dev, "Boost undervoltage error\n");
+
+	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
+}
+
+static irqreturn_t cs40l50_overtemperature(int irq, void *data)
+{
+	struct cs40l50_private *cs40l50 = data;
+
+	dev_err(cs40l50->dev, "Overtemperature error\n");
+
+	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
+}
+
+static irqreturn_t cs40l50_amp_short(int irq, void *data)
+{
+	struct cs40l50_private *cs40l50 = data;
+
+	dev_err(cs40l50->dev, "Amp short error\n");
+
+	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
+}
+
+static irqreturn_t cs40l50_global_error(int irq, void *data)
+{
+	struct cs40l50_private *cs40l50 = data;
+
+	dev_err(cs40l50->dev, "MSM chip error\n");
+
+	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
+}
+
+static const struct cs40l50_irq cs40l50_irqs[] = {
+	CS40L50_IRQ(CS40L50_AMP_SHORT_IRQ, "Amp short", cs40l50_amp_short),
+	CS40L50_IRQ(CS40L50_VIRT2_MBOX_IRQ, "Firmware interrupt", cs40l50_handle_mbox_buffer),
+	CS40L50_IRQ(CS40L50_TEMP_ERR_IRQ, "Overtemperature", cs40l50_overtemperature),
+	CS40L50_IRQ(CS40L50_BST_UVP_IRQ, "Boost undervoltage", cs40l50_boost_uv),
+	CS40L50_IRQ(CS40L50_BST_SHORT_IRQ, "Boost short", cs40l50_boost_short),
+	CS40L50_IRQ(CS40L50_BST_ILIMIT_IRQ, "Boost current limit", cs40l50_boost_current_limit),
+	CS40L50_IRQ(CS40L50_UVLO_VDDBATT_IRQ, "VDD_B undervoltage", cs40l50_vddb_uv),
+	CS40L50_IRQ(CS40L50_GLOBAL_ERROR_IRQ, "Global error", cs40l50_global_error),
+};
+
+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 const struct regmap_irq_chip cs40l50_regmap_irq_chip = {
+	.name = "cs40l50 IRQ1 Controller",
+	.status_base = CS40L50_IRQ1_INT_1,
+	.mask_base = CS40L50_IRQ1_MASK_1,
+	.ack_base = CS40L50_IRQ1_INT_1,
+	.num_regs = 19,
+	.irqs = cs40l50_reg_irqs,
+	.num_irqs = ARRAY_SIZE(cs40l50_reg_irqs),
+	.runtime_pm = true,
+};
+
+static const struct reg_sequence cs40l50_ext_bst_seq[] = {
+	{ CS40L50_BLOCK_ENABLES, CS40L50_MON_ENABLE },
+	{ CS40L50_MON_VALUE_CTRL, CS40L50_VBST_VDD_AMP },
+	{ CS40L50_TEST_KEY, CS40L50_KEY_SET_1 },
+	{ CS40L50_TEST_KEY, CS40L50_KEY_SET_2 },
+	{ CS40L50_TEST_VIS_SPARE, CS40L50_TEMP_SENSOR_3 },
+	{ CS40L50_MON_ERROR_OVERIDE, CS40L50_TEMP_ERR_THLD },
+	{ CS40L50_STATUS, CS40L50_TEMP_WARN_THLD },
+	{ CS40L50_TEST_CONFIG, CS40L50_TEMP_FILT_DEBOUNCE_OFF },
+	{ CS40L50_TEMP_WARN_CONFIG, CS40L50_TEMP_WARN_ATK_VOL },
+};
+
+static int cs40l50_config_bst(struct cs40l50_private *cs40l50)
+{
+	int error, i;
+
+	if (device_property_present(cs40l50->dev, "cirrus,external-boost")) {
+		error = regmap_multi_reg_write(cs40l50->regmap, cs40l50_ext_bst_seq,
+				ARRAY_SIZE(cs40l50_ext_bst_seq));
+		if (error)
+			return error;
+
+		for (i = 0; i < ARRAY_SIZE(cs40l50_ext_bst_seq); i++) {
+			error = cs40l50_pseq_write(cs40l50, cs40l50_ext_bst_seq[i].reg,
+					cs40l50_ext_bst_seq[i].def);
+			if (error)
+				return error;
+		}
+
+		return 0;
+	}
+
+	/* Internal boost config */
+	return cs40l50_dsp_write(cs40l50, CS40L50_BST_LPMODE_SEL, CS40L50_DCM_LOW_POWER);
+}
+
+static int cs40l50_patch_firmware(struct cs40l50_private *cs40l50)
+{
+	const struct firmware *bin = NULL, *wmfw = NULL;
+	int error = 0;
+
+	if (request_firmware(&bin, "cs40l50.bin", cs40l50->dev))
+		dev_info(cs40l50->dev, "Could not request wavetable file: %d\n", error);
+
+	if (request_firmware(&wmfw, "cs40l50.wmfw", cs40l50->dev))
+		dev_info(cs40l50->dev, "Could not request firmware patch file: %d\n", error);
+
+	if (wmfw) {
+		error = cs40l50_dsp_write(cs40l50, CS40L50_CCM_CORE_CONTROL,
+				CS40L50_DSP1_CLOCK_DISABLE);
+		if (error)
+			goto err_fw;
+
+		error = cs40l50_dsp_write(cs40l50, CS40L50_RAM_INIT,
+				CS40L50_DSP1_RAM_INIT_FLAG);
+		if (error)
+			goto err_fw;
+
+		error = cs40l50_dsp_write(cs40l50, CS40L50_PWRMGT_CTL,
+				CS40L50_DSP1_MEM_RDY_HW);
+		if (error)
+			goto err_fw;
+	}
+
+	mutex_lock(&cs40l50->lock);
+
+	cs_dsp_power_up(&cs40l50->dsp, wmfw, "cs40l50.wmfw", bin, "cs40l50.bin", "cs40l50");
+
+	mutex_unlock(&cs40l50->lock);
+
+	if (wmfw)
+		error = cs40l50_dsp_write(cs40l50, CS40L50_CCM_CORE_CONTROL,
+				CS40L50_DSP1_CLOCK_ENABLE);
+
+err_fw:
+	release_firmware(wmfw);
+	release_firmware(bin);
+
+	return error;
+}
+
+static int cs40l50_pseq_init(struct cs40l50_private *cs40l50)
+{
+	struct cs40l50_pseq_op *pseq_op;
+	int error, i, num_words;
+	u8 operation;
+	u32 *words;
+
+	INIT_LIST_HEAD(&cs40l50->pseq_op_head);
+
+	words = kcalloc(CS40L50_PSEQ_MAX_WORDS, sizeof(u32), GFP_KERNEL);
+	if (IS_ERR_OR_NULL(words))
+		return -ENOMEM;
+
+	error = regmap_bulk_read(cs40l50->regmap, CS40L50_DSP1_POWER_ON_SEQ, words,
+			CS40L50_PSEQ_MAX_WORDS);
+	if (error)
+		goto err_free;
+
+	for (i = 0; i < CS40L50_PSEQ_MAX_WORDS; i++)
+		words[i] = be32_to_cpu(words[i]);
+
+	for (i = 0; i < CS40L50_PSEQ_MAX_WORDS; i += num_words) {
+		operation = (words[i] & CS40L50_PSEQ_OP_MASK) >> CS40L50_PSEQ_OP_SHIFT;
+
+		switch (operation) {
+		case CS40L50_PSEQ_OP_END:
+			num_words = CS40L50_PSEQ_OP_END_WORDS;
+			break;
+		case CS40L50_PSEQ_OP_WRITE_ADDR8:
+		case CS40L50_PSEQ_OP_WRITE_H16:
+		case CS40L50_PSEQ_OP_WRITE_L16:
+			num_words = CS40L50_PSEQ_OP_WRITE_X16_WORDS;
+			break;
+		case CS40L50_PSEQ_OP_WRITE_FULL:
+			num_words = CS40L50_PSEQ_OP_WRITE_FULL_WORDS;
+			break;
+		default:
+			dev_err(cs40l50->dev, "Invalid OP code 0x%02X\n", operation);
+			error = -EINVAL;
+			goto err_free;
+		}
+
+		pseq_op = devm_kzalloc(cs40l50->dev, sizeof(*pseq_op), GFP_KERNEL);
+		if (!pseq_op) {
+			error = -ENOMEM;
+			goto err_free;
+		}
+
+		memcpy(pseq_op->words, &words[i], num_words * sizeof(u32));
+		pseq_op->size = num_words;
+		pseq_op->offset = i * sizeof(u32);
+		pseq_op->operation = operation;
+		list_add(&pseq_op->list, &cs40l50->pseq_op_head);
+
+		if (operation == CS40L50_PSEQ_OP_END)
+			break;
+	}
+
+	if (operation != CS40L50_PSEQ_OP_END) {
+		dev_err(cs40l50->dev, "PSEQ_END_OF_SCRIPT not found\n");
+		error = -ENOENT;
+	}
+
+err_free:
+	kfree(words);
+
+	return error;
+}
+
+static int cs40l50_input_init(struct cs40l50_private *cs40l50)
+{
+	int error;
+	u32 val;
+
+	error = cs40l50_dsp_read(cs40l50, CS40L50_DEVID, &val);
+	if (error)
+		return error;
+
+	if (val != CS40L50_DEVID_A) {
+		dev_err(cs40l50->dev, "Invalid device ID: %#010X\n", val);
+		return -EINVAL;
+	}
+
+	cs40l50->input->id.product = val & 0xFFFF;
+
+	error = cs40l50_dsp_read(cs40l50, CS40L50_REVID, &val);
+	if (error)
+		return error;
+
+	if (val != CS40L50_REVID_B0) {
+		dev_err(cs40l50->dev, "Invalid device revision: %#04X\n", val);
+		return -EINVAL;
+	}
+
+	cs40l50->input->id.version = val;
+
+	cs40l50->input = devm_input_allocate_device(cs40l50->dev);
+	if (IS_ERR_OR_NULL(cs40l50->input))
+		return cs40l50->input ? PTR_ERR(cs40l50->input) : -ENOMEM;
+
+	cs40l50->input->name = "cs40l50_input";
+	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;
+
+	clear_bit(FF_RUMBLE, cs40l50->input->ffbit);
+
+	cs40l50->input->ff->upload = cs40l50_add_effect;
+	cs40l50->input->ff->playback = cs40l50_playback_effect;
+	cs40l50->input->ff->erase = cs40l50_erase_effect;
+
+	INIT_LIST_HEAD(&cs40l50->effect_head);
+
+	return input_register_device(cs40l50->input);
+}
+
+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_DSP1_CORE_BASE;
+	cs40l50->dsp.base_sysinfo = CS40L50_DSP1_SYS_INFO_ID;
+	cs40l50->dsp.mem = cs40l50_dsp_regions;
+	cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
+	cs40l50->dsp.lock_regions = 0xFFFFFFFF;
+	cs40l50->dsp.no_core_startstop = true;
+	cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
+
+	return cs_dsp_halo_init(&cs40l50->dsp);
+}
+
+int cs40l50_probe(struct cs40l50_private *cs40l50)
+{
+	int error, i, irq;
+	u32 val;
+
+	mutex_init(&cs40l50->lock);
+
+	error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
+			cs40l50_supplies);
+	if (error)
+		return dev_err_probe(cs40l50->dev, error, "Failed to request supplies\n");
+
+	error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
+	if (error)
+		return dev_err_probe(cs40l50->dev, error, "Failed to enable supplies\n");
+
+	cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(cs40l50->reset_gpio)) {
+		error = PTR_ERR(cs40l50->reset_gpio);
+		goto err;
+	}
+
+	usleep_range(CS40L50_MIN_RESET_PULSE_WIDTH_US, CS40L50_MIN_RESET_PULSE_WIDTH_US + 100);
+
+	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
+
+	usleep_range(CS40L50_CP_READY_DELAY_US, CS40L50_CP_READY_DELAY_US + 1000);
+
+	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
+		error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_HALO_STATE, &val);
+		if (!error && val < 0xFFFF && val >= CS40L50_HALO_STATE_BOOT_DONE)
+			break;
+
+		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
+	}
+
+	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
+		dev_err(cs40l50->dev, "Firmware boot failed: %d, halo state = %#x\n", error, val);
+		goto err;
+	}
+
+	cs40l50->vibe_workqueue = alloc_ordered_workqueue("cs40l50_workqueue", WQ_HIGHPRI);
+	if (!cs40l50->vibe_workqueue) {
+		error = -ENOMEM;
+		goto err;
+	}
+
+	INIT_WORK(&cs40l50->vibe_start_work, cs40l50_vibe_start_worker);
+	INIT_WORK(&cs40l50->vibe_stop_work, cs40l50_vibe_stop_worker);
+
+	error = cs40l50_cs_dsp_init(cs40l50);
+	if (error)
+		goto err;
+
+	error = cs40l50_input_init(cs40l50);
+	if (error)
+		goto err;
+
+	error = cs40l50_patch_firmware(cs40l50);
+	if (error)
+		goto err;
+
+	error = cs40l50_pseq_init(cs40l50);
+	if (error)
+		goto err;
+
+	error = cs40l50_config_bst(cs40l50);
+	if (error)
+		goto err;
+
+	error = devm_regmap_add_irq_chip(cs40l50->dev, cs40l50->regmap, cs40l50->irq,
+			IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW, 0, &cs40l50_regmap_irq_chip,
+			&cs40l50->irq_data);
+	if (error) {
+		dev_err(cs40l50->dev, "Failed to register IRQ chip: %d\n", error);
+		goto err;
+	}
+
+	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(cs40l50->dev, "Failed to get %s\n", cs40l50_irqs[i].name);
+			error = irq;
+			goto err;
+		}
+
+		error = devm_request_threaded_irq(cs40l50->dev, irq, NULL, cs40l50_irqs[i].handler,
+				IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW,
+						cs40l50_irqs[i].name, cs40l50);
+		if (error) {
+			dev_err(cs40l50->dev, "Failed to request IRQ %s: %d\n",
+					cs40l50_irqs[i].name, error);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	cs40l50_remove(cs40l50);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(cs40l50_probe);
+
+int cs40l50_remove(struct cs40l50_private *cs40l50)
+{
+	disable_irq(cs40l50->irq);
+
+	if (cs40l50->dsp.booted)
+		cs_dsp_power_down(&cs40l50->dsp);
+	if (&cs40l50->dsp)
+		cs_dsp_remove(&cs40l50->dsp);
+
+	if (cs40l50->vibe_workqueue) {
+		flush_workqueue(cs40l50->vibe_workqueue);
+		destroy_workqueue(cs40l50->vibe_workqueue);
+	}
+
+	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs40l50_remove);
+
+MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
+MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/input/cs40l50.h b/include/linux/input/cs40l50.h
new file mode 100644
index 000000000000..f2c8a101fa00
--- /dev/null
+++ b/include/linux/input/cs40l50.h
@@ -0,0 +1,321 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
+ * and Closed-Loop Algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ */
+#ifndef __CS40L50_H__
+#define __CS40L50_H__
+
+#include <linux/bitops.h>
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/regmap.h>
+#include <linux/interrupt.h>
+#include <linux/firmware/cirrus/cs_dsp.h>
+#include <linux/firmware/cirrus/wmfw.h>
+
+/* Boost */
+#define CS40L50_TEST_KEY			0x40
+#define CS40L50_BLOCK_ENABLES			0x2018
+#define CS40L50_ERROR_RELEASE			0x2034
+#define CS40L50_PWRMGT_CTL			0x2900
+#define CS40L50_BST_LPMODE_SEL			0x3810
+#define CS40L50_STATUS				0x4200
+#define CS40L50_MON_ERROR_OVERIDE		0x4220
+#define CS40L50_TEST_CONFIG			0x4240
+#define CS40L50_MON_VALUE_CTRL			0x4404
+#define CS40L50_TEST_VIS_SPARE			0x5C00
+#define CS40L50_TEMP_WARN_CONFIG		0x6024
+#define CS40L50_KEY_SET_1		0x55
+#define CS40L50_KEY_SET_2		0xAA
+#define CS40L50_TEMP_SENSOR_3		0x400
+#define CS40L50_DCM_LOW_POWER		0x1
+#define CS40L50_MON_ENABLE		0x3201
+#define CS40L50_VBST_VDD_AMP		0x1000000
+#define CS40L50_TEMP_ERR_THLD		0x8000007D
+#define CS40L50_TEMP_WARN_THLD		0x8
+#define CS40L50_TEMP_FILT_DEBOUNCE_OFF	0x510002B5
+#define CS40L50_TEMP_WARN_ATK_VOL	0x522303
+
+/* Interrupts */
+#define CS40L50_IRQ1_INT_1			0xE010
+#define CS40L50_IRQ1_INT_2			0xE014
+#define CS40L50_IRQ1_INT_3			0xE018
+#define CS40L50_IRQ1_INT_4			0xE01C
+#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_INT_19			0xE058
+#define CS40L50_IRQ1_MASK_1			0xE090
+#define CS40L50_IRQ1_MASK_2			0xE094
+#define CS40L50_IRQ1_MASK_3			0xE098
+#define CS40L50_IRQ1_MASK_8			0xE0AC
+#define CS40L50_IRQ1_MASK_9			0xE0B0
+#define CS40L50_IRQ1_MASK_10			0xE0B4
+#define CS40L50_IRQ1_MASK_18			0xE0D4
+#define CS40L50_IRQ1_MASK_19			0xE0D8
+#define CS40L50_IRQ1_STS_1			0xE210
+#define CS40L50_IRQ1_STS_2			0xE214
+#define CS40L50_IRQ1_STS_3			0xE218
+#define CS40L50_IRQ1_STS_8			0xE22C
+#define CS40L50_IRQ1_STS_9			0xE230
+#define CS40L50_IRQ1_STS_10			0xE234
+#define CS40L50_IRQ1_STS_18			0xE254
+#define CS40L50_IRQ1_STS_19			0xE258
+#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_ERROR_RELEASE_MASK	BIT(11)
+#define CS40L50_BOOT_DONE_MASK		BIT(1)
+#define CS40L50_IRQ(_irq, _name, _hand)	\
+	{				\
+		.irq = _irq,		\
+		.name = _name,		\
+		.handler = _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_RAM_BANK_INDEX_END		0x100007F
+#define CS40L50_OWT_INDEX_START			0x1400000
+#define CS40L50_OWT_INDEX_END			0x140007F
+#define CS40L50_ROM_BANK_INDEX_START		0x1800000
+#define CS40L50_ROM_BANK_INDEX_END		0x180001A
+#define CS40L50_OTP_BUZZ			0x1800080
+#define CS40L50_RAM_BUZZ_INDEX_START		0x1800081
+#define CS40L50_RAM_BUZZ_INDEX_END		0x1800085
+#define CS40L50_MBOX_CMD_HIBERNATE		0x2000001
+#define CS40L50_MBOX_CMD_WAKEUP			0x2000002
+#define CS40L50_MBOX_CMD_PREVENT_HIBERNATE	0x2000003
+#define CS40L50_MBOX_CMD_ALLOW_HIBERNATION	0x2000004
+#define CS40L50_MBOX_CMD_SHUTDOWN		0x2000005
+#define CS40L50_MBOX_CMD_DURATION_REPORT	0x3000001
+#define CS40L50_MBOX_CMD_START_I2S		0x3000002
+#define CS40L50_MBOX_CMD_STOP_I2S		0x3000003
+#define CS40L50_MBOX_CMD_HAPTICS_LOGGER_RESET	0x3000004
+#define CS40L50_MBOX_CMD_OWT_PUSH		0x3000008
+#define CS40L50_MBOX_CMD_OWT_RESET		0x3000009
+#define CS40L50_MBOX_CMD_DVL_REINIT		0x300000A
+#define CS40L50_MBOX_CMD_STOP_PLAYBACK		0x5000000
+#define CS40L50_MBOX_CMD_F0_EST			0x7000001
+#define CS40L50_MBOX_CMD_REDC_EST		0x7000002
+#define CS40L50_MBOX_CMD_LE_EST			0x7000004
+#define CS40L50_MBOX_CMD_OWT_DELETE		0xD000000
+
+/* Mailbox Outbound Commands */
+#define CS40L50_MBOX_QUEUE_BASE				0x11004
+#define CS40L50_MBOX_QUEUE_END				0x1101C
+#define CS40L50_DSP_VIRTUAL1_MBOX_1			0x11020
+#define CS40L50_DSP1_MBOX_QUEUE_WT			0x28042C8
+#define CS40L50_DSP1_MBOX_QUEUE_RD			0x28042CC
+#define CS40L50_DSP_MBOX_HAPTIC_COMPLETE_MBOX	0x1000000
+#define CS40L50_DSP_MBOX_HAPTIC_COMPLETE_GPIO	0x1000001
+#define CS40L50_DSP_MBOX_HAPTIC_COMPLETE_I2S	0x1000002
+#define CS40L50_DSP_MBOX_HAPTIC_TRIGGER_MBOX	0x1000010
+#define CS40L50_DSP_MBOX_HAPTIC_TRIGGER_GPIO	0x1000011
+#define CS40L50_DSP_MBOX_HAPTIC_TRIGGER_I2S	0x1000012
+#define CS40L50_DSP_MBOX_INIT			0x2000000
+#define CS40L50_DSP_MBOX_AWAKE			0x2000002
+#define CS40L50_DSP_MBOX_F0_EST_START		0x7000011
+#define CS40L50_DSP_MBOX_F0_EST_DONE		0x7000021
+#define CS40L50_DSP_MBOX_REDC_EST_START		0x7000012
+#define CS40L50_DSP_MBOX_REDC_EST_DONE		0x7000022
+#define CS40L50_DSP_MBOX_LE_EST_START		0x7000014
+#define CS40L50_DSP_MBOX_LE_EST_DONE		0x7000024
+#define CS40L50_DSP_MBOX_ACK			0xA000000
+#define CS40L50_DSP_MBOX_PANIC			0xC000000
+#define CS40L50_DSP_MBOX_WATERMARK		0xD000000
+#define CS40L50_DSP_MBOX_ERR_EVENT_UNMAPPED	0xC0004B3
+#define CS40L50_DSP_MBOX_ERR_EVENT_MODIFY	0xC0004B4
+#define CS40L50_DSP_MBOX_ERR_NULLPTR		0xC0006A5
+#define CS40L50_DSP_MBOX_ERR_BRAKING		0xC0006A8
+#define CS40L50_DSP_MBOX_ERR_INVAL_SRC		0xC0006AC
+#define CS40L50_DSP_MBOX_ERR_ENABLE_RANGE	0xC000836
+#define CS40L50_DSP_MBOX_ERR_GPIO_UNMAPPED	0xC000837
+#define CS40L50_DSP_MBOX_ERR_ISR_RANGE		0xC000838
+#define CS40L50_DSP_MBOX_PERMANENT_SHORT	0xC000C1C
+#define CS40L50_DSP_MBOX_RUNTIME_SHORT		0xC000C1D
+
+/* DSP */
+#define CS40L50_DSP1_XMEM_PACKED_0		0x2000000
+#define CS40L50_DSP1_XMEM_UNPACKED32_0		0x2400000
+#define CS40L50_DSP1_SYS_INFO_ID		0x25E0000
+#define CS40L50_DSP1_XMEM_UNPACKED24_0		0x2800000
+#define CS40L50_NUM_OF_WAVES			0x280CB4C
+#define CS40L50_DSP1_CORE_BASE			0x2B80000
+#define CS40L50_DSP1_SCRATCH1			0x2B805C0
+#define CS40L50_DSP1_SCRATCH2			0x2B805C8
+#define CS40L50_DSP1_SCRATCH3			0x2B805D0
+#define CS40L50_DSP1_SCRATCH4			0x2B805D8
+#define CS40L50_CCM_CORE_CONTROL		0x2BC1000
+#define CS40L50_RAM_INIT			0x28021DC
+#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_DSP1_MEM_RDY_HW		0x2
+#define CS40L50_DSP1_RAM_INIT_FLAG	0x1
+#define CS40L50_DSP1_CLOCK_DISABLE	0x80
+#define CS40L50_DSP1_CLOCK_ENABLE	0x281
+#define CS40L50_DSP_POLL_US		1000
+#define CS40L50_DSP_TIMEOUT_COUNT	100
+#define CS40L50_MBOX_POLL_US		5000
+
+/* Calibration */
+#define CS40L50_DSP1_REDC_ESTIMATION		0x2802F7C
+#define CS40L50_DSP1_F0_ESTIMATION		0x2802F84
+#define CS40L50_DSP1_DYNAMIC_F0_ENABLE		0x2802F8C
+#define CS40L50_DSP1_DYNAMIC_F0_THRESHOLD	0x2802F90
+#define CS40L50_DSP1_F0_STORED			0x2805C00
+#define CS40L50_DSP1_REDC_STORED		0x2805C04
+#define CS40L50_DSP1_RE_EST_STATUS		0x3401B40
+#define CS40L50_REDC_EST_DELAY_US	30000
+#define CS40L50_F0_EST_DELAY_US		10000
+
+/* OWT */
+#define CS40L50_DSP1_OWT_SIZE			0x2805C38
+#define CS40L50_DSP1_OWT_NEXT			0x2805C3C
+#define CS40L50_NUM_OF_OWT_WAVES		0x2805C40
+#define CS40L50_DSP1_OWT_BASE			0x2805C34
+#define CS40L50_MAX_PWLE_SECTIONS	126
+#define CS40L50_CUSTOM_DATA_SIZE	2
+#define CS40L50_WT_HEADER_PWLE_SIZE	12
+#define CS40L50_WT_HEADER_DEFAULT_FLAGS	0x0000
+#define CS40L50_WT_HEADER_OFFSET	3
+#define CS40L50_WT_TYPE_PCM		8
+#define CS40L50_WT_TYPE_PWLE		12
+#define CS40L50_PCM_ID			0x0000
+
+/* GPIO */
+#define CS40L50_DSP1_GPIO_BASE			0x2804140
+#define CS40L50_GPIO_MAPPING_INVALID	0
+#define CS40L50_GPI_DISABLE		0x1FF
+#define CS40L50_BTN_NUM_MASK		GENMASK(14, 12)
+#define CS40L50_BTN_EDGE_MASK		BIT(15)
+
+/* State */
+#define CS40L50_DSP1_HALO_STATE				0x28021E0
+#define CS40L50_HALO_STATE_BOOT_DONE		2
+#define CS40L50_MIN_RESET_PULSE_WIDTH_US	1100
+#define CS40L50_CP_READY_DELAY_US		2200
+
+/* Device */
+#define CS40L50_DEVID			0x0
+#define CS40L50_REVID			0x4
+#define CS40L50_DEVID_A		0x40A50
+#define CS40L50_REVID_B0	0xB0
+
+/* Write sequencer */
+#define CS40L50_DSP1_POWER_ON_SEQ				0x280C320
+#define CS40L50_PSEQ_MAX_WORDS				200
+#define CS40L50_PSEQ_OP_MASK				GENMASK(23, 16)
+#define CS40L50_PSEQ_OP_SHIFT				16
+#define CS40L50_PSEQ_OP_WRITE_ADDR8			0x02
+#define CS40L50_PSEQ_OP_WRITE_L16			0x04
+#define CS40L50_PSEQ_OP_WRITE_H16			0x05
+#define CS40L50_PSEQ_OP_WRITE_FULL			0x00
+#define CS40L50_PSEQ_OP_WRITE_FULL_WORDS		3
+#define CS40L50_PSEQ_OP_WRITE_X16_WORDS			2
+#define CS40L50_PSEQ_OP_END				0xFF
+#define CS40L50_PSEQ_OP_END_WORDS			1
+#define CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_SHIFT	8
+#define CS40L50_PSEQ_WRITE_FULL_UPPER_ADDR_SHIFT	16
+#define CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_MASK		GENMASK(15, 0)
+#define CS40L50_PSEQ_WRITE_FULL_UPPER_DATA_SHIFT	24
+#define CS40L50_PSEQ_WRITE_FULL_LOWER_DATA_MASK		GENMASK(23, 0)
+
+enum cs40l50_wvfrm_bank {
+	CS40L50_WVFRM_BANK_RAM,
+	CS40L50_WVFRM_BANK_ROM,
+	CS40L50_WVFRM_BANK_OWT,
+	CS40L50_WVFRM_BANK_BUZ,
+	CS40L50_WVFRM_BANK_NUM,
+};
+
+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,
+	CS40L50_NUM_IRQ
+};
+
+union cs40l50_buzzgen {
+	u32 words[3];
+	struct {
+		u32 freq;
+		u32 level;
+		u32 duration;
+	} params;
+};
+
+struct cs40l50_owt_header {
+	u32 wvfrm_type;
+	u32 offset;
+	u32 data_size;
+};
+
+struct cs40l50_irq {
+	int irq;
+	const char *name;
+	irqreturn_t (*handler)(int irq, void *data);
+};
+
+struct cs40l50_effect {
+	int id;
+	u32 trigger_index;
+	enum cs40l50_wvfrm_bank wvfrm_bank;
+	u32 mapping;
+	struct list_head list;
+};
+
+struct cs40l50_pseq_op {
+	u16 offset;
+	u8 operation;
+	u32 words[3];
+	u32 size;
+	struct list_head list;
+};
+
+struct cs40l50_private {
+	struct device *dev;
+	struct regmap *regmap;
+	struct cs_dsp dsp;
+	struct mutex lock;
+	struct gpio_desc *reset_gpio;
+	struct input_dev *input;
+	struct list_head effect_head;
+	struct ff_effect *trigger_effect;
+	struct workqueue_struct *vibe_workqueue;
+	struct work_struct vibe_start_work;
+	struct work_struct vibe_stop_work;
+	struct regmap_irq_chip_data *irq_data;
+	struct list_head pseq_op_head;
+	int irq;
+};
+
+int cs40l50_probe(struct cs40l50_private *cs40l50);
+int cs40l50_remove(struct cs40l50_private *cs40l50);
+
+extern const struct regmap_config cs40l50_regmap;
+
+#endif /* __CS40L50_H__ */
-- 
2.25.1

Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[Krzysztof Kozlowski]

On 09/08/2023 21:10, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptics driver with waveform memory DSP and closed-loop
> algorithms.
> 
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>

...

> +
> +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_DSP1_CORE_BASE;
> +	cs40l50->dsp.base_sysinfo = CS40L50_DSP1_SYS_INFO_ID;
> +	cs40l50->dsp.mem = cs40l50_dsp_regions;
> +	cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
> +	cs40l50->dsp.lock_regions = 0xFFFFFFFF;
> +	cs40l50->dsp.no_core_startstop = true;
> +	cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
> +
> +	return cs_dsp_halo_init(&cs40l50->dsp);
> +}
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50)
> +{
> +	int error, i, irq;
> +	u32 val;
> +
> +	mutex_init(&cs40l50->lock);
> +
> +	error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
> +			cs40l50_supplies);
> +	if (error)
> +		return dev_err_probe(cs40l50->dev, error, "Failed to request supplies\n");
> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +	if (error)
> +		return dev_err_probe(cs40l50->dev, error, "Failed to enable supplies\n");
> +
> +	cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);

None of the lines above or below seem to be wrapped according to Linux
coding style (80).

> +	if (IS_ERR(cs40l50->reset_gpio)) {
> +		error = PTR_ERR(cs40l50->reset_gpio);
> +		goto err;

Why do you disable IRQ now?

I asked to test this. Your entire cleanup path is:
1. not tested.
2. buggy.
3. done differently than Linux style. Use proper cleanup calls and
multiple goto labels.

> +	}
> +
> +	usleep_range(CS40L50_MIN_RESET_PULSE_WIDTH_US, CS40L50_MIN_RESET_PULSE_WIDTH_US + 100);
> +
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
> +
> +	usleep_range(CS40L50_CP_READY_DELAY_US, CS40L50_CP_READY_DELAY_US + 1000);
> +
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_HALO_STATE, &val);
> +		if (!error && val < 0xFFFF && val >= CS40L50_HALO_STATE_BOOT_DONE)
> +			break;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> +		dev_err(cs40l50->dev, "Firmware boot failed: %d, halo state = %#x\n", error, val);
> +		goto err;
> +	}
> +
> +	cs40l50->vibe_workqueue = alloc_ordered_workqueue("cs40l50_workqueue", WQ_HIGHPRI);
> +	if (!cs40l50->vibe_workqueue) {
> +		error = -ENOMEM;
> +		goto err;
> +	}
> +
> +	INIT_WORK(&cs40l50->vibe_start_work, cs40l50_vibe_start_worker);
> +	INIT_WORK(&cs40l50->vibe_stop_work, cs40l50_vibe_stop_worker);
> +
> +	error = cs40l50_cs_dsp_init(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_input_init(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_patch_firmware(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_pseq_init(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_config_bst(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = devm_regmap_add_irq_chip(cs40l50->dev, cs40l50->regmap, cs40l50->irq,
> +			IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW, 0, &cs40l50_regmap_irq_chip,
> +			&cs40l50->irq_data);
> +	if (error) {
> +		dev_err(cs40l50->dev, "Failed to register IRQ chip: %d\n", error);
> +		goto err;
> +	}
> +
> +	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(cs40l50->dev, "Failed to get %s\n", cs40l50_irqs[i].name);
> +			error = irq;
> +			goto err;
> +		}
> +
> +		error = devm_request_threaded_irq(cs40l50->dev, irq, NULL, cs40l50_irqs[i].handler,
> +				IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW,
> +						cs40l50_irqs[i].name, cs40l50);
> +		if (error) {
> +			dev_err(cs40l50->dev, "Failed to request IRQ %s: %d\n",
> +					cs40l50_irqs[i].name, error);
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	cs40l50_remove(cs40l50);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_probe);
> +
> +int cs40l50_remove(struct cs40l50_private *cs40l50)
> +{
> +	disable_irq(cs40l50->irq);
> +
> +	if (cs40l50->dsp.booted)
> +		cs_dsp_power_down(&cs40l50->dsp);
> +	if (&cs40l50->dsp)
> +		cs_dsp_remove(&cs40l50->dsp);
> +
> +	if (cs40l50->vibe_workqueue) {
> +		flush_workqueue(cs40l50->vibe_workqueue);
> +		destroy_workqueue(cs40l50->vibe_workqueue);
> +	}
> +
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
> +	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_remove);
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");

Nothing improved here. Don't send new version immediately after
responding to my comment, but allow discussion.

Best regards,
Krzysztof

Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[James Ogletree]

> On Aug 10, 2023, at 1:17 AM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> On 09/08/2023 21:10, James Ogletree wrote:
> 
>> +
>> +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_DSP1_CORE_BASE;
>> + cs40l50->dsp.base_sysinfo = CS40L50_DSP1_SYS_INFO_ID;
>> + cs40l50->dsp.mem = cs40l50_dsp_regions;
>> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
>> + cs40l50->dsp.lock_regions = 0xFFFFFFFF;
>> + cs40l50->dsp.no_core_startstop = true;
>> + cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
>> +
>> + return cs_dsp_halo_init(&cs40l50->dsp);
>> +}
>> +
>> +int cs40l50_probe(struct cs40l50_private *cs40l50)
>> +{
>> + int error, i, irq;
>> + u32 val;
>> +
>> + mutex_init(&cs40l50->lock);
>> +
>> + error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
>> + cs40l50_supplies);
>> + if (error)
>> + return dev_err_probe(cs40l50->dev, error, "Failed to request supplies\n");
>> +
>> + error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
>> + if (error)
>> + return dev_err_probe(cs40l50->dev, error, "Failed to enable supplies\n");
>> +
>> + cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);
> 
> None of the lines above or below seem to be wrapped according to Linux
> coding style (80).

This patch abides by the 100-column line length limit which checkpatch.pl enforces.
However, I can see how some of the lines might be less jarring to the eyes if wrapped.
That will be addressed in V4.

> 
>> + if (IS_ERR(cs40l50->reset_gpio)) {
>> + error = PTR_ERR(cs40l50->reset_gpio);
>> + goto err;
> 
> Why do you disable IRQ now?
> 
> I asked to test this. Your entire cleanup path is:
> 1. not tested.
> 2. buggy.
> 3. done differently than Linux style. Use proper cleanup calls and
> multiple goto labels.

Disabling IRQ there is a mistake. The probe error path will be tightened up and made
to utilize multiple goto labels instead in V4.

Regards,
James Ogletree

Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[Krzysztof Kozlowski]

On 15/08/2023 17:56, James Ogletree wrote:
> 
> 
>> On Aug 10, 2023, at 1:17 AM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 09/08/2023 21:10, James Ogletree wrote:
>>
>>> +
>>> +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_DSP1_CORE_BASE;
>>> + cs40l50->dsp.base_sysinfo = CS40L50_DSP1_SYS_INFO_ID;
>>> + cs40l50->dsp.mem = cs40l50_dsp_regions;
>>> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
>>> + cs40l50->dsp.lock_regions = 0xFFFFFFFF;
>>> + cs40l50->dsp.no_core_startstop = true;
>>> + cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
>>> +
>>> + return cs_dsp_halo_init(&cs40l50->dsp);
>>> +}
>>> +
>>> +int cs40l50_probe(struct cs40l50_private *cs40l50)
>>> +{
>>> + int error, i, irq;
>>> + u32 val;
>>> +
>>> + mutex_init(&cs40l50->lock);
>>> +
>>> + error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
>>> + cs40l50_supplies);
>>> + if (error)
>>> + return dev_err_probe(cs40l50->dev, error, "Failed to request supplies\n");
>>> +
>>> + error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
>>> + if (error)
>>> + return dev_err_probe(cs40l50->dev, error, "Failed to enable supplies\n");
>>> +
>>> + cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);
>>
>> None of the lines above or below seem to be wrapped according to Linux
>> coding style (80).
> 
> This patch abides by the 100-column line length limit which checkpatch.pl enforces.

Checkpatch does not enforce 100, because it accepts also 80. It
complains if something is longer than 100, but coding style is still the
same. Format everything according to coding style and use 100 only if it
increases readability.

> However, I can see how some of the lines might be less jarring to the eyes if wrapped.
> That will be addressed in V4.
> 

Best regards,
Krzysztof

Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[Charles Keepax]

On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptics driver with waveform memory DSP and closed-loop
> algorithms.
> 
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
>  MAINTAINERS                      |    2 +
>  drivers/input/misc/Kconfig       |   33 +
>  drivers/input/misc/Makefile      |    3 +
>  drivers/input/misc/cs40l50-i2c.c |   67 ++
>  drivers/input/misc/cs40l50-spi.c |   67 ++
>  drivers/input/misc/cs40l50.c     | 1008 ++++++++++++++++++++++++++++++
>  include/linux/input/cs40l50.h    |  321 ++++++++++

Is this part not going to support streaming at some point? Fine
if not, but it seems likely to me and as such we should probably
follow the l26 stuff and make this an MFD from the start, even if
we haven't implemented the audio bits.

> +
> +static int cs40l50_pseq_write(struct cs40l50_private *cs40l50, u32 addr, u32 data)
> +{
> +
> +static int cs40l50_owt_upload(struct cs40l50_private *cs40l50, s16 *in_data, u32 in_data_nibbles)
> +{

These pseq and OWT bits, could they be shared with l26?
Definitely worth syncing with those guys, my assumption is the
wavetable/pseq won't have changed much and it might be nice to
factor these bits out into some library code that both drivers
can use.

> +err_free:
> +	if (is_new)
> +		error ? kfree(new_effect) : list_add(&new_effect->list, &cs40l50->effect_head);

This is a bit of an excessive use for a ternary, just make it an
if.

> +static int cs40l50_erase_effect(struct input_dev *dev, int effect_id)
> +{
> +	int error = 0;
> +	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +	struct cs40l50_effect *effect, *effect_owt;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	effect = cs40l50_find_effect(cs40l50, dev->ff->effects[effect_id].id);
> +	if (!effect) {
> +		error = -EINVAL;
> +		goto err_mutex;
> +	}
> +
> +	if (effect->mapping != CS40L50_GPIO_MAPPING_INVALID) {
> +		error = cs40l50_dsp_write(cs40l50, effect->mapping, CS40L50_GPI_DISABLE);
> +		if (error)
> +			goto err_mutex;
> +	}
> +
> +	if (effect->wvfrm_bank == CS40L50_WVFRM_BANK_OWT) {
> +		error = cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1,
> +				CS40L50_MBOX_CMD_OWT_DELETE | effect->trigger_index);
> +		if (error)
> +			goto err_mutex;
> +
> +		list_for_each_entry(effect_owt, &cs40l50->effect_head, list) {
> +			if (effect_owt->wvfrm_bank == CS40L50_WVFRM_BANK_OWT &&
> +					effect_owt->trigger_index > effect->trigger_index)
> +				effect_owt->trigger_index--;
> +		}
> +	}
> +
> +	list_del(&effect->list);
> +	kfree(effect);
> +err_mutex:
> +	mutex_unlock(&cs40l50->lock);
> +
> +	return error;
> +}

I seem to remember add/erase needed to get pushed to the work
queue too, not because the framework might call it in atomic
context, but in order to ensure ordering with the start/stops.

> +int cs40l50_probe(struct cs40l50_private *cs40l50)
> +{
> +	int error, i, irq;
> +	u32 val;
> +
> +	mutex_init(&cs40l50->lock);
> +
> +	error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
> +			cs40l50_supplies);
> +	if (error)
> +		return dev_err_probe(cs40l50->dev, error, "Failed to request supplies\n");
> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +	if (error)
> +		return dev_err_probe(cs40l50->dev, error, "Failed to enable supplies\n");
> +
> +	cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(cs40l50->reset_gpio)) {
> +		error = PTR_ERR(cs40l50->reset_gpio);
> +		goto err;
> +	}
> +
> +	usleep_range(CS40L50_MIN_RESET_PULSE_WIDTH_US, CS40L50_MIN_RESET_PULSE_WIDTH_US + 100);
> +
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
> +
> +	usleep_range(CS40L50_CP_READY_DELAY_US, CS40L50_CP_READY_DELAY_US + 1000);
> +
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_HALO_STATE, &val);
> +		if (!error && val < 0xFFFF && val >= CS40L50_HALO_STATE_BOOT_DONE)
> +			break;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> +		dev_err(cs40l50->dev, "Firmware boot failed: %d, halo state = %#x\n", error, val);
> +		goto err;
> +	}
> +
> +	cs40l50->vibe_workqueue = alloc_ordered_workqueue("cs40l50_workqueue", WQ_HIGHPRI);
> +	if (!cs40l50->vibe_workqueue) {
> +		error = -ENOMEM;
> +		goto err;
> +	}
> +
> +	INIT_WORK(&cs40l50->vibe_start_work, cs40l50_vibe_start_worker);
> +	INIT_WORK(&cs40l50->vibe_stop_work, cs40l50_vibe_stop_worker);
> +
> +	error = cs40l50_cs_dsp_init(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_input_init(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_patch_firmware(cs40l50);
> +	if (error)
> +		goto err;
> +

Doing this from probe, with a sync firmware load, can be a
little problematic as the firmware is often not available if
the driver is builtin rather than a module.

Fairly quick review as I am on holiday at the moment, but will
check back next week.

Thanks,
Charles

Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[James Ogletree]

> On Aug 10, 2023, at 5:30 AM, Charles Keepax <ckeepax@opensource.cirrus.com> wrote:
> 
> On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
>> +
>> +static int cs40l50_pseq_write(struct cs40l50_private *cs40l50, u32 addr, u32 data)
>> +{
>> +
>> +static int cs40l50_owt_upload(struct cs40l50_private *cs40l50, s16 *in_data, u32 in_data_nibbles)
>> +{
> 
> These pseq and OWT bits, could they be shared with l26?
> Definitely worth syncing with those guys, my assumption is the
> wavetable/pseq won't have changed much and it might be nice to
> factor these bits out into some library code that both drivers
> can use.

The pseq code most certainly can, likely the OWT code, perhaps others. I assume it is
acceptable to move some of these functions to a library in this patch set, even though this is
the only driver utilizing them as far as mainline is concerned? In other words, we shouldn’t
wait for one of L26 or L50 drivers to be accepted before splitting off the common code as part
of the others’ patchset? I’m probably overcomplicating; just want to be sure on the process here.

Everything else in your review will be fixed in V4. Thank you.

Regards,
James Ogletree

Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[Charles Keepax]

On Tue, Aug 15, 2023 at 10:33:00PM +0000, James Ogletree wrote:
> 
> 
> > On Aug 10, 2023, at 5:30 AM, Charles Keepax <ckeepax@opensource.cirrus.com> wrote:
> > 
> > On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
> >> +
> >> +static int cs40l50_pseq_write(struct cs40l50_private *cs40l50, u32 addr, u32 data)
> >> +{
> >> +
> >> +static int cs40l50_owt_upload(struct cs40l50_private *cs40l50, s16 *in_data, u32 in_data_nibbles)
> >> +{
> > 
> > These pseq and OWT bits, could they be shared with l26?
> > Definitely worth syncing with those guys, my assumption is the
> > wavetable/pseq won't have changed much and it might be nice to
> > factor these bits out into some library code that both drivers
> > can use.
> 
> The pseq code most certainly can, likely the OWT code, perhaps others. I assume it is
> acceptable to move some of these functions to a library in this patch set, even though this is
> the only driver utilizing them as far as mainline is concerned? In other words, we shouldn’t
> wait for one of L26 or L50 drivers to be accepted before splitting off the common code as part
> of the others’ patchset? I’m probably overcomplicating; just want to be sure on the process here.
> 
> Everything else in your review will be fixed in V4. Thank you.
> 

I think this makes sense to do now, just need to make sure the
next series of L26 is synced up to it.

Thanks,
Charles

Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[Jeff LaBundy]

Hi James,

Great work as always; just some minor comments throughout.

On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptics driver with waveform memory DSP and closed-loop
> algorithms.

From my extremely naive point of view, some of the code that follows
bares resemblance to the recently reviewed L26. My assumption is that
these devices are different enough in nature to warrant completely
different drivers; is that accurate?

One reason for asking is because the L26 driver included a cornucopia
of power-management overhead, yet we see none of that here. Assuming
both L50 and L26 are built around the same Halo DSP, why is there such
a fundamental difference between the two in terms of power management?

To that end, how does this driver handle hibernation? Is hibernation
not supported, or do we simply defer to the DSP? In the case of the
latter, why is L50 given this freedom but not L26?

This is not a suggestion to change anything; it's just to help set the
context and provide more background information. However, I'm curious
whether you've considered consolidating both L26 and L50 into a single
driver, especially given recent discussion around promoting L26 into an
MFD with both input and codec children. Would L50 ever support audio-to
-haptics over I2S?

> 
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
>  MAINTAINERS                      |    2 +
>  drivers/input/misc/Kconfig       |   33 +
>  drivers/input/misc/Makefile      |    3 +
>  drivers/input/misc/cs40l50-i2c.c |   67 ++
>  drivers/input/misc/cs40l50-spi.c |   67 ++
>  drivers/input/misc/cs40l50.c     | 1008 ++++++++++++++++++++++++++++++
>  include/linux/input/cs40l50.h    |  321 ++++++++++
>  7 files changed, 1501 insertions(+)
>  create mode 100644 drivers/input/misc/cs40l50-i2c.c
>  create mode 100644 drivers/input/misc/cs40l50-spi.c
>  create mode 100644 drivers/input/misc/cs40l50.c
>  create mode 100644 include/linux/input/cs40l50.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2869dd2dbbcf..e82d03293383 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2088,6 +2088,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/cs40l*
> +F:	include/linux/input/cs40l*
>  
>  ARM/CLKDEV SUPPORT
>  M:	Russell King <linux@armlinux.org.uk>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index c47eecc6f83b..c1c1572ec39c 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -140,6 +140,39 @@ config INPUT_BMA150
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bma150.
>  
> +config INPUT_CS40L50
> +	tristate "Cirrus Logic CS40L50 Haptic Driver support"
> +	select CS_DSP
> +	select REGMAP_IRQ
> +	help
> +	  Say Y here to enable support for CS40L50 boosted haptic
> +	  driver.
> +
> +	  To compile the driver as a module choose M here: the module
> +	  will be called cs40l50.
> +
> +config INPUT_CS40L50_I2C
> +	tristate "Support I2C bus connection"
> +	depends on INPUT_CS40L50 && I2C
> +	select REGMAP_I2C
> +	help
> +	  Say Y here if you have Cirrus Logic's CS40L50 connected
> +	  to an I2C bus.
> +
> +	  To compile the driver as a module choose M here: the
> +	  module will be called cs40l50_i2c.
> +
> +config INPUT_CS40L50_SPI
> +	tristate "Support SPI bus connection"
> +	depends on INPUT_CS40L50 && SPI
> +	select REGMAP_SPI
> +	help
> +	  Say Y here if you have Cirrus Logic's CS40L50 connected
> +	  to a SPI bus.
> +
> +	  To compile the driver as a module choose M here: the
> +	  module will be called cs40l50_spi.
> +
>  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 04296a4abe8e..77f34a33c364 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -28,6 +28,9 @@ 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)		+= cs40l50.o
> +obj-$(CONFIG_INPUT_CS40L50_I2C)		+= cs40l50-i2c.o
> +obj-$(CONFIG_INPUT_CS40L50_SPI)		+= cs40l50-spi.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-i2c.c b/drivers/input/misc/cs40l50-i2c.c
> new file mode 100644
> index 000000000000..6ebe1b51990b
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50-i2c.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms

Nit: the capitalization and general sentence structure here is hard to
follow; maybe something like "... with DSP-based waveform memory and
closed-loop algorithms" would make more sense?

> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#include <linux/i2c.h>
> +#include <linux/input/cs40l50.h>
> +
> +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 int cs40l50_i2c_probe(struct i2c_client *client)
> +{
> +	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");

Nit: line breaks tend to be more readable when aligned against the
parenthesis as follows:

	if (...)
		return dev_err_probe(...,
				     ...);

At least, it's much more common.

> +
> +	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 struct i2c_driver cs40l50_i2c_driver = {
> +	.driver = {
> +		.name = "cs40l50",
> +		.of_match_table = cs40l50_of_match,
> +	},
> +	.id_table = cs40l50_id_i2c,
> +	.probe_new = cs40l50_i2c_probe,

Mainline has moved back to .probe now; .probe_new is becoming depracated.

> +	.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/input/misc/cs40l50-spi.c b/drivers/input/misc/cs40l50-spi.c
> new file mode 100644
> index 000000000000..5fcf3ba137f0
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50-spi.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#include <linux/spi/spi.h>
> +#include <linux/input/cs40l50.h>
> +
> +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 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 struct spi_driver cs40l50_spi_driver = {
> +	.driver = {
> +		.name = "cs40l50",
> +		.of_match_table = cs40l50_of_match,
> +	},
> +	.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/drivers/input/misc/cs40l50.c b/drivers/input/misc/cs40l50.c
> new file mode 100644
> index 000000000000..9bb2881683c0
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50.c
> @@ -0,0 +1,1008 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>

Why to #include i2c.h here? This driver should be agnostic to the transport.

> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/bitfield.h>
> +#include <linux/input/cs40l50.h>

Please sort these alphabetically. Are you sure you're using all of them?

> +
> +const struct regmap_config cs40l50_regmap = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.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" },
> +	{ .supply = "va" },
> +};
> +
> +static int cs40l50_dsp_write(struct cs40l50_private *cs40l50, u32 write_reg, u32 write_val)
> +{
> +	int error = 0, i;

No need to initialize 'error' here.

> +
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = regmap_write(cs40l50->regmap, write_reg, write_val);
> +		if (!error)
> +			break;

This seems more efficient:

		if (!error)
			return 0;

...then you can avoid the check for i == CS40L50_DSP_TIMEOUT_COUNT below.

> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> +		dev_err(cs40l50->dev, "Error writing %#X to %#X: %d", write_val, write_reg, error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cs40l50_dsp_read(struct cs40l50_private *cs40l50, u32 read_reg, u32 *read_val)
> +{
> +	int error = 0, i;

...and here.

> +
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = regmap_read(cs40l50->regmap, read_reg, read_val);
> +		if (!error)
> +			break;

...and here.

> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> +		dev_err(cs40l50->dev, "Error reading %#X: %d", read_reg, error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cs40l50_pseq_write(struct cs40l50_private *cs40l50, u32 addr, u32 data)
> +{
> +	u8 operation = 0;
> +	struct cs40l50_pseq_op *op_new, *op_end;
> +	int error;

Nit: it is kind of personal preference, but "reverse Christmas tree" tends
to be more common, i.e. move the struct definitions (longest) to the top.

> +
> +	op_new = devm_kzalloc(cs40l50->dev, sizeof(*op_new), GFP_KERNEL);
> +	if (!op_new)
> +		return -ENOMEM;
> +
> +	op_new->words[0] = (addr >> CS40L50_PSEQ_WRITE_FULL_UPPER_ADDR_SHIFT);
> +	op_new->words[1] = ((addr & CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_MASK) <<
> +			CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_SHIFT) |
> +			(data >> CS40L50_PSEQ_WRITE_FULL_UPPER_DATA_SHIFT);

Nit: consider aligning the line breaks past the equal sign for readability.

> +	op_new->words[2] = data & CS40L50_PSEQ_WRITE_FULL_LOWER_DATA_MASK;
> +
> +	list_for_each_entry(op_end, &cs40l50->pseq_op_head, list) {
> +		operation = op_end->operation;
> +		if (operation == CS40L50_PSEQ_OP_END)
> +			break;
> +	}
> +
> +	if (operation != CS40L50_PSEQ_OP_END) {
> +		error = -ENOENT;
> +		goto op_new_free;
> +	}
> +
> +	if ((CS40L50_PSEQ_MAX_WORDS - op_end->offset) < ARRAY_SIZE(op_new->words)) {
> +		error = -ENOSPC;
> +		goto op_new_free;
> +	}
> +
> +	op_new->offset = op_end->offset;
> +	op_end->offset += (ARRAY_SIZE(op_new->words) * sizeof(u32));
> +
> +	error = regmap_bulk_write(cs40l50->regmap, CS40L50_DSP1_POWER_ON_SEQ + op_new->offset,
> +			op_new->words, ARRAY_SIZE(op_new->words));

Nit: alignment

> +	if (error)
> +		goto op_new_free;
> +
> +	error = cs40l50_dsp_write(cs40l50, CS40L50_DSP1_POWER_ON_SEQ + op_end->offset,
> +			op_end->words[0]);

...and here.

> +	if (error)
> +		goto op_new_free;
> +
> +	list_add(&op_new->list, &cs40l50->pseq_op_head);
> +
> +	return 0;
> +
> +op_new_free:
> +	devm_kfree(cs40l50->dev, op_new);
> +
> +	return error;
> +}
> +
> +static struct cs40l50_effect *cs40l50_find_effect(struct cs40l50_private *cs40l50, int id)
> +{
> +	struct cs40l50_effect *effect;
> +
> +	if (!list_empty(&cs40l50->effect_head)) {
> +		list_for_each_entry(effect, &cs40l50->effect_head, list) {
> +			if (effect->id == id)
> +				return effect;
> +		}
> +	}

Are you sure the check against list_empty is necessary? Won't list_for_each_entry()
simply be a NOP if the list is empty? (genuine question)

> +
> +	return NULL;
> +}
> +
> +static int cs40l50_owt_upload(struct cs40l50_private *cs40l50, s16 *in_data, u32 in_data_nibbles)
> +{
> +	size_t in_data_bytes = 2 * in_data_nibbles;
> +	struct cs40l50_owt_header header;
> +	u32 wt_offset, wt_size_words;
> +	int error;
> +	u8 *data;
> +
> +	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_OWT_NEXT, &wt_offset);
> +	if (error)
> +		return error;
> +
> +	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_OWT_SIZE, &wt_size_words);
> +	if (error)
> +		return error;
> +
> +	if ((wt_size_words * sizeof(u32)) < CS40L50_WT_HEADER_PWLE_SIZE + in_data_bytes)
> +		return -ENOSPC;
> +
> +	data = kcalloc(CS40L50_WT_HEADER_PWLE_SIZE + in_data_bytes, sizeof(u8), GFP_KERNEL);
> +	if (IS_ERR_OR_NULL(data))
> +		return -ENOMEM;
> +
> +	header.wvfrm_type = in_data[0] == CS40L50_PCM_ID ? CS40L50_WT_TYPE_PCM :
> +			CS40L50_WT_TYPE_PWLE;

Nit: this is a bit more readable:

	header.wvfrm_type = in_data[0] == CS40L50_PCM_ID ? CS40L50_WT_TYPE_PCM
							 : CS40L50_WT_TYPE_PWLE;

> +	header.offset = CS40L50_WT_HEADER_OFFSET;
> +	header.data_size = in_data_bytes / sizeof(u32);
> +
> +	memcpy(data, &header, sizeof(header));
> +	memcpy(data + (CS40L50_WT_HEADER_OFFSET * sizeof(u32)), in_data, in_data_bytes);
> +
> +	error = regmap_bulk_write(cs40l50->regmap, CS40L50_DSP1_OWT_BASE +
> +			(wt_offset * sizeof(u32)), data,
> +			CS40L50_WT_HEADER_PWLE_SIZE + in_data_bytes);
> +	if (error)
> +		goto err_free;
> +
> +	error = cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1, CS40L50_MBOX_CMD_OWT_PUSH);
> +err_free:
> +	kfree(data);

We can avoid a goto and label for this simple case as follows:

	error = ...
	if (!error)
		error = ...

	kfree(...);

	return error;

> +
> +	return error;
> +}
> +
> +static int cs40l50_add_effect(struct input_dev *dev, struct ff_effect *effect,
> +		struct ff_effect *old)

Nit: alignment

> +{
> +	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +	bool is_new = false;
> +	struct cs40l50_effect *new_effect;
> +	u32 nwaves, base_index, max_index;
> +	s16 *raw_custom_data;
> +	int error;
> +
> +	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;
> +	}
> +
> +	raw_custom_data = kcalloc(effect->u.periodic.custom_len, sizeof(s16), GFP_KERNEL);
> +	if (IS_ERR_OR_NULL(raw_custom_data))
> +		return -ENOMEM;
> +
> +	error = copy_from_user(raw_custom_data, effect->u.periodic.custom_data,
> +			sizeof(s16) * effect->u.periodic.custom_len);
> +	if (error)
> +		goto out_free;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	new_effect = cs40l50_find_effect(cs40l50, effect->id);
> +	if (!new_effect) {
> +		is_new = true;
> +		new_effect = kzalloc(sizeof(*new_effect), GFP_KERNEL);
> +		if (IS_ERR_OR_NULL(new_effect)) {
> +			error = -ENOMEM;
> +			goto err_mutex;
> +		}
> +	}
> +
> +	if (effect->u.periodic.custom_len > CS40L50_CUSTOM_DATA_SIZE) {
> +		error = cs40l50_dsp_read(cs40l50, CS40L50_NUM_OF_OWT_WAVES,
> +				&new_effect->trigger_index);
> +		if (error)
> +			goto err_free;
> +
> +		new_effect->wvfrm_bank = CS40L50_WVFRM_BANK_OWT;
> +		base_index = CS40L50_OWT_INDEX_START;
> +		max_index = CS40L50_OWT_INDEX_END;
> +	} else {
> +		new_effect->trigger_index = (raw_custom_data[1] & 0xffffu);
> +		new_effect->wvfrm_bank = (raw_custom_data[0] & 0xffffu);
> +
> +		if (new_effect->wvfrm_bank == CS40L50_WVFRM_BANK_ROM) {
> +			base_index = CS40L50_ROM_BANK_INDEX_START;
> +			max_index = CS40L50_ROM_BANK_INDEX_END;
> +		} else if (new_effect->wvfrm_bank == CS40L50_WVFRM_BANK_RAM) {
> +			error = cs40l50_dsp_read(cs40l50, CS40L50_NUM_OF_WAVES, &nwaves);
> +			if (error)
> +				goto err_free;
> +
> +			base_index = CS40L50_RAM_BANK_INDEX_START;
> +			max_index = base_index + nwaves - 1;
> +		} else {
> +			dev_err(cs40l50->dev, "Invalid waveform bank\n");
> +			error = -EINVAL;
> +			goto err_free;
> +		}
> +	}
> +
> +	new_effect->trigger_index += base_index;
> +
> +	if (new_effect->trigger_index > max_index) {
> +		dev_err(cs40l50->dev, "Index %#X out of bounds\n", new_effect->trigger_index);
> +		error = -EINVAL;
> +		goto err_free;
> +	}
> +
> +	if (effect->trigger.button) {
> +		new_effect->mapping = CS40L50_DSP1_GPIO_BASE + sizeof(u32) *
> +				FIELD_GET(CS40L50_BTN_NUM_MASK, effect->trigger.button) * 2 -
> +				FIELD_GET(CS40L50_BTN_EDGE_MASK, effect->trigger.button);
> +
> +		error = cs40l50_dsp_write(cs40l50, new_effect->mapping, effect->trigger.button);
> +		if (error)
> +			goto err_free;
> +	}
> +
> +	if (new_effect->wvfrm_bank == CS40L50_WVFRM_BANK_OWT) {
> +		error = cs40l50_owt_upload(cs40l50, raw_custom_data,
> +				effect->u.periodic.custom_len);
> +		if (error)
> +			goto err_free;
> +	}
> +
> +err_free:
> +	if (is_new)
> +		error ? kfree(new_effect) : list_add(&new_effect->list, &cs40l50->effect_head);

Just wanted to echo Charles' point that this made my head spin a bit. Good
to know it's not just me ;)

> +err_mutex:
> +	mutex_unlock(&cs40l50->lock);
> +out_free:
> +	kfree(raw_custom_data);
> +
> +	return error ? -EFAULT : 0;
> +}
> +
> +
> +static void cs40l50_vibe_start_worker(struct work_struct *work)
> +{
> +	struct cs40l50_private *cs40l50 =
> +			container_of(work, struct cs40l50_private, vibe_start_work);
> +	struct cs40l50_effect *effect;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	effect = cs40l50_find_effect(cs40l50, cs40l50->trigger_effect->id);
> +	if (effect)
> +		cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1, effect->trigger_index);
> +
> +	mutex_unlock(&cs40l50->lock);
> +}
> +
> +static void cs40l50_vibe_stop_worker(struct work_struct *work)
> +{
> +	struct cs40l50_private *cs40l50 =
> +			container_of(work, struct cs40l50_private, vibe_stop_work);
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1, CS40L50_MBOX_CMD_STOP_PLAYBACK);
> +
> +	mutex_unlock(&cs40l50->lock);
> +}
> +
> +static int cs40l50_playback_effect(struct input_dev *dev, int effect_id, int val)
> +{
> +	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +
> +	cs40l50->trigger_effect = &dev->ff->effects[effect_id];
> +
> +	if (val > 0)
> +		queue_work(cs40l50->vibe_workqueue, &cs40l50->vibe_start_work);
> +	else
> +		queue_work(cs40l50->vibe_workqueue, &cs40l50->vibe_stop_work);
> +
> +	return 0;
> +}
> +
> +static int cs40l50_erase_effect(struct input_dev *dev, int effect_id)
> +{
> +	int error = 0;
> +	struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +	struct cs40l50_effect *effect, *effect_owt;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	effect = cs40l50_find_effect(cs40l50, dev->ff->effects[effect_id].id);
> +	if (!effect) {
> +		error = -EINVAL;
> +		goto err_mutex;
> +	}
> +
> +	if (effect->mapping != CS40L50_GPIO_MAPPING_INVALID) {
> +		error = cs40l50_dsp_write(cs40l50, effect->mapping, CS40L50_GPI_DISABLE);
> +		if (error)
> +			goto err_mutex;
> +	}
> +
> +	if (effect->wvfrm_bank == CS40L50_WVFRM_BANK_OWT) {
> +		error = cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1,
> +				CS40L50_MBOX_CMD_OWT_DELETE | effect->trigger_index);

Nit: alignment here and elsewhere.

> +		if (error)
> +			goto err_mutex;
> +
> +		list_for_each_entry(effect_owt, &cs40l50->effect_head, list) {
> +			if (effect_owt->wvfrm_bank == CS40L50_WVFRM_BANK_OWT &&
> +					effect_owt->trigger_index > effect->trigger_index)
> +				effect_owt->trigger_index--;
> +		}
> +	}
> +
> +	list_del(&effect->list);
> +	kfree(effect);
> +err_mutex:
> +	mutex_unlock(&cs40l50->lock);
> +
> +	return error;
> +}
> +
> +static int cs40l50_mailbox_read_next(struct cs40l50_private *cs40l50, u32 *val)
> +{
> +	u32 wt_ptr, rd_ptr;
> +	int error = 0;

No need to initialize 'error' here.

> +
> +	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_MBOX_QUEUE_WT, &wt_ptr);
> +	if (error)
> +		return error;
> +
> +	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_MBOX_QUEUE_RD, &rd_ptr);
> +	if (error)
> +		return error;
> +
> +	if (wt_ptr == rd_ptr) {
> +		*val = 0;
> +		return 0;
> +	}
> +
> +	error = cs40l50_dsp_read(cs40l50, 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 cs40l50_dsp_write(cs40l50, CS40L50_DSP1_MBOX_QUEUE_RD, rd_ptr);
> +}
> +
> +static int cs40l50_handle_f0_est_done(struct cs40l50_private *cs40l50)
> +{
> +	int error;
> +	u32 f_zero;
> +
> +	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_F0_ESTIMATION, &f_zero);
> +	if (error)
> +		return error;
> +
> +	return cs40l50_dsp_write(cs40l50, CS40L50_DSP1_F0_STORED, f_zero);
> +}
> +
> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> +{
> +	int integer, fractional, error;
> +	u32 redc;
> +
> +	error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_RE_EST_STATUS, &redc);
> +	if (error)
> +		return error;
> +
> +	error = cs40l50_dsp_write(cs40l50, CS40L50_DSP1_REDC_ESTIMATION, redc);
> +	if (error)
> +		return error;
> +
> +	/* Convert from Q8.15 to (Q7.17 * 29/240) */
> +	integer = (redc >> 15) & 0xFF;
> +	fractional = (redc & 0x7FFF) * 4;
> +
> +	return cs40l50_dsp_write(cs40l50, CS40L50_DSP1_REDC_STORED,
> +		(((integer << 17) | fractional) * 29 / 240) & GENMASK(23, 0));
> +}
> +
> +static int cs40l50_error_release(struct cs40l50_private *cs40l50)
> +{
> +	int error;
> +
> +	error = cs40l50_dsp_write(cs40l50, CS40L50_ERROR_RELEASE, CS40L50_ERROR_RELEASE_MASK);
> +	if (error)
> +		return error;
> +
> +	return cs40l50_dsp_write(cs40l50, CS40L50_ERROR_RELEASE, 0);
> +}
> +
> +static irqreturn_t cs40l50_handle_mbox_buffer(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:
> +			goto out_irq;
> +		case CS40L50_DSP_MBOX_HAPTIC_TRIGGER_GPIO:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
> +			break;
> +		case CS40L50_DSP_MBOX_HAPTIC_TRIGGER_MBOX:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_MBOX\n");
> +			break;
> +		case CS40L50_DSP_MBOX_HAPTIC_TRIGGER_I2S:
> +			dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_I2S\n");
> +			break;
> +		case CS40L50_DSP_MBOX_HAPTIC_COMPLETE_MBOX:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_MBOX\n");
> +			break;
> +		case CS40L50_DSP_MBOX_HAPTIC_COMPLETE_GPIO:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_GPIO\n");
> +			break;
> +		case CS40L50_DSP_MBOX_HAPTIC_COMPLETE_I2S:
> +			dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_I2S\n");
> +			break;
> +		case CS40L50_DSP_MBOX_F0_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_START\n");
> +			break;
> +		case CS40L50_DSP_MBOX_F0_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: F0_EST_DONE\n");
> +			error = cs40l50_handle_f0_est_done(cs40l50);
> +			if (error)
> +				goto out_irq;
> +			break;
> +		case CS40L50_DSP_MBOX_REDC_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_START\n");
> +			break;
> +		case CS40L50_DSP_MBOX_REDC_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_DONE\n");
> +			error = cs40l50_handle_redc_est_done(cs40l50);
> +			if (error)
> +				goto out_irq;
> +			break;
> +		case CS40L50_DSP_MBOX_LE_EST_START:
> +			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_START\n");
> +			break;
> +		case CS40L50_DSP_MBOX_LE_EST_DONE:
> +			dev_dbg(cs40l50->dev, "Mailbox: LE_EST_DONE\n");
> +			break;
> +		case CS40L50_DSP_MBOX_AWAKE:
> +			dev_dbg(cs40l50->dev, "Mailbox: AWAKE\n");
> +			break;
> +		case CS40L50_DSP_MBOX_INIT:
> +			dev_dbg(cs40l50->dev, "Mailbox: INIT\n");
> +			break;
> +		case CS40L50_DSP_MBOX_ACK:
> +			dev_dbg(cs40l50->dev, "Mailbox: ACK\n");
> +			break;
> +		case CS40L50_DSP_MBOX_ERR_EVENT_UNMAPPED:
> +			dev_err(cs40l50->dev, "Unmapped event\n");
> +			break;
> +		case CS40L50_DSP_MBOX_ERR_EVENT_MODIFY:
> +			dev_err(cs40l50->dev, "Attempt to modify event index failed\n");
> +			break;
> +		case CS40L50_DSP_MBOX_ERR_NULLPTR:
> +			dev_err(cs40l50->dev, "Null pointer\n");
> +			break;
> +		case CS40L50_DSP_MBOX_ERR_BRAKING:
> +			dev_err(cs40l50->dev, "Braking not in progress\n");
> +			break;
> +		case CS40L50_DSP_MBOX_ERR_INVAL_SRC:
> +			dev_err(cs40l50->dev, "Suspend/resume invalid source\n");
> +			break;
> +		case CS40L50_DSP_MBOX_ERR_ENABLE_RANGE:
> +			dev_err(cs40l50->dev, "GPIO enable out of range\n");
> +			break;
> +		case CS40L50_DSP_MBOX_ERR_GPIO_UNMAPPED:
> +			dev_err(cs40l50->dev, "GPIO not mapped\n");
> +			break;
> +		case CS40L50_DSP_MBOX_ERR_ISR_RANGE:
> +			dev_err(cs40l50->dev, "GPIO ISR out of range\n");
> +			break;
> +		case CS40L50_DSP_MBOX_PERMANENT_SHORT:
> +			dev_crit(cs40l50->dev, "Permanent short detected\n");
> +			break;
> +		case CS40L50_DSP_MBOX_RUNTIME_SHORT:
> +			dev_err(cs40l50->dev, "Runtime short detected\n");
> +			error = cs40l50_error_release(cs40l50);
> +			if (error)
> +				goto out_irq;
> +			break;
> +		default:
> +			dev_err(cs40l50->dev, "Mailbox value %#X not recognized\n", val);
> +			error = -EINVAL;
> +			goto out_irq;
> +		}
> +	}
> +
> +	error = -EIO;
> +
> +out_irq:
> +	mutex_unlock(&cs40l50->lock);
> +
> +	return IRQ_RETVAL(!error);
> +}
> +
> +
> +static irqreturn_t cs40l50_vddb_uv(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "VDD_B undervoltage error\n");
> +
> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_boost_current_limit(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "Boost current limit engagement\n");
> +
> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_boost_short(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "Boost inductor short error\n");
> +
> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_boost_uv(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "Boost undervoltage error\n");
> +
> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_overtemperature(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "Overtemperature error\n");
> +
> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_amp_short(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "Amp short error\n");
> +
> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_global_error(int irq, void *data)
> +{
> +	struct cs40l50_private *cs40l50 = data;
> +
> +	dev_err(cs40l50->dev, "MSM chip error\n");
> +
> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static const struct cs40l50_irq cs40l50_irqs[] = {
> +	CS40L50_IRQ(CS40L50_AMP_SHORT_IRQ, "Amp short", cs40l50_amp_short),
> +	CS40L50_IRQ(CS40L50_VIRT2_MBOX_IRQ, "Firmware interrupt", cs40l50_handle_mbox_buffer),
> +	CS40L50_IRQ(CS40L50_TEMP_ERR_IRQ, "Overtemperature", cs40l50_overtemperature),
> +	CS40L50_IRQ(CS40L50_BST_UVP_IRQ, "Boost undervoltage", cs40l50_boost_uv),
> +	CS40L50_IRQ(CS40L50_BST_SHORT_IRQ, "Boost short", cs40l50_boost_short),
> +	CS40L50_IRQ(CS40L50_BST_ILIMIT_IRQ, "Boost current limit", cs40l50_boost_current_limit),
> +	CS40L50_IRQ(CS40L50_UVLO_VDDBATT_IRQ, "VDD_B undervoltage", cs40l50_vddb_uv),
> +	CS40L50_IRQ(CS40L50_GLOBAL_ERROR_IRQ, "Global error", cs40l50_global_error),
> +};
> +
> +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 const struct regmap_irq_chip cs40l50_regmap_irq_chip = {
> +	.name = "cs40l50 IRQ1 Controller",
> +	.status_base = CS40L50_IRQ1_INT_1,
> +	.mask_base = CS40L50_IRQ1_MASK_1,
> +	.ack_base = CS40L50_IRQ1_INT_1,
> +	.num_regs = 19,
> +	.irqs = cs40l50_reg_irqs,
> +	.num_irqs = ARRAY_SIZE(cs40l50_reg_irqs),
> +	.runtime_pm = true,
> +};
> +
> +static const struct reg_sequence cs40l50_ext_bst_seq[] = {
> +	{ CS40L50_BLOCK_ENABLES, CS40L50_MON_ENABLE },
> +	{ CS40L50_MON_VALUE_CTRL, CS40L50_VBST_VDD_AMP },
> +	{ CS40L50_TEST_KEY, CS40L50_KEY_SET_1 },
> +	{ CS40L50_TEST_KEY, CS40L50_KEY_SET_2 },
> +	{ CS40L50_TEST_VIS_SPARE, CS40L50_TEMP_SENSOR_3 },
> +	{ CS40L50_MON_ERROR_OVERIDE, CS40L50_TEMP_ERR_THLD },
> +	{ CS40L50_STATUS, CS40L50_TEMP_WARN_THLD },
> +	{ CS40L50_TEST_CONFIG, CS40L50_TEMP_FILT_DEBOUNCE_OFF },
> +	{ CS40L50_TEMP_WARN_CONFIG, CS40L50_TEMP_WARN_ATK_VOL },
> +};
> +
> +static int cs40l50_config_bst(struct cs40l50_private *cs40l50)
> +{
> +	int error, i;
> +
> +	if (device_property_present(cs40l50->dev, "cirrus,external-boost")) {
> +		error = regmap_multi_reg_write(cs40l50->regmap, cs40l50_ext_bst_seq,
> +				ARRAY_SIZE(cs40l50_ext_bst_seq));
> +		if (error)
> +			return error;
> +
> +		for (i = 0; i < ARRAY_SIZE(cs40l50_ext_bst_seq); i++) {
> +			error = cs40l50_pseq_write(cs40l50, cs40l50_ext_bst_seq[i].reg,
> +					cs40l50_ext_bst_seq[i].def);
> +			if (error)
> +				return error;
> +		}
> +
> +		return 0;
> +	}
> +
> +	/* Internal boost config */

This driver is conspicuously void of comments and documentation; why is
this line mysterious enough to warrant one?

> +	return cs40l50_dsp_write(cs40l50, CS40L50_BST_LPMODE_SEL, CS40L50_DCM_LOW_POWER);
> +}
> +
> +static int cs40l50_patch_firmware(struct cs40l50_private *cs40l50)
> +{
> +	const struct firmware *bin = NULL, *wmfw = NULL;
> +	int error = 0;
> +
> +	if (request_firmware(&bin, "cs40l50.bin", cs40l50->dev))
> +		dev_info(cs40l50->dev, "Could not request wavetable file: %d\n", error);
> +
> +	if (request_firmware(&wmfw, "cs40l50.wmfw", cs40l50->dev))
> +		dev_info(cs40l50->dev, "Could not request firmware patch file: %d\n", error);
> +
> +	if (wmfw) {

It is a much more common design pattern to bail if request_firmware() returns
an error, than to proceed and check against the FW pointer remaining NULL.

Is this done because cs_dsp_power_up() must be called, with or without a wmfw
or coefficient file being available?

> +		error = cs40l50_dsp_write(cs40l50, CS40L50_CCM_CORE_CONTROL,
> +				CS40L50_DSP1_CLOCK_DISABLE);
> +		if (error)
> +			goto err_fw;
> +
> +		error = cs40l50_dsp_write(cs40l50, CS40L50_RAM_INIT,
> +				CS40L50_DSP1_RAM_INIT_FLAG);
> +		if (error)
> +			goto err_fw;
> +
> +		error = cs40l50_dsp_write(cs40l50, CS40L50_PWRMGT_CTL,
> +				CS40L50_DSP1_MEM_RDY_HW);
> +		if (error)
> +			goto err_fw;
> +	}
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	cs_dsp_power_up(&cs40l50->dsp, wmfw, "cs40l50.wmfw", bin, "cs40l50.bin", "cs40l50");
> +
> +	mutex_unlock(&cs40l50->lock);
> +
> +	if (wmfw)
> +		error = cs40l50_dsp_write(cs40l50, CS40L50_CCM_CORE_CONTROL,
> +				CS40L50_DSP1_CLOCK_ENABLE);
> +
> +err_fw:
> +	release_firmware(wmfw);
> +	release_firmware(bin);
> +
> +	return error;
> +}
> +
> +static int cs40l50_pseq_init(struct cs40l50_private *cs40l50)
> +{
> +	struct cs40l50_pseq_op *pseq_op;
> +	int error, i, num_words;
> +	u8 operation;
> +	u32 *words;
> +
> +	INIT_LIST_HEAD(&cs40l50->pseq_op_head);
> +
> +	words = kcalloc(CS40L50_PSEQ_MAX_WORDS, sizeof(u32), GFP_KERNEL);
> +	if (IS_ERR_OR_NULL(words))
> +		return -ENOMEM;

Just curious what is the need to call IS_ERR_OR_NULL() here and throughout
after allocation rather than just !foo ?

> +
> +	error = regmap_bulk_read(cs40l50->regmap, CS40L50_DSP1_POWER_ON_SEQ, words,
> +			CS40L50_PSEQ_MAX_WORDS);
> +	if (error)
> +		goto err_free;
> +
> +	for (i = 0; i < CS40L50_PSEQ_MAX_WORDS; i++)
> +		words[i] = be32_to_cpu(words[i]);
> +
> +	for (i = 0; i < CS40L50_PSEQ_MAX_WORDS; i += num_words) {
> +		operation = (words[i] & CS40L50_PSEQ_OP_MASK) >> CS40L50_PSEQ_OP_SHIFT;
> +
> +		switch (operation) {
> +		case CS40L50_PSEQ_OP_END:
> +			num_words = CS40L50_PSEQ_OP_END_WORDS;
> +			break;
> +		case CS40L50_PSEQ_OP_WRITE_ADDR8:
> +		case CS40L50_PSEQ_OP_WRITE_H16:
> +		case CS40L50_PSEQ_OP_WRITE_L16:
> +			num_words = CS40L50_PSEQ_OP_WRITE_X16_WORDS;
> +			break;
> +		case CS40L50_PSEQ_OP_WRITE_FULL:
> +			num_words = CS40L50_PSEQ_OP_WRITE_FULL_WORDS;
> +			break;
> +		default:
> +			dev_err(cs40l50->dev, "Invalid OP code 0x%02X\n", operation);
> +			error = -EINVAL;
> +			goto err_free;
> +		}
> +
> +		pseq_op = devm_kzalloc(cs40l50->dev, sizeof(*pseq_op), GFP_KERNEL);
> +		if (!pseq_op) {
> +			error = -ENOMEM;
> +			goto err_free;
> +		}
> +
> +		memcpy(pseq_op->words, &words[i], num_words * sizeof(u32));
> +		pseq_op->size = num_words;
> +		pseq_op->offset = i * sizeof(u32);
> +		pseq_op->operation = operation;
> +		list_add(&pseq_op->list, &cs40l50->pseq_op_head);
> +
> +		if (operation == CS40L50_PSEQ_OP_END)
> +			break;
> +	}
> +
> +	if (operation != CS40L50_PSEQ_OP_END) {
> +		dev_err(cs40l50->dev, "PSEQ_END_OF_SCRIPT not found\n");
> +		error = -ENOENT;
> +	}
> +
> +err_free:
> +	kfree(words);
> +
> +	return error;
> +}
> +
> +static int cs40l50_input_init(struct cs40l50_private *cs40l50)
> +{
> +	int error;
> +	u32 val;
> +
> +	error = cs40l50_dsp_read(cs40l50, CS40L50_DEVID, &val);
> +	if (error)
> +		return error;
> +
> +	if (val != CS40L50_DEVID_A) {
> +		dev_err(cs40l50->dev, "Invalid device ID: %#010X\n", val);
> +		return -EINVAL;
> +	}
> +
> +	cs40l50->input->id.product = val & 0xFFFF;
> +
> +	error = cs40l50_dsp_read(cs40l50, CS40L50_REVID, &val);
> +	if (error)
> +		return error;
> +
> +	if (val != CS40L50_REVID_B0) {
> +		dev_err(cs40l50->dev, "Invalid device revision: %#04X\n", val);
> +		return -EINVAL;
> +	}
> +
> +	cs40l50->input->id.version = val;
> +
> +	cs40l50->input = devm_input_allocate_device(cs40l50->dev);
> +	if (IS_ERR_OR_NULL(cs40l50->input))
> +		return cs40l50->input ? PTR_ERR(cs40l50->input) : -ENOMEM;
> +
> +	cs40l50->input->name = "cs40l50_input";
> +	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;
> +
> +	clear_bit(FF_RUMBLE, cs40l50->input->ffbit);

If there is a HW limitation that prevents this capability, it would be
handy to explain as much with a comment.

> +
> +	cs40l50->input->ff->upload = cs40l50_add_effect;
> +	cs40l50->input->ff->playback = cs40l50_playback_effect;
> +	cs40l50->input->ff->erase = cs40l50_erase_effect;
> +
> +	INIT_LIST_HEAD(&cs40l50->effect_head);
> +
> +	return input_register_device(cs40l50->input);
> +}
> +
> +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 },
> +};

This is more readable as:

	[...],
	{
		.type = ...,
		.base = ...,
	},
	[...],

> +
> +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_DSP1_CORE_BASE;
> +	cs40l50->dsp.base_sysinfo = CS40L50_DSP1_SYS_INFO_ID;
> +	cs40l50->dsp.mem = cs40l50_dsp_regions;
> +	cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
> +	cs40l50->dsp.lock_regions = 0xFFFFFFFF;
> +	cs40l50->dsp.no_core_startstop = true;
> +	cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
> +
> +	return cs_dsp_halo_init(&cs40l50->dsp);
> +}
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50)
> +{
> +	int error, i, irq;
> +	u32 val;
> +
> +	mutex_init(&cs40l50->lock);
> +
> +	error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
> +			cs40l50_supplies);
> +	if (error)
> +		return dev_err_probe(cs40l50->dev, error, "Failed to request supplies\n");
> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +	if (error)
> +		return dev_err_probe(cs40l50->dev, error, "Failed to enable supplies\n");
> +
> +	cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(cs40l50->reset_gpio)) {
> +		error = PTR_ERR(cs40l50->reset_gpio);
> +		goto err;
> +	}

Krzysztof covered this already, but I just want to re-iterate that you are
kind of swallowing your own head by calling cs40l50_remove() in your error
path. This is a confusing and uncommon design pattern.

You cannot have possibly booted the DSP or allocated work here, so why not
just return?

> +
> +	usleep_range(CS40L50_MIN_RESET_PULSE_WIDTH_US, CS40L50_MIN_RESET_PULSE_WIDTH_US + 100);
> +
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
> +
> +	usleep_range(CS40L50_CP_READY_DELAY_US, CS40L50_CP_READY_DELAY_US + 1000);
> +
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_HALO_STATE, &val);
> +		if (!error && val < 0xFFFF && val >= CS40L50_HALO_STATE_BOOT_DONE)
> +			break;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}
> +
> +	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> +		dev_err(cs40l50->dev, "Firmware boot failed: %d, halo state = %#x\n", error, val);
> +		goto err;
> +	}
> +
> +	cs40l50->vibe_workqueue = alloc_ordered_workqueue("cs40l50_workqueue", WQ_HIGHPRI);
> +	if (!cs40l50->vibe_workqueue) {
> +		error = -ENOMEM;
> +		goto err;
> +	}
> +
> +	INIT_WORK(&cs40l50->vibe_start_work, cs40l50_vibe_start_worker);
> +	INIT_WORK(&cs40l50->vibe_stop_work, cs40l50_vibe_stop_worker);
> +
> +	error = cs40l50_cs_dsp_init(cs40l50);
> +	if (error)
> +		goto err;

It seems your error path is complex because you intend to unwind the DSP
in case of an error, in which case any teardown should be explicitly
called from the bottom of probe() rather than calling remove() on your
own.

Opinions may differ here, but mine is that you need not go out of your
way to power down the DSP if glaring errors are encountered during probe.
If you cannot read HALO_STATE, do you really expect to be able to do any
additional work anyway?

Understanding that haptics bears the risk of a motor stuck on, perhaps it
is sufficient to simply throw the device into reset if things go afoul?

> +
> +	error = cs40l50_input_init(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_patch_firmware(cs40l50);
> +	if (error)
> +		goto err;

I just want to re-iterate Charles' comment; this _will_not_ work across
all customer platforms because you cannot guarantee the filesystem is
ready here if the driver is built-in.

In the best case you will get -ENOENT right away; in the worst case you
will get a deadlock. If this device does not have nonvolatile memory and
you must load FW from probe, use request_firmware_nowait() instead.

> +
> +	error = cs40l50_pseq_init(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = cs40l50_config_bst(cs40l50);
> +	if (error)
> +		goto err;
> +
> +	error = devm_regmap_add_irq_chip(cs40l50->dev, cs40l50->regmap, cs40l50->irq,
> +			IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW, 0, &cs40l50_regmap_irq_chip,
> +			&cs40l50->irq_data);

We should not be mandating IRQ polarity in the driver. Sometimes the customer
may use a GPIO expander with inverted polarity; we need to let dts govern the
polarity.

> +	if (error) {
> +		dev_err(cs40l50->dev, "Failed to register IRQ chip: %d\n", error);
> +		goto err;
> +	}
> +
> +	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(cs40l50->dev, "Failed to get %s\n", cs40l50_irqs[i].name);
> +			error = irq;
> +			goto err;
> +		}
> +
> +		error = devm_request_threaded_irq(cs40l50->dev, irq, NULL, cs40l50_irqs[i].handler,
> +				IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW,
> +						cs40l50_irqs[i].name, cs40l50);
> +		if (error) {
> +			dev_err(cs40l50->dev, "Failed to request IRQ %s: %d\n",
> +					cs40l50_irqs[i].name, error);
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	cs40l50_remove(cs40l50);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_probe);
> +
> +int cs40l50_remove(struct cs40l50_private *cs40l50)
> +{
> +	disable_irq(cs40l50->irq);
> +
> +	if (cs40l50->dsp.booted)
> +		cs_dsp_power_down(&cs40l50->dsp);
> +	if (&cs40l50->dsp)
> +		cs_dsp_remove(&cs40l50->dsp);
> +
> +	if (cs40l50->vibe_workqueue) {
> +		flush_workqueue(cs40l50->vibe_workqueue);
> +		destroy_workqueue(cs40l50->vibe_workqueue);
> +	}
> +
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
> +	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_remove);
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/input/cs40l50.h b/include/linux/input/cs40l50.h
> new file mode 100644
> index 000000000000..f2c8a101fa00
> --- /dev/null
> +++ b/include/linux/input/cs40l50.h
> @@ -0,0 +1,321 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#ifndef __CS40L50_H__
> +#define __CS40L50_H__
> +
> +#include <linux/bitops.h>

bitops.h is quite outdated; did you mean bits.h?

> +#include <linux/gpio/consumer.h>
> +#include <linux/input.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/firmware/cirrus/wmfw.h>
> +
> +/* Boost */
> +#define CS40L50_TEST_KEY			0x40
> +#define CS40L50_BLOCK_ENABLES			0x2018
> +#define CS40L50_ERROR_RELEASE			0x2034
> +#define CS40L50_PWRMGT_CTL			0x2900
> +#define CS40L50_BST_LPMODE_SEL			0x3810
> +#define CS40L50_STATUS				0x4200
> +#define CS40L50_MON_ERROR_OVERIDE		0x4220
> +#define CS40L50_TEST_CONFIG			0x4240
> +#define CS40L50_MON_VALUE_CTRL			0x4404
> +#define CS40L50_TEST_VIS_SPARE			0x5C00
> +#define CS40L50_TEMP_WARN_CONFIG		0x6024
> +#define CS40L50_KEY_SET_1		0x55
> +#define CS40L50_KEY_SET_2		0xAA
> +#define CS40L50_TEMP_SENSOR_3		0x400
> +#define CS40L50_DCM_LOW_POWER		0x1
> +#define CS40L50_MON_ENABLE		0x3201
> +#define CS40L50_VBST_VDD_AMP		0x1000000
> +#define CS40L50_TEMP_ERR_THLD		0x8000007D
> +#define CS40L50_TEMP_WARN_THLD		0x8
> +#define CS40L50_TEMP_FILT_DEBOUNCE_OFF	0x510002B5
> +#define CS40L50_TEMP_WARN_ATK_VOL	0x522303
> +
> +/* Interrupts */
> +#define CS40L50_IRQ1_INT_1			0xE010
> +#define CS40L50_IRQ1_INT_2			0xE014
> +#define CS40L50_IRQ1_INT_3			0xE018
> +#define CS40L50_IRQ1_INT_4			0xE01C
> +#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_INT_19			0xE058
> +#define CS40L50_IRQ1_MASK_1			0xE090
> +#define CS40L50_IRQ1_MASK_2			0xE094
> +#define CS40L50_IRQ1_MASK_3			0xE098
> +#define CS40L50_IRQ1_MASK_8			0xE0AC
> +#define CS40L50_IRQ1_MASK_9			0xE0B0
> +#define CS40L50_IRQ1_MASK_10			0xE0B4
> +#define CS40L50_IRQ1_MASK_18			0xE0D4
> +#define CS40L50_IRQ1_MASK_19			0xE0D8
> +#define CS40L50_IRQ1_STS_1			0xE210
> +#define CS40L50_IRQ1_STS_2			0xE214
> +#define CS40L50_IRQ1_STS_3			0xE218
> +#define CS40L50_IRQ1_STS_8			0xE22C
> +#define CS40L50_IRQ1_STS_9			0xE230
> +#define CS40L50_IRQ1_STS_10			0xE234
> +#define CS40L50_IRQ1_STS_18			0xE254
> +#define CS40L50_IRQ1_STS_19			0xE258
> +#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_ERROR_RELEASE_MASK	BIT(11)
> +#define CS40L50_BOOT_DONE_MASK		BIT(1)
> +#define CS40L50_IRQ(_irq, _name, _hand)	\
> +	{				\
> +		.irq = _irq,		\
> +		.name = _name,		\
> +		.handler = _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_RAM_BANK_INDEX_END		0x100007F
> +#define CS40L50_OWT_INDEX_START			0x1400000
> +#define CS40L50_OWT_INDEX_END			0x140007F
> +#define CS40L50_ROM_BANK_INDEX_START		0x1800000
> +#define CS40L50_ROM_BANK_INDEX_END		0x180001A
> +#define CS40L50_OTP_BUZZ			0x1800080
> +#define CS40L50_RAM_BUZZ_INDEX_START		0x1800081
> +#define CS40L50_RAM_BUZZ_INDEX_END		0x1800085
> +#define CS40L50_MBOX_CMD_HIBERNATE		0x2000001
> +#define CS40L50_MBOX_CMD_WAKEUP			0x2000002
> +#define CS40L50_MBOX_CMD_PREVENT_HIBERNATE	0x2000003
> +#define CS40L50_MBOX_CMD_ALLOW_HIBERNATION	0x2000004
> +#define CS40L50_MBOX_CMD_SHUTDOWN		0x2000005
> +#define CS40L50_MBOX_CMD_DURATION_REPORT	0x3000001
> +#define CS40L50_MBOX_CMD_START_I2S		0x3000002
> +#define CS40L50_MBOX_CMD_STOP_I2S		0x3000003
> +#define CS40L50_MBOX_CMD_HAPTICS_LOGGER_RESET	0x3000004
> +#define CS40L50_MBOX_CMD_OWT_PUSH		0x3000008
> +#define CS40L50_MBOX_CMD_OWT_RESET		0x3000009
> +#define CS40L50_MBOX_CMD_DVL_REINIT		0x300000A
> +#define CS40L50_MBOX_CMD_STOP_PLAYBACK		0x5000000
> +#define CS40L50_MBOX_CMD_F0_EST			0x7000001
> +#define CS40L50_MBOX_CMD_REDC_EST		0x7000002
> +#define CS40L50_MBOX_CMD_LE_EST			0x7000004
> +#define CS40L50_MBOX_CMD_OWT_DELETE		0xD000000
> +
> +/* Mailbox Outbound Commands */
> +#define CS40L50_MBOX_QUEUE_BASE				0x11004
> +#define CS40L50_MBOX_QUEUE_END				0x1101C
> +#define CS40L50_DSP_VIRTUAL1_MBOX_1			0x11020
> +#define CS40L50_DSP1_MBOX_QUEUE_WT			0x28042C8
> +#define CS40L50_DSP1_MBOX_QUEUE_RD			0x28042CC
> +#define CS40L50_DSP_MBOX_HAPTIC_COMPLETE_MBOX	0x1000000
> +#define CS40L50_DSP_MBOX_HAPTIC_COMPLETE_GPIO	0x1000001
> +#define CS40L50_DSP_MBOX_HAPTIC_COMPLETE_I2S	0x1000002
> +#define CS40L50_DSP_MBOX_HAPTIC_TRIGGER_MBOX	0x1000010
> +#define CS40L50_DSP_MBOX_HAPTIC_TRIGGER_GPIO	0x1000011
> +#define CS40L50_DSP_MBOX_HAPTIC_TRIGGER_I2S	0x1000012
> +#define CS40L50_DSP_MBOX_INIT			0x2000000
> +#define CS40L50_DSP_MBOX_AWAKE			0x2000002
> +#define CS40L50_DSP_MBOX_F0_EST_START		0x7000011
> +#define CS40L50_DSP_MBOX_F0_EST_DONE		0x7000021
> +#define CS40L50_DSP_MBOX_REDC_EST_START		0x7000012
> +#define CS40L50_DSP_MBOX_REDC_EST_DONE		0x7000022
> +#define CS40L50_DSP_MBOX_LE_EST_START		0x7000014
> +#define CS40L50_DSP_MBOX_LE_EST_DONE		0x7000024
> +#define CS40L50_DSP_MBOX_ACK			0xA000000
> +#define CS40L50_DSP_MBOX_PANIC			0xC000000
> +#define CS40L50_DSP_MBOX_WATERMARK		0xD000000
> +#define CS40L50_DSP_MBOX_ERR_EVENT_UNMAPPED	0xC0004B3
> +#define CS40L50_DSP_MBOX_ERR_EVENT_MODIFY	0xC0004B4
> +#define CS40L50_DSP_MBOX_ERR_NULLPTR		0xC0006A5
> +#define CS40L50_DSP_MBOX_ERR_BRAKING		0xC0006A8
> +#define CS40L50_DSP_MBOX_ERR_INVAL_SRC		0xC0006AC
> +#define CS40L50_DSP_MBOX_ERR_ENABLE_RANGE	0xC000836
> +#define CS40L50_DSP_MBOX_ERR_GPIO_UNMAPPED	0xC000837
> +#define CS40L50_DSP_MBOX_ERR_ISR_RANGE		0xC000838
> +#define CS40L50_DSP_MBOX_PERMANENT_SHORT	0xC000C1C
> +#define CS40L50_DSP_MBOX_RUNTIME_SHORT		0xC000C1D
> +
> +/* DSP */
> +#define CS40L50_DSP1_XMEM_PACKED_0		0x2000000
> +#define CS40L50_DSP1_XMEM_UNPACKED32_0		0x2400000
> +#define CS40L50_DSP1_SYS_INFO_ID		0x25E0000
> +#define CS40L50_DSP1_XMEM_UNPACKED24_0		0x2800000
> +#define CS40L50_NUM_OF_WAVES			0x280CB4C
> +#define CS40L50_DSP1_CORE_BASE			0x2B80000
> +#define CS40L50_DSP1_SCRATCH1			0x2B805C0
> +#define CS40L50_DSP1_SCRATCH2			0x2B805C8
> +#define CS40L50_DSP1_SCRATCH3			0x2B805D0
> +#define CS40L50_DSP1_SCRATCH4			0x2B805D8

I didn't look too closely, but some of these #defines are unused.

> +#define CS40L50_CCM_CORE_CONTROL		0x2BC1000
> +#define CS40L50_RAM_INIT			0x28021DC
> +#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_DSP1_MEM_RDY_HW		0x2
> +#define CS40L50_DSP1_RAM_INIT_FLAG	0x1
> +#define CS40L50_DSP1_CLOCK_DISABLE	0x80
> +#define CS40L50_DSP1_CLOCK_ENABLE	0x281
> +#define CS40L50_DSP_POLL_US		1000
> +#define CS40L50_DSP_TIMEOUT_COUNT	100
> +#define CS40L50_MBOX_POLL_US		5000
> +
> +/* Calibration */
> +#define CS40L50_DSP1_REDC_ESTIMATION		0x2802F7C
> +#define CS40L50_DSP1_F0_ESTIMATION		0x2802F84
> +#define CS40L50_DSP1_DYNAMIC_F0_ENABLE		0x2802F8C
> +#define CS40L50_DSP1_DYNAMIC_F0_THRESHOLD	0x2802F90
> +#define CS40L50_DSP1_F0_STORED			0x2805C00
> +#define CS40L50_DSP1_REDC_STORED		0x2805C04
> +#define CS40L50_DSP1_RE_EST_STATUS		0x3401B40
> +#define CS40L50_REDC_EST_DELAY_US	30000
> +#define CS40L50_F0_EST_DELAY_US		10000
> +
> +/* OWT */
> +#define CS40L50_DSP1_OWT_SIZE			0x2805C38
> +#define CS40L50_DSP1_OWT_NEXT			0x2805C3C
> +#define CS40L50_NUM_OF_OWT_WAVES		0x2805C40
> +#define CS40L50_DSP1_OWT_BASE			0x2805C34
> +#define CS40L50_MAX_PWLE_SECTIONS	126
> +#define CS40L50_CUSTOM_DATA_SIZE	2
> +#define CS40L50_WT_HEADER_PWLE_SIZE	12
> +#define CS40L50_WT_HEADER_DEFAULT_FLAGS	0x0000
> +#define CS40L50_WT_HEADER_OFFSET	3
> +#define CS40L50_WT_TYPE_PCM		8
> +#define CS40L50_WT_TYPE_PWLE		12
> +#define CS40L50_PCM_ID			0x0000
> +
> +/* GPIO */
> +#define CS40L50_DSP1_GPIO_BASE			0x2804140
> +#define CS40L50_GPIO_MAPPING_INVALID	0
> +#define CS40L50_GPI_DISABLE		0x1FF
> +#define CS40L50_BTN_NUM_MASK		GENMASK(14, 12)
> +#define CS40L50_BTN_EDGE_MASK		BIT(15)
> +
> +/* State */
> +#define CS40L50_DSP1_HALO_STATE				0x28021E0
> +#define CS40L50_HALO_STATE_BOOT_DONE		2
> +#define CS40L50_MIN_RESET_PULSE_WIDTH_US	1100
> +#define CS40L50_CP_READY_DELAY_US		2200
> +
> +/* Device */
> +#define CS40L50_DEVID			0x0
> +#define CS40L50_REVID			0x4
> +#define CS40L50_DEVID_A		0x40A50
> +#define CS40L50_REVID_B0	0xB0
> +
> +/* Write sequencer */
> +#define CS40L50_DSP1_POWER_ON_SEQ				0x280C320
> +#define CS40L50_PSEQ_MAX_WORDS				200
> +#define CS40L50_PSEQ_OP_MASK				GENMASK(23, 16)
> +#define CS40L50_PSEQ_OP_SHIFT				16
> +#define CS40L50_PSEQ_OP_WRITE_ADDR8			0x02
> +#define CS40L50_PSEQ_OP_WRITE_L16			0x04
> +#define CS40L50_PSEQ_OP_WRITE_H16			0x05
> +#define CS40L50_PSEQ_OP_WRITE_FULL			0x00
> +#define CS40L50_PSEQ_OP_WRITE_FULL_WORDS		3
> +#define CS40L50_PSEQ_OP_WRITE_X16_WORDS			2
> +#define CS40L50_PSEQ_OP_END				0xFF
> +#define CS40L50_PSEQ_OP_END_WORDS			1
> +#define CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_SHIFT	8
> +#define CS40L50_PSEQ_WRITE_FULL_UPPER_ADDR_SHIFT	16
> +#define CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_MASK		GENMASK(15, 0)
> +#define CS40L50_PSEQ_WRITE_FULL_UPPER_DATA_SHIFT	24
> +#define CS40L50_PSEQ_WRITE_FULL_LOWER_DATA_MASK		GENMASK(23, 0)
> +
> +enum cs40l50_wvfrm_bank {
> +	CS40L50_WVFRM_BANK_RAM,
> +	CS40L50_WVFRM_BANK_ROM,
> +	CS40L50_WVFRM_BANK_OWT,
> +	CS40L50_WVFRM_BANK_BUZ,
> +	CS40L50_WVFRM_BANK_NUM,
> +};
> +
> +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,
> +	CS40L50_NUM_IRQ
> +};
> +
> +union cs40l50_buzzgen {
> +	u32 words[3];
> +	struct {
> +		u32 freq;
> +		u32 level;
> +		u32 duration;
> +	} params;
> +};
> +
> +struct cs40l50_owt_header {
> +	u32 wvfrm_type;
> +	u32 offset;
> +	u32 data_size;
> +};
> +
> +struct cs40l50_irq {
> +	int irq;
> +	const char *name;
> +	irqreturn_t (*handler)(int irq, void *data);
> +};
> +
> +struct cs40l50_effect {
> +	int id;
> +	u32 trigger_index;
> +	enum cs40l50_wvfrm_bank wvfrm_bank;
> +	u32 mapping;
> +	struct list_head list;
> +};
> +
> +struct cs40l50_pseq_op {
> +	u16 offset;
> +	u8 operation;
> +	u32 words[3];
> +	u32 size;
> +	struct list_head list;
> +};
> +
> +struct cs40l50_private {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct cs_dsp dsp;
> +	struct mutex lock;
> +	struct gpio_desc *reset_gpio;
> +	struct input_dev *input;
> +	struct list_head effect_head;
> +	struct ff_effect *trigger_effect;
> +	struct workqueue_struct *vibe_workqueue;
> +	struct work_struct vibe_start_work;
> +	struct work_struct vibe_stop_work;
> +	struct regmap_irq_chip_data *irq_data;
> +	struct list_head pseq_op_head;
> +	int irq;
> +};
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50);
> +int cs40l50_remove(struct cs40l50_private *cs40l50);
> +
> +extern const struct regmap_config cs40l50_regmap;
> +
> +#endif /* __CS40L50_H__ */
> -- 
> 2.25.1
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[James Ogletree]

> On Aug 10, 2023, at 11:07 PM, Jeff LaBundy <jeff@labundy.com> wrote:
> 
> On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
>> Introduce support for Cirrus Logic Device CS40L50: a
>> haptics driver with waveform memory DSP and closed-loop
>> algorithms.
> 
> From my extremely naive point of view, some of the code that follows
> bares resemblance to the recently reviewed L26. My assumption is that
> these devices are different enough in nature to warrant completely
> different drivers; is that accurate?
> 
> One reason for asking is because the L26 driver included a cornucopia
> of power-management overhead, yet we see none of that here. Assuming
> both L50 and L26 are built around the same Halo DSP, why is there such
> a fundamental difference between the two in terms of power management?
> 
> To that end, how does this driver handle hibernation? Is hibernation
> not supported, or do we simply defer to the DSP? In the case of the
> latter, why is L50 given this freedom but not L26?

One key difference is that L50’s Halo Core DSP is self-booting; the firmware
is burned in and no firmware download is required. On L26, firmware
downloading is compulsory. This differentiates dealing with the DSP in the
two drivers, because the L50 driver does not need to do a look up every
time it reads or writes to a firmware control. The registers are all static.

Minor reasons are that they have different power supply configurations that
require different register settings, they have errata differences, and a different set
of exposed features (L50 being much more simplistic). I think taken cumulatively
these differences warrant separate drivers. Though, I will take Charles’
recommendation to factor out the similarities into a shared library that both L50
and L26 can use.

Let me know whether you disagree on the above points or have followup
questions.

With respect to power management, I did not think that that there was any merit
in itself in maintaining equality with L26’s approach, and I was inclined to accept
your reasoning for using retry logic over the runtime PM facilities (not that the
latter way is incorrect).

Regarding the need for I2S streaming support, signs point to maybe. I will
migrate this driver to MFD for V4.

> 
>> + return cs40l50_dsp_write(cs40l50, CS40L50_BST_LPMODE_SEL, CS40L50_DCM_LOW_POWER);
>> +}
>> +
>> +static int cs40l50_patch_firmware(struct cs40l50_private *cs40l50)
>> +{
>> + const struct firmware *bin = NULL, *wmfw = NULL;
>> + int error = 0;
>> +
>> + if (request_firmware(&bin, "cs40l50.bin", cs40l50->dev))
>> + dev_info(cs40l50->dev, "Could not request wavetable file: %d\n", error);
>> +
>> + if (request_firmware(&wmfw, "cs40l50.wmfw", cs40l50->dev))
>> + dev_info(cs40l50->dev, "Could not request firmware patch file: %d\n", error);
>> +
>> + if (wmfw) {
> 
> It is a much more common design pattern to bail if request_firmware() returns
> an error, than to proceed and check against the FW pointer remaining NULL.
> 
> Is this done because cs_dsp_power_up() must be called, with or without a wmfw
> or coefficient file being available?

I don’t think that cs_dsp_power_up() must be called in the case of non-existent .wmfw
and .bin files, so I will take your suggestion and optimize this function. I will also switch
to asynchronous firmware requesting for V4.

I will also incorporate the rest of your review comments not mentioned here in V4.
Thank you for the thorough review.

Regards,
James Ogletree

Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[Jeff LaBundy]

Hi James,

On Wed, Aug 16, 2023 at 09:02:26PM +0000, James Ogletree wrote:
> 
> 
> > On Aug 10, 2023, at 11:07 PM, Jeff LaBundy <jeff@labundy.com> wrote:
> > 
> > On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
> >> Introduce support for Cirrus Logic Device CS40L50: a
> >> haptics driver with waveform memory DSP and closed-loop
> >> algorithms.
> > 
> > From my extremely naive point of view, some of the code that follows
> > bares resemblance to the recently reviewed L26. My assumption is that
> > these devices are different enough in nature to warrant completely
> > different drivers; is that accurate?
> > 
> > One reason for asking is because the L26 driver included a cornucopia
> > of power-management overhead, yet we see none of that here. Assuming
> > both L50 and L26 are built around the same Halo DSP, why is there such
> > a fundamental difference between the two in terms of power management?
> > 
> > To that end, how does this driver handle hibernation? Is hibernation
> > not supported, or do we simply defer to the DSP? In the case of the
> > latter, why is L50 given this freedom but not L26?
> 
> One key difference is that L50’s Halo Core DSP is self-booting; the firmware
> is burned in and no firmware download is required. On L26, firmware
> downloading is compulsory. This differentiates dealing with the DSP in the
> two drivers, because the L50 driver does not need to do a look up every
> time it reads or writes to a firmware control. The registers are all static.

Interesting stuff; thanks for sharing that background information.

> 
> Minor reasons are that they have different power supply configurations that
> require different register settings, they have errata differences, and a different set
> of exposed features (L50 being much more simplistic). I think taken cumulatively
> these differences warrant separate drivers. Though, I will take Charles’
> recommendation to factor out the similarities into a shared library that both L50
> and L26 can use.
> 
> Let me know whether you disagree on the above points or have followup
> questions.

Makes sense to me.

> 
> With respect to power management, I did not think that that there was any merit
> in itself in maintaining equality with L26’s approach, and I was inclined to accept
> your reasoning for using retry logic over the runtime PM facilities (not that the
> latter way is incorrect).
> 
> Regarding the need for I2S streaming support, signs point to maybe. I will
> migrate this driver to MFD for V4.

MFD seems like the right (i.e. scalable) solution if A2H is on the roadmap.
In early L25 days, very early adopters simply asked for a sysfs control
exposed from the core (then LED !!) driver to enable/disable I2S streaming
mode, but this got hairy in case customers needed to control BCLK/Fs ratio,
bit depth, etc. during runtime.

From my naive point of view, maybe the solution looks something like this:

* drivers/mfd/cs40l50-i2c.c: I2C client
* drivers/mfd/cs40l50-spi.c: SPI client
* drivers/mfd/cs40l50-core.c: common tasks such as FW loading, OTP management, etc;
  perhaps L26 can leverage it as well
* drivers/input/misc/cs40l50-vibra.c: FF-specific support (name as you like, I just
  picked what seems to be most common)
* sound/soc/codecs/cs40l50-codec.c: codec-specific support

In case I have misunderstood, please let me know.

> 
> > 
> >> + return cs40l50_dsp_write(cs40l50, CS40L50_BST_LPMODE_SEL, CS40L50_DCM_LOW_POWER);
> >> +}
> >> +
> >> +static int cs40l50_patch_firmware(struct cs40l50_private *cs40l50)
> >> +{
> >> + const struct firmware *bin = NULL, *wmfw = NULL;
> >> + int error = 0;
> >> +
> >> + if (request_firmware(&bin, "cs40l50.bin", cs40l50->dev))
> >> + dev_info(cs40l50->dev, "Could not request wavetable file: %d\n", error);
> >> +
> >> + if (request_firmware(&wmfw, "cs40l50.wmfw", cs40l50->dev))
> >> + dev_info(cs40l50->dev, "Could not request firmware patch file: %d\n", error);
> >> +
> >> + if (wmfw) {
> > 
> > It is a much more common design pattern to bail if request_firmware() returns
> > an error, than to proceed and check against the FW pointer remaining NULL.
> > 
> > Is this done because cs_dsp_power_up() must be called, with or without a wmfw
> > or coefficient file being available?
> 
> I don’t think that cs_dsp_power_up() must be called in the case of non-existent .wmfw
> and .bin files, so I will take your suggestion and optimize this function. I will also switch
> to asynchronous firmware requesting for V4.

Since L50 can work out of the box without FW, another option is to follow the
approach used by some codec drivers and wait to load FW until the first request
to stream I2S, and/or trigger an FF effect in this case. By that point, rootfs
has been available for some time and request_firmware() will succeed.

This is advantageous because even though request_firmware_nowait() can solve
the deadlock problem for FW loaded at probe, it could still return -ENOENT right
away on some platforms. The disadvantage is that the first effect would be very
delayed due to the overhead of transferring several kB of FW over I2C for the
first time. That's OK for audio applications, but not haptics where delay normally
must be within single-digit ms.

Ultimately, I think the right approach is to look for FW from probe as you have
done, but use request_firmware_nowait(). However, I recommend structuring the
driver such that rip-up would be minimal in case you had to retry FW loading from
the first FF trigger, to support a customer whose rootfs is not available at probe.
In such a case, the customer could work around the first-time latency problem by
triggering a dummy effect early in their boot (e.g. init.rc).

> 
> I will also incorporate the rest of your review comments not mentioned here in V4.
> Thank you for the thorough review.

Thank you for the productive discussion!

> 
> Regards,
> James Ogletree

Kind regards,
Jeff LaBundy