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

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(-)

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

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

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