* [PATCH v2] regulator: Don't return or expect -errno from of_map_mode()
@ 2018-04-18 3:31 Douglas Anderson
2018-04-18 7:15 ` Javier Martinez Canillas
0 siblings, 1 reply; 4+ messages in thread
From: Douglas Anderson @ 2018-04-18 3:31 UTC (permalink / raw)
To: broonie
Cc: David Collins, Javier Martinez Canillas, evgreen, swboyd,
Douglas Anderson, linux-omap, Liam Girdwood, Tony Lindgren,
linux-kernel
In of_get_regulation_constraints() we were taking the result of
of_map_mode() (an unsigned int) and assigning it to an int. We were
then checking whether this value was -EINVAL. Some implementers of
of_map_mode() were returning -EINVAL (even though the return type of
their function needed to be unsigned int) because they needed to to
signal an error back to of_get_regulation_constraints().
In general in the regulator framework the mode is always referred to
as an unsigned int. While we could fix this to be a signed int (the
highest value we store in there right now is 0x8), it's actually
pretty clean to just define the regulator mode 0x0 (the lack of any
bits set) as an invalid mode. Let's do that.
Suggested-by: Javier Martinez Canillas <javier@dowhile0.org>
Fixes: 5e5e3a42c653 ("regulator: of: Add support for parsing initial and suspend modes")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v2:
- Use Javier's suggestion of defining 0x0 as invalid
drivers/regulator/cpcap-regulator.c | 2 +-
drivers/regulator/of_regulator.c | 15 +++++++++------
drivers/regulator/twl-regulator.c | 2 +-
include/linux/regulator/consumer.h | 1 +
4 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/regulator/cpcap-regulator.c b/drivers/regulator/cpcap-regulator.c
index f541b80f1b54..bd910fe123d9 100644
--- a/drivers/regulator/cpcap-regulator.c
+++ b/drivers/regulator/cpcap-regulator.c
@@ -222,7 +222,7 @@ static unsigned int cpcap_map_mode(unsigned int mode)
case CPCAP_BIT_AUDIO_LOW_PWR:
return REGULATOR_MODE_STANDBY;
default:
- return -EINVAL;
+ return REGULATOR_MODE_INVALID;
}
}
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index f47264fa1940..22c02b7a338b 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -124,11 +124,12 @@ static void of_get_regulation_constraints(struct device_node *np,
if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
if (desc && desc->of_map_mode) {
- ret = desc->of_map_mode(pval);
- if (ret == -EINVAL)
+ unsigned int mode = desc->of_map_mode(pval);
+
+ if (mode == REGULATOR_MODE_INVALID)
pr_err("%s: invalid mode %u\n", np->name, pval);
else
- constraints->initial_mode = ret;
+ constraints->initial_mode = mode;
} else {
pr_warn("%s: mapping for mode %d not defined\n",
np->name, pval);
@@ -163,12 +164,14 @@ static void of_get_regulation_constraints(struct device_node *np,
if (!of_property_read_u32(suspend_np, "regulator-mode",
&pval)) {
if (desc && desc->of_map_mode) {
- ret = desc->of_map_mode(pval);
- if (ret == -EINVAL)
+ unsigned int mode = desc->of_map_mode(pval);
+
+ mode = desc->of_map_mode(pval);
+ if (mode == REGULATOR_MODE_INVALID)
pr_err("%s: invalid mode %u\n",
np->name, pval);
else
- suspend_state->mode = ret;
+ suspend_state->mode = mode;
} else {
pr_warn("%s: mapping for mode %d not defined\n",
np->name, pval);
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index a4456db5849d..884c7505ed91 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -274,7 +274,7 @@ static inline unsigned int twl4030reg_map_mode(unsigned int mode)
case RES_STATE_SLEEP:
return REGULATOR_MODE_STANDBY;
default:
- return -EINVAL;
+ return REGULATOR_MODE_INVALID;
}
}
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index df176d7c2b87..25602afd4844 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -80,6 +80,7 @@ struct regmap;
* These modes can be OR'ed together to make up a mask of valid register modes.
*/
+#define REGULATOR_MODE_INVALID 0x0
#define REGULATOR_MODE_FAST 0x1
#define REGULATOR_MODE_NORMAL 0x2
#define REGULATOR_MODE_IDLE 0x4
--
2.17.0.484.g0c8726318c-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] regulator: Don't return or expect -errno from of_map_mode()
2018-04-18 3:31 [PATCH v2] regulator: Don't return or expect -errno from of_map_mode() Douglas Anderson
@ 2018-04-18 7:15 ` Javier Martinez Canillas
2018-04-18 15:56 ` Doug Anderson
0 siblings, 1 reply; 4+ messages in thread
From: Javier Martinez Canillas @ 2018-04-18 7:15 UTC (permalink / raw)
To: Douglas Anderson
Cc: Mark Brown, David Collins, evgreen, swboyd, linux-omap,
Liam Girdwood, Tony Lindgren, Linux Kernel
Hi Doug,
Patch looks good to me, I just have some minor comments.
On Wed, Apr 18, 2018 at 5:31 AM, Douglas Anderson <dianders@chromium.org> wrote:
> In of_get_regulation_constraints() we were taking the result of
> of_map_mode() (an unsigned int) and assigning it to an int. We were
> then checking whether this value was -EINVAL. Some implementers of
> of_map_mode() were returning -EINVAL (even though the return type of
> their function needed to be unsigned int) because they needed to to
s/to to/to
> signal an error back to of_get_regulation_constraints().
>
> In general in the regulator framework the mode is always referred to
> as an unsigned int. While we could fix this to be a signed int (the
> highest value we store in there right now is 0x8), it's actually
> pretty clean to just define the regulator mode 0x0 (the lack of any
> bits set) as an invalid mode. Let's do that.
>
> Suggested-by: Javier Martinez Canillas <javier@dowhile0.org>
> Fixes: 5e5e3a42c653 ("regulator: of: Add support for parsing initial and suspend modes")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - Use Javier's suggestion of defining 0x0 as invalid
>
> drivers/regulator/cpcap-regulator.c | 2 +-
> drivers/regulator/of_regulator.c | 15 +++++++++------
> drivers/regulator/twl-regulator.c | 2 +-
> include/linux/regulator/consumer.h | 1 +
> 4 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/regulator/cpcap-regulator.c b/drivers/regulator/cpcap-regulator.c
> index f541b80f1b54..bd910fe123d9 100644
> --- a/drivers/regulator/cpcap-regulator.c
> +++ b/drivers/regulator/cpcap-regulator.c
> @@ -222,7 +222,7 @@ static unsigned int cpcap_map_mode(unsigned int mode)
> case CPCAP_BIT_AUDIO_LOW_PWR:
> return REGULATOR_MODE_STANDBY;
> default:
> - return -EINVAL;
> + return REGULATOR_MODE_INVALID;
> }
> }
>
> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> index f47264fa1940..22c02b7a338b 100644
> --- a/drivers/regulator/of_regulator.c
> +++ b/drivers/regulator/of_regulator.c
> @@ -124,11 +124,12 @@ static void of_get_regulation_constraints(struct device_node *np,
>
> if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
> if (desc && desc->of_map_mode) {
> - ret = desc->of_map_mode(pval);
> - if (ret == -EINVAL)
> + unsigned int mode = desc->of_map_mode(pval);
I think the convention is to always declare local variables at the
start of the function? Although I couldn't find anything in the coding
style document...
> +
> + if (mode == REGULATOR_MODE_INVALID)
> pr_err("%s: invalid mode %u\n", np->name, pval);
> else
> - constraints->initial_mode = ret;
> + constraints->initial_mode = mode;
> } else {
> pr_warn("%s: mapping for mode %d not defined\n",
> np->name, pval);
> @@ -163,12 +164,14 @@ static void of_get_regulation_constraints(struct device_node *np,
> if (!of_property_read_u32(suspend_np, "regulator-mode",
> &pval)) {
> if (desc && desc->of_map_mode) {
> - ret = desc->of_map_mode(pval);
> - if (ret == -EINVAL)
> + unsigned int mode = desc->of_map_mode(pval);
> +
> + mode = desc->of_map_mode(pval);
You are calling .of_map_mode and assigning the return value twice here.
If you post a new version, feel free to add:
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Best regards,
Javier
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] regulator: Don't return or expect -errno from of_map_mode()
2018-04-18 7:15 ` Javier Martinez Canillas
@ 2018-04-18 15:56 ` Doug Anderson
2018-04-18 16:48 ` Mark Brown
0 siblings, 1 reply; 4+ messages in thread
From: Doug Anderson @ 2018-04-18 15:56 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Mark Brown, David Collins, Evan Green, swboyd, linux-omap,
Liam Girdwood, Tony Lindgren, Linux Kernel
Hi,
On Wed, Apr 18, 2018 at 12:15 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
>> @@ -124,11 +124,12 @@ static void of_get_regulation_constraints(struct device_node *np,
>>
>> if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
>> if (desc && desc->of_map_mode) {
>> - ret = desc->of_map_mode(pval);
>> - if (ret == -EINVAL)
>> + unsigned int mode = desc->of_map_mode(pval);
>
> I think the convention is to always declare local variables at the
> start of the function? Although I couldn't find anything in the coding
> style document...
I haven't seen this as a consistent kernel convention. It seems a bit
up to the subsystem and/or driver maintainer. However, I'm happy to
put it up at the top if it makes people happy.
>> @@ -163,12 +164,14 @@ static void of_get_regulation_constraints(struct device_node *np,
>> if (!of_property_read_u32(suspend_np, "regulator-mode",
>> &pval)) {
>> if (desc && desc->of_map_mode) {
>> - ret = desc->of_map_mode(pval);
>> - if (ret == -EINVAL)
>> + unsigned int mode = desc->of_map_mode(pval);
>> +
>> + mode = desc->of_map_mode(pval);
>
> You are calling .of_map_mode and assigning the return value twice here.
Dang it, thanks for catching.
-Doug
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] regulator: Don't return or expect -errno from of_map_mode()
2018-04-18 15:56 ` Doug Anderson
@ 2018-04-18 16:48 ` Mark Brown
0 siblings, 0 replies; 4+ messages in thread
From: Mark Brown @ 2018-04-18 16:48 UTC (permalink / raw)
To: Doug Anderson
Cc: Javier Martinez Canillas, David Collins, Evan Green, swboyd,
linux-omap, Liam Girdwood, Tony Lindgren, Linux Kernel
[-- Attachment #1: Type: text/plain, Size: 943 bytes --]
On Wed, Apr 18, 2018 at 08:56:22AM -0700, Doug Anderson wrote:
> On Wed, Apr 18, 2018 at 12:15 AM, Javier Martinez Canillas
> >> if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
> >> if (desc && desc->of_map_mode) {
> >> - ret = desc->of_map_mode(pval);
> >> - if (ret == -EINVAL)
> >> + unsigned int mode = desc->of_map_mode(pval);
> > I think the convention is to always declare local variables at the
> > start of the function? Although I couldn't find anything in the coding
> > style document...
> I haven't seen this as a consistent kernel convention. It seems a bit
> up to the subsystem and/or driver maintainer. However, I'm happy to
> put it up at the top if it makes people happy.
It's *fairly* consistent (this is C code after all) - doing something
different usually has some form of motivation.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-18 16:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-18 3:31 [PATCH v2] regulator: Don't return or expect -errno from of_map_mode() Douglas Anderson
2018-04-18 7:15 ` Javier Martinez Canillas
2018-04-18 15:56 ` Doug Anderson
2018-04-18 16:48 ` Mark Brown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.