Mark Brown wrote:
On Fri, Jan 16, 2009 at 01:39:45PM +0000, Ian Molton wrote:
Oh, there's a lot of stuff that could be factored out but the whole register access area needs a good looking at.
I dont mind consolidating the cache handling code if its worth the effort (IOW its not scheduled to get totally re-written)
It should be the register address as you'd see it in the datasheet unless there's a *very* good reason for it to be something else. It's entirely private to the codec driver what the register parameter means, the rest of the code only works with values that were given to it by the codec driver.
some of the core code for dumping the codecs reg_cache looks quite broken... at least its only debug code, but broken debug code is worse than useless...
If the former, then the caching in my wm9705 and a few other drivers is broken. These treat reg as a register _address_ and shift it right by 1 to get the reg number and thus index into the reg cache.
Neither scheme is broken.
I guess not as its codec specific (wit the exception noted above)
The AC97 parts are special because AC97 only has even numbered registers so they're saving a bit of memory by only having cache space for registers that actually exist and cutting down on noise by only showing those registers in the debug output. It's not a big saving but it's there.
Actually they arent. the regs are 16 bit anyway for the most part so there is no saving - Just confusion over the meaning of the 'reg' parameter.
Basically we've got two codec types. BOTH have 16 bit regs, but on some the 'reg' parameter is a register address, and on some its the register number.
This is fine I supppose if the semantics for the underlying bus are the same, eg. the PXA AC97 bus takes reg as an address.
I havent checked to see if any AC97 codecs are passing reg as a register number - if they are then thats a clear bug as its not what the AC97 bus driver expects.
Please post an incremental patch with your other changes.
Attached below for review.
Doesn't seem to include the kmemdup() fix for WM9705
Whoops. Fixed.
- if you could post
the WM9705 changes only together with your signoff I can push the queue I'm sitting on to Takashi?
Sure, but I really think the other changes noted below need to go in ASAP as AFAICT they are clearly bogus.
Undefined register access is often a deliberate decision in order to allow access to functionality of the chip outside the fully documented sections of the register space.
Yes, I understand that.
Having the cache there may not be safe depending on the behaviour of the undocumented registers.
Of course, but drivers calling ac97_write need to get clear errors back, IMHO.
Yes, though it's less bad than it looks since each codec driver only has to agree with itself.
True.
This patch takes fixes a number of bugs in the caching code used by several ASoC codec drivers. Mostly off-by-one fixes.
This looks reasonable, though I didn't go through it completely carefully due to the lack of signoff.
I think it needs to get in ASAP. I'll send it along with the wm9705 stuff again, with my SOB (I've read through it again), but as a seperate patchset.
Patches to follow shortly.
-Ian