Add support for activity and inactivity events for all axis based on the
threshold, duration and hysteresis value set from the userspace. INT1 pin
is used to interrupt and event is pushed to userspace.
Signed-off-by: Jagath Jog J <[email protected]>
---
drivers/iio/accel/bma400.h | 11 ++
drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
2 files changed, 240 insertions(+)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index bc4641279be3..cbf8035c817e 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -93,6 +93,17 @@
#define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
#define BMA400_ACC_ODR_MIN_HZ 12
+/* Generic interrupts register */
+#define BMA400_GEN1INT_CONFIG0 0x3f
+#define BMA400_GEN2INT_CONFIG0 0x4A
+#define BMA400_GEN_CONFIG1_OFF 0x01
+#define BMA400_GEN_CONFIG2_OFF 0x02
+#define BMA400_GEN_CONFIG3_OFF 0x03
+#define BMA400_GEN_CONFIG31_OFF 0x04
+#define BMA400_INT_GEN1_MSK BIT(2)
+#define BMA400_INT_GEN2_MSK BIT(3)
+#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
+
/*
* BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
* converting to micro values for +-2g range.
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index b6c79cfabaa4..226a5f63d1a6 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -79,6 +79,7 @@ struct bma400_data {
int steps_enabled;
bool step_event_en;
bool activity_event_en;
+ u8 generic_event_en;
/* Correct time stamp alignment */
struct {
__le16 buff[3];
@@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
};
+static const struct iio_event_spec bma400_accel_event[] = {
+ {
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_PERIOD) |
+ BIT(IIO_EV_INFO_HYSTERESIS) |
+ BIT(IIO_EV_INFO_ENABLE),
+ },
+ {
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_PERIOD) |
+ BIT(IIO_EV_INFO_HYSTERESIS) |
+ BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
#define BMA400_ACC_CHANNEL(_index, _axis) { \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
+ .event_spec = bma400_accel_event, \
+ .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
}
#define BMA400_ACTIVITY_CHANNEL(_chan2) { \
@@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
struct bma400_data *data = iio_priv(indio_dev);
switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return FIELD_GET(BMA400_INT_GEN1_MSK,
+ data->generic_event_en);
+ case IIO_EV_DIR_FALLING:
+ return FIELD_GET(BMA400_INT_GEN2_MSK,
+ data->generic_event_en);
+ default:
+ return -EINVAL;
+ }
case IIO_STEPS:
return data->step_event_en;
case IIO_ACTIVITY:
@@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
{
int ret;
struct bma400_data *data = iio_priv(indio_dev);
+ int reg, msk, value, field_value;
switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ reg = BMA400_GEN1INT_CONFIG0;
+ msk = BMA400_INT_GEN1_MSK;
+ value = 2;
+ field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
+ break;
+ case IIO_EV_DIR_FALLING:
+ reg = BMA400_GEN2INT_CONFIG0;
+ msk = BMA400_INT_GEN2_MSK;
+ value = 0;
+ field_value = FIELD_PREP(BMA400_INT_GEN2_MSK, state);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ mutex_lock(&data->mutex);
+ /* Enabling all axis for interrupt evaluation */
+ ret = regmap_write(data->regmap, reg, 0xF8);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ /* OR combination of all axis for interrupt evaluation */
+ ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG1_OFF,
+ value);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ /* Initial value to avoid interrupts while enabling*/
+ ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
+ 0x0A);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ /* Initial duration value to avoid interrupts while enabling*/
+ ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG31_OFF,
+ 0x0F);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
+ msk, field_value);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
+ msk, field_value);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+
+ set_mask_bits(&data->generic_event_en, msk, field_value);
+ return 0;
case IIO_STEPS:
mutex_lock(&data->mutex);
if (!data->steps_enabled) {
@@ -1028,6 +1127,118 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
}
}
+static int get_gen_config_reg(enum iio_event_direction dir)
+{
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ return BMA400_GEN2INT_CONFIG0;
+ case IIO_EV_DIR_RISING:
+ return BMA400_GEN1INT_CONFIG0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int bma400_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret;
+ u8 reg, duration[2];
+
+ reg = get_gen_config_reg(dir);
+ if (reg < 0)
+ return -EINVAL;
+
+ *val2 = 0;
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
+ val);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ return IIO_VAL_INT;
+ case IIO_EV_INFO_PERIOD:
+ mutex_lock(&data->mutex);
+ ret = regmap_bulk_read(data->regmap,
+ reg + BMA400_GEN_CONFIG3_OFF,
+ duration, sizeof(duration));
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ *val = get_unaligned_be16(duration);
+ return IIO_VAL_INT;
+ case IIO_EV_INFO_HYSTERESIS:
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, reg, val);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int bma400_write_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret;
+ u8 reg, duration[2];
+
+ reg = get_gen_config_reg(dir);
+ if (reg < 0)
+ return -EINVAL;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ if (val < 1 || val > 255)
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
+ val);
+ mutex_unlock(&data->mutex);
+ return ret;
+ case IIO_EV_INFO_PERIOD:
+ if (val < 1 || val > 65535)
+ return -EINVAL;
+
+ put_unaligned_be16(val, duration);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_bulk_write(data->regmap,
+ reg + BMA400_GEN_CONFIG3_OFF,
+ duration, sizeof(duration));
+ mutex_unlock(&data->mutex);
+ return ret;
+ case IIO_EV_INFO_HYSTERESIS:
+ if (val < 0 || val > 3)
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, reg,
+ BMA400_GEN_HYST_MSK,
+ FIELD_PREP(BMA400_GEN_HYST_MSK, val));
+ mutex_unlock(&data->mutex);
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
static int bma400_debugfs_reg_access(struct iio_dev *indio_dev,
unsigned int reg,
unsigned int writeval,
@@ -1076,6 +1287,8 @@ static const struct iio_info bma400_info = {
.read_event_config = bma400_read_event_config,
.write_event_config = bma400_write_event_config,
.debugfs_reg_access = bma400_debugfs_reg_access,
+ .write_event_value = bma400_write_event_value,
+ .read_event_value = bma400_read_event_value,
};
static const struct iio_trigger_ops bma400_trigger_ops = {
@@ -1120,6 +1333,7 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
int ret;
__le16 status;
unsigned int act;
+ unsigned int ev_dir = IIO_EV_DIR_NONE;
mutex_lock(&data->mutex);
ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
@@ -1128,6 +1342,21 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
if (ret)
return IRQ_NONE;
+ if (FIELD_GET(BMA400_INT_GEN1_MSK, le16_to_cpu(status)))
+ ev_dir = IIO_EV_DIR_RISING;
+
+ if (FIELD_GET(BMA400_INT_GEN2_MSK, le16_to_cpu(status)))
+ ev_dir = IIO_EV_DIR_FALLING;
+
+ if (ev_dir != IIO_EV_DIR_NONE) {
+ iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_OR_Y_OR_Z,
+ IIO_EV_TYPE_MAG, ev_dir),
+ timestamp);
+ return IRQ_HANDLED;
+ }
+
if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
iio_push_event(indio_dev,
IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
--
2.17.1
Hi Jagath,
url: https://github.com/intel-lab-lkp/linux/commits/Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-randconfig-m001-20220411 (https://download.01.org/0day-ci/archive/20220412/[email protected]/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
drivers/iio/accel/bma400_core.c:1154 bma400_read_event_value() warn: impossible condition '(reg < 0) => (0-255 < 0)'
drivers/iio/accel/bma400_core.c:1202 bma400_write_event_value() warn: impossible condition '(reg < 0) => (0-255 < 0)'
vim +1154 drivers/iio/accel/bma400_core.c
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1142 static int bma400_read_event_value(struct iio_dev *indio_dev,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1143 const struct iio_chan_spec *chan,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1144 enum iio_event_type type,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1145 enum iio_event_direction dir,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1146 enum iio_event_info info,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1147 int *val, int *val2)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1148 {
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1149 struct bma400_data *data = iio_priv(indio_dev);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1150 int ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1151 u8 reg, duration[2];
^^^^^^
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1152
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1153 reg = get_gen_config_reg(dir);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 @1154 if (reg < 0)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1155 return -EINVAL;
Condition is impossible. Also propagate the return? if (ret < 0) return ret;?
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1156
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1157 *val2 = 0;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1158 switch (info) {
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1159 case IIO_EV_INFO_VALUE:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1160 mutex_lock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1161 ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1162 val);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1163 mutex_unlock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1164 if (ret)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1165 return ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1166 return IIO_VAL_INT;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1167 case IIO_EV_INFO_PERIOD:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1168 mutex_lock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1169 ret = regmap_bulk_read(data->regmap,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1170 reg + BMA400_GEN_CONFIG3_OFF,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1171 duration, sizeof(duration));
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1172 mutex_unlock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1173 if (ret)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1174 return ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1175 *val = get_unaligned_be16(duration);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1176 return IIO_VAL_INT;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1177 case IIO_EV_INFO_HYSTERESIS:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1178 mutex_lock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1179 ret = regmap_read(data->regmap, reg, val);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1180 mutex_unlock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1181 if (ret)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1182 return ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1183 *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1184 return IIO_VAL_INT;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1185 default:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1186 return -EINVAL;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1187 }
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1188 }
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1189
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1190 static int bma400_write_event_value(struct iio_dev *indio_dev,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1191 const struct iio_chan_spec *chan,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1192 enum iio_event_type type,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1193 enum iio_event_direction dir,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1194 enum iio_event_info info,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1195 int val, int val2)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1196 {
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1197 struct bma400_data *data = iio_priv(indio_dev);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1198 int ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1199 u8 reg, duration[2];
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1200
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1201 reg = get_gen_config_reg(dir);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 @1202 if (reg < 0)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1203 return -EINVAL;
Same.
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1204
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1205 switch (info) {
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1206 case IIO_EV_INFO_VALUE:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1207 if (val < 1 || val > 255)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1208 return -EINVAL;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1209
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1210 mutex_lock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1211 ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Jagath,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on v5.18-rc2 next-20220411]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: riscv-randconfig-c006-20220411 (https://download.01.org/0day-ci/archive/20220412/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project fe2478d44e4f7f191c43fef629ac7a23d0251e72)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/15ee6de45ed7a028569638c198e170bb98cef4ab
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
git checkout 15ee6de45ed7a028569638c198e170bb98cef4ab
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/iio/accel/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/iio/accel/bma400_core.c:1072:3: error: call to __compiletime_assert_272 declared with 'error' attribute: BUILD_BUG failed
set_mask_bits(&data->generic_event_en, msk, field_value);
^
include/linux/bitops.h:283:11: note: expanded from macro 'set_mask_bits'
} while (cmpxchg(ptr, old__, new__) != old__); \
^
include/linux/atomic/atomic-instrumented.h:1916:2: note: expanded from macro 'cmpxchg'
arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
^
arch/riscv/include/asm/cmpxchg.h:344:23: note: expanded from macro 'arch_cmpxchg'
(__typeof__(*(ptr))) __cmpxchg((ptr), \
^
note: (skipping 4 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
include/linux/compiler_types.h:340:2: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
^
include/linux/compiler_types.h:333:4: note: expanded from macro '__compiletime_assert'
prefix ## suffix(); \
^
<scratch space>:64:1: note: expanded from here
__compiletime_assert_272
^
1 error generated.
vim +/error +1072 drivers/iio/accel/bma400_core.c
998
999 static int bma400_write_event_config(struct iio_dev *indio_dev,
1000 const struct iio_chan_spec *chan,
1001 enum iio_event_type type,
1002 enum iio_event_direction dir, int state)
1003 {
1004 int ret;
1005 struct bma400_data *data = iio_priv(indio_dev);
1006 int reg, msk, value, field_value;
1007
1008 switch (chan->type) {
1009 case IIO_ACCEL:
1010 switch (dir) {
1011 case IIO_EV_DIR_RISING:
1012 reg = BMA400_GEN1INT_CONFIG0;
1013 msk = BMA400_INT_GEN1_MSK;
1014 value = 2;
1015 field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
1016 break;
1017 case IIO_EV_DIR_FALLING:
1018 reg = BMA400_GEN2INT_CONFIG0;
1019 msk = BMA400_INT_GEN2_MSK;
1020 value = 0;
1021 field_value = FIELD_PREP(BMA400_INT_GEN2_MSK, state);
1022 break;
1023 default:
1024 return -EINVAL;
1025 }
1026
1027 mutex_lock(&data->mutex);
1028 /* Enabling all axis for interrupt evaluation */
1029 ret = regmap_write(data->regmap, reg, 0xF8);
1030 if (ret) {
1031 mutex_unlock(&data->mutex);
1032 return ret;
1033 }
1034
1035 /* OR combination of all axis for interrupt evaluation */
1036 ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG1_OFF,
1037 value);
1038 if (ret) {
1039 mutex_unlock(&data->mutex);
1040 return ret;
1041 }
1042
1043 /* Initial value to avoid interrupts while enabling*/
1044 ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
1045 0x0A);
1046 if (ret) {
1047 mutex_unlock(&data->mutex);
1048 return ret;
1049 }
1050
1051 /* Initial duration value to avoid interrupts while enabling*/
1052 ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG31_OFF,
1053 0x0F);
1054 if (ret) {
1055 mutex_unlock(&data->mutex);
1056 return ret;
1057 }
1058
1059 ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
1060 msk, field_value);
1061 if (ret) {
1062 mutex_unlock(&data->mutex);
1063 return ret;
1064 }
1065
1066 ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
1067 msk, field_value);
1068 mutex_unlock(&data->mutex);
1069 if (ret)
1070 return ret;
1071
> 1072 set_mask_bits(&data->generic_event_en, msk, field_value);
1073 return 0;
1074 case IIO_STEPS:
1075 mutex_lock(&data->mutex);
1076 if (!data->steps_enabled) {
1077 ret = regmap_update_bits(data->regmap,
1078 BMA400_INT_CONFIG1_REG,
1079 BMA400_STEP_INT_MSK,
1080 FIELD_PREP(BMA400_STEP_INT_MSK,
1081 1));
1082 if (ret) {
1083 mutex_unlock(&data->mutex);
1084 return ret;
1085 }
1086 data->steps_enabled = 1;
1087 }
1088
1089 ret = regmap_update_bits(data->regmap,
1090 BMA400_INT12_MAP_REG,
1091 BMA400_STEP_INT_MSK,
1092 FIELD_PREP(BMA400_STEP_INT_MSK,
1093 state));
1094 mutex_unlock(&data->mutex);
1095 if (ret)
1096 return ret;
1097 data->step_event_en = state;
1098 return 0;
1099 case IIO_ACTIVITY:
1100 if (!data->step_event_en) {
1101 mutex_lock(&data->mutex);
1102 ret = regmap_update_bits(data->regmap,
1103 BMA400_INT_CONFIG1_REG,
1104 BMA400_STEP_INT_MSK,
1105 FIELD_PREP(BMA400_STEP_INT_MSK,
1106 1));
1107 if (ret) {
1108 mutex_unlock(&data->mutex);
1109 return ret;
1110 }
1111 data->steps_enabled = 1;
1112
1113 ret = regmap_update_bits(data->regmap,
1114 BMA400_INT12_MAP_REG,
1115 BMA400_STEP_INT_MSK,
1116 FIELD_PREP(BMA400_STEP_INT_MSK,
1117 1));
1118 mutex_unlock(&data->mutex);
1119 if (ret)
1120 return ret;
1121 data->step_event_en = 1;
1122 }
1123 data->activity_event_en = state;
1124 return 0;
1125 default:
1126 return -EINVAL;
1127 }
1128 }
1129
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Jagath,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on v5.18-rc2 next-20220412]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: mips-allmodconfig (https://download.01.org/0day-ci/archive/20220412/[email protected]/config)
compiler: mips-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/15ee6de45ed7a028569638c198e170bb98cef4ab
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
git checkout 15ee6de45ed7a028569638c198e170bb98cef4ab
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__cmpxchg_small" [drivers/iio/accel/bma400_core.ko] undefined!
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, 12 Apr 2022 02:01:33 +0530
Jagath Jog J <[email protected]> wrote:
> Add support for activity and inactivity events for all axis based on the
> threshold, duration and hysteresis value set from the userspace. INT1 pin
> is used to interrupt and event is pushed to userspace.
>
> Signed-off-by: Jagath Jog J <[email protected]>
> ---
> drivers/iio/accel/bma400.h | 11 ++
> drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
> 2 files changed, 240 insertions(+)
>
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index bc4641279be3..cbf8035c817e 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -93,6 +93,17 @@
> #define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
> #define BMA400_ACC_ODR_MIN_HZ 12
>
> +/* Generic interrupts register */
> +#define BMA400_GEN1INT_CONFIG0 0x3f
> +#define BMA400_GEN2INT_CONFIG0 0x4A
> +#define BMA400_GEN_CONFIG1_OFF 0x01
> +#define BMA400_GEN_CONFIG2_OFF 0x02
> +#define BMA400_GEN_CONFIG3_OFF 0x03
> +#define BMA400_GEN_CONFIG31_OFF 0x04
> +#define BMA400_INT_GEN1_MSK BIT(2)
> +#define BMA400_INT_GEN2_MSK BIT(3)
> +#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> +
> /*
> * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> * converting to micro values for +-2g range.
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index b6c79cfabaa4..226a5f63d1a6 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -79,6 +79,7 @@ struct bma400_data {
> int steps_enabled;
> bool step_event_en;
> bool activity_event_en;
> + u8 generic_event_en;
> /* Correct time stamp alignment */
> struct {
> __le16 buff[3];
> @@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
> .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> };
>
> +static const struct iio_event_spec bma400_accel_event[] = {
> + {
> + .type = IIO_EV_TYPE_MAG,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_PERIOD) |
> + BIT(IIO_EV_INFO_HYSTERESIS) |
> + BIT(IIO_EV_INFO_ENABLE),
> + },
> + {
> + .type = IIO_EV_TYPE_MAG,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_PERIOD) |
> + BIT(IIO_EV_INFO_HYSTERESIS) |
> + BIT(IIO_EV_INFO_ENABLE),
> + },
> +};
> +
> #define BMA400_ACC_CHANNEL(_index, _axis) { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
> .storagebits = 16, \
> .endianness = IIO_LE, \
> }, \
> + .event_spec = bma400_accel_event, \
> + .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
> }
>
> #define BMA400_ACTIVITY_CHANNEL(_chan2) { \
> @@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> struct bma400_data *data = iio_priv(indio_dev);
>
> switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + return FIELD_GET(BMA400_INT_GEN1_MSK,
> + data->generic_event_en);
> + case IIO_EV_DIR_FALLING:
> + return FIELD_GET(BMA400_INT_GEN2_MSK,
> + data->generic_event_en);
> + default:
> + return -EINVAL;
> + }
> case IIO_STEPS:
> return data->step_event_en;
> case IIO_ACTIVITY:
> @@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> {
> int ret;
> struct bma400_data *data = iio_priv(indio_dev);
> + int reg, msk, value, field_value;
>
> switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + reg = BMA400_GEN1INT_CONFIG0;
> + msk = BMA400_INT_GEN1_MSK;
> + value = 2;
> + field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
Hopefully you can use msk in here and the compiler can tell it's constant...
> + break;
> + case IIO_EV_DIR_FALLING:
> + reg = BMA400_GEN2INT_CONFIG0;
> + msk = BMA400_INT_GEN2_MSK;
> + value = 0;
> + field_value = FIELD_PREP(BMA400_INT_GEN2_MSK, state);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + mutex_lock(&data->mutex);
> + /* Enabling all axis for interrupt evaluation */
> + ret = regmap_write(data->regmap, reg, 0xF8);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + /* OR combination of all axis for interrupt evaluation */
> + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG1_OFF,
> + value);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + /* Initial value to avoid interrupts while enabling*/
> + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> + 0x0A);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + /* Initial duration value to avoid interrupts while enabling*/
> + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG31_OFF,
> + 0x0F);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> + msk, field_value);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> + msk, field_value);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
This whole stack or mutex_unlock() error handling is a good hint that you should
just factor out this case as a separate function then you can use a goto to
deal with the unlock cleanly.
> +
> + set_mask_bits(&data->generic_event_en, msk, field_value);
> + return 0;
> case IIO_STEPS:
> mutex_lock(&data->mutex);
> if (!data->steps_enabled) {
> @@ -1028,6 +1127,118 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> }
> }
>
> +static int get_gen_config_reg(enum iio_event_direction dir)
> +{
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + return BMA400_GEN2INT_CONFIG0;
> + case IIO_EV_DIR_RISING:
> + return BMA400_GEN1INT_CONFIG0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int bma400_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> + u8 reg, duration[2];
> +
> + reg = get_gen_config_reg(dir);
> + if (reg < 0)
> + return -EINVAL;
> +
> + *val2 = 0;
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> + val);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + case IIO_EV_INFO_PERIOD:
> + mutex_lock(&data->mutex);
> + ret = regmap_bulk_read(data->regmap,
> + reg + BMA400_GEN_CONFIG3_OFF,
> + duration, sizeof(duration));
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + *val = get_unaligned_be16(duration);
As well as dma safety question, you could just have used a __be16 for
duration then you can use be16_to_cpu() as you know it is aligned.
> + return IIO_VAL_INT;
> + case IIO_EV_INFO_HYSTERESIS:
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, reg, val);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int bma400_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> + u8 reg, duration[2];
> +
> + reg = get_gen_config_reg(dir);
> + if (reg < 0)
> + return -EINVAL;
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + if (val < 1 || val > 255)
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> + val);
> + mutex_unlock(&data->mutex);
> + return ret;
> + case IIO_EV_INFO_PERIOD:
> + if (val < 1 || val > 65535)
> + return -EINVAL;
> +
> + put_unaligned_be16(val, duration);
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_bulk_write(data->regmap,
> + reg + BMA400_GEN_CONFIG3_OFF,
> + duration, sizeof(duration));
I can't remember if we are safe or not with bulk_writes but at least
in theory we might not be and should be using a dma safe buffer.
Also locking not necessary in various places in here.
> + mutex_unlock(&data->mutex);
> + return ret;
> + case IIO_EV_INFO_HYSTERESIS:
> + if (val < 0 || val > 3)
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap, reg,
> + BMA400_GEN_HYST_MSK,
> + FIELD_PREP(BMA400_GEN_HYST_MSK, val));
> + mutex_unlock(&data->mutex);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
Hello Jonathan,
Thanks for your suggestions, I will fix the locking and unlocking for all
patches in the next series.
Please can you guide me for auto build test error reported by kernel test
robot for set_mask_bits(&data->generic_event_en, msk, field_value);
in this patch.
On Sat, Apr 16, 2022 at 05:55:37PM +0100, Jonathan Cameron wrote:
> On Tue, 12 Apr 2022 02:01:33 +0530
> Jagath Jog J <[email protected]> wrote:
>
> > Add support for activity and inactivity events for all axis based on the
> > threshold, duration and hysteresis value set from the userspace. INT1 pin
> > is used to interrupt and event is pushed to userspace.
> >
> > Signed-off-by: Jagath Jog J <[email protected]>
> > ---
> > drivers/iio/accel/bma400.h | 11 ++
> > drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
> > 2 files changed, 240 insertions(+)
> >
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index bc4641279be3..cbf8035c817e 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -93,6 +93,17 @@
> > #define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
> > #define BMA400_ACC_ODR_MIN_HZ 12
> >
> > +/* Generic interrupts register */
> > +#define BMA400_GEN1INT_CONFIG0 0x3f
> > +#define BMA400_GEN2INT_CONFIG0 0x4A
> > +#define BMA400_GEN_CONFIG1_OFF 0x01
> > +#define BMA400_GEN_CONFIG2_OFF 0x02
> > +#define BMA400_GEN_CONFIG3_OFF 0x03
> > +#define BMA400_GEN_CONFIG31_OFF 0x04
> > +#define BMA400_INT_GEN1_MSK BIT(2)
> > +#define BMA400_INT_GEN2_MSK BIT(3)
> > +#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> > +
> > /*
> > * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> > * converting to micro values for +-2g range.
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index b6c79cfabaa4..226a5f63d1a6 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -79,6 +79,7 @@ struct bma400_data {
> > int steps_enabled;
> > bool step_event_en;
> > bool activity_event_en;
> > + u8 generic_event_en;
> > /* Correct time stamp alignment */
> > struct {
> > __le16 buff[3];
> > @@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
> > .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > };
> >
> > +static const struct iio_event_spec bma400_accel_event[] = {
> > + {
> > + .type = IIO_EV_TYPE_MAG,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD) |
> > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > + {
> > + .type = IIO_EV_TYPE_MAG,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD) |
> > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > +};
> > +
> > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > .type = IIO_ACCEL, \
> > .modified = 1, \
> > @@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
> > .storagebits = 16, \
> > .endianness = IIO_LE, \
> > }, \
> > + .event_spec = bma400_accel_event, \
> > + .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
> > }
> >
> > #define BMA400_ACTIVITY_CHANNEL(_chan2) { \
> > @@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> > struct bma400_data *data = iio_priv(indio_dev);
> >
> > switch (chan->type) {
> > + case IIO_ACCEL:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return FIELD_GET(BMA400_INT_GEN1_MSK,
> > + data->generic_event_en);
> > + case IIO_EV_DIR_FALLING:
> > + return FIELD_GET(BMA400_INT_GEN2_MSK,
> > + data->generic_event_en);
> > + default:
> > + return -EINVAL;
> > + }
> > case IIO_STEPS:
> > return data->step_event_en;
> > case IIO_ACTIVITY:
> > @@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > {
> > int ret;
> > struct bma400_data *data = iio_priv(indio_dev);
> > + int reg, msk, value, field_value;
> >
> > switch (chan->type) {
> > + case IIO_ACCEL:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + reg = BMA400_GEN1INT_CONFIG0;
> > + msk = BMA400_INT_GEN1_MSK;
> > + value = 2;
> > + field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
>
> Hopefully you can use msk in here and the compiler can tell it's constant...
field_value = FIELD_PREP(msk, state);
is this the fix for error reported by kernel test robot?
>
> > + break;
> > + case IIO_EV_DIR_FALLING:
> > + reg = BMA400_GEN2INT_CONFIG0;
> > + msk = BMA400_INT_GEN2_MSK;
> > + value = 0;
> > + field_value = FIELD_PREP(BMA400_INT_GEN2_MSK, state);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + mutex_lock(&data->mutex);
> > + /* Enabling all axis for interrupt evaluation */
> > + ret = regmap_write(data->regmap, reg, 0xF8);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + /* OR combination of all axis for interrupt evaluation */
> > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG1_OFF,
> > + value);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + /* Initial value to avoid interrupts while enabling*/
> > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > + 0x0A);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + /* Initial duration value to avoid interrupts while enabling*/
> > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG31_OFF,
> > + 0x0F);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> > + msk, field_value);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> > + msk, field_value);
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return ret;
>
> This whole stack or mutex_unlock() error handling is a good hint that you should
> just factor out this case as a separate function then you can use a goto to
> deal with the unlock cleanly.
Sure, I will fix the error handling in the proper way in the next patch.
>
> > +
> > + set_mask_bits(&data->generic_event_en, msk, field_value);
> > + return 0;
> > case IIO_STEPS:
> > mutex_lock(&data->mutex);
> > if (!data->steps_enabled) {
> > @@ -1028,6 +1127,118 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > }
> > }
> >
> > +static int get_gen_config_reg(enum iio_event_direction dir)
> > +{
> > + switch (dir) {
> > + case IIO_EV_DIR_FALLING:
> > + return BMA400_GEN2INT_CONFIG0;
> > + case IIO_EV_DIR_RISING:
> > + return BMA400_GEN1INT_CONFIG0;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int bma400_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int *val, int *val2)
> > +{
> > + struct bma400_data *data = iio_priv(indio_dev);
> > + int ret;
> > + u8 reg, duration[2];
> > +
> > + reg = get_gen_config_reg(dir);
> > + if (reg < 0)
> > + return -EINVAL;
> > +
> > + *val2 = 0;
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + mutex_lock(&data->mutex);
> > + ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > + val);
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return ret;
> > + return IIO_VAL_INT;
> > + case IIO_EV_INFO_PERIOD:
> > + mutex_lock(&data->mutex);
> > + ret = regmap_bulk_read(data->regmap,
> > + reg + BMA400_GEN_CONFIG3_OFF,
> > + duration, sizeof(duration));
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return ret;
> > + *val = get_unaligned_be16(duration);
>
> As well as dma safety question, you could just have used a __be16 for
> duration then you can use be16_to_cpu() as you know it is aligned.
For dma safety, do I need to allocate memory by using local kmalloc() or
I can use __be16 local variable for regmap_bulk_read()?
>
> > + return IIO_VAL_INT;
> > + case IIO_EV_INFO_HYSTERESIS:
> > + mutex_lock(&data->mutex);
> > + ret = regmap_read(data->regmap, reg, val);
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return ret;
> > + *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int bma400_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct bma400_data *data = iio_priv(indio_dev);
> > + int ret;
> > + u8 reg, duration[2];
> > +
> > + reg = get_gen_config_reg(dir);
> > + if (reg < 0)
> > + return -EINVAL;
> > +
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + if (val < 1 || val > 255)
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > + val);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_EV_INFO_PERIOD:
> > + if (val < 1 || val > 65535)
> > + return -EINVAL;
> > +
> > + put_unaligned_be16(val, duration);
> > +
> > + mutex_lock(&data->mutex);
> > + ret = regmap_bulk_write(data->regmap,
> > + reg + BMA400_GEN_CONFIG3_OFF,
> > + duration, sizeof(duration));
>
> I can't remember if we are safe or not with bulk_writes but at least
> in theory we might not be and should be using a dma safe buffer.
Here also for regmap_bulk_write() can I allocate the memory locally by using
kmalloc().
>
> Also locking not necessary in various places in here.
I will fix the locking in all the patches in the next series.
>
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_EV_INFO_HYSTERESIS:
> > + if (val < 0 || val > 3)
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = regmap_update_bits(data->regmap, reg,
> > + BMA400_GEN_HYST_MSK,
> > + FIELD_PREP(BMA400_GEN_HYST_MSK, val));
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
>
Thank you,
Jagath
On Tue, 19 Apr 2022 03:39:43 +0530
Jagath Jog J <[email protected]> wrote:
> Hello Jonathan,
>
> Thanks for your suggestions, I will fix the locking and unlocking for all
> patches in the next series.
>
> Please can you guide me for auto build test error reported by kernel test
> robot for set_mask_bits(&data->generic_event_en, msk, field_value);
> in this patch.
>
> On Sat, Apr 16, 2022 at 05:55:37PM +0100, Jonathan Cameron wrote:
> > On Tue, 12 Apr 2022 02:01:33 +0530
> > Jagath Jog J <[email protected]> wrote:
> >
> > > Add support for activity and inactivity events for all axis based on the
> > > threshold, duration and hysteresis value set from the userspace. INT1 pin
> > > is used to interrupt and event is pushed to userspace.
> > >
> > > Signed-off-by: Jagath Jog J <[email protected]>
> > > ---
> > > drivers/iio/accel/bma400.h | 11 ++
> > > drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
> > > 2 files changed, 240 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > index bc4641279be3..cbf8035c817e 100644
> > > --- a/drivers/iio/accel/bma400.h
> > > +++ b/drivers/iio/accel/bma400.h
> > > @@ -93,6 +93,17 @@
> > > #define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
> > > #define BMA400_ACC_ODR_MIN_HZ 12
> > >
> > > +/* Generic interrupts register */
> > > +#define BMA400_GEN1INT_CONFIG0 0x3f
> > > +#define BMA400_GEN2INT_CONFIG0 0x4A
> > > +#define BMA400_GEN_CONFIG1_OFF 0x01
> > > +#define BMA400_GEN_CONFIG2_OFF 0x02
> > > +#define BMA400_GEN_CONFIG3_OFF 0x03
> > > +#define BMA400_GEN_CONFIG31_OFF 0x04
> > > +#define BMA400_INT_GEN1_MSK BIT(2)
> > > +#define BMA400_INT_GEN2_MSK BIT(3)
> > > +#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> > > +
> > > /*
> > > * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> > > * converting to micro values for +-2g range.
> > > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > > index b6c79cfabaa4..226a5f63d1a6 100644
> > > --- a/drivers/iio/accel/bma400_core.c
> > > +++ b/drivers/iio/accel/bma400_core.c
> > > @@ -79,6 +79,7 @@ struct bma400_data {
> > > int steps_enabled;
> > > bool step_event_en;
> > > bool activity_event_en;
> > > + u8 generic_event_en;
> > > /* Correct time stamp alignment */
> > > struct {
> > > __le16 buff[3];
> > > @@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
> > > .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > > };
> > >
> > > +static const struct iio_event_spec bma400_accel_event[] = {
> > > + {
> > > + .type = IIO_EV_TYPE_MAG,
> > > + .dir = IIO_EV_DIR_FALLING,
> > > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > > + BIT(IIO_EV_INFO_PERIOD) |
> > > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > > + BIT(IIO_EV_INFO_ENABLE),
> > > + },
> > > + {
> > > + .type = IIO_EV_TYPE_MAG,
> > > + .dir = IIO_EV_DIR_RISING,
> > > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > > + BIT(IIO_EV_INFO_PERIOD) |
> > > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > > + BIT(IIO_EV_INFO_ENABLE),
> > > + },
> > > +};
> > > +
> > > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > .type = IIO_ACCEL, \
> > > .modified = 1, \
> > > @@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
> > > .storagebits = 16, \
> > > .endianness = IIO_LE, \
> > > }, \
> > > + .event_spec = bma400_accel_event, \
> > > + .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
> > > }
> > >
> > > #define BMA400_ACTIVITY_CHANNEL(_chan2) { \
> > > @@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> > > struct bma400_data *data = iio_priv(indio_dev);
> > >
> > > switch (chan->type) {
> > > + case IIO_ACCEL:
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + return FIELD_GET(BMA400_INT_GEN1_MSK,
> > > + data->generic_event_en);
> > > + case IIO_EV_DIR_FALLING:
> > > + return FIELD_GET(BMA400_INT_GEN2_MSK,
> > > + data->generic_event_en);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > case IIO_STEPS:
> > > return data->step_event_en;
> > > case IIO_ACTIVITY:
> > > @@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > {
> > > int ret;
> > > struct bma400_data *data = iio_priv(indio_dev);
> > > + int reg, msk, value, field_value;
> > >
> > > switch (chan->type) {
> > > + case IIO_ACCEL:
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + reg = BMA400_GEN1INT_CONFIG0;
> > > + msk = BMA400_INT_GEN1_MSK;
> > > + value = 2;
> > > + field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
> >
> > Hopefully you can use msk in here and the compiler can tell it's constant...
>
> field_value = FIELD_PREP(msk, state);
> is this the fix for error reported by kernel test robot?
No. That issue seems to be triggered by the size of parameters passed
to the underlying cmpxchg behind set_mask_bits.
Specifically riscv cmpxchg only support 32 or 64 bit inputs.
https://elixir.bootlin.com/linux/latest/source/arch/riscv/include/asm/cmpxchg.h#L302
Easiest fix is probably just to make generic_event_en 32 bits.
..
> > > +static int bma400_read_event_value(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + int *val, int *val2)
> > > +{
> > > + struct bma400_data *data = iio_priv(indio_dev);
> > > + int ret;
> > > + u8 reg, duration[2];
> > > +
> > > + reg = get_gen_config_reg(dir);
> > > + if (reg < 0)
> > > + return -EINVAL;
> > > +
> > > + *val2 = 0;
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > > + val);
> > > + mutex_unlock(&data->mutex);
> > > + if (ret)
> > > + return ret;
> > > + return IIO_VAL_INT;
> > > + case IIO_EV_INFO_PERIOD:
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_bulk_read(data->regmap,
> > > + reg + BMA400_GEN_CONFIG3_OFF,
> > > + duration, sizeof(duration));
> > > + mutex_unlock(&data->mutex);
> > > + if (ret)
> > > + return ret;
> > > + *val = get_unaligned_be16(duration);
> >
> > As well as dma safety question, you could just have used a __be16 for
> > duration then you can use be16_to_cpu() as you know it is aligned.
>
> For dma safety, do I need to allocate memory by using local kmalloc() or
> I can use __be16 local variable for regmap_bulk_read()?
Ah. i've been unclear there. Was pointing out that if we didn't need
to force alignment larger for DMA safety then using __be16 would have
ensured that this was correctly aligned. However we do need to
force it so either use a kmalloc'd buffer as you suggest or
play games with an aligned buffer in the iio_priv() region.
Note however that we have a bug in IIO currently as we have
been forcing alignment to L1 cache size which is wrong (not enough in some cases
and far too much in others). I'll be posting
some patches to fix that in the next few days.
>
> >
> > > + return IIO_VAL_INT;
> > > + case IIO_EV_INFO_HYSTERESIS:
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_read(data->regmap, reg, val);
> > > + mutex_unlock(&data->mutex);
> > > + if (ret)
> > > + return ret;
> > > + *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
> > > + return IIO_VAL_INT;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + int val, int val2)
> > > +{
> > > + struct bma400_data *data = iio_priv(indio_dev);
> > > + int ret;
> > > + u8 reg, duration[2];
> > > +
> > > + reg = get_gen_config_reg(dir);
> > > + if (reg < 0)
> > > + return -EINVAL;
> > > +
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + if (val < 1 || val > 255)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > > + val);
> > > + mutex_unlock(&data->mutex);
> > > + return ret;
> > > + case IIO_EV_INFO_PERIOD:
> > > + if (val < 1 || val > 65535)
> > > + return -EINVAL;
> > > +
> > > + put_unaligned_be16(val, duration);
> > > +
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_bulk_write(data->regmap,
> > > + reg + BMA400_GEN_CONFIG3_OFF,
> > > + duration, sizeof(duration));
> >
> > I can't remember if we are safe or not with bulk_writes but at least
> > in theory we might not be and should be using a dma safe buffer.
>
> Here also for regmap_bulk_write() can I allocate the memory locally by using
> kmalloc().
Yes though that's usually a pain to handle in comparison with a buffer in iio_priv()
as you have to free it again.
>
> >
> > Also locking not necessary in various places in here.
>
> I will fix the locking in all the patches in the next series.
>
> >
> > > + mutex_unlock(&data->mutex);
> > > + return ret;
> > > + case IIO_EV_INFO_HYSTERESIS:
> > > + if (val < 0 || val > 3)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_update_bits(data->regmap, reg,
> > > + BMA400_GEN_HYST_MSK,
> > > + FIELD_PREP(BMA400_GEN_HYST_MSK, val));
> > > + mutex_unlock(&data->mutex);
> > > + return ret;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> >
>
> Thank you,
> Jagath
Thanks,
Jonathan
Hello Jonathan,
On Sun, Apr 24, 2022 at 9:02 PM Jonathan Cameron <[email protected]> wrote:
>
> On Tue, 19 Apr 2022 03:39:43 +0530
> Jagath Jog J <[email protected]> wrote:
>
> > Hello Jonathan,
> >
> > Thanks for your suggestions, I will fix the locking and unlocking for all
> > patches in the next series.
> >
> > Please can you guide me for auto build test error reported by kernel test
> > robot for set_mask_bits(&data->generic_event_en, msk, field_value);
> > in this patch.
> >
> > On Sat, Apr 16, 2022 at 05:55:37PM +0100, Jonathan Cameron wrote:
> > > On Tue, 12 Apr 2022 02:01:33 +0530
> > > Jagath Jog J <[email protected]> wrote:
> > >
> > > > Add support for activity and inactivity events for all axis based on the
> > > > threshold, duration and hysteresis value set from the userspace. INT1 pin
> > > > is used to interrupt and event is pushed to userspace.
> > > >
> > > > Signed-off-by: Jagath Jog J <[email protected]>
> > > > ---
> > > > drivers/iio/accel/bma400.h | 11 ++
> > > > drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
> > > > 2 files changed, 240 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > > index bc4641279be3..cbf8035c817e 100644
> > > > --- a/drivers/iio/accel/bma400.h
> > > > +++ b/drivers/iio/accel/bma400.h
> > > > @@ -93,6 +93,17 @@
> > > > #define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
> > > > #define BMA400_ACC_ODR_MIN_HZ 12
> > > >
> > > > +/* Generic interrupts register */
> > > > +#define BMA400_GEN1INT_CONFIG0 0x3f
> > > > +#define BMA400_GEN2INT_CONFIG0 0x4A
> > > > +#define BMA400_GEN_CONFIG1_OFF 0x01
> > > > +#define BMA400_GEN_CONFIG2_OFF 0x02
> > > > +#define BMA400_GEN_CONFIG3_OFF 0x03
> > > > +#define BMA400_GEN_CONFIG31_OFF 0x04
> > > > +#define BMA400_INT_GEN1_MSK BIT(2)
> > > > +#define BMA400_INT_GEN2_MSK BIT(3)
> > > > +#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> > > > +
> > > > /*
> > > > * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> > > > * converting to micro values for +-2g range.
> > > > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > > > index b6c79cfabaa4..226a5f63d1a6 100644
> > > > --- a/drivers/iio/accel/bma400_core.c
> > > > +++ b/drivers/iio/accel/bma400_core.c
> > > > @@ -79,6 +79,7 @@ struct bma400_data {
> > > > int steps_enabled;
> > > > bool step_event_en;
> > > > bool activity_event_en;
> > > > + u8 generic_event_en;
> > > > /* Correct time stamp alignment */
> > > > struct {
> > > > __le16 buff[3];
> > > > @@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
> > > > .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > > > };
> > > >
> > > > +static const struct iio_event_spec bma400_accel_event[] = {
> > > > + {
> > > > + .type = IIO_EV_TYPE_MAG,
> > > > + .dir = IIO_EV_DIR_FALLING,
> > > > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > > > + BIT(IIO_EV_INFO_PERIOD) |
> > > > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > > > + BIT(IIO_EV_INFO_ENABLE),
> > > > + },
> > > > + {
> > > > + .type = IIO_EV_TYPE_MAG,
> > > > + .dir = IIO_EV_DIR_RISING,
> > > > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > > > + BIT(IIO_EV_INFO_PERIOD) |
> > > > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > > > + BIT(IIO_EV_INFO_ENABLE),
> > > > + },
> > > > +};
> > > > +
> > > > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > > .type = IIO_ACCEL, \
> > > > .modified = 1, \
> > > > @@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
> > > > .storagebits = 16, \
> > > > .endianness = IIO_LE, \
> > > > }, \
> > > > + .event_spec = bma400_accel_event, \
> > > > + .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
> > > > }
> > > >
> > > > #define BMA400_ACTIVITY_CHANNEL(_chan2) { \
> > > > @@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> > > > struct bma400_data *data = iio_priv(indio_dev);
> > > >
> > > > switch (chan->type) {
> > > > + case IIO_ACCEL:
> > > > + switch (dir) {
> > > > + case IIO_EV_DIR_RISING:
> > > > + return FIELD_GET(BMA400_INT_GEN1_MSK,
> > > > + data->generic_event_en);
> > > > + case IIO_EV_DIR_FALLING:
> > > > + return FIELD_GET(BMA400_INT_GEN2_MSK,
> > > > + data->generic_event_en);
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > case IIO_STEPS:
> > > > return data->step_event_en;
> > > > case IIO_ACTIVITY:
> > > > @@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > > {
> > > > int ret;
> > > > struct bma400_data *data = iio_priv(indio_dev);
> > > > + int reg, msk, value, field_value;
> > > >
> > > > switch (chan->type) {
> > > > + case IIO_ACCEL:
> > > > + switch (dir) {
> > > > + case IIO_EV_DIR_RISING:
> > > > + reg = BMA400_GEN1INT_CONFIG0;
> > > > + msk = BMA400_INT_GEN1_MSK;
> > > > + value = 2;
> > > > + field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
> > >
> > > Hopefully you can use msk in here and the compiler can tell it's constant...
> >
> > field_value = FIELD_PREP(msk, state);
> > is this the fix for error reported by kernel test robot?
> No. That issue seems to be triggered by the size of parameters passed
> to the underlying cmpxchg behind set_mask_bits.
> Specifically riscv cmpxchg only support 32 or 64 bit inputs.
> https://elixir.bootlin.com/linux/latest/source/arch/riscv/include/asm/cmpxchg.h#L302
>
> Easiest fix is probably just to make generic_event_en 32 bits.
In the patch series v4 I have declared generic_event_en as unsigned int.
> ..
>
> > > > +static int bma400_read_event_value(struct iio_dev *indio_dev,
> > > > + const struct iio_chan_spec *chan,
> > > > + enum iio_event_type type,
> > > > + enum iio_event_direction dir,
> > > > + enum iio_event_info info,
> > > > + int *val, int *val2)
> > > > +{
> > > > + struct bma400_data *data = iio_priv(indio_dev);
> > > > + int ret;
> > > > + u8 reg, duration[2];
> > > > +
> > > > + reg = get_gen_config_reg(dir);
> > > > + if (reg < 0)
> > > > + return -EINVAL;
> > > > +
> > > > + *val2 = 0;
> > > > + switch (info) {
> > > > + case IIO_EV_INFO_VALUE:
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > > > + val);
> > > > + mutex_unlock(&data->mutex);
> > > > + if (ret)
> > > > + return ret;
> > > > + return IIO_VAL_INT;
> > > > + case IIO_EV_INFO_PERIOD:
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_bulk_read(data->regmap,
> > > > + reg + BMA400_GEN_CONFIG3_OFF,
> > > > + duration, sizeof(duration));
> > > > + mutex_unlock(&data->mutex);
> > > > + if (ret)
> > > > + return ret;
> > > > + *val = get_unaligned_be16(duration);
> > >
> > > As well as dma safety question, you could just have used a __be16 for
> > > duration then you can use be16_to_cpu() as you know it is aligned.
> >
> > For dma safety, do I need to allocate memory by using local kmalloc() or
> > I can use __be16 local variable for regmap_bulk_read()?
>
> Ah. i've been unclear there. Was pointing out that if we didn't need
> to force alignment larger for DMA safety then using __be16 would have
> ensured that this was correctly aligned. However we do need to
> force it so either use a kmalloc'd buffer as you suggest or
> play games with an aligned buffer in the iio_priv() region.
In v4 series, I have used a __be16 buffer in the iio_priv() region for
duration so
that it can be used for both regmap_bulk_read() and regmap_bulk_write().
>
> Note however that we have a bug in IIO currently as we have
> been forcing alignment to L1 cache size which is wrong (not enough in some cases
> and far too much in others). I'll be posting
> some patches to fix that in the next few days.
>
>
> >
> > >
> > > > + return IIO_VAL_INT;
> > > > + case IIO_EV_INFO_HYSTERESIS:
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_read(data->regmap, reg, val);
> > > > + mutex_unlock(&data->mutex);
> > > > + if (ret)
> > > > + return ret;
> > > > + *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
> > > > + return IIO_VAL_INT;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > > > +
> > > > +static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > > + const struct iio_chan_spec *chan,
> > > > + enum iio_event_type type,
> > > > + enum iio_event_direction dir,
> > > > + enum iio_event_info info,
> > > > + int val, int val2)
> > > > +{
> > > > + struct bma400_data *data = iio_priv(indio_dev);
> > > > + int ret;
> > > > + u8 reg, duration[2];
> > > > +
> > > > + reg = get_gen_config_reg(dir);
> > > > + if (reg < 0)
> > > > + return -EINVAL;
> > > > +
> > > > + switch (info) {
> > > > + case IIO_EV_INFO_VALUE:
> > > > + if (val < 1 || val > 255)
> > > > + return -EINVAL;
> > > > +
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > > > + val);
> > > > + mutex_unlock(&data->mutex);
> > > > + return ret;
> > > > + case IIO_EV_INFO_PERIOD:
> > > > + if (val < 1 || val > 65535)
> > > > + return -EINVAL;
> > > > +
> > > > + put_unaligned_be16(val, duration);
> > > > +
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_bulk_write(data->regmap,
> > > > + reg + BMA400_GEN_CONFIG3_OFF,
> > > > + duration, sizeof(duration));
> > >
> > > I can't remember if we are safe or not with bulk_writes but at least
> > > in theory we might not be and should be using a dma safe buffer.
> >
> > Here also for regmap_bulk_write() can I allocate the memory locally by using
> > kmalloc().
>
> Yes though that's usually a pain to handle in comparison with a buffer in iio_priv()
> as you have to free it again.
>
> >
> > >
> > > Also locking not necessary in various places in here.
> >
> > I will fix the locking in all the patches in the next series.
> >
> > >
> > > > + mutex_unlock(&data->mutex);
> > > > + return ret;
> > > > + case IIO_EV_INFO_HYSTERESIS:
> > > > + if (val < 0 || val > 3)
> > > > + return -EINVAL;
> > > > +
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_update_bits(data->regmap, reg,
> > > > + BMA400_GEN_HYST_MSK,
> > > > + FIELD_PREP(BMA400_GEN_HYST_MSK, val));
> > > > + mutex_unlock(&data->mutex);
> > > > + return ret;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > > > +
> > >
> >
> > Thank you,
> > Jagath
>
> Thanks,
>
> Jonathan
>
>