* [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality
@ 2024-10-15 9:20 Hardevsinh Palaniya
2024-10-16 9:25 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Hardevsinh Palaniya @ 2024-10-15 9:20 UTC (permalink / raw)
To: jic23, jmaneyrol, lars; +Cc: Hardevsinh Palaniya, linux-iio, linux-kernel
Enables pedometer functionality in the ICM42605 IMU sensor.
The pedometer feature allows for step counting, which is accessible through
a new sysfs entry. Interrupts are triggered when a step event occurs, enabling
step event detection.
Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
---
drivers/iio/imu/inv_icm42600/inv_icm42600.h | 16 ++
.../iio/imu/inv_icm42600/inv_icm42600_accel.c | 165 ++++++++++++++++++
.../iio/imu/inv_icm42600/inv_icm42600_core.c | 36 +++-
3 files changed, 211 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 3a07e43e4cf1..c3471b73152e 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -122,6 +122,7 @@ struct inv_icm42600_sensor_conf {
int filter;
};
#define INV_ICM42600_SENSOR_CONF_INIT {-1, -1, -1, -1}
+#define INV_ICM42600_SENSOR_CONF_APEX { 2, 0, 9, 6}
struct inv_icm42600_conf {
struct inv_icm42600_sensor_conf gyro;
@@ -141,6 +142,8 @@ struct inv_icm42600_suspended {
* @chip: chip identifier.
* @name: chip name.
* @map: regmap pointer.
+ * @pedometer_enable status of pedometer function
+ * @pedometer_value status of steps event occurnce
* @vdd_supply: VDD voltage regulator for the chip.
* @vddio_supply: I/O voltage regulator for the chip.
* @orientation: sensor chip orientation relative to main hardware.
@@ -157,6 +160,8 @@ struct inv_icm42600_state {
enum inv_icm42600_chip chip;
const char *name;
struct regmap *map;
+ bool pedometer_enable;
+ bool pedometer_value;
struct regulator *vdd_supply;
struct regulator *vddio_supply;
struct iio_mount_matrix orientation;
@@ -301,6 +306,15 @@ struct inv_icm42600_sensor_state {
#define INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(_f) \
FIELD_PREP(GENMASK(3, 0), (_f))
+/* Pedometer functionality */
+#define INV_ICM42600_REG_APEX_CONFIG0 0x0056
+#define INV_ICM42600_DMP_ODR_50Hz BIT(1)
+#define INV_ICM42600_PED_ENABLE BIT(5)
+#define INV_ICM42600_REG_INT_STATUS3 0x0038
+#define INV_ICM42600_STEP_DET_INT BIT(5)
+#define INV_ICM42600_REG_APEX_DATA 0x0031 // 2 Byte little-endian
+
+
#define INV_ICM42600_REG_TMST_CONFIG 0x0054
#define INV_ICM42600_TMST_CONFIG_MASK GENMASK(4, 0)
#define INV_ICM42600_TMST_CONFIG_TMST_TO_REGS_EN BIT(4)
@@ -373,6 +387,8 @@ struct inv_icm42600_sensor_state {
#define INV_ICM42600_INTF_CONFIG6_I3C_SDR_EN BIT(0)
/* User bank 4 (MSB 0x40) */
+#define INV_ICM42600_REG_INT_SOURCE6 0x404D
+#define INV_ICM42600_STEP_DET_INT1_EN BIT(5)
#define INV_ICM42600_REG_INT_SOURCE8 0x404F
#define INV_ICM42600_INT_SOURCE8_FSYNC_IBI_EN BIT(5)
#define INV_ICM42600_INT_SOURCE8_PLL_RDY_IBI_EN BIT(4)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 56ac19814250..90fe4c9e15ab 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -160,6 +160,13 @@ static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {
{},
};
+static const struct iio_event_spec icm42600_step_event = {
+ .type = IIO_EV_TYPE_CHANGE,
+ .dir = IIO_EV_DIR_NONE,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_VALUE),
+};
+
static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
INV_ICM42600_ACCEL_CHAN(IIO_MOD_X, INV_ICM42600_ACCEL_SCAN_X,
inv_icm42600_accel_ext_infos),
@@ -169,6 +176,14 @@ static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
inv_icm42600_accel_ext_infos),
INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),
IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),
+ {
+ .type = IIO_STEPS,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .scan_index = -1,
+ .event_spec = &icm42600_step_event,
+ .num_event_specs = 1,
+ },
+
};
/*
@@ -668,6 +683,31 @@ static int inv_icm42600_accel_write_offset(struct inv_icm42600_state *st,
return ret;
}
+static int inv_icm42600_steps_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ __le16 steps;
+ int ret;
+
+ if (mask == IIO_CHAN_INFO_PROCESSED) {
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+ ret = regmap_bulk_read(st->map, INV_ICM42600_REG_APEX_DATA, &steps, sizeof(steps));
+ if (ret)
+ return ret;
+ iio_device_release_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+ *val = steps;
+ return IIO_VAL_INT;
+ }
+
+ return -EINVAL;
+}
+
static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -681,6 +721,8 @@ static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev,
break;
case IIO_TEMP:
return inv_icm42600_temp_read_raw(indio_dev, chan, val, val2, mask);
+ case IIO_STEPS:
+ return inv_icm42600_steps_read_raw(indio_dev, chan, val, val2, mask);
default:
return -EINVAL;
}
@@ -824,6 +866,126 @@ static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,
return ret;
}
+/*****************Pedometer Functionality**************/
+static int inv_icm42600_step_en(struct inv_icm42600_state *st, int state)
+{
+ struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_APEX;
+ int ret, value;
+
+ if (state) {
+
+ ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0,
+ INV_ICM42600_DMP_ODR_50Hz);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
+ INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET);
+ if (ret)
+ return ret;
+ msleep(1);
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
+ INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_INT_SOURCE6,
+ INV_ICM42600_STEP_DET_INT1_EN);
+ if (ret)
+ return ret;
+
+ value = INV_ICM42600_DMP_ODR_50Hz | INV_ICM42600_PED_ENABLE;
+ ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0, value);
+ if (ret)
+ return ret;
+
+ st->pedometer_enable = true;
+
+ } else {
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_INT_SOURCE6, 0);
+ if (ret)
+ return ret;
+
+ st->pedometer_enable = false;
+ }
+
+ return 0;
+}
+
+static int inv_icm42600_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)
+{
+ struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ int ret;
+
+ if(chan->type != IIO_STEPS)
+ return -EINVAL;
+
+ mutex_lock(&st->lock);
+
+ ret = inv_icm42600_step_en(st, state);
+
+ mutex_unlock(&st->lock);
+ return ret;
+}
+
+static int inv_icm42600_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ int value;
+
+ if (chan->type != IIO_STEPS)
+ return -EINVAL;
+
+ regmap_read(st->map, INV_ICM42600_REG_APEX_CONFIG0, &value);
+
+ if (value & INV_ICM42600_PED_ENABLE)
+ return 1;
+ else
+ return 0;
+}
+
+static int inv_icm42600_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+
+ mutex_lock(&st->lock);
+
+ if (type == IIO_EV_TYPE_CHANGE) {
+ if (st->pedometer_value == true) {
+ *val = 1;
+ st->pedometer_value = false;
+ } else {
+ *val = 0;
+ }
+ mutex_unlock(&st->lock);
+ return IIO_VAL_INT;
+ }
+
+ mutex_unlock(&st->lock);
+ return -EINVAL;
+}
+
static const struct iio_info inv_icm42600_accel_info = {
.read_raw = inv_icm42600_accel_read_raw,
.read_avail = inv_icm42600_accel_read_avail,
@@ -833,6 +995,9 @@ static const struct iio_info inv_icm42600_accel_info = {
.update_scan_mode = inv_icm42600_accel_update_scan_mode,
.hwfifo_set_watermark = inv_icm42600_accel_hwfifo_set_watermark,
.hwfifo_flush_to_buffer = inv_icm42600_accel_hwfifo_flush,
+ .write_event_config = inv_icm42600_write_event_config,
+ .read_event_config = inv_icm42600_read_event_config,
+ .read_event_value = inv_icm42600_read_event_value,
};
struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index c3924cc6190e..4d78cb5ca396 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -15,7 +15,8 @@
#include <linux/pm_runtime.h>
#include <linux/property.h>
#include <linux/regmap.h>
-
+#include <linux/iio/events.h>
+#include <linux/of_irq.h>
#include <linux/iio/iio.h>
#include "inv_icm42600.h"
@@ -533,6 +534,19 @@ static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
mutex_lock(&st->lock);
+ ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS3, &status);
+ if (ret)
+ goto out_unlock;
+
+ if (status & INV_ICM42600_STEP_DET_INT) {
+ iio_push_event(st->indio_accel, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
+ IIO_NO_MOD,
+ IIO_EV_TYPE_CHANGE,
+ IIO_EV_DIR_NONE),
+ st->timestamp.accel);
+ st->pedometer_value = true;
+ }
+
ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS, &status);
if (ret)
goto out_unlock;
@@ -860,12 +876,20 @@ static int inv_icm42600_runtime_suspend(struct device *dev)
mutex_lock(&st->lock);
/* disable all sensors */
- ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
- INV_ICM42600_SENSOR_MODE_OFF, false,
- NULL);
- if (ret)
- goto error_unlock;
+ if (st->pedometer_enable) {
+ ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
+ INV_ICM42600_SENSOR_MODE_LOW_POWER,
+ false, NULL);
+ if (ret)
+ goto error_unlock;
+ } else {
+ ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
+ INV_ICM42600_SENSOR_MODE_OFF,
+ false, NULL);
+ if (ret)
+ goto error_unlock;
+ }
regulator_disable(st->vddio_supply);
error_unlock:
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality
2024-10-15 9:20 [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality Hardevsinh Palaniya
@ 2024-10-16 9:25 ` kernel test robot
2024-10-17 7:45 ` Jean-Baptiste Maneyrol
2024-10-18 18:21 ` Jonathan Cameron
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2024-10-16 9:25 UTC (permalink / raw)
To: Hardevsinh Palaniya, jic23, jmaneyrol, lars
Cc: oe-kbuild-all, Hardevsinh Palaniya, linux-iio, linux-kernel
Hi Hardevsinh,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.12-rc3 next-20241016]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hardevsinh-Palaniya/iio-imu-inv_icm42600-Enable-Pedometer-Functionality/20241015-172227
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20241015092035.10482-1-hardevsinh.palaniya%40siliconsignals.io
patch subject: [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality
config: i386-randconfig-062-20241016 (https://download.01.org/0day-ci/archive/20241016/202410161606.EbqeKmdm-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241016/202410161606.EbqeKmdm-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410161606.EbqeKmdm-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c:704:21: sparse: sparse: incorrect type in assignment (different base types) @@ expected int @@ got restricted __le16 [addressable] [usertype] steps @@
drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c:704:21: sparse: expected int
drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c:704:21: sparse: got restricted __le16 [addressable] [usertype] steps
vim +704 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
685
686 static int inv_icm42600_steps_read_raw(struct iio_dev *indio_dev,
687 struct iio_chan_spec const *chan,
688 int *val, int *val2, long mask)
689 {
690 struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
691 __le16 steps;
692 int ret;
693
694 if (mask == IIO_CHAN_INFO_PROCESSED) {
695 ret = iio_device_claim_direct_mode(indio_dev);
696 if (ret)
697 return ret;
698 ret = regmap_bulk_read(st->map, INV_ICM42600_REG_APEX_DATA, &steps, sizeof(steps));
699 if (ret)
700 return ret;
701 iio_device_release_direct_mode(indio_dev);
702 if (ret)
703 return ret;
> 704 *val = steps;
705 return IIO_VAL_INT;
706 }
707
708 return -EINVAL;
709 }
710
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality
2024-10-15 9:20 [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality Hardevsinh Palaniya
2024-10-16 9:25 ` kernel test robot
@ 2024-10-17 7:45 ` Jean-Baptiste Maneyrol
2024-10-18 12:09 ` Hardevsinh Palaniya
2024-10-18 18:21 ` Jonathan Cameron
2 siblings, 1 reply; 5+ messages in thread
From: Jean-Baptiste Maneyrol @ 2024-10-17 7:45 UTC (permalink / raw)
To: Hardevsinh Palaniya, jic23@kernel.org, lars@metafoo.de
Cc: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Hello,
thanks for the patch, but for me there are multiple important issues there.
You cannot prevent runtime_suspend to turn the chip off since it is its purpose. If you want to keep chip on when pedometer is enabled, you need to use pm_runtime_resume_and_get() when turning pedometer on, and pm_runtime_mark_last_busy()+pm_runtime_put_autosuspend() when turning pedometer off. This way you ensure the chip stays on while pedometer is turned on.
The 2nd important issue, is the ODR frequency for accel with pedometer. Pedometer requires accel to run at 50Hz minimum, otherwise it will not work correctly. I think we need to enforce this inside the driver, otherwise userspace may not understand why pedometer is not working correctly. We could prevent pedometer from running if sampling_frequency is below 50Hz, or force frequency to 50Hz. And when pedometer is running, preventing ODR switch to lower than 50Hz.
Anyway, I'm currently working on adding Wake-on-Motion (WoM) feature to the driver. WoM can also be used with pedometer to reduce power consumption. It would be better to postpone this pedometer work and wait for WoM integration before adding pedometer support.
Thanks,
JB
________________________________________
From: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
Sent: Tuesday, October 15, 2024 11:20
To: jic23@kernel.org <jic23@kernel.org>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; lars@metafoo.de <lars@metafoo.de>
Cc: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality
This Message Is From an External Sender
This message came from outside your organization.
Enables pedometer functionality in the ICM42605 IMU sensor.
The pedometer feature allows for step counting, which is accessible through
a new sysfs entry. Interrupts are triggered when a step event occurs, enabling
step event detection.
Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
---
drivers/iio/imu/inv_icm42600/inv_icm42600.h | 16 ++
.../iio/imu/inv_icm42600/inv_icm42600_accel.c | 165 ++++++++++++++++++
.../iio/imu/inv_icm42600/inv_icm42600_core.c | 36 +++-
3 files changed, 211 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 3a07e43e4cf1..c3471b73152e 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -122,6 +122,7 @@ struct inv_icm42600_sensor_conf {
int filter;
};
#define INV_ICM42600_SENSOR_CONF_INIT {-1, -1, -1, -1}
+#define INV_ICM42600_SENSOR_CONF_APEX { 2, 0, 9, 6}
struct inv_icm42600_conf {
struct inv_icm42600_sensor_conf gyro;
@@ -141,6 +142,8 @@ struct inv_icm42600_suspended {
* @chip: chip identifier.
* @name: chip name.
* @map: regmap pointer.
+ * @pedometer_enable status of pedometer function
+ * @pedometer_value status of steps event occurnce
* @vdd_supply: VDD voltage regulator for the chip.
* @vddio_supply: I/O voltage regulator for the chip.
* @orientation: sensor chip orientation relative to main hardware.
@@ -157,6 +160,8 @@ struct inv_icm42600_state {
enum inv_icm42600_chip chip;
const char *name;
struct regmap *map;
+ bool pedometer_enable;
+ bool pedometer_value;
struct regulator *vdd_supply;
struct regulator *vddio_supply;
struct iio_mount_matrix orientation;
@@ -301,6 +306,15 @@ struct inv_icm42600_sensor_state {
#define INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(_f) \
FIELD_PREP(GENMASK(3, 0), (_f))
+/* Pedometer functionality */
+#define INV_ICM42600_REG_APEX_CONFIG0 0x0056
+#define INV_ICM42600_DMP_ODR_50Hz BIT(1)
+#define INV_ICM42600_PED_ENABLE BIT(5)
+#define INV_ICM42600_REG_INT_STATUS3 0x0038
+#define INV_ICM42600_STEP_DET_INT BIT(5)
+#define INV_ICM42600_REG_APEX_DATA 0x0031 // 2 Byte little-endian
+
+
#define INV_ICM42600_REG_TMST_CONFIG 0x0054
#define INV_ICM42600_TMST_CONFIG_MASK GENMASK(4, 0)
#define INV_ICM42600_TMST_CONFIG_TMST_TO_REGS_EN BIT(4)
@@ -373,6 +387,8 @@ struct inv_icm42600_sensor_state {
#define INV_ICM42600_INTF_CONFIG6_I3C_SDR_EN BIT(0)
/* User bank 4 (MSB 0x40) */
+#define INV_ICM42600_REG_INT_SOURCE6 0x404D
+#define INV_ICM42600_STEP_DET_INT1_EN BIT(5)
#define INV_ICM42600_REG_INT_SOURCE8 0x404F
#define INV_ICM42600_INT_SOURCE8_FSYNC_IBI_EN BIT(5)
#define INV_ICM42600_INT_SOURCE8_PLL_RDY_IBI_EN BIT(4)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 56ac19814250..90fe4c9e15ab 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -160,6 +160,13 @@ static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {
{},
};
+static const struct iio_event_spec icm42600_step_event = {
+ .type = IIO_EV_TYPE_CHANGE,
+ .dir = IIO_EV_DIR_NONE,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_VALUE),
+};
+
static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
INV_ICM42600_ACCEL_CHAN(IIO_MOD_X, INV_ICM42600_ACCEL_SCAN_X,
inv_icm42600_accel_ext_infos),
@@ -169,6 +176,14 @@ static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
inv_icm42600_accel_ext_infos),
INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),
IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),
+ {
+ .type = IIO_STEPS,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .scan_index = -1,
+ .event_spec = &icm42600_step_event,
+ .num_event_specs = 1,
+ },
+
};
/*
@@ -668,6 +683,31 @@ static int inv_icm42600_accel_write_offset(struct inv_icm42600_state *st,
return ret;
}
+static int inv_icm42600_steps_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ __le16 steps;
+ int ret;
+
+ if (mask == IIO_CHAN_INFO_PROCESSED) {
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+ ret = regmap_bulk_read(st->map, INV_ICM42600_REG_APEX_DATA, &steps, sizeof(steps));
+ if (ret)
+ return ret;
+ iio_device_release_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+ *val = steps;
+ return IIO_VAL_INT;
+ }
+
+ return -EINVAL;
+}
+
static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -681,6 +721,8 @@ static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev,
break;
case IIO_TEMP:
return inv_icm42600_temp_read_raw(indio_dev, chan, val, val2, mask);
+ case IIO_STEPS:
+ return inv_icm42600_steps_read_raw(indio_dev, chan, val, val2, mask);
default:
return -EINVAL;
}
@@ -824,6 +866,126 @@ static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,
return ret;
}
+/*****************Pedometer Functionality**************/
+static int inv_icm42600_step_en(struct inv_icm42600_state *st, int state)
+{
+ struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_APEX;
+ int ret, value;
+
+ if (state) {
+
+ ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0,
+ INV_ICM42600_DMP_ODR_50Hz);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
+ INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET);
+ if (ret)
+ return ret;
+ msleep(1);
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
+ INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_INT_SOURCE6,
+ INV_ICM42600_STEP_DET_INT1_EN);
+ if (ret)
+ return ret;
+
+ value = INV_ICM42600_DMP_ODR_50Hz | INV_ICM42600_PED_ENABLE;
+ ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0, value);
+ if (ret)
+ return ret;
+
+ st->pedometer_enable = true;
+
+ } else {
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_INT_SOURCE6, 0);
+ if (ret)
+ return ret;
+
+ st->pedometer_enable = false;
+ }
+
+ return 0;
+}
+
+static int inv_icm42600_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)
+{
+ struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ int ret;
+
+ if(chan->type != IIO_STEPS)
+ return -EINVAL;
+
+ mutex_lock(&st->lock);
+
+ ret = inv_icm42600_step_en(st, state);
+
+ mutex_unlock(&st->lock);
+ return ret;
+}
+
+static int inv_icm42600_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ int value;
+
+ if (chan->type != IIO_STEPS)
+ return -EINVAL;
+
+ regmap_read(st->map, INV_ICM42600_REG_APEX_CONFIG0, &value);
+
+ if (value & INV_ICM42600_PED_ENABLE)
+ return 1;
+ else
+ return 0;
+}
+
+static int inv_icm42600_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+
+ mutex_lock(&st->lock);
+
+ if (type == IIO_EV_TYPE_CHANGE) {
+ if (st->pedometer_value == true) {
+ *val = 1;
+ st->pedometer_value = false;
+ } else {
+ *val = 0;
+ }
+ mutex_unlock(&st->lock);
+ return IIO_VAL_INT;
+ }
+
+ mutex_unlock(&st->lock);
+ return -EINVAL;
+}
+
static const struct iio_info inv_icm42600_accel_info = {
.read_raw = inv_icm42600_accel_read_raw,
.read_avail = inv_icm42600_accel_read_avail,
@@ -833,6 +995,9 @@ static const struct iio_info inv_icm42600_accel_info = {
.update_scan_mode = inv_icm42600_accel_update_scan_mode,
.hwfifo_set_watermark = inv_icm42600_accel_hwfifo_set_watermark,
.hwfifo_flush_to_buffer = inv_icm42600_accel_hwfifo_flush,
+ .write_event_config = inv_icm42600_write_event_config,
+ .read_event_config = inv_icm42600_read_event_config,
+ .read_event_value = inv_icm42600_read_event_value,
};
struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index c3924cc6190e..4d78cb5ca396 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -15,7 +15,8 @@
#include <linux/pm_runtime.h>
#include <linux/property.h>
#include <linux/regmap.h>
-
+#include <linux/iio/events.h>
+#include <linux/of_irq.h>
#include <linux/iio/iio.h>
#include "inv_icm42600.h"
@@ -533,6 +534,19 @@ static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
mutex_lock(&st->lock);
+ ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS3, &status);
+ if (ret)
+ goto out_unlock;
+
+ if (status & INV_ICM42600_STEP_DET_INT) {
+ iio_push_event(st->indio_accel, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
+ IIO_NO_MOD,
+ IIO_EV_TYPE_CHANGE,
+ IIO_EV_DIR_NONE),
+ st->timestamp.accel);
+ st->pedometer_value = true;
+ }
+
ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS, &status);
if (ret)
goto out_unlock;
@@ -860,12 +876,20 @@ static int inv_icm42600_runtime_suspend(struct device *dev)
mutex_lock(&st->lock);
/* disable all sensors */
- ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
- INV_ICM42600_SENSOR_MODE_OFF, false,
- NULL);
- if (ret)
- goto error_unlock;
+ if (st->pedometer_enable) {
+ ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
+ INV_ICM42600_SENSOR_MODE_LOW_POWER,
+ false, NULL);
+ if (ret)
+ goto error_unlock;
+ } else {
+ ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
+ INV_ICM42600_SENSOR_MODE_OFF,
+ false, NULL);
+ if (ret)
+ goto error_unlock;
+ }
regulator_disable(st->vddio_supply);
error_unlock:
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality
2024-10-17 7:45 ` Jean-Baptiste Maneyrol
@ 2024-10-18 12:09 ` Hardevsinh Palaniya
0 siblings, 0 replies; 5+ messages in thread
From: Hardevsinh Palaniya @ 2024-10-18 12:09 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol
Cc: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
Jonathan Cameron, Lars-Peter Clausen
Hi Jean,
Thank you for your suggestions.
>thanks for the patch, but for me there are multiple important issues there.
>
>You cannot prevent runtime_suspend to turn the chip off since it is its purpose. If you want to keep chip on when pedometer is enabled, you need to use >pm_runtime_resume_and_get() when turning pedometer on, and pm_runtime_mark_last_busy()+pm_runtime_put_autosuspend() when turning pedometer off. >This way you ensure the chip stays on while pedometer is turned on.
I agree, will change it
>The 2nd important issue, is the ODR frequency for accel with pedometer. Pedometer requires accel to run at 50Hz minimum, otherwise it will not work correctly. >I think we need to enforce this inside the driver, otherwise userspace may not understand why pedometer is not working correctly. We could prevent >pedometer from running if sampling_frequency is below 50Hz, or force frequency to 50Hz. And when pedometer is running, preventing ODR switch to lower >than 50Hz.
That makes sense. it could create problems
>Anyway, I'm currently working on adding Wake-on-Motion (WoM) feature to the driver. WoM can also be used with pedometer to reduce power consumption. >It would be better to postpone this pedometer work and wait for WoM integration before adding pedometer support.
That's great to hear
Could you please let me know when the integration will be completed?
I am also working on the tilt functionality, which is nearly finished.
Should I continue with this work and submit a patch for both the pedometer and tilt features after the WoM integration, or would you recommend dropping this plan?
Best Regards,
Hardev
________________________________________
From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
Sent: Thursday, October 17, 2024 1:15 PM
To: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>
Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality
CAUTION: This email originated from outside the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
Hello,
thanks for the patch, but for me there are multiple important issues there.
You cannot prevent runtime_suspend to turn the chip off since it is its purpose. If you want to keep chip on when pedometer is enabled, you need to use pm_runtime_resume_and_get() when turning pedometer on, and pm_runtime_mark_last_busy()+pm_runtime_put_autosuspend() when turning pedometer off. This way you ensure the chip stays on while pedometer is turned on.
The 2nd important issue, is the ODR frequency for accel with pedometer. Pedometer requires accel to run at 50Hz minimum, otherwise it will not work correctly. I think we need to enforce this inside the driver, otherwise userspace may not understand why pedometer is not working correctly. We could prevent pedometer from running if sampling_frequency is below 50Hz, or force frequency to 50Hz. And when pedometer is running, preventing ODR switch to lower than 50Hz.
Anyway, I'm currently working on adding Wake-on-Motion (WoM) feature to the driver. WoM can also be used with pedometer to reduce power consumption. It would be better to postpone this pedometer work and wait for WoM integration before adding pedometer support.
Thanks,
JB
________________________________________
From: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
Sent: Tuesday, October 15, 2024 11:20
To: jic23@kernel.org <jic23@kernel.org>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; lars@metafoo.de <lars@metafoo.de>
Cc: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality
This Message Is From an External Sender
This message came from outside your organization.
Enables pedometer functionality in the ICM42605 IMU sensor.
The pedometer feature allows for step counting, which is accessible through
a new sysfs entry. Interrupts are triggered when a step event occurs, enabling
step event detection.
Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
---
drivers/iio/imu/inv_icm42600/inv_icm42600.h | 16 ++
.../iio/imu/inv_icm42600/inv_icm42600_accel.c | 165 ++++++++++++++++++
.../iio/imu/inv_icm42600/inv_icm42600_core.c | 36 +++-
3 files changed, 211 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 3a07e43e4cf1..c3471b73152e 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -122,6 +122,7 @@ struct inv_icm42600_sensor_conf {
int filter;
};
#define INV_ICM42600_SENSOR_CONF_INIT {-1, -1, -1, -1}
+#define INV_ICM42600_SENSOR_CONF_APEX { 2, 0, 9, 6}
struct inv_icm42600_conf {
struct inv_icm42600_sensor_conf gyro;
@@ -141,6 +142,8 @@ struct inv_icm42600_suspended {
* @chip: chip identifier.
* @name: chip name.
* @map: regmap pointer.
+ * @pedometer_enable status of pedometer function
+ * @pedometer_value status of steps event occurnce
* @vdd_supply: VDD voltage regulator for the chip.
* @vddio_supply: I/O voltage regulator for the chip.
* @orientation: sensor chip orientation relative to main hardware.
@@ -157,6 +160,8 @@ struct inv_icm42600_state {
enum inv_icm42600_chip chip;
const char *name;
struct regmap *map;
+ bool pedometer_enable;
+ bool pedometer_value;
struct regulator *vdd_supply;
struct regulator *vddio_supply;
struct iio_mount_matrix orientation;
@@ -301,6 +306,15 @@ struct inv_icm42600_sensor_state {
#define INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(_f) \
FIELD_PREP(GENMASK(3, 0), (_f))
+/* Pedometer functionality */
+#define INV_ICM42600_REG_APEX_CONFIG0 0x0056
+#define INV_ICM42600_DMP_ODR_50Hz BIT(1)
+#define INV_ICM42600_PED_ENABLE BIT(5)
+#define INV_ICM42600_REG_INT_STATUS3 0x0038
+#define INV_ICM42600_STEP_DET_INT BIT(5)
+#define INV_ICM42600_REG_APEX_DATA 0x0031 // 2 Byte little-endian
+
+
#define INV_ICM42600_REG_TMST_CONFIG 0x0054
#define INV_ICM42600_TMST_CONFIG_MASK GENMASK(4, 0)
#define INV_ICM42600_TMST_CONFIG_TMST_TO_REGS_EN BIT(4)
@@ -373,6 +387,8 @@ struct inv_icm42600_sensor_state {
#define INV_ICM42600_INTF_CONFIG6_I3C_SDR_EN BIT(0)
/* User bank 4 (MSB 0x40) */
+#define INV_ICM42600_REG_INT_SOURCE6 0x404D
+#define INV_ICM42600_STEP_DET_INT1_EN BIT(5)
#define INV_ICM42600_REG_INT_SOURCE8 0x404F
#define INV_ICM42600_INT_SOURCE8_FSYNC_IBI_EN BIT(5)
#define INV_ICM42600_INT_SOURCE8_PLL_RDY_IBI_EN BIT(4)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 56ac19814250..90fe4c9e15ab 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -160,6 +160,13 @@ static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {
{},
};
+static const struct iio_event_spec icm42600_step_event = {
+ .type = IIO_EV_TYPE_CHANGE,
+ .dir = IIO_EV_DIR_NONE,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_VALUE),
+};
+
static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
INV_ICM42600_ACCEL_CHAN(IIO_MOD_X, INV_ICM42600_ACCEL_SCAN_X,
inv_icm42600_accel_ext_infos),
@@ -169,6 +176,14 @@ static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
inv_icm42600_accel_ext_infos),
INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),
IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),
+ {
+ .type = IIO_STEPS,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .scan_index = -1,
+ .event_spec = &icm42600_step_event,
+ .num_event_specs = 1,
+ },
+
};
/*
@@ -668,6 +683,31 @@ static int inv_icm42600_accel_write_offset(struct inv_icm42600_state *st,
return ret;
}
+static int inv_icm42600_steps_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ __le16 steps;
+ int ret;
+
+ if (mask == IIO_CHAN_INFO_PROCESSED) {
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+ ret = regmap_bulk_read(st->map, INV_ICM42600_REG_APEX_DATA, &steps, sizeof(steps));
+ if (ret)
+ return ret;
+ iio_device_release_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+ *val = steps;
+ return IIO_VAL_INT;
+ }
+
+ return -EINVAL;
+}
+
static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -681,6 +721,8 @@ static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev,
break;
case IIO_TEMP:
return inv_icm42600_temp_read_raw(indio_dev, chan, val, val2, mask);
+ case IIO_STEPS:
+ return inv_icm42600_steps_read_raw(indio_dev, chan, val, val2, mask);
default:
return -EINVAL;
}
@@ -824,6 +866,126 @@ static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,
return ret;
}
+/*****************Pedometer Functionality**************/
+static int inv_icm42600_step_en(struct inv_icm42600_state *st, int state)
+{
+ struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_APEX;
+ int ret, value;
+
+ if (state) {
+
+ ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0,
+ INV_ICM42600_DMP_ODR_50Hz);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
+ INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET);
+ if (ret)
+ return ret;
+ msleep(1);
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
+ INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_INT_SOURCE6,
+ INV_ICM42600_STEP_DET_INT1_EN);
+ if (ret)
+ return ret;
+
+ value = INV_ICM42600_DMP_ODR_50Hz | INV_ICM42600_PED_ENABLE;
+ ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0, value);
+ if (ret)
+ return ret;
+
+ st->pedometer_enable = true;
+
+ } else {
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->map, INV_ICM42600_REG_INT_SOURCE6, 0);
+ if (ret)
+ return ret;
+
+ st->pedometer_enable = false;
+ }
+
+ return 0;
+}
+
+static int inv_icm42600_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)
+{
+ struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ int ret;
+
+ if(chan->type != IIO_STEPS)
+ return -EINVAL;
+
+ mutex_lock(&st->lock);
+
+ ret = inv_icm42600_step_en(st, state);
+
+ mutex_unlock(&st->lock);
+ return ret;
+}
+
+static int inv_icm42600_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ int value;
+
+ if (chan->type != IIO_STEPS)
+ return -EINVAL;
+
+ regmap_read(st->map, INV_ICM42600_REG_APEX_CONFIG0, &value);
+
+ if (value & INV_ICM42600_PED_ENABLE)
+ return 1;
+ else
+ return 0;
+}
+
+static int inv_icm42600_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+
+ mutex_lock(&st->lock);
+
+ if (type == IIO_EV_TYPE_CHANGE) {
+ if (st->pedometer_value == true) {
+ *val = 1;
+ st->pedometer_value = false;
+ } else {
+ *val = 0;
+ }
+ mutex_unlock(&st->lock);
+ return IIO_VAL_INT;
+ }
+
+ mutex_unlock(&st->lock);
+ return -EINVAL;
+}
+
static const struct iio_info inv_icm42600_accel_info = {
.read_raw = inv_icm42600_accel_read_raw,
.read_avail = inv_icm42600_accel_read_avail,
@@ -833,6 +995,9 @@ static const struct iio_info inv_icm42600_accel_info = {
.update_scan_mode = inv_icm42600_accel_update_scan_mode,
.hwfifo_set_watermark = inv_icm42600_accel_hwfifo_set_watermark,
.hwfifo_flush_to_buffer = inv_icm42600_accel_hwfifo_flush,
+ .write_event_config = inv_icm42600_write_event_config,
+ .read_event_config = inv_icm42600_read_event_config,
+ .read_event_value = inv_icm42600_read_event_value,
};
struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index c3924cc6190e..4d78cb5ca396 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -15,7 +15,8 @@
#include <linux/pm_runtime.h>
#include <linux/property.h>
#include <linux/regmap.h>
-
+#include <linux/iio/events.h>
+#include <linux/of_irq.h>
#include <linux/iio/iio.h>
#include "inv_icm42600.h"
@@ -533,6 +534,19 @@ static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
mutex_lock(&st->lock);
+ ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS3, &status);
+ if (ret)
+ goto out_unlock;
+
+ if (status & INV_ICM42600_STEP_DET_INT) {
+ iio_push_event(st->indio_accel, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
+ IIO_NO_MOD,
+ IIO_EV_TYPE_CHANGE,
+ IIO_EV_DIR_NONE),
+ st->timestamp.accel);
+ st->pedometer_value = true;
+ }
+
ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS, &status);
if (ret)
goto out_unlock;
@@ -860,12 +876,20 @@ static int inv_icm42600_runtime_suspend(struct device *dev)
mutex_lock(&st->lock);
/* disable all sensors */
- ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
- INV_ICM42600_SENSOR_MODE_OFF, false,
- NULL);
- if (ret)
- goto error_unlock;
+ if (st->pedometer_enable) {
+ ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
+ INV_ICM42600_SENSOR_MODE_LOW_POWER,
+ false, NULL);
+ if (ret)
+ goto error_unlock;
+ } else {
+ ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
+ INV_ICM42600_SENSOR_MODE_OFF,
+ false, NULL);
+ if (ret)
+ goto error_unlock;
+ }
regulator_disable(st->vddio_supply);
error_unlock:
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality
2024-10-15 9:20 [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality Hardevsinh Palaniya
2024-10-16 9:25 ` kernel test robot
2024-10-17 7:45 ` Jean-Baptiste Maneyrol
@ 2024-10-18 18:21 ` Jonathan Cameron
2 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2024-10-18 18:21 UTC (permalink / raw)
To: Hardevsinh Palaniya; +Cc: jmaneyrol, lars, linux-iio, linux-kernel
On Tue, 15 Oct 2024 14:50:03 +0530
Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> wrote:
> Enables pedometer functionality in the ICM42605 IMU sensor.
>
> The pedometer feature allows for step counting, which is accessible through
> a new sysfs entry. Interrupts are triggered when a step event occurs, enabling
> step event detection.
>
> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
Some additional comments from a quick read.
> ---
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 16 ++
> .../iio/imu/inv_icm42600/inv_icm42600_accel.c | 165 ++++++++++++++++++
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 36 +++-
> 3 files changed, 211 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 3a07e43e4cf1..c3471b73152e 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -122,6 +122,7 @@ struct inv_icm42600_sensor_conf {
> int filter;
> };
> #define INV_ICM42600_SENSOR_CONF_INIT {-1, -1, -1, -1}
> +#define INV_ICM42600_SENSOR_CONF_APEX { 2, 0, 9, 6}
>
> struct inv_icm42600_conf {
> struct inv_icm42600_sensor_conf gyro;
> @@ -141,6 +142,8 @@ struct inv_icm42600_suspended {
> * @chip: chip identifier.
> * @name: chip name.
> * @map: regmap pointer.
> + * @pedometer_enable status of pedometer function
> + * @pedometer_value status of steps event occurnce
Check the kernel-doc style. Even better run the script over the
files you are touching.
You are missing : here.
> * @vdd_supply: VDD voltage regulator for the chip.
> * @vddio_supply: I/O voltage regulator for the chip.
> * @orientation: sensor chip orientation relative to main hardware.
> @@ -157,6 +160,8 @@ struct inv_icm42600_state {
> enum inv_icm42600_chip chip;
> const char *name;
> struct regmap *map;
> + bool pedometer_enable;
> + bool pedometer_value;
> struct regulator *vdd_supply;
> struct regulator *vddio_supply;
> struct iio_mount_matrix orientation;
> @@ -301,6 +306,15 @@ struct inv_icm42600_sensor_state {
> #define INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(_f) \
> FIELD_PREP(GENMASK(3, 0), (_f))
>
> +/* Pedometer functionality */
> +#define INV_ICM42600_REG_APEX_CONFIG0 0x0056
> +#define INV_ICM42600_DMP_ODR_50Hz BIT(1)
> +#define INV_ICM42600_PED_ENABLE BIT(5)
> +#define INV_ICM42600_REG_INT_STATUS3 0x0038
> +#define INV_ICM42600_STEP_DET_INT BIT(5)
> +#define INV_ICM42600_REG_APEX_DATA 0x0031 // 2 Byte little-endian
/* */ for comments in IIO (and most of the kernel)
Also, put it on the line above rather than making such a long line.
> +
one blank line is enough.
> +
> #define INV_ICM42600_REG_TMST_CONFIG 0x0054
> #define INV_ICM42600_TMST_CONFIG_MASK GENMASK(4, 0)
> #define INV_ICM42600_TMST_CONFIG_TMST_TO_REGS_EN BIT(4)
> @@ -373,6 +387,8 @@ struct inv_icm42600_sensor_state {
> #define INV_ICM42600_INTF_CONFIG6_I3C_SDR_EN BIT(0)
>
> /* User bank 4 (MSB 0x40) */
> +#define INV_ICM42600_REG_INT_SOURCE6 0x404D
> +#define INV_ICM42600_STEP_DET_INT1_EN BIT(5)
> #define INV_ICM42600_REG_INT_SOURCE8 0x404F
> #define INV_ICM42600_INT_SOURCE8_FSYNC_IBI_EN BIT(5)
> #define INV_ICM42600_INT_SOURCE8_PLL_RDY_IBI_EN BIT(4)
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 56ac19814250..90fe4c9e15ab 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -160,6 +160,13 @@ static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {
> {},
> };
> +static int inv_icm42600_steps_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + __le16 steps;
> + int ret;
> +
> + if (mask == IIO_CHAN_INFO_PROCESSED) {
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> + ret = regmap_bulk_read(st->map, INV_ICM42600_REG_APEX_DATA, &steps, sizeof(steps));
> + if (ret)
> + return ret;
> + iio_device_release_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> + *val = steps;
As the bot pointed out, you need an endian conversion here.
le16_to_cpu()
> + return IIO_VAL_INT;
> + }
> +
> + return -EINVAL;
> +}
> +
> static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> @@ -681,6 +721,8 @@ static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev,
> break;
> case IIO_TEMP:
> return inv_icm42600_temp_read_raw(indio_dev, chan, val, val2, mask);
> + case IIO_STEPS:
> + return inv_icm42600_steps_read_raw(indio_dev, chan, val, val2, mask);
> default:
> return -EINVAL;
> }
> @@ -824,6 +866,126 @@ static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,
> return ret;
> }
>
> +/*****************Pedometer Functionality**************/
No to structure comments like this. They add little to readability and have
a habit of rapidly becoming wrong.
> +static int inv_icm42600_step_en(struct inv_icm42600_state *st, int state)
> +{
> + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_APEX;
> + int ret, value;
> +
> + if (state) {
> +
> + ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0,
> + INV_ICM42600_DMP_ODR_50Hz);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
> + INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET);
> + if (ret)
> + return ret;
> + msleep(1);
Document the reason for this value.
> +
> + ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
> + INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->map, INV_ICM42600_REG_INT_SOURCE6,
> + INV_ICM42600_STEP_DET_INT1_EN);
> + if (ret)
> + return ret;
> +
> + value = INV_ICM42600_DMP_ODR_50Hz | INV_ICM42600_PED_ENABLE;
> + ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0, value);
> + if (ret)
> + return ret;
> +
> + st->pedometer_enable = true;
return here.
Then can drop the else.
With two such different paths, even better would be two little functions
to handle the two operations (enable + disable) as will make each individually
easier to read.
> +
> + } else {
> +
> + ret = regmap_write(st->map, INV_ICM42600_REG_APEX_CONFIG0, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->map, INV_ICM42600_REG_INT_SOURCE6, 0);
> + if (ret)
> + return ret;
> +
> + st->pedometer_enable = false;
> + }
> +
> + return 0;
> +}
> +
> +static int inv_icm42600_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)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + if(chan->type != IIO_STEPS)
> + return -EINVAL;
> +
> + mutex_lock(&st->lock);
guard() is useful in cases like this.
> +
> + ret = inv_icm42600_step_en(st, state);
> +
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +static int inv_icm42600_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int value;
> +
> + if (chan->type != IIO_STEPS)
> + return -EINVAL;
> +
> + regmap_read(st->map, INV_ICM42600_REG_APEX_CONFIG0, &value);
check return value.
> +
> + if (value & INV_ICM42600_PED_ENABLE)
> + return 1;
> + else
> + return 0;
> +}
> +
> +static int inv_icm42600_read_event_value(struct iio_dev *indio_dev,
This isn't to get if the event happened, it's for reading thresholds
etc. Not relevant for a pedometer.
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +
> + mutex_lock(&st->lock);
guard()
> +
> + if (type == IIO_EV_TYPE_CHANGE) {
flip logic and can test that before taking the lock
> + if (st->pedometer_value == true) {
> + *val = 1;
> + st->pedometer_value = false;
> + } else {
> + *val = 0;
> + }
> + mutex_unlock(&st->lock);
> + return IIO_VAL_INT;
> + }
> +
> + mutex_unlock(&st->lock);
> + return -EINVAL;
> +}
> +
> static const struct iio_info inv_icm42600_accel_info = {
> .read_raw = inv_icm42600_accel_read_raw,
> .read_avail = inv_icm42600_accel_read_avail,
> @@ -833,6 +995,9 @@ static const struct iio_info inv_icm42600_accel_info = {
> .update_scan_mode = inv_icm42600_accel_update_scan_mode,
> .hwfifo_set_watermark = inv_icm42600_accel_hwfifo_set_watermark,
> .hwfifo_flush_to_buffer = inv_icm42600_accel_hwfifo_flush,
> + .write_event_config = inv_icm42600_write_event_config,
> + .read_event_config = inv_icm42600_read_event_config,
> + .read_event_value = inv_icm42600_read_event_value,
> };
>
> struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index c3924cc6190e..4d78cb5ca396 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -15,7 +15,8 @@
> #include <linux/pm_runtime.h>
> #include <linux/property.h>
> #include <linux/regmap.h>
> -
Keep the blank line and keep the iio headers in their own section.
> +#include <linux/iio/events.h>
> +#include <linux/of_irq.h>
hmm. Can we not use the generic property accessors?
Also you aren't using any interrupt related new stuff in here so I think
this is just spurious.
> #include <linux/iio/iio.h>
>
> #include "inv_icm42600.h"
> @@ -533,6 +534,19 @@ static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
>
> mutex_lock(&st->lock);
Probably worth considering use of guard() in here as a precursor patch.
>
> + ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS3, &status);
> + if (ret)
> + goto out_unlock;
> +
> + if (status & INV_ICM42600_STEP_DET_INT) {
> + iio_push_event(st->indio_accel, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
> + IIO_NO_MOD,
> + IIO_EV_TYPE_CHANGE,
> + IIO_EV_DIR_NONE),
> + st->timestamp.accel);
> + st->pedometer_value = true;
> + }
> +
> ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS, &status);
> if (ret)
> goto out_unlock;
> @@ -860,12 +876,20 @@ static int inv_icm42600_runtime_suspend(struct device *dev)
> mutex_lock(&st->lock);
>
> /* disable all sensors */
> - ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> - INV_ICM42600_SENSOR_MODE_OFF, false,
> - NULL);
> - if (ret)
> - goto error_unlock;
> + if (st->pedometer_enable) {
> + ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> + INV_ICM42600_SENSOR_MODE_LOW_POWER,
> + false, NULL);
> + if (ret)
> + goto error_unlock;
> + } else {
>
> + ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> + INV_ICM42600_SENSOR_MODE_OFF,
> + false, NULL);
> + if (ret)
> + goto error_unlock;
> + }
> regulator_disable(st->vddio_supply);
>
> error_unlock:
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-10-18 18:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-15 9:20 [PATCH] iio: imu: inv_icm42600: Enable Pedometer Functionality Hardevsinh Palaniya
2024-10-16 9:25 ` kernel test robot
2024-10-17 7:45 ` Jean-Baptiste Maneyrol
2024-10-18 12:09 ` Hardevsinh Palaniya
2024-10-18 18:21 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox