linux-kernel - Re: [PATCH V1] regulator: da9063: Bug fix when setting max voltage on LDOs 5-11

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140211185157.GY13533@sirena.org.uk>
Date:	Tue, 11 Feb 2014 18:51:57 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Steve Twiss <stwiss.opensource@...semi.com>
Cc:	Liam Girdwood <lgirdwood@...il.com>,
	David Dajun Chen <david.chen@...semi.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V1] regulator: da9063: Bug fix when setting max voltage
 on LDOs 5-11

On Tue, Feb 11, 2014 at 05:46:41PM +0000, Steve Twiss wrote:
> From: Steve Twiss <stwiss.opensource@...semi.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.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)