[alsa-devel] [PATCH v3 1/2] ALSA: Added jack detection KControl support
Takashi Iwai
tiwai at suse.de
Mon Aug 12 12:39:10 CEST 2013
At Fri, 9 Aug 2013 10:36:04 -0700,
Felipe Tonello wrote:
>
> Hi Takashi,
>
> On Fri, Aug 9, 2013 at 6:52 AM, Takashi Iwai <tiwai at suse.de> wrote:
> > At Thu, 8 Aug 2013 23:21:55 -0700,
> > Felipe F. Tonello wrote:
> >>
> >> From: "Felipe F. Tonello" <eu at felipetonello.com>
> >>
> >> This patch adds jack support for ALSA KControl.
> >>
> >> This support is necessary since the new kcontrol is used by user-space
> >> daemons, such as PulseAudio(>=2.0), to do jack detection.)
> >>
> >> Also, HDA's CONFIG_SND_HDA_INPUT_JACK is disabled because it causes to use standard
> >> snd_jack_new() to create jacks. This can cause conflict since this codec creates
> >> jack controls directly.
> >>
> >> It makes sure that all codecs using ALSA jack API are updated to use the new
> >> API.
> >
> > Well, while this is a good move forward, this patch is a bit too
> > intrusive as a single patch. It breaks way too many. "Break then
> > fix" is no good attitude, especially when it's just something for
> > future.
>
> I agree with you, but unfortunately I had to do that due the
> non-standard way that jacks are been handled in the kernel and
> reported to user-space.
>
> I believe this is a classic case where we need a well defined kernel
> API to user-space. I'm not necessarily saying about internal kernel
> API/ABI, but the one which is exported to user-space. And to be
> honest, it's kind common to see internal API's been changed.
>
> And that's the reason jack detection only work, out of the box, with
> Intel's HD-audio. Which I think it's pretty bad in the stage we are.
> More and more embedded running PulseAudio and other core user-space
> daemons.
I don't mean about the additional support of kctl jack ctl on ASoC.
It's a damn good thing.
The problem is that you're trying to break the existing stuff, and
it's done in a single shot without describing details what to do and
what breaks. In other words, the proper "process" is missing in your
approach.
>
> [...]
>
> >> diff --git a/include/sound/control.h b/include/sound/control.h
> >> index 5358892..ffeb6b6 100644
> >> --- a/include/sound/control.h
> >> +++ b/include/sound/control.h
> >> @@ -242,6 +242,6 @@ void snd_ctl_sync_vmaster(struct snd_kcontrol *kctl, bool hook_only);
> >> struct snd_kcontrol *
> >> snd_kctl_jack_new(const char *name, int idx, void *private_data);
> >> void snd_kctl_jack_report(struct snd_card *card,
> >> - struct snd_kcontrol *kctl, bool status);
> >> + struct snd_kcontrol *kctl, bool status);
> >
> > Please don't include space or coding-fix changes.
>
> True. I changed back to bool instead of checking out the source file :P
>
> >
> >
> >>
> >> #endif /* __SOUND_CONTROL_H */
> >> diff --git a/include/sound/jack.h b/include/sound/jack.h
> >> index 5891657..0d36f20 100644
> >> --- a/include/sound/jack.h
> >> +++ b/include/sound/jack.h
> >> @@ -26,6 +26,7 @@
> >> #include <sound/core.h>
> >>
> >> struct input_dev;
> >> +struct snd_kcontrol;
> >>
> >> /**
> >> * Jack types which can be reported. These values are used as a
> >> @@ -58,11 +59,12 @@ enum snd_jack_types {
> >>
> >> struct snd_jack {
> >> struct input_dev *input_dev;
> >> + struct snd_kcontrol *kctl[SND_JACK_SWITCH_TYPES]; /* control for each key */
> >> int registered;
> >> int type;
> >> const char *id;
> >> char name[100];
> >> - unsigned int key[6]; /* Keep in sync with definitions above */
> >> + unsigned int key[SND_JACK_SWITCH_TYPES]; /* Keep in sync with definitions above */
> >> void *private_data;
> >> void (*private_free)(struct snd_jack *);
> >> };
> >> @@ -70,7 +72,7 @@ struct snd_jack {
> >> #ifdef CONFIG_SND_JACK
> >>
> >> int snd_jack_new(struct snd_card *card, const char *id, int type,
> >> - struct snd_jack **jack);
> >> + int idx, struct snd_jack **jack);
> >
> > The biggest point here is that the patch changes the API function,
> > from both API and behavioral POV. That's why you need to modify so
> > many patches in a single patch.
> >
> > Try to create a new function doing this and keep the old API and
> > behavior as is. Then adapt / replace with the new API gradually.
> > And in the last step, you can obsolete the former API.
> >
> > Or, at least keep snd_sock_jack_new() as is without additional index
> > but just use idx=0. Then a half of the whole patch can be reduced.
> >
> > Above all, the multiple indices don't work anyway with the snd_jack
> > stuff in the current form. The index was introduced only for kjack,
> > and for HD-audio, snd_jack is provided just for a compatibility
> > reason, thus it doesn't matter much even if the multiple indices don't
> > work with it.
> >
> > That being said, before going further, we need to consider how to
> > handle the input jack stuff with multiple indices.
> >
> >
>
> [...]
>
> >> diff --git a/sound/core/ctljack.c b/sound/core/ctljack.c
> >> index e4b38fb..441158d 100644
> >> --- a/sound/core/ctljack.c
> >> +++ b/sound/core/ctljack.c
> >> @@ -14,7 +14,7 @@
> >> #include <sound/core.h>
> >> #include <sound/control.h>
> >>
> >> -#define jack_detect_kctl_info snd_ctl_boolean_mono_info
> >> +#define jack_detect_kctl_info snd_ctl_boolean_mono_info
> >>
> >> static int jack_detect_kctl_get(struct snd_kcontrol *kcontrol,
> >> struct snd_ctl_elem_value *ucontrol)
> >> @@ -38,6 +38,7 @@ snd_kctl_jack_new(const char *name, int idx, void *private_data)
> >> kctl = snd_ctl_new1(&jack_detect_kctl, private_data);
> >> if (!kctl)
> >> return NULL;
> >> +
> >> snprintf(kctl->id.name, sizeof(kctl->id.name), "%s Jack", name);
> >> kctl->id.index = idx;
> >> kctl->private_value = 0;
> >
> > No unnecessary space changes please.
>
> Ok.
>
> >
> >
> >> diff --git a/sound/core/jack.c b/sound/core/jack.c
> >> index b35fe73..128154b 100644
> >> --- a/sound/core/jack.c
> >> +++ b/sound/core/jack.c
>
> [...]
>
> >>
> >> +const char * jack_get_name_by_key(const char *name, int key)
> >> +{
> >> + char *jack_name;
> >> + size_t jack_name_size;
> >> + const char *key_name;
> >> +
> >> + switch(key) {
> >> + case SW_HEADPHONE_INSERT: key_name = "Headphone"; break;
> >> + case SW_MICROPHONE_INSERT: key_name = "Mic"; break;
> >> + case SW_LINEOUT_INSERT: key_name = "Line Out"; break;
> >> + case SW_JACK_PHYSICAL_INSERT: key_name = "Mechanical"; break;
> >> + case SW_VIDEOOUT_INSERT: key_name = "Video Out"; break;
> >> + case SW_LINEIN_INSERT: key_name = "Line In"; break;
> >> + default: key_name = "Unknown";
> >> + }
> >> +
> >> + /* Avoid duplicate name in KControl */
> >> + if (strcmp(name, key_name) != 0) {
> >
> > Better to check via strstr() or such.
> > The name can be like "Front Headphone".
>
> Ok.
>
> >
> >> + /* allocate necessary memory space only */
> >> + jack_name_size = strlen(name) + strlen(key_name) + 4;
> >> + jack_name = kmalloc(jack_name_size, GFP_KERNEL);
> >
> > This leads to a memory leak. Make this function to put a string on
> > the given buffer instead of returning a string pointer.
> >
> >> +
> >> + snprintf(jack_name, jack_name_size, "%s (%s)", name, key_name);
> >> + } else {
> >> + jack_name = (char *)name;
> >> + }
> >> +
> >> + return jack_name;
> >> +}
> >> +
> >> static int snd_jack_dev_register(struct snd_device *device)
> >> {
> >> struct snd_jack *jack = device->device_data;
> >> struct snd_card *card = device->card;
> >> int err, i;
> >>
> >> - snprintf(jack->name, sizeof(jack->name), "%s %s",
> >> + snprintf(jack->name, sizeof(jack->name), "%s %s Jack",
> >> card->shortname, jack->id);
> >
> > This breaks the compatibility with the existing code.
> > You must not change the name of the existing input jack element.
> > Some drivers create "Headphone" and some do "Headphone Jack", yes.
> > It's bad, but too late to change.
>
> Why? Can't we just fix those names in those codecs?
No. The name string is the only identifier in this case, so if you
change it, it leads to a regression.
> As You see in my
> second patch, only few of them are using weird names.
You broke the things and fixed at next. This is a bad attitude, as
already mentioned.
> I know that this will potentially break user-space, but the trade off
> is not to standardize the Jack API. What do you think?
No. You cannot break. This is a general golden rule.
The only exception would be if the user-space side will adapt the
change accordingly together with the kernel change.
>
> [...]
>
> >> @@ -232,10 +305,15 @@ void snd_jack_report(struct snd_jack *jack, int status)
> >>
> >> for (i = 0; i < ARRAY_SIZE(jack_switch_types); i++) {
> >> int testbit = 1 << i;
> >> - if (jack->type & testbit)
> >> + if (jack->type & testbit) {
> >> input_report_switch(jack->input_dev,
> >> jack_switch_types[i],
> >> status & testbit);
> >> +
> >> + /* Update ALSA KControl interface */
> >> + snd_kctl_jack_report((struct snd_card *)jack->kctl[i]->private_data,
> >> + jack->kctl[i], status & testbit);
> >
> > Better to do a NULL check.
> > In this patch, snd_ctl_add() error isn't handled as a fatal error,
> > thus jack->kctl[i] can be NULL.
>
> True.
>
> >
> >
> >> + }
> >> }
> >>
> >> input_sync(jack->input_dev);
> >> diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
> >> index 59c5e9c..561abc7 100644
> >> --- a/sound/pci/hda/Kconfig
> >> +++ b/sound/pci/hda/Kconfig
> >> @@ -65,14 +65,6 @@ config SND_HDA_INPUT_BEEP_MODE
> >> Set 1 to always enable the digital beep interface for HD-audio by
> >> default.
> >>
> >> -config SND_HDA_INPUT_JACK
> >> - bool "Support jack plugging notification via input layer"
> >> - depends on INPUT=y || INPUT=SND
> >> - select SND_JACK
> >> - help
> >> - Say Y here to enable the jack plugging notification via
> >> - input layer.
> >> -
> >
> > I understand why you remove this Kconfig. But by this action, you
> > introduced an obvious regression, i.e. the input jack control is no
> > longer created for HD-audio.
>
> I did this just to see what some HD-audio developers whould say.
> Because what I would like to see is HD-audio codec also using snd_jack
> and not export those kct jack functions anymore.
Even if you would like so, you don't rule the world yet, so it can't
be a reason to disable the whole thing out of sudden :)
> BTW, who uses these input events anyway? Woudn't be better just to
> have standard way (ALSA KControl) to report it?
Felipe, why wouldn't you drop the whole input jack code for ASoC even
after you implement kctl jack controls, then? The same logic can be
applied to HD-audio input jack controls, too.
But, don't get me wrong: I'm not against the action itself, the
removal of input jack support in HD-audio. I myself did propose this
once ago. Again, what's missing in your approach is the proper
process.
An easier (or lazier) way to manage this problem would be:
- Think whether removal of input-jack support is really needed for
HD-audio;
for example, if you integrate snd_jack stuff to support both
input-jack and kctl jack, HD-audio driver can use it solely instead
of calling snd_kctl stuff. Then both input and kctl jacks will be
supported automagically.
- If it's still easier to handle kctl jacks without input jacks in
HD-audio, propose the removal at first on the list, get the general
consensus. Then put the removal patch in your series at first.
- Try to keep snd_jack_new() intact but create a new API function for
creating both input and kctl jacks. This would accept two different
name strings, one for input jack and one for kctl, with an
additional index, if needed. The different names are needed not to
break the things.
- Replace snd_soc_jack_new() with the new function, but you don't have
to add the index argument yet at this point. The handling of
multiple input-jack instances with indices isn't defined yet, so the
simplest solution would be just to skip the multiple indices. It
should be good enough for ASoC.
- Replace snd_jack_new() in the rest.
- If wanted, obsolete snd_jack_new(), but keep only the new API.
Takashi
More information about the Alsa-devel
mailing list