Hi, On 13 September 2017 at 14:22, Philipp Tomsich <[email protected]> wrote: > > > On Wed, 13 Sep 2017, Kever Yang wrote: > >> From: Elaine Zhang <[email protected]> >> >> Add Rockchip pmic rk816 support. > > > Could you summarise some of the key-features of the RK816 here? Just a few > words, so we know how it's different from the others and what it's used for > (e.g. "PMIC matching the RK3xxx"). > > >> >> Signed-off-by: Elaine Zhang <[email protected]> >> Signed-off-by: Kever Yang <[email protected]> >> --- >> >> drivers/power/pmic/rk8xx.c | 1 + >> drivers/power/regulator/rk8xx.c | 212 >> ++++++++++++++++++++++++++++++++-------- >> include/power/rk8xx_pmic.h | 9 +- >> 3 files changed, 182 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c >> index eb3ec0f..0fdea95 100644 >> --- a/drivers/power/pmic/rk8xx.c >> +++ b/drivers/power/pmic/rk8xx.c >> @@ -100,6 +100,7 @@ static struct dm_pmic_ops rk8xx_ops = { >> >> static const struct udevice_id rk8xx_ids[] = { >> { .compatible = "rockchip,rk808" }, >> + { .compatible = "rockchip,rk816" }, >> { .compatible = "rockchip,rk818" }, >> { } >> }; >> diff --git a/drivers/power/regulator/rk8xx.c >> b/drivers/power/regulator/rk8xx.c >> index 76fc2ef..cf3566e 100644 >> --- a/drivers/power/regulator/rk8xx.c >> +++ b/drivers/power/regulator/rk8xx.c >> @@ -48,6 +48,21 @@ static const struct rk8xx_reg_info rk808_buck[] = { >> { 1800000, 100000, REG_BUCK4_ON_VSEL, RK808_BUCK4_VSEL_MASK, }, >> }; >> >> +static const struct rk8xx_reg_info rk816_buck[] = { >> + /* buck 1 */ >> + { 712500, 12500, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, }, >> + { 1800000, 200000, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, }, >> + { 2300000, 0, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, }, >> + /* buck 2 */ >> + { 712500, 12500, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, }, >> + { 1800000, 200000, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, }, >> + { 2300000, 0, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, }, >> + /* buck 3 */ >> + { 712500, 12500, -1, RK818_BUCK_VSEL_MASK, }, >> + /* buck 4 */ >> + { 800000, 100000, REG_BUCK4_ON_VSEL, RK818_BUCK4_VSEL_MASK, }, >> +}; >> + >> static const struct rk8xx_reg_info rk818_buck[] = { >> { 712500, 12500, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, }, >> { 712500, 12500, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, }, >> @@ -67,6 +82,15 @@ static const struct rk8xx_reg_info rk808_ldo[] = { >> { 1800000, 100000, REG_LDO8_ON_VSEL, RK808_LDO_VSEL_MASK, }, >> }; >> >> +static const struct rk8xx_reg_info rk816_ldo[] = { >> + { 800000, 100000, REG_LDO1_ON_VSEL, RK818_LDO_VSEL_MASK, }, >> + { 800000, 100000, REG_LDO2_ON_VSEL, RK818_LDO_VSEL_MASK, }, >> + { 800000, 100000, REG_LDO3_ON_VSEL, RK818_LDO_VSEL_MASK, }, >> + { 800000, 100000, REG_LDO4_ON_VSEL, RK818_LDO_VSEL_MASK, }, >> + { 800000, 100000, REG_LDO5_ON_VSEL, RK818_LDO_VSEL_MASK, }, >> + { 800000, 100000, REG_LDO6_ON_VSEL, RK818_LDO_VSEL_MASK, }, >> +}; >> + >> static const struct rk8xx_reg_info rk818_ldo[] = { >> { 1800000, 100000, REG_LDO1_ON_VSEL, RK818_LDO_VSEL_MASK, }, >> { 1800000, 100000, REG_LDO2_ON_VSEL, RK818_LDO_VSEL_MASK, }, >> @@ -88,10 +112,24 @@ static const uint rk818_chrg_shutdown_vsel_array[] = >> { >> }; >> >> static const struct rk8xx_reg_info *get_buck_reg(struct udevice *pmic, >> - int num) >> + int num, int uvolt) >> { >> struct rk8xx_priv *priv = dev_get_priv(pmic); >> + >> switch (priv->variant) { >> + case RK816_ID: >> + switch (num) { >> + case 0: >> + case 1: >> + if (uvolt <= 1450000) >> + return &rk816_buck[num * 3 + 0]; >> + else if (uvolt <= 2200000) >> + return &rk816_buck[num * 3 + 1]; >> + else >> + return &rk816_buck[num * 3 + 2]; >> + default: >> + return &rk816_buck[num + 4]; >> + } > > > I am very concerned with this driver turning into a large switch statement: > this really is not the way driver_data is supposed to be used. > > Can we have a single path through this function and use driver_data to > parameterize the cut-offs? As an alternative, we'll need to start splitting > this into separate drivers.
Yes it is worth considering that. I don't see a good reason to have this new device in the same driver, given all the changes. If there is shared code it could be split out into a new file. Regards, Simon _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
