Add a channel for enabling/disabling the step counter, reading the
number of steps and resetting the counter.
Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
drivers/iio/imu/bmi270/bmi270_core.c | 127 +++++++++++++++++++++++++++++++++++
1 file changed, 127 insertions(+)
diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index a86be5af5ccb1f010f2282ee31c47f284c1bcc86..f09d8dead9df63df5ae8550cf473b5573374955b 100644
--- a/drivers/iio/imu/bmi270/bmi270_core.c
+++ b/drivers/iio/imu/bmi270/bmi270_core.c
@@ -31,6 +31,8 @@
#define BMI270_INT_STATUS_1_REG 0x1d
#define BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK GENMASK(7, 6)
+#define BMI270_SC_OUT_0_REG 0x1e
+
#define BMI270_INTERNAL_STATUS_REG 0x21
#define BMI270_INTERNAL_STATUS_MSG_MSK GENMASK(3, 0)
#define BMI270_INTERNAL_STATUS_MSG_INIT_OK 0x01
@@ -39,6 +41,8 @@
#define BMI270_TEMPERATURE_0_REG 0x22
+#define BMI270_FEAT_PAGE_REG 0x2f
+
#define BMI270_ACC_CONF_REG 0x40
#define BMI270_ACC_CONF_ODR_MSK GENMASK(3, 0)
#define BMI270_ACC_CONF_ODR_100HZ 0x08
@@ -90,6 +94,9 @@
#define BMI270_PWR_CTRL_ACCEL_EN_MSK BIT(2)
#define BMI270_PWR_CTRL_TEMP_EN_MSK BIT(3)
+#define BMI270_STEP_SC26_RST_CNT_MSK BIT(10)
+#define BMI270_STEP_SC26_EN_CNT_MSK BIT(12)
+
/* See datasheet section 4.6.14, Temperature Sensor */
#define BMI270_TEMP_OFFSET 11776
#define BMI270_TEMP_SCALE 1953125
@@ -111,6 +118,7 @@ struct bmi270_data {
struct iio_trigger *trig;
/* Protect device's private data from concurrent access */
struct mutex mutex;
+ int steps_enabled;
/*
* Where IIO_DMA_MINALIGN may be larger than 8 bytes, align to
@@ -282,6 +290,99 @@ static const struct bmi270_odr_item bmi270_odr_table[] = {
},
};
+enum bmi270_feature_reg_id {
+ BMI270_SC_26_REG,
+};
+
+struct bmi270_feature_reg {
+ u8 page;
+ u8 addr;
+};
+
+static const struct bmi270_feature_reg bmi270_feature_regs[] = {
+ [BMI270_SC_26_REG] = {
+ .page = 6,
+ .addr = 0x32,
+ },
+};
+
+static int bmi270_write_feature_reg(struct bmi270_data *data,
+ enum bmi270_feature_reg_id id,
+ u16 val)
+{
+ const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
+ int ret;
+
+ ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
+ if (ret)
+ return ret;
+
+ return regmap_bulk_write(data->regmap, reg->addr, &val, sizeof(val));
+}
+
+static int bmi270_read_feature_reg(struct bmi270_data *data,
+ enum bmi270_feature_reg_id id,
+ u16 *val)
+{
+ const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
+ int ret;
+
+ ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
+ if (ret)
+ return ret;
+
+ return regmap_bulk_read(data->regmap, reg->addr, val, sizeof(*val));
+}
+
+static int bmi270_update_feature_reg(struct bmi270_data *data,
+ enum bmi270_feature_reg_id id,
+ u16 mask, u16 val)
+{
+ u16 reg_val;
+ int ret;
+
+ ret = bmi270_read_feature_reg(data, id, ®_val);
+ if (ret)
+ return ret;
+
+ set_mask_bits(®_val, mask, val);
+
+ return bmi270_write_feature_reg(data, id, reg_val);
+}
+
+static int bmi270_enable_steps(struct bmi270_data *data, int val)
+{
+ int ret;
+
+ guard(mutex)(&data->mutex);
+ if (data->steps_enabled == val)
+ return 0;
+
+ ret = bmi270_update_feature_reg(data, BMI270_SC_26_REG,
+ BMI270_STEP_SC26_EN_CNT_MSK,
+ FIELD_PREP(BMI270_STEP_SC26_EN_CNT_MSK,
+ val ? 1 : 0));
+ if (ret)
+ return ret;
+
+ data->steps_enabled = val;
+ return 0;
+}
+
+static int bmi270_read_steps(struct bmi270_data *data, int *val)
+{
+ __le16 steps_count;
+ int ret;
+
+ ret = regmap_bulk_read(data->regmap, BMI270_SC_OUT_0_REG, &steps_count,
+ sizeof(steps_count));
+ if (ret)
+ return ret;
+
+ *val = sign_extend32(le16_to_cpu(steps_count), 15);
+ return IIO_VAL_INT;
+}
+
static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
{
int i;
@@ -551,6 +652,8 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
struct bmi270_data *data = iio_priv(indio_dev);
switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ return bmi270_read_steps(data, val);
case IIO_CHAN_INFO_RAW:
if (!iio_device_claim_direct(indio_dev))
return -EBUSY;
@@ -571,6 +674,10 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_SAMP_FREQ:
ret = bmi270_get_odr(data, chan->type, val, val2);
return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_ENABLE:
+ scoped_guard(mutex, &data->mutex)
+ *val = data->steps_enabled;
+ return IIO_VAL_INT;
default:
return -EINVAL;
}
@@ -596,6 +703,20 @@ static int bmi270_write_raw(struct iio_dev *indio_dev,
ret = bmi270_set_odr(data, chan->type, val, val2);
iio_device_release_direct(indio_dev);
return ret;
+ case IIO_CHAN_INFO_ENABLE:
+ return bmi270_enable_steps(data, val);
+ case IIO_CHAN_INFO_PROCESSED: {
+ guard(mutex)(&data->mutex);
+
+ if (val || !data->steps_enabled)
+ return -EINVAL;
+
+ /* Clear step counter value */
+ return bmi270_update_feature_reg(data, BMI270_SC_26_REG,
+ BMI270_STEP_SC26_RST_CNT_MSK,
+ FIELD_PREP(BMI270_STEP_SC26_RST_CNT_MSK,
+ 1));
+ }
default:
return -EINVAL;
}
@@ -698,6 +819,12 @@ static const struct iio_chan_spec bmi270_channels[] = {
BIT(IIO_CHAN_INFO_OFFSET),
.scan_index = -1, /* No buffer support */
},
+ {
+ .type = IIO_STEPS,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) |
+ BIT(IIO_CHAN_INFO_PROCESSED),
+ .scan_index = -1, /* No buffer support */
+ },
IIO_CHAN_SOFT_TIMESTAMP(BMI270_SCAN_TIMESTAMP),
};
--
2.49.0Hi Gustavo,
kernel test robot noticed the following build errors:
[auto build test ERROR on b475195fecc79a1a6e7fb0846aaaab0a1a4cb2e6]
url: https://github.com/intel-lab-lkp/linux/commits/Gustavo-Silva/iio-imu-bmi270-add-channel-for-step-counter/20250425-081720
base: b475195fecc79a1a6e7fb0846aaaab0a1a4cb2e6
patch link: https://lore.kernel.org/r/20250424-bmi270-events-v1-1-a6c722673e5f%40gmail.com
patch subject: [PATCH 1/3] iio: imu: bmi270: add channel for step counter
config: csky-randconfig-r071-20250426 (https://download.01.org/0day-ci/archive/20250507/202505071850.e6YE0Jm9-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 12.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250507/202505071850.e6YE0Jm9-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505071850.e6YE0Jm9-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from <command-line>:
In function 'bmi270_update_feature_reg',
inlined from 'bmi270_enable_steps' at drivers/iio/imu/bmi270/bmi270_core.c:361:8,
inlined from 'bmi270_write_raw' at drivers/iio/imu/bmi270/bmi270_core.c:707:10:
>> include/linux/compiler_types.h:557:45: error: call to '__compiletime_assert_399' declared with attribute error: BUILD_BUG failed
557 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:538:25: note: in definition of macro '__compiletime_assert'
538 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:557:9: note: in expansion of macro '_compiletime_assert'
557 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:59:21: note: in expansion of macro 'BUILD_BUG_ON_MSG'
59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
| ^~~~~~~~~~~~~~~~
arch/csky/include/asm/cmpxchg.h:151:17: note: in expansion of macro 'BUILD_BUG'
151 | BUILD_BUG(); \
| ^~~~~~~~~
arch/csky/include/asm/cmpxchg.h:157:10: note: in expansion of macro '__cmpxchg'
157 | (__cmpxchg((ptr), (o), (n), sizeof(*(ptr))))
| ^~~~~~~~~
include/linux/atomic/atomic-arch-fallback.h:55:21: note: in expansion of macro 'arch_cmpxchg'
55 | #define raw_cmpxchg arch_cmpxchg
| ^~~~~~~~~~~~
include/linux/atomic/atomic-arch-fallback.h:192:16: note: in expansion of macro 'raw_cmpxchg'
192 | ___r = raw_cmpxchg((_ptr), ___o, (_new)); \
| ^~~~~~~~~~~
include/linux/atomic/atomic-instrumented.h:4880:9: note: in expansion of macro 'raw_try_cmpxchg'
4880 | raw_try_cmpxchg(__ai_ptr, __ai_oldp, __VA_ARGS__); \
| ^~~~~~~~~~~~~~~
include/linux/bitops.h:367:19: note: in expansion of macro 'try_cmpxchg'
367 | } while (!try_cmpxchg(ptr, &old__, new__)); \
| ^~~~~~~~~~~
drivers/iio/imu/bmi270/bmi270_core.c:348:9: note: in expansion of macro 'set_mask_bits'
348 | set_mask_bits(®_val, mask, val);
| ^~~~~~~~~~~~~
In function 'bmi270_update_feature_reg',
inlined from 'bmi270_write_raw' at drivers/iio/imu/bmi270/bmi270_core.c:715:10:
>> include/linux/compiler_types.h:557:45: error: call to '__compiletime_assert_399' declared with attribute error: BUILD_BUG failed
557 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:538:25: note: in definition of macro '__compiletime_assert'
538 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:557:9: note: in expansion of macro '_compiletime_assert'
557 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:59:21: note: in expansion of macro 'BUILD_BUG_ON_MSG'
59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
| ^~~~~~~~~~~~~~~~
arch/csky/include/asm/cmpxchg.h:151:17: note: in expansion of macro 'BUILD_BUG'
151 | BUILD_BUG(); \
| ^~~~~~~~~
arch/csky/include/asm/cmpxchg.h:157:10: note: in expansion of macro '__cmpxchg'
157 | (__cmpxchg((ptr), (o), (n), sizeof(*(ptr))))
| ^~~~~~~~~
include/linux/atomic/atomic-arch-fallback.h:55:21: note: in expansion of macro 'arch_cmpxchg'
55 | #define raw_cmpxchg arch_cmpxchg
| ^~~~~~~~~~~~
include/linux/atomic/atomic-arch-fallback.h:192:16: note: in expansion of macro 'raw_cmpxchg'
192 | ___r = raw_cmpxchg((_ptr), ___o, (_new)); \
| ^~~~~~~~~~~
include/linux/atomic/atomic-instrumented.h:4880:9: note: in expansion of macro 'raw_try_cmpxchg'
4880 | raw_try_cmpxchg(__ai_ptr, __ai_oldp, __VA_ARGS__); \
| ^~~~~~~~~~~~~~~
include/linux/bitops.h:367:19: note: in expansion of macro 'try_cmpxchg'
367 | } while (!try_cmpxchg(ptr, &old__, new__)); \
| ^~~~~~~~~~~
drivers/iio/imu/bmi270/bmi270_core.c:348:9: note: in expansion of macro 'set_mask_bits'
348 | set_mask_bits(®_val, mask, val);
| ^~~~~~~~~~~~~
vim +/__compiletime_assert_399 +557 include/linux/compiler_types.h
eb5c2d4b45e3d2 Will Deacon 2020-07-21 543
eb5c2d4b45e3d2 Will Deacon 2020-07-21 544 #define _compiletime_assert(condition, msg, prefix, suffix) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 545 __compiletime_assert(condition, msg, prefix, suffix)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 546
eb5c2d4b45e3d2 Will Deacon 2020-07-21 547 /**
eb5c2d4b45e3d2 Will Deacon 2020-07-21 548 * compiletime_assert - break build and emit msg if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 549 * @condition: a compile-time constant condition to check
eb5c2d4b45e3d2 Will Deacon 2020-07-21 550 * @msg: a message to emit if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 551 *
eb5c2d4b45e3d2 Will Deacon 2020-07-21 552 * In tradition of POSIX assert, this macro will break the build if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 553 * supplied condition is *false*, emitting the supplied error message if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 554 * compiler has support to do so.
eb5c2d4b45e3d2 Will Deacon 2020-07-21 555 */
eb5c2d4b45e3d2 Will Deacon 2020-07-21 556 #define compiletime_assert(condition, msg) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 @557 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 558
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, 24 Apr 2025 21:14:50 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:
> Add a channel for enabling/disabling the step counter, reading the
> number of steps and resetting the counter.
>
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
Hi Gustavo,
This is tripping over the somewhat theoretical requirement for
regmap to be provided with DMA safe buffers for bulk accesses.
Jonathan
> ---
> drivers/iio/imu/bmi270/bmi270_core.c | 127 +++++++++++++++++++++++++++++++++++
> 1 file changed, 127 insertions(+)
>
> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> index a86be5af5ccb1f010f2282ee31c47f284c1bcc86..f09d8dead9df63df5ae8550cf473b5573374955b 100644
> --- a/drivers/iio/imu/bmi270/bmi270_core.c
> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> @@ -31,6 +31,8 @@
> /* See datasheet section 4.6.14, Temperature Sensor */
> #define BMI270_TEMP_OFFSET 11776
> #define BMI270_TEMP_SCALE 1953125
> @@ -111,6 +118,7 @@ struct bmi270_data {
> struct iio_trigger *trig;
> /* Protect device's private data from concurrent access */
> struct mutex mutex;
> + int steps_enabled;
Seems a little odd to have a thing called _enabled as an integer.
Probably better as a bool even though that will require slightly more
code to handle read / write.
>
> /*
> * Where IIO_DMA_MINALIGN may be larger than 8 bytes, align to
> @@ -282,6 +290,99 @@ static const struct bmi270_odr_item bmi270_odr_table[] = {
> },
> };
>
> +enum bmi270_feature_reg_id {
> + BMI270_SC_26_REG,
> +};
> +
> +struct bmi270_feature_reg {
> + u8 page;
> + u8 addr;
> +};
> +
> +static const struct bmi270_feature_reg bmi270_feature_regs[] = {
> + [BMI270_SC_26_REG] = {
> + .page = 6,
> + .addr = 0x32,
> + },
> +};
> +
> +static int bmi270_write_feature_reg(struct bmi270_data *data,
> + enum bmi270_feature_reg_id id,
> + u16 val)
> +{
> + const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
> + int ret;
> +
> + ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
> + if (ret)
> + return ret;
> +
> + return regmap_bulk_write(data->regmap, reg->addr, &val, sizeof(val));
For a regmap on top of an SPI bus. I think we are still requiring DMA safe
buffers until we can get confirmation that the API guarantees that isn't
needed. (there is another thread going on where we are trying to get that
confirmation).
That is a pain here because this can run concurrently with buffered
capture and as such I think we can't just put a additional element after
data->data but instead need to mark that new element __aligned(IIO_DMA_MINALIGN)
as well (and add a comment that it can be used concurrently with data->data).
This hole thing is a mess because in reality I think the regmap core is always
bouncing data today. In theory it could sometimes be avoiding copies
and the underlying regmap_spi does require DMA safe buffers. This all relies
on an old discussion where Mark Brown said that we should not assume any
different requirements from the the underlying bus.
> +}
> +
> +static int bmi270_read_feature_reg(struct bmi270_data *data,
> + enum bmi270_feature_reg_id id,
> + u16 *val)
> +{
> + const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
> + int ret;
> +
> + ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
> + if (ret)
> + return ret;
> +
> + return regmap_bulk_read(data->regmap, reg->addr, val, sizeof(*val));
> +}
> +
> +static int bmi270_update_feature_reg(struct bmi270_data *data,
> + enum bmi270_feature_reg_id id,
> + u16 mask, u16 val)
> +{
> + u16 reg_val;
> + int ret;
> +
> + ret = bmi270_read_feature_reg(data, id, ®_val);
> + if (ret)
> + return ret;
> +
> + set_mask_bits(®_val, mask, val);
> +
> + return bmi270_write_feature_reg(data, id, reg_val);
> +}
> +
> +static int bmi270_read_steps(struct bmi270_data *data, int *val)
> +{
> + __le16 steps_count;
> + int ret;
> +
> + ret = regmap_bulk_read(data->regmap, BMI270_SC_OUT_0_REG, &steps_count,
> + sizeof(steps_count));
> + if (ret)
> + return ret;
> +
> + *val = sign_extend32(le16_to_cpu(steps_count), 15);
> + return IIO_VAL_INT;
> +}
> +
> static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
> {
> int i;
> @@ -551,6 +652,8 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
> struct bmi270_data *data = iio_priv(indio_dev);
>
> switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + return bmi270_read_steps(data, val);
> case IIO_CHAN_INFO_RAW:
> if (!iio_device_claim_direct(indio_dev))
> return -EBUSY;
> @@ -571,6 +674,10 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_SAMP_FREQ:
> ret = bmi270_get_odr(data, chan->type, val, val2);
> return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_ENABLE:
> + scoped_guard(mutex, &data->mutex)
> + *val = data->steps_enabled;
What race is this protecting against? Protecting the write is fine because it
is about ensuring we don't race an enable against a clear of the counter.
A race here would I think just give the same as either the race to take the lock
being won by this or not (so not a race as such, just ordering of calls)
So I don't think you need the lock here.
> + return IIO_VAL_INT;
On Sat, Apr 26, 2025 at 02:40:20PM +0100, Jonathan Cameron wrote:
> On Thu, 24 Apr 2025 21:14:50 -0300
> Gustavo Silva <gustavograzs@gmail.com> wrote:
>
> > Add a channel for enabling/disabling the step counter, reading the
> > number of steps and resetting the counter.
> >
> > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
> Hi Gustavo,
>
> This is tripping over the somewhat theoretical requirement for
> regmap to be provided with DMA safe buffers for bulk accesses.
>
> Jonathan
>
Hi Jonathan,
Thanks for the review. I've got a few questions inline.
> > ---
> > drivers/iio/imu/bmi270/bmi270_core.c | 127 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 127 insertions(+)
> >
> > diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> > index a86be5af5ccb1f010f2282ee31c47f284c1bcc86..f09d8dead9df63df5ae8550cf473b5573374955b 100644
> > --- a/drivers/iio/imu/bmi270/bmi270_core.c
> > +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> > @@ -31,6 +31,8 @@
>
> > /* See datasheet section 4.6.14, Temperature Sensor */
> > #define BMI270_TEMP_OFFSET 11776
> > #define BMI270_TEMP_SCALE 1953125
> > @@ -111,6 +118,7 @@ struct bmi270_data {
> > struct iio_trigger *trig;
> > /* Protect device's private data from concurrent access */
> > struct mutex mutex;
> > + int steps_enabled;
>
> Seems a little odd to have a thing called _enabled as an integer.
> Probably better as a bool even though that will require slightly more
> code to handle read / write.
>
I agree that a bool might be more appropriate in this case. I decided to
use an int to keep consistency with other drivers, specifically bma400
and the iio dummy driver.
If you prefer, I'll use a bool here and after this series submit a patch
updating those drivers as well.
>
> >
> > /*
> > * Where IIO_DMA_MINALIGN may be larger than 8 bytes, align to
> > @@ -282,6 +290,99 @@ static const struct bmi270_odr_item bmi270_odr_table[] = {
> > },
> > };
> >
> > +enum bmi270_feature_reg_id {
> > + BMI270_SC_26_REG,
> > +};
> > +
> > +struct bmi270_feature_reg {
> > + u8 page;
> > + u8 addr;
> > +};
> > +
> > +static const struct bmi270_feature_reg bmi270_feature_regs[] = {
> > + [BMI270_SC_26_REG] = {
> > + .page = 6,
> > + .addr = 0x32,
> > + },
> > +};
> > +
> > +static int bmi270_write_feature_reg(struct bmi270_data *data,
> > + enum bmi270_feature_reg_id id,
> > + u16 val)
> > +{
> > + const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
> > + int ret;
> > +
> > + ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_bulk_write(data->regmap, reg->addr, &val, sizeof(val));
>
> For a regmap on top of an SPI bus. I think we are still requiring DMA safe
> buffers until we can get confirmation that the API guarantees that isn't
> needed. (there is another thread going on where we are trying to get that
> confirmation).
>
> That is a pain here because this can run concurrently with buffered
> capture and as such I think we can't just put a additional element after
> data->data but instead need to mark that new element __aligned(IIO_DMA_MINALIGN)
> as well (and add a comment that it can be used concurrently with data->data).
>
Just to clarify, when you say data->data, are you referring to the
bmi270_data::buffer variable? That used to be called 'data' but it was
changed to 'buffer' in commit 16c94de2a.
> This hole thing is a mess because in reality I think the regmap core is always
> bouncing data today. In theory it could sometimes be avoiding copies
> and the underlying regmap_spi does require DMA safe buffers. This all relies
> on an old discussion where Mark Brown said that we should not assume any
> different requirements from the the underlying bus.
>
> > +}
> > +
> > +static int bmi270_read_feature_reg(struct bmi270_data *data,
> > + enum bmi270_feature_reg_id id,
> > + u16 *val)
> > +{
> > + const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
> > + int ret;
> > +
> > + ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_bulk_read(data->regmap, reg->addr, val, sizeof(*val));
> > +}
> > +
> > @@ -551,6 +652,8 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
> > struct bmi270_data *data = iio_priv(indio_dev);
> >
> > switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + return bmi270_read_steps(data, val);
> > case IIO_CHAN_INFO_RAW:
> > if (!iio_device_claim_direct(indio_dev))
> > return -EBUSY;
> > @@ -571,6 +674,10 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > ret = bmi270_get_odr(data, chan->type, val, val2);
> > return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_ENABLE:
> > + scoped_guard(mutex, &data->mutex)
> > + *val = data->steps_enabled;
>
> What race is this protecting against? Protecting the write is fine because it
> is about ensuring we don't race an enable against a clear of the counter.
> A race here would I think just give the same as either the race to take the lock
> being won by this or not (so not a race as such, just ordering of calls)
> So I don't think you need the lock here.
>
Understood. I'll fix it in v2.
> > + return IIO_VAL_INT;
>
>
On Sat, 26 Apr 2025 21:19:13 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:
> On Sat, Apr 26, 2025 at 02:40:20PM +0100, Jonathan Cameron wrote:
> > On Thu, 24 Apr 2025 21:14:50 -0300
> > Gustavo Silva <gustavograzs@gmail.com> wrote:
> >
> > > Add a channel for enabling/disabling the step counter, reading the
> > > number of steps and resetting the counter.
> > >
> > > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
> > Hi Gustavo,
> >
> > This is tripping over the somewhat theoretical requirement for
> > regmap to be provided with DMA safe buffers for bulk accesses.
> >
> > Jonathan
> >
>
> Hi Jonathan,
>
> Thanks for the review. I've got a few questions inline.
>
> > > ---
> > > drivers/iio/imu/bmi270/bmi270_core.c | 127 +++++++++++++++++++++++++++++++++++
> > > 1 file changed, 127 insertions(+)
> > >
> > > diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> > > index a86be5af5ccb1f010f2282ee31c47f284c1bcc86..f09d8dead9df63df5ae8550cf473b5573374955b 100644
> > > --- a/drivers/iio/imu/bmi270/bmi270_core.c
> > > +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> > > @@ -31,6 +31,8 @@
> >
> > > /* See datasheet section 4.6.14, Temperature Sensor */
> > > #define BMI270_TEMP_OFFSET 11776
> > > #define BMI270_TEMP_SCALE 1953125
> > > @@ -111,6 +118,7 @@ struct bmi270_data {
> > > struct iio_trigger *trig;
> > > /* Protect device's private data from concurrent access */
> > > struct mutex mutex;
> > > + int steps_enabled;
> >
> > Seems a little odd to have a thing called _enabled as an integer.
> > Probably better as a bool even though that will require slightly more
> > code to handle read / write.
> >
> I agree that a bool might be more appropriate in this case. I decided to
> use an int to keep consistency with other drivers, specifically bma400
> and the iio dummy driver.
> If you prefer, I'll use a bool here and after this series submit a patch
> updating those drivers as well.
Yes. I think that would be a nice little logical improvement here and in those
drivers. Not particularly critical though!
>
> >
> > >
> > > /*
> > > * Where IIO_DMA_MINALIGN may be larger than 8 bytes, align to
> > > @@ -282,6 +290,99 @@ static const struct bmi270_odr_item bmi270_odr_table[] = {
> > > },
> > > };
> > >
> > > +enum bmi270_feature_reg_id {
> > > + BMI270_SC_26_REG,
> > > +};
> > > +
> > > +struct bmi270_feature_reg {
> > > + u8 page;
> > > + u8 addr;
> > > +};
> > > +
> > > +static const struct bmi270_feature_reg bmi270_feature_regs[] = {
> > > + [BMI270_SC_26_REG] = {
> > > + .page = 6,
> > > + .addr = 0x32,
> > > + },
> > > +};
> > > +
> > > +static int bmi270_write_feature_reg(struct bmi270_data *data,
> > > + enum bmi270_feature_reg_id id,
> > > + u16 val)
> > > +{
> > > + const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
> > > + int ret;
> > > +
> > > + ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return regmap_bulk_write(data->regmap, reg->addr, &val, sizeof(val));
> >
> > For a regmap on top of an SPI bus. I think we are still requiring DMA safe
> > buffers until we can get confirmation that the API guarantees that isn't
> > needed. (there is another thread going on where we are trying to get that
> > confirmation).
> >
> > That is a pain here because this can run concurrently with buffered
> > capture and as such I think we can't just put a additional element after
> > data->data but instead need to mark that new element __aligned(IIO_DMA_MINALIGN)
> > as well (and add a comment that it can be used concurrently with data->data).
> >
> Just to clarify, when you say data->data, are you referring to the
> bmi270_data::buffer variable? That used to be called 'data' but it was
> changed to 'buffer' in commit 16c94de2a.
Yes. The one marked __aligned(IIO_DMA_MINALIGN)
I was looking at old code I guess!
Jonathan
On Fri, Apr 25, 2025 at 3:15 AM Gustavo Silva <gustavograzs@gmail.com> wrote:
>
> Add a channel for enabling/disabling the step counter, reading the
> number of steps and resetting the counter.
Looks ideal from the style and maintaining perspective, thanks for
making the reviewer's job easy! One small comment, but
Reviewed-by: Andy Shevchenko <andy@kernel.org>
...
> + case IIO_CHAN_INFO_PROCESSED: {
> + guard(mutex)(&data->mutex);
> + if (val || !data->steps_enabled)
> + return -EINVAL;
Please, move the val check outside of the lock, no need to lock for
the local data.
> + /* Clear step counter value */
> + return bmi270_update_feature_reg(data, BMI270_SC_26_REG,
> + BMI270_STEP_SC26_RST_CNT_MSK,
> + FIELD_PREP(BMI270_STEP_SC26_RST_CNT_MSK,
> + 1));
> + }
--
With Best Regards,
Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.