Skip to contentSkip to conversation

regulator: Simplify MAX8997_REG_BUCK1DVS/MAX8997_REG_BUCK2DVS/MAX8997_REG_BUCK5DVS macros

PATCH 함명주 2 participants 3 messages May 16, 2011

Subsystems (2)
Multifunction devices (MFD) Voltage and current regulator framework
lore
Share & tools
lore

함명주

author
#0 lore raw

Hello,

Quoted 11 lines

Sender : Axel Lin<[email protected]>
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.

Quoted 3 lines

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:

---

drivers/regulator/max8997.c +4 -4
index b1c1444..aad85e4 100644
@@ -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?

Quoted 64 lines

---
drivers/regulator/max8997.c | 12 ++++++------
include/linux/mfd/max8997-private.h | 6 +++---

2 files changed, 9 insertions(+), 9 deletions(-)
drivers/regulator/max8997.c +6 -6
index b1c1444..664a31f 100644
@@ -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);
 }
include/linux/mfd/max8997-private.h +3 -3
index 69d1010..bf580a4 100644
@@ -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 {
Signature · 1 line
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�+돴윯돪�+%듚�깁負쪐w��{.n�+돴쪐{깸G쳺��{ay����,j�f＂톒쉹��묎�z_溫(�쉸듶줷"얎�m����G쳺�◀?솳鈺�&ｘ�~뤳턨O뺞촾퇉v�^�m��� �띠�◀?뻀�

#1 lore raw

Hi MyungJoo,

2011/5/16 함명주 <[email protected]>:

Quoted 13 lines

Hello,

Quoted 11 lines

Sender : Axel Lin<[email protected]>
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.

Quoted 7 lines
Quoted 3 lines

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.

Ok. I got your point for the implementation.

Quoted 27 lines

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:

---

drivers/regulator/max8997.c +4 -5
index b1c1444..aad85e4 100644
@@ -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

Quoted 2 lines

How about this one?

But it doesn't apply for below case because max8997->buck1_vol[i] is start from 0.

        for (i = 0; i < 8; i++) {
                max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i),
                                max8997->buck1_vol[i],
                                0x3f);

Maybe consider to remove the macro may make things simpler. If you agree, I'll send a v2.

drivers/regulator/max8997.c +0 -0
index b1c1444..10d5a1d 100644
@@ -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_BUCK1DVS1 + i,
max_buck1, 0x3f);
- max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i + 1),
+ max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS1 + i,
max_buck2, 0x3f);
- max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i + 1),
+ max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS1 + 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_BUCK1DVS1 + i,
max8997->buck1_vol[i], 0x3f);
- max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i + 1),
+ max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS1 + i,
max8997->buck2_vol[i], 0x3f);
- max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i + 1),
+ max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS1 + i,
max8997->buck5_vol[i], 0x3f);
}

(quoted diff) +0 -0

b/include/linux/mfd/max8997-private.h
index 69d1010..5ff2400 100644

include/linux/mfd/max8997-private.h +0 -4
@@ -311,10 +311,6 @@ 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_NUM_GPIO	12
 struct max8997_dev {
 	struct device *dev;

Regards,
Axel

author
#2 lore raw

Hello Axel,

Quoted 59 lines · Axel Lin

Hi MyungJoo,

2011/5/16 함명주 <[email protected]>:

Quoted 13 lines

Hello,

Quoted 11 lines

Sender : Axel Lin<[email protected]>
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.

Quoted 7 lines
Quoted 3 lines

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.

Ok. I got your point for the implementation.

Quoted 27 lines

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:

---

drivers/regulator/max8997.c +4 -5
index b1c1444..aad85e4 100644
@@ -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

Quoted 2 lines

How about this one?

But it doesn't apply for below case because max8997->buck1_vol[i] is start from 0.

Quoted 7 lines

       for (i = 0; i < 8; i++) { max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i), max8997->buck1_vol[i], 0x3f);

Maybe consider to remove the macro may make things simpler. If you agree, I'll send a v2.

Yes, that looks just fine to me.

Thank you.

- MyungJoo

Quoted 60 lines
drivers/regulator/max8997.c +6 -6
index b1c1444..10d5a1d 100644
@@ -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_BUCK1DVS1 + i,
                                max_buck1, 0x3f);
-               max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i + 1),
+               max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS1 + i,
                                max_buck2, 0x3f);
-               max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i + 1),
+               max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS1 + 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_BUCK1DVS1 + i,
                                max8997->buck1_vol[i],
                                0x3f);
-               max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i + 1),
+               max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS1 + i,
                                max8997->buck2_vol[i],
                                0x3f);
-               max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i + 1),
+               max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS1 + i,
                                max8997->buck5_vol[i],
                                0x3f);

       }

include/linux/mfd/max8997-private.h +0 -4
index 69d1010..5ff2400 100644
@@ -311,10 +311,6 @@ 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_NUM_GPIO       12
  struct max8997_dev {
        struct device *dev;

Regards,
Axel

Signature · 5 lines
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858