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

On Mon, Jul 14, 2008 at 12:45:55PM +0100, Mark Brown wrote:

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

quoted

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.

quoted

+	/* 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.

Yes, this should be done better.  I'm working on making this better, but
I'm hoping it is okay to have that as follow-on patches.

quoted

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

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

done

quoted

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

quoted

+	/* 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.

Cleaned up.

quoted

+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.

Originally, that had been to keep the line under 80 chars, but some of
the names have changed since then to make it no longer necessary.  I'll
clean up the checkpatch stuff.

quoted

+	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.

All the written bit does is trigger the keyclick event.  It doesn't
adjust the parameters in any way.

quoted

+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.

removed

quoted

+#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.

Doesn't #if defined() match on both static and module selection?

quoted

+#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.

fixed.

Thanks,
g.