regulator: Fix setting new voltage in s5m8767_set_voltage

PATCH RESEND Applied Axel Lin 3 participants 12 messages Mar 19, 2012 – Apr 3, 2012

Subsystems (2)

Voltage and current regulator framework Samsung multifunction PMIC device drivers

lore

Share & tools

lore

Axel Lin

author

2012-03-19 04:22 UTC #0 lore raw

PATCH RESEND 1/4 regulator: Fix setting new voltage in s5m8767_set_voltage

Current code does not really update the register with new value, fix it. I rename the variable i to sel for better readability.

Signed-off-by Axel Lin <[email protected]>

---
Hi Sangbeom,
I make the following patchs on top of this one, so I resend this one as the first patch of this serial.
I'd appreciate to have your feedback about the changes in this patch serial.

Thanks,
Axel

 drivers/regulator/s5m8767.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

drivers/regulator/s5m8767.c +7 -7

index 58447db..4ca2db0 100644
@@ -311,8 +311,7 @@ static int s5m8767_set_voltage(struct regulator_dev *rdev,
 	struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
 	const struct s5m_voltage_desc *desc;
 	int reg_id = rdev_get_id(rdev);
-	int reg, mask, ret;
-	int i;
+	int sel, reg, mask, ret;
 	u8 val;
 
 	switch (reg_id) {

@@ -333,19 +332,20 @@ static int s5m8767_set_voltage(struct regulator_dev *rdev,
 
 	desc = reg_voltage_map[reg_id];
 
-	i = s5m8767_convert_voltage_to_sel(desc, min_uV, max_uV);
-	if (i < 0)
-		return i;
+	sel = s5m8767_convert_voltage_to_sel(desc, min_uV, max_uV);
+	if (sel < 0)
+		return sel;
 
 	ret = s5m8767_get_voltage_register(rdev, &reg);
 	if (ret)
 		return ret;
 
 	s5m_reg_read(s5m8767->iodev, reg, &val);
-	val = val & mask;
+	val &= ~mask;
+	val |= sel;
 
 	ret = s5m_reg_write(s5m8767->iodev, reg, val);
-	*selector = i;
+	*selector = sel;
 
 	return ret;
 }

Signature · 1 line

1.7.5.4

Axel Lin

author

2012-03-19 04:23 UTC #1 lore raw

RFC PATCH 2/4 regulator: Rework s5m8767_set_voltage to support both LDOs and BUCKs

s5m8767_set_voltage not only implement set_voltage callbacks for LDOs, but also for BUCKs when s5m8767->buck_gpioindex is not set. s5m8767_set_voltage_buck actually only for buck[2|3|4] when s5m8767->buck_gpioindex is set.
Conditionally calling s5m8767_set_voltage in s5m8767_set_voltage_buck makes the code hard to read.

I think merging s5m8767_set_voltage_buck and s5m8767_set_voltage actually simplifies things. It's easy to use buck234_vol pointer to differentiate if we need to control voltage for buck[2|3|4] by DVS GPIOs.

This patch reworks s5m8767_set_voltage to support both LDOx and BUCKx in all cases.

Signed-off-by Axel Lin <[email protected]>

---
 drivers/regulator/s5m8767.c |  145 +++++++++++++++----------------------------
 1 files changed, 50 insertions(+), 95 deletions(-)

drivers/regulator/s5m8767.c +49 ~~-94~~

index 4ca2db0..7576134 100644
@@ -305,14 +305,33 @@ static int s5m8767_convert_voltage_to_sel(
 	return selector;
 }
 
+static inline void s5m8767_set_high(struct s5m8767_info *s5m8767)
+{
+	int temp_index = s5m8767->buck_gpioindex;
+
+	gpio_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);
+	gpio_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
+	gpio_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
+}
+
+static inline void s5m8767_set_low(struct s5m8767_info *s5m8767)
+{
+	int temp_index = s5m8767->buck_gpioindex;
+
+	gpio_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
+	gpio_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
+	gpio_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);
+}
+
 static int s5m8767_set_voltage(struct regulator_dev *rdev,
 				int min_uV, int max_uV, unsigned *selector)
 {
 	struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
 	const struct s5m_voltage_desc *desc;
 	int reg_id = rdev_get_id(rdev);
-	int sel, reg, mask, ret;
+	int sel, reg, mask, ret = 0, old_index, index = 0;
 	u8 val;
+	u8 *buck234_vol = NULL;
 
 	switch (reg_id) {
 	case S5M8767_LDO1 ... S5M8767_LDO28:

@@ -320,6 +339,12 @@ static int s5m8767_set_voltage(struct regulator_dev *rdev,
 		break;
 	case S5M8767_BUCK1 ... S5M8767_BUCK6:
 		mask = 0xff;
+		if (reg_id == S5M8767_BUCK2 && s5m8767->buck2_gpiodvs)
+			buck234_vol = &s5m8767->buck2_vol[0];
+		else if (reg_id == S5M8767_BUCK3 && s5m8767->buck3_gpiodvs)
+			buck234_vol = &s5m8767->buck3_vol[0];
+		else if (reg_id == S5M8767_BUCK4 && s5m8767->buck4_gpiodvs)
+			buck234_vol = &s5m8767->buck4_vol[0];
 		break;
 	case S5M8767_BUCK7 ... S5M8767_BUCK8:
 		return -EINVAL;

@@ -336,102 +361,32 @@ static int s5m8767_set_voltage(struct regulator_dev *rdev,
 	if (sel < 0)
 		return sel;
 
-	ret = s5m8767_get_voltage_register(rdev, &reg);
-	if (ret)
-		return ret;
-
-	s5m_reg_read(s5m8767->iodev, reg, &val);
-	val &= ~mask;
-	val |= sel;
-
-	ret = s5m_reg_write(s5m8767->iodev, reg, val);
-	*selector = sel;
-
-	return ret;
-}
-
-static inline void s5m8767_set_high(struct s5m8767_info *s5m8767)
-{
-	int temp_index = s5m8767->buck_gpioindex;
-
-	gpio_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);
-	gpio_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
-	gpio_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
-}
-
-static inline void s5m8767_set_low(struct s5m8767_info *s5m8767)
-{
-	int temp_index = s5m8767->buck_gpioindex;
-
-	gpio_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
-	gpio_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
-	gpio_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);
-}
-
-static int s5m8767_set_voltage_buck(struct regulator_dev *rdev,
-				    int min_uV, int max_uV, unsigned *selector)
-{
-	struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
-	int reg_id = rdev_get_id(rdev);
-	const struct s5m_voltage_desc *desc;
-	int new_val, old_val, i = 0;
-
-	if (reg_id < S5M8767_BUCK1 || reg_id > S5M8767_BUCK6)
-		return -EINVAL;
-
-	switch (reg_id) {
-	case S5M8767_BUCK1:
-		return s5m8767_set_voltage(rdev, min_uV, max_uV, selector);
-	case S5M8767_BUCK2 ... S5M8767_BUCK4:
-		break;
-	case S5M8767_BUCK5 ... S5M8767_BUCK6:
-		return s5m8767_set_voltage(rdev, min_uV, max_uV, selector);
-	case S5M8767_BUCK9:
-		return s5m8767_set_voltage(rdev, min_uV, max_uV, selector);
-	}
+	/* buck234_vol != NULL means to control buck234 voltage via DVS GPIO */
+	if (buck234_vol) {
+		while (*buck234_vol != sel) {
+			buck234_vol++;
+			index++;
+		}
+		old_index = s5m8767->buck_gpioindex;
+		s5m8767->buck_gpioindex = index;
+
+		if (index > old_index)
+			s5m8767_set_high(s5m8767);
+		else
+			s5m8767_set_low(s5m8767);
+	} else {
+		ret = s5m8767_get_voltage_register(rdev, &reg);
+		if (ret)
+			return ret;
 
-	desc = reg_voltage_map[reg_id];
-	new_val = s5m8767_convert_voltage_to_sel(desc, min_uV, max_uV);
-	if (new_val < 0)
-		return new_val;
+		s5m_reg_read(s5m8767->iodev, reg, &val);
+		val = (val & ~mask) | sel;
 
-	switch (reg_id) {
-	case S5M8767_BUCK2:
-		if (s5m8767->buck2_gpiodvs) {
-			while (s5m8767->buck2_vol[i] != new_val)
-				i++;
-		} else
-			return s5m8767_set_voltage(rdev, min_uV,
-						   max_uV, selector);
-		break;
-	case S5M8767_BUCK3:
-		if (s5m8767->buck3_gpiodvs) {
-			while (s5m8767->buck3_vol[i] != new_val)
-				i++;
-		} else
-			return s5m8767_set_voltage(rdev, min_uV,
-						   max_uV, selector);
-		break;
-	case S5M8767_BUCK4:
-		if (s5m8767->buck3_gpiodvs) {
-			while (s5m8767->buck4_vol[i] != new_val)
-				i++;
-		} else
-			return s5m8767_set_voltage(rdev, min_uV,
-						   max_uV, selector);
-		break;
+		ret = s5m_reg_write(s5m8767->iodev, reg, val);
 	}
 
-	old_val = s5m8767->buck_gpioindex;
-	s5m8767->buck_gpioindex = i;
-
-	if (i > old_val)
-		s5m8767_set_high(s5m8767);
-	else
-		s5m8767_set_low(s5m8767);
-
-	*selector = new_val;
-	return 0;
+	*selector = sel;
+	return ret;
 }
 
 static int s5m8767_set_voltage_time_sel(struct regulator_dev *rdev,

@@ -466,7 +421,7 @@ static struct regulator_ops s5m8767_buck_ops = {
 	.enable			= s5m8767_reg_enable,
 	.disable		= s5m8767_reg_disable,
 	.get_voltage_sel	= s5m8767_get_voltage_sel,
-	.set_voltage		= s5m8767_set_voltage_buck,
+	.set_voltage		= s5m8767_set_voltage,
 	.set_voltage_time_sel	= s5m8767_set_voltage_time_sel,
 };

Signature · 1 line

1.7.5.4

Sangbeom Kim

2012-04-02 23:16 UTC #2 lore raw

RE: [PATCH RFC 2/4] regulator: Rework s5m8767_set_voltage to support both LDOs and BUCKs

Quoted 7 lines · Axel Lin

s5m8767_set_voltage not only implement set_voltage callbacks for LDOs, but also for BUCKs when s5m8767->buck_gpioindex is not set. s5m8767_set_voltage_buck actually only for buck[2|3|4] when s5m8767->buck_gpioindex is set.
Conditionally calling s5m8767_set_voltage in s5m8767_set_voltage_buck makes the code hard to read.

Quoted 8 lines

I think merging s5m8767_set_voltage_buck and s5m8767_set_voltage actually simplifies things. It's easy to use buck234_vol pointer to differentiate if we need to control voltage for buck[2|3|4] by DVS GPIOs.
This patch reworks s5m8767_set_voltage to support both LDOx and BUCKx in all cases.
Signed-off-by Axel Lin <[email protected]>

Hi, Axel
Basically, Your optimization code looks good. But, S5M8767A support 9 Bucks.
Please Add #9 Buck handling too.
As you know, 3.4-rc1 is released. Please rebase your patch on 3.4-rc1. Some feature is added on 3.4-rc1.

Thanks,
Sangbeom.

Axel Lin

author

2012-04-03 00:01 UTC #3 lore raw

Re: [PATCH RFC 2/4] regulator: Rework s5m8767_set_voltage to support both LDOs and BUCKs

Quoted 4 lines

Hi, Axel
Basically, Your optimization code looks good. But, S5M8767A support 9 Bucks.
Please Add #9 Buck handling too.

Current code does hand #9 buck in s5m8767_set_voltage().

Quoted 3 lines

As you know, 3.4-rc1 is released. Please rebase your patch on 3.4-rc1. Some feature is added on 3.4-rc1.

I just try to apply the patch to Mark's for-next tree and linux-next tree. It looks ok. So I think no need to re-generate the patch.

Regards,
Axel

Sangbeom Kim

Acked-by on v1 2/4

2012-04-03 05:40 UTC #4 lore raw

RE: [PATCH RFC 2/4] regulator: Rework s5m8767_set_voltage to support both LDOs and BUCKs

In reply to Axel Lin

Quoted 7 lines · Axel Lin

s5m8767_set_voltage not only implement set_voltage callbacks for LDOs, but also for BUCKs when s5m8767->buck_gpioindex is not set. s5m8767_set_voltage_buck actually only for buck[2|3|4] when s5m8767->buck_gpioindex is set.
Conditionally calling s5m8767_set_voltage in s5m8767_set_voltage_buck makes the code hard to read.

Quoted 8 lines

I think merging s5m8767_set_voltage_buck and s5m8767_set_voltage actually simplifies things. It's easy to use buck234_vol pointer to differentiate if we need to control voltage for buck[2|3|4] by DVS GPIOs.
This patch reworks s5m8767_set_voltage to support both LDOx and BUCKx in all cases.
Signed-off-by Axel Lin <[email protected]>

Acked-by Sangbeom Kim <[email protected]>

Thanks,
Sangbeom.

Axel Lin

author

2012-03-19 04:24 UTC #5 lore raw

RFC PATCH 3/4 regulator: Use one s5m8767_ops for both LDOs and BUCKs

In reply to Axel Lin

All the callback function implementation are the same for both LDOs and BUCKs. Merge them to one s5m8767_ops.

Signed-off-by Axel Lin <[email protected]>

---
 drivers/regulator/s5m8767.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)

drivers/regulator/s5m8767.c +3 ~~-13~~

index 7576134..8872ee0 100644
@@ -405,17 +405,7 @@ static int s5m8767_set_voltage_time_sel(struct regulator_dev *rdev,
 	return 0;
 }
 
-static struct regulator_ops s5m8767_ldo_ops = {
-	.list_voltage		= s5m8767_list_voltage,
-	.is_enabled		= s5m8767_reg_is_enabled,
-	.enable			= s5m8767_reg_enable,
-	.disable		= s5m8767_reg_disable,
-	.get_voltage_sel	= s5m8767_get_voltage_sel,
-	.set_voltage		= s5m8767_set_voltage,
-	.set_voltage_time_sel	= s5m8767_set_voltage_time_sel,
-};
-
-static struct regulator_ops s5m8767_buck_ops = {
+static struct regulator_ops s5m8767_ops = {
 	.list_voltage		= s5m8767_list_voltage,
 	.is_enabled		= s5m8767_reg_is_enabled,
 	.enable			= s5m8767_reg_enable,

@@ -428,14 +418,14 @@ static struct regulator_ops s5m8767_buck_ops = {
 #define regulator_desc_ldo(num)		{	\
 	.name		= "LDO"#num,		\
 	.id		= S5M8767_LDO##num,	\
-	.ops		= &s5m8767_ldo_ops,	\
+	.ops		= &s5m8767_ops,	\
 	.type		= REGULATOR_VOLTAGE,	\
 	.owner		= THIS_MODULE,		\
 }
 #define regulator_desc_buck(num)	{	\
 	.name		= "BUCK"#num,		\
 	.id		= S5M8767_BUCK##num,	\
-	.ops		= &s5m8767_buck_ops,	\
+	.ops		= &s5m8767_ops,	\
 	.type		= REGULATOR_VOLTAGE,	\
 	.owner		= THIS_MODULE,		\
 }

Signature · 1 line

1.7.5.4

Sangbeom Kim

Acked-by on v1 3/4

2012-04-03 05:42 UTC #6 lore raw

RE: [PATCH RFC 3/4] regulator: Use one s5m8767_ops for both LDOs and BUCKs

Quoted 4 lines · Axel Lin

All the callback function implementation are the same for both LDOs and BUCKs. Merge them to one s5m8767_ops.
Signed-off-by Axel Lin <[email protected]>

Acked-by Sangbeom Kim <[email protected]>

Thanks,
Sangbeom.

Axel Lin

author

2012-03-19 04:25 UTC #7 lore raw

RFC PATCH 4/4 regulator: Replace regulator_desc_[ldo|buck] by s5m8767_regulator_desc macro

In reply to Axel Lin

Signed-off-by Axel Lin <[email protected]>

---
 drivers/regulator/s5m8767.c |   89 ++++++++++++++++++++-----------------------
 1 files changed, 41 insertions(+), 48 deletions(-)

drivers/regulator/s5m8767.c +41 ~~-48~~

index 8872ee0..b877df1 100644
@@ -415,59 +415,52 @@ static struct regulator_ops s5m8767_ops = {
 	.set_voltage_time_sel	= s5m8767_set_voltage_time_sel,
 };
 
-#define regulator_desc_ldo(num)		{	\
-	.name		= "LDO"#num,		\
-	.id		= S5M8767_LDO##num,	\
-	.ops		= &s5m8767_ops,	\
-	.type		= REGULATOR_VOLTAGE,	\
-	.owner		= THIS_MODULE,		\
-}
-#define regulator_desc_buck(num)	{	\
-	.name		= "BUCK"#num,		\
-	.id		= S5M8767_BUCK##num,	\
-	.ops		= &s5m8767_ops,	\
+#define s5m8767_regulator_desc(_name) {		\
+	.name		= #_name,		\
+	.id		= S5M8767_##_name,	\
+	.ops		= &s5m8767_ops,		\
 	.type		= REGULATOR_VOLTAGE,	\
 	.owner		= THIS_MODULE,		\
 }
 
 static struct regulator_desc regulators[] = {
-	regulator_desc_ldo(1),
-	regulator_desc_ldo(2),
-	regulator_desc_ldo(3),
-	regulator_desc_ldo(4),
-	regulator_desc_ldo(5),
-	regulator_desc_ldo(6),
-	regulator_desc_ldo(7),
-	regulator_desc_ldo(8),
-	regulator_desc_ldo(9),
-	regulator_desc_ldo(10),
-	regulator_desc_ldo(11),
-	regulator_desc_ldo(12),
-	regulator_desc_ldo(13),
-	regulator_desc_ldo(14),
-	regulator_desc_ldo(15),
-	regulator_desc_ldo(16),
-	regulator_desc_ldo(17),
-	regulator_desc_ldo(18),
-	regulator_desc_ldo(19),
-	regulator_desc_ldo(20),
-	regulator_desc_ldo(21),
-	regulator_desc_ldo(22),
-	regulator_desc_ldo(23),
-	regulator_desc_ldo(24),
-	regulator_desc_ldo(25),
-	regulator_desc_ldo(26),
-	regulator_desc_ldo(27),
-	regulator_desc_ldo(28),
-	regulator_desc_buck(1),
-	regulator_desc_buck(2),
-	regulator_desc_buck(3),
-	regulator_desc_buck(4),
-	regulator_desc_buck(5),
-	regulator_desc_buck(6),
-	regulator_desc_buck(7),
-	regulator_desc_buck(8),
-	regulator_desc_buck(9),
+	s5m8767_regulator_desc(LDO1),
+	s5m8767_regulator_desc(LDO2),
+	s5m8767_regulator_desc(LDO3),
+	s5m8767_regulator_desc(LDO4),
+	s5m8767_regulator_desc(LDO5),
+	s5m8767_regulator_desc(LDO6),
+	s5m8767_regulator_desc(LDO7),
+	s5m8767_regulator_desc(LDO8),
+	s5m8767_regulator_desc(LDO9),
+	s5m8767_regulator_desc(LDO10),
+	s5m8767_regulator_desc(LDO11),
+	s5m8767_regulator_desc(LDO12),
+	s5m8767_regulator_desc(LDO13),
+	s5m8767_regulator_desc(LDO14),
+	s5m8767_regulator_desc(LDO15),
+	s5m8767_regulator_desc(LDO16),
+	s5m8767_regulator_desc(LDO17),
+	s5m8767_regulator_desc(LDO18),
+	s5m8767_regulator_desc(LDO19),
+	s5m8767_regulator_desc(LDO20),
+	s5m8767_regulator_desc(LDO21),
+	s5m8767_regulator_desc(LDO22),
+	s5m8767_regulator_desc(LDO23),
+	s5m8767_regulator_desc(LDO24),
+	s5m8767_regulator_desc(LDO25),
+	s5m8767_regulator_desc(LDO26),
+	s5m8767_regulator_desc(LDO27),
+	s5m8767_regulator_desc(LDO28),
+	s5m8767_regulator_desc(BUCK1),
+	s5m8767_regulator_desc(BUCK2),
+	s5m8767_regulator_desc(BUCK3),
+	s5m8767_regulator_desc(BUCK4),
+	s5m8767_regulator_desc(BUCK5),
+	s5m8767_regulator_desc(BUCK6),
+	s5m8767_regulator_desc(BUCK7),
+	s5m8767_regulator_desc(BUCK8),
+	s5m8767_regulator_desc(BUCK9),
 };
 
 static __devinit int s5m8767_pmic_probe(struct platform_device *pdev)

Signature · 1 line

1.7.5.4

Sangbeom Kim

Acked-by on v1 4/4

2012-04-03 05:54 UTC #8 lore raw

RE: [PATCH RFC 4/4] regulator: Replace regulator_desc_[ldo|buck] by s5m8767_regulator_desc macro

Quoted 4 lines · Axel Lin

Subject: [PATCH RFC 4/4] regulator: Replace regulator_desc_[ldo|buck] by s5m8767_regulator_desc macro
Signed-off-by Axel Lin <[email protected]>

Where is commit message?
Otherwise,

Acked-by Sangbeom Kim <[email protected]>

Thanks,
Sangbeom.

Axel Lin

author

2012-03-29 12:42 UTC #9 lore raw

In reply to Axel Lin

2012/3/19 Axel Lin <[email protected]>:

Quoted 10 lines

Current code does not really update the register with new value, fix it. I rename the variable i to sel for better readability.
Signed-off-by Axel Lin <[email protected]>
---
Hi Sangbeom,
I make the following patchs on top of this one, so I resend this one as the first patch of this serial.
I'd appreciate to have your feedback about the changes in this patch serial.

Hi Sangbeom,
Are you OK with this patch serial?
I think current code in s5m8767_set_voltage() does not work without this patch.

Regards,
Axel

Sangbeom Kim

Acked-by on v1 1/4

2012-03-30 05:56 UTC #10 lore raw

In reply to Axel Lin

Quoted 4 lines · Axel Lin

Current code does not really update the register with new value, fix it. I rename the variable i to sel for better readability.
Signed-off-by Axel Lin <[email protected]>

Acked-by Sangbeom Kim <[email protected]>

Thanks for your fixing patch.
Sangbeom.

Mark Brown

Applied

2012-03-31 10:33 UTC #11 lore raw

In reply to Axel Lin

Quoted 2 lines · Axel Lin

Current code does not really update the register with new value, fix it. I rename the variable i to sel for better readability.

Applied, thanks. I'll apply the others after this has been merged with Linus since there's going to be a dependency there.