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]
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

Powered by blists - more mailing lists