Hi,
On 13 September 2017 at 14:22, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
On Wed, 13 Sep 2017, Kever Yang wrote:
From: Elaine Zhang zhangqing@rock-chips.com
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 zhangqing@rock-chips.com Signed-off-by: Kever Yang kever.yang@rock-chips.com
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];elsereturn &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