linux-kernel - Re: [PATCH 2/3] regulators: Update of ab8500 driver

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: <20101123183833.GB19212@rakim.wolfsonmicro.main>
Date:	Tue, 23 Nov 2010 18:38:33 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Bengt Jonsson <bengt.g.jonsson@...ricsson.com>
Cc:	Liam Girdwood <lrg@...mlogic.co.uk>, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Linus Walleij <linus.walleij@...ricsson.com>
Subject: Re: [PATCH 2/3] regulators: Update of ab8500 driver

On Tue, Nov 23, 2010 at 07:25:53PM +0100, Bengt Jonsson wrote:
> This patch updates the driver with some bug fixes, support
> for v2 hardware, support for regulator_count_voltages,
> changes for reading the board config, added indexing of
> the regulator array and added debug prints.

This is a whole series of changes which don't seem terribly related to
each other and aren't clearly described - please split this out into a
patch series which clearly shows each individual change.  For example,
one change per bug fix, another adding v2 support, another adding count
support and so on.  As things stand it's very difficult to review your
patch as it's not clear what each change is supposed to be doing or that
bits haven't been missed from changes.

> -	ret = abx500_get_register_interruptible(info->dev, info->voltage_bank,
> -		info->voltage_reg, &value);
> +	ret = abx500_get_register_interruptible(info->dev,
> +			info->voltage_bank, info->voltage_reg, &regval);

In addition to what's noted above you also appear to have some renaming
of variables and struct members plus some other stylistic changes which
adds to the noise, again these should be split out for ease of review.

> +	[AB8500_REGULATOR_INTCORE] = {
> +		.desc = {
> +			.name		= "intcore",

The previous way of listing the regulators with a macro seemed more
immediately readable?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/