[PATCH 0/3] iio: light: us5218d: Add interrupt support and some tidying up

[PATCH 0/3] iio: light: us5218d: Add interrupt support and some tidying up

[Adriana Reus]

The first patch fixes an inconsistency that I've missed in the px_enable and
als enable status variables.
The second patch adds interrupt support for proximity. It's the same with what
I previously sent (https://lkml.org/lkml/2015/11/29/83), but with the suggested
changes. I chose to resend it because it needs to go together with the first one
because there is a particular workflow case involving event enabling that can be
affected by that bug.
And lastly, some refactoring to improve readability regarding the read raw function. 


Adriana Reus (3):
  iio: light: us5182d: Fix enable status inconcistency
  iio: light: us5182d: Add interrupt support and events
  iio: light: us5182d: Refactor read_raw function

 drivers/iio/light/us5182d.c | 445 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 363 insertions(+), 82 deletions(-)

-- 
1.9.1

[PATCH 1/3] iio: light: us5182d: Fix enable status inconcistency

[Adriana Reus]

When setting als only or proximity only modes make sure that we mark the other
component as disabled.

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

diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index 256c4bc..f24b687 100644
--- a/drivers/iio/light/us5182d.c
+++ b/drivers/iio/light/us5182d.c
@@ -238,8 +238,12 @@ 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->power_mode == US5182D_ONESHOT) {
+		ret = us5182d_set_opmode(data, US5182D_ALS_ONLY);
+		if (ret < 0)
+			return ret;
+		data->px_enabled = false;
+	}
 
 	if (data->als_enabled)
 		return 0;
@@ -260,8 +264,12 @@ 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->power_mode == US5182D_ONESHOT) {
+		ret = us5182d_set_opmode(data, US5182D_PX_ONLY);
+		if (ret < 0)
+			return ret;
+		data->als_enabled = false;
+	}
 
 	if (data->px_enabled)
 		return 0;
-- 
1.9.1

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

[Adriana Reus]

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

Signed-off-by: Adriana Reus <adriana.reus@intel.com>
---
No changes since my last sent version other than the suggested ones:
* in us5182_setup_prox - lose the local ret variable and return directly
* in write_thresh - drop the initialization of ret
* in write_event_config - lose one goto exit route in favour of returning
  inline to improve readability
* in interrut handler - minor readability improvement 

- 
 drivers/iio/light/us5182d.c | 274 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 273 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index f24b687..033f5cf 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),
 	}
 };
 
@@ -487,11 +526,202 @@ 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);
+
+	if (dir == IIO_EV_DIR_FALLING)
+		return i2c_smbus_write_word_data(data->client,
+						 US5182D_REG_PXL_TH, val);
+
+	if (dir == IIO_EV_DIR_RISING)
+		return i2c_smbus_write_word_data(data->client,
+						 US5182D_REG_PXH_TH, val);
+
+	return 0;
+}
+
+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;
+
+	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) {
+			mutex_unlock(&data->lock);
+			return 0;
+		}
+		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) {
+			mutex_unlock(&data->lock);
+			return 0;
+		}
+		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;
+
+	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)
@@ -513,6 +743,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,
@@ -583,6 +816,35 @@ 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 = i2c_smbus_write_byte_data(data->client, US5182D_REG_CFG0,
+					ret & ~US5182D_CFG0_PX_IRQ);
+	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)
 {
@@ -614,6 +876,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

[PATCH 3/3] iio: light: us5182d: Refactor read_raw function

[Adriana Reus]

A bit of refactoring for better readability.
Moved and slightly reorganized all the activity necessary for reading als and
proximity into a different function. This way the switch in read raw becomes
clearer and more compact.

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

diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index 033f5cf..53d161d 100644
--- a/drivers/iio/light/us5182d.c
+++ b/drivers/iio/light/us5182d.c
@@ -203,23 +203,6 @@ static const struct iio_chan_spec us5182d_channels[] = {
 	}
 };
 
-static int us5182d_get_als(struct us5182d_data *data)
-{
-	int ret;
-	unsigned long result;
-
-	ret = i2c_smbus_read_word_data(data->client,
-				       US5182D_REG_ADL);
-	if (ret < 0)
-		return ret;
-
-	result = ret * data->ga / US5182D_GA_RESOLUTION;
-	if (result > 0xffff)
-		result = 0xffff;
-
-	return result;
-}
-
 static int us5182d_oneshot_en(struct us5182d_data *data)
 {
 	int ret;
@@ -324,6 +307,39 @@ static int us5182d_px_enable(struct us5182d_data *data)
 	return 0;
 }
 
+static int us5182d_get_als(struct us5182d_data *data)
+{
+	int ret;
+	unsigned long result;
+
+	ret = us5182d_als_enable(data);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_read_word_data(data->client,
+				       US5182D_REG_ADL);
+	if (ret < 0)
+		return ret;
+
+	result = ret * data->ga / US5182D_GA_RESOLUTION;
+	if (result > 0xffff)
+		result = 0xffff;
+
+	return result;
+}
+
+static int us5182d_get_px(struct us5182d_data *data)
+{
+	int ret;
+
+	ret = us5182d_px_enable(data);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_read_word_data(data->client,
+					US5182D_REG_PDL);
+}
+
 static int us5182d_shutdown_en(struct us5182d_data *data, u8 state)
 {
 	int ret;
@@ -370,6 +386,46 @@ static int us5182d_set_power_state(struct us5182d_data *data, bool on)
 	return ret;
 }
 
+static int us5182d_read_value(struct us5182d_data *data,
+			      struct iio_chan_spec const *chan)
+{
+	int ret, value;
+
+	mutex_lock(&data->lock);
+
+	if (data->power_mode == US5182D_ONESHOT) {
+		ret = us5182d_oneshot_en(data);
+		if (ret < 0)
+			goto out_err;
+	}
+
+	ret = us5182d_set_power_state(data, true);
+	if (ret < 0)
+		goto out_err;
+
+	if (chan->type == IIO_LIGHT)
+		ret = us5182d_get_als(data);
+	else
+		ret = us5182d_get_px(data);
+	if (ret < 0)
+		goto out_poweroff;
+
+	value = ret;
+
+	ret = us5182d_set_power_state(data, false);
+	if (ret < 0)
+		goto out_err;
+
+	mutex_unlock(&data->lock);
+	return value;
+
+out_poweroff:
+	us5182d_set_power_state(data, false);
+out_err:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
 static int us5182d_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long mask)
@@ -379,76 +435,21 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		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_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);
-			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_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);
-			return IIO_VAL_INT;
-		default:
-			return -EINVAL;
-		}
-
+		ret = us5182d_read_value(data, chan);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		ret = i2c_smbus_read_byte_data(data->client, US5182D_REG_CFG1);
 		if (ret < 0)
 			return ret;
-
 		*val = 0;
 		*val2 = us5182d_scales[ret & US5182D_AGAIN_MASK];
-
 		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
-
-	return -EINVAL;
-
-out_poweroff:
-	us5182d_set_power_state(data, false);
-out_err:
-	mutex_unlock(&data->lock);
-	return ret;
 }
 
 /**
-- 
1.9.1

Re: [PATCH 1/3] iio: light: us5182d: Fix enable status inconcistency

[Jonathan Cameron]

On 09/12/15 11:40, Adriana Reus wrote:
> When setting als only or proximity only modes make sure that we mark the other
> component as disabled.
> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
Hi Adriana

When you send a fix of any type, please describe the effects of the bug.
That way I have more information to figure out if the bug is tidying up
a loose end or fixing a critical bug and hence judge which path it takes
to upstream.

Please let me know for this one as I can't immediately pick out what the
effects will be.

Jonathan
> ---
>  drivers/iio/light/us5182d.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
> index 256c4bc..f24b687 100644
> --- a/drivers/iio/light/us5182d.c
> +++ b/drivers/iio/light/us5182d.c
> @@ -238,8 +238,12 @@ 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->power_mode == US5182D_ONESHOT) {
> +		ret = us5182d_set_opmode(data, US5182D_ALS_ONLY);
> +		if (ret < 0)
> +			return ret;
> +		data->px_enabled = false;
> +	}
>  
>  	if (data->als_enabled)
>  		return 0;
> @@ -260,8 +264,12 @@ 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->power_mode == US5182D_ONESHOT) {
> +		ret = us5182d_set_opmode(data, US5182D_PX_ONLY);
> +		if (ret < 0)
> +			return ret;
> +		data->als_enabled = false;
> +	}
>  
>  	if (data->px_enabled)
>  		return 0;
> 

Re: [PATCH 3/3] iio: light: us5182d: Refactor read_raw function

[Jonathan Cameron]

On 09/12/15 11:40, Adriana Reus wrote:
> A bit of refactoring for better readability.
> Moved and slightly reorganized all the activity necessary for reading als and
> proximity into a different function. This way the switch in read raw becomes
> clearer and more compact.
> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
Sensible tidy up.  Will take once the others are sorted.
> ---
>  drivers/iio/light/us5182d.c | 155 ++++++++++++++++++++++----------------------
>  1 file changed, 78 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
> index 033f5cf..53d161d 100644
> --- a/drivers/iio/light/us5182d.c
> +++ b/drivers/iio/light/us5182d.c
> @@ -203,23 +203,6 @@ static const struct iio_chan_spec us5182d_channels[] = {
>  	}
>  };
>  
> -static int us5182d_get_als(struct us5182d_data *data)
> -{
> -	int ret;
> -	unsigned long result;
> -
> -	ret = i2c_smbus_read_word_data(data->client,
> -				       US5182D_REG_ADL);
> -	if (ret < 0)
> -		return ret;
> -
> -	result = ret * data->ga / US5182D_GA_RESOLUTION;
> -	if (result > 0xffff)
> -		result = 0xffff;
> -
> -	return result;
> -}
> -
>  static int us5182d_oneshot_en(struct us5182d_data *data)
>  {
>  	int ret;
> @@ -324,6 +307,39 @@ static int us5182d_px_enable(struct us5182d_data *data)
>  	return 0;
>  }
>  
> +static int us5182d_get_als(struct us5182d_data *data)
> +{
> +	int ret;
> +	unsigned long result;
> +
> +	ret = us5182d_als_enable(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_read_word_data(data->client,
> +				       US5182D_REG_ADL);
> +	if (ret < 0)
> +		return ret;
> +
> +	result = ret * data->ga / US5182D_GA_RESOLUTION;
> +	if (result > 0xffff)
> +		result = 0xffff;
> +
> +	return result;
> +}
> +
> +static int us5182d_get_px(struct us5182d_data *data)
> +{
> +	int ret;
> +
> +	ret = us5182d_px_enable(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_read_word_data(data->client,
> +					US5182D_REG_PDL);
> +}
> +
>  static int us5182d_shutdown_en(struct us5182d_data *data, u8 state)
>  {
>  	int ret;
> @@ -370,6 +386,46 @@ static int us5182d_set_power_state(struct us5182d_data *data, bool on)
>  	return ret;
>  }
>  
> +static int us5182d_read_value(struct us5182d_data *data,
> +			      struct iio_chan_spec const *chan)
> +{
> +	int ret, value;
> +
> +	mutex_lock(&data->lock);
> +
> +	if (data->power_mode == US5182D_ONESHOT) {
> +		ret = us5182d_oneshot_en(data);
> +		if (ret < 0)
> +			goto out_err;
> +	}
> +
> +	ret = us5182d_set_power_state(data, true);
> +	if (ret < 0)
> +		goto out_err;
> +
> +	if (chan->type == IIO_LIGHT)
> +		ret = us5182d_get_als(data);
> +	else
> +		ret = us5182d_get_px(data);
> +	if (ret < 0)
> +		goto out_poweroff;
> +
> +	value = ret;
> +
> +	ret = us5182d_set_power_state(data, false);
> +	if (ret < 0)
> +		goto out_err;
> +
> +	mutex_unlock(&data->lock);
> +	return value;
> +
> +out_poweroff:
> +	us5182d_set_power_state(data, false);
> +out_err:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
>  static int us5182d_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int *val,
>  			    int *val2, long mask)
> @@ -379,76 +435,21 @@ static int us5182d_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		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_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);
> -			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_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);
> -			return IIO_VAL_INT;
> -		default:
> -			return -EINVAL;
> -		}
> -
> +		ret = us5182d_read_value(data, chan);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		ret = i2c_smbus_read_byte_data(data->client, US5182D_REG_CFG1);
>  		if (ret < 0)
>  			return ret;
> -
>  		*val = 0;
>  		*val2 = us5182d_scales[ret & US5182D_AGAIN_MASK];
> -
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
>  	}
> -
> -	return -EINVAL;
> -
> -out_poweroff:
> -	us5182d_set_power_state(data, false);
> -out_err:
> -	mutex_unlock(&data->lock);
> -	return ret;
>  }
>  
>  /**
> 

Re: [PATCH 1/3] iio: light: us5182d: Fix enable status inconcistency

[Adriana Reus]

On 12.12.2015 16:57, Jonathan Cameron wrote:
> On 09/12/15 11:40, Adriana Reus wrote:
>> When setting als only or proximity only modes make sure that we mark the other
>> component as disabled.
>>
>> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> Hi Adriana
>
> When you send a fix of any type, please describe the effects of the bug.
> That way I have more information to figure out if the bug is tidying up
> a loose end or fixing a critical bug and hence judge which path it takes
> to upstream.
Long story short it's a tidy up that prevents a bug that would occur
once events are added (patch 2 of this series adds events). The exact
use case is: if user polls proximity px_enabled will be set to true,
user then polls als, and sets the mode to ALS_ONLY, while px_enabled
wrongfully remains true, at this point if events are enabled proximity
won't actually get to be enabled in the chip and will not deliver
events. For this reason it has to be applied before or at the same time
with the patch that adds events and interrupt support.
>
> Please let me know for this one as I can't immediately pick out what the
> effects will be.
>
> Jonathan
>> ---
>>   drivers/iio/light/us5182d.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
>> index 256c4bc..f24b687 100644
>> --- a/drivers/iio/light/us5182d.c
>> +++ b/drivers/iio/light/us5182d.c
>> @@ -238,8 +238,12 @@ 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->power_mode == US5182D_ONESHOT) {
>> +		ret = us5182d_set_opmode(data, US5182D_ALS_ONLY);
>> +		if (ret < 0)
>> +			return ret;
>> +		data->px_enabled = false;
>> +	}
>>
>>   	if (data->als_enabled)
>>   		return 0;
>> @@ -260,8 +264,12 @@ 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->power_mode == US5182D_ONESHOT) {
>> +		ret = us5182d_set_opmode(data, US5182D_PX_ONLY);
>> +		if (ret < 0)
>> +			return ret;
>> +		data->als_enabled = false;
>> +	}
>>
>>   	if (data->px_enabled)
>>   		return 0;
>>
>

Re: [PATCH 1/3] iio: light: us5182d: Fix enable status inconcistency

[Jonathan Cameron]

On 14/12/15 09:15, Adriana Reus wrote:
> 
> 
> On 12.12.2015 16:57, Jonathan Cameron wrote:
>> On 09/12/15 11:40, Adriana Reus wrote:
>>> When setting als only or proximity only modes make sure that we mark the other
>>> component as disabled.
>>>
>>> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
>> Hi Adriana
>>
>> When you send a fix of any type, please describe the effects of the bug.
>> That way I have more information to figure out if the bug is tidying up
>> a loose end or fixing a critical bug and hence judge which path it takes
>> to upstream.
> Long story short it's a tidy up that prevents a bug that would occur
> once events are added (patch 2 of this series adds events). The exact
> use case is: if user polls proximity px_enabled will be set to true,
> user then polls als, and sets the mode to ALS_ONLY, while px_enabled
> wrongfully remains true, at this point if events are enabled proximity
> won't actually get to be enabled in the chip and will not deliver
> events. For this reason it has to be applied before or at the same time
> with the patch that adds events and interrupt support.

Thanks
>>
>> Please let me know for this one as I can't immediately pick out what the
>> effects will be.
>>
>> Jonathan
>>> ---
>>>   drivers/iio/light/us5182d.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
>>> index 256c4bc..f24b687 100644
>>> --- a/drivers/iio/light/us5182d.c
>>> +++ b/drivers/iio/light/us5182d.c
>>> @@ -238,8 +238,12 @@ 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->power_mode == US5182D_ONESHOT) {
>>> +        ret = us5182d_set_opmode(data, US5182D_ALS_ONLY);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +        data->px_enabled = false;
>>> +    }
>>>
>>>       if (data->als_enabled)
>>>           return 0;
>>> @@ -260,8 +264,12 @@ 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->power_mode == US5182D_ONESHOT) {
>>> +        ret = us5182d_set_opmode(data, US5182D_PX_ONLY);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +        data->als_enabled = false;
>>> +    }
>>>
>>>       if (data->px_enabled)
>>>           return 0;
>>>
>>
> -- 
> 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