Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Mar 2022 00:08:57 +0530
From:   Jagath Jog J <jagathjog1996@gmail.com>
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     dan@dlrobertson.com, andy.shevchenko@gmail.com,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support
Message-ID: <20220328183852.GA4465@jagath-PC>
References: <20220326194146.15549-1-jagathjog1996@gmail.com>
 <20220326194146.15549-4-jagathjog1996@gmail.com>
 <20220327174545.41a15150@jic23-huawei>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220327174545.41a15150@jic23-huawei>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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@gmail.com> 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@gmail.com>
> 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