by Hartmut Knaack

[permalink] [raw]

Subject: Re: [PATCH v4 6/6] iio: imu: kmx61: Add support for any motion trigger

Daniel Baluta schrieb am 03.12.2014 um 14:31:
> We use WUFE (Wake Up from Sleep Engine) and BTSE (Back to Sleep Engine)
> to detect general motion input.
>
A few recommendations, issues and questions inline.
> Signed-off-by: Daniel Baluta <[email protected]>
> ---
> drivers/iio/imu/kmx61.c | 433 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 426 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> index 0f6816a..972424b 100644
> --- a/drivers/iio/imu/kmx61.c
> +++ b/drivers/iio/imu/kmx61.c
> @@ -20,6 +20,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> #include <linux/iio/trigger.h>
> #include <linux/iio/buffer.h>
> #include <linux/iio/triggered_buffer.h>
> @@ -30,6 +31,8 @@
> #define KMX61_IRQ_NAME "kmx61_event"
>
> #define KMX61_REG_WHO_AM_I 0x00
> +#define KMX61_REG_INS1 0x01
> +#define KMX61_REG_INS2 0x02
>
> /*
> * three 16-bit accelerometer output registers for X/Y/Z axis
> @@ -63,20 +66,36 @@
> #define KMX61_REG_INL 0x28
> #define KMX61_REG_STBY 0x29
> #define KMX61_REG_CTRL1 0x2A
> +#define KMX61_REG_CTRL2 0x2B
> #define KMX61_REG_ODCNTL 0x2C
> #define KMX61_REG_INC1 0x2D
>
> +#define KMX61_REG_WUF_THRESH 0x3D
> +#define KMX61_REG_WUF_TIMER 0x3E
> +
> #define KMX61_ACC_STBY_BIT BIT(0)
> #define KMX61_MAG_STBY_BIT BIT(1)
> #define KMX61_ACT_STBY_BIT BIT(7)
>
> #define KMX61_ALL_STBY (KMX61_ACC_STBY_BIT | KMX61_MAG_STBY_BIT)
>
> +#define KMX61_REG_INS1_BIT_WUFS BIT(1)
> +
> +#define KMX61_REG_INS2_BIT_ZP BIT(0)
> +#define KMX61_REG_INS2_BIT_ZN BIT(1)
> +#define KMX61_REG_INS2_BIT_YP BIT(2)
> +#define KMX61_REG_INS2_BIT_YN BIT(3)
> +#define KMX61_REG_INS2_BIT_XP BIT(4)
> +#define KMX61_REG_INS2_BIT_XN BIT(5)
> +
> #define KMX61_REG_CTRL1_GSEL_MASK 0x03
>
> #define KMX61_REG_CTRL1_BIT_RES BIT(4)
> #define KMX61_REG_CTRL1_BIT_DRDYE BIT(5)
> +#define KMX61_REG_CTRL1_BIT_WUFE BIT(6)
> +#define KMX61_REG_CTRL1_BIT_BTSE BIT(7)
>
> +#define KMX61_REG_INC1_BIT_WUFS BIT(0)
> #define KMX61_REG_INC1_BIT_DRDYM BIT(1)
> #define KMX61_REG_INC1_BIT_DRDYA BIT(2)
> #define KMX61_REG_INC1_BIT_IEN BIT(5)
> @@ -86,6 +105,11 @@
> #define KMX61_ACC_ODR_MASK 0x0F
> #define KMX61_MAG_ODR_MASK 0xF0
>
> +#define KMX61_OWUF_MASK 0x7
> +
> +#define KMX61_DEFAULT_WAKE_THRESH 1
> +#define KMX61_DEFAULT_WAKE_DURATION 1
> +
> #define KMX61_SLEEP_DELAY_MS 2000
>
> #define KMX61_CHIP_ID 0x12
> @@ -111,11 +135,16 @@ struct kmx61_data {
> /* config bits */
> u8 range;
> u8 odr_bits;
> + u8 wake_thresh;
> + u8 wake_duration;
>
> /* accelerometer specific data */
> struct iio_dev *acc_indio_dev;
> struct iio_trigger *acc_dready_trig;
> + struct iio_trigger *motion_trig;
> bool acc_dready_trig_on;
> + bool motion_trig_on;
> + bool ev_enable_state;
>
> /* magnetometer specific data */
> struct iio_dev *mag_indio_dev;
> @@ -154,6 +183,23 @@ static const struct {
> {3, 125000, 0x0A},
> {6, 250000, 0x0B} };
>
> +static const struct {
> + int val;
> + int val2;
> + int odr_bits;
> +} kmx61_wake_up_odr_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} };
Is it intentional, that values above 50 share the same odr_bits?
> +
> static IIO_CONST_ATTR(accel_scale_available, "0.009582 0.019163 0.038326");
> static IIO_CONST_ATTR(magn_scale_available, "0.001465");
> static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> @@ -179,6 +225,14 @@ static const struct attribute_group kmx61_mag_attribute_group = {
> .attrs = kmx61_mag_attributes,
> };
>
> +static const struct iio_event_spec kmx61_event = {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_PERIOD),
> +};
> +
> #define KMX61_ACC_CHAN(_axis) { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -195,6 +249,8 @@ static const struct attribute_group kmx61_mag_attribute_group = {
> .shift = 4, \
> .endianness = IIO_LE, \
> }, \
> + .event_spec = &kmx61_event, \
> + .num_event_specs = 1 \
> }
>
> #define KMX61_MAG_CHAN(_axis) { \
> @@ -250,6 +306,31 @@ static int kmx61_convert_freq_to_bit(int val, int val2)
> return -EINVAL;
> }
>
> +static int kmx61_convert_bit_to_freq(u8 odr_bits, int *val, int *val2)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
> + if (odr_bits == kmx61_samp_freq_table[i].odr_bits) {
> + *val = kmx61_samp_freq_table[i].val;
> + *val2 = kmx61_samp_freq_table[i].val2;
> + return 0;
> + }
> + return -EINVAL;
Given that kmx61_samp_freq_table[].odr_bits is practically representing the
index, it could be handled in the way that we did when accessing
kmx61_uscale_table. Just check odr_bits to be in range of valid indexes and
then access the tables' elements with odr_bits used as index.
Not sure, why I didn't mention it on my first review - maybe I thought you
would pick it up by yourself if you liked the idea with the _scale_table.
> +}
> +
> +
> +static int kmx61_convert_wake_up_odr_to_bit(int val, int val2)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(kmx61_wake_up_odr_table); ++i)
> + if (kmx61_wake_up_odr_table[i].val == val &&
> + kmx61_wake_up_odr_table[i].val2 == val2)
> + return kmx61_wake_up_odr_table[i].odr_bits;
> + return -EINVAL;
> +}
> +
> /**
> * kmx61_set_mode() - set KMX61 device operating mode
> * @data - kmx61 device private data pointer
> @@ -338,6 +419,21 @@ static int kmx61_get_mode(struct kmx61_data *data, u8 *mode, u8 device)
> return 0;
> }
>
> +int kmx61_set_wake_up_odr(struct kmx61_data *data, int val, int val2)
> +{
> + int ret, odr_bits;
> +
> + odr_bits = kmx61_convert_wake_up_odr_to_bit(val, val2);
> + if (odr_bits < 0)
> + return odr_bits;
> +
> + ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL2,
> + odr_bits);
> + if (ret < 0)
> + dev_err(&data->client->dev, "Error writing reg_ctrl2\n");
> + return ret;
> +}
> +
> static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
> {
> int ret;
> @@ -369,6 +465,12 @@ static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
> if (ret < 0)
> return ret;
>
> + if (device & KMX61_ACC) {
> + ret = kmx61_set_wake_up_odr(data, val, val2);
> + if (ret)
> + return ret;
> + }
> +
> return kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
> }
>
> @@ -449,7 +551,7 @@ static int kmx61_set_scale(struct kmx61_data *data, u16 uscale)
>
> static int kmx61_chip_init(struct kmx61_data *data)
> {
> - int ret;
> + int ret, val, val2;
>
> ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_WHO_AM_I);
> if (ret < 0) {
> @@ -476,11 +578,23 @@ static int kmx61_chip_init(struct kmx61_data *data)
> }
> data->odr_bits = ret;
>
> + /* set output data rate for wake up (motion detection) function */
> + ret = kmx61_convert_bit_to_freq(data->odr_bits, &val, &val2);
> + if (ret < 0)
> + return ret;
> +
> + ret = kmx61_set_wake_up_odr(data, val, val2);
> + if (ret < 0)
> + return ret;
> +
> /* set acc/magn to OPERATION mode */
> ret = kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
> if (ret < 0)
> return ret;
>
> + data->wake_thresh = KMX61_DEFAULT_WAKE_THRESH;
> + data->wake_duration = KMX61_DEFAULT_WAKE_DURATION;
> +
> return 0;
> }
>
> @@ -547,6 +661,87 @@ static int kmx61_setup_new_data_interrupt(struct kmx61_data *data,
> return 0;
> }
>
> +static int kmx61_chip_update_thresholds(struct kmx61_data *data)
> +{
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + KMX61_REG_WUF_TIMER,
> + data->wake_duration);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Errow writing reg_wuf_timer\n");
> + return ret;
> + }
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + KMX61_REG_WUF_THRESH,
> + data->wake_thresh);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error writing reg_wuf_thresh\n");
> + return ret;
> + }
> +
> + return 0;
Like in kmx61_set_wake_up_odr, you could return ret here and drop it in the
last error handler.
> +}
> +
> +static int kmx61_setup_any_motion_interrupt(struct kmx61_data *data,
> + bool status, u8 device)
device is unused in this function.
> +{
> + u8 mode;
> + int ret;
> +
> + ret = kmx61_get_mode(data, &mode, KMX61_ACC | KMX61_MAG);
> + if (ret < 0)
> + return ret;
> +
> + ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> + if (ret < 0)
> + return ret;
> +
> + ret = kmx61_chip_update_thresholds(data);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INC1);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error reading reg_inc1\n");
> + return ret;
> + }
> + if (status)
> + ret |= (KMX61_REG_INC1_BIT_IEN | KMX61_REG_INC1_BIT_WUFS);
> + else
> + ret &= ~(KMX61_REG_INC1_BIT_IEN | KMX61_REG_INC1_BIT_WUFS);
> +
> + ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_INC1, ret);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error writing reg_inc1\n");
> + return ret;
> + }
> +
> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> + return ret;
> + }
> +
> + if (status)
> + ret |= KMX61_REG_CTRL1_BIT_WUFE | KMX61_REG_CTRL1_BIT_BTSE;
> + else
> + ret &= ~(KMX61_REG_CTRL1_BIT_WUFE | KMX61_REG_CTRL1_BIT_BTSE);
> +
> + ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
> + return ret;
> + }
> + mode |= KMX61_ACT_STBY_BIT;
> + ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
> + if (ret)
> + return ret;
> +
> + return 0;
Just like in the end of kmx61_set_odr(): return kmx61_set_mode(...);
> +}
> +
> /**
> * kmx61_set_power_state() - set power state for kmx61 @device
> * @data - kmx61 device private pointer
> @@ -707,12 +902,111 @@ static int kmx61_write_raw(struct iio_dev *indio_dev,
> }
> }
>
> +static int kmx61_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 kmx61_data *data = kmx61_get_data(indio_dev);
> +
> + *val2 = 0;
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + *val = data->wake_thresh;
> + break;
> + case IIO_EV_INFO_PERIOD:
> + *val = data->wake_duration;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return IIO_VAL_INT;
Stylewise, we usually put the return IIO_VAL_INT (or similar) inside each case
if there is nothing more to be done (like unlocking mutexes, ...). Makes it
slightly more obvious, what type the data is supposed to be.
> +}
> +
> +static int kmx61_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)
Parameters should be indented one more space.
> +{
> + struct kmx61_data *data = kmx61_get_data(indio_dev);
> +
> + if (data->ev_enable_state)
> + return -EBUSY;
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + data->wake_thresh = val;
> + break;
> + case IIO_EV_INFO_PERIOD:
> + data->wake_duration = val;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return IIO_VAL_INT;
Same here.
> +}
> +
> +static int kmx61_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 kmx61_data *data = kmx61_get_data(indio_dev);
> +
> + return data->ev_enable_state;
> +}
> +
> +static int kmx61_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)
Parameters should be indented one more space.
> +{
> + struct kmx61_data *data = kmx61_get_data(indio_dev);
> + int ret;
> +
> + if (state && data->ev_enable_state)
> + return 0;
> +
> + mutex_lock(&data->lock);
> +
> + if (!state && data->motion_trig_on) {
> + data->ev_enable_state = 0;
Assign true/false to bools?
> + mutex_unlock(&data->lock);
> + return 0;
> + }
> +
> + ret = kmx61_set_power_state(data, state, KMX61_ACC);
> + if (ret < 0) {
> + mutex_unlock(&data->lock);
> + return ret;
> + }
> +
> + ret = kmx61_setup_any_motion_interrupt(data, state, KMX61_ACC);
> + if (ret < 0) {
> + kmx61_set_power_state(data, false, KMX61_ACC);
> + mutex_unlock(&data->lock);
> + return ret;
> + }
> +
> + data->ev_enable_state = state;
You could place an error label here and return ret below to reduce some code
repetition in the error handlers above.
> + mutex_unlock(&data->lock);
> +
> + return 0;
> +}
> +
> static int kmx61_acc_validate_trigger(struct iio_dev *indio_dev,
> struct iio_trigger *trig)
> {
> struct kmx61_data *data = kmx61_get_data(indio_dev);
>
> - if (data->acc_dready_trig != trig)
> + if (data->acc_dready_trig != trig && data->motion_trig != trig)
> return -EINVAL;
>
> return 0;
> @@ -734,6 +1028,10 @@ static const struct iio_info kmx61_acc_info = {
> .read_raw = kmx61_read_raw,
> .write_raw = kmx61_write_raw,
> .attrs = &kmx61_acc_attribute_group,
> + .read_event_value = kmx61_read_event,
> + .write_event_value = kmx61_write_event,
> + .read_event_config = kmx61_read_event_config,
> + .write_event_config = kmx61_write_event_config,
> .validate_trigger = kmx61_acc_validate_trigger,
> };
>
> @@ -753,11 +1051,18 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
> u8 device;
>
> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> - struct kmx61_data *data = iio_priv(indio_dev);
> + struct kmx61_data *data = kmx61_get_data(indio_dev);
Oh well, fixing the issue I pointed out in your previous patch. LOL and ouch!
>
> mutex_lock(&data->lock);
>
> - if (data->acc_dready_trig == trig)
> + if (!state && data->ev_enable_state && data->motion_trig_on) {
> + data->motion_trig_on = false;
> + mutex_unlock(&data->lock);
> + return 0;
> + }
> +
> +
> + if (data->acc_dready_trig == trig || data->motion_trig)
Wouldn't you want to compare data->motion_trig with trig?
> device = KMX61_ACC;
> else
> device = KMX61_MAG;
> @@ -768,7 +1073,10 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
> return ret;
> }
>
> - ret = kmx61_setup_new_data_interrupt(data, state, device);
> + if (data->acc_dready_trig == trig || data->mag_dready_trig == trig)
> + ret = kmx61_setup_new_data_interrupt(data, state, device);
> + else
> + ret = kmx61_setup_any_motion_interrupt(data, state, KMX61_ACC);
> if (ret < 0) {
> kmx61_set_power_state(data, false, device);
> mutex_unlock(&data->lock);
> @@ -777,8 +1085,10 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
>
> if (data->acc_dready_trig == trig)
> data->acc_dready_trig_on = state;
> - else
> + else if (data->mag_dready_trig == trig)
> data->mag_dready_trig_on = state;
> + else
> + data->motion_trig_on = state;
>
> mutex_unlock(&data->lock);
>
> @@ -806,6 +1116,99 @@ static const struct iio_trigger_ops kmx61_trigger_ops = {
> .owner = THIS_MODULE,
> };
>
> +static irqreturn_t kmx61_event_handler(int irq, void *private)
> +{
> + struct kmx61_data *data = private;
> + struct iio_dev *indio_dev = data->acc_indio_dev;
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INS1);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error reading reg_ins1\n");
> + goto ack_intr;
> + }
> +
> + if (ret & KMX61_REG_INS1_BIT_WUFS) {
> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INS2);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error reading reg_ins2\n");
> + goto ack_intr;
> + }
> +
> + if (ret & KMX61_REG_INS2_BIT_XN)
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
> + 0,
> + IIO_MOD_X,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + 0);
> +
> + if (ret & KMX61_REG_INS2_BIT_XP)
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
> + 0,
> + IIO_MOD_X,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + 0);
> +
> + if (ret & KMX61_REG_INS2_BIT_YN)
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
> + 0,
> + IIO_MOD_Y,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + 0);
> +
> + if (ret & KMX61_REG_INS2_BIT_YP)
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
> + 0,
> + IIO_MOD_Y,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + 0);
> +
> + if (ret & KMX61_REG_INS2_BIT_ZN)
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
> + 0,
> + IIO_MOD_Z,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + 0);
> +
> + if (ret & KMX61_REG_INS2_BIT_ZP)
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
> + 0,
> + IIO_MOD_Z,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + 0);
> + }
> +
> +ack_intr:
> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
> + if (ret < 0)
> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> +
> + ret |= KMX61_REG_CTRL1_BIT_RES;
> + ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
> + if (ret < 0)
> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
Typo: Error writing ...
> +
> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INL);
> + if (ret < 0)
> + dev_err(&data->client->dev, "Error reading reg_inl\n");
> +
> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INS1);
Missing error handling/message here.
> +
> + return IRQ_HANDLED;
> +}
> +
> static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
> {
> struct kmx61_data *data = private;
> @@ -815,6 +1218,11 @@ static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
> if (data->mag_dready_trig_on)
> iio_trigger_poll(data->mag_dready_trig);
>
> + if (data->motion_trig_on)
> + iio_trigger_poll(data->motion_trig);
> +
> + if (data->ev_enable_state)
> + return IRQ_WAKE_THREAD;
> return IRQ_HANDLED;
> }
>
> @@ -982,7 +1390,7 @@ static int kmx61_probe(struct i2c_client *client,
> if (client->irq >= 0) {
> ret = devm_request_threaded_irq(&client->dev, client->irq,
> kmx61_data_rdy_trig_poll,
> - NULL,
> + kmx61_event_handler,
> IRQF_TRIGGER_RISING,
> KMX61_IRQ_NAME,
> data);
> @@ -1003,6 +1411,14 @@ static int kmx61_probe(struct i2c_client *client,
> goto err_trigger_unregister;
> }
>
> + data->motion_trig =
> + kmx61_trigger_setup(data, data->acc_indio_dev,
> + "any-motion");
> + if (IS_ERR(data->motion_trig)) {
> + ret = PTR_ERR(data->motion_trig);
> + goto err_trigger_unregister;
> + }
> +
> ret = iio_triggered_buffer_setup(data->acc_indio_dev,
> &iio_pollfunc_store_time,
> kmx61_trigger_handler,
> @@ -1060,6 +1476,8 @@ err_trigger_unregister:
> iio_trigger_unregister(data->acc_dready_trig);
> if (data->mag_dready_trig)
> iio_trigger_unregister(data->mag_dready_trig);
> + if (data->motion_trig)
> + iio_trigger_unregister(data->motion_trig);
> err_chip_uninit:
> kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> return ret;
> @@ -1081,6 +1499,7 @@ static int kmx61_remove(struct i2c_client *client)
> iio_triggered_buffer_cleanup(data->mag_indio_dev);
> iio_trigger_unregister(data->acc_dready_trig);
> iio_trigger_unregister(data->mag_dready_trig);
> + iio_trigger_unregister(data->motion_trig);
> }
>
> mutex_lock(&data->lock);
>

2014-12-16 11:08:10

by Daniel Baluta

[permalink] [raw]

Subject: Re: [PATCH v4 6/6] iio: imu: kmx61: Add support for any motion trigger

On Tue, Dec 16, 2014 at 1:19 AM, Hartmut Knaack <[email protected]> wrote:
> Daniel Baluta schrieb am 03.12.2014 um 14:31:
>> We use WUFE (Wake Up from Sleep Engine) and BTSE (Back to Sleep Engine)
>> to detect general motion input.
>>
> A few recommendations, issues and questions inline.
>> Signed-off-by: Daniel Baluta <[email protected]>
>> ---
>> drivers/iio/imu/kmx61.c | 433 +++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 426 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
>> index 0f6816a..972424b 100644
>> --- a/drivers/iio/imu/kmx61.c
>> +++ b/drivers/iio/imu/kmx61.c
>> @@ -20,6 +20,7 @@
>> #include <linux/pm_runtime.h>
>> #include <linux/iio/iio.h>
>> #include <linux/iio/sysfs.h>
>> +#include <linux/iio/events.h>
>> #include <linux/iio/trigger.h>
>> #include <linux/iio/buffer.h>
>> #include <linux/iio/triggered_buffer.h>
>> @@ -30,6 +31,8 @@
>> #define KMX61_IRQ_NAME "kmx61_event"
>>
>> #define KMX61_REG_WHO_AM_I 0x00
>> +#define KMX61_REG_INS1 0x01
>> +#define KMX61_REG_INS2 0x02
>>
>> /*
>> * three 16-bit accelerometer output registers for X/Y/Z axis
>> @@ -63,20 +66,36 @@
>> #define KMX61_REG_INL 0x28
>> #define KMX61_REG_STBY 0x29
>> #define KMX61_REG_CTRL1 0x2A
>> +#define KMX61_REG_CTRL2 0x2B
>> #define KMX61_REG_ODCNTL 0x2C
>> #define KMX61_REG_INC1 0x2D
>>
>> +#define KMX61_REG_WUF_THRESH 0x3D
>> +#define KMX61_REG_WUF_TIMER 0x3E
>> +
>> #define KMX61_ACC_STBY_BIT BIT(0)
>> #define KMX61_MAG_STBY_BIT BIT(1)
>> #define KMX61_ACT_STBY_BIT BIT(7)
>>
>> #define KMX61_ALL_STBY (KMX61_ACC_STBY_BIT | KMX61_MAG_STBY_BIT)
>>
>> +#define KMX61_REG_INS1_BIT_WUFS BIT(1)
>> +
>> +#define KMX61_REG_INS2_BIT_ZP BIT(0)
>> +#define KMX61_REG_INS2_BIT_ZN BIT(1)
>> +#define KMX61_REG_INS2_BIT_YP BIT(2)
>> +#define KMX61_REG_INS2_BIT_YN BIT(3)
>> +#define KMX61_REG_INS2_BIT_XP BIT(4)
>> +#define KMX61_REG_INS2_BIT_XN BIT(5)
>> +
>> #define KMX61_REG_CTRL1_GSEL_MASK 0x03
>>
>> #define KMX61_REG_CTRL1_BIT_RES BIT(4)
>> #define KMX61_REG_CTRL1_BIT_DRDYE BIT(5)
>> +#define KMX61_REG_CTRL1_BIT_WUFE BIT(6)
>> +#define KMX61_REG_CTRL1_BIT_BTSE BIT(7)
>>
>> +#define KMX61_REG_INC1_BIT_WUFS BIT(0)
>> #define KMX61_REG_INC1_BIT_DRDYM BIT(1)
>> #define KMX61_REG_INC1_BIT_DRDYA BIT(2)
>> #define KMX61_REG_INC1_BIT_IEN BIT(5)
>> @@ -86,6 +105,11 @@
>> #define KMX61_ACC_ODR_MASK 0x0F
>> #define KMX61_MAG_ODR_MASK 0xF0
>>
>> +#define KMX61_OWUF_MASK 0x7
>> +
>> +#define KMX61_DEFAULT_WAKE_THRESH 1
>> +#define KMX61_DEFAULT_WAKE_DURATION 1
>> +
>> #define KMX61_SLEEP_DELAY_MS 2000
>>
>> #define KMX61_CHIP_ID 0x12
>> @@ -111,11 +135,16 @@ struct kmx61_data {
>> /* config bits */
>> u8 range;
>> u8 odr_bits;
>> + u8 wake_thresh;
>> + u8 wake_duration;
>>
>> /* accelerometer specific data */
>> struct iio_dev *acc_indio_dev;
>> struct iio_trigger *acc_dready_trig;
>> + struct iio_trigger *motion_trig;
>> bool acc_dready_trig_on;
>> + bool motion_trig_on;
>> + bool ev_enable_state;
>>
>> /* magnetometer specific data */
>> struct iio_dev *mag_indio_dev;
>> @@ -154,6 +183,23 @@ static const struct {
>> {3, 125000, 0x0A},
>> {6, 250000, 0x0B} };
>>
>> +static const struct {
>> + int val;
>> + int val2;
>> + int odr_bits;
>> +} kmx61_wake_up_odr_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} };
> Is it intentional, that values above 50 share the same odr_bits?

Yes, this is as intended. I followed the idea from kxcjk-1013 driver.
Experimenting with ODR of > 50HZ makes no benefit to system c states.

>> +
>> static IIO_CONST_ATTR(accel_scale_available, "0.009582 0.019163 0.038326");
>> static IIO_CONST_ATTR(magn_scale_available, "0.001465");
>> static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>> @@ -179,6 +225,14 @@ static const struct attribute_group kmx61_mag_attribute_group = {
>> .attrs = kmx61_mag_attributes,
>> };
>>
>> +static const struct iio_event_spec kmx61_event = {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_EITHER,
>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> + BIT(IIO_EV_INFO_ENABLE) |
>> + BIT(IIO_EV_INFO_PERIOD),
>> +};
>> +
>> #define KMX61_ACC_CHAN(_axis) { \
>> .type = IIO_ACCEL, \
>> .modified = 1, \
>> @@ -195,6 +249,8 @@ static const struct attribute_group kmx61_mag_attribute_group = {
>> .shift = 4, \
>> .endianness = IIO_LE, \
>> }, \
>> + .event_spec = &kmx61_event, \
>> + .num_event_specs = 1 \
>> }
>>
>> #define KMX61_MAG_CHAN(_axis) { \
>> @@ -250,6 +306,31 @@ static int kmx61_convert_freq_to_bit(int val, int val2)
>> return -EINVAL;
>> }
>>
>> +static int kmx61_convert_bit_to_freq(u8 odr_bits, int *val, int *val2)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
>> + if (odr_bits == kmx61_samp_freq_table[i].odr_bits) {
>> + *val = kmx61_samp_freq_table[i].val;
>> + *val2 = kmx61_samp_freq_table[i].val2;
>> + return 0;
>> + }
>> + return -EINVAL;
> Given that kmx61_samp_freq_table[].odr_bits is practically representing the
> index, it could be handled in the way that we did when accessing
> kmx61_uscale_table. Just check odr_bits to be in range of valid indexes and
> then access the tables' elements with odr_bits used as index.
> Not sure, why I didn't mention it on my first review - maybe I thought you
> would pick it up by yourself if you liked the idea with the _scale_table.

Oh, ok.

>> +}
>> +
>> +
>> +static int kmx61_convert_wake_up_odr_to_bit(int val, int val2)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(kmx61_wake_up_odr_table); ++i)
>> + if (kmx61_wake_up_odr_table[i].val == val &&
>> + kmx61_wake_up_odr_table[i].val2 == val2)
>> + return kmx61_wake_up_odr_table[i].odr_bits;
>> + return -EINVAL;
>> +}
>> +
>> /**
>> * kmx61_set_mode() - set KMX61 device operating mode
>> * @data - kmx61 device private data pointer
>> @@ -338,6 +419,21 @@ static int kmx61_get_mode(struct kmx61_data *data, u8 *mode, u8 device)
>> return 0;
>> }
>>
>> +int kmx61_set_wake_up_odr(struct kmx61_data *data, int val, int val2)
>> +{
>> + int ret, odr_bits;
>> +
>> + odr_bits = kmx61_convert_wake_up_odr_to_bit(val, val2);
>> + if (odr_bits < 0)
>> + return odr_bits;
>> +
>> + ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL2,
>> + odr_bits);
>> + if (ret < 0)
>> + dev_err(&data->client->dev, "Error writing reg_ctrl2\n");
>> + return ret;
>> +}
>> +
>> static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
>> {
>> int ret;
>> @@ -369,6 +465,12 @@ static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
>> if (ret < 0)
>> return ret;
>>
>> + if (device & KMX61_ACC) {
>> + ret = kmx61_set_wake_up_odr(data, val, val2);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> return kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>> }
>>
>> @@ -449,7 +551,7 @@ static int kmx61_set_scale(struct kmx61_data *data, u16 uscale)
>>
>> static int kmx61_chip_init(struct kmx61_data *data)
>> {
>> - int ret;
>> + int ret, val, val2;
>>
>> ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_WHO_AM_I);
>> if (ret < 0) {
>> @@ -476,11 +578,23 @@ static int kmx61_chip_init(struct kmx61_data *data)
>> }
>> data->odr_bits = ret;
>>
>> + /* set output data rate for wake up (motion detection) function */
>> + ret = kmx61_convert_bit_to_freq(data->odr_bits, &val, &val2);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = kmx61_set_wake_up_odr(data, val, val2);
>> + if (ret < 0)
>> + return ret;
>> +
>> /* set acc/magn to OPERATION mode */
>> ret = kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
>> if (ret < 0)
>> return ret;
>>
>> + data->wake_thresh = KMX61_DEFAULT_WAKE_THRESH;
>> + data->wake_duration = KMX61_DEFAULT_WAKE_DURATION;
>> +
>> return 0;
>> }
>>
>> @@ -547,6 +661,87 @@ static int kmx61_setup_new_data_interrupt(struct kmx61_data *data,
>> return 0;
>> }
>>
>> +static int kmx61_chip_update_thresholds(struct kmx61_data *data)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_write_byte_data(data->client,
>> + KMX61_REG_WUF_TIMER,
>> + data->wake_duration);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Errow writing reg_wuf_timer\n");
>> + return ret;
>> + }
>> +
>> + ret = i2c_smbus_write_byte_data(data->client,
>> + KMX61_REG_WUF_THRESH,
>> + data->wake_thresh);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error writing reg_wuf_thresh\n");
>> + return ret;
>> + }
>> +
>> + return 0;
> Like in kmx61_set_wake_up_odr, you could return ret here and drop it in the
> last error handler.

ok.

>> +}
>> +
>> +static int kmx61_setup_any_motion_interrupt(struct kmx61_data *data,
>> + bool status, u8 device)
> device is unused in this function.
>> +{
>> + u8 mode;
>> + int ret;
>> +
>> + ret = kmx61_get_mode(data, &mode, KMX61_ACC | KMX61_MAG);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = kmx61_chip_update_thresholds(data);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INC1);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error reading reg_inc1\n");
>> + return ret;
>> + }
>> + if (status)
>> + ret |= (KMX61_REG_INC1_BIT_IEN | KMX61_REG_INC1_BIT_WUFS);
>> + else
>> + ret &= ~(KMX61_REG_INC1_BIT_IEN | KMX61_REG_INC1_BIT_WUFS);
>> +
>> + ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_INC1, ret);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error writing reg_inc1\n");
>> + return ret;
>> + }
>> +
>> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>> + return ret;
>> + }
>> +
>> + if (status)
>> + ret |= KMX61_REG_CTRL1_BIT_WUFE | KMX61_REG_CTRL1_BIT_BTSE;
>> + else
>> + ret &= ~(KMX61_REG_CTRL1_BIT_WUFE | KMX61_REG_CTRL1_BIT_BTSE);
>> +
>> + ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
>> + return ret;
>> + }
>> + mode |= KMX61_ACT_STBY_BIT;
>> + ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
> Just like in the end of kmx61_set_odr(): return kmx61_set_mode(...);

ok.

>> +}
>> +
>> /**
>> * kmx61_set_power_state() - set power state for kmx61 @device
>> * @data - kmx61 device private pointer
>> @@ -707,12 +902,111 @@ static int kmx61_write_raw(struct iio_dev *indio_dev,
>> }
>> }
>>
>> +static int kmx61_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 kmx61_data *data = kmx61_get_data(indio_dev);
>> +
>> + *val2 = 0;
>> + switch (info) {
>> + case IIO_EV_INFO_VALUE:
>> + *val = data->wake_thresh;
>> + break;
>> + case IIO_EV_INFO_PERIOD:
>> + *val = data->wake_duration;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return IIO_VAL_INT;
> Stylewise, we usually put the return IIO_VAL_INT (or similar) inside each case
> if there is nothing more to be done (like unlocking mutexes, ...). Makes it
> slightly more obvious, what type the data is supposed to be.
>> +}
ok.

>> +
>> +static int kmx61_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)
> Parameters should be indented one more space.
ok.
>> +{
>> + struct kmx61_data *data = kmx61_get_data(indio_dev);
>> +
>> + if (data->ev_enable_state)
>> + return -EBUSY;
>> +
>> + switch (info) {
>> + case IIO_EV_INFO_VALUE:
>> + data->wake_thresh = val;
>> + break;
>> + case IIO_EV_INFO_PERIOD:
>> + data->wake_duration = val;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return IIO_VAL_INT;
> Same here.
ok.
>> +}
>> +
>> +static int kmx61_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 kmx61_data *data = kmx61_get_data(indio_dev);
>> +
>> + return data->ev_enable_state;
>> +}
>> +
>> +static int kmx61_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)
> Parameters should be indented one more space.
ok.
>> +{
>> + struct kmx61_data *data = kmx61_get_data(indio_dev);
>> + int ret;
>> +
>> + if (state && data->ev_enable_state)
>> + return 0;
>> +
>> + mutex_lock(&data->lock);
>> +
>> + if (!state && data->motion_trig_on) {
>> + data->ev_enable_state = 0;
> Assign true/false to bools?
ok.
>> + mutex_unlock(&data->lock);
>> + return 0;
>> + }
>> +
>> + ret = kmx61_set_power_state(data, state, KMX61_ACC);
>> + if (ret < 0) {
>> + mutex_unlock(&data->lock);
>> + return ret;
>> + }
>> +
>> + ret = kmx61_setup_any_motion_interrupt(data, state, KMX61_ACC);
>> + if (ret < 0) {
>> + kmx61_set_power_state(data, false, KMX61_ACC);
>> + mutex_unlock(&data->lock);
>> + return ret;
>> + }
>> +
>> + data->ev_enable_state = state;
> You could place an error label here and return ret below to reduce some code
> repetition in the error handlers above.
>> + mutex_unlock(&data->lock);
>> +
>> + return 0;
>> +}
>> +
>> static int kmx61_acc_validate_trigger(struct iio_dev *indio_dev,
>> struct iio_trigger *trig)
>> {
>> struct kmx61_data *data = kmx61_get_data(indio_dev);
>>
>> - if (data->acc_dready_trig != trig)
>> + if (data->acc_dready_trig != trig && data->motion_trig != trig)
>> return -EINVAL;
>>
>> return 0;
>> @@ -734,6 +1028,10 @@ static const struct iio_info kmx61_acc_info = {
>> .read_raw = kmx61_read_raw,
>> .write_raw = kmx61_write_raw,
>> .attrs = &kmx61_acc_attribute_group,
>> + .read_event_value = kmx61_read_event,
>> + .write_event_value = kmx61_write_event,
>> + .read_event_config = kmx61_read_event_config,
>> + .write_event_config = kmx61_write_event_config,
>> .validate_trigger = kmx61_acc_validate_trigger,
>> };
>>
>> @@ -753,11 +1051,18 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
>> u8 device;
>>
>> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
>> - struct kmx61_data *data = iio_priv(indio_dev);
>> + struct kmx61_data *data = kmx61_get_data(indio_dev);
> Oh well, fixing the issue I pointed out in your previous patch. LOL and ouch!
:)). That explains why the driver didn't crash when I tested it.

>>
>> mutex_lock(&data->lock);
>>
>> - if (data->acc_dready_trig == trig)
>> + if (!state && data->ev_enable_state && data->motion_trig_on) {
>> + data->motion_trig_on = false;
>> + mutex_unlock(&data->lock);
>> + return 0;
>> + }
>> +
>> +
>> + if (data->acc_dready_trig == trig || data->motion_trig)
> Wouldn't you want to compare data->motion_trig with trig?
This is a nice catch.

>> device = KMX61_ACC;
>> else
>> device = KMX61_MAG;
>> @@ -768,7 +1073,10 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
>> return ret;
>> }
>>
>> - ret = kmx61_setup_new_data_interrupt(data, state, device);
>> + if (data->acc_dready_trig == trig || data->mag_dready_trig == trig)
>> + ret = kmx61_setup_new_data_interrupt(data, state, device);
>> + else
>> + ret = kmx61_setup_any_motion_interrupt(data, state, KMX61_ACC);
>> if (ret < 0) {
>> kmx61_set_power_state(data, false, device);
>> mutex_unlock(&data->lock);
>> @@ -777,8 +1085,10 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>
>> if (data->acc_dready_trig == trig)
>> data->acc_dready_trig_on = state;
>> - else
>> + else if (data->mag_dready_trig == trig)
>> data->mag_dready_trig_on = state;
>> + else
>> + data->motion_trig_on = state;
>>
>> mutex_unlock(&data->lock);
>>
>> @@ -806,6 +1116,99 @@ static const struct iio_trigger_ops kmx61_trigger_ops = {
>> .owner = THIS_MODULE,
>> };
>>
>> +static irqreturn_t kmx61_event_handler(int irq, void *private)
>> +{
>> + struct kmx61_data *data = private;
>> + struct iio_dev *indio_dev = data->acc_indio_dev;
>> + int ret;
>> +
>> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INS1);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error reading reg_ins1\n");
>> + goto ack_intr;
>> + }
>> +
>> + if (ret & KMX61_REG_INS1_BIT_WUFS) {
>> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INS2);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error reading reg_ins2\n");
>> + goto ack_intr;
>> + }
>> +
>> + if (ret & KMX61_REG_INS2_BIT_XN)
>> + iio_push_event(indio_dev,
>> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> + 0,
>> + IIO_MOD_X,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_FALLING),
>> + 0);
>> +
>> + if (ret & KMX61_REG_INS2_BIT_XP)
>> + iio_push_event(indio_dev,
>> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> + 0,
>> + IIO_MOD_X,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_RISING),
>> + 0);
>> +
>> + if (ret & KMX61_REG_INS2_BIT_YN)
>> + iio_push_event(indio_dev,
>> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> + 0,
>> + IIO_MOD_Y,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_FALLING),
>> + 0);
>> +
>> + if (ret & KMX61_REG_INS2_BIT_YP)
>> + iio_push_event(indio_dev,
>> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> + 0,
>> + IIO_MOD_Y,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_RISING),
>> + 0);
>> +
>> + if (ret & KMX61_REG_INS2_BIT_ZN)
>> + iio_push_event(indio_dev,
>> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> + 0,
>> + IIO_MOD_Z,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_FALLING),
>> + 0);
>> +
>> + if (ret & KMX61_REG_INS2_BIT_ZP)
>> + iio_push_event(indio_dev,
>> + IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> + 0,
>> + IIO_MOD_Z,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_RISING),
>> + 0);
>> + }
>> +
>> +ack_intr:
>> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
>> + if (ret < 0)
>> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>> +
>> + ret |= KMX61_REG_CTRL1_BIT_RES;
>> + ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
>> + if (ret < 0)
>> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> Typo: Error writing ...
ok.
>> +
>> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INL);
>> + if (ret < 0)
>> + dev_err(&data->client->dev, "Error reading reg_inl\n");
>> +
>> + ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INS1);
> Missing error handling/message here.
ok.
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
>> {
>> struct kmx61_data *data = private;
>> @@ -815,6 +1218,11 @@ static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
>> if (data->mag_dready_trig_on)
>> iio_trigger_poll(data->mag_dready_trig);
>>
>> + if (data->motion_trig_on)
>> + iio_trigger_poll(data->motion_trig);
>> +
>> + if (data->ev_enable_state)
>> + return IRQ_WAKE_THREAD;
>> return IRQ_HANDLED;
>> }
>>
>> @@ -982,7 +1390,7 @@ static int kmx61_probe(struct i2c_client *client,
>> if (client->irq >= 0) {
>> ret = devm_request_threaded_irq(&client->dev, client->irq,
>> kmx61_data_rdy_trig_poll,
>> - NULL,
>> + kmx61_event_handler,
>> IRQF_TRIGGER_RISING,
>> KMX61_IRQ_NAME,
>> data);
>> @@ -1003,6 +1411,14 @@ static int kmx61_probe(struct i2c_client *client,
>> goto err_trigger_unregister;
>> }
>>
>> + data->motion_trig =
>> + kmx61_trigger_setup(data, data->acc_indio_dev,
>> + "any-motion");
>> + if (IS_ERR(data->motion_trig)) {
>> + ret = PTR_ERR(data->motion_trig);
>> + goto err_trigger_unregister;
>> + }
>> +
>> ret = iio_triggered_buffer_setup(data->acc_indio_dev,
>> &iio_pollfunc_store_time,
>> kmx61_trigger_handler,
>> @@ -1060,6 +1476,8 @@ err_trigger_unregister:
>> iio_trigger_unregister(data->acc_dready_trig);
>> if (data->mag_dready_trig)
>> iio_trigger_unregister(data->mag_dready_trig);
>> + if (data->motion_trig)
>> + iio_trigger_unregister(data->motion_trig);
>> err_chip_uninit:
>> kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>> return ret;
>> @@ -1081,6 +1499,7 @@ static int kmx61_remove(struct i2c_client *client)
>> iio_triggered_buffer_cleanup(data->mag_indio_dev);
>> iio_trigger_unregister(data->acc_dready_trig);
>> iio_trigger_unregister(data->mag_dready_trig);
>> + iio_trigger_unregister(data->motion_trig);
>> }
>>
>> mutex_lock(&data->lock);

Thanks a lot for the feedback, Hartmut!

Daniel.