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

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

[Stefan Agner]

This patchset adds version detection for the tps6586x mfd family. This is
required because some regulator versions use different voltage tables. The
regulator driver now uses the right voltage table according to the
version.

The required voltage for the SM2 converter on the Colibri T20 is 1.8V,
however, the device tree states 3.7V right now. Newer versions (>=V1.2) of
the Colibri T20 module use the TPS658643, whereas old ones (<V1.2) use the
TPS658623. Both of them have a different voltage table for SM2. The
current device tree contains a voltage which results in 1.8V for the older
TPS658623. Since the regulator driver has correct voltage tables for both
regulator versions now, we can request the correct 1.8V from the device
tree.

Tested on Colibri T20 V1.1 and V1.2.

Stefan Agner (3):
  mfd: tps6586x: add version detection
  regulator: tps6586x: add voltage table for tps658643
  ARM: tegra: set SM2 voltage correct

 arch/arm/boot/dts/tegra20-colibri-512.dtsi |  4 +-
 drivers/mfd/tps6586x.c                     | 41 +++++++++++--
 drivers/regulator/tps6586x-regulator.c     | 97 ++++++++++++++++++++----------
 include/linux/mfd/tps6586x.h               |  9 +++
 4 files changed, 110 insertions(+), 41 deletions(-)

-- 
1.8.4.2

[PATCH 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>
---
 drivers/mfd/tps6586x.c       | 41 +++++++++++++++++++++++++++++++++++------
 include/linux/mfd/tps6586x.h |  9 +++++++++
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index ee61fd7..a3ba9c1 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;
+	enum tps6586x_version	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);
 
+enum tps6586x_version 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));
@@ -477,6 +486,7 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
 {
 	struct tps6586x_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct tps6586x *tps6586x;
+	const char *name = "";
 	int ret;
 
 	if (!pdata && client->dev.of_node)
@@ -487,20 +497,39 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
 		return -ENOTSUPP;
 	}
 
+	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
+	if (tps6586x == NULL) {
+		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
+		return -ENOMEM;
+	}
+
 	ret = i2c_smbus_read_byte_data(client, TPS6586X_VERSIONCRC);
 	if (ret < 0) {
 		dev_err(&client->dev, "Chip ID read failed: %d\n", ret);
 		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");
-		return -ENOMEM;
+	tps6586x->version = (enum tps6586x_version)ret;
+	switch (ret) {
+	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, ret);
+
 	tps6586x->client = client;
 	tps6586x->dev = &client->dev;
 	i2c_set_clientdata(client, tps6586x);
diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
index 8799454..40a36a0 100644
--- a/include/linux/mfd/tps6586x.h
+++ b/include/linux/mfd/tps6586x.h
@@ -62,6 +62,14 @@ enum {
 	TPS6586X_INT_RTC_ALM2,
 };
 
+enum tps6586x_version {
+	TPS658621A	= 0x15,
+	TPS658621CD	= 0x2c,
+	TPS658623	= 0x1b,
+	TPS658643	= 0x03,
+	TPS6586X_ANY	= -1,
+};
+
 struct tps6586x_settings {
 	int slew_rate;
 };
@@ -97,5 +105,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 enum tps6586x_version tps6586x_get_version(struct device *dev);
 
 #endif /*__LINUX_MFD_TPS6586X_H */
-- 
1.8.4.2

[PATCH 2/3] regulator: tps6586x: add voltage table for tps658643

[Stefan Agner]

Depending on version, the voltage table might be different. Add version
compatibility to the regulator information in order to select correct
voltage table.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/regulator/tps6586x-regulator.c | 97 ++++++++++++++++++++++------------
 1 file changed, 64 insertions(+), 33 deletions(-)

diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
index e8e3a8a..4fe844c 100644
--- a/drivers/regulator/tps6586x-regulator.c
+++ b/drivers/regulator/tps6586x-regulator.c
@@ -57,6 +57,8 @@
 #define TPS6586X_SMODE2		0x48
 
 struct tps6586x_regulator {
+	enum tps6586x_version version;
+
 	struct regulator_desc desc;
 
 	int enable_bit[2];
@@ -86,7 +88,7 @@ static const unsigned int tps6586x_ldo0_voltages[] = {
 	1200000, 1500000, 1800000, 2500000, 2700000, 2850000, 3100000, 3300000,
 };
 
-static const unsigned int tps6586x_ldo4_voltages[] = {
+static const unsigned int tps6586x_ldo4_sm2_voltages[] = {
 	1700000, 1725000, 1750000, 1775000, 1800000, 1825000, 1850000, 1875000,
 	1900000, 1925000, 1950000, 1975000, 2000000, 2025000, 2050000, 2075000,
 	2100000, 2125000, 2150000, 2175000, 2200000, 2225000, 2250000, 2275000,
@@ -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,
@@ -111,16 +120,16 @@ static const unsigned int tps6586x_dvm_voltages[] = {
 	1325000, 1350000, 1375000, 1400000, 1425000, 1450000, 1475000, 1500000,
 };
 
-#define TPS6586X_REGULATOR(_id, _pin_name, vdata, vreg, shift, nbits,	\
-			   ereg0, ebit0, ereg1, ebit1, goreg, gobit)	\
+#define TPS6586X_REG(_ver, _id, _pin_name, vdata, vreg, shift,	nbits,	\
+		     ereg0, ebit0, ereg1, ebit1, goreg, gobit)		\
 	.desc	= {							\
 		.supply_name = _pin_name,				\
 		.name	= "REG-" #_id,					\
 		.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),				\
@@ -129,23 +138,24 @@ static const unsigned int tps6586x_dvm_voltages[] = {
 		.apply_reg = (goreg),				\
 		.apply_bit = (gobit),				\
 	},								\
+	.version	= _ver,						\
 	.enable_reg[0]	= TPS6586X_SUPPLY##ereg0,			\
 	.enable_bit[0]	= (ebit0),					\
 	.enable_reg[1]	= TPS6586X_SUPPLY##ereg1,			\
 	.enable_bit[1]	= (ebit1),
 
-#define TPS6586X_LDO(_id, _pname, vdata, vreg, shift, nbits,		\
+#define TPS6586X_LDO(_ver, _id, _pname, vdata, vreg, shift, nbits,	\
 		     ereg0, ebit0, ereg1, ebit1)			\
 {									\
-	TPS6586X_REGULATOR(_id, _pname, vdata, vreg, shift, nbits,	\
-			   ereg0, ebit0, ereg1, ebit1, 0, 0)		\
+	TPS6586X_REG(_ver, _id, _pname, vdata, vreg, shift, nbits,	\
+		     ereg0, ebit0, ereg1, ebit1, 0, 0)	\
 }
 
-#define TPS6586X_DVM(_id, _pname, vdata, vreg, shift, nbits,		\
+#define TPS6586X_DVM(_ver, _id, _pname, vdata, vreg, shift, nbits,	\
 		     ereg0, ebit0, ereg1, ebit1, goreg, gobit)		\
 {									\
-	TPS6586X_REGULATOR(_id, _pname, vdata, vreg, shift, nbits,	\
-			   ereg0, ebit0, ereg1, ebit1, goreg, gobit)	\
+	TPS6586X_REG(_ver, _id, _pname, vdata, vreg, shift, nbits,	\
+		     ereg0, ebit0, ereg1, ebit1, goreg, gobit)		\
 }
 
 #define TPS6586X_SYS_REGULATOR()					\
@@ -158,29 +168,45 @@ static const unsigned int tps6586x_dvm_voltages[] = {
 		.id	= TPS6586X_ID_SYS,				\
 		.owner	= THIS_MODULE,					\
 	},								\
+	.version	= TPS6586X_ANY,					\
 }
 
+/* Add version specific entries before any */
 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,
-					ENB, 3, TPS6586X_VCC2, BIT(6)),
-	TPS6586X_DVM(LDO_4, "vinldo4", ldo4, LDO4V1, 0, 5, ENC, 3,
-					END, 3, TPS6586X_VCC1, BIT(6)),
-	TPS6586X_DVM(SM_0, "vin-sm0", dvm, SM0V1, 0, 5, ENA, 1,
-					ENB, 1, TPS6586X_VCC1, BIT(2)),
-	TPS6586X_DVM(SM_1, "vin-sm1", dvm, SM1V1, 0, 5, ENA, 0,
-					ENB, 0, TPS6586X_VCC1, BIT(0)),
+	TPS6586X_LDO(TPS6586X_ANY, LDO_0, "vinldo01", tps6586x_ldo0, SUPPLYV1,
+			5, 3, ENC, 0, END, 0),
+	TPS6586X_LDO(TPS6586X_ANY, LDO_3, "vinldo23", tps6586x_ldo, SUPPLYV4,
+			0, 3, ENC, 2, END, 2),
+	TPS6586X_LDO(TPS6586X_ANY, LDO_5, "REG-SYS", tps6586x_ldo, SUPPLYV6,
+			0, 3, ENE, 6, ENE, 6),
+	TPS6586X_LDO(TPS6586X_ANY, LDO_6, "vinldo678", tps6586x_ldo, SUPPLYV3,
+			0, 3, ENC, 4, END, 4),
+	TPS6586X_LDO(TPS6586X_ANY, LDO_7, "vinldo678", tps6586x_ldo, SUPPLYV3,
+			3, 3, ENC, 5, END, 5),
+	TPS6586X_LDO(TPS6586X_ANY, LDO_8, "vinldo678", tps6586x_ldo, SUPPLYV2,
+			5, 3, ENC, 6, END, 6),
+	TPS6586X_LDO(TPS6586X_ANY, LDO_9, "vinldo9", tps6586x_ldo, SUPPLYV6,
+			3, 3, ENE, 7, ENE, 7),
+	TPS6586X_LDO(TPS6586X_ANY, LDO_RTC, "REG-SYS", tps6586x_ldo, SUPPLYV4,
+			3, 3, V4, 7, V4, 7),
+	TPS6586X_LDO(TPS6586X_ANY, LDO_1, "vinldo01", tps6586x_dvm, SUPPLYV1,
+			0, 5, ENC, 1, END, 1),
+	TPS6586X_LDO(TPS658623, SM_2, "vin-sm2", tps6586x_ldo4_sm2, SUPPLYV2,
+			0, 5, ENC, 7, END, 7),
+	TPS6586X_LDO(TPS658643, SM_2, "vin-sm2", tps658643_sm2, SUPPLYV2,
+			0, 5, ENC, 7, END, 7),
+	TPS6586X_LDO(TPS6586X_ANY, SM_2, "vin-sm2", tps6586x_sm2, SUPPLYV2,
+			0, 5, ENC, 7, END, 7),
+
+	TPS6586X_DVM(TPS6586X_ANY, LDO_2, "vinldo23", tps6586x_dvm, LDO2BV1,
+			0, 5, ENA, 3, ENB, 3, TPS6586X_VCC2, BIT(6)),
+	TPS6586X_DVM(TPS6586X_ANY, LDO_4, "vinldo4", tps6586x_ldo4_sm2, LDO4V1,
+			0, 5, ENC, 3, END, 3, TPS6586X_VCC1, BIT(6)),
+	TPS6586X_DVM(TPS6586X_ANY, SM_0, "vin-sm0", tps6586x_dvm, SM0V1,
+			0, 5, ENA, 1, ENB, 1, TPS6586X_VCC1, BIT(2)),
+	TPS6586X_DVM(TPS6586X_ANY, SM_1, "vin-sm1", tps6586x_dvm, SM1V1,
+			0, 5, ENA, 0, ENB, 0, TPS6586X_VCC1, BIT(0)),
 };
 
 /*
@@ -254,14 +280,16 @@ 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 inline struct tps6586x_regulator *find_regulator_info(int id,
+			enum tps6586x_version version)
 {
 	struct tps6586x_regulator *ri;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(tps6586x_regulator); i++) {
 		ri = &tps6586x_regulator[i];
-		if (ri->desc.id == id)
+		if (ri->desc.id == id && ((ri->version == version) ||
+					  (ri->version == TPS6586X_ANY)))
 			return ri;
 	}
 	return NULL;
@@ -351,6 +379,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;
+	enum tps6586x_version reg_version;
 	int id;
 	int err;
 
@@ -373,10 +402,12 @@ 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);
+		ri = find_regulator_info(id, reg_version);
 		if (!ri) {
 			dev_err(&pdev->dev, "invalid regulator ID specified\n");
 			return -EINVAL;
-- 
1.8.4.2

[PATCH 3/3] ARM: tegra: set SM2 voltage correct

[Stefan Agner]

Set the requested SM2 voltage to the correct value of 1.8V. The value
before used to work on TPS658623 since the driver applied a wrong
voltage table too. However, the TPS658643 used on newer devices uses
yet another voltage table and those broke that compatibility. The
regulator driver now has the correct voltage table for both regulator
versions and those the correct voltage can be used in this device
tree.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 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;
 				};
 
-- 
1.8.4.2

Re: [PATCH 3/3] ARM: tegra: set SM2 voltage correct

[Lucas Stach]

Hi Stefan,

Am Mittwoch, den 27.11.2013, 00:45 +0100 schrieb Stefan Agner:
> Set the requested SM2 voltage to the correct value of 1.8V. The value
> before used to work on TPS658623 since the driver applied a wrong
> voltage table too. However, the TPS658643 used on newer devices uses
> yet another voltage table and those broke that compatibility. The
> regulator driver now has the correct voltage table for both regulator
> versions and those the correct voltage can be used in this device
> tree.

This isn't a global Tegra change, but very specific to the Colibri, so
please reword your commit headline to reflect that.

Also there are other issues with the regulator setup on Colibri, I sent
a patch for this a good while ago, but didn't come around to revise it
until now. So if you are going to touch things here, please look up that
patch and fold it into this one.

I'll take a look at the other patches later today.

Regards,
Lucas

> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 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;
>  				};
>  

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

Re: [PATCH 3/3] ARM: tegra: set SM2 voltage correct

[Stefan Agner]

Hi Lucas,

Am 2013-11-27 10:59, schrieb Lucas Stach:
> This isn't a global Tegra change, but very specific to the Colibri, so
> please reword your commit headline to reflect that.
Agree, will do that.

> Also there are other issues with the regulator setup on Colibri, I sent
> a patch for this a good while ago, but didn't come around to revise it
> until now. So if you are going to touch things here, please look up that
> patch and fold it into this one.
I guess you refer to this patchset:
http://thread.gmane.org/gmane.linux.ports.tegra/11984

Ok, I will fold patch 4 into my patchset. Not sure whether I should add
patch 3 (ULPI phy on Colibri T20) as well, since this one is somewhat
unrelated...

> I'll take a look at the other patches later today.
Thanks

--
Stefan

Re: [PATCH 3/3] ARM: tegra: set SM2 voltage correct

[Lucas Stach]

Am Mittwoch, den 27.11.2013, 12:05 +0100 schrieb Stefan Agner:
> Hi Lucas,
> 
> Am 2013-11-27 10:59, schrieb Lucas Stach:
> > This isn't a global Tegra change, but very specific to the Colibri, so
> > please reword your commit headline to reflect that.
> Agree, will do that.
> 
> > Also there are other issues with the regulator setup on Colibri, I sent
> > a patch for this a good while ago, but didn't come around to revise it
> > until now. So if you are going to touch things here, please look up that
> > patch and fold it into this one.
> I guess you refer to this patchset:
> http://thread.gmane.org/gmane.linux.ports.tegra/11984
> 
> Ok, I will fold patch 4 into my patchset. Not sure whether I should add
> patch 3 (ULPI phy on Colibri T20) as well, since this one is somewhat
> unrelated...
> 
The ULPI patch is already upstream.

At least the first hunk of patch 4 should be folded into your patch, as
it's a real regulator bugfix like your correction of the sm2 voltage.

Lowering sm0 and sm1 voltages is somewhat an optimization and may be
done as a separate patch.

Regards,
Lucas
-- 
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 |

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

[Lee Jones]

<snip>

>  static int __remove_subdev(struct device *dev, void *unused)
>  {
>  	platform_device_unregister(to_platform_device(dev));
> @@ -477,6 +486,7 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>  {
>  	struct tps6586x_platform_data *pdata = dev_get_platdata(&client->dev);
>  	struct tps6586x *tps6586x;
> +	const char *name = "";

How much memory space does this guarantee?

>  	int ret;
>  
>  	if (!pdata && client->dev.of_node)
> @@ -487,20 +497,39 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>  		return -ENOTSUPP;
>  	}
>  
> +	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
> +	if (tps6586x == NULL) {

Can you take this oppotunity to change to:
  if (!tps6586x)

> +		dev_err(&client->dev, "memory for tps6586x alloc failed\n");

I'm not keen on -ENOMEM prints, just return -ENOMEM and be done with it.

> +		return -ENOMEM;
> +	}
> +
>  	ret = i2c_smbus_read_byte_data(client, TPS6586X_VERSIONCRC);

If you're going to do this, please change 'ret' to 'version'.

>  	if (ret < 0) {
>  		dev_err(&client->dev, "Chip ID read failed: %d\n", ret);
>  		return -EIO;

Why are you returning an error here when you have a valid enum of:
  TPS6586X_ANY	= -1,

>  	}
>  
> -	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");
> -		return -ENOMEM;
> +	tps6586x->version = (enum tps6586x_version)ret;
> +	switch (ret) {
> +	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, ret);
> +

I'd suggest pulling this out of probe() and into a separate subroutine.

<snip>

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

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

[Lee Jones]

Oh, and it's usually a good idea to CC all of the maintainers.

This patch nearly slipped through the gaps.

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

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

[Stefan Agner]

Am 2013-11-27 14:09, schrieb Lee Jones:
> <snip>
> 
>>  static int __remove_subdev(struct device *dev, void *unused)
>>  {
>>  	platform_device_unregister(to_platform_device(dev));
>> @@ -477,6 +486,7 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>>  {
>>  	struct tps6586x_platform_data *pdata = dev_get_platdata(&client->dev);
>>  	struct tps6586x *tps6586x;
>> +	const char *name = "";
>
> How much memory space does this guarantee?
>
Well, its just an empty string (so, 1). Actually I could assign NULL
here, since we do have a default in the case later on.

> 
>>  	int ret;
>>
>>  	if (!pdata && client->dev.of_node)
>> @@ -487,20 +497,39 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>>  		return -ENOTSUPP;
>>  	}
>>
>> +	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
>> +	if (tps6586x == NULL) {
> 
> Can you take this oppotunity to change to:
>   if (!tps6586x)
> 
>> +		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
> 
> I'm not keen on -ENOMEM prints, just return -ENOMEM and be done with it.
> 
>> +		return -ENOMEM;
>> +	}
>> +
>>  	ret = i2c_smbus_read_byte_data(client, TPS6586X_VERSIONCRC);
> 
> If you're going to do this, please change 'ret' to 'version'.
> 
>>  	if (ret < 0) {
>>  		dev_err(&client->dev, "Chip ID read failed: %d\n", ret);
>>  		return -EIO;
> 
> Why are you returning an error here when you have a valid enum of:
>   TPS6586X_ANY	= -1,
> 
Hm, when the device is not answering on that request, the probe method
should fail I would say. This means that the device is missing most
likely. However, I should set the device version to TPS6586X_ANY if I
happen to end up in the default case.

>>  	}
>>
>> -	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");
>> -		return -ENOMEM;
>> +	tps6586x->version = (enum tps6586x_version)ret;
>> +	switch (ret) {
>> +	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, ret);
>> +
> 
> I'd suggest pulling this out of probe() and into a separate subroutine.
> 
> <snip>
Since I will alter the version when I end up in the default case in my
next patch, would you still do a separate subroutine? I think its
somewhat heavily coupled to the probe function.

Sorry missing you on the CC, will do next time :-)

--
Stefan

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

[Lee Jones]

> >>  	if (ret < 0) {
> >>  		dev_err(&client->dev, "Chip ID read failed: %d\n", ret);
> >>  		return -EIO;
> > 
> > Why are you returning an error here when you have a valid enum of:
> >   TPS6586X_ANY	= -1,
> > 
> Hm, when the device is not answering on that request, the probe method
> should fail I would say. This means that the device is missing most
> likely. However, I should set the device version to TPS6586X_ANY if I
> happen to end up in the default case.

I would say that returning an error is the sound thing to do, but I'm
missing the point of TPS6586X_ANY, as it doesn't appear to be used in
this context.

> >>  	}
> >>
> >> -	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");
> >> -		return -ENOMEM;
> >> +	tps6586x->version = (enum tps6586x_version)ret;
> >> +	switch (ret) {
> >> +	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, ret);
> >> +
> > 
> > I'd suggest pulling this out of probe() and into a separate subroutine.
> > 
> > <snip>
> Since I will alter the version when I end up in the default case in my
> next patch, would you still do a separate subroutine? I think its
> somewhat heavily coupled to the probe function.
> 
> Sorry missing you on the CC, will do next time :-)

It's not that it's unrelated, it's just a whole bulk of code which is
essentially featureless. 17 lines of code just to have the name of the
device in the bootlog. For that reason I'd like to see this abstracted
from (the useful stuff in) probe() please.

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

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

[Lee Jones]

Re-adding the list.

Please remember to 'reply to all' when discussing patches.

> > >> Hm, when the device is not answering on that request, the probe method
> > >> should fail I would say. This means that the device is missing most
> > >> likely. However, I should set the device version to TPS6586X_ANY if I
> > >> happen to end up in the default case.
> > > 
> > > I would say that returning an error is the sound thing to do, but I'm
> > > missing the point of TPS6586X_ANY, as it doesn't appear to be used in
> > > this context.
> > > 
> > 
> > Yes, its mainly used in the regulator code later on. I do assign voltage
> > table to TPS6586X_ANY, if its the appropriate table for any version of
> > the device. 
> 
> <snip>
> 
> > Ok, will change this. Setting the version to TPS6586X_ANY in the default
> > case is anyway not a good idea since it would suppress unknown versions.
> 
> Perhaps I should suggest to make TPS6586X_ANY a positive number then,
> as a negative value to me indicates more of an error than a generic
> parameter.

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

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

[Stefan Agner]

Am 2013-11-27 15:36, schrieb Lee Jones:
<snip>
>> Perhaps I should suggest to make TPS6586X_ANY a positive number then,
>> as a negative value to me indicates more of an error than a generic
>> parameter.
I see, its especially confusing since the version is filled using the
i2c_smbus_read_byte_data functions return value. The version field is a
8-Bit value according to the data sheet, I could use 0x100 as
TPS6586X_ANY identifier.

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

[Lee Jones]

On Wed, 27 Nov 2013, Stefan Agner wrote:

> Am 2013-11-27 15:36, schrieb Lee Jones:
> <snip>
> >> Perhaps I should suggest to make TPS6586X_ANY a positive number then,
> >> as a negative value to me indicates more of an error than a generic
> >> parameter.
> I see, its especially confusing since the version is filled using the
> i2c_smbus_read_byte_data functions return value. The version field is a
> 8-Bit value according to the data sheet, I could use 0x100 as
> TPS6586X_ANY identifier.

How far are we away from using 0xFF?

I'd be happy to use that and change it _if_ we ever get close.

If it's likely that it'll be used, then sure 0x100 sounds okay too.

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

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

[Stefan Agner]

Am 2013-11-27 16:30, schrieb Lee Jones:
> On Wed, 27 Nov 2013, Stefan Agner wrote:
> 
>> Am 2013-11-27 15:36, schrieb Lee Jones:
>> <snip>
>> >> Perhaps I should suggest to make TPS6586X_ANY a positive number then,
>> >> as a negative value to me indicates more of an error than a generic
>> >> parameter.
>> I see, its especially confusing since the version is filled using the
>> i2c_smbus_read_byte_data functions return value. The version field is a
>> 8-Bit value according to the data sheet, I could use 0x100 as
>> TPS6586X_ANY identifier.
> 
> How far are we away from using 0xFF?
> 
> I'd be happy to use that and change it _if_ we ever get close.
> 
> If it's likely that it'll be used, then sure 0x100 sounds okay too.

Yes, I thought about 0xFF too. The latest device we support is TPS658643
(according to data sheet release dates), which has the smallest version
number (03). Since it seems to be a CRC (hence VERSIONCRC) the number is
quite random. Also, 0xFF sounds like a bitmask which can mask all
versions, but the versions can't be used bitwise... So I would prefer to
go with 0x100.

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

[Lee Jones]

On Wed, 27 Nov 2013, Stefan Agner wrote:

> Am 2013-11-27 16:30, schrieb Lee Jones:
> > On Wed, 27 Nov 2013, Stefan Agner wrote:
> > 
> >> Am 2013-11-27 15:36, schrieb Lee Jones:
> >> <snip>
> >> >> Perhaps I should suggest to make TPS6586X_ANY a positive number then,
> >> >> as a negative value to me indicates more of an error than a generic
> >> >> parameter.
> >> I see, its especially confusing since the version is filled using the
> >> i2c_smbus_read_byte_data functions return value. The version field is a
> >> 8-Bit value according to the data sheet, I could use 0x100 as
> >> TPS6586X_ANY identifier.
> > 
> > How far are we away from using 0xFF?
> > 
> > I'd be happy to use that and change it _if_ we ever get close.
> > 
> > If it's likely that it'll be used, then sure 0x100 sounds okay too.
> 
> Yes, I thought about 0xFF too. The latest device we support is TPS658643
> (according to data sheet release dates), which has the smallest version
> number (03). Since it seems to be a CRC (hence VERSIONCRC) the number is
> quite random. Also, 0xFF sounds like a bitmask which can mask all
> versions, but the versions can't be used bitwise... So I would prefer to
> go with 0x100.

Deal!

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

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

[Stephen Warren]

On 11/26/2013 04:45 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

> @@ -477,6 +486,7 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>  {
>  	struct tps6586x_platform_data *pdata = dev_get_platdata(&client->dev);
>  	struct tps6586x *tps6586x;
> +	const char *name = "";

There's no need to assign any value here; the switch below always
assigns something over the top of the variable, so it doesn't need to be
pre-initialized.

> +	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
> +	if (tps6586x == NULL) {
> +		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
> +		return -ENOMEM;
> +	}

> +	tps6586x->version = (enum tps6586x_version)ret;

Why not make the version variable an integer of some kind. Then, the
cast wouldn't be required. The values like TPS658621A can be #defines or
const u32.

> +	switch (ret) {

That should switch on tps6586x->version since that's been assigned;
it'll make the purpose a bit clearer.

> +	default:
> +		name = "TPS6586X";
> +		break;
>  	}

I hope the rest of the code deals with unknown values of
tps6586x->version OK.

Re: [PATCH 2/3] regulator: tps6586x: add voltage table for tps658643

[Stephen Warren]

On 11/26/2013 04:45 PM, Stefan Agner wrote:
> Depending on version, the voltage table might be different. Add version
> compatibility to the regulator information in order to select correct
> voltage table.

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

> -static const unsigned int tps6586x_ldo4_voltages[] = {
> +static const unsigned int tps6586x_ldo4_sm2_voltages[] = {

> +static const unsigned int tps658643_sm2_voltages[] = {

What's the logic behind the "ldo4_sm2" v.s. "sm2" naming? Does it match
the data sheet in some way? If not, it might be better to name this
something like "tps6586x_ldo4_voltages" and "tps65863_ldo4_voltages".

> -#define TPS6586X_REGULATOR(_id, _pin_name, vdata, vreg, shift, nbits,	\
> -			   ereg0, ebit0, ereg1, ebit1, goreg, gobit)	\
> +#define TPS6586X_REG(_ver, _id, _pin_name, vdata, vreg, shift,	nbits,	\
> +		     ereg0, ebit0, ereg1, ebit1, goreg, gobit)		\

Why rename the macro?

There's an embedded TAB before "nbits".

> +/* Add version specific entries before any */
>  static struct tps6586x_regulator tps6586x_regulator[] = {
>  	TPS6586X_SYS_REGULATOR(),
> -	TPS6586X_LDO(LDO_0, "vinldo01", ldo0, SUPPLYV1, 5, 3, ENC, 0, END, 0),
...
> +	TPS6586X_LDO(TPS6586X_ANY, LDO_0, "vinldo01", tps6586x_ldo0, SUPPLYV1,
> +			5, 3, ENC, 0, END, 0),

Rather than changing all the macros and table entries, wouldn't it be
much simpler to:

1) Make tps6586x_regulator[] only contain all the common regulator
definitions.

2) Add new version-specific tables for each version of regulator, so
tps6586x_other_regulator[] and tps65863_regulator[].

3) Have probe() walk multiple tables of regulators, selecting which
tables to walk based on version.

That would result in a much smaller and less invasive diff.

Re: [PATCH 3/3] ARM: tegra: set SM2 voltage correct

[Stephen Warren]

On 11/26/2013 04:45 PM, Stefan Agner wrote:
> Set the requested SM2 voltage to the correct value of 1.8V. The value
> before used to work on TPS658623 since the driver applied a wrong
> voltage table too. However, the TPS658643 used on newer devices uses
> yet another voltage table and those broke that compatibility. The
> regulator driver now has the correct voltage table for both regulator
> versions and those the correct voltage can be used in this device
> tree.

One thing you haven't called out explicitly here is that this series is
an incompatible change to the DT, since the old buggy driver used to
allow old buggy DT content to accidentally work.

I'm not too familiar with who's using mainline on the Colibri boards.
Hopefully everyone doing that is using in-kernel DTs, so this
incompatible change won't be any issue for anyone.

This patch needs to be rolled into patch 2/3 so that "git bisect" isn't
broken.

You mention there's yet another PMIC version used on later boards. Do we
need a new DT for that specfic version of the Colibri board?

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

[Stefan Agner]

Am 2013-11-27 17:58, schrieb Stephen Warren:
<snip>
>> +	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
>> +	if (tps6586x == NULL) {
>> +		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
>> +		return -ENOMEM;
>> +	}
> 
>> +	tps6586x->version = (enum tps6586x_version)ret;
> 
> Why not make the version variable an integer of some kind. Then, the
> cast wouldn't be required. The values like TPS658621A can be #defines or
> const u32.
> 
Yep this would work too. Whatever is preferred, maybe define would be
more consistent since SLEW_RATE are defines too (both, the VERSIONCRC
and SLEW_RATE values are possible content of registers).

>> +	switch (ret) {
> 
> That should switch on tps6586x->version since that's been assigned;
> it'll make the purpose a bit clearer.
> 
>> +	default:
>> +		name = "TPS6586X";
>> +		break;
>>  	}
> 
> I hope the rest of the code deals with unknown values of
> tps6586x->version OK.
>
Well, the tps6586x->version is not completly unknown, its something
valid which is returned by i2c_smbus_read_byte_data. According to the
data sheet this should be a 8-Bit value.

However, the patch should handle every version, since I tried to make
the change backward compatible: The driver now and then supports every
device version, just the default voltage table are applied if there are
no specific tables available.

Re: [PATCH 2/3] regulator: tps6586x: add voltage table for tps658643

[Stefan Agner]

Am 2013-11-27 18:09, schrieb Stephen Warren:
> On 11/26/2013 04:45 PM, Stefan Agner wrote:
>> Depending on version, the voltage table might be different. Add version
>> compatibility to the regulator information in order to select correct
>> voltage table.
> 
>> diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
> 
>> -static const unsigned int tps6586x_ldo4_voltages[] = {
>> +static const unsigned int tps6586x_ldo4_sm2_voltages[] = {
> 
>> +static const unsigned int tps658643_sm2_voltages[] = {
> 
> What's the logic behind the "ldo4_sm2" v.s. "sm2" naming? Does it match
> the data sheet in some way? If not, it might be better to name this
> something like "tps6586x_ldo4_voltages" and "tps65863_ldo4_voltages".
> 

This table is used for the TPS6586X (e.g. TPS658621A/D, maybe others)
variant for the LDO_4 regulator. The same table applies for TPS658623
for the SM2 regulator as well, so this naming should reflect that fact.

I could be some more verbose, e.g. tps6586x_ldo4_tps658623_sm2_voltages.
The other solution would be to create two independent (but identically)
voltage tables, but takes up more space...

>> -#define TPS6586X_REGULATOR(_id, _pin_name, vdata, vreg, shift, nbits,	\
>> -			   ereg0, ebit0, ereg1, ebit1, goreg, gobit)	\
>> +#define TPS6586X_REG(_ver, _id, _pin_name, vdata, vreg, shift,	nbits,	\
>> +		     ereg0, ebit0, ereg1, ebit1, goreg, gobit)		\
> 
> Why rename the macro?
> 
> There's an embedded TAB before "nbits".
> 

I must admit that the only reason doing the renaming was to allow the
parameters on two lines while keeping the 80 character limit...

>> +/* Add version specific entries before any */
>>  static struct tps6586x_regulator tps6586x_regulator[] = {
>>  	TPS6586X_SYS_REGULATOR(),
>> -	TPS6586X_LDO(LDO_0, "vinldo01", ldo0, SUPPLYV1, 5, 3, ENC, 0, END, 0),
> ...
>> +	TPS6586X_LDO(TPS6586X_ANY, LDO_0, "vinldo01", tps6586x_ldo0, SUPPLYV1,
>> +			5, 3, ENC, 0, END, 0),
> 
> Rather than changing all the macros and table entries, wouldn't it be
> much simpler to:
> 
> 1) Make tps6586x_regulator[] only contain all the common regulator
> definitions.
> 
> 2) Add new version-specific tables for each version of regulator, so
> tps6586x_other_regulator[] and tps65863_regulator[].
> 
> 3) Have probe() walk multiple tables of regulators, selecting which
> tables to walk based on version.
> 
> That would result in a much smaller and less invasive diff.

Hm, sounds easier yes. Would also remove the need for correct ordering,
which is a bit ugly. I will try that, lets see how this works out.

Re: [PATCH 3/3] ARM: tegra: set SM2 voltage correct

[Stefan Agner]

Am 2013-11-27 18:13, schrieb Stephen Warren:
> On 11/26/2013 04:45 PM, Stefan Agner wrote:
>> Set the requested SM2 voltage to the correct value of 1.8V. The value
>> before used to work on TPS658623 since the driver applied a wrong
>> voltage table too. However, the TPS658643 used on newer devices uses
>> yet another voltage table and those broke that compatibility. The
>> regulator driver now has the correct voltage table for both regulator
>> versions and those the correct voltage can be used in this device
>> tree.
> 
> One thing you haven't called out explicitly here is that this series is
> an incompatible change to the DT, since the old buggy driver used to
> allow old buggy DT content to accidentally work.
> 
> I'm not too familiar with who's using mainline on the Colibri boards.
> Hopefully everyone doing that is using in-kernel DTs, so this
> incompatible change won't be any issue for anyone.
> 

I don't think there are a lot of users since Toradex ships the NVidia
downstream kernel. Two users for sure, Lucas and me.

> This patch needs to be rolled into patch 2/3 so that "git bisect" isn't
> broken.

Will do

 
> You mention there's yet another PMIC version used on later boards. Do we
> need a new DT for that specfic version of the Colibri board?

The newer device is the TPS658643, which uses a different voltage table,
but this change is already in the driver. Maybe I should reword that
commit a bit. I just tried to point out why the change is really needed
(since we have two regulators with different voltage tables). But there
is nothing beyond those two that... 

Because the DT now states the real, required voltage, a new device
(version) would need an appropriate driver change and things should
work.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On Wed, Nov 27, 2013 at 12:45:42AM +0100, Stefan Agner wrote:
> This patchset adds version detection for the tps6586x mfd family. This is
> required because some regulator versions use different voltage tables. The
> regulator driver now uses the right voltage table according to the
> version.
> 
> The required voltage for the SM2 converter on the Colibri T20 is 1.8V,
> however, the device tree states 3.7V right now. Newer versions (>=V1.2) of
> the Colibri T20 module use the TPS658643, whereas old ones (<V1.2) use the
> TPS658623. Both of them have a different voltage table for SM2. The
> current device tree contains a voltage which results in 1.8V for the older
> TPS658623. Since the regulator driver has correct voltage tables for both
> regulator versions now, we can request the correct 1.8V from the device
> tree.
> 
> Tested on Colibri T20 V1.1 and V1.2.
> 
> Stefan Agner (3):
>   mfd: tps6586x: add version detection
>   regulator: tps6586x: add voltage table for tps658643
>   ARM: tegra: set SM2 voltage correct
> 
>  arch/arm/boot/dts/tegra20-colibri-512.dtsi |  4 +-
>  drivers/mfd/tps6586x.c                     | 41 +++++++++++--
>  drivers/regulator/tps6586x-regulator.c     | 97 ++++++++++++++++++++----------
>  include/linux/mfd/tps6586x.h               |  9 +++
>  4 files changed, 110 insertions(+), 41 deletions(-)

Adding Kai Poggensee on Cc. There was a variant of the Tamonten board
with one of these versions of the chip (or maybe it was even yet another
one).

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] regulator: tps6586x: add voltage table for tps658643

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3097 bytes --]

On Wed, Nov 27, 2013 at 10:56:10PM +0100, Stefan Agner wrote:
> Am 2013-11-27 18:09, schrieb Stephen Warren:
> > On 11/26/2013 04:45 PM, Stefan Agner wrote:
> >> Depending on version, the voltage table might be different. Add version
> >> compatibility to the regulator information in order to select correct
> >> voltage table.
> > 
> >> diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
> > 
> >> -static const unsigned int tps6586x_ldo4_voltages[] = {
> >> +static const unsigned int tps6586x_ldo4_sm2_voltages[] = {
> > 
> >> +static const unsigned int tps658643_sm2_voltages[] = {
> > 
> > What's the logic behind the "ldo4_sm2" v.s. "sm2" naming? Does it match
> > the data sheet in some way? If not, it might be better to name this
> > something like "tps6586x_ldo4_voltages" and "tps65863_ldo4_voltages".
> > 
> 
> This table is used for the TPS6586X (e.g. TPS658621A/D, maybe others)
> variant for the LDO_4 regulator. The same table applies for TPS658623
> for the SM2 regulator as well, so this naming should reflect that fact.
> 
> I could be some more verbose, e.g. tps6586x_ldo4_tps658623_sm2_voltages.
> The other solution would be to create two independent (but identically)
> voltage tables, but takes up more space...

If you only want that name for clarity, perhaps you could just add a
#define instead of duplicating the whole array. That way you'll be able
to reference the same array by a different name (for clarity).

> >> +/* Add version specific entries before any */
> >>  static struct tps6586x_regulator tps6586x_regulator[] = {
> >>  	TPS6586X_SYS_REGULATOR(),
> >> -	TPS6586X_LDO(LDO_0, "vinldo01", ldo0, SUPPLYV1, 5, 3, ENC, 0, END, 0),
> > ...
> >> +	TPS6586X_LDO(TPS6586X_ANY, LDO_0, "vinldo01", tps6586x_ldo0, SUPPLYV1,
> >> +			5, 3, ENC, 0, END, 0),
> > 
> > Rather than changing all the macros and table entries, wouldn't it be
> > much simpler to:
> > 
> > 1) Make tps6586x_regulator[] only contain all the common regulator
> > definitions.
> > 
> > 2) Add new version-specific tables for each version of regulator, so
> > tps6586x_other_regulator[] and tps65863_regulator[].
> > 
> > 3) Have probe() walk multiple tables of regulators, selecting which
> > tables to walk based on version.
> > 
> > That would result in a much smaller and less invasive diff.
> 
> Hm, sounds easier yes. Would also remove the need for correct ordering,
> which is a bit ugly. I will try that, lets see how this works out.

I was going to propose something similar, good that I read the whole
thread at first. My idea would have been to duplicate some of the tables
for simplicity and just have a tps6586x_regulator[] array with all those
that use the "default" set of regulators and separate tables for each
variant. That's obviously suboptimal because it wastes some space, but
it keeps the code simpler, since you'll just have to select a single
array and register that.

What Stephen proposes isn't really all that complex, though, so I'd
prefer that.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] ARM: tegra: set SM2 voltage correct

[Lucas Stach]

Am Mittwoch, den 27.11.2013, 10:13 -0700 schrieb Stephen Warren:
> On 11/26/2013 04:45 PM, Stefan Agner wrote:
> > Set the requested SM2 voltage to the correct value of 1.8V. The value
> > before used to work on TPS658623 since the driver applied a wrong
> > voltage table too. However, the TPS658643 used on newer devices uses
> > yet another voltage table and those broke that compatibility. The
> > regulator driver now has the correct voltage table for both regulator
> > versions and those the correct voltage can be used in this device
> > tree.
> 
> One thing you haven't called out explicitly here is that this series is
> an incompatible change to the DT, since the old buggy driver used to
> allow old buggy DT content to accidentally work.
> 
The current (wrong and potentially dangerous) DT content only worked on
the engineering sample models. So this change definitely does improve
the situation, even with the risk of breaking a small fraction of
working boards.

> I'm not too familiar with who's using mainline on the Colibri boards.
> Hopefully everyone doing that is using in-kernel DTs, so this
> incompatible change won't be any issue for anyone.
> 
I know of some parties using mainline, but I think everyone so far is
able to use DTs bundled with the kernel.

Regards,
Lucas
-- 
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 |

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

[Kai Poggensee]

On 28.11.2013 09:13, Thierry Reding wrote:
> On Wed, Nov 27, 2013 at 12:45:42AM +0100, Stefan Agner wrote:
>> This patchset adds version detection for the tps6586x mfd family. This is
>> required because some regulator versions use different voltage tables. The
>> regulator driver now uses the right voltage table according to the
>> version.
>> 
>> The required voltage for the SM2 converter on the Colibri T20 is 1.8V,
>> however, the device tree states 3.7V right now. Newer versions (>=V1.2) of
>> the Colibri T20 module use the TPS658643, whereas old ones (<V1.2) use the
>> TPS658623. Both of them have a different voltage table for SM2. The
>> current device tree contains a voltage which results in 1.8V for the older
>> TPS658623. Since the regulator driver has correct voltage tables for both
>> regulator versions now, we can request the correct 1.8V from the device
>> tree.
>> 
>> Tested on Colibri T20 V1.1 and V1.2.
>> 
>> Stefan Agner (3):
>>   mfd: tps6586x: add version detection
>>   regulator: tps6586x: add voltage table for tps658643
>>   ARM: tegra: set SM2 voltage correct
>> 
>>  arch/arm/boot/dts/tegra20-colibri-512.dtsi |  4 +-
>>  drivers/mfd/tps6586x.c                     | 41 +++++++++++--
>>  drivers/regulator/tps6586x-regulator.c     | 97 ++++++++++++++++++++----------
>>  include/linux/mfd/tps6586x.h               |  9 +++
>>  4 files changed, 110 insertions(+), 41 deletions(-)
> 
> Adding Kai Poggensee on Cc. There was a variant of the Tamonten board
> with one of these versions of the chip (or maybe it was even yet another
> one).

Thanks Thierry,

we'll be more than happy to support testing once the
re-spun patch is posted. However, we will probably
do so by back-porting to L4T 16.3.

For the records:
We are using TPS658621C on first variant of Tamonten
Tegra 2 COM (internally called 1373-111), we are using
TPS658640 on variant two (1373-121).

This second variant was created to be 100% compatible
with our TamontenNG Tegra 3 COM (basically omitting some
previously unused interfaces), using TPS658640 was an
opportunistic optimization.

Regards,
Kai

Re: [PATCH 3/3] ARM: tegra: set SM2 voltage correct

[Stefan Agner]

Am 2013-11-28 10:49, schrieb Lucas Stach:
> Am Mittwoch, den 27.11.2013, 10:13 -0700 schrieb Stephen Warren:
>> On 11/26/2013 04:45 PM, Stefan Agner wrote:
>> > Set the requested SM2 voltage to the correct value of 1.8V. The value
>> > before used to work on TPS658623 since the driver applied a wrong
>> > voltage table too. However, the TPS658643 used on newer devices uses
>> > yet another voltage table and those broke that compatibility. The
>> > regulator driver now has the correct voltage table for both regulator
>> > versions and those the correct voltage can be used in this device
>> > tree.
>>
>> One thing you haven't called out explicitly here is that this series is
>> an incompatible change to the DT, since the old buggy driver used to
>> allow old buggy DT content to accidentally work.
>>
> The current (wrong and potentially dangerous) DT content only worked on
> the engineering sample models. So this change definitely does improve
> the situation, even with the risk of breaking a small fraction of
> working boards.
On the released modules, the current value ends in a freeze during
boot-up (since the wrong voltage table results in a too low voltage).

When I use 1.8V in the device tree, the old drivers refuses to set any
voltage. The regulators default/bootloader settings work then better
than the wrongfully applied values (tested on a new device). The probe
just fails:

[    0.243864] tps6586x-regulator tps6586x-regulator: failed to register
regulator REG-SM_2
[    0.244317] tps6586x-regulator: probe of tps6586x-regulator failed
with error -22

So, for newer modules, the incompatible 1.8V DT results in a successful
boot. Since we don't have a regulator driver at all, likely other things
are broken at that situation...

--
Stefan