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;
+}
[...]