Hi Simon, Philipp,
On 09/18/2017 01:52 AM, Simon Glass wrote:
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").
Will add detail difference between these chips, they are very similar as regulator and it's reasonable to make then in one driver instead of use different drivers for them: - different DCDC/LDO numbers; - different voltage output range and current loading; - RK816 and RK805 add mask bit for enable/disable to make read/write more safe.
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.
The framework and the setting for all these chips are the same, except the BUCK/LDO numbers are different, and rk805/rk816 need extra mask bit for enable/disable.
Abstract the difference into array structure is a good way to share the code, I don't think split this into rk805.c rk808.c rk816.c and rk818.c will have a better shape.
BTW, these are all the PMICs we have since early 2013, so don't worry, it won't grow into large switch statement.
Thanks, - Kever
Regards, Simon