Prev: [PATCH] Documentation/sysctl/vm.txt typo
Next: Wacom based devices and the mt kernel protocol.
From: Liam Girdwood on 18 Jun 2010 07:10
On Fri, 2010-06-18 at 13:57 +1000, Stuart Longland wrote:
> The TLV320AIC3204 is a low-power stereo audio CODEC capable of sample
> rates of up to 192kHz. This driver implements basic functionality,
> using I²C for the control channel.
>
> The audio interface supports many data bus formats, including I²S
> master/slave, DSP, left/right justified and TDM.
>
> What works:
> - Playback at various bitrates up to 96kHz
> - Recording at various bitrates up to 96kHz
> - Mixer interface
> - PLL generation of CODEC clocks from MCLK
>
> What could work better:
> - DAPM
>
> What isn't tested:
> - Audio interfaces other than I²S
> - PLL with clocks other than ~12MHz
> - Mono recording/playback
> - 192kHz recording/playback
>
> What isn't implemented:
> - SPI interface support
> - PLL without fractional divider (would allow <10MHz clocks)
> - Clock sources other than MCLK
> - Adaptive filtering
> - AGC
> - Headset detection, JACK framework
>
> Signed-off-by: Stuart Longland <redhatter(a)gentoo.org>

Just had a quick check and the register caching needs addressed.

I agree with your comments, I don't think we really want to cache all
16K of codec registers here as we will probably only ever use a handful
of them. I think a smaller lookup table containing only the registers
that we care about will do.

Fwiw, a generic ASoC lookup table would be best as this could be used by
other codecs with large register maps too. The table should be ordered
(for quick lookup) and also contain a readable/writeable/volatile flag
for each register.

Thanks

Liam
--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Stuart Longland on 18 Jun 2010 07:40
Hi,

To others: In case you're wondering where the patch is... I pulled it
from the moderation queue because there were a few style errors, and
some definitions I forgot to delete. A fresh one that meets coding
style requirements is on its way, as soon as scripts/checkpatch.pl is
finished (this seems to take a while on my hardware).

On Fri, Jun 18, 2010 at 12:01:02PM +0100, Liam Girdwood wrote:
> On Fri, 2010-06-18 at 13:57 +1000, Stuart Longland wrote:
> > The TLV320AIC3204 is a low-power stereo audio CODEC capable of sample
> > rates of up to 192kHz. This driver implements basic functionality,
> > using I??C for the control channel.
> >
> > The audio interface supports many data bus formats, including I??S
> > master/slave, DSP, left/right justified and TDM.
>
> Just had a quick check and the register caching needs addressed.
>
> I agree with your comments, I don't think we really want to cache all
> 16K of codec registers here as we will probably only ever use a handful
> of them. I think a smaller lookup table containing only the registers
> that we care about will do.

Yeah, I wasn't sure how to go about it. It's inefficient, but it's
simple, and works. Other options I've considered;

(1) Mark's suggestion of using per-page arrays
(2) Using address translation. that is:
- Page 0 registers 0-127 stored in cache array elements 0-127
- Page 1 registers 0-127 stored in cache array elements 128-255
- Page 2..7 are skipped
- Page 8 registers 0-127 stored in cache array elements 256-383
... etc.

> Fwiw, a generic ASoC lookup table would be best as this could be used by
> other codecs with large register maps too. The table should be ordered
> (for quick lookup) and also contain a readable/writeable/volatile flag
> for each register.

Indeed, the 'AIC3204 is not the only CODEC to use sparse register maps.
16KB is not a lot of RAM these days, but it's a lot more RAM than I'm
comfortable using for an audio driver. I'll give this some thought, but
for now I'll press on with what I have since I know it works reliably.

Another thought regarding register cache, rather than hard-coding it the
way we presently do, we could also build up the cache on demand... that
is, we maintain a table of previously read registers; if a register
value is read for the first time, an actual read is done from the CODEC
(or the value is copied from a static table). All subsequent reads then
come from cache. On suspend; if a register has not been touched, it is
deemed to be left at default settings, and skipped on resume.

The downside of this is having to maintain a table of what registers
have been read already; which would possibly be as big as the cache is
anyway. But the flip side; if a company brought out a new CODEC which
had differing power-up defaults to a similar CODEC handled by the same
driver, it would prevent the wrong default getting assumed or loaded in.
--
Stuart Longland (aka Redhatter, VK4MSL) .'''.
Gentoo Linux/MIPS Cobalt and Docs Developer '.'` :
.. . . . . . . . . . . . . . . . . . . . . . .'.'
http://dev.gentoo.org/~redhatter :.'

I haven't lost my mind...
...it's backed up on a tape somewhere.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 18 Jun 2010 12:00
On Fri, Jun 18, 2010 at 09:33:35PM +1000, Stuart Longland wrote:
> On Fri, Jun 18, 2010 at 12:01:02PM +0100, Liam Girdwood wrote:
> > On Fri, 2010-06-18 at 13:57 +1000, Stuart Longland wrote:

> > > The audio interface supports many data bus formats, including I??S
> > > master/slave, DSP, left/right justified and TDM.

> > Just had a quick check and the register caching needs addressed.

> > I agree with your comments, I don't think we really want to cache all
> > 16K of codec registers here as we will probably only ever use a handful
> > of them. I think a smaller lookup table containing only the registers
> > that we care about will do.

> Yeah, I wasn't sure how to go about it. It's inefficient, but it's
> simple, and works. Other options I've considered;

> (1) Mark's suggestion of using per-page arrays
> (2) Using address translation. that is:
> - Page 0 registers 0-127 stored in cache array elements 0-127
> - Page 1 registers 0-127 stored in cache array elements 128-255
> - Page 2..7 are skipped
> - Page 8 registers 0-127 stored in cache array elements 256-383
> ... etc.

Option 2 is what others have used here. I do think that anything we do
here really ought to have some sort of page mapping support whatever the
actual implementation it uses - TI in particular have a bunch of chips
which do this so there's a general call for something that can handle
them.

If you are going to do a lookup table one way of doing this would be to
have the lookup table be a table of range mappings (I've not looked at
the patch, perhaps that's what you've done already) - just so long as
it's got the concept of pages covered somehow.

> Another thought regarding register cache, rather than hard-coding it the
> way we presently do, we could also build up the cache on demand... that
> is, we maintain a table of previously read registers; if a register
> value is read for the first time, an actual read is done from the CODEC
> (or the value is copied from a static table). All subsequent reads then
> come from cache. On suspend; if a register has not been touched, it is
> deemed to be left at default settings, and skipped on resume.

The only problem with this is that for a number of reasons a lot of
devices have no read functionality at all. These need us to supply the
defaults from outside the device, though other devices could use what
you suggest. I'd not be so bothered about the performance here - this
has to be high performance in comparison with doing I/O on a slow bus
such as I2C. If we do run into performance issues we can always work on
substituting in a higher performance algorithm later, if everything is
well encapsulated this should be doable without much disruption.

It's also useful to have the actual defaults available for allowing
writes to be skipped when powering up the device (eg, on resume) - these
could be cached on first write so it's not insurmountable but it does
need to be considered.

> The downside of this is having to maintain a table of what registers
> have been read already; which would possibly be as big as the cache is
> anyway. But the flip side; if a company brought out a new CODEC which
> had differing power-up defaults to a similar CODEC handled by the same
> driver, it would prevent the wrong default getting assumed or loaded in.

I'm not sure that's a big risk to be honest - the reuse you see tends to
be much more complete than this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 18 Jun 2010 21:20
On Sat, Jun 19, 2010 at 08:24:36AM +1000, Stuart Longland wrote:

This all looks pretty good, the comments below are fairly minor.

> + /* AVDD <->DVDD Link enable */
> + u8 avdd_link_en;

What's this? It looks like a boolean which makes u8 an odd choice.

> + /* LDO Enable */
> + u8 ldo_en;

Similarly here.

> +
> + /* Oversampling rate (both ADC and DAC) */
> + u8 osr;

Why is this platform data rather than a runtime configurable option?

> +config SND_SOC_TLV320AIC3204
> + tristate
> + depends on I2C
> +

Drop the depends, it doesn't actually do anything for selected items.

> + /* Page 1 */
> + if (page == 1) {
> + if (reg <= 4)
> + return 1;

I can't help but think that this'd be more legible with switch ()
statements (GCC has an extension for ranges in switch statements which
you could use).

> +/*
> + * Pretty-print the value of a register
> + */
> +static int aic3204_show_reg(struct snd_soc_codec *codec, char *buf,
> + int buf_sz, unsigned int reg)
> +{
> + return snprintf(buf, buf_sz, "%02x",
> + aic3204_read_reg_cache(codec, reg));
> +}

This looks suspiciously close to the standard show_reg() - it seems
wrong that you should need it.

> + /*
> + * These registers are not manipulated by us, so we must read them from
> + * the CODEC directly.
> + */

Hrm? Also, it strikes me that this code is also used in the WCLK
function and might benefit from being factored out - it's moderately
verbose.

> +static const char *aic3204_micbias_voltages[] = {
> + "Low", "Med", "High", "V+"
> +};

I'd be inclined to spell medium out and write "V+" as "Supply" or
similar unless there's a strong reason to do otherwise - it seems more
legible.

> +static const char *aic3204_ldac_srcs[] = {
> + "disabled", "left channel", "right channel", "mix"
> +};

Capital letters are more idiomatic for enumerations.

> +/*
> + * DAC digital volumes. From -63.5 to +24 dB in 0.5 dB steps.
> + * Does not mute control.
> + */

I'm finding these comments a little too verbose but that's just me -
I stop and read them only to find there's nothing odd going on here.

> + /*
> + * Note regarding SOC_DOUBLE_R_SX_TLV...
> + *
> + * This is a bit misleading; xshift refers to the bit one bit *past*
> + * the most significant bit. Or at least that's what it appears to be
> + * doing the math. Mask needs to select the last 6 bits; which is the
> + * signed bitfield specifiying the gain in dB.
> + */

I suspect that fixing the control may break what you're doing here? It
would certainly bitrot the comment.

> + SOC_SINGLE("Differential Output Switch", AIC3204_DACS2,
> + AIC3204_DACS2_RMOD_INV_SHIFT, 1, 0),

This feels like platform data - the use of differential outputs is
normally a property of the physical wiring of the board, it's very rare
to be able to vary this usefully at runtime.

> + /* MICBIAS Options */
> + SOC_SINGLE("MICBIAS Enable", AIC3204_MICBIAS, 6, 0x1, 0),

MICBIAS Enable should definitely be DAPM.

> + SOC_ENUM("MICBIAS Level", aic3204_enum_micbias_voltage),
> + SOC_ENUM("MICBIAS Source", aic3204_enum_micbias_src),

These I would expect to be managed in kernel - they're normally either a
property of the board or need to be managed in conjunction with jack
detection code which tends to live in kernel.

> +static const struct snd_soc_dapm_widget aic3204_dapm_widgets[] = {
> + /* Driver/Amplifier Selection */
> + SND_SOC_DAPM_SWITCH("HPL Playback Switch", AIC3204_OUTDRV,
> + AIC3204_OUTDRV_HPL_UP_SHIFT, 0,
> + &aic3204_hpl_switch),
> + SND_SOC_DAPM_SWITCH("HPR Playback Switch", AIC3204_OUTDRV,
> + AIC3204_OUTDRV_HPR_UP_SHIFT, 0,
> + &aic3204_hpr_switch),

A lot of these SWITCH controls feel like they ought to be PGAs and the
switch controls mutes on those. When muting an output you normally
don't want to power up and down the path since that is slow and tends to
trigger audible issues, you'd normally control the power for path
endpoints and elements that affect routing only.

> + SND_SOC_DAPM_AIF_IN("Left Data In", "Left Playback",
> + 0, SND_SOC_NOPM, 0, 0),
> + SND_SOC_DAPM_AIF_IN("Right Data In", "Right Playback",
> + 1, SND_SOC_NOPM, 0, 0),
> + SND_SOC_DAPM_AIF_OUT("Left Data Out", "Left Capture",
> + 0, SND_SOC_NOPM, 0, 0),
> + SND_SOC_DAPM_AIF_OUT("Right Data Out", "Right Capture",
> + 1, SND_SOC_NOPM, 0, 0),

You have these AIF widgets but at least the DAC input selection was
being done in the regular controls rather than in the DAPM routing.

> + /*
> + * Set BCLK and WCLK sources...
> + *
> + * Despite DAPM; this is still needed, as DAPM doesn't yet set the
> + * source at the right time.
> + *
> + * TODO: See if we can change the order of initialisation so this
> + * selection is made after bringing up the ADCs/DACs. Alternative:
> + * see if we can find out ahead of time which one to use.

It surprises me that the ordering matters too much here; worst case you
leave the interface declocked a little longer when you need to switch
sources but that doesn't seem like the end of the world?

> + snd_soc_update_bits(codec, AIC3204_AISR3, AIC3204_AISR3_BDIV,
> + (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + ? AIC3204_AISR3_BDIV_DAC
> + : AIC3204_AISR3_BDIV_ADC);

I have to say that I'm really not a fan of the ternery operator for
legibility.

> +static int aic3204_mute(struct snd_soc_dai *dai, int mute)
> +{

....

> + aic3204_write(codec, AIC3204_DACS2, dacs2); /* Unmute DAC */

....or possibly mute it :)

> +static int aic3204_resume(struct platform_device *pdev)
> +{
> + struct snd_soc_device *socdev = platform_get_drvdata(pdev);
> + struct snd_soc_codec *codec = socdev->card->codec;
> + int i;
> + u8 data[2];
> + u8 *cache = codec->reg_cache;
> +
> + /* Sync reg_cache with the hardware */
> + for (i = 0; i < AIC3204_CACHEREGNUM; i++) {
> + data[0] = i;
> + data[1] = cache[i];
> + codec->hw_write(codec->control_data, data, 2);

Since you've got a register defaults table you could skip rewriting
registers which have their default value.

> + /* LDO enable, analogue blocks enable */
> + snd_soc_update_bits(codec, AIC3204_LDO,
> + AIC3204_LDO_ANALOG | AIC3204_LDO_AVDD_UP,
> + AIC3204_LDO_ANALOG_ENABLED |
> + (aic3204->pdata.ldo_en
> + ? AIC3204_LDO_AVDD_UP : 0));

This looks a bit fishy since the mask covers more bits than can ever be
enabled - it reads like the other two bits should be unconditionally
cleared.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Stuart Longland on 21 Jun 2010 19:50
On Sat, Jun 19, 2010 at 02:12:21AM +0100, Mark Brown wrote:
> On Sat, Jun 19, 2010 at 08:24:36AM +1000, Stuart Longland wrote:
> > + /* Page 1 */
> > + if (page == 1) {
> > + if (reg <= 4)
> > + return 1;
>
> I can't help but think that this'd be more legible with switch ()
> statements (GCC has an extension for ranges in switch statements which
> you could use).

I gave this some further thought... I'm not certain that a switch
statement is going to be much clearer. There are two ways I can tackle
this.

One is to go on a page-by-page basis, which is how I do it using the if
statements. Here; I define my ranges so that I start from the very
end... anything beyond page 70 is invalid ... voila, I eliminate those
early on. A number of pages have a similar register pattern, and so I
make use of nested if statements to explain this. The if block for
pages following always use the block before to define the upper,
non-inclusive bound.

The register tests start from register 0. I could perhaps reverse the
outer ifs to start at page 0 and work forwards too... but I instead work
backwards from page 70.

I exit the function as early as possible to skip unneeded checks, as
soon as I know a range is valid or not, I return 1 or 0. Perhaps the 1
or 0 could be made clearer (a couple of #defines maybe?) but to me, it
looks fairly clear.

I could use switch statements to replace some or all of the if
statements. There'd be a small benefit I suppose in making the outer if
statements a switch, but little anywhere else from what I can see.

The other way is that I ignore pages completely; and use the
AIC3204_PGREG macro to define ranges of absolute register addresses.
This may have a small benefit in speed since these are compiled in... as
opposed to runtime masking/shifting, but I don't see that being much
clearer either. I'd still come to a case statement, then return.

This is a function largely intended for debugging, in fact, I'm thinking
I should probably wrap it in #ifdef CONFIG_DEBUG_FS, since the function
isn't called unless debugfs is enabled. So I'm not certain that
performance is worth chasing here given the intended purpose -- it's not
something that's called all the time, nor something that will be used in
a production environment.

That's my thoughts on the issue, perhaps naïve, but I'm not sure
there's any real gain in refactoring this.
--
Stuart Longland (aka Redhatter, VK4MSL) .'''.
Gentoo Linux/MIPS Cobalt and Docs Developer '.'` :
.. . . . . . . . . . . . . . . . . . . . . . .'.'
http://dev.gentoo.org/~redhatter :.'

I haven't lost my mind...
...it's backed up on a tape somewhere.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH] Documentation/sysctl/vm.txt typo
Next: Wacom based devices and the mt kernel protocol.