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