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

Message ID 1505297094-5273-2-git-send-email-kever.yang@rock-chips.com
State New
Delegated to: Philipp Tomsich
Headers show
Series
  • [U-Boot,1/5] rockchip: i2c: rk3328: support i2c for rk3328 SoC
Related show

Commit Message

Kever Yang Sept. 13, 2017, 10:04 a.m.
From: Elaine Zhang <zhangqing@rock-chips.com>

Add Rockchip pmic rk816 support.

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(-)

Comments

Philipp Tomsich Sept. 13, 2017, 8:07 p.m. | #1
> From: Elaine Zhang <zhangqing@rock-chips.com>
> 
> Add Rockchip pmic rk816 support.
> 
> 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(-)
> 

Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Philipp Tomsich Sept. 13, 2017, 8:22 p.m. | #2
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];
> +			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.

> 	case RK818_ID:
> 		return &rk818_buck[num];
> 	default:
> @@ -101,7 +139,7 @@ static const struct rk8xx_reg_info *get_buck_reg(struct udevice *pmic,
>
> static int _buck_set_value(struct udevice *pmic, int buck, int uvolt)
> {
> -	const struct rk8xx_reg_info *info = get_buck_reg(pmic, buck - 1);
> +	const struct rk8xx_reg_info *info = get_buck_reg(pmic, buck, uvolt);
> 	int mask = info->vsel_mask;
> 	int val;
>
> @@ -114,23 +152,76 @@ static int _buck_set_value(struct udevice *pmic, int buck, int uvolt)
> 	return pmic_clrsetbits(pmic, info->vsel_reg, mask, val);
> }
>
> +static int _buck_get_enable(struct udevice *pmic, int buck)
> +{
> +	struct rk8xx_priv *priv = dev_get_priv(pmic);
> +	uint mask = 0;
> +	int ret = 0;
> +
> +	switch (priv->variant) {
> +	case RK816_ID:
> +		if (buck >= 4) {
> +			mask = 1 << (buck - 4);
> +			ret = pmic_reg_read(pmic, RK816_REG_DCDC_EN2);
> +		} else {
> +			mask = 1 << buck;
> +			ret = pmic_reg_read(pmic, RK816_REG_DCDC_EN1);
> +		}
> +		break;
> +	case RK808_ID:
> +	case RK818_ID:
> +		mask = 1 << buck;
> +		ret = pmic_reg_read(pmic, REG_DCDC_EN);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	}
> +	return ret & mask ? true : false;
> +}

Again, the RK816 appears to be a significantly different device.

> +
> +
> static int _buck_set_enable(struct udevice *pmic, int buck, bool enable)
> {
> -	uint mask;
> +	uint mask, value, en_reg;
> 	int ret;
> +	struct rk8xx_priv *priv = dev_get_priv(pmic);
>
> -	buck--;
> -	mask = 1 << buck;
> -	if (enable) {
> -		ret = pmic_clrsetbits(pmic, REG_DCDC_ILMAX, 0, 3 << (buck * 2));
> -		if (ret)
> -			return ret;
> -		ret = pmic_clrsetbits(pmic, REG_DCDC_UV_ACT, 1 << buck, 0);
> -		if (ret)
> -			return ret;
> +	switch (priv->variant) {
> +	case RK816_ID:
> +		if (buck >= 4) {
> +			buck -= 4;
> +			en_reg = RK816_REG_DCDC_EN2;
> +		} else {
> +			en_reg = RK816_REG_DCDC_EN1;
> +		}
> +		if (enable)
> +			value = ((1 << buck) | (1 << (buck + 4)));
> +		else
> +			value = ((0 << buck) | (1 << (buck + 4)));
> +		ret = pmic_reg_write(pmic, en_reg, value);
> +		break;
> +
> +	case RK808_ID:
> +	case RK818_ID:
> +		mask = 1 << buck;
> +		if (enable) {
> +			ret = pmic_clrsetbits(pmic, REG_DCDC_ILMAX,
> +					      0, 3 << (buck * 2));
> +			if (ret)
> +				return ret;
> +			ret = pmic_clrsetbits(pmic, REG_DCDC_UV_ACT,
> +					      1 << buck, 0);
> +			if (ret)
> +				return ret;
> +		}
> +		ret = pmic_clrsetbits(pmic, REG_DCDC_EN, mask,
> +				      enable ? mask : 0);
> +		break;
> +	default:
> +		ret = -EINVAL;
> 	}
>
> -	return pmic_clrsetbits(pmic, REG_DCDC_EN, mask, enable ? mask : 0);
> +	return ret;
> }
>
> #ifdef ENABLE_DRIVER

This define doesn't make any sense: I see that it's always been here, but 
still: it is just a wrapper around a "#ifndef CONFIG_SPL_BUILD". Can we 
use the #ifndef CONFIG_SPL_BUILD directly?

> @@ -138,7 +229,10 @@ static const struct rk8xx_reg_info *get_ldo_reg(struct udevice *pmic,
> 					     int num)
> {
> 	struct rk8xx_priv *priv = dev_get_priv(pmic);
> +
> 	switch (priv->variant) {
> +	case RK816_ID:
> +		return &rk816_ldo[num];
> 	case RK818_ID:
> 		return &rk818_ldo[num];
> 	default:
> @@ -146,10 +240,70 @@ static const struct rk8xx_reg_info *get_ldo_reg(struct udevice *pmic,
> 	}
> }
>
> +static int _ldo_get_enable(struct udevice *pmic, int ldo)
> +{
> +	struct rk8xx_priv *priv = dev_get_priv(pmic);
> +	uint mask = 0;
> +	int ret = 0;
> +
> +	switch (priv->variant) {
> +	case RK816_ID:
> +		if (ldo >= 4) {
> +			mask = 1 << (ldo - 4);
> +			ret = pmic_reg_read(pmic, RK816_REG_LDO_EN2);
> +		} else {
> +			mask = 1 << ldo;
> +			ret = pmic_reg_read(pmic, RK816_REG_LDO_EN1);
> +		}

Same comment as above.

> +		break;
> +	case RK808_ID:
> +	case RK818_ID:
> +		mask = 1 << ldo;
> +		ret = pmic_reg_read(pmic, REG_LDO_EN);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	}
> +	return ret & mask ? true : false;
> +}
> +
> +
> +static int _ldo_set_enable(struct udevice *pmic, int ldo, bool enable)
> +{
> +	struct rk8xx_priv *priv = dev_get_priv(pmic);
> +	uint mask, value, en_reg;
> +	int ret = 0;
> +
> +	switch (priv->variant) {
> +	case RK816_ID:
> +		if (ldo >= 4) {
> +			ldo -= 4;
> +			en_reg = RK816_REG_LDO_EN2;
> +		} else {
> +			en_reg = RK816_REG_LDO_EN1;
> +		}
> +		if (enable)
> +			value = ((1 << ldo) | (1 << (ldo + 4)));
> +		else
> +			value = ((0 << ldo) | (1 << (ldo + 4)));
> +
> +		ret = pmic_reg_write(pmic, en_reg, value);
> +		break;

Again, it looks as if this should be a separate driver.

> +	case RK808_ID:
> +	case RK818_ID:
> +		mask = 1 << ldo;
> +		ret = pmic_clrsetbits(pmic, REG_LDO_EN, mask,
> +				       enable ? mask : 0);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> static int buck_get_value(struct udevice *dev)
> {
> 	int buck = dev->driver_data - 1;
> -	const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck);
> +	const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck, 0);
> 	int mask = info->vsel_mask;
> 	int ret, val;
>
> @@ -165,14 +319,14 @@ static int buck_get_value(struct udevice *dev)
>
> static int buck_set_value(struct udevice *dev, int uvolt)
> {
> -	int buck = dev->driver_data;
> +	int buck = dev->driver_data - 1;
>
> 	return _buck_set_value(dev->parent, buck, uvolt);
> }
>
> static int buck_set_enable(struct udevice *dev, bool enable)
> {
> -	int buck = dev->driver_data;
> +	int buck = dev->driver_data - 1;
>
> 	return _buck_set_enable(dev->parent, buck, enable);
> }
> @@ -180,16 +334,8 @@ static int buck_set_enable(struct udevice *dev, bool enable)
> static int buck_get_enable(struct udevice *dev)
> {
> 	int buck = dev->driver_data - 1;
> -	int ret;
> -	uint mask;
> -
> -	mask = 1 << buck;
>
> -	ret = pmic_reg_read(dev->parent, REG_DCDC_EN);
> -	if (ret < 0)
> -		return ret;
> -
> -	return ret & mask ? true : false;
> +	return _buck_get_enable(dev->parent, buck);
> }
>
> static int ldo_get_value(struct udevice *dev)
> @@ -228,27 +374,15 @@ static int ldo_set_value(struct udevice *dev, int uvolt)
> static int ldo_set_enable(struct udevice *dev, bool enable)
> {
> 	int ldo = dev->driver_data - 1;
> -	uint mask;
> -
> -	mask = 1 << ldo;
>
> -	return pmic_clrsetbits(dev->parent, REG_LDO_EN, mask,
> -			       enable ? mask : 0);
> +	return _ldo_set_enable(dev->parent, ldo, enable);
> }
>
> static int ldo_get_enable(struct udevice *dev)
> {
> 	int ldo = dev->driver_data - 1;
> -	int ret;
> -	uint mask;
> -
> -	mask = 1 << ldo;
>
> -	ret = pmic_reg_read(dev->parent, REG_LDO_EN);
> -	if (ret < 0)
> -		return ret;
> -
> -	return ret & mask ? true : false;
> +	return _ldo_get_enable(dev->parent, ldo);
> }
>
> static int switch_set_enable(struct udevice *dev, bool enable)
> diff --git a/include/power/rk8xx_pmic.h b/include/power/rk8xx_pmic.h
> index 47a6b36..8e821c3 100644
> --- a/include/power/rk8xx_pmic.h
> +++ b/include/power/rk8xx_pmic.h
> @@ -171,8 +171,15 @@ enum {
> };
>
> enum {
> -	RK805_ID = 0x8050,
> +	RK816_REG_DCDC_EN1 = 0x23,
> +	RK816_REG_DCDC_EN2,
> +	RK816_REG_LDO_EN1 = 0x27,
> +	RK816_REG_LDO_EN2,
> +};
> +
> +enum {
> 	RK808_ID = 0x0000,
> +	RK816_ID = 0x8160,
> 	RK818_ID = 0x8180,
> };
>
>
Simon Glass Sept. 17, 2017, 5:52 p.m. | #3
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];
>> +                       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

Patch

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];
+		}
 	case RK818_ID:
 		return &rk818_buck[num];
 	default:
@@ -101,7 +139,7 @@  static const struct rk8xx_reg_info *get_buck_reg(struct udevice *pmic,
 
 static int _buck_set_value(struct udevice *pmic, int buck, int uvolt)
 {
-	const struct rk8xx_reg_info *info = get_buck_reg(pmic, buck - 1);
+	const struct rk8xx_reg_info *info = get_buck_reg(pmic, buck, uvolt);
 	int mask = info->vsel_mask;
 	int val;
 
@@ -114,23 +152,76 @@  static int _buck_set_value(struct udevice *pmic, int buck, int uvolt)
 	return pmic_clrsetbits(pmic, info->vsel_reg, mask, val);
 }
 
+static int _buck_get_enable(struct udevice *pmic, int buck)
+{
+	struct rk8xx_priv *priv = dev_get_priv(pmic);
+	uint mask = 0;
+	int ret = 0;
+
+	switch (priv->variant) {
+	case RK816_ID:
+		if (buck >= 4) {
+			mask = 1 << (buck - 4);
+			ret = pmic_reg_read(pmic, RK816_REG_DCDC_EN2);
+		} else {
+			mask = 1 << buck;
+			ret = pmic_reg_read(pmic, RK816_REG_DCDC_EN1);
+		}
+		break;
+	case RK808_ID:
+	case RK818_ID:
+		mask = 1 << buck;
+		ret = pmic_reg_read(pmic, REG_DCDC_EN);
+		if (ret < 0)
+			return ret;
+		break;
+	}
+	return ret & mask ? true : false;
+}
+
+
 static int _buck_set_enable(struct udevice *pmic, int buck, bool enable)
 {
-	uint mask;
+	uint mask, value, en_reg;
 	int ret;
+	struct rk8xx_priv *priv = dev_get_priv(pmic);
 
-	buck--;
-	mask = 1 << buck;
-	if (enable) {
-		ret = pmic_clrsetbits(pmic, REG_DCDC_ILMAX, 0, 3 << (buck * 2));
-		if (ret)
-			return ret;
-		ret = pmic_clrsetbits(pmic, REG_DCDC_UV_ACT, 1 << buck, 0);
-		if (ret)
-			return ret;
+	switch (priv->variant) {
+	case RK816_ID:
+		if (buck >= 4) {
+			buck -= 4;
+			en_reg = RK816_REG_DCDC_EN2;
+		} else {
+			en_reg = RK816_REG_DCDC_EN1;
+		}
+		if (enable)
+			value = ((1 << buck) | (1 << (buck + 4)));
+		else
+			value = ((0 << buck) | (1 << (buck + 4)));
+		ret = pmic_reg_write(pmic, en_reg, value);
+		break;
+
+	case RK808_ID:
+	case RK818_ID:
+		mask = 1 << buck;
+		if (enable) {
+			ret = pmic_clrsetbits(pmic, REG_DCDC_ILMAX,
+					      0, 3 << (buck * 2));
+			if (ret)
+				return ret;
+			ret = pmic_clrsetbits(pmic, REG_DCDC_UV_ACT,
+					      1 << buck, 0);
+			if (ret)
+				return ret;
+		}
+		ret = pmic_clrsetbits(pmic, REG_DCDC_EN, mask,
+				      enable ? mask : 0);
+		break;
+	default:
+		ret = -EINVAL;
 	}
 
-	return pmic_clrsetbits(pmic, REG_DCDC_EN, mask, enable ? mask : 0);
+	return ret;
 }
 
 #ifdef ENABLE_DRIVER
@@ -138,7 +229,10 @@  static const struct rk8xx_reg_info *get_ldo_reg(struct udevice *pmic,
 					     int num)
 {
 	struct rk8xx_priv *priv = dev_get_priv(pmic);
+
 	switch (priv->variant) {
+	case RK816_ID:
+		return &rk816_ldo[num];
 	case RK818_ID:
 		return &rk818_ldo[num];
 	default:
@@ -146,10 +240,70 @@  static const struct rk8xx_reg_info *get_ldo_reg(struct udevice *pmic,
 	}
 }
 
+static int _ldo_get_enable(struct udevice *pmic, int ldo)
+{
+	struct rk8xx_priv *priv = dev_get_priv(pmic);
+	uint mask = 0;
+	int ret = 0;
+
+	switch (priv->variant) {
+	case RK816_ID:
+		if (ldo >= 4) {
+			mask = 1 << (ldo - 4);
+			ret = pmic_reg_read(pmic, RK816_REG_LDO_EN2);
+		} else {
+			mask = 1 << ldo;
+			ret = pmic_reg_read(pmic, RK816_REG_LDO_EN1);
+		}
+		break;
+	case RK808_ID:
+	case RK818_ID:
+		mask = 1 << ldo;
+		ret = pmic_reg_read(pmic, REG_LDO_EN);
+		if (ret < 0)
+			return ret;
+		break;
+	}
+	return ret & mask ? true : false;
+}
+
+
+static int _ldo_set_enable(struct udevice *pmic, int ldo, bool enable)
+{
+	struct rk8xx_priv *priv = dev_get_priv(pmic);
+	uint mask, value, en_reg;
+	int ret = 0;
+
+	switch (priv->variant) {
+	case RK816_ID:
+		if (ldo >= 4) {
+			ldo -= 4;
+			en_reg = RK816_REG_LDO_EN2;
+		} else {
+			en_reg = RK816_REG_LDO_EN1;
+		}
+		if (enable)
+			value = ((1 << ldo) | (1 << (ldo + 4)));
+		else
+			value = ((0 << ldo) | (1 << (ldo + 4)));
+
+		ret = pmic_reg_write(pmic, en_reg, value);
+		break;
+	case RK808_ID:
+	case RK818_ID:
+		mask = 1 << ldo;
+		ret = pmic_clrsetbits(pmic, REG_LDO_EN, mask,
+				       enable ? mask : 0);
+		break;
+	}
+
+	return ret;
+}
+
 static int buck_get_value(struct udevice *dev)
 {
 	int buck = dev->driver_data - 1;
-	const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck);
+	const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck, 0);
 	int mask = info->vsel_mask;
 	int ret, val;
 
@@ -165,14 +319,14 @@  static int buck_get_value(struct udevice *dev)
 
 static int buck_set_value(struct udevice *dev, int uvolt)
 {
-	int buck = dev->driver_data;
+	int buck = dev->driver_data - 1;
 
 	return _buck_set_value(dev->parent, buck, uvolt);
 }
 
 static int buck_set_enable(struct udevice *dev, bool enable)
 {
-	int buck = dev->driver_data;
+	int buck = dev->driver_data - 1;
 
 	return _buck_set_enable(dev->parent, buck, enable);
 }
@@ -180,16 +334,8 @@  static int buck_set_enable(struct udevice *dev, bool enable)
 static int buck_get_enable(struct udevice *dev)
 {
 	int buck = dev->driver_data - 1;
-	int ret;
-	uint mask;
-
-	mask = 1 << buck;
 
-	ret = pmic_reg_read(dev->parent, REG_DCDC_EN);
-	if (ret < 0)
-		return ret;
-
-	return ret & mask ? true : false;
+	return _buck_get_enable(dev->parent, buck);
 }
 
 static int ldo_get_value(struct udevice *dev)
@@ -228,27 +374,15 @@  static int ldo_set_value(struct udevice *dev, int uvolt)
 static int ldo_set_enable(struct udevice *dev, bool enable)
 {
 	int ldo = dev->driver_data - 1;
-	uint mask;
-
-	mask = 1 << ldo;
 
-	return pmic_clrsetbits(dev->parent, REG_LDO_EN, mask,
-			       enable ? mask : 0);
+	return _ldo_set_enable(dev->parent, ldo, enable);
 }
 
 static int ldo_get_enable(struct udevice *dev)
 {
 	int ldo = dev->driver_data - 1;
-	int ret;
-	uint mask;
-
-	mask = 1 << ldo;
 
-	ret = pmic_reg_read(dev->parent, REG_LDO_EN);
-	if (ret < 0)
-		return ret;
-
-	return ret & mask ? true : false;
+	return _ldo_get_enable(dev->parent, ldo);
 }
 
 static int switch_set_enable(struct udevice *dev, bool enable)
diff --git a/include/power/rk8xx_pmic.h b/include/power/rk8xx_pmic.h
index 47a6b36..8e821c3 100644
--- a/include/power/rk8xx_pmic.h
+++ b/include/power/rk8xx_pmic.h
@@ -171,8 +171,15 @@  enum {
 };
 
 enum {
-	RK805_ID = 0x8050,
+	RK816_REG_DCDC_EN1 = 0x23,
+	RK816_REG_DCDC_EN2,
+	RK816_REG_LDO_EN1 = 0x27,
+	RK816_REG_LDO_EN2,
+};
+
+enum {
 	RK808_ID = 0x0000,
+	RK816_ID = 0x8160,
 	RK818_ID = 0x8180,
 };