* [PATCH 01/12] ASoC: cs42l42: Add SOFT_RESET_REBOOT register
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 02/12] ASoC: cs42l42: Add bitclock frequency argument to cs42l42_pll_config() Richard Fitzgerald
` (11 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
The SOFT_RESET_REBOOT register is needed to recover CS42L42 state after
a Soundwire bus reset.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
include/sound/cs42l42.h | 5 +++++
sound/soc/codecs/cs42l42.c | 2 ++
2 files changed, 7 insertions(+)
diff --git a/include/sound/cs42l42.h b/include/sound/cs42l42.h
index a55d522f1772..9cf4baabea52 100644
--- a/include/sound/cs42l42.h
+++ b/include/sound/cs42l42.h
@@ -34,6 +34,7 @@
#define CS42L42_PAGE_24 0x2400
#define CS42L42_PAGE_25 0x2500
#define CS42L42_PAGE_26 0x2600
+#define CS42L42_PAGE_27 0x2700
#define CS42L42_PAGE_28 0x2800
#define CS42L42_PAGE_29 0x2900
#define CS42L42_PAGE_2A 0x2A00
@@ -719,6 +720,10 @@
#define CS42L42_SRC_SDOUT_FS (CS42L42_PAGE_26 + 0x09)
+/* Page 0x27 DMA */
+#define CS42L42_SOFT_RESET_REBOOT (CS42L42_PAGE_27 + 0x01)
+#define CS42L42_SFT_RST_REBOOT_MASK BIT(1)
+
/* Page 0x28 S/PDIF Registers */
#define CS42L42_SPDIF_CTL1 (CS42L42_PAGE_28 + 0x01)
#define CS42L42_SPDIF_CTL2 (CS42L42_PAGE_28 + 0x02)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 42cdb051e0fb..440d414efe0a 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -294,6 +294,7 @@ static bool cs42l42_readable_register(struct device *dev, unsigned int reg)
case CS42L42_SPDIF_SW_CTL1:
case CS42L42_SRC_SDIN_FS:
case CS42L42_SRC_SDOUT_FS:
+ case CS42L42_SOFT_RESET_REBOOT:
case CS42L42_SPDIF_CTL1:
case CS42L42_SPDIF_CTL2:
case CS42L42_SPDIF_CTL3:
@@ -358,6 +359,7 @@ static bool cs42l42_volatile_register(struct device *dev, unsigned int reg)
case CS42L42_LOAD_DET_DONE:
case CS42L42_DET_STATUS1:
case CS42L42_DET_STATUS2:
+ case CS42L42_SOFT_RESET_REBOOT:
return true;
default:
return false;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 02/12] ASoC: cs42l42: Add bitclock frequency argument to cs42l42_pll_config()
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 01/12] ASoC: cs42l42: Add SOFT_RESET_REBOOT register Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 03/12] ASoC: cs42l42: Ensure MCLKint is a multiple of the sample rate Richard Fitzgerald
` (10 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
Clean up the handling of bitclock frequency by keeping all the logic
in cs42l42_pcm_hw_params(), which then simply passes the frequency as
an argument to cs42l42_pll_config().
The previous code had become clunky as a legacy of earlier versions of
the clock handling. The logic was split across cs42l42_pcm_hw_params()
and cs42l42_pll_config(), with the params-derived bclk stashed in
struct cs42l42_private only to pass it to cs42l42_pll_config().
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs42l42.c | 32 ++++++++++++++++----------------
sound/soc/codecs/cs42l42.h | 1 -
2 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 440d414efe0a..1745b83310ac 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -649,18 +649,12 @@ static const struct cs42l42_pll_params pll_ratio_table[] = {
{ 24576000, 1, 0x03, 0x40, 0x000000, 0x03, 0x10, 12288000, 128, 1}
};
-static int cs42l42_pll_config(struct snd_soc_component *component)
+static int cs42l42_pll_config(struct snd_soc_component *component, unsigned int clk)
{
struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
int i;
- u32 clk;
u32 fsync;
- if (!cs42l42->sclk)
- clk = cs42l42->bclk;
- 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)
@@ -897,19 +891,25 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
unsigned int width = (params_width(params) / 8) - 1;
unsigned int slot_width = 0;
unsigned int val = 0;
+ unsigned int bclk;
int ret;
cs42l42->srate = params_rate(params);
- /*
- * Assume 24-bit samples are in 32-bit slots, to prevent SCLK being
- * more than assumed (which would result in overclocking).
- */
- if (params_width(params) == 24)
- slot_width = 32;
+ if (cs42l42->sclk) {
+ /* machine driver has set the SCLK */
+ bclk = cs42l42->sclk;
+ } else {
+ /*
+ * Assume 24-bit samples are in 32-bit slots, to prevent SCLK being
+ * more than assumed (which would result in overclocking).
+ */
+ if (params_width(params) == 24)
+ slot_width = 32;
- /* I2S frame always has multiple of 2 channels */
- cs42l42->bclk = snd_soc_tdm_params_to_bclk(params, slot_width, 0, 2);
+ /* I2S frame always has multiple of 2 channels */
+ bclk = snd_soc_tdm_params_to_bclk(params, slot_width, 0, 2);
+ }
switch (substream->stream) {
case SNDRV_PCM_STREAM_CAPTURE:
@@ -949,7 +949,7 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
break;
}
- ret = cs42l42_pll_config(component);
+ ret = cs42l42_pll_config(component, bclk);
if (ret)
return ret;
diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
index 50299c9f283a..b4ba1467c558 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -30,7 +30,6 @@ struct cs42l42_private {
struct snd_soc_jack *jack;
struct mutex irq_lock;
int pll_config;
- int bclk;
u32 sclk;
u32 srate;
u8 plug_state;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 03/12] ASoC: cs42l42: Ensure MCLKint is a multiple of the sample rate
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 01/12] ASoC: cs42l42: Add SOFT_RESET_REBOOT register Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 02/12] ASoC: cs42l42: Add bitclock frequency argument to cs42l42_pll_config() Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 04/12] ASoC: cs42l42: Separate ASP config from PLL config Richard Fitzgerald
` (9 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
The chosen clocking configuration must give an internal MCLK (MCLKint)
that is an integer multiple of the sample rate.
On I2S each of the supported bit clock frequencies can only be generated
from one sample rate group (either the 44100 or the 48000) so the code
could use only the bitclock to look up a PLL config.
The relationship between sample rate and bitclock frequency is more
complex on Soundwire and so it is possible to set a frame shape to
generate a bitclock from the "wrong" group. For example 2*147 with a
48000 sample rate would give a bitclock of 14112000 which on I2S
could only be derived from a 44100 sample rate.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs42l42.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 1745b83310ac..66c10d24169d 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -649,7 +649,8 @@ static const struct cs42l42_pll_params pll_ratio_table[] = {
{ 24576000, 1, 0x03, 0x40, 0x000000, 0x03, 0x10, 12288000, 128, 1}
};
-static int cs42l42_pll_config(struct snd_soc_component *component, unsigned int clk)
+static int cs42l42_pll_config(struct snd_soc_component *component, unsigned int clk,
+ unsigned int sample_rate)
{
struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
int i;
@@ -664,6 +665,10 @@ static int cs42l42_pll_config(struct snd_soc_component *component, unsigned int
}
for (i = 0; i < ARRAY_SIZE(pll_ratio_table); i++) {
+ /* MCLKint must be a multiple of the sample rate */
+ if (pll_ratio_table[i].mclk_int % sample_rate)
+ continue;
+
if (pll_ratio_table[i].sclk == clk) {
cs42l42->pll_config = i;
@@ -889,6 +894,7 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
unsigned int channels = params_channels(params);
unsigned int width = (params_width(params) / 8) - 1;
+ unsigned int sample_rate = params_rate(params);
unsigned int slot_width = 0;
unsigned int val = 0;
unsigned int bclk;
@@ -949,11 +955,11 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
break;
}
- ret = cs42l42_pll_config(component, bclk);
+ ret = cs42l42_pll_config(component, bclk, sample_rate);
if (ret)
return ret;
- cs42l42_src_config(component, params_rate(params));
+ cs42l42_src_config(component, sample_rate);
return 0;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 04/12] ASoC: cs42l42: Separate ASP config from PLL config
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (2 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 03/12] ASoC: cs42l42: Ensure MCLKint is a multiple of the sample rate Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 05/12] ASoC: cs42l42: Use cs42l42->dev instead of &i2c_client->dev Richard Fitzgerald
` (8 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
Setup of the ASP (audio serial port) was being done as a side-effect of
cs42l42_pll_config() and forces a restriction on the ratio of sample_rate
to bit_clock that is invalid for Soundwire.
Move the ASP setup into a dedicated function.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs42l42.c | 81 +++++++++++++++++++++-----------------
sound/soc/codecs/cs42l42.h | 1 -
2 files changed, 44 insertions(+), 38 deletions(-)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 66c10d24169d..946b2a935256 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -654,7 +654,6 @@ static int cs42l42_pll_config(struct snd_soc_component *component, unsigned int
{
struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
int i;
- u32 fsync;
/* Don't reconfigure if there is an audio stream running */
if (cs42l42->stream_use) {
@@ -680,40 +679,6 @@ static int cs42l42_pll_config(struct snd_soc_component *component, unsigned int
(pll_ratio_table[i].mclk_int !=
24000000)) <<
CS42L42_INTERNAL_FS_SHIFT);
-
- /* Set up the LRCLK */
- fsync = clk / cs42l42->srate;
- if (((fsync * cs42l42->srate) != clk)
- || ((fsync % 2) != 0)) {
- dev_err(component->dev,
- "Unsupported sclk %d/sample rate %d\n",
- clk,
- cs42l42->srate);
- return -EINVAL;
- }
- /* Set the LRCLK period */
- snd_soc_component_update_bits(component,
- CS42L42_FSYNC_P_LOWER,
- CS42L42_FSYNC_PERIOD_MASK,
- CS42L42_FRAC0_VAL(fsync - 1) <<
- CS42L42_FSYNC_PERIOD_SHIFT);
- snd_soc_component_update_bits(component,
- CS42L42_FSYNC_P_UPPER,
- CS42L42_FSYNC_PERIOD_MASK,
- CS42L42_FRAC1_VAL(fsync - 1) <<
- CS42L42_FSYNC_PERIOD_SHIFT);
- /* Set the LRCLK to 50% duty cycle */
- fsync = fsync / 2;
- snd_soc_component_update_bits(component,
- CS42L42_FSYNC_PW_LOWER,
- CS42L42_FSYNC_PULSE_WIDTH_MASK,
- CS42L42_FRAC0_VAL(fsync - 1) <<
- CS42L42_FSYNC_PULSE_WIDTH_SHIFT);
- snd_soc_component_update_bits(component,
- CS42L42_FSYNC_PW_UPPER,
- CS42L42_FSYNC_PULSE_WIDTH_MASK,
- CS42L42_FRAC1_VAL(fsync - 1) <<
- CS42L42_FSYNC_PULSE_WIDTH_SHIFT);
if (pll_ratio_table[i].mclk_src_sel == 0) {
/* Pass the clock straight through */
snd_soc_component_update_bits(component,
@@ -805,6 +770,46 @@ static void cs42l42_src_config(struct snd_soc_component *component, unsigned int
fs << CS42L42_CLK_OASRC_SEL_SHIFT);
}
+static int cs42l42_asp_config(struct snd_soc_component *component,
+ unsigned int sclk, unsigned int sample_rate)
+{
+ u32 fsync = sclk / sample_rate;
+
+ /* Set up the LRCLK */
+ if (((fsync * sample_rate) != sclk) || ((fsync % 2) != 0)) {
+ dev_err(component->dev,
+ "Unsupported sclk %d/sample rate %d\n",
+ sclk,
+ sample_rate);
+ return -EINVAL;
+ }
+ /* Set the LRCLK period */
+ snd_soc_component_update_bits(component,
+ CS42L42_FSYNC_P_LOWER,
+ CS42L42_FSYNC_PERIOD_MASK,
+ CS42L42_FRAC0_VAL(fsync - 1) <<
+ CS42L42_FSYNC_PERIOD_SHIFT);
+ snd_soc_component_update_bits(component,
+ CS42L42_FSYNC_P_UPPER,
+ CS42L42_FSYNC_PERIOD_MASK,
+ CS42L42_FRAC1_VAL(fsync - 1) <<
+ CS42L42_FSYNC_PERIOD_SHIFT);
+ /* Set the LRCLK to 50% duty cycle */
+ fsync = fsync / 2;
+ snd_soc_component_update_bits(component,
+ CS42L42_FSYNC_PW_LOWER,
+ CS42L42_FSYNC_PULSE_WIDTH_MASK,
+ CS42L42_FRAC0_VAL(fsync - 1) <<
+ CS42L42_FSYNC_PULSE_WIDTH_SHIFT);
+ snd_soc_component_update_bits(component,
+ CS42L42_FSYNC_PW_UPPER,
+ CS42L42_FSYNC_PULSE_WIDTH_MASK,
+ CS42L42_FRAC1_VAL(fsync - 1) <<
+ CS42L42_FSYNC_PULSE_WIDTH_SHIFT);
+
+ return 0;
+}
+
static int cs42l42_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
{
struct snd_soc_component *component = codec_dai->component;
@@ -900,8 +905,6 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
unsigned int bclk;
int ret;
- cs42l42->srate = params_rate(params);
-
if (cs42l42->sclk) {
/* machine driver has set the SCLK */
bclk = cs42l42->sclk;
@@ -959,6 +962,10 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
if (ret)
return ret;
+ ret = cs42l42_asp_config(component, bclk, sample_rate);
+ if (ret)
+ return ret;
+
cs42l42_src_config(component, sample_rate);
return 0;
diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
index b4ba1467c558..2e4e01f9e389 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -31,7 +31,6 @@ struct cs42l42_private {
struct mutex irq_lock;
int pll_config;
u32 sclk;
- u32 srate;
u8 plug_state;
u8 hs_type;
u8 ts_inv;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 05/12] ASoC: cs42l42: Use cs42l42->dev instead of &i2c_client->dev
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (3 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 04/12] ASoC: cs42l42: Separate ASP config from PLL config Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 06/12] ASoC: cs42l42: Split probe() and remove() into stages Richard Fitzgerald
` (7 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
In preparation for splitting cs42l42_i2c_probe() into multiple functions
replace use of &i2c_client->dev with cs42l42->dev. This reduces diff
clutter in the patch that splits the function.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs42l42.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 946b2a935256..4c81bda6b593 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -2233,7 +2233,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
cs42l42->regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap);
if (IS_ERR(cs42l42->regmap)) {
ret = PTR_ERR(cs42l42->regmap);
- dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret);
+ dev_err(cs42l42->dev, "regmap_init() failed: %d\n", ret);
return ret;
}
@@ -2241,11 +2241,11 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
for (i = 0; i < ARRAY_SIZE(cs42l42->supplies); i++)
cs42l42->supplies[i].supply = cs42l42_supply_names[i];
- ret = devm_regulator_bulk_get(&i2c_client->dev,
+ ret = devm_regulator_bulk_get(cs42l42->dev,
ARRAY_SIZE(cs42l42->supplies),
cs42l42->supplies);
if (ret != 0) {
- dev_err(&i2c_client->dev,
+ dev_err(cs42l42->dev,
"Failed to request supplies: %d\n", ret);
return ret;
}
@@ -2253,13 +2253,13 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
ret = regulator_bulk_enable(ARRAY_SIZE(cs42l42->supplies),
cs42l42->supplies);
if (ret != 0) {
- dev_err(&i2c_client->dev,
+ dev_err(cs42l42->dev,
"Failed to enable supplies: %d\n", ret);
return ret;
}
/* Reset the Device */
- cs42l42->reset_gpio = devm_gpiod_get_optional(&i2c_client->dev,
+ cs42l42->reset_gpio = devm_gpiod_get_optional(cs42l42->dev,
"reset", GPIOD_OUT_LOW);
if (IS_ERR(cs42l42->reset_gpio)) {
ret = PTR_ERR(cs42l42->reset_gpio);
@@ -2267,7 +2267,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
}
if (cs42l42->reset_gpio) {
- dev_dbg(&i2c_client->dev, "Found reset GPIO\n");
+ dev_dbg(cs42l42->dev, "Found reset GPIO\n");
gpiod_set_value_cansleep(cs42l42->reset_gpio, 1);
}
usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
@@ -2281,7 +2281,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
if (ret == -EPROBE_DEFER) {
goto err_disable_noirq;
} else if (ret != 0) {
- dev_err(&i2c_client->dev,
+ dev_err(cs42l42->dev,
"Failed to request IRQ: %d\n", ret);
goto err_disable_noirq;
}
@@ -2291,13 +2291,13 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
devid = cirrus_read_device_id(cs42l42->regmap, CS42L42_DEVID_AB);
if (devid < 0) {
ret = devid;
- dev_err(&i2c_client->dev, "Failed to read device ID: %d\n", ret);
+ dev_err(cs42l42->dev, "Failed to read device ID: %d\n", ret);
goto err_disable;
}
if (devid != CS42L42_CHIP_ID) {
ret = -ENODEV;
- dev_err(&i2c_client->dev,
+ dev_err(cs42l42->dev,
"CS42L42 Device ID (%X). Expected %X\n",
devid, CS42L42_CHIP_ID);
goto err_disable;
@@ -2305,11 +2305,11 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
ret = regmap_read(cs42l42->regmap, CS42L42_REVID, ®);
if (ret < 0) {
- dev_err(&i2c_client->dev, "Get Revision ID failed\n");
+ dev_err(cs42l42->dev, "Get Revision ID failed\n");
goto err_shutdown;
}
- dev_info(&i2c_client->dev,
+ dev_info(cs42l42->dev,
"Cirrus Logic CS42L42, Revision: %02X\n", reg & 0xFF);
/* Power up the codec */
@@ -2329,7 +2329,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
(1 << CS42L42_ADC_PDN_SHIFT) |
(0 << CS42L42_PDN_ALL_SHIFT));
- ret = cs42l42_handle_device_data(&i2c_client->dev, cs42l42);
+ ret = cs42l42_handle_device_data(cs42l42->dev, cs42l42);
if (ret != 0)
goto err_shutdown;
@@ -2340,7 +2340,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
cs42l42_set_interrupt_masks(cs42l42);
/* Register codec for machine driver */
- ret = devm_snd_soc_register_component(&i2c_client->dev,
+ ret = devm_snd_soc_register_component(cs42l42->dev,
&soc_component_dev_cs42l42, &cs42l42_dai, 1);
if (ret < 0)
goto err_shutdown;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 06/12] ASoC: cs42l42: Split probe() and remove() into stages
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (4 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 05/12] ASoC: cs42l42: Use cs42l42->dev instead of &i2c_client->dev Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 07/12] ASoC: cs42l42: Split cs42l42_resume into two functions Richard Fitzgerald
` (6 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
To prepare for adding Soundwire the probe must be split into three
parts:
1) The bus-specific probe
2) Common bus-agnostic probe steps
3) Initialization of the peripheral registers
Step (3) must be separate because on Soundwire devices the probe must
enable power supplies and release reset so that the peripheral can be
enumerated by the bus, but it isn't possible to access registers until
enumeration has completed.
The call to devm_snd_soc_register_component() must be done at stage (2)
so that it can EPROBE_DEFER if necessary. In Soundwire systems stage (3)
is not a probe event so a deferral at this stage would not result in
re-probing dependencies.
A new init_done flag indicates that the chip has been identified and
initialized. This is used to prevent cs42l42_remove(), cs42l42_suspend(),
cs42l42_restore() and cs42l42_irq_thread() from attempting register
accesses if the chip was not successfully initialized. Although this
cannot happen on I2C, because the entire probe would fail, it is
possible on Soundwire if probe succeeds but the cs42l42 is never
enumerated.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs42l42.c | 122 +++++++++++++++++++++++++------------
sound/soc/codecs/cs42l42.h | 2 +
2 files changed, 84 insertions(+), 40 deletions(-)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 4c81bda6b593..8c5909a43df6 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -1641,7 +1641,7 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data)
unsigned int i;
mutex_lock(&cs42l42->irq_lock);
- if (cs42l42->suspended) {
+ if (cs42l42->suspended || !cs42l42->init_done) {
mutex_unlock(&cs42l42->irq_lock);
return IRQ_NONE;
}
@@ -2215,28 +2215,13 @@ static int __maybe_unused cs42l42_resume(struct device *dev)
return 0;
}
-static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
+static int cs42l42_common_probe(struct cs42l42_private *cs42l42)
{
- struct cs42l42_private *cs42l42;
- int ret, i, devid;
- unsigned int reg;
-
- cs42l42 = devm_kzalloc(&i2c_client->dev, sizeof(struct cs42l42_private),
- GFP_KERNEL);
- if (!cs42l42)
- return -ENOMEM;
+ int ret, i;
- cs42l42->dev = &i2c_client->dev;
- i2c_set_clientdata(i2c_client, cs42l42);
+ dev_set_drvdata(cs42l42->dev, cs42l42);
mutex_init(&cs42l42->irq_lock);
- cs42l42->regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap);
- if (IS_ERR(cs42l42->regmap)) {
- ret = PTR_ERR(cs42l42->regmap);
- dev_err(cs42l42->dev, "regmap_init() failed: %d\n", ret);
- return ret;
- }
-
BUILD_BUG_ON(ARRAY_SIZE(cs42l42_supply_names) != ARRAY_SIZE(cs42l42->supplies));
for (i = 0; i < ARRAY_SIZE(cs42l42->supplies); i++)
cs42l42->supplies[i].supply = cs42l42_supply_names[i];
@@ -2273,8 +2258,8 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
/* Request IRQ if one was specified */
- if (i2c_client->irq) {
- ret = request_threaded_irq(i2c_client->irq,
+ if (cs42l42->irq) {
+ ret = request_threaded_irq(cs42l42->irq,
NULL, cs42l42_irq_thread,
IRQF_ONESHOT | IRQF_TRIGGER_LOW,
"cs42l42", cs42l42);
@@ -2287,6 +2272,32 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
}
}
+ /* Register codec now so it can EPROBE_DEFER */
+ ret = devm_snd_soc_register_component(cs42l42->dev,
+ &soc_component_dev_cs42l42,
+ &cs42l42_dai, 1);
+ if (ret < 0)
+ goto err;
+
+ return 0;
+
+err:
+ if (cs42l42->irq)
+ free_irq(cs42l42->irq, cs42l42);
+
+err_disable_noirq:
+ gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
+err_disable_noreset:
+ regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies);
+
+ return ret;
+}
+
+static int cs42l42_init(struct cs42l42_private *cs42l42)
+{
+ unsigned int reg;
+ int devid, ret;
+
/* initialize codec */
devid = cirrus_read_device_id(cs42l42->regmap, CS42L42_DEVID_AB);
if (devid < 0) {
@@ -2336,15 +2347,15 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
/* Setup headset detection */
cs42l42_setup_hs_type_detect(cs42l42);
+ /*
+ * Set init_done before unmasking interrupts so any triggered
+ * immediately will be handled.
+ */
+ cs42l42->init_done = true;
+
/* Mask/Unmask Interrupts */
cs42l42_set_interrupt_masks(cs42l42);
- /* Register codec for machine driver */
- ret = devm_snd_soc_register_component(cs42l42->dev,
- &soc_component_dev_cs42l42, &cs42l42_dai, 1);
- if (ret < 0)
- goto err_shutdown;
-
return 0;
err_shutdown:
@@ -2353,34 +2364,65 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
regmap_write(cs42l42->regmap, CS42L42_PWR_CTL1, 0xff);
err_disable:
- if (i2c_client->irq)
- free_irq(i2c_client->irq, cs42l42);
-
-err_disable_noirq:
gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
-err_disable_noreset:
regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies),
cs42l42->supplies);
return ret;
}
-static int cs42l42_i2c_remove(struct i2c_client *i2c_client)
+static void cs42l42_common_remove(struct cs42l42_private *cs42l42)
{
- struct cs42l42_private *cs42l42 = i2c_get_clientdata(i2c_client);
-
- if (i2c_client->irq)
- free_irq(i2c_client->irq, cs42l42);
+ if (cs42l42->irq)
+ free_irq(cs42l42->irq, cs42l42);
/*
* The driver might not have control of reset and power supplies,
* so ensure that the chip internals are powered down.
*/
- regmap_write(cs42l42->regmap, CS42L42_CODEC_INT_MASK, 0xff);
- regmap_write(cs42l42->regmap, CS42L42_TSRS_PLUG_INT_MASK, 0xff);
- regmap_write(cs42l42->regmap, CS42L42_PWR_CTL1, 0xff);
+ if (cs42l42->init_done) {
+ regmap_write(cs42l42->regmap, CS42L42_CODEC_INT_MASK, 0xff);
+ regmap_write(cs42l42->regmap, CS42L42_TSRS_PLUG_INT_MASK, 0xff);
+ regmap_write(cs42l42->regmap, CS42L42_PWR_CTL1, 0xff);
+ }
gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies);
+}
+
+static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
+{
+ struct device *dev = &i2c_client->dev;
+ struct cs42l42_private *cs42l42;
+ struct regmap *regmap;
+ int ret;
+
+ cs42l42 = devm_kzalloc(dev, sizeof(struct cs42l42_private), GFP_KERNEL);
+ if (!cs42l42)
+ return -ENOMEM;
+
+ regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap);
+ if (IS_ERR(regmap)) {
+ ret = PTR_ERR(regmap);
+ dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret);
+ return ret;
+ }
+
+ cs42l42->dev = dev;
+ cs42l42->regmap = regmap;
+ cs42l42->irq = i2c_client->irq;
+
+ ret = cs42l42_common_probe(cs42l42);
+ if (ret)
+ return ret;
+
+ return cs42l42_init(cs42l42);
+}
+
+static int cs42l42_i2c_remove(struct i2c_client *i2c_client)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(&i2c_client->dev);
+
+ cs42l42_common_remove(cs42l42);
return 0;
}
diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
index 2e4e01f9e389..4ee61136080d 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -29,6 +29,7 @@ struct cs42l42_private {
struct completion pdn_done;
struct snd_soc_jack *jack;
struct mutex irq_lock;
+ int irq;
int pll_config;
u32 sclk;
u8 plug_state;
@@ -45,6 +46,7 @@ struct cs42l42_private {
u8 stream_use;
bool hp_adc_up_pending;
bool suspended;
+ bool init_done;
};
#endif /* __CS42L42_H__ */
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 07/12] ASoC: cs42l42: Split cs42l42_resume into two functions
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (5 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 06/12] ASoC: cs42l42: Split probe() and remove() into stages Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 08/12] ASoC: cs42l42: Pass component and dai defs into common probe Richard Fitzgerald
` (5 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
On Soundwire the system resume cannot restore registers until the
host controller has re-enumerated the peripheral.
This patch splits cs42l42_resume() into two functions, one to
power up and the other to restore registers, ready for adding
Soundwire support.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs42l42.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 8c5909a43df6..54fe92b3d4da 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -2199,6 +2199,15 @@ static int __maybe_unused cs42l42_resume(struct device *dev)
gpiod_set_value_cansleep(cs42l42->reset_gpio, 1);
usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
+ dev_dbg(dev, "System resume powered up\n");
+
+ return 0;
+}
+
+static void __maybe_unused cs42l42_resume_restore(struct device *dev)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
+
regcache_cache_only(cs42l42->regmap, false);
regcache_mark_dirty(cs42l42->regmap);
@@ -2211,6 +2220,17 @@ static int __maybe_unused cs42l42_resume(struct device *dev)
mutex_unlock(&cs42l42->irq_lock);
dev_dbg(dev, "System resumed\n");
+}
+
+static int __maybe_unused cs42l42_i2c_resume(struct device *dev)
+{
+ int ret;
+
+ ret = cs42l42_resume(dev);
+ if (ret)
+ return ret;
+
+ cs42l42_resume_restore(dev);
return 0;
}
@@ -2428,7 +2448,7 @@ static int cs42l42_i2c_remove(struct i2c_client *i2c_client)
}
static const struct dev_pm_ops cs42l42_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(cs42l42_suspend, cs42l42_resume)
+ SET_SYSTEM_SLEEP_PM_OPS(cs42l42_suspend, cs42l42_i2c_resume)
};
#ifdef CONFIG_OF
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 08/12] ASoC: cs42l42: Pass component and dai defs into common probe
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (6 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 07/12] ASoC: cs42l42: Split cs42l42_resume into two functions Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 09/12] ASoC: cs42l42: Split I2C identity into separate module Richard Fitzgerald
` (4 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
Pass pointers to snd_soc_component_driver and snd_soc_dai_driver
objects into cs42l42_common_probe().
This is in preparation for adding Soundwire support.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs42l42.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 54fe92b3d4da..a98b4e6a1f05 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -581,7 +581,7 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
return 0;
}
-static const struct snd_soc_component_driver soc_component_dev_cs42l42 = {
+static const struct snd_soc_component_driver cs42l42_soc_component = {
.set_jack = cs42l42_set_jack,
.dapm_widgets = cs42l42_dapm_widgets,
.num_dapm_widgets = ARRAY_SIZE(cs42l42_dapm_widgets),
@@ -2235,7 +2235,9 @@ static int __maybe_unused cs42l42_i2c_resume(struct device *dev)
return 0;
}
-static int cs42l42_common_probe(struct cs42l42_private *cs42l42)
+static int cs42l42_common_probe(struct cs42l42_private *cs42l42,
+ const struct snd_soc_component_driver *component_drv,
+ struct snd_soc_dai_driver *dai)
{
int ret, i;
@@ -2293,9 +2295,7 @@ static int cs42l42_common_probe(struct cs42l42_private *cs42l42)
}
/* Register codec now so it can EPROBE_DEFER */
- ret = devm_snd_soc_register_component(cs42l42->dev,
- &soc_component_dev_cs42l42,
- &cs42l42_dai, 1);
+ ret = devm_snd_soc_register_component(cs42l42->dev, component_drv, dai, 1);
if (ret < 0)
goto err;
@@ -2431,7 +2431,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
cs42l42->regmap = regmap;
cs42l42->irq = i2c_client->irq;
- ret = cs42l42_common_probe(cs42l42);
+ ret = cs42l42_common_probe(cs42l42, &cs42l42_soc_component, &cs42l42_dai);
if (ret)
return ret;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 09/12] ASoC: cs42l42: Split I2C identity into separate module
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (7 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 08/12] ASoC: cs42l42: Pass component and dai defs into common probe Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 10/12] ASoC: cs42l42: Export some functions for Soundwire Richard Fitzgerald
` (3 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
Split the I2C bus driver definition and probe()/remove() into a
separate module so that a Soundwire build of CS42L42 support does
not have a spurious dependency on I2C.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/Kconfig | 8 ++-
sound/soc/codecs/Makefile | 4 +-
sound/soc/codecs/cs42l42-i2c.c | 107 +++++++++++++++++++++++++++++++
sound/soc/codecs/cs42l42.c | 111 ++++++---------------------------
sound/soc/codecs/cs42l42.h | 15 +++++
5 files changed, 152 insertions(+), 93 deletions(-)
create mode 100644 sound/soc/codecs/cs42l42-i2c.c
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index d16b4efb88a7..9f6f0f97cfb9 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -690,9 +690,15 @@ config SND_SOC_CS35L45_I2C
Enable support for Cirrus Logic CS35L45 smart speaker amplifier
with I2C control.
+config SND_SOC_CS42L42_CORE
+ tristate
+
config SND_SOC_CS42L42
- tristate "Cirrus Logic CS42L42 CODEC"
+ tristate "Cirrus Logic CS42L42 CODEC (I2C)"
depends on I2C
+ select REGMAP
+ select REGMAP_I2C
+ select SND_SOC_CS42L42_CORE
config SND_SOC_CS42L51
tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 92fd441d426a..d91f3c1fc2b3 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -65,6 +65,7 @@ snd-soc-cs35l45-objs := cs35l45.o
snd-soc-cs35l45-spi-objs := cs35l45-spi.o
snd-soc-cs35l45-i2c-objs := cs35l45-i2c.o
snd-soc-cs42l42-objs := cs42l42.o
+snd-soc-cs42l42-i2c-objs := cs42l42-i2c.o
snd-soc-cs42l51-objs := cs42l51.o
snd-soc-cs42l51-i2c-objs := cs42l51-i2c.o
snd-soc-cs42l52-objs := cs42l52.o
@@ -419,7 +420,8 @@ obj-$(CONFIG_SND_SOC_CS35L45_TABLES) += snd-soc-cs35l45-tables.o
obj-$(CONFIG_SND_SOC_CS35L45) += snd-soc-cs35l45.o
obj-$(CONFIG_SND_SOC_CS35L45_SPI) += snd-soc-cs35l45-spi.o
obj-$(CONFIG_SND_SOC_CS35L45_I2C) += snd-soc-cs35l45-i2c.o
-obj-$(CONFIG_SND_SOC_CS42L42) += snd-soc-cs42l42.o
+obj-$(CONFIG_SND_SOC_CS42L42_CORE) += snd-soc-cs42l42.o
+obj-$(CONFIG_SND_SOC_CS42L42) += snd-soc-cs42l42-i2c.o
obj-$(CONFIG_SND_SOC_CS42L51) += snd-soc-cs42l51.o
obj-$(CONFIG_SND_SOC_CS42L51_I2C) += snd-soc-cs42l51-i2c.o
obj-$(CONFIG_SND_SOC_CS42L52) += snd-soc-cs42l52.o
diff --git a/sound/soc/codecs/cs42l42-i2c.c b/sound/soc/codecs/cs42l42-i2c.c
new file mode 100644
index 000000000000..5f01b6adc17e
--- /dev/null
+++ b/sound/soc/codecs/cs42l42-i2c.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * cs42l42-i2c.c -- CS42L42 ALSA SoC audio driver for I2C
+ *
+ * Copyright 2016, 2022 Cirrus Logic, Inc.
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "cs42l42.h"
+
+static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
+{
+ struct device *dev = &i2c_client->dev;
+ struct cs42l42_private *cs42l42;
+ struct regmap *regmap;
+ int ret;
+
+ cs42l42 = devm_kzalloc(dev, sizeof(*cs42l42), GFP_KERNEL);
+ if (!cs42l42)
+ return -ENOMEM;
+
+ regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap);
+ if (IS_ERR(regmap)) {
+ ret = PTR_ERR(regmap);
+ dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret);
+ return ret;
+ }
+
+ cs42l42->dev = dev;
+ cs42l42->regmap = regmap;
+ cs42l42->irq = i2c_client->irq;
+
+ ret = cs42l42_common_probe(cs42l42, &cs42l42_soc_component, &cs42l42_dai);
+ if (ret)
+ return ret;
+
+ return cs42l42_init(cs42l42);
+}
+
+static int cs42l42_i2c_remove(struct i2c_client *i2c_client)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(&i2c_client->dev);
+
+ cs42l42_common_remove(cs42l42);
+
+ return 0;
+}
+
+static int __maybe_unused cs42l42_i2c_resume(struct device *dev)
+{
+ int ret;
+
+ ret = cs42l42_resume(dev);
+ if (ret)
+ return ret;
+
+ cs42l42_resume_restore(dev);
+
+ return 0;
+}
+
+static const struct dev_pm_ops cs42l42_i2c_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(cs42l42_suspend, cs42l42_i2c_resume)
+};
+
+static const struct of_device_id __maybe_unused cs42l42_of_match[] = {
+ { .compatible = "cirrus,cs42l42", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, cs42l42_of_match);
+
+static const struct acpi_device_id __maybe_unused cs42l42_acpi_match[] = {
+ {"10134242", 0,},
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, cs42l42_acpi_match);
+
+static const struct i2c_device_id cs42l42_id[] = {
+ {"cs42l42", 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, cs42l42_id);
+
+static struct i2c_driver cs42l42_i2c_driver = {
+ .driver = {
+ .name = "cs42l42",
+ .pm = &cs42l42_i2c_pm_ops,
+ .of_match_table = of_match_ptr(cs42l42_of_match),
+ .acpi_match_table = ACPI_PTR(cs42l42_acpi_match),
+ },
+ .id_table = cs42l42_id,
+ .probe_new = cs42l42_i2c_probe,
+ .remove = cs42l42_i2c_remove,
+};
+
+module_i2c_driver(cs42l42_i2c_driver);
+
+MODULE_DESCRIPTION("ASoC CS42L42 I2C driver");
+MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.cirrus.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(SND_SOC_CS42L42_CORE);
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index a98b4e6a1f05..7f16de593424 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -15,7 +15,6 @@
#include <linux/types.h>
#include <linux/init.h>
#include <linux/delay.h>
-#include <linux/i2c.h>
#include <linux/gpio.h>
#include <linux/regmap.h>
#include <linux/slab.h>
@@ -377,7 +376,7 @@ static const struct regmap_range_cfg cs42l42_page_range = {
.window_len = 256,
};
-static const struct regmap_config cs42l42_regmap = {
+const struct regmap_config cs42l42_regmap = {
.reg_bits = 8,
.val_bits = 8,
@@ -395,6 +394,7 @@ static const struct regmap_config cs42l42_regmap = {
.use_single_read = true,
.use_single_write = true,
};
+EXPORT_SYMBOL_NS_GPL(cs42l42_regmap, SND_SOC_CS42L42_CORE);
static DECLARE_TLV_DB_SCALE(adc_tlv, -9700, 100, true);
static DECLARE_TLV_DB_SCALE(mixer_tlv, -6300, 100, true);
@@ -581,7 +581,7 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
return 0;
}
-static const struct snd_soc_component_driver cs42l42_soc_component = {
+const struct snd_soc_component_driver cs42l42_soc_component = {
.set_jack = cs42l42_set_jack,
.dapm_widgets = cs42l42_dapm_widgets,
.num_dapm_widgets = ARRAY_SIZE(cs42l42_dapm_widgets),
@@ -592,6 +592,7 @@ static const struct snd_soc_component_driver cs42l42_soc_component = {
.idle_bias_on = 1,
.endianness = 1,
};
+EXPORT_SYMBOL_NS_GPL(cs42l42_soc_component, SND_SOC_CS42L42_CORE);
/* Switch to SCLK. Atomic delay after the write to allow the switch to complete. */
static const struct reg_sequence cs42l42_to_sclk_seq[] = {
@@ -1101,7 +1102,7 @@ static const struct snd_soc_dai_ops cs42l42_ops = {
.mute_stream = cs42l42_mute_stream,
};
-static struct snd_soc_dai_driver cs42l42_dai = {
+struct snd_soc_dai_driver cs42l42_dai = {
.name = "cs42l42",
.playback = {
.stream_name = "Playback",
@@ -1121,6 +1122,7 @@ static struct snd_soc_dai_driver cs42l42_dai = {
.symmetric_sample_bits = 1,
.ops = &cs42l42_ops,
};
+EXPORT_SYMBOL_NS_GPL(cs42l42_dai, SND_SOC_CS42L42_CORE);
static void cs42l42_manual_hs_type_detect(struct cs42l42_private *cs42l42)
{
@@ -2116,7 +2118,7 @@ static const struct reg_sequence __maybe_unused cs42l42_shutdown_seq[] = {
REG_SEQ0(CS42L42_PWR_CTL1, 0xFF)
};
-static int __maybe_unused cs42l42_suspend(struct device *dev)
+int cs42l42_suspend(struct device *dev)
{
struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
unsigned int reg;
@@ -2176,8 +2178,9 @@ static int __maybe_unused cs42l42_suspend(struct device *dev)
return 0;
}
+EXPORT_SYMBOL_NS_GPL(cs42l42_suspend, SND_SOC_CS42L42_CORE);
-static int __maybe_unused cs42l42_resume(struct device *dev)
+int cs42l42_resume(struct device *dev)
{
struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
int ret;
@@ -2203,8 +2206,9 @@ static int __maybe_unused cs42l42_resume(struct device *dev)
return 0;
}
+EXPORT_SYMBOL_NS_GPL(cs42l42_resume, SND_SOC_CS42L42_CORE);
-static void __maybe_unused cs42l42_resume_restore(struct device *dev)
+void cs42l42_resume_restore(struct device *dev)
{
struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
@@ -2221,6 +2225,7 @@ static void __maybe_unused cs42l42_resume_restore(struct device *dev)
dev_dbg(dev, "System resumed\n");
}
+EXPORT_SYMBOL_NS_GPL(cs42l42_resume_restore, SND_SOC_CS42L42_CORE);
static int __maybe_unused cs42l42_i2c_resume(struct device *dev)
{
@@ -2235,9 +2240,9 @@ static int __maybe_unused cs42l42_i2c_resume(struct device *dev)
return 0;
}
-static int cs42l42_common_probe(struct cs42l42_private *cs42l42,
- const struct snd_soc_component_driver *component_drv,
- struct snd_soc_dai_driver *dai)
+int cs42l42_common_probe(struct cs42l42_private *cs42l42,
+ const struct snd_soc_component_driver *component_drv,
+ struct snd_soc_dai_driver *dai)
{
int ret, i;
@@ -2312,8 +2317,9 @@ static int cs42l42_common_probe(struct cs42l42_private *cs42l42,
return ret;
}
+EXPORT_SYMBOL_NS_GPL(cs42l42_common_probe, SND_SOC_CS42L42_CORE);
-static int cs42l42_init(struct cs42l42_private *cs42l42)
+int cs42l42_init(struct cs42l42_private *cs42l42)
{
unsigned int reg;
int devid, ret;
@@ -2389,8 +2395,9 @@ static int cs42l42_init(struct cs42l42_private *cs42l42)
cs42l42->supplies);
return ret;
}
+EXPORT_SYMBOL_NS_GPL(cs42l42_init, SND_SOC_CS42L42_CORE);
-static void cs42l42_common_remove(struct cs42l42_private *cs42l42)
+void cs42l42_common_remove(struct cs42l42_private *cs42l42)
{
if (cs42l42->irq)
free_irq(cs42l42->irq, cs42l42);
@@ -2408,85 +2415,7 @@ static void cs42l42_common_remove(struct cs42l42_private *cs42l42)
gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies);
}
-
-static int cs42l42_i2c_probe(struct i2c_client *i2c_client)
-{
- struct device *dev = &i2c_client->dev;
- struct cs42l42_private *cs42l42;
- struct regmap *regmap;
- int ret;
-
- cs42l42 = devm_kzalloc(dev, sizeof(struct cs42l42_private), GFP_KERNEL);
- if (!cs42l42)
- return -ENOMEM;
-
- regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap);
- if (IS_ERR(regmap)) {
- ret = PTR_ERR(regmap);
- dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret);
- return ret;
- }
-
- cs42l42->dev = dev;
- cs42l42->regmap = regmap;
- cs42l42->irq = i2c_client->irq;
-
- ret = cs42l42_common_probe(cs42l42, &cs42l42_soc_component, &cs42l42_dai);
- if (ret)
- return ret;
-
- return cs42l42_init(cs42l42);
-}
-
-static int cs42l42_i2c_remove(struct i2c_client *i2c_client)
-{
- struct cs42l42_private *cs42l42 = dev_get_drvdata(&i2c_client->dev);
-
- cs42l42_common_remove(cs42l42);
-
- return 0;
-}
-
-static const struct dev_pm_ops cs42l42_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(cs42l42_suspend, cs42l42_i2c_resume)
-};
-
-#ifdef CONFIG_OF
-static const struct of_device_id cs42l42_of_match[] = {
- { .compatible = "cirrus,cs42l42", },
- {}
-};
-MODULE_DEVICE_TABLE(of, cs42l42_of_match);
-#endif
-
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id cs42l42_acpi_match[] = {
- {"10134242", 0,},
- {}
-};
-MODULE_DEVICE_TABLE(acpi, cs42l42_acpi_match);
-#endif
-
-static const struct i2c_device_id cs42l42_id[] = {
- {"cs42l42", 0},
- {}
-};
-
-MODULE_DEVICE_TABLE(i2c, cs42l42_id);
-
-static struct i2c_driver cs42l42_i2c_driver = {
- .driver = {
- .name = "cs42l42",
- .pm = &cs42l42_pm_ops,
- .of_match_table = of_match_ptr(cs42l42_of_match),
- .acpi_match_table = ACPI_PTR(cs42l42_acpi_match),
- },
- .id_table = cs42l42_id,
- .probe_new = cs42l42_i2c_probe,
- .remove = cs42l42_i2c_remove,
-};
-
-module_i2c_driver(cs42l42_i2c_driver);
+EXPORT_SYMBOL_NS_GPL(cs42l42_common_remove, SND_SOC_CS42L42_CORE);
MODULE_DESCRIPTION("ASoC CS42L42 driver");
MODULE_AUTHOR("James Schulman, Cirrus Logic Inc, <james.schulman@cirrus.com>");
diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
index 4ee61136080d..942054434afd 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -20,6 +20,8 @@
#include <linux/regulator/consumer.h>
#include <sound/jack.h>
#include <sound/cs42l42.h>
+#include <sound/soc-component.h>
+#include <sound/soc-dai.h>
struct cs42l42_private {
struct regmap *regmap;
@@ -49,4 +51,17 @@ struct cs42l42_private {
bool init_done;
};
+extern const struct regmap_config cs42l42_regmap;
+extern const struct snd_soc_component_driver cs42l42_soc_component;
+extern struct snd_soc_dai_driver cs42l42_dai;
+
+int cs42l42_suspend(struct device *dev);
+int cs42l42_resume(struct device *dev);
+void cs42l42_resume_restore(struct device *dev);
+int cs42l42_common_probe(struct cs42l42_private *cs42l42,
+ const struct snd_soc_component_driver *component_drv,
+ struct snd_soc_dai_driver *dai);
+int cs42l42_init(struct cs42l42_private *cs42l42);
+void cs42l42_common_remove(struct cs42l42_private *cs42l42);
+
#endif /* __CS42L42_H__ */
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 10/12] ASoC: cs42l42: Export some functions for Soundwire
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (8 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 09/12] ASoC: cs42l42: Split I2C identity into separate module Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-19 12:52 ` [PATCH 11/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (2 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
Export functions that will be needed by a Soundwire module.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs42l42.c | 14 +++++++++-----
sound/soc/codecs/cs42l42.h | 5 +++++
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 7f16de593424..3a4f65233360 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -650,8 +650,8 @@ static const struct cs42l42_pll_params pll_ratio_table[] = {
{ 24576000, 1, 0x03, 0x40, 0x000000, 0x03, 0x10, 12288000, 128, 1}
};
-static int cs42l42_pll_config(struct snd_soc_component *component, unsigned int clk,
- unsigned int sample_rate)
+int cs42l42_pll_config(struct snd_soc_component *component, unsigned int clk,
+ unsigned int sample_rate)
{
struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
int i;
@@ -737,8 +737,9 @@ static int cs42l42_pll_config(struct snd_soc_component *component, unsigned int
return -EINVAL;
}
+EXPORT_SYMBOL_NS_GPL(cs42l42_pll_config, SND_SOC_CS42L42_CORE);
-static void cs42l42_src_config(struct snd_soc_component *component, unsigned int sample_rate)
+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;
@@ -770,6 +771,7 @@ static void cs42l42_src_config(struct snd_soc_component *component, unsigned int
CS42L42_CLK_OASRC_SEL_MASK,
fs << CS42L42_CLK_OASRC_SEL_SHIFT);
}
+EXPORT_SYMBOL_NS_GPL(cs42l42_src_config, SND_SOC_CS42L42_CORE);
static int cs42l42_asp_config(struct snd_soc_component *component,
unsigned int sclk, unsigned int sample_rate)
@@ -996,7 +998,7 @@ static int cs42l42_set_sysclk(struct snd_soc_dai *dai,
return -EINVAL;
}
-static int cs42l42_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
+int cs42l42_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
{
struct snd_soc_component *component = dai->component;
struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
@@ -1089,6 +1091,7 @@ static int cs42l42_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
return 0;
}
+EXPORT_SYMBOL_NS_GPL(cs42l42_mute_stream, SND_SOC_CS42L42_CORE);
#define CS42L42_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\
SNDRV_PCM_FMTBIT_S24_LE |\
@@ -1633,7 +1636,7 @@ static const struct cs42l42_irq_params irq_params_table[] = {
CS42L42_TSRS_PLUG_VAL_MASK}
};
-static irqreturn_t cs42l42_irq_thread(int irq, void *data)
+irqreturn_t cs42l42_irq_thread(int irq, void *data)
{
struct cs42l42_private *cs42l42 = (struct cs42l42_private *)data;
unsigned int stickies[12];
@@ -1750,6 +1753,7 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data)
return IRQ_HANDLED;
}
+EXPORT_SYMBOL_NS_GPL(cs42l42_irq_thread, SND_SOC_CS42L42_CORE);
static void cs42l42_set_interrupt_masks(struct cs42l42_private *cs42l42)
{
diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
index 942054434afd..f575ef9b5498 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -55,6 +55,11 @@ extern const struct regmap_config cs42l42_regmap;
extern const struct snd_soc_component_driver cs42l42_soc_component;
extern struct snd_soc_dai_driver cs42l42_dai;
+int cs42l42_pll_config(struct snd_soc_component *component,
+ unsigned int clk, unsigned int sample_rate);
+void cs42l42_src_config(struct snd_soc_component *component, unsigned int sample_rate);
+int cs42l42_mute_stream(struct snd_soc_dai *dai, int mute, int stream);
+irqreturn_t cs42l42_irq_thread(int irq, void *data);
int cs42l42_suspend(struct device *dev);
int cs42l42_resume(struct device *dev);
void cs42l42_resume_restore(struct device *dev);
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 11/12] ASoC: cs42l42: Add Soundwire support
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (9 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 10/12] ASoC: cs42l42: Export some functions for Soundwire Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-22 11:15 ` Pierre-Louis Bossart
2022-08-19 12:52 ` [PATCH 12/12] ASoC: cs42l42: Add support for Soundwire interrupts Richard Fitzgerald
2022-09-23 16:54 ` [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Mark Brown
12 siblings, 1 reply; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
This adds support for using CS42L42 as a Soundwire device.
Soundwire-specifics are kept separate from the I2S implementation as
much as possible, aiming to limit the risk of breaking the I2C+I2S
support.
There are some important differences in the silicon behaviour between
I2S and Soundwire mode that are reflected in the implementation:
- ASP (I2S) most not be used in Soundwire mode because the two interfaces
share pins.
- The Soundwire capture (record) port only supports 1 channel. It does
not have left-to-right duplication like the ASP.
- DP2 can only be prepared if the HP has powered-up. DP1 can only be
prepared if the ADC has powered-up. (This ordering restriction does
not exist for ASPs.) The Soundwire core port-prepare step is
triggered by the DAI-link prepare(). This happens before the
codec DAI prepare() or the DAPM sequence so these cannot be used
to enable HP/ADC. Instead the HP/ADC enable are done in hw_params()
and hw_free().
- The SRCs are an integral part of the audio chain but in silicon their
power control is linked to the ASP. There is no equivalent power link
to Soundwire DPs so the driver must take "manual" control of SRC power.
- The Soundwire control registers occupy the lower part of the Soundwire
address space so cs42l42 registers are offset by 0x8000 (non-paged) in
Soundwire mode.
- Register addresses are 8-bit paged in I2C mode but 16-bit unpaged in
Soundwire.
- Special procedures are needed on register read/writes to (a) ensure
that the previous internal bus transaction has completed, and
(b) handle delayed read results, when the read value could not be
returned within the Soundwire read command.
There are also some differences in driver implementation between I2S
and Soundwire operation:
- CS42L42 does not runtime_suspend, but runtime_suspend/resume are required
in Soundwire mode as the most convenient way to power-up the bus manager
and to handle the unattach_request condition.
- Intel Soundwire host controllers have a low-power clock-stop mode that
requires resetting all peripherals when resuming. This is not compatible
with the plug-detect and button-detect because it will clear the
condition that caused the wakeup before the driver can handle it. So
clock-stop must be blocked when a snd_soc_jack handler is registered.
- As in I2S mode, the PLL is only used while audio is active because
of clocking quirks in the silicon. For Soundwire the cs42l42_pll_config()
is deferred until the DAI prepare(), to allow the cs42l42_bus_config()
callback to set the SCLK.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/Kconfig | 6 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/cs42l42-sdw.c | 601 +++++++++++++++++++++++++++++++++
sound/soc/codecs/cs42l42.c | 35 ++
sound/soc/codecs/cs42l42.h | 3 +
5 files changed, 647 insertions(+)
create mode 100644 sound/soc/codecs/cs42l42-sdw.c
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 9f6f0f97cfb9..464e44efa06b 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -68,6 +68,7 @@ config SND_SOC_ALL_CODECS
imply SND_SOC_CS35L45_I2C
imply SND_SOC_CS35L45_SPI
imply SND_SOC_CS42L42
+ imply SND_SOC_CS42L42_SDW
imply SND_SOC_CS42L51_I2C
imply SND_SOC_CS42L52
imply SND_SOC_CS42L56
@@ -700,6 +701,11 @@ config SND_SOC_CS42L42
select REGMAP_I2C
select SND_SOC_CS42L42_CORE
+config SND_SOC_CS42L42_SDW
+ tristate "Cirrus Logic CS42L42 CODEC on Soundwire"
+ depends on SOUNDWIRE
+ select SND_SOC_CS42L42_CORE
+
config SND_SOC_CS42L51
tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index d91f3c1fc2b3..11c19df9cb6a 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -66,6 +66,7 @@ snd-soc-cs35l45-spi-objs := cs35l45-spi.o
snd-soc-cs35l45-i2c-objs := cs35l45-i2c.o
snd-soc-cs42l42-objs := cs42l42.o
snd-soc-cs42l42-i2c-objs := cs42l42-i2c.o
+snd-soc-cs42l42-sdw-objs := cs42l42-sdw.o
snd-soc-cs42l51-objs := cs42l51.o
snd-soc-cs42l51-i2c-objs := cs42l51-i2c.o
snd-soc-cs42l52-objs := cs42l52.o
@@ -422,6 +423,7 @@ obj-$(CONFIG_SND_SOC_CS35L45_SPI) += snd-soc-cs35l45-spi.o
obj-$(CONFIG_SND_SOC_CS35L45_I2C) += snd-soc-cs35l45-i2c.o
obj-$(CONFIG_SND_SOC_CS42L42_CORE) += snd-soc-cs42l42.o
obj-$(CONFIG_SND_SOC_CS42L42) += snd-soc-cs42l42-i2c.o
+obj-$(CONFIG_SND_SOC_CS42L42_SDW) += snd-soc-cs42l42-sdw.o
obj-$(CONFIG_SND_SOC_CS42L51) += snd-soc-cs42l51.o
obj-$(CONFIG_SND_SOC_CS42L51_I2C) += snd-soc-cs42l51-i2c.o
obj-$(CONFIG_SND_SOC_CS42L52) += snd-soc-cs42l52.o
diff --git a/sound/soc/codecs/cs42l42-sdw.c b/sound/soc/codecs/cs42l42-sdw.c
new file mode 100644
index 000000000000..ed69a0a44d8c
--- /dev/null
+++ b/sound/soc/codecs/cs42l42-sdw.c
@@ -0,0 +1,601 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// cs42l42-sdw.c -- CS42L42 ALSA SoC audio driver Soundwire binding
+//
+// Copyright (C) 2022 Cirrus Logic, Inc. and
+// Cirrus Logic International Semiconductor Ltd.
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of_irq.h>
+#include <linux/pm_runtime.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_registers.h>
+#include <linux/soundwire/sdw_type.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include "cs42l42.h"
+
+#define CS42L42_SDW_CAPTURE_PORT 1
+#define CS42L42_SDW_PLAYBACK_PORT 2
+
+/* Register addresses are offset when sent over Soundwire */
+#define CS42L42_SDW_ADDR_OFFSET 0x8000
+
+#define CS42L42_SDW_MEM_ACCESS_STATUS 0xd0
+#define CS42L42_SDW_MEM_READ_DATA 0xd8
+
+#define CS42L42_SDW_LAST_LATE BIT(3)
+#define CS42L42_SDW_CMD_IN_PROGRESS BIT(2)
+#define CS42L42_SDW_RDATA_RDY BIT(0)
+
+#define CS42L42_DELAYED_READ_POLL_US 1
+#define CS42L42_DELAYED_READ_TIMEOUT_US 100
+
+static const struct snd_soc_dapm_route cs42l42_sdw_audio_map[] = {
+ /* Playback Path */
+ { "HP", NULL, "MIXER" },
+ { "MIXER", NULL, "DACSRC" },
+ { "DACSRC", NULL, "Playback" },
+
+ /* Capture Path */
+ { "ADCSRC", NULL, "HS" },
+ { "Capture", NULL, "ADCSRC" },
+};
+
+static int cs42l42_sdw_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
+
+ if (!cs42l42->init_done)
+ return -ENODEV;
+
+ return 0;
+}
+
+static int cs42l42_sdw_dai_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
+ struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
+ struct sdw_stream_config sconfig;
+ struct sdw_port_config pconfig;
+ unsigned int pdn_mask;
+ int ret;
+
+ if (!sdw_stream)
+ return -EINVAL;
+
+ /* Needed for PLL configuration when we are notified of new bus config */
+ cs42l42->sample_rate = params_rate(params);
+
+ memset(&sconfig, 0, sizeof(sconfig));
+ memset(&pconfig, 0, sizeof(pconfig));
+
+ sconfig.frame_rate = params_rate(params);
+ sconfig.ch_count = params_channels(params);
+ sconfig.bps = snd_pcm_format_width(params_format(params));
+ pconfig.ch_mask = GENMASK(sconfig.ch_count - 1, 0);
+
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ sconfig.direction = SDW_DATA_DIR_RX;
+ pconfig.num = CS42L42_SDW_PLAYBACK_PORT;
+ pdn_mask = CS42L42_HP_PDN_MASK;
+ } else {
+ sconfig.direction = SDW_DATA_DIR_TX;
+ pconfig.num = CS42L42_SDW_CAPTURE_PORT;
+ pdn_mask = CS42L42_ADC_PDN_MASK;
+ }
+
+ /*
+ * The DAI-link prepare() will trigger Soundwire DP prepare. But CS42L42
+ * DP will only prepare if the HP/ADC is already powered-up. The
+ * DAI prepare() and DAPM sequence run after DAI-link prepare() so the
+ * PDN bit must be written here.
+ */
+ regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
+ usleep_range(CS42L42_HP_ADC_EN_TIME_US, CS42L42_HP_ADC_EN_TIME_US + 1000);
+
+ ret = sdw_stream_add_slave(cs42l42->sdw_peripheral, &sconfig, &pconfig, 1, sdw_stream);
+ if (ret) {
+ dev_err(dai->dev, "Failed to add sdw stream: %d\n", ret);
+ goto err;
+ }
+
+ cs42l42_src_config(dai->component, params_rate(params));
+
+ return 0;
+
+err:
+ regmap_set_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
+
+ return ret;
+}
+
+static int cs42l42_sdw_dai_prepare(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
+
+ dev_dbg(dai->dev, "dai_prepare: sclk=%u rate=%u\n", cs42l42->sclk, cs42l42->sample_rate);
+
+ if (!cs42l42->sclk || !cs42l42->sample_rate)
+ return -EINVAL;
+
+ return cs42l42_pll_config(dai->component, cs42l42->sclk, cs42l42->sample_rate);
+}
+
+static int cs42l42_sdw_dai_hw_free(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
+ struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
+ unsigned int pdn_mask;
+
+ sdw_stream_remove_slave(cs42l42->sdw_peripheral, sdw_stream);
+ cs42l42->sample_rate = 0;
+
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+ pdn_mask = CS42L42_HP_PDN_MASK;
+ else
+ pdn_mask = CS42L42_ADC_PDN_MASK;
+
+ regmap_set_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
+
+ return 0;
+}
+
+static int cs42l42_sdw_dai_set_sdw_stream(struct snd_soc_dai *dai, void *sdw_stream,
+ int direction)
+{
+ if (!sdw_stream)
+ return 0;
+
+ if (direction == SNDRV_PCM_STREAM_PLAYBACK)
+ dai->playback_dma_data = sdw_stream;
+ else
+ dai->capture_dma_data = sdw_stream;
+
+ return 0;
+}
+
+static void cs42l42_sdw_dai_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ snd_soc_dai_set_dma_data(dai, substream, NULL);
+}
+
+static const struct snd_soc_dai_ops cs42l42_sdw_dai_ops = {
+ .startup = cs42l42_sdw_dai_startup,
+ .shutdown = cs42l42_sdw_dai_shutdown,
+ .hw_params = cs42l42_sdw_dai_hw_params,
+ .prepare = cs42l42_sdw_dai_prepare,
+ .hw_free = cs42l42_sdw_dai_hw_free,
+ .mute_stream = cs42l42_mute_stream,
+ .set_stream = cs42l42_sdw_dai_set_sdw_stream,
+};
+
+static struct snd_soc_dai_driver cs42l42_sdw_dai = {
+ .name = "cs42l42-sdw",
+ .playback = {
+ .stream_name = "Playback",
+ .channels_min = 1,
+ .channels_max = 2,
+ .rates = SNDRV_PCM_RATE_8000_96000,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE |
+ SNDRV_PCM_FMTBIT_S24_LE |
+ SNDRV_PCM_FMTBIT_S32_LE,
+ },
+ .capture = {
+ .stream_name = "Capture",
+ .channels_min = 1,
+ .channels_max = 1,
+ .rates = SNDRV_PCM_RATE_8000_96000,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE |
+ SNDRV_PCM_FMTBIT_S24_LE |
+ SNDRV_PCM_FMTBIT_S32_LE,
+ },
+ .symmetric_rate = 1,
+ .ops = &cs42l42_sdw_dai_ops,
+};
+
+static int cs42l42_sdw_poll_status(struct sdw_slave *peripheral, u8 mask, u8 match)
+{
+ int ret, sdwret;
+
+ ret = read_poll_timeout(sdw_read_no_pm, sdwret,
+ (sdwret < 0) || ((sdwret & mask) == match),
+ CS42L42_DELAYED_READ_POLL_US, CS42L42_DELAYED_READ_TIMEOUT_US,
+ false, peripheral, CS42L42_SDW_MEM_ACCESS_STATUS);
+ if (ret == 0)
+ ret = sdwret;
+
+ if (ret < 0)
+ dev_err(&peripheral->dev, "MEM_ACCESS_STATUS & %#x for %#x fail: %d\n",
+ mask, match, ret);
+
+ return ret;
+}
+
+static int cs42l42_sdw_read(void *context, unsigned int reg, unsigned int *val)
+{
+ struct sdw_slave *peripheral = context;
+ u8 data;
+ int ret;
+
+ reg += CS42L42_SDW_ADDR_OFFSET;
+
+ ret = cs42l42_sdw_poll_status(peripheral, CS42L42_SDW_CMD_IN_PROGRESS, 0);
+ if (ret < 0)
+ return ret;
+
+ ret = sdw_read_no_pm(peripheral, reg);
+ if (ret < 0) {
+ dev_err(&peripheral->dev, "Failed to issue read @0x%x: %d\n", reg, ret);
+ return ret;
+ }
+
+ data = (u8)ret; /* possible non-delayed read value */
+ ret = sdw_read_no_pm(peripheral, CS42L42_SDW_MEM_ACCESS_STATUS);
+ if (ret < 0) {
+ dev_err(&peripheral->dev, "Failed to read MEM_ACCESS_STATUS: %d\n", ret);
+ return ret;
+ }
+
+ /* If read was not delayed we already have the result */
+ if ((ret & CS42L42_SDW_LAST_LATE) == 0) {
+ *val = data;
+ return 0;
+ }
+
+ /* Poll for delayed read completion */
+ if ((ret & CS42L42_SDW_RDATA_RDY) == 0) {
+ ret = cs42l42_sdw_poll_status(peripheral,
+ CS42L42_SDW_RDATA_RDY, CS42L42_SDW_RDATA_RDY);
+ if (ret < 0)
+ return ret;
+ }
+
+ ret = sdw_read_no_pm(peripheral, CS42L42_SDW_MEM_READ_DATA);
+ if (ret < 0) {
+ dev_err(&peripheral->dev, "Failed to read READ_DATA: %d\n", ret);
+ return ret;
+ }
+
+ *val = (u8)ret;
+
+ return 0;
+}
+
+static int cs42l42_sdw_write(void *context, unsigned int reg, unsigned int val)
+{
+ struct sdw_slave *peripheral = context;
+ int ret;
+
+ ret = cs42l42_sdw_poll_status(peripheral, CS42L42_SDW_CMD_IN_PROGRESS, 0);
+ if (ret < 0)
+ return ret;
+
+ return sdw_write_no_pm(peripheral, reg + CS42L42_SDW_ADDR_OFFSET, (u8)val);
+}
+
+static void cs42l42_sdw_init(struct sdw_slave *peripheral)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
+ int ret = 0;
+
+ regcache_cache_only(cs42l42->regmap, false);
+
+ ret = cs42l42_init(cs42l42);
+ if (ret < 0) {
+ regcache_cache_only(cs42l42->regmap, true);
+ return;
+ }
+
+ /* Write out any cached changes that happened between probe and attach */
+ ret = regcache_sync(cs42l42->regmap);
+ if (ret < 0)
+ dev_warn(cs42l42->dev, "Failed to sync cache: %d\n", ret);
+
+ /* Disable internal logic that makes clock-stop conditional */
+ regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3, CS42L42_SW_CLK_STP_STAT_SEL_MASK);
+
+ /*
+ * pm_runtime is needed to control bus manager suspend, and to
+ * recover from an unattach_request when the manager suspends.
+ * Autosuspend delay must be long enough to enumerate.
+ */
+ pm_runtime_set_autosuspend_delay(cs42l42->dev, 3000);
+ pm_runtime_use_autosuspend(cs42l42->dev);
+ pm_runtime_set_active(cs42l42->dev);
+ pm_runtime_enable(cs42l42->dev);
+ pm_runtime_mark_last_busy(cs42l42->dev);
+ pm_runtime_idle(cs42l42->dev);
+}
+
+static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
+ struct sdw_slave_prop *prop = &peripheral->prop;
+ struct sdw_dpn_prop *ports;
+
+ ports = devm_kcalloc(cs42l42->dev, 2, sizeof(*ports), GFP_KERNEL);
+ if (!ports)
+ return -ENOMEM;
+
+ prop->source_ports = BIT(CS42L42_SDW_CAPTURE_PORT);
+ prop->sink_ports = BIT(CS42L42_SDW_PLAYBACK_PORT);
+ prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
+ prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
+
+ /* DP1 - capture */
+ ports[0].num = CS42L42_SDW_CAPTURE_PORT,
+ ports[0].type = SDW_DPN_FULL,
+ ports[0].ch_prep_timeout = 10,
+ prop->src_dpn_prop = &ports[0];
+
+ /* DP2 - playback */
+ ports[1].num = CS42L42_SDW_PLAYBACK_PORT,
+ ports[1].type = SDW_DPN_FULL,
+ ports[1].ch_prep_timeout = 10,
+ prop->sink_dpn_prop = &ports[1];
+
+ return 0;
+}
+
+static int cs42l42_sdw_update_status(struct sdw_slave *peripheral,
+ enum sdw_slave_status status)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
+
+ switch (status) {
+ case SDW_SLAVE_ATTACHED:
+ dev_dbg(cs42l42->dev, "ATTACHED\n");
+ if (!cs42l42->init_done)
+ cs42l42_sdw_init(peripheral);
+ break;
+ case SDW_SLAVE_UNATTACHED:
+ dev_dbg(cs42l42->dev, "UNATTACHED\n");
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static int cs42l42_sdw_bus_config(struct sdw_slave *peripheral,
+ struct sdw_bus_params *params)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
+ unsigned int new_sclk = params->curr_dr_freq / 2;
+
+ /* The cs42l42 cannot support a glitchless SWIRE_CLK change. */
+ if ((new_sclk != cs42l42->sclk) && cs42l42->stream_use) {
+ dev_warn(cs42l42->dev, "Rejected SCLK change while audio active\n");
+ return -EBUSY;
+ }
+
+ cs42l42->sclk = new_sclk;
+
+ dev_dbg(cs42l42->dev, "bus_config: sclk=%u c=%u r=%u\n",
+ cs42l42->sclk, params->col, params->row);
+
+ return 0;
+}
+
+static int __maybe_unused cs42l42_sdw_clk_stop(struct sdw_slave *peripheral,
+ enum sdw_clk_stop_mode mode,
+ enum sdw_clk_stop_type type)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
+
+ dev_dbg(cs42l42->dev, "clk_stop mode:%d type:%d\n", mode, type);
+
+ return 0;
+}
+
+static const struct sdw_slave_ops cs42l42_sdw_ops = {
+ .read_prop = cs42l42_sdw_read_prop,
+ .update_status = cs42l42_sdw_update_status,
+ .bus_config = cs42l42_sdw_bus_config,
+#ifdef DEBUG
+ .clk_stop = cs42l42_sdw_clk_stop,
+#endif
+};
+
+static int __maybe_unused cs42l42_sdw_runtime_suspend(struct device *dev)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
+
+ dev_dbg(dev, "Runtime suspend\n");
+
+ /* The host controller could suspend, which would mean no register access */
+ regcache_cache_only(cs42l42->regmap, true);
+
+ return 0;
+}
+
+static const struct reg_sequence __maybe_unused cs42l42_soft_reboot_seq[] = {
+ REG_SEQ0(CS42L42_SOFT_RESET_REBOOT, 0x1e),
+};
+
+static int __maybe_unused cs42l42_sdw_handle_unattach(struct cs42l42_private *cs42l42)
+{
+ struct sdw_slave *peripheral = cs42l42->sdw_peripheral;
+
+ if (!peripheral->unattach_request)
+ return 0;
+
+ /* Cannot access registers until master re-attaches. */
+ dev_dbg(&peripheral->dev, "Wait for initialization_complete\n");
+ if (!wait_for_completion_timeout(&peripheral->initialization_complete,
+ msecs_to_jiffies(5000))) {
+ dev_err(&peripheral->dev, "initialization_complete timed out\n");
+ return -ETIMEDOUT;
+ }
+
+ peripheral->unattach_request = 0;
+
+ /*
+ * After a bus reset there must be a reconfiguration reset to
+ * reinitialize the internal state of CS42L42.
+ */
+ regmap_multi_reg_write_bypassed(cs42l42->regmap,
+ cs42l42_soft_reboot_seq,
+ ARRAY_SIZE(cs42l42_soft_reboot_seq));
+ usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
+ regcache_mark_dirty(cs42l42->regmap);
+
+ return 0;
+}
+
+static int __maybe_unused cs42l42_sdw_runtime_resume(struct device *dev)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
+ int ret;
+
+ dev_dbg(dev, "Runtime resume\n");
+
+ ret = cs42l42_sdw_handle_unattach(cs42l42);
+ if (ret < 0)
+ return ret;
+
+ regcache_cache_only(cs42l42->regmap, false);
+
+ /* Sync LATCH_TO_VP first so the VP domain registers sync correctly */
+ regcache_sync_region(cs42l42->regmap, CS42L42_MIC_DET_CTL1, CS42L42_MIC_DET_CTL1);
+ regcache_sync(cs42l42->regmap);
+
+ return 0;
+}
+
+static int __maybe_unused cs42l42_sdw_resume(struct device *dev)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
+ int ret;
+
+ dev_dbg(dev, "System resume\n");
+
+ /* Power-up so it can re-enumerate */
+ ret = cs42l42_resume(dev);
+ if (ret)
+ return ret;
+
+ /* Wait for re-attach */
+ ret = cs42l42_sdw_handle_unattach(cs42l42);
+ if (ret < 0)
+ return ret;
+
+ cs42l42_resume_restore(dev);
+
+ return 0;
+}
+
+static int cs42l42_sdw_probe(struct sdw_slave *peripheral, const struct sdw_device_id *id)
+{
+ struct device *dev = &peripheral->dev;
+ struct cs42l42_private *cs42l42;
+ struct regmap_config *regmap_conf;
+ struct regmap *regmap;
+ struct snd_soc_component_driver *component_drv;
+ int irq, ret;
+
+ cs42l42 = devm_kzalloc(dev, sizeof(*cs42l42), GFP_KERNEL);
+ if (!cs42l42)
+ return -ENOMEM;
+
+ if (has_acpi_companion(dev))
+ irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
+ else
+ irq = of_irq_get(dev->of_node, 0);
+
+ if (irq == -ENOENT)
+ irq = 0;
+ else if (irq < 0)
+ return dev_err_probe(dev, irq, "Failed to get IRQ\n");
+
+ regmap_conf = devm_kmemdup(dev, &cs42l42_regmap, sizeof(cs42l42_regmap), GFP_KERNEL);
+ if (!regmap_conf)
+ return -ENOMEM;
+ regmap_conf->reg_bits = 16;
+ regmap_conf->num_ranges = 0;
+ regmap_conf->reg_read = cs42l42_sdw_read;
+ regmap_conf->reg_write = cs42l42_sdw_write;
+
+ regmap = devm_regmap_init(dev, NULL, peripheral, regmap_conf);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap), "Failed to allocate register map\n");
+
+ /* Start in cache-only until device is enumerated */
+ regcache_cache_only(regmap, true);
+
+ component_drv = devm_kmemdup(dev,
+ &cs42l42_soc_component,
+ sizeof(cs42l42_soc_component),
+ GFP_KERNEL);
+ if (!component_drv)
+ return -ENOMEM;
+
+ component_drv->dapm_routes = cs42l42_sdw_audio_map;
+ component_drv->num_dapm_routes = ARRAY_SIZE(cs42l42_sdw_audio_map);
+
+ cs42l42->dev = dev;
+ cs42l42->regmap = regmap;
+ cs42l42->sdw_peripheral = peripheral;
+ cs42l42->irq = irq;
+
+ ret = cs42l42_common_probe(cs42l42, component_drv, &cs42l42_sdw_dai);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int cs42l42_sdw_remove(struct sdw_slave *peripheral)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
+
+ /* Resume so that cs42l42_remove() can access registers */
+ pm_runtime_get_sync(cs42l42->dev);
+ cs42l42_common_remove(cs42l42);
+ pm_runtime_put(cs42l42->dev);
+ pm_runtime_disable(cs42l42->dev);
+
+ return 0;
+}
+
+static const struct dev_pm_ops cs42l42_sdw_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(cs42l42_suspend, cs42l42_sdw_resume)
+ SET_RUNTIME_PM_OPS(cs42l42_sdw_runtime_suspend, cs42l42_sdw_runtime_resume, NULL)
+};
+
+static const struct sdw_device_id cs42l42_sdw_id[] = {
+ SDW_SLAVE_ENTRY(0x01FA, 0x4242, 0),
+ {},
+};
+MODULE_DEVICE_TABLE(sdw, cs42l42_sdw_id);
+
+static struct sdw_driver cs42l42_sdw_driver = {
+ .driver = {
+ .name = "cs42l42-sdw",
+ .pm = &cs42l42_sdw_pm,
+ },
+ .probe = cs42l42_sdw_probe,
+ .remove = cs42l42_sdw_remove,
+ .ops = &cs42l42_sdw_ops,
+ .id_table = cs42l42_sdw_id,
+};
+
+module_sdw_driver(cs42l42_sdw_driver);
+
+MODULE_DESCRIPTION("ASoC CS42L42 Soundwire driver");
+MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.cirrus.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(SND_SOC_CS42L42_CORE);
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 3a4f65233360..15996bb3be96 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -20,6 +20,7 @@
#include <linux/slab.h>
#include <linux/acpi.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/property.h>
#include <linux/regulator/consumer.h>
#include <linux/gpio/consumer.h>
@@ -522,6 +523,10 @@ static const struct snd_soc_dapm_widget cs42l42_dapm_widgets[] = {
/* Playback/Capture Requirements */
SND_SOC_DAPM_SUPPLY("SCLK", CS42L42_ASP_CLK_CFG, CS42L42_ASP_SCLK_EN_SHIFT, 0, NULL, 0),
+
+ /* Soundwire SRC power control */
+ SND_SOC_DAPM_PGA("DACSRC", CS42L42_PWR_CTL2, CS42L42_DAC_SRC_PDNB_SHIFT, 0, NULL, 0),
+ SND_SOC_DAPM_PGA("ADCSRC", CS42L42_PWR_CTL2, CS42L42_ADC_SRC_PDNB_SHIFT, 0, NULL, 0),
};
static const struct snd_soc_dapm_route cs42l42_audio_map[] = {
@@ -559,6 +564,20 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
{
struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
+ /*
+ * If the Soundwire controller issues bus reset when coming out of
+ * clock-stop it will erase the jack state. This can lose button press
+ * events, and plug/unplug interrupt bits take between 125ms and 1500ms
+ * before they are valid again.
+ * Prevent this by holding our pm_runtime to block clock-stop.
+ */
+ if (cs42l42->sdw_peripheral) {
+ if (jk)
+ pm_runtime_get_sync(cs42l42->dev);
+ else
+ pm_runtime_put_autosuspend(cs42l42->dev);
+ }
+
/* Prevent race with interrupt handler */
mutex_lock(&cs42l42->irq_lock);
cs42l42->jack = jk;
@@ -1645,9 +1664,11 @@ irqreturn_t cs42l42_irq_thread(int irq, void *data)
unsigned int current_button_status;
unsigned int i;
+ pm_runtime_get_sync(cs42l42->dev);
mutex_lock(&cs42l42->irq_lock);
if (cs42l42->suspended || !cs42l42->init_done) {
mutex_unlock(&cs42l42->irq_lock);
+ pm_runtime_put_autosuspend(cs42l42->dev);
return IRQ_NONE;
}
@@ -1750,6 +1771,8 @@ irqreturn_t cs42l42_irq_thread(int irq, void *data)
}
mutex_unlock(&cs42l42->irq_lock);
+ pm_runtime_mark_last_busy(cs42l42->dev);
+ pm_runtime_put_autosuspend(cs42l42->dev);
return IRQ_HANDLED;
}
@@ -2374,6 +2397,18 @@ int cs42l42_init(struct cs42l42_private *cs42l42)
if (ret != 0)
goto err_shutdown;
+ /*
+ * SRC power is linked to ASP power so doesn't work in Soundwire mode.
+ * Override it and use DAPM to control SRC power for Soundwire.
+ */
+ if (cs42l42->sdw_peripheral) {
+ regmap_update_bits(cs42l42->regmap, CS42L42_PWR_CTL2,
+ CS42L42_SRC_PDN_OVERRIDE_MASK |
+ CS42L42_DAC_SRC_PDNB_MASK |
+ CS42L42_ADC_SRC_PDNB_MASK,
+ CS42L42_SRC_PDN_OVERRIDE_MASK);
+ }
+
/* Setup headset detection */
cs42l42_setup_hs_type_detect(cs42l42);
diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
index f575ef9b5498..038db45d95b3 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -18,6 +18,7 @@
#include <linux/mutex.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
+#include <linux/soundwire/sdw.h>
#include <sound/jack.h>
#include <sound/cs42l42.h>
#include <sound/soc-component.h>
@@ -30,10 +31,12 @@ struct cs42l42_private {
struct gpio_desc *reset_gpio;
struct completion pdn_done;
struct snd_soc_jack *jack;
+ struct sdw_slave *sdw_peripheral;
struct mutex irq_lock;
int irq;
int pll_config;
u32 sclk;
+ u32 sample_rate;
u8 plug_state;
u8 hs_type;
u8 ts_inv;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 11/12] ASoC: cs42l42: Add Soundwire support
2022-08-19 12:52 ` [PATCH 11/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
@ 2022-08-22 11:15 ` Pierre-Louis Bossart
2022-08-22 13:50 ` Richard Fitzgerald
0 siblings, 1 reply; 21+ messages in thread
From: Pierre-Louis Bossart @ 2022-08-22 11:15 UTC (permalink / raw)
To: Richard Fitzgerald, broonie; +Cc: patches, alsa-devel, linux-kernel
Thanks Richard, this looks quite good.
Nit-pick for the entire patch: SoundWire (capital W).
> - Intel Soundwire host controllers have a low-power clock-stop mode that
> requires resetting all peripherals when resuming. This is not compatible
> with the plug-detect and button-detect because it will clear the
> condition that caused the wakeup before the driver can handle it. So
> clock-stop must be blocked when a snd_soc_jack handler is registered.
What do you mean by 'clock-stop must be blocked'? The peripheral cannot
prevent the host from stopping the clock. Maybe this is explained
further down in this patch, but that statement is a bit odd.
Even if the condition that caused the wakeup was cleared, presumably
when resetting the device the same condition will be raised again, no?
> +static int cs42l42_sdw_dai_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
> + struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> + struct sdw_stream_config sconfig;
> + struct sdw_port_config pconfig;
> + unsigned int pdn_mask;
> + int ret;
> +
> + if (!sdw_stream)
> + return -EINVAL;
> +
> + /* Needed for PLL configuration when we are notified of new bus config */
> + cs42l42->sample_rate = params_rate(params);
> +
> + memset(&sconfig, 0, sizeof(sconfig));
> + memset(&pconfig, 0, sizeof(pconfig));
> +
> + sconfig.frame_rate = params_rate(params);
> + sconfig.ch_count = params_channels(params);
> + sconfig.bps = snd_pcm_format_width(params_format(params));
> + pconfig.ch_mask = GENMASK(sconfig.ch_count - 1, 0);
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + sconfig.direction = SDW_DATA_DIR_RX;
> + pconfig.num = CS42L42_SDW_PLAYBACK_PORT;
> + pdn_mask = CS42L42_HP_PDN_MASK;
> + } else {
> + sconfig.direction = SDW_DATA_DIR_TX;
> + pconfig.num = CS42L42_SDW_CAPTURE_PORT;
> + pdn_mask = CS42L42_ADC_PDN_MASK;
> + }
> +
> + /*
> + * The DAI-link prepare() will trigger Soundwire DP prepare. But CS42L42
> + * DP will only prepare if the HP/ADC is already powered-up. The
> + * DAI prepare() and DAPM sequence run after DAI-link prepare() so the
> + * PDN bit must be written here.
> + */
Why not make use of the callbacks that were added precisely to let the
codec driver perform device-specific operations? You can add your own
code in pre and post operation for both prepare and bank switch. It's
not clear why you would do this in a hw_params (which can be called
multiple times per ALSA conventions).
> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
> + usleep_range(CS42L42_HP_ADC_EN_TIME_US, CS42L42_HP_ADC_EN_TIME_US + 1000);
> +
> + ret = sdw_stream_add_slave(cs42l42->sdw_peripheral, &sconfig, &pconfig, 1, sdw_stream);
> + if (ret) {
> + dev_err(dai->dev, "Failed to add sdw stream: %d\n", ret);
> + goto err;
> + }
> +
> + cs42l42_src_config(dai->component, params_rate(params));
> +
> + return 0;
> +
> +err:
> + regmap_set_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
> +
> + return ret;
> +}
> +
> +static int cs42l42_sdw_dai_prepare(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
> +
> + dev_dbg(dai->dev, "dai_prepare: sclk=%u rate=%u\n", cs42l42->sclk, cs42l42->sample_rate);
> +
> + if (!cs42l42->sclk || !cs42l42->sample_rate)
what does sclk mean in the SoundWire context?
> + return -EINVAL;
> +
> + return cs42l42_pll_config(dai->component, cs42l42->sclk, cs42l42->sample_rate);
> +}
> +
There should be a really big comment here that the following functions
implement delayed reads and writes - this was not needed for previous
codecs.
> +static int cs42l42_sdw_poll_status(struct sdw_slave *peripheral, u8 mask, u8 match)
> +{
> + int ret, sdwret;
> +
> + ret = read_poll_timeout(sdw_read_no_pm, sdwret,
> + (sdwret < 0) || ((sdwret & mask) == match),
> + CS42L42_DELAYED_READ_POLL_US, CS42L42_DELAYED_READ_TIMEOUT_US,
> + false, peripheral, CS42L42_SDW_MEM_ACCESS_STATUS);
> + if (ret == 0)
> + ret = sdwret;
> +
> + if (ret < 0)
> + dev_err(&peripheral->dev, "MEM_ACCESS_STATUS & %#x for %#x fail: %d\n",
> + mask, match, ret);
> +
> + return ret;
> +}
> +
> +static int cs42l42_sdw_read(void *context, unsigned int reg, unsigned int *val)
> +{
> + struct sdw_slave *peripheral = context;
> + u8 data;
> + int ret;
> +
> + reg += CS42L42_SDW_ADDR_OFFSET;
> +
> + ret = cs42l42_sdw_poll_status(peripheral, CS42L42_SDW_CMD_IN_PROGRESS, 0);
> + if (ret < 0)
> + return ret;
> +
> + ret = sdw_read_no_pm(peripheral, reg);
> + if (ret < 0) {
> + dev_err(&peripheral->dev, "Failed to issue read @0x%x: %d\n", reg, ret);
> + return ret;
> + }
> +
> + data = (u8)ret; /* possible non-delayed read value */
> + ret = sdw_read_no_pm(peripheral, CS42L42_SDW_MEM_ACCESS_STATUS);
> + if (ret < 0) {
> + dev_err(&peripheral->dev, "Failed to read MEM_ACCESS_STATUS: %d\n", ret);
> + return ret;
> + }
> +
> + /* If read was not delayed we already have the result */
> + if ((ret & CS42L42_SDW_LAST_LATE) == 0) {
> + *val = data;
> + return 0;
> + }
> +
> + /* Poll for delayed read completion */
> + if ((ret & CS42L42_SDW_RDATA_RDY) == 0) {
> + ret = cs42l42_sdw_poll_status(peripheral,
> + CS42L42_SDW_RDATA_RDY, CS42L42_SDW_RDATA_RDY);
> + if (ret < 0)
> + return ret;
> + }
> +
> + ret = sdw_read_no_pm(peripheral, CS42L42_SDW_MEM_READ_DATA);
> + if (ret < 0) {
> + dev_err(&peripheral->dev, "Failed to read READ_DATA: %d\n", ret);
> + return ret;
> + }
> +
> + *val = (u8)ret;
> +
> + return 0;
> +}
> +
> +static int cs42l42_sdw_write(void *context, unsigned int reg, unsigned int val)
> +{
> + struct sdw_slave *peripheral = context;
> + int ret;
> +
> + ret = cs42l42_sdw_poll_status(peripheral, CS42L42_SDW_CMD_IN_PROGRESS, 0);
> + if (ret < 0)
> + return ret;
> +
> + return sdw_write_no_pm(peripheral, reg + CS42L42_SDW_ADDR_OFFSET, (u8)val);
> +}
> +
> +static void cs42l42_sdw_init(struct sdw_slave *peripheral)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> + int ret = 0;
> +
> + regcache_cache_only(cs42l42->regmap, false);
> +
> + ret = cs42l42_init(cs42l42);
> + if (ret < 0) {
> + regcache_cache_only(cs42l42->regmap, true);
> + return;
> + }
> +
> + /* Write out any cached changes that happened between probe and attach */
> + ret = regcache_sync(cs42l42->regmap);
> + if (ret < 0)
> + dev_warn(cs42l42->dev, "Failed to sync cache: %d\n", ret);
> +
> + /* Disable internal logic that makes clock-stop conditional */
> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3, CS42L42_SW_CLK_STP_STAT_SEL_MASK);
> +
> + /*
> + * pm_runtime is needed to control bus manager suspend, and to
I don't think the intent is that the codec can control the manager
suspend, but that the manager cannot be suspended by the framework
unless the codec suspends first?
> + * recover from an unattach_request when the manager suspends.
> + * Autosuspend delay must be long enough to enumerate.
No, this last sentence is not correct. The enumeration can be done no
matter what pm_runtime state the Linux codec device is in. And it's
really the other way around, it's when the codec reports as ATTACHED
that the codec driver will be resumed.
> + */
> + pm_runtime_set_autosuspend_delay(cs42l42->dev, 3000);
> + pm_runtime_use_autosuspend(cs42l42->dev);
> + pm_runtime_set_active(cs42l42->dev);
> + pm_runtime_enable(cs42l42->dev);
> + pm_runtime_mark_last_busy(cs42l42->dev);
> + pm_runtime_idle(cs42l42->dev);
> +}
> +
> +static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> + struct sdw_slave_prop *prop = &peripheral->prop;
> + struct sdw_dpn_prop *ports;
> +
> + ports = devm_kcalloc(cs42l42->dev, 2, sizeof(*ports), GFP_KERNEL);
> + if (!ports)
> + return -ENOMEM;
> +
> + prop->source_ports = BIT(CS42L42_SDW_CAPTURE_PORT);
> + prop->sink_ports = BIT(CS42L42_SDW_PLAYBACK_PORT);
> + prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
> + prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
> +
> + /* DP1 - capture */
> + ports[0].num = CS42L42_SDW_CAPTURE_PORT,
> + ports[0].type = SDW_DPN_FULL,
> + ports[0].ch_prep_timeout = 10,
> + prop->src_dpn_prop = &ports[0];
> +
> + /* DP2 - playback */
> + ports[1].num = CS42L42_SDW_PLAYBACK_PORT,
> + ports[1].type = SDW_DPN_FULL,
> + ports[1].ch_prep_timeout = 10,
> + prop->sink_dpn_prop = &ports[1];
> +
> + return 0;
> +}
> +static int cs42l42_sdw_bus_config(struct sdw_slave *peripheral,
> + struct sdw_bus_params *params)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> + unsigned int new_sclk = params->curr_dr_freq / 2;
> +
> + /* The cs42l42 cannot support a glitchless SWIRE_CLK change. */
nit-pick: SoundWire
> + if ((new_sclk != cs42l42->sclk) && cs42l42->stream_use) {
> + dev_warn(cs42l42->dev, "Rejected SCLK change while audio active\n");
> + return -EBUSY;
> + }
It's good to have this test but so far we haven't changed the bus clock
on the fly so it's not tested.
> +
> + cs42l42->sclk = new_sclk;
> +
> + dev_dbg(cs42l42->dev, "bus_config: sclk=%u c=%u r=%u\n",
> + cs42l42->sclk, params->col, params->row);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused cs42l42_sdw_clk_stop(struct sdw_slave *peripheral,
> + enum sdw_clk_stop_mode mode,
> + enum sdw_clk_stop_type type)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> +
> + dev_dbg(cs42l42->dev, "clk_stop mode:%d type:%d\n", mode, type);
> +
> + return 0;
> +}
> +
> +static const struct sdw_slave_ops cs42l42_sdw_ops = {
> + .read_prop = cs42l42_sdw_read_prop,
> + .update_status = cs42l42_sdw_update_status,
> + .bus_config = cs42l42_sdw_bus_config,
> +#ifdef DEBUG
> + .clk_stop = cs42l42_sdw_clk_stop,
> +#endif
do you really need this wrapper?
> +};
> +
> +static int __maybe_unused cs42l42_sdw_runtime_suspend(struct device *dev)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
> +
> + dev_dbg(dev, "Runtime suspend\n");
> +
> + /* The host controller could suspend, which would mean no register access */
> + regcache_cache_only(cs42l42->regmap, true);
> +
> + return 0;
> +}
> +
> +static const struct reg_sequence __maybe_unused cs42l42_soft_reboot_seq[] = {
> + REG_SEQ0(CS42L42_SOFT_RESET_REBOOT, 0x1e),
> +};
>
> +static int __maybe_unused cs42l42_sdw_resume(struct device *dev)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
> + int ret;
> +
> + dev_dbg(dev, "System resume\n");
> +
> + /* Power-up so it can re-enumerate */
??
You cannot power-up with the bus, did you mean that side channels are
used for powering up?
> + ret = cs42l42_resume(dev);
> + if (ret)
> + return ret;
> +
> + /* Wait for re-attach */
> + ret = cs42l42_sdw_handle_unattach(cs42l42);
> + if (ret < 0)
> + return ret;
> +
> + cs42l42_resume_restore(dev);
> +
> + return 0;
> +}
> +
> +static int cs42l42_sdw_probe(struct sdw_slave *peripheral, const struct sdw_device_id *id)
> +{
> + struct device *dev = &peripheral->dev;
> + struct cs42l42_private *cs42l42;
> + struct regmap_config *regmap_conf;
> + struct regmap *regmap;
> + struct snd_soc_component_driver *component_drv;
> + int irq, ret;
> +
> + cs42l42 = devm_kzalloc(dev, sizeof(*cs42l42), GFP_KERNEL);
> + if (!cs42l42)
> + return -ENOMEM;
> +
> + if (has_acpi_companion(dev))
> + irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> + else
> + irq = of_irq_get(dev->of_node, 0);
why do you need an interrupt when SoundWire provides an in-band one? You
need a lot more explanations on the requirement and what you intend to
do with this interrupt?
> +
> + if (irq == -ENOENT)
> + irq = 0;
> + else if (irq < 0)
> + return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> +
> + regmap_conf = devm_kmemdup(dev, &cs42l42_regmap, sizeof(cs42l42_regmap), GFP_KERNEL);
> + if (!regmap_conf)
> + return -ENOMEM;
> + regmap_conf->reg_bits = 16;
> + regmap_conf->num_ranges = 0;
> + regmap_conf->reg_read = cs42l42_sdw_read;
> + regmap_conf->reg_write = cs42l42_sdw_write;
> +
> + regmap = devm_regmap_init(dev, NULL, peripheral, regmap_conf);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap), "Failed to allocate register map\n");
> +
> + /* Start in cache-only until device is enumerated */
> + regcache_cache_only(regmap, true);
> +
> + component_drv = devm_kmemdup(dev,
> + &cs42l42_soc_component,
> + sizeof(cs42l42_soc_component),
> + GFP_KERNEL);
> + if (!component_drv)
> + return -ENOMEM;
> +
> + component_drv->dapm_routes = cs42l42_sdw_audio_map;
> + component_drv->num_dapm_routes = ARRAY_SIZE(cs42l42_sdw_audio_map);
> +
> + cs42l42->dev = dev;
> + cs42l42->regmap = regmap;
> + cs42l42->sdw_peripheral = peripheral;
> + cs42l42->irq = irq;
> +
> + ret = cs42l42_common_probe(cs42l42, component_drv, &cs42l42_sdw_dai);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int cs42l42_sdw_remove(struct sdw_slave *peripheral)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> +
> + /* Resume so that cs42l42_remove() can access registers */
> + pm_runtime_get_sync(cs42l42->dev);
you need to test if this resume succeeded, and possibly use
pm_resume_resume_and_get() to avoid issues.
> + cs42l42_common_remove(cs42l42);
> + pm_runtime_put(cs42l42->dev);
> + pm_runtime_disable(cs42l42->dev);
> +
> + return 0;
> +}
> static const struct snd_soc_dapm_route cs42l42_audio_map[] = {
> @@ -559,6 +564,20 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
> {
> struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
>
> + /*
> + * If the Soundwire controller issues bus reset when coming out of
> + * clock-stop it will erase the jack state. This can lose button press
> + * events, and plug/unplug interrupt bits take between 125ms and 1500ms
> + * before they are valid again.
> + * Prevent this by holding our pm_runtime to block clock-stop.
> + */
> + if (cs42l42->sdw_peripheral) {
> + if (jk)
> + pm_runtime_get_sync(cs42l42->dev);
> + else
> + pm_runtime_put_autosuspend(cs42l42->dev);
> + }
> +
I *really* don't understand this sequence.
The bus will be suspended when ALL devices have been idle for some time.
If the user presses a button AFTER the bus is suspended, the device can
still use the in-band wake and resume.
Granted the button press will be lost but the plug/unplug status will
still be handled with a delay.
> /* Prevent race with interrupt handler */
> mutex_lock(&cs42l42->irq_lock);
> cs42l42->jack = jk;
> @@ -1645,9 +1664,11 @@ irqreturn_t cs42l42_irq_thread(int irq, void *data)
> unsigned int current_button_status;
> unsigned int i;
>
> + pm_runtime_get_sync(cs42l42->dev);
> mutex_lock(&cs42l42->irq_lock);
> if (cs42l42->suspended || !cs42l42->init_done) {
> mutex_unlock(&cs42l42->irq_lock);
> + pm_runtime_put_autosuspend(cs42l42->dev);
> return IRQ_NONE;
> }
>
> @@ -1750,6 +1771,8 @@ irqreturn_t cs42l42_irq_thread(int irq, void *data)
> }
>
> mutex_unlock(&cs42l42->irq_lock);
> + pm_runtime_mark_last_busy(cs42l42->dev);
> + pm_runtime_put_autosuspend(cs42l42->dev);
>
> return IRQ_HANDLED;
Again in SoundWire more you should not use a dedicated interrupt.
There's something missing in the explanations on why this thread is
required.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 11/12] ASoC: cs42l42: Add Soundwire support
2022-08-22 11:15 ` Pierre-Louis Bossart
@ 2022-08-22 13:50 ` Richard Fitzgerald
2022-08-22 14:55 ` Pierre-Louis Bossart
0 siblings, 1 reply; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-22 13:50 UTC (permalink / raw)
To: Pierre-Louis Bossart, broonie; +Cc: patches, alsa-devel, linux-kernel
On 22/08/2022 12:15, Pierre-Louis Bossart wrote:
> Thanks Richard, this looks quite good.
>
> Nit-pick for the entire patch: SoundWire (capital W).
>
>
>> - Intel Soundwire host controllers have a low-power clock-stop mode that
>> requires resetting all peripherals when resuming. This is not compatible
>> with the plug-detect and button-detect because it will clear the
>> condition that caused the wakeup before the driver can handle it. So
>> clock-stop must be blocked when a snd_soc_jack handler is registered.
>
> What do you mean by 'clock-stop must be blocked'? The peripheral cannot
> prevent the host from stopping the clock.
Are you sure about that? We're going to have serious problems if the
Intel manager driver can clock-stop even though there are peripheral
drivers still using the clock.
Currently the Intel code will only clock-stop when it goes into
runtime suspend, which only happens if all peripheral drivers are also
in runtime suspend. Or on system suspend, which is handled specially by
the cs42l42 driver. If you are saying this isn't guaranteed behavior
then we'll need to add something to the core Soundwire core code to tell
it when it's allowed to clock-stop.
I tried returning an error from the codec driver clk_stop() callback but
the core code and cadence code treat that as unexpected and dev_err()
it, then the intel.c code ignores the error and carries on suspending.
Maybe this is explained
> further down in this patch, but that statement is a bit odd.
>
> Even if the condition that caused the wakeup was cleared, presumably
> when resetting the device the same condition will be raised again, no?
>
>> +static int cs42l42_sdw_dai_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
>> + struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
>> + struct sdw_stream_config sconfig;
>> + struct sdw_port_config pconfig;
>> + unsigned int pdn_mask;
>> + int ret;
>> +
>> + if (!sdw_stream)
>> + return -EINVAL;
>> +
>> + /* Needed for PLL configuration when we are notified of new bus config */
>> + cs42l42->sample_rate = params_rate(params);
>> +
>> + memset(&sconfig, 0, sizeof(sconfig));
>> + memset(&pconfig, 0, sizeof(pconfig));
>> +
>> + sconfig.frame_rate = params_rate(params);
>> + sconfig.ch_count = params_channels(params);
>> + sconfig.bps = snd_pcm_format_width(params_format(params));
>> + pconfig.ch_mask = GENMASK(sconfig.ch_count - 1, 0);
>> +
>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> + sconfig.direction = SDW_DATA_DIR_RX;
>> + pconfig.num = CS42L42_SDW_PLAYBACK_PORT;
>> + pdn_mask = CS42L42_HP_PDN_MASK;
>> + } else {
>> + sconfig.direction = SDW_DATA_DIR_TX;
>> + pconfig.num = CS42L42_SDW_CAPTURE_PORT;
>> + pdn_mask = CS42L42_ADC_PDN_MASK;
>> + }
>> +
>> + /*
>> + * The DAI-link prepare() will trigger Soundwire DP prepare. But CS42L42
>> + * DP will only prepare if the HP/ADC is already powered-up. The
>> + * DAI prepare() and DAPM sequence run after DAI-link prepare() so the
>> + * PDN bit must be written here.
>> + */
>
> Why not make use of the callbacks that were added precisely to let the
> codec driver perform device-specific operations? You can add your own
> code in pre and post operation for both prepare and bank switch. It's
> not clear why you would do this in a hw_params (which can be called
> multiple times per ALSA conventions).
>
Ah, I'd not noticed the port_prep callback. Maybe it didn't exist when
this code was first written.
>> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
>> + usleep_range(CS42L42_HP_ADC_EN_TIME_US, CS42L42_HP_ADC_EN_TIME_US + 1000);
>> +
>> + ret = sdw_stream_add_slave(cs42l42->sdw_peripheral, &sconfig, &pconfig, 1, sdw_stream);
>> + if (ret) {
>> + dev_err(dai->dev, "Failed to add sdw stream: %d\n", ret);
>> + goto err;
>> + }
>> +
>> + cs42l42_src_config(dai->component, params_rate(params));
>> +
>> + return 0;
>> +
>> +err:
>> + regmap_set_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
>> +
>> + return ret;
>> +}
>> +
>> +static int cs42l42_sdw_dai_prepare(struct snd_pcm_substream *substream,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
>> +
>> + dev_dbg(dai->dev, "dai_prepare: sclk=%u rate=%u\n", cs42l42->sclk, cs42l42->sample_rate);
>> +
>> + if (!cs42l42->sclk || !cs42l42->sample_rate)
>
> what does sclk mean in the SoundWire context?
>
>> + return -EINVAL;
>> +
>> + return cs42l42_pll_config(dai->component, cs42l42->sclk, cs42l42->sample_rate);
>> +}
>> +
>
> There should be a really big comment here that the following functions
> implement delayed reads and writes - this was not needed for previous
> codecs.
>
>> +static int cs42l42_sdw_poll_status(struct sdw_slave *peripheral, u8 mask, u8 match)
>> +{
>> + int ret, sdwret;
>> +
>> + ret = read_poll_timeout(sdw_read_no_pm, sdwret,
>> + (sdwret < 0) || ((sdwret & mask) == match),
>> + CS42L42_DELAYED_READ_POLL_US, CS42L42_DELAYED_READ_TIMEOUT_US,
>> + false, peripheral, CS42L42_SDW_MEM_ACCESS_STATUS);
>> + if (ret == 0)
>> + ret = sdwret;
>> +
>> + if (ret < 0)
>> + dev_err(&peripheral->dev, "MEM_ACCESS_STATUS & %#x for %#x fail: %d\n",
>> + mask, match, ret);
>> +
>> + return ret;
>> +}
>> +
>> +static int cs42l42_sdw_read(void *context, unsigned int reg, unsigned int *val)
>> +{
>> + struct sdw_slave *peripheral = context;
>> + u8 data;
>> + int ret;
>> +
>> + reg += CS42L42_SDW_ADDR_OFFSET;
>> +
>> + ret = cs42l42_sdw_poll_status(peripheral, CS42L42_SDW_CMD_IN_PROGRESS, 0);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = sdw_read_no_pm(peripheral, reg);
>> + if (ret < 0) {
>> + dev_err(&peripheral->dev, "Failed to issue read @0x%x: %d\n", reg, ret);
>> + return ret;
>> + }
>> +
>> + data = (u8)ret; /* possible non-delayed read value */
>> + ret = sdw_read_no_pm(peripheral, CS42L42_SDW_MEM_ACCESS_STATUS);
>> + if (ret < 0) {
>> + dev_err(&peripheral->dev, "Failed to read MEM_ACCESS_STATUS: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + /* If read was not delayed we already have the result */
>> + if ((ret & CS42L42_SDW_LAST_LATE) == 0) {
>> + *val = data;
>> + return 0;
>> + }
>> +
>> + /* Poll for delayed read completion */
>> + if ((ret & CS42L42_SDW_RDATA_RDY) == 0) {
>> + ret = cs42l42_sdw_poll_status(peripheral,
>> + CS42L42_SDW_RDATA_RDY, CS42L42_SDW_RDATA_RDY);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> + ret = sdw_read_no_pm(peripheral, CS42L42_SDW_MEM_READ_DATA);
>> + if (ret < 0) {
>> + dev_err(&peripheral->dev, "Failed to read READ_DATA: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + *val = (u8)ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int cs42l42_sdw_write(void *context, unsigned int reg, unsigned int val)
>> +{
>> + struct sdw_slave *peripheral = context;
>> + int ret;
>> +
>> + ret = cs42l42_sdw_poll_status(peripheral, CS42L42_SDW_CMD_IN_PROGRESS, 0);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return sdw_write_no_pm(peripheral, reg + CS42L42_SDW_ADDR_OFFSET, (u8)val);
>> +}
>> +
>> +static void cs42l42_sdw_init(struct sdw_slave *peripheral)
>> +{
>> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>> + int ret = 0;
>> +
>> + regcache_cache_only(cs42l42->regmap, false);
>> +
>> + ret = cs42l42_init(cs42l42);
>> + if (ret < 0) {
>> + regcache_cache_only(cs42l42->regmap, true);
>> + return;
>> + }
>> +
>> + /* Write out any cached changes that happened between probe and attach */
>> + ret = regcache_sync(cs42l42->regmap);
>> + if (ret < 0)
>> + dev_warn(cs42l42->dev, "Failed to sync cache: %d\n", ret);
>> +
>> + /* Disable internal logic that makes clock-stop conditional */
>> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3, CS42L42_SW_CLK_STP_STAT_SEL_MASK);
>> +
>> + /*
>> + * pm_runtime is needed to control bus manager suspend, and to
>
> I don't think the intent is that the codec can control the manager
> suspend, but that the manager cannot be suspended by the framework
> unless the codec suspends first?
>
That sounds the same to me. But I can re-word the comment.
>> + * recover from an unattach_request when the manager suspends.
>> + * Autosuspend delay must be long enough to enumerate.
>
> No, this last sentence is not correct. The enumeration can be done no
> matter what pm_runtime state the Linux codec device is in. And it's
> really the other way around, it's when the codec reports as ATTACHED
> that the codec driver will be resumed.
>
It can't if the device has powered off. So there has to be some way to
ensure the codec driver won't suspend before the core has completed
enumeration and notified an ATTACH to the codec driver.
>> + */
>> + pm_runtime_set_autosuspend_delay(cs42l42->dev, 3000);
>> + pm_runtime_use_autosuspend(cs42l42->dev);
>> + pm_runtime_set_active(cs42l42->dev);
>> + pm_runtime_enable(cs42l42->dev);
>> + pm_runtime_mark_last_busy(cs42l42->dev);
>> + pm_runtime_idle(cs42l42->dev);
>> +}
>> +
>> +static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
>> +{
>> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>> + struct sdw_slave_prop *prop = &peripheral->prop;
>> + struct sdw_dpn_prop *ports;
>> +
>> + ports = devm_kcalloc(cs42l42->dev, 2, sizeof(*ports), GFP_KERNEL);
>> + if (!ports)
>> + return -ENOMEM;
>> +
>> + prop->source_ports = BIT(CS42L42_SDW_CAPTURE_PORT);
>> + prop->sink_ports = BIT(CS42L42_SDW_PLAYBACK_PORT);
>> + prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
>> + prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
>> +
>> + /* DP1 - capture */
>> + ports[0].num = CS42L42_SDW_CAPTURE_PORT,
>> + ports[0].type = SDW_DPN_FULL,
>> + ports[0].ch_prep_timeout = 10,
>> + prop->src_dpn_prop = &ports[0];
>> +
>> + /* DP2 - playback */
>> + ports[1].num = CS42L42_SDW_PLAYBACK_PORT,
>> + ports[1].type = SDW_DPN_FULL,
>> + ports[1].ch_prep_timeout = 10,
>> + prop->sink_dpn_prop = &ports[1];
>> +
>> + return 0;
>> +}
>
>> +static int cs42l42_sdw_bus_config(struct sdw_slave *peripheral,
>> + struct sdw_bus_params *params)
>> +{
>> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>> + unsigned int new_sclk = params->curr_dr_freq / 2;
>> +
>> + /* The cs42l42 cannot support a glitchless SWIRE_CLK change. */
>
> nit-pick: SoundWire
>
>> + if ((new_sclk != cs42l42->sclk) && cs42l42->stream_use) {
>> + dev_warn(cs42l42->dev, "Rejected SCLK change while audio active\n");
>> + return -EBUSY;
>> + }
>
> It's good to have this test but so far we haven't changed the bus clock
> on the fly so it's not tested.
>
>> +
>> + cs42l42->sclk = new_sclk;
>> +
>> + dev_dbg(cs42l42->dev, "bus_config: sclk=%u c=%u r=%u\n",
>> + cs42l42->sclk, params->col, params->row);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused cs42l42_sdw_clk_stop(struct sdw_slave *peripheral,
>> + enum sdw_clk_stop_mode mode,
>> + enum sdw_clk_stop_type type)
>> +{
>> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>> +
>> + dev_dbg(cs42l42->dev, "clk_stop mode:%d type:%d\n", mode, type);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct sdw_slave_ops cs42l42_sdw_ops = {
>> + .read_prop = cs42l42_sdw_read_prop,
>> + .update_status = cs42l42_sdw_update_status,
>> + .bus_config = cs42l42_sdw_bus_config,
>> +#ifdef DEBUG
>> + .clk_stop = cs42l42_sdw_clk_stop,
>> +#endif
>
> do you really need this wrapper?
>
>> +};
>> +
>> +static int __maybe_unused cs42l42_sdw_runtime_suspend(struct device *dev)
>> +{
>> + struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
>> +
>> + dev_dbg(dev, "Runtime suspend\n");
>> +
>> + /* The host controller could suspend, which would mean no register access */
>> + regcache_cache_only(cs42l42->regmap, true);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct reg_sequence __maybe_unused cs42l42_soft_reboot_seq[] = {
>> + REG_SEQ0(CS42L42_SOFT_RESET_REBOOT, 0x1e),
>> +};
>>
>> +static int __maybe_unused cs42l42_sdw_resume(struct device *dev)
>> +{
>> + struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + dev_dbg(dev, "System resume\n");
>> +
>> + /* Power-up so it can re-enumerate */
>
> ??
> You cannot power-up with the bus, did you mean that side channels are
> used for powering up?
>
>> + ret = cs42l42_resume(dev);
>> + if (ret)
>> + return ret;
>> +
>> + /* Wait for re-attach */
>> + ret = cs42l42_sdw_handle_unattach(cs42l42);
>> + if (ret < 0)
>> + return ret;
>> +
>> + cs42l42_resume_restore(dev);
>> +
>> + return 0;
>> +}
>> +
>> +static int cs42l42_sdw_probe(struct sdw_slave *peripheral, const struct sdw_device_id *id)
>> +{
>> + struct device *dev = &peripheral->dev;
>> + struct cs42l42_private *cs42l42;
>> + struct regmap_config *regmap_conf;
>> + struct regmap *regmap;
>> + struct snd_soc_component_driver *component_drv;
>> + int irq, ret;
>> +
>> + cs42l42 = devm_kzalloc(dev, sizeof(*cs42l42), GFP_KERNEL);
>> + if (!cs42l42)
>> + return -ENOMEM;
>> +
>> + if (has_acpi_companion(dev))
>> + irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
>> + else
>> + irq = of_irq_get(dev->of_node, 0);
>
> why do you need an interrupt when SoundWire provides an in-band one? You
> need a lot more explanations on the requirement and what you intend to
> do with this interrupt?
>
>> +
>> + if (irq == -ENOENT)
>> + irq = 0;
>> + else if (irq < 0)
>> + return dev_err_probe(dev, irq, "Failed to get IRQ\n");
>> +
>> + regmap_conf = devm_kmemdup(dev, &cs42l42_regmap, sizeof(cs42l42_regmap), GFP_KERNEL);
>> + if (!regmap_conf)
>> + return -ENOMEM;
>> + regmap_conf->reg_bits = 16;
>> + regmap_conf->num_ranges = 0;
>> + regmap_conf->reg_read = cs42l42_sdw_read;
>> + regmap_conf->reg_write = cs42l42_sdw_write;
>> +
>> + regmap = devm_regmap_init(dev, NULL, peripheral, regmap_conf);
>> + if (IS_ERR(regmap))
>> + return dev_err_probe(dev, PTR_ERR(regmap), "Failed to allocate register map\n");
>> +
>> + /* Start in cache-only until device is enumerated */
>> + regcache_cache_only(regmap, true);
>> +
>> + component_drv = devm_kmemdup(dev,
>> + &cs42l42_soc_component,
>> + sizeof(cs42l42_soc_component),
>> + GFP_KERNEL);
>> + if (!component_drv)
>> + return -ENOMEM;
>> +
>> + component_drv->dapm_routes = cs42l42_sdw_audio_map;
>> + component_drv->num_dapm_routes = ARRAY_SIZE(cs42l42_sdw_audio_map);
>> +
>> + cs42l42->dev = dev;
>> + cs42l42->regmap = regmap;
>> + cs42l42->sdw_peripheral = peripheral;
>> + cs42l42->irq = irq;
>> +
>> + ret = cs42l42_common_probe(cs42l42, component_drv, &cs42l42_sdw_dai);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int cs42l42_sdw_remove(struct sdw_slave *peripheral)
>> +{
>> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>> +
>> + /* Resume so that cs42l42_remove() can access registers */
>> + pm_runtime_get_sync(cs42l42->dev);
>
> you need to test if this resume succeeded, and possibly use
> pm_resume_resume_and_get() to avoid issues.
>
>> + cs42l42_common_remove(cs42l42);
>> + pm_runtime_put(cs42l42->dev);
>> + pm_runtime_disable(cs42l42->dev);
>> +
>> + return 0;
>> +}
>
>> static const struct snd_soc_dapm_route cs42l42_audio_map[] = {
>> @@ -559,6 +564,20 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
>> {
>> struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
>>
>> + /*
>> + * If the Soundwire controller issues bus reset when coming out of
>> + * clock-stop it will erase the jack state. This can lose button press
>> + * events, and plug/unplug interrupt bits take between 125ms and 1500ms
>> + * before they are valid again.
>> + * Prevent this by holding our pm_runtime to block clock-stop.
>> + */
>> + if (cs42l42->sdw_peripheral) {
>> + if (jk)
>> + pm_runtime_get_sync(cs42l42->dev);
>> + else
>> + pm_runtime_put_autosuspend(cs42l42->dev);
>> + }
>> +
>
> I *really* don't understand this sequence.
>
> The bus will be suspended when ALL devices have been idle for some time.
> If the user presses a button AFTER the bus is suspended, the device can
> still use the in-band wake and resume.
Only if it has that capability. The cs42l42 has very limited wake
capability and cannot wake on interrupt, and it certainly can't accept
the Intel code resetting it before it has a chance to find out what
condition caused the wake.
> Granted the button press will be lost but the plug/unplug status will
> still be handled with a delay.
>
I'm finding it difficult to believe it's acceptable to end users for
button events to be lost.
>> /* Prevent race with interrupt handler */
>> mutex_lock(&cs42l42->irq_lock);
>> cs42l42->jack = jk;
>> @@ -1645,9 +1664,11 @@ irqreturn_t cs42l42_irq_thread(int irq, void *data)
>> unsigned int current_button_status;
>> unsigned int i;
>>
>> + pm_runtime_get_sync(cs42l42->dev);
>> mutex_lock(&cs42l42->irq_lock);
>> if (cs42l42->suspended || !cs42l42->init_done) {
>> mutex_unlock(&cs42l42->irq_lock);
>> + pm_runtime_put_autosuspend(cs42l42->dev);
>> return IRQ_NONE;
>> }
>>
>> @@ -1750,6 +1771,8 @@ irqreturn_t cs42l42_irq_thread(int irq, void *data)
>> }
>>
>> mutex_unlock(&cs42l42->irq_lock);
>> + pm_runtime_mark_last_busy(cs42l42->dev);
>> + pm_runtime_put_autosuspend(cs42l42->dev);
>>
>> return IRQ_HANDLED;
>
> Again in SoundWire more you should not use a dedicated interrupt.
> There's something missing in the explanations on why this thread is
> required.
>
Do you have a situation where it will actually cause a problem or are
you just saying that in an ideal world where all the hardware was
perfect it wouldn't need one?
Bear in mind that cs42l42 is roughly 7 years old so its Soundwire
implementation may not be all that you'd expect from a device designed
today to SW1.2 with Soundwire as its primary interface.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 11/12] ASoC: cs42l42: Add Soundwire support
2022-08-22 13:50 ` Richard Fitzgerald
@ 2022-08-22 14:55 ` Pierre-Louis Bossart
2022-08-22 16:31 ` Richard Fitzgerald
0 siblings, 1 reply; 21+ messages in thread
From: Pierre-Louis Bossart @ 2022-08-22 14:55 UTC (permalink / raw)
To: Richard Fitzgerald, broonie; +Cc: patches, alsa-devel, linux-kernel
Thanks Richard for your answers, good discussion. There are several
topics I could use more details to understand your line of thought and
requirements, see below.
>>> - Intel Soundwire host controllers have a low-power clock-stop mode that
>>> requires resetting all peripherals when resuming. This is not
>>> compatible
>>> with the plug-detect and button-detect because it will clear the
>>> condition that caused the wakeup before the driver can handle it. So
>>> clock-stop must be blocked when a snd_soc_jack handler is registered.
>>
>> What do you mean by 'clock-stop must be blocked'? The peripheral cannot
>> prevent the host from stopping the clock.
>
> Are you sure about that? We're going to have serious problems if the
> Intel manager driver can clock-stop even though there are peripheral
> drivers still using the clock.
>
> Currently the Intel code will only clock-stop when it goes into
> runtime suspend, which only happens if all peripheral drivers are also
> in runtime suspend. Or on system suspend, which is handled specially by
> the cs42l42 driver. If you are saying this isn't guaranteed behavior
> then we'll need to add something to the core Soundwire core code to tell
> it when it's allowed to clock-stop.
If the peripheral remains pm_runtime active, the manager will never
suspend, but power-wise that's a non-starter.
The premise is that the audio subsystem goes to a low-power state with
only a detector running. The functionality will resume on *any* in-band
wake coming from the peripheral.
> I tried returning an error from the codec driver clk_stop() callback but
> the core code and cadence code treat that as unexpected and dev_err()
> it, then the intel.c code ignores the error and carries on suspending.
Yes, we ignore those errors on purpose, because when the system restarts
the device will have gone through a reset sequence. I don't see a good
reason to prevent a pm_runtime or system suspend, this would have very
large impacts on standby power.
> Maybe this is explained
>> further down in this patch, but that statement is a bit odd.
>>
>> Even if the condition that caused the wakeup was cleared, presumably
>> when resetting the device the same condition will be raised again, no?
>>
>>> +static int cs42l42_sdw_dai_hw_params(struct snd_pcm_substream
>>> *substream,
>>> + struct snd_pcm_hw_params *params,
>>> + struct snd_soc_dai *dai)
>>> +{
>>> + struct cs42l42_private *cs42l42 =
>>> snd_soc_component_get_drvdata(dai->component);
>>> + struct sdw_stream_runtime *sdw_stream =
>>> snd_soc_dai_get_dma_data(dai, substream);
>>> + struct sdw_stream_config sconfig;
>>> + struct sdw_port_config pconfig;
>>> + unsigned int pdn_mask;
>>> + int ret;
>>> +
>>> + if (!sdw_stream)
>>> + return -EINVAL;
>>> +
>>> + /* Needed for PLL configuration when we are notified of new bus
>>> config */
>>> + cs42l42->sample_rate = params_rate(params);
>>> +
>>> + memset(&sconfig, 0, sizeof(sconfig));
>>> + memset(&pconfig, 0, sizeof(pconfig));
>>> +
>>> + sconfig.frame_rate = params_rate(params);
>>> + sconfig.ch_count = params_channels(params);
>>> + sconfig.bps = snd_pcm_format_width(params_format(params));
>>> + pconfig.ch_mask = GENMASK(sconfig.ch_count - 1, 0);
>>> +
>>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>>> + sconfig.direction = SDW_DATA_DIR_RX;
>>> + pconfig.num = CS42L42_SDW_PLAYBACK_PORT;
>>> + pdn_mask = CS42L42_HP_PDN_MASK;
>>> + } else {
>>> + sconfig.direction = SDW_DATA_DIR_TX;
>>> + pconfig.num = CS42L42_SDW_CAPTURE_PORT;
>>> + pdn_mask = CS42L42_ADC_PDN_MASK;
>>> + }
>>> +
>>> + /*
>>> + * The DAI-link prepare() will trigger Soundwire DP prepare. But
>>> CS42L42
>>> + * DP will only prepare if the HP/ADC is already powered-up. The
>>> + * DAI prepare() and DAPM sequence run after DAI-link prepare()
>>> so the
>>> + * PDN bit must be written here.
>>> + */
>>
>> Why not make use of the callbacks that were added precisely to let the
>> codec driver perform device-specific operations? You can add your own
>> code in pre and post operation for both prepare and bank switch. It's
>> not clear why you would do this in a hw_params (which can be called
>> multiple times per ALSA conventions).
>>
>
> Ah, I'd not noticed the port_prep callback. Maybe it didn't exist when
> this code was first written.
it's been upstream since 4.17 in 2018, and earlier in internal releases.
>>> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
>>> + usleep_range(CS42L42_HP_ADC_EN_TIME_US,
>>> CS42L42_HP_ADC_EN_TIME_US + 1000);
>>> +
>>> + ret = sdw_stream_add_slave(cs42l42->sdw_peripheral, &sconfig,
>>> &pconfig, 1, sdw_stream);
>>> + if (ret) {
>>> + dev_err(dai->dev, "Failed to add sdw stream: %d\n", ret);
>>> + goto err;
>>> + }
>>> +
>>> + cs42l42_src_config(dai->component, params_rate(params));
>>> +
>>> + return 0;
>>> +
>>> +err:
>>> + regmap_set_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void cs42l42_sdw_init(struct sdw_slave *peripheral)
>>> +{
>>> + struct cs42l42_private *cs42l42 =
>>> dev_get_drvdata(&peripheral->dev);
>>> + int ret = 0;
>>> +
>>> + regcache_cache_only(cs42l42->regmap, false);
>>> +
>>> + ret = cs42l42_init(cs42l42);
>>> + if (ret < 0) {
>>> + regcache_cache_only(cs42l42->regmap, true);
>>> + return;
>>> + }
>>> +
>>> + /* Write out any cached changes that happened between probe and
>>> attach */
>>> + ret = regcache_sync(cs42l42->regmap);
>>> + if (ret < 0)
>>> + dev_warn(cs42l42->dev, "Failed to sync cache: %d\n", ret);
>>> +
>>> + /* Disable internal logic that makes clock-stop conditional */
>>> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3,
>>> CS42L42_SW_CLK_STP_STAT_SEL_MASK);
>>> +
>>> + /*
>>> + * pm_runtime is needed to control bus manager suspend, and to
>>
>> I don't think the intent is that the codec can control the manager
>> suspend, but that the manager cannot be suspended by the framework
>> unless the codec suspends first?
>>
>
> That sounds the same to me. But I can re-word the comment.
the initial wording makes it sound like you want to actively control the
manager state, that's different to letting the framework deal with the
parent-child relationship.
>
>>> + * recover from an unattach_request when the manager suspends.
>>> + * Autosuspend delay must be long enough to enumerate.
>>
>> No, this last sentence is not correct. The enumeration can be done no
>> matter what pm_runtime state the Linux codec device is in. And it's
>> really the other way around, it's when the codec reports as ATTACHED
>> that the codec driver will be resumed.
>>
>
> It can't if the device has powered off. So there has to be some way to
> ensure the codec driver won't suspend before the core has completed
> enumeration and notified an ATTACH to the codec driver.
Powered-off? We don't have any mechanisms in SoundWire to deal with
power. Can you describe what the sequence should be?
All existing codecs will look for the sync pattern as soon as the bus
reset is complete. The functionality behind the interface might be off,
but that's a separate topic.
if it's required to resume the child device when the manager resumes, so
as to deal with sideband power management then indeed this would be a
SoundWire core change. It's not hard to do, we've already implemented a
loop to force codec devices to pm_runtime resume in a .prepare callback,
we could tag the device with the flag.
>>> + */
>>> + pm_runtime_set_autosuspend_delay(cs42l42->dev, 3000);
>>> + pm_runtime_use_autosuspend(cs42l42->dev);
>>> + pm_runtime_set_active(cs42l42->dev);
>>> + pm_runtime_enable(cs42l42->dev);
>>> + pm_runtime_mark_last_busy(cs42l42->dev);
>>> + pm_runtime_idle(cs42l42->dev);
>>> +}
>>> static const struct snd_soc_dapm_route cs42l42_audio_map[] = {
>>> @@ -559,6 +564,20 @@ static int cs42l42_set_jack(struct
>>> snd_soc_component *component, struct snd_soc_
>>> {
>>> struct cs42l42_private *cs42l42 =
>>> snd_soc_component_get_drvdata(component);
>>> + /*
>>> + * If the Soundwire controller issues bus reset when coming out of
>>> + * clock-stop it will erase the jack state. This can lose button
>>> press
>>> + * events, and plug/unplug interrupt bits take between 125ms and
>>> 1500ms
>>> + * before they are valid again.
>>> + * Prevent this by holding our pm_runtime to block clock-stop.
>>> + */
>>> + if (cs42l42->sdw_peripheral) {
>>> + if (jk)
>>> + pm_runtime_get_sync(cs42l42->dev);
>>> + else
>>> + pm_runtime_put_autosuspend(cs42l42->dev);
>>> + }
>>> +
>>
>> I *really* don't understand this sequence.
>>
>> The bus will be suspended when ALL devices have been idle for some time.
>> If the user presses a button AFTER the bus is suspended, the device can
>> still use the in-band wake and resume.
>
> Only if it has that capability. The cs42l42 has very limited wake
> capability and cannot wake on interrupt, and it certainly can't accept
> the Intel code resetting it before it has a chance to find out what
> condition caused the wake.
>
>> Granted the button press will be lost but the plug/unplug status will
>> still be handled with a delay.
>>
>
> I'm finding it difficult to believe it's acceptable to end users for
> button events to be lost.
I don't understand what 'limited wake functionality' means. It can
either generate a wake or it cannot.
In the event that it can, then the Intel manager will detect an in-band
wake and restart the system. When the headset device enumerates and
initializes, it should initiate a check for the jack status. Button
press will be handled once plug-in status is confirmed.
I don't think there is a requirement to keep track of every button press
why the system is suspended. The user-experience is that the system
restarts and plug-in or button-press are handled at some point. It would
be counter-productive to prevent the Intel manager from suspending to
save even 500ms on restart.
>>> /* Prevent race with interrupt handler */
>>> mutex_lock(&cs42l42->irq_lock);
>>> cs42l42->jack = jk;
>>> @@ -1645,9 +1664,11 @@ irqreturn_t cs42l42_irq_thread(int irq, void
>>> *data)
>>> unsigned int current_button_status;
>>> unsigned int i;
>>> + pm_runtime_get_sync(cs42l42->dev);
>>> mutex_lock(&cs42l42->irq_lock);
>>> if (cs42l42->suspended || !cs42l42->init_done) {
>>> mutex_unlock(&cs42l42->irq_lock);
>>> + pm_runtime_put_autosuspend(cs42l42->dev);
>>> return IRQ_NONE;
>>> }
>>> @@ -1750,6 +1771,8 @@ irqreturn_t cs42l42_irq_thread(int irq, void
>>> *data)
>>> }
>>> mutex_unlock(&cs42l42->irq_lock);
>>> + pm_runtime_mark_last_busy(cs42l42->dev);
>>> + pm_runtime_put_autosuspend(cs42l42->dev);
>>> return IRQ_HANDLED;
>>
>> Again in SoundWire more you should not use a dedicated interrupt.
>> There's something missing in the explanations on why this thread is
>> required.
>>
>
> Do you have a situation where it will actually cause a problem or are
> you just saying that in an ideal world where all the hardware was
> perfect it wouldn't need one?
> Bear in mind that cs42l42 is roughly 7 years old so its Soundwire
> implementation may not be all that you'd expect from a device designed
> today to SW1.2 with Soundwire as its primary interface.
Nothing is ideal in a standard, there's always different
interpretations, that's ok.
We've never seen a device with a dedicated interrupt line and I think
it's only fair to ask why it was necessary. It's extra complexity for
BIOS integration, possibly machine driver, and more validation work.
If the message was that a dedicated interrupt line is required, let's
enable it. If the in-band wake is good-enough, then let's avoid more
options.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 11/12] ASoC: cs42l42: Add Soundwire support
2022-08-22 14:55 ` Pierre-Louis Bossart
@ 2022-08-22 16:31 ` Richard Fitzgerald
2022-08-22 17:15 ` Pierre-Louis Bossart
0 siblings, 1 reply; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-22 16:31 UTC (permalink / raw)
To: Pierre-Louis Bossart, broonie; +Cc: patches, alsa-devel, linux-kernel
On 22/08/2022 15:55, Pierre-Louis Bossart wrote:
> Thanks Richard for your answers, good discussion. There are several
> topics I could use more details to understand your line of thought and
> requirements, see below.
>
>>>> - Intel Soundwire host controllers have a low-power clock-stop mode that
>>>> requires resetting all peripherals when resuming. This is not
>>>> compatible
>>>> with the plug-detect and button-detect because it will clear the
>>>> condition that caused the wakeup before the driver can handle it. So
>>>> clock-stop must be blocked when a snd_soc_jack handler is registered.
>>>
>>> What do you mean by 'clock-stop must be blocked'? The peripheral cannot
>>> prevent the host from stopping the clock.
>>
>> Are you sure about that? We're going to have serious problems if the
>> Intel manager driver can clock-stop even though there are peripheral
>> drivers still using the clock.
>>
>> Currently the Intel code will only clock-stop when it goes into
>> runtime suspend, which only happens if all peripheral drivers are also
>> in runtime suspend. Or on system suspend, which is handled specially by
>> the cs42l42 driver. If you are saying this isn't guaranteed behavior
>> then we'll need to add something to the core Soundwire core code to tell
>> it when it's allowed to clock-stop.
>
> If the peripheral remains pm_runtime active, the manager will never
> suspend, but power-wise that's a non-starter.
>
I agree it's not ideal but ultimately you get what the hardware can
support, The cs42l42 driver doesn't support runtime suspend in I2C mode.
It's not a critical blocker to delay submitting any CS42L42 Soundwire
support to the kernel.
> The premise is that the audio subsystem goes to a low-power state with
> only a detector running. The functionality will resume on *any* in-band
> wake coming from the peripheral.
>
>> I tried returning an error from the codec driver clk_stop() callback but
>> the core code and cadence code treat that as unexpected and dev_err()
>> it, then the intel.c code ignores the error and carries on suspending.
>
> Yes, we ignore those errors on purpose, because when the system restarts
> the device will have gone through a reset sequence. I don't see a good
> reason to prevent a pm_runtime or system suspend, this would have very
> large impacts on standby power.
>
>> Maybe this is explained
>>> further down in this patch, but that statement is a bit odd.
>>>
>>> Even if the condition that caused the wakeup was cleared, presumably
>>> when resetting the device the same condition will be raised again, no?
>>>
>>>> +static int cs42l42_sdw_dai_hw_params(struct snd_pcm_substream
>>>> *substream,
>>>> + struct snd_pcm_hw_params *params,
>>>> + struct snd_soc_dai *dai)
>>>> +{
>>>> + struct cs42l42_private *cs42l42 =
>>>> snd_soc_component_get_drvdata(dai->component);
>>>> + struct sdw_stream_runtime *sdw_stream =
>>>> snd_soc_dai_get_dma_data(dai, substream);
>>>> + struct sdw_stream_config sconfig;
>>>> + struct sdw_port_config pconfig;
>>>> + unsigned int pdn_mask;
>>>> + int ret;
>>>> +
>>>> + if (!sdw_stream)
>>>> + return -EINVAL;
>>>> +
>>>> + /* Needed for PLL configuration when we are notified of new bus
>>>> config */
>>>> + cs42l42->sample_rate = params_rate(params);
>>>> +
>>>> + memset(&sconfig, 0, sizeof(sconfig));
>>>> + memset(&pconfig, 0, sizeof(pconfig));
>>>> +
>>>> + sconfig.frame_rate = params_rate(params);
>>>> + sconfig.ch_count = params_channels(params);
>>>> + sconfig.bps = snd_pcm_format_width(params_format(params));
>>>> + pconfig.ch_mask = GENMASK(sconfig.ch_count - 1, 0);
>>>> +
>>>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>>>> + sconfig.direction = SDW_DATA_DIR_RX;
>>>> + pconfig.num = CS42L42_SDW_PLAYBACK_PORT;
>>>> + pdn_mask = CS42L42_HP_PDN_MASK;
>>>> + } else {
>>>> + sconfig.direction = SDW_DATA_DIR_TX;
>>>> + pconfig.num = CS42L42_SDW_CAPTURE_PORT;
>>>> + pdn_mask = CS42L42_ADC_PDN_MASK;
>>>> + }
>>>> +
>>>> + /*
>>>> + * The DAI-link prepare() will trigger Soundwire DP prepare. But
>>>> CS42L42
>>>> + * DP will only prepare if the HP/ADC is already powered-up. The
>>>> + * DAI prepare() and DAPM sequence run after DAI-link prepare()
>>>> so the
>>>> + * PDN bit must be written here.
>>>> + */
>>>
>>> Why not make use of the callbacks that were added precisely to let the
>>> codec driver perform device-specific operations? You can add your own
>>> code in pre and post operation for both prepare and bank switch. It's
>>> not clear why you would do this in a hw_params (which can be called
>>> multiple times per ALSA conventions).
>>>
>>
>> Ah, I'd not noticed the port_prep callback. Maybe it didn't exist when
>> this code was first written.
>
> it's been upstream since 4.17 in 2018, and earlier in internal releases.
>
>>>> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
>>>> + usleep_range(CS42L42_HP_ADC_EN_TIME_US,
>>>> CS42L42_HP_ADC_EN_TIME_US + 1000);
>>>> +
>>>> + ret = sdw_stream_add_slave(cs42l42->sdw_peripheral, &sconfig,
>>>> &pconfig, 1, sdw_stream);
>>>> + if (ret) {
>>>> + dev_err(dai->dev, "Failed to add sdw stream: %d\n", ret);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + cs42l42_src_config(dai->component, params_rate(params));
>>>> +
>>>> + return 0;
>>>> +
>>>> +err:
>>>> + regmap_set_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>
>>>> +static void cs42l42_sdw_init(struct sdw_slave *peripheral)
>>>> +{
>>>> + struct cs42l42_private *cs42l42 =
>>>> dev_get_drvdata(&peripheral->dev);
>>>> + int ret = 0;
>>>> +
>>>> + regcache_cache_only(cs42l42->regmap, false);
>>>> +
>>>> + ret = cs42l42_init(cs42l42);
>>>> + if (ret < 0) {
>>>> + regcache_cache_only(cs42l42->regmap, true);
>>>> + return;
>>>> + }
>>>> +
>>>> + /* Write out any cached changes that happened between probe and
>>>> attach */
>>>> + ret = regcache_sync(cs42l42->regmap);
>>>> + if (ret < 0)
>>>> + dev_warn(cs42l42->dev, "Failed to sync cache: %d\n", ret);
>>>> +
>>>> + /* Disable internal logic that makes clock-stop conditional */
>>>> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3,
>>>> CS42L42_SW_CLK_STP_STAT_SEL_MASK);
>>>> +
>>>> + /*
>>>> + * pm_runtime is needed to control bus manager suspend, and to
>>>
>>> I don't think the intent is that the codec can control the manager
>>> suspend, but that the manager cannot be suspended by the framework
>>> unless the codec suspends first?
>>>
>>
>> That sounds the same to me. But I can re-word the comment.
>
> the initial wording makes it sound like you want to actively control the
> manager state, that's different to letting the framework deal with the
> parent-child relationship.
>
>>
>>>> + * recover from an unattach_request when the manager suspends.
>>>> + * Autosuspend delay must be long enough to enumerate.
>>>
>>> No, this last sentence is not correct. The enumeration can be done no
>>> matter what pm_runtime state the Linux codec device is in. And it's
>>> really the other way around, it's when the codec reports as ATTACHED
>>> that the codec driver will be resumed.
>>>
>>
>> It can't if the device has powered off. So there has to be some way to
>> ensure the codec driver won't suspend before the core has completed
>> enumeration and notified an ATTACH to the codec driver.
>
> Powered-off? We don't have any mechanisms in SoundWire to deal with
> power. Can you describe what the sequence should be?
>
> All existing codecs will look for the sync pattern as soon as the bus
> reset is complete. The functionality behind the interface might be off,
> but that's a separate topic.
>
What I'm thinking of is what to do if the driver probe()s but the
peripheral never enumerates. Should we take some action to maybe
turn it off (like asserting RESET?). Or is it ok to leave it on
forever?
Though in the case of cs42l42.c the runtime_suspend doesn't power-off or
reset so it doesn't really matter. The comment about autosuspend is
now redundant and can be deleted.
> if it's required to resume the child device when the manager resumes, so
> as to deal with sideband power management then indeed this would be a
> SoundWire core change. It's not hard to do, we've already implemented a
> loop to force codec devices to pm_runtime resume in a .prepare callback,
> we could tag the device with the flag.
>
>>>> + */
>>>> + pm_runtime_set_autosuspend_delay(cs42l42->dev, 3000);
>>>> + pm_runtime_use_autosuspend(cs42l42->dev);
>>>> + pm_runtime_set_active(cs42l42->dev);
>>>> + pm_runtime_enable(cs42l42->dev);
>>>> + pm_runtime_mark_last_busy(cs42l42->dev);
>>>> + pm_runtime_idle(cs42l42->dev);
>>>> +}
>
>>>> static const struct snd_soc_dapm_route cs42l42_audio_map[] = {
>>>> @@ -559,6 +564,20 @@ static int cs42l42_set_jack(struct
>>>> snd_soc_component *component, struct snd_soc_
>>>> {
>>>> struct cs42l42_private *cs42l42 =
>>>> snd_soc_component_get_drvdata(component);
>>>> + /*
>>>> + * If the Soundwire controller issues bus reset when coming out of
>>>> + * clock-stop it will erase the jack state. This can lose button
>>>> press
>>>> + * events, and plug/unplug interrupt bits take between 125ms and
>>>> 1500ms
>>>> + * before they are valid again.
>>>> + * Prevent this by holding our pm_runtime to block clock-stop.
>>>> + */
>>>> + if (cs42l42->sdw_peripheral) {
>>>> + if (jk)
>>>> + pm_runtime_get_sync(cs42l42->dev);
>>>> + else
>>>> + pm_runtime_put_autosuspend(cs42l42->dev);
>>>> + }
>>>> +
>>>
>>> I *really* don't understand this sequence.
>>>
>>> The bus will be suspended when ALL devices have been idle for some time.
>>> If the user presses a button AFTER the bus is suspended, the device can
>>> still use the in-band wake and resume.
>>
>> Only if it has that capability. The cs42l42 has very limited wake
>> capability and cannot wake on interrupt, and it certainly can't accept
>> the Intel code resetting it before it has a chance to find out what
>> condition caused the wake.
>>
>>> Granted the button press will be lost but the plug/unplug status will
>>> still be handled with a delay.
>>>
>>
>> I'm finding it difficult to believe it's acceptable to end users for
>> button events to be lost.
>
> I don't understand what 'limited wake functionality' means. It can
> either generate a wake or it cannot.
It can generate wakes. Whether it can generate one when you want one
is another question entirely...
>
> In the event that it can, then the Intel manager will detect an in-band
> wake and restart the system. When the headset device enumerates and
> initializes, it should initiate a check for the jack status. Button
> press will be handled once plug-in status is confirmed.
>
> I don't think there is a requirement to keep track of every button press
> why the system is suspended. The user-experience is that the system
> restarts and plug-in or button-press are handled at some point. It would
> be counter-productive to prevent the Intel manager from suspending to
> save even 500ms on restart.
>
>>>> /* Prevent race with interrupt handler */
>>>> mutex_lock(&cs42l42->irq_lock);
>>>> cs42l42->jack = jk;
>>>> @@ -1645,9 +1664,11 @@ irqreturn_t cs42l42_irq_thread(int irq, void
>>>> *data)
>>>> unsigned int current_button_status;
>>>> unsigned int i;
>>>> + pm_runtime_get_sync(cs42l42->dev);
>>>> mutex_lock(&cs42l42->irq_lock);
>>>> if (cs42l42->suspended || !cs42l42->init_done) {
>>>> mutex_unlock(&cs42l42->irq_lock);
>>>> + pm_runtime_put_autosuspend(cs42l42->dev);
>>>> return IRQ_NONE;
>>>> }
>>>> @@ -1750,6 +1771,8 @@ irqreturn_t cs42l42_irq_thread(int irq, void
>>>> *data)
>>>> }
>>>> mutex_unlock(&cs42l42->irq_lock);
>>>> + pm_runtime_mark_last_busy(cs42l42->dev);
>>>> + pm_runtime_put_autosuspend(cs42l42->dev);
>>>> return IRQ_HANDLED;
>>>
>>> Again in SoundWire more you should not use a dedicated interrupt.
>>> There's something missing in the explanations on why this thread is
>>> required.
>>>
>>
>> Do you have a situation where it will actually cause a problem or are
>> you just saying that in an ideal world where all the hardware was
>> perfect it wouldn't need one?
>> Bear in mind that cs42l42 is roughly 7 years old so its Soundwire
>> implementation may not be all that you'd expect from a device designed
>> today to SW1.2 with Soundwire as its primary interface.
>
> Nothing is ideal in a standard, there's always different
> interpretations, that's ok.
>
> We've never seen a device with a dedicated interrupt line and I think
> it's only fair to ask why it was necessary. It's extra complexity for
> BIOS integration, possibly machine driver, and more validation work.
>
> If the message was that a dedicated interrupt line is required, let's
> enable it. If the in-band wake is good-enough, then let's avoid more
> options.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 11/12] ASoC: cs42l42: Add Soundwire support
2022-08-22 16:31 ` Richard Fitzgerald
@ 2022-08-22 17:15 ` Pierre-Louis Bossart
0 siblings, 0 replies; 21+ messages in thread
From: Pierre-Louis Bossart @ 2022-08-22 17:15 UTC (permalink / raw)
To: Richard Fitzgerald, broonie; +Cc: patches, alsa-devel, linux-kernel
On 8/22/22 18:31, Richard Fitzgerald wrote:
> On 22/08/2022 15:55, Pierre-Louis Bossart wrote:
>> Thanks Richard for your answers, good discussion. There are several
>> topics I could use more details to understand your line of thought and
>> requirements, see below.
>>
>>>>> - Intel Soundwire host controllers have a low-power clock-stop mode
>>>>> that
>>>>> requires resetting all peripherals when resuming. This is not
>>>>> compatible
>>>>> with the plug-detect and button-detect because it will clear the
>>>>> condition that caused the wakeup before the driver can handle
>>>>> it. So
>>>>> clock-stop must be blocked when a snd_soc_jack handler is
>>>>> registered.
>>>>
>>>> What do you mean by 'clock-stop must be blocked'? The peripheral cannot
>>>> prevent the host from stopping the clock.
>>>
>>> Are you sure about that? We're going to have serious problems if the
>>> Intel manager driver can clock-stop even though there are peripheral
>>> drivers still using the clock.
>>>
>>> Currently the Intel code will only clock-stop when it goes into
>>> runtime suspend, which only happens if all peripheral drivers are also
>>> in runtime suspend. Or on system suspend, which is handled specially by
>>> the cs42l42 driver. If you are saying this isn't guaranteed behavior
>>> then we'll need to add something to the core Soundwire core code to tell
>>> it when it's allowed to clock-stop.
>>
>> If the peripheral remains pm_runtime active, the manager will never
>> suspend, but power-wise that's a non-starter.
>>
>
> I agree it's not ideal but ultimately you get what the hardware can
> support, The cs42l42 driver doesn't support runtime suspend in I2C mode.
It's a completely different mode. In I2C mode, the Intel DSP is
suspended until there's something audio-related to do. In SoundWire
mode, the DSP needs to remain partly powered and that's a real issue if
the DSP cannot suspend.
> It's not a critical blocker to delay submitting any CS42L42 Soundwire
> support to the kernel.
I wasn't trying to push back, more to understand what needs to happen to
support this device.
We could also add the 'normal' clock stop mode and see how things go
first. IIRC we have a quirk already in the SOF driver.
>>>>> + * recover from an unattach_request when the manager suspends.
>>>>> + * Autosuspend delay must be long enough to enumerate.
>>>>
>>>> No, this last sentence is not correct. The enumeration can be done no
>>>> matter what pm_runtime state the Linux codec device is in. And it's
>>>> really the other way around, it's when the codec reports as ATTACHED
>>>> that the codec driver will be resumed.
>>>>
>>>
>>> It can't if the device has powered off. So there has to be some way to
>>> ensure the codec driver won't suspend before the core has completed
>>> enumeration and notified an ATTACH to the codec driver.
>>
>> Powered-off? We don't have any mechanisms in SoundWire to deal with
>> power. Can you describe what the sequence should be?
>>
>> All existing codecs will look for the sync pattern as soon as the bus
>> reset is complete. The functionality behind the interface might be off,
>> but that's a separate topic.
>>
>
> What I'm thinking of is what to do if the driver probe()s but the
> peripheral never enumerates. Should we take some action to maybe
> turn it off (like asserting RESET?). Or is it ok to leave it on
> forever?
>
> Though in the case of cs42l42.c the runtime_suspend doesn't power-off or
> reset so it doesn't really matter. The comment about autosuspend is
> now redundant and can be deleted.
It's really common for us to see a driver probe and the device never
enumerates - that's typical of 'ghost' devices exposed in the ACPI DSDT
but not populated in hardware. It's fine, nothing will happen.
I am not sure what you mean by asserting RESET because until the device
is enumerated, you cannot talk to it with the SoundWire command
protocol. You could always have some sort of sideband power link, but
that would require a bit more work at the core level to switch that
sideband on when the manager resumes.
>
>> if it's required to resume the child device when the manager resumes, so
>> as to deal with sideband power management then indeed this would be a
>> SoundWire core change. It's not hard to do, we've already implemented a
>> loop to force codec devices to pm_runtime resume in a .prepare callback,
>> we could tag the device with the flag.
>>
>>>>> + */
>>>>> + pm_runtime_set_autosuspend_delay(cs42l42->dev, 3000);
>>>>> + pm_runtime_use_autosuspend(cs42l42->dev);
>>>>> + pm_runtime_set_active(cs42l42->dev);
>>>>> + pm_runtime_enable(cs42l42->dev);
>>>>> + pm_runtime_mark_last_busy(cs42l42->dev);
>>>>> + pm_runtime_idle(cs42l42->dev);
>>>>> +}
>>
>>>>> static const struct snd_soc_dapm_route cs42l42_audio_map[] = {
>>>>> @@ -559,6 +564,20 @@ static int cs42l42_set_jack(struct
>>>>> snd_soc_component *component, struct snd_soc_
>>>>> {
>>>>> struct cs42l42_private *cs42l42 =
>>>>> snd_soc_component_get_drvdata(component);
>>>>> + /*
>>>>> + * If the Soundwire controller issues bus reset when coming
>>>>> out of
>>>>> + * clock-stop it will erase the jack state. This can lose button
>>>>> press
>>>>> + * events, and plug/unplug interrupt bits take between 125ms and
>>>>> 1500ms
>>>>> + * before they are valid again.
>>>>> + * Prevent this by holding our pm_runtime to block clock-stop.
>>>>> + */
>>>>> + if (cs42l42->sdw_peripheral) {
>>>>> + if (jk)
>>>>> + pm_runtime_get_sync(cs42l42->dev);
>>>>> + else
>>>>> + pm_runtime_put_autosuspend(cs42l42->dev);
>>>>> + }
>>>>> +
>>>>
>>>> I *really* don't understand this sequence.
>>>>
>>>> The bus will be suspended when ALL devices have been idle for some
>>>> time.
>>>> If the user presses a button AFTER the bus is suspended, the device can
>>>> still use the in-band wake and resume.
>>>
>>> Only if it has that capability. The cs42l42 has very limited wake
>>> capability and cannot wake on interrupt, and it certainly can't accept
>>> the Intel code resetting it before it has a chance to find out what
>>> condition caused the wake.
>>>
>>>> Granted the button press will be lost but the plug/unplug status will
>>>> still be handled with a delay.
>>>>
>>>
>>> I'm finding it difficult to believe it's acceptable to end users for
>>> button events to be lost.
>>
>> I don't understand what 'limited wake functionality' means. It can
>> either generate a wake or it cannot.
>
> It can generate wakes. Whether it can generate one when you want one
> is another question entirely...
You're losing me here. the in-band wake is only relevant in the context
of clock-stop. The manager has zero expectations as to when those wakes
are asserted.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 12/12] ASoC: cs42l42: Add support for Soundwire interrupts
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (10 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 11/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
@ 2022-08-19 12:52 ` Richard Fitzgerald
2022-08-22 11:33 ` Pierre-Louis Bossart
2022-09-23 16:54 ` [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Mark Brown
12 siblings, 1 reply; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-19 12:52 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, patches, Richard Fitzgerald
This adds support for using the Soundwire interrupt mechanism to
handle CS42L42 chip interrupts.
Soundwire interrupts are used if a hard INT line is not declared.
Wake-from-clock-stop is not used. The CS42L42 has limited wake
capability, but clock-stop is already disabled when a snd_soc_jack is
registered to prevent the host controller issuing a bus-reset on exit
from clock stop mode, which would clear the interrupt status and break
jack and button detection.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs42l42-sdw.c | 90 +++++++++++++++++++++++++++++++++-
sound/soc/codecs/cs42l42.h | 3 ++
2 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/cs42l42-sdw.c b/sound/soc/codecs/cs42l42-sdw.c
index ed69a0a44d8c..1bdeed93587d 100644
--- a/sound/soc/codecs/cs42l42-sdw.c
+++ b/sound/soc/codecs/cs42l42-sdw.c
@@ -14,6 +14,7 @@
#include <linux/soundwire/sdw.h>
#include <linux/soundwire/sdw_registers.h>
#include <linux/soundwire/sdw_type.h>
+#include <linux/workqueue.h>
#include <sound/pcm.h>
#include <sound/pcm_params.h>
#include <sound/soc.h>
@@ -26,6 +27,8 @@
/* Register addresses are offset when sent over Soundwire */
#define CS42L42_SDW_ADDR_OFFSET 0x8000
+#define CS42L42_SDW_GEN_INT_STATUS_1 0xc0
+#define CS42L42_SDW_GEN_INT_MASK_1 0xc1
#define CS42L42_SDW_MEM_ACCESS_STATUS 0xd0
#define CS42L42_SDW_MEM_READ_DATA 0xd8
@@ -33,6 +36,11 @@
#define CS42L42_SDW_CMD_IN_PROGRESS BIT(2)
#define CS42L42_SDW_RDATA_RDY BIT(0)
+#define CS42L42_SDW_M_SCP_IMP_DEF1 BIT(0)
+#define CS42L42_GEN_INT_CASCADE SDW_SCP_INT1_IMPL_DEF
+
+#define CS42L42_SDW_INT_MASK_CODEC_IRQ BIT(0)
+
#define CS42L42_DELAYED_READ_POLL_US 1
#define CS42L42_DELAYED_READ_TIMEOUT_US 100
@@ -306,6 +314,13 @@ static void cs42l42_sdw_init(struct sdw_slave *peripheral)
/* Disable internal logic that makes clock-stop conditional */
regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3, CS42L42_SW_CLK_STP_STAT_SEL_MASK);
+ /* Enable Soundwire interrupts */
+ if (!cs42l42->irq) {
+ dev_dbg(cs42l42->dev, "Using Soundwire interrupts\n");
+ sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1,
+ CS42L42_SDW_INT_MASK_CODEC_IRQ);
+ }
+
/*
* pm_runtime is needed to control bus manager suspend, and to
* recover from an unattach_request when the manager suspends.
@@ -319,6 +334,49 @@ static void cs42l42_sdw_init(struct sdw_slave *peripheral)
pm_runtime_idle(cs42l42->dev);
}
+static int cs42l42_sdw_interrupt(struct sdw_slave *peripheral,
+ struct sdw_slave_intr_status *status)
+{
+ struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
+
+ /* Soundwire core holds our pm_runtime when calling this function. */
+
+ dev_dbg(cs42l42->dev, "int control_port=0x%x\n", status->control_port);
+
+ if ((status->control_port & CS42L42_GEN_INT_CASCADE) == 0)
+ return 0;
+
+ /*
+ * Clear and mask until it has been handled. The read of GEN_INT_STATUS_1
+ * is required as per the Soundwire spec for interrupt status bits to clear.
+ */
+ sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1, 0);
+ sdw_read_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1);
+ sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1, 0xFF);
+ queue_work(system_power_efficient_wq, &cs42l42->sdw_irq_work);
+
+ /* Prevent host controller suspending before we handle the interrupt */
+ pm_runtime_get_noresume(cs42l42->dev);
+
+ return 0;
+}
+
+static void cs42l42_sdw_irq_work(struct work_struct *work)
+{
+ struct cs42l42_private *cs42l42 = container_of(work,
+ struct cs42l42_private,
+ sdw_irq_work);
+
+ cs42l42_irq_thread(-1, cs42l42);
+
+ /* unmask interrupt */
+ if (!cs42l42->sdw_irq_no_unmask)
+ sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
+ CS42L42_SDW_INT_MASK_CODEC_IRQ);
+
+ pm_runtime_put_autosuspend(cs42l42->dev);
+}
+
static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
{
struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
@@ -334,6 +392,14 @@ static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
+ /*
+ * CS42L42 doesn't have a SDW_SCP_INT1_IMPL_DEF mask bit but it must be
+ * set in scp_int1_mask else the Soundwire framework won't notify us
+ * when the IMPL_DEF interrupt is asserted.
+ */
+ if (!cs42l42->irq)
+ prop->scp_int1_mask |= SDW_SCP_INT1_IMPL_DEF;
+
/* DP1 - capture */
ports[0].num = CS42L42_SDW_CAPTURE_PORT,
ports[0].type = SDW_DPN_FULL,
@@ -403,6 +469,7 @@ static int __maybe_unused cs42l42_sdw_clk_stop(struct sdw_slave *peripheral,
static const struct sdw_slave_ops cs42l42_sdw_ops = {
.read_prop = cs42l42_sdw_read_prop,
+ .interrupt_callback = cs42l42_sdw_interrupt,
.update_status = cs42l42_sdw_update_status,
.bus_config = cs42l42_sdw_bus_config,
#ifdef DEBUG
@@ -473,6 +540,11 @@ static int __maybe_unused cs42l42_sdw_runtime_resume(struct device *dev)
regcache_sync_region(cs42l42->regmap, CS42L42_MIC_DET_CTL1, CS42L42_MIC_DET_CTL1);
regcache_sync(cs42l42->regmap);
+ /* Re-enable Soundwire interrupts */
+ if (!cs42l42->irq)
+ sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
+ CS42L42_SDW_INT_MASK_CODEC_IRQ);
+
return 0;
}
@@ -495,6 +567,11 @@ static int __maybe_unused cs42l42_sdw_resume(struct device *dev)
cs42l42_resume_restore(dev);
+ /* Re-enable Soundwire interrupts */
+ if (!cs42l42->irq)
+ sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
+ CS42L42_SDW_INT_MASK_CODEC_IRQ);
+
return 0;
}
@@ -546,6 +623,7 @@ static int cs42l42_sdw_probe(struct sdw_slave *peripheral, const struct sdw_devi
component_drv->dapm_routes = cs42l42_sdw_audio_map;
component_drv->num_dapm_routes = ARRAY_SIZE(cs42l42_sdw_audio_map);
+ INIT_WORK(&cs42l42->sdw_irq_work, cs42l42_sdw_irq_work);
cs42l42->dev = dev;
cs42l42->regmap = regmap;
cs42l42->sdw_peripheral = peripheral;
@@ -562,8 +640,18 @@ static int cs42l42_sdw_remove(struct sdw_slave *peripheral)
{
struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
- /* Resume so that cs42l42_remove() can access registers */
+ /* Resume so that we can access registers */
pm_runtime_get_sync(cs42l42->dev);
+
+ /* Disable Soundwire interrupts */
+ if (!cs42l42->irq) {
+ cs42l42->sdw_irq_no_unmask = true;
+ cancel_work_sync(&cs42l42->sdw_irq_work);
+ sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1, 0);
+ sdw_read_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1);
+ sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1, 0xFF);
+ }
+
cs42l42_common_remove(cs42l42);
pm_runtime_put(cs42l42->dev);
pm_runtime_disable(cs42l42->dev);
diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
index 038db45d95b3..b29126d218c4 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -19,6 +19,7 @@
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/soundwire/sdw.h>
+#include <linux/workqueue.h>
#include <sound/jack.h>
#include <sound/cs42l42.h>
#include <sound/soc-component.h>
@@ -32,6 +33,7 @@ struct cs42l42_private {
struct completion pdn_done;
struct snd_soc_jack *jack;
struct sdw_slave *sdw_peripheral;
+ struct work_struct sdw_irq_work;
struct mutex irq_lock;
int irq;
int pll_config;
@@ -52,6 +54,7 @@ struct cs42l42_private {
bool hp_adc_up_pending;
bool suspended;
bool init_done;
+ bool sdw_irq_no_unmask;
};
extern const struct regmap_config cs42l42_regmap;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 12/12] ASoC: cs42l42: Add support for Soundwire interrupts
2022-08-19 12:52 ` [PATCH 12/12] ASoC: cs42l42: Add support for Soundwire interrupts Richard Fitzgerald
@ 2022-08-22 11:33 ` Pierre-Louis Bossart
2022-08-22 15:01 ` Richard Fitzgerald
0 siblings, 1 reply; 21+ messages in thread
From: Pierre-Louis Bossart @ 2022-08-22 11:33 UTC (permalink / raw)
To: Richard Fitzgerald, broonie; +Cc: patches, alsa-devel, linux-kernel
On 8/19/22 14:52, Richard Fitzgerald wrote:
> This adds support for using the Soundwire interrupt mechanism to
> handle CS42L42 chip interrupts.
>
> Soundwire interrupts are used if a hard INT line is not declared.
This sounds really weird.
The register access would still use the SoundWire read/writes, which
raises a number of opens since the interrupt cannot be handled until the
bus is resumed/operational, so there would be no speed-up at all. Unless
I completely missed something, this seems like wasting one pin with no
benefits?
> Wake-from-clock-stop is not used. The CS42L42 has limited wake
> capability, but clock-stop is already disabled when a snd_soc_jack is
> registered to prevent the host controller issuing a bus-reset on exit
> from clock stop mode, which would clear the interrupt status and break
> jack and button detection.
Same open as in previous patch on why this is needed.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> sound/soc/codecs/cs42l42-sdw.c | 90 +++++++++++++++++++++++++++++++++-
> sound/soc/codecs/cs42l42.h | 3 ++
> 2 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/codecs/cs42l42-sdw.c b/sound/soc/codecs/cs42l42-sdw.c
> index ed69a0a44d8c..1bdeed93587d 100644
> --- a/sound/soc/codecs/cs42l42-sdw.c
> +++ b/sound/soc/codecs/cs42l42-sdw.c
> @@ -14,6 +14,7 @@
> #include <linux/soundwire/sdw.h>
> #include <linux/soundwire/sdw_registers.h>
> #include <linux/soundwire/sdw_type.h>
> +#include <linux/workqueue.h>
> #include <sound/pcm.h>
> #include <sound/pcm_params.h>
> #include <sound/soc.h>
> @@ -26,6 +27,8 @@
> /* Register addresses are offset when sent over Soundwire */
> #define CS42L42_SDW_ADDR_OFFSET 0x8000
>
> +#define CS42L42_SDW_GEN_INT_STATUS_1 0xc0
> +#define CS42L42_SDW_GEN_INT_MASK_1 0xc1
> #define CS42L42_SDW_MEM_ACCESS_STATUS 0xd0
> #define CS42L42_SDW_MEM_READ_DATA 0xd8
>
> @@ -33,6 +36,11 @@
> #define CS42L42_SDW_CMD_IN_PROGRESS BIT(2)
> #define CS42L42_SDW_RDATA_RDY BIT(0)
>
> +#define CS42L42_SDW_M_SCP_IMP_DEF1 BIT(0)
> +#define CS42L42_GEN_INT_CASCADE SDW_SCP_INT1_IMPL_DEF
> +
> +#define CS42L42_SDW_INT_MASK_CODEC_IRQ BIT(0)
> +
> #define CS42L42_DELAYED_READ_POLL_US 1
> #define CS42L42_DELAYED_READ_TIMEOUT_US 100
>
> @@ -306,6 +314,13 @@ static void cs42l42_sdw_init(struct sdw_slave *peripheral)
> /* Disable internal logic that makes clock-stop conditional */
> regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3, CS42L42_SW_CLK_STP_STAT_SEL_MASK);
>
> + /* Enable Soundwire interrupts */
> + if (!cs42l42->irq) {
> + dev_dbg(cs42l42->dev, "Using Soundwire interrupts\n");
> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1,
> + CS42L42_SDW_INT_MASK_CODEC_IRQ);
> + }
> +
> /*
> * pm_runtime is needed to control bus manager suspend, and to
> * recover from an unattach_request when the manager suspends.
> @@ -319,6 +334,49 @@ static void cs42l42_sdw_init(struct sdw_slave *peripheral)
> pm_runtime_idle(cs42l42->dev);
> }
>
> +static int cs42l42_sdw_interrupt(struct sdw_slave *peripheral,
> + struct sdw_slave_intr_status *status)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> +
> + /* Soundwire core holds our pm_runtime when calling this function. */
> +
> + dev_dbg(cs42l42->dev, "int control_port=0x%x\n", status->control_port);
> +
> + if ((status->control_port & CS42L42_GEN_INT_CASCADE) == 0)
> + return 0;
> +
> + /*
> + * Clear and mask until it has been handled. The read of GEN_INT_STATUS_1
> + * is required as per the Soundwire spec for interrupt status bits to clear.
Humm, this explanation is not very clear. What part of the spec are you
referring to?
Section 11.1.2 "Interrupt Model" says that a read is necessary to make
sure a condition is not missed while clearing the status with successful
write. You need to write the status to clear, and re-read the status to
see if another condition remains. That's not how I understand the code
below, which does the write and read in the opposite order.
> + */
> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1, 0);
> + sdw_read_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1);
> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1, 0xFF);
> + queue_work(system_power_efficient_wq, &cs42l42->sdw_irq_work);
> +
> + /* Prevent host controller suspending before we handle the interrupt */
> + pm_runtime_get_noresume(cs42l42->dev);
> +
> + return 0;
> +}
> +
> +static void cs42l42_sdw_irq_work(struct work_struct *work)
> +{
> + struct cs42l42_private *cs42l42 = container_of(work,
> + struct cs42l42_private,
> + sdw_irq_work);
> +
> + cs42l42_irq_thread(-1, cs42l42);
> +
> + /* unmask interrupt */
> + if (!cs42l42->sdw_irq_no_unmask)
> + sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
> + CS42L42_SDW_INT_MASK_CODEC_IRQ);
> +
> + pm_runtime_put_autosuspend(cs42l42->dev);
> +}
> +
> static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
> {
> struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> @@ -334,6 +392,14 @@ static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
> prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
> prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
>
> + /*
> + * CS42L42 doesn't have a SDW_SCP_INT1_IMPL_DEF mask bit but it must be
> + * set in scp_int1_mask else the Soundwire framework won't notify us
> + * when the IMPL_DEF interrupt is asserted.
> + */
> + if (!cs42l42->irq)
> + prop->scp_int1_mask |= SDW_SCP_INT1_IMPL_DEF;
Sorry, I don't follow the explanation. If you don't have a bit defined
for a specific interrupt, how would that interrupt be handled?
> /* DP1 - capture */
> ports[0].num = CS42L42_SDW_CAPTURE_PORT,
> ports[0].type = SDW_DPN_FULL,
> @@ -403,6 +469,7 @@ static int __maybe_unused cs42l42_sdw_clk_stop(struct sdw_slave *peripheral,
>
> static const struct sdw_slave_ops cs42l42_sdw_ops = {
> .read_prop = cs42l42_sdw_read_prop,
> + .interrupt_callback = cs42l42_sdw_interrupt,
> .update_status = cs42l42_sdw_update_status,
> .bus_config = cs42l42_sdw_bus_config,
> #ifdef DEBUG
> @@ -473,6 +540,11 @@ static int __maybe_unused cs42l42_sdw_runtime_resume(struct device *dev)
> regcache_sync_region(cs42l42->regmap, CS42L42_MIC_DET_CTL1, CS42L42_MIC_DET_CTL1);
> regcache_sync(cs42l42->regmap);
>
> + /* Re-enable Soundwire interrupts */
> + if (!cs42l42->irq)
> + sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
> + CS42L42_SDW_INT_MASK_CODEC_IRQ);
> +
> return 0;
> }
>
> @@ -495,6 +567,11 @@ static int __maybe_unused cs42l42_sdw_resume(struct device *dev)
>
> cs42l42_resume_restore(dev);
>
> + /* Re-enable Soundwire interrupts */
> + if (!cs42l42->irq)
> + sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
> + CS42L42_SDW_INT_MASK_CODEC_IRQ);
> +
that would prevent the device from waking up the system while in
suspend? How would the resume be triggered then, only by the manager?
That doesn't seem like a working model for a headset codec.
This seems also weird since I don't see where the interrupts are
disabled on suspend, so this 're-enable' does not have a clear 'disable'
dual operation.
> return 0;
> }
>
> @@ -546,6 +623,7 @@ static int cs42l42_sdw_probe(struct sdw_slave *peripheral, const struct sdw_devi
> component_drv->dapm_routes = cs42l42_sdw_audio_map;
> component_drv->num_dapm_routes = ARRAY_SIZE(cs42l42_sdw_audio_map);
>
> + INIT_WORK(&cs42l42->sdw_irq_work, cs42l42_sdw_irq_work);
> cs42l42->dev = dev;
> cs42l42->regmap = regmap;
> cs42l42->sdw_peripheral = peripheral;
> @@ -562,8 +640,18 @@ static int cs42l42_sdw_remove(struct sdw_slave *peripheral)
> {
> struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>
> - /* Resume so that cs42l42_remove() can access registers */
> + /* Resume so that we can access registers */
> pm_runtime_get_sync(cs42l42->dev);
> +
> + /* Disable Soundwire interrupts */
> + if (!cs42l42->irq) {
> + cs42l42->sdw_irq_no_unmask = true;
> + cancel_work_sync(&cs42l42->sdw_irq_work);
> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1, 0);
> + sdw_read_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1);
> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1, 0xFF);
> + }
> +
> cs42l42_common_remove(cs42l42);
> pm_runtime_put(cs42l42->dev);
> pm_runtime_disable(cs42l42->dev);
> diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
> index 038db45d95b3..b29126d218c4 100644
> --- a/sound/soc/codecs/cs42l42.h
> +++ b/sound/soc/codecs/cs42l42.h
> @@ -19,6 +19,7 @@
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/soundwire/sdw.h>
> +#include <linux/workqueue.h>
> #include <sound/jack.h>
> #include <sound/cs42l42.h>
> #include <sound/soc-component.h>
> @@ -32,6 +33,7 @@ struct cs42l42_private {
> struct completion pdn_done;
> struct snd_soc_jack *jack;
> struct sdw_slave *sdw_peripheral;
> + struct work_struct sdw_irq_work;
> struct mutex irq_lock;
> int irq;
> int pll_config;
> @@ -52,6 +54,7 @@ struct cs42l42_private {
> bool hp_adc_up_pending;
> bool suspended;
> bool init_done;
> + bool sdw_irq_no_unmask;
> };
>
> extern const struct regmap_config cs42l42_regmap;
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 12/12] ASoC: cs42l42: Add support for Soundwire interrupts
2022-08-22 11:33 ` Pierre-Louis Bossart
@ 2022-08-22 15:01 ` Richard Fitzgerald
0 siblings, 0 replies; 21+ messages in thread
From: Richard Fitzgerald @ 2022-08-22 15:01 UTC (permalink / raw)
To: Pierre-Louis Bossart, broonie; +Cc: patches, alsa-devel, linux-kernel
On 22/08/2022 12:33, Pierre-Louis Bossart wrote:
>
>
> On 8/19/22 14:52, Richard Fitzgerald wrote:
>> This adds support for using the Soundwire interrupt mechanism to
>> handle CS42L42 chip interrupts.
>>
>> Soundwire interrupts are used if a hard INT line is not declared.
>
> This sounds really weird.
>
> The register access would still use the SoundWire read/writes, which
> raises a number of opens since the interrupt cannot be handled until the
> bus is resumed/operational, so there would be no speed-up at all. Unless
I didn't say it was for speed.
The cs42l42 has various quirks. One is that the interrupt controller can
still assert hard INT while in Soundwire clock-stop but cannot trigger a
Soundwire wake. We could remove INT support from this first submission
if you prefer and add it when/if necessary.
> I completely missed something, this seems like wasting one pin with no
> benefits?
>
The pin has no other function than INT so it's there whether you use it
or not.
>> Wake-from-clock-stop is not used. The CS42L42 has limited wake
>> capability, but clock-stop is already disabled when a snd_soc_jack is
>> registered to prevent the host controller issuing a bus-reset on exit
>> from clock stop mode, which would clear the interrupt status and break
>> jack and button detection.
>
> Same open as in previous patch on why this is needed.
>
As said, the CS42L42 has limited ability to issue a wake from clock
stop and even with its limited capability things won't work well if
it gets bus-reset before the source can be handled.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>> sound/soc/codecs/cs42l42-sdw.c | 90 +++++++++++++++++++++++++++++++++-
>> sound/soc/codecs/cs42l42.h | 3 ++
>> 2 files changed, 92 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/codecs/cs42l42-sdw.c b/sound/soc/codecs/cs42l42-sdw.c
>> index ed69a0a44d8c..1bdeed93587d 100644
>> --- a/sound/soc/codecs/cs42l42-sdw.c
>> +++ b/sound/soc/codecs/cs42l42-sdw.c
>> @@ -14,6 +14,7 @@
>> #include <linux/soundwire/sdw.h>
>> #include <linux/soundwire/sdw_registers.h>
>> #include <linux/soundwire/sdw_type.h>
>> +#include <linux/workqueue.h>
>> #include <sound/pcm.h>
>> #include <sound/pcm_params.h>
>> #include <sound/soc.h>
>> @@ -26,6 +27,8 @@
>> /* Register addresses are offset when sent over Soundwire */
>> #define CS42L42_SDW_ADDR_OFFSET 0x8000
>>
>> +#define CS42L42_SDW_GEN_INT_STATUS_1 0xc0
>> +#define CS42L42_SDW_GEN_INT_MASK_1 0xc1
>> #define CS42L42_SDW_MEM_ACCESS_STATUS 0xd0
>> #define CS42L42_SDW_MEM_READ_DATA 0xd8
>>
>> @@ -33,6 +36,11 @@
>> #define CS42L42_SDW_CMD_IN_PROGRESS BIT(2)
>> #define CS42L42_SDW_RDATA_RDY BIT(0)
>>
>> +#define CS42L42_SDW_M_SCP_IMP_DEF1 BIT(0)
>> +#define CS42L42_GEN_INT_CASCADE SDW_SCP_INT1_IMPL_DEF
>> +
>> +#define CS42L42_SDW_INT_MASK_CODEC_IRQ BIT(0)
>> +
>> #define CS42L42_DELAYED_READ_POLL_US 1
>> #define CS42L42_DELAYED_READ_TIMEOUT_US 100
>>
>> @@ -306,6 +314,13 @@ static void cs42l42_sdw_init(struct sdw_slave *peripheral)
>> /* Disable internal logic that makes clock-stop conditional */
>> regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3, CS42L42_SW_CLK_STP_STAT_SEL_MASK);
>>
>> + /* Enable Soundwire interrupts */
>> + if (!cs42l42->irq) {
>> + dev_dbg(cs42l42->dev, "Using Soundwire interrupts\n");
>> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1,
>> + CS42L42_SDW_INT_MASK_CODEC_IRQ);
>> + }
>> +
>> /*
>> * pm_runtime is needed to control bus manager suspend, and to
>> * recover from an unattach_request when the manager suspends.
>> @@ -319,6 +334,49 @@ static void cs42l42_sdw_init(struct sdw_slave *peripheral)
>> pm_runtime_idle(cs42l42->dev);
>> }
>>
>> +static int cs42l42_sdw_interrupt(struct sdw_slave *peripheral,
>> + struct sdw_slave_intr_status *status)
>> +{
>> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>> +
>> + /* Soundwire core holds our pm_runtime when calling this function. */
>> +
>> + dev_dbg(cs42l42->dev, "int control_port=0x%x\n", status->control_port);
>> +
>> + if ((status->control_port & CS42L42_GEN_INT_CASCADE) == 0)
>> + return 0;
>> +
>> + /*
>> + * Clear and mask until it has been handled. The read of GEN_INT_STATUS_1
>> + * is required as per the Soundwire spec for interrupt status bits to clear.
>
> Humm, this explanation is not very clear. What part of the spec are you
> referring to?
>
> Section 11.1.2 "Interrupt Model" says that a read is necessary to make
> sure a condition is not missed while clearing the status with successful
> write. You need to write the status to clear, and re-read the status to
> see if another condition remains. That's not how I understand the code
> below, which does the write and read in the opposite order.
>
The spec is
1. The status must be read before it can be cleared
2. It must then be written to clear it
3. Any new events triggered since the read will not be cleared by
the write since there hasn't been another read, and will remain
pending.
Thus the sequence is
1. Read INT_STATUS to ack the interrupts we've seen and enable clearing
them.
2. Write INT_STATUS to clear the interrupts acked by the previous read.
3. New interrupts since the read of INT_STATUS will not clear so we
don't lose them. However since we do a pre-step of masking the input
sources to GEN_INT_STATUS_1 no new interrupt can actually be triggered,
in this register - this prevents an ALERT storm while waiting for the
work function to run and clear the interrupt sources.
(BTW these are custom registers, not SW spec registers, so their
behavior is implementation-defined, although GEN_INT_STATUS_1 does
implement the normal SW read-write-to-clear behavior.)
>> + */
>> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1, 0);
>> + sdw_read_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1);
>> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1, 0xFF);
>> + queue_work(system_power_efficient_wq, &cs42l42->sdw_irq_work);
>> +
>> + /* Prevent host controller suspending before we handle the interrupt */
>> + pm_runtime_get_noresume(cs42l42->dev);
>> +
>> + return 0;
>> +}
>> +
>> +static void cs42l42_sdw_irq_work(struct work_struct *work)
>> +{
>> + struct cs42l42_private *cs42l42 = container_of(work,
>> + struct cs42l42_private,
>> + sdw_irq_work);
>> +
>> + cs42l42_irq_thread(-1, cs42l42);
>> +
>> + /* unmask interrupt */
>> + if (!cs42l42->sdw_irq_no_unmask)
>> + sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
>> + CS42L42_SDW_INT_MASK_CODEC_IRQ);
>> +
>> + pm_runtime_put_autosuspend(cs42l42->dev);
>> +}
>> +
>> static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
>> {
>> struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>> @@ -334,6 +392,14 @@ static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
>> prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
>> prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
>>
>> + /*
>> + * CS42L42 doesn't have a SDW_SCP_INT1_IMPL_DEF mask bit but it must be
>> + * set in scp_int1_mask else the Soundwire framework won't notify us
>> + * when the IMPL_DEF interrupt is asserted.
>> + */
>> + if (!cs42l42->irq)
>> + prop->scp_int1_mask |= SDW_SCP_INT1_IMPL_DEF;
>
> Sorry, I don't follow the explanation. If you don't have a bit defined
> for a specific interrupt, how would that interrupt be handled?
The IMP_DEF interrupt is always unmasked. It does not have a mask bit.
>
>> /* DP1 - capture */
>> ports[0].num = CS42L42_SDW_CAPTURE_PORT,
>> ports[0].type = SDW_DPN_FULL,
>> @@ -403,6 +469,7 @@ static int __maybe_unused cs42l42_sdw_clk_stop(struct sdw_slave *peripheral,
>>
>> static const struct sdw_slave_ops cs42l42_sdw_ops = {
>> .read_prop = cs42l42_sdw_read_prop,
>> + .interrupt_callback = cs42l42_sdw_interrupt,
>> .update_status = cs42l42_sdw_update_status,
>> .bus_config = cs42l42_sdw_bus_config,
>> #ifdef DEBUG
>> @@ -473,6 +540,11 @@ static int __maybe_unused cs42l42_sdw_runtime_resume(struct device *dev)
>> regcache_sync_region(cs42l42->regmap, CS42L42_MIC_DET_CTL1, CS42L42_MIC_DET_CTL1);
>> regcache_sync(cs42l42->regmap);
>>
>> + /* Re-enable Soundwire interrupts */
>> + if (!cs42l42->irq)
>> + sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
>> + CS42L42_SDW_INT_MASK_CODEC_IRQ);
>> +
>> return 0;
>> }
>>
>> @@ -495,6 +567,11 @@ static int __maybe_unused cs42l42_sdw_resume(struct device *dev)
>>
>> cs42l42_resume_restore(dev);
>>
>> + /* Re-enable Soundwire interrupts */
>> + if (!cs42l42->irq)
>> + sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
>> + CS42L42_SDW_INT_MASK_CODEC_IRQ);
>> +
>
> that would prevent the device from waking up the system while in
> suspend? How would the resume be triggered then, only by the manager?
> That doesn't seem like a working model for a headset codec.
>
I don't understand your concern here. When you bus-reset our peripheral
on exit from clock-stop it will reset the custom implementation-defined
interrupt masks. The core code won't restore them because it doesn't
know about them. So we have to restore them otherwise we will never get
any more jack or button ALERTs from the CS42L42. This is no different
from doing a regcache_sync() to restore registers.
> This seems also weird since I don't see where the interrupts are
> disabled on suspend, so this 're-enable' does not have a clear 'disable'
> dual operation.
>
We don't want to disable them. We need to recover from an Intel forced
bus-reset. It would be simpler if we didn't have to consider a mode 0
clock stop possibly resetting the peripheral, but as Intel does that we
must handle everything the core code can't restore automatically.
>> return 0;
>> }
>>
>> @@ -546,6 +623,7 @@ static int cs42l42_sdw_probe(struct sdw_slave *peripheral, const struct sdw_devi
>> component_drv->dapm_routes = cs42l42_sdw_audio_map;
>> component_drv->num_dapm_routes = ARRAY_SIZE(cs42l42_sdw_audio_map);
>>
>> + INIT_WORK(&cs42l42->sdw_irq_work, cs42l42_sdw_irq_work);
>> cs42l42->dev = dev;
>> cs42l42->regmap = regmap;
>> cs42l42->sdw_peripheral = peripheral;
>> @@ -562,8 +640,18 @@ static int cs42l42_sdw_remove(struct sdw_slave *peripheral)
>> {
>> struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>>
>> - /* Resume so that cs42l42_remove() can access registers */
>> + /* Resume so that we can access registers */
>> pm_runtime_get_sync(cs42l42->dev);
>> +
>> + /* Disable Soundwire interrupts */
>> + if (!cs42l42->irq) {
>> + cs42l42->sdw_irq_no_unmask = true;
>> + cancel_work_sync(&cs42l42->sdw_irq_work);
>> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1, 0);
>> + sdw_read_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1);
>> + sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1, 0xFF);
>> + }
>> +
>> cs42l42_common_remove(cs42l42);
>> pm_runtime_put(cs42l42->dev);
>> pm_runtime_disable(cs42l42->dev);
>> diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
>> index 038db45d95b3..b29126d218c4 100644
>> --- a/sound/soc/codecs/cs42l42.h
>> +++ b/sound/soc/codecs/cs42l42.h
>> @@ -19,6 +19,7 @@
>> #include <linux/regmap.h>
>> #include <linux/regulator/consumer.h>
>> #include <linux/soundwire/sdw.h>
>> +#include <linux/workqueue.h>
>> #include <sound/jack.h>
>> #include <sound/cs42l42.h>
>> #include <sound/soc-component.h>
>> @@ -32,6 +33,7 @@ struct cs42l42_private {
>> struct completion pdn_done;
>> struct snd_soc_jack *jack;
>> struct sdw_slave *sdw_peripheral;
>> + struct work_struct sdw_irq_work;
>> struct mutex irq_lock;
>> int irq;
>> int pll_config;
>> @@ -52,6 +54,7 @@ struct cs42l42_private {
>> bool hp_adc_up_pending;
>> bool suspended;
>> bool init_done;
>> + bool sdw_irq_no_unmask;
>> };
>>
>> extern const struct regmap_config cs42l42_regmap;
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 00/12] ASoC: cs42l42: Add Soundwire support
2022-08-19 12:52 [PATCH 00/12] ASoC: cs42l42: Add Soundwire support Richard Fitzgerald
` (11 preceding siblings ...)
2022-08-19 12:52 ` [PATCH 12/12] ASoC: cs42l42: Add support for Soundwire interrupts Richard Fitzgerald
@ 2022-09-23 16:54 ` Mark Brown
12 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2022-09-23 16:54 UTC (permalink / raw)
To: Richard Fitzgerald; +Cc: linux-kernel, patches, alsa-devel
On Fri, 19 Aug 2022 13:52:18 +0100, Richard Fitzgerald wrote:
> The CS42L42 has a Soundwire interface for control and audio. This
> chain of patches adds support for this.
>
> Patches #1 .. #10 split out various changes to the existing code that
> are needed for adding Soundwire. These are mostly around clocking and
> supporting the separate probe and enumeration stages in Soundwire.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[01/12] ASoC: cs42l42: Add SOFT_RESET_REBOOT register
(no commit info)
[02/12] ASoC: cs42l42: Add bitclock frequency argument to cs42l42_pll_config()
commit: 7e178946c3e4e64cebda4e60d0b7e5c02a502d13
[03/12] ASoC: cs42l42: Ensure MCLKint is a multiple of the sample rate
(no commit info)
[04/12] ASoC: cs42l42: Separate ASP config from PLL config
(no commit info)
[05/12] ASoC: cs42l42: Use cs42l42->dev instead of &i2c_client->dev
commit: 2feab7e7d8c01b67d9ffbfb902d1591c08e9d564
[06/12] ASoC: cs42l42: Split probe() and remove() into stages
commit: 0285042feda799edca63b35cea0cda32ed0c47c2
[07/12] ASoC: cs42l42: Split cs42l42_resume into two functions
commit: 56746683c2560ba5604bb212f73eb01f5edfd312
[08/12] ASoC: cs42l42: Pass component and dai defs into common probe
commit: 52c2e370df07092437d1515e773d28a5f53fc810
[09/12] ASoC: cs42l42: Split I2C identity into separate module
commit: ae9f5e607da47104bc3d02e5c0ed237749f5db51
[10/12] ASoC: cs42l42: Export some functions for Soundwire
(no commit info)
[11/12] ASoC: cs42l42: Add Soundwire support
(no commit info)
[12/12] ASoC: cs42l42: Add support for Soundwire interrupts
(no commit info)
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
^ permalink raw reply [flat|nested] 21+ messages in thread