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
