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

[PATCH v2 0/3] 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.

v2
- changed 'static const char *names' to 'static const char * const names'
  as checkpatch.pl still complained about this one,
- updated Documentation and Kconfig on TMP435,
- added missing error check in tmp401_init_client(),
- removed the redundant "Initialization failed" string since we'll get
  'tmp401: probe of X-00XX failed with error -121' anyway in dmesg,

v1:
https://www.mail-archive.com/linux-kernel%40vger.kernel.org/msg781930.html

Bartosz Golaszewski (2):
  hwmon: tmp401: add TMP435 to the list of supported chips in docs and Kconfig
  hwmon: tmp401: bail-out from tmp401_probe() in case of write errors

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

 Documentation/hwmon/tmp401 |  7 +++++--
 drivers/hwmon/Kconfig      |  2 +-
 drivers/hwmon/tmp401.c     | 45 ++++++++++++++++++++++++++++++++-------------
 3 files changed, 38 insertions(+), 16 deletions(-)

-- 
2.1.3

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

[Bartosz Golaszewski]

From: Patrick Titiano <ptitiano@baylibre.com>

Signed-off-by: Patrick Titiano <ptitiano@baylibre.com>
[Bartosz Golaszewski: prepared for submission, code review fixes]
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..a5f0973 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 * const names[] = {
+		"TMP401", "TMP411", "TMP431", "TMP432", "TMP435"
+	};
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct tmp401_data *data;
-- 
2.1.3

[PATCH v2 2/3] hwmon: tmp401: add TMP435 to the list of supported chips in docs and Kconfig

[Bartosz Golaszewski]

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 Documentation/hwmon/tmp401 | 8 ++++++--
 drivers/hwmon/Kconfig      | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/tmp401 b/Documentation/hwmon/tmp401
index f91e3fa..445ff7b 100644
--- a/Documentation/hwmon/tmp401
+++ b/Documentation/hwmon/tmp401
@@ -18,6 +18,10 @@ Supported chips:
     Prefix: 'tmp432'
     Addresses scanned: I2C 0x4c, 0x4d
     Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp432.html
+  * Texas Instruments TMP435
+    Prefix: 'tmp435'
+    Addresses scanned: I2C 0x4c
+    Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp435.html
 
 Authors:
          Hans de Goede <hdegoede@redhat.com>
@@ -27,8 +31,8 @@ Description
 -----------
 
 This driver implements support for Texas Instruments TMP401, TMP411,
-TMP431, and TMP432 chips. These chips implement one or two remote and
-one local temperature sensors. Temperature is measured in degrees
+TMP431, TMP432 and TMP435 chips. These chips implement one or two remote
+and one local temperature sensors. Temperature is measured in degrees
 Celsius. Resolution of the remote sensor is 0.0625 degree. Local
 sensor resolution can be set to 0.5, 0.25, 0.125 or 0.0625 degree (not
 supported by the driver so far, so using the default resolution of 0.5
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index b1ce6a0..6529c09 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1466,7 +1466,7 @@ config SENSORS_TMP401
 	depends on I2C
 	help
 	  If you say yes here you get support for Texas Instruments TMP401,
-	  TMP411, TMP431, and TMP432 temperature sensor chips.
+	  TMP411, TMP431, TMP432 and TMP435 temperature sensor chips.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called tmp401.
-- 
2.1.3

[PATCH v2 3/3] 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 | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
index a5f0973..ce7e579 100644
--- a/drivers/hwmon/tmp401.c
+++ b/drivers/hwmon/tmp401.c
@@ -618,10 +618,10 @@ 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;
+	int config, config_orig, status;
 
 	/* Set the conversion rate to 2 Hz */
 	i2c_smbus_write_byte_data(client, TMP401_CONVERSION_RATE_WRITE, 5);
@@ -629,16 +629,21 @@ 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;
-	}
+	if (config < 0)
+		return config;
 
 	config_orig = config;
 	config &= ~TMP401_CONFIG_SHUTDOWN;
 
-	if (config != config_orig)
-		i2c_smbus_write_byte_data(client, TMP401_CONFIG_WRITE, config);
+	if (config != config_orig) {
+		status = i2c_smbus_write_byte_data(client,
+						   TMP401_CONFIG_WRITE,
+						   config);
+		if (status < 0)
+			return status;
+	}
+
+	return 0;
 }
 
 static int tmp401_detect(struct i2c_client *client,
@@ -721,7 +726,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 +737,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 v2 1/3] hwmon: tmp401: add support for TI TMP435

[Guenter Roeck]

On Thu, Dec 04, 2014 at 04:22:52PM +0100, Bartosz Golaszewski wrote:
> From: Patrick Titiano <ptitiano@baylibre.com>
> 
> Signed-off-by: Patrick Titiano <ptitiano@baylibre.com>
> [Bartosz Golaszewski: prepared for submission, code review fixes]
> 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..a5f0973 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>
> + *

Really ?

Adding a copyright for a couple of device detection lines is excessive
and highly unusual. I'll have to think hard about this.

Guenter

>   * 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 * const names[] = {
> +		"TMP401", "TMP411", "TMP431", "TMP432", "TMP435"
> +	};
>  	struct device *dev = &client->dev;
>  	struct device *hwmon_dev;
>  	struct tmp401_data *data;
> -- 
> 2.1.3
> 

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

[Guenter Roeck]

On Thu, Dec 04, 2014 at 04:22:54PM +0100, 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 | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
> index a5f0973..ce7e579 100644
> --- a/drivers/hwmon/tmp401.c
> +++ b/drivers/hwmon/tmp401.c
> @@ -618,10 +618,10 @@ 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;
> +	int config, config_orig, status;
>  
>  	/* Set the conversion rate to 2 Hz */
>  	i2c_smbus_write_byte_data(client, TMP401_CONVERSION_RATE_WRITE, 5);
> @@ -629,16 +629,21 @@ 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;
> -	}
> +	if (config < 0)
> +		return config;
>  
>  	config_orig = config;
>  	config &= ~TMP401_CONFIG_SHUTDOWN;
>  
> -	if (config != config_orig)
> -		i2c_smbus_write_byte_data(client, TMP401_CONFIG_WRITE, config);
> +	if (config != config_orig) {
> +		status = i2c_smbus_write_byte_data(client,
> +						   TMP401_CONFIG_WRITE,
> +						   config);
> +		if (status < 0)
> +			return status;

Just pre-initialize status to 0 and return status unconditionally.

Thanks,
Guenter