From: Jonathan Cameron <jic23@kernel.org>
To: Akinobu Mita <akinobu.mita@gmail.com>
Cc: linux-iio@vger.kernel.org, Peter Meerwald <pmeerw@pmeerw.net>
Subject: Re: [PATCH 2/2] iio: light: tcs3472: support out-of-threshold events
Date: Wed, 21 Jun 2017 20:33:49 +0100 [thread overview]
Message-ID: <20170621203349.089ed3b9@kernel.org> (raw)
In-Reply-To: <1497279909-11197-3-git-send-email-akinobu.mita@gmail.com>
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
>
next prev parent reply other threads:[~2017-06-21 19:33 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-12 15:05 [PATCH 0/2] iio: light: tcs3472: bug fix and iio event handling support Akinobu Mita
2017-06-12 15:05 ` [PATCH 1/2] iio: light: tcs3472: fix ATIME register write Akinobu Mita
2017-06-21 19:29 ` Jonathan Cameron
2017-06-21 20:38 ` Peter Meerwald-Stadler
2017-06-12 15:05 ` [PATCH 2/2] iio: light: tcs3472: support out-of-threshold events Akinobu Mita
2017-06-21 19:33 ` Jonathan Cameron [this message]
2017-06-21 20:38 ` Peter Meerwald-Stadler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170621203349.089ed3b9@kernel.org \
--to=jic23@kernel.org \
--cc=akinobu.mita@gmail.com \
--cc=linux-iio@vger.kernel.org \
--cc=pmeerw@pmeerw.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.