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

[Jonathan Cameron]

On Mon, 2 May 2022 01:55:49 +0530
Jagath Jog J <jagathjog1996@xxxxxxxxx> wrote:

> 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?
> 
It's something you have judge based on whether you think a change
is significant enough to warrant dropping tags.
If you do drop them you should always state why in the cover
letter or change log.

Thanks,

Jonathan

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