[PATCH 0/7] iio: mlx90614 enhancements

[PATCH 0/7] iio: mlx90614 enhancements

[Vianney le Clément de Saint-Marcq]

This set of patches adds the following additional features to the
mlx90614 temperature sensor driver.

* Processed temperature output (patch 3)
* Support for the dual IR sensor model (patch 4)
* EEPROM configuration tuning (patch 5)
* Power management (patch 6)
* Raw IR value access (RFC, patch 7)

The changes have been split into 7 patches to ease code review.
Patches 1 and 2 contain mandatory symbol definitions.
Patches 3-7 implement the additional features and are mostly independent
from each other.  If some features are not desired, the corresponding
patches can be left out.

Perhaps the most tricky part is the power management implemented by
patch 6, as it involves an extra GPIO messing with the SDA line in order
to wake up the sensor.  See the commit message for details.

Patch 6 introduces device tree bindings and is the only one sent to the
devicetree list.

Vianney le Clément de Saint-Marcq (7):
  iio: mlx90614: Refactor register symbols
  iio: mlx90614: Add symbols for accessible registers
  iio: mlx90614: Add processed temperature output
  iio: mlx90614: Support devices with dual IR sensor
  iio: mlx90614: Allow tuning EEPROM configuration
  iio: mlx90614: Add power management
  iio: mlx90614: Provide raw IR value for object channels

 .../bindings/iio/temperature/mlx90614.txt          |  24 +
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/iio/temperature/mlx90614.c                 | 574 ++++++++++++++++++++-
 3 files changed, 580 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/mlx90614.txt

-- 
2.3.0

[PATCH 1/7] iio: mlx90614: Refactor register symbols

[Vianney le Clément de Saint-Marcq]

The defined registers only make sense when used for accessing RAM. Make
MLX90614_OP_RAM part of the symbol definition to avoid accidental access
to the wrong register.

Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/iio/temperature/mlx90614.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index c8b6ac8..e2c6f1a 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -23,8 +23,8 @@
 #define MLX90614_OP_RAM 0x00
 
 /* RAM offsets with 16-bit data, MSB first */
-#define MLX90614_TA 0x06 /* ambient temperature */
-#define MLX90614_TOBJ1 0x07 /* object temperature */
+#define MLX90614_TA	(MLX90614_OP_RAM | 0x06) /* ambient temperature */
+#define MLX90614_TOBJ1	(MLX90614_OP_RAM | 0x07) /* object 1 temperature */
 
 struct mlx90614_data {
 	struct i2c_client *client;
@@ -42,13 +42,13 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 		switch (channel->channel2) {
 		case IIO_MOD_TEMP_AMBIENT:
 			ret = i2c_smbus_read_word_data(data->client,
-			    MLX90614_OP_RAM | MLX90614_TA);
+			    MLX90614_TA);
 			if (ret < 0)
 				return ret;
 			break;
 		case IIO_MOD_TEMP_OBJECT:
 			ret = i2c_smbus_read_word_data(data->client,
-			    MLX90614_OP_RAM | MLX90614_TOBJ1);
+			    MLX90614_TOBJ1);
 			if (ret < 0)
 				return ret;
 			break;
-- 
2.3.0

Re: [PATCH 1/7] iio: mlx90614: Refactor register symbols

[Jonathan Cameron]

On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
> The defined registers only make sense when used for accessing RAM. Make
> MLX90614_OP_RAM part of the symbol definition to avoid accidental access
> to the wrong register.
> 
> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Seems an entirely sensible change.

Applied to the togreg branch of iio.git.  Initially pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/temperature/mlx90614.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index c8b6ac8..e2c6f1a 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -23,8 +23,8 @@
>  #define MLX90614_OP_RAM 0x00
>  
>  /* RAM offsets with 16-bit data, MSB first */
> -#define MLX90614_TA 0x06 /* ambient temperature */
> -#define MLX90614_TOBJ1 0x07 /* object temperature */
> +#define MLX90614_TA	(MLX90614_OP_RAM | 0x06) /* ambient temperature */
> +#define MLX90614_TOBJ1	(MLX90614_OP_RAM | 0x07) /* object 1 temperature */
>  
>  struct mlx90614_data {
>  	struct i2c_client *client;
> @@ -42,13 +42,13 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  		switch (channel->channel2) {
>  		case IIO_MOD_TEMP_AMBIENT:
>  			ret = i2c_smbus_read_word_data(data->client,
> -			    MLX90614_OP_RAM | MLX90614_TA);
> +			    MLX90614_TA);
>  			if (ret < 0)
>  				return ret;
>  			break;
>  		case IIO_MOD_TEMP_OBJECT:
>  			ret = i2c_smbus_read_word_data(data->client,
> -			    MLX90614_OP_RAM | MLX90614_TOBJ1);
> +			    MLX90614_TOBJ1);
>  			if (ret < 0)
>  				return ret;
>  			break;
> 

[PATCH 2/7] iio: mlx90614: Add symbols for accessible registers

[Vianney le Clément de Saint-Marcq]

Add symbols for all accessible RAM and EEPROM registers, as well as the
sleep command and timings defined in the datasheet.

Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/iio/temperature/mlx90614.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index e2c6f1a..a2e3aa6 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -20,11 +20,35 @@
 
 #include <linux/iio/iio.h>
 
-#define MLX90614_OP_RAM 0x00
+#define MLX90614_OP_RAM		0x00
+#define MLX90614_OP_EEPROM	0x20
+#define MLX90614_OP_SLEEP	0xff
 
 /* RAM offsets with 16-bit data, MSB first */
+#define MLX90614_RAW1	(MLX90614_OP_RAM | 0x04) /* raw data IR channel 1 */
+#define MLX90614_RAW2	(MLX90614_OP_RAM | 0x05) /* raw data IR channel 2 */
 #define MLX90614_TA	(MLX90614_OP_RAM | 0x06) /* ambient temperature */
 #define MLX90614_TOBJ1	(MLX90614_OP_RAM | 0x07) /* object 1 temperature */
+#define MLX90614_TOBJ2	(MLX90614_OP_RAM | 0x08) /* object 2 temperature */
+
+/* EEPROM offsets with 16-bit data, MSB first */
+#define MLX90614_EMISSIVITY	(MLX90614_OP_EEPROM | 0x04) /* emissivity correction coefficient */
+#define MLX90614_CONFIG		(MLX90614_OP_EEPROM | 0x05) /* configuration register */
+
+/* Control bits in configuration register */
+#define MLX90614_CONFIG_IIR_SHIFT 0 /* IIR coefficient */
+#define MLX90614_CONFIG_IIR_MASK (0x7 << MLX90614_CONFIG_IIR_SHIFT)
+#define MLX90614_CONFIG_DUAL_SHIFT 6 /* single (0) or dual (1) IR sensor */
+#define MLX90614_CONFIG_DUAL_MASK (1 << MLX90614_CONFIG_DUAL_SHIFT)
+#define MLX90614_CONFIG_FIR_SHIFT 8 /* FIR coefficient */
+#define MLX90614_CONFIG_FIR_MASK (0x7 << MLX90614_CONFIG_FIR_SHIFT)
+#define MLX90614_CONFIG_GAIN_SHIFT 11 /* gain */
+#define MLX90614_CONFIG_GAIN_MASK (0x7 << MLX90614_CONFIG_GAIN_SHIFT)
+
+/* Timings (in ms) */
+#define MLX90614_TIMING_EEPROM 20 /* time for EEPROM write/erase to complete */
+#define MLX90614_TIMING_WAKEUP 34 /* time to hold SDA low for wake-up */
+#define MLX90614_TIMING_STARTUP 250 /* time before first data after wake-up */
 
 struct mlx90614_data {
 	struct i2c_client *client;
-- 
2.3.0

[PATCH 3/7] iio: mlx90614: Add processed temperature output

[Vianney le Clément de Saint-Marcq]

Provide processed temperature in milli-Celsius through the
in_temp_*_input interface.

Also correct the formula in the comment as 1/20 = 0.05 instead of 0.02.

Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/iio/temperature/mlx90614.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index a2e3aa6..d5b41fd 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -2,6 +2,7 @@
  * mlx90614.c - Support for Melexis MLX90614 contactless IR temperature sensor
  *
  * Copyright (c) 2014 Peter Meerwald <pmeerw@pmeerw.net>
+ * Copyright (c) 2015 Essensium NV
  *
  * This file is subject to the terms and conditions of version 2 of
  * the GNU General Public License.  See the file COPYING in the main
@@ -62,7 +63,8 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 	s32 ret;
 
 	switch (mask) {
-	case IIO_CHAN_INFO_RAW: /* 0.02K / LSB */
+	case IIO_CHAN_INFO_RAW: /* 0.05K / LSB */
+	case IIO_CHAN_INFO_PROCESSED:
 		switch (channel->channel2) {
 		case IIO_MOD_TEMP_AMBIENT:
 			ret = i2c_smbus_read_word_data(data->client,
@@ -79,7 +81,14 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
-		*val = ret;
+		switch (mask) {
+		case IIO_CHAN_INFO_RAW:
+			*val = ret;
+			break;
+		case IIO_CHAN_INFO_PROCESSED:
+			*val = ret * 20 - 273150;
+			break;
+		}
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OFFSET:
 		*val = 13657;
@@ -98,7 +107,8 @@ static const struct iio_chan_spec mlx90614_channels[] = {
 		.type = IIO_TEMP,
 		.modified = 1,
 		.channel2 = IIO_MOD_TEMP_AMBIENT,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		    BIT(IIO_CHAN_INFO_PROCESSED),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
 		    BIT(IIO_CHAN_INFO_SCALE),
 	},
@@ -106,7 +116,8 @@ static const struct iio_chan_spec mlx90614_channels[] = {
 		.type = IIO_TEMP,
 		.modified = 1,
 		.channel2 = IIO_MOD_TEMP_OBJECT,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		    BIT(IIO_CHAN_INFO_PROCESSED),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
 		    BIT(IIO_CHAN_INFO_SCALE),
 	},
@@ -170,5 +181,6 @@ static struct i2c_driver mlx90614_driver = {
 module_i2c_driver(mlx90614_driver);
 
 MODULE_AUTHOR("Peter Meerwald <pmeerw@pmeerw.net>");
+MODULE_AUTHOR("Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>");
 MODULE_DESCRIPTION("Melexis MLX90614 contactless IR temperature sensor driver");
 MODULE_LICENSE("GPL");
-- 
2.3.0

Re: [PATCH 3/7] iio: mlx90614: Add processed temperature output

[Jonathan Cameron]

On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
> Provide processed temperature in milli-Celsius through the
> in_temp_*_input interface.
Why?  It's trivial for userspace to do the conversion, so
why provide the additional interface?  As a rule we don't provide
both as in general these sensors are wrapped up in a library that can
do the work more efficiently in userspace when needed.

J
> 
> Also correct the formula in the comment as 1/20 = 0.05 instead of 0.02.
> 
> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> ---
>  drivers/iio/temperature/mlx90614.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index a2e3aa6..d5b41fd 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -2,6 +2,7 @@
>   * mlx90614.c - Support for Melexis MLX90614 contactless IR temperature sensor
>   *
>   * Copyright (c) 2014 Peter Meerwald <pmeerw@pmeerw.net>
> + * Copyright (c) 2015 Essensium NV
>   *
>   * This file is subject to the terms and conditions of version 2 of
>   * the GNU General Public License.  See the file COPYING in the main
> @@ -62,7 +63,8 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  	s32 ret;
>  
>  	switch (mask) {
> -	case IIO_CHAN_INFO_RAW: /* 0.02K / LSB */
> +	case IIO_CHAN_INFO_RAW: /* 0.05K / LSB */
> +	case IIO_CHAN_INFO_PROCESSED:
>  		switch (channel->channel2) {
>  		case IIO_MOD_TEMP_AMBIENT:
>  			ret = i2c_smbus_read_word_data(data->client,
> @@ -79,7 +81,14 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> -		*val = ret;
> +		switch (mask) {
> +		case IIO_CHAN_INFO_RAW:
> +			*val = ret;
> +			break;
> +		case IIO_CHAN_INFO_PROCESSED:
> +			*val = ret * 20 - 273150;
> +			break;
> +		}
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_OFFSET:
>  		*val = 13657;
> @@ -98,7 +107,8 @@ static const struct iio_chan_spec mlx90614_channels[] = {
>  		.type = IIO_TEMP,
>  		.modified = 1,
>  		.channel2 = IIO_MOD_TEMP_AMBIENT,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +		    BIT(IIO_CHAN_INFO_PROCESSED),
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
>  		    BIT(IIO_CHAN_INFO_SCALE),
>  	},
> @@ -106,7 +116,8 @@ static const struct iio_chan_spec mlx90614_channels[] = {
>  		.type = IIO_TEMP,
>  		.modified = 1,
>  		.channel2 = IIO_MOD_TEMP_OBJECT,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +		    BIT(IIO_CHAN_INFO_PROCESSED),
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
>  		    BIT(IIO_CHAN_INFO_SCALE),
>  	},
> @@ -170,5 +181,6 @@ static struct i2c_driver mlx90614_driver = {
>  module_i2c_driver(mlx90614_driver);
>  
>  MODULE_AUTHOR("Peter Meerwald <pmeerw@pmeerw.net>");
> +MODULE_AUTHOR("Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>");
>  MODULE_DESCRIPTION("Melexis MLX90614 contactless IR temperature sensor driver");
>  MODULE_LICENSE("GPL");
> 

Re: [PATCH 3/7] iio: mlx90614: Add processed temperature output

[Vianney le Clément]

On Mon, Mar 9, 2015 at 4:08 , Jonathan Cameron <jic23@kernel.org> wrote:
> On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
>>  Provide processed temperature in milli-Celsius through the
>>  in_temp_*_input interface.
> Why?  It's trivial for userspace to do the conversion, so
> why provide the additional interface?  As a rule we don't provide
> both as in general these sensors are wrapped up in a library that can
> do the work more efficiently in userspace when needed.

Many thanks for reviewing this series, Jonathan.

I was not aware of this rule.  I will remove this patch from v2 (except 
for the tiny comment fix).

Vianney

[PATCH 4/7] iio: mlx90614: Support devices with dual IR sensor

[Vianney le Clément de Saint-Marcq]

The model is detected by reading the EEPROM configuration during
probing.  In the dual IR sensor model, the ambient temperature is
exported twice to userspace.  This makes the two channels appear
completely symmetrical.

Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/iio/temperature/mlx90614.c | 78 +++++++++++++++++++++++++++++++++-----
 1 file changed, 68 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index d5b41fd..0b36746 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -60,6 +60,7 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 			    int *val2, long mask)
 {
 	struct mlx90614_data *data = iio_priv(indio_dev);
+	u8 cmd;
 	s32 ret;
 
 	switch (mask) {
@@ -67,20 +68,28 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (channel->channel2) {
 		case IIO_MOD_TEMP_AMBIENT:
-			ret = i2c_smbus_read_word_data(data->client,
-			    MLX90614_TA);
-			if (ret < 0)
-				return ret;
+			cmd = MLX90614_TA;
 			break;
 		case IIO_MOD_TEMP_OBJECT:
-			ret = i2c_smbus_read_word_data(data->client,
-			    MLX90614_TOBJ1);
-			if (ret < 0)
-				return ret;
+			switch (channel->channel) {
+			case 0:
+				cmd = MLX90614_TOBJ1;
+				break;
+			case 1:
+				cmd = MLX90614_TOBJ2;
+				break;
+			default:
+				return -EINVAL;
+			}
 			break;
 		default:
 			return -EINVAL;
 		}
+
+		ret = i2c_smbus_read_word_data(data->client, cmd);
+		if (ret < 0)
+			return ret;
+
 		switch (mask) {
 		case IIO_CHAN_INFO_RAW:
 			*val = ret;
@@ -121,6 +130,28 @@ static const struct iio_chan_spec mlx90614_channels[] = {
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
 		    BIT(IIO_CHAN_INFO_SCALE),
 	},
+	{
+		.type = IIO_TEMP,
+		.indexed = 1,
+		.modified = 1,
+		.channel = 1,
+		.channel2 = IIO_MOD_TEMP_AMBIENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		    BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
+		    BIT(IIO_CHAN_INFO_SCALE),
+	},
+	{
+		.type = IIO_TEMP,
+		.indexed = 1,
+		.modified = 1,
+		.channel = 1,
+		.channel2 = IIO_MOD_TEMP_OBJECT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		    BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
+		    BIT(IIO_CHAN_INFO_SCALE),
+	},
 };
 
 static const struct iio_info mlx90614_info = {
@@ -128,11 +159,25 @@ static const struct iio_info mlx90614_info = {
 	.driver_module = THIS_MODULE,
 };
 
+/* Return 0 for single sensor, 1 for dual sensor, <0 on error. */
+static int mlx90614_probe_model(struct i2c_client *client)
+{
+	s32 ret;
+
+	ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG);
+
+	if (ret < 0)
+		return ret;
+
+	return (ret & MLX90614_CONFIG_DUAL_MASK) ? 1 : 0;
+}
+
 static int mlx90614_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct iio_dev *indio_dev;
 	struct mlx90614_data *data;
+	int ret;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -ENODEV;
@@ -150,8 +195,21 @@ static int mlx90614_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &mlx90614_info;
 
-	indio_dev->channels = mlx90614_channels;
-	indio_dev->num_channels = ARRAY_SIZE(mlx90614_channels);
+	ret = mlx90614_probe_model(client);
+	switch (ret) {
+	case 0:
+		dev_dbg(&client->dev, "Found single sensor");
+		indio_dev->channels = mlx90614_channels;
+		indio_dev->num_channels = 2;
+		break;
+	case 1:
+		dev_dbg(&client->dev, "Found dual sensor");
+		indio_dev->channels = mlx90614_channels;
+		indio_dev->num_channels = 4;
+		break;
+	default:
+		return ret;
+	}
 
 	return iio_device_register(indio_dev);
 }
-- 
2.3.0

Re: [PATCH 4/7] iio: mlx90614: Support devices with dual IR sensor

[Jonathan Cameron]

On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
> The model is detected by reading the EEPROM configuration during
> probing.  In the dual IR sensor model, the ambient temperature is
> exported twice to userspace.  This makes the two channels appear
> completely symmetrical.
Why do we want it to be symmetrical?  

Is there a part number difference for the dual sensor version?  If so we can still
autoprobe for it, but want to make sure we have it listed as a supported part.

Otherwise one small comment inline.  Ideally I'd like Peter to take a look at this
series as well as he is a lot more familiar with the driver than I am.

Peter, shout if you are snowed under and don't have time!

Jonathan
> 
> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> ---
>  drivers/iio/temperature/mlx90614.c | 78 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 68 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index d5b41fd..0b36746 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -60,6 +60,7 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  			    int *val2, long mask)
>  {
>  	struct mlx90614_data *data = iio_priv(indio_dev);
> +	u8 cmd;
>  	s32 ret;
>  
>  	switch (mask) {
> @@ -67,20 +68,28 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_PROCESSED:
>  		switch (channel->channel2) {
>  		case IIO_MOD_TEMP_AMBIENT:
> -			ret = i2c_smbus_read_word_data(data->client,
> -			    MLX90614_TA);
> -			if (ret < 0)
> -				return ret;
> +			cmd = MLX90614_TA;
>  			break;
>  		case IIO_MOD_TEMP_OBJECT:
> -			ret = i2c_smbus_read_word_data(data->client,
> -			    MLX90614_TOBJ1);
> -			if (ret < 0)
> -				return ret;
> +			switch (channel->channel) {
> +			case 0:
> +				cmd = MLX90614_TOBJ1;
> +				break;
> +			case 1:
> +				cmd = MLX90614_TOBJ2;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
>  			break;
>  		default:
>  			return -EINVAL;
>  		}
> +
> +		ret = i2c_smbus_read_word_data(data->client, cmd);
> +		if (ret < 0)
> +			return ret;
> +
>  		switch (mask) {
>  		case IIO_CHAN_INFO_RAW:
>  			*val = ret;
> @@ -121,6 +130,28 @@ static const struct iio_chan_spec mlx90614_channels[] = {
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
>  		    BIT(IIO_CHAN_INFO_SCALE),
>  	},
> +	{
> +		.type = IIO_TEMP,
> +		.indexed = 1,
> +		.modified = 1,
> +		.channel = 1,
> +		.channel2 = IIO_MOD_TEMP_AMBIENT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +		    BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
> +		    BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +	{
> +		.type = IIO_TEMP,
> +		.indexed = 1,
> +		.modified = 1,
> +		.channel = 1,
> +		.channel2 = IIO_MOD_TEMP_OBJECT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +		    BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
> +		    BIT(IIO_CHAN_INFO_SCALE),
> +	},
>  };
>  
>  static const struct iio_info mlx90614_info = {
> @@ -128,11 +159,25 @@ static const struct iio_info mlx90614_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +/* Return 0 for single sensor, 1 for dual sensor, <0 on error. */
> +static int mlx90614_probe_model(struct i2c_client *client)
I'd give this a more specific name.  probe_num_ir_sensors or similar.

> +{
> +	s32 ret;
> +
> +	ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return (ret & MLX90614_CONFIG_DUAL_MASK) ? 1 : 0;
> +}
> +
>  static int mlx90614_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
>  	struct iio_dev *indio_dev;
>  	struct mlx90614_data *data;
> +	int ret;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
>  		return -ENODEV;
> @@ -150,8 +195,21 @@ static int mlx90614_probe(struct i2c_client *client,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &mlx90614_info;
>  
> -	indio_dev->channels = mlx90614_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(mlx90614_channels);
> +	ret = mlx90614_probe_model(client);
> +	switch (ret) {
> +	case 0:
> +		dev_dbg(&client->dev, "Found single sensor");
> +		indio_dev->channels = mlx90614_channels;
> +		indio_dev->num_channels = 2;
> +		break;
> +	case 1:
> +		dev_dbg(&client->dev, "Found dual sensor");
> +		indio_dev->channels = mlx90614_channels;
> +		indio_dev->num_channels = 4;
> +		break;
> +	default:
> +		return ret;
> +	}
>  
>  	return iio_device_register(indio_dev);
>  }
> 

Re: [PATCH 4/7] iio: mlx90614: Support devices with dual IR sensor

[Vianney le Clément]

On Mon, Mar 9, 2015 at 4:18 , Jonathan Cameron <jic23@kernel.org> wrote:
> On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
>>  The model is detected by reading the EEPROM configuration during
>>  probing.  In the dual IR sensor model, the ambient temperature is
>>  exported twice to userspace.  This makes the two channels appear
>>  completely symmetrical.
> Why do we want it to be symmetrical?

Well, except for the beauty of symmetry, I do not have any hard 
argument, nor do I have any strong feelings about it :).  I can leave 
the duplicate out in v2.

> Is there a part number difference for the dual sensor version?  If so 
> we can still
> autoprobe for it, but want to make sure we have it listed as a 
> supported part.

Nope.  The only difference lies in the option code: 
MLX90614ESF-BAA-000-TU is single-zone, while MLX90614ESF-BBA-000-TU is 
dual (notice the second letter in the second group).

>>  +/* Return 0 for single sensor, 1 for dual sensor, <0 on error. */
>>  +static int mlx90614_probe_model(struct i2c_client *client)
> I'd give this a more specific name.  probe_num_ir_sensors or similar.

Will be changed in v2.

Vianney

[PATCH 5/7] iio: mlx90614: Allow tuning EEPROM configuration

[Vianney le Clément de Saint-Marcq]

Add device attributes for getting/setting emissivity, IIR, and FIR
coefficients, and getting the gain (which should not be modified in
order to keep factory calibration).

The attributes provide raw values whose meaning is described in the
datasheet [1].

Writing to EEPROM requires an explicit erase by writing zero.  In
addition, it takes 20ms for the erase/write to complete.  During this
time no EEPROM register should be accessed.  Therefore, two msleep()s
are added to the write function and a mutex protects against concurrent
access.

Since it is not expected to be updated frequently, the configuration
register is read before modifying it rather than caching it.

[1] http://melexis.com/Assets/IR-sensor-thermometer-MLX90614-Datasheet-5152.aspx

Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/iio/temperature/mlx90614.c | 164 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 163 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index 0b36746..ab98fb6 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -12,14 +12,16 @@
  *
  * (7-bit I2C slave address 0x5a, 100KHz bus speed only!)
  *
- * TODO: sleep mode, configuration EEPROM
+ * TODO: sleep mode
  */
 
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/delay.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
 
 #define MLX90614_OP_RAM		0x00
 #define MLX90614_OP_EEPROM	0x20
@@ -53,8 +55,47 @@
 
 struct mlx90614_data {
 	struct i2c_client *client;
+	struct mutex lock; /* for EEPROM access only */
 };
 
+/*
+ * Erase an address and write word.
+ * The mutex must be locked before calling.
+ */
+static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
+			       u16 value)
+{
+	/*
+	 * Note: The mlx90614 requires a PEC on writing but does not send us a
+	 * valid PEC on reading.  Hence, we cannot set I2C_CLIENT_PEC in
+	 * i2c_client.flags.  As a workaround, we use i2c_smbus_xfer here.
+	 */
+	union i2c_smbus_data data;
+	s32 ret;
+
+	dev_dbg(&client->dev, "Writing 0x%x to address 0x%x", value, command);
+
+	data.word = 0x0000; /* erase command */
+	ret = i2c_smbus_xfer(client->adapter, client->addr,
+			     client->flags | I2C_CLIENT_PEC,
+			     I2C_SMBUS_WRITE, command,
+			     I2C_SMBUS_WORD_DATA, &data);
+	if (ret < 0)
+		return ret;
+
+	msleep(MLX90614_TIMING_EEPROM);
+
+	data.word = value; /* actual write */
+	ret = i2c_smbus_xfer(client->adapter, client->addr,
+			     client->flags | I2C_CLIENT_PEC,
+			     I2C_SMBUS_WRITE, command,
+			     I2C_SMBUS_WORD_DATA, &data);
+
+	msleep(MLX90614_TIMING_EEPROM);
+
+	return ret;
+}
+
 static int mlx90614_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *channel, int *val,
 			    int *val2, long mask)
@@ -111,6 +152,102 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static ssize_t mlx90614_show_emissivity(struct device *dev,
+					struct device_attribute *devattr,
+					char *buf)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mlx90614_data *data = iio_priv(indio_dev);
+	s32 ret;
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_read_word_data(data->client, MLX90614_EMISSIVITY);
+	mutex_unlock(&data->lock);
+
+	if (ret < 0)
+		return ret;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+static ssize_t mlx90614_store_emissivity(struct device *dev,
+					 struct device_attribute *devattr,
+					 const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mlx90614_data *data = iio_priv(indio_dev);
+	u16 val;
+	s32 ret;
+
+	if (kstrtou16(buf, 0, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->lock);
+	ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY, val);
+	mutex_unlock(&data->lock);
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static ssize_t mlx90614_show_config(struct device *dev,
+				    struct device_attribute *devattr,
+				    char *buf)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mlx90614_data *data = iio_priv(indio_dev);
+	struct iio_dev_attr *indio_attr = to_iio_dev_attr(devattr);
+	u16 mask = indio_attr->address & 0xffff;
+	u16 shift = indio_attr->address >> 16;
+	s32 ret;
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
+	mutex_unlock(&data->lock);
+
+	if (ret < 0)
+		return ret;
+
+	ret = (ret & mask) >> shift;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+static ssize_t mlx90614_store_config(struct device *dev,
+				     struct device_attribute *devattr,
+				     const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mlx90614_data *data = iio_priv(indio_dev);
+	struct iio_dev_attr *indio_attr = to_iio_dev_attr(devattr);
+	u16 mask = indio_attr->address & 0xffff;
+	u16 shift = indio_attr->address >> 16;
+	u16 val;
+	s32 ret;
+
+	if (kstrtou16(buf, 0, &val) || val != (val & (mask >> shift)))
+		return -EINVAL;
+
+	mutex_lock(&data->lock);
+
+	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
+	if (ret < 0)
+		goto out;
+
+	val = (val << shift) | ((u16)ret & ~mask);
+	ret = mlx90614_write_word(data->client, MLX90614_CONFIG, val);
+
+out:
+	mutex_unlock(&data->lock);
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
 static const struct iio_chan_spec mlx90614_channels[] = {
 	{
 		.type = IIO_TEMP,
@@ -154,7 +291,31 @@ static const struct iio_chan_spec mlx90614_channels[] = {
 	},
 };
 
+static IIO_DEVICE_ATTR(emissivity, S_IRUGO | S_IWUSR, mlx90614_show_emissivity,
+		mlx90614_store_emissivity, -1);
+static IIO_DEVICE_ATTR(iir, S_IRUGO | S_IWUSR,
+		mlx90614_show_config, mlx90614_store_config,
+		(MLX90614_CONFIG_IIR_SHIFT << 16) | MLX90614_CONFIG_IIR_MASK);
+static IIO_DEVICE_ATTR(fir, S_IRUGO | S_IWUSR,
+		mlx90614_show_config, mlx90614_store_config,
+		(MLX90614_CONFIG_FIR_SHIFT << 16) | MLX90614_CONFIG_FIR_MASK);
+static IIO_DEVICE_ATTR(gain, S_IRUGO, mlx90614_show_config, NULL,
+		(MLX90614_CONFIG_GAIN_SHIFT << 16) | MLX90614_CONFIG_GAIN_MASK);
+
+static struct attribute *mlx90614_attr[] = {
+	&iio_dev_attr_emissivity.dev_attr.attr,
+	&iio_dev_attr_iir.dev_attr.attr,
+	&iio_dev_attr_fir.dev_attr.attr,
+	&iio_dev_attr_gain.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group mlx90614_attr_group = {
+	.attrs = mlx90614_attr,
+};
+
 static const struct iio_info mlx90614_info = {
+	.attrs = &mlx90614_attr_group,
 	.read_raw = mlx90614_read_raw,
 	.driver_module = THIS_MODULE,
 };
@@ -189,6 +350,7 @@ static int mlx90614_probe(struct i2c_client *client,
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
+	mutex_init(&data->lock);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = id->name;
-- 
2.3.0

Re: [PATCH 5/7] iio: mlx90614: Allow tuning EEPROM configuration

[Jonathan Cameron]

On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
> Add device attributes for getting/setting emissivity, IIR, and FIR
> coefficients, and getting the gain (which should not be modified in
> order to keep factory calibration).
> 
> The attributes provide raw values whose meaning is described in the
> datasheet [1].
> 
> Writing to EEPROM requires an explicit erase by writing zero.  In
> addition, it takes 20ms for the erase/write to complete.  During this
> time no EEPROM register should be accessed.  Therefore, two msleep()s
> are added to the write function and a mutex protects against concurrent
> access.
> 
> Since it is not expected to be updated frequently, the configuration
> register is read before modifying it rather than caching it.
> 
> [1] http://melexis.com/Assets/IR-sensor-thermometer-MLX90614-Datasheet-5152.aspx
> 
> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Wolfram, bit of odd i2c usage inline I'd like you to take quick look at.
> ---
>  drivers/iio/temperature/mlx90614.c | 164 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 163 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index 0b36746..ab98fb6 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -12,14 +12,16 @@
>   *
>   * (7-bit I2C slave address 0x5a, 100KHz bus speed only!)
>   *
> - * TODO: sleep mode, configuration EEPROM
> + * TODO: sleep mode
>   */
>  
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
> +#include <linux/delay.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
>  
>  #define MLX90614_OP_RAM		0x00
>  #define MLX90614_OP_EEPROM	0x20
> @@ -53,8 +55,47 @@
>  
>  struct mlx90614_data {
>  	struct i2c_client *client;
> +	struct mutex lock; /* for EEPROM access only */
>  };
>  
> +/*
> + * Erase an address and write word.
> + * The mutex must be locked before calling.
> + */
> +static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
> +			       u16 value)
> +{
> +	/*
> +	 * Note: The mlx90614 requires a PEC on writing but does not send us a
> +	 * valid PEC on reading.  Hence, we cannot set I2C_CLIENT_PEC in
> +	 * i2c_client.flags.  As a workaround, we use i2c_smbus_xfer here.
> +	 */
That's particularly hideous and someone should shout at whoever implemented that!
Ah well, such is life.  I want a go ahead on doing this from Wolfram however.
It's the sort of thing that might well get randomly broken in the future.
Alternative is to add another flag that indicates to the i2c core that the PEC
that comes back will be wrong.
 
> +	union i2c_smbus_data data;
> +	s32 ret;
> +
> +	dev_dbg(&client->dev, "Writing 0x%x to address 0x%x", value, command);
> +
> +	data.word = 0x0000; /* erase command */
> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
> +			     client->flags | I2C_CLIENT_PEC,
> +			     I2C_SMBUS_WRITE, command,
> +			     I2C_SMBUS_WORD_DATA, &data);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(MLX90614_TIMING_EEPROM);
> +
> +	data.word = value; /* actual write */
> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
> +			     client->flags | I2C_CLIENT_PEC,
> +			     I2C_SMBUS_WRITE, command,
> +			     I2C_SMBUS_WORD_DATA, &data);
> +
> +	msleep(MLX90614_TIMING_EEPROM);
> +
> +	return ret;
> +}
> +
>  static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *channel, int *val,
>  			    int *val2, long mask)
> @@ -111,6 +152,102 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static ssize_t mlx90614_show_emissivity(struct device *dev,
> +					struct device_attribute *devattr,
> +					char *buf)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +	s32 ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_read_word_data(data->client, MLX90614_EMISSIVITY);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
> +}
> +
> +static ssize_t mlx90614_store_emissivity(struct device *dev,
> +					 struct device_attribute *devattr,
> +					 const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +	u16 val;
> +	s32 ret;
> +
> +	if (kstrtou16(buf, 0, &val))
> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +	ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY, val);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
Emissivity is well defined. I'd like this to be a standard attribute with
full documentation please rather than a driver specific one.  Happy
to have this added to the infomask elements if it make sense.
> +
> +static ssize_t mlx90614_show_config(struct device *dev,
> +				    struct device_attribute *devattr,
> +				    char *buf)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +	struct iio_dev_attr *indio_attr = to_iio_dev_attr(devattr);
> +	u16 mask = indio_attr->address & 0xffff;
> +	u16 shift = indio_attr->address >> 16;
> +	s32 ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = (ret & mask) >> shift;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
> +}
> +
> +static ssize_t mlx90614_store_config(struct device *dev,
> +				     struct device_attribute *devattr,
> +				     const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +	struct iio_dev_attr *indio_attr = to_iio_dev_attr(devattr);
> +	u16 mask = indio_attr->address & 0xffff;
> +	u16 shift = indio_attr->address >> 16;
> +	u16 val;
> +	s32 ret;
> +
> +	if (kstrtou16(buf, 0, &val) || val != (val & (mask >> shift)))
> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
> +	if (ret < 0)
> +		goto out;
> +
> +	val = (val << shift) | ((u16)ret & ~mask);
> +	ret = mlx90614_write_word(data->client, MLX90614_CONFIG, val);
> +
> +out:
> +	mutex_unlock(&data->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
>  static const struct iio_chan_spec mlx90614_channels[] = {
>  	{
>  		.type = IIO_TEMP,
> @@ -154,7 +291,31 @@ static const struct iio_chan_spec mlx90614_channels[] = {
>  	},
>  };
>  
> +static IIO_DEVICE_ATTR(emissivity, S_IRUGO | S_IWUSR, mlx90614_show_emissivity,
> +		mlx90614_store_emissivity, -1);
> +static IIO_DEVICE_ATTR(iir, S_IRUGO | S_IWUSR,
> +		mlx90614_show_config, mlx90614_store_config,
> +		(MLX90614_CONFIG_IIR_SHIFT << 16) | MLX90614_CONFIG_IIR_MASK);
> +static IIO_DEVICE_ATTR(fir, S_IRUGO | S_IWUSR,
> +		mlx90614_show_config, mlx90614_store_config,
> +		(MLX90614_CONFIG_FIR_SHIFT << 16) | MLX90614_CONFIG_FIR_MASK);
> +static IIO_DEVICE_ATTR(gain, S_IRUGO, mlx90614_show_config, NULL,
> +		(MLX90614_CONFIG_GAIN_SHIFT << 16) | MLX90614_CONFIG_GAIN_MASK);
> +
> +static struct attribute *mlx90614_attr[] = {
> +	&iio_dev_attr_emissivity.dev_attr.attr,
> +	&iio_dev_attr_iir.dev_attr.attr,
Hmm odd to have two different filter types with separate control.
However we have _filter_low_pass_3db_frequency and whilst it is a pain
obviously to map this to that form that's the way to go from a useful
userspace interface point of view.  If we need to extend the filter
description interface (filter type has been suggested before) then please
propose that.

Here I see they are suggesting one filter is effectively for ignoreing
fast passing objects, and the other for noise prevention.  They both
effect the settling time.

Anyhow it's not entirely obvious what the right answer is, but its
definitely not a driver specific interface such as we have.

Peter, any thoughts?
> +	&iio_dev_attr_fir.dev_attr.attr,
> +	&iio_dev_attr_gain.dev_attr.attr,
Please use the standard IIO infomask elements for scale (or perhaps
calibscale - I haven't checked which is appropriate).
> +	NULL
> +};
> +
> +static struct attribute_group mlx90614_attr_group = {
> +	.attrs = mlx90614_attr,
> +};
> +
>  static const struct iio_info mlx90614_info = {
> +	.attrs = &mlx90614_attr_group,
>  	.read_raw = mlx90614_read_raw,
>  	.driver_module = THIS_MODULE,
>  };
> @@ -189,6 +350,7 @@ static int mlx90614_probe(struct i2c_client *client,
>  	data = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
> +	mutex_init(&data->lock);
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->name = id->name;
> 

Re: [PATCH 5/7] iio: mlx90614: Allow tuning EEPROM configuration

[Jonathan Cameron]

On 09/03/15 15:35, Jonathan Cameron wrote:
> On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
>> Add device attributes for getting/setting emissivity, IIR, and FIR
>> coefficients, and getting the gain (which should not be modified in
>> order to keep factory calibration).
>>
>> The attributes provide raw values whose meaning is described in the
>> datasheet [1].
>>
>> Writing to EEPROM requires an explicit erase by writing zero.  In
>> addition, it takes 20ms for the erase/write to complete.  During this
>> time no EEPROM register should be accessed.  Therefore, two msleep()s
>> are added to the write function and a mutex protects against concurrent
>> access.
>>
>> Since it is not expected to be updated frequently, the configuration
>> register is read before modifying it rather than caching it.
>>
>> [1] http://melexis.com/Assets/IR-sensor-thermometer-MLX90614-Datasheet-5152.aspx
>>
>> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
>> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> Wolfram, bit of odd i2c usage inline I'd like you to take quick look at.
Sorry, this would work better if I cleared Wolfram's old email addresses out of
my address book at somepoint!

>> ---
>>  drivers/iio/temperature/mlx90614.c | 164 ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 163 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
>> index 0b36746..ab98fb6 100644
>> --- a/drivers/iio/temperature/mlx90614.c
>> +++ b/drivers/iio/temperature/mlx90614.c
>> @@ -12,14 +12,16 @@
>>   *
>>   * (7-bit I2C slave address 0x5a, 100KHz bus speed only!)
>>   *
>> - * TODO: sleep mode, configuration EEPROM
>> + * TODO: sleep mode
>>   */
>>  
>>  #include <linux/err.h>
>>  #include <linux/i2c.h>
>>  #include <linux/module.h>
>> +#include <linux/delay.h>
>>  
>>  #include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>>  
>>  #define MLX90614_OP_RAM		0x00
>>  #define MLX90614_OP_EEPROM	0x20
>> @@ -53,8 +55,47 @@
>>  
>>  struct mlx90614_data {
>>  	struct i2c_client *client;
>> +	struct mutex lock; /* for EEPROM access only */
>>  };
>>  
>> +/*
>> + * Erase an address and write word.
>> + * The mutex must be locked before calling.
>> + */
>> +static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
>> +			       u16 value)
>> +{
>> +	/*
>> +	 * Note: The mlx90614 requires a PEC on writing but does not send us a
>> +	 * valid PEC on reading.  Hence, we cannot set I2C_CLIENT_PEC in
>> +	 * i2c_client.flags.  As a workaround, we use i2c_smbus_xfer here.
>> +	 */
> That's particularly hideous and someone should shout at whoever implemented that!
> Ah well, such is life.  I want a go ahead on doing this from Wolfram however.
> It's the sort of thing that might well get randomly broken in the future.
> Alternative is to add another flag that indicates to the i2c core that the PEC
> that comes back will be wrong.
>  
>> +	union i2c_smbus_data data;
>> +	s32 ret;
>> +
>> +	dev_dbg(&client->dev, "Writing 0x%x to address 0x%x", value, command);
>> +
>> +	data.word = 0x0000; /* erase command */
>> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
>> +			     client->flags | I2C_CLIENT_PEC,
>> +			     I2C_SMBUS_WRITE, command,
>> +			     I2C_SMBUS_WORD_DATA, &data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	msleep(MLX90614_TIMING_EEPROM);
>> +
>> +	data.word = value; /* actual write */
>> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
>> +			     client->flags | I2C_CLIENT_PEC,
>> +			     I2C_SMBUS_WRITE, command,
>> +			     I2C_SMBUS_WORD_DATA, &data);
>> +
>> +	msleep(MLX90614_TIMING_EEPROM);
>> +
>> +	return ret;
>> +}
>> +
>>  static int mlx90614_read_raw(struct iio_dev *indio_dev,
>>  			    struct iio_chan_spec const *channel, int *val,
>>  			    int *val2, long mask)
>> @@ -111,6 +152,102 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>>  	}
>>  }
>>  
>> +static ssize_t mlx90614_show_emissivity(struct device *dev,
>> +					struct device_attribute *devattr,
>> +					char *buf)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct mlx90614_data *data = iio_priv(indio_dev);
>> +	s32 ret;
>> +
>> +	mutex_lock(&data->lock);
>> +	ret = i2c_smbus_read_word_data(data->client, MLX90614_EMISSIVITY);
>> +	mutex_unlock(&data->lock);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
>> +}
>> +
>> +static ssize_t mlx90614_store_emissivity(struct device *dev,
>> +					 struct device_attribute *devattr,
>> +					 const char *buf, size_t count)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct mlx90614_data *data = iio_priv(indio_dev);
>> +	u16 val;
>> +	s32 ret;
>> +
>> +	if (kstrtou16(buf, 0, &val))
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&data->lock);
>> +	ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY, val);
>> +	mutex_unlock(&data->lock);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return count;
>> +}
> Emissivity is well defined. I'd like this to be a standard attribute with
> full documentation please rather than a driver specific one.  Happy
> to have this added to the infomask elements if it make sense.
>> +
>> +static ssize_t mlx90614_show_config(struct device *dev,
>> +				    struct device_attribute *devattr,
>> +				    char *buf)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct mlx90614_data *data = iio_priv(indio_dev);
>> +	struct iio_dev_attr *indio_attr = to_iio_dev_attr(devattr);
>> +	u16 mask = indio_attr->address & 0xffff;
>> +	u16 shift = indio_attr->address >> 16;
>> +	s32 ret;
>> +
>> +	mutex_lock(&data->lock);
>> +	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
>> +	mutex_unlock(&data->lock);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = (ret & mask) >> shift;
>> +
>> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
>> +}
>> +
>> +static ssize_t mlx90614_store_config(struct device *dev,
>> +				     struct device_attribute *devattr,
>> +				     const char *buf, size_t count)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct mlx90614_data *data = iio_priv(indio_dev);
>> +	struct iio_dev_attr *indio_attr = to_iio_dev_attr(devattr);
>> +	u16 mask = indio_attr->address & 0xffff;
>> +	u16 shift = indio_attr->address >> 16;
>> +	u16 val;
>> +	s32 ret;
>> +
>> +	if (kstrtou16(buf, 0, &val) || val != (val & (mask >> shift)))
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&data->lock);
>> +
>> +	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	val = (val << shift) | ((u16)ret & ~mask);
>> +	ret = mlx90614_write_word(data->client, MLX90614_CONFIG, val);
>> +
>> +out:
>> +	mutex_unlock(&data->lock);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
>>  static const struct iio_chan_spec mlx90614_channels[] = {
>>  	{
>>  		.type = IIO_TEMP,
>> @@ -154,7 +291,31 @@ static const struct iio_chan_spec mlx90614_channels[] = {
>>  	},
>>  };
>>  
>> +static IIO_DEVICE_ATTR(emissivity, S_IRUGO | S_IWUSR, mlx90614_show_emissivity,
>> +		mlx90614_store_emissivity, -1);
>> +static IIO_DEVICE_ATTR(iir, S_IRUGO | S_IWUSR,
>> +		mlx90614_show_config, mlx90614_store_config,
>> +		(MLX90614_CONFIG_IIR_SHIFT << 16) | MLX90614_CONFIG_IIR_MASK);
>> +static IIO_DEVICE_ATTR(fir, S_IRUGO | S_IWUSR,
>> +		mlx90614_show_config, mlx90614_store_config,
>> +		(MLX90614_CONFIG_FIR_SHIFT << 16) | MLX90614_CONFIG_FIR_MASK);
>> +static IIO_DEVICE_ATTR(gain, S_IRUGO, mlx90614_show_config, NULL,
>> +		(MLX90614_CONFIG_GAIN_SHIFT << 16) | MLX90614_CONFIG_GAIN_MASK);
>> +
>> +static struct attribute *mlx90614_attr[] = {
>> +	&iio_dev_attr_emissivity.dev_attr.attr,
>> +	&iio_dev_attr_iir.dev_attr.attr,
> Hmm odd to have two different filter types with separate control.
> However we have _filter_low_pass_3db_frequency and whilst it is a pain
> obviously to map this to that form that's the way to go from a useful
> userspace interface point of view.  If we need to extend the filter
> description interface (filter type has been suggested before) then please
> propose that.
> 
> Here I see they are suggesting one filter is effectively for ignoreing
> fast passing objects, and the other for noise prevention.  They both
> effect the settling time.
> 
> Anyhow it's not entirely obvious what the right answer is, but its
> definitely not a driver specific interface such as we have.
> 
> Peter, any thoughts?
>> +	&iio_dev_attr_fir.dev_attr.attr,
>> +	&iio_dev_attr_gain.dev_attr.attr,
> Please use the standard IIO infomask elements for scale (or perhaps
> calibscale - I haven't checked which is appropriate).
>> +	NULL
>> +};
>> +
>> +static struct attribute_group mlx90614_attr_group = {
>> +	.attrs = mlx90614_attr,
>> +};
>> +
>>  static const struct iio_info mlx90614_info = {
>> +	.attrs = &mlx90614_attr_group,
>>  	.read_raw = mlx90614_read_raw,
>>  	.driver_module = THIS_MODULE,
>>  };
>> @@ -189,6 +350,7 @@ static int mlx90614_probe(struct i2c_client *client,
>>  	data = iio_priv(indio_dev);
>>  	i2c_set_clientdata(client, indio_dev);
>>  	data->client = client;
>> +	mutex_init(&data->lock);
>>  
>>  	indio_dev->dev.parent = &client->dev;
>>  	indio_dev->name = id->name;
>>
> 

Re: [PATCH 5/7] iio: mlx90614: Allow tuning EEPROM configuration

[Wolfram Sang]

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

On Mon, Mar 09, 2015 at 03:41:30PM +0000, Jonathan Cameron wrote:
> On 09/03/15 15:35, Jonathan Cameron wrote:
> > On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
> >> Add device attributes for getting/setting emissivity, IIR, and FIR
> >> coefficients, and getting the gain (which should not be modified in
> >> order to keep factory calibration).
> >>
> >> The attributes provide raw values whose meaning is described in the
> >> datasheet [1].
> >>
> >> Writing to EEPROM requires an explicit erase by writing zero.  In
> >> addition, it takes 20ms for the erase/write to complete.  During this
> >> time no EEPROM register should be accessed.  Therefore, two msleep()s
> >> are added to the write function and a mutex protects against concurrent
> >> access.
> >>
> >> Since it is not expected to be updated frequently, the configuration
> >> register is read before modifying it rather than caching it.
> >>
> >> [1] http://melexis.com/Assets/IR-sensor-thermometer-MLX90614-Datasheet-5152.aspx
> >>
> >> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
> >> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> > Wolfram, bit of odd i2c usage inline I'd like you to take quick look at.

What do you mean? The direct calls to i2c_smbus_xfer? Was there any
reason given to use it directly?

> Sorry, this would work better if I cleared Wolfram's old email addresses out of
> my address book at somepoint!

How about: right now? ;)

> 
> >> ---
> >>  drivers/iio/temperature/mlx90614.c | 164 ++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 163 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> >> index 0b36746..ab98fb6 100644
> >> --- a/drivers/iio/temperature/mlx90614.c
> >> +++ b/drivers/iio/temperature/mlx90614.c
> >> @@ -12,14 +12,16 @@
> >>   *
> >>   * (7-bit I2C slave address 0x5a, 100KHz bus speed only!)
> >>   *
> >> - * TODO: sleep mode, configuration EEPROM
> >> + * TODO: sleep mode
> >>   */
> >>  
> >>  #include <linux/err.h>
> >>  #include <linux/i2c.h>
> >>  #include <linux/module.h>
> >> +#include <linux/delay.h>
> >>  
> >>  #include <linux/iio/iio.h>
> >> +#include <linux/iio/sysfs.h>
> >>  
> >>  #define MLX90614_OP_RAM		0x00
> >>  #define MLX90614_OP_EEPROM	0x20
> >> @@ -53,8 +55,47 @@
> >>  
> >>  struct mlx90614_data {
> >>  	struct i2c_client *client;
> >> +	struct mutex lock; /* for EEPROM access only */
> >>  };
> >>  
> >> +/*
> >> + * Erase an address and write word.
> >> + * The mutex must be locked before calling.
> >> + */
> >> +static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
> >> +			       u16 value)
> >> +{
> >> +	/*
> >> +	 * Note: The mlx90614 requires a PEC on writing but does not send us a
> >> +	 * valid PEC on reading.  Hence, we cannot set I2C_CLIENT_PEC in
> >> +	 * i2c_client.flags.  As a workaround, we use i2c_smbus_xfer here.
> >> +	 */
> > That's particularly hideous and someone should shout at whoever implemented that!
> > Ah well, such is life.  I want a go ahead on doing this from Wolfram however.
> > It's the sort of thing that might well get randomly broken in the future.
> > Alternative is to add another flag that indicates to the i2c core that the PEC
> > that comes back will be wrong.
> >  
> >> +	union i2c_smbus_data data;
> >> +	s32 ret;
> >> +
> >> +	dev_dbg(&client->dev, "Writing 0x%x to address 0x%x", value, command);
> >> +
> >> +	data.word = 0x0000; /* erase command */
> >> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
> >> +			     client->flags | I2C_CLIENT_PEC,
> >> +			     I2C_SMBUS_WRITE, command,
> >> +			     I2C_SMBUS_WORD_DATA, &data);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	msleep(MLX90614_TIMING_EEPROM);
> >> +
> >> +	data.word = value; /* actual write */
> >> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
> >> +			     client->flags | I2C_CLIENT_PEC,
> >> +			     I2C_SMBUS_WRITE, command,
> >> +			     I2C_SMBUS_WORD_DATA, &data);
> >> +
> >> +	msleep(MLX90614_TIMING_EEPROM);
> >> +
> >> +	return ret;
> >> +}

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

Re: [PATCH 5/7] iio: mlx90614: Allow tuning EEPROM configuration

[Jonathan Cameron]

On 09/03/15 16:45, Wolfram Sang wrote:
> On Mon, Mar 09, 2015 at 03:41:30PM +0000, Jonathan Cameron wrote:
>> On 09/03/15 15:35, Jonathan Cameron wrote:
>>> On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
>>>> Add device attributes for getting/setting emissivity, IIR, and FIR
>>>> coefficients, and getting the gain (which should not be modified in
>>>> order to keep factory calibration).
>>>>
>>>> The attributes provide raw values whose meaning is described in the
>>>> datasheet [1].
>>>>
>>>> Writing to EEPROM requires an explicit erase by writing zero.  In
>>>> addition, it takes 20ms for the erase/write to complete.  During this
>>>> time no EEPROM register should be accessed.  Therefore, two msleep()s
>>>> are added to the write function and a mutex protects against concurrent
>>>> access.
>>>>
>>>> Since it is not expected to be updated frequently, the configuration
>>>> register is read before modifying it rather than caching it.
>>>>
>>>> [1] http://melexis.com/Assets/IR-sensor-thermometer-MLX90614-Datasheet-5152.aspx
>>>>
>>>> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
>>>> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
>>> Wolfram, bit of odd i2c usage inline I'd like you to take quick look at.
> 
> What do you mean? The direct calls to i2c_smbus_xfer? Was there any
> reason given to use it directly?
Apparently the returned PEC is present but wrong.  However it requires
a correct PEC to be transmitted to it.  Genious
> 
>> Sorry, this would work better if I cleared Wolfram's old email addresses out of
>> my address book at somepoint!
> 
> How about: right now? ;)
Did so earlier - was hiding in several different places.
> 
>>
>>>> ---
>>>>  drivers/iio/temperature/mlx90614.c | 164 ++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 163 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
>>>> index 0b36746..ab98fb6 100644
>>>> --- a/drivers/iio/temperature/mlx90614.c
>>>> +++ b/drivers/iio/temperature/mlx90614.c
>>>> @@ -12,14 +12,16 @@
>>>>   *
>>>>   * (7-bit I2C slave address 0x5a, 100KHz bus speed only!)
>>>>   *
>>>> - * TODO: sleep mode, configuration EEPROM
>>>> + * TODO: sleep mode
>>>>   */
>>>>  
>>>>  #include <linux/err.h>
>>>>  #include <linux/i2c.h>
>>>>  #include <linux/module.h>
>>>> +#include <linux/delay.h>
>>>>  
>>>>  #include <linux/iio/iio.h>
>>>> +#include <linux/iio/sysfs.h>
>>>>  
>>>>  #define MLX90614_OP_RAM		0x00
>>>>  #define MLX90614_OP_EEPROM	0x20
>>>> @@ -53,8 +55,47 @@
>>>>  
>>>>  struct mlx90614_data {
>>>>  	struct i2c_client *client;
>>>> +	struct mutex lock; /* for EEPROM access only */
>>>>  };
>>>>  
>>>> +/*
>>>> + * Erase an address and write word.
>>>> + * The mutex must be locked before calling.
>>>> + */
>>>> +static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
>>>> +			       u16 value)
>>>> +{
>>>> +	/*
>>>> +	 * Note: The mlx90614 requires a PEC on writing but does not send us a
>>>> +	 * valid PEC on reading.  Hence, we cannot set I2C_CLIENT_PEC in
>>>> +	 * i2c_client.flags.  As a workaround, we use i2c_smbus_xfer here.
>>>> +	 */
>>> That's particularly hideous and someone should shout at whoever implemented that!
>>> Ah well, such is life.  I want a go ahead on doing this from Wolfram however.
>>> It's the sort of thing that might well get randomly broken in the future.
>>> Alternative is to add another flag that indicates to the i2c core that the PEC
>>> that comes back will be wrong.
>>>  
>>>> +	union i2c_smbus_data data;
>>>> +	s32 ret;
>>>> +
>>>> +	dev_dbg(&client->dev, "Writing 0x%x to address 0x%x", value, command);
>>>> +
>>>> +	data.word = 0x0000; /* erase command */
>>>> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
>>>> +			     client->flags | I2C_CLIENT_PEC,
>>>> +			     I2C_SMBUS_WRITE, command,
>>>> +			     I2C_SMBUS_WORD_DATA, &data);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	msleep(MLX90614_TIMING_EEPROM);
>>>> +
>>>> +	data.word = value; /* actual write */
>>>> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
>>>> +			     client->flags | I2C_CLIENT_PEC,
>>>> +			     I2C_SMBUS_WRITE, command,
>>>> +			     I2C_SMBUS_WORD_DATA, &data);
>>>> +
>>>> +	msleep(MLX90614_TIMING_EEPROM);
>>>> +
>>>> +	return ret;
>>>> +}

Re: [PATCH 5/7] iio: mlx90614: Allow tuning EEPROM configuration

[Wolfram Sang]

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

On Mon, Mar 09, 2015 at 05:02:31PM +0000, Jonathan Cameron wrote:
> On 09/03/15 16:45, Wolfram Sang wrote:
> > On Mon, Mar 09, 2015 at 03:41:30PM +0000, Jonathan Cameron wrote:
> >> On 09/03/15 15:35, Jonathan Cameron wrote:
> >>> On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
> >>>> Add device attributes for getting/setting emissivity, IIR, and FIR
> >>>> coefficients, and getting the gain (which should not be modified in
> >>>> order to keep factory calibration).
> >>>>
> >>>> The attributes provide raw values whose meaning is described in the
> >>>> datasheet [1].
> >>>>
> >>>> Writing to EEPROM requires an explicit erase by writing zero.  In
> >>>> addition, it takes 20ms for the erase/write to complete.  During this
> >>>> time no EEPROM register should be accessed.  Therefore, two msleep()s
> >>>> are added to the write function and a mutex protects against concurrent
> >>>> access.
> >>>>
> >>>> Since it is not expected to be updated frequently, the configuration
> >>>> register is read before modifying it rather than caching it.
> >>>>
> >>>> [1] http://melexis.com/Assets/IR-sensor-thermometer-MLX90614-Datasheet-5152.aspx
> >>>>
> >>>> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
> >>>> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> >>> Wolfram, bit of odd i2c usage inline I'd like you to take quick look at.
> > 
> > What do you mean? The direct calls to i2c_smbus_xfer? Was there any
> > reason given to use it directly?
> Apparently the returned PEC is present but wrong.  However it requires
> a correct PEC to be transmitted to it.  Genious

Ouch. Well, in such a case I think it is okay to call i2c_smbus_transfer
directly, especially since it is described well in the comment. lm90
does this, too. But there, it is the writes that are broken. I don't
feel to add quirk flags for all possible permutations of this problem.


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

Re: [PATCH 5/7] iio: mlx90614: Allow tuning EEPROM configuration

[Vianney le Clément]

On Mon, Mar 9, 2015 at 4:41 , Jonathan Cameron <jic23@kernel.org> wrote:
> 
>  Emissivity is well defined. I'd like this to be a standard attribute 
> with
> 
>  full documentation please rather than a driver specific one.  Happy
>  to have this added to the infomask elements if it make sense.

It does make sense.  This parameter cannot be set separately for the 
two IR sensors though (for the dual-sensor model).  Should the 
attribute still be shared by type, even though it does not apply to the 
ambient temperature?  Or should we add "shared by modifier" attributes 
(not sure it is worth it)?

> 
>  Hmm odd to have two different filter types with separate control.
> 
>  However we have _filter_low_pass_3db_frequency and whilst it is a 
> pain
> 
>  obviously to map this to that form that's the way to go from a useful
> 
>  userspace interface point of view.  If we need to extend the filter
> 
>  description interface (filter type has been suggested before) then 
> please
> 
>  propose that.
> 
> 
> 
>  Here I see they are suggesting one filter is effectively for 
> ignoreing
> 
>  fast passing objects, and the other for noise prevention.  They both
> 
>  effect the settling time.
> 
> 
> 
>  Anyhow it's not entirely obvious what the right answer is, but its
> 
>  definitely not a driver specific interface such as we have.
> 
> 
>  Peter, any thoughts?

I will look into this.  Any thoughts from Peter are indeed welcome.

>>  +	&iio_dev_attr_gain.dev_attr.attr,
> 
>  Please use the standard IIO infomask elements for scale (or perhaps
>  calibscale - I haven't checked which is appropriate).

Ok.

Vianney

[PATCH 6/7] iio: mlx90614: Add power management

[Vianney le Clément de Saint-Marcq]

Add support for system sleep and runtime power management.

To wake up the device, the SDA line should be held low for at least 33ms
while SCL is high.  As this is not possible using the i2c API (and not
supported by all i2c adapters), a GPIO connected to the SDA line is
needed.  The GPIO is named "wakeup" and can be specified in a device
tree with the "wakeup-gpios" binding.

If the wake-up GPIO is not given, disable power management for the
device.  Entering sleep requires an SMBus byte access, hence power
management is also disabled if byte access is not supported by the
adapter.

Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

---

The default autosleep delay (5s) is chosen arbitrarily, trying to take
into account the long startup time (250ms).  Feel free to change it to
another value if you think it is saner.
---
 .../bindings/iio/temperature/mlx90614.txt          |  24 ++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/iio/temperature/mlx90614.c                 | 244 ++++++++++++++++++++-
 3 files changed, 267 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/mlx90614.txt

diff --git a/Documentation/devicetree/bindings/iio/temperature/mlx90614.txt b/Documentation/devicetree/bindings/iio/temperature/mlx90614.txt
new file mode 100644
index 0000000..9be57b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/temperature/mlx90614.txt
@@ -0,0 +1,24 @@
+* Melexis MLX90614 contactless IR temperature sensor
+
+http://melexis.com/Infrared-Thermometer-Sensors/Infrared-Thermometer-Sensors/MLX90614-615.aspx
+
+Required properties:
+
+  - compatible: should be "melexis,mlx90614"
+  - reg: the I2C address of the sensor
+
+Optional properties:
+
+  - wakeup-gpios: device tree identifier of the GPIO connected to the SDA line
+      to hold low in order to wake up the device.  In normal operation, the
+      GPIO is set as input and will not interfere in I2C communication.  There
+      is no need for a GPIO driving the SCL line.  If no GPIO is given, power
+      management is disabled.
+
+Example:
+
+mlx90614@5a {
+	compatible = "melexis,mlx90614";
+	reg = <0x5a>;
+	wakeup-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 389ca13..ceacc40 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -108,6 +108,7 @@ lltc	Linear Technology Corporation
 marvell	Marvell Technology Group Ltd.
 maxim	Maxim Integrated Products
 mediatek	MediaTek Inc.
+melexis	Melexis N.V.
 merrii	Merrii Technology Co., Ltd.
 micrel	Micrel Inc.
 microchip	Microchip Technology Inc.
diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index ab98fb6..49c517a 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -12,13 +12,22 @@
  *
  * (7-bit I2C slave address 0x5a, 100KHz bus speed only!)
  *
- * TODO: sleep mode
+ * To wake up from sleep mode, the SDA line must be held low while SCL is high
+ * for at least 33ms.  This is achieved with an extra GPIO that can be connected
+ * directly to the SDA line.  In normal operation, the GPIO is set as input and
+ * will not interfere in I2C communication.  While the GPIO is driven low, the
+ * i2c adapter is locked since it cannot be used by other clients.  The SCL line
+ * always has a pull-up so we do not need an extra GPIO to drive it high.  If
+ * the "wakeup" GPIO is not given, power management will be disabled.
  */
 
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/gpio/consumer.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -53,9 +62,13 @@
 #define MLX90614_TIMING_WAKEUP 34 /* time to hold SDA low for wake-up */
 #define MLX90614_TIMING_STARTUP 250 /* time before first data after wake-up */
 
+#define MLX90614_AUTOSLEEP_DELAY 5000 /* default autosleep delay */
+
 struct mlx90614_data {
 	struct i2c_client *client;
 	struct mutex lock; /* for EEPROM access only */
+	struct gpio_desc *wakeup_gpio; /* NULL to disable sleep/wake-up */
+	unsigned long ready_timestamp; /* in jiffies */
 };
 
 /*
@@ -96,6 +109,52 @@ static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
 	return ret;
 }
 
+#ifdef CONFIG_PM
+/*
+ * If @startup is true, make sure MLX90614_TIMING_STARTUP ms have elapsed since
+ * the last wake-up.  This is normally only needed to get a valid temperature
+ * reading.  EEPROM access does not need such delay.
+ */
+static void mlx90614_power_get(struct mlx90614_data *data, bool startup)
+{
+	unsigned long now;
+
+	if (!data->wakeup_gpio)
+		return;
+
+	pm_runtime_get_sync(&data->client->dev);
+
+	if (startup) {
+		/*
+		 * If the process requesting the temperature gets killed when
+		 * waiting here, it does not matter whether the reading is
+		 * valid.  Hence, the sleep is interruptible.
+		 */
+		now = jiffies;
+		if (time_before(now, data->ready_timestamp))
+			msleep_interruptible(jiffies_to_msecs(
+						data->ready_timestamp - now));
+	}
+}
+
+static void mlx90614_power_put(struct mlx90614_data *data)
+{
+	if (!data->wakeup_gpio)
+		return;
+
+	pm_runtime_mark_last_busy(&data->client->dev);
+	pm_runtime_put_autosuspend(&data->client->dev);
+}
+#else
+static inline void mlx90614_power_get(struct mlx90614_data *data, bool startup)
+{
+}
+
+static inline void mlx90614_power_put(struct mlx90614_data *data)
+{
+}
+#endif
+
 static int mlx90614_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *channel, int *val,
 			    int *val2, long mask)
@@ -127,7 +186,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 
+		mlx90614_power_get(data, true);
 		ret = i2c_smbus_read_word_data(data->client, cmd);
+		mlx90614_power_put(data);
+
 		if (ret < 0)
 			return ret;
 
@@ -160,9 +222,11 @@ static ssize_t mlx90614_show_emissivity(struct device *dev,
 	struct mlx90614_data *data = iio_priv(indio_dev);
 	s32 ret;
 
+	mlx90614_power_get(data, false);
 	mutex_lock(&data->lock);
 	ret = i2c_smbus_read_word_data(data->client, MLX90614_EMISSIVITY);
 	mutex_unlock(&data->lock);
+	mlx90614_power_put(data);
 
 	if (ret < 0)
 		return ret;
@@ -182,9 +246,11 @@ static ssize_t mlx90614_store_emissivity(struct device *dev,
 	if (kstrtou16(buf, 0, &val))
 		return -EINVAL;
 
+	mlx90614_power_get(data, false);
 	mutex_lock(&data->lock);
 	ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY, val);
 	mutex_unlock(&data->lock);
+	mlx90614_power_put(data);
 
 	if (ret < 0)
 		return ret;
@@ -203,9 +269,11 @@ static ssize_t mlx90614_show_config(struct device *dev,
 	u16 shift = indio_attr->address >> 16;
 	s32 ret;
 
+	mlx90614_power_get(data, false);
 	mutex_lock(&data->lock);
 	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
 	mutex_unlock(&data->lock);
+	mlx90614_power_put(data);
 
 	if (ret < 0)
 		return ret;
@@ -230,6 +298,7 @@ static ssize_t mlx90614_store_config(struct device *dev,
 	if (kstrtou16(buf, 0, &val) || val != (val & (mask >> shift)))
 		return -EINVAL;
 
+	mlx90614_power_get(data, false);
 	mutex_lock(&data->lock);
 
 	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
@@ -241,6 +310,7 @@ static ssize_t mlx90614_store_config(struct device *dev,
 
 out:
 	mutex_unlock(&data->lock);
+	mlx90614_power_put(data);
 
 	if (ret < 0)
 		return ret;
@@ -320,6 +390,98 @@ static const struct iio_info mlx90614_info = {
 	.driver_module = THIS_MODULE,
 };
 
+#ifdef CONFIG_PM
+static int mlx90614_sleep(struct mlx90614_data *data)
+{
+	s32 ret;
+
+	if (!data->wakeup_gpio) {
+		dev_dbg(&data->client->dev, "Sleep disabled");
+		return -ENOSYS;
+	}
+
+	dev_dbg(&data->client->dev, "Requesting sleep");
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_xfer(data->client->adapter, data->client->addr,
+			     data->client->flags | I2C_CLIENT_PEC,
+			     I2C_SMBUS_WRITE, MLX90614_OP_SLEEP,
+			     I2C_SMBUS_BYTE, NULL);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int mlx90614_wakeup(struct mlx90614_data *data)
+{
+	if (!data->wakeup_gpio) {
+		dev_dbg(&data->client->dev, "Wake-up disabled");
+		return -ENOSYS;
+	}
+
+	dev_dbg(&data->client->dev, "Requesting wake-up");
+
+	i2c_lock_adapter(data->client->adapter);
+	gpiod_direction_output(data->wakeup_gpio, 0);
+	msleep(MLX90614_TIMING_WAKEUP);
+	gpiod_direction_input(data->wakeup_gpio);
+	i2c_unlock_adapter(data->client->adapter);
+
+	data->ready_timestamp = jiffies +
+			msecs_to_jiffies(MLX90614_TIMING_STARTUP);
+
+	/*
+	 * Quirk: the i2c controller may get confused right after the
+	 * wake-up signal has been sent.  As a workaround, do a dummy read.
+	 * If the read fails, the controller will probably be reset so that
+	 * further reads will work.
+	 */
+	i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
+
+	return 0;
+}
+
+/* Return wake-up GPIO or NULL if sleep functionality should be disabled. */
+static struct gpio_desc *mlx90614_probe_wakeup(struct i2c_client *client)
+{
+	struct gpio_desc *gpio;
+
+	if (!i2c_check_functionality(client->adapter,
+						I2C_FUNC_SMBUS_WRITE_BYTE)) {
+		dev_info(&client->dev,
+			 "i2c adapter does not support SMBUS_WRITE_BYTE, sleep disabled");
+		return NULL;
+	}
+
+	gpio = devm_gpiod_get_optional(&client->dev, "wakeup", GPIOD_IN);
+
+	if (IS_ERR(gpio)) {
+		dev_warn(&client->dev,
+			 "gpio acquisition failed with error %ld, sleep disabled",
+			 PTR_ERR(gpio));
+		return NULL;
+	} else if (!gpio) {
+		dev_info(&client->dev,
+			 "wakeup-gpio not found, sleep disabled");
+	}
+
+	return gpio;
+}
+#else
+static inline int mlx90614_sleep(struct mlx90614_data *data)
+{
+	return -ENOSYS;
+}
+static inline int mlx90614_wakeup(struct mlx90614_data *data)
+{
+	return -ENOSYS;
+}
+static inline struct gpio_desc *mlx90614_probe_wakeup(struct i2c_client *client)
+{
+	return NULL;
+}
+#endif
+
 /* Return 0 for single sensor, 1 for dual sensor, <0 on error. */
 static int mlx90614_probe_model(struct i2c_client *client)
 {
@@ -351,6 +513,9 @@ static int mlx90614_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
 	mutex_init(&data->lock);
+	data->wakeup_gpio = mlx90614_probe_wakeup(client);
+
+	mlx90614_wakeup(data);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = id->name;
@@ -373,12 +538,30 @@ static int mlx90614_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	if (data->wakeup_gpio) {
+		pm_runtime_set_autosuspend_delay(&client->dev,
+						 MLX90614_AUTOSLEEP_DELAY);
+		pm_runtime_use_autosuspend(&client->dev);
+		pm_runtime_set_active(&client->dev);
+		pm_runtime_enable(&client->dev);
+	}
+
 	return iio_device_register(indio_dev);
 }
 
 static int mlx90614_remove(struct i2c_client *client)
 {
-	iio_device_unregister(i2c_get_clientdata(client));
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct mlx90614_data *data = iio_priv(indio_dev);
+
+	if (data->wakeup_gpio) {
+		pm_runtime_disable(&client->dev);
+		if (!pm_runtime_status_suspended(&client->dev))
+			mlx90614_sleep(data);
+		pm_runtime_set_suspended(&client->dev);
+	}
+
+	iio_device_unregister(indio_dev);
 
 	return 0;
 }
@@ -389,10 +572,67 @@ static const struct i2c_device_id mlx90614_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, mlx90614_id);
 
+#ifdef CONFIG_PM_SLEEP
+static int mlx90614_pm_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mlx90614_data *data = iio_priv(indio_dev);
+
+	if (data->wakeup_gpio && pm_runtime_active(dev))
+		return mlx90614_sleep(data);
+
+	return 0;
+}
+
+static int mlx90614_pm_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mlx90614_data *data = iio_priv(indio_dev);
+	int err;
+
+	if (data->wakeup_gpio) {
+		err = mlx90614_wakeup(data);
+		if (err < 0)
+			return err;
+
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_PM
+static int mlx90614_pm_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mlx90614_data *data = iio_priv(indio_dev);
+
+	return mlx90614_sleep(data);
+}
+
+static int mlx90614_pm_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mlx90614_data *data = iio_priv(indio_dev);
+
+	return mlx90614_wakeup(data);
+}
+#endif
+
+static const struct dev_pm_ops mlx90614_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(mlx90614_pm_suspend, mlx90614_pm_resume)
+	SET_RUNTIME_PM_OPS(mlx90614_pm_runtime_suspend,
+			   mlx90614_pm_runtime_resume, NULL)
+};
+
 static struct i2c_driver mlx90614_driver = {
 	.driver = {
 		.name	= "mlx90614",
 		.owner	= THIS_MODULE,
+		.pm	= &mlx90614_pm_ops,
 	},
 	.probe = mlx90614_probe,
 	.remove = mlx90614_remove,
-- 
2.3.0

Re: [PATCH 6/7] iio: mlx90614: Add power management

[Jonathan Cameron]

On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
> Add support for system sleep and runtime power management.
> 
> To wake up the device, the SDA line should be held low for at least 33ms
> while SCL is high.  As this is not possible using the i2c API (and not
> supported by all i2c adapters), a GPIO connected to the SDA line is
> needed.  The GPIO is named "wakeup" and can be specified in a device
> tree with the "wakeup-gpios" binding.
Needs some i2c specialist input on this!  As you mentioned it is liable
to be controversial.  Wolfram, is this a one off special or do
any other devices do this sort of magic?
> 
> If the wake-up GPIO is not given, disable power management for the
> device.  Entering sleep requires an SMBus byte access, hence power
> management is also disabled if byte access is not supported by the
> adapter.
> 
> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> 
> ---
> 
> The default autosleep delay (5s) is chosen arbitrarily, trying to take
> into account the long startup time (250ms).  Feel free to change it to
> another value if you think it is saner.
> ---
>  .../bindings/iio/temperature/mlx90614.txt          |  24 ++
>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>  drivers/iio/temperature/mlx90614.c                 | 244 ++++++++++++++++++++-
>  3 files changed, 267 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/mlx90614.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/mlx90614.txt b/Documentation/devicetree/bindings/iio/temperature/mlx90614.txt
> new file mode 100644
> index 0000000..9be57b0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/mlx90614.txt
> @@ -0,0 +1,24 @@
> +* Melexis MLX90614 contactless IR temperature sensor
> +
> +http://melexis.com/Infrared-Thermometer-Sensors/Infrared-Thermometer-Sensors/MLX90614-615.aspx
> +
> +Required properties:
> +
> +  - compatible: should be "melexis,mlx90614"
> +  - reg: the I2C address of the sensor
> +
> +Optional properties:
> +
> +  - wakeup-gpios: device tree identifier of the GPIO connected to the SDA line
> +      to hold low in order to wake up the device.  In normal operation, the
> +      GPIO is set as input and will not interfere in I2C communication.  There
> +      is no need for a GPIO driving the SCL line.  If no GPIO is given, power
> +      management is disabled.
> +
> +Example:
> +
> +mlx90614@5a {
> +	compatible = "melexis,mlx90614";
> +	reg = <0x5a>;
> +	wakeup-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 389ca13..ceacc40 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -108,6 +108,7 @@ lltc	Linear Technology Corporation
>  marvell	Marvell Technology Group Ltd.
>  maxim	Maxim Integrated Products
>  mediatek	MediaTek Inc.
> +melexis	Melexis N.V.
>  merrii	Merrii Technology Co., Ltd.
>  micrel	Micrel Inc.
>  microchip	Microchip Technology Inc.
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index ab98fb6..49c517a 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -12,13 +12,22 @@
>   *
>   * (7-bit I2C slave address 0x5a, 100KHz bus speed only!)
>   *
> - * TODO: sleep mode
> + * To wake up from sleep mode, the SDA line must be held low while SCL is high
> + * for at least 33ms.  This is achieved with an extra GPIO that can be connected
> + * directly to the SDA line.  In normal operation, the GPIO is set as input and
> + * will not interfere in I2C communication.  While the GPIO is driven low, the
> + * i2c adapter is locked since it cannot be used by other clients.  The SCL line
> + * always has a pull-up so we do not need an extra GPIO to drive it high.  If
> + * the "wakeup" GPIO is not given, power management will be disabled.
>   */
>  
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/delay.h>
> +#include <linux/jiffies.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -53,9 +62,13 @@
>  #define MLX90614_TIMING_WAKEUP 34 /* time to hold SDA low for wake-up */
>  #define MLX90614_TIMING_STARTUP 250 /* time before first data after wake-up */
>  
> +#define MLX90614_AUTOSLEEP_DELAY 5000 /* default autosleep delay */
> +
>  struct mlx90614_data {
>  	struct i2c_client *client;
>  	struct mutex lock; /* for EEPROM access only */
> +	struct gpio_desc *wakeup_gpio; /* NULL to disable sleep/wake-up */
> +	unsigned long ready_timestamp; /* in jiffies */
>  };
>  
>  /*
> @@ -96,6 +109,52 @@ static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_PM
> +/*
> + * If @startup is true, make sure MLX90614_TIMING_STARTUP ms have elapsed since
> + * the last wake-up.  This is normally only needed to get a valid temperature
> + * reading.  EEPROM access does not need such delay.
> + */
> +static void mlx90614_power_get(struct mlx90614_data *data, bool startup)
> +{
> +	unsigned long now;
> +
> +	if (!data->wakeup_gpio)
> +		return;
> +
> +	pm_runtime_get_sync(&data->client->dev);
> +
> +	if (startup) {
> +		/*
> +		 * If the process requesting the temperature gets killed when
> +		 * waiting here, it does not matter whether the reading is
> +		 * valid.  Hence, the sleep is interruptible.
> +		 */
> +		now = jiffies;
> +		if (time_before(now, data->ready_timestamp))
> +			msleep_interruptible(jiffies_to_msecs(
> +						data->ready_timestamp - now));
> +	}
> +}
> +
> +static void mlx90614_power_put(struct mlx90614_data *data)
> +{
> +	if (!data->wakeup_gpio)
> +		return;
> +
> +	pm_runtime_mark_last_busy(&data->client->dev);
> +	pm_runtime_put_autosuspend(&data->client->dev);
> +}
> +#else
> +static inline void mlx90614_power_get(struct mlx90614_data *data, bool startup)
> +{
> +}
> +
> +static inline void mlx90614_power_put(struct mlx90614_data *data)
> +{
> +}
> +#endif
> +
>  static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *channel, int *val,
>  			    int *val2, long mask)
> @@ -127,7 +186,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  
> +		mlx90614_power_get(data, true);
>  		ret = i2c_smbus_read_word_data(data->client, cmd);
> +		mlx90614_power_put(data);
> +
>  		if (ret < 0)
>  			return ret;
>  
> @@ -160,9 +222,11 @@ static ssize_t mlx90614_show_emissivity(struct device *dev,
>  	struct mlx90614_data *data = iio_priv(indio_dev);
>  	s32 ret;
>  
> +	mlx90614_power_get(data, false);
>  	mutex_lock(&data->lock);
>  	ret = i2c_smbus_read_word_data(data->client, MLX90614_EMISSIVITY);
>  	mutex_unlock(&data->lock);
> +	mlx90614_power_put(data);
>  
>  	if (ret < 0)
>  		return ret;
> @@ -182,9 +246,11 @@ static ssize_t mlx90614_store_emissivity(struct device *dev,
>  	if (kstrtou16(buf, 0, &val))
>  		return -EINVAL;
>  
> +	mlx90614_power_get(data, false);
>  	mutex_lock(&data->lock);
>  	ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY, val);
>  	mutex_unlock(&data->lock);
> +	mlx90614_power_put(data);
>  
>  	if (ret < 0)
>  		return ret;
> @@ -203,9 +269,11 @@ static ssize_t mlx90614_show_config(struct device *dev,
>  	u16 shift = indio_attr->address >> 16;
>  	s32 ret;
>  
> +	mlx90614_power_get(data, false);
>  	mutex_lock(&data->lock);
>  	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
>  	mutex_unlock(&data->lock);
> +	mlx90614_power_put(data);
>  
>  	if (ret < 0)
>  		return ret;
> @@ -230,6 +298,7 @@ static ssize_t mlx90614_store_config(struct device *dev,
>  	if (kstrtou16(buf, 0, &val) || val != (val & (mask >> shift)))
>  		return -EINVAL;
>  
> +	mlx90614_power_get(data, false);
>  	mutex_lock(&data->lock);
>  
>  	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
> @@ -241,6 +310,7 @@ static ssize_t mlx90614_store_config(struct device *dev,
>  
>  out:
>  	mutex_unlock(&data->lock);
> +	mlx90614_power_put(data);
>  
>  	if (ret < 0)
>  		return ret;
> @@ -320,6 +390,98 @@ static const struct iio_info mlx90614_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +#ifdef CONFIG_PM
> +static int mlx90614_sleep(struct mlx90614_data *data)
> +{
> +	s32 ret;
> +
> +	if (!data->wakeup_gpio) {
> +		dev_dbg(&data->client->dev, "Sleep disabled");
> +		return -ENOSYS;
> +	}
> +
> +	dev_dbg(&data->client->dev, "Requesting sleep");
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_xfer(data->client->adapter, data->client->addr,
> +			     data->client->flags | I2C_CLIENT_PEC,
> +			     I2C_SMBUS_WRITE, MLX90614_OP_SLEEP,
> +			     I2C_SMBUS_BYTE, NULL);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int mlx90614_wakeup(struct mlx90614_data *data)
> +{
> +	if (!data->wakeup_gpio) {
> +		dev_dbg(&data->client->dev, "Wake-up disabled");
> +		return -ENOSYS;
> +	}
> +
> +	dev_dbg(&data->client->dev, "Requesting wake-up");
> +
> +	i2c_lock_adapter(data->client->adapter);
> +	gpiod_direction_output(data->wakeup_gpio, 0);
> +	msleep(MLX90614_TIMING_WAKEUP);
> +	gpiod_direction_input(data->wakeup_gpio);
> +	i2c_unlock_adapter(data->client->adapter);
> +
> +	data->ready_timestamp = jiffies +
> +			msecs_to_jiffies(MLX90614_TIMING_STARTUP);
> +
> +	/*
> +	 * Quirk: the i2c controller may get confused right after the
> +	 * wake-up signal has been sent.  As a workaround, do a dummy read.
> +	 * If the read fails, the controller will probably be reset so that
> +	 * further reads will work.
> +	 */
> +	i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
> +
> +	return 0;
> +}
> +
> +/* Return wake-up GPIO or NULL if sleep functionality should be disabled. */
> +static struct gpio_desc *mlx90614_probe_wakeup(struct i2c_client *client)
> +{
> +	struct gpio_desc *gpio;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +						I2C_FUNC_SMBUS_WRITE_BYTE)) {
> +		dev_info(&client->dev,
> +			 "i2c adapter does not support SMBUS_WRITE_BYTE, sleep disabled");
> +		return NULL;
> +	}
> +
> +	gpio = devm_gpiod_get_optional(&client->dev, "wakeup", GPIOD_IN);
> +
> +	if (IS_ERR(gpio)) {
> +		dev_warn(&client->dev,
> +			 "gpio acquisition failed with error %ld, sleep disabled",
> +			 PTR_ERR(gpio));
> +		return NULL;
> +	} else if (!gpio) {
> +		dev_info(&client->dev,
> +			 "wakeup-gpio not found, sleep disabled");
> +	}
> +
> +	return gpio;
> +}
> +#else
> +static inline int mlx90614_sleep(struct mlx90614_data *data)
> +{
> +	return -ENOSYS;
> +}
> +static inline int mlx90614_wakeup(struct mlx90614_data *data)
> +{
> +	return -ENOSYS;
> +}
> +static inline struct gpio_desc *mlx90614_probe_wakeup(struct i2c_client *client)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  /* Return 0 for single sensor, 1 for dual sensor, <0 on error. */
>  static int mlx90614_probe_model(struct i2c_client *client)
>  {
> @@ -351,6 +513,9 @@ static int mlx90614_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
>  	mutex_init(&data->lock);
> +	data->wakeup_gpio = mlx90614_probe_wakeup(client);
> +
> +	mlx90614_wakeup(data);
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->name = id->name;
> @@ -373,12 +538,30 @@ static int mlx90614_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	if (data->wakeup_gpio) {
> +		pm_runtime_set_autosuspend_delay(&client->dev,
> +						 MLX90614_AUTOSLEEP_DELAY);
> +		pm_runtime_use_autosuspend(&client->dev);
> +		pm_runtime_set_active(&client->dev);
> +		pm_runtime_enable(&client->dev);
> +	}
> +
>  	return iio_device_register(indio_dev);
>  }
>  
>  static int mlx90614_remove(struct i2c_client *client)
>  {
> -	iio_device_unregister(i2c_get_clientdata(client));
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +
> +	if (data->wakeup_gpio) {
> +		pm_runtime_disable(&client->dev);
> +		if (!pm_runtime_status_suspended(&client->dev))
> +			mlx90614_sleep(data);
> +		pm_runtime_set_suspended(&client->dev);
> +	}
> +
> +	iio_device_unregister(indio_dev);
>  
>  	return 0;
>  }
> @@ -389,10 +572,67 @@ static const struct i2c_device_id mlx90614_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, mlx90614_id);
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int mlx90614_pm_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +
> +	if (data->wakeup_gpio && pm_runtime_active(dev))
> +		return mlx90614_sleep(data);
> +
> +	return 0;
> +}
> +
> +static int mlx90614_pm_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +	int err;
> +
> +	if (data->wakeup_gpio) {
> +		err = mlx90614_wakeup(data);
> +		if (err < 0)
> +			return err;
> +
> +		pm_runtime_disable(dev);
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +#ifdef CONFIG_PM
> +static int mlx90614_pm_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +
> +	return mlx90614_sleep(data);
> +}
> +
> +static int mlx90614_pm_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +
> +	return mlx90614_wakeup(data);
> +}
> +#endif
> +
> +static const struct dev_pm_ops mlx90614_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(mlx90614_pm_suspend, mlx90614_pm_resume)
> +	SET_RUNTIME_PM_OPS(mlx90614_pm_runtime_suspend,
> +			   mlx90614_pm_runtime_resume, NULL)
> +};
> +
>  static struct i2c_driver mlx90614_driver = {
>  	.driver = {
>  		.name	= "mlx90614",
>  		.owner	= THIS_MODULE,
> +		.pm	= &mlx90614_pm_ops,
>  	},
>  	.probe = mlx90614_probe,
>  	.remove = mlx90614_remove,
> 

Re: [PATCH 6/7] iio: mlx90614: Add power management

[Jonathan Cameron]

On 09/03/15 15:39, Jonathan Cameron wrote:
> On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
>> Add support for system sleep and runtime power management.
>>
>> To wake up the device, the SDA line should be held low for at least 33ms
>> while SCL is high.  As this is not possible using the i2c API (and not
>> supported by all i2c adapters), a GPIO connected to the SDA line is
>> needed.  The GPIO is named "wakeup" and can be specified in a device
>> tree with the "wakeup-gpios" binding.
> Needs some i2c specialist input on this!  As you mentioned it is liable
> to be controversial.  Wolfram, is this a one off special or do
> any other devices do this sort of magic?
>>
>> If the wake-up GPIO is not given, disable power management for the
>> device.  Entering sleep requires an SMBus byte access, hence power
>> management is also disabled if byte access is not supported by the
>> adapter.
>>
>> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
>> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Again, with a current address for Wolfram.
>>
>> ---
>>
>> The default autosleep delay (5s) is chosen arbitrarily, trying to take
>> into account the long startup time (250ms).  Feel free to change it to
>> another value if you think it is saner.
>> ---
>>  .../bindings/iio/temperature/mlx90614.txt          |  24 ++
>>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>>  drivers/iio/temperature/mlx90614.c                 | 244 ++++++++++++++++++++-
>>  3 files changed, 267 insertions(+), 2 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/mlx90614.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/temperature/mlx90614.txt b/Documentation/devicetree/bindings/iio/temperature/mlx90614.txt
>> new file mode 100644
>> index 0000000..9be57b0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/temperature/mlx90614.txt
>> @@ -0,0 +1,24 @@
>> +* Melexis MLX90614 contactless IR temperature sensor
>> +
>> +http://melexis.com/Infrared-Thermometer-Sensors/Infrared-Thermometer-Sensors/MLX90614-615.aspx
>> +
>> +Required properties:
>> +
>> +  - compatible: should be "melexis,mlx90614"
>> +  - reg: the I2C address of the sensor
>> +
>> +Optional properties:
>> +
>> +  - wakeup-gpios: device tree identifier of the GPIO connected to the SDA line
>> +      to hold low in order to wake up the device.  In normal operation, the
>> +      GPIO is set as input and will not interfere in I2C communication.  There
>> +      is no need for a GPIO driving the SCL line.  If no GPIO is given, power
>> +      management is disabled.
>> +
>> +Example:
>> +
>> +mlx90614@5a {
>> +	compatible = "melexis,mlx90614";
>> +	reg = <0x5a>;
>> +	wakeup-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
>> +};
>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> index 389ca13..ceacc40 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> @@ -108,6 +108,7 @@ lltc	Linear Technology Corporation
>>  marvell	Marvell Technology Group Ltd.
>>  maxim	Maxim Integrated Products
>>  mediatek	MediaTek Inc.
>> +melexis	Melexis N.V.
>>  merrii	Merrii Technology Co., Ltd.
>>  micrel	Micrel Inc.
>>  microchip	Microchip Technology Inc.
>> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
>> index ab98fb6..49c517a 100644
>> --- a/drivers/iio/temperature/mlx90614.c
>> +++ b/drivers/iio/temperature/mlx90614.c
>> @@ -12,13 +12,22 @@
>>   *
>>   * (7-bit I2C slave address 0x5a, 100KHz bus speed only!)
>>   *
>> - * TODO: sleep mode
>> + * To wake up from sleep mode, the SDA line must be held low while SCL is high
>> + * for at least 33ms.  This is achieved with an extra GPIO that can be connected
>> + * directly to the SDA line.  In normal operation, the GPIO is set as input and
>> + * will not interfere in I2C communication.  While the GPIO is driven low, the
>> + * i2c adapter is locked since it cannot be used by other clients.  The SCL line
>> + * always has a pull-up so we do not need an extra GPIO to drive it high.  If
>> + * the "wakeup" GPIO is not given, power management will be disabled.
>>   */
>>  
>>  #include <linux/err.h>
>>  #include <linux/i2c.h>
>>  #include <linux/module.h>
>>  #include <linux/delay.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/pm_runtime.h>
>>  
>>  #include <linux/iio/iio.h>
>>  #include <linux/iio/sysfs.h>
>> @@ -53,9 +62,13 @@
>>  #define MLX90614_TIMING_WAKEUP 34 /* time to hold SDA low for wake-up */
>>  #define MLX90614_TIMING_STARTUP 250 /* time before first data after wake-up */
>>  
>> +#define MLX90614_AUTOSLEEP_DELAY 5000 /* default autosleep delay */
>> +
>>  struct mlx90614_data {
>>  	struct i2c_client *client;
>>  	struct mutex lock; /* for EEPROM access only */
>> +	struct gpio_desc *wakeup_gpio; /* NULL to disable sleep/wake-up */
>> +	unsigned long ready_timestamp; /* in jiffies */
>>  };
>>  
>>  /*
>> @@ -96,6 +109,52 @@ static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
>>  	return ret;
>>  }
>>  
>> +#ifdef CONFIG_PM
>> +/*
>> + * If @startup is true, make sure MLX90614_TIMING_STARTUP ms have elapsed since
>> + * the last wake-up.  This is normally only needed to get a valid temperature
>> + * reading.  EEPROM access does not need such delay.
>> + */
>> +static void mlx90614_power_get(struct mlx90614_data *data, bool startup)
>> +{
>> +	unsigned long now;
>> +
>> +	if (!data->wakeup_gpio)
>> +		return;
>> +
>> +	pm_runtime_get_sync(&data->client->dev);
>> +
>> +	if (startup) {
>> +		/*
>> +		 * If the process requesting the temperature gets killed when
>> +		 * waiting here, it does not matter whether the reading is
>> +		 * valid.  Hence, the sleep is interruptible.
>> +		 */
>> +		now = jiffies;
>> +		if (time_before(now, data->ready_timestamp))
>> +			msleep_interruptible(jiffies_to_msecs(
>> +						data->ready_timestamp - now));
>> +	}
>> +}
>> +
>> +static void mlx90614_power_put(struct mlx90614_data *data)
>> +{
>> +	if (!data->wakeup_gpio)
>> +		return;
>> +
>> +	pm_runtime_mark_last_busy(&data->client->dev);
>> +	pm_runtime_put_autosuspend(&data->client->dev);
>> +}
>> +#else
>> +static inline void mlx90614_power_get(struct mlx90614_data *data, bool startup)
>> +{
>> +}
>> +
>> +static inline void mlx90614_power_put(struct mlx90614_data *data)
>> +{
>> +}
>> +#endif
>> +
>>  static int mlx90614_read_raw(struct iio_dev *indio_dev,
>>  			    struct iio_chan_spec const *channel, int *val,
>>  			    int *val2, long mask)
>> @@ -127,7 +186,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>>  			return -EINVAL;
>>  		}
>>  
>> +		mlx90614_power_get(data, true);
>>  		ret = i2c_smbus_read_word_data(data->client, cmd);
>> +		mlx90614_power_put(data);
>> +
>>  		if (ret < 0)
>>  			return ret;
>>  
>> @@ -160,9 +222,11 @@ static ssize_t mlx90614_show_emissivity(struct device *dev,
>>  	struct mlx90614_data *data = iio_priv(indio_dev);
>>  	s32 ret;
>>  
>> +	mlx90614_power_get(data, false);
>>  	mutex_lock(&data->lock);
>>  	ret = i2c_smbus_read_word_data(data->client, MLX90614_EMISSIVITY);
>>  	mutex_unlock(&data->lock);
>> +	mlx90614_power_put(data);
>>  
>>  	if (ret < 0)
>>  		return ret;
>> @@ -182,9 +246,11 @@ static ssize_t mlx90614_store_emissivity(struct device *dev,
>>  	if (kstrtou16(buf, 0, &val))
>>  		return -EINVAL;
>>  
>> +	mlx90614_power_get(data, false);
>>  	mutex_lock(&data->lock);
>>  	ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY, val);
>>  	mutex_unlock(&data->lock);
>> +	mlx90614_power_put(data);
>>  
>>  	if (ret < 0)
>>  		return ret;
>> @@ -203,9 +269,11 @@ static ssize_t mlx90614_show_config(struct device *dev,
>>  	u16 shift = indio_attr->address >> 16;
>>  	s32 ret;
>>  
>> +	mlx90614_power_get(data, false);
>>  	mutex_lock(&data->lock);
>>  	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
>>  	mutex_unlock(&data->lock);
>> +	mlx90614_power_put(data);
>>  
>>  	if (ret < 0)
>>  		return ret;
>> @@ -230,6 +298,7 @@ static ssize_t mlx90614_store_config(struct device *dev,
>>  	if (kstrtou16(buf, 0, &val) || val != (val & (mask >> shift)))
>>  		return -EINVAL;
>>  
>> +	mlx90614_power_get(data, false);
>>  	mutex_lock(&data->lock);
>>  
>>  	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
>> @@ -241,6 +310,7 @@ static ssize_t mlx90614_store_config(struct device *dev,
>>  
>>  out:
>>  	mutex_unlock(&data->lock);
>> +	mlx90614_power_put(data);
>>  
>>  	if (ret < 0)
>>  		return ret;
>> @@ -320,6 +390,98 @@ static const struct iio_info mlx90614_info = {
>>  	.driver_module = THIS_MODULE,
>>  };
>>  
>> +#ifdef CONFIG_PM
>> +static int mlx90614_sleep(struct mlx90614_data *data)
>> +{
>> +	s32 ret;
>> +
>> +	if (!data->wakeup_gpio) {
>> +		dev_dbg(&data->client->dev, "Sleep disabled");
>> +		return -ENOSYS;
>> +	}
>> +
>> +	dev_dbg(&data->client->dev, "Requesting sleep");
>> +
>> +	mutex_lock(&data->lock);
>> +	ret = i2c_smbus_xfer(data->client->adapter, data->client->addr,
>> +			     data->client->flags | I2C_CLIENT_PEC,
>> +			     I2C_SMBUS_WRITE, MLX90614_OP_SLEEP,
>> +			     I2C_SMBUS_BYTE, NULL);
>> +	mutex_unlock(&data->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int mlx90614_wakeup(struct mlx90614_data *data)
>> +{
>> +	if (!data->wakeup_gpio) {
>> +		dev_dbg(&data->client->dev, "Wake-up disabled");
>> +		return -ENOSYS;
>> +	}
>> +
>> +	dev_dbg(&data->client->dev, "Requesting wake-up");
>> +
>> +	i2c_lock_adapter(data->client->adapter);
>> +	gpiod_direction_output(data->wakeup_gpio, 0);
>> +	msleep(MLX90614_TIMING_WAKEUP);
>> +	gpiod_direction_input(data->wakeup_gpio);
>> +	i2c_unlock_adapter(data->client->adapter);
>> +
>> +	data->ready_timestamp = jiffies +
>> +			msecs_to_jiffies(MLX90614_TIMING_STARTUP);
>> +
>> +	/*
>> +	 * Quirk: the i2c controller may get confused right after the
>> +	 * wake-up signal has been sent.  As a workaround, do a dummy read.
>> +	 * If the read fails, the controller will probably be reset so that
>> +	 * further reads will work.
>> +	 */
>> +	i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
>> +
>> +	return 0;
>> +}
>> +
>> +/* Return wake-up GPIO or NULL if sleep functionality should be disabled. */
>> +static struct gpio_desc *mlx90614_probe_wakeup(struct i2c_client *client)
>> +{
>> +	struct gpio_desc *gpio;
>> +
>> +	if (!i2c_check_functionality(client->adapter,
>> +						I2C_FUNC_SMBUS_WRITE_BYTE)) {
>> +		dev_info(&client->dev,
>> +			 "i2c adapter does not support SMBUS_WRITE_BYTE, sleep disabled");
>> +		return NULL;
>> +	}
>> +
>> +	gpio = devm_gpiod_get_optional(&client->dev, "wakeup", GPIOD_IN);
>> +
>> +	if (IS_ERR(gpio)) {
>> +		dev_warn(&client->dev,
>> +			 "gpio acquisition failed with error %ld, sleep disabled",
>> +			 PTR_ERR(gpio));
>> +		return NULL;
>> +	} else if (!gpio) {
>> +		dev_info(&client->dev,
>> +			 "wakeup-gpio not found, sleep disabled");
>> +	}
>> +
>> +	return gpio;
>> +}
>> +#else
>> +static inline int mlx90614_sleep(struct mlx90614_data *data)
>> +{
>> +	return -ENOSYS;
>> +}
>> +static inline int mlx90614_wakeup(struct mlx90614_data *data)
>> +{
>> +	return -ENOSYS;
>> +}
>> +static inline struct gpio_desc *mlx90614_probe_wakeup(struct i2c_client *client)
>> +{
>> +	return NULL;
>> +}
>> +#endif
>> +
>>  /* Return 0 for single sensor, 1 for dual sensor, <0 on error. */
>>  static int mlx90614_probe_model(struct i2c_client *client)
>>  {
>> @@ -351,6 +513,9 @@ static int mlx90614_probe(struct i2c_client *client,
>>  	i2c_set_clientdata(client, indio_dev);
>>  	data->client = client;
>>  	mutex_init(&data->lock);
>> +	data->wakeup_gpio = mlx90614_probe_wakeup(client);
>> +
>> +	mlx90614_wakeup(data);
>>  
>>  	indio_dev->dev.parent = &client->dev;
>>  	indio_dev->name = id->name;
>> @@ -373,12 +538,30 @@ static int mlx90614_probe(struct i2c_client *client,
>>  		return ret;
>>  	}
>>  
>> +	if (data->wakeup_gpio) {
>> +		pm_runtime_set_autosuspend_delay(&client->dev,
>> +						 MLX90614_AUTOSLEEP_DELAY);
>> +		pm_runtime_use_autosuspend(&client->dev);
>> +		pm_runtime_set_active(&client->dev);
>> +		pm_runtime_enable(&client->dev);
>> +	}
>> +
>>  	return iio_device_register(indio_dev);
>>  }
>>  
>>  static int mlx90614_remove(struct i2c_client *client)
>>  {
>> -	iio_device_unregister(i2c_get_clientdata(client));
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct mlx90614_data *data = iio_priv(indio_dev);
>> +
>> +	if (data->wakeup_gpio) {
>> +		pm_runtime_disable(&client->dev);
>> +		if (!pm_runtime_status_suspended(&client->dev))
>> +			mlx90614_sleep(data);
>> +		pm_runtime_set_suspended(&client->dev);
>> +	}
>> +
>> +	iio_device_unregister(indio_dev);
>>  
>>  	return 0;
>>  }
>> @@ -389,10 +572,67 @@ static const struct i2c_device_id mlx90614_id[] = {
>>  };
>>  MODULE_DEVICE_TABLE(i2c, mlx90614_id);
>>  
>> +#ifdef CONFIG_PM_SLEEP
>> +static int mlx90614_pm_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct mlx90614_data *data = iio_priv(indio_dev);
>> +
>> +	if (data->wakeup_gpio && pm_runtime_active(dev))
>> +		return mlx90614_sleep(data);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mlx90614_pm_resume(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct mlx90614_data *data = iio_priv(indio_dev);
>> +	int err;
>> +
>> +	if (data->wakeup_gpio) {
>> +		err = mlx90614_wakeup(data);
>> +		if (err < 0)
>> +			return err;
>> +
>> +		pm_runtime_disable(dev);
>> +		pm_runtime_set_active(dev);
>> +		pm_runtime_enable(dev);
>> +	}
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +#ifdef CONFIG_PM
>> +static int mlx90614_pm_runtime_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct mlx90614_data *data = iio_priv(indio_dev);
>> +
>> +	return mlx90614_sleep(data);
>> +}
>> +
>> +static int mlx90614_pm_runtime_resume(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct mlx90614_data *data = iio_priv(indio_dev);
>> +
>> +	return mlx90614_wakeup(data);
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops mlx90614_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(mlx90614_pm_suspend, mlx90614_pm_resume)
>> +	SET_RUNTIME_PM_OPS(mlx90614_pm_runtime_suspend,
>> +			   mlx90614_pm_runtime_resume, NULL)
>> +};
>> +
>>  static struct i2c_driver mlx90614_driver = {
>>  	.driver = {
>>  		.name	= "mlx90614",
>>  		.owner	= THIS_MODULE,
>> +		.pm	= &mlx90614_pm_ops,
>>  	},
>>  	.probe = mlx90614_probe,
>>  	.remove = mlx90614_remove,
>>
> 

Re: [PATCH 6/7] iio: mlx90614: Add power management

[Wolfram Sang]

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

On Mon, Mar 09, 2015 at 03:39:35PM +0000, Jonathan Cameron wrote:
> On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
> > Add support for system sleep and runtime power management.
> > 
> > To wake up the device, the SDA line should be held low for at least 33ms
> > while SCL is high.  As this is not possible using the i2c API (and not
> > supported by all i2c adapters), a GPIO connected to the SDA line is
> > needed.  The GPIO is named "wakeup" and can be specified in a device
> > tree with the "wakeup-gpios" binding.
> Needs some i2c specialist input on this!  As you mentioned it is liable
> to be controversial.  Wolfram, is this a one off special or do
> any other devices do this sort of magic?

s/magic/insanity/ :)

I have never heard of something like this. Unsuprisingly, I can
not recommend doing it this way. But we all know hardware...

And while we could think about reusing the bus_recovery_infrastructure,
I don't think it is worth the hazzle. So, doing this GPIO fallback may
well be the best we can do now.


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

Re: [PATCH 6/7] iio: mlx90614: Add power management

[Jonathan Cameron]

On 09/03/15 16:43, Wolfram Sang wrote:
> On Mon, Mar 09, 2015 at 03:39:35PM +0000, Jonathan Cameron wrote:
>> On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
>>> Add support for system sleep and runtime power management.
>>>
>>> To wake up the device, the SDA line should be held low for at least 33ms
>>> while SCL is high.  As this is not possible using the i2c API (and not
>>> supported by all i2c adapters), a GPIO connected to the SDA line is
>>> needed.  The GPIO is named "wakeup" and can be specified in a device
>>> tree with the "wakeup-gpios" binding.
>> Needs some i2c specialist input on this!  As you mentioned it is liable
>> to be controversial.  Wolfram, is this a one off special or do
>> any other devices do this sort of magic?
> 
> s/magic/insanity/ :)
> 
> I have never heard of something like this. Unsuprisingly, I can
> not recommend doing it this way. But we all know hardware...
> 
> And while we could think about reusing the bus_recovery_infrastructure,
> I don't think it is worth the hazzle. So, doing this GPIO fallback may
> well be the best we can do now.
> 
Fair enough.  Lets go with this approach then and hope this is the only bit
of hardware ever to do this (yeah right ;)

[PATCH 7/7] iio: mlx90614: Provide raw IR value for object channels

[Vianney le Clément de Saint-Marcq]

The unit and interpretation of the raw IR value is unspecified.

Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

---

This patch is a RFC.  The raw IR value could be used to implement custom
signal processing in userspace if sampled at a high enough rate.  Not
sure whether it is relevant for mainline though.
---
 drivers/iio/temperature/mlx90614.c | 40 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index 49c517a..b3771c5 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -214,6 +214,36 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static ssize_t mlx90614_read_raw_ir(struct iio_dev *indio_dev,
+				    uintptr_t priv,
+				    struct iio_chan_spec const *channel,
+				    char *buf)
+{
+	struct mlx90614_data *data = iio_priv(indio_dev);
+	u8 cmd;
+	s32 ret;
+
+	switch (channel->channel) {
+	case 0:
+		cmd = MLX90614_RAW1;
+		break;
+	case 1:
+		cmd = MLX90614_RAW2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mlx90614_power_get(data, true);
+	ret = i2c_smbus_read_word_data(data->client, cmd);
+	mlx90614_power_put(data);
+
+	if (ret < 0)
+		return ret;
+
+	return scnprintf(buf, PAGE_SIZE, "0x%04x\n", ret);
+}
+
 static ssize_t mlx90614_show_emissivity(struct device *dev,
 					struct device_attribute *devattr,
 					char *buf)
@@ -318,6 +348,14 @@ out:
 	return count;
 }
 
+static const struct iio_chan_spec_ext_info mlx90614_channel_obj_ext[] = {
+	{
+		.name = "raw_ir",
+		.read = mlx90614_read_raw_ir,
+	},
+	{ }
+};
+
 static const struct iio_chan_spec mlx90614_channels[] = {
 	{
 		.type = IIO_TEMP,
@@ -336,6 +374,7 @@ static const struct iio_chan_spec mlx90614_channels[] = {
 		    BIT(IIO_CHAN_INFO_PROCESSED),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
 		    BIT(IIO_CHAN_INFO_SCALE),
+		.ext_info = mlx90614_channel_obj_ext,
 	},
 	{
 		.type = IIO_TEMP,
@@ -358,6 +397,7 @@ static const struct iio_chan_spec mlx90614_channels[] = {
 		    BIT(IIO_CHAN_INFO_PROCESSED),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
 		    BIT(IIO_CHAN_INFO_SCALE),
+		.ext_info = mlx90614_channel_obj_ext,
 	},
 };
 
-- 
2.3.0