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

[Lars-Peter Clausen <lars@metafoo.de>]

On 03/31/2014 01:21 PM, Mark Brown wrote:
> 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.

I think the idea is that since we know that for one-hot encodings only 
powers of two are valid values the other bits are used to encode the 
register number. E.g 0x4 means bit 3 in register 0, 0x5 means bit 3 in 
register 1, 0x6 means bit 3 in register 2 and so on. I guess it is possible 
to make it work. But this seems to be quite hack-ish to me. You'd have to be 
careful that MULTI_MUX_INPUT_OFFSET(reg_idx) never evaluates to a power of 
two and there are probably some more pitfalls.

- Lars