Re: [PATCH v2 2/3] iio: accel: bmc150: use DMA-safe buffers for regmap bulk reads

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 15 Aug 2026 23:27:27 +0530
Yash Suthar <yashsuthar983@gmail.com> wrote:

> The FIFO and read_raw buffers are passed to regmap bulk/raw reads
> ,which is not DMA-safe.Moved them into struct bmc150_accel_data

Move the comma up a line.

> after scan, each aligned to IIO_DMA_MINALIGN.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>

My main question here is around locking and whether we need
to space all 3 buffers out in a cache line each. That is painful
on some architectures.  If we ensure that DMA to one of these
never overlaps with the CPU accessing a different one, then we can
just mark the first one as IIO_DMA_MINALIGN.

Jonathan

> ---
>  drivers/iio/accel/bmc150-accel-core.c | 14 ++++++--------
>  drivers/iio/accel/bmc150-accel.h      |  5 +++++
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 89a475ef9a9b..e8d27fd1be3f 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -125,7 +125,6 @@
>  #define BMC150_ACCEL_REG_FIFO_CONFIG0		0x30
>  #define BMC150_ACCEL_REG_FIFO_CONFIG1		0x3E
>  #define BMC150_ACCEL_REG_FIFO_DATA		0x3F
> -#define BMC150_ACCEL_FIFO_LENGTH		32
>  
>  enum bmc150_accel_axis {
>  	AXIS_X,
> @@ -622,7 +621,6 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
>  	struct device *dev = regmap_get_device(data->regmap);
>  	int ret;
>  	int axis = chan->scan_index;
> -	__le16 raw_val;
>  
>  	mutex_lock(&data->mutex);
>  	ret = bmc150_accel_set_power_state(data, true);
> @@ -632,14 +630,14 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
>  	}
>  
>  	ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
> -			       &raw_val, sizeof(raw_val));
> +			       &data->regval, sizeof(data->regval));
>  	if (ret < 0) {
>  		dev_err(dev, "Error reading axis %d\n", axis);
>  		bmc150_accel_set_power_state(data, false);
>  		mutex_unlock(&data->mutex);
>  		return ret;
>  	}
> -	*val = sign_extend32(le16_to_cpu(raw_val) >> chan->scan_type.shift,
> +	*val = sign_extend32(le16_to_cpu(data->regval) >> chan->scan_type.shift,
>  			     chan->scan_type.realbits - 1);
>  	ret = bmc150_accel_set_power_state(data, false);
>  	mutex_unlock(&data->mutex);
> @@ -918,7 +916,7 @@ static int bmc150_accel_set_watermark(struct iio_dev *indio_dev, unsigned val)
>   * frame data is discarded.
>   */
>  static int bmc150_accel_fifo_transfer(struct bmc150_accel_data *data,
> -				      char *buffer, int samples)
> +				      void *buffer, int samples)
>  {
>  	struct device *dev = regmap_get_device(data->regmap);
>  	int sample_length = 3 * 2;
> @@ -941,7 +939,6 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
>  	struct device *dev = regmap_get_device(data->regmap);
>  	int ret, i;
>  	u8 count;
> -	u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
>  	int64_t tstamp;
>  	uint64_t sample_period;
>  	unsigned int val;
> @@ -993,7 +990,7 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
>  
>  	count = min_t(u8, count, BMC150_ACCEL_FIFO_LENGTH);
>  
> -	ret = bmc150_accel_fifo_transfer(data, (u8 *)buffer, count);
> +	ret = bmc150_accel_fifo_transfer(data, data->fifo_buff, count);
>  	if (ret)
>  		return ret;
>  
> @@ -1008,7 +1005,8 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
>  
>  		j = 0;
>  		iio_for_each_active_channel(indio_dev, bit)
> -			memcpy(&data->scan.channels[j++], &buffer[i * 3 + bit],
> +			memcpy(&data->scan.channels[j++],
> +			       &data->fifo_buff[i * 3 + bit],
>  			       sizeof(data->scan.channels[0]));
>  
>  		iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
> index 9deff256aed5..8f64b24339e4 100644
> --- a/drivers/iio/accel/bmc150-accel.h
> +++ b/drivers/iio/accel/bmc150-accel.h
> @@ -56,6 +56,8 @@ enum bmc150_accel_trigger_id {
>  	BMC150_ACCEL_TRIGGERS,
>  };
>  
> +#define BMC150_ACCEL_FIFO_LENGTH		32
> +
>  struct bmc150_accel_data {
>  	struct regmap *regmap;
>  	int irq;
> @@ -81,6 +83,9 @@ struct bmc150_accel_data {
>  		__le16 channels[3];
>  		aligned_s64 ts;
>  	} scan __aligned(IIO_DMA_MINALIGN);
> +	/* DMA-safe buffers for bulk/raw reads */
> +	__le16 fifo_buff[BMC150_ACCEL_FIFO_LENGTH * 3] __aligned(IIO_DMA_MINALIGN);
> +	__le16 regval __aligned(IIO_DMA_MINALIGN);

This is potentially a lot of padding that might not be necessary.
What locking protects these?  I'd kind of expect all 3 to be used
under a single lock.  If that's the case and locks are held for all
such usage, then we can just force alignment of the first one.


>  };
>  
>  int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,