Re: [alsa-devel] [PATCH 002/002] ASoC: Replace max98090 Device Driver
On Thu, Jan 24, 2013 at 03:28:54PM -0800, Jerry Wong wrote:
This is an odd control that is a combination of an enum and a power management widget. The 0 value of the enum turns power off. Not sure what is another choice. The enum value needs to be saved so it can be restored when DAPM turns the control on.
That doesn't appear to be what your control actually does - what happens if someone writes ot the control while the widget is powered down?
+static const struct snd_kcontrol_new max98090_snd_controls[] = {
SOC_SINGLE("MIC Bias VCM Bandgap", M98090_REG_BIAS_CNTL,M98090_VCM_MODE_SHIFT, M98090_VCM_MODE_NUM - 1, 0),
Why is this a user visible control?
It is not controlled by DAPM. The low power function is taken care of by /SHDN, so this retains the user choice of bandgap or divider reference.
Why would the user want this choice?
+static int max98090_runtime_resume(struct device *dev) +{
struct max98090_priv *max98090 = dev_get_drvdata(dev);regcache_cache_only(max98090->regmap, false);max98090_reset(max98090);regcache_sync(max98090->regmap);
Do you really need to reset the device here? If anything I'd expect it on suspend since the reset state is probably the lowest power state.
Yes, for a soft reset where the device is not power cycled, DAPM has the device go in and out of the low power /SHDN\ state, and the old register values are retained. Without this, we would need to specify that the PMIC power cycle the chip.
This doesn't really answer my questions. Why would it matter if the PMIC has power cycled the chip, and if you do need to do this why is resume the time to do it?
________________________________________ From: Mark Brown [broonie@opensource.wolfsonmicro.com] Sent: Thursday, January 24, 2013 11:21 PM To: Jerry Wong Cc: jwong@jwassociates.com; alsa-devel@alsa-project.org Subject: Re: [alsa-devel][PATCH 002/002] ASoC: Replace max98090 Device Driver
On Thu, Jan 24, 2013 at 03:28:54PM -0800, Jerry Wong wrote:
This is an odd control that is a combination of an enum and a power management widget. The 0 value of the enum turns power off. Not sure what is another choice. The enum value needs to be saved so it can be restored when DAPM turns the control on.
That doesn't appear to be what your control actually does - what happens if someone writes ot the control while the widget is powered down?
It stores the desired value for DAPM to use when the widget is powered up.
+static const struct snd_kcontrol_new max98090_snd_controls[] = {
SOC_SINGLE("MIC Bias VCM Bandgap", M98090_REG_BIAS_CNTL,M98090_VCM_MODE_SHIFT, M98090_VCM_MODE_NUM - 1, 0),
Why is this a user visible control?
It is not controlled by DAPM. The low power function is taken care of by /SHDN, so this retains the user choice of bandgap or divider reference.
Why would the user want this choice?
This allows the user to select VCM Bandgap for all their references (higher power and higher audio quality when in use), or divider reference (lower power and lower audio quality when in use). /SHDN\ prevents power drain when either is not in use.
+static int max98090_runtime_resume(struct device *dev) +{
struct max98090_priv *max98090 = dev_get_drvdata(dev);regcache_cache_only(max98090->regmap, false);max98090_reset(max98090);regcache_sync(max98090->regmap);
Do you really need to reset the device here? If anything I'd expect it on suspend since the reset state is probably the lowest power state.
Yes, for a soft reset where the device is not power cycled, DAPM has the device go in and out of the low power /SHDN\ state, and the old register values are retained. Without this, we would need to specify that the PMIC power cycle the chip.
This doesn't really answer my questions. Why would it matter if the PMIC has power cycled the chip, and if you do need to do this why is resume the time to do it?
I see what you are asking. Good point. I will move it to probe.
Mark,
Shall I make the change listed third below, or shall I wait for feedback on yesterdays submission first?
Jerry ________________________________________ From: Mark Brown [broonie@opensource.wolfsonmicro.com] Sent: Thursday, January 24, 2013 11:21 PM To: Jerry Wong Cc: jwong@jwassociates.com; alsa-devel@alsa-project.org Subject: Re: [alsa-devel][PATCH 002/002] ASoC: Replace max98090 Device Driver
On Thu, Jan 24, 2013 at 03:28:54PM -0800, Jerry Wong wrote:
This is an odd control that is a combination of an enum and a power management widget. The 0 value of the enum turns power off. Not sure what is another choice. The enum value needs to be saved so it can be restored when DAPM turns the control on.
That doesn't appear to be what your control actually does - what happens if someone writes ot the control while the widget is powered down?
It stores the desired value for DAPM to use when the widget is powered up.
+static const struct snd_kcontrol_new max98090_snd_controls[] = {
SOC_SINGLE("MIC Bias VCM Bandgap", M98090_REG_BIAS_CNTL,M98090_VCM_MODE_SHIFT, M98090_VCM_MODE_NUM - 1, 0),
Why is this a user visible control?
It is not controlled by DAPM. The low power function is taken care of by /SHDN, so this retains the user choice of bandgap or divider reference.
Why would the user want this choice?
This allows the user to select VCM Bandgap for all their references (higher power and higher audio quality when in use), or divider reference (lower power and lower audio quality when in use). /SHDN\ prevents power drain when either is not in use.
+static int max98090_runtime_resume(struct device *dev) +{
struct max98090_priv *max98090 = dev_get_drvdata(dev);regcache_cache_only(max98090->regmap, false);max98090_reset(max98090);regcache_sync(max98090->regmap);
Do you really need to reset the device here? If anything I'd expect it on suspend since the reset state is probably the lowest power state.
Yes, for a soft reset where the device is not power cycled, DAPM has the device go in and out of the low power /SHDN\ state, and the old register values are retained. Without this, we would need to specify that the PMIC power cycle the chip.
This doesn't really answer my questions. Why would it matter if the PMIC has power cycled the chip, and if you do need to do this why is resume the time to do it?
I see what you are asking. Good point. I will move it to probe.
On Fri, Jan 25, 2013 at 09:02:45AM -0800, Jerry Wong wrote:
Shall I make the change listed third below, or shall I wait for feedback on yesterdays submission first?
Do the updates then I'll review all at once.
On Fri, Jan 25, 2013 at 07:39:45AM -0800, Jerry Wong wrote:
On Thu, Jan 24, 2013 at 03:28:54PM -0800, Jerry Wong wrote:
This is an odd control that is a combination of an enum and a power management widget. The 0 value of the enum turns power off. Not sure what is another choice. The enum value needs to be saved so it can be restored when DAPM turns the control on.
That doesn't appear to be what your control actually does - what happens if someone writes ot the control while the widget is powered down?
It stores the desired value for DAPM to use when the widget is powered up.
It looks like it's a SOC_ENUM_SINGLE so it should map directly on to the register map?
+static const struct snd_kcontrol_new max98090_snd_controls[] = {
SOC_SINGLE("MIC Bias VCM Bandgap", M98090_REG_BIAS_CNTL,M98090_VCM_MODE_SHIFT, M98090_VCM_MODE_NUM - 1, 0),
Why is this a user visible control?
It is not controlled by DAPM. The low power function is taken care of by /SHDN, so this retains the user choice of bandgap or divider reference.
Why would the user want this choice?
This allows the user to select VCM Bandgap for all their references (higher power and higher audio quality when in use), or divider reference (lower power and lower audio quality when in use). /SHDN\ prevents power drain when either is not in use.
So in that case I'd recommend doing something like making it an enmerated control which makes the tradeoff clear.
participants (2)
-
Jerry Wong -
Mark Brown