[PATCH v2 1/7] iio: light: rpr0521 disable sensor -bugfix

[PATCH v2 1/7] iio: light: rpr0521 disable sensor -bugfix

[Mikko Koivunen]

Sensor was marked enabled on each call even if the call was for disabling
sensor.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.

 drivers/iio/light/rpr0521.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 7de0f39..c15529b 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -197,8 +197,10 @@ static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
 	if (ret < 0)
 		return ret;
 
-	data->als_dev_en = true;
-
+	if (status & RPR0521_MODE_ALS_MASK)
+		data->als_dev_en = true;
+	else
+		data->als_dev_en = false;
 	return 0;
 }
 
@@ -212,7 +214,10 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
 	if (ret < 0)
 		return ret;
 
-	data->pxs_dev_en = true;
+	if (status & RPR0521_MODE_PXS_MASK)
+		data->pxs_dev_en = true;
+	else
+		data->pxs_dev_en = false;
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH v2 2/7] iio: light: rpr0521 whitespace fixes

[Mikko Koivunen]

Just whitespace changes, no functional changes.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 drivers/iio/light/rpr0521.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index c15529b..30c2592 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -357,8 +357,8 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 			return ret;
 
 		*val = le16_to_cpu(raw_data);
-
 		return IIO_VAL_INT;
+
 	case IIO_CHAN_INFO_SCALE:
 		mutex_lock(&data->lock);
 		ret = rpr0521_get_gain(data, chan->address, val, val2);
-- 
1.7.9.5

[PATCH v2 3/7] iio: light: rpr0521 sample_frequency read/write added

[Mikko Koivunen]

Added sysfs read/write sample frequency.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.

Patch v1->v2 changes:
multiline comments fixed

 drivers/iio/light/rpr0521.c |   99 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 30c2592..e92a8bb 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -132,6 +132,30 @@ static const struct rpr0521_gain_info {
 	},
 };
 
+struct rpr0521_samp_freq {
+	int	als_hz;
+	int	als_uhz;
+	int	pxs_hz;
+	int	pxs_uhz;
+};
+
+static const struct rpr0521_samp_freq rpr0521_samp_freq_i[13] = {
+/*	{ALS, PXS} */
+	{0, 0, 0, 0},		/* 0000, 0=standby */
+	{0, 0, 100, 0},		/* 0001 */
+	{0, 0, 25, 0},		/* 0010 */
+	{0, 0, 10, 0},		/* 0011 */
+	{0, 0, 2, 500000},	/* 0100 */
+	{10, 0, 20, 0},		/* 0101 */
+	{10, 0, 10, 0},		/* 0110 */
+	{10, 0, 2, 500000},	/* 0111 */
+	{2, 500000, 20, 0},	/* 1000, measurement 100ms, sleep 300ms */
+	{2, 500000, 10, 0},	/* 1001, measurement 100ms, sleep 300ms */
+	{2, 500000, 0, 0},	/* 1010, high sensitivity mode */
+	{2, 500000, 2, 500000},	/* 1011, high sensitivity mode */
+	{20, 0, 20, 0}	/* 1100, ALS_data x 0.5, see specification P.18 */
+};
+
 struct rpr0521_data {
 	struct i2c_client *client;
 
@@ -152,9 +176,15 @@ struct rpr0521_data {
 static IIO_CONST_ATTR(in_intensity_scale_available, RPR0521_ALS_SCALE_AVAIL);
 static IIO_CONST_ATTR(in_proximity_scale_available, RPR0521_PXS_SCALE_AVAIL);
 
+/* Start with easy freq first, whole table of freq combinations is more
+ * complicated.
+ */
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("2.5 10");
+
 static struct attribute *rpr0521_attributes[] = {
 	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
 	&iio_const_attr_in_proximity_scale_available.dev_attr.attr,
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
 	NULL,
 };
 
@@ -170,6 +200,7 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.channel2 = IIO_MOD_LIGHT_BOTH,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -178,12 +209,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.channel2 = IIO_MOD_LIGHT_IR,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
 	{
 		.type = IIO_PROXIMITY,
 		.address = RPR0521_CHAN_PXS,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}
 };
 
@@ -319,6 +352,56 @@ static int rpr0521_set_gain(struct rpr0521_data *data, int chan,
 				  idx << rpr0521_gain[chan].shift);
 }
 
+static int rpr0521_read_samp_freq(struct rpr0521_data *data,
+				enum iio_chan_type chan_type,
+			    int *val, int *val2)
+{
+	int reg, ret;
+
+	ret = regmap_read(data->regmap, RPR0521_REG_MODE_CTRL, &reg);
+	if (ret < 0)
+		return ret;
+	reg &= RPR0521_MODE_MEAS_TIME_MASK;
+	if (reg >= ARRAY_SIZE(rpr0521_samp_freq_i))
+		return -EINVAL;
+
+	if (chan_type == IIO_INTENSITY) {
+		*val = rpr0521_samp_freq_i[reg].als_hz;
+		*val2 = rpr0521_samp_freq_i[reg].als_uhz;
+	} else if (chan_type == IIO_PROXIMITY) {
+		*val = rpr0521_samp_freq_i[reg].pxs_hz;
+		*val2 = rpr0521_samp_freq_i[reg].pxs_uhz;
+	} else {
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
+				enum iio_chan_type chan_type,
+				int val, int val2)
+{
+	int i;
+
+	/* Ignore channel
+	 * both pxs and als are setup only to same freq because of simplicity
+	 */
+	for (i = 0; i < ARRAY_SIZE(rpr0521_samp_freq_i); i++)
+		if ((val == rpr0521_samp_freq_i[i].als_hz)
+			&& (val2 == rpr0521_samp_freq_i[i].als_uhz)
+			&& (val == rpr0521_samp_freq_i[i].pxs_hz)
+			&& (val2 == rpr0521_samp_freq_i[i].pxs_uhz)) {
+			break;
+		}
+	if (i == ARRAY_SIZE(rpr0521_samp_freq_i))
+		return -EINVAL;
+
+	return regmap_update_bits(data->regmap,
+		RPR0521_REG_MODE_CTRL,
+		RPR0521_MODE_MEAS_TIME_MASK,
+		i);
+}
+
 static int rpr0521_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long mask)
@@ -365,8 +448,16 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&data->lock);
 		if (ret < 0)
 			return ret;
+		return IIO_VAL_INT_PLUS_MICRO;
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&data->lock);
+		ret = rpr0521_read_samp_freq(data, chan->type, val, val2);
+		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
 		return IIO_VAL_INT_PLUS_MICRO;
+
 	default:
 		return -EINVAL;
 	}
@@ -384,8 +475,16 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
 		mutex_lock(&data->lock);
 		ret = rpr0521_set_gain(data, chan->address, val, val2);
 		mutex_unlock(&data->lock);
+		return ret;
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&data->lock);
+		ret = rpr0521_write_samp_freq_common(data,
+			chan->type,
+			val, val2);
+		mutex_unlock(&data->lock);
 		return ret;
+
 	default:
 		return -EINVAL;
 	}
-- 
1.7.9.5

[PATCH v2 4/7] iio: light: rpr0521 proximity offset read/write added

[Mikko Koivunen]

Added sysfs read/write proximity offset feature. Offset is read/write from
sensor registers. Values are proximity raw 10-bit values. After applying
offset value, output values will be (measured_raw - offset_value). Output
values are unsigned so offset value doesn't make output negative.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
Builds OK with torvalds/linux 4.9
checkpatch.pl OK.

Patch v1->v2 changes:
read/write_ps_offset() rewritten with:
 -  static, __le16, cpu_to_le16(), sizeof()

 drivers/iio/light/rpr0521.c |   53 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index e92a8bb..35c759b 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -30,6 +30,9 @@
 #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
+#define RPR0521_PS_OFFSET_LSB		0x53
+#define RPR0521_PS_OFFSET_MSB		0x54
+
 #define RPR0521_REG_ID			0x92
 
 #define RPR0521_MODE_ALS_MASK		BIT(7)
@@ -215,7 +218,9 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.type = IIO_PROXIMITY,
 		.address = RPR0521_CHAN_PXS,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_OFFSET) |
 			BIT(IIO_CHAN_INFO_SCALE),
+
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}
 };
@@ -402,6 +407,40 @@ static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
 		i);
 }
 
+static int rpr0521_read_ps_offset(struct rpr0521_data *data, int *offset)
+{
+	int ret;
+	__le16 buffer;
+
+	ret = regmap_bulk_read(data->regmap,
+		RPR0521_PS_OFFSET_LSB, &buffer, sizeof(buffer));
+
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to read PS OFFSET register\n");
+		return ret;
+	}
+	*offset = le16_to_cpu(buffer);
+
+	return ret;
+}
+
+static int rpr0521_write_ps_offset(struct rpr0521_data *data, int offset)
+{
+	int ret;
+	__le16 buffer;
+
+	buffer = cpu_to_le16(offset & 0x3ff);
+	ret = regmap_raw_write(data->regmap,
+		RPR0521_PS_OFFSET_LSB, &buffer, sizeof(buffer));
+
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to write PS OFFSET register\n");
+		return ret;
+	}
+
+	return ret;
+}
+
 static int rpr0521_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long mask)
@@ -458,6 +497,14 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case IIO_CHAN_INFO_OFFSET:
+		mutex_lock(&data->lock);
+		ret = rpr0521_read_ps_offset(data, val);
+		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -485,6 +532,12 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&data->lock);
 		return ret;
 
+	case IIO_CHAN_INFO_OFFSET:
+		mutex_lock(&data->lock);
+		ret = rpr0521_write_ps_offset(data, val);
+		mutex_unlock(&data->lock);
+		return ret;
+
 	default:
 		return -EINVAL;
 	}
-- 
1.7.9.5

[PATCH v2 5/7] iio: light: rpr0521 channel numbers reordered

[Mikko Koivunen]

Moved proximity channel from last to first in structs to avoid confusion
later with buffered triggers. Proximity data output is first in rpr0521
register map.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 drivers/iio/light/rpr0521.c |   41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 35c759b..dc522fd 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -80,9 +80,9 @@ static const struct rpr0521_gain rpr0521_pxs_gain[3] = {
 };
 
 enum rpr0521_channel {
+	RPR0521_CHAN_PXS,
 	RPR0521_CHAN_ALS_DATA0,
 	RPR0521_CHAN_ALS_DATA1,
-	RPR0521_CHAN_PXS,
 };
 
 struct rpr0521_reg_desc {
@@ -91,6 +91,10 @@ struct rpr0521_reg_desc {
 };
 
 static const struct rpr0521_reg_desc rpr0521_data_reg[] = {
+	[RPR0521_CHAN_PXS]	= {
+		.address	= RPR0521_REG_PXS_DATA,
+		.device_mask	= RPR0521_MODE_PXS_MASK,
+	},
 	[RPR0521_CHAN_ALS_DATA0] = {
 		.address	= RPR0521_REG_ALS_DATA0,
 		.device_mask	= RPR0521_MODE_ALS_MASK,
@@ -99,10 +103,6 @@ static const struct rpr0521_reg_desc rpr0521_data_reg[] = {
 		.address	= RPR0521_REG_ALS_DATA1,
 		.device_mask	= RPR0521_MODE_ALS_MASK,
 	},
-	[RPR0521_CHAN_PXS]	= {
-		.address	= RPR0521_REG_PXS_DATA,
-		.device_mask	= RPR0521_MODE_PXS_MASK,
-	},
 };
 
 static const struct rpr0521_gain_info {
@@ -112,6 +112,13 @@ static const struct rpr0521_gain_info {
 	const struct rpr0521_gain *gain;
 	int size;
 } rpr0521_gain[] = {
+	[RPR0521_CHAN_PXS] = {
+		.reg	= RPR0521_REG_PXS_CTRL,
+		.mask	= RPR0521_PXS_GAIN_MASK,
+		.shift	= RPR0521_PXS_GAIN_SHIFT,
+		.gain	= rpr0521_pxs_gain,
+		.size	= ARRAY_SIZE(rpr0521_pxs_gain),
+	},
 	[RPR0521_CHAN_ALS_DATA0] = {
 		.reg	= RPR0521_REG_ALS_CTRL,
 		.mask	= RPR0521_ALS_DATA0_GAIN_MASK,
@@ -126,13 +133,6 @@ static const struct rpr0521_gain_info {
 		.gain	= rpr0521_als_gain,
 		.size	= ARRAY_SIZE(rpr0521_als_gain),
 	},
-	[RPR0521_CHAN_PXS] = {
-		.reg	= RPR0521_REG_PXS_CTRL,
-		.mask	= RPR0521_PXS_GAIN_MASK,
-		.shift	= RPR0521_PXS_GAIN_SHIFT,
-		.gain	= rpr0521_pxs_gain,
-		.size	= ARRAY_SIZE(rpr0521_pxs_gain),
-	},
 };
 
 struct rpr0521_samp_freq {
@@ -197,6 +197,14 @@ static const struct attribute_group rpr0521_attribute_group = {
 
 static const struct iio_chan_spec rpr0521_channels[] = {
 	{
+		.type = IIO_PROXIMITY,
+		.address = RPR0521_CHAN_PXS,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_OFFSET) |
+			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	},
+	{
 		.type = IIO_INTENSITY,
 		.modified = 1,
 		.address = RPR0521_CHAN_ALS_DATA0,
@@ -214,15 +222,6 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
-	{
-		.type = IIO_PROXIMITY,
-		.address = RPR0521_CHAN_PXS,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_OFFSET) |
-			BIT(IIO_CHAN_INFO_SCALE),
-
-		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
-	}
 };
 
 static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
-- 
1.7.9.5

[PATCH v2 6/7] iio: light: rpr0521 triggered buffer added

[Mikko Koivunen]

Driver sets up and uses triggered buffer if there is irq defined for device in
device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
Trigger consumer reads rpr0521 data to scan buffer.
Depends on previous commits of _scale and _offset.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
Builds OK with torvalds/linux 4.9
checkpatch.pl OK.

Patch v1->v2 changes:
removed flagging of the new feature
removed unnecesssary rpr0521_update_scan_mode()
removed unnecessary comments.
added IIO_LE to channel spec

 drivers/iio/light/rpr0521.c |  287 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 282 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index dc522fd..72f608d 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -9,9 +9,10 @@
  *
  * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
  *
- * TODO: illuminance channel, PM support, buffer
+ * TODO: illuminance channel, PM support
  */
 
+
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
@@ -20,6 +21,10 @@
 #include <linux/acpi.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/iio/sysfs.h>
 #include <linux/pm_runtime.h>
 
@@ -30,6 +35,7 @@
 #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
+#define RPR0521_REG_INTERRUPT		0x4A
 #define RPR0521_PS_OFFSET_LSB		0x53
 #define RPR0521_PS_OFFSET_MSB		0x54
 
@@ -44,16 +50,33 @@
 #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
 #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
 #define RPR0521_PXS_GAIN_SHIFT		4
+#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
+#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
+#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
+#define RPR0521_INTERRUPT_INT_LATCH_MASK	BIT(2)
+#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
+#define RPR0521_INTERRUPT_INT_MODE_MASK		GENMASK(5, 4)
 
 #define RPR0521_MODE_ALS_ENABLE		BIT(7)
 #define RPR0521_MODE_ALS_DISABLE	0x00
 #define RPR0521_MODE_PXS_ENABLE		BIT(6)
 #define RPR0521_MODE_PXS_DISABLE	0x00
+#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
+#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
+#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
+#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
+#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
+#define RPR0521_INTERRUPT_INT_LATCH_DISABLE	BIT(2)
+#define RPR0521_INTERRUPT_INT_LATCH_ENABLE	0x00
+#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
+#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
+#define RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT	BIT(5)
 
 #define RPR0521_MANUFACT_ID		0xE0
 #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
 
 #define RPR0521_DRV_NAME		"RPR0521"
+#define RPR0521_IRQ_NAME		"rpr0521_event"
 #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
 
 #define RPR0521_SLEEP_DELAY_MS	2000
@@ -169,6 +192,9 @@ struct rpr0521_data {
 	bool als_dev_en;
 	bool pxs_dev_en;
 
+	struct iio_trigger *drdy_trigger0;
+	bool drdy_trigger_on;
+
 	/* optimize runtime pm ops - enable device only if needed */
 	bool als_ps_need_en;
 	bool pxs_ps_need_en;
@@ -195,6 +221,13 @@ static const struct attribute_group rpr0521_attribute_group = {
 	.attrs = rpr0521_attributes,
 };
 
+/* Order of the channel data in buffer */
+enum rpr0521_scan_index_order {
+	RPR0521_CHAN_INDEX_PXS,
+	RPR0521_CHAN_INDEX_BOTH,
+	RPR0521_CHAN_INDEX_IR,
+};
+
 static const struct iio_chan_spec rpr0521_channels[] = {
 	{
 		.type = IIO_PROXIMITY,
@@ -203,6 +236,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 			BIT(IIO_CHAN_INFO_OFFSET) |
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_index = RPR0521_CHAN_INDEX_PXS,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.shift = 0,
+			.endianness = IIO_LE,
+		},
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -212,6 +253,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_index = RPR0521_CHAN_INDEX_BOTH,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.shift = 0,
+			.endianness = IIO_LE,
+		},
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -221,9 +270,154 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_index = RPR0521_CHAN_INDEX_IR,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.shift = 0,
+			.endianness = IIO_LE,
+		},
 	},
 };
 
+static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
+				   u8 device_mask);
+static int rpr0521_als_enable(struct rpr0521_data *data, u8 status);
+static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status);
+
+/* IRQ to trigger -handler */
+static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct rpr0521_data *data = iio_priv(indio_dev);
+
+	/* Notify trigger0 consumers/subscribers */
+	if (data->drdy_trigger_on)
+		iio_trigger_poll(data->drdy_trigger0);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct rpr0521_data *data = iio_priv(indio_dev);
+	int ret;
+
+	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
+
+	ret = regmap_bulk_read(data->regmap,
+		RPR0521_REG_PXS_DATA,
+		&buffer,
+		(3*2)+1);	/* 3 * 16-bit + (discarded) int clear reg. */
+	if (ret < 0)
+		goto done;
+
+	iio_push_to_buffers_with_timestamp(indio_dev,
+		buffer,
+		pf->timestamp);
+done:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int rpr0521_write_int_enable(struct rpr0521_data *data)
+{
+	int err;
+
+	/* Interrupt after each measurement */
+	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
+		RPR0521_PXS_PERSISTENCE_MASK,
+		RPR0521_PXS_PERSISTENCE_DRDY);
+	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
+		RPR0521_INTERRUPT_INT_REASSERT_MASK |
+		RPR0521_INTERRUPT_INT_LATCH_MASK |
+		RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
+		RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
+		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
+		RPR0521_INTERRUPT_INT_LATCH_ENABLE |
+		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
+		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
+		);
+	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
+		RPR0521_INTERRUPT_INT_MODE_MASK,
+		RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT);
+	if (err) {
+		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
+		return -EBUSY;
+		}
+	return 0;
+}
+
+static int rpr0521_write_int_disable(struct rpr0521_data *data)
+{
+	return regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
+				RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
+				RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
+				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
+				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
+				);
+}
+
+/* Trigger -enable/disable */
+static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
+	bool enable_drdy)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
+	struct rpr0521_data *data = iio_priv(indio_dev);
+	int err;
+
+	if (enable_drdy) {
+		/* ENABLE DRDY */
+		mutex_lock(&data->lock);
+		err = rpr0521_set_power_state(data, true,
+			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
+		mutex_unlock(&data->lock);
+		if (err)
+			goto rpr0521_set_state_err;
+		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
+		if (err)
+			goto rpr0521_set_state_err;
+		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
+		if (err)
+			goto rpr0521_set_state_err;
+		err = rpr0521_write_int_enable(data);
+		if (err)
+			goto rpr0521_set_state_err;
+	} else {
+		/* DISABLE DRDY */
+		mutex_lock(&data->lock);
+		err = rpr0521_set_power_state(data, false,
+			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
+		mutex_unlock(&data->lock);
+		if (err)
+			goto rpr0521_set_state_err;
+		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_DISABLE);
+		if (err)
+			goto rpr0521_set_state_err;
+		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_DISABLE);
+		if (err)
+			goto rpr0521_set_state_err;
+		err = rpr0521_write_int_disable(data);
+		if (err)
+			goto rpr0521_set_state_err;
+	}
+	data->drdy_trigger_on = enable_drdy;
+	return 0;
+
+rpr0521_set_state_err:
+	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
+	return err;
+}
+
+static const struct iio_trigger_ops rpr0521_trigger_ops = {
+	.set_trigger_state = rpr0521_pxs_drdy_set_state,
+	.owner = THIS_MODULE,
+	};
+
 static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
 {
 	int ret;
@@ -454,6 +648,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
 			return -EINVAL;
 
+		if (iio_buffer_enabled(indio_dev))
+			return -EBUSY;
+
 		device_mask = rpr0521_data_reg[chan->address].device_mask;
 
 		mutex_lock(&data->lock);
@@ -664,20 +861,90 @@ static int rpr0521_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = pm_runtime_set_active(&client->dev);
-	if (ret < 0)
-		return ret;
+	/* IRQ to trigger setup */
+	if (client->irq) {
+		/* Trigger0 producer setup */
+		data->drdy_trigger0 = devm_iio_trigger_alloc(
+			indio_dev->dev.parent,
+			"%s-dev%d",
+			indio_dev->name,
+			indio_dev->id);
+		if (!data->drdy_trigger0) {
+			ret = -ENOMEM;
+			return ret;
+		}
+		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
+		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
+		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
+
+		ret = iio_trigger_register(data->drdy_trigger0);
+		if (ret) {
+			dev_err(&client->dev, "iio trigger register failed\n");
+			return ret;
+		}
 
+		/* Set trigger0 as default trigger (and inc refcount) */
+		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
+
+		/* Ties irq to trigger producer handler. */
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+			rpr0521_drdy_irq_handler,
+			NULL,
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			RPR0521_IRQ_NAME,
+			indio_dev);
+		if (ret < 0) {
+			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
+				client->irq);
+			goto err_iio_trigger_put_unregister;
+			}
+		/*
+		 * Now whole pipe from physical interrupt (irq defined by
+		 * devicetree to device) to trigger0 output is set up.
+		 */
+
+		/* Trigger consumer setup */
+		ret = iio_triggered_buffer_setup(indio_dev,
+			&iio_pollfunc_store_time,
+			rpr0521_trigger_consumer_handler,
+			NULL);		//Use default _ops
+		if (ret < 0) {
+			dev_err(&client->dev, "iio triggered buffer setup failed\n");
+			/* Count on devm to free_irq */
+			goto err_iio_trigger_put_unregister;
+		}
+	}
+
+	ret = pm_runtime_set_active(&client->dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "set_active failed\n");
+		goto err_iio_unregister_trigger_consumer;
+		}
 	pm_runtime_enable(&client->dev);
 	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
 	pm_runtime_use_autosuspend(&client->dev);
 
 	return iio_device_register(indio_dev);
+
+
+err_iio_unregister_trigger_consumer:
+	iio_triggered_buffer_cleanup(indio_dev);
+err_iio_trigger_put_unregister:
+	if (indio_dev->trig) {
+		iio_trigger_put(indio_dev->trig);
+		indio_dev->trig = NULL;
+		}
+	if (data->drdy_trigger0) {
+		iio_trigger_unregister(data->drdy_trigger0);
+		data->drdy_trigger0 = NULL;
+		}
+	return ret;
 }
 
 static int rpr0521_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct rpr0521_data *data = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
 
@@ -685,8 +952,18 @@ static int rpr0521_remove(struct i2c_client *client)
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_put_noidle(&client->dev);
 
-	rpr0521_poweroff(iio_priv(indio_dev));
+	rpr0521_poweroff(data);
 
+	iio_triggered_buffer_cleanup(indio_dev);
+	if (indio_dev->trig) {
+		iio_trigger_put(indio_dev->trig);
+		indio_dev->trig = NULL;
+		}
+	if (data->drdy_trigger0) {
+		iio_trigger_unregister(data->drdy_trigger0);
+		data->drdy_trigger0 = NULL;
+		}
+	/* Rest of the cleanup is done by devm. */
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH v2 7/7] iio: light: rpr0521 magic number to sizeof() on value read

[Mikko Koivunen]

Changed magic number to sizeof() on value read.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Tested on LeMaker HiKey AOSP7, kernel 4.4.
checkpatch.pl ok
---
 drivers/iio/light/rpr0521.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 72f608d..75fd3ca 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -662,7 +662,7 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 
 		ret = regmap_bulk_read(data->regmap,
 				       rpr0521_data_reg[chan->address].address,
-				       &raw_data, 2);
+				       &raw_data, sizeof(raw_data));
 		if (ret < 0) {
 			rpr0521_set_power_state(data, false, device_mask);
 			mutex_unlock(&data->lock);
-- 
1.7.9.5

Re: [PATCH v2 1/7] iio: light: rpr0521 disable sensor -bugfix

[Jonathan Cameron]

On 07/04/17 13:07, Mikko Koivunen wrote:
> Sensor was marked enabled on each call even if the call was for disabling
> sensor.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
What effect does this have on device operation?
What I'm looking for is info on whether this wants to go to stable as
it will cause the device not to work, or whether it is wasting power etc.

Also useful to add a fixes tag as that makes it easy for stable maintainers
to filter incoming patches for relevance to their trees.

Will be looking for Acks from Daniel on all of these as he wrote the original
and is still active.

Jonathan
> ---
> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
> 
>  drivers/iio/light/rpr0521.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 7de0f39..c15529b 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -197,8 +197,10 @@ static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>  	if (ret < 0)
>  		return ret;
>  
> -	data->als_dev_en = true;
> -
> +	if (status & RPR0521_MODE_ALS_MASK)
> +		data->als_dev_en = true;
> +	else
> +		data->als_dev_en = false;
>  	return 0;
>  }
>  
> @@ -212,7 +214,10 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
>  	if (ret < 0)
>  		return ret;
>  
> -	data->pxs_dev_en = true;
> +	if (status & RPR0521_MODE_PXS_MASK)
> +		data->pxs_dev_en = true;
> +	else
> +		data->pxs_dev_en = false;
>  
>  	return 0;
>  }
> 

Re: [PATCH v2 2/7] iio: light: rpr0521 whitespace fixes

[Jonathan Cameron]

On 07/04/17 13:07, Mikko Koivunen wrote:
> Just whitespace changes, no functional changes.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> ---
>  drivers/iio/light/rpr0521.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index c15529b..30c2592 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -357,8 +357,8 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  
>  		*val = le16_to_cpu(raw_data);
> -
I'd argue that there are good readability reasons to leave this blank line.
If the original driver hadn't had it I wouldn't have felt strongly enough
to comment on it, but adding noise to remove it seems rather pointless..
>  		return IIO_VAL_INT;
> +
Adding this one is definitely worthwhile though!
>  	case IIO_CHAN_INFO_SCALE:
>  		mutex_lock(&data->lock);
>  		ret = rpr0521_get_gain(data, chan->address, val, val2);
> 

Re: [PATCH v2 3/7] iio: light: rpr0521 sample_frequency read/write added

[Jonathan Cameron]

On 07/04/17 13:07, Mikko Koivunen wrote:
> Added sysfs read/write sample frequency.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
A few comments inline but looks basically good to me.

Odd little bit of hardware!

Jonathan
> ---
> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
> 
> Patch v1->v2 changes:
> multiline comments fixed
> 
>  drivers/iio/light/rpr0521.c |   99 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 30c2592..e92a8bb 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -132,6 +132,30 @@ static const struct rpr0521_gain_info {
>  	},
>  };
>  
> +struct rpr0521_samp_freq {
> +	int	als_hz;
> +	int	als_uhz;
> +	int	pxs_hz;
> +	int	pxs_uhz;
> +};
> +
> +static const struct rpr0521_samp_freq rpr0521_samp_freq_i[13] = {
> +/*	{ALS, PXS} */
Hmm. You have them all listed here so that you can use the index as
the register value.    Maybe add a symbol to indicate in the comment
which ones you can actually use here...
> +	{0, 0, 0, 0},		/* 0000, 0=standby */
> +	{0, 0, 100, 0},		/* 0001 */
> +	{0, 0, 25, 0},		/* 0010 */
> +	{0, 0, 10, 0},		/* 0011 */
> +	{0, 0, 2, 500000},	/* 0100 */
> +	{10, 0, 20, 0},		/* 0101 */
> +	{10, 0, 10, 0},		/* 0110 */
> +	{10, 0, 2, 500000},	/* 0111 */
> +	{2, 500000, 20, 0},	/* 1000, measurement 100ms, sleep 300ms */
> +	{2, 500000, 10, 0},	/* 1001, measurement 100ms, sleep 300ms */
> +	{2, 500000, 0, 0},	/* 1010, high sensitivity mode */
> +	{2, 500000, 2, 500000},	/* 1011, high sensitivity mode */
> +	{20, 0, 20, 0}	/* 1100, ALS_data x 0.5, see specification P.18 */
Whilst this one is odd, I don't think your write function below will refuse
to set it...
> +};
> +
>  struct rpr0521_data {
>  	struct i2c_client *client;
>  
> @@ -152,9 +176,15 @@ struct rpr0521_data {
>  static IIO_CONST_ATTR(in_intensity_scale_available, RPR0521_ALS_SCALE_AVAIL);
>  static IIO_CONST_ATTR(in_proximity_scale_available, RPR0521_PXS_SCALE_AVAIL);
>  
> +/* Start with easy freq first, whole table of freq combinations is more
/*
 * Start...

(kernel multiline comment syntax)...
> + * complicated.
> + */
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("2.5 10");
20 not available?
> +
>  static struct attribute *rpr0521_attributes[] = {
>  	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
>  	&iio_const_attr_in_proximity_scale_available.dev_attr.attr,
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>  	NULL,
>  };
>  
> @@ -170,6 +200,7 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  		.channel2 = IIO_MOD_LIGHT_BOTH,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	},
>  	{
>  		.type = IIO_INTENSITY,
> @@ -178,12 +209,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  		.channel2 = IIO_MOD_LIGHT_IR,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	},
>  	{
>  		.type = IIO_PROXIMITY,
>  		.address = RPR0521_CHAN_PXS,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	}
>  };
>  
> @@ -319,6 +352,56 @@ static int rpr0521_set_gain(struct rpr0521_data *data, int chan,
>  				  idx << rpr0521_gain[chan].shift);
>  }
>  
> +static int rpr0521_read_samp_freq(struct rpr0521_data *data,
> +				enum iio_chan_type chan_type,
> +			    int *val, int *val2)
> +{
> +	int reg, ret;
> +
> +	ret = regmap_read(data->regmap, RPR0521_REG_MODE_CTRL, &reg);
> +	if (ret < 0)
> +		return ret;
I'd put a blank line here..
> +	reg &= RPR0521_MODE_MEAS_TIME_MASK;
> +	if (reg >= ARRAY_SIZE(rpr0521_samp_freq_i))
> +		return -EINVAL;
> +
> +	if (chan_type == IIO_INTENSITY) {
> +		*val = rpr0521_samp_freq_i[reg].als_hz;
> +		*val2 = rpr0521_samp_freq_i[reg].als_uhz;
> +	} else if (chan_type == IIO_PROXIMITY) {
> +		*val = rpr0521_samp_freq_i[reg].pxs_hz;
> +		*val2 = rpr0521_samp_freq_i[reg].pxs_uhz;
> +	} else {
> +		return -EINVAL;
switch might be tidier..  Also, perhaps return directly from first two
cases.
> +	}
> +	return 0;
> +}
> +
> +static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
> +				enum iio_chan_type chan_type,
> +				int val, int val2)
> +{
> +	int i;
> +
> +	/* Ignore channel
/*
 * Ignore channel
 ...
> +	 * both pxs and als are setup only to same freq because of simplicity
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(rpr0521_samp_freq_i); i++)
> +		if ((val == rpr0521_samp_freq_i[i].als_hz)
> +			&& (val2 == rpr0521_samp_freq_i[i].als_uhz)
> +			&& (val == rpr0521_samp_freq_i[i].pxs_hz)
> +			&& (val2 == rpr0521_samp_freq_i[i].pxs_uhz)) {
Hmm. a lot of complexity introduced above by sort of allowing for them
to be different (which would indeed be a real pain to handle! Why not
just drop the ones you can never actually use?

> +			break;
> +		}
> +	if (i == ARRAY_SIZE(rpr0521_samp_freq_i))
> +		return -EINVAL;
> +
> +	return regmap_update_bits(data->regmap,
> +		RPR0521_REG_MODE_CTRL,
> +		RPR0521_MODE_MEAS_TIME_MASK,
> +		i);
> +}
> +
>  static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int *val,
>  			    int *val2, long mask)
> @@ -365,8 +448,16 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  		mutex_unlock(&data->lock);
>  		if (ret < 0)
>  			return ret;
> +		return IIO_VAL_INT_PLUS_MICRO;
>  
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&data->lock);
> +		ret = rpr0521_read_samp_freq(data, chan->type, val, val2);
> +		mutex_unlock(&data->lock);
> +		if (ret < 0)
> +			return ret;
>  		return IIO_VAL_INT_PLUS_MICRO;
> +
Again, I think you are messing with original white space.  Don't do
that as it just gives odd diff results like this one...
>  	default:
>  		return -EINVAL;
>  	}
> @@ -384,8 +475,16 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>  		mutex_lock(&data->lock);
>  		ret = rpr0521_set_gain(data, chan->address, val, val2);
>  		mutex_unlock(&data->lock);
You are removing the blank line here.. (I think anyway!). Not a change
that should be in this patch (or possibly happen at all - this one
is in the personal taste category).  If nothing else it made diff less
readable!
> +		return ret;
>  
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&data->lock);
> +		ret = rpr0521_write_samp_freq_common(data,
> +			chan->type,
> +			val, val2);
> +		mutex_unlock(&data->lock);
>  		return ret;
> +
>  	default:
>  		return -EINVAL;
>  	}
> 

Re: [PATCH v2 4/7] iio: light: rpr0521 proximity offset read/write added

[Jonathan Cameron]

On 07/04/17 13:07, Mikko Koivunen wrote:
> Added sysfs read/write proximity offset feature. Offset is read/write from
> sensor registers. Values are proximity raw 10-bit values. After applying
> offset value, output values will be (measured_raw - offset_value). Output
> values are unsigned so offset value doesn't make output negative.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> ---
A few bits and bobs inline.

Jonathan
> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
> Builds OK with torvalds/linux 4.9
> checkpatch.pl OK.
> 
> Patch v1->v2 changes:
> read/write_ps_offset() rewritten with:
>  -  static, __le16, cpu_to_le16(), sizeof()
> 
>  drivers/iio/light/rpr0521.c |   53 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index e92a8bb..35c759b 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -30,6 +30,9 @@
>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
> +#define RPR0521_PS_OFFSET_LSB		0x53
> +#define RPR0521_PS_OFFSET_MSB		0x54
Not used, so drop it.
> +
Given previous naming always has REG_ in it you should maintain that..
Also, why the blank line here?
>  #define RPR0521_REG_ID			0x92
>  
>  #define RPR0521_MODE_ALS_MASK		BIT(7)
> @@ -215,7 +218,9 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  		.type = IIO_PROXIMITY,
>  		.address = RPR0521_CHAN_PXS,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_OFFSET) |
>  			BIT(IIO_CHAN_INFO_SCALE),
This blank line shouldn't be added here. Nothing to do with tis patch.
> +
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	}
>  };
> @@ -402,6 +407,40 @@ static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
>  		i);
>  }
>  
> +static int rpr0521_read_ps_offset(struct rpr0521_data *data, int *offset)
> +{
> +	int ret;
> +	__le16 buffer;
> +
> +	ret = regmap_bulk_read(data->regmap,
> +		RPR0521_PS_OFFSET_LSB, &buffer, sizeof(buffer));
> +
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Failed to read PS OFFSET register\n");
> +		return ret;
> +	}
> +	*offset = le16_to_cpu(buffer);
> +
> +	return ret;
> +}
> +
> +static int rpr0521_write_ps_offset(struct rpr0521_data *data, int offset)
> +{
> +	int ret;
> +	__le16 buffer;
> +
> +	buffer = cpu_to_le16(offset & 0x3ff);
> +	ret = regmap_raw_write(data->regmap,
> +		RPR0521_PS_OFFSET_LSB, &buffer, sizeof(buffer));
> +
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Failed to write PS OFFSET register\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
>  static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int *val,
>  			    int *val2, long mask)
> @@ -458,6 +497,14 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  
> +	case IIO_CHAN_INFO_OFFSET:
> +		mutex_lock(&data->lock);
> +		ret = rpr0521_read_ps_offset(data, val);
> +		mutex_unlock(&data->lock);
> +		if (ret < 0)
> +			return ret;
Slight preference for a blank line here.
> +		return IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -485,6 +532,12 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>  		mutex_unlock(&data->lock);
>  		return ret;
>  
> +	case IIO_CHAN_INFO_OFFSET:
> +		mutex_lock(&data->lock);
> +		ret = rpr0521_write_ps_offset(data, val);
> +		mutex_unlock(&data->lock);
> +		return ret;
> +
>  	default:
>  		return -EINVAL;
>  	}
> 

Re: [PATCH v2 5/7] iio: light: rpr0521 channel numbers reordered

[Jonathan Cameron]

On 07/04/17 13:07, Mikko Koivunen wrote:
> Moved proximity channel from last to first in structs to avoid confusion
> later with buffered triggers. Proximity data output is first in rpr0521
> register map.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
Doesn't result in any ABI change that I can see so fine by me if
it makes your code a bit simpler.
> ---
>  drivers/iio/light/rpr0521.c |   41 ++++++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 35c759b..dc522fd 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -80,9 +80,9 @@ static const struct rpr0521_gain rpr0521_pxs_gain[3] = {
>  };
>  
>  enum rpr0521_channel {
> +	RPR0521_CHAN_PXS,
>  	RPR0521_CHAN_ALS_DATA0,
>  	RPR0521_CHAN_ALS_DATA1,
> -	RPR0521_CHAN_PXS,
>  };
>  
>  struct rpr0521_reg_desc {
> @@ -91,6 +91,10 @@ struct rpr0521_reg_desc {
>  };
>  
>  static const struct rpr0521_reg_desc rpr0521_data_reg[] = {
> +	[RPR0521_CHAN_PXS]	= {
> +		.address	= RPR0521_REG_PXS_DATA,
> +		.device_mask	= RPR0521_MODE_PXS_MASK,
> +	},
>  	[RPR0521_CHAN_ALS_DATA0] = {
>  		.address	= RPR0521_REG_ALS_DATA0,
>  		.device_mask	= RPR0521_MODE_ALS_MASK,
> @@ -99,10 +103,6 @@ static const struct rpr0521_reg_desc rpr0521_data_reg[] = {
>  		.address	= RPR0521_REG_ALS_DATA1,
>  		.device_mask	= RPR0521_MODE_ALS_MASK,
>  	},
> -	[RPR0521_CHAN_PXS]	= {
> -		.address	= RPR0521_REG_PXS_DATA,
> -		.device_mask	= RPR0521_MODE_PXS_MASK,
> -	},
>  };
>  
>  static const struct rpr0521_gain_info {
> @@ -112,6 +112,13 @@ static const struct rpr0521_gain_info {
>  	const struct rpr0521_gain *gain;
>  	int size;
>  } rpr0521_gain[] = {
> +	[RPR0521_CHAN_PXS] = {
> +		.reg	= RPR0521_REG_PXS_CTRL,
> +		.mask	= RPR0521_PXS_GAIN_MASK,
> +		.shift	= RPR0521_PXS_GAIN_SHIFT,
> +		.gain	= rpr0521_pxs_gain,
> +		.size	= ARRAY_SIZE(rpr0521_pxs_gain),
> +	},
>  	[RPR0521_CHAN_ALS_DATA0] = {
>  		.reg	= RPR0521_REG_ALS_CTRL,
>  		.mask	= RPR0521_ALS_DATA0_GAIN_MASK,
> @@ -126,13 +133,6 @@ static const struct rpr0521_gain_info {
>  		.gain	= rpr0521_als_gain,
>  		.size	= ARRAY_SIZE(rpr0521_als_gain),
>  	},
> -	[RPR0521_CHAN_PXS] = {
> -		.reg	= RPR0521_REG_PXS_CTRL,
> -		.mask	= RPR0521_PXS_GAIN_MASK,
> -		.shift	= RPR0521_PXS_GAIN_SHIFT,
> -		.gain	= rpr0521_pxs_gain,
> -		.size	= ARRAY_SIZE(rpr0521_pxs_gain),
> -	},
>  };
>  
>  struct rpr0521_samp_freq {
> @@ -197,6 +197,14 @@ static const struct attribute_group rpr0521_attribute_group = {
>  
>  static const struct iio_chan_spec rpr0521_channels[] = {
>  	{
> +		.type = IIO_PROXIMITY,
> +		.address = RPR0521_CHAN_PXS,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_OFFSET) |
> +			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	},
> +	{
>  		.type = IIO_INTENSITY,
>  		.modified = 1,
>  		.address = RPR0521_CHAN_ALS_DATA0,
> @@ -214,15 +222,6 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	},
> -	{
> -		.type = IIO_PROXIMITY,
> -		.address = RPR0521_CHAN_PXS,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -			BIT(IIO_CHAN_INFO_OFFSET) |
> -			BIT(IIO_CHAN_INFO_SCALE),
> -
> -		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> -	}
>  };
>  
>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
> 

Re: [PATCH v2 6/7] iio: light: rpr0521 triggered buffer added

[Jonathan Cameron]

On 07/04/17 13:07, Mikko Koivunen wrote:
> Driver sets up and uses triggered buffer if there is irq defined for device in
> device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
> Trigger consumer reads rpr0521 data to scan buffer.
> Depends on previous commits of _scale and _offset.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> ---
A few comments inline...

Jonathan
> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
> Builds OK with torvalds/linux 4.9
> checkpatch.pl OK.
> 
> Patch v1->v2 changes:
> removed flagging of the new feature
> removed unnecesssary rpr0521_update_scan_mode()
> removed unnecessary comments.
> added IIO_LE to channel spec
> 
>  drivers/iio/light/rpr0521.c |  287 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 282 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index dc522fd..72f608d 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -9,9 +9,10 @@
>   *
>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>   *
> - * TODO: illuminance channel, PM support, buffer
> + * TODO: illuminance channel, PM support
>   */
>  
> +
Be careful to clear out stray whitespace changes like this one.
Just add noise to the patch and may mean it takes longer to be accepted...
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
> @@ -20,6 +21,10 @@
>  #include <linux/acpi.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/pm_runtime.h>
>  
> @@ -30,6 +35,7 @@
>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
> +#define RPR0521_REG_INTERRUPT		0x4A
>  #define RPR0521_PS_OFFSET_LSB		0x53
>  #define RPR0521_PS_OFFSET_MSB		0x54
>  
> @@ -44,16 +50,33 @@
>  #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
>  #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
>  #define RPR0521_PXS_GAIN_SHIFT		4
> +#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
> +#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
> +#define RPR0521_INTERRUPT_INT_LATCH_MASK	BIT(2)
> +#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
> +#define RPR0521_INTERRUPT_INT_MODE_MASK		GENMASK(5, 4)
>  
>  #define RPR0521_MODE_ALS_ENABLE		BIT(7)
>  #define RPR0521_MODE_ALS_DISABLE	0x00
>  #define RPR0521_MODE_PXS_ENABLE		BIT(6)
>  #define RPR0521_MODE_PXS_DISABLE	0x00
> +#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
> +#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
> +#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
> +#define RPR0521_INTERRUPT_INT_LATCH_DISABLE	BIT(2)
> +#define RPR0521_INTERRUPT_INT_LATCH_ENABLE	0x00
> +#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
> +#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
> +#define RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT	BIT(5)
>  
>  #define RPR0521_MANUFACT_ID		0xE0
>  #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
>  
>  #define RPR0521_DRV_NAME		"RPR0521"
> +#define RPR0521_IRQ_NAME		"rpr0521_event"
>  #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
>  
>  #define RPR0521_SLEEP_DELAY_MS	2000
> @@ -169,6 +192,9 @@ struct rpr0521_data {
>  	bool als_dev_en;
>  	bool pxs_dev_en;
>  
> +	struct iio_trigger *drdy_trigger0;
> +	bool drdy_trigger_on;
> +
>  	/* optimize runtime pm ops - enable device only if needed */
>  	bool als_ps_need_en;
>  	bool pxs_ps_need_en;
> @@ -195,6 +221,13 @@ static const struct attribute_group rpr0521_attribute_group = {
>  	.attrs = rpr0521_attributes,
>  };
>  
> +/* Order of the channel data in buffer */
> +enum rpr0521_scan_index_order {
> +	RPR0521_CHAN_INDEX_PXS,
> +	RPR0521_CHAN_INDEX_BOTH,
> +	RPR0521_CHAN_INDEX_IR,
> +};
> +
>  static const struct iio_chan_spec rpr0521_channels[] = {
>  	{
>  		.type = IIO_PROXIMITY,
> @@ -203,6 +236,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  			BIT(IIO_CHAN_INFO_OFFSET) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.scan_index = RPR0521_CHAN_INDEX_PXS,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.shift = 0,
> +			.endianness = IIO_LE,
> +		},
>  	},
>  	{
>  		.type = IIO_INTENSITY,
> @@ -212,6 +253,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.scan_index = RPR0521_CHAN_INDEX_BOTH,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.shift = 0,
> +			.endianness = IIO_LE,
> +		},
>  	},
>  	{
>  		.type = IIO_INTENSITY,
> @@ -221,9 +270,154 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.scan_index = RPR0521_CHAN_INDEX_IR,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.shift = 0,
shift default makes sense as 0 so don't bother specifying it
(will be set to 0 anyway)
> +			.endianness = IIO_LE,
> +		},
>  	},
>  };
>  
> +static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
> +				   u8 device_mask);
> +static int rpr0521_als_enable(struct rpr0521_data *data, u8 status);
> +static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status);
These forward definitions rather look like they could be removed with a bit
of reorganization fo the file.  Do that in preference to having these in
the middle of the code.
> +
> +/* IRQ to trigger -handler */
why the -?
> +static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct rpr0521_data *data = iio_priv(indio_dev);
> +
> +	/* Notify trigger0 consumers/subscribers */
> +	if (data->drdy_trigger_on)
> +		iio_trigger_poll(data->drdy_trigger0);
How else are you getting here? Also the core will be fine
if nothing cares and the trigger is polled.

If there is another cause, we aren't handling it so should claim
to be doing so.
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct rpr0521_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
> +
> +	ret = regmap_bulk_read(data->regmap,
> +		RPR0521_REG_PXS_DATA,
> +		&buffer,
> +		(3*2)+1);	/* 3 * 16-bit + (discarded) int clear reg. */
kernel style is spaces around the * and +
> +	if (ret < 0)
> +		goto done;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev,
> +		buffer,
Dont wrap more than you have two.  So make this two lines perhaps.
> +		pf->timestamp);
> +done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int rpr0521_write_int_enable(struct rpr0521_data *data)
> +{
> +	int err;
> +
> +	/* Interrupt after each measurement */
> +	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
> +		RPR0521_PXS_PERSISTENCE_MASK,
> +		RPR0521_PXS_PERSISTENCE_DRDY);
> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
umm. really?  All of them? Are we looking at a clear the whole register
case?  If so we should probably only clear relevant ones..

I don't like these stacked error handlers (err = err | *).
Just handle each err as and when.  It's more code, but easier
to review / follow.

> +		RPR0521_INTERRUPT_INT_REASSERT_MASK |
> +		RPR0521_INTERRUPT_INT_LATCH_MASK |
> +		RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
> +		RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
> +		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
> +		RPR0521_INTERRUPT_INT_LATCH_ENABLE |
> +		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
> +		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
> +		);
> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
> +		RPR0521_INTERRUPT_INT_MODE_MASK,
> +		RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT);
> +	if (err) {
> +		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
> +		return -EBUSY;
> +		}
> +	return 0;
> +}
> +
> +static int rpr0521_write_int_disable(struct rpr0521_data *data)
> +{
> +	return regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
> +				RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
> +				RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
> +				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
> +				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
> +				);
> +}
> +
> +/* Trigger -enable/disable */
> +static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
> +	bool enable_drdy)
> +{
This may be called from the moment iio_trigger_register is called until
the trigger is unregistered on remove.

Could that potentially be a problem as it might occur after the device
is powered off in remove?  Hard to be sure without diving into the data
sheet.  If it if safe as it is, then use the devm functions as described
below, but perhaps put a note here about the fact it is safe to have
this potential race.
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
> +	struct rpr0521_data *data = iio_priv(indio_dev);
> +	int err;
> +
> +	if (enable_drdy) {
> +		/* ENABLE DRDY */
> +		mutex_lock(&data->lock);
> +		err = rpr0521_set_power_state(data, true,
> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
> +		mutex_unlock(&data->lock);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +		err = rpr0521_write_int_enable(data);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +	} else {
> +		/* DISABLE DRDY */
> +		mutex_lock(&data->lock);
> +		err = rpr0521_set_power_state(data, false,
> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
> +		mutex_unlock(&data->lock);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_DISABLE);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_DISABLE);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +		err = rpr0521_write_int_disable(data);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +	}
> +	data->drdy_trigger_on = enable_drdy;
> +	return 0;
> +
> +rpr0521_set_state_err:
> +	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
> +	return err;
> +}
> +
> +static const struct iio_trigger_ops rpr0521_trigger_ops = {
> +	.set_trigger_state = rpr0521_pxs_drdy_set_state,
> +	.owner = THIS_MODULE,
> +	};
> +
>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>  {
>  	int ret;
> @@ -454,6 +648,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
>  			return -EINVAL;
>  
> +		if (iio_buffer_enabled(indio_dev))
> +			return -EBUSY;
> +
>  		device_mask = rpr0521_data_reg[chan->address].device_mask;
>  
>  		mutex_lock(&data->lock);
> @@ -664,20 +861,90 @@ static int rpr0521_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	ret = pm_runtime_set_active(&client->dev);
> -	if (ret < 0)
> -		return ret;
> +	/* IRQ to trigger setup */
> +	if (client->irq) {
> +		/* Trigger0 producer setup */
> +		data->drdy_trigger0 = devm_iio_trigger_alloc(
> +			indio_dev->dev.parent,
> +			"%s-dev%d",
> +			indio_dev->name,
> +			indio_dev->id);
> +		if (!data->drdy_trigger0) {
> +			ret = -ENOMEM;
> +			return ret;
> +		}
> +		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
> +		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
> +		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
> +
> +		ret = iio_trigger_register(data->drdy_trigger0);
devm_iio_trigger_register should be fine
> +		if (ret) {
> +			dev_err(&client->dev, "iio trigger register failed\n");
> +			return ret;
> +		}
>  
> +		/* Set trigger0 as default trigger (and inc refcount) */
> +		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
> +
> +		/* Ties irq to trigger producer handler. */
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +			rpr0521_drdy_irq_handler,
> +			NULL,
> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			RPR0521_IRQ_NAME,
> +			indio_dev);
> +		if (ret < 0) {
> +			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
> +				client->irq);
> +			goto err_iio_trigger_put_unregister;
> +			}
> +		/*
> +		 * Now whole pipe from physical interrupt (irq defined by
> +		 * devicetree to device) to trigger0 output is set up.
> +		 */
> +
> +		/* Trigger consumer setup */
> +		ret = iio_triggered_buffer_setup(indio_dev,
> +			&iio_pollfunc_store_time,
> +			rpr0521_trigger_consumer_handler,
> +			NULL);		//Use default _ops
> +		if (ret < 0) {
> +			dev_err(&client->dev, "iio triggered buffer setup failed\n");
> +			/* Count on devm to free_irq */
> +			goto err_iio_trigger_put_unregister;
> +		}
> +	}
> +
> +	ret = pm_runtime_set_active(&client->dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "set_active failed\n");
I'm not particularly bothered either way by the change, but technically
adding this error message is unconnected to the subject of this patch so
should not be here.

> +		goto err_iio_unregister_trigger_consumer;
> +		}
>  	pm_runtime_enable(&client->dev);
>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>  	pm_runtime_use_autosuspend(&client->dev);
>  
>  	return iio_device_register(indio_dev);
> +
One line is enough.
> +
Use devm versions and this lot goes away.
> +err_iio_unregister_trigger_consumer:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +err_iio_trigger_put_unregister:
> +	if (indio_dev->trig) {
> +		iio_trigger_put(indio_dev->trig);
> +		indio_dev->trig = NULL;
> +		}
> +	if (data->drdy_trigger0) {
> +		iio_trigger_unregister(data->drdy_trigger0);
> +		data->drdy_trigger0 = NULL;
> +		}
> +	return ret;
>  }
>  
>  static int rpr0521_remove(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct rpr0521_data *data = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
>  
> @@ -685,8 +952,18 @@ static int rpr0521_remove(struct i2c_client *client)
>  	pm_runtime_set_suspended(&client->dev);
>  	pm_runtime_put_noidle(&client->dev);
>  
> -	rpr0521_poweroff(iio_priv(indio_dev));
> +	rpr0521_poweroff(data);
>  
> +	iio_triggered_buffer_cleanup(indio_dev);
> +	if (indio_dev->trig) {
> +		iio_trigger_put(indio_dev->trig);
> +		indio_dev->trig = NULL;
> +		}
> +	if (data->drdy_trigger0) {
> +		iio_trigger_unregister(data->drdy_trigger0);
> +		data->drdy_trigger0 = NULL;
> +		}
> +	/* Rest of the cleanup is done by devm. */
Givem where you have them, I don't think anything stops you using devm to
handle the trigger registration and buffer cleanup.  No need to set
either of hte magic flags to NULL as far as I can see...

Use the devm versions and remove should be unchanged...

However - look above - there is a race in how you have it currently ordered
that means you should order it differently and not use the devm versions.

>  	return 0;
>  }
>  
> 

Re: [PATCH v2 7/7] iio: light: rpr0521 magic number to sizeof() on value read

[Jonathan Cameron]

On 07/04/17 13:07, Mikko Koivunen wrote:
> Changed magic number to sizeof() on value read.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
Nice little change. I'd have preferred it near the start of the set
before you got going as it could have been picked up whilst the
complex stuff was undergoing revision.

Anyhow, will pick up when the rest is ready.

Jonathan
> ---
> Tested on LeMaker HiKey AOSP7, kernel 4.4.
> checkpatch.pl ok
> ---
>  drivers/iio/light/rpr0521.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 72f608d..75fd3ca 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -662,7 +662,7 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  
>  		ret = regmap_bulk_read(data->regmap,
>  				       rpr0521_data_reg[chan->address].address,
> -				       &raw_data, 2);
> +				       &raw_data, sizeof(raw_data));
>  		if (ret < 0) {
>  			rpr0521_set_power_state(data, false, device_mask);
>  			mutex_unlock(&data->lock);
> 

Re: [PATCH v2 2/7] iio: light: rpr0521 whitespace fixes

[Koivunen, Mikko]

On 8.4.2017 17:49, Jonathan Cameron wrote:
> On 07/04/17 13:07, Mikko Koivunen wrote:
>> Just whitespace changes, no functional changes.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> ---
>>  drivers/iio/light/rpr0521.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index c15529b..30c2592 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -357,8 +357,8 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  			return ret;
>>  
>>  		*val = le16_to_cpu(raw_data);
>> -
> I'd argue that there are good readability reasons to leave this blank line.
> If the original driver hadn't had it I wouldn't have felt strongly enough
> to comment on it, but adding noise to remove it seems rather pointless..

Ack.
Patchset was chopped for mailing list from one big commit and from your
comments on next patches I see some other whitespace changes should also
be in this commit. Let's see in v3 if I get it better.

>>  		return IIO_VAL_INT;
>> +
> Adding this one is definitely worthwhile though!
>>  	case IIO_CHAN_INFO_SCALE:
>>  		mutex_lock(&data->lock);
>>  		ret = rpr0521_get_gain(data, chan->address, val, val2);
>>
>

Re: [PATCH v2 3/7] iio: light: rpr0521 sample_frequency read/write added

[Koivunen, Mikko]

On 8.4.2017 18:02, Jonathan Cameron wrote:
> On 07/04/17 13:07, Mikko Koivunen wrote:
>> Added sysfs read/write sample frequency.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> A few comments inline but looks basically good to me.
>
> Odd little bit of hardware!
>
> Jonathan
>> ---
>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>>
>> Patch v1->v2 changes:
>> multiline comments fixed
>>
>>  drivers/iio/light/rpr0521.c |   99 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 99 insertions(+)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index 30c2592..e92a8bb 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -132,6 +132,30 @@ static const struct rpr0521_gain_info {
>>  	},
>>  };
>>  
>> +struct rpr0521_samp_freq {
>> +	int	als_hz;
>> +	int	als_uhz;
>> +	int	pxs_hz;
>> +	int	pxs_uhz;
>> +};
>> +
>> +static const struct rpr0521_samp_freq rpr0521_samp_freq_i[13] = {
>> +/*	{ALS, PXS} */
> Hmm. You have them all listed here so that you can use the index as
> the register value.    Maybe add a symbol to indicate in the comment
> which ones you can actually use here...

Ack.

>> +	{0, 0, 0, 0},		/* 0000, 0=standby */
>> +	{0, 0, 100, 0},		/* 0001 */
>> +	{0, 0, 25, 0},		/* 0010 */
>> +	{0, 0, 10, 0},		/* 0011 */
>> +	{0, 0, 2, 500000},	/* 0100 */
>> +	{10, 0, 20, 0},		/* 0101 */
>> +	{10, 0, 10, 0},		/* 0110 */
>> +	{10, 0, 2, 500000},	/* 0111 */
>> +	{2, 500000, 20, 0},	/* 1000, measurement 100ms, sleep 300ms */
>> +	{2, 500000, 10, 0},	/* 1001, measurement 100ms, sleep 300ms */
>> +	{2, 500000, 0, 0},	/* 1010, high sensitivity mode */
>> +	{2, 500000, 2, 500000},	/* 1011, high sensitivity mode */
>> +	{20, 0, 20, 0}	/* 1100, ALS_data x 0.5, see specification P.18 */
> Whilst this one is odd, I don't think your write function below will refuse
> to set it...

Good catch. I'll change write in patchset v3 to also skip 20.

>> +};
>> +
>>  struct rpr0521_data {
>>  	struct i2c_client *client;
>>  
>> @@ -152,9 +176,15 @@ struct rpr0521_data {
>>  static IIO_CONST_ATTR(in_intensity_scale_available, RPR0521_ALS_SCALE_AVAIL);
>>  static IIO_CONST_ATTR(in_proximity_scale_available, RPR0521_PXS_SCALE_AVAIL);
>>  
>> +/* Start with easy freq first, whole table of freq combinations is more
> /*
>  * Start...
>
> (kernel multiline comment syntax)...

Ack. Maybe it finally gets right on v3.

>> + * complicated.
>> + */
>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("2.5 10");
> 20 not available?

Not advertising that since output values need scaling on 20. Scaling is
easy to do, but so far not done.

>> +
>>  static struct attribute *rpr0521_attributes[] = {
>>  	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
>>  	&iio_const_attr_in_proximity_scale_available.dev_attr.attr,
>> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>  	NULL,
>>  };
>>  
>> @@ -170,6 +200,7 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  		.channel2 = IIO_MOD_LIGHT_BOTH,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -178,12 +209,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  		.channel2 = IIO_MOD_LIGHT_IR,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>  	},
>>  	{
>>  		.type = IIO_PROXIMITY,
>>  		.address = RPR0521_CHAN_PXS,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>  	}
>>  };
>>  
>> @@ -319,6 +352,56 @@ static int rpr0521_set_gain(struct rpr0521_data *data, int chan,
>>  				  idx << rpr0521_gain[chan].shift);
>>  }
>>  
>> +static int rpr0521_read_samp_freq(struct rpr0521_data *data,
>> +				enum iio_chan_type chan_type,
>> +			    int *val, int *val2)
>> +{
>> +	int reg, ret;
>> +
>> +	ret = regmap_read(data->regmap, RPR0521_REG_MODE_CTRL, &reg);
>> +	if (ret < 0)
>> +		return ret;
> I'd put a blank line here..

Ack.

>> +	reg &= RPR0521_MODE_MEAS_TIME_MASK;
>> +	if (reg >= ARRAY_SIZE(rpr0521_samp_freq_i))
>> +		return -EINVAL;
>> +
>> +	if (chan_type == IIO_INTENSITY) {
>> +		*val = rpr0521_samp_freq_i[reg].als_hz;
>> +		*val2 = rpr0521_samp_freq_i[reg].als_uhz;
>> +	} else if (chan_type == IIO_PROXIMITY) {
>> +		*val = rpr0521_samp_freq_i[reg].pxs_hz;
>> +		*val2 = rpr0521_samp_freq_i[reg].pxs_uhz;
>> +	} else {
>> +		return -EINVAL;
> switch might be tidier..  Also, perhaps return directly from first two
> cases.

Ack.

>> +	}
>> +	return 0;
>> +}
>> +
>> +static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
>> +				enum iio_chan_type chan_type,
>> +				int val, int val2)
>> +{
>> +	int i;
>> +
>> +	/* Ignore channel
> /*
>  * Ignore channel
>  ...

Ack.

>> +	 * both pxs and als are setup only to same freq because of simplicity
>> +	 */
>> +	for (i = 0; i < ARRAY_SIZE(rpr0521_samp_freq_i); i++)
>> +		if ((val == rpr0521_samp_freq_i[i].als_hz)
>> +			&& (val2 == rpr0521_samp_freq_i[i].als_uhz)
>> +			&& (val == rpr0521_samp_freq_i[i].pxs_hz)
>> +			&& (val2 == rpr0521_samp_freq_i[i].pxs_uhz)) {
> Hmm. a lot of complexity introduced above by sort of allowing for them
> to be different (which would indeed be a real pain to handle! Why not
> just drop the ones you can never actually use?

Because I'm hoping to get/planning to do full implementation later. Just
replace multi line comment with read+matching other channels freq.
However got short of time so it looks like this now.
But you are right, maybe better option at this point would be using
switch-case and add the complexity back later if needed. Changing to v3.

>> +			break;
>> +		}
>> +	if (i == ARRAY_SIZE(rpr0521_samp_freq_i))
>> +		return -EINVAL;
>> +
>> +	return regmap_update_bits(data->regmap,
>> +		RPR0521_REG_MODE_CTRL,
>> +		RPR0521_MODE_MEAS_TIME_MASK,
>> +		i);
>> +}
>> +
>>  static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  			    struct iio_chan_spec const *chan, int *val,
>>  			    int *val2, long mask)
>> @@ -365,8 +448,16 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  		mutex_unlock(&data->lock);
>>  		if (ret < 0)
>>  			return ret;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>>  
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		mutex_lock(&data->lock);
>> +		ret = rpr0521_read_samp_freq(data, chan->type, val, val2);
>> +		mutex_unlock(&data->lock);
>> +		if (ret < 0)
>> +			return ret;
>>  		return IIO_VAL_INT_PLUS_MICRO;
>> +
> Again, I think you are messing with original white space.  Don't do
> that as it just gives odd diff results like this one...

Thanks for pointing out. It has bothered me before, but not enough to
investigate reason.

>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -384,8 +475,16 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>>  		mutex_lock(&data->lock);
>>  		ret = rpr0521_set_gain(data, chan->address, val, val2);
>>  		mutex_unlock(&data->lock);
> You are removing the blank line here.. (I think anyway!). Not a change
> that should be in this patch (or possibly happen at all - this one
> is in the personal taste category).  If nothing else it made diff less
> readable!

Ack.

>> +		return ret;
>>  
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		mutex_lock(&data->lock);
>> +		ret = rpr0521_write_samp_freq_common(data,
>> +			chan->type,
>> +			val, val2);
>> +		mutex_unlock(&data->lock);
>>  		return ret;
>> +
>>  	default:
>>  		return -EINVAL;
>>  	}
>>
>

Re: [PATCH v2 4/7] iio: light: rpr0521 proximity offset read/write added

[Koivunen, Mikko]

On 8.4.2017 18:07, Jonathan Cameron wrote:
> On 07/04/17 13:07, Mikko Koivunen wrote:
>> Added sysfs read/write proximity offset feature. Offset is read/write from
>> sensor registers. Values are proximity raw 10-bit values. After applying
>> offset value, output values will be (measured_raw - offset_value). Output
>> values are unsigned so offset value doesn't make output negative.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> ---
> A few bits and bobs inline.
>
> Jonathan

Fixing in patchset v3.

>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>> Builds OK with torvalds/linux 4.9
>> checkpatch.pl OK.
>>
>> Patch v1->v2 changes:
>> read/write_ps_offset() rewritten with:
>>  -  static, __le16, cpu_to_le16(), sizeof()
>>
>>  drivers/iio/light/rpr0521.c |   53 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index e92a8bb..35c759b 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -30,6 +30,9 @@
>>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
>> +#define RPR0521_PS_OFFSET_LSB		0x53
>> +#define RPR0521_PS_OFFSET_MSB		0x54
> Not used, so drop it.
Ack.
>> +
> Given previous naming always has REG_ in it you should maintain that..
> Also, why the blank line here?

Ack.
>>  #define RPR0521_REG_ID			0x92
>>  
>>  #define RPR0521_MODE_ALS_MASK		BIT(7)
>> @@ -215,7 +218,9 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  		.type = IIO_PROXIMITY,
>>  		.address = RPR0521_CHAN_PXS,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +			BIT(IIO_CHAN_INFO_OFFSET) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
> This blank line shouldn't be added here. Nothing to do with tis patch.
Ack.
>> +
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>  	}
>>  };
>> @@ -402,6 +407,40 @@ static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
>>  		i);
>>  }
>>  
>> +static int rpr0521_read_ps_offset(struct rpr0521_data *data, int *offset)
>> +{
>> +	int ret;
>> +	__le16 buffer;
>> +
>> +	ret = regmap_bulk_read(data->regmap,
>> +		RPR0521_PS_OFFSET_LSB, &buffer, sizeof(buffer));
>> +
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Failed to read PS OFFSET register\n");
>> +		return ret;
>> +	}
>> +	*offset = le16_to_cpu(buffer);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rpr0521_write_ps_offset(struct rpr0521_data *data, int offset)
>> +{
>> +	int ret;
>> +	__le16 buffer;
>> +
>> +	buffer = cpu_to_le16(offset & 0x3ff);
>> +	ret = regmap_raw_write(data->regmap,
>> +		RPR0521_PS_OFFSET_LSB, &buffer, sizeof(buffer));
>> +
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Failed to write PS OFFSET register\n");
>> +		return ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  			    struct iio_chan_spec const *chan, int *val,
>>  			    int *val2, long mask)
>> @@ -458,6 +497,14 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  			return ret;
>>  		return IIO_VAL_INT_PLUS_MICRO;
>>  
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		mutex_lock(&data->lock);
>> +		ret = rpr0521_read_ps_offset(data, val);
>> +		mutex_unlock(&data->lock);
>> +		if (ret < 0)
>> +			return ret;
> Slight preference for a blank line here.

Disagree, but ack.

>
>> +		return IIO_VAL_INT;
>> +
>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -485,6 +532,12 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>>  		mutex_unlock(&data->lock);
>>  		return ret;
>>  
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		mutex_lock(&data->lock);
>> +		ret = rpr0521_write_ps_offset(data, val);
>> +		mutex_unlock(&data->lock);
>> +		return ret;
>> +
>>  	default:
>>  		return -EINVAL;
>>  	}
>>
>

Re: [PATCH v2 6/7] iio: light: rpr0521 triggered buffer added

[Koivunen, Mikko]

On 8.4.2017 18:28, Jonathan Cameron wrote:
> On 07/04/17 13:07, Mikko Koivunen wrote:
>> Driver sets up and uses triggered buffer if there is irq defined for device in
>> device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
>> Trigger consumer reads rpr0521 data to scan buffer.
>> Depends on previous commits of _scale and _offset.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> ---
> A few comments inline...
>
> Jonathan
>> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
>> Builds OK with torvalds/linux 4.9
>> checkpatch.pl OK.
>>
>> Patch v1->v2 changes:
>> removed flagging of the new feature
>> removed unnecesssary rpr0521_update_scan_mode()
>> removed unnecessary comments.
>> added IIO_LE to channel spec
>>
>>  drivers/iio/light/rpr0521.c |  287 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 282 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index dc522fd..72f608d 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -9,9 +9,10 @@
>>   *
>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>   *
>> - * TODO: illuminance channel, PM support, buffer
>> + * TODO: illuminance channel, PM support
>>   */
>>  
>> +
> Be careful to clear out stray whitespace changes like this one.
> Just add noise to the patch and may mean it takes longer to be accepted...

Ack. Need to double check next time.

>>  #include <linux/module.h>
>>  #include <linux/init.h>
>>  #include <linux/i2c.h>
>> @@ -20,6 +21,10 @@
>>  #include <linux/acpi.h>
>>  
>>  #include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>>  #include <linux/iio/sysfs.h>
>>  #include <linux/pm_runtime.h>
>>  
>> @@ -30,6 +35,7 @@
>>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
>> +#define RPR0521_REG_INTERRUPT		0x4A
>>  #define RPR0521_PS_OFFSET_LSB		0x53
>>  #define RPR0521_PS_OFFSET_MSB		0x54
>>  
>> @@ -44,16 +50,33 @@
>>  #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
>>  #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
>>  #define RPR0521_PXS_GAIN_SHIFT		4
>> +#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
>> +#define RPR0521_INTERRUPT_INT_LATCH_MASK	BIT(2)
>> +#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
>> +#define RPR0521_INTERRUPT_INT_MODE_MASK		GENMASK(5, 4)
>>  
>>  #define RPR0521_MODE_ALS_ENABLE		BIT(7)
>>  #define RPR0521_MODE_ALS_DISABLE	0x00
>>  #define RPR0521_MODE_PXS_ENABLE		BIT(6)
>>  #define RPR0521_MODE_PXS_DISABLE	0x00
>> +#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_LATCH_DISABLE	BIT(2)
>> +#define RPR0521_INTERRUPT_INT_LATCH_ENABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
>> +#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT	BIT(5)
>>  
>>  #define RPR0521_MANUFACT_ID		0xE0
>>  #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
>>  
>>  #define RPR0521_DRV_NAME		"RPR0521"
>> +#define RPR0521_IRQ_NAME		"rpr0521_event"
>>  #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
>>  
>>  #define RPR0521_SLEEP_DELAY_MS	2000
>> @@ -169,6 +192,9 @@ struct rpr0521_data {
>>  	bool als_dev_en;
>>  	bool pxs_dev_en;
>>  
>> +	struct iio_trigger *drdy_trigger0;
>> +	bool drdy_trigger_on;
>> +
>>  	/* optimize runtime pm ops - enable device only if needed */
>>  	bool als_ps_need_en;
>>  	bool pxs_ps_need_en;
>> @@ -195,6 +221,13 @@ static const struct attribute_group rpr0521_attribute_group = {
>>  	.attrs = rpr0521_attributes,
>>  };
>>  
>> +/* Order of the channel data in buffer */
>> +enum rpr0521_scan_index_order {
>> +	RPR0521_CHAN_INDEX_PXS,
>> +	RPR0521_CHAN_INDEX_BOTH,
>> +	RPR0521_CHAN_INDEX_IR,
>> +};
>> +
>>  static const struct iio_chan_spec rpr0521_channels[] = {
>>  	{
>>  		.type = IIO_PROXIMITY,
>> @@ -203,6 +236,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  			BIT(IIO_CHAN_INFO_OFFSET) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.scan_index = RPR0521_CHAN_INDEX_PXS,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +			.shift = 0,
>> +			.endianness = IIO_LE,
>> +		},
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -212,6 +253,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.scan_index = RPR0521_CHAN_INDEX_BOTH,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +			.shift = 0,
>> +			.endianness = IIO_LE,
>> +		},
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -221,9 +270,154 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.scan_index = RPR0521_CHAN_INDEX_IR,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +			.shift = 0,
> shift default makes sense as 0 so don't bother specifying it
> (will be set to 0 anyway)

Ack.
>> +			.endianness = IIO_LE,
>> +		},
>>  	},
>>  };
>>  
>> +static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>> +				   u8 device_mask);
>> +static int rpr0521_als_enable(struct rpr0521_data *data, u8 status);
>> +static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status);
> These forward definitions rather look like they could be removed with a bit
> of reorganization fo the file.  Do that in preference to having these in
> the middle of the code.

Ok.

>> +
>> +/* IRQ to trigger -handler */
> why the -?
>> +static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
>> +{
>> +	struct iio_dev *indio_dev = private;
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +
>> +	/* Notify trigger0 consumers/subscribers */
>> +	if (data->drdy_trigger_on)
>> +		iio_trigger_poll(data->drdy_trigger0);
> How else are you getting here? Also the core will be fine
> if nothing cares and the trigger is polled.
> If there is another cause, we aren't handling it so should claim
> to be doing so.

Sometimes there is static on line and interrupt is generated even though
sensor interrupt output is in high impedance mode. What I've seen it's
max 2 interrupts in second, usually none. This doesn't matter much for
stability if sensor is switched on. It just causes extra reads. But if
extra interrupt happens when sensor power is switched off it isn't there
to respond to i2c data reading.
I don't know enough to know if all systems can handle this gracefully or
not. That's why safe playing here.
I'm keeping it unless someone can confirm that 2Hz regmap_bulk_read in
trigger_consumer_handler will not be causing ill effects on system when
there is no answer from sensor.

>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
>> +
>> +	ret = regmap_bulk_read(data->regmap,
>> +		RPR0521_REG_PXS_DATA,
>> +		&buffer,
>> +		(3*2)+1);	/* 3 * 16-bit + (discarded) int clear reg. */
> kernel style is spaces around the * and +
Ack.
>> +	if (ret < 0)
>> +		goto done;
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev,
>> +		buffer,
> Dont wrap more than you have two.  So make this two lines perhaps.
>> +		pf->timestamp);
>> +done:
Refactoring to v3 to get rid of done:
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int rpr0521_write_int_enable(struct rpr0521_data *data)
>> +{
>> +	int err;
>> +
>> +	/* Interrupt after each measurement */
>> +	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
>> +		RPR0521_PXS_PERSISTENCE_MASK,
>> +		RPR0521_PXS_PERSISTENCE_DRDY);
>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
> umm. really?  All of them? Are we looking at a clear the whole register
> case?  If so we should probably only clear relevant ones..

Hmm. .  Valid point. In drdy case we can write what ever to latch and
mode. This will be cleaner with write instead up write_bits.
==> change to v3
> I don't like these stacked error handlers (err = err | *).
> Just handle each err as and when.  It's more code, but easier
> to review / follow.

It's (err = err || *), bail out on first error.
Matter of taste, ack.

>> +		RPR0521_INTERRUPT_INT_REASSERT_MASK |
>> +		RPR0521_INTERRUPT_INT_LATCH_MASK |
>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>> +		RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>> +		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
>> +		RPR0521_INTERRUPT_INT_LATCH_ENABLE |
>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>> +		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
>> +		);
>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>> +		RPR0521_INTERRUPT_INT_MODE_MASK,
>> +		RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT);
>> +	if (err) {
>> +		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
>> +		return -EBUSY;
>> +		}
>> +	return 0;
>> +}
>> +
>> +static int rpr0521_write_int_disable(struct rpr0521_data *data)
>> +{
>> +	return regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>> +				RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>> +				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
>> +				);
>> +}
>> +
>> +/* Trigger -enable/disable */
>> +static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
>> +	bool enable_drdy)
>> +{
> This may be called from the moment iio_trigger_register is called until
> the trigger is unregistered on remove.
>
> Could that potentially be a problem as it might occur after the device
> is powered off in remove?  Hard to be sure without diving into the data
> sheet.  If it if safe as it is, then use the devm functions as described
> below, but perhaps put a note here about the fact it is safe to have
> this potential race.

Sensor itself doesn't care much, but since measurement is enabled we are
draining power for nothing.
I didn't like to touch pm_ -stuff, since I didn't know much of it. But
now since I did, I think I found couple of bugs both in the current code
and new code.

There is another thing for the order. Int pin keeps state on power down.
So shutting down sensor before clearing interrupt might cause power
drain also on interrupt line.

==>
Reordering the _probe() and _remove() for v3.
Adding int clear to poweroff.
Adding couple commits for pm bugfixes.

One question though. Is it ok to register device first, and then
trigger/buffer stuff? For now it seems like correct order.

>> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +	int err;
>> +
>> +	if (enable_drdy) {
>> +		/* ENABLE DRDY */
>> +		mutex_lock(&data->lock);
>> +		err = rpr0521_set_power_state(data, true,
>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>> +		mutex_unlock(&data->lock);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);

Removing rpr0521_[pxs/als]_[en/dis]able(), since they are by default
enabled from rpr0521_init and _set_power_state sets them on if pm has
disabled them.
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_write_int_enable(data);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +	} else {
>> +		/* DISABLE DRDY */
>> +		mutex_lock(&data->lock);
>> +		err = rpr0521_set_power_state(data, false,
>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>> +		mutex_unlock(&data->lock);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_DISABLE);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_DISABLE);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_write_int_disable(data);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +	}
>> +	data->drdy_trigger_on = enable_drdy;
>> +	return 0;
>> +
>> +rpr0521_set_state_err:
>> +	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
>> +	return err;
>> +}
>> +
>> +static const struct iio_trigger_ops rpr0521_trigger_ops = {
>> +	.set_trigger_state = rpr0521_pxs_drdy_set_state,
>> +	.owner = THIS_MODULE,
>> +	};
>> +
>>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>>  {
>>  	int ret;
>> @@ -454,6 +648,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
>>  			return -EINVAL;
>>  
>> +		if (iio_buffer_enabled(indio_dev))
>> +			return -EBUSY;
>> +
>>  		device_mask = rpr0521_data_reg[chan->address].device_mask;
>>  
>>  		mutex_lock(&data->lock);
>> @@ -664,20 +861,90 @@ static int rpr0521_probe(struct i2c_client *client,
>>  		return ret;
>>  	}
>>  
>> -	ret = pm_runtime_set_active(&client->dev);
>> -	if (ret < 0)
>> -		return ret;
>> +	/* IRQ to trigger setup */
>> +	if (client->irq) {
>> +		/* Trigger0 producer setup */
>> +		data->drdy_trigger0 = devm_iio_trigger_alloc(
>> +			indio_dev->dev.parent,
>> +			"%s-dev%d",
>> +			indio_dev->name,
>> +			indio_dev->id);
>> +		if (!data->drdy_trigger0) {
>> +			ret = -ENOMEM;
>> +			return ret;
>> +		}
>> +		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
>> +		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
>> +		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
>> +
>> +		ret = iio_trigger_register(data->drdy_trigger0);
> devm_iio_trigger_register should be fine
See below.
>> +		if (ret) {
>> +			dev_err(&client->dev, "iio trigger register failed\n");
>> +			return ret;
>> +		}
>>  
>> +		/* Set trigger0 as default trigger (and inc refcount) */
>> +		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
>> +
>> +		/* Ties irq to trigger producer handler. */
>> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
>> +			rpr0521_drdy_irq_handler,
>> +			NULL,
>> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +			RPR0521_IRQ_NAME,
>> +			indio_dev);
>> +		if (ret < 0) {
>> +			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
>> +				client->irq);
>> +			goto err_iio_trigger_put_unregister;
>> +			}
>> +		/*
>> +		 * Now whole pipe from physical interrupt (irq defined by
>> +		 * devicetree to device) to trigger0 output is set up.
>> +		 */
>> +
>> +		/* Trigger consumer setup */
>> +		ret = iio_triggered_buffer_setup(indio_dev,
>> +			&iio_pollfunc_store_time,
>> +			rpr0521_trigger_consumer_handler,
>> +			NULL);		//Use default _ops
>> +		if (ret < 0) {
>> +			dev_err(&client->dev, "iio triggered buffer setup failed\n");
>> +			/* Count on devm to free_irq */
>> +			goto err_iio_trigger_put_unregister;
>> +		}
>> +	}
>> +
>> +	ret = pm_runtime_set_active(&client->dev);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "set_active failed\n");
> I'm not particularly bothered either way by the change, but technically
> adding this error message is unconnected to the subject of this patch so
> should not be here.
>
>> +		goto err_iio_unregister_trigger_consumer;
>> +		}
>>  	pm_runtime_enable(&client->dev);
>>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>>  	pm_runtime_use_autosuspend(&client->dev);
>>  
>>  	return iio_device_register(indio_dev);
>> +
> One line is enough.
Ok.
>> +
> Use devm versions and this lot goes away.
>> +err_iio_unregister_trigger_consumer:
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +err_iio_trigger_put_unregister:
>> +	if (indio_dev->trig) {
>> +		iio_trigger_put(indio_dev->trig);
>> +		indio_dev->trig = NULL;
>> +		}
>> +	if (data->drdy_trigger0) {
>> +		iio_trigger_unregister(data->drdy_trigger0);
>> +		data->drdy_trigger0 = NULL;
>> +		}
>> +	return ret;
>>  }
>>  
>>  static int rpr0521_remove(struct i2c_client *client)
>>  {
>>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>  
>>  	iio_device_unregister(indio_dev);
>>  
>> @@ -685,8 +952,18 @@ static int rpr0521_remove(struct i2c_client *client)
>>  	pm_runtime_set_suspended(&client->dev);
>>  	pm_runtime_put_noidle(&client->dev);
>>  
>> -	rpr0521_poweroff(iio_priv(indio_dev));
>> +	rpr0521_poweroff(data);
>>  
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +	if (indio_dev->trig) {
>> +		iio_trigger_put(indio_dev->trig);
>> +		indio_dev->trig = NULL;
>> +		}
>> +	if (data->drdy_trigger0) {
>> +		iio_trigger_unregister(data->drdy_trigger0);
>> +		data->drdy_trigger0 = NULL;
>> +		}
>> +	/* Rest of the cleanup is done by devm. */
> Givem where you have them, I don't think anything stops you using devm to
> handle the trigger registration and buffer cleanup.  No need to set
> either of hte magic flags to NULL as far as I can see...

For a second let's think we removed this line

>>+	if (data->drdy_trigger_on)

from irq_handler().
If irq handler is not yet unregistered by devm_ and thus interrupt can
be generated (f.ex. by static), then what does
iio_trigger_poll(data->drdy_trigger0) do with this unregistered trigger
pointer? If call is ok, then no need for NULL I guess.

>
> Use the devm versions and remove should be unchanged...
Copy-paste from another post:

Can't, since no devm_ on kernel 4.4.
I could make another commit to change to devm_, but I don't have target
to test it.

> OK. We can leave it for someone else to clean up in future.

> However - look above - there is a race in how you have it currently ordered
> that means you should order it differently and not use the devm versions.
Ack.

>>  	return 0;
>>  }
>>  
>>

Re: [PATCH v2 1/7] iio: light: rpr0521 disable sensor -bugfix

[Koivunen, Mikko]

On 8.4.2017 17:47, Jonathan Cameron wrote:
> On 07/04/17 13:07, Mikko Koivunen wrote:
>> Sensor was marked enabled on each call even if the call was for disabling
>> sensor.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> What effect does this have on device operation?

Actually none with current code.
Because it's never called for disabling, not even in_poweroff(). This
could also be fixed by removing second parameter and hardcoding status
as _MODE_x_ENABLE.

> What I'm looking for is info on whether this wants to go to stable as
> it will cause the device not to work, or whether it is wasting power etc.
>
> Also useful to add a fixes tag as that makes it easy for stable maintainers
> to filter incoming patches for relevance to their trees.
>
> Will be looking for Acks from Daniel on all of these as he wrote the original
> and is still active.
>
> Jonathan
>> ---
>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>>
>>  drivers/iio/light/rpr0521.c |   11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index 7de0f39..c15529b 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -197,8 +197,10 @@ static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	data->als_dev_en = true;
>> -
>> +	if (status & RPR0521_MODE_ALS_MASK)
>> +		data->als_dev_en = true;
>> +	else
>> +		data->als_dev_en = false;
>>  	return 0;
>>  }
>>  
>> @@ -212,7 +214,10 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	data->pxs_dev_en = true;
>> +	if (status & RPR0521_MODE_PXS_MASK)
>> +		data->pxs_dev_en = true;
>> +	else
>> +		data->pxs_dev_en = false;
>>  
>>  	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
>

Re: [PATCH v2 6/7] iio: light: rpr0521 triggered buffer added

[Jonathan Cameron]

On 12/04/17 14:44, Koivunen, Mikko wrote:
> On 8.4.2017 18:28, Jonathan Cameron wrote:
>> On 07/04/17 13:07, Mikko Koivunen wrote:
>>> Driver sets up and uses triggered buffer if there is irq defined for device in
>>> device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
>>> Trigger consumer reads rpr0521 data to scan buffer.
>>> Depends on previous commits of _scale and _offset.
>>>
>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>>> ---
>> A few comments inline...
>>
>> Jonathan
>>> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
>>> Builds OK with torvalds/linux 4.9
>>> checkpatch.pl OK.
>>>
>>> Patch v1->v2 changes:
>>> removed flagging of the new feature
>>> removed unnecesssary rpr0521_update_scan_mode()
>>> removed unnecessary comments.
>>> added IIO_LE to channel spec
>>>
>>>  drivers/iio/light/rpr0521.c |  287 ++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 282 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>> index dc522fd..72f608d 100644
>>> --- a/drivers/iio/light/rpr0521.c
>>> +++ b/drivers/iio/light/rpr0521.c
>>> @@ -9,9 +9,10 @@
>>>   *
>>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>>   *
>>> - * TODO: illuminance channel, PM support, buffer
>>> + * TODO: illuminance channel, PM support
>>>   */
>>>  
>>> +
>> Be careful to clear out stray whitespace changes like this one.
>> Just add noise to the patch and may mean it takes longer to be accepted...
> 
> Ack. Need to double check next time.
> 
>>>  #include <linux/module.h>
>>>  #include <linux/init.h>
>>>  #include <linux/i2c.h>
>>> @@ -20,6 +21,10 @@
>>>  #include <linux/acpi.h>
>>>  
>>>  #include <linux/iio/iio.h>
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>>  #include <linux/iio/sysfs.h>
>>>  #include <linux/pm_runtime.h>
>>>  
>>> @@ -30,6 +35,7 @@
>>>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>>>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>>>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
>>> +#define RPR0521_REG_INTERRUPT		0x4A
>>>  #define RPR0521_PS_OFFSET_LSB		0x53
>>>  #define RPR0521_PS_OFFSET_MSB		0x54
>>>  
>>> @@ -44,16 +50,33 @@
>>>  #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
>>>  #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
>>>  #define RPR0521_PXS_GAIN_SHIFT		4
>>> +#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
>>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
>>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
>>> +#define RPR0521_INTERRUPT_INT_LATCH_MASK	BIT(2)
>>> +#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
>>> +#define RPR0521_INTERRUPT_INT_MODE_MASK		GENMASK(5, 4)
>>>  
>>>  #define RPR0521_MODE_ALS_ENABLE		BIT(7)
>>>  #define RPR0521_MODE_ALS_DISABLE	0x00
>>>  #define RPR0521_MODE_PXS_ENABLE		BIT(6)
>>>  #define RPR0521_MODE_PXS_DISABLE	0x00
>>> +#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
>>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
>>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
>>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
>>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
>>> +#define RPR0521_INTERRUPT_INT_LATCH_DISABLE	BIT(2)
>>> +#define RPR0521_INTERRUPT_INT_LATCH_ENABLE	0x00
>>> +#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
>>> +#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
>>> +#define RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT	BIT(5)
>>>  
>>>  #define RPR0521_MANUFACT_ID		0xE0
>>>  #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
>>>  
>>>  #define RPR0521_DRV_NAME		"RPR0521"
>>> +#define RPR0521_IRQ_NAME		"rpr0521_event"
>>>  #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
>>>  
>>>  #define RPR0521_SLEEP_DELAY_MS	2000
>>> @@ -169,6 +192,9 @@ struct rpr0521_data {
>>>  	bool als_dev_en;
>>>  	bool pxs_dev_en;
>>>  
>>> +	struct iio_trigger *drdy_trigger0;
>>> +	bool drdy_trigger_on;
>>> +
>>>  	/* optimize runtime pm ops - enable device only if needed */
>>>  	bool als_ps_need_en;
>>>  	bool pxs_ps_need_en;
>>> @@ -195,6 +221,13 @@ static const struct attribute_group rpr0521_attribute_group = {
>>>  	.attrs = rpr0521_attributes,
>>>  };
>>>  
>>> +/* Order of the channel data in buffer */
>>> +enum rpr0521_scan_index_order {
>>> +	RPR0521_CHAN_INDEX_PXS,
>>> +	RPR0521_CHAN_INDEX_BOTH,
>>> +	RPR0521_CHAN_INDEX_IR,
>>> +};
>>> +
>>>  static const struct iio_chan_spec rpr0521_channels[] = {
>>>  	{
>>>  		.type = IIO_PROXIMITY,
>>> @@ -203,6 +236,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>>  			BIT(IIO_CHAN_INFO_OFFSET) |
>>>  			BIT(IIO_CHAN_INFO_SCALE),
>>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>> +		.scan_index = RPR0521_CHAN_INDEX_PXS,
>>> +		.scan_type = {
>>> +			.sign = 'u',
>>> +			.realbits = 16,
>>> +			.storagebits = 16,
>>> +			.shift = 0,
>>> +			.endianness = IIO_LE,
>>> +		},
>>>  	},
>>>  	{
>>>  		.type = IIO_INTENSITY,
>>> @@ -212,6 +253,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>  			BIT(IIO_CHAN_INFO_SCALE),
>>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>> +		.scan_index = RPR0521_CHAN_INDEX_BOTH,
>>> +		.scan_type = {
>>> +			.sign = 'u',
>>> +			.realbits = 16,
>>> +			.storagebits = 16,
>>> +			.shift = 0,
>>> +			.endianness = IIO_LE,
>>> +		},
>>>  	},
>>>  	{
>>>  		.type = IIO_INTENSITY,
>>> @@ -221,9 +270,154 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>  			BIT(IIO_CHAN_INFO_SCALE),
>>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>> +		.scan_index = RPR0521_CHAN_INDEX_IR,
>>> +		.scan_type = {
>>> +			.sign = 'u',
>>> +			.realbits = 16,
>>> +			.storagebits = 16,
>>> +			.shift = 0,
>> shift default makes sense as 0 so don't bother specifying it
>> (will be set to 0 anyway)
> 
> Ack.
>>> +			.endianness = IIO_LE,
>>> +		},
>>>  	},
>>>  };
>>>  
>>> +static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>> +				   u8 device_mask);
>>> +static int rpr0521_als_enable(struct rpr0521_data *data, u8 status);
>>> +static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status);
>> These forward definitions rather look like they could be removed with a bit
>> of reorganization fo the file.  Do that in preference to having these in
>> the middle of the code.
> 
> Ok.
> 
>>> +
>>> +/* IRQ to trigger -handler */
>> why the -?
>>> +static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
>>> +{
>>> +	struct iio_dev *indio_dev = private;
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>> +
>>> +	/* Notify trigger0 consumers/subscribers */
>>> +	if (data->drdy_trigger_on)
>>> +		iio_trigger_poll(data->drdy_trigger0);
>> How else are you getting here? Also the core will be fine
>> if nothing cares and the trigger is polled.
>> If there is another cause, we aren't handling it so should claim
>> to be doing so.
> 
> Sometimes there is static on line and interrupt is generated even though
> sensor interrupt output is in high impedance mode. What I've seen it's
> max 2 interrupts in second, usually none. This doesn't matter much for
> stability if sensor is switched on. It just causes extra reads. But if
> extra interrupt happens when sensor power is switched off it isn't there
> to respond to i2c data reading.
> I don't know enough to know if all systems can handle this gracefully or
> not. That's why safe playing here.
> I'm keeping it unless someone can confirm that 2Hz regmap_bulk_read in
> trigger_consumer_handler will not be causing ill effects on system when
> there is no answer from sensor.
Perhaps add a comment to mention that an issue has been observed.
Normally we don't put this sort of code in to protect against hardware
bugs like this, but if you have a platform where it is true, then fine
but it needs to be mentioned, so someone doesn't come along later
and remove what looks like pointless code!
> 
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>>> +{
>>> +	struct iio_poll_func *pf = p;
>>> +	struct iio_dev *indio_dev = pf->indio_dev;
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>> +	int ret;
>>> +
>>> +	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
>>> +
>>> +	ret = regmap_bulk_read(data->regmap,
>>> +		RPR0521_REG_PXS_DATA,
>>> +		&buffer,
>>> +		(3*2)+1);	/* 3 * 16-bit + (discarded) int clear reg. */
>> kernel style is spaces around the * and +
> Ack.
>>> +	if (ret < 0)
>>> +		goto done;
>>> +
>>> +	iio_push_to_buffers_with_timestamp(indio_dev,
>>> +		buffer,
>> Dont wrap more than you have two.  So make this two lines perhaps.
>>> +		pf->timestamp);
>>> +done:
> Refactoring to v3 to get rid of done:
>>> +	iio_trigger_notify_done(indio_dev->trig);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int rpr0521_write_int_enable(struct rpr0521_data *data)
>>> +{
>>> +	int err;
>>> +
>>> +	/* Interrupt after each measurement */
>>> +	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
>>> +		RPR0521_PXS_PERSISTENCE_MASK,
>>> +		RPR0521_PXS_PERSISTENCE_DRDY);
>>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>> umm. really?  All of them? Are we looking at a clear the whole register
>> case?  If so we should probably only clear relevant ones..
> 
> Hmm. .  Valid point. In drdy case we can write what ever to latch and
> mode. This will be cleaner with write instead up write_bits.
> ==> change to v3
>> I don't like these stacked error handlers (err = err | *).
>> Just handle each err as and when.  It's more code, but easier
>> to review / follow.
> 
> It's (err = err || *), bail out on first error.
> Matter of taste, ack.
> 
>>> +		RPR0521_INTERRUPT_INT_REASSERT_MASK |
>>> +		RPR0521_INTERRUPT_INT_LATCH_MASK |
>>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>>> +		RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>>> +		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
>>> +		RPR0521_INTERRUPT_INT_LATCH_ENABLE |
>>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>>> +		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
>>> +		);
>>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>>> +		RPR0521_INTERRUPT_INT_MODE_MASK,
>>> +		RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT);
>>> +	if (err) {
>>> +		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
>>> +		return -EBUSY;
>>> +		}
>>> +	return 0;
>>> +}
>>> +
>>> +static int rpr0521_write_int_disable(struct rpr0521_data *data)
>>> +{
>>> +	return regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>>> +				RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>>> +				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
>>> +				);
>>> +}
>>> +
>>> +/* Trigger -enable/disable */
>>> +static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
>>> +	bool enable_drdy)
>>> +{
>> This may be called from the moment iio_trigger_register is called until
>> the trigger is unregistered on remove.
>>
>> Could that potentially be a problem as it might occur after the device
>> is powered off in remove?  Hard to be sure without diving into the data
>> sheet.  If it if safe as it is, then use the devm functions as described
>> below, but perhaps put a note here about the fact it is safe to have
>> this potential race.
> 
> Sensor itself doesn't care much, but since measurement is enabled we are
> draining power for nothing.
> I didn't like to touch pm_ -stuff, since I didn't know much of it. But
> now since I did, I think I found couple of bugs both in the current code
> and new code.
> 
> There is another thing for the order. Int pin keeps state on power down.
> So shutting down sensor before clearing interrupt might cause power
> drain also on interrupt line.
> 
> ==>
> Reordering the _probe() and _remove() for v3.
> Adding int clear to poweroff.
> Adding couple commits for pm bugfixes.
> 
> One question though. Is it ok to register device first, and then
> trigger/buffer stuff? For now it seems like correct order.
No.  The moment the register occurs, it's exposed to userspace.
The buffer at least is hidden until after that.

Trigger vs device is more a matter of convention. Tends to be easier to
make sure the trigger register doesn't cause trouble if something tries
to use it early than the whole device (the userspace surface is smaller).
> 
>>> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>> +	int err;
>>> +
>>> +	if (enable_drdy) {
>>> +		/* ENABLE DRDY */
>>> +		mutex_lock(&data->lock);
>>> +		err = rpr0521_set_power_state(data, true,
>>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>>> +		mutex_unlock(&data->lock);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
> 
> Removing rpr0521_[pxs/als]_[en/dis]able(), since they are by default
> enabled from rpr0521_init and _set_power_state sets them on if pm has
> disabled them.
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_write_int_enable(data);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +	} else {
>>> +		/* DISABLE DRDY */
>>> +		mutex_lock(&data->lock);
>>> +		err = rpr0521_set_power_state(data, false,
>>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>>> +		mutex_unlock(&data->lock);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_DISABLE);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_DISABLE);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_write_int_disable(data);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +	}
>>> +	data->drdy_trigger_on = enable_drdy;
>>> +	return 0;
>>> +
>>> +rpr0521_set_state_err:
>>> +	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
>>> +	return err;
>>> +}
>>> +
>>> +static const struct iio_trigger_ops rpr0521_trigger_ops = {
>>> +	.set_trigger_state = rpr0521_pxs_drdy_set_state,
>>> +	.owner = THIS_MODULE,
>>> +	};
>>> +
>>>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>>>  {
>>>  	int ret;
>>> @@ -454,6 +648,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>>  		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
>>>  			return -EINVAL;
>>>  
>>> +		if (iio_buffer_enabled(indio_dev))
>>> +			return -EBUSY;
>>> +
>>>  		device_mask = rpr0521_data_reg[chan->address].device_mask;
>>>  
>>>  		mutex_lock(&data->lock);
>>> @@ -664,20 +861,90 @@ static int rpr0521_probe(struct i2c_client *client,
>>>  		return ret;
>>>  	}
>>>  
>>> -	ret = pm_runtime_set_active(&client->dev);
>>> -	if (ret < 0)
>>> -		return ret;
>>> +	/* IRQ to trigger setup */
>>> +	if (client->irq) {
>>> +		/* Trigger0 producer setup */
>>> +		data->drdy_trigger0 = devm_iio_trigger_alloc(
>>> +			indio_dev->dev.parent,
>>> +			"%s-dev%d",
>>> +			indio_dev->name,
>>> +			indio_dev->id);
>>> +		if (!data->drdy_trigger0) {
>>> +			ret = -ENOMEM;
>>> +			return ret;
>>> +		}
>>> +		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
>>> +		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
>>> +		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
>>> +
>>> +		ret = iio_trigger_register(data->drdy_trigger0);
>> devm_iio_trigger_register should be fine
> See below.
>>> +		if (ret) {
>>> +			dev_err(&client->dev, "iio trigger register failed\n");
>>> +			return ret;
>>> +		}
>>>  
>>> +		/* Set trigger0 as default trigger (and inc refcount) */
>>> +		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
>>> +
>>> +		/* Ties irq to trigger producer handler. */
>>> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
>>> +			rpr0521_drdy_irq_handler,
>>> +			NULL,
>>> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>> +			RPR0521_IRQ_NAME,
>>> +			indio_dev);
>>> +		if (ret < 0) {
>>> +			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
>>> +				client->irq);
>>> +			goto err_iio_trigger_put_unregister;
>>> +			}
>>> +		/*
>>> +		 * Now whole pipe from physical interrupt (irq defined by
>>> +		 * devicetree to device) to trigger0 output is set up.
>>> +		 */
>>> +
>>> +		/* Trigger consumer setup */
>>> +		ret = iio_triggered_buffer_setup(indio_dev,
>>> +			&iio_pollfunc_store_time,
>>> +			rpr0521_trigger_consumer_handler,
>>> +			NULL);		//Use default _ops
>>> +		if (ret < 0) {
>>> +			dev_err(&client->dev, "iio triggered buffer setup failed\n");
>>> +			/* Count on devm to free_irq */
>>> +			goto err_iio_trigger_put_unregister;
>>> +		}
>>> +	}
>>> +
>>> +	ret = pm_runtime_set_active(&client->dev);
>>> +	if (ret < 0) {
>>> +		dev_err(&client->dev, "set_active failed\n");
>> I'm not particularly bothered either way by the change, but technically
>> adding this error message is unconnected to the subject of this patch so
>> should not be here.
>>
>>> +		goto err_iio_unregister_trigger_consumer;
>>> +		}
>>>  	pm_runtime_enable(&client->dev);
>>>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>>>  	pm_runtime_use_autosuspend(&client->dev);
>>>  
>>>  	return iio_device_register(indio_dev);
>>> +
>> One line is enough.
> Ok.
>>> +
>> Use devm versions and this lot goes away.
>>> +err_iio_unregister_trigger_consumer:
>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>> +err_iio_trigger_put_unregister:
>>> +	if (indio_dev->trig) {
>>> +		iio_trigger_put(indio_dev->trig);
>>> +		indio_dev->trig = NULL;
>>> +		}
>>> +	if (data->drdy_trigger0) {
>>> +		iio_trigger_unregister(data->drdy_trigger0);
>>> +		data->drdy_trigger0 = NULL;
>>> +		}
>>> +	return ret;
>>>  }
>>>  
>>>  static int rpr0521_remove(struct i2c_client *client)
>>>  {
>>>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>>  
>>>  	iio_device_unregister(indio_dev);
>>>  
>>> @@ -685,8 +952,18 @@ static int rpr0521_remove(struct i2c_client *client)
>>>  	pm_runtime_set_suspended(&client->dev);
>>>  	pm_runtime_put_noidle(&client->dev);
>>>  
>>> -	rpr0521_poweroff(iio_priv(indio_dev));
>>> +	rpr0521_poweroff(data);
>>>  
>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>> +	if (indio_dev->trig) {
>>> +		iio_trigger_put(indio_dev->trig);
>>> +		indio_dev->trig = NULL;
>>> +		}
>>> +	if (data->drdy_trigger0) {
>>> +		iio_trigger_unregister(data->drdy_trigger0);
>>> +		data->drdy_trigger0 = NULL;
>>> +		}
>>> +	/* Rest of the cleanup is done by devm. */
>> Givem where you have them, I don't think anything stops you using devm to
>> handle the trigger registration and buffer cleanup.  No need to set
>> either of hte magic flags to NULL as far as I can see...
> 
> For a second let's think we removed this line
> 
>>> +	if (data->drdy_trigger_on)
> 
> from irq_handler().
> If irq handler is not yet unregistered by devm_ and thus interrupt can
> be generated (f.ex. by static),
This is one of the nasty reasons why we tend to not spend all that much
effort dealing with false interrupts. It's almost always possible to find
a race where they will crash things.
> then what does
> iio_trigger_poll(data->drdy_trigger0) do with this unregistered trigger
> pointer? If call is ok, then no need for NULL I guess.
Firstly there is always a race here if this is an issue, you are just
making it smaller.   This is all going to occur very fast in any case.

Still all irrelevant anyway ;)

> 
>>
>> Use the devm versions and remove should be unchanged...
> Copy-paste from another post:
> 
> Can't, since no devm_ on kernel 4.4.
> I could make another commit to change to devm_, but I don't have target
> to test it.
> 
>> OK. We can leave it for someone else to clean up in future.
> 
>> However - look above - there is a race in how you have it currently ordered
>> that means you should order it differently and not use the devm versions.
> Ack.
> 
>>>  	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
> 

Re: [PATCH v2 6/7] iio: light: rpr0521 triggered buffer added

[Koivunen, Mikko]

On 14.4.2017 18:21, Jonathan Cameron wrote:
> On 12/04/17 14:44, Koivunen, Mikko wrote:
>> On 8.4.2017 18:28, Jonathan Cameron wrote:
>>> On 07/04/17 13:07, Mikko Koivunen wrote:
>>>> Driver sets up and uses triggered buffer if there is irq defined for device in
>>>> device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
>>>> Trigger consumer reads rpr0521 data to scan buffer.
>>>> Depends on previous commits of _scale and _offset.
>>>>
>>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>>>> ---
>>> A few comments inline...
>>>
>>> Jonathan
>>>> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
>>>> Builds OK with torvalds/linux 4.9
>>>> checkpatch.pl OK.
>>>>
>>>> Patch v1->v2 changes:
>>>> removed flagging of the new feature
>>>> removed unnecesssary rpr0521_update_scan_mode()
>>>> removed unnecessary comments.
>>>> added IIO_LE to channel spec
>>>>
>>>>  drivers/iio/light/rpr0521.c |  287 ++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 282 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>>> index dc522fd..72f608d 100644
>>>> --- a/drivers/iio/light/rpr0521.c
>>>> +++ b/drivers/iio/light/rpr0521.c
>>>> @@ -9,9 +9,10 @@
>>>>   *
>>>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>>>   *
>>>> - * TODO: illuminance channel, PM support, buffer
>>>> + * TODO: illuminance channel, PM support
>>>>   */
>>>>  
>>>> +
>>> Be careful to clear out stray whitespace changes like this one.
>>> Just add noise to the patch and may mean it takes longer to be accepted...
>> Ack. Need to double check next time.
>>
>>>>  #include <linux/module.h>
>>>>  #include <linux/init.h>
>>>>  #include <linux/i2c.h>
>>>> @@ -20,6 +21,10 @@
>>>>  #include <linux/acpi.h>
>>>>  
>>>>  #include <linux/iio/iio.h>
>>>> +#include <linux/iio/buffer.h>
>>>> +#include <linux/iio/trigger.h>
>>>> +#include <linux/iio/trigger_consumer.h>
>>>> +#include <linux/iio/triggered_buffer.h>
>>>>  #include <linux/iio/sysfs.h>
>>>>  #include <linux/pm_runtime.h>
>>>>  
>>>> @@ -30,6 +35,7 @@
>>>>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>>>>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>>>>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
>>>> +#define RPR0521_REG_INTERRUPT		0x4A
>>>>  #define RPR0521_PS_OFFSET_LSB		0x53
>>>>  #define RPR0521_PS_OFFSET_MSB		0x54
>>>>  
>>>> @@ -44,16 +50,33 @@
>>>>  #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
>>>>  #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
>>>>  #define RPR0521_PXS_GAIN_SHIFT		4
>>>> +#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
>>>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
>>>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
>>>> +#define RPR0521_INTERRUPT_INT_LATCH_MASK	BIT(2)
>>>> +#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
>>>> +#define RPR0521_INTERRUPT_INT_MODE_MASK		GENMASK(5, 4)
>>>>  
>>>>  #define RPR0521_MODE_ALS_ENABLE		BIT(7)
>>>>  #define RPR0521_MODE_ALS_DISABLE	0x00
>>>>  #define RPR0521_MODE_PXS_ENABLE		BIT(6)
>>>>  #define RPR0521_MODE_PXS_DISABLE	0x00
>>>> +#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
>>>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
>>>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
>>>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
>>>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
>>>> +#define RPR0521_INTERRUPT_INT_LATCH_DISABLE	BIT(2)
>>>> +#define RPR0521_INTERRUPT_INT_LATCH_ENABLE	0x00
>>>> +#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
>>>> +#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
>>>> +#define RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT	BIT(5)
>>>>  
>>>>  #define RPR0521_MANUFACT_ID		0xE0
>>>>  #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
>>>>  
>>>>  #define RPR0521_DRV_NAME		"RPR0521"
>>>> +#define RPR0521_IRQ_NAME		"rpr0521_event"
>>>>  #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
>>>>  
>>>>  #define RPR0521_SLEEP_DELAY_MS	2000
>>>> @@ -169,6 +192,9 @@ struct rpr0521_data {
>>>>  	bool als_dev_en;
>>>>  	bool pxs_dev_en;
>>>>  
>>>> +	struct iio_trigger *drdy_trigger0;
>>>> +	bool drdy_trigger_on;
>>>> +
>>>>  	/* optimize runtime pm ops - enable device only if needed */
>>>>  	bool als_ps_need_en;
>>>>  	bool pxs_ps_need_en;
>>>> @@ -195,6 +221,13 @@ static const struct attribute_group rpr0521_attribute_group = {
>>>>  	.attrs = rpr0521_attributes,
>>>>  };
>>>>  
>>>> +/* Order of the channel data in buffer */
>>>> +enum rpr0521_scan_index_order {
>>>> +	RPR0521_CHAN_INDEX_PXS,
>>>> +	RPR0521_CHAN_INDEX_BOTH,
>>>> +	RPR0521_CHAN_INDEX_IR,
>>>> +};
>>>> +
>>>>  static const struct iio_chan_spec rpr0521_channels[] = {
>>>>  	{
>>>>  		.type = IIO_PROXIMITY,
>>>> @@ -203,6 +236,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>>>  			BIT(IIO_CHAN_INFO_OFFSET) |
>>>>  			BIT(IIO_CHAN_INFO_SCALE),
>>>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>>> +		.scan_index = RPR0521_CHAN_INDEX_PXS,
>>>> +		.scan_type = {
>>>> +			.sign = 'u',
>>>> +			.realbits = 16,
>>>> +			.storagebits = 16,
>>>> +			.shift = 0,
>>>> +			.endianness = IIO_LE,
>>>> +		},
>>>>  	},
>>>>  	{
>>>>  		.type = IIO_INTENSITY,
>>>> @@ -212,6 +253,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>>  			BIT(IIO_CHAN_INFO_SCALE),
>>>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>>> +		.scan_index = RPR0521_CHAN_INDEX_BOTH,
>>>> +		.scan_type = {
>>>> +			.sign = 'u',
>>>> +			.realbits = 16,
>>>> +			.storagebits = 16,
>>>> +			.shift = 0,
>>>> +			.endianness = IIO_LE,
>>>> +		},
>>>>  	},
>>>>  	{
>>>>  		.type = IIO_INTENSITY,
>>>> @@ -221,9 +270,154 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>>  			BIT(IIO_CHAN_INFO_SCALE),
>>>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>>> +		.scan_index = RPR0521_CHAN_INDEX_IR,
>>>> +		.scan_type = {
>>>> +			.sign = 'u',
>>>> +			.realbits = 16,
>>>> +			.storagebits = 16,
>>>> +			.shift = 0,
>>> shift default makes sense as 0 so don't bother specifying it
>>> (will be set to 0 anyway)
>> Ack.
>>>> +			.endianness = IIO_LE,
>>>> +		},
>>>>  	},
>>>>  };
>>>>  
>>>> +static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>>> +				   u8 device_mask);
>>>> +static int rpr0521_als_enable(struct rpr0521_data *data, u8 status);
>>>> +static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status);
>>> These forward definitions rather look like they could be removed with a bit
>>> of reorganization fo the file.  Do that in preference to having these in
>>> the middle of the code.
>> Ok.
>>
>>>> +
>>>> +/* IRQ to trigger -handler */
>>> why the -?
>>>> +static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
>>>> +{
>>>> +	struct iio_dev *indio_dev = private;
>>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>>> +
>>>> +	/* Notify trigger0 consumers/subscribers */
>>>> +	if (data->drdy_trigger_on)
>>>> +		iio_trigger_poll(data->drdy_trigger0);
>>> How else are you getting here? Also the core will be fine
>>> if nothing cares and the trigger is polled.
>>> If there is another cause, we aren't handling it so should claim
>>> to be doing so.
>> Sometimes there is static on line and interrupt is generated even though
>> sensor interrupt output is in high impedance mode. What I've seen it's
>> max 2 interrupts in second, usually none. This doesn't matter much for
>> stability if sensor is switched on. It just causes extra reads. But if
>> extra interrupt happens when sensor power is switched off it isn't there
>> to respond to i2c data reading.
>> I don't know enough to know if all systems can handle this gracefully or
>> not. That's why safe playing here.
>> I'm keeping it unless someone can confirm that 2Hz regmap_bulk_read in
>> trigger_consumer_handler will not be causing ill effects on system when
>> there is no answer from sensor.
> Perhaps add a comment to mention that an issue has been observed.
> Normally we don't put this sort of code in to protect against hardware
> bugs like this, but if you have a platform where it is true, then fine
> but it needs to be mentioned, so someone doesn't come along later
> and remove what looks like pointless code!

Ok.

>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>>>> +{
>>>> +	struct iio_poll_func *pf = p;
>>>> +	struct iio_dev *indio_dev = pf->indio_dev;
>>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>>> +	int ret;
>>>> +
>>>> +	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
>>>> +
>>>> +	ret = regmap_bulk_read(data->regmap,
>>>> +		RPR0521_REG_PXS_DATA,
>>>> +		&buffer,
>>>> +		(3*2)+1);	/* 3 * 16-bit + (discarded) int clear reg. */
>>> kernel style is spaces around the * and +
>> Ack.
>>>> +	if (ret < 0)
>>>> +		goto done;
>>>> +
>>>> +	iio_push_to_buffers_with_timestamp(indio_dev,
>>>> +		buffer,
>>> Dont wrap more than you have two.  So make this two lines perhaps.
>>>> +		pf->timestamp);
>>>> +done:
>> Refactoring to v3 to get rid of done:
>>>> +	iio_trigger_notify_done(indio_dev->trig);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int rpr0521_write_int_enable(struct rpr0521_data *data)
>>>> +{
>>>> +	int err;
>>>> +
>>>> +	/* Interrupt after each measurement */
>>>> +	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
>>>> +		RPR0521_PXS_PERSISTENCE_MASK,
>>>> +		RPR0521_PXS_PERSISTENCE_DRDY);
>>>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>>> umm. really?  All of them? Are we looking at a clear the whole register
>>> case?  If so we should probably only clear relevant ones..
>> Hmm. .  Valid point. In drdy case we can write what ever to latch and
>> mode. This will be cleaner with write instead up write_bits.
>> ==> change to v3
>>> I don't like these stacked error handlers (err = err | *).
>>> Just handle each err as and when.  It's more code, but easier
>>> to review / follow.
>> It's (err = err || *), bail out on first error.
>> Matter of taste, ack.
>>
>>>> +		RPR0521_INTERRUPT_INT_REASSERT_MASK |
>>>> +		RPR0521_INTERRUPT_INT_LATCH_MASK |
>>>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>>>> +		RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>>>> +		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
>>>> +		RPR0521_INTERRUPT_INT_LATCH_ENABLE |
>>>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>>>> +		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
>>>> +		);
>>>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>>>> +		RPR0521_INTERRUPT_INT_MODE_MASK,
>>>> +		RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT);
>>>> +	if (err) {
>>>> +		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
>>>> +		return -EBUSY;
>>>> +		}
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rpr0521_write_int_disable(struct rpr0521_data *data)
>>>> +{
>>>> +	return regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>>>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>>>> +				RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>>>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>>>> +				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
>>>> +				);
>>>> +}
>>>> +
>>>> +/* Trigger -enable/disable */
>>>> +static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
>>>> +	bool enable_drdy)
>>>> +{
>>> This may be called from the moment iio_trigger_register is called until
>>> the trigger is unregistered on remove.
>>>
>>> Could that potentially be a problem as it might occur after the device
>>> is powered off in remove?  Hard to be sure without diving into the data
>>> sheet.  If it if safe as it is, then use the devm functions as described
>>> below, but perhaps put a note here about the fact it is safe to have
>>> this potential race.
>> Sensor itself doesn't care much, but since measurement is enabled we are
>> draining power for nothing.
>> I didn't like to touch pm_ -stuff, since I didn't know much of it. But
>> now since I did, I think I found couple of bugs both in the current code
>> and new code.
>>
>> There is another thing for the order. Int pin keeps state on power down.
>> So shutting down sensor before clearing interrupt might cause power
>> drain also on interrupt line.
>>
>> ==>
>> Reordering the _probe() and _remove() for v3.
>> Adding int clear to poweroff.
>> Adding couple commits for pm bugfixes.
>>
>> One question though. Is it ok to register device first, and then
>> trigger/buffer stuff? For now it seems like correct order.
> No.  The moment the register occurs, it's exposed to userspace.
> The buffer at least is hidden until after that.

Ok, I already forgot this was  also said in:
http://lxr.free-electrons.com/source/drivers/iio/buffer/industrialio-triggered-buffer.c?v=4.4#L37
So, setup buffer first, then register device. And on remove, unregister
device first, then remove buffer.

> Trigger vs device is more a matter of convention. Tends to be easier to
> make sure the trigger register doesn't cause trouble if something tries
> to use it early than the whole device (the userspace surface is smaller).
>>>> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
>>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>>> +	int err;
>>>> +
>>>> +	if (enable_drdy) {
>>>> +		/* ENABLE DRDY */
>>>> +		mutex_lock(&data->lock);
>>>> +		err = rpr0521_set_power_state(data, true,
>>>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>>>> +		mutex_unlock(&data->lock);
>>>> +		if (err)
>>>> +			goto rpr0521_set_state_err;
>>>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>> Removing rpr0521_[pxs/als]_[en/dis]able(), since they are by default
>> enabled from rpr0521_init and _set_power_state sets them on if pm has
>> disabled them.
>>>> +		if (err)
>>>> +			goto rpr0521_set_state_err;
>>>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>>> +		if (err)
>>>> +			goto rpr0521_set_state_err;
>>>> +		err = rpr0521_write_int_enable(data);
>>>> +		if (err)
>>>> +			goto rpr0521_set_state_err;
>>>> +	} else {
>>>> +		/* DISABLE DRDY */
>>>> +		mutex_lock(&data->lock);
>>>> +		err = rpr0521_set_power_state(data, false,
>>>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>>>> +		mutex_unlock(&data->lock);
>>>> +		if (err)
>>>> +			goto rpr0521_set_state_err;
>>>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_DISABLE);
>>>> +		if (err)
>>>> +			goto rpr0521_set_state_err;
>>>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_DISABLE);
>>>> +		if (err)
>>>> +			goto rpr0521_set_state_err;
>>>> +		err = rpr0521_write_int_disable(data);
>>>> +		if (err)
>>>> +			goto rpr0521_set_state_err;
>>>> +	}
>>>> +	data->drdy_trigger_on = enable_drdy;
>>>> +	return 0;
>>>> +
>>>> +rpr0521_set_state_err:
>>>> +	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
>>>> +	return err;
>>>> +}
>>>> +
>>>> +static const struct iio_trigger_ops rpr0521_trigger_ops = {
>>>> +	.set_trigger_state = rpr0521_pxs_drdy_set_state,
>>>> +	.owner = THIS_MODULE,
>>>> +	};
>>>> +
>>>>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>>>>  {
>>>>  	int ret;
>>>> @@ -454,6 +648,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>>>  		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
>>>>  			return -EINVAL;
>>>>  
>>>> +		if (iio_buffer_enabled(indio_dev))
>>>> +			return -EBUSY;
>>>> +
>>>>  		device_mask = rpr0521_data_reg[chan->address].device_mask;
>>>>  
>>>>  		mutex_lock(&data->lock);
>>>> @@ -664,20 +861,90 @@ static int rpr0521_probe(struct i2c_client *client,
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> -	ret = pm_runtime_set_active(&client->dev);
>>>> -	if (ret < 0)
>>>> -		return ret;
>>>> +	/* IRQ to trigger setup */
>>>> +	if (client->irq) {
>>>> +		/* Trigger0 producer setup */
>>>> +		data->drdy_trigger0 = devm_iio_trigger_alloc(
>>>> +			indio_dev->dev.parent,
>>>> +			"%s-dev%d",
>>>> +			indio_dev->name,
>>>> +			indio_dev->id);
>>>> +		if (!data->drdy_trigger0) {
>>>> +			ret = -ENOMEM;
>>>> +			return ret;
>>>> +		}
>>>> +		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
>>>> +		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
>>>> +		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
>>>> +
>>>> +		ret = iio_trigger_register(data->drdy_trigger0);
>>> devm_iio_trigger_register should be fine
>> See below.
>>>> +		if (ret) {
>>>> +			dev_err(&client->dev, "iio trigger register failed\n");
>>>> +			return ret;
>>>> +		}
>>>>  
>>>> +		/* Set trigger0 as default trigger (and inc refcount) */
>>>> +		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
>>>> +
>>>> +		/* Ties irq to trigger producer handler. */
>>>> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
>>>> +			rpr0521_drdy_irq_handler,
>>>> +			NULL,
>>>> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>>> +			RPR0521_IRQ_NAME,
>>>> +			indio_dev);
>>>> +		if (ret < 0) {
>>>> +			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
>>>> +				client->irq);
>>>> +			goto err_iio_trigger_put_unregister;
>>>> +			}
>>>> +		/*
>>>> +		 * Now whole pipe from physical interrupt (irq defined by
>>>> +		 * devicetree to device) to trigger0 output is set up.
>>>> +		 */
>>>> +
>>>> +		/* Trigger consumer setup */
>>>> +		ret = iio_triggered_buffer_setup(indio_dev,
>>>> +			&iio_pollfunc_store_time,
>>>> +			rpr0521_trigger_consumer_handler,
>>>> +			NULL);		//Use default _ops
>>>> +		if (ret < 0) {
>>>> +			dev_err(&client->dev, "iio triggered buffer setup failed\n");
>>>> +			/* Count on devm to free_irq */
>>>> +			goto err_iio_trigger_put_unregister;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	ret = pm_runtime_set_active(&client->dev);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&client->dev, "set_active failed\n");
>>> I'm not particularly bothered either way by the change, but technically
>>> adding this error message is unconnected to the subject of this patch so
>>> should not be here.
>>>
>>>> +		goto err_iio_unregister_trigger_consumer;
>>>> +		}
>>>>  	pm_runtime_enable(&client->dev);
>>>>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>>>>  	pm_runtime_use_autosuspend(&client->dev);
>>>>  
>>>>  	return iio_device_register(indio_dev);
>>>> +
>>> One line is enough.
>> Ok.
>>>> +
>>> Use devm versions and this lot goes away.
>>>> +err_iio_unregister_trigger_consumer:
>>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>>> +err_iio_trigger_put_unregister:
>>>> +	if (indio_dev->trig) {
>>>> +		iio_trigger_put(indio_dev->trig);
>>>> +		indio_dev->trig = NULL;
>>>> +		}
>>>> +	if (data->drdy_trigger0) {
>>>> +		iio_trigger_unregister(data->drdy_trigger0);
>>>> +		data->drdy_trigger0 = NULL;
>>>> +		}
>>>> +	return ret;
>>>>  }
>>>>  
>>>>  static int rpr0521_remove(struct i2c_client *client)
>>>>  {
>>>>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>>>  
>>>>  	iio_device_unregister(indio_dev);
>>>>  
>>>> @@ -685,8 +952,18 @@ static int rpr0521_remove(struct i2c_client *client)
>>>>  	pm_runtime_set_suspended(&client->dev);
>>>>  	pm_runtime_put_noidle(&client->dev);
>>>>  
>>>> -	rpr0521_poweroff(iio_priv(indio_dev));
>>>> +	rpr0521_poweroff(data);
>>>>  
>>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>>> +	if (indio_dev->trig) {
>>>> +		iio_trigger_put(indio_dev->trig);
>>>> +		indio_dev->trig = NULL;
>>>> +		}
>>>> +	if (data->drdy_trigger0) {
>>>> +		iio_trigger_unregister(data->drdy_trigger0);
>>>> +		data->drdy_trigger0 = NULL;
>>>> +		}
>>>> +	/* Rest of the cleanup is done by devm. */
>>> Givem where you have them, I don't think anything stops you using devm to
>>> handle the trigger registration and buffer cleanup.  No need to set
>>> either of hte magic flags to NULL as far as I can see...
>> For a second let's think we removed this line
>>
>>>> +	if (data->drdy_trigger_on)
>> from irq_handler().
>> If irq handler is not yet unregistered by devm_ and thus interrupt can
>> be generated (f.ex. by static),
> This is one of the nasty reasons why we tend to not spend all that much
> effort dealing with false interrupts. It's almost always possible to find
> a race where they will crash things.
>> then what does
>> iio_trigger_poll(data->drdy_trigger0) do with this unregistered trigger
>> pointer? If call is ok, then no need for NULL I guess.
> Firstly there is always a race here if this is an issue, you are just
> making it smaller.   This is all going to occur very fast in any case.
>
> Still all irrelevant anyway ;)

Removing NULL's for v3.

>>> Use the devm versions and remove should be unchanged...
>> Copy-paste from another post:
>>
>> Can't, since no devm_ on kernel 4.4.
>> I could make another commit to change to devm_, but I don't have target
>> to test it.
>>
>>> OK. We can leave it for someone else to clean up in future.
>>> However - look above - there is a race in how you have it currently ordered
>>> that means you should order it differently and not use the devm versions.
>> Ack.
>>
>>>>  	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
>>