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 <[email protected]> > > > > 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
signature.asc
Description: PGP signature
