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

Am 2013-12-02 10:36, schrieb Thierry Reding:

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

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

quoted

diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c

[...]

quoted

+#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.

quoted

+	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?

The last 4 lines indentation was already that far, so I just adopted
those lines in order to avoid having to touch them too.

quoted

@@ -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_"?

quoted

@@ -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.

Sounds reasonable, will change that accordingly in the next patch
version.