Re: [PATCH v3 1/2] ALSA: Added jack detection KControl support

[Felipe Tonello]

Hi Takashi,

On Mon, Aug 12, 2013 at 3:39 AM, Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx> wrote:
>> > At Thu,  8 Aug 2013 23:21:55 -0700,
>> > Felipe F. Tonello wrote:
>> >>
>> >> From: "Felipe F. Tonello" <eu@xxxxxxxxxxxxxxxxx>
>> >>
>> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/