[alsa-devel] [PATCHv2 0/6] ASoC: da7213: support for usage with simple-card
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"; }; }; ---------------------------------------------------------------------
Changes since PATCHv1: * https://lore.kernel.org/alsa-devel/20191108174843.11227-1-sebastian.reichel@... * add patch adding da7212 compatible to DT bindings * update regulator patch, so that VDDA is enabled together with VDDIO while the device is enabled to avoid device reset * update clock patch, so that automatic PLL handling is not enabled when PLL is configured manually * update clock patch, so that automatic PLL is disabled when the device is suspended * update clock patch, so that automatic PLL is configured into bypass mode when possible
-- Sebastian
Sebastian Reichel (6): ASoC: da7213: Add da7212 DT compatible ASoC: da7213: Add regulator support ASoC: da7213: Provide selectable option ASoC: da7213: Move set_sysclk to codec level ASoC: da7213: Move set_pll to codec level ASoC: da7213: Add default clock handling
.../devicetree/bindings/sound/da7213.txt | 8 +- sound/soc/codecs/Kconfig | 3 +- sound/soc/codecs/da7213.c | 171 ++++++++++++++++-- sound/soc/codecs/da7213.h | 11 ++ 4 files changed, 177 insertions(+), 16 deletions(-)
This adds a compatible for da7212. It's handled exactly the same way as DA7213 and follows the ACPI bindings.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com --- Documentation/devicetree/bindings/sound/da7213.txt | 4 ++-- sound/soc/codecs/da7213.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt index 58902802d56c..759bb04e0283 100644 --- a/Documentation/devicetree/bindings/sound/da7213.txt +++ b/Documentation/devicetree/bindings/sound/da7213.txt @@ -1,9 +1,9 @@ -Dialog Semiconductor DA7213 Audio Codec bindings +Dialog Semiconductor DA7212/DA7213 Audio Codec bindings
======
Required properties: -- compatible : Should be "dlg,da7213" +- compatible : Should be "dlg,da7212" or "dlg,7213" - reg: Specifies the I2C slave address
Optional properties: diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index 925a03996db4..aff306bb58df 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -1571,6 +1571,7 @@ static int da7213_set_bias_level(struct snd_soc_component *component, #if defined(CONFIG_OF) /* DT */ static const struct of_device_id da7213_of_match[] = { + { .compatible = "dlg,da7212", }, { .compatible = "dlg,da7213", }, { } };
On 20 November 2019 15:24, Sebastian Reichel wrote:
This adds a compatible for da7212. It's handled exactly the same way as DA7213 and follows the ACPI bindings.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
Documentation/devicetree/bindings/sound/da7213.txt | 4 ++-- sound/soc/codecs/da7213.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt index 58902802d56c..759bb04e0283 100644 --- a/Documentation/devicetree/bindings/sound/da7213.txt +++ b/Documentation/devicetree/bindings/sound/da7213.txt @@ -1,9 +1,9 @@ -Dialog Semiconductor DA7213 Audio Codec bindings +Dialog Semiconductor DA7212/DA7213 Audio Codec bindings
======
Required properties: -- compatible : Should be "dlg,da7213" +- compatible : Should be "dlg,da7212" or "dlg,7213"
Typo? "dlg,da7213"
- reg: Specifies the I2C slave address
Optional properties: diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index 925a03996db4..aff306bb58df 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -1571,6 +1571,7 @@ static int da7213_set_bias_level(struct snd_soc_component *component, #if defined(CONFIG_OF) /* DT */ static const struct of_device_id da7213_of_match[] = {
- { .compatible = "dlg,da7212", }, { .compatible = "dlg,da7213", }, { }
};
2.24.0
Hi,
On Thu, Nov 21, 2019 at 08:10:30PM +0000, Adam Thomson wrote:
[...]
Required properties: -- compatible : Should be "dlg,da7213" +- compatible : Should be "dlg,da7212" or "dlg,7213"
Typo? "dlg,da7213"
Ack.
[...]
-- Sebastian
This adds support for most regulators of da7212 for improved power management. The only thing skipped was the speaker supply, which has some undocumented dependencies. It's supposed to be either always-enabled or always-disabled.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com --- .../devicetree/bindings/sound/da7213.txt | 4 + sound/soc/codecs/da7213.c | 79 ++++++++++++++++++- sound/soc/codecs/da7213.h | 9 +++ 3 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt index 759bb04e0283..cc8200b7d748 100644 --- a/Documentation/devicetree/bindings/sound/da7213.txt +++ b/Documentation/devicetree/bindings/sound/da7213.txt @@ -21,6 +21,10 @@ Optional properties: - dlg,dmic-clkrate : DMIC clock frequency (Hz). [<1500000>, <3000000>]
+ - VDDA-supply : Regulator phandle for Analogue power supply + - VDDMIC-supply : Regulator phandle for Mic Bias + - VDDIO-supply : Regulator phandle for I/O power supply + ======
Example: diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index aff306bb58df..0359249118d0 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -19,6 +19,7 @@ #include <linux/module.h> #include <sound/pcm.h> #include <sound/pcm_params.h> +#include <linux/pm_runtime.h> #include <sound/soc.h> #include <sound/initval.h> #include <sound/tlv.h> @@ -806,6 +807,11 @@ static int da7213_dai_event(struct snd_soc_dapm_widget *w, */
static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = { + /* + * Power Supply + */ + SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0), + /* * Input & Output */ @@ -932,6 +938,9 @@ static const struct snd_soc_dapm_route da7213_audio_map[] = { /* Dest Connecting Widget source */
/* Input path */ + {"Mic Bias 1", NULL, "VDDMIC"}, + {"Mic Bias 2", NULL, "VDDMIC"}, + {"MIC1", NULL, "Mic Bias 1"}, {"MIC2", NULL, "Mic Bias 2"},
@@ -1691,6 +1700,8 @@ static int da7213_probe(struct snd_soc_component *component) { struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+ pm_runtime_get_sync(component->dev); + /* Default to using ALC auto offset calibration mode. */ snd_soc_component_update_bits(component, DA7213_ALC_CTRL1, DA7213_ALC_CALIB_MODE_MAN, 0); @@ -1811,6 +1822,8 @@ static int da7213_probe(struct snd_soc_component *component) DA7213_DMIC_CLK_RATE_MASK, dmic_cfg); }
+ pm_runtime_put_sync(component->dev); + /* Check if MCLK provided */ da7213->mclk = devm_clk_get(component->dev, "mclk"); if (IS_ERR(da7213->mclk)) { @@ -1848,11 +1861,22 @@ static const struct regmap_config da7213_regmap_config = { .cache_type = REGCACHE_RBTREE, };
+static void da7213_power_off(void *data) +{ + struct da7213_priv *da7213 = data; + regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies); +} + +static const char *da7213_supply_names[DA7213_NUM_SUPPLIES] = { + [DA7213_SUPPLY_VDDA] = "VDDA", + [DA7213_SUPPLY_VDDIO] = "VDDIO", +}; + static int da7213_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct da7213_priv *da7213; - int ret; + int i, ret;
da7213 = devm_kzalloc(&i2c->dev, sizeof(*da7213), GFP_KERNEL); if (!da7213) @@ -1860,6 +1884,25 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, da7213);
+ /* Get required supplies */ + for (i = 0; i < DA7213_NUM_SUPPLIES; ++i) + da7213->supplies[i].supply = da7213_supply_names[i]; + + ret = devm_regulator_bulk_get(&i2c->dev, DA7213_NUM_SUPPLIES, + da7213->supplies); + if (ret) { + dev_err(&i2c->dev, "Failed to get supplies: %d\n", ret); + return ret; + } + + ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies); + if (ret < 0) + return ret; + + ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off, da7213); + if (ret < 0) + return ret; + da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config); if (IS_ERR(da7213->regmap)) { ret = PTR_ERR(da7213->regmap); @@ -1867,6 +1910,11 @@ static int da7213_i2c_probe(struct i2c_client *i2c, return ret; }
+ pm_runtime_set_autosuspend_delay(&i2c->dev, 100); + pm_runtime_use_autosuspend(&i2c->dev); + pm_runtime_set_active(&i2c->dev); + pm_runtime_enable(&i2c->dev); + ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_da7213, &da7213_dai, 1); if (ret < 0) { @@ -1876,6 +1924,34 @@ static int da7213_i2c_probe(struct i2c_client *i2c, return ret; }
+static int __maybe_unused da7213_runtime_suspend(struct device *dev) +{ + struct da7213_priv *da7213 = dev_get_drvdata(dev); + + regcache_cache_only(da7213->regmap, true); + regcache_mark_dirty(da7213->regmap); + regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies); + + return 0; +} + +static int __maybe_unused da7213_runtime_resume(struct device *dev) +{ + struct da7213_priv *da7213 = dev_get_drvdata(dev); + int ret; + + ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies); + if (ret < 0) + return ret; + regcache_cache_only(da7213->regmap, false); + regcache_sync(da7213->regmap); + return 0; +} + +static const struct dev_pm_ops da7213_pm = { + SET_RUNTIME_PM_OPS(da7213_runtime_suspend, da7213_runtime_resume, NULL) +}; + static const struct i2c_device_id da7213_i2c_id[] = { { "da7213", 0 }, { } @@ -1888,6 +1964,7 @@ static struct i2c_driver da7213_i2c_driver = { .name = "da7213", .of_match_table = of_match_ptr(da7213_of_match), .acpi_match_table = ACPI_PTR(da7213_acpi_match), + .pm = &da7213_pm, }, .probe = da7213_i2c_probe, .id_table = da7213_i2c_id, diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h index 3250a3821fcc..3890829dfb6e 100644 --- a/sound/soc/codecs/da7213.h +++ b/sound/soc/codecs/da7213.h @@ -12,6 +12,7 @@
#include <linux/clk.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <sound/da7213.h>
/* @@ -521,9 +522,17 @@ enum da7213_sys_clk { DA7213_SYSCLK_PLL_32KHZ };
+/* Regulators */ +enum da7213_supplies { + DA7213_SUPPLY_VDDA = 0, + DA7213_SUPPLY_VDDIO, + DA7213_NUM_SUPPLIES, +}; + /* Codec private data */ struct da7213_priv { struct regmap *regmap; + struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES]; struct clk *mclk; unsigned int mclk_rate; int clk_src;
On 20 November 2019 15:24, Sebastian Reichel wrote:
This adds support for most regulators of da7212 for improved power management. The only thing skipped was the speaker supply, which has some undocumented dependencies. It's supposed to be either always-enabled or always-disabled.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
.../devicetree/bindings/sound/da7213.txt | 4 + sound/soc/codecs/da7213.c | 79 ++++++++++++++++++- sound/soc/codecs/da7213.h | 9 +++ 3 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt index 759bb04e0283..cc8200b7d748 100644 --- a/Documentation/devicetree/bindings/sound/da7213.txt +++ b/Documentation/devicetree/bindings/sound/da7213.txt @@ -21,6 +21,10 @@ Optional properties:
- dlg,dmic-clkrate : DMIC clock frequency (Hz). [<1500000>, <3000000>]
- VDDA-supply : Regulator phandle for Analogue power supply
- VDDMIC-supply : Regulator phandle for Mic Bias
- VDDIO-supply : Regulator phandle for I/O power supply
======
Example: diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index aff306bb58df..0359249118d0 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -19,6 +19,7 @@ #include <linux/module.h> #include <sound/pcm.h> #include <sound/pcm_params.h> +#include <linux/pm_runtime.h> #include <sound/soc.h> #include <sound/initval.h> #include <sound/tlv.h> @@ -806,6 +807,11 @@ static int da7213_dai_event(struct snd_soc_dapm_widget *w, */
static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
- /*
* Power Supply*/- SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
- /*
*/
- Input & Output
@@ -932,6 +938,9 @@ static const struct snd_soc_dapm_route da7213_audio_map[] = { /* Dest Connecting Widget source */
/* Input path */
- {"Mic Bias 1", NULL, "VDDMIC"},
- {"Mic Bias 2", NULL, "VDDMIC"},
- {"MIC1", NULL, "Mic Bias 1"}, {"MIC2", NULL, "Mic Bias 2"},
@@ -1691,6 +1700,8 @@ static int da7213_probe(struct snd_soc_component *component) { struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
- pm_runtime_get_sync(component->dev);
It seems that this function can return errors, although I do see lots of instances of this being called where the return value isn't checked. Not had time to walk the code fully but are we sure no errors are going to happen here?
- /* Default to using ALC auto offset calibration mode. */ snd_soc_component_update_bits(component, DA7213_ALC_CTRL1, DA7213_ALC_CALIB_MODE_MAN, 0);
@@ -1811,6 +1822,8 @@ static int da7213_probe(struct snd_soc_component *component) DA7213_DMIC_CLK_RATE_MASK, dmic_cfg); }
- pm_runtime_put_sync(component->dev);
Same question here.
- /* Check if MCLK provided */ da7213->mclk = devm_clk_get(component->dev, "mclk"); if (IS_ERR(da7213->mclk)) {
@@ -1848,11 +1861,22 @@ static const struct regmap_config da7213_regmap_config = { .cache_type = REGCACHE_RBTREE, };
+static void da7213_power_off(void *data) +{
- struct da7213_priv *da7213 = data;
- regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
+}
+static const char *da7213_supply_names[DA7213_NUM_SUPPLIES] = {
- [DA7213_SUPPLY_VDDA] = "VDDA",
- [DA7213_SUPPLY_VDDIO] = "VDDIO",
+};
static int da7213_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct da7213_priv *da7213;
- int ret;
int i, ret;
da7213 = devm_kzalloc(&i2c->dev, sizeof(*da7213), GFP_KERNEL); if (!da7213)
@@ -1860,6 +1884,25 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, da7213);
- /* Get required supplies */
- for (i = 0; i < DA7213_NUM_SUPPLIES; ++i)
da7213->supplies[i].supply = da7213_supply_names[i];- ret = devm_regulator_bulk_get(&i2c->dev, DA7213_NUM_SUPPLIES,
da7213->supplies);- if (ret) {
dev_err(&i2c->dev, "Failed to get supplies: %d\n", ret);return ret;- }
- ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
supplies);
- if (ret < 0)
return ret;- ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
da7213);
- if (ret < 0)
return ret;- da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config); if (IS_ERR(da7213->regmap)) { ret = PTR_ERR(da7213->regmap);
@@ -1867,6 +1910,11 @@ static int da7213_i2c_probe(struct i2c_client *i2c, return ret; }
- pm_runtime_set_autosuspend_delay(&i2c->dev, 100);
- pm_runtime_use_autosuspend(&i2c->dev);
- pm_runtime_set_active(&i2c->dev);
Again this can return an error. Are we certain this won't fail?
- pm_runtime_enable(&i2c->dev);
- ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_da7213, &da7213_dai, 1); if (ret < 0) {
@@ -1876,6 +1924,34 @@ static int da7213_i2c_probe(struct i2c_client *i2c, return ret; }
+static int __maybe_unused da7213_runtime_suspend(struct device *dev) +{
- struct da7213_priv *da7213 = dev_get_drvdata(dev);
- regcache_cache_only(da7213->regmap, true);
- regcache_mark_dirty(da7213->regmap);
- regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
- return 0;
+}
+static int __maybe_unused da7213_runtime_resume(struct device *dev) +{
- struct da7213_priv *da7213 = dev_get_drvdata(dev);
- int ret;
- ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
supplies);
- if (ret < 0)
return ret;- regcache_cache_only(da7213->regmap, false);
- regcache_sync(da7213->regmap);
- return 0;
+}
+static const struct dev_pm_ops da7213_pm = {
- SET_RUNTIME_PM_OPS(da7213_runtime_suspend,
da7213_runtime_resume, NULL) +};
static const struct i2c_device_id da7213_i2c_id[] = { { "da7213", 0 }, { } @@ -1888,6 +1964,7 @@ static struct i2c_driver da7213_i2c_driver = { .name = "da7213", .of_match_table = of_match_ptr(da7213_of_match), .acpi_match_table = ACPI_PTR(da7213_acpi_match),
.pm = &da7213_pm,diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h index 3250a3821fcc..3890829dfb6e 100644 --- a/sound/soc/codecs/da7213.h +++ b/sound/soc/codecs/da7213.h @@ -12,6 +12,7 @@
#include <linux/clk.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <sound/da7213.h>
/* @@ -521,9 +522,17 @@ enum da7213_sys_clk { DA7213_SYSCLK_PLL_32KHZ };
+/* Regulators */ +enum da7213_supplies {
- DA7213_SUPPLY_VDDA = 0,
- DA7213_SUPPLY_VDDIO,
- DA7213_NUM_SUPPLIES,
+};
/* Codec private data */ struct da7213_priv { struct regmap *regmap;
- struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES]; struct clk *mclk; unsigned int mclk_rate; int clk_src;
-- 2.24.0
Hi,
On Thu, Nov 21, 2019 at 09:15:02PM +0000, Adam Thomson wrote:
On 20 November 2019 15:24, Sebastian Reichel wrote:
This adds support for most regulators of da7212 for improved power management. The only thing skipped was the speaker supply, which has some undocumented dependencies. It's supposed to be either always-enabled or always-disabled.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
.../devicetree/bindings/sound/da7213.txt | 4 + sound/soc/codecs/da7213.c | 79 ++++++++++++++++++- sound/soc/codecs/da7213.h | 9 +++ 3 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt index 759bb04e0283..cc8200b7d748 100644 --- a/Documentation/devicetree/bindings/sound/da7213.txt +++ b/Documentation/devicetree/bindings/sound/da7213.txt @@ -21,6 +21,10 @@ Optional properties:
- dlg,dmic-clkrate : DMIC clock frequency (Hz). [<1500000>, <3000000>]
- VDDA-supply : Regulator phandle for Analogue power supply
- VDDMIC-supply : Regulator phandle for Mic Bias
- VDDIO-supply : Regulator phandle for I/O power supply
======
Example: diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index aff306bb58df..0359249118d0 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -19,6 +19,7 @@ #include <linux/module.h> #include <sound/pcm.h> #include <sound/pcm_params.h> +#include <linux/pm_runtime.h> #include <sound/soc.h> #include <sound/initval.h> #include <sound/tlv.h> @@ -806,6 +807,11 @@ static int da7213_dai_event(struct snd_soc_dapm_widget *w, */
static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
- /*
* Power Supply*/- SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
- /*
*/
- Input & Output
@@ -932,6 +938,9 @@ static const struct snd_soc_dapm_route da7213_audio_map[] = { /* Dest Connecting Widget source */
/* Input path */
- {"Mic Bias 1", NULL, "VDDMIC"},
- {"Mic Bias 2", NULL, "VDDMIC"},
- {"MIC1", NULL, "Mic Bias 1"}, {"MIC2", NULL, "Mic Bias 2"},
@@ -1691,6 +1700,8 @@ static int da7213_probe(struct snd_soc_component *component) { struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
- pm_runtime_get_sync(component->dev);
It seems that this function can return errors, although I do see lots of instances of this being called where the return value isn't checked. Not had time to walk the code fully but are we sure no errors are going to happen here?
In this case, the runtime PM is already enabled because of pm_runtime_set_active() being called previously. So this only increases the usage counter.
- /* Default to using ALC auto offset calibration mode. */ snd_soc_component_update_bits(component, DA7213_ALC_CTRL1, DA7213_ALC_CALIB_MODE_MAN, 0);
@@ -1811,6 +1822,8 @@ static int da7213_probe(struct snd_soc_component *component) DA7213_DMIC_CLK_RATE_MASK, dmic_cfg); }
- pm_runtime_put_sync(component->dev);
Same question here.
da7213_runtime_suspend() always returns 0.
- /* Check if MCLK provided */ da7213->mclk = devm_clk_get(component->dev, "mclk"); if (IS_ERR(da7213->mclk)) {
@@ -1848,11 +1861,22 @@ static const struct regmap_config da7213_regmap_config = { .cache_type = REGCACHE_RBTREE, };
+static void da7213_power_off(void *data) +{
- struct da7213_priv *da7213 = data;
- regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
+}
+static const char *da7213_supply_names[DA7213_NUM_SUPPLIES] = {
- [DA7213_SUPPLY_VDDA] = "VDDA",
- [DA7213_SUPPLY_VDDIO] = "VDDIO",
+};
static int da7213_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct da7213_priv *da7213;
- int ret;
int i, ret;
da7213 = devm_kzalloc(&i2c->dev, sizeof(*da7213), GFP_KERNEL); if (!da7213)
@@ -1860,6 +1884,25 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, da7213);
- /* Get required supplies */
- for (i = 0; i < DA7213_NUM_SUPPLIES; ++i)
da7213->supplies[i].supply = da7213_supply_names[i];- ret = devm_regulator_bulk_get(&i2c->dev, DA7213_NUM_SUPPLIES,
da7213->supplies);- if (ret) {
dev_err(&i2c->dev, "Failed to get supplies: %d\n", ret);return ret;- }
- ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
supplies);
- if (ret < 0)
return ret;- ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
da7213);
- if (ret < 0)
return ret;- da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config); if (IS_ERR(da7213->regmap)) { ret = PTR_ERR(da7213->regmap);
@@ -1867,6 +1910,11 @@ static int da7213_i2c_probe(struct i2c_client *i2c, return ret; }
- pm_runtime_set_autosuspend_delay(&i2c->dev, 100);
- pm_runtime_use_autosuspend(&i2c->dev);
- pm_runtime_set_active(&i2c->dev);
Again this can return an error. Are we certain this won't fail?
This only provides the information, that the device is running. The parent might be affected, but that is running anyways since we are probing a child device.
- pm_runtime_enable(&i2c->dev);
- ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_da7213, &da7213_dai, 1); if (ret < 0) {
@@ -1876,6 +1924,34 @@ static int da7213_i2c_probe(struct i2c_client *i2c, return ret; }
+static int __maybe_unused da7213_runtime_suspend(struct device *dev) +{
- struct da7213_priv *da7213 = dev_get_drvdata(dev);
- regcache_cache_only(da7213->regmap, true);
- regcache_mark_dirty(da7213->regmap);
- regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
- return 0;
+}
+static int __maybe_unused da7213_runtime_resume(struct device *dev) +{
- struct da7213_priv *da7213 = dev_get_drvdata(dev);
- int ret;
- ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
supplies);
- if (ret < 0)
return ret;- regcache_cache_only(da7213->regmap, false);
- regcache_sync(da7213->regmap);
- return 0;
+}
+static const struct dev_pm_ops da7213_pm = {
- SET_RUNTIME_PM_OPS(da7213_runtime_suspend,
da7213_runtime_resume, NULL) +};
static const struct i2c_device_id da7213_i2c_id[] = { { "da7213", 0 }, { } @@ -1888,6 +1964,7 @@ static struct i2c_driver da7213_i2c_driver = { .name = "da7213", .of_match_table = of_match_ptr(da7213_of_match), .acpi_match_table = ACPI_PTR(da7213_acpi_match),
.pm = &da7213_pm,diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h index 3250a3821fcc..3890829dfb6e 100644 --- a/sound/soc/codecs/da7213.h +++ b/sound/soc/codecs/da7213.h @@ -12,6 +12,7 @@
#include <linux/clk.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <sound/da7213.h>
/* @@ -521,9 +522,17 @@ enum da7213_sys_clk { DA7213_SYSCLK_PLL_32KHZ };
+/* Regulators */ +enum da7213_supplies {
- DA7213_SUPPLY_VDDA = 0,
- DA7213_SUPPLY_VDDIO,
- DA7213_NUM_SUPPLIES,
+};
/* Codec private data */ struct da7213_priv { struct regmap *regmap;
- struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES]; struct clk *mclk; unsigned int mclk_rate; int clk_src;
-- 2.24.0
On 22 November 2019 17:10, Sebastian Reichel wrote:
Hi,
Ok, based on feedback I have no further comment so:
Reviewed-by: Adam Thomson Adam.Thomson.Opensource@diasemi.com
On Thu, Nov 21, 2019 at 09:15:02PM +0000, Adam Thomson wrote:
On 20 November 2019 15:24, Sebastian Reichel wrote:
This adds support for most regulators of da7212 for improved power management. The only thing skipped was the speaker supply, which has some undocumented dependencies. It's supposed to be either always-enabled or always-disabled.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
.../devicetree/bindings/sound/da7213.txt | 4 + sound/soc/codecs/da7213.c | 79 ++++++++++++++++++- sound/soc/codecs/da7213.h | 9 +++ 3 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt index 759bb04e0283..cc8200b7d748 100644 --- a/Documentation/devicetree/bindings/sound/da7213.txt +++ b/Documentation/devicetree/bindings/sound/da7213.txt @@ -21,6 +21,10 @@ Optional properties:
- dlg,dmic-clkrate : DMIC clock frequency (Hz). [<1500000>, <3000000>]
- VDDA-supply : Regulator phandle for Analogue power supply
- VDDMIC-supply : Regulator phandle for Mic Bias
- VDDIO-supply : Regulator phandle for I/O power supply
======
Example: diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index aff306bb58df..0359249118d0 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -19,6 +19,7 @@ #include <linux/module.h> #include <sound/pcm.h> #include <sound/pcm_params.h> +#include <linux/pm_runtime.h> #include <sound/soc.h> #include <sound/initval.h> #include <sound/tlv.h> @@ -806,6 +807,11 @@ static int da7213_dai_event(struct snd_soc_dapm_widget *w, */
static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
- /*
* Power Supply*/- SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
- /*
*/
- Input & Output
@@ -932,6 +938,9 @@ static const struct snd_soc_dapm_route da7213_audio_map[] = { /* Dest Connecting Widget source */
/* Input path */
- {"Mic Bias 1", NULL, "VDDMIC"},
- {"Mic Bias 2", NULL, "VDDMIC"},
- {"MIC1", NULL, "Mic Bias 1"}, {"MIC2", NULL, "Mic Bias 2"},
@@ -1691,6 +1700,8 @@ static int da7213_probe(struct snd_soc_component *component) { struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
- pm_runtime_get_sync(component->dev);
It seems that this function can return errors, although I do see lots of instances of this being called where the return value isn't checked. Not had time to walk the code fully but are we sure no errors are going to happen here?
In this case, the runtime PM is already enabled because of pm_runtime_set_active() being called previously. So this only increases the usage counter.
- /* Default to using ALC auto offset calibration mode. */ snd_soc_component_update_bits(component, DA7213_ALC_CTRL1, DA7213_ALC_CALIB_MODE_MAN, 0);
@@ -1811,6 +1822,8 @@ static int da7213_probe(struct snd_soc_component *component) DA7213_DMIC_CLK_RATE_MASK, dmic_cfg); }
- pm_runtime_put_sync(component->dev);
Same question here.
da7213_runtime_suspend() always returns 0.
- /* Check if MCLK provided */ da7213->mclk = devm_clk_get(component->dev, "mclk"); if (IS_ERR(da7213->mclk)) {
@@ -1848,11 +1861,22 @@ static const struct regmap_config da7213_regmap_config = { .cache_type = REGCACHE_RBTREE, };
+static void da7213_power_off(void *data) +{
- struct da7213_priv *da7213 = data;
- regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
+}
+static const char *da7213_supply_names[DA7213_NUM_SUPPLIES] = {
- [DA7213_SUPPLY_VDDA] = "VDDA",
- [DA7213_SUPPLY_VDDIO] = "VDDIO",
+};
static int da7213_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct da7213_priv *da7213;
- int ret;
int i, ret;
da7213 = devm_kzalloc(&i2c->dev, sizeof(*da7213), GFP_KERNEL); if (!da7213)
@@ -1860,6 +1884,25 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, da7213);
- /* Get required supplies */
- for (i = 0; i < DA7213_NUM_SUPPLIES; ++i)
da7213->supplies[i].supply = da7213_supply_names[i];- ret = devm_regulator_bulk_get(&i2c->dev, DA7213_NUM_SUPPLIES,
da7213->supplies);- if (ret) {
dev_err(&i2c->dev, "Failed to get supplies: %d\n", ret);return ret;- }
- ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
supplies);
- if (ret < 0)
return ret;- ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
da7213);
- if (ret < 0)
return ret;- da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config); if (IS_ERR(da7213->regmap)) { ret = PTR_ERR(da7213->regmap);
@@ -1867,6 +1910,11 @@ static int da7213_i2c_probe(struct i2c_client *i2c, return ret; }
- pm_runtime_set_autosuspend_delay(&i2c->dev, 100);
- pm_runtime_use_autosuspend(&i2c->dev);
- pm_runtime_set_active(&i2c->dev);
Again this can return an error. Are we certain this won't fail?
This only provides the information, that the device is running. The parent might be affected, but that is running anyways since we are probing a child device.
- pm_runtime_enable(&i2c->dev);
- ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_da7213, &da7213_dai, 1); if (ret < 0) {
@@ -1876,6 +1924,34 @@ static int da7213_i2c_probe(struct i2c_client *i2c, return ret; }
+static int __maybe_unused da7213_runtime_suspend(struct device *dev) +{
- struct da7213_priv *da7213 = dev_get_drvdata(dev);
- regcache_cache_only(da7213->regmap, true);
- regcache_mark_dirty(da7213->regmap);
- regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
- return 0;
+}
+static int __maybe_unused da7213_runtime_resume(struct device *dev) +{
- struct da7213_priv *da7213 = dev_get_drvdata(dev);
- int ret;
- ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
supplies);
- if (ret < 0)
return ret;- regcache_cache_only(da7213->regmap, false);
- regcache_sync(da7213->regmap);
- return 0;
+}
+static const struct dev_pm_ops da7213_pm = {
- SET_RUNTIME_PM_OPS(da7213_runtime_suspend,
da7213_runtime_resume, NULL) +};
static const struct i2c_device_id da7213_i2c_id[] = { { "da7213", 0 }, { } @@ -1888,6 +1964,7 @@ static struct i2c_driver da7213_i2c_driver = { .name = "da7213", .of_match_table = of_match_ptr(da7213_of_match), .acpi_match_table = ACPI_PTR(da7213_acpi_match),
.pm = &da7213_pm,diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h index 3250a3821fcc..3890829dfb6e 100644 --- a/sound/soc/codecs/da7213.h +++ b/sound/soc/codecs/da7213.h @@ -12,6 +12,7 @@
#include <linux/clk.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <sound/da7213.h>
/* @@ -521,9 +522,17 @@ enum da7213_sys_clk { DA7213_SYSCLK_PLL_32KHZ };
+/* Regulators */ +enum da7213_supplies {
- DA7213_SUPPLY_VDDA = 0,
- DA7213_SUPPLY_VDDIO,
- DA7213_NUM_SUPPLIES,
+};
/* Codec private data */ struct da7213_priv { struct regmap *regmap;
- struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES]; struct clk *mclk; unsigned int mclk_rate; int clk_src;
-- 2.24.0
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
On 20 November 2019 15:24, Sebastian Reichel wrote:
This commit adds the Dialog DA7213 audio codec as a selectable option in the kernel config. Currently the driver can only be selected for Intel Baytrail/Cherrytrail devices or if SND_SOC_ALL_CODECS is enabled.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
Reviewed-by: Adam Thomson Adam.Thomson.Opensource@diasemi.com
sound/soc/codecs/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 229cc89f8c5a..1d44fbc3d407 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -646,7 +646,8 @@ config SND_SOC_DA7210 tristate
config SND_SOC_DA7213
tristate
- tristate "Dialog DA7213 CODEC"
- depends on I2C
config SND_SOC_DA7218 tristate -- 2.24.0
Move set_sysclk function to component level, so that it can be used at both component and DAI level.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com --- sound/soc/codecs/da7213.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index 0359249118d0..9686948b16ea 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -1343,10 +1343,10 @@ static int da7213_mute(struct snd_soc_dai *dai, int mute) #define DA7213_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\ SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
-static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai, - int clk_id, unsigned int freq, int dir) +static int da7213_set_component_sysclk(struct snd_soc_component *component, + int clk_id, int source, + unsigned int freq, int dir) { - struct snd_soc_component *component = codec_dai->component; struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component); int ret = 0;
@@ -1354,7 +1354,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai, return 0;
if (((freq < 5000000) && (freq != 32768)) || (freq > 54000000)) { - dev_err(codec_dai->dev, "Unsupported MCLK value %d\n", + dev_err(component->dev, "Unsupported MCLK value %d\n", freq); return -EINVAL; } @@ -1370,7 +1370,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai, DA7213_PLL_MCLK_SQR_EN); break; default: - dev_err(codec_dai->dev, "Unknown clock source %d\n", clk_id); + dev_err(component->dev, "Unknown clock source %d\n", clk_id); return -EINVAL; }
@@ -1380,7 +1380,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai, freq = clk_round_rate(da7213->mclk, freq); ret = clk_set_rate(da7213->mclk, freq); if (ret) { - dev_err(codec_dai->dev, "Failed to set clock rate %d\n", + dev_err(component->dev, "Failed to set clock rate %d\n", freq); return ret; } @@ -1507,7 +1507,6 @@ static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id, static const struct snd_soc_dai_ops da7213_dai_ops = { .hw_params = da7213_hw_params, .set_fmt = da7213_set_dai_fmt, - .set_sysclk = da7213_set_dai_sysclk, .set_pll = da7213_set_dai_pll, .digital_mute = da7213_mute, }; @@ -1845,6 +1844,7 @@ static const struct snd_soc_component_driver soc_component_dev_da7213 = { .num_dapm_widgets = ARRAY_SIZE(da7213_dapm_widgets), .dapm_routes = da7213_audio_map, .num_dapm_routes = ARRAY_SIZE(da7213_audio_map), + .set_sysclk = da7213_set_component_sysclk, .idle_bias_on = 1, .use_pmdown_time = 1, .endianness = 1,
On 20 November 2019 15:24, Sebastian Reichel wrote:
Move set_sysclk function to component level, so that it can be used at both component and DAI level.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
Reviewed-by: Adam Thomson Adam.Thomson.Opensource@diasemi.com
sound/soc/codecs/da7213.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index 0359249118d0..9686948b16ea 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -1343,10 +1343,10 @@ static int da7213_mute(struct snd_soc_dai *dai, int mute) #define DA7213_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\ SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
-static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai,
int clk_id, unsigned int freq, int dir)+static int da7213_set_component_sysclk(struct snd_soc_component *component,
int clk_id, int source,unsigned int freq, int dir){
- struct snd_soc_component *component = codec_dai->component; struct da7213_priv *da7213 =
snd_soc_component_get_drvdata(component); int ret = 0;
@@ -1354,7 +1354,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai, return 0;
if (((freq < 5000000) && (freq != 32768)) || (freq > 54000000)) {
dev_err(codec_dai->dev, "Unsupported MCLK value %d\n",
dev_err(component->dev, "Unsupported MCLK value %d\n", freq);@@ -1370,7 +1370,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai, DA7213_PLL_MCLK_SQR_EN); break; default:
dev_err(codec_dai->dev, "Unknown clock source %d\n", clk_id);
dev_err(component->dev, "Unknown clock source %d\n",clk_id); return -EINVAL; }
@@ -1380,7 +1380,7 @@ static int da7213_set_dai_sysclk(struct snd_soc_dai *codec_dai, freq = clk_round_rate(da7213->mclk, freq); ret = clk_set_rate(da7213->mclk, freq); if (ret) {
dev_err(codec_dai->dev, "Failed to set clock rate %d\n",
dev_err(component->dev, "Failed to set clock rate%d\n", freq); return ret; } @@ -1507,7 +1507,6 @@ static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id, static const struct snd_soc_dai_ops da7213_dai_ops = { .hw_params = da7213_hw_params, .set_fmt = da7213_set_dai_fmt,
- .set_sysclk = da7213_set_dai_sysclk, .set_pll = da7213_set_dai_pll, .digital_mute = da7213_mute,
}; @@ -1845,6 +1844,7 @@ static const struct snd_soc_component_driver soc_component_dev_da7213 = { .num_dapm_widgets = ARRAY_SIZE(da7213_dapm_widgets), .dapm_routes = da7213_audio_map, .num_dapm_routes = ARRAY_SIZE(da7213_audio_map),
- .set_sysclk = da7213_set_component_sysclk, .idle_bias_on = 1, .use_pmdown_time = 1, .endianness = 1,
-- 2.24.0
Move set_pll function to component level, so that it can be used at both component and DAI level.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com --- sound/soc/codecs/da7213.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index 9686948b16ea..3e6ad996741b 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -1392,10 +1392,10 @@ static int da7213_set_component_sysclk(struct snd_soc_component *component, }
/* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */ -static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id, - int source, unsigned int fref, unsigned int fout) +static int da7213_set_component_pll(struct snd_soc_component *component, + int pll_id, int source, + unsigned int fref, unsigned int fout) { - struct snd_soc_component *component = codec_dai->component; struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
u8 pll_ctrl, indiv_bits, indiv; @@ -1507,7 +1507,6 @@ static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id, static const struct snd_soc_dai_ops da7213_dai_ops = { .hw_params = da7213_hw_params, .set_fmt = da7213_set_dai_fmt, - .set_pll = da7213_set_dai_pll, .digital_mute = da7213_mute, };
@@ -1845,6 +1844,7 @@ static const struct snd_soc_component_driver soc_component_dev_da7213 = { .dapm_routes = da7213_audio_map, .num_dapm_routes = ARRAY_SIZE(da7213_audio_map), .set_sysclk = da7213_set_component_sysclk, + .set_pll = da7213_set_component_pll, .idle_bias_on = 1, .use_pmdown_time = 1, .endianness = 1,
On 20 November 2019 15:24, Sebastian Reichel wrote:
Move set_pll function to component level, so that it can be used at both component and DAI level.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
Reviewed-by: Adam Thomson Adam.Thomson.Opensource@diasemi.com
sound/soc/codecs/da7213.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index 9686948b16ea..3e6ad996741b 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -1392,10 +1392,10 @@ static int da7213_set_component_sysclk(struct snd_soc_component *component, }
/* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */ -static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
int source, unsigned int fref, unsigned int fout)+static int da7213_set_component_pll(struct snd_soc_component *component,
int pll_id, int source,unsigned int fref, unsigned int fout){
- struct snd_soc_component *component = codec_dai->component; struct da7213_priv *da7213 =
snd_soc_component_get_drvdata(component);
u8 pll_ctrl, indiv_bits, indiv; @@ -1507,7 +1507,6 @@ static int da7213_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id, static const struct snd_soc_dai_ops da7213_dai_ops = { .hw_params = da7213_hw_params, .set_fmt = da7213_set_dai_fmt,
- .set_pll = da7213_set_dai_pll, .digital_mute = da7213_mute,
};
@@ -1845,6 +1844,7 @@ static const struct snd_soc_component_driver soc_component_dev_da7213 = { .dapm_routes = da7213_audio_map, .num_dapm_routes = ARRAY_SIZE(da7213_audio_map), .set_sysclk = da7213_set_component_sysclk,
- .set_pll = da7213_set_component_pll, .idle_bias_on = 1, .use_pmdown_time = 1, .endianness = 1,
-- 2.24.0
This adds default clock/PLL configuration to the driver for usage with generic drivers like simple-card for usage with a fixed rate clock.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com --- sound/soc/codecs/da7213.c | 75 ++++++++++++++++++++++++++++++++++++--- sound/soc/codecs/da7213.h | 2 ++ 2 files changed, 73 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index 3e6ad996741b..ff1a936240be 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -1156,6 +1156,7 @@ static int da7213_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct snd_soc_component *component = dai->component; + struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component); u8 dai_ctrl = 0; u8 fs;
@@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct snd_pcm_substream *substream, switch (params_rate(params)) { case 8000: fs = DA7213_SR_8000; + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000; break; case 11025: fs = DA7213_SR_11025; + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800; break; case 12000: fs = DA7213_SR_12000; + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000; break; case 16000: fs = DA7213_SR_16000; + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000; break; case 22050: fs = DA7213_SR_22050; + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800; break; case 32000: fs = DA7213_SR_32000; + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000; break; case 44100: fs = DA7213_SR_44100; + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800; break; case 48000: fs = DA7213_SR_48000; + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000; break; case 88200: fs = DA7213_SR_88200; + da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800; break; case 96000: fs = DA7213_SR_96000; + da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000; break; default: return -EINVAL; @@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct snd_soc_component *component, }
/* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */ -static int da7213_set_component_pll(struct snd_soc_component *component, - int pll_id, int source, - unsigned int fref, unsigned int fout) +static int _da7213_set_component_pll(struct snd_soc_component *component, + int pll_id, int source, + unsigned int fref, unsigned int fout) { struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
@@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct snd_soc_component *component, return 0; }
+static int da7213_set_component_pll(struct snd_soc_component *component, + int pll_id, int source, + unsigned int fref, unsigned int fout) +{ + struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component); + da7213->fixed_clk_auto_pll = false; + + return _da7213_set_component_pll(component, pll_id, source, fref, fout); +} + /* DAI operations */ static const struct snd_soc_dai_ops da7213_dai_ops = { .hw_params = da7213_hw_params, @@ -1532,6 +1553,43 @@ static struct snd_soc_dai_driver da7213_dai = { .symmetric_rates = 1, };
+static int da7213_set_auto_pll(struct snd_soc_component *component, bool enable) +{ + struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component); + int mode; + + if (!da7213->fixed_clk_auto_pll) + return 0; + + da7213->mclk_rate = clk_get_rate(da7213->mclk); + + if (enable) + mode = DA7213_SYSCLK_PLL; + else + mode = DA7213_SYSCLK_MCLK; + + switch (da7213->out_rate) { + case DA7213_PLL_FREQ_OUT_90316800: + if (da7213->mclk_rate == 11289600 || + da7213->mclk_rate == 22579200 || + da7213->mclk_rate == 45158400) + mode = DA7213_SYSCLK_MCLK; + break; + case DA7213_PLL_FREQ_OUT_98304000: + if (da7213->mclk_rate == 12288000 || + da7213->mclk_rate == 24576000 || + da7213->mclk_rate == 49152000) + mode = DA7213_SYSCLK_MCLK; + + break; + default: + return -1; + } + + return _da7213_set_component_pll(component, 0, mode, + da7213->mclk_rate, da7213->out_rate); +} + static int da7213_set_bias_level(struct snd_soc_component *component, enum snd_soc_bias_level level) { @@ -1551,6 +1609,8 @@ static int da7213_set_bias_level(struct snd_soc_component *component, "Failed to enable mclk\n"); return ret; } + + da7213_set_auto_pll(component, true); } } break; @@ -1562,8 +1622,10 @@ static int da7213_set_bias_level(struct snd_soc_component *component, DA7213_VMID_EN | DA7213_BIAS_EN); } else { /* Remove MCLK */ - if (da7213->mclk) + if (da7213->mclk) { + da7213_set_auto_pll(component, false); clk_disable_unprepare(da7213->mclk); + } } break; case SND_SOC_BIAS_OFF: @@ -1829,6 +1891,11 @@ static int da7213_probe(struct snd_soc_component *component) return PTR_ERR(da7213->mclk); else da7213->mclk = NULL; + } else { + /* Do automatic PLL handling assuming fixed clock until + * set_pll() has been called. This makes the codec usable + * with the simple-audio-card driver. */ + da7213->fixed_clk_auto_pll = true; }
return 0; diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h index 3890829dfb6e..97ccf0ddd2be 100644 --- a/sound/soc/codecs/da7213.h +++ b/sound/soc/codecs/da7213.h @@ -535,10 +535,12 @@ struct da7213_priv { struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES]; struct clk *mclk; unsigned int mclk_rate; + unsigned int out_rate; int clk_src; bool master; bool alc_calib_auto; bool alc_en; + bool fixed_clk_auto_pll; struct da7213_platform_data *pdata; };
On 20 November 2019 15:24, Sebastian Reichel wrote:
This adds default clock/PLL configuration to the driver for usage with generic drivers like simple-card for usage with a fixed rate clock.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
sound/soc/codecs/da7213.c | 75 ++++++++++++++++++++++++++++++++++++--- sound/soc/codecs/da7213.h | 2 ++ 2 files changed, 73 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index 3e6ad996741b..ff1a936240be 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -1156,6 +1156,7 @@ static int da7213_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct snd_soc_component *component = dai->component;
- struct da7213_priv *da7213 =
snd_soc_component_get_drvdata(component); u8 dai_ctrl = 0; u8 fs;
@@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct snd_pcm_substream *substream, switch (params_rate(params)) { case 8000: fs = DA7213_SR_8000;
da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;@@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct snd_soc_component *component, }
/* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */ -static int da7213_set_component_pll(struct snd_soc_component *component,
int pll_id, int source,unsigned int fref, unsigned int fout)+static int _da7213_set_component_pll(struct snd_soc_component *component,
int pll_id, int source,unsigned int fref, unsigned int fout){ struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
@@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct snd_soc_component *component, return 0; }
+static int da7213_set_component_pll(struct snd_soc_component *component,
int pll_id, int source,unsigned int fref, unsigned int fout)+{
- struct da7213_priv *da7213 =
snd_soc_component_get_drvdata(component);
- da7213->fixed_clk_auto_pll = false;
- return _da7213_set_component_pll(component, pll_id, source, fref,
fout); +}
/* DAI operations */ static const struct snd_soc_dai_ops da7213_dai_ops = { .hw_params = da7213_hw_params, @@ -1532,6 +1553,43 @@ static struct snd_soc_dai_driver da7213_dai = { .symmetric_rates = 1, };
+static int da7213_set_auto_pll(struct snd_soc_component *component, bool enable) +{
- struct da7213_priv *da7213 =
snd_soc_component_get_drvdata(component);
- int mode;
- if (!da7213->fixed_clk_auto_pll)
return 0;- da7213->mclk_rate = clk_get_rate(da7213->mclk);
- if (enable)
mode = DA7213_SYSCLK_PLL;- else
mode = DA7213_SYSCLK_MCLK;
If we're the clock slave, and we're using an MCLK that's not a harmonic then SRM is required to synchronise the PLL to the incoming WCLK signal. I assume simple sound card should allow for both master and slave modes? If so we'll need to do something here to determine this as well.
- switch (da7213->out_rate) {
- case DA7213_PLL_FREQ_OUT_90316800:
if (da7213->mclk_rate == 11289600 ||da7213->mclk_rate == 22579200 ||da7213->mclk_rate == 45158400)mode = DA7213_SYSCLK_MCLK;break;- case DA7213_PLL_FREQ_OUT_98304000:
if (da7213->mclk_rate == 12288000 ||da7213->mclk_rate == 24576000 ||da7213->mclk_rate == 49152000)mode = DA7213_SYSCLK_MCLK;break;- default:
return -1;- }
- return _da7213_set_component_pll(component, 0, mode,
da7213->mclk_rate, da7213->out_rate);+}
static int da7213_set_bias_level(struct snd_soc_component *component, enum snd_soc_bias_level level) { @@ -1551,6 +1609,8 @@ static int da7213_set_bias_level(struct snd_soc_component *component, "Failed to enable mclk\n"); return ret; }
da7213_set_auto_pll(component, true);
I've thought more about this and for the scenario where a machine driver controls the PLL through a DAPM widget associated with this codec (like some Intel boards do), then the PLL will be configured once here and then again when the relevant widget is called. I don't think that will matter but I will take a further look just in case this might cause some oddities.
} } break;@@ -1562,8 +1622,10 @@ static int da7213_set_bias_level(struct snd_soc_component *component, DA7213_VMID_EN | DA7213_BIAS_EN); } else { /* Remove MCLK */
if (da7213->mclk)
if (da7213->mclk) {da7213_set_auto_pll(component, false); clk_disable_unprepare(da7213->mclk);}@@ -1829,6 +1891,11 @@ static int da7213_probe(struct snd_soc_component *component) return PTR_ERR(da7213->mclk); else da7213->mclk = NULL;
} else {
/* Do automatic PLL handling assuming fixed clock until* set_pll() has been called. This makes the codec usable* with the simple-audio-card driver. */da7213->fixed_clk_auto_pll = true;}
return 0;
diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h index 3890829dfb6e..97ccf0ddd2be 100644 --- a/sound/soc/codecs/da7213.h +++ b/sound/soc/codecs/da7213.h @@ -535,10 +535,12 @@ struct da7213_priv { struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES]; struct clk *mclk; unsigned int mclk_rate;
- unsigned int out_rate; int clk_src; bool master; bool alc_calib_auto; bool alc_en;
- bool fixed_clk_auto_pll; struct da7213_platform_data *pdata;
};
-- 2.24.0
On 21 November 2019 21:49, Adam Thomson wrote:
On 20 November 2019 15:24, Sebastian Reichel wrote:
This adds default clock/PLL configuration to the driver for usage with generic drivers like simple-card for usage with a fixed rate clock.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
sound/soc/codecs/da7213.c | 75 ++++++++++++++++++++++++++++++++++++--- sound/soc/codecs/da7213.h | 2 ++ 2 files changed, 73 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index 3e6ad996741b..ff1a936240be 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -1156,6 +1156,7 @@ static int da7213_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct snd_soc_component *component = dai->component;
- struct da7213_priv *da7213 =
snd_soc_component_get_drvdata(component); u8 dai_ctrl = 0; u8 fs;
@@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct snd_pcm_substream *substream, switch (params_rate(params)) { case 8000: fs = DA7213_SR_8000;
da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;@@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct snd_soc_component *component, }
/* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */ -static int da7213_set_component_pll(struct snd_soc_component
*component,
int pll_id, int source,unsigned int fref, unsigned int fout)+static int _da7213_set_component_pll(struct snd_soc_component *component,
int pll_id, int source,unsigned int fref, unsigned int fout){ struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
@@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct snd_soc_component *component, return 0; }
+static int da7213_set_component_pll(struct snd_soc_component
*component,
int pll_id, int source,unsigned int fref, unsigned int fout)+{
- struct da7213_priv *da7213 =
snd_soc_component_get_drvdata(component);
- da7213->fixed_clk_auto_pll = false;
- return _da7213_set_component_pll(component, pll_id, source, fref,
fout); +}
/* DAI operations */ static const struct snd_soc_dai_ops da7213_dai_ops = { .hw_params = da7213_hw_params, @@ -1532,6 +1553,43 @@ static struct snd_soc_dai_driver da7213_dai = { .symmetric_rates = 1, };
+static int da7213_set_auto_pll(struct snd_soc_component *component, bool enable) +{
- struct da7213_priv *da7213 =
snd_soc_component_get_drvdata(component);
- int mode;
- if (!da7213->fixed_clk_auto_pll)
return 0;- da7213->mclk_rate = clk_get_rate(da7213->mclk);
- if (enable)
mode = DA7213_SYSCLK_PLL;- else
mode = DA7213_SYSCLK_MCLK;If we're the clock slave, and we're using an MCLK that's not a harmonic then SRM is required to synchronise the PLL to the incoming WCLK signal. I assume simple sound card should allow for both master and slave modes? If so we'll need to do something here to determine this as well.
- switch (da7213->out_rate) {
- case DA7213_PLL_FREQ_OUT_90316800:
if (da7213->mclk_rate == 11289600 ||da7213->mclk_rate == 22579200 ||da7213->mclk_rate == 45158400)mode = DA7213_SYSCLK_MCLK;break;- case DA7213_PLL_FREQ_OUT_98304000:
if (da7213->mclk_rate == 12288000 ||da7213->mclk_rate == 24576000 ||da7213->mclk_rate == 49152000)mode = DA7213_SYSCLK_MCLK;break;- default:
return -1;- }
- return _da7213_set_component_pll(component, 0, mode,
da7213->mclk_rate, da7213->out_rate);+}
static int da7213_set_bias_level(struct snd_soc_component *component, enum snd_soc_bias_level level) { @@ -1551,6 +1609,8 @@ static int da7213_set_bias_level(struct snd_soc_component *component, "Failed to enable mclk\n"); return ret; }
da7213_set_auto_pll(component, true);I've thought more about this and for the scenario where a machine driver controls the PLL through a DAPM widget associated with this codec (like some Intel boards do), then the PLL will be configured once here and then again when the relevant widget is called. I don't think that will matter but I will take a further look just in case this might cause some oddities.
So I don't see any issues per say with the PLL function being called twice in the example I mentioned. However it still feels a bit clunky; You either live with it or you have something in the machine driver to call the codec's PLL function early doors to prevent the bias_level() code of the codec controlling the PLL automatically. Am wondering though if there would be some use in having an indicator that simple-card is being used so we can avoid this? I guess we could check the parent sound card instance's OF node to look at the compatible string and see if it's a simple-card instantiation. Bit messy maybe. Alternatively would it be worthwhile storing in the snd_soc_card instance that this is a simple-card instantiation? We could then use that to determine whether the codec driver should automatically deal with the PLL or not. This would of course require an update to the snd_soc_card structure to add the new flag and then some update to simple-card.c to flag this.
I think relying on the timing of the call to the codec's PLL configuration function from a machine driver isn't ideal.
} } break;@@ -1562,8 +1622,10 @@ static int da7213_set_bias_level(struct snd_soc_component *component, DA7213_VMID_EN | DA7213_BIAS_EN); } else { /* Remove MCLK */
if (da7213->mclk)
if (da7213->mclk) {da7213_set_auto_pll(component, false); clk_disable_unprepare(da7213->mclk);}@@ -1829,6 +1891,11 @@ static int da7213_probe(struct snd_soc_component *component) return PTR_ERR(da7213->mclk); else da7213->mclk = NULL;
} else {
/* Do automatic PLL handling assuming fixed clock until* set_pll() has been called. This makes the codec usable* with the simple-audio-card driver. */da7213->fixed_clk_auto_pll = true;}
return 0;
diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h index 3890829dfb6e..97ccf0ddd2be 100644 --- a/sound/soc/codecs/da7213.h +++ b/sound/soc/codecs/da7213.h @@ -535,10 +535,12 @@ struct da7213_priv { struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES]; struct clk *mclk; unsigned int mclk_rate;
- unsigned int out_rate; int clk_src; bool master; bool alc_calib_auto; bool alc_en;
- bool fixed_clk_auto_pll; struct da7213_platform_data *pdata;
};
-- 2.24.0
On Tue, Nov 26, 2019 at 04:55:39PM +0000, Adam Thomson wrote:
On 21 November 2019 21:49, Adam Thomson wrote:
On 20 November 2019 15:24, Sebastian Reichel wrote:
I've thought more about this and for the scenario where a machine driver controls the PLL through a DAPM widget associated with this codec (like some Intel boards do), then the PLL will be configured once here and then again when the relevant widget is called. I don't think that will matter but I will take a further look just in case this might cause some oddities.
So I don't see any issues per say with the PLL function being called twice in the example I mentioned. However it still feels a bit clunky; You either live with it or you have something in the machine driver to call the codec's PLL function early doors to prevent the bias_level() code of the codec controlling the PLL automatically. Am wondering though if there would be some use in having an indicator that simple-card is being used so we can avoid this? I guess we
If we're special casing simple-card we're doing it wrong - there's nothing stopping any other machine driver behaving in the same way.
On 26 November 2019 17:09, Mark Brown wrote:
On Tue, Nov 26, 2019 at 04:55:39PM +0000, Adam Thomson wrote:
On 21 November 2019 21:49, Adam Thomson wrote:
On 20 November 2019 15:24, Sebastian Reichel wrote:
I've thought more about this and for the scenario where a machine driver controls the PLL through a DAPM widget associated with this codec (like some Intel boards do), then the PLL will be configured once here and then again when the relevant widget is called. I don't think that will matter but I will take a further look just in case this might cause some oddities.
So I don't see any issues per say with the PLL function being called twice in the example I mentioned. However it still feels a bit clunky; You either live with it or you have something in the machine driver to call the codec's PLL function early doors to prevent the bias_level() code of the codec controlling the PLL automatically. Am wondering though if there would be some use in
having
an indicator that simple-card is being used so we can avoid this? I guess we
If we're special casing simple-card we're doing it wrong - there's nothing stopping any other machine driver behaving in the same way.
Ok, what's being proposed here is for the codec to automatically control the PLL rather than leaving it to the machine driver as is the case right now. In the possible scenario where this is done using a card level widget to enable/disable the PLL we will conflict with that using the current suggested approach for the da7213 driver, albeit with no real consequence other than configuring the PLL twice the first time a stream is started. It's a case of how to determine who's in control of the PLL here; machine driver or codec?
On Tue, Nov 26, 2019 at 05:39:45PM +0000, Adam Thomson wrote:
On 26 November 2019 17:09, Mark Brown wrote:
If we're special casing simple-card we're doing it wrong - there's nothing stopping any other machine driver behaving in the same way.
Ok, what's being proposed here is for the codec to automatically control the PLL rather than leaving it to the machine driver as is the case right now. In the possible scenario where this is done using a card level widget to enable/disable
Wasn't the proposal to add a new mode where the machine driver *could* choose to delgate PLL selection to the CODEC driver rather than do so unconditionally?
the PLL we will conflict with that using the current suggested approach for the da7213 driver, albeit with no real consequence other than configuring the PLL twice the first time a stream is started. It's a case of how to determine who's in control of the PLL here; machine driver or codec?
The patch looked like the idea was that as soon as the machine driver configures the PLL the CODEC driver will stop touching it which looked like a reasonable way of doing it, you could also do it with an explicit SYSCLK value (which would have to be 0 for generic machine drivers to pick it up) but this works just as well and may even be better. Perhaps I misread it though.
On 26 November 2019 17:51, Mark Brown wrote:
On Tue, Nov 26, 2019 at 05:39:45PM +0000, Adam Thomson wrote:
On 26 November 2019 17:09, Mark Brown wrote:
If we're special casing simple-card we're doing it wrong - there's nothing stopping any other machine driver behaving in the same way.
Ok, what's being proposed here is for the codec to automatically control the PLL rather than leaving it to the machine driver as is the case right now. In the possible scenario where this is done using a card level widget to enable/disable
Wasn't the proposal to add a new mode where the machine driver *could* choose to delgate PLL selection to the CODEC driver rather than do so unconditionally?
Yes, but by default the assumption is the codec will do things automatically until the machine driver calls the relevant PLL config code.....
the PLL we will conflict with that using the current suggested approach for the da7213 driver, albeit with no real consequence other than configuring the PLL twice the first time a stream is started. It's a case of how to determine who's in control of the PLL here; machine driver or codec?
The patch looked like the idea was that as soon as the machine driver configures the PLL the CODEC driver will stop touching it which looked like a reasonable way of doing it, you could also do it with an explicit SYSCLK value (which would have to be 0 for generic machine drivers to pick it up) but this works just as well and may even be better. Perhaps I misread it though.
...so yes you're right in your comment here. However the bias_level code is called prior to the DAPM paths being sequenced. If a dedicated machine driver were to want to control the PLL via a DAPM widget at the card level (which is done for other codecs for example on Intel platforms), the code in the bias_level() function of the codec to auto configure the PLL would always be called the very first time a path is enabled before the relevant DAPM widget for the card is called, so you'd have two configurations of the PLL in succession. I don't expect that would cause issues, but it's not ideal. The other approach would be to have something in the machine driver like a dai_link init function to default the PLL config using the codec's PLL function which then prevents any chance of auto configuration subsequently. However that requires the person implementing the machine driver to know about this behaviour.
As I said it's a small thing and requires a specific use-case to occur, but having the PLL configured twice for the very first stream in that scenario seems messy. Regarding the SYSCLK approach you mention, I'm not clear how that would work so I'm obviously missing something. If we had some init stage indication though that auto PLL was required then we're golden.
On Wed, Nov 27, 2019 at 11:32:54AM +0000, Adam Thomson wrote:
As I said it's a small thing and requires a specific use-case to occur, but having the PLL configured twice for the very first stream in that scenario seems messy. Regarding the SYSCLK approach you mention, I'm not clear how that would work so I'm obviously missing something. If we had some init stage indication though that auto PLL was required then we're golden.
There's a bunch of other drivers using the SYSCLK thing, when you call set_sysclk() they provide a different SYSCLK number if they want to use manual mode. If there's a concern about drivers doing stuff on init you could always ask them to set the PLL during init, even if only briefly.
On 27 November 2019 12:33, Mark Brown wrote:
On Wed, Nov 27, 2019 at 11:32:54AM +0000, Adam Thomson wrote:
As I said it's a small thing and requires a specific use-case to occur, but having the PLL configured twice for the very first stream in that scenario seems messy. Regarding the SYSCLK approach you mention, I'm not clear how
that
would work so I'm obviously missing something. If we had some init stage indication though that auto PLL was required then we're golden.
There's a bunch of other drivers using the SYSCLK thing, when you call set_sysclk() they provide a different SYSCLK number if they want to use manual mode. If there's a concern about drivers doing stuff on init you could always ask them to set the PLL during init, even if only briefly.
Yeah OK I see what you mean; using the sysclk id. Still doesn't feel like the nicest solution here though. I guess we're stuck with people having to manually configure the PLL for bypass when a non-generic machine driver inits, to avoid the initial double config, as I don't see other options unless you have something to specify at init that it's auto or manual, and this doesn't feel like a DT device specific property thing as it's more software than hardware. At least Sebastian's patch has a good comment block to highlight this.
On Wed, Nov 27, 2019 at 01:42:54PM +0000, Adam Thomson wrote:
nicest solution here though. I guess we're stuck with people having to manually configure the PLL for bypass when a non-generic machine driver inits, to avoid the initial double config, as I don't see other options unless you have something to specify at init that it's auto or manual, and this doesn't feel like a DT device specific property thing as it's more software than hardware. At least Sebastian's patch has a good comment block to highlight this.
Not sure I follow here - if we're configuring the PLL explicitly then I'd expect the PLL to be configured first, then the SYSCLK, so I'd expect that the automatic PLL configuration wouldn't kick in.
On 27 November 2019 15:41, Mark Brown wrote:
On Wed, Nov 27, 2019 at 01:42:54PM +0000, Adam Thomson wrote:
nicest solution here though. I guess we're stuck with people having to manually configure the PLL for bypass when a non-generic machine driver inits, to avoid the initial double config, as I don't see other options unless you have something to specify at init that it's auto or manual, and this doesn't feel like a DT device specific property thing as it's more software than hardware. At least Sebastian's patch has a good comment block to highlight this.
Not sure I follow here - if we're configuring the PLL explicitly then I'd expect the PLL to be configured first, then the SYSCLK, so I'd expect that the automatic PLL configuration wouldn't kick in.
The PLL in the codec relies on MCLK. The MCLK rate can be specified/configured by a machine driver using the relevant codec sysclk function, as is done in a number of drivers. Surely that has to happen first before we configure the PLL as the PLL functions needs to know what rate is coming in so the correct dividers can be applied for the required internal clocking to match up with the desired sample rates. I guess I'm still missing something regarding your discussion around SYSCLK?
On Wed, Nov 27, 2019 at 04:33:12PM +0000, Adam Thomson wrote:
On 27 November 2019 15:41, Mark Brown wrote:
Not sure I follow here - if we're configuring the PLL explicitly then I'd expect the PLL to be configured first, then the SYSCLK, so I'd expect that the automatic PLL configuration wouldn't kick in.
The PLL in the codec relies on MCLK. The MCLK rate can be specified/configured by a machine driver using the relevant codec sysclk function, as is done in a number of drivers. Surely that has to happen first before we configure the PLL as the PLL functions needs to know what rate is coming in so the correct dividers can be applied for the required internal clocking to match up with the desired sample rates. I guess I'm still missing something regarding your discussion around SYSCLK?
The PLL configuration specifies both input and output clock rates (as well as an input clock source) so if it's got to configure the MCLK I'd expect the driver to figure that out without needing the caller to separately set the MCLK rate.
On 27 November 2019 16:41, Mark Brown wrote:
The PLL in the codec relies on MCLK. The MCLK rate can be specified/configured by a machine driver using the relevant codec sysclk function, as is done in a number of drivers. Surely that has to happen first before we configure the PLL as the PLL functions needs to know what rate is coming in so the correct dividers can be applied for the required internal clocking to match up with the desired sample rates. I guess I'm still missing something regarding your discussion around SYSCLK?
The PLL configuration specifies both input and output clock rates (as well as an input clock source) so if it's got to configure the MCLK I'd expect the driver to figure that out without needing the caller to separately set the MCLK rate.
Yes it does but the name of the function implies it's setting the codec's PLL, not the system clock, whereas the other function implies setting the system clock and not the PLL. Also generally you're only setting the sysclk once whereas you may want to configure and enable/disable the PLL more dynamically, at least for devices which do have a built-in PLL. Of course that could still be handled through the single PLL function call.
Just as an informational, what's the future for these two functions if essentially one is only really required and the other deemed redundant? I would just like to be clear so I'm not falling over things like this in the future, and wasting your time as well. Seems that the PLL call isn't part of simple generic card code so would the be deemed surplus to requirements some point down the line?
On Wed, Nov 27, 2019 at 06:10:00PM +0000, Adam Thomson wrote:
On 27 November 2019 16:41, Mark Brown wrote:
The PLL configuration specifies both input and output clock rates (as well as an input clock source) so if it's got to configure the MCLK I'd expect the driver to figure that out without needing the caller to separately set the MCLK rate.
Yes it does but the name of the function implies it's setting the codec's PLL, not the system clock, whereas the other function implies setting the system clock and not the PLL. Also generally you're only setting the sysclk once whereas you may want to configure and enable/disable the PLL more dynamically, at least for devices which do have a built-in PLL. Of course that could still be handled through the single PLL function call.
I wouldn't be so sure about only setting the SYSCLK once - if you've got an input clock you can configure then you might for example want to switch between 44.1kHz and 48kHz bases, and disable it or run it at very low frequency when idle. In some systems it's as dynamic as a PLL is.
Just as an informational, what's the future for these two functions if essentially one is only really required and the other deemed redundant? I would just like to be clear so I'm not falling over things like this in the future, and wasting your time as well. Seems that the PLL call isn't part of simple generic card code so would the be deemed surplus to requirements some point down the line?
Things like simple-card are good for 90% of systems but I'm fairly sure we'll never be able to handle more complex setups entirely automatically. What we *should* be doing over time is transitioning all this clock stuff into the actual clock framework but that's a big, long term thing which nobody is currently actually working on.
participants (3)
-
Adam Thomson -
Mark Brown -
Sebastian Reichel