[PATCH] regulator: Fix bug in regulator_mode_to_status() core function.

[PATCH] regulator: Fix bug in regulator_mode_to_status() core function.

[Krystian Garbaciak]

Fix typo for case REGULATOR_STATUS_STANDBY -> REGULATOR_MODE_STANDBY.
For undefined mode, return REGULATOR_STATUS_ERROR (0 is not valid status).

Signed-off-by: Krystian Garbaciak <krystian.garbaciak@diasemi.com>
---
 drivers/regulator/core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 09a737c..b821a9f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2909,10 +2909,10 @@ int regulator_mode_to_status(unsigned int mode)
 		return REGULATOR_STATUS_NORMAL;
 	case REGULATOR_MODE_IDLE:
 		return REGULATOR_STATUS_IDLE;
-	case REGULATOR_STATUS_STANDBY:
+	case REGULATOR_MODE_STANDBY:
 		return REGULATOR_STATUS_STANDBY;
 	default:
-		return 0;
+		return REGULATOR_STATUS_ERROR;
 	}
 }
 EXPORT_SYMBOL_GPL(regulator_mode_to_status);
-- 
1.7.0.4

Re: [PATCH] regulator: Fix bug in regulator_mode_to_status() core function.

[Mark Brown]

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

On Wed, Jul 11, 2012 at 01:10:01PM +0100, Krystian Garbaciak wrote:
> Fix typo for case REGULATOR_STATUS_STANDBY -> REGULATOR_MODE_STANDBY.
> For undefined mode, return REGULATOR_STATUS_ERROR (0 is not valid status).

This is deliberate.  It's not reporting an error, it's reporting an
indeterminate status which is a different thing.

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

Re: [PATCH] regulator: Fix bug in regulator_mode_to_status() core function.

[Krystian Garbaciak]

> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: 11 July 2012 14:29
> 
> On Wed, Jul 11, 2012 at 01:10:01PM +0100, Krystian Garbaciak wrote:
> > Fix typo for case REGULATOR_STATUS_STANDBY -> REGULATOR_MODE_STANDBY.
> > For undefined mode, return REGULATOR_STATUS_ERROR (0 is not valid status).
> 
> This is deliberate.  It's not reporting an error, it's reporting an
> indeterminate status which is a different thing.

Ok, then I would propose to use REGULATOR_STATUS_OFF instead of 0, to present
your deliberate decision here.

Re: [PATCH] regulator: Fix bug in regulator_mode_to_status() core function.

[Mark Brown]

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

On Wed, Jul 11, 2012 at 03:18:00PM +0100, Krystian Garbaciak wrote:

> > This is deliberate.  It's not reporting an error, it's reporting an
> > indeterminate status which is a different thing.

> Ok, then I would propose to use REGULATOR_STATUS_OFF instead of 0, to present
> your deliberate decision here.

I don't think you're fully understanding "indeterminate" there...  the
whole point is that we can't adequately represent the state, making
something up isn't helping things.

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

Re: [PATCH] regulator: Fix bug in regulator_mode_to_status() core function.

[Krystian Garbaciak]

> > > This is deliberate.  It's not reporting an error, it's reporting an
> > > indeterminate status which is a different thing.
> 
> > Ok, then I would propose to use REGULATOR_STATUS_OFF instead of 0, to present
> > your deliberate decision here.
> 
> I don't think you're fully understanding "indeterminate" there...  the
> whole point is that we can't adequately represent the state, making
> something up isn't helping things.

Would it make more sense to have some special enum value for that case, let say
there would be REGULATOR_STATUS_UNDEFINED?

Returning 0 is interpreted as REGULATOR_STATUS_OFF outside the function.
But it may change, if ever the enumeration changes.

Re: [PATCH] regulator: Fix bug in regulator_mode_to_status() core function.

[Mark Brown]

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

On Wed, Jul 11, 2012 at 04:13:00PM +0100, Krystian Garbaciak wrote:

> Would it make more sense to have some special enum value for that case, let say
> there would be REGULATOR_STATUS_UNDEFINED?

> Returning 0 is interpreted as REGULATOR_STATUS_OFF outside the function.
> But it may change, if ever the enumeration changes.

That'd be fine.

Also, please do submit separate changes separately.

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

[PATCH] regulator: Fix a typo in regulator_mode_to_status() core function.

[Krystian Garbaciak]

Case REGULATOR_STATUS_STANDBY -> REGULATOR_MODE_STANDBY.

Signed-off-by: Krystian Garbaciak <krystian.garbaciak@diasemi.com>
---
 drivers/regulator/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 09a737c..af44b94 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2909,7 +2909,7 @@ int regulator_mode_to_status(unsigned int mode)
 		return REGULATOR_STATUS_NORMAL;
 	case REGULATOR_MODE_IDLE:
 		return REGULATOR_STATUS_IDLE;
-	case REGULATOR_STATUS_STANDBY:
+	case REGULATOR_MODE_STANDBY:
 		return REGULATOR_STATUS_STANDBY;
 	default:
 		return 0;
-- 
1.7.0.4

[PATCH] regulator: Add REGULATOR_STATUS_UNDEFINED.

[Krystian Garbaciak]

REGULATOR_STATUS_UNDEFINED is to be returned by regulator, if any other state
doesn't really apply.

Signed-off-by: Krystian Garbaciak <krystian.garbaciak@diasemi.com>
---
 drivers/regulator/core.c         |    5 ++++-
 include/linux/regulator/driver.h |    2 ++
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index af44b94..f042a8a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -427,6 +427,9 @@ static ssize_t regulator_status_show(struct device *dev,
 	case REGULATOR_STATUS_STANDBY:
 		label = "standby";
 		break;
+	case REGULATOR_STATUS_UNDEFINED:
+		label = "undefined";
+		break;
 	default:
 		return -ERANGE;
 	}
@@ -2912,7 +2915,7 @@ int regulator_mode_to_status(unsigned int mode)
 	case REGULATOR_MODE_STANDBY:
 		return REGULATOR_STATUS_STANDBY;
 	default:
-		return 0;
+		return REGULATOR_STATUS_UNDEFINED;
 	}
 }
 EXPORT_SYMBOL_GPL(regulator_mode_to_status);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index b0432cc..180ac49 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -32,6 +32,8 @@ enum regulator_status {
 	REGULATOR_STATUS_NORMAL,
 	REGULATOR_STATUS_IDLE,
 	REGULATOR_STATUS_STANDBY,
+	/* in case that any other status doesn't apply */
+	REGULATOR_STATUS_UNDEFINED,
 };
 
 /**
-- 
1.7.0.4

Re: [PATCH] regulator: Fix a typo in regulator_mode_to_status() core function.

[Mark Brown]

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

On Thu, Jul 12, 2012 at 11:50:38AM +0100, Krystian Garbaciak wrote:
> Case REGULATOR_STATUS_STANDBY -> REGULATOR_MODE_STANDBY.
> 
> Signed-off-by: Krystian Garbaciak <krystian.garbaciak@diasemi.com>

This doesn't apply against -next (or the topic/core branch).  Please
check what's going on there.

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

Re: [PATCH] regulator: Fix a typo in regulator_mode_to_status() core function.

[Mark Brown]

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

On Thu, Jul 12, 2012 at 11:50:38AM +0100, Krystian Garbaciak wrote:
> Case REGULATOR_STATUS_STANDBY -> REGULATOR_MODE_STANDBY.

Gah, actually this was due to git am applying your two patches in the
wrong order - applied now, thanks.

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

Re: [PATCH] regulator: Add REGULATOR_STATUS_UNDEFINED.

[Mark Brown]

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

On Thu, Jul 12, 2012 at 01:53:35PM +0100, Krystian Garbaciak wrote:
> REGULATOR_STATUS_UNDEFINED is to be returned by regulator, if any other state
> doesn't really apply.

Applied, thanks.

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

Re: [PATCH] regulator: Fix bug in regulator_mode_to_status() core function.

[Krystian Garbaciak]

> +static const struct i_table vsys_tbl[] = {
> > +	ILINE(0, DA906X_ADC_MAX, 2500, 5500)
> > +};
> > +
> > +static const struct i_table adcin_tbl[] = {
> > +	ILINE(0, DA906X_ADC_MAX, 0, 2500)
> > +};
> > +
> > +static const struct i_table tjunc_tbl[] = {
> > +	ILINE(0, DA906X_ADC_MAX, 333, -86)
> > +};
> > +
> > +static const struct i_table vbbat_tbl[] = {
> > +	ILINE(0, DA906X_ADC_MAX, 0, 5000)
> > +};
> 
> Since the first parameter to ILINE is always 0, it is not needed. This means
> that x0 in itable is also always 0 and thus not needed.
> 
> > +
> > +static const struct channel_info da906x_channels[] = {
> > +	[DA906X_VSYS]	= { "VSYS",
> > +			    vsys_tbl, ARRAY_SIZE(vsys_tbl) - 1,
> > +			    DA906X_REG_VSYS_RES },
> > +	[DA906X_ADCIN1]	= { "ADCIN1",
> > +			    adcin_tbl,	ARRAY_SIZE(adcin_tbl) - 1,
> > +			    DA906X_REG_ADCIN1_RES },
> > +	[DA906X_ADCIN2]	= { "ADCIN2",
> > +			    adcin_tbl,	ARRAY_SIZE(adcin_tbl) - 1,
> > +			    DA906X_REG_ADCIN2_RES },
> > +	[DA906X_ADCIN3]	= { "ADCIN3",
> > +			    adcin_tbl,	ARRAY_SIZE(adcin_tbl) - 1,
> > +			    DA906X_REG_ADCIN3_RES },
> > +	[DA906X_TJUNC]	= { "TJUNC",
> > +			    tjunc_tbl,	ARRAY_SIZE(tjunc_tbl) - 1 },
> > +	[DA906X_VBBAT]	= { "VBBAT",
> > +			    vbbat_tbl,	ARRAY_SIZE(vbbat_tbl) - 1}
> 
> 	s/1}/1 }/
> 
> > +};
> 
> You lost me here a bit (I am missing something ?).
> 
> Seems to be each table has exactly one entry. Since the table size is 1,
> ARRAY_SIZE(vbbat_tbl) - 1 is 0, and ...

An initial idea was to make the interpolation of the channel values using more
ILINE segments. Eventually, it ended up with one linear segment for every
channel, so it makes sense to reduce the code as you propose.

As suggested, driver name will be changed from "da906x" to "da9063".
I will adapt proposed changes and fixes.

Thank you for your comments,
Krystian