Re: [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls

[David Henningsson <david.henningsson@canonical.com>]

On 2015-04-08 09:59, Jie, Yang wrote:
>> -----Original Message-----
>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>> Sent: Wednesday, April 08, 2015 3:21 PM
>> To: Takashi Iwai; Jie, Yang
>> Cc: alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R
>> Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
>>
>>
>>
>> On 2015-04-07 18:06, Takashi Iwai wrote:
>>> This would work, yes.  But, I have some uneasy feeling, something not
>>> well digested...
>>>
>>> Ideally, we want a single API for representing both input and kctl
>>> jacks.
>>
>> Maybe this is somewhat my fault for steering Yang in that direction. But the
>> requirements are somewhat different.
>>
>> HDA has the phantom jacks, and the exact naming for each kctl requirements.
>> ASoC has the combination/button requirements, i e one jack can represent
>> more than one kctl.
>>
>> The phantom jack requirement means that the snd_kctl_jack_new API
>> cannot be removed straight away; we could move it to be internal to HDA
>> (it's not much code anyway), but I don't see a need for that.
>>
>> But the HDA code can be moved around to look like this:
>>
>>      if (phantom_jack) {
>>         snd_kctl_jack_new();
>
> Let me add line here to clarify:
> +        snd_hda_ctl_add();
>
>>      }
>>      else {
>>         snd_jack_new();
>>         snd_jack_add_new_kctls();
>
> I thought of this at the beginning, as you talk below, it looks more like what
> we did for ASoC case.
>
> What I concern here is that it may make input jack kctl looks like a little
> different with before(I am not sure, maybe you guys are more clearly about
> what will occurs without calling snd_ctl_add()-->snd_array_new()...)?

I'm not sure I understand this question. Maybe the answers below help 
answering it?

>>      }
>>
>> Now the HDA looks more like the ASoC variant. Yang, what do you think
>> about that? That would make the API simpler, wouldn't it?
>
> Here repeat what I stated in another reply:
>
> For jack creating, we use the same API -- snd_jack_new();
>
> For kctl creating, yes, we use different APIs:
> snd_jack_kctl_new() for input jacks(HDA),
> snd_jack_add_kctls() for kctl jacks(ASoC).
>
> There are 2 reasons that I made them different:
> 1. a. for HDA phantom jack, in the old/exist logic, __snd_hda_jack_add_kctl()
> will also call snd_kctl_jack_new() and snd_hda_ctl_add(), it will create kctls
> and add them to card(also assigning some arrays, they are different with calling
> snd_ctl_add() only, which is what we will do for ASoC kctl adding);

Actually, now that I look at snd_hda_ctl_add, I don't know why we need 
to call it for HDA jacks. There does not seem to be anything relevant 
for HDA jacks there. I think we can just call snd_ctl_add for HDA jacks too.

With your refactoring, all we need to remember is to remove the jacks (i 
e both the input device and the kctls) in both snd_hda_codec_reset and 
snd_hda_codec_free, and I suspect this is already taken care of?

>      b. for HDA input/phantom jack, all of those occurs before calling
> snd_jack_new().

My point is that it would be possible to move the HDA code around so 
that snd_jack_new is called first. And with Takashi's suggestion of a 
"create_input_dev" flag, it would be called for both real and phantom jacks.

> So, we have to split jack new and kctl new functions here, because for HDA
> phantom/input jack and ASoC kctl jack, they are quite different here.
>
> 2. we may need generate kctl name for ASoC kctls(snd_jack_kctl_name_gen(),
> removing suffix " Jack" as you proposed before, or everything needed after);
> but for HDA input jack kctls, the naming has been finished before calling
> __snd_hda_jack_add_kctl(), we should not change them anymore.

snd_jack_kctl_name_gen seems only to remove a " Jack" suffix. I think we 
can safely call it for both ASoC and HDA jacks. If a HDA jack ends up 
being labelled something with " Jack Jack", it's a bug anyway.

(Side note: but you might want to fix snd_jack_kctl_name_gen to make 
strange names like "My Jacket Computer" not be cropped to "My Jacket Com")

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic