linux-kernel - Re: [PATCH v2 2/3] regulator: tps6586x: add and use correct voltage table

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]

Date:	Mon, 2 Dec 2013 10:36:23 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Stefan Agner <stefan@...er.ch>
Cc:	swarren@...dotorg.org, dev@...xeye.de, lee.jones@...aro.org,
	lgirdwood@...il.com, broonie@...nel.org,
	kai.poggensee@...onic-design.de, sameo@...ux.intel.com,
	linux-tegra@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] regulator: tps6586x: add and use correct voltage
 table

On Sun, Dec 01, 2013 at 04:59:14PM +0100, Stefan Agner wrote:
[...]

This looks pretty good generally. A few minor nits below...

> diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
[...]
> +#define tps658623_sm2_voltages tps6586x_ldo4_voltages
>  static const unsigned int tps6586x_ldo4_voltages[] = {
>  	1700000, 1725000, 1750000, 1775000, 1800000, 1825000, 1850000, 1875000,
>  	1900000, 1925000, 1950000, 1975000, 2000000, 2025000, 2050000, 2075000,

I'd put the #define below the ldo4 table. This doesn't actually matter
for the preprocessor, but it makes it easier to read the code. Also an
additional blank line would help with readability.

> +	TPS6586X_LDO(LDO_0, "vinldo01", tps6586x_ldo0, SUPPLYV1, 5, 3, ENC, 0,
> +					END, 0),

Perhaps reduce the indentation here so there's more room in case this
ever needs to be extended?

> @@ -351,6 +380,7 @@ static int tps6586x_regulator_probe(struct platform_device *pdev)
>  	struct regulator_init_data *reg_data;
>  	struct tps6586x_platform_data *pdata;
>  	struct of_regulator_match *tps6586x_reg_matches = NULL;
> +	int reg_version;

Why the prefix "reg_"?

> @@ -373,10 +403,27 @@ static int tps6586x_regulator_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	reg_version = tps6586x_get_version(pdev->dev.parent);
> +
>  	for (id = 0; id < TPS6586X_ID_MAX_REGULATOR; ++id) {
>  		reg_data = pdata->reg_init_data[id];
>  
> -		ri = find_regulator_info(id);
> +		switch(reg_version) {
> +		case TPS658623:
> +			ri = find_regulator_info(id, tps658623_regulator,
> +					ARRAY_SIZE(tps658623_regulator));
> +			break;
> +		case TPS658643:
> +			ri = find_regulator_info(id, tps658643_regulator,
> +					ARRAY_SIZE(tps658643_regulator));
> +			break;
> +		}

Perhaps instead of repeating the function calls this could be:

		switch (version) {
		case TPS6586XYZ:
			num = ARRAY_SIZE(tps6586xyz_regulator);
			table = tps6586xys_regulator;
			break;

		...
		}

		if (table)
			ri = find_regulator_info(id, table, num);

That's slightly longer, but I find that to be more intuitive. Perhaps
a bit more future-proof since you only have a single call. But that's
perhaps subjective, so I'm fine with your alternative, too.

Thierry

Content of type "application/pgp-signature" skipped