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.