[PATCH 0/5] iio: light: us5281d: Add power managmenet and interrupt support

[PATCH 0/5] iio: light: us5281d: Add power managmenet and interrupt support

[Adriana Reus]

The first patches add a dt property for choosing the power mode for raw reads.
The following adds helper functions preparing for pm support.
The last ones add power management and interrupt support.

Adriana Reus (5):
  iio: light: us5182d: Add property for choosing default power mode
  Documentation: devicetree: Add property for controlling power saving
    mode for the us5182 als sensor
  iio: light: us5182d: Add functions for selectively enabling als and
    proximity
  iio: light: us8152d: Add power management support
  iio: light: us5182d: Add interrupt support and events

 .../devicetree/bindings/iio/light/us5182d.txt      |  11 +
 drivers/iio/light/us5182d.c                        | 513 ++++++++++++++++++++-
 2 files changed, 504 insertions(+), 20 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] iio: light: us5182d: Add property for choosing default power mode

[Adriana Reus]

This chip supports two power modes.
1. "one-shot" mode - the chip activates and executes one complete
conversion loop and then shuts itself down. This is the default mode
chosen for raw reads.
2. "continuous" mode - the chip takes continuous measurements.

Continuous mode is more expensive power-wise but may be more reliable.
Add a property so that if preferred, the default power mode for raw
reads can be set to continuous.
Separate one-shot enabling in a separate function that will be used
depending on the chosen power mode. Also create a function for
powering the chip on and off.

Signed-off-by: Adriana Reus <adriana.reus@intel.com>
---
 drivers/iio/light/us5182d.c | 90 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index 49dab3c..7993014 100644
--- a/drivers/iio/light/us5182d.c
+++ b/drivers/iio/light/us5182d.c
@@ -99,6 +99,11 @@ enum mode {
 	US5182D_PX_ONLY
 };
 
+enum pmode {
+	US5182D_CONTINUOUS,
+	US5182D_ONESHOT
+};
+
 struct us5182d_data {
 	struct i2c_client *client;
 	struct mutex lock;
@@ -112,6 +117,9 @@ struct us5182d_data {
 	u16 *us5182d_dark_ths;
 
 	u8 opmode;
+	u8 power_mode;
+
+	bool default_continuous;
 };
 
 static IIO_CONST_ATTR(in_illuminance_scale_available,
@@ -130,13 +138,11 @@ static const struct {
 	u8 reg;
 	u8 val;
 } us5182d_regvals[] = {
-	{US5182D_REG_CFG0, (US5182D_CFG0_SHUTDOWN_EN |
-			    US5182D_CFG0_WORD_ENABLE)},
+	{US5182D_REG_CFG0, US5182D_CFG0_WORD_ENABLE},
 	{US5182D_REG_CFG1, US5182D_CFG1_ALS_RES16},
 	{US5182D_REG_CFG2, (US5182D_CFG2_PX_RES16 |
 			    US5182D_CFG2_PXGAIN_DEFAULT)},
 	{US5182D_REG_CFG3, US5182D_CFG3_LED_CURRENT100},
-	{US5182D_REG_MODE_STORE, US5182D_STORE_MODE},
 	{US5182D_REG_CFG4, 0x00},
 };
 
@@ -169,7 +175,7 @@ static int us5182d_get_als(struct us5182d_data *data)
 	return result;
 }
 
-static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
+static int us5182d_oneshot_en(struct us5182d_data *data)
 {
 	int ret;
 
@@ -182,6 +188,20 @@ static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
 	 * required measurement.
 	 */
 	ret = ret | US5182D_CFG0_ONESHOT_EN;
+	return i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);
+
+}
+
+static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
+{
+	int ret;
+
+	if (mode == data->opmode)
+		return 0;
+
+	ret = i2c_smbus_read_byte_data(data->client, US5182D_REG_CFG0);
+	if (ret < 0)
+		return ret;
 
 	/* update mode */
 	ret = ret & ~US5182D_OPMODE_MASK;
@@ -196,9 +216,6 @@ static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
 	if (ret < 0)
 		return ret;
 
-	if (mode == data->opmode)
-		return 0;
-
 	ret = i2c_smbus_write_byte_data(data->client, US5182D_REG_MODE_STORE,
 					US5182D_STORE_MODE);
 	if (ret < 0)
@@ -210,6 +227,23 @@ static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
 	return 0;
 }
 
+static int us5182d_shutdown_en (struct us5182d_data *data, u8 state)
+{
+	int ret;
+
+	if (data->power_mode == US5182D_ONESHOT)
+		return 0;
+
+	ret = i2c_smbus_read_byte_data(data->client, US5182D_REG_CFG0);
+	if (ret < 0)
+		return ret;
+
+	ret = ret & ~US5182D_CFG0_SHUTDOWN_EN;
+	ret = ret | state;
+
+	return i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);
+}
+
 static int us5182d_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long mask)
@@ -222,6 +256,11 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
 		switch (chan->type) {
 		case IIO_LIGHT:
 			mutex_lock(&data->lock);
+			if (data->power_mode == US5182D_ONESHOT) {
+				ret = us5182d_oneshot_en(data);
+				if (ret < 0)
+					goto out_err;
+			}
 			ret = us5182d_set_opmode(data, US5182D_OPMODE_ALS);
 			if (ret < 0)
 				goto out_err;
@@ -234,6 +273,11 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
 			return IIO_VAL_INT;
 		case IIO_PROXIMITY:
 			mutex_lock(&data->lock);
+			if (data->power_mode == US5182D_ONESHOT) {
+				ret = us5182d_oneshot_en(data);
+				if (ret < 0)
+					goto out_err;
+			}
 			ret = us5182d_set_opmode(data, US5182D_OPMODE_PX);
 			if (ret < 0)
 				goto out_err;
@@ -368,6 +412,7 @@ static int us5182d_init(struct iio_dev *indio_dev)
 		return ret;
 
 	data->opmode = 0;
+	data->power_mode = US5182D_CONTINUOUS;
 	for (i = 0; i < ARRAY_SIZE(us5182d_regvals); i++) {
 		ret = i2c_smbus_write_byte_data(data->client,
 						us5182d_regvals[i].reg,
@@ -376,7 +421,15 @@ static int us5182d_init(struct iio_dev *indio_dev)
 			return ret;
 	}
 
-	return 0;
+	if (!data->default_continuous) {
+		ret = us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
+		if (ret < 0)
+			return ret;
+		data->power_mode = US5182D_ONESHOT;
+	}
+
+
+	return ret;
 }
 
 static void us5182d_get_platform_data(struct iio_dev *indio_dev)
@@ -399,6 +452,8 @@ static void us5182d_get_platform_data(struct iio_dev *indio_dev)
 				    "upisemi,lower-dark-gain",
 				    &data->lower_dark_gain))
 		data->lower_dark_gain = US5182D_REG_AUTO_LDARK_GAIN_DEFAULT;
+	data->default_continuous = device_property_read_bool(&data->client->dev,
+							     "upisemi,continuous");
 }
 
 static int  us5182d_dark_gain_config(struct iio_dev *indio_dev)
@@ -464,16 +519,27 @@ static int us5182d_probe(struct i2c_client *client,
 
 	ret = us5182d_dark_gain_config(indio_dev);
 	if (ret < 0)
-		return ret;
+		goto out_err;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0)
+		goto out_err;
+
+	return 0;
+
+out_err:
+	us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
+	return ret;
 
-	return iio_device_register(indio_dev);
 }
 
 static int us5182d_remove(struct i2c_client *client)
 {
+	struct us5182d_data *data = iio_priv(i2c_get_clientdata(client));
+
 	iio_device_unregister(i2c_get_clientdata(client));
-	return i2c_smbus_write_byte_data(client, US5182D_REG_CFG0,
-					 US5182D_CFG0_SHUTDOWN_EN);
+
+	return us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
 }
 
 static const struct acpi_device_id us5182d_acpi_match[] = {
-- 
1.9.1

Re: [PATCH 1/5] iio: light: us5182d: Add property for choosing default power mode

[Jonathan Cameron]

On 24/11/15 10:59, Adriana Reus wrote:
> This chip supports two power modes.
> 1. "one-shot" mode - the chip activates and executes one complete
> conversion loop and then shuts itself down. This is the default mode
> chosen for raw reads.
> 2. "continuous" mode - the chip takes continuous measurements.
> 
> Continuous mode is more expensive power-wise but may be more reliable.
> Add a property so that if preferred, the default power mode for raw
> reads can be set to continuous.
> Separate one-shot enabling in a separate function that will be used
> depending on the chosen power mode. Also create a function for
> powering the chip on and off.
> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
A couple of nitpicks inline...  I've fixed them up when applying to the
togreg branch of iio.git - initially pushed out as testing for the autobuilders
to play with them.

Thanks,

Jonathan
> ---
>  drivers/iio/light/us5182d.c | 90 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 78 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
> index 49dab3c..7993014 100644
> --- a/drivers/iio/light/us5182d.c
> +++ b/drivers/iio/light/us5182d.c
> @@ -99,6 +99,11 @@ enum mode {
>  	US5182D_PX_ONLY
>  };
>  
> +enum pmode {
> +	US5182D_CONTINUOUS,
> +	US5182D_ONESHOT
> +};
> +
>  struct us5182d_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
> @@ -112,6 +117,9 @@ struct us5182d_data {
>  	u16 *us5182d_dark_ths;
>  
>  	u8 opmode;
> +	u8 power_mode;
> +
> +	bool default_continuous;
>  };
>  
>  static IIO_CONST_ATTR(in_illuminance_scale_available,
> @@ -130,13 +138,11 @@ static const struct {
>  	u8 reg;
>  	u8 val;
>  } us5182d_regvals[] = {
> -	{US5182D_REG_CFG0, (US5182D_CFG0_SHUTDOWN_EN |
> -			    US5182D_CFG0_WORD_ENABLE)},
> +	{US5182D_REG_CFG0, US5182D_CFG0_WORD_ENABLE},
>  	{US5182D_REG_CFG1, US5182D_CFG1_ALS_RES16},
>  	{US5182D_REG_CFG2, (US5182D_CFG2_PX_RES16 |
>  			    US5182D_CFG2_PXGAIN_DEFAULT)},
>  	{US5182D_REG_CFG3, US5182D_CFG3_LED_CURRENT100},
> -	{US5182D_REG_MODE_STORE, US5182D_STORE_MODE},
>  	{US5182D_REG_CFG4, 0x00},
>  };
>  
> @@ -169,7 +175,7 @@ static int us5182d_get_als(struct us5182d_data *data)
>  	return result;
>  }
>  
> -static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
> +static int us5182d_oneshot_en(struct us5182d_data *data)
>  {
>  	int ret;
>  
> @@ -182,6 +188,20 @@ static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
>  	 * required measurement.
>  	 */
>  	ret = ret | US5182D_CFG0_ONESHOT_EN;
> +	return i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);
Unwanted extra blank line here...
> +
> +}
> +
> +static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
> +{
> +	int ret;
> +
> +	if (mode == data->opmode)
> +		return 0;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, US5182D_REG_CFG0);
> +	if (ret < 0)
> +		return ret;
>  
>  	/* update mode */
>  	ret = ret & ~US5182D_OPMODE_MASK;
> @@ -196,9 +216,6 @@ static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (mode == data->opmode)
> -		return 0;
> -
>  	ret = i2c_smbus_write_byte_data(data->client, US5182D_REG_MODE_STORE,
>  					US5182D_STORE_MODE);
>  	if (ret < 0)
> @@ -210,6 +227,23 @@ static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
>  	return 0;
>  }
>  
> +static int us5182d_shutdown_en (struct us5182d_data *data, u8 state)
Stray space above.. Checkpatch would have highlighted this for you, please
do run it as a sanity check before you send a series out.

> +{
> +	int ret;
> +
> +	if (data->power_mode == US5182D_ONESHOT)
> +		return 0;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, US5182D_REG_CFG0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ret & ~US5182D_CFG0_SHUTDOWN_EN;
> +	ret = ret | state;
> +
> +	return i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);
> +}
> +
>  static int us5182d_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int *val,
>  			    int *val2, long mask)
> @@ -222,6 +256,11 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
>  		switch (chan->type) {
>  		case IIO_LIGHT:
>  			mutex_lock(&data->lock);
> +			if (data->power_mode == US5182D_ONESHOT) {
> +				ret = us5182d_oneshot_en(data);
> +				if (ret < 0)
> +					goto out_err;
> +			}
>  			ret = us5182d_set_opmode(data, US5182D_OPMODE_ALS);
>  			if (ret < 0)
>  				goto out_err;
> @@ -234,6 +273,11 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
>  			return IIO_VAL_INT;
>  		case IIO_PROXIMITY:
>  			mutex_lock(&data->lock);
> +			if (data->power_mode == US5182D_ONESHOT) {
> +				ret = us5182d_oneshot_en(data);
> +				if (ret < 0)
> +					goto out_err;
> +			}
>  			ret = us5182d_set_opmode(data, US5182D_OPMODE_PX);
>  			if (ret < 0)
>  				goto out_err;
> @@ -368,6 +412,7 @@ static int us5182d_init(struct iio_dev *indio_dev)
>  		return ret;
>  
>  	data->opmode = 0;
> +	data->power_mode = US5182D_CONTINUOUS;
>  	for (i = 0; i < ARRAY_SIZE(us5182d_regvals); i++) {
>  		ret = i2c_smbus_write_byte_data(data->client,
>  						us5182d_regvals[i].reg,
> @@ -376,7 +421,15 @@ static int us5182d_init(struct iio_dev *indio_dev)
>  			return ret;
>  	}
>  
> -	return 0;
> +	if (!data->default_continuous) {
> +		ret = us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
> +		if (ret < 0)
> +			return ret;
> +		data->power_mode = US5182D_ONESHOT;
> +	}
> +
> +
> +	return ret;
>  }
>  
>  static void us5182d_get_platform_data(struct iio_dev *indio_dev)
> @@ -399,6 +452,8 @@ static void us5182d_get_platform_data(struct iio_dev *indio_dev)
>  				    "upisemi,lower-dark-gain",
>  				    &data->lower_dark_gain))
>  		data->lower_dark_gain = US5182D_REG_AUTO_LDARK_GAIN_DEFAULT;
> +	data->default_continuous = device_property_read_bool(&data->client->dev,
> +							     "upisemi,continuous");
>  }
>  
>  static int  us5182d_dark_gain_config(struct iio_dev *indio_dev)
> @@ -464,16 +519,27 @@ static int us5182d_probe(struct i2c_client *client,
>  
>  	ret = us5182d_dark_gain_config(indio_dev);
>  	if (ret < 0)
> -		return ret;
> +		goto out_err;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0)
> +		goto out_err;
> +
> +	return 0;
> +
> +out_err:
> +	us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
> +	return ret;
>  
> -	return iio_device_register(indio_dev);
>  }
>  
>  static int us5182d_remove(struct i2c_client *client)
>  {
> +	struct us5182d_data *data = iio_priv(i2c_get_clientdata(client));
> +
>  	iio_device_unregister(i2c_get_clientdata(client));
> -	return i2c_smbus_write_byte_data(client, US5182D_REG_CFG0,
> -					 US5182D_CFG0_SHUTDOWN_EN);
> +
> +	return us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
>  }
>  
>  static const struct acpi_device_id us5182d_acpi_match[] = {
> 

[PATCH 2/5] Documentation: devicetree: Add property for controlling power saving mode for the us5182 als sensor

[Adriana Reus]

Add a property to allow changing the default power-saving mode.
By default, at read raw the chip will activate and provide
one measurent, then it will shut itself down. However, the
chip can also work in "continuous" mode which may be more reliable
but is also more power consuming.

Signed-off-by: Adriana Reus <adriana.reus@intel.com>
---
 Documentation/devicetree/bindings/iio/light/us5182d.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/us5182d.txt b/Documentation/devicetree/bindings/iio/light/us5182d.txt
index 6f0a530..a619799 100644
--- a/Documentation/devicetree/bindings/iio/light/us5182d.txt
+++ b/Documentation/devicetree/bindings/iio/light/us5182d.txt
@@ -7,13 +7,24 @@ Required properties:
 Optional properties:
 - upisemi,glass-coef: glass attenuation factor - compensation factor of
                       resolution 1000 for material transmittance.
+
 - upisemi,dark-ths: array of 8 elements containing 16-bit thresholds (adc
                     counts) corresponding to every scale.
+
 - upisemi,upper-dark-gain: 8-bit dark gain compensation factor(4 int and 4
                            fractional bits - Q4.4) applied when light > threshold
+
 - upisemi,lower-dark-gain: 8-bit dark gain compensation factor(4 int and 4
                            fractional bits - Q4.4) applied when light < threshold
 
+- upisemi,continuous: This chip has two power modes: one-shot (chip takes one
+                      measurement and then shuts itself down) and continuous (
+                      chip takes continuous measurements). The one-shot mode is
+                      more power-friendly but the continuous mode may be more
+                      reliable. If this property is specified the continuous
+                      mode will be used instead of the default one-shot one for
+                      raw reads.
+
 If the optional properties are not specified these factors will default to the
 values in the below example.
 The glass-coef defaults to no compensation for the covering material.
-- 
1.9.1

Re: [PATCH 2/5] Documentation: devicetree: Add property for controlling power saving mode for the us5182 als sensor

[Rob Herring]

On Tue, Nov 24, 2015 at 12:59:49PM +0200, Adriana Reus wrote:
> Add a property to allow changing the default power-saving mode.
> By default, at read raw the chip will activate and provide
> one measurent, then it will shut itself down. However, the
> chip can also work in "continuous" mode which may be more reliable
> but is also more power consuming.
> 
> Signed-off-by: Adriana Reus <adriana.reus-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/iio/light/us5182d.txt | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/us5182d.txt b/Documentation/devicetree/bindings/iio/light/us5182d.txt
> index 6f0a530..a619799 100644
> --- a/Documentation/devicetree/bindings/iio/light/us5182d.txt
> +++ b/Documentation/devicetree/bindings/iio/light/us5182d.txt
> @@ -7,13 +7,24 @@ Required properties:
>  Optional properties:
>  - upisemi,glass-coef: glass attenuation factor - compensation factor of
>                        resolution 1000 for material transmittance.
> +
>  - upisemi,dark-ths: array of 8 elements containing 16-bit thresholds (adc
>                      counts) corresponding to every scale.
> +
>  - upisemi,upper-dark-gain: 8-bit dark gain compensation factor(4 int and 4
>                             fractional bits - Q4.4) applied when light > threshold
> +
>  - upisemi,lower-dark-gain: 8-bit dark gain compensation factor(4 int and 4
>                             fractional bits - Q4.4) applied when light < threshold
>  
> +- upisemi,continuous: This chip has two power modes: one-shot (chip takes one
> +                      measurement and then shuts itself down) and continuous (
> +                      chip takes continuous measurements). The one-shot mode is
> +                      more power-friendly but the continuous mode may be more
> +                      reliable. If this property is specified the continuous
> +                      mode will be used instead of the default one-shot one for
> +                      raw reads.

I could imagine an OS may want to decide this on its own or use a 
mixture of the modes.

Rob

Re: [PATCH 2/5] Documentation: devicetree: Add property for controlling power saving mode for the us5182 als sensor

[Adriana Reus]

On 25.11.2015 02:01, Rob Herring wrote:
> On Tue, Nov 24, 2015 at 12:59:49PM +0200, Adriana Reus wrote:
>> Add a property to allow changing the default power-saving mode.
>> By default, at read raw the chip will activate and provide
>> one measurent, then it will shut itself down. However, the
>> chip can also work in "continuous" mode which may be more reliable
>> but is also more power consuming.
>>
>> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
>> ---
>>   Documentation/devicetree/bindings/iio/light/us5182d.txt | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/light/us5182d.txt b/Documentation/devicetree/bindings/iio/light/us5182d.txt
>> index 6f0a530..a619799 100644
>> --- a/Documentation/devicetree/bindings/iio/light/us5182d.txt
>> +++ b/Documentation/devicetree/bindings/iio/light/us5182d.txt
>> @@ -7,13 +7,24 @@ Required properties:
>>   Optional properties:
>>   - upisemi,glass-coef: glass attenuation factor - compensation factor of
>>                         resolution 1000 for material transmittance.
>> +
>>   - upisemi,dark-ths: array of 8 elements containing 16-bit thresholds (adc
>>                       counts) corresponding to every scale.
>> +
>>   - upisemi,upper-dark-gain: 8-bit dark gain compensation factor(4 int and 4
>>                              fractional bits - Q4.4) applied when light > threshold
>> +
>>   - upisemi,lower-dark-gain: 8-bit dark gain compensation factor(4 int and 4
>>                              fractional bits - Q4.4) applied when light < threshold
>>
>> +- upisemi,continuous: This chip has two power modes: one-shot (chip takes one
>> +                      measurement and then shuts itself down) and continuous (
>> +                      chip takes continuous measurements). The one-shot mode is
>> +                      more power-friendly but the continuous mode may be more
>> +                      reliable. If this property is specified the continuous
>> +                      mode will be used instead of the default one-shot one for
>> +                      raw reads.
>
> I could imagine an OS may want to decide this on its own or use a
> mixture of the modes.
>
> Rob
>

There is no possibility of mixing them up (at the same time), so for 
example proximity cannot work in one mode and als the other.

The one-shot mode can only be used for raw reads (for example when 
user-space polls in_[proximity|light]_raw). If user-space wants to 
enable events (activate interrupts when certain thresholds are met - 
patch 5 of the series), then the chip has to switch to continuous 
nonetheless because it needs to be active all the time. So one work-flow 
scenario would be:

Consumer1 starts polling the raw interface - default_mode
Consumer2 activates events - continuous mode
Consumer2 deactivates events - back to default_mode

The only choice here is the default mode for raw reads, it currently is
one-shot, this patch allows for continuous to be used if preferred.

Re: [PATCH 2/5] Documentation: devicetree: Add property for controlling power saving mode for the us5182 als sensor

[Rob Herring]

On Wed, Nov 25, 2015 at 11:50:30AM +0200, Adriana Reus wrote:
> 
> 
> On 25.11.2015 02:01, Rob Herring wrote:
> >On Tue, Nov 24, 2015 at 12:59:49PM +0200, Adriana Reus wrote:
> >>Add a property to allow changing the default power-saving mode.
> >>By default, at read raw the chip will activate and provide
> >>one measurent, then it will shut itself down. However, the
> >>chip can also work in "continuous" mode which may be more reliable
> >>but is also more power consuming.
> >>
> >>Signed-off-by: Adriana Reus <adriana.reus-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>---
> >>  Documentation/devicetree/bindings/iio/light/us5182d.txt | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >>diff --git a/Documentation/devicetree/bindings/iio/light/us5182d.txt b/Documentation/devicetree/bindings/iio/light/us5182d.txt
> >>index 6f0a530..a619799 100644
> >>--- a/Documentation/devicetree/bindings/iio/light/us5182d.txt
> >>+++ b/Documentation/devicetree/bindings/iio/light/us5182d.txt
> >>@@ -7,13 +7,24 @@ Required properties:
> >>  Optional properties:
> >>  - upisemi,glass-coef: glass attenuation factor - compensation factor of
> >>                        resolution 1000 for material transmittance.
> >>+
> >>  - upisemi,dark-ths: array of 8 elements containing 16-bit thresholds (adc
> >>                      counts) corresponding to every scale.
> >>+
> >>  - upisemi,upper-dark-gain: 8-bit dark gain compensation factor(4 int and 4
> >>                             fractional bits - Q4.4) applied when light > threshold
> >>+
> >>  - upisemi,lower-dark-gain: 8-bit dark gain compensation factor(4 int and 4
> >>                             fractional bits - Q4.4) applied when light < threshold
> >>
> >>+- upisemi,continuous: This chip has two power modes: one-shot (chip takes one
> >>+                      measurement and then shuts itself down) and continuous (
> >>+                      chip takes continuous measurements). The one-shot mode is
> >>+                      more power-friendly but the continuous mode may be more
> >>+                      reliable. If this property is specified the continuous
> >>+                      mode will be used instead of the default one-shot one for
> >>+                      raw reads.
> >
> >I could imagine an OS may want to decide this on its own or use a
> >mixture of the modes.
> >
> >Rob
> >
> 
> There is no possibility of mixing them up (at the same time), so for example
> proximity cannot work in one mode and als the other.
> 
> The one-shot mode can only be used for raw reads (for example when
> user-space polls in_[proximity|light]_raw). If user-space wants to enable
> events (activate interrupts when certain thresholds are met - patch 5 of the
> series), then the chip has to switch to continuous nonetheless because it
> needs to be active all the time. So one work-flow scenario would be:
> 
> Consumer1 starts polling the raw interface - default_mode
> Consumer2 activates events - continuous mode
> Consumer2 deactivates events - back to default_mode
> 
> The only choice here is the default mode for raw reads, it currently is
> one-shot, this patch allows for continuous to be used if preferred.

Okay, then:

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] Documentation: devicetree: Add property for controlling power saving mode for the us5182 als sensor

[Jonathan Cameron]

On 25/11/15 23:55, Rob Herring wrote:
> On Wed, Nov 25, 2015 at 11:50:30AM +0200, Adriana Reus wrote:
>>
>>
>> On 25.11.2015 02:01, Rob Herring wrote:
>>> On Tue, Nov 24, 2015 at 12:59:49PM +0200, Adriana Reus wrote:
>>>> Add a property to allow changing the default power-saving mode.
>>>> By default, at read raw the chip will activate and provide
>>>> one measurent, then it will shut itself down. However, the
>>>> chip can also work in "continuous" mode which may be more reliable
>>>> but is also more power consuming.
>>>>
>>>> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/iio/light/us5182d.txt | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/light/us5182d.txt b/Documentation/devicetree/bindings/iio/light/us5182d.txt
>>>> index 6f0a530..a619799 100644
>>>> --- a/Documentation/devicetree/bindings/iio/light/us5182d.txt
>>>> +++ b/Documentation/devicetree/bindings/iio/light/us5182d.txt
>>>> @@ -7,13 +7,24 @@ Required properties:
>>>>  Optional properties:
>>>>  - upisemi,glass-coef: glass attenuation factor - compensation factor of
>>>>                        resolution 1000 for material transmittance.
>>>> +
>>>>  - upisemi,dark-ths: array of 8 elements containing 16-bit thresholds (adc
>>>>                      counts) corresponding to every scale.
>>>> +
>>>>  - upisemi,upper-dark-gain: 8-bit dark gain compensation factor(4 int and 4
>>>>                             fractional bits - Q4.4) applied when light > threshold
>>>> +
>>>>  - upisemi,lower-dark-gain: 8-bit dark gain compensation factor(4 int and 4
>>>>                             fractional bits - Q4.4) applied when light < threshold
>>>>
>>>> +- upisemi,continuous: This chip has two power modes: one-shot (chip takes one
>>>> +                      measurement and then shuts itself down) and continuous (
>>>> +                      chip takes continuous measurements). The one-shot mode is
>>>> +                      more power-friendly but the continuous mode may be more
>>>> +                      reliable. If this property is specified the continuous
>>>> +                      mode will be used instead of the default one-shot one for
>>>> +                      raw reads.
>>>
>>> I could imagine an OS may want to decide this on its own or use a
>>> mixture of the modes.
>>>
>>> Rob
>>>
>>
>> There is no possibility of mixing them up (at the same time), so for example
>> proximity cannot work in one mode and als the other.
>>
>> The one-shot mode can only be used for raw reads (for example when
>> user-space polls in_[proximity|light]_raw). If user-space wants to enable
>> events (activate interrupts when certain thresholds are met - patch 5 of the
>> series), then the chip has to switch to continuous nonetheless because it
>> needs to be active all the time. So one work-flow scenario would be:
>>
>> Consumer1 starts polling the raw interface - default_mode
>> Consumer2 activates events - continuous mode
>> Consumer2 deactivates events - back to default_mode
>>
>> The only choice here is the default mode for raw reads, it currently is
>> one-shot, this patch allows for continuous to be used if preferred.
> 
> Okay, then:
> 
> Acked-by: Rob Herring <robh@kernel.org>
Applied to the togreg branch of iio.git - initially pushed out as testing.

Thanks,

Jonathan
> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 3/5] iio: light: us5182d: Add functions for selectively enabling als and proximity

[Adriana Reus]

Keep track of the als and px enabled/disabled status in
order to enable them selectively.

Signed-off-by: Adriana Reus <adriana.reus-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/iio/light/us5182d.c | 64 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index 7993014..3d959f3 100644
--- a/drivers/iio/light/us5182d.c
+++ b/drivers/iio/light/us5182d.c
@@ -119,6 +119,9 @@ struct us5182d_data {
 	u8 opmode;
 	u8 power_mode;
 
+	bool als_enabled;
+	bool px_enabled;
+
 	bool default_continuous;
 };
 
@@ -227,6 +230,48 @@ static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
 	return 0;
 }
 
+static int us5182d_als_enable(struct us5182d_data *data)
+{
+	int ret;
+	u8 mode;
+
+	if (data->power_mode == US5182D_ONESHOT)
+		return us5182d_set_opmode(data, US5182D_ALS_ONLY);
+
+	if (data->als_enabled)
+		return 0;
+
+	mode = data->px_enabled ? US5182D_ALS_PX : US5182D_ALS_ONLY;
+
+	ret = us5182d_set_opmode(data, mode);
+	if (ret < 0)
+		return ret;
+
+	data->als_enabled = true;
+	return 0;
+}
+
+static int us5182d_px_enable(struct us5182d_data *data)
+{
+	int ret;
+	u8 mode;
+
+	if (data->power_mode == US5182D_ONESHOT)
+		return us5182d_set_opmode(data, US5182D_PX_ONLY);
+
+	if (data->px_enabled)
+		return 0;
+
+	mode = data->als_enabled ? US5182D_ALS_PX : US5182D_PX_ONLY;
+
+	ret = us5182d_set_opmode(data, mode);
+	if (ret < 0)
+		return ret;
+
+	data->px_enabled = true;
+	return 0;
+}
+
 static int us5182d_shutdown_en (struct us5182d_data *data, u8 state)
 {
 	int ret;
@@ -241,7 +286,16 @@ static int us5182d_shutdown_en (struct us5182d_data *data, u8 state)
 	ret = ret & ~US5182D_CFG0_SHUTDOWN_EN;
 	ret = ret | state;
 
-	return i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);
+	ret = i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);
+	if (ret < 0)
+		return ret;
+
+	if (state & US5182D_CFG0_SHUTDOWN_EN) {
+		data->als_enabled = false;
+		data->px_enabled = false;
+	}
+
+	return ret;
 }
 
 static int us5182d_read_raw(struct iio_dev *indio_dev,
@@ -261,7 +315,7 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
 				if (ret < 0)
 					goto out_err;
 			}
-			ret = us5182d_set_opmode(data, US5182D_OPMODE_ALS);
+			ret = us5182d_als_enable(data);
 			if (ret < 0)
 				goto out_err;
 
@@ -278,7 +332,7 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
 				if (ret < 0)
 					goto out_err;
 			}
-			ret = us5182d_set_opmode(data, US5182D_OPMODE_PX);
+			ret = us5182d_px_enable(data);
 			if (ret < 0)
 				goto out_err;
 
@@ -421,6 +475,9 @@ static int us5182d_init(struct iio_dev *indio_dev)
 			return ret;
 	}
 
+	data->als_enabled = true;
+	data->px_enabled = true;
+
 	if (!data->default_continuous) {
 		ret = us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
 		if (ret < 0)
@@ -428,7 +485,6 @@ static int us5182d_init(struct iio_dev *indio_dev)
 		data->power_mode = US5182D_ONESHOT;
 	}
 
-
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH 3/5] iio: light: us5182d: Add functions for selectively enabling als and proximity

[Jonathan Cameron]

On 24/11/15 10:59, Adriana Reus wrote:
> Keep track of the als and px enabled/disabled status in
> order to enable them selectively.
> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
Couple more nitpicks, but again fixed up during apply.
> ---
>  drivers/iio/light/us5182d.c | 64 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
> index 7993014..3d959f3 100644
> --- a/drivers/iio/light/us5182d.c
> +++ b/drivers/iio/light/us5182d.c
> @@ -119,6 +119,9 @@ struct us5182d_data {
>  	u8 opmode;
>  	u8 power_mode;
>  
> +	bool als_enabled;
> +	bool px_enabled;
> +
>  	bool default_continuous;
>  };
>  
> @@ -227,6 +230,48 @@ static int us5182d_set_opmode(struct us5182d_data *data, u8 mode)
>  	return 0;
>  }
>  
> +static int us5182d_als_enable(struct us5182d_data *data)
> +{
> +	int ret;
> +	u8 mode;
> +
> +	if (data->power_mode == US5182D_ONESHOT)
> +		return us5182d_set_opmode(data, US5182D_ALS_ONLY);
> +
> +	if (data->als_enabled)
> +		return 0;
> +
> +	mode = data->px_enabled ? US5182D_ALS_PX : US5182D_ALS_ONLY;
> +
> +	ret = us5182d_set_opmode(data, mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->als_enabled = true;
Blank line here ideally.
> +	return 0;
> +}
> +
> +static int us5182d_px_enable(struct us5182d_data *data)
> +{
> +	int ret;
> +	u8 mode;
> +
> +	if (data->power_mode == US5182D_ONESHOT)
> +		return us5182d_set_opmode(data, US5182D_PX_ONLY);
> +
> +	if (data->px_enabled)
> +		return 0;
> +
> +	mode = data->als_enabled ? US5182D_ALS_PX : US5182D_PX_ONLY;
> +
> +	ret = us5182d_set_opmode(data, mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->px_enabled = true;
And here...
> +	return 0;
> +}
> +
>  static int us5182d_shutdown_en (struct us5182d_data *data, u8 state)
>  {
>  	int ret;
> @@ -241,7 +286,16 @@ static int us5182d_shutdown_en (struct us5182d_data *data, u8 state)
>  	ret = ret & ~US5182D_CFG0_SHUTDOWN_EN;
>  	ret = ret | state;
>  
> -	return i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);
> +	ret = i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (state & US5182D_CFG0_SHUTDOWN_EN) {
> +		data->als_enabled = false;
> +		data->px_enabled = false;
> +	}
> +
> +	return ret;
>  }
>  
>  static int us5182d_read_raw(struct iio_dev *indio_dev,
> @@ -261,7 +315,7 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
>  				if (ret < 0)
>  					goto out_err;
>  			}
> -			ret = us5182d_set_opmode(data, US5182D_OPMODE_ALS);
> +			ret = us5182d_als_enable(data);
>  			if (ret < 0)
>  				goto out_err;
>  
> @@ -278,7 +332,7 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
>  				if (ret < 0)
>  					goto out_err;
>  			}
> -			ret = us5182d_set_opmode(data, US5182D_OPMODE_PX);
> +			ret = us5182d_px_enable(data);
>  			if (ret < 0)
>  				goto out_err;
>  
> @@ -421,6 +475,9 @@ static int us5182d_init(struct iio_dev *indio_dev)
>  			return ret;
>  	}
>  
> +	data->als_enabled = true;
> +	data->px_enabled = true;
> +
>  	if (!data->default_continuous) {
>  		ret = us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
>  		if (ret < 0)
> @@ -428,7 +485,6 @@ static int us5182d_init(struct iio_dev *indio_dev)
>  		data->power_mode = US5182D_ONESHOT;
>  	}
>  
> -
>  	return ret;
>  }
>  
> 

[PATCH 4/5] iio: light: us8152d: Add power management support

[Adriana Reus]

Add power management for sleep as well as runtime pm.

Signed-off-by: Adriana Reus <adriana.reus@intel.com>
---
 drivers/iio/light/us5182d.c | 95 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 88 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index 3d959f3..60cab4a 100644
--- a/drivers/iio/light/us5182d.c
+++ b/drivers/iio/light/us5182d.c
@@ -23,6 +23,8 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
 
 #define US5182D_REG_CFG0				0x00
 #define US5182D_CFG0_ONESHOT_EN				BIT(6)
@@ -81,6 +83,7 @@
 #define US5182D_READ_BYTE			1
 #define US5182D_READ_WORD			2
 #define US5182D_OPSTORE_SLEEP_TIME		20 /* ms */
+#define US5182D_SLEEP_MS			3000 /* ms */
 
 /* Available ranges: [12354, 7065, 3998, 2202, 1285, 498, 256, 138] lux */
 static const int us5182d_scales[] = {188500, 107800, 61000, 33600, 19600, 7600,
@@ -298,6 +301,26 @@ static int us5182d_shutdown_en (struct us5182d_data *data, u8 state)
 	return ret;
 }
 
+
+static int us5182d_set_power_state(struct us5182d_data *data, bool on)
+{
+	int ret;
+
+	if (data->power_mode == US5182D_ONESHOT)
+		return 0;
+
+	if (on) {
+		ret = pm_runtime_get_sync(&data->client->dev);
+		if (ret < 0)
+			pm_runtime_put_noidle(&data->client->dev);
+	} else {
+		pm_runtime_mark_last_busy(&data->client->dev);
+		ret = pm_runtime_put_autosuspend(&data->client->dev);
+	}
+
+	return ret;
+}
+
 static int us5182d_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long mask)
@@ -315,15 +338,20 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
 				if (ret < 0)
 					goto out_err;
 			}
-			ret = us5182d_als_enable(data);
+			ret = us5182d_set_power_state(data, true);
 			if (ret < 0)
 				goto out_err;
-
+			ret = us5182d_als_enable(data);
+			if (ret < 0)
+				goto out_poweroff;
 			ret = us5182d_get_als(data);
 			if (ret < 0)
+				goto out_poweroff;
+			*val = ret;
+			ret = us5182d_set_power_state(data, false);
+			if (ret < 0)
 				goto out_err;
 			mutex_unlock(&data->lock);
-			*val = ret;
 			return IIO_VAL_INT;
 		case IIO_PROXIMITY:
 			mutex_lock(&data->lock);
@@ -332,17 +360,22 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
 				if (ret < 0)
 					goto out_err;
 			}
-			ret = us5182d_px_enable(data);
+			ret = us5182d_set_power_state(data, true);
 			if (ret < 0)
 				goto out_err;
-
+			ret = us5182d_px_enable(data);
+			if (ret < 0)
+				goto out_poweroff;
 			ret = i2c_smbus_read_word_data(data->client,
 						       US5182D_REG_PDL);
 			if (ret < 0)
+				goto out_poweroff;
+			*val = ret;
+			ret = us5182d_set_power_state(data, false);
+			if (ret < 0)
 				goto out_err;
 			mutex_unlock(&data->lock);
-			*val = ret;
-			return  IIO_VAL_INT;
+			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
 		}
@@ -361,6 +394,9 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
 	}
 
 	return -EINVAL;
+
+out_poweroff:
+	us5182d_set_power_state(data, false);
 out_err:
 	mutex_unlock(&data->lock);
 	return ret;
@@ -577,6 +613,17 @@ static int us5182d_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto out_err;
 
+	if (data->default_continuous) {
+		pm_runtime_set_active(&client->dev);
+		if (ret < 0)
+			goto out_err;
+	}
+
+	pm_runtime_enable(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev,
+					 US5182D_SLEEP_MS);
+	pm_runtime_use_autosuspend(&client->dev);
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
 		goto out_err;
@@ -595,9 +642,42 @@ static int us5182d_remove(struct i2c_client *client)
 
 	iio_device_unregister(i2c_get_clientdata(client));
 
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+
 	return us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
 }
 
+#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM)
+static int us5182d_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct us5182d_data *data = iio_priv(indio_dev);
+
+	if (data->power_mode == US5182D_CONTINUOUS)
+		return us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
+
+	return 0;
+}
+
+static int us5182d_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct us5182d_data *data = iio_priv(indio_dev);
+
+	if (data->power_mode == US5182D_CONTINUOUS)
+		return us5182d_shutdown_en(data,
+					   ~US5182D_CFG0_SHUTDOWN_EN & 0xff);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops us5182d_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(us5182d_suspend, us5182d_resume)
+	SET_RUNTIME_PM_OPS(us5182d_suspend, us5182d_resume, NULL)
+};
+
 static const struct acpi_device_id us5182d_acpi_match[] = {
 	{ "USD5182", 0},
 	{}
@@ -615,6 +695,7 @@ MODULE_DEVICE_TABLE(i2c, us5182d_id);
 static struct i2c_driver us5182d_driver = {
 	.driver = {
 		.name = US5182D_DRV_NAME,
+		.pm = &us5182d_pm_ops,
 		.acpi_match_table = ACPI_PTR(us5182d_acpi_match),
 	},
 	.probe = us5182d_probe,
-- 
1.9.1

Re: [PATCH 4/5] iio: light: us8152d: Add power management support

[Jonathan Cameron]

On 24/11/15 10:59, Adriana Reus wrote:
> Add power management for sleep as well as runtime pm.
> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
Mostly fine, but a comment on a possible future tidy up inline.

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

> ---
>  drivers/iio/light/us5182d.c | 95 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 88 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
> index 3d959f3..60cab4a 100644
> --- a/drivers/iio/light/us5182d.c
> +++ b/drivers/iio/light/us5182d.c
> @@ -23,6 +23,8 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
>  
>  #define US5182D_REG_CFG0				0x00
>  #define US5182D_CFG0_ONESHOT_EN				BIT(6)
> @@ -81,6 +83,7 @@
>  #define US5182D_READ_BYTE			1
>  #define US5182D_READ_WORD			2
>  #define US5182D_OPSTORE_SLEEP_TIME		20 /* ms */
> +#define US5182D_SLEEP_MS			3000 /* ms */
>  
>  /* Available ranges: [12354, 7065, 3998, 2202, 1285, 498, 256, 138] lux */
>  static const int us5182d_scales[] = {188500, 107800, 61000, 33600, 19600, 7600,
> @@ -298,6 +301,26 @@ static int us5182d_shutdown_en (struct us5182d_data *data, u8 state)
>  	return ret;
>  }
>  
> +
> +static int us5182d_set_power_state(struct us5182d_data *data, bool on)
> +{
> +	int ret;
> +
> +	if (data->power_mode == US5182D_ONESHOT)
> +		return 0;
> +
> +	if (on) {
> +		ret = pm_runtime_get_sync(&data->client->dev);
> +		if (ret < 0)
> +			pm_runtime_put_noidle(&data->client->dev);
> +	} else {
> +		pm_runtime_mark_last_busy(&data->client->dev);
> +		ret = pm_runtime_put_autosuspend(&data->client->dev);
> +	}
> +
> +	return ret;
> +}
> +
>  static int us5182d_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int *val,
>  			    int *val2, long mask)
> @@ -315,15 +338,20 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
>  				if (ret < 0)
>  					goto out_err;
>  			}
> -			ret = us5182d_als_enable(data);
> +			ret = us5182d_set_power_state(data, true);
>  			if (ret < 0)
>  				goto out_err;
> -
> +			ret = us5182d_als_enable(data);
> +			if (ret < 0)
> +				goto out_poweroff;
>  			ret = us5182d_get_als(data);
>  			if (ret < 0)
> +				goto out_poweroff;
> +			*val = ret;
> +			ret = us5182d_set_power_state(data, false);
> +			if (ret < 0)
>  				goto out_err;
>  			mutex_unlock(&data->lock);
> -			*val = ret;
>  			return IIO_VAL_INT;
>  		case IIO_PROXIMITY:
I'd argue the complexity of this has reached the point where ideally you'd
break it out to a function.  The jumps out of the switch statement are 
particularly nasty from a readability point of view.

Perhaps a job for a followup patch?
>  			mutex_lock(&data->lock);
> @@ -332,17 +360,22 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
>  				if (ret < 0)
>  					goto out_err;
>  			}
> -			ret = us5182d_px_enable(data);
> +			ret = us5182d_set_power_state(data, true);
>  			if (ret < 0)
>  				goto out_err;
> -
> +			ret = us5182d_px_enable(data);
> +			if (ret < 0)
> +				goto out_poweroff;
>  			ret = i2c_smbus_read_word_data(data->client,
>  						       US5182D_REG_PDL);
>  			if (ret < 0)
> +				goto out_poweroff;
> +			*val = ret;
> +			ret = us5182d_set_power_state(data, false);
> +			if (ret < 0)
>  				goto out_err;
>  			mutex_unlock(&data->lock);
> -			*val = ret;
> -			return  IIO_VAL_INT;
> +			return IIO_VAL_INT;
This is a little bit of noise that should have been dealt with separately...

>  		default:
>  			return -EINVAL;
>  		}
> @@ -361,6 +394,9 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
>  	}
>  
>  	return -EINVAL;
> +
> +out_poweroff:
> +	us5182d_set_power_state(data, false);
>  out_err:
>  	mutex_unlock(&data->lock);
>  	return ret;
> @@ -577,6 +613,17 @@ static int us5182d_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		goto out_err;
>  
> +	if (data->default_continuous) {
> +		pm_runtime_set_active(&client->dev);
> +		if (ret < 0)
> +			goto out_err;
> +	}
> +
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_set_autosuspend_delay(&client->dev,
> +					 US5182D_SLEEP_MS);
> +	pm_runtime_use_autosuspend(&client->dev);
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		goto out_err;
> @@ -595,9 +642,42 @@ static int us5182d_remove(struct i2c_client *client)
>  
>  	iio_device_unregister(i2c_get_clientdata(client));
>  
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +
>  	return us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
>  }
>  
> +#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM)
> +static int us5182d_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct us5182d_data *data = iio_priv(indio_dev);
> +
> +	if (data->power_mode == US5182D_CONTINUOUS)
> +		return us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
> +
> +	return 0;
> +}
> +
> +static int us5182d_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct us5182d_data *data = iio_priv(indio_dev);
> +
> +	if (data->power_mode == US5182D_CONTINUOUS)
> +		return us5182d_shutdown_en(data,
> +					   ~US5182D_CFG0_SHUTDOWN_EN & 0xff);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops us5182d_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(us5182d_suspend, us5182d_resume)
> +	SET_RUNTIME_PM_OPS(us5182d_suspend, us5182d_resume, NULL)
> +};
> +
>  static const struct acpi_device_id us5182d_acpi_match[] = {
>  	{ "USD5182", 0},
>  	{}
> @@ -615,6 +695,7 @@ MODULE_DEVICE_TABLE(i2c, us5182d_id);
>  static struct i2c_driver us5182d_driver = {
>  	.driver = {
>  		.name = US5182D_DRV_NAME,
> +		.pm = &us5182d_pm_ops,
>  		.acpi_match_table = ACPI_PTR(us5182d_acpi_match),
>  	},
>  	.probe = us5182d_probe,
> 

[PATCH 5/5] iio: light: us5182d: Add interrupt support and events

[Adriana Reus]

Add interrupt support and events for proximity.
Add two threshold events to signal rising and falling directions.

Signed-off-by: Adriana Reus <adriana.reus@intel.com>
---
 drivers/iio/light/us5182d.c | 272 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 271 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index 60cab4a..4d1f80d 100644
--- a/drivers/iio/light/us5182d.c
+++ b/drivers/iio/light/us5182d.c
@@ -20,7 +20,10 @@
 #include <linux/acpi.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/iio/sysfs.h>
 #include <linux/mutex.h>
 #include <linux/pm.h>
@@ -30,6 +33,8 @@
 #define US5182D_CFG0_ONESHOT_EN				BIT(6)
 #define US5182D_CFG0_SHUTDOWN_EN			BIT(7)
 #define US5182D_CFG0_WORD_ENABLE			BIT(0)
+#define US5182D_CFG0_PROX				BIT(3)
+#define US5182D_CFG0_PX_IRQ				BIT(2)
 
 #define US5182D_REG_CFG1				0x01
 #define US5182D_CFG1_ALS_RES16				BIT(4)
@@ -41,6 +46,7 @@
 
 #define US5182D_REG_CFG3				0x03
 #define US5182D_CFG3_LED_CURRENT100			(BIT(4) | BIT(5))
+#define US5182D_CFG3_INT_SOURCE_PX			BIT(3)
 
 #define US5182D_REG_CFG4				0x10
 
@@ -55,6 +61,13 @@
 #define US5182D_REG_AUTO_LDARK_GAIN		0x29
 #define US5182D_REG_AUTO_HDARK_GAIN		0x2a
 
+/* Thresholds for events: px low (0x08-l, 0x09-h), px high (0x0a-l 0x0b-h) */
+#define US5182D_REG_PXL_TH			0x08
+#define US5182D_REG_PXH_TH			0x0a
+
+#define US5182D_REG_PXL_TH_DEFAULT		1000
+#define US5182D_REG_PXH_TH_DEFAULT		30000
+
 #define US5182D_OPMODE_ALS			0x01
 #define US5182D_OPMODE_PX			0x02
 #define US5182D_OPMODE_SHIFT			4
@@ -84,6 +97,8 @@
 #define US5182D_READ_WORD			2
 #define US5182D_OPSTORE_SLEEP_TIME		20 /* ms */
 #define US5182D_SLEEP_MS			3000 /* ms */
+#define US5182D_PXH_TH_DISABLE			0xffff
+#define US5182D_PXL_TH_DISABLE			0x0000
 
 /* Available ranges: [12354, 7065, 3998, 2202, 1285, 498, 256, 138] lux */
 static const int us5182d_scales[] = {188500, 107800, 61000, 33600, 19600, 7600,
@@ -119,6 +134,12 @@ struct us5182d_data {
 	u8 upper_dark_gain;
 	u16 *us5182d_dark_ths;
 
+	u16 px_low_th;
+	u16 px_high_th;
+
+	int rising_en;
+	int falling_en;
+
 	u8 opmode;
 	u8 power_mode;
 
@@ -148,10 +169,26 @@ static const struct {
 	{US5182D_REG_CFG1, US5182D_CFG1_ALS_RES16},
 	{US5182D_REG_CFG2, (US5182D_CFG2_PX_RES16 |
 			    US5182D_CFG2_PXGAIN_DEFAULT)},
-	{US5182D_REG_CFG3, US5182D_CFG3_LED_CURRENT100},
+	{US5182D_REG_CFG3, US5182D_CFG3_LED_CURRENT100 |
+			   US5182D_CFG3_INT_SOURCE_PX},
 	{US5182D_REG_CFG4, 0x00},
 };
 
+static const struct iio_event_spec us5182d_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
 static const struct iio_chan_spec us5182d_channels[] = {
 	{
 		.type = IIO_LIGHT,
@@ -161,6 +198,8 @@ static const struct iio_chan_spec us5182d_channels[] = {
 	{
 		.type = IIO_PROXIMITY,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.event_spec = us5182d_events,
+		.num_event_specs = ARRAY_SIZE(us5182d_events),
 	}
 };
 
@@ -477,11 +516,199 @@ static int us5182d_write_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int us5182d_setup_prox(struct iio_dev *indio_dev,
+			      enum iio_event_direction dir, u16 val)
+{
+	struct us5182d_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (dir == IIO_EV_DIR_FALLING)
+		ret = i2c_smbus_write_word_data(data->client,
+						US5182D_REG_PXL_TH, val);
+
+	if (dir == IIO_EV_DIR_RISING)
+		ret = i2c_smbus_write_word_data(data->client,
+						US5182D_REG_PXH_TH, val);
+
+	return ret;
+}
+
+static int us5182d_read_thresh(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, enum iio_event_info info, int *val,
+	int *val2)
+{
+	struct us5182d_data *data = iio_priv(indio_dev);
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		mutex_lock(&data->lock);
+		*val = data->px_high_th;
+		mutex_unlock(&data->lock);
+		break;
+	case IIO_EV_DIR_FALLING:
+		mutex_lock(&data->lock);
+		*val = data->px_low_th;
+		mutex_unlock(&data->lock);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return IIO_VAL_INT;
+}
+
+static int us5182d_write_thresh(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, enum iio_event_info info, int val,
+	int val2)
+{
+	struct us5182d_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (val < 0 || val > USHRT_MAX || val2 != 0)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		mutex_lock(&data->lock);
+		if (data->rising_en) {
+			ret = us5182d_setup_prox(indio_dev, dir, val);
+			if (ret < 0)
+				goto err;
+		}
+		data->px_high_th = val;
+		mutex_unlock(&data->lock);
+		break;
+	case IIO_EV_DIR_FALLING:
+		mutex_lock(&data->lock);
+		if (data->falling_en) {
+			ret = us5182d_setup_prox(indio_dev, dir, val);
+			if (ret < 0)
+				goto err;
+		}
+		data->px_low_th = val;
+		mutex_unlock(&data->lock);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+err:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static int us5182d_read_event_config(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir)
+{
+	struct us5182d_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		mutex_lock(&data->lock);
+		ret = data->rising_en;
+		mutex_unlock(&data->lock);
+		break;
+	case IIO_EV_DIR_FALLING:
+		mutex_lock(&data->lock);
+		ret = data->falling_en;
+		mutex_unlock(&data->lock);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int us5182d_write_event_config(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, int state)
+{
+	struct us5182d_data *data = iio_priv(indio_dev);
+	int ret;
+	u16 new_th;
+
+	mutex_lock(&data->lock);
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		if (data->rising_en == state)
+			goto out;
+		new_th = US5182D_PXH_TH_DISABLE;
+		if (state) {
+			data->power_mode = US5182D_CONTINUOUS;
+			ret = us5182d_set_power_state(data, true);
+			if (ret < 0)
+				goto err;
+			ret = us5182d_px_enable(data);
+			if (ret < 0)
+				goto err_poweroff;
+			new_th = data->px_high_th;
+		}
+		ret = us5182d_setup_prox(indio_dev, dir, new_th);
+		if (ret < 0)
+			goto err_poweroff;
+		data->rising_en = state;
+		break;
+	case IIO_EV_DIR_FALLING:
+		if (data->falling_en == state)
+			goto out;
+		new_th =  US5182D_PXL_TH_DISABLE;
+		if (state) {
+			data->power_mode = US5182D_CONTINUOUS;
+			ret = us5182d_set_power_state(data, true);
+			if (ret < 0)
+				goto err;
+			ret = us5182d_px_enable(data);
+			if (ret < 0)
+				goto err_poweroff;
+			new_th = data->px_low_th;
+		}
+		ret = us5182d_setup_prox(indio_dev, dir, new_th);
+		if (ret < 0)
+			goto err_poweroff;
+		data->falling_en = state;
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+	if (!state) {
+		ret = us5182d_set_power_state(data, false);
+		if (ret < 0)
+			goto err;
+	}
+
+	if (!data->falling_en && !data->rising_en && !data->default_continuous)
+		data->power_mode = US5182D_ONESHOT;
+
+out:
+	mutex_unlock(&data->lock);
+	return 0;
+err_poweroff:
+	if (state)
+		us5182d_set_power_state(data, false);
+err:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
 static const struct iio_info us5182d_info = {
 	.driver_module	= THIS_MODULE,
 	.read_raw = us5182d_read_raw,
 	.write_raw = us5182d_write_raw,
 	.attrs = &us5182d_attr_group,
+	.read_event_value = &us5182d_read_thresh,
+	.write_event_value = &us5182d_write_thresh,
+	.read_event_config = &us5182d_read_event_config,
+	.write_event_config = &us5182d_write_event_config,
 };
 
 static int us5182d_reset(struct iio_dev *indio_dev)
@@ -503,6 +730,9 @@ static int us5182d_init(struct iio_dev *indio_dev)
 
 	data->opmode = 0;
 	data->power_mode = US5182D_CONTINUOUS;
+	data->px_low_th = US5182D_REG_PXL_TH_DEFAULT;
+	data->px_high_th = US5182D_REG_PXH_TH_DEFAULT;
+
 	for (i = 0; i < ARRAY_SIZE(us5182d_regvals); i++) {
 		ret = i2c_smbus_write_byte_data(data->client,
 						us5182d_regvals[i].reg,
@@ -573,6 +803,36 @@ static int  us5182d_dark_gain_config(struct iio_dev *indio_dev)
 					 US5182D_REG_DARK_AUTO_EN_DEFAULT);
 }
 
+static irqreturn_t us5182d_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct us5182d_data *data = iio_priv(indio_dev);
+	enum iio_event_direction dir;
+	int ret;
+	int approach;
+	u64 ev;
+
+	ret = i2c_smbus_read_byte_data(data->client, US5182D_REG_CFG0);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "i2c transfer error in irq\n");
+		return IRQ_HANDLED;
+	}
+
+	approach = ret & US5182D_CFG0_PROX;
+	dir = approach ? IIO_EV_DIR_RISING : IIO_EV_DIR_FALLING;
+	ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 1, IIO_EV_TYPE_THRESH, dir);
+
+	iio_push_event(indio_dev, ev, iio_get_time_ns());
+
+	ret = ret & ~US5182D_CFG0_PX_IRQ;
+
+	ret = i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);
+	if (ret < 0)
+		dev_err(&data->client->dev, "i2c transfer error in irq\n");
+
+	return IRQ_HANDLED;
+}
+
 static int us5182d_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -604,6 +864,16 @@ static int us5182d_probe(struct i2c_client *client,
 		return (ret < 0) ? ret : -ENODEV;
 	}
 
+	if (client->irq > 0) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+						us5182d_irq_thread_handler,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						"us5182d-irq", indio_dev);
+		if (ret < 0)
+			return ret;
+	} else
+		dev_warn(&client->dev, "no valid irq found\n");
+
 	us5182d_get_platform_data(indio_dev);
 	ret = us5182d_init(indio_dev);
 	if (ret < 0)
-- 
1.9.1

Re: [PATCH 5/5] iio: light: us5182d: Add interrupt support and events

[Jonathan Cameron]

On 24/11/15 10:59, Adriana Reus wrote:
> Add interrupt support and events for proximity.
> Add two threshold events to signal rising and falling directions.
> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
A few bits and bobs inline...
> ---
>  drivers/iio/light/us5182d.c | 272 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 271 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
> index 60cab4a..4d1f80d 100644
> --- a/drivers/iio/light/us5182d.c
> +++ b/drivers/iio/light/us5182d.c
> @@ -20,7 +20,10 @@
>  #include <linux/acpi.h>
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/mutex.h>
>  #include <linux/pm.h>
> @@ -30,6 +33,8 @@
>  #define US5182D_CFG0_ONESHOT_EN				BIT(6)
>  #define US5182D_CFG0_SHUTDOWN_EN			BIT(7)
>  #define US5182D_CFG0_WORD_ENABLE			BIT(0)
> +#define US5182D_CFG0_PROX				BIT(3)
> +#define US5182D_CFG0_PX_IRQ				BIT(2)
>  
>  #define US5182D_REG_CFG1				0x01
>  #define US5182D_CFG1_ALS_RES16				BIT(4)
> @@ -41,6 +46,7 @@
>  
>  #define US5182D_REG_CFG3				0x03
>  #define US5182D_CFG3_LED_CURRENT100			(BIT(4) | BIT(5))
> +#define US5182D_CFG3_INT_SOURCE_PX			BIT(3)
>  
>  #define US5182D_REG_CFG4				0x10
>  
> @@ -55,6 +61,13 @@
>  #define US5182D_REG_AUTO_LDARK_GAIN		0x29
>  #define US5182D_REG_AUTO_HDARK_GAIN		0x2a
>  
> +/* Thresholds for events: px low (0x08-l, 0x09-h), px high (0x0a-l 0x0b-h) */
> +#define US5182D_REG_PXL_TH			0x08
> +#define US5182D_REG_PXH_TH			0x0a
> +
> +#define US5182D_REG_PXL_TH_DEFAULT		1000
> +#define US5182D_REG_PXH_TH_DEFAULT		30000
> +
>  #define US5182D_OPMODE_ALS			0x01
>  #define US5182D_OPMODE_PX			0x02
>  #define US5182D_OPMODE_SHIFT			4
> @@ -84,6 +97,8 @@
>  #define US5182D_READ_WORD			2
>  #define US5182D_OPSTORE_SLEEP_TIME		20 /* ms */
>  #define US5182D_SLEEP_MS			3000 /* ms */
> +#define US5182D_PXH_TH_DISABLE			0xffff
> +#define US5182D_PXL_TH_DISABLE			0x0000
>  
>  /* Available ranges: [12354, 7065, 3998, 2202, 1285, 498, 256, 138] lux */
>  static const int us5182d_scales[] = {188500, 107800, 61000, 33600, 19600, 7600,
> @@ -119,6 +134,12 @@ struct us5182d_data {
>  	u8 upper_dark_gain;
>  	u16 *us5182d_dark_ths;
>  
> +	u16 px_low_th;
> +	u16 px_high_th;
> +
> +	int rising_en;
> +	int falling_en;
> +
>  	u8 opmode;
>  	u8 power_mode;
>  
> @@ -148,10 +169,26 @@ static const struct {
>  	{US5182D_REG_CFG1, US5182D_CFG1_ALS_RES16},
>  	{US5182D_REG_CFG2, (US5182D_CFG2_PX_RES16 |
>  			    US5182D_CFG2_PXGAIN_DEFAULT)},
> -	{US5182D_REG_CFG3, US5182D_CFG3_LED_CURRENT100},
> +	{US5182D_REG_CFG3, US5182D_CFG3_LED_CURRENT100 |
> +			   US5182D_CFG3_INT_SOURCE_PX},
>  	{US5182D_REG_CFG4, 0x00},
>  };
>  
> +static const struct iio_event_spec us5182d_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +				BIT(IIO_EV_INFO_ENABLE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +				BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
>  static const struct iio_chan_spec us5182d_channels[] = {
>  	{
>  		.type = IIO_LIGHT,
> @@ -161,6 +198,8 @@ static const struct iio_chan_spec us5182d_channels[] = {
>  	{
>  		.type = IIO_PROXIMITY,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.event_spec = us5182d_events,
> +		.num_event_specs = ARRAY_SIZE(us5182d_events),
>  	}
>  };
>  
> @@ -477,11 +516,199 @@ static int us5182d_write_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static int us5182d_setup_prox(struct iio_dev *indio_dev,
> +			      enum iio_event_direction dir, u16 val)
> +{
> +	struct us5182d_data *data = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	if (dir == IIO_EV_DIR_FALLING)
> +		ret = i2c_smbus_write_word_data(data->client,
> +						US5182D_REG_PXL_TH, val);
> +
> +	if (dir == IIO_EV_DIR_RISING)
> +		ret = i2c_smbus_write_word_data(data->client,
> +						US5182D_REG_PXH_TH, val);
> +
> +	return ret;
return directly above, no need for the local ret variable and return
0 here.
> +}
> +
> +static int us5182d_read_thresh(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int *val,
> +	int *val2)
> +{
> +	struct us5182d_data *data = iio_priv(indio_dev);
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		mutex_lock(&data->lock);
> +		*val = data->px_high_th;
> +		mutex_unlock(&data->lock);
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		mutex_lock(&data->lock);
> +		*val = data->px_low_th;
> +		mutex_unlock(&data->lock);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int us5182d_write_thresh(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int val,
> +	int val2)
> +{
> +	struct us5182d_data *data = iio_priv(indio_dev);
> +	int ret = 0;
There is no path in which ret is used and not overriden that I can see
so drop the initialize.

> +
> +	if (val < 0 || val > USHRT_MAX || val2 != 0)
> +		return -EINVAL;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		mutex_lock(&data->lock);
> +		if (data->rising_en) {
> +			ret = us5182d_setup_prox(indio_dev, dir, val);
> +			if (ret < 0)
> +				goto err;
> +		}
> +		data->px_high_th = val;
> +		mutex_unlock(&data->lock);
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		mutex_lock(&data->lock);
> +		if (data->falling_en) {
> +			ret = us5182d_setup_prox(indio_dev, dir, val);
> +			if (ret < 0)
> +				goto err;
> +		}
> +		data->px_low_th = val;
> +		mutex_unlock(&data->lock);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +err:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static int us5182d_read_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir)
> +{
> +	struct us5182d_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		mutex_lock(&data->lock);
> +		ret = data->rising_en;
> +		mutex_unlock(&data->lock);

> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		mutex_lock(&data->lock);
> +		ret = data->falling_en;
> +		mutex_unlock(&data->lock);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int us5182d_write_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, int state)
> +{
> +	struct us5182d_data *data = iio_priv(indio_dev);
> +	int ret;
> +	u16 new_th;
> +
> +	mutex_lock(&data->lock);
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		if (data->rising_en == state)
> +			goto out;
> +		new_th = US5182D_PXH_TH_DISABLE;
> +		if (state) {
> +			data->power_mode = US5182D_CONTINUOUS;
> +			ret = us5182d_set_power_state(data, true);
> +			if (ret < 0)
> +				goto err;
> +			ret = us5182d_px_enable(data);
> +			if (ret < 0)
> +				goto err_poweroff;
> +			new_th = data->px_high_th;
> +		}
> +		ret = us5182d_setup_prox(indio_dev, dir, new_th);
> +		if (ret < 0)
> +			goto err_poweroff;
> +		data->rising_en = state;
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		if (data->falling_en == state)
> +			goto out;
> +		new_th =  US5182D_PXL_TH_DISABLE;
> +		if (state) {
> +			data->power_mode = US5182D_CONTINUOUS;
> +			ret = us5182d_set_power_state(data, true);
> +			if (ret < 0)
> +				goto err;
> +			ret = us5182d_px_enable(data);
> +			if (ret < 0)
> +				goto err_poweroff;
> +			new_th = data->px_low_th;
> +		}
> +		ret = us5182d_setup_prox(indio_dev, dir, new_th);
> +		if (ret < 0)
> +			goto err_poweroff;
> +		data->falling_en = state;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	if (!state) {
> +		ret = us5182d_set_power_state(data, false);
> +		if (ret < 0)
> +			goto err;
> +	}
> +
> +	if (!data->falling_en && !data->rising_en && !data->default_continuous)
> +		data->power_mode = US5182D_ONESHOT;
> +
> +out:
> +	mutex_unlock(&data->lock);
> +	return 0;
I'm not terribly keen on these multiple exit routes reached by gotos but
can't see a trivial way to clean this up other than simply having
mutex unlock and return inline where you use the above goto.  Might
be easier to follow.

> +err_poweroff:
> +	if (state)
> +		us5182d_set_power_state(data, false);
> +err:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
>  static const struct iio_info us5182d_info = {
>  	.driver_module	= THIS_MODULE,
>  	.read_raw = us5182d_read_raw,
>  	.write_raw = us5182d_write_raw,
>  	.attrs = &us5182d_attr_group,
> +	.read_event_value = &us5182d_read_thresh,
> +	.write_event_value = &us5182d_write_thresh,
> +	.read_event_config = &us5182d_read_event_config,
> +	.write_event_config = &us5182d_write_event_config,
>  };
>  
>  static int us5182d_reset(struct iio_dev *indio_dev)
> @@ -503,6 +730,9 @@ static int us5182d_init(struct iio_dev *indio_dev)
>  
>  	data->opmode = 0;
>  	data->power_mode = US5182D_CONTINUOUS;
> +	data->px_low_th = US5182D_REG_PXL_TH_DEFAULT;
> +	data->px_high_th = US5182D_REG_PXH_TH_DEFAULT;
> +
>  	for (i = 0; i < ARRAY_SIZE(us5182d_regvals); i++) {
>  		ret = i2c_smbus_write_byte_data(data->client,
>  						us5182d_regvals[i].reg,
> @@ -573,6 +803,36 @@ static int  us5182d_dark_gain_config(struct iio_dev *indio_dev)
>  					 US5182D_REG_DARK_AUTO_EN_DEFAULT);
>  }
>  
> +static irqreturn_t us5182d_irq_thread_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct us5182d_data *data = iio_priv(indio_dev);
> +	enum iio_event_direction dir;
> +	int ret;
> +	int approach;
> +	u64 ev;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, US5182D_REG_CFG0);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "i2c transfer error in irq\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	approach = ret & US5182D_CFG0_PROX;
> +	dir = approach ? IIO_EV_DIR_RISING : IIO_EV_DIR_FALLING;
> +	ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 1, IIO_EV_TYPE_THRESH, dir);
> +
> +	iio_push_event(indio_dev, ev, iio_get_time_ns());
I might argue slightly in favour of doing the timestamp acquire in the
top half, but I doubt anyone is ever that bothered by precise timing
of such an event so lets leave it as is for now.

> +
> +	ret = ret & ~US5182D_CFG0_PX_IRQ;
> +
>From a reability point of view, I'd have been tempted to do this manipulation
directly in the next call - out here you almost look ato be manipulating
return values rather than working out the value to write.

> +	ret = i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0, ret);

> +	if (ret < 0)
> +		dev_err(&data->client->dev, "i2c transfer error in irq\n");
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int us5182d_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -604,6 +864,16 @@ static int us5182d_probe(struct i2c_client *client,
>  		return (ret < 0) ? ret : -ENODEV;
>  	}
>  
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +						us5182d_irq_thread_handler,
> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +						"us5182d-irq", indio_dev);
> +		if (ret < 0)
> +			return ret;
> +	} else
> +		dev_warn(&client->dev, "no valid irq found\n");
> +
>  	us5182d_get_platform_data(indio_dev);
>  	ret = us5182d_init(indio_dev);
>  	if (ret < 0)
>