[alsa-devel] [PATCH 1/3] sound: soc: codecs: Add es8328 codec

[Mark Brown]

On Fri, Feb 07, 2014 at 01:05:15PM +0800, Sean Cross wrote:

Please use subject likes matching the style for the subsystem.  If your
changelog looks different to others in the same area it probably needs
an update.

In general this looks like it should be making much more use of the
framework rather than open coding, it looks like it's very much hard
coded for one use cae.

> +config SND_SOC_ES8328
> +	tristate
> +

It looks like you're going to use this on an ARM system, you should add
DT support and make it visible in Kconfig.

> +static const struct snd_soc_dapm_widget es8328_dapm_widgets[] = {
> +	SND_SOC_DAPM_DAC("Speaker Volume", "HiFi Playback", SND_SOC_NOPM, 0, 0),

Don't declare a stream by name, use DAPM routes to connect the stream to
the widget.

> +	SND_SOC_DAPM_OUTPUT("VOUTL"),
> +	SND_SOC_DAPM_OUTPUT("VOUTR"),
> +        SND_SOC_DAPM_INPUT("LINE_IN"),
> +        SND_SOC_DAPM_INPUT("MIC_IN"),
> +        SND_SOC_DAPM_OUTPUT("HP_OUT"),
> +        SND_SOC_DAPM_OUTPUT("SPK_OUT"),

Something is messed up with your indentation, spaces vs tabs I expect.

> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {

An else clause is more idiomatic here.

> +		snd_soc_write(codec, ES8328_ADCCONTROL5, adc);

It's more idiomatic to use update_bits() here and save doing a manual
read/modify/write cycle - it also avoids the write if not needed.

> +static int es8328_adc_enable(struct snd_soc_codec *codec)
> +{
> +	u16 reg = snd_soc_read(codec, ES8328_CHIPPOWER);
> +	reg &= ~(ES8328_CHIPPOWER_ADCVREF_OFF |
> +		 ES8328_CHIPPOWER_ADCPLL_OFF |
> +		 ES8328_CHIPPOWER_ADCSTM_RESET |
> +		 ES8328_CHIPPOWER_ADCDIG_OFF);
> +	snd_soc_write(codec, ES8328_CHIPPOWER, reg);

This looks like it should be done in DAPM.

> +
> +	/* Set up microphone to be differential input */
> +	snd_soc_write(codec, ES8328_ADCCONTROL2, 0xf0);

This looks like something that should be platform data and/or DAPM -
other platforms may be wired differently.

> +	/* Set ADC to act as I2S master */
> +	snd_soc_write(codec, ES8328_ADCCONTROL3, 0x02);

set_dai_fmt().

> +	/* Set I2S to 16-bit mode */
> +	snd_soc_write(codec, ES8328_ADCCONTROL4, 0x18);

This should be being done in hw_params.

> +	/* Frequency clock of 272 */
> +	snd_soc_write(codec, ES8328_ADCCONTROL5, 0x02);

What is the frequency clock in this context?

> +	/* Power up LOUT2 ROUT2, and power down xOUT1 */
> +	snd_soc_write(codec, ES8328_DACPOWER,
> +			ES8328_DACPOWER_ROUT2_ON |
> +			ES8328_DACPOWER_LOUT2_ON);

This looks like it should be being done in DAPM.

> +	/* Enable click-free power up */
> +	snd_soc_write(codec, ES8328_DACCONTROL6, ES8328_DACCONTROL6_CLICKFREE);
> +	snd_soc_write(codec, ES8328_DACCONTROL3, 0x36);

Just do this once on startup?

> +	/* Set I2S to 16-bit mode */
> +	snd_soc_write(codec, ES8328_DACCONTROL1, ES8328_DACCONTROL1_DACWL_16);

hw_params().

> +	/* No attenuation */
> +	snd_soc_write(codec, ES8328_DACCONTROL4, 0x00);
> +	snd_soc_write(codec, ES8328_DACCONTROL5, 0x00);

This and the rest of the function looks like it should be done in a
combination of DAPM and normal ALSA controls.

> +	for (i = 0; i < 4; i++)
> +		snd_soc_write(codec, i + ES8328_DACCONTROL24, old_volumes[i]);

You are probably looking for something like SOC_DAPM_SINGLE_AUTODISABLE.

> +static const struct snd_soc_dai_ops es8328_dai_ops = {
> +	.hw_params	= es8328_hw_params,
> +	.prepare	= es8328_pcm_prepare,
> +	.shutdown	= es8328_pcm_shutdown,
> +//	.digital_mute	= es8328_mute,

Hrm?

> +static const struct of_device_id es8328_of_match[] = {
> +	{ .compatible = "everest,es8328", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, es8328_of_match);

Any device tree device needs a binding document.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140207/73030e86/attachment.sig>