[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 at kontron.de>

Thanks
Frieder