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 <[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). > >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/[email protected]/ Thanks Peng
