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

[Peng Fan]

On Thu, Sep 25, 2025 at 04:28:15PM +0100, Andrew Goodbody wrote:
>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

There is a applied message when I apply the patch, I use b4 ty for this.

https://lore.kernel.org/u-boot/175873139553.3222.3482387473099940414.b4-ty@nxp.com/

Thanks
Peng