[alsa-devel] [PATCH 1/3] sound: soc: codecs: Add es8328 codec
Lars-Peter Clausen
lars at metafoo.de
Sat Feb 8 17:06:39 CET 2014
On 02/07/2014 06:05 AM, Sean Cross wrote:
> Add support for the ES8328 audio codec.
Hi,
a couple of minor coments. Also when submitting a patch it is a good idea to
run scripts/checkpatch.pl on the patch itself and `make C=2` and `make C=2
CHECK=scripts/coccicheck` on your driver module (E.g. in your case `make C=2
sound/soc/codecs/es8328.o`). These tools check your driver for a couple of
known issues. Fixing those before the submission will make the reviews job
easier.
> [...]
> +uint8_t sample_ratios[] = {
static const
> + [ES8328_RATE_8019 ] = 0x9,
> + [ES8328_RATE_11025] = 0x7,
> + [ES8328_RATE_22050] = 0x4,
> + [ES8328_RATE_44100] = 0x2,
> +};
> [...]
> +
> +static int es8328_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct snd_soc_codec *codec = dai->codec;
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + u8 dac = snd_soc_read(codec, ES8328_DACCONTROL2);
> + dac &= ~ES8328_DACCONTROL2_RATEMASK;
> +
> + switch (params_rate(params)) {
> + case 8000:
> + dac |= sample_ratios[ES8328_RATE_8019];
> + break;
> + case 11025:
> + dac |= sample_ratios[ES8328_RATE_11025];
> + break;
> + case 22050:
> + dac |= sample_ratios[ES8328_RATE_22050];
> + break;
> + case 44100:
> + dac |= sample_ratios[ES8328_RATE_44100];
> + break;
> + default:
> + dev_err(codec->dev, "%s: unknown rate %d\n",
> + __func__, params_rate(params));
> + return -EINVAL;
> + }
> + snd_soc_write(codec, ES8328_DACCONTROL2, dac);
> + }
> +
> + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> + u8 adc = snd_soc_read(codec, ES8328_ADCCONTROL5);
> + adc &= ~ES8328_ADCCONTROL5_RATEMASK;
> +
> + switch (params_rate(params)) {
> + case 8000:
> + adc |= sample_ratios[ES8328_RATE_8019];
> + break;
> + case 11025:
> + adc |= sample_ratios[ES8328_RATE_11025];
> + break;
> + case 22050:
> + adc |= sample_ratios[ES8328_RATE_22050];
> + break;
> + case 44100:
> + adc |= sample_ratios[ES8328_RATE_44100];
> + break;
> + default:
> + dev_err(codec->dev, "%s: unknown rate %d\n",
> + __func__, params_rate(params));
> + return -EINVAL;
> + }
> + snd_soc_write(codec, ES8328_ADCCONTROL5, adc);
> + }
The only difference between the capture and the playback path seems to be
the register that's written two. By adding a variable that holds the
register number and gets initialized based on the direction you can elimate
about half of the code in this function.
> +
> + return 0;
[...]
> +static int es8328_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> + int clk_id, unsigned int freq, int dir)
> +{
> + struct snd_soc_codec *codec = codec_dai->codec;
> + struct es8328_priv *es8328 = snd_soc_codec_get_drvdata(codec);
> +
> + switch (clk_id) {
> + case 0:
> + es8328->sysclk = freq;
es8328->sysclk seems to be unused. I guess it might be used if you add
support for different base clock rates. For now you should probably return
an error if freq != 22579200.
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
[...]
> +static int es8328_dac_enable(struct snd_soc_codec *codec)
> +{
> + u16 old_volumes[4];
> + u16 reg;
> + int i;
> +
> + for (i = 0; i < 4; i++) {
> + old_volumes[i] = snd_soc_read(codec, i + ES8328_DACCONTROL24);
> + snd_soc_write(codec, i + ES8328_DACCONTROL24, 0);
> + }
This looks like a job for AUTOMUTE controls. They'll set the volume to zero
before the DAC is powered down and restore the original value after the DAC
was powered up.
> +
> + /* Power up LOUT2 ROUT2, and power down xOUT1 */
> + snd_soc_write(codec, ES8328_DACPOWER,
> + ES8328_DACPOWER_ROUT2_ON |
> + ES8328_DACPOWER_LOUT2_ON);
> +
> + /* Enable click-free power up */
> + snd_soc_write(codec, ES8328_DACCONTROL6, ES8328_DACCONTROL6_CLICKFREE);
> + snd_soc_write(codec, ES8328_DACCONTROL3, 0x36);
> +
> + /* Set I2S to 16-bit mode */
> + snd_soc_write(codec, ES8328_DACCONTROL1, ES8328_DACCONTROL1_DACWL_16);
> +
> + /* No attenuation */
> + snd_soc_write(codec, ES8328_DACCONTROL4, 0x00);
> + snd_soc_write(codec, ES8328_DACCONTROL5, 0x00);
> +
> + /* Set LIN2 for the output mixer */
> + snd_soc_write(codec, ES8328_DACCONTROL16,
> + ES8328_DACCONTROL16_RMIXSEL_RIN2 |
> + ES8328_DACCONTROL16_LMIXSEL_LIN2);
> +
> + /* Point the left DAC at the left mixer */
> + snd_soc_write(codec, ES8328_DACCONTROL17, ES8328_DACCONTROL17_LD2LO);
> + /* Point the right DAC at the right mixer */
> + snd_soc_write(codec, ES8328_DACCONTROL20, ES8328_DACCONTROL20_RD2RO);
> +
> + /* Disable all other outputs */
> + snd_soc_write(codec, ES8328_DACCONTROL18, 0x00);
> + snd_soc_write(codec, ES8328_DACCONTROL19, 0x00);
> +
> +
> + /* Disable mono mode for DACL, and mute DACR */
> + snd_soc_write(codec, ES8328_DACCONTROL7, 0x00);
> +
> + for (i = 0; i < 4; i++)
> + snd_soc_write(codec, i + ES8328_DACCONTROL24, old_volumes[i]);
> +
> + reg = snd_soc_read(codec, ES8328_CHIPPOWER);
> + reg &= ~(ES8328_CHIPPOWER_DACVREF_OFF |
> + ES8328_CHIPPOWER_DACPLL_OFF |
> + ES8328_CHIPPOWER_DACSTM_RESET |
> + ES8328_CHIPPOWER_DACDIG_OFF);
> + snd_soc_write(codec, ES8328_CHIPPOWER, reg);
> + snd_soc_write(codec, ES8328_DACCONTROL3, 0x32);
> +
> + return 0;
> +}
[...]
> +static int es8328_suspend(struct snd_soc_codec *codec)
> +{
> + return 0;
> +}
> +
If you don't do anything in suspend there is no need to provide a callback
function.
> +static int es8328_resume(struct snd_soc_codec *codec)
> +{
> + es8328_init(codec);
> + return 0;
> +}
> +
> +static int es8328_probe(struct snd_soc_codec *codec)
> +{
> + int ret;
> + struct device *dev = codec->dev;
> +
> + ret = snd_soc_codec_set_cache_io(codec, 7, 9, SND_SOC_REGMAP);
> + if (ret < 0) {
> + dev_err(dev, "failed to configure cache I/O: %d\n", ret);
> + return ret;
> + }
> +
> + /* power on device */
> + es8328_init(codec);
> +
> + return 0;
> +}
> +
> +static int es8328_remove(struct snd_soc_codec *codec)
> +{
> + /* Power everything down and reset the cip */
> + snd_soc_write(codec, ES8328_CHIPPOWER,
> + ES8328_CHIPPOWER_DACSTM_RESET |
> + ES8328_CHIPPOWER_ADCSTM_RESET |
> + ES8328_CHIPPOWER_DACDIG_OFF |
> + ES8328_CHIPPOWER_ADCDIG_OFF |
> + ES8328_CHIPPOWER_DACVREF_OFF |
> + ES8328_CHIPPOWER_ADCVREF_OFF);
> +
> + return 0;
> +}
> +
> +static struct snd_soc_codec_driver soc_codec_dev_es8328 = {
es8328_codec_driver is a better name.
THe soc_codec_dev_... naming scheme you see in some drivers comes from a pre
multi-component era where it was more firt
> + .probe = es8328_probe,
> + .remove = es8328_remove,
> + .suspend = es8328_suspend,
> + .resume = es8328_resume,
> + .controls = es8328_snd_controls,
> + .num_controls = ARRAY_SIZE(es8328_snd_controls),
> + .dapm_widgets = es8328_dapm_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(es8328_dapm_widgets),
> + .dapm_routes = es8328_intercon,
> + .num_dapm_routes = ARRAY_SIZE(es8328_intercon),
> +};
[...]
> +#if defined(CONFIG_SPI_MASTER)
> +static int es8328_spi_probe(struct spi_device *spi)
> +{
> + struct es8328_priv *es8328;
> + int ret;
> +
> + es8328 = devm_kzalloc(&spi->dev, sizeof(struct es8328_priv),
> + GFP_KERNEL);
sizeof(*es8328) is the preferred style for kernel code.
> + if (es8328 == NULL)
> + return -ENOMEM;
> +
> + es8328->regmap = devm_regmap_init_spi(spi, &es8328_regmap);
> + if (IS_ERR(es8328->regmap))
> + return PTR_ERR(es8328->regmap);
> +
> + spi_set_drvdata(spi, es8328);
> +
> + ret = snd_soc_register_codec(&spi->dev,
> + &soc_codec_dev_es8328, &es8328_dai, 1);
> + if (ret < 0)
> + dev_err(&spi->dev, "unable to register codec: %d\n", ret);
> +
> + return ret;
> +}
[...]
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> +static int es8328_i2c_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct es8328_priv *es8328;
> + int ret;
> +
> + es8328 = devm_kzalloc(&i2c->dev, sizeof(struct es8328_priv),
> + GFP_KERNEL);
same here.
> + if (es8328 == NULL)
> + return -ENOMEM;
> +
> + es8328->regmap = devm_regmap_init_i2c(i2c, &es8328_regmap);
> + if (IS_ERR(es8328->regmap))
> + return PTR_ERR(es8328->regmap);
> +
> + i2c_set_clientdata(i2c, es8328);
> +
> + ret = snd_soc_register_codec(&i2c->dev,
> + &soc_codec_dev_es8328, &es8328_dai, 1);
> +
> + return ret;
> +}
> +
[...]
More information about the Alsa-devel
mailing list