On Thu, Sep 4, 2008 at 5:04 AM, Mark Brown <broonie at sirena.org.uk> wrote:
> On Wed, Sep 03, 2008 at 10:01:58PM -0700, sakoman at gmail.com 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