* [alsa-devel] [PATCHv1 0/5] ASoC: da7213: support for usage with simple-card
@ 2019-11-08 17:48 ` Sebastian Reichel
0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: alsa-devel, Takashi Iwai, linux-kernel, Sebastian Reichel, kernel
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 = <®_2v5_audio>;
VDDSP-supply = <®_5v0_audio>;
VDDMIC-supply = <®_3v3_audio>;
VDDIO-supply = <®_3v3_audio>;
clocks = <&clk_ext_audio_codec>;
clock-names = "mclk";
};
};
---------------------------------------------------------------------
-- Sebastian
Sebastian Reichel (5):
ASoC: da7213: Add regulator support
ASoC: Add DA7213 audio codec as 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 | 4 +
sound/soc/codecs/Kconfig | 3 +-
sound/soc/codecs/da7213.c | 128 ++++++++++++++++--
sound/soc/codecs/da7213.h | 3 +
4 files changed, 125 insertions(+), 13 deletions(-)
--
2.24.0.rc1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCHv1 1/5] ASoC: da7213: Add regulator support
2019-11-08 17:48 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-08 17:48 ` Sebastian Reichel
-1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel, kernel,
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 | 72 +++++++++++++++++++
sound/soc/codecs/da7213.h | 2 +
3 files changed, 78 insertions(+)
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..36e5a7c9ac33 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,12 @@ 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("VDDA", 0, 0),
+ SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
+
/*
* Input & Output
*/
@@ -931,7 +938,16 @@ static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
static const struct snd_soc_dapm_route da7213_audio_map[] = {
/* Dest Connecting Widget source */
+ /* Main Power Supply */
+ {"DAC Left", NULL, "VDDA"},
+ {"DAC Right", NULL, "VDDA"},
+ {"ADC Left", NULL, "VDDA"},
+ {"ADC Right", NULL, "VDDA"},
+
/* Input path */
+ {"Mic Bias 1", NULL, "VDDMIC"},
+ {"Mic Bias 2", NULL, "VDDMIC"},
+
{"MIC1", NULL, "Mic Bias 1"},
{"MIC2", NULL, "Mic Bias 2"},
@@ -1691,6 +1707,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 +1829,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,6 +1868,12 @@ 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_disable(da7213->vddio);
+}
+
static int da7213_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
@@ -1860,6 +1886,18 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, da7213);
+ da7213->vddio = devm_regulator_get(&i2c->dev, "VDDIO");
+ if (IS_ERR(da7213->vddio))
+ return PTR_ERR(da7213->vddio);
+
+ ret = regulator_enable(da7213->vddio);
+ 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 +1905,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 +1919,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_disable(da7213->vddio);
+
+ return 0;
+}
+
+static int __maybe_unused da7213_runtime_resume(struct device *dev)
+{
+ struct da7213_priv *da7213 = dev_get_drvdata(dev);
+ int ret;
+
+ ret = regulator_enable(da7213->vddio);
+ 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 +1959,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..97a250ea39e6 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>
/*
@@ -524,6 +525,7 @@ enum da7213_sys_clk {
/* Codec private data */
struct da7213_priv {
struct regmap *regmap;
+ struct regulator *vddio;
struct clk *mclk;
unsigned int mclk_rate;
int clk_src;
--
2.24.0.rc1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [alsa-devel] [PATCHv1 1/5] ASoC: da7213: Add regulator support
@ 2019-11-08 17:48 ` Sebastian Reichel
0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: alsa-devel, Takashi Iwai, linux-kernel, Sebastian Reichel, kernel
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 | 72 +++++++++++++++++++
sound/soc/codecs/da7213.h | 2 +
3 files changed, 78 insertions(+)
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..36e5a7c9ac33 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,12 @@ 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("VDDA", 0, 0),
+ SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
+
/*
* Input & Output
*/
@@ -931,7 +938,16 @@ static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
static const struct snd_soc_dapm_route da7213_audio_map[] = {
/* Dest Connecting Widget source */
+ /* Main Power Supply */
+ {"DAC Left", NULL, "VDDA"},
+ {"DAC Right", NULL, "VDDA"},
+ {"ADC Left", NULL, "VDDA"},
+ {"ADC Right", NULL, "VDDA"},
+
/* Input path */
+ {"Mic Bias 1", NULL, "VDDMIC"},
+ {"Mic Bias 2", NULL, "VDDMIC"},
+
{"MIC1", NULL, "Mic Bias 1"},
{"MIC2", NULL, "Mic Bias 2"},
@@ -1691,6 +1707,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 +1829,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,6 +1868,12 @@ 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_disable(da7213->vddio);
+}
+
static int da7213_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
@@ -1860,6 +1886,18 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, da7213);
+ da7213->vddio = devm_regulator_get(&i2c->dev, "VDDIO");
+ if (IS_ERR(da7213->vddio))
+ return PTR_ERR(da7213->vddio);
+
+ ret = regulator_enable(da7213->vddio);
+ 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 +1905,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 +1919,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_disable(da7213->vddio);
+
+ return 0;
+}
+
+static int __maybe_unused da7213_runtime_resume(struct device *dev)
+{
+ struct da7213_priv *da7213 = dev_get_drvdata(dev);
+ int ret;
+
+ ret = regulator_enable(da7213->vddio);
+ 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 +1959,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..97a250ea39e6 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>
/*
@@ -524,6 +525,7 @@ enum da7213_sys_clk {
/* Codec private data */
struct da7213_priv {
struct regmap *regmap;
+ struct regulator *vddio;
struct clk *mclk;
unsigned int mclk_rate;
int clk_src;
--
2.24.0.rc1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread* RE: [PATCHv1 1/5] ASoC: da7213: Add regulator support
2019-11-08 17:48 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-11 14:07 ` Adam Thomson
-1 siblings, 0 replies; 26+ messages in thread
From: Adam Thomson @ 2019-11-11 14:07 UTC (permalink / raw)
To: Sebastian Reichel, Support Opensource, Liam Girdwood, Mark Brown
Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel, kernel
On 08 November 2019 17:49, 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 | 72 +++++++++++++++++++
> sound/soc/codecs/da7213.h | 2 +
> 3 files changed, 78 insertions(+)
>
> 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..36e5a7c9ac33 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,12 @@ 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("VDDA", 0, 0),
Having spoken with our HW team, this will cause a POR in the device so we can't
just enable/disable VDD_A supply. Needs to present at all times. How are you
verifying this?
> + SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
> +
> /*
> * Input & Output
> */
> @@ -931,7 +938,16 @@ static const struct snd_soc_dapm_widget
> da7213_dapm_widgets[] = {
> static const struct snd_soc_dapm_route da7213_audio_map[] = {
> /* Dest Connecting Widget source */
>
> + /* Main Power Supply */
> + {"DAC Left", NULL, "VDDA"},
> + {"DAC Right", NULL, "VDDA"},
> + {"ADC Left", NULL, "VDDA"},
> + {"ADC Right", NULL, "VDDA"},
> +
> /* Input path */
> + {"Mic Bias 1", NULL, "VDDMIC"},
> + {"Mic Bias 2", NULL, "VDDMIC"},
> +
> {"MIC1", NULL, "Mic Bias 1"},
> {"MIC2", NULL, "Mic Bias 2"},
>
> @@ -1691,6 +1707,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 +1829,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,6 +1868,12 @@ 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_disable(da7213->vddio);
> +}
> +
> static int da7213_i2c_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> {
> @@ -1860,6 +1886,18 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
>
> i2c_set_clientdata(i2c, da7213);
>
> + da7213->vddio = devm_regulator_get(&i2c->dev, "VDDIO");
> + if (IS_ERR(da7213->vddio))
> + return PTR_ERR(da7213->vddio);
> +
> + ret = regulator_enable(da7213->vddio);
> + if (ret < 0)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> da7213);
> + if (ret < 0)
> + return ret;
We're seemingly leaving the VDDIO regulator enabled on failure, unless I'm
missing some magic somewhere?
> +
> da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config);
> if (IS_ERR(da7213->regmap)) {
> ret = PTR_ERR(da7213->regmap);
> @@ -1867,6 +1905,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 +1919,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_disable(da7213->vddio);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused da7213_runtime_resume(struct device *dev)
> +{
> + struct da7213_priv *da7213 = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = regulator_enable(da7213->vddio);
> + 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 +1959,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..97a250ea39e6 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>
>
> /*
> @@ -524,6 +525,7 @@ enum da7213_sys_clk {
> /* Codec private data */
> struct da7213_priv {
> struct regmap *regmap;
> + struct regulator *vddio;
> struct clk *mclk;
> unsigned int mclk_rate;
> int clk_src;
> --
> 2.24.0.rc1
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [alsa-devel] [PATCHv1 1/5] ASoC: da7213: Add regulator support
@ 2019-11-11 14:07 ` Adam Thomson
0 siblings, 0 replies; 26+ messages in thread
From: Adam Thomson @ 2019-11-11 14:07 UTC (permalink / raw)
To: Sebastian Reichel, Support Opensource, Liam Girdwood, Mark Brown
Cc: linux-kernel, alsa-devel, kernel, Takashi Iwai
On 08 November 2019 17:49, 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 | 72 +++++++++++++++++++
> sound/soc/codecs/da7213.h | 2 +
> 3 files changed, 78 insertions(+)
>
> 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..36e5a7c9ac33 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,12 @@ 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("VDDA", 0, 0),
Having spoken with our HW team, this will cause a POR in the device so we can't
just enable/disable VDD_A supply. Needs to present at all times. How are you
verifying this?
> + SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
> +
> /*
> * Input & Output
> */
> @@ -931,7 +938,16 @@ static const struct snd_soc_dapm_widget
> da7213_dapm_widgets[] = {
> static const struct snd_soc_dapm_route da7213_audio_map[] = {
> /* Dest Connecting Widget source */
>
> + /* Main Power Supply */
> + {"DAC Left", NULL, "VDDA"},
> + {"DAC Right", NULL, "VDDA"},
> + {"ADC Left", NULL, "VDDA"},
> + {"ADC Right", NULL, "VDDA"},
> +
> /* Input path */
> + {"Mic Bias 1", NULL, "VDDMIC"},
> + {"Mic Bias 2", NULL, "VDDMIC"},
> +
> {"MIC1", NULL, "Mic Bias 1"},
> {"MIC2", NULL, "Mic Bias 2"},
>
> @@ -1691,6 +1707,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 +1829,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,6 +1868,12 @@ 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_disable(da7213->vddio);
> +}
> +
> static int da7213_i2c_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> {
> @@ -1860,6 +1886,18 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
>
> i2c_set_clientdata(i2c, da7213);
>
> + da7213->vddio = devm_regulator_get(&i2c->dev, "VDDIO");
> + if (IS_ERR(da7213->vddio))
> + return PTR_ERR(da7213->vddio);
> +
> + ret = regulator_enable(da7213->vddio);
> + if (ret < 0)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> da7213);
> + if (ret < 0)
> + return ret;
We're seemingly leaving the VDDIO regulator enabled on failure, unless I'm
missing some magic somewhere?
> +
> da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config);
> if (IS_ERR(da7213->regmap)) {
> ret = PTR_ERR(da7213->regmap);
> @@ -1867,6 +1905,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 +1919,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_disable(da7213->vddio);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused da7213_runtime_resume(struct device *dev)
> +{
> + struct da7213_priv *da7213 = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = regulator_enable(da7213->vddio);
> + 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 +1959,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..97a250ea39e6 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>
>
> /*
> @@ -524,6 +525,7 @@ enum da7213_sys_clk {
> /* Codec private data */
> struct da7213_priv {
> struct regmap *regmap;
> + struct regulator *vddio;
> struct clk *mclk;
> unsigned int mclk_rate;
> int clk_src;
> --
> 2.24.0.rc1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCHv1 1/5] ASoC: da7213: Add regulator support
2019-11-11 14:07 ` [alsa-devel] " Adam Thomson
@ 2019-11-12 15:24 ` Sebastian Reichel
-1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-12 15:24 UTC (permalink / raw)
To: Adam Thomson
Cc: Support Opensource, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, alsa-devel, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 7597 bytes --]
Hi,
On Mon, Nov 11, 2019 at 02:07:46PM +0000, Adam Thomson wrote:
> On 08 November 2019 17:49, 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 | 72 +++++++++++++++++++
> > sound/soc/codecs/da7213.h | 2 +
> > 3 files changed, 78 insertions(+)
> >
> > 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..36e5a7c9ac33 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,12 @@ 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("VDDA", 0, 0),
>
> Having spoken with our HW team, this will cause a POR in the device so we can't
> just enable/disable VDD_A supply. Needs to present at all times. How are you
> verifying this?
Ok. The system, that I used for testing shared a regulator
for VDDIO and VDDA. I suppose this needs to be moved next
to enabling VDDIO then.
> > + SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
> > +
> > /*
> > * Input & Output
> > */
> > @@ -931,7 +938,16 @@ static const struct snd_soc_dapm_widget
> > da7213_dapm_widgets[] = {
> > static const struct snd_soc_dapm_route da7213_audio_map[] = {
> > /* Dest Connecting Widget source */
> >
> > + /* Main Power Supply */
> > + {"DAC Left", NULL, "VDDA"},
> > + {"DAC Right", NULL, "VDDA"},
> > + {"ADC Left", NULL, "VDDA"},
> > + {"ADC Right", NULL, "VDDA"},
> > +
> > /* Input path */
> > + {"Mic Bias 1", NULL, "VDDMIC"},
> > + {"Mic Bias 2", NULL, "VDDMIC"},
> > +
> > {"MIC1", NULL, "Mic Bias 1"},
> > {"MIC2", NULL, "Mic Bias 2"},
> >
> > @@ -1691,6 +1707,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 +1829,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,6 +1868,12 @@ 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_disable(da7213->vddio);
> > +}
> > +
> > static int da7213_i2c_probe(struct i2c_client *i2c,
> > const struct i2c_device_id *id)
> > {
> > @@ -1860,6 +1886,18 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> >
> > i2c_set_clientdata(i2c, da7213);
> >
> > + da7213->vddio = devm_regulator_get(&i2c->dev, "VDDIO");
> > + if (IS_ERR(da7213->vddio))
> > + return PTR_ERR(da7213->vddio);
> > +
> > + ret = regulator_enable(da7213->vddio);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> > da7213);
> > + if (ret < 0)
> > + return ret;
>
> We're seemingly leaving the VDDIO regulator enabled on failure, unless I'm
> missing some magic somewhere?
If regulator_enable fails, the regulator is not enabled. If devm_add_action_or_reset
fails, it will call da7213_power_off().
> > da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config);
> > if (IS_ERR(da7213->regmap)) {
> > ret = PTR_ERR(da7213->regmap);
> > @@ -1867,6 +1905,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 +1919,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_disable(da7213->vddio);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused da7213_runtime_resume(struct device *dev)
> > +{
> > + struct da7213_priv *da7213 = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = regulator_enable(da7213->vddio);
> > + 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 +1959,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..97a250ea39e6 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>
> >
> > /*
> > @@ -524,6 +525,7 @@ enum da7213_sys_clk {
> > /* Codec private data */
> > struct da7213_priv {
> > struct regmap *regmap;
> > + struct regulator *vddio;
> > struct clk *mclk;
> > unsigned int mclk_rate;
> > int clk_src;
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [alsa-devel] [PATCHv1 1/5] ASoC: da7213: Add regulator support
@ 2019-11-12 15:24 ` Sebastian Reichel
0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-12 15:24 UTC (permalink / raw)
To: Adam Thomson
Cc: alsa-devel, Support Opensource, Liam Girdwood, linux-kernel,
Takashi Iwai, Mark Brown, kernel
[-- Attachment #1.1: Type: text/plain, Size: 7597 bytes --]
Hi,
On Mon, Nov 11, 2019 at 02:07:46PM +0000, Adam Thomson wrote:
> On 08 November 2019 17:49, 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 | 72 +++++++++++++++++++
> > sound/soc/codecs/da7213.h | 2 +
> > 3 files changed, 78 insertions(+)
> >
> > 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..36e5a7c9ac33 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,12 @@ 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("VDDA", 0, 0),
>
> Having spoken with our HW team, this will cause a POR in the device so we can't
> just enable/disable VDD_A supply. Needs to present at all times. How are you
> verifying this?
Ok. The system, that I used for testing shared a regulator
for VDDIO and VDDA. I suppose this needs to be moved next
to enabling VDDIO then.
> > + SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
> > +
> > /*
> > * Input & Output
> > */
> > @@ -931,7 +938,16 @@ static const struct snd_soc_dapm_widget
> > da7213_dapm_widgets[] = {
> > static const struct snd_soc_dapm_route da7213_audio_map[] = {
> > /* Dest Connecting Widget source */
> >
> > + /* Main Power Supply */
> > + {"DAC Left", NULL, "VDDA"},
> > + {"DAC Right", NULL, "VDDA"},
> > + {"ADC Left", NULL, "VDDA"},
> > + {"ADC Right", NULL, "VDDA"},
> > +
> > /* Input path */
> > + {"Mic Bias 1", NULL, "VDDMIC"},
> > + {"Mic Bias 2", NULL, "VDDMIC"},
> > +
> > {"MIC1", NULL, "Mic Bias 1"},
> > {"MIC2", NULL, "Mic Bias 2"},
> >
> > @@ -1691,6 +1707,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 +1829,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,6 +1868,12 @@ 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_disable(da7213->vddio);
> > +}
> > +
> > static int da7213_i2c_probe(struct i2c_client *i2c,
> > const struct i2c_device_id *id)
> > {
> > @@ -1860,6 +1886,18 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> >
> > i2c_set_clientdata(i2c, da7213);
> >
> > + da7213->vddio = devm_regulator_get(&i2c->dev, "VDDIO");
> > + if (IS_ERR(da7213->vddio))
> > + return PTR_ERR(da7213->vddio);
> > +
> > + ret = regulator_enable(da7213->vddio);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> > da7213);
> > + if (ret < 0)
> > + return ret;
>
> We're seemingly leaving the VDDIO regulator enabled on failure, unless I'm
> missing some magic somewhere?
If regulator_enable fails, the regulator is not enabled. If devm_add_action_or_reset
fails, it will call da7213_power_off().
> > da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config);
> > if (IS_ERR(da7213->regmap)) {
> > ret = PTR_ERR(da7213->regmap);
> > @@ -1867,6 +1905,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 +1919,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_disable(da7213->vddio);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused da7213_runtime_resume(struct device *dev)
> > +{
> > + struct da7213_priv *da7213 = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = regulator_enable(da7213->vddio);
> > + 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 +1959,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..97a250ea39e6 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>
> >
> > /*
> > @@ -524,6 +525,7 @@ enum da7213_sys_clk {
> > /* Codec private data */
> > struct da7213_priv {
> > struct regmap *regmap;
> > + struct regulator *vddio;
> > struct clk *mclk;
> > unsigned int mclk_rate;
> > int clk_src;
-- Sebastian
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCHv1 1/5] ASoC: da7213: Add regulator support
2019-11-12 15:24 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-12 16:58 ` Mark Brown
-1 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2019-11-12 16:58 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Adam Thomson, Support Opensource, Liam Girdwood, Jaroslav Kysela,
Takashi Iwai, alsa-devel, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 511 bytes --]
On Tue, Nov 12, 2019 at 04:24:11PM +0100, Sebastian Reichel wrote:
> On Mon, Nov 11, 2019 at 02:07:46PM +0000, Adam Thomson wrote:
> > Having spoken with our HW team, this will cause a POR in the device so we can't
> > just enable/disable VDD_A supply. Needs to present at all times. How are you
> > verifying this?
> Ok. The system, that I used for testing shared a regulator
> for VDDIO and VDDA. I suppose this needs to be moved next
> to enabling VDDIO then.
regulator_bulk_enable() might be handy here.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [alsa-devel] [PATCHv1 1/5] ASoC: da7213: Add regulator support
@ 2019-11-12 16:58 ` Mark Brown
0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2019-11-12 16:58 UTC (permalink / raw)
To: Sebastian Reichel
Cc: alsa-devel, Support Opensource, Liam Girdwood, linux-kernel,
Takashi Iwai, Adam Thomson, kernel
[-- Attachment #1.1: Type: text/plain, Size: 511 bytes --]
On Tue, Nov 12, 2019 at 04:24:11PM +0100, Sebastian Reichel wrote:
> On Mon, Nov 11, 2019 at 02:07:46PM +0000, Adam Thomson wrote:
> > Having spoken with our HW team, this will cause a POR in the device so we can't
> > just enable/disable VDD_A supply. Needs to present at all times. How are you
> > verifying this?
> Ok. The system, that I used for testing shared a regulator
> for VDDIO and VDDA. I suppose this needs to be moved next
> to enabling VDDIO then.
regulator_bulk_enable() might be handy here.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCHv1 1/5] ASoC: da7213: Add regulator support
2019-11-12 15:24 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-13 10:51 ` Adam Thomson
-1 siblings, 0 replies; 26+ messages in thread
From: Adam Thomson @ 2019-11-13 10:51 UTC (permalink / raw)
To: Sebastian Reichel, Adam Thomson
Cc: Support Opensource, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, alsa-devel, linux-kernel, kernel
On 12 November 2019 15:24, Sebastian Reichel wrote:
> > > static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
> > > + /*
> > > + * Power Supply
> > > + */
> > > + SND_SOC_DAPM_REGULATOR_SUPPLY("VDDA", 0, 0),
> >
> > Having spoken with our HW team, this will cause a POR in the device so we can't
> > just enable/disable VDD_A supply. Needs to present at all times. How are you
> > verifying this?
>
> Ok. The system, that I used for testing shared a regulator
> for VDDIO and VDDA. I suppose this needs to be moved next
> to enabling VDDIO then.
Yes, and as Mark mentioned you can use the bulk enable/disable calls.
> > > + da7213->vddio = devm_regulator_get(&i2c->dev, "VDDIO");
> > > + if (IS_ERR(da7213->vddio))
> > > + return PTR_ERR(da7213->vddio);
> > > +
> > > + ret = regulator_enable(da7213->vddio);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> > > da7213);
> > > + if (ret < 0)
> > > + return ret;
> >
> > We're seemingly leaving the VDDIO regulator enabled on failure, unless I'm
> > missing some magic somewhere?
>
> If regulator_enable fails, the regulator is not enabled. If
> devm_add_action_or_reset
> fails, it will call da7213_power_off().
Right, didn't spot that as haven't seen that used before. Nice :)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [alsa-devel] [PATCHv1 1/5] ASoC: da7213: Add regulator support
@ 2019-11-13 10:51 ` Adam Thomson
0 siblings, 0 replies; 26+ messages in thread
From: Adam Thomson @ 2019-11-13 10:51 UTC (permalink / raw)
To: Sebastian Reichel, Adam Thomson
Cc: alsa-devel, Support Opensource, Liam Girdwood, linux-kernel,
Takashi Iwai, Mark Brown, kernel
On 12 November 2019 15:24, Sebastian Reichel wrote:
> > > static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
> > > + /*
> > > + * Power Supply
> > > + */
> > > + SND_SOC_DAPM_REGULATOR_SUPPLY("VDDA", 0, 0),
> >
> > Having spoken with our HW team, this will cause a POR in the device so we can't
> > just enable/disable VDD_A supply. Needs to present at all times. How are you
> > verifying this?
>
> Ok. The system, that I used for testing shared a regulator
> for VDDIO and VDDA. I suppose this needs to be moved next
> to enabling VDDIO then.
Yes, and as Mark mentioned you can use the bulk enable/disable calls.
> > > + da7213->vddio = devm_regulator_get(&i2c->dev, "VDDIO");
> > > + if (IS_ERR(da7213->vddio))
> > > + return PTR_ERR(da7213->vddio);
> > > +
> > > + ret = regulator_enable(da7213->vddio);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> > > da7213);
> > > + if (ret < 0)
> > > + return ret;
> >
> > We're seemingly leaving the VDDIO regulator enabled on failure, unless I'm
> > missing some magic somewhere?
>
> If regulator_enable fails, the regulator is not enabled. If
> devm_add_action_or_reset
> fails, it will call da7213_power_off().
Right, didn't spot that as haven't seen that used before. Nice :)
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCHv1 2/5] ASoC: Add DA7213 audio codec as selectable option
2019-11-08 17:48 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-08 17:48 ` Sebastian Reichel
-1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel, kernel,
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.rc1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [alsa-devel] [PATCHv1 2/5] ASoC: Add DA7213 audio codec as selectable option
@ 2019-11-08 17:48 ` Sebastian Reichel
0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: alsa-devel, Takashi Iwai, linux-kernel, Sebastian Reichel, kernel
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.rc1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv1 3/5] ASoC: da7213: move set_sysclk to codec level
2019-11-08 17:48 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-08 17:48 ` Sebastian Reichel
-1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel, kernel,
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 36e5a7c9ac33..c7734876e4f0 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1350,10 +1350,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;
@@ -1361,7 +1361,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;
}
@@ -1377,7 +1377,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;
}
@@ -1387,7 +1387,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;
}
@@ -1514,7 +1514,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,
};
@@ -1852,6 +1851,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.rc1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [alsa-devel] [PATCHv1 3/5] ASoC: da7213: move set_sysclk to codec level
@ 2019-11-08 17:48 ` Sebastian Reichel
0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: alsa-devel, Takashi Iwai, linux-kernel, Sebastian Reichel, kernel
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 36e5a7c9ac33..c7734876e4f0 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1350,10 +1350,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;
@@ -1361,7 +1361,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;
}
@@ -1377,7 +1377,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;
}
@@ -1387,7 +1387,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;
}
@@ -1514,7 +1514,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,
};
@@ -1852,6 +1851,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.rc1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv1 4/5] ASoC: da7213: move set_pll to codec level
2019-11-08 17:48 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-08 17:48 ` Sebastian Reichel
-1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel, kernel,
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 c7734876e4f0..197609691525 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1399,10 +1399,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;
@@ -1514,7 +1514,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,
};
@@ -1852,6 +1851,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.rc1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [alsa-devel] [PATCHv1 4/5] ASoC: da7213: move set_pll to codec level
@ 2019-11-08 17:48 ` Sebastian Reichel
0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: alsa-devel, Takashi Iwai, linux-kernel, Sebastian Reichel, kernel
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 c7734876e4f0..197609691525 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1399,10 +1399,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;
@@ -1514,7 +1514,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,
};
@@ -1852,6 +1851,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.rc1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv1 5/5] ASoC: da7213: add default clock handling
2019-11-08 17:48 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-08 17:48 ` Sebastian Reichel
-1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel, kernel,
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.
Upstreaming this requires a good way to disable the automatic
clock handling for systems doing it manually to avoid breaking
existing setups.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
sound/soc/codecs/da7213.h | 1 +
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index 197609691525..a4ed382ddfc7 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1163,6 +1163,8 @@ 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);
+ int freq = 0;
u8 dai_ctrl = 0;
u8 fs;
@@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct snd_pcm_substream *substream,
switch (params_rate(params)) {
case 8000:
fs = DA7213_SR_8000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 11025:
fs = DA7213_SR_11025;
+ freq = DA7213_PLL_FREQ_OUT_90316800;
break;
case 12000:
fs = DA7213_SR_12000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 16000:
fs = DA7213_SR_16000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 22050:
fs = DA7213_SR_22050;
+ freq = DA7213_PLL_FREQ_OUT_90316800;
break;
case 32000:
fs = DA7213_SR_32000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 44100:
fs = DA7213_SR_44100;
+ freq = DA7213_PLL_FREQ_OUT_90316800;
break;
case 48000:
fs = DA7213_SR_48000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 88200:
fs = DA7213_SR_88200;
+ freq = DA7213_PLL_FREQ_OUT_90316800;
break;
case 96000:
fs = DA7213_SR_96000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
default:
return -EINVAL;
}
+ /* setup PLL */
+ if (da7213->fixed_clk_auto) {
+ snd_soc_component_set_pll(component, 0, DA7213_SYSCLK_PLL,
+ da7213->mclk_rate, freq);
+ }
+
snd_soc_component_update_bits(component, DA7213_DAI_CTRL, DA7213_DAI_WORD_LENGTH_MASK,
dai_ctrl);
snd_soc_component_write(component, DA7213_SR, fs);
@@ -1700,10 +1718,10 @@ static struct da7213_platform_data
return pdata;
}
-
static int da7213_probe(struct snd_soc_component *component)
{
struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+ int ret;
pm_runtime_get_sync(component->dev);
@@ -1836,6 +1854,20 @@ static int da7213_probe(struct snd_soc_component *component)
return PTR_ERR(da7213->mclk);
else
da7213->mclk = NULL;
+ } else {
+ /* Store clock rate for fixed clocks for automatic PLL setup */
+ ret = clk_prepare_enable(da7213->mclk);
+ if (ret) {
+ dev_err(component->dev, "Failed to enable mclk\n");
+ return ret;
+ }
+
+ da7213->mclk_rate = clk_get_rate(da7213->mclk);
+
+ clk_disable_unprepare(da7213->mclk);
+
+ /* assume fixed clock until set_sysclk() is being called */
+ da7213->fixed_clk_auto = true;
}
return 0;
diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
index 97a250ea39e6..00aca0126cdb 100644
--- a/sound/soc/codecs/da7213.h
+++ b/sound/soc/codecs/da7213.h
@@ -532,6 +532,7 @@ struct da7213_priv {
bool master;
bool alc_calib_auto;
bool alc_en;
+ bool fixed_clk_auto;
struct da7213_platform_data *pdata;
};
--
2.24.0.rc1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [alsa-devel] [PATCHv1 5/5] ASoC: da7213: add default clock handling
@ 2019-11-08 17:48 ` Sebastian Reichel
0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:48 UTC (permalink / raw)
To: Support Opensource, Liam Girdwood, Mark Brown
Cc: alsa-devel, Takashi Iwai, linux-kernel, Sebastian Reichel, kernel
This adds default clock/PLL configuration to the driver
for usage with generic drivers like simple-card for usage
with a fixed rate clock.
Upstreaming this requires a good way to disable the automatic
clock handling for systems doing it manually to avoid breaking
existing setups.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
sound/soc/codecs/da7213.h | 1 +
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index 197609691525..a4ed382ddfc7 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1163,6 +1163,8 @@ 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);
+ int freq = 0;
u8 dai_ctrl = 0;
u8 fs;
@@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct snd_pcm_substream *substream,
switch (params_rate(params)) {
case 8000:
fs = DA7213_SR_8000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 11025:
fs = DA7213_SR_11025;
+ freq = DA7213_PLL_FREQ_OUT_90316800;
break;
case 12000:
fs = DA7213_SR_12000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 16000:
fs = DA7213_SR_16000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 22050:
fs = DA7213_SR_22050;
+ freq = DA7213_PLL_FREQ_OUT_90316800;
break;
case 32000:
fs = DA7213_SR_32000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 44100:
fs = DA7213_SR_44100;
+ freq = DA7213_PLL_FREQ_OUT_90316800;
break;
case 48000:
fs = DA7213_SR_48000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
case 88200:
fs = DA7213_SR_88200;
+ freq = DA7213_PLL_FREQ_OUT_90316800;
break;
case 96000:
fs = DA7213_SR_96000;
+ freq = DA7213_PLL_FREQ_OUT_98304000;
break;
default:
return -EINVAL;
}
+ /* setup PLL */
+ if (da7213->fixed_clk_auto) {
+ snd_soc_component_set_pll(component, 0, DA7213_SYSCLK_PLL,
+ da7213->mclk_rate, freq);
+ }
+
snd_soc_component_update_bits(component, DA7213_DAI_CTRL, DA7213_DAI_WORD_LENGTH_MASK,
dai_ctrl);
snd_soc_component_write(component, DA7213_SR, fs);
@@ -1700,10 +1718,10 @@ static struct da7213_platform_data
return pdata;
}
-
static int da7213_probe(struct snd_soc_component *component)
{
struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+ int ret;
pm_runtime_get_sync(component->dev);
@@ -1836,6 +1854,20 @@ static int da7213_probe(struct snd_soc_component *component)
return PTR_ERR(da7213->mclk);
else
da7213->mclk = NULL;
+ } else {
+ /* Store clock rate for fixed clocks for automatic PLL setup */
+ ret = clk_prepare_enable(da7213->mclk);
+ if (ret) {
+ dev_err(component->dev, "Failed to enable mclk\n");
+ return ret;
+ }
+
+ da7213->mclk_rate = clk_get_rate(da7213->mclk);
+
+ clk_disable_unprepare(da7213->mclk);
+
+ /* assume fixed clock until set_sysclk() is being called */
+ da7213->fixed_clk_auto = true;
}
return 0;
diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
index 97a250ea39e6..00aca0126cdb 100644
--- a/sound/soc/codecs/da7213.h
+++ b/sound/soc/codecs/da7213.h
@@ -532,6 +532,7 @@ struct da7213_priv {
bool master;
bool alc_calib_auto;
bool alc_en;
+ bool fixed_clk_auto;
struct da7213_platform_data *pdata;
};
--
2.24.0.rc1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread* RE: [PATCHv1 5/5] ASoC: da7213: add default clock handling
2019-11-08 17:48 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-11 14:20 ` Adam Thomson
-1 siblings, 0 replies; 26+ messages in thread
From: Adam Thomson @ 2019-11-11 14:20 UTC (permalink / raw)
To: Sebastian Reichel, Support Opensource, Liam Girdwood, Mark Brown
Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel, kernel
On 08 November 2019 17:49, 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.
>
> Upstreaming this requires a good way to disable the automatic
> clock handling for systems doing it manually to avoid breaking
> existing setups.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
> sound/soc/codecs/da7213.h | 1 +
> 2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 197609691525..a4ed382ddfc7 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1163,6 +1163,8 @@ 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);
> + int freq = 0;
> u8 dai_ctrl = 0;
> u8 fs;
>
> @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
> switch (params_rate(params)) {
> case 8000:
> fs = DA7213_SR_8000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 11025:
> fs = DA7213_SR_11025;
> + freq = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 12000:
> fs = DA7213_SR_12000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 16000:
> fs = DA7213_SR_16000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 22050:
> fs = DA7213_SR_22050;
> + freq = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 32000:
> fs = DA7213_SR_32000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 44100:
> fs = DA7213_SR_44100;
> + freq = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 48000:
> fs = DA7213_SR_48000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 88200:
> fs = DA7213_SR_88200;
> + freq = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 96000:
> fs = DA7213_SR_96000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> default:
> return -EINVAL;
> }
>
> + /* setup PLL */
> + if (da7213->fixed_clk_auto) {
> + snd_soc_component_set_pll(component, 0,
> DA7213_SYSCLK_PLL,
> + da7213->mclk_rate, freq);
> + }
> +
Are we happy with the PLL being always enabled? Seems like a power drain,
especially if you're using an MCLK which is a natural harmonic for the SR in
question in which case the PLL can be bypassed. Also the bias level function in
this driver will enable and disable the MCLK, if it has been provided, so it's a
bit strange to have the PLL enabled but it's clock source taken away.
> snd_soc_component_update_bits(component, DA7213_DAI_CTRL,
> DA7213_DAI_WORD_LENGTH_MASK,
> dai_ctrl);
> snd_soc_component_write(component, DA7213_SR, fs);
> @@ -1700,10 +1718,10 @@ static struct da7213_platform_data
> return pdata;
> }
>
> -
> static int da7213_probe(struct snd_soc_component *component)
> {
> struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> + int ret;
>
> pm_runtime_get_sync(component->dev);
>
> @@ -1836,6 +1854,20 @@ static int da7213_probe(struct snd_soc_component
> *component)
> return PTR_ERR(da7213->mclk);
> else
> da7213->mclk = NULL;
> + } else {
> + /* Store clock rate for fixed clocks for automatic PLL setup */
> + ret = clk_prepare_enable(da7213->mclk);
> + if (ret) {
> + dev_err(component->dev, "Failed to enable mclk\n");
> + return ret;
> + }
I've not gone through the clk framework code but surprised to see the need to
enable a clock to retrieve it's rate.
> +
> + da7213->mclk_rate = clk_get_rate(da7213->mclk);
> +
> + clk_disable_unprepare(da7213->mclk);
> +
> + /* assume fixed clock until set_sysclk() is being called */
> + da7213->fixed_clk_auto = true;
I don't see any code where 'fixed_clk_auto' is set to false so it seems that
previous operational usage is being broken here.
> }
>
> return 0;
> diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> index 97a250ea39e6..00aca0126cdb 100644
> --- a/sound/soc/codecs/da7213.h
> +++ b/sound/soc/codecs/da7213.h
> @@ -532,6 +532,7 @@ struct da7213_priv {
> bool master;
> bool alc_calib_auto;
> bool alc_en;
> + bool fixed_clk_auto;
> struct da7213_platform_data *pdata;
> };
>
> --
> 2.24.0.rc1
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [alsa-devel] [PATCHv1 5/5] ASoC: da7213: add default clock handling
@ 2019-11-11 14:20 ` Adam Thomson
0 siblings, 0 replies; 26+ messages in thread
From: Adam Thomson @ 2019-11-11 14:20 UTC (permalink / raw)
To: Sebastian Reichel, Support Opensource, Liam Girdwood, Mark Brown
Cc: linux-kernel, alsa-devel, kernel, Takashi Iwai
On 08 November 2019 17:49, 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.
>
> Upstreaming this requires a good way to disable the automatic
> clock handling for systems doing it manually to avoid breaking
> existing setups.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
> sound/soc/codecs/da7213.h | 1 +
> 2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 197609691525..a4ed382ddfc7 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1163,6 +1163,8 @@ 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);
> + int freq = 0;
> u8 dai_ctrl = 0;
> u8 fs;
>
> @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
> switch (params_rate(params)) {
> case 8000:
> fs = DA7213_SR_8000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 11025:
> fs = DA7213_SR_11025;
> + freq = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 12000:
> fs = DA7213_SR_12000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 16000:
> fs = DA7213_SR_16000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 22050:
> fs = DA7213_SR_22050;
> + freq = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 32000:
> fs = DA7213_SR_32000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 44100:
> fs = DA7213_SR_44100;
> + freq = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 48000:
> fs = DA7213_SR_48000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> case 88200:
> fs = DA7213_SR_88200;
> + freq = DA7213_PLL_FREQ_OUT_90316800;
> break;
> case 96000:
> fs = DA7213_SR_96000;
> + freq = DA7213_PLL_FREQ_OUT_98304000;
> break;
> default:
> return -EINVAL;
> }
>
> + /* setup PLL */
> + if (da7213->fixed_clk_auto) {
> + snd_soc_component_set_pll(component, 0,
> DA7213_SYSCLK_PLL,
> + da7213->mclk_rate, freq);
> + }
> +
Are we happy with the PLL being always enabled? Seems like a power drain,
especially if you're using an MCLK which is a natural harmonic for the SR in
question in which case the PLL can be bypassed. Also the bias level function in
this driver will enable and disable the MCLK, if it has been provided, so it's a
bit strange to have the PLL enabled but it's clock source taken away.
> snd_soc_component_update_bits(component, DA7213_DAI_CTRL,
> DA7213_DAI_WORD_LENGTH_MASK,
> dai_ctrl);
> snd_soc_component_write(component, DA7213_SR, fs);
> @@ -1700,10 +1718,10 @@ static struct da7213_platform_data
> return pdata;
> }
>
> -
> static int da7213_probe(struct snd_soc_component *component)
> {
> struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> + int ret;
>
> pm_runtime_get_sync(component->dev);
>
> @@ -1836,6 +1854,20 @@ static int da7213_probe(struct snd_soc_component
> *component)
> return PTR_ERR(da7213->mclk);
> else
> da7213->mclk = NULL;
> + } else {
> + /* Store clock rate for fixed clocks for automatic PLL setup */
> + ret = clk_prepare_enable(da7213->mclk);
> + if (ret) {
> + dev_err(component->dev, "Failed to enable mclk\n");
> + return ret;
> + }
I've not gone through the clk framework code but surprised to see the need to
enable a clock to retrieve it's rate.
> +
> + da7213->mclk_rate = clk_get_rate(da7213->mclk);
> +
> + clk_disable_unprepare(da7213->mclk);
> +
> + /* assume fixed clock until set_sysclk() is being called */
> + da7213->fixed_clk_auto = true;
I don't see any code where 'fixed_clk_auto' is set to false so it seems that
previous operational usage is being broken here.
> }
>
> return 0;
> diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> index 97a250ea39e6..00aca0126cdb 100644
> --- a/sound/soc/codecs/da7213.h
> +++ b/sound/soc/codecs/da7213.h
> @@ -532,6 +532,7 @@ struct da7213_priv {
> bool master;
> bool alc_calib_auto;
> bool alc_en;
> + bool fixed_clk_auto;
> struct da7213_platform_data *pdata;
> };
>
> --
> 2.24.0.rc1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCHv1 5/5] ASoC: da7213: add default clock handling
2019-11-11 14:20 ` [alsa-devel] " Adam Thomson
@ 2019-11-12 16:29 ` Sebastian Reichel
-1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-12 16:29 UTC (permalink / raw)
To: Adam Thomson
Cc: Support Opensource, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, alsa-devel, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 5593 bytes --]
Hi,
On Mon, Nov 11, 2019 at 02:20:07PM +0000, Adam Thomson wrote:
> On 08 November 2019 17:49, 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.
> >
> > Upstreaming this requires a good way to disable the automatic
> > clock handling for systems doing it manually to avoid breaking
> > existing setups.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> > sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
> > sound/soc/codecs/da7213.h | 1 +
> > 2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> > index 197609691525..a4ed382ddfc7 100644
> > --- a/sound/soc/codecs/da7213.c
> > +++ b/sound/soc/codecs/da7213.c
> > @@ -1163,6 +1163,8 @@ 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);
> > + int freq = 0;
> > u8 dai_ctrl = 0;
> > u8 fs;
> >
> > @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> > snd_pcm_substream *substream,
> > switch (params_rate(params)) {
> > case 8000:
> > fs = DA7213_SR_8000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 11025:
> > fs = DA7213_SR_11025;
> > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > break;
> > case 12000:
> > fs = DA7213_SR_12000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 16000:
> > fs = DA7213_SR_16000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 22050:
> > fs = DA7213_SR_22050;
> > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > break;
> > case 32000:
> > fs = DA7213_SR_32000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 44100:
> > fs = DA7213_SR_44100;
> > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > break;
> > case 48000:
> > fs = DA7213_SR_48000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 88200:
> > fs = DA7213_SR_88200;
> > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > break;
> > case 96000:
> > fs = DA7213_SR_96000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > default:
> > return -EINVAL;
> > }
> >
> > + /* setup PLL */
> > + if (da7213->fixed_clk_auto) {
> > + snd_soc_component_set_pll(component, 0,
> > DA7213_SYSCLK_PLL,
> > + da7213->mclk_rate, freq);
> > + }
> > +
>
> Are we happy with the PLL being always enabled? Seems like a power drain,
> especially if you're using an MCLK which is a natural harmonic for the SR in
> question in which case the PLL can be bypassed. Also the bias level function in
> this driver will enable and disable the MCLK, if it has been provided, so it's a
> bit strange to have the PLL enabled but it's clock source taken away.
So you are suggesting adding something like this to
da7213_set_bias_level()?
if (freq % da7213->mclk_rate == 0)
source = DA7213_SYSCLK_MCLK
else
source = DA7213_SYSCLK_PLL
snd_soc_component_set_pll(component, 0, source, da7213->mclk_rate, freq);
> > snd_soc_component_update_bits(component, DA7213_DAI_CTRL,
> > DA7213_DAI_WORD_LENGTH_MASK,
> > dai_ctrl);
> > snd_soc_component_write(component, DA7213_SR, fs);
> > @@ -1700,10 +1718,10 @@ static struct da7213_platform_data
> > return pdata;
> > }
> >
> > -
> > static int da7213_probe(struct snd_soc_component *component)
> > {
> > struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> > + int ret;
> >
> > pm_runtime_get_sync(component->dev);
> >
> > @@ -1836,6 +1854,20 @@ static int da7213_probe(struct snd_soc_component
> > *component)
> > return PTR_ERR(da7213->mclk);
> > else
> > da7213->mclk = NULL;
> > + } else {
> > + /* Store clock rate for fixed clocks for automatic PLL setup */
> > + ret = clk_prepare_enable(da7213->mclk);
> > + if (ret) {
> > + dev_err(component->dev, "Failed to enable mclk\n");
> > + return ret;
> > + }
>
> I've not gone through the clk framework code but surprised to see the need to
> enable a clock to retrieve it's rate.
/**
* clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
* This is only valid once the clock source has been enabled.
* @clk: clock source
*/
unsigned long clk_get_rate(struct clk *clk);
Which makes sense for a non-fixed clock, which uses the same API.
> > + da7213->mclk_rate = clk_get_rate(da7213->mclk);
> > +
> > + clk_disable_unprepare(da7213->mclk);
> > +
> > + /* assume fixed clock until set_sysclk() is being called */
> > + da7213->fixed_clk_auto = true;
>
> I don't see any code where 'fixed_clk_auto' is set to false so it seems that
> previous operational usage is being broken here.
oops.
> > }
> >
> > return 0;
> > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> > index 97a250ea39e6..00aca0126cdb 100644
> > --- a/sound/soc/codecs/da7213.h
> > +++ b/sound/soc/codecs/da7213.h
> > @@ -532,6 +532,7 @@ struct da7213_priv {
> > bool master;
> > bool alc_calib_auto;
> > bool alc_en;
> > + bool fixed_clk_auto;
> > struct da7213_platform_data *pdata;
> > };
> >
> > --
> > 2.24.0.rc1
>
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [alsa-devel] [PATCHv1 5/5] ASoC: da7213: add default clock handling
@ 2019-11-12 16:29 ` Sebastian Reichel
0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Reichel @ 2019-11-12 16:29 UTC (permalink / raw)
To: Adam Thomson
Cc: alsa-devel, Support Opensource, Liam Girdwood, linux-kernel,
Takashi Iwai, Mark Brown, kernel
[-- Attachment #1.1: Type: text/plain, Size: 5593 bytes --]
Hi,
On Mon, Nov 11, 2019 at 02:20:07PM +0000, Adam Thomson wrote:
> On 08 November 2019 17:49, 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.
> >
> > Upstreaming this requires a good way to disable the automatic
> > clock handling for systems doing it manually to avoid breaking
> > existing setups.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> > sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
> > sound/soc/codecs/da7213.h | 1 +
> > 2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> > index 197609691525..a4ed382ddfc7 100644
> > --- a/sound/soc/codecs/da7213.c
> > +++ b/sound/soc/codecs/da7213.c
> > @@ -1163,6 +1163,8 @@ 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);
> > + int freq = 0;
> > u8 dai_ctrl = 0;
> > u8 fs;
> >
> > @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> > snd_pcm_substream *substream,
> > switch (params_rate(params)) {
> > case 8000:
> > fs = DA7213_SR_8000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 11025:
> > fs = DA7213_SR_11025;
> > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > break;
> > case 12000:
> > fs = DA7213_SR_12000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 16000:
> > fs = DA7213_SR_16000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 22050:
> > fs = DA7213_SR_22050;
> > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > break;
> > case 32000:
> > fs = DA7213_SR_32000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 44100:
> > fs = DA7213_SR_44100;
> > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > break;
> > case 48000:
> > fs = DA7213_SR_48000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > case 88200:
> > fs = DA7213_SR_88200;
> > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > break;
> > case 96000:
> > fs = DA7213_SR_96000;
> > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > break;
> > default:
> > return -EINVAL;
> > }
> >
> > + /* setup PLL */
> > + if (da7213->fixed_clk_auto) {
> > + snd_soc_component_set_pll(component, 0,
> > DA7213_SYSCLK_PLL,
> > + da7213->mclk_rate, freq);
> > + }
> > +
>
> Are we happy with the PLL being always enabled? Seems like a power drain,
> especially if you're using an MCLK which is a natural harmonic for the SR in
> question in which case the PLL can be bypassed. Also the bias level function in
> this driver will enable and disable the MCLK, if it has been provided, so it's a
> bit strange to have the PLL enabled but it's clock source taken away.
So you are suggesting adding something like this to
da7213_set_bias_level()?
if (freq % da7213->mclk_rate == 0)
source = DA7213_SYSCLK_MCLK
else
source = DA7213_SYSCLK_PLL
snd_soc_component_set_pll(component, 0, source, da7213->mclk_rate, freq);
> > snd_soc_component_update_bits(component, DA7213_DAI_CTRL,
> > DA7213_DAI_WORD_LENGTH_MASK,
> > dai_ctrl);
> > snd_soc_component_write(component, DA7213_SR, fs);
> > @@ -1700,10 +1718,10 @@ static struct da7213_platform_data
> > return pdata;
> > }
> >
> > -
> > static int da7213_probe(struct snd_soc_component *component)
> > {
> > struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> > + int ret;
> >
> > pm_runtime_get_sync(component->dev);
> >
> > @@ -1836,6 +1854,20 @@ static int da7213_probe(struct snd_soc_component
> > *component)
> > return PTR_ERR(da7213->mclk);
> > else
> > da7213->mclk = NULL;
> > + } else {
> > + /* Store clock rate for fixed clocks for automatic PLL setup */
> > + ret = clk_prepare_enable(da7213->mclk);
> > + if (ret) {
> > + dev_err(component->dev, "Failed to enable mclk\n");
> > + return ret;
> > + }
>
> I've not gone through the clk framework code but surprised to see the need to
> enable a clock to retrieve it's rate.
/**
* clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
* This is only valid once the clock source has been enabled.
* @clk: clock source
*/
unsigned long clk_get_rate(struct clk *clk);
Which makes sense for a non-fixed clock, which uses the same API.
> > + da7213->mclk_rate = clk_get_rate(da7213->mclk);
> > +
> > + clk_disable_unprepare(da7213->mclk);
> > +
> > + /* assume fixed clock until set_sysclk() is being called */
> > + da7213->fixed_clk_auto = true;
>
> I don't see any code where 'fixed_clk_auto' is set to false so it seems that
> previous operational usage is being broken here.
oops.
> > }
> >
> > return 0;
> > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> > index 97a250ea39e6..00aca0126cdb 100644
> > --- a/sound/soc/codecs/da7213.h
> > +++ b/sound/soc/codecs/da7213.h
> > @@ -532,6 +532,7 @@ struct da7213_priv {
> > bool master;
> > bool alc_calib_auto;
> > bool alc_en;
> > + bool fixed_clk_auto;
> > struct da7213_platform_data *pdata;
> > };
> >
> > --
> > 2.24.0.rc1
>
-- Sebastian
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* RE: [PATCHv1 5/5] ASoC: da7213: add default clock handling
2019-11-12 16:29 ` [alsa-devel] " Sebastian Reichel
@ 2019-11-13 11:25 ` Adam Thomson
-1 siblings, 0 replies; 26+ messages in thread
From: Adam Thomson @ 2019-11-13 11:25 UTC (permalink / raw)
To: Sebastian Reichel, Adam Thomson
Cc: Support Opensource, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, alsa-devel, linux-kernel, kernel
On 12 November 2019 16:30, Sebastian Reichel wrote:
> > > @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> > > snd_pcm_substream *substream,
> > > switch (params_rate(params)) {
> > > case 8000:
> > > fs = DA7213_SR_8000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 11025:
> > > fs = DA7213_SR_11025;
> > > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > > break;
> > > case 12000:
> > > fs = DA7213_SR_12000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 16000:
> > > fs = DA7213_SR_16000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 22050:
> > > fs = DA7213_SR_22050;
> > > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > > break;
> > > case 32000:
> > > fs = DA7213_SR_32000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 44100:
> > > fs = DA7213_SR_44100;
> > > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > > break;
> > > case 48000:
> > > fs = DA7213_SR_48000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 88200:
> > > fs = DA7213_SR_88200;
> > > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > > break;
> > > case 96000:
> > > fs = DA7213_SR_96000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > default:
> > > return -EINVAL;
> > > }
> > >
> > > + /* setup PLL */
> > > + if (da7213->fixed_clk_auto) {
> > > + snd_soc_component_set_pll(component, 0,
> > > DA7213_SYSCLK_PLL,
> > > + da7213->mclk_rate, freq);
> > > + }
> > > +
> >
> > Are we happy with the PLL being always enabled? Seems like a power drain,
> > especially if you're using an MCLK which is a natural harmonic for the SR in
> > question in which case the PLL can be bypassed. Also the bias level function in
> > this driver will enable and disable the MCLK, if it has been provided, so it's a
> > bit strange to have the PLL enabled but it's clock source taken away.
>
> So you are suggesting adding something like this to
> da7213_set_bias_level()?
>
> if (freq % da7213->mclk_rate == 0)
> source = DA7213_SYSCLK_MCLK
> else
> source = DA7213_SYSCLK_PLL
> snd_soc_component_set_pll(component, 0, source, da7213->mclk_rate, freq);
Yes it would make more sense to control the PLL there as for MCLK. Also for the
transition back to SND_SOC_BIAS_STANDBY you would want to configure the PLL as
DA7213_SYSCLK_MCLK so it's in bypass mode (i.e. disabled).
Selecting bypass mode for natural harmonic MCLK frequencies (11.2896/12.288,
22.5792/24.576 and 45.1584/49.152 as stated in the datasheet) would be ideal. I
think the check you suggest above though might not be enough as it will pick out
valid MCLK rates of 5.6448/6.144 as allowing PLL bypass but the datasheet
doesn't state those as valid options for bypass.
> > > @@ -1836,6 +1854,20 @@ static int da7213_probe(struct
> snd_soc_component
> > > *component)
> > > return PTR_ERR(da7213->mclk);
> > > else
> > > da7213->mclk = NULL;
> > > + } else {
> > > + /* Store clock rate for fixed clocks for automatic PLL setup */
> > > + ret = clk_prepare_enable(da7213->mclk);
> > > + if (ret) {
> > > + dev_err(component->dev, "Failed to enable mclk\n");
> > > + return ret;
> > > + }
> >
> > I've not gone through the clk framework code but surprised to see the need to
> > enable a clock to retrieve it's rate.
>
> /**
> * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
> * This is only valid once the clock source has been enabled.
> * @clk: clock source
> */
> unsigned long clk_get_rate(struct clk *clk);
>
> Which makes sense for a non-fixed clock, which uses the same API.
Hmmm. Fair enough. Just seems odd to me that you would need to enable a clock
to retrieve it's configured rate. Surely it would have been configured prior to
enabling. Reading into that code, the clk rate value is always taken from cache
so until it's enabled then the cache isn't updated. Actually with the suggestion
to move PLL control to the bias_level() function, you could just get the clk
rate there instead.
>
> > > + da7213->mclk_rate = clk_get_rate(da7213->mclk);
> > > +
> > > + clk_disable_unprepare(da7213->mclk);
> > > +
> > > + /* assume fixed clock until set_sysclk() is being called */
> > > + da7213->fixed_clk_auto = true;
> >
> > I don't see any code where 'fixed_clk_auto' is set to false so it seems that
> > previous operational usage is being broken here.
>
> oops.
:)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [alsa-devel] [PATCHv1 5/5] ASoC: da7213: add default clock handling
@ 2019-11-13 11:25 ` Adam Thomson
0 siblings, 0 replies; 26+ messages in thread
From: Adam Thomson @ 2019-11-13 11:25 UTC (permalink / raw)
To: Sebastian Reichel, Adam Thomson
Cc: alsa-devel, Support Opensource, Liam Girdwood, linux-kernel,
Takashi Iwai, Mark Brown, kernel
On 12 November 2019 16:30, Sebastian Reichel wrote:
> > > @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> > > snd_pcm_substream *substream,
> > > switch (params_rate(params)) {
> > > case 8000:
> > > fs = DA7213_SR_8000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 11025:
> > > fs = DA7213_SR_11025;
> > > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > > break;
> > > case 12000:
> > > fs = DA7213_SR_12000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 16000:
> > > fs = DA7213_SR_16000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 22050:
> > > fs = DA7213_SR_22050;
> > > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > > break;
> > > case 32000:
> > > fs = DA7213_SR_32000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 44100:
> > > fs = DA7213_SR_44100;
> > > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > > break;
> > > case 48000:
> > > fs = DA7213_SR_48000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > case 88200:
> > > fs = DA7213_SR_88200;
> > > + freq = DA7213_PLL_FREQ_OUT_90316800;
> > > break;
> > > case 96000:
> > > fs = DA7213_SR_96000;
> > > + freq = DA7213_PLL_FREQ_OUT_98304000;
> > > break;
> > > default:
> > > return -EINVAL;
> > > }
> > >
> > > + /* setup PLL */
> > > + if (da7213->fixed_clk_auto) {
> > > + snd_soc_component_set_pll(component, 0,
> > > DA7213_SYSCLK_PLL,
> > > + da7213->mclk_rate, freq);
> > > + }
> > > +
> >
> > Are we happy with the PLL being always enabled? Seems like a power drain,
> > especially if you're using an MCLK which is a natural harmonic for the SR in
> > question in which case the PLL can be bypassed. Also the bias level function in
> > this driver will enable and disable the MCLK, if it has been provided, so it's a
> > bit strange to have the PLL enabled but it's clock source taken away.
>
> So you are suggesting adding something like this to
> da7213_set_bias_level()?
>
> if (freq % da7213->mclk_rate == 0)
> source = DA7213_SYSCLK_MCLK
> else
> source = DA7213_SYSCLK_PLL
> snd_soc_component_set_pll(component, 0, source, da7213->mclk_rate, freq);
Yes it would make more sense to control the PLL there as for MCLK. Also for the
transition back to SND_SOC_BIAS_STANDBY you would want to configure the PLL as
DA7213_SYSCLK_MCLK so it's in bypass mode (i.e. disabled).
Selecting bypass mode for natural harmonic MCLK frequencies (11.2896/12.288,
22.5792/24.576 and 45.1584/49.152 as stated in the datasheet) would be ideal. I
think the check you suggest above though might not be enough as it will pick out
valid MCLK rates of 5.6448/6.144 as allowing PLL bypass but the datasheet
doesn't state those as valid options for bypass.
> > > @@ -1836,6 +1854,20 @@ static int da7213_probe(struct
> snd_soc_component
> > > *component)
> > > return PTR_ERR(da7213->mclk);
> > > else
> > > da7213->mclk = NULL;
> > > + } else {
> > > + /* Store clock rate for fixed clocks for automatic PLL setup */
> > > + ret = clk_prepare_enable(da7213->mclk);
> > > + if (ret) {
> > > + dev_err(component->dev, "Failed to enable mclk\n");
> > > + return ret;
> > > + }
> >
> > I've not gone through the clk framework code but surprised to see the need to
> > enable a clock to retrieve it's rate.
>
> /**
> * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
> * This is only valid once the clock source has been enabled.
> * @clk: clock source
> */
> unsigned long clk_get_rate(struct clk *clk);
>
> Which makes sense for a non-fixed clock, which uses the same API.
Hmmm. Fair enough. Just seems odd to me that you would need to enable a clock
to retrieve it's configured rate. Surely it would have been configured prior to
enabling. Reading into that code, the clk rate value is always taken from cache
so until it's enabled then the cache isn't updated. Actually with the suggestion
to move PLL control to the bias_level() function, you could just get the clk
rate there instead.
>
> > > + da7213->mclk_rate = clk_get_rate(da7213->mclk);
> > > +
> > > + clk_disable_unprepare(da7213->mclk);
> > > +
> > > + /* assume fixed clock until set_sysclk() is being called */
> > > + da7213->fixed_clk_auto = true;
> >
> > I don't see any code where 'fixed_clk_auto' is set to false so it seems that
> > previous operational usage is being broken here.
>
> oops.
:)
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 26+ messages in thread