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

Hi Jonathan
thanks for review
i agreed with you, earlier i did same as you mentioned see:
https://lore.kernel.org/all/[email protected]/
but after review from Andy Shevchenko:
https://lore.kernel.org/all/[email protected]/#t
I agreed and revert the flow for v2, but in doing so I moved
mutex_unlock() before
push.

In v3 will hold mutex for both read and push, single IIO_DMA_MINALIGN on scan
is sufficient, so fifo_buff and regval no longer need separate
__aligned(IIO_DMA_MINALIGN)
Will also replace AXIS_MAX*2 with sizeof(data->scan.channels) and drop
the unused AXIS_MAX.

Sincerely,
Yash Suthar

On Sun, Aug 23, 2026 at 3:36 AM Jonathan Cameron <[email protected]> wrote:
>
> On Sat, 15 Aug 2026 23:27:27 +0530
> Yash Suthar <[email protected]> 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 <[email protected]>
> > Signed-off-by: Yash Suthar <[email protected]>
>
> 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,
>