* [PATCH] regulator: Fix return type of of_map_mode()
@ 2018-04-17 17:12 Douglas Anderson
2018-04-17 17:13 ` Mark Brown
2018-04-17 17:22 ` Mark Brown
0 siblings, 2 replies; 5+ messages in thread
From: Douglas Anderson @ 2018-04-17 17:12 UTC (permalink / raw)
To: broonie
Cc: David Collins, evgreen, swboyd, Douglas Anderson,
Javier Martinez Canillas, linux-kernel, Liam Girdwood,
Tony Lindgren, linux-omap
In of_get_regulation_constraints() it can clearly be seen that the
return value of of_map_mode() is assigned to a signed integer. This
is important because the first thing the regulator core does with this
value is to compare it to -EINVAL.
Let's fix the return type of all of the current of_map_mode()
functions. While we're at it, we'll remove one pointless "inline".
Fixes: 5e5e3a42c653 ("regulator: of: Add support for parsing initial and suspend modes")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/regulator/cpcap-regulator.c | 2 +-
drivers/regulator/max77802-regulator.c | 2 +-
drivers/regulator/qcom_spmi-regulator.c | 2 +-
drivers/regulator/twl-regulator.c | 2 +-
include/linux/regulator/driver.h | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/regulator/cpcap-regulator.c b/drivers/regulator/cpcap-regulator.c
index f541b80f1b54..46eba038f8e9 100644
--- a/drivers/regulator/cpcap-regulator.c
+++ b/drivers/regulator/cpcap-regulator.c
@@ -214,7 +214,7 @@ static int cpcap_regulator_disable(struct regulator_dev *rdev)
return error;
}
-static unsigned int cpcap_map_mode(unsigned int mode)
+static int cpcap_map_mode(unsigned int mode)
{
switch (mode) {
case CPCAP_BIT_AUDIO_NORMAL_MODE:
diff --git a/drivers/regulator/max77802-regulator.c b/drivers/regulator/max77802-regulator.c
index b6261903818c..8c6b5d2aec86 100644
--- a/drivers/regulator/max77802-regulator.c
+++ b/drivers/regulator/max77802-regulator.c
@@ -75,7 +75,7 @@ struct max77802_regulator_prv {
unsigned int opmode[MAX77802_REG_MAX];
};
-static inline unsigned int max77802_map_mode(unsigned int mode)
+static inline int max77802_map_mode(unsigned int mode)
{
return mode == MAX77802_OPMODE_NORMAL ?
REGULATOR_MODE_NORMAL : REGULATOR_MODE_STANDBY;
diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 63c7a0c17777..c83124061632 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1432,7 +1432,7 @@ static void spmi_regulator_get_dt_config(struct spmi_regulator *vreg,
&data->vs_soft_start_strength);
}
-static unsigned int spmi_regulator_of_map_mode(unsigned int mode)
+static int spmi_regulator_of_map_mode(unsigned int mode)
{
if (mode == 1)
return REGULATOR_MODE_NORMAL;
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index a4456db5849d..496917bfad05 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -266,7 +266,7 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
return twl4030_send_pb_msg(message);
}
-static inline unsigned int twl4030reg_map_mode(unsigned int mode)
+static int twl4030reg_map_mode(unsigned int mode)
{
switch (mode) {
case RES_STATE_ACTIVE:
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 4fc96cb8e5d7..c98da2500c9f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -367,7 +367,7 @@ struct regulator_desc {
unsigned int off_on_delay;
- unsigned int (*of_map_mode)(unsigned int mode);
+ int (*of_map_mode)(unsigned int mode);
};
/**
--
2.17.0.484.g0c8726318c-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] regulator: Fix return type of of_map_mode()
2018-04-17 17:12 [PATCH] regulator: Fix return type of of_map_mode() Douglas Anderson
@ 2018-04-17 17:13 ` Mark Brown
2018-04-17 17:22 ` Mark Brown
1 sibling, 0 replies; 5+ messages in thread
From: Mark Brown @ 2018-04-17 17:13 UTC (permalink / raw)
To: Douglas Anderson
Cc: David Collins, evgreen, swboyd, Javier Martinez Canillas,
linux-kernel, Liam Girdwood, Tony Lindgren, linux-omap
[-- Attachment #1: Type: text/plain, Size: 378 bytes --]
On Tue, Apr 17, 2018 at 10:12:04AM -0700, Douglas Anderson wrote:
> In of_get_regulation_constraints() it can clearly be seen that the
> return value of of_map_mode() is assigned to a signed integer. This
> is important because the first thing the regulator core does with this
> value is to compare it to -EINVAL.
Hrm, so how does the compiler manage to not warn about this?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] regulator: Fix return type of of_map_mode()
2018-04-17 17:12 [PATCH] regulator: Fix return type of of_map_mode() Douglas Anderson
2018-04-17 17:13 ` Mark Brown
@ 2018-04-17 17:22 ` Mark Brown
2018-04-17 17:48 ` Javier Martinez Canillas
1 sibling, 1 reply; 5+ messages in thread
From: Mark Brown @ 2018-04-17 17:22 UTC (permalink / raw)
To: Douglas Anderson
Cc: David Collins, evgreen, swboyd, Javier Martinez Canillas,
linux-kernel, Liam Girdwood, Tony Lindgren, linux-omap
[-- Attachment #1: Type: text/plain, Size: 839 bytes --]
On Tue, Apr 17, 2018 at 10:12:04AM -0700, Douglas Anderson wrote:
> In of_get_regulation_constraints() it can clearly be seen that the
> return value of of_map_mode() is assigned to a signed integer. This
> is important because the first thing the regulator core does with this
> value is to compare it to -EINVAL.
>
> Let's fix the return type of all of the current of_map_mode()
> functions. While we're at it, we'll remove one pointless "inline".
Ah, I see... the thing here is that the mode is always an unsigned int
since it's a bitmask - this goes out beying the use in of_map_mode() and
into all the other APIs. We only actually use 4 bits currently so I
think there's no problem switching to int but it seems we should
probably do that consistently throughout the API so that things don't
get missed later on.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] regulator: Fix return type of of_map_mode()
2018-04-17 17:22 ` Mark Brown
@ 2018-04-17 17:48 ` Javier Martinez Canillas
2018-04-18 3:35 ` Doug Anderson
0 siblings, 1 reply; 5+ messages in thread
From: Javier Martinez Canillas @ 2018-04-17 17:48 UTC (permalink / raw)
To: Mark Brown
Cc: Douglas Anderson, David Collins, evgreen, swboyd, Linux Kernel,
Liam Girdwood, Tony Lindgren, linux-omap
On Tue, Apr 17, 2018 at 7:22 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Apr 17, 2018 at 10:12:04AM -0700, Douglas Anderson wrote:
>> In of_get_regulation_constraints() it can clearly be seen that the
>> return value of of_map_mode() is assigned to a signed integer. This
>> is important because the first thing the regulator core does with this
>> value is to compare it to -EINVAL.
>>
Sorry about that, as Mark mentioned both the regulator device specific
and standard modes are a bitmap, and that's why I used an unsigned int
as return type.
IIRC, at some point in the review someone mentioned error handling and
I just blindly added a check for -EINVAL assuming the drivers would
return that, but forgot about the return type :(
>> Let's fix the return type of all of the current of_map_mode()
>> functions. While we're at it, we'll remove one pointless "inline".
>
> Ah, I see... the thing here is that the mode is always an unsigned int
> since it's a bitmask - this goes out beying the use in of_map_mode() and
> into all the other APIs. We only actually use 4 bits currently so I
> think there's no problem switching to int but it seems we should
> probably do that consistently throughout the API so that things don't
> get missed later on.
Maybe another option could be to add a REGULATOR_MODE_INVALID with
value 0x0, and fix the drivers that are returning -EINVAL to return
that instead?
In of_get_regulation_constraints() we could check for that and
propagate -EINVAL.
Best regards,
Javier
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] regulator: Fix return type of of_map_mode()
2018-04-17 17:48 ` Javier Martinez Canillas
@ 2018-04-18 3:35 ` Doug Anderson
0 siblings, 0 replies; 5+ messages in thread
From: Doug Anderson @ 2018-04-18 3:35 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Mark Brown, David Collins, Evan Green, swboyd, Linux Kernel,
Liam Girdwood, Tony Lindgren, linux-omap
Hi,
On Tue, Apr 17, 2018 at 10:48 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
>>> Let's fix the return type of all of the current of_map_mode()
>>> functions. While we're at it, we'll remove one pointless "inline".
>>
>> Ah, I see... the thing here is that the mode is always an unsigned int
>> since it's a bitmask - this goes out beying the use in of_map_mode() and
>> into all the other APIs. We only actually use 4 bits currently so I
>> think there's no problem switching to int but it seems we should
>> probably do that consistently throughout the API so that things don't
>> get missed later on.
>
> Maybe another option could be to add a REGULATOR_MODE_INVALID with
> value 0x0, and fix the drivers that are returning -EINVAL to return
> that instead?
>
> In of_get_regulation_constraints() we could check for that and
> propagate -EINVAL.
I like this idea. Posted at
<https://patchwork.kernel.org/patch/10347345/>. Note that there's no
actual error to propagate since of_get_regulation_constraints() just
prints the error and continues on its merry way.
-Doug
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-04-18 3:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-17 17:12 [PATCH] regulator: Fix return type of of_map_mode() Douglas Anderson
2018-04-17 17:13 ` Mark Brown
2018-04-17 17:22 ` Mark Brown
2018-04-17 17:48 ` Javier Martinez Canillas
2018-04-18 3:35 ` Doug Anderson
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.