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

[Tom Rini]

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).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250925/0c9954c2/attachment.sig>