[PATCH v5 0/5] Add support for CS40L50

[PATCH v5 0/5] Add support for CS40L50

[James Ogletree]

There is a massive delta from V4, so only the highlgihts are
mentioned below. I intend for this version to address all
feedback to-date, so if you find something left unaddressed
then please let me know.

Changes in v5:
- Added a codec sub-device to support I2S streaming
- Moved write sequencer code from cirrus_haptics to cs_dsp
- Reverted cirrus_haptics library; future Cirrus input
  drivers will export and utilize cs40l50_vibra functions
- Added more comments
- Many small stylistic and logical improvements

Changes in v4:
- Moved from Input to MFD
- Moved common Cirrus haptic functions to a library
- Incorporated runtime PM framework
- Many style improvements

Changes in v3:
- YAML formatting corrections
- Fixed typo in MAINTAINERS
- Used generic node name "haptic-driver"
- Fixed probe error code paths
- Switched to "sizeof(*)"
- Removed tree reference in MAINTAINERS

Changes in v2:
- Fixed checkpatch warnings

James Ogletree (5):
  firmware: cs_dsp: Add write sequencer interface
  dt-bindings: input: cirrus,cs40l50: Add initial DT binding
  mfd: cs40l50: Add support for CS40L50 core driver
  Input: cs40l50 - Add support for the CS40L50 haptic driver
  ASoC: cs40l50: Support I2S streaming to CS40L50

 .../bindings/input/cirrus,cs40l50.yaml        |  70 +++
 MAINTAINERS                                   |  12 +
 drivers/firmware/cirrus/cs_dsp.c              | 261 ++++++++
 drivers/input/misc/Kconfig                    |  10 +
 drivers/input/misc/Makefile                   |   1 +
 drivers/input/misc/cs40l50-vibra.c            | 572 ++++++++++++++++++
 drivers/mfd/Kconfig                           |  30 +
 drivers/mfd/Makefile                          |   4 +
 drivers/mfd/cs40l50-core.c                    | 536 ++++++++++++++++
 drivers/mfd/cs40l50-i2c.c                     |  69 +++
 drivers/mfd/cs40l50-spi.c                     |  69 +++
 include/linux/firmware/cirrus/cs_dsp.h        |  28 +
 include/linux/mfd/cs40l50.h                   | 128 ++++
 sound/soc/codecs/Kconfig                      |  11 +
 sound/soc/codecs/Makefile                     |   2 +
 sound/soc/codecs/cs40l50-codec.c              | 304 ++++++++++
 16 files changed, 2107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
 create mode 100644 drivers/input/misc/cs40l50-vibra.c
 create mode 100644 drivers/mfd/cs40l50-core.c
 create mode 100644 drivers/mfd/cs40l50-i2c.c
 create mode 100644 drivers/mfd/cs40l50-spi.c
 create mode 100644 include/linux/mfd/cs40l50.h
 create mode 100644 sound/soc/codecs/cs40l50-codec.c

-- 
2.25.1

[PATCH v5 1/5] firmware: cs_dsp: Add write sequencer interface

[James Ogletree]

A write sequencer is a sequence of register addresses
and values executed by some Cirrus DSPs following
power-up or exit from hibernation, used for avoiding
the overhead of bus transactions.

Add support for Cirrus drivers to update or add to a
write sequencer present in firmware.

Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
---
 drivers/firmware/cirrus/cs_dsp.c       | 261 +++++++++++++++++++++++++
 include/linux/firmware/cirrus/cs_dsp.h |  28 +++
 2 files changed, 289 insertions(+)

diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c
index 79d4254d1f9b..31a999f42e84 100644
--- a/drivers/firmware/cirrus/cs_dsp.c
+++ b/drivers/firmware/cirrus/cs_dsp.c
@@ -275,6 +275,15 @@
 #define HALO_MPU_VIO_ERR_SRC_MASK           0x00007fff
 #define HALO_MPU_VIO_ERR_SRC_SHIFT                   0
 
+/*
+ * Write Sequencer
+ */
+#define WSEQ_OP_FULL_WORDS	3
+#define WSEQ_OP_X16_WORDS	2
+#define WSEQ_OP_END_WORDS	1
+#define WSEQ_OP_UNLOCK_WORDS	1
+#define WSEQ_END_OF_SCRIPT	0xFFFFFF
+
 struct cs_dsp_ops {
 	bool (*validate_version)(struct cs_dsp *dsp, unsigned int version);
 	unsigned int (*parse_sizes)(struct cs_dsp *dsp,
@@ -2233,6 +2242,111 @@ static int cs_dsp_create_name(struct cs_dsp *dsp)
 	return 0;
 }
 
+struct cs_dsp_wseq_op {
+	struct list_head list;
+	u32 words[3];
+	u32 address;
+	u32 data;
+	u16 offset;
+	u8 operation;
+};
+
+static int cs_dsp_populate_wseq(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq)
+{
+	struct cs_dsp_wseq_op *op = NULL;
+	struct cs_dsp_chunk ch;
+	int i, num_words, ret;
+	u32 *words;
+
+	if (wseq->size <= 0 || !wseq->reg)
+		return -EINVAL;
+
+	words = kcalloc(wseq->size, sizeof(u32), GFP_KERNEL);
+	if (!words)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&wseq->ops);
+
+	ret = regmap_raw_read(dsp->regmap, wseq->reg, words,
+			      wseq->size * sizeof(u32));
+	if (ret)
+		goto err_free;
+
+	ch = cs_dsp_chunk(words, wseq->size * sizeof(u32));
+
+	for (i = 0; i < wseq->size; i += num_words) {
+		op = devm_kzalloc(dsp->dev, sizeof(*op), GFP_KERNEL);
+		if (!op) {
+			ret = -ENOMEM;
+			goto err_free;
+		}
+
+		op->offset = ch.bytes;
+		op->operation = cs_dsp_chunk_read(&ch, 8);
+
+		switch (op->operation) {
+		case CS_DSP_WSEQ_END:
+			num_words = WSEQ_OP_END_WORDS;
+			break;
+		case CS_DSP_WSEQ_UNLOCK:
+			num_words = WSEQ_OP_UNLOCK_WORDS;
+			op->address = 0;
+			op->data = cs_dsp_chunk_read(&ch, 16);
+			break;
+		case CS_DSP_WSEQ_ADDR8:
+		case CS_DSP_WSEQ_H16:
+		case CS_DSP_WSEQ_L16:
+			num_words = WSEQ_OP_X16_WORDS;
+			op->address = cs_dsp_chunk_read(&ch, 24);
+			op->data = cs_dsp_chunk_read(&ch, 16);
+			break;
+		case CS_DSP_WSEQ_FULL:
+			num_words = WSEQ_OP_FULL_WORDS;
+			op->address = cs_dsp_chunk_read(&ch, 32);
+			op->data = cs_dsp_chunk_read(&ch, 32);
+			break;
+		default:
+			ret = -EINVAL;
+			cs_dsp_err(dsp, "Unsupported op: %u\n", op->operation);
+			goto err_free;
+		}
+
+		list_add(&op->list, &wseq->ops);
+
+		if (op->operation == CS_DSP_WSEQ_END)
+			break;
+	}
+
+	if (op && op->operation != CS_DSP_WSEQ_END)
+		ret = -ENOENT;
+err_free:
+	kfree(words);
+
+	return ret;
+}
+
+/**
+ * cs_dsp_wseq_init() - Initialize write sequences contained within the loaded DSP firmware
+ * @dsp: pointer to DSP structure
+ * @wseqs: list of write sequences to initialize
+ * @num_wseqs: number of write sequences to initialize
+ *
+ * Return: Zero for success, a negative number on error.
+ */
+int cs_dsp_wseq_init(struct cs_dsp *dsp, struct cs_dsp_wseq *wseqs, unsigned int num_wseqs)
+{
+	int i, ret;
+
+	for (i = 0; i < num_wseqs; i++) {
+		ret = cs_dsp_populate_wseq(dsp, &wseqs[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs_dsp_wseq_init);
+
 static int cs_dsp_common_init(struct cs_dsp *dsp)
 {
 	int ret;
@@ -3339,6 +3453,153 @@ int cs_dsp_chunk_read(struct cs_dsp_chunk *ch, int nbits)
 }
 EXPORT_SYMBOL_NS_GPL(cs_dsp_chunk_read, FW_CS_DSP);
 
+static struct cs_dsp_wseq_op *cs_dsp_wseq_find_op(u8 op_code, u32 addr,
+						  struct list_head *wseq_ops)
+{
+	struct cs_dsp_wseq_op *op;
+
+	list_for_each_entry(op, wseq_ops, list) {
+		if (op->operation == op_code && op->address == addr)
+			return op;
+	}
+
+	return NULL;
+}
+
+/**
+ * cs_dsp_wseq_write() - Add or update an entry in a write sequence
+ * @dsp: Pointer to a DSP structure
+ * @wseq: Write sequence to write to
+ * @addr: Address of the register to be written to
+ * @data: Data to be written
+ * @update: If true, searches for the first entry in the Write Sequencer with
+ * the same address and op_code, and replaces it. If false, creates a new entry
+ * at the tail.
+ * @op_code: The type of operation of the new entry
+ *
+ * This function formats register address and value pairs into the format
+ * required for write sequence entries, and either updates or adds the
+ * new entry into the write sequence.
+ *
+ * Return: Zero for success, a negative number on error.
+ */
+int cs_dsp_wseq_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
+		      u32 addr, u32 data, bool update, u8 op_code)
+{
+	struct cs_dsp_wseq_op *op_end, *op_new;
+	struct cs_dsp_chunk ch;
+	u32 wseq_bytes;
+	int new_op_size, ret;
+
+	if (update) {
+		op_new = cs_dsp_wseq_find_op(op_code, addr, &wseq->ops);
+		if (!op_new)
+			return -EINVAL;
+	} else {
+		op_new = devm_kzalloc(dsp->dev, sizeof(*op_new), GFP_KERNEL);
+		if (!op_new)
+			return -ENOMEM;
+
+		op_new->operation = op_code;
+		op_new->address = addr;
+	}
+
+	op_new->data = data;
+
+	ch = cs_dsp_chunk((void *) op_new->words,
+			  WSEQ_OP_FULL_WORDS * sizeof(u32));
+	cs_dsp_chunk_write(&ch, 8, op_new->operation);
+	switch (op_code) {
+	case CS_DSP_WSEQ_FULL:
+		cs_dsp_chunk_write(&ch, 32, op_new->address);
+		cs_dsp_chunk_write(&ch, 32, op_new->data);
+		break;
+	case CS_DSP_WSEQ_L16:
+	case CS_DSP_WSEQ_H16:
+		cs_dsp_chunk_write(&ch, 24, op_new->address);
+		cs_dsp_chunk_write(&ch, 16, op_new->data);
+		break;
+	default:
+		ret = -EINVAL;
+		goto op_new_free;
+	}
+
+	new_op_size = cs_dsp_chunk_bytes(&ch);
+
+	op_end = cs_dsp_wseq_find_op(CS_DSP_WSEQ_END, 0, &wseq->ops);
+	if (!op_end) {
+		cs_dsp_err(dsp, "Missing write sequencer list terminator\n");
+		ret = -EINVAL;
+		goto op_new_free;
+	}
+
+	wseq_bytes = wseq->size * sizeof(u32);
+
+	if (wseq_bytes - op_end->offset < new_op_size) {
+		cs_dsp_err(dsp, "Not enough memory in Write Sequencer for entry\n");
+		ret = -ENOMEM;
+		goto op_new_free;
+	}
+
+	if (!update) {
+		op_new->offset = op_end->offset;
+		op_end->offset += new_op_size;
+	}
+
+	ret = regmap_raw_write(dsp->regmap, wseq->reg + op_new->offset,
+			       op_new->words, new_op_size);
+	if (ret)
+		goto op_new_free;
+
+	if (!update) {
+		ret = regmap_write(dsp->regmap, wseq->reg + op_end->offset,
+				   WSEQ_END_OF_SCRIPT);
+		if (ret)
+			goto op_new_free;
+
+		list_add(&op_new->list, &wseq->ops);
+	}
+
+	return 0;
+
+op_new_free:
+	devm_kfree(dsp->dev, op_new);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cs_dsp_wseq_write);
+
+/**
+ * cs_dsp_wseq_multi_write() - Add or update multiple entries in the write sequence
+ * @dsp: Pointer to a DSP structure
+ * @wseq: Write sequence to write to
+ * @reg_seq: List of address-data pairs
+ * @num_regs: Number of address-data pairs
+ * @update: If true, searches for the first entry in the write sequence with the same
+ * address and op code, and replaces it. If false, creates a new entry at the tail.
+ * @op_code: The types of operations of the new entries
+ *
+ * This function calls cs_dsp_wseq_write() for multiple address-data pairs.
+ *
+ * Return: Zero for success, a negative number on error.
+ */
+int cs_dsp_wseq_multi_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
+			    const struct reg_sequence *reg_seq,
+			    int num_regs, bool update, u8 op_code)
+{
+	int ret, i;
+
+	for (i = 0; i < num_regs; i++) {
+		ret = cs_dsp_wseq_write(dsp, wseq, reg_seq[i].reg,
+					reg_seq[i].def, update, op_code);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs_dsp_wseq_multi_write);
+
 MODULE_DESCRIPTION("Cirrus Logic DSP Support");
 MODULE_AUTHOR("Simon Trimmer <simont@opensource.cirrus.com>");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h
index 29cd11d5a3cf..d674fc061e9d 100644
--- a/include/linux/firmware/cirrus/cs_dsp.h
+++ b/include/linux/firmware/cirrus/cs_dsp.h
@@ -42,6 +42,16 @@
 #define CS_DSP_ACKED_CTL_MIN_VALUE           0
 #define CS_DSP_ACKED_CTL_MAX_VALUE           0xFFFFFF
 
+/*
+ * Write sequencer operation codes
+ */
+#define CS_DSP_WSEQ_FULL	0x00
+#define CS_DSP_WSEQ_ADDR8	0x02
+#define CS_DSP_WSEQ_L16		0x04
+#define CS_DSP_WSEQ_H16		0x05
+#define CS_DSP_WSEQ_UNLOCK	0xFD
+#define CS_DSP_WSEQ_END		0xFF
+
 /**
  * struct cs_dsp_region - Describes a logical memory region in DSP address space
  * @type:	Memory region type
@@ -107,6 +117,18 @@ struct cs_dsp_coeff_ctl {
 struct cs_dsp_ops;
 struct cs_dsp_client_ops;
 
+/**
+ * struct cs_dsp_wseq - Describes a write sequence
+ * @reg:	Address of the head of the write sequence register
+ * @size:	Size of the write sequence in words
+ * @ops:	Operations contained within the write sequence
+ */
+struct cs_dsp_wseq {
+	unsigned int reg;
+	unsigned int size;
+	struct list_head ops;
+};
+
 /**
  * struct cs_dsp - Configuration and state of a Cirrus Logic DSP
  * @name:		The name of the DSP instance
@@ -254,6 +276,12 @@ struct cs_dsp_alg_region *cs_dsp_find_alg_region(struct cs_dsp *dsp,
 						 int type, unsigned int id);
 
 const char *cs_dsp_mem_region_name(unsigned int type);
+int cs_dsp_wseq_init(struct cs_dsp *dsp, struct cs_dsp_wseq *wseqs, unsigned int num_wseqs);
+int cs_dsp_wseq_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq, u32 addr, u32 data,
+		      bool update, u8 op_code);
+int cs_dsp_wseq_multi_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
+			    const struct reg_sequence *reg_seq,
+			    int num_regs, bool update, u8 op_code);
 
 /**
  * struct cs_dsp_chunk - Describes a buffer holding data formatted for the DSP
-- 
2.25.1

Re: [PATCH v5 1/5] firmware: cs_dsp: Add write sequencer interface

[Charles Keepax]

On Thu, Jan 04, 2024 at 10:36:34PM +0000, James Ogletree wrote:
> A write sequencer is a sequence of register addresses
> and values executed by some Cirrus DSPs following
> power-up or exit from hibernation, used for avoiding
> the overhead of bus transactions.
> 
> Add support for Cirrus drivers to update or add to a
> write sequencer present in firmware.
> 
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
>  drivers/firmware/cirrus/cs_dsp.c       | 261 +++++++++++++++++++++++++
>  include/linux/firmware/cirrus/cs_dsp.h |  28 +++
>  2 files changed, 289 insertions(+)
> 
> diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c
> index 79d4254d1f9b..31a999f42e84 100644
> --- a/drivers/firmware/cirrus/cs_dsp.c
> +++ b/drivers/firmware/cirrus/cs_dsp.c
> @@ -275,6 +275,15 @@
>  #define HALO_MPU_VIO_ERR_SRC_MASK           0x00007fff
>  #define HALO_MPU_VIO_ERR_SRC_SHIFT                   0
>  
> +/*
> + * Write Sequencer
> + */
> +#define WSEQ_OP_FULL_WORDS	3
> +#define WSEQ_OP_X16_WORDS	2
> +#define WSEQ_OP_END_WORDS	1
> +#define WSEQ_OP_UNLOCK_WORDS	1
> +#define WSEQ_END_OF_SCRIPT	0xFFFFFF
> +
>  struct cs_dsp_ops {
>  	bool (*validate_version)(struct cs_dsp *dsp, unsigned int version);
>  	unsigned int (*parse_sizes)(struct cs_dsp *dsp,
> @@ -2233,6 +2242,111 @@ static int cs_dsp_create_name(struct cs_dsp *dsp)
>  	return 0;
>  }
>  
> +struct cs_dsp_wseq_op {
> +	struct list_head list;
> +	u32 words[3];
> +	u32 address;
> +	u32 data;
> +	u16 offset;
> +	u8 operation;
> +};
> +
> +static int cs_dsp_populate_wseq(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq)
> +{
> +	struct cs_dsp_wseq_op *op = NULL;
> +	struct cs_dsp_chunk ch;
> +	int i, num_words, ret;
> +	u32 *words;
> +
> +	if (wseq->size <= 0 || !wseq->reg)
> +		return -EINVAL;

I would be tempted to give this an error message.

> +
> +	words = kcalloc(wseq->size, sizeof(u32), GFP_KERNEL);
> +	if (!words)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&wseq->ops);
> +
> +	ret = regmap_raw_read(dsp->regmap, wseq->reg, words,
> +			      wseq->size * sizeof(u32));
> +	if (ret)
> +		goto err_free;
> +
> +	ch = cs_dsp_chunk(words, wseq->size * sizeof(u32));
> +
> +	for (i = 0; i < wseq->size; i += num_words) {

Can just drop num_words and i, and just do:

	while(!cs_dsp_chunk_end(&ch)) {

Also allows you to drop the length defines for each OP.

> +		op = devm_kzalloc(dsp->dev, sizeof(*op), GFP_KERNEL);
> +		if (!op) {
> +			ret = -ENOMEM;
> +			goto err_free;
> +		}
> +
> +		op->offset = ch.bytes;

Use cs_dsp_chunk_bytes, cleaner to not access the internals
directly incase we need to refactor them at some point.

> +		op->operation = cs_dsp_chunk_read(&ch, 8);
> +
> +		switch (op->operation) {
> +		case CS_DSP_WSEQ_END:
> +			num_words = WSEQ_OP_END_WORDS;
> +			break;
> +		case CS_DSP_WSEQ_UNLOCK:
> +			num_words = WSEQ_OP_UNLOCK_WORDS;
> +			op->address = 0;
> +			op->data = cs_dsp_chunk_read(&ch, 16);
> +			break;
> +		case CS_DSP_WSEQ_ADDR8:
> +		case CS_DSP_WSEQ_H16:
> +		case CS_DSP_WSEQ_L16:
> +			num_words = WSEQ_OP_X16_WORDS;
> +			op->address = cs_dsp_chunk_read(&ch, 24);
> +			op->data = cs_dsp_chunk_read(&ch, 16);
> +			break;
> +		case CS_DSP_WSEQ_FULL:
> +			num_words = WSEQ_OP_FULL_WORDS;
> +			op->address = cs_dsp_chunk_read(&ch, 32);
> +			op->data = cs_dsp_chunk_read(&ch, 32);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			cs_dsp_err(dsp, "Unsupported op: %u\n", op->operation);
> +			goto err_free;
> +		}
> +
> +		list_add(&op->list, &wseq->ops);
> +
> +		if (op->operation == CS_DSP_WSEQ_END)
> +			break;
> +	}
> +
> +	if (op && op->operation != CS_DSP_WSEQ_END)
> +		ret = -ENOENT;

This definitely wants an error message, since this indicates the
firmware is in a broken state, or the buffer passed in was not a
write sequence.

> +err_free:
> +	kfree(words);
> +
> +	return ret;
> +}
> +
> +/**
> + * cs_dsp_wseq_init() - Initialize write sequences contained within the loaded DSP firmware
> + * @dsp: pointer to DSP structure
> + * @wseqs: list of write sequences to initialize
> + * @num_wseqs: number of write sequences to initialize
> + *
> + * Return: Zero for success, a negative number on error.
> + */
> +int cs_dsp_wseq_init(struct cs_dsp *dsp, struct cs_dsp_wseq *wseqs, unsigned int num_wseqs)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < num_wseqs; i++) {
> +		ret = cs_dsp_populate_wseq(dsp, &wseqs[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs_dsp_wseq_init);
> +

This location in the middle of the file is a bit weird, would be
nicer to keep all the wseq stuff together move this down to the
bottom of the file with the other functions.

>  static int cs_dsp_common_init(struct cs_dsp *dsp)
>  {
>  	int ret;
> @@ -3339,6 +3453,153 @@ int cs_dsp_chunk_read(struct cs_dsp_chunk *ch, int nbits)
>  }
>  EXPORT_SYMBOL_NS_GPL(cs_dsp_chunk_read, FW_CS_DSP);
>  
> +static struct cs_dsp_wseq_op *cs_dsp_wseq_find_op(u8 op_code, u32 addr,
> +						  struct list_head *wseq_ops)
> +{
> +	struct cs_dsp_wseq_op *op;
> +
> +	list_for_each_entry(op, wseq_ops, list) {
> +		if (op->operation == op_code && op->address == addr)
> +			return op;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * cs_dsp_wseq_write() - Add or update an entry in a write sequence
> + * @dsp: Pointer to a DSP structure
> + * @wseq: Write sequence to write to
> + * @addr: Address of the register to be written to
> + * @data: Data to be written
> + * @update: If true, searches for the first entry in the Write Sequencer with
> + * the same address and op_code, and replaces it. If false, creates a new entry
> + * at the tail.
> + * @op_code: The type of operation of the new entry
> + *
> + * This function formats register address and value pairs into the format
> + * required for write sequence entries, and either updates or adds the
> + * new entry into the write sequence.
> + *
> + * Return: Zero for success, a negative number on error.
> + */
> +int cs_dsp_wseq_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
> +		      u32 addr, u32 data, bool update, u8 op_code)

Feels weird to have the op_code after the update flag in the
order of arguments. addr, data and op_code are all parts of
the new entry they should go together. Also be nice for the API
to be consistent in the order it uses them, wseq_find_op is
op_code, addr.

> +{
> +	struct cs_dsp_wseq_op *op_end, *op_new;
> +	struct cs_dsp_chunk ch;
> +	u32 wseq_bytes;
> +	int new_op_size, ret;
> +
> +	if (update) {
> +		op_new = cs_dsp_wseq_find_op(op_code, addr, &wseq->ops);
> +		if (!op_new)
> +			return -EINVAL;

This could also have an error message.

> +	} else {

I would be tempted to pull the init of op_end up here like:

		op_end = cs_dsp_wseq_find_op(CS_DSP_WSEQ_END, 0, &wseq->ops);
		if (!op_end) {
			cs_dsp_err(dsp, "Missing write sequencer list terminator\n");
			return -EINVAL;
		}

> +		op_new = devm_kzalloc(dsp->dev, sizeof(*op_new), GFP_KERNEL);
> +		if (!op_new)
> +			return -ENOMEM;
> +
> +		op_new->operation = op_code;
> +		op_new->address = addr;

And:

		op_new->offset = op_end->offset;
> +	}
> +
> +	op_new->data = data;
> +
> +	ch = cs_dsp_chunk((void *) op_new->words,
> +			  WSEQ_OP_FULL_WORDS * sizeof(u32));

Since this is the only place you use op->words make it a local
variable, its only 3 ints on the stack and it saves having 3
redundant ints in every op in the list.

> +	cs_dsp_chunk_write(&ch, 8, op_new->operation);
> +	switch (op_code) {
> +	case CS_DSP_WSEQ_FULL:
> +		cs_dsp_chunk_write(&ch, 32, op_new->address);
> +		cs_dsp_chunk_write(&ch, 32, op_new->data);
> +		break;
> +	case CS_DSP_WSEQ_L16:
> +	case CS_DSP_WSEQ_H16:
> +		cs_dsp_chunk_write(&ch, 24, op_new->address);
> +		cs_dsp_chunk_write(&ch, 16, op_new->data);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto op_new_free;

This also could have an error message, in general I would
recommend have error messages for places where handling
arguments from the user of the API. It is much more friendly
for other developers, since they get immediate feedback if
when they do something wrong when using the API.

> +	}
> +

With op_end pre-initialised this bit becomes:

> +	new_op_size = cs_dsp_chunk_bytes(&ch);
> +
> +	wseq_bytes = wseq->size * sizeof(u32);
> +
> +	if (wseq_bytes - op_end->offset < new_op_size) {
> +		cs_dsp_err(dsp, "Not enough memory in Write Sequencer for entry\n");
> +		ret = -ENOMEM;
> +		goto op_new_free;
> +	}
> +
> +	ret = regmap_raw_write(dsp->regmap, wseq->reg + op_new->offset,
> +			       op_new->words, new_op_size);
> +	if (ret)
> +		goto op_new_free;
> +
> +	if (!update) {

Then pull the shift of op_end->offset into here:

		op_end->offset += new_op_size;

> +		ret = regmap_write(dsp->regmap, wseq->reg + op_end->offset,
> +				   WSEQ_END_OF_SCRIPT);
> +		if (ret)
> +			goto op_new_free;
> +
> +		list_add(&op_new->list, &wseq->ops);
> +	}
> +
> +	return 0;
> +
> +op_new_free:
> +	devm_kfree(dsp->dev, op_new);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cs_dsp_wseq_write);
> +
> +/**
> + * cs_dsp_wseq_multi_write() - Add or update multiple entries in the write sequence
> + * @dsp: Pointer to a DSP structure
> + * @wseq: Write sequence to write to
> + * @reg_seq: List of address-data pairs
> + * @num_regs: Number of address-data pairs
> + * @update: If true, searches for the first entry in the write sequence with the same
> + * address and op code, and replaces it. If false, creates a new entry at the tail.
> + * @op_code: The types of operations of the new entries
> + *
> + * This function calls cs_dsp_wseq_write() for multiple address-data pairs.
> + *
> + * Return: Zero for success, a negative number on error.
> + */
> +int cs_dsp_wseq_multi_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
> +			    const struct reg_sequence *reg_seq,
> +			    int num_regs, bool update, u8 op_code)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < num_regs; i++) {
> +		ret = cs_dsp_wseq_write(dsp, wseq, reg_seq[i].reg,
> +					reg_seq[i].def, update, op_code);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs_dsp_wseq_multi_write);
> +
>  MODULE_DESCRIPTION("Cirrus Logic DSP Support");
>  MODULE_AUTHOR("Simon Trimmer <simont@opensource.cirrus.com>");
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h
> index 29cd11d5a3cf..d674fc061e9d 100644
> --- a/include/linux/firmware/cirrus/cs_dsp.h
> +++ b/include/linux/firmware/cirrus/cs_dsp.h
> @@ -42,6 +42,16 @@
>  #define CS_DSP_ACKED_CTL_MIN_VALUE           0
>  #define CS_DSP_ACKED_CTL_MAX_VALUE           0xFFFFFF
>  
> +/*
> + * Write sequencer operation codes
> + */
> +#define CS_DSP_WSEQ_FULL	0x00
> +#define CS_DSP_WSEQ_ADDR8	0x02
> +#define CS_DSP_WSEQ_L16		0x04
> +#define CS_DSP_WSEQ_H16		0x05
> +#define CS_DSP_WSEQ_UNLOCK	0xFD
> +#define CS_DSP_WSEQ_END		0xFF
> +
>  /**
>   * struct cs_dsp_region - Describes a logical memory region in DSP address space
>   * @type:	Memory region type
> @@ -107,6 +117,18 @@ struct cs_dsp_coeff_ctl {
>  struct cs_dsp_ops;
>  struct cs_dsp_client_ops;
>  
> +/**
> + * struct cs_dsp_wseq - Describes a write sequence
> + * @reg:	Address of the head of the write sequence register
> + * @size:	Size of the write sequence in words

The only user that wants the size in words is the loop counter
that can be deleted. Is there any reason not to specify the
size in bytes?

> + * @ops:	Operations contained within the write sequence
> + */
> +struct cs_dsp_wseq {
> +	unsigned int reg;
> +	unsigned int size;
> +	struct list_head ops;
> +};
> +
>  /**
>   * struct cs_dsp - Configuration and state of a Cirrus Logic DSP
>   * @name:		The name of the DSP instance
> @@ -254,6 +276,12 @@ struct cs_dsp_alg_region *cs_dsp_find_alg_region(struct cs_dsp *dsp,
>  						 int type, unsigned int id);
>  
>  const char *cs_dsp_mem_region_name(unsigned int type);
> +int cs_dsp_wseq_init(struct cs_dsp *dsp, struct cs_dsp_wseq *wseqs, unsigned int num_wseqs);
> +int cs_dsp_wseq_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq, u32 addr, u32 data,
> +		      bool update, u8 op_code);
> +int cs_dsp_wseq_multi_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
> +			    const struct reg_sequence *reg_seq,
> +			    int num_regs, bool update, u8 op_code);
>  

This is also pretty spaced through the file, leave the defines
where they are, but gather the struct and the funcs and move
them to the bottom of the file in a group. Keeps all the API
together when someone is looking it up.

>  /**
>   * struct cs_dsp_chunk - Describes a buffer holding data formatted for the DSP
> -- 
> 2.25.1
> 

Overall my only other concern is still the register based API
rather than control based. I guess there is some precident
with the compressed stuff although that is at least taking
addresses from the DSP and translating them into register
addresses so the host can use them.

Richard is off today, but back on Monday let me discuss with
him then and we should have a chat too.

Thanks,
Charles

[PATCH v5 2/5] dt-bindings: input: cirrus,cs40l50: Add initial DT binding

[James Ogletree]

The CS40L50 is a haptic driver with waveform memory,
integrated DSP, and closed-loop algorithms.

Add a YAML DT binding document for this device.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
---
 .../bindings/input/cirrus,cs40l50.yaml        | 70 +++++++++++++++++++
 MAINTAINERS                                   |  8 +++
 2 files changed, 78 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..6a5bdafed56b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
@@ -0,0 +1,70 @@
+# 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,
+  integrated DSP, and closed-loop algorithms.
+
+properties:
+  compatible:
+    enum:
+      - cirrus,cs40l50
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  va-supply:
+    description: Power supply for internal analog circuits.
+
+  vp-supply:
+    description: Power supply for always-on circuits.
+
+  vio-supply:
+    description: Power supply for digital input/output.
+
+  vamp-supply:
+    description: Power supply for the Class D amplifier.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - reset-gpios
+  - vp-supply
+  - vio-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.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>;
+        vp-supply = <&vreg>;
+        vio-supply = <&vreg>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index dd5de540ec0b..b71017a187f8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4933,6 +4933,14 @@ F:	sound/pci/hda/cs*
 F:	sound/pci/hda/hda_cs_dsp_ctl.*
 F:	sound/soc/codecs/cs*
 
+CIRRUS LOGIC HAPTIC DRIVERS
+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
+
 CIRRUS LOGIC DSP FIRMWARE DRIVER
 M:	Simon Trimmer <simont@opensource.cirrus.com>
 M:	Charles Keepax <ckeepax@opensource.cirrus.com>
-- 
2.25.1

[PATCH v5 3/5] mfd: cs40l50: Add support for CS40L50 core driver

[James Ogletree]

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

The MFD component registers and initializes the device.

Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
---
 MAINTAINERS                 |   2 +
 drivers/mfd/Kconfig         |  30 ++
 drivers/mfd/Makefile        |   4 +
 drivers/mfd/cs40l50-core.c  | 536 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/cs40l50-i2c.c   |  69 +++++
 drivers/mfd/cs40l50-spi.c   |  69 +++++
 include/linux/mfd/cs40l50.h | 128 +++++++++
 7 files changed, 838 insertions(+)
 create mode 100644 drivers/mfd/cs40l50-core.c
 create mode 100644 drivers/mfd/cs40l50-i2c.c
 create mode 100644 drivers/mfd/cs40l50-spi.c
 create mode 100644 include/linux/mfd/cs40l50.h

diff --git a/MAINTAINERS b/MAINTAINERS
index b71017a187f8..69a9e0a3b968 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4940,6 +4940,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/mfd/cs40l*
+F:	include/linux/mfd/cs40l*
 
 CIRRUS LOGIC DSP FIRMWARE DRIVER
 M:	Simon Trimmer <simont@opensource.cirrus.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 90ce58fd629e..6273c255f107 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2241,6 +2241,36 @@ config MCP_UCB1200_TS
 
 endmenu
 
+config MFD_CS40L50_CORE
+	tristate
+	select MFD_CORE
+	select FW_CS_DSP
+	select REGMAP_IRQ
+
+config MFD_CS40L50_I2C
+	tristate "Cirrus Logic CS40L50 (I2C)"
+	select REGMAP_I2C
+	select MFD_CS40L50_CORE
+	depends on I2C
+	help
+	  Select this to support the Cirrus Logic CS40L50 Haptic
+	  Driver over I2C.
+
+	  This driver can be built as a module. If built as a module it will be
+	  called "cs40l50-i2c".
+
+config MFD_CS40L50_SPI
+	tristate "Cirrus Logic CS40L50 (SPI)"
+	select REGMAP_SPI
+	select MFD_CS40L50_CORE
+	depends on SPI
+	help
+	  Select this to support the Cirrus Logic CS40L50 Haptic
+	  Driver over SPI.
+
+	  This driver can be built as a module. If built as a module it will be
+	  called "cs40l50-spi".
+
 config MFD_VEXPRESS_SYSREG
 	tristate "Versatile Express System Registers"
 	depends on VEXPRESS_CONFIG && GPIOLIB
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..a8d18ba155d0 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -88,6 +88,10 @@ obj-$(CONFIG_MFD_MADERA)	+= madera.o
 obj-$(CONFIG_MFD_MADERA_I2C)	+= madera-i2c.o
 obj-$(CONFIG_MFD_MADERA_SPI)	+= madera-spi.o
 
+obj-$(CONFIG_MFD_CS40L50_CORE)	+= cs40l50-core.o
+obj-$(CONFIG_MFD_CS40L50_I2C)	+= cs40l50-i2c.o
+obj-$(CONFIG_MFD_CS40L50_SPI)	+= cs40l50-spi.o
+
 obj-$(CONFIG_TPS6105X)		+= tps6105x.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_TPS6507X)		+= tps6507x.o
diff --git a/drivers/mfd/cs40l50-core.c b/drivers/mfd/cs40l50-core.c
new file mode 100644
index 000000000000..ecc7846e9f8d
--- /dev/null
+++ b/drivers/mfd/cs40l50-core.c
@@ -0,0 +1,536 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CS40L50 Advanced Haptic Driver with waveform memory,
+ * integrated DSP, and closed-loop algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ * Author: James Ogletree <james.ogletree@cirrus.com>
+ */
+
+#include <linux/firmware/cirrus/cs_dsp.h>
+#include <linux/firmware/cirrus/wmfw.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/cs40l50.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+
+static const struct mfd_cell cs40l50_devs[] = {
+	{ .name = "cs40l50-codec", },
+	{ .name = "cs40l50-vibra", },
+};
+
+const struct regmap_config cs40l50_regmap = {
+	.reg_bits =		32,
+	.reg_stride =		4,
+	.val_bits =		32,
+	.reg_format_endian =	REGMAP_ENDIAN_BIG,
+	.val_format_endian =	REGMAP_ENDIAN_BIG,
+};
+EXPORT_SYMBOL_GPL(cs40l50_regmap);
+
+static const char * const cs40l50_supplies[] = {
+	"vp", "vio",
+};
+
+static const struct regmap_irq cs40l50_reg_irqs[] = {
+	REGMAP_IRQ_REG(CS40L50_DSP_QUEUE_IRQ, CS40L50_IRQ1_INT_2_OFFSET,
+		       CS40L50_DSP_QUEUE_MASK),
+	REGMAP_IRQ_REG(CS40L50_AMP_SHORT_IRQ, CS40L50_IRQ1_INT_1_OFFSET,
+		       CS40L50_AMP_SHORT_MASK),
+	REGMAP_IRQ_REG(CS40L50_TEMP_ERR_IRQ, CS40L50_IRQ1_INT_8_OFFSET,
+		       CS40L50_TEMP_ERR_MASK),
+	REGMAP_IRQ_REG(CS40L50_BST_UVP_IRQ, CS40L50_IRQ1_INT_9_OFFSET,
+		       CS40L50_BST_UVP_MASK),
+	REGMAP_IRQ_REG(CS40L50_BST_SHORT_IRQ, CS40L50_IRQ1_INT_9_OFFSET,
+		       CS40L50_BST_SHORT_MASK),
+	REGMAP_IRQ_REG(CS40L50_BST_ILIMIT_IRQ, CS40L50_IRQ1_INT_9_OFFSET,
+		       CS40L50_BST_ILIMIT_MASK),
+	REGMAP_IRQ_REG(CS40L50_UVLO_VDDBATT_IRQ, CS40L50_IRQ1_INT_10_OFFSET,
+		       CS40L50_UVLO_VDDBATT_MASK),
+	REGMAP_IRQ_REG(CS40L50_GLOBAL_ERROR_IRQ, CS40L50_IRQ1_INT_18_OFFSET,
+		       CS40L50_GLOBAL_ERROR_MASK),
+};
+
+static struct regmap_irq_chip cs40l50_irq_chip = {
+	.name =		"CS40L50 IRQ Controller",
+
+	.status_base =	CS40L50_IRQ1_INT_1,
+	.mask_base =	CS40L50_IRQ1_MASK_1,
+	.ack_base =	CS40L50_IRQ1_INT_1,
+	.num_regs =	22,
+
+	.irqs =		cs40l50_reg_irqs,
+	.num_irqs =	ARRAY_SIZE(cs40l50_reg_irqs),
+
+	.runtime_pm =	true,
+};
+
+static const struct cs_dsp_region cs40l50_dsp_regions[] = {
+	{
+		.type = WMFW_HALO_PM_PACKED,
+		.base = CS40L50_PMEM_0
+	},
+	{
+		.type = WMFW_HALO_XM_PACKED,
+		.base = CS40L50_XMEM_PACKED_0
+	},
+	{
+		.type = WMFW_HALO_YM_PACKED,
+		.base = CS40L50_YMEM_PACKED_0
+	},
+	{
+		.type = WMFW_ADSP2_XM,
+		.base = CS40L50_XMEM_UNPACKED24_0
+	},
+	{
+		.type = WMFW_ADSP2_YM,
+		.base = CS40L50_YMEM_UNPACKED24_0
+	},
+};
+
+static void cs40l50_dsp_remove(void *data)
+{
+	cs_dsp_remove((struct cs_dsp *)data);
+}
+
+static int cs40l50_dsp_init(struct cs40l50 *cs40l50)
+{
+	int err;
+
+	cs40l50->dsp.num = 1;
+	cs40l50->dsp.type = WMFW_HALO;
+	cs40l50->dsp.dev = cs40l50->dev;
+	cs40l50->dsp.regmap = cs40l50->regmap;
+	cs40l50->dsp.base = CS40L50_CORE_BASE;
+	cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
+	cs40l50->dsp.mem = cs40l50_dsp_regions;
+	cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
+	cs40l50->dsp.no_core_startstop = true;
+
+	err = cs_dsp_halo_init(&cs40l50->dsp);
+	if (err)
+		return err;
+
+	return devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_remove,
+					&cs40l50->dsp);
+}
+
+static const struct reg_sequence cs40l50_err_rls[] = {
+	{ CS40L50_ERR_RLS, CS40L50_GLOBAL_ERR_RLS_SET },
+	{ CS40L50_ERR_RLS, CS40L50_GLOBAL_ERR_RLS_CLEAR },
+};
+
+struct cs40l50_irq {
+	char *name;
+	u32 virq;
+};
+
+static struct cs40l50_irq cs40l50_irqs[] = {
+	{ .name = "DSP", },
+	{ .name = "Global", },
+	{ .name = "Boost UVLO", },
+	{ .name = "Boost current limit", },
+	{ .name = "Boost short", },
+	{ .name = "Boost undervolt", },
+	{ .name = "Overtemp", },
+	{ .name = "Amp short", },
+};
+
+static int cs40l50_handle_hw_err(struct cs40l50 *cs40l50, int irq)
+{
+	int i;
+
+	/* Log hardware interrupt and execute error release sequence */
+	for (i = 1; i < ARRAY_SIZE(cs40l50_irqs); i++) {
+		if (cs40l50_irqs[i].virq == irq) {
+			dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[i].name);
+			return regmap_multi_reg_write(cs40l50->regmap, cs40l50_err_rls,
+						      ARRAY_SIZE(cs40l50_err_rls));
+		}
+	}
+
+	return 0;
+}
+
+static int cs40l50_process_dsp_queue(struct cs40l50 *cs40l50)
+{
+	u32 val, rd_ptr, wt_ptr;
+	int err = 0;
+
+	/* Read from DSP queue and update read pointer */
+	while (!err) {
+		err = regmap_read(cs40l50->regmap, CS40L50_DSP_QUEUE_WT, &wt_ptr);
+		if (err)
+			break;
+
+		err = regmap_read(cs40l50->regmap, CS40L50_DSP_QUEUE_RD, &rd_ptr);
+		if (err)
+			break;
+
+		/* Check if queue is empty */
+		if (wt_ptr == rd_ptr)
+			break;
+
+		err = regmap_read(cs40l50->regmap, rd_ptr, &val);
+		if (err)
+			break;
+
+		dev_dbg(cs40l50->dev, "DSP payload: %#X", val);
+
+		rd_ptr += sizeof(u32);
+
+		if (rd_ptr > CS40L50_DSP_QUEUE_END)
+			rd_ptr = CS40L50_DSP_QUEUE_BASE;
+
+		err =  regmap_write(cs40l50->regmap, CS40L50_DSP_QUEUE_RD, rd_ptr);
+		if (err)
+			break;
+	}
+
+	return err;
+}
+
+static irqreturn_t cs40l50_irq_handler(int irq, void *data)
+{
+	struct cs40l50 *cs40l50 = data;
+	int err;
+
+	mutex_lock(&cs40l50->lock);
+
+	if (irq == cs40l50_irqs[0].virq)
+		err = cs40l50_process_dsp_queue(cs40l50);
+	else
+		err = cs40l50_handle_hw_err(cs40l50, irq);
+
+	mutex_unlock(&cs40l50->lock);
+
+	return IRQ_RETVAL(!err);
+}
+
+
+static int cs40l50_irq_init(struct cs40l50 *cs40l50)
+{
+	struct device *dev = cs40l50->dev;
+	int i, err, virq;
+
+	err = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
+				       IRQF_ONESHOT | IRQF_SHARED, 0,
+				       &cs40l50_irq_chip, &cs40l50->irq_data);
+	if (err) {
+		dev_err(dev, "Failed adding IRQ chip\n");
+		return err;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
+		virq = regmap_irq_get_virq(cs40l50->irq_data, i);
+		if (virq < 0) {
+			dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
+			return virq;
+		}
+
+		cs40l50_irqs[i].virq = virq;
+
+		err = devm_request_threaded_irq(dev, virq, NULL,
+						cs40l50_irq_handler,
+						IRQF_ONESHOT | IRQF_SHARED,
+						cs40l50_irqs[i].name, cs40l50);
+		if (err) {
+			dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static const struct reg_sequence cs40l50_stop_core[] = {
+	{ CS40L50_CCM_CORE_CONTROL, CS40L50_CLOCK_DISABLE},
+	{ CS40L50_RAM_INIT, CS40L50_RAM_INIT_FLAG},
+	{ CS40L50_PWRMGT_CTL, CS40L50_MEM_RDY_HW},
+};
+
+static const struct reg_sequence cs40l50_internal_vamp_config[] = {
+	{ CS40L50_BST_LPMODE_SEL,	CS40L50_DCM_LOW_POWER },
+	{ CS40L50_BLOCK_ENABLES2,	CS40L50_OVERTEMP_WARN },
+};
+
+static const struct reg_sequence cs40l50_irq_mask_override[] = {
+	{ CS40L50_IRQ1_MASK_2,	CS40L50_IRQ_MASK_2_OVERRIDE },
+	{ CS40L50_IRQ1_MASK_20,	CS40L50_IRQ_MASK_20_OVERRIDE },
+};
+
+static void cs40l50_dsp_power_down(void *data)
+{
+	cs_dsp_power_down((struct cs_dsp *)data);
+}
+
+static int cs40l50_power_up_dsp(struct cs40l50 *cs40l50)
+{
+	int err;
+
+	mutex_lock(&cs40l50->lock);
+
+	if (cs40l50->patch) {
+		/* Stop core if loading patch file */
+		err = regmap_multi_reg_write(cs40l50->regmap, cs40l50_stop_core,
+					     ARRAY_SIZE(cs40l50_stop_core));
+		if (err)
+			goto err_mutex;
+	}
+
+	err = cs_dsp_power_up(&cs40l50->dsp, cs40l50->patch, "cs40l50.wmfw",
+			      cs40l50->bin, "cs40l50.bin", "cs40l50");
+	if (err)
+		goto err_mutex;
+
+	err = devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_power_down,
+				       &cs40l50->dsp);
+	if (err)
+		goto err_mutex;
+
+	if (cs40l50->patch) {
+		/* Resume core after loading patch file */
+		err = regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL,
+				   CS40L50_CLOCK_ENABLE);
+		if (err)
+			goto err_mutex;
+	}
+err_mutex:
+	mutex_unlock(&cs40l50->lock);
+
+	return err;
+}
+
+static struct cs_dsp_wseq cs40l50_wseqs[] = {
+	{
+		.reg =	CS40L50_POWER_ON_WSEQ,
+		.size = CS40L50_PSEQ_SIZE
+	},
+};
+
+static int cs40l50_configure_dsp(struct cs40l50 *cs40l50)
+{
+	u32 nwaves;
+	int err;
+
+	if (cs40l50->bin) {
+		/* Log number of effects if wavetable was loaded */
+		err = regmap_read(cs40l50->regmap, CS40L50_NUM_WAVES, &nwaves);
+		if (err)
+			return err;
+
+		dev_info(cs40l50->dev, "Loaded with %u RAM waveforms\n", nwaves);
+	}
+
+	err = cs_dsp_wseq_init(&cs40l50->dsp, cs40l50_wseqs,
+			       ARRAY_SIZE(cs40l50_wseqs));
+	if (err) {
+		dev_err(cs40l50->dev, "Failed to initialize write sequence\n");
+		return err;
+	}
+
+	/* Configure CS40L50 to use an internal V_AMP supply */
+	err = regmap_multi_reg_write(cs40l50->regmap, cs40l50_internal_vamp_config,
+				     ARRAY_SIZE(cs40l50_internal_vamp_config));
+	if (err)
+		return err;
+
+	err = cs_dsp_wseq_multi_write(&cs40l50->dsp, &cs40l50_wseqs[0],
+				      cs40l50_internal_vamp_config,
+				      ARRAY_SIZE(cs40l50_internal_vamp_config),
+				      false, CS_DSP_WSEQ_FULL);
+	if (err)
+		return err;
+
+	/* Override firmware defaults for IRQ masks */
+	err = regmap_multi_reg_write(cs40l50->regmap, cs40l50_irq_mask_override,
+				     ARRAY_SIZE(cs40l50_irq_mask_override));
+	if (err)
+		return err;
+
+	return cs_dsp_wseq_multi_write(&cs40l50->dsp, &cs40l50_wseqs[0],
+				       cs40l50_irq_mask_override,
+				       ARRAY_SIZE(cs40l50_irq_mask_override),
+				       false, CS_DSP_WSEQ_FULL);
+}
+
+static void cs40l50_start_dsp(const struct firmware *bin, void *context)
+{
+	struct cs40l50 *cs40l50 = context;
+	int err;
+
+	/* Wavetable is optional; proceed even if NULL */
+	cs40l50->bin = bin;
+
+	err = cs40l50_power_up_dsp(cs40l50);
+	if (err) {
+		dev_err(cs40l50->dev, "Failed to power up DSP: %d\n", err);
+		goto err_fw;
+	}
+
+	err = cs40l50_configure_dsp(cs40l50);
+	if (err)
+		dev_err(cs40l50->dev, "Failed to configure DSP: %d\n", err);
+
+err_fw:
+	release_firmware(cs40l50->bin);
+	release_firmware(cs40l50->patch);
+}
+
+static void cs40l50_request_patch(const struct firmware *patch, void *context)
+{
+	struct cs40l50 *cs40l50 = context;
+
+	/* Patch file is optional; proceed even if NULL */
+	cs40l50->patch = patch;
+
+	if (request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT, CS40L50_WT,
+				    cs40l50->dev, GFP_KERNEL,
+				    cs40l50, cs40l50_start_dsp)) {
+		dev_err(cs40l50->dev, "Failed to request %s\n", CS40L50_WT);
+		release_firmware(cs40l50->patch);
+	}
+}
+
+static int cs40l50_get_model(struct cs40l50 *cs40l50)
+{
+	struct device *dev = cs40l50->dev;
+	int err;
+
+	err = regmap_read(cs40l50->regmap, CS40L50_DEVID, &cs40l50->devid);
+	if (err)
+		return err;
+
+	if (cs40l50->devid != CS40L50_DEVID_A)
+		return -EINVAL;
+
+	err = regmap_read(cs40l50->regmap, CS40L50_REVID, &cs40l50->revid);
+	if (err)
+		return err;
+
+	if (cs40l50->revid < CS40L50_REVID_B0)
+		return -EINVAL;
+
+	dev_info(dev, "Cirrus Logic CS40L50 revision %02X\n", cs40l50->revid);
+
+	return 0;
+}
+
+static void cs40l50_pm_runtime_setup(struct device *dev)
+{
+	pm_runtime_set_autosuspend_delay(dev, CS40L50_AUTOSUSPEND_MS);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
+	devm_pm_runtime_enable(dev);
+}
+
+int cs40l50_probe(struct cs40l50 *cs40l50)
+{
+	struct device *dev = cs40l50->dev;
+	int err;
+
+	mutex_init(&cs40l50->lock);
+
+	cs40l50->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(cs40l50->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(cs40l50->reset_gpio),
+				     "Failed getting reset GPIO\n");
+
+	err = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(cs40l50_supplies),
+					     cs40l50_supplies);
+	if (err)
+		return dev_err_probe(dev, err, "Failed getting supplies\n");
+
+	/* Ensure minimum reset pulse width */
+	usleep_range(CS40L50_RESET_PULSE_US, CS40L50_RESET_PULSE_US + 100);
+
+	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
+
+	/* Wait for control port to be ready */
+	usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 100);
+
+	err = cs40l50_dsp_init(cs40l50);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to initialize DSP\n");
+
+	cs40l50_pm_runtime_setup(dev);
+
+	err = cs40l50_get_model(cs40l50);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to get part number\n");
+
+	err = cs40l50_irq_init(cs40l50);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to initialize IRQs\n");
+
+	err = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT, CS40L50_FW,
+				      dev, GFP_KERNEL, cs40l50, cs40l50_request_patch);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to request %s\n", CS40L50_FW);
+
+	err = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cs40l50_devs,
+				   ARRAY_SIZE(cs40l50_devs), NULL, 0, NULL);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to add sub devices\n");
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs40l50_probe);
+
+int cs40l50_remove(struct cs40l50 *cs40l50)
+{
+	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs40l50_remove);
+
+static int cs40l50_runtime_suspend(struct device *dev)
+{
+	struct cs40l50 *cs40l50 = dev_get_drvdata(dev);
+
+	return regmap_write(cs40l50->regmap, CS40L50_DSP_QUEUE, CS40L50_ALLOW_HIBER);
+}
+
+static int cs40l50_runtime_resume(struct device *dev)
+{
+	struct cs40l50 *cs40l50 = dev_get_drvdata(dev);
+	int err, i;
+	u32 val;
+
+	/* Device NAKs when exiting hibernation, so optionally retry here. */
+	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
+		err = regmap_write(cs40l50->regmap, CS40L50_DSP_QUEUE,
+				     CS40L50_PREVENT_HIBER);
+		if (!err)
+			break;
+
+		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
+	}
+
+	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
+		err = regmap_read(cs40l50->regmap, CS40L50_DSP_QUEUE, &val);
+		if (!err && val == 0)
+			return 0;
+
+		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
+	}
+
+	dev_err(cs40l50->dev, "Failed to resume: %d\n", err ?: -ETIMEDOUT);
+
+	return err ?: -ETIMEDOUT;
+}
+
+EXPORT_GPL_DEV_PM_OPS(cs40l50_pm_ops) = {
+	RUNTIME_PM_OPS(cs40l50_runtime_suspend, cs40l50_runtime_resume, NULL)
+};
+
+MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
+MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(FW_CS_DSP);
diff --git a/drivers/mfd/cs40l50-i2c.c b/drivers/mfd/cs40l50-i2c.c
new file mode 100644
index 000000000000..5214b611ed5c
--- /dev/null
+++ b/drivers/mfd/cs40l50-i2c.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CS40L50 Advanced Haptic Driver with waveform memory,
+ * integrated DSP, and closed-loop algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ * Author: James Ogletree <james.ogletree@cirrus.com>
+ */
+
+#include <linux/i2c.h>
+#include <linux/mfd/cs40l50.h>
+
+static int cs40l50_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct cs40l50 *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 *cs40l50 = i2c_get_clientdata(client);
+
+	cs40l50_remove(cs40l50);
+}
+
+static const struct i2c_device_id cs40l50_id_i2c[] = {
+	{"cs40l50"},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, cs40l50_id_i2c);
+
+static const struct of_device_id cs40l50_of_match[] = {
+	{ .compatible = "cirrus,cs40l50" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, cs40l50_of_match);
+
+static struct i2c_driver cs40l50_i2c_driver = {
+	.driver = {
+		.name = "cs40l50",
+		.of_match_table = cs40l50_of_match,
+		.pm = pm_ptr(&cs40l50_pm_ops),
+	},
+	.id_table = cs40l50_id_i2c,
+	.probe = cs40l50_i2c_probe,
+	.remove = cs40l50_i2c_remove,
+};
+module_i2c_driver(cs40l50_i2c_driver);
+
+MODULE_DESCRIPTION("CS40L50 I2C Driver");
+MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/cs40l50-spi.c b/drivers/mfd/cs40l50-spi.c
new file mode 100644
index 000000000000..a7468ea53acd
--- /dev/null
+++ b/drivers/mfd/cs40l50-spi.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CS40L50 Advanced Haptic Driver with waveform memory,
+ * integrated DSP, and closed-loop algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ * Author: James Ogletree <james.ogletree@cirrus.com>
+ */
+
+#include <linux/mfd/cs40l50.h>
+#include <linux/mfd/spi.h>
+
+static int cs40l50_spi_probe(struct spi_device *spi)
+{
+	struct cs40l50 *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 *cs40l50 = spi_get_drvdata(spi);
+
+	cs40l50_remove(cs40l50);
+}
+
+static const struct spi_device_id cs40l50_id_spi[] = {
+	{"cs40l50"},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, cs40l50_id_spi);
+
+static const struct of_device_id cs40l50_of_match[] = {
+	{ .compatible = "cirrus,cs40l50" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, cs40l50_of_match);
+
+static struct spi_driver cs40l50_spi_driver = {
+	.driver = {
+		.name = "cs40l50",
+		.of_match_table = cs40l50_of_match,
+		.pm = pm_ptr(&cs40l50_pm_ops),
+	},
+	.id_table = cs40l50_id_spi,
+	.probe = cs40l50_spi_probe,
+	.remove = cs40l50_spi_remove,
+};
+module_spi_driver(cs40l50_spi_driver);
+
+MODULE_DESCRIPTION("CS40L50 SPI Driver");
+MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/cs40l50.h b/include/linux/mfd/cs40l50.h
new file mode 100644
index 000000000000..260f192f4dfd
--- /dev/null
+++ b/include/linux/mfd/cs40l50.h
@@ -0,0 +1,128 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * CS40L50 Advanced Haptic Driver with waveform memory,
+ * integrated DSP, and closed-loop algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ * Author: James Ogletree <james.ogletree@cirrus.com>
+ */
+
+#ifndef __CS40L50_H__
+#define __CS40L50_H__
+
+#include <linux/firmware/cirrus/cs_dsp.h>
+#include <linux/gpio/consumer.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+
+/* Power Supply Configuration */
+#define CS40L50_BLOCK_ENABLES2			0x201C
+#define CS40L50_ERR_RLS				0x2034
+#define CS40L50_PWRMGT_CTL			0x2900
+#define CS40L50_BST_LPMODE_SEL			0x3810
+#define CS40L50_DCM_LOW_POWER		0x1
+#define CS40L50_OVERTEMP_WARN		0x4000010
+
+/* Interrupts */
+#define CS40L50_IRQ1_INT_1			0xE010
+#define CS40L50_IRQ1_MASK_1			0xE090
+#define CS40L50_IRQ1_MASK_2			0xE094
+#define CS40L50_IRQ1_MASK_20			0xE0DC
+#define CS40L50_IRQ_MASK_2_OVERRIDE	0xFFDF7FFF
+#define CS40L50_IRQ_MASK_20_OVERRIDE	0x15C01000
+#define CS40L50_IRQ1_INT_1_OFFSET	(4 * 0)
+#define CS40L50_IRQ1_INT_2_OFFSET	(4 * 1)
+#define CS40L50_IRQ1_INT_8_OFFSET	(4 * 7)
+#define CS40L50_IRQ1_INT_9_OFFSET	(4 * 8)
+#define CS40L50_IRQ1_INT_10_OFFSET	(4 * 9)
+#define CS40L50_IRQ1_INT_18_OFFSET	(4 * 17)
+#define CS40L50_GLOBAL_ERR_RLS_SET	BIT(11)
+#define CS40L50_GLOBAL_ERR_RLS_CLEAR	0
+#define CS40L50_AMP_SHORT_MASK		BIT(31)
+#define CS40L50_DSP_QUEUE_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(18)
+#define CS40L50_UVLO_VDDBATT_MASK	BIT(16)
+#define CS40L50_GLOBAL_ERROR_MASK	BIT(15)
+
+enum cs40l50_irq_list {
+	CS40L50_DSP_QUEUE_IRQ,
+	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_AMP_SHORT_IRQ,
+};
+
+/* DSP */
+#define CS40L50_XMEM_PACKED_0			0x2000000
+#define CS40L50_XMEM_UNPACKED24_0		0x2800000
+#define CS40L50_SYS_INFO_ID			0x25E0000
+#define CS40L50_RAM_INIT			0x28021DC
+#define CS40L50_DSP_QUEUE_WT			0x28042C8
+#define CS40L50_DSP_QUEUE_RD			0x28042CC
+#define CS40L50_POWER_ON_WSEQ			0x2804320
+#define CS40L50_NUM_WAVES			0x280CB4C
+#define CS40L50_CORE_BASE			0x2B80000
+#define CS40L50_CCM_CORE_CONTROL		0x2BC1000
+#define CS40L50_YMEM_PACKED_0			0x2C00000
+#define CS40L50_YMEM_UNPACKED24_0		0x3400000
+#define CS40L50_PMEM_0				0x3800000
+#define CS40L50_MEM_RDY_HW		0x2
+#define CS40L50_RAM_INIT_FLAG		0x1
+#define CS40L50_CLOCK_DISABLE		0x80
+#define CS40L50_CLOCK_ENABLE		0x281
+#define CS40L50_DSP_POLL_US		1000
+#define CS40L50_DSP_TIMEOUT_COUNT	100
+#define CS40L50_RESET_PULSE_US		2200
+#define CS40L50_CP_READY_US		3100
+#define CS40L50_AUTOSUSPEND_MS		2000
+#define CS40L50_PSEQ_SIZE		200
+
+/* DSP Commands */
+#define CS40L50_DSP_QUEUE_BASE		0x11004
+#define CS40L50_DSP_QUEUE_END		0x1101C
+#define CS40L50_DSP_QUEUE		0x11020
+#define CS40L50_PREVENT_HIBER	0x2000003
+#define CS40L50_ALLOW_HIBER	0x2000004
+#define CS40L50_START_I2S	0x3000002
+#define CS40L50_OWT_PUSH	0x3000008
+#define CS40L50_STOP_PLAYBACK	0x5000000
+#define CS40L50_OWT_DELETE	0xD000000
+
+/* Firmware files */
+#define CS40L50_FW	"cs40l50.wmfw"
+#define CS40L50_WT	"cs40l50.bin"
+
+/* Device */
+#define CS40L50_DEVID			0x0
+#define CS40L50_REVID			0x4
+#define CS40L50_DEVID_A		0x40A50
+#define CS40L50_REVID_B0	0xB0
+
+struct cs40l50 {
+	struct device *dev;
+	struct regmap *regmap;
+	struct mutex lock;
+	struct cs_dsp dsp;
+	struct gpio_desc *reset_gpio;
+	struct regmap_irq_chip_data *irq_data;
+	const struct firmware *patch;
+	const struct firmware *bin;
+	int irq;
+	u32 devid;
+	u32 revid;
+};
+
+int cs40l50_probe(struct cs40l50 *cs40l50);
+int cs40l50_remove(struct cs40l50 *cs40l50);
+
+extern const struct regmap_config cs40l50_regmap;
+extern const struct dev_pm_ops cs40l50_pm_ops;
+
+#endif /* __CS40L50_H__ */
-- 
2.25.1

Re: [PATCH v5 3/5] mfd: cs40l50: Add support for CS40L50 core driver

[Charles Keepax]

On Thu, Jan 04, 2024 at 10:36:36PM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
> 
> The MFD component registers and initializes the device.
> 
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
> +config MFD_CS40L50_CORE
> +	tristate
> +	select MFD_CORE
> +	select FW_CS_DSP
> +	select REGMAP_IRQ
> +
> +config MFD_CS40L50_I2C
> +	tristate "Cirrus Logic CS40L50 (I2C)"
> +	select REGMAP_I2C
> +	select MFD_CS40L50_CORE
> +	depends on I2C
> +	help
> +	  Select this to support the Cirrus Logic CS40L50 Haptic
> +	  Driver over I2C.
> +
> +	  This driver can be built as a module. If built as a module it will be
> +	  called "cs40l50-i2c".
> +
> +config MFD_CS40L50_SPI
> +	tristate "Cirrus Logic CS40L50 (SPI)"
> +	select REGMAP_SPI
> +	select MFD_CS40L50_CORE
> +	depends on SPI
> +	help
> +	  Select this to support the Cirrus Logic CS40L50 Haptic
> +	  Driver over SPI.
> +
> +	  This driver can be built as a module. If built as a module it will be
> +	  called "cs40l50-spi".
> +

Generally the order in Kconfigs should be alphabetical, probably
up around Cirrus Madera stuff would make most sense.

> +static int cs40l50_dsp_init(struct cs40l50 *cs40l50)
> +{
> +	int err;
> +
> +	cs40l50->dsp.num = 1;
> +	cs40l50->dsp.type = WMFW_HALO;
> +	cs40l50->dsp.dev = cs40l50->dev;
> +	cs40l50->dsp.regmap = cs40l50->regmap;
> +	cs40l50->dsp.base = CS40L50_CORE_BASE;
> +	cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
> +	cs40l50->dsp.mem = cs40l50_dsp_regions;
> +	cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
> +	cs40l50->dsp.no_core_startstop = true;
> +
> +	err = cs_dsp_halo_init(&cs40l50->dsp);
> +	if (err)
> +		return err;
> +
> +	return devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_remove,
> +					&cs40l50->dsp);

Hmm... I notice you use this for both dsp_remove and
dsp_power_down. Are you sure devm will guarantee those are called
in the right order? Its not immediately clear to me that would be
have to be the case.

> +static irqreturn_t cs40l50_irq_handler(int irq, void *data)
> +{
> +	struct cs40l50 *cs40l50 = data;
> +	int err;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	if (irq == cs40l50_irqs[0].virq)
> +		err = cs40l50_process_dsp_queue(cs40l50);
> +	else
> +		err = cs40l50_handle_hw_err(cs40l50, irq);

Feels kinda weird to assign the same handler to every IRQ and
then depending on which IRQ it was call a different function.
Would it not be simpler just to assign a different handler?

> +static int cs40l50_power_up_dsp(struct cs40l50 *cs40l50)
> +{
> +	int err;
> +
> +	mutex_lock(&cs40l50->lock);
> +
> +	if (cs40l50->patch) {
> +		/* Stop core if loading patch file */
> +		err = regmap_multi_reg_write(cs40l50->regmap, cs40l50_stop_core,
> +					     ARRAY_SIZE(cs40l50_stop_core));
> +		if (err)
> +			goto err_mutex;
> +	}
> +
> +	err = cs_dsp_power_up(&cs40l50->dsp, cs40l50->patch, "cs40l50.wmfw",
> +			      cs40l50->bin, "cs40l50.bin", "cs40l50");
> +	if (err)
> +		goto err_mutex;
> +
> +	err = devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_power_down,
> +				       &cs40l50->dsp);
> +	if (err)
> +		goto err_mutex;
> +
> +	if (cs40l50->patch) {
> +		/* Resume core after loading patch file */
> +		err = regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL,
> +				   CS40L50_CLOCK_ENABLE);

This feels like this needs a comment, why are we skipping the
normal DSP init and doing it manually (this appears to be the
same writes start_core would have done)? I assume its something to
do with what you are really doing is you don't want lock_memory
to run?

> +static int cs40l50_configure_dsp(struct cs40l50 *cs40l50)
> +{
> +	u32 nwaves;
> +	int err;
> +
> +	if (cs40l50->bin) {
> +		/* Log number of effects if wavetable was loaded */
> +		err = regmap_read(cs40l50->regmap, CS40L50_NUM_WAVES, &nwaves);
> +		if (err)
> +			return err;
> +
> +		dev_info(cs40l50->dev, "Loaded with %u RAM waveforms\n", nwaves);

Kinda nervous about the fact we access all these DSP controls
directly through address, rather than using the DSP control
accessors, we have the accessors for a reason. They manage things
like access permissions etc. and historically, the firmware
guys have not been able to guarantee these remain in consistent
locations between firmware versions.

I guess this is so you can access them even in the case of the
ROM firmware, but you could have a meta-data only firmware file
that you load in that case to give you the controls.  I don't
feel the need to NAK the driver based on this but please think
about this very carefully it's a strange way to use the DSP
controls, and feels likely to cause problems to me. It is also
quite hostile to fixing it in the future since as you are not
using the controls no one will be checking that things like the
access flags in the firmware are set correctly, which is annoying
if the decision has to be reversed later since there will likely
be a bunch of broken firmwares already in the field.

> +int cs40l50_probe(struct cs40l50 *cs40l50)
> +{
> +	struct device *dev = cs40l50->dev;
> +	int err;
> +
> +	mutex_init(&cs40l50->lock);
> +
> +	cs40l50->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(cs40l50->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(cs40l50->reset_gpio),
> +				     "Failed getting reset GPIO\n");
> +
> +	err = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(cs40l50_supplies),
> +					     cs40l50_supplies);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed getting supplies\n");
> +
> +	/* Ensure minimum reset pulse width */
> +	usleep_range(CS40L50_RESET_PULSE_US, CS40L50_RESET_PULSE_US + 100);
> +
> +	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
> +
> +	/* Wait for control port to be ready */
> +	usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 100);
> +
> +	err = cs40l50_dsp_init(cs40l50);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to initialize DSP\n");
> +
> +	cs40l50_pm_runtime_setup(dev);
> +
> +	err = cs40l50_get_model(cs40l50);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to get part number\n");
> +
> +	err = cs40l50_irq_init(cs40l50);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to initialize IRQs\n");
> +
> +	err = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT, CS40L50_FW,
> +				      dev, GFP_KERNEL, cs40l50, cs40l50_request_patch);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to request %s\n", CS40L50_FW);
> +
> +	err = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cs40l50_devs,
> +				   ARRAY_SIZE(cs40l50_devs), NULL, 0, NULL);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to add sub devices\n");
> +

Do you want to add the child devices here? Or after the firmware
init has been done? If you do it here then the child devices may
well probe and become available before all the setup you have in
the DSP loading stuff is done. What happens if one of those
drivers tries to do something before init is complete?

> +static int cs40l50_runtime_resume(struct device *dev)
> +{
> +	struct cs40l50 *cs40l50 = dev_get_drvdata(dev);
> +	int err, i;
> +	u32 val;
> +
> +	/* Device NAKs when exiting hibernation, so optionally retry here. */
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		err = regmap_write(cs40l50->regmap, CS40L50_DSP_QUEUE,
> +				     CS40L50_PREVENT_HIBER);
> +		if (!err)
> +			break;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}

Are you comfortable with the behaviour here? If the chip fails to
respond before the TIMEOUT, you will proceed on and do the read
loop. Since the read loop is just looking for a zero in the
queue, it looks like if for some reason the chip was too slow to
respond this function would succeed despite the chip never
receiving the PREVENT_HIBER. Would it not be safer to skip the
read loop if you fail to send the PREVENT_HIBER?

> +
> +	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> +		err = regmap_read(cs40l50->regmap, CS40L50_DSP_QUEUE, &val);
> +		if (!err && val == 0)
> +			return 0;
> +
> +		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> +	}

I am not sure if the ignoring errors is important here (as it is
for the first loop), but if it is a comment should be added to
say why, and if it isn't, couldn't this be a
regmap_read_poll_timeout instead of a manual loop?

> +++ b/drivers/mfd/cs40l50-spi.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + * Author: James Ogletree <james.ogletree@cirrus.com>
> + */
> +
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/mfd/spi.h>

Should be linux/spi/spi.h, make sure your build testing the whole
patch.

> +#ifndef __CS40L50_H__
> +#define __CS40L50_H__
> +
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +
> +/* Power Supply Configuration */
> +#define CS40L50_BLOCK_ENABLES2			0x201C
> +#define CS40L50_ERR_RLS				0x2034
> +#define CS40L50_PWRMGT_CTL			0x2900
> +#define CS40L50_BST_LPMODE_SEL			0x3810
> +#define CS40L50_DCM_LOW_POWER		0x1
> +#define CS40L50_OVERTEMP_WARN		0x4000010
> +
> +/* Interrupts */
> +#define CS40L50_IRQ1_INT_1			0xE010
> +#define CS40L50_IRQ1_MASK_1			0xE090
> +#define CS40L50_IRQ1_MASK_2			0xE094
> +#define CS40L50_IRQ1_MASK_20			0xE0DC
> +#define CS40L50_IRQ_MASK_2_OVERRIDE	0xFFDF7FFF
> +#define CS40L50_IRQ_MASK_20_OVERRIDE	0x15C01000
> +#define CS40L50_IRQ1_INT_1_OFFSET	(4 * 0)
> +#define CS40L50_IRQ1_INT_2_OFFSET	(4 * 1)
> +#define CS40L50_IRQ1_INT_8_OFFSET	(4 * 7)
> +#define CS40L50_IRQ1_INT_9_OFFSET	(4 * 8)
> +#define CS40L50_IRQ1_INT_10_OFFSET	(4 * 9)
> +#define CS40L50_IRQ1_INT_18_OFFSET	(4 * 17)
> +#define CS40L50_GLOBAL_ERR_RLS_SET	BIT(11)
> +#define CS40L50_GLOBAL_ERR_RLS_CLEAR	0
> +#define CS40L50_AMP_SHORT_MASK		BIT(31)
> +#define CS40L50_DSP_QUEUE_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(18)
> +#define CS40L50_UVLO_VDDBATT_MASK	BIT(16)
> +#define CS40L50_GLOBAL_ERROR_MASK	BIT(15)
> +
> +enum cs40l50_irq_list {
> +	CS40L50_DSP_QUEUE_IRQ,
> +	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_AMP_SHORT_IRQ,
> +};
> +
> +/* DSP */
> +#define CS40L50_XMEM_PACKED_0			0x2000000
> +#define CS40L50_XMEM_UNPACKED24_0		0x2800000
> +#define CS40L50_SYS_INFO_ID			0x25E0000
> +#define CS40L50_RAM_INIT			0x28021DC
> +#define CS40L50_DSP_QUEUE_WT			0x28042C8
> +#define CS40L50_DSP_QUEUE_RD			0x28042CC
> +#define CS40L50_POWER_ON_WSEQ			0x2804320
> +#define CS40L50_NUM_WAVES			0x280CB4C
> +#define CS40L50_CORE_BASE			0x2B80000
> +#define CS40L50_CCM_CORE_CONTROL		0x2BC1000
> +#define CS40L50_YMEM_PACKED_0			0x2C00000
> +#define CS40L50_YMEM_UNPACKED24_0		0x3400000
> +#define CS40L50_PMEM_0				0x3800000
> +#define CS40L50_MEM_RDY_HW		0x2
> +#define CS40L50_RAM_INIT_FLAG		0x1
> +#define CS40L50_CLOCK_DISABLE		0x80
> +#define CS40L50_CLOCK_ENABLE		0x281
> +#define CS40L50_DSP_POLL_US		1000
> +#define CS40L50_DSP_TIMEOUT_COUNT	100
> +#define CS40L50_RESET_PULSE_US		2200
> +#define CS40L50_CP_READY_US		3100
> +#define CS40L50_AUTOSUSPEND_MS		2000
> +#define CS40L50_PSEQ_SIZE		200
> +
> +/* DSP Commands */
> +#define CS40L50_DSP_QUEUE_BASE		0x11004
> +#define CS40L50_DSP_QUEUE_END		0x1101C
> +#define CS40L50_DSP_QUEUE		0x11020
> +#define CS40L50_PREVENT_HIBER	0x2000003
> +#define CS40L50_ALLOW_HIBER	0x2000004
> +#define CS40L50_START_I2S	0x3000002
> +#define CS40L50_OWT_PUSH	0x3000008
> +#define CS40L50_STOP_PLAYBACK	0x5000000
> +#define CS40L50_OWT_DELETE	0xD000000
> +
> +/* Firmware files */
> +#define CS40L50_FW	"cs40l50.wmfw"
> +#define CS40L50_WT	"cs40l50.bin"
> +
> +/* Device */
> +#define CS40L50_DEVID			0x0
> +#define CS40L50_REVID			0x4
> +#define CS40L50_DEVID_A		0x40A50
> +#define CS40L50_REVID_B0	0xB0

Admittedly a bit nitpicky but the tabbing here is all over the
place, would be nicer to line up the values on these defines.

Thanks,
Charles

Re: [PATCH v5 3/5] mfd: cs40l50: Add support for CS40L50 core driver

[James Ogletree]

Hi Charles,

Thank you for your excellent review. Anything not replied to will be
adopted as-is in the next version.

> On Jan 5, 2024, at 8:04 AM, Charles Keepax <ckeepax@opensource.cirrus.com> wrote:
> 
> On Thu, Jan 04, 2024 at 10:36:36PM +0000, James Ogletree wrote
> 
>> +static int cs40l50_dsp_init(struct cs40l50 *cs40l50)
>> +{
>> + int err;
>> +
>> + cs40l50->dsp.num = 1;
>> + cs40l50->dsp.type = WMFW_HALO;
>> + cs40l50->dsp.dev = cs40l50->dev;
>> + cs40l50->dsp.regmap = cs40l50->regmap;
>> + cs40l50->dsp.base = CS40L50_CORE_BASE;
>> + cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
>> + cs40l50->dsp.mem = cs40l50_dsp_regions;
>> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
>> + cs40l50->dsp.no_core_startstop = true;
>> +
>> + err = cs_dsp_halo_init(&cs40l50->dsp);
>> + if (err)
>> + return err;
>> +
>> + return devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_remove,
>> + &cs40l50->dsp);
> 
> Hmm... I notice you use this for both dsp_remove and
> dsp_power_down. Are you sure devm will guarantee those are called
> in the right order? Its not immediately clear to me that would be
> have to be the case.

On my inspection of the devm code, actions are always added to the
tail, and played back from head to tail on driver detach.

> 
>> +static int cs40l50_power_up_dsp(struct cs40l50 *cs40l50)
>> +{
>> + int err;
>> +
>> + mutex_lock(&cs40l50->lock);
>> +
>> + if (cs40l50->patch) {
>> + /* Stop core if loading patch file */
>> + err = regmap_multi_reg_write(cs40l50->regmap, cs40l50_stop_core,
>> +     ARRAY_SIZE(cs40l50_stop_core));
>> + if (err)
>> + goto err_mutex;
>> + }
>> +
>> + err = cs_dsp_power_up(&cs40l50->dsp, cs40l50->patch, "cs40l50.wmfw",
>> +      cs40l50->bin, "cs40l50.bin", "cs40l50");
>> + if (err)
>> + goto err_mutex;
>> +
>> + err = devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_power_down,
>> +       &cs40l50->dsp);
>> + if (err)
>> + goto err_mutex;
>> +
>> + if (cs40l50->patch) {
>> + /* Resume core after loading patch file */
>> + err = regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL,
>> +   CS40L50_CLOCK_ENABLE);
> 
> This feels like this needs a comment, why are we skipping the
> normal DSP init and doing it manually (this appears to be the
> same writes start_core would have done)? I assume its something to
> do with what you are really doing is you don't want lock_memory
> to run?

The dsp struct uses cs_dsp_halo_ao_ops, made for self-booting
DSPs, which has none of the ops used in cs_dsp_run(). The
manual stop is because it is self-booting (already running you could
say) but we need to stop the clock to patch the firmware. Please
correct me if that is not right.

>> +static int cs40l50_configure_dsp(struct cs40l50 *cs40l50)
>> +{
>> + u32 nwaves;
>> + int err;
>> +
>> + if (cs40l50->bin) {
>> + /* Log number of effects if wavetable was loaded */
>> + err = regmap_read(cs40l50->regmap, CS40L50_NUM_WAVES, &nwaves);
>> + if (err)
>> + return err;
>> +
>> + dev_info(cs40l50->dev, "Loaded with %u RAM waveforms\n", nwaves);
> 
> Kinda nervous about the fact we access all these DSP controls
> directly through address, rather than using the DSP control
> accessors, we have the accessors for a reason. They manage things
> like access permissions etc. and historically, the firmware
> guys have not been able to guarantee these remain in consistent
> locations between firmware versions.
> 
> I guess this is so you can access them even in the case of the
> ROM firmware, but you could have a meta-data only firmware file
> that you load in that case to give you the controls.  I don't
> feel the need to NAK the driver based on this but please think
> about this very carefully it's a strange way to use the DSP
> controls, and feels likely to cause problems to me. It is also
> quite hostile to fixing it in the future since as you are not
> using the controls no one will be checking that things like the
> access flags in the firmware are set correctly, which is annoying
> if the decision has to be reversed later since there will likely
> be a bunch of broken firmwares already in the field.

Noting here that we discussed this offline. The driver is going to
stay with a static register design for now, but the write sequence
interface is being modified to be control based.

Best,
James

[PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[James Ogletree]

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

The input driver provides the interface for control of
haptic effects through the device.

Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
---
 MAINTAINERS                        |   1 +
 drivers/input/misc/Kconfig         |  10 +
 drivers/input/misc/Makefile        |   1 +
 drivers/input/misc/cs40l50-vibra.c | 572 +++++++++++++++++++++++++++++
 4 files changed, 584 insertions(+)
 create mode 100644 drivers/input/misc/cs40l50-vibra.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 69a9e0a3b968..24cfb4f017bb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4940,6 +4940,7 @@ 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:	drivers/mfd/cs40l*
 F:	include/linux/mfd/cs40l*
 
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 6ba984d7f0b1..ee45dbb0636e 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -140,6 +140,16 @@ config INPUT_BMA150
 	  To compile this driver as a module, choose M here: the
 	  module will be called bma150.
 
+config INPUT_CS40L50_VIBRA
+	tristate "CS40L50 Haptic Driver support"
+	depends on MFD_CS40L50_CORE
+	help
+	  Say Y here to enable support for Cirrus Logic's CS40L50
+	  haptic driver.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cs40l50-vibra.
+
 config INPUT_E3X0_BUTTON
 	tristate "NI Ettus Research USRP E3xx Button support."
 	default n
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 04296a4abe8e..88279de6d3d5 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_INPUT_CMA3000)		+= cma3000_d0x.o
 obj-$(CONFIG_INPUT_CMA3000_I2C)		+= cma3000_d0x_i2c.o
 obj-$(CONFIG_INPUT_COBALT_BTNS)		+= cobalt_btns.o
 obj-$(CONFIG_INPUT_CPCAP_PWRBUTTON)	+= cpcap-pwrbutton.o
+obj-$(CONFIG_INPUT_CS40L50_VIBRA)	+= cs40l50-vibra.o
 obj-$(CONFIG_INPUT_DA7280_HAPTICS)	+= da7280.o
 obj-$(CONFIG_INPUT_DA9052_ONKEY)	+= da9052_onkey.o
 obj-$(CONFIG_INPUT_DA9055_ONKEY)	+= da9055_onkey.o
diff --git a/drivers/input/misc/cs40l50-vibra.c b/drivers/input/misc/cs40l50-vibra.c
new file mode 100644
index 000000000000..cd3c2bf132da
--- /dev/null
+++ b/drivers/input/misc/cs40l50-vibra.c
@@ -0,0 +1,572 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CS40L50 Advanced Haptic Driver with waveform memory,
+ * integrated DSP, and closed-loop algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ * Author: James Ogletree <james.ogletree@cirrus.com>
+ */
+
+#include <linux/input.h>
+#include <linux/mfd/cs40l50.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+/* Wavetables */
+#define CS40L50_RAM_INDEX_START		0x1000000
+#define CS40L50_RAM_INDEX_END		0x100007F
+#define CS40L50_RTH_INDEX_START		0x1400000
+#define CS40L50_RTH_INDEX_END		0x1400001
+#define CS40L50_ROM_INDEX_START		0x1800000
+#define CS40L50_ROM_INDEX_END		0x180001A
+#define CS40L50_OWT_HEADER_SIZE		12
+#define CS40L50_OWT_TYPE_PCM		8
+#define CS40L50_OWT_TYPE_PWLE		12
+#define CS40L50_OWT_PCM_ID		0x0
+#define CS40L50_OWT_CUSTOM_DATA_SIZE	2
+
+/* DSP */
+#define CS40L50_GPIO_BASE		0x2804140
+#define CS40L50_OWT_BASE		0x2805C34
+#define CS40L50_OWT_SIZE		0x2805C38
+#define CS40L50_OWT_NEXT		0x2805C3C
+
+/* GPIO */
+#define CS40L50_GPIO_NUM_MASK		GENMASK(14, 12)
+#define CS40L50_GPIO_EDGE_MASK		BIT(15)
+#define CS40L50_GPIO_MAPPING_INVALID	0
+#define CS40L50_GPIO_DISABLE		0x1FF
+
+enum vibra_bank_type {
+	WVFRM_BANK_RAM,
+	WVFRM_BANK_ROM,
+	WVFRM_BANK_OWT,
+	WVFRM_BANK_NUM,
+};
+
+/* Describes an area in DSP memory populated by effects */
+struct vibra_bank {
+	enum vibra_bank_type bank;
+	u32 base_index;
+	u32 max_index;
+};
+
+struct vibra_effect {
+	enum vibra_bank_type bank;
+	struct list_head list;
+	u32 gpio_mapping;
+	u32 index;
+	int id;
+};
+
+/* Describes haptic interface of loaded DSP firmware */
+struct vibra_dsp {
+	struct vibra_bank *banks;
+	u32 gpio_base_reg;
+	u32 owt_offset_reg;
+	u32 owt_size_reg;
+	u32 owt_base_reg;
+	u32 mailbox_reg;
+	u32 push_owt_cmd;
+	u32 delete_owt_cmd;
+	u32 stop_cmd;
+};
+
+/* Describes configuration and state of haptic operations */
+struct vibra_info {
+	struct device *dev;
+	struct regmap *regmap;
+	struct input_dev *input;
+	struct mutex lock;
+	struct workqueue_struct *vibe_wq;
+	struct work_struct vibe_start_work;
+	struct work_struct vibe_stop_work;
+	struct work_struct erase_work;
+	struct work_struct add_work;
+	struct ff_effect *start_effect;
+	struct ff_effect *erase_effect;
+	struct ff_effect add_effect;
+	struct list_head effect_head;
+	struct vibra_dsp dsp;
+	int erase_error;
+	int add_error;
+};
+
+static struct vibra_effect *vibra_find_effect(int id, struct list_head *effect_head)
+{
+	struct vibra_effect *effect;
+
+	list_for_each_entry(effect, effect_head, list)
+		if (effect->id == id)
+			return effect;
+
+	return NULL;
+}
+
+static int vibra_effect_bank_set(struct vibra_info *info,
+				 struct vibra_effect *effect,
+				 struct ff_periodic_effect add_effect)
+{
+	s16 bank = add_effect.custom_data[0] & 0xffffu;
+	unsigned int len = add_effect.custom_len;
+
+	if (bank >= WVFRM_BANK_NUM) {
+		dev_err(info->dev, "Invalid waveform bank: %d\n", bank);
+		return -EINVAL;
+	}
+
+	effect->bank = len > CS40L50_OWT_CUSTOM_DATA_SIZE ? WVFRM_BANK_OWT : bank;
+
+	return 0;
+}
+
+static int vibra_effect_mapping_set(struct vibra_info *info, u16 button,
+				    struct vibra_effect *effect)
+{
+	u32 gpio_num, gpio_edge;
+
+	if (button) {
+		gpio_num = FIELD_GET(CS40L50_GPIO_NUM_MASK, button);
+		gpio_edge = FIELD_GET(CS40L50_GPIO_EDGE_MASK, button);
+		effect->gpio_mapping = info->dsp.gpio_base_reg +
+				       (gpio_num * 8) - gpio_edge;
+
+		return regmap_write(info->regmap, effect->gpio_mapping, button);
+	}
+
+	effect->gpio_mapping = CS40L50_GPIO_MAPPING_INVALID;
+
+	return 0;
+}
+
+static int vibra_effect_index_set(struct vibra_info *info,
+				  struct vibra_effect *effect,
+				  struct ff_periodic_effect add_effect)
+{
+	struct vibra_effect *owt_effect;
+	u32 base_index, max_index;
+
+	base_index = info->dsp.banks[effect->bank].base_index;
+	max_index = info->dsp.banks[effect->bank].max_index;
+
+	effect->index = base_index;
+
+	switch (effect->bank) {
+	case WVFRM_BANK_OWT:
+		list_for_each_entry(owt_effect, &info->effect_head, list)
+			if (owt_effect->bank == WVFRM_BANK_OWT)
+				effect->index++;
+		break;
+	case WVFRM_BANK_ROM:
+	case WVFRM_BANK_RAM:
+		effect->index += add_effect.custom_data[1] & 0xffffu;
+		break;
+	default:
+		dev_err(info->dev, "Bank not supported: %d\n", effect->bank);
+		return -EINVAL;
+	}
+
+	if (effect->index > max_index || effect->index < base_index) {
+		dev_err(info->dev, "Index out of bounds: %u\n", effect->index);
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
+/* Describes a header for an effect in the OWT bank */
+struct owt_header {
+	u32 type;
+	u32 data_words;
+	u32 offset;
+} __packed;
+
+static int vibra_upload_owt(struct vibra_info *info, struct vibra_effect *effect,
+			    struct ff_periodic_effect add_effect)
+{
+	u32 len, wt_offset, wt_size;
+	struct owt_header header;
+	u8 *out_data;
+	int error;
+
+	error = regmap_read(info->regmap, info->dsp.owt_offset_reg, &wt_offset);
+	if (error)
+		return error;
+
+	error = regmap_read(info->regmap, info->dsp.owt_size_reg, &wt_size);
+	if (error)
+		return error;
+
+	len = 2 * add_effect.custom_len;
+
+	if ((wt_size * sizeof(u32)) < CS40L50_OWT_HEADER_SIZE + len)
+		return -ENOSPC;
+
+	out_data = kzalloc(CS40L50_OWT_HEADER_SIZE + len, GFP_KERNEL);
+	if (!out_data)
+		return -ENOMEM;
+
+	header.type = add_effect.custom_data[0] == CS40L50_OWT_PCM_ID ? CS40L50_OWT_TYPE_PCM :
+									CS40L50_OWT_TYPE_PWLE;
+	header.offset = CS40L50_OWT_HEADER_SIZE / sizeof(u32);
+	header.data_words = len / sizeof(u32);
+
+	memcpy(out_data, &header, sizeof(header));
+	memcpy(out_data + CS40L50_OWT_HEADER_SIZE, add_effect.custom_data, len);
+
+	error = regmap_bulk_write(info->regmap, info->dsp.owt_base_reg +
+				  (wt_offset * sizeof(u32)), out_data,
+				  CS40L50_OWT_HEADER_SIZE + len);
+	if (error)
+		goto err_free;
+
+	error = regmap_write(info->regmap, info->dsp.mailbox_reg,
+			     info->dsp.push_owt_cmd);
+
+err_free:
+	kfree(out_data);
+
+	return error;
+}
+
+static void vibra_add_worker(struct work_struct *work)
+{
+	struct vibra_info *info = container_of(work, struct vibra_info, add_work);
+	struct ff_effect add_effect = info->add_effect;
+	struct vibra_effect *effect;
+	bool is_new = false;
+	int error;
+
+	error = pm_runtime_resume_and_get(info->dev);
+	if (error < 0) {
+		info->add_error = error;
+		return;
+	}
+
+	mutex_lock(&info->lock);
+
+	effect = vibra_find_effect(add_effect.id, &info->effect_head);
+	if (!effect) {
+		effect = kzalloc(sizeof(*effect), GFP_KERNEL);
+		if (!effect) {
+			error = -ENOMEM;
+			goto err_mutex;
+		}
+		effect->id = add_effect.id;
+		is_new = true;
+	}
+
+	error = vibra_effect_bank_set(info, effect, add_effect.u.periodic);
+	if (error)
+		goto err_free;
+
+	error = vibra_effect_index_set(info, effect, add_effect.u.periodic);
+	if (error)
+		goto err_free;
+
+	error = vibra_effect_mapping_set(info, add_effect.trigger.button, effect);
+	if (error)
+		goto err_free;
+
+	if (effect->bank == WVFRM_BANK_OWT)
+		error = vibra_upload_owt(info, effect,  add_effect.u.periodic);
+
+err_free:
+	if (is_new) {
+		if (error)
+			kfree(effect);
+		else
+			list_add(&effect->list, &info->effect_head);
+	}
+
+err_mutex:
+	mutex_unlock(&info->lock);
+
+	pm_runtime_mark_last_busy(info->dev);
+	pm_runtime_put_autosuspend(info->dev);
+
+	info->add_error = error;
+}
+
+static int vibra_add(struct input_dev *dev, struct ff_effect *effect,
+		     struct ff_effect *old)
+{
+	struct vibra_info *info = input_get_drvdata(dev);
+	u32 len = effect->u.periodic.custom_len;
+
+	if (effect->type != FF_PERIODIC || effect->u.periodic.waveform != FF_CUSTOM) {
+		dev_err(info->dev, "Type (%#X) or waveform (%#X) unsupported\n",
+			effect->type, effect->u.periodic.waveform);
+		return -EINVAL;
+	}
+
+	memcpy(&info->add_effect, effect, sizeof(struct ff_effect));
+
+	info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
+	if (!info->add_effect.u.periodic.custom_data)
+		return -ENOMEM;
+
+	if (copy_from_user(info->add_effect.u.periodic.custom_data,
+			   effect->u.periodic.custom_data, sizeof(s16) * len)) {
+		info->add_error = -EFAULT;
+		goto out_free;
+	}
+
+	queue_work(info->vibe_wq, &info->add_work);
+	flush_work(&info->add_work);
+out_free:
+	kfree(info->add_effect.u.periodic.custom_data);
+	info->add_effect.u.periodic.custom_data = NULL;
+
+	return info->add_error;
+}
+
+static void vibra_start_worker(struct work_struct *work)
+{
+	struct vibra_info *info = container_of(work, struct vibra_info,
+					       vibe_start_work);
+	struct vibra_effect *effect;
+
+	if (pm_runtime_resume_and_get(info->dev) < 0)
+		return;
+
+	mutex_lock(&info->lock);
+
+	effect = vibra_find_effect(info->start_effect->id, &info->effect_head);
+	if (effect)
+		regmap_write(info->regmap, info->dsp.mailbox_reg, effect->index);
+
+	mutex_unlock(&info->lock);
+
+	if (!effect)
+		dev_err(info->dev, "Effect to play not found\n");
+
+	pm_runtime_mark_last_busy(info->dev);
+	pm_runtime_put_autosuspend(info->dev);
+}
+
+static void vibra_stop_worker(struct work_struct *work)
+{
+	struct vibra_info *info = container_of(work, struct vibra_info,
+					       vibe_stop_work);
+
+	if (pm_runtime_resume_and_get(info->dev) < 0)
+		return;
+
+	mutex_lock(&info->lock);
+
+	regmap_write(info->regmap, info->dsp.mailbox_reg, info->dsp.stop_cmd);
+
+	mutex_unlock(&info->lock);
+
+	pm_runtime_mark_last_busy(info->dev);
+	pm_runtime_put_autosuspend(info->dev);
+}
+
+static int vibra_playback(struct input_dev *dev, int effect_id, int val)
+{
+	struct vibra_info *info = input_get_drvdata(dev);
+
+	if (val > 0) {
+		info->start_effect = &dev->ff->effects[effect_id];
+		queue_work(info->vibe_wq, &info->vibe_start_work);
+	} else {
+		queue_work(info->vibe_wq, &info->vibe_stop_work);
+	}
+
+	return 0;
+}
+
+static void vibra_erase_worker(struct work_struct *work)
+{
+	struct vibra_info *info = container_of(work, struct vibra_info,
+					       erase_work);
+	struct vibra_effect *owt_effect, *erase_effect;
+	int error;
+
+	error = pm_runtime_resume_and_get(info->dev);
+	if (error < 0) {
+		info->erase_error = error;
+		return;
+	}
+
+	mutex_lock(&info->lock);
+
+	erase_effect = vibra_find_effect(info->erase_effect->id,
+					 &info->effect_head);
+	if (!erase_effect) {
+		dev_err(info->dev, "Effect to erase not found\n");
+		error = -EINVAL;
+		goto err_mutex;
+	}
+
+	if (erase_effect->gpio_mapping != CS40L50_GPIO_MAPPING_INVALID) {
+		error = regmap_write(info->regmap, erase_effect->gpio_mapping,
+				     CS40L50_GPIO_DISABLE);
+		if (error)
+			goto err_mutex;
+	}
+
+	if (erase_effect->bank == WVFRM_BANK_OWT) {
+		error = regmap_write(info->regmap, info->dsp.mailbox_reg,
+				     info->dsp.delete_owt_cmd | erase_effect->index);
+		if (error)
+			goto err_mutex;
+
+		list_for_each_entry(owt_effect, &info->effect_head, list)
+			if (owt_effect->bank == WVFRM_BANK_OWT &&
+			    owt_effect->index > erase_effect->index)
+				owt_effect->index--;
+	}
+
+	list_del(&erase_effect->list);
+	kfree(erase_effect);
+
+err_mutex:
+	mutex_unlock(&info->lock);
+
+	pm_runtime_mark_last_busy(info->dev);
+	pm_runtime_put_autosuspend(info->dev);
+
+	info->erase_error = error;
+}
+
+static int vibra_erase(struct input_dev *dev, int effect_id)
+{
+	struct vibra_info *info = input_get_drvdata(dev);
+
+	info->erase_effect = &dev->ff->effects[effect_id];
+
+	queue_work(info->vibe_wq, &info->erase_work);
+	flush_work(&info->erase_work);
+
+	return info->erase_error;
+}
+
+static struct vibra_bank cs40l50_banks[] = {
+	{
+		.bank =		WVFRM_BANK_RAM,
+		.base_index =	CS40L50_RAM_INDEX_START,
+		.max_index =	CS40L50_RAM_INDEX_END,
+	},
+	{
+		.bank =		WVFRM_BANK_ROM,
+		.base_index =	CS40L50_ROM_INDEX_START,
+		.max_index =	CS40L50_ROM_INDEX_END,
+	},
+	{
+		.bank =		WVFRM_BANK_OWT,
+		.base_index =	CS40L50_RTH_INDEX_START,
+		.max_index =	CS40L50_RTH_INDEX_END,
+	},
+};
+
+static struct vibra_dsp cs40l50_dsp = {
+	.banks = cs40l50_banks,
+	.gpio_base_reg = CS40L50_GPIO_BASE,
+	.owt_base_reg = CS40L50_OWT_BASE,
+	.owt_offset_reg = CS40L50_OWT_NEXT,
+	.owt_size_reg = CS40L50_OWT_SIZE,
+	.mailbox_reg = CS40L50_DSP_QUEUE,
+	.push_owt_cmd = CS40L50_OWT_PUSH,
+	.delete_owt_cmd = CS40L50_OWT_DELETE,
+	.stop_cmd = CS40L50_STOP_PLAYBACK,
+};
+
+static void vibra_input_unregister(void *data)
+{
+	input_unregister_device((struct input_dev *)data);
+}
+
+static void vibra_remove_wq(void *data)
+{
+	struct vibra_info *info = data;
+
+	flush_workqueue(info->vibe_wq);
+	destroy_workqueue(info->vibe_wq);
+}
+
+static int cs40l50_vibra_probe(struct platform_device *pdev)
+{
+	struct cs40l50 *cs40l50 = dev_get_drvdata(pdev->dev.parent);
+	struct vibra_info *info;
+	int error;
+
+	info = devm_kzalloc(pdev->dev.parent, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = cs40l50->dev;
+	info->regmap = cs40l50->regmap;
+
+	info->input = devm_input_allocate_device(info->dev);
+	if (!info->input)
+		return -ENOMEM;
+
+	info->input->id.product = cs40l50->devid & 0xFFFF;
+	info->input->id.version = cs40l50->revid;
+	info->input->name = "cs40l50_vibra";
+
+	input_set_drvdata(info->input, info);
+	input_set_capability(info->input, EV_FF, FF_PERIODIC);
+	input_set_capability(info->input, EV_FF, FF_CUSTOM);
+
+	error = input_ff_create(info->input, FF_MAX_EFFECTS);
+	if (error) {
+		dev_err(info->dev, "Failed to create input device\n");
+		return error;
+	}
+
+	info->input->ff->upload = vibra_add;
+	info->input->ff->playback = vibra_playback;
+	info->input->ff->erase = vibra_erase;
+
+	INIT_LIST_HEAD(&info->effect_head);
+
+	info->dsp = cs40l50_dsp;
+
+	info->vibe_wq = alloc_ordered_workqueue("vibe_wq", 0);
+	if (!info->vibe_wq)
+		return -ENOMEM;
+
+	error = devm_add_action_or_reset(info->dev, vibra_remove_wq, info);
+	if (error)
+		return error;
+
+	mutex_init(&info->lock);
+
+	INIT_WORK(&info->vibe_start_work, vibra_start_worker);
+	INIT_WORK(&info->vibe_stop_work, vibra_stop_worker);
+	INIT_WORK(&info->erase_work, vibra_erase_worker);
+	INIT_WORK(&info->add_work, vibra_add_worker);
+
+	error = input_register_device(info->input);
+	if (error) {
+		dev_err(info->dev, "Failed to register input device\n");
+		input_free_device(info->input);
+		return error;
+	}
+
+	return devm_add_action_or_reset(info->dev, vibra_input_unregister,
+					info->input);
+}
+
+static const struct platform_device_id vibra_id_match[] = {
+	{ "cs40l50-vibra", },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, vibra_id_match);
+
+static struct platform_driver cs40l50_vibra_driver = {
+	.probe		= cs40l50_vibra_probe,
+	.id_table	= vibra_id_match,
+	.driver		= {
+		.name	= "cs40l50-vibra",
+	},
+};
+module_platform_driver(cs40l50_vibra_driver);
+
+MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
+MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[Charles Keepax]

On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
> 
> The input driver provides the interface for control of
> haptic effects through the device.
> 
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
> +#include <linux/input.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>

Need bitfield.h

Thanks,
Charles

Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[Dmitry Torokhov]

Hi James,

On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
> +static int vibra_add(struct input_dev *dev, struct ff_effect *effect,
> +		     struct ff_effect *old)
> +{
> +	struct vibra_info *info = input_get_drvdata(dev);
> +	u32 len = effect->u.periodic.custom_len;
> +
> +	if (effect->type != FF_PERIODIC || effect->u.periodic.waveform != FF_CUSTOM) {
> +		dev_err(info->dev, "Type (%#X) or waveform (%#X) unsupported\n",
> +			effect->type, effect->u.periodic.waveform);
> +		return -EINVAL;
> +	}
> +
> +	memcpy(&info->add_effect, effect, sizeof(struct ff_effect));

structures can be assigned, no need for memcpy.

> +
> +	info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
> +	if (!info->add_effect.u.periodic.custom_data)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(info->add_effect.u.periodic.custom_data,
> +			   effect->u.periodic.custom_data, sizeof(s16) * len)) {
> +		info->add_error = -EFAULT;
> +		goto out_free;
> +	}
> +
> +	queue_work(info->vibe_wq, &info->add_work);
> +	flush_work(&info->add_work);

I do not understand the need of scheduling a work here. You are
obviously in a sleeping context (otherwise you would not be able to
execute flush_work()) so you should be able to upload the effect right
here.

...

> +
> +static int vibra_playback(struct input_dev *dev, int effect_id, int val)
> +{
> +	struct vibra_info *info = input_get_drvdata(dev);
> +
> +	if (val > 0) {

value is supposed to signal how many times an effect should be repeated.
It looks like you are not handling this at all.

> +		info->start_effect = &dev->ff->effects[effect_id];
> +		queue_work(info->vibe_wq, &info->vibe_start_work);

The API allows playback of several effects at once, the way you have it
done here if multiple requests come at same time only one will be
handled.

> +	} else {
> +		queue_work(info->vibe_wq, &info->vibe_stop_work);

Which effect are you stopping? All of them? You need to stop a
particular one.

> +	}

Essentially you need a queue of requests and a single work handling all
of them...

...

> +
> +static int cs40l50_vibra_probe(struct platform_device *pdev)
> +{
> +	struct cs40l50 *cs40l50 = dev_get_drvdata(pdev->dev.parent);
> +	struct vibra_info *info;
> +	int error;
> +
> +	info = devm_kzalloc(pdev->dev.parent, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->dev = cs40l50->dev;
> +	info->regmap = cs40l50->regmap;
> +
> +	info->input = devm_input_allocate_device(info->dev);
> +	if (!info->input)
> +		return -ENOMEM;
> +
> +	info->input->id.product = cs40l50->devid & 0xFFFF;
> +	info->input->id.version = cs40l50->revid;
> +	info->input->name = "cs40l50_vibra";
> +
> +	input_set_drvdata(info->input, info);
> +	input_set_capability(info->input, EV_FF, FF_PERIODIC);
> +	input_set_capability(info->input, EV_FF, FF_CUSTOM);
> +
> +	error = input_ff_create(info->input, FF_MAX_EFFECTS);
> +	if (error) {
> +		dev_err(info->dev, "Failed to create input device\n");
> +		return error;
> +	}
> +
> +	info->input->ff->upload = vibra_add;
> +	info->input->ff->playback = vibra_playback;
> +	info->input->ff->erase = vibra_erase;
> +
> +	INIT_LIST_HEAD(&info->effect_head);
> +
> +	info->dsp = cs40l50_dsp;
> +
> +	info->vibe_wq = alloc_ordered_workqueue("vibe_wq", 0);
> +	if (!info->vibe_wq)
> +		return -ENOMEM;
> +
> +	error = devm_add_action_or_reset(info->dev, vibra_remove_wq, info);
> +	if (error)
> +		return error;

Why do you need a dedicated workqueue? So you can flush works?

> +
> +	mutex_init(&info->lock);
> +
> +	INIT_WORK(&info->vibe_start_work, vibra_start_worker);
> +	INIT_WORK(&info->vibe_stop_work, vibra_stop_worker);
> +	INIT_WORK(&info->erase_work, vibra_erase_worker);
> +	INIT_WORK(&info->add_work, vibra_add_worker);
> +
> +	error = input_register_device(info->input);
> +	if (error) {
> +		dev_err(info->dev, "Failed to register input device\n");
> +		input_free_device(info->input);

Not needed, you are using devm_input_allocate_device().

> +		return error;
> +	}
> +
> +	return devm_add_action_or_reset(info->dev, vibra_input_unregister,
> +					info->input);

Not needed, managed input devices will be unregistered automatically by
devm.

Thanks.

-- 
Dmitry

Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[James Ogletree]

Hi Dmitry,

Thank you for your excellent review. Just a few questions.

> On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:

>> +
>> + info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
>> + if (!info->add_effect.u.periodic.custom_data)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(info->add_effect.u.periodic.custom_data,
>> +   effect->u.periodic.custom_data, sizeof(s16) * len)) {
>> + info->add_error = -EFAULT;
>> + goto out_free;
>> + }
>> +
>> + queue_work(info->vibe_wq, &info->add_work);
>> + flush_work(&info->add_work);
> 
> I do not understand the need of scheduling a work here. You are
> obviously in a sleeping context (otherwise you would not be able to
> execute flush_work()) so you should be able to upload the effect right
> here.

Scheduling work here is to ensure its ordering with “playback" worker
items, which themselves are called in atomic context and so need
deferred work. I think this explains why we need a workqueue as well,
but please correct me.

> 
>> +
>> +static int vibra_playback(struct input_dev *dev, int effect_id, int val)
>> +{
>> + struct vibra_info *info = input_get_drvdata(dev);
>> +
>> + if (val > 0) {
> 
> value is supposed to signal how many times an effect should be repeated.
> It looks like you are not handling this at all.

For playbacks, we mandate that the input_event value field is set to either 1
or 0 to command either a start playback or stop playback respectively.
Values other than that should be rejected, so in the next version I will fix this
to explicitly check for 1 or 0.

> 
>> + info->start_effect = &dev->ff->effects[effect_id];
>> + queue_work(info->vibe_wq, &info->vibe_start_work);
> 
> The API allows playback of several effects at once, the way you have it
> done here if multiple requests come at same time only one will be
> handled.

I think I may need some clarification on this point. Why would concurrent
start/stop playback commands get dropped? It seems they would all be
added to the workqueue and executed eventually.

> 
>> + } else {
>> + queue_work(info->vibe_wq, &info->vibe_stop_work);
> 
> Which effect are you stopping? All of them? You need to stop a
> particular one.

Our implementation of “stop” stops all effects in flight which is intended.
That is probably unusual so I will add a comment here in the next
version.

Best,
James

Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[Dmitry Torokhov]

On Tue, Jan 09, 2024 at 10:03:02PM +0000, James Ogletree wrote:
> Hi Dmitry,
> 
> Thank you for your excellent review. Just a few questions.
> 
> > On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
> 
> >> +
> >> + info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
> >> + if (!info->add_effect.u.periodic.custom_data)
> >> + return -ENOMEM;
> >> +
> >> + if (copy_from_user(info->add_effect.u.periodic.custom_data,
> >> +   effect->u.periodic.custom_data, sizeof(s16) * len)) {
> >> + info->add_error = -EFAULT;
> >> + goto out_free;
> >> + }
> >> +
> >> + queue_work(info->vibe_wq, &info->add_work);
> >> + flush_work(&info->add_work);
> > 
> > I do not understand the need of scheduling a work here. You are
> > obviously in a sleeping context (otherwise you would not be able to
> > execute flush_work()) so you should be able to upload the effect right
> > here.
> 
> Scheduling work here is to ensure its ordering with “playback" worker
> items, which themselves are called in atomic context and so need
> deferred work. I think this explains why we need a workqueue as well,
> but please correct me.
> 
> > 
> >> +
> >> +static int vibra_playback(struct input_dev *dev, int effect_id, int val)
> >> +{
> >> + struct vibra_info *info = input_get_drvdata(dev);
> >> +
> >> + if (val > 0) {
> > 
> > value is supposed to signal how many times an effect should be repeated.
> > It looks like you are not handling this at all.
> 
> For playbacks, we mandate that the input_event value field is set to either 1

I am sorry, who is "we"?

> or 0 to command either a start playback or stop playback respectively.
> Values other than that should be rejected, so in the next version I will fix this
> to explicitly check for 1 or 0.

No, please implement the API properly.

> 
> > 
> >> + info->start_effect = &dev->ff->effects[effect_id];
> >> + queue_work(info->vibe_wq, &info->vibe_start_work);
> > 
> > The API allows playback of several effects at once, the way you have it
> > done here if multiple requests come at same time only one will be
> > handled.
> 
> I think I may need some clarification on this point. Why would concurrent
> start/stop playback commands get dropped? It seems they would all be
> added to the workqueue and executed eventually.

You only have one instance of vibe_start_work, as well as only one
"slot" to hold the effect you want to start. So if you issue 2 request
back to back to play effect 1 and 2 you are likely to end with
info->start_effect == 2 and that is what vibe_start_work handler will
observe, effectively dropping request to play effect 1 on the floor.

> 
> > 
> >> + } else {
> >> + queue_work(info->vibe_wq, &info->vibe_stop_work);
> > 
> > Which effect are you stopping? All of them? You need to stop a
> > particular one.
> 
> Our implementation of “stop” stops all effects in flight which is intended.
> That is probably unusual so I will add a comment here in the next
> version.

Again, please implement the driver properly, not define your own
carveouts for the expected behavior.

Thanks.

-- 
Dmitry

Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[James Ogletree]

> On Jan 9, 2024, at 4:31 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> On Tue, Jan 09, 2024 at 10:03:02PM +0000, James Ogletree wrote:
>> Hi Dmitry,
>> 
>> Thank you for your excellent review. Just a few questions.
>> 
>>> On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>> 
>>> On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
>>>> +
>>>> + info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
>>>> + if (!info->add_effect.u.periodic.custom_data)
>>>> + return -ENOMEM;
>>>> +
>>>> + if (copy_from_user(info->add_effect.u.periodic.custom_data,
>>>> +   effect->u.periodic.custom_data, sizeof(s16) * len)) {
>>>> + info->add_error = -EFAULT;
>>>> + goto out_free;
>>>> + }
>>>> +
>>>> + queue_work(info->vibe_wq, &info->add_work);
>>>> + flush_work(&info->add_work);
>>> 
>>> I do not understand the need of scheduling a work here. You are
>>> obviously in a sleeping context (otherwise you would not be able to
>>> execute flush_work()) so you should be able to upload the effect right
>>> here.
>> 
>> Scheduling work here is to ensure its ordering with “playback" worker
>> items, which themselves are called in atomic context and so need
>> deferred work. I think this explains why we need a workqueue as well,
>> but please correct me.
>> 
>>> 
>>>> +
>>>> +static int vibra_playback(struct input_dev *dev, int effect_id, int val)
>>>> +{
>>>> + struct vibra_info *info = input_get_drvdata(dev);
>>>> +
>>>> + if (val > 0) {
>>> 
>>> value is supposed to signal how many times an effect should be repeated.
>>> It looks like you are not handling this at all.
>> 
>> For playbacks, we mandate that the input_event value field is set to either 1
> 
> I am sorry, who is "we"?

Just a royal “I”. Apologies, no claim to authority intended here. :)

> 
>> or 0 to command either a start playback or stop playback respectively.
>> Values other than that should be rejected, so in the next version I will fix this
>> to explicitly check for 1 or 0.
> 
> No, please implement the API properly.

Ack.

> 
>> 
>>> 
>>>> + info->start_effect = &dev->ff->effects[effect_id];
>>>> + queue_work(info->vibe_wq, &info->vibe_start_work);
>>> 
>>> The API allows playback of several effects at once, the way you have it
>>> done here if multiple requests come at same time only one will be
>>> handled.
>> 
>> I think I may need some clarification on this point. Why would concurrent
>> start/stop playback commands get dropped? It seems they would all be
>> added to the workqueue and executed eventually.
> 
> You only have one instance of vibe_start_work, as well as only one
> "slot" to hold the effect you want to start. So if you issue 2 request
> back to back to play effect 1 and 2 you are likely to end with
> info->start_effect == 2 and that is what vibe_start_work handler will
> observe, effectively dropping request to play effect 1 on the floor.

Understood, ack.

> 
>> 
>>> 
>>>> + } else {
>>>> + queue_work(info->vibe_wq, &info->vibe_stop_work);
>>> 
>>> Which effect are you stopping? All of them? You need to stop a
>>> particular one.
>> 
>> Our implementation of “stop” stops all effects in flight which is intended.
>> That is probably unusual so I will add a comment here in the next
>> version.
> 
> Again, please implement the driver properly, not define your own
> carveouts for the expected behavior.

Ack, and a clarification question: the device is not actually able to
play multiple effects at once. In that case, does stopping a specific
effect entail just cancelling an effect in the queue?

Best,
James

Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[Dmitry Torokhov]

On Wed, Jan 10, 2024 at 02:36:55PM +0000, James Ogletree wrote:
> 
> > On Jan 9, 2024, at 4:31 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > On Tue, Jan 09, 2024 at 10:03:02PM +0000, James Ogletree wrote:
> >> Hi Dmitry,
> >> 
> >> Thank you for your excellent review. Just a few questions.
> >> 
> >>> On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >>> 
> >>> On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
> >>>> +
> >>>> + info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
> >>>> + if (!info->add_effect.u.periodic.custom_data)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + if (copy_from_user(info->add_effect.u.periodic.custom_data,
> >>>> +   effect->u.periodic.custom_data, sizeof(s16) * len)) {
> >>>> + info->add_error = -EFAULT;
> >>>> + goto out_free;
> >>>> + }
> >>>> +
> >>>> + queue_work(info->vibe_wq, &info->add_work);
> >>>> + flush_work(&info->add_work);
> >>> 
> >>> I do not understand the need of scheduling a work here. You are
> >>> obviously in a sleeping context (otherwise you would not be able to
> >>> execute flush_work()) so you should be able to upload the effect right
> >>> here.
> >> 
> >> Scheduling work here is to ensure its ordering with “playback" worker
> >> items, which themselves are called in atomic context and so need
> >> deferred work. I think this explains why we need a workqueue as well,
> >> but please correct me.
> >> 
> >>> 
> >>>> +
> >>>> +static int vibra_playback(struct input_dev *dev, int effect_id, int val)
> >>>> +{
> >>>> + struct vibra_info *info = input_get_drvdata(dev);
> >>>> +
> >>>> + if (val > 0) {
> >>> 
> >>> value is supposed to signal how many times an effect should be repeated.
> >>> It looks like you are not handling this at all.
> >> 
> >> For playbacks, we mandate that the input_event value field is set to either 1
> > 
> > I am sorry, who is "we"?
> 
> Just a royal “I”. Apologies, no claim to authority intended here. :)
> 
> > 
> >> or 0 to command either a start playback or stop playback respectively.
> >> Values other than that should be rejected, so in the next version I will fix this
> >> to explicitly check for 1 or 0.
> > 
> > No, please implement the API properly.
> 
> Ack.
> 
> > 
> >> 
> >>> 
> >>>> + info->start_effect = &dev->ff->effects[effect_id];
> >>>> + queue_work(info->vibe_wq, &info->vibe_start_work);
> >>> 
> >>> The API allows playback of several effects at once, the way you have it
> >>> done here if multiple requests come at same time only one will be
> >>> handled.
> >> 
> >> I think I may need some clarification on this point. Why would concurrent
> >> start/stop playback commands get dropped? It seems they would all be
> >> added to the workqueue and executed eventually.
> > 
> > You only have one instance of vibe_start_work, as well as only one
> > "slot" to hold the effect you want to start. So if you issue 2 request
> > back to back to play effect 1 and 2 you are likely to end with
> > info->start_effect == 2 and that is what vibe_start_work handler will
> > observe, effectively dropping request to play effect 1 on the floor.
> 
> Understood, ack.
> 
> > 
> >> 
> >>> 
> >>>> + } else {
> >>>> + queue_work(info->vibe_wq, &info->vibe_stop_work);
> >>> 
> >>> Which effect are you stopping? All of them? You need to stop a
> >>> particular one.
> >> 
> >> Our implementation of “stop” stops all effects in flight which is intended.
> >> That is probably unusual so I will add a comment here in the next
> >> version.
> > 
> > Again, please implement the driver properly, not define your own
> > carveouts for the expected behavior.
> 
> Ack, and a clarification question: the device is not actually able to
> play multiple effects at once. In that case, does stopping a specific
> effect entail just cancelling an effect in the queue?

In this case I believe the device should declare maximum number of
effects as 1. Userspace is supposed to determine maximum number of
simultaneously playable effects by issuing EVIOCGEFFECTS ioctl on the
corresponding event device.

Thanks.

-- 
Dmitry

Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[James Ogletree]

> On Jan 11, 2024, at 1:28 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> On Wed, Jan 10, 2024 at 02:36:55PM +0000, James Ogletree wrote:
>> 
>>> On Jan 9, 2024, at 4:31 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>> 
>>> On Tue, Jan 09, 2024 at 10:03:02PM +0000, James Ogletree wrote:
>>>> 
>>>> 
>>>>> On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>>>> 
>>>>> On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
>>>>>> + } else {
>>>>>> + queue_work(info->vibe_wq, &info->vibe_stop_work);
>>>>> 
>>>>> Which effect are you stopping? All of them? You need to stop a
>>>>> particular one.
>>>> 
>>>> Our implementation of “stop” stops all effects in flight which is intended.
>>>> That is probably unusual so I will add a comment here in the next
>>>> version.
>>> 
>>> Again, please implement the driver properly, not define your own
>>> carveouts for the expected behavior.
>> 
>> Ack, and a clarification question: the device is not actually able to
>> play multiple effects at once. In that case, does stopping a specific
>> effect entail just cancelling an effect in the queue?
> 
> In this case I believe the device should declare maximum number of
> effects as 1. Userspace is supposed to determine maximum number of
> simultaneously playable effects by issuing EVIOCGEFFECTS ioctl on the
> corresponding event device.

Is it possible to specify the device’s maximum simultaneous effects
without also restricting the number of effects the user can upload? It
looks like both are tied to ff->max_effects.

Best,
James

Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[James Ogletree]

Hi Dmitry,

> On Jan 12, 2024, at 9:41 AM, James Ogletree <James.Ogletree@cirrus.com> wrote:
> 
>> On Jan 11, 2024, at 1:28 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> 
>> On Wed, Jan 10, 2024 at 02:36:55PM +0000, James Ogletree wrote:
>>> 
>>>> On Jan 9, 2024, at 4:31 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>>> 
>>>> On Tue, Jan 09, 2024 at 10:03:02PM +0000, James Ogletree wrote:
>>>>> 
>>>>> 
>>>>>> On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>>>>> 
>>>>>> On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
>>>>>>> + } else {
>>>>>>> + queue_work(info->vibe_wq, &info->vibe_stop_work);
>>>>>> 
>>>>>> Which effect are you stopping? All of them? You need to stop a
>>>>>> particular one.
>>>>> 
>>>>> Our implementation of “stop” stops all effects in flight which is intended.
>>>>> That is probably unusual so I will add a comment here in the next
>>>>> version.
>>>> 
>>>> Again, please implement the driver properly, not define your own
>>>> carveouts for the expected behavior.
>>> 
>>> Ack, and a clarification question: the device is not actually able to
>>> play multiple effects at once. In that case, does stopping a specific
>>> effect entail just cancelling an effect in the queue?
>> 
>> In this case I believe the device should declare maximum number of
>> effects as 1. Userspace is supposed to determine maximum number of
>> simultaneously playable effects by issuing EVIOCGEFFECTS ioctl on the
>> corresponding event device.
> 
> Is it possible to specify the device’s maximum simultaneous effects
> without also restricting the number of effects the user can upload? It
> looks like both are tied to ff->max_effects.
> 
> Best,
> James
> 

Is there an opportunity here for a subsystem change to disassociate max
upload-able effects and max simultaneously playable effects, or if not what
do you advise in the case of a device in which the two differ? Or is this
a misuse of the subsystem in some way?

Best,
James

[PATCH v5 5/5] ASoC: cs40l50: Support I2S streaming to CS40L50

[James Ogletree]

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

The ASoC driver enables I2S streaming to the device.

Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
---
 MAINTAINERS                      |   1 +
 sound/soc/codecs/Kconfig         |  11 ++
 sound/soc/codecs/Makefile        |   2 +
 sound/soc/codecs/cs40l50-codec.c | 304 +++++++++++++++++++++++++++++++
 4 files changed, 318 insertions(+)
 create mode 100644 sound/soc/codecs/cs40l50-codec.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 24cfb4f017bb..fca2454a7a38 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4943,6 +4943,7 @@ F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
 F:	drivers/input/misc/cs40l*
 F:	drivers/mfd/cs40l*
 F:	include/linux/mfd/cs40l*
+F:	sound/soc/codecs/cs40l*
 
 CIRRUS LOGIC DSP FIRMWARE DRIVER
 M:	Simon Trimmer <simont@opensource.cirrus.com>
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index f1e1dbc509f6..9338a36c4c85 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -73,6 +73,7 @@ config SND_SOC_ALL_CODECS
 	imply SND_SOC_CS35L56_I2C
 	imply SND_SOC_CS35L56_SPI
 	imply SND_SOC_CS35L56_SDW
+	imply SND_SOC_CS40L50
 	imply SND_SOC_CS42L42
 	imply SND_SOC_CS42L42_SDW
 	imply SND_SOC_CS42L43
@@ -800,6 +801,16 @@ config SND_SOC_CS35L56_SDW
 	help
 	  Enable support for Cirrus Logic CS35L56 boosted amplifier with SoundWire control
 
+config SND_SOC_CS40L50
+	tristate "Cirrus Logic CS40L50 CODEC"
+	depends on MFD_CS40L50_CORE
+	help
+	  This option enables support for I2S streaming to Cirrus Logic CS40L50.
+
+		CS40L50 is a haptic driver with waveform memory, an integrated
+		DSP, and closed-loop algorithms. If built as a module, it will be
+		called snd-soc-cs40l50.
+
 config SND_SOC_CS42L42_CORE
 	tristate
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index a87e56938ce5..7e31f000774a 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -74,6 +74,7 @@ snd-soc-cs35l56-shared-objs := cs35l56-shared.o
 snd-soc-cs35l56-i2c-objs := cs35l56-i2c.o
 snd-soc-cs35l56-spi-objs := cs35l56-spi.o
 snd-soc-cs35l56-sdw-objs := cs35l56-sdw.o
+snd-soc-cs40l50-objs := cs40l50-codec.o
 snd-soc-cs42l42-objs := cs42l42.o
 snd-soc-cs42l42-i2c-objs := cs42l42-i2c.o
 snd-soc-cs42l42-sdw-objs := cs42l42-sdw.o
@@ -460,6 +461,7 @@ obj-$(CONFIG_SND_SOC_CS35L56_SHARED)	+= snd-soc-cs35l56-shared.o
 obj-$(CONFIG_SND_SOC_CS35L56_I2C)	+= snd-soc-cs35l56-i2c.o
 obj-$(CONFIG_SND_SOC_CS35L56_SPI)	+= snd-soc-cs35l56-spi.o
 obj-$(CONFIG_SND_SOC_CS35L56_SDW)	+= snd-soc-cs35l56-sdw.o
+obj-$(CONFIG_SND_SOC_CS40L50)		+= snd-soc-cs40l50.o
 obj-$(CONFIG_SND_SOC_CS42L42_CORE)	+= snd-soc-cs42l42.o
 obj-$(CONFIG_SND_SOC_CS42L42)	+= snd-soc-cs42l42-i2c.o
 obj-$(CONFIG_SND_SOC_CS42L42_SDW)	+= snd-soc-cs42l42-sdw.o
diff --git a/sound/soc/codecs/cs40l50-codec.c b/sound/soc/codecs/cs40l50-codec.c
new file mode 100644
index 000000000000..c4035719cf04
--- /dev/null
+++ b/sound/soc/codecs/cs40l50-codec.c
@@ -0,0 +1,304 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CS40L50 Advanced Haptic Driver with waveform memory,
+ * integrated DSP, and closed-loop algorithms
+ *
+ * Copyright 2023 Cirrus Logic, Inc.
+ *
+ * Author: James Ogletree <james.ogletree@cirrus.com>
+ */
+
+#include <linux/mfd/cs40l50.h>
+#include <linux/pm_runtime.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#define CS40L50_REFCLK_INPUT			0x2C04
+#define CS40L50_ASP_CONTROL2			0x4808
+#define CS40L50_ASP_DATA_CONTROL5		0x4840
+
+/* PLL Config */
+#define CS40L50_PLL_CLK_CFG_32768	0x00
+#define CS40L50_PLL_CLK_CFG_1536000	0x1B
+#define CS40L50_PLL_CLK_CFG_3072000	0x21
+#define CS40L50_PLL_CLK_CFG_6144000	0x28
+#define CS40L50_PLL_CLK_CFG_9600000	0x30
+#define CS40L50_PLL_CLK_CFG_12288000	0x33
+#define CS40L50_PLL_CLK_FRQ_32768	32768
+#define CS40L50_PLL_CLK_FRQ_1536000	1536000
+#define CS40L50_PLL_CLK_FRQ_3072000	3072000
+#define CS40L50_PLL_CLK_FRQ_6144000	6144000
+#define CS40L50_PLL_CLK_FRQ_9600000	9600000
+#define CS40L50_PLL_CLK_FRQ_12288000	12288000
+#define CS40L50_PLL_REFCLK_BCLK		0x0
+#define CS40L50_PLL_REFCLK_MCLK		0x5
+#define CS40L50_PLL_REFCLK_LOOP_MASK	BIT(11)
+#define CS40L50_PLL_REFCLK_OPEN_LOOP	1
+#define CS40L50_PLL_REFCLK_CLOSED_LOOP	0
+#define CS40L50_PLL_REFCLK_LOOP_SHIFT	11
+#define CS40L50_PLL_REFCLK_FREQ_MASK	GENMASK(10, 5)
+#define CS40L50_PLL_REFCLK_FREQ_SHIFT	5
+#define CS40L50_PLL_REFCLK_SEL_MASK	GENMASK(2, 0)
+
+/* ASP Config */
+#define CS40L50_ASP_RX_WIDTH_SHIFT	24
+#define CS40L50_ASP_RX_WIDTH_MASK	GENMASK(31, 24)
+#define CS40L50_ASP_RX_WL_MASK		GENMASK(5, 0)
+#define CS40L50_ASP_FSYNC_INV_MASK	BIT(2)
+#define CS40L50_ASP_BCLK_INV_MASK	BIT(6)
+#define CS40L50_ASP_FMT_MASK		GENMASK(10, 8)
+#define CS40L50_ASP_FMT_I2S		0x2
+#define CS40L50_FORMATS			(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
+
+struct cs40l50_pll_sysclk_config {
+	unsigned int freq;
+	unsigned int clk_cfg;
+};
+
+struct cs40l50_codec {
+	struct device *dev;
+	struct regmap *regmap;
+	unsigned int sysclk_rate;
+	unsigned int daifmt;
+};
+
+static const struct cs40l50_pll_sysclk_config cs40l50_pll_sysclk[] = {
+	{CS40L50_PLL_CLK_FRQ_32768, CS40L50_PLL_CLK_CFG_32768},
+	{CS40L50_PLL_CLK_FRQ_1536000, CS40L50_PLL_CLK_CFG_1536000},
+	{CS40L50_PLL_CLK_FRQ_3072000, CS40L50_PLL_CLK_CFG_3072000},
+	{CS40L50_PLL_CLK_FRQ_6144000, CS40L50_PLL_CLK_CFG_6144000},
+	{CS40L50_PLL_CLK_FRQ_9600000, CS40L50_PLL_CLK_CFG_9600000},
+	{CS40L50_PLL_CLK_FRQ_12288000, CS40L50_PLL_CLK_CFG_12288000},
+};
+
+static int cs40l50_get_clk_config(unsigned int freq, unsigned int *clk_cfg)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(cs40l50_pll_sysclk); i++) {
+		if (cs40l50_pll_sysclk[i].freq == freq) {
+			*clk_cfg = cs40l50_pll_sysclk[i].clk_cfg;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int cs40l50_swap_ext_clk(struct cs40l50_codec *codec, unsigned int clk_src)
+{
+	unsigned int clk_cfg;
+	int ret;
+
+	switch (clk_src) {
+	case CS40L50_PLL_REFCLK_BCLK:
+		ret = cs40l50_get_clk_config(codec->sysclk_rate, &clk_cfg);
+		if (ret)
+			return ret;
+		break;
+	case CS40L50_PLL_REFCLK_MCLK:
+		clk_cfg = CS40L50_PLL_CLK_CFG_32768;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(codec->regmap, CS40L50_REFCLK_INPUT,
+				 CS40L50_PLL_REFCLK_LOOP_MASK,
+				 CS40L50_PLL_REFCLK_OPEN_LOOP <<
+				 CS40L50_PLL_REFCLK_LOOP_SHIFT);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(codec->regmap, CS40L50_REFCLK_INPUT,
+				 CS40L50_PLL_REFCLK_FREQ_MASK |
+				 CS40L50_PLL_REFCLK_SEL_MASK,
+				 (clk_cfg << CS40L50_PLL_REFCLK_FREQ_SHIFT) | clk_src);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(codec->regmap, CS40L50_REFCLK_INPUT,
+				  CS40L50_PLL_REFCLK_LOOP_MASK,
+				  CS40L50_PLL_REFCLK_CLOSED_LOOP <<
+				  CS40L50_PLL_REFCLK_LOOP_SHIFT);
+}
+
+static int cs40l50_clk_en(struct snd_soc_dapm_widget *w,
+			  struct snd_kcontrol *kcontrol,
+			  int event)
+{
+	struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
+	struct cs40l50_codec *codec = snd_soc_component_get_drvdata(comp);
+	int ret;
+
+	switch (event) {
+	case SND_SOC_DAPM_POST_PMU:
+		ret = regmap_write(codec->regmap, CS40L50_DSP_QUEUE, CS40L50_STOP_PLAYBACK);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(codec->regmap, CS40L50_DSP_QUEUE, CS40L50_START_I2S);
+		if (ret)
+			return ret;
+
+		ret = cs40l50_swap_ext_clk(codec, CS40L50_PLL_REFCLK_BCLK);
+		if (ret)
+			return ret;
+		break;
+	case SND_SOC_DAPM_PRE_PMD:
+		ret = cs40l50_swap_ext_clk(codec, CS40L50_PLL_REFCLK_MCLK);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct snd_soc_dapm_widget cs40l50_dapm_widgets[] = {
+	SND_SOC_DAPM_SUPPLY_S("ASP PLL", 0, SND_SOC_NOPM, 0, 0, cs40l50_clk_en,
+		SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
+	SND_SOC_DAPM_AIF_IN("ASPRX1", NULL, 0, SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_AIF_IN("ASPRX2", NULL, 0, SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_OUTPUT("OUT"),
+};
+
+static const struct snd_soc_dapm_route cs40l50_dapm_routes[] = {
+	{ "ASP Playback", NULL, "ASP PLL" },
+	{ "ASPRX1", NULL, "ASP Playback" },
+	{ "ASPRX2", NULL, "ASP Playback" },
+
+	{ "OUT", NULL, "ASPRX1" },
+	{ "OUT", NULL, "ASPRX2" },
+};
+
+static int cs40l50_component_set_sysclk(struct snd_soc_component *component,
+		int clk_id, int source, unsigned int freq, int dir)
+{
+	struct cs40l50_codec *codec = snd_soc_component_get_drvdata(component);
+
+	codec->sysclk_rate = freq;
+
+	return 0;
+}
+
+static int cs40l50_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
+{
+	struct cs40l50_codec *codec = snd_soc_component_get_drvdata(codec_dai->component);
+
+	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS)
+		return -EINVAL;
+
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_NB_NF:
+		codec->daifmt = 0;
+		break;
+	case SND_SOC_DAIFMT_NB_IF:
+		codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK;
+		break;
+	case SND_SOC_DAIFMT_IB_NF:
+		codec->daifmt = CS40L50_ASP_BCLK_INV_MASK;
+		break;
+	case SND_SOC_DAIFMT_IB_IF:
+		codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK | CS40L50_ASP_BCLK_INV_MASK;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) == SND_SOC_DAIFMT_I2S)
+		codec->daifmt |= FIELD_PREP(CS40L50_ASP_FMT_MASK, CS40L50_ASP_FMT_I2S);
+
+	return 0;
+}
+
+static int cs40l50_hw_params(struct snd_pcm_substream *substream,
+			     struct snd_pcm_hw_params *params,
+			     struct snd_soc_dai *dai)
+{
+	struct cs40l50_codec *codec = snd_soc_component_get_drvdata(dai->component);
+	unsigned int asp_rx_wl = params_width(params);
+	int ret;
+
+	ret = regmap_update_bits(codec->regmap, CS40L50_ASP_DATA_CONTROL5,
+				 CS40L50_ASP_RX_WL_MASK, asp_rx_wl);
+	if (ret)
+		return ret;
+
+	codec->daifmt |= (asp_rx_wl << CS40L50_ASP_RX_WIDTH_SHIFT);
+
+	return regmap_update_bits(codec->regmap, CS40L50_ASP_CONTROL2,
+				  CS40L50_ASP_FSYNC_INV_MASK |
+				  CS40L50_ASP_BCLK_INV_MASK |
+				  CS40L50_ASP_FMT_MASK |
+				  CS40L50_ASP_RX_WIDTH_MASK, codec->daifmt);
+}
+
+static const struct snd_soc_dai_ops cs40l50_dai_ops = {
+	.set_fmt = cs40l50_set_dai_fmt,
+	.hw_params = cs40l50_hw_params,
+};
+
+static struct snd_soc_dai_driver cs40l50_dai[] = {
+	{
+		.name = "cs40l50-pcm",
+		.id = 0,
+		.playback = {
+			.stream_name = "ASP Playback",
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = SNDRV_PCM_RATE_48000,
+			.formats = CS40L50_FORMATS,
+		},
+		.ops = &cs40l50_dai_ops,
+		.symmetric_rate = 1,
+	},
+};
+
+static int cs40l50_codec_probe(struct snd_soc_component *component)
+{
+	struct cs40l50_codec *codec = snd_soc_component_get_drvdata(component);
+
+	codec->sysclk_rate = CS40L50_PLL_CLK_FRQ_1536000;
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver soc_codec_dev_cs40l50 = {
+	.probe = cs40l50_codec_probe,
+	.set_sysclk = cs40l50_component_set_sysclk,
+	.dapm_widgets = cs40l50_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(cs40l50_dapm_widgets),
+	.dapm_routes = cs40l50_dapm_routes,
+	.num_dapm_routes = ARRAY_SIZE(cs40l50_dapm_routes),
+};
+
+static int cs40l50_codec_driver_probe(struct platform_device *pdev)
+{
+	struct cs40l50 *cs40l50 = dev_get_drvdata(pdev->dev.parent);
+	struct cs40l50_codec *codec;
+
+	codec = devm_kzalloc(&pdev->dev, sizeof(*codec), GFP_KERNEL);
+	if (!codec)
+		return -ENOMEM;
+
+	codec->regmap = cs40l50->regmap;
+	codec->dev = &pdev->dev;
+
+	return devm_snd_soc_register_component(&pdev->dev, &soc_codec_dev_cs40l50,
+					       cs40l50_dai, ARRAY_SIZE(cs40l50_dai));
+}
+
+static struct platform_driver cs40l50_codec_driver = {
+	.probe = cs40l50_codec_driver_probe,
+	.driver = {
+		.name = "cs40l50-codec",
+	},
+};
+module_platform_driver(cs40l50_codec_driver);
+
+MODULE_DESCRIPTION("ASoC CS40L50 driver");
+MODULE_AUTHOR("James Ogletree <james.ogletree@cirrus.com>");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v5 5/5] ASoC: cs40l50: Support I2S streaming to CS40L50

[Charles Keepax]

On Thu, Jan 04, 2024 at 10:36:38PM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
> 
> The ASoC driver enables I2S streaming to the device.
> 
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/pm_runtime.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>

Need to also include bitfield.h for FIELD_PREP etc.

> +static int cs40l50_clk_en(struct snd_soc_dapm_widget *w,
> +			  struct snd_kcontrol *kcontrol,
> +			  int event)
> +{
> +	struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
> +	struct cs40l50_codec *codec = snd_soc_component_get_drvdata(comp);
> +	int ret;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		ret = regmap_write(codec->regmap, CS40L50_DSP_QUEUE, CS40L50_STOP_PLAYBACK);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(codec->regmap, CS40L50_DSP_QUEUE, CS40L50_START_I2S);
> +		if (ret)
> +			return ret;
> +

Feels weird that we don't wait for these two commands to be
acknowledged by the DSP before doing the clock swap. Is that
intentional? Is the DSP just guaranteed to be so fast it doesn't
matter, in which case a comment would be nice.

> +static int cs40l50_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
> +{
> +	struct cs40l50_codec *codec = snd_soc_component_get_drvdata(codec_dai->component);
> +
> +	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS)
> +		return -EINVAL;
> +
> +	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> +	case SND_SOC_DAIFMT_NB_NF:
> +		codec->daifmt = 0;
> +		break;
> +	case SND_SOC_DAIFMT_NB_IF:
> +		codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK;
> +		break;
> +	case SND_SOC_DAIFMT_IB_NF:
> +		codec->daifmt = CS40L50_ASP_BCLK_INV_MASK;
> +		break;
> +	case SND_SOC_DAIFMT_IB_IF:
> +		codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK | CS40L50_ASP_BCLK_INV_MASK;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) == SND_SOC_DAIFMT_I2S)
> +		codec->daifmt |= FIELD_PREP(CS40L50_ASP_FMT_MASK, CS40L50_ASP_FMT_I2S);

It feels unlikely the chip supports all formats with no
additional settings? Probably should have a switch for the
supported formats and return an error.

> +static struct snd_soc_dai_driver cs40l50_dai[] = {
> +	{
> +		.name = "cs40l50-pcm",
> +		.id = 0,
> +		.playback = {
> +			.stream_name = "ASP Playback",
> +			.channels_min = 1,
> +			.channels_max = 2,
> +			.rates = SNDRV_PCM_RATE_48000,
> +			.formats = CS40L50_FORMATS,
> +		},
> +		.ops = &cs40l50_dai_ops,
> +		.symmetric_rate = 1,

The symmetric_rate feels a bit redundant since we only have
playback supported.

Thanks,
Charles