[PATCH 0/6] ALSA: hda/ca0132: Various cleanups and fixes

[PATCH 0/6] ALSA: hda/ca0132: Various cleanups and fixes

[Alastair Bridgewater]

The ca0132 codec support is fairly crufty, and the new R3Di and SBZ
support hasn't exactly improved that situation.  This patch series
tries to clean up a small amount of longstanding cruft, reverse some
behavioral changes to other systems made by the recent R3Di/SBZ
patches, and make the DMic work properly on the Alienware M17x R4.

For two of these patches (the QUIRK_ALIENWARE patch and the DMic
patch), it may be that the correct behavior is the exact OPPOSITE of
what the patch does as far as quirk testing goes (that is, it should
apply globally, rather than limited to some specific quirk or set of
quirks), but I have no way to make such a determination at this time,
so erring on the side of the status quo ante seems appropriate.

Alastair Bridgewater (6):
  ALSA: hda/ca0132: Delete pointless assignments to struct auto_pin_cfg
    fields
  ALSA: hda/ca0132: Delete redundant UNSOL event requests
  ALSA: hda/ca0132: Restore behavior of QUIRK_ALIENWARE
  ALSA: hda/ca0132: Don't test for QUIRK_NONE
  ALSA: hda/ca0132: Restore PCM Analog Mic-In2
  ALSA: hda/ca0132: Fix DMic data rate for Alienware M17x R4

 sound/pci/hda/patch_ca0132.c | 62 ++++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 40 deletions(-)

-- 
2.16.1

[PATCH 1/6] ALSA: hda/ca0132: Delete pointless assignments to struct auto_pin_cfg fields

[Alastair Bridgewater]

ca0132_config() was setting some values in the auto_pin_cfg for
the codec... but it is called prior to snd_hda_parse_pin_defcfg(),
which does a memset() to clear the entire structure as one of its
first actions, making the entire exercise pointless.

Kill all use of struct auto_pin_cfg from ca0132_config().

Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index 292e2c592c17..e03461aab68c 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -7343,7 +7343,6 @@ static const struct hda_codec_ops ca0132_patch_ops = {
 static void ca0132_config(struct hda_codec *codec)
 {
 	struct ca0132_spec *spec = codec->spec;
-	struct auto_pin_cfg *cfg = &spec->autocfg;
 
 	spec->dacs[0] = 0x2;
 	spec->dacs[1] = 0x3;
@@ -7405,12 +7404,7 @@ static void ca0132_config(struct hda_codec *codec)
 		/* SPDIF I/O */
 		spec->dig_out = 0x05;
 		spec->multiout.dig_out_nid = spec->dig_out;
-		cfg->dig_out_pins[0] = 0x0c;
-		cfg->dig_outs = 1;
-		cfg->dig_out_type[0] = HDA_PCM_TYPE_SPDIF;
 		spec->dig_in = 0x09;
-		cfg->dig_in_pin = 0x0e;
-		cfg->dig_in_type = HDA_PCM_TYPE_SPDIF;
 		break;
 	case QUIRK_R3DI:
 		codec_dbg(codec, "%s: QUIRK_R3DI applied.\n", __func__);
@@ -7438,9 +7432,6 @@ static void ca0132_config(struct hda_codec *codec)
 		/* SPDIF I/O */
 		spec->dig_out = 0x05;
 		spec->multiout.dig_out_nid = spec->dig_out;
-		cfg->dig_out_pins[0] = 0x0c;
-		cfg->dig_outs = 1;
-		cfg->dig_out_type[0] = HDA_PCM_TYPE_SPDIF;
 		break;
 	default:
 		spec->num_outputs = 2;
@@ -7463,12 +7454,7 @@ static void ca0132_config(struct hda_codec *codec)
 		/* SPDIF I/O */
 		spec->dig_out = 0x05;
 		spec->multiout.dig_out_nid = spec->dig_out;
-		cfg->dig_out_pins[0] = 0x0c;
-		cfg->dig_outs = 1;
-		cfg->dig_out_type[0] = HDA_PCM_TYPE_SPDIF;
 		spec->dig_in = 0x09;
-		cfg->dig_in_pin = 0x0e;
-		cfg->dig_in_type = HDA_PCM_TYPE_SPDIF;
 		break;
 	}
 }
-- 
2.16.1

[PATCH 2/6] ALSA: hda/ca0132: Delete redundant UNSOL event requests

[Alastair Bridgewater]

During ca0132_init(), ca0132_init_unsol() is run before the
spec->spec_init_verbs are written.  ca0132_init_unsol() calls
snd_hda_jack_detect_enable_callback(), which requests UNSOL events
for three or four nodes, two of which were also (redundantly)
requested by spec_init_verbs.

Kill the redundant AC_VERB_SET_UNSOLICITED_ENABLE verbs.

Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index e03461aab68c..bdd626bde267 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -7462,7 +7462,7 @@ static void ca0132_config(struct hda_codec *codec)
 static int ca0132_prepare_verbs(struct hda_codec *codec)
 {
 /* Verbs + terminator (an empty element) */
-#define NUM_SPEC_VERBS 4
+#define NUM_SPEC_VERBS 2
 	struct ca0132_spec *spec = codec->spec;
 
 	spec->chip_init_verbs = ca0132_init_verbs0;
@@ -7472,34 +7472,24 @@ static int ca0132_prepare_verbs(struct hda_codec *codec)
 	if (!spec->spec_init_verbs)
 		return -ENOMEM;
 
-	/* HP jack autodetection */
-	spec->spec_init_verbs[0].nid = spec->unsol_tag_hp;
-	spec->spec_init_verbs[0].param = AC_VERB_SET_UNSOLICITED_ENABLE;
-	spec->spec_init_verbs[0].verb = AC_USRSP_EN | spec->unsol_tag_hp;
-
-	/* MIC1 jack autodetection */
-	spec->spec_init_verbs[1].nid = spec->unsol_tag_amic1;
-	spec->spec_init_verbs[1].param = AC_VERB_SET_UNSOLICITED_ENABLE;
-	spec->spec_init_verbs[1].verb = AC_USRSP_EN | spec->unsol_tag_amic1;
-
 	/* config EAPD */
-	spec->spec_init_verbs[2].nid = 0x0b;
-	spec->spec_init_verbs[2].param = 0x78D;
-	spec->spec_init_verbs[2].verb = 0x00;
+	spec->spec_init_verbs[0].nid = 0x0b;
+	spec->spec_init_verbs[0].param = 0x78D;
+	spec->spec_init_verbs[0].verb = 0x00;
 
 	/* Previously commented configuration */
 	/*
-	spec->spec_init_verbs[3].nid = 0x0b;
-	spec->spec_init_verbs[3].param = AC_VERB_SET_EAPD_BTLENABLE;
+	spec->spec_init_verbs[2].nid = 0x0b;
+	spec->spec_init_verbs[2].param = AC_VERB_SET_EAPD_BTLENABLE;
+	spec->spec_init_verbs[2].verb = 0x02;
+
+	spec->spec_init_verbs[3].nid = 0x10;
+	spec->spec_init_verbs[3].param = 0x78D;
 	spec->spec_init_verbs[3].verb = 0x02;
 
 	spec->spec_init_verbs[4].nid = 0x10;
-	spec->spec_init_verbs[4].param = 0x78D;
+	spec->spec_init_verbs[4].param = AC_VERB_SET_EAPD_BTLENABLE;
 	spec->spec_init_verbs[4].verb = 0x02;
-
-	spec->spec_init_verbs[5].nid = 0x10;
-	spec->spec_init_verbs[5].param = AC_VERB_SET_EAPD_BTLENABLE;
-	spec->spec_init_verbs[5].verb = 0x02;
 	*/
 
 	/* Terminator: spec->spec_init_verbs[NUM_SPEC_VERBS-1] */
-- 
2.16.1

[PATCH 3/6] ALSA: hda/ca0132: Restore behavior of QUIRK_ALIENWARE

[Alastair Bridgewater]

Commit e93ac30a32a6ba7ac3b4b2a4379af1dadb91e505 (ALSA: HDA/ca0132:
add extra init functions for r3di + sbz) introduced an extra
initialization function that was improperly guarded, taking effect
on systems with QUIRK_ALIENWARE, even though such systems were
supposedly not affected.

It may be that this piece of initialization should be done for all
systems, but that's not a call that I can make.

Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index bdd626bde267..ca7375e48138 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -7223,7 +7223,7 @@ static int ca0132_init(struct hda_codec *codec)
 
 	snd_hda_sequence_write(codec, spec->base_init_verbs);
 
-	if (spec->quirk != QUIRK_NONE)
+	if (spec->use_alt_functions)
 		ca0132_alt_init(codec);
 
 	ca0132_download_dsp(codec);
-- 
2.16.1

Re: [PATCH 3/6] ALSA: hda/ca0132: Restore behavior of QUIRK_ALIENWARE

[Connor McAdams]

I agree with this one too, must have been one I missed. I added
use_alt_functions explicitly to get away from using spec->quirk !=
QUIRK_NONE.

On Fri, Jun 15, 2018 at 9:56 PM, Alastair Bridgewater
<alastair.bridgewater@gmail.com> wrote:
> Commit e93ac30a32a6ba7ac3b4b2a4379af1dadb91e505 (ALSA: HDA/ca0132:
> add extra init functions for r3di + sbz) introduced an extra
> initialization function that was improperly guarded, taking effect
> on systems with QUIRK_ALIENWARE, even though such systems were
> supposedly not affected.
>
> It may be that this piece of initialization should be done for all
> systems, but that's not a call that I can make.
>
> Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>
>
> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> index bdd626bde267..ca7375e48138 100644
> --- a/sound/pci/hda/patch_ca0132.c
> +++ b/sound/pci/hda/patch_ca0132.c
> @@ -7223,7 +7223,7 @@ static int ca0132_init(struct hda_codec *codec)
>
>         snd_hda_sequence_write(codec, spec->base_init_verbs);
>
> -       if (spec->quirk != QUIRK_NONE)
> +       if (spec->use_alt_functions)
>                 ca0132_alt_init(codec);
>
>         ca0132_download_dsp(codec);
> --
> 2.16.1
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/6] ALSA: hda/ca0132: Restore behavior of QUIRK_ALIENWARE

[Connor McAdams]

Going through the code again today, I realized why I did this. I don't
believe the Chromebook will ever go into Windows, so I don't think I
have to set the 0x797 and 0x798 audio volume verbs to 0x00 on it. I
did this so that the Alienware laptops, which probably have booted
into Windows, would have those values set to 0 if they hadn't been
already.

On Fri, Jun 15, 2018 at 9:56 PM, Alastair Bridgewater
<alastair.bridgewater@gmail.com> wrote:
> Commit e93ac30a32a6ba7ac3b4b2a4379af1dadb91e505 (ALSA: HDA/ca0132:
> add extra init functions for r3di + sbz) introduced an extra
> initialization function that was improperly guarded, taking effect
> on systems with QUIRK_ALIENWARE, even though such systems were
> supposedly not affected.
>
> It may be that this piece of initialization should be done for all
> systems, but that's not a call that I can make.
>
> Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>
>
> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> index bdd626bde267..ca7375e48138 100644
> --- a/sound/pci/hda/patch_ca0132.c
> +++ b/sound/pci/hda/patch_ca0132.c
> @@ -7223,7 +7223,7 @@ static int ca0132_init(struct hda_codec *codec)
>
>         snd_hda_sequence_write(codec, spec->base_init_verbs);
>
> -       if (spec->quirk != QUIRK_NONE)
> +       if (spec->use_alt_functions)
>                 ca0132_alt_init(codec);
>
>         ca0132_download_dsp(codec);
> --
> 2.16.1
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/6] ALSA: hda/ca0132: Restore behavior of QUIRK_ALIENWARE

[Alastair Bridgewater]

That makes a certain amount of sense, but there are a few things that
would invalidate that reasoning.  First, the contrived scenario:
Running Linux on the Chromebook with the sound card routed to a Qemu
VM running Some Other Operating System.  Second, there's no quirk
matching for the Chromebook, which means that every unknown card (and
these unknown cards would presumably have the opportunity to be
dual-boot windows devices) would be denied the benefit of having the
extra volume bits cleared.  Third, if Linux starts to use those extra
bits, and someone is dual-booting between kernel versions, we get the
same effect.

At this point, unless there is reason to expect that clearing these
volume parameters could be harmful, I think that doing so globally is
the right thing.  I can prep a patch for that in a day or so.

On Sat, Jun 16, 2018 at 11:37 AM, Connor McAdams <conmanx360@gmail.com>
wrote:

> Going through the code again today, I realized why I did this. I don't
> believe the Chromebook will ever go into Windows, so I don't think I
> have to set the 0x797 and 0x798 audio volume verbs to 0x00 on it. I
> did this so that the Alienware laptops, which probably have booted
> into Windows, would have those values set to 0 if they hadn't been
> already.
>
> On Fri, Jun 15, 2018 at 9:56 PM, Alastair Bridgewater
> <alastair.bridgewater@gmail.com> wrote:
> > Commit e93ac30a32a6ba7ac3b4b2a4379af1dadb91e505 (ALSA: HDA/ca0132:
> > add extra init functions for r3di + sbz) introduced an extra
> > initialization function that was improperly guarded, taking effect
> > on systems with QUIRK_ALIENWARE, even though such systems were
> > supposedly not affected.
> >
> > It may be that this piece of initialization should be done for all
> > systems, but that's not a call that I can make.
> >
> > Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>
> >
> > diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> > index bdd626bde267..ca7375e48138 100644
> > --- a/sound/pci/hda/patch_ca0132.c
> > +++ b/sound/pci/hda/patch_ca0132.c
> > @@ -7223,7 +7223,7 @@ static int ca0132_init(struct hda_codec *codec)
> >
> >         snd_hda_sequence_write(codec, spec->base_init_verbs);
> >
> > -       if (spec->quirk != QUIRK_NONE)
> > +       if (spec->use_alt_functions)
> >                 ca0132_alt_init(codec);
> >
> >         ca0132_download_dsp(codec);
> > --
> > 2.16.1
> >
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH 3/6] ALSA: hda/ca0132: Restore behavior of QUIRK_ALIENWARE

[Connor McAdams]

Well, here's how I've been treating this. The ca0132 driver was made
for the Chromebook's ca0132 chip. So in my mind, QUIRK_NONE is the
Chromebook, and any added support for other cards is something that
has been tacked on top of it's base. I don't know of any Core3D based
cards that work just fine by going through the default setup without
some sort of QUIRK related fixes. I don't envision ALSA getting
floating point decibel volume, but I guess if that were to happen,
maybe this could become an issue.

In my mind, there is probably a reason Creative didn't make any
reference to that verb on the driver that they, the people with the
actual documentation, wrote. I try not to make any changes to things
that I have no real reason to make a change to. It worked fine before
without it, so I see no reason to begin doing that. But, if you
believe that it's a necessary thing to do, then I guess you can. I
just don't like touching things that I don't need to.

On Sat, Jun 16, 2018 at 8:56 PM, Alastair Bridgewater
<alastair.bridgewater@gmail.com> wrote:
> That makes a certain amount of sense, but there are a few things that
> would invalidate that reasoning.  First, the contrived scenario:
> Running Linux on the Chromebook with the sound card routed to a Qemu
> VM running Some Other Operating System.  Second, there's no quirk
> matching for the Chromebook, which means that every unknown card (and
> these unknown cards would presumably have the opportunity to be
> dual-boot windows devices) would be denied the benefit of having the
> extra volume bits cleared.  Third, if Linux starts to use those extra
> bits, and someone is dual-booting between kernel versions, we get the
> same effect.
>
> At this point, unless there is reason to expect that clearing these
> volume parameters could be harmful, I think that doing so globally is
> the right thing.  I can prep a patch for that in a day or so.
>
> On Sat, Jun 16, 2018 at 11:37 AM, Connor McAdams <conmanx360@gmail.com>
> wrote:
>>
>> Going through the code again today, I realized why I did this. I don't
>> believe the Chromebook will ever go into Windows, so I don't think I
>> have to set the 0x797 and 0x798 audio volume verbs to 0x00 on it. I
>> did this so that the Alienware laptops, which probably have booted
>> into Windows, would have those values set to 0 if they hadn't been
>> already.
>>
>> On Fri, Jun 15, 2018 at 9:56 PM, Alastair Bridgewater
>> <alastair.bridgewater@gmail.com> wrote:
>> > Commit e93ac30a32a6ba7ac3b4b2a4379af1dadb91e505 (ALSA: HDA/ca0132:
>> > add extra init functions for r3di + sbz) introduced an extra
>> > initialization function that was improperly guarded, taking effect
>> > on systems with QUIRK_ALIENWARE, even though such systems were
>> > supposedly not affected.
>> >
>> > It may be that this piece of initialization should be done for all
>> > systems, but that's not a call that I can make.
>> >
>> > Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>
>> >
>> > diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
>> > index bdd626bde267..ca7375e48138 100644
>> > --- a/sound/pci/hda/patch_ca0132.c
>> > +++ b/sound/pci/hda/patch_ca0132.c
>> > @@ -7223,7 +7223,7 @@ static int ca0132_init(struct hda_codec *codec)
>> >
>> >         snd_hda_sequence_write(codec, spec->base_init_verbs);
>> >
>> > -       if (spec->quirk != QUIRK_NONE)
>> > +       if (spec->use_alt_functions)
>> >                 ca0132_alt_init(codec);
>> >
>> >         ca0132_download_dsp(codec);
>> > --
>> > 2.16.1
>> >
>> > _______________________________________________
>> > Alsa-devel mailing list
>> > Alsa-devel@alsa-project.org
>> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
>

[PATCH 4/6] ALSA: hda/ca0132: Don't test for QUIRK_NONE

[Alastair Bridgewater]

QUIRK_NONE is, quite explicitly, the default case.  The entire
point of a quirks system is to allow "programming by difference"
from a given base case, which requires that merely defining a new
quirk for some piece of hardware should not change the behavior of
the driver for that hardware.  In turn, this means that testing
for QUIRK_NONE explicitly is a violation of that implicit contract.

Change a test for QUIRK_NONE and QUIRK_ALIENWARE to default, and
add a test for QUIRK_SBZ to disable the default behavior in that
instance.

Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index ca7375e48138..71487b82eef5 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -7237,8 +7237,9 @@ static int ca0132_init(struct hda_codec *codec)
 	case QUIRK_R3DI:
 		r3di_setup_defaults(codec);
 		break;
-	case QUIRK_NONE:
-	case QUIRK_ALIENWARE:
+	case QUIRK_SBZ:
+		break;
+	default:
 		ca0132_setup_defaults(codec);
 		ca0132_init_analog_mic2(codec);
 		ca0132_init_dmic(codec);
-- 
2.16.1

Re: [PATCH 4/6] ALSA: hda/ca0132: Don't test for QUIRK_NONE

[Connor McAdams]

Good call. Please understand that I'm fairly new to all of this, so
mistakes like this are more due to being ignorant than anything else.
I was afraid certain things may end up broken by my changes, and I
hope it didn't inconvenience you.

On Fri, Jun 15, 2018 at 9:56 PM, Alastair Bridgewater
<alastair.bridgewater@gmail.com> wrote:
> QUIRK_NONE is, quite explicitly, the default case.  The entire
> point of a quirks system is to allow "programming by difference"
> from a given base case, which requires that merely defining a new
> quirk for some piece of hardware should not change the behavior of
> the driver for that hardware.  In turn, this means that testing
> for QUIRK_NONE explicitly is a violation of that implicit contract.
>
> Change a test for QUIRK_NONE and QUIRK_ALIENWARE to default, and
> add a test for QUIRK_SBZ to disable the default behavior in that
> instance.
>
> Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>
>
> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> index ca7375e48138..71487b82eef5 100644
> --- a/sound/pci/hda/patch_ca0132.c
> +++ b/sound/pci/hda/patch_ca0132.c
> @@ -7237,8 +7237,9 @@ static int ca0132_init(struct hda_codec *codec)
>         case QUIRK_R3DI:
>                 r3di_setup_defaults(codec);
>                 break;
> -       case QUIRK_NONE:
> -       case QUIRK_ALIENWARE:
> +       case QUIRK_SBZ:
> +               break;
> +       default:
>                 ca0132_setup_defaults(codec);
>                 ca0132_init_analog_mic2(codec);
>                 ca0132_init_dmic(codec);
> --
> 2.16.1
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 4/6] ALSA: hda/ca0132: Don't test for QUIRK_NONE

[Alastair Bridgewater]

My inconvenience from the R3Di/SBZ changes has been minor at worst.

Actually, the timing on this has been very good.  Finding the R3Di
patches and QemuHDADump two weeks ago, just as I was starting to look
again into trying to improve sound support on my M17x R4 was a
pleasant surprise.  Yes, there are rough edges, but they're the sort
of thing that should get caught by careful code review or from testing
various -rc kernels.  And you'll get better at this if you keep at it.

Speaking of rough edges, QUIRK_SBZ and QUIRK_R3DI have code paths that
use both ca0132_alt_select_in() (from ca0132_init() and the input
source control) and ca0132_select_mic() (from amic_callback(), tied to
a jack detect).  I'm fairly certain that this is wrong.

On Fri, Jun 15, 2018 at 10:20 PM, Connor McAdams <conmanx360@gmail.com>
wrote:

> Good call. Please understand that I'm fairly new to all of this, so
> mistakes like this are more due to being ignorant than anything else.
> I was afraid certain things may end up broken by my changes, and I
> hope it didn't inconvenience you.
>
> On Fri, Jun 15, 2018 at 9:56 PM, Alastair Bridgewater
> <alastair.bridgewater@gmail.com> wrote:
> > QUIRK_NONE is, quite explicitly, the default case.  The entire
> > point of a quirks system is to allow "programming by difference"
> > from a given base case, which requires that merely defining a new
> > quirk for some piece of hardware should not change the behavior of
> > the driver for that hardware.  In turn, this means that testing
> > for QUIRK_NONE explicitly is a violation of that implicit contract.
> >
> > Change a test for QUIRK_NONE and QUIRK_ALIENWARE to default, and
> > add a test for QUIRK_SBZ to disable the default behavior in that
> > instance.
> >
> > Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>
> >
> > diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> > index ca7375e48138..71487b82eef5 100644
> > --- a/sound/pci/hda/patch_ca0132.c
> > +++ b/sound/pci/hda/patch_ca0132.c
> > @@ -7237,8 +7237,9 @@ static int ca0132_init(struct hda_codec *codec)
> >         case QUIRK_R3DI:
> >                 r3di_setup_defaults(codec);
> >                 break;
> > -       case QUIRK_NONE:
> > -       case QUIRK_ALIENWARE:
> > +       case QUIRK_SBZ:
> > +               break;
> > +       default:
> >                 ca0132_setup_defaults(codec);
> >                 ca0132_init_analog_mic2(codec);
> >                 ca0132_init_dmic(codec);
> > --
> > 2.16.1
> >
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH 4/6] ALSA: hda/ca0132: Don't test for QUIRK_NONE

[Takashi Iwai]

On Sat, 16 Jun 2018 04:20:18 +0200,
Connor McAdams wrote:
> 
> Good call. Please understand that I'm fairly new to all of this, so
> mistakes like this are more due to being ignorant than anything else.
> I was afraid certain things may end up broken by my changes, and I
> hope it didn't inconvenience you.

Connor, if the change looks good and it doesn't break your device
either, just give your Reviewed-by, Acked-by and/or Tested-by tag, so
that I can merge them quickly.


thanks,

Takashi

[PATCH 5/6] ALSA: hda/ca0132: Restore PCM Analog Mic-In2

[Alastair Bridgewater]

Commit 009b8f979bf8cb5f7ec6d3dd7683585122ed10f8 conditionalized
adding the "CA0132 Analog Mic-In2" PCM with a comment to the
effect that, "desktops don't use this ADC", but the test was set
up such that the ADC was only created for desktops.  Invert the
test.

Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index 71487b82eef5..cef22c60d7ee 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -5966,7 +5966,7 @@ static int ca0132_build_pcms(struct hda_codec *codec)
 	info->stream[SNDRV_PCM_STREAM_CAPTURE].nid = spec->adcs[0];
 
 	/* With the DSP enabled, desktops don't use this ADC. */
-	if (spec->use_alt_functions) {
+	if (!spec->use_alt_functions) {
 		info = snd_hda_codec_pcm_new(codec, "CA0132 Analog Mic-In2");
 		if (!info)
 			return -ENOMEM;
-- 
2.16.1

[PATCH 6/6] ALSA: hda/ca0132: Fix DMic data rate for Alienware M17x R4

[Alastair Bridgewater]

The commentary says to use various parameters, and lays out what
the mapping is...  The code used a 32KHz rate when the comment
says that it needs to use a 48KHz rate.  And this has been the
case since day one.

On the Alienware M17x R4, the DMic used to have exceptionally quiet
pickup and a lot of noise.  Changing the data rate fixes both of
these issues.

Searching the kernel bug tracker for ca0132-related issues shows no
mention of this being an issue for other hardware, and I have no
other hardware to test with, so a quirk is used to limit the effect
to just the M17x R4.

Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index cef22c60d7ee..614f69407c9f 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -991,6 +991,7 @@ struct ca0132_spec {
 enum {
 	QUIRK_NONE,
 	QUIRK_ALIENWARE,
+	QUIRK_ALIENWARE_M17XR4,
 	QUIRK_SBZ,
 	QUIRK_R3DI,
 };
@@ -1040,6 +1041,7 @@ static const struct hda_pintbl r3di_pincfgs[] = {
 };
 
 static const struct snd_pci_quirk ca0132_quirks[] = {
+	SND_PCI_QUIRK(0x1028, 0x057b, "Alienware M17x R4", QUIRK_ALIENWARE_M17XR4),
 	SND_PCI_QUIRK(0x1028, 0x0685, "Alienware 15 2015", QUIRK_ALIENWARE),
 	SND_PCI_QUIRK(0x1028, 0x0688, "Alienware 17 2015", QUIRK_ALIENWARE),
 	SND_PCI_QUIRK(0x1028, 0x0708, "Alienware 15 R2 2016", QUIRK_ALIENWARE),
@@ -6130,7 +6132,10 @@ static void ca0132_init_dmic(struct hda_codec *codec)
 	 * Bit   6: set to select Data2, clear for Data1
 	 * Bit   7: set to enable DMic, clear for AMic
 	 */
-	val = 0x23;
+	if (spec->quirk == QUIRK_ALIENWARE_M17XR4)
+		val = 0x33;
+	else
+		val = 0x23;
 	/* keep a copy of dmic ctl val for enable/disable dmic purpuse */
 	spec->dmic_ctl = val;
 	snd_hda_codec_write(codec, spec->input_pins[0], 0,
-- 
2.16.1

Re: [PATCH 6/6] ALSA: hda/ca0132: Fix DMic data rate for Alienware M17x R4

[Connor McAdams]

In my experience, the Dmic is always set to have the voice focus
effect enabled on the Chromebook. The reason it drops the sample rate
is to ease the load on the DSP. On the Recon3Di and the Sound Blaster
Z, if effects are enabled, it drops to 16khz when effects in general
are enabled. I don't work for Creative so I have no clue about the
actual details, but I do know if input effects are on on R3Di or SBZ,
the card becomes laggy and less responsive to commands.

So, it may depend upon the implementation. You could probably figure
out definitively by using my program QemuHDADump to dump the data if
you really wanted to find out for sure what sample rate the mic is set
to by default, and the other parameters. I would be willing to look
through the data dump to help you determine it. Let me know.

On Fri, Jun 15, 2018 at 9:56 PM, Alastair Bridgewater
<alastair.bridgewater@gmail.com> wrote:
> The commentary says to use various parameters, and lays out what
> the mapping is...  The code used a 32KHz rate when the comment
> says that it needs to use a 48KHz rate.  And this has been the
> case since day one.
>
> On the Alienware M17x R4, the DMic used to have exceptionally quiet
> pickup and a lot of noise.  Changing the data rate fixes both of
> these issues.
>
> Searching the kernel bug tracker for ca0132-related issues shows no
> mention of this being an issue for other hardware, and I have no
> other hardware to test with, so a quirk is used to limit the effect
> to just the M17x R4.
>
> Signed-off-by: Alastair Bridgewater <alastair.bridgewater@gmail.com>
>
> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> index cef22c60d7ee..614f69407c9f 100644
> --- a/sound/pci/hda/patch_ca0132.c
> +++ b/sound/pci/hda/patch_ca0132.c
> @@ -991,6 +991,7 @@ struct ca0132_spec {
>  enum {
>         QUIRK_NONE,
>         QUIRK_ALIENWARE,
> +       QUIRK_ALIENWARE_M17XR4,
>         QUIRK_SBZ,
>         QUIRK_R3DI,
>  };
> @@ -1040,6 +1041,7 @@ static const struct hda_pintbl r3di_pincfgs[] = {
>  };
>
>  static const struct snd_pci_quirk ca0132_quirks[] = {
> +       SND_PCI_QUIRK(0x1028, 0x057b, "Alienware M17x R4", QUIRK_ALIENWARE_M17XR4),
>         SND_PCI_QUIRK(0x1028, 0x0685, "Alienware 15 2015", QUIRK_ALIENWARE),
>         SND_PCI_QUIRK(0x1028, 0x0688, "Alienware 17 2015", QUIRK_ALIENWARE),
>         SND_PCI_QUIRK(0x1028, 0x0708, "Alienware 15 R2 2016", QUIRK_ALIENWARE),
> @@ -6130,7 +6132,10 @@ static void ca0132_init_dmic(struct hda_codec *codec)
>          * Bit   6: set to select Data2, clear for Data1
>          * Bit   7: set to enable DMic, clear for AMic
>          */
> -       val = 0x23;
> +       if (spec->quirk == QUIRK_ALIENWARE_M17XR4)
> +               val = 0x33;
> +       else
> +               val = 0x23;
>         /* keep a copy of dmic ctl val for enable/disable dmic purpuse */
>         spec->dmic_ctl = val;
>         snd_hda_codec_write(codec, spec->input_pins[0], 0,
> --
> 2.16.1
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/6] ALSA: hda/ca0132: Various cleanups and fixes

[Takashi Iwai]

On Sat, 16 Jun 2018 03:56:14 +0200,
Alastair Bridgewater wrote:
> 
> The ca0132 codec support is fairly crufty, and the new R3Di and SBZ
> support hasn't exactly improved that situation.  This patch series
> tries to clean up a small amount of longstanding cruft, reverse some
> behavioral changes to other systems made by the recent R3Di/SBZ
> patches, and make the DMic work properly on the Alienware M17x R4.
> 
> For two of these patches (the QUIRK_ALIENWARE patch and the DMic
> patch), it may be that the correct behavior is the exact OPPOSITE of
> what the patch does as far as quirk testing goes (that is, it should
> apply globally, rather than limited to some specific quirk or set of
> quirks), but I have no way to make such a determination at this time,
> so erring on the side of the status quo ante seems appropriate.
> 
> Alastair Bridgewater (6):
>   ALSA: hda/ca0132: Delete pointless assignments to struct auto_pin_cfg
>     fields
>   ALSA: hda/ca0132: Delete redundant UNSOL event requests
>   ALSA: hda/ca0132: Restore behavior of QUIRK_ALIENWARE
>   ALSA: hda/ca0132: Don't test for QUIRK_NONE
>   ALSA: hda/ca0132: Restore PCM Analog Mic-In2
>   ALSA: hda/ca0132: Fix DMic data rate for Alienware M17x R4

Since these look like reasonable fixes, I applied all six patches as
is now.

If any further fixes are needed, please make on top of these.


thanks,

Takashi

Re: [PATCH 0/6] ALSA: hda/ca0132: Various cleanups and fixes

[Alastair Bridgewater]

On Sun, Jun 17, 2018 at 3:22 AM, Takashi Iwai <tiwai@suse.de> wrote:

> On Sat, 16 Jun 2018 03:56:14 +0200,
> Alastair Bridgewater wrote:
> >
> > The ca0132 codec support is fairly crufty, and the new R3Di and SBZ
> > support hasn't exactly improved that situation.  This patch series
> > tries to clean up a small amount of longstanding cruft, reverse some
> > behavioral changes to other systems made by the recent R3Di/SBZ
> > patches, and make the DMic work properly on the Alienware M17x R4.
> >
> > For two of these patches (the QUIRK_ALIENWARE patch and the DMic
> > patch), it may be that the correct behavior is the exact OPPOSITE of
> > what the patch does as far as quirk testing goes (that is, it should
> > apply globally, rather than limited to some specific quirk or set of
> > quirks), but I have no way to make such a determination at this time,
> > so erring on the side of the status quo ante seems appropriate.
> >
> > Alastair Bridgewater (6):
> >   ALSA: hda/ca0132: Delete pointless assignments to struct auto_pin_cfg
> >     fields
> >   ALSA: hda/ca0132: Delete redundant UNSOL event requests
> >   ALSA: hda/ca0132: Restore behavior of QUIRK_ALIENWARE
> >   ALSA: hda/ca0132: Don't test for QUIRK_NONE
> >   ALSA: hda/ca0132: Restore PCM Analog Mic-In2
> >   ALSA: hda/ca0132: Fix DMic data rate for Alienware M17x R4
>
> Since these look like reasonable fixes, I applied all six patches as
> is now.
>
> If any further fixes are needed, please make on top of these.
>

Wonderful!  Thank you.  About the only other patch that I can see
making at this point is to call ca0132_alt_vol_setup() on all systems,
and I could go either way on that.

With a bit of luck, I may have another patch series in a month or so.
Not suitable for 4.18, but maybe for 4.19 or so?


> thanks,
>
> Takashi
>

Re: [PATCH 0/6] ALSA: hda/ca0132: Various cleanups and fixes

[Takashi Iwai]

On Tue, 19 Jun 2018 15:18:41 +0200,
Alastair Bridgewater wrote:
> 
> On Sun, Jun 17, 2018 at 3:22 AM, Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Sat, 16 Jun 2018 03:56:14 +0200,
> > Alastair Bridgewater wrote:
> > >
> > > The ca0132 codec support is fairly crufty, and the new R3Di and SBZ
> > > support hasn't exactly improved that situation.  This patch series
> > > tries to clean up a small amount of longstanding cruft, reverse some
> > > behavioral changes to other systems made by the recent R3Di/SBZ
> > > patches, and make the DMic work properly on the Alienware M17x R4.
> > >
> > > For two of these patches (the QUIRK_ALIENWARE patch and the DMic
> > > patch), it may be that the correct behavior is the exact OPPOSITE of
> > > what the patch does as far as quirk testing goes (that is, it should
> > > apply globally, rather than limited to some specific quirk or set of
> > > quirks), but I have no way to make such a determination at this time,
> > > so erring on the side of the status quo ante seems appropriate.
> > >
> > > Alastair Bridgewater (6):
> > >   ALSA: hda/ca0132: Delete pointless assignments to struct auto_pin_cfg
> > >     fields
> > >   ALSA: hda/ca0132: Delete redundant UNSOL event requests
> > >   ALSA: hda/ca0132: Restore behavior of QUIRK_ALIENWARE
> > >   ALSA: hda/ca0132: Don't test for QUIRK_NONE
> > >   ALSA: hda/ca0132: Restore PCM Analog Mic-In2
> > >   ALSA: hda/ca0132: Fix DMic data rate for Alienware M17x R4
> >
> > Since these look like reasonable fixes, I applied all six patches as
> > is now.
> >
> > If any further fixes are needed, please make on top of these.
> >
> 
> Wonderful!  Thank you.  About the only other patch that I can see
> making at this point is to call ca0132_alt_vol_setup() on all systems,
> and I could go either way on that.
> 
> With a bit of luck, I may have another patch series in a month or so.
> Not suitable for 4.18, but maybe for 4.19 or so?

Yes, sounds like a plan.


thanks,

Takashi