On Wed, Dec 08, 2010 at 03:02:52PM +0300, Alexander wrote:
> From: Alexander Sverdlin <subaparts@xxxxxxxxx>
>
> Added support for CS4271 codec to ASoC.
>
> Signed-off-by: Alexander Sverdlin <subaparts@xxxxxxxxx>
Overall this looks very good but a few issues below, mostly coding
style/clarity.
> +static int cs4271_set_dai_fmt(struct snd_soc_dai *codec_dai,
> + unsigned int format)
> +{
> + struct snd_soc_codec *codec = codec_dai->codec;
> + struct cs4271_private *cs4271 = snd_soc_codec_get_drvdata(codec);
> +
> + cs4271->mode = format & SND_SOC_DAIFMT_FORMAT_MASK;
> +
> + switch (format & SND_SOC_DAIFMT_MASTER_MASK) {
> + default:
> + dev_err(codec->dev, "Invalid DAI format\n");
> + return -EINVAL;
> + case SND_SOC_DAIFMT_CBS_CFS:
> + cs4271->master = 0;
> + break;
> + case SND_SOC_DAIFMT_CBM_CFM:
> + cs4271->master = 1;
> + }
Missing break; It's also much more usual to have the default: case be
the last one. Similarly for your other switch statements.
> + /* Configure ADC */
> + snd_soc_update_bits(codec, CS4271_ADCCTL, CS4271_ADCCTL_ADC_DIF_MASK,
> + (cs4271->mode == SND_SOC_DAIFMT_I2S) ?
> + CS4271_ADCCTL_ADC_DIF_I2S : CS4271_ADCCTL_ADC_DIF_LJ);
Please don't use the ternery operator like this, it's not terribly
legible.
> +static const char *cs4271_de_texts[] = {"None", "44.1KHz", "48KHz", "32KHz"};
> +static const struct soc_enum cs4271_de_enum =
> + SOC_ENUM_SINGLE(CS4271_DACCTL, 4, 4, cs4271_de_texts);
Ideally the deemphasis would be managed based on the sample rate.
> +struct snd_soc_dai_driver cs4271_dai = {
> + .name = "cs4271-hifi",
> + .playback = {
> + .stream_name = "Playback",
> + .channels_min = 2,
> + .channels_max = 2,
> + .rates = SNDRV_PCM_RATE_8000_96000,
> + .rate_min = 8000,
> + .rate_max = 96000,
No need to set rate_min and rate_max if rates is set.
Looking at the code above I suspect you need to set symmetric_rates on
the DAI too - it looks like the playback and capture must be at the same
rate.
> +/*
> + * This function writes all writeble registers from cache to codec.
> + * It's used to setup initial config and restore after suspend.
> + */
> +static int cs4271_write_cache(struct snd_soc_codec *codec)
> +{
> + int i, ret;
> +
> + for (i = CS4271_LASTREG; i >= CS4271_FIRSTREG; i--) {
> + ret = snd_soc_write(codec, i, snd_soc_read(codec, i));
> + if (ret) {
> + dev_err(codec->dev, "Cache write failed\n");
> + return ret;
> + }
> + }
If you use the standard cache code in soc-cache.c there's a standard
cache sync implementation you could use - snd_soc_cache_sync().
> +#ifdef CONFIG_PM
> +static int cs4271_soc_suspend(struct snd_soc_codec *codec, pm_message_t mesg)
> +{
> + /* Set power-down bit */
> + snd_soc_update_bits(codec, CS4271_MODE2, CS4271_MODE2_PDN,
> + CS4271_MODE2_PDN);
> +
> + return 0;
> +}
> +#else
> +#define cs4271_soc_suspend NULL
> +#endif /* CONFIG_PM */
> +
> +static int cs4271_soc_resume(struct snd_soc_codec *codec)
> +{
A comment explaining why this isn't in the ifdef above would be useful.
I guessed why it was being done before I searched but it'd be good to be
clear.
> + if ((gpio_disable >= 0) &&
> + gpio_request(gpio_disable, "CS4271 Disable"))
> + gpio_disable = -EINVAL;
> + if (gpio_disable >= 0)
> + gpio_direction_output(gpio_disable, 0);
This is quite hard to read. One issue is that the indentation of the
second argument to the && is very confusing as it's lined up with the
contents of the if statement, the other is that non-explicit check on
the return value of gpio_request() means the expected value is not so
clear as it could be.
> +static struct i2c_driver cs4271_i2c_driver = {
> + .driver = {
> + .name = "cs4271-codec",
No need for the -codec.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
