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

[Jonathan Cameron <jic23@kernel.org>]

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;