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

[Jagath Jog J]

Hi Jonathan,

On Sun, May 1, 2022 at 9:42 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Wed, 27 Apr 2022 14:34:57 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> > On Wed, Apr 20, 2022 at 11:11 PM Jagath Jog J <jagathjog1996@xxxxxxxxx> 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@xxxxxxxxx>
> 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@xxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxx/
> 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 :)
>
>

Sure, I will change that to __aligned(IIO_ALIGN); in the next series.

>
> > > +       __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.

Sure I will add another entry only for the accelerometer axis and I
will make changes
accordingly in the next series.

Do I need to add 'Reviewed-by' tag if the patch gets modified again
after getting the
tag?


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