Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751979AbdHQLZF convert rfc822-to-8bit (ORCPT ); Thu, 17 Aug 2017 07:25:05 -0400 Received: from mout02.posteo.de ([185.67.36.66]:59584 "EHLO mout02.posteo.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750775AbdHQLZD (ORCPT ); Thu, 17 Aug 2017 07:25:03 -0400 Date: Thu, 17 Aug 2017 13:24:53 +0200 In-Reply-To: <0a512995-3fd5-e38e-73a6-417337515ce6@gmail.com> References: <1502750644-8854-1-git-send-email-harinath922@gmail.com> <0a512995-3fd5-e38e-73a6-417337515ce6@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Subject: Re: [PATCH] iio: accel: mma8452: Bugfix to enbale and allow different events to work parallely. To: Harinath Nampally , Peter Meerwald-Stadler CC: jic23@kernel.org, knaack.h@gmx.de, lars@metafoo.de, gregkh@linuxfoundation.org, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, amsfield22@gmail.com From: Martin Kepplinger Message-ID: <13587F36-588D-4011-8E70-A8892F81887E@posteo.de> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 33158 Lines: 973 Am 17. August 2017 02:39:05 MESZ schrieb Harinath Nampally : >> Hello, >> >> fixes are always welcome, some comments regarding the patch >Thanks for the review. >> >> in subject: typo "enable" >Will fix it. >>> 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)? >I am planning to add 'supported events' field in > >struct mma_chip_info which indicates which chip can have which events. >During initialization in 'mma_chip_info_table' would set this >'supported events' field for each chip. >But I wonder should I add those changes as part of this patch? is it necessary or can it be documentation? And this patch should have been called "v2". please include a persistent version history to v3 of this patch. thanks > >> >>> 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 >>> --- >>> 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 >> >Good catch! Will fix it. >>> + 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 >Sure. >>> +} >>> + >>> 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... >Ok will remove it from this patch. >>> + 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, >>> }, >>> }; >>> >>> > >On 08/16/2017 09:12 AM, Peter Meerwald-Stadler wrote: >> 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 >>> --- >>> 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) >>> retu -- Martin Kepplinger http://martinkepplinger.com sent from mobile