[PATCH 1/4] regulator: core: Allow specify apply_[reg|bit] for regmap based voltage_sel operations

[PATCH 1/4] regulator: core: Allow specify apply_[reg|bit] for regmap based voltage_sel operations

[Axel Lin]

Some DVM regulators needs to update apply_bit after setting vsel_reg to
initiate voltage change on the output.  This patch adds apply_reg and
apply_bit to struct regulator_desc and update
regulator_set_voltage_sel_regmap() to set apply_bit of apply_reg when
apply_bit is set.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/core.c         |   12 +++++++++++-
 include/linux/regulator/driver.h |    6 ++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a35858e..9afcdda3 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2074,10 +2074,20 @@ EXPORT_SYMBOL_GPL(regulator_get_voltage_sel_regmap);
  */
 int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel)
 {
+	int ret;
+
 	sel <<= ffs(rdev->desc->vsel_mask) - 1;
 
-	return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
+	ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
 				  rdev->desc->vsel_mask, sel);
+	if (ret)
+		return ret;
+
+	if (rdev->desc->apply_bit)
+		ret = regmap_update_bits(rdev->regmap, rdev->desc->apply_reg,
+					 rdev->desc->apply_bit,
+					 rdev->desc->apply_bit);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_sel_regmap);
 
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index d10bb0f..23070fd 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -193,6 +193,10 @@ enum regulator_type {
  *
  * @vsel_reg: Register for selector when using regulator_regmap_X_voltage_
  * @vsel_mask: Mask for register bitfield used for selector
+ * @apply_reg: Register for initiate voltage change on the output when
+ *                using regulator_set_voltage_sel_regmap
+ * @apply_bit: Register bitfield used for initiate voltage change on the
+ *                output when using regulator_set_voltage_sel_regmap
  * @enable_reg: Register for control when using regmap enable/disable ops
  * @enable_mask: Mask for control when using regmap enable/disable ops
  *
@@ -218,6 +222,8 @@ struct regulator_desc {
 
 	unsigned int vsel_reg;
 	unsigned int vsel_mask;
+	unsigned int apply_reg;
+	unsigned int apply_bit;
 	unsigned int enable_reg;
 	unsigned int enable_mask;
 	unsigned int bypass_reg;
-- 
1.7.9.5

[PATCH 2/4] regulator: 88pm8607: Use apply_[reg|bit] with regmap based voltage_sel operations

[Axel Lin]

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/88pm8607.c |   43 ++++++++++++------------------------------
 1 file changed, 12 insertions(+), 31 deletions(-)

diff --git a/drivers/regulator/88pm8607.c b/drivers/regulator/88pm8607.c
index 2b55711..9ebdf8c 100644
--- a/drivers/regulator/88pm8607.c
+++ b/drivers/regulator/88pm8607.c
@@ -30,8 +30,6 @@ struct pm8607_regulator_info {
 	unsigned int	*vol_table;
 	unsigned int	*vol_suspend;
 
-	int	update_reg;
-	int	update_bit;
 	int	slope_double;
 };
 
@@ -222,29 +220,6 @@ static int pm8607_list_voltage(struct regulator_dev *rdev, unsigned index)
 	return ret;
 }
 
-static int pm8607_set_voltage_sel(struct regulator_dev *rdev, unsigned selector)
-{
-	struct pm8607_regulator_info *info = rdev_get_drvdata(rdev);
-	uint8_t val;
-	int ret;
-
-	val = (uint8_t)(selector << (ffs(rdev->desc->vsel_mask) - 1));
-
-	ret = pm860x_set_bits(info->i2c, rdev->desc->vsel_reg,
-			      rdev->desc->vsel_mask, val);
-	if (ret)
-		return ret;
-	switch (info->desc.id) {
-	case PM8607_ID_BUCK1:
-	case PM8607_ID_BUCK3:
-		ret = pm860x_set_bits(info->i2c, info->update_reg,
-				      1 << info->update_bit,
-				      1 << info->update_bit);
-		break;
-	}
-	return ret;
-}
-
 static int pm8606_preg_enable(struct regulator_dev *rdev)
 {
 	struct pm8607_regulator_info *info = rdev_get_drvdata(rdev);
@@ -276,7 +251,7 @@ static int pm8606_preg_is_enabled(struct regulator_dev *rdev)
 
 static struct regulator_ops pm8607_regulator_ops = {
 	.list_voltage	= pm8607_list_voltage,
-	.set_voltage_sel = pm8607_set_voltage_sel,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -313,11 +288,11 @@ static struct regulator_ops pm8606_preg_ops = {
 		.n_voltages = ARRAY_SIZE(vreg##_table),			\
 		.vsel_reg = PM8607_##vreg,				\
 		.vsel_mask = ARRAY_SIZE(vreg##_table) - 1,		\
+		.apply_reg = PM8607_##ureg,				\
+		.apply_bit = (ubit),					\
 		.enable_reg = PM8607_##ereg,				\
 		.enable_mask = 1 << (ebit),				\
 	},								\
-	.update_reg	= PM8607_##ureg,				\
-	.update_bit	= (ubit),					\
 	.slope_double	= (0),						\
 	.vol_table	= (unsigned int *)&vreg##_table,		\
 	.vol_suspend	= (unsigned int *)&vreg##_suspend_table,	\
@@ -343,9 +318,15 @@ static struct regulator_ops pm8606_preg_ops = {
 }
 
 static struct pm8607_regulator_info pm8607_regulator_info[] = {
-	PM8607_DVC(BUCK1, GO, 0, SUPPLIES_EN11, 0),
-	PM8607_DVC(BUCK2, GO, 1, SUPPLIES_EN11, 1),
-	PM8607_DVC(BUCK3, GO, 2, SUPPLIES_EN11, 2),
+	PM8607_DVC(BUCK1, GO, BIT(0), SUPPLIES_EN11, 0),
+	/*
+	 * BUCK2 is designed to control voltage of CP domain.
+	 * Linux only runs in AP side, so we don't need to control BUCK2 in AP
+	 * side. Thus set the apply bit to be 0 instead of BIT(1) to avoid
+	 * updating apply_bit for BUCK2.
+	 */
+	PM8607_DVC(BUCK2, GO, 0, SUPPLIES_EN11, 1),
+	PM8607_DVC(BUCK3, GO, BIT(2), SUPPLIES_EN11, 2),
 
 	PM8607_LDO(1,         LDO1, 0, SUPPLIES_EN11, 3),
 	PM8607_LDO(2,         LDO2, 0, SUPPLIES_EN11, 4),
-- 
1.7.9.5

[PATCH 3/4] regulator: da9052: Use apply_[reg|bit] with regmap based voltage_sel operations

[Axel Lin]

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/da9052-regulator.c |   41 +++++-----------------------------
 1 file changed, 6 insertions(+), 35 deletions(-)

diff --git a/drivers/regulator/da9052-regulator.c b/drivers/regulator/da9052-regulator.c
index d096309..c6d8651 100644
--- a/drivers/regulator/da9052-regulator.c
+++ b/drivers/regulator/da9052-regulator.c
@@ -70,7 +70,6 @@ struct da9052_regulator_info {
 	int step_uV;
 	int min_uV;
 	int max_uV;
-	unsigned char activate_bit;
 };
 
 struct da9052_regulator {
@@ -210,36 +209,6 @@ static int da9052_map_voltage(struct regulator_dev *rdev,
 	return sel;
 }
 
-static int da9052_regulator_set_voltage_sel(struct regulator_dev *rdev,
-					    unsigned int selector)
-{
-	struct da9052_regulator *regulator = rdev_get_drvdata(rdev);
-	struct da9052_regulator_info *info = regulator->info;
-	int id = rdev_get_id(rdev);
-	int ret;
-
-	ret = da9052_reg_update(regulator->da9052, rdev->desc->vsel_reg,
-				rdev->desc->vsel_mask, selector);
-	if (ret < 0)
-		return ret;
-
-	/* Some LDOs and DCDCs are DVC controlled which requires enabling of
-	 * the activate bit to implment the changes on the output.
-	 */
-	switch (id) {
-	case DA9052_ID_BUCK1:
-	case DA9052_ID_BUCK2:
-	case DA9052_ID_BUCK3:
-	case DA9052_ID_LDO2:
-	case DA9052_ID_LDO3:
-		ret = da9052_reg_update(regulator->da9052, DA9052_SUPPLY_REG,
-					info->activate_bit, info->activate_bit);
-		break;
-	}
-
-	return ret;
-}
-
 static struct regulator_ops da9052_dcdc_ops = {
 	.get_current_limit = da9052_dcdc_get_current_limit,
 	.set_current_limit = da9052_dcdc_set_current_limit,
@@ -247,7 +216,7 @@ static struct regulator_ops da9052_dcdc_ops = {
 	.list_voltage = da9052_list_voltage,
 	.map_voltage = da9052_map_voltage,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
-	.set_voltage_sel = da9052_regulator_set_voltage_sel,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -257,7 +226,7 @@ static struct regulator_ops da9052_ldo_ops = {
 	.list_voltage = da9052_list_voltage,
 	.map_voltage = da9052_map_voltage,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
-	.set_voltage_sel = da9052_regulator_set_voltage_sel,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -274,13 +243,14 @@ static struct regulator_ops da9052_ldo_ops = {
 		.owner = THIS_MODULE,\
 		.vsel_reg = DA9052_BUCKCORE_REG + DA9052_ID_##_id, \
 		.vsel_mask = (1 << (sbits)) - 1,\
+		.apply_reg = DA9052_SUPPLY_REG, \
+		.apply_bit = (abits), \
 		.enable_reg = DA9052_BUCKCORE_REG + DA9052_ID_##_id, \
 		.enable_mask = 1 << (ebits),\
 	},\
 	.min_uV = (min) * 1000,\
 	.max_uV = (max) * 1000,\
 	.step_uV = (step) * 1000,\
-	.activate_bit = (abits),\
 }
 
 #define DA9052_DCDC(_id, step, min, max, sbits, ebits, abits) \
@@ -294,13 +264,14 @@ static struct regulator_ops da9052_ldo_ops = {
 		.owner = THIS_MODULE,\
 		.vsel_reg = DA9052_BUCKCORE_REG + DA9052_ID_##_id, \
 		.vsel_mask = (1 << (sbits)) - 1,\
+		.apply_reg = DA9052_SUPPLY_REG, \
+		.apply_bit = (abits), \
 		.enable_reg = DA9052_BUCKCORE_REG + DA9052_ID_##_id, \
 		.enable_mask = 1 << (ebits),\
 	},\
 	.min_uV = (min) * 1000,\
 	.max_uV = (max) * 1000,\
 	.step_uV = (step) * 1000,\
-	.activate_bit = (abits),\
 }
 
 static struct da9052_regulator_info da9052_regulator_info[] = {
-- 
1.7.9.5

[PATCH 4/4] regulator: tps6586x: Use apply_[reg|bit] with regmap based voltage_sel operations

[Axel Lin]

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/tps6586x-regulator.c |   54 ++++++--------------------------
 1 file changed, 10 insertions(+), 44 deletions(-)

diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
index f86da67..e68382d 100644
--- a/drivers/regulator/tps6586x-regulator.c
+++ b/drivers/regulator/tps6586x-regulator.c
@@ -61,10 +61,6 @@ struct tps6586x_regulator {
 
 	int enable_bit[2];
 	int enable_reg[2];
-
-	/* for DVM regulators */
-	int go_reg;
-	int go_bit;
 };
 
 static inline struct device *to_tps6586x_dev(struct regulator_dev *rdev)
@@ -72,37 +68,10 @@ static inline struct device *to_tps6586x_dev(struct regulator_dev *rdev)
 	return rdev_get_dev(rdev)->parent;
 }
 
-static int tps6586x_set_voltage_sel(struct regulator_dev *rdev,
-				    unsigned selector)
-{
-	struct tps6586x_regulator *ri = rdev_get_drvdata(rdev);
-	struct device *parent = to_tps6586x_dev(rdev);
-	int ret, val, rid = rdev_get_id(rdev);
-	uint8_t mask;
-
-	val = selector << (ffs(rdev->desc->vsel_mask) - 1);
-	mask = rdev->desc->vsel_mask;
-
-	ret = tps6586x_update(parent, rdev->desc->vsel_reg, val, mask);
-	if (ret)
-		return ret;
-
-	/* Update go bit for DVM regulators */
-	switch (rid) {
-	case TPS6586X_ID_LDO_2:
-	case TPS6586X_ID_LDO_4:
-	case TPS6586X_ID_SM_0:
-	case TPS6586X_ID_SM_1:
-		ret = tps6586x_set_bits(parent, ri->go_reg, 1 << ri->go_bit);
-		break;
-	}
-	return ret;
-}
-
 static struct regulator_ops tps6586x_regulator_ops = {
 	.list_voltage = regulator_list_voltage_table,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
-	.set_voltage_sel = tps6586x_set_voltage_sel,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 
 	.is_enabled = regulator_is_enabled_regmap,
 	.enable = regulator_enable_regmap,
@@ -142,7 +111,7 @@ static const unsigned int tps6586x_dvm_voltages[] = {
 };
 
 #define TPS6586X_REGULATOR(_id, _pin_name, vdata, vreg, shift, nbits,	\
-			   ereg0, ebit0, ereg1, ebit1)			\
+			   ereg0, ebit0, ereg1, ebit1, goreg, gobit)	\
 	.desc	= {							\
 		.supply_name = _pin_name,				\
 		.name	= "REG-" #_id,					\
@@ -156,29 +125,26 @@ static const unsigned int tps6586x_dvm_voltages[] = {
 		.enable_mask = 1 << (ebit0),				\
 		.vsel_reg = TPS6586X_##vreg,				\
 		.vsel_mask = ((1 << (nbits)) - 1) << (shift),		\
+		.apply_reg = (goreg),				\
+		.apply_bit = (gobit),				\
 	},								\
 	.enable_reg[0]	= TPS6586X_SUPPLY##ereg0,			\
 	.enable_bit[0]	= (ebit0),					\
 	.enable_reg[1]	= TPS6586X_SUPPLY##ereg1,			\
 	.enable_bit[1]	= (ebit1),
 
-#define TPS6586X_REGULATOR_DVM_GOREG(goreg, gobit)			\
-	.go_reg = TPS6586X_##goreg,					\
-	.go_bit = (gobit),
-
 #define TPS6586X_LDO(_id, _pname, vdata, vreg, shift, nbits,		\
 		     ereg0, ebit0, ereg1, ebit1)			\
 {									\
 	TPS6586X_REGULATOR(_id, _pname, vdata, vreg, shift, nbits,	\
-			   ereg0, ebit0, ereg1, ebit1)			\
+			   ereg0, ebit0, ereg1, ebit1, 0, 0)		\
 }
 
 #define TPS6586X_DVM(_id, _pname, vdata, vreg, shift, nbits,		\
 		     ereg0, ebit0, ereg1, ebit1, goreg, gobit)		\
 {									\
 	TPS6586X_REGULATOR(_id, _pname, vdata, vreg, shift, nbits,	\
-			   ereg0, ebit0, ereg1, ebit1)			\
-	TPS6586X_REGULATOR_DVM_GOREG(goreg, gobit)			\
+			   ereg0, ebit0, ereg1, ebit1, goreg, gobit)	\
 }
 
 #define TPS6586X_SYS_REGULATOR()					\
@@ -207,13 +173,13 @@ static struct tps6586x_regulator tps6586x_regulator[] = {
 	TPS6586X_LDO(SM_2, "vin-sm2", sm2, SUPPLYV2, 0, 5, ENC, 7, END, 7),
 
 	TPS6586X_DVM(LDO_2, "vinldo23", dvm, LDO2BV1, 0, 5, ENA, 3,
-					ENB, 3, VCC2, 6),
+					ENB, 3, TPS6586X_VCC2, BIT(6)),
 	TPS6586X_DVM(LDO_4, "vinldo4", ldo4, LDO4V1, 0, 5, ENC, 3,
-					END, 3, VCC1, 6),
+					END, 3, TPS6586X_VCC1, BIT(6)),
 	TPS6586X_DVM(SM_0, "vin-sm0", dvm, SM0V1, 0, 5, ENA, 1,
-					ENB, 1, VCC1, 2),
+					ENB, 1, TPS6586X_VCC1, BIT(2)),
 	TPS6586X_DVM(SM_1, "vin-sm1", dvm, SM1V1, 0, 5, ENA, 0,
-					ENB, 0, VCC1, 0),
+					ENB, 0, TPS6586X_VCC1, BIT(0)),
 };
 
 /*
-- 
1.7.9.5

Re: [PATCH 1/4] regulator: core: Allow specify apply_[reg|bit] for regmap based voltage_sel operations

[Haojian Zhuang]

On Tue, Dec 18, 2012 at 9:30 AM, Axel Lin <axel.lin@ingics.com> wrote:
> Some DVM regulators needs to update apply_bit after setting vsel_reg to
> initiate voltage change on the output.  This patch adds apply_reg and
> apply_bit to struct regulator_desc and update
> regulator_set_voltage_sel_regmap() to set apply_bit of apply_reg when
> apply_bit is set.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>  drivers/regulator/core.c         |   12 +++++++++++-
>  include/linux/regulator/driver.h |    6 ++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com>

Re: [PATCH 2/4] regulator: 88pm8607: Use apply_[reg|bit] with regmap based voltage_sel operations

[Haojian Zhuang]

On Tue, Dec 18, 2012 at 9:31 AM, Axel Lin <axel.lin@ingics.com> wrote:
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>  drivers/regulator/88pm8607.c |   43 ++++++++++++------------------------------
>  1 file changed, 12 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/regulator/88pm8607.c b/drivers/regulator/88pm8607.c
> index 2b55711..9ebdf8c 100644
> --- a/drivers/regulator/88pm8607.c
> +++ b/drivers/regulator/88pm8607.c
> @@ -30,8 +30,6 @@ struct pm8607_regulator_info {
>         unsigned int    *vol_table;
>         unsigned int    *vol_suspend;
>
> -       int     update_reg;
> -       int     update_bit;
>         int     slope_double;
>  };
>
> @@ -222,29 +220,6 @@ static int pm8607_list_voltage(struct regulator_dev *rdev, unsigned index)
>         return ret;
>  }
>
> -static int pm8607_set_voltage_sel(struct regulator_dev *rdev, unsigned selector)
> -{
> -       struct pm8607_regulator_info *info = rdev_get_drvdata(rdev);
> -       uint8_t val;
> -       int ret;
> -
> -       val = (uint8_t)(selector << (ffs(rdev->desc->vsel_mask) - 1));
> -
> -       ret = pm860x_set_bits(info->i2c, rdev->desc->vsel_reg,
> -                             rdev->desc->vsel_mask, val);
> -       if (ret)
> -               return ret;
> -       switch (info->desc.id) {
> -       case PM8607_ID_BUCK1:
> -       case PM8607_ID_BUCK3:
> -               ret = pm860x_set_bits(info->i2c, info->update_reg,
> -                                     1 << info->update_bit,
> -                                     1 << info->update_bit);
> -               break;
> -       }
> -       return ret;
> -}
> -
>  static int pm8606_preg_enable(struct regulator_dev *rdev)
>  {
>         struct pm8607_regulator_info *info = rdev_get_drvdata(rdev);
> @@ -276,7 +251,7 @@ static int pm8606_preg_is_enabled(struct regulator_dev *rdev)
>
>  static struct regulator_ops pm8607_regulator_ops = {
>         .list_voltage   = pm8607_list_voltage,
> -       .set_voltage_sel = pm8607_set_voltage_sel,
> +       .set_voltage_sel = regulator_set_voltage_sel_regmap,
>         .get_voltage_sel = regulator_get_voltage_sel_regmap,
>         .enable = regulator_enable_regmap,
>         .disable = regulator_disable_regmap,
> @@ -313,11 +288,11 @@ static struct regulator_ops pm8606_preg_ops = {
>                 .n_voltages = ARRAY_SIZE(vreg##_table),                 \
>                 .vsel_reg = PM8607_##vreg,                              \
>                 .vsel_mask = ARRAY_SIZE(vreg##_table) - 1,              \
> +               .apply_reg = PM8607_##ureg,                             \
> +               .apply_bit = (ubit),                                    \
>                 .enable_reg = PM8607_##ereg,                            \
>                 .enable_mask = 1 << (ebit),                             \
>         },                                                              \
> -       .update_reg     = PM8607_##ureg,                                \
> -       .update_bit     = (ubit),                                       \
>         .slope_double   = (0),                                          \
>         .vol_table      = (unsigned int *)&vreg##_table,                \
>         .vol_suspend    = (unsigned int *)&vreg##_suspend_table,        \
> @@ -343,9 +318,15 @@ static struct regulator_ops pm8606_preg_ops = {
>  }
>
>  static struct pm8607_regulator_info pm8607_regulator_info[] = {
> -       PM8607_DVC(BUCK1, GO, 0, SUPPLIES_EN11, 0),
> -       PM8607_DVC(BUCK2, GO, 1, SUPPLIES_EN11, 1),
> -       PM8607_DVC(BUCK3, GO, 2, SUPPLIES_EN11, 2),
> +       PM8607_DVC(BUCK1, GO, BIT(0), SUPPLIES_EN11, 0),
> +       /*
> +        * BUCK2 is designed to control voltage of CP domain.
> +        * Linux only runs in AP side, so we don't need to control BUCK2 in AP
> +        * side. Thus set the apply bit to be 0 instead of BIT(1) to avoid
> +        * updating apply_bit for BUCK2.
> +        */
> +       PM8607_DVC(BUCK2, GO, 0, SUPPLIES_EN11, 1),
Since update bit is already defined in GO register of PM8607, it seems
better to
define it as BIT(1) in this register. Otherwise, the comment is so strange.

I'm sorry on this since I didn't look at your patch carefully in the
first round. Please
help to change it as
      PM8607_DVC(BUCK2, GO, BIT(1), SUPPLIES_EN11, 1).

Since I messed it with the implementation of set_voltage_sel() api.

For others, Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com>

> +       PM8607_DVC(BUCK3, GO, BIT(2), SUPPLIES_EN11, 2),
>
>         PM8607_LDO(1,         LDO1, 0, SUPPLIES_EN11, 3),
>         PM8607_LDO(2,         LDO2, 0, SUPPLIES_EN11, 4),
> --
> 1.7.9.5
>
>
>

Re: [PATCH 1/4] regulator: core: Allow specify apply_[reg|bit] for regmap based voltage_sel operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

On Tue, Dec 18, 2012 at 09:30:10AM +0800, Axel Lin wrote:
> Some DVM regulators needs to update apply_bit after setting vsel_reg to
> initiate voltage change on the output.  This patch adds apply_reg and

Applied all except patch 2 (where you've done a new version, I'll wait
for Haojian to ack though) - thanks!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] regulator: core: Allow specify apply_[reg|bit] for regmap based voltage_sel operations

[Haojian Zhuang]

On Tue, Dec 25, 2012 at 12:34 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Dec 18, 2012 at 09:30:10AM +0800, Axel Lin wrote:
>> Some DVM regulators needs to update apply_bit after setting vsel_reg to
>> initiate voltage change on the output.  This patch adds apply_reg and
>
> Applied all except patch 2 (where you've done a new version, I'll wait
> for Haojian to ack though) - thanks!

I acked 2nd patch just now.

Best Regards
Haojian

Re: [PATCH 3/4] regulator: da9052: Use apply_[reg|bit] with regmap based voltage_sel operations

[Michael Grzeschik]

Hi Axel, Mark,

we should not play to much with generous functions for this pmics. With this
patch we loose the fix_io code path from da9052_reg_update.

include/linux/mfd/da9052.h
...
        if (da9052->fix_io) {
                ret = da9052->fix_io(da9052, reg);
                if (ret < 0)
                        return ret;
...

This fix_io used in drivers/mfd/da9052-i2c.c

/*
 * There is an issue with DA9052 and DA9053_AA/BA/BB PMIC where the PMIC
 * gets lockup up or fails to respond following a system reset.
 * This fix is to follow any read or write with a dummy read to a safe
 * register.
 */
static int da9052_i2c_fix(struct da9052 *da9052, unsigned char reg)
{
        int val;

        switch (da9052->chip_id) {
        case DA9052:
        case DA9053_AA:
        case DA9053_BA:
        case DA9053_BB:
                /* A dummy read to a safe register address. */
        if (!i2c_safe_reg(reg))
                        return regmap_read(da9052->regmap,
                                           DA9052_PARK_REGISTER,
                                           &val);
                break;
        default:
                /*
                 * For other chips parking of I2C register
                 * to a safe place is not required.
                 */
                break;
        }

        return 0;
}

I suggest that Mark queue the revert of this patch.

Thanks,
Michael

On Tue, Dec 18, 2012 at 09:32:52AM +0800, Axel Lin wrote:
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>  drivers/regulator/da9052-regulator.c |   41 +++++-----------------------------
>  1 file changed, 6 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/regulator/da9052-regulator.c b/drivers/regulator/da9052-regulator.c
> index d096309..c6d8651 100644
> --- a/drivers/regulator/da9052-regulator.c
> +++ b/drivers/regulator/da9052-regulator.c
> @@ -70,7 +70,6 @@ struct da9052_regulator_info {
>  	int step_uV;
>  	int min_uV;
>  	int max_uV;
> -	unsigned char activate_bit;
>  };
>  
>  struct da9052_regulator {
> @@ -210,36 +209,6 @@ static int da9052_map_voltage(struct regulator_dev *rdev,
>  	return sel;
>  }
>  
> -static int da9052_regulator_set_voltage_sel(struct regulator_dev *rdev,
> -					    unsigned int selector)
> -{
> -	struct da9052_regulator *regulator = rdev_get_drvdata(rdev);
> -	struct da9052_regulator_info *info = regulator->info;
> -	int id = rdev_get_id(rdev);
> -	int ret;
> -
> -	ret = da9052_reg_update(regulator->da9052, rdev->desc->vsel_reg,
> -				rdev->desc->vsel_mask, selector);
> -	if (ret < 0)
> -		return ret;
> -
> -	/* Some LDOs and DCDCs are DVC controlled which requires enabling of
> -	 * the activate bit to implment the changes on the output.
> -	 */
> -	switch (id) {
> -	case DA9052_ID_BUCK1:
> -	case DA9052_ID_BUCK2:
> -	case DA9052_ID_BUCK3:
> -	case DA9052_ID_LDO2:
> -	case DA9052_ID_LDO3:
> -		ret = da9052_reg_update(regulator->da9052, DA9052_SUPPLY_REG,
> -					info->activate_bit, info->activate_bit);
> -		break;
> -	}
> -
> -	return ret;
> -}
> -
>  static struct regulator_ops da9052_dcdc_ops = {
>  	.get_current_limit = da9052_dcdc_get_current_limit,
>  	.set_current_limit = da9052_dcdc_set_current_limit,
> @@ -247,7 +216,7 @@ static struct regulator_ops da9052_dcdc_ops = {
>  	.list_voltage = da9052_list_voltage,
>  	.map_voltage = da9052_map_voltage,
>  	.get_voltage_sel = regulator_get_voltage_sel_regmap,
> -	.set_voltage_sel = da9052_regulator_set_voltage_sel,
> +	.set_voltage_sel = regulator_set_voltage_sel_regmap,
>  	.is_enabled = regulator_is_enabled_regmap,
>  	.enable = regulator_enable_regmap,
>  	.disable = regulator_disable_regmap,
> @@ -257,7 +226,7 @@ static struct regulator_ops da9052_ldo_ops = {
>  	.list_voltage = da9052_list_voltage,
>  	.map_voltage = da9052_map_voltage,
>  	.get_voltage_sel = regulator_get_voltage_sel_regmap,
> -	.set_voltage_sel = da9052_regulator_set_voltage_sel,
> +	.set_voltage_sel = regulator_set_voltage_sel_regmap,
>  	.is_enabled = regulator_is_enabled_regmap,
>  	.enable = regulator_enable_regmap,
>  	.disable = regulator_disable_regmap,
> @@ -274,13 +243,14 @@ static struct regulator_ops da9052_ldo_ops = {
>  		.owner = THIS_MODULE,\
>  		.vsel_reg = DA9052_BUCKCORE_REG + DA9052_ID_##_id, \
>  		.vsel_mask = (1 << (sbits)) - 1,\
> +		.apply_reg = DA9052_SUPPLY_REG, \
> +		.apply_bit = (abits), \
>  		.enable_reg = DA9052_BUCKCORE_REG + DA9052_ID_##_id, \
>  		.enable_mask = 1 << (ebits),\
>  	},\
>  	.min_uV = (min) * 1000,\
>  	.max_uV = (max) * 1000,\
>  	.step_uV = (step) * 1000,\
> -	.activate_bit = (abits),\
>  }
>  
>  #define DA9052_DCDC(_id, step, min, max, sbits, ebits, abits) \
> @@ -294,13 +264,14 @@ static struct regulator_ops da9052_ldo_ops = {
>  		.owner = THIS_MODULE,\
>  		.vsel_reg = DA9052_BUCKCORE_REG + DA9052_ID_##_id, \
>  		.vsel_mask = (1 << (sbits)) - 1,\
> +		.apply_reg = DA9052_SUPPLY_REG, \
> +		.apply_bit = (abits), \
>  		.enable_reg = DA9052_BUCKCORE_REG + DA9052_ID_##_id, \
>  		.enable_mask = 1 << (ebits),\
>  	},\
>  	.min_uV = (min) * 1000,\
>  	.max_uV = (max) * 1000,\
>  	.step_uV = (step) * 1000,\
> -	.activate_bit = (abits),\
>  }
>  
>  static struct da9052_regulator_info da9052_regulator_info[] = {
> -- 
> 1.7.9.5




-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] regulator: da9052: Use apply_[reg|bit] with regmap based voltage_sel operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

On Wed, Oct 02, 2013 at 12:15:00PM +0200, Michael Grzeschik wrote:

> we should not play to much with generous functions for this pmics. With this
> patch we loose the fix_io code path from da9052_reg_update.

> I suggest that Mark queue the revert of this patch.

This seems far too obscure to be robust - if we need to bodge all I2C
I/O (which is what that looks like) then that should be being done in
the regmap for the device rather than by having stuff open coded in the
function drivers like this.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]