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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
