[PATCH v2 0/2] Add support for Sensortek STK3310

[PATCH v2 0/2] Add support for Sensortek STK3310

[Tiberiu Breana]

These patches include an iio driver for the Sensortek STK3310
ambient light and proximity sensor. The STK3311 model is also
supported.

Datasheet:
http://www.datasheetspdf.com/datasheet/STK3310.html

Patches are as following:

1. basic functionality:
- raw readings of light and proximity data
- configuration of parameters like gain and integration time
- power management

2. interrupt support:
- interrupt support for proximity events
- enabling/disabling interrupts (events) via sysfs
- setting proximity thresholds

Changes since v1:
- addressed Jonathan's comments
- replaced devm_iio_device_register w/ iio_device_register
- merged power management patch into patch 1
- replaced the cache table with a regmap that now handles all
  parameter and interrupt configurations
- removed the set_cfg function along with enums and defines
  that were no longer needed
- fixed IT values exposed to userspace
- removed the reset_psint function and moved its functionality
  to the event handler
- added an irq_handler to capture  more accurate event timestamps

Regards,
Tiberiu


Tiberiu Breana (2):
  iio: light: Add support for Sensortek STK3310
  iio: light: Add threshold interrupt support for STK3310

 drivers/iio/light/Kconfig   |  11 +
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/stk3310.c | 780 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 792 insertions(+)
 create mode 100644 drivers/iio/light/stk3310.c

-- 
1.9.1

[PATCH v2 1/2] iio: light: Add support for Sensortek STK3310

[Tiberiu Breana]

Minimal implementation of an IIO driver for the Sensortek
STK3310 ambient light and proximity sensor. The STK3311
model is also supported.

Includes:
- ACPI support;
- read_raw and write_raw;
- reading and setting configuration parameters for gain/scale
  and integration time for both ALS and PS.
- power management

Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/light/Kconfig   |  11 +
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/stk3310.c | 508 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 520 insertions(+)
 create mode 100644 drivers/iio/light/stk3310.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 01a1a16..096129d 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -174,6 +174,17 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
 
+config STK3310
+	tristate "STK3310 ALS and proximity sensor"
+	depends on I2C
+	help
+	 Say yes here to get support for the Sensortek STK3310 ambient light
+	 and proximity sensor. The STK3311 model is also supported by this
+	 driver.
+
+	 Choosing M will build the driver as a module. If so, the module
+	 will be called stk3310.
+
 config TCS3414
 	tristate "TAOS TCS3414 digital color sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index ad7c30f..90e7fd2 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
 obj-$(CONFIG_SENSORS_TSL2563)	+= tsl2563.o
+obj-$(CONFIG_STK3310)          += stk3310.o
 obj-$(CONFIG_TCS3414)		+= tcs3414.o
 obj-$(CONFIG_TCS3472)		+= tcs3472.o
 obj-$(CONFIG_TSL4531)		+= tsl4531.o
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
new file mode 100644
index 0000000..825a65a
--- /dev/null
+++ b/drivers/iio/light/stk3310.c
@@ -0,0 +1,508 @@
+/**
+ * Sensortek STK3310/STK3311 Ambient Light and Proximity Sensor
+ *
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License. See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * IIO driver for STK3310/STK3311. 7-bit I2C address: 0x48.
+ */
+
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define STK3310_REG_STATE			0x00
+#define STK3310_REG_PSCTRL			0x01
+#define STK3310_REG_ALSCTRL			0x02
+#define STK3310_REG_PS_DATA_MSB			0x11
+#define STK3310_REG_PS_DATA_LSB			0x12
+#define STK3310_REG_ALS_DATA_MSB		0x13
+#define STK3310_REG_ALS_DATA_LSB		0x14
+#define STK3310_REG_ID				0x3E
+#define STK3310_MAX_REG				0x80
+
+#define STK3310_STATE_EN_PS			0x01
+#define STK3310_STATE_EN_ALS			0x02
+#define STK3310_STATE_STANDBY			0x00
+
+#define STK3310_CHIP_ID_VAL			0x13
+#define STK3311_CHIP_ID_VAL			0x1D
+#define STK3310_PS_MAX_VAL			0xffff
+
+#define STK3310_SCALE_AVAILABLE			"6.4 1.6 0.4 0.1"
+
+#define STK3310_DRIVER_NAME			"stk3310"
+#define STK3310_REGMAP_NAME			"stk3310_regmap"
+
+static const struct reg_field reg_field_state =
+				REG_FIELD(STK3310_REG_STATE, 0, 2);
+static const struct reg_field reg_field_als_gain =
+				REG_FIELD(STK3310_REG_ALSCTRL, 4, 5);
+static const struct reg_field reg_field_ps_gain =
+				REG_FIELD(STK3310_REG_PSCTRL, 4, 5);
+static const struct reg_field reg_field_als_it =
+				REG_FIELD(STK3310_REG_ALSCTRL, 0, 3);
+static const struct reg_field reg_field_ps_it =
+				REG_FIELD(STK3310_REG_PSCTRL, 0, 3);
+
+/*
+ * Maximum PS values with regard to scale. Used to export the 'inverse'
+ * PS value (high values for far objects, low values for near objects).
+ */
+static const int stk3310_ps_max[4] = {
+	STK3310_PS_MAX_VAL / 64,
+	STK3310_PS_MAX_VAL / 16,
+	STK3310_PS_MAX_VAL /  4,
+	STK3310_PS_MAX_VAL,
+};
+
+static const int stk3310_scale_table[][2] = {
+	{6, 400000}, {1, 600000}, {0, 400000}, {0, 100000}
+};
+
+/* Integration time in seconds, microseconds */
+static const int stk3310_it_table[][2] = {
+	{0, 185},	{0, 370},	{0, 741},	{0, 1480},
+	{0, 2960},	{0, 5920},	{0, 11840},	{0, 23680},
+	{0, 47360},	{0, 94720},	{0, 189440},	{0, 378880},
+	{0, 757760},	{1, 515520},	{3, 31040},	{6, 62080},
+};
+
+struct stk3310_data {
+	struct i2c_client *client;
+	struct mutex lock;
+	bool als_enabled;
+	bool ps_enabled;
+	struct regmap *regmap;
+	struct regmap_field *reg_state;
+	struct regmap_field *reg_als_gain;
+	struct regmap_field *reg_ps_gain;
+	struct regmap_field *reg_als_it;
+	struct regmap_field *reg_ps_it;
+};
+
+static const struct iio_chan_spec stk3310_channels[] = {
+	{
+		.channel = 0,
+		.type = IIO_LIGHT,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE) |
+			BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+	{
+		.channel = 1,
+		.type = IIO_PROXIMITY,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE) |
+			BIT(IIO_CHAN_INFO_INT_TIME),
+	}
+};
+
+static ssize_t stk3310_get_it_vals(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	int i;
+	int len;
+
+	for (i = 0, len = 0; i < ARRAY_SIZE(stk3310_it_table); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				stk3310_it_table[i][0],
+				stk3310_it_table[i][1]);
+	}
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_CONST_ATTR(in_illuminance_scale_available, STK3310_SCALE_AVAILABLE);
+
+static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,
+		       S_IRUGO, stk3310_get_it_vals, NULL, 0);
+
+static IIO_DEVICE_ATTR(in_proximity_integration_time_available,
+		       S_IRUGO, stk3310_get_it_vals, NULL, 0);
+
+static struct attribute *stk3310_attributes[] = {
+	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
+	&iio_dev_attr_in_proximity_integration_time_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group stk3310_attribute_group = {
+	.attrs = stk3310_attributes
+};
+
+static int stk3310_get_index(const int table[][2], int table_size,
+			     int val, int val2)
+{
+	int i;
+
+	for (i = 0; i < table_size; i++) {
+		if (val == table[i][0] && val2 == table[i][1])
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int stk3310_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	u8 reg;
+	u16 buf;
+	unsigned int index;
+	struct stk3310_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+
+	switch (chan->type) {
+	case IIO_LIGHT:
+		reg = STK3310_REG_ALS_DATA_MSB;
+		break;
+	case IIO_PROXIMITY:
+		reg = STK3310_REG_PS_DATA_MSB;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&data->lock);
+		regmap_bulk_read(data->regmap, reg, &buf, 2);
+		if (buf < 0) {
+			dev_err(&client->dev, "register read failed\n");
+			mutex_unlock(&data->lock);
+			return buf;
+		}
+		*val = swab16(buf);
+		if (chan->type == IIO_PROXIMITY) {
+			/*
+			 * Invert the proximity data so we return low values
+			 * for close objects and high values for far ones.
+			 */
+			regmap_field_read(data->reg_ps_gain, &index);
+			*val = stk3310_ps_max[index] - *val;
+		}
+		mutex_unlock(&data->lock);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_INT_TIME:
+		if (chan->type == IIO_LIGHT)
+			regmap_field_read(data->reg_als_it, &index);
+		else
+			regmap_field_read(data->reg_ps_it, &index);
+		*val = stk3310_it_table[index][0];
+		*val2 = stk3310_it_table[index][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_LIGHT)
+			regmap_field_read(data->reg_als_gain, &index);
+		else
+			regmap_field_read(data->reg_ps_gain, &index);
+		*val = stk3310_scale_table[index][0];
+		*val2 = stk3310_scale_table[index][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+
+	return -EINVAL;
+}
+
+static int stk3310_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	int ret;
+	unsigned int index;
+	struct stk3310_data *data = iio_priv(indio_dev);
+
+	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		index = stk3310_get_index(stk3310_it_table,
+					  ARRAY_SIZE(stk3310_it_table),
+					  val, val2);
+		if (index < 0)
+			return -EINVAL;
+		mutex_lock(&data->lock);
+		if (chan->type == IIO_LIGHT)
+			ret = regmap_field_write(data->reg_als_it, index);
+		else
+			ret = regmap_field_write(data->reg_ps_it, index);
+		if (ret < 0)
+			dev_err(&data->client->dev,
+					"sensor configuration failed\n");
+		mutex_unlock(&data->lock);
+		return ret;
+
+	case IIO_CHAN_INFO_SCALE:
+		index = stk3310_get_index(stk3310_scale_table,
+					  ARRAY_SIZE(stk3310_scale_table),
+					  val, val2);
+		if (index < 0)
+			return -EINVAL;
+		mutex_lock(&data->lock);
+		if (chan->type == IIO_LIGHT)
+			ret = regmap_field_write(data->reg_als_gain, index);
+		else
+			ret = regmap_field_write(data->reg_ps_gain, index);
+		if (ret < 0)
+			dev_err(&data->client->dev,
+					"sensor configuration failed\n");
+		mutex_unlock(&data->lock);
+		return ret;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info stk3310_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= stk3310_read_raw,
+	.write_raw		= stk3310_write_raw,
+	.attrs			= &stk3310_attribute_group,
+};
+
+static int stk3310_set_state(struct stk3310_data *data, u8 state)
+{
+	int ret;
+	struct i2c_client *client = data->client;
+
+	/* 3-bit state; 0b100 is not supported. */
+	if (state < 0 || state > 7 || state == 4)
+		return -EINVAL;
+
+	mutex_lock(&data->lock);
+	ret = regmap_field_write(data->reg_state, state);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to change sensor state\n");
+	/* Don't reset the 'enabled' flags if we're going in standby */
+	} else if (state != STK3310_STATE_STANDBY) {
+		data->ps_enabled  = !!(state & 0x01);
+		data->als_enabled = !!(state & 0x02);
+	}
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int stk3310_init(struct iio_dev *indio_dev)
+{
+	int ret;
+	int chipid;
+	u8 state;
+	struct stk3310_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+
+	regmap_read(data->regmap, STK3310_REG_ID, &chipid);
+	if (chipid != STK3310_CHIP_ID_VAL &&
+	    chipid != STK3311_CHIP_ID_VAL) {
+		dev_err(&client->dev, "invalid chip id: 0x%x\n", chipid);
+		return -ENODEV;
+	}
+
+	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
+	ret = stk3310_set_state(data, state);
+	if (ret < 0)
+		dev_err(&client->dev, "failed to enable sensor");
+
+	return ret;
+}
+
+static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case STK3310_REG_ALS_DATA_MSB:
+	case STK3310_REG_ALS_DATA_LSB:
+	case STK3310_REG_PS_DATA_LSB:
+	case STK3310_REG_PS_DATA_MSB:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static struct regmap_config stk3310_regmap_config = {
+	.name = STK3310_REGMAP_NAME,
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = STK3310_MAX_REG,
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = stk3310_is_volatile_reg,
+};
+
+static int stk3310_regmap_init(struct stk3310_data *data)
+{
+	struct regmap *regmap;
+	struct i2c_client *client;
+
+	client = data->client;
+	regmap = devm_regmap_init_i2c(client, &stk3310_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&client->dev, "regmap initialization failed.\n");
+		return PTR_ERR(regmap);
+	}
+	data->regmap = regmap;
+
+	data->reg_state = devm_regmap_field_alloc(&client->dev, regmap,
+			reg_field_state);
+	if (IS_ERR(data->reg_state)) {
+		dev_err(&client->dev, "reg field alloc failed.\n");
+		return PTR_ERR(data->reg_state);
+	}
+
+	data->reg_als_gain = devm_regmap_field_alloc(&client->dev, regmap,
+			reg_field_als_gain);
+	if (IS_ERR(data->reg_als_gain)) {
+		dev_err(&client->dev, "reg field alloc failed.\n");
+		return PTR_ERR(data->reg_als_gain);
+	}
+
+	data->reg_ps_gain = devm_regmap_field_alloc(&client->dev, regmap,
+			reg_field_ps_gain);
+	if (IS_ERR(data->reg_ps_gain)) {
+		dev_err(&client->dev, "reg field alloc failed.\n");
+		return PTR_ERR(data->reg_ps_gain);
+	}
+
+	data->reg_als_it = devm_regmap_field_alloc(&client->dev, regmap,
+			reg_field_als_it);
+	if (IS_ERR(data->reg_als_it)) {
+		dev_err(&client->dev, "reg field alloc failed.\n");
+		return PTR_ERR(data->reg_als_it);
+	}
+
+	data->reg_ps_it = devm_regmap_field_alloc(&client->dev, regmap,
+			reg_field_ps_it);
+	if (IS_ERR(data->reg_ps_it)) {
+		dev_err(&client->dev, "reg field alloc failed.\n");
+		return PTR_ERR(data->reg_ps_it);
+	}
+
+	return 0;
+}
+
+static int stk3310_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	int ret;
+	struct iio_dev *indio_dev;
+	struct stk3310_data *data;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev) {
+		dev_err(&client->dev, "iio allocation failed!\n");
+		return -ENOMEM;
+	}
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+	i2c_set_clientdata(client, indio_dev);
+	mutex_init(&data->lock);
+
+	ret = stk3310_regmap_init(data);
+	if (ret < 0)
+		return ret;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &stk3310_info;
+	indio_dev->name = STK3310_DRIVER_NAME;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = stk3310_channels;
+	indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
+
+	ret = stk3310_init(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "sensor initialization failed\n");
+		return ret;
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "device_register failed\n");
+		stk3310_set_state(data, STK3310_STATE_STANDBY);
+	}
+
+	return ret;
+}
+
+static int stk3310_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+	return stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int stk3310_suspend(struct device *dev)
+{
+	struct stk3310_data *data;
+
+	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
+
+	return stk3310_set_state(data, STK3310_STATE_STANDBY);
+}
+
+static int stk3310_resume(struct device *dev)
+{
+	int state = 0;
+	struct stk3310_data *data;
+
+	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
+	if (data->ps_enabled)
+		state |= STK3310_STATE_EN_PS;
+	if (data->als_enabled)
+		state |= STK3310_STATE_EN_ALS;
+
+	return stk3310_set_state(data, state);
+}
+
+static SIMPLE_DEV_PM_OPS(stk3310_pm_ops, stk3310_suspend, stk3310_resume);
+
+#define STK3310_PM_OPS (&stk3310_pm_ops)
+#else
+#define STK3310_PM_OPS NULL
+#endif
+
+static const struct i2c_device_id stk3310_i2c_id[] = {
+	{"STK3310", 0},
+	{"STK3311", 0},
+	{}
+};
+
+static const struct acpi_device_id stk3310_acpi_id[] = {
+	{"STK3310", 0},
+	{"STK3311", 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(acpi, stk3310_acpi_id);
+
+static struct i2c_driver stk3310_driver = {
+	.driver = {
+		.name = "stk3310",
+		.pm = STK3310_PM_OPS,
+		.acpi_match_table = ACPI_PTR(stk3310_acpi_id),
+	},
+	.probe =            stk3310_probe,
+	.remove =           stk3310_remove,
+	.id_table =         stk3310_i2c_id,
+};
+
+module_i2c_driver(stk3310_driver);
+
+MODULE_AUTHOR("Tiberiu Breana <tiberiu.a.breana@intel.com>");
+MODULE_DESCRIPTION("STK3310 Ambient Light and Proximity Sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v2 1/2] iio: light: Add support for Sensortek STK3310

[Peter Meerwald]

> Minimal implementation of an IIO driver for the Sensortek
> STK3310 ambient light and proximity sensor. The STK3311
> model is also supported.
> 
> Includes:
> - ACPI support;
> - read_raw and write_raw;
> - reading and setting configuration parameters for gain/scale
>   and integration time for both ALS and PS.
> - power management

please find my comments below
 
> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
> ---
>  drivers/iio/light/Kconfig   |  11 +
>  drivers/iio/light/Makefile  |   1 +
>  drivers/iio/light/stk3310.c | 508 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 520 insertions(+)
>  create mode 100644 drivers/iio/light/stk3310.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 01a1a16..096129d 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -174,6 +174,17 @@ config LTR501
>  	 This driver can also be built as a module.  If so, the module
>           will be called ltr501.
>  
> +config STK3310
> +	tristate "STK3310 ALS and proximity sensor"
> +	depends on I2C
> +	help
> +	 Say yes here to get support for the Sensortek STK3310 ambient light
> +	 and proximity sensor. The STK3311 model is also supported by this
> +	 driver.
> +
> +	 Choosing M will build the driver as a module. If so, the module
> +	 will be called stk3310.
> +
>  config TCS3414
>  	tristate "TAOS TCS3414 digital color sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index ad7c30f..90e7fd2 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_JSA1212)		+= jsa1212.o
>  obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
>  obj-$(CONFIG_LTR501)		+= ltr501.o
>  obj-$(CONFIG_SENSORS_TSL2563)	+= tsl2563.o
> +obj-$(CONFIG_STK3310)          += stk3310.o
>  obj-$(CONFIG_TCS3414)		+= tcs3414.o
>  obj-$(CONFIG_TCS3472)		+= tcs3472.o
>  obj-$(CONFIG_TSL4531)		+= tsl4531.o
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> new file mode 100644
> index 0000000..825a65a
> --- /dev/null
> +++ b/drivers/iio/light/stk3310.c
> @@ -0,0 +1,508 @@
> +/**
> + * Sensortek STK3310/STK3311 Ambient Light and Proximity Sensor
> + *
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License. See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * IIO driver for STK3310/STK3311. 7-bit I2C address: 0x48.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define STK3310_REG_STATE			0x00
> +#define STK3310_REG_PSCTRL			0x01
> +#define STK3310_REG_ALSCTRL			0x02
> +#define STK3310_REG_PS_DATA_MSB			0x11
> +#define STK3310_REG_PS_DATA_LSB			0x12
> +#define STK3310_REG_ALS_DATA_MSB		0x13
> +#define STK3310_REG_ALS_DATA_LSB		0x14
> +#define STK3310_REG_ID				0x3E
> +#define STK3310_MAX_REG				0x80
> +
> +#define STK3310_STATE_EN_PS			0x01
> +#define STK3310_STATE_EN_ALS			0x02
> +#define STK3310_STATE_STANDBY			0x00
> +
> +#define STK3310_CHIP_ID_VAL			0x13
> +#define STK3311_CHIP_ID_VAL			0x1D
> +#define STK3310_PS_MAX_VAL			0xffff

probably 0xFFFF to have all-uppercase hex constants everywhere

> +
> +#define STK3310_SCALE_AVAILABLE			"6.4 1.6 0.4 0.1"
> +
> +#define STK3310_DRIVER_NAME			"stk3310"
> +#define STK3310_REGMAP_NAME			"stk3310_regmap"
> +
> +static const struct reg_field reg_field_state =

stk3310_ prefix

> +				REG_FIELD(STK3310_REG_STATE, 0, 2);
> +static const struct reg_field reg_field_als_gain =
> +				REG_FIELD(STK3310_REG_ALSCTRL, 4, 5);
> +static const struct reg_field reg_field_ps_gain =
> +				REG_FIELD(STK3310_REG_PSCTRL, 4, 5);
> +static const struct reg_field reg_field_als_it =
> +				REG_FIELD(STK3310_REG_ALSCTRL, 0, 3);
> +static const struct reg_field reg_field_ps_it =
> +				REG_FIELD(STK3310_REG_PSCTRL, 0, 3);
> +
> +/*
> + * Maximum PS values with regard to scale. Used to export the 'inverse'
> + * PS value (high values for far objects, low values for near objects).
> + */
> +static const int stk3310_ps_max[4] = {
> +	STK3310_PS_MAX_VAL / 64,
> +	STK3310_PS_MAX_VAL / 16,
> +	STK3310_PS_MAX_VAL /  4,
> +	STK3310_PS_MAX_VAL,
> +};
> +
> +static const int stk3310_scale_table[][2] = {
> +	{6, 400000}, {1, 600000}, {0, 400000}, {0, 100000}
> +};
> +
> +/* Integration time in seconds, microseconds */
> +static const int stk3310_it_table[][2] = {
> +	{0, 185},	{0, 370},	{0, 741},	{0, 1480},
> +	{0, 2960},	{0, 5920},	{0, 11840},	{0, 23680},
> +	{0, 47360},	{0, 94720},	{0, 189440},	{0, 378880},
> +	{0, 757760},	{1, 515520},	{3, 31040},	{6, 62080},
> +};
> +
> +struct stk3310_data {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +	bool als_enabled;
> +	bool ps_enabled;
> +	struct regmap *regmap;
> +	struct regmap_field *reg_state;
> +	struct regmap_field *reg_als_gain;
> +	struct regmap_field *reg_ps_gain;
> +	struct regmap_field *reg_als_it;
> +	struct regmap_field *reg_ps_it;
> +};
> +
> +static const struct iio_chan_spec stk3310_channels[] = {
> +	{
> +		.channel = 0,

no need for channel number, the channels have distinct names due to their 
type

> +		.type = IIO_LIGHT,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE) |
> +			BIT(IIO_CHAN_INFO_INT_TIME),
> +	},
> +	{
> +		.channel = 1,
> +		.type = IIO_PROXIMITY,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE) |
> +			BIT(IIO_CHAN_INFO_INT_TIME),
> +	}
> +};
> +
> +static ssize_t stk3310_get_it_vals(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	int i;
> +	int len;
> +
> +	for (i = 0, len = 0; i < ARRAY_SIZE(stk3310_it_table); i++) {
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
> +				stk3310_it_table[i][0],
> +				stk3310_it_table[i][1]);
> +	}
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static IIO_CONST_ATTR(in_illuminance_scale_available, STK3310_SCALE_AVAILABLE);
> +
> +static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,

these should be IIO_CONST_ATTR() as well?

> +		       S_IRUGO, stk3310_get_it_vals, NULL, 0);
> +
> +static IIO_DEVICE_ATTR(in_proximity_integration_time_available,
> +		       S_IRUGO, stk3310_get_it_vals, NULL, 0);
> +
> +static struct attribute *stk3310_attributes[] = {
> +	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,

where is proximity_scale_available?

> +	&iio_dev_attr_in_proximity_integration_time_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group stk3310_attribute_group = {
> +	.attrs = stk3310_attributes
> +};
> +
> +static int stk3310_get_index(const int table[][2], int table_size,

this function appears over and over in IIO... maybe have a generic one?

> +			     int val, int val2)
> +{
> +	int i;
> +
> +	for (i = 0; i < table_size; i++) {
> +		if (val == table[i][0] && val2 == table[i][1])
> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int stk3310_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	u8 reg;
> +	u16 buf;
> +	unsigned int index;
> +	struct stk3310_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +
> +	switch (chan->type) {

this block makes only sense for _INFO_RAW and should be moved there

> +	case IIO_LIGHT:
> +		reg = STK3310_REG_ALS_DATA_MSB;
> +		break;
> +	case IIO_PROXIMITY:
> +		reg = STK3310_REG_PS_DATA_MSB;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&data->lock);
> +		regmap_bulk_read(data->regmap, reg, &buf, 2);

regmap_bulk_read() returns an integer, let's check that...

> +		if (buf < 0) {

buf is u16, how can the be < 0?

> +			dev_err(&client->dev, "register read failed\n");
> +			mutex_unlock(&data->lock);
> +			return buf;
> +		}
> +		*val = swab16(buf);
> +		if (chan->type == IIO_PROXIMITY) {
> +			/*
> +			 * Invert the proximity data so we return low values
> +			 * for close objects and high values for far ones.
> +			 */

it would/should be _PROCESSED then due to the inversion?

other drivers report proximity such that close objects have high values 
and far objects have low values -- it is a reflection value really; I'd 
rather fix that in the documentation

> +			regmap_field_read(data->reg_ps_gain, &index);
> +			*val = stk3310_ps_max[index] - *val;
> +		}
> +		mutex_unlock(&data->lock);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (chan->type == IIO_LIGHT)
> +			regmap_field_read(data->reg_als_it, &index);
> +		else
> +			regmap_field_read(data->reg_ps_it, &index);
> +		*val = stk3310_it_table[index][0];
> +		*val2 = stk3310_it_table[index][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_LIGHT)
> +			regmap_field_read(data->reg_als_gain, &index);
> +		else
> +			regmap_field_read(data->reg_ps_gain, &index);
> +		*val = stk3310_scale_table[index][0];
> +		*val2 = stk3310_scale_table[index][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int stk3310_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	int ret;
> +	unsigned int index;
> +	struct stk3310_data *data = iio_priv(indio_dev);
> +
> +	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
> +		return -EINVAL;

most drivers don't check the channel type and assume that chan points to 
one of the channels registered

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		index = stk3310_get_index(stk3310_it_table,
> +					  ARRAY_SIZE(stk3310_it_table),
> +					  val, val2);
> +		if (index < 0)
> +			return -EINVAL;
> +		mutex_lock(&data->lock);
> +		if (chan->type == IIO_LIGHT)
> +			ret = regmap_field_write(data->reg_als_it, index);
> +		else
> +			ret = regmap_field_write(data->reg_ps_it, index);
> +		if (ret < 0)
> +			dev_err(&data->client->dev,
> +					"sensor configuration failed\n");
> +		mutex_unlock(&data->lock);
> +		return ret;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		index = stk3310_get_index(stk3310_scale_table,
> +					  ARRAY_SIZE(stk3310_scale_table),
> +					  val, val2);
> +		if (index < 0)
> +			return -EINVAL;
> +		mutex_lock(&data->lock);
> +		if (chan->type == IIO_LIGHT)
> +			ret = regmap_field_write(data->reg_als_gain, index);
> +		else
> +			ret = regmap_field_write(data->reg_ps_gain, index);
> +		if (ret < 0)
> +			dev_err(&data->client->dev,
> +					"sensor configuration failed\n");
> +		mutex_unlock(&data->lock);
> +		return ret;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info stk3310_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= stk3310_read_raw,
> +	.write_raw		= stk3310_write_raw,
> +	.attrs			= &stk3310_attribute_group,
> +};
> +
> +static int stk3310_set_state(struct stk3310_data *data, u8 state)
> +{
> +	int ret;
> +	struct i2c_client *client = data->client;
> +
> +	/* 3-bit state; 0b100 is not supported. */
> +	if (state < 0 || state > 7 || state == 4)

state is u8, can't be negative

> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +	ret = regmap_field_write(data->reg_state, state);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to change sensor state\n");
> +	/* Don't reset the 'enabled' flags if we're going in standby */

move the comment to the block where it belongs to

> +	} else if (state != STK3310_STATE_STANDBY) {
> +		data->ps_enabled  = !!(state & 0x01);
> +		data->als_enabled = !!(state & 0x02);
> +	}
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int stk3310_init(struct iio_dev *indio_dev)
> +{
> +	int ret;
> +	int chipid;
> +	u8 state;
> +	struct stk3310_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +
> +	regmap_read(data->regmap, STK3310_REG_ID, &chipid);
> +	if (chipid != STK3310_CHIP_ID_VAL &&
> +	    chipid != STK3311_CHIP_ID_VAL) {
> +		dev_err(&client->dev, "invalid chip id: 0x%x\n", chipid);
> +		return -ENODEV;
> +	}
> +
> +	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
> +	ret = stk3310_set_state(data, state);
> +	if (ret < 0)
> +		dev_err(&client->dev, "failed to enable sensor");
> +
> +	return ret;
> +}
> +
> +static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case STK3310_REG_ALS_DATA_MSB:
> +	case STK3310_REG_ALS_DATA_LSB:
> +	case STK3310_REG_PS_DATA_LSB:
> +	case STK3310_REG_PS_DATA_MSB:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static struct regmap_config stk3310_regmap_config = {
> +	.name = STK3310_REGMAP_NAME,
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = STK3310_MAX_REG,
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_reg = stk3310_is_volatile_reg,
> +};
> +
> +static int stk3310_regmap_init(struct stk3310_data *data)
> +{
> +	struct regmap *regmap;
> +	struct i2c_client *client;
> +
> +	client = data->client;
> +	regmap = devm_regmap_init_i2c(client, &stk3310_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "regmap initialization failed.\n");
> +		return PTR_ERR(regmap);
> +	}
> +	data->regmap = regmap;
> +

code below is clumsy; many lines for little effect, maybe a helper 
function/macro?

> +	data->reg_state = devm_regmap_field_alloc(&client->dev, regmap,
> +			reg_field_state);
> +	if (IS_ERR(data->reg_state)) {
> +		dev_err(&client->dev, "reg field alloc failed.\n");
> +		return PTR_ERR(data->reg_state);
> +	}
> +
> +	data->reg_als_gain = devm_regmap_field_alloc(&client->dev, regmap,
> +			reg_field_als_gain);
> +	if (IS_ERR(data->reg_als_gain)) {
> +		dev_err(&client->dev, "reg field alloc failed.\n");
> +		return PTR_ERR(data->reg_als_gain);
> +	}
> +
> +	data->reg_ps_gain = devm_regmap_field_alloc(&client->dev, regmap,
> +			reg_field_ps_gain);
> +	if (IS_ERR(data->reg_ps_gain)) {
> +		dev_err(&client->dev, "reg field alloc failed.\n");
> +		return PTR_ERR(data->reg_ps_gain);
> +	}
> +
> +	data->reg_als_it = devm_regmap_field_alloc(&client->dev, regmap,
> +			reg_field_als_it);
> +	if (IS_ERR(data->reg_als_it)) {
> +		dev_err(&client->dev, "reg field alloc failed.\n");
> +		return PTR_ERR(data->reg_als_it);
> +	}
> +
> +	data->reg_ps_it = devm_regmap_field_alloc(&client->dev, regmap,
> +			reg_field_ps_it);
> +	if (IS_ERR(data->reg_ps_it)) {
> +		dev_err(&client->dev, "reg field alloc failed.\n");
> +		return PTR_ERR(data->reg_ps_it);
> +	}
> +
> +	return 0;
> +}
> +
> +static int stk3310_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev;
> +	struct stk3310_data *data;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev) {
> +		dev_err(&client->dev, "iio allocation failed!\n");
> +		return -ENOMEM;
> +	}
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +	i2c_set_clientdata(client, indio_dev);
> +	mutex_init(&data->lock);
> +
> +	ret = stk3310_regmap_init(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &stk3310_info;
> +	indio_dev->name = STK3310_DRIVER_NAME;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = stk3310_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
> +
> +	ret = stk3310_init(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "sensor initialization failed\n");

we already printed a error message in stk3310_init()...

> +		return ret;
> +	}
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "device_register failed\n");
> +		stk3310_set_state(data, STK3310_STATE_STANDBY);
> +	}
> +
> +	return ret;
> +}
> +
> +static int stk3310_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
> +	return stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int stk3310_suspend(struct device *dev)
> +{
> +	struct stk3310_data *data;
> +
> +	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> +
> +	return stk3310_set_state(data, STK3310_STATE_STANDBY);
> +}
> +
> +static int stk3310_resume(struct device *dev)
> +{
> +	int state = 0;
> +	struct stk3310_data *data;
> +
> +	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> +	if (data->ps_enabled)
> +		state |= STK3310_STATE_EN_PS;
> +	if (data->als_enabled)
> +		state |= STK3310_STATE_EN_ALS;
> +
> +	return stk3310_set_state(data, state);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(stk3310_pm_ops, stk3310_suspend, stk3310_resume);
> +
> +#define STK3310_PM_OPS (&stk3310_pm_ops)
> +#else
> +#define STK3310_PM_OPS NULL
> +#endif
> +
> +static const struct i2c_device_id stk3310_i2c_id[] = {
> +	{"STK3310", 0},
> +	{"STK3311", 0},

if there is no software-visible difference, I'd rather not distinguish the 
IDs; otherwise, you could/should check that the ID here matches the ID 
returned from the device

> +	{}
> +};
> +
> +static const struct acpi_device_id stk3310_acpi_id[] = {
> +	{"STK3310", 0},
> +	{"STK3311", 0},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, stk3310_acpi_id);
> +
> +static struct i2c_driver stk3310_driver = {
> +	.driver = {
> +		.name = "stk3310",
> +		.pm = STK3310_PM_OPS,
> +		.acpi_match_table = ACPI_PTR(stk3310_acpi_id),
> +	},
> +	.probe =            stk3310_probe,
> +	.remove =           stk3310_remove,
> +	.id_table =         stk3310_i2c_id,
> +};
> +
> +module_i2c_driver(stk3310_driver);
> +
> +MODULE_AUTHOR("Tiberiu Breana <tiberiu.a.breana@intel.com>");
> +MODULE_DESCRIPTION("STK3310 Ambient Light and Proximity Sensor driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH v2 1/2] iio: light: Add support for Sensortek STK3310

[Jonathan Cameron]

On 24/04/15 17:41, Peter Meerwald wrote:
> 
>> Minimal implementation of an IIO driver for the Sensortek
>> STK3310 ambient light and proximity sensor. The STK3311
>> model is also supported.
>>
>> Includes:
>> - ACPI support;
>> - read_raw and write_raw;
>> - reading and setting configuration parameters for gain/scale
>>   and integration time for both ALS and PS.
>> - power management
> 
> please find my comments below
Nice review Peter.  I missed loads of stuff in V1 didn't I (oops)

Anyhow, all minor stuff Tiberiu so looking forward to v3.
>  
>> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
>> ---
>>  drivers/iio/light/Kconfig   |  11 +
>>  drivers/iio/light/Makefile  |   1 +
>>  drivers/iio/light/stk3310.c | 508 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 520 insertions(+)
>>  create mode 100644 drivers/iio/light/stk3310.c
>>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index 01a1a16..096129d 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -174,6 +174,17 @@ config LTR501
>>  	 This driver can also be built as a module.  If so, the module
>>           will be called ltr501.
>>  
>> +config STK3310
>> +	tristate "STK3310 ALS and proximity sensor"
>> +	depends on I2C
>> +	help
>> +	 Say yes here to get support for the Sensortek STK3310 ambient light
>> +	 and proximity sensor. The STK3311 model is also supported by this
>> +	 driver.
>> +
>> +	 Choosing M will build the driver as a module. If so, the module
>> +	 will be called stk3310.
>> +
>>  config TCS3414
>>  	tristate "TAOS TCS3414 digital color sensor"
>>  	depends on I2C
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index ad7c30f..90e7fd2 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_JSA1212)		+= jsa1212.o
>>  obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
>>  obj-$(CONFIG_LTR501)		+= ltr501.o
>>  obj-$(CONFIG_SENSORS_TSL2563)	+= tsl2563.o
>> +obj-$(CONFIG_STK3310)          += stk3310.o
>>  obj-$(CONFIG_TCS3414)		+= tcs3414.o
>>  obj-$(CONFIG_TCS3472)		+= tcs3472.o
>>  obj-$(CONFIG_TSL4531)		+= tsl4531.o
>> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
>> new file mode 100644
>> index 0000000..825a65a
>> --- /dev/null
>> +++ b/drivers/iio/light/stk3310.c
>> @@ -0,0 +1,508 @@
>> +/**
>> + * Sensortek STK3310/STK3311 Ambient Light and Proximity Sensor
>> + *
>> + * Copyright (c) 2015, Intel Corporation.
>> + *
>> + * This file is subject to the terms and conditions of version 2 of
>> + * the GNU General Public License. See the file COPYING in the main
>> + * directory of this archive for more details.
>> + *
>> + * IIO driver for STK3310/STK3311. 7-bit I2C address: 0x48.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define STK3310_REG_STATE			0x00
>> +#define STK3310_REG_PSCTRL			0x01
>> +#define STK3310_REG_ALSCTRL			0x02
>> +#define STK3310_REG_PS_DATA_MSB			0x11
>> +#define STK3310_REG_PS_DATA_LSB			0x12
>> +#define STK3310_REG_ALS_DATA_MSB		0x13
>> +#define STK3310_REG_ALS_DATA_LSB		0x14
>> +#define STK3310_REG_ID				0x3E
>> +#define STK3310_MAX_REG				0x80
>> +
>> +#define STK3310_STATE_EN_PS			0x01
>> +#define STK3310_STATE_EN_ALS			0x02
>> +#define STK3310_STATE_STANDBY			0x00
>> +
>> +#define STK3310_CHIP_ID_VAL			0x13
>> +#define STK3311_CHIP_ID_VAL			0x1D
>> +#define STK3310_PS_MAX_VAL			0xffff
> 
> probably 0xFFFF to have all-uppercase hex constants everywhere
> 
>> +
>> +#define STK3310_SCALE_AVAILABLE			"6.4 1.6 0.4 0.1"
>> +
>> +#define STK3310_DRIVER_NAME			"stk3310"
>> +#define STK3310_REGMAP_NAME			"stk3310_regmap"
>> +
>> +static const struct reg_field reg_field_state =
> 
> stk3310_ prefix
> 
>> +				REG_FIELD(STK3310_REG_STATE, 0, 2);
>> +static const struct reg_field reg_field_als_gain =
>> +				REG_FIELD(STK3310_REG_ALSCTRL, 4, 5);
>> +static const struct reg_field reg_field_ps_gain =
>> +				REG_FIELD(STK3310_REG_PSCTRL, 4, 5);
>> +static const struct reg_field reg_field_als_it =
>> +				REG_FIELD(STK3310_REG_ALSCTRL, 0, 3);
>> +static const struct reg_field reg_field_ps_it =
>> +				REG_FIELD(STK3310_REG_PSCTRL, 0, 3);
>> +
>> +/*
>> + * Maximum PS values with regard to scale. Used to export the 'inverse'
>> + * PS value (high values for far objects, low values for near objects).
>> + */
>> +static const int stk3310_ps_max[4] = {
>> +	STK3310_PS_MAX_VAL / 64,
>> +	STK3310_PS_MAX_VAL / 16,
>> +	STK3310_PS_MAX_VAL /  4,
>> +	STK3310_PS_MAX_VAL,
>> +};
>> +
>> +static const int stk3310_scale_table[][2] = {
>> +	{6, 400000}, {1, 600000}, {0, 400000}, {0, 100000}
>> +};
>> +
>> +/* Integration time in seconds, microseconds */
>> +static const int stk3310_it_table[][2] = {
>> +	{0, 185},	{0, 370},	{0, 741},	{0, 1480},
>> +	{0, 2960},	{0, 5920},	{0, 11840},	{0, 23680},
>> +	{0, 47360},	{0, 94720},	{0, 189440},	{0, 378880},
>> +	{0, 757760},	{1, 515520},	{3, 31040},	{6, 62080},
>> +};
>> +
>> +struct stk3310_data {
>> +	struct i2c_client *client;
>> +	struct mutex lock;
>> +	bool als_enabled;
>> +	bool ps_enabled;
>> +	struct regmap *regmap;
>> +	struct regmap_field *reg_state;
>> +	struct regmap_field *reg_als_gain;
>> +	struct regmap_field *reg_ps_gain;
>> +	struct regmap_field *reg_als_it;
>> +	struct regmap_field *reg_ps_it;
>> +};
>> +
>> +static const struct iio_chan_spec stk3310_channels[] = {
>> +	{
>> +		.channel = 0,
> 
> no need for channel number, the channels have distinct names due to their 
> type
Also, without indexed being set it won't be used in the naming anyway.
> 
>> +		.type = IIO_LIGHT,
>> +		.info_mask_separate =
>> +			BIT(IIO_CHAN_INFO_RAW) |
>> +			BIT(IIO_CHAN_INFO_SCALE) |
>> +			BIT(IIO_CHAN_INFO_INT_TIME),
>> +	},
>> +	{
>> +		.channel = 1,
>> +		.type = IIO_PROXIMITY,
>> +		.info_mask_separate =
>> +			BIT(IIO_CHAN_INFO_RAW) |
>> +			BIT(IIO_CHAN_INFO_SCALE) |
>> +			BIT(IIO_CHAN_INFO_INT_TIME),
>> +	}
>> +};
>> +
>> +static ssize_t stk3310_get_it_vals(struct device *dev,
>> +				   struct device_attribute *attr, char *buf)
>> +{
>> +	int i;
>> +	int len;
>> +
>> +	for (i = 0, len = 0; i < ARRAY_SIZE(stk3310_it_table); i++) {
>> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
>> +				stk3310_it_table[i][0],
>> +				stk3310_it_table[i][1]);
>> +	}
>> +	buf[len - 1] = '\n';
>> +
>> +	return len;
>> +}
>> +
>> +static IIO_CONST_ATTR(in_illuminance_scale_available, STK3310_SCALE_AVAILABLE);
>> +
>> +static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,
> 
> these should be IIO_CONST_ATTR() as well?
> 
>> +		       S_IRUGO, stk3310_get_it_vals, NULL, 0);
>> +
>> +static IIO_DEVICE_ATTR(in_proximity_integration_time_available,
>> +		       S_IRUGO, stk3310_get_it_vals, NULL, 0);
>> +
>> +static struct attribute *stk3310_attributes[] = {
>> +	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
>> +	&iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
> 
> where is proximity_scale_available?
> 
>> +	&iio_dev_attr_in_proximity_integration_time_available.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group stk3310_attribute_group = {
>> +	.attrs = stk3310_attributes
>> +};
>> +
>> +static int stk3310_get_index(const int table[][2], int table_size,
> 
> this function appears over and over in IIO... maybe have a generic one?
Would probably make sense...  Go on Peter, it was your idea ;)
> 
>> +			     int val, int val2)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < table_size; i++) {
>> +		if (val == table[i][0] && val2 == table[i][1])
>> +			return i;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int stk3310_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	u8 reg;
>> +	u16 buf;
>> +	unsigned int index;
>> +	struct stk3310_data *data = iio_priv(indio_dev);
>> +	struct i2c_client *client = data->client;
>> +
>> +	switch (chan->type) {
> 
> this block makes only sense for _INFO_RAW and should be moved there
> 
>> +	case IIO_LIGHT:
>> +		reg = STK3310_REG_ALS_DATA_MSB;
>> +		break;
>> +	case IIO_PROXIMITY:
>> +		reg = STK3310_REG_PS_DATA_MSB;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&data->lock);
>> +		regmap_bulk_read(data->regmap, reg, &buf, 2);
> 
> regmap_bulk_read() returns an integer, let's check that...
> 
>> +		if (buf < 0) {
> 
> buf is u16, how can the be < 0?
> 
>> +			dev_err(&client->dev, "register read failed\n");
>> +			mutex_unlock(&data->lock);
>> +			return buf;
>> +		}
>> +		*val = swab16(buf);
>> +		if (chan->type == IIO_PROXIMITY) {
>> +			/*
>> +			 * Invert the proximity data so we return low values
>> +			 * for close objects and high values for far ones.
>> +			 */
> 
> it would/should be _PROCESSED then due to the inversion?
> 
> other drivers report proximity such that close objects have high values 
> and far objects have low values -- it is a reflection value really; I'd 
> rather fix that in the documentation
> 
>> +			regmap_field_read(data->reg_ps_gain, &index);
>> +			*val = stk3310_ps_max[index] - *val;
>> +		}
>> +		mutex_unlock(&data->lock);
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_INT_TIME:
>> +		if (chan->type == IIO_LIGHT)
>> +			regmap_field_read(data->reg_als_it, &index);
>> +		else
>> +			regmap_field_read(data->reg_ps_it, &index);
>> +		*val = stk3310_it_table[index][0];
>> +		*val2 = stk3310_it_table[index][1];
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (chan->type == IIO_LIGHT)
>> +			regmap_field_read(data->reg_als_gain, &index);
>> +		else
>> +			regmap_field_read(data->reg_ps_gain, &index);
>> +		*val = stk3310_scale_table[index][0];
>> +		*val2 = stk3310_scale_table[index][1];
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int stk3310_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int val, int val2, long mask)
>> +{
>> +	int ret;
>> +	unsigned int index;
>> +	struct stk3310_data *data = iio_priv(indio_dev);
>> +
>> +	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
>> +		return -EINVAL;
> 
> most drivers don't check the channel type and assume that chan points to 
> one of the channels registered
> 
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_INT_TIME:
>> +		index = stk3310_get_index(stk3310_it_table,
>> +					  ARRAY_SIZE(stk3310_it_table),
>> +					  val, val2);
>> +		if (index < 0)
>> +			return -EINVAL;
>> +		mutex_lock(&data->lock);
>> +		if (chan->type == IIO_LIGHT)
>> +			ret = regmap_field_write(data->reg_als_it, index);
>> +		else
>> +			ret = regmap_field_write(data->reg_ps_it, index);
>> +		if (ret < 0)
>> +			dev_err(&data->client->dev,
>> +					"sensor configuration failed\n");
>> +		mutex_unlock(&data->lock);
>> +		return ret;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		index = stk3310_get_index(stk3310_scale_table,
>> +					  ARRAY_SIZE(stk3310_scale_table),
>> +					  val, val2);
>> +		if (index < 0)
>> +			return -EINVAL;
>> +		mutex_lock(&data->lock);
>> +		if (chan->type == IIO_LIGHT)
>> +			ret = regmap_field_write(data->reg_als_gain, index);
>> +		else
>> +			ret = regmap_field_write(data->reg_ps_gain, index);
>> +		if (ret < 0)
>> +			dev_err(&data->client->dev,
>> +					"sensor configuration failed\n");
>> +		mutex_unlock(&data->lock);
>> +		return ret;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info stk3310_info = {
>> +	.driver_module		= THIS_MODULE,
>> +	.read_raw		= stk3310_read_raw,
>> +	.write_raw		= stk3310_write_raw,
>> +	.attrs			= &stk3310_attribute_group,
>> +};
>> +
>> +static int stk3310_set_state(struct stk3310_data *data, u8 state)
>> +{
>> +	int ret;
>> +	struct i2c_client *client = data->client;
>> +
>> +	/* 3-bit state; 0b100 is not supported. */
>> +	if (state < 0 || state > 7 || state == 4)
> 
> state is u8, can't be negative
> 
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&data->lock);
>> +	ret = regmap_field_write(data->reg_state, state);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "failed to change sensor state\n");
>> +	/* Don't reset the 'enabled' flags if we're going in standby */
> 
> move the comment to the block where it belongs to
> 
>> +	} else if (state != STK3310_STATE_STANDBY) {
>> +		data->ps_enabled  = !!(state & 0x01);
>> +		data->als_enabled = !!(state & 0x02);
>> +	}
>> +	mutex_unlock(&data->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stk3310_init(struct iio_dev *indio_dev)
>> +{
>> +	int ret;
>> +	int chipid;
>> +	u8 state;
>> +	struct stk3310_data *data = iio_priv(indio_dev);
>> +	struct i2c_client *client = data->client;
>> +
>> +	regmap_read(data->regmap, STK3310_REG_ID, &chipid);
>> +	if (chipid != STK3310_CHIP_ID_VAL &&
>> +	    chipid != STK3311_CHIP_ID_VAL) {
>> +		dev_err(&client->dev, "invalid chip id: 0x%x\n", chipid);
>> +		return -ENODEV;
>> +	}
>> +
>> +	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
>> +	ret = stk3310_set_state(data, state);
>> +	if (ret < 0)
>> +		dev_err(&client->dev, "failed to enable sensor");
>> +
>> +	return ret;
>> +}
>> +
>> +static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> +	switch (reg) {
>> +	case STK3310_REG_ALS_DATA_MSB:
>> +	case STK3310_REG_ALS_DATA_LSB:
>> +	case STK3310_REG_PS_DATA_LSB:
>> +	case STK3310_REG_PS_DATA_MSB:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static struct regmap_config stk3310_regmap_config = {
>> +	.name = STK3310_REGMAP_NAME,
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.max_register = STK3310_MAX_REG,
>> +	.cache_type = REGCACHE_RBTREE,
>> +	.volatile_reg = stk3310_is_volatile_reg,
>> +};
>> +
>> +static int stk3310_regmap_init(struct stk3310_data *data)
>> +{
>> +	struct regmap *regmap;
>> +	struct i2c_client *client;
>> +
>> +	client = data->client;
>> +	regmap = devm_regmap_init_i2c(client, &stk3310_regmap_config);
>> +	if (IS_ERR(regmap)) {
>> +		dev_err(&client->dev, "regmap initialization failed.\n");
>> +		return PTR_ERR(regmap);
>> +	}
>> +	data->regmap = regmap;
>> +
> 
> code below is clumsy; many lines for little effect, maybe a helper 
> function/macro?
> 
>> +	data->reg_state = devm_regmap_field_alloc(&client->dev, regmap,
>> +			reg_field_state);
>> +	if (IS_ERR(data->reg_state)) {
>> +		dev_err(&client->dev, "reg field alloc failed.\n");
>> +		return PTR_ERR(data->reg_state);
>> +	}
>> +
>> +	data->reg_als_gain = devm_regmap_field_alloc(&client->dev, regmap,
>> +			reg_field_als_gain);
>> +	if (IS_ERR(data->reg_als_gain)) {
>> +		dev_err(&client->dev, "reg field alloc failed.\n");
>> +		return PTR_ERR(data->reg_als_gain);
>> +	}
>> +
>> +	data->reg_ps_gain = devm_regmap_field_alloc(&client->dev, regmap,
>> +			reg_field_ps_gain);
>> +	if (IS_ERR(data->reg_ps_gain)) {
>> +		dev_err(&client->dev, "reg field alloc failed.\n");
>> +		return PTR_ERR(data->reg_ps_gain);
>> +	}
>> +
>> +	data->reg_als_it = devm_regmap_field_alloc(&client->dev, regmap,
>> +			reg_field_als_it);
>> +	if (IS_ERR(data->reg_als_it)) {
>> +		dev_err(&client->dev, "reg field alloc failed.\n");
>> +		return PTR_ERR(data->reg_als_it);
>> +	}
>> +
>> +	data->reg_ps_it = devm_regmap_field_alloc(&client->dev, regmap,
>> +			reg_field_ps_it);
>> +	if (IS_ERR(data->reg_ps_it)) {
>> +		dev_err(&client->dev, "reg field alloc failed.\n");
>> +		return PTR_ERR(data->reg_ps_it);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int stk3310_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	int ret;
>> +	struct iio_dev *indio_dev;
>> +	struct stk3310_data *data;
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> +	if (!indio_dev) {
>> +		dev_err(&client->dev, "iio allocation failed!\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	data = iio_priv(indio_dev);
>> +	data->client = client;
>> +	i2c_set_clientdata(client, indio_dev);
>> +	mutex_init(&data->lock);
>> +
>> +	ret = stk3310_regmap_init(data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	indio_dev->dev.parent = &client->dev;
>> +	indio_dev->info = &stk3310_info;
>> +	indio_dev->name = STK3310_DRIVER_NAME;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = stk3310_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
>> +
>> +	ret = stk3310_init(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "sensor initialization failed\n");
> 
> we already printed a error message in stk3310_init()...
> 
>> +		return ret;
>> +	}
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "device_register failed\n");
>> +		stk3310_set_state(data, STK3310_STATE_STANDBY);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int stk3310_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +
>> +	iio_device_unregister(indio_dev);
>> +	return stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int stk3310_suspend(struct device *dev)
>> +{
>> +	struct stk3310_data *data;
>> +
>> +	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
>> +
>> +	return stk3310_set_state(data, STK3310_STATE_STANDBY);
>> +}
>> +
>> +static int stk3310_resume(struct device *dev)
>> +{
>> +	int state = 0;
>> +	struct stk3310_data *data;
>> +
>> +	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
>> +	if (data->ps_enabled)
>> +		state |= STK3310_STATE_EN_PS;
>> +	if (data->als_enabled)
>> +		state |= STK3310_STATE_EN_ALS;
>> +
>> +	return stk3310_set_state(data, state);
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(stk3310_pm_ops, stk3310_suspend, stk3310_resume);
>> +
>> +#define STK3310_PM_OPS (&stk3310_pm_ops)
>> +#else
>> +#define STK3310_PM_OPS NULL
>> +#endif
>> +
>> +static const struct i2c_device_id stk3310_i2c_id[] = {
>> +	{"STK3310", 0},
>> +	{"STK3311", 0},
> 
> if there is no software-visible difference, I'd rather not distinguish the 
> IDs; otherwise, you could/should check that the ID here matches the ID 
> returned from the device
Except that device tree writers are going to expect the id to match their
part number. So I'd keep them both even if they can't be distinguished.
(of course if they can, do check it!)
> 
>> +	{}
>> +};
>> +
>> +static const struct acpi_device_id stk3310_acpi_id[] = {
>> +	{"STK3310", 0},
>> +	{"STK3311", 0},
>> +	{}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(acpi, stk3310_acpi_id);
>> +
>> +static struct i2c_driver stk3310_driver = {
>> +	.driver = {
>> +		.name = "stk3310",
>> +		.pm = STK3310_PM_OPS,
>> +		.acpi_match_table = ACPI_PTR(stk3310_acpi_id),
>> +	},
>> +	.probe =            stk3310_probe,
>> +	.remove =           stk3310_remove,
>> +	.id_table =         stk3310_i2c_id,
>> +};
>> +
>> +module_i2c_driver(stk3310_driver);
>> +
>> +MODULE_AUTHOR("Tiberiu Breana <tiberiu.a.breana@intel.com>");
>> +MODULE_DESCRIPTION("STK3310 Ambient Light and Proximity Sensor driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 

RE: [PATCH v2 1/2] iio: light: Add support for Sensortek STK3310

[Breana, Tiberiu A]

Thanks for the reviews. I've added some notes inline.
Patch v3 will be up soon.

Tiberiu
________________________________________
From: Jonathan Cameron [jic23@kernel.org]
Sent: Sunday, April 26, 2015 7:55 PM
To: Peter Meerwald; Breana, Tiberiu A
Cc: linux-iio@vger.kernel.org
Subject: Re: [PATCH v2 1/2] iio: light: Add support for Sensortek STK3310

On 24/04/15 17:41, Peter Meerwald wrote:
>
>> Minimal implementation of an IIO driver for the Sensortek
>> STK3310 ambient light and proximity sensor. The STK3311
>> model is also supported.
>>
>> Includes:
>> - ACPI support;
>> - read_raw and write_raw;
>> - reading and setting configuration parameters for gain/scale
>>   and integration time for both ALS and PS.
>> - power management
>
> please find my comments below
Nice review Peter.  I missed loads of stuff in V1 didn't I (oops)

Anyhow, all minor stuff Tiberiu so looking forward to v3.
>
>> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
>> ---
>>  drivers/iio/light/Kconfig   |  11 +
>>  drivers/iio/light/Makefile  |   1 +
>>  drivers/iio/light/stk3310.c | 508 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 520 insertions(+)
>>  create mode 100644 drivers/iio/light/stk3310.c
>>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index 01a1a16..096129d 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -174,6 +174,17 @@ config LTR501
>>       This driver can also be built as a module.  If so, the module
>>           will be called ltr501.
>>
>> +config STK3310
>> +    tristate "STK3310 ALS and proximity sensor"
>> +    depends on I2C
>> +    help
>> +     Say yes here to get support for the Sensortek STK3310 ambient light
>> +     and proximity sensor. The STK3311 model is also supported by this
>> +     driver.
>> +
>> +     Choosing M will build the driver as a module. If so, the module
>> +     will be called stk3310.
>> +
>>  config TCS3414
>>      tristate "TAOS TCS3414 digital color sensor"
>>      depends on I2C
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index ad7c30f..90e7fd2 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_JSA1212)              += jsa1212.o
>>  obj-$(CONFIG_SENSORS_LM3533)        += lm3533-als.o
>>  obj-$(CONFIG_LTR501)                += ltr501.o
>>  obj-$(CONFIG_SENSORS_TSL2563)       += tsl2563.o
>> +obj-$(CONFIG_STK3310)          += stk3310.o
>>  obj-$(CONFIG_TCS3414)               += tcs3414.o
>>  obj-$(CONFIG_TCS3472)               += tcs3472.o
>>  obj-$(CONFIG_TSL4531)               += tsl4531.o
>> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
>> new file mode 100644
>> index 0000000..825a65a
>> --- /dev/null
>> +++ b/drivers/iio/light/stk3310.c
>> @@ -0,0 +1,508 @@
>> +/**
>> + * Sensortek STK3310/STK3311 Ambient Light and Proximity Sensor
>> + *
>> + * Copyright (c) 2015, Intel Corporation.
>> + *
>> + * This file is subject to the terms and conditions of version 2 of
>> + * the GNU General Public License. See the file COPYING in the main
>> + * directory of this archive for more details.
>> + *
>> + * IIO driver for STK3310/STK3311. 7-bit I2C address: 0x48.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define STK3310_REG_STATE                   0x00
>> +#define STK3310_REG_PSCTRL                  0x01
>> +#define STK3310_REG_ALSCTRL                 0x02
>> +#define STK3310_REG_PS_DATA_MSB                     0x11
>> +#define STK3310_REG_PS_DATA_LSB                     0x12
>> +#define STK3310_REG_ALS_DATA_MSB            0x13
>> +#define STK3310_REG_ALS_DATA_LSB            0x14
>> +#define STK3310_REG_ID                              0x3E
>> +#define STK3310_MAX_REG                             0x80
>> +
>> +#define STK3310_STATE_EN_PS                 0x01
>> +#define STK3310_STATE_EN_ALS                        0x02
>> +#define STK3310_STATE_STANDBY                       0x00
>> +
>> +#define STK3310_CHIP_ID_VAL                 0x13
>> +#define STK3311_CHIP_ID_VAL                 0x1D
>> +#define STK3310_PS_MAX_VAL                  0xffff
>
> probably 0xFFFF to have all-uppercase hex constants everywhere
>
>> +
>> +#define STK3310_SCALE_AVAILABLE                     "6.4 1.6 0.4 0.1"
>> +
>> +#define STK3310_DRIVER_NAME                 "stk3310"
>> +#define STK3310_REGMAP_NAME                 "stk3310_regmap"
>> +
>> +static const struct reg_field reg_field_state =
>
> stk3310_ prefix
>
>> +                            REG_FIELD(STK3310_REG_STATE, 0, 2);
>> +static const struct reg_field reg_field_als_gain =
>> +                            REG_FIELD(STK3310_REG_ALSCTRL, 4, 5);
>> +static const struct reg_field reg_field_ps_gain =
>> +                            REG_FIELD(STK3310_REG_PSCTRL, 4, 5);
>> +static const struct reg_field reg_field_als_it =
>> +                            REG_FIELD(STK3310_REG_ALSCTRL, 0, 3);
>> +static const struct reg_field reg_field_ps_it =
>> +                            REG_FIELD(STK3310_REG_PSCTRL, 0, 3);
>> +
>> +/*
>> + * Maximum PS values with regard to scale. Used to export the 'inverse'
>> + * PS value (high values for far objects, low values for near objects).
>> + */
>> +static const int stk3310_ps_max[4] = {
>> +    STK3310_PS_MAX_VAL / 64,
>> +    STK3310_PS_MAX_VAL / 16,
>> +    STK3310_PS_MAX_VAL /  4,
>> +    STK3310_PS_MAX_VAL,
>> +};
>> +
>> +static const int stk3310_scale_table[][2] = {
>> +    {6, 400000}, {1, 600000}, {0, 400000}, {0, 100000}
>> +};
>> +
>> +/* Integration time in seconds, microseconds */
>> +static const int stk3310_it_table[][2] = {
>> +    {0, 185},       {0, 370},       {0, 741},       {0, 1480},
>> +    {0, 2960},      {0, 5920},      {0, 11840},     {0, 23680},
>> +    {0, 47360},     {0, 94720},     {0, 189440},    {0, 378880},
>> +    {0, 757760},    {1, 515520},    {3, 31040},     {6, 62080},
>> +};
>> +
>> +struct stk3310_data {
>> +    struct i2c_client *client;
>> +    struct mutex lock;
>> +    bool als_enabled;
>> +    bool ps_enabled;
>> +    struct regmap *regmap;
>> +    struct regmap_field *reg_state;
>> +    struct regmap_field *reg_als_gain;
>> +    struct regmap_field *reg_ps_gain;
>> +    struct regmap_field *reg_als_it;
>> +    struct regmap_field *reg_ps_it;
>> +};
>> +
>> +static const struct iio_chan_spec stk3310_channels[] = {
>> +    {
>> +            .channel = 0,
>
> no need for channel number, the channels have distinct names due to their
> type
Also, without indexed being set it won't be used in the naming anyway.
>
>> +            .type = IIO_LIGHT,
>> +            .info_mask_separate =
>> +                    BIT(IIO_CHAN_INFO_RAW) |
>> +                    BIT(IIO_CHAN_INFO_SCALE) |
>> +                    BIT(IIO_CHAN_INFO_INT_TIME),
>> +    },
>> +    {
>> +            .channel = 1,
>> +            .type = IIO_PROXIMITY,
>> +            .info_mask_separate =
>> +                    BIT(IIO_CHAN_INFO_RAW) |
>> +                    BIT(IIO_CHAN_INFO_SCALE) |
>> +                    BIT(IIO_CHAN_INFO_INT_TIME),
>> +    }
>> +};
>> +
>> +static ssize_t stk3310_get_it_vals(struct device *dev,
>> +                               struct device_attribute *attr, char *buf)
>> +{
>> +    int i;
>> +    int len;
>> +
>> +    for (i = 0, len = 0; i < ARRAY_SIZE(stk3310_it_table); i++) {
>> +            len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
>> +                            stk3310_it_table[i][0],
>> +                            stk3310_it_table[i][1]);
>> +    }
>> +    buf[len - 1] = '\n';
>> +
>> +    return len;
>> +}
>> +
>> +static IIO_CONST_ATTR(in_illuminance_scale_available, STK3310_SCALE_AVAILABLE);
>> +
>> +static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,
>
> these should be IIO_CONST_ATTR() as well?
>
>> +                   S_IRUGO, stk3310_get_it_vals, NULL, 0);
>> +
>> +static IIO_DEVICE_ATTR(in_proximity_integration_time_available,
>> +                   S_IRUGO, stk3310_get_it_vals, NULL, 0);
>> +
>> +static struct attribute *stk3310_attributes[] = {
>> +    &iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
>> +    &iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
>
> where is proximity_scale_available?
>
>> +    &iio_dev_attr_in_proximity_integration_time_available.dev_attr.attr,
>> +    NULL,
>> +};
>> +
>> +static const struct attribute_group stk3310_attribute_group = {
>> +    .attrs = stk3310_attributes
>> +};
>> +
>> +static int stk3310_get_index(const int table[][2], int table_size,
>
> this function appears over and over in IIO... maybe have a generic one?
Would probably make sense...  Go on Peter, it was your idea ;)
>
>> +                         int val, int val2)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < table_size; i++) {
>> +            if (val == table[i][0] && val2 == table[i][1])
>> +                    return i;
>> +    }
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +static int stk3310_read_raw(struct iio_dev *indio_dev,
>> +                        struct iio_chan_spec const *chan,
>> +                        int *val, int *val2, long mask)
>> +{
>> +    u8 reg;
>> +    u16 buf;
>> +    unsigned int index;
>> +    struct stk3310_data *data = iio_priv(indio_dev);
>> +    struct i2c_client *client = data->client;
>> +
>> +    switch (chan->type) {
>
> this block makes only sense for _INFO_RAW and should be moved there
>
>> +    case IIO_LIGHT:
>> +            reg = STK3310_REG_ALS_DATA_MSB;
>> +            break;
>> +    case IIO_PROXIMITY:
>> +            reg = STK3310_REG_PS_DATA_MSB;
>> +            break;
>> +    default:
>> +            return -EINVAL;
>> +    }
>> +
>> +    switch (mask) {
>> +    case IIO_CHAN_INFO_RAW:
>> +            mutex_lock(&data->lock);
>> +            regmap_bulk_read(data->regmap, reg, &buf, 2);
>
> regmap_bulk_read() returns an integer, let's check that...
>
>> +            if (buf < 0) {
>
> buf is u16, how can the be < 0?
>
>> +                    dev_err(&client->dev, "register read failed\n");
>> +                    mutex_unlock(&data->lock);
>> +                    return buf;
>> +            }
>> +            *val = swab16(buf);
>> +            if (chan->type == IIO_PROXIMITY) {
>> +                    /*
>> +                     * Invert the proximity data so we return low values
>> +                     * for close objects and high values for far ones.
>> +                     */
>
> it would/should be _PROCESSED then due to the inversion?
>
> other drivers report proximity such that close objects have high values
> and far objects have low values -- it is a reflection value really; I'd
> rather fix that in the documentation
>

//tiberiu:
The sx9500 driver behaves the same way.
The ABI says: "the reported values should behave in the same way as a distance,
i.e. lower values indicate something is closer to the sensor."

http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio

>> +                    regmap_field_read(data->reg_ps_gain, &index);
>> +                    *val = stk3310_ps_max[index] - *val;
>> +            }
>> +            mutex_unlock(&data->lock);
>> +            return IIO_VAL_INT;
>> +    case IIO_CHAN_INFO_INT_TIME:
>> +            if (chan->type == IIO_LIGHT)
>> +                    regmap_field_read(data->reg_als_it, &index);
>> +            else
>> +                    regmap_field_read(data->reg_ps_it, &index);
>> +            *val = stk3310_it_table[index][0];
>> +            *val2 = stk3310_it_table[index][1];
>> +            return IIO_VAL_INT_PLUS_MICRO;
>> +    case IIO_CHAN_INFO_SCALE:
>> +            if (chan->type == IIO_LIGHT)
>> +                    regmap_field_read(data->reg_als_gain, &index);
>> +            else
>> +                    regmap_field_read(data->reg_ps_gain, &index);
>> +            *val = stk3310_scale_table[index][0];
>> +            *val2 = stk3310_scale_table[index][1];
>> +            return IIO_VAL_INT_PLUS_MICRO;
>> +    }
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +static int stk3310_write_raw(struct iio_dev *indio_dev,
>> +                         struct iio_chan_spec const *chan,
>> +                         int val, int val2, long mask)
>> +{
>> +    int ret;
>> +    unsigned int index;
>> +    struct stk3310_data *data = iio_priv(indio_dev);
>> +
>> +    if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
>> +            return -EINVAL;
>
> most drivers don't check the channel type and assume that chan points to
> one of the channels registered
>
>> +
>> +    switch (mask) {
>> +    case IIO_CHAN_INFO_INT_TIME:
>> +            index = stk3310_get_index(stk3310_it_table,
>> +                                      ARRAY_SIZE(stk3310_it_table),
>> +                                      val, val2);
>> +            if (index < 0)
>> +                    return -EINVAL;
>> +            mutex_lock(&data->lock);
>> +            if (chan->type == IIO_LIGHT)
>> +                    ret = regmap_field_write(data->reg_als_it, index);
>> +            else
>> +                    ret = regmap_field_write(data->reg_ps_it, index);
>> +            if (ret < 0)
>> +                    dev_err(&data->client->dev,
>> +                                    "sensor configuration failed\n");
>> +            mutex_unlock(&data->lock);
>> +            return ret;
>> +
>> +    case IIO_CHAN_INFO_SCALE:
>> +            index = stk3310_get_index(stk3310_scale_table,
>> +                                      ARRAY_SIZE(stk3310_scale_table),
>> +                                      val, val2);
>> +            if (index < 0)
>> +                    return -EINVAL;
>> +            mutex_lock(&data->lock);
>> +            if (chan->type == IIO_LIGHT)
>> +                    ret = regmap_field_write(data->reg_als_gain, index);
>> +            else
>> +                    ret = regmap_field_write(data->reg_ps_gain, index);
>> +            if (ret < 0)
>> +                    dev_err(&data->client->dev,
>> +                                    "sensor configuration failed\n");
>> +            mutex_unlock(&data->lock);
>> +            return ret;
>> +    }
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +static const struct iio_info stk3310_info = {
>> +    .driver_module          = THIS_MODULE,
>> +    .read_raw               = stk3310_read_raw,
>> +    .write_raw              = stk3310_write_raw,
>> +    .attrs                  = &stk3310_attribute_group,
>> +};
>> +
>> +static int stk3310_set_state(struct stk3310_data *data, u8 state)
>> +{
>> +    int ret;
>> +    struct i2c_client *client = data->client;
>> +
>> +    /* 3-bit state; 0b100 is not supported. */
>> +    if (state < 0 || state > 7 || state == 4)
>
> state is u8, can't be negative
>
>> +            return -EINVAL;
>> +
>> +    mutex_lock(&data->lock);
>> +    ret = regmap_field_write(data->reg_state, state);
>> +    if (ret < 0) {
>> +            dev_err(&client->dev, "failed to change sensor state\n");
>> +    /* Don't reset the 'enabled' flags if we're going in standby */
>
> move the comment to the block where it belongs to
>
>> +    } else if (state != STK3310_STATE_STANDBY) {
>> +            data->ps_enabled  = !!(state & 0x01);
>> +            data->als_enabled = !!(state & 0x02);
>> +    }
>> +    mutex_unlock(&data->lock);
>> +
>> +    return ret;
>> +}
>> +
>> +static int stk3310_init(struct iio_dev *indio_dev)
>> +{
>> +    int ret;
>> +    int chipid;
>> +    u8 state;
>> +    struct stk3310_data *data = iio_priv(indio_dev);
>> +    struct i2c_client *client = data->client;
>> +
>> +    regmap_read(data->regmap, STK3310_REG_ID, &chipid);
>> +    if (chipid != STK3310_CHIP_ID_VAL &&
>> +        chipid != STK3311_CHIP_ID_VAL) {
>> +            dev_err(&client->dev, "invalid chip id: 0x%x\n", chipid);
>> +            return -ENODEV;
>> +    }
>> +
>> +    state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
>> +    ret = stk3310_set_state(data, state);
>> +    if (ret < 0)
>> +            dev_err(&client->dev, "failed to enable sensor");
>> +
>> +    return ret;
>> +}
>> +
>> +static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> +    switch (reg) {
>> +    case STK3310_REG_ALS_DATA_MSB:
>> +    case STK3310_REG_ALS_DATA_LSB:
>> +    case STK3310_REG_PS_DATA_LSB:
>> +    case STK3310_REG_PS_DATA_MSB:
>> +            return true;
>> +    default:
>> +            return false;
>> +    }
>> +}
>> +
>> +static struct regmap_config stk3310_regmap_config = {
>> +    .name = STK3310_REGMAP_NAME,
>> +    .reg_bits = 8,
>> +    .val_bits = 8,
>> +    .max_register = STK3310_MAX_REG,
>> +    .cache_type = REGCACHE_RBTREE,
>> +    .volatile_reg = stk3310_is_volatile_reg,
>> +};
>> +
>> +static int stk3310_regmap_init(struct stk3310_data *data)
>> +{
>> +    struct regmap *regmap;
>> +    struct i2c_client *client;
>> +
>> +    client = data->client;
>> +    regmap = devm_regmap_init_i2c(client, &stk3310_regmap_config);
>> +    if (IS_ERR(regmap)) {
>> +            dev_err(&client->dev, "regmap initialization failed.\n");
>> +            return PTR_ERR(regmap);
>> +    }
>> +    data->regmap = regmap;
>> +
>
> code below is clumsy; many lines for little effect, maybe a helper
> function/macro?
>
>> +    data->reg_state = devm_regmap_field_alloc(&client->dev, regmap,
>> +                    reg_field_state);
>> +    if (IS_ERR(data->reg_state)) {
>> +            dev_err(&client->dev, "reg field alloc failed.\n");
>> +            return PTR_ERR(data->reg_state);
>> +    }
>> +
>> +    data->reg_als_gain = devm_regmap_field_alloc(&client->dev, regmap,
>> +                    reg_field_als_gain);
>> +    if (IS_ERR(data->reg_als_gain)) {
>> +            dev_err(&client->dev, "reg field alloc failed.\n");
>> +            return PTR_ERR(data->reg_als_gain);
>> +    }
>> +
>> +    data->reg_ps_gain = devm_regmap_field_alloc(&client->dev, regmap,
>> +                    reg_field_ps_gain);
>> +    if (IS_ERR(data->reg_ps_gain)) {
>> +            dev_err(&client->dev, "reg field alloc failed.\n");
>> +            return PTR_ERR(data->reg_ps_gain);
>> +    }
>> +
>> +    data->reg_als_it = devm_regmap_field_alloc(&client->dev, regmap,
>> +                    reg_field_als_it);
>> +    if (IS_ERR(data->reg_als_it)) {
>> +            dev_err(&client->dev, "reg field alloc failed.\n");
>> +            return PTR_ERR(data->reg_als_it);
>> +    }
>> +
>> +    data->reg_ps_it = devm_regmap_field_alloc(&client->dev, regmap,
>> +                    reg_field_ps_it);
>> +    if (IS_ERR(data->reg_ps_it)) {
>> +            dev_err(&client->dev, "reg field alloc failed.\n");
>> +            return PTR_ERR(data->reg_ps_it);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int stk3310_probe(struct i2c_client *client,
>> +                     const struct i2c_device_id *id)
>> +{
>> +    int ret;
>> +    struct iio_dev *indio_dev;
>> +    struct stk3310_data *data;
>> +
>> +    indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> +    if (!indio_dev) {
>> +            dev_err(&client->dev, "iio allocation failed!\n");
>> +            return -ENOMEM;
>> +    }
>> +
>> +    data = iio_priv(indio_dev);
>> +    data->client = client;
>> +    i2c_set_clientdata(client, indio_dev);
>> +    mutex_init(&data->lock);
>> +
>> +    ret = stk3310_regmap_init(data);
>> +    if (ret < 0)
>> +            return ret;
>> +
>> +    indio_dev->dev.parent = &client->dev;
>> +    indio_dev->info = &stk3310_info;
>> +    indio_dev->name = STK3310_DRIVER_NAME;
>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>> +    indio_dev->channels = stk3310_channels;
>> +    indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
>> +
>> +    ret = stk3310_init(indio_dev);
>> +    if (ret < 0) {
>> +            dev_err(&client->dev, "sensor initialization failed\n");
>
> we already printed a error message in stk3310_init()...
>
>> +            return ret;
>> +    }
>> +
>> +    ret = iio_device_register(indio_dev);
>> +    if (ret < 0) {
>> +            dev_err(&client->dev, "device_register failed\n");
>> +            stk3310_set_state(data, STK3310_STATE_STANDBY);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int stk3310_remove(struct i2c_client *client)
>> +{
>> +    struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +
>> +    iio_device_unregister(indio_dev);
>> +    return stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int stk3310_suspend(struct device *dev)
>> +{
>> +    struct stk3310_data *data;
>> +
>> +    data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
>> +
>> +    return stk3310_set_state(data, STK3310_STATE_STANDBY);
>> +}
>> +
>> +static int stk3310_resume(struct device *dev)
>> +{
>> +    int state = 0;
>> +    struct stk3310_data *data;
>> +
>> +    data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
>> +    if (data->ps_enabled)
>> +            state |= STK3310_STATE_EN_PS;
>> +    if (data->als_enabled)
>> +            state |= STK3310_STATE_EN_ALS;
>> +
>> +    return stk3310_set_state(data, state);
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(stk3310_pm_ops, stk3310_suspend, stk3310_resume);
>> +
>> +#define STK3310_PM_OPS (&stk3310_pm_ops)
>> +#else
>> +#define STK3310_PM_OPS NULL
>> +#endif
>> +
>> +static const struct i2c_device_id stk3310_i2c_id[] = {
>> +    {"STK3310", 0},
>> +    {"STK3311", 0},
>
> if there is no software-visible difference, I'd rather not distinguish the
> IDs; otherwise, you could/should check that the ID here matches the ID
> returned from the device
Except that device tree writers are going to expect the id to match their
part number. So I'd keep them both even if they can't be distinguished.
(of course if they can, do check it!)

//tiberiu: We're not sure yet how vendors will expose this device through
ACPI or DT. If they choose to expose "STK3311", we want this id to be
available.

>
>> +    {}
>> +};
>> +
>> +static const struct acpi_device_id stk3310_acpi_id[] = {
>> +    {"STK3310", 0},
>> +    {"STK3311", 0},
>> +    {}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(acpi, stk3310_acpi_id);
>> +
>> +static struct i2c_driver stk3310_driver = {
>> +    .driver = {
>> +            .name = "stk3310",
>> +            .pm = STK3310_PM_OPS,
>> +            .acpi_match_table = ACPI_PTR(stk3310_acpi_id),
>> +    },
>> +    .probe =            stk3310_probe,
>> +    .remove =           stk3310_remove,
>> +    .id_table =         stk3310_i2c_id,
>> +};
>> +
>> +module_i2c_driver(stk3310_driver);
>> +
>> +MODULE_AUTHOR("Tiberiu Breana <tiberiu.a.breana@intel.com>");
>> +MODULE_DESCRIPTION("STK3310 Ambient Light and Proximity Sensor driver");
>> +MODULE_LICENSE("GPL v2");
>>
>

[PATCH v2 2/2] iio: light: Add threshold interrupt support for STK3310

[Tiberiu Breana]

Added interrupt support for proximity threshold events
to the stk3310 driver.

Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/light/stk3310.c | 276 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 274 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 825a65a..1702fe3 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -12,15 +12,22 @@
 
 #include <linux/acpi.h>
 #include <linux/i2c.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
 #define STK3310_REG_STATE			0x00
 #define STK3310_REG_PSCTRL			0x01
 #define STK3310_REG_ALSCTRL			0x02
+#define STK3310_REG_INT				0x04
+#define STK3310_REG_THDH_PS			0x06
+#define STK3310_REG_THDL_PS			0x08
+#define STK3310_REG_FLAG			0x10
 #define STK3310_REG_PS_DATA_MSB			0x11
 #define STK3310_REG_PS_DATA_LSB			0x12
 #define STK3310_REG_ALS_DATA_MSB		0x13
@@ -34,12 +41,16 @@
 
 #define STK3310_CHIP_ID_VAL			0x13
 #define STK3311_CHIP_ID_VAL			0x1D
+#define STK3310_PSINT_EN			0x01
 #define STK3310_PS_MAX_VAL			0xffff
+#define STK3310_THRESH_MAX			0xffff
 
 #define STK3310_SCALE_AVAILABLE			"6.4 1.6 0.4 0.1"
 
 #define STK3310_DRIVER_NAME			"stk3310"
 #define STK3310_REGMAP_NAME			"stk3310_regmap"
+#define STK3310_EVENT				"stk3310_event"
+#define STK3310_GPIO				"stk3310_gpio"
 
 static const struct reg_field reg_field_state =
 				REG_FIELD(STK3310_REG_STATE, 0, 2);
@@ -51,7 +62,12 @@ static const struct reg_field reg_field_als_it =
 				REG_FIELD(STK3310_REG_ALSCTRL, 0, 3);
 static const struct reg_field reg_field_ps_it =
 				REG_FIELD(STK3310_REG_PSCTRL, 0, 3);
-
+static const struct reg_field reg_field_int_ps =
+				REG_FIELD(STK3310_REG_INT, 0, 2);
+static const struct reg_field reg_field_flag_psint =
+				REG_FIELD(STK3310_REG_FLAG, 4, 4);
+static const struct reg_field reg_field_flag_nf =
+				REG_FIELD(STK3310_REG_FLAG, 0, 0);
 /*
  * Maximum PS values with regard to scale. Used to export the 'inverse'
  * PS value (high values for far objects, low values for near objects).
@@ -80,12 +96,33 @@ struct stk3310_data {
 	struct mutex lock;
 	bool als_enabled;
 	bool ps_enabled;
+	u64 timestamp;
 	struct regmap *regmap;
 	struct regmap_field *reg_state;
 	struct regmap_field *reg_als_gain;
 	struct regmap_field *reg_ps_gain;
 	struct regmap_field *reg_als_it;
 	struct regmap_field *reg_ps_it;
+	struct regmap_field *reg_int_ps;
+	struct regmap_field *reg_flag_psint;
+	struct regmap_field *reg_flag_nf;
+};
+
+static const struct iio_event_spec stk3310_events[] = {
+	/* Proximity event */
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+	/* Out-of-proximity event */
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
 };
 
 static const struct iio_chan_spec stk3310_channels[] = {
@@ -104,6 +141,9 @@ static const struct iio_chan_spec stk3310_channels[] = {
 			BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE) |
 			BIT(IIO_CHAN_INFO_INT_TIME),
+		.event_spec = stk3310_events,
+		.num_event_specs = ARRAY_SIZE(stk3310_events),
+
 	}
 };
 
@@ -155,6 +195,120 @@ static int stk3310_get_index(const int table[][2], int table_size,
 	return -EINVAL;
 }
 
+static int stk3310_read_event(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan,
+			      enum iio_event_type type,
+			      enum iio_event_direction dir,
+			      enum iio_event_info info,
+			      int *val, int *val2)
+{
+	u8 reg;
+	u16 buf;
+	int index;
+	struct stk3310_data *data = iio_priv(indio_dev);
+
+	if (info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	/*
+	 * Only proximity interrupts are implemented at the moment.
+	 * Since we're inverting proximity values, the sensor's 'high'
+	 * threshold will become our 'low' threshold, associated with
+	 * 'near' events. Similarly, the sensor's 'low' threshold will
+	 * be our 'high' threshold, associated with 'far' events.
+	 */
+	if (dir == IIO_EV_DIR_RISING)
+		reg = STK3310_REG_THDL_PS;
+	else if (dir == IIO_EV_DIR_FALLING)
+		reg = STK3310_REG_THDH_PS;
+	else
+		return -EINVAL;
+
+	mutex_lock(&data->lock);
+	regmap_bulk_read(data->regmap, reg, &buf, 2);
+	mutex_unlock(&data->lock);
+	if (buf < 0) {
+		dev_err(&data->client->dev, "register read failed\n");
+		return buf;
+	}
+	regmap_field_read(data->reg_ps_gain, &index);
+	*val = swab16(stk3310_ps_max[index] - buf);
+
+	return IIO_VAL_INT;
+}
+
+static int stk3310_write_event(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info,
+			       int val, int val2)
+{
+	int ret;
+	unsigned int index;
+	unsigned int reg;
+	u16 buf;
+	struct stk3310_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+
+	regmap_field_read(data->reg_ps_gain, &index);
+	if (val > stk3310_ps_max[index])
+		return -EINVAL;
+
+	if (dir == IIO_EV_DIR_RISING)
+		reg = STK3310_REG_THDL_PS;
+	else if (dir == IIO_EV_DIR_FALLING)
+		reg = STK3310_REG_THDH_PS;
+	else
+		return -EINVAL;
+
+	buf = swab16(stk3310_ps_max[index] - val);
+	ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
+	if (ret < 0)
+		dev_err(&client->dev, "failed to set PS threshold!\n");
+
+	return ret;
+}
+
+static int stk3310_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 stk3310_data *data = iio_priv(indio_dev);
+	unsigned int event_val;
+
+	if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+	regmap_field_read(data->reg_int_ps, &event_val);
+
+	return event_val;
+}
+
+static int stk3310_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)
+{
+	int ret;
+	struct stk3310_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+
+	if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH ||
+	    state < 0 || state > 7)
+		return -EINVAL;
+
+	/* Set INT_PS value */
+	mutex_lock(&data->lock);
+	ret = regmap_field_write(data->reg_int_ps, state);
+	if (ret < 0)
+		dev_err(&client->dev, "failed to set interrupt mode\n");
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
 static int stk3310_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -272,6 +426,10 @@ static const struct iio_info stk3310_info = {
 	.read_raw		= stk3310_read_raw,
 	.write_raw		= stk3310_write_raw,
 	.attrs			= &stk3310_attribute_group,
+	.read_event_value	= stk3310_read_event,
+	.write_event_value	= stk3310_write_event,
+	.read_event_config	= stk3310_read_event_config,
+	.write_event_config	= stk3310_write_event_config,
 };
 
 static int stk3310_set_state(struct stk3310_data *data, u8 state)
@@ -314,8 +472,43 @@ static int stk3310_init(struct iio_dev *indio_dev)
 
 	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
 	ret = stk3310_set_state(data, state);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&client->dev, "failed to enable sensor");
+		return ret;
+	}
+
+	/* Enable PS interrupts */
+	ret = regmap_field_write(data->reg_int_ps, STK3310_PSINT_EN);
+	if (ret < 0)
+		dev_err(&client->dev, "failed to enable interrupts!\n");
+
+	return ret;
+}
+
+static int stk3310_gpio_probe(struct i2c_client *client)
+{
+	struct device *dev;
+	struct gpio_desc *gpio;
+	int ret;
+
+	if (!client)
+		return -EINVAL;
+
+	dev = &client->dev;
+
+	/* gpio interrupt pin */
+	gpio = devm_gpiod_get_index(dev, STK3310_GPIO, 0);
+	if (IS_ERR(gpio)) {
+		dev_err(dev, "acpi gpio get index failed\n");
+		return PTR_ERR(gpio);
+	}
+
+	ret = gpiod_direction_input(gpio);
+	if (ret)
+		return ret;
+
+	ret = gpiod_to_irq(gpio);
+	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
 
 	return ret;
 }
@@ -327,6 +520,7 @@ static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg)
 	case STK3310_REG_ALS_DATA_LSB:
 	case STK3310_REG_PS_DATA_LSB:
 	case STK3310_REG_PS_DATA_MSB:
+	case STK3310_REG_FLAG:
 		return true;
 	default:
 		return false;
@@ -390,9 +584,72 @@ static int stk3310_regmap_init(struct stk3310_data *data)
 		return PTR_ERR(data->reg_ps_it);
 	}
 
+	data->reg_int_ps = devm_regmap_field_alloc(&client->dev, regmap,
+			reg_field_int_ps);
+	if (IS_ERR(data->reg_int_ps)) {
+		dev_err(&client->dev, "reg field alloc failed.\n");
+		return PTR_ERR(data->reg_int_ps);
+	}
+
+	data->reg_flag_psint = devm_regmap_field_alloc(&client->dev, regmap,
+			reg_field_flag_psint);
+	if (IS_ERR(data->reg_flag_psint)) {
+		dev_err(&client->dev, "reg field alloc failed.\n");
+		return PTR_ERR(data->reg_flag_psint);
+	}
+
+	data->reg_flag_nf = devm_regmap_field_alloc(&client->dev, regmap,
+			reg_field_flag_nf);
+	if (IS_ERR(data->reg_flag_nf)) {
+		dev_err(&client->dev, "reg field alloc failed.\n");
+		return PTR_ERR(data->reg_flag_nf);
+	}
+
 	return 0;
 }
 
+static irqreturn_t stk3310_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct stk3310_data *data = iio_priv(indio_dev);
+
+	data->timestamp = iio_get_time_ns();
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t stk3310_irq_event_handler(int irq, void *private)
+{
+	int ret;
+	unsigned int dir;
+	u64 event;
+
+	struct iio_dev *indio_dev = private;
+	struct stk3310_data *data = iio_priv(indio_dev);
+
+	/* Read FLAG_NF to figure out what threshold has been met. */
+	mutex_lock(&data->lock);
+	ret = regmap_field_read(data->reg_flag_nf, &dir);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "register read failed\n");
+		mutex_unlock(&data->lock);
+		return ret;
+	}
+	event = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 1,
+				     IIO_EV_TYPE_THRESH,
+				     (dir ? IIO_EV_DIR_RISING :
+					    IIO_EV_DIR_FALLING));
+	iio_push_event(indio_dev, event, data->timestamp);
+
+	/* Reset the interrupt flag */
+	ret = regmap_field_write(data->reg_flag_psint, 0);
+	if (ret < 0)
+		dev_err(&data->client->dev, "failed to reset interrupts\n");
+	mutex_unlock(&data->lock);
+
+	return IRQ_HANDLED;
+}
+
 static int stk3310_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -434,6 +691,21 @@ static int stk3310_probe(struct i2c_client *client,
 		stk3310_set_state(data, STK3310_STATE_STANDBY);
 	}
 
+	if (client->irq <= 0)
+		client->irq = stk3310_gpio_probe(client);
+
+	if (client->irq >= 0) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						stk3310_irq_handler,
+						stk3310_irq_event_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						STK3310_EVENT, indio_dev);
+		if (ret < 0)
+			dev_err(&client->dev, "request irq %d failed\n",
+					client->irq);
+	}
+
 	return ret;
 }
 
-- 
1.9.1