linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing
@ 2019-09-03 16:53 Katsuhiro Suzuki
  2019-09-03 16:53 ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Katsuhiro Suzuki
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Katsuhiro Suzuki @ 2019-09-03 16:53 UTC (permalink / raw)
  To: Mark Brown, David Yang, Daniel Drake, Hans de Goede
  Cc: alsa-devel, linux-kernel, Katsuhiro Suzuki

This patch change the judge timing about playing/capturing PCM rate.

Original code set constraints list of PCM rate limits at set_sysclk.
This strategy works well if system is using fixed rate clock.

But some boards and SoC (such as RockPro64 and RockChip I2S) has
connected SoC MCLK out to ES8316 MCLK in. In this case, SoC side I2S
will choose suitable frequency of MCLK such as fs * mclk-fs when
user starts playing or capturing.

Bad scenario as follows (mclk-fs = 256):
  - Initialize sysclk by correct value (Ex. 12.288MHz)
    - ES8316 set constraints of PCM rate by sysclk
      48kHz (1/256), 32kHz (1/384), 30.720kHz (1/400),
      24kHz (1/512), 16kHz (1/768), 12kHz (1/1024)
  - Play 48kHz sound, it's acceptable
  - Sysclk is not changed

  - Play 32kHz sound, it's acceptable
  - Set sysclk by 8.192MHz (= fs * mclk-fs = 32k * 256)
    - ES8316 set constraints of PCM rate by sysclk
      32kHz (1/256), 21.33kHz (1/384), 20.48kHz (1/400),
      16kHz (1/512), 10.66kHz (1/768), 8kHz (1/1024)

  - Play 48kHz again, but it's NOT acceptable because constraints
    list does not allow 48kHz

Root cause of this strange behavior is changing constraints list at
set_sysclk timing. It seems that is too early to determine. So this
patch does not use constraints list and check PCM rate limit more
later timing at hw_params.

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
---
 sound/soc/codecs/es8316.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index ed2959dbe1fb..229808fa627c 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -363,27 +363,12 @@ static int es8316_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 {
 	struct snd_soc_component *component = codec_dai->component;
 	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
-	int i;
-	int count = 0;
 
 	es8316->sysclk = freq;
 
 	if (freq == 0)
 		return 0;
 
-	/* Limit supported sample rates to ones that can be autodetected
-	 * by the codec running in slave mode.
-	 */
-	for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
-		const unsigned int ratio = supported_mclk_lrck_ratios[i];
-
-		if (freq % ratio == 0)
-			es8316->allowed_rates[count++] = freq / ratio;
-	}
-
-	es8316->sysclk_constraints.list = es8316->allowed_rates;
-	es8316->sysclk_constraints.count = count;
-
 	return 0;
 }
 
@@ -449,13 +434,6 @@ static int es8316_pcm_startup(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
-	/* The set of sample rates that can be supported depends on the
-	 * MCLK supplied to the CODEC.
-	 */
-	snd_pcm_hw_constraint_list(substream->runtime, 0,
-				   SNDRV_PCM_HW_PARAM_RATE,
-				   &es8316->sysclk_constraints);
-
 	return 0;
 }
 
@@ -466,12 +444,27 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_component *component = dai->component;
 	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
 	u8 wordlen = 0;
+	int i;
 
 	if (!es8316->sysclk) {
 		dev_err(component->dev, "No MCLK configured\n");
 		return -EINVAL;
 	}
 
+	/* Limit supported sample rates to ones that can be autodetected
+	 * by the codec running in slave mode.
+	 */
+	for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
+		const unsigned int ratio = supported_mclk_lrck_ratios[i];
+
+		if (es8316->sysclk % ratio != 0)
+			continue;
+		if (es8316->sysclk / ratio == params_rate(params))
+			break;
+	}
+	if (i == NR_SUPPORTED_MCLK_LRCK_RATIOS)
+		return -EINVAL;
+
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
 		wordlen = ES8316_SERDATA2_LEN_16;
-- 
2.23.0.rc1


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

* [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK
  2019-09-03 16:53 [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing Katsuhiro Suzuki
@ 2019-09-03 16:53 ` Katsuhiro Suzuki
  2019-09-03 18:00   ` Applied "ASoC: es8316: add clock control of MCLK" to the asoc tree Mark Brown
  2019-09-04 14:37   ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Andy Shevchenko
  2019-09-03 16:53 ` [PATCH v3 3/4] ASoC: es8316: support fixed clock rate Katsuhiro Suzuki
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Katsuhiro Suzuki @ 2019-09-03 16:53 UTC (permalink / raw)
  To: Mark Brown, David Yang, Daniel Drake, Hans de Goede
  Cc: alsa-devel, linux-kernel, Katsuhiro Suzuki

This patch introduce clock property for MCLK master freq control.
Driver will set rate of MCLK master if set_sysclk is called and
changing sysclk by board driver.

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>

---

Changes from v1:
  - Output logs if clock error is found
  - Disable and unprepare mclk when remove this driver
---
 sound/soc/codecs/es8316.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index 229808fa627c..ab41f2c056bd 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -9,6 +9,7 @@
 
 #include <linux/module.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
@@ -33,6 +34,7 @@ static const unsigned int supported_mclk_lrck_ratios[] = {
 
 struct es8316_priv {
 	struct mutex lock;
+	struct clk *mclk;
 	struct regmap *regmap;
 	struct snd_soc_component *component;
 	struct snd_soc_jack *jack;
@@ -363,12 +365,19 @@ static int es8316_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 {
 	struct snd_soc_component *component = codec_dai->component;
 	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	es8316->sysclk = freq;
 
 	if (freq == 0)
 		return 0;
 
+	if (es8316->mclk) {
+		ret = clk_set_rate(es8316->mclk, freq);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -693,9 +702,26 @@ static int es8316_set_jack(struct snd_soc_component *component,
 static int es8316_probe(struct snd_soc_component *component)
 {
 	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	es8316->component = component;
 
+	es8316->mclk = devm_clk_get(component->dev, "mclk");
+	if (PTR_ERR(es8316->mclk) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (IS_ERR(es8316->mclk)) {
+		dev_err(component->dev, "clock is invalid, ignored\n");
+		es8316->mclk = NULL;
+	}
+
+	if (es8316->mclk) {
+		ret = clk_prepare_enable(es8316->mclk);
+		if (ret) {
+			dev_err(component->dev, "unable to enable clock\n");
+			return ret;
+		}
+	}
+
 	/* Reset codec and enable current state machine */
 	snd_soc_component_write(component, ES8316_RESET, 0x3f);
 	usleep_range(5000, 5500);
@@ -718,8 +744,17 @@ static int es8316_probe(struct snd_soc_component *component)
 	return 0;
 }
 
+static void es8316_remove(struct snd_soc_component *component)
+{
+	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
+
+	if (es8316->mclk)
+		clk_disable_unprepare(es8316->mclk);
+}
+
 static const struct snd_soc_component_driver soc_component_dev_es8316 = {
 	.probe			= es8316_probe,
+	.remove			= es8316_remove,
 	.set_jack		= es8316_set_jack,
 	.controls		= es8316_snd_controls,
 	.num_controls		= ARRAY_SIZE(es8316_snd_controls),
-- 
2.23.0.rc1


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

* [PATCH v3 3/4] ASoC: es8316: support fixed clock rate
  2019-09-03 16:53 [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing Katsuhiro Suzuki
  2019-09-03 16:53 ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Katsuhiro Suzuki
@ 2019-09-03 16:53 ` Katsuhiro Suzuki
  2019-09-03 17:29   ` Mark Brown
  2019-09-03 16:53 ` [PATCH v3 4/4] ASoC: es8316: add DT-bindings Katsuhiro Suzuki
  2019-09-03 17:48 ` [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing Mark Brown
  3 siblings, 1 reply; 13+ messages in thread
From: Katsuhiro Suzuki @ 2019-09-03 16:53 UTC (permalink / raw)
  To: Mark Brown, David Yang, Daniel Drake, Hans de Goede
  Cc: alsa-devel, linux-kernel, Katsuhiro Suzuki

This patch supports some type of machine drivers that use fixed clock
rate. After applied this patch, sysclk == 0 means there is no
constraint of sound rate and other values will set constraints which
is derived by sysclk setting.

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>

---

Changes from v2:
  - Newly added
---
 sound/soc/codecs/es8316.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index ab41f2c056bd..bf390bc64177 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -432,20 +432,6 @@ static int es8316_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
-static int es8316_pcm_startup(struct snd_pcm_substream *substream,
-			      struct snd_soc_dai *dai)
-{
-	struct snd_soc_component *component = dai->component;
-	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
-
-	if (es8316->sysclk == 0) {
-		dev_err(component->dev, "No sysclk provided\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
 				struct snd_pcm_hw_params *params,
 				struct snd_soc_dai *dai)
@@ -455,17 +441,16 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
 	u8 wordlen = 0;
 	int i;
 
-	if (!es8316->sysclk) {
-		dev_err(component->dev, "No MCLK configured\n");
-		return -EINVAL;
-	}
-
 	/* Limit supported sample rates to ones that can be autodetected
 	 * by the codec running in slave mode.
 	 */
 	for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
 		const unsigned int ratio = supported_mclk_lrck_ratios[i];
 
+		/* Accept any rates if sysclk is 0. */
+		if (es8316->sysclk == 0)
+			break;
+
 		if (es8316->sysclk % ratio != 0)
 			continue;
 		if (es8316->sysclk / ratio == params_rate(params))
@@ -509,7 +494,6 @@ static int es8316_mute(struct snd_soc_dai *dai, int mute)
 			SNDRV_PCM_FMTBIT_S24_LE)
 
 static const struct snd_soc_dai_ops es8316_ops = {
-	.startup = es8316_pcm_startup,
 	.hw_params = es8316_pcm_hw_params,
 	.set_fmt = es8316_set_dai_fmt,
 	.set_sysclk = es8316_set_dai_sysclk,
-- 
2.23.0.rc1


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

* [PATCH v3 4/4] ASoC: es8316: add DT-bindings
  2019-09-03 16:53 [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing Katsuhiro Suzuki
  2019-09-03 16:53 ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Katsuhiro Suzuki
  2019-09-03 16:53 ` [PATCH v3 3/4] ASoC: es8316: support fixed clock rate Katsuhiro Suzuki
@ 2019-09-03 16:53 ` Katsuhiro Suzuki
  2019-09-03 18:00   ` Applied "ASoC: es8316: add DT-bindings" to the asoc tree Mark Brown
  2019-09-03 17:48 ` [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing Mark Brown
  3 siblings, 1 reply; 13+ messages in thread
From: Katsuhiro Suzuki @ 2019-09-03 16:53 UTC (permalink / raw)
  To: Mark Brown, David Yang, Daniel Drake, Hans de Goede
  Cc: alsa-devel, linux-kernel, Katsuhiro Suzuki

This patch adds missing DT-bindings document for Everest ES8316.

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
---
 .../bindings/sound/everest,es8316.txt         | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/everest,es8316.txt

diff --git a/Documentation/devicetree/bindings/sound/everest,es8316.txt b/Documentation/devicetree/bindings/sound/everest,es8316.txt
new file mode 100644
index 000000000000..aefcff9c48a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/everest,es8316.txt
@@ -0,0 +1,20 @@
+Everest ES8316 audio CODEC
+
+This device supports both I2C and SPI.
+
+Required properties:
+
+  - compatible  : should be "everest,es8316"
+  - reg : the I2C address of the device for I2C
+  - clocks : a list of phandle, should contain entries for clock-names
+  - clock-names : should include as follows:
+         "mclk" : master clock (MCLK) of the device
+
+Example:
+
+es8316: codec@11 {
+	compatible = "everest,es8316";
+	reg = <0x11>;
+	clocks = <&clks 10>;
+	clock-names = "mclk";
+};
-- 
2.23.0.rc1


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

* Re: [PATCH v3 3/4] ASoC: es8316: support fixed clock rate
  2019-09-03 16:53 ` [PATCH v3 3/4] ASoC: es8316: support fixed clock rate Katsuhiro Suzuki
@ 2019-09-03 17:29   ` Mark Brown
  0 siblings, 0 replies; 13+ messages in thread
From: Mark Brown @ 2019-09-03 17:29 UTC (permalink / raw)
  To: Katsuhiro Suzuki
  Cc: David Yang, Daniel Drake, Hans de Goede, alsa-devel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On Wed, Sep 04, 2019 at 01:53:21AM +0900, Katsuhiro Suzuki wrote:
> This patch supports some type of machine drivers that use fixed clock
> rate. After applied this patch, sysclk == 0 means there is no
> constraint of sound rate and other values will set constraints which
> is derived by sysclk setting.

This is to support variable clock rate isn't it?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing
  2019-09-03 16:53 [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing Katsuhiro Suzuki
                   ` (2 preceding siblings ...)
  2019-09-03 16:53 ` [PATCH v3 4/4] ASoC: es8316: add DT-bindings Katsuhiro Suzuki
@ 2019-09-03 17:48 ` Mark Brown
  2019-09-04 15:06   ` Katsuhiro Suzuki
  3 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2019-09-03 17:48 UTC (permalink / raw)
  To: Katsuhiro Suzuki
  Cc: David Yang, Daniel Drake, Hans de Goede, alsa-devel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]

On Wed, Sep 04, 2019 at 01:53:19AM +0900, Katsuhiro Suzuki wrote:

> Root cause of this strange behavior is changing constraints list at
> set_sysclk timing. It seems that is too early to determine. So this
> patch does not use constraints list and check PCM rate limit more
> later timing at hw_params.

hw_params is a bit late to impose constraints, you want them to be
available to be available to the application before it gets as far as
picking the parameters it wants so that you don't get hw_params failing
due to an invalid configuration.  That makes everything run more
smoothly, applications should be able to trust the constraints they got
and some will not handle failures well.

The way this works with the variable MCLKs is that you end up in one of
two cases (wm8731 and wm8741 do this):

   1. The system is idle, MCLK is set to 0.  In this case no constraints
      are set and we just set MCLK to whatever is required in hw_params()
      in the machine driver.
   2. One direction is active, MCLK is set to whatever that needed.  In
      this case startup() sets constraints derived from the MCLK.

There are races in this if streams are being started and torn down
simultaneously, there's not much we can do about them with the API the
way it is so we do have to validate in hw_params() anyway but it should
be validation not constraint imposition.

If the system has a fixed MCLK it just sets that on probe then we always
get the constraints applied on startup through the same code that
handles case 2.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Applied "ASoC: es8316: add DT-bindings" to the asoc tree
  2019-09-03 16:53 ` [PATCH v3 4/4] ASoC: es8316: add DT-bindings Katsuhiro Suzuki
@ 2019-09-03 18:00   ` Mark Brown
  0 siblings, 0 replies; 13+ messages in thread
From: Mark Brown @ 2019-09-03 18:00 UTC (permalink / raw)
  To: Katsuhiro Suzuki
  Cc: alsa-devel, Daniel Drake, David Yang, Hans de Goede,
	linux-kernel, Mark Brown

The patch

   ASoC: es8316: add DT-bindings

has been applied to the asoc tree at

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

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

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

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

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

Thanks,
Mark

From 3a3edd6ffe671115c4b3d715f08ed0cf4e927ce1 Mon Sep 17 00:00:00 2001
From: Katsuhiro Suzuki <katsuhiro@katsuster.net>
Date: Wed, 4 Sep 2019 01:53:22 +0900
Subject: [PATCH] ASoC: es8316: add DT-bindings

This patch adds missing DT-bindings document for Everest ES8316.

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
Link: https://lore.kernel.org/r/20190903165322.20791-4-katsuhiro@katsuster.net
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 .../bindings/sound/everest,es8316.txt         | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/everest,es8316.txt

diff --git a/Documentation/devicetree/bindings/sound/everest,es8316.txt b/Documentation/devicetree/bindings/sound/everest,es8316.txt
new file mode 100644
index 000000000000..aefcff9c48a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/everest,es8316.txt
@@ -0,0 +1,20 @@
+Everest ES8316 audio CODEC
+
+This device supports both I2C and SPI.
+
+Required properties:
+
+  - compatible  : should be "everest,es8316"
+  - reg : the I2C address of the device for I2C
+  - clocks : a list of phandle, should contain entries for clock-names
+  - clock-names : should include as follows:
+         "mclk" : master clock (MCLK) of the device
+
+Example:
+
+es8316: codec@11 {
+	compatible = "everest,es8316";
+	reg = <0x11>;
+	clocks = <&clks 10>;
+	clock-names = "mclk";
+};
-- 
2.20.1


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

* Applied "ASoC: es8316: add clock control of MCLK" to the asoc tree
  2019-09-03 16:53 ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Katsuhiro Suzuki
@ 2019-09-03 18:00   ` Mark Brown
  2019-09-04 14:37   ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Andy Shevchenko
  1 sibling, 0 replies; 13+ messages in thread
From: Mark Brown @ 2019-09-03 18:00 UTC (permalink / raw)
  To: Katsuhiro Suzuki
  Cc: alsa-devel, Daniel Drake, David Yang, Hans de Goede,
	linux-kernel, Mark Brown

The patch

   ASoC: es8316: add clock control of MCLK

has been applied to the asoc tree at

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

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

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

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

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

Thanks,
Mark

From 6dd567dc964878c04269a44114ef13693712edf1 Mon Sep 17 00:00:00 2001
From: Katsuhiro Suzuki <katsuhiro@katsuster.net>
Date: Wed, 4 Sep 2019 01:53:20 +0900
Subject: [PATCH] ASoC: es8316: add clock control of MCLK

This patch introduce clock property for MCLK master freq control.
Driver will set rate of MCLK master if set_sysclk is called and
changing sysclk by board driver.

[Modified slightly to apply without an earlier patch in the series due
to context diffs -- broonie]

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
Link: https://lore.kernel.org/r/20190903165322.20791-2-katsuhiro@katsuster.net
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/es8316.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index 6db002cc2058..6248b01ca049 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -9,6 +9,7 @@
 
 #include <linux/module.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
@@ -33,6 +34,7 @@ static const unsigned int supported_mclk_lrck_ratios[] = {
 
 struct es8316_priv {
 	struct mutex lock;
+	struct clk *mclk;
 	struct regmap *regmap;
 	struct snd_soc_component *component;
 	struct snd_soc_jack *jack;
@@ -360,7 +362,7 @@ static int es8316_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 {
 	struct snd_soc_component *component = codec_dai->component;
 	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
-	int i;
+	int i, ret;
 	int count = 0;
 
 	es8316->sysclk = freq;
@@ -368,6 +370,12 @@ static int es8316_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	if (freq == 0)
 		return 0;
 
+	if (es8316->mclk) {
+		ret = clk_set_rate(es8316->mclk, freq);
+		if (ret)
+			return ret;
+	}
+
 	/* Limit supported sample rates to ones that can be autodetected
 	 * by the codec running in slave mode.
 	 */
@@ -697,9 +705,26 @@ static int es8316_set_jack(struct snd_soc_component *component,
 static int es8316_probe(struct snd_soc_component *component)
 {
 	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	es8316->component = component;
 
+	es8316->mclk = devm_clk_get(component->dev, "mclk");
+	if (PTR_ERR(es8316->mclk) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (IS_ERR(es8316->mclk)) {
+		dev_err(component->dev, "clock is invalid, ignored\n");
+		es8316->mclk = NULL;
+	}
+
+	if (es8316->mclk) {
+		ret = clk_prepare_enable(es8316->mclk);
+		if (ret) {
+			dev_err(component->dev, "unable to enable clock\n");
+			return ret;
+		}
+	}
+
 	/* Reset codec and enable current state machine */
 	snd_soc_component_write(component, ES8316_RESET, 0x3f);
 	usleep_range(5000, 5500);
@@ -722,8 +747,17 @@ static int es8316_probe(struct snd_soc_component *component)
 	return 0;
 }
 
+static void es8316_remove(struct snd_soc_component *component)
+{
+	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
+
+	if (es8316->mclk)
+		clk_disable_unprepare(es8316->mclk);
+}
+
 static const struct snd_soc_component_driver soc_component_dev_es8316 = {
 	.probe			= es8316_probe,
+	.remove			= es8316_remove,
 	.set_jack		= es8316_set_jack,
 	.controls		= es8316_snd_controls,
 	.num_controls		= ARRAY_SIZE(es8316_snd_controls),
-- 
2.20.1


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

* Re: [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK
  2019-09-03 16:53 ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Katsuhiro Suzuki
  2019-09-03 18:00   ` Applied "ASoC: es8316: add clock control of MCLK" to the asoc tree Mark Brown
@ 2019-09-04 14:37   ` Andy Shevchenko
  2019-09-04 15:46     ` Katsuhiro Suzuki
  1 sibling, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2019-09-04 14:37 UTC (permalink / raw)
  To: Katsuhiro Suzuki
  Cc: Mark Brown, David Yang, Daniel Drake, Hans de Goede,
	ALSA Development Mailing List, Linux Kernel Mailing List

On Tue, Sep 3, 2019 at 7:54 PM Katsuhiro Suzuki <katsuhiro@katsuster.net> wrote:
>
> This patch introduce clock property for MCLK master freq control.
> Driver will set rate of MCLK master if set_sysclk is called and
> changing sysclk by board driver.
>
> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>


> +       if (es8316->mclk) {

You don't need this if clock has been requested as optional
(clk_get_optional() or so).

> +               ret = clk_set_rate(es8316->mclk, freq);
> +               if (ret)
> +                       return ret;
> +       }

> +       es8316->mclk = devm_clk_get(component->dev, "mclk");
> +       if (PTR_ERR(es8316->mclk) == -EPROBE_DEFER)
> +               return -EPROBE_DEFER;
> +       if (IS_ERR(es8316->mclk)) {
> +               dev_err(component->dev, "clock is invalid, ignored\n");
> +               es8316->mclk = NULL;
> +       }

devm_clk_get_optional()

> +       if (es8316->mclk) {

Ditto as above.

> +               ret = clk_prepare_enable(es8316->mclk);
> +               if (ret) {
> +                       dev_err(component->dev, "unable to enable clock\n");
> +                       return ret;
> +               }
> +       }

> +       if (es8316->mclk)

Ditto.

> +               clk_disable_unprepare(es8316->mclk);
> +}


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing
  2019-09-03 17:48 ` [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing Mark Brown
@ 2019-09-04 15:06   ` Katsuhiro Suzuki
  2019-09-04 15:30     ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Katsuhiro Suzuki @ 2019-09-04 15:06 UTC (permalink / raw)
  To: Mark Brown
  Cc: David Yang, Daniel Drake, Hans de Goede, alsa-devel, linux-kernel

Hello Mark,

On 2019/09/04 2:48, Mark Brown wrote:
> On Wed, Sep 04, 2019 at 01:53:19AM +0900, Katsuhiro Suzuki wrote:
> 
>> Root cause of this strange behavior is changing constraints list at
>> set_sysclk timing. It seems that is too early to determine. So this
>> patch does not use constraints list and check PCM rate limit more
>> later timing at hw_params.
> 
> hw_params is a bit late to impose constraints, you want them to be
> available to be available to the application before it gets as far as
> picking the parameters it wants so that you don't get hw_params failing
> due to an invalid configuration.  That makes everything run more
> smoothly, applications should be able to trust the constraints they got
> and some will not handle failures well.
> 
> The way this works with the variable MCLKs is that you end up in one of
> two cases (wm8731 and wm8741 do this):
> 
>     1. The system is idle, MCLK is set to 0.  In this case no constraints
>        are set and we just set MCLK to whatever is required in hw_params()
>        in the machine driver.
>     2. One direction is active, MCLK is set to whatever that needed.  In
>        this case startup() sets constraints derived from the MCLK.
> 
> There are races in this if streams are being started and torn down
> simultaneously, there's not much we can do about them with the API the
> way it is so we do have to validate in hw_params() anyway but it should
> be validation not constraint imposition.
> 
> If the system has a fixed MCLK it just sets that on probe then we always
> get the constraints applied on startup through the same code that
> handles case 2.
> 

Thank you for explanation. I agree with apply no constraints if MCLK is
set to 0, and suitable constraints if MCLK is set to other values like
as wm8731 and wm8741 drivers.

I'll change my patch set and send.


Would you tell me one more thing. I don't understand who sets MCLK to 0.
Is it needed original machine driver instead of audio-graph-card?

On my test environment (audio-graph-card + Rockchip I2S + ES8316), it
seems audio-graph-card has never called set_sysclk() with freq = 0 after
stop play/capture sound. So my env will go to bad scenario as I 
described in this patch.

Best Regards,
Katsuhiro Suzuki

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

* Re: [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing
  2019-09-04 15:06   ` Katsuhiro Suzuki
@ 2019-09-04 15:30     ` Mark Brown
  2019-09-04 15:55       ` Katsuhiro Suzuki
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2019-09-04 15:30 UTC (permalink / raw)
  To: Katsuhiro Suzuki
  Cc: David Yang, Daniel Drake, Hans de Goede, alsa-devel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Thu, Sep 05, 2019 at 12:06:23AM +0900, Katsuhiro Suzuki wrote:

> Would you tell me one more thing. I don't understand who sets MCLK to 0.
> Is it needed original machine driver instead of audio-graph-card?

> On my test environment (audio-graph-card + Rockchip I2S + ES8316), it
> seems audio-graph-card has never called set_sysclk() with freq = 0 after
> stop play/capture sound. So my env will go to bad scenario as I described in
> this patch.

You shouldn't need a custom machine driver - you'll just be the first
person who ran into this with audio-graph-card.  I'd just add this
support to the audio-graph-card, either with custom startup and shutdown
callbacks or using a set_bias_level() callback (both get used, I'd guess
the set_bias_level() is easier since you don't need to reference count
anything).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK
  2019-09-04 14:37   ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Andy Shevchenko
@ 2019-09-04 15:46     ` Katsuhiro Suzuki
  0 siblings, 0 replies; 13+ messages in thread
From: Katsuhiro Suzuki @ 2019-09-04 15:46 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mark Brown, David Yang, Daniel Drake, Hans de Goede,
	ALSA Development Mailing List, Linux Kernel Mailing List

Hello Andy,

Thank you for reviewing.

On 2019/09/04 23:37, Andy Shevchenko wrote:
> On Tue, Sep 3, 2019 at 7:54 PM Katsuhiro Suzuki <katsuhiro@katsuster.net> wrote:
>>
>> This patch introduce clock property for MCLK master freq control.
>> Driver will set rate of MCLK master if set_sysclk is called and
>> changing sysclk by board driver.
>>
>> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
> 
> 
>> +       if (es8316->mclk) {
> 
> You don't need this if clock has been requested as optional
> (clk_get_optional() or so).
> 
>> +               ret = clk_set_rate(es8316->mclk, freq);
>> +               if (ret)
>> +                       return ret;
>> +       }
> 
>> +       es8316->mclk = devm_clk_get(component->dev, "mclk");
>> +       if (PTR_ERR(es8316->mclk) == -EPROBE_DEFER)
>> +               return -EPROBE_DEFER;
>> +       if (IS_ERR(es8316->mclk)) {
>> +               dev_err(component->dev, "clock is invalid, ignored\n");
>> +               es8316->mclk = NULL;
>> +       }
> 
> devm_clk_get_optional()
> 
>> +       if (es8316->mclk) {
> 
> Ditto as above.
> 
>> +               ret = clk_prepare_enable(es8316->mclk);
>> +               if (ret) {
>> +                       dev_err(component->dev, "unable to enable clock\n");
>> +                       return ret;
>> +               }
>> +       }
> 
>> +       if (es8316->mclk)
> 
> Ditto.
> 
>> +               clk_disable_unprepare(es8316->mclk);
>> +}
> 
> 

Indeed, NULL check of MCLK is not needed.
I'll make and send fixup patch.

Best Regards,
Katsuhiro Suzuki

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

* Re: [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing
  2019-09-04 15:30     ` Mark Brown
@ 2019-09-04 15:55       ` Katsuhiro Suzuki
  0 siblings, 0 replies; 13+ messages in thread
From: Katsuhiro Suzuki @ 2019-09-04 15:55 UTC (permalink / raw)
  To: Mark Brown
  Cc: David Yang, Daniel Drake, Hans de Goede, alsa-devel, linux-kernel

On 2019/09/05 0:30, Mark Brown wrote:
> On Thu, Sep 05, 2019 at 12:06:23AM +0900, Katsuhiro Suzuki wrote:
> 
>> Would you tell me one more thing. I don't understand who sets MCLK to 0.
>> Is it needed original machine driver instead of audio-graph-card?
> 
>> On my test environment (audio-graph-card + Rockchip I2S + ES8316), it
>> seems audio-graph-card has never called set_sysclk() with freq = 0 after
>> stop play/capture sound. So my env will go to bad scenario as I described in
>> this patch.
> 
> You shouldn't need a custom machine driver - you'll just be the first
> person who ran into this with audio-graph-card.  I'd just add this
> support to the audio-graph-card, either with custom startup and shutdown
> callbacks or using a set_bias_level() callback (both get used, I'd guess
> the set_bias_level() is easier since you don't need to reference count
> anything).
> 

Oh, I understand current situation. I'll try it.
Thanks for your support!

Best Regards,
Katsuhiro Suzuki

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

end of thread, other threads:[~2019-09-04 15:55 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-03 16:53 [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing Katsuhiro Suzuki
2019-09-03 16:53 ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Katsuhiro Suzuki
2019-09-03 18:00   ` Applied "ASoC: es8316: add clock control of MCLK" to the asoc tree Mark Brown
2019-09-04 14:37   ` [PATCH v3 2/4] ASoC: es8316: add clock control of MCLK Andy Shevchenko
2019-09-04 15:46     ` Katsuhiro Suzuki
2019-09-03 16:53 ` [PATCH v3 3/4] ASoC: es8316: support fixed clock rate Katsuhiro Suzuki
2019-09-03 17:29   ` Mark Brown
2019-09-03 16:53 ` [PATCH v3 4/4] ASoC: es8316: add DT-bindings Katsuhiro Suzuki
2019-09-03 18:00   ` Applied "ASoC: es8316: add DT-bindings" to the asoc tree Mark Brown
2019-09-03 17:48 ` [PATCH v3 1/4] ASoC: es8316: judge PCM rate at later timing Mark Brown
2019-09-04 15:06   ` Katsuhiro Suzuki
2019-09-04 15:30     ` Mark Brown
2019-09-04 15:55       ` Katsuhiro Suzuki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).