[alsa-devel] [PATCH v3 4/5] ASoC: sirf-soc-inner: add drivers for both CPU and Codec DAIs

[Mark Brown]

On Fri, Jan 03, 2014 at 02:05:03PM +0800, RongJun Ying wrote:

> @@ -6,5 +6,8 @@ config SND_SIRF_SOC
>  config SND_SOC_SIRF_I2S
>  	tristate
>  
> +config SND_SIRF_SOC_INNER
> +	tristate
> +
>  config SND_SOC_SIRF_USP

Everything else is SND_SOC_SIRF_, please keep this consistent.

> +static struct sirf_soc_inner_audio_reg_bits sirf_soc_inner_audio_reg_bits_prima2 = {
> +	20, 21, 22, 23, 24, 25, 26, 27,
> +};

> +static struct sirf_soc_inner_audio_reg_bits sirf_soc_inner_audio_reg_bits_atlas6 = {
> +	22, 23, 24, 25, 26, 27, 28, 29,
> +};

Please used named initialisers to set these values rather than just a
list of numbers, this isn't very easy to read.

> +static const struct snd_kcontrol_new sirf_inner_input_mode_control =
> +	SOC_DAPM_ENUM("Route", sirf_inner_enum[0]);

Either use constants to reference the array or (better) use named
variables for each enum.  This is more readable and less error prone.

> +static struct snd_kcontrol_new volume_controls_atlas6[] = {
> +	SOC_DOUBLE("Playback Volume", AUDIO_IC_CODEC_CTRL0, 21, 14,
> +			0x7F, 0),
> +	SOC_DOUBLE("Capture Volume", AUDIO_IC_CODEC_CTRL1, 16, 10,
> +			0x3F, 0),

Please provide TLV information.

> +static struct snd_kcontrol_new left_input_path_controls[] = {
> +	SOC_DAPM_SINGLE("Line left Switch", AUDIO_IC_CODEC_CTRL1, 6, 1, 0),

Line Left Switch.

> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		/* Enable capture power of codec*/
> +		val = sirfsoc_rtc_iobrg_readl(sinner_audio->sys_pwrc_reg_base +
> +			PWRC_PDN_CTRL_OFFSET);
> +		val |= (1 << AUDIO_POWER_EN_BIT);
> +		sirfsoc_rtc_iobrg_writel(val,
> +			sinner_audio->sys_pwrc_reg_base + PWRC_PDN_CTRL_OFFSET);
> +		break;
> +	case SND_SOC_DAPM_POST_PMU:
> +		/*After enable adc, Delay 200ms to avoid pop noise*/
> +		msleep(200);

No need to split these operations, just have one callback.  Why is there
no power down callback?

> +static int hp_amp_left_event(struct snd_soc_dapm_widget *w,
> +		struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = w->codec;
> +	struct sirf_soc_inner_audio *sinner_audio = dev_get_drvdata(codec->dev);
> +	u32 val;
> +
> +	val = snd_soc_read(codec, AUDIO_IC_CODEC_CTRL1);
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		val |= (1 << sinner_audio->reg_bits->firdac_hsl_en_bits);
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		val &= ~(1 << sinner_audio->reg_bits->firdac_hsl_en_bits);
> +	default:
> +		break;
> +	}
> +	snd_soc_write(codec, AUDIO_IC_CODEC_CTRL1, val);
> +	return 0;

snd_soc_update_bits().  Can this not be represented as multiple widgets
- remember that DAPM will coalesce writes as much as it can.

> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		spin_lock(&sinner_audio->lock);
> +		if (playback) {

The lock only gets used in the trigger function, is it needed?

> +static struct snd_soc_codec_driver soc_codec_device_sirf_inner_codec = {
> +	.probe = sirf_inner_codec_probe,
> +	.remove = sirf_inner_codec_remove,
> +	.read = sirf_inner_codec_reg_read,
> +	.write = sirf_inner_codec_reg_write,

Don't use custom read and write callbacks, use a MMIO regmap.
-------------- 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/20140106/2ecc4e61/attachment.sig>