[alsa-devel] [Pull request] Support for wm9705 codec and two machines that use it.

[Mark Brown]

On Fri, Jan 16, 2009 at 01:39:45PM +0000, Ian Molton wrote:

> It looks to me like most chips could be handled with little difficulty -
> a reg mask for some chips that have eg. 9 bit registers represented in a
> u16, a reg_step to allow easy handling of different reg sizes, and an
> array of register numbers for which the core should not cache values.

> Easily 10 chips presently supported could use that scheme, whilst
> leaving the function pointer based system available for the few really
> weird chips.

Oh, there's a lot of stuff that could be factored out but the whole
register access area needs a good looking at.  It's just not the highest
priority cleanup - that's getting the card registration API converted
over to V2 so we can have multiple cards in one system.

> What does seem to be clear though is that there seems to be some
> confusion as to what the 'reg' parameter to codec->read should be -
> should it be a register _address_, or a register _number_.

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.

> 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.  Most codecs have no gaps in the register
space and therefore just use a straight array with a 1:1 mapping between
registers and cache slots.  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.  

> >Please post an incremental patch with your other changes.

> Attached below for review.

Doesn't seem to include the kmemdup() fix for WM9705 - if you could post
the WM9705 changes only together with your signoff I can push the queue
I'm sitting on to Takashi?

> I haven't added my SoB: as I havent tested it.

> As you can see, the cache handling is done in several ways by different 
> codecs, with inconsistent semantics and return codes (some BUG, some 

Yes.  Broadly, writing outside of the register space the driver wants to
support is a clear bug, but the driver can choose to cache some or none
of the registers.  This makes the specifics of the error handling less
important for the system as a whole.

> return 1 some -1 some -EIO. Some wont allow undefined reg access, some 
> do, but only uncached.) This seems like ripe territory for bugs (albeit 

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.  Having the cache there may not be safe
depending on the behaviour of the undocumented registers.

> fairly harmless ones, but bugs nontheless).

Yes, though it's less bad than it looks since each codec driver only has
to agree with itself.

This is why I'm saying the whole area needs going over fairly carefully
- there's useful cleanups that can be done but it needs a degree of care
due to the way the code is now in order to ensure that existing
functionality doesn't get broken.  Fortunately much of the care should
scale well over doing cleanups of the whole area since everything is
private to the individual codec drivers.

> 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.