Re: [PATCH] regulator: Simplify MAX8997_REG_BUCK1DVS/MAX8997_REG_BUCK2DVS/MAX8997_REG_BUCK5DVS macros

[함명주 <myungjoo.ham@samsung.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 4452 bytes --]

Hello,

> Sender : Axel Lin<axel.lin@gmail.com>
> Date : 2011-05-16 15:52 (GMT+09:00)
> Title : [PATCH] regulator: Simplify MAX8997_REG_BUCK1DVS/MAX8997_REG_BUCK2DVS/MAX8997_REG_BUCK5DVS macros
> 
> Looks like the original macro implementation assumes the caller pass the
> parameter starting from 1.
> Since now we have +1 operation from all the caller, it would be easier to
> assume the caller pass the parameter starting from 0.
> Then we can simplify the +1 operation from the caller and then -1 operation
> in the macro implementation.
> 
> I think this change also improves readability.
> 
> Signed-off-by: Axel Lin 

Well, I'd rather just change the for loop statement for your purpose, which I also agree.

The reason I've used BUCKxDVS1 as the starting point is because of the register names; register names of BUCKxDVS starts from 1, not from 0.

Thus, in order to maintain the consistency between the code and the chip manual, I'd rather not change that part, but change like this:

---
diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c
index b1c1444..aad85e4 100644
--- a/drivers/regulator/max8997.c
+++ b/drivers/regulator/max8997.c
@@ -1031,12 +1031,12 @@ static __devinit int max8997_pmic_probe(struct platform_device *pdev)
 	}
 
 	/* For the safety, set max voltage before setting up */
-	for (i = 0; i < 8; i++) {
-		max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i + 1),
+	for (i = 1; i <= 8; i++) {
+		max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i),
 				max_buck1, 0x3f);
-		max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i + 1),
+		max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i),
 				max_buck2, 0x3f);
-		max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i + 1),
+		max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i),
 				max_buck5, 0x3f);
 	}

--
1.7.4.1

How about this one?

> ---
> drivers/regulator/max8997.c         |   12 ++++++------
> include/linux/mfd/max8997-private.h |    6 +++---
> 2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c
> index b1c1444..664a31f 100644
> --- a/drivers/regulator/max8997.c
> +++ b/drivers/regulator/max8997.c
> @@ -1032,11 +1032,11 @@ static __devinit int max8997_pmic_probe(struct platform_device *pdev)
> 
> /* For the safety, set max voltage before setting up */
> for (i = 0; i < 8; i++) {
> - max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i + 1),
> + max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i),
> max_buck1, 0x3f);
> - max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i + 1),
> + max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i),
> max_buck2, 0x3f);
> - max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i + 1),
> + max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i),
> max_buck5, 0x3f);
> }
> 
> @@ -1113,13 +1113,13 @@ static __devinit int max8997_pmic_probe(struct platform_device *pdev)
> 
> /* Initialize all the DVS related BUCK registers */
> for (i = 0; i < 8; i++) {
> - max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i + 1),
> + max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i),
> max8997->buck1_vol[i],
> 0x3f);
> - max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i + 1),
> + max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i),
> max8997->buck2_vol[i],
> 0x3f);
> - max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i + 1),
> + max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i),
> max8997->buck5_vol[i],
> 0x3f);
> }
> diff --git a/include/linux/mfd/max8997-private.h b/include/linux/mfd/max8997-private.h
> index 69d1010..bf580a4 100644
> --- a/include/linux/mfd/max8997-private.h
> +++ b/include/linux/mfd/max8997-private.h
> @@ -311,9 +311,9 @@ enum max8997_irq {
> MAX8997_IRQ_NR,
> };
> 
> -#define MAX8997_REG_BUCK1DVS(x) (MAX8997_REG_BUCK1DVS1 + (x) - 1)
> -#define MAX8997_REG_BUCK2DVS(x) (MAX8997_REG_BUCK2DVS1 + (x) - 1)
> -#define MAX8997_REG_BUCK5DVS(x) (MAX8997_REG_BUCK5DVS1 + (x) - 1)
> +#define MAX8997_REG_BUCK1DVS(x) (MAX8997_REG_BUCK1DVS1 + (x))
> +#define MAX8997_REG_BUCK2DVS(x) (MAX8997_REG_BUCK2DVS1 + (x))
> +#define MAX8997_REG_BUCK5DVS(x) (MAX8997_REG_BUCK5DVS1 + (x))
> 
> #define MAX8997_NUM_GPIO 12
> struct max8997_dev {
> -- 
> 1.7.1
> 
> 
> 
> 



 MyungJoo Ham (ÇÔ¸íÁÖ)
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: +82-10-6714-2858 / office: +82-31-279-8033ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥