Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683412 > unrolled thread

[PATCH 1/2] regulator: cpcap: Fix standby mode

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] regulator: cpcap: Fix standby mode Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-07 22:10 +0200
    Re: [PATCH 1/2] regulator: cpcap: Fix standby mode Tony Lindgren <tony@atomide.com> - 2017-07-08 07:40 +0200
    Re: [PATCH 1/2] regulator: cpcap: Fix standby mode Mark Brown <broonie@kernel.org> - 2017-07-10 14:30 +0200
      Re: [PATCH 1/2] regulator: cpcap: Fix standby mode Tony Lindgren <tony@atomide.com> - 2017-07-10 15:50 +0200
      Re: [PATCH 1/2] regulator: cpcap: Fix standby mode Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-10 15:50 +0200

#1683412 — [PATCH 1/2] regulator: cpcap: Fix standby mode

While working on the audio-codec I noticed, that the
low power mode of the regulators are not properly
supported. This fixes the issue for vaudio.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 drivers/regulator/cpcap-regulator.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/cpcap-regulator.c b/drivers/regulator/cpcap-regulator.c
index cc98aceed1c1..65da6dba0b82 100644
--- a/drivers/regulator/cpcap-regulator.c
+++ b/drivers/regulator/cpcap-regulator.c
@@ -121,6 +121,7 @@ struct cpcap_regulator {
 		.enable_val = (mode_val),				\
 		.disable_val = (off_val),				\
 		.ramp_delay = (volt_trans_time),			\
+		.of_map_mode = cpcap_map_mode,				\
 	},								\
 	.assign_reg = (assignment_reg),					\
 	.assign_mask = (assignment_mask),				\
@@ -211,13 +212,23 @@ static int cpcap_regulator_disable(struct regulator_dev *rdev)
 	return error;
 }
 
+static unsigned int cpcap_map_mode(unsigned int mode)
+{
+	switch (mode) {
+	case CPCAP_BIT_AUDIO_LOW_PWR:
+		return REGULATOR_MODE_STANDBY;
+	default:
+		return REGULATOR_MODE_NORMAL;
+	}
+}
+
 static unsigned int cpcap_regulator_get_mode(struct regulator_dev *rdev)
 {
 	int value;
 
 	regmap_read(rdev->regmap, rdev->desc->enable_reg, &value);
 
-	if (!(value & CPCAP_BIT_AUDIO_LOW_PWR))
+	if (value & CPCAP_BIT_AUDIO_LOW_PWR)
 		return REGULATOR_MODE_STANDBY;
 
 	return REGULATOR_MODE_NORMAL;
@@ -230,10 +241,10 @@ static int cpcap_regulator_set_mode(struct regulator_dev *rdev,
 
 	switch (mode) {
 	case REGULATOR_MODE_NORMAL:
-		value = CPCAP_BIT_AUDIO_LOW_PWR;
+		value = 0;
 		break;
 	case REGULATOR_MODE_STANDBY:
-		value = 0;
+		value = CPCAP_BIT_AUDIO_LOW_PWR;
 		break;
 	default:
 		return -EINVAL;
-- 
2.13.2

[toc] | [next] | [standalone]

#1683539

* Sebastian Reichel <sebastian.reichel@collabora.co.uk> [170707 13:08]:
> While working on the audio-codec I noticed, that the
> low power mode of the regulators are not properly
> supported. This fixes the issue for vaudio.

Yeah good catch:

Acked-by: Tony Lindgren <tony@atomide.com>

[toc] | [prev] | [next] | [standalone]

#1684187

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jul 07, 2017 at 10:08:12PM +0200, Sebastian Reichel wrote:

> While working on the audio-codec I noticed, that the
> low power mode of the regulators are not properly
> supported. This fixes the issue for vaudio.

In what way is it not properly supported and how does this change ensure
that it is properly supported?

> +static unsigned int cpcap_map_mode(unsigned int mode)
> +{
> +	switch (mode) {
> +	case CPCAP_BIT_AUDIO_LOW_PWR:
> +		return REGULATOR_MODE_STANDBY;
> +	default:
> +		return REGULATOR_MODE_NORMAL;
> +	}
> +}

This function is being added but is never referenced AFAICT, it should
either be removed or used.  If it is being used it should be changed so
that it doesn't accept the default case but instead accepts only
specific values so that the mapping of modes to regulator modes stays
1:1, unknown modes should error out.

[toc] | [prev] | [next] | [standalone]

#1684247

* Sebastian Reichel <sebastian.reichel@collabora.co.uk> [170710 06:45]:
> Hi,
> 
> On Mon, Jul 10, 2017 at 01:23:45PM +0100, Mark Brown wrote:
> > On Fri, Jul 07, 2017 at 10:08:12PM +0200, Sebastian Reichel wrote:
> > 
> > > While working on the audio-codec I noticed, that the
> > > low power mode of the regulators are not properly
> > > supported. This fixes the issue for vaudio.
> > 
> > In what way is it not properly supported and how does this change ensure
> > that it is properly supported?

Just to clarify this, I had inverted use of a register bit bug in my
original patch that $subject patch fixes.

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1684254

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Mon, Jul 10, 2017 at 01:23:45PM +0100, Mark Brown wrote:
> On Fri, Jul 07, 2017 at 10:08:12PM +0200, Sebastian Reichel wrote:
> 
> > While working on the audio-codec I noticed, that the
> > low power mode of the regulators are not properly
> > supported. This fixes the issue for vaudio.
> 
> In what way is it not properly supported and how does this change ensure
> that it is properly supported?
> 
> > +static unsigned int cpcap_map_mode(unsigned int mode)
> > +{
> > +	switch (mode) {
> > +	case CPCAP_BIT_AUDIO_LOW_PWR:
> > +		return REGULATOR_MODE_STANDBY;
> > +	default:
> > +		return REGULATOR_MODE_NORMAL;
> > +	}
> > +}
> 
> This function is being added but is never referenced AFAICT, it should
> either be removed or used.

You probably skipped over the patch to fast, it is referenced in the
patch section before the one adding the function:

+               .of_map_mode = cpcap_map_mode,                          \

> If it is being used it should be changed so that it doesn't accept
> the default case but instead accepts only specific values so that
> the mapping of modes to regulator modes stays 1:1, unknown modes
> should error out.

Fair point.

-- Sebastian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web