[PATCH v4 0/3] regulator: tps6586x: add version detection and voltage tables

[PATCH v4 0/3] regulator: tps6586x: add version detection and voltage tables

[Stefan Agner]

This is the 4rd version of this patchset which adds version detection
for the tps6586x mfd family. Just discovered a missing null reference
check which did not triggered in my tests, sorry about that.

This is required because some regulator versions use different voltage
tables. The regulator driver now uses the right voltage table according
to the version.

While at it, there are a few other incorrect regulator entries in the
device tree, change them accordingly (this patch is new and grew out
of a patch with the same name which Lucas Stach posted in July).

Tested on Colibri T20 V1.1 and V1.2 which makes use of diffent version
of that regulator.

Changes since v3:
  - Check if version specific table is necessary

Changes since v2:
  - Avoid moving devm_kzalloc
  - Removed reg_ from reg_version
  - Moved walk through version dependent tables to find_regulator_info,
    removed the inline definition. This reduces .o size and encapsulates
    the logic of finding the right regulator into one function. It comes
    with a slight code duplication, the table search now appears twice.

Changes since v1:
  - Merged DT and driver change (preserves bisectability)
  - Use different regulator tables for different regulator version
    * This also makes the TPS6586X_ANY version needless
  - Replaced enum with defines
  - Move version message in a sperate function
  - Include regulator bugfix, use correct supply voltage
  - Checked all voltages, fix SM0/SM1 (Core/CPU) and LDO6 voltage (VDAC/VI)
    in another commit
  - Rebased on top of v3.13-rc2


Stefan Agner (3):
  mfd: tps6586x: add version detection
  regulator: tps6586x: add and use correct voltage table
  ARM: tegra: correct Colibri T20 regulator settings

 arch/arm/boot/dts/tegra20-colibri-512.dtsi | 34 +++++------
 drivers/mfd/tps6586x.c                     | 43 ++++++++++++--
 drivers/regulator/tps6586x-regulator.c     | 93 ++++++++++++++++++++++++------
 include/linux/mfd/tps6586x.h               |  7 +++
 4 files changed, 136 insertions(+), 41 deletions(-)

-- 
1.8.4.2

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Stefan Agner]

Use the VERSIONCRC to determine the exact device version. According to
the datasheet this register can be used as device identifier. The
identification is needed since some tps6586x regulators use a different
voltage table.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Since the struct tps6586x is inside the c file, I could not easily
move the tps6586x_get_version function as inline to the header file.

Changes since v2:
  - Avoid moving devm_kzalloc
---
 drivers/mfd/tps6586x.c       | 43 ++++++++++++++++++++++++++++++++++++++-----
 include/linux/mfd/tps6586x.h |  7 +++++++
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index ee61fd7..56a8422 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -124,6 +124,7 @@ struct tps6586x {
 	struct device		*dev;
 	struct i2c_client	*client;
 	struct regmap		*regmap;
+	int			version;
 
 	int			irq;
 	struct irq_chip		irq_chip;
@@ -208,6 +209,14 @@ int tps6586x_irq_get_virq(struct device *dev, int irq)
 }
 EXPORT_SYMBOL_GPL(tps6586x_irq_get_virq);
 
+int tps6586x_get_version(struct device *dev)
+{
+	struct tps6586x *tps6586x = dev_get_drvdata(dev);
+
+	return tps6586x->version;
+}
+EXPORT_SYMBOL_GPL(tps6586x_get_version);
+
 static int __remove_subdev(struct device *dev, void *unused)
 {
 	platform_device_unregister(to_platform_device(dev));
@@ -472,6 +481,31 @@ static void tps6586x_power_off(void)
 	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
 }
 
+static void tps6586x_print_version(struct i2c_client *client, int version)
+{
+	const char *name;
+
+	switch (version) {
+	case TPS658621A:
+		name = "TPS658621A";
+		break;
+	case TPS658621CD:
+		name = "TPS658621C/D";
+		break;
+	case TPS658623:
+		name = "TPS658623";
+		break;
+	case TPS658643:
+		name = "TPS658643";
+		break;
+	default:
+		name = "TPS6586X";
+		break;
+	}
+
+	dev_info(&client->dev, "Found %s, VERSIONCRC is %02x\n", name, version);
+}
+
 static int tps6586x_i2c_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
@@ -493,13 +527,12 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
 		return -EIO;
 	}
 
-	dev_info(&client->dev, "VERSIONCRC is %02x\n", ret);
-
 	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
-	if (tps6586x == NULL) {
-		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
+	if (!tps6586x)
 		return -ENOMEM;
-	}
+
+	tps6586x->version = ret;
+	tps6586x_print_version(client, tps6586x->version);
 
 	tps6586x->client = client;
 	tps6586x->dev = &client->dev;
diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
index 8799454..cbecec2 100644
--- a/include/linux/mfd/tps6586x.h
+++ b/include/linux/mfd/tps6586x.h
@@ -13,6 +13,12 @@
 #define TPS6586X_SLEW_RATE_SET		0x08
 #define TPS6586X_SLEW_RATE_MASK         0x07
 
+/* VERSION CRC */
+#define TPS658621A	0x15
+#define TPS658621CD	0x2c
+#define TPS658623	0x1b
+#define TPS658643	0x03
+
 enum {
 	TPS6586X_ID_SYS,
 	TPS6586X_ID_SM_0,
@@ -97,5 +103,6 @@ extern int tps6586x_clr_bits(struct device *dev, int reg, uint8_t bit_mask);
 extern int tps6586x_update(struct device *dev, int reg, uint8_t val,
 			   uint8_t mask);
 extern int tps6586x_irq_get_virq(struct device *dev, int irq);
+extern int tps6586x_get_version(struct device *dev);
 
 #endif /*__LINUX_MFD_TPS6586X_H */
-- 
1.8.4.2

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

[Stefan Agner]

Depending on the regulator version, the voltage table might be
different. Use version specific regulator tables in order to select
correct voltage table. For the following regulator versions different
voltage tables are now used:

  * TPS658623: Use correct voltage table for SM2
  * TPS658643: New voltage table for SM2

Both versions are in use on the Colibri T20 module. Make use of the
correct tables by requesting the correct SM2 voltage of 1.8V.

This change is not backward compatible since an old driver is not able
to correctly set that value. The value 1.8V is out of range for the old
driver and will refuse to probe the device. The regulator starts with
default settings and the driver shows appropriate error messages.

On Colibri T20, the old value used to work with TPS658623 since the
driver applied a wrong voltage table too. However, the TPS658643 used
on V1.2 devices uses yet another voltage table and those broke that
pseudo-compatibility. The regulator driver now has the correct voltage
table for both regulator versions and those the correct voltage can be
used in the device tree.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Changes since v3:
  - Check if version specific table is necessary

Changes since v2:
  - Removed reg_ from reg_version
  - Moved walk through version dependent tables to find_regulator_info,
    removed the inline definition. This reduces .o size and encapsulates
    the logic of finding the right regulator into one function. It comes
    with a slight code duplication, the table search now appears twice.
---
 arch/arm/boot/dts/tegra20-colibri-512.dtsi |  4 +-
 drivers/regulator/tps6586x-regulator.c     | 93 ++++++++++++++++++++++++------
 2 files changed, 76 insertions(+), 21 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
index d5c9bca..cbe89ff 100644
--- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
+++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
@@ -268,8 +268,8 @@
 					reg = <3>;
 					regulator-compatible = "sm2";
 					regulator-name = "vdd_sm2,vin_ldo*";
-					regulator-min-microvolt = <3700000>;
-					regulator-max-microvolt = <3700000>;
+					regulator-min-microvolt = <1800000>;
+					regulator-max-microvolt = <1800000>;
 					regulator-always-on;
 				};
 
diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
index e8e3a8a..0485d47 100644
--- a/drivers/regulator/tps6586x-regulator.c
+++ b/drivers/regulator/tps6586x-regulator.c
@@ -93,6 +93,8 @@ static const unsigned int tps6586x_ldo4_voltages[] = {
 	2300000, 2325000, 2350000, 2375000, 2400000, 2425000, 2450000, 2475000,
 };
 
+#define tps658623_sm2_voltages tps6586x_ldo4_voltages
+
 static const unsigned int tps6586x_ldo_voltages[] = {
 	1250000, 1500000, 1800000, 2500000, 2700000, 2850000, 3100000, 3300000,
 };
@@ -104,6 +106,13 @@ static const unsigned int tps6586x_sm2_voltages[] = {
 	4200000, 4250000, 4300000, 4350000, 4400000, 4450000, 4500000, 4550000,
 };
 
+static const unsigned int tps658643_sm2_voltages[] = {
+	1025000, 1050000, 1075000, 1100000, 1125000, 1150000, 1175000, 1200000,
+	1225000, 1250000, 1275000, 1300000, 1325000, 1350000, 1375000, 1400000,
+	1425000, 1450000, 1475000, 1500000, 1525000, 1550000, 1575000, 1600000,
+	1625000, 1650000, 1675000, 1700000, 1725000, 1750000, 1775000, 1800000,
+};
+
 static const unsigned int tps6586x_dvm_voltages[] = {
 	 725000,  750000,  775000,  800000,  825000,  850000,  875000,  900000,
 	 925000,  950000,  975000, 1000000, 1025000, 1050000, 1075000, 1100000,
@@ -119,8 +128,8 @@ static const unsigned int tps6586x_dvm_voltages[] = {
 		.ops	= &tps6586x_regulator_ops,			\
 		.type	= REGULATOR_VOLTAGE,				\
 		.id	= TPS6586X_ID_##_id,				\
-		.n_voltages = ARRAY_SIZE(tps6586x_##vdata##_voltages),	\
-		.volt_table = tps6586x_##vdata##_voltages,		\
+		.n_voltages = ARRAY_SIZE(vdata##_voltages),		\
+		.volt_table = vdata##_voltages,				\
 		.owner	= THIS_MODULE,					\
 		.enable_reg = TPS6586X_SUPPLY##ereg0,			\
 		.enable_mask = 1 << (ebit0),				\
@@ -162,27 +171,47 @@ static const unsigned int tps6586x_dvm_voltages[] = {
 
 static struct tps6586x_regulator tps6586x_regulator[] = {
 	TPS6586X_SYS_REGULATOR(),
-	TPS6586X_LDO(LDO_0, "vinldo01", ldo0, SUPPLYV1, 5, 3, ENC, 0, END, 0),
-	TPS6586X_LDO(LDO_3, "vinldo23", ldo, SUPPLYV4, 0, 3, ENC, 2, END, 2),
-	TPS6586X_LDO(LDO_5, "REG-SYS", ldo, SUPPLYV6, 0, 3, ENE, 6, ENE, 6),
-	TPS6586X_LDO(LDO_6, "vinldo678", ldo, SUPPLYV3, 0, 3, ENC, 4, END, 4),
-	TPS6586X_LDO(LDO_7, "vinldo678", ldo, SUPPLYV3, 3, 3, ENC, 5, END, 5),
-	TPS6586X_LDO(LDO_8, "vinldo678", ldo, SUPPLYV2, 5, 3, ENC, 6, END, 6),
-	TPS6586X_LDO(LDO_9, "vinldo9", ldo, SUPPLYV6, 3, 3, ENE, 7, ENE, 7),
-	TPS6586X_LDO(LDO_RTC, "REG-SYS", ldo, SUPPLYV4, 3, 3, V4, 7, V4, 7),
-	TPS6586X_LDO(LDO_1, "vinldo01", dvm, SUPPLYV1, 0, 5, ENC, 1, END, 1),
-	TPS6586X_LDO(SM_2, "vin-sm2", sm2, SUPPLYV2, 0, 5, ENC, 7, END, 7),
-
-	TPS6586X_DVM(LDO_2, "vinldo23", dvm, LDO2BV1, 0, 5, ENA, 3,
+	TPS6586X_LDO(LDO_0, "vinldo01", tps6586x_ldo0, SUPPLYV1, 5, 3, ENC, 0,
+					END, 0),
+	TPS6586X_LDO(LDO_3, "vinldo23", tps6586x_ldo, SUPPLYV4, 0, 3, ENC, 2,
+					END, 2),
+	TPS6586X_LDO(LDO_5, "REG-SYS", tps6586x_ldo, SUPPLYV6, 0, 3, ENE, 6,
+					ENE, 6),
+	TPS6586X_LDO(LDO_6, "vinldo678", tps6586x_ldo, SUPPLYV3, 0, 3, ENC, 4,
+					END, 4),
+	TPS6586X_LDO(LDO_7, "vinldo678", tps6586x_ldo, SUPPLYV3, 3, 3, ENC, 5,
+					END, 5),
+	TPS6586X_LDO(LDO_8, "vinldo678", tps6586x_ldo, SUPPLYV2, 5, 3, ENC, 6,
+					END, 6),
+	TPS6586X_LDO(LDO_9, "vinldo9", tps6586x_ldo, SUPPLYV6, 3, 3, ENE, 7,
+					ENE, 7),
+	TPS6586X_LDO(LDO_RTC, "REG-SYS", tps6586x_ldo, SUPPLYV4, 3, 3, V4, 7,
+					V4, 7),
+	TPS6586X_LDO(LDO_1, "vinldo01", tps6586x_dvm, SUPPLYV1, 0, 5, ENC, 1,
+					END, 1),
+	TPS6586X_LDO(SM_2, "vin-sm2", tps6586x_sm2, SUPPLYV2, 0, 5, ENC, 7,
+					END, 7),
+
+	TPS6586X_DVM(LDO_2, "vinldo23", tps6586x_dvm, LDO2BV1, 0, 5, ENA, 3,
 					ENB, 3, TPS6586X_VCC2, BIT(6)),
-	TPS6586X_DVM(LDO_4, "vinldo4", ldo4, LDO4V1, 0, 5, ENC, 3,
+	TPS6586X_DVM(LDO_4, "vinldo4", tps6586x_ldo4, LDO4V1, 0, 5, ENC, 3,
 					END, 3, TPS6586X_VCC1, BIT(6)),
-	TPS6586X_DVM(SM_0, "vin-sm0", dvm, SM0V1, 0, 5, ENA, 1,
+	TPS6586X_DVM(SM_0, "vin-sm0", tps6586x_dvm, SM0V1, 0, 5, ENA, 1,
 					ENB, 1, TPS6586X_VCC1, BIT(2)),
-	TPS6586X_DVM(SM_1, "vin-sm1", dvm, SM1V1, 0, 5, ENA, 0,
+	TPS6586X_DVM(SM_1, "vin-sm1", tps6586x_dvm, SM1V1, 0, 5, ENA, 0,
 					ENB, 0, TPS6586X_VCC1, BIT(0)),
 };
 
+static struct tps6586x_regulator tps658623_regulator[] = {
+	TPS6586X_LDO(SM_2, "vin-sm2", tps658623_sm2, SUPPLYV2, 0, 5, ENC, 7,
+					END, 7),
+};
+
+static struct tps6586x_regulator tps658643_regulator[] = {
+	TPS6586X_LDO(SM_2, "vin-sm2", tps658643_sm2, SUPPLYV2, 0, 5, ENC, 7,
+					END, 7),
+};
+
 /*
  * TPS6586X has 2 enable bits that are OR'ed to determine the actual
  * regulator state. Clearing one of this bits allows switching
@@ -254,11 +283,33 @@ static int tps6586x_regulator_set_slew_rate(struct platform_device *pdev,
 			setting->slew_rate & TPS6586X_SLEW_RATE_MASK);
 }
 
-static inline struct tps6586x_regulator *find_regulator_info(int id)
+static struct tps6586x_regulator *find_regulator_info(int id, int version)
 {
 	struct tps6586x_regulator *ri;
+	struct tps6586x_regulator *table = NULL;
+	int num;
 	int i;
 
+	switch (version) {
+	case TPS658623:
+		table = tps658623_regulator;
+		num = ARRAY_SIZE(tps658623_regulator);
+		break;
+	case TPS658643:
+		table = tps658643_regulator;
+		num = ARRAY_SIZE(tps658643_regulator);
+		break;
+	}
+
+	/* Search version specific table first */
+	if (table) {
+		for (i = 0; i < num; i++) {
+			ri = &table[i];
+			if (ri->desc.id == id)
+				return ri;
+		}
+	}
+
 	for (i = 0; i < ARRAY_SIZE(tps6586x_regulator); i++) {
 		ri = &tps6586x_regulator[i];
 		if (ri->desc.id == id)
@@ -351,6 +402,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 version;
 	int id;
 	int err;
 
@@ -373,10 +425,13 @@ static int tps6586x_regulator_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	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);
+		ri = find_regulator_info(id, version);
+
 		if (!ri) {
 			dev_err(&pdev->dev, "invalid regulator ID specified\n");
 			return -EINVAL;
-- 
1.8.4.2

[PATCH v4 3/3] ARM: tegra: correct Colibri T20 regulator settings

[Stefan Agner]

Set the parent of the regulators LDO2 to LDO9 according to the
schematic. Set the base voltage to 3.3V, there is only 3.3V on the
module itself.

Set the Core and CPU voltage to the specified voltages of 1.2V and
1.0V respectivly.

LDO6 should deliver 2.85V. The attached peripherals were not in
use so far.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/tegra20-colibri-512.dtsi | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
index cbe89ff..51e0880 100644
--- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
+++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
@@ -225,15 +225,15 @@
 			#gpio-cells = <2>;
 			gpio-controller;
 
-			sys-supply = <&vdd_5v0_reg>;
+			sys-supply = <&vdd_3v3_reg>;
 			vin-sm0-supply = <&sys_reg>;
 			vin-sm1-supply = <&sys_reg>;
 			vin-sm2-supply = <&sys_reg>;
 			vinldo01-supply = <&sm2_reg>;
-			vinldo23-supply = <&sm2_reg>;
-			vinldo4-supply = <&sm2_reg>;
-			vinldo678-supply = <&sm2_reg>;
-			vinldo9-supply = <&sm2_reg>;
+			vinldo23-supply = <&vdd_3v3_reg>;
+			vinldo4-supply = <&vdd_3v3_reg>;
+			vinldo678-supply = <&vdd_3v3_reg>;
+			vinldo9-supply = <&vdd_3v3_reg>;
 
 			regulators {
 				#address-cells = <1>;
@@ -250,8 +250,8 @@
 					reg = <1>;
 					regulator-compatible = "sm0";
 					regulator-name = "vdd_sm0,vdd_core";
-					regulator-min-microvolt = <1275000>;
-					regulator-max-microvolt = <1275000>;
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;
 					regulator-always-on;
 				};
 
@@ -259,8 +259,8 @@
 					reg = <2>;
 					regulator-compatible = "sm1";
 					regulator-name = "vdd_sm1,vdd_cpu";
-					regulator-min-microvolt = <1100000>;
-					regulator-max-microvolt = <1100000>;
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <1000000>;
 					regulator-always-on;
 				};
 
@@ -316,8 +316,8 @@
 					reg = <10>;
 					regulator-compatible = "ldo6";
 					regulator-name = "vdd_ldo6,avdd_vdac,vddio_vi,vddio_cam";
-					regulator-min-microvolt = <1800000>;
-					regulator-max-microvolt = <1800000>;
+					regulator-min-microvolt = <2850000>;
+					regulator-max-microvolt = <2850000>;
 				};
 
 				hdmi_vdd_reg: regulator at 11 {
@@ -504,12 +504,12 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		vdd_5v0_reg: regulator at 100 {
+		vdd_3v3_reg: regulator at 100 {
 			compatible = "regulator-fixed";
 			reg = <100>;
-			regulator-name = "vdd_5v0";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
+			regulator-name = "vdd_3v3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
 			regulator-always-on;
 		};
 
-- 
1.8.4.2

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Lee Jones]

> +int tps6586x_get_version(struct device *dev)
> +{
> +	struct tps6586x *tps6586x = dev_get_drvdata(dev);
> +
> +	return tps6586x->version;
> +}
> +EXPORT_SYMBOL_GPL(tps6586x_get_version);

I thought Mark suggested that this routine was converted to a 'static
inline' and moved into the header? Did you not think this was a good
idea?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Stefan Agner]

Am 2013-12-04 09:10, schrieb Lee Jones:
>> +int tps6586x_get_version(struct device *dev)
>> +{
>> +	struct tps6586x *tps6586x = dev_get_drvdata(dev);
>> +
>> +	return tps6586x->version;
>> +}
>> +EXPORT_SYMBOL_GPL(tps6586x_get_version);
> 
> I thought Mark suggested that this routine was converted to a 'static
> inline' and moved into the header? Did you not think this was a good
> idea?
As I pointed out in the comment above, the struct tps6586x is in the C
file, so I would need to move that too. That's why I did not made that
change in the end. What do you think, should I still move (and move the
struct too?) 

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

[Thierry Reding]

On Tue, Dec 03, 2013 at 11:18:47PM +0100, Stefan Agner wrote:
[...]
> diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
[...]
> +	/* Search version specific table first */
> +	if (table) {
> +		for (i = 0; i < num; i++) {
> +			ri = &table[i];
> +			if (ri->desc.id == id)
> +				return ri;
> +		}
> +	}

Ah... there you go. Looks good:

Reviewed-by: Thierry Reding <treding@nvidia.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131204/417edcbe/attachment-0001.sig>

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Lee Jones]

On Wed, 04 Dec 2013, Stefan Agner wrote:

> Am 2013-12-04 09:10, schrieb Lee Jones:
> >> +int tps6586x_get_version(struct device *dev)
> >> +{
> >> +	struct tps6586x *tps6586x = dev_get_drvdata(dev);
> >> +
> >> +	return tps6586x->version;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tps6586x_get_version);
> > 
> > I thought Mark suggested that this routine was converted to a 'static
> > inline' and moved into the header? Did you not think this was a good
> > idea?
> As I pointed out in the comment above, the struct tps6586x is in the C
> file, so I would need to move that too. That's why I did not made that
> change in the end. What do you think, should I still move (and move the
> struct too?) 

Why would the struct have to be moved if the function is inline?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Stefan Agner]

Am 2013-12-04 11:07, schrieb Lee Jones:
> On Wed, 04 Dec 2013, Stefan Agner wrote:
> 
>> Am 2013-12-04 09:10, schrieb Lee Jones:
>> >> +int tps6586x_get_version(struct device *dev)
>> >> +{
>> >> +	struct tps6586x *tps6586x = dev_get_drvdata(dev);
>> >> +
>> >> +	return tps6586x->version;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(tps6586x_get_version);
>> >
>> > I thought Mark suggested that this routine was converted to a 'static
>> > inline' and moved into the header? Did you not think this was a good
>> > idea?
>> As I pointed out in the comment above, the struct tps6586x is in the C
>> file, so I would need to move that too. That's why I did not made that
>> change in the end. What do you think, should I still move (and move the
>> struct too?)
> 
> Why would the struct have to be moved if the function is inline?

True, the inline I could have done without moving the struct and the
function. Would you like me to create another revision doing this?

But moving the function needs moving of the struct tps6586x
declaration...


[Sorry, this time with answer all]

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Lee Jones]

On Wed, 04 Dec 2013, Stefan Agner wrote:

> Am 2013-12-04 11:07, schrieb Lee Jones:
> > On Wed, 04 Dec 2013, Stefan Agner wrote:
> > 
> >> Am 2013-12-04 09:10, schrieb Lee Jones:
> >> >> +int tps6586x_get_version(struct device *dev)
> >> >> +{
> >> >> +	struct tps6586x *tps6586x = dev_get_drvdata(dev);
> >> >> +
> >> >> +	return tps6586x->version;
> >> >> +}
> >> >> +EXPORT_SYMBOL_GPL(tps6586x_get_version);
> >> >
> >> > I thought Mark suggested that this routine was converted to a 'static
> >> > inline' and moved into the header? Did you not think this was a good
> >> > idea?
> >> As I pointed out in the comment above, the struct tps6586x is in the C
> >> file, so I would need to move that too. That's why I did not made that
> >> change in the end. What do you think, should I still move (and move the
> >> struct too?)
> > 
> > Why would the struct have to be moved if the function is inline?
> 
> True, the inline I could have done without moving the struct and the
> function. Would you like me to create another revision doing this?
> 
> But moving the function needs moving of the struct tps6586x
> declaration...
> 
> [Sorry, this time with answer all]

Do you know what, it's really not that important.

Patch applied.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Mark Brown]

On Wed, Dec 04, 2013 at 10:07:28AM +0000, Lee Jones wrote:
> On Wed, 04 Dec 2013, Stefan Agner wrote:

> > As I pointed out in the comment above, the struct tps6586x is in the C
> > file, so I would need to move that too. That's why I did not made that
> > change in the end. What do you think, should I still move (and move the
> > struct too?) 

> Why would the struct have to be moved if the function is inline?

If the function is in the header and trying to use a struct that's only
defined in the C file then it's not going to build - keeping the struct
in the C file only does seem like a worthwhile thing for encapsulation.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131204/971a8602/attachment.sig>

[PATCH v4 3/3] ARM: tegra: correct Colibri T20 regulator settings

[Lucas Stach]

Am Dienstag, den 03.12.2013, 23:18 +0100 schrieb Stefan Agner:
> Set the parent of the regulators LDO2 to LDO9 according to the
> schematic. Set the base voltage to 3.3V, there is only 3.3V on the
> module itself.
> 
> Set the Core and CPU voltage to the specified voltages of 1.2V and
> 1.0V respectivly.
> 
> LDO6 should deliver 2.85V. The attached peripherals were not in
> use so far.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  arch/arm/boot/dts/tegra20-colibri-512.dtsi | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> index cbe89ff..51e0880 100644
> --- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> +++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> @@ -225,15 +225,15 @@
>  			#gpio-cells = <2>;
>  			gpio-controller;
>  
> -			sys-supply = <&vdd_5v0_reg>;
> +			sys-supply = <&vdd_3v3_reg>;
>  			vin-sm0-supply = <&sys_reg>;
>  			vin-sm1-supply = <&sys_reg>;
>  			vin-sm2-supply = <&sys_reg>;
>  			vinldo01-supply = <&sm2_reg>;
> -			vinldo23-supply = <&sm2_reg>;
> -			vinldo4-supply = <&sm2_reg>;
> -			vinldo678-supply = <&sm2_reg>;
> -			vinldo9-supply = <&sm2_reg>;
> +			vinldo23-supply = <&vdd_3v3_reg>;
> +			vinldo4-supply = <&vdd_3v3_reg>;
> +			vinldo678-supply = <&vdd_3v3_reg>;
> +			vinldo9-supply = <&vdd_3v3_reg>;
>  
>  			regulators {
>  				#address-cells = <1>;
> @@ -250,8 +250,8 @@
>  					reg = <1>;
>  					regulator-compatible = "sm0";
>  					regulator-name = "vdd_sm0,vdd_core";
> -					regulator-min-microvolt = <1275000>;
> -					regulator-max-microvolt = <1275000>;
> +					regulator-min-microvolt = <1200000>;
> +					regulator-max-microvolt = <1200000>;
>  					regulator-always-on;
>  				};
>  
> @@ -259,8 +259,8 @@
>  					reg = <2>;
>  					regulator-compatible = "sm1";
>  					regulator-name = "vdd_sm1,vdd_cpu";
> -					regulator-min-microvolt = <1100000>;
> -					regulator-max-microvolt = <1100000>;
> +					regulator-min-microvolt = <1000000>;
> +					regulator-max-microvolt = <1000000>;
>  					regulator-always-on;
>  				};
>  
> @@ -316,8 +316,8 @@
>  					reg = <10>;
>  					regulator-compatible = "ldo6";
>  					regulator-name = "vdd_ldo6,avdd_vdac,vddio_vi,vddio_cam";
> -					regulator-min-microvolt = <1800000>;
> -					regulator-max-microvolt = <1800000>;
> +					regulator-min-microvolt = <2850000>;
> +					regulator-max-microvolt = <2850000>;
>  				};
>  
>  				hdmi_vdd_reg: regulator at 11 {
> @@ -504,12 +504,12 @@
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
> -		vdd_5v0_reg: regulator at 100 {
> +		vdd_3v3_reg: regulator at 100 {
>  			compatible = "regulator-fixed";
>  			reg = <100>;
> -			regulator-name = "vdd_5v0";
> -			regulator-min-microvolt = <5000000>;
> -			regulator-max-microvolt = <5000000>;
> +			regulator-name = "vdd_3v3";
> +			regulator-min-microvolt = <3300000>;
> +			regulator-max-microvolt = <3300000>;
>  			regulator-always-on;
>  		};
>  

-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Lee Jones]

> > > As I pointed out in the comment above, the struct tps6586x is in the C
> > > file, so I would need to move that too. That's why I did not made that
> > > change in the end. What do you think, should I still move (and move the
> > > struct too?) 
> 
> > Why would the struct have to be moved if the function is inline?
> 
> If the function is in the header and trying to use a struct that's only
> defined in the C file then it's not going to build - keeping the struct
> in the C file only does seem like a worthwhile thing for encapsulation.

Yes, I just carried out my own testing and found that out. Prior to
this I was under the impression that inline functions were directly
transposed into the location of the call, were perhaps it could make
use of any resources declared within that context. It appears that
impression was not correct.

Every day continues to be a school day. :)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Mark Brown]

On Tue, Dec 03, 2013 at 11:18:47PM +0100, Stefan Agner wrote:
> Depending on the regulator version, the voltage table might be
> different. Use version specific regulator tables in order to select
> correct voltage table. For the following regulator versions different
> voltage tables are now used:

Acked-by: Mark Brown <broonie@linaro.org>

I won't apply since I think that patch 3 needs to go along with this,
unless people decide the regulator tree is the easiest way to merge.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131204/4e21d4ac/attachment-0001.sig>

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

[Stefan Agner]

Am 2013-12-04 13:14, schrieb Mark Brown:
> On Tue, Dec 03, 2013 at 11:18:47PM +0100, Stefan Agner wrote:
>> Depending on the regulator version, the voltage table might be
>> different. Use version specific regulator tables in order to select
>> correct voltage table. For the following regulator versions different
>> voltage tables are now used:
> 
> Acked-by: Mark Brown <broonie@linaro.org>
> 
> I won't apply since I think that patch 3 needs to go along with this,
> unless people decide the regulator tree is the easiest way to merge.

I merged the dependent DT changes into that patch, so from my side,
patch 3 don't needs to go along with this...

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

[Mark Brown]

On Wed, Dec 04, 2013 at 03:17:59PM +0100, Stefan Agner wrote:
> Am 2013-12-04 13:14, schrieb Mark Brown:

> > Acked-by: Mark Brown <broonie@linaro.org>

> > I won't apply since I think that patch 3 needs to go along with this,
> > unless people decide the regulator tree is the easiest way to merge.

> I merged the dependent DT changes into that patch, so from my side,
> patch 3 don't needs to go along with this...

OK, though now I look I see it also depends on patch 1 itself so at
least those two will need to go together.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131204/813a4258/attachment-0001.sig>

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

[Lee Jones]

> > > Acked-by: Mark Brown <broonie@linaro.org>
> 
> > > I won't apply since I think that patch 3 needs to go along with this,
> > > unless people decide the regulator tree is the easiest way to merge.
> 
> > I merged the dependent DT changes into that patch, so from my side,
> > patch 3 don't needs to go along with this...
> 
> OK, though now I look I see it also depends on patch 1 itself so at
> least those two will need to go together.

Well as I've already applied patch 1, I can take this too, no problem.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Stephen Warren]

On 12/03/2013 03:18 PM, Stefan Agner wrote:
> Use the VERSIONCRC to determine the exact device version. According to
> the datasheet this register can be used as device identifier. The
> identification is needed since some tps6586x regulators use a different
> voltage table.

> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c

> @@ -493,13 +527,12 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>  		return -EIO;
>  	}
>  
> -	dev_info(&client->dev, "VERSIONCRC is %02x\n", ret);
> -
>  	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
> -	if (tps6586x == NULL) {
> -		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
> +	if (!tps6586x)
>  		return -ENOMEM;
> -	}
> +
> +	tps6586x->version = ret;

I have to say, I dislike this version of the patch. Separating the
reading of the version register from the assignment to tps6586x->version
doesn't make any sense, especially given that the version value is
stored in a variable named "ret"; that name isn't remotely related to
what's stored there. What if someone comes along later and adds more
code that assigns to ret between where it's repurposed for the version
value and where it's assigned to tps6586x->version? It'd be extremely
difficult for a patch reviewer to spot that given the limited context in
a diff, and quite non-obvious to the person changing the code too..

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

[Stephen Warren]

On 12/03/2013 03:18 PM, Stefan Agner wrote:
> Depending on the regulator version, the voltage table might be 
> different. Use version specific regulator tables in order to
> select correct voltage table. For the following regulator versions
> different voltage tables are now used:
> 
> * TPS658623: Use correct voltage table for SM2 * TPS658643: New
> voltage table for SM2
> 
> Both versions are in use on the Colibri T20 module. Make use of
> the correct tables by requesting the correct SM2 voltage of 1.8V.
> 
> This change is not backward compatible since an old driver is not
> able to correctly set that value. The value 1.8V is out of range
> for the old driver and will refuse to probe the device. The
> regulator starts with default settings and the driver shows
> appropriate error messages.
> 
> On Colibri T20, the old value used to work with TPS658623 since
> the driver applied a wrong voltage table too. However, the
> TPS658643 used on V1.2 devices uses yet another voltage table and
> those broke that pseudo-compatibility. The regulator driver now has
> the correct voltage table for both regulator versions and those the
> correct voltage can be used in the device tree.

Acked-by: Stephen Warren <swarren@nvidia.com>
(yes, taking this through the MFD tree with patch 1/3 makes sense)

[PATCH v4 3/3] ARM: tegra: correct Colibri T20 regulator settings

[Stephen Warren]

On 12/03/2013 03:18 PM, Stefan Agner wrote:
> Set the parent of the regulators LDO2 to LDO9 according to the
> schematic. Set the base voltage to 3.3V, there is only 3.3V on the
> module itself.
> 
> Set the Core and CPU voltage to the specified voltages of 1.2V and
> 1.0V respectivly.
> 
> LDO6 should deliver 2.85V. The attached peripherals were not in
> use so far.

Does this depend on patch 1/3 or 2/3, or can I apply it to the Tegra
tree which doesn't include those patches?

If this does depend on patch 1/3 or 2/3... this patch feels a bit big to
apply to the MFD tree. Perhaps patch 1/3 and 2/3 could be applied alone
to a topic branch in the MFD tree so I can merge that into the Tegra
tree in order to apply this?

[PATCH v4 3/3] ARM: tegra: correct Colibri T20 regulator settings

[Stefan Agner]

Am 2013-12-05 18:12, schrieb Stephen Warren:
> On 12/03/2013 03:18 PM, Stefan Agner wrote:
>> Set the parent of the regulators LDO2 to LDO9 according to the
>> schematic. Set the base voltage to 3.3V, there is only 3.3V on the
>> module itself.
>>
>> Set the Core and CPU voltage to the specified voltages of 1.2V and
>> 1.0V respectivly.
>>
>> LDO6 should deliver 2.85V. The attached peripherals were not in
>> use so far.
> 
> Does this depend on patch 1/3 or 2/3, or can I apply it to the Tegra
> tree which doesn't include those patches?

This patch is independent from 1/3 and 2/3, so you can apply it to the
Tegra tree. The relevant DT change for 2/3 is included in 2/3...

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Stephen Warren]

On 12/05/2013 10:43 AM, Stefan Agner wrote:
> Am 2013-12-05 18:06, schrieb Stephen Warren:
> <snip>
>>> @@ -493,13 +527,12 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>>>  		return -EIO;
>>>  	}
>>>
>>> -	dev_info(&client->dev, "VERSIONCRC is %02x\n", ret);
>>> -
>>>  	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
>>> -	if (tps6586x == NULL) {
>>> -		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
>>> +	if (!tps6586x)
>>>  		return -ENOMEM;
>>> -	}
>>> +
>>> +	tps6586x->version = ret;
>>
>> I have to say, I dislike this version of the patch. Separating the
>> reading of the version register from the assignment to tps6586x->version
>> doesn't make any sense, especially given that the version value is
>> stored in a variable named "ret"; that name isn't remotely related to
>> what's stored there. What if someone comes along later and adds more
>> code that assigns to ret between where it's repurposed for the version
>> value and where it's assigned to tps6586x->version? It'd be extremely
>> difficult for a patch reviewer to spot that given the limited context in
>> a diff, and quite non-obvious to the person changing the code too..
> 
> The value comes from the return value of i2c_smbus_read_byte_data. If
> the value is below zero its an EIO error. 
> 
> I could add a variable "version", but for me it felt strange because we
> check if version is below zero. This feels like its a wrong version
> rather than a transmit error. So I would prefer ret over version. But I
> agree, when one just reads the patch, its not obvious what exactly
> happens.

In my opinion, using a variable named "version" here would be
preferable. Testing that against <0 is just the way the I2C API works,
so the same argument could be applied to any I2C access.

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Stefan Agner]

Am 2013-12-05 18:06, schrieb Stephen Warren:
<snip>
>> @@ -493,13 +527,12 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>>  		return -EIO;
>>  	}
>>
>> -	dev_info(&client->dev, "VERSIONCRC is %02x\n", ret);
>> -
>>  	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
>> -	if (tps6586x == NULL) {
>> -		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
>> +	if (!tps6586x)
>>  		return -ENOMEM;
>> -	}
>> +
>> +	tps6586x->version = ret;
> 
> I have to say, I dislike this version of the patch. Separating the
> reading of the version register from the assignment to tps6586x->version
> doesn't make any sense, especially given that the version value is
> stored in a variable named "ret"; that name isn't remotely related to
> what's stored there. What if someone comes along later and adds more
> code that assigns to ret between where it's repurposed for the version
> value and where it's assigned to tps6586x->version? It'd be extremely
> difficult for a patch reviewer to spot that given the limited context in
> a diff, and quite non-obvious to the person changing the code too..

The value comes from the return value of i2c_smbus_read_byte_data. If
the value is below zero its an EIO error. 

I could add a variable "version", but for me it felt strange because we
check if version is below zero. This feels like its a wrong version
rather than a transmit error. So I would prefer ret over version. But I
agree, when one just reads the patch, its not obvious what exactly
happens.

In v2, I moved the i2c_smbus_read_byte_data function call after the
allocation, so it was more obvious for the reader. But then, as Thierry
Reding pointed out, not moving it is an optimization: In case reading
fails, we don't allocate memory first.

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Stefan Agner]

Am 2013-12-05 18:40, schrieb Stephen Warren:
> On 12/05/2013 10:43 AM, Stefan Agner wrote:
>> Am 2013-12-05 18:06, schrieb Stephen Warren:
>> <snip>
>>>> @@ -493,13 +527,12 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>>>>  		return -EIO;
>>>>  	}
>>>>
>>>> -	dev_info(&client->dev, "VERSIONCRC is %02x\n", ret);
>>>> -
>>>>  	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
>>>> -	if (tps6586x == NULL) {
>>>> -		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
>>>> +	if (!tps6586x)
>>>>  		return -ENOMEM;
>>>> -	}
>>>> +
>>>> +	tps6586x->version = ret;
>>>
>>> I have to say, I dislike this version of the patch. Separating the
>>> reading of the version register from the assignment to tps6586x->version
>>> doesn't make any sense, especially given that the version value is
>>> stored in a variable named "ret"; that name isn't remotely related to
>>> what's stored there. What if someone comes along later and adds more
>>> code that assigns to ret between where it's repurposed for the version
>>> value and where it's assigned to tps6586x->version? It'd be extremely
>>> difficult for a patch reviewer to spot that given the limited context in
>>> a diff, and quite non-obvious to the person changing the code too..
>>
>> The value comes from the return value of i2c_smbus_read_byte_data. If
>> the value is below zero its an EIO error.
>>
>> I could add a variable "version", but for me it felt strange because we
>> check if version is below zero. This feels like its a wrong version
>> rather than a transmit error. So I would prefer ret over version. But I
>> agree, when one just reads the patch, its not obvious what exactly
>> happens.
> 
> In my opinion, using a variable named "version" here would be
> preferable. Testing that against <0 is just the way the I2C API works,
> so the same argument could be applied to any I2C access.

Hm, I try the empiric way:

$ grep -r -e i2c_smbus_read_byte_data | grep "ret =" | wc -l
139
$ grep -r -e i2c_smbus_read_byte_data | grep "version =" | wc -l
3

Ok, thats not fair at all, version is usage specific whilst ret is not.

$ grep -r -e i2c_smbus_read_byte_data | grep " = " | wc -l
703

On the other hand is the additional variable. But I think the compiler
will optimize that anyway, so this might not be an argument at all :-)

I see your point... Should I create another patch revision? Lee, is the
patch already merged?

[PATCH v4 1/3] mfd: tps6586x: add version detection

[Lee Jones]

> >> <snip>
> >>>> @@ -493,13 +527,12 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
> >>>>  		return -EIO;
> >>>>  	}
> >>>>
> >>>> -	dev_info(&client->dev, "VERSIONCRC is %02x\n", ret);
> >>>> -
> >>>>  	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
> >>>> -	if (tps6586x == NULL) {
> >>>> -		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
> >>>> +	if (!tps6586x)
> >>>>  		return -ENOMEM;
> >>>> -	}
> >>>> +
> >>>> +	tps6586x->version = ret;
> >>>
> >>> I have to say, I dislike this version of the patch. Separating the
> >>> reading of the version register from the assignment to tps6586x->version
> >>> doesn't make any sense, especially given that the version value is
> >>> stored in a variable named "ret"; that name isn't remotely related to
> >>> what's stored there. What if someone comes along later and adds more
> >>> code that assigns to ret between where it's repurposed for the version
> >>> value and where it's assigned to tps6586x->version? It'd be extremely
> >>> difficult for a patch reviewer to spot that given the limited context in
> >>> a diff, and quite non-obvious to the person changing the code too..
> >>
> >> The value comes from the return value of i2c_smbus_read_byte_data. If
> >> the value is below zero its an EIO error.
> >>
> >> I could add a variable "version", but for me it felt strange because we
> >> check if version is below zero. This feels like its a wrong version
> >> rather than a transmit error. So I would prefer ret over version. But I
> >> agree, when one just reads the patch, its not obvious what exactly
> >> happens.
> > 
> > In my opinion, using a variable named "version" here would be
> > preferable. Testing that against <0 is just the way the I2C API works,
> > so the same argument could be applied to any I2C access.

So, FWIW I agree with Stephen and have done from the start. Please
see my original comment from the first submission:

> >       ret = i2c_smbus_read_byte_data(client, TPS6586X_VERSIONCRC);
>   If you're going to do this, please change 'ret' to 'version'.

> Hm, I try the empiric way:
> 
> $ grep -r -e i2c_smbus_read_byte_data | grep "ret =" | wc -l
> 139
> $ grep -r -e i2c_smbus_read_byte_data | grep "version =" | wc -l
> 3
> 
> Ok, thats not fair at all, version is usage specific whilst ret is not.
> 
> $ grep -r -e i2c_smbus_read_byte_data | grep " = " | wc -l
> 703

I not really that worried about what everyone else does. I'm more
concerned with doing what we deem to be the correct thing here.

> On the other hand is the additional variable. But I think the compiler
> will optimize that anyway, so this might not be an argument at all :-)
> 
> I see your point... Should I create another patch revision? Lee, is the
> patch already merged?

It isn't. Please submit another version as Stephen requests.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog