diff mbox series

[2/5] ASoC: max9867: Fix power management

Message ID 20181123142729.GC7347@lenoch (mailing list archive)
State New, archived
Headers show
Series max9867 driver update | expand

Commit Message

Ladislav Michl Nov. 23, 2018, 2:27 p.m. UTC 
Move device enable to probe function. Doing that from prepare
callback allows only DAC to be enabled.
While here move suspend and resume functions to more common place.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 sound/soc/codecs/max9867.c | 62 +++++++++++++++-----------------------
 sound/soc/codecs/max9867.h |  2 +-
 2 files changed, 26 insertions(+), 38 deletions(-)

Comments

Mark Brown Nov. 26, 2018, 12:57 p.m. UTC | #1 
On Fri, Nov 23, 2018 at 03:27:29PM +0100, Ladislav Michl wrote:

> Move device enable to probe function. Doing that from prepare
> callback allows only DAC to be enabled.

This seems like it'll be a power consumption regression - instead of
managing the power at runtime we'll just leave the power on all the
time.  The normal place to do this would be either directly through DAPM
or via set_bias_level() - the latter seems a better fit here.

> While here move suspend and resume functions to more common place.

It would be better to split this out from the rest of the change as it's
a fairly large bit of code motion relative to the rest of the patch.

> @@ -491,19 +458,40 @@ static int max9867_i2c_probe(struct i2c_client *i2c,
>  	}
>  	ret = regmap_read(max9867->regmap, MAX9867_REVISION, &reg);
>  	if (ret < 0) {
> -		dev_err(&i2c->dev, "Failed to read: %d\n", ret);
> +		dev_err(&i2c->dev, "Failed to read revision: %d\n", ret);
>  		return ret;
>  	}

This is a cosmetic change unrelated to the rest of the patch, it should
be sent separately.
Ladislav Michl Nov. 27, 2018, 5:37 p.m. UTC | #2 
On Mon, Nov 26, 2018 at 12:57:19PM +0000, Mark Brown wrote:
> On Fri, Nov 23, 2018 at 03:27:29PM +0100, Ladislav Michl wrote:
> 
> > Move device enable to probe function. Doing that from prepare
> > callback allows only DAC to be enabled.
> 
> This seems like it'll be a power consumption regression - instead of
> managing the power at runtime we'll just leave the power on all the
> time.  The normal place to do this would be either directly through DAPM
> or via set_bias_level() - the latter seems a better fit here.

Well, I was unable to measure any consumption difference as all chip
domains are powered down anyway. But see bellow.

> > While here move suspend and resume functions to more common place.
> 
> It would be better to split this out from the rest of the change as it's
> a fairly large bit of code motion relative to the rest of the patch.

So something like this is preffered solution? (untested)

diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
index 1cda54b59854..4510f98724c2 100644
--- a/sound/soc/codecs/max9867.c
+++ b/sound/soc/codecs/max9867.c
@@ -248,17 +248,6 @@ static int max9867_dai_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static int max9867_prepare(struct snd_pcm_substream *substream,
-			 struct snd_soc_dai *dai)
-{
-	struct snd_soc_component *component = dai->component;
-	struct max9867_priv *max9867 = snd_soc_component_get_drvdata(component);
-
-	regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
-		MAX9867_SHTDOWN_MASK, MAX9867_SHTDOWN_MASK);
-	return 0;
-}
-
 static int max9867_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct snd_soc_component *component = dai->component;
@@ -361,7 +350,6 @@ static int max9867_dai_set_fmt(struct snd_soc_dai *codec_dai,
 static const struct snd_soc_dai_ops max9867_dai_ops = {
 	.set_fmt = max9867_dai_set_fmt,
 	.set_sysclk	= max9867_set_dai_sysclk,
-	.prepare	= max9867_prepare,
 	.digital_mute	= max9867_mute,
 	.hw_params = max9867_dai_hw_params,
 };
@@ -392,27 +380,59 @@ static struct snd_soc_dai_driver max9867_dai[] = {
 	}
 };
 
-#ifdef CONFIG_PM_SLEEP
-static int max9867_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int max9867_suspend(struct snd_soc_component *component)
 {
-	struct max9867_priv *max9867 = dev_get_drvdata(dev);
+	snd_soc_component_force_bias_level(component, SND_SOC_BIAS_OFF);
 
-	/* Drop down to power saving mode when system is suspended */
-	regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
-		MAX9867_SHTDOWN_MASK, ~MAX9867_SHTDOWN_MASK);
 	return 0;
 }
 
-static int max9867_resume(struct device *dev)
+static int max9867_resume(struct snd_soc_component *component)
 {
-	struct max9867_priv *max9867 = dev_get_drvdata(dev);
+	snd_soc_component_force_bias_level(component, SND_SOC_BIAS_STANDBY);
 
-	regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
-		MAX9867_SHTDOWN_MASK, MAX9867_SHTDOWN_MASK);
 	return 0;
 }
+#else
+#define max9867_suspend	NULL
+#define max9867_resume	NULL
 #endif
 
+static int max9867_set_bias_level(struct snd_soc_component *component,
+				  enum snd_soc_bias_level level)
+{
+	int err;
+	struct max9867_priv *max9867 = snd_soc_component_get_drvdata(component);
+
+	switch (level) {
+	case SND_SOC_BIAS_STANDBY:
+		if (snd_soc_component_get_bias_level(component) == SND_SOC_BIAS_OFF) {
+			err = regcache_sync(max9867->regmap);
+			if (err)
+				return err;
+
+			err = regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
+						 MAX9867_SHTDOWN, MAX9867_SHTDOWN);
+			if (err)
+				return err;
+		}
+		break;
+	case SND_SOC_BIAS_OFF:
+		err = regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
+					 MAX9867_SHTDOWN, 0);
+		if (err)
+			return err;
+
+		regcache_mark_dirty(max9867->regmap);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static const struct snd_soc_component_driver max9867_component = {
 	.controls		= max9867_snd_controls,
 	.num_controls		= ARRAY_SIZE(max9867_snd_controls),
@@ -420,6 +440,9 @@ static const struct snd_soc_component_driver max9867_component = {
 	.num_dapm_routes	= ARRAY_SIZE(max9867_audio_map),
 	.dapm_widgets		= max9867_dapm_widgets,
 	.num_dapm_widgets	= ARRAY_SIZE(max9867_dapm_widgets),
+	.suspend		= max9867_suspend,
+	.resume			= max9867_resume,
+	.set_bias_level		= max9867_set_bias_level,
 	.idle_bias_on		= 1,
 	.use_pmdown_time	= 1,
 	.endianness		= 1,
@@ -516,15 +539,10 @@ static const struct of_device_id max9867_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, max9867_of_match);
 
-static const struct dev_pm_ops max9867_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(max9867_suspend, max9867_resume)
-};
-
 static struct i2c_driver max9867_i2c_driver = {
 	.driver = {
 		.name = "max9867",
 		.of_match_table = of_match_ptr(max9867_of_match),
-		.pm = &max9867_pm_ops,
 	},
 	.probe  = max9867_i2c_probe,
 	.id_table = max9867_i2c_id,
diff --git a/sound/soc/codecs/max9867.h b/sound/soc/codecs/max9867.h
index 55cd9976ff47..d9170850c96e 100644
--- a/sound/soc/codecs/max9867.h
+++ b/sound/soc/codecs/max9867.h
@@ -67,7 +67,7 @@
 #define MAX9867_MICCONFIG    0x15
 #define MAX9867_MODECONFIG   0x16
 #define MAX9867_PWRMAN       0x17
-#define MAX9867_SHTDOWN_MASK (1<<7)
+#define MAX9867_SHTDOWN      0x80
 #define MAX9867_REVISION     0xff
 
 #define MAX9867_CACHEREGNUM 10

> > @@ -491,19 +458,40 @@ static int max9867_i2c_probe(struct i2c_client *i2c,
> >  	}
> >  	ret = regmap_read(max9867->regmap, MAX9867_REVISION, &reg);
> >  	if (ret < 0) {
> > -		dev_err(&i2c->dev, "Failed to read: %d\n", ret);
> > +		dev_err(&i2c->dev, "Failed to read revision: %d\n", ret);
> >  		return ret;
> >  	}
> 
> This is a cosmetic change unrelated to the rest of the patch, it should
> be sent separately.

I'll drop it completely, it is probably not worth sending separately.

Thank you,
	ladis
Mark Brown Nov. 28, 2018, 9:22 a.m. UTC | #3 
On Tue, Nov 27, 2018 at 06:37:27PM +0100, Ladislav Michl wrote:
> On Mon, Nov 26, 2018 at 12:57:19PM +0000, Mark Brown wrote:

> > > While here move suspend and resume functions to more common place.

> > It would be better to split this out from the rest of the change as it's
> > a fairly large bit of code motion relative to the rest of the patch.

> So something like this is preffered solution? (untested)

Yes.
diff mbox series

Patch

diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
index 1cda54b59854..07a8205b211a 100644
--- a/sound/soc/codecs/max9867.c
+++ b/sound/soc/codecs/max9867.c
@@ -248,17 +248,6 @@  static int max9867_dai_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static int max9867_prepare(struct snd_pcm_substream *substream,
-			 struct snd_soc_dai *dai)
-{
-	struct snd_soc_component *component = dai->component;
-	struct max9867_priv *max9867 = snd_soc_component_get_drvdata(component);
-
-	regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
-		MAX9867_SHTDOWN_MASK, MAX9867_SHTDOWN_MASK);
-	return 0;
-}
-
 static int max9867_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct snd_soc_component *component = dai->component;
@@ -361,7 +350,6 @@  static int max9867_dai_set_fmt(struct snd_soc_dai *codec_dai,
 static const struct snd_soc_dai_ops max9867_dai_ops = {
 	.set_fmt = max9867_dai_set_fmt,
 	.set_sysclk	= max9867_set_dai_sysclk,
-	.prepare	= max9867_prepare,
 	.digital_mute	= max9867_mute,
 	.hw_params = max9867_dai_hw_params,
 };
@@ -392,27 +380,6 @@  static struct snd_soc_dai_driver max9867_dai[] = {
 	}
 };
 
-#ifdef CONFIG_PM_SLEEP
-static int max9867_suspend(struct device *dev)
-{
-	struct max9867_priv *max9867 = dev_get_drvdata(dev);
-
-	/* Drop down to power saving mode when system is suspended */
-	regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
-		MAX9867_SHTDOWN_MASK, ~MAX9867_SHTDOWN_MASK);
-	return 0;
-}
-
-static int max9867_resume(struct device *dev)
-{
-	struct max9867_priv *max9867 = dev_get_drvdata(dev);
-
-	regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
-		MAX9867_SHTDOWN_MASK, MAX9867_SHTDOWN_MASK);
-	return 0;
-}
-#endif
-
 static const struct snd_soc_component_driver max9867_component = {
 	.controls		= max9867_snd_controls,
 	.num_controls		= ARRAY_SIZE(max9867_snd_controls),
@@ -491,19 +458,40 @@  static int max9867_i2c_probe(struct i2c_client *i2c,
 	}
 	ret = regmap_read(max9867->regmap, MAX9867_REVISION, &reg);
 	if (ret < 0) {
-		dev_err(&i2c->dev, "Failed to read: %d\n", ret);
+		dev_err(&i2c->dev, "Failed to read revision: %d\n", ret);
 		return ret;
 	}
 	dev_info(&i2c->dev, "device revision: %x\n", reg);
-	ret = devm_snd_soc_register_component(&i2c->dev, &max9867_component,
-			max9867_dai, ARRAY_SIZE(max9867_dai));
+	ret = regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
+				 MAX9867_SHTDOWN, MAX9867_SHTDOWN);
 	if (ret < 0) {
-		dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
+		dev_err(&i2c->dev, "Failed to enable: %d\n", ret);
 		return ret;
 	}
+	ret = devm_snd_soc_register_component(&i2c->dev, &max9867_component,
+			max9867_dai, ARRAY_SIZE(max9867_dai));
+	if (ret < 0)
+		dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
 	return ret;
 }
 
+static int __maybe_unused max9867_suspend(struct device *dev)
+{
+	struct max9867_priv *max9867 = dev_get_drvdata(dev);
+
+	/* Drop down to power saving mode when system is suspended */
+	return regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
+				  MAX9867_SHTDOWN, 0);
+}
+
+static int __maybe_unused max9867_resume(struct device *dev)
+{
+	struct max9867_priv *max9867 = dev_get_drvdata(dev);
+
+	return regmap_update_bits(max9867->regmap, MAX9867_PWRMAN,
+				  MAX9867_SHTDOWN, MAX9867_SHTDOWN);
+}
+
 static const struct i2c_device_id max9867_i2c_id[] = {
 	{ "max9867", 0 },
 	{ }
diff --git a/sound/soc/codecs/max9867.h b/sound/soc/codecs/max9867.h
index 55cd9976ff47..d9170850c96e 100644
--- a/sound/soc/codecs/max9867.h
+++ b/sound/soc/codecs/max9867.h
@@ -67,7 +67,7 @@ 
 #define MAX9867_MICCONFIG    0x15
 #define MAX9867_MODECONFIG   0x16
 #define MAX9867_PWRMAN       0x17
-#define MAX9867_SHTDOWN_MASK (1<<7)
+#define MAX9867_SHTDOWN      0x80
 #define MAX9867_REVISION     0xff
 
 #define MAX9867_CACHEREGNUM 10