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

[Jonathan Cameron <jic23@kernel.org>]

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),
> +};