Re: [PATCH v5] ASoC: add RT286 CODEC driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2410 bytes --]

On Tue, Mar 11, 2014 at 03:11:36PM +0800, bardliao@realtek.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> This patch adds the ALC286 codec driver.
> 
> ALC286 is a dual mode codec, which can run as HD-A or I2S mode.
> It is controlled by HD-A verb commands via I2C protocol.

Some or all of this documentation needs to end up in the code, we need
to be able to understand and maintain the code going forwards and
while the commit message is going to be kept it's still useful if the
code can be followed.  Right now that's extremely hard.

> The following is the I/O difference between ALC286 and general I2S codecs.
> 1. A HD-A verb command contains three parts, NID, VID, and PID.
>    And an I2S command contains only two parts: address and data.

It'd probably help to explain waht these are.

> 2. Not only the register address is written, but the read command also
>    includes the entire write command.
> 3. rt286 uses different registers for read and write the same bits.
> As a result, standard regmap is difficult to be used on ALC286.
> We don't request a standard I/O by snd_soc_codec_set_cache_io anymore.
> Now we have ,reg_write and .reg_read functions for ALC286's I/O.
> And we don't use cache due to item 3 above.
> Some dummy registers (address <= 0xff) are defined for dapm routing.
> Thhe dummy registers are cache only.

Don't use dummy registers, DAPM already has virtual controls of various
kinds - if you need more let's extend them.  Storing data in virtual
registers just makes things confusing and fragile.  Some older CODEC
drivers did it and they're harder to work with now than they should be.

> Due to item 2 above, HD-A verb commands are put into the address part of regmap.
> When we issue HD-A verb write commands, the data part of regmap is zero.

I'm having a really hard time understanding how this follows.  We do
have some other devices that do things like have multi-level register
addresses with registers collected in pages.  By analogy here what I'd
expect to see is something which combines the various pieces of
addressing information into a bitfields within a single number that can
be used as a register address.

Or perhaps this just isn't anything like a register map at all?
Whatever is going on something that ignores the value part of the regmap
interfaces doesn't seem like it is a very good fit.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]