lkml.org 
[lkml]   [2014]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V1] regulator: da9063: Bug fix when setting max voltage on LDOs 5-11
Date
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:broonie@kernel.org]

>> From: Steve Twiss <stwiss.opensource@diasemi.com>
>>
>> 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.


\
 
 \ /
  Last update: 2014-02-12 10:21    [W:0.040 / U:0.720 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site