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

[Mark Brown]

On Fri, Jan 16, 2009 at 03:00:35PM +0000, Ian Molton wrote:
> 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's not likely to be rewritten dramatically, like I say it's more about
the effort taken to do it - I know there's a bunch of people who really
want multiple cards.

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

Could you be more specific, please?  The read interface is in rather
active use.  The only thing I'm aware of is that only things marked as
cached are visible.  The read interface certainly gets a lot of use, the
write interface less so but still.

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

There is.  The natural layout would be to just have an array of u16s
which gives us an array of 16 bit register slots like this:

  [0][1][2][3][4][5]..

but we're actually storing:

  [0][2][4]...

since we know that the odd numbered registers don't exist.

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

The registers referred to by ASoC are *always* register numbers as you'd
see in the chip documentation; the storage method used for any register
cache is entirely up to the codec drivers and is not something that the
core concerns itself with.  The core is also unaware of the actual width
of the registers.

You're confusing yourself here by thinking of these things memory mapped
devices - they aren't.  How software chooses to lay out any view it has
of the registers in memory has no meaning in hardware since there is, in
general, no way to interact with the hardware with a resolution less
than a full register.

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

For marshalling purposes all the I2C and SPI codecs are their own "bus
driver" - the only external thing that cares is the hardware itself, the
I2C and SPI bus drivers just deal in byte streams.  AC97 is the only
actual bus in use here and it has the same register number interface
that ASoC uses.

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

All the AC97 codecs are using reg as a register number because that is
what both ASoC and the AC97 bus drivers expect.  Note that the AC97 bus
drivers have no idea that the codecs have caches, they just work with
register/value pairs.

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

They're not *that* urgent given how long they've been there - it's a
nice cleanup not something I'd push to Linus unless they fixed issues on
live systems (in which case I'd really expect to see patches to fix the
callers as well).  The risk/reward doesn't seem worth it.

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

Hardware I/O is a different matter - the error codes there are more
interesting since that could realistically go wrong at runtime.
Accessing invalid registers is a clear coding error, but it's possible
that something is relying on the fact that errors aren't reported
(probably by completely ignoring the return values) so it needs a bit of
care to actually change anything.