Re: [PATCH v4 4/9] iio: accel: bma400: Add triggered buffer support

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 27 Apr 2022 14:34:57 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Apr 20, 2022 at 11:11 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >
> > Added trigger buffer support to read continuous acceleration
> > data from device with data ready interrupt which is mapped
> > to INT1 pin.  
> 
> LGTM,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Agreed.  A couple of 'comments' inline but no actual need to change anything.
One is contingent on a fix I've not sent out yet for the rest of IIO.
The other is potentially a minor improvement for the future.

Thanks,

Jonathan

> 
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > ---
> >  drivers/iio/accel/Kconfig       |   2 +
> >  drivers/iio/accel/bma400.h      |  10 +-
> >  drivers/iio/accel/bma400_core.c | 162 +++++++++++++++++++++++++++++++-
> >  drivers/iio/accel/bma400_i2c.c  |   2 +-
> >  drivers/iio/accel/bma400_spi.c  |   2 +-
> >  5 files changed, 170 insertions(+), 8 deletions(-)

> >
> >  #include "bma400.h"
> >
> > @@ -61,6 +66,14 @@ struct bma400_data {
> >         struct bma400_sample_freq sample_freq;
> >         int oversampling_ratio;
> >         int scale;
> > +       struct iio_trigger *trig;
> > +       /* Correct time stamp alignment */
> > +       struct {
> > +               __le16 buff[3];
> > +               u8 temperature;
> > +               s64 ts __aligned(8);
> > +       } buffer ____cacheline_aligned;

If you are rolling again, could you change this to
__aligned(IIO_ALIGN);  See
https://lore.kernel.org/linux-iio/20220419121241.00002e42@Huawei.com/
for why.
Note that I'll be sending a fix patch out for IIO_ALIGN to define
it as ARCH_KMALLOC_ALIGN in next few days.

If you'd pref not to get caught up in that, send it as it stands
and I'll fix up once that fix is in place.  What's one more driver
on top of the 80+ I have to do anyway :)



> > +       __le16 status;
> >  };
> >

> > +
> > +static const unsigned long bma400_avail_scan_masks[] = {
> > +       GENMASK(3, 0),
> > +       0
> > +};
> > +
> >  static const struct iio_info bma400_info = {
> >         .read_raw          = bma400_read_raw,
> >         .read_avail        = bma400_read_avail,
> > @@ -814,7 +869,72 @@ static const struct iio_info bma400_info = {
> >         .write_raw_get_fmt = bma400_write_raw_get_fmt,
> >  };
> >
> > -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name)
> > +static const struct iio_trigger_ops bma400_trigger_ops = {
> > +       .set_trigger_state = &bma400_data_rdy_trigger_set_state,
> > +       .validate_device = &iio_trigger_validate_own_device,
> > +};
> > +
> > +static irqreturn_t bma400_trigger_handler(int irq, void *p)
> > +{
> > +       struct iio_poll_func *pf = p;
> > +       struct iio_dev *indio_dev = pf->indio_dev;
> > +       struct bma400_data *data = iio_priv(indio_dev);
> > +       int ret, temp;
> > +
> > +       /* Lock to protect the data->buffer */
> > +       mutex_lock(&data->mutex);
> > +
> > +       /* bulk read six registers, with the base being the LSB register */
> > +       ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> > +                              &data->buffer.buff, sizeof(data->buffer.buff));
> > +       if (ret)
> > +               goto unlock_err;
> > +
> > +       ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);

Given the temperature read is a separate action, it seems like you could sensible
add another entry to bma400_avail_scan_masks() for just the accelerometer axis
and then only perform this read if the temperature is requested.

It would be a feature though, so no need to have it in this patch if you
prefer not to.

> > +       if (ret)
> > +               goto unlock_err;
> > +
> > +       data->buffer.temperature = temp;
> > +
> > +       iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> > +                                          iio_get_time_ns(indio_dev));
> > +
> > +       mutex_unlock(&data->mutex);
> > +       iio_trigger_notify_done(indio_dev->trig);
> > +       return IRQ_HANDLED;
> > +
> > +unlock_err:
> > +       mutex_unlock(&data->mutex);
> > +       return IRQ_NONE;
> > +}