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
| ||
|
Date: Mon, 16 May 2011 17:29:51 +0900 From: MyungJoo Ham <myungjoo.ham@...sung.com> To: axel.lin@...il.com Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 박경민 <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> Subject: Re: [PATCH] regulator: Simplify MAX8997_REG_BUCK1DVS/MAX8997_REG_BUCK2DVS/MAX8997_REG_BUCK5DVS macros Hello Axel, On Mon, May 16, 2011 at 5:05 PM, Axel Lin <axel.lin@...il.com> wrote: > Hi MyungJoo, > > 2011/5/16 함명주 <myungjoo.ham@...sung.com>: >> 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. > > Ok. I got your point for the implementation. > >> 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? >> > 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. Yes, that looks just fine to me. Thank you. - MyungJoo > > diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c > index b1c1444..10d5a1d 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_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); > } > diff --git a/include/linux/mfd/max8997-private.h > b/include/linux/mfd/max8997-private.h > index 69d1010..5ff2400 100644 > --- a/include/linux/mfd/max8997-private.h > +++ b/include/linux/mfd/max8997-private.h > @@ -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 > -- MyungJoo Ham (함명주), Ph.D. Mobile Software Platform Lab, Digital Media and Communications (DMC) Business Samsung Electronics cell: 82-10-6714-2858 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists