| Message ID | 20181127120041.90759-3-cychiang@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] mfd: cros_ec: Add commands to control codec | expand |
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
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
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 */
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