linux-kernel - Re: [PATCH] regulator: Simplify MAX8997_REG_BUCK1DVS/MAX8997_REG_BUCK2DVS/MAX8997_REG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-id: <25864346.87131305530134778.JavaMail.weblogic@epml26>
Date:	Mon, 16 May 2011 07:15:35 +0000 (GMT)
From:	함명주 <myungjoo.ham@...sung.com>
To:	Axel Lin <axel.lin@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:	박경민 <kyungmin.park@...sung.com>,
	Liam Girdwood <lrg@...mlogic.co.uk>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	"myungjoo.ham@...il.com" <myungjoo.ham@...il.com>
Subject: Re: [PATCH] regulator: Simplify
 MAX8997_REG_BUCK1DVS/MAX8997_REG_BUCK2DVS/MAX8997_REG_BUCK5DVS macros

Hello,

> Sender : Axel Lin<axel.lin@...il.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