The WM5102 and WM8997 codecs have an internal dynamic clock booster.
When this booster is active, the DCVDD voltage must be increased.
If all the currently active audio paths can run with the root SYSCLK
we can disable the booster, allowing us to turn down DCVDD voltage
to save power.
Previously this was being done by having the booster enable bit set
as a side-effect of the LDO1 regulator driver, which is unexpected
behaviour of a regulator and not compatible with using an external
regulator.
This patch exports functions to handle the booster enable and
DCVDD voltage, with each relevant subsystem flagging whether it can
currently run without the booster. Note that these subsystems are
stateless and none of them are nestable, so there's no need for
reference counting, we only need a simple boolean for each subsystem
of whether their current condition could require the booster or will
allow us to turn the codec down to lower operating power.
Signed-off-by: Richard Fitzgerald <[email protected]>
---
sound/soc/codecs/arizona.c | 127 ++++++++++++++++++++++++++++++++++++++++++++
sound/soc/codecs/arizona.h | 13 +++++
sound/soc/codecs/wm5102.c | 12 +++--
sound/soc/codecs/wm8997.c | 11 +++-
4 files changed, 156 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/arizona.c b/sound/soc/codecs/arizona.c
index eff4b4d..69965ae 100644
--- a/sound/soc/codecs/arizona.c
+++ b/sound/soc/codecs/arizona.c
@@ -851,6 +851,133 @@ int arizona_hp_ev(struct snd_soc_dapm_widget *w,
}
EXPORT_SYMBOL_GPL(arizona_hp_ev);
+static int arizona_dvfs_enable(struct snd_soc_codec *codec)
+{
+ const struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
+ struct arizona *arizona = priv->arizona;
+ int ret;
+
+ ret = regulator_set_voltage(arizona->dcvdd, 1800000, 1800000);
+ if (ret) {
+ dev_err(codec->dev, "Failed to boost DCVDD: %d\n", ret);
+ return ret;
+ }
+
+ ret = regmap_update_bits(arizona->regmap,
+ ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
+ ARIZONA_SUBSYS_MAX_FREQ,
+ ARIZONA_SUBSYS_MAX_FREQ);
+ if (ret) {
+ dev_err(codec->dev, "Failed to enable subsys max: %d\n", ret);
+ regulator_set_voltage(arizona->dcvdd, 1200000, 1800000);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int arizona_dvfs_disable(struct snd_soc_codec *codec)
+{
+ const struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
+ struct arizona *arizona = priv->arizona;
+ int ret;
+
+ ret = regmap_update_bits(arizona->regmap,
+ ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
+ ARIZONA_SUBSYS_MAX_FREQ, 0);
+ if (ret) {
+ dev_err(codec->dev, "Failed to disable subsys max: %d\n", ret);
+ return ret;
+ }
+
+ ret = regulator_set_voltage(arizona->dcvdd, 1200000, 1800000);
+ if (ret) {
+ dev_err(codec->dev, "Failed to unboost DCVDD: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+int arizona_dvfs_up(struct snd_soc_codec *codec, unsigned int flags)
+{
+ struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
+ int ret = 0;
+
+ mutex_lock(&priv->dvfs_lock);
+
+ if (!priv->dvfs_cached && !priv->dvfs_reqs) {
+ ret = arizona_dvfs_enable(codec);
+ if (ret)
+ goto err;
+ }
+
+ priv->dvfs_reqs |= flags;
+err:
+ mutex_unlock(&priv->dvfs_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(arizona_dvfs_up);
+
+int arizona_dvfs_down(struct snd_soc_codec *codec, unsigned int flags)
+{
+ struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
+ unsigned int old_reqs = priv->dvfs_reqs;
+ int ret = 0;
+
+ mutex_lock(&priv->dvfs_lock);
+
+ priv->dvfs_reqs &= ~flags;
+
+ if (!priv->dvfs_cached && old_reqs && !priv->dvfs_reqs)
+ ret = arizona_dvfs_disable(codec);
+
+ mutex_unlock(&priv->dvfs_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(arizona_dvfs_down);
+
+int arizona_dvfs_sysclk_ev(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event)
+{
+ struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+ struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
+ int ret = 0;
+
+ mutex_lock(&priv->dvfs_lock);
+
+ switch (event) {
+ case SND_SOC_DAPM_POST_PMU:
+ if (priv->dvfs_reqs)
+ ret = arizona_dvfs_enable(codec);
+
+ priv->dvfs_cached = false;
+ break;
+ case SND_SOC_DAPM_PRE_PMD:
+ /* We must ensure DVFS is disabled before the codec goes into
+ * suspend so that we are never in an illegal state of DVFS
+ * enabled without enough DCVDD
+ */
+ priv->dvfs_cached = true;
+
+ if (priv->dvfs_reqs)
+ ret = arizona_dvfs_disable(codec);
+ break;
+ default:
+ break;
+ }
+
+ mutex_unlock(&priv->dvfs_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(arizona_dvfs_sysclk_ev);
+
+void arizona_init_dvfs(struct arizona_priv *priv)
+{
+ mutex_init(&priv->dvfs_lock);
+}
+EXPORT_SYMBOL_GPL(arizona_init_dvfs);
+
static unsigned int arizona_sysclk_48k_rates[] = {
6144000,
12288000,
diff --git a/sound/soc/codecs/arizona.h b/sound/soc/codecs/arizona.h
index 11ff899..e82ca21 100644
--- a/sound/soc/codecs/arizona.h
+++ b/sound/soc/codecs/arizona.h
@@ -60,6 +60,9 @@
#define ARIZONA_MAX_DAI 6
#define ARIZONA_MAX_ADSP 4
+#define ARIZONA_DVFS_SR1_RQ 0x001
+#define ARIZONA_DVFS_ADSP1_RQ 0x100
+
struct arizona;
struct wm_adsp;
@@ -84,6 +87,10 @@ struct arizona_priv {
unsigned int spk_ena:2;
unsigned int spk_ena_pending:1;
+
+ unsigned int dvfs_reqs;
+ struct mutex dvfs_lock;
+ bool dvfs_cached;
};
#define ARIZONA_NUM_MIXER_INPUTS 103
@@ -245,6 +252,12 @@ struct arizona_fll {
char clock_ok_name[ARIZONA_FLL_NAME_LEN];
};
+extern int arizona_dvfs_up(struct snd_soc_codec *codec, unsigned int flags);
+extern int arizona_dvfs_down(struct snd_soc_codec *codec, unsigned int flags);
+extern int arizona_dvfs_sysclk_ev(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event);
+extern void arizona_init_dvfs(struct arizona_priv *priv);
+
extern int arizona_init_fll(struct arizona *arizona, int id, int base,
int lock_irq, int ok_irq, struct arizona_fll *fll);
extern int arizona_set_fll_refclk(struct arizona_fll *fll, int source,
diff --git a/sound/soc/codecs/wm5102.c b/sound/soc/codecs/wm5102.c
index 0c6d1bc..b73e3a3 100644
--- a/sound/soc/codecs/wm5102.c
+++ b/sound/soc/codecs/wm5102.c
@@ -605,12 +605,13 @@ static int wm5102_sysclk_ev(struct snd_soc_dapm_widget *w,
regmap_write_async(regmap, patch[i].reg,
patch[i].def);
break;
-
- default:
+ case SND_SOC_DAPM_PRE_PMD:
break;
+ default:
+ return 0;
}
- return 0;
+ return arizona_dvfs_sysclk_ev(w, kcontrol, event);
}
static int wm5102_out_comp_coeff_get(struct snd_kcontrol *kcontrol,
@@ -1036,7 +1037,8 @@ static const struct snd_kcontrol_new wm5102_aec_loopback_mux =
static const struct snd_soc_dapm_widget wm5102_dapm_widgets[] = {
SND_SOC_DAPM_SUPPLY("SYSCLK", ARIZONA_SYSTEM_CLOCK_1, ARIZONA_SYSCLK_ENA_SHIFT,
- 0, wm5102_sysclk_ev, SND_SOC_DAPM_POST_PMU),
+ 0, wm5102_sysclk_ev,
+ SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
SND_SOC_DAPM_SUPPLY("ASYNCCLK", ARIZONA_ASYNC_CLOCK_1,
ARIZONA_ASYNC_CLK_ENA_SHIFT, 0, NULL, 0),
SND_SOC_DAPM_SUPPLY("OPCLK", ARIZONA_OUTPUT_SYSTEM_CLOCK,
@@ -1909,6 +1911,8 @@ static int wm5102_probe(struct platform_device *pdev)
wm5102->core.arizona = arizona;
wm5102->core.num_inputs = 6;
+ arizona_init_dvfs(&wm5102->core);
+
wm5102->core.adsp[0].part = "wm5102";
wm5102->core.adsp[0].num = 1;
wm5102->core.adsp[0].type = WMFW_ADSP2;
diff --git a/sound/soc/codecs/wm8997.c b/sound/soc/codecs/wm8997.c
index a4d1177..2a129dc 100644
--- a/sound/soc/codecs/wm8997.c
+++ b/sound/soc/codecs/wm8997.c
@@ -106,11 +106,13 @@ static int wm8997_sysclk_ev(struct snd_soc_dapm_widget *w,
regmap_write_async(regmap, patch[i].reg,
patch[i].def);
break;
- default:
+ case SND_SOC_DAPM_PRE_PMD:
break;
+ default:
+ return 0;
}
- return 0;
+ return arizona_dvfs_sysclk_ev(w, kcontrol, event);
}
static const char *wm8997_osr_text[] = {
@@ -409,7 +411,8 @@ static const struct snd_kcontrol_new wm8997_aec_loopback_mux =
static const struct snd_soc_dapm_widget wm8997_dapm_widgets[] = {
SND_SOC_DAPM_SUPPLY("SYSCLK", ARIZONA_SYSTEM_CLOCK_1, ARIZONA_SYSCLK_ENA_SHIFT,
- 0, wm8997_sysclk_ev, SND_SOC_DAPM_POST_PMU),
+ 0, wm8997_sysclk_ev,
+ SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
SND_SOC_DAPM_SUPPLY("ASYNCCLK", ARIZONA_ASYNC_CLOCK_1,
ARIZONA_ASYNC_CLK_ENA_SHIFT, 0, NULL, 0),
SND_SOC_DAPM_SUPPLY("OPCLK", ARIZONA_OUTPUT_SYSTEM_CLOCK,
@@ -1126,6 +1129,8 @@ static int wm8997_probe(struct platform_device *pdev)
wm8997->core.arizona = arizona;
wm8997->core.num_inputs = 4;
+ arizona_init_dvfs(&wm8997->core);
+
for (i = 0; i < ARRAY_SIZE(wm8997->fll); i++)
wm8997->fll[i].vco_mult = 1;
--
1.7.2.5
In theory the ADSP driver should not need to know anything
about the codec it is part of. But the WM5102 needs DVFS
control based on ADSP clocking speed. This was being handled
by bundling part of the knowledge of this into the ADSP driver.
This change moves this handling out of the ADSP driver and
into the WM5102 driver.
Signed-off-by: Richard Fitzgerald <[email protected]>
---
sound/soc/codecs/wm5102.c | 47 +++++++++++++++++++++++++++-
sound/soc/codecs/wm5110.c | 2 +-
sound/soc/codecs/wm_adsp.c | 73 +-------------------------------------------
sound/soc/codecs/wm_adsp.h | 15 ++++-----
4 files changed, 54 insertions(+), 83 deletions(-)
diff --git a/sound/soc/codecs/wm5102.c b/sound/soc/codecs/wm5102.c
index b73e3a3..11eba0e 100644
--- a/sound/soc/codecs/wm5102.c
+++ b/sound/soc/codecs/wm5102.c
@@ -614,6 +614,49 @@ static int wm5102_sysclk_ev(struct snd_soc_dapm_widget *w,
return arizona_dvfs_sysclk_ev(w, kcontrol, event);
}
+static int wm5102_adsp_power_ev(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event)
+{
+ struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+ struct arizona *arizona = dev_get_drvdata(codec->dev->parent);
+ unsigned int v;
+ int ret;
+
+ switch (event) {
+ case SND_SOC_DAPM_PRE_PMU:
+ ret = regmap_read(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, &v);
+ if (ret != 0) {
+ dev_err(codec->dev,
+ "Failed to read SYSCLK state: %d\n", ret);
+ return -EIO;
+ }
+
+ v = (v & ARIZONA_SYSCLK_FREQ_MASK) >> ARIZONA_SYSCLK_FREQ_SHIFT;
+
+ if (v >= 3) {
+ ret = arizona_dvfs_up(codec, ARIZONA_DVFS_ADSP1_RQ);
+ if (ret) {
+ dev_err(codec->dev,
+ "Failed to raise DVFS: %d\n", ret);
+ return ret;
+ }
+ }
+ break;
+
+ case SND_SOC_DAPM_POST_PMD:
+ ret = arizona_dvfs_down(codec, ARIZONA_DVFS_ADSP1_RQ);
+ if (ret)
+ dev_warn(codec->dev,
+ "Failed to lower DVFS: %d\n", ret);
+ break;
+
+ default:
+ break;
+ }
+
+ return wm_adsp2_early_event(w, kcontrol, event);
+}
+
static int wm5102_out_comp_coeff_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
@@ -1369,7 +1412,7 @@ ARIZONA_MUX_WIDGETS(ISRC2DEC2, "ISRC2DEC2"),
ARIZONA_MUX_WIDGETS(ISRC2INT1, "ISRC2INT1"),
ARIZONA_MUX_WIDGETS(ISRC2INT2, "ISRC2INT2"),
-WM_ADSP2("DSP1", 0),
+WM_ADSP2_E("DSP1", 0, wm5102_adsp_power_ev),
SND_SOC_DAPM_OUTPUT("HPOUT1L"),
SND_SOC_DAPM_OUTPUT("HPOUT1R"),
@@ -1922,7 +1965,7 @@ static int wm5102_probe(struct platform_device *pdev)
wm5102->core.adsp[0].mem = wm5102_dsp1_regions;
wm5102->core.adsp[0].num_mems = ARRAY_SIZE(wm5102_dsp1_regions);
- ret = wm_adsp2_init(&wm5102->core.adsp[0], true);
+ ret = wm_adsp2_init(&wm5102->core.adsp[0]);
if (ret != 0)
return ret;
diff --git a/sound/soc/codecs/wm5110.c b/sound/soc/codecs/wm5110.c
index fbaeddb..d65364e 100644
--- a/sound/soc/codecs/wm5110.c
+++ b/sound/soc/codecs/wm5110.c
@@ -1697,7 +1697,7 @@ static int wm5110_probe(struct platform_device *pdev)
wm5110->core.adsp[i].num_mems
= ARRAY_SIZE(wm5110_dsp1_regions);
- ret = wm_adsp2_init(&wm5110->core.adsp[i], false);
+ ret = wm_adsp2_init(&wm5110->core.adsp[i]);
if (ret != 0)
return ret;
}
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index f6642c1..6e358b5 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -1787,35 +1787,6 @@ static void wm_adsp2_boot_work(struct work_struct *work)
return;
}
- if (dsp->dvfs) {
- ret = regmap_read(dsp->regmap,
- dsp->base + ADSP2_CLOCKING, &val);
- if (ret != 0) {
- adsp_err(dsp, "Failed to read clocking: %d\n", ret);
- return;
- }
-
- if ((val & ADSP2_CLK_SEL_MASK) >= 3) {
- ret = regulator_enable(dsp->dvfs);
- if (ret != 0) {
- adsp_err(dsp,
- "Failed to enable supply: %d\n",
- ret);
- return;
- }
-
- ret = regulator_set_voltage(dsp->dvfs,
- 1800000,
- 1800000);
- if (ret != 0) {
- adsp_err(dsp,
- "Failed to raise supply: %d\n",
- ret);
- return;
- }
- }
- }
-
ret = wm_adsp2_ena(dsp);
if (ret != 0)
return;
@@ -1909,21 +1880,6 @@ int wm_adsp2_event(struct snd_soc_dapm_widget *w,
regmap_write(dsp->regmap, dsp->base + ADSP2_WDMA_CONFIG_2, 0);
regmap_write(dsp->regmap, dsp->base + ADSP2_RDMA_CONFIG_1, 0);
- if (dsp->dvfs) {
- ret = regulator_set_voltage(dsp->dvfs, 1200000,
- 1800000);
- if (ret != 0)
- adsp_warn(dsp,
- "Failed to lower supply: %d\n",
- ret);
-
- ret = regulator_disable(dsp->dvfs);
- if (ret != 0)
- adsp_err(dsp,
- "Failed to enable supply: %d\n",
- ret);
- }
-
list_for_each_entry(ctl, &dsp->ctl_list, list)
ctl->enabled = 0;
@@ -1950,7 +1906,7 @@ err:
}
EXPORT_SYMBOL_GPL(wm_adsp2_event);
-int wm_adsp2_init(struct wm_adsp *dsp, bool dvfs)
+int wm_adsp2_init(struct wm_adsp *dsp)
{
int ret;
@@ -1969,33 +1925,6 @@ int wm_adsp2_init(struct wm_adsp *dsp, bool dvfs)
INIT_LIST_HEAD(&dsp->ctl_list);
INIT_WORK(&dsp->boot_work, wm_adsp2_boot_work);
- if (dvfs) {
- dsp->dvfs = devm_regulator_get(dsp->dev, "DCVDD");
- if (IS_ERR(dsp->dvfs)) {
- ret = PTR_ERR(dsp->dvfs);
- adsp_err(dsp, "Failed to get DCVDD: %d\n", ret);
- return ret;
- }
-
- ret = regulator_enable(dsp->dvfs);
- if (ret != 0) {
- adsp_err(dsp, "Failed to enable DCVDD: %d\n", ret);
- return ret;
- }
-
- ret = regulator_set_voltage(dsp->dvfs, 1200000, 1800000);
- if (ret != 0) {
- adsp_err(dsp, "Failed to initialise DVFS: %d\n", ret);
- return ret;
- }
-
- ret = regulator_disable(dsp->dvfs);
- if (ret != 0) {
- adsp_err(dsp, "Failed to disable DCVDD: %d\n", ret);
- return ret;
- }
- }
-
return 0;
}
EXPORT_SYMBOL_GPL(wm_adsp2_init);
diff --git a/sound/soc/codecs/wm_adsp.h b/sound/soc/codecs/wm_adsp.h
index 4fe0667..0e5f07c 100644
--- a/sound/soc/codecs/wm_adsp.h
+++ b/sound/soc/codecs/wm_adsp.h
@@ -18,8 +18,6 @@
#include "wmfw.h"
-struct regulator;
-
struct wm_adsp_region {
int type;
unsigned int base;
@@ -56,8 +54,6 @@ struct wm_adsp {
int fw_ver;
bool running;
- struct regulator *dvfs;
-
struct list_head ctl_list;
struct work_struct boot_work;
@@ -67,19 +63,22 @@ struct wm_adsp {
SND_SOC_DAPM_PGA_E(wname, SND_SOC_NOPM, num, 0, NULL, 0, \
wm_adsp1_event, SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD)
-#define WM_ADSP2(wname, num) \
+#define WM_ADSP2_E(wname, num, event_fn) \
{ .id = snd_soc_dapm_dai_link, .name = wname " Preloader", \
- .reg = SND_SOC_NOPM, .shift = num, .event = wm_adsp2_early_event, \
- .event_flags = SND_SOC_DAPM_PRE_PMU }, \
+ .reg = SND_SOC_NOPM, .shift = num, .event = event_fn, \
+ .event_flags = SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD }, \
{ .id = snd_soc_dapm_out_drv, .name = wname, \
.reg = SND_SOC_NOPM, .shift = num, .event = wm_adsp2_event, \
.event_flags = SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD }
+#define WM_ADSP2(wname, num) \
+ WM_ADSP2_E(wname, num, wm_adsp2_early_event)
+
extern const struct snd_kcontrol_new wm_adsp1_fw_controls[];
extern const struct snd_kcontrol_new wm_adsp2_fw_controls[];
int wm_adsp1_init(struct wm_adsp *dsp);
-int wm_adsp2_init(struct wm_adsp *dsp, bool dvfs);
+int wm_adsp2_init(struct wm_adsp *dsp);
int wm_adsp1_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event);
int wm_adsp2_early_event(struct snd_soc_dapm_widget *w,
--
1.7.2.5
The WM8997 and WM5102 codecs need to boost DVFS for higher sample rates.
Signed-off-by: Richard Fitzgerald <[email protected]>
Signed-off-by: Charles Keepax <[email protected]>
---
sound/soc/codecs/arizona.c | 19 ++++++++++++++++++-
1 files changed, 18 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/arizona.c b/sound/soc/codecs/arizona.c
index 69965ae..f4f3022 100644
--- a/sound/soc/codecs/arizona.c
+++ b/sound/soc/codecs/arizona.c
@@ -1393,7 +1393,7 @@ static int arizona_hw_params_rate(struct snd_pcm_substream *substream,
struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
struct arizona_dai_priv *dai_priv = &priv->dai[dai->id - 1];
int base = dai->driver->base;
- int i, sr_val;
+ int i, sr_val, ret;
/*
* We will need to be more flexible than this in future,
@@ -1409,6 +1409,23 @@ static int arizona_hw_params_rate(struct snd_pcm_substream *substream,
}
sr_val = i;
+ switch (priv->arizona->type) {
+ case WM5102:
+ case WM8997:
+ if (arizona_sr_vals[sr_val] >= 88200)
+ ret = arizona_dvfs_up(codec, ARIZONA_DVFS_SR1_RQ);
+ else
+ ret = arizona_dvfs_down(codec, ARIZONA_DVFS_SR1_RQ);
+
+ if (ret) {
+ arizona_aif_err(dai, "Failed to change DVFS %d\n", ret);
+ return ret;
+ }
+ break;
+ default:
+ break;
+ }
+
switch (dai_priv->clk) {
case ARIZONA_CLK_SYSCLK:
switch (priv->arizona->type) {
--
1.7.2.5
Using the driver for the internal regulator to also enable/disable
the codec internal clock frequency controller is an unexpected
side-effect for a regulator, and also means that the core clocks
won't be changed as expected if an external regulator is used to
power the codec.
The DVFS is now handled by the codec driver so can be removed from
the LDO1 driver.
Signed-off-by: Richard Fitzgerald <[email protected]>
---
drivers/regulator/arizona-ldo1.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c
index 9094163..5e947a8 100644
--- a/drivers/regulator/arizona-ldo1.c
+++ b/drivers/regulator/arizona-ldo1.c
@@ -78,11 +78,6 @@ static int arizona_ldo1_hc_set_voltage_sel(struct regulator_dev *rdev,
if (ret != 0)
return ret;
- ret = regmap_update_bits(regmap, ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
- ARIZONA_SUBSYS_MAX_FREQ, val);
- if (ret != 0)
- return ret;
-
if (val)
return 0;
--
1.7.2.5
On Mon, Jun 01, 2015 at 02:04:48PM +0100, Richard Fitzgerald wrote:
> +int arizona_dvfs_down(struct snd_soc_codec *codec, unsigned int flags)
> +{
> + struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
> + unsigned int old_reqs = priv->dvfs_reqs;
> + int ret = 0;
> +
> + mutex_lock(&priv->dvfs_lock);
> +
> + priv->dvfs_reqs &= ~flags;
> +
> + if (!priv->dvfs_cached && old_reqs && !priv->dvfs_reqs)
> + ret = arizona_dvfs_disable(codec);
What is the lock intended to protect here? We read old_reqs outside the
lock so it's possible that dvfs_reqs could change between us reading
old_reqs and the locked section - I would have expected to see all the
reads and updates to be in the locked section but perhaps it doesn't
protect what I think it protects (all the DVFS-related variables).
> + case SND_SOC_DAPM_PRE_PMD:
> + /* We must ensure DVFS is disabled before the codec goes into
> + * suspend so that we are never in an illegal state of DVFS
> + * enabled without enough DCVDD
> + */
> + priv->dvfs_cached = true;
> +
> + if (priv->dvfs_reqs)
> + ret = arizona_dvfs_disable(codec);
Are you sure that the function shouldn't check for requests? It seems
like every caller is repeating the same check.
On Mon, Jun 01, 2015 at 05:10:47PM +0100, Mark Brown wrote:
> On Mon, Jun 01, 2015 at 02:04:48PM +0100, Richard Fitzgerald wrote:
>
> > +int arizona_dvfs_down(struct snd_soc_codec *codec, unsigned int flags)
> > +{
> > + struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
> > + unsigned int old_reqs = priv->dvfs_reqs;
> > + int ret = 0;
> > +
> > + mutex_lock(&priv->dvfs_lock);
> > +
> > + priv->dvfs_reqs &= ~flags;
> > +
> > + if (!priv->dvfs_cached && old_reqs && !priv->dvfs_reqs)
> > + ret = arizona_dvfs_disable(codec);
>
> What is the lock intended to protect here? We read old_reqs outside the
> lock so it's possible that dvfs_reqs could change between us reading
> old_reqs and the locked section - I would have expected to see all the
> reads and updates to be in the locked section but perhaps it doesn't
> protect what I think it protects (all the DVFS-related variables).
>
Damn, I didn't notice that assignment when I added the mutex lock.
> > + case SND_SOC_DAPM_PRE_PMD:
> > + /* We must ensure DVFS is disabled before the codec goes into
> > + * suspend so that we are never in an illegal state of DVFS
> > + * enabled without enough DCVDD
> > + */
> > + priv->dvfs_cached = true;
> > +
> > + if (priv->dvfs_reqs)
> > + ret = arizona_dvfs_disable(codec);
>
> Are you sure that the function shouldn't check for requests? It seems
> like every caller is repeating the same check.
Sorry, I don't understand your comment here. When SYSCLK is disabled
we need to disable DVFS so that the codec can't go into suspend in the
illegal state of DVFS enabled but insifficient DCVDD. However, if
dvfs_reqs==0 DVFS is already disabled so we can save some time.
On Mon, Jun 01, 2015 at 05:22:51PM +0100, Richard Fitzgerald wrote:
> On Mon, Jun 01, 2015 at 05:10:47PM +0100, Mark Brown wrote:
> > > + if (priv->dvfs_reqs)
> > > + ret = arizona_dvfs_disable(codec);
> > Are you sure that the function shouldn't check for requests? It seems
> > like every caller is repeating the same check.
> Sorry, I don't understand your comment here. When SYSCLK is disabled
> we need to disable DVFS so that the codec can't go into suspend in the
> illegal state of DVFS enabled but insifficient DCVDD. However, if
> dvfs_reqs==0 DVFS is already disabled so we can save some time.
Sure, but you repeat the same tests for dvfs_reqs at every call site
which suggests it should be factored into the functions.
On Mon, Jun 01, 2015 at 05:36:04PM +0100, Mark Brown wrote:
> On Mon, Jun 01, 2015 at 05:22:51PM +0100, Richard Fitzgerald wrote:
> > On Mon, Jun 01, 2015 at 05:10:47PM +0100, Mark Brown wrote:
>
> > > > + if (priv->dvfs_reqs)
> > > > + ret = arizona_dvfs_disable(codec);
>
> > > Are you sure that the function shouldn't check for requests? It seems
> > > like every caller is repeating the same check.
>
> > Sorry, I don't understand your comment here. When SYSCLK is disabled
> > we need to disable DVFS so that the codec can't go into suspend in the
> > illegal state of DVFS enabled but insifficient DCVDD. However, if
> > dvfs_reqs==0 DVFS is already disabled so we can save some time.
>
> Sure, but you repeat the same tests for dvfs_reqs at every call site
> which suggests it should be factored into the functions.
I don't. There are two cases that call them if (!dvfs_reqs) and two
that call them if (dvfs_reqs)