[PATCH v1 0/3] Various Fixes/Improvements for CS35L41 HDA

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH v1 0/3] Various Fixes/Improvements for CS35L41 HDA
@ 2023-02-09 16:04 Stefan Binding
  2023-02-09 16:04 ` [PATCH v1 1/3] ALSA: hda/cs35l41: Fix error condition check Stefan Binding
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Stefan Binding @ 2023-02-09 16:04 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai
  Cc: alsa-devel, linux-kernel, patches, Stefan Binding

Fixes issue in calibration, where return codes were misinterpreted.
Enable High Pass filter to reduce pops and clicks.
Add improvement to ensure firmware and tuning files are always loaded
together.

Stefan Binding (2):
  ALSA: hda: cs35l41: Ensure firmware/tuning pairs are always loaded
  ALSA: hda: cs35l41: Enable Amp High Pass Filter

Vitaly Rodionov (1):
  ALSA: hda/cs35l41: Fix error condition check

 sound/pci/hda/cs35l41_hda.c | 117 ++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 59 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v1 1/3] ALSA: hda/cs35l41: Fix error condition check
  2023-02-09 16:04 [PATCH v1 0/3] Various Fixes/Improvements for CS35L41 HDA Stefan Binding
@ 2023-02-09 16:04 ` Stefan Binding
  2023-02-10  9:10   ` Takashi Iwai
  2023-02-09 16:04 ` [PATCH v1 2/3] ALSA: hda: cs35l41: Ensure firmware/tuning pairs are always loaded Stefan Binding
  2023-02-09 16:04 ` [PATCH v1 3/3] ALSA: hda: cs35l41: Enable Amp High Pass Filter Stefan Binding
  2 siblings, 1 reply; 5+ messages in thread
From: Stefan Binding @ 2023-02-09 16:04 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai
  Cc: alsa-devel, linux-kernel, patches, Vitaly Rodionov

From: Vitaly Rodionov <vitalyr@opensource.cirrus.com>

Function hda_cs_dsp_write_ctl() returns 3 possible values:
0 - no change, 1 - value has changed and -1 - error, so value 1
is not an error.

Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index f7815ee24f83..4dc57454201e 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -316,27 +316,27 @@ static int cs35l41_apply_calibration(struct cs35l41_hda *cs35l41, unsigned int a
 
 	ret = hda_cs_dsp_write_ctl(&cs35l41->cs_dsp, CAL_AMBIENT_DSP_CTL_NAME, CAL_DSP_CTL_TYPE,
 				   CAL_DSP_CTL_ALG, &ambient, 4);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(cs35l41->dev, "Cannot Write Control: %s - %d\n", CAL_AMBIENT_DSP_CTL_NAME,
 			ret);
 		return ret;
 	}
 	ret = hda_cs_dsp_write_ctl(&cs35l41->cs_dsp, CAL_R_DSP_CTL_NAME, CAL_DSP_CTL_TYPE,
 				   CAL_DSP_CTL_ALG, &r0, 4);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(cs35l41->dev, "Cannot Write Control: %s - %d\n", CAL_R_DSP_CTL_NAME, ret);
 		return ret;
 	}
 	ret = hda_cs_dsp_write_ctl(&cs35l41->cs_dsp, CAL_STATUS_DSP_CTL_NAME, CAL_DSP_CTL_TYPE,
 				   CAL_DSP_CTL_ALG, &status, 4);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(cs35l41->dev, "Cannot Write Control: %s - %d\n", CAL_STATUS_DSP_CTL_NAME,
 			ret);
 		return ret;
 	}
 	ret = hda_cs_dsp_write_ctl(&cs35l41->cs_dsp, CAL_CHECKSUM_DSP_CTL_NAME, CAL_DSP_CTL_TYPE,
 				   CAL_DSP_CTL_ALG, &checksum, 4);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(cs35l41->dev, "Cannot Write Control: %s - %d\n", CAL_CHECKSUM_DSP_CTL_NAME,
 			ret);
 		return ret;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v1 1/3] ALSA: hda/cs35l41: Fix error condition check
  2023-02-09 16:04 ` [PATCH v1 1/3] ALSA: hda/cs35l41: Fix error condition check Stefan Binding
@ 2023-02-10  9:10   ` Takashi Iwai
  0 siblings, 0 replies; 5+ messages in thread
From: Takashi Iwai @ 2023-02-10  9:10 UTC (permalink / raw)
  To: Stefan Binding
  Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel, patches,
	Vitaly Rodionov

On Thu, 09 Feb 2023 17:04:01 +0100,
Stefan Binding wrote:
> 
> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com>
> 
> Function hda_cs_dsp_write_ctl() returns 3 possible values:
> 0 - no change, 1 - value has changed and -1 - error, so value 1
> is not an error.
> 
> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com>

When you submit a patch, you must add your Signed-off-by line.

Also, this looks like no new bug, so it's worth to add Fixes tag, if
any?  (Not only about this patch but others, too.)

Last but not least, please make the subject prefix consistent over the
patch series.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v1 2/3] ALSA: hda: cs35l41: Ensure firmware/tuning pairs are always loaded
  2023-02-09 16:04 [PATCH v1 0/3] Various Fixes/Improvements for CS35L41 HDA Stefan Binding
  2023-02-09 16:04 ` [PATCH v1 1/3] ALSA: hda/cs35l41: Fix error condition check Stefan Binding
@ 2023-02-09 16:04 ` Stefan Binding
  2023-02-09 16:04 ` [PATCH v1 3/3] ALSA: hda: cs35l41: Enable Amp High Pass Filter Stefan Binding
  2 siblings, 0 replies; 5+ messages in thread
From: Stefan Binding @ 2023-02-09 16:04 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai
  Cc: alsa-devel, linux-kernel, patches, Stefan Binding

To ensure firmware for cs35l41 is correctly running, it is necessary
that a corresponding tuning file is also loaded. Without both,
the firmware may not be performing correctly
Ensure that if we load the firmware, we have also loaded the correct
tuning file. Otherwise, fall back to default firmware and tuning.
If default tuning is also missing, then disable DSP firmware.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 103 ++++++++++++++++++------------------
 1 file changed, 51 insertions(+), 52 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 4dc57454201e..f577b20c241e 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -178,11 +178,10 @@ static int cs35l41_request_firmware_files_spkid(struct cs35l41_hda *cs35l41,
 					    cs35l41->speaker_id, "wmfw");
 	if (!ret) {
 		/* try cirrus/part-dspN-fwtype-sub<-spkidN><-ampname>.bin */
-		cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
-					      CS35L41_FIRMWARE_ROOT,
-					      cs35l41->acpi_subsystem_id, cs35l41->amp_name,
-					      cs35l41->speaker_id, "bin");
-		return 0;
+		return cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
+						     CS35L41_FIRMWARE_ROOT,
+						     cs35l41->acpi_subsystem_id, cs35l41->amp_name,
+						     cs35l41->speaker_id, "bin");
 	}
 
 	/* try cirrus/part-dspN-fwtype-sub<-ampname>.wmfw */
@@ -191,10 +190,10 @@ static int cs35l41_request_firmware_files_spkid(struct cs35l41_hda *cs35l41,
 					    cs35l41->amp_name, -1, "wmfw");
 	if (!ret) {
 		/* try cirrus/part-dspN-fwtype-sub<-spkidN><-ampname>.bin */
-		cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
-					      CS35L41_FIRMWARE_ROOT, cs35l41->acpi_subsystem_id,
-					      cs35l41->amp_name, cs35l41->speaker_id, "bin");
-		return 0;
+		return cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
+						     CS35L41_FIRMWARE_ROOT,
+						     cs35l41->acpi_subsystem_id, cs35l41->amp_name,
+						     cs35l41->speaker_id, "bin");
 	}
 
 	/* try cirrus/part-dspN-fwtype-sub<-spkidN>.wmfw */
@@ -209,11 +208,10 @@ static int cs35l41_request_firmware_files_spkid(struct cs35l41_hda *cs35l41,
 						    cs35l41->amp_name, cs35l41->speaker_id, "bin");
 		if (ret)
 			/* try cirrus/part-dspN-fwtype-sub<-spkidN>.bin */
-			cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
-						CS35L41_FIRMWARE_ROOT,
-						cs35l41->acpi_subsystem_id,
-						NULL, cs35l41->speaker_id, "bin");
-		return 0;
+			return cs35l41_request_firmware_file(cs35l41, coeff_firmware,
+							     coeff_filename, CS35L41_FIRMWARE_ROOT,
+							     cs35l41->acpi_subsystem_id, NULL,
+							     cs35l41->speaker_id, "bin");
 	}
 
 	/* try cirrus/part-dspN-fwtype-sub.wmfw */
@@ -224,29 +222,16 @@ static int cs35l41_request_firmware_files_spkid(struct cs35l41_hda *cs35l41,
 		/* try cirrus/part-dspN-fwtype-sub<-spkidN><-ampname>.bin */
 		ret = cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
 						    CS35L41_FIRMWARE_ROOT,
-						    cs35l41->acpi_subsystem_id,
-						    cs35l41->amp_name, cs35l41->speaker_id, "bin");
+						    cs35l41->acpi_subsystem_id, cs35l41->amp_name,
+						    cs35l41->speaker_id, "bin");
 		if (ret)
 			/* try cirrus/part-dspN-fwtype-sub<-spkidN>.bin */
-			cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
-						      CS35L41_FIRMWARE_ROOT,
-						      cs35l41->acpi_subsystem_id,
-						      NULL, cs35l41->speaker_id, "bin");
-		return 0;
-	}
-
-	/* fallback try cirrus/part-dspN-fwtype.wmfw */
-	ret = cs35l41_request_firmware_file(cs35l41, wmfw_firmware, wmfw_filename,
-					    CS35L41_FIRMWARE_ROOT, NULL, NULL, -1, "wmfw");
-	if (!ret) {
-		/* fallback try cirrus/part-dspN-fwtype.bin */
-		cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
-					      CS35L41_FIRMWARE_ROOT, NULL, NULL, -1, "bin");
-		return 0;
+			return cs35l41_request_firmware_file(cs35l41, coeff_firmware,
+							     coeff_filename, CS35L41_FIRMWARE_ROOT,
+							     cs35l41->acpi_subsystem_id, NULL,
+							     cs35l41->speaker_id, "bin");
 	}
 
-	dev_warn(cs35l41->dev, "Failed to request firmware\n");
-
 	return ret;
 }
 
@@ -258,9 +243,12 @@ static int cs35l41_request_firmware_files(struct cs35l41_hda *cs35l41,
 {
 	int ret;
 
-	if (cs35l41->speaker_id > -1)
-		return cs35l41_request_firmware_files_spkid(cs35l41, wmfw_firmware, wmfw_filename,
-							    coeff_firmware, coeff_filename);
+	if (cs35l41->speaker_id > -1) {
+		ret = cs35l41_request_firmware_files_spkid(cs35l41, wmfw_firmware, wmfw_filename,
+							   coeff_firmware, coeff_filename);
+		goto out;
+
+	}
 
 	/* try cirrus/part-dspN-fwtype-sub<-ampname>.wmfw */
 	ret = cs35l41_request_firmware_file(cs35l41, wmfw_firmware, wmfw_filename,
@@ -268,10 +256,11 @@ static int cs35l41_request_firmware_files(struct cs35l41_hda *cs35l41,
 					    cs35l41->amp_name, -1, "wmfw");
 	if (!ret) {
 		/* try cirrus/part-dspN-fwtype-sub<-ampname>.bin */
-		cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
-					      CS35L41_FIRMWARE_ROOT, cs35l41->acpi_subsystem_id,
-					      cs35l41->amp_name, -1, "bin");
-		return 0;
+		ret = cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
+						    CS35L41_FIRMWARE_ROOT,
+						    cs35l41->acpi_subsystem_id, cs35l41->amp_name,
+						    -1, "bin");
+		goto out;
 	}
 
 	/* try cirrus/part-dspN-fwtype-sub.wmfw */
@@ -286,25 +275,35 @@ static int cs35l41_request_firmware_files(struct cs35l41_hda *cs35l41,
 						    cs35l41->amp_name, -1, "bin");
 		if (ret)
 			/* try cirrus/part-dspN-fwtype-sub.bin */
-			cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
-						      CS35L41_FIRMWARE_ROOT,
-						      cs35l41->acpi_subsystem_id,
-						      NULL, -1, "bin");
-		return 0;
+			ret = cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
+							    CS35L41_FIRMWARE_ROOT,
+							    cs35l41->acpi_subsystem_id, NULL, -1,
+							    "bin");
 	}
 
+out:
+	if (!ret)
+		return 0;
+
+	/* Handle fallback */
+	dev_warn(cs35l41->dev, "Falling back to default firmware.\n");
+
+	release_firmware(*wmfw_firmware);
+	kfree(*wmfw_filename);
+
 	/* fallback try cirrus/part-dspN-fwtype.wmfw */
 	ret = cs35l41_request_firmware_file(cs35l41, wmfw_firmware, wmfw_filename,
 					    CS35L41_FIRMWARE_ROOT, NULL, NULL, -1, "wmfw");
-	if (!ret) {
+	if (!ret)
 		/* fallback try cirrus/part-dspN-fwtype.bin */
-		cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
-					      CS35L41_FIRMWARE_ROOT, NULL, NULL, -1, "bin");
-		return 0;
-	}
-
-	dev_warn(cs35l41->dev, "Failed to request firmware\n");
+		ret = cs35l41_request_firmware_file(cs35l41, coeff_firmware, coeff_filename,
+						    CS35L41_FIRMWARE_ROOT, NULL, NULL, -1, "bin");
 
+	if (ret) {
+		release_firmware(*wmfw_firmware);
+		kfree(*wmfw_filename);
+		dev_warn(cs35l41->dev, "Unable to find firmware and tuning\n");
+	}
 	return ret;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v1 3/3] ALSA: hda: cs35l41: Enable Amp High Pass Filter
  2023-02-09 16:04 [PATCH v1 0/3] Various Fixes/Improvements for CS35L41 HDA Stefan Binding
  2023-02-09 16:04 ` [PATCH v1 1/3] ALSA: hda/cs35l41: Fix error condition check Stefan Binding
  2023-02-09 16:04 ` [PATCH v1 2/3] ALSA: hda: cs35l41: Ensure firmware/tuning pairs are always loaded Stefan Binding
@ 2023-02-09 16:04 ` Stefan Binding
  2 siblings, 0 replies; 5+ messages in thread
From: Stefan Binding @ 2023-02-09 16:04 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai
  Cc: alsa-devel, linux-kernel, patches, Stefan Binding

This helps smooth out pops and clicks in the amps.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index f577b20c241e..dc047c93bb63 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -58,7 +58,7 @@ static const struct reg_sequence cs35l41_hda_config[] = {
 	{ CS35L41_DSP1_RX3_SRC,         0x00000018 }, // DSP1RX3 SRC = VMON
 	{ CS35L41_DSP1_RX4_SRC,         0x00000019 }, // DSP1RX4 SRC = IMON
 	{ CS35L41_DSP1_RX5_SRC,         0x00000020 }, // DSP1RX5 SRC = ERRVOL
-	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00000000 }, // AMP_VOL_PCM  0.0 dB
+	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00008000 }, // AMP_HPF_PCM_EN = 1, AMP_VOL_PCM  0.0 dB
 	{ CS35L41_AMP_GAIN_CTRL,	0x00000084 }, // AMP_GAIN_PCM 4.5 dB
 };
 
@@ -82,13 +82,13 @@ static const struct reg_sequence cs35l41_hda_config_dsp[] = {
 	{ CS35L41_DSP1_RX3_SRC,         0x00000018 }, // DSP1RX3 SRC = VMON
 	{ CS35L41_DSP1_RX4_SRC,         0x00000019 }, // DSP1RX4 SRC = IMON
 	{ CS35L41_DSP1_RX5_SRC,         0x00000029 }, // DSP1RX5 SRC = VBSTMON
-	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00000000 }, // AMP_VOL_PCM  0.0 dB
+	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00008000 }, // AMP_HPF_PCM_EN = 1, AMP_VOL_PCM  0.0 dB
 	{ CS35L41_AMP_GAIN_CTRL,	0x00000233 }, // AMP_GAIN_PCM = 17.5dB AMP_GAIN_PDM = 19.5dB
 };
 
 static const struct reg_sequence cs35l41_hda_mute[] = {
 	{ CS35L41_AMP_GAIN_CTRL,	0x00000000 }, // AMP_GAIN_PCM 0.5 dB
-	{ CS35L41_AMP_DIG_VOL_CTRL,	0x0000A678 }, // AMP_VOL_PCM Mute
+	{ CS35L41_AMP_DIG_VOL_CTRL,	0x0000A678 }, // AMP_HPF_PCM_EN = 1, AMP_VOL_PCM Mute
 };
 
 static void cs35l41_add_controls(struct cs35l41_hda *cs35l41)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-02-10  9:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-09 16:04 [PATCH v1 0/3] Various Fixes/Improvements for CS35L41 HDA Stefan Binding
2023-02-09 16:04 ` [PATCH v1 1/3] ALSA: hda/cs35l41: Fix error condition check Stefan Binding
2023-02-10  9:10   ` Takashi Iwai
2023-02-09 16:04 ` [PATCH v1 2/3] ALSA: hda: cs35l41: Ensure firmware/tuning pairs are always loaded Stefan Binding
2023-02-09 16:04 ` [PATCH v1 3/3] ALSA: hda: cs35l41: Enable Amp High Pass Filter Stefan Binding

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox