[PATCH 0/3] BMI270: Add support for step counter and motion events

[PATCH 0/3] BMI270: Add support for step counter and motion events

[Gustavo Silva]

This series adds support for step counter and motion events using
interrupts in the BMI270 driver.

The step counter can be enabled, disabled, and configured with a
watermark, all from userspace.

Any-motion and no-motion events are generated by detecting changes
in the acceleration slope on each axis.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
Gustavo Silva (3):
      iio: imu: bmi270: add channel for step counter
      iio: imu: bmi270: add step counter watermark event
      iio: imu: bmi270: add support for motion events

 drivers/iio/imu/bmi270/bmi270_core.c | 523 ++++++++++++++++++++++++++++++++++-
 1 file changed, 520 insertions(+), 3 deletions(-)
---
base-commit: b475195fecc79a1a6e7fb0846aaaab0a1a4cb2e6
change-id: 20250424-bmi270-events-74c6ef5f4243

Best regards,
-- 
Gustavo Silva <gustavograzs@gmail.com>

[PATCH 1/3] iio: imu: bmi270: add channel for step counter

[Gustavo Silva]

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, &reg_val);
+	if (ret)
+		return ret;
+
+	set_mask_bits(&reg_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.0

Re: [PATCH 1/3] iio: imu: bmi270: add channel for step counter

[Andy Shevchenko]

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

Re: [PATCH 1/3] iio: imu: bmi270: add channel for step counter

[Jonathan Cameron]

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, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	set_mask_bits(&reg_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;

Re: [PATCH 1/3] iio: imu: bmi270: add channel for step counter

[Gustavo Silva]

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;
> 
> 

Re: [PATCH 1/3] iio: imu: bmi270: add channel for step counter

[Jonathan Cameron]

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

Re: [PATCH 1/3] iio: imu: bmi270: add channel for step counter

[kernel test robot]

Hi 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(&reg_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(&reg_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

[PATCH 2/3] iio: imu: bmi270: add step counter watermark event

[Gustavo Silva]

Add support for generating events when the step counter reaches the
configurable watermark.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 drivers/iio/imu/bmi270/bmi270_core.c | 168 ++++++++++++++++++++++++++++++++++-
 1 file changed, 165 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index f09d8dead9df63df5ae8550cf473b5573374955b..07a24ed9a4edabeafd98a746ba09469f9e41c38a 100644
--- a/drivers/iio/imu/bmi270/bmi270_core.c
+++ b/drivers/iio/imu/bmi270/bmi270_core.c
@@ -8,6 +8,7 @@
 #include <linux/regmap.h>
 #include <linux/units.h>
 
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger.h>
@@ -28,6 +29,9 @@
 #define BMI270_ACCEL_X_REG				0x0c
 #define BMI270_ANG_VEL_X_REG				0x12
 
+#define BMI270_INT_STATUS_0_REG				0x1c
+#define BMI270_INT_STATUS_0_STEP_CNT_MSK		BIT(1)
+
 #define BMI270_INT_STATUS_1_REG				0x1d
 #define BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK		GENMASK(7, 6)
 
@@ -74,6 +78,10 @@
 #define BMI270_INT_LATCH_REG				0x55
 #define BMI270_INT_LATCH_REG_MSK			BIT(0)
 
+#define BMI270_INT1_MAP_FEAT_REG			0x56
+#define BMI270_INT2_MAP_FEAT_REG			0x57
+#define BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK		BIT(1)
+
 #define BMI270_INT_MAP_DATA_REG				0x58
 #define BMI270_INT_MAP_DATA_DRDY_INT1_MSK		BIT(2)
 #define BMI270_INT_MAP_DATA_DRDY_INT2_MSK		BIT(6)
@@ -94,6 +102,7 @@
 #define BMI270_PWR_CTRL_ACCEL_EN_MSK			BIT(2)
 #define BMI270_PWR_CTRL_TEMP_EN_MSK			BIT(3)
 
+#define BMI270_STEP_SC26_WTRMRK_MSK			GENMASK(9, 0)
 #define BMI270_STEP_SC26_RST_CNT_MSK			BIT(10)
 #define BMI270_STEP_SC26_EN_CNT_MSK			BIT(12)
 
@@ -119,6 +128,7 @@ struct bmi270_data {
 	 /* Protect device's private data from concurrent access */
 	struct mutex mutex;
 	int steps_enabled;
+	unsigned int feature_events;
 
 	/*
 	 * Where IIO_DMA_MINALIGN may be larger than 8 bytes, align to
@@ -383,6 +393,42 @@ static int bmi270_read_steps(struct bmi270_data *data, int *val)
 	return IIO_VAL_INT;
 }
 
+static int bmi270_int_map_reg(enum bmi270_irq_pin pin)
+{
+	switch (pin) {
+	case BMI270_IRQ_INT1:
+		return BMI270_INT1_MAP_FEAT_REG;
+	case BMI270_IRQ_INT2:
+		return BMI270_INT2_MAP_FEAT_REG;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi270_step_wtrmrk_en(struct bmi270_data *data, bool state)
+{
+	int ret, reg, field_value;
+
+	guard(mutex)(&data->mutex);
+	if (!data->steps_enabled)
+		return -EINVAL;
+
+	reg = bmi270_int_map_reg(data->irq_pin);
+	if (reg < 0)
+		return -EINVAL;
+
+	field_value = FIELD_PREP(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, state);
+	ret = regmap_update_bits(data->regmap, reg,
+				 BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
+				 field_value);
+	if (ret)
+		return ret;
+
+	set_mask_bits(&data->feature_events,
+		      BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, field_value);
+	return 0;
+}
+
 static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
 {
 	int i;
@@ -539,19 +585,32 @@ static irqreturn_t bmi270_irq_thread_handler(int irq, void *private)
 {
 	struct iio_dev *indio_dev = private;
 	struct bmi270_data *data = iio_priv(indio_dev);
-	unsigned int status;
+	unsigned int status0, status1;
+	s64 timestamp = iio_get_time_ns(indio_dev);
 	int ret;
 
 	scoped_guard(mutex, &data->mutex) {
+		ret = regmap_read(data->regmap, BMI270_INT_STATUS_0_REG,
+				  &status0);
+		if (ret)
+			return IRQ_NONE;
+
 		ret = regmap_read(data->regmap, BMI270_INT_STATUS_1_REG,
-				  &status);
+				  &status1);
 		if (ret)
 			return IRQ_NONE;
 	}
 
-	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status))
+	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status1))
 		iio_trigger_poll_nested(data->trig);
 
+	if (FIELD_GET(BMI270_INT_STATUS_0_STEP_CNT_MSK, status0))
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
+							     IIO_NO_MOD,
+							     IIO_EV_TYPE_CHANGE,
+							     IIO_EV_DIR_NONE),
+			       timestamp);
+
 	return IRQ_HANDLED;
 }
 
@@ -761,10 +820,111 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static int bmi270_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir, bool state)
+{
+	struct bmi270_data *data = iio_priv(indio_dev);
+
+	switch (type) {
+	case IIO_EV_TYPE_CHANGE:
+		return bmi270_step_wtrmrk_en(data, state);
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int bmi270_read_event_config(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir)
+{
+	struct bmi270_data *data = iio_priv(indio_dev);
+
+	guard(mutex)(&data->mutex);
+
+	switch (chan->type) {
+	case IIO_STEPS:
+		return FIELD_GET(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
+				 data->feature_events) ? 1 : 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi270_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 bmi270_data *data = iio_priv(indio_dev);
+	unsigned int raw;
+
+	guard(mutex)(&data->mutex);
+
+	switch (type) {
+	case IIO_EV_TYPE_CHANGE:
+		if (!in_range(val, 0, 20461))
+			return -EINVAL;
+
+		raw = val / 20;
+		return bmi270_update_feature_reg(data, BMI270_SC_26_REG,
+						 BMI270_STEP_SC26_WTRMRK_MSK,
+						 FIELD_PREP(BMI270_STEP_SC26_WTRMRK_MSK,
+							    raw));
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi270_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 bmi270_data *data = iio_priv(indio_dev);
+	unsigned int raw;
+	u16 reg_val;
+	int ret;
+
+	guard(mutex)(&data->mutex);
+
+	switch (type) {
+	case IIO_EV_TYPE_CHANGE:
+		ret = bmi270_read_feature_reg(data, BMI270_SC_26_REG, &reg_val);
+		if (ret)
+			return ret;
+
+		raw = FIELD_GET(BMI270_STEP_SC26_WTRMRK_MSK, reg_val);
+		*val = raw * 20;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_event_spec bmi270_step_wtrmrk_event = {
+	.type = IIO_EV_TYPE_CHANGE,
+	.dir = IIO_EV_DIR_NONE,
+	.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+			       BIT(IIO_EV_INFO_VALUE),
+};
+
 static const struct iio_info bmi270_info = {
 	.read_raw = bmi270_read_raw,
 	.write_raw = bmi270_write_raw,
 	.read_avail = bmi270_read_avail,
+	.write_event_config = bmi270_write_event_config,
+	.read_event_config = bmi270_read_event_config,
+	.write_event_value = bmi270_write_event_value,
+	.read_event_value = bmi270_read_event_value,
 };
 
 #define BMI270_ACCEL_CHANNEL(_axis) {				\
@@ -824,6 +984,8 @@ static const struct iio_chan_spec bmi270_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) |
 				      BIT(IIO_CHAN_INFO_PROCESSED),
 		.scan_index = -1, /* No buffer support */
+		.event_spec = &bmi270_step_wtrmrk_event,
+		.num_event_specs = 1,
 	},
 	IIO_CHAN_SOFT_TIMESTAMP(BMI270_SCAN_TIMESTAMP),
 };

-- 
2.49.0

Re: [PATCH 2/3] iio: imu: bmi270: add step counter watermark event

[Andy Shevchenko]

On Fri, Apr 25, 2025 at 3:15 AM Gustavo Silva <gustavograzs@gmail.com> wrote:
>
> Add support for generating events when the step counter reaches the
> configurable watermark.

With the below being addressed,
Reviewed-by: Andy Shevchenko <andy@kernel.org>

...

> +static int bmi270_write_event_config(struct iio_dev *indio_dev,
> +                                    const struct iio_chan_spec *chan,
> +                                    enum iio_event_type type,
> +                                    enum iio_event_direction dir, bool state)
> +{
> +       struct bmi270_data *data = iio_priv(indio_dev);
> +
> +       switch (type) {
> +       case IIO_EV_TYPE_CHANGE:
> +               return bmi270_step_wtrmrk_en(data, state);
> +       default:
> +               return -EINVAL;
> +       }

> +
> +       return 0;

Dead code.

> +}

...

> +       switch (type) {
> +       case IIO_EV_TYPE_CHANGE:

> +               if (!in_range(val, 0, 20461))

I prefer that + 1 to be separated and the value defined.

(0, _FOO + 1)

> +                       return -EINVAL;

> +               raw = val / 20;

Needs a comment.  Is this in the Datasheet? Then reference to the
section / table / formula would be nice to have.

> +               return bmi270_update_feature_reg(data, BMI270_SC_26_REG,
> +                                                BMI270_STEP_SC26_WTRMRK_MSK,
> +                                                FIELD_PREP(BMI270_STEP_SC26_WTRMRK_MSK,
> +                                                           raw));
> +       default:
> +               return -EINVAL;
> +       }

...

> +               raw = FIELD_GET(BMI270_STEP_SC26_WTRMRK_MSK, reg_val);
> +               *val = raw * 20;

Same.

> +               return IIO_VAL_INT;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] iio: imu: bmi270: add step counter watermark event

[Gustavo Silva]

On Fri, Apr 25, 2025 at 07:33:20AM +0300, Andy Shevchenko wrote:
> On Fri, Apr 25, 2025 at 3:15 AM Gustavo Silva <gustavograzs@gmail.com> wrote:
> >
> > Add support for generating events when the step counter reaches the
> > configurable watermark.
> 
> With the below being addressed,
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
>

Hi Andy,

Thanks for the review.

> ...
> 
> > +static int bmi270_write_event_config(struct iio_dev *indio_dev,
> > +                                    const struct iio_chan_spec *chan,
> > +                                    enum iio_event_type type,
> > +                                    enum iio_event_direction dir, bool state)
> > +{
> > +       struct bmi270_data *data = iio_priv(indio_dev);
> > +
> > +       switch (type) {
> > +       case IIO_EV_TYPE_CHANGE:
> > +               return bmi270_step_wtrmrk_en(data, state);
> > +       default:
> > +               return -EINVAL;
> > +       }
> 
> > +
> > +       return 0;
> 
> Dead code.
> 
Ack.

> > +}
> 
> ...
> 
> > +       switch (type) {
> > +       case IIO_EV_TYPE_CHANGE:
> 
> > +               if (!in_range(val, 0, 20461))
> 
> I prefer that + 1 to be separated and the value defined.
> 
> (0, _FOO + 1)
> 
Ack.

> > +                       return -EINVAL;
> 
> > +               raw = val / 20;
> 
> Needs a comment.  Is this in the Datasheet? Then reference to the
> section / table / formula would be nice to have.
> 
According to the datasheet, there's a factor of 20 to the step counter
watermark level.
I'll add a comment referencing that section of the datasheet in v2.

> > +               return bmi270_update_feature_reg(data, BMI270_SC_26_REG,
> > +                                                BMI270_STEP_SC26_WTRMRK_MSK,
> > +                                                FIELD_PREP(BMI270_STEP_SC26_WTRMRK_MSK,
> > +                                                           raw));
> > +       default:
> > +               return -EINVAL;
> > +       }
> 
> ...
> 
> > +               raw = FIELD_GET(BMI270_STEP_SC26_WTRMRK_MSK, reg_val);
> > +               *val = raw * 20;
> 
> Same.
> 
Ack.

> > +               return IIO_VAL_INT;
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 2/3] iio: imu: bmi270: add step counter watermark event

[Jonathan Cameron]

On Thu, 24 Apr 2025 21:14:51 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> Add support for generating events when the step counter reaches the
> configurable watermark.
> 
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>

Main thing in here is I think the event type isn't the right one.

> @@ -119,6 +128,7 @@ struct bmi270_data {
>  	 /* Protect device's private data from concurrent access */
>  	struct mutex mutex;
>  	int steps_enabled;
> +	unsigned int feature_events;

Why do we need this rather than just checking the register?


> +
> +static int bmi270_step_wtrmrk_en(struct bmi270_data *data, bool state)
> +{
> +	int ret, reg, field_value;
> +
> +	guard(mutex)(&data->mutex);
> +	if (!data->steps_enabled)
> +		return -EINVAL;
> +
> +	reg = bmi270_int_map_reg(data->irq_pin);
> +	if (reg < 0)
> +		return -EINVAL;
> +
> +	field_value = FIELD_PREP(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, state);
> +	ret = regmap_update_bits(data->regmap, reg,
> +				 BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
> +				 field_value);
> +	if (ret)
> +		return ret;
> +
> +	set_mask_bits(&data->feature_events,
> +		      BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, field_value);

Given we wrote the register, why do we need a cached value?  Can't we just read
it back again (or rely on a regmap cache for it if enabled in this driver)

> +	return 0;
> +}
> +
>  static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
>  {
>  	int i;
> @@ -539,19 +585,32 @@ static irqreturn_t bmi270_irq_thread_handler(int irq, void *private)
>  {
>  	struct iio_dev *indio_dev = private;
>  	struct bmi270_data *data = iio_priv(indio_dev);
> -	unsigned int status;
> +	unsigned int status0, status1;
> +	s64 timestamp = iio_get_time_ns(indio_dev);
>  	int ret;
>  
>  	scoped_guard(mutex, &data->mutex) {
> +		ret = regmap_read(data->regmap, BMI270_INT_STATUS_0_REG,
> +				  &status0);
> +		if (ret)
> +			return IRQ_NONE;
> +
>  		ret = regmap_read(data->regmap, BMI270_INT_STATUS_1_REG,
> -				  &status);
> +				  &status1);
>  		if (ret)
>  			return IRQ_NONE;
>  	}
>  
> -	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status))
> +	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status1))
>  		iio_trigger_poll_nested(data->trig);
>  
> +	if (FIELD_GET(BMI270_INT_STATUS_0_STEP_CNT_MSK, status0))
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
> +							     IIO_NO_MOD,
why use IIO_MOD_EVENT_CODE() if not modified?

> +							     IIO_EV_TYPE_CHANGE,
> +							     IIO_EV_DIR_NONE),
As below. This looks like a rising threshold event.

Change tends to be for things like activity detection (walking/standing etc)
 
> +			       timestamp);
> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -761,10 +820,111 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
>  	}
>  }
>
> +
> +static const struct iio_event_spec bmi270_step_wtrmrk_event = {
> +	.type = IIO_EV_TYPE_CHANGE,

Change would be a per step event.
IIUC this is a rising threshold.

> +	.dir = IIO_EV_DIR_NONE,
> +	.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> +			       BIT(IIO_EV_INFO_VALUE),
> +};

Re: [PATCH 2/3] iio: imu: bmi270: add step counter watermark event

[Gustavo Silva]

On Sat, Apr 26, 2025 at 02:47:39PM +0100, Jonathan Cameron wrote:
> On Thu, 24 Apr 2025 21:14:51 -0300
> Gustavo Silva <gustavograzs@gmail.com> wrote:
> 
> > Add support for generating events when the step counter reaches the
> > configurable watermark.
> > 
> > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
> 
> Main thing in here is I think the event type isn't the right one.
>

Hi Jonathan,

Thanks for the review.
I think the answers to your questions in this patch come down to me
trying to keep this driver consistency with the bmi323 driver, since
the two devices are very similar.
However I have no objections to change the event type if you think it
is appropriate.

> > @@ -119,6 +128,7 @@ struct bmi270_data {
> >  	 /* Protect device's private data from concurrent access */
> >  	struct mutex mutex;
> >  	int steps_enabled;
> > +	unsigned int feature_events;
> 
> Why do we need this rather than just checking the register?
> 
Not really needed, I just tried to keep it similar to the bmi323 driver.

> 
> > +
> > +static int bmi270_step_wtrmrk_en(struct bmi270_data *data, bool state)
> > +{
> > +	int ret, reg, field_value;
> > +
> > +	guard(mutex)(&data->mutex);
> > +	if (!data->steps_enabled)
> > +		return -EINVAL;
> > +
> > +	reg = bmi270_int_map_reg(data->irq_pin);
> > +	if (reg < 0)
> > +		return -EINVAL;
> > +
> > +	field_value = FIELD_PREP(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, state);
> > +	ret = regmap_update_bits(data->regmap, reg,
> > +				 BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
> > +				 field_value);
> > +	if (ret)
> > +		return ret;
> > +
> > +	set_mask_bits(&data->feature_events,
> > +		      BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, field_value);
> 
> Given we wrote the register, why do we need a cached value?  Can't we just read
> it back again (or rely on a regmap cache for it if enabled in this driver)
>
Ditto.

> > +	return 0;
> > +}
> > +
> >  static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
> >  {
> >  	int i;
> > @@ -539,19 +585,32 @@ static irqreturn_t bmi270_irq_thread_handler(int irq, void *private)
> >  {
> >  	struct iio_dev *indio_dev = private;
> >  	struct bmi270_data *data = iio_priv(indio_dev);
> > -	unsigned int status;
> > +	unsigned int status0, status1;
> > +	s64 timestamp = iio_get_time_ns(indio_dev);
> >  	int ret;
> >  
> >  	scoped_guard(mutex, &data->mutex) {
> > +		ret = regmap_read(data->regmap, BMI270_INT_STATUS_0_REG,
> > +				  &status0);
> > +		if (ret)
> > +			return IRQ_NONE;
> > +
> >  		ret = regmap_read(data->regmap, BMI270_INT_STATUS_1_REG,
> > -				  &status);
> > +				  &status1);
> >  		if (ret)
> >  			return IRQ_NONE;
> >  	}
> >  
> > -	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status))
> > +	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status1))
> >  		iio_trigger_poll_nested(data->trig);
> >  
> > +	if (FIELD_GET(BMI270_INT_STATUS_0_STEP_CNT_MSK, status0))
> > +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
> > +							     IIO_NO_MOD,
> why use IIO_MOD_EVENT_CODE() if not modified?
> 
My bad, I blindly followed what is implemented in the bmi323 driver.
I'll fix it in v2.

> > +							     IIO_EV_TYPE_CHANGE,
> > +							     IIO_EV_DIR_NONE),
> As below. This looks like a rising threshold event.
> 
> Change tends to be for things like activity detection (walking/standing etc)
>  
Using rising threshold event is fine by me, but then shouldn't we
update the bmi323 driver as well?

> > +			       timestamp);
> > +
> >  	return IRQ_HANDLED;
> >  }
> >  
> > @@ -761,10 +820,111 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
> >  	}
> >  }
> >
> > +
> > +static const struct iio_event_spec bmi270_step_wtrmrk_event = {
> > +	.type = IIO_EV_TYPE_CHANGE,
> 
> Change would be a per step event.
> IIUC this is a rising threshold.
>
Yeah, if the watermark level is configured to N steps, an event is
generated every time the step counter counts N steps.

> > +	.dir = IIO_EV_DIR_NONE,
> > +	.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > +			       BIT(IIO_EV_INFO_VALUE),
> > +};
> 
> 

Re: [PATCH 2/3] iio: imu: bmi270: add step counter watermark event

[Jonathan Cameron]

On Sat, 26 Apr 2025 21:57:21 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> On Sat, Apr 26, 2025 at 02:47:39PM +0100, Jonathan Cameron wrote:
> > On Thu, 24 Apr 2025 21:14:51 -0300
> > Gustavo Silva <gustavograzs@gmail.com> wrote:
> >   
> > > Add support for generating events when the step counter reaches the
> > > configurable watermark.
> > > 
> > > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>  
> > 
> > Main thing in here is I think the event type isn't the right one.
> >  
> 
> Hi Jonathan,
> 
> Thanks for the review.
> I think the answers to your questions in this patch come down to me
> trying to keep this driver consistency with the bmi323 driver, since
> the two devices are very similar.
> However I have no objections to change the event type if you think it
> is appropriate.

Yeah. From the discussion with Lothar, it's clear we have some inconsistency
on these event types :(  Anyhow, I may well be wrong - see below.

> 
> > > @@ -119,6 +128,7 @@ struct bmi270_data {
> > >  	 /* Protect device's private data from concurrent access */
> > >  	struct mutex mutex;
> > >  	int steps_enabled;
> > > +	unsigned int feature_events;  
> > 
> > Why do we need this rather than just checking the register?
> >   
> Not really needed, I just tried to keep it similar to the bmi323 driver.

Generally I'd prefer drives to use regmap caching to get benefits of caching everything
appropriate rather than use their own local caches.


> > > +	return 0;
> > > +}
> > > +
> > >  static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
> > >  {
> > >  	int i;
> > > @@ -539,19 +585,32 @@ static irqreturn_t bmi270_irq_thread_handler(int irq, void *private)
> > >  {
> > >  	struct iio_dev *indio_dev = private;
> > >  	struct bmi270_data *data = iio_priv(indio_dev);
> > > -	unsigned int status;
> > > +	unsigned int status0, status1;
> > > +	s64 timestamp = iio_get_time_ns(indio_dev);
> > >  	int ret;
> > >  
> > >  	scoped_guard(mutex, &data->mutex) {
> > > +		ret = regmap_read(data->regmap, BMI270_INT_STATUS_0_REG,
> > > +				  &status0);
> > > +		if (ret)
> > > +			return IRQ_NONE;
> > > +
> > >  		ret = regmap_read(data->regmap, BMI270_INT_STATUS_1_REG,
> > > -				  &status);
> > > +				  &status1);
> > >  		if (ret)
> > >  			return IRQ_NONE;
> > >  	}
> > >  
> > > -	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status))
> > > +	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status1))
> > >  		iio_trigger_poll_nested(data->trig);
> > >  
> > > +	if (FIELD_GET(BMI270_INT_STATUS_0_STEP_CNT_MSK, status0))
> > > +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
> > > +							     IIO_NO_MOD,  
> > why use IIO_MOD_EVENT_CODE() if not modified?
> >   
> My bad, I blindly followed what is implemented in the bmi323 driver.
> I'll fix it in v2.

It's not wrong as such, just that we have a more specific macro for this case.
> 
> > > +							     IIO_EV_TYPE_CHANGE,
> > > +							     IIO_EV_DIR_NONE),  
> > As below. This looks like a rising threshold event.
> > 
> > Change tends to be for things like activity detection (walking/standing etc)

I got this wrong.  Forgot about how this ABI was defined until reading it earlier
today for the discussion with Lothar!

> >    
> Using rising threshold event is fine by me, but then shouldn't we
> update the bmi323 driver as well?

If it is an event that occurs every step?  In that case CHANGE is correct.
If it is an event on a particular threshold of steps being passed.  I.e.
1000 steps, then a threshold is appropriate.   Vs every 1000 steps
in which case change is appropriate.  Seems from below comment it is
every N so this is fine as is. 

> 
> > > +			       timestamp);
> > > +
> > >  	return IRQ_HANDLED;
> > >  }
> > >  
> > > @@ -761,10 +820,111 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
> > >  	}
> > >  }
> > >
> > > +
> > > +static const struct iio_event_spec bmi270_step_wtrmrk_event = {
> > > +	.type = IIO_EV_TYPE_CHANGE,  
> > 
> > Change would be a per step event.
> > IIUC this is a rising threshold.
> >  
> Yeah, if the watermark level is configured to N steps, an event is
> generated every time the step counter counts N steps.

This is fine then as change.  My mistake!

Jonathan



> 
> > > +	.dir = IIO_EV_DIR_NONE,
> > > +	.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > > +			       BIT(IIO_EV_INFO_VALUE),
> > > +};  
> > 
> >   

[PATCH 3/3] iio: imu: bmi270: add support for motion events

[Gustavo Silva]

Add support for any-motion/no-motion events based on acceleration slope
on each axis. Threshold and duration can be configured from userspace.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 drivers/iio/imu/bmi270/bmi270_core.c | 230 ++++++++++++++++++++++++++++++++++-
 1 file changed, 229 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index 07a24ed9a4edabeafd98a746ba09469f9e41c38a..57734f9cf5906aa77b7a14dfba793559a817c1e7 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_0_REG				0x1c
 #define BMI270_INT_STATUS_0_STEP_CNT_MSK		BIT(1)
+#define BMI270_INT_STATUS_0_NOMOTION_MSK		BIT(5)
+#define BMI270_INT_STATUS_0_MOTION_MSK			BIT(6)
 
 #define BMI270_INT_STATUS_1_REG				0x1d
 #define BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK		GENMASK(7, 6)
@@ -81,6 +83,8 @@
 #define BMI270_INT1_MAP_FEAT_REG			0x56
 #define BMI270_INT2_MAP_FEAT_REG			0x57
 #define BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK		BIT(1)
+#define BMI270_INT_MAP_FEAT_NOMOTION_MSK		BIT(5)
+#define BMI270_INT_MAP_FEAT_ANYMOTION_MSK		BIT(6)
 
 #define BMI270_INT_MAP_DATA_REG				0x58
 #define BMI270_INT_MAP_DATA_DRDY_INT1_MSK		BIT(2)
@@ -106,10 +110,25 @@
 #define BMI270_STEP_SC26_RST_CNT_MSK			BIT(10)
 #define BMI270_STEP_SC26_EN_CNT_MSK			BIT(12)
 
+#define BMI270_FEAT_MOTION_DURATION_MSK			GENMASK(12, 0)
+#define BMI270_FEAT_MOTION_XYZ_MSK			GENMASK(15, 13)
+#define BMI270_FEAT_MOTION_THRESHOLD_MSK		GENMASK(10, 0)
+#define BMI270_FEAT_MOTION_OUT_CONF_MSK			GENMASK(14, 11)
+#define BMI270_FEAT_MOTION_ENABLE_MSK			BIT(15)
+
+#define BMI270_MOTION_XYZ_MSK				GENMASK(2, 0)
+
+#define BMI270_MOTION_THRES_SCALE			GENMASK(10, 0)
+#define BMI270_MOTION_DURAT_SCALE			50
+
 /* See datasheet section 4.6.14, Temperature Sensor */
 #define BMI270_TEMP_OFFSET				11776
 #define BMI270_TEMP_SCALE				1953125
 
+#define BMI270_INT_MICRO_TO_RAW(val, val2, scale) ((val) * (scale) + \
+						  ((val2) * (scale)) / MEGA)
+#define BMI270_RAW_TO_MICRO(raw, scale) ((((raw) % (scale)) * MEGA) / scale)
+
 #define BMI260_INIT_DATA_FILE "bmi260-init-data.fw"
 #define BMI270_INIT_DATA_FILE "bmi270-init-data.fw"
 
@@ -301,6 +320,13 @@ static const struct  bmi270_odr_item bmi270_odr_table[] = {
 };
 
 enum bmi270_feature_reg_id {
+	/* Page 1 registers */
+	BMI270_ANYMO1_REG,
+	BMI270_ANYMO2_REG,
+	/* Page 2 registers */
+	BMI270_NOMO1_REG,
+	BMI270_NOMO2_REG,
+	/* Page 6 registers */
 	BMI270_SC_26_REG,
 };
 
@@ -310,6 +336,22 @@ struct bmi270_feature_reg {
 };
 
 static const struct bmi270_feature_reg bmi270_feature_regs[] = {
+	[BMI270_ANYMO1_REG] = {
+		.page = 1,
+		.addr = 0x3c,
+	},
+	[BMI270_ANYMO2_REG] = {
+		.page = 1,
+		.addr = 0x3e,
+	},
+	[BMI270_NOMO1_REG] = {
+		.page = 2,
+		.addr = 0x30,
+	},
+	[BMI270_NOMO2_REG] = {
+		.page = 2,
+		.addr = 0x32,
+	},
 	[BMI270_SC_26_REG] = {
 		.page = 6,
 		.addr = 0x32,
@@ -426,6 +468,72 @@ static int bmi270_step_wtrmrk_en(struct bmi270_data *data, bool state)
 
 	set_mask_bits(&data->feature_events,
 		      BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, field_value);
+
+	return 0;
+}
+
+static int bmi270_motion_config_reg(enum iio_event_direction dir)
+{
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return BMI270_ANYMO1_REG;
+	case IIO_EV_DIR_FALLING:
+		return BMI270_NOMO1_REG;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi270_motion_event_en(struct bmi270_data *data,
+				  enum iio_event_direction dir, bool state)
+{
+	int ret, config1, config2, irq_reg;
+	int irq_msk, irq_field_val;
+
+	irq_reg = bmi270_int_map_reg(data->irq_pin);
+	if (irq_reg < 0)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		config1 = BMI270_ANYMO1_REG;
+		config2 = BMI270_ANYMO2_REG;
+		irq_msk = BMI270_INT_MAP_FEAT_ANYMOTION_MSK;
+		irq_field_val = FIELD_PREP(BMI270_INT_MAP_FEAT_ANYMOTION_MSK,
+					   state);
+		break;
+	case IIO_EV_DIR_FALLING:
+		config1 = BMI270_NOMO1_REG;
+		config2 = BMI270_NOMO2_REG;
+		irq_msk = BMI270_INT_MAP_FEAT_NOMOTION_MSK;
+		irq_field_val = FIELD_PREP(BMI270_INT_MAP_FEAT_NOMOTION_MSK,
+					   state);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	guard(mutex)(&data->mutex);
+	ret = bmi270_update_feature_reg(data, config1,
+					BMI270_FEAT_MOTION_XYZ_MSK,
+					FIELD_PREP(BMI270_FEAT_MOTION_XYZ_MSK,
+						   state ? BMI270_MOTION_XYZ_MSK : 0));
+	if (ret)
+		return ret;
+
+	ret = bmi270_update_feature_reg(data, config2,
+					BMI270_FEAT_MOTION_ENABLE_MSK,
+					FIELD_PREP(BMI270_FEAT_MOTION_ENABLE_MSK,
+						   state));
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(data->regmap, irq_reg, irq_msk, irq_field_val);
+	if (ret)
+		return ret;
+
+	set_mask_bits(&data->feature_events, irq_msk, irq_field_val);
+
 	return 0;
 }
 
@@ -604,6 +712,20 @@ static irqreturn_t bmi270_irq_thread_handler(int irq, void *private)
 	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status1))
 		iio_trigger_poll_nested(data->trig);
 
+	if (FIELD_GET(BMI270_INT_STATUS_0_MOTION_MSK, status0))
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							     IIO_MOD_X_OR_Y_OR_Z,
+							     IIO_EV_TYPE_MAG,
+							     IIO_EV_DIR_RISING),
+			       timestamp);
+
+	if (FIELD_GET(BMI270_INT_STATUS_0_NOMOTION_MSK, status0))
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							     IIO_MOD_X_OR_Y_OR_Z,
+							     IIO_EV_TYPE_MAG,
+							     IIO_EV_DIR_FALLING),
+			       timestamp);
+
 	if (FIELD_GET(BMI270_INT_STATUS_0_STEP_CNT_MSK, status0))
 		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
 							     IIO_NO_MOD,
@@ -820,6 +942,20 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static IIO_CONST_ATTR(in_accel_mag_value_available, "[0.0 0.00049 1.0]");
+
+static IIO_CONST_ATTR(in_accel_mag_period_available, "[0.0 0.02 162.0]");
+
+static struct attribute *bmi270_event_attributes[] = {
+	&iio_const_attr_in_accel_mag_value_available.dev_attr.attr,
+	&iio_const_attr_in_accel_mag_period_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group bmi270_event_attribute_group = {
+	.attrs = bmi270_event_attributes,
+};
+
 static int bmi270_write_event_config(struct iio_dev *indio_dev,
 				     const struct iio_chan_spec *chan,
 				     enum iio_event_type type,
@@ -828,6 +964,8 @@ static int bmi270_write_event_config(struct iio_dev *indio_dev,
 	struct bmi270_data *data = iio_priv(indio_dev);
 
 	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		return bmi270_motion_event_en(data, dir, state);
 	case IIO_EV_TYPE_CHANGE:
 		return bmi270_step_wtrmrk_en(data, state);
 	default:
@@ -847,6 +985,17 @@ static int bmi270_read_event_config(struct iio_dev *indio_dev,
 	guard(mutex)(&data->mutex);
 
 	switch (chan->type) {
+	case IIO_ACCEL:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return FIELD_GET(BMI270_INT_MAP_FEAT_ANYMOTION_MSK,
+					 data->feature_events) ? 1 : 0;
+		case IIO_EV_DIR_FALLING:
+			return FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK,
+					 data->feature_events) ? 1 : 0;
+		default:
+			return -EINVAL;
+		}
 	case IIO_STEPS:
 		return FIELD_GET(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
 				 data->feature_events) ? 1 : 0;
@@ -864,10 +1013,42 @@ static int bmi270_write_event_value(struct iio_dev *indio_dev,
 {
 	struct bmi270_data *data = iio_priv(indio_dev);
 	unsigned int raw;
+	int reg;
 
 	guard(mutex)(&data->mutex);
 
 	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		reg = bmi270_motion_config_reg(dir);
+		if (reg < 0)
+			return -EINVAL;
+
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			if (!in_range(val, 0, 1))
+				return -EINVAL;
+
+			raw = BMI270_INT_MICRO_TO_RAW(val, val2,
+						      BMI270_MOTION_THRES_SCALE);
+
+			return bmi270_update_feature_reg(data, reg,
+				BMI270_FEAT_MOTION_THRESHOLD_MSK,
+				FIELD_PREP(BMI270_FEAT_MOTION_THRESHOLD_MSK,
+					   raw));
+		case IIO_EV_INFO_PERIOD:
+			if (!in_range(val, 0, 163))
+				return -EINVAL;
+
+			raw = BMI270_INT_MICRO_TO_RAW(val, val2,
+						      BMI270_MOTION_DURAT_SCALE);
+
+			return bmi270_update_feature_reg(data, reg,
+				BMI270_FEAT_MOTION_DURATION_MSK,
+				FIELD_PREP(BMI270_FEAT_MOTION_DURATION_MSK,
+					   raw));
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_CHANGE:
 		if (!in_range(val, 0, 20461))
 			return -EINVAL;
@@ -891,12 +1072,39 @@ static int bmi270_read_event_value(struct iio_dev *indio_dev,
 {
 	struct bmi270_data *data = iio_priv(indio_dev);
 	unsigned int raw;
+	int ret, reg;
 	u16 reg_val;
-	int ret;
 
 	guard(mutex)(&data->mutex);
 
 	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		reg = bmi270_motion_config_reg(dir);
+		if (reg < 0)
+			return -EINVAL;
+
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = bmi270_read_feature_reg(data, reg, &reg_val);
+			if (ret)
+				return ret;
+
+			raw = FIELD_GET(BMI270_FEAT_MOTION_THRESHOLD_MSK, reg_val);
+			*val = raw / BMI270_MOTION_THRES_SCALE;
+			*val2 = BMI270_RAW_TO_MICRO(raw, BMI270_MOTION_THRES_SCALE);
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_EV_INFO_PERIOD:
+			ret = bmi270_read_feature_reg(data, reg, &reg_val);
+			if (ret)
+				return ret;
+
+			raw = FIELD_GET(BMI270_FEAT_MOTION_DURATION_MSK, reg_val);
+			*val = raw / BMI270_MOTION_DURAT_SCALE;
+			*val2 = BMI270_RAW_TO_MICRO(raw, BMI270_MOTION_DURAT_SCALE);
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_CHANGE:
 		ret = bmi270_read_feature_reg(data, BMI270_SC_26_REG, &reg_val);
 		if (ret)
@@ -917,6 +1125,23 @@ static const struct iio_event_spec bmi270_step_wtrmrk_event = {
 			       BIT(IIO_EV_INFO_VALUE),
 };
 
+static const struct iio_event_spec bmi270_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_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_ENABLE),
+	},
+};
+
 static const struct iio_info bmi270_info = {
 	.read_raw = bmi270_read_raw,
 	.write_raw = bmi270_write_raw,
@@ -925,6 +1150,7 @@ static const struct iio_info bmi270_info = {
 	.read_event_config = bmi270_read_event_config,
 	.write_event_value = bmi270_write_event_value,
 	.read_event_value = bmi270_read_event_value,
+	.event_attrs = &bmi270_event_attribute_group,
 };
 
 #define BMI270_ACCEL_CHANNEL(_axis) {				\
@@ -944,6 +1170,8 @@ static const struct iio_info bmi270_info = {
 		.storagebits = 16,				\
 		.endianness = IIO_LE,				\
 	},	                                                \
+	.event_spec = bmi270_accel_event,			\
+	.num_event_specs = ARRAY_SIZE(bmi270_accel_event),	\
 }
 
 #define BMI270_ANG_VEL_CHANNEL(_axis) {				\

-- 
2.49.0

Re: [PATCH 3/3] iio: imu: bmi270: add support for motion events

[Andy Shevchenko]

On Fri, Apr 25, 2025 at 3:15 AM Gustavo Silva <gustavograzs@gmail.com> wrote:
>
> Add support for any-motion/no-motion events based on acceleration slope
> on each axis. Threshold and duration can be configured from userspace.

I'm wondering if you are using --histogram diff algo when preparing
the patches. If not, please do so for the next version.

...

> +       irq_reg = bmi270_int_map_reg(data->irq_pin);
> +       if (irq_reg < 0)
> +               return -EINVAL;

Why is the error code shadowed?

...

> +               case IIO_EV_INFO_PERIOD:
> +                       if (!in_range(val, 0, 163))

162 + 1

> +                               return -EINVAL;
> +
> +                       raw = BMI270_INT_MICRO_TO_RAW(val, val2,
> +                                                     BMI270_MOTION_DURAT_SCALE);
> +
> +                       return bmi270_update_feature_reg(data, reg,
> +                               BMI270_FEAT_MOTION_DURATION_MSK,
> +                               FIELD_PREP(BMI270_FEAT_MOTION_DURATION_MSK,
> +                                          raw));

...

> +       case IIO_EV_TYPE_MAG:
> +               reg = bmi270_motion_config_reg(dir);
> +               if (reg < 0)
> +                       return -EINVAL;

Shadowed error code, why?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] iio: imu: bmi270: add support for motion events

[Gustavo Silva]

On Fri, Apr 25, 2025 at 08:25:00AM +0300, Andy Shevchenko wrote:
> On Fri, Apr 25, 2025 at 3:15 AM Gustavo Silva <gustavograzs@gmail.com> wrote:
> >
> > Add support for any-motion/no-motion events based on acceleration slope
> > on each axis. Threshold and duration can be configured from userspace.
> 
> I'm wondering if you are using --histogram diff algo when preparing
> the patches. If not, please do so for the next version.
> 
Yeah, I am indeed using the histogram algorithm.

> ...
> 
> > +       irq_reg = bmi270_int_map_reg(data->irq_pin);
> > +       if (irq_reg < 0)
> > +               return -EINVAL;
> 
> Why is the error code shadowed?
> 
I might have forgotten to update this line. Will fix it in v2.

> ...
> 
> > +               case IIO_EV_INFO_PERIOD:
> > +                       if (!in_range(val, 0, 163))
> 
> 162 + 1
> 
Ack.

> > +                               return -EINVAL;
> > +
> > +                       raw = BMI270_INT_MICRO_TO_RAW(val, val2,
> > +                                                     BMI270_MOTION_DURAT_SCALE);
> > +
> > +                       return bmi270_update_feature_reg(data, reg,
> > +                               BMI270_FEAT_MOTION_DURATION_MSK,
> > +                               FIELD_PREP(BMI270_FEAT_MOTION_DURATION_MSK,
> > +                                          raw));
> 
> ...
> 
> > +       case IIO_EV_TYPE_MAG:
> > +               reg = bmi270_motion_config_reg(dir);
> > +               if (reg < 0)
> > +                       return -EINVAL;
> 
> Shadowed error code, why?
> 

Same as above. Will fix in v2.

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 3/3] iio: imu: bmi270: add support for motion events

[Jonathan Cameron]

On Thu, 24 Apr 2025 21:14:52 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> Add support for any-motion/no-motion events based on acceleration slope
> on each axis. Threshold and duration can be configured from userspace.
Hi Gustavo,

These events are a bit of a pain (as Lothar +CC has been discovering recently).

The detection of activity / motion is the (slightly) easier one, though here it sounds
like an adaptive threshold event. Despite wording it has nothing to do with the
slope that I can tell as no reference to dividing by number of samples / time
or similar.  It's just a difference from an estimate of a last state that was interesting
(where there was considered to be motion)

So I think this is best described as an adaptive threshold? But on a magnitude...
Similar to,
in_accel_x|y|z_adaptive_thresh_rising_*
so we probably need to define
in_accel_x|y|z_adaptive_mag_rising_*

Though it has separate channel enables so
in_accel_x_adaptive_mag_rising_*
etc and report an or event as we don't know which axis tripped it (as you
did in this patch).

So no-motion.  The challenge Lothar ran into recently was how we make it
clear what is going on wrt to x&y&z and with separate enables.  We concluded
in the end there was little use for a no motion type detector if we didn't
enable it on all axes.  Otherwise we have to do x&y, x&z an y&z event channels
and event codes just to describe what is enabled.

This time it does seem to be rate of change, so you need a 'pseudo' (made
up additional) channel for x&y&z and then have

in_accel_x&y&z_roc_rising

I can't figure out where deta_t is coming from however so the scaling
of that is non obvious.

Given that means you need some significant changes in how this looks I haven't
reviewed the code in detail this time.

I was discussing with Lothar the other day that we need better documentation
for how to map from datasheet descriptions of events to actual events.
How different devices detect 'motion/nomotion' seems to vary a lot, so it
will need a fairly complex description.

Jonathan


> 
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
> ---
>  drivers/iio/imu/bmi270/bmi270_core.c | 230 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 229 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> index 07a24ed9a4edabeafd98a746ba09469f9e41c38a..57734f9cf5906aa77b7a14dfba793559a817c1e7 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_0_REG				0x1c
>  #define BMI270_INT_STATUS_0_STEP_CNT_MSK		BIT(1)
> +#define BMI270_INT_STATUS_0_NOMOTION_MSK		BIT(5)
> +#define BMI270_INT_STATUS_0_MOTION_MSK			BIT(6)
>  
>  #define BMI270_INT_STATUS_1_REG				0x1d
>  #define BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK		GENMASK(7, 6)
> @@ -81,6 +83,8 @@
>  #define BMI270_INT1_MAP_FEAT_REG			0x56
>  #define BMI270_INT2_MAP_FEAT_REG			0x57
>  #define BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK		BIT(1)
> +#define BMI270_INT_MAP_FEAT_NOMOTION_MSK		BIT(5)
> +#define BMI270_INT_MAP_FEAT_ANYMOTION_MSK		BIT(6)
>  
>  #define BMI270_INT_MAP_DATA_REG				0x58
>  #define BMI270_INT_MAP_DATA_DRDY_INT1_MSK		BIT(2)
> @@ -106,10 +110,25 @@
>  #define BMI270_STEP_SC26_RST_CNT_MSK			BIT(10)
>  #define BMI270_STEP_SC26_EN_CNT_MSK			BIT(12)
>  
> +#define BMI270_FEAT_MOTION_DURATION_MSK			GENMASK(12, 0)
> +#define BMI270_FEAT_MOTION_XYZ_MSK			GENMASK(15, 13)
> +#define BMI270_FEAT_MOTION_THRESHOLD_MSK		GENMASK(10, 0)
> +#define BMI270_FEAT_MOTION_OUT_CONF_MSK			GENMASK(14, 11)
> +#define BMI270_FEAT_MOTION_ENABLE_MSK			BIT(15)
> +
> +#define BMI270_MOTION_XYZ_MSK				GENMASK(2, 0)
> +
> +#define BMI270_MOTION_THRES_SCALE			GENMASK(10, 0)
> +#define BMI270_MOTION_DURAT_SCALE			50
> +
>  /* See datasheet section 4.6.14, Temperature Sensor */
>  #define BMI270_TEMP_OFFSET				11776
>  #define BMI270_TEMP_SCALE				1953125
>  
> +#define BMI270_INT_MICRO_TO_RAW(val, val2, scale) ((val) * (scale) + \
> +						  ((val2) * (scale)) / MEGA)
> +#define BMI270_RAW_TO_MICRO(raw, scale) ((((raw) % (scale)) * MEGA) / scale)
> +
>  #define BMI260_INIT_DATA_FILE "bmi260-init-data.fw"
>  #define BMI270_INIT_DATA_FILE "bmi270-init-data.fw"
>  
> @@ -301,6 +320,13 @@ static const struct  bmi270_odr_item bmi270_odr_table[] = {
>  };
>  
>  enum bmi270_feature_reg_id {
> +	/* Page 1 registers */
> +	BMI270_ANYMO1_REG,
> +	BMI270_ANYMO2_REG,
> +	/* Page 2 registers */
> +	BMI270_NOMO1_REG,
> +	BMI270_NOMO2_REG,
> +	/* Page 6 registers */
>  	BMI270_SC_26_REG,
>  };
>  
> @@ -310,6 +336,22 @@ struct bmi270_feature_reg {
>  };
>  
>  static const struct bmi270_feature_reg bmi270_feature_regs[] = {
> +	[BMI270_ANYMO1_REG] = {
> +		.page = 1,
> +		.addr = 0x3c,
> +	},
> +	[BMI270_ANYMO2_REG] = {
> +		.page = 1,
> +		.addr = 0x3e,
> +	},
> +	[BMI270_NOMO1_REG] = {
> +		.page = 2,
> +		.addr = 0x30,
> +	},
> +	[BMI270_NOMO2_REG] = {
> +		.page = 2,
> +		.addr = 0x32,
> +	},
>  	[BMI270_SC_26_REG] = {
>  		.page = 6,
>  		.addr = 0x32,
> @@ -426,6 +468,72 @@ static int bmi270_step_wtrmrk_en(struct bmi270_data *data, bool state)
>  
>  	set_mask_bits(&data->feature_events,
>  		      BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, field_value);
> +
> +	return 0;
> +}
> +
> +static int bmi270_motion_config_reg(enum iio_event_direction dir)
> +{
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		return BMI270_ANYMO1_REG;
> +	case IIO_EV_DIR_FALLING:
> +		return BMI270_NOMO1_REG;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bmi270_motion_event_en(struct bmi270_data *data,
> +				  enum iio_event_direction dir, bool state)
> +{
> +	int ret, config1, config2, irq_reg;
> +	int irq_msk, irq_field_val;
> +
> +	irq_reg = bmi270_int_map_reg(data->irq_pin);
> +	if (irq_reg < 0)
> +		return -EINVAL;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		config1 = BMI270_ANYMO1_REG;
> +		config2 = BMI270_ANYMO2_REG;
> +		irq_msk = BMI270_INT_MAP_FEAT_ANYMOTION_MSK;
> +		irq_field_val = FIELD_PREP(BMI270_INT_MAP_FEAT_ANYMOTION_MSK,
> +					   state);
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		config1 = BMI270_NOMO1_REG;
> +		config2 = BMI270_NOMO2_REG;
> +		irq_msk = BMI270_INT_MAP_FEAT_NOMOTION_MSK;
> +		irq_field_val = FIELD_PREP(BMI270_INT_MAP_FEAT_NOMOTION_MSK,
> +					   state);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	guard(mutex)(&data->mutex);
> +	ret = bmi270_update_feature_reg(data, config1,
> +					BMI270_FEAT_MOTION_XYZ_MSK,
> +					FIELD_PREP(BMI270_FEAT_MOTION_XYZ_MSK,
> +						   state ? BMI270_MOTION_XYZ_MSK : 0));
> +	if (ret)
> +		return ret;
> +
> +	ret = bmi270_update_feature_reg(data, config2,
> +					BMI270_FEAT_MOTION_ENABLE_MSK,
> +					FIELD_PREP(BMI270_FEAT_MOTION_ENABLE_MSK,
> +						   state));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(data->regmap, irq_reg, irq_msk, irq_field_val);
> +	if (ret)
> +		return ret;
> +
> +	set_mask_bits(&data->feature_events, irq_msk, irq_field_val);
> +
>  	return 0;
>  }
>  
> @@ -604,6 +712,20 @@ static irqreturn_t bmi270_irq_thread_handler(int irq, void *private)
>  	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status1))
>  		iio_trigger_poll_nested(data->trig);
>  
> +	if (FIELD_GET(BMI270_INT_STATUS_0_MOTION_MSK, status0))
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +							     IIO_MOD_X_OR_Y_OR_Z,
> +							     IIO_EV_TYPE_MAG,
> +							     IIO_EV_DIR_RISING),
> +			       timestamp);
> +
> +	if (FIELD_GET(BMI270_INT_STATUS_0_NOMOTION_MSK, status0))
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
No motion tends to be X_AND_Y_AND_Z.

Which we tend to need to represent as a separate 'pseudo' channel so that we
can have in_accel_x&y&z_mag_falling or similar.



> +							     IIO_MOD_X_OR_Y_OR_Z,
> +							     IIO_EV_TYPE_MAG,
> +							     IIO_EV_DIR_FALLING),
> +			       timestamp);
> +
>  	if (FIELD_GET(BMI270_INT_STATUS_0_STEP_CNT_MSK, status0))
>  		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
>  							     IIO_NO_MOD,
> @@ -820,6 +942,20 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static IIO_CONST_ATTR(in_accel_mag_value_available, "[0.0 0.00049 1.0]");
> +
> +static IIO_CONST_ATTR(in_accel_mag_period_available, "[0.0 0.02 162.0]");
> +
> +static struct attribute *bmi270_event_attributes[] = {
> +	&iio_const_attr_in_accel_mag_value_available.dev_attr.attr,
> +	&iio_const_attr_in_accel_mag_period_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group bmi270_event_attribute_group = {
> +	.attrs = bmi270_event_attributes,
> +};
> +
>  static int bmi270_write_event_config(struct iio_dev *indio_dev,
>  				     const struct iio_chan_spec *chan,
>  				     enum iio_event_type type,
> @@ -828,6 +964,8 @@ static int bmi270_write_event_config(struct iio_dev *indio_dev,
>  	struct bmi270_data *data = iio_priv(indio_dev);
>  
>  	switch (type) {
> +	case IIO_EV_TYPE_MAG:
> +		return bmi270_motion_event_en(data, dir, state);
>  	case IIO_EV_TYPE_CHANGE:
>  		return bmi270_step_wtrmrk_en(data, state);
>  	default:
> @@ -847,6 +985,17 @@ static int bmi270_read_event_config(struct iio_dev *indio_dev,
>  	guard(mutex)(&data->mutex);
>  
>  	switch (chan->type) {
> +	case IIO_ACCEL:
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			return FIELD_GET(BMI270_INT_MAP_FEAT_ANYMOTION_MSK,
> +					 data->feature_events) ? 1 : 0;
> +		case IIO_EV_DIR_FALLING:
> +			return FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK,
> +					 data->feature_events) ? 1 : 0;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_STEPS:
>  		return FIELD_GET(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
>  				 data->feature_events) ? 1 : 0;
> @@ -864,10 +1013,42 @@ static int bmi270_write_event_value(struct iio_dev *indio_dev,
>  {
>  	struct bmi270_data *data = iio_priv(indio_dev);
>  	unsigned int raw;
> +	int reg;
>  
>  	guard(mutex)(&data->mutex);
>  
>  	switch (type) {
> +	case IIO_EV_TYPE_MAG:
> +		reg = bmi270_motion_config_reg(dir);
> +		if (reg < 0)
> +			return -EINVAL;
> +
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			if (!in_range(val, 0, 1))
> +				return -EINVAL;
> +
> +			raw = BMI270_INT_MICRO_TO_RAW(val, val2,
> +						      BMI270_MOTION_THRES_SCALE);
> +
> +			return bmi270_update_feature_reg(data, reg,
> +				BMI270_FEAT_MOTION_THRESHOLD_MSK,
> +				FIELD_PREP(BMI270_FEAT_MOTION_THRESHOLD_MSK,
> +					   raw));
> +		case IIO_EV_INFO_PERIOD:
> +			if (!in_range(val, 0, 163))
> +				return -EINVAL;
> +
> +			raw = BMI270_INT_MICRO_TO_RAW(val, val2,
> +						      BMI270_MOTION_DURAT_SCALE);
> +
> +			return bmi270_update_feature_reg(data, reg,
> +				BMI270_FEAT_MOTION_DURATION_MSK,
> +				FIELD_PREP(BMI270_FEAT_MOTION_DURATION_MSK,
> +					   raw));
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_EV_TYPE_CHANGE:
>  		if (!in_range(val, 0, 20461))
>  			return -EINVAL;
> @@ -891,12 +1072,39 @@ static int bmi270_read_event_value(struct iio_dev *indio_dev,
>  {
>  	struct bmi270_data *data = iio_priv(indio_dev);
>  	unsigned int raw;
> +	int ret, reg;
>  	u16 reg_val;
> -	int ret;
>  
>  	guard(mutex)(&data->mutex);
>  
>  	switch (type) {
> +	case IIO_EV_TYPE_MAG:
> +		reg = bmi270_motion_config_reg(dir);
> +		if (reg < 0)
> +			return -EINVAL;
> +
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			ret = bmi270_read_feature_reg(data, reg, &reg_val);
> +			if (ret)
> +				return ret;
> +
> +			raw = FIELD_GET(BMI270_FEAT_MOTION_THRESHOLD_MSK, reg_val);
> +			*val = raw / BMI270_MOTION_THRES_SCALE;
> +			*val2 = BMI270_RAW_TO_MICRO(raw, BMI270_MOTION_THRES_SCALE);
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_EV_INFO_PERIOD:
> +			ret = bmi270_read_feature_reg(data, reg, &reg_val);
> +			if (ret)
> +				return ret;
> +
> +			raw = FIELD_GET(BMI270_FEAT_MOTION_DURATION_MSK, reg_val);
> +			*val = raw / BMI270_MOTION_DURAT_SCALE;
> +			*val2 = BMI270_RAW_TO_MICRO(raw, BMI270_MOTION_DURAT_SCALE);
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_EV_TYPE_CHANGE:
>  		ret = bmi270_read_feature_reg(data, BMI270_SC_26_REG, &reg_val);
>  		if (ret)
> @@ -917,6 +1125,23 @@ static const struct iio_event_spec bmi270_step_wtrmrk_event = {
>  			       BIT(IIO_EV_INFO_VALUE),
>  };
>