[PATCH 0/4 v2] arizona: Improvements to codec DVFS control

[PATCH 0/4 v2] arizona: Improvements to codec DVFS control

[Charles Keepax]

Hi,

Respin of Richard's DVFS series, from me since he is on
holiday. I have based these on Mark's ASoC for-next branch
as there a few changes in there to the same parts of code.
But let me know if you would rather have a series based on
Lee's tree instead.

This set of changes cleans up the way the drivers handle
the control of internal subsystem clocks for codecs with
DVFS. This previously relied on the clock control happening
as a side-effect of changing the regulator voltage, rather
than explicitly, which assumed that you were using the
built-in regulator. Also DVFS control was being done from
the ADSP driver despite that driver supposedly being codec-
agnostic.
There are other cases on some codecs where DVFS control is
needed so this sequence of patches cleans up the way DVFS
is implemented and adds the extra case of DVFS boost for
higher sample rates on WM8997 and WM5102


Changes since v1:
 - Changed mask variable to flags to be more consistent
   with its usage
 - Fixed minor tabbing issue
 - Adding missing SND_SOC_DAPM_PRE_PMD event flag in WM_ADSP2_E


Richard Fitzgerald (4):
  mfd: arizona: Export function to control subsystem DVFS
  ASoC: wm_adsp: Move DVFS control into codec driver
  ASoC: arizona: Add DVFS handling for sample rate control
  regulator: arizona-ldo1: Do not control clocking from regulator

 drivers/mfd/arizona-core.c       |   84 ++++++++++++++++++++++++++++++++++++++
 drivers/regulator/arizona-ldo1.c |    5 --
 include/linux/mfd/arizona/core.h |   12 +++++
 sound/soc/codecs/arizona.c       |   22 +++++++++-
 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       |   13 +++---
 8 files changed, 171 insertions(+), 87 deletions(-)

-- 
1.7.2.5

[PATCH 1/4 v2] mfd: arizona: Export function to control subsystem DVFS

[Charles Keepax]

From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>

Moving this control from being a side-effect of the LDO1
regulator driver to a specific exported function.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c       |   84 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/arizona/core.h |   12 +++++
 2 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index cfc191a..f03a69b 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -94,6 +94,89 @@ int arizona_clk32k_disable(struct arizona *arizona)
 }
 EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
 
+int arizona_dvfs_up(struct arizona *arizona, unsigned int flags)
+{
+	unsigned int new_flags;
+	int ret = 0;
+
+	mutex_lock(&arizona->subsys_max_lock);
+
+	new_flags = arizona->subsys_max_rq | flags;
+
+	if (arizona->subsys_max_rq != new_flags) {
+		switch (arizona->type) {
+		case WM5102:
+		case WM8997:
+			ret = regulator_set_voltage(arizona->dcvdd,
+						    1800000, 1800000);
+			if (ret != 0) {
+				dev_err(arizona->dev,
+					"Failed to raise dcvdd (%u)\n", ret);
+				goto err;
+			}
+
+			ret = regmap_update_bits(arizona->regmap,
+					ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
+					ARIZONA_SUBSYS_MAX_FREQ, 1);
+			if (ret != 0) {
+				dev_err(arizona->dev,
+					"Failed to enable subsys max (%u)\n",
+					ret);
+				regulator_set_voltage(arizona->dcvdd,
+						      1200000, 1800000);
+				goto err;
+			}
+			break;
+
+		default:
+			break;
+		}
+
+		arizona->subsys_max_rq = new_flags;
+	}
+err:
+	mutex_unlock(&arizona->subsys_max_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(arizona_dvfs_up);
+
+int arizona_dvfs_down(struct arizona *arizona, unsigned int flags)
+{
+	int ret = 0;
+
+	mutex_lock(&arizona->subsys_max_lock);
+
+	arizona->subsys_max_rq &= ~flags;
+
+	if (arizona->subsys_max_rq == 0) {
+		switch (arizona->type) {
+		case WM5102:
+		case WM8997:
+			ret = regmap_update_bits(arizona->regmap,
+					ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
+					ARIZONA_SUBSYS_MAX_FREQ, 0);
+			if (ret != 0)
+				dev_err(arizona->dev,
+					"Failed to disable subsys max (%u)\n",
+					ret);
+
+			ret = regulator_set_voltage(arizona->dcvdd,
+						    1200000, 1800000);
+			if (ret != 0)
+				dev_err(arizona->dev,
+					"Failed to lower dcvdd (%u)\n", ret);
+			break;
+
+		default:
+			break;
+		}
+	}
+
+	mutex_unlock(&arizona->subsys_max_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(arizona_dvfs_down);
+
 static irqreturn_t arizona_clkgen_err(int irq, void *data)
 {
 	struct arizona *arizona = data;
@@ -641,6 +724,7 @@ int arizona_dev_init(struct arizona *arizona)
 
 	dev_set_drvdata(arizona->dev, arizona);
 	mutex_init(&arizona->clk_lock);
+	mutex_init(&arizona->subsys_max_lock);
 
 	if (dev_get_platdata(arizona->dev))
 		memcpy(&arizona->pdata, dev_get_platdata(arizona->dev),
diff --git a/include/linux/mfd/arizona/core.h b/include/linux/mfd/arizona/core.h
index a614b33..4aec6a3 100644
--- a/include/linux/mfd/arizona/core.h
+++ b/include/linux/mfd/arizona/core.h
@@ -109,6 +109,9 @@ struct arizona {
 	struct mutex clk_lock;
 	int clk32k_ref;
 
+	struct mutex subsys_max_lock;
+	unsigned int subsys_max_rq;
+
 	struct snd_soc_dapm_context *dapm;
 
 	int tdm_width[ARIZONA_MAX_AIF];
@@ -118,8 +121,17 @@ struct arizona {
 	uint8_t dac_comp_enabled;
 };
 
+#define ARIZONA_DVFS_SR1_RQ          0x00000001
+#define ARIZONA_DVFS_SR2_RQ          0x00000002
+#define ARIZONA_DVFS_SR3_RQ          0x00000004
+#define ARIZONA_DVFS_ASR1_RQ         0x00000010
+#define ARIZONA_DVFS_ASR2_RQ         0x00000020
+#define ARIZONA_DVFS_ADSP1_RQ        0x00010000
+
 int arizona_clk32k_enable(struct arizona *arizona);
 int arizona_clk32k_disable(struct arizona *arizona);
+int arizona_dvfs_up(struct arizona *arizona, unsigned int mask);
+int arizona_dvfs_down(struct arizona *arizona, unsigned int mask);
 
 int arizona_request_irq(struct arizona *arizona, int irq, char *name,
 			irq_handler_t handler, void *data);
-- 
1.7.2.5

[PATCH 2/4 v2] ASoC: wm_adsp: Move DVFS control into codec driver

[Charles Keepax]

From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>

In theory the ADSP driver should not need to know
anything about the codec it is part of. But some codecs
need 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 removes this handling out of the ADSP driver.
A new macro WM_ADSP2_E() takes a callback function to be
called by the preloader widget in place of the default
handler, and this can be used to do codec-specific power
control.

The WM5102 driver has been updated to implement the DVFS.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 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 |   13 ++++---
 4 files changed, 54 insertions(+), 81 deletions(-)

diff --git a/sound/soc/codecs/wm5102.c b/sound/soc/codecs/wm5102.c
index fa24d55..b938f3f 100644
--- a/sound/soc/codecs/wm5102.c
+++ b/sound/soc/codecs/wm5102.c
@@ -612,6 +612,49 @@ static int wm5102_sysclk_ev(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int wm5102_adsp_power_ev(struct snd_soc_dapm_widget *w,
+		   struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = w->codec;
+	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(arizona, ARIZONA_DVFS_ADSP1_RQ);
+			if (ret != 0) {
+				dev_err(codec->dev,
+					"Failed to raise DVFS: %d\n", ret);
+				return ret;
+			}
+		}
+		break;
+
+	case SND_SOC_DAPM_PRE_PMD:
+		ret = arizona_dvfs_down(arizona, ARIZONA_DVFS_ADSP1_RQ);
+		if (ret != 0)
+			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)
 {
@@ -1362,7 +1405,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"),
@@ -1909,7 +1952,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 2e5fcb5..aec7481 100644
--- a/sound/soc/codecs/wm5110.c
+++ b/sound/soc/codecs/wm5110.c
@@ -1687,7 +1687,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 0600271..cdf98bf 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -1539,35 +1539,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;
@@ -1668,21 +1639,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;
 
@@ -1709,7 +1665,7 @@ err:
 }
 EXPORT_SYMBOL_GPL(wm_adsp2_event);
 
-int wm_adsp2_init(struct wm_adsp *adsp, bool dvfs)
+int wm_adsp2_init(struct wm_adsp *adsp)
 {
 	int ret;
 
@@ -1728,33 +1684,6 @@ int wm_adsp2_init(struct wm_adsp *adsp, bool dvfs)
 	INIT_LIST_HEAD(&adsp->ctl_list);
 	INIT_WORK(&adsp->boot_work, wm_adsp2_boot_work);
 
-	if (dvfs) {
-		adsp->dvfs = devm_regulator_get(adsp->dev, "DCVDD");
-		if (IS_ERR(adsp->dvfs)) {
-			ret = PTR_ERR(adsp->dvfs);
-			adsp_err(adsp, "Failed to get DCVDD: %d\n", ret);
-			return ret;
-		}
-
-		ret = regulator_enable(adsp->dvfs);
-		if (ret != 0) {
-			adsp_err(adsp, "Failed to enable DCVDD: %d\n", ret);
-			return ret;
-		}
-
-		ret = regulator_set_voltage(adsp->dvfs, 1200000, 1800000);
-		if (ret != 0) {
-			adsp_err(adsp, "Failed to initialise DVFS: %d\n", ret);
-			return ret;
-		}
-
-		ret = regulator_disable(adsp->dvfs);
-		if (ret != 0) {
-			adsp_err(adsp, "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 a4f6b64..e273db76 100644
--- a/sound/soc/codecs/wm_adsp.h
+++ b/sound/soc/codecs/wm_adsp.h
@@ -56,8 +56,6 @@ struct wm_adsp {
 	int fw;
 	bool running;
 
-	struct regulator *dvfs;
-
 	struct list_head ctl_list;
 
 	struct work_struct boot_work;
@@ -67,19 +65,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_PRE_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 *adsp);
-int wm_adsp2_init(struct wm_adsp *adsp, bool dvfs);
+int wm_adsp2_init(struct wm_adsp *adsp);
 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

[PATCH 3/4 v2] ASoC: arizona: Add DVFS handling for sample rate control

[Charles Keepax]

From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>

Some codecs need to boost DVFS for higher sample rates.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/arizona.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/arizona.c b/sound/soc/codecs/arizona.c
index 41b56ee..0713b10 100644
--- a/sound/soc/codecs/arizona.c
+++ b/sound/soc/codecs/arizona.c
@@ -1160,7 +1160,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,
@@ -1176,6 +1176,26 @@ 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(priv->arizona,
+					      ARIZONA_DVFS_SR1_RQ);
+		else
+			ret = arizona_dvfs_down(priv->arizona,
+						ARIZONA_DVFS_SR1_RQ);
+
+		if (ret != 0) {
+			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

[PATCH 4/4 v2] regulator: arizona-ldo1: Do not control clocking from regulator

[Charles Keepax]

From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>

Using the driver for the internal regulator to also control
the clock frequency of blocks inside the codec 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 clocking control is now handled by the core arizona MFD
driver so can be removed from the LDO1 driver.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 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 04f262a..47b48c8 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

Re: [PATCH 1/4 v2] mfd: arizona: Export function to control subsystem DVFS

[Mark Brown]

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

On Fri, Jun 20, 2014 at 03:41:10PM +0100, Charles Keepax wrote:

> +	int ret = 0;

> +			ret = regulator_set_voltage(arizona->dcvdd,
> +						    1800000, 1800000);
> +			if (ret != 0) {
> +				dev_err(arizona->dev,
> +					"Failed to raise dcvdd (%u)\n", ret);
> +				goto err;
> +			}

I'm really nitpicking here but the error code is a signed integer being
printed for some reason with %u and while I'm at it DCVDD would normally
be written in all caps for human consumption.

> +int arizona_dvfs_down(struct arizona *arizona, unsigned int flags)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&arizona->subsys_max_lock);
> +
> +	arizona->subsys_max_rq &= ~flags;

Is it worth checking to see if the request was asserted and logging a
warning if it wasn't?  The lack of refcounting suggests that a bit of
defensiveness might be in order.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH 2/4 v2] ASoC: wm_adsp: Move DVFS control into codec driver

[Mark Brown]

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

On Fri, Jun 20, 2014 at 03:41:11PM +0100, Charles Keepax wrote:

> +	case SND_SOC_DAPM_PRE_PMD:
> +		ret = arizona_dvfs_down(arizona, ARIZONA_DVFS_ADSP1_RQ);
> +		if (ret != 0)
> +			dev_warn(codec->dev,
> +				 "Failed to lower DVFS: %d\n", ret);
> +		break;

I would expect this to be _POST_PMD since it happens...

> +	default:
> +		break;
> +	}
> +
> +	return wm_adsp2_early_event(w, kcontrol, event);

...before we even give the ADSP event a chance to run meaning the
voltage will be ramped down prior to halting the DSP.  If there are any
connected outputs still live this might result in audible issues as the
device goes out of spec briefly prior to being halted and there is some
remote chance that it could cause problems on next power up I guess (it
did go out of spec after all).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH 3/4 v2] ASoC: arizona: Add DVFS handling for sample rate control

[Mark Brown]

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

On Fri, Jun 20, 2014 at 03:41:12PM +0100, Charles Keepax wrote:
> From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> 
> Some codecs need to boost DVFS for higher sample rates.

One tiny nit:

> +		if (ret != 0) {
> +			arizona_aif_err(dai, "Failed to change DVFS %d\n", ret);
> +			return ret;

Normal style would be "Failed to change DVFS: %d\n".

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH 4/4 v2] regulator: arizona-ldo1: Do not control clocking from regulator

[Mark Brown]

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

On Fri, Jun 20, 2014 at 03:41:13PM +0100, Charles Keepax wrote:
> From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> 
> Using the driver for the internal regulator to also control
> the clock frequency of blocks inside the codec 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 clocking control is now handled by the core arizona MFD
> driver so can be removed from the LDO1 driver.

I would expect this to be squashed into the first patch since otherwise
there's a bisection issue with both drivers trying to maintain the same
thing at the same time.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH 1/4 v2] mfd: arizona: Export function to control subsystem DVFS

[Charles Keepax]

On Sat, Jun 21, 2014 at 09:45:49PM +0100, Mark Brown wrote:
> On Fri, Jun 20, 2014 at 03:41:10PM +0100, Charles Keepax wrote:
> 
> > +	int ret = 0;
> 
> > +			ret = regulator_set_voltage(arizona->dcvdd,
> > +						    1800000, 1800000);
> > +			if (ret != 0) {
> > +				dev_err(arizona->dev,
> > +					"Failed to raise dcvdd (%u)\n", ret);
> > +				goto err;
> > +			}
> 
> I'm really nitpicking here but the error code is a signed integer being
> printed for some reason with %u and while I'm at it DCVDD would normally
> be written in all caps for human consumption.

Yeah that should be fixed.

> 
> > +int arizona_dvfs_down(struct arizona *arizona, unsigned int flags)
> > +{
> > +	int ret = 0;
> > +
> > +	mutex_lock(&arizona->subsys_max_lock);
> > +
> > +	arizona->subsys_max_rq &= ~flags;
> 
> Is it worth checking to see if the request was asserted and logging a
> warning if it wasn't?  The lack of refcounting suggests that a bit of
> defensiveness might be in order.

I think it seems reasonable, I will add some code for this.

Thanks,
Charles

Re: [PATCH 2/4 v2] ASoC: wm_adsp: Move DVFS control into codec driver

[Charles Keepax]

On Sat, Jun 21, 2014 at 09:50:37PM +0100, Mark Brown wrote:
> On Fri, Jun 20, 2014 at 03:41:11PM +0100, Charles Keepax wrote:
> 
> > +	case SND_SOC_DAPM_PRE_PMD:
> > +		ret = arizona_dvfs_down(arizona, ARIZONA_DVFS_ADSP1_RQ);
> > +		if (ret != 0)
> > +			dev_warn(codec->dev,
> > +				 "Failed to lower DVFS: %d\n", ret);
> > +		break;
> 
> I would expect this to be _POST_PMD since it happens...

It might be a little more logical that way.

> 
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return wm_adsp2_early_event(w, kcontrol, event);
> 
> ...before we even give the ADSP event a chance to run meaning the
> voltage will be ramped down prior to halting the DSP.  If there are any
> connected outputs still live this might result in audible issues as the
> device goes out of spec briefly prior to being halted and there is some
> remote chance that it could cause problems on next power up I guess (it
> did go out of spec after all).

Although this isn't actually an issue as this is the preloader
widget we added for the firmware, which is a dai_link widget so
the DSP will have been shutdown by the main widget before this is
ever called. That said _POST_PMD makes the intention much more
clear so worth changing too.

Thanks,
Charles

Re: [PATCH 4/4 v2] regulator: arizona-ldo1: Do not control clocking from regulator

[Charles Keepax]

On Sat, Jun 21, 2014 at 09:53:49PM +0100, Mark Brown wrote:
> On Fri, Jun 20, 2014 at 03:41:13PM +0100, Charles Keepax wrote:
> > From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> > 
> > Using the driver for the internal regulator to also control
> > the clock frequency of blocks inside the codec 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 clocking control is now handled by the core arizona MFD
> > driver so can be removed from the LDO1 driver.
> 
> I would expect this to be squashed into the first patch since otherwise
> there's a bisection issue with both drivers trying to maintain the same
> thing at the same time.

Whilst there are two things controlling it for a couple of
patches they both set the same value one after another, so it
should be safe. If it goes into the first patch nothing is
controlling the DVFS for a few patches so that actually creates a
bisection issue. It could be squashed into the second last patch
but this way feels cleaner?

Thanks,
Charles

Re: [PATCH 4/4 v2] regulator: arizona-ldo1: Do not control clocking from regulator

[Charles Keepax]

On Mon, Jun 23, 2014 at 04:53:37PM +0100, Charles Keepax wrote:
> On Sat, Jun 21, 2014 at 09:53:49PM +0100, Mark Brown wrote:
> > On Fri, Jun 20, 2014 at 03:41:13PM +0100, Charles Keepax wrote:
> > > From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> > > 
> > > Using the driver for the internal regulator to also control
> > > the clock frequency of blocks inside the codec 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 clocking control is now handled by the core arizona MFD
> > > driver so can be removed from the LDO1 driver.
> > 
> > I would expect this to be squashed into the first patch since otherwise
> > there's a bisection issue with both drivers trying to maintain the same
> > thing at the same time.
> 
> Whilst there are two things controlling it for a couple of
> patches they both set the same value one after another, so it
> should be safe. If it goes into the first patch nothing is
> controlling the DVFS for a few patches so that actually creates a
> bisection issue. It could be squashed into the second last patch
> but this way feels cleaner?

Although looking at that again it can happily merge into the
second patch in the series so I will do that.

Thanks,
Charles

Re: [PATCH 4/4 v2] regulator: arizona-ldo1: Do not control clocking from regulator

[Mark Brown]

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

On Mon, Jun 23, 2014 at 05:29:22PM +0100, Charles Keepax wrote:
> On Mon, Jun 23, 2014 at 04:53:37PM +0100, Charles Keepax wrote:

> > Whilst there are two things controlling it for a couple of
> > patches they both set the same value one after another, so it
> > should be safe. If it goes into the first patch nothing is
> > controlling the DVFS for a few patches so that actually creates a
> > bisection issue. It could be squashed into the second last patch
> > but this way feels cleaner?

> Although looking at that again it can happily merge into the
> second patch in the series so I will do that.

OK, or if you do want to leave them separate I'd suggest noting why it's
safe in the commit log.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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