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

[Andrew Goodbody]

On 25/09/2025 15:32, Tom Rini wrote:
> On Thu, Sep 25, 2025 at 12:11:14PM +0100, Andrew Goodbody wrote:
>> 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?
> 
> Please update the patchwork state. I assume patchwork also shows the b4
> email from when I applied it. However due to patchwork being slow
> sometimes the state change gets missed and I only run my script that
> updates patchwork to include the git hash when I also merge next to
> master (which is something I should look in to doing more frequently).

No, patchwork does not show an email from you applying the patch. 
Actually it looks like Peng Fan applied the patch then sent you a pull 
request which you merged but no b4 message from Peng that I can see in 
my inbox. Also the other patches in the series do not show such emails 
either although I did receive a b4 email from you which was sent as a 
reply to the cover letter but patchwork does not seem to have picked 
that up.

Andrew