Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event

Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4144 bytes --]

CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220326194146.15549-6-jagathjog1996@gmail.com>
References: <20220326194146.15549-6-jagathjog1996@gmail.com>
TO: Jagath Jog J <jagathjog1996@gmail.com>
TO: dan(a)dlrobertson.com
TO: jic23(a)kernel.org
TO: andy.shevchenko(a)gmail.com
CC: linux-iio(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org

Hi Jagath,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.17]
[cannot apply to jic23-iio/togreg next-20220325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Jagath-Jog-J/iio-accel-bma400-Add-support-for-buffer-and-step/20220327-034340
base:    f443e374ae131c168a065ea1748feac6b2e76613
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
config: parisc-randconfig-c024-20220327 (https://download.01.org/0day-ci/archive/20220327/202203270615.KLDZXbOm-lkp(a)intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.2.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>


cocci warnings: (new ones prefixed by >>)
>> drivers/iio/accel/bma400_core.c:924:4-10: preceding lock on line 916

vim +924 drivers/iio/accel/bma400_core.c

289ea151b60e04 Jagath Jog J 2022-03-27  905  
289ea151b60e04 Jagath Jog J 2022-03-27  906  static int bma400_write_event_config(struct iio_dev *indio_dev,
289ea151b60e04 Jagath Jog J 2022-03-27  907  				     const struct iio_chan_spec *chan,
289ea151b60e04 Jagath Jog J 2022-03-27  908  				     enum iio_event_type type,
289ea151b60e04 Jagath Jog J 2022-03-27  909  				     enum iio_event_direction dir, int state)
289ea151b60e04 Jagath Jog J 2022-03-27  910  {
289ea151b60e04 Jagath Jog J 2022-03-27  911  	int ret;
289ea151b60e04 Jagath Jog J 2022-03-27  912  	struct bma400_data *data = iio_priv(indio_dev);
289ea151b60e04 Jagath Jog J 2022-03-27  913  
289ea151b60e04 Jagath Jog J 2022-03-27  914  	switch (type) {
289ea151b60e04 Jagath Jog J 2022-03-27  915  	case IIO_EV_TYPE_CHANGE:
289ea151b60e04 Jagath Jog J 2022-03-27 @916  		mutex_lock(&data->mutex);
289ea151b60e04 Jagath Jog J 2022-03-27  917  		if (!data->steps_enabled) {
289ea151b60e04 Jagath Jog J 2022-03-27  918  			ret = regmap_update_bits(data->regmap,
289ea151b60e04 Jagath Jog J 2022-03-27  919  						 BMA400_INT_CONFIG1_REG,
289ea151b60e04 Jagath Jog J 2022-03-27  920  						 BMA400_STEP_INT_MSK,
289ea151b60e04 Jagath Jog J 2022-03-27  921  						 FIELD_PREP(BMA400_STEP_INT_MSK,
289ea151b60e04 Jagath Jog J 2022-03-27  922  							    1));
289ea151b60e04 Jagath Jog J 2022-03-27  923  			if (ret)
289ea151b60e04 Jagath Jog J 2022-03-27 @924  				return ret;
289ea151b60e04 Jagath Jog J 2022-03-27  925  			data->steps_enabled = 1;
289ea151b60e04 Jagath Jog J 2022-03-27  926  		}
289ea151b60e04 Jagath Jog J 2022-03-27  927  
289ea151b60e04 Jagath Jog J 2022-03-27  928  		ret = regmap_update_bits(data->regmap,
289ea151b60e04 Jagath Jog J 2022-03-27  929  					 BMA400_INT12_MAP_REG,
289ea151b60e04 Jagath Jog J 2022-03-27  930  					 BMA400_STEP_INT_MSK,
289ea151b60e04 Jagath Jog J 2022-03-27  931  					 FIELD_PREP(BMA400_STEP_INT_MSK,
289ea151b60e04 Jagath Jog J 2022-03-27  932  						    state));
289ea151b60e04 Jagath Jog J 2022-03-27  933  		mutex_unlock(&data->mutex);
289ea151b60e04 Jagath Jog J 2022-03-27  934  		if (ret)
289ea151b60e04 Jagath Jog J 2022-03-27  935  			return ret;
289ea151b60e04 Jagath Jog J 2022-03-27  936  		data->step_event_en = state;
289ea151b60e04 Jagath Jog J 2022-03-27  937  		return 0;
289ea151b60e04 Jagath Jog J 2022-03-27  938  	default:
289ea151b60e04 Jagath Jog J 2022-03-27  939  		return -EINVAL;
289ea151b60e04 Jagath Jog J 2022-03-27  940  	}
289ea151b60e04 Jagath Jog J 2022-03-27  941  }
289ea151b60e04 Jagath Jog J 2022-03-27  942  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH v2 0/5] iio: accel: bma400: Add support for buffer and step

[Jagath Jog J]

This patch series adds trigger buffer support with data ready interrupt,
separate channel for step counter and an event for step change interrupt.

changes since v1
1. Added comment section that describes the math for scale calculation.
2. Added separate devm_add_action_or_reset() calls to disable regulator
   and to put the sensor in power down mode.
3. Remove the err_reg_disable and out, goto labels and returning directly
   if error occurs.
4. Added mutex calls while putting sensor in power down.
5. Added ___cacheline_aligned for device data.
6. Ordering the header includes.
7. Handling erroneous and spurious interrupts in the interrupt handler
   by returning IRQ_NONE.
8. Using dev_err_probe() instead of dev_err().
9. Configured the interrupt to open drain.
10. Using le16_to_cpu() to fix the sparse warning.
11. Checking the step change event is enabled or not.
12. Enabling the step change event will also enable the step channel.
13. Using FIELD_GET() instead of bitwise operation.
14. Removal of dead code in the _event_config().

Jagath Jog J (5):
  iio: accel: bma400: Fix the scale min and max macro values
  iio: accel: bma400: conversion to device-managed function
  iio: accel: bma400: Add triggered buffer support
  iio: accel: bma400: Add separate channel for step counter
  iio: accel: bma400: Add step change event

 drivers/iio/accel/Kconfig       |   2 +
 drivers/iio/accel/bma400.h      |  37 +++-
 drivers/iio/accel/bma400_core.c | 351 +++++++++++++++++++++++++++-----
 drivers/iio/accel/bma400_i2c.c  |  10 +-
 drivers/iio/accel/bma400_spi.c  |  10 +-
 5 files changed, 341 insertions(+), 69 deletions(-)

-- 
2.17.1

[PATCH v2 5/5] iio: accel: bma400: Add step change event

[Jagath Jog J]

Added support for event when there is a detection of step change.
INT1 pin is used to interrupt and event is pushed to userspace.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 drivers/iio/accel/bma400.h      |  2 +
 drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index c9b856b37021..c4ec0cf6dc00 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -39,6 +39,7 @@
 #define BMA400_INT_STAT0_REG        0x0e
 #define BMA400_INT_STAT1_REG        0x0f
 #define BMA400_INT_STAT2_REG        0x10
+#define BMA400_INT12_MAP_REG	    0x23
 
 /* Temperature register */
 #define BMA400_TEMP_DATA_REG        0x11
@@ -54,6 +55,7 @@
 #define BMA400_STEP_CNT3_REG        0x17
 #define BMA400_STEP_STAT_REG        0x18
 #define BMA400_STEP_INT_MSK	    BIT(0)
+#define BMA400_STEP_STAT_MASK	    GENMASK(9, 8)
 
 /*
  * Read-write configuration registers
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index ec2f9c380bda..aaa104a2698b 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -24,6 +24,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
@@ -70,6 +71,7 @@ struct bma400_data {
 	int scale;
 	struct iio_trigger *trig;
 	int steps_enabled;
+	bool step_event_en;
 	/* Correct time stamp alignment */
 	struct {
 		__le16 buff[3];
@@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
 	{ }
 };
 
+static const struct iio_event_spec bma400_step_detect_event = {
+	.type = IIO_EV_TYPE_CHANGE,
+	.dir = IIO_EV_DIR_NONE,
+	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+};
+
 #define BMA400_ACC_CHANNEL(_index, _axis) { \
 	.type = IIO_ACCEL, \
 	.modified = 1, \
@@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				      BIT(IIO_CHAN_INFO_ENABLE),
 		.scan_index = -1, /* No buffer support */
+		.event_spec = &bma400_step_detect_event,
+		.num_event_specs = 1,
 	},
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
@@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
+static int bma400_read_event_config(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir)
+{
+	struct bma400_data *data = iio_priv(indio_dev);
+
+	switch (type) {
+	case IIO_EV_TYPE_CHANGE:
+		return data->step_event_en;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bma400_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir, int state)
+{
+	int ret;
+	struct bma400_data *data = iio_priv(indio_dev);
+
+	switch (type) {
+	case IIO_EV_TYPE_CHANGE:
+		mutex_lock(&data->mutex);
+		if (!data->steps_enabled) {
+			ret = regmap_update_bits(data->regmap,
+						 BMA400_INT_CONFIG1_REG,
+						 BMA400_STEP_INT_MSK,
+						 FIELD_PREP(BMA400_STEP_INT_MSK,
+							    1));
+			if (ret)
+				return ret;
+			data->steps_enabled = 1;
+		}
+
+		ret = regmap_update_bits(data->regmap,
+					 BMA400_INT12_MAP_REG,
+					 BMA400_STEP_INT_MSK,
+					 FIELD_PREP(BMA400_STEP_INT_MSK,
+						    state));
+		mutex_unlock(&data->mutex);
+		if (ret)
+			return ret;
+		data->step_event_en = state;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
 					     bool state)
 {
@@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
 	.read_avail        = bma400_read_avail,
 	.write_raw         = bma400_write_raw,
 	.write_raw_get_fmt = bma400_write_raw_get_fmt,
+	.read_event_config = bma400_read_event_config,
+	.write_event_config = bma400_write_event_config,
 };
 
 static const struct iio_trigger_ops bma400_trigger_ops = {
@@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
 		ret = IRQ_HANDLED;
 	}
 
+	if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
+		iio_push_event(indio_dev,
+			       IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
+					      IIO_EV_DIR_NONE,
+					      IIO_EV_TYPE_CHANGE, 0, 0, 0),
+			       iio_get_time_ns(indio_dev));
+		ret = IRQ_HANDLED;
+	}
+
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event

[Jonathan Cameron]

On Sun, 27 Mar 2022 01:11:46 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Added support for event when there is a detection of step change.
> INT1 pin is used to interrupt and event is pushed to userspace.
> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>

These last two patches look fine to me.  Simply having the
event enable the channel makes things simpler.

I briefly wondered if we need to care about sequences like

1) Enable event
2) Enable channel (already enabled, but perhaps this indicates separate intent)
3) Disable event.
4) Is the channel still enabled?

or the simpler case of whether we should disable the channel if the event is
disabled and it wasn't otherwise turned on.

However, I can't see a sensible way to do so. Hence I think what you have
gone with is the best we can do.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/bma400.h      |  2 +
>  drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index c9b856b37021..c4ec0cf6dc00 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -39,6 +39,7 @@
>  #define BMA400_INT_STAT0_REG        0x0e
>  #define BMA400_INT_STAT1_REG        0x0f
>  #define BMA400_INT_STAT2_REG        0x10
> +#define BMA400_INT12_MAP_REG	    0x23
>  
>  /* Temperature register */
>  #define BMA400_TEMP_DATA_REG        0x11
> @@ -54,6 +55,7 @@
>  #define BMA400_STEP_CNT3_REG        0x17
>  #define BMA400_STEP_STAT_REG        0x18
>  #define BMA400_STEP_INT_MSK	    BIT(0)
> +#define BMA400_STEP_STAT_MASK	    GENMASK(9, 8)
>  
>  /*
>   * Read-write configuration registers
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index ec2f9c380bda..aaa104a2698b 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -24,6 +24,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> @@ -70,6 +71,7 @@ struct bma400_data {
>  	int scale;
>  	struct iio_trigger *trig;
>  	int steps_enabled;
> +	bool step_event_en;
>  	/* Correct time stamp alignment */
>  	struct {
>  		__le16 buff[3];
> @@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
>  	{ }
>  };
>  
> +static const struct iio_event_spec bma400_step_detect_event = {
> +	.type = IIO_EV_TYPE_CHANGE,
> +	.dir = IIO_EV_DIR_NONE,
> +	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +};
> +
>  #define BMA400_ACC_CHANNEL(_index, _axis) { \
>  	.type = IIO_ACCEL, \
>  	.modified = 1, \
> @@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
>  				      BIT(IIO_CHAN_INFO_ENABLE),
>  		.scan_index = -1, /* No buffer support */
> +		.event_spec = &bma400_step_detect_event,
> +		.num_event_specs = 1,
>  	},
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
> @@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int bma400_read_event_config(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir)
> +{
> +	struct bma400_data *data = iio_priv(indio_dev);
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_CHANGE:
> +		return data->step_event_en;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bma400_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir, int state)
> +{
> +	int ret;
> +	struct bma400_data *data = iio_priv(indio_dev);
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_CHANGE:
> +		mutex_lock(&data->mutex);
> +		if (!data->steps_enabled) {
> +			ret = regmap_update_bits(data->regmap,
> +						 BMA400_INT_CONFIG1_REG,
> +						 BMA400_STEP_INT_MSK,
> +						 FIELD_PREP(BMA400_STEP_INT_MSK,
> +							    1));
> +			if (ret)
> +				return ret;
> +			data->steps_enabled = 1;
> +		}
> +
> +		ret = regmap_update_bits(data->regmap,
> +					 BMA400_INT12_MAP_REG,
> +					 BMA400_STEP_INT_MSK,
> +					 FIELD_PREP(BMA400_STEP_INT_MSK,
> +						    state));
> +		mutex_unlock(&data->mutex);
> +		if (ret)
> +			return ret;
> +		data->step_event_en = state;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  					     bool state)
>  {
> @@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
>  	.read_avail        = bma400_read_avail,
>  	.write_raw         = bma400_write_raw,
>  	.write_raw_get_fmt = bma400_write_raw_get_fmt,
> +	.read_event_config = bma400_read_event_config,
> +	.write_event_config = bma400_write_event_config,
>  };
>  
>  static const struct iio_trigger_ops bma400_trigger_ops = {
> @@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
>  		ret = IRQ_HANDLED;
>  	}
>  
> +	if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
> +		iio_push_event(indio_dev,
> +			       IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> +					      IIO_EV_DIR_NONE,
> +					      IIO_EV_TYPE_CHANGE, 0, 0, 0),
> +			       iio_get_time_ns(indio_dev));
> +		ret = IRQ_HANDLED;
> +	}
> +
>  	return ret;
>  }
>  

Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event

[Jagath Jog J]

Hi Jonathan,

On Sun, Mar 27, 2022 at 05:50:36PM +0100, Jonathan Cameron wrote:
> On Sun, 27 Mar 2022 01:11:46 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
> 
> > Added support for event when there is a detection of step change.
> > INT1 pin is used to interrupt and event is pushed to userspace.
> > 
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> 
> These last two patches look fine to me.  Simply having the
> event enable the channel makes things simpler.

Means do I need to drop the step _INFO_ENABLE and handle the
enabling and disabling of step channel through the event enable and
disable?

> I briefly wondered if we need to care about sequences like
> 
> 1) Enable event
> 2) Enable channel (already enabled, but perhaps this indicates separate intent)
> 3) Disable event.
> 4) Is the channel still enabled?
> 
> or the simpler case of whether we should disable the channel if the event is
> disabled and it wasn't otherwise turned on.
> 
> However, I can't see a sensible way to do so. Hence I think what you have
> gone with is the best we can do.
> 
> Thanks,
> 
> Jonathan

Thanks for reviewing the patch series. I will also address all the comments
from Andy in the next patch v3.

Thank you
Jagath
> 
> > ---
> >  drivers/iio/accel/bma400.h      |  2 +
> >  drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
> >  2 files changed, 75 insertions(+)
> > 
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index c9b856b37021..c4ec0cf6dc00 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -39,6 +39,7 @@
> >  #define BMA400_INT_STAT0_REG        0x0e
> >  #define BMA400_INT_STAT1_REG        0x0f
> >  #define BMA400_INT_STAT2_REG        0x10
> > +#define BMA400_INT12_MAP_REG	    0x23
> >  
> >  /* Temperature register */
> >  #define BMA400_TEMP_DATA_REG        0x11
> > @@ -54,6 +55,7 @@
> >  #define BMA400_STEP_CNT3_REG        0x17
> >  #define BMA400_STEP_STAT_REG        0x18
> >  #define BMA400_STEP_INT_MSK	    BIT(0)
> > +#define BMA400_STEP_STAT_MASK	    GENMASK(9, 8)
> >  
> >  /*
> >   * Read-write configuration registers
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index ec2f9c380bda..aaa104a2698b 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> >  #include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> >  #include <linux/iio/trigger.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/triggered_buffer.h>
> > @@ -70,6 +71,7 @@ struct bma400_data {
> >  	int scale;
> >  	struct iio_trigger *trig;
> >  	int steps_enabled;
> > +	bool step_event_en;
> >  	/* Correct time stamp alignment */
> >  	struct {
> >  		__le16 buff[3];
> > @@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> >  	{ }
> >  };
> >  
> > +static const struct iio_event_spec bma400_step_detect_event = {
> > +	.type = IIO_EV_TYPE_CHANGE,
> > +	.dir = IIO_EV_DIR_NONE,
> > +	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > +};
> > +
> >  #define BMA400_ACC_CHANNEL(_index, _axis) { \
> >  	.type = IIO_ACCEL, \
> >  	.modified = 1, \
> > @@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
> >  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> >  				      BIT(IIO_CHAN_INFO_ENABLE),
> >  		.scan_index = -1, /* No buffer support */
> > +		.event_spec = &bma400_step_detect_event,
> > +		.num_event_specs = 1,
> >  	},
> >  	IIO_CHAN_SOFT_TIMESTAMP(4),
> >  };
> > @@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> >  	}
> >  }
> >  
> > +static int bma400_read_event_config(struct iio_dev *indio_dev,
> > +				    const struct iio_chan_spec *chan,
> > +				    enum iio_event_type type,
> > +				    enum iio_event_direction dir)
> > +{
> > +	struct bma400_data *data = iio_priv(indio_dev);
> > +
> > +	switch (type) {
> > +	case IIO_EV_TYPE_CHANGE:
> > +		return data->step_event_en;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int bma400_write_event_config(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     enum iio_event_type type,
> > +				     enum iio_event_direction dir, int state)
> > +{
> > +	int ret;
> > +	struct bma400_data *data = iio_priv(indio_dev);
> > +
> > +	switch (type) {
> > +	case IIO_EV_TYPE_CHANGE:
> > +		mutex_lock(&data->mutex);
> > +		if (!data->steps_enabled) {
> > +			ret = regmap_update_bits(data->regmap,
> > +						 BMA400_INT_CONFIG1_REG,
> > +						 BMA400_STEP_INT_MSK,
> > +						 FIELD_PREP(BMA400_STEP_INT_MSK,
> > +							    1));
> > +			if (ret)
> > +				return ret;
> > +			data->steps_enabled = 1;
> > +		}
> > +
> > +		ret = regmap_update_bits(data->regmap,
> > +					 BMA400_INT12_MAP_REG,
> > +					 BMA400_STEP_INT_MSK,
> > +					 FIELD_PREP(BMA400_STEP_INT_MSK,
> > +						    state));
> > +		mutex_unlock(&data->mutex);
> > +		if (ret)
> > +			return ret;
> > +		data->step_event_en = state;
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> >  					     bool state)
> >  {
> > @@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
> >  	.read_avail        = bma400_read_avail,
> >  	.write_raw         = bma400_write_raw,
> >  	.write_raw_get_fmt = bma400_write_raw_get_fmt,
> > +	.read_event_config = bma400_read_event_config,
> > +	.write_event_config = bma400_write_event_config,
> >  };
> >  
> >  static const struct iio_trigger_ops bma400_trigger_ops = {
> > @@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> >  		ret = IRQ_HANDLED;
> >  	}
> >  
> > +	if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
> > +		iio_push_event(indio_dev,
> > +			       IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> > +					      IIO_EV_DIR_NONE,
> > +					      IIO_EV_TYPE_CHANGE, 0, 0, 0),
> > +			       iio_get_time_ns(indio_dev));
> > +		ret = IRQ_HANDLED;
> > +	}
> > +
> >  	return ret;
> >  }
> >  
> 

Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event

[Jonathan Cameron]

On Tue, 29 Mar 2022 02:07:11 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sun, Mar 27, 2022 at 05:50:36PM +0100, Jonathan Cameron wrote:
> > On Sun, 27 Mar 2022 01:11:46 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >   
> > > Added support for event when there is a detection of step change.
> > > INT1 pin is used to interrupt and event is pushed to userspace.
> > > 
> > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>  
> > 
> > These last two patches look fine to me.  Simply having the
> > event enable the channel makes things simpler.  
> 
> Means do I need to drop the step _INFO_ENABLE and handle the
> enabling and disabling of step channel through the event enable and
> disable?

No.  I was trying to say I like the solution you have now.

> 
> > I briefly wondered if we need to care about sequences like
> > 
> > 1) Enable event
> > 2) Enable channel (already enabled, but perhaps this indicates separate intent)
> > 3) Disable event.
> > 4) Is the channel still enabled?
> > 
> > or the simpler case of whether we should disable the channel if the event is
> > disabled and it wasn't otherwise turned on.
> > 
> > However, I can't see a sensible way to do so. Hence I think what you have
> > gone with is the best we can do.
> > 
> > Thanks,
> > 
> > Jonathan  
> 
> Thanks for reviewing the patch series. I will also address all the comments
> from Andy in the next patch v3.
> 
> Thank you
> Jagath
> >   
> > > ---
> > >  drivers/iio/accel/bma400.h      |  2 +
> > >  drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 75 insertions(+)
> > > 
> > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > index c9b856b37021..c4ec0cf6dc00 100644
> > > --- a/drivers/iio/accel/bma400.h
> > > +++ b/drivers/iio/accel/bma400.h
> > > @@ -39,6 +39,7 @@
> > >  #define BMA400_INT_STAT0_REG        0x0e
> > >  #define BMA400_INT_STAT1_REG        0x0f
> > >  #define BMA400_INT_STAT2_REG        0x10
> > > +#define BMA400_INT12_MAP_REG	    0x23
> > >  
> > >  /* Temperature register */
> > >  #define BMA400_TEMP_DATA_REG        0x11
> > > @@ -54,6 +55,7 @@
> > >  #define BMA400_STEP_CNT3_REG        0x17
> > >  #define BMA400_STEP_STAT_REG        0x18
> > >  #define BMA400_STEP_INT_MSK	    BIT(0)
> > > +#define BMA400_STEP_STAT_MASK	    GENMASK(9, 8)
> > >  
> > >  /*
> > >   * Read-write configuration registers
> > > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > > index ec2f9c380bda..aaa104a2698b 100644
> > > --- a/drivers/iio/accel/bma400_core.c
> > > +++ b/drivers/iio/accel/bma400_core.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/sysfs.h>
> > >  #include <linux/iio/buffer.h>
> > > +#include <linux/iio/events.h>
> > >  #include <linux/iio/trigger.h>
> > >  #include <linux/iio/trigger_consumer.h>
> > >  #include <linux/iio/triggered_buffer.h>
> > > @@ -70,6 +71,7 @@ struct bma400_data {
> > >  	int scale;
> > >  	struct iio_trigger *trig;
> > >  	int steps_enabled;
> > > +	bool step_event_en;
> > >  	/* Correct time stamp alignment */
> > >  	struct {
> > >  		__le16 buff[3];
> > > @@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> > >  	{ }
> > >  };
> > >  
> > > +static const struct iio_event_spec bma400_step_detect_event = {
> > > +	.type = IIO_EV_TYPE_CHANGE,
> > > +	.dir = IIO_EV_DIR_NONE,
> > > +	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > > +};
> > > +
> > >  #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > >  	.type = IIO_ACCEL, \
> > >  	.modified = 1, \
> > > @@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
> > >  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > >  				      BIT(IIO_CHAN_INFO_ENABLE),
> > >  		.scan_index = -1, /* No buffer support */
> > > +		.event_spec = &bma400_step_detect_event,
> > > +		.num_event_specs = 1,
> > >  	},
> > >  	IIO_CHAN_SOFT_TIMESTAMP(4),
> > >  };
> > > @@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> > >  	}
> > >  }
> > >  
> > > +static int bma400_read_event_config(struct iio_dev *indio_dev,
> > > +				    const struct iio_chan_spec *chan,
> > > +				    enum iio_event_type type,
> > > +				    enum iio_event_direction dir)
> > > +{
> > > +	struct bma400_data *data = iio_priv(indio_dev);
> > > +
> > > +	switch (type) {
> > > +	case IIO_EV_TYPE_CHANGE:
> > > +		return data->step_event_en;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > +				     const struct iio_chan_spec *chan,
> > > +				     enum iio_event_type type,
> > > +				     enum iio_event_direction dir, int state)
> > > +{
> > > +	int ret;
> > > +	struct bma400_data *data = iio_priv(indio_dev);
> > > +
> > > +	switch (type) {
> > > +	case IIO_EV_TYPE_CHANGE:
> > > +		mutex_lock(&data->mutex);
> > > +		if (!data->steps_enabled) {
> > > +			ret = regmap_update_bits(data->regmap,
> > > +						 BMA400_INT_CONFIG1_REG,
> > > +						 BMA400_STEP_INT_MSK,
> > > +						 FIELD_PREP(BMA400_STEP_INT_MSK,
> > > +							    1));
> > > +			if (ret)
> > > +				return ret;
> > > +			data->steps_enabled = 1;
> > > +		}
> > > +
> > > +		ret = regmap_update_bits(data->regmap,
> > > +					 BMA400_INT12_MAP_REG,
> > > +					 BMA400_STEP_INT_MSK,
> > > +					 FIELD_PREP(BMA400_STEP_INT_MSK,
> > > +						    state));
> > > +		mutex_unlock(&data->mutex);
> > > +		if (ret)
> > > +			return ret;
> > > +		data->step_event_en = state;
> > > +		return 0;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > >  static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> > >  					     bool state)
> > >  {
> > > @@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
> > >  	.read_avail        = bma400_read_avail,
> > >  	.write_raw         = bma400_write_raw,
> > >  	.write_raw_get_fmt = bma400_write_raw_get_fmt,
> > > +	.read_event_config = bma400_read_event_config,
> > > +	.write_event_config = bma400_write_event_config,
> > >  };
> > >  
> > >  static const struct iio_trigger_ops bma400_trigger_ops = {
> > > @@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> > >  		ret = IRQ_HANDLED;
> > >  	}
> > >  
> > > +	if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
> > > +		iio_push_event(indio_dev,
> > > +			       IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> > > +					      IIO_EV_DIR_NONE,
> > > +					      IIO_EV_TYPE_CHANGE, 0, 0, 0),
> > > +			       iio_get_time_ns(indio_dev));
> > > +		ret = IRQ_HANDLED;
> > > +	}
> > > +
> > >  	return ret;
> > >  }
> > >    
> >   

Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event

[Jagath Jog J]

On Sat, Apr 02, 2022 at 05:37:07PM +0100, Jonathan Cameron wrote:
> On Tue, 29 Mar 2022 02:07:11 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
> 
> > Hi Jonathan,
> > 
> > On Sun, Mar 27, 2022 at 05:50:36PM +0100, Jonathan Cameron wrote:
> > > On Sun, 27 Mar 2022 01:11:46 +0530
> > > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> > >   
> > > > Added support for event when there is a detection of step change.
> > > > INT1 pin is used to interrupt and event is pushed to userspace.
> > > > 
> > > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>  
> > > 
> > > These last two patches look fine to me.  Simply having the
> > > event enable the channel makes things simpler.  
> > 
> > Means do I need to drop the step _INFO_ENABLE and handle the
> > enabling and disabling of step channel through the event enable and
> > disable?
> 
> No.  I was trying to say I like the solution you have now.

Thanks, I will keep the solution same.
Currently I am testing the BMA400 activity events like STILL, WALKING,
RUNNING and also BMA400 acceleration threshold events, soon I will send the
next v3 patch series by including these events.

> 
> > 
> > > I briefly wondered if we need to care about sequences like
> > > 
> > > 1) Enable event
> > > 2) Enable channel (already enabled, but perhaps this indicates separate intent)
> > > 3) Disable event.
> > > 4) Is the channel still enabled?
> > > 
> > > or the simpler case of whether we should disable the channel if the event is
> > > disabled and it wasn't otherwise turned on.
> > > 
> > > However, I can't see a sensible way to do so. Hence I think what you have
> > > gone with is the best we can do.
> > > 
> > > Thanks,
> > > 
> > > Jonathan  
> > 
> > Thanks for reviewing the patch series. I will also address all the comments
> > from Andy in the next patch v3.
> > 
> > Thank you
> > Jagath
> > >   
> > > > ---
> > > >  drivers/iio/accel/bma400.h      |  2 +
> > > >  drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
> > > >  2 files changed, 75 insertions(+)
> > > > 
> > > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > > index c9b856b37021..c4ec0cf6dc00 100644
> > > > --- a/drivers/iio/accel/bma400.h
> > > > +++ b/drivers/iio/accel/bma400.h
> > > > @@ -39,6 +39,7 @@
> > > >  #define BMA400_INT_STAT0_REG        0x0e
> > > >  #define BMA400_INT_STAT1_REG        0x0f
> > > >  #define BMA400_INT_STAT2_REG        0x10
> > > > +#define BMA400_INT12_MAP_REG	    0x23
> > > >  
> > > >  /* Temperature register */
> > > >  #define BMA400_TEMP_DATA_REG        0x11
> > > > @@ -54,6 +55,7 @@
> > > >  #define BMA400_STEP_CNT3_REG        0x17
> > > >  #define BMA400_STEP_STAT_REG        0x18
> > > >  #define BMA400_STEP_INT_MSK	    BIT(0)
> > > > +#define BMA400_STEP_STAT_MASK	    GENMASK(9, 8)
> > > >  
> > > >  /*
> > > >   * Read-write configuration registers
> > > > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > > > index ec2f9c380bda..aaa104a2698b 100644
> > > > --- a/drivers/iio/accel/bma400_core.c
> > > > +++ b/drivers/iio/accel/bma400_core.c
> > > > @@ -24,6 +24,7 @@
> > > >  #include <linux/iio/iio.h>
> > > >  #include <linux/iio/sysfs.h>
> > > >  #include <linux/iio/buffer.h>
> > > > +#include <linux/iio/events.h>
> > > >  #include <linux/iio/trigger.h>
> > > >  #include <linux/iio/trigger_consumer.h>
> > > >  #include <linux/iio/triggered_buffer.h>
> > > > @@ -70,6 +71,7 @@ struct bma400_data {
> > > >  	int scale;
> > > >  	struct iio_trigger *trig;
> > > >  	int steps_enabled;
> > > > +	bool step_event_en;
> > > >  	/* Correct time stamp alignment */
> > > >  	struct {
> > > >  		__le16 buff[3];
> > > > @@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> > > >  	{ }
> > > >  };
> > > >  
> > > > +static const struct iio_event_spec bma400_step_detect_event = {
> > > > +	.type = IIO_EV_TYPE_CHANGE,
> > > > +	.dir = IIO_EV_DIR_NONE,
> > > > +	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > > > +};
> > > > +
> > > >  #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > >  	.type = IIO_ACCEL, \
> > > >  	.modified = 1, \
> > > > @@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
> > > >  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > >  				      BIT(IIO_CHAN_INFO_ENABLE),
> > > >  		.scan_index = -1, /* No buffer support */
> > > > +		.event_spec = &bma400_step_detect_event,
> > > > +		.num_event_specs = 1,
> > > >  	},
> > > >  	IIO_CHAN_SOFT_TIMESTAMP(4),
> > > >  };
> > > > @@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> > > >  	}
> > > >  }
> > > >  
> > > > +static int bma400_read_event_config(struct iio_dev *indio_dev,
> > > > +				    const struct iio_chan_spec *chan,
> > > > +				    enum iio_event_type type,
> > > > +				    enum iio_event_direction dir)
> > > > +{
> > > > +	struct bma400_data *data = iio_priv(indio_dev);
> > > > +
> > > > +	switch (type) {
> > > > +	case IIO_EV_TYPE_CHANGE:
> > > > +		return data->step_event_en;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > > +}
> > > > +
> > > > +static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > > +				     const struct iio_chan_spec *chan,
> > > > +				     enum iio_event_type type,
> > > > +				     enum iio_event_direction dir, int state)
> > > > +{
> > > > +	int ret;
> > > > +	struct bma400_data *data = iio_priv(indio_dev);
> > > > +
> > > > +	switch (type) {
> > > > +	case IIO_EV_TYPE_CHANGE:
> > > > +		mutex_lock(&data->mutex);
> > > > +		if (!data->steps_enabled) {
> > > > +			ret = regmap_update_bits(data->regmap,
> > > > +						 BMA400_INT_CONFIG1_REG,
> > > > +						 BMA400_STEP_INT_MSK,
> > > > +						 FIELD_PREP(BMA400_STEP_INT_MSK,
> > > > +							    1));
> > > > +			if (ret)
> > > > +				return ret;
> > > > +			data->steps_enabled = 1;
> > > > +		}
> > > > +
> > > > +		ret = regmap_update_bits(data->regmap,
> > > > +					 BMA400_INT12_MAP_REG,
> > > > +					 BMA400_STEP_INT_MSK,
> > > > +					 FIELD_PREP(BMA400_STEP_INT_MSK,
> > > > +						    state));
> > > > +		mutex_unlock(&data->mutex);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +		data->step_event_en = state;
> > > > +		return 0;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > > +}
> > > > +
> > > >  static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> > > >  					     bool state)
> > > >  {
> > > > @@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
> > > >  	.read_avail        = bma400_read_avail,
> > > >  	.write_raw         = bma400_write_raw,
> > > >  	.write_raw_get_fmt = bma400_write_raw_get_fmt,
> > > > +	.read_event_config = bma400_read_event_config,
> > > > +	.write_event_config = bma400_write_event_config,
> > > >  };
> > > >  
> > > >  static const struct iio_trigger_ops bma400_trigger_ops = {
> > > > @@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> > > >  		ret = IRQ_HANDLED;
> > > >  	}
> > > >  
> > > > +	if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
> > > > +		iio_push_event(indio_dev,
> > > > +			       IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> > > > +					      IIO_EV_DIR_NONE,
> > > > +					      IIO_EV_TYPE_CHANGE, 0, 0, 0),
> > > > +			       iio_get_time_ns(indio_dev));
> > > > +		ret = IRQ_HANDLED;
> > > > +	}
> > > > +
> > > >  	return ret;
> > > >  }
> > > >    
> > >   
>