[PATCH 1/3] iio: light: Add driver for ap3216c

[PATCH 1/3] iio: light: Add driver for ap3216c

[Robert Eshleman]

This patch adds support for the ap3216c ambient light and proximity
sensor.

Supported features include:

* Illuminance (lux)
* Proximity (raw)
* IR (raw)
* Rising/falling threshold events for illuminance and proximity
* Calibration scale for illuminance
* Calibration bias for proximity

Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
---
 drivers/iio/light/Kconfig   |  11 +
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/ap3216c.c | 793 ++++++++++++++++++++++++++++++++++++
 3 files changed, 805 insertions(+)
 create mode 100644 drivers/iio/light/ap3216c.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 36f458433480..74688d19beb1 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -41,6 +41,17 @@ config AL3320A
 	 To compile this driver as a module, choose M here: the
 	 module will be called al3320a.
 
+config AP3216C
+	tristate "AP3216C Ambient Light and Proximity sensor"
+	select REGMAP_I2C
+	depends on I2C
+	help
+	  Say Y here to build a driver for the AP3216C Ambient Light and
+	  Proximity sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ap3216c.
+
 config APDS9300
 	tristate "APDS9300 ambient light sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 286bf3975372..7d2f8fa0f30d 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -7,6 +7,7 @@
 obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
 obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
 obj-$(CONFIG_AL3320A)		+= al3320a.o
+obj-$(CONFIG_AP3216C)		+= ap3216c.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o
 obj-$(CONFIG_BH1750)		+= bh1750.o
diff --git a/drivers/iio/light/ap3216c.c b/drivers/iio/light/ap3216c.c
new file mode 100644
index 000000000000..12802b45c01c
--- /dev/null
+++ b/drivers/iio/light/ap3216c.c
@@ -0,0 +1,793 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AP3216C Ambient Light and Infrared Proximity Sensor
+ *
+ * Copyright (c) 2019, Robert Eshleman.
+ *
+ * Datasheet: https://pdf-datasheet-datasheet.netdna-ssl.com/pdf-down/A/P/3/AP3216C-LITE-ON.pdf
+ *
+ * 7-bit I2C slave address 0x1E
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/bits.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+#define AP3216C_DRV_NAME "ap3216c"
+
+/* register addresses */
+#define AP3216C_SYS 0x0
+#define AP3216C_INT_STATUS 0x01
+#define AP3216C_INT_CLR 0x02
+#define AP3216C_IR_DATA_LO 0x0A
+#define AP3216C_IR_DATA_HI 0x0B
+#define AP3216C_ALS_DATA_LO 0x0C
+#define AP3216C_ALS_DATA_HI 0x0D
+#define AP3216C_PS_DATA_LO 0x0E
+#define AP3216C_PS_DATA_HI 0x0F
+#define AP3216C_ALS_CFG 0x10
+#define AP3216C_ALS_CALIB 0x19
+#define AP3216C_ALS_LO_THR_LO 0x1A
+#define AP3216C_ALS_LO_THR_HI 0x1B
+#define AP3216C_ALS_HI_THR_LO 0x1C
+#define AP3216C_ALS_HI_THR_HI 0x1D
+#define AP3216C_PS_CFG 0x20
+#define AP3216C_PS_CALIB_LO 0x28
+#define AP3216C_PS_CALIB_HI 0x29
+#define AP3216C_PS_LO_THR_LO 0x2A
+#define AP3216C_PS_LO_THR_HI 0x2B
+#define AP3216C_PS_HI_THR_LO 0x2C
+#define AP3216C_PS_HI_THR_HI 0x2D
+
+/* SYS_MODE mask and config value */
+#define AP3216C_SYS_MODE_ALS_PS GENMASK(1, 0)
+#define AP3216C_SYS_MODE_ALS_ONLY 0x01
+
+/* INT_STATUS masks and config value */
+#define AP3216C_INT_STATUS_ALS_MASK 1
+#define AP3216C_INT_STATUS_PS_MASK (1 << 1)
+#define AP3216C_INT_STATUS_CLR GENMASK(1, 0)
+#define AP3216C_INT_CLR_MANUAL 1
+
+/* IR_DATA mask/shift */
+#define AP3216C_IR_DATA_LO_MASK GENMASK(1, 0)
+#define AP3216C_IR_DATA_HI_SHIFT 2
+
+/* ALS_DATA shift and fractional helper */
+#define AP3216C_ALS_DATA_HI_SHIFT 8
+#define AP3216C_ALS_DATA_DENOM 100000
+
+/* ALS_CALIB masks/shifts */
+#define AP3216C_ALS_CALIB_INT_MASK GENMASK(7, 6)
+#define AP3216C_ALS_CALIB_INT_SHIFT 6
+#define AP3216C_ALS_CALIB_DEC_MASK GENMASK(5, 0)
+
+/* PS_DATA shifts/masks/bits */
+#define AP3216C_PS_DATA_LO_MASK GENMASK(3, 0)
+#define AP3216C_PS_DATA_LO_IR_OF BIT(6)
+#define AP3216C_PS_DATA_HI_MASK GENMASK(5, 0)
+#define AP3216C_PS_DATA_HI_SHIFT 4
+#define AP3216C_PS_DATA_HI_IR_OF BIT(6)
+
+/* ALS_CFG masks */
+#define AP3216C_ALS_CFG_GAIN_MASK GENMASK(5, 4)
+
+/* ALS_CALIB shifts */
+#define AP3216C_ALS_CALIB_INT_SHIFT 6
+
+/* ALS_HI_THR masks and shifts */
+#define AP3216C_ALS_HI_THR_LO_MASK GENMASK(7, 0)
+#define AP3216C_ALS_HI_THR_HI_SHIFT 8
+#define AP3216C_ALS_HI_THR_HI_MASK GENMASK(7, 4)
+#define AP3216C_ALS_HI_THR_HI_SHIFT 8
+
+/* ALS_LO_THR masks and shifts */
+#define AP3216C_ALS_LO_THR_LO_MASK GENMASK(3, 0)
+#define AP3216C_ALS_LO_THR_HI_MASK GENMASK(7, 4)
+#define AP3216C_ALS_LO_THR_HI_SHIFT 8
+
+/* PS_CFG reg mask/shift/bit values */
+#define AP3216C_PS_CFG_GAIN_MASK GENMASK(3, 2)
+#define AP3216C_PS_CFG_GAIN_SHIFT 2
+#define AP3216C_PS_CFG_GAIN(val) \
+	(1 << ((val & AP3216C_PS_CFG_GAIN_MASK) >> AP3216C_PS_CFG_GAIN_SHIFT))
+
+/* PS_LO and PS_HI shift/mask and multiplier values */
+#define AP3216C_PS_LO_THR_HI_MULT 4
+#define AP3216C_PS_HI_THR_HI_SHIFT 2
+#define AP3216C_PS_HI_THR_HI_MULT 4
+
+/* PS_THR_LO and PS_THR_HI masks */
+#define AP3216C_PS_THR_LO_MASK GENMASK(1, 0)
+#define AP3216C_PS_THR_HI_MASK GENMASK(10, 2)
+
+/* PS_CALIB_HI and PS_CALIB_LO shift/mask/bit values */
+#define AP3216C_PS_CALIB_HI_SHIFT 1
+#define AP3216C_PS_CALIB_HI_MASK GENMASK(8, 1)
+#define AP3216C_PS_CALIB_LO_MASK BIT(0)
+
+/* ALS fractional helper values */
+#define AP3216C_ALS_INTEG_MULT 10000
+#define AP3216C_ALS_FRACT_DIV 100
+
+/* ALS_CALIB min/max values (0.0 <= ALS_CALIB <= 3.984375) */
+#define AP3216C_ALS_CALIB_INT_MIN 0
+#define AP3216C_ALS_CALIB_DEC_MIN 0
+#define AP3216C_ALS_CALIB_INT_MAX 3
+#define AP3216C_ALS_CALIB_DEC_MAX 984375
+
+/* ALS_CALIB conversion denominator */
+#define AP3216C_ALS_CALIB_DENOM 64
+
+#define AP3216C_IIO_MULT 1000000
+
+static u16 ap3216c_gain_array[] = {
+	3500,  /* 0.3500 lux resolution */
+	 788,  /* 0.0788 lux resolution */
+	 197,  /* 0.0197 lux resolution */
+	  49,  /* 0.0049 lux resolution */
+};
+
+struct ap3216c_data {
+	struct regmap *regmap;
+	struct i2c_client *client;
+	bool als_thresh_en;
+	bool prox_thresh_en;
+};
+
+static const struct iio_event_spec ap3216c_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_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static const struct iio_chan_spec ap3216c_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_CALIBSCALE),
+		.event_spec = ap3216c_event_spec,
+		.num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
+	},
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_CALIBBIAS),
+		.event_spec = ap3216c_event_spec,
+		.num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
+	},
+	{
+		.type = IIO_LIGHT,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_IR,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}
+};
+
+static int ap3216c_write_event_value(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan,
+				       enum iio_event_type type,
+				       enum iio_event_direction dir,
+				       enum iio_event_info info,
+				       int val, int val2)
+{
+	struct ap3216c_data *data = iio_priv(indio_dev);
+	int cfg, gain;
+	int integral, fractional;
+	int lo, hi;
+	u16 thr;
+	int ret;
+
+	switch (chan->type) {
+	case IIO_LIGHT:
+		ret = regmap_read(data->regmap, AP3216C_ALS_CFG, &cfg);
+		if (ret < 0)
+			return ret;
+
+		gain = ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
+		integral = val * AP3216C_ALS_INTEG_MULT / gain;
+		fractional = val2 / AP3216C_ALS_FRACT_DIV / gain;
+
+		thr = integral + fractional;
+
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_write(data->regmap, AP3216C_ALS_HI_THR_LO,
+					   thr & AP3216C_ALS_HI_THR_LO_MASK);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_write(data->regmap,
+					   AP3216C_ALS_HI_THR_HI,
+					   (thr & AP3216C_ALS_HI_THR_HI_MASK) >>
+					   AP3216C_ALS_HI_THR_HI_SHIFT);
+			if (ret < 0)
+				return ret;
+
+			return ret;
+
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_write(data->regmap,
+					   AP3216C_ALS_LO_THR_LO,
+					   thr & AP3216C_ALS_LO_THR_LO_MASK);
+
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_write(data->regmap,
+					   AP3216C_ALS_LO_THR_HI,
+					   (thr & AP3216C_ALS_LO_THR_HI_MASK) >>
+					   AP3216C_ALS_LO_THR_HI_SHIFT);
+			if (ret < 0)
+				return ret;
+
+			return ret;
+
+		default:
+			return -EINVAL;
+		}
+		return -EINVAL;
+
+	case IIO_PROXIMITY:
+		lo = val & AP3216C_PS_THR_LO_MASK;
+		hi = (val & AP3216C_PS_THR_HI_MASK) >>
+			AP3216C_PS_HI_THR_HI_SHIFT;
+
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_write(data->regmap,
+					   AP3216C_PS_HI_THR_LO, lo);
+			if (ret < 0)
+				return ret;
+
+			return regmap_write(data->regmap,
+					    AP3216C_PS_HI_THR_HI, hi);
+
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_write(data->regmap,
+					   AP3216C_PS_LO_THR_LO, lo);
+			if (ret < 0)
+				return ret;
+
+			return regmap_write(data->regmap,
+					    AP3216C_PS_LO_THR_HI, hi);
+
+		default:
+			return -EINVAL;
+		}
+
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
+static int ap3216c_read_event_value(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     enum iio_event_info info,
+				     int *val, int *val2)
+{
+	struct ap3216c_data *data = iio_priv(indio_dev);
+	unsigned int lo, hi;
+	int ret;
+
+	switch (chan->type) {
+	case IIO_LIGHT:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_read(data->regmap,
+					  AP3216C_ALS_HI_THR_LO, &lo);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_ALS_HI_THR_HI, &hi);
+			if (ret < 0)
+				return ret;
+
+			*val = (hi << AP3216C_ALS_HI_THR_HI_SHIFT) | lo;
+			return IIO_VAL_INT;
+
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_read(data->regmap,
+					  AP3216C_ALS_LO_THR_LO, &lo);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_ALS_LO_THR_HI, &hi);
+			if (ret < 0)
+				return ret;
+
+			*val = (hi << AP3216C_ALS_LO_THR_HI_SHIFT) | lo;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_PROXIMITY:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_read(data->regmap,
+					  AP3216C_PS_HI_THR_LO, &lo);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_PS_HI_THR_HI, &hi);
+			if (ret < 0)
+				return ret;
+
+			*val = (hi * AP3216C_PS_HI_THR_HI_MULT) +
+				(lo & AP3216C_PS_THR_LO_MASK);
+
+			return IIO_VAL_INT;
+
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_read(data->regmap,
+					  AP3216C_PS_LO_THR_LO, &lo);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_PS_LO_THR_HI, &hi);
+			if (ret < 0)
+				return ret;
+
+			*val = (hi * AP3216C_PS_LO_THR_HI_MULT) +
+				(lo & AP3216C_PS_THR_LO_MASK);
+
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
+
+	default:
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static int ap3216c_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 ap3216c_data *data = iio_priv(indio_dev);
+
+	switch (chan->type) {
+	case IIO_LIGHT:
+		return data->als_thresh_en;
+
+	case IIO_PROXIMITY:
+		return data->prox_thresh_en;
+
+	default:
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static int ap3216c_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir, int state)
+{
+	struct ap3216c_data *data = iio_priv(indio_dev);
+
+	switch (chan->type) {
+	case IIO_LIGHT:
+		data->als_thresh_en = state;
+		return 0;
+
+	case IIO_PROXIMITY:
+		data->prox_thresh_en = state;
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static const struct regmap_config ap3216c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AP3216C_PS_HI_THR_HI,
+};
+
+/**
+ * Returns integral part of decimal between 0.0 and 3.984275
+ */
+static int ap3216c_als_calibscale_int(int val)
+{
+	if (val > AP3216C_ALS_CALIB_INT_MAX)
+		return AP3216C_ALS_CALIB_INT_MAX;
+
+	if (val <= AP3216C_ALS_CALIB_INT_MIN)
+		return AP3216C_ALS_CALIB_INT_MIN;
+
+	return val;
+}
+
+/**
+ * Returns decimal part of decimal number between 0.0 and 3.984275
+ */
+static int ap3216c_als_calibscale_dec(int val, int val2)
+{
+	/* Return max decimal if number exceeds calibscale max */
+	if (val > AP3216C_ALS_CALIB_INT_MAX ||
+	    (val == AP3216C_ALS_CALIB_INT_MAX &&
+	     val2 > AP3216C_ALS_CALIB_DEC_MAX))
+		return AP3216C_ALS_CALIB_DEC_MAX;
+
+	/* Floor the decimal if integral below minimum */
+	if (val <= AP3216C_ALS_CALIB_INT_MIN)
+		return AP3216C_ALS_CALIB_DEC_MIN;
+
+	/* Floor the decimal if decimal below minimum */
+	if (val2 < AP3216C_ALS_CALIB_DEC_MIN)
+		return AP3216C_ALS_CALIB_DEC_MIN;
+
+	/* Return max decimal if decimal above maximum */
+	if (val2 > AP3216C_ALS_CALIB_DEC_MAX)
+		return AP3216C_ALS_CALIB_DEC_MAX;
+
+	return val2;
+}
+
+static int ap3216c_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct ap3216c_data *data = iio_priv(indio_dev);
+	int integral, decimal;
+	int tmp;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			integral = ap3216c_als_calibscale_int(val);
+			decimal = ap3216c_als_calibscale_dec(val, val2);
+
+			tmp = integral << AP3216C_ALS_CALIB_INT_SHIFT;
+
+			/*
+			 * Reverse scaling by multiplying by the scaling
+			 * denominator and dividing by IIO multiplier.
+			 */
+			tmp |= (decimal * AP3216C_ALS_CALIB_DENOM) /
+				AP3216C_IIO_MULT;
+
+			return regmap_write(data->regmap,
+					    AP3216C_ALS_CALIB, tmp);
+
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_CALIBBIAS:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			ret = regmap_write(data->regmap, AP3216C_PS_CALIB_LO,
+					   val & AP3216C_PS_CALIB_LO_MASK);
+			if (ret < 0)
+				return ret;
+
+			tmp = (val & AP3216C_PS_CALIB_HI_MASK) >>
+				AP3216C_PS_CALIB_HI_SHIFT;
+
+			return regmap_write(data->regmap,
+					    AP3216C_PS_CALIB_HI, tmp);
+
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
+static int ap3216c_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct ap3216c_data *data = iio_priv(indio_dev);
+	int lo, hi;
+	int cfg, tmp;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			ret = regmap_read(data->regmap,
+					  AP3216C_ALS_CALIB, &tmp);
+			if (ret < 0)
+				return ret;
+
+			/* Scale is multiplied by 1/64 */
+			*val = tmp;
+			*val2 = AP3216C_ALS_CALIB_DENOM;
+			return IIO_VAL_FRACTIONAL;
+
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_CALIBBIAS:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			ret = regmap_read(data->regmap,
+					  AP3216C_PS_CALIB_LO, &lo);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_PS_CALIB_HI, &hi);
+			if (ret < 0)
+				return ret;
+
+
+			*val = lo | (hi << AP3216C_PS_CALIB_HI_SHIFT);
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_PROCESSED:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			ret = regmap_read(data->regmap,
+					  AP3216C_ALS_DATA_LO, &lo);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_ALS_DATA_HI, &hi);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_ALS_CFG, &cfg);
+			if (ret < 0)
+				return ret;
+
+			tmp = (hi << AP3216C_ALS_DATA_HI_SHIFT) | lo;
+			tmp *= ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
+
+			*val = tmp;
+			*val2 = AP3216C_ALS_DATA_DENOM;
+
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			if (chan->channel2 != IIO_MOD_LIGHT_IR)
+				return -EINVAL;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_IR_DATA_LO, &lo);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_IR_DATA_HI, &hi);
+			if (ret < 0)
+				return ret;
+
+			tmp = lo & AP3216C_IR_DATA_LO_MASK;
+			tmp |= hi << AP3216C_IR_DATA_HI_SHIFT;
+			*val = tmp;
+
+			return IIO_VAL_INT;
+
+		case IIO_PROXIMITY:
+			ret = regmap_read(data->regmap,
+					  AP3216C_PS_DATA_LO, &lo);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_PS_DATA_HI, &hi);
+			if (ret < 0)
+				return ret;
+
+			ret = regmap_read(data->regmap,
+					  AP3216C_PS_CFG, &cfg);
+			if (ret < 0)
+				return ret;
+
+			tmp = lo & AP3216C_PS_DATA_LO_MASK;
+			tmp |= (hi & AP3216C_PS_DATA_HI_MASK) <<
+				AP3216C_PS_DATA_HI_SHIFT;
+			*val = tmp * AP3216C_PS_CFG_GAIN(cfg);
+
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
+static const struct iio_info ap3216c_info = {
+	.read_raw = ap3216c_read_raw,
+	.write_raw = ap3216c_write_raw,
+	.read_event_value = ap3216c_read_event_value,
+	.write_event_value = ap3216c_write_event_value,
+	.read_event_config = ap3216c_read_event_config,
+	.write_event_config = ap3216c_write_event_config,
+};
+
+static int ap3216c_clear_int(struct ap3216c_data *data)
+{
+	return regmap_write(data->regmap,
+			    AP3216C_INT_STATUS, AP3216C_INT_STATUS_CLR);
+}
+
+static irqreturn_t ap3216c_event_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct ap3216c_data *data = iio_priv(indio_dev);
+	int status;
+	s64 timestamp;
+	int ret;
+
+	ret = regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
+	if (ret) {
+		/*
+		 * Without being able to guarantee the interrupt came from
+		 * this device, we must return IRQ_HANDLED instead of
+		 * IRQ_NONE.
+		 */
+		dev_err(&data->client->dev, "error reading IRQ status\n");
+		return IRQ_HANDLED;
+	}
+
+	/* The IRQ was not from this device */
+	if (!status)
+		return IRQ_NONE;
+
+	timestamp = iio_get_time_ns(indio_dev);
+	if ((status & AP3216C_INT_STATUS_PS_MASK) && data->prox_thresh_en)
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_EITHER),
+			       timestamp);
+
+	if ((status & AP3216C_INT_STATUS_ALS_MASK) && data->als_thresh_en)
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_EITHER),
+			       timestamp);
+
+	ret = ap3216c_clear_int(data);
+	if (ret < 0)
+		dev_err(&data->client->dev, "error clearing IRQ\n");
+
+	return IRQ_HANDLED;
+}
+
+static int ap3216c_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct ap3216c_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &ap3216c_info;
+	indio_dev->name = AP3216C_DRV_NAME;
+	indio_dev->channels = ap3216c_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ap3216c_channels);
+
+	data->regmap = devm_regmap_init_i2c(client, &ap3216c_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(&client->dev, "Failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	/* Default to thresh events disabled */
+	data->als_thresh_en = false;
+	data->prox_thresh_en = false;
+
+	/*
+	 * Require that that the interrupt is cleared only when the INT
+	 * register is written to, instead of when data is read.  This
+	 * prevents the interrupt from falsely reporting IRQ_NONE.
+	 */
+	ret = regmap_write(data->regmap,
+			   AP3216C_INT_CLR, AP3216C_INT_CLR_MANUAL);
+	if (ret < 0)
+		return ret;
+
+	/* Before setting up IRQ, clear any stale interrupt */
+	ret = ap3216c_clear_int(data);
+	if (ret < 0)
+		return ret;
+
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						NULL, ap3216c_event_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_SHARED | IRQF_ONESHOT,
+						client->name, indio_dev);
+		if (ret)
+			return ret;
+	}
+
+	/* Enable ALS and PS+IR */
+	ret = regmap_write(data->regmap, AP3216C_SYS, AP3216C_SYS_MODE_ALS_PS);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct of_device_id ap3216c_of_match[] = {
+	{ .compatible = "liteon,ap3216c", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ap3216c_of_match);
+
+static const struct i2c_device_id ap3216c_id[] = {
+	{"ap3216c", 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ap3216c_id);
+
+static struct i2c_driver ap3216c_driver = {
+	.driver = {
+		.name	= AP3216C_DRV_NAME,
+	},
+	.probe		= ap3216c_probe,
+	.id_table	= ap3216c_id,
+};
+module_i2c_driver(ap3216c_driver);
+
+MODULE_AUTHOR("Robert Eshleman <bobbyeshleman@gmail.com>");
+MODULE_DESCRIPTION("APC3216C Ambient Light and Proximity Sensor");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

[PATCH 3/3] dt-bindings: iio: light: Add ap3216c

[Robert Eshleman]

Adds device tree bindings for the ap3216c ambient light and proximity
sensor.

Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
---
 .../devicetree/bindings/iio/light/ap3216c.txt | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/ap3216c.txt

diff --git a/Documentation/devicetree/bindings/iio/light/ap3216c.txt b/Documentation/devicetree/bindings/iio/light/ap3216c.txt
new file mode 100644
index 000000000000..c5ae80c79dd8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/ap3216c.txt
@@ -0,0 +1,22 @@
+* AP3216C Ambient Light and Proximity Sensor
+
+Required properties:
+  - compatible: should be "liteon,ap3216c"
+  - reg: the I2C address of the sensor (default is <0x1e>)
+
+Optional properties:
+  - interrupts: interrupt mapping for GPIO IRQ. Should be configured with
+    IRQ_TYPE_EDGE_FALLING.
+
+Refer to interrupt-controller/interrupts.txt for generic interrupt client
+node bindings.
+
+Example:
+
+light-sensor@1e {
+	compatible = "liteon,ap3216c";
+	reg = <0x1e>;
+
+	interrupt-parent = <&gpio1>;
+	interrupts = <17 IRQ_TYPE_EDGE_FALLING>;
+};
-- 
2.20.1

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Peter Meerwald-Stadler]

On Sun, 10 Feb 2019, Robert Eshleman wrote:

> This patch adds support for the ap3216c ambient light and proximity
> sensor.

comments below
 
> Supported features include:
> 
> * Illuminance (lux)
> * Proximity (raw)
> * IR (raw)
> * Rising/falling threshold events for illuminance and proximity
> * Calibration scale for illuminance
> * Calibration bias for proximity
> 
> Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
> ---
>  drivers/iio/light/Kconfig   |  11 +
>  drivers/iio/light/Makefile  |   1 +
>  drivers/iio/light/ap3216c.c | 793 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 805 insertions(+)
>  create mode 100644 drivers/iio/light/ap3216c.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 36f458433480..74688d19beb1 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -41,6 +41,17 @@ config AL3320A
>  	 To compile this driver as a module, choose M here: the
>  	 module will be called al3320a.
>  
> +config AP3216C
> +	tristate "AP3216C Ambient Light and Proximity sensor"
> +	select REGMAP_I2C
> +	depends on I2C
> +	help
> +	  Say Y here to build a driver for the AP3216C Ambient Light and
> +	  Proximity sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ap3216c.
> +
>  config APDS9300
>  	tristate "APDS9300 ambient light sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 286bf3975372..7d2f8fa0f30d 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -7,6 +7,7 @@
>  obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
>  obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
>  obj-$(CONFIG_AL3320A)		+= al3320a.o
> +obj-$(CONFIG_AP3216C)		+= ap3216c.o
>  obj-$(CONFIG_APDS9300)		+= apds9300.o
>  obj-$(CONFIG_APDS9960)		+= apds9960.o
>  obj-$(CONFIG_BH1750)		+= bh1750.o
> diff --git a/drivers/iio/light/ap3216c.c b/drivers/iio/light/ap3216c.c
> new file mode 100644
> index 000000000000..12802b45c01c
> --- /dev/null
> +++ b/drivers/iio/light/ap3216c.c
> @@ -0,0 +1,793 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AP3216C Ambient Light and Infrared Proximity Sensor
> + *
> + * Copyright (c) 2019, Robert Eshleman.
> + *
> + * Datasheet: https://pdf-datasheet-datasheet.netdna-ssl.com/pdf-down/A/P/3/AP3216C-LITE-ON.pdf
> + *
> + * 7-bit I2C slave address 0x1E
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/bits.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +
> +#define AP3216C_DRV_NAME "ap3216c"
> +
> +/* register addresses */
> +#define AP3216C_SYS 0x0
> +#define AP3216C_INT_STATUS 0x01
> +#define AP3216C_INT_CLR 0x02
> +#define AP3216C_IR_DATA_LO 0x0A
> +#define AP3216C_IR_DATA_HI 0x0B
> +#define AP3216C_ALS_DATA_LO 0x0C
> +#define AP3216C_ALS_DATA_HI 0x0D
> +#define AP3216C_PS_DATA_LO 0x0E
> +#define AP3216C_PS_DATA_HI 0x0F
> +#define AP3216C_ALS_CFG 0x10
> +#define AP3216C_ALS_CALIB 0x19
> +#define AP3216C_ALS_LO_THR_LO 0x1A
> +#define AP3216C_ALS_LO_THR_HI 0x1B
> +#define AP3216C_ALS_HI_THR_LO 0x1C
> +#define AP3216C_ALS_HI_THR_HI 0x1D
> +#define AP3216C_PS_CFG 0x20
> +#define AP3216C_PS_CALIB_LO 0x28
> +#define AP3216C_PS_CALIB_HI 0x29
> +#define AP3216C_PS_LO_THR_LO 0x2A
> +#define AP3216C_PS_LO_THR_HI 0x2B
> +#define AP3216C_PS_HI_THR_LO 0x2C
> +#define AP3216C_PS_HI_THR_HI 0x2D
> +
> +/* SYS_MODE mask and config value */
> +#define AP3216C_SYS_MODE_ALS_PS GENMASK(1, 0)
> +#define AP3216C_SYS_MODE_ALS_ONLY 0x01
> +
> +/* INT_STATUS masks and config value */
> +#define AP3216C_INT_STATUS_ALS_MASK 1
> +#define AP3216C_INT_STATUS_PS_MASK (1 << 1)

use BIT()?

> +#define AP3216C_INT_STATUS_CLR GENMASK(1, 0)
> +#define AP3216C_INT_CLR_MANUAL 1
> +
> +/* IR_DATA mask/shift */
> +#define AP3216C_IR_DATA_LO_MASK GENMASK(1, 0)
> +#define AP3216C_IR_DATA_HI_SHIFT 2
> +
> +/* ALS_DATA shift and fractional helper */
> +#define AP3216C_ALS_DATA_HI_SHIFT 8
> +#define AP3216C_ALS_DATA_DENOM 100000
> +
> +/* ALS_CALIB masks/shifts */
> +#define AP3216C_ALS_CALIB_INT_MASK GENMASK(7, 6)
> +#define AP3216C_ALS_CALIB_INT_SHIFT 6
> +#define AP3216C_ALS_CALIB_DEC_MASK GENMASK(5, 0)
> +
> +/* PS_DATA shifts/masks/bits */
> +#define AP3216C_PS_DATA_LO_MASK GENMASK(3, 0)
> +#define AP3216C_PS_DATA_LO_IR_OF BIT(6)
> +#define AP3216C_PS_DATA_HI_MASK GENMASK(5, 0)
> +#define AP3216C_PS_DATA_HI_SHIFT 4
> +#define AP3216C_PS_DATA_HI_IR_OF BIT(6)
> +
> +/* ALS_CFG masks */
> +#define AP3216C_ALS_CFG_GAIN_MASK GENMASK(5, 4)
> +
> +/* ALS_CALIB shifts */
> +#define AP3216C_ALS_CALIB_INT_SHIFT 6
> +
> +/* ALS_HI_THR masks and shifts */
> +#define AP3216C_ALS_HI_THR_LO_MASK GENMASK(7, 0)
> +#define AP3216C_ALS_HI_THR_HI_SHIFT 8
> +#define AP3216C_ALS_HI_THR_HI_MASK GENMASK(7, 4)
> +#define AP3216C_ALS_HI_THR_HI_SHIFT 8
> +
> +/* ALS_LO_THR masks and shifts */
> +#define AP3216C_ALS_LO_THR_LO_MASK GENMASK(3, 0)
> +#define AP3216C_ALS_LO_THR_HI_MASK GENMASK(7, 4)
> +#define AP3216C_ALS_LO_THR_HI_SHIFT 8
> +
> +/* PS_CFG reg mask/shift/bit values */
> +#define AP3216C_PS_CFG_GAIN_MASK GENMASK(3, 2)
> +#define AP3216C_PS_CFG_GAIN_SHIFT 2
> +#define AP3216C_PS_CFG_GAIN(val) \
> +	(1 << ((val & AP3216C_PS_CFG_GAIN_MASK) >> AP3216C_PS_CFG_GAIN_SHIFT))
> +
> +/* PS_LO and PS_HI shift/mask and multiplier values */
> +#define AP3216C_PS_LO_THR_HI_MULT 4
> +#define AP3216C_PS_HI_THR_HI_SHIFT 2
> +#define AP3216C_PS_HI_THR_HI_MULT 4
> +
> +/* PS_THR_LO and PS_THR_HI masks */
> +#define AP3216C_PS_THR_LO_MASK GENMASK(1, 0)
> +#define AP3216C_PS_THR_HI_MASK GENMASK(10, 2)
> +
> +/* PS_CALIB_HI and PS_CALIB_LO shift/mask/bit values */
> +#define AP3216C_PS_CALIB_HI_SHIFT 1
> +#define AP3216C_PS_CALIB_HI_MASK GENMASK(8, 1)
> +#define AP3216C_PS_CALIB_LO_MASK BIT(0)
> +
> +/* ALS fractional helper values */
> +#define AP3216C_ALS_INTEG_MULT 10000
> +#define AP3216C_ALS_FRACT_DIV 100
> +
> +/* ALS_CALIB min/max values (0.0 <= ALS_CALIB <= 3.984375) */
> +#define AP3216C_ALS_CALIB_INT_MIN 0
> +#define AP3216C_ALS_CALIB_DEC_MIN 0
> +#define AP3216C_ALS_CALIB_INT_MAX 3
> +#define AP3216C_ALS_CALIB_DEC_MAX 984375
> +
> +/* ALS_CALIB conversion denominator */
> +#define AP3216C_ALS_CALIB_DENOM 64
> +
> +#define AP3216C_IIO_MULT 1000000
> +
> +static u16 ap3216c_gain_array[] = {

const

> +	3500,  /* 0.3500 lux resolution */
> +	 788,  /* 0.0788 lux resolution */
> +	 197,  /* 0.0197 lux resolution */
> +	  49,  /* 0.0049 lux resolution */
> +};
> +
> +struct ap3216c_data {
> +	struct regmap *regmap;
> +	struct i2c_client *client;
> +	bool als_thresh_en;
> +	bool prox_thresh_en;
> +};
> +
> +static const struct iio_event_spec ap3216c_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_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_chan_spec ap3216c_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +				      BIT(IIO_CHAN_INFO_CALIBSCALE),

should probably be INFO_SCALE, as is used to select sensitivity and is not 
meant to calibrate the chip

> +		.event_spec = ap3216c_event_spec,
> +		.num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
> +	},
> +	{
> +		.type = IIO_PROXIMITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_CALIBBIAS),

should probably be INFO_SCALE

> +		.event_spec = ap3216c_event_spec,
> +		.num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
> +	},
> +	{
> +		.type = IIO_LIGHT,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_IR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	}
> +};
> +
> +static int ap3216c_write_event_value(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan,
> +				       enum iio_event_type type,
> +				       enum iio_event_direction dir,
> +				       enum iio_event_info info,
> +				       int val, int val2)
> +{
> +	struct ap3216c_data *data = iio_priv(indio_dev);
> +	int cfg, gain;
> +	int integral, fractional;
> +	int lo, hi;
> +	u16 thr;
> +	int ret;
> +
> +	switch (chan->type) {
> +	case IIO_LIGHT:
> +		ret = regmap_read(data->regmap, AP3216C_ALS_CFG, &cfg);
> +		if (ret < 0)
> +			return ret;
> +
> +		gain = ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
> +		integral = val * AP3216C_ALS_INTEG_MULT / gain;
> +		fractional = val2 / AP3216C_ALS_FRACT_DIV / gain;
> +
> +		thr = integral + fractional;
> +
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:

mutex around the two writes to have atomic update of regs?

> +			ret = regmap_write(data->regmap, AP3216C_ALS_HI_THR_LO,
> +					   thr & AP3216C_ALS_HI_THR_LO_MASK);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_write(data->regmap,
> +					   AP3216C_ALS_HI_THR_HI,
> +					   (thr & AP3216C_ALS_HI_THR_HI_MASK) >>
> +					   AP3216C_ALS_HI_THR_HI_SHIFT);
> +			if (ret < 0)
> +				return ret;
> +
> +			return ret;
> +
> +		case IIO_EV_DIR_FALLING:
> +			ret = regmap_write(data->regmap,
> +					   AP3216C_ALS_LO_THR_LO,
> +					   thr & AP3216C_ALS_LO_THR_LO_MASK);
> +
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_write(data->regmap,
> +					   AP3216C_ALS_LO_THR_HI,
> +					   (thr & AP3216C_ALS_LO_THR_HI_MASK) >>
> +					   AP3216C_ALS_LO_THR_HI_SHIFT);
> +			if (ret < 0)
> +				return ret;
> +
> +			return ret;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +		return -EINVAL;
> +
> +	case IIO_PROXIMITY:
> +		lo = val & AP3216C_PS_THR_LO_MASK;
> +		hi = (val & AP3216C_PS_THR_HI_MASK) >>
> +			AP3216C_PS_HI_THR_HI_SHIFT;
> +
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = regmap_write(data->regmap,
> +					   AP3216C_PS_HI_THR_LO, lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			return regmap_write(data->regmap,
> +					    AP3216C_PS_HI_THR_HI, hi);
> +
> +		case IIO_EV_DIR_FALLING:
> +			ret = regmap_write(data->regmap,
> +					   AP3216C_PS_LO_THR_LO, lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			return regmap_write(data->regmap,
> +					    AP3216C_PS_LO_THR_HI, hi);
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +	return -EINVAL;

should not be needed

> +}
> +
> +static int ap3216c_read_event_value(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     enum iio_event_info info,
> +				     int *val, int *val2)
> +{
> +	struct ap3216c_data *data = iio_priv(indio_dev);
> +	unsigned int lo, hi;
> +	int ret;
> +
> +	switch (chan->type) {
> +	case IIO_LIGHT:
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_ALS_HI_THR_LO, &lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_ALS_HI_THR_HI, &hi);
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = (hi << AP3216C_ALS_HI_THR_HI_SHIFT) | lo;
> +			return IIO_VAL_INT;
> +
> +		case IIO_EV_DIR_FALLING:
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_ALS_LO_THR_LO, &lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_ALS_LO_THR_HI, &hi);
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = (hi << AP3216C_ALS_LO_THR_HI_SHIFT) | lo;
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_PROXIMITY:
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_PS_HI_THR_LO, &lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_PS_HI_THR_HI, &hi);
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = (hi * AP3216C_PS_HI_THR_HI_MULT) +
> +				(lo & AP3216C_PS_THR_LO_MASK);
> +
> +			return IIO_VAL_INT;
> +
> +		case IIO_EV_DIR_FALLING:
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_PS_LO_THR_LO, &lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_PS_LO_THR_HI, &hi);
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = (hi * AP3216C_PS_LO_THR_HI_MULT) +
> +				(lo & AP3216C_PS_THR_LO_MASK);
> +
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;

should not be needed; compilers are smart enough to see that this is 
unreachable (here and elsewhere)

> +}
> +
> +static int ap3216c_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 ap3216c_data *data = iio_priv(indio_dev);
> +
> +	switch (chan->type) {
> +	case IIO_LIGHT:
> +		return data->als_thresh_en;
> +
> +	case IIO_PROXIMITY:
> +		return data->prox_thresh_en;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ap3216c_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir, int state)
> +{
> +	struct ap3216c_data *data = iio_priv(indio_dev);
> +
> +	switch (chan->type) {
> +	case IIO_LIGHT:
> +		data->als_thresh_en = state;
> +		return 0;
> +
> +	case IIO_PROXIMITY:
> +		data->prox_thresh_en = state;
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct regmap_config ap3216c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AP3216C_PS_HI_THR_HI,
> +};
> +
> +/**
> + * Returns integral part of decimal between 0.0 and 3.984275
> + */
> +static int ap3216c_als_calibscale_int(int val)
> +{
> +	if (val > AP3216C_ALS_CALIB_INT_MAX)
> +		return AP3216C_ALS_CALIB_INT_MAX;
> +
> +	if (val <= AP3216C_ALS_CALIB_INT_MIN)
> +		return AP3216C_ALS_CALIB_INT_MIN;
> +
> +	return val;
> +}
> +
> +/**
> + * Returns decimal part of decimal number between 0.0 and 3.984275
> + */
> +static int ap3216c_als_calibscale_dec(int val, int val2)
> +{
> +	/* Return max decimal if number exceeds calibscale max */
> +	if (val > AP3216C_ALS_CALIB_INT_MAX ||
> +	    (val == AP3216C_ALS_CALIB_INT_MAX &&
> +	     val2 > AP3216C_ALS_CALIB_DEC_MAX))
> +		return AP3216C_ALS_CALIB_DEC_MAX;
> +
> +	/* Floor the decimal if integral below minimum */
> +	if (val <= AP3216C_ALS_CALIB_INT_MIN)
> +		return AP3216C_ALS_CALIB_DEC_MIN;
> +
> +	/* Floor the decimal if decimal below minimum */
> +	if (val2 < AP3216C_ALS_CALIB_DEC_MIN)
> +		return AP3216C_ALS_CALIB_DEC_MIN;
> +
> +	/* Return max decimal if decimal above maximum */
> +	if (val2 > AP3216C_ALS_CALIB_DEC_MAX)
> +		return AP3216C_ALS_CALIB_DEC_MAX;
> +
> +	return val2;
> +}
> +
> +static int ap3216c_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct ap3216c_data *data = iio_priv(indio_dev);
> +	int integral, decimal;
> +	int tmp;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			integral = ap3216c_als_calibscale_int(val);
> +			decimal = ap3216c_als_calibscale_dec(val, val2);
> +
> +			tmp = integral << AP3216C_ALS_CALIB_INT_SHIFT;
> +
> +			/*
> +			 * Reverse scaling by multiplying by the scaling
> +			 * denominator and dividing by IIO multiplier.
> +			 */
> +			tmp |= (decimal * AP3216C_ALS_CALIB_DENOM) /
> +				AP3216C_IIO_MULT;
> +
> +			return regmap_write(data->regmap,
> +					    AP3216C_ALS_CALIB, tmp);
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			ret = regmap_write(data->regmap, AP3216C_PS_CALIB_LO,
> +					   val & AP3216C_PS_CALIB_LO_MASK);
> +			if (ret < 0)
> +				return ret;
> +
> +			tmp = (val & AP3216C_PS_CALIB_HI_MASK) >>
> +				AP3216C_PS_CALIB_HI_SHIFT;
> +
> +			return regmap_write(data->regmap,
> +					    AP3216C_PS_CALIB_HI, tmp);
> +
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int ap3216c_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct ap3216c_data *data = iio_priv(indio_dev);
> +	int lo, hi;
> +	int cfg, tmp;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_ALS_CALIB, &tmp);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Scale is multiplied by 1/64 */
> +			*val = tmp;
> +			*val2 = AP3216C_ALS_CALIB_DENOM;
> +			return IIO_VAL_FRACTIONAL;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_PS_CALIB_LO, &lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_PS_CALIB_HI, &hi);
> +			if (ret < 0)
> +				return ret;
> +
> +
> +			*val = lo | (hi << AP3216C_PS_CALIB_HI_SHIFT);
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_PROCESSED:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_ALS_DATA_LO, &lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_ALS_DATA_HI, &hi);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_ALS_CFG, &cfg);
> +			if (ret < 0)
> +				return ret;
> +
> +			tmp = (hi << AP3216C_ALS_DATA_HI_SHIFT) | lo;
> +			tmp *= ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
> +
> +			*val = tmp;
> +			*val2 = AP3216C_ALS_DATA_DENOM;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			if (chan->channel2 != IIO_MOD_LIGHT_IR)
> +				return -EINVAL;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_IR_DATA_LO, &lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_IR_DATA_HI, &hi);
> +			if (ret < 0)
> +				return ret;
> +
> +			tmp = lo & AP3216C_IR_DATA_LO_MASK;
> +			tmp |= hi << AP3216C_IR_DATA_HI_SHIFT;
> +			*val = tmp;
> +
> +			return IIO_VAL_INT;
> +
> +		case IIO_PROXIMITY:
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_PS_DATA_LO, &lo);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_PS_DATA_HI, &hi);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = regmap_read(data->regmap,
> +					  AP3216C_PS_CFG, &cfg);
> +			if (ret < 0)
> +				return ret;
> +
> +			tmp = lo & AP3216C_PS_DATA_LO_MASK;
> +			tmp |= (hi & AP3216C_PS_DATA_HI_MASK) <<
> +				AP3216C_PS_DATA_HI_SHIFT;
> +			*val = tmp * AP3216C_PS_CFG_GAIN(cfg);
> +
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info ap3216c_info = {
> +	.read_raw = ap3216c_read_raw,
> +	.write_raw = ap3216c_write_raw,
> +	.read_event_value = ap3216c_read_event_value,
> +	.write_event_value = ap3216c_write_event_value,
> +	.read_event_config = ap3216c_read_event_config,
> +	.write_event_config = ap3216c_write_event_config,
> +};
> +
> +static int ap3216c_clear_int(struct ap3216c_data *data)

function needed?

> +{
> +	return regmap_write(data->regmap,
> +			    AP3216C_INT_STATUS, AP3216C_INT_STATUS_CLR);
> +}
> +
> +static irqreturn_t ap3216c_event_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct ap3216c_data *data = iio_priv(indio_dev);
> +	int status;
> +	s64 timestamp;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
> +	if (ret) {
> +		/*
> +		 * Without being able to guarantee the interrupt came from
> +		 * this device, we must return IRQ_HANDLED instead of
> +		 * IRQ_NONE.
> +		 */
> +		dev_err(&data->client->dev, "error reading IRQ status\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	/* The IRQ was not from this device */
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	timestamp = iio_get_time_ns(indio_dev);
> +	if ((status & AP3216C_INT_STATUS_PS_MASK) && data->prox_thresh_en)
> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_EITHER),
> +			       timestamp);
> +
> +	if ((status & AP3216C_INT_STATUS_ALS_MASK) && data->als_thresh_en)
> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_EITHER),
> +			       timestamp);
> +
> +	ret = ap3216c_clear_int(data);
> +	if (ret < 0)
> +		dev_err(&data->client->dev, "error clearing IRQ\n");
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ap3216c_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct ap3216c_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &ap3216c_info;
> +	indio_dev->name = AP3216C_DRV_NAME;
> +	indio_dev->channels = ap3216c_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ap3216c_channels);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &ap3216c_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(&client->dev, "Failed to allocate register map\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	/* Default to thresh events disabled */
> +	data->als_thresh_en = false;
> +	data->prox_thresh_en = false;
> +
> +	/*
> +	 * Require that that the interrupt is cleared only when the INT

that that

> +	 * register is written to, instead of when data is read.  This
> +	 * prevents the interrupt from falsely reporting IRQ_NONE.
> +	 */
> +	ret = regmap_write(data->regmap,
> +			   AP3216C_INT_CLR, AP3216C_INT_CLR_MANUAL);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Before setting up IRQ, clear any stale interrupt */
> +	ret = ap3216c_clear_int(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, ap3216c_event_handler,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_SHARED | IRQF_ONESHOT,
> +						client->name, indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Enable ALS and PS+IR */
> +	ret = regmap_write(data->regmap, AP3216C_SYS, AP3216C_SYS_MODE_ALS_PS);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct of_device_id ap3216c_of_match[] = {
> +	{ .compatible = "liteon,ap3216c", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ap3216c_of_match);
> +
> +static const struct i2c_device_id ap3216c_id[] = {
> +	{"ap3216c", 0},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ap3216c_id);
> +
> +static struct i2c_driver ap3216c_driver = {
> +	.driver = {
> +		.name	= AP3216C_DRV_NAME,
> +	},
> +	.probe		= ap3216c_probe,
> +	.id_table	= ap3216c_id,
> +};
> +module_i2c_driver(ap3216c_driver);
> +
> +MODULE_AUTHOR("Robert Eshleman <bobbyeshleman@gmail.com>");
> +MODULE_DESCRIPTION("APC3216C Ambient Light and Proximity Sensor");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Robert Eshleman]

Hey Peter,

Thanks for the feedback.  I have a question regarding INFO_SCALE and
the calibration scale/bias below.

Thanks for the feedback, it's much appreciated.

On Mon, Feb 11, 2019 at 03:58:27PM +0100, Peter Meerwald-Stadler wrote:
> On Sun, 10 Feb 2019, Robert Eshleman wrote:
> 
> > This patch adds support for the ap3216c ambient light and proximity
> > sensor.
> 
> comments below
>  
> > Supported features include:
> > 
> > * Illuminance (lux)
> > * Proximity (raw)
> > * IR (raw)
> > * Rising/falling threshold events for illuminance and proximity
> > * Calibration scale for illuminance
> > * Calibration bias for proximity
> > 
> > Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
> > ---
> >  drivers/iio/light/Kconfig   |  11 +
> >  drivers/iio/light/Makefile  |   1 +
> >  drivers/iio/light/ap3216c.c | 793 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 805 insertions(+)
> >  create mode 100644 drivers/iio/light/ap3216c.c
> > 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 36f458433480..74688d19beb1 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -41,6 +41,17 @@ config AL3320A
> >  	 To compile this driver as a module, choose M here: the
> >  	 module will be called al3320a.
> >  
> > +config AP3216C
> > +	tristate "AP3216C Ambient Light and Proximity sensor"
> > +	select REGMAP_I2C
> > +	depends on I2C
> > +	help
> > +	  Say Y here to build a driver for the AP3216C Ambient Light and
> > +	  Proximity sensor.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called ap3216c.
> > +
> >  config APDS9300
> >  	tristate "APDS9300 ambient light sensor"
> >  	depends on I2C
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index 286bf3975372..7d2f8fa0f30d 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -7,6 +7,7 @@
> >  obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
> >  obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
> >  obj-$(CONFIG_AL3320A)		+= al3320a.o
> > +obj-$(CONFIG_AP3216C)		+= ap3216c.o
> >  obj-$(CONFIG_APDS9300)		+= apds9300.o
> >  obj-$(CONFIG_APDS9960)		+= apds9960.o
> >  obj-$(CONFIG_BH1750)		+= bh1750.o
> > diff --git a/drivers/iio/light/ap3216c.c b/drivers/iio/light/ap3216c.c
> > new file mode 100644
> > index 000000000000..12802b45c01c
> > --- /dev/null
> > +++ b/drivers/iio/light/ap3216c.c
> > @@ -0,0 +1,793 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * AP3216C Ambient Light and Infrared Proximity Sensor
> > + *
> > + * Copyright (c) 2019, Robert Eshleman.
> > + *
> > + * Datasheet: https://pdf-datasheet-datasheet.netdna-ssl.com/pdf-down/A/P/3/AP3216C-LITE-ON.pdf
> > + *
> > + * 7-bit I2C slave address 0x1E
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/bits.h>
> > +#include <linux/regmap.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/events.h>
> > +
> > +#define AP3216C_DRV_NAME "ap3216c"
> > +
> > +/* register addresses */
> > +#define AP3216C_SYS 0x0
> > +#define AP3216C_INT_STATUS 0x01
> > +#define AP3216C_INT_CLR 0x02
> > +#define AP3216C_IR_DATA_LO 0x0A
> > +#define AP3216C_IR_DATA_HI 0x0B
> > +#define AP3216C_ALS_DATA_LO 0x0C
> > +#define AP3216C_ALS_DATA_HI 0x0D
> > +#define AP3216C_PS_DATA_LO 0x0E
> > +#define AP3216C_PS_DATA_HI 0x0F
> > +#define AP3216C_ALS_CFG 0x10
> > +#define AP3216C_ALS_CALIB 0x19
> > +#define AP3216C_ALS_LO_THR_LO 0x1A
> > +#define AP3216C_ALS_LO_THR_HI 0x1B
> > +#define AP3216C_ALS_HI_THR_LO 0x1C
> > +#define AP3216C_ALS_HI_THR_HI 0x1D
> > +#define AP3216C_PS_CFG 0x20
> > +#define AP3216C_PS_CALIB_LO 0x28
> > +#define AP3216C_PS_CALIB_HI 0x29
> > +#define AP3216C_PS_LO_THR_LO 0x2A
> > +#define AP3216C_PS_LO_THR_HI 0x2B
> > +#define AP3216C_PS_HI_THR_LO 0x2C
> > +#define AP3216C_PS_HI_THR_HI 0x2D
> > +
> > +/* SYS_MODE mask and config value */
> > +#define AP3216C_SYS_MODE_ALS_PS GENMASK(1, 0)
> > +#define AP3216C_SYS_MODE_ALS_ONLY 0x01
> > +
> > +/* INT_STATUS masks and config value */
> > +#define AP3216C_INT_STATUS_ALS_MASK 1
> > +#define AP3216C_INT_STATUS_PS_MASK (1 << 1)
> 
> use BIT()?

Got it.

> 
> > +#define AP3216C_INT_STATUS_CLR GENMASK(1, 0)
> > +#define AP3216C_INT_CLR_MANUAL 1
> > +
> > +/* IR_DATA mask/shift */
> > +#define AP3216C_IR_DATA_LO_MASK GENMASK(1, 0)
> > +#define AP3216C_IR_DATA_HI_SHIFT 2
> > +
> > +/* ALS_DATA shift and fractional helper */
> > +#define AP3216C_ALS_DATA_HI_SHIFT 8
> > +#define AP3216C_ALS_DATA_DENOM 100000
> > +
> > +/* ALS_CALIB masks/shifts */
> > +#define AP3216C_ALS_CALIB_INT_MASK GENMASK(7, 6)
> > +#define AP3216C_ALS_CALIB_INT_SHIFT 6
> > +#define AP3216C_ALS_CALIB_DEC_MASK GENMASK(5, 0)
> > +
> > +/* PS_DATA shifts/masks/bits */
> > +#define AP3216C_PS_DATA_LO_MASK GENMASK(3, 0)
> > +#define AP3216C_PS_DATA_LO_IR_OF BIT(6)
> > +#define AP3216C_PS_DATA_HI_MASK GENMASK(5, 0)
> > +#define AP3216C_PS_DATA_HI_SHIFT 4
> > +#define AP3216C_PS_DATA_HI_IR_OF BIT(6)
> > +
> > +/* ALS_CFG masks */
> > +#define AP3216C_ALS_CFG_GAIN_MASK GENMASK(5, 4)
> > +
> > +/* ALS_CALIB shifts */
> > +#define AP3216C_ALS_CALIB_INT_SHIFT 6
> > +
> > +/* ALS_HI_THR masks and shifts */
> > +#define AP3216C_ALS_HI_THR_LO_MASK GENMASK(7, 0)
> > +#define AP3216C_ALS_HI_THR_HI_SHIFT 8
> > +#define AP3216C_ALS_HI_THR_HI_MASK GENMASK(7, 4)
> > +#define AP3216C_ALS_HI_THR_HI_SHIFT 8
> > +
> > +/* ALS_LO_THR masks and shifts */
> > +#define AP3216C_ALS_LO_THR_LO_MASK GENMASK(3, 0)
> > +#define AP3216C_ALS_LO_THR_HI_MASK GENMASK(7, 4)
> > +#define AP3216C_ALS_LO_THR_HI_SHIFT 8
> > +
> > +/* PS_CFG reg mask/shift/bit values */
> > +#define AP3216C_PS_CFG_GAIN_MASK GENMASK(3, 2)
> > +#define AP3216C_PS_CFG_GAIN_SHIFT 2
> > +#define AP3216C_PS_CFG_GAIN(val) \
> > +	(1 << ((val & AP3216C_PS_CFG_GAIN_MASK) >> AP3216C_PS_CFG_GAIN_SHIFT))
> > +
> > +/* PS_LO and PS_HI shift/mask and multiplier values */
> > +#define AP3216C_PS_LO_THR_HI_MULT 4
> > +#define AP3216C_PS_HI_THR_HI_SHIFT 2
> > +#define AP3216C_PS_HI_THR_HI_MULT 4
> > +
> > +/* PS_THR_LO and PS_THR_HI masks */
> > +#define AP3216C_PS_THR_LO_MASK GENMASK(1, 0)
> > +#define AP3216C_PS_THR_HI_MASK GENMASK(10, 2)
> > +
> > +/* PS_CALIB_HI and PS_CALIB_LO shift/mask/bit values */
> > +#define AP3216C_PS_CALIB_HI_SHIFT 1
> > +#define AP3216C_PS_CALIB_HI_MASK GENMASK(8, 1)
> > +#define AP3216C_PS_CALIB_LO_MASK BIT(0)
> > +
> > +/* ALS fractional helper values */
> > +#define AP3216C_ALS_INTEG_MULT 10000
> > +#define AP3216C_ALS_FRACT_DIV 100
> > +
> > +/* ALS_CALIB min/max values (0.0 <= ALS_CALIB <= 3.984375) */
> > +#define AP3216C_ALS_CALIB_INT_MIN 0
> > +#define AP3216C_ALS_CALIB_DEC_MIN 0
> > +#define AP3216C_ALS_CALIB_INT_MAX 3
> > +#define AP3216C_ALS_CALIB_DEC_MAX 984375
> > +
> > +/* ALS_CALIB conversion denominator */
> > +#define AP3216C_ALS_CALIB_DENOM 64
> > +
> > +#define AP3216C_IIO_MULT 1000000
> > +
> > +static u16 ap3216c_gain_array[] = {
> 
> const
> 
> > +	3500,  /* 0.3500 lux resolution */
> > +	 788,  /* 0.0788 lux resolution */
> > +	 197,  /* 0.0197 lux resolution */
> > +	  49,  /* 0.0049 lux resolution */
> > +};
> > +
> > +struct ap3216c_data {
> > +	struct regmap *regmap;
> > +	struct i2c_client *client;
> > +	bool als_thresh_en;
> > +	bool prox_thresh_en;
> > +};
> > +
> > +static const struct iio_event_spec ap3216c_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_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > +	},
> > +};
> > +
> > +static const struct iio_chan_spec ap3216c_channels[] = {
> > +	{
> > +		.type = IIO_LIGHT,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > +				      BIT(IIO_CHAN_INFO_CALIBSCALE),
> 
> should probably be INFO_SCALE, as is used to select sensitivity and is not 
> meant to calibrate the chip
> 

The datasheet describes the register used here as being for
calibration.  Considering this, is INFO_SCALE still a
better fit than INFO_CALIBSCALE?

> > +		.event_spec = ap3216c_event_spec,
> > +		.num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
> > +	},
> > +	{
> > +		.type = IIO_PROXIMITY,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_CALIBBIAS),
> 
> should probably be INFO_SCALE
> 

The datasheet claims that this similar to the calibration for light,
but it really behaves quite differently.  It is intended for
post-assembly calibration.  The higher the value goes, the more the
PS reading decreases (not documented in the datasheet).  It reaches
zero _very_ quickly.  This seemed like a bias calibration to me, but
I could totally be wrong.  Is INFO_SCALE the best fit for this?

> > +		.event_spec = ap3216c_event_spec,
> > +		.num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
> > +	},
> > +	{
> > +		.type = IIO_LIGHT,
> > +		.modified = 1,
> > +		.channel2 = IIO_MOD_LIGHT_IR,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +	}
> > +};
> > +
> > +static int ap3216c_write_event_value(struct iio_dev *indio_dev,
> > +				       const struct iio_chan_spec *chan,
> > +				       enum iio_event_type type,
> > +				       enum iio_event_direction dir,
> > +				       enum iio_event_info info,
> > +				       int val, int val2)
> > +{
> > +	struct ap3216c_data *data = iio_priv(indio_dev);
> > +	int cfg, gain;
> > +	int integral, fractional;
> > +	int lo, hi;
> > +	u16 thr;
> > +	int ret;
> > +
> > +	switch (chan->type) {
> > +	case IIO_LIGHT:
> > +		ret = regmap_read(data->regmap, AP3216C_ALS_CFG, &cfg);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		gain = ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
> > +		integral = val * AP3216C_ALS_INTEG_MULT / gain;
> > +		fractional = val2 / AP3216C_ALS_FRACT_DIV / gain;
> > +
> > +		thr = integral + fractional;
> > +
> > +		switch (dir) {
> > +		case IIO_EV_DIR_RISING:
> 
> mutex around the two writes to have atomic update of regs?

Got it.

> 
> > +			ret = regmap_write(data->regmap, AP3216C_ALS_HI_THR_LO,
> > +					   thr & AP3216C_ALS_HI_THR_LO_MASK);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_write(data->regmap,
> > +					   AP3216C_ALS_HI_THR_HI,
> > +					   (thr & AP3216C_ALS_HI_THR_HI_MASK) >>
> > +					   AP3216C_ALS_HI_THR_HI_SHIFT);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			return ret;
> > +
> > +		case IIO_EV_DIR_FALLING:
> > +			ret = regmap_write(data->regmap,
> > +					   AP3216C_ALS_LO_THR_LO,
> > +					   thr & AP3216C_ALS_LO_THR_LO_MASK);
> > +
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_write(data->regmap,
> > +					   AP3216C_ALS_LO_THR_HI,
> > +					   (thr & AP3216C_ALS_LO_THR_HI_MASK) >>
> > +					   AP3216C_ALS_LO_THR_HI_SHIFT);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			return ret;
> > +
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +		return -EINVAL;
> > +
> > +	case IIO_PROXIMITY:
> > +		lo = val & AP3216C_PS_THR_LO_MASK;
> > +		hi = (val & AP3216C_PS_THR_HI_MASK) >>
> > +			AP3216C_PS_HI_THR_HI_SHIFT;
> > +
> > +		switch (dir) {
> > +		case IIO_EV_DIR_RISING:
> > +			ret = regmap_write(data->regmap,
> > +					   AP3216C_PS_HI_THR_LO, lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			return regmap_write(data->regmap,
> > +					    AP3216C_PS_HI_THR_HI, hi);
> > +
> > +		case IIO_EV_DIR_FALLING:
> > +			ret = regmap_write(data->regmap,
> > +					   AP3216C_PS_LO_THR_LO, lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			return regmap_write(data->regmap,
> > +					    AP3216C_PS_LO_THR_HI, hi);
> > +
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return -EINVAL;
> 
> should not be needed
> 
> > +}
> > +
> > +static int ap3216c_read_event_value(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     enum iio_event_type type,
> > +				     enum iio_event_direction dir,
> > +				     enum iio_event_info info,
> > +				     int *val, int *val2)
> > +{
> > +	struct ap3216c_data *data = iio_priv(indio_dev);
> > +	unsigned int lo, hi;
> > +	int ret;
> > +
> > +	switch (chan->type) {
> > +	case IIO_LIGHT:
> > +		switch (dir) {
> > +		case IIO_EV_DIR_RISING:
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_ALS_HI_THR_LO, &lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_ALS_HI_THR_HI, &hi);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			*val = (hi << AP3216C_ALS_HI_THR_HI_SHIFT) | lo;
> > +			return IIO_VAL_INT;
> > +
> > +		case IIO_EV_DIR_FALLING:
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_ALS_LO_THR_LO, &lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_ALS_LO_THR_HI, &hi);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			*val = (hi << AP3216C_ALS_LO_THR_HI_SHIFT) | lo;
> > +			return IIO_VAL_INT;
> > +
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +	case IIO_PROXIMITY:
> > +		switch (dir) {
> > +		case IIO_EV_DIR_RISING:
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_PS_HI_THR_LO, &lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_PS_HI_THR_HI, &hi);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			*val = (hi * AP3216C_PS_HI_THR_HI_MULT) +
> > +				(lo & AP3216C_PS_THR_LO_MASK);
> > +
> > +			return IIO_VAL_INT;
> > +
> > +		case IIO_EV_DIR_FALLING:
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_PS_LO_THR_LO, &lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_PS_LO_THR_HI, &hi);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			*val = (hi * AP3216C_PS_LO_THR_HI_MULT) +
> > +				(lo & AP3216C_PS_THR_LO_MASK);
> > +
> > +			return IIO_VAL_INT;
> > +
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return -EINVAL;
> 
> should not be needed; compilers are smart enough to see that this is 
> unreachable (here and elsewhere)
> 

Got it.

> > +}
> > +
> > +static int ap3216c_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 ap3216c_data *data = iio_priv(indio_dev);
> > +
> > +	switch (chan->type) {
> > +	case IIO_LIGHT:
> > +		return data->als_thresh_en;
> > +
> > +	case IIO_PROXIMITY:
> > +		return data->prox_thresh_en;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int ap3216c_write_event_config(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     enum iio_event_type type,
> > +				     enum iio_event_direction dir, int state)
> > +{
> > +	struct ap3216c_data *data = iio_priv(indio_dev);
> > +
> > +	switch (chan->type) {
> > +	case IIO_LIGHT:
> > +		data->als_thresh_en = state;
> > +		return 0;
> > +
> > +	case IIO_PROXIMITY:
> > +		data->prox_thresh_en = state;
> > +		return 0;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static const struct regmap_config ap3216c_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = AP3216C_PS_HI_THR_HI,
> > +};
> > +
> > +/**
> > + * Returns integral part of decimal between 0.0 and 3.984275
> > + */
> > +static int ap3216c_als_calibscale_int(int val)
> > +{
> > +	if (val > AP3216C_ALS_CALIB_INT_MAX)
> > +		return AP3216C_ALS_CALIB_INT_MAX;
> > +
> > +	if (val <= AP3216C_ALS_CALIB_INT_MIN)
> > +		return AP3216C_ALS_CALIB_INT_MIN;
> > +
> > +	return val;
> > +}
> > +
> > +/**
> > + * Returns decimal part of decimal number between 0.0 and 3.984275
> > + */
> > +static int ap3216c_als_calibscale_dec(int val, int val2)
> > +{
> > +	/* Return max decimal if number exceeds calibscale max */
> > +	if (val > AP3216C_ALS_CALIB_INT_MAX ||
> > +	    (val == AP3216C_ALS_CALIB_INT_MAX &&
> > +	     val2 > AP3216C_ALS_CALIB_DEC_MAX))
> > +		return AP3216C_ALS_CALIB_DEC_MAX;
> > +
> > +	/* Floor the decimal if integral below minimum */
> > +	if (val <= AP3216C_ALS_CALIB_INT_MIN)
> > +		return AP3216C_ALS_CALIB_DEC_MIN;
> > +
> > +	/* Floor the decimal if decimal below minimum */
> > +	if (val2 < AP3216C_ALS_CALIB_DEC_MIN)
> > +		return AP3216C_ALS_CALIB_DEC_MIN;
> > +
> > +	/* Return max decimal if decimal above maximum */
> > +	if (val2 > AP3216C_ALS_CALIB_DEC_MAX)
> > +		return AP3216C_ALS_CALIB_DEC_MAX;
> > +
> > +	return val2;
> > +}
> > +
> > +static int ap3216c_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct ap3216c_data *data = iio_priv(indio_dev);
> > +	int integral, decimal;
> > +	int tmp;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		switch (chan->type) {
> > +		case IIO_LIGHT:
> > +			integral = ap3216c_als_calibscale_int(val);
> > +			decimal = ap3216c_als_calibscale_dec(val, val2);
> > +
> > +			tmp = integral << AP3216C_ALS_CALIB_INT_SHIFT;
> > +
> > +			/*
> > +			 * Reverse scaling by multiplying by the scaling
> > +			 * denominator and dividing by IIO multiplier.
> > +			 */
> > +			tmp |= (decimal * AP3216C_ALS_CALIB_DENOM) /
> > +				AP3216C_IIO_MULT;
> > +
> > +			return regmap_write(data->regmap,
> > +					    AP3216C_ALS_CALIB, tmp);
> > +
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			ret = regmap_write(data->regmap, AP3216C_PS_CALIB_LO,
> > +					   val & AP3216C_PS_CALIB_LO_MASK);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			tmp = (val & AP3216C_PS_CALIB_HI_MASK) >>
> > +				AP3216C_PS_CALIB_HI_SHIFT;
> > +
> > +			return regmap_write(data->regmap,
> > +					    AP3216C_PS_CALIB_HI, tmp);
> > +
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> > +static int ap3216c_read_raw(struct iio_dev *indio_dev,
> > +				struct iio_chan_spec const *chan,
> > +				int *val, int *val2, long mask)
> > +{
> > +	struct ap3216c_data *data = iio_priv(indio_dev);
> > +	int lo, hi;
> > +	int cfg, tmp;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		switch (chan->type) {
> > +		case IIO_LIGHT:
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_ALS_CALIB, &tmp);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			/* Scale is multiplied by 1/64 */
> > +			*val = tmp;
> > +			*val2 = AP3216C_ALS_CALIB_DENOM;
> > +			return IIO_VAL_FRACTIONAL;
> > +
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_PS_CALIB_LO, &lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_PS_CALIB_HI, &hi);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +
> > +			*val = lo | (hi << AP3216C_PS_CALIB_HI_SHIFT);
> > +			return IIO_VAL_INT;
> > +
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		switch (chan->type) {
> > +		case IIO_LIGHT:
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_ALS_DATA_LO, &lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_ALS_DATA_HI, &hi);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_ALS_CFG, &cfg);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			tmp = (hi << AP3216C_ALS_DATA_HI_SHIFT) | lo;
> > +			tmp *= ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
> > +
> > +			*val = tmp;
> > +			*val2 = AP3216C_ALS_DATA_DENOM;
> > +
> > +			return IIO_VAL_FRACTIONAL;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_LIGHT:
> > +			if (chan->channel2 != IIO_MOD_LIGHT_IR)
> > +				return -EINVAL;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_IR_DATA_LO, &lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_IR_DATA_HI, &hi);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			tmp = lo & AP3216C_IR_DATA_LO_MASK;
> > +			tmp |= hi << AP3216C_IR_DATA_HI_SHIFT;
> > +			*val = tmp;
> > +
> > +			return IIO_VAL_INT;
> > +
> > +		case IIO_PROXIMITY:
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_PS_DATA_LO, &lo);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_PS_DATA_HI, &hi);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ret = regmap_read(data->regmap,
> > +					  AP3216C_PS_CFG, &cfg);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			tmp = lo & AP3216C_PS_DATA_LO_MASK;
> > +			tmp |= (hi & AP3216C_PS_DATA_HI_MASK) <<
> > +				AP3216C_PS_DATA_HI_SHIFT;
> > +			*val = tmp * AP3216C_PS_CFG_GAIN(cfg);
> > +
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> > +static const struct iio_info ap3216c_info = {
> > +	.read_raw = ap3216c_read_raw,
> > +	.write_raw = ap3216c_write_raw,
> > +	.read_event_value = ap3216c_read_event_value,
> > +	.write_event_value = ap3216c_write_event_value,
> > +	.read_event_config = ap3216c_read_event_config,
> > +	.write_event_config = ap3216c_write_event_config,
> > +};
> > +
> > +static int ap3216c_clear_int(struct ap3216c_data *data)
> 
> function needed?
> 

Will decompose this, and move to individual clearing of bits.

> > +{
> > +	return regmap_write(data->regmap,
> > +			    AP3216C_INT_STATUS, AP3216C_INT_STATUS_CLR);
> > +}
> > +
> > +static irqreturn_t ap3216c_event_handler(int irq, void *p)
> > +{
> > +	struct iio_dev *indio_dev = p;
> > +	struct ap3216c_data *data = iio_priv(indio_dev);
> > +	int status;
> > +	s64 timestamp;
> > +	int ret;
> > +
> > +	ret = regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
> > +	if (ret) {
> > +		/*
> > +		 * Without being able to guarantee the interrupt came from
> > +		 * this device, we must return IRQ_HANDLED instead of
> > +		 * IRQ_NONE.
> > +		 */
> > +		dev_err(&data->client->dev, "error reading IRQ status\n");
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	/* The IRQ was not from this device */
> > +	if (!status)
> > +		return IRQ_NONE;
> > +
> > +	timestamp = iio_get_time_ns(indio_dev);
> > +	if ((status & AP3216C_INT_STATUS_PS_MASK) && data->prox_thresh_en)
> > +		iio_push_event(indio_dev,
> > +			       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> > +						    IIO_EV_TYPE_THRESH,
> > +						    IIO_EV_DIR_EITHER),
> > +			       timestamp);
> > +
> > +	if ((status & AP3216C_INT_STATUS_ALS_MASK) && data->als_thresh_en)
> > +		iio_push_event(indio_dev,
> > +			       IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> > +						    IIO_EV_TYPE_THRESH,
> > +						    IIO_EV_DIR_EITHER),
> > +			       timestamp);
> > +
> > +	ret = ap3216c_clear_int(data);
> > +	if (ret < 0)
> > +		dev_err(&data->client->dev, "error clearing IRQ\n");
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int ap3216c_probe(struct i2c_client *client,
> > +			  const struct i2c_device_id *id)
> > +{
> > +	struct ap3216c_data *data;
> > +	struct iio_dev *indio_dev;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->info = &ap3216c_info;
> > +	indio_dev->name = AP3216C_DRV_NAME;
> > +	indio_dev->channels = ap3216c_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(ap3216c_channels);
> > +
> > +	data->regmap = devm_regmap_init_i2c(client, &ap3216c_regmap_config);
> > +	if (IS_ERR(data->regmap)) {
> > +		dev_err(&client->dev, "Failed to allocate register map\n");
> > +		return PTR_ERR(data->regmap);
> > +	}
> > +
> > +	/* Default to thresh events disabled */
> > +	data->als_thresh_en = false;
> > +	data->prox_thresh_en = false;
> > +
> > +	/*
> > +	 * Require that that the interrupt is cleared only when the INT
> 
> that that
> 
> > +	 * register is written to, instead of when data is read.  This
> > +	 * prevents the interrupt from falsely reporting IRQ_NONE.
> > +	 */
> > +	ret = regmap_write(data->regmap,
> > +			   AP3216C_INT_CLR, AP3216C_INT_CLR_MANUAL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Before setting up IRQ, clear any stale interrupt */
> > +	ret = ap3216c_clear_int(data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (client->irq) {
> > +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > +						NULL, ap3216c_event_handler,
> > +						IRQF_TRIGGER_FALLING |
> > +						IRQF_SHARED | IRQF_ONESHOT,
> > +						client->name, indio_dev);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	/* Enable ALS and PS+IR */
> > +	ret = regmap_write(data->regmap, AP3216C_SYS, AP3216C_SYS_MODE_ALS_PS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct of_device_id ap3216c_of_match[] = {
> > +	{ .compatible = "liteon,ap3216c", },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ap3216c_of_match);
> > +
> > +static const struct i2c_device_id ap3216c_id[] = {
> > +	{"ap3216c", 0},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ap3216c_id);
> > +
> > +static struct i2c_driver ap3216c_driver = {
> > +	.driver = {
> > +		.name	= AP3216C_DRV_NAME,
> > +	},
> > +	.probe		= ap3216c_probe,
> > +	.id_table	= ap3216c_id,
> > +};
> > +module_i2c_driver(ap3216c_driver);
> > +
> > +MODULE_AUTHOR("Robert Eshleman <bobbyeshleman@gmail.com>");
> > +MODULE_DESCRIPTION("APC3216C Ambient Light and Proximity Sensor");
> > +MODULE_LICENSE("GPL v2");
> > 
> 
> -- 
> 
> Peter Meerwald-Stadler
> Mobile: +43 664 24 44 418


Thanks for the feedback again.

-Bobby

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck]

Hi Robert,

On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman <bobbyeshleman@gmail.com> wrote:
>
> This patch adds support for the ap3216c ambient light and proximity
> sensor.

Comments below.

>
> Supported features include:
>
> * Illuminance (lux)
> * Proximity (raw)
> * IR (raw)
> * Rising/falling threshold events for illuminance and proximity
> * Calibration scale for illuminance
> * Calibration bias for proximity
>
> Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
> ---
>  drivers/iio/light/Kconfig   |  11 +
>  drivers/iio/light/Makefile  |   1 +
>  drivers/iio/light/ap3216c.c | 793 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 805 insertions(+)
>  create mode 100644 drivers/iio/light/ap3216c.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 36f458433480..74688d19beb1 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -41,6 +41,17 @@ config AL3320A
>          To compile this driver as a module, choose M here: the
>          module will be called al3320a.
>
> +config AP3216C
> +       tristate "AP3216C Ambient Light and Proximity sensor"
> +       select REGMAP_I2C
> +       depends on I2C
> +       help
> +         Say Y here to build a driver for the AP3216C Ambient Light and
> +         Proximity sensor.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called ap3216c.
> +
>  config APDS9300
>         tristate "APDS9300 ambient light sensor"
>         depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 286bf3975372..7d2f8fa0f30d 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -7,6 +7,7 @@
>  obj-$(CONFIG_ACPI_ALS)         += acpi-als.o
>  obj-$(CONFIG_ADJD_S311)                += adjd_s311.o
>  obj-$(CONFIG_AL3320A)          += al3320a.o
> +obj-$(CONFIG_AP3216C)          += ap3216c.o
>  obj-$(CONFIG_APDS9300)         += apds9300.o
>  obj-$(CONFIG_APDS9960)         += apds9960.o
>  obj-$(CONFIG_BH1750)           += bh1750.o
> diff --git a/drivers/iio/light/ap3216c.c b/drivers/iio/light/ap3216c.c
> new file mode 100644
> index 000000000000..12802b45c01c
> --- /dev/null
> +++ b/drivers/iio/light/ap3216c.c
> @@ -0,0 +1,793 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AP3216C Ambient Light and Infrared Proximity Sensor
> + *
> + * Copyright (c) 2019, Robert Eshleman.
> + *
> + * Datasheet: https://pdf-datasheet-datasheet.netdna-ssl.com/pdf-down/A/P/3/AP3216C-LITE-ON.pdf
> + *
> + * 7-bit I2C slave address 0x1E
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/bits.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +
> +#define AP3216C_DRV_NAME "ap3216c"
> +
> +/* register addresses */
> +#define AP3216C_SYS 0x0
> +#define AP3216C_INT_STATUS 0x01
> +#define AP3216C_INT_CLR 0x02
> +#define AP3216C_IR_DATA_LO 0x0A
> +#define AP3216C_IR_DATA_HI 0x0B
> +#define AP3216C_ALS_DATA_LO 0x0C
> +#define AP3216C_ALS_DATA_HI 0x0D
> +#define AP3216C_PS_DATA_LO 0x0E
> +#define AP3216C_PS_DATA_HI 0x0F
> +#define AP3216C_ALS_CFG 0x10
> +#define AP3216C_ALS_CALIB 0x19
> +#define AP3216C_ALS_LO_THR_LO 0x1A
> +#define AP3216C_ALS_LO_THR_HI 0x1B
> +#define AP3216C_ALS_HI_THR_LO 0x1C
> +#define AP3216C_ALS_HI_THR_HI 0x1D
> +#define AP3216C_PS_CFG 0x20
> +#define AP3216C_PS_CALIB_LO 0x28
> +#define AP3216C_PS_CALIB_HI 0x29
> +#define AP3216C_PS_LO_THR_LO 0x2A
> +#define AP3216C_PS_LO_THR_HI 0x2B
> +#define AP3216C_PS_HI_THR_LO 0x2C
> +#define AP3216C_PS_HI_THR_HI 0x2D
> +
> +/* SYS_MODE mask and config value */
> +#define AP3216C_SYS_MODE_ALS_PS GENMASK(1, 0)
> +#define AP3216C_SYS_MODE_ALS_ONLY 0x01
> +
> +/* INT_STATUS masks and config value */
> +#define AP3216C_INT_STATUS_ALS_MASK 1
> +#define AP3216C_INT_STATUS_PS_MASK (1 << 1)
> +#define AP3216C_INT_STATUS_CLR GENMASK(1, 0)
> +#define AP3216C_INT_CLR_MANUAL 1
> +
> +/* IR_DATA mask/shift */
> +#define AP3216C_IR_DATA_LO_MASK GENMASK(1, 0)
> +#define AP3216C_IR_DATA_HI_SHIFT 2
> +
> +/* ALS_DATA shift and fractional helper */
> +#define AP3216C_ALS_DATA_HI_SHIFT 8
> +#define AP3216C_ALS_DATA_DENOM 100000
> +
> +/* ALS_CALIB masks/shifts */
> +#define AP3216C_ALS_CALIB_INT_MASK GENMASK(7, 6)
> +#define AP3216C_ALS_CALIB_INT_SHIFT 6
> +#define AP3216C_ALS_CALIB_DEC_MASK GENMASK(5, 0)
> +
> +/* PS_DATA shifts/masks/bits */
> +#define AP3216C_PS_DATA_LO_MASK GENMASK(3, 0)
> +#define AP3216C_PS_DATA_LO_IR_OF BIT(6)
> +#define AP3216C_PS_DATA_HI_MASK GENMASK(5, 0)
> +#define AP3216C_PS_DATA_HI_SHIFT 4
> +#define AP3216C_PS_DATA_HI_IR_OF BIT(6)
> +
> +/* ALS_CFG masks */
> +#define AP3216C_ALS_CFG_GAIN_MASK GENMASK(5, 4)
> +
> +/* ALS_CALIB shifts */
> +#define AP3216C_ALS_CALIB_INT_SHIFT 6
> +
> +/* ALS_HI_THR masks and shifts */
> +#define AP3216C_ALS_HI_THR_LO_MASK GENMASK(7, 0)
> +#define AP3216C_ALS_HI_THR_HI_SHIFT 8
> +#define AP3216C_ALS_HI_THR_HI_MASK GENMASK(7, 4)
> +#define AP3216C_ALS_HI_THR_HI_SHIFT 8
> +
> +/* ALS_LO_THR masks and shifts */
> +#define AP3216C_ALS_LO_THR_LO_MASK GENMASK(3, 0)
> +#define AP3216C_ALS_LO_THR_HI_MASK GENMASK(7, 4)
> +#define AP3216C_ALS_LO_THR_HI_SHIFT 8
> +
> +/* PS_CFG reg mask/shift/bit values */
> +#define AP3216C_PS_CFG_GAIN_MASK GENMASK(3, 2)
> +#define AP3216C_PS_CFG_GAIN_SHIFT 2
> +#define AP3216C_PS_CFG_GAIN(val) \
> +       (1 << ((val & AP3216C_PS_CFG_GAIN_MASK) >> AP3216C_PS_CFG_GAIN_SHIFT))
> +
> +/* PS_LO and PS_HI shift/mask and multiplier values */
> +#define AP3216C_PS_LO_THR_HI_MULT 4
> +#define AP3216C_PS_HI_THR_HI_SHIFT 2
> +#define AP3216C_PS_HI_THR_HI_MULT 4
> +
> +/* PS_THR_LO and PS_THR_HI masks */
> +#define AP3216C_PS_THR_LO_MASK GENMASK(1, 0)
> +#define AP3216C_PS_THR_HI_MASK GENMASK(10, 2)
> +
> +/* PS_CALIB_HI and PS_CALIB_LO shift/mask/bit values */
> +#define AP3216C_PS_CALIB_HI_SHIFT 1
> +#define AP3216C_PS_CALIB_HI_MASK GENMASK(8, 1)
> +#define AP3216C_PS_CALIB_LO_MASK BIT(0)
> +
> +/* ALS fractional helper values */
> +#define AP3216C_ALS_INTEG_MULT 10000
> +#define AP3216C_ALS_FRACT_DIV 100
> +
> +/* ALS_CALIB min/max values (0.0 <= ALS_CALIB <= 3.984375) */
> +#define AP3216C_ALS_CALIB_INT_MIN 0
> +#define AP3216C_ALS_CALIB_DEC_MIN 0
> +#define AP3216C_ALS_CALIB_INT_MAX 3
> +#define AP3216C_ALS_CALIB_DEC_MAX 984375
> +
> +/* ALS_CALIB conversion denominator */
> +#define AP3216C_ALS_CALIB_DENOM 64
> +
> +#define AP3216C_IIO_MULT 1000000
> +
> +static u16 ap3216c_gain_array[] = {
> +       3500,  /* 0.3500 lux resolution */
> +        788,  /* 0.0788 lux resolution */
> +        197,  /* 0.0197 lux resolution */
> +         49,  /* 0.0049 lux resolution */
> +};
> +
> +struct ap3216c_data {
> +       struct regmap *regmap;
> +       struct i2c_client *client;
> +       bool als_thresh_en;
> +       bool prox_thresh_en;
> +};
> +
> +static const struct iio_event_spec ap3216c_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_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> +       },
> +};
> +
> +static const struct iio_chan_spec ap3216c_channels[] = {
> +       {
> +               .type = IIO_LIGHT,
> +               .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +                                     BIT(IIO_CHAN_INFO_CALIBSCALE),
> +               .event_spec = ap3216c_event_spec,
> +               .num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
> +       },
> +       {
> +               .type = IIO_PROXIMITY,
> +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +                                     BIT(IIO_CHAN_INFO_CALIBBIAS),
> +               .event_spec = ap3216c_event_spec,
> +               .num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
> +       },
> +       {
> +               .type = IIO_LIGHT,
> +               .modified = 1,
> +               .channel2 = IIO_MOD_LIGHT_IR,
> +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +       }
> +};
> +
> +static int ap3216c_write_event_value(struct iio_dev *indio_dev,
> +                                      const struct iio_chan_spec *chan,
> +                                      enum iio_event_type type,
> +                                      enum iio_event_direction dir,
> +                                      enum iio_event_info info,
> +                                      int val, int val2)
> +{
> +       struct ap3216c_data *data = iio_priv(indio_dev);
> +       int cfg, gain;
> +       int integral, fractional;
> +       int lo, hi;
> +       u16 thr;
> +       int ret;
> +
> +       switch (chan->type) {
> +       case IIO_LIGHT:
> +               ret = regmap_read(data->regmap, AP3216C_ALS_CFG, &cfg);
> +               if (ret < 0)
> +                       return ret;
> +
> +               gain = ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
> +               integral = val * AP3216C_ALS_INTEG_MULT / gain;
> +               fractional = val2 / AP3216C_ALS_FRACT_DIV / gain;
> +
> +               thr = integral + fractional;
> +
> +               switch (dir) {
> +               case IIO_EV_DIR_RISING:
> +                       ret = regmap_write(data->regmap, AP3216C_ALS_HI_THR_LO,
> +                                          thr & AP3216C_ALS_HI_THR_LO_MASK);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_write(data->regmap,
> +                                          AP3216C_ALS_HI_THR_HI,
> +                                          (thr & AP3216C_ALS_HI_THR_HI_MASK) >>
> +                                          AP3216C_ALS_HI_THR_HI_SHIFT);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       return ret;
> +
> +               case IIO_EV_DIR_FALLING:
> +                       ret = regmap_write(data->regmap,
> +                                          AP3216C_ALS_LO_THR_LO,
> +                                          thr & AP3216C_ALS_LO_THR_LO_MASK);
> +
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_write(data->regmap,
> +                                          AP3216C_ALS_LO_THR_HI,
> +                                          (thr & AP3216C_ALS_LO_THR_HI_MASK) >>
> +                                          AP3216C_ALS_LO_THR_HI_SHIFT);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       return ret;
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +               return -EINVAL;
> +
> +       case IIO_PROXIMITY:
> +               lo = val & AP3216C_PS_THR_LO_MASK;
> +               hi = (val & AP3216C_PS_THR_HI_MASK) >>
> +                       AP3216C_PS_HI_THR_HI_SHIFT;
> +
> +               switch (dir) {
> +               case IIO_EV_DIR_RISING:
> +                       ret = regmap_write(data->regmap,
> +                                          AP3216C_PS_HI_THR_LO, lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       return regmap_write(data->regmap,
> +                                           AP3216C_PS_HI_THR_HI, hi);
> +
> +               case IIO_EV_DIR_FALLING:
> +                       ret = regmap_write(data->regmap,
> +                                          AP3216C_PS_LO_THR_LO, lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       return regmap_write(data->regmap,
> +                                           AP3216C_PS_LO_THR_HI, hi);
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +
> +       default:
> +               return -EINVAL;
> +       }
> +       return -EINVAL;
> +}
> +
> +static int ap3216c_read_event_value(struct iio_dev *indio_dev,
> +                                    const struct iio_chan_spec *chan,
> +                                    enum iio_event_type type,
> +                                    enum iio_event_direction dir,
> +                                    enum iio_event_info info,
> +                                    int *val, int *val2)
> +{
> +       struct ap3216c_data *data = iio_priv(indio_dev);
> +       unsigned int lo, hi;
> +       int ret;
> +
> +       switch (chan->type) {
> +       case IIO_LIGHT:
> +               switch (dir) {
> +               case IIO_EV_DIR_RISING:
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_ALS_HI_THR_LO, &lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_ALS_HI_THR_HI, &hi);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       *val = (hi << AP3216C_ALS_HI_THR_HI_SHIFT) | lo;
> +                       return IIO_VAL_INT;
> +
> +               case IIO_EV_DIR_FALLING:
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_ALS_LO_THR_LO, &lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_ALS_LO_THR_HI, &hi);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       *val = (hi << AP3216C_ALS_LO_THR_HI_SHIFT) | lo;
> +                       return IIO_VAL_INT;
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +
> +       case IIO_PROXIMITY:
> +               switch (dir) {
> +               case IIO_EV_DIR_RISING:
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_PS_HI_THR_LO, &lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_PS_HI_THR_HI, &hi);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       *val = (hi * AP3216C_PS_HI_THR_HI_MULT) +
> +                               (lo & AP3216C_PS_THR_LO_MASK);
> +
> +                       return IIO_VAL_INT;
> +
> +               case IIO_EV_DIR_FALLING:
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_PS_LO_THR_LO, &lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_PS_LO_THR_HI, &hi);

Does the iio core serialize access to this function?
If so, disregard this comment.

If this function can be executed concurrently by multiple threads,
then the read to _LO and _HI needs to be serialized. Otherwise
you could end up reading nonsense values.

Datasheet says that when _LO is read, _HI is latched into a
temporary register. So imagine two threads reading out the value
concurrently:

Chip performs a conversion (call it 1)
Thread A reads _LO (1), chip stores _HI(1), thread pauses
The chip performs a new conversion (call it 2)
Thread B reads _LO (2), chip stores _HI(2)
Thread A wakes, reads _HI (2)

So now thread A has a _LO from conversion (1), and a _HI from
conversion (2) !!

I think you could solve this by placing a mutex around the two
reads, but see my comment below.

> +                       if (ret < 0)
> +                               return ret;
> +
> +                       *val = (hi * AP3216C_PS_LO_THR_HI_MULT) +
> +                               (lo & AP3216C_PS_THR_LO_MASK);
> +
> +                       return IIO_VAL_INT;
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static int ap3216c_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 ap3216c_data *data = iio_priv(indio_dev);
> +
> +       switch (chan->type) {
> +       case IIO_LIGHT:
> +               return data->als_thresh_en;
> +
> +       case IIO_PROXIMITY:
> +               return data->prox_thresh_en;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static int ap3216c_write_event_config(struct iio_dev *indio_dev,
> +                                    const struct iio_chan_spec *chan,
> +                                    enum iio_event_type type,
> +                                    enum iio_event_direction dir, int state)
> +{
> +       struct ap3216c_data *data = iio_priv(indio_dev);
> +
> +       switch (chan->type) {
> +       case IIO_LIGHT:
> +               data->als_thresh_en = state;
> +               return 0;
> +
> +       case IIO_PROXIMITY:
> +               data->prox_thresh_en = state;
> +               return 0;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static const struct regmap_config ap3216c_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .max_register = AP3216C_PS_HI_THR_HI,
> +};
> +
> +/**
> + * Returns integral part of decimal between 0.0 and 3.984275
> + */
> +static int ap3216c_als_calibscale_int(int val)
> +{
> +       if (val > AP3216C_ALS_CALIB_INT_MAX)
> +               return AP3216C_ALS_CALIB_INT_MAX;
> +
> +       if (val <= AP3216C_ALS_CALIB_INT_MIN)
> +               return AP3216C_ALS_CALIB_INT_MIN;
> +
> +       return val;
> +}
> +
> +/**
> + * Returns decimal part of decimal number between 0.0 and 3.984275
> + */
> +static int ap3216c_als_calibscale_dec(int val, int val2)
> +{
> +       /* Return max decimal if number exceeds calibscale max */
> +       if (val > AP3216C_ALS_CALIB_INT_MAX ||
> +           (val == AP3216C_ALS_CALIB_INT_MAX &&
> +            val2 > AP3216C_ALS_CALIB_DEC_MAX))
> +               return AP3216C_ALS_CALIB_DEC_MAX;
> +
> +       /* Floor the decimal if integral below minimum */
> +       if (val <= AP3216C_ALS_CALIB_INT_MIN)
> +               return AP3216C_ALS_CALIB_DEC_MIN;
> +
> +       /* Floor the decimal if decimal below minimum */
> +       if (val2 < AP3216C_ALS_CALIB_DEC_MIN)
> +               return AP3216C_ALS_CALIB_DEC_MIN;
> +
> +       /* Return max decimal if decimal above maximum */
> +       if (val2 > AP3216C_ALS_CALIB_DEC_MAX)
> +               return AP3216C_ALS_CALIB_DEC_MAX;
> +
> +       return val2;
> +}
> +
> +static int ap3216c_write_raw(struct iio_dev *indio_dev,
> +                            struct iio_chan_spec const *chan,
> +                            int val, int val2, long mask)
> +{
> +       struct ap3216c_data *data = iio_priv(indio_dev);
> +       int integral, decimal;
> +       int tmp;
> +       int ret;
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_CALIBSCALE:
> +               switch (chan->type) {
> +               case IIO_LIGHT:
> +                       integral = ap3216c_als_calibscale_int(val);
> +                       decimal = ap3216c_als_calibscale_dec(val, val2);
> +
> +                       tmp = integral << AP3216C_ALS_CALIB_INT_SHIFT;
> +
> +                       /*
> +                        * Reverse scaling by multiplying by the scaling
> +                        * denominator and dividing by IIO multiplier.
> +                        */
> +                       tmp |= (decimal * AP3216C_ALS_CALIB_DENOM) /
> +                               AP3216C_IIO_MULT;
> +
> +                       return regmap_write(data->regmap,
> +                                           AP3216C_ALS_CALIB, tmp);
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +
> +       case IIO_CHAN_INFO_CALIBBIAS:
> +               switch (chan->type) {
> +               case IIO_PROXIMITY:
> +                       ret = regmap_write(data->regmap, AP3216C_PS_CALIB_LO,
> +                                          val & AP3216C_PS_CALIB_LO_MASK);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       tmp = (val & AP3216C_PS_CALIB_HI_MASK) >>
> +                               AP3216C_PS_CALIB_HI_SHIFT;
> +
> +                       return regmap_write(data->regmap,
> +                                           AP3216C_PS_CALIB_HI, tmp);
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +       default:
> +               return -EINVAL;
> +       }
> +       return -EINVAL;
> +}
> +
> +static int ap3216c_read_raw(struct iio_dev *indio_dev,
> +                               struct iio_chan_spec const *chan,
> +                               int *val, int *val2, long mask)
> +{
> +       struct ap3216c_data *data = iio_priv(indio_dev);
> +       int lo, hi;
> +       int cfg, tmp;
> +       int ret;
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_CALIBSCALE:
> +               switch (chan->type) {
> +               case IIO_LIGHT:
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_ALS_CALIB, &tmp);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       /* Scale is multiplied by 1/64 */
> +                       *val = tmp;
> +                       *val2 = AP3216C_ALS_CALIB_DENOM;
> +                       return IIO_VAL_FRACTIONAL;
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +
> +       case IIO_CHAN_INFO_CALIBBIAS:
> +               switch (chan->type) {
> +               case IIO_PROXIMITY:
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_PS_CALIB_LO, &lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_PS_CALIB_HI, &hi);
> +                       if (ret < 0)
> +                               return ret;
> +
> +
> +                       *val = lo | (hi << AP3216C_PS_CALIB_HI_SHIFT);
> +                       return IIO_VAL_INT;
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +
> +       case IIO_CHAN_INFO_PROCESSED:
> +               switch (chan->type) {
> +               case IIO_LIGHT:
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_ALS_DATA_LO, &lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_ALS_DATA_HI, &hi);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_ALS_CFG, &cfg);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       tmp = (hi << AP3216C_ALS_DATA_HI_SHIFT) | lo;
> +                       tmp *= ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
> +
> +                       *val = tmp;
> +                       *val2 = AP3216C_ALS_DATA_DENOM;
> +
> +                       return IIO_VAL_FRACTIONAL;
> +               default:
> +                       return -EINVAL;
> +               }
> +       case IIO_CHAN_INFO_RAW:
> +               switch (chan->type) {
> +               case IIO_LIGHT:
> +                       if (chan->channel2 != IIO_MOD_LIGHT_IR)
> +                               return -EINVAL;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_IR_DATA_LO, &lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_IR_DATA_HI, &hi);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       tmp = lo & AP3216C_IR_DATA_LO_MASK;
> +                       tmp |= hi << AP3216C_IR_DATA_HI_SHIFT;
> +                       *val = tmp;
> +
> +                       return IIO_VAL_INT;
> +
> +               case IIO_PROXIMITY:
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_PS_DATA_LO, &lo);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_PS_DATA_HI, &hi);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ret = regmap_read(data->regmap,
> +                                         AP3216C_PS_CFG, &cfg);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       tmp = lo & AP3216C_PS_DATA_LO_MASK;
> +                       tmp |= (hi & AP3216C_PS_DATA_HI_MASK) <<
> +                               AP3216C_PS_DATA_HI_SHIFT;
> +                       *val = tmp * AP3216C_PS_CFG_GAIN(cfg);
> +
> +                       return IIO_VAL_INT;
> +               default:
> +                       return -EINVAL;
> +               }
> +       default:
> +               return -EINVAL;
> +       }
> +       return -EINVAL;
> +}
> +
> +static const struct iio_info ap3216c_info = {
> +       .read_raw = ap3216c_read_raw,
> +       .write_raw = ap3216c_write_raw,
> +       .read_event_value = ap3216c_read_event_value,
> +       .write_event_value = ap3216c_write_event_value,
> +       .read_event_config = ap3216c_read_event_config,
> +       .write_event_config = ap3216c_write_event_config,
> +};
> +
> +static int ap3216c_clear_int(struct ap3216c_data *data)
> +{
> +       return regmap_write(data->regmap,
> +                           AP3216C_INT_STATUS, AP3216C_INT_STATUS_CLR);
> +}
> +
> +static irqreturn_t ap3216c_event_handler(int irq, void *p)
> +{
> +       struct iio_dev *indio_dev = p;
> +       struct ap3216c_data *data = iio_priv(indio_dev);
> +       int status;
> +       s64 timestamp;
> +       int ret;
> +
> +       ret = regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
> +       if (ret) {
> +               /*
> +                * Without being able to guarantee the interrupt came from
> +                * this device, we must return IRQ_HANDLED instead of
> +                * IRQ_NONE.
> +                */

Do you know why? I'm not saying this is wrong, just curious to know why.
Because if the chip becomes unresponsive, now the driver will consume
all interrupts on the shared interrupt line, right ?

> +               dev_err(&data->client->dev, "error reading IRQ status\n");
> +               return IRQ_HANDLED;
> +       }
> +
> +       /* The IRQ was not from this device */
> +       if (!status)
> +               return IRQ_NONE;
> +
> +       timestamp = iio_get_time_ns(indio_dev);
> +       if ((status & AP3216C_INT_STATUS_PS_MASK) && data->prox_thresh_en)
> +               iio_push_event(indio_dev,
> +                              IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> +                                                   IIO_EV_TYPE_THRESH,
> +                                                   IIO_EV_DIR_EITHER),
> +                              timestamp);
> +
> +       if ((status & AP3216C_INT_STATUS_ALS_MASK) && data->als_thresh_en)
> +               iio_push_event(indio_dev,
> +                              IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +                                                   IIO_EV_TYPE_THRESH,
> +                                                   IIO_EV_DIR_EITHER),
> +                              timestamp);

I'm a complete newbie re. this driver and the iio subsystem in general, but I
suspect that events could be lost here:

Example flow:
1. device generates ALS data, writes it to the register, asserts interrupt
2. this event handler executes, it's a threaded event handler, so may be
interleaved with userspace threads
3. this event handler calls iio_push_event() and is then paused by the scheduler
4. a userspace thread monitoring the iio interface reacts to to the event,
and reads the ALS data
5. the device generates _new_ ALS data, interrupt already asserted
6. the threaded event handler continues executing, clears the interrupt

Result: new ALS data is available, but iio did not get an event ?

If this is indeed an issue, you might be able to address this by locking
the interrupt handler and ALS/PS read with a mutex ?

But of course if you do that, you might end up with an interrupt / event
for a value you've already read out. That's fine only if iio core is ok with
spurious events.

> +
> +       ret = ap3216c_clear_int(data);
> +       if (ret < 0)
> +               dev_err(&data->client->dev, "error clearing IRQ\n");
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int ap3216c_probe(struct i2c_client *client,
> +                         const struct i2c_device_id *id)
> +{
> +       struct ap3216c_data *data;
> +       struct iio_dev *indio_dev;
> +       int ret;
> +
> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       data = iio_priv(indio_dev);
> +       data->client = client;
> +       indio_dev->dev.parent = &client->dev;
> +       indio_dev->info = &ap3216c_info;
> +       indio_dev->name = AP3216C_DRV_NAME;
> +       indio_dev->channels = ap3216c_channels;
> +       indio_dev->num_channels = ARRAY_SIZE(ap3216c_channels);
> +
> +       data->regmap = devm_regmap_init_i2c(client, &ap3216c_regmap_config);
> +       if (IS_ERR(data->regmap)) {
> +               dev_err(&client->dev, "Failed to allocate register map\n");
> +               return PTR_ERR(data->regmap);
> +       }
> +
> +       /* Default to thresh events disabled */
> +       data->als_thresh_en = false;
> +       data->prox_thresh_en = false;
> +
> +       /*
> +        * Require that that the interrupt is cleared only when the INT
> +        * register is written to, instead of when data is read.  This
> +        * prevents the interrupt from falsely reporting IRQ_NONE.
> +        */
> +       ret = regmap_write(data->regmap,
> +                          AP3216C_INT_CLR, AP3216C_INT_CLR_MANUAL);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Before setting up IRQ, clear any stale interrupt */
> +       ret = ap3216c_clear_int(data);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (client->irq) {
> +               ret = devm_request_threaded_irq(&client->dev, client->irq,
> +                                               NULL, ap3216c_event_handler,
> +                                               IRQF_TRIGGER_FALLING |
> +                                               IRQF_SHARED | IRQF_ONESHOT,
> +                                               client->name, indio_dev);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /* Enable ALS and PS+IR */
> +       ret = regmap_write(data->regmap, AP3216C_SYS, AP3216C_SYS_MODE_ALS_PS);
> +       if (ret < 0)
> +               return ret;
> +
> +       return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct of_device_id ap3216c_of_match[] = {
> +       { .compatible = "liteon,ap3216c", },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, ap3216c_of_match);
> +
> +static const struct i2c_device_id ap3216c_id[] = {
> +       {"ap3216c", 0},
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, ap3216c_id);
> +
> +static struct i2c_driver ap3216c_driver = {
> +       .driver = {
> +               .name   = AP3216C_DRV_NAME,
> +       },
> +       .probe          = ap3216c_probe,
> +       .id_table       = ap3216c_id,
> +};
> +module_i2c_driver(ap3216c_driver);
> +
> +MODULE_AUTHOR("Robert Eshleman <bobbyeshleman@gmail.com>");
> +MODULE_DESCRIPTION("APC3216C Ambient Light and Proximity Sensor");
> +MODULE_LICENSE("GPL v2");
> --
> 2.20.1
>

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Jonathan Cameron]

On Mon, 11 Feb 2019 14:09:12 -0500
Sven Van Asbroeck <thesven73@gmail.com> wrote:

> Hi Robert,
> 
> On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman <bobbyeshleman@gmail.com> wrote:
> >
> > This patch adds support for the ap3216c ambient light and proximity
> > sensor.  
> 
> Comments below.
> 
Follow up inline...

Mostly looks good to me, but a few places to tidy up.

Thanks,

Jonathan

> >
> > Supported features include:
> >
> > * Illuminance (lux)
> > * Proximity (raw)
> > * IR (raw)
> > * Rising/falling threshold events for illuminance and proximity
> > * Calibration scale for illuminance
> > * Calibration bias for proximity
> >
> > Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
> > ---
> >  drivers/iio/light/Kconfig   |  11 +
> >  drivers/iio/light/Makefile  |   1 +
> >  drivers/iio/light/ap3216c.c | 793 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 805 insertions(+)
> >  create mode 100644 drivers/iio/light/ap3216c.c
> >
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 36f458433480..74688d19beb1 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -41,6 +41,17 @@ config AL3320A
> >          To compile this driver as a module, choose M here: the
> >          module will be called al3320a.
> >
> > +config AP3216C
> > +       tristate "AP3216C Ambient Light and Proximity sensor"
> > +       select REGMAP_I2C
> > +       depends on I2C
> > +       help
> > +         Say Y here to build a driver for the AP3216C Ambient Light and
> > +         Proximity sensor.
> > +
> > +         To compile this driver as a module, choose M here: the
> > +         module will be called ap3216c.
> > +
> >  config APDS9300
> >         tristate "APDS9300 ambient light sensor"
> >         depends on I2C
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index 286bf3975372..7d2f8fa0f30d 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -7,6 +7,7 @@
> >  obj-$(CONFIG_ACPI_ALS)         += acpi-als.o
> >  obj-$(CONFIG_ADJD_S311)                += adjd_s311.o
> >  obj-$(CONFIG_AL3320A)          += al3320a.o
> > +obj-$(CONFIG_AP3216C)          += ap3216c.o
> >  obj-$(CONFIG_APDS9300)         += apds9300.o
> >  obj-$(CONFIG_APDS9960)         += apds9960.o
> >  obj-$(CONFIG_BH1750)           += bh1750.o
> > diff --git a/drivers/iio/light/ap3216c.c b/drivers/iio/light/ap3216c.c
> > new file mode 100644
> > index 000000000000..12802b45c01c
> > --- /dev/null
> > +++ b/drivers/iio/light/ap3216c.c
> > @@ -0,0 +1,793 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * AP3216C Ambient Light and Infrared Proximity Sensor
> > + *
> > + * Copyright (c) 2019, Robert Eshleman.
> > + *
> > + * Datasheet: https://pdf-datasheet-datasheet.netdna-ssl.com/pdf-down/A/P/3/AP3216C-LITE-ON.pdf
> > + *
> > + * 7-bit I2C slave address 0x1E
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/bits.h>
> > +#include <linux/regmap.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/events.h>
> > +
> > +#define AP3216C_DRV_NAME "ap3216c"
> > +
> > +/* register addresses */
> > +#define AP3216C_SYS 0x0
> > +#define AP3216C_INT_STATUS 0x01
> > +#define AP3216C_INT_CLR 0x02
> > +#define AP3216C_IR_DATA_LO 0x0A
> > +#define AP3216C_IR_DATA_HI 0x0B
> > +#define AP3216C_ALS_DATA_LO 0x0C
> > +#define AP3216C_ALS_DATA_HI 0x0D
> > +#define AP3216C_PS_DATA_LO 0x0E
> > +#define AP3216C_PS_DATA_HI 0x0F
> > +#define AP3216C_ALS_CFG 0x10
> > +#define AP3216C_ALS_CALIB 0x19
> > +#define AP3216C_ALS_LO_THR_LO 0x1A
> > +#define AP3216C_ALS_LO_THR_HI 0x1B
> > +#define AP3216C_ALS_HI_THR_LO 0x1C
> > +#define AP3216C_ALS_HI_THR_HI 0x1D
> > +#define AP3216C_PS_CFG 0x20
> > +#define AP3216C_PS_CALIB_LO 0x28
> > +#define AP3216C_PS_CALIB_HI 0x29
> > +#define AP3216C_PS_LO_THR_LO 0x2A
> > +#define AP3216C_PS_LO_THR_HI 0x2B
> > +#define AP3216C_PS_HI_THR_LO 0x2C
> > +#define AP3216C_PS_HI_THR_HI 0x2D
> > +
> > +/* SYS_MODE mask and config value */
> > +#define AP3216C_SYS_MODE_ALS_PS GENMASK(1, 0)
> > +#define AP3216C_SYS_MODE_ALS_ONLY 0x01
> > +
> > +/* INT_STATUS masks and config value */
> > +#define AP3216C_INT_STATUS_ALS_MASK 1
> > +#define AP3216C_INT_STATUS_PS_MASK (1 << 1)
> > +#define AP3216C_INT_STATUS_CLR GENMASK(1, 0)
> > +#define AP3216C_INT_CLR_MANUAL 1
> > +
> > +/* IR_DATA mask/shift */
> > +#define AP3216C_IR_DATA_LO_MASK GENMASK(1, 0)
> > +#define AP3216C_IR_DATA_HI_SHIFT 2
> > +
> > +/* ALS_DATA shift and fractional helper */
> > +#define AP3216C_ALS_DATA_HI_SHIFT 8
> > +#define AP3216C_ALS_DATA_DENOM 100000
> > +
> > +/* ALS_CALIB masks/shifts */
> > +#define AP3216C_ALS_CALIB_INT_MASK GENMASK(7, 6)
> > +#define AP3216C_ALS_CALIB_INT_SHIFT 6
> > +#define AP3216C_ALS_CALIB_DEC_MASK GENMASK(5, 0)
> > +
> > +/* PS_DATA shifts/masks/bits */
> > +#define AP3216C_PS_DATA_LO_MASK GENMASK(3, 0)
> > +#define AP3216C_PS_DATA_LO_IR_OF BIT(6)
> > +#define AP3216C_PS_DATA_HI_MASK GENMASK(5, 0)
> > +#define AP3216C_PS_DATA_HI_SHIFT 4
> > +#define AP3216C_PS_DATA_HI_IR_OF BIT(6)
> > +
> > +/* ALS_CFG masks */
> > +#define AP3216C_ALS_CFG_GAIN_MASK GENMASK(5, 4)
> > +
> > +/* ALS_CALIB shifts */
> > +#define AP3216C_ALS_CALIB_INT_SHIFT 6
> > +
> > +/* ALS_HI_THR masks and shifts */
> > +#define AP3216C_ALS_HI_THR_LO_MASK GENMASK(7, 0)
> > +#define AP3216C_ALS_HI_THR_HI_SHIFT 8
> > +#define AP3216C_ALS_HI_THR_HI_MASK GENMASK(7, 4)
> > +#define AP3216C_ALS_HI_THR_HI_SHIFT 8
> > +
> > +/* ALS_LO_THR masks and shifts */
> > +#define AP3216C_ALS_LO_THR_LO_MASK GENMASK(3, 0)
> > +#define AP3216C_ALS_LO_THR_HI_MASK GENMASK(7, 4)
> > +#define AP3216C_ALS_LO_THR_HI_SHIFT 8
> > +
> > +/* PS_CFG reg mask/shift/bit values */
> > +#define AP3216C_PS_CFG_GAIN_MASK GENMASK(3, 2)
> > +#define AP3216C_PS_CFG_GAIN_SHIFT 2
> > +#define AP3216C_PS_CFG_GAIN(val) \
> > +       (1 << ((val & AP3216C_PS_CFG_GAIN_MASK) >> AP3216C_PS_CFG_GAIN_SHIFT))
> > +
> > +/* PS_LO and PS_HI shift/mask and multiplier values */
> > +#define AP3216C_PS_LO_THR_HI_MULT 4
> > +#define AP3216C_PS_HI_THR_HI_SHIFT 2
> > +#define AP3216C_PS_HI_THR_HI_MULT 4
> > +
> > +/* PS_THR_LO and PS_THR_HI masks */
> > +#define AP3216C_PS_THR_LO_MASK GENMASK(1, 0)
> > +#define AP3216C_PS_THR_HI_MASK GENMASK(10, 2)
> > +
> > +/* PS_CALIB_HI and PS_CALIB_LO shift/mask/bit values */
> > +#define AP3216C_PS_CALIB_HI_SHIFT 1
> > +#define AP3216C_PS_CALIB_HI_MASK GENMASK(8, 1)
> > +#define AP3216C_PS_CALIB_LO_MASK BIT(0)
> > +
> > +/* ALS fractional helper values */
> > +#define AP3216C_ALS_INTEG_MULT 10000
> > +#define AP3216C_ALS_FRACT_DIV 100
> > +
> > +/* ALS_CALIB min/max values (0.0 <= ALS_CALIB <= 3.984375) */
> > +#define AP3216C_ALS_CALIB_INT_MIN 0
> > +#define AP3216C_ALS_CALIB_DEC_MIN 0
> > +#define AP3216C_ALS_CALIB_INT_MAX 3
> > +#define AP3216C_ALS_CALIB_DEC_MAX 984375
> > +
> > +/* ALS_CALIB conversion denominator */
> > +#define AP3216C_ALS_CALIB_DENOM 64
> > +
> > +#define AP3216C_IIO_MULT 1000000
> > +
> > +static u16 ap3216c_gain_array[] = {
> > +       3500,  /* 0.3500 lux resolution */
> > +        788,  /* 0.0788 lux resolution */
> > +        197,  /* 0.0197 lux resolution */
> > +         49,  /* 0.0049 lux resolution */
> > +};
> > +
> > +struct ap3216c_data {
> > +       struct regmap *regmap;
> > +       struct i2c_client *client;
> > +       bool als_thresh_en;
> > +       bool prox_thresh_en;
> > +};
> > +
> > +static const struct iio_event_spec ap3216c_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_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > +       },
> > +};
> > +
> > +static const struct iio_chan_spec ap3216c_channels[] = {
> > +       {
> > +               .type = IIO_LIGHT,
> > +               .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > +                                     BIT(IIO_CHAN_INFO_CALIBSCALE),
> > +               .event_spec = ap3216c_event_spec,
> > +               .num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
> > +       },
> > +       {
> > +               .type = IIO_PROXIMITY,
> > +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +                                     BIT(IIO_CHAN_INFO_CALIBBIAS),
> > +               .event_spec = ap3216c_event_spec,
> > +               .num_event_specs = ARRAY_SIZE(ap3216c_event_spec),
> > +       },
> > +       {
> > +               .type = IIO_LIGHT,
> > +               .modified = 1,
> > +               .channel2 = IIO_MOD_LIGHT_IR,
> > +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +       }
> > +};
> > +
> > +static int ap3216c_write_event_value(struct iio_dev *indio_dev,
> > +                                      const struct iio_chan_spec *chan,
> > +                                      enum iio_event_type type,
> > +                                      enum iio_event_direction dir,
> > +                                      enum iio_event_info info,
> > +                                      int val, int val2)
> > +{
> > +       struct ap3216c_data *data = iio_priv(indio_dev);
> > +       int cfg, gain;
> > +       int integral, fractional;
> > +       int lo, hi;
> > +       u16 thr;
> > +       int ret;
> > +
> > +       switch (chan->type) {
> > +       case IIO_LIGHT:
> > +               ret = regmap_read(data->regmap, AP3216C_ALS_CFG, &cfg);
> > +               if (ret < 0)
> > +                       return ret;
> > +
> > +               gain = ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
> > +               integral = val * AP3216C_ALS_INTEG_MULT / gain;
> > +               fractional = val2 / AP3216C_ALS_FRACT_DIV / gain;
> > +
> > +               thr = integral + fractional;
> > +
> > +               switch (dir) {
> > +               case IIO_EV_DIR_RISING:
> > +                       ret = regmap_write(data->regmap, AP3216C_ALS_HI_THR_LO,
> > +                                          thr & AP3216C_ALS_HI_THR_LO_MASK);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_write(data->regmap,
> > +                                          AP3216C_ALS_HI_THR_HI,
> > +                                          (thr & AP3216C_ALS_HI_THR_HI_MASK) >>
> > +                                          AP3216C_ALS_HI_THR_HI_SHIFT);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       return ret;
> > +
> > +               case IIO_EV_DIR_FALLING:
> > +                       ret = regmap_write(data->regmap,
> > +                                          AP3216C_ALS_LO_THR_LO,
> > +                                          thr & AP3216C_ALS_LO_THR_LO_MASK);
> > +
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_write(data->regmap,
> > +                                          AP3216C_ALS_LO_THR_HI,
> > +                                          (thr & AP3216C_ALS_LO_THR_HI_MASK) >>
> > +                                          AP3216C_ALS_LO_THR_HI_SHIFT);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       return ret;
> > +
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +               return -EINVAL;
> > +
> > +       case IIO_PROXIMITY:
> > +               lo = val & AP3216C_PS_THR_LO_MASK;
> > +               hi = (val & AP3216C_PS_THR_HI_MASK) >>
> > +                       AP3216C_PS_HI_THR_HI_SHIFT;
> > +
> > +               switch (dir) {
> > +               case IIO_EV_DIR_RISING:
> > +                       ret = regmap_write(data->regmap,
> > +                                          AP3216C_PS_HI_THR_LO, lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       return regmap_write(data->regmap,
> > +                                           AP3216C_PS_HI_THR_HI, hi);
> > +
> > +               case IIO_EV_DIR_FALLING:
> > +                       ret = regmap_write(data->regmap,
> > +                                          AP3216C_PS_LO_THR_LO, lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       return regmap_write(data->regmap,
> > +                                           AP3216C_PS_LO_THR_HI, hi);
> > +
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +       return -EINVAL;
> > +}
> > +
> > +static int ap3216c_read_event_value(struct iio_dev *indio_dev,
> > +                                    const struct iio_chan_spec *chan,
> > +                                    enum iio_event_type type,
> > +                                    enum iio_event_direction dir,
> > +                                    enum iio_event_info info,
> > +                                    int *val, int *val2)
> > +{
> > +       struct ap3216c_data *data = iio_priv(indio_dev);
> > +       unsigned int lo, hi;
> > +       int ret;
> > +
> > +       switch (chan->type) {
> > +       case IIO_LIGHT:
> > +               switch (dir) {
> > +               case IIO_EV_DIR_RISING:
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_ALS_HI_THR_LO, &lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_ALS_HI_THR_HI, &hi);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       *val = (hi << AP3216C_ALS_HI_THR_HI_SHIFT) | lo;
> > +                       return IIO_VAL_INT;
> > +
> > +               case IIO_EV_DIR_FALLING:
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_ALS_LO_THR_LO, &lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_ALS_LO_THR_HI, &hi);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       *val = (hi << AP3216C_ALS_LO_THR_HI_SHIFT) | lo;
> > +                       return IIO_VAL_INT;
> > +
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +
> > +       case IIO_PROXIMITY:
> > +               switch (dir) {
> > +               case IIO_EV_DIR_RISING:
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_PS_HI_THR_LO, &lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_PS_HI_THR_HI, &hi);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       *val = (hi * AP3216C_PS_HI_THR_HI_MULT) +
> > +                               (lo & AP3216C_PS_THR_LO_MASK);
> > +
> > +                       return IIO_VAL_INT;
> > +
> > +               case IIO_EV_DIR_FALLING:
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_PS_LO_THR_LO, &lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_PS_LO_THR_HI, &hi);  
> 
> Does the iio core serialize access to this function?

It doesn't.

> If so, disregard this comment.
> 
> If this function can be executed concurrently by multiple threads,
> then the read to _LO and _HI needs to be serialized. Otherwise
> you could end up reading nonsense values.
> 
> Datasheet says that when _LO is read, _HI is latched into a
> temporary register. So imagine two threads reading out the value
> concurrently:
> 
> Chip performs a conversion (call it 1)
> Thread A reads _LO (1), chip stores _HI(1), thread pauses
> The chip performs a new conversion (call it 2)
> Thread B reads _LO (2), chip stores _HI(2)
> Thread A wakes, reads _HI (2)
> 
> So now thread A has a _LO from conversion (1), and a _HI from
> conversion (2) !!
> 
> I think you could solve this by placing a mutex around the two
> reads, but see my comment below.

Agreed.  Or potentially just use regmap_bulk_read and rely on 
the regmap internal locking to do it for you.

> 
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       *val = (hi * AP3216C_PS_LO_THR_HI_MULT) +
> > +                               (lo & AP3216C_PS_THR_LO_MASK);
> > +
> > +                       return IIO_VAL_INT;
> > +
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static int ap3216c_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 ap3216c_data *data = iio_priv(indio_dev);
> > +
> > +       switch (chan->type) {
> > +       case IIO_LIGHT:
> > +               return data->als_thresh_en;
> > +
> > +       case IIO_PROXIMITY:
> > +               return data->prox_thresh_en;
> > +
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static int ap3216c_write_event_config(struct iio_dev *indio_dev,
> > +                                    const struct iio_chan_spec *chan,
> > +                                    enum iio_event_type type,
> > +                                    enum iio_event_direction dir, int state)
> > +{
> > +       struct ap3216c_data *data = iio_priv(indio_dev);
> > +
> > +       switch (chan->type) {
> > +       case IIO_LIGHT:
> > +               data->als_thresh_en = state;
> > +               return 0;
> > +
> > +       case IIO_PROXIMITY:
> > +               data->prox_thresh_en = state;
> > +               return 0;
> > +
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static const struct regmap_config ap3216c_regmap_config = {
> > +       .reg_bits = 8,
> > +       .val_bits = 8,
> > +       .max_register = AP3216C_PS_HI_THR_HI,
> > +};
> > +
> > +/**
> > + * Returns integral part of decimal between 0.0 and 3.984275
> > + */
> > +static int ap3216c_als_calibscale_int(int val)
> > +{
> > +       if (val > AP3216C_ALS_CALIB_INT_MAX)
> > +               return AP3216C_ALS_CALIB_INT_MAX;
> > +
> > +       if (val <= AP3216C_ALS_CALIB_INT_MIN)
> > +               return AP3216C_ALS_CALIB_INT_MIN;
> > +
> > +       return val;
> > +}
> > +
> > +/**
> > + * Returns decimal part of decimal number between 0.0 and 3.984275
> > + */
> > +static int ap3216c_als_calibscale_dec(int val, int val2)
> > +{
> > +       /* Return max decimal if number exceeds calibscale max */
> > +       if (val > AP3216C_ALS_CALIB_INT_MAX ||
> > +           (val == AP3216C_ALS_CALIB_INT_MAX &&
> > +            val2 > AP3216C_ALS_CALIB_DEC_MAX))
> > +               return AP3216C_ALS_CALIB_DEC_MAX;
> > +
> > +       /* Floor the decimal if integral below minimum */
> > +       if (val <= AP3216C_ALS_CALIB_INT_MIN)
> > +               return AP3216C_ALS_CALIB_DEC_MIN;
> > +
> > +       /* Floor the decimal if decimal below minimum */
> > +       if (val2 < AP3216C_ALS_CALIB_DEC_MIN)
> > +               return AP3216C_ALS_CALIB_DEC_MIN;
> > +
> > +       /* Return max decimal if decimal above maximum */
> > +       if (val2 > AP3216C_ALS_CALIB_DEC_MAX)
> > +               return AP3216C_ALS_CALIB_DEC_MAX;
> > +
> > +       return val2;
> > +}
> > +
> > +static int ap3216c_write_raw(struct iio_dev *indio_dev,
> > +                            struct iio_chan_spec const *chan,
> > +                            int val, int val2, long mask)
> > +{
> > +       struct ap3216c_data *data = iio_priv(indio_dev);
> > +       int integral, decimal;
> > +       int tmp;
> > +       int ret;
> > +
> > +       switch (mask) {
> > +       case IIO_CHAN_INFO_CALIBSCALE:
> > +               switch (chan->type) {
> > +               case IIO_LIGHT:
> > +                       integral = ap3216c_als_calibscale_int(val);
> > +                       decimal = ap3216c_als_calibscale_dec(val, val2);
> > +
> > +                       tmp = integral << AP3216C_ALS_CALIB_INT_SHIFT;
> > +
> > +                       /*
> > +                        * Reverse scaling by multiplying by the scaling
> > +                        * denominator and dividing by IIO multiplier.
> > +                        */
> > +                       tmp |= (decimal * AP3216C_ALS_CALIB_DENOM) /
> > +                               AP3216C_IIO_MULT;
> > +
> > +                       return regmap_write(data->regmap,
> > +                                           AP3216C_ALS_CALIB, tmp);
> > +
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +
> > +       case IIO_CHAN_INFO_CALIBBIAS:
> > +               switch (chan->type) {
> > +               case IIO_PROXIMITY:
> > +                       ret = regmap_write(data->regmap, AP3216C_PS_CALIB_LO,
> > +                                          val & AP3216C_PS_CALIB_LO_MASK);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       tmp = (val & AP3216C_PS_CALIB_HI_MASK) >>
> > +                               AP3216C_PS_CALIB_HI_SHIFT;
regmap_bulk_write perhaps?
> > +
> > +                       return regmap_write(data->regmap,
> > +                                           AP3216C_PS_CALIB_HI, tmp);
> > +
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +       default:
> > +               return -EINVAL;
> > +       }

This final return should be unneeded.

> > +       return -EINVAL;
> > +}
> > +
> > +static int ap3216c_read_raw(struct iio_dev *indio_dev,
> > +                               struct iio_chan_spec const *chan,
> > +                               int *val, int *val2, long mask)
> > +{
> > +       struct ap3216c_data *data = iio_priv(indio_dev);
> > +       int lo, hi;
> > +       int cfg, tmp;
> > +       int ret;
> > +
> > +       switch (mask) {
> > +       case IIO_CHAN_INFO_CALIBSCALE:
> > +               switch (chan->type) {
> > +               case IIO_LIGHT:
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_ALS_CALIB, &tmp);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       /* Scale is multiplied by 1/64 */
> > +                       *val = tmp;
> > +                       *val2 = AP3216C_ALS_CALIB_DENOM;
> > +                       return IIO_VAL_FRACTIONAL;
> > +
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +
> > +       case IIO_CHAN_INFO_CALIBBIAS:
> > +               switch (chan->type) {
> > +               case IIO_PROXIMITY:

regmap_read_bulk should get you both in one call with concurrency protection
as an added bonus.

> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_PS_CALIB_LO, &lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_PS_CALIB_HI, &hi);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +
> > +                       *val = lo | (hi << AP3216C_PS_CALIB_HI_SHIFT);  

lo should probably be masked. Bits are documented as reserved, not 0 as
far as I can see.

> > +                       return IIO_VAL_INT;
> > +
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +
> > +       case IIO_CHAN_INFO_PROCESSED:
> > +               switch (chan->type) {
> > +               case IIO_LIGHT:
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_ALS_DATA_LO, &lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_ALS_DATA_HI, &hi);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_ALS_CFG, &cfg);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       tmp = (hi << AP3216C_ALS_DATA_HI_SHIFT) | lo;
> > +                       tmp *= ap3216c_gain_array[cfg & AP3216C_ALS_CFG_GAIN_MASK];
> > +
> > +                       *val = tmp;
> > +                       *val2 = AP3216C_ALS_DATA_DENOM;
> > +
> > +                       return IIO_VAL_FRACTIONAL;
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +       case IIO_CHAN_INFO_RAW:
> > +               switch (chan->type) {
> > +               case IIO_LIGHT:
> > +                       if (chan->channel2 != IIO_MOD_LIGHT_IR)
> > +                               return -EINVAL;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_IR_DATA_LO, &lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_IR_DATA_HI, &hi);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       tmp = lo & AP3216C_IR_DATA_LO_MASK;
> > +                       tmp |= hi << AP3216C_IR_DATA_HI_SHIFT;
> > +                       *val = tmp;
> > +
> > +                       return IIO_VAL_INT;
> > +
> > +               case IIO_PROXIMITY:
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_PS_DATA_LO, &lo);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_PS_DATA_HI, &hi);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       ret = regmap_read(data->regmap,
> > +                                         AP3216C_PS_CFG, &cfg);
> > +                       if (ret < 0)
> > +                               return ret;
> > +
> > +                       tmp = lo & AP3216C_PS_DATA_LO_MASK;
> > +                       tmp |= (hi & AP3216C_PS_DATA_HI_MASK) <<
> > +                               AP3216C_PS_DATA_HI_SHIFT;
> > +                       *val = tmp * AP3216C_PS_CFG_GAIN(cfg);
> > +
> > +                       return IIO_VAL_INT;
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +       return -EINVAL;
> > +}
> > +
> > +static const struct iio_info ap3216c_info = {
> > +       .read_raw = ap3216c_read_raw,
> > +       .write_raw = ap3216c_write_raw,
> > +       .read_event_value = ap3216c_read_event_value,
> > +       .write_event_value = ap3216c_write_event_value,
> > +       .read_event_config = ap3216c_read_event_config,
> > +       .write_event_config = ap3216c_write_event_config,
> > +};
> > +
> > +static int ap3216c_clear_int(struct ap3216c_data *data)
> > +{
> > +       return regmap_write(data->regmap,
> > +                           AP3216C_INT_STATUS, AP3216C_INT_STATUS_CLR);
> > +}
> > +
> > +static irqreturn_t ap3216c_event_handler(int irq, void *p)
> > +{
> > +       struct iio_dev *indio_dev = p;
> > +       struct ap3216c_data *data = iio_priv(indio_dev);
> > +       int status;
> > +       s64 timestamp;
> > +       int ret;
> > +
> > +       ret = regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
> > +       if (ret) {
> > +               /*
> > +                * Without being able to guarantee the interrupt came from
> > +                * this device, we must return IRQ_HANDLED instead of
> > +                * IRQ_NONE.
> > +                */  
> 
> Do you know why? I'm not saying this is wrong, just curious to know why.
> Because if the chip becomes unresponsive, now the driver will consume
> all interrupts on the shared interrupt line, right ?
> 
> > +               dev_err(&data->client->dev, "error reading IRQ status\n");
> > +               return IRQ_HANDLED;
> > +       }
> > +
> > +       /* The IRQ was not from this device */
> > +       if (!status)
> > +               return IRQ_NONE;
> > +
> > +       timestamp = iio_get_time_ns(indio_dev);
> > +       if ((status & AP3216C_INT_STATUS_PS_MASK) && data->prox_thresh_en)
> > +               iio_push_event(indio_dev,
> > +                              IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> > +                                                   IIO_EV_TYPE_THRESH,
> > +                                                   IIO_EV_DIR_EITHER),
> > +                              timestamp);
> > +
> > +       if ((status & AP3216C_INT_STATUS_ALS_MASK) && data->als_thresh_en)
> > +               iio_push_event(indio_dev,
> > +                              IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> > +                                                   IIO_EV_TYPE_THRESH,
> > +                                                   IIO_EV_DIR_EITHER),
> > +                              timestamp);  
> 
> I'm a complete newbie re. this driver and the iio subsystem in general, but I
> suspect that events could be lost here:
> 
> Example flow:
> 1. device generates ALS data, writes it to the register, asserts interrupt
> 2. this event handler executes, it's a threaded event handler, so may be
> interleaved with userspace threads
> 3. this event handler calls iio_push_event() and is then paused by the scheduler
> 4. a userspace thread monitoring the iio interface reacts to to the event,
> and reads the ALS data
> 5. the device generates _new_ ALS data, interrupt already asserted
> 6. the threaded event handler continues executing, clears the interrupt
> 
> Result: new ALS data is available, but iio did not get an event ?
> 
> If this is indeed an issue, you might be able to address this by locking
> the interrupt handler and ALS/PS read with a mutex ?
> 
> But of course if you do that, you might end up with an interrupt / event
> for a value you've already read out. That's fine only if iio core is ok with
> spurious events.

I may be missing something. That datasheet isn't the clearest I've
ever seen...

I think this device is running in a self clocked mode if we are running
with events.

So an additional read will not result in an additional event.

Also the interrupt is latching.  If we get a repeat of the same event
it is generally of no interest to userspace. If it's a different
event we'll get an additional bit latched.

So yes, it's more than possible that userspace won't get the same number
of events as samples taken over the limit, but I don't know why we care.
We can about missing a threshold being passed entirely, not about knowing
how many samples we were above it for.

> 
> > +
> > +       ret = ap3216c_clear_int(data);
> > +       if (ret < 0)
> > +               dev_err(&data->client->dev, "error clearing IRQ\n");
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int ap3216c_probe(struct i2c_client *client,
> > +                         const struct i2c_device_id *id)
> > +{
> > +       struct ap3216c_data *data;
> > +       struct iio_dev *indio_dev;
> > +       int ret;
> > +
> > +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +       if (!indio_dev)
> > +               return -ENOMEM;
> > +
> > +       data = iio_priv(indio_dev);
> > +       data->client = client;
> > +       indio_dev->dev.parent = &client->dev;
> > +       indio_dev->info = &ap3216c_info;
> > +       indio_dev->name = AP3216C_DRV_NAME;
> > +       indio_dev->channels = ap3216c_channels;
> > +       indio_dev->num_channels = ARRAY_SIZE(ap3216c_channels);
> > +
> > +       data->regmap = devm_regmap_init_i2c(client, &ap3216c_regmap_config);
> > +       if (IS_ERR(data->regmap)) {
> > +               dev_err(&client->dev, "Failed to allocate register map\n");
> > +               return PTR_ERR(data->regmap);
> > +       }
> > +
> > +       /* Default to thresh events disabled */
> > +       data->als_thresh_en = false;
> > +       data->prox_thresh_en = false;
> > +
> > +       /*
> > +        * Require that that the interrupt is cleared only when the INT
> > +        * register is written to, instead of when data is read.  This
> > +        * prevents the interrupt from falsely reporting IRQ_NONE.
> > +        */
> > +       ret = regmap_write(data->regmap,
> > +                          AP3216C_INT_CLR, AP3216C_INT_CLR_MANUAL);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       /* Before setting up IRQ, clear any stale interrupt */
> > +       ret = ap3216c_clear_int(data);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (client->irq) {
> > +               ret = devm_request_threaded_irq(&client->dev, client->irq,
> > +                                               NULL, ap3216c_event_handler,
> > +                                               IRQF_TRIGGER_FALLING |
> > +                                               IRQF_SHARED | IRQF_ONESHOT,
> > +                                               client->name, indio_dev);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       /* Enable ALS and PS+IR */
> > +       ret = regmap_write(data->regmap, AP3216C_SYS, AP3216C_SYS_MODE_ALS_PS);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct of_device_id ap3216c_of_match[] = {
> > +       { .compatible = "liteon,ap3216c", },
> > +       { },
> > +};
> > +MODULE_DEVICE_TABLE(of, ap3216c_of_match);
> > +
> > +static const struct i2c_device_id ap3216c_id[] = {
> > +       {"ap3216c", 0},
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ap3216c_id);
> > +
> > +static struct i2c_driver ap3216c_driver = {
> > +       .driver = {
> > +               .name   = AP3216C_DRV_NAME,
> > +       },
> > +       .probe          = ap3216c_probe,
> > +       .id_table       = ap3216c_id,
> > +};
> > +module_i2c_driver(ap3216c_driver);
> > +
> > +MODULE_AUTHOR("Robert Eshleman <bobbyeshleman@gmail.com>");
> > +MODULE_DESCRIPTION("APC3216C Ambient Light and Proximity Sensor");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.20.1
> >  

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck]

On Mon, Feb 11, 2019 at 4:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Agreed.  Or potentially just use regmap_bulk_read and rely on
> the regmap internal locking to do it for you.

Neat solution. But it may only work correctly iff regmap_bulk_read()
reads the low
address first. I'm not sure if this function has that guarantee. If
somebody changes
the read order, the driver will break. But I think I'm being overly
paranoid here :)

> So yes, it's more than possible that userspace won't get the same number
> of events as samples taken over the limit, but I don't know why we care.
> We can about missing a threshold being passed entirely, not about knowing
> how many samples we were above it for.

I suspect that we run a small risk of losing an event, like so:

PS (12.5 ms)
--> interrupt -> iio event
ALS (100 ms)
--> interrupt -> iio event
PS (12.5 ms)
--> interrupt ========= no iio event generated
ALS (100 ms)
--> interrupt -> iio event

To see why, imagine that the scheduler decides to move away from the
threaded interrupt
handler right before ap3216c_clear_int(). Say 20ms, which I know is a
loooong time,
but bear with me, the point is that it _could_ happen as we're not a RTOS.

static irqreturn_t ap3216c_event_handler(int irq, void *p)
{
        /* imagine ALS interrupt came in, INT_STATUS is 0b01 */
        regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
        if (status & mask1) iio_push_event(PROX);
        if (status & mask2) iio_push_event(LIGHT);

        /* imagine schedule happens here */
        msleep(20);
        /* while we were not running, PS interrupt came in
           INT_STATUS is now 0b11
           yet no new interrupt is generated, as we are ONESHOT
        */
        ap3216c_clear_int(data);
        /* clears both bits, interrupt line goes low.
            knowledge that the PS interrupt came in is now lost */
}

Not sure if that's acceptable driver behaviour. In real life it
probably wouldn't matter much,
except for occasional added latency maybe ?

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Jonathan Cameron]

On Mon, 11 Feb 2019 17:30:18 -0500
Sven Van Asbroeck <thesven73@gmail.com> wrote:

> On Mon, Feb 11, 2019 at 4:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > Agreed.  Or potentially just use regmap_bulk_read and rely on
> > the regmap internal locking to do it for you.  
> 
> Neat solution. But it may only work correctly iff regmap_bulk_read()
> reads the low
> address first. I'm not sure if this function has that guarantee. If
> somebody changes
> the read order, the driver will break. But I think I'm being overly
> paranoid here :)

Good question on whether it is guaranteed to read in increasing register
order (I didn't actually check the addresses are in increasing order
but assume they are or you would have pointed that out ;)

That strikes me as behaviour that should probably be documented as long
as it is true currently.

> 
> > So yes, it's more than possible that userspace won't get the same number
> > of events as samples taken over the limit, but I don't know why we care.
> > We can about missing a threshold being passed entirely, not about knowing
> > how many samples we were above it for.  
> 
> I suspect that we run a small risk of losing an event, like so:
> 
> PS (12.5 ms)
> --> interrupt -> iio event  
More interesting if this one never happened, so we got a one off proximity
event missed.

> ALS (100 ms)
> --> interrupt -> iio event  
> PS (12.5 ms)
> --> interrupt ========= no iio event generated  
> ALS (100 ms)
> --> interrupt -> iio event  
> 
> To see why, imagine that the scheduler decides to move away from the
> threaded interrupt
> handler right before ap3216c_clear_int(). Say 20ms, which I know is a
> loooong time,
> but bear with me, the point is that it _could_ happen as we're not a RTOS.
> 
> static irqreturn_t ap3216c_event_handler(int irq, void *p)
> {
>         /* imagine ALS interrupt came in, INT_STATUS is 0b01 */
>         regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
>         if (status & mask1) iio_push_event(PROX);
>         if (status & mask2) iio_push_event(LIGHT);
> 
>         /* imagine schedule happens here */
>         msleep(20);
>         /* while we were not running, PS interrupt came in
>            INT_STATUS is now 0b11
>            yet no new interrupt is generated, as we are ONESHOT
>         */
>         ap3216c_clear_int(data);
>         /* clears both bits, interrupt line goes low.
>             knowledge that the PS interrupt came in is now lost */
> }
> 
> Not sure if that's acceptable driver behaviour. In real life it
> probably wouldn't matter much,
> except for occasional added latency maybe ?
Good point, I'd missed that a single clear was clearing both bits
rather than just the one we thought had fired.

If we clear just the right one, (which I think we can do from
the datasheet
"1: Software clear after writing 1 into address 0x01 each bit#"
However the code isn't writing a 3 in that clear, so I'm not
sure if the datasheet is correct or not...

and it is a level interrupt (which I think it is?) then we would
be safe against this miss.

If either we can only globally clear or it's not a level interrupt
there isn't much we can do to avoid a miss, it's just a bad hardware
design.

Jonathan

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck]

On Tue, Feb 12, 2019 at 3:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
>
> Good question on whether it is guaranteed to read in increasing register
> order (I didn't actually check the addresses are in increasing order
> but assume they are or you would have pointed that out ;)
>
> That strikes me as behaviour that should probably be documented as long
> as it is true currently.
>

Looking at the datasheet closely... this chip doesn't seem to support
bulk/raw accesses. At least, it's not documented. So maybe we should
steer clear of that, and tell the regmap core, via .use_single_read
and .use_single_write in regmap_config.

So once these flags are set, we could call regmap_bulk_read() on the
LO/HI combo registers, and it would probably work. But do we really
want to? The LO register, when read, cause side-effects
in the corresponding HI register. That's weird / unexpected for developers.
Maybe it makes sense to make this explicit, not implicit. In addition,
bulk_read() behaviour changes in the future may break the register
reads ?

> >
>
> If we clear just the right one, (which I think we can do from
> the datasheet
> "1: Software clear after writing 1 into address 0x01 each bit#"
> However the code isn't writing a 3 in that clear, so I'm not
> sure if the datasheet is correct or not...
>
> and it is a level interrupt (which I think it is?) then we would
> be safe against this miss.
>
> If either we can only globally clear or it's not a level interrupt
> there isn't much we can do to avoid a miss, it's just a bad hardware
> design.
>

I think we're in luck, and this would work ! Note 1 on page 12 of the
datasheet:

"Note1. The INT pin will be set low and set INT status bit when ALS or
PS or (ALS+PS) interrupt event occurrence.
User can clear INT bit and individual status bits when reading the
register 0xD(ALS) , 0xF(PS) and 0xD+0xF(ALS+PS) respectively."

>
>

I'm sorry to wear out your patience, but I think there are more
synchronization issues lurking here.

The iio_push_event(THRESH) tells interested userspace processes:
"hey, maybe you want to check if a threshold is crossed", right?
Is my understanding correct?

If so, I think it's possible that a threshold is crossed, without
userspace knowing. To see why, let's look at the handler again:

static irqreturn_t ap3216c_event_handler(int irq, void *p)
{
        /* imagine ALS interrupt came in */
        regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
        if (status & als) iio_push_event(LIGHT);

        /* imagine schedule happens here */
        msleep(...);
        /* while we are not running, userspace reacts to the event,
         * reads the new ALS value.
         * Next, imagine a _new_ ALS interrupt comes in, while we are
         * still sleeping. the irq does not get re-scheduled, as it's
         * still running !
         * Next, we proceed:
        */
        ap3216c_clear_als(data);
        /* at this point there has been a new ALS interrupt without
         * userspace knowing about it.
         */
}

The _chance_ of this happening is very low, as ALS/PS events are quite
widely spaced. But we're not an RTOS. Question is: do we want to take
the risk? Idk, perhaps it's ok to trade simplicity for the low chance of
missing a threshold.

+static int ap3216c_write_event_config(struct iio_dev *indio_dev,
+                                    const struct iio_chan_spec *chan,
+                                    enum iio_event_type type,
+                                    enum iio_event_direction dir, int state)
+{
+       struct ap3216c_data *data = iio_priv(indio_dev);
+
+       switch (chan->type) {
+       case IIO_LIGHT:
+               data->als_thresh_en = state;
+               return 0;
+
+       case IIO_PROXIMITY:
+               data->prox_thresh_en = state;
+               return 0;
+
+       default:
+               return -EINVAL;
+       }

I think this may not work as intended. One thread (userspace) writes
a variable, another thread (threaded irq handler) checks it. but there
is no explicit or implicit memory barrier. So when userspace activates
thresholding, it may take a long time for the handler to 'see' it !

One way to guarantee that the irq handler 'sees' this immediately
is to grab a mutex, which issues an implicit memory barrier.

>
>

Allow me to suggest a simple, straightforward way to make all the above
issues go away (if indeed they are issues) :

First, disable fine-grained regmap locking, by setting .disable_locking
in regmap_config.

Next, read ALS and PS _exclusively_ in the irq handler, guard it with
a mutex:

static irqreturn_t ap3216c_event_handler(int irq, void *p)
{
    int ret = IRQ_NONE;

    mutex_lock(&data->m);
    regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
    if (status & als) {
        /* mutex m ensures LO and HI are read non-interleaved */
        regmap_read(data->regmap, ALS_LO, &als_lo);
        regmap_read(data->regmap, ALS_HI, &als_hi);
        /* mutex m's memory barrier lets userspace 'see' data->als change */
        data->als = als_lo | (als_hi<<8);
        /* mutex m's memory barrier lets us 'see' do_thresholding change */
        if (data->do_thresholding)
            iio_push_event();
        /* mutex m prevents userspace from reading the cached value
         * in between iio_push_event() and als_clear(), which means
         * userspace never 'misses' interrupts.
         */
        als_clear(data);
        ret = IRQ_HANDLED;
    }
    ( ... ps case left out for brevity ... )
    mutex_unlock(&data->m);

    return ret;
}

Now, ap3216c_read_raw becomes:

ap3216c_read_raw()
{
    mutex_lock(&data->m);
    switch (mask) {
        IIO_CHAN_INFO_PROCESSED:
            switch (chan->type) {
                case IIO_LIGHT:
                    *val = some_function_of(data->als);
            }
    }
    mutex_unlock(&data->m);
}

and ap3216c_write_event_config becomes:

ap3216c_write_event_config()
{
    mutex_lock(&data->m);
    switch (chan->type) {
       case IIO_LIGHT:
               data->als_thresh_en = state;
               goto out;
    }
out:
    mutex_unlock(&data->m);
}

In fact, we'd need mutex_lock around any function that touches the
regmap, cached data, or threshold enable/disable flags.

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck]

On Tue, Feb 12, 2019 at 11:40 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> Next, read ALS and PS _exclusively_ in the irq handler, guard it with
> a mutex:
>

Wait a second, that wouldn't work, because we don't get an interrupt
on every ALS/PS
conversion, correct ?

In that case, don't cache the als/ps value in the irq handler, but
read it from the hardware in
ap3216c_read_raw, while holding the mutex.

That would still guard against:
- _LO and _HI getting interleaved
- userspace reading the value in between sending the event, and
clearing the int bit
- the int handler not immediately seeing _thresh_en

Apologies for the confusion :)

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Jonathan Cameron]

On Tue, 12 Feb 2019 23:40:13 -0500
Sven Van Asbroeck <thesven73@gmail.com> wrote:

> On Tue, Feb 12, 2019 at 3:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> >
Hi Sven

I think the issue, here is that you are putting guarantees on 'consistency'
that IIO does not imply.  In fact as I mention below, for many sensors
it is not possible to imply them, so there is no reason to put false
complexity in here either.

The read of the sysfs value reads the value as near as possible to now.
That value has no particularly relationship with the state when the event
occurred.

> > Good question on whether it is guaranteed to read in increasing register
> > order (I didn't actually check the addresses are in increasing order
> > but assume they are or you would have pointed that out ;)
> >
> > That strikes me as behaviour that should probably be documented as long
> > as it is true currently.
> >  
> 
> Looking at the datasheet closely... this chip doesn't seem to support
> bulk/raw accesses. At least, it's not documented. So maybe we should
> steer clear of that, and tell the regmap core, via .use_single_read
> and .use_single_write in regmap_config.
> 
> So once these flags are set, we could call regmap_bulk_read() on the
> LO/HI combo registers, and it would probably work. But do we really
> want to? The LO register, when read, cause side-effects
> in the corresponding HI register. That's weird / unexpected for developers.
> Maybe it makes sense to make this explicit, not implicit. In addition,
> bulk_read() behaviour changes in the future may break the register
> reads ?

I suspect that would break lots of devices if it happened, but
fair enough that explicit might be good.  One option would be
to document clearly in regmap the requirement that bulk read is ordered.

> 
> > >  
> >
> > If we clear just the right one, (which I think we can do from
> > the datasheet
> > "1: Software clear after writing 1 into address 0x01 each bit#"
> > However the code isn't writing a 3 in that clear, so I'm not
> > sure if the datasheet is correct or not...
> >
> > and it is a level interrupt (which I think it is?) then we would
> > be safe against this miss.
> >
> > If either we can only globally clear or it's not a level interrupt
> > there isn't much we can do to avoid a miss, it's just a bad hardware
> > design.
> >  
> 
> I think we're in luck, and this would work ! Note 1 on page 12 of the
> datasheet:
> 
> "Note1. The INT pin will be set low and set INT status bit when ALS or
> PS or (ALS+PS) interrupt event occurrence.
> User can clear INT bit and individual status bits when reading the
> register 0xD(ALS) , 0xF(PS) and 0xD+0xF(ALS+PS) respectively."
> 
> >
> >  
> 
> I'm sorry to wear out your patience, but I think there are more
> synchronization issues lurking here.
> 
> The iio_push_event(THRESH) tells interested userspace processes:
> "hey, maybe you want to check if a threshold is crossed", right?
> Is my understanding correct?

Nope. It tells you that it has definitely been crossed.  It doesn't
say it wasn't crossed multiple times (userspace is unlikely to care
for a light sensor).

> 
> If so, I think it's possible that a threshold is crossed, without
> userspace knowing. To see why, let's look at the handler again:
> 
> static irqreturn_t ap3216c_event_handler(int irq, void *p)
> {
>         /* imagine ALS interrupt came in */
>         regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
>         if (status & als) iio_push_event(LIGHT);
> 
>         /* imagine schedule happens here */
>         msleep(...);
>         /* while we are not running, userspace reacts to the event,
>          * reads the new ALS value.
>          * Next, imagine a _new_ ALS interrupt comes in, while we are
>          * still sleeping. the irq does not get re-scheduled, as it's
>          * still running !
>          * Next, we proceed:
>         */
>         ap3216c_clear_als(data);

Need to clear the right bit only then we are fine, because we immediately
get another interrupt, because it's a level interrupt (I think)
and we haven't acknowledged it.

>         /* at this point there has been a new ALS interrupt without
>          * userspace knowing about it.
>          */
> }
> 
> The _chance_ of this happening is very low, as ALS/PS events are quite
> widely spaced. But we're not an RTOS. Question is: do we want to take
> the risk? Idk, perhaps it's ok to trade simplicity for the low chance of
> missing a threshold.

This is the fundamental problem.... The relationship between events and
reading from the sysfs files is not what you think...

Semantics are we got an event that said it was crossed.  This was correctly
reported to userspace.   Userspace doesn't typically care if there are multiple
so close together that we missed one.  We aren't accurately counting how
long it was dark, just noting that it became dark.

> 
> +static int ap3216c_write_event_config(struct iio_dev *indio_dev,
> +                                    const struct iio_chan_spec *chan,
> +                                    enum iio_event_type type,
> +                                    enum iio_event_direction dir, int state)
> +{
> +       struct ap3216c_data *data = iio_priv(indio_dev);
> +
> +       switch (chan->type) {
> +       case IIO_LIGHT:
> +               data->als_thresh_en = state;
> +               return 0;
> +
> +       case IIO_PROXIMITY:
> +               data->prox_thresh_en = state;
> +               return 0;
> +
> +       default:
> +               return -EINVAL;
> +       }
> 
> I think this may not work as intended. One thread (userspace) writes
> a variable, another thread (threaded irq handler) checks it. but there
> is no explicit or implicit memory barrier. So when userspace activates
> thresholding, it may take a long time for the handler to 'see' it !

Yes.  But if userspace took a while to get around to writing this value,
it would also take longer...  It's not time critical exactly when you
enable the event.  One can create cases where someone might
care, but they are pretty obscure.

> 
> One way to guarantee that the irq handler 'sees' this immediately
> is to grab a mutex, which issues an implicit memory barrier.
> 
> >
> >  
> 
> Allow me to suggest a simple, straightforward way to make all the above
> issues go away (if indeed they are issues) :
> 
> First, disable fine-grained regmap locking, by setting .disable_locking
> in regmap_config.
> 
> Next, read ALS and PS _exclusively_ in the irq handler, guard it with
> a mutex:
> 
> static irqreturn_t ap3216c_event_handler(int irq, void *p)
> {
>     int ret = IRQ_NONE;
> 
>     mutex_lock(&data->m);
>     regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
>     if (status & als) {
>         /* mutex m ensures LO and HI are read non-interleaved */
>         regmap_read(data->regmap, ALS_LO, &als_lo);
>         regmap_read(data->regmap, ALS_HI, &als_hi);

No. There is no explicit data gathering in an IIO event.  That's the semantics
we have, and we aren't going to change them for a theoretical race which doesn't
actually have any meaningful effect.

If you get an event, the threshold was passed.  If you read after that
from usersapce you may or may not find the value is above the threshold.
It doesn't matter, you still know it was.

So, lets have a rapidly varying light source to illustrate why this is incredibly
unlikely to matter, because no system can be defined to require that it does...

Light, detecting low because I drew it that way ;)
-------         ---------------
       |_______|


sample   |     |

So did that second one hit the window?  Maybe, maybe not.  However, did one of them
hit the window so we got an interrupt.  Yes, because our read frequency is fast enough.

So the case you are dealing with is trying to ensure you get the right number of
interrupts.   That isn't how these sensors are used.  They aren't that precise and
the real world isn't that precise.

What we need to guarantee is:

1) If the sensor reads on an occasion where the threshold is passed, we do not miss the event
   The event is the threshold being passed, not the existence of the reading, or how many
   readings etc.

2) A data read will result in a value.  There is no guarantee that it will match with the
   event.  All manner of delays could result in new data having occurred before that read.

Many devices run at many times the frequency we read them at, allow detection of fast
transitions, we don't expect them to always give us a reading that 'confirms' the
threshold pass. We should not do so here either.


>         /* mutex m's memory barrier lets userspace 'see' data->als change */
>         data->als = als_lo | (als_hi<<8);
>         /* mutex m's memory barrier lets us 'see' do_thresholding change */
>         if (data->do_thresholding)
>             iio_push_event();
>         /* mutex m prevents userspace from reading the cached value
>          * in between iio_push_event() and als_clear(), which means
>          * userspace never 'misses' interrupts.

Ah.. This kind of makes it clear where the confusion likes.  Why would we care
if it does read it there?  It would read the value, what has the value got
to do with the interrupt?  Nothing whatsoever.  They were generated by
the same actual read, but userspace should not care about that as we don't
make any promises that they were...

>          */
>         als_clear(data);
>         ret = IRQ_HANDLED;
>     }
>     ( ... ps case left out for brevity ... )
>     mutex_unlock(&data->m);
> 
>     return ret;
> }
> 
> Now, ap3216c_read_raw becomes:
> 
> ap3216c_read_raw()
> {
>     mutex_lock(&data->m);
>     switch (mask) {
>         IIO_CHAN_INFO_PROCESSED:
>             switch (chan->type) {
>                 case IIO_LIGHT:
>                     *val = some_function_of(data->als);
Which means that If I come along and read 10 minutes later
I get your latched value?

Why would I want that?  I'm not asking for what was the value when the
interrupt fired, I'm asking what is the value now.

>             }
>     }
>     mutex_unlock(&data->m);
> }
> 
> and ap3216c_write_event_config becomes:
> 
> ap3216c_write_event_config()
> {
>     mutex_lock(&data->m);
>     switch (chan->type) {
>        case IIO_LIGHT:
>                data->als_thresh_en = state;
>                goto out;
>     }
> out:
>     mutex_unlock(&data->m);
> }
> 
> In fact, we'd need mutex_lock around any function that touches the
> regmap, cached data, or threshold enable/disable flags.
There may be potential need for more locking, but not to protect some race
around the value, vs the interrupt...

Jonathan

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck]

Hi Jonathan,

Thanks again for your clear and extensive feedback !

On Mon, Feb 18, 2019 at 10:16 AM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> I suspect that would break lots of devices if it happened, but
> fair enough that explicit might be good.  One option would be
> to document clearly in regmap the requirement that bulk read is ordered.
>

Yes, it would be interesting to hear the regmap people's opinion on ordering.
In the mean time, we can make this explicit.
Re-reading the thread, I can also see that Peter Meerwald-Stadler was first
to spot this race condition.

> What we need to guarantee is:
>
> 1) If the sensor reads on an occasion where the threshold is passed, we do not miss the event
>    The event is the threshold being passed, not the existence of the reading, or how many
>    readings etc.
>
> 2) A data read will result in a value.  There is no guarantee that it will match with the
>    event.  All manner of delays could result in new data having occurred before that read.
>

My feedback was based on two incorrect assumptions:
a. the interrupt fires whenever new PS/ALS values become available (wrong)
b. there are strict consistency guarantees between the THRESH event, and what
userspace will read out (also wrong)

Taking that into account, I am 100% in agreement with your other comments.
Thank you so much for the explanation!

There is one exception, though:

> > +static int ap3216c_write_event_config(struct iio_dev *indio_dev,
> > +                                    const struct iio_chan_spec *chan,
> > +                                    enum iio_event_type type,
> > +                                    enum iio_event_direction dir, int state)
> > +{
> > +       struct ap3216c_data *data = iio_priv(indio_dev);
> > +
> > +       switch (chan->type) {
> > +       case IIO_LIGHT:
> > +               data->als_thresh_en = state;
> > +               return 0;
> > +
> > +       case IIO_PROXIMITY:
> > +               data->prox_thresh_en = state;
> > +               return 0;
> > +
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +static irqreturn_t ap3216c_event_handler(int irq, void *p)
> > +{
> > + if ((status & AP3216C_INT_STATUS_PS_MASK) && data->prox_thresh_en)
> > + iio_push_event(...);
> > +
> >
> > I think this may not work as intended. One thread (userspace) writes
> > a variable, another thread (threaded irq handler) checks it. but there
> > is no explicit or implicit memory barrier. So when userspace activates
> > thresholding, it may take a long time for the handler to 'see' it !
>
> Yes.  But if userspace took a while to get around to writing this value,
> it would also take longer...  It's not time critical exactly when you
> enable the event.  One can create cases where someone might
> care, but they are pretty obscure.
>

Are you sure? I suspect that it's perfectly possible for the threaded irq
handler not to 'see' the store to (als|prox)_thresh_en for a _very_ long time.

AFAIK only a memory barrier will guarantee that the handler 'sees' the store
right away. A lock will do - it issues an implicit memory barrier.

Most drivers use a lock to guarantee visibility. There are a few drivers that
resort to explicit barriers to make a flag visible from one thread to another.

E.g. search for mb() or wmb() in:
drivers/input/keyboard/matrix_keypad.c
drivers/input/misc/cm109.c
drivers/input/misc/yealink.c

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Jonathan Cameron]

On Mon, 18 Feb 2019 14:35:51 -0500
Sven Van Asbroeck <thesven73@gmail.com> wrote:

> Hi Jonathan,
> 
> Thanks again for your clear and extensive feedback !
> 
> On Mon, Feb 18, 2019 at 10:16 AM Jonathan Cameron
> <jonathan.cameron@huawei.com> wrote:
> >
> > I suspect that would break lots of devices if it happened, but
> > fair enough that explicit might be good.  One option would be
> > to document clearly in regmap the requirement that bulk read is ordered.
> >  
> 
> Yes, it would be interesting to hear the regmap people's opinion on ordering.
> In the mean time, we can make this explicit.
> Re-reading the thread, I can also see that Peter Meerwald-Stadler was first
> to spot this race condition.
> 
> > What we need to guarantee is:
> >
> > 1) If the sensor reads on an occasion where the threshold is passed, we do not miss the event
> >    The event is the threshold being passed, not the existence of the reading, or how many
> >    readings etc.
> >
> > 2) A data read will result in a value.  There is no guarantee that it will match with the
> >    event.  All manner of delays could result in new data having occurred before that read.
> >  
> 
> My feedback was based on two incorrect assumptions:
> a. the interrupt fires whenever new PS/ALS values become available (wrong)
> b. there are strict consistency guarantees between the THRESH event, and what
> userspace will read out (also wrong)
> 
> Taking that into account, I am 100% in agreement with your other comments.
> Thank you so much for the explanation!
> 
> There is one exception, though:
> 
> > > +static int ap3216c_write_event_config(struct iio_dev *indio_dev,
> > > +                                    const struct iio_chan_spec *chan,
> > > +                                    enum iio_event_type type,
> > > +                                    enum iio_event_direction dir, int state)
> > > +{
> > > +       struct ap3216c_data *data = iio_priv(indio_dev);
> > > +
> > > +       switch (chan->type) {
> > > +       case IIO_LIGHT:
> > > +               data->als_thresh_en = state;
> > > +               return 0;
> > > +
> > > +       case IIO_PROXIMITY:
> > > +               data->prox_thresh_en = state;
> > > +               return 0;
> > > +
> > > +       default:
> > > +               return -EINVAL;
> > > +       }
> > > +static irqreturn_t ap3216c_event_handler(int irq, void *p)
> > > +{
> > > + if ((status & AP3216C_INT_STATUS_PS_MASK) && data->prox_thresh_en)
> > > + iio_push_event(...);
> > > +
> > >
> > > I think this may not work as intended. One thread (userspace) writes
> > > a variable, another thread (threaded irq handler) checks it. but there
> > > is no explicit or implicit memory barrier. So when userspace activates
> > > thresholding, it may take a long time for the handler to 'see' it !  
> >
> > Yes.  But if userspace took a while to get around to writing this value,
> > it would also take longer...  It's not time critical exactly when you
> > enable the event.  One can create cases where someone might
> > care, but they are pretty obscure.
> >  
> 
> Are you sure? I suspect that it's perfectly possible for the threaded irq
> handler not to 'see' the store to (als|prox)_thresh_en for a _very_ long time.

That is a serious - "in theory" circumstance.  The moment we hit any path at
all that results in a memory barrier it will see it.  Here its not critical
so we can wait.  In this case this is triggered by a userspace write.

Looks to me like that happens (I haven't checked that thoroughly) via
kernfs_fops_write which takes a mutex - so we have a barrier.

There are of course cases where multiple concurrent in kernel actions need
to be protected and need a memory barrier, but this doesn't look like one
of those to me.

> 
> AFAIK only a memory barrier will guarantee that the handler 'sees' the store
> right away. A lock will do - it issues an implicit memory barrier.
> 
> Most drivers use a lock to guarantee visibility. There are a few drivers that
> resort to explicit barriers to make a flag visible from one thread to another.

That's misleading. Most drivers use a lock to protect state against concurrent
inconsistent writes.  They don't take a lock because of it's memory barrier.
I have no objection to seeing one here as it's easier to know it is correct,
and the scope of lock can be nice and apparent.

> 
> E.g. search for mb() or wmb() in:
> drivers/input/keyboard/matrix_keypad.c
> drivers/input/misc/cm109.c
> drivers/input/misc/yealink.c

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck]

Again thanks for the feedback, Jonathan !

On Wed, Feb 20, 2019 at 7:32 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Looks to me like that happens (I haven't checked that thoroughly) via
> kernfs_fops_write which takes a mutex - so we have a barrier.
>

Yes, if there's a mutex in the path somewhere (which sysfs/kernfs appears to
provide in this case) then we're ok, as the mutex_unlock() should provide
the barrier which makes the flags visible to the threaded handler.

If that never changes, we are good. As with regmap_bulk_write(), changing the
current behaviour may break many drivers. So it's unlikely to change.

>
> That is a serious - "in theory" circumstance.  The moment we hit any path at
> all that results in a memory barrier it will see it.  Here its not critical
> so we can wait.  In this case this is triggered by a userspace write.

I wish this was an "in theory" circumstance ! I've personally been stung by
this very issue. Code worked great in all tests we could throw at it, but
failed at the customer, of course.

IMHO it's easy for developers to be lulled into a false sense of security.
In many/most cases, there'll be a barrier in the 'invisible' supporting code
somewhere. Or we have to use a lock anyway to protect against concurrent
inconsistent writes, which also implies a barrier. So when developers forget
that these visibility limitations even exist, usually nothing bad will happen.

Until it does, and then we get bitten :)

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Robert Eshleman]

On Tue, Feb 12, 2019 at 08:47:30PM +0000, Jonathan Cameron wrote:
> On Mon, 11 Feb 2019 17:30:18 -0500
> Sven Van Asbroeck <thesven73@gmail.com> wrote:
> 
> > On Mon, Feb 11, 2019 at 4:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > Agreed.  Or potentially just use regmap_bulk_read and rely on
> > > the regmap internal locking to do it for you.  
> > 
> > Neat solution. But it may only work correctly iff regmap_bulk_read()
> > reads the low
> > address first. I'm not sure if this function has that guarantee. If
> > somebody changes
> > the read order, the driver will break. But I think I'm being overly
> > paranoid here :)
> 
> Good question on whether it is guaranteed to read in increasing register
> order (I didn't actually check the addresses are in increasing order
> but assume they are or you would have pointed that out ;)
> 
> That strikes me as behaviour that should probably be documented as long
> as it is true currently.
> 
> > 
> > > So yes, it's more than possible that userspace won't get the same number
> > > of events as samples taken over the limit, but I don't know why we care.
> > > We can about missing a threshold being passed entirely, not about knowing
> > > how many samples we were above it for.  
> > 
> > I suspect that we run a small risk of losing an event, like so:
> > 
> > PS (12.5 ms)
> > --> interrupt -> iio event  
> More interesting if this one never happened, so we got a one off proximity
> event missed.
> 
> > ALS (100 ms)
> > --> interrupt -> iio event  
> > PS (12.5 ms)
> > --> interrupt ========= no iio event generated  
> > ALS (100 ms)
> > --> interrupt -> iio event  
> > 
> > To see why, imagine that the scheduler decides to move away from the
> > threaded interrupt
> > handler right before ap3216c_clear_int(). Say 20ms, which I know is a
> > loooong time,
> > but bear with me, the point is that it _could_ happen as we're not a RTOS.
> > 
> > static irqreturn_t ap3216c_event_handler(int irq, void *p)
> > {
> >         /* imagine ALS interrupt came in, INT_STATUS is 0b01 */
> >         regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
> >         if (status & mask1) iio_push_event(PROX);
> >         if (status & mask2) iio_push_event(LIGHT);
> > 
> >         /* imagine schedule happens here */
> >         msleep(20);
> >         /* while we were not running, PS interrupt came in
> >            INT_STATUS is now 0b11
> >            yet no new interrupt is generated, as we are ONESHOT
> >         */
> >         ap3216c_clear_int(data);
> >         /* clears both bits, interrupt line goes low.
> >             knowledge that the PS interrupt came in is now lost */
> > }
> > 
> > Not sure if that's acceptable driver behaviour. In real life it
> > probably wouldn't matter much,
> > except for occasional added latency maybe ?
> Good point, I'd missed that a single clear was clearing both bits
> rather than just the one we thought had fired.
> 
> If we clear just the right one, (which I think we can do from
> the datasheet
> "1: Software clear after writing 1 into address 0x01 each bit#"
> However the code isn't writing a 3 in that clear, so I'm not
> sure if the datasheet is correct or not...
> 
> and it is a level interrupt (which I think it is?) then we would
> be safe against this miss.
> 
> If either we can only globally clear or it's not a level interrupt
> there isn't much we can do to avoid a miss, it's just a bad hardware
> design.

This totally makes sense, obviously something I had missed.  
I think you are right, if each INT bit is cleared individually,
then that second event won't be lost.

I'll take a closer look at the ideas put forth by you and Sven here on
using mutex on some of the other cases (such as the write_event_config
vs event_handler race condition) and put those into v2.

Thanks again for all the constructive feedback.

-Bobby

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck]

On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman <bobbyeshleman@gmail.com> wrote:
>
> This patch adds support for the ap3216c ambient light and proximity
> sensor.

PS

Why not use the chip in the mode where the interrupt is automatically cleared by
reading the data? This could work if you read the data in the
interrupt routine, store
it in a buffer, then send the event to iio. Then when userspace wants
to read out the
value, don't actually touch the hardware, but return the buffered value.

I don't think you then need any synchronization primitives to
accomplish this, such as
mutexes, spin locks, etc. Because the interrupt routine is single-threaded.

You don't even need a memory barrier for the buffered value,
because the iio core uses a waitqueue internally, which automatically issues
an mb(). As far as I know.

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Robert Eshleman]

On Mon, Feb 11, 2019 at 02:29:58PM -0500, Sven Van Asbroeck wrote:
> On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman <bobbyeshleman@gmail.com> wrote:
> >
> > This patch adds support for the ap3216c ambient light and proximity
> > sensor.
> 
> PS
> 
> Why not use the chip in the mode where the interrupt is automatically cleared by
> reading the data? This could work if you read the data in the
> interrupt routine, store
> it in a buffer, then send the event to iio. Then when userspace wants
> to read out the
> value, don't actually touch the hardware, but return the buffered value.
> 
> I don't think you then need any synchronization primitives to
> accomplish this, such as
> mutexes, spin locks, etc. Because the interrupt routine is single-threaded.
> 
> You don't even need a memory barrier for the buffered value,
> because the iio core uses a waitqueue internally, which automatically issues
> an mb(). As far as I know.

Hey Sven,

First, thank you for the feedback.

I had initially went with a similar design, but there is
the case in which the interrupt fires and then before the status
register is read by the handler a user process reads the data and
clears the interrupt.  When the handler continues execution it will
read a zero status and return IRQ_NONE.  My understanding of how
Linux handles IRQ_NONE is pretty poor, but I felt that this behavior
is incorrect even if inconsequential.  This could be avoided by
doing a status register read with every data read, and buffering
that as well, but then we lose the benefit altogether by increasing
I2C reads.

In the approach you describe here, it seems like that would
work if this driver wasn't supporting shared interrupts.  In the
case that a user-space read happens to clear the interrupt before
the handler reads the status register, I think we would end up
falsely returning IRQ_NONE.

Is my understanding of this correct?  It's very possible I'm
misunderstanding IRQ_NONE and shared interrupts.

Again, thank you for the feedback.

-Bobby

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck]

Hi Bobby,

On Tue, Feb 12, 2019 at 9:17 PM Robert Eshleman <bobbyeshleman@gmail.com> wrote:
>
> First, thank you for the feedback.

First of all, thank _you_ for doing the hard work on this driver !
I very much respect what you've done here.

>
> I had initially went with a similar design, but there is
> the case in which the interrupt fires and then before the status
> register is read by the handler a user process reads the data and
> clears the interrupt.  When the handler continues execution it will
> read a zero status and return IRQ_NONE.  My understanding of how
> Linux handles IRQ_NONE is pretty poor, but I felt that this behavior
> is incorrect even if inconsequential.  This could be avoided by
> doing a status register read with every data read, and buffering
> that as well, but then we lose the benefit altogether by increasing
> I2C reads.
>
> In the approach you describe here, it seems like that would
> work if this driver wasn't supporting shared interrupts.  In the
> case that a user-space read happens to clear the interrupt before
> the handler reads the status register, I think we would end up
> falsely returning IRQ_NONE.
>
> Is my understanding of this correct?  It's very possible I'm
> misunderstanding IRQ_NONE and shared interrupts.
>

Yes, I can see how one can run into those issues.

I believe that this whole class of problems goes away if PS/ALS
are _exclusively_ read inside the interrupt, and cached.

Then, whenever a user process wants to read the data, the function
does not touch the h/w, but simply return the cached value.

But hang on, I will have more to say on this when replying to Jonathan's
feedback.

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Jonathan Cameron]

On Tue, 12 Feb 2019 22:25:39 -0500
Sven Van Asbroeck <thesven73@gmail.com> wrote:

> Hi Bobby,
> 
> On Tue, Feb 12, 2019 at 9:17 PM Robert Eshleman <bobbyeshleman@gmail.com> wrote:
> >
> > First, thank you for the feedback.  
> 
> First of all, thank _you_ for doing the hard work on this driver !
> I very much respect what you've done here.
> 
> >
> > I had initially went with a similar design, but there is
> > the case in which the interrupt fires and then before the status
> > register is read by the handler a user process reads the data and
> > clears the interrupt.  When the handler continues execution it will
> > read a zero status and return IRQ_NONE.  My understanding of how
> > Linux handles IRQ_NONE is pretty poor, but I felt that this behavior
> > is incorrect even if inconsequential.  This could be avoided by
> > doing a status register read with every data read, and buffering
> > that as well, but then we lose the benefit altogether by increasing
> > I2C reads.
> >
> > In the approach you describe here, it seems like that would
> > work if this driver wasn't supporting shared interrupts.  In the
> > case that a user-space read happens to clear the interrupt before
> > the handler reads the status register, I think we would end up
> > falsely returning IRQ_NONE.
> >
> > Is my understanding of this correct?  It's very possible I'm
> > misunderstanding IRQ_NONE and shared interrupts.
> >  
> 
> Yes, I can see how one can run into those issues.
> 
> I believe that this whole class of problems goes away if PS/ALS
> are _exclusively_ read inside the interrupt, and cached.
> 
> Then, whenever a user process wants to read the data, the function
> does not touch the h/w, but simply return the cached value.
> 
> But hang on, I will have more to say on this when replying to Jonathan's
> feedback.

As mentioned in the other thread.  Can't do that.  Then we don't get a readout for
a normal read of the value.  If we aren't above the threshold then
we don't an interrupt, hence no value is ever read.

So, what I'm reading above is worrying.   The interrupt is cleared
by the read of the data registers?  I thought the datasheet allowed
for an explicit clear?

If it clears on a read, then we are on one of those rare and really
annoying devices where we have to deal with them not really being able
to do monitoring and polled reading at the same time and also not
providing a dataready interrupt (which allows for the solution Sven
gives)

Basically we've only ever had one of these that I can recall and on that
max1363 it was more a case of the format being really nasty to decode
than the hardware not being capable of safely do it, so we didn't bother.

Jonathan

Re: [PATCH 1/3] iio: light: Add driver for ap3216c

[Sven Van Asbroeck]

Hello Jonathan,

First of all, thank you so much for taking the time for such a detailed
reply !

On Mon, Feb 18, 2019 at 10:22 AM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
>
> So, what I'm reading above is worrying.   The interrupt is cleared
> by the read of the data registers?  I thought the datasheet allowed
> for an explicit clear?
>

I don't think there's any need to worry. The part allows us to choose between
clear-on-read, and explicit clear. Bobby's approach is fundamentally sound.

IMHO most of my suggestions were based on an incomplete understanding of
a) the interrupt in the device, and b) the event guarantees in iio.

I will respond to your larger reply properly soon.