[PATCH] regulator: Rename s5m8767_convert_voltage to s5m8767_get_proper_voltage_sel

[PATCH] regulator: Rename s5m8767_convert_voltage to s5m8767_get_proper_voltage_sel

[Axel Lin]

This function finds the smallest voltage that falls within the specified range,
and then returns the selector. This rename makes the intention more clear.

Also remove unneeded local variables min_vol and max_vol in s5m8767_set_voltage
and s5m8767_set_voltage_buck.

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
I was confused by the function name, s5m8767_convert_voltage() converts what to what?

Axel
 drivers/regulator/s5m8767.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index e369d9e..03d5e14 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -294,11 +294,11 @@ static int s5m8767_get_voltage_sel(struct regulator_dev *rdev)
 	return val;
 }
 
-static inline int s5m8767_convert_voltage(
+static int s5m8767_get_proper_voltage_sel(
 		const struct s5m_voltage_desc *desc,
 		int min_vol, int max_vol)
 {
-	int out_vol = 0;
+	int selector = 0;
 
 	if (desc == NULL)
 		return -EINVAL;
@@ -306,19 +306,18 @@ static inline int s5m8767_convert_voltage(
 	if (max_vol < desc->min || min_vol > desc->max)
 		return -EINVAL;
 
-	out_vol = (min_vol - desc->min) / desc->step;
+	selector = (min_vol - desc->min) / desc->step;
 
-	if (desc->min + desc->step * out_vol > max_vol)
+	if (desc->min + desc->step * selector > max_vol)
 		return -EINVAL;
 
-	return out_vol;
+	return selector;
 }
 
 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);
-	int min_vol = min_uV, max_vol = max_uV;
 	const struct s5m_voltage_desc *desc;
 	int reg_id = rdev_get_id(rdev);
 	int reg, mask, ret;
@@ -343,7 +342,7 @@ static int s5m8767_set_voltage(struct regulator_dev *rdev,
 
 	desc = reg_voltage_map[reg_id];
 
-	i = s5m8767_convert_voltage(desc, min_vol, max_vol);
+	i = s5m8767_get_proper_voltage_sel(desc, min_uV, max_uV);
 	if (i < 0)
 		return i;
 
@@ -385,7 +384,6 @@ static int s5m8767_set_voltage_buck(struct regulator_dev *rdev,
 	int reg_id = rdev_get_id(rdev);
 	const struct s5m_voltage_desc *desc;
 	int new_val, old_val, i = 0;
-	int min_vol = min_uV, max_vol = max_uV;
 
 	if (reg_id < S5M8767_BUCK1 || reg_id > S5M8767_BUCK6)
 		return -EINVAL;
@@ -402,7 +400,7 @@ static int s5m8767_set_voltage_buck(struct regulator_dev *rdev,
 	}
 
 	desc = reg_voltage_map[reg_id];
-	new_val = s5m8767_convert_voltage(desc, min_vol, max_vol);
+	new_val = s5m8767_get_proper_voltage_sel(desc, min_uV, max_uV);
 	if (new_val < 0)
 		return new_val;
 
@@ -580,7 +578,7 @@ static __devinit int s5m8767_pmic_probe(struct platform_device *pdev)
 	for (i = 0; i < 8; i++) {
 		if (s5m8767->buck2_gpiodvs) {
 			s5m8767->buck2_vol[i] =
-				s5m8767_convert_voltage(
+				s5m8767_get_proper_voltage_sel(
 						&buck_voltage_val2,
 						pdata->buck2_voltage[i],
 						pdata->buck2_voltage[i] +
@@ -589,7 +587,7 @@ static __devinit int s5m8767_pmic_probe(struct platform_device *pdev)
 
 		if (s5m8767->buck3_gpiodvs) {
 			s5m8767->buck3_vol[i] =
-				s5m8767_convert_voltage(
+				s5m8767_get_proper_voltage_sel(
 						&buck_voltage_val2,
 						pdata->buck3_voltage[i],
 						pdata->buck3_voltage[i] +
@@ -598,7 +596,7 @@ static __devinit int s5m8767_pmic_probe(struct platform_device *pdev)
 
 		if (s5m8767->buck4_gpiodvs) {
 			s5m8767->buck4_vol[i] =
-				s5m8767_convert_voltage(
+				s5m8767_get_proper_voltage_sel(
 						&buck_voltage_val2,
 						pdata->buck4_voltage[i],
 						pdata->buck4_voltage[i] +
-- 
1.7.5.4

Re: [PATCH] regulator: Rename s5m8767_convert_voltage to s5m8767_get_proper_voltage_sel

[Axel Lin]

2012/3/7 Axel Lin <axel.lin@gmail.com>:
> This function finds the smallest voltage that falls within the specified range,
> and then returns the selector. This rename makes the intention more clear.
>
> Also remove unneeded local variables min_vol and max_vol in s5m8767_set_voltage
> and s5m8767_set_voltage_buck.
>
> Signed-off-by: Axel Lin <axel.lin@gmail.com>
> ---
> I was confused by the function name, s5m8767_convert_voltage() converts what to what?
>

BTW, I'm also confused by s5m8767_set_high and s5m8767_set_low functions.
Both function sets the same gpios to the same state. ( by different order )
Maybe we need comments (or rename) for both functions.

Regards,
Axel

RE: [PATCH] regulator: Rename s5m8767_convert_voltage to s5m8767_get_proper_voltage_sel

[Sangbeom Kim]

Hi,

> > I was confused by the function name, s5m8767_convert_voltage() converts
> what to what?
Convert means that Normal Voltage unit to 8767 internal voltage register
unit.
> BTW, I'm also confused by s5m8767_set_high and s5m8767_set_low functions.
> Both function sets the same gpios to the same state. ( by different order
)
> Maybe we need comments (or rename) for both functions.
S5M8767A can be controlled with 3 GPIO.
To control voltage with GPIO, Order is very important.
Because Voltage inversion should not be happened.
If DVFS table is like a below,
1.5Ghz  1.5V
1.0Ghz  1.0V
0.5Ghz  0.5V

If System try to change 1.0Ghz(1.0V) -> 1.5Ghz(1.5V)
Absolutely 0.5V should not be set.

Thanks,
Sangbeom.

Re: [PATCH] regulator: Rename s5m8767_convert_voltage to s5m8767_get_proper_voltage_sel

[Mark Brown]

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

On Wed, Mar 07, 2012 at 12:12:02PM +0800, Axel Lin wrote:
> This function finds the smallest voltage that falls within the specified range,
> and then returns the selector. This rename makes the intention more clear.

Sangbeom, are you OK with this change?

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

RE: [PATCH] regulator: Rename s5m8767_convert_voltage to s5m8767_get_proper_voltage_sel

[Sangbeom Kim]

Hi!

On Wednesday, Mar 07, 2012 at 1:12AM, Axel Lin wrote:
> -static inline int s5m8767_convert_voltage(
> +static int s5m8767_get_proper_voltage_sel(

s5m8767_get_voltage_sel and s5m8767_get_proper_voltage_sel 
have confusion too.
Except that, Looks OK.

Thanks,

Re: [PATCH] regulator: Rename s5m8767_convert_voltage to s5m8767_get_proper_voltage_sel

[Axel Lin]

Hi Sangbeom,

2012/3/9 Sangbeom Kim <sbkim73@samsung.com>:
> Hi!
>
> On Wednesday, Mar 07, 2012 at 1:12AM, Axel Lin wrote:
>> -static inline int s5m8767_convert_voltage(
>> +static int s5m8767_get_proper_voltage_sel(
>
> s5m8767_get_voltage_sel and s5m8767_get_proper_voltage_sel
> have confusion too.

How about s5m8767_convert_voltage_to_proper_sel ?

If it is ok for you, I'll send the patch right away. :-)

Regards,
Axel

RE: [PATCH] regulator: Rename s5m8767_convert_voltage to s5m8767_get_proper_voltage_sel

[Sangbeom Kim]

Hi Axel,

On Friday, Mar 09, 2012 at 11:40 AM, Axel Lin wrote:
> How about s5m8767_convert_voltage_to_proper_sel ?
It looks too long.
How about s5m8767_convert_voltage_to_sel ?

Thanks,
Sangbeom.