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

[Andrew Goodbody]

On 01/09/2025 17:58, Frieder Schrempf wrote:
> 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


This patch is still showing as 'Changes requested' in Patchwork but I am 
not sure why. Should I change its state or should I leave that to 
someone else?

Thanks,
Andrew