Re: [PATCH] New ASoC Drivers for ADI AD1938 codec

[ååå]

2009/6/19 Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>:
> On Fri, Jun 19, 2009 at 05:28:15PM +0800, Barry Song wrote:
>> 1. add AD1938 codec driver          (codec)
>> 2. add blackfin SPORT-TDM DAI and PCM driver (platform)
>> 3. 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;
>> + Â Â else
>> + Â Â Â Â Â Â reg |= 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.

I want to use ADC/DAC widgets.
static const struct snd_soc_dapm_widget ad1938_dapm_widgets[] = {
        SND_SOC_DAPM_DAC("DAC", "HiFi Playback", AD1938_DAC_CTRL0, 0, 1),
        SND_SOC_DAPM_ADC("ADC", "HiFi Capture", AD1938_ADC_CTRL0, 0, 1),
};
But for this AD1938 codec, DAC's work depends on ADC is powered on in
hardware.  I think there is no any mechanism to handle this kind of
strange depending now.  So is there a generic way to handle this?


>
>> +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-poweron pll 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);
>> + Â Â else
>> + Â Â Â Â Â Â codec->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, adc aux mode */
>> + Â Â codec->write(codec, AD1938_ADC_CTRL2, 0x6F); /* left high, driver on 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.
>



-- 
ååå 21cnbao@xxxxxxxx
http://21cnbao.blog.51cto.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/