[U-Boot] [PATCH v2 1/5] rockchip: i2c: rk3328: support i2c for rk3328 SoC

[U-Boot] [PATCH v2 1/5] rockchip: i2c: rk3328: support i2c for rk3328 SoC

[Kever Yang]

From: Elaine Zhang <zhangqing@rock-chips.com>

Add compatible to support rk3328 i2c

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

Changes in v2: None

 drivers/i2c/rk_i2c.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c
index 68e6653..a051893 100644
--- a/drivers/i2c/rk_i2c.c
+++ b/drivers/i2c/rk_i2c.c
@@ -396,6 +396,7 @@ static const struct udevice_id rockchip_i2c_ids[] = {
 	{ .compatible = "rockchip,rk3066-i2c" },
 	{ .compatible = "rockchip,rk3188-i2c" },
 	{ .compatible = "rockchip,rk3288-i2c" },
+	{ .compatible = "rockchip,rk3328-i2c" },
 	{ .compatible = "rockchip,rk3399-i2c" },
 	{ }
 };
-- 
1.9.1

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

[Kever Yang]

From: Elaine Zhang <zhangqing@rock-chips.com>

Add Rockchip pmic rk816 support.

RK816 have 4 DCDC, 6 LDO, RTC, CHARGER, FUEL GAUGE, GPIO, PWRKEY.
The DCDC and LDO voltage output range and current loading are different
from RK808 and RK818.
RK816 add mask bit for enable/disable for read/write safety.

The RK816 most used with rk312x/rk322x/rk332x SoCs.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

Changes in v2:
- add introduce info for RK816 in commit message

 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];
+		}
 	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,
 };
 
-- 
1.9.1

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

[Philipp Tomsich]

On Wed, 27 Sep 2017, Kever Yang wrote:

> From: Elaine Zhang <zhangqing@rock-chips.com>
>
> Add Rockchip pmic rk816 support.
>
> RK816 have 4 DCDC, 6 LDO, RTC, CHARGER, FUEL GAUGE, GPIO, PWRKEY.
> The DCDC and LDO voltage output range and current loading are different
> from RK808 and RK818.
> RK816 add mask bit for enable/disable for read/write safety.
>
> The RK816 most used with rk312x/rk322x/rk332x SoCs.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>

The changes I requested (https://patchwork.ozlabs.org/patch/813297/) to 
to split this up (so we avoid having different logic in a switch/case for 
every function have not been addressed).

The RK816 apparently is different enough from the RK808 and RK816 to 
warrant a separate DM driver.

Our basic rule-of-thumb for deciding when to split a driver should be 
whether the new device can be added using driverdata containing parameters 
or whether we end up adding new conditional and exclusive code-paths (i.e. 
if there's a conditional statement in the middle that just adds/removes 
one step of processing, when this certainly won't justify splitting a 
driver) to the overwhelming number of the ops.

Please split this to introduce a new driver for the RK816 and keep the 
existing driver as simple as it is today.

> ---
>
> Changes in v2:
> - add introduce info for RK816 in commit message
>
> 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];
> +		}
> 	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,
> };
>
>

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

[Kever Yang]

Hi Philipp,


On 09/28/2017 04:47 PM, Philipp Tomsich wrote:
>
>
> On Wed, 27 Sep 2017, Kever Yang wrote:
>
>> From: Elaine Zhang <zhangqing@rock-chips.com>
>>
>> Add Rockchip pmic rk816 support.
>>
>> RK816 have 4 DCDC, 6 LDO, RTC, CHARGER, FUEL GAUGE, GPIO, PWRKEY.
>> The DCDC and LDO voltage output range and current loading are different
>> from RK808 and RK818.
>> RK816 add mask bit for enable/disable for read/write safety.
>>
>> The RK816 most used with rk312x/rk322x/rk332x SoCs.
>>
>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>
> The changes I requested (https://patchwork.ozlabs.org/patch/813297/) 
> to to split this up (so we avoid having different logic in a 
> switch/case for every function have not been addressed).
>
> The RK816 apparently is different enough from the RK808 and RK816 to 
> warrant a separate DM driver.
>
> Our basic rule-of-thumb for deciding when to split a driver should be 
> whether the new device can be added using driverdata containing 
> parameters or whether we end up adding new conditional and exclusive 
> code-paths (i.e. if there's a conditional statement in the middle that 
> just adds/removes one step of processing, when this certainly won't 
> justify splitting a driver) to the overwhelming number of the ops.
>
> Please split this to introduce a new driver for the RK816 and keep the 
> existing driver as simple as it is today.

I have reply your comment in patch set V1, and explain why we think it's 
reason-able
to make then in one driver instead of two. What you are asking is not 
like to keep code clean and less duplicate code.
I copy my comment here again, I prefer to improve the implementation 
instead of split them into different driver.

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.

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
>
>> ---
>>
>> Changes in v2:
>> - add introduce info for RK816 in commit message
>>
>> 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];
>> +        }
>>     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,
>> };
>>
>>
>

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

[Dr. Philipp Tomsich]

Kever,

> On 28 Sep 2017, at 12:12, Kever Yang <kever.yang@rock-chips.com> wrote:
> 
> Hi Philipp,
> 
> On 09/28/2017 04:47 PM, Philipp Tomsich wrote:
>> 
>> 
>> On Wed, 27 Sep 2017, Kever Yang wrote: 
>> 
>>> From: Elaine Zhang <zhangqing@rock-chips.com> <mailto:zhangqing@rock-chips.com> 
>>> 
>>> Add Rockchip pmic rk816 support. 
>>> 
>>> RK816 have 4 DCDC, 6 LDO, RTC, CHARGER, FUEL GAUGE, GPIO, PWRKEY. 
>>> The DCDC and LDO voltage output range and current loading are different 
>>> from RK808 and RK818. 
>>> RK816 add mask bit for enable/disable for read/write safety. 
>>> 
>>> The RK816 most used with rk312x/rk322x/rk332x SoCs. 
>>> 
>>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com> <mailto:zhangqing@rock-chips.com> 
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> <mailto:kever.yang@rock-chips.com> 
>> 
>> The changes I requested (https://patchwork.ozlabs.org/patch/813297/ <https://patchwork.ozlabs.org/patch/813297/>) to to split this up (so we avoid having different logic in a switch/case for every function have not been addressed). 
>> 
>> The RK816 apparently is different enough from the RK808 and RK816 to warrant a separate DM driver. 
>> 
>> Our basic rule-of-thumb for deciding when to split a driver should be whether the new device can be added using driverdata containing parameters or whether we end up adding new conditional and exclusive code-paths (i.e. if there's a conditional statement in the middle that just adds/removes one step of processing, when this certainly won't justify splitting a driver) to the overwhelming number of the ops. 
>> 
>> Please split this to introduce a new driver for the RK816 and keep the existing driver as simple as it is today. 
> 
> I have reply your comment in patch set V1, and explain why we think it's reason-able
> to make then in one driver instead of two. What you are asking is not like to keep code clean and less duplicate code.
> I copy my comment here again, I prefer to improve the implementation instead of split them into different driver.

I had seen the comment (patchworks picked it up and I keep checking there to make sure I didn’t miss emails).
However, I disagree: see below.

> 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.
> 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.
In these cases, we should have a file containing the common code and then mini-drivers that use them.
I had the same discussion with Simon, when I tried to get the rk3399 hdmi merged—and we finally decided
there on using the model of having small DM-drivers that have the different logic and then using shared
functions for the shared infrastructure

> 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.
The array structure is great (and I very much like how it’s done), but the changed implementation concerns
me. Let’s look at _buck_set_enable as an example…

Initially it had a very clean flow (pseudocode):
{
	if (enable)
		/* set/clear a few bits */

	return /* another op */;
}

Now it has morphed into:
{
	switch (id) {
	case ONE_CHIP:
		if (enable)
			/* set/clear some bits */
		ret = /* another op */
		break;

	case ANOTHER_CHIP:
		if (enable)
			/* set/clear some bits */
		ret = /* other ops */;
		break;

	default:
		/* handle a bad id */
	}

	return ret;
}

This is clearly a case where multiple different functions should be used… and normally another
DM-driver will be called for.

Note that there is also a runtime penalty (the compiler can’t optimise the switch-statemenet away)
and a size penalty (important, as regulator-drivers may be needed as early as TPL).

> BTW, these are all the PMICs we have since early 2013, so don't worry, 
> it won't grow into large switch statement.
That’s good to hear ;-)
However, this is yet another reason why this should be a separate mini-driver…

For the time being, I’ll pick up the remainder of the series for rc1 and we can then sort out how
to deal with this specific driver after the upcoming golden week?

> Thanks,
> - Kever
>> 
>>> --- 
>>> 
>>> Changes in v2: 
>>> - add introduce info for RK816 in commit message 
>>> 
>>> 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]; 
>>> +        } 
>>>     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, 
>>> }; 
>>> 
>>> 
>> 
> 

[U-Boot] [PATCH v2 3/5] power: pmic: rk805: support rk805 pmic

[Kever Yang]

From: Elaine Zhang <zhangqing@rock-chips.com>

RK805 have 4 DCDC, 4 LDO, RTC.
The configuration parameters are the same with RK816.

The RK805 most used with rk312x/rk322x/rk332x products which do not
need charger and fuel guage.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

Changes in v2:
- add introduce info for RK816 in commit message

 drivers/power/pmic/rk8xx.c      | 1 +
 drivers/power/regulator/rk8xx.c | 6 ++++++
 include/power/rk8xx_pmic.h      | 1 +
 3 files changed, 8 insertions(+)

diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
index 0fdea95..f2a2f07 100644
--- a/drivers/power/pmic/rk8xx.c
+++ b/drivers/power/pmic/rk8xx.c
@@ -99,6 +99,7 @@ static struct dm_pmic_ops rk8xx_ops = {
 };
 
 static const struct udevice_id rk8xx_ids[] = {
+	{ .compatible = "rockchip,rk805" },
 	{ .compatible = "rockchip,rk808" },
 	{ .compatible = "rockchip,rk816" },
 	{ .compatible = "rockchip,rk818" },
diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
index cf3566e..6d4a243 100644
--- a/drivers/power/regulator/rk8xx.c
+++ b/drivers/power/regulator/rk8xx.c
@@ -117,6 +117,7 @@ static const struct rk8xx_reg_info *get_buck_reg(struct udevice *pmic,
 	struct rk8xx_priv *priv = dev_get_priv(pmic);
 
 	switch (priv->variant) {
+	case RK805_ID:
 	case RK816_ID:
 		switch (num) {
 		case 0:
@@ -159,6 +160,7 @@ static int _buck_get_enable(struct udevice *pmic, int buck)
 	int ret = 0;
 
 	switch (priv->variant) {
+	case RK805_ID:
 	case RK816_ID:
 		if (buck >= 4) {
 			mask = 1 << (buck - 4);
@@ -187,6 +189,7 @@ static int _buck_set_enable(struct udevice *pmic, int buck, bool enable)
 	struct rk8xx_priv *priv = dev_get_priv(pmic);
 
 	switch (priv->variant) {
+	case RK805_ID:
 	case RK816_ID:
 		if (buck >= 4) {
 			buck -= 4;
@@ -231,6 +234,7 @@ static const struct rk8xx_reg_info *get_ldo_reg(struct udevice *pmic,
 	struct rk8xx_priv *priv = dev_get_priv(pmic);
 
 	switch (priv->variant) {
+	case RK805_ID:
 	case RK816_ID:
 		return &rk816_ldo[num];
 	case RK818_ID:
@@ -247,6 +251,7 @@ static int _ldo_get_enable(struct udevice *pmic, int ldo)
 	int ret = 0;
 
 	switch (priv->variant) {
+	case RK805_ID:
 	case RK816_ID:
 		if (ldo >= 4) {
 			mask = 1 << (ldo - 4);
@@ -275,6 +280,7 @@ static int _ldo_set_enable(struct udevice *pmic, int ldo, bool enable)
 	int ret = 0;
 
 	switch (priv->variant) {
+	case RK805_ID:
 	case RK816_ID:
 		if (ldo >= 4) {
 			ldo -= 4;
diff --git a/include/power/rk8xx_pmic.h b/include/power/rk8xx_pmic.h
index 8e821c3..b1482b7 100644
--- a/include/power/rk8xx_pmic.h
+++ b/include/power/rk8xx_pmic.h
@@ -178,6 +178,7 @@ enum {
 };
 
 enum {
+	RK805_ID = 0x8050,
 	RK808_ID = 0x0000,
 	RK816_ID = 0x8160,
 	RK818_ID = 0x8180,
-- 
1.9.1

[U-Boot] [U-Boot, v2, 3/5] power: pmic: rk805: support rk805 pmic

[Philipp Tomsich]

On Wed, 27 Sep 2017, Kever Yang wrote:

> From: Elaine Zhang <zhangqing@rock-chips.com>
>
> RK805 have 4 DCDC, 4 LDO, RTC.
> The configuration parameters are the same with RK816.
>
> The RK805 most used with rk312x/rk322x/rk332x products which do not
> need charger and fuel guage.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
> Changes in v2:
> - add introduce info for RK816 in commit message
>
> drivers/power/pmic/rk8xx.c      | 1 +
> drivers/power/regulator/rk8xx.c | 6 ++++++
> include/power/rk8xx_pmic.h      | 1 +
> 3 files changed, 8 insertions(+)
>
> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> index 0fdea95..f2a2f07 100644
> --- a/drivers/power/pmic/rk8xx.c
> +++ b/drivers/power/pmic/rk8xx.c
> @@ -99,6 +99,7 @@ static struct dm_pmic_ops rk8xx_ops = {
> };
>
> static const struct udevice_id rk8xx_ids[] = {
> +	{ .compatible = "rockchip,rk805" },
> 	{ .compatible = "rockchip,rk808" },
> 	{ .compatible = "rockchip,rk816" },
> 	{ .compatible = "rockchip,rk818" },
> diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
> index cf3566e..6d4a243 100644
> --- a/drivers/power/regulator/rk8xx.c
> +++ b/drivers/power/regulator/rk8xx.c
> @@ -117,6 +117,7 @@ static const struct rk8xx_reg_info *get_buck_reg(struct udevice *pmic,
> 	struct rk8xx_priv *priv = dev_get_priv(pmic);
>
> 	switch (priv->variant) {
> +	case RK805_ID:
> 	case RK816_ID:
> 		switch (num) {
> 		case 0:
> @@ -159,6 +160,7 @@ static int _buck_get_enable(struct udevice *pmic, int buck)
> 	int ret = 0;
>
> 	switch (priv->variant) {
> +	case RK805_ID:
> 	case RK816_ID:
> 		if (buck >= 4) {
> 			mask = 1 << (buck - 4);
> @@ -187,6 +189,7 @@ static int _buck_set_enable(struct udevice *pmic, int buck, bool enable)
> 	struct rk8xx_priv *priv = dev_get_priv(pmic);
>
> 	switch (priv->variant) {
> +	case RK805_ID:
> 	case RK816_ID:
> 		if (buck >= 4) {
> 			buck -= 4;
> @@ -231,6 +234,7 @@ static const struct rk8xx_reg_info *get_ldo_reg(struct udevice *pmic,
> 	struct rk8xx_priv *priv = dev_get_priv(pmic);
>
> 	switch (priv->variant) {
> +	case RK805_ID:
> 	case RK816_ID:
> 		return &rk816_ldo[num];
> 	case RK818_ID:
> @@ -247,6 +251,7 @@ static int _ldo_get_enable(struct udevice *pmic, int ldo)
> 	int ret = 0;
>
> 	switch (priv->variant) {
> +	case RK805_ID:
> 	case RK816_ID:
> 		if (ldo >= 4) {
> 			mask = 1 << (ldo - 4);
> @@ -275,6 +280,7 @@ static int _ldo_set_enable(struct udevice *pmic, int ldo, bool enable)
> 	int ret = 0;
>
> 	switch (priv->variant) {
> +	case RK805_ID:
> 	case RK816_ID:
> 		if (ldo >= 4) {
> 			ldo -= 4;
> diff --git a/include/power/rk8xx_pmic.h b/include/power/rk8xx_pmic.h
> index 8e821c3..b1482b7 100644
> --- a/include/power/rk8xx_pmic.h
> +++ b/include/power/rk8xx_pmic.h
> @@ -178,6 +178,7 @@ enum {
> };
>
> enum {
> +	RK805_ID = 0x8050,
> 	RK808_ID = 0x0000,
> 	RK816_ID = 0x8160,
> 	RK818_ID = 0x8180,
>

[U-Boot] [U-Boot, v2, 3/5] power: pmic: rk805: support rk805 pmic

[Philipp Tomsich]

> From: Elaine Zhang <zhangqing@rock-chips.com>
> 
> RK805 have 4 DCDC, 4 LDO, RTC.
> The configuration parameters are the same with RK816.
> 
> The RK805 most used with rk312x/rk322x/rk332x products which do not
> need charger and fuel guage.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
> Changes in v2:
> - add introduce info for RK816 in commit message
> 
>  drivers/power/pmic/rk8xx.c      | 1 +
>  drivers/power/regulator/rk8xx.c | 6 ++++++
>  include/power/rk8xx_pmic.h      | 1 +
>  3 files changed, 8 insertions(+)
> 

Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

[U-Boot] [U-Boot, v2, 3/5] power: pmic: rk805: support rk805 pmic

[Philipp Tomsich]

> From: Elaine Zhang <zhangqing@rock-chips.com>
> 
> RK805 have 4 DCDC, 4 LDO, RTC.
> The configuration parameters are the same with RK816.
> 
> The RK805 most used with rk312x/rk322x/rk332x products which do not
> need charger and fuel guage.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
> Changes in v2:
> - add introduce info for RK816 in commit message
> 
>  drivers/power/pmic/rk8xx.c      | 1 +
>  drivers/power/regulator/rk8xx.c | 6 ++++++
>  include/power/rk8xx_pmic.h      | 1 +
>  3 files changed, 8 insertions(+)
> 

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

[U-Boot] [PATCH v2 4/5] configs: rk3328: add support for pmic rk8xx and regulator and i2c driver

[Kever Yang]

From: Elaine Zhang <zhangqing@rock-chips.com>

Add defconfig for rk8xx and regulator and i2c controller.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

Changes in v2: None

 configs/evb-rk3328_defconfig | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/configs/evb-rk3328_defconfig b/configs/evb-rk3328_defconfig
index 7bec001..6d0f3af 100644
--- a/configs/evb-rk3328_defconfig
+++ b/configs/evb-rk3328_defconfig
@@ -22,12 +22,18 @@ CONFIG_REGMAP=y
 CONFIG_SYSCON=y
 CONFIG_CLK=y
 CONFIG_ROCKCHIP_GPIO=y
+CONFIG_SYS_I2C_ROCKCHIP=y
 CONFIG_MMC_DW=y
 CONFIG_MMC_DW_ROCKCHIP=y
 CONFIG_PINCTRL=y
 CONFIG_PINCTRL_ROCKCHIP_RK3328=y
+CONFIG_DM_PMIC=y
+CONFIG_PMIC_RK8XX=y
+CONFIG_DM_REGULATOR=y
 CONFIG_REGULATOR_PWM=y
 CONFIG_DM_REGULATOR_FIXED=y
+CONFIG_REGULATOR_RK8XX=y
+CONFIG_PWM_ROCKCHIP=y
 CONFIG_RAM=y
 CONFIG_BAUDRATE=1500000
 CONFIG_DEBUG_UART_BASE=0xFF130000
-- 
1.9.1

[U-Boot] [U-Boot, v2, 4/5] configs: rk3328: add support for pmic rk8xx and regulator and i2c driver

[Philipp Tomsich]

> From: Elaine Zhang <zhangqing@rock-chips.com>
> 
> Add defconfig for rk8xx and regulator and i2c controller.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
> 
> Changes in v2: None
> 
>  configs/evb-rk3328_defconfig | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH v2 5/5] rockchip: dts: rk3328-evb: add i2c1 and rk805 nodes

[Kever Yang]

From: Elaine Zhang <zhangqing@rock-chips.com>

add i2c1 and rk805 nodes to support rk805 init setting.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

Changes in v2: None

 arch/arm/dts/rk3328-evb.dts | 118 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/arch/arm/dts/rk3328-evb.dts b/arch/arm/dts/rk3328-evb.dts
index 8a14c65..f71690f 100644
--- a/arch/arm/dts/rk3328-evb.dts
+++ b/arch/arm/dts/rk3328-evb.dts
@@ -87,3 +87,121 @@
 	vbus-supply = <&vcc5v0_host_xhci>;
 	status = "okay";
 };
+
+&i2c1 {
+	clock-frequency = <400000>;
+	i2c-scl-rising-time-ns = <168>;
+	i2c-scl-falling-time-ns = <4>;
+	status = "okay";
+
+	rk805: pmic at 18 {
+		compatible = "rockchip,rk805";
+		status = "okay";
+		reg = <0x18>;
+		interrupt-parent = <&gpio2>;
+		interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pmic_int_l>;
+		rockchip,system-power-controller;
+		wakeup-source;
+		gpio-controller;
+		#gpio-cells = <2>;
+		#clock-cells = <1>;
+		clock-output-names = "xin32k", "rk805-clkout2";
+
+		regulators {
+			vdd_logic: DCDC_REG1 {
+				regulator-name = "vdd_logic";
+				regulator-min-microvolt = <712500>;
+				regulator-max-microvolt = <1450000>;
+				regulator-ramp-delay = <6001>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1000000>;
+				};
+			};
+
+			vdd_arm: DCDC_REG2 {
+				regulator-name = "vdd_arm";
+				regulator-min-microvolt = <712500>;
+				regulator-max-microvolt = <1450000>;
+				regulator-ramp-delay = <6001>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1000000>;
+				};
+			};
+
+			vcc_ddr: DCDC_REG3 {
+				regulator-name = "vcc_ddr";
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc_io: DCDC_REG4 {
+				regulator-name = "vcc_io";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3300000>;
+				};
+			};
+
+			vdd_18: LDO_REG1 {
+				regulator-name = "vdd_18";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vcc_18emmc: LDO_REG2 {
+				regulator-name = "vcc_18emmc";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vdd_10: LDO_REG3 {
+				regulator-name = "vdd_10";
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <1000000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1000000>;
+				};
+			};
+		};
+	};
+};
+
+&pinctrl {
+	pmic {
+		pmic_int_l: pmic-int-l {
+		rockchip,pins =
+			<2 RK_PA6 RK_FUNC_GPIO &pcfg_pull_up>;	/* gpio2_a6 */
+		};
+	};
+};
+
-- 
1.9.1

[U-Boot] [U-Boot, v2, 5/5] rockchip: dts: rk3328-evb: add i2c1 and rk805 nodes

[Philipp Tomsich]

> From: Elaine Zhang <zhangqing@rock-chips.com>
> 
> add i2c1 and rk805 nodes to support rk805 init setting.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
> 
> Changes in v2: None
> 
>  arch/arm/dts/rk3328-evb.dts | 118 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
> 

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH v2 1/5] rockchip: i2c: rk3328: support i2c for rk3328 SoC

[Heiko Schocher]

Hello Yang,

Am 27.09.2017 um 09:23 schrieb Kever Yang:
> From: Elaine Zhang <zhangqing@rock-chips.com>
>
> Add compatible to support rk3328 i2c
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> Changes in v2: None
>
>   drivers/i2c/rk_i2c.c | 1 +
>   1 file changed, 1 insertion(+)

Acked-by: Heiko Schocher<hs@denx.de>

bye,
Heiko Schocher
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [U-Boot, v2, 1/5] rockchip: i2c: rk3328: support i2c for rk3328 SoC

[Philipp Tomsich]

> From: Elaine Zhang <zhangqing@rock-chips.com>
> 
> Add compatible to support rk3328 i2c
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Acked-by: Heiko Schocher<hs@denx.de>
> ---
> 
> Changes in v2: None
> 
>  drivers/i2c/rk_i2c.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Applied to u-boot-rockchip, thanks!