Hi Mark, ***For the new DAI format According to I2S spec, it doesn't definite a I2S with TDM as a standard I2S. http://www.nxp.com/acrobat_download/various/I2SBUS.pdf
It looks like you are admitting this kind of timing into I2S DAI too: http://i3.6.cn/cvbnm/8f/3d/08/268a4560e0daa1b41d69b82419da06e1.jpg I think I can follow it too.
Due to my test boards, at present, the AD1938 is working in and supporting TDM timing like the diagram: http://i3.6.cn/cvbnm/2f/e2/f2/03ae2b51c4e90749972e70bf887f926f.jpg It looks like DSP mode with TDM, so can I path related codes into SND_SOC_DAIFMT_DSP switch?
***For volume controls based on stereo pairs Even though DAC1-DAC8 are named as DACL1,DACR1, DACL2,DACR2..., but the DACLx and DACRx are not always in a pair, in fact, they are independent. As a codec supporting 8 channels, it can be configed into 2, 2.1, 4.1, 5.1, 6.1, 7.1, how to handle the pairs?
Thanks Barry
2009/6/19 Mark Brown broonie@opensource.wolfsonmicro.com
On Fri, Jun 19, 2009 at 05:28:15PM +0800, Barry Song wrote:
- add AD1938 codec driver (codec)
- add blackfin SPORT-TDM DAI and PCM driver (platform)
- add bf5xx board with AD1938 driver (machine)
As Liam said you really need to submit this as a patch series rather than as a single big patch - as your commit log here indicates you've got several different things going on here.
+++ b/include/sound/soc-dai.h @@ -30,6 +30,7 @@ struct snd_pcm_substream; #define SND_SOC_DAIFMT_DSP_A 3 /* L data msb after FRM LRC */ #define SND_SOC_DAIFMT_DSP_B 4 /* L data msb during FRM LRC */ #define SND_SOC_DAIFMT_AC97 5 /* AC97 */ +#define SND_SOC_DAIFMT_SPORT_TDM 6 /* SPORT TDM for ADI parts */
If you're going to add a new DAI format that really needs more explanation than this explaining what the DAI format is. It'd be very surprising to see hardware needing a new format.
Looking at the datasheet for the ad1938 it appears that the actual format here is just normal I2S with TDM. This does not need a new DAI format or new CPU DAI, you just need to add suport for TDM to the Blackfin I2S driver. The format is fairly standard and implemented by a number of other devices.
See set_tdm_slot() for setting up the higher channel counts - there's some ongoing revisions to that API so you'll want to also ensure that the code is set up so that it can cope with specification of the sample width for each slot in set_tdm_slot().
Given this I've only looked at the CODEC driver below.
diff --git a/sound/soc/codecs/ad1938.c b/sound/soc/codecs/ad1938.c new file mode 100644 index 0000000..9aa78e1 --- /dev/null +++ b/sound/soc/codecs/ad1938.c
- Revision history
- 4 June 2009 Initial version.
Don't include this, git provides code history for us.
+struct snd_soc_device *ad1938_socdev;
+/* dac de-emphasis enum control */ +static const char *ad1938_deemp[] = {"flat", "48kHz", "44.1kHz",
"32kHz"};
For consistency with other drivers "flat" should be "None".
+/* AD1938 volume/mute/de-emphasis etc. controls */ +static const struct snd_kcontrol_new ad1938_snd_controls[] = {
/* DAC volume control */SOC_SINGLE("DAC L1 Volume", AD1938_DAC_L1_VOL, 0, 0xFF, 1),SOC_SINGLE("DAC R1 Volume", AD1938_DAC_R1_VOL, 0, 0xFF, 1),These (and the other stereo pairs below) should be SOC_DOUBLE_R(). This allows ALSA to represent them as stereo controls to applications rather than as two separate controls. You should also provide TLV information so actually SOC_DOUBLE_R_TLV() if possible.
/* DAC mute control */SOC_SINGLE("DAC L1 Switch", AD1938_DAC_CHNL_MUTE, 0, 1, 1),SOC_SINGLE("DAC R1 Switch", AD1938_DAC_CHNL_MUTE, 1, 1, 1),These should be stereo controls too - SOC_DOUBLE() since they're in the same register.
/* ADC mute control */SOC_SINGLE("ADC L1 Switch", AD1938_ADC_CTRL0, ADC0_MUTE, 1, 1),SOC_SINGLE("ADC R1 Switch", AD1938_ADC_CTRL0, ADC1_MUTE, 1, 1),These too.
/* DAC de-emphasis */SOC_ENUM("Playback Deemphasis", ad1938_enum[0]),Don't put your enums in an array, use named variables for them. This makes drivers easier to maintian when you get a lot of enums.
+static int ad1938_add_controls(struct snd_soc_codec *codec) +{
int err, i;for (i = 0; i < ARRAY_SIZE(ad1938_snd_controls); i++) {err = snd_ctl_add(codec->card,snd_soc_cnew(&ad1938_snd_controls[i],codec, NULL));
Use snd_soc_add_controls() here - you can replace the entire function with a call to that.
+/* dac/adc/pll poweron/off functions */ +static int ad1938_dac_powerctrl(struct snd_soc_codec *codec, int cmd) +{
int reg;reg = codec->read(codec, AD1938_DAC_CTRL0);if (cmd)reg &= ~DAC_POWERDOWN;elsereg |= DAC_POWERDOWN;codec->write(codec, AD1938_DAC_CTRL0, reg);This should be handled by DAPM - either have a single DAC widget representing all the channels (since you don't appear to have independant control anyway) or have a bunch of dummy DAC widgets and a supply widget representing the actual power control. The same thing applies to the ADCs.
+static int ad1938_set_pll(struct snd_soc_dai *codec_dai,
int pll_id, unsigned int freq_in, unsigned int freq_out)+{
struct snd_soc_codec *codec = codec_dai->codec;if (freq_out)ad1938_pll_powerctrl(codec, 1);else {/* playing while recording, framework will poweroff-poweronpll redundantly */
if ((!codec_dai->capture.active) &&(!codec_dai->playback.active))
ad1938_pll_powerctrl(codec, 0);}Hrm. This appears to completely ignore the frequencies supplied for the PLL and just provide power control. I suspect that you can just handle the PLL as a SND_SOC_DAPM_SUPPLY(), there seems to be no need to expose the set_pll() operation and make machine drivers call it given that there isn't any frequency configuration going on.
+static int ad1938_mute(struct snd_soc_dai *dai, int mute) +{
struct snd_soc_codec *codec = dai->codec;if (!mute)codec->write(codec, AD1938_DAC_CHNL_MUTE, 0);elsecodec->write(codec, AD1938_DAC_CHNL_MUTE, 0xff);return 0;+}
This isn't going to play well with the explicit mute controls you've got above - it's writing to the same register bits without any coordination. One or the other set of controls ought to be removed.
+static int ad1938_tdm_set(struct snd_soc_codec *codec) +{
codec->write(codec, AD1938_DAC_CTRL0, (codec->read(codec,AD1938_DAC_CTRL0) &
(~DAC_SERFMT_MASK)) | DAC_SERFMT_TDM);codec->write(codec, AD1938_DAC_CTRL1, 0x84); /* invert bclk,256bclk/frame, latch in mid */
codec->write(codec, AD1938_ADC_CTRL1, 0x43); /* sata delay=1, adcaux mode */
codec->write(codec, AD1938_ADC_CTRL2, 0x6F); /* left high, driveron rising edge */
return 0;+}
If you use set_tdm_slot() then the BCLK/frame ratio will be set by that.
Inversion of BCLK (and any other clocks) should be handled by the set_dai_fmt() operation based on the machine driver request rather than done unconditionally.
/* bit size */switch (params_format(params)) {case SNDRV_PCM_FORMAT_S16_LE:word_len = 3;break;Once you implement set_tdm_slot() you should allow the word length to be configured there if it's called or otherwise keep this code here - see Daniel Ribeiro's patche "change set_tdm_slot api to allow slot_width override" posted to the ALSA list this week.
+static int __devinit ad1938_spi_probe(struct spi_device *spi) +{
spi->dev.power.power_state = PMSG_ON;ad1938_socdev->card->codec->control_data = spi;return 0;+}
+static int __devexit ad1938_spi_remove(struct spi_device *spi) +{
return 0;+}
Your device probing should all be restructured so that the SPI device for the CODEC is registered as any other SPI device rather than being set up as part of probing the ASoC device. See the wm8731 driver for an example of doing this for a SPI device.
This will require that the arch code for any systems with the ad1938 do the setup of the device.
.name = "AD1938",.playback = {.stream_name = "Playback",.channels_min = 2,.channels_max = 8,.rates = SNDRV_PCM_RATE_48000,.formats = SNDRV_PCM_FMTBIT_S32_LE |SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S24_LE, },
Please keep your lines to under 80 columns.
+#define AD1938_PLL_CLK_CTRL0 0 +#define PLL_POWERDOWN 0x01 +#define AD1938_PLL_CLK_CTRL1 1 +#define AD1938_DAC_CTRL0 2 +#define DAC_POWERDOWN 0x01 +#define DAC_SERFMT_MASK 0xC0 +#define DAC_SERFMT_STEREO (0 << 6) +#define DAC_SERFMT_TDM (1 << 6)
These defines need namespacing if they're going to appear in the headers
- everything should have the AD1938_ prefix.