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;