[RFC PATCH 0/3] Enable using multiple kcontrol types in a widget

[RFC PATCH 0/3] Enable using multiple kcontrol types in a widget

[Jaska Uimonen]

Hi,

I'm requesting comments to this small series to enable multiple
different kcontrol types for a single widget.

Currently asoc allows to define and parse multiple same type kcontrols
for single widget. So you can have for example two volume controls in a
widget but not for example one byte and one enum control. I personally
had this kind of use case where I wanted to set filter coefficients with
bytes and something else with an enum in one processing widget.

I don't have that good perspective on the asoc history on this part so I
don't know is there some reason for the existing logic or is it just ok
to change it like in this series. I've been told some drivers use virtual
widget for this kind of situation, so they define two widget's (one
virtual) with separate controls, but in reality bind the controls
into the same component in lower level. Or can we just actually use
separate types in one widget?

First patch is just adding the type field to kcontrol struct. Second
patch is a refactoring to take into account the different control types
in parsing the topology. So it looks a little bit messy, but is just
actually looping through the types in the kcontrol creation. Third patch
is me using this in sof driver, so not so directly needed here. I've
tested this thing personally up from topology down till the dsp and for
me it seems to work, but as said I can't be sure if I'm breaking something
else here.

Jaska Uimonen (3):
  ALSA: control: add kcontrol_type to control
  ASoC: soc-topology: enable use of multiple control types
  ASoC: SOF: topology: use individual kcontrol types

 include/sound/control.h  |   2 +
 sound/core/control.c     |   2 +
 sound/soc/soc-topology.c | 462 ++++++++++++++++++---------------------
 sound/soc/sof/topology.c |  13 +-
 4 files changed, 230 insertions(+), 249 deletions(-)

-- 
2.24.1

[RFC PATCH 1/3] ALSA: control: add kcontrol_type to control

[Jaska Uimonen]

Current kcontrol structs don't have a member to describe the control
type. The type is present in the widget which contains the control. As
there can be many controls in one widget it is inherently presumed that
the control types are the same.

Lately there has been use cases where different types of controls would
be needed for single widget. Thus enable this by adding the control type
to kcontrol and kcontrol_new structs.

Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>
---
 include/sound/control.h | 2 ++
 sound/core/control.c    | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/include/sound/control.h b/include/sound/control.h
index 77d9fa10812d..3933823a606d 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -46,6 +46,7 @@ struct snd_kcontrol_new {
 	unsigned int index;		/* index of item */
 	unsigned int access;		/* access rights */
 	unsigned int count;		/* count of same elements */
+	unsigned int kcontrol_type;
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
@@ -65,6 +66,7 @@ struct snd_kcontrol {
 	struct list_head list;		/* list of controls */
 	struct snd_ctl_elem_id id;
 	unsigned int count;		/* count of same elements */
+	unsigned int kcontrol_type;
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
diff --git a/sound/core/control.c b/sound/core/control.c
index 3b44378b9dec..f081ae4f839c 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -268,6 +268,8 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol,
 	}
 	kctl->id.index = ncontrol->index;
 
+	kctl->kcontrol_type = ncontrol->kcontrol_type;
+
 	kctl->info = ncontrol->info;
 	kctl->get = ncontrol->get;
 	kctl->put = ncontrol->put;
-- 
2.24.1

Re: [RFC PATCH 1/3] ALSA: control: add kcontrol_type to control

[Jaroslav Kysela]

Dne 08. 01. 21 v 12:23 Jaska Uimonen napsal(a):
> Current kcontrol structs don't have a member to describe the control
> type. The type is present in the widget which contains the control. As
> there can be many controls in one widget it is inherently presumed that
> the control types are the same.
> 
> Lately there has been use cases where different types of controls would
> be needed for single widget. Thus enable this by adding the control type
> to kcontrol and kcontrol_new structs.

It looks like a SoC only extension. Use private_data to carry this
information. It has no value for the toplevel code.

				Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 1/3] ALSA: control: add kcontrol_type to control

[Takashi Sakamoto]

Hi,

On Fri, Jan 08, 2021 at 12:40:28PM +0100, Jaroslav Kysela wrote:
> Dne 08. 01. 21 v 12:23 Jaska Uimonen napsal(a):
> > Current kcontrol structs don't have a member to describe the control
> > type. The type is present in the widget which contains the control. As
> > there can be many controls in one widget it is inherently presumed that
> > the control types are the same.
> > 
> > Lately there has been use cases where different types of controls would
> > be needed for single widget. Thus enable this by adding the control type
> > to kcontrol and kcontrol_new structs.
> 
> It looks like a SoC only extension. Use private_data to carry this
> information. It has no value for the toplevel code.
> 
> 				Jaroslav

In current design of ALSA control core, the type of control element is
firstly determined by driver in callback of snd_kcontrol.info(). The
callback is done when userspace applications call ioctl(2) with
SNDRV_CTL_IOCTL_ELEM_INFO request.

The patch doesn't touch to the above processing. It means that the type
information is just for kernel-land implementation and is not exposed to
userspace application.

Essentially, driver is dominant to determine the type of control element
in control element set which the driver adds. It's possible to achieve
your intension without changing ALSA control core itself, in my opinion.

As Jaroslav said, it's better to change core of ALSA SoC part according
to your intention. If you'd like to change ALSA control core, I'd like
to request for the check of mismatch between the value of added member
in snd_kcontrol and the value of type of control element returned from
driver, like:

```
diff --git a/sound/core/control.c b/sound/core/control.c
index 809b0a62e..c3ae70574 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -973,6 +973,7 @@ static int __snd_ctl_elem_info(struct snd_card *card,
        result = kctl->info(kctl, info);
        if (result >= 0) {
                snd_BUG_ON(info->access);
+               snd_BUG_ON(info->type == kctl->kcontrol_type);
                index_offset = snd_ctl_get_ioff(kctl, &info->id);
                vd = &kctl->vd[index_offset];
                snd_ctl_build_ioff(&info->id, kctl, index_offset);
```


Regards

Takashi Sakamoto

Re: [RFC PATCH 1/3] ALSA: control: add kcontrol_type to control

[Jaska Uimonen]

On Fri, Jan 08, 2021 at 11:06:59PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> On Fri, Jan 08, 2021 at 12:40:28PM +0100, Jaroslav Kysela wrote:
> > Dne 08. 01. 21 v 12:23 Jaska Uimonen napsal(a):
> > > Current kcontrol structs don't have a member to describe the control
> > > type. The type is present in the widget which contains the control. As
> > > there can be many controls in one widget it is inherently presumed that
> > > the control types are the same.
> > > 
> > > Lately there has been use cases where different types of controls would
> > > be needed for single widget. Thus enable this by adding the control type
> > > to kcontrol and kcontrol_new structs.
> > 
> > It looks like a SoC only extension. Use private_data to carry this
> > information. It has no value for the toplevel code.
> > 
> > 				Jaroslav
> 
> In current design of ALSA control core, the type of control element is
> firstly determined by driver in callback of snd_kcontrol.info(). The
> callback is done when userspace applications call ioctl(2) with
> SNDRV_CTL_IOCTL_ELEM_INFO request.
> 
> The patch doesn't touch to the above processing. It means that the type
> information is just for kernel-land implementation and is not exposed to
> userspace application.
> 
> Essentially, driver is dominant to determine the type of control element
> in control element set which the driver adds. It's possible to achieve
> your intension without changing ALSA control core itself, in my opinion.
> 
> As Jaroslav said, it's better to change core of ALSA SoC part according
> to your intention. If you'd like to change ALSA control core, I'd like
> to request for the check of mismatch between the value of added member
> in snd_kcontrol and the value of type of control element returned from
> driver, like:
> 
> ```
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 809b0a62e..c3ae70574 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -973,6 +973,7 @@ static int __snd_ctl_elem_info(struct snd_card *card,
>         result = kctl->info(kctl, info);
>         if (result >= 0) {
>                 snd_BUG_ON(info->access);
> +               snd_BUG_ON(info->type == kctl->kcontrol_type);
>                 index_offset = snd_ctl_get_ioff(kctl, &info->id);
>                 vd = &kctl->vd[index_offset];
>                 snd_ctl_build_ioff(&info->id, kctl, index_offset);
> ```
> 
> 
> Regards
> 
> Takashi Sakamoto

Hi,

Thanks for the comments, I tried to do the same thing now in asoc level, 
will send v2.

br,
Jaska

[RFC PATCH 2/3] ASoC: soc-topology: enable use of multiple control types

[Jaska Uimonen]

Modify the kcontrol creation and delete functions to operate based on
individual kcontrol type. Previously all kcontrols in a widget we're
assumed to have the same type.

Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>
---
 sound/soc/soc-topology.c | 462 ++++++++++++++++++---------------------
 1 file changed, 218 insertions(+), 244 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index eb2633dd6454..096474c9ae51 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1194,249 +1194,219 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	return ret;
 }
 
-static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
-	struct soc_tplg *tplg, int num_kcontrols)
+static int soc_tplg_dapm_widget_dmixer_create(struct soc_tplg *tplg, struct snd_kcontrol_new *kc)
 {
-	struct snd_kcontrol_new *kc;
 	struct soc_mixer_control *sm;
 	struct snd_soc_tplg_mixer_control *mc;
-	int i, err;
-
-	kc = devm_kcalloc(tplg->dev, num_kcontrols, sizeof(*kc), GFP_KERNEL);
-	if (kc == NULL)
-		return NULL;
-
-	for (i = 0; i < num_kcontrols; i++) {
-		mc = (struct snd_soc_tplg_mixer_control *)tplg->pos;
-
-		/* validate kcontrol */
-		if (strnlen(mc->hdr.name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			goto err_sm;
+	int err;
 
-		sm = devm_kzalloc(tplg->dev, sizeof(*sm), GFP_KERNEL);
-		if (sm == NULL)
-			goto err_sm;
+	mc = (struct snd_soc_tplg_mixer_control *)tplg->pos;
 
-		tplg->pos += (sizeof(struct snd_soc_tplg_mixer_control) +
-			      le32_to_cpu(mc->priv.size));
+	/* validate kcontrol */
+	if (strnlen(mc->hdr.name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
+	    SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
+		return -EINVAL;
 
-		dev_dbg(tplg->dev, " adding DAPM widget mixer control %s at %d\n",
-			mc->hdr.name, i);
+	sm = devm_kzalloc(tplg->dev, sizeof(*sm), GFP_KERNEL);
+	if (!sm)
+		return -ENOMEM;
 
-		kc[i].private_value = (long)sm;
-		kc[i].name = devm_kstrdup(tplg->dev, mc->hdr.name, GFP_KERNEL);
-		if (kc[i].name == NULL)
-			goto err_sm;
-		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
-		kc[i].access = le32_to_cpu(mc->hdr.access);
+	tplg->pos += sizeof(struct snd_soc_tplg_mixer_control) +
+		le32_to_cpu(mc->priv.size);
 
-		/* we only support FL/FR channel mapping atm */
-		sm->reg = tplc_chan_get_reg(tplg, mc->channel,
-			SNDRV_CHMAP_FL);
-		sm->rreg = tplc_chan_get_reg(tplg, mc->channel,
-			SNDRV_CHMAP_FR);
-		sm->shift = tplc_chan_get_shift(tplg, mc->channel,
-			SNDRV_CHMAP_FL);
-		sm->rshift = tplc_chan_get_shift(tplg, mc->channel,
-			SNDRV_CHMAP_FR);
+	dev_dbg(tplg->dev, " adding DAPM widget mixer control %s\n",
+		mc->hdr.name);
 
-		sm->max = le32_to_cpu(mc->max);
-		sm->min = le32_to_cpu(mc->min);
-		sm->invert = le32_to_cpu(mc->invert);
-		sm->platform_max = le32_to_cpu(mc->platform_max);
-		sm->dobj.index = tplg->index;
-		INIT_LIST_HEAD(&sm->dobj.list);
-
-		/* map io handlers */
-		err = soc_tplg_kcontrol_bind_io(&mc->hdr, &kc[i], tplg);
-		if (err) {
-			soc_control_err(tplg, &mc->hdr, mc->hdr.name);
-			goto err_sm;
-		}
+	kc->private_value = (long)sm;
+	kc->name = devm_kstrdup(tplg->dev, mc->hdr.name, GFP_KERNEL);
+	if (!kc->name)
+		return -ENOMEM;
+	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	kc->access = le32_to_cpu(mc->hdr.access);
+	kc->kcontrol_type = SND_SOC_TPLG_TYPE_MIXER;
+
+	/* we only support FL/FR channel mapping atm */
+	sm->reg = tplc_chan_get_reg(tplg, mc->channel,
+				    SNDRV_CHMAP_FL);
+	sm->rreg = tplc_chan_get_reg(tplg, mc->channel,
+				     SNDRV_CHMAP_FR);
+	sm->shift = tplc_chan_get_shift(tplg, mc->channel,
+					SNDRV_CHMAP_FL);
+	sm->rshift = tplc_chan_get_shift(tplg, mc->channel,
+					 SNDRV_CHMAP_FR);
+
+	sm->max = le32_to_cpu(mc->max);
+	sm->min = le32_to_cpu(mc->min);
+	sm->invert = le32_to_cpu(mc->invert);
+	sm->platform_max = le32_to_cpu(mc->platform_max);
+	sm->dobj.index = tplg->index;
+	INIT_LIST_HEAD(&sm->dobj.list);
+
+	/* map io handlers */
+	err = soc_tplg_kcontrol_bind_io(&mc->hdr, kc, tplg);
+	if (err) {
+		soc_control_err(tplg, &mc->hdr, mc->hdr.name);
+		return err;
+	}
 
-		/* create any TLV data */
-		err = soc_tplg_create_tlv(tplg, &kc[i], &mc->hdr);
-		if (err < 0) {
-			dev_err(tplg->dev, "ASoC: failed to create TLV %s\n",
-				mc->hdr.name);
-			goto err_sm;
-		}
+	/* create any TLV data */
+	err = soc_tplg_create_tlv(tplg, kc, &mc->hdr);
+	if (err < 0) {
+		dev_err(tplg->dev, "ASoC: failed to create TLV %s\n",
+			mc->hdr.name);
+		return err;
+	}
 
-		/* pass control to driver for optional further init */
-		err = soc_tplg_init_kcontrol(tplg, &kc[i],
-			(struct snd_soc_tplg_ctl_hdr *)mc);
-		if (err < 0) {
-			dev_err(tplg->dev, "ASoC: failed to init %s\n",
-				mc->hdr.name);
-			goto err_sm;
-		}
+	/* pass control to driver for optional further init */
+	err = soc_tplg_init_kcontrol(tplg, kc,
+				     (struct snd_soc_tplg_ctl_hdr *)mc);
+	if (err < 0) {
+		dev_err(tplg->dev, "ASoC: failed to init %s\n",
+			mc->hdr.name);
+		return err;
 	}
-	return kc;
 
-err_sm:
-	return NULL;
+	return 0;
 }
 
-static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
-	struct soc_tplg *tplg, int num_kcontrols)
+static int soc_tplg_dapm_widget_denum_create(struct soc_tplg *tplg, struct snd_kcontrol_new *kc)
 {
-	struct snd_kcontrol_new *kc;
 	struct snd_soc_tplg_enum_control *ec;
 	struct soc_enum *se;
-	int i, err;
-
-	kc = devm_kcalloc(tplg->dev, num_kcontrols, sizeof(*kc), GFP_KERNEL);
-	if (kc == NULL)
-		return NULL;
-
-	for (i = 0; i < num_kcontrols; i++) {
-		ec = (struct snd_soc_tplg_enum_control *)tplg->pos;
-		/* validate kcontrol */
-		if (strnlen(ec->hdr.name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			goto err_se;
+	int err;
 
-		se = devm_kzalloc(tplg->dev, sizeof(*se), GFP_KERNEL);
-		if (se == NULL)
-			goto err_se;
+	ec = (struct snd_soc_tplg_enum_control *)tplg->pos;
+	/* validate kcontrol */
+	if (strnlen(ec->hdr.name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
+	    SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
+		return -EINVAL;
 
-		tplg->pos += (sizeof(struct snd_soc_tplg_enum_control) +
-			      le32_to_cpu(ec->priv.size));
+	se = devm_kzalloc(tplg->dev, sizeof(*se), GFP_KERNEL);
+	if (!se)
+		return -ENOMEM;
 
-		dev_dbg(tplg->dev, " adding DAPM widget enum control %s\n",
-			ec->hdr.name);
+	tplg->pos += (sizeof(struct snd_soc_tplg_enum_control) +
+		      le32_to_cpu(ec->priv.size));
 
-		kc[i].private_value = (long)se;
-		kc[i].name = devm_kstrdup(tplg->dev, ec->hdr.name, GFP_KERNEL);
-		if (kc[i].name == NULL)
-			goto err_se;
-		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
-		kc[i].access = le32_to_cpu(ec->hdr.access);
+	dev_dbg(tplg->dev, " adding DAPM widget enum control %s\n",
+		ec->hdr.name);
 
-		/* we only support FL/FR channel mapping atm */
-		se->reg = tplc_chan_get_reg(tplg, ec->channel, SNDRV_CHMAP_FL);
-		se->shift_l = tplc_chan_get_shift(tplg, ec->channel,
-						  SNDRV_CHMAP_FL);
-		se->shift_r = tplc_chan_get_shift(tplg, ec->channel,
-						  SNDRV_CHMAP_FR);
+	kc->private_value = (long)se;
+	kc->name = devm_kstrdup(tplg->dev, ec->hdr.name, GFP_KERNEL);
+	if (!kc->name)
+		return -ENOMEM;
+	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	kc->access = le32_to_cpu(ec->hdr.access);
+	kc->kcontrol_type = SND_SOC_TPLG_TYPE_ENUM;
 
-		se->items = le32_to_cpu(ec->items);
-		se->mask = le32_to_cpu(ec->mask);
-		se->dobj.index = tplg->index;
+	/* we only support FL/FR channel mapping atm */
+	se->reg = tplc_chan_get_reg(tplg, ec->channel, SNDRV_CHMAP_FL);
+	se->shift_l = tplc_chan_get_shift(tplg, ec->channel,
+					  SNDRV_CHMAP_FL);
+	se->shift_r = tplc_chan_get_shift(tplg, ec->channel,
+					  SNDRV_CHMAP_FR);
 
-		switch (le32_to_cpu(ec->hdr.ops.info)) {
-		case SND_SOC_TPLG_CTL_ENUM_VALUE:
-		case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE:
-			err = soc_tplg_denum_create_values(tplg, se, ec);
-			if (err < 0) {
-				dev_err(tplg->dev, "ASoC: could not create values for %s\n",
-					ec->hdr.name);
-				goto err_se;
-			}
-			fallthrough;
-		case SND_SOC_TPLG_CTL_ENUM:
-		case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE:
-		case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT:
-			err = soc_tplg_denum_create_texts(tplg, se, ec);
-			if (err < 0) {
-				dev_err(tplg->dev, "ASoC: could not create texts for %s\n",
-					ec->hdr.name);
-				goto err_se;
-			}
-			break;
-		default:
-			dev_err(tplg->dev, "ASoC: invalid enum control type %d for %s\n",
-				ec->hdr.ops.info, ec->hdr.name);
-			goto err_se;
-		}
+	se->items = le32_to_cpu(ec->items);
+	se->mask = le32_to_cpu(ec->mask);
+	se->dobj.index = tplg->index;
 
-		/* map io handlers */
-		err = soc_tplg_kcontrol_bind_io(&ec->hdr, &kc[i], tplg);
-		if (err) {
-			soc_control_err(tplg, &ec->hdr, ec->hdr.name);
-			goto err_se;
+	switch (le32_to_cpu(ec->hdr.ops.info)) {
+	case SND_SOC_TPLG_CTL_ENUM_VALUE:
+	case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE:
+		err = soc_tplg_denum_create_values(tplg, se, ec);
+		if (err < 0) {
+			dev_err(tplg->dev, "ASoC: could not create values for %s\n",
+				ec->hdr.name);
+			return err;
 		}
-
-		/* pass control to driver for optional further init */
-		err = soc_tplg_init_kcontrol(tplg, &kc[i],
-			(struct snd_soc_tplg_ctl_hdr *)ec);
+		fallthrough;
+	case SND_SOC_TPLG_CTL_ENUM:
+	case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE:
+	case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT:
+		err = soc_tplg_denum_create_texts(tplg, se, ec);
 		if (err < 0) {
-			dev_err(tplg->dev, "ASoC: failed to init %s\n",
+			dev_err(tplg->dev, "ASoC: could not create texts for %s\n",
 				ec->hdr.name);
-			goto err_se;
+			return err;
 		}
+		break;
+	default:
+		dev_err(tplg->dev, "ASoC: invalid enum control type %d for %s\n",
+			ec->hdr.ops.info, ec->hdr.name);
+		return -EINVAL;
 	}
 
-	return kc;
+	/* map io handlers */
+	err = soc_tplg_kcontrol_bind_io(&ec->hdr, kc, tplg);
+	if (err) {
+		soc_control_err(tplg, &ec->hdr, ec->hdr.name);
+		return err;
+	}
 
-err_se:
-	return NULL;
+	/* pass control to driver for optional further init */
+	err = soc_tplg_init_kcontrol(tplg, kc,
+				     (struct snd_soc_tplg_ctl_hdr *)ec);
+	if (err < 0) {
+		dev_err(tplg->dev, "ASoC: failed to init %s\n",
+			ec->hdr.name);
+		return err;
+	}
+
+	return 0;
 }
 
-static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
-	struct soc_tplg *tplg, int num_kcontrols)
+static int soc_tplg_dapm_widget_dbytes_create(struct soc_tplg *tplg, struct snd_kcontrol_new *kc)
 {
 	struct snd_soc_tplg_bytes_control *be;
 	struct soc_bytes_ext *sbe;
-	struct snd_kcontrol_new *kc;
-	int i, err;
-
-	kc = devm_kcalloc(tplg->dev, num_kcontrols, sizeof(*kc), GFP_KERNEL);
-	if (!kc)
-		return NULL;
+	int err;
 
-	for (i = 0; i < num_kcontrols; i++) {
-		be = (struct snd_soc_tplg_bytes_control *)tplg->pos;
+	be = (struct snd_soc_tplg_bytes_control *)tplg->pos;
 
-		/* validate kcontrol */
-		if (strnlen(be->hdr.name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			goto err_sbe;
-
-		sbe = devm_kzalloc(tplg->dev, sizeof(*sbe), GFP_KERNEL);
-		if (sbe == NULL)
-			goto err_sbe;
+	/* validate kcontrol */
+	if (strnlen(be->hdr.name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
+	    SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
+		return -EINVAL;
 
-		tplg->pos += (sizeof(struct snd_soc_tplg_bytes_control) +
-			      le32_to_cpu(be->priv.size));
+	sbe = devm_kzalloc(tplg->dev, sizeof(*sbe), GFP_KERNEL);
+	if (!sbe)
+		return -ENOMEM;
 
-		dev_dbg(tplg->dev,
-			"ASoC: adding bytes kcontrol %s with access 0x%x\n",
-			be->hdr.name, be->hdr.access);
+	tplg->pos += (sizeof(struct snd_soc_tplg_bytes_control) +
+		      le32_to_cpu(be->priv.size));
 
-		kc[i].private_value = (long)sbe;
-		kc[i].name = devm_kstrdup(tplg->dev, be->hdr.name, GFP_KERNEL);
-		if (kc[i].name == NULL)
-			goto err_sbe;
-		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
-		kc[i].access = le32_to_cpu(be->hdr.access);
+	dev_dbg(tplg->dev,
+		"ASoC: adding bytes kcontrol %s with access 0x%x\n",
+		be->hdr.name, be->hdr.access);
 
-		sbe->max = le32_to_cpu(be->max);
-		INIT_LIST_HEAD(&sbe->dobj.list);
+	kc->private_value = (long)sbe;
+	kc->name = devm_kstrdup(tplg->dev, be->hdr.name, GFP_KERNEL);
+	if (!kc->name)
+		return -ENOMEM;
+	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	kc->access = le32_to_cpu(be->hdr.access);
+	kc->kcontrol_type = SND_SOC_TPLG_TYPE_BYTES;
 
-		/* map standard io handlers and check for external handlers */
-		err = soc_tplg_kcontrol_bind_io(&be->hdr, &kc[i], tplg);
-		if (err) {
-			soc_control_err(tplg, &be->hdr, be->hdr.name);
-			goto err_sbe;
-		}
+	sbe->max = le32_to_cpu(be->max);
+	INIT_LIST_HEAD(&sbe->dobj.list);
 
-		/* pass control to driver for optional further init */
-		err = soc_tplg_init_kcontrol(tplg, &kc[i],
-			(struct snd_soc_tplg_ctl_hdr *)be);
-		if (err < 0) {
-			dev_err(tplg->dev, "ASoC: failed to init %s\n",
-				be->hdr.name);
-			goto err_sbe;
-		}
+	/* map standard io handlers and check for external handlers */
+	err = soc_tplg_kcontrol_bind_io(&be->hdr, kc, tplg);
+	if (err) {
+		soc_control_err(tplg, &be->hdr, be->hdr.name);
+		return err;
 	}
 
-	return kc;
-
-err_sbe:
+	/* pass control to driver for optional further init */
+	err = soc_tplg_init_kcontrol(tplg, kc,
+				     (struct snd_soc_tplg_ctl_hdr *)be);
+	if (err < 0) {
+		dev_err(tplg->dev, "ASoC: failed to init %s\n",
+			be->hdr.name);
+		return err;
+	}
 
-	return NULL;
+	return 0;
 }
 
 static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
@@ -1446,8 +1416,12 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
 	struct snd_soc_dapm_widget template, *widget;
 	struct snd_soc_tplg_ctl_hdr *control_hdr;
 	struct snd_soc_card *card = tplg->comp->card;
-	unsigned int kcontrol_type;
+	struct snd_kcontrol_new *kc;
+	int mixer_count = 0;
+	int bytes_count = 0;
+	int enum_count = 0;
 	int ret = 0;
+	int i;
 
 	if (strnlen(w->name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
 		SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
@@ -1490,7 +1464,6 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
 		 le32_to_cpu(w->priv.size));
 
 	if (w->num_kcontrols == 0) {
-		kcontrol_type = 0;
 		template.num_kcontrols = 0;
 		goto widget;
 	}
@@ -1499,57 +1472,58 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
 	dev_dbg(tplg->dev, "ASoC: template %s has %d controls of type %x\n",
 		w->name, w->num_kcontrols, control_hdr->type);
 
-	switch (le32_to_cpu(control_hdr->ops.info)) {
-	case SND_SOC_TPLG_CTL_VOLSW:
-	case SND_SOC_TPLG_CTL_STROBE:
-	case SND_SOC_TPLG_CTL_VOLSW_SX:
-	case SND_SOC_TPLG_CTL_VOLSW_XR_SX:
-	case SND_SOC_TPLG_CTL_RANGE:
-	case SND_SOC_TPLG_DAPM_CTL_VOLSW:
-		kcontrol_type = SND_SOC_TPLG_TYPE_MIXER;  /* volume mixer */
-		template.num_kcontrols = le32_to_cpu(w->num_kcontrols);
-		template.kcontrol_news =
-			soc_tplg_dapm_widget_dmixer_create(tplg,
-			template.num_kcontrols);
-		if (!template.kcontrol_news) {
-			ret = -ENOMEM;
-			goto hdr_err;
-		}
-		break;
-	case SND_SOC_TPLG_CTL_ENUM:
-	case SND_SOC_TPLG_CTL_ENUM_VALUE:
-	case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE:
-	case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT:
-	case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE:
-		kcontrol_type = SND_SOC_TPLG_TYPE_ENUM;	/* enumerated mixer */
-		template.num_kcontrols = le32_to_cpu(w->num_kcontrols);
-		template.kcontrol_news =
-			soc_tplg_dapm_widget_denum_create(tplg,
-			template.num_kcontrols);
-		if (!template.kcontrol_news) {
-			ret = -ENOMEM;
-			goto hdr_err;
-		}
-		break;
-	case SND_SOC_TPLG_CTL_BYTES:
-		kcontrol_type = SND_SOC_TPLG_TYPE_BYTES; /* bytes control */
-		template.num_kcontrols = le32_to_cpu(w->num_kcontrols);
-		template.kcontrol_news =
-			soc_tplg_dapm_widget_dbytes_create(tplg,
-				template.num_kcontrols);
-		if (!template.kcontrol_news) {
-			ret = -ENOMEM;
+	template.num_kcontrols = le32_to_cpu(w->num_kcontrols);
+	kc = devm_kcalloc(tplg->dev, w->num_kcontrols, sizeof(*kc), GFP_KERNEL);
+	if (!kc)
+		goto err;
+
+	for (i = 0; i < w->num_kcontrols; i++) {
+		control_hdr = (struct snd_soc_tplg_ctl_hdr *)tplg->pos;
+		switch (le32_to_cpu(control_hdr->ops.info)) {
+		case SND_SOC_TPLG_CTL_VOLSW:
+		case SND_SOC_TPLG_CTL_STROBE:
+		case SND_SOC_TPLG_CTL_VOLSW_SX:
+		case SND_SOC_TPLG_CTL_VOLSW_XR_SX:
+		case SND_SOC_TPLG_CTL_RANGE:
+		case SND_SOC_TPLG_DAPM_CTL_VOLSW:
+			/* volume mixer */
+			kc[i].index = mixer_count;
+			mixer_count++;
+			ret = soc_tplg_dapm_widget_dmixer_create(tplg, &kc[i]);
+			if (ret < 0)
+				goto hdr_err;
+			break;
+		case SND_SOC_TPLG_CTL_ENUM:
+		case SND_SOC_TPLG_CTL_ENUM_VALUE:
+		case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE:
+		case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT:
+		case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE:
+			/* enumerated mixer */
+			kc[i].index = enum_count;
+			enum_count++;
+			ret = soc_tplg_dapm_widget_denum_create(tplg, &kc[i]);
+			if (ret < 0)
+				goto hdr_err;
+			break;
+		case SND_SOC_TPLG_CTL_BYTES:
+			/* bytes control */
+			kc[i].index = bytes_count;
+			bytes_count++;
+			ret = soc_tplg_dapm_widget_dbytes_create(tplg, &kc[i]);
+			if (ret < 0)
+				goto hdr_err;
+			break;
+		default:
+			dev_err(tplg->dev, "ASoC: invalid widget control type %d:%d:%d\n",
+				control_hdr->ops.get, control_hdr->ops.put,
+				le32_to_cpu(control_hdr->ops.info));
+			ret = -EINVAL;
 			goto hdr_err;
 		}
-		break;
-	default:
-		dev_err(tplg->dev, "ASoC: invalid widget control type %d:%d:%d\n",
-			control_hdr->ops.get, control_hdr->ops.put,
-			le32_to_cpu(control_hdr->ops.info));
-		ret = -EINVAL;
-		goto hdr_err;
 	}
 
+	template.kcontrol_news = kc;
+
 widget:
 	ret = soc_tplg_widget_load(tplg, &template, w);
 	if (ret < 0)
@@ -1567,7 +1541,6 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
 	}
 
 	widget->dobj.type = SND_SOC_DOBJ_WIDGET;
-	widget->dobj.widget.kcontrol_type = kcontrol_type;
 	widget->dobj.ops = tplg->ops;
 	widget->dobj.index = tplg->index;
 	list_add(&widget->dobj.list, &tplg->comp->dobj_list);
@@ -1586,6 +1559,7 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
 	snd_soc_dapm_free_widget(widget);
 hdr_err:
 	kfree(template.sname);
+	kfree(kc);
 err:
 	kfree(template.name);
 	return ret;
-- 
2.24.1

[RFC PATCH 3/3] ASoC: SOF: topology: use individual kcontrol types

[Jaska Uimonen]

Change control creation and deletion to use individual kcontrol types.
This enables the use of multiple different type of controls in single
widget.

Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>
---
 sound/soc/sof/topology.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index 2c9581c6b92d..3855796936bb 100644
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -1063,6 +1063,7 @@ static int sof_control_load_volume(struct snd_soc_component *scomp,
 	scontrol->min_volume_step = le32_to_cpu(mc->min);
 	scontrol->max_volume_step = le32_to_cpu(mc->max);
 	scontrol->num_channels = le32_to_cpu(mc->num_channels);
+	scontrol->control_data->index = kc->index;
 
 	/* set cmd for mixer control */
 	if (le32_to_cpu(mc->max) == 1) {
@@ -1140,7 +1141,7 @@ static int sof_control_load_enum(struct snd_soc_component *scomp,
 
 	scontrol->comp_id = sdev->next_comp_id;
 	scontrol->num_channels = le32_to_cpu(ec->num_channels);
-
+	scontrol->control_data->index = kc->index;
 	scontrol->cmd = SOF_CTRL_CMD_ENUM;
 
 	dev_dbg(scomp->dev, "tplg: load kcontrol index %d chans %d comp_id %d\n",
@@ -1188,6 +1189,7 @@ static int sof_control_load_bytes(struct snd_soc_component *scomp,
 
 	scontrol->comp_id = sdev->next_comp_id;
 	scontrol->cmd = SOF_CTRL_CMD_BINARY;
+	scontrol->control_data->index = kc->index;
 
 	dev_dbg(scomp->dev, "tplg: load kcontrol index %d chans %d\n",
 		scontrol->comp_id, scontrol->num_channels);
@@ -2133,7 +2135,7 @@ static int sof_get_control_data(struct snd_soc_component *scomp,
 	for (i = 0; i < widget->num_kcontrols; i++) {
 		kc = &widget->kcontrol_news[i];
 
-		switch (widget->dobj.widget.kcontrol_type) {
+		switch (kc->kcontrol_type) {
 		case SND_SOC_TPLG_TYPE_MIXER:
 			sm = (struct soc_mixer_control *)kc->private_value;
 			wdata[i].control = sm->dobj.private;
@@ -2148,7 +2150,7 @@ static int sof_get_control_data(struct snd_soc_component *scomp,
 			break;
 		default:
 			dev_err(scomp->dev, "error: unknown kcontrol type %d in widget %s\n",
-				widget->dobj.widget.kcontrol_type,
+				kc->kcontrol_type,
 				widget->name);
 			return -EINVAL;
 		}
@@ -2164,7 +2166,8 @@ static int sof_get_control_data(struct snd_soc_component *scomp,
 			return -EINVAL;
 
 		/* make sure data is valid - data can be updated at runtime */
-		if (wdata[i].pdata->magic != SOF_ABI_MAGIC)
+		if (kc->kcontrol_type == SND_SOC_TPLG_TYPE_BYTES &&
+		    wdata[i].pdata->magic != SOF_ABI_MAGIC)
 			return -EINVAL;
 
 		*size += wdata[i].pdata->size;
@@ -2605,7 +2608,7 @@ static int sof_widget_unload(struct snd_soc_component *scomp,
 	}
 	for (i = 0; i < widget->num_kcontrols; i++) {
 		kc = &widget->kcontrol_news[i];
-		switch (dobj->widget.kcontrol_type) {
+		switch (kc->kcontrol_type) {
 		case SND_SOC_TPLG_TYPE_MIXER:
 			sm = (struct soc_mixer_control *)kc->private_value;
 			scontrol = sm->dobj.private;
-- 
2.24.1