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

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-buIOx9BAs4sybS5Ee8rs3A@public.gmane.org>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout-agwphqFHkxc@public.gmane.org>
> 
> ---
> 
> 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-buIOx9BAs4sybS5Ee8rs3A@public.gmane.org>
>> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout-agwphqFHkxc@public.gmane.org>
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 ;)