Hi Jonas, On Sun, 6 Aug 2023 at 06:04, Jonas Karlman <[email protected]> wrote: > > From: Joseph Chen <[email protected]> > > Information from the first range group is always used to calculate the > voltage returned for buck converters. This may result in wrong voltage > reported back to the regulator_get_value caller. > > Traverse all the possible BUCK ranges to fix this issue. > > Fixes: addd062beacc ("power: pmic: rk816: support rk816 pmic") > Fixes: b62280745e55 ("power: pmic: rk805: support rk805 pmic") > Fixes: b4a35574b38d ("power: pmic: rk817: support rk817 pmic") > Fixes: ee30068fa574 ("power: pmic: rk809: support rk809 pmic") > Signed-off-by: Joseph Chen <[email protected]> > [[email protected]: fix checkpatch error, update commit message] > Signed-off-by: Jonas Karlman <[email protected]> > --- > drivers/power/regulator/rk8xx.c | 98 ++++++++++++++++++++------------- > 1 file changed, 60 insertions(+), 38 deletions(-)
Reviewed-by: Simon Glass <[email protected]> > > diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c > index e95640a39b0a..21949d678326 100644 > --- a/drivers/power/regulator/rk8xx.c > +++ b/drivers/power/regulator/rk8xx.c > @@ -88,62 +88,65 @@ struct rk8xx_reg_info { > u8 config_reg; > u8 vsel_mask; > u8 min_sel; > + /* only for buck now */ > + u8 max_sel; > + u8 range_num; > }; Please comment the items. What is range_num? The number of ranges or the number of ranges - 1? > > static const struct rk8xx_reg_info rk808_buck[] = { > - { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK808_BUCK_VSEL_MASK, }, > - { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK808_BUCK_VSEL_MASK, }, > - { 712500, 12500, NA, NA, > REG_BUCK3_CONFIG, RK808_BUCK_VSEL_MASK, }, > - { 1800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, > REG_BUCK4_CONFIG, RK808_BUCK4_VSEL_MASK, }, > + { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK808_BUCK_VSEL_MASK, 0x00, 0x3f, 1}, > + { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK808_BUCK_VSEL_MASK, 0x00, 0x3f, 1}, > + { NA, NA, NA, NA, > REG_BUCK3_CONFIG, NA, NA, NA, 1}, > + { 1800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, > REG_BUCK4_CONFIG, RK808_BUCK4_VSEL_MASK, 0x00, 0x0f, 1}, > }; > > static const struct rk8xx_reg_info rk816_buck[] = { > /* buck 1 */ > - { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, }, > - { 1800000, 200000, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x3c, }, > - { 2300000, 0, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x3f, }, > + { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, 0x3b, 3}, > + { 1800000, 200000, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x3c, 0x3e, 3}, > + { 2300000, 0, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x3f, 0x3f, 3}, > /* buck 2 */ > - { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, }, > - { 1800000, 200000, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x3c, }, > - { 2300000, 0, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x3f, }, > + { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, 0x3b, 3}, > + { 1800000, 200000, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x3c, 0x3e, 3}, > + { 2300000, 0, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x3f, 0x3f, 3}, > /* buck 3 */ > - { 712500, 12500, NA, NA, > REG_BUCK3_CONFIG, RK818_BUCK_VSEL_MASK, }, > + { NA, NA, NA, NA, > REG_BUCK3_CONFIG, NA, NA, NA, 1}, > /* buck 4 */ > - { 800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, > REG_BUCK4_CONFIG, RK818_BUCK4_VSEL_MASK, }, > + { 800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, > REG_BUCK4_CONFIG, RK818_BUCK4_VSEL_MASK, 0x00, 0x1b, 1}, > }; > > static const struct rk8xx_reg_info rk809_buck5[] = { > /* buck 5 */ > - { 1500000, 0, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, > RK809_BUCK5_VSEL_MASK, 0x00, }, > - { 1800000, 200000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, > RK809_BUCK5_VSEL_MASK, 0x01, }, > - { 2800000, 200000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, > RK809_BUCK5_VSEL_MASK, 0x04, }, > - { 3300000, 300000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, > RK809_BUCK5_VSEL_MASK, 0x06, }, > + { 1500000, 0, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, > RK809_BUCK5_VSEL_MASK, 0x00, 0x00, 4}, > + { 1800000, 200000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, > RK809_BUCK5_VSEL_MASK, 0x01, 0x03, 4}, > + { 2800000, 200000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, > RK809_BUCK5_VSEL_MASK, 0x04, 0x05, 4}, > + { 3300000, 300000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, > RK809_BUCK5_VSEL_MASK, 0x06, 0x07, 4}, > }; > > static const struct rk8xx_reg_info rk817_buck[] = { > /* buck 1 */ > - { 500000, 12500, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), > RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x00, }, > - { 1500000, 100000, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), > RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x50, }, > - { 2400000, 0, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), > RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x59, }, > + { 500000, 12500, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), > RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x00, 0x4f, 3}, > + { 1500000, 100000, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), > RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x50, 0x58, 3}, > + { 2400000, 0, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), > RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x59, 0x59, 3}, > /* buck 2 */ > - { 500000, 12500, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), > RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x00, }, > - { 1500000, 100000, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), > RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x50, }, > - { 2400000, 0, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), > RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x59, }, > + { 500000, 12500, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), > RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x00, 0x4f, 3}, > + { 1500000, 100000, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), > RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x50, 0x58, 3}, > + { 2400000, 0, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), > RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x59, 0x59, 3}, > /* buck 3 */ > - { 500000, 12500, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), > RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x00, }, > - { 1500000, 100000, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), > RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x50, }, > - { 2400000, 0, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), > RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x59, }, > + { 500000, 12500, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), > RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x00, 0x4f, 3}, > + { 1500000, 100000, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), > RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x50, 0x58, 3}, > + { 2400000, 0, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), > RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x59, 0x59, 3}, > /* buck 4 */ > - { 500000, 12500, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), > RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x00, }, > - { 1500000, 100000, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), > RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x50, }, > - { 3400000, 0, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), > RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x63, }, > + { 500000, 12500, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), > RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x00, 0x4f, 3}, > + { 1500000, 100000, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), > RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x50, 0x62, 3}, > + { 3400000, 0, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), > RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x63, 0x63, 3}, > }; > > static const struct rk8xx_reg_info rk818_buck[] = { > - { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, }, > - { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, }, > - { 712500, 12500, NA, NA, > REG_BUCK3_CONFIG, RK818_BUCK_VSEL_MASK, }, > - { 1800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, > REG_BUCK4_CONFIG, RK818_BUCK4_VSEL_MASK, }, > + { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, > REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, 0x3f, 1}, > + { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, > REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, 0x3f, 1}, > + { NA, NA, NA, NA, > REG_BUCK3_CONFIG, NA, NA, NA, 1}, > + { 1800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, > REG_BUCK4_CONFIG, RK818_BUCK4_VSEL_MASK, 0x00, 0x10, 1}, > }; > > #ifdef ENABLE_DRIVER > @@ -706,10 +709,9 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, > int ldo) > static int buck_get_value(struct udevice *dev) > { > int buck = dev->driver_data - 1; > - /* We assume level-1 voltage is enough for usage in U-Boot */ > const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck, > 0); > int mask = info->vsel_mask; > - int ret, val; > + int i, ret, val; > > if (info->vsel_reg == NA) > return -ENOSYS; > @@ -717,9 +719,20 @@ static int buck_get_value(struct udevice *dev) > ret = pmic_reg_read(dev->parent, info->vsel_reg); > if (ret < 0) > return ret; > + > val = ret & mask; > + if (val >= info->min_sel && val <= info->max_sel) > + goto finish; > > - return info->min_uv + val * info->step_uv; > + /* unlucky to try */ > + for (i = 1; i < info->range_num; i++) { > + info++; > + if (val <= info->max_sel && val >= info->min_sel) > + break; > + } Can you add a comment here as to what happens when you fall through? > + > +finish: > + return info->min_uv + (val - info->min_sel) * info->step_uv; > } > > static int buck_set_value(struct udevice *dev, int uvolt) > @@ -732,10 +745,9 @@ static int buck_set_value(struct udevice *dev, int uvolt) > static int buck_get_suspend_value(struct udevice *dev) > { > int buck = dev->driver_data - 1; > - /* We assume level-1 voltage is enough for usage in U-Boot */ > const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck, > 0); > int mask = info->vsel_mask; > - int ret, val; > + int i, ret, val; > > if (info->vsel_sleep_reg == NA) > return -ENOSYS; > @@ -745,8 +757,18 @@ static int buck_get_suspend_value(struct udevice *dev) > return ret; > > val = ret & mask; > + if (val <= info->max_sel && val >= info->min_sel) > + goto finish; > > - return info->min_uv + val * info->step_uv; > + /* unlucky to try */ > + for (i = 1; i < info->range_num; i++) { > + info++; It seems you could drop the if() part above and just use this loop, if i started at 0 and you incremented info after the next line? > + if (val <= info->max_sel && val >= info->min_sel) > + break; > + } > + > +finish: > + return info->min_uv + (val - info->min_sel) * info->step_uv; > } > > static int buck_set_suspend_value(struct udevice *dev, int uvolt) > -- > 2.41.0 > Regards, Simon
