[alsa-devel] [PATCH 2/4] SOUND: SOC: CODECS: Add support for the TWL4030 audio codec

[Steve Sakoman]

On Tue, Sep 2, 2008 at 7:53 AM, Mark Brown
<broonie at opensource.wolfsonmicro.com> wrote:
> On Tue, Sep 02, 2008 at 07:25:48AM -0700, Steve Sakoman wrote:
>> On Tue, Sep 2, 2008 at 4:26 AM, Mark Brown <broonie at sirena.org.uk> wrote:
>
>> > This should also be added to SND_SOC_ALL_CODECS to ensure testing
>> > coverage.
>
>> Hmm . . . I get no hits grep-ing for SND_SOC_ALL_CODECS.  Could this
>> be something we are missing in the linux-omap git?
>
> Yes, it's queued in ALSA for merge in the next window:

OK, I'll leave this out for now and maerge when it appears in linux-omap

>> >> +struct twl4030_priv {
>> >> +     unsigned int dummy;
>> >> +};
>
>> > If this is empty it should be removable?
>
>> I plan to support further codec features in future patches and have a
>> strong suspicion that private data may be required.  Is it preferred
>> to add the infrastructure now, or wait till it is required in the
>> future?  I opted for the former, but don't really care either way.
>
> It'd be a bit cleaner to remove it but it's not a big deal either way -
> leave it in if you find it useful.

I'll remove it for now and add it back when required.

>> >> +             twl4030_write(codec, REG_ARXL2PGA, 0x00);
>> >> +             twl4030_write(codec, REG_ARXR2PGA, 0x00);
>> >> +             twl4030_write_reg_cache(codec, REG_ARXL2PGA, ldac_reg);
>> >> +             twl4030_write_reg_cache(codec, REG_ARXR2PGA, rdac_reg);
>> >> +     } else {
>> >> +             twl4030_write(codec, REG_ARXL2PGA, ldac_reg);
>> >> +             twl4030_write(codec, REG_ARXR2PGA, rdac_reg);
>> >> +     }
>
>> > It may be better to make these user visible controls - usually the mute
>> > provided here is specifically for the DAC but these look like controls
>> > for the PGA.  The intention here is to avoid artifacts from the DAC when
>> > the input clock stops and start - if there aren't any then user space
>> > may well appreciate the control.  Either way is fine.
>
>> I'm not hearing any clicks & pops, so I will leave this in.
>
> The point is that adding it in suppresses problems that might otherwise
> occur - if it doesn't misbehave without this users may find it useful if
> the mute control to be visible to them.

I'll investigate to see if removing this results in clicks & pops.
Will then do "the right thing"

>> >> +     twl4030_init_chip();
>> >> +     twl4030_power_up(codec, APLL_RATE_44100 | OPT_MODE);
>
>> > It looks like the driver is assuming a particular clock rate going into
>> > the codec - does the chip require a fixed clock or is the driver only
>> > supporting one configuration?  Either is fine.
>
>> There is a fixed clock.
>
> Fixed per board or fixed by the chip?

The chip allows 19.2 Mhz, 26 Mhz, or 38.4 Mhz input clocks to the
codec.  All existing boards (Overo, Beagle, and OMAP3 EVM) use 26 Mhz
(generated by a fixed external oscillator chip).  I'll check with the
Pandora folks, but I believe that they are also 26 Mhz.

Steve