[PATCH v6 0/1] mfd: arizona: Export functions to control subsystem DVFS

[PATCH v6 0/1] mfd: arizona: Export functions to control subsystem DVFS

[Richard Fitzgerald]

V4 of this patch was acked by Lee but this new version is significantly
different from V4 so I've removed the ack.
Main changes since V4 are around better control of the DVFS across
suspend/resume, and added handling for the WM8998 and WM1814 codecs.

Richard Fitzgerald (1):
  mfd: arizona: Export functions to control subsystem DVFS

 drivers/mfd/arizona-core.c       |  188 +++++++++++++++++++++++++++++++++++++-
 include/linux/mfd/arizona/core.h |   13 +++
 2 files changed, 198 insertions(+), 3 deletions(-)

-- 
1.7.2.5

[PATCH v6 1/1] mfd: arizona: Export functions to control subsystem DVFS

[Richard Fitzgerald]

The WM5102, WM8997, WM8998 and WM1814 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 <rf@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c       |  188 +++++++++++++++++++++++++++++++++++++-
 include/linux/mfd/arizona/core.h |   13 +++
 2 files changed, 198 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 16828cc..2679d33 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -94,6 +94,122 @@ int arizona_clk32k_disable(struct arizona *arizona)
 }
 EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
 
+static int arizona_dvfs_apply_boost(struct arizona *arizona)
+{
+	int ret;
+
+	ret = regulator_set_voltage(arizona->dcvdd, 1800000, 1800000);
+	if (ret != 0) {
+		dev_err(arizona->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, 1);
+	if (ret != 0) {
+		dev_err(arizona->dev,
+			"Failed to enable subsys max: %d\n", ret);
+
+		regulator_set_voltage(arizona->dcvdd, 1200000, 1800000);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int arizona_dvfs_remove_boost(struct arizona *arizona)
+{
+	int ret;
+
+	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: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_set_voltage(arizona->dcvdd, 1200000, 1800000);
+	if (ret != 0) {
+		dev_err(arizona->dev,
+			"Failed to unboost DCVDD : %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+int arizona_dvfs_up(struct arizona *arizona, unsigned int flags)
+{
+	unsigned int old_flags;
+	int ret = 0;
+
+	switch (arizona->type) {
+	case WM5102:
+	case WM8997:
+	case WM8998:
+	case WM1814:
+		break;
+	default:
+		return 0;
+	}
+
+	mutex_lock(&arizona->subsys_max_lock);
+
+	old_flags = arizona->subsys_max_rq;
+	arizona->subsys_max_rq |= flags;
+
+	/* If currently caching the change will be applied in runtime resume */
+	if (arizona->subsys_max_cached) {
+		dev_dbg(arizona->dev, "subsys_max_cached (dvfs up)\n");
+		goto out;
+	}
+
+	if ((old_flags == 0) && (arizona->subsys_max_rq != 0))
+		ret = arizona_dvfs_apply_boost(arizona);
+out:
+	mutex_unlock(&arizona->subsys_max_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(arizona_dvfs_up);
+
+int arizona_dvfs_down(struct arizona *arizona, unsigned int flags)
+{
+	unsigned int old_flags;
+	int ret = 0;
+
+	switch (arizona->type) {
+	case WM5102:
+	case WM8997:
+	case WM8998:
+	case WM1814:
+		break;
+	default:
+		return 0;
+	}
+
+	mutex_lock(&arizona->subsys_max_lock);
+
+	old_flags = arizona->subsys_max_rq;
+	arizona->subsys_max_rq &= ~flags;
+
+	/* If currently caching the change will be applied in runtime resume */
+	if (arizona->subsys_max_cached) {
+		dev_dbg(arizona->dev, "subsys_max_cached (dvfs down)\n");
+		goto out;
+	}
+
+	if ((old_flags != 0) && (arizona->subsys_max_rq == 0))
+		ret = arizona_dvfs_remove_boost(arizona);
+out:
+	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;
@@ -462,6 +578,54 @@ static int wm5102_clear_write_sequencer(struct arizona *arizona)
 }
 
 #ifdef CONFIG_PM
+static int arizona_restore_dvfs(struct arizona *arizona)
+{
+	int ret;
+
+	switch (arizona->type) {
+	case WM5102:
+	case WM8997:
+	case WM8998:
+	case WM1814:
+		ret = 0;
+		mutex_lock(&arizona->subsys_max_lock);
+		if (arizona->subsys_max_rq != 0) {
+			dev_dbg(arizona->dev, "Restore subsys_max boost\n");
+			ret = arizona_dvfs_apply_boost(arizona);
+		}
+
+		if (ret == 0)
+			arizona->subsys_max_cached = false;
+
+		mutex_unlock(&arizona->subsys_max_lock);
+		return ret;
+	default:
+		return 0;	/* no DVFS */
+	}
+}
+
+static int arizona_suspend_dvfs(struct arizona *arizona)
+{
+	int ret;
+
+	switch (arizona->type) {
+	case WM5102:
+	case WM8997:
+	case WM8998:
+	case WM1814:
+		mutex_lock(&arizona->subsys_max_lock);
+		ret = arizona_dvfs_remove_boost(arizona);
+
+		if (ret == 0)
+			arizona->subsys_max_cached = true;
+
+		mutex_unlock(&arizona->subsys_max_lock);
+		break;
+	default:
+		return 0;
+	}
+}
+
 static int arizona_runtime_resume(struct device *dev)
 {
 	struct arizona *arizona = dev_get_drvdata(dev);
@@ -590,6 +754,12 @@ static int arizona_runtime_resume(struct device *dev)
 		goto err;
 	}
 
+	ret = arizona_restore_dvfs(arizona);
+	if (ret) {
+		dev_err(arizona->dev, "Failed to restore DVFS: %d\n", ret);
+		goto err;
+	}
+
 	return 0;
 
 err:
@@ -612,6 +782,13 @@ static int arizona_runtime_suspend(struct device *dev)
 		return ret;
 	}
 
+	/* Must disable DVFS boost before powering down DCVDD */
+	ret = arizona_suspend_dvfs(arizona);
+	if (ret) {
+		dev_err(dev, "Failed to suspend DVFS: %d\n", ret);
+		return ret;
+	}
+
 	if (arizona->external_dcvdd) {
 		ret = regmap_update_bits(arizona->regmap,
 					 ARIZONA_ISOLATION_CONTROL,
@@ -620,7 +797,7 @@ static int arizona_runtime_suspend(struct device *dev)
 		if (ret != 0) {
 			dev_err(arizona->dev, "Failed to isolate DCVDD: %d\n",
 				ret);
-			return ret;
+			goto err;
 		}
 	}
 
@@ -639,7 +816,7 @@ static int arizona_runtime_suspend(struct device *dev)
 		if (ret < 0) {
 			dev_err(arizona->dev,
 				"Failed to set suspend voltage: %d\n", ret);
-			return ret;
+			goto err;
 		}
 		break;
 	case WM5102:
@@ -650,7 +827,7 @@ static int arizona_runtime_suspend(struct device *dev)
 				dev_err(arizona->dev,
 					"Failed to clear write sequencer: %d\n",
 					ret);
-				return ret;
+				goto err;
 			}
 		}
 		break;
@@ -675,6 +852,10 @@ static int arizona_runtime_suspend(struct device *dev)
 	}
 
 	return 0;
+
+err:
+	arizona_restore_dvfs(arizona);
+	return ret;
 }
 #endif
 
@@ -933,6 +1114,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 b34a625..f18e4ca 100644
--- a/include/linux/mfd/arizona/core.h
+++ b/include/linux/mfd/arizona/core.h
@@ -136,6 +136,10 @@ struct arizona {
 	struct mutex clk_lock;
 	int clk32k_ref;
 
+	struct mutex subsys_max_lock;
+	unsigned int subsys_max_rq;
+	bool subsys_max_cached;
+
 	bool ctrlif_error;
 
 	struct snd_soc_dapm_context *dapm;
@@ -148,8 +152,17 @@ struct arizona {
 	struct mutex dac_comp_lock;
 };
 
+#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

Re: [PATCH v6 1/1] mfd: arizona: Export functions to control subsystem DVFS

[Charles Keepax]

On Tue, May 19, 2015 at 03:57:42PM +0100, Richard Fitzgerald wrote:
> The WM5102, WM8997, WM8998 and WM1814 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 <rf@opensource.wolfsonmicro.com>
> ---

Reviewed-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Thanks,
Charles

Re: [PATCH v6 1/1] mfd: arizona: Export functions to control subsystem DVFS

[Lee Jones]

On Tue, 19 May 2015, Richard Fitzgerald wrote:

> The WM5102, WM8997, WM8998 and WM1814 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.

The Arizona MFD has far too much knowledge for my liking.  It's
starting to look like a dumping ground for functionality you're
struggling to find a suitable home for.

Shouldn't this DVFS dump live in drivers/cpufreq?

> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c       |  188 +++++++++++++++++++++++++++++++++++++-
>  include/linux/mfd/arizona/core.h |   13 +++
>  2 files changed, 198 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 16828cc..2679d33 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -94,6 +94,122 @@ int arizona_clk32k_disable(struct arizona *arizona)
>  }
>  EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
>  
> +static int arizona_dvfs_apply_boost(struct arizona *arizona)
> +{
> +	int ret;
> +
> +	ret = regulator_set_voltage(arizona->dcvdd, 1800000, 1800000);
> +	if (ret != 0) {
> +		dev_err(arizona->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, 1);
> +	if (ret != 0) {
> +		dev_err(arizona->dev,
> +			"Failed to enable subsys max: %d\n", ret);
> +
> +		regulator_set_voltage(arizona->dcvdd, 1200000, 1800000);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int arizona_dvfs_remove_boost(struct arizona *arizona)
> +{
> +	int ret;
> +
> +	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: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_set_voltage(arizona->dcvdd, 1200000, 1800000);
> +	if (ret != 0) {
> +		dev_err(arizona->dev,
> +			"Failed to unboost DCVDD : %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int arizona_dvfs_up(struct arizona *arizona, unsigned int flags)
> +{
> +	unsigned int old_flags;
> +	int ret = 0;
> +
> +	switch (arizona->type) {
> +	case WM5102:
> +	case WM8997:
> +	case WM8998:
> +	case WM1814:
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	mutex_lock(&arizona->subsys_max_lock);
> +
> +	old_flags = arizona->subsys_max_rq;
> +	arizona->subsys_max_rq |= flags;
> +
> +	/* If currently caching the change will be applied in runtime resume */
> +	if (arizona->subsys_max_cached) {
> +		dev_dbg(arizona->dev, "subsys_max_cached (dvfs up)\n");
> +		goto out;
> +	}
> +
> +	if ((old_flags == 0) && (arizona->subsys_max_rq != 0))
> +		ret = arizona_dvfs_apply_boost(arizona);
> +out:
> +	mutex_unlock(&arizona->subsys_max_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(arizona_dvfs_up);
> +
> +int arizona_dvfs_down(struct arizona *arizona, unsigned int flags)
> +{
> +	unsigned int old_flags;
> +	int ret = 0;
> +
> +	switch (arizona->type) {
> +	case WM5102:
> +	case WM8997:
> +	case WM8998:
> +	case WM1814:
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	mutex_lock(&arizona->subsys_max_lock);
> +
> +	old_flags = arizona->subsys_max_rq;
> +	arizona->subsys_max_rq &= ~flags;
> +
> +	/* If currently caching the change will be applied in runtime resume */
> +	if (arizona->subsys_max_cached) {
> +		dev_dbg(arizona->dev, "subsys_max_cached (dvfs down)\n");
> +		goto out;
> +	}
> +
> +	if ((old_flags != 0) && (arizona->subsys_max_rq == 0))
> +		ret = arizona_dvfs_remove_boost(arizona);
> +out:
> +	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;
> @@ -462,6 +578,54 @@ static int wm5102_clear_write_sequencer(struct arizona *arizona)
>  }
>  
>  #ifdef CONFIG_PM
> +static int arizona_restore_dvfs(struct arizona *arizona)
> +{
> +	int ret;
> +
> +	switch (arizona->type) {
> +	case WM5102:
> +	case WM8997:
> +	case WM8998:
> +	case WM1814:
> +		ret = 0;
> +		mutex_lock(&arizona->subsys_max_lock);
> +		if (arizona->subsys_max_rq != 0) {
> +			dev_dbg(arizona->dev, "Restore subsys_max boost\n");
> +			ret = arizona_dvfs_apply_boost(arizona);
> +		}
> +
> +		if (ret == 0)
> +			arizona->subsys_max_cached = false;
> +
> +		mutex_unlock(&arizona->subsys_max_lock);
> +		return ret;
> +	default:
> +		return 0;	/* no DVFS */
> +	}
> +}
> +
> +static int arizona_suspend_dvfs(struct arizona *arizona)
> +{
> +	int ret;
> +
> +	switch (arizona->type) {
> +	case WM5102:
> +	case WM8997:
> +	case WM8998:
> +	case WM1814:
> +		mutex_lock(&arizona->subsys_max_lock);
> +		ret = arizona_dvfs_remove_boost(arizona);
> +
> +		if (ret == 0)
> +			arizona->subsys_max_cached = true;
> +
> +		mutex_unlock(&arizona->subsys_max_lock);
> +		break;
> +	default:
> +		return 0;
> +	}
> +}
> +
>  static int arizona_runtime_resume(struct device *dev)
>  {
>  	struct arizona *arizona = dev_get_drvdata(dev);
> @@ -590,6 +754,12 @@ static int arizona_runtime_resume(struct device *dev)
>  		goto err;
>  	}
>  
> +	ret = arizona_restore_dvfs(arizona);
> +	if (ret) {
> +		dev_err(arizona->dev, "Failed to restore DVFS: %d\n", ret);
> +		goto err;
> +	}
> +
>  	return 0;
>  
>  err:
> @@ -612,6 +782,13 @@ static int arizona_runtime_suspend(struct device *dev)
>  		return ret;
>  	}
>  
> +	/* Must disable DVFS boost before powering down DCVDD */
> +	ret = arizona_suspend_dvfs(arizona);
> +	if (ret) {
> +		dev_err(dev, "Failed to suspend DVFS: %d\n", ret);
> +		return ret;
> +	}
> +
>  	if (arizona->external_dcvdd) {
>  		ret = regmap_update_bits(arizona->regmap,
>  					 ARIZONA_ISOLATION_CONTROL,
> @@ -620,7 +797,7 @@ static int arizona_runtime_suspend(struct device *dev)
>  		if (ret != 0) {
>  			dev_err(arizona->dev, "Failed to isolate DCVDD: %d\n",
>  				ret);
> -			return ret;
> +			goto err;
>  		}
>  	}
>  
> @@ -639,7 +816,7 @@ static int arizona_runtime_suspend(struct device *dev)
>  		if (ret < 0) {
>  			dev_err(arizona->dev,
>  				"Failed to set suspend voltage: %d\n", ret);
> -			return ret;
> +			goto err;
>  		}
>  		break;
>  	case WM5102:
> @@ -650,7 +827,7 @@ static int arizona_runtime_suspend(struct device *dev)
>  				dev_err(arizona->dev,
>  					"Failed to clear write sequencer: %d\n",
>  					ret);
> -				return ret;
> +				goto err;
>  			}
>  		}
>  		break;
> @@ -675,6 +852,10 @@ static int arizona_runtime_suspend(struct device *dev)
>  	}
>  
>  	return 0;
> +
> +err:
> +	arizona_restore_dvfs(arizona);
> +	return ret;
>  }
>  #endif
>  
> @@ -933,6 +1114,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 b34a625..f18e4ca 100644
> --- a/include/linux/mfd/arizona/core.h
> +++ b/include/linux/mfd/arizona/core.h
> @@ -136,6 +136,10 @@ struct arizona {
>  	struct mutex clk_lock;
>  	int clk32k_ref;
>  
> +	struct mutex subsys_max_lock;
> +	unsigned int subsys_max_rq;
> +	bool subsys_max_cached;
> +
>  	bool ctrlif_error;
>  
>  	struct snd_soc_dapm_context *dapm;
> @@ -148,8 +152,17 @@ struct arizona {
>  	struct mutex dac_comp_lock;
>  };
>  
> +#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);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 1/1] mfd: arizona: Export functions to control subsystem DVFS

[Mark Brown]

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

On Tue, May 26, 2015 at 02:52:05PM +0100, Lee Jones wrote:

> Shouldn't this DVFS dump live in drivers/cpufreq?

cpufreq is specifically for CPUs (with the governors and everything) and
has its own problems here.  There is devfreq which was crated for this
sort of thing (though more on a SoC level) though, not sure if it's a
good fit or not.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v6 1/1] mfd: arizona: Export functions to control subsystem DVFS

[Richard Fitzgerald]

On Tue, May 26, 2015 at 04:29:42PM +0100, Mark Brown wrote:
> On Tue, May 26, 2015 at 02:52:05PM +0100, Lee Jones wrote:
> 
> > Shouldn't this DVFS dump live in drivers/cpufreq?
> 
> cpufreq is specifically for CPUs (with the governors and everything) and
> has its own problems here.  There is devfreq which was crated for this
> sort of thing (though more on a SoC level) though, not sure if it's a
> good fit or not.

The only codecs that have DVFS are WM8997, WM8998 and WM5102. No newer
codecs have it so it feels like unnecessary effort to create a whole
new driver for these three codecs. In any case, all we're really doing
here is setting the state of a single bit which tells the codec whether
we're feeding it enough DCVDD for it to do its internal DVFS. Most of
the additional complexity here is around co-operating with the
runtime_suspend/resume code in the MFD driver,

Re: [PATCH v6 1/1] mfd: arizona: Export functions to control subsystem DVFS

[Richard Fitzgerald]

On Tue, May 26, 2015 at 04:29:42PM +0100, Mark Brown wrote:
> On Tue, May 26, 2015 at 02:52:05PM +0100, Lee Jones wrote:
> 
> > Shouldn't this DVFS dump live in drivers/cpufreq?
> 
> cpufreq is specifically for CPUs (with the governors and everything) and
> has its own problems here.  There is devfreq which was crated for this
> sort of thing (though more on a SoC level) though, not sure if it's a
> good fit or not.

devfreq does look as though it's at the wrong sort of level. Also looks
like it could introduce more complexity than is justified.

An simpler alternative if we don't want this code in mfd is that
potentially this DVFS handling could be moved into the SoC codec driver.

Re: [PATCH v6 1/1] mfd: arizona: Export functions to control subsystem DVFS

[Lee Jones]

On Tue, 26 May 2015, Richard Fitzgerald wrote:

> On Tue, May 26, 2015 at 04:29:42PM +0100, Mark Brown wrote:
> > On Tue, May 26, 2015 at 02:52:05PM +0100, Lee Jones wrote:
> > 
> > > Shouldn't this DVFS dump live in drivers/cpufreq?
> > 
> > cpufreq is specifically for CPUs (with the governors and everything) and
> > has its own problems here.  There is devfreq which was crated for this
> > sort of thing (though more on a SoC level) though, not sure if it's a
> > good fit or not.
> 
> devfreq does look as though it's at the wrong sort of level. Also looks
> like it could introduce more complexity than is justified.
> 
> An simpler alternative if we don't want this code in mfd is that
> potentially this DVFS handling could be moved into the SoC codec driver.

If the DVFS configuration is specific to the CODEC, then yes, that
makes sense.

Sorry for making this awkward, but I'm starting to get pretty crabby
about MFD being a convenient dumping ground for misfit functionality.
I also understand that this isn't just down to you.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog