diff mbox series

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

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

Commit Message

Kever Yang Sept. 13, 2017, 10:04 a.m. UTC 
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. UTC | #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. UTC | #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. UTC | #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
Kever Yang Sept. 27, 2017, 7 a.m. UTC | #4 
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];
>>> +                       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, 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
>
diff mbox series

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,
 };