From: Steve Twiss <[email protected]>
Bug fix to allow the setting of maximum voltage for certain LDOs.
Signed-off-by: Steve Twiss <[email protected]>
---
Checks performed with next-20140211/scripts/checkpatch.pl
da9063-regulator.c total: 0 errors, 0 warnings, 927 lines checked
This patch applies against kernel version linux-next next-20140211
This fixes a problem caused by an invalid calculation of n_voltages
in the driver. This n_voltages value has the potential to be
different for each regulator and the new calculation takes this
into account.
Several of the regulators have a non-linear response of voltage
versus voltage selector. This is true for the following LDOs:
5, 6, 7, 8, 9, 10 and 11.
This patch also includes several minor alterations to clean up
the code so that checkpatch.pl will not display any warnings.
Regards,
Steve Twiss, Dialog Semiconductor Ltd.
drivers/regulator/da9063-regulator.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index 56727eb..6a2b26f 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -1,3 +1,4 @@
+
/*
* Regulator driver for DA9063 PMIC series
*
@@ -60,7 +61,8 @@ struct da9063_regulator_info {
.desc.ops = &da9063_ldo_ops, \
.desc.min_uV = (min_mV) * 1000, \
.desc.uV_step = (step_mV) * 1000, \
- .desc.n_voltages = (((max_mV) - (min_mV))/(step_mV) + 1), \
+ .desc.n_voltages = (((max_mV) - (min_mV))/(step_mV) + 1 \
+ + (DA9063_V##regl_name##_BIAS)), \
.desc.enable_reg = DA9063_REG_##regl_name##_CONT, \
.desc.enable_mask = DA9063_LDO_EN, \
.desc.vsel_reg = DA9063_REG_V##regl_name##_A, \
@@ -387,7 +389,8 @@ static int da9063_suspend_disable(struct regulator_dev *rdev)
return regmap_field_write(regl->suspend, 0);
}
-static int da9063_buck_set_suspend_mode(struct regulator_dev *rdev, unsigned mode)
+static int da9063_buck_set_suspend_mode(struct regulator_dev *rdev,
+ unsigned mode)
{
struct da9063_regulator *regl = rdev_get_drvdata(rdev);
int val;
@@ -409,7 +412,8 @@ static int da9063_buck_set_suspend_mode(struct regulator_dev *rdev, unsigned mod
return regmap_field_write(regl->mode, val);
}
-static int da9063_ldo_set_suspend_mode(struct regulator_dev *rdev, unsigned mode)
+static int da9063_ldo_set_suspend_mode(struct regulator_dev *rdev,
+ unsigned mode)
{
struct da9063_regulator *regl = rdev_get_drvdata(rdev);
unsigned val;
@@ -833,8 +837,9 @@ static int da9063_regulator_probe(struct platform_device *pdev)
regl->sleep = devm_regmap_field_alloc(&pdev->dev,
da9063->regmap, regl->info->sleep);
if (regl->info->suspend_sleep.reg)
- regl->suspend_sleep = devm_regmap_field_alloc(&pdev->dev,
- da9063->regmap, regl->info->suspend_sleep);
+ regl->suspend_sleep =
+ devm_regmap_field_alloc(&pdev->dev,
+ da9063->regmap, regl->info->suspend_sleep);
if (regl->info->ilimit.reg)
regl->ilimit = devm_regmap_field_alloc(&pdev->dev,
da9063->regmap, regl->info->ilimit);
@@ -842,7 +847,8 @@ static int da9063_regulator_probe(struct platform_device *pdev)
/* Register regulator */
memset(&config, 0, sizeof(config));
config.dev = &pdev->dev;
- config.init_data = da9063_get_regulator_initdata(regl_pdata, id);
+ config.init_data =
+ da9063_get_regulator_initdata(regl_pdata, id);
config.driver_data = regl;
if (da9063_reg_matches)
config.of_node = da9063_reg_matches[id].of_node;
--
end-of-patch for PATCH V1
On Tue, Feb 11, 2014 at 05:46:41PM +0000, Steve Twiss wrote:
> From: Steve Twiss <[email protected]>
>
> Bug fix to allow the setting of maximum voltage for certain LDOs.
This changelog isn't very useful, it doesn't say what the bug was or
what the fix is. However...
> This fixes a problem caused by an invalid calculation of n_voltages
> in the driver. This n_voltages value has the potential to be
> different for each regulator and the new calculation takes this
> into account.
> Several of the regulators have a non-linear response of voltage
> versus voltage selector. This is true for the following LDOs:
> 5, 6, 7, 8, 9, 10 and 11.
...but this one is more so. It should have been the actual changelog,
along with a description of what the fix actually is. In general
anything that's not in the changelog should normally be process stuff
not a description of the change.
> This patch also includes several minor alterations to clean up
> the code so that checkpatch.pl will not display any warnings.
Don't do things like this for bug fix patches, fix the bug and then
separately do any style fixes. Mixing in unrelated changes makes things
harder to review since there is and makes it harder to send things to stable.
> @@ -60,7 +61,8 @@ struct da9063_regulator_info {
> .desc.ops = &da9063_ldo_ops, \
> .desc.min_uV = (min_mV) * 1000, \
> .desc.uV_step = (step_mV) * 1000, \
> - .desc.n_voltages = (((max_mV) - (min_mV))/(step_mV) + 1), \
> + .desc.n_voltages = (((max_mV) - (min_mV))/(step_mV) + 1 \
> + + (DA9063_V##regl_name##_BIAS)), \
This seems a bit confusing - the number of voltages is affected by
something called _BIAS? That's not very clear. There was also some
mention of non-linear responses which doesn't seem to have been
addressed at all in the change.
> @@ -387,7 +389,8 @@ static int da9063_suspend_disable(struct regulator_dev *rdev)
> return regmap_field_write(regl->suspend, 0);
> }
>
> -static int da9063_buck_set_suspend_mode(struct regulator_dev *rdev, unsigned mode)
> +static int da9063_buck_set_suspend_mode(struct regulator_dev *rdev,
> + unsigned mode)
> {
> struct da9063_regulator *regl = rdev_get_drvdata(rdev);
> int val;
This is an example of something that just shouldn't be in this change at
all, send it separately. There's no overlap in either the purpose of
the patch or in the code affected. In fact it looks like the entire
rest of the patch is unrelated stylistic changes? If that's the case
there's far more checkpatch in here than anything related to the main
purpose of the patch.
As usual, thank you for your quick response Mark.
I take all of your comments and will resubmit the change for this
bug-fix as a single patch.
Regards,
Steve
On 11 February 2014 18:52, Mark Brown wrote:
>From: Mark Brown [mailto:[email protected]]
>> From: Steve Twiss <[email protected]>
>>
>> Bug fix to allow the setting of maximum voltage for certain LDOs.
>
>This changelog isn't very useful, it doesn't say what the bug was or
>what the fix is. However...
>
>> This fixes a problem caused by an invalid calculation of n_voltages
>> in the driver. This n_voltages value has the potential to be
>> different for each regulator and the new calculation takes this
>> into account.
>
>> Several of the regulators have a non-linear response of voltage
>> versus voltage selector. This is true for the following LDOs:
>> 5, 6, 7, 8, 9, 10 and 11.
>
>...but this one is more so. It should have been the actual changelog,
>along with a description of what the fix actually is. In general
>anything that's not in the changelog should normally be process stuff
>not a description of the change.
>
>> This patch also includes several minor alterations to clean up
>> the code so that checkpatch.pl will not display any warnings.
>
>Don't do things like this for bug fix patches, fix the bug and then
>separately do any style fixes. Mixing in unrelated changes makes things
>harder to review since there is and makes it harder to send things to stable.
>
>> @@ -60,7 +61,8 @@ struct da9063_regulator_info {
>> .desc.ops = &da9063_ldo_ops, \
>> .desc.min_uV = (min_mV) * 1000, \
>> .desc.uV_step = (step_mV) * 1000, \
>> - .desc.n_voltages = (((max_mV) - (min_mV))/(step_mV) + 1), \
>> + .desc.n_voltages = (((max_mV) - (min_mV))/(step_mV) + 1 \
>> + + (DA9063_V##regl_name##_BIAS)), \
>
>This seems a bit confusing - the number of voltages is affected by
>something called _BIAS? That's not very clear. There was also some
>mention of non-linear responses which doesn't seem to have been
>addressed at all in the change.
>
>> @@ -387,7 +389,8 @@ static int da9063_suspend_disable(struct regulator_dev
>*rdev)
>> return regmap_field_write(regl->suspend, 0);
>> }
>>
>> -static int da9063_buck_set_suspend_mode(struct regulator_dev *rdev, unsigned
>mode)
>> +static int da9063_buck_set_suspend_mode(struct regulator_dev *rdev,
>> + unsigned mode)
>> {
>> struct da9063_regulator *regl = rdev_get_drvdata(rdev);
>> int val;
>
>This is an example of something that just shouldn't be in this change at
>all, send it separately. There's no overlap in either the purpose of
>the patch or in the code affected. In fact it looks like the entire
>rest of the patch is unrelated stylistic changes? If that's the case
>there's far more checkpatch in here than anything related to the main
>purpose of the patch.