| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <FR3P281MB1757E3C062BA1D363B6D7E80CEBDA@FR3P281MB1757.DEUP281.PROD.OUTLOOK.COM>
Date: Mon, 27 Nov 2023 10:38:18 +0000
From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
To: Hiten Chauhan <hiten.chauhan@...iconsignals.io>,
"jic23@...nel.org" <jic23@...nel.org>,
"lars@...afoo.de" <lars@...afoo.de>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600
Hello Hiten,
this is more complex than that.
First, you need to use pm_runtime functions to handle chip on/off state (you can have a look inside inv_icm42600_accel.c for direct reg access how it is done).
You cannot directly write inside PWR_MGMT0 register, otherwise you are overwriting sensor states. For example with your code, if the chip buffer is running with accel and gyro on, when turning the tilt on it will power off gyro and move accel in low-power mode. We really don't want that.
We need to track the existing power states, and only do the required changes. For that, you can use inv_icm42600_set_accel_conf() for turning accel on. But you will have to add support and handle correctly the INV_ICM42600_SENSOR_MODE_LOW_POWER sensor mode and the associated filtering (INV_ICM42600_FILTER_AVG_1X can be sufficient for tilt).
This is the multiplexing I was speaking off. That's more complex than it first seems. If power is not very important for you, you can simplify things by just setting the accel to low-noise mode when turning it on with inv_icm42600_set_accel_conf().
For testing your tilt implementation, you need to turn it on/off while data buffer is off and while data buffer is on, and check that it doesn't impact the data flow (accel and gyro have to stay turned on in low-noise mode).
Thanks.
Best regards,
JB
From: Hiten Chauhan <hiten.chauhan@...iconsignals.io>
Sent: Saturday, November 25, 2023 08:05
To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>; jic23@...nel.org <jic23@...nel.org>; lars@...afoo.de <lars@...afoo.de>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
Cc: kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600
Hello Jean, Thanks for your support, For the first issue can I use "struct iio_event_spec" for tilt interrupt instead of a custom sysfs file? In the second issue I have just disabled tilt related register so when I turn tilt off other
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
ZjQcmQRYFpfptBannerEnd
Hello Jean,
Thanks for your support,
For the first issue can I use "struct iio_event_spec" for tilt interrupt instead of a custom sysfs file?
In the second issue I have just disabled tilt related register so when I turn tilt off other functionality on the accelerometer will work fine. can you please cross-check?
Thanks & Regards,
Hiten Chauhan
From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
Sent: Monday, November 20, 2023 7:48 PM
To: Hiten Chauhan <hiten.chauhan@...iconsignals.io>; jic23@...nel.org <jic23@...nel.org>; lars@...afoo.de <lars@...afoo.de>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
Cc: kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600
Hello Hiten,
thanks for your patch.
I see first a big issue at the root. Tilt event is something that should be reported as an IIO event, not in a custom sysfs file. Jonathan can confirm this, but this is my understanding.
Second issue, there is no multiplexing between the tilt and normal data sampling. Meaning turning tilt off will stop the data output of the chip if it was on. And turning data output off will stop tilt functionnality. All these things have to be multiplexed together and chip power off/on must be centralized.
Thanks for your work.
Best regards,
JB
From: Hiten Chauhan <hiten.chauhan@...iconsignals.io>
Sent: Friday, November 17, 2023 16:14
To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>; jic23@...nel.org <jic23@...nel.org>; lars@...afoo.de <lars@...afoo.de>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
Cc: Hiten Chauhan <hiten.chauhan@...iconsignals.io>; kernel test robot <lkp@...el.com>
Subject: [PATCH v3] Added tilt interrupt support in inv_icm42600
Description: Add new device attribute to enable and disable Tilt interrupt from kernel user space Signed-off-by: Hiten Chauhan <hiten. chauhan@ siliconsignals. io> Reported-by: kernel test robot <lkp@ intel. com> Closes: [https: //urldefense. com/v3/__https: //lore. kernel. org/oe-kbuild-all/202311170235. HaVJnmWa-lkp@ intel. com/__;!!FtrhtPsWDhZ6tw!Abqqh_UwyEydZ0xeIy7YQwPWb_knCM2hsJJWavoAq3igeGccV4RZI87CTV__lZgfBjZytNesx5cUc_RXsP6mu9lmvUGZg_rGWg$[lore[. ]kernel[. ]org]]https: //urldefense. com/v3/__https: //lore. kernel. org/oe-kbuild-all/202311170235. HaVJnmWa-lkp@ intel. com/__;!!FtrhtPsWDhZ6tw!Abqqh_UwyEydZ0xeIy7YQwPWb_knCM2hsJJWavoAq3igeGccV4RZI87CTV__lZgfBjZytNesx5cUc_RXsP6mu9lmvUGZg_rGWg$[lore[. ]kernel[. ]org]
ZjQcmQRYFpfptBannerStart
This Message Is From an Untrusted Sender
You have not previously corresponded with this sender.
ZjQcmQRYFpfptBannerEnd
Description:
Add new device attribute to enable and disable
Tilt interrupt from kernel user space
Signed-off-by: Hiten Chauhan <hiten.chauhan@...iconsignals.io>
Reported-by: kernel test robot <lkp@...el.com>
Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/202311170235.HaVJnmWa-lkp@intel.com/__;!!FtrhtPsWDhZ6tw!Abqqh_UwyEydZ0xeIy7YQwPWb_knCM2hsJJWavoAq3igeGccV4RZI87CTV__lZgfBjZytNesx5cUc_RXsP6mu9lmvUGZg_rGWg$[lore[.]kernel[.]org]
---
drivers/iio/imu/inv_icm42600/inv_icm42600.h | 24 ++++
.../iio/imu/inv_icm42600/inv_icm42600_accel.c | 129 ++++++++++++++++++
2 files changed, 153 insertions(+)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 0e290c807b0f..39ed39e77deb 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -187,6 +187,8 @@ struct inv_icm42600_state {
#define INV_ICM42600_FIFO_CONFIG_STOP_ON_FULL \
FIELD_PREP(INV_ICM42600_FIFO_CONFIG_MASK, 2)
+#define INV_ICM42600_REG_MASK GENMASK(7, 0)
+
/* all sensor data are 16 bits (2 registers wide) in big-endian */
#define INV_ICM42600_REG_TEMP_DATA 0x001D
#define INV_ICM42600_REG_ACCEL_DATA_X 0x001F
@@ -239,6 +241,7 @@ struct inv_icm42600_state {
#define INV_ICM42600_REG_PWR_MGMT0 0x004E
#define INV_ICM42600_PWR_MGMT0_TEMP_DIS BIT(5)
#define INV_ICM42600_PWR_MGMT0_IDLE BIT(4)
+#define INV_ICM42600_PWR_ACCEL_MODE BIT(1)
#define INV_ICM42600_PWR_MGMT0_GYRO(_mode) \
FIELD_PREP(GENMASK(3, 2), (_mode))
#define INV_ICM42600_PWR_MGMT0_ACCEL(_mode) \
@@ -306,6 +309,21 @@ struct inv_icm42600_state {
#define INV_ICM42600_WHOAMI_ICM42622 0x46
#define INV_ICM42600_WHOAMI_ICM42631 0x5C
+/* Register configs for tilt interrupt */
+#define INV_ICM42605_REG_APEX_CONFIG4 0x4043
+#define INV_ICM42605_APEX_CONFIG4_MASK GENMASK(7, 0)
+
+#define INV_ICM42605_REG_APEX_CONFIG0 0x0056
+#define INV_ICM42605_APEX_CONFIG0_TILT_ENABLE BIT(4)
+#define INV_ICM42605_APEX_CONFIG0 BIT(1)
+
+#define INV_ICM42605_REG_INTF_CONFIG1 0x404D
+#define INV_ICM42605_INTF_CONFIG1_MASK GENMASK(5, 0)
+#define INV_ICM42605_INTF_CONFIG1_TILT_DET_INT1_EN BIT(3)
+
+#define INV_ICM42605_REG_INT_STATUS3 0x0038
+
+
/* User bank 1 (MSB 0x10) */
#define INV_ICM42600_REG_SENSOR_CONFIG0 0x1003
#define INV_ICM42600_SENSOR_CONFIG0_ZG_DISABLE BIT(5)
@@ -364,6 +382,8 @@ typedef int (*inv_icm42600_bus_setup)(struct inv_icm42600_state *);
extern const struct regmap_config inv_icm42600_regmap_config;
extern const struct dev_pm_ops inv_icm42600_pm_ops;
+extern uint8_t inv_icm42605_int_reg;
+
const struct iio_mount_matrix *
inv_icm42600_get_mount_matrix(const struct iio_dev *indio_dev,
const struct iio_chan_spec *chan);
@@ -395,4 +415,8 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st);
int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev);
+int inv_icm42605_generate_tilt_interrupt(struct inv_icm42600_state *st);
+
+int inv_icm42605_disable_tilt_interrupt(struct inv_icm42600_state *st);
+
#endif
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index b1e4fde27d25..311f6ea09e64 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -47,6 +47,8 @@
.ext_info = _ext_info, \
}
+uint8_t inv_icm42605_int_reg;
+
enum inv_icm42600_accel_scan {
INV_ICM42600_ACCEL_SCAN_X,
INV_ICM42600_ACCEL_SCAN_Y,
@@ -60,6 +62,68 @@ static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {
{},
};
+static ssize_t tilt_interrupt_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct inv_icm42600_state *st = dev_get_drvdata(dev);
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(st->map, inv_icm42605_int_reg, &val);
+
+ if (ret != 0)
+ return ret;
+
+ snprintf(buf, PAGE_SIZE, "Read reg %x value %x\n", inv_icm42605_int_reg, val);
+
+ return strlen(buf);
+}
+
+static ssize_t tilt_interrupt_store(struct device *dev,
+ struct device_attribute *attr, const char *buf,
+ size_t count)
+{
+ struct inv_icm42600_state *st = dev_get_drvdata(dev);
+ int ret;
+ int value;
+
+ if (!st)
+ return -EINVAL;
+
+ if (kstrtoint(buf, 10, &value))
+ return -EINVAL;
+
+ inv_icm42605_int_reg = INV_ICM42605_REG_INT_STATUS3;
+
+ switch (value) {
+ case 1:
+ ret = inv_icm42605_generate_tilt_interrupt(st);
+ if (ret != 0)
+ return -EIO;
+ break;
+ case 0:
+ ret = inv_icm42605_disable_tilt_interrupt(st);
+ if (ret != 0)
+ return -EIO;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(tilt_interrupt);
+
+static struct attribute *icm42605_attrs[] = {
+ &dev_attr_tilt_interrupt.attr,
+ NULL,
+};
+
+static const struct attribute_group icm42605_attrs_group = {
+ .attrs = icm42605_attrs,
+};
+
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),
@@ -702,6 +766,7 @@ 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,
+ .attrs = &icm42605_attrs_group,
};
struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
@@ -791,3 +856,67 @@ int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev)
return 0;
}
+
+int inv_icm42605_generate_tilt_interrupt(struct inv_icm42600_state *st)
+{
+ int ret;
+ int val;
+ char sleep = 10;
+
+ ret = regmap_update_bits(st->map, INV_ICM42605_REG_APEX_CONFIG4,
+ INV_ICM42605_APEX_CONFIG4_MASK, 0);
+ if (ret)
+ return ret;
+
+ val = INV_ICM42600_PWR_ACCEL_MODE;
+ ret = regmap_write(st->map, INV_ICM42600_REG_PWR_MGMT0, val);
+ if (ret)
+ return ret;
+
+ val = INV_ICM42605_APEX_CONFIG0;
+ ret = regmap_write(st->map, INV_ICM42605_REG_APEX_CONFIG0, val);
+ if (ret)
+ return ret;
+
+ val = INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET;
+ ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET, val);
+ if (ret)
+ return ret;
+
+ msleep(sleep);
+
+ val = INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN;
+ ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET, val);
+ if (ret)
+ return ret;
+
+ val = INV_ICM42605_APEX_CONFIG0_TILT_ENABLE |
+ INV_ICM42605_APEX_CONFIG0;
+ ret = regmap_write(st->map, INV_ICM42605_REG_APEX_CONFIG0, val);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(st->map, INV_ICM42605_REG_INTF_CONFIG1,
+ INV_ICM42605_INTF_CONFIG1_MASK,
+ INV_ICM42605_INTF_CONFIG1_TILT_DET_INT1_EN);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+int inv_icm42605_disable_tilt_interrupt(struct inv_icm42600_state *st)
+{
+ int ret;
+
+ ret = regmap_write(st->map, INV_ICM42605_REG_APEX_CONFIG0, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(st->map, INV_ICM42605_REG_INTF_CONFIG1,
+ INV_ICM42605_INTF_CONFIG1_MASK, 0);
+ if (ret)
+ return ret;
+
+ return 0;
+}
base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
--
2.25.1
Powered by blists - more mailing lists