On Thu, Sep 4, 2008 at 5:04 AM, Mark Brown <broonie@xxxxxxxxxxxxx> wrote: > On Wed, Sep 03, 2008 at 10:01:58PM -0700, sakoman@xxxxxxxxx wrote: >> + /* initiate offset cancellation */ >> + twl4030_write(codec, REG_ANAMICL, >> + twl4030_reg[REG_ANAMICL] | CNCL_OFFSET_START); > > It looks a bit odd to see this reading the register defaults rather than > the register cache. True. I will fix this. > >> + /* wait for offset cancellation to complete */ >> + twl4030_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, &byte, REG_ANAMICL); >> + while ((byte & CNCL_OFFSET_START) == CNCL_OFFSET_START) >> + twl4030_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, >> + &byte, REG_ANAMICL); > > How long is this likely to take? If it might take a while then putting > a delay in between reads might be nice. As Jarkko said, a timeout would > be a good idea too in case something went wrong. It happens quite quickly. I'll add an exit to the loop and measure how many passes it takes. If it is a large number I'll add a delay. >> + /* anti-pop when changing analog gain */ >> + twl4030_write(codec, REG_MISC_SET_1, >> + twl4030_reg[REG_MISC_SET_1] | SMOOTH_ANAVOL_EN); > > Another one reading the register defaults rather than register cache... I'll fix this. >> + /* disable bias out */ >> + popn &= ~VMID_EN; >> + twl4030_write(codec, REG_HS_POPN_SET, popn); > > ... > >> + case SND_SOC_BIAS_ON: >> + twl4030_power_up(codec); >> + break; >> + case SND_SOC_BIAS_PREPARE: >> + break; >> + case SND_SOC_BIAS_STANDBY: >> + twl4030_power_down(codec); >> + break; > > Normally SND_SOC_BIAS_STANDBY would leave Vmid enabled but your power > down function turns it off. The normal thing here is to have standby > bring the codec up and then apply relatively small tweaks in _ON and > _PREPARE that do things like improve the quality of the reference > voltages. Equally well, since the driver does not yet implement DAPM > support doing this will give you power savings that you wouldn't > otherwise get. > > Does the codec have any bypass paths that can be set up? If not it > shouldn't do much harm either way until you implement DAPM. How about I add a "TODO" on this? I'm swamped for the next few weeks and won't be able to give it the attention it deserves test-wise. >> +/* AUDIO_IF (0x0E) Fields */ >> + >> +#define AIF_SLAVE_EN 0x80 >> +#define DATA_WIDTH 0x60 > > These should really all get namespaced - some of them look relatively > likely to collide with registers for CPU side stuff. Good point. Steve _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
