2012-06-20 05:21:05

by Yadwinder Singh Brar

[permalink] [raw]
Subject: [PATCH v2] regulator: max77686: Implement .set_ramp_delay() callback.

This patch implements the .set_ramp_delay callback to set the ramp_delay on
hardware for BUCK2/3/4 if ramp_delay is set in regulator constraints.

This patch also do some cleaning work for unrequired members of
struct max77686_data.

Signed-off-by: Yadwinder Singh Brar <[email protected]>
---
drivers/regulator/max77686.c | 37 +++++++++++++++++++++++++++++--------
1 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index a29eee3..5c2a32f 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -65,11 +65,34 @@ enum max77686_ramp_rate {
};

struct max77686_data {
- struct device *dev;
- struct max77686_dev *iodev;
struct regulator_dev **rdev;
};

+static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
+{
+ unsigned int ramp_value = RAMP_RATE_NO_CTRL;
+
+ switch (ramp_delay) {
+ case 1 ... 13750:
+ ramp_value = RAMP_RATE_13P75MV;
+ break;
+ case 13751 ... 27500:
+ ramp_value = RAMP_RATE_27P5MV;
+ break;
+ case 27501 ... 55000:
+ ramp_value = RAMP_RATE_55MV;
+ break;
+ case 55001 ... 100000:
+ break;
+ default:
+ pr_warn("%s: ramp_delay: %d not supported, setting 100000\n",
+ rdev->desc->name, ramp_delay);
+ }
+
+ return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+ MAX77686_RAMP_RATE_MASK, ramp_value << 6);
+}
+
static struct regulator_ops max77686_ops = {
.list_voltage = regulator_list_voltage_linear,
.map_voltage = regulator_map_voltage_linear,
@@ -90,6 +113,7 @@ static struct regulator_ops max77686_buck_dvs_ops = {
.get_voltage_sel = regulator_get_voltage_sel_regmap,
.set_voltage_sel = regulator_set_voltage_sel_regmap,
.set_voltage_time_sel = regulator_set_voltage_time_sel,
+ .set_ramp_delay = max77686_set_ramp_delay,
};

#define regulator_desc_ldo(num) { \
@@ -238,20 +262,17 @@ static __devinit int max77686_pmic_probe(struct platform_device *pdev)
return -ENOMEM;

rdev = max77686->rdev;
- max77686->dev = &pdev->dev;
- max77686->iodev = iodev;
+ config.dev = &pdev->dev;
+ config.regmap = iodev->regmap;
platform_set_drvdata(pdev, max77686);

for (i = 0; i < MAX77686_REGULATORS; i++) {
- config.dev = max77686->dev;
- config.regmap = iodev->regmap;
- config.driver_data = max77686;
config.init_data = pdata->regulators[i].initdata;

rdev[i] = regulator_register(&regulators[i], &config);
if (IS_ERR(rdev[i])) {
ret = PTR_ERR(rdev[i]);
- dev_err(max77686->dev,
+ dev_err(&pdev->dev,
"regulator init failed for %d\n", i);
rdev[i] = NULL;
goto err;
--
1.7.0.4


2012-06-20 06:56:30

by MyungJoo Ham

[permalink] [raw]
Subject: Re: [PATCH v2] regulator: max77686: Implement .set_ramp_delay() callback.

> This patch implements the .set_ramp_delay callback to set the ramp_delay on
> hardware for BUCK2/3/4 if ramp_delay is set in regulator constraints.
>
> This patch also do some cleaning work for unrequired members of
> struct max77686_data.
>
> Signed-off-by: Yadwinder Singh Brar <[email protected]>

I guess code-clean part may be seperated. Anyway, except that, it looks good.


Acked-by: MyungJoo Ham <[email protected]>

> ---
> drivers/regulator/max77686.c | 37 +++++++++++++++++++++++++++++--------
> 1 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index a29eee3..5c2a32f 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c
> @@ -65,11 +65,34 @@ enum max77686_ramp_rate {
> };
>
> struct max77686_data {
> - struct device *dev;
> - struct max77686_dev *iodev;
> struct regulator_dev **rdev;
> };
>
> +static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> +{
> + unsigned int ramp_value = RAMP_RATE_NO_CTRL;
> +
> + switch (ramp_delay) {
> + case 1 ... 13750:
> + ramp_value = RAMP_RATE_13P75MV;
> + break;
> + case 13751 ... 27500:
> + ramp_value = RAMP_RATE_27P5MV;
> + break;
> + case 27501 ... 55000:
> + ramp_value = RAMP_RATE_55MV;
> + break;
> + case 55001 ... 100000:
> + break;
> + default:
> + pr_warn("%s: ramp_delay: %d not supported, setting 100000\n",
> + rdev->desc->name, ramp_delay);
> + }
> +
> + return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> + MAX77686_RAMP_RATE_MASK, ramp_value << 6);
> +}
> +
> static struct regulator_ops max77686_ops = {
> .list_voltage = regulator_list_voltage_linear,
> .map_voltage = regulator_map_voltage_linear,
> @@ -90,6 +113,7 @@ static struct regulator_ops max77686_buck_dvs_ops = {
> .get_voltage_sel = regulator_get_voltage_sel_regmap,
> .set_voltage_sel = regulator_set_voltage_sel_regmap,
> .set_voltage_time_sel = regulator_set_voltage_time_sel,
> + .set_ramp_delay = max77686_set_ramp_delay,
> };
>
> #define regulator_desc_ldo(num) { \
> @@ -238,20 +262,17 @@ static __devinit int max77686_pmic_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> rdev = max77686->rdev;
> - max77686->dev = &pdev->dev;
> - max77686->iodev = iodev;
> + config.dev = &pdev->dev;
> + config.regmap = iodev->regmap;
> platform_set_drvdata(pdev, max77686);
>
> for (i = 0; i < MAX77686_REGULATORS; i++) {
> - config.dev = max77686->dev;
> - config.regmap = iodev->regmap;
> - config.driver_data = max77686;
> config.init_data = pdata->regulators[i].initdata;
>
> rdev[i] = regulator_register(&regulators[i], &config);
> if (IS_ERR(rdev[i])) {
> ret = PTR_ERR(rdev[i]);
> - dev_err(max77686->dev,
> + dev_err(&pdev->dev,
> "regulator init failed for %d\n", i);
> rdev[i] = NULL;
> goto err;
> --
> 1.7.0.4
>
>
>
>
>
>
>
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2012-06-20 07:08:28

by Axel Lin

[permalink] [raw]
Subject: Re: [PATCH v2] regulator: max77686: Implement .set_ramp_delay() callback.

2012/6/20 Yadwinder Singh Brar <[email protected]>:
> This patch implements the .set_ramp_delay callback to set the ramp_delay on
> hardware for BUCK2/3/4 if ramp_delay is set in regulator constraints.
>
> This patch also do some cleaning work for unrequired members of
> struct max77686_data.
>
> Signed-off-by: Yadwinder Singh Brar <[email protected]>
> ---
> ?drivers/regulator/max77686.c | ? 37 +++++++++++++++++++++++++++++--------
> ?1 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index a29eee3..5c2a32f 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c
> @@ -65,11 +65,34 @@ enum max77686_ramp_rate {
> ?};
>
> ?struct max77686_data {
> - ? ? ? struct device *dev;
> - ? ? ? struct max77686_dev *iodev;
> ? ? ? ?struct regulator_dev **rdev;
> ?};
>
> +static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> +{
> + ? ? ? unsigned int ramp_value = RAMP_RATE_NO_CTRL;
> +
> + ? ? ? switch (ramp_delay) {
> + ? ? ? case 1 ... 13750:
> + ? ? ? ? ? ? ? ramp_value = RAMP_RATE_13P75MV;
> + ? ? ? ? ? ? ? break;
> + ? ? ? case 13751 ... 27500:
> + ? ? ? ? ? ? ? ramp_value = RAMP_RATE_27P5MV;
> + ? ? ? ? ? ? ? break;
> + ? ? ? case 27501 ... 55000:
> + ? ? ? ? ? ? ? ramp_value = RAMP_RATE_55MV;
> + ? ? ? ? ? ? ? break;
> + ? ? ? case 55001 ... 100000:
> + ? ? ? ? ? ? ? break;
> + ? ? ? default:
> + ? ? ? ? ? ? ? pr_warn("%s: ramp_delay: %d not supported, setting 100000\n",
> + ? ? ? ? ? ? ? ? ? ? ? rdev->desc->name, ramp_delay);
Hi Yadwinder,
I'm a bit confused about this default setting.
If set_ramp_delay() is not called, the default setting for BUCK234 is
MAX77686_DVS_RAMP_DELAY (27500 uV/uS).
So why you set default to 100000 here.

Regards,
Axel

2012-06-20 10:18:27

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v2] regulator: max77686: Implement .set_ramp_delay() callback.

On Wed, Jun 20, 2012 at 10:50:51AM +0530, Yadwinder Singh Brar wrote:
> This patch implements the .set_ramp_delay callback to set the ramp_delay on
> hardware for BUCK2/3/4 if ramp_delay is set in regulator constraints.

Applied, thanks. It does seem reasonable to set the fastest ramp if the
ramp rate requested is over the maximum rate suported.

> This patch also do some cleaning work for unrequired members of
> struct max77686_data.

As Myungjoo said this really should have been split into a separate
patch, please don't do this in future. One patch per change.


Attachments:
(No filename) (571.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments