[PATCHv2 0/6] ASoC: da7213: support for usage with simple-card

[PATCHv2 0/6] ASoC: da7213: support for usage with simple-card

[Sebastian Reichel]

Hi,

This extends the da7213 driver to be used with simple-audio-card in
combination with a fixed clock. Here is a snippet of the downstream
board's DT, that is supposed to be supported by this patchset.

---------------------------------------------------------------------
/ {
	sound {
		compatible = "simple-audio-card";
		simple-audio-card,name = "audio-card";
		simple-audio-card,format = "i2s";
		simple-audio-card,bitclock-master = <&dailink_master>;
		simple-audio-card,frame-master = <&dailink_master>;

		simple-audio-card,widgets = "Speaker", "Ext Spk";
		simple-audio-card,audio-routing = "Ext Spk", "LINE";

		simple-audio-card,cpu {
			sound-dai = <&ssi1>;
		};

		dailink_master: simple-audio-card,codec {
			sound-dai = <&codec>;
		};
	};


	clk_ext_audio_codec: clock-codec {
		compatible = "fixed-clock";
		#clock-cells = <0>;
		clock-frequency = <12288000>;
	};
};

&i2c1 {
	codec: audio-codec@1a {
		compatible = "dlg,da7212";
		reg = <0x1a>;
		#sound-dai-cells = <0>;
		VDDA-supply = <&reg_2v5_audio>;
		VDDSP-supply = <&reg_5v0_audio>;
		VDDMIC-supply = <&reg_3v3_audio>;
		VDDIO-supply = <&reg_3v3_audio>;
		clocks = <&clk_ext_audio_codec>;
		clock-names = "mclk";
	};
};
---------------------------------------------------------------------

Changes since PATCHv1:
 * https://lore.kernel.org/alsa-devel/20191108174843.11227-1-sebastian.reichel@collabora.com/
 * add patch adding da7212 compatible to DT bindings
 * update regulator patch, so that VDDA is enabled together with VDDIO
   while the device is enabled to avoid device reset
 * update clock patch, so that automatic PLL handling is not enabled
   when PLL is configured manually
 * update clock patch, so that automatic PLL is disabled when the device
   is suspended
 * update clock patch, so that automatic PLL is configured into bypass
   mode when possible

-- Sebastian

Sebastian Reichel (6):
  ASoC: da7213: Add da7212 DT compatible
  ASoC: da7213: Add regulator support
  ASoC: da7213: Provide selectable option
  ASoC: da7213: Move set_sysclk to codec level
  ASoC: da7213: Move set_pll to codec level
  ASoC: da7213: Add default clock handling

 .../devicetree/bindings/sound/da7213.txt      |   8 +-
 sound/soc/codecs/Kconfig                      |   3 +-
 sound/soc/codecs/da7213.c                     | 171 ++++++++++++++++--
 sound/soc/codecs/da7213.h                     |  11 ++
 4 files changed, 177 insertions(+), 16 deletions(-)

-- 
2.24.0

[PATCHv2 1/6] ASoC: da7213: Add da7212 DT compatible

[Sebastian Reichel]

This adds a compatible for da7212. It's handled exactly the
same way as DA7213 and follows the ACPI bindings.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 Documentation/devicetree/bindings/sound/da7213.txt | 4 ++--
 sound/soc/codecs/da7213.c                          | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt
index 58902802d56c..759bb04e0283 100644
--- a/Documentation/devicetree/bindings/sound/da7213.txt
+++ b/Documentation/devicetree/bindings/sound/da7213.txt
@@ -1,9 +1,9 @@
-Dialog Semiconductor DA7213 Audio Codec bindings
+Dialog Semiconductor DA7212/DA7213 Audio Codec bindings
 
 ======
 
 Required properties:
-- compatible : Should be "dlg,da7213"
+- compatible : Should be "dlg,da7212" or "dlg,7213"
 - reg: Specifies the I2C slave address
 
 Optional properties:
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index 925a03996db4..aff306bb58df 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1571,6 +1571,7 @@ static int da7213_set_bias_level(struct snd_soc_component *component,
 #if defined(CONFIG_OF)
 /* DT */
 static const struct of_device_id da7213_of_match[] = {
+	{ .compatible = "dlg,da7212", },
 	{ .compatible = "dlg,da7213", },
 	{ }
 };
-- 
2.24.0

RE: [PATCHv2 1/6] ASoC: da7213: Add da7212 DT compatible

[Adam Thomson]

On 20 November 2019 15:24, Sebastian Reichel wrote: 

> This adds a compatible for da7212. It's handled exactly the
> same way as DA7213 and follows the ACPI bindings.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  Documentation/devicetree/bindings/sound/da7213.txt | 4 ++--
>  sound/soc/codecs/da7213.c                          | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/da7213.txt
> b/Documentation/devicetree/bindings/sound/da7213.txt
> index 58902802d56c..759bb04e0283 100644
> --- a/Documentation/devicetree/bindings/sound/da7213.txt
> +++ b/Documentation/devicetree/bindings/sound/da7213.txt
> @@ -1,9 +1,9 @@
> -Dialog Semiconductor DA7213 Audio Codec bindings
> +Dialog Semiconductor DA7212/DA7213 Audio Codec bindings
> 
>  ======
> 
>  Required properties:
> -- compatible : Should be "dlg,da7213"
> +- compatible : Should be "dlg,da7212" or "dlg,7213"

Typo? "dlg,da7213"

>  - reg: Specifies the I2C slave address
> 
>  Optional properties:
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 925a03996db4..aff306bb58df 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1571,6 +1571,7 @@ static int da7213_set_bias_level(struct
> snd_soc_component *component,
>  #if defined(CONFIG_OF)
>  /* DT */
>  static const struct of_device_id da7213_of_match[] = {
> +	{ .compatible = "dlg,da7212", },
>  	{ .compatible = "dlg,da7213", },
>  	{ }
>  };
> --
> 2.24.0

Re: [PATCHv2 1/6] ASoC: da7213: Add da7212 DT compatible

[Sebastian Reichel]

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

Hi,

On Thu, Nov 21, 2019 at 08:10:30PM +0000, Adam Thomson wrote:
> [...]
>
> >  Required properties:
> > -- compatible : Should be "dlg,da7213"
> > +- compatible : Should be "dlg,da7212" or "dlg,7213"
> 
> Typo? "dlg,da7213"

Ack.

> [...]

-- Sebastian

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

[PATCHv2 2/6] ASoC: da7213: Add regulator support

[Sebastian Reichel]

This adds support for most regulators of da7212 for improved
power management. The only thing skipped was the speaker supply,
which has some undocumented dependencies. It's supposed to be
either always-enabled or always-disabled.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../devicetree/bindings/sound/da7213.txt      |  4 +
 sound/soc/codecs/da7213.c                     | 79 ++++++++++++++++++-
 sound/soc/codecs/da7213.h                     |  9 +++
 3 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt
index 759bb04e0283..cc8200b7d748 100644
--- a/Documentation/devicetree/bindings/sound/da7213.txt
+++ b/Documentation/devicetree/bindings/sound/da7213.txt
@@ -21,6 +21,10 @@ Optional properties:
 - dlg,dmic-clkrate : DMIC clock frequency (Hz).
 	[<1500000>, <3000000>]
 
+ - VDDA-supply : Regulator phandle for Analogue power supply
+ - VDDMIC-supply : Regulator phandle for Mic Bias
+ - VDDIO-supply : Regulator phandle for I/O power supply
+
 ======
 
 Example:
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index aff306bb58df..0359249118d0 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
+#include <linux/pm_runtime.h>
 #include <sound/soc.h>
 #include <sound/initval.h>
 #include <sound/tlv.h>
@@ -806,6 +807,11 @@ static int da7213_dai_event(struct snd_soc_dapm_widget *w,
  */
 
 static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
+	/*
+	 * Power Supply
+	 */
+	SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
+
 	/*
 	 * Input & Output
 	 */
@@ -932,6 +938,9 @@ static const struct snd_soc_dapm_route da7213_audio_map[] = {
 	/* Dest       Connecting Widget    source */
 
 	/* Input path */
+	{"Mic Bias 1", NULL, "VDDMIC"},
+	{"Mic Bias 2", NULL, "VDDMIC"},
+
 	{"MIC1", NULL, "Mic Bias 1"},
 	{"MIC2", NULL, "Mic Bias 2"},
 
@@ -1691,6 +1700,8 @@ static int da7213_probe(struct snd_soc_component *component)
 {
 	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
 
+	pm_runtime_get_sync(component->dev);
+
 	/* Default to using ALC auto offset calibration mode. */
 	snd_soc_component_update_bits(component, DA7213_ALC_CTRL1,
 			    DA7213_ALC_CALIB_MODE_MAN, 0);
@@ -1811,6 +1822,8 @@ static int da7213_probe(struct snd_soc_component *component)
 				    DA7213_DMIC_CLK_RATE_MASK, dmic_cfg);
 	}
 
+	pm_runtime_put_sync(component->dev);
+
 	/* Check if MCLK provided */
 	da7213->mclk = devm_clk_get(component->dev, "mclk");
 	if (IS_ERR(da7213->mclk)) {
@@ -1848,11 +1861,22 @@ static const struct regmap_config da7213_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+static void da7213_power_off(void *data)
+{
+	struct da7213_priv *da7213 = data;
+	regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
+}
+
+static const char *da7213_supply_names[DA7213_NUM_SUPPLIES] = {
+	[DA7213_SUPPLY_VDDA] = "VDDA",
+	[DA7213_SUPPLY_VDDIO] = "VDDIO",
+};
+
 static int da7213_i2c_probe(struct i2c_client *i2c,
 			    const struct i2c_device_id *id)
 {
 	struct da7213_priv *da7213;
-	int ret;
+	int i, ret;
 
 	da7213 = devm_kzalloc(&i2c->dev, sizeof(*da7213), GFP_KERNEL);
 	if (!da7213)
@@ -1860,6 +1884,25 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
 
 	i2c_set_clientdata(i2c, da7213);
 
+	/* Get required supplies */
+	for (i = 0; i < DA7213_NUM_SUPPLIES; ++i)
+		da7213->supplies[i].supply = da7213_supply_names[i];
+
+	ret = devm_regulator_bulk_get(&i2c->dev, DA7213_NUM_SUPPLIES,
+				      da7213->supplies);
+	if (ret) {
+		dev_err(&i2c->dev, "Failed to get supplies: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off, da7213);
+	if (ret < 0)
+		return ret;
+
 	da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config);
 	if (IS_ERR(da7213->regmap)) {
 		ret = PTR_ERR(da7213->regmap);
@@ -1867,6 +1910,11 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	pm_runtime_set_autosuspend_delay(&i2c->dev, 100);
+	pm_runtime_use_autosuspend(&i2c->dev);
+	pm_runtime_set_active(&i2c->dev);
+	pm_runtime_enable(&i2c->dev);
+
 	ret = devm_snd_soc_register_component(&i2c->dev,
 			&soc_component_dev_da7213, &da7213_dai, 1);
 	if (ret < 0) {
@@ -1876,6 +1924,34 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
 	return ret;
 }
 
+static int __maybe_unused da7213_runtime_suspend(struct device *dev)
+{
+	struct da7213_priv *da7213 = dev_get_drvdata(dev);
+
+	regcache_cache_only(da7213->regmap, true);
+	regcache_mark_dirty(da7213->regmap);
+	regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
+
+	return 0;
+}
+
+static int __maybe_unused da7213_runtime_resume(struct device *dev)
+{
+	struct da7213_priv *da7213 = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
+	if (ret < 0)
+		return ret;
+	regcache_cache_only(da7213->regmap, false);
+	regcache_sync(da7213->regmap);
+	return 0;
+}
+
+static const struct dev_pm_ops da7213_pm = {
+	SET_RUNTIME_PM_OPS(da7213_runtime_suspend, da7213_runtime_resume, NULL)
+};
+
 static const struct i2c_device_id da7213_i2c_id[] = {
 	{ "da7213", 0 },
 	{ }
@@ -1888,6 +1964,7 @@ static struct i2c_driver da7213_i2c_driver = {
 		.name = "da7213",
 		.of_match_table = of_match_ptr(da7213_of_match),
 		.acpi_match_table = ACPI_PTR(da7213_acpi_match),
+		.pm = &da7213_pm,
 	},
 	.probe		= da7213_i2c_probe,
 	.id_table	= da7213_i2c_id,
diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
index 3250a3821fcc..3890829dfb6e 100644
--- a/sound/soc/codecs/da7213.h
+++ b/sound/soc/codecs/da7213.h
@@ -12,6 +12,7 @@
 
 #include <linux/clk.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <sound/da7213.h>
 
 /*
@@ -521,9 +522,17 @@ enum da7213_sys_clk {
 	DA7213_SYSCLK_PLL_32KHZ
 };
 
+/* Regulators */
+enum da7213_supplies {
+	DA7213_SUPPLY_VDDA = 0,
+	DA7213_SUPPLY_VDDIO,
+	DA7213_NUM_SUPPLIES,
+};
+
 /* Codec private data */
 struct da7213_priv {
 	struct regmap *regmap;
+	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
 	struct clk *mclk;
 	unsigned int mclk_rate;
 	int clk_src;
-- 
2.24.0

RE: [PATCHv2 2/6] ASoC: da7213: Add regulator support

[Adam Thomson]

On 20 November 2019 15:24, Sebastian Reichel wrote:

> This adds support for most regulators of da7212 for improved
> power management. The only thing skipped was the speaker supply,
> which has some undocumented dependencies. It's supposed to be
> either always-enabled or always-disabled.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../devicetree/bindings/sound/da7213.txt      |  4 +
>  sound/soc/codecs/da7213.c                     | 79 ++++++++++++++++++-
>  sound/soc/codecs/da7213.h                     |  9 +++
>  3 files changed, 91 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/da7213.txt
> b/Documentation/devicetree/bindings/sound/da7213.txt
> index 759bb04e0283..cc8200b7d748 100644
> --- a/Documentation/devicetree/bindings/sound/da7213.txt
> +++ b/Documentation/devicetree/bindings/sound/da7213.txt
> @@ -21,6 +21,10 @@ Optional properties:
>  - dlg,dmic-clkrate : DMIC clock frequency (Hz).
>  	[<1500000>, <3000000>]
>
> + - VDDA-supply : Regulator phandle for Analogue power supply
> + - VDDMIC-supply : Regulator phandle for Mic Bias
> + - VDDIO-supply : Regulator phandle for I/O power supply
> +
>  ======
>
>  Example:
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index aff306bb58df..0359249118d0 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -19,6 +19,7 @@
>  #include <linux/module.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
> +#include <linux/pm_runtime.h>
>  #include <sound/soc.h>
>  #include <sound/initval.h>
>  #include <sound/tlv.h>
> @@ -806,6 +807,11 @@ static int da7213_dai_event(struct
> snd_soc_dapm_widget *w,
>   */
>
>  static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
> +	/*
> +	 * Power Supply
> +	 */
> +	SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
> +
>  	/*
>  	 * Input & Output
>  	 */
> @@ -932,6 +938,9 @@ static const struct snd_soc_dapm_route
> da7213_audio_map[] = {
>  	/* Dest       Connecting Widget    source */
>
>  	/* Input path */
> +	{"Mic Bias 1", NULL, "VDDMIC"},
> +	{"Mic Bias 2", NULL, "VDDMIC"},
> +
>  	{"MIC1", NULL, "Mic Bias 1"},
>  	{"MIC2", NULL, "Mic Bias 2"},
>
> @@ -1691,6 +1700,8 @@ static int da7213_probe(struct snd_soc_component
> *component)
>  {
>  	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
>
> +	pm_runtime_get_sync(component->dev);

It seems that this function can return errors, although I do see lots of
instances of this being called where the return value isn't checked. Not had
time to walk the code fully but are we sure no errors are going to happen here?

> +
>  	/* Default to using ALC auto offset calibration mode. */
>  	snd_soc_component_update_bits(component, DA7213_ALC_CTRL1,
>  			    DA7213_ALC_CALIB_MODE_MAN, 0);
> @@ -1811,6 +1822,8 @@ static int da7213_probe(struct snd_soc_component
> *component)
>  				    DA7213_DMIC_CLK_RATE_MASK, dmic_cfg);
>  	}
>
> +	pm_runtime_put_sync(component->dev);

Same question here.

> +
>  	/* Check if MCLK provided */
>  	da7213->mclk = devm_clk_get(component->dev, "mclk");
>  	if (IS_ERR(da7213->mclk)) {
> @@ -1848,11 +1861,22 @@ static const struct regmap_config
> da7213_regmap_config = {
>  	.cache_type = REGCACHE_RBTREE,
>  };
>
> +static void da7213_power_off(void *data)
> +{
> +	struct da7213_priv *da7213 = data;
> +	regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
> +}
> +
> +static const char *da7213_supply_names[DA7213_NUM_SUPPLIES] = {
> +	[DA7213_SUPPLY_VDDA] = "VDDA",
> +	[DA7213_SUPPLY_VDDIO] = "VDDIO",
> +};
> +
>  static int da7213_i2c_probe(struct i2c_client *i2c,
>  			    const struct i2c_device_id *id)
>  {
>  	struct da7213_priv *da7213;
> -	int ret;
> +	int i, ret;
>
>  	da7213 = devm_kzalloc(&i2c->dev, sizeof(*da7213), GFP_KERNEL);
>  	if (!da7213)
> @@ -1860,6 +1884,25 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
>
>  	i2c_set_clientdata(i2c, da7213);
>
> +	/* Get required supplies */
> +	for (i = 0; i < DA7213_NUM_SUPPLIES; ++i)
> +		da7213->supplies[i].supply = da7213_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(&i2c->dev, DA7213_NUM_SUPPLIES,
> +				      da7213->supplies);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to get supplies: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
> >supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> da7213);
> +	if (ret < 0)
> +		return ret;
> +
>  	da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config);
>  	if (IS_ERR(da7213->regmap)) {
>  		ret = PTR_ERR(da7213->regmap);
> @@ -1867,6 +1910,11 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
>  		return ret;
>  	}
>
> +	pm_runtime_set_autosuspend_delay(&i2c->dev, 100);
> +	pm_runtime_use_autosuspend(&i2c->dev);
> +	pm_runtime_set_active(&i2c->dev);

Again this can return an error. Are we certain this won't fail?

> +	pm_runtime_enable(&i2c->dev);
> +
>  	ret = devm_snd_soc_register_component(&i2c->dev,
>  			&soc_component_dev_da7213, &da7213_dai, 1);
>  	if (ret < 0) {
> @@ -1876,6 +1924,34 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
>  	return ret;
>  }
>
> +static int __maybe_unused da7213_runtime_suspend(struct device *dev)
> +{
> +	struct da7213_priv *da7213 = dev_get_drvdata(dev);
> +
> +	regcache_cache_only(da7213->regmap, true);
> +	regcache_mark_dirty(da7213->regmap);
> +	regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused da7213_runtime_resume(struct device *dev)
> +{
> +	struct da7213_priv *da7213 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
> >supplies);
> +	if (ret < 0)
> +		return ret;
> +	regcache_cache_only(da7213->regmap, false);
> +	regcache_sync(da7213->regmap);
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops da7213_pm = {
> +	SET_RUNTIME_PM_OPS(da7213_runtime_suspend,
> da7213_runtime_resume, NULL)
> +};
> +
>  static const struct i2c_device_id da7213_i2c_id[] = {
>  	{ "da7213", 0 },
>  	{ }
> @@ -1888,6 +1964,7 @@ static struct i2c_driver da7213_i2c_driver = {
>  		.name = "da7213",
>  		.of_match_table = of_match_ptr(da7213_of_match),
>  		.acpi_match_table = ACPI_PTR(da7213_acpi_match),
> +		.pm = &da7213_pm,
>  	},
>  	.probe		= da7213_i2c_probe,
>  	.id_table	= da7213_i2c_id,
> diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> index 3250a3821fcc..3890829dfb6e 100644
> --- a/sound/soc/codecs/da7213.h
> +++ b/sound/soc/codecs/da7213.h
> @@ -12,6 +12,7 @@
>
>  #include <linux/clk.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <sound/da7213.h>
>
>  /*
> @@ -521,9 +522,17 @@ enum da7213_sys_clk {
>  	DA7213_SYSCLK_PLL_32KHZ
>  };
>
> +/* Regulators */
> +enum da7213_supplies {
> +	DA7213_SUPPLY_VDDA = 0,
> +	DA7213_SUPPLY_VDDIO,
> +	DA7213_NUM_SUPPLIES,
> +};
> +
>  /* Codec private data */
>  struct da7213_priv {
>  	struct regmap *regmap;
> +	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
>  	struct clk *mclk;
>  	unsigned int mclk_rate;
>  	int clk_src;
> --
> 2.24.0

Re: [PATCHv2 2/6] ASoC: da7213: Add regulator support

[Sebastian Reichel]

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

Hi,

On Thu, Nov 21, 2019 at 09:15:02PM +0000, Adam Thomson wrote:
> On 20 November 2019 15:24, Sebastian Reichel wrote:
> 
> > This adds support for most regulators of da7212 for improved
> > power management. The only thing skipped was the speaker supply,
> > which has some undocumented dependencies. It's supposed to be
> > either always-enabled or always-disabled.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  .../devicetree/bindings/sound/da7213.txt      |  4 +
> >  sound/soc/codecs/da7213.c                     | 79 ++++++++++++++++++-
> >  sound/soc/codecs/da7213.h                     |  9 +++
> >  3 files changed, 91 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/da7213.txt
> > b/Documentation/devicetree/bindings/sound/da7213.txt
> > index 759bb04e0283..cc8200b7d748 100644
> > --- a/Documentation/devicetree/bindings/sound/da7213.txt
> > +++ b/Documentation/devicetree/bindings/sound/da7213.txt
> > @@ -21,6 +21,10 @@ Optional properties:
> >  - dlg,dmic-clkrate : DMIC clock frequency (Hz).
> >  	[<1500000>, <3000000>]
> >
> > + - VDDA-supply : Regulator phandle for Analogue power supply
> > + - VDDMIC-supply : Regulator phandle for Mic Bias
> > + - VDDIO-supply : Regulator phandle for I/O power supply
> > +
> >  ======
> >
> >  Example:
> > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> > index aff306bb58df..0359249118d0 100644
> > --- a/sound/soc/codecs/da7213.c
> > +++ b/sound/soc/codecs/da7213.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/module.h>
> >  #include <sound/pcm.h>
> >  #include <sound/pcm_params.h>
> > +#include <linux/pm_runtime.h>
> >  #include <sound/soc.h>
> >  #include <sound/initval.h>
> >  #include <sound/tlv.h>
> > @@ -806,6 +807,11 @@ static int da7213_dai_event(struct
> > snd_soc_dapm_widget *w,
> >   */
> >
> >  static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
> > +	/*
> > +	 * Power Supply
> > +	 */
> > +	SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
> > +
> >  	/*
> >  	 * Input & Output
> >  	 */
> > @@ -932,6 +938,9 @@ static const struct snd_soc_dapm_route
> > da7213_audio_map[] = {
> >  	/* Dest       Connecting Widget    source */
> >
> >  	/* Input path */
> > +	{"Mic Bias 1", NULL, "VDDMIC"},
> > +	{"Mic Bias 2", NULL, "VDDMIC"},
> > +
> >  	{"MIC1", NULL, "Mic Bias 1"},
> >  	{"MIC2", NULL, "Mic Bias 2"},
> >
> > @@ -1691,6 +1700,8 @@ static int da7213_probe(struct snd_soc_component
> > *component)
> >  {
> >  	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> >
> > +	pm_runtime_get_sync(component->dev);
> 
> It seems that this function can return errors, although I do see lots of
> instances of this being called where the return value isn't checked. Not had
> time to walk the code fully but are we sure no errors are going to happen here?

In this case, the runtime PM is already enabled because of
pm_runtime_set_active() being called previously. So this only
increases the usage counter.

> > +
> >  	/* Default to using ALC auto offset calibration mode. */
> >  	snd_soc_component_update_bits(component, DA7213_ALC_CTRL1,
> >  			    DA7213_ALC_CALIB_MODE_MAN, 0);
> > @@ -1811,6 +1822,8 @@ static int da7213_probe(struct snd_soc_component
> > *component)
> >  				    DA7213_DMIC_CLK_RATE_MASK, dmic_cfg);
> >  	}
> >
> > +	pm_runtime_put_sync(component->dev);
> 
> Same question here.

da7213_runtime_suspend() always returns 0.

> > +
> >  	/* Check if MCLK provided */
> >  	da7213->mclk = devm_clk_get(component->dev, "mclk");
> >  	if (IS_ERR(da7213->mclk)) {
> > @@ -1848,11 +1861,22 @@ static const struct regmap_config
> > da7213_regmap_config = {
> >  	.cache_type = REGCACHE_RBTREE,
> >  };
> >
> > +static void da7213_power_off(void *data)
> > +{
> > +	struct da7213_priv *da7213 = data;
> > +	regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
> > +}
> > +
> > +static const char *da7213_supply_names[DA7213_NUM_SUPPLIES] = {
> > +	[DA7213_SUPPLY_VDDA] = "VDDA",
> > +	[DA7213_SUPPLY_VDDIO] = "VDDIO",
> > +};
> > +
> >  static int da7213_i2c_probe(struct i2c_client *i2c,
> >  			    const struct i2c_device_id *id)
> >  {
> >  	struct da7213_priv *da7213;
> > -	int ret;
> > +	int i, ret;
> >
> >  	da7213 = devm_kzalloc(&i2c->dev, sizeof(*da7213), GFP_KERNEL);
> >  	if (!da7213)
> > @@ -1860,6 +1884,25 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> >
> >  	i2c_set_clientdata(i2c, da7213);
> >
> > +	/* Get required supplies */
> > +	for (i = 0; i < DA7213_NUM_SUPPLIES; ++i)
> > +		da7213->supplies[i].supply = da7213_supply_names[i];
> > +
> > +	ret = devm_regulator_bulk_get(&i2c->dev, DA7213_NUM_SUPPLIES,
> > +				      da7213->supplies);
> > +	if (ret) {
> > +		dev_err(&i2c->dev, "Failed to get supplies: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
> > >supplies);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> > da7213);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config);
> >  	if (IS_ERR(da7213->regmap)) {
> >  		ret = PTR_ERR(da7213->regmap);
> > @@ -1867,6 +1910,11 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> >  		return ret;
> >  	}
> >
> > +	pm_runtime_set_autosuspend_delay(&i2c->dev, 100);
> > +	pm_runtime_use_autosuspend(&i2c->dev);
> > +	pm_runtime_set_active(&i2c->dev);
> 
> Again this can return an error. Are we certain this won't fail?

This only provides the information, that the device is running. The
parent might be affected, but that is running anyways since we are
probing a child device.

> > +	pm_runtime_enable(&i2c->dev);
> > +
> >  	ret = devm_snd_soc_register_component(&i2c->dev,
> >  			&soc_component_dev_da7213, &da7213_dai, 1);
> >  	if (ret < 0) {
> > @@ -1876,6 +1924,34 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> >  	return ret;
> >  }
> >
> > +static int __maybe_unused da7213_runtime_suspend(struct device *dev)
> > +{
> > +	struct da7213_priv *da7213 = dev_get_drvdata(dev);
> > +
> > +	regcache_cache_only(da7213->regmap, true);
> > +	regcache_mark_dirty(da7213->regmap);
> > +	regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused da7213_runtime_resume(struct device *dev)
> > +{
> > +	struct da7213_priv *da7213 = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
> > >supplies);
> > +	if (ret < 0)
> > +		return ret;
> > +	regcache_cache_only(da7213->regmap, false);
> > +	regcache_sync(da7213->regmap);
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops da7213_pm = {
> > +	SET_RUNTIME_PM_OPS(da7213_runtime_suspend,
> > da7213_runtime_resume, NULL)
> > +};
> > +
> >  static const struct i2c_device_id da7213_i2c_id[] = {
> >  	{ "da7213", 0 },
> >  	{ }
> > @@ -1888,6 +1964,7 @@ static struct i2c_driver da7213_i2c_driver = {
> >  		.name = "da7213",
> >  		.of_match_table = of_match_ptr(da7213_of_match),
> >  		.acpi_match_table = ACPI_PTR(da7213_acpi_match),
> > +		.pm = &da7213_pm,
> >  	},
> >  	.probe		= da7213_i2c_probe,
> >  	.id_table	= da7213_i2c_id,
> > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> > index 3250a3821fcc..3890829dfb6e 100644
> > --- a/sound/soc/codecs/da7213.h
> > +++ b/sound/soc/codecs/da7213.h
> > @@ -12,6 +12,7 @@
> >
> >  #include <linux/clk.h>
> >  #include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <sound/da7213.h>
> >
> >  /*
> > @@ -521,9 +522,17 @@ enum da7213_sys_clk {
> >  	DA7213_SYSCLK_PLL_32KHZ
> >  };
> >
> > +/* Regulators */
> > +enum da7213_supplies {
> > +	DA7213_SUPPLY_VDDA = 0,
> > +	DA7213_SUPPLY_VDDIO,
> > +	DA7213_NUM_SUPPLIES,
> > +};
> > +
> >  /* Codec private data */
> >  struct da7213_priv {
> >  	struct regmap *regmap;
> > +	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
> >  	struct clk *mclk;
> >  	unsigned int mclk_rate;
> >  	int clk_src;
> > --
> > 2.24.0

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

RE: [PATCHv2 2/6] ASoC: da7213: Add regulator support

[Adam Thomson]

On 22 November 2019 17:10, Sebastian Reichel wrote:

> Hi,

Ok, based on feedback I have no further comment so:

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> 
> On Thu, Nov 21, 2019 at 09:15:02PM +0000, Adam Thomson wrote:
> > On 20 November 2019 15:24, Sebastian Reichel wrote:
> >
> > > This adds support for most regulators of da7212 for improved
> > > power management. The only thing skipped was the speaker supply,
> > > which has some undocumented dependencies. It's supposed to be
> > > either always-enabled or always-disabled.
> > >
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  .../devicetree/bindings/sound/da7213.txt      |  4 +
> > >  sound/soc/codecs/da7213.c                     | 79 ++++++++++++++++++-
> > >  sound/soc/codecs/da7213.h                     |  9 +++
> > >  3 files changed, 91 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/sound/da7213.txt
> > > b/Documentation/devicetree/bindings/sound/da7213.txt
> > > index 759bb04e0283..cc8200b7d748 100644
> > > --- a/Documentation/devicetree/bindings/sound/da7213.txt
> > > +++ b/Documentation/devicetree/bindings/sound/da7213.txt
> > > @@ -21,6 +21,10 @@ Optional properties:
> > >  - dlg,dmic-clkrate : DMIC clock frequency (Hz).
> > >  	[<1500000>, <3000000>]
> > >
> > > + - VDDA-supply : Regulator phandle for Analogue power supply
> > > + - VDDMIC-supply : Regulator phandle for Mic Bias
> > > + - VDDIO-supply : Regulator phandle for I/O power supply
> > > +
> > >  ======
> > >
> > >  Example:
> > > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> > > index aff306bb58df..0359249118d0 100644
> > > --- a/sound/soc/codecs/da7213.c
> > > +++ b/sound/soc/codecs/da7213.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/module.h>
> > >  #include <sound/pcm.h>
> > >  #include <sound/pcm_params.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <sound/soc.h>
> > >  #include <sound/initval.h>
> > >  #include <sound/tlv.h>
> > > @@ -806,6 +807,11 @@ static int da7213_dai_event(struct
> > > snd_soc_dapm_widget *w,
> > >   */
> > >
> > >  static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
> > > +	/*
> > > +	 * Power Supply
> > > +	 */
> > > +	SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
> > > +
> > >  	/*
> > >  	 * Input & Output
> > >  	 */
> > > @@ -932,6 +938,9 @@ static const struct snd_soc_dapm_route
> > > da7213_audio_map[] = {
> > >  	/* Dest       Connecting Widget    source */
> > >
> > >  	/* Input path */
> > > +	{"Mic Bias 1", NULL, "VDDMIC"},
> > > +	{"Mic Bias 2", NULL, "VDDMIC"},
> > > +
> > >  	{"MIC1", NULL, "Mic Bias 1"},
> > >  	{"MIC2", NULL, "Mic Bias 2"},
> > >
> > > @@ -1691,6 +1700,8 @@ static int da7213_probe(struct snd_soc_component
> > > *component)
> > >  {
> > >  	struct da7213_priv *da7213 =
> > > snd_soc_component_get_drvdata(component);
> > >
> > > +	pm_runtime_get_sync(component->dev);
> >
> > It seems that this function can return errors, although I do see lots of
> > instances of this being called where the return value isn't checked. Not had
> > time to walk the code fully but are we sure no errors are going to happen here?
> 
> In this case, the runtime PM is already enabled because of
> pm_runtime_set_active() being called previously. So this only
> increases the usage counter.
> 
> > > +
> > >  	/* Default to using ALC auto offset calibration mode. */
> > >  	snd_soc_component_update_bits(component, DA7213_ALC_CTRL1,
> > >  			    DA7213_ALC_CALIB_MODE_MAN, 0);
> > > @@ -1811,6 +1822,8 @@ static int da7213_probe(struct snd_soc_component
> > > *component)
> > >  				    DA7213_DMIC_CLK_RATE_MASK, dmic_cfg);
> > >  	}
> > >
> > > +	pm_runtime_put_sync(component->dev);
> >
> > Same question here.
> 
> da7213_runtime_suspend() always returns 0.
> 
> > > +
> > >  	/* Check if MCLK provided */
> > >  	da7213->mclk = devm_clk_get(component->dev, "mclk");
> > >  	if (IS_ERR(da7213->mclk)) {
> > > @@ -1848,11 +1861,22 @@ static const struct regmap_config
> > > da7213_regmap_config = {
> > >  	.cache_type = REGCACHE_RBTREE,
> > >  };
> > >
> > > +static void da7213_power_off(void *data)
> > > +{
> > > +	struct da7213_priv *da7213 = data;
> > > +	regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
> > > +}
> > > +
> > > +static const char *da7213_supply_names[DA7213_NUM_SUPPLIES] = {
> > > +	[DA7213_SUPPLY_VDDA] = "VDDA",
> > > +	[DA7213_SUPPLY_VDDIO] = "VDDIO",
> > > +};
> > > +
> > >  static int da7213_i2c_probe(struct i2c_client *i2c,
> > >  			    const struct i2c_device_id *id)
> > >  {
> > >  	struct da7213_priv *da7213;
> > > -	int ret;
> > > +	int i, ret;
> > >
> > >  	da7213 = devm_kzalloc(&i2c->dev, sizeof(*da7213), GFP_KERNEL);
> > >  	if (!da7213)
> > > @@ -1860,6 +1884,25 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> > >
> > >  	i2c_set_clientdata(i2c, da7213);
> > >
> > > +	/* Get required supplies */
> > > +	for (i = 0; i < DA7213_NUM_SUPPLIES; ++i)
> > > +		da7213->supplies[i].supply = da7213_supply_names[i];
> > > +
> > > +	ret = devm_regulator_bulk_get(&i2c->dev, DA7213_NUM_SUPPLIES,
> > > +				      da7213->supplies);
> > > +	if (ret) {
> > > +		dev_err(&i2c->dev, "Failed to get supplies: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
> > > >supplies);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> > > da7213);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > >  	da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config);
> > >  	if (IS_ERR(da7213->regmap)) {
> > >  		ret = PTR_ERR(da7213->regmap);
> > > @@ -1867,6 +1910,11 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> > >  		return ret;
> > >  	}
> > >
> > > +	pm_runtime_set_autosuspend_delay(&i2c->dev, 100);
> > > +	pm_runtime_use_autosuspend(&i2c->dev);
> > > +	pm_runtime_set_active(&i2c->dev);
> >
> > Again this can return an error. Are we certain this won't fail?
> 
> This only provides the information, that the device is running. The
> parent might be affected, but that is running anyways since we are
> probing a child device.
> 
> > > +	pm_runtime_enable(&i2c->dev);
> > > +
> > >  	ret = devm_snd_soc_register_component(&i2c->dev,
> > >  			&soc_component_dev_da7213, &da7213_dai, 1);
> > >  	if (ret < 0) {
> > > @@ -1876,6 +1924,34 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> > >  	return ret;
> > >  }
> > >
> > > +static int __maybe_unused da7213_runtime_suspend(struct device *dev)
> > > +{
> > > +	struct da7213_priv *da7213 = dev_get_drvdata(dev);
> > > +
> > > +	regcache_cache_only(da7213->regmap, true);
> > > +	regcache_mark_dirty(da7213->regmap);
> > > +	regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __maybe_unused da7213_runtime_resume(struct device *dev)
> > > +{
> > > +	struct da7213_priv *da7213 = dev_get_drvdata(dev);
> > > +	int ret;
> > > +
> > > +	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
> > > >supplies);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	regcache_cache_only(da7213->regmap, false);
> > > +	regcache_sync(da7213->regmap);
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct dev_pm_ops da7213_pm = {
> > > +	SET_RUNTIME_PM_OPS(da7213_runtime_suspend,
> > > da7213_runtime_resume, NULL)
> > > +};
> > > +
> > >  static const struct i2c_device_id da7213_i2c_id[] = {
> > >  	{ "da7213", 0 },
> > >  	{ }
> > > @@ -1888,6 +1964,7 @@ static struct i2c_driver da7213_i2c_driver = {
> > >  		.name = "da7213",
> > >  		.of_match_table = of_match_ptr(da7213_of_match),
> > >  		.acpi_match_table = ACPI_PTR(da7213_acpi_match),
> > > +		.pm = &da7213_pm,
> > >  	},
> > >  	.probe		= da7213_i2c_probe,
> > >  	.id_table	= da7213_i2c_id,
> > > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> > > index 3250a3821fcc..3890829dfb6e 100644
> > > --- a/sound/soc/codecs/da7213.h
> > > +++ b/sound/soc/codecs/da7213.h
> > > @@ -12,6 +12,7 @@
> > >
> > >  #include <linux/clk.h>
> > >  #include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > >  #include <sound/da7213.h>
> > >
> > >  /*
> > > @@ -521,9 +522,17 @@ enum da7213_sys_clk {
> > >  	DA7213_SYSCLK_PLL_32KHZ
> > >  };
> > >
> > > +/* Regulators */
> > > +enum da7213_supplies {
> > > +	DA7213_SUPPLY_VDDA = 0,
> > > +	DA7213_SUPPLY_VDDIO,
> > > +	DA7213_NUM_SUPPLIES,
> > > +};
> > > +
> > >  /* Codec private data */
> > >  struct da7213_priv {
> > >  	struct regmap *regmap;
> > > +	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
> > >  	struct clk *mclk;
> > >  	unsigned int mclk_rate;
> > >  	int clk_src;
> > > --
> > > 2.24.0

[PATCHv2 3/6] ASoC: da7213: Provide selectable option

[Sebastian Reichel]

This commit adds the Dialog DA7213 audio codec as a selectable option
in the kernel config. Currently the driver can only be selected for
Intel Baytrail/Cherrytrail devices or if SND_SOC_ALL_CODECS is enabled.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 sound/soc/codecs/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 229cc89f8c5a..1d44fbc3d407 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -646,7 +646,8 @@ config SND_SOC_DA7210
         tristate
 
 config SND_SOC_DA7213
-        tristate
+	tristate "Dialog DA7213 CODEC"
+	depends on I2C
 
 config SND_SOC_DA7218
 	tristate
-- 
2.24.0

RE: [PATCHv2 3/6] ASoC: da7213: Provide selectable option

[Adam Thomson]

On 20 November 2019 15:24, Sebastian Reichel wrote:

> This commit adds the Dialog DA7213 audio codec as a selectable option
> in the kernel config. Currently the driver can only be selected for
> Intel Baytrail/Cherrytrail devices or if SND_SOC_ALL_CODECS is enabled.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
>  sound/soc/codecs/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 229cc89f8c5a..1d44fbc3d407 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -646,7 +646,8 @@ config SND_SOC_DA7210
>          tristate
> 
>  config SND_SOC_DA7213
> -        tristate
> +	tristate "Dialog DA7213 CODEC"
> +	depends on I2C
> 
>  config SND_SOC_DA7218
>  	tristate
> --
> 2.24.0

[PATCHv2 4/6] ASoC: da7213: Move set_sysclk to codec level

[Sebastian Reichel]

Move set_sysclk function to component level, so that it can be used at
both component and DAI level.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 sound/soc/codecs/da7213.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index 0359249118d0..9686948b16ea 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1343,10 +1343,10 @@ static int da7213_mute(struct snd_soc_dai *dai, int mute)
 #define DA7213_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
 			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
 
-static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai,
-				 int clk_id, unsigned int freq, int dir)
+static int da7213_set_component_sysclk(struct snd_soc_component *component,
+				       int clk_id, int source,
+				       unsigned int freq, int dir)
 {
-	struct snd_soc_component *component = codec_dai->component;
 	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
 	int ret = 0;
 
@@ -1354,7 +1354,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 		return 0;
 
 	if (((freq < 5000000) && (freq != 32768)) || (freq > 54000000)) {
-		dev_err(codec_dai->dev, "Unsupported MCLK value %d\n",
+		dev_err(component->dev, "Unsupported MCLK value %d\n",
 			freq);
 		return -EINVAL;
 	}
@@ -1370,7 +1370,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 				    DA7213_PLL_MCLK_SQR_EN);
 		break;
 	default:
-		dev_err(codec_dai->dev, "Unknown clock source %d\n", clk_id);
+		dev_err(component->dev, "Unknown clock source %d\n", clk_id);
 		return -EINVAL;
 	}
 
@@ -1380,7 +1380,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 		freq = clk_round_rate(da7213->mclk, freq);
 		ret = clk_set_rate(da7213->mclk, freq);
 		if (ret) {
-			dev_err(codec_dai->dev, "Failed to set clock rate %d\n",
+			dev_err(component->dev, "Failed to set clock rate %d\n",
 				freq);
 			return ret;
 		}
@@ -1507,7 +1507,6 @@ static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
 static const struct snd_soc_dai_ops da7213_dai_ops = {
 	.hw_params	= da7213_hw_params,
 	.set_fmt	= da7213_set_dai_fmt,
-	.set_sysclk	= da7213_set_dai_sysclk,
 	.set_pll	= da7213_set_dai_pll,
 	.digital_mute	= da7213_mute,
 };
@@ -1845,6 +1844,7 @@ static const struct snd_soc_component_driver soc_component_dev_da7213 = {
 	.num_dapm_widgets	= ARRAY_SIZE(da7213_dapm_widgets),
 	.dapm_routes		= da7213_audio_map,
 	.num_dapm_routes	= ARRAY_SIZE(da7213_audio_map),
+	.set_sysclk		= da7213_set_component_sysclk,
 	.idle_bias_on		= 1,
 	.use_pmdown_time	= 1,
 	.endianness		= 1,
-- 
2.24.0

RE: [PATCHv2 4/6] ASoC: da7213: Move set_sysclk to codec level

[Adam Thomson]

On 20 November 2019 15:24, Sebastian Reichel wrote:

> Move set_sysclk function to component level, so that it can be used at
> both component and DAI level.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
>  sound/soc/codecs/da7213.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 0359249118d0..9686948b16ea 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1343,10 +1343,10 @@ static int da7213_mute(struct snd_soc_dai *dai, int
> mute)
>  #define DA7213_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |
> SNDRV_PCM_FMTBIT_S20_3LE |\
>  			SNDRV_PCM_FMTBIT_S24_LE |
> SNDRV_PCM_FMTBIT_S32_LE)
> 
> -static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> -				 int clk_id, unsigned int freq, int dir)
> +static int da7213_set_component_sysclk(struct snd_soc_component
> *component,
> +				       int clk_id, int source,
> +				       unsigned int freq, int dir)
>  {
> -	struct snd_soc_component *component = codec_dai->component;
>  	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
>  	int ret = 0;
> 
> @@ -1354,7 +1354,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai
> *codec_dai,
>  		return 0;
> 
>  	if (((freq < 5000000) && (freq != 32768)) || (freq > 54000000)) {
> -		dev_err(codec_dai->dev, "Unsupported MCLK value %d\n",
> +		dev_err(component->dev, "Unsupported MCLK value %d\n",
>  			freq);
>  		return -EINVAL;
>  	}
> @@ -1370,7 +1370,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai
> *codec_dai,
>  				    DA7213_PLL_MCLK_SQR_EN);
>  		break;
>  	default:
> -		dev_err(codec_dai->dev, "Unknown clock source %d\n", clk_id);
> +		dev_err(component->dev, "Unknown clock source %d\n",
> clk_id);
>  		return -EINVAL;
>  	}
> 
> @@ -1380,7 +1380,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai
> *codec_dai,
>  		freq = clk_round_rate(da7213->mclk, freq);
>  		ret = clk_set_rate(da7213->mclk, freq);
>  		if (ret) {
> -			dev_err(codec_dai->dev, "Failed to set clock rate %d\n",
> +			dev_err(component->dev, "Failed to set clock rate
> %d\n",
>  				freq);
>  			return ret;
>  		}
> @@ -1507,7 +1507,6 @@ static int da7213_set_dai_pll(struct snd_soc_dai
> *codec_dai, int pll_id,
>  static const struct snd_soc_dai_ops da7213_dai_ops = {
>  	.hw_params	= da7213_hw_params,
>  	.set_fmt	= da7213_set_dai_fmt,
> -	.set_sysclk	= da7213_set_dai_sysclk,
>  	.set_pll	= da7213_set_dai_pll,
>  	.digital_mute	= da7213_mute,
>  };
> @@ -1845,6 +1844,7 @@ static const struct snd_soc_component_driver
> soc_component_dev_da7213 = {
>  	.num_dapm_widgets	= ARRAY_SIZE(da7213_dapm_widgets),
>  	.dapm_routes		= da7213_audio_map,
>  	.num_dapm_routes	= ARRAY_SIZE(da7213_audio_map),
> +	.set_sysclk		= da7213_set_component_sysclk,
>  	.idle_bias_on		= 1,
>  	.use_pmdown_time	= 1,
>  	.endianness		= 1,
> --
> 2.24.0

[PATCHv2 5/6] ASoC: da7213: Move set_pll to codec level

[Sebastian Reichel]

Move set_pll function to component level, so that it can be used at
both component and DAI level.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 sound/soc/codecs/da7213.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index 9686948b16ea..3e6ad996741b 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1392,10 +1392,10 @@ static int da7213_set_component_sysclk(struct snd_soc_component *component,
 }
 
 /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
-static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
-			      int source, unsigned int fref, unsigned int fout)
+static int da7213_set_component_pll(struct snd_soc_component *component,
+				    int pll_id, int source,
+				    unsigned int fref, unsigned int fout)
 {
-	struct snd_soc_component *component = codec_dai->component;
 	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
 
 	u8 pll_ctrl, indiv_bits, indiv;
@@ -1507,7 +1507,6 @@ static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
 static const struct snd_soc_dai_ops da7213_dai_ops = {
 	.hw_params	= da7213_hw_params,
 	.set_fmt	= da7213_set_dai_fmt,
-	.set_pll	= da7213_set_dai_pll,
 	.digital_mute	= da7213_mute,
 };
 
@@ -1845,6 +1844,7 @@ static const struct snd_soc_component_driver soc_component_dev_da7213 = {
 	.dapm_routes		= da7213_audio_map,
 	.num_dapm_routes	= ARRAY_SIZE(da7213_audio_map),
 	.set_sysclk		= da7213_set_component_sysclk,
+	.set_pll		= da7213_set_component_pll,
 	.idle_bias_on		= 1,
 	.use_pmdown_time	= 1,
 	.endianness		= 1,
-- 
2.24.0

RE: [PATCHv2 5/6] ASoC: da7213: Move set_pll to codec level

[Adam Thomson]

On 20 November 2019 15:24, Sebastian Reichel wrote:

> Move set_pll function to component level, so that it can be used at
> both component and DAI level.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
>  sound/soc/codecs/da7213.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 9686948b16ea..3e6ad996741b 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1392,10 +1392,10 @@ static int da7213_set_component_sysclk(struct
> snd_soc_component *component,
>  }
> 
>  /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
> -static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
> -			      int source, unsigned int fref, unsigned int fout)
> +static int da7213_set_component_pll(struct snd_soc_component *component,
> +				    int pll_id, int source,
> +				    unsigned int fref, unsigned int fout)
>  {
> -	struct snd_soc_component *component = codec_dai->component;
>  	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> 
>  	u8 pll_ctrl, indiv_bits, indiv;
> @@ -1507,7 +1507,6 @@ static int da7213_set_dai_pll(struct snd_soc_dai
> *codec_dai, int pll_id,
>  static const struct snd_soc_dai_ops da7213_dai_ops = {
>  	.hw_params	= da7213_hw_params,
>  	.set_fmt	= da7213_set_dai_fmt,
> -	.set_pll	= da7213_set_dai_pll,
>  	.digital_mute	= da7213_mute,
>  };
> 
> @@ -1845,6 +1844,7 @@ static const struct snd_soc_component_driver
> soc_component_dev_da7213 = {
>  	.dapm_routes		= da7213_audio_map,
>  	.num_dapm_routes	= ARRAY_SIZE(da7213_audio_map),
>  	.set_sysclk		= da7213_set_component_sysclk,
> +	.set_pll		= da7213_set_component_pll,
>  	.idle_bias_on		= 1,
>  	.use_pmdown_time	= 1,
>  	.endianness		= 1,
> --
> 2.24.0

[PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Sebastian Reichel]

This adds default clock/PLL configuration to the driver
for usage with generic drivers like simple-card for usage
with a fixed rate clock.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 sound/soc/codecs/da7213.c | 75 ++++++++++++++++++++++++++++++++++++---
 sound/soc/codecs/da7213.h |  2 ++
 2 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index 3e6ad996741b..ff1a936240be 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1156,6 +1156,7 @@ static int da7213_hw_params(struct snd_pcm_substream *substream,
 			    struct snd_soc_dai *dai)
 {
 	struct snd_soc_component *component = dai->component;
+	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
 	u8 dai_ctrl = 0;
 	u8 fs;
 
@@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct snd_pcm_substream *substream,
 	switch (params_rate(params)) {
 	case 8000:
 		fs = DA7213_SR_8000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 11025:
 		fs = DA7213_SR_11025;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 12000:
 		fs = DA7213_SR_12000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 16000:
 		fs = DA7213_SR_16000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 22050:
 		fs = DA7213_SR_22050;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 32000:
 		fs = DA7213_SR_32000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 44100:
 		fs = DA7213_SR_44100;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 48000:
 		fs = DA7213_SR_48000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 88200:
 		fs = DA7213_SR_88200;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 96000:
 		fs = DA7213_SR_96000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	default:
 		return -EINVAL;
@@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct snd_soc_component *component,
 }
 
 /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
-static int da7213_set_component_pll(struct snd_soc_component *component,
-				    int pll_id, int source,
-				    unsigned int fref, unsigned int fout)
+static int _da7213_set_component_pll(struct snd_soc_component *component,
+				     int pll_id, int source,
+				     unsigned int fref, unsigned int fout)
 {
 	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
 
@@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct snd_soc_component *component,
 	return 0;
 }
 
+static int da7213_set_component_pll(struct snd_soc_component *component,
+				    int pll_id, int source,
+				    unsigned int fref, unsigned int fout)
+{
+	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+	da7213->fixed_clk_auto_pll = false;
+
+	return _da7213_set_component_pll(component, pll_id, source, fref, fout);
+}
+
 /* DAI operations */
 static const struct snd_soc_dai_ops da7213_dai_ops = {
 	.hw_params	= da7213_hw_params,
@@ -1532,6 +1553,43 @@ static struct snd_soc_dai_driver da7213_dai = {
 	.symmetric_rates = 1,
 };
 
+static int da7213_set_auto_pll(struct snd_soc_component *component, bool enable)
+{
+	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+	int mode;
+
+	if (!da7213->fixed_clk_auto_pll)
+		return 0;
+
+	da7213->mclk_rate = clk_get_rate(da7213->mclk);
+
+	if (enable)
+		mode = DA7213_SYSCLK_PLL;
+	else
+		mode = DA7213_SYSCLK_MCLK;
+
+	switch (da7213->out_rate) {
+	case DA7213_PLL_FREQ_OUT_90316800:
+		if (da7213->mclk_rate == 11289600 ||
+		    da7213->mclk_rate == 22579200 ||
+		    da7213->mclk_rate == 45158400)
+			mode = DA7213_SYSCLK_MCLK;
+		break;
+	case DA7213_PLL_FREQ_OUT_98304000:
+		if (da7213->mclk_rate == 12288000 ||
+		    da7213->mclk_rate == 24576000 ||
+		    da7213->mclk_rate == 49152000)
+			mode = DA7213_SYSCLK_MCLK;
+
+		break;
+	default:
+		return -1;
+	}
+
+	return _da7213_set_component_pll(component, 0, mode,
+					 da7213->mclk_rate, da7213->out_rate);
+}
+
 static int da7213_set_bias_level(struct snd_soc_component *component,
 				 enum snd_soc_bias_level level)
 {
@@ -1551,6 +1609,8 @@ static int da7213_set_bias_level(struct snd_soc_component *component,
 						"Failed to enable mclk\n");
 					return ret;
 				}
+
+				da7213_set_auto_pll(component, true);
 			}
 		}
 		break;
@@ -1562,8 +1622,10 @@ static int da7213_set_bias_level(struct snd_soc_component *component,
 					    DA7213_VMID_EN | DA7213_BIAS_EN);
 		} else {
 			/* Remove MCLK */
-			if (da7213->mclk)
+			if (da7213->mclk) {
+				da7213_set_auto_pll(component, false);
 				clk_disable_unprepare(da7213->mclk);
+			}
 		}
 		break;
 	case SND_SOC_BIAS_OFF:
@@ -1829,6 +1891,11 @@ static int da7213_probe(struct snd_soc_component *component)
 			return PTR_ERR(da7213->mclk);
 		else
 			da7213->mclk = NULL;
+	} else {
+		/* Do automatic PLL handling assuming fixed clock until
+		 * set_pll() has been called. This makes the codec usable
+		 * with the simple-audio-card driver. */
+		da7213->fixed_clk_auto_pll = true;
 	}
 
 	return 0;
diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
index 3890829dfb6e..97ccf0ddd2be 100644
--- a/sound/soc/codecs/da7213.h
+++ b/sound/soc/codecs/da7213.h
@@ -535,10 +535,12 @@ struct da7213_priv {
 	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
 	struct clk *mclk;
 	unsigned int mclk_rate;
+	unsigned int out_rate;
 	int clk_src;
 	bool master;
 	bool alc_calib_auto;
 	bool alc_en;
+	bool fixed_clk_auto_pll;
 	struct da7213_platform_data *pdata;
 };
 
-- 
2.24.0

RE: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Adam Thomson]

On 20 November 2019 15:24, Sebastian Reichel wrote:

> This adds default clock/PLL configuration to the driver
> for usage with generic drivers like simple-card for usage
> with a fixed rate clock.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  sound/soc/codecs/da7213.c | 75
> ++++++++++++++++++++++++++++++++++++---
>  sound/soc/codecs/da7213.h |  2 ++
>  2 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 3e6ad996741b..ff1a936240be 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1156,6 +1156,7 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
>  			    struct snd_soc_dai *dai)
>  {
>  	struct snd_soc_component *component = dai->component;
> +	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
>  	u8 dai_ctrl = 0;
>  	u8 fs;
> 
> @@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
>  	switch (params_rate(params)) {
>  	case 8000:
>  		fs = DA7213_SR_8000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 11025:
>  		fs = DA7213_SR_11025;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 12000:
>  		fs = DA7213_SR_12000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 16000:
>  		fs = DA7213_SR_16000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 22050:
>  		fs = DA7213_SR_22050;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 32000:
>  		fs = DA7213_SR_32000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 44100:
>  		fs = DA7213_SR_44100;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 48000:
>  		fs = DA7213_SR_48000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 88200:
>  		fs = DA7213_SR_88200;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 96000:
>  		fs = DA7213_SR_96000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct
> snd_soc_component *component,
>  }
> 
>  /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
> -static int da7213_set_component_pll(struct snd_soc_component *component,
> -				    int pll_id, int source,
> -				    unsigned int fref, unsigned int fout)
> +static int _da7213_set_component_pll(struct snd_soc_component
> *component,
> +				     int pll_id, int source,
> +				     unsigned int fref, unsigned int fout)
>  {
>  	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> 
> @@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct
> snd_soc_component *component,
>  	return 0;
>  }
> 
> +static int da7213_set_component_pll(struct snd_soc_component *component,
> +				    int pll_id, int source,
> +				    unsigned int fref, unsigned int fout)
> +{
> +	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> +	da7213->fixed_clk_auto_pll = false;
> +
> +	return _da7213_set_component_pll(component, pll_id, source, fref,
> fout);
> +}
> +
>  /* DAI operations */
>  static const struct snd_soc_dai_ops da7213_dai_ops = {
>  	.hw_params	= da7213_hw_params,
> @@ -1532,6 +1553,43 @@ static struct snd_soc_dai_driver da7213_dai = {
>  	.symmetric_rates = 1,
>  };
> 
> +static int da7213_set_auto_pll(struct snd_soc_component *component, bool
> enable)
> +{
> +	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> +	int mode;
> +
> +	if (!da7213->fixed_clk_auto_pll)
> +		return 0;
> +
> +	da7213->mclk_rate = clk_get_rate(da7213->mclk);
> +
> +	if (enable)
> +		mode = DA7213_SYSCLK_PLL;
> +	else
> +		mode = DA7213_SYSCLK_MCLK;

If we're the clock slave, and we're using an MCLK that's not a harmonic then
SRM is required to synchronise the PLL to the incoming WCLK signal. I assume
simple sound card should allow for both master and slave modes? If so we'll
need to do something here to determine this as well.

> +
> +	switch (da7213->out_rate) {
> +	case DA7213_PLL_FREQ_OUT_90316800:
> +		if (da7213->mclk_rate == 11289600 ||
> +		    da7213->mclk_rate == 22579200 ||
> +		    da7213->mclk_rate == 45158400)
> +			mode = DA7213_SYSCLK_MCLK;
> +		break;
> +	case DA7213_PLL_FREQ_OUT_98304000:
> +		if (da7213->mclk_rate == 12288000 ||
> +		    da7213->mclk_rate == 24576000 ||
> +		    da7213->mclk_rate == 49152000)
> +			mode = DA7213_SYSCLK_MCLK;
> +
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +	return _da7213_set_component_pll(component, 0, mode,
> +					 da7213->mclk_rate, da7213->out_rate);
> +}
> +
>  static int da7213_set_bias_level(struct snd_soc_component *component,
>  				 enum snd_soc_bias_level level)
>  {
> @@ -1551,6 +1609,8 @@ static int da7213_set_bias_level(struct
> snd_soc_component *component,
>  						"Failed to enable mclk\n");
>  					return ret;
>  				}
> +
> +				da7213_set_auto_pll(component, true);

I've thought more about this and for the scenario where a machine driver
controls the PLL through a DAPM widget associated with this codec (like some
Intel boards do), then the PLL will be configured once here and then again
when the relevant widget is called. I don't think that will matter but I will
take a further look just in case this might cause some oddities. 

>  			}
>  		}
>  		break;
> @@ -1562,8 +1622,10 @@ static int da7213_set_bias_level(struct
> snd_soc_component *component,
>  					    DA7213_VMID_EN | DA7213_BIAS_EN);
>  		} else {
>  			/* Remove MCLK */
> -			if (da7213->mclk)
> +			if (da7213->mclk) {
> +				da7213_set_auto_pll(component, false);
>  				clk_disable_unprepare(da7213->mclk);
> +			}
>  		}
>  		break;
>  	case SND_SOC_BIAS_OFF:
> @@ -1829,6 +1891,11 @@ static int da7213_probe(struct snd_soc_component
> *component)
>  			return PTR_ERR(da7213->mclk);
>  		else
>  			da7213->mclk = NULL;
> +	} else {
> +		/* Do automatic PLL handling assuming fixed clock until
> +		 * set_pll() has been called. This makes the codec usable
> +		 * with the simple-audio-card driver. */
> +		da7213->fixed_clk_auto_pll = true;
>  	}
> 
>  	return 0;
> diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> index 3890829dfb6e..97ccf0ddd2be 100644
> --- a/sound/soc/codecs/da7213.h
> +++ b/sound/soc/codecs/da7213.h
> @@ -535,10 +535,12 @@ struct da7213_priv {
>  	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
>  	struct clk *mclk;
>  	unsigned int mclk_rate;
> +	unsigned int out_rate;
>  	int clk_src;
>  	bool master;
>  	bool alc_calib_auto;
>  	bool alc_en;
> +	bool fixed_clk_auto_pll;
>  	struct da7213_platform_data *pdata;
>  };
> 
> --
> 2.24.0

RE: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Adam Thomson]

On 21 November 2019 21:49, Adam Thomson wrote:

> On 20 November 2019 15:24, Sebastian Reichel wrote:
> 
> > This adds default clock/PLL configuration to the driver
> > for usage with generic drivers like simple-card for usage
> > with a fixed rate clock.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  sound/soc/codecs/da7213.c | 75
> > ++++++++++++++++++++++++++++++++++++---
> >  sound/soc/codecs/da7213.h |  2 ++
> >  2 files changed, 73 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> > index 3e6ad996741b..ff1a936240be 100644
> > --- a/sound/soc/codecs/da7213.c
> > +++ b/sound/soc/codecs/da7213.c
> > @@ -1156,6 +1156,7 @@ static int da7213_hw_params(struct
> > snd_pcm_substream *substream,
> >  			    struct snd_soc_dai *dai)
> >  {
> >  	struct snd_soc_component *component = dai->component;
> > +	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> >  	u8 dai_ctrl = 0;
> >  	u8 fs;
> >
> > @@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct
> > snd_pcm_substream *substream,
> >  	switch (params_rate(params)) {
> >  	case 8000:
> >  		fs = DA7213_SR_8000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 11025:
> >  		fs = DA7213_SR_11025;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 12000:
> >  		fs = DA7213_SR_12000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 16000:
> >  		fs = DA7213_SR_16000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 22050:
> >  		fs = DA7213_SR_22050;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 32000:
> >  		fs = DA7213_SR_32000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 44100:
> >  		fs = DA7213_SR_44100;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 48000:
> >  		fs = DA7213_SR_48000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 88200:
> >  		fs = DA7213_SR_88200;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 96000:
> >  		fs = DA7213_SR_96000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	default:
> >  		return -EINVAL;
> > @@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct
> > snd_soc_component *component,
> >  }
> >
> >  /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
> > -static int da7213_set_component_pll(struct snd_soc_component
> *component,
> > -				    int pll_id, int source,
> > -				    unsigned int fref, unsigned int fout)
> > +static int _da7213_set_component_pll(struct snd_soc_component
> > *component,
> > +				     int pll_id, int source,
> > +				     unsigned int fref, unsigned int fout)
> >  {
> >  	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> >
> > @@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct
> > snd_soc_component *component,
> >  	return 0;
> >  }
> >
> > +static int da7213_set_component_pll(struct snd_soc_component
> *component,
> > +				    int pll_id, int source,
> > +				    unsigned int fref, unsigned int fout)
> > +{
> > +	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> > +	da7213->fixed_clk_auto_pll = false;
> > +
> > +	return _da7213_set_component_pll(component, pll_id, source, fref,
> > fout);
> > +}
> > +
> >  /* DAI operations */
> >  static const struct snd_soc_dai_ops da7213_dai_ops = {
> >  	.hw_params	= da7213_hw_params,
> > @@ -1532,6 +1553,43 @@ static struct snd_soc_dai_driver da7213_dai = {
> >  	.symmetric_rates = 1,
> >  };
> >
> > +static int da7213_set_auto_pll(struct snd_soc_component *component, bool
> > enable)
> > +{
> > +	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> > +	int mode;
> > +
> > +	if (!da7213->fixed_clk_auto_pll)
> > +		return 0;
> > +
> > +	da7213->mclk_rate = clk_get_rate(da7213->mclk);
> > +
> > +	if (enable)
> > +		mode = DA7213_SYSCLK_PLL;
> > +	else
> > +		mode = DA7213_SYSCLK_MCLK;
> 
> If we're the clock slave, and we're using an MCLK that's not a harmonic then
> SRM is required to synchronise the PLL to the incoming WCLK signal. I assume
> simple sound card should allow for both master and slave modes? If so we'll
> need to do something here to determine this as well.
> 
> > +
> > +	switch (da7213->out_rate) {
> > +	case DA7213_PLL_FREQ_OUT_90316800:
> > +		if (da7213->mclk_rate == 11289600 ||
> > +		    da7213->mclk_rate == 22579200 ||
> > +		    da7213->mclk_rate == 45158400)
> > +			mode = DA7213_SYSCLK_MCLK;
> > +		break;
> > +	case DA7213_PLL_FREQ_OUT_98304000:
> > +		if (da7213->mclk_rate == 12288000 ||
> > +		    da7213->mclk_rate == 24576000 ||
> > +		    da7213->mclk_rate == 49152000)
> > +			mode = DA7213_SYSCLK_MCLK;
> > +
> > +		break;
> > +	default:
> > +		return -1;
> > +	}
> > +
> > +	return _da7213_set_component_pll(component, 0, mode,
> > +					 da7213->mclk_rate, da7213->out_rate);
> > +}
> > +
> >  static int da7213_set_bias_level(struct snd_soc_component *component,
> >  				 enum snd_soc_bias_level level)
> >  {
> > @@ -1551,6 +1609,8 @@ static int da7213_set_bias_level(struct
> > snd_soc_component *component,
> >  						"Failed to enable mclk\n");
> >  					return ret;
> >  				}
> > +
> > +				da7213_set_auto_pll(component, true);
> 
> I've thought more about this and for the scenario where a machine driver
> controls the PLL through a DAPM widget associated with this codec (like some
> Intel boards do), then the PLL will be configured once here and then again
> when the relevant widget is called. I don't think that will matter but I will
> take a further look just in case this might cause some oddities.

So I don't see any issues per say with the PLL function being called twice in
the example I mentioned. However it still feels a bit clunky; You either live
with it or you have something in the machine driver to call the codec's PLL
function early doors to prevent the bias_level() code of the codec controlling
the PLL automatically. Am wondering though if there would be some use in having
an indicator that simple-card is being used so we can avoid this? I guess we
could check the parent sound card instance's OF node to look at the compatible
string and see if it's a simple-card instantiation. Bit messy maybe.
Alternatively would it be worthwhile storing in the snd_soc_card instance that
this is a simple-card instantiation? We could then use that to determine
whether the codec driver should automatically deal with the PLL or not. This
would of course require an update to the snd_soc_card structure to add the new
flag and then some update to simple-card.c to flag this.

I think relying on the timing of the call to the codec's PLL configuration
function from a machine driver isn't ideal.

> 
> >  			}
> >  		}
> >  		break;
> > @@ -1562,8 +1622,10 @@ static int da7213_set_bias_level(struct
> > snd_soc_component *component,
> >  					    DA7213_VMID_EN | DA7213_BIAS_EN);
> >  		} else {
> >  			/* Remove MCLK */
> > -			if (da7213->mclk)
> > +			if (da7213->mclk) {
> > +				da7213_set_auto_pll(component, false);
> >  				clk_disable_unprepare(da7213->mclk);
> > +			}
> >  		}
> >  		break;
> >  	case SND_SOC_BIAS_OFF:
> > @@ -1829,6 +1891,11 @@ static int da7213_probe(struct snd_soc_component
> > *component)
> >  			return PTR_ERR(da7213->mclk);
> >  		else
> >  			da7213->mclk = NULL;
> > +	} else {
> > +		/* Do automatic PLL handling assuming fixed clock until
> > +		 * set_pll() has been called. This makes the codec usable
> > +		 * with the simple-audio-card driver. */
> > +		da7213->fixed_clk_auto_pll = true;
> >  	}
> >
> >  	return 0;
> > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> > index 3890829dfb6e..97ccf0ddd2be 100644
> > --- a/sound/soc/codecs/da7213.h
> > +++ b/sound/soc/codecs/da7213.h
> > @@ -535,10 +535,12 @@ struct da7213_priv {
> >  	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
> >  	struct clk *mclk;
> >  	unsigned int mclk_rate;
> > +	unsigned int out_rate;
> >  	int clk_src;
> >  	bool master;
> >  	bool alc_calib_auto;
> >  	bool alc_en;
> > +	bool fixed_clk_auto_pll;
> >  	struct da7213_platform_data *pdata;
> >  };
> >
> > --
> > 2.24.0

Re: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Mark Brown]

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

On Tue, Nov 26, 2019 at 04:55:39PM +0000, Adam Thomson wrote:
> On 21 November 2019 21:49, Adam Thomson wrote:
> > On 20 November 2019 15:24, Sebastian Reichel wrote:

> > I've thought more about this and for the scenario where a machine driver
> > controls the PLL through a DAPM widget associated with this codec (like some
> > Intel boards do), then the PLL will be configured once here and then again
> > when the relevant widget is called. I don't think that will matter but I will
> > take a further look just in case this might cause some oddities.

> So I don't see any issues per say with the PLL function being called twice in
> the example I mentioned. However it still feels a bit clunky; You either live
> with it or you have something in the machine driver to call the codec's PLL
> function early doors to prevent the bias_level() code of the codec controlling
> the PLL automatically. Am wondering though if there would be some use in having
> an indicator that simple-card is being used so we can avoid this? I guess we

If we're special casing simple-card we're doing it wrong - there's
nothing stopping any other machine driver behaving in the same way.

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

RE: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Adam Thomson]

On 26 November 2019 17:09, Mark Brown wrote:

> On Tue, Nov 26, 2019 at 04:55:39PM +0000, Adam Thomson wrote:
> > On 21 November 2019 21:49, Adam Thomson wrote:
> > > On 20 November 2019 15:24, Sebastian Reichel wrote:
> 
> > > I've thought more about this and for the scenario where a machine driver
> > > controls the PLL through a DAPM widget associated with this codec (like some
> > > Intel boards do), then the PLL will be configured once here and then again
> > > when the relevant widget is called. I don't think that will matter but I will
> > > take a further look just in case this might cause some oddities.
> 
> > So I don't see any issues per say with the PLL function being called twice in
> > the example I mentioned. However it still feels a bit clunky; You either live
> > with it or you have something in the machine driver to call the codec's PLL
> > function early doors to prevent the bias_level() code of the codec controlling
> > the PLL automatically. Am wondering though if there would be some use in
> having
> > an indicator that simple-card is being used so we can avoid this? I guess we
> 
> If we're special casing simple-card we're doing it wrong - there's
> nothing stopping any other machine driver behaving in the same way.

Ok, what's being proposed here is for the codec to automatically control the PLL
rather than leaving it to the machine driver as is the case right now. In the
possible scenario where this is done using a card level widget to enable/disable
the PLL we will conflict with that using the current suggested approach for the
da7213 driver, albeit with no real consequence other than configuring the PLL
twice the first time a stream is started. It's a case of how to determine who's
in control of the PLL here; machine driver or codec?

Re: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Mark Brown]

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

On Tue, Nov 26, 2019 at 05:39:45PM +0000, Adam Thomson wrote:
> On 26 November 2019 17:09, Mark Brown wrote:

> > If we're special casing simple-card we're doing it wrong - there's
> > nothing stopping any other machine driver behaving in the same way.

> Ok, what's being proposed here is for the codec to automatically control the PLL
> rather than leaving it to the machine driver as is the case right now. In the
> possible scenario where this is done using a card level widget to enable/disable

Wasn't the proposal to add a new mode where the machine driver *could*
choose to delgate PLL selection to the CODEC driver rather than do so
unconditionally?  

> the PLL we will conflict with that using the current suggested approach for the
> da7213 driver, albeit with no real consequence other than configuring the PLL
> twice the first time a stream is started. It's a case of how to determine who's
> in control of the PLL here; machine driver or codec?

The patch looked like the idea was that as soon as the machine driver
configures the PLL the CODEC driver will stop touching it which looked
like a reasonable way of doing it, you could also do it with an explicit
SYSCLK value (which would have to be 0 for generic machine drivers to
pick it up) but this works just as well and may even be better.  Perhaps
I misread it though.

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

RE: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Adam Thomson]

On 26 November 2019 17:51, Mark Brown wrote:

> On Tue, Nov 26, 2019 at 05:39:45PM +0000, Adam Thomson wrote:
> > On 26 November 2019 17:09, Mark Brown wrote:
>
> > > If we're special casing simple-card we're doing it wrong - there's
> > > nothing stopping any other machine driver behaving in the same way.
>
> > Ok, what's being proposed here is for the codec to automatically control the PLL
> > rather than leaving it to the machine driver as is the case right now. In the
> > possible scenario where this is done using a card level widget to enable/disable
>
> Wasn't the proposal to add a new mode where the machine driver *could*
> choose to delgate PLL selection to the CODEC driver rather than do so
> unconditionally?

Yes, but by default the assumption is the codec will do things automatically
until the machine driver calls the relevant PLL config code.....

>
> > the PLL we will conflict with that using the current suggested approach for the
> > da7213 driver, albeit with no real consequence other than configuring the PLL
> > twice the first time a stream is started. It's a case of how to determine who's
> > in control of the PLL here; machine driver or codec?
>
> The patch looked like the idea was that as soon as the machine driver
> configures the PLL the CODEC driver will stop touching it which looked
> like a reasonable way of doing it, you could also do it with an explicit
> SYSCLK value (which would have to be 0 for generic machine drivers to
> pick it up) but this works just as well and may even be better.  Perhaps
> I misread it though.

...so yes you're right in your comment here. However the bias_level code is
called prior to the DAPM paths being sequenced. If a dedicated machine driver
were to want to control the PLL via a DAPM widget at the card level (which is
done for other codecs for example on Intel platforms), the code in the
bias_level() function of the codec to auto configure the PLL would always be
called the very first time a path is enabled before the relevant DAPM widget for
the card is called, so you'd have two configurations of the PLL in succession. I
don't expect that would cause issues, but it's not ideal. The other approach
would be to have something in the machine driver like a dai_link init function
to default the PLL config using the codec's PLL function which then prevents
any chance of auto configuration subsequently. However that requires the person
implementing the machine driver to know about this behaviour.

As I said it's a small thing and requires a specific use-case to occur, but
having the PLL configured twice for the very first stream in that scenario
seems messy. Regarding the SYSCLK approach you mention, I'm not clear how that
would work so I'm obviously missing something. If we had some init stage
indication though that auto PLL was required then we're golden.

Re: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Mark Brown]

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

On Wed, Nov 27, 2019 at 11:32:54AM +0000, Adam Thomson wrote:

> As I said it's a small thing and requires a specific use-case to occur, but
> having the PLL configured twice for the very first stream in that scenario
> seems messy. Regarding the SYSCLK approach you mention, I'm not clear how that
> would work so I'm obviously missing something. If we had some init stage
> indication though that auto PLL was required then we're golden.

There's a bunch of other drivers using the SYSCLK thing, when you call
set_sysclk() they provide a different SYSCLK number if they want to use
manual mode.  If there's a concern about drivers doing stuff on init you
could always ask them to set the PLL during init, even if only briefly.

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

RE: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Adam Thomson]

On 27 November 2019 12:33, Mark Brown wrote:

> On Wed, Nov 27, 2019 at 11:32:54AM +0000, Adam Thomson wrote:
> 
> > As I said it's a small thing and requires a specific use-case to occur, but
> > having the PLL configured twice for the very first stream in that scenario
> > seems messy. Regarding the SYSCLK approach you mention, I'm not clear how
> that
> > would work so I'm obviously missing something. If we had some init stage
> > indication though that auto PLL was required then we're golden.
> 
> There's a bunch of other drivers using the SYSCLK thing, when you call
> set_sysclk() they provide a different SYSCLK number if they want to use
> manual mode.  If there's a concern about drivers doing stuff on init you
> could always ask them to set the PLL during init, even if only briefly.

Yeah OK I see what you mean; using the sysclk id. Still doesn't feel like the
nicest solution here though. I guess we're stuck with people having to manually
configure the PLL for bypass when a non-generic machine driver inits, to avoid
the initial double config, as I don't see other options unless you have
something to specify at init that it's auto or manual, and this doesn't feel
like a DT device specific property thing as it's more software than hardware.
At least Sebastian's patch has a good comment block to highlight this.

Re: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Mark Brown]

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

On Wed, Nov 27, 2019 at 01:42:54PM +0000, Adam Thomson wrote:

> nicest solution here though. I guess we're stuck with people having to manually
> configure the PLL for bypass when a non-generic machine driver inits, to avoid
> the initial double config, as I don't see other options unless you have
> something to specify at init that it's auto or manual, and this doesn't feel
> like a DT device specific property thing as it's more software than hardware.
> At least Sebastian's patch has a good comment block to highlight this.

Not sure I follow here - if we're configuring the PLL explicitly then
I'd expect the PLL to be configured first, then the SYSCLK, so I'd
expect that the automatic PLL configuration wouldn't kick in.

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

RE: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Adam Thomson]

On 27 November 2019 15:41, Mark Brown wrote:

> On Wed, Nov 27, 2019 at 01:42:54PM +0000, Adam Thomson wrote:
> 
> > nicest solution here though. I guess we're stuck with people having to manually
> > configure the PLL for bypass when a non-generic machine driver inits, to avoid
> > the initial double config, as I don't see other options unless you have
> > something to specify at init that it's auto or manual, and this doesn't feel
> > like a DT device specific property thing as it's more software than hardware.
> > At least Sebastian's patch has a good comment block to highlight this.
> 
> Not sure I follow here - if we're configuring the PLL explicitly then
> I'd expect the PLL to be configured first, then the SYSCLK, so I'd
> expect that the automatic PLL configuration wouldn't kick in.

The PLL in the codec relies on MCLK. The MCLK rate can be specified/configured
by a machine driver using the relevant codec sysclk function, as is done in a
number of drivers. Surely that has to happen first before we configure the PLL
as the PLL functions needs to know what rate is coming in so the correct
dividers can be applied for the required internal clocking to match up with the
desired sample rates. I guess I'm still missing something regarding your
discussion around SYSCLK?

Re: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Mark Brown]

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

On Wed, Nov 27, 2019 at 04:33:12PM +0000, Adam Thomson wrote:
> On 27 November 2019 15:41, Mark Brown wrote:

> > Not sure I follow here - if we're configuring the PLL explicitly then
> > I'd expect the PLL to be configured first, then the SYSCLK, so I'd
> > expect that the automatic PLL configuration wouldn't kick in.

> The PLL in the codec relies on MCLK. The MCLK rate can be specified/configured
> by a machine driver using the relevant codec sysclk function, as is done in a
> number of drivers. Surely that has to happen first before we configure the PLL
> as the PLL functions needs to know what rate is coming in so the correct
> dividers can be applied for the required internal clocking to match up with the
> desired sample rates. I guess I'm still missing something regarding your
> discussion around SYSCLK?

The PLL configuration specifies both input and output clock rates (as
well as an input clock source) so if it's got to configure the MCLK I'd
expect the driver to figure that out without needing the caller to
separately set the MCLK rate.

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

RE: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Adam Thomson]

On 27 November 2019 16:41, Mark Brown wrote:

> > The PLL in the codec relies on MCLK. The MCLK rate can be specified/configured
> > by a machine driver using the relevant codec sysclk function, as is done in a
> > number of drivers. Surely that has to happen first before we configure the PLL
> > as the PLL functions needs to know what rate is coming in so the correct
> > dividers can be applied for the required internal clocking to match up with the
> > desired sample rates. I guess I'm still missing something regarding your
> > discussion around SYSCLK?
>
> The PLL configuration specifies both input and output clock rates (as
> well as an input clock source) so if it's got to configure the MCLK I'd
> expect the driver to figure that out without needing the caller to
> separately set the MCLK rate.

Yes it does but the name of the function implies it's setting the codec's PLL,
not the system clock, whereas the other function implies setting the system
clock and not the PLL. Also generally you're only setting the sysclk once
whereas you may want to configure and enable/disable the PLL more dynamically,
at least for devices which do have a built-in PLL. Of course that could still be
handled through the single PLL function call.

Just as an informational, what's the future for these two functions if
essentially one is only really required and the other deemed redundant? I would
just like to be clear so I'm not falling over things like this in the future,
and wasting your time as well. Seems that the PLL call isn't part of simple
generic card code so would the be deemed surplus to requirements some point down
the line?

Re: [PATCHv2 6/6] ASoC: da7213: Add default clock handling

[Mark Brown]

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

On Wed, Nov 27, 2019 at 06:10:00PM +0000, Adam Thomson wrote:
> On 27 November 2019 16:41, Mark Brown wrote:

> > The PLL configuration specifies both input and output clock rates (as
> > well as an input clock source) so if it's got to configure the MCLK I'd
> > expect the driver to figure that out without needing the caller to
> > separately set the MCLK rate.

> Yes it does but the name of the function implies it's setting the codec's PLL,
> not the system clock, whereas the other function implies setting the system
> clock and not the PLL. Also generally you're only setting the sysclk once
> whereas you may want to configure and enable/disable the PLL more dynamically,
> at least for devices which do have a built-in PLL. Of course that could still be
> handled through the single PLL function call.

I wouldn't be so sure about only setting the SYSCLK once - if you've got
an input clock you can configure then you might for example want to
switch between 44.1kHz and 48kHz bases, and disable it or run it at very
low frequency when idle.  In some systems it's as dynamic as a PLL is.

> Just as an informational, what's the future for these two functions if
> essentially one is only really required and the other deemed redundant? I would
> just like to be clear so I'm not falling over things like this in the future,
> and wasting your time as well. Seems that the PLL call isn't part of simple
> generic card code so would the be deemed surplus to requirements some point down
> the line?

Things like simple-card are good for 90% of systems but I'm fairly sure
we'll never be able to handle more complex setups entirely
automatically.  What we *should* be doing over time is transitioning all
this clock stuff into the actual clock framework but that's a big, long
term thing which nobody is currently actually working on.

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