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