[alsa-devel] [PATCH] ASoC: dapm: Add support for multi register mux
- Modify soc_enum struct to handle pointers for reg and mask - Add dapm get and put APIs for multi register mux with one hot encoding - Update snd_soc_dapm_update struct to support multiple reg update
Signed-off-by: Arun Shamanna Lakshmi aruns@nvidia.com Signed-off-by: Songhee Baek sbaek@nvidia.com --- include/sound/soc-dapm.h | 17 ++++- include/sound/soc.h | 34 +++++++-- sound/soc/soc-core.c | 12 ++-- sound/soc/soc-dapm.c | 174 +++++++++++++++++++++++++++++++++++++++------- 4 files changed, 197 insertions(+), 40 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index ef78f56..ded46732 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -305,6 +305,12 @@ struct device; .get = snd_soc_dapm_get_enum_double, \ .put = snd_soc_dapm_put_enum_double, \ .private_value = (unsigned long)&xenum } +#define SOC_DAPM_ENUM_ONEHOT(xname, xenum) \ +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \ + .info = snd_soc_info_enum_double, \ + .get = snd_soc_dapm_get_enum_onehot, \ + .put = snd_soc_dapm_put_enum_onehot, \ + .private_value = (unsigned long)&xenum } #define SOC_DAPM_ENUM_VIRT(xname, xenum) \ SOC_DAPM_ENUM(xname, xenum) #define SOC_DAPM_ENUM_EXT(xname, xenum, xget, xput) \ @@ -378,6 +384,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_enum_onehot(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol); +int snd_soc_dapm_put_enum_onehot(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 +600,10 @@ struct snd_soc_dapm_widget {
struct snd_soc_dapm_update { struct snd_kcontrol *kcontrol; - int reg; - int mask; - int val; + int reg[3]; + int mask[3]; + int val[3]; + int num_regs; };
/* DAPM context */ diff --git a/include/sound/soc.h b/include/sound/soc.h index 0b83168..add326a 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -177,18 +177,24 @@ {.reg = xreg, .min = xmin, .max = xmax, \ .platform_max = xmax} } #define SOC_ENUM_DOUBLE(xreg, xshift_l, xshift_r, xitems, xtexts) \ -{ .reg = xreg, .shift_l = xshift_l, .shift_r = xshift_r, \ +{ .reg = &(int){(xreg)}, .shift_l = xshift_l, .shift_r = xshift_r, \ .items = xitems, .texts = xtexts, \ - .mask = xitems ? roundup_pow_of_two(xitems) - 1 : 0} + .mask = &(unsigned int){(xitems ? roundup_pow_of_two(xitems) - 1 : 0)}, \ + .num_regs = 1, .type = SND_SOC_ENUM_NONE } #define SOC_ENUM_SINGLE(xreg, xshift, xitems, xtexts) \ SOC_ENUM_DOUBLE(xreg, xshift, xshift, xitems, xtexts) #define SOC_ENUM_SINGLE_EXT(xitems, xtexts) \ { .items = xitems, .texts = xtexts } #define SOC_VALUE_ENUM_DOUBLE(xreg, xshift_l, xshift_r, xmask, xitems, xtexts, xvalues) \ -{ .reg = xreg, .shift_l = xshift_l, .shift_r = xshift_r, \ - .mask = xmask, .items = xitems, .texts = xtexts, .values = xvalues} +{ .reg = &(int){(xreg)}, .shift_l = xshift_l, .shift_r = xshift_r, \ + .mask = &(unsigned int){(xmask)}, .items = xitems, .texts = xtexts, \ + .values = xvalues, .num_regs = 1, .type = SND_SOC_ENUM_NONE } #define SOC_VALUE_ENUM_SINGLE(xreg, xshift, xmask, xnitmes, xtexts, xvalues) \ SOC_VALUE_ENUM_DOUBLE(xreg, xshift, xshift, xmask, xnitmes, xtexts, xvalues) +#define SOC_VALUE_ENUM_ONEHOT(xregs, xmasks, xnum_regs, xitems, xtexts, xvalues) \ +{ .reg = xregs, .mask = xmasks, .num_regs = xnum_regs, \ + .items = xitems, .texts = xtexts, .values = xvalues, \ + .type = SND_SOC_ENUM_ONEHOT } #define SOC_ENUM_SINGLE_VIRT(xitems, xtexts) \ SOC_ENUM_SINGLE(SND_SOC_NOPM, 0, xitems, xtexts) #define SOC_ENUM(xname, xenum) \ @@ -293,6 +299,9 @@ #define SOC_VALUE_ENUM_DOUBLE_DECL(name, xreg, xshift_l, xshift_r, xmask, xtexts, xvalues) \ const struct soc_enum name = SOC_VALUE_ENUM_DOUBLE(xreg, xshift_l, xshift_r, xmask, \ ARRAY_SIZE(xtexts), xtexts, xvalues) +#define SOC_VALUE_ENUM_ONEHOT_DECL(name, xregs, xmasks, xnum_regs, xtexts, xvalues) \ + const struct soc_enum name = SOC_VALUE_ENUM_ONEHOT(xregs, xmasks, xnum_regs, \ + ARRAY_SIZE(xtexts), xtexts, xvalues) #define SOC_VALUE_ENUM_SINGLE_DECL(name, xreg, xshift, xmask, xtexts, xvalues) \ SOC_VALUE_ENUM_DOUBLE_DECL(name, xreg, xshift, xshift, xmask, xtexts, xvalues) #define SOC_ENUM_SINGLE_VIRT_DECL(name, xtexts) \ @@ -326,6 +335,17 @@ enum snd_soc_bias_level { SND_SOC_BIAS_ON = 3, };
+/* + * Soc Enum Type + * + * @NONE: soc_enum type for SINGLE, DOUBLE or VIRTUAL mux + * @ONEHOT: soc_enum type for one hot encoding mux + */ +enum snd_soc_enum_type { + SND_SOC_ENUM_NONE = 0, + SND_SOC_ENUM_ONEHOT = 1, +}; + struct device_node; struct snd_jack; struct snd_soc_card; @@ -1098,13 +1118,15 @@ struct soc_mreg_control {
/* enumerated kcontrol */ struct soc_enum { - int reg; + int *reg; unsigned char shift_l; unsigned char shift_r; unsigned int items; - unsigned int mask; + unsigned int *mask; const char * const *texts; const unsigned int *values; + enum snd_soc_enum_type type; + unsigned int num_regs; };
/** diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index caebd63..cf29722 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2601,12 +2601,12 @@ int snd_soc_get_enum_double(struct snd_kcontrol *kcontrol, unsigned int val, item; unsigned int reg_val;
- reg_val = snd_soc_read(codec, e->reg); - val = (reg_val >> e->shift_l) & e->mask; + reg_val = snd_soc_read(codec, e->reg[0]); + val = (reg_val >> e->shift_l) & e->mask[0]; item = snd_soc_enum_val_to_item(e, val); ucontrol->value.enumerated.item[0] = item; if (e->shift_l != e->shift_r) { - val = (reg_val >> e->shift_l) & e->mask; + val = (reg_val >> e->shift_l) & e->mask[0]; item = snd_soc_enum_val_to_item(e, val); ucontrol->value.enumerated.item[1] = item; } @@ -2636,15 +2636,15 @@ int snd_soc_put_enum_double(struct snd_kcontrol *kcontrol, if (item[0] >= e->items) return -EINVAL; val = snd_soc_enum_item_to_val(e, item[0]) << e->shift_l; - mask = e->mask << 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_r; - mask |= e->mask << e->shift_r; + mask |= e->mask[0] << e->shift_r; }
- return snd_soc_update_bits_locked(codec, e->reg, mask, val); + return snd_soc_update_bits_locked(codec, e->reg[0], mask, val); } EXPORT_SYMBOL_GPL(snd_soc_put_enum_double);
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index c8a780d..19b004a 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -511,13 +511,26 @@ static int dapm_connect_mux(struct snd_soc_dapm_context *dapm, const struct snd_kcontrol_new *kcontrol) { struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; - unsigned int val, item; + unsigned int val, item, bit_pos = -1; int i;
- if (e->reg != SND_SOC_NOPM) { - soc_widget_read(dest, e->reg, &val); - val = (val >> e->shift_l) & e->mask; - item = snd_soc_enum_val_to_item(e, val); + if (e->reg[0] != SND_SOC_NOPM) { + if (e->type == SND_SOC_ENUM_ONEHOT) { + for (i = 0; i < e->num_regs; i++) { + soc_widget_read(dest, e->reg[i], &val); + val = val & e->mask[i]; + if (val != 0) { + bit_pos = __ffs(val) + + (8 * dest->codec->val_bytes * i); + break; + } + } + item = snd_soc_enum_val_to_item(e, bit_pos); + } else { + soc_widget_read(dest, e->reg[0], &val); + val = (val >> e->shift_l) & e->mask[0]; + item = snd_soc_enum_val_to_item(e, val); + } } else { /* since a virtual mux has no backing registers to * decide which path to connect, it will try to match @@ -1553,8 +1566,8 @@ static void dapm_widget_update(struct snd_soc_card *card) struct snd_soc_dapm_update *update = card->update; struct snd_soc_dapm_widget_list *wlist; struct snd_soc_dapm_widget *w = NULL; - unsigned int wi; - int ret; + unsigned int wi, i; + int ret = 0;
if (!update || !dapm_kcontrol_is_powered(update->kcontrol)) return; @@ -1575,8 +1588,12 @@ static void dapm_widget_update(struct snd_soc_card *card) if (!w) return;
- ret = soc_widget_update_bits_locked(w, update->reg, update->mask, - update->val); + /* dapm update for multiple registers */ + for (i = 0; i < update->num_regs; i++) { + ret |= soc_widget_update_bits_locked(w, update->reg[i], + update->mask[i], update->val[i]); + } + if (ret < 0) dev_err(w->dapm->dev, "ASoC: %s DAPM update failed: %d\n", w->name, ret); @@ -2866,10 +2883,10 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, if (change) { if (reg != SND_SOC_NOPM) { update.kcontrol = kcontrol; - update.reg = reg; - update.mask = mask; - update.val = val; - + update.reg[0] = reg; + update.mask[0] = mask; + update.val[0] = val; + update.num_regs = 1; card->update = &update; }
@@ -2903,15 +2920,15 @@ int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol, struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; unsigned int reg_val, val;
- if (e->reg != SND_SOC_NOPM) - reg_val = snd_soc_read(codec, e->reg); + if (e->reg[0] != SND_SOC_NOPM) + reg_val = snd_soc_read(codec, e->reg[0]); else reg_val = dapm_kcontrol_get_value(kcontrol);
- val = (reg_val >> e->shift_l) & e->mask; + 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; + val = (reg_val >> e->shift_r) & e->mask[0]; val = snd_soc_enum_val_to_item(e, val); ucontrol->value.enumerated.item[1] = val; } @@ -2945,27 +2962,28 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, return -EINVAL;
val = snd_soc_enum_item_to_val(e, item[0]) << e->shift_l; - mask = e->mask << 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 << e->shift_r; + mask |= e->mask[0] << e->shift_r; }
mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
- if (e->reg != SND_SOC_NOPM) - change = snd_soc_test_bits(codec, e->reg, mask, val); + if (e->reg[0] != SND_SOC_NOPM) + change = snd_soc_test_bits(codec, e->reg[0], mask, val); else change = dapm_kcontrol_set_value(kcontrol, val);
if (change) { - if (e->reg != SND_SOC_NOPM) { + if (e->reg[0] != SND_SOC_NOPM) { update.kcontrol = kcontrol; - update.reg = e->reg; - update.mask = mask; - update.val = val; + update.reg[0] = e->reg[0]; + update.mask[0] = mask; + update.val[0] = val; + update.num_regs = 1; card->update = &update; }
@@ -2984,6 +3002,112 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double);
/** + * snd_soc_dapm_get_enum_onehot - dapm enumerated onehot mixer get callback + * @kcontrol: mixer control + * @ucontrol: control element information + * + * Callback to get the value of a dapm enumerated onehot encoded mixer control + * + * Returns 0 for success. + */ +int snd_soc_dapm_get_enum_onehot(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, bit_pos = -1, reg_idx; + + 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->mask[reg_idx]; + if (val != 0) { + bit_pos = __ffs(val) + (8 * codec->val_bytes * reg_idx); + break; + } + } + + ucontrol->value.enumerated.item[0] = + snd_soc_enum_val_to_item(e, bit_pos); + + return 0; +} +EXPORT_SYMBOL_GPL(snd_soc_dapm_get_enum_onehot); + +/** + * snd_soc_dapm_put_enum_onehot - dapm enumerated onehot mixer put callback + * @kcontrol: mixer control + * @ucontrol: control element information + * + * Callback to put the value of a dapm enumerated onehot encoded mixer control + * + * Returns 0 for success. + */ +int snd_soc_dapm_put_enum_onehot(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 = 0, reg_idx = 0, value, bit_pos; + struct snd_soc_dapm_update update; + int ret = 0, reg_val = 0, i, update_idx = 0; + + if (item[0] >= e->items) + return -EINVAL; + + value = snd_soc_enum_item_to_val(e, item[0]); + + if (value >= 0) { + /* get the register index and value to set */ + reg_idx = value / (8 * codec->val_bytes); + bit_pos = value % (8 * codec->val_bytes); + reg_val = BIT(bit_pos); + } + + for (i = 0; i < e->num_regs; i++) { + if (i == reg_idx) { + change = snd_soc_test_bits(codec, e->reg[i], + e->mask[i], reg_val); + /* set the selected register */ + update.reg[e->num_regs - 1] = e->reg[reg_idx]; + update.mask[e->num_regs - 1] = e->mask[reg_idx]; + update.val[e->num_regs - 1] = reg_val; + } else { + /* accumulate the change to update the DAPM path + when none is selected */ + change |= snd_soc_test_bits(codec, e->reg[i], + e->mask[i], 0); + + /* clear the register when not selected */ + update.reg[update_idx] = e->reg[i]; + update.mask[update_idx] = e->mask[i]; + update.val[update_idx++] = 0; + } + } + + mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME); + + if (change) { + update.kcontrol = kcontrol; + update.num_regs = 3; + 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; +} +EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_onehot); + +/** * snd_soc_dapm_info_pin_switch - Info for a pin switch * * @kcontrol: mixer control
On 04/03/2014 05:11 AM, Arun Shamanna Lakshmi wrote:
This looks essentially good to me. A few minor issues, once those are fixed things should be good to go.
[...]
struct snd_soc_dapm_update { struct snd_kcontrol *kcontrol;
- int reg;
- int mask;
- int val;
- int reg[3];
- int mask[3];
- int val[3];
Make the 3 a define and check against it in the put handler.
- int num_regs;
unsigned int
};
[...]
+/*
- Soc Enum Type
- @NONE: soc_enum type for SINGLE, DOUBLE or VIRTUAL mux
- @ONEHOT: soc_enum type for one hot encoding mux
- */
This should be kernel doc style so
/** * enum snd_soc_enum_type - Type of the ASoC enum control * @SND_SOC_ENUM_NONE: ... * ... */
+enum snd_soc_enum_type {
- SND_SOC_ENUM_NONE = 0,
I'm not sure if NONE is the right term. Maybe BINARY is better.
- SND_SOC_ENUM_ONEHOT = 1,
+};
[...]
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index c8a780d..19b004a 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -511,13 +511,26 @@ static int dapm_connect_mux(struct snd_soc_dapm_context *dapm, const struct snd_kcontrol_new *kcontrol) { struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
- unsigned int val, item;
- unsigned int val, item, bit_pos = -1;
default for bit_pos should probably be 0.
[...]
@@ -1575,8 +1588,12 @@ static void dapm_widget_update(struct snd_soc_card *card) if (!w) return;
- ret = soc_widget_update_bits_locked(w, update->reg, update->mask,
update->val);
- /* dapm update for multiple registers */
- for (i = 0; i < update->num_regs; i++) {
ret |= soc_widget_update_bits_locked(w, update->reg[i],update->mask[i], update->val[i]);
I'd prefer ret = soc_widget_update_bits_locked(...); if (ret < 0) break;
- }
- if (ret < 0) dev_err(w->dapm->dev, "ASoC: %s DAPM update failed: %d\n", w->name, ret);
[...]
@@ -2984,6 +3002,112 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double);
/**
- snd_soc_dapm_get_enum_onehot - dapm enumerated onehot mixer get callback
- @kcontrol: mixer control
- @ucontrol: control element information
- Callback to get the value of a dapm enumerated onehot encoded mixer control
- Returns 0 for success.
- */
+int snd_soc_dapm_get_enum_onehot(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, bit_pos = -1, reg_idx;
Here as well, default for bit_pos should be 0.
- 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->mask[reg_idx];if (val != 0) {bit_pos = __ffs(val) + (8 * codec->val_bytes * reg_idx);break;}- }
- ucontrol->value.enumerated.item[0] =
snd_soc_enum_val_to_item(e, bit_pos);- return 0;
+} +EXPORT_SYMBOL_GPL(snd_soc_dapm_get_enum_onehot);
+/**
- snd_soc_dapm_put_enum_onehot - dapm enumerated onehot mixer put callback
- @kcontrol: mixer control
- @ucontrol: control element information
- Callback to put the value of a dapm enumerated onehot encoded mixer control
- Returns 0 for success.
- */
+int snd_soc_dapm_put_enum_onehot(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 = 0, reg_idx = 0, value, bit_pos;
- struct snd_soc_dapm_update update;
- int ret = 0, reg_val = 0, i, update_idx = 0;
- if (item[0] >= e->items)
return -EINVAL;- value = snd_soc_enum_item_to_val(e, item[0]);
- if (value >= 0) {
value is unsigned int, so this is never false.
/* get the register index and value to set */reg_idx = value / (8 * codec->val_bytes);bit_pos = value % (8 * codec->val_bytes);reg_val = BIT(bit_pos);- }
- for (i = 0; i < e->num_regs; i++) {
if (i == reg_idx) {change = snd_soc_test_bits(codec, e->reg[i],e->mask[i], reg_val);
change |=
/* set the selected register */update.reg[e->num_regs - 1] = e->reg[reg_idx];update.mask[e->num_regs - 1] = e->mask[reg_idx];update.val[e->num_regs - 1] = reg_val;} else {/* accumulate the change to update the DAPM pathwhen none is selected */change |= snd_soc_test_bits(codec, e->reg[i],e->mask[i], 0);/* clear the register when not selected */update.reg[update_idx] = e->reg[i];update.mask[update_idx] = e->mask[i];update.val[update_idx++] = 0;}- }
- mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
- if (change) {
update.kcontrol = kcontrol;update.num_regs = 3;
Should be e->num_regs;
card->update = &update;ret = soc_dapm_mux_update_power(card, kcontrol, item[0], e);card->update = NULL;- }
On Thu, Apr 03, 2014 at 10:27:17AM +0200, Lars-Peter Clausen wrote:
On 04/03/2014 05:11 AM, Arun Shamanna Lakshmi wrote:
+enum snd_soc_enum_type {
- SND_SOC_ENUM_NONE = 0,
I'm not sure if NONE is the right term. Maybe BINARY is better.
Yes, it's definitely not none.
-----Original Message----- From: Lars-Peter Clausen [mailto:lars@metafoo.de] Sent: Thursday, April 03, 2014 1:27 AM To: Arun Shamanna Lakshmi Cc: lgirdwood@gmail.com; broonie@kernel.org;
swarren@wwwdotorg.org;
perex@perex.cz; tiwai@suse.de; alsa- devel@alsa-project.org; linux-kernel@vger.kernel.org; Songhee Baek Subject: Re: [PATCH] ASoC: dapm: Add support for multi register mux
On 04/03/2014 05:11 AM, Arun Shamanna Lakshmi wrote:
This looks essentially good to me. A few minor issues, once those are fixed things should be good to go.
[...]
@@ -2984,6 +3002,112 @@ int
snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double);
/**
- snd_soc_dapm_get_enum_onehot - dapm enumerated onehot
mixer get
+callback
- @kcontrol: mixer control
- @ucontrol: control element information
- Callback to get the value of a dapm enumerated onehot encoded
+mixer control
- Returns 0 for success.
- */
+int snd_soc_dapm_get_enum_onehot(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, bit_pos = -1, reg_idx;
Here as well, default for bit_pos should be 0.
This means when 'None' of the options are selected, by default, it enumerates to 0. Since we are using __ffs, BIT(0) of Register-0 also enumerates to 0. That's the reason why I used just ffs in the first place. Let me know your opinion. My value table looks like below.
#define MUX_VALUE(npart, nbit) (nbit + 32 * npart) static const int mux_values[] = { 0, MUX_VALUE(0, 0), . . . MUX_VALUE(0, 31), /* above inputs are for part0 mux */ MUX_VALUE(1, 0), . . . MUX_VALUE(1, 31), /* above inputs are for part1 mux */ MUX_VALUE(2, 0), . . . MUX_VALUE(2, 31), /* above inputs are for part2 mux */ };
- 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->mask[reg_idx];if (val != 0) {bit_pos = __ffs(val) + (8 * codec->val_bytes *reg_idx);
break;}- }
- ucontrol->value.enumerated.item[0] =
snd_soc_enum_val_to_item(e, bit_pos);- return 0;
+} +EXPORT_SYMBOL_GPL(snd_soc_dapm_get_enum_onehot);
+/**
On 04/03/2014 10:11 PM, Arun Shamanna Lakshmi wrote: [...]
Here as well, default for bit_pos should be 0.
This means when 'None' of the options are selected, by default, it enumerates to 0. Since we are using __ffs, BIT(0) of Register-0 also enumerates to 0. That's the reason why I used just ffs in the first place. Let me know your opinion. My value table looks like below.
#define MUX_VALUE(npart, nbit) (nbit + 32 * npart) static const int mux_values[] = { 0, MUX_VALUE(0, 0), . . . MUX_VALUE(0, 31), /* above inputs are for part0 mux */ MUX_VALUE(1, 0), . . . MUX_VALUE(1, 31), /* above inputs are for part1 mux */ MUX_VALUE(2, 0), . . . MUX_VALUE(2, 31), /* above inputs are for part2 mux */ };
Ok, so having none of the input selected should be a valid user selectable option?
-----Original Message----- From: Lars-Peter Clausen [mailto:lars@metafoo.de] Sent: Friday, April 04, 2014 12:32 AM To: Arun Shamanna Lakshmi Cc: lgirdwood@gmail.com; broonie@kernel.org; swarren@wwwdotorg.org; perex@perex.cz; tiwai@suse.de; alsa- devel@alsa-project.org; linux-kernel@vger.kernel.org; Songhee Baek Subject: Re: [PATCH] ASoC: dapm: Add support for multi register mux
On 04/03/2014 10:11 PM, Arun Shamanna Lakshmi wrote: [...]
Here as well, default for bit_pos should be 0.
This means when 'None' of the options are selected, by default, it enumerates to 0. Since we are using __ffs, BIT(0) of Register-0 also enumerates to 0. That's the reason why I used just ffs in the first place. Let me know your opinion. My value table looks like below.
#define MUX_VALUE(npart, nbit) (nbit + 32 * npart) static const int mux_values[] = { 0, MUX_VALUE(0, 0), . . . MUX_VALUE(0, 31), /* above inputs are for part0 mux */ MUX_VALUE(1, 0), . . . MUX_VALUE(1, 31), /* above inputs are for part1 mux */ MUX_VALUE(2, 0), . . . MUX_VALUE(2, 31), /* above inputs are for part2 mux */ };
Ok, so having none of the input selected should be a valid user selectable option?
Yes. If 'None' is selected, it goes and clears the register. So, can we have ffs( ) instead of __ffs( ) ? It would fix this case.
- Arun
On 04/04/2014 09:34 AM, Arun Shamanna Lakshmi wrote:
-----Original Message----- From: Lars-Peter Clausen [mailto:lars@metafoo.de] Sent: Friday, April 04, 2014 12:32 AM To: Arun Shamanna Lakshmi Cc: lgirdwood@gmail.com; broonie@kernel.org; swarren@wwwdotorg.org; perex@perex.cz; tiwai@suse.de; alsa- devel@alsa-project.org; linux-kernel@vger.kernel.org; Songhee Baek Subject: Re: [PATCH] ASoC: dapm: Add support for multi register mux
On 04/03/2014 10:11 PM, Arun Shamanna Lakshmi wrote: [...]
Here as well, default for bit_pos should be 0.
This means when 'None' of the options are selected, by default, it enumerates to 0. Since we are using __ffs, BIT(0) of Register-0 also enumerates to 0. That's the reason why I used just ffs in the first place. Let me know your opinion. My value table looks like below.
#define MUX_VALUE(npart, nbit) (nbit + 32 * npart) static const int mux_values[] = { 0, MUX_VALUE(0, 0), . . . MUX_VALUE(0, 31), /* above inputs are for part0 mux */ MUX_VALUE(1, 0), . . . MUX_VALUE(1, 31), /* above inputs are for part1 mux */ MUX_VALUE(2, 0), . . . MUX_VALUE(2, 31), /* above inputs are for part2 mux */ };
Ok, so having none of the input selected should be a valid user selectable option?
Yes. If 'None' is selected, it goes and clears the register. So, can we have ffs( ) instead of __ffs( ) ? It would fix this case.
Yes, but you need to make sure to handle it also correctly in the put handler, since all of the registers need to be written to 0 in that case.
At Wed, 2 Apr 2014 20:11:50 -0700, Arun Shamanna Lakshmi wrote:
- Modify soc_enum struct to handle pointers for reg and mask
- Add dapm get and put APIs for multi register mux with one hot encoding
- Update snd_soc_dapm_update struct to support multiple reg update
Signed-off-by: Arun Shamanna Lakshmi aruns@nvidia.com Signed-off-by: Songhee Baek sbaek@nvidia.com
I'm a bit late in the game, but I feel a bit uneasy through looking at the whole changes. My primary question is, whether do we really need to share the same struct soc_enum for the onehot type? What makes hard to use a struct soc_enum_onehot for them? You need different individual get/put for each type. We may still need to change soc_dapm_update stuff, but it's different from sharing soc_enum.
thanks,
Takashi
include/sound/soc-dapm.h | 17 ++++- include/sound/soc.h | 34 +++++++-- sound/soc/soc-core.c | 12 ++-- sound/soc/soc-dapm.c | 174 +++++++++++++++++++++++++++++++++++++++------- 4 files changed, 197 insertions(+), 40 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index ef78f56..ded46732 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -305,6 +305,12 @@ struct device; .get = snd_soc_dapm_get_enum_double, \ .put = snd_soc_dapm_put_enum_double, \ .private_value = (unsigned long)&xenum } +#define SOC_DAPM_ENUM_ONEHOT(xname, xenum) \ +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
- .info = snd_soc_info_enum_double, \
- .get = snd_soc_dapm_get_enum_onehot, \
- .put = snd_soc_dapm_put_enum_onehot, \
- .private_value = (unsigned long)&xenum }
#define SOC_DAPM_ENUM_VIRT(xname, xenum) \ SOC_DAPM_ENUM(xname, xenum) #define SOC_DAPM_ENUM_EXT(xname, xenum, xget, xput) \ @@ -378,6 +384,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_enum_onehot(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol);
+int snd_soc_dapm_put_enum_onehot(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 +600,10 @@ struct snd_soc_dapm_widget {
struct snd_soc_dapm_update { struct snd_kcontrol *kcontrol;
- int reg;
- int mask;
- int val;
- int reg[3];
- int mask[3];
- int val[3];
- int num_regs;
};
/* DAPM context */ diff --git a/include/sound/soc.h b/include/sound/soc.h index 0b83168..add326a 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -177,18 +177,24 @@ {.reg = xreg, .min = xmin, .max = xmax, \ .platform_max = xmax} } #define SOC_ENUM_DOUBLE(xreg, xshift_l, xshift_r, xitems, xtexts) \ -{ .reg = xreg, .shift_l = xshift_l, .shift_r = xshift_r, \ +{ .reg = &(int){(xreg)}, .shift_l = xshift_l, .shift_r = xshift_r, \ .items = xitems, .texts = xtexts, \
- .mask = xitems ? roundup_pow_of_two(xitems) - 1 : 0}
- .mask = &(unsigned int){(xitems ? roundup_pow_of_two(xitems) - 1 : 0)}, \
- .num_regs = 1, .type = SND_SOC_ENUM_NONE }
#define SOC_ENUM_SINGLE(xreg, xshift, xitems, xtexts) \ SOC_ENUM_DOUBLE(xreg, xshift, xshift, xitems, xtexts) #define SOC_ENUM_SINGLE_EXT(xitems, xtexts) \ { .items = xitems, .texts = xtexts } #define SOC_VALUE_ENUM_DOUBLE(xreg, xshift_l, xshift_r, xmask, xitems, xtexts, xvalues) \ -{ .reg = xreg, .shift_l = xshift_l, .shift_r = xshift_r, \
- .mask = xmask, .items = xitems, .texts = xtexts, .values = xvalues}
+{ .reg = &(int){(xreg)}, .shift_l = xshift_l, .shift_r = xshift_r, \
- .mask = &(unsigned int){(xmask)}, .items = xitems, .texts = xtexts, \
- .values = xvalues, .num_regs = 1, .type = SND_SOC_ENUM_NONE }
#define SOC_VALUE_ENUM_SINGLE(xreg, xshift, xmask, xnitmes, xtexts, xvalues) \ SOC_VALUE_ENUM_DOUBLE(xreg, xshift, xshift, xmask, xnitmes, xtexts, xvalues) +#define SOC_VALUE_ENUM_ONEHOT(xregs, xmasks, xnum_regs, xitems, xtexts, xvalues) \ +{ .reg = xregs, .mask = xmasks, .num_regs = xnum_regs, \
- .items = xitems, .texts = xtexts, .values = xvalues, \
- .type = SND_SOC_ENUM_ONEHOT }
#define SOC_ENUM_SINGLE_VIRT(xitems, xtexts) \ SOC_ENUM_SINGLE(SND_SOC_NOPM, 0, xitems, xtexts) #define SOC_ENUM(xname, xenum) \ @@ -293,6 +299,9 @@ #define SOC_VALUE_ENUM_DOUBLE_DECL(name, xreg, xshift_l, xshift_r, xmask, xtexts, xvalues) \ const struct soc_enum name = SOC_VALUE_ENUM_DOUBLE(xreg, xshift_l, xshift_r, xmask, \ ARRAY_SIZE(xtexts), xtexts, xvalues) +#define SOC_VALUE_ENUM_ONEHOT_DECL(name, xregs, xmasks, xnum_regs, xtexts, xvalues) \
- const struct soc_enum name = SOC_VALUE_ENUM_ONEHOT(xregs, xmasks, xnum_regs, \
ARRAY_SIZE(xtexts), xtexts, xvalues)#define SOC_VALUE_ENUM_SINGLE_DECL(name, xreg, xshift, xmask, xtexts, xvalues) \ SOC_VALUE_ENUM_DOUBLE_DECL(name, xreg, xshift, xshift, xmask, xtexts, xvalues) #define SOC_ENUM_SINGLE_VIRT_DECL(name, xtexts) \ @@ -326,6 +335,17 @@ enum snd_soc_bias_level { SND_SOC_BIAS_ON = 3, };
+/*
- Soc Enum Type
- @NONE: soc_enum type for SINGLE, DOUBLE or VIRTUAL mux
- @ONEHOT: soc_enum type for one hot encoding mux
- */
+enum snd_soc_enum_type {
- SND_SOC_ENUM_NONE = 0,
- SND_SOC_ENUM_ONEHOT = 1,
+};
struct device_node; struct snd_jack; struct snd_soc_card; @@ -1098,13 +1118,15 @@ struct soc_mreg_control {
/* enumerated kcontrol */ struct soc_enum {
- int reg;
- int *reg; unsigned char shift_l; unsigned char shift_r; unsigned int items;
- unsigned int mask;
- unsigned int *mask; const char * const *texts; const unsigned int *values;
- enum snd_soc_enum_type type;
- unsigned int num_regs;
};
/** diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index caebd63..cf29722 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2601,12 +2601,12 @@ int snd_soc_get_enum_double(struct snd_kcontrol *kcontrol, unsigned int val, item; unsigned int reg_val;
- reg_val = snd_soc_read(codec, e->reg);
- val = (reg_val >> e->shift_l) & e->mask;
- reg_val = snd_soc_read(codec, e->reg[0]);
- val = (reg_val >> e->shift_l) & e->mask[0]; item = snd_soc_enum_val_to_item(e, val); ucontrol->value.enumerated.item[0] = item; if (e->shift_l != e->shift_r) {
val = (reg_val >> e->shift_l) & e->mask;
val = (reg_val >> e->shift_l) & e->mask[0];@@ -2636,15 +2636,15 @@ int snd_soc_put_enum_double(struct snd_kcontrol *kcontrol, if (item[0] >= e->items) return -EINVAL; val = snd_soc_enum_item_to_val(e, item[0]) << e->shift_l;
- mask = e->mask << 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_r;
mask |= e->mask << e->shift_r;
mask |= e->mask[0] << e->shift_r;
- return snd_soc_update_bits_locked(codec, e->reg, mask, val);
- return snd_soc_update_bits_locked(codec, e->reg[0], mask, val);
} EXPORT_SYMBOL_GPL(snd_soc_put_enum_double);
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index c8a780d..19b004a 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -511,13 +511,26 @@ static int dapm_connect_mux(struct snd_soc_dapm_context *dapm, const struct snd_kcontrol_new *kcontrol) { struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
- unsigned int val, item;
- unsigned int val, item, bit_pos = -1; int i;
- if (e->reg != SND_SOC_NOPM) {
soc_widget_read(dest, e->reg, &val);val = (val >> e->shift_l) & e->mask;item = snd_soc_enum_val_to_item(e, val);
- if (e->reg[0] != SND_SOC_NOPM) {
if (e->type == SND_SOC_ENUM_ONEHOT) {for (i = 0; i < e->num_regs; i++) {soc_widget_read(dest, e->reg[i], &val);val = val & e->mask[i];if (val != 0) {bit_pos = __ffs(val) +(8 * dest->codec->val_bytes * i);break;}}item = snd_soc_enum_val_to_item(e, bit_pos);} else {soc_widget_read(dest, e->reg[0], &val);val = (val >> e->shift_l) & e->mask[0];item = snd_soc_enum_val_to_item(e, val);}
- decide which path to connect, it will try to match
@@ -1553,8 +1566,8 @@ static void dapm_widget_update(struct snd_soc_card *card) struct snd_soc_dapm_update *update = card->update; struct snd_soc_dapm_widget_list *wlist; struct snd_soc_dapm_widget *w = NULL;
- unsigned int wi;
- int ret;
unsigned int wi, i;
int ret = 0;
if (!update || !dapm_kcontrol_is_powered(update->kcontrol)) return;
@@ -1575,8 +1588,12 @@ static void dapm_widget_update(struct snd_soc_card *card) if (!w) return;
- ret = soc_widget_update_bits_locked(w, update->reg, update->mask,
update->val);
- /* dapm update for multiple registers */
- for (i = 0; i < update->num_regs; i++) {
ret |= soc_widget_update_bits_locked(w, update->reg[i],update->mask[i], update->val[i]);- }
- if (ret < 0) dev_err(w->dapm->dev, "ASoC: %s DAPM update failed: %d\n", w->name, ret);
@@ -2866,10 +2883,10 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, if (change) { if (reg != SND_SOC_NOPM) { update.kcontrol = kcontrol;
update.reg = reg;update.mask = mask;update.val = val;
update.reg[0] = reg;update.mask[0] = mask;update.val[0] = val;update.num_regs = 1; card->update = &update;@@ -2903,15 +2920,15 @@ int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol, struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; unsigned int reg_val, val;
- if (e->reg != SND_SOC_NOPM)
reg_val = snd_soc_read(codec, e->reg);
- if (e->reg[0] != SND_SOC_NOPM)
reg_val = snd_soc_read(codec, e->reg[0]);
- val = (reg_val >> e->shift_l) & e->mask;
- 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;
val = (reg_val >> e->shift_r) & e->mask[0];@@ -2945,27 +2962,28 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, return -EINVAL;
val = snd_soc_enum_item_to_val(e, item[0]) << e->shift_l;
- mask = e->mask << 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 << e->shift_r;
mask |= e->mask[0] << e->shift_r;}
mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
- if (e->reg != SND_SOC_NOPM)
change = snd_soc_test_bits(codec, e->reg, mask, val);
if (e->reg[0] != SND_SOC_NOPM)
change = snd_soc_test_bits(codec, e->reg[0], mask, val);else change = dapm_kcontrol_set_value(kcontrol, val);
if (change) {
if (e->reg != SND_SOC_NOPM) {
if (e->reg[0] != SND_SOC_NOPM) { update.kcontrol = kcontrol;
update.reg = e->reg;update.mask = mask;update.val = val;
update.reg[0] = e->reg[0];update.mask[0] = mask;update.val[0] = val;update.num_regs = 1; card->update = &update;@@ -2984,6 +3002,112 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double);
/**
- snd_soc_dapm_get_enum_onehot - dapm enumerated onehot mixer get callback
- @kcontrol: mixer control
- @ucontrol: control element information
- Callback to get the value of a dapm enumerated onehot encoded mixer control
- Returns 0 for success.
- */
+int snd_soc_dapm_get_enum_onehot(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, bit_pos = -1, reg_idx;
- 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->mask[reg_idx];if (val != 0) {bit_pos = __ffs(val) + (8 * codec->val_bytes * reg_idx);break;}- }
- ucontrol->value.enumerated.item[0] =
snd_soc_enum_val_to_item(e, bit_pos);- return 0;
+} +EXPORT_SYMBOL_GPL(snd_soc_dapm_get_enum_onehot);
+/**
- snd_soc_dapm_put_enum_onehot - dapm enumerated onehot mixer put callback
- @kcontrol: mixer control
- @ucontrol: control element information
- Callback to put the value of a dapm enumerated onehot encoded mixer control
- Returns 0 for success.
- */
+int snd_soc_dapm_put_enum_onehot(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 = 0, reg_idx = 0, value, bit_pos;
- struct snd_soc_dapm_update update;
- int ret = 0, reg_val = 0, i, update_idx = 0;
- if (item[0] >= e->items)
return -EINVAL;- value = snd_soc_enum_item_to_val(e, item[0]);
- if (value >= 0) {
/* get the register index and value to set */reg_idx = value / (8 * codec->val_bytes);bit_pos = value % (8 * codec->val_bytes);reg_val = BIT(bit_pos);- }
- for (i = 0; i < e->num_regs; i++) {
if (i == reg_idx) {change = snd_soc_test_bits(codec, e->reg[i],e->mask[i], reg_val);/* set the selected register */update.reg[e->num_regs - 1] = e->reg[reg_idx];update.mask[e->num_regs - 1] = e->mask[reg_idx];update.val[e->num_regs - 1] = reg_val;} else {/* accumulate the change to update the DAPM pathwhen none is selected */change |= snd_soc_test_bits(codec, e->reg[i],e->mask[i], 0);/* clear the register when not selected */update.reg[update_idx] = e->reg[i];update.mask[update_idx] = e->mask[i];update.val[update_idx++] = 0;}- }
- mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
- if (change) {
update.kcontrol = kcontrol;update.num_regs = 3;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;
+} +EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_onehot);
+/**
- snd_soc_dapm_info_pin_switch - Info for a pin switch
- @kcontrol: mixer control
-- 1.7.9.5
On Thu, Apr 03, 2014 at 11:47:15AM +0200, Takashi Iwai wrote:
I'm a bit late in the game, but I feel a bit uneasy through looking at the whole changes. My primary question is, whether do we really need to share the same struct soc_enum for the onehot type? What makes hard to use a struct soc_enum_onehot for them? You need different individual get/put for each type. We may still need to change soc_dapm_update stuff, but it's different from sharing soc_enum.
Indeed, I had thought this was where the discussion was heading - not looked at this version of the patch yet.
On 04/03/2014 11:53 AM, Mark Brown wrote:
On Thu, Apr 03, 2014 at 11:47:15AM +0200, Takashi Iwai wrote:
I'm a bit late in the game, but I feel a bit uneasy through looking at the whole changes. My primary question is, whether do we really need to share the same struct soc_enum for the onehot type? What makes hard to use a struct soc_enum_onehot for them? You need different individual get/put for each type. We may still need to change soc_dapm_update stuff, but it's different from sharing soc_enum.
Indeed, I had thought this was where the discussion was heading - not looked at this version of the patch yet.
It would be nice, but it also requires some slight restructuring. The issue we have right now is that there is strictly speaking a bit of a layering violation. The DAPM widgets should not need to know how the kcontrols that are attached to the widget do their IO. What we essentially do in dapm_connect_mux() (and also dapm_connect_mixer) is an open-coded version of the controls get handler. Replacing that by calling the get handler instead should allow us to use different structs for enums and onehot enums.
- Lars
At Thu, 03 Apr 2014 15:31:58 +0200, Lars-Peter Clausen wrote:
On 04/03/2014 11:53 AM, Mark Brown wrote:
On Thu, Apr 03, 2014 at 11:47:15AM +0200, Takashi Iwai wrote:
I'm a bit late in the game, but I feel a bit uneasy through looking at the whole changes. My primary question is, whether do we really need to share the same struct soc_enum for the onehot type? What makes hard to use a struct soc_enum_onehot for them? You need different individual get/put for each type. We may still need to change soc_dapm_update stuff, but it's different from sharing soc_enum.
Indeed, I had thought this was where the discussion was heading - not looked at this version of the patch yet.
It would be nice, but it also requires some slight restructuring. The issue we have right now is that there is strictly speaking a bit of a layering violation. The DAPM widgets should not need to know how the kcontrols that are attached to the widget do their IO. What we essentially do in dapm_connect_mux() (and also dapm_connect_mixer) is an open-coded version of the controls get handler. Replacing that by calling the get handler instead should allow us to use different structs for enums and onehot enums.
So, something like below? It's totally untested, just a proof of concept.
If the performance matters, we can optimize it by checking kcontrol->get == snd_soc_dapm_get_enum_double or kcontrol->get == snd_soc_dapm_get_volsw and bypass to the current open-code functions instead of the generic get/put callers.
thanks,
Takashi
--- diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index c8a780d0d057..5947c6e2fcc8 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -508,64 +508,71 @@ out: static int dapm_connect_mux(struct snd_soc_dapm_context *dapm, struct snd_soc_dapm_widget *src, struct snd_soc_dapm_widget *dest, struct snd_soc_dapm_path *path, const char *control_name, - const struct snd_kcontrol_new *kcontrol) + struct snd_kcontrol *kcontrol) { - struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; - unsigned int val, item; - int i; + struct snd_ctl_elem_info *uinfo; + struct snd_ctl_elem_value *ucontrol; + unsigned int i, item, items; + int err;
- if (e->reg != SND_SOC_NOPM) { - soc_widget_read(dest, e->reg, &val); - val = (val >> e->shift_l) & e->mask; - item = snd_soc_enum_val_to_item(e, val); - } else { - /* since a virtual mux has no backing registers to - * decide which path to connect, it will try to match - * with the first enumeration. This is to ensure - * that the default mux choice (the first) will be - * correctly powered up during initialization. - */ - item = 0; + uinfo = kzalloc(sizeof(*uinfo), GFP_KERNEL); + ucontrol = kzalloc(sizeof(*ucontrol), GFP_KERNEL); + if (!uinfo || !ucontrol) { + err = -ENOMEM; + goto out; + } + + err = kcontrol->info(kcontrol, uinfo); + if (err < 0) + goto out; + if (WARN_ON(uinfo->type != SNDRV_CTL_ELEM_TYPE_ENUMERATED)) { + err = -EINVAL; + goto out; } + items = uinfo->value.enumerated.items;
- for (i = 0; i < e->items; i++) { - if (!(strcmp(control_name, e->texts[i]))) { + err = kcontrol->get(kcontrol, ucontrol); + if (err < 0) + goto out; + item = ucontrol->value.enumerated.item[0]; + + for (i = 0; i < items; i++) { + uinfo->value.enumerated.item = i; + err = kcontrol->info(kcontrol, uinfo); + if (err < 0) + goto out; + if (!(strcmp(control_name, uinfo->value.enumerated.name))) { list_add(&path->list, &dapm->card->paths); list_add(&path->list_sink, &dest->sources); list_add(&path->list_source, &src->sinks); - path->name = (char*)e->texts[i]; + path->name = control_name; if (i == item) path->connect = 1; else path->connect = 0; - return 0; + goto out; } }
- return -ENODEV; + err = -ENODEV; + out: + kfree(ucontrol); + kfree(uinfo); + return err < 0 ? err : 0; }
/* set up initial codec paths */ static void dapm_set_mixer_path_status(struct snd_soc_dapm_widget *w, struct snd_soc_dapm_path *p, int i) { - struct soc_mixer_control *mc = (struct soc_mixer_control *) - w->kcontrol_news[i].private_value; - unsigned int reg = mc->reg; - unsigned int shift = mc->shift; - unsigned int max = mc->max; - unsigned int mask = (1 << fls(max)) - 1; - unsigned int invert = mc->invert; - unsigned int val; + struct snd_kcontrol *kcontrol = w->kcontrols[i]; + struct snd_ctl_elem_value *ucontrol = + kzalloc(sizeof(*ucontrol), GFP_KERNEL);
- if (reg != SND_SOC_NOPM) { - soc_widget_read(w, reg, &val); - val = (val >> shift) & mask; - if (invert) - val = max - val; - p->connect = !!val; - } else { - p->connect = 0; + if (ucontrol) { + if (kcontrol->get(kcontrol, ucontrol) >= 0) + p->connect = !!ucontrol->value.integer.value[0]; + kfree(ucontrol); } }
@@ -2415,7 +2422,7 @@ static int snd_soc_dapm_add_path(struct snd_soc_dapm_context *dapm, return 0; case snd_soc_dapm_mux: ret = dapm_connect_mux(dapm, wsource, wsink, path, control, - &wsink->kcontrol_news[0]); + wsink->kcontrols[0]); if (ret != 0) goto err; break;
On Thu, Apr 03, 2014 at 05:06:28PM +0200, Takashi Iwai wrote:
Lars-Peter Clausen wrote:
It would be nice, but it also requires some slight restructuring. The issue we have right now is that there is strictly speaking a bit of a layering violation. The DAPM widgets should not need to know how the kcontrols that are attached to the widget do their IO. What we essentially do in dapm_connect_mux() (and also dapm_connect_mixer) is an open-coded version of the controls get handler. Replacing that by calling the get handler instead should allow us to use different structs for enums and onehot enums.
Right, that's because DAPM has always just reimplemented everything and the virtual controls were grafted on the side of the existing register stuff rather than adding an abstraction layer.
So, something like below? It's totally untested, just a proof of concept.
Yes, that looks sensible to me but I didn't test it yet either.
If the performance matters, we can optimize it by checking kcontrol->get == snd_soc_dapm_get_enum_double or kcontrol->get == snd_soc_dapm_get_volsw and bypass to the current open-code functions instead of the generic get/put callers.
Performance isn't that big a deal, probably avoiding the alloc/frees by just keeping one around somewhere convenient would be useful too.
participants (4)
-
Arun Shamanna Lakshmi -
Lars-Peter Clausen -
Mark Brown -
Takashi Iwai