Hello, fixes are always welcome, some comments regarding the patch in subject: typo "enable" > 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. which chip can have which event(s)? > 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, p. > Signed-off-by: Harinath Nampally <harinath922@xxxxxxxxx> > --- > drivers/iio/accel/mma8452.c | 309 ++++++++++++++++++++------------------------ > 1 file changed, 141 insertions(+), 168 deletions(-) > > diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c > index eb6e3dc..1b83e52 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; > + break; > + 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_ths_mask is not set here > + ev_regs->ev_count = MMA8452_TRANSIENT_COUNT; > + break; > + default: > + return -EINVAL; > + } > + break; > + default: > + return -EINVAL; > + } > + return 0; could replace 'breaks' with return 0 to save some lines and simplify control flow > +} > + > 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,23 @@ 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: this adds a check for chan-type == IIO_ACCESS, so strictly this would be an unrelated change... > + 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)); > > - return !!(ret & BIT(chan->scan_index + > - chip->ev_cfg_chan_shift)); > + default: > + return -EINVAL; > + } > default: > return -EINVAL; > } > @@ -890,39 +921,33 @@ 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); > default: > return -EINVAL; > } > @@ -934,35 +959,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 +989,41 @@ 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; > > + 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 +1255,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, > }, > }; > > -- Peter Meerwald-Stadler Mobile: +43 664 24 44 418 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html