[PATCH v4 0/4] iio: vcnl: Add interrupts support for VCNL4010/20.

[PATCH v4 0/4] iio: vcnl: Add interrupts support for VCNL4010/20.

[Mathieu Othacehe]

Hello,

Here's a v4 addressing Peter and Andy remarks. The most important change is
the removal of most mutex uses, as i2c accesses are already protected
by the i2c framework.

Thanks,

Mathieu

Changes from v3:
* Use i2c_smbus_read_byte_data and i2c_smbus_write_word_data
for read and write functions.
* Rename vcnl4010_prox_threshold to vcnl4010_config_threshold.
* Do not lock i2c accesses as they are already protected.
* Fix a typo in irq name.
* Do not provide ALS sampling frequency operation, as ALS data
are not buffered anymore.
* Return bool in vcnl4010_in_periodic_mode and vcnl4010_thr_enabled
functions.

Changes from v2:
* Rebase on iio testing branch.
* Remove useless test in vcnl4010_probe_trigger.

Changes from v1:
* Split into four different patches.
* Use iio_device_claim_direct_mode to protect
raw access from buffer capture.
* Requesting a sampling frequency above the limit is no longer possible.
* Inline read_isr and write_isr functions.
* Remove IIO_LIGHT data from buffer capture.
* Make sure postenable and predisable functions respect the common form.
* Do not set the trigger by default.
* Remove the devm_iio_triggered_buffer_setup top half.

Mathieu Othacehe (4):
  iio: vcnl4000: Factorize data reading and writing.
  iio: vcnl4000: Add event support for VCNL4010/20.
  iio: vcnl4000: Add sampling frequency support for VCNL4010/20.
  iio: vcnl4000: Add buffer support for VCNL4010/20.

 drivers/iio/light/Kconfig    |   2 +
 drivers/iio/light/vcnl4000.c | 747 +++++++++++++++++++++++++++++++----
 2 files changed, 683 insertions(+), 66 deletions(-)

-- 
2.26.0

[PATCH v4 1/4] iio: vcnl4000: Factorize data reading and writing.

[Mathieu Othacehe]

Factorize data reading in vcnl4000_measure into a vcnl4000_read_data
function. Also add a vcnl4000_write_data function.

Signed-off-by: Mathieu Othacehe <m.othacehe@gmail.com>
---
 drivers/iio/light/vcnl4000.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 58e97462e803..695a81e95d8d 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -215,11 +215,34 @@ static int vcnl4200_init(struct vcnl4000_data *data)
 	return 0;
 };
 
+static int vcnl4000_read_data(struct vcnl4000_data *data, u8 data_reg, int *val)
+{
+	s32 ret;
+
+	ret = i2c_smbus_read_word_data(data->client, data_reg);
+	if (ret < 0)
+		return ret;
+
+	*val = be16_to_cpu(ret);
+	return 0;
+}
+
+static int vcnl4000_write_data(struct vcnl4000_data *data, u8 data_reg, int val)
+{
+	__be16 be_val;
+
+	if (val > U16_MAX)
+		return -ERANGE;
+
+	be_val = cpu_to_be16(val);
+	return i2c_smbus_write_word_data(data->client, data_reg, be_val);
+}
+
+
 static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 				u8 rdy_mask, u8 data_reg, int *val)
 {
 	int tries = 20;
-	__be16 buf;
 	int ret;
 
 	mutex_lock(&data->vcnl4000_lock);
@@ -246,13 +269,11 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 		goto fail;
 	}
 
-	ret = i2c_smbus_read_i2c_block_data(data->client,
-		data_reg, sizeof(buf), (u8 *) &buf);
+	ret = vcnl4000_read_data(data, data_reg, val);
 	if (ret < 0)
 		goto fail;
 
 	mutex_unlock(&data->vcnl4000_lock);
-	*val = be16_to_cpu(buf);
 
 	return 0;
 
-- 
2.26.0

[PATCH v4 2/4] iio: vcnl4000: Add event support for VCNL4010/20.

[Mathieu Othacehe]

The VCNL4010 and VCNL4020 chips are able to raise interrupts on proximity
threshold events. Add support for threshold rising and falling events for
those two chips.

Signed-off-by: Mathieu Othacehe <m.othacehe@gmail.com>
---
 drivers/iio/light/vcnl4000.c | 456 ++++++++++++++++++++++++++++++-----
 1 file changed, 394 insertions(+), 62 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 695a81e95d8d..db5c15541174 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -13,7 +13,6 @@
  *
  * TODO:
  *   allow to adjust IR current
- *   proximity threshold and event handling
  *   periodic ALS/proximity measurement (VCNL4010/20)
  *   interrupts (VCNL4010/20/40, VCNL4200)
  */
@@ -23,7 +22,9 @@
 #include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/pm_runtime.h>
+#include <linux/interrupt.h>
 
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
@@ -35,14 +36,22 @@
 
 #define VCNL4000_COMMAND	0x80 /* Command register */
 #define VCNL4000_PROD_REV	0x81 /* Product ID and Revision ID */
+#define VCNL4010_PROX_RATE      0x82 /* Proximity rate */
 #define VCNL4000_LED_CURRENT	0x83 /* IR LED current for proximity mode */
 #define VCNL4000_AL_PARAM	0x84 /* Ambient light parameter register */
+#define VCNL4010_ALS_PARAM      0x84 /* ALS rate */
 #define VCNL4000_AL_RESULT_HI	0x85 /* Ambient light result register, MSB */
 #define VCNL4000_AL_RESULT_LO	0x86 /* Ambient light result register, LSB */
 #define VCNL4000_PS_RESULT_HI	0x87 /* Proximity result register, MSB */
 #define VCNL4000_PS_RESULT_LO	0x88 /* Proximity result register, LSB */
 #define VCNL4000_PS_MEAS_FREQ	0x89 /* Proximity test signal frequency */
+#define VCNL4010_INT_CTRL	0x89 /* Interrupt control */
 #define VCNL4000_PS_MOD_ADJ	0x8a /* Proximity modulator timing adjustment */
+#define VCNL4010_LOW_THR_HI     0x8a /* Low threshold, MSB */
+#define VCNL4010_LOW_THR_LO     0x8b /* Low threshold, LSB */
+#define VCNL4010_HIGH_THR_HI    0x8c /* High threshold, MSB */
+#define VCNL4010_HIGH_THR_LO    0x8d /* High threshold, LSB */
+#define VCNL4010_ISR		0x8e /* Interrupt status */
 
 #define VCNL4200_AL_CONF	0x00 /* Ambient light configuration */
 #define VCNL4200_PS_CONF1	0x03 /* Proximity configuration */
@@ -57,6 +66,26 @@
 #define VCNL4000_PS_RDY		BIT(5) /* proximity data ready? */
 #define VCNL4000_AL_OD		BIT(4) /* start on-demand ALS measurement */
 #define VCNL4000_PS_OD		BIT(3) /* start on-demand proximity measurement */
+#define VCNL4000_ALS_EN		BIT(2) /* start ALS measurement */
+#define VCNL4000_PROX_EN	BIT(1) /* start proximity measurement */
+#define VCNL4000_SELF_TIMED_EN	BIT(0) /* start self-timed measurement */
+
+/* Bit masks for interrupt registers. */
+#define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
+#define VCNL4010_INT_THR_EN	BIT(1) /* Threshold interrupt type */
+#define VCNL4010_INT_ALS_EN	BIT(2) /* Enable on ALS data ready */
+#define VCNL4010_INT_PROX_EN	BIT(3) /* Enable on proximity data ready */
+
+#define VCNL4010_INT_THR_HIGH	0 /* High threshold exceeded */
+#define VCNL4010_INT_THR_LOW	1 /* Low threshold exceeded */
+#define VCNL4010_INT_ALS	2 /* ALS data ready */
+#define VCNL4010_INT_PROXIMITY	3 /* Proximity data ready */
+
+#define VCNL4010_INT_THR \
+	(BIT(VCNL4010_INT_THR_LOW) | BIT(VCNL4010_INT_THR_HIGH))
+#define VCNL4010_INT_DRDY \
+	(BIT(VCNL4010_INT_PROXIMITY) | BIT(VCNL4010_INT_ALS))
+
 
 #define VCNL4000_SLEEP_DELAY_MS	2000 /* before we enter pm_runtime_suspend */
 
@@ -88,6 +117,10 @@ struct vcnl4000_data {
 
 struct vcnl4000_chip_spec {
 	const char *prod;
+	struct iio_chan_spec const *channels;
+	const int num_channels;
+	const struct iio_info *info;
+	bool irq_support;
 	int (*init)(struct vcnl4000_data *data);
 	int (*measure_light)(struct vcnl4000_data *data, int *val);
 	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
@@ -333,67 +366,16 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
 	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
 }
 
-static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
-	[VCNL4000] = {
-		.prod = "VCNL4000",
-		.init = vcnl4000_init,
-		.measure_light = vcnl4000_measure_light,
-		.measure_proximity = vcnl4000_measure_proximity,
-		.set_power_state = vcnl4000_set_power_state,
-	},
-	[VCNL4010] = {
-		.prod = "VCNL4010/4020",
-		.init = vcnl4000_init,
-		.measure_light = vcnl4000_measure_light,
-		.measure_proximity = vcnl4000_measure_proximity,
-		.set_power_state = vcnl4000_set_power_state,
-	},
-	[VCNL4040] = {
-		.prod = "VCNL4040",
-		.init = vcnl4200_init,
-		.measure_light = vcnl4200_measure_light,
-		.measure_proximity = vcnl4200_measure_proximity,
-		.set_power_state = vcnl4200_set_power_state,
-	},
-	[VCNL4200] = {
-		.prod = "VCNL4200",
-		.init = vcnl4200_init,
-		.measure_light = vcnl4200_measure_light,
-		.measure_proximity = vcnl4200_measure_proximity,
-		.set_power_state = vcnl4200_set_power_state,
-	},
-};
-
-static ssize_t vcnl4000_read_near_level(struct iio_dev *indio_dev,
-					uintptr_t priv,
-					const struct iio_chan_spec *chan,
-					char *buf)
+static bool vcnl4010_in_periodic_mode(struct vcnl4000_data *data)
 {
-	struct vcnl4000_data *data = iio_priv(indio_dev);
-
-	return sprintf(buf, "%u\n", data->near_level);
-}
+	int ret;
 
-static const struct iio_chan_spec_ext_info vcnl4000_ext_info[] = {
-	{
-		.name = "nearlevel",
-		.shared = IIO_SEPARATE,
-		.read = vcnl4000_read_near_level,
-	},
-	{ /* sentinel */ }
-};
+	ret = i2c_smbus_read_byte_data(data->client, VCNL4000_COMMAND);
+	if (ret < 0)
+		return false;
 
-static const struct iio_chan_spec vcnl4000_channels[] = {
-	{
-		.type = IIO_LIGHT,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_SCALE),
-	}, {
-		.type = IIO_PROXIMITY,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-		.ext_info = vcnl4000_ext_info,
-	}
-};
+	return (ret & VCNL4000_SELF_TIMED_EN) > 0;
+}
 
 static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on)
 {
@@ -453,10 +435,347 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int vcnl4010_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	int ret;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+	case IIO_CHAN_INFO_SCALE:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		/* Protect against event capture. */
+		if (vcnl4010_in_periodic_mode(data)) {
+			ret = -EBUSY;
+		} else {
+			ret = vcnl4000_read_raw(indio_dev, chan, val, val2,
+						mask);
+		}
+
+		iio_device_release_direct_mode(indio_dev);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vcnl4010_read_event(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info,
+			       int *val, int *val2)
+{
+	int ret;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = vcnl4000_read_data(data, VCNL4010_HIGH_THR_HI,
+						 val);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT;
+		case IIO_EV_DIR_FALLING:
+			ret = vcnl4000_read_data(data, VCNL4010_LOW_THR_HI,
+						 val);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vcnl4010_write_event(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int val, int val2)
+{
+	int ret;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = vcnl4000_write_data(data, VCNL4010_HIGH_THR_HI,
+						  val);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT;
+		case IIO_EV_DIR_FALLING:
+			ret = vcnl4000_write_data(data, VCNL4010_LOW_THR_HI,
+						  val);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static bool vcnl4010_thr_enabled(struct vcnl4000_data *data)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, VCNL4010_INT_CTRL);
+	if (ret < 0)
+		return false;
+
+	return (ret & VCNL4010_INT_THR_EN) > 0;
+}
+
+static int vcnl4010_read_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir)
+{
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		return vcnl4010_thr_enabled(data);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vcnl4010_config_threshold(struct iio_dev *indio_dev, bool state)
+{
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+	int ret;
+	int icr;
+	int command;
+
+	if (state) {
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		/* Enable periodic measurement of proximity data. */
+		command = VCNL4000_SELF_TIMED_EN | VCNL4000_PROX_EN;
+
+		/*
+		 * Enable interrupts on threshold, for proximity data by
+		 * default.
+		 */
+		icr = VCNL4010_INT_THR_EN;
+	} else {
+		if (!vcnl4010_thr_enabled(data))
+			return 0;
+
+		command = 0;
+		icr = 0;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
+					command);
+	if (ret < 0)
+		goto end;
+
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL, icr);
+
+end:
+	if (state)
+		iio_device_release_direct_mode(indio_dev);
+
+	return ret;
+}
+
+static int vcnl4010_write_event_config(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan,
+				       enum iio_event_type type,
+				       enum iio_event_direction dir,
+				       int state)
+{
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		return vcnl4010_config_threshold(indio_dev, state);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t vcnl4000_read_near_level(struct iio_dev *indio_dev,
+					uintptr_t priv,
+					const struct iio_chan_spec *chan,
+					char *buf)
+{
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	return sprintf(buf, "%u\n", data->near_level);
+}
+
+static const struct iio_chan_spec_ext_info vcnl4000_ext_info[] = {
+	{
+		.name = "nearlevel",
+		.shared = IIO_SEPARATE,
+		.read = vcnl4000_read_near_level,
+	},
+	{ /* sentinel */ }
+};
+
+static const struct iio_event_spec vcnl4000_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	}
+};
+
+static const struct iio_chan_spec vcnl4000_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+	}, {
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.ext_info = vcnl4000_ext_info,
+	}
+};
+
+static const struct iio_chan_spec vcnl4010_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.scan_index = -1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+	}, {
+		.type = IIO_PROXIMITY,
+		.scan_index = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.event_spec = vcnl4000_event_spec,
+		.num_event_specs = ARRAY_SIZE(vcnl4000_event_spec),
+		.ext_info = vcnl4000_ext_info,
+	},
+};
+
 static const struct iio_info vcnl4000_info = {
 	.read_raw = vcnl4000_read_raw,
 };
 
+static const struct iio_info vcnl4010_info = {
+	.read_raw = vcnl4010_read_raw,
+	.read_event_value = vcnl4010_read_event,
+	.write_event_value = vcnl4010_write_event,
+	.read_event_config = vcnl4010_read_event_config,
+	.write_event_config = vcnl4010_write_event_config,
+};
+
+static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
+	[VCNL4000] = {
+		.prod = "VCNL4000",
+		.init = vcnl4000_init,
+		.measure_light = vcnl4000_measure_light,
+		.measure_proximity = vcnl4000_measure_proximity,
+		.set_power_state = vcnl4000_set_power_state,
+		.channels = vcnl4000_channels,
+		.num_channels = ARRAY_SIZE(vcnl4000_channels),
+		.info = &vcnl4000_info,
+		.irq_support = false,
+	},
+	[VCNL4010] = {
+		.prod = "VCNL4010/4020",
+		.init = vcnl4000_init,
+		.measure_light = vcnl4000_measure_light,
+		.measure_proximity = vcnl4000_measure_proximity,
+		.set_power_state = vcnl4000_set_power_state,
+		.channels = vcnl4010_channels,
+		.num_channels = ARRAY_SIZE(vcnl4010_channels),
+		.info = &vcnl4010_info,
+		.irq_support = true,
+	},
+	[VCNL4040] = {
+		.prod = "VCNL4040",
+		.init = vcnl4200_init,
+		.measure_light = vcnl4200_measure_light,
+		.measure_proximity = vcnl4200_measure_proximity,
+		.set_power_state = vcnl4200_set_power_state,
+		.channels = vcnl4000_channels,
+		.num_channels = ARRAY_SIZE(vcnl4000_channels),
+		.info = &vcnl4000_info,
+		.irq_support = false,
+	},
+	[VCNL4200] = {
+		.prod = "VCNL4200",
+		.init = vcnl4200_init,
+		.measure_light = vcnl4200_measure_light,
+		.measure_proximity = vcnl4200_measure_proximity,
+		.set_power_state = vcnl4200_set_power_state,
+		.channels = vcnl4000_channels,
+		.num_channels = ARRAY_SIZE(vcnl4000_channels),
+		.info = &vcnl4000_info,
+		.irq_support = false,
+	},
+};
+
+static irqreturn_t vcnl4010_irq_thread(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+	unsigned long isr;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, VCNL4010_ISR);
+	if (ret < 0)
+		goto end;
+
+	isr = ret;
+
+	if (isr & VCNL4010_INT_THR) {
+		if (test_bit(VCNL4010_INT_THR_LOW, &isr)) {
+			iio_push_event(indio_dev,
+				       IIO_UNMOD_EVENT_CODE(
+					       IIO_PROXIMITY,
+					       1,
+					       IIO_EV_TYPE_THRESH,
+					       IIO_EV_DIR_FALLING),
+				       iio_get_time_ns(indio_dev));
+		}
+
+		if (test_bit(VCNL4010_INT_THR_HIGH, &isr)) {
+			iio_push_event(indio_dev,
+				       IIO_UNMOD_EVENT_CODE(
+					       IIO_PROXIMITY,
+					       1,
+					       IIO_EV_TYPE_THRESH,
+					       IIO_EV_DIR_RISING),
+				       iio_get_time_ns(indio_dev));
+		}
+
+		i2c_smbus_write_byte_data(data->client, VCNL4010_ISR,
+					  isr & VCNL4010_INT_THR);
+	}
+
+end:
+	return IRQ_HANDLED;
+}
+
 static int vcnl4000_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
@@ -486,12 +805,25 @@ static int vcnl4000_probe(struct i2c_client *client,
 		data->near_level = 0;
 
 	indio_dev->dev.parent = &client->dev;
-	indio_dev->info = &vcnl4000_info;
-	indio_dev->channels = vcnl4000_channels;
-	indio_dev->num_channels = ARRAY_SIZE(vcnl4000_channels);
+	indio_dev->info = data->chip_spec->info;
+	indio_dev->channels = data->chip_spec->channels;
+	indio_dev->num_channels = data->chip_spec->num_channels;
 	indio_dev->name = VCNL4000_DRV_NAME;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
+	if (client->irq && data->chip_spec->irq_support) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						NULL, vcnl4010_irq_thread,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						"vcnl4010_irq",
+						indio_dev);
+		if (ret < 0) {
+			dev_err(&client->dev, "irq request failed\n");
+			return ret;
+		}
+	}
+
 	ret = pm_runtime_set_active(&client->dev);
 	if (ret < 0)
 		goto fail_poweroff;
-- 
2.26.0

Re: [PATCH v4 2/4] iio: vcnl4000: Add event support for VCNL4010/20.

[Andy Shevchenko]

On Tue, Apr 21, 2020 at 10:57 AM Mathieu Othacehe <m.othacehe@gmail.com> wrote:
>
> The VCNL4010 and VCNL4020 chips are able to raise interrupts on proximity
> threshold events. Add support for threshold rising and falling events for
> those two chips.

Some nitpicks below (up to you and maintainer to address)

...

> +static bool vcnl4010_in_periodic_mode(struct vcnl4000_data *data)

Since it's boolean I would name it ..._is_in_prediodic_mode().

>  {
> +       int ret;
>
> +       ret = i2c_smbus_read_byte_data(data->client, VCNL4000_COMMAND);
> +       if (ret < 0)
> +               return false;
>

> +       return (ret & VCNL4000_SELF_TIMED_EN) > 0;

This > 0  for bitmasked values looks slightly strange. And actually if
a sign bit is included, potentially wrong.

I would rather go without any comparison, or do !!(foo & BAR).

> +}

...

> +static bool vcnl4010_thr_enabled(struct vcnl4000_data *data)

_is_thr_enabled() ?

> +{
> +       int ret;
> +
> +       ret = i2c_smbus_read_byte_data(data->client, VCNL4010_INT_CTRL);
> +       if (ret < 0)
> +               return false;
> +

> +       return (ret & VCNL4010_INT_THR_EN) > 0;

Ditto.

> +}

...

> +               ret = devm_request_threaded_irq(&client->dev, client->irq,
> +                                               NULL, vcnl4010_irq_thread,
> +                                               IRQF_TRIGGER_FALLING |

> +                                               IRQF_ONESHOT,

Isn't it by default when threaded IRQ is asked with NULL for hw handler?

> +                                               "vcnl4010_irq",
> +                                               indio_dev);


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/4] iio: vcnl4000: Add event support for VCNL4010/20.

[Mathieu Othacehe]

Hello Andy,

>> +               ret = devm_request_threaded_irq(&client->dev, client->irq,
>> +                                               NULL, vcnl4010_irq_thread,
>> +                                               IRQF_TRIGGER_FALLING |
>
>> +                                               IRQF_ONESHOT,
>
> Isn't it by default when threaded IRQ is asked with NULL for hw handler?

No, and it fails with this error message if IRQF_ONESHOT is not set:

pr_err("Threaded irq requested with handler=NULL and !ONESHOT for %s (irq %d)\n",

Thanks,

Mathieu

Re: [PATCH v4 2/4] iio: vcnl4000: Add event support for VCNL4010/20.

[Andy Shevchenko]

On Wed, Apr 22, 2020 at 11:02 AM Mathieu Othacehe <m.othacehe@gmail.com> wrote:

> >> +               ret = devm_request_threaded_irq(&client->dev, client->irq,
> >> +                                               NULL, vcnl4010_irq_thread,
> >> +                                               IRQF_TRIGGER_FALLING |
> >
> >> +                                               IRQF_ONESHOT,
> >
> > Isn't it by default when threaded IRQ is asked with NULL for hw handler?
>
> No, and it fails with this error message if IRQF_ONESHOT is not set:
>
> pr_err("Threaded irq requested with handler=NULL and !ONESHOT for %s (irq %d)\n",

Ok, thanks.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/4] iio: vcnl4000: Add event support for VCNL4010/20.

[Jonathan Cameron]

On Tue, 21 Apr 2020 09:55:30 +0200
Mathieu Othacehe <m.othacehe@gmail.com> wrote:

> The VCNL4010 and VCNL4020 chips are able to raise interrupts on proximity
> threshold events. Add support for threshold rising and falling events for
> those two chips.
> 
> Signed-off-by: Mathieu Othacehe <m.othacehe@gmail.com>
> ---
...
> +
> +static const struct iio_chan_spec vcnl4010_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.scan_index = -1,

Why introduce scan index here?  That's only used for buffers.

> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE),
> +	}, {
> +		.type = IIO_PROXIMITY,
> +		.scan_index = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.event_spec = vcnl4000_event_spec,
> +		.num_event_specs = ARRAY_SIZE(vcnl4000_event_spec),
> +		.ext_info = vcnl4000_ext_info,
> +	},
> +};
> +
>  static const struct iio_info vcnl4000_info = {
>  	.read_raw = vcnl4000_read_raw,
>  };
>  
> +static const struct iio_info vcnl4010_info = {
> +	.read_raw = vcnl4010_read_raw,
> +	.read_event_value = vcnl4010_read_event,
> +	.write_event_value = vcnl4010_write_event,
> +	.read_event_config = vcnl4010_read_event_config,
> +	.write_event_config = vcnl4010_write_event_config,
> +};
> +
> +static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> +	[VCNL4000] = {
> +		.prod = "VCNL4000",
> +		.init = vcnl4000_init,
> +		.measure_light = vcnl4000_measure_light,
> +		.measure_proximity = vcnl4000_measure_proximity,
> +		.set_power_state = vcnl4000_set_power_state,
> +		.channels = vcnl4000_channels,
> +		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> +		.info = &vcnl4000_info,
> +		.irq_support = false,
> +	},
> +	[VCNL4010] = {
> +		.prod = "VCNL4010/4020",
> +		.init = vcnl4000_init,
> +		.measure_light = vcnl4000_measure_light,
> +		.measure_proximity = vcnl4000_measure_proximity,
> +		.set_power_state = vcnl4000_set_power_state,
> +		.channels = vcnl4010_channels,
> +		.num_channels = ARRAY_SIZE(vcnl4010_channels),
> +		.info = &vcnl4010_info,
> +		.irq_support = true,
> +	},
> +	[VCNL4040] = {
> +		.prod = "VCNL4040",
> +		.init = vcnl4200_init,
> +		.measure_light = vcnl4200_measure_light,
> +		.measure_proximity = vcnl4200_measure_proximity,
> +		.set_power_state = vcnl4200_set_power_state,
> +		.channels = vcnl4000_channels,
> +		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> +		.info = &vcnl4000_info,
> +		.irq_support = false,
> +	},
> +	[VCNL4200] = {
> +		.prod = "VCNL4200",
> +		.init = vcnl4200_init,
> +		.measure_light = vcnl4200_measure_light,
> +		.measure_proximity = vcnl4200_measure_proximity,
> +		.set_power_state = vcnl4200_set_power_state,
> +		.channels = vcnl4000_channels,
> +		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> +		.info = &vcnl4000_info,
> +		.irq_support = false,
> +	},
> +};
> +
> +static irqreturn_t vcnl4010_irq_thread(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct vcnl4000_data *data = iio_priv(indio_dev);
> +	unsigned long isr;
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, VCNL4010_ISR);
> +	if (ret < 0)
> +		goto end;
> +
> +	isr = ret;
> +
> +	if (isr & VCNL4010_INT_THR) {
> +		if (test_bit(VCNL4010_INT_THR_LOW, &isr)) {
> +			iio_push_event(indio_dev,
> +				       IIO_UNMOD_EVENT_CODE(
> +					       IIO_PROXIMITY,
> +					       1,
> +					       IIO_EV_TYPE_THRESH,
> +					       IIO_EV_DIR_FALLING),
> +				       iio_get_time_ns(indio_dev));
> +		}
> +
> +		if (test_bit(VCNL4010_INT_THR_HIGH, &isr)) {
> +			iio_push_event(indio_dev,
> +				       IIO_UNMOD_EVENT_CODE(
> +					       IIO_PROXIMITY,
> +					       1,
> +					       IIO_EV_TYPE_THRESH,
> +					       IIO_EV_DIR_RISING),
> +				       iio_get_time_ns(indio_dev));
> +		}
> +
> +		i2c_smbus_write_byte_data(data->client, VCNL4010_ISR,
> +					  isr & VCNL4010_INT_THR);
> +	}
> +
> +end:
> +	return IRQ_HANDLED;
> +}
> +
>  static int vcnl4000_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
> @@ -486,12 +805,25 @@ static int vcnl4000_probe(struct i2c_client *client,
>  		data->near_level = 0;
>  
>  	indio_dev->dev.parent = &client->dev;
> -	indio_dev->info = &vcnl4000_info;
> -	indio_dev->channels = vcnl4000_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(vcnl4000_channels);
> +	indio_dev->info = data->chip_spec->info;
> +	indio_dev->channels = data->chip_spec->channels;
> +	indio_dev->num_channels = data->chip_spec->num_channels;
>  	indio_dev->name = VCNL4000_DRV_NAME;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> +	if (client->irq && data->chip_spec->irq_support) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, vcnl4010_irq_thread,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						"vcnl4010_irq",
> +						indio_dev);
> +		if (ret < 0) {
> +			dev_err(&client->dev, "irq request failed\n");
> +			return ret;
> +		}
> +	}
> +
>  	ret = pm_runtime_set_active(&client->dev);
>  	if (ret < 0)
>  		goto fail_poweroff;

[PATCH v4 3/4] iio: vcnl4000: Add sampling frequency support for VCNL4010/20.

[Mathieu Othacehe]

Add sampling frequency support for proximity data on VCNL4010 and VCNL4020
chips.

Signed-off-by: Mathieu Othacehe <m.othacehe@gmail.com>
---
 drivers/iio/light/vcnl4000.c | 112 ++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index db5c15541174..1e00a9666534 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -86,6 +86,19 @@
 #define VCNL4010_INT_DRDY \
 	(BIT(VCNL4010_INT_PROXIMITY) | BIT(VCNL4010_INT_ALS))
 
+#define VCNL4010_PROXIMITY_SAMPLING_FREQUENCY_AVAILABLE  \
+	"1.95 3.90 7.81 16.62 31.25 62.5 125 250"
+
+static const int vcnl4010_prox_sampling_frequency[][2] = {
+	{1, 950000},
+	{3, 906250},
+	{7, 812500},
+	{16, 625000},
+	{31, 250000},
+	{62, 500000},
+	{125, 0},
+	{250, 0}
+};
 
 #define VCNL4000_SLEEP_DELAY_MS	2000 /* before we enter pm_runtime_suspend */
 
@@ -366,6 +379,24 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
 	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
 }
 
+static int vcnl4010_read_proxy_samp_freq(struct vcnl4000_data *data, int *val,
+					 int *val2)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, VCNL4010_PROX_RATE);
+	if (ret < 0)
+		return ret;
+
+	if (ret >= ARRAY_SIZE(vcnl4010_prox_sampling_frequency))
+		return -EINVAL;
+
+	*val = vcnl4010_prox_sampling_frequency[ret][0];
+	*val2 = vcnl4010_prox_sampling_frequency[ret][1];
+
+	return 0;
+}
+
 static bool vcnl4010_in_periodic_mode(struct vcnl4000_data *data)
 {
 	int ret;
@@ -459,11 +490,75 @@ static int vcnl4010_read_raw(struct iio_dev *indio_dev,
 
 		iio_device_release_direct_mode(indio_dev);
 		return ret;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			ret = vcnl4010_read_proxy_samp_freq(data, val, val2);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
 }
 
+static int vcnl4010_write_proxy_samp_freq(struct vcnl4000_data *data, int val,
+					  int val2)
+{
+	int i;
+	const int len = ARRAY_SIZE(vcnl4010_prox_sampling_frequency);
+
+	for (i = 0; i < len; i++) {
+		if (val <= vcnl4010_prox_sampling_frequency[i][0])
+			break;
+	}
+
+	if (i == len)
+		return -EINVAL;
+
+	return i2c_smbus_write_byte_data(data->client, VCNL4010_PROX_RATE, i);
+}
+
+static int vcnl4010_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	int ret;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	/* Protect against event capture. */
+	if (vcnl4010_in_periodic_mode(data)) {
+		ret = -EBUSY;
+		goto end;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			ret = vcnl4010_write_proxy_samp_freq(data, val, val2);
+			goto end;
+		default:
+			ret = -EINVAL;
+			goto end;
+		}
+	default:
+		ret = -EINVAL;
+		goto end;
+	}
+
+end:
+	iio_device_release_direct_mode(indio_dev);
+	return ret;
+}
+
 static int vcnl4010_read_event(struct iio_dev *indio_dev,
 			       const struct iio_chan_spec *chan,
 			       enum iio_event_type type,
@@ -668,23 +763,38 @@ static const struct iio_chan_spec vcnl4010_channels[] = {
 	}, {
 		.type = IIO_PROXIMITY,
 		.scan_index = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.event_spec = vcnl4000_event_spec,
 		.num_event_specs = ARRAY_SIZE(vcnl4000_event_spec),
 		.ext_info = vcnl4000_ext_info,
 	},
 };
 
+static IIO_CONST_ATTR(in_proximity_sampling_frequency_available,
+		      VCNL4010_PROXIMITY_SAMPLING_FREQUENCY_AVAILABLE);
+
+static struct attribute *vcnl4010_attributes[] = {
+	&iio_const_attr_in_proximity_sampling_frequency_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group vcnl4010_attribute_group = {
+	.attrs = vcnl4010_attributes,
+};
+
 static const struct iio_info vcnl4000_info = {
 	.read_raw = vcnl4000_read_raw,
 };
 
 static const struct iio_info vcnl4010_info = {
 	.read_raw = vcnl4010_read_raw,
+	.write_raw = vcnl4010_write_raw,
 	.read_event_value = vcnl4010_read_event,
 	.write_event_value = vcnl4010_write_event,
 	.read_event_config = vcnl4010_read_event_config,
 	.write_event_config = vcnl4010_write_event_config,
+	.attrs    = &vcnl4010_attribute_group,
 };
 
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
-- 
2.26.0

Re: [PATCH v4 3/4] iio: vcnl4000: Add sampling frequency support for VCNL4010/20.

[Andy Shevchenko]

On Tue, Apr 21, 2020 at 10:56 AM Mathieu Othacehe <m.othacehe@gmail.com> wrote:
>
> Add sampling frequency support for proximity data on VCNL4010 and VCNL4020
> chips.

Couple of nitpicks below.

...

> +static const int vcnl4010_prox_sampling_frequency[][2] = {
> +       {1, 950000},
> +       {3, 906250},
> +       {7, 812500},
> +       {16, 625000},
> +       {31, 250000},
> +       {62, 500000},
> +       {125, 0},

> +       {250, 0}

Leave comma here, potentially helpful if it will be extended.

> +};

...

> +static int vcnl4010_write_proxy_samp_freq(struct vcnl4000_data *data, int val,
> +                                         int val2)
> +{
> +       int i;
> +       const int len = ARRAY_SIZE(vcnl4010_prox_sampling_frequency);
> +
> +       for (i = 0; i < len; i++) {
> +               if (val <= vcnl4010_prox_sampling_frequency[i][0])
> +                       break;
> +       }
> +
> +       if (i == len)
> +               return -EINVAL;

I would refactor this

  unsigned int i = ARRAY_SIZE(vcnl4010_prox_sampling_frequency);

  do {
    if (val > vcnl4010_prox_sampling_frequency[i][0])
      break;
  } while (--i);

You won't need to go full array to return error in this case.

> +       return i2c_smbus_write_byte_data(data->client, VCNL4010_PROX_RATE, i);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/4] iio: vcnl4000: Add sampling frequency support for VCNL4010/20.

[Jonathan Cameron]

On Tue, 21 Apr 2020 15:22:11 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 21, 2020 at 10:56 AM Mathieu Othacehe <m.othacehe@gmail.com> wrote:
> >
> > Add sampling frequency support for proximity data on VCNL4010 and VCNL4020
> > chips.  
> 
> Couple of nitpicks below.
> 
> ...
> 
> > +static const int vcnl4010_prox_sampling_frequency[][2] = {
> > +       {1, 950000},
> > +       {3, 906250},
> > +       {7, 812500},
> > +       {16, 625000},
> > +       {31, 250000},
> > +       {62, 500000},
> > +       {125, 0},  
> 
> > +       {250, 0}  
> 
> Leave comma here, potentially helpful if it will be extended.

Hi Andy,

Doesn't particularly matter either way, but given this is a list of the values
supported by the device, very unlikely it will be extended.

Games like trying to share the first part of a longer array between
multiple device types might occur, but those are usually really ugly.

Jonathan

> 
> > +};  
> 
> ...
> 
> > +static int vcnl4010_write_proxy_samp_freq(struct vcnl4000_data *data, int val,
> > +                                         int val2)
> > +{
> > +       int i;
> > +       const int len = ARRAY_SIZE(vcnl4010_prox_sampling_frequency);
> > +
> > +       for (i = 0; i < len; i++) {
> > +               if (val <= vcnl4010_prox_sampling_frequency[i][0])
> > +                       break;
> > +       }
> > +
> > +       if (i == len)
> > +               return -EINVAL;  
> 
> I would refactor this
> 
>   unsigned int i = ARRAY_SIZE(vcnl4010_prox_sampling_frequency);
> 
>   do {
>     if (val > vcnl4010_prox_sampling_frequency[i][0])
>       break;
>   } while (--i);
> 
> You won't need to go full array to return error in this case.
> 
> > +       return i2c_smbus_write_byte_data(data->client, VCNL4010_PROX_RATE, i);
> > +}  
> 

Re: [PATCH v4 3/4] iio: vcnl4000: Add sampling frequency support for VCNL4010/20.

[Andy Shevchenko]

On Sat, Apr 25, 2020 at 6:52 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 21 Apr 2020 15:22:11 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Apr 21, 2020 at 10:56 AM Mathieu Othacehe <m.othacehe@gmail.com> wrote:

...

> > > +static const int vcnl4010_prox_sampling_frequency[][2] = {
> > > +       {1, 950000},
> > > +       {3, 906250},
> > > +       {7, 812500},
> > > +       {16, 625000},
> > > +       {31, 250000},
> > > +       {62, 500000},
> > > +       {125, 0},
> >
> > > +       {250, 0}
> >
> > Leave comma here, potentially helpful if it will be extended.
>
> Hi Andy,
>
> Doesn't particularly matter either way, but given this is a list of the values
> supported by the device, very unlikely it will be extended.
>
> Games like trying to share the first part of a longer array between
> multiple device types might occur, but those are usually really ugly.

Good point. Though I would limit amount of entries by explicitly
writing down the array size.

> > > +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/4] iio: vcnl4000: Add sampling frequency support for VCNL4010/20.

[Jonathan Cameron]

On Tue, 21 Apr 2020 09:55:31 +0200
Mathieu Othacehe <m.othacehe@gmail.com> wrote:

> Add sampling frequency support for proximity data on VCNL4010 and VCNL4020
> chips.
> 
> Signed-off-by: Mathieu Othacehe <m.othacehe@gmail.com>
> ---
>  drivers/iio/light/vcnl4000.c | 112 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 111 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index db5c15541174..1e00a9666534 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -86,6 +86,19 @@
>  #define VCNL4010_INT_DRDY \
>  	(BIT(VCNL4010_INT_PROXIMITY) | BIT(VCNL4010_INT_ALS))
>  
> +#define VCNL4010_PROXIMITY_SAMPLING_FREQUENCY_AVAILABLE  \
> +	"1.95 3.90 7.81 16.62 31.25 62.5 125 250"
> +
> +static const int vcnl4010_prox_sampling_frequency[][2] = {
> +	{1, 950000},
> +	{3, 906250},
> +	{7, 812500},
> +	{16, 625000},
> +	{31, 250000},
> +	{62, 500000},
> +	{125, 0},
> +	{250, 0}
> +};
see below for observation on using read_avail instead of the string above..

>  
>  #define VCNL4000_SLEEP_DELAY_MS	2000 /* before we enter pm_runtime_suspend */
>  
> @@ -366,6 +379,24 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
>  	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
>  }
>  
> +static int vcnl4010_read_proxy_samp_freq(struct vcnl4000_data *data, int *val,
> +					 int *val2)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, VCNL4010_PROX_RATE);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret >= ARRAY_SIZE(vcnl4010_prox_sampling_frequency))
> +		return -EINVAL;
> +
> +	*val = vcnl4010_prox_sampling_frequency[ret][0];
> +	*val2 = vcnl4010_prox_sampling_frequency[ret][1];
> +
> +	return 0;
> +}
> +
>  static bool vcnl4010_in_periodic_mode(struct vcnl4000_data *data)
>  {
>  	int ret;
> @@ -459,11 +490,75 @@ static int vcnl4010_read_raw(struct iio_dev *indio_dev,
>  
>  		iio_device_release_direct_mode(indio_dev);
>  		return ret;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			ret = vcnl4010_read_proxy_samp_freq(data, val, val2);
> +			if (ret < 0)
> +				return ret;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> +static int vcnl4010_write_proxy_samp_freq(struct vcnl4000_data *data, int val,
> +					  int val2)
> +{
> +	int i;
> +	const int len = ARRAY_SIZE(vcnl4010_prox_sampling_frequency);
> +
> +	for (i = 0; i < len; i++) {
> +		if (val <= vcnl4010_prox_sampling_frequency[i][0])
> +			break;
> +	}
> +
> +	if (i == len)
> +		return -EINVAL;
> +
> +	return i2c_smbus_write_byte_data(data->client, VCNL4010_PROX_RATE, i);
> +}
> +
> +static int vcnl4010_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	int ret;
> +	struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Protect against event capture. */
> +	if (vcnl4010_in_periodic_mode(data)) {
> +		ret = -EBUSY;
> +		goto end;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			ret = vcnl4010_write_proxy_samp_freq(data, val, val2);
> +			goto end;
> +		default:
> +			ret = -EINVAL;
> +			goto end;
> +		}
> +	default:
> +		ret = -EINVAL;
> +		goto end;
> +	}
> +
> +end:
> +	iio_device_release_direct_mode(indio_dev);
> +	return ret;
> +}
> +
>  static int vcnl4010_read_event(struct iio_dev *indio_dev,
>  			       const struct iio_chan_spec *chan,
>  			       enum iio_event_type type,
> @@ -668,23 +763,38 @@ static const struct iio_chan_spec vcnl4010_channels[] = {
>  	}, {
>  		.type = IIO_PROXIMITY,
>  		.scan_index = 0,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  		.event_spec = vcnl4000_event_spec,
>  		.num_event_specs = ARRAY_SIZE(vcnl4000_event_spec),
>  		.ext_info = vcnl4000_ext_info,
>  	},
>  };
>  
> +static IIO_CONST_ATTR(in_proximity_sampling_frequency_available,
> +		      VCNL4010_PROXIMITY_SAMPLING_FREQUENCY_AVAILABLE);
Its a bit late in the game, but I've just noticed you have an appropriate
array to do this directly using the read_avail callbacks and avoid
having the values all present twice in the driver so as to present this
string.

If you'd rather leave it alone for now then fair enough.  I'm not 'yet'
insisting people use the read_avail method.

> +
> +static struct attribute *vcnl4010_attributes[] = {
> +	&iio_const_attr_in_proximity_sampling_frequency_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group vcnl4010_attribute_group = {
> +	.attrs = vcnl4010_attributes,
> +};
> +
>  static const struct iio_info vcnl4000_info = {
>  	.read_raw = vcnl4000_read_raw,
>  };
>  
>  static const struct iio_info vcnl4010_info = {
>  	.read_raw = vcnl4010_read_raw,
> +	.write_raw = vcnl4010_write_raw,
>  	.read_event_value = vcnl4010_read_event,
>  	.write_event_value = vcnl4010_write_event,
>  	.read_event_config = vcnl4010_read_event_config,
>  	.write_event_config = vcnl4010_write_event_config,
> +	.attrs    = &vcnl4010_attribute_group,
>  };
>  
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {

[PATCH v4 4/4] iio: vcnl4000: Add buffer support for VCNL4010/20.

[Mathieu Othacehe]

The VCNL4010 and VCNL4020 chips are able to raise interrupts on data ready.
Use it to provide triggered buffer support for proximity data.

Those two chips also provide ambient light data. However, they are sampled
at different rate than proximity data. As this is not handled by the IIO
framework for now, and the sample frequencies of ambient light data are
very low, do add buffer support for them.

Signed-off-by: Mathieu Othacehe <m.othacehe@gmail.com>
---
 drivers/iio/light/Kconfig    |   2 +
 drivers/iio/light/vcnl4000.c | 156 ++++++++++++++++++++++++++++++++++-
 2 files changed, 156 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 74970f18a93b..05f61b1e223a 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -506,6 +506,8 @@ config US5182D
 
 config VCNL4000
 	tristate "VCNL4000/4010/4020/4200 combined ALS and proximity sensor"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	depends on I2C
 	help
 	  Say Y here if you want to build a driver for the Vishay VCNL4000,
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 1e00a9666534..094ba931d460 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -5,6 +5,7 @@
  *
  * Copyright 2012 Peter Meerwald <pmeerw@pmeerw.net>
  * Copyright 2019 Pursim SPC
+ * Copyright 2020 Mathieu Othacehe <m.othacehe@gmail.com>
  *
  * IIO driver for:
  *   VCNL4000/10/20 (7-bit I2C slave address 0x13)
@@ -13,8 +14,7 @@
  *
  * TODO:
  *   allow to adjust IR current
- *   periodic ALS/proximity measurement (VCNL4010/20)
- *   interrupts (VCNL4010/20/40, VCNL4200)
+ *   interrupts (VCNL4040, VCNL4200)
  */
 
 #include <linux/module.h>
@@ -24,9 +24,13 @@
 #include <linux/pm_runtime.h>
 #include <linux/interrupt.h>
 
+#include <linux/iio/buffer.h>
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 #define VCNL4000_DRV_NAME "vcnl4000"
 #define VCNL4000_PROD_ID	0x01
@@ -768,7 +772,14 @@ static const struct iio_chan_spec vcnl4010_channels[] = {
 		.event_spec = vcnl4000_event_spec,
 		.num_event_specs = ARRAY_SIZE(vcnl4000_event_spec),
 		.ext_info = vcnl4000_ext_info,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
 };
 
 static IIO_CONST_ATTR(in_proximity_sampling_frequency_available,
@@ -882,10 +893,136 @@ static irqreturn_t vcnl4010_irq_thread(int irq, void *p)
 					  isr & VCNL4010_INT_THR);
 	}
 
+	if (isr & VCNL4010_INT_DRDY && iio_buffer_enabled(indio_dev))
+		iio_trigger_poll_chained(indio_dev->trig);
+
+end:
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t vcnl4010_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+	const unsigned long *active_scan_mask = indio_dev->active_scan_mask;
+	u16 buffer[8] = {0}; /* 1x16-bit + ts */
+	bool data_read = false;
+	unsigned long isr;
+	int val = 0;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, VCNL4010_ISR);
+	if (ret < 0)
+		goto end;
+
+	isr = ret;
+
+	if (test_bit(0, active_scan_mask)) {
+		if (test_bit(VCNL4010_INT_PROXIMITY, &isr)) {
+			ret = vcnl4000_read_data(data,
+						 VCNL4000_PS_RESULT_HI,
+						 &val);
+			if (ret < 0)
+				goto end;
+
+			buffer[0] = val;
+			data_read = true;
+		}
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4010_ISR,
+					isr & VCNL4010_INT_DRDY);
+	if (ret < 0 || !data_read)
+		goto end;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
+					   iio_get_time_ns(indio_dev));
+
 end:
+	iio_trigger_notify_done(indio_dev->trig);
 	return IRQ_HANDLED;
 }
 
+static int vcnl4010_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+	int ret;
+	int cmd;
+
+	ret = iio_triggered_buffer_postenable(indio_dev);
+	if (ret)
+		return ret;
+
+	/* Do not enable the buffer if we are already capturing events. */
+	if (vcnl4010_in_periodic_mode(data)) {
+		ret = -EBUSY;
+		goto end;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL,
+					VCNL4010_INT_PROX_EN);
+	if (ret < 0)
+		goto end;
+
+	cmd = VCNL4000_SELF_TIMED_EN | VCNL4000_PROX_EN;
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, cmd);
+	if (ret < 0)
+		goto end;
+
+end:
+	if (ret < 0)
+		iio_triggered_buffer_predisable(indio_dev);
+
+	return ret;
+}
+
+static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+	int ret, ret_disable;
+
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL, 0);
+	if (ret < 0)
+		goto end;
+
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
+
+end:
+	ret_disable = iio_triggered_buffer_predisable(indio_dev);
+	if (ret == 0)
+		ret = ret_disable;
+
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops vcnl4010_buffer_ops = {
+	.postenable = &vcnl4010_buffer_postenable,
+	.predisable = &vcnl4010_buffer_predisable,
+};
+
+static const struct iio_trigger_ops vcnl4010_trigger_ops = {
+	.validate_device = iio_trigger_validate_own_device,
+};
+
+static int vcnl4010_probe_trigger(struct iio_dev *indio_dev)
+{
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	struct iio_trigger *trigger;
+
+	trigger = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
+					 indio_dev->name, indio_dev->id);
+	if (!trigger)
+		return -ENOMEM;
+
+	trigger->dev.parent = &client->dev;
+	trigger->ops = &vcnl4010_trigger_ops;
+	iio_trigger_set_drvdata(trigger, indio_dev);
+
+	return devm_iio_trigger_register(&client->dev, trigger);
+}
+
 static int vcnl4000_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
@@ -922,6 +1059,16 @@ static int vcnl4000_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	if (client->irq && data->chip_spec->irq_support) {
+		ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
+						      NULL,
+						      vcnl4010_trigger_handler,
+						      &vcnl4010_buffer_ops);
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"unable to setup iio triggered buffer\n");
+			return ret;
+		}
+
 		ret = devm_request_threaded_irq(&client->dev, client->irq,
 						NULL, vcnl4010_irq_thread,
 						IRQF_TRIGGER_FALLING |
@@ -932,6 +1079,10 @@ static int vcnl4000_probe(struct i2c_client *client,
 			dev_err(&client->dev, "irq request failed\n");
 			return ret;
 		}
+
+		ret = vcnl4010_probe_trigger(indio_dev);
+		if (ret < 0)
+			return ret;
 	}
 
 	ret = pm_runtime_set_active(&client->dev);
@@ -1027,5 +1178,6 @@ static struct i2c_driver vcnl4000_driver = {
 module_i2c_driver(vcnl4000_driver);
 
 MODULE_AUTHOR("Peter Meerwald <pmeerw@pmeerw.net>");
+MODULE_AUTHOR("Mathieu Othacehe <m.othacehe@gmail.com>");
 MODULE_DESCRIPTION("Vishay VCNL4000 proximity/ambient light sensor driver");
 MODULE_LICENSE("GPL");
-- 
2.26.0

Re: [PATCH v4 4/4] iio: vcnl4000: Add buffer support for VCNL4010/20.

[Andy Shevchenko]

On Tue, Apr 21, 2020 at 10:59 AM Mathieu Othacehe <m.othacehe@gmail.com> wrote:
>
> The VCNL4010 and VCNL4020 chips are able to raise interrupts on data ready.
> Use it to provide triggered buffer support for proximity data.
>
> Those two chips also provide ambient light data. However, they are sampled
> at different rate than proximity data. As this is not handled by the IIO
> framework for now, and the sample frequencies of ambient light data are
> very low, do add buffer support for them.

...

> +static irqreturn_t vcnl4010_trigger_handler(int irq, void *p)
> +{
> +       struct iio_poll_func *pf = p;
> +       struct iio_dev *indio_dev = pf->indio_dev;
> +       struct vcnl4000_data *data = iio_priv(indio_dev);
> +       const unsigned long *active_scan_mask = indio_dev->active_scan_mask;
> +       u16 buffer[8] = {0}; /* 1x16-bit + ts */
> +       bool data_read = false;
> +       unsigned long isr;
> +       int val = 0;
> +       int ret;
> +
> +       ret = i2c_smbus_read_byte_data(data->client, VCNL4010_ISR);
> +       if (ret < 0)
> +               goto end;
> +
> +       isr = ret;
> +
> +       if (test_bit(0, active_scan_mask)) {
> +               if (test_bit(VCNL4010_INT_PROXIMITY, &isr)) {
> +                       ret = vcnl4000_read_data(data,
> +                                                VCNL4000_PS_RESULT_HI,
> +                                                &val);
> +                       if (ret < 0)
> +                               goto end;
> +
> +                       buffer[0] = val;
> +                       data_read = true;
> +               }
> +       }
> +
> +       ret = i2c_smbus_write_byte_data(data->client, VCNL4010_ISR,
> +                                       isr & VCNL4010_INT_DRDY);

> +       if (ret < 0 || !data_read)

I would split them, because they are logically different checks.

> +               goto end;
> +
> +       iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> +                                          iio_get_time_ns(indio_dev));
> +
>  end:
> +       iio_trigger_notify_done(indio_dev->trig);
>         return IRQ_HANDLED;
>  }

...

> +static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +       struct vcnl4000_data *data = iio_priv(indio_dev);
> +       int ret, ret_disable;
> +
> +       ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL, 0);
> +       if (ret < 0)
> +               goto end;
> +
> +       ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
> +
> +end:

> +       ret_disable = iio_triggered_buffer_predisable(indio_dev);
> +       if (ret == 0)
> +               ret = ret_disable;

What is this?

Can't you rather call IIO API first, and then try to handle the rest?

> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/4] iio: vcnl4000: Add buffer support for VCNL4010/20.

[Mathieu Othacehe]

>> +static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
>> +{
>> +       struct vcnl4000_data *data = iio_priv(indio_dev);
>> +       int ret, ret_disable;
>> +
>> +       ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL, 0);
>> +       if (ret < 0)
>> +               goto end;
>> +
>> +       ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
>> +
>> +end:
>
>> +       ret_disable = iio_triggered_buffer_predisable(indio_dev);
>> +       if (ret == 0)
>> +               ret = ret_disable;
>
> What is this?
>
> Can't you rather call IIO API first, and then try to handle the rest?

Well, iio_triggered_buffer_predisable will call free_irq which requires
that the interruption source is disabled, hence this strange pattern.

However, this may be some misunderstanding from me, but I noticed
something strange here. In a configuration with one CPU and
CONFIG_PREEMPT disabled, I have kernel lockups when disabling the
buffer.

This is because free_irq calls synchronize_irq that will wait for any
IRQ handler to be over. If the kthread handling the interruption is
still running, it has no chances to terminate, and synchronize_irq waits
forever. So maybe I'm missing something.

Anyway, I'll send a v5 addressing your remarks.

Thanks,

Mathieu

Re: [PATCH v4 4/4] iio: vcnl4000: Add buffer support for VCNL4010/20.

[Jonathan Cameron]

On Wed, 22 Apr 2020 10:14:21 +0200
Mathieu Othacehe <m.othacehe@gmail.com> wrote:

> >> +static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
> >> +{
> >> +       struct vcnl4000_data *data = iio_priv(indio_dev);
> >> +       int ret, ret_disable;
> >> +
> >> +       ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL, 0);
> >> +       if (ret < 0)
> >> +               goto end;
> >> +
> >> +       ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
> >> +
> >> +end:  
> >  
> >> +       ret_disable = iio_triggered_buffer_predisable(indio_dev);
> >> +       if (ret == 0)
> >> +               ret = ret_disable;  
> >
> > What is this?
> >
> > Can't you rather call IIO API first, and then try to handle the rest?  
> 
> Well, iio_triggered_buffer_predisable will call free_irq which requires
> that the interruption source is disabled, hence this strange pattern.
> 
> However, this may be some misunderstanding from me, but I noticed
> something strange here. In a configuration with one CPU and
> CONFIG_PREEMPT disabled, I have kernel lockups when disabling the
> buffer.
> 
> This is because free_irq calls synchronize_irq that will wait for any
> IRQ handler to be over. If the kthread handling the interruption is
> still running, it has no chances to terminate, and synchronize_irq waits
> forever. So maybe I'm missing something.

That is indeed worrying.  The synchronize_irq is documented as
sleeping if we have a threaded interrupt.  From a quick look I'd have
expected the wait in there to result in the interrupt thread being able
to complete whether or not we had preemption enabled.

I wonder what I'm missing...

Jonathan


> 
> Anyway, I'll send a v5 addressing your remarks.
> 
> Thanks,
> 
> Mathieu

Re: [PATCH v4 4/4] iio: vcnl4000: Add buffer support for VCNL4010/20.

[Jonathan Cameron]

On Tue, 21 Apr 2020 15:27:14 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 21, 2020 at 10:59 AM Mathieu Othacehe <m.othacehe@gmail.com> wrote:
> >
> > The VCNL4010 and VCNL4020 chips are able to raise interrupts on data ready.
> > Use it to provide triggered buffer support for proximity data.
> >
> > Those two chips also provide ambient light data. However, they are sampled
> > at different rate than proximity data. As this is not handled by the IIO
> > framework for now, and the sample frequencies of ambient light data are
> > very low, do add buffer support for them.  
> 
> ...
> 
> > +static irqreturn_t vcnl4010_trigger_handler(int irq, void *p)
> > +{
> > +       struct iio_poll_func *pf = p;
> > +       struct iio_dev *indio_dev = pf->indio_dev;
> > +       struct vcnl4000_data *data = iio_priv(indio_dev);
> > +       const unsigned long *active_scan_mask = indio_dev->active_scan_mask;
> > +       u16 buffer[8] = {0}; /* 1x16-bit + ts */
> > +       bool data_read = false;
> > +       unsigned long isr;
> > +       int val = 0;
> > +       int ret;
> > +
> > +       ret = i2c_smbus_read_byte_data(data->client, VCNL4010_ISR);
> > +       if (ret < 0)
> > +               goto end;
> > +
> > +       isr = ret;
> > +
> > +       if (test_bit(0, active_scan_mask)) {
> > +               if (test_bit(VCNL4010_INT_PROXIMITY, &isr)) {
> > +                       ret = vcnl4000_read_data(data,
> > +                                                VCNL4000_PS_RESULT_HI,
> > +                                                &val);
> > +                       if (ret < 0)
> > +                               goto end;
> > +
> > +                       buffer[0] = val;
> > +                       data_read = true;
> > +               }
> > +       }
> > +
> > +       ret = i2c_smbus_write_byte_data(data->client, VCNL4010_ISR,
> > +                                       isr & VCNL4010_INT_DRDY);  
> 
> > +       if (ret < 0 || !data_read)  
> 
> I would split them, because they are logically different checks.
> 
> > +               goto end;
> > +
> > +       iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> > +                                          iio_get_time_ns(indio_dev));
> > +
> >  end:
> > +       iio_trigger_notify_done(indio_dev->trig);
> >         return IRQ_HANDLED;
> >  }  
> 
> ...
> 
> > +static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +       struct vcnl4000_data *data = iio_priv(indio_dev);
> > +       int ret, ret_disable;
> > +
> > +       ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL, 0);
> > +       if (ret < 0)
> > +               goto end;
> > +
> > +       ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
> > +
> > +end:  
> 
> > +       ret_disable = iio_triggered_buffer_predisable(indio_dev);
> > +       if (ret == 0)
> > +               ret = ret_disable;  
> 
> What is this?
> 
> Can't you rather call IIO API first, and then try to handle the rest?

There is an additional complexity here. Alex is in the middle of trying
to refactor all drivers to handle this in the same order so as to
ultimately remote the need to explicitly make that call at all.

So until that is sorted, I think that needs to be the last call made
even if there isn't a driver related reason for the ordering.

It's a big job with lots of complex corner cases so will be a little
while yet I guess before we can do the core rework.

Jonathan

> 
> > +       return ret;
> > +}  
>