[RFC PATCH 0/2] select master clock in wm8994 driver based on DT clocks configuration

[RFC PATCH 0/2] select master clock in wm8994 driver based on DT clocks configuration

[Olivier Moysan]

Hello,

This RFC follows a previous RFC related to master clock issues with Wolfson wm8994 codec:
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-March/118834.html

This RFC provides a new proposal regarding master clock selection in wm8994 driver.
Below is a reminder of the problem:

Use case:
CPU DAI and codec are managed through audio graph card.
Wolson codec wm8994 is set as slave of CPU DAI and CPU DAI feeds codec with master clock.
Master clock is derived from mclk-fs property and provided to CPU DAI and codec through snd_soc_dai_set_sysclk API. 

Analysis:
The audio graph card sets clock id to 0, while wm8994 codec driver expects a clock id in the range [1..4]. (MCLK1, MCLK2 ..)

Proposal:
Wolfson wm8994 codec bindings exposes MCLK1 and MCLK1 clocks.
It seems that these clocks are not supported in wm8994 driver, yet.
First patch adds support of these clocks.

Second patch sets master clock according to clocks provided in DT.
The patch assumes that MCLK1 and MCLK2 are linked to aif1 and aif2 interfaces respectively.
If MCLKx is defined, is it used as source clock for aifx interface.
Otherwise clock id parameter is used as usual. 
By default clock rate is requested from clock framework. 
This is not convenient, when mclk clock frequency is computed for mclk-fs ratio,
as codec set_sysclk() is called before cpu set_sysclk() callback.
In this case frequency provided by set_sysclk() must be used.
So, if MCLKx rate is 0, frequency parameter provided by wm8994_set_dai_sysclk() is used.

I have a limited view of potential side effects here, so any comments are welcome.
If some adaptations are required to make this change more generic, please let me know.

Regards
Olivier

Olivier Moysan (2):
  ASoC: add support of mclk clock providers in wm8894 driver
  ASoC: select sysclk clock from mlck clock provider in wm8994 driver

 drivers/mfd/wm8994-core.c        |  9 +++++++++
 include/linux/mfd/wm8994/pdata.h |  6 ++++++
 sound/soc/codecs/wm8994.c        | 20 +++++++++++++++++++-
 3 files changed, 34 insertions(+), 1 deletion(-)

-- 
1.9.1

[RFC PATCH 0/2] select master clock in wm8994 driver based on DT clocks configuration

[Olivier Moysan]

Hello,

This RFC follows a previous RFC related to master clock issues with Wolfson wm8994 codec:
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-March/118834.html

This RFC provides a new proposal regarding master clock selection in wm8994 driver.
Below is a reminder of the problem:

Use case:
CPU DAI and codec are managed through audio graph card.
Wolson codec wm8994 is set as slave of CPU DAI and CPU DAI feeds codec with master clock.
Master clock is derived from mclk-fs property and provided to CPU DAI and codec through snd_soc_dai_set_sysclk API. 

Analysis:
The audio graph card sets clock id to 0, while wm8994 codec driver expects a clock id in the range [1..4]. (MCLK1, MCLK2 ..)

Proposal:
Wolfson wm8994 codec bindings exposes MCLK1 and MCLK1 clocks.
It seems that these clocks are not supported in wm8994 driver, yet.
First patch adds support of these clocks.

Second patch sets master clock according to clocks provided in DT.
The patch assumes that MCLK1 and MCLK2 are linked to aif1 and aif2 interfaces respectively.
If MCLKx is defined, is it used as source clock for aifx interface.
Otherwise clock id parameter is used as usual. 
By default clock rate is requested from clock framework. 
This is not convenient, when mclk clock frequency is computed for mclk-fs ratio,
as codec set_sysclk() is called before cpu set_sysclk() callback.
In this case frequency provided by set_sysclk() must be used.
So, if MCLKx rate is 0, frequency parameter provided by wm8994_set_dai_sysclk() is used.

I have a limited view of potential side effects here, so any comments are welcome.
If some adaptations are required to make this change more generic, please let me know.

Regards
Olivier

Olivier Moysan (2):
  ASoC: add support of mclk clock providers in wm8894 driver
  ASoC: select sysclk clock from mlck clock provider in wm8994 driver

 drivers/mfd/wm8994-core.c        |  9 +++++++++
 include/linux/mfd/wm8994/pdata.h |  6 ++++++
 sound/soc/codecs/wm8994.c        | 20 +++++++++++++++++++-
 3 files changed, 34 insertions(+), 1 deletion(-)

-- 
1.9.1

[RFC PATCH 0/2] select master clock in wm8994 driver based on DT clocks configuration

[Olivier Moysan]

Hello,

This RFC follows a previous RFC related to master clock issues with Wolfson wm8994 codec:
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-March/118834.html

This RFC provides a new proposal regarding master clock selection in wm8994 driver.
Below is a reminder of the problem:

Use case:
CPU DAI and codec are managed through audio graph card.
Wolson codec wm8994 is set as slave of CPU DAI and CPU DAI feeds codec with master clock.
Master clock is derived from mclk-fs property and provided to CPU DAI and codec through snd_soc_dai_set_sysclk API. 

Analysis:
The audio graph card sets clock id to 0, while wm8994 codec driver expects a clock id in the range [1..4]. (MCLK1, MCLK2 ..)

Proposal:
Wolfson wm8994 codec bindings exposes MCLK1 and MCLK1 clocks.
It seems that these clocks are not supported in wm8994 driver, yet.
First patch adds support of these clocks.

Second patch sets master clock according to clocks provided in DT.
The patch assumes that MCLK1 and MCLK2 are linked to aif1 and aif2 interfaces respectively.
If MCLKx is defined, is it used as source clock for aifx interface.
Otherwise clock id parameter is used as usual. 
By default clock rate is requested from clock framework. 
This is not convenient, when mclk clock frequency is computed for mclk-fs ratio,
as codec set_sysclk() is called before cpu set_sysclk() callback.
In this case frequency provided by set_sysclk() must be used.
So, if MCLKx rate is 0, frequency parameter provided by wm8994_set_dai_sysclk() is used.

I have a limited view of potential side effects here, so any comments are welcome.
If some adaptations are required to make this change more generic, please let me know.

Regards
Olivier

Olivier Moysan (2):
  ASoC: add support of mclk clock providers in wm8894 driver
  ASoC: select sysclk clock from mlck clock provider in wm8994 driver

 drivers/mfd/wm8994-core.c        |  9 +++++++++
 include/linux/mfd/wm8994/pdata.h |  6 ++++++
 sound/soc/codecs/wm8994.c        | 20 +++++++++++++++++++-
 3 files changed, 34 insertions(+), 1 deletion(-)

-- 
1.9.1

[RFC PATCH 1/2] ASoC: add support of mclk clock providers in wm8894 driver

[Olivier Moysan]

Wolfson wm8994 codec bindings exposes MCLK1 and MCLK1 clocks.
This patch adds support of MCLK1 and MCLK2 in mfd driver.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 drivers/mfd/wm8994-core.c        | 9 +++++++++
 include/linux/mfd/wm8994/pdata.h | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c
index 953d079..f1ff9d8 100644
--- a/drivers/mfd/wm8994-core.c
+++ b/drivers/mfd/wm8994-core.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include <linux/clk.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -310,6 +311,14 @@ static int wm8994_set_pdata_from_of(struct wm8994 *wm8994)
 	if (pdata->ldo[1].enable < 0)
 		pdata->ldo[1].enable = 0;
 
+	pdata->mclk1 = devm_clk_get(wm8994->dev, "MCLK1");
+	if (IS_ERR(pdata->mclk1))
+		pdata->mclk1 = NULL;
+
+	pdata->mclk2 = devm_clk_get(wm8994->dev, "MCLK2");
+	if (IS_ERR(pdata->mclk2))
+		pdata->mclk2 = NULL;
+
 	return 0;
 }
 #else
diff --git a/include/linux/mfd/wm8994/pdata.h b/include/linux/mfd/wm8994/pdata.h
index 90c6052..8037d26 100644
--- a/include/linux/mfd/wm8994/pdata.h
+++ b/include/linux/mfd/wm8994/pdata.h
@@ -233,6 +233,12 @@ struct wm8994_pdata {
 	 * GPIO for the IRQ pin if host only supports edge triggering
 	 */
 	int irq_gpio;
+
+	/* MCLK1 clock provider */
+	struct clk *mclk1;
+
+	/* MCLK2 clock provider */
+	struct clk *mclk2;
 };
 
 #endif
-- 
1.9.1

[RFC PATCH 1/2] ASoC: add support of mclk clock providers in wm8894 driver

[Olivier Moysan]

Wolfson wm8994 codec bindings exposes MCLK1 and MCLK1 clocks.
This patch adds support of MCLK1 and MCLK2 in mfd driver.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 drivers/mfd/wm8994-core.c        | 9 +++++++++
 include/linux/mfd/wm8994/pdata.h | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c
index 953d079..f1ff9d8 100644
--- a/drivers/mfd/wm8994-core.c
+++ b/drivers/mfd/wm8994-core.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include <linux/clk.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -310,6 +311,14 @@ static int wm8994_set_pdata_from_of(struct wm8994 *wm8994)
 	if (pdata->ldo[1].enable < 0)
 		pdata->ldo[1].enable = 0;
 
+	pdata->mclk1 = devm_clk_get(wm8994->dev, "MCLK1");
+	if (IS_ERR(pdata->mclk1))
+		pdata->mclk1 = NULL;
+
+	pdata->mclk2 = devm_clk_get(wm8994->dev, "MCLK2");
+	if (IS_ERR(pdata->mclk2))
+		pdata->mclk2 = NULL;
+
 	return 0;
 }
 #else
diff --git a/include/linux/mfd/wm8994/pdata.h b/include/linux/mfd/wm8994/pdata.h
index 90c6052..8037d26 100644
--- a/include/linux/mfd/wm8994/pdata.h
+++ b/include/linux/mfd/wm8994/pdata.h
@@ -233,6 +233,12 @@ struct wm8994_pdata {
 	 * GPIO for the IRQ pin if host only supports edge triggering
 	 */
 	int irq_gpio;
+
+	/* MCLK1 clock provider */
+	struct clk *mclk1;
+
+	/* MCLK2 clock provider */
+	struct clk *mclk2;
 };
 
 #endif
-- 
1.9.1

[RFC PATCH 1/2] ASoC: add support of mclk clock providers in wm8894 driver

[Olivier Moysan]

Wolfson wm8994 codec bindings exposes MCLK1 and MCLK1 clocks.
This patch adds support of MCLK1 and MCLK2 in mfd driver.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 drivers/mfd/wm8994-core.c        | 9 +++++++++
 include/linux/mfd/wm8994/pdata.h | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c
index 953d079..f1ff9d8 100644
--- a/drivers/mfd/wm8994-core.c
+++ b/drivers/mfd/wm8994-core.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include <linux/clk.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -310,6 +311,14 @@ static int wm8994_set_pdata_from_of(struct wm8994 *wm8994)
 	if (pdata->ldo[1].enable < 0)
 		pdata->ldo[1].enable = 0;
 
+	pdata->mclk1 = devm_clk_get(wm8994->dev, "MCLK1");
+	if (IS_ERR(pdata->mclk1))
+		pdata->mclk1 = NULL;
+
+	pdata->mclk2 = devm_clk_get(wm8994->dev, "MCLK2");
+	if (IS_ERR(pdata->mclk2))
+		pdata->mclk2 = NULL;
+
 	return 0;
 }
 #else
diff --git a/include/linux/mfd/wm8994/pdata.h b/include/linux/mfd/wm8994/pdata.h
index 90c6052..8037d26 100644
--- a/include/linux/mfd/wm8994/pdata.h
+++ b/include/linux/mfd/wm8994/pdata.h
@@ -233,6 +233,12 @@ struct wm8994_pdata {
 	 * GPIO for the IRQ pin if host only supports edge triggering
 	 */
 	int irq_gpio;
+
+	/* MCLK1 clock provider */
+	struct clk *mclk1;
+
+	/* MCLK2 clock provider */
+	struct clk *mclk2;
 };
 
 #endif
-- 
1.9.1

[RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier Moysan]

When defined in device tree, MCLK1 and MCLK2 are used
as sysclk for aif1 and aif2 interfaces respectively.
If clock rate is let 0, the frequency provided by
wm8994_set_dai_sysclk() is used instead.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 sound/soc/codecs/wm8994.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
index 21ffd64..7a84e37 100644
--- a/sound/soc/codecs/wm8994.c
+++ b/sound/soc/codecs/wm8994.c
@@ -11,6 +11,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
@@ -2376,18 +2377,35 @@ static int wm8994_set_dai_sysclk(struct snd_soc_dai *dai,
 {
 	struct snd_soc_codec *codec = dai->codec;
 	struct wm8994_priv *wm8994 = snd_soc_codec_get_drvdata(codec);
+	struct wm8994 *control = wm8994->wm8994;
+	struct wm8994_pdata *pdata = &control->pdata;
+	unsigned long rate;
 	int i;
 
 	switch (dai->id) {
 	case 1:
+		if (pdata->mclk1) {
+			rate = clk_get_rate(pdata->mclk1);
+			if (rate)
+				freq = (unsigned int)rate;
+			clk_id = WM8994_SYSCLK_MCLK1;
+		}
+		break;
 	case 2:
+		if (pdata->mclk2) {
+			rate = clk_get_rate(pdata->mclk2);
+			if (rate)
+				freq = (unsigned int)rate;
+			clk_id = WM8994_SYSCLK_MCLK2;
+		}
 		break;
-
 	default:
 		/* AIF3 shares clocking with AIF1/2 */
 		return -EINVAL;
 	}
 
+	dev_info(codec->dev, "%s:.clock id %d\n", __func__, clk_id);
+
 	switch (clk_id) {
 	case WM8994_SYSCLK_MCLK1:
 		wm8994->sysclk[dai->id - 1] = WM8994_SYSCLK_MCLK1;
-- 
1.9.1

[RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier Moysan]

When defined in device tree, MCLK1 and MCLK2 are used
as sysclk for aif1 and aif2 interfaces respectively.
If clock rate is let 0, the frequency provided by
wm8994_set_dai_sysclk() is used instead.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 sound/soc/codecs/wm8994.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
index 21ffd64..7a84e37 100644
--- a/sound/soc/codecs/wm8994.c
+++ b/sound/soc/codecs/wm8994.c
@@ -11,6 +11,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
@@ -2376,18 +2377,35 @@ static int wm8994_set_dai_sysclk(struct snd_soc_dai *dai,
 {
 	struct snd_soc_codec *codec = dai->codec;
 	struct wm8994_priv *wm8994 = snd_soc_codec_get_drvdata(codec);
+	struct wm8994 *control = wm8994->wm8994;
+	struct wm8994_pdata *pdata = &control->pdata;
+	unsigned long rate;
 	int i;
 
 	switch (dai->id) {
 	case 1:
+		if (pdata->mclk1) {
+			rate = clk_get_rate(pdata->mclk1);
+			if (rate)
+				freq = (unsigned int)rate;
+			clk_id = WM8994_SYSCLK_MCLK1;
+		}
+		break;
 	case 2:
+		if (pdata->mclk2) {
+			rate = clk_get_rate(pdata->mclk2);
+			if (rate)
+				freq = (unsigned int)rate;
+			clk_id = WM8994_SYSCLK_MCLK2;
+		}
 		break;
-
 	default:
 		/* AIF3 shares clocking with AIF1/2 */
 		return -EINVAL;
 	}
 
+	dev_info(codec->dev, "%s:.clock id %d\n", __func__, clk_id);
+
 	switch (clk_id) {
 	case WM8994_SYSCLK_MCLK1:
 		wm8994->sysclk[dai->id - 1] = WM8994_SYSCLK_MCLK1;
-- 
1.9.1

[RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier Moysan]

When defined in device tree, MCLK1 and MCLK2 are used
as sysclk for aif1 and aif2 interfaces respectively.
If clock rate is let 0, the frequency provided by
wm8994_set_dai_sysclk() is used instead.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 sound/soc/codecs/wm8994.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
index 21ffd64..7a84e37 100644
--- a/sound/soc/codecs/wm8994.c
+++ b/sound/soc/codecs/wm8994.c
@@ -11,6 +11,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
@@ -2376,18 +2377,35 @@ static int wm8994_set_dai_sysclk(struct snd_soc_dai *dai,
 {
 	struct snd_soc_codec *codec = dai->codec;
 	struct wm8994_priv *wm8994 = snd_soc_codec_get_drvdata(codec);
+	struct wm8994 *control = wm8994->wm8994;
+	struct wm8994_pdata *pdata = &control->pdata;
+	unsigned long rate;
 	int i;
 
 	switch (dai->id) {
 	case 1:
+		if (pdata->mclk1) {
+			rate = clk_get_rate(pdata->mclk1);
+			if (rate)
+				freq = (unsigned int)rate;
+			clk_id = WM8994_SYSCLK_MCLK1;
+		}
+		break;
 	case 2:
+		if (pdata->mclk2) {
+			rate = clk_get_rate(pdata->mclk2);
+			if (rate)
+				freq = (unsigned int)rate;
+			clk_id = WM8994_SYSCLK_MCLK2;
+		}
 		break;
-
 	default:
 		/* AIF3 shares clocking with AIF1/2 */
 		return -EINVAL;
 	}
 
+	dev_info(codec->dev, "%s:.clock id %d\n", __func__, clk_id);
+
 	switch (clk_id) {
 	case WM8994_SYSCLK_MCLK1:
 		wm8994->sysclk[dai->id - 1] = WM8994_SYSCLK_MCLK1;
-- 
1.9.1

Re: [RFC PATCH 1/2] ASoC: add support of mclk clock providers in wm8894 driver

[Mark Brown]

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

On Thu, Dec 14, 2017 at 05:53:57PM +0100, Olivier Moysan wrote:

> +	pdata->mclk1 = devm_clk_get(wm8994->dev, "MCLK1");
> +	if (IS_ERR(pdata->mclk1))
> +		pdata->mclk1 = NULL;

These should special case -EPROBE_DEFER so we defer properly if we need
to (and ideally log an error in case there was a MCLK and we legit ran
into an error).

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

Re: [RFC PATCH 1/2] ASoC: add support of mclk clock providers in wm8894 driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 336 bytes --]

On Thu, Dec 14, 2017 at 05:53:57PM +0100, Olivier Moysan wrote:

> +	pdata->mclk1 = devm_clk_get(wm8994->dev, "MCLK1");
> +	if (IS_ERR(pdata->mclk1))
> +		pdata->mclk1 = NULL;

These should special case -EPROBE_DEFER so we defer properly if we need
to (and ideally log an error in case there was a MCLK and we legit ran
into an error).

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Mark Brown]

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

On Thu, Dec 14, 2017 at 05:53:58PM +0100, Olivier Moysan wrote:
> When defined in device tree, MCLK1 and MCLK2 are used
> as sysclk for aif1 and aif2 interfaces respectively.

That's not a valid assumption as far as I remember?  The AIFs can use
either MCLK depending on the system configuration I think.

> If clock rate is let 0, the frequency provided by
> wm8994_set_dai_sysclk() is used instead.

I'd expect this the other way around, if we didn't specify a frequency
then read it from the input otherwise try to use clk_set_rate() to
propagate things up.

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

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 561 bytes --]

On Thu, Dec 14, 2017 at 05:53:58PM +0100, Olivier Moysan wrote:
> When defined in device tree, MCLK1 and MCLK2 are used
> as sysclk for aif1 and aif2 interfaces respectively.

That's not a valid assumption as far as I remember?  The AIFs can use
either MCLK depending on the system configuration I think.

> If clock rate is let 0, the frequency provided by
> wm8994_set_dai_sysclk() is used instead.

I'd expect this the other way around, if we didn't specify a frequency
then read it from the input otherwise try to use clk_set_rate() to
propagate things up.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Charles Keepax]

On Thu, Dec 14, 2017 at 05:36:24PM +0000, Mark Brown wrote:
> On Thu, Dec 14, 2017 at 05:53:58PM +0100, Olivier Moysan wrote:
> > When defined in device tree, MCLK1 and MCLK2 are used
> > as sysclk for aif1 and aif2 interfaces respectively.
> 
> That's not a valid assumption as far as I remember?  The AIFs can use
> either MCLK depending on the system configuration I think.
> 

Indeed yes the code added here is literally just above the code
that selects that, it should be fairly simple to update the patch
to take this into account.

Thanks,
Charles

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Charles Keepax]

On Thu, Dec 14, 2017 at 05:36:24PM +0000, Mark Brown wrote:
> On Thu, Dec 14, 2017 at 05:53:58PM +0100, Olivier Moysan wrote:
> > When defined in device tree, MCLK1 and MCLK2 are used
> > as sysclk for aif1 and aif2 interfaces respectively.
> 
> That's not a valid assumption as far as I remember?  The AIFs can use
> either MCLK depending on the system configuration I think.
> 

Indeed yes the code added here is literally just above the code
that selects that, it should be fairly simple to update the patch
to take this into account.

Thanks,
Charles

[RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Charles Keepax]

On Thu, Dec 14, 2017 at 05:36:24PM +0000, Mark Brown wrote:
> On Thu, Dec 14, 2017 at 05:53:58PM +0100, Olivier Moysan wrote:
> > When defined in device tree, MCLK1 and MCLK2 are used
> > as sysclk for aif1 and aif2 interfaces respectively.
> 
> That's not a valid assumption as far as I remember?  The AIFs can use
> either MCLK depending on the system configuration I think.
> 

Indeed yes the code added here is literally just above the code
that selects that, it should be fairly simple to update the patch
to take this into account.

Thanks,
Charles

Re: [RFC PATCH 1/2] ASoC: add support of mclk clock providers in wm8894 driver

[Charles Keepax]

On Thu, Dec 14, 2017 at 05:30:25PM +0000, Mark Brown wrote:
> On Thu, Dec 14, 2017 at 05:53:57PM +0100, Olivier Moysan wrote:
> 
> > +	pdata->mclk1 = devm_clk_get(wm8994->dev, "MCLK1");
> > +	if (IS_ERR(pdata->mclk1))
> > +		pdata->mclk1 = NULL;
> 
> These should special case -EPROBE_DEFER so we defer properly if we need
> to (and ideally log an error in case there was a MCLK and we legit ran
> into an error).

We probably want the special case on there being no clock at all
which should silently proceed as this code does and then actual
errors and PROBE_DEFERs can log and fail.

Thanks,
Charles

Re: [RFC PATCH 1/2] ASoC: add support of mclk clock providers in wm8894 driver

[Charles Keepax]

On Thu, Dec 14, 2017 at 05:30:25PM +0000, Mark Brown wrote:
> On Thu, Dec 14, 2017 at 05:53:57PM +0100, Olivier Moysan wrote:
> 
> > +	pdata->mclk1 = devm_clk_get(wm8994->dev, "MCLK1");
> > +	if (IS_ERR(pdata->mclk1))
> > +		pdata->mclk1 = NULL;
> 
> These should special case -EPROBE_DEFER so we defer properly if we need
> to (and ideally log an error in case there was a MCLK and we legit ran
> into an error).

We probably want the special case on there being no clock at all
which should silently proceed as this code does and then actual
errors and PROBE_DEFERs can log and fail.

Thanks,
Charles

[RFC PATCH 1/2] ASoC: add support of mclk clock providers in wm8894 driver

[Charles Keepax]

On Thu, Dec 14, 2017 at 05:30:25PM +0000, Mark Brown wrote:
> On Thu, Dec 14, 2017 at 05:53:57PM +0100, Olivier Moysan wrote:
> 
> > +	pdata->mclk1 = devm_clk_get(wm8994->dev, "MCLK1");
> > +	if (IS_ERR(pdata->mclk1))
> > +		pdata->mclk1 = NULL;
> 
> These should special case -EPROBE_DEFER so we defer properly if we need
> to (and ideally log an error in case there was a MCLK and we legit ran
> into an error).

We probably want the special case on there being no clock at all
which should silently proceed as this code does and then actual
errors and PROBE_DEFERs can log and fail.

Thanks,
Charles

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier MOYSAN]

Hello Mark,

Thanks for your comment.

On 12/14/2017 06:36 PM, Mark Brown wrote:
> On Thu, Dec 14, 2017 at 05:53:58PM +0100, Olivier Moysan wrote:
>> When defined in device tree, MCLK1 and MCLK2 are used
>> as sysclk for aif1 and aif2 interfaces respectively.
> 
> That's not a valid assumption as far as I remember?  The AIFs can use
> either MCLK depending on the system configuration I think.
> 

You are right. wm8994 allows to select either MCLK for each AIF.
 From this point of view, the current patch is too limiting.
MCLK information in DT allows to enforce MCLK use, but an additionnal 
information is required to determine AIF MCLK assignment.
Available properties in codec DAI node, such as clocks property, cannot 
help here.

Maybe a DAPM linked to a control is a better way to select AIF source,
When source is not provided by clk_id in wm8994_set_dai_sysclk().
In this case, wm8994_set_dai_sysclk() would only have to check
if clock source is not already set.

Please let me know if this option sounds better to you.

>> If clock rate is let 0, the frequency provided by
>> wm8994_set_dai_sysclk() is used instead.
> 
> I'd expect this the other way around, if we didn't specify a frequency
> then read it from the input otherwise try to use clk_set_rate() to
> propagate things up.
> 

If I implement a control to select the AIF source, I will drop the code
related to mclk clock provider.

Regards
Olivier

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier MOYSAN]

Hello Mark,

Thanks for your comment.

On 12/14/2017 06:36 PM, Mark Brown wrote:
> On Thu, Dec 14, 2017 at 05:53:58PM +0100, Olivier Moysan wrote:
>> When defined in device tree, MCLK1 and MCLK2 are used
>> as sysclk for aif1 and aif2 interfaces respectively.
> 
> That's not a valid assumption as far as I remember?  The AIFs can use
> either MCLK depending on the system configuration I think.
> 

You are right. wm8994 allows to select either MCLK for each AIF.
 From this point of view, the current patch is too limiting.
MCLK information in DT allows to enforce MCLK use, but an additionnal 
information is required to determine AIF MCLK assignment.
Available properties in codec DAI node, such as clocks property, cannot 
help here.

Maybe a DAPM linked to a control is a better way to select AIF source,
When source is not provided by clk_id in wm8994_set_dai_sysclk().
In this case, wm8994_set_dai_sysclk() would only have to check
if clock source is not already set.

Please let me know if this option sounds better to you.

>> If clock rate is let 0, the frequency provided by
>> wm8994_set_dai_sysclk() is used instead.
> 
> I'd expect this the other way around, if we didn't specify a frequency
> then read it from the input otherwise try to use clk_set_rate() to
> propagate things up.
> 

If I implement a control to select the AIF source, I will drop the code
related to mclk clock provider.

Regards
Olivier

[RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier MOYSAN]

Hello Mark,

Thanks for your comment.

On 12/14/2017 06:36 PM, Mark Brown wrote:
> On Thu, Dec 14, 2017 at 05:53:58PM +0100, Olivier Moysan wrote:
>> When defined in device tree, MCLK1 and MCLK2 are used
>> as sysclk for aif1 and aif2 interfaces respectively.
> 
> That's not a valid assumption as far as I remember?  The AIFs can use
> either MCLK depending on the system configuration I think.
> 

You are right. wm8994 allows to select either MCLK for each AIF.
 From this point of view, the current patch is too limiting.
MCLK information in DT allows to enforce MCLK use, but an additionnal 
information is required to determine AIF MCLK assignment.
Available properties in codec DAI node, such as clocks property, cannot 
help here.

Maybe a DAPM linked to a control is a better way to select AIF source,
When source is not provided by clk_id in wm8994_set_dai_sysclk().
In this case, wm8994_set_dai_sysclk() would only have to check
if clock source is not already set.

Please let me know if this option sounds better to you.

>> If clock rate is let 0, the frequency provided by
>> wm8994_set_dai_sysclk() is used instead.
> 
> I'd expect this the other way around, if we didn't specify a frequency
> then read it from the input otherwise try to use clk_set_rate() to
> propagate things up.
> 

If I implement a control to select the AIF source, I will drop the code
related to mclk clock provider.

Regards
Olivier

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Mark Brown]

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

On Fri, Dec 15, 2017 at 03:15:22PM +0000, Olivier MOYSAN wrote:

> You are right. wm8994 allows to select either MCLK for each AIF.
>  From this point of view, the current patch is too limiting.
> MCLK information in DT allows to enforce MCLK use, but an additionnal 
> information is required to determine AIF MCLK assignment.
> Available properties in codec DAI node, such as clocks property, cannot 
> help here.

> Maybe a DAPM linked to a control is a better way to select AIF source,
> When source is not provided by clk_id in wm8994_set_dai_sysclk().
> In this case, wm8994_set_dai_sysclk() would only have to check
> if clock source is not already set.

> Please let me know if this option sounds better to you.

What are you trying to accomplish here?  You appear to be trying to move
the system clocking configuration from the machine driver to the CODEC
which is not how things are supposed to work.

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

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 930 bytes --]

On Fri, Dec 15, 2017 at 03:15:22PM +0000, Olivier MOYSAN wrote:

> You are right. wm8994 allows to select either MCLK for each AIF.
>  From this point of view, the current patch is too limiting.
> MCLK information in DT allows to enforce MCLK use, but an additionnal 
> information is required to determine AIF MCLK assignment.
> Available properties in codec DAI node, such as clocks property, cannot 
> help here.

> Maybe a DAPM linked to a control is a better way to select AIF source,
> When source is not provided by clk_id in wm8994_set_dai_sysclk().
> In this case, wm8994_set_dai_sysclk() would only have to check
> if clock source is not already set.

> Please let me know if this option sounds better to you.

What are you trying to accomplish here?  You appear to be trying to move
the system clocking configuration from the machine driver to the CODEC
which is not how things are supposed to work.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Mark Brown]

On Fri, Dec 15, 2017 at 03:15:22PM +0000, Olivier MOYSAN wrote:

> You are right. wm8994 allows to select either MCLK for each AIF.
>  From this point of view, the current patch is too limiting.
> MCLK information in DT allows to enforce MCLK use, but an additionnal 
> information is required to determine AIF MCLK assignment.
> Available properties in codec DAI node, such as clocks property, cannot 
> help here.

> Maybe a DAPM linked to a control is a better way to select AIF source,
> When source is not provided by clk_id in wm8994_set_dai_sysclk().
> In this case, wm8994_set_dai_sysclk() would only have to check
> if clock source is not already set.

> Please let me know if this option sounds better to you.

What are you trying to accomplish here?  You appear to be trying to move
the system clocking configuration from the machine driver to the CODEC
which is not how things are supposed to work.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 484 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171219/7608e3f2/attachment.sig>

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier MOYSAN]

Hello Mark,

On 12/19/2017 10:35 AM, Mark Brown wrote:
> On Fri, Dec 15, 2017 at 03:15:22PM +0000, Olivier MOYSAN wrote:
> 
>> You are right. wm8994 allows to select either MCLK for each AIF.
>>   From this point of view, the current patch is too limiting.
>> MCLK information in DT allows to enforce MCLK use, but an additionnal
>> information is required to determine AIF MCLK assignment.
>> Available properties in codec DAI node, such as clocks property, cannot
>> help here.
> 
>> Maybe a DAPM linked to a control is a better way to select AIF source,
>> When source is not provided by clk_id in wm8994_set_dai_sysclk().
>> In this case, wm8994_set_dai_sysclk() would only have to check
>> if clock source is not already set.
> 
>> Please let me know if this option sounds better to you.
> 
> What are you trying to accomplish here?  You appear to be trying to move
> the system clocking configuration from the machine driver to the CODEC
> which is not how things are supposed to work.
> 

As a generic machine, simple or audio graph cards are not able to manage 
codec clock muxing.
If we exclude the management of muxing through codec controls,
the remaining solution is to handle it fully through clock framework.
The current patch only supports a limited range of muxing capabilities 
of the codec.
To have a full management of the muxing, I think it is necessary to add
a device tree node for each codec interface and to define an aif clock 
in these nodes.
Then parent clock assignment of these aif clocks would allow to handle 
the muxing.

Please, let me known if is this the right direction for you.

BRs
olivier

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier MOYSAN]

Hello Mark,

On 12/19/2017 10:35 AM, Mark Brown wrote:
> On Fri, Dec 15, 2017 at 03:15:22PM +0000, Olivier MOYSAN wrote:
> 
>> You are right. wm8994 allows to select either MCLK for each AIF.
>>   From this point of view, the current patch is too limiting.
>> MCLK information in DT allows to enforce MCLK use, but an additionnal
>> information is required to determine AIF MCLK assignment.
>> Available properties in codec DAI node, such as clocks property, cannot
>> help here.
> 
>> Maybe a DAPM linked to a control is a better way to select AIF source,
>> When source is not provided by clk_id in wm8994_set_dai_sysclk().
>> In this case, wm8994_set_dai_sysclk() would only have to check
>> if clock source is not already set.
> 
>> Please let me know if this option sounds better to you.
> 
> What are you trying to accomplish here?  You appear to be trying to move
> the system clocking configuration from the machine driver to the CODEC
> which is not how things are supposed to work.
> 

As a generic machine, simple or audio graph cards are not able to manage 
codec clock muxing.
If we exclude the management of muxing through codec controls,
the remaining solution is to handle it fully through clock framework.
The current patch only supports a limited range of muxing capabilities 
of the codec.
To have a full management of the muxing, I think it is necessary to add
a device tree node for each codec interface and to define an aif clock 
in these nodes.
Then parent clock assignment of these aif clocks would allow to handle 
the muxing.

Please, let me known if is this the right direction for you.

BRs
olivier

[RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier MOYSAN]

Hello Mark,

On 12/19/2017 10:35 AM, Mark Brown wrote:
> On Fri, Dec 15, 2017 at 03:15:22PM +0000, Olivier MOYSAN wrote:
> 
>> You are right. wm8994 allows to select either MCLK for each AIF.
>>   From this point of view, the current patch is too limiting.
>> MCLK information in DT allows to enforce MCLK use, but an additionnal
>> information is required to determine AIF MCLK assignment.
>> Available properties in codec DAI node, such as clocks property, cannot
>> help here.
> 
>> Maybe a DAPM linked to a control is a better way to select AIF source,
>> When source is not provided by clk_id in wm8994_set_dai_sysclk().
>> In this case, wm8994_set_dai_sysclk() would only have to check
>> if clock source is not already set.
> 
>> Please let me know if this option sounds better to you.
> 
> What are you trying to accomplish here?  You appear to be trying to move
> the system clocking configuration from the machine driver to the CODEC
> which is not how things are supposed to work.
> 

As a generic machine, simple or audio graph cards are not able to manage 
codec clock muxing.
If we exclude the management of muxing through codec controls,
the remaining solution is to handle it fully through clock framework.
The current patch only supports a limited range of muxing capabilities 
of the codec.
To have a full management of the muxing, I think it is necessary to add
a device tree node for each codec interface and to define an aif clock 
in these nodes.
Then parent clock assignment of these aif clocks would allow to handle 
the muxing.

Please, let me known if is this the right direction for you.

BRs
olivier

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Mark Brown]

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

On Wed, Dec 20, 2017 at 12:42:10PM +0000, Olivier MOYSAN wrote:

> As a generic machine, simple or audio graph cards are not able to manage 
> codec clock muxing.
> If we exclude the management of muxing through codec controls,
> the remaining solution is to handle it fully through clock framework.
> The current patch only supports a limited range of muxing capabilities 
> of the codec.
> To have a full management of the muxing, I think it is necessary to add
> a device tree node for each codec interface and to define an aif clock 
> in these nodes.
> Then parent clock assignment of these aif clocks would allow to handle 
> the muxing.

Controlling clocking through a clock API binding would be good, yes.
That'd solve a bunch of other problems with use of multi-purpose clocks
for audio as well.

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

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 822 bytes --]

On Wed, Dec 20, 2017 at 12:42:10PM +0000, Olivier MOYSAN wrote:

> As a generic machine, simple or audio graph cards are not able to manage 
> codec clock muxing.
> If we exclude the management of muxing through codec controls,
> the remaining solution is to handle it fully through clock framework.
> The current patch only supports a limited range of muxing capabilities 
> of the codec.
> To have a full management of the muxing, I think it is necessary to add
> a device tree node for each codec interface and to define an aif clock 
> in these nodes.
> Then parent clock assignment of these aif clocks would allow to handle 
> the muxing.

Controlling clocking through a clock API binding would be good, yes.
That'd solve a bunch of other problems with use of multi-purpose clocks
for audio as well.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier MOYSAN]

Hello Mark,

On 12/20/2017 04:50 PM, Mark Brown wrote:
> On Wed, Dec 20, 2017 at 12:42:10PM +0000, Olivier MOYSAN wrote:
> 
>> As a generic machine, simple or audio graph cards are not able to manage
>> codec clock muxing.
>> If we exclude the management of muxing through codec controls,
>> the remaining solution is to handle it fully through clock framework.
>> The current patch only supports a limited range of muxing capabilities
>> of the codec.
>> To have a full management of the muxing, I think it is necessary to add
>> a device tree node for each codec interface and to define an aif clock
>> in these nodes.
>> Then parent clock assignment of these aif clocks would allow to handle
>> the muxing.
> 
> Controlling clocking through a clock API binding would be good, yes.
> That'd solve a bunch of other problems with use of multi-purpose clocks
> for audio as well.
> 

Thanks for your feedback. I will implement this in a patch v2.

BRs

Re: [RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier MOYSAN]

Hello Mark,

On 12/20/2017 04:50 PM, Mark Brown wrote:
> On Wed, Dec 20, 2017 at 12:42:10PM +0000, Olivier MOYSAN wrote:
> 
>> As a generic machine, simple or audio graph cards are not able to manage
>> codec clock muxing.
>> If we exclude the management of muxing through codec controls,
>> the remaining solution is to handle it fully through clock framework.
>> The current patch only supports a limited range of muxing capabilities
>> of the codec.
>> To have a full management of the muxing, I think it is necessary to add
>> a device tree node for each codec interface and to define an aif clock
>> in these nodes.
>> Then parent clock assignment of these aif clocks would allow to handle
>> the muxing.
> 
> Controlling clocking through a clock API binding would be good, yes.
> That'd solve a bunch of other problems with use of multi-purpose clocks
> for audio as well.
> 

Thanks for your feedback. I will implement this in a patch v2.

BRs

[RFC PATCH 2/2] ASoC: select sysclk clock from mlck clock provider in wm8994 driver

[Olivier MOYSAN]

Hello Mark,

On 12/20/2017 04:50 PM, Mark Brown wrote:
> On Wed, Dec 20, 2017 at 12:42:10PM +0000, Olivier MOYSAN wrote:
> 
>> As a generic machine, simple or audio graph cards are not able to manage
>> codec clock muxing.
>> If we exclude the management of muxing through codec controls,
>> the remaining solution is to handle it fully through clock framework.
>> The current patch only supports a limited range of muxing capabilities
>> of the codec.
>> To have a full management of the muxing, I think it is necessary to add
>> a device tree node for each codec interface and to define an aif clock
>> in these nodes.
>> Then parent clock assignment of these aif clocks would allow to handle
>> the muxing.
> 
> Controlling clocking through a clock API binding would be good, yes.
> That'd solve a bunch of other problems with use of multi-purpose clocks
> for audio as well.
> 

Thanks for your feedback. I will implement this in a patch v2.

BRs