| Message ID | 1414411911-5539-3-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Krzysztof, On 10/27/2014 01:11 PM, Krzysztof Kozlowski wrote: > Introduce simple helper for calculating the shift for OPMODE field in > registers. This allows storing the current value of opmode in > non-shifted form and simplifies a little set_suspend_disable and enable > functions. Additionally this will allow adding support LDOs to the > existing set_suspend_disable function. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > drivers/regulator/max77686.c | 49 ++++++++++++++++++++++++++++++-------------- > 1 file changed, 34 insertions(+), 15 deletions(-) > > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c The patch looks good to me. Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > > static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay) > @@ -495,7 +513,8 @@ static int max77686_pmic_probe(struct platform_device *pdev) > config.init_data = pdata->regulators[i].initdata; > config.of_node = pdata->regulators[i].of_node; > > - max77686->opmode[i] = regulators[i].enable_mask; > + max77686->opmode[i] = regulators[i].enable_mask >> > + max77686_get_opmode_shift(i); I don't think it has to be done in your patch but as a follow-up it would be good to change this to: max77686->opmode[i] = MAX77686_NORMAL; since that is really what this code is trying to do AFAIU. This just happens to work because the value of MAX77686_OPMODE_MASK (0x3) is also "Output ON in Normal Mode" but the code is not correct IMHO. Or even better, the regulator mode should be read from the hw registers instead of setting always to normal. That is what the max77802 driver does for example. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On pon, 2014-10-27 at 14:37 +0100, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 10/27/2014 01:11 PM, Krzysztof Kozlowski wrote: > > Introduce simple helper for calculating the shift for OPMODE field in > > registers. This allows storing the current value of opmode in > > non-shifted form and simplifies a little set_suspend_disable and enable > > functions. Additionally this will allow adding support LDOs to the > > existing set_suspend_disable function. > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > > --- > > drivers/regulator/max77686.c | 49 ++++++++++++++++++++++++++++++-------------- > > 1 file changed, 34 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c > > The patch looks good to me. > > Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > > > > > static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay) > > @@ -495,7 +513,8 @@ static int max77686_pmic_probe(struct platform_device *pdev) > > config.init_data = pdata->regulators[i].initdata; > > config.of_node = pdata->regulators[i].of_node; > > > > - max77686->opmode[i] = regulators[i].enable_mask; > > + max77686->opmode[i] = regulators[i].enable_mask >> > > + max77686_get_opmode_shift(i); > > I don't think it has to be done in your patch but as a follow-up it would be > good to change this to: > > max77686->opmode[i] = MAX77686_NORMAL; > > since that is really what this code is trying to do AFAIU. This just happens > to work because the value of MAX77686_OPMODE_MASK (0x3) is also "Output ON in > Normal Mode" but the code is not correct IMHO. It is technically correct (and regulator core use it this way if enable_val is not specified) but I agree that MAX77686_NORMAL would be more readable. I have some next patches almost ready, so I'll change it there. > Or even better, the regulator mode should be read from the hw registers instead > of setting always to normal. That is what the max77802 driver does for example. This would put trust on the bootloader... I don't know if I could trust him :) -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 27, 2014 at 01:11:48PM +0100, Krzysztof Kozlowski wrote: > Introduce simple helper for calculating the shift for OPMODE field in > registers. This allows storing the current value of opmode in > non-shifted form and simplifies a little set_suspend_disable and enable > functions. Additionally this will allow adding support LDOs to the > existing set_suspend_disable function. Applied, thanks.
diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c index c625a8a7940d..2ebc4257425b 100644 --- a/drivers/regulator/max77686.c +++ b/drivers/regulator/max77686.c @@ -85,20 +85,32 @@ struct max77686_data { unsigned int opmode[MAX77686_REGULATORS]; }; +static unsigned int max77686_get_opmode_shift(int id) +{ + switch (id) { + case MAX77686_BUCK1: + case MAX77686_BUCK5 ... MAX77686_BUCK9: + return 0; + case MAX77686_BUCK2 ... MAX77686_BUCK4: + return MAX77686_OPMODE_BUCK234_SHIFT; + default: + /* all LDOs */ + return MAX77686_OPMODE_SHIFT; + } +} + /* Some BUCKS supports Normal[ON/OFF] mode during suspend */ static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev) { - unsigned int val; + unsigned int val, shift; struct max77686_data *max77686 = rdev_get_drvdata(rdev); int ret, id = rdev_get_id(rdev); - if (id == MAX77686_BUCK1) - val = MAX77686_OFF_PWRREQ; - else - val = MAX77686_OFF_PWRREQ << MAX77686_OPMODE_BUCK234_SHIFT; + shift = max77686_get_opmode_shift(id); + val = MAX77686_OFF_PWRREQ; ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, - rdev->desc->enable_mask, val); + rdev->desc->enable_mask, val << shift); if (ret) return ret; @@ -120,10 +132,10 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev, switch (mode) { case REGULATOR_MODE_IDLE: /* ON in LP Mode */ - val = MAX77686_LDO_LOWPOWER_PWRREQ << MAX77686_OPMODE_SHIFT; + val = MAX77686_LDO_LOWPOWER_PWRREQ; break; case REGULATOR_MODE_NORMAL: /* ON in Normal Mode */ - val = MAX77686_NORMAL << MAX77686_OPMODE_SHIFT; + val = MAX77686_NORMAL; break; default: pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n", @@ -132,7 +144,8 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev, } ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, - rdev->desc->enable_mask, val); + rdev->desc->enable_mask, + val << MAX77686_OPMODE_SHIFT); if (ret) return ret; @@ -150,13 +163,13 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev, switch (mode) { case REGULATOR_MODE_STANDBY: /* switch off */ - val = MAX77686_OFF_PWRREQ << MAX77686_OPMODE_SHIFT; + val = MAX77686_OFF_PWRREQ; break; case REGULATOR_MODE_IDLE: /* ON in LP Mode */ - val = MAX77686_LDO_LOWPOWER_PWRREQ << MAX77686_OPMODE_SHIFT; + val = MAX77686_LDO_LOWPOWER_PWRREQ; break; case REGULATOR_MODE_NORMAL: /* ON in Normal Mode */ - val = MAX77686_NORMAL << MAX77686_OPMODE_SHIFT; + val = MAX77686_NORMAL; break; default: pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n", @@ -165,7 +178,8 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev, } ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, - rdev->desc->enable_mask, val); + rdev->desc->enable_mask, + val << MAX77686_OPMODE_SHIFT); if (ret) return ret; @@ -176,10 +190,14 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev, static int max77686_enable(struct regulator_dev *rdev) { struct max77686_data *max77686 = rdev_get_drvdata(rdev); + unsigned int shift; + int id = rdev_get_id(rdev); + + shift = max77686_get_opmode_shift(id); return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, rdev->desc->enable_mask, - max77686->opmode[rdev_get_id(rdev)]); + max77686->opmode[id] << shift); } static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay) @@ -495,7 +513,8 @@ static int max77686_pmic_probe(struct platform_device *pdev) config.init_data = pdata->regulators[i].initdata; config.of_node = pdata->regulators[i].of_node; - max77686->opmode[i] = regulators[i].enable_mask; + max77686->opmode[i] = regulators[i].enable_mask >> + max77686_get_opmode_shift(i); rdev = devm_regulator_register(&pdev->dev, ®ulators[i], &config); if (IS_ERR(rdev)) {
Introduce simple helper for calculating the shift for OPMODE field in registers. This allows storing the current value of opmode in non-shifted form and simplifies a little set_suspend_disable and enable functions. Additionally this will allow adding support LDOs to the existing set_suspend_disable function. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- drivers/regulator/max77686.c | 49 ++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 15 deletions(-)