Re: [alsa-devel] [PATCH] ASoC: Add new TI TLV320AIC3204 CODEC driver
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@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
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.
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.
On Fri, Jun 18, 2010 at 04:53:23PM +0100, Mark Brown wrote:
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.
Yeah... for now I've re-sent a new patch... it's on its way... with the register cache in its present form. It's huge and unsightly, but won't otherwise cause a problem.
I'd sooner get the driver out there in its known working form, then come up with a solution to fix the problem in a scalable fashion, then quickly try to hack something up here (I'm at home presently) where I have no test hardware to check I haven't broken something... and where the implementation and design will likely be an appauling mess. ;-)
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.
Indeed... I don't check if a register has been modified; I just copy it across anyway. I'm aware some devices do not implement reads, that was a comment made about the TLV320AIC3X device driver that I based my driver on... in this situation you'd have no choice but to do it the way it's presently done now.
I'll think about this over the next few days... but I'm thinking the basic structure of such a register cache would possibly take the form of something like this:
struct soc_reg_cache_pg { /* Page index; for single-page registers, this would be zero */ int index; /* Number of registers cached */ int size; /* Register offset cached */ int offset; /* Pointer to register flags */ const int *flags_data[]; /* Pointer to default values */ const int *default_data[]; /* Pointer to cached values */ int *cached_data[]; /* Pointer to next block of page registers (for sparse maps) */ struct soc_reg_cache_pg *next_block; };
... then you'd use several of these in an array... one for each block of registers in a page. If your device uses sparse registers, you can use the next_block pointer to point to the next such struct that has the next bank of registers, to save you scanning through *every* struct.
The register cache could be an array of pointers, size equal to the total number of pages, if a page is not used; it is set to NULL. If a page is sparse, the other blocks would be accessed using the next_block pointer.
The flags_data array would be optional; and could be used to indicate the read/write status, volatile bits, and other information. It'd be a bitfield.
Thoughts?
Regards,
On Sat, Jun 19, 2010 at 08:43:14AM +1000, Stuart Longland wrote:
I'll think about this over the next few days... but I'm thinking the basic structure of such a register cache would possibly take the form of something like this:
struct soc_reg_cache_pg { /* Page index; for single-page registers, this would be zero */ int index; /* Number of registers cached */ int size; /* Register offset cached */ int offset; /* Pointer to register flags */ const int *flags_data[]; /* Pointer to default values */ const int *default_data[];
This should probably be either unsigned or void (the latter allowing for pluggable register sizes - otherwise you might burn as much data as you save from the sparseness on chips that have small registers).
... then you'd use several of these in an array... one for each block of registers in a page. If your device uses sparse registers, you can use the next_block pointer to point to the next such struct that has the next bank of registers, to save you scanning through *every* struct.
This is going to be painful to write out when writing the data table - if it's useful to do it'd be better to do it at runtime, but I think rather than handling pages it would be at least as easy to munge the page number into the register number like everyone does at the minute and then just optimise plain register lookups in a sparse map. That way way sparse chips of all kinds get the benefit.
The register cache could be an array of pointers, size equal to the total number of pages, if a page is not used; it is set to NULL. If a page is sparse, the other blocks would be accessed using the next_block pointer.
Like I say, I think optimising for page based access is not the most general approach - it'd be better to optimise sparse register maps in general and then map page based register maps onto this.
participants (3)
-
Liam Girdwood -
Mark Brown -
Stuart Longland