Re: [alsa-devel] [PATCH] ASoC: Add support for multi register mux

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]

On Sat, Mar 29, 2014 at 11:12:30PM -0700, Arun Shamanna Lakshmi wrote:

Fix your mailer to word wrap within paragraphs, your mails are
excessively hard to read.

> > I'm not sure I understand how that MUX_OFFSET would work. To get the
> > selected mux output you can use the ffs instruction.

> > foreach(reg) {
> > 	reg_val = read(reg) & mask;
> > 	if (reg_val != 0) {
> > 		val = __ffs(reg_val);
> > 		break;
> > 	}
> > }

> There are 2 options to do this. The first option is what you specified
> above, in which case I think we cannot share get and put functions as
> they use the reg_val directly inside snd_soc_enum_val_to_item API (not
> the bit position being set). If we change to bit position like above,
> then the current users of these APIs should also change their soc_enum
> value table. And, the second option being the one that we proposed.

Sharing the functions isn't the goal, coming up with a usable API is.

> That being said, MUX_OFFSET which is the second option works in the
> following way. We know that reg_val is a power of 2 (2^0  to 2^31)
> which is one hot code. This method adds a unique offset for this
> reg_val for each incremental register that we want to set (say 2^n +
> MUX_OFFSET(reg_id)) inside get function and does the reverse of it in
> put function. For current users of only one register, it doesn't
> change anything as we use reg_val.

I'm afraid I can't understand the above at all, sorry.  The code below
is quoted like Lars wrote it but I think it's actually written by you,
please check your quoting when replying:

> > 	if (e->reg[0] != SND_SOC_NOPM) {
> > 		for (reg_idx = 0; reg_idx < e->num_regs; reg_idx++) {
> > 	    		reg_val = snd_soc_read(codec, e->reg[reg_idx]);
> > 			val = (reg_val >> e->shift_l) & e->mask[reg_idx];
> > 			if (val) {
> > 				val += MULTI_MUX_INPUT_OFFSET(reg_idx);
> > 				break;
> > 			}
> > 		}
> > 	} else {
> > 		reg_val = dapm_kcontrol_get_value(kcontrol);
> > 		val = (reg_val >> e->shift_l) & e->mask[0];
> > 	}

The above is a bit confusing...  partly this is because of a lack of
context (what is MULTI_MUX_INPUT_OFFSET?) and partly because it isn't
entirely obvious that stopping as soon as we see any value set is the
right choice, especially given the addition to rather than setting of
val.

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