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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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@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:

    git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

> >> +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.

> >> +             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.

> >> +     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?