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 <[email protected]>
---
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
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.
On 21 November 2015 at 18:52, Mark Brown <[email protected]> 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
On Sat, Nov 21, 2015 at 07:41:54PM +0530, Saurabh Sengar wrote:
> On 21 November 2015 at 18:52, Mark Brown <[email protected]> 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.