From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH v3 1/2] ALSA: jack: create jack kcontrols for every jack input device Date: Sat, 21 Mar 2015 08:46:50 +0100 Message-ID: References: <1426865953-1606-1-git-send-email-yang.jie@intel.com> <1426865953-1606-2-git-send-email-yang.jie@intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id B06E82604F7 for ; Sat, 21 Mar 2015 08:46:54 +0100 (CET) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: "Jie, Yang" Cc: Liam Girdwood , "alsa-devel@alsa-project.org" , "broonie@kernel.org" , "Tanu Kaskinen (tanu.kaskinen@linux.intel.com)" , "Girdwood, Liam R" List-Id: alsa-devel@alsa-project.org 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