* [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
@ 2017-07-31 11:17 Harinath Nampally
2017-07-31 15:07 ` Martin Kepplinger
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Harinath Nampally @ 2017-07-31 11:17 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 and current design doesn't have the flexibility to add more events.
This patch fixes 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 multiple 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.
Signed-off-by: Harinath Nampally <harinath922@gmail.com>
---
drivers/iio/accel/mma8452.c | 348 ++++++++++++++++++++++----------------------
1 file changed, 175 insertions(+), 173 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index eb6e3dc..114b0e3 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,40 @@ 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_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 mma8452_event_regs {
+ 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;
+};
+
/**
* struct mma_chip_info - chip specific data
* @chip_id: WHO_AM_I register's value
@@ -116,40 +153,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 {
@@ -394,11 +403,11 @@ static ssize_t mma8452_show_os_ratio_avail(struct device *dev,
}
static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mma8452_show_samp_freq_avail);
-static IIO_DEVICE_ATTR(in_accel_scale_available, S_IRUGO,
+static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
mma8452_show_scale_avail, NULL, 0);
static IIO_DEVICE_ATTR(in_accel_filter_high_pass_3db_frequency_available,
- S_IRUGO, mma8452_show_hp_cutoff_avail, NULL, 0);
-static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, S_IRUGO,
+ 0444, mma8452_show_hp_cutoff_avail, NULL, 0);
+static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, 0444,
mma8452_show_os_ratio_avail, NULL, 0);
static int mma8452_get_samp_freq_index(struct mma8452_data *data,
@@ -602,9 +611,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 +622,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 +747,50 @@ 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,
+ struct mma8452_event_regs *ev_regs
+ )
+{
+ if (!chan)
+ return -EINVAL;
+ switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
+ ev_regs->ev_cfg_ele = MMA8452_FF_MT_CFG_ELE;
+ ev_regs->ev_cfg_chan_shift = MMA8452_FF_MT_CHAN_SHIFT;
+ ev_regs->ev_src = MMA8452_FF_MT_SRC;
+ ev_regs->ev_src_xe = MMA8452_FF_MT_SRC_XHE;
+ ev_regs->ev_src_ye = MMA8452_FF_MT_SRC_YHE;
+ ev_regs->ev_src_ze = MMA8452_FF_MT_SRC_ZHE;
+ ev_regs->ev_ths = MMA8452_FF_MT_THS;
+ ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
+ ev_regs->ev_count = MMA8452_FF_MT_COUNT;
+ break;
+ case IIO_EV_DIR_RISING:
+ ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
+ ev_regs->ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE;
+ ev_regs->ev_cfg_chan_shift = MMA8452_TRANSIENT_CHAN_SHIFT;
+ ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
+ ev_regs->ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE;
+ ev_regs->ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE;
+ ev_regs->ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE;
+ ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
+ ev_regs->ev_ths_mask = MMA8452_TRANSIENT_THS_MASK;
+ ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
+ break;
+ default:
+ return -EINVAL;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
static int mma8452_read_thresh(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan,
enum iio_event_type type,
@@ -749,21 +800,24 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, us, power_mode;
+ 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);
+ ev_regs.ev_count);
if (ret < 0)
return ret;
@@ -809,14 +863,18 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, reg, steps;
+ 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 +888,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,26 +918,29 @@ 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;
+ switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ return mma8452_freefall_mode_enabled(data);
+ case IIO_EV_DIR_RISING:
+ ret = i2c_smbus_read_byte_data(data->client,
+ MMA8452_TRANSIENT_CFG);
+ if (ret < 0)
+ return ret;
- ret = i2c_smbus_read_byte_data(data->client,
- data->chip_info->ev_cfg);
- if (ret < 0)
- return ret;
+ return ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index) ? 1 : 0;
- return !!(ret & BIT(chan->scan_index +
- chip->ev_cfg_chan_shift));
+ default:
+ return -EINVAL;
+ }
+ break;
default:
return -EINVAL;
}
+ return 0;
}
static int mma8452_write_event_config(struct iio_dev *indio_dev,
@@ -890,39 +950,36 @@ 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);
+
if (ret)
return ret;
- switch (dir) {
- 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);
- 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);
+ switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ 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,
+ MMA8452_TRANSIENT_CFG);
+ if (val < 0)
+ return val;
+
+ if (state)
+ val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
+ else
+ val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
+
+ val |= MMA8452_TRANSIENT_CFG_ELE;
+
+ return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
+ default:
+ return -EINVAL;
}
-
- val |= chip->ev_cfg_ele;
-
- return mma8452_change_config(data, chip->ev_cfg, val);
+ break;
default:
return -EINVAL;
}
@@ -934,35 +991,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 +1021,38 @@ 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 src, enabled_interrupts;
src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
if (src < 0)
return IRQ_NONE;
- if (src & MMA8452_INT_DRDY) {
+ enabled_interrupts = i2c_smbus_read_byte_data(data->client,
+ MMA8452_CTRL_REG4);
+ if (enabled_interrupts < 0)
+ return enabled_interrupts;
+
+ if ((src & MMA8452_INT_DRDY) && (src & enabled_interrupts)) {
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) && (src & enabled_interrupts)) {
+ 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) && (src & enabled_interrupts)) {
mma8452_transient_interrupt(indio_dev);
ret = IRQ_HANDLED;
}
@@ -1020,8 +1082,8 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
}
static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
- unsigned reg, unsigned writeval,
- unsigned *readval)
+ unsigned int reg, unsigned int writeval,
+ unsigned int *readval)
{
int ret;
struct mma8452_data *data = iio_priv(indio_dev);
@@ -1222,96 +1284,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] 12+ messages in thread
* Re: [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
2017-07-31 11:17 [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely Harinath Nampally
@ 2017-07-31 15:07 ` Martin Kepplinger
[not found] ` <CAAGUq_oD443x-Uq0FAZGQ8ietk9BO_Fjs7MYSNXrnR9Ogb+5tA@mail.gmail.com>
2017-08-09 13:37 ` Jonathan Cameron
2017-08-09 13:58 ` Jonathan Cameron
2 siblings, 1 reply; 12+ messages in thread
From: Martin Kepplinger @ 2017-07-31 15:07 UTC (permalink / raw)
To: Harinath Nampally, jic23
Cc: knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel, amsfield22
Am 31. Juli 2017 13:17:38 MESZ schrieb Harinath Nampally <harinath922@gmail.com>:
>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 and current design doesn't have the flexibility to add more
>events.
>
>This patch fixes 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 multiple 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.
Thanks for doing that work. I have had it on my list for a long time and you seem to fix it. Although I'd happily review and possibly test it, unfortunately I can't do so before the week of August 21st.
If this might go in quick, nothing will stop me from reviewing either, so, whatever. Thanks again!
>
>Signed-off-by: Harinath Nampally <harinath922@gmail.com>
>---
>drivers/iio/accel/mma8452.c | 348
>++++++++++++++++++++++----------------------
> 1 file changed, 175 insertions(+), 173 deletions(-)
>
>diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>index eb6e3dc..114b0e3 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,40 @@ 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_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 mma8452_event_regs {
>+ 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;
>+};
>+
> /**
> * struct mma_chip_info - chip specific data
> * @chip_id: WHO_AM_I register's value
>@@ -116,40 +153,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 {
>@@ -394,11 +403,11 @@ static ssize_t mma8452_show_os_ratio_avail(struct
>device *dev,
> }
>
> static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mma8452_show_samp_freq_avail);
>-static IIO_DEVICE_ATTR(in_accel_scale_available, S_IRUGO,
>+static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
> mma8452_show_scale_avail, NULL, 0);
>static
>IIO_DEVICE_ATTR(in_accel_filter_high_pass_3db_frequency_available,
>- S_IRUGO, mma8452_show_hp_cutoff_avail, NULL, 0);
>-static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, S_IRUGO,
>+ 0444, mma8452_show_hp_cutoff_avail, NULL, 0);
>+static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, 0444,
> mma8452_show_os_ratio_avail, NULL, 0);
>
> static int mma8452_get_samp_freq_index(struct mma8452_data *data,
>@@ -602,9 +611,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 +622,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 +747,50 @@ 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,
>+ struct mma8452_event_regs *ev_regs
>+ )
>+{
>+ if (!chan)
>+ return -EINVAL;
>+ switch (chan->type) {
>+ case IIO_ACCEL:
>+ switch (dir) {
>+ case IIO_EV_DIR_FALLING:
>+ ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
>+ ev_regs->ev_cfg_ele = MMA8452_FF_MT_CFG_ELE;
>+ ev_regs->ev_cfg_chan_shift = MMA8452_FF_MT_CHAN_SHIFT;
>+ ev_regs->ev_src = MMA8452_FF_MT_SRC;
>+ ev_regs->ev_src_xe = MMA8452_FF_MT_SRC_XHE;
>+ ev_regs->ev_src_ye = MMA8452_FF_MT_SRC_YHE;
>+ ev_regs->ev_src_ze = MMA8452_FF_MT_SRC_ZHE;
>+ ev_regs->ev_ths = MMA8452_FF_MT_THS;
>+ ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
>+ ev_regs->ev_count = MMA8452_FF_MT_COUNT;
>+ break;
>+ case IIO_EV_DIR_RISING:
>+ ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
>+ ev_regs->ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE;
>+ ev_regs->ev_cfg_chan_shift = MMA8452_TRANSIENT_CHAN_SHIFT;
>+ ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
>+ ev_regs->ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE;
>+ ev_regs->ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE;
>+ ev_regs->ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE;
>+ ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
>+ ev_regs->ev_ths_mask = MMA8452_TRANSIENT_THS_MASK;
>+ ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
>+ break;
>+ default:
>+ return -EINVAL;
>+ }
>+ break;
>+ default:
>+ return -EINVAL;
>+ }
>+ return 0;
>+}
>+
> static int mma8452_read_thresh(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
>@@ -749,21 +800,24 @@ static int mma8452_read_thresh(struct iio_dev
>*indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, us, power_mode;
>+ 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);
>+ ev_regs.ev_count);
> if (ret < 0)
> return ret;
>
>@@ -809,14 +863,18 @@ static int mma8452_write_thresh(struct iio_dev
>*indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, reg, steps;
>+ 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 +888,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,26 +918,29 @@ 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;
>+ switch (chan->type) {
>+ case IIO_ACCEL:
>+ switch (dir) {
>+ case IIO_EV_DIR_FALLING:
>+ return mma8452_freefall_mode_enabled(data);
>+ case IIO_EV_DIR_RISING:
>+ ret = i2c_smbus_read_byte_data(data->client,
>+ MMA8452_TRANSIENT_CFG);
>+ if (ret < 0)
>+ return ret;
>
>- ret = i2c_smbus_read_byte_data(data->client,
>- data->chip_info->ev_cfg);
>- if (ret < 0)
>- return ret;
>+ return ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index) ? 1 : 0;
>
>- return !!(ret & BIT(chan->scan_index +
>- chip->ev_cfg_chan_shift));
>+ default:
>+ return -EINVAL;
>+ }
>+ break;
> default:
> return -EINVAL;
> }
>+ return 0;
> }
>
> static int mma8452_write_event_config(struct iio_dev *indio_dev,
>@@ -890,39 +950,36 @@ 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);
>+
> if (ret)
> return ret;
>
>- switch (dir) {
>- 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);
>- 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);
>+ switch (chan->type) {
>+ case IIO_ACCEL:
>+ switch (dir) {
>+ 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,
>+ MMA8452_TRANSIENT_CFG);
>+ if (val < 0)
>+ return val;
>+
>+ if (state)
>+ val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
>+ else
>+ val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
>+
>+ val |= MMA8452_TRANSIENT_CFG_ELE;
>+
>+ return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
>+ default:
>+ return -EINVAL;
> }
>-
>- val |= chip->ev_cfg_ele;
>-
>- return mma8452_change_config(data, chip->ev_cfg, val);
>+ break;
> default:
> return -EINVAL;
> }
>@@ -934,35 +991,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 +1021,38 @@ 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 src, enabled_interrupts;
>
> src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
> if (src < 0)
> return IRQ_NONE;
>
>- if (src & MMA8452_INT_DRDY) {
>+ enabled_interrupts = i2c_smbus_read_byte_data(data->client,
>+ MMA8452_CTRL_REG4);
>+ if (enabled_interrupts < 0)
>+ return enabled_interrupts;
>+
>+ if ((src & MMA8452_INT_DRDY) && (src & enabled_interrupts)) {
> 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) && (src & enabled_interrupts)) {
>+ 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) && (src & enabled_interrupts)) {
> mma8452_transient_interrupt(indio_dev);
> ret = IRQ_HANDLED;
> }
>@@ -1020,8 +1082,8 @@ static irqreturn_t mma8452_trigger_handler(int
>irq, void *p)
> }
>
> static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>- unsigned reg, unsigned writeval,
>- unsigned *readval)
>+ unsigned int reg, unsigned int writeval,
>+ unsigned int *readval)
> {
> int ret;
> struct mma8452_data *data = iio_priv(indio_dev);
>@@ -1222,96 +1284,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,
> },
> };
>
--
Martin Kepplinger
http://martinkepplinger.com
sent from mobile
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
[not found] ` <CAAGUq_oD443x-Uq0FAZGQ8ietk9BO_Fjs7MYSNXrnR9Ogb+5tA@mail.gmail.com>
@ 2017-08-01 3:08 ` Harinath Nampally
2017-08-01 15:50 ` Martin Kepplinger
0 siblings, 1 reply; 12+ messages in thread
From: Harinath Nampally @ 2017-08-01 3:08 UTC (permalink / raw)
To: Martin Kepplinger
Cc: Jonathan Cameron, knaack.h, lars, Peter Meerwald-Stadler,
Greg KH, linux-iio, linux-kernel, Alison Schofield
> Thanks for doing that work. I have had it on my list for a long time and you seem to fix it. Although I'd happily review and possibly test it, unfortunately I can't do so before the week of August 21st.
>
> If this might go in quick, nothing will stop me from reviewing either, so, whatever. Thanks again!
Sure no problem, looking forward to your review comments.
Actually I am planning to add Orientation events for FXLS8471Q, for
that is it good idea to overload existing
IIO_ROT channel type? Also thinking of adding 4 channel modifiers i.e
portrait up/down, landscape left/right.
Any suggestions are welcome. Thank you.
On 07/31/2017 10:17 PM, harinath Nampally wrote:
>> 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 and current design doesn't have the flexibility to add more
>> events.
>>
>> This patch fixes 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 multiple 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.
> Thanks for doing that work. I have had it on my list for a long time and you seem to fix it. Although I'd happily review and possibly test it, unfortunately I can't do so before the week of August 21st.
>
> If this might go in quick, nothing will stop me from reviewing either, so, whatever. Thanks again!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
2017-08-01 3:08 ` Harinath Nampally
@ 2017-08-01 15:50 ` Martin Kepplinger
2017-08-10 3:51 ` Harinath Nampally
0 siblings, 1 reply; 12+ messages in thread
From: Martin Kepplinger @ 2017-08-01 15:50 UTC (permalink / raw)
To: Harinath Nampally
Cc: Jonathan Cameron, knaack.h, lars, Peter Meerwald-Stadler,
Greg KH, linux-iio, linux-kernel, Alison Schofield
On 2017-08-01 05:08, Harinath Nampally wrote:
>> Thanks for doing that work. I have had it on my list for a long time
>> and you seem to fix it. Although I'd happily review and possibly test
>> it, unfortunately I can't do so before the week of August 21st.
>>
>> If this might go in quick, nothing will stop me from reviewing either,
>> so, whatever. Thanks again!
> Sure no problem, looking forward to your review comments.
> Actually I am planning to add Orientation events for FXLS8471Q, for
> that is it good idea to overload existing
> IIO_ROT channel type? Also thinking of adding 4 channel modifiers i.e
> portrait up/down, landscape left/right.
> Any suggestions are welcome. Thank you.
>
My only suggestion for adding all these chips' orientation features, is
to start the discussion independently from this driver. Are there other
device series that provide such an orientation interrupt? Is it worth
finding a representation in iio?
Additionally to portait up/down, landscape left/right there is
back/front facing, so you'd have 8 new channel modifiers.
If IIO_ROT is a current userspace "standard" to read for rotating the
screen, it may be worth discussing how to fit this in without new
modifiers. Would you have to make up fake angle values? Anything else
userspace already uses for getting the orientation?
But again, instead of replying here and going off topic, write up a
proposal and post it independently.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
2017-07-31 11:17 [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely Harinath Nampally
2017-07-31 15:07 ` Martin Kepplinger
@ 2017-08-09 13:37 ` Jonathan Cameron
2017-08-10 3:22 ` Harinath Nampally
2017-08-09 13:58 ` Jonathan Cameron
2 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2017-08-09 13:37 UTC (permalink / raw)
To: Harinath Nampally
Cc: knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel,
amsfield22, martink
On Mon, 31 Jul 2017 07:17:38 -0400
Harinath Nampally <harinath922@gmail.com> wrote:
> 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 and current design doesn't have the flexibility to add more events.
>
> This patch fixes 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 multiple 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.
>
> Signed-off-by: Harinath Nampally <harinath922@gmail.com>
Just a quick process point before I catch up with the rest of the thread.
Please ensure you put the driver name in the patch title. We have a lot
of accelerometers these days and doing that will help draw the attention
of people who care!
Jonathan
> ---
> drivers/iio/accel/mma8452.c | 348 ++++++++++++++++++++++----------------------
> 1 file changed, 175 insertions(+), 173 deletions(-)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index eb6e3dc..114b0e3 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,40 @@ 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_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 mma8452_event_regs {
> + 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;
> +};
> +
> /**
> * struct mma_chip_info - chip specific data
> * @chip_id: WHO_AM_I register's value
> @@ -116,40 +153,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 {
> @@ -394,11 +403,11 @@ static ssize_t mma8452_show_os_ratio_avail(struct device *dev,
> }
>
> static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mma8452_show_samp_freq_avail);
> -static IIO_DEVICE_ATTR(in_accel_scale_available, S_IRUGO,
> +static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
> mma8452_show_scale_avail, NULL, 0);
> static IIO_DEVICE_ATTR(in_accel_filter_high_pass_3db_frequency_available,
> - S_IRUGO, mma8452_show_hp_cutoff_avail, NULL, 0);
> -static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, S_IRUGO,
> + 0444, mma8452_show_hp_cutoff_avail, NULL, 0);
> +static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, 0444,
> mma8452_show_os_ratio_avail, NULL, 0);
>
> static int mma8452_get_samp_freq_index(struct mma8452_data *data,
> @@ -602,9 +611,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 +622,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 +747,50 @@ 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,
> + struct mma8452_event_regs *ev_regs
> + )
> +{
> + if (!chan)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
> + ev_regs->ev_cfg_ele = MMA8452_FF_MT_CFG_ELE;
> + ev_regs->ev_cfg_chan_shift = MMA8452_FF_MT_CHAN_SHIFT;
> + ev_regs->ev_src = MMA8452_FF_MT_SRC;
> + ev_regs->ev_src_xe = MMA8452_FF_MT_SRC_XHE;
> + ev_regs->ev_src_ye = MMA8452_FF_MT_SRC_YHE;
> + ev_regs->ev_src_ze = MMA8452_FF_MT_SRC_ZHE;
> + ev_regs->ev_ths = MMA8452_FF_MT_THS;
> + ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
> + ev_regs->ev_count = MMA8452_FF_MT_COUNT;
> + break;
> + case IIO_EV_DIR_RISING:
> + ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
> + ev_regs->ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE;
> + ev_regs->ev_cfg_chan_shift = MMA8452_TRANSIENT_CHAN_SHIFT;
> + ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
> + ev_regs->ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE;
> + ev_regs->ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE;
> + ev_regs->ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE;
> + ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
> + ev_regs->ev_ths_mask = MMA8452_TRANSIENT_THS_MASK;
> + ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> static int mma8452_read_thresh(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> @@ -749,21 +800,24 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, us, power_mode;
> + 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);
> + ev_regs.ev_count);
> if (ret < 0)
> return ret;
>
> @@ -809,14 +863,18 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, reg, steps;
> + 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 +888,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,26 +918,29 @@ 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;
> + switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + return mma8452_freefall_mode_enabled(data);
> + case IIO_EV_DIR_RISING:
> + ret = i2c_smbus_read_byte_data(data->client,
> + MMA8452_TRANSIENT_CFG);
> + if (ret < 0)
> + return ret;
>
> - ret = i2c_smbus_read_byte_data(data->client,
> - data->chip_info->ev_cfg);
> - if (ret < 0)
> - return ret;
> + return ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index) ? 1 : 0;
>
> - return !!(ret & BIT(chan->scan_index +
> - chip->ev_cfg_chan_shift));
> + default:
> + return -EINVAL;
> + }
> + break;
> default:
> return -EINVAL;
> }
> + return 0;
> }
>
> static int mma8452_write_event_config(struct iio_dev *indio_dev,
> @@ -890,39 +950,36 @@ 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);
> +
> if (ret)
> return ret;
>
> - switch (dir) {
> - 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);
> - 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);
> + switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + 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,
> + MMA8452_TRANSIENT_CFG);
> + if (val < 0)
> + return val;
> +
> + if (state)
> + val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
> + else
> + val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
> +
> + val |= MMA8452_TRANSIENT_CFG_ELE;
> +
> + return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
> + default:
> + return -EINVAL;
> }
> -
> - val |= chip->ev_cfg_ele;
> -
> - return mma8452_change_config(data, chip->ev_cfg, val);
> + break;
> default:
> return -EINVAL;
> }
> @@ -934,35 +991,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 +1021,38 @@ 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 src, enabled_interrupts;
>
> src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
> if (src < 0)
> return IRQ_NONE;
>
> - if (src & MMA8452_INT_DRDY) {
> + enabled_interrupts = i2c_smbus_read_byte_data(data->client,
> + MMA8452_CTRL_REG4);
> + if (enabled_interrupts < 0)
> + return enabled_interrupts;
> +
> + if ((src & MMA8452_INT_DRDY) && (src & enabled_interrupts)) {
> 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) && (src & enabled_interrupts)) {
> + 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) && (src & enabled_interrupts)) {
> mma8452_transient_interrupt(indio_dev);
> ret = IRQ_HANDLED;
> }
> @@ -1020,8 +1082,8 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
> }
>
> static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
> - unsigned reg, unsigned writeval,
> - unsigned *readval)
> + unsigned int reg, unsigned int writeval,
> + unsigned int *readval)
> {
> int ret;
> struct mma8452_data *data = iio_priv(indio_dev);
> @@ -1222,96 +1284,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,
> },
> };
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
2017-07-31 11:17 [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely Harinath Nampally
2017-07-31 15:07 ` Martin Kepplinger
2017-08-09 13:37 ` Jonathan Cameron
@ 2017-08-09 13:58 ` Jonathan Cameron
2017-08-10 3:39 ` Harinath Nampally
2017-08-19 2:08 ` [PATCH] iio: accel: mma8452: code improvements to handle more than one event Harinath Nampally
2 siblings, 2 replies; 12+ messages in thread
From: Jonathan Cameron @ 2017-08-09 13:58 UTC (permalink / raw)
To: Harinath Nampally
Cc: knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel,
amsfield22, martink
On Mon, 31 Jul 2017 07:17:38 -0400
Harinath Nampally <harinath922@gmail.com> wrote:
> 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 and current design doesn't have the flexibility to add more events.
>
> This patch fixes 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 multiple 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.
>
> Signed-off-by: Harinath Nampally <harinath922@gmail.com>
Hi,
A few minor bits and bobs inline.
Jonathan
> ---
> drivers/iio/accel/mma8452.c | 348 ++++++++++++++++++++++----------------------
> 1 file changed, 175 insertions(+), 173 deletions(-)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index eb6e3dc..114b0e3 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,40 @@ 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_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 mma8452_event_regs {
> + u8 ev_cfg;
> + u8 ev_cfg_ele;
> + u8 ev_cfg_chan_shift;
As far as I can tell the above isn't used...
Please sanity check the others
> + 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;
> +};
> +
> /**
> * struct mma_chip_info - chip specific data
> * @chip_id: WHO_AM_I register's value
> @@ -116,40 +153,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 {
> @@ -394,11 +403,11 @@ static ssize_t mma8452_show_os_ratio_avail(struct device *dev,
> }
>
> static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mma8452_show_samp_freq_avail);
> -static IIO_DEVICE_ATTR(in_accel_scale_available, S_IRUGO,
> +static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
> mma8452_show_scale_avail, NULL, 0);
> static IIO_DEVICE_ATTR(in_accel_filter_high_pass_3db_frequency_available,
> - S_IRUGO, mma8452_show_hp_cutoff_avail, NULL, 0);
> -static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, S_IRUGO,
> + 0444, mma8452_show_hp_cutoff_avail, NULL, 0);
> +static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, 0444,
> mma8452_show_os_ratio_avail, NULL, 0);
Separate change. Please do it in a precursor patch rather than
adding noise to this one..
>
> static int mma8452_get_samp_freq_index(struct mma8452_data *data,
> @@ -602,9 +611,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 +622,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 +747,50 @@ 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,
> + struct mma8452_event_regs *ev_regs
> + )
> +{
> + if (!chan)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
> + ev_regs->ev_cfg_ele = MMA8452_FF_MT_CFG_ELE;
> + ev_regs->ev_cfg_chan_shift = MMA8452_FF_MT_CHAN_SHIFT;
> + ev_regs->ev_src = MMA8452_FF_MT_SRC;
> + ev_regs->ev_src_xe = MMA8452_FF_MT_SRC_XHE;
> + ev_regs->ev_src_ye = MMA8452_FF_MT_SRC_YHE;
> + ev_regs->ev_src_ze = MMA8452_FF_MT_SRC_ZHE;
> + ev_regs->ev_ths = MMA8452_FF_MT_THS;
> + ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
> + ev_regs->ev_count = MMA8452_FF_MT_COUNT;
> + break;
> + case IIO_EV_DIR_RISING:
> + ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
> + ev_regs->ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE;
> + ev_regs->ev_cfg_chan_shift = MMA8452_TRANSIENT_CHAN_SHIFT;
> + ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
> + ev_regs->ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE;
> + ev_regs->ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE;
> + ev_regs->ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE;
> + ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
> + ev_regs->ev_ths_mask = MMA8452_TRANSIENT_THS_MASK;
> + ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> static int mma8452_read_thresh(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> @@ -749,21 +800,24 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, us, power_mode;
> + 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);
> + ev_regs.ev_count);
This indenting looks somewhat odd..
> if (ret < 0)
> return ret;
>
> @@ -809,14 +863,18 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, reg, steps;
> + 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 +888,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,26 +918,29 @@ 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;
> + switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + return mma8452_freefall_mode_enabled(data);
> + case IIO_EV_DIR_RISING:
> + ret = i2c_smbus_read_byte_data(data->client,
> + MMA8452_TRANSIENT_CFG);
Again, some crazy stuff going on with indenting..
> + if (ret < 0)
> + return ret;
>
> - ret = i2c_smbus_read_byte_data(data->client,
> - data->chip_info->ev_cfg);
> - if (ret < 0)
> - return ret;
> + return ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index) ? 1 : 0;
It's a nasty trick in a way, but commonly used in the kernel.
return !!(ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index));
>
> - return !!(ret & BIT(chan->scan_index +
> - chip->ev_cfg_chan_shift));
> + default:
> + return -EINVAL;
> + }
> + break;
More strange indenting.
> default:
> return -EINVAL;
> }
> + return 0;
Err, how can we get here? Line not needed.
> }
>
> static int mma8452_write_event_config(struct iio_dev *indio_dev,
> @@ -890,39 +950,36 @@ 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);
> +
Make sure you check patches for things that have been accidentally
introduced like this.
> if (ret)
> return ret;
>
> - switch (dir) {
> - 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);
> - 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);
> + switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + 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,
> + MMA8452_TRANSIENT_CFG);
> + if (val < 0)
> + return val;
> +
> + if (state)
> + val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
> + else
> + val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
> +
> + val |= MMA8452_TRANSIENT_CFG_ELE;
> +
> + return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
> + default:
> + return -EINVAL;
> }
> -
> - val |= chip->ev_cfg_ele;
> -
> - return mma8452_change_config(data, chip->ev_cfg, val);
> + break;
> default:
> return -EINVAL;
> }
> @@ -934,35 +991,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 +1021,38 @@ 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 src, enabled_interrupts;
>
> src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
> if (src < 0)
> return IRQ_NONE;
>
> - if (src & MMA8452_INT_DRDY) {
> + enabled_interrupts = i2c_smbus_read_byte_data(data->client,
> + MMA8452_CTRL_REG4);
> + if (enabled_interrupts < 0)
> + return enabled_interrupts;
> +
> + if ((src & MMA8452_INT_DRDY) && (src & enabled_interrupts)) {
> 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) && (src & enabled_interrupts)) {
> + 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) && (src & enabled_interrupts)) {
> mma8452_transient_interrupt(indio_dev);
> ret = IRQ_HANDLED;
> }
> @@ -1020,8 +1082,8 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
> }
>
> static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
> - unsigned reg, unsigned writeval,
> - unsigned *readval)
> + unsigned int reg, unsigned int writeval,
> + unsigned int *readval)
> {
> int ret;
> struct mma8452_data *data = iio_priv(indio_dev);
> @@ -1222,96 +1284,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,
> },
> };
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
2017-08-09 13:37 ` Jonathan Cameron
@ 2017-08-10 3:22 ` Harinath Nampally
0 siblings, 0 replies; 12+ messages in thread
From: Harinath Nampally @ 2017-08-10 3:22 UTC (permalink / raw)
To: Jonathan Cameron
Cc: knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel,
amsfield22, martink
> On Mon, 31 Jul 2017 07:17:38 -0400
> Harinath Nampally<harinath922@gmail.com> wrote:
>
>> 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 and current design doesn't have the flexibility to add more events.
>>
>> This patch fixes 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 multiple 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.
>>
>> Signed-off-by: Harinath Nampally<harinath922@gmail.com>
> Just a quick process point before I catch up with the rest of the thread.
> Please ensure you put the driver name in the patch title. We have a lot
> of accelerometers these days and doing that will help draw the attention
> of people who care!
Sure, I will update it. Thanks.
On 08/09/2017 09:37 AM, Jonathan Cameron wrote:
> On Mon, 31 Jul 2017 07:17:38 -0400
> Harinath Nampally<harinath922@gmail.com> wrote:
>
>> 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 and current design doesn't have the flexibility to add more events.
>>
>> This patch fixes 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 multiple 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.
>>
>> Signed-off-by: Harinath Nampally<harinath922@gmail.com>
> Just a quick process point before I catch up with the rest of the thread.
> Please ensure you put the driver name in the patch title. We have a lot
> of accelerometers these days and doing that will help draw the attention
> of people who care!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
2017-08-09 13:58 ` Jonathan Cameron
@ 2017-08-10 3:39 ` Harinath Nampally
2017-08-19 2:08 ` [PATCH] iio: accel: mma8452: code improvements to handle more than one event Harinath Nampally
1 sibling, 0 replies; 12+ messages in thread
From: Harinath Nampally @ 2017-08-10 3:39 UTC (permalink / raw)
To: Jonathan Cameron
Cc: knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel,
amsfield22, martink
> On Mon, 31 Jul 2017 07:17:38 -0400
> Harinath Nampally <harinath922@gmail.com> wrote:
>
>> 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 and current design doesn't have the flexibility to add more events.
>>
>> This patch fixes 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 multiple 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.
>>
>> Signed-off-by: Harinath Nampally <harinath922@gmail.com>
> Hi,
>
> A few minor bits and bobs inline.
>
> Jonathan
Thank you for the review.
> + /**
> + * struct mma8452_event_regs - chip specific data related to events
> + * @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 mma8452_event_regs {
> + u8 ev_cfg;
> + u8 ev_cfg_ele;
> + u8 ev_cfg_chan_shift;
> As far as I can tell the above isn't used...
> Please sanity check the others
>
Yes they are not used and not really necessary I think, probably I
should remove them!
It makes sense to only have ev_cfg, ev_src, ev_ths, ev_ths_mask and
ev_count.
as they are common to other events as well like orientation,
single/double tap etc.
So in future this same struct can be reused across different events.
> enum {
> @@ -394,11 +403,11 @@ static ssize_t mma8452_show_os_ratio_avail(struct device *dev,
> }
>
> static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mma8452_show_samp_freq_avail);
> -static IIO_DEVICE_ATTR(in_accel_scale_available, S_IRUGO,
> +static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
> mma8452_show_scale_avail, NULL, 0);
> static IIO_DEVICE_ATTR(in_accel_filter_high_pass_3db_frequency_available,
> - S_IRUGO, mma8452_show_hp_cutoff_avail, NULL, 0);
> -static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, S_IRUGO,
> + 0444, mma8452_show_hp_cutoff_avail, NULL, 0);
> +static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, 0444,
> mma8452_show_os_ratio_avail, NULL, 0);
> Separate change. Please do it in a precursor patch rather than
> adding noise to this one..
Sure I will.
> case IIO_EV_INFO_PERIOD:
> ret = i2c_smbus_read_byte_data(data->client,
> - data->chip_info->ev_count);
> + ev_regs.ev_count);
> This indenting looks somewhat odd..
Yes I agree, will fix it.
> + switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + return mma8452_freefall_mode_enabled(data);
> + case IIO_EV_DIR_RISING:
> + ret = i2c_smbus_read_byte_data(data->client,
> + MMA8452_TRANSIENT_CFG);
> Again, some crazy stuff going on with indenting..
>> + if (ret < 0)
>> + return ret;
>>
>> - ret = i2c_smbus_read_byte_data(data->client,
>> - data->chip_info->ev_cfg);
>> - if (ret < 0)
>> - return ret;
>> + return ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index) ? 1 : 0;
> It's a nasty trick in a way, but commonly used in the kernel.
> return !!(ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index));
>
>>
>> - return !!(ret & BIT(chan->scan_index +
>> - chip->ev_cfg_chan_shift));
>> + default:
>> + return -EINVAL;
>> + }
>> + break;
> More strange indenting.
>> default:
>> return -EINVAL;
>> }
>> + return 0;
> Err, how can we get here? Line not needed.
>> }
>>
>> static int mma8452_write_event_config(struct iio_dev *indio_dev,
>> @@ -890,39 +950,36 @@ 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);
>> +
> Make sure you check patches for things that have been accidentally
> introduced like this.
Sure, will fix all the indentations and extra line issues etc.
On 08/09/2017 09:58 AM, Jonathan Cameron wrote:
> On Mon, 31 Jul 2017 07:17:38 -0400
> Harinath Nampally <harinath922@gmail.com> wrote:
>
>> 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 and current design doesn't have the flexibility to add more events.
>>
>> This patch fixes 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 multiple 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.
>>
>> Signed-off-by: Harinath Nampally <harinath922@gmail.com>
> Hi,
>
> A few minor bits and bobs inline.
>
> Jonathan
>> ---
>> drivers/iio/accel/mma8452.c | 348 ++++++++++++++++++++++----------------------
>> 1 file changed, 175 insertions(+), 173 deletions(-)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index eb6e3dc..114b0e3 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,40 @@ 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_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 mma8452_event_regs {
>> + u8 ev_cfg;
>> + u8 ev_cfg_ele;
>> + u8 ev_cfg_chan_shift;
> As far as I can tell the above isn't used...
> Please sanity check the others
>
>> + 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;
>> +};
>> +
>> /**
>> * struct mma_chip_info - chip specific data
>> * @chip_id: WHO_AM_I register's value
>> @@ -116,40 +153,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 {
>> @@ -394,11 +403,11 @@ static ssize_t mma8452_show_os_ratio_avail(struct device *dev,
>> }
>>
>> static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mma8452_show_samp_freq_avail);
>> -static IIO_DEVICE_ATTR(in_accel_scale_available, S_IRUGO,
>> +static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
>> mma8452_show_scale_avail, NULL, 0);
>> static IIO_DEVICE_ATTR(in_accel_filter_high_pass_3db_frequency_available,
>> - S_IRUGO, mma8452_show_hp_cutoff_avail, NULL, 0);
>> -static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, S_IRUGO,
>> + 0444, mma8452_show_hp_cutoff_avail, NULL, 0);
>> +static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, 0444,
>> mma8452_show_os_ratio_avail, NULL, 0);
> Separate change. Please do it in a precursor patch rather than
> adding noise to this one..
>>
>> static int mma8452_get_samp_freq_index(struct mma8452_data *data,
>> @@ -602,9 +611,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 +622,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 +747,50 @@ 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,
>> + struct mma8452_event_regs *ev_regs
>> + )
>> +{
>> + if (!chan)
>> + return -EINVAL;
>> + switch (chan->type) {
>> + case IIO_ACCEL:
>> + switch (dir) {
>> + case IIO_EV_DIR_FALLING:
>> + ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
>> + ev_regs->ev_cfg_ele = MMA8452_FF_MT_CFG_ELE;
>> + ev_regs->ev_cfg_chan_shift = MMA8452_FF_MT_CHAN_SHIFT;
>> + ev_regs->ev_src = MMA8452_FF_MT_SRC;
>> + ev_regs->ev_src_xe = MMA8452_FF_MT_SRC_XHE;
>> + ev_regs->ev_src_ye = MMA8452_FF_MT_SRC_YHE;
>> + ev_regs->ev_src_ze = MMA8452_FF_MT_SRC_ZHE;
>> + ev_regs->ev_ths = MMA8452_FF_MT_THS;
>> + ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
>> + ev_regs->ev_count = MMA8452_FF_MT_COUNT;
>> + break;
>> + case IIO_EV_DIR_RISING:
>> + ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
>> + ev_regs->ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE;
>> + ev_regs->ev_cfg_chan_shift = MMA8452_TRANSIENT_CHAN_SHIFT;
>> + ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
>> + ev_regs->ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE;
>> + ev_regs->ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE;
>> + ev_regs->ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE;
>> + ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
>> + ev_regs->ev_ths_mask = MMA8452_TRANSIENT_THS_MASK;
>> + ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + return 0;
>> +}
>> +
>> static int mma8452_read_thresh(struct iio_dev *indio_dev,
>> const struct iio_chan_spec *chan,
>> enum iio_event_type type,
>> @@ -749,21 +800,24 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
>> {
>> struct mma8452_data *data = iio_priv(indio_dev);
>> int ret, us, power_mode;
>> + 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);
>> + ev_regs.ev_count);
> This indenting looks somewhat odd..
>> if (ret < 0)
>> return ret;
>>
>> @@ -809,14 +863,18 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
>> {
>> struct mma8452_data *data = iio_priv(indio_dev);
>> int ret, reg, steps;
>> + 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 +888,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,26 +918,29 @@ 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;
>> + switch (chan->type) {
>> + case IIO_ACCEL:
>> + switch (dir) {
>> + case IIO_EV_DIR_FALLING:
>> + return mma8452_freefall_mode_enabled(data);
>> + case IIO_EV_DIR_RISING:
>> + ret = i2c_smbus_read_byte_data(data->client,
>> + MMA8452_TRANSIENT_CFG);
> Again, some crazy stuff going on with indenting..
>> + if (ret < 0)
>> + return ret;
>>
>> - ret = i2c_smbus_read_byte_data(data->client,
>> - data->chip_info->ev_cfg);
>> - if (ret < 0)
>> - return ret;
>> + return ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index) ? 1 : 0;
> It's a nasty trick in a way, but commonly used in the kernel.
> return !!(ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index));
>
>>
>> - return !!(ret & BIT(chan->scan_index +
>> - chip->ev_cfg_chan_shift));
>> + default:
>> + return -EINVAL;
>> + }
>> + break;
> More strange indenting.
>> default:
>> return -EINVAL;
>> }
>> + return 0;
> Err, how can we get here? Line not needed.
>> }
>>
>> static int mma8452_write_event_config(struct iio_dev *indio_dev,
>> @@ -890,39 +950,36 @@ 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);
>> +
> Make sure you check patches for things that have been accidentally
> introduced like this.
>> if (ret)
>> return ret;
>>
>> - switch (dir) {
>> - 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);
>> - 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);
>> + switch (chan->type) {
>> + case IIO_ACCEL:
>> + switch (dir) {
>> + 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,
>> + MMA8452_TRANSIENT_CFG);
>> + if (val < 0)
>> + return val;
>> +
>> + if (state)
>> + val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
>> + else
>> + val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
>> +
>> + val |= MMA8452_TRANSIENT_CFG_ELE;
>> +
>> + return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
>> + default:
>> + return -EINVAL;
>> }
>> -
>> - val |= chip->ev_cfg_ele;
>> -
>> - return mma8452_change_config(data, chip->ev_cfg, val);
>> + break;
>> default:
>> return -EINVAL;
>> }
>> @@ -934,35 +991,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 +1021,38 @@ 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 src, enabled_interrupts;
>>
>> src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>> if (src < 0)
>> return IRQ_NONE;
>>
>> - if (src & MMA8452_INT_DRDY) {
>> + enabled_interrupts = i2c_smbus_read_byte_data(data->client,
>> + MMA8452_CTRL_REG4);
>> + if (enabled_interrupts < 0)
>> + return enabled_interrupts;
>> +
>> + if ((src & MMA8452_INT_DRDY) && (src & enabled_interrupts)) {
>> 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) && (src & enabled_interrupts)) {
>> + 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) && (src & enabled_interrupts)) {
>> mma8452_transient_interrupt(indio_dev);
>> ret = IRQ_HANDLED;
>> }
>> @@ -1020,8 +1082,8 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
>> }
>>
>> static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>> - unsigned reg, unsigned writeval,
>> - unsigned *readval)
>> + unsigned int reg, unsigned int writeval,
>> + unsigned int *readval)
>> {
>> int ret;
>> struct mma8452_data *data = iio_priv(indio_dev);
>> @@ -1222,96 +1284,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,
>> },
>> };
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
2017-08-01 15:50 ` Martin Kepplinger
@ 2017-08-10 3:51 ` Harinath Nampally
0 siblings, 0 replies; 12+ messages in thread
From: Harinath Nampally @ 2017-08-10 3:51 UTC (permalink / raw)
To: Martin Kepplinger
Cc: Jonathan Cameron, knaack.h, lars, Peter Meerwald-Stadler,
Greg KH, linux-iio, linux-kernel, Alison Schofield
> My only suggestion for adding all these chips' orientation features, is
> to start the discussion independently from this driver. Are there other
> device series that provide such an orientation interrupt? Is it worth
> finding a representation in iio?
Given the number of accelerometers these days have built in orientation
event support,
I think its worth to have a representation in IIO,
> Additionally to portait up/down, landscape left/right there is
> back/front facing, so you'd have 8 new channel modifiers.
Yes that's correct but I wonder if its good idea to add 8(too many!) new
channel modifiers.
> If IIO_ROT is a current userspace "standard" to read for rotating the
> screen, it may be worth discussing how to fit this in without new
> modifiers. Would you have to make up fake angle values? Anything else
> userspace already uses for getting the orientation?
Yes I agree, I don't think I need to make up fake angle values, not sure
how userspace gets orientation currently. Need to do some research on that.
> But again, instead of replying here and going off topic, write up a
> proposal and post it independently.
Sure will do that. Thanks for your response.
On 08/01/2017 11:50 AM, Martin Kepplinger wrote:
> On 2017-08-01 05:08, Harinath Nampally wrote:
>>> Thanks for doing that work. I have had it on my list for a long time
>>> and you seem to fix it. Although I'd happily review and possibly test
>>> it, unfortunately I can't do so before the week of August 21st.
>>>
>>> If this might go in quick, nothing will stop me from reviewing either,
>>> so, whatever. Thanks again!
>> Sure no problem, looking forward to your review comments.
>> Actually I am planning to add Orientation events for FXLS8471Q, for
>> that is it good idea to overload existing
>> IIO_ROT channel type? Also thinking of adding 4 channel modifiers i.e
>> portrait up/down, landscape left/right.
>> Any suggestions are welcome. Thank you.
>>
> My only suggestion for adding all these chips' orientation features, is
> to start the discussion independently from this driver. Are there other
> device series that provide such an orientation interrupt? Is it worth
> finding a representation in iio?
>
> Additionally to portait up/down, landscape left/right there is
> back/front facing, so you'd have 8 new channel modifiers.
>
> If IIO_ROT is a current userspace "standard" to read for rotating the
> screen, it may be worth discussing how to fit this in without new
> modifiers. Would you have to make up fake angle values? Anything else
> userspace already uses for getting the orientation?
>
> But again, instead of replying here and going off topic, write up a
> proposal and post it independently.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] iio: accel: mma8452: code improvements to handle more than one event
2017-08-09 13:58 ` Jonathan Cameron
2017-08-10 3:39 ` Harinath Nampally
@ 2017-08-19 2:08 ` Harinath Nampally
2017-08-20 10:45 ` Jonathan Cameron
1 sibling, 1 reply; 12+ messages in thread
From: Harinath Nampally @ 2017-08-19 2:08 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 and 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 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 | 272 +++++++++++++++++++-------------------------
1 file changed, 118 insertions(+), 154 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index eb6e3dc..8de9928 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,26 @@ 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;
+};
+
/**
* struct mma_chip_info - chip specific data
* @chip_id: WHO_AM_I register's value
@@ -116,40 +139,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 +597,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 +608,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 +733,39 @@ 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,
+ struct mma8452_event_regs *ev_regs
+ )
+{
+ if (!chan)
+ return -EINVAL;
+ switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
+ ev_regs->ev_src = MMA8452_FF_MT_SRC;
+ ev_regs->ev_ths = MMA8452_FF_MT_THS;
+ ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
+ ev_regs->ev_count = MMA8452_FF_MT_COUNT;
+ return 0;
+ case IIO_EV_DIR_RISING:
+ ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
+ ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
+ ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
+ ev_regs->ev_ths_mask = MMA8452_TRANSIENT_THS_MASK;
+ ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
+ return 0;
+ default:
+ return -EINVAL;
+ }
+ break;
+ 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 +775,23 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, us, power_mode;
+ 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 +837,18 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, reg, steps;
+ 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 +862,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 +892,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 +916,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 +926,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;
+ if (state)
+ val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
+ else
+ val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
- val &= ~BIT(chan->scan_index + chip->ev_cfg_chan_shift);
- }
-
- 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 +949,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 +979,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 +1246,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] 12+ messages in thread
* Re: [PATCH] iio: accel: mma8452: code improvements to handle more than one event
2017-08-19 2:08 ` [PATCH] iio: accel: mma8452: code improvements to handle more than one event Harinath Nampally
@ 2017-08-20 10:45 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2017-08-20 10:45 UTC (permalink / raw)
To: Harinath Nampally
Cc: knaack.h, lars, pmeerw, gregkh, linux-iio, linux-kernel,
amsfield22, martink
On Fri, 18 Aug 2017 22:08:10 -0400
Harinath Nampally <harinath922@gmail.com> wrote:
> 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 and 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.
Please keep description to sub 75 characters (it tends to get indented by some
tools so best to leave it a bit shorter than the 80 char limit).
>
> 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 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>
For some reason this appears to be a reply to an earlier series. Please
always start a fresh thread for a fresh version of a patch. I almost
missed it entirely.
Anyhow, one suggestion inline. We never tend to like functions that fill
in what is basically constant data. If it's constant then make it so as
there are lots of advantages (including XIP).
Jonathan
> ---
> drivers/iio/accel/mma8452.c | 272 +++++++++++++++++++-------------------------
> 1 file changed, 118 insertions(+), 154 deletions(-)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index eb6e3dc..8de9928 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,26 @@ 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;
> +};
> +
> /**
> * struct mma_chip_info - chip specific data
> * @chip_id: WHO_AM_I register's value
> @@ -116,40 +139,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 +597,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 +608,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 +733,39 @@ 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,
> + struct mma8452_event_regs *ev_regs
> + )
> +{
> + if (!chan)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
> + ev_regs->ev_src = MMA8452_FF_MT_SRC;
> + ev_regs->ev_ths = MMA8452_FF_MT_THS;
> + ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
> + ev_regs->ev_count = MMA8452_FF_MT_COUNT;
> + return 0;
> + case IIO_EV_DIR_RISING:
> + ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
> + ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
> + ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
> + ev_regs->ev_ths_mask = MMA8452_TRANSIENT_THS_MASK;
> + ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
This feels very much like something that could be moved into a set of static
const strutures of the various options.
Then this function could simply pick the right one and return
a pointer to it rather than having to fill in the structure.
static const struct mma8452_event_regs ev_regs_accel_falling = {
.ev_cfg = MMA8452_FF_MT_CFG,
..
};
static const struct mma8452_event_regs ev_regs_accel_rising = {
.ev_cfg = MMA8452_TRANSIENT_CFG,
..
};
static int mma8452_get_event_regs(const struct iio_chan_spec *chan,
enum iio_event_direction dir,
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;
}
}
> + return 0;
> + default:
> + return -EINVAL;
> + }
> + break;
> + 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 +775,23 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, us, power_mode;
> + 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 +837,18 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret, reg, steps;
> + 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 +862,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 +892,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 +916,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 +926,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;
> + if (state)
> + val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
> + else
> + val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
>
> - val &= ~BIT(chan->scan_index + chip->ev_cfg_chan_shift);
> - }
> -
> - 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 +949,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 +979,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 +1246,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,
> },
> };
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely.
@ 2017-07-31 1:17 Harinath Nampally
0 siblings, 0 replies; 12+ messages in thread
From: Harinath Nampally @ 2017-07-31 1:17 UTC (permalink / raw)
To: lars
Cc: Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio,
devel, linux-kernel
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 and current design doesn't have the flexibility to add more events.
This patch fixes 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 multiple 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.
Signed-off-by: Harinath Nampally <harinath922@gmail.com>
---
drivers/iio/accel/mma8452.c | 348 ++++++++++++++++++++++----------------------
1 file changed, 175 insertions(+), 173 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index eb6e3dc..114b0e3 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,40 @@ 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_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 mma8452_event_regs {
+ 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;
+};
+
/**
* struct mma_chip_info - chip specific data
* @chip_id: WHO_AM_I register's value
@@ -116,40 +153,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 {
@@ -394,11 +403,11 @@ static ssize_t mma8452_show_os_ratio_avail(struct device *dev,
}
static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mma8452_show_samp_freq_avail);
-static IIO_DEVICE_ATTR(in_accel_scale_available, S_IRUGO,
+static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
mma8452_show_scale_avail, NULL, 0);
static IIO_DEVICE_ATTR(in_accel_filter_high_pass_3db_frequency_available,
- S_IRUGO, mma8452_show_hp_cutoff_avail, NULL, 0);
-static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, S_IRUGO,
+ 0444, mma8452_show_hp_cutoff_avail, NULL, 0);
+static IIO_DEVICE_ATTR(in_accel_oversampling_ratio_available, 0444,
mma8452_show_os_ratio_avail, NULL, 0);
static int mma8452_get_samp_freq_index(struct mma8452_data *data,
@@ -602,9 +611,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 +622,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 +747,50 @@ 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,
+ struct mma8452_event_regs *ev_regs
+ )
+{
+ if (!chan)
+ return -EINVAL;
+ switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
+ ev_regs->ev_cfg_ele = MMA8452_FF_MT_CFG_ELE;
+ ev_regs->ev_cfg_chan_shift = MMA8452_FF_MT_CHAN_SHIFT;
+ ev_regs->ev_src = MMA8452_FF_MT_SRC;
+ ev_regs->ev_src_xe = MMA8452_FF_MT_SRC_XHE;
+ ev_regs->ev_src_ye = MMA8452_FF_MT_SRC_YHE;
+ ev_regs->ev_src_ze = MMA8452_FF_MT_SRC_ZHE;
+ ev_regs->ev_ths = MMA8452_FF_MT_THS;
+ ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
+ ev_regs->ev_count = MMA8452_FF_MT_COUNT;
+ break;
+ case IIO_EV_DIR_RISING:
+ ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
+ ev_regs->ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE;
+ ev_regs->ev_cfg_chan_shift = MMA8452_TRANSIENT_CHAN_SHIFT;
+ ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
+ ev_regs->ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE;
+ ev_regs->ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE;
+ ev_regs->ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE;
+ ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
+ ev_regs->ev_ths_mask = MMA8452_TRANSIENT_THS_MASK;
+ ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
+ break;
+ default:
+ return -EINVAL;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
static int mma8452_read_thresh(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan,
enum iio_event_type type,
@@ -749,21 +800,24 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, us, power_mode;
+ 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);
+ ev_regs.ev_count);
if (ret < 0)
return ret;
@@ -809,14 +863,18 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, reg, steps;
+ 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 +888,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,26 +918,29 @@ 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;
+ switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ return mma8452_freefall_mode_enabled(data);
+ case IIO_EV_DIR_RISING:
+ ret = i2c_smbus_read_byte_data(data->client,
+ MMA8452_TRANSIENT_CFG);
+ if (ret < 0)
+ return ret;
- ret = i2c_smbus_read_byte_data(data->client,
- data->chip_info->ev_cfg);
- if (ret < 0)
- return ret;
+ return ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index) ? 1 : 0;
- return !!(ret & BIT(chan->scan_index +
- chip->ev_cfg_chan_shift));
+ default:
+ return -EINVAL;
+ }
+ break;
default:
return -EINVAL;
}
+ return 0;
}
static int mma8452_write_event_config(struct iio_dev *indio_dev,
@@ -890,39 +950,36 @@ 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);
+
if (ret)
return ret;
- switch (dir) {
- 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);
- 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);
+ switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ 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,
+ MMA8452_TRANSIENT_CFG);
+ if (val < 0)
+ return val;
+
+ if (state)
+ val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
+ else
+ val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
+
+ val |= MMA8452_TRANSIENT_CFG_ELE;
+
+ return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
+ default:
+ return -EINVAL;
}
-
- val |= chip->ev_cfg_ele;
-
- return mma8452_change_config(data, chip->ev_cfg, val);
+ break;
default:
return -EINVAL;
}
@@ -934,35 +991,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 +1021,38 @@ 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 src, enabled_interrupts;
src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
if (src < 0)
return IRQ_NONE;
- if (src & MMA8452_INT_DRDY) {
+ enabled_interrupts = i2c_smbus_read_byte_data(data->client,
+ MMA8452_CTRL_REG4);
+ if (enabled_interrupts < 0)
+ return enabled_interrupts;
+
+ if ((src & MMA8452_INT_DRDY) && (src & enabled_interrupts)) {
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) && (src & enabled_interrupts)) {
+ 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) && (src & enabled_interrupts)) {
mma8452_transient_interrupt(indio_dev);
ret = IRQ_HANDLED;
}
@@ -1020,8 +1082,8 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
}
static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
- unsigned reg, unsigned writeval,
- unsigned *readval)
+ unsigned int reg, unsigned int writeval,
+ unsigned int *readval)
{
int ret;
struct mma8452_data *data = iio_priv(indio_dev);
@@ -1222,96 +1284,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] 12+ messages in thread
end of thread, other threads:[~2017-08-20 10:45 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-31 11:17 [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely Harinath Nampally
2017-07-31 15:07 ` Martin Kepplinger
[not found] ` <CAAGUq_oD443x-Uq0FAZGQ8ietk9BO_Fjs7MYSNXrnR9Ogb+5tA@mail.gmail.com>
2017-08-01 3:08 ` Harinath Nampally
2017-08-01 15:50 ` Martin Kepplinger
2017-08-10 3:51 ` Harinath Nampally
2017-08-09 13:37 ` Jonathan Cameron
2017-08-10 3:22 ` Harinath Nampally
2017-08-09 13:58 ` Jonathan Cameron
2017-08-10 3:39 ` Harinath Nampally
2017-08-19 2:08 ` [PATCH] iio: accel: mma8452: code improvements to handle more than one event Harinath Nampally
2017-08-20 10:45 ` Jonathan Cameron
-- strict thread matches above, loose matches on Subject: below --
2017-07-31 1:17 [PATCH] iio: accel: Bugfix to enbale and allow different events to work parallely 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).