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

Groups > linux.kernel > #1273773 > unrolled thread

[PATCH] regulator: of: simplifing the parsing code

Back to article view | Back to linux.kernel

Contents

  [PATCH] regulator: of: simplifing the parsing code Saurabh Sengar <saurabh.truth@gmail.com> - 2015-11-20 09:00 +0100
    Re: [PATCH] regulator: of: simplifing the parsing code Mark Brown <broonie@kernel.org> - 2015-11-21 14:30 +0100
      Re: [PATCH] regulator: of: simplifing the parsing code Saurabh Sengar <saurabh.truth@gmail.com> - 2015-11-21 15:20 +0100
        Re: [PATCH] regulator: of: simplifing the parsing code Mark Brown <broonie@kernel.org> - 2015-11-22 14:10 +0100

#1273773 — [PATCH] regulator: of: simplifing the parsing code

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1274649

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

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.

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

#1274664

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1274863

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

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web