Hi Quentin, On 2024-09-02 13:14, Quentin Schulz wrote: > Hi Jonas, > > On 8/31/24 12:42 AM, Jonas Karlman wrote: >> Wrong POWER_EN reg is used to get and set enabled state for the RK806 >> buck 4 and 8 regulators, also wrong POWER_SLP_EN0 bit is used for >> suspend state for the RK806 buck 1-8 regulators. >> >> Fix this by not adding one to the zero based buck variable. >> >> Fixes: f172575d92cd ("power: rk8xx: add support for RK806") > > Shoot, I made a lot of mistakes in that driver :/ > > Thanks for catching those :) > >> Signed-off-by: Jonas Karlman <jo...@kwiboo.se> >> --- >> drivers/power/regulator/rk8xx.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/power/regulator/rk8xx.c >> b/drivers/power/regulator/rk8xx.c >> index 34e61511d884..3f5ec02b3824 100644 >> --- a/drivers/power/regulator/rk8xx.c >> +++ b/drivers/power/regulator/rk8xx.c >> @@ -415,7 +415,7 @@ static int _buck_set_enable(struct udevice *pmic, int >> buck, bool enable) >> break; >> case RK806_ID: >> value = RK806_POWER_EN_CLRSETBITS(buck % 4, enable); >> - en_reg = RK806_POWER_EN((buck + 1) / 4); >> + en_reg = RK806_POWER_EN(buck / 4); >> ret = pmic_reg_write(pmic, en_reg, value); >> break; >> case RK808_ID: >> @@ -494,7 +494,7 @@ static int _buck_get_enable(struct udevice *pmic, int >> buck) >> break; >> case RK806_ID: >> mask = BIT(buck % 4); >> - ret = pmic_reg_read(pmic, RK806_POWER_EN((buck + 1) / 4)); >> + ret = pmic_reg_read(pmic, RK806_POWER_EN(buck / 4)); >> break; >> case RK808_ID: >> case RK818_ID: >> @@ -541,10 +541,10 @@ static int _buck_set_suspend_enable(struct udevice >> *pmic, int buck, bool enable) >> >> if (buck + 1 >= 9) { >> reg = RK806_POWER_SLP_EN1; >> - mask = BIT(buck + 1 - 3); >> + mask = BIT(buck - 2); > > I like my (+ 1 - 3) here to match buck + 1 above. buck + 1 represents > the buck number in the datasheet (index starts at one), so you need to > subtract 3 to that index to find the bit index in the register.
I understand this reasoning and would fully agree if this was the only use of buck in BIT(), however each time I tried to scan over this code, (with buck is 0-based in mind) this buck + 1 - 3 made me stop and re-think if this really was correct. I have no strong opinion and can revert this change if you like. > >> } else { >> reg = RK806_POWER_SLP_EN0; >> - mask = BIT(buck + 1); >> + mask = BIT(buck); >> } >> ret = pmic_clrsetbits(pmic, reg, mask, enable ? mask : >> 0); >> } >> @@ -592,10 +592,10 @@ static int _buck_get_suspend_enable(struct udevice >> *pmic, int buck) >> >> if (buck + 1 >= 9) { >> reg = RK806_POWER_SLP_EN1; >> - mask = BIT(buck + 1 - 3); >> + mask = BIT(buck - 2); > > Ditto. > > Just a matter of taste though! Let me know if you want me to send a v2 with this change reverted. Regards, Jonas > > Reviewed-by: Quentin Schulz <quentin.sch...@cherry.de> > > @Kever Since it's a bugfix, can we have this in your next MR for > v2024.10 please? Thanks! > > Thanks! > Quentin