On 07/20/2014 09:04 AM, Jonathan Cameron wrote:
On 17/07/14 01:42, Srinivas Pandruvada wrote:This chip can operate in two modes. In one mode it can issue periodic interrupts based on sample rate setting or when there is a motion.But not both? This had me confused below.
The purpose of this change is so that CPU doesn't wake up to process interrupt when there is no significant change, when user space application can tolerate by using event thresholds. This tolerance depends on use case. If you enable both, then this is not meaningful as data ready will generate continuous interrupts anyway taking precedence.
Also, the event description is incorrect for conventional motion detectionWe don't want to depend on event enable when there is no threshold set, events will have(magnitude rising detection). JUsing events mechanism to allow configuration and receive events. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> ---drivers/iio/accel/kxcjk-1013.c | 238 +++++++++++++++++++++++++++++++++++++++--1 file changed, 230 insertions(+), 8 deletions(-)diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.cindex 975f8a6..f5bb682 100644 --- a/drivers/iio/accel/kxcjk-1013.c +++ b/drivers/iio/accel/kxcjk-1013.c @@ -27,6 +27,7 @@ #include <linux/iio/sysfs.h> #include <linux/iio/buffer.h> #include <linux/iio/trigger.h> +#include <linux/iio/events.h> #include <linux/iio/trigger_consumer.h> #include <linux/iio/triggered_buffer.h> #include <linux/iio/accel/kxcjk_1013.h> @@ -92,6 +93,9 @@ struct kxcjk1013_data { u8 odr_bits; u8 range; bool active_high_intr; + int ev_enable_state; + int wake_thres; + int wake_dur; bool trigger_on; }; @@ -116,6 +120,23 @@ static const struct { {200, 0, 0x04}, {400, 0, 0x05}, {800, 0, 0x06}, {1600, 0, 0x07} }; +static const struct { + int val; + int val2; + int odr_bits; +} wake_odr_data_rate_table[] = { {0, 781000, 0x00}, + {1, 563000, 0x01}, + {3, 125000, 0x02}, + {6, 250000, 0x03}, + {12, 500000, 0x04}, + {25, 0, 0x05}, + {50, 0, 0x06}, + {100, 0, 0x06}, + {200, 0, 0x06}, + {400, 0, 0x06}, + {800, 0, 0x06}, + {1600, 0, 0x06} }; + /* Refer to section 4 of the specification */ static const struct { int odr_bits;@@ -283,10 +304,36 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)return 0; }+static int kxcjk1013_chip_update_thresholds(struct kxcjk1013_data *data)+{ + int ret; + + ret = i2c_smbus_write_byte_data(data->client, + KXCJK1013_REG_WAKE_TIMER, + data->wake_dur); + if (ret < 0) { + dev_err(&data->client->dev, + "Error writing reg_wake_timer\n"); + return ret; + } + + ret = i2c_smbus_write_byte_data(data->client, + KXCJK1013_REG_WAKE_THRES, + data->wake_thres); + if (ret < 0) { + dev_err(&data->client->dev, + "Error writing reg_wake_thres\n"); + return ret; + } + + return 0; +} + static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data, bool status) { int ret; + u8 intr_bit; enum kxcjk1013_mode store_mode; ret = kxcjk1013_get_mode(data, &store_mode);@@ -316,6 +363,16 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,return ret; } + if (data->wake_thres) {rather than ev_enable_state?
no meaning.
+ if (status) { + ret = kxcjk1013_chip_update_thresholds(data); + if (ret < 0) + return ret; + } + intr_bit = KXCJK1013_REG_CTRL1_BIT_WUFE; + } else + intr_bit = KXCJK1013_REG_CTRL1_BIT_DRDY; +Does this mean the dataready interrupt is disabled whenever thresholds are non zero? This needs some explanation.
Data ready will take precedence and will send periodic interrupts.
ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1); if (ret < 0) { dev_err(&data->client->dev, "Error reading reg_ctrl1\n");@@ -323,9 +380,10 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,} if (status) - ret |= KXCJK1013_REG_CTRL1_BIT_DRDY; + ret |= intr_bit; else - ret &= ~KXCJK1013_REG_CTRL1_BIT_DRDY; + ret &= ~(KXCJK1013_REG_CTRL1_BIT_WUFE | + KXCJK1013_REG_CTRL1_BIT_DRDY); ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_CTRL1, ret);@@ -357,6 +415,20 @@ static int kxcjk1013_convert_freq_to_bit(int val, int val2)return -EINVAL; } +static int kxcjk1013_convert_wake_odr_to_bit(int val, int val2) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(wake_odr_data_rate_table); ++i) { + if (wake_odr_data_rate_table[i].val == val && + wake_odr_data_rate_table[i].val2 == val2) { + return wake_odr_data_rate_table[i].odr_bits; + } + } + + return -EINVAL; +} +static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2){ int ret;@@ -385,6 +457,17 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)data->odr_bits = odr_bits; + odr_bits = kxcjk1013_convert_wake_odr_to_bit(val, val2); + if (odr_bits < 0) + return odr_bits; + + ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_CTRL2, + odr_bits); + if (ret < 0) { + dev_err(&data->client->dev, "Error writing reg_ctrl2\n"); + return ret; + } + if (store_mode == OPERATION) { ret = kxcjk1013_set_mode(data, OPERATION); if (ret < 0)@@ -567,6 +650,94 @@ static int kxcjk1013_write_raw(struct iio_dev *indio_dev,return ret; } +static int kxcjk1013_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 kxcjk1013_data *data = iio_priv(indio_dev); + + *val2 = 0; + switch (info) { + case IIO_EV_INFO_VALUE: + *val = data->wake_thres; + break; + case IIO_EV_INFO_PERIOD: + *val = data->wake_dur; + break; + default: + return -EINVAL; + } + + return IIO_VAL_INT; +} + +static int kxcjk1013_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 kxcjk1013_data *data = iio_priv(indio_dev); +Perhaps check val2 == 0
OK
Makes sense. I interpret as x,y, z, change in any direction will result in event.+ switch (info) { + case IIO_EV_INFO_VALUE: + data->wake_thres = val; + break; + case IIO_EV_INFO_PERIOD: + data->wake_dur = val;Double space above.+ break; + default: + return -EINVAL; + } + + return 0; +} + +static int kxcjk1013_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 kxcjk1013_data *data = iio_priv(indio_dev); + + return data->ev_enable_state; +} + +static int kxcjk1013_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 kxcjk1013_data *data = iio_priv(indio_dev); + int ret; + + if (data->trigger_on) + return -EAGAIN; + + if (state && data->ev_enable_state) + return 0; + + mutex_lock(&data->mutex); + ret = kxcjk1013_chip_setup_interrupt(data, state); + if (!ret) { + ret = kxcjk1013_set_power_state(data, state); + if (ret < 0) { + mutex_unlock(&data->mutex); + return ret; + } + } + data->ev_enable_state = state; + mutex_unlock(&data->mutex); + + return 0; +} + static int kxcjk1013_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig) {@@ -593,6 +764,14 @@ static const struct attribute_group kxcjk1013_attrs_group = {.attrs = kxcjk1013_attributes, }; +static const struct iio_event_spec kxcjk1013_event = { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_EITHER,As stated below I'm a little doubtful this is correct.+ .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE) | + BIT(IIO_EV_INFO_PERIOD) +}; + #define KXCJK1013_CHANNEL(_axis) { \ .type = IIO_ACCEL, \ .modified = 1, \@@ -608,6 +787,8 @@ static const struct attribute_group kxcjk1013_attrs_group = {.shift = 4, \ .endianness = IIO_LE, \ }, \ + .event_spec = &kxcjk1013_event, \ + .num_event_specs = 1 \ } static const struct iio_chan_spec kxcjk1013_channels[] = { @@ -621,6 +802,10 @@ static const struct iio_info kxcjk1013_info = { .attrs = &kxcjk1013_attrs_group, .read_raw = kxcjk1013_read_raw, .write_raw = kxcjk1013_write_raw, + .read_event_value = kxcjk1013_read_event, + .write_event_value = kxcjk1013_write_event, + .write_event_config = kxcjk1013_write_event_config, + .read_event_config = kxcjk1013_read_event_config, .validate_trigger = kxcjk1013_validate_trigger, .driver_module = THIS_MODULE, };@@ -675,6 +860,9 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,struct kxcjk1013_data *data = iio_priv(indio_dev); int ret; + if (data->ev_enable_state) + return -EAGAIN; + if (state && data->trigger_on) return 0;@@ -699,6 +887,38 @@ static const struct iio_trigger_ops kxcjk1013_trigger_ops = {.owner = THIS_MODULE, }; +static irqreturn_t kxcjk1013_event_handler(int irq, void *private) +{ + struct iio_dev *indio_dev = private; + struct kxcjk1013_data *data = iio_priv(indio_dev); + int ret; +Slight preference for a comment here saying what the event is...Really? Note this means that a fall in the magnitude will trigger the event+ iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL, + 0, + IIO_MOD_X_OR_Y_OR_Z, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_EITHER),as well as a rise? (e.g. stop of a previously existing motion). It's possible but seems unlikely. I'm guessing you want IIO_EV_TYPE_MAG and IIO_EV_DIR_RISING.
I will correct this.
+ iio_get_time_ns()); ++ ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_REL);+ if (ret < 0) + dev_err(&data->client->dev, "Error reading reg_int_rel\n"); + + return IRQ_HANDLED; +} + +static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private) +{ + struct iio_dev *indio_dev = private; + struct kxcjk1013_data *data = iio_priv(indio_dev); + + if (data->trigger_on) { + iio_trigger_poll(data->trig); + return IRQ_HANDLED; + } else + return IRQ_WAKE_THREAD; +} + static int kxcjk1013_acpi_gpio_probe(struct i2c_client *client, struct kxcjk1013_data *data) {@@ -783,11 +1003,13 @@ static int kxcjk1013_probe(struct i2c_client *client,data->trig_mode = true; - ret = devm_request_irq(&client->dev, client->irq, - iio_trigger_generic_data_rdy_poll, - IRQF_TRIGGER_RISING, - KXCJK1013_IRQ_NAME, - trig); + ret = devm_request_threaded_irq(&client->dev, client->irq, + kxcjk1013_data_rdy_trig_poll, + kxcjk1013_event_handler, + IRQF_TRIGGER_RISING | + IRQF_ONESHOT, + KXCJK1013_IRQ_NAME, + indio_dev); if (ret) { dev_err(&client->dev, "unable to request IRQ\n"); goto err_trigger_free; @@ -889,7 +1111,7 @@ static int kxcjk1013_resume(struct device *dev) mutex_lock(&data->mutex); /* Check, if the suspend occured while active */ - if (data->trigger_on) + if (data->trigger_on || data->ev_enable_state) ret = kxcjk1013_set_mode(data, OPERATION); mutex_unlock(&data->mutex);
-- 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
