Re: [PATCH 5/6] iio: accel: kxcjk-1013: Support thresholds

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

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 
> detection
> (magnitude rising detection).
>
> J
>> Using events mechanism to allow configuration and receive events.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> ---
>>   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.c
>> index 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?
We don't want to depend on event enable when there is no threshold set, 
events will have
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
>> +    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...
>> +    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),
> Really? Note this means that a fall in the magnitude will trigger the 
> event
> 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.
Makes sense. I interpret as x,y, z, change in any direction will result 
in event.
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);
>>
>>
>
>