[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 at 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 */
>>   };
>>   
>>
>