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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
