[PATCH v5 0/2] ASoC: Add support for Conexant CX2092X DSP

[PATCH v5 0/2] ASoC: Add support for Conexant CX2092X DSP

[simon.ho.cnxt]

From: Simon Ho <simon.ho@conexant.com>

Thanks to Charles' review, this fixed things were pointed by him. 

Changes with v4:
	- Changed lable name to lower case.
	- Uses non-blocking sleep function msleep() instead of mdelay(). 
	- Fixed incorrect return code when sendcmd operation fails. 

Simon Ho (2):
  doc: cx2092x: Add DT bingings doc for CX2092X DSP
  ASoC: Add support for Conexant CX2092X DSP

 .../devicetree/bindings/sound/cnxt,cx2092x.txt     |  25 ++
 sound/soc/codecs/Kconfig                           |   6 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/cx2092x.c                         | 440 +++++++++++++++++++++
 sound/soc/codecs/cx2092x.h                         |  19 +
 5 files changed, 492 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/cnxt,cx2092x.txt
 create mode 100644 sound/soc/codecs/cx2092x.c
 create mode 100644 sound/soc/codecs/cx2092x.h

-- 
2.7.4

[PATCH v5 1/2] doc: cx2092x: Add DT bingings doc for CX2092X DSP

[simon.ho.cnxt]

From: Simon Ho <simon.ho@conexant.com>

Initial version of CX2092X device tree bindings document.

Signed-off-by: Simon Ho <simon.ho@conexant.com>
---
 .../devicetree/bindings/sound/cnxt,cx2092x.txt     | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/cnxt,cx2092x.txt

diff --git a/Documentation/devicetree/bindings/sound/cnxt,cx2092x.txt b/Documentation/devicetree/bindings/sound/cnxt,cx2092x.txt
new file mode 100644
index 0000000..8dd2428
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/cnxt,cx2092x.txt
@@ -0,0 +1,25 @@
+Conexant CX20921/CX209724 Voice Processor DSP
+
+The devices supports the I2C bus.
+
+Required properties:
+
+  - compatible: One of "cnxt,cx20921", "cnxt,cx20924".
+  - reg: The I2C address of the device for I2C, it should be <0x41>
+
+Optional properties:
+
+  - reset-gpios: A GPIO spec to define which pin is connected to the chip's
+		 !RESET pin. If specified, the driver will assert a hardware
+		 reset at probe time.
+
+CODEC input pins:
+  "MIC"	- Microphone input
+
+Example:
+
+codec: cx20921@41 {
+	compatible = "cnxt,cx20921";
+	reg = <0x41>;
+	reset-gpios = <&gpio1 23 0>;
+};
-- 
2.7.4

[PATCH v5 2/2] ASoC: Add support for Conexant CX2092X DSP

[simon.ho.cnxt]

From: Simon Ho <simon.ho@conexant.com>

Initial commit of Conexant CX20921/CX20924 I2S Audio DSP driver

The CX2092X devices are designed for virtual assisant application need to
be always open, listening for users to summon it. There is no any power
saving mode support on this device. The processed voice data will be sent
to automatic speech recognition (ASR) application for further processing.

Signed-off-by: Simon Ho <simon.ho@conexant.com>
---
 sound/soc/codecs/Kconfig   |   6 +
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/cx2092x.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/cx2092x.h |  19 ++
 4 files changed, 467 insertions(+)
 create mode 100644 sound/soc/codecs/cx2092x.c
 create mode 100644 sound/soc/codecs/cx2092x.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 70decab..2e1d468 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -64,6 +64,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_CS47L24 if MFD_CS47L24
 	select SND_SOC_CS53L30 if I2C
 	select SND_SOC_CX20442 if TTY
+	select SND_SOC_CX2092X if I2C
 	select SND_SOC_DA7210 if SND_SOC_I2C_AND_SPI
 	select SND_SOC_DA7213 if I2C
 	select SND_SOC_DA7218 if I2C
@@ -500,6 +501,11 @@ config SND_SOC_CX20442
 	tristate
 	depends on TTY
 
+config SND_SOC_CX2092X
+	tristate "Conexant CX20921/CX2094 CODEC (I2C)"
+	depends on I2C
+	select REGMAP_I2C
+
 config SND_SOC_JZ4740_CODEC
 	select REGMAP_MMIO
 	tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index bbef31e..11d3906 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -57,6 +57,7 @@ snd-soc-cs4349-objs := cs4349.o
 snd-soc-cs47l24-objs := cs47l24.o
 snd-soc-cs53l30-objs := cs53l30.o
 snd-soc-cx20442-objs := cx20442.o
+snd-soc-cx2092x-objs := cx2092x.o
 snd-soc-da7210-objs := da7210.o
 snd-soc-da7213-objs := da7213.o
 snd-soc-da7218-objs := da7218.o
@@ -290,6 +291,7 @@ obj-$(CONFIG_SND_SOC_CS4349)	+= snd-soc-cs4349.o
 obj-$(CONFIG_SND_SOC_CS47L24)	+= snd-soc-cs47l24.o
 obj-$(CONFIG_SND_SOC_CS53L30)	+= snd-soc-cs53l30.o
 obj-$(CONFIG_SND_SOC_CX20442)	+= snd-soc-cx20442.o
+obj-$(CONFIG_SND_SOC_CX2092X)	+= snd-soc-cx2092x.o
 obj-$(CONFIG_SND_SOC_DA7210)	+= snd-soc-da7210.o
 obj-$(CONFIG_SND_SOC_DA7213)	+= snd-soc-da7213.o
 obj-$(CONFIG_SND_SOC_DA7218)	+= snd-soc-da7218.o
diff --git a/sound/soc/codecs/cx2092x.c b/sound/soc/codecs/cx2092x.c
new file mode 100644
index 0000000..edad9c5
--- /dev/null
+++ b/sound/soc/codecs/cx2092x.c
@@ -0,0 +1,440 @@
+/*
+ * cx2092x.c -- CX20921 and CX20924 ALSA SoC Audio driver
+ *
+ * Copyright:   (C) 2017 Conexant Systems, Inc.
+ *
+ * This is based on Alexander Sverdlin's CS4271 driver code.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/i2c.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regmap.h>
+#include <sound/pcm.h>
+#include <sound/soc.h>
+#include "cx2092x.h"
+
+#define CX2092X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
+			 SNDRV_PCM_FMTBIT_S24_LE | \
+			 SNDRV_PCM_FMTBIT_S32_LE)
+
+#define CX2092X_CAPE_ID(a, b, c, d)  (((a) - 0x20) << 8 | \
+				      ((b) - 0x20) << 14 | \
+				      ((c) - 0x20) << 20 | \
+				      ((d) - 0x20) << 26)
+
+#define CX2092X_ID2CH_A(id)  (((((unsigned int)(id)) >> 8) & 0x3f) + 0x20)
+#define CX2092X_ID2CH_B(id)  (((((unsigned int)(id)) >> 14) & 0x3f) + 0x20)
+#define CX2092X_ID2CH_C(id)  (((((unsigned int)(id)) >> 20) & 0x3f) + 0x20)
+#define CX2092X_ID2CH_D(id)  (((((unsigned int)(id)) >> 26) & 0x3f) + 0x20)
+
+#define CX2092X_CONTROL(xname, xinfo, xget, xput, xaccess) { \
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
+	.access = xaccess, .info = xinfo, .get = xget, .put = xput, \
+	}
+
+#define CX2092X_CMD_GET(item)   ((item) |  0x0100)
+#define CX2092X_CMD_SIZE 13
+
+/*
+ * Defines the command format which is used to communicate with cx2092x device.
+ */
+struct cx2092x_cmd {
+	int	num_32b_words:16;   /* Indicates how many data to be sent.
+				     * If operation is successful, this will
+				     * be updated with the number of returned
+				     * data in word. one word == 4 bytes.
+				     */
+
+	u32	command_id:15;
+	u32	reply:1;            /* The device will set this flag once
+				     * the operation is complete.
+				     */
+	u32	app_module_id;
+	u32	data[CX2092X_CMD_SIZE]; /* Used for storing parameters and
+					 * receiving the returned data from
+					 * device.
+					 */
+};
+
+/* codec private data*/
+struct cx2092x_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct gpio_desc *gpiod_reset;
+	struct cx2092x_cmd cmd;
+	int cmd_res;
+};
+
+/*
+ * This functions takes cx2092x_cmd structure as input and output parameters
+ * to communicate CX2092X. If operation is successfully, it returns number of
+ * returned data and stored the returned data in "cmd->data" array.
+ * Otherwise, it returns the error code.
+ */
+static int cx2092x_sendcmd(struct snd_soc_codec *codec,
+			   struct cx2092x_cmd *cmd)
+{
+	struct cx2092x_priv *cx2092x = snd_soc_codec_get_drvdata(codec);
+	int ret = 0;
+	int num_32b_words = cmd->num_32b_words;
+	unsigned long time_out;
+	u32 *i2c_data = (u32 *)cmd;
+	int size = num_32b_words + 2;
+
+	/* calculate how many WORD that will be wrote to device*/
+	cmd->num_32b_words = cmd->command_id & CX2092X_CMD_GET(0) ?
+			     CX2092X_CMD_SIZE : num_32b_words;
+
+	/* write all command data except fo frist 4 bytes*/
+	ret = regmap_bulk_write(cx2092x->regmap, 4, &i2c_data[1], size - 1);
+	if (ret < 0) {
+		dev_err(cx2092x->dev,
+			"sendcmd: Failed to write command data, err = %d\n",
+			ret);
+		goto leave;
+	}
+
+	/* write first 4 bytes command data*/
+	ret = regmap_bulk_write(cx2092x->regmap, 0, i2c_data, 1);
+	if (ret < 0) {
+		dev_err(cx2092x->dev,
+			"sendcmd: Failed to write command, err = %d\n",	ret);
+		goto leave;
+	}
+
+	/* continuously read the first bytes data from device until
+	 * either timeout or the flag 'reply' is set.
+	 */
+	time_out = msecs_to_jiffies(2000);
+	time_out += jiffies;
+	do {
+		regmap_bulk_read(cx2092x->regmap, 0, &i2c_data[0], 1);
+		if (cmd->reply == 1)
+			break;
+		msleep(10);
+
+	} while (!time_after(jiffies, time_out));
+
+	if (cmd->reply == 1) {
+		ret = cmd->num_32b_words;
+		/* check if there is any returned data. If yes, then copy the
+		 * returned data to cmd->data array.
+		 */
+		if (cmd->num_32b_words > 0) {
+			regmap_bulk_read(cx2092x->regmap, 8, &i2c_data[2],
+					 cmd->num_32b_words);
+		/* print and return error code if operation is not successful*/
+		} else if (cmd->num_32b_words < 0) {
+			dev_err(cx2092x->dev, "sendcmd failed: %d\n",
+				cmd->num_32b_words);
+			ret = -EINVAL;
+		}
+	} else {
+		dev_err(cx2092x->dev, "SendCmd timeouti!\n");
+		ret = -EBUSY;
+	}
+
+leave:
+	return ret;
+}
+
+static int cmd_info(struct snd_kcontrol *kcontrol,
+		    struct snd_ctl_elem_info *uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES;
+	uinfo->count = sizeof(struct cx2092x_cmd);
+
+	return 0;
+}
+
+static int cmd_get(struct snd_kcontrol *kcontrol,
+		   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct cx2092x_priv *cx2092x =
+		snd_soc_component_get_drvdata(component);
+
+	memcpy(ucontrol->value.bytes.data, &cx2092x->cmd, sizeof(cx2092x->cmd));
+
+	return 0;
+}
+
+static int cmd_put(struct snd_kcontrol *kcontrol,
+		   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct cx2092x_priv *cx2092x = snd_soc_component_get_drvdata(component);
+	struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
+
+	memcpy(&cx2092x->cmd, ucontrol->value.bytes.data, sizeof(cx2092x->cmd));
+
+	cx2092x->cmd_res = cx2092x_sendcmd(codec, &cx2092x->cmd);
+
+	if (cx2092x->cmd_res < 0)
+		dev_err(codec->dev, "Failed to send cmd, ret = %d\n",
+			cx2092x->cmd_res);
+
+	return cx2092x->cmd_res < 0 ? cx2092x->cmd_res : 0;
+}
+
+static int mode_info(struct snd_kcontrol *kcontrol,
+		     struct snd_ctl_elem_info *uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES;
+	uinfo->count = 4;
+
+	return 0;
+}
+
+static int mode_get(struct snd_kcontrol *kcontrol,
+		    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
+	struct cx2092x_cmd cmd;
+	int ret = 0;
+
+	cmd.command_id = 0x12f; /*CX2092X_CMD_GET(SOS_RESOURCE);*/
+	cmd.reply = 0;
+	cmd.app_module_id = CX2092X_CAPE_ID('S', 'O', 'S', ' ');
+	cmd.num_32b_words = 1;
+	cmd.data[0] = CX2092X_CAPE_ID('C', 'T', 'R', 'L');
+
+	ret = cx2092x_sendcmd(codec, &cmd);
+	if (ret <= 0) {
+		dev_err(codec->dev, "Failed to get current mode, ret = %d\n",
+			ret);
+	} else {
+		ucontrol->value.bytes.data[0] = CX2092X_ID2CH_A(cmd.data[0]);
+		ucontrol->value.bytes.data[1] = CX2092X_ID2CH_B(cmd.data[0]);
+		ucontrol->value.bytes.data[2] = CX2092X_ID2CH_C(cmd.data[0]);
+		ucontrol->value.bytes.data[3] = CX2092X_ID2CH_D(cmd.data[0]);
+
+		dev_dbg(codec->dev, "Current mode = %c%c%c%c\n",
+			ucontrol->value.bytes.data[0],
+			ucontrol->value.bytes.data[1],
+			ucontrol->value.bytes.data[2],
+			ucontrol->value.bytes.data[3]);
+
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static int mode_put(struct snd_kcontrol *kcontrol,
+		    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
+	struct cx2092x_cmd cmd;
+	int ret = -1;
+
+	cmd.command_id = 4;
+	cmd.reply = 0;
+	cmd.app_module_id = CX2092X_CAPE_ID('C', 'T', 'R', 'L');
+	cmd.num_32b_words = 1;
+	cmd.data[0] = CX2092X_CAPE_ID(ucontrol->value.bytes.data[0],
+				      ucontrol->value.bytes.data[1],
+				      ucontrol->value.bytes.data[2],
+				      ucontrol->value.bytes.data[3]);
+
+	ret = cx2092x_sendcmd(codec, &cmd);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to set mode, ret =%d\n", ret);
+	} else {
+		dev_dbg(codec->dev, "Set mode successfully, ret = %d\n", ret);
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static const struct snd_kcontrol_new cx2092x_snd_controls[] = {
+	CX2092X_CONTROL("SendCmd", cmd_info, cmd_get, cmd_put,
+			SNDRV_CTL_ELEM_ACCESS_READ |
+			SNDRV_CTL_ELEM_ACCESS_WRITE |
+			SNDRV_CTL_ELEM_ACCESS_VOLATILE),
+	CX2092X_CONTROL("Mode", mode_info, mode_get, mode_put,
+			SNDRV_CTL_ELEM_ACCESS_READ |
+			SNDRV_CTL_ELEM_ACCESS_WRITE |
+			SNDRV_CTL_ELEM_ACCESS_VOLATILE),
+};
+
+static const struct snd_soc_dapm_widget cx2092x_dapm_widgets[] = {
+	SND_SOC_DAPM_AIF_OUT("Mic AIF", "Capture", 0,
+			     SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_INPUT("MIC"),
+};
+
+static const struct snd_soc_dapm_route cx2092x_intercon[] = {
+	{"Mic AIF", NULL, "MIC"},
+};
+
+static struct snd_soc_dai_driver soc_codec_cx2092x_dai[] = {
+	{
+		.name = "cx2092x-aif",
+		.capture = {
+			.stream_name = "Capture",
+			.channels_min = 2,
+			.channels_max = 2,
+			.rates = SNDRV_PCM_RATE_48000,
+			.formats = CX2092X_FORMATS,
+		},
+	},
+	{
+		.name = "cx2092x-dsp",
+		.capture = {
+			.stream_name = "AEC Ref",
+			.channels_min = 2,
+			.channels_max = 2,
+			.rates = SNDRV_PCM_RATE_48000,
+			.formats = CX2092X_FORMATS,
+		},
+	},
+};
+
+static int cx2092x_reset(struct snd_soc_codec *codec)
+{
+	struct cx2092x_priv *cx2092x = snd_soc_codec_get_drvdata(codec);
+
+	if (cx2092x->gpiod_reset) {
+		gpiod_set_value_cansleep(cx2092x->gpiod_reset, 0);
+		mdelay(10);
+		gpiod_set_value_cansleep(cx2092x->gpiod_reset, 1);
+	}
+
+	return 0;
+}
+
+static int cx2092x_probe(struct snd_soc_codec *codec)
+{
+	return cx2092x_reset(codec);
+}
+
+static int cx2092x_remove(struct snd_soc_codec *codec)
+{
+	struct cx2092x_priv *cx2092x = snd_soc_codec_get_drvdata(codec);
+
+	if (cx2092x->gpiod_reset)
+		/* Set codec to the reset state */
+		gpiod_set_value_cansleep(cx2092x->gpiod_reset, 0);
+
+	return 0;
+}
+
+static const struct snd_soc_codec_driver soc_codec_driver_cx2092x = {
+	.probe = cx2092x_probe,
+	.remove = cx2092x_remove,
+
+	.component_driver = {
+		.controls = cx2092x_snd_controls,
+		.num_controls = ARRAY_SIZE(cx2092x_snd_controls),
+		.dapm_widgets = cx2092x_dapm_widgets,
+		.num_dapm_widgets = ARRAY_SIZE(cx2092x_dapm_widgets),
+		.dapm_routes = cx2092x_intercon,
+		.num_dapm_routes = ARRAY_SIZE(cx2092x_intercon),
+	},
+};
+EXPORT_SYMBOL_GPL(soc_codec_driver_cx2092x);
+
+static bool cx2092x_volatile_register(struct device *dev, unsigned int reg)
+{
+	return true; /*all register are volatile*/
+}
+
+const struct regmap_config cx2092x_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = CX2092X_REG_MAX,
+	.cache_type = REGCACHE_NONE,
+	.volatile_reg = cx2092x_volatile_register,
+	.val_format_endian = REGMAP_ENDIAN_NATIVE,
+};
+EXPORT_SYMBOL_GPL(cx2092x_regmap_config);
+
+static int cx2092x_i2c_probe(struct i2c_client *i2c,
+			     const struct i2c_device_id *id)
+{
+	struct cx2092x_priv *cx2092x;
+	int ret;
+	struct device *dev = &i2c->dev;
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i2c(i2c, &cx2092x_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	cx2092x = devm_kzalloc(dev, sizeof(*cx2092x), GFP_KERNEL);
+	if (!cx2092x)
+		return -ENOMEM;
+
+	/* GPIOs */
+	cx2092x->gpiod_reset = devm_gpiod_get_optional(dev, "reset",
+						       GPIOD_OUT_LOW);
+	if (IS_ERR(cx2092x->gpiod_reset))
+		return PTR_ERR(cx2092x->gpiod_reset);
+
+	dev_set_drvdata(dev, cx2092x);
+	cx2092x->regmap = regmap;
+	cx2092x->dev = dev;
+
+	ret = snd_soc_register_codec(cx2092x->dev, &soc_codec_driver_cx2092x,
+				     soc_codec_cx2092x_dai,
+				     ARRAY_SIZE(soc_codec_cx2092x_dai));
+	if (ret < 0)
+		dev_err(dev, "Failed to register codec: %d\n", ret);
+	else
+		dev_dbg(dev, "%s: Register codec.\n", __func__);
+
+	return ret;
+}
+
+static int cx2092x_i2c_remove(struct i2c_client *client)
+{
+	snd_soc_unregister_codec(&client->dev);
+
+	return 0;
+}
+
+const struct of_device_id cx2092x_dt_ids[] = {
+	{ .compatible = "cnxt,cx20921", },
+	{ .compatible = "cnxt,cx20924", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cx2092x_dt_ids);
+
+static const struct i2c_device_id cx2092x_i2c_id[] = {
+	{"cx20921", 0},
+	{"cx20924", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, cx2092x_i2c_id);
+
+static struct i2c_driver cx2092x_i2c_driver = {
+	.driver = {
+		.name = "cx2092x",
+		.of_match_table = of_match_ptr(cx2092x_dt_ids),
+	},
+	.id_table = cx2092x_i2c_id,
+	.probe = cx2092x_i2c_probe,
+	.remove = cx2092x_i2c_remove,
+};
+
+module_i2c_driver(cx2092x_i2c_driver);
+
+MODULE_DESCRIPTION("ASoC CX2092X ALSA SoC Driver");
+MODULE_AUTHOR("Simon Ho <simon.ho@conexant.com>");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/cx2092x.h b/sound/soc/codecs/cx2092x.h
new file mode 100644
index 0000000..09dd610
--- /dev/null
+++ b/sound/soc/codecs/cx2092x.h
@@ -0,0 +1,19 @@
+/*
+ * cx2092x.h -- CX20921 and CX20924 Audio driver
+ *
+ * Copyright:   (C) 2017 Conexant Systems, Inc.
+ *
+ * This is based on Alexander Sverdlin's CS4271 driver code.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __CX2092X_PRIV_H__
+#define __CX2092X_PRIV_H__
+
+#define CX2092X_REG_MAX 0x2000
+
+#endif
-- 
2.7.4

Re: [PATCH v5 2/2] ASoC: Add support for Conexant CX2092X DSP

[Pierre-Louis Bossart]

On 4/6/17 5:04 AM, simon.ho.cnxt@gmail.com wrote:
> From: Simon Ho <simon.ho@conexant.com>
>
> Initial commit of Conexant CX20921/CX20924 I2S Audio DSP driver
>
> The CX2092X devices are designed for virtual assisant application need to
> be always open, listening for users to summon it. There is no any power
> saving mode support on this device. The processed voice data will be sent
> to automatic speech recognition (ASR) application for further processing.
>
> Signed-off-by: Simon Ho <simon.ho@conexant.com>
> ---
>  sound/soc/codecs/Kconfig   |   6 +
>  sound/soc/codecs/Makefile  |   2 +
>  sound/soc/codecs/cx2092x.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
>  sound/soc/codecs/cx2092x.h |  19 ++
>  4 files changed, 467 insertions(+)
>  create mode 100644 sound/soc/codecs/cx2092x.c
>  create mode 100644 sound/soc/codecs/cx2092x.h
>
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 70decab..2e1d468 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -64,6 +64,7 @@ config SND_SOC_ALL_CODECS
>  	select SND_SOC_CS47L24 if MFD_CS47L24
>  	select SND_SOC_CS53L30 if I2C
>  	select SND_SOC_CX20442 if TTY
> +	select SND_SOC_CX2092X if I2C
>  	select SND_SOC_DA7210 if SND_SOC_I2C_AND_SPI
>  	select SND_SOC_DA7213 if I2C
>  	select SND_SOC_DA7218 if I2C
> @@ -500,6 +501,11 @@ config SND_SOC_CX20442
>  	tristate
>  	depends on TTY
>
> +config SND_SOC_CX2092X
> +	tristate "Conexant CX20921/CX2094 CODEC (I2C)"
> +	depends on I2C
> +	select REGMAP_I2C
> +
>  config SND_SOC_JZ4740_CODEC
>  	select REGMAP_MMIO
>  	tristate
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index bbef31e..11d3906 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -57,6 +57,7 @@ snd-soc-cs4349-objs := cs4349.o
>  snd-soc-cs47l24-objs := cs47l24.o
>  snd-soc-cs53l30-objs := cs53l30.o
>  snd-soc-cx20442-objs := cx20442.o
> +snd-soc-cx2092x-objs := cx2092x.o
>  snd-soc-da7210-objs := da7210.o
>  snd-soc-da7213-objs := da7213.o
>  snd-soc-da7218-objs := da7218.o
> @@ -290,6 +291,7 @@ obj-$(CONFIG_SND_SOC_CS4349)	+= snd-soc-cs4349.o
>  obj-$(CONFIG_SND_SOC_CS47L24)	+= snd-soc-cs47l24.o
>  obj-$(CONFIG_SND_SOC_CS53L30)	+= snd-soc-cs53l30.o
>  obj-$(CONFIG_SND_SOC_CX20442)	+= snd-soc-cx20442.o
> +obj-$(CONFIG_SND_SOC_CX2092X)	+= snd-soc-cx2092x.o
>  obj-$(CONFIG_SND_SOC_DA7210)	+= snd-soc-da7210.o
>  obj-$(CONFIG_SND_SOC_DA7213)	+= snd-soc-da7213.o
>  obj-$(CONFIG_SND_SOC_DA7218)	+= snd-soc-da7218.o
> diff --git a/sound/soc/codecs/cx2092x.c b/sound/soc/codecs/cx2092x.c
> new file mode 100644
> index 0000000..edad9c5
> --- /dev/null
> +++ b/sound/soc/codecs/cx2092x.c
> @@ -0,0 +1,440 @@
> +/*
> + * cx2092x.c -- CX20921 and CX20924 ALSA SoC Audio driver
> + *
> + * Copyright:   (C) 2017 Conexant Systems, Inc.
> + *
> + * This is based on Alexander Sverdlin's CS4271 driver code.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regmap.h>
> +#include <sound/pcm.h>
> +#include <sound/soc.h>
> +#include "cx2092x.h"
> +
> +#define CX2092X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
> +			 SNDRV_PCM_FMTBIT_S24_LE | \
> +			 SNDRV_PCM_FMTBIT_S32_LE)
> +
> +#define CX2092X_CAPE_ID(a, b, c, d)  (((a) - 0x20) << 8 | \
> +				      ((b) - 0x20) << 14 | \
> +				      ((c) - 0x20) << 20 | \
> +				      ((d) - 0x20) << 26)
> +
> +#define CX2092X_ID2CH_A(id)  (((((unsigned int)(id)) >> 8) & 0x3f) + 0x20)
> +#define CX2092X_ID2CH_B(id)  (((((unsigned int)(id)) >> 14) & 0x3f) + 0x20)
> +#define CX2092X_ID2CH_C(id)  (((((unsigned int)(id)) >> 20) & 0x3f) + 0x20)
> +#define CX2092X_ID2CH_D(id)  (((((unsigned int)(id)) >> 26) & 0x3f) + 0x20)
> +
> +#define CX2092X_CONTROL(xname, xinfo, xget, xput, xaccess) { \
> +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
> +	.access = xaccess, .info = xinfo, .get = xget, .put = xput, \
> +	}
> +
> +#define CX2092X_CMD_GET(item)   ((item) |  0x0100)
> +#define CX2092X_CMD_SIZE 13
> +
> +/*
> + * Defines the command format which is used to communicate with cx2092x device.
> + */
> +struct cx2092x_cmd {
> +	int	num_32b_words:16;   /* Indicates how many data to be sent.
> +				     * If operation is successful, this will
> +				     * be updated with the number of returned
> +				     * data in word. one word == 4 bytes.
> +				     */
> +
> +	u32	command_id:15;
> +	u32	reply:1;            /* The device will set this flag once
> +				     * the operation is complete.
> +				     */
> +	u32	app_module_id;
> +	u32	data[CX2092X_CMD_SIZE]; /* Used for storing parameters and
> +					 * receiving the returned data from
> +					 * device.
> +					 */
> +};
> +
> +/* codec private data*/
> +struct cx2092x_priv {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct gpio_desc *gpiod_reset;
> +	struct cx2092x_cmd cmd;
> +	int cmd_res;
> +};
> +
> +/*
> + * This functions takes cx2092x_cmd structure as input and output parameters
> + * to communicate CX2092X. If operation is successfully, it returns number of
> + * returned data and stored the returned data in "cmd->data" array.
> + * Otherwise, it returns the error code.
> + */
> +static int cx2092x_sendcmd(struct snd_soc_codec *codec,
> +			   struct cx2092x_cmd *cmd)
> +{
> +	struct cx2092x_priv *cx2092x = snd_soc_codec_get_drvdata(codec);
> +	int ret = 0;
> +	int num_32b_words = cmd->num_32b_words;
> +	unsigned long time_out;
> +	u32 *i2c_data = (u32 *)cmd;
> +	int size = num_32b_words + 2;
> +
> +	/* calculate how many WORD that will be wrote to device*/
> +	cmd->num_32b_words = cmd->command_id & CX2092X_CMD_GET(0) ?
> +			     CX2092X_CMD_SIZE : num_32b_words;
> +
> +	/* write all command data except fo frist 4 bytes*/

for first.

> +	ret = regmap_bulk_write(cx2092x->regmap, 4, &i2c_data[1], size - 1);
> +	if (ret < 0) {
> +		dev_err(cx2092x->dev,
> +			"sendcmd: Failed to write command data, err = %d\n",
> +			ret);
> +		goto leave;
> +	}
> +
> +	/* write first 4 bytes command data*/
> +	ret = regmap_bulk_write(cx2092x->regmap, 0, i2c_data, 1);
> +	if (ret < 0) {
> +		dev_err(cx2092x->dev,
> +			"sendcmd: Failed to write command, err = %d\n",	ret);
> +		goto leave;
> +	}
> +
> +	/* continuously read the first bytes data from device until
> +	 * either timeout or the flag 'reply' is set.
> +	 */
> +	time_out = msecs_to_jiffies(2000);
> +	time_out += jiffies;
> +	do {
> +		regmap_bulk_read(cx2092x->regmap, 0, &i2c_data[0], 1);
> +		if (cmd->reply == 1)
> +			break;
> +		msleep(10);
> +
> +	} while (!time_after(jiffies, time_out));
> +
> +	if (cmd->reply == 1) {
> +		ret = cmd->num_32b_words;
> +		/* check if there is any returned data. If yes, then copy the
> +		 * returned data to cmd->data array.
> +		 */
> +		if (cmd->num_32b_words > 0) {
> +			regmap_bulk_read(cx2092x->regmap, 8, &i2c_data[2],
> +					 cmd->num_32b_words);
> +		/* print and return error code if operation is not successful*/
> +		} else if (cmd->num_32b_words < 0) {
> +			dev_err(cx2092x->dev, "sendcmd failed: %d\n",
> +				cmd->num_32b_words);
> +			ret = -EINVAL;
> +		}

so if num_32b_words is zero what happens? it's not clear if the case is 
omitted intentionally or the error case is not handled.

> +	} else {
> +		dev_err(cx2092x->dev, "SendCmd timeouti!\n");
> +		ret = -EBUSY;
> +	}
> +
> +leave:
> +	return ret;
> +}
> +
> +static int cmd_info(struct snd_kcontrol *kcontrol,
> +		    struct snd_ctl_elem_info *uinfo)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES;
> +	uinfo->count = sizeof(struct cx2092x_cmd);
> +
> +	return 0;
> +}
> +
> +static int cmd_get(struct snd_kcontrol *kcontrol,
> +		   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct cx2092x_priv *cx2092x =
> +		snd_soc_component_get_drvdata(component);
> +
> +	memcpy(ucontrol->value.bytes.data, &cx2092x->cmd, sizeof(cx2092x->cmd));
> +
> +	return 0;
> +}
> +
> +static int cmd_put(struct snd_kcontrol *kcontrol,
> +		   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct cx2092x_priv *cx2092x = snd_soc_component_get_drvdata(component);
> +	struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
> +
> +	memcpy(&cx2092x->cmd, ucontrol->value.bytes.data, sizeof(cx2092x->cmd));
> +
> +	cx2092x->cmd_res = cx2092x_sendcmd(codec, &cx2092x->cmd);
> +
> +	if (cx2092x->cmd_res < 0)
> +		dev_err(codec->dev, "Failed to send cmd, ret = %d\n",
> +			cx2092x->cmd_res);
> +
> +	return cx2092x->cmd_res < 0 ? cx2092x->cmd_res : 0;
> +}
> +
> +static int mode_info(struct snd_kcontrol *kcontrol,
> +		     struct snd_ctl_elem_info *uinfo)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES;
> +	uinfo->count = 4;
> +
> +	return 0;
> +}
> +
> +static int mode_get(struct snd_kcontrol *kcontrol,
> +		    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
> +	struct cx2092x_cmd cmd;
> +	int ret = 0;
> +
> +	cmd.command_id = 0x12f; /*CX2092X_CMD_GET(SOS_RESOURCE);*/
> +	cmd.reply = 0;
> +	cmd.app_module_id = CX2092X_CAPE_ID('S', 'O', 'S', ' ');
> +	cmd.num_32b_words = 1;
> +	cmd.data[0] = CX2092X_CAPE_ID('C', 'T', 'R', 'L');
> +
> +	ret = cx2092x_sendcmd(codec, &cmd);
> +	if (ret <= 0) {
> +		dev_err(codec->dev, "Failed to get current mode, ret = %d\n",
> +			ret);
> +	} else {
> +		ucontrol->value.bytes.data[0] = CX2092X_ID2CH_A(cmd.data[0]);
> +		ucontrol->value.bytes.data[1] = CX2092X_ID2CH_B(cmd.data[0]);
> +		ucontrol->value.bytes.data[2] = CX2092X_ID2CH_C(cmd.data[0]);
> +		ucontrol->value.bytes.data[3] = CX2092X_ID2CH_D(cmd.data[0]);
> +
> +		dev_dbg(codec->dev, "Current mode = %c%c%c%c\n",
> +			ucontrol->value.bytes.data[0],
> +			ucontrol->value.bytes.data[1],
> +			ucontrol->value.bytes.data[2],
> +			ucontrol->value.bytes.data[3]);
> +
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static int mode_put(struct snd_kcontrol *kcontrol,
> +		    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
> +	struct cx2092x_cmd cmd;
> +	int ret = -1;
> +
> +	cmd.command_id = 4;
> +	cmd.reply = 0;
> +	cmd.app_module_id = CX2092X_CAPE_ID('C', 'T', 'R', 'L');
> +	cmd.num_32b_words = 1;
> +	cmd.data[0] = CX2092X_CAPE_ID(ucontrol->value.bytes.data[0],
> +				      ucontrol->value.bytes.data[1],
> +				      ucontrol->value.bytes.data[2],
> +				      ucontrol->value.bytes.data[3]);
> +
> +	ret = cx2092x_sendcmd(codec, &cmd);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Failed to set mode, ret =%d\n", ret);
> +	} else {
> +		dev_dbg(codec->dev, "Set mode successfully, ret = %d\n", ret);
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct snd_kcontrol_new cx2092x_snd_controls[] = {
> +	CX2092X_CONTROL("SendCmd", cmd_info, cmd_get, cmd_put,
> +			SNDRV_CTL_ELEM_ACCESS_READ |
> +			SNDRV_CTL_ELEM_ACCESS_WRITE |
> +			SNDRV_CTL_ELEM_ACCESS_VOLATILE),
> +	CX2092X_CONTROL("Mode", mode_info, mode_get, mode_put,
> +			SNDRV_CTL_ELEM_ACCESS_READ |
> +			SNDRV_CTL_ELEM_ACCESS_WRITE |
> +			SNDRV_CTL_ELEM_ACCESS_VOLATILE),
> +};
> +
> +static const struct snd_soc_dapm_widget cx2092x_dapm_widgets[] = {
> +	SND_SOC_DAPM_AIF_OUT("Mic AIF", "Capture", 0,
> +			     SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_INPUT("MIC"),
> +};
> +
> +static const struct snd_soc_dapm_route cx2092x_intercon[] = {
> +	{"Mic AIF", NULL, "MIC"},
> +};
> +
> +static struct snd_soc_dai_driver soc_codec_cx2092x_dai[] = {
> +	{
> +		.name = "cx2092x-aif",
> +		.capture = {
> +			.stream_name = "Capture",
> +			.channels_min = 2,
> +			.channels_max = 2,
> +			.rates = SNDRV_PCM_RATE_48000,
> +			.formats = CX2092X_FORMATS,
> +		},
> +	},
> +	{
> +		.name = "cx2092x-dsp",
> +		.capture = {
> +			.stream_name = "AEC Ref",
> +			.channels_min = 2,
> +			.channels_max = 2,
> +			.rates = SNDRV_PCM_RATE_48000,
> +			.formats = CX2092X_FORMATS,
> +		},
> +	},
> +};
> +
> +static int cx2092x_reset(struct snd_soc_codec *codec)
> +{
> +	struct cx2092x_priv *cx2092x = snd_soc_codec_get_drvdata(codec);
> +
> +	if (cx2092x->gpiod_reset) {
> +		gpiod_set_value_cansleep(cx2092x->gpiod_reset, 0);
> +		mdelay(10);

msleep()?
> +		gpiod_set_value_cansleep(cx2092x->gpiod_reset, 1);
> +	}
> +
> +	return 0;
> +}
> +
> +static int cx2092x_probe(struct snd_soc_codec *codec)
> +{
> +	return cx2092x_reset(codec);
> +}
> +
> +static int cx2092x_remove(struct snd_soc_codec *codec)
> +{
> +	struct cx2092x_priv *cx2092x = snd_soc_codec_get_drvdata(codec);
> +
> +	if (cx2092x->gpiod_reset)
> +		/* Set codec to the reset state */
> +		gpiod_set_value_cansleep(cx2092x->gpiod_reset, 0);
> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_codec_driver soc_codec_driver_cx2092x = {
> +	.probe = cx2092x_probe,
> +	.remove = cx2092x_remove,
> +
> +	.component_driver = {
> +		.controls = cx2092x_snd_controls,
> +		.num_controls = ARRAY_SIZE(cx2092x_snd_controls),
> +		.dapm_widgets = cx2092x_dapm_widgets,
> +		.num_dapm_widgets = ARRAY_SIZE(cx2092x_dapm_widgets),
> +		.dapm_routes = cx2092x_intercon,
> +		.num_dapm_routes = ARRAY_SIZE(cx2092x_intercon),
> +	},
> +};
> +EXPORT_SYMBOL_GPL(soc_codec_driver_cx2092x);
> +
> +static bool cx2092x_volatile_register(struct device *dev, unsigned int reg)
> +{
> +	return true; /*all register are volatile*/
> +}
> +
> +const struct regmap_config cx2092x_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = CX2092X_REG_MAX,
> +	.cache_type = REGCACHE_NONE,
> +	.volatile_reg = cx2092x_volatile_register,
> +	.val_format_endian = REGMAP_ENDIAN_NATIVE,
> +};
> +EXPORT_SYMBOL_GPL(cx2092x_regmap_config);
> +
> +static int cx2092x_i2c_probe(struct i2c_client *i2c,
> +			     const struct i2c_device_id *id)
> +{
> +	struct cx2092x_priv *cx2092x;
> +	int ret;
> +	struct device *dev = &i2c->dev;
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i2c(i2c, &cx2092x_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	cx2092x = devm_kzalloc(dev, sizeof(*cx2092x), GFP_KERNEL);
> +	if (!cx2092x)
> +		return -ENOMEM;
> +
> +	/* GPIOs */
> +	cx2092x->gpiod_reset = devm_gpiod_get_optional(dev, "reset",
> +						       GPIOD_OUT_LOW);
> +	if (IS_ERR(cx2092x->gpiod_reset))
> +		return PTR_ERR(cx2092x->gpiod_reset);
> +
> +	dev_set_drvdata(dev, cx2092x);
> +	cx2092x->regmap = regmap;
> +	cx2092x->dev = dev;
> +
> +	ret = snd_soc_register_codec(cx2092x->dev, &soc_codec_driver_cx2092x,
> +				     soc_codec_cx2092x_dai,
> +				     ARRAY_SIZE(soc_codec_cx2092x_dai));
> +	if (ret < 0)
> +		dev_err(dev, "Failed to register codec: %d\n", ret);
> +	else
> +		dev_dbg(dev, "%s: Register codec.\n", __func__);
> +
> +	return ret;
> +}
> +
> +static int cx2092x_i2c_remove(struct i2c_client *client)
> +{
> +	snd_soc_unregister_codec(&client->dev);
> +
> +	return 0;
> +}
> +
> +const struct of_device_id cx2092x_dt_ids[] = {
> +	{ .compatible = "cnxt,cx20921", },
> +	{ .compatible = "cnxt,cx20924", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, cx2092x_dt_ids);
> +
> +static const struct i2c_device_id cx2092x_i2c_id[] = {
> +	{"cx20921", 0},
> +	{"cx20924", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, cx2092x_i2c_id);
> +
> +static struct i2c_driver cx2092x_i2c_driver = {
> +	.driver = {
> +		.name = "cx2092x",
> +		.of_match_table = of_match_ptr(cx2092x_dt_ids),
> +	},
> +	.id_table = cx2092x_i2c_id,
> +	.probe = cx2092x_i2c_probe,
> +	.remove = cx2092x_i2c_remove,
> +};
> +
> +module_i2c_driver(cx2092x_i2c_driver);
> +
> +MODULE_DESCRIPTION("ASoC CX2092X ALSA SoC Driver");
> +MODULE_AUTHOR("Simon Ho <simon.ho@conexant.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/sound/soc/codecs/cx2092x.h b/sound/soc/codecs/cx2092x.h
> new file mode 100644
> index 0000000..09dd610
> --- /dev/null
> +++ b/sound/soc/codecs/cx2092x.h
> @@ -0,0 +1,19 @@
> +/*
> + * cx2092x.h -- CX20921 and CX20924 Audio driver
> + *
> + * Copyright:   (C) 2017 Conexant Systems, Inc.
> + *
> + * This is based on Alexander Sverdlin's CS4271 driver code.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef __CX2092X_PRIV_H__
> +#define __CX2092X_PRIV_H__
> +
> +#define CX2092X_REG_MAX 0x2000
> +
> +#endif
>

Re: [PATCH v5 2/2] ASoC: Add support for Conexant CX2092X DSP

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1768 bytes --]

On Thu, Apr 06, 2017 at 11:04:34AM +0800, simon.ho.cnxt@gmail.com wrote:
> From: Simon Ho <simon.ho@conexant.com>
> 
> Initial commit of Conexant CX20921/CX20924 I2S Audio DSP driver
> 
> The CX2092X devices are designed for virtual assisant application need to
> be always open, listening for users to summon it. There is no any power
> saving mode support on this device. The processed voice data will be sent
> to automatic speech recognition (ASR) application for further processing.

This is still fundamentally just passing undocumented commands straight
through to the device (presumably to a firmware flashed onto the device
since there's not even a firmware download happening) without any
substantial integration with the rest of the system.  That still doesn't
really feel like a Linux driver at all, it seems more like it's just
punching a hole through the Linux driver stack for proprietary software
to do its thing.  I'm also not clear how this can even implement always
on speech recognition, there seems to be no interrupt support which I'd
have thought would be a requirement.

There are other vendors with voice processing features, the drivers for
all these devices should be presenting interfaces that look similar to
each other.  What those drivers doing treaming the speech data to the
host via a normal PCM which blocks while there's no speech data, that is
much better integrated than this.

You really need to address this, a big part of how Linux has got to
where it is today is that it doesn't just have custom interfaces for
each device but instead generalizes things so that we share good
practice and features between devices and users can write Linux
applications rather than vendor specific applications.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 2/2] ASoC: Add support for Conexant CX2092X DSP

[Simon Ho]

On Fri, Apr 07, 2017 at 12:37:37PM +0100, Mark Brown wrote:
> On Thu, Apr 06, 2017 at 11:04:34AM +0800, simon.ho.cnxt@gmail.com wrote:
> > From: Simon Ho <simon.ho@conexant.com>
> > 
> > Initial commit of Conexant CX20921/CX20924 I2S Audio DSP driver
> > 
> > The CX2092X devices are designed for virtual assisant application need to
> > be always open, listening for users to summon it. There is no any power
> > saving mode support on this device. The processed voice data will be sent
> > to automatic speech recognition (ASR) application for further processing.

> 
> This is still fundamentally just passing undocumented commands straight
> through to the device (presumably to a firmware flashed onto the device
> since there's not even a firmware download happening) without any
> substantial integration with the rest of the system.  That still doesn't
> really feel like a Linux driver at all, it seems more like it's just
> punching a hole through the Linux driver stack for proprietary software
> to do its thing.  I'm also not clear how this can even implement always
> on speech recognition, there seems to be no interrupt support which I'd
> have thought would be a requirement.
> 

Thanks for review and Yes, there is a SPI memory for firmware and 
platform-specific configuration. The device can load and apply 
confguration automatically upon device is power on. No configuration 
is needed from driver side.

The purpose of the tunnel interface are for debugng and tunning purpose.
The manufactor can use the them to customized the firmware to suit their
needs. furtherore, the commands are platform specific and also firmware
version specific. So that it is impossible to provide such commands in 
kernel unless we change our firmware design and policy.

The device have GPIO pin can be configured as interrupt out pin, this 
feaures is option, not a must for SR application actually. In addition,
I think the interrupt is platfrom-specpfic should be configured by 
Machine driver, am I right? 

> There are other vendors with voice processing features, the drivers for
> all these devices should be presenting interfaces that look similar to
> each other.  What those drivers doing treaming the speech data to the
> host via a normal PCM which blocks while there's no speech data, that is
> much better integrated than this.
> 
> You really need to address this, a big part of how Linux has got to
> where it is today is that it doesn't just have custom interfaces for
> each device but instead generalizes things so that we share good
> practice and features between devices and users can write Linux
> applications rather than vendor specific applications.

Just like DMic, I imagine. The DMIC is not configurable, but it still
requires a driver to communicate with the reset of the system. This
won't prevent people from developing the application for DMIC to extend
its functionality. 

The device send the normal PCM data to host as it claimed by codec
driver, and we confirmed that the device can work with most SR 
software without problem.  

Re: [PATCH v5 2/2] ASoC: Add support for Conexant CX2092X DSP

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2301 bytes --]

On Tue, Apr 11, 2017 at 03:43:40AM +0800, Simon Ho wrote:

> The purpose of the tunnel interface are for debugng and tunning purpose.
> The manufactor can use the them to customized the firmware to suit their
> needs. furtherore, the commands are platform specific and also firmware
> version specific. So that it is impossible to provide such commands in 
> kernel unless we change our firmware design and policy.

Changing that policy does sound like a good idea...  in any case if this
genuinely is just for tuning then I'd suggest removing this from your
initial submission and adding it as a separate patch so as not to slow
down the review of the bit that's used in production systems.

> The device have GPIO pin can be configured as interrupt out pin, this 
> feaures is option, not a must for SR application actually. In addition,
> I think the interrupt is platfrom-specpfic should be configured by 
> Machine driver, am I right? 

No, I'd not expect the machine driver to need to get involved for basic
interrupt functionality - look at how other drivers do interrupts.

> > You really need to address this, a big part of how Linux has got to
> > where it is today is that it doesn't just have custom interfaces for
> > each device but instead generalizes things so that we share good
> > practice and features between devices and users can write Linux
> > applications rather than vendor specific applications.

> Just like DMic, I imagine. The DMIC is not configurable, but it still
> requires a driver to communicate with the reset of the system. This
> won't prevent people from developing the application for DMIC to extend
> its functionality. 

Some components are indeed very simple but they still require
integration with the rest of the system.

> The device send the normal PCM data to host as it claimed by codec
> driver, and we confirmed that the device can work with most SR 
> software without problem.  

Could you please explain how this device works and integrates in the
system, I'm completely at a loss as to how this is supposed to work.
You appear to be saying that the device simply passes through audio and
provides no information to the rest of the system.  That makes it
unclear what function the device is performing.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]