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

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

[Mikko Koivunen]

Sensor was marked enabled on each call even if the call was for disabling
sensor.
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 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.9.1

[PATCH 2/6] 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.9.1

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

[Mikko Koivunen]

Added sysfs read/write sample frequency.
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.

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

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 30c2592..13e2f9a 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -132,6 +132,30 @@ struct rpr0521_reg_desc {
 	},
 };
 
+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,14 @@ 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 +199,7 @@ struct rpr0521_data {
 		.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 +208,14 @@ struct rpr0521_data {
 		.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 +351,55 @@ 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 +446,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 +473,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.9.1

[PATCH 4/6] 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.

Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
Builds OK with torvalds/linux 4.9
checkpatch.pl OK.

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

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 13e2f9a..579bf6d 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)
@@ -214,7 +217,9 @@ struct rpr0521_data {
 		.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),
 	}
 };
@@ -400,6 +405,42 @@ static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
 		i);
 }
 
+int rpr0521_read_ps_offset(struct rpr0521_data *data, int *offset)
+{
+	int ret;
+	u8 buffer[2];
+
+	ret = regmap_bulk_read(data->regmap,
+		RPR0521_PS_OFFSET_LSB, &buffer, 2);
+
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to read PS OFFSET register\n");
+		return ret;
+	}
+	*offset = buffer[0] + (buffer[1] << 8);
+
+	return ret;
+}
+
+int rpr0521_write_ps_offset(struct rpr0521_data *data, int offset)
+{
+	int ret;
+	u8 buffer[2];
+
+	buffer[0] = offset & 0xff;
+	buffer[1] = (offset >> 8) & (BIT(0) | BIT(1));
+
+	ret = regmap_raw_write(data->regmap,
+		RPR0521_PS_OFFSET_LSB, &buffer, 2);
+
+	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)
@@ -456,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;
 	}
@@ -483,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.9.1

[PATCH 5/6] 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 579bf6d..fa642b7 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -80,9 +80,9 @@ struct rpr0521_gain {
 };
 
 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 @@ struct rpr0521_reg_desc {
 		.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 @@ struct rpr0521_reg_desc {
 	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 @@ struct rpr0521_reg_desc {
 		.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 {
@@ -196,6 +196,14 @@ struct rpr0521_data {
 
 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,
@@ -213,15 +221,6 @@ struct rpr0521_data {
 			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.9.1

[PATCH 6/6] 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.

Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
Builds OK with torvalds/linux 4.9
checkpatch.pl OK.

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

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index fa642b7..22cb097 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -9,9 +9,11 @@
  *
  * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
  *
- * TODO: illuminance channel, PM support, buffer
+ * TODO: illuminance channel, PM support
  */
 
+#define RPR0521_TRIGGERS
+
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
@@ -20,6 +22,12 @@
 #include <linux/acpi.h>
 
 #include <linux/iio/iio.h>
+#ifdef RPR0521_TRIGGERS
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#endif
 #include <linux/iio/sysfs.h>
 #include <linux/pm_runtime.h>
 
@@ -30,6 +38,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 +53,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 +195,11 @@ struct rpr0521_data {
 	bool als_dev_en;
 	bool pxs_dev_en;
 
+#ifdef RPR0521_TRIGGERS
+	struct iio_trigger *drdy_trigger0;
+	bool drdy_trigger_on;
+	u16 *scan_buffer;
+#endif
 	/* optimize runtime pm ops - enable device only if needed */
 	bool als_ps_need_en;
 	bool pxs_ps_need_en;
@@ -194,6 +225,13 @@ struct rpr0521_data {
 	.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,
@@ -202,6 +240,13 @@ struct rpr0521_data {
 			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,
+		},
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -211,6 +256,13 @@ struct rpr0521_data {
 		.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,
+		},
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -220,9 +272,155 @@ struct rpr0521_data {
 		.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,
+		},
 	},
 };
 
+#ifdef RPR0521_TRIGGERS
+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,
+	};
+#endif
+
 static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
 {
 	int ret;
@@ -454,6 +652,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);
@@ -542,11 +743,28 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int rpr0521_update_scan_mode(struct iio_dev *indio_dev,
+	const unsigned long *scan_mask)
+{
+	struct rpr0521_data *data = iio_priv(indio_dev);
+
+	mutex_lock(&data->lock);
+	kfree(data->scan_buffer);
+	data->scan_buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
+	mutex_unlock(&data->lock);
+
+	if (data->scan_buffer == NULL)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static const struct iio_info rpr0521_info = {
 	.driver_module	= THIS_MODULE,
 	.read_raw	= rpr0521_read_raw,
 	.write_raw	= rpr0521_write_raw,
 	.attrs		= &rpr0521_attribute_group,
+	.update_scan_mode = &rpr0521_update_scan_mode,
 };
 
 static int rpr0521_init(struct rpr0521_data *data)
@@ -664,20 +882,95 @@ static int rpr0521_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = pm_runtime_set_active(&client->dev);
-	if (ret < 0)
-		return ret;
+#ifdef RPR0521_TRIGGERS
+	/* 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;
+			/* since devm, we can bail out with 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;
+		}
+	}
+#endif
+
+	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:
+#ifdef RPR0521_TRIGGERS
+	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;
+		}
+#endif
+	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 +978,21 @@ 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);
 
+#ifdef RPR0521_TRIGGERS
+	iio_triggered_buffer_cleanup(indio_dev);
+	kfree(data->scan_buffer);
+	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;
+		}
+#endif
+	/* Rest of the cleanup is done by devm. */
 	return 0;
 }
 
-- 
1.9.1

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

[Daniel Baluta]

On Thu, Apr 6, 2017 at 9:37 AM, Mikko Koivunen
<mikko.koivunen@fi.rohmeurope.com> 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.
>
> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
> Builds OK with torvalds/linux 4.9
> checkpatch.pl OK.
>

Comments like this will not matter for commit history. It's only
useful now, so I would suggest adding them under the scissor line.

> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> ---
^ this is the scissor line.

>  drivers/iio/light/rpr0521.c | 316 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 311 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index fa642b7..22cb097 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -9,9 +9,11 @@
>   *
>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>   *
> - * TODO: illuminance channel, PM support, buffer
> + * TODO: illuminance channel, PM support
>   */
>
> +#define RPR0521_TRIGGERS

Why do we need this define?

> +
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
> @@ -20,6 +22,12 @@
>  #include <linux/acpi.h>
>
>  #include <linux/iio/iio.h>
> +#ifdef RPR0521_TRIGGERS
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#endif
>  #include <linux/iio/sysfs.h>
>  #include <linux/pm_runtime.h>
>
> @@ -30,6 +38,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 +53,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 +195,11 @@ struct rpr0521_data {
>         bool als_dev_en;
>         bool pxs_dev_en;
>
> +#ifdef RPR0521_TRIGGERS
> +       struct iio_trigger *drdy_trigger0;
> +       bool drdy_trigger_on;
> +       u16 *scan_buffer;
> +#endif
>         /* optimize runtime pm ops - enable device only if needed */
>         bool als_ps_need_en;
>         bool pxs_ps_need_en;
> @@ -194,6 +225,13 @@ struct rpr0521_data {
>         .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,
> @@ -202,6 +240,13 @@ struct rpr0521_data {
>                         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,
> +               },
>         },
>         {
>                 .type = IIO_INTENSITY,
> @@ -211,6 +256,13 @@ struct rpr0521_data {
>                 .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,
> +               },
>         },
>         {
>                 .type = IIO_INTENSITY,
> @@ -220,9 +272,155 @@ struct rpr0521_data {
>                 .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,
> +               },
>         },
>  };
>
> +#ifdef RPR0521_TRIGGERS
> +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);

Is there any reason to continue if regmap_update_bits fails?

err = err || regmap_update_bits(), this looks strange :).

Daniel.

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

[Koivunen, Mikko]

On 6.4.2017 10:55, Daniel Baluta wrote:
> On Thu, Apr 6, 2017 at 9:37 AM, Mikko Koivunen
> <mikko.koivunen@fi.rohmeurope.com> 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.
>>
>> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
>> Builds OK with torvalds/linux 4.9
>> checkpatch.pl OK.
>>
> Comments like this will not matter for commit history. It's only
> useful now, so I would suggest adding them under the scissor line.
>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> ---
> ^ this is the scissor line.

Ack.

>>  drivers/iio/light/rpr0521.c | 316 +++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 311 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index fa642b7..22cb097 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -9,9 +9,11 @@
>>   *
>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>   *
>> - * TODO: illuminance channel, PM support, buffer
>> + * TODO: illuminance channel, PM support
>>   */
>>
>> +#define RPR0521_TRIGGERS
> Why do we need this define?

No you don't. This was old habit of getting extra code out when not
using the feature. Removing.

>> +
>>  #include <linux/module.h>
>>  #include <linux/init.h>
>>  #include <linux/i2c.h>
>> @@ -20,6 +22,12 @@
>>  #include <linux/acpi.h>
>>
>>  #include <linux/iio/iio.h>
>> +#ifdef RPR0521_TRIGGERS
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#endif
>>  #include <linux/iio/sysfs.h>
>>  #include <linux/pm_runtime.h>
>>
>> @@ -30,6 +38,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 +53,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 +195,11 @@ struct rpr0521_data {
>>         bool als_dev_en;
>>         bool pxs_dev_en;
>>
>> +#ifdef RPR0521_TRIGGERS
>> +       struct iio_trigger *drdy_trigger0;
>> +       bool drdy_trigger_on;
>> +       u16 *scan_buffer;
>> +#endif
>>         /* optimize runtime pm ops - enable device only if needed */
>>         bool als_ps_need_en;
>>         bool pxs_ps_need_en;
>> @@ -194,6 +225,13 @@ struct rpr0521_data {
>>         .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,
>> @@ -202,6 +240,13 @@ struct rpr0521_data {
>>                         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,
>> +               },
>>         },
>>         {
>>                 .type = IIO_INTENSITY,
>> @@ -211,6 +256,13 @@ struct rpr0521_data {
>>                 .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,
>> +               },
>>         },
>>         {
>>                 .type = IIO_INTENSITY,
>> @@ -220,9 +272,155 @@ struct rpr0521_data {
>>                 .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,
>> +               },
>>         },
>>  };
>>
>> +#ifdef RPR0521_TRIGGERS
>> +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);
> Is there any reason to continue if regmap_update_bits fails?
>
> err = err || regmap_update_bits(), this looks strange :).

No reason to continue after fail.
It will skip the rest of the lines since rest of the lines get evaluated
already true on "= err ||" before executing the functions after "||".

--

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

[Koivunen, Mikko]

On 5.4.2017 15:14, Peter Meerwald-Stadler wrote:
> On Wed, 5 Apr 2017, 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.
> actually, there is a nasty #define which shouldn't be needed
> comments below

Ack.

>> Trigger consumer reads rpr0521 data to scan buffer.
>> Depends on previous commits of _scale and _offset.
>>
>> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
>> Builds OK with torvalds/linux 4.9
>> checkpatch.pl OK.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> ---
>>  drivers/iio/light/rpr0521.c | 316 +++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 311 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index fa642b7..22cb097 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -9,9 +9,11 @@
>>   *
>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>   *
>> - * TODO: illuminance channel, PM support, buffer
>> + * TODO: illuminance channel, PM support
>>   */
>>  
>> +#define RPR0521_TRIGGERS
> no, don't

Ack.

>> +
>>  #include <linux/module.h>
>>  #include <linux/init.h>
>>  #include <linux/i2c.h>
>> @@ -20,6 +22,12 @@
>>  #include <linux/acpi.h>
>>  
>>  #include <linux/iio/iio.h>
>> +#ifdef RPR0521_TRIGGERS
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#endif
>>  #include <linux/iio/sysfs.h>
>>  #include <linux/pm_runtime.h>
>>  
>> @@ -30,6 +38,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 +53,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 +195,11 @@ struct rpr0521_data {
>>  	bool als_dev_en;
>>  	bool pxs_dev_en;
>>  
>> +#ifdef RPR0521_TRIGGERS
>> +	struct iio_trigger *drdy_trigger0;
>> +	bool drdy_trigger_on;
>> +	u16 *scan_buffer;
>> +#endif
>>  	/* optimize runtime pm ops - enable device only if needed */
>>  	bool als_ps_need_en;
>>  	bool pxs_ps_need_en;
>> @@ -194,6 +225,13 @@ struct rpr0521_data {
>>  	.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,
>> @@ -202,6 +240,13 @@ struct rpr0521_data {
>>  			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,
> the data is little endian, needs to be specified here

Ack.

>
>> +			.shift = 0,
>> +		},
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -211,6 +256,13 @@ struct rpr0521_data {
>>  		.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,
>> +		},
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -220,9 +272,155 @@ struct rpr0521_data {
>>  		.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,
>> +		},
>>  	},
>>  };
>>  
>> +#ifdef RPR0521_TRIGGERS
>> +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,
>> +	};
>> +#endif
>> +
>>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>>  {
>>  	int ret;
>> @@ -454,6 +652,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);
>> @@ -542,11 +743,28 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>>  	}
>>  }
>>  
>> +static int rpr0521_update_scan_mode(struct iio_dev *indio_dev,
>> +	const unsigned long *scan_mask)
>> +{
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +
> this is super-ugly; probably just allocate the maximum possible allocation 
> size?

Actually this is not needed at all. Receiving buffer is already in
rpr0521_trigger_consumer_handler() and this buffer is not even used.
Removing this.
There is probably need to recheck other drivers too for this pattern.

>> +	mutex_lock(&data->lock);
>> +	kfree(data->scan_buffer);
>> +	data->scan_buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> +	mutex_unlock(&data->lock);
>> +
>> +	if (data->scan_buffer == NULL)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct iio_info rpr0521_info = {
>>  	.driver_module	= THIS_MODULE,
>>  	.read_raw	= rpr0521_read_raw,
>>  	.write_raw	= rpr0521_write_raw,
>>  	.attrs		= &rpr0521_attribute_group,
>> +	.update_scan_mode = &rpr0521_update_scan_mode,
> no & for functions

Ack. + removing the function.


>>  };
>>  
>>  static int rpr0521_init(struct rpr0521_data *data)
>> @@ -664,20 +882,95 @@ static int rpr0521_probe(struct i2c_client *client,
>>  		return ret;
>>  	}
>>  
>> -	ret = pm_runtime_set_active(&client->dev);
>> -	if (ret < 0)
>> -		return ret;
>> +#ifdef RPR0521_TRIGGERS
>> +	/* 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_?

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.

>> +		if (ret) {
>> +			dev_err(&client->dev, "iio trigger register failed\n");
>> +			return ret;
>> +			/* since devm, we can bail out with ret */
> no need to document that...

Ack.

>> +		}
>> +
>> +		/* 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,
> devm_?

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.

>> +			&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;
>> +		}
>> +	}
>> +#endif
>> +
>> +	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:
>> +#ifdef RPR0521_TRIGGERS
>> +	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;
>> +		}
>> +#endif
>> +	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 +978,21 @@ 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);
>>  
>> +#ifdef RPR0521_TRIGGERS
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +	kfree(data->scan_buffer);
>> +	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;
>> +		}
>> +#endif
>> +	/* Rest of the cleanup is done by devm. */
>>  	return 0;
>>  }
>>  
>>

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

[Jonathan Cameron]

On 07/04/17 12:54, Koivunen, Mikko wrote:
> On 5.4.2017 15:14, Peter Meerwald-Stadler wrote:
>> On Wed, 5 Apr 2017, 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.
>> actually, there is a nasty #define which shouldn't be needed
>> comments below
> 
> Ack.
> 
>>> Trigger consumer reads rpr0521 data to scan buffer.
>>> Depends on previous commits of _scale and _offset.
>>>
>>> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
>>> Builds OK with torvalds/linux 4.9
>>> checkpatch.pl OK.
>>>
>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>>> ---
>>>  drivers/iio/light/rpr0521.c | 316 +++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 311 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>> index fa642b7..22cb097 100644
>>> --- a/drivers/iio/light/rpr0521.c
>>> +++ b/drivers/iio/light/rpr0521.c
>>> @@ -9,9 +9,11 @@
>>>   *
>>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>>   *
>>> - * TODO: illuminance channel, PM support, buffer
>>> + * TODO: illuminance channel, PM support
>>>   */
>>>  
>>> +#define RPR0521_TRIGGERS
>> no, don't
> 
> Ack.
> 
>>> +
>>>  #include <linux/module.h>
>>>  #include <linux/init.h>
>>>  #include <linux/i2c.h>
>>> @@ -20,6 +22,12 @@
>>>  #include <linux/acpi.h>
>>>  
>>>  #include <linux/iio/iio.h>
>>> +#ifdef RPR0521_TRIGGERS
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>> +#endif
>>>  #include <linux/iio/sysfs.h>
>>>  #include <linux/pm_runtime.h>
>>>  
>>> @@ -30,6 +38,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 +53,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 +195,11 @@ struct rpr0521_data {
>>>  	bool als_dev_en;
>>>  	bool pxs_dev_en;
>>>  
>>> +#ifdef RPR0521_TRIGGERS
>>> +	struct iio_trigger *drdy_trigger0;
>>> +	bool drdy_trigger_on;
>>> +	u16 *scan_buffer;
>>> +#endif
>>>  	/* optimize runtime pm ops - enable device only if needed */
>>>  	bool als_ps_need_en;
>>>  	bool pxs_ps_need_en;
>>> @@ -194,6 +225,13 @@ struct rpr0521_data {
>>>  	.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,
>>> @@ -202,6 +240,13 @@ struct rpr0521_data {
>>>  			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,
>> the data is little endian, needs to be specified here
> 
> Ack.
> 
>>
>>> +			.shift = 0,
>>> +		},
>>>  	},
>>>  	{
>>>  		.type = IIO_INTENSITY,
>>> @@ -211,6 +256,13 @@ struct rpr0521_data {
>>>  		.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,
>>> +		},
>>>  	},
>>>  	{
>>>  		.type = IIO_INTENSITY,
>>> @@ -220,9 +272,155 @@ struct rpr0521_data {
>>>  		.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,
>>> +		},
>>>  	},
>>>  };
>>>  
>>> +#ifdef RPR0521_TRIGGERS
>>> +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,
>>> +	};
>>> +#endif
>>> +
>>>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>>>  {
>>>  	int ret;
>>> @@ -454,6 +652,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);
>>> @@ -542,11 +743,28 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>>>  	}
>>>  }
>>>  
>>> +static int rpr0521_update_scan_mode(struct iio_dev *indio_dev,
>>> +	const unsigned long *scan_mask)
>>> +{
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>> +
>> this is super-ugly; probably just allocate the maximum possible allocation 
>> size?
> 
> Actually this is not needed at all. Receiving buffer is already in
> rpr0521_trigger_consumer_handler() and this buffer is not even used.
> Removing this.
> There is probably need to recheck other drivers too for this pattern.
> 
>>> +	mutex_lock(&data->lock);
>>> +	kfree(data->scan_buffer);
>>> +	data->scan_buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>>> +	mutex_unlock(&data->lock);
>>> +
>>> +	if (data->scan_buffer == NULL)
>>> +		return -ENOMEM;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static const struct iio_info rpr0521_info = {
>>>  	.driver_module	= THIS_MODULE,
>>>  	.read_raw	= rpr0521_read_raw,
>>>  	.write_raw	= rpr0521_write_raw,
>>>  	.attrs		= &rpr0521_attribute_group,
>>> +	.update_scan_mode = &rpr0521_update_scan_mode,
>> no & for functions
> 
> Ack. + removing the function.
> 
> 
>>>  };
>>>  
>>>  static int rpr0521_init(struct rpr0521_data *data)
>>> @@ -664,20 +882,95 @@ static int rpr0521_probe(struct i2c_client *client,
>>>  		return ret;
>>>  	}
>>>  
>>> -	ret = pm_runtime_set_active(&client->dev);
>>> -	if (ret < 0)
>>> -		return ret;
>>> +#ifdef RPR0521_TRIGGERS
>>> +	/* 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_?
> 
> 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.
> 
>>> +		if (ret) {
>>> +			dev_err(&client->dev, "iio trigger register failed\n");
>>> +			return ret;
>>> +			/* since devm, we can bail out with ret */
>> no need to document that...
> 
> Ack.
> 
>>> +		}
>>> +
>>> +		/* 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,
>> devm_?
> 
> 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.
> 
>>> +			&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;
>>> +		}
>>> +	}
>>> +#endif
>>> +
>>> +	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:
>>> +#ifdef RPR0521_TRIGGERS
>>> +	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;
>>> +		}
>>> +#endif
>>> +	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 +978,21 @@ 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);
>>>  
>>> +#ifdef RPR0521_TRIGGERS
>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>> +	kfree(data->scan_buffer);
>>> +	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;
>>> +		}
>>> +#endif
>>> +	/* Rest of the cleanup is done by devm. */
>>>  	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
>