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,