[PATCH v2 0/8] ASoC: cs42l42: Add Soundwire support

[PATCH v2 0/8] ASoC: cs42l42: Add Soundwire support

[Stefan Binding]

The CS42L42 has a Soundwire interface for control and audio. This
chain of patches adds support for this.

Patches #1 .. #5 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.

Patches #6 .. #8 actually adds the Soundwire handling.

Changes since v1:
- fixes for various review comments from v1
- add support for wakeup from clock stop using hardware interrupts
- use port_prep callback to prepare/deprepare codec

Richard Fitzgerald (6):
  ASoC: cs42l42: Add SOFT_RESET_REBOOT register
  ASoC: cs42l42: Ensure MCLKint is a multiple of the sample rate
  ASoC: cs42l42: Separate ASP config from PLL config
  ASoC: cs42l42: Export some functions for Soundwire
  ASoC: cs42l42: Add Soundwire support
  ASoC: cs42l42: Don't set idle_bias_on

Stefan Binding (2):
  soundwire: stream: Add specific prep/deprep commands to port_prep
    callback
  ASoC: cs42l42: Wait for debounce interval after resume

 drivers/soundwire/stream.c     |   4 +-
 include/linux/soundwire/sdw.h  |   8 +-
 include/sound/cs42l42.h        |   5 +
 sound/soc/codecs/Kconfig       |   8 +
 sound/soc/codecs/Makefile      |   2 +
 sound/soc/codecs/cs42l42-sdw.c | 603 +++++++++++++++++++++++++++++++++
 sound/soc/codecs/cs42l42.c     | 127 ++++---
 sound/soc/codecs/cs42l42.h     |   9 +-
 8 files changed, 716 insertions(+), 50 deletions(-)
 create mode 100644 sound/soc/codecs/cs42l42-sdw.c

-- 
2.34.1

[PATCH v2 1/8] soundwire: stream: Add specific prep/deprep commands to port_prep callback

[Stefan Binding]

Currently, port_prep callback only has commands for PRE_PREP, PREP,
and POST_PREP, which doesn't directly say whether this is for a
prepare or deprepare call. Extend the command list enum to say
whether the call is for prepare or deprepare aswell.

Also remove SDW_OPS_PORT_PREP from sdw_port_prep_ops as this is unused,
and update this enum to be simpler and more consistent with enum
sdw_clk_stop_type.

Note: Currently, the only users of SDW_OPS_PORT_POST_PREP are codec
drivers sound/soc/codecs/wsa881x.c and sound/soc/codecs/wsa883x.c, both
of which seem to assume that POST_PREP only occurs after a prepare,
even though it would also have occurred after a deprepare. Since it
doesn't make sense to mark the port prepared after a deprepare, changing
the enum to separate PORT_DEPREP from PORT_PREP should make the check
for PORT_PREP in those drivers be more logical.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 drivers/soundwire/stream.c    | 4 ++--
 include/linux/soundwire/sdw.h | 8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index df3b36670df4c..1652fb5737d9d 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -469,7 +469,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
 	}
 
 	/* Inform slave about the impending port prepare */
-	sdw_do_port_prep(s_rt, prep_ch, SDW_OPS_PORT_PRE_PREP);
+	sdw_do_port_prep(s_rt, prep_ch, prep ? SDW_OPS_PORT_PRE_PREP : SDW_OPS_PORT_PRE_DEPREP);
 
 	/* Prepare Slave port implementing CP_SM */
 	if (!dpn_prop->simple_ch_prep_sm) {
@@ -501,7 +501,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
 	}
 
 	/* Inform slaves about ports prepared */
-	sdw_do_port_prep(s_rt, prep_ch, SDW_OPS_PORT_POST_PREP);
+	sdw_do_port_prep(s_rt, prep_ch, prep ? SDW_OPS_PORT_POST_PREP : SDW_OPS_PORT_POST_DEPREP);
 
 	/* Disable interrupt after Port de-prepare */
 	if (!prep && intr)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 3cd2a761911ff..547fc1b30a51a 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -569,13 +569,15 @@ struct sdw_prepare_ch {
  * enum sdw_port_prep_ops: Prepare operations for Data Port
  *
  * @SDW_OPS_PORT_PRE_PREP: Pre prepare operation for the Port
- * @SDW_OPS_PORT_PREP: Prepare operation for the Port
+ * @SDW_OPS_PORT_PRE_DEPREP: Pre deprepare operation for the Port
  * @SDW_OPS_PORT_POST_PREP: Post prepare operation for the Port
+ * @SDW_OPS_PORT_POST_DEPREP: Post deprepare operation for the Port
  */
 enum sdw_port_prep_ops {
 	SDW_OPS_PORT_PRE_PREP = 0,
-	SDW_OPS_PORT_PREP = 1,
-	SDW_OPS_PORT_POST_PREP = 2,
+	SDW_OPS_PORT_PRE_DEPREP,
+	SDW_OPS_PORT_POST_PREP,
+	SDW_OPS_PORT_POST_DEPREP,
 };
 
 /**
-- 
2.34.1

[PATCH v2 2/8] ASoC: cs42l42: Add SOFT_RESET_REBOOT register

[Stefan Binding]

From: Richard Fitzgerald <rf@opensource.cirrus.com>

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>
Signed-off-by: Stefan Binding <sbinding@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 1d1c24fdd0cae..3994e933db195 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
@@ -720,6 +721,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 2fefbcf7bd130..82aa11d6937be 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -293,6 +293,7 @@ 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 @@ 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.34.1

[PATCH v2 3/8] ASoC: cs42l42: Ensure MCLKint is a multiple of the sample rate

[Stefan Binding]

From: Richard Fitzgerald <rf@opensource.cirrus.com>

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>
Signed-off-by: Stefan Binding <sbinding@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 82aa11d6937be..939f8bcc222c0 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -653,7 +653,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;
@@ -668,6 +669,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;
 
@@ -893,6 +898,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;
@@ -956,11 +962,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.34.1

[PATCH v2 4/8] ASoC: cs42l42: Separate ASP config from PLL config

[Stefan Binding]

From: Richard Fitzgerald <rf@opensource.cirrus.com>

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>
Signed-off-by: Stefan Binding <sbinding@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 939f8bcc222c0..d81c6eb1c1e59 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -658,7 +658,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) {
@@ -684,40 +683,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,
@@ -809,6 +774,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;
@@ -904,8 +909,6 @@ static int cs42l42_pcm_hw_params(struct snd_pcm_substream *substream,
 	unsigned int bclk;
 	int ret;
 
-	cs42l42->srate = params_rate(params);
-
 	if (cs42l42->bclk_ratio) {
 		/* machine driver has set the BCLK/samp-rate ratio */
 		bclk = cs42l42->bclk_ratio * params_rate(params);
@@ -966,6 +969,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 a721366641127..17aab06adc8e6 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -36,7 +36,6 @@ struct  cs42l42_private {
 	int pll_config;
 	u32 sclk;
 	u32 bclk_ratio;
-	u32 srate;
 	u8 plug_state;
 	u8 hs_type;
 	u8 ts_inv;
-- 
2.34.1

[PATCH v2 5/8] ASoC: cs42l42: Export some functions for Soundwire

[Stefan Binding]

From: Richard Fitzgerald <rf@opensource.cirrus.com>

Export functions that will be needed by a Soundwire module.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: Stefan Binding <sbinding@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 d81c6eb1c1e59..cefefd7061689 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -653,8 +653,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;
@@ -740,8 +740,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;
@@ -773,6 +774,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)
@@ -1013,7 +1015,7 @@ static int cs42l42_set_bclk_ratio(struct snd_soc_dai *dai,
 	return 0;
 }
 
-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);
@@ -1106,6 +1108,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 |\
@@ -1648,7 +1651,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];
@@ -1765,6 +1768,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 17aab06adc8e6..ef8219f489100 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -61,6 +61,11 @@ extern struct snd_soc_dai_driver cs42l42_dai;
 bool cs42l42_readable_register(struct device *dev, unsigned int reg);
 bool cs42l42_volatile_register(struct device *dev, unsigned int reg);
 
+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.34.1

[PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Stefan Binding]

From: Richard Fitzgerald <rf@opensource.cirrus.com>

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/disable are done during
  the port_prep callback.

- 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 means that the
  interrupt registers will be reset in between the interrupt being
  generated and the interrupt being handled, and since the interrupt
  status is debounced, these values may not be accurrate immediately,
  and may cause spurious unplug events before settling.

- 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>
Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/soc/codecs/Kconfig       |   8 +
 sound/soc/codecs/Makefile      |   2 +
 sound/soc/codecs/cs42l42-sdw.c | 595 +++++++++++++++++++++++++++++++++
 sound/soc/codecs/cs42l42.c     |  21 ++
 sound/soc/codecs/cs42l42.h     |   3 +
 5 files changed, 629 insertions(+)
 create mode 100644 sound/soc/codecs/cs42l42-sdw.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 6b4ee14640abc..1e5558f0c7b2a 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
@@ -703,6 +704,13 @@ 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
+	help
+	  Enable support for Cirrus Logic CS42L42 codec with Soundwire control
+
 config SND_SOC_CS42L51
 	tristate
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 71d3ce5867e4f..31c8921028cce 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
@@ -427,6 +428,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 0000000000000..67800b275e422
--- /dev/null
+++ b/sound/soc/codecs/cs42l42-sdw.c
@@ -0,0 +1,595 @@
+// 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/sdw.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 stream_config = {0};
+	struct sdw_port_config port_config = {0};
+	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);
+
+	snd_sdw_params_to_config(substream, params, &stream_config, &port_config);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		port_config.num = CS42L42_SDW_PLAYBACK_PORT;
+	else
+		port_config.num = CS42L42_SDW_CAPTURE_PORT;
+
+	ret = sdw_stream_add_slave(cs42l42->sdw_peripheral, &stream_config, &port_config, 1,
+				   sdw_stream);
+	if (ret) {
+		dev_err(dai->dev, "Failed to add sdw stream: %d\n", ret);
+		return ret;
+	}
+
+	cs42l42_src_config(dai->component, params_rate(params));
+
+	return 0;
+}
+
+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);
+
+	sdw_stream_remove_slave(cs42l42->sdw_peripheral, sdw_stream);
+	cs42l42->sample_rate = 0;
+
+	return 0;
+}
+
+static int cs42l42_sdw_port_prep(struct sdw_slave *slave,
+				 struct sdw_prepare_ch *prepare_ch,
+				 enum sdw_port_prep_ops state)
+{
+	struct cs42l42_private *cs42l42 = dev_get_drvdata(&slave->dev);
+	unsigned int pdn_mask;
+
+	if (prepare_ch->num == CS42L42_SDW_PLAYBACK_PORT)
+		pdn_mask = CS42L42_HP_PDN_MASK;
+	else
+		pdn_mask = CS42L42_ADC_PDN_MASK;
+
+	if (state == SDW_OPS_PORT_PRE_PREP) {
+		dev_dbg(cs42l42->dev, "Prep Port pdn_mask:%x\n", pdn_mask);
+		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);
+	} else if (state == SDW_OPS_PORT_POST_DEPREP) {
+		dev_dbg(cs42l42->dev, "Deprep Port pdn_mask:%x\n", 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,
+	.port_prep = cs42l42_sdw_port_prep,
+#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 snd_soc_component_driver *component_drv;
+	struct device *dev = &peripheral->dev;
+	struct cs42l42_private *cs42l42;
+	struct regmap_config *regmap_conf;
+	struct regmap *regmap;
+	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;
+	cs42l42->devid = CS42L42_CHIP_ID;
+
+	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 cefefd7061689..a92499876ce2a 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>
@@ -525,6 +526,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[] = {
@@ -1660,9 +1665,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;
 	}
 
@@ -1765,6 +1772,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;
 }
@@ -2388,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 ef8219f489100..4bd7b85a57471 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,11 +31,13 @@ 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 devid;
 	int irq;
 	int pll_config;
 	u32 sclk;
+	u32 sample_rate;
 	u32 bclk_ratio;
 	u8 plug_state;
 	u8 hs_type;
-- 
2.34.1

[PATCH v2 7/8] ASoC: cs42l42: Don't set idle_bias_on

[Stefan Binding]

From: Richard Fitzgerald <rf@opensource.cirrus.com>

idle_bias_on was set because cs42l42 has a "VMID" type pseudo-midrail
supply (named FILT+), and these typically take a long time to charge.
But the driver never enabled pm_runtime so it would never have powered-
down the cs42l42 anyway.

In fact, FILT+ can charge to operating voltage within 12.5 milliseconds
of enabling HP or ADC. This time is already covered by the startup
delay of the HP/ADC.

The datasheet warning about FILT+ taking up to 1 second to charge only
applies in the special cases that either the PLL is started or
DETECT_MODE set to non-zero while both HP and ADC are off. The driver
never does either of these.

Removing idle_bias_on allows the Soundwire host controller to suspend
if there isn't a snd_soc_jack handler registered.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/soc/codecs/cs42l42.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index a92499876ce2a..aa2223bfb885a 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -597,7 +597,6 @@ const struct snd_soc_component_driver cs42l42_soc_component = {
 	.num_dapm_routes	= ARRAY_SIZE(cs42l42_audio_map),
 	.controls		= cs42l42_snd_controls,
 	.num_controls		= ARRAY_SIZE(cs42l42_snd_controls),
-	.idle_bias_on		= 1,
 	.endianness		= 1,
 };
 EXPORT_SYMBOL_NS_GPL(cs42l42_soc_component, SND_SOC_CS42L42_CORE);
-- 
2.34.1

[PATCH v2 8/8] ASoC: cs42l42: Wait for debounce interval after resume

[Stefan Binding]

Since clock stop causes bus reset on Intel controllers, we need
to wait for the debounce interval on resume, to ensure all the
interrupt status registers are set correctly.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/soc/codecs/cs42l42-sdw.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/cs42l42-sdw.c b/sound/soc/codecs/cs42l42-sdw.c
index 67800b275e422..27653ea0f947c 100644
--- a/sound/soc/codecs/cs42l42-sdw.c
+++ b/sound/soc/codecs/cs42l42-sdw.c
@@ -451,14 +451,22 @@ static int __maybe_unused cs42l42_sdw_handle_unattach(struct cs42l42_private *cs
 
 static int __maybe_unused cs42l42_sdw_runtime_resume(struct device *dev)
 {
+	static const unsigned int ts_dbnce_ms[] = { 0, 125, 250, 500, 750, 1000, 1250, 1500};
 	struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
+	unsigned int dbnce;
 	int ret;
 
 	dev_dbg(dev, "Runtime resume\n");
 
 	ret = cs42l42_sdw_handle_unattach(cs42l42);
-	if (ret < 0)
+	if (ret < 0) {
 		return ret;
+	} else if (ret > 0) {
+		dbnce = max(cs42l42->ts_dbnc_rise, cs42l42->ts_dbnc_fall);
+
+		if (dbnce > 0)
+			msleep(ts_dbnce_ms[dbnce]);
+	}
 
 	regcache_cache_only(cs42l42->regmap, false);
 
-- 
2.34.1

Re: [PATCH v2 1/8] soundwire: stream: Add specific prep/deprep commands to port_prep callback

[Pierre-Louis Bossart]

On 1/18/23 10:04, Stefan Binding wrote:
> Currently, port_prep callback only has commands for PRE_PREP, PREP,
> and POST_PREP, which doesn't directly say whether this is for a
> prepare or deprepare call. Extend the command list enum to say
> whether the call is for prepare or deprepare aswell.
> 
> Also remove SDW_OPS_PORT_PREP from sdw_port_prep_ops as this is unused,
> and update this enum to be simpler and more consistent with enum
> sdw_clk_stop_type.

yes, I don't know why this PORT_PREP was added, clearly the prepare part
is something that would be done with standard registers without the need
to inform the codec driver. The codec driver only need the pre- and
post- notifications.

Good cleanup!


> Note: Currently, the only users of SDW_OPS_PORT_POST_PREP are codec
> drivers sound/soc/codecs/wsa881x.c and sound/soc/codecs/wsa883x.c, both
> of which seem to assume that POST_PREP only occurs after a prepare,
> even though it would also have occurred after a deprepare. Since it
> doesn't make sense to mark the port prepared after a deprepare, changing
> the enum to separate PORT_DEPREP from PORT_PREP should make the check
> for PORT_PREP in those drivers be more logical.
> 
> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

> ---
>  drivers/soundwire/stream.c    | 4 ++--
>  include/linux/soundwire/sdw.h | 8 +++++---
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index df3b36670df4c..1652fb5737d9d 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -469,7 +469,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
>  	}
>  
>  	/* Inform slave about the impending port prepare */
> -	sdw_do_port_prep(s_rt, prep_ch, SDW_OPS_PORT_PRE_PREP);
> +	sdw_do_port_prep(s_rt, prep_ch, prep ? SDW_OPS_PORT_PRE_PREP : SDW_OPS_PORT_PRE_DEPREP);
>  
>  	/* Prepare Slave port implementing CP_SM */
>  	if (!dpn_prop->simple_ch_prep_sm) {
> @@ -501,7 +501,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
>  	}
>  
>  	/* Inform slaves about ports prepared */
> -	sdw_do_port_prep(s_rt, prep_ch, SDW_OPS_PORT_POST_PREP);
> +	sdw_do_port_prep(s_rt, prep_ch, prep ? SDW_OPS_PORT_POST_PREP : SDW_OPS_PORT_POST_DEPREP);
>  
>  	/* Disable interrupt after Port de-prepare */
>  	if (!prep && intr)
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 3cd2a761911ff..547fc1b30a51a 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -569,13 +569,15 @@ struct sdw_prepare_ch {
>   * enum sdw_port_prep_ops: Prepare operations for Data Port
>   *
>   * @SDW_OPS_PORT_PRE_PREP: Pre prepare operation for the Port
> - * @SDW_OPS_PORT_PREP: Prepare operation for the Port
> + * @SDW_OPS_PORT_PRE_DEPREP: Pre deprepare operation for the Port
>   * @SDW_OPS_PORT_POST_PREP: Post prepare operation for the Port
> + * @SDW_OPS_PORT_POST_DEPREP: Post deprepare operation for the Port
>   */
>  enum sdw_port_prep_ops {
>  	SDW_OPS_PORT_PRE_PREP = 0,
> -	SDW_OPS_PORT_PREP = 1,
> -	SDW_OPS_PORT_POST_PREP = 2,
> +	SDW_OPS_PORT_PRE_DEPREP,
> +	SDW_OPS_PORT_POST_PREP,
> +	SDW_OPS_PORT_POST_DEPREP,
>  };
>  
>  /**

Re: [PATCH v2 2/8] ASoC: cs42l42: Add SOFT_RESET_REBOOT register

[Pierre-Louis Bossart]

On 1/18/23 10:04, Stefan Binding wrote:
> From: Richard Fitzgerald <rf@opensource.cirrus.com>
> 
> The SOFT_RESET_REBOOT register is needed to recover CS42L42 state after
> a Soundwire bus reset.

Humm, you probably want to clarify the terminology, the 'soft reset' is
defined in the SoundWire spec as the case where the peripheral device
loses sync. Bus reset is a Severe Reset, but there's also a Hard Reset.

does this 'SOFT_RESET_REBOOT' need to be accessed when there's a soft
reset, or only after a Severe/Hard Reset?

> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Signed-off-by: Stefan Binding <sbinding@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 1d1c24fdd0cae..3994e933db195 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
> @@ -720,6 +721,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 2fefbcf7bd130..82aa11d6937be 100644
> --- a/sound/soc/codecs/cs42l42.c
> +++ b/sound/soc/codecs/cs42l42.c
> @@ -293,6 +293,7 @@ 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 @@ 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;

Re: [PATCH v2 3/8] ASoC: cs42l42: Ensure MCLKint is a multiple of the sample rate

[Pierre-Louis Bossart]

On 1/18/23 10:04, Stefan Binding wrote:
> From: Richard Fitzgerald <rf@opensource.cirrus.com>
> 
> 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.

The explanation is a bit convoluted, clearly the 147-row configuration
was only meant to be used with 44100 kHz. I don't think 48kHz can be
supported without using the source- or sink-controlled async modes.

This is still a valid change so:

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Signed-off-by: Stefan Binding <sbinding@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 82aa11d6937be..939f8bcc222c0 100644
> --- a/sound/soc/codecs/cs42l42.c
> +++ b/sound/soc/codecs/cs42l42.c
> @@ -653,7 +653,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;
> @@ -668,6 +669,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;
>  
> @@ -893,6 +898,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;
> @@ -956,11 +962,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;
>  }

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Pierre-Louis Bossart]

nitpick: please use the MIPI SoundWire spelling

> 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/disable are done during
>   the port_prep callback.
> 
> - 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.
That's an impressive commit message indeed.

I couldn't really follow this paragraph though. The main reason why
having suspend/resume routines is to wait for initialization to be
complete, as well as handle the regcache status to deal with access to
regmap'ed registers, if any, while the bus is stopped or resuming.

Edit after reaching the end of this patch: that's actually what is done
in the implementation below so you may want to clarify this part.

> - Intel Soundwire host controllers have a low-power clock-stop mode that
>   requires resetting all peripherals when resuming. This means that the
>   interrupt registers will be reset in between the interrupt being
>   generated and the interrupt being handled, and since the interrupt
>   status is debounced, these values may not be accurrate immediately,

accurate


> diff --git a/sound/soc/codecs/cs42l42-sdw.c b/sound/soc/codecs/cs42l42-sdw.c
> new file mode 100644
> index 0000000000000..67800b275e422
> --- /dev/null
> +++ b/sound/soc/codecs/cs42l42-sdw.c
> @@ -0,0 +1,595 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// cs42l42-sdw.c -- CS42L42 ALSA SoC audio driver Soundwire binding

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/sdw.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 */

nitpick: 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;

Can this happen? IIRC the ASoC framework would use
pm_runtime_resume_and_get() before .startup, which would guarantee that
the device is initialized, no?

> +
> +	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 stream_config = {0};
> +	struct sdw_port_config port_config = {0};
> +	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);

wouldn't it be better to check if the sample_rate is supported by the
PLL here, instead of in the .prepare step ...

> +
> +	snd_sdw_params_to_config(substream, params, &stream_config, &port_config);
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		port_config.num = CS42L42_SDW_PLAYBACK_PORT;
> +	else
> +		port_config.num = CS42L42_SDW_CAPTURE_PORT;
> +
> +	ret = sdw_stream_add_slave(cs42l42->sdw_peripheral, &stream_config, &port_config, 1,
> +				   sdw_stream);
> +	if (ret) {
> +		dev_err(dai->dev, "Failed to add sdw stream: %d\n", ret);
> +		return ret;
> +	}
> +
> +	cs42l42_src_config(dai->component, params_rate(params));
> +
> +	return 0;
> +}
> +
> +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);

... it's a bit late to verify the sample_rate is indeed supported, no?

> +}
> +
> +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);
> +
> +	sdw_stream_remove_slave(cs42l42->sdw_peripheral, sdw_stream);
> +	cs42l42->sample_rate = 0;
> +
> +	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;

Humm, this is interesting, you are not using the sdw_stream_data that
all other codecs use, but in hindsight I have no idea why we allocate
something to only store a pointer.


> +
> +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,

Are the rates and formats needed? IIRC only the channels are used.

> +	},
> +	.symmetric_rate = 1,
> +	.ops = &cs42l42_sdw_dai_ops,
> +};
> +

> +static void cs42l42_sdw_init(struct sdw_slave *peripheral)
> +{
> +	struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> +	int ret = 0;

unnecessary init

> +
> +	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);

you would want to set all this once during the first initialization.

> +	pm_runtime_mark_last_busy(cs42l42->dev);

usually this is added before the pm_runtime_enable()

> +	pm_runtime_idle(cs42l42->dev);

is this needed?

> +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);

unclear to me what happens is the bus suspends, how would you redo the init?

> +		break;
> +	case SDW_SLAVE_UNATTACHED:
> +		dev_dbg(cs42l42->dev, "UNATTACHED\n");
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	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;

that doesn't sound terribly useful?

> +}
> +
> +static const struct sdw_slave_ops cs42l42_sdw_ops = {
> +	.read_prop = cs42l42_sdw_read_prop,
> +	.update_status = cs42l42_sdw_update_status,

what about .interrupt_callback?

I vaguely remember something about not using the in-band wake mechanism
and having a separate interrupt line, if that's what happens it's worthy
of a comment here.

> +	.bus_config = cs42l42_sdw_bus_config,
> +	.port_prep = cs42l42_sdw_port_prep,
> +#ifdef DEBUG
> +	.clk_stop = cs42l42_sdw_clk_stop,
> +#endif
> +};

> +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);

is this necessary? I thought the device framework always did that.

> +	cs42l42_common_remove(cs42l42);
> +	pm_runtime_put(cs42l42->dev);
> +	pm_runtime_disable(cs42l42->dev);
> +
> +	return 0;
> +}

Re: [PATCH v2 0/8] ASoC: cs42l42: Add Soundwire support

[Pierre-Louis Bossart]

On 1/18/23 10:04, Stefan Binding wrote:
> The CS42L42 has a Soundwire interface for control and audio. This
> chain of patches adds support for this.
> 
> Patches #1 .. #5 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.
> 
> Patches #6 .. #8 actually adds the Soundwire handling.
> 
> Changes since v1:
> - fixes for various review comments from v1
> - add support for wakeup from clock stop using hardware interrupts
> - use port_prep callback to prepare/deprepare codec
> 
> Richard Fitzgerald (6):
>   ASoC: cs42l42: Add SOFT_RESET_REBOOT register
>   ASoC: cs42l42: Ensure MCLKint is a multiple of the sample rate
>   ASoC: cs42l42: Separate ASP config from PLL config
>   ASoC: cs42l42: Export some functions for Soundwire
>   ASoC: cs42l42: Add Soundwire support
>   ASoC: cs42l42: Don't set idle_bias_on
> 
> Stefan Binding (2):
>   soundwire: stream: Add specific prep/deprep commands to port_prep
>     callback

probably want to CC: Vinod and Bard to get their Acked-by tag...

>   ASoC: cs42l42: Wait for debounce interval after resume
> 
>  drivers/soundwire/stream.c     |   4 +-
>  include/linux/soundwire/sdw.h  |   8 +-
>  include/sound/cs42l42.h        |   5 +
>  sound/soc/codecs/Kconfig       |   8 +
>  sound/soc/codecs/Makefile      |   2 +
>  sound/soc/codecs/cs42l42-sdw.c | 603 +++++++++++++++++++++++++++++++++
>  sound/soc/codecs/cs42l42.c     | 127 ++++---
>  sound/soc/codecs/cs42l42.h     |   9 +-
>  8 files changed, 716 insertions(+), 50 deletions(-)
>  create mode 100644 sound/soc/codecs/cs42l42-sdw.c
> 

Re: [PATCH v2 2/8] ASoC: cs42l42: Add SOFT_RESET_REBOOT register

[Richard Fitzgerald]

On 18/1/23 16:41, Pierre-Louis Bossart wrote:
> 
> 
> On 1/18/23 10:04, Stefan Binding wrote:
>> From: Richard Fitzgerald <rf@opensource.cirrus.com>
>>
>> The SOFT_RESET_REBOOT register is needed to recover CS42L42 state after
>> a Soundwire bus reset.
> 
> Humm, you probably want to clarify the terminology, the 'soft reset' is

SOFT_RESET is what the register is called.

> defined in the SoundWire spec as the case where the peripheral device
> loses sync. Bus reset is a Severe Reset, but there's also a Hard Reset.
>
> does this 'SOFT_RESET_REBOOT' need to be accessed when there's a soft
> reset, or only after a Severe/Hard Reset?
> 

After a bus(severe)-reset or a forced(hard)-reset, but the code in
driver/soundwire doesn't issue FORCE_RESET so there's no need to handle
that. If there was some reason to have the core SoundWire code issue
FORCE_RESET it would also need to add a callback to all clients so they
can do any special handling.

 From the datasheet:

"After a FORCE_RESET, the master must issue a reboot command (set 
SFT_RST_REBOOT; see
p. 162) and wait for 2.5 ms.

After a bus reset, the master must issue a reboot command (set
SFT_RST_REBOOT; see p. 162) and wait for 2.5 ms."

SFT_RESET_REBOOT is in the SOFT_RESET_REBOOT register.

>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> Signed-off-by: Stefan Binding <sbinding@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 1d1c24fdd0cae..3994e933db195 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
>> @@ -720,6 +721,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 2fefbcf7bd130..82aa11d6937be 100644
>> --- a/sound/soc/codecs/cs42l42.c
>> +++ b/sound/soc/codecs/cs42l42.c
>> @@ -293,6 +293,7 @@ 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 @@ 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;

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Richard Fitzgerald]

On 18/1/23 17:41, Pierre-Louis Bossart wrote:
> 
> nitpick: please use the MIPI SoundWire spelling
> 
>> 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/disable are done during
>>    the port_prep callback.
>>
>> - 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.
> That's an impressive commit message indeed.
> 
> I couldn't really follow this paragraph though. The main reason why
> having suspend/resume routines is to wait for initialization to be
> complete, as well as handle the regcache status to deal with access to
> regmap'ed registers, if any, while the bus is stopped or resuming.
> 

Can be re-worded. The point was that the CS42L42 driver doesn't have any
pm_runtime support, so why add it for SoundWire? Answer: because it's
simpler to make use of the automatic suspend/resume of the host
controller that comes with having pm_runtime enabled. It's much nicer
than having to manually pm_runtime_get_sync() the bus manager and check
for unattach_request every time we want to do a register transfer.

> Edit after reaching the end of this patch: that's actually what is done
> in the implementation below so you may want to clarify this part.
> 
>> - Intel Soundwire host controllers have a low-power clock-stop mode that
>>    requires resetting all peripherals when resuming. This means that the
>>    interrupt registers will be reset in between the interrupt being
>>    generated and the interrupt being handled, and since the interrupt
>>    status is debounced, these values may not be accurrate immediately,
> 
> accurate
> 
> 
>> diff --git a/sound/soc/codecs/cs42l42-sdw.c b/sound/soc/codecs/cs42l42-sdw.c
>> new file mode 100644
>> index 0000000000000..67800b275e422
>> --- /dev/null
>> +++ b/sound/soc/codecs/cs42l42-sdw.c
>> @@ -0,0 +1,595 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// cs42l42-sdw.c -- CS42L42 ALSA SoC audio driver Soundwire binding
> 
> 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/sdw.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 */
> 
> nitpick: 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;
> 
> Can this happen? IIRC the ASoC framework would use
> pm_runtime_resume_and_get() before .startup, which would guarantee that
> the device is initialized, no?
> 

Yes, this can happen. Because of the way that the SoundWire enumeration
was implemented in the core code, it isn't a probe event so we cannot
call snd_soc_register_component() on enumeration because -EPROBE_DEFER
wouldn't be handled. So the snd_soc_register_component() must be called
from probe(). This leaves a limbo situation where we've registered the
driver but in fact don't yet have any hardware. ALSA/ASoC doesn't know
that we've registered before we are functional so they are happy to
go ahead and try to use the soundcard. If for some reason the hardware
failed to enumerate we can get here without having enumerated.

>> +
>> +	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 stream_config = {0};
>> +	struct sdw_port_config port_config = {0};
>> +	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);
> 
> wouldn't it be better to check if the sample_rate is supported by the
> PLL here, instead of in the .prepare step ...
> 
It depends on the soundwire bus clock. We need to know both to determine
whether they are valid. IFF we can assume that the call to
sdw_stream_add_slave() will always invoke the bus_config() callback we
can call cs42l42_pll_config() from cs42l42_sdw_bus_config() and return
an error from cs42l42_sdw_bus_config() if the {swire_clk, sample_rate}
pair isn't valid.

>> +
>> +	snd_sdw_params_to_config(substream, params, &stream_config, &port_config);
>> +
>> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +		port_config.num = CS42L42_SDW_PLAYBACK_PORT;
>> +	else
>> +		port_config.num = CS42L42_SDW_CAPTURE_PORT;
>> +
>> +	ret = sdw_stream_add_slave(cs42l42->sdw_peripheral, &stream_config, &port_config, 1,
>> +				   sdw_stream);
>> +	if (ret) {
>> +		dev_err(dai->dev, "Failed to add sdw stream: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	cs42l42_src_config(dai->component, params_rate(params));
>> +
>> +	return 0;
>> +}
>> +
>> +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);
> 
> ... it's a bit late to verify the sample_rate is indeed supported, no?
> 
>> +}
>> +
>> +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);
>> +
>> +	sdw_stream_remove_slave(cs42l42->sdw_peripheral, sdw_stream);
>> +	cs42l42->sample_rate = 0;
>> +
>> +	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;
> 
> Humm, this is interesting, you are not using the sdw_stream_data that
> all other codecs use, but in hindsight I have no idea why we allocate
> something to only store a pointer.
> 

Indeed. I can see no reason to wrap this pointer in another struct when
we can store the pointer direct so I dropped the wrapper struct.

> 
>> +
>> +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,
> 
> Are the rates and formats needed? IIRC only the channels are used.
>

These are the only rates and formats we support so we want ASoC to
create parameter constraints so ALSA will refine any app requests down
to a supported combination.

>> +	},
>> +	.symmetric_rate = 1,
>> +	.ops = &cs42l42_sdw_dai_ops,
>> +};
>> +
> 
>> +static void cs42l42_sdw_init(struct sdw_slave *peripheral)
>> +{
>> +	struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
>> +	int ret = 0;
> 
> unnecessary init
> 
>> +
>> +	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);
> 
> you would want to set all this once during the first initialization.
> 
>> +	pm_runtime_mark_last_busy(cs42l42->dev);
> 
> usually this is added before the pm_runtime_enable()
> 
>> +	pm_runtime_idle(cs42l42->dev);
> 
> is this needed?
> 
>> +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);
> 
> unclear to me what happens is the bus suspends, how would you redo the init?
> 

We don't need to re-run the init(). A regcache_sync() will restore
settings.

>> +		break;
>> +	case SDW_SLAVE_UNATTACHED:
>> +		dev_dbg(cs42l42->dev, "UNATTACHED\n");
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	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;
> 
> that doesn't sound terribly useful?
> 

Well, it's _very_ useful for debug but this function could be
dropped.

>> +}
>> +
>> +static const struct sdw_slave_ops cs42l42_sdw_ops = {
>> +	.read_prop = cs42l42_sdw_read_prop,
>> +	.update_status = cs42l42_sdw_update_status,
> 
> what about .interrupt_callback?
>

Not used. That's a for later patch if we can get CS42L42 to issue
SoundWire wakeup events in any useful way. Currently the only
chip interrupt we use is for jack detect and that needs the
hard INT line because it cannot issue a SoundWire WAKE during
clock-stop.

> I vaguely remember something about not using the in-band wake mechanism
> and having a separate interrupt line, if that's what happens it's worthy
> of a comment here.

Yes, we can add a comment

> 
>> +	.bus_config = cs42l42_sdw_bus_config,
>> +	.port_prep = cs42l42_sdw_port_prep,
>> +#ifdef DEBUG
>> +	.clk_stop = cs42l42_sdw_clk_stop,
>> +#endif
>> +};
> 
>> +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);
> 
> is this necessary? I thought the device framework always did that.
>

You're correct. There's other drivers that do this so I was cautious
but yes this can be removed.

>> +	cs42l42_common_remove(cs42l42);
>> +	pm_runtime_put(cs42l42->dev);
>> +	pm_runtime_disable(cs42l42->dev);
>> +
>> +	return 0;
>> +}
> 

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Pierre-Louis Bossart]

>>> +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;
>>
>> Can this happen? IIRC the ASoC framework would use
>> pm_runtime_resume_and_get() before .startup, which would guarantee that
>> the device is initialized, no?
>>
> 
> Yes, this can happen. Because of the way that the SoundWire enumeration
> was implemented in the core code, it isn't a probe event so we cannot
> call snd_soc_register_component() on enumeration because -EPROBE_DEFER
> wouldn't be handled. So the snd_soc_register_component() must be called
> from probe(). This leaves a limbo situation where we've registered the
> driver but in fact don't yet have any hardware. ALSA/ASoC doesn't know
> that we've registered before we are functional so they are happy to
> go ahead and try to use the soundcard. If for some reason the hardware
> failed to enumerate we can get here without having enumerated.

Humm, yes, but you've also made the regmap cache-only, so is there
really a problem?

FWIW I don't see a startup callback in any other codec driver. It may be
wrong but it's also a sign that this isn't a problem we've seen so far
on existing Intel-based platforms.

> 
>>> +
>>> +    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 stream_config = {0};
>>> +    struct sdw_port_config port_config = {0};
>>> +    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);
>>
>> wouldn't it be better to check if the sample_rate is supported by the
>> PLL here, instead of in the .prepare step ...
>>
> It depends on the soundwire bus clock. We need to know both to determine
> whether they are valid. IFF we can assume that the call to
> sdw_stream_add_slave() will always invoke the bus_config() callback we
> can call cs42l42_pll_config() from cs42l42_sdw_bus_config() and return
> an error from cs42l42_sdw_bus_config() if the {swire_clk, sample_rate}
> pair isn't valid.

You lost me here. Are you saying the soundwire bus clock is only known
in the prepare stage?


>>> +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;
>>
>> Humm, this is interesting, you are not using the sdw_stream_data that
>> all other codecs use, but in hindsight I have no idea why we allocate
>> something to only store a pointer.
>>
> 
> Indeed. I can see no reason to wrap this pointer in another struct when
> we can store the pointer direct so I dropped the wrapper struct.

I'll see if we can simplify the other codec drivers.

>>> +static int cs42l42_sdw_update_status(struct sdw_slave *peripheral,
>>> +                     enum sdw_slave_status status);s
>>> +{
>>> +    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);
>>
>> unclear to me what happens is the bus suspends, how would you redo the
>> init?
>>
> 
> We don't need to re-run the init(). A regcache_sync() will restore
> settings.

ah, interesting. Other codec drivers play with specific registers that
aren't in regmap. There's also headset calibration that's done
differently in the first init or later.

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Richard Fitzgerald]

On 19/1/23 14:48, Pierre-Louis Bossart wrote:
> 
>>>> +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;
>>>
>>> Can this happen? IIRC the ASoC framework would use
>>> pm_runtime_resume_and_get() before .startup, which would guarantee that
>>> the device is initialized, no?
>>>
>>
>> Yes, this can happen. Because of the way that the SoundWire enumeration
>> was implemented in the core code, it isn't a probe event so we cannot
>> call snd_soc_register_component() on enumeration because -EPROBE_DEFER
>> wouldn't be handled. So the snd_soc_register_component() must be called
>> from probe(). This leaves a limbo situation where we've registered the
>> driver but in fact don't yet have any hardware. ALSA/ASoC doesn't know
>> that we've registered before we are functional so they are happy to
>> go ahead and try to use the soundcard. If for some reason the hardware
>> failed to enumerate we can get here without having enumerated.
> 
> Humm, yes, but you've also made the regmap cache-only, so is there
> really a problem?
> 

It's true that normally we go past these stages in cache-only, but that
is because normally (non-Soundwire) we already initialized the hardware
to good state during probe().
If we just carry on when it hasn't enumerated and we haven't initialized
it yet, who knows what will happen if it enumerates some time later.

We could just ignore it and see if anyone has a problem but for the sake
of a couple of lines of code I feel like I'd rather check for it.

> FWIW I don't see a startup callback in any other codec driver. It may be
> wrong but it's also a sign that this isn't a problem we've seen so far
> on existing Intel-based platforms.
>

It's nicer to do the check in startup() because then the application
open() will fail cleanly. We could delay until prepare - which is the
point we really need the hardware to be accessible - and hope the
hardware enumerated and initialized by that time. But that's not so
nice from the app point of view.

>>
>>>> +
>>>> +    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 stream_config = {0};
>>>> +    struct sdw_port_config port_config = {0};
>>>> +    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);
>>>
>>> wouldn't it be better to check if the sample_rate is supported by the
>>> PLL here, instead of in the .prepare step ...
>>>
>> It depends on the soundwire bus clock. We need to know both to determine
>> whether they are valid. IFF we can assume that the call to
>> sdw_stream_add_slave() will always invoke the bus_config() callback we
>> can call cs42l42_pll_config() from cs42l42_sdw_bus_config() and return
>> an error from cs42l42_sdw_bus_config() if the {swire_clk, sample_rate}
>> pair isn't valid.
> 
> You lost me here. Are you saying the soundwire bus clock is only known
> in the prepare stage?
>

hw_params() doesn't know the Soundwire bus clock so it can't do the
check. We need to wait until we have both the sample rate and the
chosen SWIRE_CLK.

I delayed it until prepare() because by that time the SWIRE_CLK must
have been chosen. and it avoids making an assumption about whether 
bus_config() will be called if the SWIRE_CLK hasn't changed.
If we move the cs42l42_pll_config() to the bus_config we must be sure
that
1) the bus_config() callback will be called from sdw_stream_add_slave().
(If bus_config() is only called when the machine driver prepares then
the check won't happen during hw_params phase.)

2) the bus_config must be called even if the SWIRE_CLK does not change,
so that we can reconsider our PLL config if the sample rate has changed.

If (1) and (2) are not guaranteed then moving the cs42l42_pll_config()
call to the bus_config() callback would be worse.

Putting it in prepare() means that hw_params() must have run and the
bus_config() callback has been called. And we don't care if bus_config()
was triggered by our hw_params or the machine driver. Also we don't
care if bus_config() wasn't called because the config hasn't changed.

In the end I'm not sure that it matters. Returning an error from
hw_params() won't re-run the ALSA constraint refinement. It's too late
by then because the params have already been accepted. The ACPI should
be configuring the Soundwire manager to only output valid SWIRE_CLK
frequencies for CS42L42, so we should never hit this error. But if we
come across a broken ACPI we have an error logged in dmesg to tell us
what went wrong, instead of a mysterious user complaint that their
audio is strange.

> 
>>>> +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;
>>>
>>> Humm, this is interesting, you are not using the sdw_stream_data that
>>> all other codecs use, but in hindsight I have no idea why we allocate
>>> something to only store a pointer.
>>>
>>
>> Indeed. I can see no reason to wrap this pointer in another struct when
>> we can store the pointer direct so I dropped the wrapper struct.
> 
> I'll see if we can simplify the other codec drivers.
> 
>>>> +static int cs42l42_sdw_update_status(struct sdw_slave *peripheral,
>>>> +                     enum sdw_slave_status status);s
>>>> +{
>>>> +    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);
>>>
>>> unclear to me what happens is the bus suspends, how would you redo the
>>> init?
>>>
>>
>> We don't need to re-run the init(). A regcache_sync() will restore
>> settings.
> 
> ah, interesting. Other codec drivers play with specific registers that
> aren't in regmap. There's also headset calibration that's done
> differently in the first init or later.

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Pierre-Louis Bossart]

>> You lost me here. Are you saying the soundwire bus clock is only known
>> in the prepare stage?
>>
> 
> hw_params() doesn't know the Soundwire bus clock so it can't do the
> check. We need to wait until we have both the sample rate and the
> chosen SWIRE_CLK.

Yes, makes sense. I forgot that all the stream management and bandwidth
allocation takes place in the prepare stage at the dailink level, and
the dai prepare happens after that. Thanks for the clarification.

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Richard Fitzgerald]

On 19/1/23 16:27, Pierre-Louis Bossart wrote:
> 
>>> You lost me here. Are you saying the soundwire bus clock is only known
>>> in the prepare stage?
>>>
>>
>> hw_params() doesn't know the Soundwire bus clock so it can't do the
>> check. We need to wait until we have both the sample rate and the
>> chosen SWIRE_CLK.
> 
> Yes, makes sense. I forgot that all the stream management and bandwidth
> allocation takes place in the prepare stage at the dailink level, and
> the dai prepare happens after that. Thanks for the clarification.

Also, this isn't validating the params passed by the application.
The application cannot pass us "bad" params that would cause pll_config
to fail.

The only way the pll_config could fail is if the SoundWire core code
chose a SWIRE_CLK that CS42L42 cannot support. This should never happen
and if it does it means there's an error in the ACPI or the machine
driver.

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Pierre-Louis Bossart]

On 1/19/23 09:35, Richard Fitzgerald wrote:
> On 19/1/23 14:48, Pierre-Louis Bossart wrote:
>>
>>>>> +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;
>>>>
>>>> Can this happen? IIRC the ASoC framework would use
>>>> pm_runtime_resume_and_get() before .startup, which would guarantee that
>>>> the device is initialized, no?
>>>>
>>>
>>> Yes, this can happen. Because of the way that the SoundWire enumeration
>>> was implemented in the core code, it isn't a probe event so we cannot
>>> call snd_soc_register_component() on enumeration because -EPROBE_DEFER
>>> wouldn't be handled. So the snd_soc_register_component() must be called
>>> from probe(). This leaves a limbo situation where we've registered the
>>> driver but in fact don't yet have any hardware. ALSA/ASoC doesn't know
>>> that we've registered before we are functional so they are happy to
>>> go ahead and try to use the soundcard. If for some reason the hardware
>>> failed to enumerate we can get here without having enumerated.
>>
>> Humm, yes, but you've also made the regmap cache-only, so is there
>> really a problem?
>>
> 
> It's true that normally we go past these stages in cache-only, but that
> is because normally (non-Soundwire) we already initialized the hardware
> to good state during probe().
> If we just carry on when it hasn't enumerated and we haven't initialized
> it yet, who knows what will happen if it enumerates some time later.
> 
> We could just ignore it and see if anyone has a problem but for the sake
> of a couple of lines of code I feel like I'd rather check for it.
> 
>> FWIW I don't see a startup callback in any other codec driver. It may be
>> wrong but it's also a sign that this isn't a problem we've seen so far
>> on existing Intel-based platforms.
>>
> 
> It's nicer to do the check in startup() because then the application
> open() will fail cleanly. We could delay until prepare - which is the
> point we really need the hardware to be accessible - and hope the
> hardware enumerated and initialized by that time. But that's not so
> nice from the app point of view.

Another way to avoid problems is to rely on the codec component .probe
to check if the SoundWire device is initialized before registering a card.

I just tried with a system where the ACPI info exposes a codec which is
not connected, it fails nicely. That avoids the pitfalls of creating a
card which isn't functional since all dependencies are not met.

[   64.616530] snd_soc_sof_sdw:mc_probe: sof_sdw sof_sdw: Entry
[   64.616549] snd_soc_sof_sdw:log_quirks: sof_sdw sof_sdw: quirk
SOF_SDW_PCH_DMIC enabled
[   64.616559] snd_soc_sof_sdw:sof_card_dai_links_create: sof_sdw
sof_sdw: sdw 2, ssp 0, dmic 2, hdmi 0
[   64.616587] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
dai link SDW0-Playback, id 0
[   64.616600] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
dai link SDW0-Capture, id 1
[   64.616607] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
dai link dmic01, id 2
[   64.616614] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
dai link dmic16k, id 3
[   69.757115] rt5682 sdw:0:025d:5682:00: Initialization not complete,
timed out
[   69.757128] rt5682 sdw:0:025d:5682:00: ASoC: error at
snd_soc_component_probe on sdw:0:025d:5682:00: -110
[   69.757224] sof_sdw sof_sdw: ASoC: failed to instantiate card -110
[   69.757734] sof_sdw sof_sdw: snd_soc_register_card failed -110

see
https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/rt5682.c#L2927

I think this is compatible with the device model and bind/unbind, but it
could be improved with the removal of the wait if we had a way to return
-EPROBEDEFER, and have a mechanism to force the deferred probe work to
be triggered when a device actually shows up. It's a generic problem
that the probe cannot always be a synchronous function but may complete
'later'.

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Richard Fitzgerald]

On 20/01/2023 19:55, Pierre-Louis Bossart wrote:
> 
> 
> On 1/19/23 09:35, Richard Fitzgerald wrote:
>> On 19/1/23 14:48, Pierre-Louis Bossart wrote:
>>>
>>>>>> +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;
>>>>>
>>>>> Can this happen? IIRC the ASoC framework would use
>>>>> pm_runtime_resume_and_get() before .startup, which would guarantee that
>>>>> the device is initialized, no?
>>>>>
>>>>
>>>> Yes, this can happen. Because of the way that the SoundWire enumeration
>>>> was implemented in the core code, it isn't a probe event so we cannot
>>>> call snd_soc_register_component() on enumeration because -EPROBE_DEFER
>>>> wouldn't be handled. So the snd_soc_register_component() must be called
>>>> from probe(). This leaves a limbo situation where we've registered the
>>>> driver but in fact don't yet have any hardware. ALSA/ASoC doesn't know
>>>> that we've registered before we are functional so they are happy to
>>>> go ahead and try to use the soundcard. If for some reason the hardware
>>>> failed to enumerate we can get here without having enumerated.
>>>
>>> Humm, yes, but you've also made the regmap cache-only, so is there
>>> really a problem?
>>>
>>
>> It's true that normally we go past these stages in cache-only, but that
>> is because normally (non-Soundwire) we already initialized the hardware
>> to good state during probe().
>> If we just carry on when it hasn't enumerated and we haven't initialized
>> it yet, who knows what will happen if it enumerates some time later.
>>
>> We could just ignore it and see if anyone has a problem but for the sake
>> of a couple of lines of code I feel like I'd rather check for it.
>>
>>> FWIW I don't see a startup callback in any other codec driver. It may be
>>> wrong but it's also a sign that this isn't a problem we've seen so far
>>> on existing Intel-based platforms.
>>>
>>
>> It's nicer to do the check in startup() because then the application
>> open() will fail cleanly. We could delay until prepare - which is the
>> point we really need the hardware to be accessible - and hope the
>> hardware enumerated and initialized by that time. But that's not so
>> nice from the app point of view.
> 
> Another way to avoid problems is to rely on the codec component .probe
> to check if the SoundWire device is initialized before registering a card.
> 
> I just tried with a system where the ACPI info exposes a codec which is
> not connected, it fails nicely. That avoids the pitfalls of creating a
> card which isn't functional since all dependencies are not met.
> 
> [   64.616530] snd_soc_sof_sdw:mc_probe: sof_sdw sof_sdw: Entry
> [   64.616549] snd_soc_sof_sdw:log_quirks: sof_sdw sof_sdw: quirk
> SOF_SDW_PCH_DMIC enabled
> [   64.616559] snd_soc_sof_sdw:sof_card_dai_links_create: sof_sdw
> sof_sdw: sdw 2, ssp 0, dmic 2, hdmi 0
> [   64.616587] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
> dai link SDW0-Playback, id 0
> [   64.616600] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
> dai link SDW0-Capture, id 1
> [   64.616607] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
> dai link dmic01, id 2
> [   64.616614] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
> dai link dmic16k, id 3
> [   69.757115] rt5682 sdw:0:025d:5682:00: Initialization not complete,
> timed out
> [   69.757128] rt5682 sdw:0:025d:5682:00: ASoC: error at
> snd_soc_component_probe on sdw:0:025d:5682:00: -110
> [   69.757224] sof_sdw sof_sdw: ASoC: failed to instantiate card -110
> [   69.757734] sof_sdw sof_sdw: snd_soc_register_card failed -110
> 
> see
> https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/rt5682.c#L2927
> 
> I think this is compatible with the device model and bind/unbind, but it
> could be improved with the removal of the wait if we had a way to return
> -EPROBEDEFER, and have a mechanism to force the deferred probe work to
> be triggered when a device actually shows up. It's a generic problem
> that the probe cannot always be a synchronous function but may complete
> 'later'.

I see what you've done in your patch, but I had already experimented
with this idea and found that the wait_for_completion() can deadlock the
Soundwire core.

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Pierre-Louis Bossart]

>>> It's nicer to do the check in startup() because then the application
>>> open() will fail cleanly. We could delay until prepare - which is the
>>> point we really need the hardware to be accessible - and hope the
>>> hardware enumerated and initialized by that time. But that's not so
>>> nice from the app point of view.
>>
>> Another way to avoid problems is to rely on the codec component .probe
>> to check if the SoundWire device is initialized before registering a
>> card.
>>
>> I just tried with a system where the ACPI info exposes a codec which is
>> not connected, it fails nicely. That avoids the pitfalls of creating a
>> card which isn't functional since all dependencies are not met.
>>
>> [   64.616530] snd_soc_sof_sdw:mc_probe: sof_sdw sof_sdw: Entry
>> [   64.616549] snd_soc_sof_sdw:log_quirks: sof_sdw sof_sdw: quirk
>> SOF_SDW_PCH_DMIC enabled
>> [   64.616559] snd_soc_sof_sdw:sof_card_dai_links_create: sof_sdw
>> sof_sdw: sdw 2, ssp 0, dmic 2, hdmi 0
>> [   64.616587] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>> dai link SDW0-Playback, id 0
>> [   64.616600] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>> dai link SDW0-Capture, id 1
>> [   64.616607] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>> dai link dmic01, id 2
>> [   64.616614] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>> dai link dmic16k, id 3
>> [   69.757115] rt5682 sdw:0:025d:5682:00: Initialization not complete,
>> timed out
>> [   69.757128] rt5682 sdw:0:025d:5682:00: ASoC: error at
>> snd_soc_component_probe on sdw:0:025d:5682:00: -110
>> [   69.757224] sof_sdw sof_sdw: ASoC: failed to instantiate card -110
>> [   69.757734] sof_sdw sof_sdw: snd_soc_register_card failed -110
>>
>> see
>> https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/rt5682.c#L2927
>>
>> I think this is compatible with the device model and bind/unbind, but it
>> could be improved with the removal of the wait if we had a way to return
>> -EPROBEDEFER, and have a mechanism to force the deferred probe work to
>> be triggered when a device actually shows up. It's a generic problem
>> that the probe cannot always be a synchronous function but may complete
>> 'later'.
> 
> I see what you've done in your patch, but I had already experimented
> with this idea and found that the wait_for_completion() can deadlock the
> Soundwire core.

That's not good. Do you have any logs or explanation on what the
root-cause of this deadlock might be? If something's broken, we might as
well fix it.

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Richard Fitzgerald]

On 23/01/2023 16:05, Pierre-Louis Bossart wrote:
> 
>>>> It's nicer to do the check in startup() because then the application
>>>> open() will fail cleanly. We could delay until prepare - which is the
>>>> point we really need the hardware to be accessible - and hope the
>>>> hardware enumerated and initialized by that time. But that's not so
>>>> nice from the app point of view.
>>>
>>> Another way to avoid problems is to rely on the codec component .probe
>>> to check if the SoundWire device is initialized before registering a
>>> card.
>>>
>>> I just tried with a system where the ACPI info exposes a codec which is
>>> not connected, it fails nicely. That avoids the pitfalls of creating a
>>> card which isn't functional since all dependencies are not met.
>>>
>>> [   64.616530] snd_soc_sof_sdw:mc_probe: sof_sdw sof_sdw: Entry
>>> [   64.616549] snd_soc_sof_sdw:log_quirks: sof_sdw sof_sdw: quirk
>>> SOF_SDW_PCH_DMIC enabled
>>> [   64.616559] snd_soc_sof_sdw:sof_card_dai_links_create: sof_sdw
>>> sof_sdw: sdw 2, ssp 0, dmic 2, hdmi 0
>>> [   64.616587] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>>> dai link SDW0-Playback, id 0
>>> [   64.616600] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>>> dai link SDW0-Capture, id 1
>>> [   64.616607] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>>> dai link dmic01, id 2
>>> [   64.616614] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>>> dai link dmic16k, id 3
>>> [   69.757115] rt5682 sdw:0:025d:5682:00: Initialization not complete,
>>> timed out
>>> [   69.757128] rt5682 sdw:0:025d:5682:00: ASoC: error at
>>> snd_soc_component_probe on sdw:0:025d:5682:00: -110
>>> [   69.757224] sof_sdw sof_sdw: ASoC: failed to instantiate card -110
>>> [   69.757734] sof_sdw sof_sdw: snd_soc_register_card failed -110
>>>
>>> see
>>> https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/rt5682.c#L2927
>>>
>>> I think this is compatible with the device model and bind/unbind, but it
>>> could be improved with the removal of the wait if we had a way to return
>>> -EPROBEDEFER, and have a mechanism to force the deferred probe work to
>>> be triggered when a device actually shows up. It's a generic problem
>>> that the probe cannot always be a synchronous function but may complete
>>> 'later'.
>>
>> I see what you've done in your patch, but I had already experimented
>> with this idea and found that the wait_for_completion() can deadlock the
>> Soundwire core.
> 
> That's not good. Do you have any logs or explanation on what the
> root-cause of this deadlock might be? If something's broken, we might as
> well fix it.

I suspect it might be the big mutex lock around the call to probe(), 
that I removed in one of my pending patches
(https://lore.kernel.org/all/20221121162453.1834170-1-rf@opensource.cirrus.com/)
So fixing that might make the problem go away.

Charles just pointed out to me that whether component_probe() is
called within probe() depends whether everything needed to create
a soundcard is already present. Most likely in my case everything is
available and so snd_soc_register_component() immediately calls
my component_probe(). So probably in your case not everything is
ready and so the call to component_probe() is deferred and you
don't see the deadlock.

Re: [PATCH v2 6/8] ASoC: cs42l42: Add Soundwire support

[Pierre-Louis Bossart]

On 1/23/23 10:14, Richard Fitzgerald wrote:
> On 23/01/2023 16:05, Pierre-Louis Bossart wrote:
>>
>>>>> It's nicer to do the check in startup() because then the application
>>>>> open() will fail cleanly. We could delay until prepare - which is the
>>>>> point we really need the hardware to be accessible - and hope the
>>>>> hardware enumerated and initialized by that time. But that's not so
>>>>> nice from the app point of view.
>>>>
>>>> Another way to avoid problems is to rely on the codec component .probe
>>>> to check if the SoundWire device is initialized before registering a
>>>> card.
>>>>
>>>> I just tried with a system where the ACPI info exposes a codec which is
>>>> not connected, it fails nicely. That avoids the pitfalls of creating a
>>>> card which isn't functional since all dependencies are not met.
>>>>
>>>> [   64.616530] snd_soc_sof_sdw:mc_probe: sof_sdw sof_sdw: Entry
>>>> [   64.616549] snd_soc_sof_sdw:log_quirks: sof_sdw sof_sdw: quirk
>>>> SOF_SDW_PCH_DMIC enabled
>>>> [   64.616559] snd_soc_sof_sdw:sof_card_dai_links_create: sof_sdw
>>>> sof_sdw: sdw 2, ssp 0, dmic 2, hdmi 0
>>>> [   64.616587] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>>>> dai link SDW0-Playback, id 0
>>>> [   64.616600] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>>>> dai link SDW0-Capture, id 1
>>>> [   64.616607] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>>>> dai link dmic01, id 2
>>>> [   64.616614] snd_soc_sof_sdw:init_dai_link: sof_sdw sof_sdw: create
>>>> dai link dmic16k, id 3
>>>> [   69.757115] rt5682 sdw:0:025d:5682:00: Initialization not complete,
>>>> timed out
>>>> [   69.757128] rt5682 sdw:0:025d:5682:00: ASoC: error at
>>>> snd_soc_component_probe on sdw:0:025d:5682:00: -110
>>>> [   69.757224] sof_sdw sof_sdw: ASoC: failed to instantiate card -110
>>>> [   69.757734] sof_sdw sof_sdw: snd_soc_register_card failed -110
>>>>
>>>> see
>>>> https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/rt5682.c#L2927
>>>>
>>>> I think this is compatible with the device model and bind/unbind,
>>>> but it
>>>> could be improved with the removal of the wait if we had a way to
>>>> return
>>>> -EPROBEDEFER, and have a mechanism to force the deferred probe work to
>>>> be triggered when a device actually shows up. It's a generic problem
>>>> that the probe cannot always be a synchronous function but may complete
>>>> 'later'.
>>>
>>> I see what you've done in your patch, but I had already experimented
>>> with this idea and found that the wait_for_completion() can deadlock the
>>> Soundwire core.
>>
>> That's not good. Do you have any logs or explanation on what the
>> root-cause of this deadlock might be? If something's broken, we might as
>> well fix it.
> 
> I suspect it might be the big mutex lock around the call to probe(),
> that I removed in one of my pending patches
> (https://lore.kernel.org/all/20221121162453.1834170-1-rf@opensource.cirrus.com/)
> So fixing that might make the problem go away.
> 
> Charles just pointed out to me that whether component_probe() is
> called within probe() depends whether everything needed to create
> a soundcard is already present. Most likely in my case everything is
> available and so snd_soc_register_component() immediately calls
> my component_probe(). So probably in your case not everything is
> ready and so the call to component_probe() is deferred and you
> don't see the deadlock.

In the case I tested, the codec driver was probed based on the presence
of ACPI information and the register component did happen. That means
all the resources were present for the card to be created, except that
the codec hardware was not connected to the bus so the initialization
never happened of course.