[PATCH] regulator: of: simplifing the parsing code

[PATCH] regulator: of: simplifing the parsing code

[Saurabh Sengar]

in case of_property_read_u32 fails, it keeps the parameter unchanged
so no need to test if its success and then assign the value

Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
---
Hi Mark,

I also have concern related to how we are passing 'regulator-mode' and
'regulator-initial-mode'. Currently this require a extra function to be
set in 'of_map_mode', which can be avoided.
These two parameters can be set directly from the device tree as in below patch:
https://lkml.org/lkml/2014/1/16/263
All drivers can have only out of 4 predefined values for these parameters,
define in linux/iregulator/consumer.h, its not driver specific.
Please let me know your comments, if this suits you I will send a one more
patch on top of this it will further simplify this code.
Regards,
Saurabh


 drivers/regulator/of_regulator.c | 41 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 499e437..61b74a9 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -51,16 +51,14 @@ static void of_get_regulation_constraints(struct device_node *np,
 	if (min_uV && max_uV && constraints->min_uV == constraints->max_uV)
 		constraints->apply_uV = true;
 
-	if (!of_property_read_u32(np, "regulator-microvolt-offset", &pval))
-		constraints->uV_offset = pval;
-	if (!of_property_read_u32(np, "regulator-min-microamp", &pval))
-		constraints->min_uA = pval;
-	if (!of_property_read_u32(np, "regulator-max-microamp", &pval))
-		constraints->max_uA = pval;
-
-	if (!of_property_read_u32(np, "regulator-input-current-limit-microamp",
-				  &pval))
-		constraints->ilim_uA = pval;
+	of_property_read_u32(np, "regulator-microvolt-offset",
+						&constraints->uV_offset);
+	of_property_read_u32(np, "regulator-min-microamp",
+						&constraints->min_uA);
+	of_property_read_u32(np, "regulator-max-microamp",
+						&constraints->max_uA);
+	of_property_read_u32(np, "regulator-input-current-limit-microamp",
+						&constraints->ilim_uA);
 
 	/* Current change possible? */
 	if (constraints->min_uA != constraints->max_uA)
@@ -79,17 +77,13 @@ static void of_get_regulation_constraints(struct device_node *np,
 	if (of_property_read_bool(np, "regulator-allow-set-load"))
 		constraints->valid_ops_mask |= REGULATOR_CHANGE_DRMS;
 
-	ret = of_property_read_u32(np, "regulator-ramp-delay", &pval);
-	if (!ret) {
-		if (pval)
-			constraints->ramp_delay = pval;
-		else
+	if (!of_property_read_u32(np, "regulator-ramp-delay",
+					&constraints->ramp_delay))
+		if (!constraints->ramp_delay)
 			constraints->ramp_disable = true;
-	}
 
-	ret = of_property_read_u32(np, "regulator-enable-ramp-delay", &pval);
-	if (!ret)
-		constraints->enable_time = pval;
+	of_property_read_u32(np, "regulator-enable-ramp-delay",
+					&constraints->enable_time);
 
 	constraints->soft_start = of_property_read_bool(np,
 					"regulator-soft-start");
@@ -107,8 +101,8 @@ static void of_get_regulation_constraints(struct device_node *np,
 		}
 	}
 
-	if (!of_property_read_u32(np, "regulator-system-load", &pval))
-		constraints->system_load = pval;
+	of_property_read_u32(np, "regulator-system-load",
+					&constraints->system_load);
 
 	constraints->over_current_protection = of_property_read_bool(np,
 					"regulator-over-current-protection");
@@ -154,9 +148,8 @@ static void of_get_regulation_constraints(struct device_node *np,
 					"regulator-off-in-suspend"))
 			suspend_state->disabled = true;
 
-		if (!of_property_read_u32(suspend_np,
-					"regulator-suspend-microvolt", &pval))
-			suspend_state->uV = pval;
+		of_property_read_u32(suspend_np,
+			"regulator-suspend-microvolt", &suspend_state->uV);
 
 		of_node_put(suspend_np);
 		suspend_state = NULL;
-- 
1.9.1

Re: [PATCH] regulator: of: simplifing the parsing code

[Mark Brown]

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

On Fri, Nov 20, 2015 at 01:27:27PM +0530, Saurabh Sengar wrote:

> in case of_property_read_u32 fails, it keeps the parameter unchanged
> so no need to test if its success and then assign the value

On the other hand the current code doesn't require us to know that
detail...  it's really not entirely clear to me that this is a great
win.

> I also have concern related to how we are passing 'regulator-mode' and
> 'regulator-initial-mode'. Currently this require a extra function to be
> set in 'of_map_mode', which can be avoided.
> These two parameters can be set directly from the device tree as in below patch:
> https://lkml.org/lkml/2014/1/16/263

No, they can't - the values we set for modes in the DT are defined per
regulator and we need to translate these into the regulator API
definitions.

> +	of_property_read_u32(np, "regulator-microvolt-offset",
> +						&constraints->uV_offset);

The indentation for these is very strange - the second line is indented
a really long way.

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

Re: [PATCH] regulator: of: simplifing the parsing code

[Saurabh Sengar]

On 21 November 2015 at 18:52, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Nov 20, 2015 at 01:27:27PM +0530, Saurabh Sengar wrote:
>
>> I also have concern related to how we are passing 'regulator-mode' and
>> 'regulator-initial-mode'. Currently this require a extra function to be
>> set in 'of_map_mode', which can be avoided.
>> These two parameters can be set directly from the device tree as in below patch:
>> https://lkml.org/lkml/2014/1/16/263
>
> No, they can't - the values we set for modes in the DT are defined per
> regulator and we need to translate these into the regulator API
> definitions.
>

What I want to say is there are only 4 possible values which can be
set for initial_mode and suspend_state modes:

#define REGULATOR_MODE_FAST                     0x1
#define REGULATOR_MODE_NORMAL                   0x2
#define REGULATOR_MODE_IDLE                     0x4
#define REGULATOR_MODE_STANDBY                  0x8

these are defined in include/linux/regulator/consumer.h (core regulator file).
'of_map_modes' of each regulator driver ultimately set these values only.
I want to suggest that these values can directly be pass from dts.
Different dts can pass different values according to the regulator
they are using,
no need to decode them from of_map_modes function.
Let me know your comments on this.

Regards,
Saurabh

Re: [PATCH] regulator: of: simplifing the parsing code

[Mark Brown]

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

On Sat, Nov 21, 2015 at 07:41:54PM +0530, Saurabh Sengar wrote:
> On 21 November 2015 at 18:52, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Nov 20, 2015 at 01:27:27PM +0530, Saurabh Sengar wrote:

> > No, they can't - the values we set for modes in the DT are defined per
> > regulator and we need to translate these into the regulator API
> > definitions.

> What I want to say is there are only 4 possible values which can be
> set for initial_mode and suspend_state modes:

> #define REGULATOR_MODE_FAST                     0x1
> #define REGULATOR_MODE_NORMAL                   0x2
> #define REGULATOR_MODE_IDLE                     0x4
> #define REGULATOR_MODE_STANDBY                  0x8

No, the regulator can specify *any* value in their bindings - they don't
have to be those values, they should be values that are relevant to the
hardware rather than values that mean something in the current regulator
API abstractions.  Things like the values that are specified in the
datasheet for example.

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