* [PATCH 1/3] mfd: cros_ec: Add commands to control codec
@ 2018-11-27 12:00 Cheng-Yi Chiang
2018-11-27 12:00 ` [PATCH 2/3] ASoC: Documentation: Add google,cros-ec-codec Cheng-Yi Chiang
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Cheng-Yi Chiang @ 2018-11-27 12:00 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Brown, alsa-devel, dgreid, tzungbi, Rohit kumar, Cheng-Yi Chiang
Add EC host commands to control codec on EC.
Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
include/linux/mfd/cros_ec_commands.h | 94 ++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 07fe4ea0361d3..a3ac792f4915b 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -4077,6 +4077,100 @@ struct __ec_align1 ec_response_i2c_passthru_protect {
uint8_t status; /* Status flags (0: unlocked, 1: locked) */
};
+/*****************************************************************************/
+/* Commands for I2S recording on audio codec. */
+
+#define EC_CMD_CODEC_I2S 0x00BC
+
+enum ec_codec_i2s_subcmd {
+ EC_CODEC_SET_SAMPLE_DEPTH = 0x0,
+ EC_CODEC_SET_GAIN = 0x1,
+ EC_CODEC_GET_GAIN = 0x2,
+ EC_CODEC_I2S_ENABLE = 0x3,
+ EC_CODEC_I2S_SET_CONFIG = 0x4,
+ EC_CODEC_I2S_SET_TDM_CONFIG = 0x5,
+ EC_CODEC_I2S_SET_BCLK = 0x6,
+};
+
+enum ec_sample_depth_value {
+ EC_CODEC_SAMPLE_DEPTH_16 = 0,
+ EC_CODEC_SAMPLE_DEPTH_24 = 1,
+};
+
+enum ec_i2s_config {
+ EC_DAI_FMT_I2S = 0,
+ EC_DAI_FMT_RIGHT_J = 1,
+ EC_DAI_FMT_LEFT_J = 2,
+ EC_DAI_FMT_PCM_A = 3,
+ EC_DAI_FMT_PCM_B = 4,
+ EC_DAI_FMT_PCM_TDM = 5,
+};
+
+struct ec_param_codec_i2s {
+ /*
+ * enum ec_codec_i2s_subcmd
+ */
+ uint8_t cmd;
+ union {
+ /*
+ * EC_CODEC_SET_SAMPLE_DEPTH
+ * Value should be one of ec_sample_depth_value.
+ */
+ uint8_t depth;
+
+ /*
+ * EC_CODEC_SET_GAIN
+ * Value should be 0~43 for both channels.
+ */
+ struct ec_param_codec_i2s_set_gain {
+ uint8_t left;
+ uint8_t right;
+ } __packed gain;
+
+ /*
+ * EC_CODEC_I2S_ENABLE
+ * 1 to enable, 0 to disable.
+ */
+ uint8_t i2s_enable;
+
+ /*
+ * EC_CODEC_I2S_SET_COFNIG
+ * Value should be one of ec_i2s_config.
+ */
+ uint8_t i2s_config;
+
+ /*
+ * EC_CODEC_I2S_SET_TDM_CONFIG
+ * Value should be one of ec_i2s_config.
+ */
+ struct ec_param_codec_i2s_tdm {
+ /*
+ * 0 to 496
+ */
+ int16_t ch0_delay;
+ /*
+ * -1 to 496
+ */
+ int16_t ch1_delay;
+ uint8_t adjacent_to_ch0;
+ uint8_t adjacent_to_ch1;
+ } __packed tdm_param;
+
+ /*
+ * EC_CODEC_I2S_SET_BCLK
+ */
+ uint32_t bclk;
+ };
+} __packed;
+
+/*
+ * For subcommand EC_CODEC_GET_GAIN.
+ */
+struct ec_response_codec_gain {
+ uint8_t left;
+ uint8_t right;
+} __packed;
+
/*****************************************************************************/
/* System commands */
--
2.20.0.rc0.387.gc7a69e6b6c-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] ASoC: Documentation: Add google,cros-ec-codec
2018-11-27 12:00 [PATCH 1/3] mfd: cros_ec: Add commands to control codec Cheng-Yi Chiang
@ 2018-11-27 12:00 ` Cheng-Yi Chiang
2018-11-27 12:00 ` [PATCH 3/3] ASoC: cros_ec_codec: Add codec driver for Cros EC Cheng-Yi Chiang
2018-11-28 9:34 ` [PATCH 1/3] mfd: cros_ec: Add commands to control codec Lee Jones
2 siblings, 0 replies; 13+ messages in thread
From: Cheng-Yi Chiang @ 2018-11-27 12:00 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Brown, alsa-devel, dgreid, tzungbi, Rohit kumar, Cheng-Yi Chiang
Add documentation for Chrome EC codec driver.
Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
.../bindings/sound/google,cros-ec-codec.txt | 24 +++++++++++++++++++
MAINTAINERS | 5 ++++
2 files changed, 29 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
diff --git a/Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt b/Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
new file mode 100644
index 0000000000000..57718382b3a36
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
@@ -0,0 +1,24 @@
+* Audio codec controlled by ChromeOS EC
+
+Google's ChromeOS EC codec is a digital mic codec provided by the
+Embedded Controller (EC) and is controlled via a host-command interface.
+
+An EC codec node should only be found as a sub-node of the EC node (see
+Documentation/devicetree/bindings/mfd/cros-ec.txt).
+
+Required properties:
+- compatible: Must contain "google,cros-ec-codec"
+- #sound-dai-cells: Should be 1. The cell specifies number of DAIs.
+
+Example:
+
+cros-ec@0 {
+ compatible = "google,cros-ec-spi";
+
+ ...
+
+ cros_ec_codec: ec-codec {
+ compatible = "google,cros-ec-codec";
+ #sound-dai-cells = <1>;
+ };
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 91b4218195aac..5cf8ab296cc61 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3552,6 +3552,11 @@ S: Maintained
T: git git://git.kernel.org/pub/scm/linux/kernel/git/bleung/chrome-platform.git
F: drivers/platform/chrome/
+CHROME EC CODEC DRIVER
+M: Cheng-Yi Chiang <cychiang@chromium.org>
+S: Maintained
+F: Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
+
CIRRUS LOGIC AUDIO CODEC DRIVERS
M: Brian Austin <brian.austin@cirrus.com>
M: Paul Handrigan <Paul.Handrigan@cirrus.com>
--
2.20.0.rc0.387.gc7a69e6b6c-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] ASoC: cros_ec_codec: Add codec driver for Cros EC
2018-11-27 12:00 [PATCH 1/3] mfd: cros_ec: Add commands to control codec Cheng-Yi Chiang
2018-11-27 12:00 ` [PATCH 2/3] ASoC: Documentation: Add google,cros-ec-codec Cheng-Yi Chiang
@ 2018-11-27 12:00 ` Cheng-Yi Chiang
2018-11-29 12:42 ` [alsa-devel] " Enric Balletbo Serra
2018-11-28 9:34 ` [PATCH 1/3] mfd: cros_ec: Add commands to control codec Lee Jones
2 siblings, 1 reply; 13+ messages in thread
From: Cheng-Yi Chiang @ 2018-11-27 12:00 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Brown, alsa-devel, dgreid, tzungbi, Rohit kumar, Cheng-Yi Chiang
Add a codec driver to control ChromeOS EC codec.
Use EC Host command to enable/disable I2S recording and control other
configurations.
Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
MAINTAINERS | 1 +
sound/soc/codecs/Kconfig | 8 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/cros_ec_codec.c | 499 +++++++++++++++++++++++++++++++
sound/soc/codecs/cros_ec_codec.h | 33 ++
5 files changed, 543 insertions(+)
create mode 100644 sound/soc/codecs/cros_ec_codec.c
create mode 100644 sound/soc/codecs/cros_ec_codec.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 5cf8ab296cc61..f1999ef19d1cc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3556,6 +3556,7 @@ CHROME EC CODEC DRIVER
M: Cheng-Yi Chiang <cychiang@chromium.org>
S: Maintained
F: Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
+F: sound/soc/codecs/cros_ec_codec.*
CIRRUS LOGIC AUDIO CODEC DRIVERS
M: Brian Austin <brian.austin@cirrus.com>
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index efb095dbcd714..3e3e9294c0259 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -49,6 +49,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_BT_SCO
select SND_SOC_BD28623
select SND_SOC_CQ0093VC
+ select SND_SOC_CROS_EC_CODEC
select SND_SOC_CS35L32 if I2C
select SND_SOC_CS35L33 if I2C
select SND_SOC_CS35L34 if I2C
@@ -445,6 +446,13 @@ config SND_SOC_CPCAP
config SND_SOC_CQ0093VC
tristate
+config SND_SOC_CROS_EC_CODEC
+ tristate "codec driver for Cros EC"
+ depends on MFD_CROS_EC
+ help
+ If you say yes here you will get support for the
+ Chrome OS Embedded Controller's Audio Codec.
+
config SND_SOC_CS35L32
tristate "Cirrus Logic CS35L32 CODEC"
depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 7ae7c85e8219f..ff05c260c5776 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -41,6 +41,7 @@ snd-soc-bd28623-objs := bd28623.o
snd-soc-bt-sco-objs := bt-sco.o
snd-soc-cpcap-objs := cpcap.o
snd-soc-cq93vc-objs := cq93vc.o
+snd-soc-cros-ec-codec-objs := cros_ec_codec.o
snd-soc-cs35l32-objs := cs35l32.o
snd-soc-cs35l33-objs := cs35l33.o
snd-soc-cs35l34-objs := cs35l34.o
@@ -301,6 +302,7 @@ obj-$(CONFIG_SND_SOC_BD28623) += snd-soc-bd28623.o
obj-$(CONFIG_SND_SOC_BT_SCO) += snd-soc-bt-sco.o
obj-$(CONFIG_SND_SOC_CQ0093VC) += snd-soc-cq93vc.o
obj-$(CONFIG_SND_SOC_CPCAP) += snd-soc-cpcap.o
+obj-$(CONFIG_SND_SOC_CROS_EC_CODEC) += snd-soc-cros-ec-codec.o
obj-$(CONFIG_SND_SOC_CS35L32) += snd-soc-cs35l32.o
obj-$(CONFIG_SND_SOC_CS35L33) += snd-soc-cs35l33.o
obj-$(CONFIG_SND_SOC_CS35L34) += snd-soc-cs35l34.o
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
new file mode 100644
index 0000000000000..e24174c11a7de
--- /dev/null
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cros_ec_codec - Driver for Chrome OS Embedded Controller codec.
+ *
+ * Copyright 2018 Google, Inc
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * This driver uses the cros-ec interface to communicate with the Chrome OS
+ * EC for audio function.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/module.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+#include <linux/platform_device.h>
+
+#include "cros_ec_codec.h"
+
+#define MAX_GAIN 43
+
+#define DRV_NAME "cros-ec-codec"
+
+static const DECLARE_TLV_DB_SCALE(ec_mic_gain_tlv, 0, 100, 0);
+/*
+ * Wrapper for EC command.
+ */
+static int ec_command(struct snd_soc_component *component, int version,
+ int command, uint8_t *outdata, int outsize,
+ uint8_t *indata, int insize)
+{
+ struct cros_ec_codec_data *codec_data = snd_soc_component_get_drvdata(
+ component);
+ struct cros_ec_device *ec_device = codec_data->ec_device;
+ struct cros_ec_command *msg;
+ int ret;
+
+ msg = kzalloc(sizeof(*msg) + max(insize, outsize), GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ msg->version = version;
+ msg->command = command;
+ msg->outsize = outsize;
+ msg->insize = insize;
+
+ if (outsize)
+ memcpy(msg->data, outdata, outsize);
+
+ ret = cros_ec_cmd_xfer_status(ec_device, msg);
+ if (ret > 0 && insize)
+ memcpy(indata, msg->data, insize);
+
+ kfree(msg);
+ return ret;
+}
+
+static int set_i2s_config(struct snd_soc_component *component,
+ enum ec_i2s_config i2s_config)
+{
+ struct ec_param_codec_i2s param;
+ int ret;
+
+ dev_dbg(component->dev, "%s set I2S format to %u\n", __func__,
+ i2s_config);
+
+ param.cmd = EC_CODEC_I2S_SET_CONFIG;
+ param.i2s_config = i2s_config;
+
+ ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
+ (uint8_t *)¶m, sizeof(param),
+ NULL, 0);
+ if (ret < 0) {
+ dev_err(component->dev,
+ "set I2S format to %u command returned 0x%x\n",
+ i2s_config, ret);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int cros_ec_i2s_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+ struct snd_soc_component *component = dai->component;
+ enum ec_i2s_config i2s_config;
+
+ dev_dbg(component->dev, "%s enter\n", __func__);
+
+ switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+ case SND_SOC_DAIFMT_CBS_CFS:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+ case SND_SOC_DAIFMT_NB_NF:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+ case SND_SOC_DAIFMT_I2S:
+ i2s_config = EC_DAI_FMT_I2S;
+ break;
+
+ case SND_SOC_DAIFMT_RIGHT_J:
+ i2s_config = EC_DAI_FMT_RIGHT_J;
+ break;
+
+ case SND_SOC_DAIFMT_LEFT_J:
+ i2s_config = EC_DAI_FMT_LEFT_J;
+ break;
+
+ case SND_SOC_DAIFMT_DSP_A:
+ i2s_config = EC_DAI_FMT_PCM_A;
+ break;
+
+ case SND_SOC_DAIFMT_DSP_B:
+ i2s_config = EC_DAI_FMT_PCM_B;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ set_i2s_config(component, i2s_config);
+
+ dev_dbg(component->dev, "%s set I2S DAI format\n", __func__);
+
+ return 0;
+}
+
+static int set_i2s_sample_depth(struct snd_soc_component *component,
+ enum ec_sample_depth_value depth)
+{
+ struct ec_param_codec_i2s param;
+ int ret;
+
+ dev_dbg(component->dev, "%s set depth to %u\n", __func__, depth);
+
+ param.cmd = EC_CODEC_SET_SAMPLE_DEPTH;
+ param.depth = depth;
+
+ ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
+ (uint8_t *)¶m, sizeof(param),
+ NULL, 0);
+ if (ret < 0) {
+ dev_err(component->dev, "I2S sample depth %u returned 0x%x\n",
+ depth, ret);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int set_bclk(struct snd_soc_component *component, uint32_t bclk)
+{
+ struct ec_param_codec_i2s param;
+ int ret;
+
+ dev_dbg(component->dev, "%s set i2s bclk to %u\n", __func__, bclk);
+
+ param.cmd = EC_CODEC_I2S_SET_BCLK;
+ param.bclk = bclk;
+
+ ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
+ (uint8_t *)¶m, sizeof(param),
+ NULL, 0);
+ if (ret < 0) {
+ dev_err(component->dev, "I2S set bclk %u command returned 0x%x\n",
+ bclk, ret);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int cros_ec_i2s_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
+{
+ struct snd_soc_component *component = dai->component;
+ int frame_size;
+ unsigned int rate, bclk;
+ int ret;
+
+ frame_size = snd_soc_params_to_frame_size(params);
+ if (frame_size < 0) {
+ dev_err(component->dev, "Unsupported frame size: %d\n",
+ frame_size);
+ return -EINVAL;
+ }
+
+ rate = params_rate(params);
+ if (rate != 48000) {
+ dev_err(component->dev, "Unsupported rate\n");
+ return -EINVAL;
+ }
+
+ switch (params_format(params)) {
+ case SNDRV_PCM_FORMAT_S16_LE:
+ ret = set_i2s_sample_depth(component, EC_CODEC_SAMPLE_DEPTH_16);
+ break;
+ case SNDRV_PCM_FORMAT_S24_LE:
+ ret = set_i2s_sample_depth(component, EC_CODEC_SAMPLE_DEPTH_24);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (ret)
+ return ret;
+
+ bclk = snd_soc_params_to_bclk(params);
+ ret = set_bclk(component, bclk);
+
+ return ret;
+}
+
+static const struct snd_soc_dai_ops cros_ec_i2s_dai_ops = {
+ .hw_params = cros_ec_i2s_hw_params,
+ .set_fmt = cros_ec_i2s_set_dai_fmt,
+};
+
+struct snd_soc_dai_driver cros_ec_dai[] = {
+ {
+ .name = "cros_ec_codec I2S",
+ .id = 0,
+ .capture = {
+ .stream_name = "I2S Capture",
+ .channels_min = 2,
+ .channels_max = 2,
+ .rates = SNDRV_PCM_RATE_48000,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE |
+ SNDRV_PCM_FMTBIT_S24_LE,
+ },
+ .ops = &cros_ec_i2s_dai_ops,
+ }
+};
+
+static int get_ec_mic_gain(struct snd_soc_component *component,
+ uint8_t *left, uint8_t *right)
+{
+ struct ec_param_codec_i2s param;
+ struct ec_response_codec_gain resp;
+ int ret;
+
+ dev_dbg(component->dev, "%s get mic gain\n", __func__);
+
+ param.cmd = EC_CODEC_GET_GAIN;
+
+ ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
+ (uint8_t *)¶m, sizeof(param),
+ (uint8_t *)&resp, sizeof(resp));
+ if (ret < 0) {
+ dev_err(component->dev, "I2S get gain command returned 0x%x\n",
+ ret);
+ return -EINVAL;
+ }
+
+ *left = resp.left;
+ *right = resp.right;
+
+ dev_dbg(component->dev, "%s get mic gain %u, %u\n", __func__,
+ *left, *right);
+
+ return 0;
+}
+
+static int mic_gain_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct snd_soc_component *component = snd_soc_kcontrol_component(
+ kcontrol);
+ uint8_t left, right;
+ int ret;
+
+ ret = get_ec_mic_gain(component, &left, &right);
+
+ if (ret)
+ return ret;
+
+ ucontrol->value.integer.value[0] = left;
+ ucontrol->value.integer.value[1] = right;
+
+ return 0;
+}
+
+static int set_ec_mic_gain(struct snd_soc_component *component,
+ uint8_t left, uint8_t right)
+{
+ struct ec_param_codec_i2s param;
+ int ret;
+
+ dev_dbg(component->dev, "%s set mic gain to %u, %u\n",
+ __func__, left, right);
+
+ param.cmd = EC_CODEC_SET_GAIN;
+ param.gain.left = left;
+ param.gain.right = right;
+
+ ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
+ (uint8_t *)¶m, sizeof(param),
+ NULL, 0);
+ if (ret < 0) {
+ dev_err(component->dev, "I2S set gain command returned 0x%x\n",
+ ret);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int mic_gain_put(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct snd_soc_component *component = snd_soc_kcontrol_component(
+ kcontrol);
+ int left = ucontrol->value.integer.value[0];
+ int right = ucontrol->value.integer.value[1];
+ int ret;
+
+ if (left > MAX_GAIN || right > MAX_GAIN)
+ return -EINVAL;
+
+ ret = set_ec_mic_gain(component, (uint8_t)left, (uint8_t)right);
+
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct snd_kcontrol_new cros_ec_snd_controls[] = {
+ SOC_DOUBLE_EXT_TLV("EC Mic Gain", SND_SOC_NOPM, SND_SOC_NOPM, 0, 43, 0,
+ mic_gain_get, mic_gain_put, ec_mic_gain_tlv)
+};
+
+static int enable_i2s(struct snd_soc_component *component, int enable)
+{
+ struct ec_param_codec_i2s param;
+ int ret;
+
+ dev_dbg(component->dev, "%s set i2s to %u\n", __func__, enable);
+
+ param.cmd = EC_CODEC_I2S_ENABLE;
+ param.i2s_enable = enable;
+
+ ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
+ (uint8_t *)¶m, sizeof(param),
+ NULL, 0);
+ if (ret < 0) {
+ dev_err(component->dev, "I2S enable %d command returned 0x%x\n",
+ enable, ret);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int cros_ec_i2s_enable_event(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event)
+{
+ struct snd_soc_component *component = snd_soc_dapm_to_component(
+ w->dapm);
+
+ dev_dbg(component->dev, "%s enter\n", __func__);
+
+ switch (event) {
+
+ case SND_SOC_DAPM_PRE_PMU:
+ dev_dbg(component->dev,
+ "%s got SND_SOC_DAPM_PRE_PMU event\n", __func__);
+ return enable_i2s(component, 1);
+
+ case SND_SOC_DAPM_PRE_PMD:
+ dev_dbg(component->dev,
+ "%s got SND_SOC_DAPM_PRE_PMD event\n", __func__);
+ return enable_i2s(component, 0);
+ }
+
+ return 0;
+}
+
+/*
+ * The goal of this DAPM route is to turn on/off I2S using EC
+ * host command when capture stream is started/stopped.
+ */
+static const struct snd_soc_dapm_widget cros_ec_dapm_widgets[] = {
+ SND_SOC_DAPM_INPUT("DMIC"),
+
+ /*
+ * Control EC to enable/disable I2S.
+ */
+ SND_SOC_DAPM_SUPPLY("I2S Enable", SND_SOC_NOPM,
+ 0, 0, cros_ec_i2s_enable_event,
+ SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD),
+
+ SND_SOC_DAPM_AIF_OUT("I2STX", "I2S Capture", 0, SND_SOC_NOPM, 0, 0),
+};
+
+static const struct snd_soc_dapm_route cros_ec_dapm_routes[] = {
+ { "I2STX", NULL, "DMIC" },
+ { "I2STX", NULL, "I2S Enable" },
+};
+
+
+static const struct snd_soc_component_driver cros_ec_component_driver = {
+ .controls = cros_ec_snd_controls,
+ .num_controls = ARRAY_SIZE(cros_ec_snd_controls),
+ .dapm_widgets = cros_ec_dapm_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(cros_ec_dapm_widgets),
+ .dapm_routes = cros_ec_dapm_routes,
+ .num_dapm_routes = ARRAY_SIZE(cros_ec_dapm_routes),
+};
+
+/*
+ * Platform device and platform driver fro cros-ec-codec.
+ */
+static int cros_ec_codec_platform_probe(struct platform_device *pd)
+{
+ struct device *dev = &pd->dev;
+ struct cros_ec_device *ec_device = dev_get_drvdata(pd->dev.parent);
+ struct cros_ec_codec_data *codec_data;
+ int rc;
+
+ dev_dbg(dev, "%s start\n", __func__);
+
+ /*
+ * If the parent ec device has not been probed yet, defer the probe.
+ */
+ if (ec_device == NULL) {
+ dev_dbg(dev, "No EC device found yet.\n");
+ return -EPROBE_DEFER;
+ }
+
+ codec_data = devm_kzalloc(dev, sizeof(struct cros_ec_codec_data),
+ GFP_KERNEL);
+ if (!codec_data)
+ return -ENOMEM;
+
+ codec_data->dev = dev;
+ codec_data->ec_device = ec_device;
+
+ platform_set_drvdata(pd, codec_data);
+
+ rc = snd_soc_register_component(dev, &cros_ec_component_driver,
+ cros_ec_dai, ARRAY_SIZE(cros_ec_dai));
+
+ dev_dbg(dev, "%s done\n", __func__);
+
+ return 0;
+}
+
+static int cros_ec_codec_platform_remove(struct platform_device *pd)
+{
+ struct device *dev = &pd->dev;
+
+ dev_dbg(dev, "%s\n", __func__);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id cros_ec_codec_of_match[] = {
+ { .compatible = "google,cros-ec-codec" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, cros_ec_codec_of_match);
+#endif
+
+static struct platform_driver cros_ec_codec_platform_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(cros_ec_codec_of_match),
+ },
+ .probe = cros_ec_codec_platform_probe,
+ .remove = cros_ec_codec_platform_remove,
+};
+
+module_platform_driver(cros_ec_codec_platform_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Chrome EC codec driver");
+MODULE_AUTHOR("Cheng-Yi Chiang <cychiang@chromium.org>");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/sound/soc/codecs/cros_ec_codec.h b/sound/soc/codecs/cros_ec_codec.h
new file mode 100644
index 0000000000000..3a72e16a7ba65
--- /dev/null
+++ b/sound/soc/codecs/cros_ec_codec.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ChromeOS EC Codec
+ *
+ * Copyright 2018 Google, Inc
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __CROS_EC_CODEC_H
+#define __CROS_EC_CODEC_H
+
+/*
+ * struct cros_ec_codec_data - ChromeOS EC codec driver data.
+ *
+ * @dev: Device structure used in sysfs.
+ * @ec_device: cros_ec_device structure to talk to the physical device.
+ * @component: Pointer to the component.
+ */
+struct cros_ec_codec_data {
+ struct device *dev;
+ struct cros_ec_device *ec_device;
+ struct snd_soc_component *component;
+};
+
+#endif /* __CROS_EC_CODEC_H */
--
2.20.0.rc0.387.gc7a69e6b6c-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] mfd: cros_ec: Add commands to control codec
2018-11-27 12:00 [PATCH 1/3] mfd: cros_ec: Add commands to control codec Cheng-Yi Chiang
2018-11-27 12:00 ` [PATCH 2/3] ASoC: Documentation: Add google,cros-ec-codec Cheng-Yi Chiang
2018-11-27 12:00 ` [PATCH 3/3] ASoC: cros_ec_codec: Add codec driver for Cros EC Cheng-Yi Chiang
@ 2018-11-28 9:34 ` Lee Jones
2018-12-03 11:19 ` Lee Jones
2 siblings, 1 reply; 13+ messages in thread
From: Lee Jones @ 2018-11-28 9:34 UTC (permalink / raw)
To: Cheng-Yi Chiang
Cc: linux-kernel, Mark Brown, alsa-devel, dgreid, tzungbi, Rohit kumar
On Tue, 27 Nov 2018, Cheng-Yi Chiang wrote:
> Add EC host commands to control codec on EC.
>
> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
> include/linux/mfd/cros_ec_commands.h | 94 ++++++++++++++++++++++++++++
> 1 file changed, 94 insertions(+)
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [alsa-devel] [PATCH 3/3] ASoC: cros_ec_codec: Add codec driver for Cros EC
2018-11-27 12:00 ` [PATCH 3/3] ASoC: cros_ec_codec: Add codec driver for Cros EC Cheng-Yi Chiang
@ 2018-11-29 12:42 ` Enric Balletbo Serra
2018-12-24 8:47 ` Cheng-yi Chiang
0 siblings, 1 reply; 13+ messages in thread
From: Enric Balletbo Serra @ 2018-11-29 12:42 UTC (permalink / raw)
To: Cheng-Yi Chiang
Cc: linux-kernel, alsa-devel, tzungbi, Mark Brown, rohitkr, dgreid
Hi Cheng-Yi
Many thanks for the patch. I am not really an ASoC expert, so my
comments are more based on the feedback for other cros-ec drivers. So,
various nits and few comments below.
The first question I'd like to ask is if there is any EC_FEATURE that
tells you that the cros-ec has the codec. And second (if you can
answer), could you tell me which device you used to test this?
Missatge de Cheng-Yi Chiang <cychiang@chromium.org> del dia dt., 27 de
nov. 2018 a les 13:02:
>
> Add a codec driver to control ChromeOS EC codec.
>
> Use EC Host command to enable/disable I2S recording and control other
> configurations.
>
> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
> MAINTAINERS | 1 +
> sound/soc/codecs/Kconfig | 8 +
> sound/soc/codecs/Makefile | 2 +
> sound/soc/codecs/cros_ec_codec.c | 499 +++++++++++++++++++++++++++++++
> sound/soc/codecs/cros_ec_codec.h | 33 ++
> 5 files changed, 543 insertions(+)
> create mode 100644 sound/soc/codecs/cros_ec_codec.c
> create mode 100644 sound/soc/codecs/cros_ec_codec.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5cf8ab296cc61..f1999ef19d1cc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3556,6 +3556,7 @@ CHROME EC CODEC DRIVER
Trying to be coherent with names s/CHROME/CHROMEOS/
> M: Cheng-Yi Chiang <cychiang@chromium.org>
Do you mind adding me as a reviewer? I am interested in review all the
cros-ec related patches.
R: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> S: Maintained
> F: Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
> +F: sound/soc/codecs/cros_ec_codec.*
>
> CIRRUS LOGIC AUDIO CODEC DRIVERS
> M: Brian Austin <brian.austin@cirrus.com>
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index efb095dbcd714..3e3e9294c0259 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -49,6 +49,7 @@ config SND_SOC_ALL_CODECS
> select SND_SOC_BT_SCO
> select SND_SOC_BD28623
> select SND_SOC_CQ0093VC
> + select SND_SOC_CROS_EC_CODEC
> select SND_SOC_CS35L32 if I2C
> select SND_SOC_CS35L33 if I2C
> select SND_SOC_CS35L34 if I2C
> @@ -445,6 +446,13 @@ config SND_SOC_CPCAP
> config SND_SOC_CQ0093VC
> tristate
>
> +config SND_SOC_CROS_EC_CODEC
> + tristate "codec driver for Cros EC"
s/Cros EC/ChromeOS EC
> + depends on MFD_CROS_EC
> + help
> + If you say yes here you will get support for the
> + Chrome OS Embedded Controller's Audio Codec.
nit: s/Chrome OS/ChromeOS/
Based on past discussions It's not really clear if you should use
Chrome OS or ChromeOS. Personally, I like the second version which is
more used but I don't mind which one you use, but don't mix Chrome OS
and ChromeOS, use the same form for in your patch. I'll point you
where you used "Chrome OS" in this review.
> +
> config SND_SOC_CS35L32
> tristate "Cirrus Logic CS35L32 CODEC"
> depends on I2C
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index 7ae7c85e8219f..ff05c260c5776 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -41,6 +41,7 @@ snd-soc-bd28623-objs := bd28623.o
> snd-soc-bt-sco-objs := bt-sco.o
> snd-soc-cpcap-objs := cpcap.o
> snd-soc-cq93vc-objs := cq93vc.o
> +snd-soc-cros-ec-codec-objs := cros_ec_codec.o
> snd-soc-cs35l32-objs := cs35l32.o
> snd-soc-cs35l33-objs := cs35l33.o
> snd-soc-cs35l34-objs := cs35l34.o
> @@ -301,6 +302,7 @@ obj-$(CONFIG_SND_SOC_BD28623) += snd-soc-bd28623.o
> obj-$(CONFIG_SND_SOC_BT_SCO) += snd-soc-bt-sco.o
> obj-$(CONFIG_SND_SOC_CQ0093VC) += snd-soc-cq93vc.o
> obj-$(CONFIG_SND_SOC_CPCAP) += snd-soc-cpcap.o
> +obj-$(CONFIG_SND_SOC_CROS_EC_CODEC) += snd-soc-cros-ec-codec.o
> obj-$(CONFIG_SND_SOC_CS35L32) += snd-soc-cs35l32.o
> obj-$(CONFIG_SND_SOC_CS35L33) += snd-soc-cs35l33.o
> obj-$(CONFIG_SND_SOC_CS35L34) += snd-soc-cs35l34.o
> diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
> new file mode 100644
> index 0000000000000..e24174c11a7de
> --- /dev/null
> +++ b/sound/soc/codecs/cros_ec_codec.c
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * cros_ec_codec - Driver for Chrome OS Embedded Controller codec.
nit: remove "cros_ec_code -" just put the description here. If for
some weird reason the file changes the name this will be probably
incorrect and doesn't apport anything.
nit: s/Chrome OS/ChromeOS/
> + *
> + * Copyright 2018 Google, Inc
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
You can remove LICENSE boilerplate, is inherent to the SPDX-License-Identifier.
> + * This driver uses the cros-ec interface to communicate with the Chrome OS
nit: s/Chrome OS/ChromeOS/
> + * EC for audio function.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/module.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <sound/tlv.h>
> +#include <linux/platform_device.h>
nit: I like see alphabetical order here
> +
> +#include "cros_ec_codec.h"
> +
> +#define MAX_GAIN 43
> +
> +#define DRV_NAME "cros-ec-codec"
> +
> +static const DECLARE_TLV_DB_SCALE(ec_mic_gain_tlv, 0, 100, 0);
> +/*
> + * Wrapper for EC command.
> + */
> +static int ec_command(struct snd_soc_component *component, int version,
> + int command, uint8_t *outdata, int outsize,
> + uint8_t *indata, int insize)
nit: s/uint8_t/u8/ checkpatch --strict ? Will spot more format issues
in this patch. I'm going to add some of these to this review as a nit.
> +{
> + struct cros_ec_codec_data *codec_data = snd_soc_component_get_drvdata(
nit: Lines should not end with a '('
> + component);
> + struct cros_ec_device *ec_device = codec_data->ec_device;
> + struct cros_ec_command *msg;
> + int ret;
> +
> + msg = kzalloc(sizeof(*msg) + max(insize, outsize), GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + msg->version = version;
> + msg->command = command;
> + msg->outsize = outsize;
> + msg->insize = insize;
> +
> + if (outsize)
> + memcpy(msg->data, outdata, outsize);
> +
> + ret = cros_ec_cmd_xfer_status(ec_device, msg);
> + if (ret > 0 && insize)
> + memcpy(indata, msg->data, insize);
> +
> + kfree(msg);
> + return ret;
> +}
> +
> +static int set_i2s_config(struct snd_soc_component *component,
> + enum ec_i2s_config i2s_config)
nit: Alignment should match open parenthesis
> +{
> + struct ec_param_codec_i2s param;
> + int ret;
> +
> + dev_dbg(component->dev, "%s set I2S format to %u\n", __func__,
> + i2s_config);
> +
> + param.cmd = EC_CODEC_I2S_SET_CONFIG;
> + param.i2s_config = i2s_config;
> +
> + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> + (uint8_t *)¶m, sizeof(param),
nit: Prefer kernel type 'u8' over 'uint8_t'
> + NULL, 0);
> + if (ret < 0) {
> + dev_err(component->dev,
> + "set I2S format to %u command returned 0x%x\n",
> + i2s_config, ret);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int cros_ec_i2s_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> + struct snd_soc_component *component = dai->component;
> + enum ec_i2s_config i2s_config;
> +
> + dev_dbg(component->dev, "%s enter\n", __func__);
Entry and exit debugging log are not really useful, and you can use
kernel trace tools to check that, so better remove it.
> +
> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> + case SND_SOC_DAIFMT_CBS_CFS:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> + case SND_SOC_DAIFMT_NB_NF:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> + case SND_SOC_DAIFMT_I2S:
> + i2s_config = EC_DAI_FMT_I2S;
> + break;
> +
> + case SND_SOC_DAIFMT_RIGHT_J:
> + i2s_config = EC_DAI_FMT_RIGHT_J;
> + break;
> +
> + case SND_SOC_DAIFMT_LEFT_J:
> + i2s_config = EC_DAI_FMT_LEFT_J;
> + break;
> +
> + case SND_SOC_DAIFMT_DSP_A:
> + i2s_config = EC_DAI_FMT_PCM_A;
> + break;
> +
> + case SND_SOC_DAIFMT_DSP_B:
> + i2s_config = EC_DAI_FMT_PCM_B;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + set_i2s_config(component, i2s_config);
> +
> + dev_dbg(component->dev, "%s set I2S DAI format\n", __func__);
Remove this debug message it is only used to trace the exit of the function.
> +
> + return 0;
> +}
> +
> +static int set_i2s_sample_depth(struct snd_soc_component *component,
> + enum ec_sample_depth_value depth)
> +{
> + struct ec_param_codec_i2s param;
> + int ret;
> +
> + dev_dbg(component->dev, "%s set depth to %u\n", __func__, depth);
> +
> + param.cmd = EC_CODEC_SET_SAMPLE_DEPTH;
> + param.depth = depth;
> +
> + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> + (uint8_t *)¶m, sizeof(param),
> + NULL, 0);
> + if (ret < 0) {
> + dev_err(component->dev, "I2S sample depth %u returned 0x%x\n",
> + depth, ret);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int set_bclk(struct snd_soc_component *component, uint32_t bclk)
> +{
> + struct ec_param_codec_i2s param;
> + int ret;
> +
> + dev_dbg(component->dev, "%s set i2s bclk to %u\n", __func__, bclk);
> +
> + param.cmd = EC_CODEC_I2S_SET_BCLK;
> + param.bclk = bclk;
> +
> + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> + (uint8_t *)¶m, sizeof(param),
> + NULL, 0);
> + if (ret < 0) {
> + dev_err(component->dev, "I2S set bclk %u command returned 0x%x\n",
> + bclk, ret);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int cros_ec_i2s_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> +{
> + struct snd_soc_component *component = dai->component;
> + int frame_size;
> + unsigned int rate, bclk;
> + int ret;
> +
> + frame_size = snd_soc_params_to_frame_size(params);
> + if (frame_size < 0) {
> + dev_err(component->dev, "Unsupported frame size: %d\n",
> + frame_size);
> + return -EINVAL;
> + }
> +
> + rate = params_rate(params);
> + if (rate != 48000) {
> + dev_err(component->dev, "Unsupported rate\n");
> + return -EINVAL;
> + }
> +
> + switch (params_format(params)) {
> + case SNDRV_PCM_FORMAT_S16_LE:
> + ret = set_i2s_sample_depth(component, EC_CODEC_SAMPLE_DEPTH_16);
> + break;
> + case SNDRV_PCM_FORMAT_S24_LE:
> + ret = set_i2s_sample_depth(component, EC_CODEC_SAMPLE_DEPTH_24);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (ret)
> + return ret;
> +
> + bclk = snd_soc_params_to_bclk(params);
> + ret = set_bclk(component, bclk);
> +
> + return ret;
> +}
> +
> +static const struct snd_soc_dai_ops cros_ec_i2s_dai_ops = {
> + .hw_params = cros_ec_i2s_hw_params,
> + .set_fmt = cros_ec_i2s_set_dai_fmt,
> +};
> +
> +struct snd_soc_dai_driver cros_ec_dai[] = {
> + {
> + .name = "cros_ec_codec I2S",
> + .id = 0,
> + .capture = {
> + .stream_name = "I2S Capture",
> + .channels_min = 2,
> + .channels_max = 2,
> + .rates = SNDRV_PCM_RATE_48000,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE |
> + SNDRV_PCM_FMTBIT_S24_LE,
> + },
> + .ops = &cros_ec_i2s_dai_ops,
> + }
> +};
> +
> +static int get_ec_mic_gain(struct snd_soc_component *component,
> + uint8_t *left, uint8_t *right)
> +{
> + struct ec_param_codec_i2s param;
> + struct ec_response_codec_gain resp;
> + int ret;
> +
> + dev_dbg(component->dev, "%s get mic gain\n", __func__);
Remove the trace.
> +
> + param.cmd = EC_CODEC_GET_GAIN;
> +
> + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> + (uint8_t *)¶m, sizeof(param),
> + (uint8_t *)&resp, sizeof(resp));
> + if (ret < 0) {
> + dev_err(component->dev, "I2S get gain command returned 0x%x\n",
> + ret);
> + return -EINVAL;
> + }
> +
> + *left = resp.left;
> + *right = resp.right;
> +
> + dev_dbg(component->dev, "%s get mic gain %u, %u\n", __func__,
> + *left, *right);
> +
> + return 0;
> +}
> +
> +static int mic_gain_get(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component = snd_soc_kcontrol_component(
> + kcontrol);
> + uint8_t left, right;
> + int ret;
> +
> + ret = get_ec_mic_gain(component, &left, &right);
> +
> + if (ret)
> + return ret;
> +
> + ucontrol->value.integer.value[0] = left;
> + ucontrol->value.integer.value[1] = right;
> +
> + return 0;
> +}
> +
> +static int set_ec_mic_gain(struct snd_soc_component *component,
> + uint8_t left, uint8_t right)
> +{
> + struct ec_param_codec_i2s param;
> + int ret;
> +
> + dev_dbg(component->dev, "%s set mic gain to %u, %u\n",
> + __func__, left, right);
> +
> + param.cmd = EC_CODEC_SET_GAIN;
> + param.gain.left = left;
> + param.gain.right = right;
> +
> + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> + (uint8_t *)¶m, sizeof(param),
> + NULL, 0);
> + if (ret < 0) {
> + dev_err(component->dev, "I2S set gain command returned 0x%x\n",
%d ? Doesn't make more sense print the decimal format here?
I know that other cros_ec drivers do this, is something to fix I guess.
> + ret);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int mic_gain_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component = snd_soc_kcontrol_component(
> + kcontrol);
> + int left = ucontrol->value.integer.value[0];
> + int right = ucontrol->value.integer.value[1];
> + int ret;
> +
> + if (left > MAX_GAIN || right > MAX_GAIN)
> + return -EINVAL;
> +
> + ret = set_ec_mic_gain(component, (uint8_t)left, (uint8_t)right);
> +
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static const struct snd_kcontrol_new cros_ec_snd_controls[] = {
> + SOC_DOUBLE_EXT_TLV("EC Mic Gain", SND_SOC_NOPM, SND_SOC_NOPM, 0, 43, 0,
> + mic_gain_get, mic_gain_put, ec_mic_gain_tlv)
> +};
> +
> +static int enable_i2s(struct snd_soc_component *component, int enable)
> +{
> + struct ec_param_codec_i2s param;
> + int ret;
> +
> + dev_dbg(component->dev, "%s set i2s to %u\n", __func__, enable);
> +
> + param.cmd = EC_CODEC_I2S_ENABLE;
> + param.i2s_enable = enable;
> +
> + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> + (uint8_t *)¶m, sizeof(param),
> + NULL, 0);
> + if (ret < 0) {
> + dev_err(component->dev, "I2S enable %d command returned 0x%x\n",
> + enable, ret);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int cros_ec_i2s_enable_event(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_component *component = snd_soc_dapm_to_component(
> + w->dapm);
> +
> + dev_dbg(component->dev, "%s enter\n", __func__);
> +
> + switch (event) {
> +
> + case SND_SOC_DAPM_PRE_PMU:
> + dev_dbg(component->dev,
> + "%s got SND_SOC_DAPM_PRE_PMU event\n", __func__);
> + return enable_i2s(component, 1);
> +
> + case SND_SOC_DAPM_PRE_PMD:
> + dev_dbg(component->dev,
> + "%s got SND_SOC_DAPM_PRE_PMD event\n", __func__);
> + return enable_i2s(component, 0);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * The goal of this DAPM route is to turn on/off I2S using EC
> + * host command when capture stream is started/stopped.
> + */
> +static const struct snd_soc_dapm_widget cros_ec_dapm_widgets[] = {
> + SND_SOC_DAPM_INPUT("DMIC"),
> +
> + /*
> + * Control EC to enable/disable I2S.
> + */
> + SND_SOC_DAPM_SUPPLY("I2S Enable", SND_SOC_NOPM,
> + 0, 0, cros_ec_i2s_enable_event,
> + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD),
> +
> + SND_SOC_DAPM_AIF_OUT("I2STX", "I2S Capture", 0, SND_SOC_NOPM, 0, 0),
> +};
> +
> +static const struct snd_soc_dapm_route cros_ec_dapm_routes[] = {
> + { "I2STX", NULL, "DMIC" },
> + { "I2STX", NULL, "I2S Enable" },
> +};
> +
> +
nit: Please don't use multiple blank lines.
> +static const struct snd_soc_component_driver cros_ec_component_driver = {
> + .controls = cros_ec_snd_controls,
> + .num_controls = ARRAY_SIZE(cros_ec_snd_controls),
> + .dapm_widgets = cros_ec_dapm_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(cros_ec_dapm_widgets),
> + .dapm_routes = cros_ec_dapm_routes,
> + .num_dapm_routes = ARRAY_SIZE(cros_ec_dapm_routes),
> +};
> +
> +/*
> + * Platform device and platform driver fro cros-ec-codec.
> + */
> +static int cros_ec_codec_platform_probe(struct platform_device *pd)
> +{
> + struct device *dev = &pd->dev;
> + struct cros_ec_device *ec_device = dev_get_drvdata(pd->dev.parent);
> + struct cros_ec_codec_data *codec_data;
> + int rc;
> +
> + dev_dbg(dev, "%s start\n", __func__);
Remove the debug trace.
> +
> + /*
> + * If the parent ec device has not been probed yet, defer the probe.
> + */
> + if (ec_device == NULL) {
> + dev_dbg(dev, "No EC device found yet.\n");
> + return -EPROBE_DEFER;
> + }
I think this check is unnecessary. The parent (mfd) will instantiate
this driver so will be always there. Probably this is a copy from
other cros-ec drivers that have also this unnecessary check. Did you
find a use case where this is necessary?
> +
> + codec_data = devm_kzalloc(dev, sizeof(struct cros_ec_codec_data),
> + GFP_KERNEL);
> + if (!codec_data)
> + return -ENOMEM;
> +
> + codec_data->dev = dev;
> + codec_data->ec_device = ec_device;
> +
> + platform_set_drvdata(pd, codec_data);
> +
> + rc = snd_soc_register_component(dev, &cros_ec_component_driver,
> + cros_ec_dai, ARRAY_SIZE(cros_ec_dai));
> +
> + dev_dbg(dev, "%s done\n", __func__);
> +
> + return 0;
> +}
> +
> +static int cros_ec_codec_platform_remove(struct platform_device *pd)
> +{
> + struct device *dev = &pd->dev;
> +
> + dev_dbg(dev, "%s\n", __func__);
> +
> + return 0;
> +}
I think you can remove this function it j only prints a message and as
I said you can use trace tools for that.
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id cros_ec_codec_of_match[] = {
> + { .compatible = "google,cros-ec-codec" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, cros_ec_codec_of_match);
> +#endif
> +
> +static struct platform_driver cros_ec_codec_platform_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .owner = THIS_MODULE,
I think is not necessary set the .owner here.
> + .of_match_table = of_match_ptr(cros_ec_codec_of_match),
> + },
> + .probe = cros_ec_codec_platform_probe,
> + .remove = cros_ec_codec_platform_remove,
.remove can go away.
> +};
> +
> +module_platform_driver(cros_ec_codec_platform_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Chrome EC codec driver");
ChromeOS
> +MODULE_AUTHOR("Cheng-Yi Chiang <cychiang@chromium.org>");
> +MODULE_ALIAS("platform:" DRV_NAME);
> diff --git a/sound/soc/codecs/cros_ec_codec.h b/sound/soc/codecs/cros_ec_codec.h
> new file mode 100644
> index 0000000000000..3a72e16a7ba65
> --- /dev/null
> +++ b/sound/soc/codecs/cros_ec_codec.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ChromeOS EC Codec
> + *
> + * Copyright 2018 Google, Inc
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
Remove the License boilerplate
> + */
> +
> +#ifndef __CROS_EC_CODEC_H
> +#define __CROS_EC_CODEC_H
> +
> +/*
> + * struct cros_ec_codec_data - ChromeOS EC codec driver data.
> + *
> + * @dev: Device structure used in sysfs.
> + * @ec_device: cros_ec_device structure to talk to the physical device.
> + * @component: Pointer to the component.
> + */
Please use the kernel documentation format here.
Thanks,
Enric
> +struct cros_ec_codec_data {
> + struct device *dev;
> + struct cros_ec_device *ec_device;
> + struct snd_soc_component *component;
> +};
> +
> +#endif /* __CROS_EC_CODEC_H */
> --
> 2.20.0.rc0.387.gc7a69e6b6c-goog
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] mfd: cros_ec: Add commands to control codec
2018-11-28 9:34 ` [PATCH 1/3] mfd: cros_ec: Add commands to control codec Lee Jones
@ 2018-12-03 11:19 ` Lee Jones
2018-12-04 1:46 ` Cheng-yi Chiang
0 siblings, 1 reply; 13+ messages in thread
From: Lee Jones @ 2018-12-03 11:19 UTC (permalink / raw)
To: Cheng-Yi Chiang
Cc: linux-kernel, Mark Brown, alsa-devel, dgreid, tzungbi, Rohit kumar
On Wed, 28 Nov 2018, Lee Jones wrote:
> On Tue, 27 Nov 2018, Cheng-Yi Chiang wrote:
>
> > Add EC host commands to control codec on EC.
> >
> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> > include/linux/mfd/cros_ec_commands.h | 94 ++++++++++++++++++++++++++++
> > 1 file changed, 94 insertions(+)
>
> Applied, thanks.
Turns out that this does not actually apply.
What is it based on?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] mfd: cros_ec: Add commands to control codec
2018-12-03 11:19 ` Lee Jones
@ 2018-12-04 1:46 ` Cheng-yi Chiang
2018-12-05 9:01 ` Cheng-yi Chiang
0 siblings, 1 reply; 13+ messages in thread
From: Cheng-yi Chiang @ 2018-12-04 1:46 UTC (permalink / raw)
To: lee.jones
Cc: linux-kernel, Mark Brown, alsa-devel, Dylan Reid, tzungbi, rohitkr
On Mon, Dec 3, 2018 at 7:19 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 28 Nov 2018, Lee Jones wrote:
>
> > On Tue, 27 Nov 2018, Cheng-Yi Chiang wrote:
> >
> > > Add EC host commands to control codec on EC.
> > >
> > > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > > ---
> > > include/linux/mfd/cros_ec_commands.h | 94 ++++++++++++++++++++++++++++
> > > 1 file changed, 94 insertions(+)
> >
> > Applied, thanks.
>
> Turns out that this does not actually apply.
>
> What is it based on?
Hi Lee,
I am sorry about that!
The patch series was based on Mark Brown's tree (
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git )
for-next branch.
I will rebase it on upstream master branch in v2 when I address other
comments in codec driver.
Thanks!
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] mfd: cros_ec: Add commands to control codec
2018-12-04 1:46 ` Cheng-yi Chiang
@ 2018-12-05 9:01 ` Cheng-yi Chiang
2018-12-05 11:30 ` Lee Jones
0 siblings, 1 reply; 13+ messages in thread
From: Cheng-yi Chiang @ 2018-12-05 9:01 UTC (permalink / raw)
To: Lee Jones
Cc: linux-kernel, Mark Brown, alsa-devel, Dylan Reid, tzungbi, rohitkr
Hi Lee,
I tried to apply this patch based on
for-mfd-next branch of mfd tree (
git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git )
Then, I tried to compile all modules by
ARCH=x86_64 make allyesconfig
ARCH=x86_64 make -j64
There was no error.
Could you please let me know what was the error when you failed to
apply this patch ?
Thanks!
On Tue, Dec 4, 2018 at 9:46 AM Cheng-yi Chiang <cychiang@chromium.org> wrote:
>
> On Mon, Dec 3, 2018 at 7:19 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Wed, 28 Nov 2018, Lee Jones wrote:
> >
> > > On Tue, 27 Nov 2018, Cheng-Yi Chiang wrote:
> > >
> > > > Add EC host commands to control codec on EC.
> > > >
> > > > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > > > ---
> > > > include/linux/mfd/cros_ec_commands.h | 94 ++++++++++++++++++++++++++++
> > > > 1 file changed, 94 insertions(+)
> > >
> > > Applied, thanks.
> >
> > Turns out that this does not actually apply.
> >
> > What is it based on?
>
> Hi Lee,
> I am sorry about that!
> The patch series was based on Mark Brown's tree (
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git )
> for-next branch.
> I will rebase it on upstream master branch in v2 when I address other
> comments in codec driver.
> Thanks!
>
> >
> > --
> > Lee Jones [李琼斯]
> > Linaro Services Technical Lead
> > Linaro.org │ Open source software for ARM SoCs
> > Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] mfd: cros_ec: Add commands to control codec
2018-12-05 9:01 ` Cheng-yi Chiang
@ 2018-12-05 11:30 ` Lee Jones
2018-12-05 11:34 ` Lee Jones
0 siblings, 1 reply; 13+ messages in thread
From: Lee Jones @ 2018-12-05 11:30 UTC (permalink / raw)
To: Cheng-yi Chiang
Cc: linux-kernel, Mark Brown, alsa-devel, Dylan Reid, tzungbi, rohitkr
On Wed, 05 Dec 2018, Cheng-yi Chiang wrote:
> Hi Lee,
>
> I tried to apply this patch based on
> for-mfd-next branch of mfd tree (
> git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git )
> Then, I tried to compile all modules by
>
> ARCH=x86_64 make allyesconfig
> ARCH=x86_64 make -j64
>
> There was no error.
> Could you please let me know what was the error when you failed to
> apply this patch ?
Doesn't work for me.
lee@host:~/projects/kernel [for-mfd-next]$ formfdnext
Applying: mfd: cros_ec: Add commands to control codec
error: sha1 information is lacking or useless (include/linux/mfd/cros_ec_commands.h).
error: could not build fake ancestor
Patch failed at 0001 mfd: cros_ec: Add commands to control codec
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
> On Tue, Dec 4, 2018 at 9:46 AM Cheng-yi Chiang <cychiang@chromium.org> wrote:
> >
> > On Mon, Dec 3, 2018 at 7:19 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Wed, 28 Nov 2018, Lee Jones wrote:
> > >
> > > > On Tue, 27 Nov 2018, Cheng-Yi Chiang wrote:
> > > >
> > > > > Add EC host commands to control codec on EC.
> > > > >
> > > > > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > > > > ---
> > > > > include/linux/mfd/cros_ec_commands.h | 94 ++++++++++++++++++++++++++++
> > > > > 1 file changed, 94 insertions(+)
> > > >
> > > > Applied, thanks.
> > >
> > > Turns out that this does not actually apply.
> > >
> > > What is it based on?
> >
> > Hi Lee,
> > I am sorry about that!
> > The patch series was based on Mark Brown's tree (
> > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git )
> > for-next branch.
> > I will rebase it on upstream master branch in v2 when I address other
> > comments in codec driver.
> > Thanks!
> >
> > >
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] mfd: cros_ec: Add commands to control codec
2018-12-05 11:30 ` Lee Jones
@ 2018-12-05 11:34 ` Lee Jones
2018-12-06 2:36 ` Cheng-yi Chiang
0 siblings, 1 reply; 13+ messages in thread
From: Lee Jones @ 2018-12-05 11:34 UTC (permalink / raw)
To: Cheng-yi Chiang
Cc: linux-kernel, Mark Brown, alsa-devel, Dylan Reid, tzungbi, rohitkr
On Wed, 05 Dec 2018, Lee Jones wrote:
> On Wed, 05 Dec 2018, Cheng-yi Chiang wrote:
>
> > Hi Lee,
> >
> > I tried to apply this patch based on
> > for-mfd-next branch of mfd tree (
> > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git )
> > Then, I tried to compile all modules by
> >
> > ARCH=x86_64 make allyesconfig
> > ARCH=x86_64 make -j64
> >
> > There was no error.
> > Could you please let me know what was the error when you failed to
> > apply this patch ?
>
> Doesn't work for me.
>
> lee@host:~/projects/kernel [for-mfd-next]$ formfdnext
> Applying: mfd: cros_ec: Add commands to control codec
> error: sha1 information is lacking or useless (include/linux/mfd/cros_ec_commands.h).
> error: could not build fake ancestor
> Patch failed at 0001 mfd: cros_ec: Add commands to control codec
> Use 'git am --show-current-patch' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
Not that the following lines (as seen in your patch) are not present
in the Mainline kernel.
> @@ -4077,6 +4077,100 @@ struct __ec_align1 ec_response_i2c_passthru_protect {
> uint8_t status; /* Status flags (0: unlocked, 1: locked) */
> };
Do you have other patches applied to your tree?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] mfd: cros_ec: Add commands to control codec
2018-12-05 11:34 ` Lee Jones
@ 2018-12-06 2:36 ` Cheng-yi Chiang
2018-12-07 7:23 ` Lee Jones
0 siblings, 1 reply; 13+ messages in thread
From: Cheng-yi Chiang @ 2018-12-06 2:36 UTC (permalink / raw)
To: Lee Jones
Cc: linux-kernel, Mark Brown, alsa-devel, Dylan Reid, tzungbi, rohitkr
On Wed, Dec 5, 2018 at 7:34 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 05 Dec 2018, Lee Jones wrote:
>
> > On Wed, 05 Dec 2018, Cheng-yi Chiang wrote:
> >
> > > Hi Lee,
> > >
> > > I tried to apply this patch based on
> > > for-mfd-next branch of mfd tree (
> > > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git )
> > > Then, I tried to compile all modules by
> > >
> > > ARCH=x86_64 make allyesconfig
> > > ARCH=x86_64 make -j64
> > >
> > > There was no error.
> > > Could you please let me know what was the error when you failed to
> > > apply this patch ?
> >
> > Doesn't work for me.
> >
> > lee@host:~/projects/kernel [for-mfd-next]$ formfdnext
> > Applying: mfd: cros_ec: Add commands to control codec
> > error: sha1 information is lacking or useless (include/linux/mfd/cros_ec_commands.h).
> > error: could not build fake ancestor
> > Patch failed at 0001 mfd: cros_ec: Add commands to control codec
> > Use 'git am --show-current-patch' to see the failed patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
>
> Not that the following lines (as seen in your patch) are not present
> in the Mainline kernel.
>
> > @@ -4077,6 +4077,100 @@ struct __ec_align1 ec_response_i2c_passthru_protect {
> > uint8_t status; /* Status flags (0: unlocked, 1: locked) */
> > };
>
> Do you have other patches applied to your tree?
Hi Lee,
Sorry, I see the problem.
I was using git am -3 which solves the conflict by itself.
I will resend a patch based on your for-mfd-next branch so we can keep
the patch clean.
Thanks for your time.
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] mfd: cros_ec: Add commands to control codec
2018-12-06 2:36 ` Cheng-yi Chiang
@ 2018-12-07 7:23 ` Lee Jones
0 siblings, 0 replies; 13+ messages in thread
From: Lee Jones @ 2018-12-07 7:23 UTC (permalink / raw)
To: Cheng-yi Chiang
Cc: linux-kernel, Mark Brown, alsa-devel, Dylan Reid, tzungbi, rohitkr
On Thu, 06 Dec 2018, Cheng-yi Chiang wrote:
> On Wed, Dec 5, 2018 at 7:34 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Wed, 05 Dec 2018, Lee Jones wrote:
> >
> > > On Wed, 05 Dec 2018, Cheng-yi Chiang wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > I tried to apply this patch based on
> > > > for-mfd-next branch of mfd tree (
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git )
> > > > Then, I tried to compile all modules by
> > > >
> > > > ARCH=x86_64 make allyesconfig
> > > > ARCH=x86_64 make -j64
> > > >
> > > > There was no error.
> > > > Could you please let me know what was the error when you failed to
> > > > apply this patch ?
> > >
> > > Doesn't work for me.
> > >
> > > lee@host:~/projects/kernel [for-mfd-next]$ formfdnext
> > > Applying: mfd: cros_ec: Add commands to control codec
> > > error: sha1 information is lacking or useless (include/linux/mfd/cros_ec_commands.h).
> > > error: could not build fake ancestor
> > > Patch failed at 0001 mfd: cros_ec: Add commands to control codec
> > > Use 'git am --show-current-patch' to see the failed patch
> > > When you have resolved this problem, run "git am --continue".
> > > If you prefer to skip this patch, run "git am --skip" instead.
> > > To restore the original branch and stop patching, run "git am --abort".
> >
> > Not that the following lines (as seen in your patch) are not present
> > in the Mainline kernel.
> >
> > > @@ -4077,6 +4077,100 @@ struct __ec_align1 ec_response_i2c_passthru_protect {
> > > uint8_t status; /* Status flags (0: unlocked, 1: locked) */
> > > };
> >
> > Do you have other patches applied to your tree?
>
> Hi Lee,
> Sorry, I see the problem.
> I was using git am -3 which solves the conflict by itself.
That's what I use, but it still had issues.
Git is probably using a previous resolution in your case.
> I will resend a patch based on your for-mfd-next branch so we can keep
> the patch clean.
> Thanks for your time.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [alsa-devel] [PATCH 3/3] ASoC: cros_ec_codec: Add codec driver for Cros EC
2018-11-29 12:42 ` [alsa-devel] " Enric Balletbo Serra
@ 2018-12-24 8:47 ` Cheng-yi Chiang
0 siblings, 0 replies; 13+ messages in thread
From: Cheng-yi Chiang @ 2018-12-24 8:47 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: linux-kernel, alsa-devel, tzungbi, Mark Brown, Rohit kumar, Dylan Reid
On Thu, Nov 29, 2018 at 8:42 PM Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
>
> Hi Cheng-Yi
>
> Many thanks for the patch. I am not really an ASoC expert, so my
> comments are more based on the feedback for other cros-ec drivers. So,
> various nits and few comments below.
>
Hi Enric,
Thank you for the review and sorry for the late response.
It took me a while to clean up this patches. I have addressed all your
comments in v2.
> The first question I'd like to ask is if there is any EC_FEATURE that
> tells you that the cros-ec has the codec. And second (if you can
> answer), could you tell me which device you used to test this?
Yes, there is an EC_FEATURE = 38 on EC side, which was added in this
CL: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1192702
But on kernel side for the board I am using, it is not used because
the presence of EC is from entry in DTS.
This is different from MFD ec_device_probe path in drivers/mfd/cros_ec_dev.c.
The device I am using to test this codec driver is Qualcomm SDM845 board.
>
> Missatge de Cheng-Yi Chiang <cychiang@chromium.org> del dia dt., 27 de
> nov. 2018 a les 13:02:
> >
> > Add a codec driver to control ChromeOS EC codec.
> >
> > Use EC Host command to enable/disable I2S recording and control other
> > configurations.
> >
> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> > MAINTAINERS | 1 +
> > sound/soc/codecs/Kconfig | 8 +
> > sound/soc/codecs/Makefile | 2 +
> > sound/soc/codecs/cros_ec_codec.c | 499 +++++++++++++++++++++++++++++++
> > sound/soc/codecs/cros_ec_codec.h | 33 ++
> > 5 files changed, 543 insertions(+)
> > create mode 100644 sound/soc/codecs/cros_ec_codec.c
> > create mode 100644 sound/soc/codecs/cros_ec_codec.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5cf8ab296cc61..f1999ef19d1cc 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3556,6 +3556,7 @@ CHROME EC CODEC DRIVER
>
> Trying to be coherent with names s/CHROME/CHROMEOS/
>
Fixed in v2.
> > M: Cheng-Yi Chiang <cychiang@chromium.org>
>
> Do you mind adding me as a reviewer? I am interested in review all the
> cros-ec related patches.
>
> R: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>
No problem.
Fixed in v2.
> > S: Maintained
> > F: Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
> > +F: sound/soc/codecs/cros_ec_codec.*
> >
> > CIRRUS LOGIC AUDIO CODEC DRIVERS
> > M: Brian Austin <brian.austin@cirrus.com>
> > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> > index efb095dbcd714..3e3e9294c0259 100644
> > --- a/sound/soc/codecs/Kconfig
> > +++ b/sound/soc/codecs/Kconfig
> > @@ -49,6 +49,7 @@ config SND_SOC_ALL_CODECS
> > select SND_SOC_BT_SCO
> > select SND_SOC_BD28623
> > select SND_SOC_CQ0093VC
> > + select SND_SOC_CROS_EC_CODEC
> > select SND_SOC_CS35L32 if I2C
> > select SND_SOC_CS35L33 if I2C
> > select SND_SOC_CS35L34 if I2C
> > @@ -445,6 +446,13 @@ config SND_SOC_CPCAP
> > config SND_SOC_CQ0093VC
> > tristate
> >
> > +config SND_SOC_CROS_EC_CODEC
> > + tristate "codec driver for Cros EC"
>
> s/Cros EC/ChromeOS EC
>
Fixed in v2.
> > + depends on MFD_CROS_EC
> > + help
> > + If you say yes here you will get support for the
> > + Chrome OS Embedded Controller's Audio Codec.
>
> nit: s/Chrome OS/ChromeOS/
>
> Based on past discussions It's not really clear if you should use
> Chrome OS or ChromeOS. Personally, I like the second version which is
> more used but I don't mind which one you use, but don't mix Chrome OS
> and ChromeOS, use the same form for in your patch. I'll point you
> where you used "Chrome OS" in this review.
>
I should use ChromeOS.
Fixed in v2.
> > +
> > config SND_SOC_CS35L32
> > tristate "Cirrus Logic CS35L32 CODEC"
> > depends on I2C
> > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> > index 7ae7c85e8219f..ff05c260c5776 100644
> > --- a/sound/soc/codecs/Makefile
> > +++ b/sound/soc/codecs/Makefile
> > @@ -41,6 +41,7 @@ snd-soc-bd28623-objs := bd28623.o
> > snd-soc-bt-sco-objs := bt-sco.o
> > snd-soc-cpcap-objs := cpcap.o
> > snd-soc-cq93vc-objs := cq93vc.o
> > +snd-soc-cros-ec-codec-objs := cros_ec_codec.o
> > snd-soc-cs35l32-objs := cs35l32.o
> > snd-soc-cs35l33-objs := cs35l33.o
> > snd-soc-cs35l34-objs := cs35l34.o
> > @@ -301,6 +302,7 @@ obj-$(CONFIG_SND_SOC_BD28623) += snd-soc-bd28623.o
> > obj-$(CONFIG_SND_SOC_BT_SCO) += snd-soc-bt-sco.o
> > obj-$(CONFIG_SND_SOC_CQ0093VC) += snd-soc-cq93vc.o
> > obj-$(CONFIG_SND_SOC_CPCAP) += snd-soc-cpcap.o
> > +obj-$(CONFIG_SND_SOC_CROS_EC_CODEC) += snd-soc-cros-ec-codec.o
> > obj-$(CONFIG_SND_SOC_CS35L32) += snd-soc-cs35l32.o
> > obj-$(CONFIG_SND_SOC_CS35L33) += snd-soc-cs35l33.o
> > obj-$(CONFIG_SND_SOC_CS35L34) += snd-soc-cs35l34.o
> > diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
> > new file mode 100644
> > index 0000000000000..e24174c11a7de
> > --- /dev/null
> > +++ b/sound/soc/codecs/cros_ec_codec.c
> > @@ -0,0 +1,499 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * cros_ec_codec - Driver for Chrome OS Embedded Controller codec.
>
> nit: remove "cros_ec_code -" just put the description here. If for
> some weird reason the file changes the name this will be probably
> incorrect and doesn't apport anything.
> nit: s/Chrome OS/ChromeOS/
>
Fixed in v2.
> > + *
> > + * Copyright 2018 Google, Inc
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
>
> You can remove LICENSE boilerplate, is inherent to the SPDX-License-Identifier.
>
Fixed in v2.
> > + * This driver uses the cros-ec interface to communicate with the Chrome OS
>
> nit: s/Chrome OS/ChromeOS/
>
Fixed in v2.
> > + * EC for audio function.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/cros_ec.h>
> > +#include <linux/mfd/cros_ec_commands.h>
> > +#include <linux/module.h>
> > +#include <sound/pcm.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/soc.h>
> > +#include <sound/tlv.h>
> > +#include <linux/platform_device.h>
>
> nit: I like see alphabetical order here
>
Fixed in v2.
> > +
> > +#include "cros_ec_codec.h"
> > +
> > +#define MAX_GAIN 43
> > +
> > +#define DRV_NAME "cros-ec-codec"
> > +
> > +static const DECLARE_TLV_DB_SCALE(ec_mic_gain_tlv, 0, 100, 0);
> > +/*
> > + * Wrapper for EC command.
> > + */
> > +static int ec_command(struct snd_soc_component *component, int version,
> > + int command, uint8_t *outdata, int outsize,
> > + uint8_t *indata, int insize)
>
> nit: s/uint8_t/u8/ checkpatch --strict ? Will spot more format issues
> in this patch. I'm going to add some of these to this review as a nit.
>
Thank you for the reminder. Using --strict is better.
Fixed in v2.
> > +{
> > + struct cros_ec_codec_data *codec_data = snd_soc_component_get_drvdata(
>
> nit: Lines should not end with a '('
>
Fixed in v2.
> > + component);
> > + struct cros_ec_device *ec_device = codec_data->ec_device;
> > + struct cros_ec_command *msg;
> > + int ret;
> > +
> > + msg = kzalloc(sizeof(*msg) + max(insize, outsize), GFP_KERNEL);
> > + if (!msg)
> > + return -ENOMEM;
> > +
> > + msg->version = version;
> > + msg->command = command;
> > + msg->outsize = outsize;
> > + msg->insize = insize;
> > +
> > + if (outsize)
> > + memcpy(msg->data, outdata, outsize);
> > +
> > + ret = cros_ec_cmd_xfer_status(ec_device, msg);
> > + if (ret > 0 && insize)
> > + memcpy(indata, msg->data, insize);
> > +
> > + kfree(msg);
> > + return ret;
> > +}
> > +
> > +static int set_i2s_config(struct snd_soc_component *component,
> > + enum ec_i2s_config i2s_config)
>
> nit: Alignment should match open parenthesis
>
Fixed in v2.
> > +{
> > + struct ec_param_codec_i2s param;
> > + int ret;
> > +
> > + dev_dbg(component->dev, "%s set I2S format to %u\n", __func__,
> > + i2s_config);
> > +
> > + param.cmd = EC_CODEC_I2S_SET_CONFIG;
> > + param.i2s_config = i2s_config;
> > +
> > + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> > + (uint8_t *)¶m, sizeof(param),
>
> nit: Prefer kernel type 'u8' over 'uint8_t'
>
Fixed in v2.
> > + NULL, 0);
> > + if (ret < 0) {
> > + dev_err(component->dev,
> > + "set I2S format to %u command returned 0x%x\n",
> > + i2s_config, ret);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +static int cros_ec_i2s_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> > +{
> > + struct snd_soc_component *component = dai->component;
> > + enum ec_i2s_config i2s_config;
> > +
> > + dev_dbg(component->dev, "%s enter\n", __func__);
>
> Entry and exit debugging log are not really useful, and you can use
> kernel trace tools to check that, so better remove it.
>
Fixed in v2.
> > +
> > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > + case SND_SOC_DAIFMT_CBS_CFS:
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> > + case SND_SOC_DAIFMT_NB_NF:
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> > + case SND_SOC_DAIFMT_I2S:
> > + i2s_config = EC_DAI_FMT_I2S;
> > + break;
> > +
> > + case SND_SOC_DAIFMT_RIGHT_J:
> > + i2s_config = EC_DAI_FMT_RIGHT_J;
> > + break;
> > +
> > + case SND_SOC_DAIFMT_LEFT_J:
> > + i2s_config = EC_DAI_FMT_LEFT_J;
> > + break;
> > +
> > + case SND_SOC_DAIFMT_DSP_A:
> > + i2s_config = EC_DAI_FMT_PCM_A;
> > + break;
> > +
> > + case SND_SOC_DAIFMT_DSP_B:
> > + i2s_config = EC_DAI_FMT_PCM_B;
> > + break;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + set_i2s_config(component, i2s_config);
> > +
> > + dev_dbg(component->dev, "%s set I2S DAI format\n", __func__);
>
> Remove this debug message it is only used to trace the exit of the function.
>
Fixed in v2.
> > +
> > + return 0;
> > +}
> > +
> > +static int set_i2s_sample_depth(struct snd_soc_component *component,
> > + enum ec_sample_depth_value depth)
> > +{
> > + struct ec_param_codec_i2s param;
> > + int ret;
> > +
> > + dev_dbg(component->dev, "%s set depth to %u\n", __func__, depth);
> > +
> > + param.cmd = EC_CODEC_SET_SAMPLE_DEPTH;
> > + param.depth = depth;
> > +
> > + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> > + (uint8_t *)¶m, sizeof(param),
> > + NULL, 0);
> > + if (ret < 0) {
> > + dev_err(component->dev, "I2S sample depth %u returned 0x%x\n",
> > + depth, ret);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +static int set_bclk(struct snd_soc_component *component, uint32_t bclk)
> > +{
> > + struct ec_param_codec_i2s param;
> > + int ret;
> > +
> > + dev_dbg(component->dev, "%s set i2s bclk to %u\n", __func__, bclk);
> > +
> > + param.cmd = EC_CODEC_I2S_SET_BCLK;
> > + param.bclk = bclk;
> > +
> > + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> > + (uint8_t *)¶m, sizeof(param),
> > + NULL, 0);
> > + if (ret < 0) {
> > + dev_err(component->dev, "I2S set bclk %u command returned 0x%x\n",
> > + bclk, ret);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +static int cros_ec_i2s_hw_params(struct snd_pcm_substream *substream,
> > + struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> > +{
> > + struct snd_soc_component *component = dai->component;
> > + int frame_size;
> > + unsigned int rate, bclk;
> > + int ret;
> > +
> > + frame_size = snd_soc_params_to_frame_size(params);
> > + if (frame_size < 0) {
> > + dev_err(component->dev, "Unsupported frame size: %d\n",
> > + frame_size);
> > + return -EINVAL;
> > + }
> > +
> > + rate = params_rate(params);
> > + if (rate != 48000) {
> > + dev_err(component->dev, "Unsupported rate\n");
> > + return -EINVAL;
> > + }
> > +
> > + switch (params_format(params)) {
> > + case SNDRV_PCM_FORMAT_S16_LE:
> > + ret = set_i2s_sample_depth(component, EC_CODEC_SAMPLE_DEPTH_16);
> > + break;
> > + case SNDRV_PCM_FORMAT_S24_LE:
> > + ret = set_i2s_sample_depth(component, EC_CODEC_SAMPLE_DEPTH_24);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (ret)
> > + return ret;
> > +
> > + bclk = snd_soc_params_to_bclk(params);
> > + ret = set_bclk(component, bclk);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct snd_soc_dai_ops cros_ec_i2s_dai_ops = {
> > + .hw_params = cros_ec_i2s_hw_params,
> > + .set_fmt = cros_ec_i2s_set_dai_fmt,
> > +};
> > +
> > +struct snd_soc_dai_driver cros_ec_dai[] = {
> > + {
> > + .name = "cros_ec_codec I2S",
> > + .id = 0,
> > + .capture = {
> > + .stream_name = "I2S Capture",
> > + .channels_min = 2,
> > + .channels_max = 2,
> > + .rates = SNDRV_PCM_RATE_48000,
> > + .formats = SNDRV_PCM_FMTBIT_S16_LE |
> > + SNDRV_PCM_FMTBIT_S24_LE,
> > + },
> > + .ops = &cros_ec_i2s_dai_ops,
> > + }
> > +};
> > +
> > +static int get_ec_mic_gain(struct snd_soc_component *component,
> > + uint8_t *left, uint8_t *right)
> > +{
> > + struct ec_param_codec_i2s param;
> > + struct ec_response_codec_gain resp;
> > + int ret;
> > +
> > + dev_dbg(component->dev, "%s get mic gain\n", __func__);
>
> Remove the trace.
>
Fixed in v2.
> > +
> > + param.cmd = EC_CODEC_GET_GAIN;
> > +
> > + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> > + (uint8_t *)¶m, sizeof(param),
> > + (uint8_t *)&resp, sizeof(resp));
> > + if (ret < 0) {
> > + dev_err(component->dev, "I2S get gain command returned 0x%x\n",
> > + ret);
> > + return -EINVAL;
> > + }
> > +
> > + *left = resp.left;
> > + *right = resp.right;
> > +
> > + dev_dbg(component->dev, "%s get mic gain %u, %u\n", __func__,
> > + *left, *right);
> > +
> > + return 0;
> > +}
> > +
> > +static int mic_gain_get(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol)
> > +{
> > + struct snd_soc_component *component = snd_soc_kcontrol_component(
> > + kcontrol);
> > + uint8_t left, right;
> > + int ret;
> > +
> > + ret = get_ec_mic_gain(component, &left, &right);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + ucontrol->value.integer.value[0] = left;
> > + ucontrol->value.integer.value[1] = right;
> > +
> > + return 0;
> > +}
> > +
> > +static int set_ec_mic_gain(struct snd_soc_component *component,
> > + uint8_t left, uint8_t right)
> > +{
> > + struct ec_param_codec_i2s param;
> > + int ret;
> > +
> > + dev_dbg(component->dev, "%s set mic gain to %u, %u\n",
> > + __func__, left, right);
> > +
> > + param.cmd = EC_CODEC_SET_GAIN;
> > + param.gain.left = left;
> > + param.gain.right = right;
> > +
> > + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> > + (uint8_t *)¶m, sizeof(param),
> > + NULL, 0);
> > + if (ret < 0) {
> > + dev_err(component->dev, "I2S set gain command returned 0x%x\n",
>
> %d ? Doesn't make more sense print the decimal format here?
> I know that other cros_ec drivers do this, is something to fix I guess.
>
Fixed in v2. Also fixed in other places.
> > + ret);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mic_gain_put(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol)
> > +{
> > + struct snd_soc_component *component = snd_soc_kcontrol_component(
> > + kcontrol);
> > + int left = ucontrol->value.integer.value[0];
> > + int right = ucontrol->value.integer.value[1];
> > + int ret;
> > +
> > + if (left > MAX_GAIN || right > MAX_GAIN)
> > + return -EINVAL;
> > +
> > + ret = set_ec_mic_gain(component, (uint8_t)left, (uint8_t)right);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct snd_kcontrol_new cros_ec_snd_controls[] = {
> > + SOC_DOUBLE_EXT_TLV("EC Mic Gain", SND_SOC_NOPM, SND_SOC_NOPM, 0, 43, 0,
> > + mic_gain_get, mic_gain_put, ec_mic_gain_tlv)
> > +};
> > +
> > +static int enable_i2s(struct snd_soc_component *component, int enable)
> > +{
> > + struct ec_param_codec_i2s param;
> > + int ret;
> > +
> > + dev_dbg(component->dev, "%s set i2s to %u\n", __func__, enable);
> > +
> > + param.cmd = EC_CODEC_I2S_ENABLE;
> > + param.i2s_enable = enable;
> > +
> > + ret = ec_command(component, 0, EC_CMD_CODEC_I2S,
> > + (uint8_t *)¶m, sizeof(param),
> > + NULL, 0);
> > + if (ret < 0) {
> > + dev_err(component->dev, "I2S enable %d command returned 0x%x\n",
> > + enable, ret);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +static int cros_ec_i2s_enable_event(struct snd_soc_dapm_widget *w,
> > + struct snd_kcontrol *kcontrol, int event)
> > +{
> > + struct snd_soc_component *component = snd_soc_dapm_to_component(
> > + w->dapm);
> > +
> > + dev_dbg(component->dev, "%s enter\n", __func__);
> > +
> > + switch (event) {
> > +
> > + case SND_SOC_DAPM_PRE_PMU:
> > + dev_dbg(component->dev,
> > + "%s got SND_SOC_DAPM_PRE_PMU event\n", __func__);
> > + return enable_i2s(component, 1);
> > +
> > + case SND_SOC_DAPM_PRE_PMD:
> > + dev_dbg(component->dev,
> > + "%s got SND_SOC_DAPM_PRE_PMD event\n", __func__);
> > + return enable_i2s(component, 0);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * The goal of this DAPM route is to turn on/off I2S using EC
> > + * host command when capture stream is started/stopped.
> > + */
> > +static const struct snd_soc_dapm_widget cros_ec_dapm_widgets[] = {
> > + SND_SOC_DAPM_INPUT("DMIC"),
> > +
> > + /*
> > + * Control EC to enable/disable I2S.
> > + */
> > + SND_SOC_DAPM_SUPPLY("I2S Enable", SND_SOC_NOPM,
> > + 0, 0, cros_ec_i2s_enable_event,
> > + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD),
> > +
> > + SND_SOC_DAPM_AIF_OUT("I2STX", "I2S Capture", 0, SND_SOC_NOPM, 0, 0),
> > +};
> > +
> > +static const struct snd_soc_dapm_route cros_ec_dapm_routes[] = {
> > + { "I2STX", NULL, "DMIC" },
> > + { "I2STX", NULL, "I2S Enable" },
> > +};
> > +
> > +
>
> nit: Please don't use multiple blank lines.
>
Fixed in v2.
> > +static const struct snd_soc_component_driver cros_ec_component_driver = {
> > + .controls = cros_ec_snd_controls,
> > + .num_controls = ARRAY_SIZE(cros_ec_snd_controls),
> > + .dapm_widgets = cros_ec_dapm_widgets,
> > + .num_dapm_widgets = ARRAY_SIZE(cros_ec_dapm_widgets),
> > + .dapm_routes = cros_ec_dapm_routes,
> > + .num_dapm_routes = ARRAY_SIZE(cros_ec_dapm_routes),
> > +};
> > +
> > +/*
> > + * Platform device and platform driver fro cros-ec-codec.
> > + */
> > +static int cros_ec_codec_platform_probe(struct platform_device *pd)
> > +{
> > + struct device *dev = &pd->dev;
> > + struct cros_ec_device *ec_device = dev_get_drvdata(pd->dev.parent);
> > + struct cros_ec_codec_data *codec_data;
> > + int rc;
> > +
> > + dev_dbg(dev, "%s start\n", __func__);
>
> Remove the debug trace.
>
Fixed in v2.
> > +
> > + /*
> > + * If the parent ec device has not been probed yet, defer the probe.
> > + */
> > + if (ec_device == NULL) {
> > + dev_dbg(dev, "No EC device found yet.\n");
> > + return -EPROBE_DEFER;
> > + }
>
> I think this check is unnecessary. The parent (mfd) will instantiate
> this driver so will be always there. Probably this is a copy from
> other cros-ec drivers that have also this unnecessary check. Did you
> find a use case where this is necessary?
>
Thank you for pointing this out.
In my use case, device is created from DTS, not MFD.
But I think you are right that parent will be created first so I
should remove this.
The place that I saw this usage was on cros_ec_keyb_probe for ACPI
flow only so it should be a different path.
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/450838/6/drivers/input/keyboard/cros_ec_keyb.c#620
> > +
> > + codec_data = devm_kzalloc(dev, sizeof(struct cros_ec_codec_data),
> > + GFP_KERNEL);
> > + if (!codec_data)
> > + return -ENOMEM;
> > +
> > + codec_data->dev = dev;
> > + codec_data->ec_device = ec_device;
> > +
> > + platform_set_drvdata(pd, codec_data);
> > +
> > + rc = snd_soc_register_component(dev, &cros_ec_component_driver,
> > + cros_ec_dai, ARRAY_SIZE(cros_ec_dai));
> > +
> > + dev_dbg(dev, "%s done\n", __func__);
> > +
> > + return 0;
> > +}
> > +
> > +static int cros_ec_codec_platform_remove(struct platform_device *pd)
> > +{
> > + struct device *dev = &pd->dev;
> > +
> > + dev_dbg(dev, "%s\n", __func__);
> > +
> > + return 0;
> > +}
>
> I think you can remove this function it j only prints a message and as
> I said you can use trace tools for that.
>
Fixed in v2.
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id cros_ec_codec_of_match[] = {
> > + { .compatible = "google,cros-ec-codec" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, cros_ec_codec_of_match);
> > +#endif
> > +
> > +static struct platform_driver cros_ec_codec_platform_driver = {
> > + .driver = {
> > + .name = DRV_NAME,
> > + .owner = THIS_MODULE,
>
> I think is not necessary set the .owner here.
>
Fixed in v2.
> > + .of_match_table = of_match_ptr(cros_ec_codec_of_match),
> > + },
> > + .probe = cros_ec_codec_platform_probe,
> > + .remove = cros_ec_codec_platform_remove,
>
> .remove can go away.
>
Fixed in v2.
> > +};
> > +
> > +module_platform_driver(cros_ec_codec_platform_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Chrome EC codec driver");
>
> ChromeOS
>
Fixed in v2.
> > +MODULE_AUTHOR("Cheng-Yi Chiang <cychiang@chromium.org>");
> > +MODULE_ALIAS("platform:" DRV_NAME);
> > diff --git a/sound/soc/codecs/cros_ec_codec.h b/sound/soc/codecs/cros_ec_codec.h
> > new file mode 100644
> > index 0000000000000..3a72e16a7ba65
> > --- /dev/null
> > +++ b/sound/soc/codecs/cros_ec_codec.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * ChromeOS EC Codec
> > + *
> > + * Copyright 2018 Google, Inc
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
>
> Remove the License boilerplate
>
Fixed in v2.
> > + */
> > +
> > +#ifndef __CROS_EC_CODEC_H
> > +#define __CROS_EC_CODEC_H
> > +
> > +/*
> > + * struct cros_ec_codec_data - ChromeOS EC codec driver data.
> > + *
> > + * @dev: Device structure used in sysfs.
> > + * @ec_device: cros_ec_device structure to talk to the physical device.
> > + * @component: Pointer to the component.
> > + */
>
> Please use the kernel documentation format here.
>
Fixed in v2.
> Thanks,
> Enric
Thanks a lot for the detailed review!
> > +struct cros_ec_codec_data {
> > + struct device *dev;
> > + struct cros_ec_device *ec_device;
> > + struct snd_soc_component *component;
> > +};
> > +
> > +#endif /* __CROS_EC_CODEC_H */
> > --
> > 2.20.0.rc0.387.gc7a69e6b6c-goog
> >
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-12-24 8:47 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-27 12:00 [PATCH 1/3] mfd: cros_ec: Add commands to control codec Cheng-Yi Chiang
2018-11-27 12:00 ` [PATCH 2/3] ASoC: Documentation: Add google,cros-ec-codec Cheng-Yi Chiang
2018-11-27 12:00 ` [PATCH 3/3] ASoC: cros_ec_codec: Add codec driver for Cros EC Cheng-Yi Chiang
2018-11-29 12:42 ` [alsa-devel] " Enric Balletbo Serra
2018-12-24 8:47 ` Cheng-yi Chiang
2018-11-28 9:34 ` [PATCH 1/3] mfd: cros_ec: Add commands to control codec Lee Jones
2018-12-03 11:19 ` Lee Jones
2018-12-04 1:46 ` Cheng-yi Chiang
2018-12-05 9:01 ` Cheng-yi Chiang
2018-12-05 11:30 ` Lee Jones
2018-12-05 11:34 ` Lee Jones
2018-12-06 2:36 ` Cheng-yi Chiang
2018-12-07 7:23 ` Lee Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).