Re: [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support

[Jonathan Cameron <jic23@kernel.org>]

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.


...

>  
>  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.

> +	return ret;
> +}
> +