| Message ID | 20181226070317.72022-3-cychiang@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] mfd: cros_ec: Add commands to control codec | expand |
On Wed, Dec 26, 2018 at 03:03:19PM +0800, Cheng-Yi Chiang wrote: > 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> > --- > Changes in v3: > 1.remove cros_ec_codec.h. > 2.Fix error code overriding in > set_i2s_config > set_i2s_sample_depth > set_bclk > get_ec_mic_gain > set_ec_mic_gain > enable_i2s > 3.Fix missing return code in cros_ec_i2s_set_dai_fmt. > 4.Simplify return code in cros_ec_i2s_hw_params and mic_gain_put. > 5.Remove useless debug message in cros_ec_codec_platform_probe. > > MAINTAINERS | 2 + > sound/soc/codecs/Kconfig | 8 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/cros_ec_codec.c | 454 +++++++++++++++++++++++++++++++ > 4 files changed, 466 insertions(+) > create mode 100644 sound/soc/codecs/cros_ec_codec.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 05e1922624e58..d66f80f3252d7 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3638,8 +3638,10 @@ F: drivers/platform/chrome/ > > CHROMEOS EC CODEC DRIVER > M: Cheng-Yi Chiang <cychiang@chromium.org> > +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 87cb9c51e6f5a..0b36428159b71 100644 > --- a/sound/soc/codecs/Kconfig > +++ b/sound/soc/codecs/Kconfig > @@ -50,6 +50,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 This unconditionally selects SND_SOC_CROS_EC_CODEC, but SND_SOC_CROS_EC_CODEC depends on MFD_CROS_EC. This is missing an "if MFD_CROS_EC" qualifier. > select SND_SOC_CS35L32 if I2C > select SND_SOC_CS35L33 if I2C > select SND_SOC_CS35L34 if I2C > @@ -457,6 +458,13 @@ config SND_SOC_CPCAP > config SND_SOC_CQ0093VC > tristate > > +config SND_SOC_CROS_EC_CODEC > + tristate "codec driver for ChromeOS EC" > + depends on MFD_CROS_EC > + help > + If you say yes here you will get support for the > + ChromeOS 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 9bb3346fab2fe..3cfd8f5f61705 100644 > --- a/sound/soc/codecs/Makefile > +++ b/sound/soc/codecs/Makefile > @@ -42,6 +42,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 > @@ -310,6 +311,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..85ea23f4b681c > --- /dev/null > +++ b/sound/soc/codecs/cros_ec_codec.c > @@ -0,0 +1,454 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for ChromeOS Embedded Controller codec. > + * > + * This driver uses the cros-ec interface to communicate with the 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 <linux/platform_device.h> > +#include <sound/pcm.h> > +#include <sound/pcm_params.h> > +#include <sound/soc.h> > +#include <sound/tlv.h> > + > +#define MAX_GAIN 43 Is there some reason for this magic number ? What does it reflect ? > + > +#define DRV_NAME "cros-ec-codec" > + > +/** > + * 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; > +}; > + > +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, u8 *outdata, int outsize, > + u8 *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; As far as I can see, the calling parameter is always "struct ec_param_codec_i2s". With that in mind, this code seems unnecessarily complex. Is this code expected to be called multiple times in parallel and requires multiple instances of struct ec_param_codec_i2s to be allocated ? Even if so, why not just use a local variable / array ? [ This makes me wonder how EC transfers are synchronized in cros_ec_cmd_xfer_status() and below. Does anyone know ? I don't immedately see it. ] > + > + 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; I know this is wide-spread in cros_ec drivers, but that doesn't make it better. The positive return value isn't used anywhere. On top of that, the called low level code already generates error messages. As a result, the code calling this function is unnecessarily complex. If the function was to return 0 for success and a negative error code otherwise, pret much all all callers could be simplified to return ec_command(...); > +} > + > +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, > + (u8 *)¶m, sizeof(param), > + NULL, 0); > + if (ret < 0) { > + dev_err(component->dev, > + "set I2S format to %u command returned %d\n", > + i2s_config, ret); > + return ret; > + } > + 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; > + > + 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; > + } > + > + return set_i2s_config(component, i2s_config); > +} > + > +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, > + (u8 *)¶m, sizeof(param), > + NULL, 0); > + if (ret < 0) { > + dev_err(component->dev, "I2S sample depth %u returned %d\n", > + depth, ret); > + return ret; > + } > + 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, > + (u8 *)¶m, sizeof(param), > + NULL, 0); > + if (ret < 0) { > + dev_err(component->dev, "I2S set bclk %u command returned %d\n", > + bclk, ret); > + return ret; > + } > + 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; There is quite some inconsistency in error messages. Is there some kind of plan behind it ? Do the error mesages above add value, but an error message here wouldn't ? > + } > + if (ret < 0) > + return ret; > + > + bclk = snd_soc_params_to_bclk(params); > + return set_bclk(component, bclk); > +} > + > +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, > + u8 *left, u8 *right) > +{ > + struct ec_param_codec_i2s param; > + struct ec_response_codec_gain resp; > + int ret; > + > + param.cmd = EC_CODEC_GET_GAIN; > + > + ret = ec_command(component, 0, EC_CMD_CODEC_I2S, > + (u8 *)¶m, sizeof(param), > + (u8 *)&resp, sizeof(resp)); > + if (ret < 0) { > + dev_err(component->dev, "I2S get gain command returned %d\n", > + ret); > + return ret; > + } > + > + *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); > + u8 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, > + u8 left, u8 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, > + (u8 *)¶m, sizeof(param), > + NULL, 0); > + if (ret < 0) { > + dev_err(component->dev, "I2S set gain command returned %d\n", > + ret); > + return ret; > + } > + > + 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]; > + > + if (left > MAX_GAIN || right > MAX_GAIN) > + return -EINVAL; > + > + return set_ec_mic_gain(component, (u8)left, (u8)right); > +} > + > +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, > + (u8 *)¶m, sizeof(param), > + NULL, 0); > + if (ret < 0) { > + dev_err(component->dev, "I2S enable %d command returned %d\n", > + enable, ret); > + return ret; > + } > + 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); > + > + 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; > + > + 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)); > + > + return 0; Why ignore errors from snd_soc_register_component() ? Is this on purpose or an oversight ? > +} > + > +#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, > + .of_match_table = of_match_ptr(cros_ec_codec_of_match), > + }, > + .probe = cros_ec_codec_platform_probe, > +}; > + > +module_platform_driver(cros_ec_codec_platform_driver); The "platform" in the various variable and function names don't really add any value. > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("ChromeOS EC codec driver"); > +MODULE_AUTHOR("Cheng-Yi Chiang <cychiang@chromium.org>"); > +MODULE_ALIAS("platform:" DRV_NAME);
On Tue, Jan 15, 2019 at 10:18:02PM -0800, Guenter Roeck wrote: > On Wed, Dec 26, 2018 at 03:03:19PM +0800, Cheng-Yi Chiang wrote: > > 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> > > --- > > Changes in v3: > > 1.remove cros_ec_codec.h. > > 2.Fix error code overriding in > > set_i2s_config > > set_i2s_sample_depth > > set_bclk > > get_ec_mic_gain > > set_ec_mic_gain > > enable_i2s > > 3.Fix missing return code in cros_ec_i2s_set_dai_fmt. > > 4.Simplify return code in cros_ec_i2s_hw_params and mic_gain_put. > > 5.Remove useless debug message in cros_ec_codec_platform_probe. > > > > MAINTAINERS | 2 + > > sound/soc/codecs/Kconfig | 8 + > > sound/soc/codecs/Makefile | 2 + > > sound/soc/codecs/cros_ec_codec.c | 454 +++++++++++++++++++++++++++++++ > > 4 files changed, 466 insertions(+) > > create mode 100644 sound/soc/codecs/cros_ec_codec.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 05e1922624e58..d66f80f3252d7 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3638,8 +3638,10 @@ F: drivers/platform/chrome/ > > > > CHROMEOS EC CODEC DRIVER > > M: Cheng-Yi Chiang <cychiang@chromium.org> > > +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 87cb9c51e6f5a..0b36428159b71 100644 > > --- a/sound/soc/codecs/Kconfig > > +++ b/sound/soc/codecs/Kconfig > > @@ -50,6 +50,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 > > This unconditionally selects SND_SOC_CROS_EC_CODEC, but SND_SOC_CROS_EC_CODEC > depends on MFD_CROS_EC. This is missing an "if MFD_CROS_EC" qualifier. > > > select SND_SOC_CS35L32 if I2C > > select SND_SOC_CS35L33 if I2C > > select SND_SOC_CS35L34 if I2C > > @@ -457,6 +458,13 @@ config SND_SOC_CPCAP > > config SND_SOC_CQ0093VC > > tristate > > > > +config SND_SOC_CROS_EC_CODEC > > + tristate "codec driver for ChromeOS EC" > > + depends on MFD_CROS_EC > > + help > > + If you say yes here you will get support for the > > + ChromeOS 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 9bb3346fab2fe..3cfd8f5f61705 100644 > > --- a/sound/soc/codecs/Makefile > > +++ b/sound/soc/codecs/Makefile > > @@ -42,6 +42,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 > > @@ -310,6 +311,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..85ea23f4b681c > > --- /dev/null > > +++ b/sound/soc/codecs/cros_ec_codec.c > > @@ -0,0 +1,454 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Driver for ChromeOS Embedded Controller codec. > > + * > > + * This driver uses the cros-ec interface to communicate with the 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 <linux/platform_device.h> > > +#include <sound/pcm.h> > > +#include <sound/pcm_params.h> > > +#include <sound/soc.h> > > +#include <sound/tlv.h> > > + > > +#define MAX_GAIN 43 > > Is there some reason for this magic number ? What does it reflect ? > > > + > > +#define DRV_NAME "cros-ec-codec" > > + > > +/** > > + * 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; > > +}; > > + > > +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, u8 *outdata, int outsize, > > + u8 *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; > > As far as I can see, the calling parameter is always "struct ec_param_codec_i2s". > With that in mind, this code seems unnecessarily complex. Is this code expected > to be called multiple times in parallel and requires multiple instances of > struct ec_param_codec_i2s to be allocated ? Even if so, why not just use > a local variable / array ? > > [ This makes me wonder how EC transfers are synchronized in > cros_ec_cmd_xfer_status() and below. Does anyone know ? I don't immedately > see it. > ] > Answering this part myself: locking is implemented in cros_ec_cmd_xfer(), which is called from cros_ec_cmd_xfer_status(). No idea why I didn't see that earlier. Guenter
Hi Guenter, Thanks for the review! On Wed, Jan 16, 2019 at 2:18 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Dec 26, 2018 at 03:03:19PM +0800, Cheng-Yi Chiang wrote: > > 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> > > --- > > Changes in v3: > > 1.remove cros_ec_codec.h. > > 2.Fix error code overriding in > > set_i2s_config > > set_i2s_sample_depth > > set_bclk > > get_ec_mic_gain > > set_ec_mic_gain > > enable_i2s > > 3.Fix missing return code in cros_ec_i2s_set_dai_fmt. > > 4.Simplify return code in cros_ec_i2s_hw_params and mic_gain_put. > > 5.Remove useless debug message in cros_ec_codec_platform_probe. > > > > MAINTAINERS | 2 + > > sound/soc/codecs/Kconfig | 8 + > > sound/soc/codecs/Makefile | 2 + > > sound/soc/codecs/cros_ec_codec.c | 454 +++++++++++++++++++++++++++++++ > > 4 files changed, 466 insertions(+) > > create mode 100644 sound/soc/codecs/cros_ec_codec.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 05e1922624e58..d66f80f3252d7 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3638,8 +3638,10 @@ F: drivers/platform/chrome/ > > > > CHROMEOS EC CODEC DRIVER > > M: Cheng-Yi Chiang <cychiang@chromium.org> > > +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 87cb9c51e6f5a..0b36428159b71 100644 > > --- a/sound/soc/codecs/Kconfig > > +++ b/sound/soc/codecs/Kconfig > > @@ -50,6 +50,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 > > This unconditionally selects SND_SOC_CROS_EC_CODEC, but SND_SOC_CROS_EC_CODEC > depends on MFD_CROS_EC. This is missing an "if MFD_CROS_EC" qualifier. > Will fix in v4. > > select SND_SOC_CS35L32 if I2C > > select SND_SOC_CS35L33 if I2C > > select SND_SOC_CS35L34 if I2C > > @@ -457,6 +458,13 @@ config SND_SOC_CPCAP > > config SND_SOC_CQ0093VC > > tristate > > > > +config SND_SOC_CROS_EC_CODEC > > + tristate "codec driver for ChromeOS EC" > > + depends on MFD_CROS_EC > > + help > > + If you say yes here you will get support for the > > + ChromeOS 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 9bb3346fab2fe..3cfd8f5f61705 100644 > > --- a/sound/soc/codecs/Makefile > > +++ b/sound/soc/codecs/Makefile > > @@ -42,6 +42,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 > > @@ -310,6 +311,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..85ea23f4b681c > > --- /dev/null > > +++ b/sound/soc/codecs/cros_ec_codec.c > > @@ -0,0 +1,454 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Driver for ChromeOS Embedded Controller codec. > > + * > > + * This driver uses the cros-ec interface to communicate with the 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 <linux/platform_device.h> > > +#include <sound/pcm.h> > > +#include <sound/pcm_params.h> > > +#include <sound/soc.h> > > +#include <sound/tlv.h> > > + > > +#define MAX_GAIN 43 > > Is there some reason for this magic number ? What does it reflect ? > This value is determined by the EC chip to represent the maximum gain in dB that EC codec can support. Since it might be different in the future for different EC chip, it needs to be adjustable. I plan to pass this value as "max-dmic-gain" from device property. This driver can lookup the property it its probe function, and modify the max value in mixer control for gain accordingly. I could not find similar use case in other codec driver but it should be doable. Please let me know if this is not encouraged. > > + > > +#define DRV_NAME "cros-ec-codec" > > + > > +/** > > + * 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; > > +}; > > + > > +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, u8 *outdata, int outsize, > > + u8 *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; > > As far as I can see, the calling parameter is always "struct ec_param_codec_i2s". > With that in mind, this code seems unnecessarily complex. Is this code expected > to be called multiple times in parallel and requires multiple instances of > struct ec_param_codec_i2s to be allocated ? Even if so, why not just use > a local variable / array ? > > [ This makes me wonder how EC transfers are synchronized in > cros_ec_cmd_xfer_status() and below. Does anyone know ? I don't immedately > see it. > ] I agree with you suggestion of using local variable. All the needed commands are subcommands in EC_CMD_CODEC_I2S command. So "struct ec_param_codec_i2s" is the only param that will be passed to ec_command function. This ec_command function can be greatly simplified. As for multiple access to this ec_command function, I need to dig more into it. There are locks in stream and kcontrol to guard the access respectively, but I am not sure if stream and kcontrol will access this ec_command function at the same time. It should be safer to use local variable to hold this parameter. > > > + > > + 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; > > I know this is wide-spread in cros_ec drivers, but that doesn't make it better. > The positive return value isn't used anywhere. On top of that, the called low > level code already generates error messages. As a result, the code calling this > function is unnecessarily complex. If the function was to return 0 for success > and a negative error code otherwise, pret much all all callers could be > simplified to > > return ec_command(...); > My intention was to print some error message so I can identify the command that failed. cros_ec_cmd_xfer and cros_ec_cmd_xfer_status have messages for error, but not the command it tried to send. I can remove them since they cause unnecessary complexity. I guess I can still rely on error messages in upper layers to tell what command fails. Will follow your advice in v4. Thanks again! > > +} > > + > > +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, > > + (u8 *)¶m, sizeof(param), > > + NULL, 0); > > + if (ret < 0) { > > + dev_err(component->dev, > > + "set I2S format to %u command returned %d\n", > > + i2s_config, ret); > > + return ret; > > + } > > + 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; > > + > > + 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; > > + } > > + > > + return set_i2s_config(component, i2s_config); > > +} > > + > > +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, > > + (u8 *)¶m, sizeof(param), > > + NULL, 0); > > + if (ret < 0) { > > + dev_err(component->dev, "I2S sample depth %u returned %d\n", > > + depth, ret); > > + return ret; > > + } > > + 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, > > + (u8 *)¶m, sizeof(param), > > + NULL, 0); > > + if (ret < 0) { > > + dev_err(component->dev, "I2S set bclk %u command returned %d\n", > > + bclk, ret); > > + return ret; > > + } > > + 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; > > There is quite some inconsistency in error messages. Is there some kind of > plan behind it ? Do the error mesages above add value, but an error message > here wouldn't ? > > > + } > > + if (ret < 0) > > + return ret; > > + > > + bclk = snd_soc_params_to_bclk(params); > > + return set_bclk(component, bclk); > > +} > > + > > +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, > > + u8 *left, u8 *right) > > +{ > > + struct ec_param_codec_i2s param; > > + struct ec_response_codec_gain resp; > > + int ret; > > + > > + param.cmd = EC_CODEC_GET_GAIN; > > + > > + ret = ec_command(component, 0, EC_CMD_CODEC_I2S, > > + (u8 *)¶m, sizeof(param), > > + (u8 *)&resp, sizeof(resp)); > > + if (ret < 0) { > > + dev_err(component->dev, "I2S get gain command returned %d\n", > > + ret); > > + return ret; > > + } > > + > > + *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); > > + u8 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, > > + u8 left, u8 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, > > + (u8 *)¶m, sizeof(param), > > + NULL, 0); > > + if (ret < 0) { > > + dev_err(component->dev, "I2S set gain command returned %d\n", > > + ret); > > + return ret; > > + } > > + > > + 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]; > > + > > + if (left > MAX_GAIN || right > MAX_GAIN) > > + return -EINVAL; > > + > > + return set_ec_mic_gain(component, (u8)left, (u8)right); > > +} > > + > > +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, > > + (u8 *)¶m, sizeof(param), > > + NULL, 0); > > + if (ret < 0) { > > + dev_err(component->dev, "I2S enable %d command returned %d\n", > > + enable, ret); > > + return ret; > > + } > > + 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); > > + > > + 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; > > + > > + 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)); > > + > > + return 0; > > Why ignore errors from snd_soc_register_component() ? Is this on purpose > or an oversight ? > > > +} > > + > > +#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, > > + .of_match_table = of_match_ptr(cros_ec_codec_of_match), > > + }, > > + .probe = cros_ec_codec_platform_probe, > > +}; > > + > > +module_platform_driver(cros_ec_codec_platform_driver); > > The "platform" in the various variable and function names don't really > add any value. > > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("ChromeOS EC codec driver"); > > +MODULE_AUTHOR("Cheng-Yi Chiang <cychiang@chromium.org>"); > > +MODULE_ALIAS("platform:" DRV_NAME);
On 1/16/19 8:25 PM, Cheng-yi Chiang wrote: > Hi Guenter, > > Thanks for the review! > > On Wed, Jan 16, 2019 at 2:18 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Wed, Dec 26, 2018 at 03:03:19PM +0800, Cheng-Yi Chiang wrote: >>> 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> >>> --- >>> Changes in v3: >>> 1.remove cros_ec_codec.h. >>> 2.Fix error code overriding in >>> set_i2s_config >>> set_i2s_sample_depth >>> set_bclk >>> get_ec_mic_gain >>> set_ec_mic_gain >>> enable_i2s >>> 3.Fix missing return code in cros_ec_i2s_set_dai_fmt. >>> 4.Simplify return code in cros_ec_i2s_hw_params and mic_gain_put. >>> 5.Remove useless debug message in cros_ec_codec_platform_probe. >>> >>> MAINTAINERS | 2 + >>> sound/soc/codecs/Kconfig | 8 + >>> sound/soc/codecs/Makefile | 2 + >>> sound/soc/codecs/cros_ec_codec.c | 454 +++++++++++++++++++++++++++++++ >>> 4 files changed, 466 insertions(+) >>> create mode 100644 sound/soc/codecs/cros_ec_codec.c >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index 05e1922624e58..d66f80f3252d7 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -3638,8 +3638,10 @@ F: drivers/platform/chrome/ >>> >>> CHROMEOS EC CODEC DRIVER >>> M: Cheng-Yi Chiang <cychiang@chromium.org> >>> +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 87cb9c51e6f5a..0b36428159b71 100644 >>> --- a/sound/soc/codecs/Kconfig >>> +++ b/sound/soc/codecs/Kconfig >>> @@ -50,6 +50,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 >> >> This unconditionally selects SND_SOC_CROS_EC_CODEC, but SND_SOC_CROS_EC_CODEC >> depends on MFD_CROS_EC. This is missing an "if MFD_CROS_EC" qualifier. >> > Will fix in v4. >>> select SND_SOC_CS35L32 if I2C >>> select SND_SOC_CS35L33 if I2C >>> select SND_SOC_CS35L34 if I2C >>> @@ -457,6 +458,13 @@ config SND_SOC_CPCAP >>> config SND_SOC_CQ0093VC >>> tristate >>> >>> +config SND_SOC_CROS_EC_CODEC >>> + tristate "codec driver for ChromeOS EC" >>> + depends on MFD_CROS_EC >>> + help >>> + If you say yes here you will get support for the >>> + ChromeOS 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 9bb3346fab2fe..3cfd8f5f61705 100644 >>> --- a/sound/soc/codecs/Makefile >>> +++ b/sound/soc/codecs/Makefile >>> @@ -42,6 +42,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 >>> @@ -310,6 +311,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..85ea23f4b681c >>> --- /dev/null >>> +++ b/sound/soc/codecs/cros_ec_codec.c >>> @@ -0,0 +1,454 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Driver for ChromeOS Embedded Controller codec. >>> + * >>> + * This driver uses the cros-ec interface to communicate with the 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 <linux/platform_device.h> >>> +#include <sound/pcm.h> >>> +#include <sound/pcm_params.h> >>> +#include <sound/soc.h> >>> +#include <sound/tlv.h> >>> + >>> +#define MAX_GAIN 43 >> >> Is there some reason for this magic number ? What does it reflect ? >> > > This value is determined by the EC chip to represent the maximum gain > in dB that EC codec can support. > Since it might be different in the future for different EC chip, it > needs to be adjustable. > I plan to pass this value as "max-dmic-gain" from device property. > This driver can lookup the property it its probe function, and modify > the max value in mixer control for gain accordingly. > I could not find similar use case in other codec driver but it should be doable. > Please let me know if this is not encouraged. > A brief comment such as "Maximum gain in dB supported by EC codec" might be useful. Thanks, Guenter >>> + >>> +#define DRV_NAME "cros-ec-codec" >>> + >>> +/** >>> + * 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; >>> +}; >>> + >>> +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, u8 *outdata, int outsize, >>> + u8 *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; >> >> As far as I can see, the calling parameter is always "struct ec_param_codec_i2s". >> With that in mind, this code seems unnecessarily complex. Is this code expected >> to be called multiple times in parallel and requires multiple instances of >> struct ec_param_codec_i2s to be allocated ? Even if so, why not just use >> a local variable / array ? >> >> [ This makes me wonder how EC transfers are synchronized in >> cros_ec_cmd_xfer_status() and below. Does anyone know ? I don't immedately >> see it. >> ] > > I agree with you suggestion of using local variable. > All the needed commands are subcommands in EC_CMD_CODEC_I2S command. > So "struct ec_param_codec_i2s" is the only param that will be passed > to ec_command function. > This ec_command function can be greatly simplified. > > As for multiple access to this ec_command function, I need to dig more into it. > There are locks in stream and kcontrol to guard the access > respectively, but I am not sure if stream and kcontrol will access > this ec_command function at the same time. > It should be safer to use local variable to hold this parameter. > >> >>> + >>> + 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; >> >> I know this is wide-spread in cros_ec drivers, but that doesn't make it better. >> The positive return value isn't used anywhere. On top of that, the called low >> level code already generates error messages. As a result, the code calling this >> function is unnecessarily complex. If the function was to return 0 for success >> and a negative error code otherwise, pret much all all callers could be >> simplified to >> >> return ec_command(...); >> > > My intention was to print some error message so I can identify the > command that failed. > cros_ec_cmd_xfer and cros_ec_cmd_xfer_status have messages for error, > but not the command it tried to send. > I can remove them since they cause unnecessary complexity. > I guess I can still rely on error messages in upper layers to tell > what command fails. > Will follow your advice in v4. > > Thanks again! > > > >>> +} >>> + >>> +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, >>> + (u8 *)¶m, sizeof(param), >>> + NULL, 0); >>> + if (ret < 0) { >>> + dev_err(component->dev, >>> + "set I2S format to %u command returned %d\n", >>> + i2s_config, ret); >>> + return ret; >>> + } >>> + 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; >>> + >>> + 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; >>> + } >>> + >>> + return set_i2s_config(component, i2s_config); >>> +} >>> + >>> +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, >>> + (u8 *)¶m, sizeof(param), >>> + NULL, 0); >>> + if (ret < 0) { >>> + dev_err(component->dev, "I2S sample depth %u returned %d\n", >>> + depth, ret); >>> + return ret; >>> + } >>> + 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, >>> + (u8 *)¶m, sizeof(param), >>> + NULL, 0); >>> + if (ret < 0) { >>> + dev_err(component->dev, "I2S set bclk %u command returned %d\n", >>> + bclk, ret); >>> + return ret; >>> + } >>> + 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; >> >> There is quite some inconsistency in error messages. Is there some kind of >> plan behind it ? Do the error mesages above add value, but an error message >> here wouldn't ? >> >>> + } >>> + if (ret < 0) >>> + return ret; >>> + >>> + bclk = snd_soc_params_to_bclk(params); >>> + return set_bclk(component, bclk); >>> +} >>> + >>> +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, >>> + u8 *left, u8 *right) >>> +{ >>> + struct ec_param_codec_i2s param; >>> + struct ec_response_codec_gain resp; >>> + int ret; >>> + >>> + param.cmd = EC_CODEC_GET_GAIN; >>> + >>> + ret = ec_command(component, 0, EC_CMD_CODEC_I2S, >>> + (u8 *)¶m, sizeof(param), >>> + (u8 *)&resp, sizeof(resp)); >>> + if (ret < 0) { >>> + dev_err(component->dev, "I2S get gain command returned %d\n", >>> + ret); >>> + return ret; >>> + } >>> + >>> + *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); >>> + u8 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, >>> + u8 left, u8 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, >>> + (u8 *)¶m, sizeof(param), >>> + NULL, 0); >>> + if (ret < 0) { >>> + dev_err(component->dev, "I2S set gain command returned %d\n", >>> + ret); >>> + return ret; >>> + } >>> + >>> + 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]; >>> + >>> + if (left > MAX_GAIN || right > MAX_GAIN) >>> + return -EINVAL; >>> + >>> + return set_ec_mic_gain(component, (u8)left, (u8)right); >>> +} >>> + >>> +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, >>> + (u8 *)¶m, sizeof(param), >>> + NULL, 0); >>> + if (ret < 0) { >>> + dev_err(component->dev, "I2S enable %d command returned %d\n", >>> + enable, ret); >>> + return ret; >>> + } >>> + 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); >>> + >>> + 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; >>> + >>> + 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)); >>> + >>> + return 0; >> >> Why ignore errors from snd_soc_register_component() ? Is this on purpose >> or an oversight ? >> >>> +} >>> + >>> +#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, >>> + .of_match_table = of_match_ptr(cros_ec_codec_of_match), >>> + }, >>> + .probe = cros_ec_codec_platform_probe, >>> +}; >>> + >>> +module_platform_driver(cros_ec_codec_platform_driver); >> >> The "platform" in the various variable and function names don't really >> add any value. >> >>> + >>> +MODULE_LICENSE("GPL v2"); >>> +MODULE_DESCRIPTION("ChromeOS EC codec driver"); >>> +MODULE_AUTHOR("Cheng-Yi Chiang <cychiang@chromium.org>"); >>> +MODULE_ALIAS("platform:" DRV_NAME); >
On Thu, Jan 17, 2019 at 12:25 PM Cheng-yi Chiang <cychiang@chromium.org> wrote: > > Hi Guenter, > > Thanks for the review! > > On Wed, Jan 16, 2019 at 2:18 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Wed, Dec 26, 2018 at 03:03:19PM +0800, Cheng-Yi Chiang wrote: > > > 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> > > > --- > > > Changes in v3: > > > 1.remove cros_ec_codec.h. > > > 2.Fix error code overriding in > > > set_i2s_config > > > set_i2s_sample_depth > > > set_bclk > > > get_ec_mic_gain > > > set_ec_mic_gain > > > enable_i2s > > > 3.Fix missing return code in cros_ec_i2s_set_dai_fmt. > > > 4.Simplify return code in cros_ec_i2s_hw_params and mic_gain_put. > > > 5.Remove useless debug message in cros_ec_codec_platform_probe. > > > > > > MAINTAINERS | 2 + > > > sound/soc/codecs/Kconfig | 8 + > > > sound/soc/codecs/Makefile | 2 + > > > sound/soc/codecs/cros_ec_codec.c | 454 +++++++++++++++++++++++++++++++ > > > 4 files changed, 466 insertions(+) > > > create mode 100644 sound/soc/codecs/cros_ec_codec.c > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index 05e1922624e58..d66f80f3252d7 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -3638,8 +3638,10 @@ F: drivers/platform/chrome/ > > > > > > CHROMEOS EC CODEC DRIVER > > > M: Cheng-Yi Chiang <cychiang@chromium.org> > > > +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 87cb9c51e6f5a..0b36428159b71 100644 > > > --- a/sound/soc/codecs/Kconfig > > > +++ b/sound/soc/codecs/Kconfig > > > @@ -50,6 +50,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 > > > > This unconditionally selects SND_SOC_CROS_EC_CODEC, but SND_SOC_CROS_EC_CODEC > > depends on MFD_CROS_EC. This is missing an "if MFD_CROS_EC" qualifier. > > > Will fix in v4. > > > select SND_SOC_CS35L32 if I2C > > > select SND_SOC_CS35L33 if I2C > > > select SND_SOC_CS35L34 if I2C > > > @@ -457,6 +458,13 @@ config SND_SOC_CPCAP > > > config SND_SOC_CQ0093VC > > > tristate > > > > > > +config SND_SOC_CROS_EC_CODEC > > > + tristate "codec driver for ChromeOS EC" > > > + depends on MFD_CROS_EC > > > + help > > > + If you say yes here you will get support for the > > > + ChromeOS 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 9bb3346fab2fe..3cfd8f5f61705 100644 > > > --- a/sound/soc/codecs/Makefile > > > +++ b/sound/soc/codecs/Makefile > > > @@ -42,6 +42,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 > > > @@ -310,6 +311,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..85ea23f4b681c > > > --- /dev/null > > > +++ b/sound/soc/codecs/cros_ec_codec.c > > > @@ -0,0 +1,454 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Driver for ChromeOS Embedded Controller codec. > > > + * > > > + * This driver uses the cros-ec interface to communicate with the 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 <linux/platform_device.h> > > > +#include <sound/pcm.h> > > > +#include <sound/pcm_params.h> > > > +#include <sound/soc.h> > > > +#include <sound/tlv.h> > > > + > > > +#define MAX_GAIN 43 > > > > Is there some reason for this magic number ? What does it reflect ? > > > > This value is determined by the EC chip to represent the maximum gain > in dB that EC codec can support. > Since it might be different in the future for different EC chip, it > needs to be adjustable. > I plan to pass this value as "max-dmic-gain" from device property. > This driver can lookup the property it its probe function, and modify > the max value in mixer control for gain accordingly. > I could not find similar use case in other codec driver but it should be doable. > Please let me know if this is not encouraged. > > > > + > > > +#define DRV_NAME "cros-ec-codec" > > > + > > > +/** > > > + * 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; > > > +}; > > > + > > > +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, u8 *outdata, int outsize, > > > + u8 *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; > > > > As far as I can see, the calling parameter is always "struct ec_param_codec_i2s". > > With that in mind, this code seems unnecessarily complex. Is this code expected > > to be called multiple times in parallel and requires multiple instances of > > struct ec_param_codec_i2s to be allocated ? Even if so, why not just use > > a local variable / array ? > > > > [ This makes me wonder how EC transfers are synchronized in > > cros_ec_cmd_xfer_status() and below. Does anyone know ? I don't immedately > > see it. > > ] > > I agree with you suggestion of using local variable. > All the needed commands are subcommands in EC_CMD_CODEC_I2S command. > So "struct ec_param_codec_i2s" is the only param that will be passed > to ec_command function. > This ec_command function can be greatly simplified. > > As for multiple access to this ec_command function, I need to dig more into it. > There are locks in stream and kcontrol to guard the access > respectively, but I am not sure if stream and kcontrol will access > this ec_command function at the same time. > It should be safer to use local variable to hold this parameter. > Unfortunately I found that for ec command that needs a response (e.g. getting mic gain from EC), I still need to allocate buffer for message based on max(insize,outsize). But for all other cases where there is no need for response, the code can be simplified. So I plan is to rename this function to ec_command_with_resp static int ec_command_with_resp(struct snd_soc_component *component, u8 *outdata, int outsize, u8 *indata, int insize) And add another function for commands that do not need response: static int ec_command_no_resp(struct snd_soc_component *component, struct ec_param_codec_i2s* param) Many places that use ec_command_no_resp can be greatly simplified like static int set_i2s_bclk(struct snd_soc_component *component, uint32_t bclk) { struct ec_param_codec_i2s param; param.cmd = EC_CODEC_I2S_SET_BCLK; param.bclk = bclk; return ec_command_no_resp(component, ¶m); } > > > > > + > > > + 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; > > > > I know this is wide-spread in cros_ec drivers, but that doesn't make it better. > > The positive return value isn't used anywhere. On top of that, the called low > > level code already generates error messages. As a result, the code calling this > > function is unnecessarily complex. If the function was to return 0 for success > > and a negative error code otherwise, pret much all all callers could be > > simplified to > > > > return ec_command(...); > > > > My intention was to print some error message so I can identify the > command that failed. > cros_ec_cmd_xfer and cros_ec_cmd_xfer_status have messages for error, > but not the command it tried to send. > I can remove them since they cause unnecessary complexity. > I guess I can still rely on error messages in upper layers to tell > what command fails. > Will follow your advice in v4. > > Thanks again! > > > > > > +} > > > + > > > +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, > > > + (u8 *)¶m, sizeof(param), > > > + NULL, 0); > > > + if (ret < 0) { > > > + dev_err(component->dev, > > > + "set I2S format to %u command returned %d\n", > > > + i2s_config, ret); > > > + return ret; > > > + } > > > + 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; > > > + > > > + 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; > > > + } > > > + > > > + return set_i2s_config(component, i2s_config); > > > +} > > > + > > > +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, > > > + (u8 *)¶m, sizeof(param), > > > + NULL, 0); > > > + if (ret < 0) { > > > + dev_err(component->dev, "I2S sample depth %u returned %d\n", > > > + depth, ret); > > > + return ret; > > > + } > > > + 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, > > > + (u8 *)¶m, sizeof(param), > > > + NULL, 0); > > > + if (ret < 0) { > > > + dev_err(component->dev, "I2S set bclk %u command returned %d\n", > > > + bclk, ret); > > > + return ret; > > > + } > > > + 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; > > > > There is quite some inconsistency in error messages. Is there some kind of > > plan behind it ? Do the error mesages above add value, but an error message > > here wouldn't ? > > > > > + } > > > + if (ret < 0) > > > + return ret; > > > + > > > + bclk = snd_soc_params_to_bclk(params); > > > + return set_bclk(component, bclk); > > > +} > > > + > > > +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, > > > + u8 *left, u8 *right) > > > +{ > > > + struct ec_param_codec_i2s param; > > > + struct ec_response_codec_gain resp; > > > + int ret; > > > + > > > + param.cmd = EC_CODEC_GET_GAIN; > > > + > > > + ret = ec_command(component, 0, EC_CMD_CODEC_I2S, > > > + (u8 *)¶m, sizeof(param), > > > + (u8 *)&resp, sizeof(resp)); > > > + if (ret < 0) { > > > + dev_err(component->dev, "I2S get gain command returned %d\n", > > > + ret); > > > + return ret; > > > + } > > > + > > > + *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); > > > + u8 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, > > > + u8 left, u8 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, > > > + (u8 *)¶m, sizeof(param), > > > + NULL, 0); > > > + if (ret < 0) { > > > + dev_err(component->dev, "I2S set gain command returned %d\n", > > > + ret); > > > + return ret; > > > + } > > > + > > > + 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]; > > > + > > > + if (left > MAX_GAIN || right > MAX_GAIN) > > > + return -EINVAL; > > > + > > > + return set_ec_mic_gain(component, (u8)left, (u8)right); > > > +} > > > + > > > +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, > > > + (u8 *)¶m, sizeof(param), > > > + NULL, 0); > > > + if (ret < 0) { > > > + dev_err(component->dev, "I2S enable %d command returned %d\n", > > > + enable, ret); > > > + return ret; > > > + } > > > + 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); > > > + > > > + 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; > > > + > > > + 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)); > > > + > > > + return 0; > > > > Why ignore errors from snd_soc_register_component() ? Is this on purpose > > or an oversight ? > > > > > +} > > > + > > > +#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, > > > + .of_match_table = of_match_ptr(cros_ec_codec_of_match), > > > + }, > > > + .probe = cros_ec_codec_platform_probe, > > > +}; > > > + > > > +module_platform_driver(cros_ec_codec_platform_driver); > > > > The "platform" in the various variable and function names don't really > > add any value. > > > > > + > > > +MODULE_LICENSE("GPL v2"); > > > +MODULE_DESCRIPTION("ChromeOS EC codec driver"); > > > +MODULE_AUTHOR("Cheng-Yi Chiang <cychiang@chromium.org>"); > > > +MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/MAINTAINERS b/MAINTAINERS index 05e1922624e58..d66f80f3252d7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3638,8 +3638,10 @@ F: drivers/platform/chrome/ CHROMEOS EC CODEC DRIVER M: Cheng-Yi Chiang <cychiang@chromium.org> +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 87cb9c51e6f5a..0b36428159b71 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -50,6 +50,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 @@ -457,6 +458,13 @@ config SND_SOC_CPCAP config SND_SOC_CQ0093VC tristate +config SND_SOC_CROS_EC_CODEC + tristate "codec driver for ChromeOS EC" + depends on MFD_CROS_EC + help + If you say yes here you will get support for the + ChromeOS 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 9bb3346fab2fe..3cfd8f5f61705 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -42,6 +42,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 @@ -310,6 +311,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..85ea23f4b681c --- /dev/null +++ b/sound/soc/codecs/cros_ec_codec.c @@ -0,0 +1,454 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Driver for ChromeOS Embedded Controller codec. + * + * This driver uses the cros-ec interface to communicate with the 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 <linux/platform_device.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/tlv.h> + +#define MAX_GAIN 43 + +#define DRV_NAME "cros-ec-codec" + +/** + * 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; +}; + +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, u8 *outdata, int outsize, + u8 *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, + (u8 *)¶m, sizeof(param), + NULL, 0); + if (ret < 0) { + dev_err(component->dev, + "set I2S format to %u command returned %d\n", + i2s_config, ret); + return ret; + } + 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; + + 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; + } + + return set_i2s_config(component, i2s_config); +} + +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, + (u8 *)¶m, sizeof(param), + NULL, 0); + if (ret < 0) { + dev_err(component->dev, "I2S sample depth %u returned %d\n", + depth, ret); + return ret; + } + 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, + (u8 *)¶m, sizeof(param), + NULL, 0); + if (ret < 0) { + dev_err(component->dev, "I2S set bclk %u command returned %d\n", + bclk, ret); + return ret; + } + 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 < 0) + return ret; + + bclk = snd_soc_params_to_bclk(params); + return set_bclk(component, bclk); +} + +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, + u8 *left, u8 *right) +{ + struct ec_param_codec_i2s param; + struct ec_response_codec_gain resp; + int ret; + + param.cmd = EC_CODEC_GET_GAIN; + + ret = ec_command(component, 0, EC_CMD_CODEC_I2S, + (u8 *)¶m, sizeof(param), + (u8 *)&resp, sizeof(resp)); + if (ret < 0) { + dev_err(component->dev, "I2S get gain command returned %d\n", + ret); + return ret; + } + + *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); + u8 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, + u8 left, u8 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, + (u8 *)¶m, sizeof(param), + NULL, 0); + if (ret < 0) { + dev_err(component->dev, "I2S set gain command returned %d\n", + ret); + return ret; + } + + 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]; + + if (left > MAX_GAIN || right > MAX_GAIN) + return -EINVAL; + + return set_ec_mic_gain(component, (u8)left, (u8)right); +} + +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, + (u8 *)¶m, sizeof(param), + NULL, 0); + if (ret < 0) { + dev_err(component->dev, "I2S enable %d command returned %d\n", + enable, ret); + return ret; + } + 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); + + 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; + + 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)); + + 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, + .of_match_table = of_match_ptr(cros_ec_codec_of_match), + }, + .probe = cros_ec_codec_platform_probe, +}; + +module_platform_driver(cros_ec_codec_platform_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("ChromeOS EC codec driver"); +MODULE_AUTHOR("Cheng-Yi Chiang <cychiang@chromium.org>"); +MODULE_ALIAS("platform:" DRV_NAME);
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> --- Changes in v3: 1.remove cros_ec_codec.h. 2.Fix error code overriding in set_i2s_config set_i2s_sample_depth set_bclk get_ec_mic_gain set_ec_mic_gain enable_i2s 3.Fix missing return code in cros_ec_i2s_set_dai_fmt. 4.Simplify return code in cros_ec_i2s_hw_params and mic_gain_put. 5.Remove useless debug message in cros_ec_codec_platform_probe. MAINTAINERS | 2 + sound/soc/codecs/Kconfig | 8 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/cros_ec_codec.c | 454 +++++++++++++++++++++++++++++++ 4 files changed, 466 insertions(+) create mode 100644 sound/soc/codecs/cros_ec_codec.c