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:broo...@kernel.org] >> From: Steve Twiss <stwiss.opensou...@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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
