Hi Jonathan,
On Sun, Mar 27, 2022 at 05:45:45PM +0100, Jonathan Cameron wrote:
> On Sun, 27 Mar 2022 01:11:44 +0530
> 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.
> >
> > Signed-off-by: Jagath Jog J <jagathjog1996@xxxxxxxxx>
> Hi Jagath,
>
> Just a few small things noticed on this read through.
>
> Thanks,
>
> Jonathan
>
> > ---
> > 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, 168 insertions(+), 10 deletions(-)
> >
>
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index dc273381a0a2..fa3f4b5f229f 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -11,16 +11,22 @@
> > * - Create channel for sensor time
> > */
> >
> > +#include <linux/bitfield.h>
> > #include <linux/bitops.h>
> > #include <linux/device.h>
> > -#include <linux/iio/iio.h>
> > -#include <linux/iio/sysfs.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/regmap.h>
> > #include <linux/regulator/consumer.h>
> >
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
>
> Is iio/sysfs.h actually used? It rarely is these days as it contains
> the infrastructure for custom attributes and we try not to use any
> of those anymore.
>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +
> This reorganization of headers is good but shouldn't be in this patch.
> Add an earlier patch in the series to move the existing pair down here
> before this patch then adds the new ones.
Sure I will do the reorganization of headers in the seperate patch.
>
>
> ...
>
> >
> > static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
> > @@ -659,6 +687,10 @@ static int bma400_init(struct bma400_data *data)
> > if (ret)
> > return ret;
> >
> > + /* Configure INT1 pin to open drain */
> > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06);
> > + if (ret)
> > + return ret;
> > /*
> > * Once the interrupt engine is supported we might use the
> > * data_src_reg, but for now ensure this is set to the
> > @@ -807,6 +839,33 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> > }
> > }
> >
> > +static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> > + bool state)
> > +{
> > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > + struct bma400_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> > + BMA400_INT_DRDY_MSK,
> > + FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> > + BMA400_INT_DRDY_MSK,
> > + FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> return regmap_update_bits()...
>
> > +}
>
> ...
>
>
> > +static irqreturn_t bma400_interrupt(int irq, void *private)
> > +{
> > + struct iio_dev *indio_dev = private;
> > + struct bma400_data *data = iio_priv(indio_dev);
> > + irqreturn_t ret = IRQ_NONE;
> > + __le16 status;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
> > + sizeof(status));
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return IRQ_NONE;
> > +
> > + if (FIELD_GET(BMA400_INT_DRDY_MSK, le16_to_cpu(status))) {
> > + iio_trigger_poll_chained(data->trig);
> > + ret = IRQ_HANDLED;
> Preference for this style
> return IRQ_HANDLED;
> > + }
> > +
> return IRQ_NONE;
> and don't initialize above.
Sure I will make these changes and I will try to handle the events before
the data ready since step interrupt will occur less frequently compared to
data ready interrupts.
>
> > + return ret;
> > +}
> > +
>
>
Thank you,
Jagath
