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... > >> 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? The last 4 lines indentation was already that far, so I just adopted those lines in order to avoid having to touch them too. > >> @@ -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. Sounds reasonable, will change that accordingly in the next patch version. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html