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