On Thu, Sep 18, 2008 at 03:03:09PM +0200, Manuel Lauss wrote:
Driver for the Analog Devices AD1935/AD1937 I2C and AD1938/AD1939 SPI codecs.
Thanks. Overall this looks good, I've got some comments below but there's only some fairly minor stuff that *needs* to be addressed before merging.
--- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -2,6 +2,9 @@ config SND_SOC_AC97_CODEC tristate select SND_AC97_CODEC
+config SND_SOC_AD1939
- tristate
Please also add this to the SND_SOC_ALL_CODECS Kconfig option.
@@ -1,4 +1,5 @@ snd-soc-ac97-objs := ac97.o +snd-soc-ad1939-objs := ad1939.o snd-soc-ak4535-objs := ak4535.o
Not sure if git will apply this cleanly - best to generate against the topic/asoc branch of Takashi's tree:
git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
+/* Does the codec have power? In case of STANDBY/OFF BIAS levels, the
- hardware access functions below assume the codec is without power
- to avoid futile bus transactions.
- */
+static int codec_is_powered(struct snd_soc_codec *c) +{
- return (c->bias_level == SND_SOC_BIAS_PREPARE) ||
(c->bias_level == SND_SOC_BIAS_ON);+}
It'd be worth updating this comment to mention what you're doing with the chip power in the standby state - it's unusual for the codec to be held with so little power in standby mode that register writes don't work, which is why this is a driver-specific thing. I guess if DAPM support were implemented then this wouldn't be required any more?
+/* add non dapm controls */ +static int ad1939_add_controls(struct snd_soc_codec *codec) +{
- struct ad1939_private *ad = codec->private_data;
- int err, i;
- for (i = 0; i < ARRAY_SIZE(ad1939_snd_ctls); i++) {
if ((i <= 3) && (i >= ad->mixpairs))continue;
This could really use a comment, at least referencing the documentation in the header file.
+static int ad1939_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)+{
- /* sample rate */
- dac0 &= ~(3<<1); /* 48kHz */
- adc0 &= ~(3<<6); /* 48kHz */
Might be worth making these comments (and the similar ones you have elsewhere) say "Default: 48kHz" or similar. I can see what you're doing here but on first reading through it was a bit confusing. Not really important, though.
- switch (rate) {
- case 32000 ... 48000:
break;- case 64000 ... 96000:
dac0 |= (1<<1);adc0 |= (1<<6);break;- case 128000 ... 192000:
dac0 |= (2<<1);adc0 |= (2<<6);break;
This and some of the other code will force the ADC and DAC to have the same configuration - is that needed by the codec? If not then it'd be better to check which substream it is operating on and only configure the ADC or DAC as appropriate (you could save lots of conditionals by only checking when actually writing the configuration out at the end of the function).
With the code as it currently stands the codec should impose constraints to let user space know that the playback and capture streams have to be in the same mode. There's examples of how to do that in cs4270, ssm2602, wm8903 and possibly other drivers.
One or the other of these should probably be done prior to merge.
- /* codec clocks master/slave setup */
- dac1 &= ~((1<<4) | (1<<5)); /* LRCK BCK slave */
- adc2 &= ~((1<<3) | (1<<6)); /* LRCK BCK slave */
- switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
- case SND_SOC_DAIFMT_CBM_CFM: /* BCK/LCK master */
dac1 |= (1<<4) | (1<<5); /* LRCK BCK master */adc2 |= (1<<3) | (1<<6); /* LRCK BCK master */if (ad->drvflags & AD1939_DRV_ADCDAC_COMMON_BCK) {if (ad->drvflags & AD1939_DRV_ADC_BCK_MASTER)dac1 &= ~(1<<5); /* DAC BCLK slave */elseadc2 &= ~(1<<6); /* ADC BCLK slave */}
In general if the codec has clocking which can be configured as flexibly as this one appears to then it's normally better to allow the machine driver to configure the clocking manually via the clock configuration APIs.
This avoids having to have code in the codec driver to support unusual machine configurations such as those where the audio clocks are also used for other devices and lets machine drivers do things like change clock sources dynamically at run time. For example, a system may only enable the PLL for some clock rates and use a crystal source for others, saving power. It also frees you from having to worry about imposing constraints arising from things like shared LRCLKs in the codec driver, saving quite a bit of complexity.
This is probably OK for merge as-is but it's very different to how most codec drivers work and is going to limit what people can do with the driver.
+#define AD1939_RATES \
- (SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 | \
SNDRV_PCM_RATE_192000)
Your code appeared to support rather more rates than this?
- /* limit output attenuation controls to requested number */
- ad->mixpairs = setup->mixpairs;
- if ((ad->mixpairs < 1) || (ad->mixpairs > 4))
ad->mixpairs = 4;
It's probably worth complaining if someone tries to set ad->mixpairs to something more than 4. It may be better to just not do this and leave the controls exposed - why is this being done?
- /* Bitclock sources for the ADC and DAC I2S interfaces */
- r0 = ad1939_read(codec, AD1939_DACCTL1);
- r1 = ad1939_read(codec, AD1939_ADCCTL2);
- r0 &= ~AD1939_BCLKSRC_DAC_PLL;
- r1 &= ~AD1939_BCLKSRC_ADC_PLL;
- r0 |= setup->dac_adc_clksrc & AD1939_BCLKSRC_DAC_PLL;
- r1 |= setup->dac_adc_clksrc & AD1939_BCLKSRC_ADC_PLL;
- ad1939_write(codec, AD1939_DACCTL1, r0);
- ad1939_write(codec, AD1939_ADCCTL2, r1);
This definitely makes it look like the chip is flexible enough to warrant allowing machine drivers finer grained control.
- /* register pcms */
- ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1,
SNDRV_DEFAULT_STR1);- if (unlikely(ret < 0)) {
Optimist :)
- printk(KERN_ERR CODEC_NAME ": i2c codec init failed\n");
Since you have an I2C device here this could be dev_err().
+#define ad1939_i2c_suspend NULL +#define ad1939_i2c_resume NULL
+#define ad1939_spi_suspend NULL +#define ad1939_spi_resume NULL
It'd be better to just remove these - they can be added when someone implements them and in the mean time it'll avoid anyone seeing the references and assuming suspend and resume are supported.
+/* Master PLL clock source, select one */ +#define AD1939_PLL_SRC_MCLK 0 /* external clock */ +#define AD1939_PLL_SRC_DACLRCK 1 /* get from DAC LRCLK */ +#define AD1939_PLL_SRC_ADCLRCK 2 /* get from ADC LRCLK */
+/* clock sources for ADC, DAC. Refer to the manual for more information
- (for 192000kHz modes, internal PLL _MUST_ be used). Select one for ADC
- and DAC.
+/* MCLK_XO pin configuration */ +#define AD1939_MCLKXO_MCLKXI 0 /* mirror MCLK_XI pin */ +#define AD1939_MCLKXO_256FS 1 +#define AD1939_MCLKXO_512FS 2 +#define AD1939_MCLKXO_OFF 3 /* disable MCLK_XO output */
These are things that I'd definitely expect to see configured via clock configuration calls.