ASoC: Add support for multi register mux
PATCH 1/1 ASoC: Add support for multi register mux
If the mux uses 1 bit position per input, and requires to set one single bit at a time, then an N bit register can support up to N inputs. In more recent Tegra chips, we have at least greater than 64 inputs which requires at least 2 .reg fields in struct soc_enum.
--- include/sound/soc-dapm.h | 20 +++++- include/sound/soc.h | 44 ++++++++++--- sound/soc/soc-core.c | 118 +++++++++++++++++++++++++++++++++-- sound/soc/soc-dapm.c | 155 +++++++++++++++++++++++++++++++++++++++------- 4 files changed, 300 insertions(+), 37 deletions(-)
include/sound/soc-dapm.h +17 -3
@@ -315,6 +315,12 @@ struct device; .private_value = (unsigned long)&xenum }#define SOC_DAPM_VALUE_ENUM (xname, xenum) \ SOC_DAPM_ENUM(xname, xenum) +#define SOC_DAPM_VALUE_ENUM_WIDE (xname, xenum) \ +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \ + .info = snd_soc_info_enum_wide, \ + .get = snd_soc_dapm_get_value_enum_wide, \ + .put = snd_soc_dapm_put_value_enum_wide, \ + .private_value = (unsigned long)&xenum }#define SOC_DAPM_PIN_SWITCH (xname) \ { .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname " Switch", \ .info = snd_soc_dapm_info_pin_switch, \
@@ -352,6 +358,9 @@ struct device;/* regulator widget flags */ #define SND_SOC_DAPM_REGULATOR_BYPASS 0x1/* bypass when disabled */ +/* maximum number of registers to update */ +#define SOC_DAPM_UPDATE_MAX_REGS 3 +struct snd_soc_dapm_widget ;enum snd_soc_dapm_type ;struct snd_soc_dapm_path ;
@@ -378,6 +387,10 @@ int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol);int snd_soc_dapm_put_enum_double (struct snd_kcontrol *kcontrol,struct snd_ctl_elem_value *ucontrol); +int snd_soc_dapm_get_value_enum_wide (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol); +int snd_soc_dapm_put_value_enum_wide (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol);int snd_soc_dapm_info_pin_switch (struct snd_kcontrol *kcontrol,struct snd_ctl_elem_info *uinfo); int snd_soc_dapm_get_pin_switch(struct snd_kcontrol *kcontrol,
@@ -590,9 +603,10 @@ struct snd_soc_dapm_widget {struct snd_soc_dapm_update {struct snd_kcontrol *kcontrol; -int reg; -int mask; -int val; +int reg[SOC_DAPM_UPDATE_MAX_REGS ]; +int mask[SOC_DAPM_UPDATE_MAX_REGS ]; +int val[SOC_DAPM_UPDATE_MAX_REGS ]; +int num_regs; };/* DAPM context */
include/sound/soc.h +37 -7
91 lines not on this page · raw message
sound/soc/soc-core.c +112 -6
143 lines not on this page · raw message
sound/soc/soc-dapm.c +134 -21
215 lines not on this page · raw message
Signature · 1 line
1.7.9.5
Quoted 7 lines · Arun Shamanna Lakshmi
If the mux uses 1 bit position per input, and requires to set one single bit at a time, then an N bit register can support up to N inputs. In more recent Tegra chips, we have at least greater than 64 inputs which requires at least 2 .reg fields in struct
soc_enum.Signed-off-by Arun Shamanna Lakshmi <[email protected]>Signed-off-by Songhee Baek <[email protected]>
The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000, 0x00000000, 0x000000010x00000000, 0x00000000, 0x000000020x00000000, 0x00000000, 0x000000030x00000000, 0x00000000, 0x000000040x00000000, 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?
[...]
Quoted 2 lines
/* enumerated kcontrol */
struct soc_enum {There doesn't actually be any code that is shared between normal enums and wide enums. This patch doubles the size of the soc_enum struct, how about having a separate struct for wide enums?
include/sound/soc.h struct soc_mreg_control { · line 1134
Quoted 7 lines
include/sound/soc.h +2
-2-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.
Quoted 5 lines
const char * const *texts;
const unsigned int *values;include/sound/soc.h +1
-0+unsigned int num_regs; };
Quoted 28 lines
-----Original Message-----
From: Lars-Peter Clausen [mailto:[email protected]]
Sent: Wednesday, March 26, 2014 12:39 PM
To: Arun Shamanna Lakshmi
Cc: [email protected]; [email protected]; [email protected]; Songhee Baek; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [alsa-devel] [PATCH] ASoC: Add support for multi register muxQuoted 7 lines · Arun Shamanna Lakshmi
If the mux uses 1 bit position per input, and requires to set one single bit at a time, then an N bit register can support up to N inputs. In more recent Tegra chips, we have at least greater than 64 inputs which requires at least 2 .reg fields in struct
soc_enum.Signed-off-by Arun Shamanna Lakshmi <[email protected]>Signed-off-by Songhee Baek <[email protected]>The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000,0x00000000,0x000000010x00000000,0x00000000,0x000000020x00000000,0x00000000,0x000000030x00000000,0x00000000,0x000000040x00000000,0x00000000,0x00000008
...
Quoted 3 lines
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?
Quoted 9 lines
[...]
Quoted 2 lines
/* enumerated kcontrol */ struct soc_enum {There doesn't actually be any code that is shared between normal enums and wide enums. This patch doubles the size of the
soc_enumstruct, how about having a separate struct for wide enums?
We are using DAPM widgets for these muxes and DAPM uses soc_enum struct inside APIs, so we couldn't use additional struct for that. If there is the way to use additional struc in dapm, please guide me.
Quoted 11 lines
include/sound/soc.h struct soc_mreg_control { · line 1134
Quoted 7 lines
include/sound/soc.h +2
-2-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.
To use pointers instead of arrays, it will be flexible but I need to update SOC_ENUM SINGLE/DOUBLE macros. It will changes a lot in current soc-core.c and soc-dapm.c.
Quoted 5 lines
Quoted 5 lines
const char * const *texts;
const unsigned int *values;include/sound/soc.h +1
-0+unsigned int num_regs; };
Quoted 28 lines · Songhee Baek
Quoted 28 lines
-----Original Message-----
From: Lars-Peter Clausen [mailto:[email protected]]
Sent: Wednesday, March 26, 2014 12:39 PM
To: Arun Shamanna Lakshmi
Cc: [email protected]; [email protected]; [email protected]; Songhee Baek; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [alsa-devel] [PATCH] ASoC: Add support for multi register muxQuoted 7 lines · Arun Shamanna Lakshmi
If the mux uses 1 bit position per input, and requires to set one single bit at a time, then an N bit register can support up to N inputs. In more recent Tegra chips, we have at least greater than 64 inputs which requires at least 2 .reg fields in struct
soc_enum.Signed-off-by Arun Shamanna Lakshmi <[email protected]>Signed-off-by Songhee Baek <[email protected]>The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000,0x00000000,0x000000010x00000000,0x00000000,0x000000020x00000000,0x00000000,0x000000030x00000000,0x00000000,0x000000040x00000000,0x00000000,0x00000008
...
Quoted 6 lines
Quoted 3 lines
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?
Quoted 12 lines
Quoted 11 lines
include/sound/soc.h struct soc_mreg_control { · line 1134
Quoted 7 lines
include/sound/soc.h +2
-2-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.
Quoted 2 lines
To use pointers instead of arrays, it will be flexible but I need to update
SOC_ENUMSINGLE/DOUBLE macros. It will changes a lot in currentsoc-core.candsoc-dapm.c.
In the existing macros you can do something like this:
...
.reg = &(unsigned int){(xreg)},
...Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
const char * const *texts; const unsigned int *values;include/sound/soc.h +1
-0+unsigned int num_regs; };
Quoted 51 lines
-----Original Message-----
From: Lars-Peter Clausen [mailto:[email protected]]
Sent: Thursday, March 27, 2014 2:20 AM
To: Songhee Baek
Cc: Arun Shamanna Lakshmi; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [alsa-devel] [PATCH] ASoC: Add support for multi register muxQuoted 5 lines · Songhee Baek
Quoted 5 lines
-----Original Message-----
From: Lars-Peter Clausen [mailto:[email protected]]
Sent: Wednesday, March 26, 2014 12:39 PM
To: Arun Shamanna Lakshmi
Cc: [email protected]; [email protected];Quoted 24 lines
Quoted 24 lines
Songhee Baek; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [alsa-devel] [PATCH] ASoC: Add support for multi register muxQuoted 7 lines · Arun Shamanna Lakshmi
If the mux uses 1 bit position per input, and requires to set one single bit at a time, then an N bit register can support up to N inputs. In more recent Tegra chips, we have at least greater than 64 inputs which requires at least 2 .reg fields in struct
soc_enum.Signed-off-by Arun Shamanna Lakshmi <[email protected]>Signed-off-by Songhee Baek <[email protected]>The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000,0x00000000,0x000000010x00000000,0x00000000,0x000000020x00000000,0x00000000,0x000000030x00000000,0x00000000,0x000000040x00000000,0x00000000,0x00000008
...Quoted 5 lines
Quoted 3 lines
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?
Quoted 1 line
Quoted 5 lines
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.
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
include/sound/soc.h struct soc_mreg_control { · line 1134
Quoted 7 lines
include/sound/soc.h +2
-2-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.
Quoted 9 lines
Quoted 1 line
To use pointers instead of arrays, it will be flexible but I need to update
SOC_ENUMSINGLE/DOUBLE macros.Quoted 1 line
It will changes a lot in current
soc-core.candsoc-dapm.c.In the existing macros you can do something like this:
... .reg = &(unsigned int){(xreg)}, ...
Ok, I will update SINGLE/DOUBLE macro with this way.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
const char * const *texts; const unsigned int *values;include/sound/soc.h +1
-0+unsigned int num_regs; };
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 26 lines
Quoted 26 lines · Songhee Baek
Quoted 12 lines
Quoted 12 lines · Arun Shamanna Lakshmi
The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000,0x00000000,0x000000010x00000000,0x00000000,0x000000020x00000000,0x00000000,0x000000030x00000000,0x00000000,0x000000040x00000000,0x00000000,0x00000008
...Quoted 6 lines
Quoted 3 lines
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?
Quoted 1 line
Quoted 8 lines
Quoted 6 lines
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.
#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);
}
} 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, update_idx = 0;
unsigned int mask;
struct snd_soc_dapm_update update;
int ret = 0, reg_val;
if (item[0] >= e->items)
return -EINVAL;
value = 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;
value |= snd_soc_enum_item_to_val(e, item[1]) << e->shift_l;
mask |= e->mask[0] << e->shift_r;
}
if (e->num_regs < 2) {
goto update_reg;
} for (i = 0; i < e->num_regs; i++) {
reg_val = value - 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;
}Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
include/sound/soc.h struct soc_mreg_control { · line 1134
Quoted 7 lines
include/sound/soc.h +2
-2-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.
Quoted 41 lines
-----Original Message-----
From: Songhee Baek
Sent: Friday, March 28, 2014 11:10 AM
To: 'Lars-Peter Clausen'
Cc: Arun Shamanna Lakshmi; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'
Subject: RE: [alsa-devel] [PATCH] ASoC: Add support for multi register muxQuoted 30 lines
Quoted 22 lines · Songhee Baek
Quoted 9 lines
Quoted 9 lines · Arun Shamanna Lakshmi
The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000,0x00000000,0x000000010x00000000,0x00000000,0x000000020x00000000,0x00000000,0x000000030x00000000,0x00000000,0x000000040x00000000,0x00000000,0x00000008...Quoted 6 lines
Quoted 3 lines
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?
Quoted 1 line
Quoted 6 lines
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;
}Quoted 11 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
include/sound/soc.h struct soc_mreg_control { · line 1134
Quoted 7 lines
include/sound/soc.h +2
-2-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.
Quoted 39 lines · Songhee Baek
Quoted 39 lines
-----Original Message-----
From: Songhee Baek
Sent: Friday, March 28, 2014 11:10 AM
To: 'Lars-Peter Clausen'
Cc: Arun Shamanna Lakshmi; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'
Subject: RE: [alsa-devel] [PATCH] ASoC: Add support for multi register muxQuoted 29 lines
Quoted 28 lines · Songhee Baek
Quoted 9 lines
Quoted 9 lines · Arun Shamanna Lakshmi
The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000,0x00000000,0x000000010x00000000,0x00000000,0x000000020x00000000,0x00000000,0x000000030x00000000,0x00000000,0x000000040x00000000,0x00000000,0x00000008...Quoted 6 lines
Quoted 3 lines
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?
Quoted 1 line
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?
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
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_OFFSETto 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
Quoted 62 lines
-----Original Message-----
From: Lars-Peter Clausen [mailto:[email protected]]
Sent: Saturday, March 29, 2014 3:53 AM
To: Songhee Baek
Cc: Arun Shamanna Lakshmi; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ASoC: Add support for multi register muxQuoted 7 lines · Songhee Baek
Quoted 7 lines
-----Original Message-----
From: Songhee Baek
Sent: Friday, March 28, 2014 11:10 AM
To: 'Lars-Peter Clausen'
Cc: Arun Shamanna Lakshmi; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'; 'linux-Quoted 33 lines
Quoted 33 lines
Subject: RE: [alsa-devel] [PATCH] ASoC: Add support for multi register mux
Quoted 29 lines
Quoted 28 lines · Songhee Baek
Quoted 9 lines
Quoted 9 lines · Arun Shamanna Lakshmi
The way you describe this it seems to me that a value array for this kind of mux would look like.
0x00000000,0x00000000,0x000000010x00000000,0x00000000,0x000000020x00000000,0x00000000,0x000000030x00000000,0x00000000,0x000000040x00000000,0x00000000,0x00000008...Quoted 6 lines
Quoted 3 lines
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?
Quoted 1 line
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?Quoted 4 lines
Quoted 2 lines
Quoted 1 line
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_OFFSETto distinguish between the 3 registers as shown in the code below.I'm not sure I understand how that
MUX_OFFSETwould work. To get the selected mux output you can use the ffs instruction.
Quoted 8 lines
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.
Quoted 5 lines
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) {Quoted 8 lines
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
Mark Brown
maintainerOn 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.
Quoted 11 lines
Quoted 10 lines
I'm not sure I understand how that
MUX_OFFSETwould 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; } }
Quoted 6 lines
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_valdirectly insidesnd_soc_enum_val_to_itemAPI (not the bit position being set). If we change to bit position like above, then the current users of these APIs should also change theirsoc_enumvalue 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.
Quoted 7 lines
That being said,
MUX_OFFSETwhich is the second option works in the following way. We know thatreg_valis a power of 2 (2^0 to 2^31) which is one hot code. This method adds a unique offset for thisreg_valfor 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 usereg_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:
Quoted 5 lines
Quoted 5 lines
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) {
Quoted 8 lines
Quoted 8 lines
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.
Quoted 51 lines · Mark Brown
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.
Quoted 11 lines
Quoted 10 lines
I'm not sure I understand how that
MUX_OFFSETwould 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; } }Quoted 6 lines
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_valdirectly insidesnd_soc_enum_val_to_itemAPI (not the bit position being set). If we change to bit position like above, then the current users of these APIs should also change theirsoc_enumvalue 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.
Quoted 7 lines
That being said,
MUX_OFFSETwhich is the second option works in the following way. We know thatreg_valis a power of 2 (2^0 to 2^31) which is one hot code. This method adds a unique offset for thisreg_valfor 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 usereg_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:\
Quoted 5 lines
Quoted 5 lines
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) {Quoted 8 lines
Quoted 8 lines
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]; }
Quoted 5 lines
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
Mark Brown
maintainerQuoted 8 lines · Lars-Peter Clausen
Quoted 5 lines · Mark Brown
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.
Quoted 7 lines
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
0x4means bit 3 in register 0,0x5means bit 3 in register 1,0x6means 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 thatMULTI_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.
Quoted 29 lines
-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Monday, March 31, 2014 5:08 AM
To: Lars-Peter Clausen
Cc: Arun Shamanna Lakshmi; Songhee Baek; '[email protected]'; '[email protected]'; '[email protected]'; '[email protected]'; 'linux- [email protected]'
Subject: Re: [alsa-devel] [PATCH] ASoC: Add support for multi register mux* PGP Signed by an unknown key
Quoted 8 lines · Lars-Peter Clausen
Quoted 5 lines · Mark Brown
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.Quoted 7 lines
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
0x4means bit 3 in register 0,0x5means bit 3 in register 1,0x6means 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 thatMULTI_MUX_INPUT_OFFSET(reg_idx)never evaluates to a power of two and there are probably some more pitfalls.
Quoted 2 lines
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.
Quoted 3 lines
* Unknown Key
*0x7EA229BD
Mark Brown
maintainerQuoted 11 lines · Lars-Peter Clausen
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,0x000000010x00000000,0x00000000,0x000000020x00000000,0x00000000,0x000000030x00000000,0x00000000,0x000000040x00000000,0x00000000,0x00000008
Quoted 3 lines
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?
Or hide it behind utility macros at any rate; I've got this horrible feeling that as soon as we have this people will notice that they have more standard enums that are splatted over multiple registers (I think from memory I've seen them but they got fudged).
Quoted 7 lines
[...]
Quoted 2 lines
/* enumerated kcontrol */
structsoc_enum{There doesn't actually be any code that is shared between normal enums and wide enums. This patch doubles the size of the
soc_enumstruct, how about having a separate struct for wide enums?
Or if they are going to share the same struct then they shouldn't be duplicating the code and instead keying off num_regs (which was my first thought earlier on when I saw the separate functions). We definitely shouldn't be sharing the data without also sharing the code I think.
Quoted 10 lines
include/sound/soc.h struct soc_mreg_control { · line 1134
Quoted 7 lines
include/sound/soc.h +2
-2-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.
Right, which pushes towards not sharing. Though with an arrayified mask the specification by shift syntax would get to be slightly obscure (is it relative to the enums or the registers?) so perhaps we don't want to do that at all if we've got specification by shift. If we do that then we could get away with a variable length array at the end of the struct though I think that may be painful for the static declarations. Someone would need to look to see what works
Quoted 57 lines
-----Original Message-----
From: [email protected] [mailto:alsa-devel-bounces@alsa- project.org] On Behalf Of Mark Brown
Sent: Wednesday, March 26, 2014 6:09 PM
To: Lars-Peter Clausen
Cc: Songhee Baek; Arun Shamanna Lakshmi; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [alsa-devel] [PATCH] ASoC: Add support for multi register mux* PGP Signed by an unknown key
Quoted 11 lines · Lars-Peter Clausen
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,0x000000010x00000000,0x00000000,0x000000020x00000000,0x00000000,0x000000030x00000000,0x00000000,0x000000040x00000000,0x00000000,0x00000008Quoted 3 lines
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?
Or hide it behind utility macros at any rate; I've got this horrible feeling that as soon as we have this people will notice that they have more standard enums that are splatted over multiple registers (I think from memory I've seen them but they got fudged).
Quoted 7 lines
[...]
Quoted 2 lines
/* enumerated kcontrol */
structsoc_enum{There doesn't actually be any code that is shared between normal enums and wide enums. This patch doubles the size of the
soc_enumstruct, how about having a separate struct for wide enums?Or if they are going to share the same struct then they shouldn't be duplicating the code and instead keying off
num_regs(which was my first thought earlier on when I saw the separate functions). We definitely shouldn't be sharing the data without also sharing the code I think.Quoted 10 lines
include/sound/soc.h struct soc_mreg_control { · line 1134
Quoted 7 lines
include/sound/soc.h +2
-2-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.
Quoted 6 lines
Right, which pushes towards not sharing. Though with an arrayified mask the specification by shift syntax would get to be slightly obscure (is it relative to the enums or the registers?) so perhaps we don't want to do that at all if we've got specification by shift. If we do that then we could get away with a variable length array at the end of the struct though I think that may be painful for the static declarations. Someone would need to look to see what works
Making a separate soc_enum_wide is a better way compared to using soc_enum for this use case. If we add a separate soc_enum_wide, we need to update the following functions :
dapm_connect_mux,
soc_dapm_mux_update_power
snd_soc_dapm_mux_update_power
These functions are using texts field in soc_enum struct, I think that we can pass texts pointer instead of soc_enum struct pointer. I want to know whether it is Ok to do this.
Quoted 3 lines
* Unknown Key
*0x7EA229BD
Mark Brown
maintainersound/soc/soc-core.c int snd_soc_put_enum_double(struct snd_kcontrol *kcontrol, · line 2716
Quoted 6 lines · Arun Shamanna Lakshmi
sound/soc/soc-core.c +6
-0+ } +if (!match) { +dev_err (codec->dev,"ASoC: Failed to find matched enum value\n" ); +return -EINVAL ; + }else + ucontrol->value.enumerated.item[0 ] = i;
Coding style nit: if one side of the if has braces both should. Most of this code could also use more blank lines.
sound/soc/soc-core.c int snd_soc_put_enum_double(struct snd_kcontrol *kcontrol, · line 2752
Quoted 7 lines
sound/soc/soc-core.c +7
-0+for (reg_idx =0 ; reg_idx < e->num_regs; reg_idx++) { + val = e->values[item * e->num_regs + reg_idx]; + ret =snd_soc_update_bits_locked (codec, e->reg[reg_idx], + e->mask[reg_idx], val); +if (ret) +return ret; + }
So, this is a bit interesting. It will update one register at a time which means that we are likely to transiently set an invalid value sometimes which might not make the hardware happy or may cause us to write a valid value with undesirable consequences. I'd expect to see some handling of this, some combination of providing a safe value that the hardware could be reset to prior to change and doing a bulk write to all the registers simultaneously if we can (I know sometimes hardware has special handling for atomic updates of multi-register values in a single block transfer).