[PATCH 0/2] iio: light: tcs3472: bug fix and iio event handling support

[PATCH 0/2] iio: light: tcs3472: bug fix and iio event handling support

[Akinobu Mita]

This contains a single bug fix and support iio event handling for
tcs3472 driver.

Akinobu Mita (2):
  iio: light: tcs3472: fix ATIME register write
  iio: light: tcs3472: support out-of-threshold events

 drivers/iio/light/tcs3472.c | 271 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 262 insertions(+), 9 deletions(-)

Cc: Peter Meerwald <pmeerw@pmeerw.net>
Cc: Jonathan Cameron <jic23@kernel.org>
-- 
2.7.4

[PATCH 1/2] iio: light: tcs3472: fix ATIME register write

[Akinobu Mita]

The integration time is controlled by the ATIME register only.  However,
this register is written by i2c_smbus_write_word_data() in write_raw().

We actually don't need to write a subsequent register.  So just use
i2c_smbus_write_byte_data() instead.

Cc: Peter Meerwald <pmeerw@pmeerw.net>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/light/tcs3472.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 3aa71e3..a9e153b 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -169,7 +169,7 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
 		for (i = 0; i < 256; i++) {
 			if (val2 == (256 - i) * 2400) {
 				data->atime = i;
-				return i2c_smbus_write_word_data(
+				return i2c_smbus_write_byte_data(
 					data->client, TCS3472_ATIME,
 					data->atime);
 			}
-- 
2.7.4

[PATCH 2/2] iio: light: tcs3472: support out-of-threshold events

[Akinobu Mita]

The TCS3472 device provides interrupt signal for out-of-threshold events
with persistence filter.

This change adds interrupt support for the threshold events and enables
to configure the period of time by persistence filter.

Cc: Peter Meerwald <pmeerw@pmeerw.net>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/light/tcs3472.c | 269 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 261 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index a9e153b..905e777 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -11,7 +11,9 @@
  * 7-bit I2C slave address 0x39 (TCS34721, TCS34723) or 0x29 (TCS34725,
  * TCS34727)
  *
- * TODO: interrupt support, thresholds, wait time
+ * Datasheet: http://ams.com/eng/content/download/319364/1117183/file/TCS3472_Datasheet_EN_v2.pdf
+ *
+ * TODO: wait time
  */
 
 #include <linux/module.h>
@@ -21,6 +23,7 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/triggered_buffer.h>
@@ -29,12 +32,15 @@
 
 #define TCS3472_COMMAND BIT(7)
 #define TCS3472_AUTO_INCR BIT(5)
+#define TCS3472_SPECIAL_FUNC (BIT(5) | BIT(6))
+
+#define TCS3472_INTR_CLEAR (TCS3472_COMMAND | TCS3472_SPECIAL_FUNC | 0x6)
 
 #define TCS3472_ENABLE (TCS3472_COMMAND | 0x00)
 #define TCS3472_ATIME (TCS3472_COMMAND | 0x01)
 #define TCS3472_WTIME (TCS3472_COMMAND | 0x03)
-#define TCS3472_AILT (TCS3472_COMMAND | 0x04)
-#define TCS3472_AIHT (TCS3472_COMMAND | 0x06)
+#define TCS3472_AILT (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x04)
+#define TCS3472_AIHT (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x06)
 #define TCS3472_PERS (TCS3472_COMMAND | 0x0c)
 #define TCS3472_CONFIG (TCS3472_COMMAND | 0x0d)
 #define TCS3472_CONTROL (TCS3472_COMMAND | 0x0f)
@@ -45,19 +51,42 @@
 #define TCS3472_GDATA (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x18)
 #define TCS3472_BDATA (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x1a)
 
+#define TCS3472_STATUS_AINT BIT(4)
 #define TCS3472_STATUS_AVALID BIT(0)
+#define TCS3472_ENABLE_AIEN BIT(4)
 #define TCS3472_ENABLE_AEN BIT(1)
 #define TCS3472_ENABLE_PON BIT(0)
 #define TCS3472_CONTROL_AGAIN_MASK (BIT(0) | BIT(1))
 
 struct tcs3472_data {
 	struct i2c_client *client;
+	struct mutex lock;
+	u16 low_thresh;
+	u16 high_thresh;
 	u8 enable;
 	u8 control;
 	u8 atime;
+	u8 apers;
 	u16 buffer[8]; /* 4 16-bit channels + 64-bit timestamp */
 };
 
+static const struct iio_event_spec tcs3472_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				 BIT(IIO_EV_INFO_PERIOD),
+	},
+};
+
 #define TCS3472_CHANNEL(_color, _si, _addr) { \
 	.type = IIO_INTENSITY, \
 	.modified = 1, \
@@ -73,6 +102,8 @@ struct tcs3472_data {
 		.storagebits = 16, \
 		.endianness = IIO_CPU, \
 	}, \
+	.event_spec = _si ? NULL : tcs3472_events, \
+	.num_event_specs = _si ? 0 : ARRAY_SIZE(tcs3472_events), \
 }
 
 static const int tcs3472_agains[] = { 1, 4, 16, 60 };
@@ -180,6 +211,166 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+/*
+ * Translation from APERS field value to the number of consecutive out-of-range
+ * Clear occurrences before an interrupt is generated
+ */
+static const int tcs3472_intr_pers[] = {
+	0, 1, 2, 3, 5, 10, 15, 20, 25, 30, 35, 40, 45, 50, 55, 60
+};
+
+static int tcs3472_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)
+{
+	struct tcs3472_data *data = iio_priv(indio_dev);
+	int ret;
+	unsigned int period;
+
+	mutex_lock(&data->lock);
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		*val = (dir == IIO_EV_DIR_RISING) ?
+			data->high_thresh : data->low_thresh;
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_EV_INFO_PERIOD:
+		period = (256 - data->atime) * 2400 *
+			tcs3472_intr_pers[data->apers];
+		*val = period / USEC_PER_SEC;
+		*val2 = period % USEC_PER_SEC;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int tcs3472_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)
+{
+	struct tcs3472_data *data = iio_priv(indio_dev);
+	int ret;
+	u8 command;
+	unsigned int period;
+	int i;
+
+	mutex_lock(&data->lock);
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			command = TCS3472_AIHT;
+			break;
+		case IIO_EV_DIR_FALLING:
+			command = TCS3472_AILT;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		ret = i2c_smbus_write_word_data(data->client, command, val);
+		if (ret)
+			goto error;
+
+		if (dir == IIO_EV_DIR_RISING)
+			data->high_thresh = val;
+		else
+			data->low_thresh = val;
+		break;
+	case IIO_EV_INFO_PERIOD:
+		period = val * USEC_PER_SEC + val2;
+		for (i = 1; i < ARRAY_SIZE(tcs3472_intr_pers) - 1; i++) {
+			if (period <= (256 - data->atime) * 2400 *
+					tcs3472_intr_pers[i])
+				break;
+		}
+		ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS, i);
+		if (ret)
+			goto error;
+
+		data->apers = i;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+error:
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int tcs3472_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 tcs3472_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = !!(data->enable & TCS3472_ENABLE_AIEN);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int tcs3472_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 tcs3472_data *data = iio_priv(indio_dev);
+	int ret = 0;
+	u8 enable_old;
+
+	mutex_lock(&data->lock);
+
+	enable_old = data->enable;
+
+	if (state)
+		data->enable |= TCS3472_ENABLE_AIEN;
+	else
+		data->enable &= ~TCS3472_ENABLE_AIEN;
+
+	if (enable_old != data->enable) {
+		ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
+						data->enable);
+		if (ret)
+			data->enable = enable_old;
+	}
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static irqreturn_t tcs3472_event_handler(int irq, void *priv)
+{
+	struct iio_dev *indio_dev = priv;
+	struct tcs3472_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, TCS3472_STATUS);
+	if (ret >= 0 && (ret & TCS3472_STATUS_AINT)) {
+		iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, 0,
+						IIO_EV_TYPE_THRESH,
+						IIO_EV_DIR_EITHER),
+				iio_get_time_ns(indio_dev));
+
+		i2c_smbus_read_byte_data(data->client, TCS3472_INTR_CLEAR);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t tcs3472_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -243,6 +434,10 @@ static const struct attribute_group tcs3472_attribute_group = {
 static const struct iio_info tcs3472_info = {
 	.read_raw = tcs3472_read_raw,
 	.write_raw = tcs3472_write_raw,
+	.read_event_value = tcs3472_read_event,
+	.write_event_value = tcs3472_write_event,
+	.read_event_config = tcs3472_read_event_config,
+	.write_event_config = tcs3472_write_event_config,
 	.attrs = &tcs3472_attribute_group,
 	.driver_module = THIS_MODULE,
 };
@@ -261,6 +456,7 @@ static int tcs3472_probe(struct i2c_client *client,
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
+	mutex_init(&data->lock);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &tcs3472_info;
@@ -290,12 +486,29 @@ static int tcs3472_probe(struct i2c_client *client,
 		return ret;
 	data->atime = ret;
 
+	ret = i2c_smbus_read_word_data(data->client, TCS3472_AILT);
+	if (ret < 0)
+		return ret;
+	data->low_thresh = ret;
+
+	ret = i2c_smbus_read_word_data(data->client, TCS3472_AIHT);
+	if (ret < 0)
+		return ret;
+	data->high_thresh = ret;
+
+	data->apers = 1;
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS,
+					data->apers);
+	if (ret < 0)
+		return ret;
+
 	ret = i2c_smbus_read_byte_data(data->client, TCS3472_ENABLE);
 	if (ret < 0)
 		return ret;
 
 	/* enable device */
 	data->enable = ret | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
+	data->enable &= ~TCS3472_ENABLE_AIEN;
 	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
 		data->enable);
 	if (ret < 0)
@@ -306,12 +519,29 @@ static int tcs3472_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	if (client->irq) {
+		ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS,
+						data->apers);
+		if (ret)
+			goto buffer_cleanup;
+
+		ret = request_threaded_irq(client->irq, NULL,
+					   tcs3472_event_handler,
+					   IRQF_TRIGGER_FALLING | IRQF_SHARED |
+					   IRQF_ONESHOT,
+					   client->name, indio_dev);
+		if (ret)
+			goto buffer_cleanup;
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
-		goto buffer_cleanup;
+		goto free_irq;
 
 	return 0;
 
+free_irq:
+	free_irq(client->irq, indio_dev);
 buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
 	return ret;
@@ -319,14 +549,26 @@ static int tcs3472_probe(struct i2c_client *client,
 
 static int tcs3472_powerdown(struct tcs3472_data *data)
 {
-	return i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
-		data->enable & ~(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON));
+	int ret;
+	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
+
+	mutex_lock(&data->lock);
+
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
+		data->enable & ~enable_mask);
+	if (!ret)
+		data->enable &= ~enable_mask;
+
+	mutex_unlock(&data->lock);
+
+	return ret;
 }
 
 static int tcs3472_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 
+	free_irq(client->irq, indio_dev);
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
 	tcs3472_powerdown(iio_priv(indio_dev));
@@ -346,8 +588,19 @@ static int tcs3472_resume(struct device *dev)
 {
 	struct tcs3472_data *data = iio_priv(i2c_get_clientdata(
 		to_i2c_client(dev)));
-	return i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
-		data->enable | (TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON));
+	int ret;
+	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
+
+	mutex_lock(&data->lock);
+
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
+		data->enable | enable_mask);
+	if (!ret)
+		data->enable |= enable_mask;
+
+	mutex_unlock(&data->lock);
+
+	return ret;
 }
 #endif
 
-- 
2.7.4

Re: [PATCH 1/2] iio: light: tcs3472: fix ATIME register write

[Jonathan Cameron]

On Tue, 13 Jun 2017 00:05:08 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> The integration time is controlled by the ATIME register only.  However,
> this register is written by i2c_smbus_write_word_data() in write_raw().
> 
> We actually don't need to write a subsequent register.  So just use
> i2c_smbus_write_byte_data() instead.
> 
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
As I read the datasheet, this looks like it won't cause any actual
harm (where the top byte is written is unused).

Hence I'm going to apply this as part of the normal merge path
rather than as a fix as such.

Well worth cleaning up though!

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/light/tcs3472.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 3aa71e3..a9e153b 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c
> @@ -169,7 +169,7 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
>  		for (i = 0; i < 256; i++) {
>  			if (val2 == (256 - i) * 2400) {
>  				data->atime = i;
> -				return i2c_smbus_write_word_data(
> +				return i2c_smbus_write_byte_data(
>  					data->client, TCS3472_ATIME,
>  					data->atime);
>  			}

Re: [PATCH 2/2] iio: light: tcs3472: support out-of-threshold events

[Jonathan Cameron]

On Tue, 13 Jun 2017 00:05:09 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> The TCS3472 device provides interrupt signal for out-of-threshold events
> with persistence filter.
> 
> This change adds interrupt support for the threshold events and enables
> to configure the period of time by persistence filter.
> 
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
One comment inline.  Looks fine to me, but I'd like to give
Peter more time to have a look at it.

If we don't hear from Peter within two weeks (ish) give me a bump
and I'll either chase up or take the view Peter is too busy.

Jonathan
> ---
>  drivers/iio/light/tcs3472.c | 269 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 261 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index a9e153b..905e777 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c
> @@ -11,7 +11,9 @@
>   * 7-bit I2C slave address 0x39 (TCS34721, TCS34723) or 0x29 (TCS34725,
>   * TCS34727)
>   *
> - * TODO: interrupt support, thresholds, wait time
> + * Datasheet: http://ams.com/eng/content/download/319364/1117183/file/TCS3472_Datasheet_EN_v2.pdf
I'd have have marginally preferred this change as a separate patch, but if everything else
is fine I don't care enough to delay this.
> + *
> + * TODO: wait time
>   */
>  
>  #include <linux/module.h>
> @@ -21,6 +23,7 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
> @@ -29,12 +32,15 @@
>  
>  #define TCS3472_COMMAND BIT(7)
>  #define TCS3472_AUTO_INCR BIT(5)
> +#define TCS3472_SPECIAL_FUNC (BIT(5) | BIT(6))
> +
> +#define TCS3472_INTR_CLEAR (TCS3472_COMMAND | TCS3472_SPECIAL_FUNC | 0x6)
>  
>  #define TCS3472_ENABLE (TCS3472_COMMAND | 0x00)
>  #define TCS3472_ATIME (TCS3472_COMMAND | 0x01)
>  #define TCS3472_WTIME (TCS3472_COMMAND | 0x03)
> -#define TCS3472_AILT (TCS3472_COMMAND | 0x04)
> -#define TCS3472_AIHT (TCS3472_COMMAND | 0x06)
> +#define TCS3472_AILT (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x04)
> +#define TCS3472_AIHT (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x06)
>  #define TCS3472_PERS (TCS3472_COMMAND | 0x0c)
>  #define TCS3472_CONFIG (TCS3472_COMMAND | 0x0d)
>  #define TCS3472_CONTROL (TCS3472_COMMAND | 0x0f)
> @@ -45,19 +51,42 @@
>  #define TCS3472_GDATA (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x18)
>  #define TCS3472_BDATA (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x1a)
>  
> +#define TCS3472_STATUS_AINT BIT(4)
>  #define TCS3472_STATUS_AVALID BIT(0)
> +#define TCS3472_ENABLE_AIEN BIT(4)
>  #define TCS3472_ENABLE_AEN BIT(1)
>  #define TCS3472_ENABLE_PON BIT(0)
>  #define TCS3472_CONTROL_AGAIN_MASK (BIT(0) | BIT(1))
>  
>  struct tcs3472_data {
>  	struct i2c_client *client;
> +	struct mutex lock;
> +	u16 low_thresh;
> +	u16 high_thresh;
>  	u8 enable;
>  	u8 control;
>  	u8 atime;
> +	u8 apers;
>  	u16 buffer[8]; /* 4 16-bit channels + 64-bit timestamp */
>  };
>  
> +static const struct iio_event_spec tcs3472_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	}, {
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	}, {
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> +				 BIT(IIO_EV_INFO_PERIOD),
> +	},
> +};
> +
>  #define TCS3472_CHANNEL(_color, _si, _addr) { \
>  	.type = IIO_INTENSITY, \
>  	.modified = 1, \
> @@ -73,6 +102,8 @@ struct tcs3472_data {
>  		.storagebits = 16, \
>  		.endianness = IIO_CPU, \
>  	}, \
> +	.event_spec = _si ? NULL : tcs3472_events, \
> +	.num_event_specs = _si ? 0 : ARRAY_SIZE(tcs3472_events), \
>  }
>  
>  static const int tcs3472_agains[] = { 1, 4, 16, 60 };
> @@ -180,6 +211,166 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +/*
> + * Translation from APERS field value to the number of consecutive out-of-range
> + * Clear occurrences before an interrupt is generated
> + */
> +static const int tcs3472_intr_pers[] = {
> +	0, 1, 2, 3, 5, 10, 15, 20, 25, 30, 35, 40, 45, 50, 55, 60
> +};
> +
> +static int tcs3472_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)
> +{
> +	struct tcs3472_data *data = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int period;
> +
> +	mutex_lock(&data->lock);
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		*val = (dir == IIO_EV_DIR_RISING) ?
> +			data->high_thresh : data->low_thresh;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_EV_INFO_PERIOD:
> +		period = (256 - data->atime) * 2400 *
> +			tcs3472_intr_pers[data->apers];
> +		*val = period / USEC_PER_SEC;
> +		*val2 = period % USEC_PER_SEC;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int tcs3472_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)
> +{
> +	struct tcs3472_data *data = iio_priv(indio_dev);
> +	int ret;
> +	u8 command;
> +	unsigned int period;
> +	int i;
> +
> +	mutex_lock(&data->lock);
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			command = TCS3472_AIHT;
> +			break;
> +		case IIO_EV_DIR_FALLING:
> +			command = TCS3472_AILT;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +		ret = i2c_smbus_write_word_data(data->client, command, val);
> +		if (ret)
> +			goto error;
> +
> +		if (dir == IIO_EV_DIR_RISING)
> +			data->high_thresh = val;
> +		else
> +			data->low_thresh = val;
> +		break;
> +	case IIO_EV_INFO_PERIOD:
> +		period = val * USEC_PER_SEC + val2;
> +		for (i = 1; i < ARRAY_SIZE(tcs3472_intr_pers) - 1; i++) {
> +			if (period <= (256 - data->atime) * 2400 *
> +					tcs3472_intr_pers[i])
> +				break;
> +		}
> +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS, i);
> +		if (ret)
> +			goto error;
> +
> +		data->apers = i;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +error:
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int tcs3472_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 tcs3472_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = !!(data->enable & TCS3472_ENABLE_AIEN);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int tcs3472_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 tcs3472_data *data = iio_priv(indio_dev);
> +	int ret = 0;
> +	u8 enable_old;
> +
> +	mutex_lock(&data->lock);
> +
> +	enable_old = data->enable;
> +
> +	if (state)
> +		data->enable |= TCS3472_ENABLE_AIEN;
> +	else
> +		data->enable &= ~TCS3472_ENABLE_AIEN;
> +
> +	if (enable_old != data->enable) {
> +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> +						data->enable);
> +		if (ret)
> +			data->enable = enable_old;
> +	}
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t tcs3472_event_handler(int irq, void *priv)
> +{
> +	struct iio_dev *indio_dev = priv;
> +	struct tcs3472_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, TCS3472_STATUS);
> +	if (ret >= 0 && (ret & TCS3472_STATUS_AINT)) {
> +		iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, 0,
> +						IIO_EV_TYPE_THRESH,
> +						IIO_EV_DIR_EITHER),
> +				iio_get_time_ns(indio_dev));
> +
> +		i2c_smbus_read_byte_data(data->client, TCS3472_INTR_CLEAR);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t tcs3472_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -243,6 +434,10 @@ static const struct attribute_group tcs3472_attribute_group = {
>  static const struct iio_info tcs3472_info = {
>  	.read_raw = tcs3472_read_raw,
>  	.write_raw = tcs3472_write_raw,
> +	.read_event_value = tcs3472_read_event,
> +	.write_event_value = tcs3472_write_event,
> +	.read_event_config = tcs3472_read_event_config,
> +	.write_event_config = tcs3472_write_event_config,
>  	.attrs = &tcs3472_attribute_group,
>  	.driver_module = THIS_MODULE,
>  };
> @@ -261,6 +456,7 @@ static int tcs3472_probe(struct i2c_client *client,
>  	data = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
> +	mutex_init(&data->lock);
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->info = &tcs3472_info;
> @@ -290,12 +486,29 @@ static int tcs3472_probe(struct i2c_client *client,
>  		return ret;
>  	data->atime = ret;
>  
> +	ret = i2c_smbus_read_word_data(data->client, TCS3472_AILT);
> +	if (ret < 0)
> +		return ret;
> +	data->low_thresh = ret;
> +
> +	ret = i2c_smbus_read_word_data(data->client, TCS3472_AIHT);
> +	if (ret < 0)
> +		return ret;
> +	data->high_thresh = ret;
> +
> +	data->apers = 1;
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS,
> +					data->apers);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = i2c_smbus_read_byte_data(data->client, TCS3472_ENABLE);
>  	if (ret < 0)
>  		return ret;
>  
>  	/* enable device */
>  	data->enable = ret | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
> +	data->enable &= ~TCS3472_ENABLE_AIEN;
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  		data->enable);
>  	if (ret < 0)
> @@ -306,12 +519,29 @@ static int tcs3472_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (client->irq) {
> +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS,
> +						data->apers);
> +		if (ret)
> +			goto buffer_cleanup;
> +
> +		ret = request_threaded_irq(client->irq, NULL,
> +					   tcs3472_event_handler,
> +					   IRQF_TRIGGER_FALLING | IRQF_SHARED |
> +					   IRQF_ONESHOT,
> +					   client->name, indio_dev);
> +		if (ret)
> +			goto buffer_cleanup;
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
> -		goto buffer_cleanup;
> +		goto free_irq;
>  
>  	return 0;
>  
> +free_irq:
> +	free_irq(client->irq, indio_dev);
>  buffer_cleanup:
>  	iio_triggered_buffer_cleanup(indio_dev);
>  	return ret;
> @@ -319,14 +549,26 @@ static int tcs3472_probe(struct i2c_client *client,
>  
>  static int tcs3472_powerdown(struct tcs3472_data *data)
>  {
> -	return i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> -		data->enable & ~(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON));
> +	int ret;
> +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> +		data->enable & ~enable_mask);
> +	if (!ret)
> +		data->enable &= ~enable_mask;
> +
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
>  }
>  
>  static int tcs3472_remove(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>  
> +	free_irq(client->irq, indio_dev);
>  	iio_device_unregister(indio_dev);
>  	iio_triggered_buffer_cleanup(indio_dev);
>  	tcs3472_powerdown(iio_priv(indio_dev));
> @@ -346,8 +588,19 @@ static int tcs3472_resume(struct device *dev)
>  {
>  	struct tcs3472_data *data = iio_priv(i2c_get_clientdata(
>  		to_i2c_client(dev)));
> -	return i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> -		data->enable | (TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON));
> +	int ret;
> +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> +		data->enable | enable_mask);
> +	if (!ret)
> +		data->enable |= enable_mask;
> +
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
>  }
>  #endif
>  

Re: [PATCH 2/2] iio: light: tcs3472: support out-of-threshold events

[Peter Meerwald-Stadler]

> > The TCS3472 device provides interrupt signal for out-of-threshold events
> > with persistence filter.
> > 
> > This change adds interrupt support for the threshold events and enables
> > to configure the period of time by persistence filter.
> > 
> > Cc: Peter Meerwald <pmeerw@pmeerw.net>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> One comment inline.  Looks fine to me, but I'd like to give
> Peter more time to have a look at it.

my comments below, mostly minor
 
> If we don't hear from Peter within two weeks (ish) give me a bump
> and I'll either chase up or take the view Peter is too busy.
> 
> Jonathan
> > ---
> >  drivers/iio/light/tcs3472.c | 269 ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 261 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> > index a9e153b..905e777 100644
> > --- a/drivers/iio/light/tcs3472.c
> > +++ b/drivers/iio/light/tcs3472.c
> > @@ -11,7 +11,9 @@
> >   * 7-bit I2C slave address 0x39 (TCS34721, TCS34723) or 0x29 (TCS34725,
> >   * TCS34727)
> >   *
> > - * TODO: interrupt support, thresholds, wait time
> > + * Datasheet: http://ams.com/eng/content/download/319364/1117183/file/TCS3472_Datasheet_EN_v2.pdf
> I'd have have marginally preferred this change as a separate patch, but if everything else
> is fine I don't care enough to delay this.
> > + *
> > + * TODO: wait time
> >   */
> >  
> >  #include <linux/module.h>
> > @@ -21,6 +23,7 @@
> >  
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/events.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/triggered_buffer.h>
> > @@ -29,12 +32,15 @@
> >  
> >  #define TCS3472_COMMAND BIT(7)
> >  #define TCS3472_AUTO_INCR BIT(5)
> > +#define TCS3472_SPECIAL_FUNC (BIT(5) | BIT(6))
> > +
> > +#define TCS3472_INTR_CLEAR (TCS3472_COMMAND | TCS3472_SPECIAL_FUNC | 0x6)

nitpick: the special function code is 5 bits, so should be 0x06 instead of 
0x6
  
> >  #define TCS3472_ENABLE (TCS3472_COMMAND | 0x00)
> >  #define TCS3472_ATIME (TCS3472_COMMAND | 0x01)
> >  #define TCS3472_WTIME (TCS3472_COMMAND | 0x03)
> > -#define TCS3472_AILT (TCS3472_COMMAND | 0x04)
> > -#define TCS3472_AIHT (TCS3472_COMMAND | 0x06)
> > +#define TCS3472_AILT (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x04)
> > +#define TCS3472_AIHT (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x06)
> >  #define TCS3472_PERS (TCS3472_COMMAND | 0x0c)
> >  #define TCS3472_CONFIG (TCS3472_COMMAND | 0x0d)
> >  #define TCS3472_CONTROL (TCS3472_COMMAND | 0x0f)
> > @@ -45,19 +51,42 @@
> >  #define TCS3472_GDATA (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x18)
> >  #define TCS3472_BDATA (TCS3472_COMMAND | TCS3472_AUTO_INCR | 0x1a)
> >  
> > +#define TCS3472_STATUS_AINT BIT(4)
> >  #define TCS3472_STATUS_AVALID BIT(0)
> > +#define TCS3472_ENABLE_AIEN BIT(4)
> >  #define TCS3472_ENABLE_AEN BIT(1)
> >  #define TCS3472_ENABLE_PON BIT(0)
> >  #define TCS3472_CONTROL_AGAIN_MASK (BIT(0) | BIT(1))
> >  
> >  struct tcs3472_data {
> >  	struct i2c_client *client;
> > +	struct mutex lock;
> > +	u16 low_thresh;
> > +	u16 high_thresh;
> >  	u8 enable;
> >  	u8 control;
> >  	u8 atime;
> > +	u8 apers;
> >  	u16 buffer[8]; /* 4 16-bit channels + 64-bit timestamp */
> >  };
> >  
> > +static const struct iio_event_spec tcs3472_events[] = {
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> > +	}, {
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> > +	}, {
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_EITHER,
> > +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> > +				 BIT(IIO_EV_INFO_PERIOD),
> > +	},
> > +};
> > +
> >  #define TCS3472_CHANNEL(_color, _si, _addr) { \
> >  	.type = IIO_INTENSITY, \
> >  	.modified = 1, \
> > @@ -73,6 +102,8 @@ struct tcs3472_data {
> >  		.storagebits = 16, \
> >  		.endianness = IIO_CPU, \
> >  	}, \
> > +	.event_spec = _si ? NULL : tcs3472_events, \
> > +	.num_event_specs = _si ? 0 : ARRAY_SIZE(tcs3472_events), \
> >  }
> >  
> >  static const int tcs3472_agains[] = { 1, 4, 16, 60 };
> > @@ -180,6 +211,166 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
> >  	return -EINVAL;
> >  }
> >  
> > +/*
> > + * Translation from APERS field value to the number of consecutive out-of-range
> > + * Clear occurrences before an interrupt is generated

this wording could be improved, maybe 2nd line could be:
* clear channel values before an interrupt is generated

> > + */
> > +static const int tcs3472_intr_pers[] = {
> > +	0, 1, 2, 3, 5, 10, 15, 20, 25, 30, 35, 40, 45, 50, 55, 60
> > +};
> > +
> > +static int tcs3472_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)
> > +{
> > +	struct tcs3472_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +	unsigned int period;
> > +
> > +	mutex_lock(&data->lock);
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		*val = (dir == IIO_EV_DIR_RISING) ?
> > +			data->high_thresh : data->low_thresh;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_EV_INFO_PERIOD:
> > +		period = (256 - data->atime) * 2400 *
> > +			tcs3472_intr_pers[data->apers];
> > +		*val = period / USEC_PER_SEC;
> > +		*val2 = period % USEC_PER_SEC;
> > +		ret = IIO_VAL_INT_PLUS_MICRO;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int tcs3472_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)
> > +{
> > +	struct tcs3472_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +	u8 command;
> > +	unsigned int period;
> > +	int i;
> > +
> > +	mutex_lock(&data->lock);
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		switch (dir) {
> > +		case IIO_EV_DIR_RISING:
> > +			command = TCS3472_AIHT;
> > +			break;
> > +		case IIO_EV_DIR_FALLING:
> > +			command = TCS3472_AILT;
> > +			break;
> > +		default:
> > +			ret = -EINVAL;

no, it should 
goto error 
here; otherwise command is undefined in the write below

> > +			break;
> > +		}
> > +		ret = i2c_smbus_write_word_data(data->client, command, val);
> > +		if (ret)
> > +			goto error;
> > +
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			data->high_thresh = val;
> > +		else
> > +			data->low_thresh = val;
> > +		break;
> > +	case IIO_EV_INFO_PERIOD:
> > +		period = val * USEC_PER_SEC + val2;

what if val is negative?

> > +		for (i = 1; i < ARRAY_SIZE(tcs3472_intr_pers) - 1; i++) {
> > +			if (period <= (256 - data->atime) * 2400 *
> > +					tcs3472_intr_pers[i])
> > +				break;
> > +		}
> > +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS, i);
> > +		if (ret)
> > +			goto error;
> > +
> > +		data->apers = i;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +error:
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int tcs3472_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 tcs3472_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = !!(data->enable & TCS3472_ENABLE_AIEN);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int tcs3472_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 tcs3472_data *data = iio_priv(indio_dev);
> > +	int ret = 0;
> > +	u8 enable_old;
> > +
> > +	mutex_lock(&data->lock);
> > +
> > +	enable_old = data->enable;
> > +
> > +	if (state)
> > +		data->enable |= TCS3472_ENABLE_AIEN;
> > +	else
> > +		data->enable &= ~TCS3472_ENABLE_AIEN;
> > +
> > +	if (enable_old != data->enable) {
> > +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> > +						data->enable);
> > +		if (ret)
> > +			data->enable = enable_old;
> > +	}
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t tcs3472_event_handler(int irq, void *priv)
> > +{
> > +	struct iio_dev *indio_dev = priv;
> > +	struct tcs3472_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_byte_data(data->client, TCS3472_STATUS);
> > +	if (ret >= 0 && (ret & TCS3472_STATUS_AINT)) {
> > +		iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, 0,
> > +						IIO_EV_TYPE_THRESH,
> > +						IIO_EV_DIR_EITHER),
> > +				iio_get_time_ns(indio_dev));
> > +
> > +		i2c_smbus_read_byte_data(data->client, TCS3472_INTR_CLEAR);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static irqreturn_t tcs3472_trigger_handler(int irq, void *p)
> >  {
> >  	struct iio_poll_func *pf = p;
> > @@ -243,6 +434,10 @@ static const struct attribute_group tcs3472_attribute_group = {
> >  static const struct iio_info tcs3472_info = {
> >  	.read_raw = tcs3472_read_raw,
> >  	.write_raw = tcs3472_write_raw,
> > +	.read_event_value = tcs3472_read_event,
> > +	.write_event_value = tcs3472_write_event,
> > +	.read_event_config = tcs3472_read_event_config,
> > +	.write_event_config = tcs3472_write_event_config,
> >  	.attrs = &tcs3472_attribute_group,
> >  	.driver_module = THIS_MODULE,
> >  };
> > @@ -261,6 +456,7 @@ static int tcs3472_probe(struct i2c_client *client,
> >  	data = iio_priv(indio_dev);
> >  	i2c_set_clientdata(client, indio_dev);
> >  	data->client = client;
> > +	mutex_init(&data->lock);
> >  
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->info = &tcs3472_info;
> > @@ -290,12 +486,29 @@ static int tcs3472_probe(struct i2c_client *client,
> >  		return ret;
> >  	data->atime = ret;
> >  
> > +	ret = i2c_smbus_read_word_data(data->client, TCS3472_AILT);
> > +	if (ret < 0)
> > +		return ret;
> > +	data->low_thresh = ret;
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, TCS3472_AIHT);
> > +	if (ret < 0)
> > +		return ret;
> > +	data->high_thresh = ret;
> > +
> > +	data->apers = 1;
> > +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS,
> > +					data->apers);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	ret = i2c_smbus_read_byte_data(data->client, TCS3472_ENABLE);
> >  	if (ret < 0)
> >  		return ret;
> >  
> >  	/* enable device */
> >  	data->enable = ret | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
> > +	data->enable &= ~TCS3472_ENABLE_AIEN;
> >  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> >  		data->enable);
> >  	if (ret < 0)
> > @@ -306,12 +519,29 @@ static int tcs3472_probe(struct i2c_client *client,
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	if (client->irq) {
> > +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS,
> > +						data->apers);

_PERS has been written before?

> > +		if (ret)
> > +			goto buffer_cleanup;
> > +
> > +		ret = request_threaded_irq(client->irq, NULL,
> > +					   tcs3472_event_handler,
> > +					   IRQF_TRIGGER_FALLING | IRQF_SHARED |
> > +					   IRQF_ONESHOT,
> > +					   client->name, indio_dev);
> > +		if (ret)
> > +			goto buffer_cleanup;
> > +	}
> > +
> >  	ret = iio_device_register(indio_dev);
> >  	if (ret < 0)
> > -		goto buffer_cleanup;
> > +		goto free_irq;
> >  
> >  	return 0;
> >  
> > +free_irq:
> > +	free_irq(client->irq, indio_dev);
> >  buffer_cleanup:
> >  	iio_triggered_buffer_cleanup(indio_dev);
> >  	return ret;
> > @@ -319,14 +549,26 @@ static int tcs3472_probe(struct i2c_client *client,
> >  
> >  static int tcs3472_powerdown(struct tcs3472_data *data)
> >  {
> > -	return i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> > -		data->enable & ~(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON));
> > +	int ret;
> > +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> > +
> > +	mutex_lock(&data->lock);
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> > +		data->enable & ~enable_mask);
> > +	if (!ret)
> > +		data->enable &= ~enable_mask;
> > +
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> >  }
> >  
> >  static int tcs3472_remove(struct i2c_client *client)
> >  {
> >  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> >  
> > +	free_irq(client->irq, indio_dev);

teardown should be in reverse order, so free_irq() should be below 
_unregister()

> >  	iio_device_unregister(indio_dev);
> >  	iio_triggered_buffer_cleanup(indio_dev);
> >  	tcs3472_powerdown(iio_priv(indio_dev));
> > @@ -346,8 +588,19 @@ static int tcs3472_resume(struct device *dev)
> >  {
> >  	struct tcs3472_data *data = iio_priv(i2c_get_clientdata(
> >  		to_i2c_client(dev)));
> > -	return i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> > -		data->enable | (TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON));
> > +	int ret;
> > +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> > +
> > +	mutex_lock(&data->lock);
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> > +		data->enable | enable_mask);
> > +	if (!ret)
> > +		data->enable |= enable_mask;
> > +
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> >  }
> >  #endif
> >  
> 

-- 

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

Re: [PATCH 1/2] iio: light: tcs3472: fix ATIME register write

[Peter Meerwald-Stadler]

> > The integration time is controlled by the ATIME register only.  However,
> > this register is written by i2c_smbus_write_word_data() in write_raw().
> > 
> > We actually don't need to write a subsequent register.  So just use
> > i2c_smbus_write_byte_data() instead.

> > Cc: Peter Meerwald <pmeerw@pmeerw.net>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> As I read the datasheet, this looks like it won't cause any actual
> harm (where the top byte is written is unused).

looks good to me as well,
Acked-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>

> > ---
> >  drivers/iio/light/tcs3472.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> > index 3aa71e3..a9e153b 100644
> > --- a/drivers/iio/light/tcs3472.c
> > +++ b/drivers/iio/light/tcs3472.c
> > @@ -169,7 +169,7 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
> >  		for (i = 0; i < 256; i++) {
> >  			if (val2 == (256 - i) * 2400) {
> >  				data->atime = i;
> > -				return i2c_smbus_write_word_data(
> > +				return i2c_smbus_write_byte_data(
> >  					data->client, TCS3472_ATIME,
> >  					data->atime);
> >  			}
> 

-- 

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