Re: [U-Boot] [U-Boot,2/5] power: pmic: rk816: support rk816 pmic

Kever Yang Wed, 27 Sep 2017 00:02:24 -0700

Hi Simon, Philipp,


On 09/18/2017 01:52 AM, Simon Glass wrote:

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").

Will add detail difference between these chips, they are very similar asregulator andit's reasonable to make then in one driver instead of use differentdrivers 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/writemore safe.

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.

The framework and the setting for all these chips are the same, exceptthe BUCK/LDOnumbers are different, and rk805/rk816 need extra mask bit forenable/disable.

Abstract the difference into array structure is a good way to share thecode, I don'tthink split this into rk805.c rk808.c rk816.c and rk818.c will have abetter 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



_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [U-Boot,2/5] power: pmic: rk816: support rk816 pmic

Reply via email to