[PATCH 0/6] power: regulator: Fix some Smatch reported issues

[PATCH 0/6] power: regulator: Fix some Smatch reported issues

[Andrew Goodbody]

Smatch reported some issues in the regulator drivers, mostly repeated
instances of testing an unsigned variable for being negative but also an
expression needing parenthesis to be interpreted as expected.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
Andrew Goodbody (6):
      power: regulator: lp873x: Cannot test unsigned for being negative
      power: regulator: lp87565: Cannot test unsigned for being negative
      power: regulator: palmas: Cannot test unsigned for being negative
      power: regulator: rzg2l-usbphy: Add parenthesis to return expression
      power: regulator: tps65910: Cannot test unsigned for being negative
      power: regulator: tps65941: Cannot test unsigned for being negative

 drivers/power/regulator/lp873x_regulator.c       | 8 ++++----
 drivers/power/regulator/lp87565_regulator.c      | 4 ++--
 drivers/power/regulator/palmas_regulator.c       | 8 ++++----
 drivers/power/regulator/rzg2l-usbphy-regulator.c | 2 +-
 drivers/power/regulator/tps65941_regulator.c     | 8 ++++----
 include/power/tps65910_pmic.h                    | 2 +-
 6 files changed, 16 insertions(+), 16 deletions(-)
---
base-commit: 3526f990b77fca1c933f1d8b24eb9385010a05bf
change-id: 20250807-pwr_regulator-fa8bf0e4ac3d

Best regards,
-- 
Andrew Goodbody <andrew.goodbody@linaro.org>

[PATCH 1/6] power: regulator: lp873x: Cannot test unsigned for being negative

[Andrew Goodbody]

In lp873x_buck_val and lp873x_ldo_val hex is an unsigned variable being
assigned the return value from a function that returns int. Change hex
to be an int so that the following test for an error as a negative value
will work as expected.

This issue was found by Smatch.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 drivers/power/regulator/lp873x_regulator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/regulator/lp873x_regulator.c b/drivers/power/regulator/lp873x_regulator.c
index 271a7e45139..62d91a23f77 100644
--- a/drivers/power/regulator/lp873x_regulator.c
+++ b/drivers/power/regulator/lp873x_regulator.c
@@ -83,8 +83,8 @@ static int lp873x_buck_hex2volt(int hex)
 
 static int lp873x_buck_val(struct udevice *dev, int op, int *uV)
 {
-	unsigned int hex, adr;
-	int ret;
+	unsigned int adr;
+	int hex, ret;
 	struct dm_regulator_uclass_plat *uc_pdata;
 
 	uc_pdata = dev_get_uclass_plat(dev);
@@ -177,8 +177,8 @@ static int lp873x_ldo_hex2volt(int hex)
 
 static int lp873x_ldo_val(struct udevice *dev, int op, int *uV)
 {
-	unsigned int hex, adr;
-	int ret;
+	unsigned int adr;
+	int hex, ret;
 
 	struct dm_regulator_uclass_plat *uc_pdata;
 

-- 
2.39.5

[PATCH 2/6] power: regulator: lp87565: Cannot test unsigned for being negative

[Andrew Goodbody]

In lp87565_buck_val hex is an unsigned variable being assigned the return
value from a function that returns int. Change hex to be an int so that
the following test for an error as a negative value will work as expected.

This issue was found by Smatch.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 drivers/power/regulator/lp87565_regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/regulator/lp87565_regulator.c b/drivers/power/regulator/lp87565_regulator.c
index 2212cb5c651..ea9c7685e9f 100644
--- a/drivers/power/regulator/lp87565_regulator.c
+++ b/drivers/power/regulator/lp87565_regulator.c
@@ -82,8 +82,8 @@ static int lp87565_buck_val2volt(int val)
 
 static int lp87565_buck_val(struct udevice *dev, int op, int *uV)
 {
-	unsigned int hex, adr;
-	int ret;
+	unsigned int adr;
+	int hex, ret;
 	struct dm_regulator_uclass_plat *uc_pdata;
 
 	uc_pdata = dev_get_uclass_plat(dev);

-- 
2.39.5

[PATCH 3/6] power: regulator: palmas: Cannot test unsigned for being negative

[Andrew Goodbody]

In palmas_smps_val and palmas_ldo_val hex is an unsigned variable being
assigned the return value from a function that returns int. Change hex
to be an int so that the following test for an error as a negative value
will work as expected.

This issue was found by Smatch.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 drivers/power/regulator/palmas_regulator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/regulator/palmas_regulator.c b/drivers/power/regulator/palmas_regulator.c
index 2286eac93fb..7212062c8c8 100644
--- a/drivers/power/regulator/palmas_regulator.c
+++ b/drivers/power/regulator/palmas_regulator.c
@@ -115,8 +115,8 @@ static int palmas_smps_hex2volt(int hex, bool range)
 
 static int palmas_smps_val(struct udevice *dev, int op, int *uV)
 {
-	unsigned int hex, adr;
-	int ret;
+	unsigned int adr;
+	int hex, ret;
 	bool range;
 	struct dm_regulator_uclass_plat *uc_pdata;
 
@@ -251,8 +251,8 @@ static int palmas_ldo_hex2volt(int hex)
 
 static int palmas_ldo_val(struct udevice *dev, int op, int *uV)
 {
-	unsigned int hex, adr;
-	int ret;
+	unsigned int adr;
+	int hex, ret;
 
 	struct dm_regulator_uclass_plat *uc_pdata;
 

-- 
2.39.5

[PATCH 4/6] power: regulator: rzg2l-usbphy: Add parenthesis to return expression

[Andrew Goodbody]

In order to get the expected result from
rzg2l_usbphy_regulator_get_enable the return expression needs
parenthesis so that the binary and is performed before the double
logical not.

This issue was found by Smatch.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 drivers/power/regulator/rzg2l-usbphy-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/regulator/rzg2l-usbphy-regulator.c b/drivers/power/regulator/rzg2l-usbphy-regulator.c
index 451f04c140e..0354555d0b5 100644
--- a/drivers/power/regulator/rzg2l-usbphy-regulator.c
+++ b/drivers/power/regulator/rzg2l-usbphy-regulator.c
@@ -27,7 +27,7 @@ static int rzg2l_usbphy_regulator_get_enable(struct udevice *dev)
 {
 	struct rzg2l_usbphy_ctrl_priv *priv = dev_get_priv(dev->parent);
 
-	return !!readl(priv->regs + VBENCTL) & VBENCTL_VBUS_SEL;
+	return !!(readl(priv->regs + VBENCTL) & VBENCTL_VBUS_SEL);
 }
 
 static const struct dm_regulator_ops rzg2l_usbphy_regulator_ops = {

-- 
2.39.5

[PATCH 5/6] power: regulator: tps65910: Cannot test unsigned for being negative

[Andrew Goodbody]

The code in tps65910_regulator.c treats the field supply in struct
tps65910_regulator_pdata as an int and even tests the value for being
negative so change it from a u32 to int so that the code all works as
expected.

This issue was found by Smatch.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 include/power/tps65910_pmic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/power/tps65910_pmic.h b/include/power/tps65910_pmic.h
index 2026ec2f426..8fa02f33ab1 100644
--- a/include/power/tps65910_pmic.h
+++ b/include/power/tps65910_pmic.h
@@ -123,7 +123,7 @@ enum {
 
 /* platform data */
 struct tps65910_regulator_pdata {
-	u32 supply;	/* regulator supply voltage in uV */
+	int supply;	/* regulator supply voltage in uV */
 	uint unit;	/* unit-address according to DT */
 };
 

-- 
2.39.5

[PATCH 6/6] power: regulator: tps65941: Cannot test unsigned for being negative

[Andrew Goodbody]

In tps65941_buck_val and tps65941_ldo_val hex is an unsigned variable
being assigned the return value from a function that returns int. Change
hex to be an int so that the following test for an error as a negative
value will work as expected.

This issue was found by Smatch.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 drivers/power/regulator/tps65941_regulator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/regulator/tps65941_regulator.c b/drivers/power/regulator/tps65941_regulator.c
index 13f94b730d4..2561d6f4c6c 100644
--- a/drivers/power/regulator/tps65941_regulator.c
+++ b/drivers/power/regulator/tps65941_regulator.c
@@ -277,8 +277,8 @@ static const struct tps65941_reg_conv_ops buck_conv_ops[] = {
 
 static int tps65941_buck_val(struct udevice *dev, int op, int *uV)
 {
-	unsigned int hex, adr;
-	int ret, delta, uwait, slew, idx;
+	unsigned int adr;
+	int hex, ret, delta, uwait, slew, idx;
 	struct dm_regulator_uclass_plat *uc_pdata;
 	const struct tps65941_reg_conv_ops *conv_ops;
 	ulong chip_id;
@@ -479,8 +479,8 @@ static const struct tps65941_reg_conv_ops ldo_conv_ops[] = {
 
 static int tps65941_ldo_val(struct udevice *dev, int op, int *uV)
 {
-	unsigned int hex, adr;
-	int ret, ret_volt, idx, ldo_bypass;
+	unsigned int adr;
+	int hex, ret, ret_volt, idx, ldo_bypass;
 	struct dm_regulator_uclass_plat *uc_pdata;
 	const struct tps65941_reg_conv_ops *conv_ops;
 	ulong chip_id;

-- 
2.39.5

Re: [PATCH 1/6] power: regulator: lp873x: Cannot test unsigned for being negative

[Frieder Schrempf]

Am 07.08.25 um 18:35 schrieb Andrew Goodbody:
> In lp873x_buck_val and lp873x_ldo_val hex is an unsigned variable being
> assigned the return value from a function that returns int. Change hex
> to be an int so that the following test for an error as a negative value
> will work as expected.
> 
> This issue was found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Re: [PATCH 2/6] power: regulator: lp87565: Cannot test unsigned for being negative

[Frieder Schrempf]

Am 07.08.25 um 18:35 schrieb Andrew Goodbody:
> In lp87565_buck_val hex is an unsigned variable being assigned the return
> value from a function that returns int. Change hex to be an int so that
> the following test for an error as a negative value will work as expected.
> 
> This issue was found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Re: [PATCH 3/6] power: regulator: palmas: Cannot test unsigned for being negative

[Frieder Schrempf]

Am 07.08.25 um 18:35 schrieb Andrew Goodbody:
> In palmas_smps_val and palmas_ldo_val hex is an unsigned variable being
> assigned the return value from a function that returns int. Change hex
> to be an int so that the following test for an error as a negative value
> will work as expected.
> 
> This issue was found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Re: [PATCH 4/6] power: regulator: rzg2l-usbphy: Add parenthesis to return expression

[Frieder Schrempf]

Am 07.08.25 um 18:35 schrieb Andrew Goodbody:
> In order to get the expected result from
> rzg2l_usbphy_regulator_get_enable the return expression needs
> parenthesis so that the binary and is performed before the double
> logical not.
> 
> This issue was found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Re: [PATCH 5/6] power: regulator: tps65910: Cannot test unsigned for being negative

[Frieder Schrempf]

Am 07.08.25 um 18:35 schrieb Andrew Goodbody:
> The code in tps65910_regulator.c treats the field supply in struct
> tps65910_regulator_pdata as an int and even tests the value for being
> negative so change it from a u32 to int so that the code all works as
> expected.

I'm not sure if this is the best solution. The supply field holds a
voltage value in uV and u32 seems like a reasonable type to use.

I would argue that the driver should be changed to not use int and
remove the negative value check.

> 
> This issue was found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>  include/power/tps65910_pmic.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/power/tps65910_pmic.h b/include/power/tps65910_pmic.h
> index 2026ec2f426..8fa02f33ab1 100644
> --- a/include/power/tps65910_pmic.h
> +++ b/include/power/tps65910_pmic.h
> @@ -123,7 +123,7 @@ enum {
>  
>  /* platform data */
>  struct tps65910_regulator_pdata {
> -	u32 supply;	/* regulator supply voltage in uV */
> +	int supply;	/* regulator supply voltage in uV */
>  	uint unit;	/* unit-address according to DT */
>  };
>  
> 

Re: [PATCH 6/6] power: regulator: tps65941: Cannot test unsigned for being negative

[Frieder Schrempf]

Am 07.08.25 um 18:35 schrieb Andrew Goodbody:
> In tps65941_buck_val and tps65941_ldo_val hex is an unsigned variable
> being assigned the return value from a function that returns int. Change
> hex to be an int so that the following test for an error as a negative
> value will work as expected.
> 
> This issue was found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Re: (subset) [PATCH 0/6] power: regulator: Fix some Smatch reported issues

[Tom Rini]

On Thu, 07 Aug 2025 17:35:17 +0100, Andrew Goodbody wrote:

> Smatch reported some issues in the regulator drivers, mostly repeated
> instances of testing an unsigned variable for being negative but also an
> expression needing parenthesis to be interpreted as expected.
> 
> 

Applied to u-boot/next, thanks!

[1/6] power: regulator: lp873x: Cannot test unsigned for being negative
      commit: 42f959d0b091b779f235d7d0e54a794d4d857917
[2/6] power: regulator: lp87565: Cannot test unsigned for being negative
      commit: 8f90028ccdbfc74487c4b40bd6fe5d34c3309b7a
[3/6] power: regulator: palmas: Cannot test unsigned for being negative
      commit: 96312ed79630838b09368c8c07ec164fd40a025c
[4/6] power: regulator: rzg2l-usbphy: Add parenthesis to return expression
      commit: cdc0e32e9c56fc3d14007d4c93394e8bcc8daa08
[6/6] power: regulator: tps65941: Cannot test unsigned for being negative
      commit: c18435e648f7004dee1a80d06dbc06223d3b76b3
-- 
Tom

Re: [PATCH 5/6] power: regulator: tps65910: Cannot test unsigned for being negative

[Andrew Goodbody]

On 27/08/2025 08:54, Frieder Schrempf wrote:
> Am 07.08.25 um 18:35 schrieb Andrew Goodbody:
>> The code in tps65910_regulator.c treats the field supply in struct
>> tps65910_regulator_pdata as an int and even tests the value for being
>> negative so change it from a u32 to int so that the code all works as
>> expected.
> 
> I'm not sure if this is the best solution. The supply field holds a
> voltage value in uV and u32 seems like a reasonable type to use.
> 
> I would argue that the driver should be changed to not use int and
> remove the negative value check.

Hi Frieder,

I would offer the counter argument that the TPS65910 has an absolute 
maximum rating of 7V so any advantage of being able to use all 32 bits 
vs 'only' 31 bits to hold a value in uV, when 23 bits would be enough, 
is somewhat lost.

As I say in the commit message all the rest of the code in this driver 
treats this field as an int and declares int variables to hold its value 
so using a u32 throughout this driver would just mean changes being made 
with no benefit. Removing the negative value check leaves the code open 
to unexpected behaviour and hard to find bugs.

More than that the field is being assigned to from the function 
regulator_get_value() which is in the regulator uclass and returns an 
int. So following your suggestion to its logical conclusion would mean 
changing the uclass and then that would lead to changes also being made 
to all other clients. This would turn into a major project which I am 
not very keen to take on as I do not see any benefit to it.

So yes, simply changing one field in a struct from u32 to int does seem 
to me to be the best solution. It achieves the aim of fixing the code 
with the minimum of effort and I see no downside to it.

Andrew

>> This issue was found by Smatch.
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>> ---
>>   include/power/tps65910_pmic.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/power/tps65910_pmic.h b/include/power/tps65910_pmic.h
>> index 2026ec2f426..8fa02f33ab1 100644
>> --- a/include/power/tps65910_pmic.h
>> +++ b/include/power/tps65910_pmic.h
>> @@ -123,7 +123,7 @@ enum {
>>   
>>   /* platform data */
>>   struct tps65910_regulator_pdata {
>> -	u32 supply;	/* regulator supply voltage in uV */
>> +	int supply;	/* regulator supply voltage in uV */
>>   	uint unit;	/* unit-address according to DT */
>>   };
>>   
>>
> 

Re: [PATCH 5/6] power: regulator: tps65910: Cannot test unsigned for being negative

[Frieder Schrempf]

Hi Andrew,

Am 01.09.25 um 18:26 schrieb Andrew Goodbody:
> On 27/08/2025 08:54, Frieder Schrempf wrote:
>> Am 07.08.25 um 18:35 schrieb Andrew Goodbody:
>>> The code in tps65910_regulator.c treats the field supply in struct
>>> tps65910_regulator_pdata as an int and even tests the value for being
>>> negative so change it from a u32 to int so that the code all works as
>>> expected.
>>
>> I'm not sure if this is the best solution. The supply field holds a
>> voltage value in uV and u32 seems like a reasonable type to use.
>>
>> I would argue that the driver should be changed to not use int and
>> remove the negative value check.
> 
> Hi Frieder,
> 
> I would offer the counter argument that the TPS65910 has an absolute
> maximum rating of 7V so any advantage of being able to use all 32 bits
> vs 'only' 31 bits to hold a value in uV, when 23 bits would be enough,
> is somewhat lost.
> 
> As I say in the commit message all the rest of the code in this driver
> treats this field as an int and declares int variables to hold its value
> so using a u32 throughout this driver would just mean changes being made
> with no benefit. Removing the negative value check leaves the code open
> to unexpected behaviour and hard to find bugs.
> 
> More than that the field is being assigned to from the function
> regulator_get_value() which is in the regulator uclass and returns an
> int. So following your suggestion to its logical conclusion would mean
> changing the uclass and then that would lead to changes also being made
> to all other clients. This would turn into a major project which I am
> not very keen to take on as I do not see any benefit to it.
> 
> So yes, simply changing one field in a struct from u32 to int does seem
> to me to be the best solution. It achieves the aim of fixing the code
> with the minimum of effort and I see no downside to it.

Thanks for the reply. I totally missed the fact that
regulator_get_value() returns an int. When I was looking at the code, I
assumed that there are only positive values ever used.

You are right. Sorry for the noise. The patch is fine.

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Thanks
Frieder