* [PATCH RFT 1/2] regulator: ab8500: Fix set voltage for AB8540_LDO_AUX3 @ 2013-04-10 6:40 Axel Lin 2013-04-10 6:46 ` [PATCH RFT 2/2] regulator: ab8500: Optimize ab8540_aux3_regulator_get_voltage_sel Axel Lin 2013-04-16 10:57 ` [PATCH RFT 1/2] regulator: ab8500: Fix set voltage for AB8540_LDO_AUX3 Bengt Jönsson 0 siblings, 2 replies; 5+ messages in thread From: Axel Lin @ 2013-04-10 6:40 UTC (permalink / raw) To: Mark Brown Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel When setting voltage for AB8540_LDO_AUX3, current code only updates one of info->voltage_reg and info->expand_register registers which is wrong. To ensure we set to correct voltage, it always needs to clear or set expand_register.voltage_mask bit of expand_register. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- drivers/regulator/ab8500.c | 55 ++++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c index 9ebd131..222a63e 100644 --- a/drivers/regulator/ab8500.c +++ b/drivers/regulator/ab8500.c @@ -605,39 +605,54 @@ static int ab8540_aux3_regulator_set_voltage_sel(struct regulator_dev *rdev, { int ret; struct ab8500_regulator_info *info = rdev_get_drvdata(rdev); - u8 regval; + u8 regval, regval_expand; if (info == NULL) { dev_err(rdev_get_dev(rdev), "regulator info null pointer\n"); return -EINVAL; } - if (selector >= info->expand_register.voltage_limit) { - /* Vaux3 bit4 has different layout */ - regval = (u8)selector << info->expand_register.voltage_shift; - ret = abx500_mask_and_set_register_interruptible(info->dev, - info->expand_register.voltage_bank, - info->expand_register.voltage_reg, - info->expand_register.voltage_mask, - regval); - } else { - /* set the registers for the request */ - regval = (u8)selector << info->voltage_shift; - ret = abx500_mask_and_set_register_interruptible(info->dev, + if (selector >= info->expand_register.voltage_limit) + regval_expand = info->expand_register.voltage_mask; + else + regval_expand = 0; + + ret = abx500_mask_and_set_register_interruptible(info->dev, + info->expand_register.voltage_bank, + info->expand_register.voltage_reg, + info->expand_register.voltage_mask, + regval_expand); + if (ret < 0) { + dev_err(rdev_get_dev(rdev), + "couldn't set expand voltage reg for regulator\n"); + return ret; + } + + dev_vdbg(rdev_get_dev(rdev), + "%s-set_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n", + info->desc.name, info->expand_register.voltage_bank, + info->expand_register.voltage_reg, + info->expand_register.voltage_mask, regval_expand); + + if (selector >= info->expand_register.voltage_limit) + return 0; + + regval = (u8)selector << info->voltage_shift; + ret = abx500_mask_and_set_register_interruptible(info->dev, info->voltage_bank, info->voltage_reg, info->voltage_mask, regval); - } - if (ret < 0) + if (ret < 0) { dev_err(rdev_get_dev(rdev), "couldn't set voltage reg for regulator\n"); + return ret; + } dev_vdbg(rdev_get_dev(rdev), - "%s-set_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x," - " 0x%x\n", - info->desc.name, info->voltage_bank, info->voltage_reg, - info->voltage_mask, regval); + "%s-set_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n", + info->desc.name, info->voltage_bank, info->voltage_reg, + info->voltage_mask, regval); - return ret; + return 0; } static struct regulator_ops ab8500_regulator_volt_mode_ops = { -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH RFT 2/2] regulator: ab8500: Optimize ab8540_aux3_regulator_get_voltage_sel 2013-04-10 6:40 [PATCH RFT 1/2] regulator: ab8500: Fix set voltage for AB8540_LDO_AUX3 Axel Lin @ 2013-04-10 6:46 ` Axel Lin 2013-04-16 11:12 ` Bengt Jönsson 2013-04-16 11:24 ` Mark Brown 2013-04-16 10:57 ` [PATCH RFT 1/2] regulator: ab8500: Fix set voltage for AB8540_LDO_AUX3 Bengt Jönsson 1 sibling, 2 replies; 5+ messages in thread From: Axel Lin @ 2013-04-10 6:46 UTC (permalink / raw) To: Mark Brown Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel We can save a register read operation in some case if read expand_register first. If info->expand_register.voltage_mask bit is set, no need to read voltage_reg. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- drivers/regulator/ab8500.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c index 222a63e..7a86fe6 100644 --- a/drivers/regulator/ab8500.c +++ b/drivers/regulator/ab8500.c @@ -521,7 +521,7 @@ static int ab8500_regulator_get_voltage_sel(struct regulator_dev *rdev) static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev) { - int ret, val; + int ret; struct ab8500_regulator_info *info = rdev_get_drvdata(rdev); u8 regval, regval_expand; @@ -531,18 +531,25 @@ static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev) } ret = abx500_get_register_interruptible(info->dev, - info->voltage_bank, info->voltage_reg, ®val); - + info->expand_register.voltage_bank, + info->expand_register.voltage_reg, ®val_expand); if (ret < 0) { dev_err(rdev_get_dev(rdev), - "couldn't read voltage reg for regulator\n"); + "couldn't read voltage expand reg for regulator\n"); return ret; } - ret = abx500_get_register_interruptible(info->dev, - info->expand_register.voltage_bank, - info->expand_register.voltage_reg, ®val_expand); + dev_vdbg(rdev_get_dev(rdev), + "%s-get_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n", + info->desc.name, info->expand_register.voltage_bank, + info->expand_register.voltage_reg, + info->expand_register.voltage_mask, regval_expand); + + if (regval_expand & info->expand_register.voltage_mask) + return info->expand_register.voltage_limit; + ret = abx500_get_register_interruptible(info->dev, + info->voltage_bank, info->voltage_reg, ®val); if (ret < 0) { dev_err(rdev_get_dev(rdev), "couldn't read voltage reg for regulator\n"); @@ -550,24 +557,11 @@ static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev) } dev_vdbg(rdev_get_dev(rdev), - "%s-get_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x," - " 0x%x\n", - info->desc.name, info->voltage_bank, info->voltage_reg, - info->voltage_mask, regval); - dev_vdbg(rdev_get_dev(rdev), - "%s-get_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x," - " 0x%x\n", - info->desc.name, info->expand_register.voltage_bank, - info->expand_register.voltage_reg, - info->expand_register.voltage_mask, regval_expand); - - if (regval_expand&(info->expand_register.voltage_mask)) - /* Vaux3 has a different layout */ - val = info->expand_register.voltage_limit; - else - val = (regval & info->voltage_mask) >> info->voltage_shift; + "%s-get_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n", + info->desc.name, info->voltage_bank, info->voltage_reg, + info->voltage_mask, regval); - return val; + return (regval & info->voltage_mask) >> info->voltage_shift; } static int ab8500_regulator_set_voltage_sel(struct regulator_dev *rdev, -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH RFT 2/2] regulator: ab8500: Optimize ab8540_aux3_regulator_get_voltage_sel 2013-04-10 6:46 ` [PATCH RFT 2/2] regulator: ab8500: Optimize ab8540_aux3_regulator_get_voltage_sel Axel Lin @ 2013-04-16 11:12 ` Bengt Jönsson 2013-04-16 11:24 ` Mark Brown 1 sibling, 0 replies; 5+ messages in thread From: Bengt Jönsson @ 2013-04-16 11:12 UTC (permalink / raw) To: Axel Lin Cc: Mark Brown, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel@vger.kernel.org On 04/10/2013 08:46 AM, Axel Lin wrote: > We can save a register read operation in some case if read > expand_register first. > If info->expand_register.voltage_mask bit is set, no need to read > voltage_reg. > > Signed-off-by: Axel Lin <axel.lin@ingics.com> Looks good. Acked-by: Bengt Jonsson <bengt.g.jonsson@stericsson.com> > --- > drivers/regulator/ab8500.c | 42 ++++++++++++++++++------------------------ > 1 file changed, 18 insertions(+), 24 deletions(-) > > diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c > index 222a63e..7a86fe6 100644 > --- a/drivers/regulator/ab8500.c > +++ b/drivers/regulator/ab8500.c > @@ -521,7 +521,7 @@ static int ab8500_regulator_get_voltage_sel(struct regulator_dev *rdev) > > static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev) > { > - int ret, val; > + int ret; > struct ab8500_regulator_info *info = rdev_get_drvdata(rdev); > u8 regval, regval_expand; > > @@ -531,18 +531,25 @@ static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev) > } > > ret = abx500_get_register_interruptible(info->dev, > - info->voltage_bank, info->voltage_reg, ®val); > - > + info->expand_register.voltage_bank, > + info->expand_register.voltage_reg, ®val_expand); > if (ret < 0) { > dev_err(rdev_get_dev(rdev), > - "couldn't read voltage reg for regulator\n"); > + "couldn't read voltage expand reg for regulator\n"); > return ret; > } > > - ret = abx500_get_register_interruptible(info->dev, > - info->expand_register.voltage_bank, > - info->expand_register.voltage_reg, ®val_expand); > + dev_vdbg(rdev_get_dev(rdev), > + "%s-get_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n", > + info->desc.name, info->expand_register.voltage_bank, > + info->expand_register.voltage_reg, > + info->expand_register.voltage_mask, regval_expand); > + > + if (regval_expand & info->expand_register.voltage_mask) > + return info->expand_register.voltage_limit; > > + ret = abx500_get_register_interruptible(info->dev, > + info->voltage_bank, info->voltage_reg, ®val); > if (ret < 0) { > dev_err(rdev_get_dev(rdev), > "couldn't read voltage reg for regulator\n"); > @@ -550,24 +557,11 @@ static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev) > } > > dev_vdbg(rdev_get_dev(rdev), > - "%s-get_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x," > - " 0x%x\n", > - info->desc.name, info->voltage_bank, info->voltage_reg, > - info->voltage_mask, regval); > - dev_vdbg(rdev_get_dev(rdev), > - "%s-get_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x," > - " 0x%x\n", > - info->desc.name, info->expand_register.voltage_bank, > - info->expand_register.voltage_reg, > - info->expand_register.voltage_mask, regval_expand); > - > - if (regval_expand&(info->expand_register.voltage_mask)) > - /* Vaux3 has a different layout */ > - val = info->expand_register.voltage_limit; > - else > - val = (regval & info->voltage_mask) >> info->voltage_shift; > + "%s-get_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n", > + info->desc.name, info->voltage_bank, info->voltage_reg, > + info->voltage_mask, regval); > > - return val; > + return (regval & info->voltage_mask) >> info->voltage_shift; > } > > static int ab8500_regulator_set_voltage_sel(struct regulator_dev *rdev, ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFT 2/2] regulator: ab8500: Optimize ab8540_aux3_regulator_get_voltage_sel 2013-04-10 6:46 ` [PATCH RFT 2/2] regulator: ab8500: Optimize ab8540_aux3_regulator_get_voltage_sel Axel Lin 2013-04-16 11:12 ` Bengt Jönsson @ 2013-04-16 11:24 ` Mark Brown 1 sibling, 0 replies; 5+ messages in thread From: Mark Brown @ 2013-04-16 11:24 UTC (permalink / raw) To: Axel Lin Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel [-- Attachment #1: Type: text/plain, Size: 245 bytes --] On Wed, Apr 10, 2013 at 02:46:20PM +0800, Axel Lin wrote: > We can save a register read operation in some case if read > expand_register first. > If info->expand_register.voltage_mask bit is set, no need to read > voltage_reg. Applied, thanks. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFT 1/2] regulator: ab8500: Fix set voltage for AB8540_LDO_AUX3 2013-04-10 6:40 [PATCH RFT 1/2] regulator: ab8500: Fix set voltage for AB8540_LDO_AUX3 Axel Lin 2013-04-10 6:46 ` [PATCH RFT 2/2] regulator: ab8500: Optimize ab8540_aux3_regulator_get_voltage_sel Axel Lin @ 2013-04-16 10:57 ` Bengt Jönsson 1 sibling, 0 replies; 5+ messages in thread From: Bengt Jönsson @ 2013-04-16 10:57 UTC (permalink / raw) To: Axel Lin Cc: Mark Brown, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel@vger.kernel.org On 04/10/2013 08:40 AM, Axel Lin wrote: > When setting voltage for AB8540_LDO_AUX3, current code only updates one of > info->voltage_reg and info->expand_register registers which is wrong. > To ensure we set to correct voltage, it always needs to clear or set > expand_register.voltage_mask bit of expand_register. The original code is wrong. It is not possible to leave 3.05 V once set. The function of the expand register bit is the following (from the user manual): 0: VAUX3 output voltage is determined by Vaux3Sel bit settings in register VldoCVaux3Sel 1: VAUX3 output voltage is set to 3.05 V regardless of Vaux3Sel settings in register VldoCVaux3Sel (VldoCVaux3Sel is the register at 0x0421) So when going to 3.05 V I would like to set the bit as you are doing. But when leaving 3.05 V for another voltage I would prefer setting the target voltage before clearing the expand register bit. > > Signed-off-by: Axel Lin <axel.lin@ingics.com> > --- > drivers/regulator/ab8500.c | 55 ++++++++++++++++++++++++++++---------------- > 1 file changed, 35 insertions(+), 20 deletions(-) > > diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c > index 9ebd131..222a63e 100644 > --- a/drivers/regulator/ab8500.c > +++ b/drivers/regulator/ab8500.c > @@ -605,39 +605,54 @@ static int ab8540_aux3_regulator_set_voltage_sel(struct regulator_dev *rdev, > { > int ret; > struct ab8500_regulator_info *info = rdev_get_drvdata(rdev); > - u8 regval; > + u8 regval, regval_expand; > > if (info == NULL) { > dev_err(rdev_get_dev(rdev), "regulator info null pointer\n"); > return -EINVAL; > } > > - if (selector >= info->expand_register.voltage_limit) { > - /* Vaux3 bit4 has different layout */ > - regval = (u8)selector << info->expand_register.voltage_shift; > - ret = abx500_mask_and_set_register_interruptible(info->dev, > - info->expand_register.voltage_bank, > - info->expand_register.voltage_reg, > - info->expand_register.voltage_mask, > - regval); > - } else { > - /* set the registers for the request */ > - regval = (u8)selector << info->voltage_shift; > - ret = abx500_mask_and_set_register_interruptible(info->dev, > + if (selector >= info->expand_register.voltage_limit) > + regval_expand = info->expand_register.voltage_mask; > + else > + regval_expand = 0; > + > + ret = abx500_mask_and_set_register_interruptible(info->dev, > + info->expand_register.voltage_bank, > + info->expand_register.voltage_reg, > + info->expand_register.voltage_mask, > + regval_expand); > + if (ret < 0) { > + dev_err(rdev_get_dev(rdev), > + "couldn't set expand voltage reg for regulator\n"); > + return ret; > + } > + > + dev_vdbg(rdev_get_dev(rdev), > + "%s-set_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n", > + info->desc.name, info->expand_register.voltage_bank, > + info->expand_register.voltage_reg, > + info->expand_register.voltage_mask, regval_expand); > + > + if (selector >= info->expand_register.voltage_limit) > + return 0; > + > + regval = (u8)selector << info->voltage_shift; > + ret = abx500_mask_and_set_register_interruptible(info->dev, > info->voltage_bank, info->voltage_reg, > info->voltage_mask, regval); > - } > - if (ret < 0) > + if (ret < 0) { > dev_err(rdev_get_dev(rdev), > "couldn't set voltage reg for regulator\n"); > + return ret; > + } > > dev_vdbg(rdev_get_dev(rdev), > - "%s-set_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x," > - " 0x%x\n", > - info->desc.name, info->voltage_bank, info->voltage_reg, > - info->voltage_mask, regval); > + "%s-set_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n", > + info->desc.name, info->voltage_bank, info->voltage_reg, > + info->voltage_mask, regval); > > - return ret; > + return 0; > } > > static struct regulator_ops ab8500_regulator_volt_mode_ops = { ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-04-16 11:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-10 6:40 [PATCH RFT 1/2] regulator: ab8500: Fix set voltage for AB8540_LDO_AUX3 Axel Lin 2013-04-10 6:46 ` [PATCH RFT 2/2] regulator: ab8500: Optimize ab8540_aux3_regulator_get_voltage_sel Axel Lin 2013-04-16 11:12 ` Bengt Jönsson 2013-04-16 11:24 ` Mark Brown 2013-04-16 10:57 ` [PATCH RFT 1/2] regulator: ab8500: Fix set voltage for AB8540_LDO_AUX3 Bengt Jönsson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox