* [PATCH v4] iio: accel: mma8452: improvements to handle multiple events
@ 2017-08-20 16:06 Harinath Nampally
2017-08-21 8:47 ` Martin Kepplinger
0 siblings, 1 reply; 5+ messages in thread
From: Harinath Nampally @ 2017-08-20 16:06 UTC (permalink / raw)
To: jic23
Cc: knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel,
amsfield22, martink
This driver supports multiple devices like mma8653,
mma8652, mma8452, mma8453 and fxls8471. Almost all
these devices have more than one event.
Current driver design hardcodes the event specific
information, so only one event can be supported by this
driver at any given time.
Also current design doesn't have the flexibility to
add more events.
This patch improves by detaching the event related
information from chip_info struct,and based on channel
type and event direction the corresponding event
configuration registers are picked dynamically.
Hence both transient and freefall events can be
handled in read/write callbacks.
Changes are thoroughly tested on fxls8471 device on imx6UL
Eval board using iio_event_monitor user space program.
After this fix both Freefall and Transient events are
handled by the driver without any conflicts.
Changes since v3 -> v4
-Add 'const struct ev_regs_accel_falling'
-Add 'const struct ev_regs_accel_rising'
-Refactor mma8452_get_event_regs function to
remove the fill in the struct and return above structs
-Condense the commit's subject message
Changes since v2 -> v3
-Fix typo in commit message
-Replace word 'Bugfix' with 'Improvements'
-Describe more accurate commit message
-Replace breaks with returns
-Initialise transient event threshold mask
-Remove unrelated change of IIO_ACCEL channel type
check in read/write event callbacks
Changes since v1 -> v2
-Fix indentations
-Remove unused fields in mma8452_event_regs struct
-Remove redundant return statement
-Remove unrelated changes like checkpatch.pl warning fixes
Signed-off-by: Harinath Nampally <harinath922@gmail.com>
---
drivers/iio/accel/mma8452.c | 277 ++++++++++++++++++++------------------------
1 file changed, 123 insertions(+), 154 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index eb6e3dc..7bfc257 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -59,7 +59,9 @@
#define MMA8452_FF_MT_THS 0x17
#define MMA8452_FF_MT_THS_MASK 0x7f
#define MMA8452_FF_MT_COUNT 0x18
+#define MMA8452_FF_MT_CHAN_SHIFT 3
#define MMA8452_TRANSIENT_CFG 0x1d
+#define MMA8452_TRANSIENT_CFG_CHAN(chan) BIT(chan + 1)
#define MMA8452_TRANSIENT_CFG_HPF_BYP BIT(0)
#define MMA8452_TRANSIENT_CFG_ELE BIT(4)
#define MMA8452_TRANSIENT_SRC 0x1e
@@ -69,6 +71,7 @@
#define MMA8452_TRANSIENT_THS 0x1f
#define MMA8452_TRANSIENT_THS_MASK GENMASK(6, 0)
#define MMA8452_TRANSIENT_COUNT 0x20
+#define MMA8452_TRANSIENT_CHAN_SHIFT 1
#define MMA8452_CTRL_REG1 0x2a
#define MMA8452_CTRL_ACTIVE BIT(0)
#define MMA8452_CTRL_DR_MASK GENMASK(5, 3)
@@ -107,6 +110,42 @@ struct mma8452_data {
const struct mma_chip_info *chip_info;
};
+ /**
+ * struct mma8452_event_regs - chip specific data related to events
+ * @ev_cfg: event config register address
+ * @ev_src: event source register address
+ * @ev_ths: event threshold register address
+ * @ev_ths_mask: mask for the threshold value
+ * @ev_count: event count (period) register address
+ *
+ * Since not all chips supported by the driver support comparing high pass
+ * filtered data for events (interrupts), different interrupt sources are
+ * used for different chips and the relevant registers are included here.
+ */
+struct mma8452_event_regs {
+ u8 ev_cfg;
+ u8 ev_src;
+ u8 ev_ths;
+ u8 ev_ths_mask;
+ u8 ev_count;
+};
+
+static const struct mma8452_event_regs ev_regs_accel_falling = {
+ .ev_cfg = MMA8452_FF_MT_CFG,
+ .ev_src = MMA8452_FF_MT_SRC,
+ .ev_ths = MMA8452_FF_MT_THS,
+ .ev_ths_mask = MMA8452_FF_MT_THS_MASK,
+ .ev_count = MMA8452_FF_MT_COUNT
+};
+
+static const struct mma8452_event_regs ev_regs_accel_rising = {
+ .ev_cfg = MMA8452_TRANSIENT_CFG,
+ .ev_src = MMA8452_TRANSIENT_SRC,
+ .ev_ths = MMA8452_TRANSIENT_THS,
+ .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
+ .ev_count = MMA8452_TRANSIENT_COUNT,
+};
+
/**
* struct mma_chip_info - chip specific data
* @chip_id: WHO_AM_I register's value
@@ -116,40 +155,12 @@ struct mma8452_data {
* @mma_scales: scale factors for converting register values
* to m/s^2; 3 modes: 2g, 4g, 8g; 2 integers
* per mode: m/s^2 and micro m/s^2
- * @ev_cfg: event config register address
- * @ev_cfg_ele: latch bit in event config register
- * @ev_cfg_chan_shift: number of the bit to enable events in X
- * direction; in event config register
- * @ev_src: event source register address
- * @ev_src_xe: bit in event source register that indicates
- * an event in X direction
- * @ev_src_ye: bit in event source register that indicates
- * an event in Y direction
- * @ev_src_ze: bit in event source register that indicates
- * an event in Z direction
- * @ev_ths: event threshold register address
- * @ev_ths_mask: mask for the threshold value
- * @ev_count: event count (period) register address
- *
- * Since not all chips supported by the driver support comparing high pass
- * filtered data for events (interrupts), different interrupt sources are
- * used for different chips and the relevant registers are included here.
*/
struct mma_chip_info {
u8 chip_id;
const struct iio_chan_spec *channels;
int num_channels;
const int mma_scales[3][2];
- u8 ev_cfg;
- u8 ev_cfg_ele;
- u8 ev_cfg_chan_shift;
- u8 ev_src;
- u8 ev_src_xe;
- u8 ev_src_ye;
- u8 ev_src_ze;
- u8 ev_ths;
- u8 ev_ths_mask;
- u8 ev_count;
};
enum {
@@ -602,9 +613,8 @@ static int mma8452_set_power_mode(struct mma8452_data *data, u8 mode)
static int mma8452_freefall_mode_enabled(struct mma8452_data *data)
{
int val;
- const struct mma_chip_info *chip = data->chip_info;
- val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
+ val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
if (val < 0)
return val;
@@ -614,29 +624,28 @@ static int mma8452_freefall_mode_enabled(struct mma8452_data *data)
static int mma8452_set_freefall_mode(struct mma8452_data *data, bool state)
{
int val;
- const struct mma_chip_info *chip = data->chip_info;
if ((state && mma8452_freefall_mode_enabled(data)) ||
(!state && !(mma8452_freefall_mode_enabled(data))))
return 0;
- val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
+ val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
if (val < 0)
return val;
if (state) {
- val |= BIT(idx_x + chip->ev_cfg_chan_shift);
- val |= BIT(idx_y + chip->ev_cfg_chan_shift);
- val |= BIT(idx_z + chip->ev_cfg_chan_shift);
+ val |= BIT(idx_x + MMA8452_FF_MT_CHAN_SHIFT);
+ val |= BIT(idx_y + MMA8452_FF_MT_CHAN_SHIFT);
+ val |= BIT(idx_z + MMA8452_FF_MT_CHAN_SHIFT);
val &= ~MMA8452_FF_MT_CFG_OAE;
} else {
- val &= ~BIT(idx_x + chip->ev_cfg_chan_shift);
- val &= ~BIT(idx_y + chip->ev_cfg_chan_shift);
- val &= ~BIT(idx_z + chip->ev_cfg_chan_shift);
+ val &= ~BIT(idx_x + MMA8452_FF_MT_CHAN_SHIFT);
+ val &= ~BIT(idx_y + MMA8452_FF_MT_CHAN_SHIFT);
+ val &= ~BIT(idx_z + MMA8452_FF_MT_CHAN_SHIFT);
val |= MMA8452_FF_MT_CFG_OAE;
}
- return mma8452_change_config(data, chip->ev_cfg, val);
+ return mma8452_change_config(data, MMA8452_FF_MT_CFG, val);
}
static int mma8452_set_hp_filter_frequency(struct mma8452_data *data,
@@ -740,6 +749,28 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
return ret;
}
+static int mma8452_get_event_regs(const struct iio_chan_spec *chan,
+ enum iio_event_direction dir, const struct mma8452_event_regs **ev_reg)
+{
+ if (!chan)
+ return -EINVAL;
+ switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ *ev_reg = &ev_regs_accel_falling;
+ return 0;
+ case IIO_EV_DIR_RISING:
+ *ev_reg = &ev_regs_accel_rising;
+ return 0;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
static int mma8452_read_thresh(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan,
enum iio_event_type type,
@@ -749,21 +780,23 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, us, power_mode;
+ const struct mma8452_event_regs *ev_regs;
+ ret = mma8452_get_event_regs(chan, dir, &ev_regs);
+ if (ret)
+ return ret;
switch (info) {
case IIO_EV_INFO_VALUE:
- ret = i2c_smbus_read_byte_data(data->client,
- data->chip_info->ev_ths);
+ ret = i2c_smbus_read_byte_data(data->client, ev_regs->ev_ths);
if (ret < 0)
return ret;
- *val = ret & data->chip_info->ev_ths_mask;
+ *val = ret & ev_regs->ev_ths_mask;
return IIO_VAL_INT;
case IIO_EV_INFO_PERIOD:
- ret = i2c_smbus_read_byte_data(data->client,
- data->chip_info->ev_count);
+ ret = i2c_smbus_read_byte_data(data->client, ev_regs->ev_count);
if (ret < 0)
return ret;
@@ -809,14 +842,18 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, reg, steps;
+ const struct mma8452_event_regs *ev_regs;
+
+ ret = mma8452_get_event_regs(chan, dir, &ev_regs);
+ if (ret)
+ return ret;
switch (info) {
case IIO_EV_INFO_VALUE:
- if (val < 0 || val > MMA8452_TRANSIENT_THS_MASK)
+ if (val < 0 || val > ev_regs->ev_ths_mask)
return -EINVAL;
- return mma8452_change_config(data, data->chip_info->ev_ths,
- val);
+ return mma8452_change_config(data, ev_regs->ev_ths, val);
case IIO_EV_INFO_PERIOD:
ret = mma8452_get_power_mode(data);
@@ -830,8 +867,7 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
if (steps < 0 || steps > 0xff)
return -EINVAL;
- return mma8452_change_config(data, data->chip_info->ev_count,
- steps);
+ return mma8452_change_config(data, ev_regs->ev_count, steps);
case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
reg = i2c_smbus_read_byte_data(data->client,
@@ -861,23 +897,18 @@ static int mma8452_read_event_config(struct iio_dev *indio_dev,
enum iio_event_direction dir)
{
struct mma8452_data *data = iio_priv(indio_dev);
- const struct mma_chip_info *chip = data->chip_info;
int ret;
switch (dir) {
case IIO_EV_DIR_FALLING:
return mma8452_freefall_mode_enabled(data);
case IIO_EV_DIR_RISING:
- if (mma8452_freefall_mode_enabled(data))
- return 0;
-
- ret = i2c_smbus_read_byte_data(data->client,
- data->chip_info->ev_cfg);
+ ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
if (ret < 0)
return ret;
- return !!(ret & BIT(chan->scan_index +
- chip->ev_cfg_chan_shift));
+ return !!(ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index));
+
default:
return -EINVAL;
}
@@ -890,7 +921,6 @@ static int mma8452_write_event_config(struct iio_dev *indio_dev,
int state)
{
struct mma8452_data *data = iio_priv(indio_dev);
- const struct mma_chip_info *chip = data->chip_info;
int val, ret;
ret = mma8452_set_runtime_pm_state(data->client, state);
@@ -901,28 +931,18 @@ static int mma8452_write_event_config(struct iio_dev *indio_dev,
case IIO_EV_DIR_FALLING:
return mma8452_set_freefall_mode(data, state);
case IIO_EV_DIR_RISING:
- val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
+ val = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
if (val < 0)
return val;
- if (state) {
- if (mma8452_freefall_mode_enabled(data)) {
- val &= ~BIT(idx_x + chip->ev_cfg_chan_shift);
- val &= ~BIT(idx_y + chip->ev_cfg_chan_shift);
- val &= ~BIT(idx_z + chip->ev_cfg_chan_shift);
- val |= MMA8452_FF_MT_CFG_OAE;
- }
- val |= BIT(chan->scan_index + chip->ev_cfg_chan_shift);
- } else {
- if (mma8452_freefall_mode_enabled(data))
- return 0;
-
- val &= ~BIT(chan->scan_index + chip->ev_cfg_chan_shift);
- }
+ if (state)
+ val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
+ else
+ val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
- val |= chip->ev_cfg_ele;
+ val |= MMA8452_TRANSIENT_CFG_ELE;
- return mma8452_change_config(data, chip->ev_cfg, val);
+ return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
default:
return -EINVAL;
}
@@ -934,35 +954,25 @@ static void mma8452_transient_interrupt(struct iio_dev *indio_dev)
s64 ts = iio_get_time_ns(indio_dev);
int src;
- src = i2c_smbus_read_byte_data(data->client, data->chip_info->ev_src);
+ src = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_SRC);
if (src < 0)
return;
- if (mma8452_freefall_mode_enabled(data)) {
- iio_push_event(indio_dev,
- IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
- IIO_MOD_X_AND_Y_AND_Z,
- IIO_EV_TYPE_MAG,
- IIO_EV_DIR_FALLING),
- ts);
- return;
- }
-
- if (src & data->chip_info->ev_src_xe)
+ if (src & MMA8452_TRANSIENT_SRC_XTRANSE)
iio_push_event(indio_dev,
IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X,
IIO_EV_TYPE_MAG,
IIO_EV_DIR_RISING),
ts);
- if (src & data->chip_info->ev_src_ye)
+ if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
iio_push_event(indio_dev,
IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Y,
IIO_EV_TYPE_MAG,
IIO_EV_DIR_RISING),
ts);
- if (src & data->chip_info->ev_src_ze)
+ if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
iio_push_event(indio_dev,
IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Z,
IIO_EV_TYPE_MAG,
@@ -974,23 +984,42 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
{
struct iio_dev *indio_dev = p;
struct mma8452_data *data = iio_priv(indio_dev);
- const struct mma_chip_info *chip = data->chip_info;
int ret = IRQ_NONE;
int src;
+ int enabled_interrupts;
src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
if (src < 0)
return IRQ_NONE;
+ enabled_interrupts = i2c_smbus_read_byte_data(data->client,
+ MMA8452_CTRL_REG4);
+ if (enabled_interrupts < 0)
+ return enabled_interrupts;
+
+ if (!(src & enabled_interrupts))
+ return IRQ_NONE;
+
if (src & MMA8452_INT_DRDY) {
iio_trigger_poll_chained(indio_dev->trig);
ret = IRQ_HANDLED;
}
- if ((src & MMA8452_INT_TRANS &&
- chip->ev_src == MMA8452_TRANSIENT_SRC) ||
- (src & MMA8452_INT_FF_MT &&
- chip->ev_src == MMA8452_FF_MT_SRC)) {
+ if (src & MMA8452_INT_FF_MT) {
+ if (mma8452_freefall_mode_enabled(data)) {
+ s64 ts = iio_get_time_ns(indio_dev);
+
+ iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_AND_Y_AND_Z,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_FALLING),
+ ts);
+ }
+ ret = IRQ_HANDLED;
+ }
+
+ if (src & MMA8452_INT_TRANS) {
mma8452_transient_interrupt(indio_dev);
ret = IRQ_HANDLED;
}
@@ -1222,96 +1251,36 @@ static const struct mma_chip_info mma_chip_info_table[] = {
* g * N * 1000000 / 2048 for N = 2, 4, 8 and g=9.80665
*/
.mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
- .ev_cfg = MMA8452_TRANSIENT_CFG,
- .ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
- .ev_cfg_chan_shift = 1,
- .ev_src = MMA8452_TRANSIENT_SRC,
- .ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
- .ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
- .ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
- .ev_ths = MMA8452_TRANSIENT_THS,
- .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
- .ev_count = MMA8452_TRANSIENT_COUNT,
},
[mma8452] = {
.chip_id = MMA8452_DEVICE_ID,
.channels = mma8452_channels,
.num_channels = ARRAY_SIZE(mma8452_channels),
.mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
- .ev_cfg = MMA8452_TRANSIENT_CFG,
- .ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
- .ev_cfg_chan_shift = 1,
- .ev_src = MMA8452_TRANSIENT_SRC,
- .ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
- .ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
- .ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
- .ev_ths = MMA8452_TRANSIENT_THS,
- .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
- .ev_count = MMA8452_TRANSIENT_COUNT,
},
[mma8453] = {
.chip_id = MMA8453_DEVICE_ID,
.channels = mma8453_channels,
.num_channels = ARRAY_SIZE(mma8453_channels),
.mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
- .ev_cfg = MMA8452_TRANSIENT_CFG,
- .ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
- .ev_cfg_chan_shift = 1,
- .ev_src = MMA8452_TRANSIENT_SRC,
- .ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
- .ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
- .ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
- .ev_ths = MMA8452_TRANSIENT_THS,
- .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
- .ev_count = MMA8452_TRANSIENT_COUNT,
},
[mma8652] = {
.chip_id = MMA8652_DEVICE_ID,
.channels = mma8652_channels,
.num_channels = ARRAY_SIZE(mma8652_channels),
.mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
- .ev_cfg = MMA8452_FF_MT_CFG,
- .ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
- .ev_cfg_chan_shift = 3,
- .ev_src = MMA8452_FF_MT_SRC,
- .ev_src_xe = MMA8452_FF_MT_SRC_XHE,
- .ev_src_ye = MMA8452_FF_MT_SRC_YHE,
- .ev_src_ze = MMA8452_FF_MT_SRC_ZHE,
- .ev_ths = MMA8452_FF_MT_THS,
- .ev_ths_mask = MMA8452_FF_MT_THS_MASK,
- .ev_count = MMA8452_FF_MT_COUNT,
},
[mma8653] = {
.chip_id = MMA8653_DEVICE_ID,
.channels = mma8653_channels,
.num_channels = ARRAY_SIZE(mma8653_channels),
.mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
- .ev_cfg = MMA8452_FF_MT_CFG,
- .ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
- .ev_cfg_chan_shift = 3,
- .ev_src = MMA8452_FF_MT_SRC,
- .ev_src_xe = MMA8452_FF_MT_SRC_XHE,
- .ev_src_ye = MMA8452_FF_MT_SRC_YHE,
- .ev_src_ze = MMA8452_FF_MT_SRC_ZHE,
- .ev_ths = MMA8452_FF_MT_THS,
- .ev_ths_mask = MMA8452_FF_MT_THS_MASK,
- .ev_count = MMA8452_FF_MT_COUNT,
},
[fxls8471] = {
.chip_id = FXLS8471_DEVICE_ID,
.channels = mma8451_channels,
.num_channels = ARRAY_SIZE(mma8451_channels),
.mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
- .ev_cfg = MMA8452_TRANSIENT_CFG,
- .ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
- .ev_cfg_chan_shift = 1,
- .ev_src = MMA8452_TRANSIENT_SRC,
- .ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
- .ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
- .ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
- .ev_ths = MMA8452_TRANSIENT_THS,
- .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
- .ev_count = MMA8452_TRANSIENT_COUNT,
},
};
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4] iio: accel: mma8452: improvements to handle multiple events
2017-08-20 16:06 [PATCH v4] iio: accel: mma8452: improvements to handle multiple events Harinath Nampally
@ 2017-08-21 8:47 ` Martin Kepplinger
2017-08-23 0:29 ` Harinath Nampally
0 siblings, 1 reply; 5+ messages in thread
From: Martin Kepplinger @ 2017-08-21 8:47 UTC (permalink / raw)
To: Harinath Nampally
Cc: jic23, knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel,
amsfield22, linux-iio-owner
Thanks. I may have missed something, but I'm concerned about only one
thing:
devices without transient event registers. See my comments below.
Am 20.08.2017 18:06 schrieb Harinath Nampally:
> This driver supports multiple devices like mma8653,
> mma8652, mma8452, mma8453 and fxls8471. Almost all
> these devices have more than one event.
>
> Current driver design hardcodes the event specific
> information, so only one event can be supported by this
> driver at any given time.
> Also current design doesn't have the flexibility to
> add more events.
>
> This patch improves by detaching the event related
> information from chip_info struct,and based on channel
> type and event direction the corresponding event
> configuration registers are picked dynamically.
> Hence both transient and freefall events can be
> handled in read/write callbacks.
>
> Changes are thoroughly tested on fxls8471 device on imx6UL
> Eval board using iio_event_monitor user space program.
>
> After this fix both Freefall and Transient events are
> handled by the driver without any conflicts.
>
> Changes since v3 -> v4
> -Add 'const struct ev_regs_accel_falling'
> -Add 'const struct ev_regs_accel_rising'
> -Refactor mma8452_get_event_regs function to
> remove the fill in the struct and return above structs
> -Condense the commit's subject message
>
> Changes since v2 -> v3
> -Fix typo in commit message
> -Replace word 'Bugfix' with 'Improvements'
> -Describe more accurate commit message
> -Replace breaks with returns
> -Initialise transient event threshold mask
> -Remove unrelated change of IIO_ACCEL channel type
> check in read/write event callbacks
>
> Changes since v1 -> v2
> -Fix indentations
> -Remove unused fields in mma8452_event_regs struct
> -Remove redundant return statement
> -Remove unrelated changes like checkpatch.pl warning fixes
>
> Signed-off-by: Harinath Nampally <harinath922@gmail.com>
> ---
> drivers/iio/accel/mma8452.c | 277
> ++++++++++++++++++++------------------------
> 1 file changed, 123 insertions(+), 154 deletions(-)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index eb6e3dc..7bfc257 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -59,7 +59,9 @@
> #define MMA8452_FF_MT_THS 0x17
> #define MMA8452_FF_MT_THS_MASK 0x7f
> #define MMA8452_FF_MT_COUNT 0x18
> +#define MMA8452_FF_MT_CHAN_SHIFT 3
> #define MMA8452_TRANSIENT_CFG 0x1d
> +#define MMA8452_TRANSIENT_CFG_CHAN(chan) BIT(chan + 1)
> #define MMA8452_TRANSIENT_CFG_HPF_BYP BIT(0)
> #define MMA8452_TRANSIENT_CFG_ELE BIT(4)
> #define MMA8452_TRANSIENT_SRC 0x1e
> @@ -69,6 +71,7 @@
> #define MMA8452_TRANSIENT_THS 0x1f
> #define MMA8452_TRANSIENT_THS_MASK GENMASK(6, 0)
> #define MMA8452_TRANSIENT_COUNT 0x20
> +#define MMA8452_TRANSIENT_CHAN_SHIFT 1
> #define MMA8452_CTRL_REG1 0x2a
> #define MMA8452_CTRL_ACTIVE BIT(0)
> #define MMA8452_CTRL_DR_MASK GENMASK(5, 3)
> @@ -107,6 +110,42 @@ struct mma8452_data {
> const struct mma_chip_info *chip_info;
> };
>
> + /**
> + * struct mma8452_event_regs - chip specific data related to events
> + * @ev_cfg: event config register address
> + * @ev_src: event source register address
> + * @ev_ths: event threshold register address
> + * @ev_ths_mask: mask for the threshold value
> + * @ev_count: event count (period) register address
> + *
> + * Since not all chips supported by the driver support comparing high
> pass
> + * filtered data for events (interrupts), different interrupt sources
> are
> + * used for different chips and the relevant registers are included
> here.
> + */
> +struct mma8452_event_regs {
> + u8 ev_cfg;
> + u8 ev_src;
> + u8 ev_ths;
> + u8 ev_ths_mask;
> + u8 ev_count;
> +};
> +
> +static const struct mma8452_event_regs ev_regs_accel_falling = {
> + .ev_cfg = MMA8452_FF_MT_CFG,
> + .ev_src = MMA8452_FF_MT_SRC,
> + .ev_ths = MMA8452_FF_MT_THS,
> + .ev_ths_mask = MMA8452_FF_MT_THS_MASK,
> + .ev_count = MMA8452_FF_MT_COUNT
> +};
> +
> +static const struct mma8452_event_regs ev_regs_accel_rising = {
> + .ev_cfg = MMA8452_TRANSIENT_CFG,
> + .ev_src = MMA8452_TRANSIENT_SRC,
> + .ev_ths = MMA8452_TRANSIENT_THS,
> + .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
> + .ev_count = MMA8452_TRANSIENT_COUNT,
> +};
For devices without transient event registers, this cannot work. See
below.
> +
> /**
> * struct mma_chip_info - chip specific data
> * @chip_id: WHO_AM_I register's value
> @@ -116,40 +155,12 @@ struct mma8452_data {
> * @mma_scales: scale factors for converting register values
> * to m/s^2; 3 modes: 2g, 4g, 8g; 2 integers
> * per mode: m/s^2 and micro m/s^2
> - * @ev_cfg: event config register address
> - * @ev_cfg_ele: latch bit in event config register
> - * @ev_cfg_chan_shift: number of the bit to enable events in X
> - * direction; in event config register
> - * @ev_src: event source register address
> - * @ev_src_xe: bit in event source register that indicates
> - * an event in X direction
> - * @ev_src_ye: bit in event source register that indicates
> - * an event in Y direction
> - * @ev_src_ze: bit in event source register that indicates
> - * an event in Z direction
> - * @ev_ths: event threshold register address
> - * @ev_ths_mask: mask for the threshold value
> - * @ev_count: event count (period) register address
> - *
> - * Since not all chips supported by the driver support comparing high
> pass
> - * filtered data for events (interrupts), different interrupt sources
> are
> - * used for different chips and the relevant registers are included
> here.
> */
> struct mma_chip_info {
> u8 chip_id;
> const struct iio_chan_spec *channels;
> int num_channels;
> const int mma_scales[3][2];
> - u8 ev_cfg;
> - u8 ev_cfg_ele;
> - u8 ev_cfg_chan_shift;
> - u8 ev_src;
> - u8 ev_src_xe;
> - u8 ev_src_ye;
> - u8 ev_src_ze;
> - u8 ev_ths;
> - u8 ev_ths_mask;
> - u8 ev_count;
> };
>
> enum {
> @@ -602,9 +613,8 @@ static int mma8452_set_power_mode(struct
> mma8452_data *data, u8 mode)
> static int mma8452_freefall_mode_enabled(struct mma8452_data *data)
> {
> int val;
> - const struct mma_chip_info *chip = data->chip_info;
>
> - val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
> + val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
> if (val < 0)
> return val;
>
> @@ -614,29 +624,28 @@ static int mma8452_freefall_mode_enabled(struct
> mma8452_data *data)
> static int mma8452_set_freefall_mode(struct mma8452_data *data, bool
> state)
> {
> int val;
> - const struct mma_chip_info *chip = data->chip_info;
>
> if ((state && mma8452_freefall_mode_enabled(data)) ||
> (!state && !(mma8452_freefall_mode_enabled(data))))
> return 0;
>
> - val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
> + val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
> if (val < 0)
> return val;
>
> if (state) {
> - val |= BIT(idx_x + chip->ev_cfg_chan_shift);
> - val |= BIT(idx_y + chip->ev_cfg_chan_shift);
> - val |= BIT(idx_z + chip->ev_cfg_chan_shift);
> + val |= BIT(idx_x + MMA8452_FF_MT_CHAN_SHIFT);
> + val |= BIT(idx_y + MMA8452_FF_MT_CHAN_SHIFT);
> + val |= BIT(idx_z + MMA8452_FF_MT_CHAN_SHIFT);
> val &= ~MMA8452_FF_MT_CFG_OAE;
> } else {
> - val &= ~BIT(idx_x + chip->ev_cfg_chan_shift);
> - val &= ~BIT(idx_y + chip->ev_cfg_chan_shift);
> - val &= ~BIT(idx_z + chip->ev_cfg_chan_shift);
> + val &= ~BIT(idx_x + MMA8452_FF_MT_CHAN_SHIFT);
> + val &= ~BIT(idx_y + MMA8452_FF_MT_CHAN_SHIFT);
> + val &= ~BIT(idx_z + MMA8452_FF_MT_CHAN_SHIFT);
> val |= MMA8452_FF_MT_CFG_OAE;
> }
>
> - return mma8452_change_config(data, chip->ev_cfg, val);
> + return mma8452_change_config(data, MMA8452_FF_MT_CFG, val);
> }
>
> static int mma8452_set_hp_filter_frequency(struct mma8452_data *data,
> @@ -740,6 +749,28 @@ static int mma8452_write_raw(struct iio_dev
> *indio_dev,
> return ret;
> }
>
> +static int mma8452_get_event_regs(const struct iio_chan_spec *chan,
> + enum iio_event_direction dir, const struct mma8452_event_regs
> **ev_reg)
> +{
> + if (!chan)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + *ev_reg = &ev_regs_accel_falling;
> + return 0;
> + case IIO_EV_DIR_RISING:
> + *ev_reg = &ev_regs_accel_rising;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static int mma8452_read_thresh(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> @@ -749,21 +780,23 @@ static int mma8452_read_thresh(struct iio_dev
> *indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, us, power_mode;
> + const struct mma8452_event_regs *ev_regs;
>
> + ret = mma8452_get_event_regs(chan, dir, &ev_regs);
> + if (ret)
> + return ret;
> switch (info) {
> case IIO_EV_INFO_VALUE:
> - ret = i2c_smbus_read_byte_data(data->client,
> - data->chip_info->ev_ths);
> + ret = i2c_smbus_read_byte_data(data->client, ev_regs->ev_ths);
> if (ret < 0)
> return ret;
>
> - *val = ret & data->chip_info->ev_ths_mask;
> + *val = ret & ev_regs->ev_ths_mask;
>
> return IIO_VAL_INT;
>
> case IIO_EV_INFO_PERIOD:
> - ret = i2c_smbus_read_byte_data(data->client,
> - data->chip_info->ev_count);
> + ret = i2c_smbus_read_byte_data(data->client, ev_regs->ev_count);
> if (ret < 0)
> return ret;
>
> @@ -809,14 +842,18 @@ static int mma8452_write_thresh(struct iio_dev
> *indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, reg, steps;
> + const struct mma8452_event_regs *ev_regs;
> +
> + ret = mma8452_get_event_regs(chan, dir, &ev_regs);
> + if (ret)
> + return ret;
>
> switch (info) {
> case IIO_EV_INFO_VALUE:
> - if (val < 0 || val > MMA8452_TRANSIENT_THS_MASK)
> + if (val < 0 || val > ev_regs->ev_ths_mask)
> return -EINVAL;
>
> - return mma8452_change_config(data, data->chip_info->ev_ths,
> - val);
> + return mma8452_change_config(data, ev_regs->ev_ths, val);
>
> case IIO_EV_INFO_PERIOD:
> ret = mma8452_get_power_mode(data);
> @@ -830,8 +867,7 @@ static int mma8452_write_thresh(struct iio_dev
> *indio_dev,
> if (steps < 0 || steps > 0xff)
> return -EINVAL;
>
> - return mma8452_change_config(data, data->chip_info->ev_count,
> - steps);
> + return mma8452_change_config(data, ev_regs->ev_count, steps);
>
> case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
> reg = i2c_smbus_read_byte_data(data->client,
> @@ -861,23 +897,18 @@ static int mma8452_read_event_config(struct
> iio_dev *indio_dev,
> enum iio_event_direction dir)
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> - const struct mma_chip_info *chip = data->chip_info;
> int ret;
>
> switch (dir) {
> case IIO_EV_DIR_FALLING:
> return mma8452_freefall_mode_enabled(data);
> case IIO_EV_DIR_RISING:
> - if (mma8452_freefall_mode_enabled(data))
> - return 0;
> -
> - ret = i2c_smbus_read_byte_data(data->client,
> - data->chip_info->ev_cfg);
> + ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
> if (ret < 0)
> return ret;
>
> - return !!(ret & BIT(chan->scan_index +
> - chip->ev_cfg_chan_shift));
> + return !!(ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index));
> +
> default:
> return -EINVAL;
> }
There are chips that don't have transient event registers at all. See
below.
> @@ -890,7 +921,6 @@ static int mma8452_write_event_config(struct
> iio_dev *indio_dev,
> int state)
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> - const struct mma_chip_info *chip = data->chip_info;
> int val, ret;
>
> ret = mma8452_set_runtime_pm_state(data->client, state);
> @@ -901,28 +931,18 @@ static int mma8452_write_event_config(struct
> iio_dev *indio_dev,
> case IIO_EV_DIR_FALLING:
> return mma8452_set_freefall_mode(data, state);
> case IIO_EV_DIR_RISING:
> - val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
> + val = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
> if (val < 0)
> return val;
>
> - if (state) {
> - if (mma8452_freefall_mode_enabled(data)) {
> - val &= ~BIT(idx_x + chip->ev_cfg_chan_shift);
> - val &= ~BIT(idx_y + chip->ev_cfg_chan_shift);
> - val &= ~BIT(idx_z + chip->ev_cfg_chan_shift);
> - val |= MMA8452_FF_MT_CFG_OAE;
> - }
> - val |= BIT(chan->scan_index + chip->ev_cfg_chan_shift);
> - } else {
> - if (mma8452_freefall_mode_enabled(data))
> - return 0;
> -
> - val &= ~BIT(chan->scan_index + chip->ev_cfg_chan_shift);
> - }
> + if (state)
> + val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
> + else
> + val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
>
> - val |= chip->ev_cfg_ele;
> + val |= MMA8452_TRANSIENT_CFG_ELE;
>
> - return mma8452_change_config(data, chip->ev_cfg, val);
> + return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
> default:
> return -EINVAL;
> }
> @@ -934,35 +954,25 @@ static void mma8452_transient_interrupt(struct
> iio_dev *indio_dev)
> s64 ts = iio_get_time_ns(indio_dev);
> int src;
>
> - src = i2c_smbus_read_byte_data(data->client,
> data->chip_info->ev_src);
> + src = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_SRC);
> if (src < 0)
> return;
>
> - if (mma8452_freefall_mode_enabled(data)) {
> - iio_push_event(indio_dev,
> - IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> - IIO_MOD_X_AND_Y_AND_Z,
> - IIO_EV_TYPE_MAG,
> - IIO_EV_DIR_FALLING),
> - ts);
> - return;
> - }
> -
> - if (src & data->chip_info->ev_src_xe)
> + if (src & MMA8452_TRANSIENT_SRC_XTRANSE)
> iio_push_event(indio_dev,
> IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X,
> IIO_EV_TYPE_MAG,
> IIO_EV_DIR_RISING),
> ts);
>
> - if (src & data->chip_info->ev_src_ye)
> + if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
> iio_push_event(indio_dev,
> IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Y,
> IIO_EV_TYPE_MAG,
> IIO_EV_DIR_RISING),
> ts);
>
> - if (src & data->chip_info->ev_src_ze)
> + if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
> iio_push_event(indio_dev,
> IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Z,
> IIO_EV_TYPE_MAG,
> @@ -974,23 +984,42 @@ static irqreturn_t mma8452_interrupt(int irq,
> void *p)
> {
> struct iio_dev *indio_dev = p;
> struct mma8452_data *data = iio_priv(indio_dev);
> - const struct mma_chip_info *chip = data->chip_info;
> int ret = IRQ_NONE;
> int src;
> + int enabled_interrupts;
>
> src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
> if (src < 0)
> return IRQ_NONE;
>
> + enabled_interrupts = i2c_smbus_read_byte_data(data->client,
> + MMA8452_CTRL_REG4);
> + if (enabled_interrupts < 0)
> + return enabled_interrupts;
> +
> + if (!(src & enabled_interrupts))
> + return IRQ_NONE;
> +
> if (src & MMA8452_INT_DRDY) {
> iio_trigger_poll_chained(indio_dev->trig);
> ret = IRQ_HANDLED;
> }
>
> - if ((src & MMA8452_INT_TRANS &&
> - chip->ev_src == MMA8452_TRANSIENT_SRC) ||
> - (src & MMA8452_INT_FF_MT &&
> - chip->ev_src == MMA8452_FF_MT_SRC)) {
> + if (src & MMA8452_INT_FF_MT) {
> + if (mma8452_freefall_mode_enabled(data)) {
> + s64 ts = iio_get_time_ns(indio_dev);
> +
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> + IIO_MOD_X_AND_Y_AND_Z,
> + IIO_EV_TYPE_MAG,
> + IIO_EV_DIR_FALLING),
> + ts);
> + }
> + ret = IRQ_HANDLED;
> + }
> +
> + if (src & MMA8452_INT_TRANS) {
> mma8452_transient_interrupt(indio_dev);
> ret = IRQ_HANDLED;
> }
> @@ -1222,96 +1251,36 @@ static const struct mma_chip_info
> mma_chip_info_table[] = {
> * g * N * 1000000 / 2048 for N = 2, 4, 8 and g=9.80665
> */
> .mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
> - .ev_cfg = MMA8452_TRANSIENT_CFG,
> - .ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
> - .ev_cfg_chan_shift = 1,
> - .ev_src = MMA8452_TRANSIENT_SRC,
> - .ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
> - .ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
> - .ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
> - .ev_ths = MMA8452_TRANSIENT_THS,
> - .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
> - .ev_count = MMA8452_TRANSIENT_COUNT,
> },
> [mma8452] = {
> .chip_id = MMA8452_DEVICE_ID,
> .channels = mma8452_channels,
> .num_channels = ARRAY_SIZE(mma8452_channels),
> .mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
> - .ev_cfg = MMA8452_TRANSIENT_CFG,
> - .ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
> - .ev_cfg_chan_shift = 1,
> - .ev_src = MMA8452_TRANSIENT_SRC,
> - .ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
> - .ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
> - .ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
> - .ev_ths = MMA8452_TRANSIENT_THS,
> - .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
> - .ev_count = MMA8452_TRANSIENT_COUNT,
> },
> [mma8453] = {
> .chip_id = MMA8453_DEVICE_ID,
> .channels = mma8453_channels,
> .num_channels = ARRAY_SIZE(mma8453_channels),
> .mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
> - .ev_cfg = MMA8452_TRANSIENT_CFG,
> - .ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
> - .ev_cfg_chan_shift = 1,
> - .ev_src = MMA8452_TRANSIENT_SRC,
> - .ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
> - .ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
> - .ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
> - .ev_ths = MMA8452_TRANSIENT_THS,
> - .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
> - .ev_count = MMA8452_TRANSIENT_COUNT,
> },
> [mma8652] = {
> .chip_id = MMA8652_DEVICE_ID,
> .channels = mma8652_channels,
> .num_channels = ARRAY_SIZE(mma8652_channels),
> .mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
> - .ev_cfg = MMA8452_FF_MT_CFG,
> - .ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
> - .ev_cfg_chan_shift = 3,
> - .ev_src = MMA8452_FF_MT_SRC,
> - .ev_src_xe = MMA8452_FF_MT_SRC_XHE,
> - .ev_src_ye = MMA8452_FF_MT_SRC_YHE,
> - .ev_src_ze = MMA8452_FF_MT_SRC_ZHE,
> - .ev_ths = MMA8452_FF_MT_THS,
> - .ev_ths_mask = MMA8452_FF_MT_THS_MASK,
> - .ev_count = MMA8452_FF_MT_COUNT,
> },
> [mma8653] = {
> .chip_id = MMA8653_DEVICE_ID,
> .channels = mma8653_channels,
> .num_channels = ARRAY_SIZE(mma8653_channels),
> .mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
> - .ev_cfg = MMA8452_FF_MT_CFG,
> - .ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
> - .ev_cfg_chan_shift = 3,
> - .ev_src = MMA8452_FF_MT_SRC,
> - .ev_src_xe = MMA8452_FF_MT_SRC_XHE,
> - .ev_src_ye = MMA8452_FF_MT_SRC_YHE,
> - .ev_src_ze = MMA8452_FF_MT_SRC_ZHE,
> - .ev_ths = MMA8452_FF_MT_THS,
> - .ev_ths_mask = MMA8452_FF_MT_THS_MASK,
> - .ev_count = MMA8452_FF_MT_COUNT,
Are you sure you don't break MMA8652FC and MMA8653FC? Those chips don't
have
transient event registers. Aren't you trying to use those registers
nevertheless in your
code above?
What I think about is:
If rising: use transient OR ff_mt device-dependent like before. But now
save it in a simple flag,
whether transient registers are available.
If falling: switch to ff_mt in any case. (fixing freefall for the
transient-devices)
But please explain if I missed something!
thanks
martin
> },
> [fxls8471] = {
> .chip_id = FXLS8471_DEVICE_ID,
> .channels = mma8451_channels,
> .num_channels = ARRAY_SIZE(mma8451_channels),
> .mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
> - .ev_cfg = MMA8452_TRANSIENT_CFG,
> - .ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
> - .ev_cfg_chan_shift = 1,
> - .ev_src = MMA8452_TRANSIENT_SRC,
> - .ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
> - .ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
> - .ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
> - .ev_ths = MMA8452_TRANSIENT_THS,
> - .ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
> - .ev_count = MMA8452_TRANSIENT_COUNT,
> },
> };
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] iio: accel: mma8452: improvements to handle multiple events
2017-08-21 8:47 ` Martin Kepplinger
@ 2017-08-23 0:29 ` Harinath Nampally
2017-08-23 4:52 ` Martin Kepplinger
0 siblings, 1 reply; 5+ messages in thread
From: Harinath Nampally @ 2017-08-23 0:29 UTC (permalink / raw)
To: Martin Kepplinger
Cc: jic23, knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel,
amsfield22, linux-iio-owner
>
> If rising: use transient OR ff_mt device-dependent like before. But
> now save it in a simple flag,
> whether transient registers are available.
Ok, is it good idea to add the flag to struct mma_chip_info like below?
* @mma_scales: scale factors for converting
register values
* to m/s^2; 3 modes: 2g, 4g, 8g; 2 integers
* per mode: m/s^2 and micro m/s^2
+ * @transient_supported: flag indicating whether chip support
transient
+ * event, as not all chips support
transient event
*/
struct mma_chip_info {
u8 chip_id;
const struct iio_chan_spec *channels;
int num_channels;
const int mma_scales[3][2];
+ bool transient_supported;
};
>
> If falling: switch to ff_mt in any case. (fixing freefall for the
> transient-devices)
ok sure.
Thanks,
Hari
On 08/21/2017 04:47 AM, Martin Kepplinger wrote:
>
> If rising: use transient OR ff_mt device-dependent like before. But
> now save it in a simple flag,
> whether transient registers are available.
>
> If falling: switch to ff_mt in any case. (fixing freefall for the
> transient-devices)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] iio: accel: mma8452: improvements to handle multiple events
2017-08-23 0:29 ` Harinath Nampally
@ 2017-08-23 4:52 ` Martin Kepplinger
2017-08-24 2:42 ` harinath Nampally
0 siblings, 1 reply; 5+ messages in thread
From: Martin Kepplinger @ 2017-08-23 4:52 UTC (permalink / raw)
To: Harinath Nampally
Cc: jic23, knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel,
amsfield22, linux-iio-owner
Am 23.08.2017 02:29 schrieb Harinath Nampally:
>>
>> If rising: use transient OR ff_mt device-dependent like before. But
>> now save it in a simple flag,
>> whether transient registers are available.
> Ok, is it good idea to add the flag to struct mma_chip_info like below?
>
> * @mma_scales: scale factors for converting
> register values
> * to m/s^2; 3 modes: 2g, 4g, 8g; 2
> integers
> * per mode: m/s^2 and micro m/s^2
> + * @transient_supported: flag indicating whether chip support
> transient
> + * event, as not all chips support
> transient event
> */
> struct mma_chip_info {
> u8 chip_id;
> const struct iio_chan_spec *channels;
> int num_channels;
> const int mma_scales[3][2];
> + bool transient_supported;
> };
>
I'd avoid boolean and use int and define EVENT_TYPE_TRANSIENT BIT(1) and
EVENT_TYPE_FF_MT BIT(0) for example. So something like
"supported_event_types"
can have all types supported.
But this has quite some implications on your implementation, so your
complete
solution would be more interesting to see. Keep it simple and focus on
only this one
issue of enabling freefall (FF_MT registers) for the devices that
currently use
transient registers.
thanks
>>
>> If falling: switch to ff_mt in any case. (fixing freefall for the
>> transient-devices)
> ok sure.
>
> Thanks,
>
> Hari
>
> On 08/21/2017 04:47 AM, Martin Kepplinger wrote:
>>
>> If rising: use transient OR ff_mt device-dependent like before. But
>> now save it in a simple flag,
>> whether transient registers are available.
>>
>> If falling: switch to ff_mt in any case. (fixing freefall for the
>> transient-devices)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] iio: accel: mma8452: improvements to handle multiple events
2017-08-23 4:52 ` Martin Kepplinger
@ 2017-08-24 2:42 ` harinath Nampally
0 siblings, 0 replies; 5+ messages in thread
From: harinath Nampally @ 2017-08-24 2:42 UTC (permalink / raw)
To: Martin Kepplinger
Cc: Jonathan Cameron, knaack.h, lars, Peter Meerwald-Stadler,
Greg KH, linux-iio, linux-kernel, Alison Schofield,
linux-iio-owner
> Am 23.08.2017 02:29 schrieb Harinath Nampally:
> If rising: use transient OR ff_mt device-dependent like before. But now save it in a simple flag,
> whether transient registers are available.
> Ok, is it good idea to add the flag to struct mma_chip_info like below?
> * @mma_scales: scale factors for converting
> register values
> * to m/s^2; 3 modes: 2g, 4g, 8g; 2 integers
> * per mode: m/s^2 and micro m/s^2
> + * @transient_supported: flag indicating whether chip support transient
> + * event, as not all chips support transient event
> */
> struct mma_chip_info {
> u8 chip_id;
> const struct iio_chan_spec *channels;
> int num_channels;
> const int mma_scales[3][2];
> + bool transient_supported;
> };
>
> I'd avoid boolean and use int and define EVENT_TYPE_TRANSIENT BIT(1) and
> EVENT_TYPE_FF_MT BIT(0) for example. So something like "supported_event_types"
> can have all types supported.
ok sure, I am thinking to adding 'int supported_event_types'(chip
supported events) and 'int enabled_event_types'(events enabled by this
driver for this chip). So in the probe method based on chip specific
'supported_event_types' and 'enabled_event_types' I can configure the
interrupt register accordingly.
> But this has quite some implications on your implementation, so your complete
> solution would be more interesting to see. Keep it simple and focus on only this one
> issue of enabling freefall (FF_MT registers) for the devices that currently use
> transient registers.
The main motivation of this patch was to add new events like tap and
orientation for fxls8471, So I would like to
make code changes such a way that it fixes the issue of enabling
freefall(FF_MT registers) for the devices
that currently use transient registers and also make the driver
flexible enough to add multiple new events.
Thanks,
Hari
On Wed, Aug 23, 2017 at 12:52 AM, Martin Kepplinger <martink@posteo.de> wrote:
> Am 23.08.2017 02:29 schrieb Harinath Nampally:
>>>
>>>
>>> If rising: use transient OR ff_mt device-dependent like before. But now
>>> save it in a simple flag,
>>> whether transient registers are available.
>>
>> Ok, is it good idea to add the flag to struct mma_chip_info like below?
>>
>> * @mma_scales: scale factors for converting
>> register values
>> * to m/s^2; 3 modes: 2g, 4g, 8g; 2 integers
>> * per mode: m/s^2 and micro m/s^2
>> + * @transient_supported: flag indicating whether chip support
>> transient
>> + * event, as not all chips support transient
>> event
>> */
>> struct mma_chip_info {
>> u8 chip_id;
>> const struct iio_chan_spec *channels;
>> int num_channels;
>> const int mma_scales[3][2];
>> + bool transient_supported;
>> };
>>
>
> I'd avoid boolean and use int and define EVENT_TYPE_TRANSIENT BIT(1) and
> EVENT_TYPE_FF_MT BIT(0) for example. So something like
> "supported_event_types"
> can have all types supported.
>
> But this has quite some implications on your implementation, so your
> complete
> solution would be more interesting to see. Keep it simple and focus on only
> this one
> issue of enabling freefall (FF_MT registers) for the devices that currently
> use
> transient registers.
>
> thanks
>
>
>>>
>>> If falling: switch to ff_mt in any case. (fixing freefall for the
>>> transient-devices)
>>
>> ok sure.
>>
>> Thanks,
>>
>> Hari
>>
>> On 08/21/2017 04:47 AM, Martin Kepplinger wrote:
>>>
>>>
>>> If rising: use transient OR ff_mt device-dependent like before. But now
>>> save it in a simple flag,
>>> whether transient registers are available.
>>>
>>> If falling: switch to ff_mt in any case. (fixing freefall for the
>>> transient-devices)
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-24 2:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-20 16:06 [PATCH v4] iio: accel: mma8452: improvements to handle multiple events Harinath Nampally
2017-08-21 8:47 ` Martin Kepplinger
2017-08-23 0:29 ` Harinath Nampally
2017-08-23 4:52 ` Martin Kepplinger
2017-08-24 2:42 ` harinath Nampally
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).