Skip to contentSkip to conversation

ASoC: cs42l42: Collection of bugfixes

PATCH In review Richard Fitzgerald 2 participants 21 messages Oct 15, 2021

Subsystems (3)
Sound ASoC Cirrus Logic audio codecs
lore
Share & tools
lore

Richard Fitzgerald

author maintainer
#0 lore raw

cover ASoC: cs42l42: Collection of bugfixes

This patch set contains various bugfixes for the cs42l42 codec driver.

Patches marked "Fixes" will apply cleanly to the patch that first introduced the bug.

Patches NOT marked "Fixes" will not apply cleanly to the point the bug was first introduced and/or the bug is not having sufficient impact to risk churning older code versions.

Richard Fitzgerald (16):
ASoC: cs42l42: Don't reconfigure the PLL while it is running
ASoC: cs42l42: Always configure both ASP TX channels
ASoC: cs42l42: Correct some register default values
ASoC: cs42l42: Don't set defaults for volatile registers
ASoC: cs42l42: Defer probe if request_threaded_irq() returns
EPROBE_DEFER
ASoC: cs42l42: Reset GPIO is mandatory
ASoC: cs42l42: Correct power-up sequence to match datasheet
ASoC: cs42l42: Reset and power-down on driver remove()
ASoC: cs42l42: Prevent NULL pointer deref in interrupt handler
ASoC: cs42l42: Don't claim to support 192k
ASoC: cs42l42: Use PLL for SCLK > 12.288MHz
ASoC: cs42l42: Allow time for HP/ADC to power-up after enable
ASoC: cs42l42: Set correct SRC MCLK
ASoC: cs42l42: Mark OSC_SWITCH_STATUS register volatile
ASoC: cs42l42: Fix WARN in remove() if running without an interrupt
ASoC: cs42l42: Always enable TS_PLUG and TS_UNPLUG interrupts

 sound/soc/codecs/cs42l42.c | 282 +++++++++++++++++++++++++--------------------
 sound/soc/codecs/cs42l42.h |   9 +-
 2 files changed, 160 insertions(+), 131 deletions(-)
Signature · 1 line
2.11.0
author maintainer
#1 lore raw

PATCH 1/16 ASoC: cs42l42: Don't reconfigure the PLL while it is running

When capture and playback substreams are both running at the same time, cs42l42_pcm_hw_params() would be called for each direction. The first call will configure the PLL. The second call must not write the PLL configuration registers again if the first substream is already running, as this could destabilize the PLL.

The DAI is marked symmetric sample bits and sample rate, so the two directions will always have the same SCLK (I2S always has 2 channel slots so the DAI does not need to require symmetric channels to guarantee the same SCLK). However, since cs42l42_pll_config() is checking for an active stream it may as well test that the requested SCLK is the same as the currently active configuration.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 8 ++++++++
 1 file changed, 8 insertions(+)
sound/soc/codecs/cs42l42.c +8 -0
index 8de23e4732b3..26f6a3510a03 100644
@@ -618,6 +618,14 @@ static int cs42l42_pll_config(struct snd_soc_component *component)
 	else
 		clk = cs42l42->sclk;
 
+	/* Don't reconfigure if there is an audio stream running */
+	if (cs42l42->stream_use) {
+		if (pll_ratio_table[cs42l42->pll_config].sclk == clk)
+			return 0;
+		else
+			return -EBUSY;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(pll_ratio_table); i++) {
 		if (pll_ratio_table[i].sclk == clk) {
 			cs42l42->pll_config = i;
Signature · 1 line
2.11.0
author maintainer
#2 lore raw

PATCH 2/16 ASoC: cs42l42: Always configure both ASP TX channels

In reply to Richard Fitzgerald

An I2S frame always has two slots (left and right) even when sending mono. The right channel (channel 2) of ASP TX will always have the same bit width as the left channel and will always be on the high phase of LRCLK.

The previous implementation always passed the field masks for both channels to snd_soc_component_update_bits() but for mono the written value only contained the settings for channel 1. The result was that for mono channel 2 was set to 8-bit (which is an invalid configuration) with both channels on the low phase of LRCLK.

Signed-off-by Richard Fitzgerald <[email protected]>
Fixes 585e7079de0e ("ASoC: cs42l42: Add Capture Support")
---
 sound/soc/codecs/cs42l42.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
sound/soc/codecs/cs42l42.c +4 -5
index 26f6a3510a03..c52393301294 100644
@@ -861,11 +861,10 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
 
 	switch (substream->stream) {
 	case SNDRV_PCM_STREAM_CAPTURE:
-		if (channels == 2) {
-			val |= CS42L42_ASP_TX_CH2_AP_MASK;
-			val |= width << CS42L42_ASP_TX_CH2_RES_SHIFT;
-		}
-		val |= width << CS42L42_ASP_TX_CH1_RES_SHIFT;
+		/* channel 2 on high LRCLK */
+		val = CS42L42_ASP_TX_CH2_AP_MASK |
+		      (width << CS42L42_ASP_TX_CH2_RES_SHIFT) |
+		      (width << CS42L42_ASP_TX_CH1_RES_SHIFT);
 
 		snd_soc_component_update_bits(component, CS42L42_ASP_TX_CH_AP_RES,
 				CS42L42_ASP_TX_CH1_AP_MASK | CS42L42_ASP_TX_CH2_AP_MASK |
Signature · 1 line
2.11.0
author maintainer
#3 lore raw

PATCH 3/16 ASoC: cs42l42: Correct some register default values

In reply to Richard Fitzgerald

Some registers had wrong default values in cs42l42_reg_defaults[].

Signed-off-by Richard Fitzgerald <[email protected]>
Fixes 2c394ca79604 ("ASoC: Add support for CS42L42 codec")
---
 sound/soc/codecs/cs42l42.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
sound/soc/codecs/cs42l42.c +2 -2
index c52393301294..a5c460f2ec8c 100644
@@ -93,7 +93,7 @@ static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_ASP_RX_INT_MASK,		0x1F },
 	{ CS42L42_ASP_TX_INT_MASK,		0x0F },
 	{ CS42L42_CODEC_INT_MASK,		0x03 },
-	{ CS42L42_SRCPL_INT_MASK,		0xFF },
+	{ CS42L42_SRCPL_INT_MASK,		0x7F },
 	{ CS42L42_VPMON_INT_MASK,		0x01 },
 	{ CS42L42_PLL_LOCK_INT_MASK,		0x01 },
 	{ CS42L42_TSRS_PLUG_INT_MASK,		0x0F },

@@ -130,7 +130,7 @@ static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_MIXER_CHA_VOL,		0x3F },
 	{ CS42L42_MIXER_ADC_VOL,		0x3F },
 	{ CS42L42_MIXER_CHB_VOL,		0x3F },
-	{ CS42L42_EQ_COEF_IN0,			0x22 },
+	{ CS42L42_EQ_COEF_IN0,			0x00 },
 	{ CS42L42_EQ_COEF_IN1,			0x00 },
 	{ CS42L42_EQ_COEF_IN2,			0x00 },
 	{ CS42L42_EQ_COEF_IN3,			0x00 },
Signature · 1 line
2.11.0
author maintainer
#4 lore raw

PATCH 4/16 ASoC: cs42l42: Don't set defaults for volatile registers

In reply to Richard Fitzgerald

Volatile registers don't need a default value.

Signed-off-by Richard Fitzgerald <[email protected]>
Fixes 2c394ca79604 ("ASoC: Add support for CS42L42 codec")
---
 sound/soc/codecs/cs42l42.c | 20 --------------------
 1 file changed, 20 deletions(-)
sound/soc/codecs/cs42l42.c +0 -20
index a5c460f2ec8c..c4efdc8f5d24 100644
@@ -41,7 +41,6 @@
 static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_FRZ_CTL,			0x00 },
 	{ CS42L42_SRC_CTL,			0x10 },
-	{ CS42L42_MCLK_STATUS,			0x02 },
 	{ CS42L42_MCLK_CTL,			0x02 },
 	{ CS42L42_SFTRAMP_RATE,			0xA4 },
 	{ CS42L42_I2C_DEBOUNCE,			0x88 },

@@ -57,11 +56,9 @@ static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_RSENSE_CTL3,			0x1B },
 	{ CS42L42_TSENSE_CTL,			0x1B },
 	{ CS42L42_TSRS_INT_DISABLE,		0x00 },
-	{ CS42L42_TRSENSE_STATUS,		0x00 },
 	{ CS42L42_HSDET_CTL1,			0x77 },
 	{ CS42L42_HSDET_CTL2,			0x00 },
 	{ CS42L42_HS_SWITCH_CTL,		0xF3 },
-	{ CS42L42_HS_DET_STATUS,		0x00 },
 	{ CS42L42_HS_CLAMP_DISABLE,		0x00 },
 	{ CS42L42_MCLK_SRC_SEL,			0x00 },
 	{ CS42L42_SPDIF_CLK_CFG,		0x00 },

@@ -75,18 +72,6 @@ static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_IN_ASRC_CLK,			0x00 },
 	{ CS42L42_OUT_ASRC_CLK,			0x00 },
 	{ CS42L42_PLL_DIV_CFG1,			0x00 },
-	{ CS42L42_ADC_OVFL_STATUS,		0x00 },
-	{ CS42L42_MIXER_STATUS,			0x00 },
-	{ CS42L42_SRC_STATUS,			0x00 },
-	{ CS42L42_ASP_RX_STATUS,		0x00 },
-	{ CS42L42_ASP_TX_STATUS,		0x00 },
-	{ CS42L42_CODEC_STATUS,			0x00 },
-	{ CS42L42_DET_INT_STATUS1,		0x00 },
-	{ CS42L42_DET_INT_STATUS2,		0x00 },
-	{ CS42L42_SRCPL_INT_STATUS,		0x00 },
-	{ CS42L42_VPMON_STATUS,			0x00 },
-	{ CS42L42_PLL_LOCK_STATUS,		0x00 },
-	{ CS42L42_TSRS_PLUG_STATUS,		0x00 },
 	{ CS42L42_ADC_OVFL_INT_MASK,		0x01 },
 	{ CS42L42_MIXER_INT_MASK,		0x0F },
 	{ CS42L42_SRC_INT_MASK,			0x0F },

@@ -105,8 +90,6 @@ static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_PLL_CTL3,			0x10 },
 	{ CS42L42_PLL_CAL_RATIO,		0x80 },
 	{ CS42L42_PLL_CTL4,			0x03 },
-	{ CS42L42_LOAD_DET_RCSTAT,		0x00 },
-	{ CS42L42_LOAD_DET_DONE,		0x00 },
 	{ CS42L42_LOAD_DET_EN,			0x00 },
 	{ CS42L42_HSBIAS_SC_AUTOCTL,		0x03 },
 	{ CS42L42_WAKE_CTL,			0xC0 },

@@ -115,8 +98,6 @@ static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_MISC_DET_CTL,			0x03 },
 	{ CS42L42_MIC_DET_CTL1,			0x1F },
 	{ CS42L42_MIC_DET_CTL2,			0x2F },
-	{ CS42L42_DET_STATUS1,			0x00 },
-	{ CS42L42_DET_STATUS2,			0x00 },
 	{ CS42L42_DET_INT1_MASK,		0xE0 },
 	{ CS42L42_DET_INT2_MASK,		0xFF },
 	{ CS42L42_HS_BIAS_CTL,			0xC2 },

@@ -182,7 +163,6 @@ static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_ASP_RX_DAI1_CH2_AP_RES,	0x03 },
 	{ CS42L42_ASP_RX_DAI1_CH2_BIT_MSB,	0x00 },
 	{ CS42L42_ASP_RX_DAI1_CH2_BIT_LSB,	0x00 },
-	{ CS42L42_SUB_REVID,			0x03 },
 };
 
 static bool cs42l42_readable_register(struct device *dev, unsigned int reg)
Signature · 1 line
2.11.0
author maintainer
#5 lore raw

PATCH 5/16 ASoC: cs42l42: Defer probe if request_threaded_irq() returns EPROBE_DEFER

In reply to Richard Fitzgerald

The driver can run without an interrupt so if devm_request_threaded_irq() failed, the probe() just carried on. But if this was EPROBE_DEFER the driver would continue without an interrupt instead of deferring to wait for the interrupt to become available.

Fixes 2c394ca79604 ("ASoC: Add support for CS42L42 codec")
Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
sound/soc/codecs/cs42l42.c +3 -2
index c4efdc8f5d24..0ecf2129ea45 100644
@@ -2053,8 +2053,9 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 			NULL, cs42l42_irq_thread,
 			IRQF_ONESHOT | IRQF_TRIGGER_LOW,
 			"cs42l42", cs42l42);
-
-	if (ret != 0)
+	if (ret == -EPROBE_DEFER)
+		goto err_disable;
+	else if (ret != 0)
 		dev_err(&i2c_client->dev,
 			"Failed to request IRQ: %d\n", ret);
Signature · 1 line
2.11.0
author maintainer
#6 lore raw

PATCH 6/16 ASoC: cs42l42: Reset GPIO is mandatory

In reply to Richard Fitzgerald

The hard RESET must be used to correctly power-up the cs42l42, as described in the datasheet.

The code was getting the GPIO with devm_gpiod_get_optional(). Change this to devm_gpiod_get().

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
sound/soc/codecs/cs42l42.c +3 -6
index 0ecf2129ea45..629a0783e693 100644
@@ -2034,17 +2034,14 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 	}
 
 	/* Reset the Device */
-	cs42l42->reset_gpio = devm_gpiod_get_optional(&i2c_client->dev,
-		"reset", GPIOD_OUT_LOW);
+	cs42l42->reset_gpio = devm_gpiod_get(&i2c_client->dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(cs42l42->reset_gpio)) {
 		ret = PTR_ERR(cs42l42->reset_gpio);
+		dev_err(&i2c_client->dev, "Failed to request reset gpio: %d\n", ret);
 		goto err_disable;
 	}
 
-	if (cs42l42->reset_gpio) {
-		dev_dbg(&i2c_client->dev, "Found reset GPIO\n");
-		gpiod_set_value_cansleep(cs42l42->reset_gpio, 1);
-	}
+	gpiod_set_value_cansleep(cs42l42->reset_gpio, 1);
 	usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
 
 	/* Request IRQ */
Signature · 1 line
2.11.0
maintainer
#7 lore raw

Re: [PATCH 06/16] ASoC: cs42l42: Reset GPIO is mandatory

Quoted 5 lines · Richard Fitzgerald

The hard RESET must be used to correctly power-up the cs42l42, as described in the datasheet.

The code was getting the GPIO with devm_gpiod_get_optional(). Change this to devm_gpiod_get().

Does that power sequencing have to be done by the CPU though? Usually if a GPIO is not supplied it's because the sequencing is done during the general power up sequence (PMICs can be programmed to assert GPIOs as part of the their sequencing for example).

author maintainer
#8 lore raw

PATCH 7/16 ASoC: cs42l42: Correct power-up sequence to match datasheet

In reply to Richard Fitzgerald

The power-up sequence mandated in the datasheet is:

- VP must turn on first
- VA, VCP, VL, in any order
- VD_FILT after VL
- RESET must be asserted while VP turns on

- VD_FILT must turn off before VL
- VP must turn off last

This patch fixes the order the regulators are enabled and holds RESET asserted around the power-up. The datasheet power-down order is the reverse of the power-up order so this is automatically covered by listing the bulk regulators in power-up order.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 22 ++++++++++++----------
 sound/soc/codecs/cs42l42.h |  4 ++--
 2 files changed, 14 insertions(+), 12 deletions(-)
sound/soc/codecs/cs42l42.c +12 -10
index 629a0783e693..420e16563c45 100644
@@ -2025,22 +2025,23 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 		return ret;
 	}
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(cs42l42->supplies),
-				    cs42l42->supplies);
-	if (ret != 0) {
-		dev_err(&i2c_client->dev,
-			"Failed to enable supplies: %d\n", ret);
-		return ret;
-	}
-
-	/* Reset the Device */
+	/* Hold device in reset while it powers up */
 	cs42l42->reset_gpio = devm_gpiod_get(&i2c_client->dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(cs42l42->reset_gpio)) {
 		ret = PTR_ERR(cs42l42->reset_gpio);
 		dev_err(&i2c_client->dev, "Failed to request reset gpio: %d\n", ret);
-		goto err_disable;
+		return ret;
 	}
 
+	ret = regulator_bulk_enable(ARRAY_SIZE(cs42l42->supplies),
+				    cs42l42->supplies);
+	if (ret != 0) {
+		dev_err(&i2c_client->dev,
+			"Failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	/* Release reset and wait for boot */
 	gpiod_set_value_cansleep(cs42l42->reset_gpio, 1);
 	usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
 

@@ -2116,6 +2117,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 	return 0;
 
 err_disable:
+	gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
 	regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies),
 				cs42l42->supplies);
 	return ret;
sound/soc/codecs/cs42l42.h +2 -2
index 0704c902475f..2343213d0cdb 100644
@@ -822,11 +822,11 @@
 #define CS42L42_PLL_LOCK_TIMEOUT_US	1250
 
 static const char *const cs42l42_supply_names[CS42L42_NUM_SUPPLIES] = {
-	"VA",
 	"VP",
+	"VA",
 	"VCP",
-	"VD_FILT",
 	"VL",
+	"VD_FILT",
 };
 
 struct  cs42l42_private {
Signature · 1 line
2.11.0
maintainer
#9 lore raw

Re: [PATCH 07/16] ASoC: cs42l42: Correct power-up sequence to match datasheet

Quoted 7 lines · Richard Fitzgerald

The power-up sequence mandated in the datasheet is:

- VP must turn on first
- VA, VCP, VL, in any order
- VD_FILT after VL

static const char *const cs42l42_supply_names[CS42L42_NUM_SUPPLIES] = {

sound/soc/codecs/cs42l42.h line 830

Quoted 8 lines
sound/soc/codecs/cs42l42.h +2 -2
-	"VA",
 	"VP",
+	"VA",
 	"VCP",
-	"VD_FILT",
 	"VL",
+	"VD_FILT",
 };

If you need the regulators to be turned on in sequence you shouldn't rely on bulk enable doing it for you - the existing regulator code will initiate all the enables in parallel and then wait for them all to complete ramping up so if for example VD_FILT were to ramp more quickly than the earlier regulators the hardware might notice it getting to whatever voltage the hardware cares about before them. The only sequencing you're getting at the minute is when the enables for the regulators are toggled and you shouldn't even rely on that.

To get the sequencing guaranteed you should pull VP and VD_FILT out of the bulk enable and do individual enables for them.

author maintainer
#10 lore raw

PATCH 8/16 ASoC: cs42l42: Reset and power-down on driver remove()

In reply to Richard Fitzgerald

Driver remove() should assert RESET and disable the supplies. Previously this assumed that calling pm_runtime_suspend() would result in a call to cs42l42_runtime_suspend() to power-down. This isn't guaranteed - pm_runtime can be disabled.

Signed-off-by Richard Fitzgerald <[email protected]>
Fixes 2b869e0ea598 ("ASoC: cs42l42: Remove power if the driver is being removed")
---
 sound/soc/codecs/cs42l42.c | 3 +++
 1 file changed, 3 insertions(+)
sound/soc/codecs/cs42l42.c +3 -0
index 420e16563c45..cf1f68474d21 100644
@@ -2131,6 +2131,9 @@ static int cs42l42_i2c_remove(struct i2c_client *i2c_client)
 	pm_runtime_suspend(&i2c_client->dev);
 	pm_runtime_disable(&i2c_client->dev);
 
+	gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
+	regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies);
+
 	return 0;
 }
Signature · 1 line
2.11.0
maintainer
#11 lore raw

Re: [PATCH 08/16] ASoC: cs42l42: Reset and power-down on driver remove()

sound/soc/codecs/cs42l42.c static int cs42l42_i2c_remove(struct i2c_client *i2c_client) · line 2135

Quoted 10 lines · Richard Fitzgerald

Driver remove() should assert RESET and disable the supplies. Previously this assumed that calling pm_runtime_suspend() would result in a call to cs42l42_runtime_suspend() to power-down. This isn't guaranteed - pm_runtime can be disabled.

sound/soc/codecs/cs42l42.c +2 -0
 	pm_runtime_suspend(&i2c_client->dev);
 	pm_runtime_disable(&i2c_client->dev);
 
+	gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
+	regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies);

Won't this end up with an extra disable of the regulators if they're already disabled?

author maintainer
#12 lore raw

PATCH 9/16 ASoC: cs42l42: Prevent NULL pointer deref in interrupt handler

In reply to Richard Fitzgerald

The interrupt handling code was getting the struct device* from a struct snd_soc_component* stored in struct cs42l42_private. If the interrupt was asserted before ASoC calls component_probe() the snd_soc_component* will be NULL.

The stored snd_soc_component* is not actually used for anything other than indirectly getting the struct device*. Remove it, and store the struct device* in struct cs42l42_private.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 28 +++++++++-------------------
 sound/soc/codecs/cs42l42.h |  2 +-
 2 files changed, 10 insertions(+), 20 deletions(-)
sound/soc/codecs/cs42l42.c +9 -19
index cf1f68474d21..174f738e9e5a 100644
@@ -506,17 +506,7 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
 	return 0;
 }
 
-static int cs42l42_component_probe(struct snd_soc_component *component)
-{
-	struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
-
-	cs42l42->component = component;
-
-	return 0;
-}
-
 static const struct snd_soc_component_driver soc_component_dev_cs42l42 = {
-	.probe			= cs42l42_component_probe,
 	.set_jack		= cs42l42_set_jack,
 	.dapm_widgets		= cs42l42_dapm_widgets,
 	.num_dapm_widgets	= ARRAY_SIZE(cs42l42_dapm_widgets),

@@ -1168,7 +1158,7 @@ static void cs42l42_process_hs_type_detect(struct cs42l42_private *cs42l42)
 	 */
 	if (cs42l42->hs_type == CS42L42_PLUG_INVALID ||
 		cs42l42->hs_type == CS42L42_PLUG_HEADPHONE) {
-		dev_dbg(cs42l42->component->dev, "Running Manual Detection Fallback\n");
+		dev_dbg(cs42l42->dev, "Running Manual Detection Fallback\n");
 		cs42l42_manual_hs_type_detect(cs42l42);
 	}
 

@@ -1467,19 +1457,19 @@ static int cs42l42_handle_button_press(struct cs42l42_private *cs42l42)
 	switch (bias_level) {
 	case 1: /* Function C button press */
 		bias_level = SND_JACK_BTN_2;
-		dev_dbg(cs42l42->component->dev, "Function C button press\n");
+		dev_dbg(cs42l42->dev, "Function C button press\n");
 		break;
 	case 2: /* Function B button press */
 		bias_level = SND_JACK_BTN_1;
-		dev_dbg(cs42l42->component->dev, "Function B button press\n");
+		dev_dbg(cs42l42->dev, "Function B button press\n");
 		break;
 	case 3: /* Function D button press */
 		bias_level = SND_JACK_BTN_3;
-		dev_dbg(cs42l42->component->dev, "Function D button press\n");
+		dev_dbg(cs42l42->dev, "Function D button press\n");
 		break;
 	case 4: /* Function A button press */
 		bias_level = SND_JACK_BTN_0;
-		dev_dbg(cs42l42->component->dev, "Function A button press\n");
+		dev_dbg(cs42l42->dev, "Function A button press\n");
 		break;
 	default:
 		bias_level = 0;

@@ -1553,7 +1543,6 @@ static const struct cs42l42_irq_params irq_params_table[] = {
 static irqreturn_t cs42l42_irq_thread(int irq, void *data)
 {
 	struct cs42l42_private *cs42l42 = (struct cs42l42_private *)data;
-	struct snd_soc_component *component = cs42l42->component;
 	unsigned int stickies[12];
 	unsigned int masks[12];
 	unsigned int current_plug_status;

@@ -1600,7 +1589,7 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data)
 			default:
 				break;
 			}
-			dev_dbg(component->dev, "Auto detect done (%d)\n", cs42l42->hs_type);
+			dev_dbg(cs42l42->dev, "Auto detect done (%d)\n", cs42l42->hs_type);
 		}
 	}
 

@@ -1634,7 +1623,7 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data)
 						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
 						    SND_JACK_BTN_2 | SND_JACK_BTN_3);
 
-				dev_dbg(component->dev, "Unplug event\n");
+				dev_dbg(cs42l42->dev, "Unplug event\n");
 			}
 			break;
 

@@ -1650,7 +1639,7 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data)
 			CS42L42_M_HSBIAS_HIZ_MASK)) {
 
 			if (current_button_status & CS42L42_M_DETECT_TF_MASK) {
-				dev_dbg(component->dev, "Button released\n");
+				dev_dbg(cs42l42->dev, "Button released\n");
 				report = 0;
 			} else if (current_button_status & CS42L42_M_DETECT_FT_MASK) {
 				report = cs42l42_handle_button_press(cs42l42);

@@ -2004,6 +1993,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 	if (!cs42l42)
 		return -ENOMEM;
 
+	cs42l42->dev = &i2c_client->dev;
 	i2c_set_clientdata(i2c_client, cs42l42);
 
 	cs42l42->regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap);
sound/soc/codecs/cs42l42.h +1 -1
index 2343213d0cdb..d30643398084 100644
@@ -831,7 +831,7 @@ static const char *const cs42l42_supply_names[CS42L42_NUM_SUPPLIES] = {
 
 struct  cs42l42_private {
 	struct regmap *regmap;
-	struct snd_soc_component *component;
+	struct device *dev;
 	struct regulator_bulk_data supplies[CS42L42_NUM_SUPPLIES];
 	struct gpio_desc *reset_gpio;
 	struct completion pdn_done;
Signature · 1 line
2.11.0
author maintainer
#13 lore raw

PATCH 10/16 ASoC: cs42l42: Don't claim to support 192k

In reply to Richard Fitzgerald

The driver currently only supports configuring for sample rates <= 96k and it isn't possible to setup a configuration that will support all sample rates up to 192k.

For sample rates up to 96k MCLK is in the 12MHz group. However, although 192k only requires an I2S clock in the 12MHz group, the cs42l42 audio path is not natively 192k so the audio must be resampled. But for 192k the SRC requires a 24MHz MCLK.

It is not possible to switch MCLK between 12MHz and 24MHz groups on-the-fly. The 12MHz group supports all sample rates up to 96k.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
sound/soc/codecs/cs42l42.c +3 -3
index 174f738e9e5a..c18f42cc1044 100644
@@ -802,7 +802,7 @@ static int cs42l42_dai_startup(struct snd_pcm_substream *substream, struct snd_s
 	/* Machine driver has not set a SCLK, limit bottom end to 44.1 kHz */
 	return snd_pcm_hw_constraint_minmax(substream->runtime,
 					    SNDRV_PCM_HW_PARAM_RATE,
-					    44100, 192000);
+					    44100, 96000);
 }
 
 static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,

@@ -998,14 +998,14 @@ static struct snd_soc_dai_driver cs42l42_dai = {
 			.stream_name = "Playback",
 			.channels_min = 1,
 			.channels_max = 2,
-			.rates = SNDRV_PCM_RATE_8000_192000,
+			.rates = SNDRV_PCM_RATE_8000_96000,
 			.formats = CS42L42_FORMATS,
 		},
 		.capture = {
 			.stream_name = "Capture",
 			.channels_min = 1,
 			.channels_max = 2,
-			.rates = SNDRV_PCM_RATE_8000_192000,
+			.rates = SNDRV_PCM_RATE_8000_96000,
 			.formats = CS42L42_FORMATS,
 		},
 		.symmetric_rate = 1,
Signature · 1 line
2.11.0
author maintainer
#14 lore raw

PATCH 11/16 ASoC: cs42l42: Use PLL for SCLK > 12.288MHz

In reply to Richard Fitzgerald

It isn't possible to switch MCLK between 12MHz and 24MHz rate groups on-the-fly - this can only be done when cs42l42 is powered-down.

All "normal" SCLK rates use an MCLK in the 12MHz group, so change the configs for SCLK > 12.288 MHz to use the PLL to generate an MCLK in the 12MHz group.

As this means MCLK_DIV is always 0 it can be removed from the pll configuration setup.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)
sound/soc/codecs/cs42l42.c +18 -23
index c18f42cc1044..64bcabeb8f57 100644
@@ -539,7 +539,6 @@ static const struct reg_sequence cs42l42_to_osc_seq[] = {
 
 struct cs42l42_pll_params {
 	u32 sclk;
-	u8 mclk_div;
 	u8 mclk_src_sel;
 	u8 sclk_prediv;
 	u8 pll_div_int;

@@ -556,24 +555,24 @@ struct cs42l42_pll_params {
  * Table 4-5 from the Datasheet
  */
 static const struct cs42l42_pll_params pll_ratio_table[] = {
-	{ 1411200, 0, 1, 0x00, 0x80, 0x000000, 0x03, 0x10, 11289600, 128, 2},
-	{ 1536000, 0, 1, 0x00, 0x7D, 0x000000, 0x03, 0x10, 12000000, 125, 2},
-	{ 2304000, 0, 1, 0x00, 0x55, 0xC00000, 0x02, 0x10, 12288000,  85, 2},
-	{ 2400000, 0, 1, 0x00, 0x50, 0x000000, 0x03, 0x10, 12000000,  80, 2},
-	{ 2822400, 0, 1, 0x00, 0x40, 0x000000, 0x03, 0x10, 11289600, 128, 1},
-	{ 3000000, 0, 1, 0x00, 0x40, 0x000000, 0x03, 0x10, 12000000, 128, 1},
-	{ 3072000, 0, 1, 0x00, 0x3E, 0x800000, 0x03, 0x10, 12000000, 125, 1},
-	{ 4000000, 0, 1, 0x00, 0x30, 0x800000, 0x03, 0x10, 12000000,  96, 1},
-	{ 4096000, 0, 1, 0x00, 0x2E, 0xE00000, 0x03, 0x10, 12000000,  94, 1},
-	{ 5644800, 0, 1, 0x01, 0x40, 0x000000, 0x03, 0x10, 11289600, 128, 1},
-	{ 6000000, 0, 1, 0x01, 0x40, 0x000000, 0x03, 0x10, 12000000, 128, 1},
-	{ 6144000, 0, 1, 0x01, 0x3E, 0x800000, 0x03, 0x10, 12000000, 125, 1},
-	{ 11289600, 0, 0, 0, 0, 0, 0, 0, 11289600, 0, 1},
-	{ 12000000, 0, 0, 0, 0, 0, 0, 0, 12000000, 0, 1},
-	{ 12288000, 0, 0, 0, 0, 0, 0, 0, 12288000, 0, 1},
-	{ 22579200, 1, 0, 0, 0, 0, 0, 0, 22579200, 0, 1},
-	{ 24000000, 1, 0, 0, 0, 0, 0, 0, 24000000, 0, 1},
-	{ 24576000, 1, 0, 0, 0, 0, 0, 0, 24576000, 0, 1}
+	{ 1411200,  1, 0x00, 0x80, 0x000000, 0x03, 0x10, 11289600, 128, 2},
+	{ 1536000,  1, 0x00, 0x7D, 0x000000, 0x03, 0x10, 12000000, 125, 2},
+	{ 2304000,  1, 0x00, 0x55, 0xC00000, 0x02, 0x10, 12288000,  85, 2},
+	{ 2400000,  1, 0x00, 0x50, 0x000000, 0x03, 0x10, 12000000,  80, 2},
+	{ 2822400,  1, 0x00, 0x40, 0x000000, 0x03, 0x10, 11289600, 128, 1},
+	{ 3000000,  1, 0x00, 0x40, 0x000000, 0x03, 0x10, 12000000, 128, 1},
+	{ 3072000,  1, 0x00, 0x3E, 0x800000, 0x03, 0x10, 12000000, 125, 1},
+	{ 4000000,  1, 0x00, 0x30, 0x800000, 0x03, 0x10, 12000000,  96, 1},
+	{ 4096000,  1, 0x00, 0x2E, 0xE00000, 0x03, 0x10, 12000000,  94, 1},
+	{ 5644800,  1, 0x01, 0x40, 0x000000, 0x03, 0x10, 11289600, 128, 1},
+	{ 6000000,  1, 0x01, 0x40, 0x000000, 0x03, 0x10, 12000000, 128, 1},
+	{ 6144000,  1, 0x01, 0x3E, 0x800000, 0x03, 0x10, 12000000, 125, 1},
+	{ 11289600, 0, 0, 0, 0, 0, 0, 11289600, 0, 1},
+	{ 12000000, 0, 0, 0, 0, 0, 0, 12000000, 0, 1},
+	{ 12288000, 0, 0, 0, 0, 0, 0, 12288000, 0, 1},
+	{ 22579200, 1, 0x03, 0x40, 0x000000, 0x03, 0x10, 11289600, 128, 1},
+	{ 24000000, 1, 0x03, 0x40, 0x000000, 0x03, 0x10, 12000000, 128, 1},
+	{ 24576000, 1, 0x03, 0x40, 0x000000, 0x03, 0x10, 12288000, 128, 1}
 };
 
 static int cs42l42_pll_config(struct snd_soc_component *component)

@@ -609,10 +608,6 @@ static int cs42l42_pll_config(struct snd_soc_component *component)
 					24000000)) <<
 					CS42L42_INTERNAL_FS_SHIFT);
 
-			snd_soc_component_update_bits(component, CS42L42_MCLK_SRC_SEL,
-					CS42L42_MCLKDIV_MASK,
-					(pll_ratio_table[i].mclk_div <<
-					CS42L42_MCLKDIV_SHIFT));
 			/* Set up the LRCLK */
 			fsync = clk / cs42l42->srate;
 			if (((fsync * cs42l42->srate) != clk)
Signature · 1 line
2.11.0
author maintainer
#15 lore raw

PATCH 12/16 ASoC: cs42l42: Allow time for HP/ADC to power-up after enable

In reply to Richard Fitzgerald

After enabling the HP or ADC by writing the corresponding PDN=0, it takes around 20 milliseconds for it to power up and the midrail supply to be stable. Add this wait into a DAPM widget callback.

If HP and ADC are both powering up in a DAPM sequence, there's no need to do the wait twice. The widget will perform one wait in the POST_PMU if there was a PRE_PMU for one or both.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 31 +++++++++++++++++++++++++++++--
 sound/soc/codecs/cs42l42.h |  2 ++
 2 files changed, 31 insertions(+), 2 deletions(-)
sound/soc/codecs/cs42l42.c +29 -2
index 64bcabeb8f57..54b4bc391ee9 100644
@@ -435,10 +435,36 @@ static const struct snd_kcontrol_new cs42l42_snd_controls[] = {
 				0x3f, 1, mixer_tlv)
 };
 
+static int cs42l42_hp_adc_ev(struct snd_soc_dapm_widget *w,
+			     struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
+	struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
+
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		cs42l42->hp_adc_up_pending = true;
+		break;
+	case SND_SOC_DAPM_POST_PMU:
+		/* Only need one delay if HP and ADC are both powering-up */
+		if (cs42l42->hp_adc_up_pending) {
+			usleep_range(CS42L42_HP_ADC_EN_TIME_US,
+				     CS42L42_HP_ADC_EN_TIME_US + 1000);
+			cs42l42->hp_adc_up_pending = false;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static const struct snd_soc_dapm_widget cs42l42_dapm_widgets[] = {
 	/* Playback Path */
 	SND_SOC_DAPM_OUTPUT("HP"),
-	SND_SOC_DAPM_DAC("DAC", NULL, CS42L42_PWR_CTL1, CS42L42_HP_PDN_SHIFT, 1),
+	SND_SOC_DAPM_DAC_E("DAC", NULL, CS42L42_PWR_CTL1, CS42L42_HP_PDN_SHIFT, 1,
+			   cs42l42_hp_adc_ev, SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU),
 	SND_SOC_DAPM_MIXER("MIXER", CS42L42_PWR_CTL1, CS42L42_MIXER_PDN_SHIFT, 1, NULL, 0),
 	SND_SOC_DAPM_AIF_IN("SDIN1", NULL, 0, SND_SOC_NOPM, 0, 0),
 	SND_SOC_DAPM_AIF_IN("SDIN2", NULL, 1, SND_SOC_NOPM, 0, 0),

@@ -448,7 +474,8 @@ static const struct snd_soc_dapm_widget cs42l42_dapm_widgets[] = {
 
 	/* Capture Path */
 	SND_SOC_DAPM_INPUT("HS"),
-	SND_SOC_DAPM_ADC("ADC", NULL, CS42L42_PWR_CTL1, CS42L42_ADC_PDN_SHIFT, 1),
+	SND_SOC_DAPM_ADC_E("ADC", NULL, CS42L42_PWR_CTL1, CS42L42_ADC_PDN_SHIFT, 1,
+			   cs42l42_hp_adc_ev, SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU),
 	SND_SOC_DAPM_AIF_OUT("SDOUT1", NULL, 0, CS42L42_ASP_TX_CH_EN, CS42L42_ASP_TX0_CH1_SHIFT, 0),
 	SND_SOC_DAPM_AIF_OUT("SDOUT2", NULL, 1, CS42L42_ASP_TX_CH_EN, CS42L42_ASP_TX0_CH2_SHIFT, 0),
sound/soc/codecs/cs42l42.h +2 -0
index d30643398084..024760300937 100644
@@ -820,6 +820,7 @@
 #define CS42L42_CLOCK_SWITCH_DELAY_US 150
 #define CS42L42_PLL_LOCK_POLL_US	250
 #define CS42L42_PLL_LOCK_TIMEOUT_US	1250
+#define CS42L42_HP_ADC_EN_TIME_US	20000
 
 static const char *const cs42l42_supply_names[CS42L42_NUM_SUPPLIES] = {
 	"VP",

@@ -853,6 +854,7 @@ struct  cs42l42_private {
 	u8 hs_bias_ramp_time;
 	u8 hs_bias_sense_en;
 	u8 stream_use;
+	bool hp_adc_up_pending;
 };
 
 #endif /* __CS42L42_H__ */
Signature · 1 line
2.11.0
author maintainer
#16 lore raw

PATCH 13/16 ASoC: cs42l42: Set correct SRC MCLK

In reply to Richard Fitzgerald

According to the datasheet the SRC MCLK must be as near as possible to (125 * sample rate). This means it should be ~6MHz for rates up to 48k and ~12MHz for rates above that. As per datasheet table 4-21.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 58 ++++++++++++++++++++++++++++++++--------------
 sound/soc/codecs/cs42l42.h |  1 +
 2 files changed, 42 insertions(+), 17 deletions(-)
sound/soc/codecs/cs42l42.c +41 -17
index 54b4bc391ee9..05b8ae62b20d 100644
@@ -668,22 +668,6 @@ static int cs42l42_pll_config(struct snd_soc_component *component)
 					CS42L42_FSYNC_PULSE_WIDTH_MASK,
 					CS42L42_FRAC1_VAL(fsync - 1) <<
 					CS42L42_FSYNC_PULSE_WIDTH_SHIFT);
-			/* Set the sample rates (96k or lower) */
-			snd_soc_component_update_bits(component, CS42L42_FS_RATE_EN,
-					CS42L42_FS_EN_MASK,
-					(CS42L42_FS_EN_IASRC_96K |
-					CS42L42_FS_EN_OASRC_96K) <<
-					CS42L42_FS_EN_SHIFT);
-			/* Set the input/output internal MCLK clock ~12 MHz */
-			snd_soc_component_update_bits(component, CS42L42_IN_ASRC_CLK,
-					CS42L42_CLK_IASRC_SEL_MASK,
-					CS42L42_CLK_IASRC_SEL_12 <<
-					CS42L42_CLK_IASRC_SEL_SHIFT);
-			snd_soc_component_update_bits(component,
-					CS42L42_OUT_ASRC_CLK,
-					CS42L42_CLK_OASRC_SEL_MASK,
-					CS42L42_CLK_OASRC_SEL_12 <<
-					CS42L42_CLK_OASRC_SEL_SHIFT);
 			if (pll_ratio_table[i].mclk_src_sel == 0) {
 				/* Pass the clock straight through */
 				snd_soc_component_update_bits(component,

@@ -746,6 +730,39 @@ static int cs42l42_pll_config(struct snd_soc_component *component)
 	return -EINVAL;
 }
 
+static void cs42l42_src_config(struct snd_soc_component *component, unsigned int sample_rate)
+{
+	struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
+	unsigned int fs;
+
+	/* Don't reconfigure if there is an audio stream running */
+	if (cs42l42->stream_use)
+		return;
+
+	/* SRC MCLK must be as close as possible to 125 * sample rate */
+	if (sample_rate <= 48000)
+		fs = CS42L42_CLK_IASRC_SEL_6;
+	else
+		fs = CS42L42_CLK_IASRC_SEL_12;
+
+	/* Set the sample rates (96k or lower) */
+	snd_soc_component_update_bits(component,
+				      CS42L42_FS_RATE_EN,
+				      CS42L42_FS_EN_MASK,
+				      (CS42L42_FS_EN_IASRC_96K |
+				       CS42L42_FS_EN_OASRC_96K) <<
+				      CS42L42_FS_EN_SHIFT);
+
+	snd_soc_component_update_bits(component,
+				      CS42L42_IN_ASRC_CLK,
+				      CS42L42_CLK_IASRC_SEL_MASK,
+				      fs << CS42L42_CLK_IASRC_SEL_SHIFT);
+	snd_soc_component_update_bits(component,
+				      CS42L42_OUT_ASRC_CLK,
+				      CS42L42_CLK_OASRC_SEL_MASK,
+				      fs << CS42L42_CLK_OASRC_SEL_SHIFT);
+}
+
 static int cs42l42_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
 {
 	struct snd_soc_component *component = codec_dai->component;

@@ -836,6 +853,7 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
 	unsigned int channels = params_channels(params);
 	unsigned int width = (params_width(params) / 8) - 1;
 	unsigned int val = 0;
+	int ret;
 
 	cs42l42->srate = params_rate(params);
 	cs42l42->bclk = snd_soc_params_to_bclk(params);

@@ -889,7 +907,13 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
 		break;
 	}
 
-	return cs42l42_pll_config(component);
+	ret = cs42l42_pll_config(component);
+	if (ret)
+		return ret;
+
+	cs42l42_src_config(component, params_rate(params));
+
+	return 0;
 }
 
 static int cs42l42_set_sysclk(struct snd_soc_dai *dai,
sound/soc/codecs/cs42l42.h +1 -0
index 024760300937..46074624d300 100644
@@ -347,6 +347,7 @@
 #define CS42L42_IN_ASRC_CLK		(CS42L42_PAGE_12 + 0x0A)
 #define CS42L42_CLK_IASRC_SEL_SHIFT	0
 #define CS42L42_CLK_IASRC_SEL_MASK	(1 << CS42L42_CLK_IASRC_SEL_SHIFT)
+#define CS42L42_CLK_IASRC_SEL_6		0
 #define CS42L42_CLK_IASRC_SEL_12	1
 
 #define CS42L42_OUT_ASRC_CLK		(CS42L42_PAGE_12 + 0x0B)
Signature · 1 line
2.11.0
author maintainer
#17 lore raw

PATCH 14/16 ASoC: cs42l42: Mark OSC_SWITCH_STATUS register volatile

In reply to Richard Fitzgerald

OSC_SWITCH_STATUS is a volatile register indicating the current state of the clock switch logic.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/codecs/cs42l42.c +1 -1
index 05b8ae62b20d..8e4a43c5a120 100644
@@ -52,7 +52,6 @@ static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_RSENSE_CTL1,			0x40 },
 	{ CS42L42_RSENSE_CTL2,			0x00 },
 	{ CS42L42_OSC_SWITCH,			0x00 },
-	{ CS42L42_OSC_SWITCH_STATUS,		0x05 },
 	{ CS42L42_RSENSE_CTL3,			0x1B },
 	{ CS42L42_TSENSE_CTL,			0x1B },
 	{ CS42L42_TSRS_INT_DISABLE,		0x00 },

@@ -331,6 +330,7 @@ static bool cs42l42_volatile_register(struct device *dev, unsigned int reg)
 	case CS42L42_DEVID_CD:
 	case CS42L42_DEVID_E:
 	case CS42L42_MCLK_STATUS:
+	case CS42L42_OSC_SWITCH_STATUS:
 	case CS42L42_TRSENSE_STATUS:
 	case CS42L42_HS_DET_STATUS:
 	case CS42L42_ADC_OVFL_STATUS:
Signature · 1 line
2.11.0
author maintainer
#18 lore raw

PATCH 15/16 ASoC: cs42l42: Fix WARN in remove() if running without an interrupt

In reply to Richard Fitzgerald

The driver must free the IRQ in remove() to prevent the potential race where an IRQ starts to be handled while the driver is being removed but devres has not yet called free_irq(). However, the driver can run without an interrupt but devm_free_irq() will hit a WARN() if no devres-managed interrupt was ever created.

Fix this by only attempting to create the interrupt handler if the hardware config specified an interrupt, and failing probe() if the interrupt could not be created. This means that in cs42l42_remove() an interrupt must have been registered if the irq number is valid and therefore it is safe to call devm_free_irq().

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)
sound/soc/codecs/cs42l42.c +18 -12
index 8e4a43c5a120..8f0c58097d64 100644
@@ -2081,17 +2081,21 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 	gpiod_set_value_cansleep(cs42l42->reset_gpio, 1);
 	usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
 
-	/* Request IRQ */
-	ret = devm_request_threaded_irq(&i2c_client->dev,
-			i2c_client->irq,
-			NULL, cs42l42_irq_thread,
-			IRQF_ONESHOT | IRQF_TRIGGER_LOW,
-			"cs42l42", cs42l42);
-	if (ret == -EPROBE_DEFER)
-		goto err_disable;
-	else if (ret != 0)
-		dev_err(&i2c_client->dev,
-			"Failed to request IRQ: %d\n", ret);
+	/* Request IRQ if one was specified */
+	if (i2c_client->irq) {
+		ret = devm_request_threaded_irq(&i2c_client->dev,
+						i2c_client->irq,
+						NULL, cs42l42_irq_thread,
+						IRQF_ONESHOT | IRQF_TRIGGER_LOW,
+						"cs42l42", cs42l42);
+		if (ret == -EPROBE_DEFER) {
+			goto err_disable;
+		} else if (ret != 0) {
+			dev_err(&i2c_client->dev,
+				"Failed to request IRQ: %d\n", ret);
+			goto err_disable;
+		}
+	}
 
 	/* initialize codec */
 	devid = cirrus_read_device_id(cs42l42->regmap, CS42L42_DEVID_AB);

@@ -2163,7 +2167,9 @@ static int cs42l42_i2c_remove(struct i2c_client *i2c_client)
 {
 	struct cs42l42_private *cs42l42 = i2c_get_clientdata(i2c_client);
 
-	devm_free_irq(&i2c_client->dev, i2c_client->irq, cs42l42);
+	if (i2c_client->irq)
+		devm_free_irq(&i2c_client->dev, i2c_client->irq, cs42l42);
+
 	pm_runtime_suspend(&i2c_client->dev);
 	pm_runtime_disable(&i2c_client->dev);
Signature · 1 line
2.11.0
author maintainer
#19 lore raw

PATCH 16/16 ASoC: cs42l42: Always enable TS_PLUG and TS_UNPLUG interrupts

In reply to Richard Fitzgerald

The headset type detection must run to set the analogue switches correctly for the attached headset type. Without this only headsets with wiring matching the chip default will have a functioning mic.

commit c26a5289e865 ("ASoC: cs42l42: Add support for set_jack calls") moved the interrupt unmasking to the component set_jack() callback. But it's not mandatory for a machine driver to register a struct snd_soc_jack handler. Without a registered handler the type detection would not have run and so the mic would not work on some types of headset.

This patch restores the unmasking of TS_PLUG and TS_UNPLUG interrupts during probe.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/cs42l42.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
sound/soc/codecs/cs42l42.c +2 -8
index 8f0c58097d64..d30bb9ad4191 100644
@@ -524,12 +524,6 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
 
 	cs42l42->jack = jk;
 
-	regmap_update_bits(cs42l42->regmap, CS42L42_TSRS_PLUG_INT_MASK,
-			   CS42L42_RS_PLUG_MASK | CS42L42_RS_UNPLUG_MASK |
-			   CS42L42_TS_PLUG_MASK | CS42L42_TS_UNPLUG_MASK,
-			   (1 << CS42L42_RS_PLUG_SHIFT) | (1 << CS42L42_RS_UNPLUG_SHIFT) |
-			   (0 << CS42L42_TS_PLUG_SHIFT) | (0 << CS42L42_TS_UNPLUG_SHIFT));
-
 	return 0;
 }
 

@@ -1798,8 +1792,8 @@ static void cs42l42_set_interrupt_masks(struct cs42l42_private *cs42l42)
 			CS42L42_TS_UNPLUG_MASK,
 			(1 << CS42L42_RS_PLUG_SHIFT) |
 			(1 << CS42L42_RS_UNPLUG_SHIFT) |
-			(1 << CS42L42_TS_PLUG_SHIFT) |
-			(1 << CS42L42_TS_UNPLUG_SHIFT));
+			(0 << CS42L42_TS_PLUG_SHIFT) |
+			(0 << CS42L42_TS_UNPLUG_SHIFT));
 }
 
 static void cs42l42_setup_hs_type_detect(struct cs42l42_private *cs42l42)
Signature · 1 line
2.11.0
maintainer
#20 lore raw

Re: (subset) [PATCH 00/16] ASoC: cs42l42: Collection of bugfixes

In reply to Richard Fitzgerald
Quoted 11 lines · Richard Fitzgerald

This patch set contains various bugfixes for the cs42l42 codec driver.

Patches marked "Fixes" will apply cleanly to the patch that first introduced the bug.

Patches NOT marked "Fixes" will not apply cleanly to the point the bug was first introduced and/or the bug is not having sufficient impact to risk churning older code versions.

[...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[01/16] ASoC: cs42l42: Don't reconfigure the PLL while it is running

        commit: 06441c82f0cd836402ca5fa4162d28ed07cfb0ed
[02/16] ASoC: cs42l42: Always configure both ASP TX channels
        commit: 6e6825801ab926360f7f4f2dbcfd107d5ab8f025
[03/16] ASoC: cs42l42: Correct some register default values
        commit: d591d4b32aa9552af14a0c7c586a2d3fe9ecc6e0
[04/16] ASoC: cs42l42: Don't set defaults for volatile registers
        commit: 917d5758014b37cf97b946dd130aad9353c354dc
[05/16] ASoC: cs42l42: Defer probe if request_threaded_irq() returns EPROBE_DEFER
        commit: 0306988789d9d91a18ff70bd2bf165d3ae0ef1dd
[10/16] ASoC: cs42l42: Don't claim to support 192k
        commit: 2a031a99428bafba089437e9044b8fd5dc6e7551
[11/16] ASoC: cs42l42: Use PLL for SCLK > 12.288MHz
        commit: 3c211cb7db2905221f9f006aa66b8af17bfcd480
[12/16] ASoC: cs42l42: Allow time for HP/ADC to power-up after enable
        commit: 4ae1d8f911d6fc20baefd5eb061bf6964fa22a32
[13/16] ASoC: cs42l42: Set correct SRC MCLK
        commit: fdbd256175a1e11c1ba827112d56b9a3952e1219
[14/16] ASoC: cs42l42: Mark OSC_SWITCH_STATUS register volatile
        commit: 0c3d6c6ff75aa6b21cd4ac872dd3050b6525c75c
[15/16] ASoC: cs42l42: Fix WARN in remove() if running without an interrupt
        commit: 4c8d49bc476c7cf1fb7377b469ced43ced470027
[16/16] ASoC: cs42l42: Always enable TS_PLUG and TS_UNPLUG interrupts
        commit: 4ca239f33737198827c7f4ac68a1f6fc8a9d79ba

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark