Re: [PATCH v3 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Takashi Iwai <tiwai@suse.de>]

At Sat, 21 Mar 2015 02:22:47 +0000,
Jie, Yang wrote:
> 
> > > +{
> > > +	struct snd_kcontrol *kctl;
> > > +	int len = strlen(name);
> > > +
> > > +	down_read(&card->controls_rwsem);
> > > +
> > > +	list_for_each_entry(kctl, &card->controls, list) {
> > > +		if (!strncmp(name, kctl->id.name, len) &&
> > > +		    !strcmp(" Jack", kctl->id.name + len) &&
> > > +		    kctl->id.index == index) {
> > 
> > This doesn't work when ctl elements are with multiple counts.  A single kctl
> > object may contain multiple snd_kcontrol_volatile entries.
> > That's why snd_ctl_find_id() checks like:
> > 
> > 	list_for_each_entry(kctl, &card->controls, list) {
> > 		....
> > 		if (kctl->id.index > id->index)
> > 			continue;
> > 		if (kctl->id.index + kctl->count <= id->index)
> > 			continue;
> > 
> > Also, more importantly, your code checks only the name string.  But the
> > same name string can be assigned for different iface, device and subdevice
> > entries.  You need to compare them as well.
> > 
> > That said, it'd be even simpler just to call snd_ctl_find_id() after creating a ctl
> > id instance instead of writing an own funciton.  It ends up with a bit more
> > stack usage, but it should be still acceptable.
> [Keyon] Here I did the getting index work by imitating function from
> had_jack.c:get_unique_index(), and I also noticed and concerned about
> not checking other snd_ctl_elem_id items.

In the case of hda_jack.c it works like that because it checks the own
list of ctls, not the global list.  The driver knows that there will
be no other "XXX Jack" controls.

> > 
> > Now, a bigger question:
> > 
> > > +			index = get_available_index(card,jack->id);
> > > +			kctl = snd_kctl_jack_new(jack->id, index, card);
> > 
> > So, here you are creating multiple kctl elements with the very same name
> > string.  Did you take a look at the actual usages of snd_jack_new(), especially
> > snd_soc_card_jack_new()?
> [Keyon] yes, I noticed this.  I can also find it at soc/intel/broadwell.c:
> 
>         ret = snd_soc_card_jack_new(rtd->card, "Headset",
>                 SND_JACK_HEADSET | SND_JACK_BTN_0, &broadwell_headset,
>                 broadwell_headset_pins, ARRAY_SIZE(broadwell_headset_pins));
> 
> it will create 2 kctl elements with the same name string "Headset":
> 
> numid=12,iface=CARD,name='Headset Jack'
> numid=13,iface=CARD,name='Headset Jack',index=1
> 
> > 
> > Try like
> > 
> > % git grep -A2 snd_soc_card_jack_new sound/soc/
> > 
> > sound/soc/fsl/imx-es8328.c:             ret = snd_soc_card_jack_new(rtd->card,
> > "Headphone",
> > sound/soc/fsl/imx-es8328.c-                                         SND_JACK_HEADPHONE |
> > SND_JACK_BTN_0,
> > sound/soc/fsl/imx-es8328.c-                                         &headset_jack, NULL, 0);
> > --
> > sound/soc/fsl/wm1133-ev1.c:     snd_soc_card_jack_new(rtd->card,
> > "Headphone", SND_JACK_HEADPHONE,
> > sound/soc/fsl/wm1133-ev1.c-                           &hp_jack, hp_jack_pins,
> > ARRAY_SIZE(hp_jack_pins));
> > sound/soc/fsl/wm1133-ev1.c-     wm8350_hp_jack_detect(codec,
> > WM8350_JDR, &hp_jack, SND_JACK_HEADPHONE);
> > --
> > sound/soc/fsl/wm1133-ev1.c:     snd_soc_card_jack_new(rtd->card,
> > "Microphone",
> > sound/soc/fsl/wm1133-ev1.c-                           SND_JACK_MICROPHONE |
> > SND_JACK_BTN_0, &mic_jack,
> > sound/soc/fsl/wm1133-ev1.c-                           mic_jack_pins,
> > ARRAY_SIZE(mic_jack_pins));
> > .....
> > 
> > As you'll find through the whole output, many calls are combined with
> > SND_JACK_BTN_* bits.  And think again how this will result with your patch.
> > A call like
> > 
> > 	snd_soc_card_jack_new(card, "Headphone",
> > 		SND_JACK_HEADPHONE | SND_JACK_BTN_0 |
> > SND_JACK_BTN_1, ...)
> > 
> > would give three kctls, all "Headphone Jack" with indices 0, 1 and 2.
> > How user-space knows what is for what?
> [Keyon] yes, this is an issue I was thinking about, we need discussing and conclusion
> about that, including comments from user-space, here adding Tanu.
> Here I am thinking about
> 1. do we really need kctls for buttons(SND_JACK_BTN_n)? if no, that is simplest, 
> we can filter out them at this kctl creating stage.
> 2. if yes, then we need an name regenerator here, to generate understandable
> (for user-space) kctl names for buttons.

IMO, we should create kctls with proper unique names.

> > Also, when you look through the grep above, you'll find that many calls
> > already contain "Jack" name suffix.  So, they will result in a name like
> > "Headset Jack Jack".
> [Keyon] do you think we should implement intelligent name regenerator here
> to remove this suffix? 

Yes, "XXX Jack" is easy to check.

> And furthermore, we can check more here, e.g. if the name ("Headset") is 
> matched with the type(type should have SND_JACK_HEADSET bits), or 
> something like that. 

I don't think we need a naming police in the core code.


Takashi