[alsa-devel] [PATCH] ASoC: dapm: Add support for multi register mux
1. Modify soc_enum struct to handle pointers for reg and mask 2. Add dapm get and put APIs for multi register one hot encoded mux 3. Update snd_soc_dapm_update struct to support multiple reg update
Signed-off-by: Arun S L aruns@nvidia.com Signed-off-by: Songhee Baek sbaek@nvidia.com --- include/sound/soc-dapm.h | 20 ++++- include/sound/soc.h | 33 +++++++-- sound/soc/soc-core.c | 12 +-- sound/soc/soc-dapm.c | 181 +++++++++++++++++++++++++++++++++++++++------- 4 files changed, 203 insertions(+), 43 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index ef78f56..c255349 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) \ @@ -352,6 +358,9 @@ struct device; /* regulator widget flags */ #define SND_SOC_DAPM_REGULATOR_BYPASS 0x1 /* bypass when disabled */
+/* maximum registers to update */ +#define SND_SOC_DAPM_UPDATE_MAX_REG 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_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 +603,10 @@ struct snd_soc_dapm_widget {
struct snd_soc_dapm_update { struct snd_kcontrol *kcontrol; - int reg; - int mask; - int val; + int reg[SND_SOC_DAPM_UPDATE_MAX_REG]; + int mask[SND_SOC_DAPM_UPDATE_MAX_REG]; + int val[SND_SOC_DAPM_UPDATE_MAX_REG]; + unsigned int num_regs; };
/* DAPM context */ diff --git a/include/sound/soc.h b/include/sound/soc.h index 0b83168..dcdf1cb 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_BINARY } #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_BINARY } #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,16 @@ enum snd_soc_bias_level { SND_SOC_BIAS_ON = 3, };
+/* + * enum snd_soc_enum_type - Type of ASoC enum control + * @SND_SOC_ENUM_BINARY: soc_enum type for SINGLE, DOUBLE or VIRTUAL mux + * @SND_SOC_ENUM_ONEHOT: soc_enum type for ONEHOT encoding mux + */ +enum snd_soc_enum_type { + SND_SOC_ENUM_BINARY = 0, + SND_SOC_ENUM_ONEHOT = 1, +}; + struct device_node; struct snd_jack; struct snd_soc_card; @@ -1098,13 +1117,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..be9cd63 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 = 0; 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,11 +1588,16 @@ 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); - if (ret < 0) - dev_err(w->dapm->dev, "ASoC: %s DAPM update failed: %d\n", - w->name, ret); + /* 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); + break; + } + }
for (wi = 0; wi < wlist->num_widgets; wi++) { w = wlist->widgets[wi]; @@ -2866,10 +2884,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 +2921,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 +2963,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 +3003,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 = 0, 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; + unsigned int i, reg_val = 0, update_idx = 0; + struct snd_soc_dapm_update update; + int ret = 0; + + if (item[0] >= e->items || e->num_regs > SND_SOC_DAPM_UPDATE_MAX_REG) + return -EINVAL; + + value = snd_soc_enum_item_to_val(e, item[0]); + + if (value) { + /* get the register index and value to set */ + reg_idx = (value - 1) / (8 * codec->val_bytes); + bit_pos = (value - 1) % (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 = e->num_regs; + 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/05/2014 02:12 AM, Arun Shamanna Lakshmi wrote:
- Modify soc_enum struct to handle pointers for reg and mask
- Add dapm get and put APIs for multi register one hot encoded mux
- Update snd_soc_dapm_update struct to support multiple reg update
Signed-off-by: Arun S L aruns@nvidia.com Signed-off-by: Songhee Baek sbaek@nvidia.com
Looks good to me, so:
Reviewed-by: Lars-Peter Clausen lars@metafoo.de
As Takashi said it is not that nice that there is a bit of code churn by having to update all the existing users of e->reg and e->mask. But implementing this properly seems to cause even more code churn. And I think it will be done best in an effort to consolidate the kcontrol helpers and the DAPM kcontrol helpers by adding an additional layer of abstraction between the kcontrols and the hardware access that translates between the logical value and the physical value(s).
E.g. something like
struct kcontrol_ops { int (*read)(...); int (*write)(...); };
And then have one kind of ops for each kind of control type and at the high level only have put and get handlers for enums and for switches/volumes.
- Lars
At Mon, 07 Apr 2014 14:54:11 +0200, Lars-Peter Clausen wrote:
On 04/05/2014 02:12 AM, Arun Shamanna Lakshmi wrote:
- Modify soc_enum struct to handle pointers for reg and mask
- Add dapm get and put APIs for multi register one hot encoded mux
- Update snd_soc_dapm_update struct to support multiple reg update
Signed-off-by: Arun S L aruns@nvidia.com Signed-off-by: Songhee Baek sbaek@nvidia.com
Looks good to me, so:
Reviewed-by: Lars-Peter Clausen lars@metafoo.de
As Takashi said it is not that nice that there is a bit of code churn by having to update all the existing users of e->reg and e->mask. But implementing this properly seems to cause even more code churn.
Well, I'm not sure about it. It's already too late for 3.15, so we have some time to enhance for this feature properly.
I personally like a hackish solution, too, but only if it really gives a clear benefit over the generic solution. In this case, it changes the data type and yet increases the data size significantly, which isn't a good sign. (Note that replacing int with pointer can be twice on 64bit, and there will be pads for alignment, in addition to the extra space for number instances by this implementation.)
Also, as the data handling is branched per type, a more readable implementation would be to impose a union between normal and onehot types in soc_enum struct, I guess. But, then a question comes to the point whether we really want such a unification at all.
Last but not least, another concern is that there can be multiple onehot types: with 0 or without 0 handling. It'd be easy to add the implementation, but it indicates that the implementation isn't generic enough in comparison with the existing snd_soc code for single register.
And I think it will be done best in an effort to consolidate the kcontrol helpers and the DAPM kcontrol helpers by adding an additional layer of abstraction between the kcontrols and the hardware access that translates between the logical value and the physical value(s).
E.g. something like
struct kcontrol_ops { int (*read)(...); int (*write)(...); };
And then have one kind of ops for each kind of control type and at the high level only have put and get handlers for enums and for switches/volumes.
This might work well, indeed. But, let the actual code talk :)
thanks,
Takashi
On Fri, Apr 04, 2014 at 05:12:10PM -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 one hot encoded mux
- Update snd_soc_dapm_update struct to support multiple reg update
If you've got several changes like this it's probably a sign that the change ought to be split into a patch series.
I'm still not seeing any handling of the issues with having invalid configurations written to the device during the process of carrying out multi register updates; I did raise this with one of the earlier versions but don't recall any response.
I also think I agree with Takashi on this one - trying to implement this without adding an abstraction for the control values is making the code more complex than it needs to be, all the conditional paths for _ONEHOT aren't pretty (and don't use switches which is the usual idiom for this stuff if it's not indirected via functions).
participants (4)
-
Arun Shamanna Lakshmi
-
Lars-Peter Clausen
-
Mark Brown
-
Takashi Iwai