Re: [alsa-devel] [alsa-cvslog] alsa-kmirror: ALSA kernel mirror repository branch, master now at v1.0.21-334-g6739046
At Thu, 12 Nov 2009 10:54:02 +0100 (CET), noreply-git@alsa-project.org wrote:
commit 6739046df36c7adf80c961bcba4870270e66dbf6 Author: Jaroslav Kysela perex@perex.cz AuthorDate: Thu Nov 12 10:15:48 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:48 2009 +0100
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping This patch adds support for dynamically created controls to proc codec file (Control: lines). Signed-off-by: Jaroslav Kysela <perex@perex.cz>commit c45b73bf328cd8ace53cf39994328cf9d6548c4f Author: Jaroslav Kysela perex@perex.cz AuthorDate: Wed Nov 11 13:43:01 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:16 2009 +0100
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment This is an initial patch to show universal control<->NID assigment in proc codec file. The change helps to debug codec related problems. Signed-off-by: Jaroslav Kysela <perex@perex.cz>
I find the second one is the nice hack, but the first (newer) one isn't good since it abuses the subdevice field of the ctl id. It is a part of API/ABI, and if we do any changes the semantics, we should define the changed behavior *beforehand* publicly.
Also, both commits give many warnings via checkpatch.pl... So I postpone the merge so far.
thanks,
Takashi
On Thu, 12 Nov 2009, Takashi Iwai wrote:
At Thu, 12 Nov 2009 10:54:02 +0100 (CET), noreply-git@alsa-project.org wrote:
commit 6739046df36c7adf80c961bcba4870270e66dbf6 Author: Jaroslav Kysela perex@perex.cz AuthorDate: Thu Nov 12 10:15:48 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:48 2009 +0100
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping This patch adds support for dynamically created controls to proc codec file (Control: lines). Signed-off-by: Jaroslav Kysela <perex@perex.cz>commit c45b73bf328cd8ace53cf39994328cf9d6548c4f Author: Jaroslav Kysela perex@perex.cz AuthorDate: Wed Nov 11 13:43:01 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:16 2009 +0100
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment This is an initial patch to show universal control<->NID assigment in proc codec file. The change helps to debug codec related problems. Signed-off-by: Jaroslav Kysela <perex@perex.cz>I find the second one is the nice hack, but the first (newer) one isn't good since it abuses the subdevice field of the ctl id. It is a part of API/ABI, and if we do any changes the semantics, we should define the changed behavior *beforehand* publicly.
The subdevice member IS NOT used (it's always zero) for mixer elements and the value is not PASSED to the midlevel kernel API. Also, the 31. bit checking ensures that subdevice contains right value.
My idea was to keep the changes according the nid tracking at minimum (at least it would be difficult to do major changes for static snd_kcontrol_new arrays).
Also, both commits give many warnings via checkpatch.pl... So I postpone the merge so far.
I'll fix that, if we agree how to go...
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Thu, 12 Nov 2009 12:02:40 +0100 (CET), Jaroslav Kysela wrote:
On Thu, 12 Nov 2009, Takashi Iwai wrote:
At Thu, 12 Nov 2009 10:54:02 +0100 (CET), noreply-git@alsa-project.org wrote:
commit 6739046df36c7adf80c961bcba4870270e66dbf6 Author: Jaroslav Kysela perex@perex.cz AuthorDate: Thu Nov 12 10:15:48 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:48 2009 +0100
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping This patch adds support for dynamically created controls to proc codec file (Control: lines). Signed-off-by: Jaroslav Kysela <perex@perex.cz>commit c45b73bf328cd8ace53cf39994328cf9d6548c4f Author: Jaroslav Kysela perex@perex.cz AuthorDate: Wed Nov 11 13:43:01 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:16 2009 +0100
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment This is an initial patch to show universal control<->NID assigment in proc codec file. The change helps to debug codec related problems. Signed-off-by: Jaroslav Kysela <perex@perex.cz>I find the second one is the nice hack, but the first (newer) one isn't good since it abuses the subdevice field of the ctl id. It is a part of API/ABI, and if we do any changes the semantics, we should define the changed behavior *beforehand* publicly.
The subdevice member IS NOT used (it's always zero) for mixer elements and the value is not PASSED to the midlevel kernel API. Also, the 31. bit checking ensures that subdevice contains right value.
Ah, OK, it's again zero-cleared. Hrm, it's hackish but works. But then please document this somewhere clearly. Otherwise it can lead to misunderstanding pretty easily like me.
My idea was to keep the changes according the nid tracking at minimum (at least it would be difficult to do major changes for static snd_kcontrol_new arrays).
Speaking of patch size: the changes in patch_via.c can be reduced by retrieving nid from the composed private_value. I suppose all (or almost all) callers of via_add_control() passe the value of HDA_COMPOSE_AMP_VAL().
Similarly patch_realtek.c changes can be reduced, I guess.
Also, both commits give many warnings via checkpatch.pl... So I postpone the merge so far.
I'll fix that, if we agree how to go...
As long as the changes are local, I'm fine. But let's try to reduce the changes and a bit more clean up.
thanks,
Takashi
On Thu, 12 Nov 2009, Jaroslav Kysela wrote:
Also, both commits give many warnings via checkpatch.pl... So I postpone the merge so far.
I'll fix that, if we agree how to go...
I recoded both patches:
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=fb8b61ef469a50...
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=aedbf652769307...
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Mon, 16 Nov 2009 10:23:42 +0100 (CET), Jaroslav Kysela wrote:
On Thu, 12 Nov 2009, Jaroslav Kysela wrote:
Also, both commits give many warnings via checkpatch.pl... So I postpone the merge so far.
I'll fix that, if we agree how to go...
I recoded both patches:
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=fb8b61ef469a50...
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=aedbf652769307...
Yeah, this looks a lot better! Applied both now in addition to a minor clean up by defining 1<<31 as a const together with some understandable comments.
Thanks.
Takashi
On Mon, 16 Nov 2009, Takashi Iwai wrote:
At Mon, 16 Nov 2009 10:23:42 +0100 (CET), Jaroslav Kysela wrote:
On Thu, 12 Nov 2009, Jaroslav Kysela wrote:
Also, both commits give many warnings via checkpatch.pl... So I postpone the merge so far.
I'll fix that, if we agree how to go...
I recoded both patches:
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=fb8b61ef469a50...
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=aedbf652769307...
Yeah, this looks a lot better! Applied both now in addition to a minor clean up by defining 1<<31 as a const together with some understandable comments.
Thanks. Please, apply also:
ALSA: hda - move snd_hda_pcm_type_name from hda_codec.h to hda_local.h
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=b1fb471ab578d5...
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Mon, 16 Nov 2009 13:22:56 +0100 (CET), Jaroslav Kysela wrote:
On Mon, 16 Nov 2009, Takashi Iwai wrote:
At Mon, 16 Nov 2009 10:23:42 +0100 (CET), Jaroslav Kysela wrote:
On Thu, 12 Nov 2009, Jaroslav Kysela wrote:
Also, both commits give many warnings via checkpatch.pl... So I postpone the merge so far.
I'll fix that, if we agree how to go...
I recoded both patches:
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=fb8b61ef469a50...
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=aedbf652769307...
Yeah, this looks a lot better! Applied both now in addition to a minor clean up by defining 1<<31 as a const together with some understandable comments.
Thanks. Please, apply also:
ALSA: hda - move snd_hda_pcm_type_name from hda_codec.h to hda_local.h
http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=b1fb471ab578d5...
OK, done. Thanks.
Takashi
At Thu, 12 Nov 2009 11:39:12 +0100, I wrote:
At Thu, 12 Nov 2009 10:54:02 +0100 (CET), noreply-git@alsa-project.org wrote:
commit 6739046df36c7adf80c961bcba4870270e66dbf6 Author: Jaroslav Kysela perex@perex.cz AuthorDate: Thu Nov 12 10:15:48 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:48 2009 +0100
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping This patch adds support for dynamically created controls to proc codec file (Control: lines). Signed-off-by: Jaroslav Kysela <perex@perex.cz>commit c45b73bf328cd8ace53cf39994328cf9d6548c4f Author: Jaroslav Kysela perex@perex.cz AuthorDate: Wed Nov 11 13:43:01 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:16 2009 +0100
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment This is an initial patch to show universal control<->NID assigment in proc codec file. The change helps to debug codec related problems. Signed-off-by: Jaroslav Kysela <perex@perex.cz>I find the second one is the nice hack, but the first (newer) one isn't good since it abuses the subdevice field of the ctl id. It is a part of API/ABI, and if we do any changes the semantics, we should define the changed behavior *beforehand* publicly.
BTW, I don't mean that using subdevice is wrong. Instead: if we use it, let's define the proper definition first. For example, define (1 << 31) as the private (driver-specific) field bit in control.h.
Actually, using the subdevice can make the change of snd_hda_add_ctl easier, too. Then you don't keep nid in another field but just use snd_kcontrol to retrieve the nid.
Of course, we'd need to check the existing apps (at least our own) whether subdevice is really safe to change freely, though.
thanks,
Takashi
On Thu, 12 Nov 2009, Takashi Iwai wrote:
At Thu, 12 Nov 2009 11:39:12 +0100, I wrote:
At Thu, 12 Nov 2009 10:54:02 +0100 (CET), noreply-git@alsa-project.org wrote:
commit 6739046df36c7adf80c961bcba4870270e66dbf6 Author: Jaroslav Kysela perex@perex.cz AuthorDate: Thu Nov 12 10:15:48 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:48 2009 +0100
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping This patch adds support for dynamically created controls to proc codec file (Control: lines). Signed-off-by: Jaroslav Kysela <perex@perex.cz>commit c45b73bf328cd8ace53cf39994328cf9d6548c4f Author: Jaroslav Kysela perex@perex.cz AuthorDate: Wed Nov 11 13:43:01 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:16 2009 +0100
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment This is an initial patch to show universal control<->NID assigment in proc codec file. The change helps to debug codec related problems. Signed-off-by: Jaroslav Kysela <perex@perex.cz>I find the second one is the nice hack, but the first (newer) one isn't good since it abuses the subdevice field of the ctl id. It is a part of API/ABI, and if we do any changes the semantics, we should define the changed behavior *beforehand* publicly.
BTW, I don't mean that using subdevice is wrong. Instead: if we use it, let's define the proper definition first. For example, define (1 << 31) as the private (driver-specific) field bit in control.h.
Actually, using the subdevice can make the change of snd_hda_add_ctl easier, too. Then you don't keep nid in another field but just use snd_kcontrol to retrieve the nid.
Of course, we'd need to check the existing apps (at least our own) whether subdevice is really safe to change freely, though.
I think that you overlooked this code in snd_hda_add_new_ctls():
if (knew->subdevice & (1<<31)) { nid = knew->subdevice & 0xffff; kctl->id.subdevice = 0; } else { nid = 0; }
The subdevice value for nid contents will never leave the HDA code.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Thu, 12 Nov 2009 12:11:17 +0100 (CET), Jaroslav Kysela wrote:
On Thu, 12 Nov 2009, Takashi Iwai wrote:
At Thu, 12 Nov 2009 11:39:12 +0100, I wrote:
At Thu, 12 Nov 2009 10:54:02 +0100 (CET), noreply-git@alsa-project.org wrote:
commit 6739046df36c7adf80c961bcba4870270e66dbf6 Author: Jaroslav Kysela perex@perex.cz AuthorDate: Thu Nov 12 10:15:48 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:48 2009 +0100
ALSA: hda - proc - add support for dynamic controls to mixer<->NID mapping This patch adds support for dynamically created controls to proc codec file (Control: lines). Signed-off-by: Jaroslav Kysela <perex@perex.cz>commit c45b73bf328cd8ace53cf39994328cf9d6548c4f Author: Jaroslav Kysela perex@perex.cz AuthorDate: Wed Nov 11 13:43:01 2009 +0100 Commit: Jaroslav Kysela perex@perex.cz CommitDate: Thu Nov 12 10:51:16 2009 +0100
ALSA: hda - proc - introduce Control: lines to show mixer<->NID assignment This is an initial patch to show universal control<->NID assigment in proc codec file. The change helps to debug codec related problems. Signed-off-by: Jaroslav Kysela <perex@perex.cz>I find the second one is the nice hack, but the first (newer) one isn't good since it abuses the subdevice field of the ctl id. It is a part of API/ABI, and if we do any changes the semantics, we should define the changed behavior *beforehand* publicly.
BTW, I don't mean that using subdevice is wrong. Instead: if we use it, let's define the proper definition first. For example, define (1 << 31) as the private (driver-specific) field bit in control.h.
Actually, using the subdevice can make the change of snd_hda_add_ctl easier, too. Then you don't keep nid in another field but just use snd_kcontrol to retrieve the nid.
Of course, we'd need to check the existing apps (at least our own) whether subdevice is really safe to change freely, though.
I think that you overlooked this code in snd_hda_add_new_ctls():
if (knew->subdevice & (1<<31)) { nid = knew->subdevice & 0xffff; kctl->id.subdevice = 0; } else { nid = 0; }The subdevice value for nid contents will never leave the HDA code.
Yep, I overlooked that.
Takashi
participants (2)
-
Jaroslav Kysela -
Takashi Iwai