lkml.org 
[lkml]   [2013]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 1/2] ALSA: Added jack detection KControl support
From
Hi Takashi,

On Mon, Aug 12, 2013 at 3:39 AM, Takashi Iwai <tiwai@suse.de> wrote:
> 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@suse.de> wrote:
>> > At Thu, 8 Aug 2013 23:21:55 -0700,
>> > Felipe F. Tonello wrote:
>> >>
>> >> From: "Felipe F. Tonello" <eu@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.

Ok. I will try to follow your instructions.

[...]

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

Got it.

[...]

>>
>> >
>> >
>> >> + }
>> >> }
>> >>
>> >> 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.

Because I tried to maintain this back compatibility. But now I see
that it didn't maintain a lot anyway, because of the jack names.

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

I see that my patches were kind radical. But at least I'm getting
things clearer now.

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

Ok. Nice.

Thanks for all the comments. I appreciate very much.

Felipe Tonello


\
 
 \ /
  Last update: 2013-08-12 21:01    [W:0.082 / U:1.504 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site