[PATCH 0/2] hwmon: tmp401: support for TMP435 and fix for a probe issue

[PATCH 0/2] hwmon: tmp401: support for TMP435 and fix for a probe issue

[Bartosz Golaszewski]

This series adds support for the TI TMP435 chip and fixes an issue similar
to the one we observed for ina2xx - ignoring the write errors when probing the
device.

Bartosz Golaszewski (1):
  hwmon: tmp401: bail-out from tmp401_probe() in case of write errors

Patrick Titiano (1):
  hwmon: tmp401: add support for TI TMP435

 drivers/hwmon/tmp401.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

-- 
2.1.3

[PATCH 1/2] hwmon: tmp401: add support for TI TMP435

[Bartosz Golaszewski]

From: Patrick Titiano <ptitiano@baylibre.com>

Signed-off-by: Patrick Titiano <ptitiano@baylibre.com>
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/hwmon/tmp401.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
index 7fa6e7d..5a72d7f 100644
--- a/drivers/hwmon/tmp401.c
+++ b/drivers/hwmon/tmp401.c
@@ -8,6 +8,9 @@
  * Cleanup and support for TMP431 and TMP432 by Guenter Roeck
  * Copyright (c) 2013 Guenter Roeck <linux@roeck-us.net>
  *
+ * Support for TMP435 by Patrick Titiano
+ * Copyright (c) 2014 Patrick Titiano <ptitiano@baylibre.com>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -46,7 +49,7 @@
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = { 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
 
-enum chips { tmp401, tmp411, tmp431, tmp432 };
+enum chips { tmp401, tmp411, tmp431, tmp432, tmp435 };
 
 /*
  * The TMP401 registers, note some registers have different addresses for
@@ -136,6 +139,7 @@ static const u8 TMP432_STATUS_REG[] = {
 #define TMP411C_DEVICE_ID			0x10
 #define TMP431_DEVICE_ID			0x31
 #define TMP432_DEVICE_ID			0x32
+#define TMP435_DEVICE_ID			0x35
 
 /*
  * Driver data (common to all clients)
@@ -146,6 +150,7 @@ static const struct i2c_device_id tmp401_id[] = {
 	{ "tmp411", tmp411 },
 	{ "tmp431", tmp431 },
 	{ "tmp432", tmp432 },
+	{ "tmp435", tmp435 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tmp401_id);
@@ -684,6 +689,11 @@ static int tmp401_detect(struct i2c_client *client,
 			return -ENODEV;
 		kind = tmp432;
 		break;
+	case TMP435_DEVICE_ID:
+		if (client->addr != 0x4c)
+			return -ENODEV;
+		kind = tmp435;
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -705,7 +715,9 @@ static int tmp401_detect(struct i2c_client *client,
 static int tmp401_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	const char *names[] = { "TMP401", "TMP411", "TMP431", "TMP432" };
+	static const char *names[] = {
+		"TMP401", "TMP411", "TMP431", "TMP432", "TMP435"
+	};
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct tmp401_data *data;
-- 
2.1.3

[PATCH 2/2] hwmon: tmp401: bail-out from tmp401_probe() in case of write errors

[Bartosz Golaszewski]

The return value of i2c_smbus_read_byte_data() is checked in
tmp401_init_client(), but only a warning is printed and the device is
registered anyway. This leads to devices being registered even if they
cannot be physically detected.

Bail-out from probe in case of write errors and notify the user.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/hwmon/tmp401.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
index 5a72d7f..75b3210 100644
--- a/drivers/hwmon/tmp401.c
+++ b/drivers/hwmon/tmp401.c
@@ -618,8 +618,8 @@ static const struct attribute_group tmp432_group = {
  * Begin non sysfs callback code (aka Real code)
  */
 
-static void tmp401_init_client(struct tmp401_data *data,
-			       struct i2c_client *client)
+static int tmp401_init_client(struct tmp401_data *data,
+			      struct i2c_client *client)
 {
 	int config, config_orig;
 
@@ -630,8 +630,8 @@ static void tmp401_init_client(struct tmp401_data *data,
 	/* Start conversions (disable shutdown if necessary) */
 	config = i2c_smbus_read_byte_data(client, TMP401_CONFIG_READ);
 	if (config < 0) {
-		dev_warn(&client->dev, "Initialization failed!\n");
-		return;
+		dev_err(&client->dev, "Initialization failed!");
+		return config;
 	}
 
 	config_orig = config;
@@ -639,6 +639,8 @@ static void tmp401_init_client(struct tmp401_data *data,
 
 	if (config != config_orig)
 		i2c_smbus_write_byte_data(client, TMP401_CONFIG_WRITE, config);
+
+	return 0;
 }
 
 static int tmp401_detect(struct i2c_client *client,
@@ -721,7 +723,7 @@ static int tmp401_probe(struct i2c_client *client,
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct tmp401_data *data;
-	int groups = 0;
+	int groups = 0, status;
 
 	data = devm_kzalloc(dev, sizeof(struct tmp401_data), GFP_KERNEL);
 	if (!data)
@@ -732,7 +734,9 @@ static int tmp401_probe(struct i2c_client *client,
 	data->kind = id->driver_data;
 
 	/* Initialize the TMP401 chip */
-	tmp401_init_client(data, client);
+	status = tmp401_init_client(data, client);
+	if (status < 0)
+		return status;
 
 	/* Register sysfs hooks */
 	data->groups[groups++] = &tmp401_group;
-- 
2.1.3

Re: [PATCH 1/2] hwmon: tmp401: add support for TI TMP435

[Guenter Roeck]

On 12/04/2014 01:57 AM, Bartosz Golaszewski wrote:
> From: Patrick Titiano <ptitiano@baylibre.com>
>
> Signed-off-by: Patrick Titiano <ptitiano@baylibre.com>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   drivers/hwmon/tmp401.c | 16 ++++++++++++++--

Please also update Kconfig and Documentation/hwmon/tmp401.

>   1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
> index 7fa6e7d..5a72d7f 100644
> --- a/drivers/hwmon/tmp401.c
> +++ b/drivers/hwmon/tmp401.c
> @@ -8,6 +8,9 @@
>    * Cleanup and support for TMP431 and TMP432 by Guenter Roeck
>    * Copyright (c) 2013 Guenter Roeck <linux@roeck-us.net>
>    *
> + * Support for TMP435 by Patrick Titiano
> + * Copyright (c) 2014 Patrick Titiano <ptitiano@baylibre.com>
> + *
>    * This program is free software; you can redistribute it and/or modify
>    * it under the terms of the GNU General Public License as published by
>    * the Free Software Foundation; either version 2 of the License, or
> @@ -46,7 +49,7 @@
>   /* Addresses to scan */
>   static const unsigned short normal_i2c[] = { 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
>
> -enum chips { tmp401, tmp411, tmp431, tmp432 };
> +enum chips { tmp401, tmp411, tmp431, tmp432, tmp435 };
>
>   /*
>    * The TMP401 registers, note some registers have different addresses for
> @@ -136,6 +139,7 @@ static const u8 TMP432_STATUS_REG[] = {
>   #define TMP411C_DEVICE_ID			0x10
>   #define TMP431_DEVICE_ID			0x31
>   #define TMP432_DEVICE_ID			0x32
> +#define TMP435_DEVICE_ID			0x35

The datasheet claims otherwise (0x31, specifically).
Is this based on real chip information ?
Can you send me the output from i2cdump for the chip ?

Thanks,
Guenter

>
>   /*
>    * Driver data (common to all clients)
> @@ -146,6 +150,7 @@ static const struct i2c_device_id tmp401_id[] = {
>   	{ "tmp411", tmp411 },
>   	{ "tmp431", tmp431 },
>   	{ "tmp432", tmp432 },
> +	{ "tmp435", tmp435 },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, tmp401_id);
> @@ -684,6 +689,11 @@ static int tmp401_detect(struct i2c_client *client,
>   			return -ENODEV;
>   		kind = tmp432;
>   		break;
> +	case TMP435_DEVICE_ID:
> +		if (client->addr != 0x4c)
> +			return -ENODEV;
> +		kind = tmp435;
> +		break;
>   	default:
>   		return -ENODEV;
>   	}
> @@ -705,7 +715,9 @@ static int tmp401_detect(struct i2c_client *client,
>   static int tmp401_probe(struct i2c_client *client,
>   			const struct i2c_device_id *id)
>   {
> -	const char *names[] = { "TMP401", "TMP411", "TMP431", "TMP432" };
> +	static const char *names[] = {
> +		"TMP401", "TMP411", "TMP431", "TMP432", "TMP435"
> +	};
>   	struct device *dev = &client->dev;
>   	struct device *hwmon_dev;
>   	struct tmp401_data *data;
>

Re: [PATCH 2/2] hwmon: tmp401: bail-out from tmp401_probe() in case of write errors

[Guenter Roeck]

On 12/04/2014 01:57 AM, Bartosz Golaszewski wrote:
> The return value of i2c_smbus_read_byte_data() is checked in
> tmp401_init_client(), but only a warning is printed and the device is
> registered anyway. This leads to devices being registered even if they
> cannot be physically detected.
>
> Bail-out from probe in case of write errors and notify the user.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   drivers/hwmon/tmp401.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
> index 5a72d7f..75b3210 100644
> --- a/drivers/hwmon/tmp401.c
> +++ b/drivers/hwmon/tmp401.c
> @@ -618,8 +618,8 @@ static const struct attribute_group tmp432_group = {
>    * Begin non sysfs callback code (aka Real code)
>    */
>
> -static void tmp401_init_client(struct tmp401_data *data,
> -			       struct i2c_client *client)
> +static int tmp401_init_client(struct tmp401_data *data,
> +			      struct i2c_client *client)
>   {
>   	int config, config_orig;
>
> @@ -630,8 +630,8 @@ static void tmp401_init_client(struct tmp401_data *data,
>   	/* Start conversions (disable shutdown if necessary) */
>   	config = i2c_smbus_read_byte_data(client, TMP401_CONFIG_READ);
>   	if (config < 0) {
> -		dev_warn(&client->dev, "Initialization failed!\n");
> -		return;
> +		dev_err(&client->dev, "Initialization failed!");
> +		return config;
>   	}
>
>   	config_orig = config;
> @@ -639,6 +639,8 @@ static void tmp401_init_client(struct tmp401_data *data,
>
>   	if (config != config_orig)
>   		i2c_smbus_write_byte_data(client, TMP401_CONFIG_WRITE, config);

No error check and return here ?

Guenter

> +
> +	return 0;
>   }
>
>   static int tmp401_detect(struct i2c_client *client,
> @@ -721,7 +723,7 @@ static int tmp401_probe(struct i2c_client *client,
>   	struct device *dev = &client->dev;
>   	struct device *hwmon_dev;
>   	struct tmp401_data *data;
> -	int groups = 0;
> +	int groups = 0, status;
>
>   	data = devm_kzalloc(dev, sizeof(struct tmp401_data), GFP_KERNEL);
>   	if (!data)
> @@ -732,7 +734,9 @@ static int tmp401_probe(struct i2c_client *client,
>   	data->kind = id->driver_data;
>
>   	/* Initialize the TMP401 chip */
> -	tmp401_init_client(data, client);
> +	status = tmp401_init_client(data, client);
> +	if (status < 0)
> +		return status;
>
>   	/* Register sysfs hooks */
>   	data->groups[groups++] = &tmp401_group;
>

Re: [PATCH 1/2] hwmon: tmp401: add support for TI TMP435

[Bartosz Golaszewski]

2014-12-04 15:19 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> The datasheet claims otherwise (0x31, specifically).
> Is this based on real chip information ?
> Can you send me the output from i2cdump for the chip ?
>

This is what I'm reading:

# i2cdump -y 1 0x4c
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 1a 1b 00 00 05 55 00 55 00 00 05 55 00 55 00 0d    ??..?U.U..?U.U.?
10: 70 00 00 00 00 90 00 00 00 55 1c XX XX XX XX 00    p....?...U?XXXX.
20: 55 0a 70 XX XX 0f XX XX XX XX XX XX XX XX XX XX    U?pXX?XXXXXXXXXX
30: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
40: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
50: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
60: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
70: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
80: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
90: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
a0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
f0: XX XX XX XX XX XX XX XX XX XX XX XX 00 35 55 35    XXXXXXXXXXXX.5U5

# i2cget -y 1 0x4c 0xff b
0x35

Bart

Re: [lm-sensors] [PATCH 1/2] hwmon: tmp401: add support for TI TMP435

[Jean Delvare]

On Thu, 4 Dec 2014 15:33:47 +0100, Bartosz Golaszewski wrote:
> 2014-12-04 15:19 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> > The datasheet claims otherwise (0x31, specifically).
> > Is this based on real chip information ?
> > Can you send me the output from i2cdump for the chip ?
> >
> 
> This is what I'm reading:
> 
> # i2cdump -y 1 0x4c
> No size specified (using byte-data access)
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 1a 1b 00 00 05 55 00 55 00 00 05 55 00 55 00 0d    ??..?U.U..?U.U.?
> 10: 70 00 00 00 00 90 00 00 00 55 1c XX XX XX XX 00    p....?...U?XXXX.
> 20: 55 0a 70 XX XX 0f XX XX XX XX XX XX XX XX XX XX    U?pXX?XXXXXXXXXX
> 30: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 40: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 50: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 60: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 70: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 80: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 90: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> a0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> f0: XX XX XX XX XX XX XX XX XX XX XX XX 00 35 55 35    XXXXXXXXXXXX.5U5
> 
> # i2cget -y 1 0x4c 0xff b
> 0x35

That makes sense for a chip named TMP435. Looks like TI forgot to
update the value when copying the datasheet from TMP431 to TMP435.

Thanks for the dump, by the way :)

-- 
Jean Delvare
SUSE L3 Support

Re: [lm-sensors] [PATCH 1/2] hwmon: tmp401: add support for TI TMP435

[Guenter Roeck]

On Thu, Dec 04, 2014 at 06:32:28PM +0100, Jean Delvare wrote:
> On Thu, 4 Dec 2014 15:33:47 +0100, Bartosz Golaszewski wrote:
> > 2014-12-04 15:19 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> > > The datasheet claims otherwise (0x31, specifically).
> > > Is this based on real chip information ?
> > > Can you send me the output from i2cdump for the chip ?
> > >
> > 
> > This is what I'm reading:
> > 
> > # i2cdump -y 1 0x4c
> > No size specified (using byte-data access)
> >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> > 00: 1a 1b 00 00 05 55 00 55 00 00 05 55 00 55 00 0d    ??..?U.U..?U.U.?
> > 10: 70 00 00 00 00 90 00 00 00 55 1c XX XX XX XX 00    p....?...U?XXXX.
> > 20: 55 0a 70 XX XX 0f XX XX XX XX XX XX XX XX XX XX    U?pXX?XXXXXXXXXX
> > 30: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > 40: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > 50: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > 60: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > 70: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > 80: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > 90: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > a0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> > f0: XX XX XX XX XX XX XX XX XX XX XX XX 00 35 55 35    XXXXXXXXXXXX.5U5
> > 
> > # i2cget -y 1 0x4c 0xff b
> > 0x35
> 
> That makes sense for a chip named TMP435. Looks like TI forgot to
> update the value when copying the datasheet from TMP431 to TMP435.
> 
Guess so.

I'll make the necessary updates to sensors-detect and the web page.

> Thanks for the dump, by the way :)
> 

Same here .. that also helps for writing module test scripts.

Thanks,
Guenter