RE: [alsa-devel] [PATCH] Add driver for Analog Devices ADAU1701SigmaDSP

[Cai, Cliff]

>-----Original Message-----
>From: Mark Brown [mailto:broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx]
>Sent: 2011å3æ9æ 18:12
>To: Cliff Cai
>Cc: Cai, Cliff; device-drivers-devel@xxxxxxxxxxxxxxxxxxxx;
>akpm@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>alsa-devel@xxxxxxxxxxxxxxxx; lrg@xxxxxxxxxxxxxxx
>Subject: Re: [alsa-devel] [PATCH] Add driver for Analog
>Devices ADAU1701 SigmaDSP
>
>On Wed, Mar 09, 2011 at 03:04:03PM +0800, Cliff Cai wrote:
>> On Mon, Mar 7, 2011 at 7:41 PM, Mark Brown
>> > On Mon, Mar 07, 2011 at 09:11:42AM +0800,
>cliff.cai@xxxxxxxxxx wrote:
>
>> > It loooks like the register length is hard coded in every location
>> > that the register is referenced. ?This doesn't seem ideal
>- it'd be
>> > much nicer to have the register I/O functions work this
>out without
>> > the callers needing to.
>
>> I'm afraid it's not easy to do so.
>
>What's the issue?  I'd expect something like a lookup table or
>switch statement going from register to register length - I'd
>be surprised if it were any more complicated than getting all
>the callers right.

All right,just looks nicer.

>> >> +static int adau1701_pcm_prepare(struct snd_pcm_substream
>> >> +*substream,
>> >> +                             struct snd_soc_dai *dai) {
>> >> +     struct snd_soc_codec *codec = dai->codec;
>> >> +     int reg = 0;
>
>> >> +     reg = SEROCTL_MASTER | SEROCTL_OBF16 | SEROCTL_OLF1024;
>> >> +     adau1701_write_register(codec, ADAU1701_SEROCTL, 2, reg);
>
>> >> +     return 0;
>> >> +}
>
>> > This looks like some of it is DAI format and word length
>configuration?
>
>> no,it makes the processor work in master mode.and output bit
>clock and
>> frame clock.
>
>This controls the CODEC, not the CPU?  Master/slave should be
>controlled by set_dai_fmt() rather than being hard coded.

Actually,if the DAI works in master mode,it will always output clocks.
it's not we want,so we have to make it in slave mode when shutdown,and
Make it in Master mode before starting to play.

>> >> +static void adau1701_shutdown(struct snd_pcm_substream
>*substream,
>> >> +                           struct snd_soc_dai *dai) {
>> >> +     struct snd_soc_codec *codec = dai->codec;
>
>> >> +     adau1701_write_register(codec, ADAU1701_SEROCTL, 2, 0); }
>
>> > I suspect this isn't going to work for simultaneous playback and
>> > capture
>> > - it's not clear what the code does but I'd guess it will stop
>> > things completely.
>
>> this SigmaDSP doesn't support duplex operation,it can choose either
>> ADCs or serial port as input source.
>
>The driver should be enforcing this constraint, then.

It's the firmware that is in charge of this work.

>> >> +static int adau1701_set_bias_level(struct snd_soc_codec *codec,
>> >> +                              enum snd_soc_bias_level level) {
>> >> +     u16 reg;
>> >> +     switch (level) {
>> >> +     case SND_SOC_BIAS_ON:
>> >> +             reg = adau1701_read_register(codec,
>ADAU1701_AUXNPOW,
>> >> +2);
>> >> +             reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD |
>AUXNPOW_D1PD |
>> >> +AUXNPOW_D2PD |
>> >> +                      AUXNPOW_D3PD | AUXNPOW_VBPD |
>AUXNPOW_VRPD);
>> >> +             adau1701_write_register(codec, ADAU1701_AUXNPOW, 2,
>> >> +reg);
>
>> > You were also updating some of the same register bits in the mute
>> > function.  This looks buggy.
>
>> the processor has no switchs to mute or unmute
>ADCS/DACs,only thing we
>> can do is turning them off or on.
>
>If mute can't be implemented don't implement it.  Right now
>the code is clearly not going to do what's expected as you've
>got two different bits of code trying to control the same
>thing - at least one set of register updates isn't going to do
>anything?

All right.

>> >> +     ret = adau1701_setprogram(codec);
>> >> +     if (ret < 0) {
>> >> +             printk(KERN_ERR "Loading program data failed\n");
>> >> +             goto error;
>> >> +     }
>> >> +     reg = DSPCTRL_DAM | DSPCTRL_ADM;
>> >> +     adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
>> >> +     reg = 0x08;
>> >> +     adau1701_write_register(codec, ADAU1701_DSPRES, 1, reg);
>
>> > Should these DSP configuations things be part of
>downloading firmware?
>
>> these configurations above loading firmware mainly used to avoid
>> pops/clicks and cleanup some registers in the DSP core.
>
>Right, but is this something that's always useful when loading
>firmware (in which case the firmware load should just wrap it
>up so it always happens)?

Sure,thanks.

>> >> +     adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, 0);
>> >> +     reg = AUXADCE_AAEN;
>> >> +     adau1701_write_register(codec, ADAU1701_AUXADCE, 2, reg);
>> >> +     reg = DACSET_DACEN;
>> >> +     adau1701_write_register(codec, ADAU1701_DACSET, 2, reg);
>> >> +     reg = DSPCTRL_DAM | DSPCTRL_ADM | DSPCTRL_CR;
>> >> +     adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
>> >> +     /* Power-up the oscillator */
>> >> +     adau1701_write_register(codec, ADAU1701_OSCIPOW, 2, 0);
>
>> > This looks like it's all power management which I'd expect to see
>> > handled in either the bias management functions or ideally
>DAPM.  It
>> > also appears that the oscillator is an optional feature so
>it should
>> > be used conditionally.
>
>> the processor can receive MCLK either from external clock source or
>> crystal oscillator, currently we use the on board
>crystal,and it can't
>> be turned off,otherwise the whole chip will be in an unpredicted
>> status, only output clocks can be disabled.
>
>That's specific to your board design, though.  Another board
>design might use a different clocking setup.

Yes,so I think I can add a MACRO to indicate it.

Cliff
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—