[alsa-devel] [PATCH v2 3/3] ALSA SoC: Add Texas Instruments TLV320AIC26 codec driver

[Mark Brown]

On Sat, Jul 12, 2008 at 02:39:39AM -0600, Grant Likely wrote:

> ASoC Codec driver for the TLV320AIC26 device.  This driver uses the ASoC
> v1 API, so I don't expect it to get merged as-is, but I want to get it
> out there for review.

This looks basically good - most of the issues below are nitpicky.

> +	/* Configure PLL */
> +	pval = 1;
> +	jval = (fsref == 44100) ? 7 : 8;
> +	dval = (fsref == 44100) ? 5264 : 1920;
> +	qval = 0;
> +	reg = 0x8000 | qval << 11 | pval << 8 | jval << 2;
> +	aic26_reg_write(codec, AIC26_REG_PLL_PROG1, reg);
> +	reg = dval << 2;
> +	aic26_reg_write(codec, AIC26_REG_PLL_PROG2, reg);

Does the PLL configuration not depend on the input clock frequency?  You
have a set_sysclk() method to configure the MCLK supplied but the driver
never appears to reference the value anywhere.

> +	/* Power up CODEC */
> +	aic26_reg_write(codec, AIC26_REG_POWER_CTRL, 0);

As discussed this should probably just be removed from hw_params().

> +static int aic26_set_fmt(struct snd_soc_codec_dai *codec_dai, unsigned int fmt)
> +{

> +	/* interface format */
> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S: aic26->datfm = AIC26_DATFM_I2S; break;
> +	case SND_SOC_DAIFMT_DSP_A: aic26->datfm = AIC26_DATFM_DSP; break;
> +	case SND_SOC_DAIFMT_RIGHT_J: aic26->datfm = AIC26_DATFM_RIGHTJ; break;
> +	case SND_SOC_DAIFMT_LEFT_J: aic26->datfm = AIC26_DATFM_LEFTJ; break;
> +	default: dev_dbg(&aic26->spi->dev, "bad format\n"); return -EINVAL;
> +	}

I'm having a hard time liking this indentation style.  It's not an
obstacle to merging but it doesn't really help legibility - there's not
enough white space and once you have a non-standard line like the
default: one.

> +static const char *aic26_capture_src_text[] = {"Mic", "Aux"};
> +static const struct soc_enum aic26_capture_src_enum =
> +	SOC_ENUM_SINGLE(AIC26_REG_AUDIO_CTRL1, 12,2, aic26_capture_src_text);

checkpatch complains about the 12,2 here and a bunch of other stuff -
ALSA is generally very strict about that.

> +	SOC_SINGLE("Keyclick activate", AIC26_REG_AUDIO_CTRL2, 15, 0x1, 0),
> +	SOC_SINGLE("Keyclick amplitude", AIC26_REG_AUDIO_CTRL2, 12, 0x7, 0),
> +	SOC_SINGLE("Keyclick frequency", AIC26_REG_AUDIO_CTRL2, 8, 0x7, 0),
> +	SOC_SINGLE("Keyclick period", AIC26_REG_AUDIO_CTRL2, 4, 0xf, 0),

This configuration is also exposed via a sysfs file, including some of
the configurability.  Exposing the information via sysfs isn't a
particular problem but allowing it to be written to without going
through ALSA seems wrong.

> +static ssize_t aic26_regs_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{

As discussed this is replicating the existing codec register display
sysfs file.

> +#if defined(CONFIG_SND_SOC_OF)
> +	/* Tell the of_soc helper about this codec */
> +	of_snd_soc_register_codec(&aic26_soc_codec_dev, aic26, &aic26_dai,
> +				  spi->dev.archdata.of_node);
> +#endif

CONFIG_SND_SOC_OF could be a module - this needs to also check for it
being a module.

> +#define AIC26_RATES	(SNDRV_PCM_RATE_8000  | SNDRV_PCM_RATE_11025 |\
> +			 SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
> +			 SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> +			 SNDRV_PCM_RATE_48000)
> +#define AIC26_FORMATS	(SNDRV_PCM_FMTBIT_S8     | SNDRV_PCM_FMTBIT_S16_BE |\
> +			 SNDRV_PCM_FMTBIT_S24_BE | SNDRV_PCM_FMTBIT_S32_BE)

These are usually kept in the driver code.