> > 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@xxxxxxxxxx> > > Cc: Jonathan Cameron <jic23@xxxxxxxxxx> > > Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
