Re: [alsa-devel] [PATCH] ASoC: Add support for multi register mux
-----Original Message----- From: Songhee Baek Sent: Friday, March 28, 2014 11:10 AM To: 'Lars-Peter Clausen' Cc: Arun Shamanna Lakshmi; 'lgirdwood@gmail.com'; 'broonie@kernel.org'; 'swarren@wwwdotorg.org'; 'alsa-devel@alsa-project.org'; 'tiwai@suse.de'; 'linux-kernel@vger.kernel.org' Subject: RE: [alsa-devel] [PATCH] ASoC: Add support for multi register mux
On 03/26/2014 11:41 PM, Songhee Baek wrote:
On 03/26/2014 01:02 AM, Arun Shamanna Lakshmi wrote:
The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000, 0x00000000, 0x00000001 0x00000000, 0x00000000, 0x00000002 0x00000000, 0x00000000, 0x00000003 0x00000000, 0x00000000, 0x00000004 0x00000000, 0x00000000, 0x00000008 ...
That seems to be extremely tedious. If the MUX uses a one hot encoding how about storing the index of the bit in the values array and use (1 << value) when writing the value to the register?
If we store the index of the bit, the value will be duplicated for each
registers inputs since register has 0 to 31bits to shift, then we need to decode the index to interpret value for which registers to set. If we need to interpret the decoded value of index, it is better to have custom put/get function in our driver, isn't it?
I'm not sure I understand. If you use (val / 32) to pick the register and (val % 32) to pick the bit in the register this should work just fine. Maybe I'm missing something. Do you have a real world code example of of the this type of enum is used?
I can use val/32 and val%32 for this multi register mux.
With this logic, put function would be easy however get function becomes tedious due to looping on each bit per register for 3 of them in our case. Rather, it would be easy to identify a unique MUX_OFFSET to distinguish between the 3 registers as shown in the code below.
These get/put functions are updated from previous mail, now it works for multi register mux, please review these function whether I can add in current put/get function.
#define MULTI_MUX_INPUT_OFFSET(n) (5 * n)
int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_soc_codec *codec = snd_soc_dapm_kcontrol_codec(kcontrol); struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; unsigned int reg_val, val, reg_idx;
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]; }
ucontrol->value.enumerated.item[0] = snd_soc_enum_val_to_item(e, val); if (e->shift_l != e->shift_r) { val = (reg_val >> e->shift_r) & e->mask[0]; val = snd_soc_enum_val_to_item(e, val); ucontrol->value.enumerated.item[1] = val; }
return 0; }
int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_soc_codec *codec = snd_soc_dapm_kcontrol_codec(kcontrol); struct snd_soc_card *card = codec->card; struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; unsigned int *item = ucontrol->value.enumerated.item; unsigned int change, i, value, val, update_idx = 0; unsigned int mask; struct snd_soc_dapm_update update; int ret = 0, reg_val;
if (item[0] >= e->items) return -EINVAL;
val = snd_soc_enum_item_to_val(e, item[0]) << e->shift_l; mask = e->mask[0] << e->shift_l; if (e->shift_l != e->shift_r) { if (item[1] > e->items) return -EINVAL; val |= snd_soc_enum_item_to_val(e, item[1]) << e->shift_l; mask |= e->mask[0] << e->shift_r; }
if (e->num_regs < 2) { value = val; goto update_reg; }
for (i = 0; i < e->num_regs; i++) { reg_val = val - MULTI_MUX_INPUT_OFFSET(i);
/* checking reg_val is power of 2 : one-hot code */ /* if reg_val after subtract MULTI_MUX_INPUT_OFFSET is not power of 2, reg[i] should be zero */ if (reg_val & (reg_val - 1)) { /* clear the current input register */ snd_soc_write(codec, e->reg[i], 0); } else { /* reg_val is power of 2, store updated info */ value = reg_val; mask = e->mask[i]; update_idx = i; } }
update_reg: mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
if (e->reg[update_idx] != SND_SOC_NOPM) change = snd_soc_test_bits(codec, e->reg[update_idx], mask, value); else change = dapm_kcontrol_set_value(kcontrol, value);
if (change) { if (e->reg[update_idx] != SND_SOC_NOPM) { update.kcontrol = kcontrol; update.reg = e->reg[update_idx]; update.mask= mask; update.val = value; card->update = &update; } ret = soc_dapm_mux_update_power(card, kcontrol, item[0], e); card->update = NULL; } mutex_unlock(&card->dapm_mutex);
if (ret > 0) soc_dpcm_runtime_update(card);
return change; }
- int reg;
- int reg[SOC_ENUM_MAX_REGS]; unsigned char shift_l; unsigned char shift_r; unsigned int items;
- unsigned int mask;
- unsigned int mask[SOC_ENUM_MAX_REGS];
If you make mask and reg pointers instead of arrays this should be much more flexible and not be limited to 3 registers.
We will make reg* and mask* instead of arrays and since we use the same structure, the plan is to share the get and put function code.
Thanks. Songhee.
On 03/29/2014 03:30 AM, Songhee Baek wrote:
-----Original Message----- From: Songhee Baek Sent: Friday, March 28, 2014 11:10 AM To: 'Lars-Peter Clausen' Cc: Arun Shamanna Lakshmi; 'lgirdwood@gmail.com'; 'broonie@kernel.org'; 'swarren@wwwdotorg.org'; 'alsa-devel@alsa-project.org'; 'tiwai@suse.de'; 'linux-kernel@vger.kernel.org' Subject: RE: [alsa-devel] [PATCH] ASoC: Add support for multi register mux
On 03/26/2014 11:41 PM, Songhee Baek wrote:
On 03/26/2014 01:02 AM, Arun Shamanna Lakshmi wrote:
The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000, 0x00000000, 0x00000001 0x00000000, 0x00000000, 0x00000002 0x00000000, 0x00000000, 0x00000003 0x00000000, 0x00000000, 0x00000004 0x00000000, 0x00000000, 0x00000008 ...
That seems to be extremely tedious. If the MUX uses a one hot encoding how about storing the index of the bit in the values array and use (1 << value) when writing the value to the register?
If we store the index of the bit, the value will be duplicated for each
registers inputs since register has 0 to 31bits to shift, then we need to decode the index to interpret value for which registers to set. If we need to interpret the decoded value of index, it is better to have custom put/get function in our driver, isn't it?
I'm not sure I understand. If you use (val / 32) to pick the register and (val % 32) to pick the bit in the register this should work just fine. Maybe I'm missing something. Do you have a real world code example of of the this type of enum is used?
I can use val/32 and val%32 for this multi register mux.
With this logic, put function would be easy however get function becomes tedious due to looping on each bit per register for 3 of them in our case. Rather, it would be easy to identify a unique MUX_OFFSET to distinguish between the 3 registers as shown in the code below.
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; } }
- Lars
-----Original Message----- From: Lars-Peter Clausen [mailto:lars@metafoo.de] Sent: Saturday, March 29, 2014 3:53 AM To: Songhee Baek Cc: Arun Shamanna Lakshmi; 'lgirdwood@gmail.com'; 'broonie@kernel.org'; 'swarren@wwwdotorg.org'; 'alsa-devel@alsa-project.org'; 'tiwai@suse.de'; 'linux-kernel@vger.kernel.org' Subject: Re: [alsa-devel] [PATCH] ASoC: Add support for multi register mux
On 03/29/2014 03:30 AM, Songhee Baek wrote:
-----Original Message----- From: Songhee Baek Sent: Friday, March 28, 2014 11:10 AM To: 'Lars-Peter Clausen' Cc: Arun Shamanna Lakshmi; 'lgirdwood@gmail.com'; 'broonie@kernel.org'; 'swarren@wwwdotorg.org'; 'alsa-devel@alsa-project.org'; 'tiwai@suse.de'; 'linux-
kernel@vger.kernel.org'
Subject: RE: [alsa-devel] [PATCH] ASoC: Add support for multi register mux
On 03/26/2014 11:41 PM, Songhee Baek wrote:
> On 03/26/2014 01:02 AM, Arun Shamanna Lakshmi wrote: > > The way you describe this it seems to me that a value array for > this kind of mux would look like. > > 0x00000000, 0x00000000, 0x00000001 0x00000000, 0x00000000, > 0x00000002 0x00000000, 0x00000000, 0x00000003 0x00000000, > 0x00000000, 0x00000004 0x00000000, 0x00000000, 0x00000008 ... > > That seems to be extremely tedious. If the MUX uses a one hot > encoding how about storing the index of the bit in the values > array and use (1 << value) when writing the value to the register?
If we store the index of the bit, the value will be duplicated for each
registers inputs since register has 0 to 31bits to shift, then we need to decode the index to interpret value for which registers to set. If we need to interpret the decoded value of index, it is better to have custom put/get function in our driver, isn't it?
I'm not sure I understand. If you use (val / 32) to pick the register and (val % 32) to pick the bit in the register this should work just fine. Maybe I'm missing something. Do you have a real world code example of of the this type of enum is used?
I can use val/32 and val%32 for this multi register mux.
With this logic, put function would be easy however get function becomes tedious due to looping on each bit per register for 3 of them in our
case. Rather, it would be easy to identify a unique MUX_OFFSET to distinguish between the 3 registers as shown in the code below.
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.
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.
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]; }
Both the options are the same. The first one adds 32 or 64 to the bit position to identify uniquely for each register and the other adds a MUX_OFFSET to reg_val to achieve the same. The only advantage of the latter being that we can share the code with get_enum_double and put_enum_double which has been our goal since the beginning of this discussion (as we share the soc_enum struct)
We have tested the second option to work well for our use case. Please let us know what works for you and we will make the necessary changes quickly.
- Arun
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.
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
On Mon, Mar 31, 2014 at 01:55:52PM +0200, Lars-Peter Clausen wrote:
On 03/31/2014 01:21 PM, Mark Brown wrote:
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.
Ugh, right. The fact that I couldn't tell that this was what the code was trying to do from looking at it is not a good sign here.
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Monday, March 31, 2014 5:08 AM To: Lars-Peter Clausen Cc: Arun Shamanna Lakshmi; Songhee Baek; 'alsa-devel@alsa-project.org'; 'swarren@wwwdotorg.org'; 'tiwai@suse.de'; 'lgirdwood@gmail.com'; 'linux- kernel@vger.kernel.org' Subject: Re: [alsa-devel] [PATCH] ASoC: Add support for multi register mux
- PGP Signed by an unknown key
On Mon, Mar 31, 2014 at 01:55:52PM +0200, Lars-Peter Clausen wrote:
On 03/31/2014 01:21 PM, Mark Brown wrote:
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.
Ugh, right. The fact that I couldn't tell that this was what the code was trying to do from looking at it is not a good sign here.
I will work on this and submit another patch. Thanks for all your feedback.
- Unknown Key
- 0x7EA229BD
participants (4)
-
Arun Shamanna Lakshmi -
Lars-Peter Clausen -
Mark Brown -
Songhee Baek