| Message ID | 20231117151446.49738-1-hiten.chauhan@siliconsignals.io (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] Added tilt interrupt support in inv_icm42600 | expand |
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@siliconsignals.io> Sent: Friday, November 17, 2023 16:14 To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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] 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@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] --- 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
On Sat, 25 Nov 2023 07:05:50 +0000 Hiten Chauhan <hiten.chauhan@siliconsignals.io> wrote: > 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? Yes - that will cover the configuration aspects. It should ultimately be reported via iio_push_event() which reports to userspace via a chardev. See the example code in tools/iio/iio_event_monitor.c Jonathan > > 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@tdk.com> > Sent: Monday, November 20, 2023 7:48 PM > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > Sent: Friday, November 17, 2023 16:14 > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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] > 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@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] > --- > 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 >
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@siliconsignals.io> Sent: Saturday, November 25, 2023 08:05 To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Cc: kernel test robot <lkp@intel.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@tdk.com> Sent: Monday, November 20, 2023 7:48 PM To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Cc: kernel test robot <lkp@intel.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@siliconsignals.io> Sent: Friday, November 17, 2023 16:14 To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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@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] --- 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
Hi Hiten, you can define property that are shared between channels by defining them inside info_mask_shared_by_type or info_mask_shared_by_all filed. As done for scale or for sampling_frequency. Thanks, JB From: Hiten Chauhan <hiten.chauhan@siliconsignals.io> Sent: Friday, December 8, 2023 15:14 To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Cc: kernel test robot <lkp@intel.com> Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 Hi Jean, As per your suggestions (thanks for this), I have looked into creating channels instead of custom sysfs entries. I have looked into an example of "mma9551.c". Now, by comparing the datasheet of mma9551 and inv_icm42600, in mma9551 they provide X, Y, and Z tilt interrupt registers. But in the case of inv_icm42600, there is only one register to receive interrupt for tilt. It appears hardware limitation in our case, so can you please suggest here how we can overcome this? Appreciate your feedback. -- Thanks and Regards, Hiten Chauhan From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> Sent: Monday, November 27, 2023 4:08 PM To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Cc: kernel test robot <lkp@intel.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@siliconsignals.io> Sent: Saturday, November 25, 2023 08:05 To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Cc: kernel test robot <lkp@intel.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@tdk.com> Sent: Monday, November 20, 2023 7:48 PM To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Cc: kernel test robot <lkp@intel.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@siliconsignals.io> Sent: Friday, November 17, 2023 16:14 To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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]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@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] --- 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
On Fri, 8 Dec 2023 15:37:32 +0000 Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > Hi Hiten, > > you can define property that are shared between channels by defining them inside info_mask_shared_by_type or info_mask_shared_by_all filed. As done for scale or for sampling_frequency. > > Thanks, > JB > > > From: Hiten Chauhan <hiten.chauhan@siliconsignals.io> > Sent: Friday, December 8, 2023 15:14 > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Cc: kernel test robot <lkp@intel.com> > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > Hi Jean, > > As per your suggestions (thanks for this), I have looked into creating channels instead of custom sysfs entries. > > I have looked into an example of "mma9551.c". > > Now, by comparing the datasheet of mma9551 and inv_icm42600, in mma9551 they provide X, Y, and Z tilt interrupt registers. > But in the case of inv_icm42600, there is only one register to receive interrupt for tilt. Just to check. Can you tell which axis the tilt event was on, or is it simply 'tilted somehow?' From a look at the datasheet its 'tilted by 35 degrees in some direction' There are a couple of ways we can report this case if that's what you have. There is a modifier for X_OR_Y_OR_Z and we have several users already. With hindsight that's a bad design option and we should have just reported it on each axis but there is precedence for doing it with this modifier so we are stuck with that. Maybe we should consider adding an explicit X_OR_Y_OR_Z Channel that just has events. The risk is that existing userspace software won't expect that so might not know what to do. For event controls, it's fine if one control affects multiple events. So enabling tilt_x may well enable tilt_y and tilt_z as well. User space is meant to cope with getting events it didn't explicitly request. Jonathan > > It appears hardware limitation in our case, so can you please suggest here how we can overcome this? > > Appreciate your feedback. > > -- > Thanks and Regards, > Hiten Chauhan > > > > From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > Sent: Monday, November 27, 2023 4:08 PM > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > Sent: Saturday, November 25, 2023 08:05 > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Cc: kernel test robot <lkp@intel.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@tdk.com> > Sent: Monday, November 20, 2023 7:48 PM > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > Sent: Friday, November 17, 2023 16:14 > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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]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@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] > --- > 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
On Mon, 18 Dec 2023 06:36:28 +0000 Hiten Chauhan <hiten.chauhan@siliconsignals.io> wrote: > Hi Jonathan, > > 1. As per the datasheet for tilt interrupt, there is no direction, angle, or axis. Tilt only gets reported beyond 35* angle. This can still be exposed by providing the threshold parameter but making it read only. This seems to be axis free tilt which is unusual and a bit tricky to define as we normally define rotations around a particular axis. So whatever we do is going to be something that userspace won't really know how to deal with. One option is to do what we allow for single axis rotation sensors and have in_rot channel (no modifier) then defines events for that. Given how poorly defined this is on the datasheet I think that's the best we can do. Tilt has to be relative to something, but there is no information as to what. Is it relative to orientation when the feature is enabled? Is it relative to some reference plane? I can't find any indication of which. Jonathan > 2. For the userspace application what I can suggest is, that when tilt gets detected we can share accelerometer x,y,z as per theory(Please correct me here if I am wrong) > 3. In such a case, what is the recommended design? > > Thanks &Regards, > Hiten Chauhan > ________________________________ > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, December 10, 2023 4:50 PM > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > On Fri, 8 Dec 2023 15:37:32 +0000 > Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > > > Hi Hiten, > > > > you can define property that are shared between channels by defining them inside info_mask_shared_by_type or info_mask_shared_by_all filed. As done for scale or for sampling_frequency. > > > > Thanks, > > JB > > > > > > From: Hiten Chauhan <hiten.chauhan@siliconsignals.io> > > Sent: Friday, December 8, 2023 15:14 > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > Cc: kernel test robot <lkp@intel.com> > > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > > > Hi Jean, > > > > As per your suggestions (thanks for this), I have looked into creating channels instead of custom sysfs entries. > > > > I have looked into an example of "mma9551.c". > > > > Now, by comparing the datasheet of mma9551 and inv_icm42600, in mma9551 they provide X, Y, and Z tilt interrupt registers. > > But in the case of inv_icm42600, there is only one register to receive interrupt for tilt. > > Just to check. Can you tell which axis the tilt event was on, or is it simply 'tilted somehow?' From a look > at the datasheet its 'tilted by 35 degrees in some direction' > There are a couple of ways we can report this case if that's what you have. There is a modifier for > X_OR_Y_OR_Z and we have several users already. With hindsight that's a bad design option and we should have > just reported it on each axis but there is precedence for doing it with this modifier so we are stuck with that. > Maybe we should consider adding an explicit X_OR_Y_OR_Z Channel that just has events. The risk is that > existing userspace software won't expect that so might not know what to do. > > For event controls, it's fine if one control affects multiple events. So enabling tilt_x may well enable tilt_y > and tilt_z as well. User space is meant to cope with getting events it didn't explicitly request. > > Jonathan > > > > > It appears hardware limitation in our case, so can you please suggest here how we can overcome this? > > > > Appreciate your feedback. > > > > -- > > Thanks and Regards, > > Hiten Chauhan > > > > > > > > From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > > Sent: Monday, November 27, 2023 4:08 PM > > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > > Sent: Saturday, November 25, 2023 08:05 > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > Cc: kernel test robot <lkp@intel.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@tdk.com> > > Sent: Monday, November 20, 2023 7:48 PM > > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > > Sent: Friday, November 17, 2023 16:14 > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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]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@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] > > --- > > 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 >
Hi Jonhathan, Hiten,
I can be of some help to explain the feature.
This Tilt feature is the implementation of the TILT_DETECTOR sensor from Android system. The angle computed is between gravity vectors. The 1st one is computed as a reference when turning the feature on, and the 2nd is continuously evaluated on a time frame. When the angle between these 2 gravity vectors is large enough, a tilt event is generated.
Here are the TILT_DETECTOR specs from Android system:
/**
* WAKE_UP_TILT_DETECTOR
* reporting-mode: special (setDelay has no impact)
*
* A sensor of this type generates an event each time a tilt event is
* detected. A tilt event must be generated if the direction of the
* 2-seconds window average gravity changed by at least 35 degrees since the
* activation or the last trigger of the sensor.
*
* reference_estimated_gravity = average of accelerometer measurements over
* the first 1 second after activation or the estimated gravity at the last
* trigger.
*
* current_estimated_gravity = average of accelerometer measurements over
* the last 2 seconds.
*
* trigger when
* angle(reference_estimated_gravity, current_estimated_gravity)
* > 35 degrees
*
* Large accelerations without a change in phone orientation must not
* trigger a tilt event.
* For example, a sharp turn or strong acceleration while driving a car
* must not trigger a tilt event, even though the angle of the average
* acceleration might vary by more than 35 degrees.
*
* Typically, this sensor is implemented with the help of only an
* accelerometer. Other sensors can be used as well if they do not increase
* the power consumption significantly. This is a low power sensor that
* must allow the AP to go into suspend mode. Do not emulate this sensor
* in the HAL.
* Like other wake up sensors, the driver is expected to a hold a wake_lock
* with a timeout of 200 ms while reporting this event. The only allowed
* return value is 1.0.
*
* Implement only the wake-up version of this sensor.
*/
Hope it helps to better understand.
Thanks,
JB
From: Jonathan Cameron <jic23@kernel.org>
Sent: Wednesday, December 20, 2023 13:27
To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>
Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600
On Mon, 18 Dec 2023 06: 36: 28 +0000 Hiten Chauhan <hiten. chauhan@ siliconsignals. io> wrote: > Hi Jonathan, > > 1. As per the datasheet for tilt interrupt, there is no direction, angle, or axis. Tilt only gets reported beyond 35*
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
ZjQcmQRYFpfptBannerEnd
On Mon, 18 Dec 2023 06:36:28 +0000
Hiten Chauhan <hiten.chauhan@siliconsignals.io> wrote:
> Hi Jonathan,
>
> 1. As per the datasheet for tilt interrupt, there is no direction, angle, or axis. Tilt only gets reported beyond 35* angle.
This can still be exposed by providing the threshold parameter but making it
read only. This seems to be axis free tilt which is unusual and a bit tricky
to define as we normally define rotations around a particular axis.
So whatever we do is going to be something that userspace won't really know
how to deal with. One option is to do what we allow for single axis
rotation sensors and have in_rot channel (no modifier) then defines events
for that.
Given how poorly defined this is on the datasheet I think that's the best we can do.
Tilt has to be relative to something, but there is no information as to what. Is it
relative to orientation when the feature is enabled? Is it relative to some
reference plane? I can't find any indication of which.
Jonathan
> 2. For the userspace application what I can suggest is, that when tilt gets detected we can share accelerometer x,y,z as per theory(Please correct me here if I am wrong)
> 3. In such a case, what is the recommended design?
>
> Thanks &Regards,
> Hiten Chauhan
> ________________________________
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, December 10, 2023 4:50 PM
> To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
> Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600
>
> On Fri, 8 Dec 2023 15:37:32 +0000
> Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:
>
> > Hi Hiten,
> >
> > you can define property that are shared between channels by defining them inside info_mask_shared_by_type or info_mask_shared_by_all filed. As done for scale or for sampling_frequency.
> >
> > Thanks,
> > JB
> >
> >
> > From: Hiten Chauhan <hiten.chauhan@siliconsignals.io>
> > Sent: Friday, December 8, 2023 15:14
> > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> > Cc: kernel test robot <lkp@intel.com>
> > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600
> >
> > Hi Jean,
> >
> > As per your suggestions (thanks for this), I have looked into creating channels instead of custom sysfs entries.
> >
> > I have looked into an example of "mma9551.c".
> >
> > Now, by comparing the datasheet of mma9551 and inv_icm42600, in mma9551 they provide X, Y, and Z tilt interrupt registers.
> > But in the case of inv_icm42600, there is only one register to receive interrupt for tilt.
>
> Just to check. Can you tell which axis the tilt event was on, or is it simply 'tilted somehow?' From a look
> at the datasheet its 'tilted by 35 degrees in some direction'
> There are a couple of ways we can report this case if that's what you have. There is a modifier for
> X_OR_Y_OR_Z and we have several users already. With hindsight that's a bad design option and we should have
> just reported it on each axis but there is precedence for doing it with this modifier so we are stuck with that.
> Maybe we should consider adding an explicit X_OR_Y_OR_Z Channel that just has events. The risk is that
> existing userspace software won't expect that so might not know what to do.
>
> For event controls, it's fine if one control affects multiple events. So enabling tilt_x may well enable tilt_y
> and tilt_z as well. User space is meant to cope with getting events it didn't explicitly request.
>
> Jonathan
>
> >
> > It appears hardware limitation in our case, so can you please suggest here how we can overcome this?
> >
> > Appreciate your feedback.
> >
> > --
> > Thanks and Regards,
> > Hiten Chauhan
> >
> >
> >
> > From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
> > Sent: Monday, November 27, 2023 4:08 PM
> > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> > Cc: kernel test robot <lkp@intel.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@siliconsignals.io>
> > Sent: Saturday, November 25, 2023 08:05
> > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> > Cc: kernel test robot <lkp@intel.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@tdk.com>
> > Sent: Monday, November 20, 2023 7:48 PM
> > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> > Cc: kernel test robot <lkp@intel.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@siliconsignals.io>
> > Sent: Friday, November 17, 2023 16:14
> > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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]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@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]
>> ---
> > 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
>
On Wed, 20 Dec 2023 13:29:49 +0000 Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > Hi Jonhathan, Hiten, > > I can be of some help to explain the feature. > > This Tilt feature is the implementation of the TILT_DETECTOR sensor from Android system. The angle computed is between gravity vectors. The 1st one is computed as a reference when turning the feature on, and the 2nd is continuously evaluated on a time frame. When the angle between these 2 gravity vectors is large enough, a tilt event is generated. > > Here are the TILT_DETECTOR specs from Android system: > /** > * WAKE_UP_TILT_DETECTOR > * reporting-mode: special (setDelay has no impact) > * > * A sensor of this type generates an event each time a tilt event is > * detected. A tilt event must be generated if the direction of the > * 2-seconds window average gravity changed by at least 35 degrees since the > * activation or the last trigger of the sensor. > * > * reference_estimated_gravity = average of accelerometer measurements over > * the first 1 second after activation or the estimated gravity at the last > * trigger. > * > * current_estimated_gravity = average of accelerometer measurements over > * the last 2 seconds. > * > * trigger when > * angle(reference_estimated_gravity, current_estimated_gravity) > * > 35 degrees > * > * Large accelerations without a change in phone orientation must not > * trigger a tilt event. > * For example, a sharp turn or strong acceleration while driving a car > * must not trigger a tilt event, even though the angle of the average > * acceleration might vary by more than 35 degrees. > * > * Typically, this sensor is implemented with the help of only an > * accelerometer. Other sensors can be used as well if they do not increase > * the power consumption significantly. This is a low power sensor that > * must allow the AP to go into suspend mode. Do not emulate this sensor > * in the HAL. > * Like other wake up sensors, the driver is expected to a hold a wake_lock > * with a timeout of 200 ms while reporting this event. The only allowed > * return value is 1.0. > * > * Implement only the wake-up version of this sensor. > */ > > > Hope it helps to better understand. Thanks - that is indeed very useful. A reference to this in the driver would be a good idea. I'm open to suggestions on how to describe this. Maybe we just need to use the gesture route? (I'm not keen on that unless it's the last resort as it provides no real grouping of channel types or expectations on scaling etc). Or we could use a new rotation channel modifier for 'tilt' and just document that as relative to a 'reference' value. Effectively treat it like we treat quaternions or Euler angles for rotation. If the channel was exposed, we'd then support explicitly resetting it but here it is event only so I suppose just using the event enable is enough. in_rot_tilt_* The averaging makes this even messier. I guess that's there to effectively remove the non gravity component and we should probably just not bother describing it. If it is useful to do so we could add it as a form of event signal filter. Jonathan > > Thanks, > JB > > > From: Jonathan Cameron <jic23@kernel.org> > Sent: Wednesday, December 20, 2023 13:27 > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io> > Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > On Mon, 18 Dec 2023 06: 36: 28 +0000 Hiten Chauhan <hiten. chauhan@ siliconsignals. io> wrote: > Hi Jonathan, > > 1. As per the datasheet for tilt interrupt, there is no direction, angle, or axis. Tilt only gets reported beyond 35* > ZjQcmQRYFpfptBannerStart > This Message Is From an External Sender > This message came from outside your organization. > > ZjQcmQRYFpfptBannerEnd > On Mon, 18 Dec 2023 06:36:28 +0000 > Hiten Chauhan <hiten.chauhan@siliconsignals.io> wrote: > > > Hi Jonathan, > > > > 1. As per the datasheet for tilt interrupt, there is no direction, angle, or axis. Tilt only gets reported beyond 35* angle. > > This can still be exposed by providing the threshold parameter but making it > read only. This seems to be axis free tilt which is unusual and a bit tricky > to define as we normally define rotations around a particular axis. > So whatever we do is going to be something that userspace won't really know > how to deal with. One option is to do what we allow for single axis > rotation sensors and have in_rot channel (no modifier) then defines events > for that. > > Given how poorly defined this is on the datasheet I think that's the best we can do. > > Tilt has to be relative to something, but there is no information as to what. Is it > relative to orientation when the feature is enabled? Is it relative to some > reference plane? I can't find any indication of which. > > Jonathan > > > > > > 2. For the userspace application what I can suggest is, that when tilt gets detected we can share accelerometer x,y,z as per theory(Please correct me here if I am wrong) > > > > 3. In such a case, what is the recommended design? > > > > Thanks &Regards, > > Hiten Chauhan > > ________________________________ > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Sunday, December 10, 2023 4:50 PM > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > > > On Fri, 8 Dec 2023 15:37:32 +0000 > > Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > > > > > Hi Hiten, > > > > > > you can define property that are shared between channels by defining them inside info_mask_shared_by_type or info_mask_shared_by_all filed. As done for scale or for sampling_frequency. > > > > > > Thanks, > > > JB > > > > > > > > > From: Hiten Chauhan <hiten.chauhan@siliconsignals.io> > > > Sent: Friday, December 8, 2023 15:14 > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: kernel test robot <lkp@intel.com> > > > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > > > > > Hi Jean, > > > > > > As per your suggestions (thanks for this), I have looked into creating channels instead of custom sysfs entries. > > > > > > I have looked into an example of "mma9551.c". > > > > > > Now, by comparing the datasheet of mma9551 and inv_icm42600, in mma9551 they provide X, Y, and Z tilt interrupt registers. > > > But in the case of inv_icm42600, there is only one register to receive interrupt for tilt. > > > > Just to check. Can you tell which axis the tilt event was on, or is it simply 'tilted somehow?' From a look > > at the datasheet its 'tilted by 35 degrees in some direction' > > There are a couple of ways we can report this case if that's what you have. There is a modifier for > > X_OR_Y_OR_Z and we have several users already. With hindsight that's a bad design option and we should have > > just reported it on each axis but there is precedence for doing it with this modifier so we are stuck with that. > > Maybe we should consider adding an explicit X_OR_Y_OR_Z Channel that just has events. The risk is that > > existing userspace software won't expect that so might not know what to do. > > > > For event controls, it's fine if one control affects multiple events. So enabling tilt_x may well enable tilt_y > > and tilt_z as well. User space is meant to cope with getting events it didn't explicitly request. > > > > Jonathan > > > > > > > > It appears hardware limitation in our case, so can you please suggest here how we can overcome this? > > > > > > Appreciate your feedback. > > > > > > -- > > > Thanks and Regards, > > > Hiten Chauhan > > > > > > > > > > > > From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > > > Sent: Monday, November 27, 2023 4:08 PM > > > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > > > Sent: Saturday, November 25, 2023 08:05 > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: kernel test robot <lkp@intel.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@tdk.com> > > > Sent: Monday, November 20, 2023 7:48 PM > > > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > > > Sent: Friday, November 17, 2023 16:14 > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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]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@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] > >> --- > > > 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 > > >
Hello, this feature is using only accelerometer values, it is not based on a quaternion (it is a low power feature, aimed at waking the device only when detecting a tilt). Thus in_accel_tilt would make more sense. But perhaps we could add a new channel of type inclination that is reporting only a threshold event. That would make more sense, I think. Thanks, JB From: Jonathan Cameron <jic23@kernel.org> Sent: Thursday, December 21, 2023 11:44 To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 On Wed, 20 Dec 2023 13: 29: 49 +0000 Jean-Baptiste Maneyrol <Jean-Baptiste. Maneyrol@ tdk. com> wrote: > Hi Jonhathan, Hiten, > > I can be of some help to explain the feature. > > This Tilt feature is the implementation of the ZjQcmQRYFpfptBannerStart This Message Is From an External Sender This message came from outside your organization. ZjQcmQRYFpfptBannerEnd On Wed, 20 Dec 2023 13:29:49 +0000 Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > Hi Jonhathan, Hiten, > > I can be of some help to explain the feature. > > This Tilt feature is the implementation of the TILT_DETECTOR sensor from Android system. The angle computed is between gravity vectors. The 1st one is computed as a reference when turning the feature on, and the 2nd is continuously evaluated on a time frame. When the angle between these 2 gravity vectors is large enough, a tilt event is generated. > > Here are the TILT_DETECTOR specs from Android system: > /** > * WAKE_UP_TILT_DETECTOR > * reporting-mode: special (setDelay has no impact) > * > * A sensor of this type generates an event each time a tilt event is > * detected. A tilt event must be generated if the direction of the > * 2-seconds window average gravity changed by at least 35 degrees since the > * activation or the last trigger of the sensor. > * > * reference_estimated_gravity = average of accelerometer measurements over > * the first 1 second after activation or the estimated gravity at the last > * trigger. > * > * current_estimated_gravity = average of accelerometer measurements over > * the last 2 seconds. > * > * trigger when > * angle(reference_estimated_gravity, current_estimated_gravity) > * > 35 degrees > * > * Large accelerations without a change in phone orientation must not > * trigger a tilt event. > * For example, a sharp turn or strong acceleration while driving a car > * must not trigger a tilt event, even though the angle of the average > * acceleration might vary by more than 35 degrees. > * > * Typically, this sensor is implemented with the help of only an > * accelerometer. Other sensors can be used as well if they do not increase > * the power consumption significantly. This is a low power sensor that > * must allow the AP to go into suspend mode. Do not emulate this sensor > * in the HAL. > * Like other wake up sensors, the driver is expected to a hold a wake_lock > * with a timeout of 200 ms while reporting this event. The only allowed > * return value is 1.0. > * > * Implement only the wake-up version of this sensor. > */ > > > Hope it helps to better understand. Thanks - that is indeed very useful. A reference to this in the driver would be a good idea. I'm open to suggestions on how to describe this. Maybe we just need to use the gesture route? (I'm not keen on that unless it's the last resort as it provides no real grouping of channel types or expectations on scaling etc). Or we could use a new rotation channel modifier for 'tilt' and just document that as relative to a 'reference' value. Effectively treat it like we treat quaternions or Euler angles for rotation. If the channel was exposed, we'd then support explicitly resetting it but here it is event only so I suppose just using the event enable is enough. in_rot_tilt_* The averaging makes this even messier. I guess that's there to effectively remove the non gravity component and we should probably just not bother describing it. If it is useful to do so we could add it as a form of event signal filter. Jonathan > > Thanks, > JB > > > From: Jonathan Cameron <jic23@kernel.org> > Sent: Wednesday, December 20, 2023 13:27 > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io> > Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > On Mon, 18 Dec 2023 06: 36: 28 +0000 Hiten Chauhan <hiten. chauhan@ siliconsignals. io> wrote: > Hi Jonathan, > > 1. As per the datasheet for tilt interrupt, there is no direction, angle, or axis. Tilt only gets reported beyond 35* > ZjQcmQRYFpfptBannerStart > This Message Is From an External Sender > This message came from outside your organization. > > ZjQcmQRYFpfptBannerEnd > On Mon, 18 Dec 2023 06:36:28 +0000 > Hiten Chauhan <hiten.chauhan@siliconsignals.io> wrote: > > > Hi Jonathan, > > > > 1. As per the datasheet for tilt interrupt, there is no direction, angle, or axis. Tilt only gets reported beyond 35* angle. > > This can still be exposed by providing the threshold parameter but making it > read only. This seems to be axis free tilt which is unusual and a bit tricky > to define as we normally define rotations around a particular axis. > So whatever we do is going to be something that userspace won't really know > how to deal with. One option is to do what we allow for single axis > rotation sensors and have in_rot channel (no modifier) then defines events > for that. > > Given how poorly defined this is on the datasheet I think that's the best we can do. > > Tilt has to be relative to something, but there is no information as to what. Is it > relative to orientation when the feature is enabled? Is it relative to some > reference plane? I can't find any indication of which. > > Jonathan > > > > > > 2. For the userspace application what I can suggest is, that when tilt gets detected we can share accelerometer x,y,z as per theory(Please correct me here if I am wrong) > > > > 3. In such a case, what is the recommended design? > > > > Thanks &Regards, > > Hiten Chauhan > > ________________________________ > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Sunday, December 10, 2023 4:50 PM > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > > > On Fri, 8 Dec 2023 15:37:32 +0000 > > Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > > > > > Hi Hiten, > > > > > > you can define property that are shared between channels by defining them inside info_mask_shared_by_type or info_mask_shared_by_all filed. As done for scale or for sampling_frequency. > > > > > > Thanks, > > > JB > > > > > > > > > From: Hiten Chauhan <hiten.chauhan@siliconsignals.io> > > > Sent: Friday, December 8, 2023 15:14 > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: kernel test robot <lkp@intel.com> > > > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > > > > > Hi Jean, > > > > > > As per your suggestions (thanks for this), I have looked into creating channels instead of custom sysfs entries. > > > > > > I have looked into an example of "mma9551.c". > > > > > > Now, by comparing the datasheet of mma9551 and inv_icm42600, in mma9551 they provide X, Y, and Z tilt interrupt registers. > > > But in the case of inv_icm42600, there is only one register to receive interrupt for tilt. > > > > Just to check. Can you tell which axis the tilt event was on, or is it simply 'tilted somehow?' From a look > > at the datasheet its 'tilted by 35 degrees in some direction' > > There are a couple of ways we can report this case if that's what you have. There is a modifier for > > X_OR_Y_OR_Z and we have several users already. With hindsight that's a bad design option and we should have > > just reported it on each axis but there is precedence for doing it with this modifier so we are stuck with that. > > Maybe we should consider adding an explicit X_OR_Y_OR_Z Channel that just has events. The risk is that > > existing userspace software won't expect that so might not know what to do. > > > > For event controls, it's fine if one control affects multiple events. So enabling tilt_x may well enable tilt_y > > and tilt_z as well. User space is meant to cope with getting events it didn't explicitly request. > > > > Jonathan > > > > > > > > It appears hardware limitation in our case, so can you please suggest here how we can overcome this? > > > > > > Appreciate your feedback. > > > > > > -- > > > Thanks and Regards, > > > Hiten Chauhan > > > > > > > > > > > > From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > > > Sent: Monday, November 27, 2023 4:08 PM > > > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > > > Sent: Saturday, November 25, 2023 08:05 > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: kernel test robot <lkp@intel.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@tdk.com> > > > Sent: Monday, November 20, 2023 7:48 PM > > > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > > > Sent: Friday, November 17, 2023 16:14 > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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]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@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] >>> --- > > > 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 > > >
On Fri, 22 Dec 2023 11:06:26 +0000 Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > Hello, > > this feature is using only accelerometer values, it is not based on a quaternion (it is a low power feature, aimed at waking the device only when detecting a tilt). Understood. My aim was to explain that it was a computed channel type (like a quaternion is) rather than draw a specific connection between this case and quaternions. > Thus in_accel_tilt would make more sense. > > But perhaps we could add a new channel of type inclination that is reporting only a threshold event. That would make more sense, I think. Just because this device only has events, doesn't mean a sensible future device wouldn't report the current value of the tilt so it has to work as a normal channel to which a threshold is applied. It's not measuring an acceleration (though derived from one) so we can't make it a modified acceleration channel (as that would have units of m/s^2 which makes no sense). So if we are using a modifier it needs to be a modified rotation channel. Hence suggestion of in_rot_tilt Units are in radians and a fixed threshold of 35/10 * M_PI should be exposed to userspace, but the modifier is a bit like the ones we have for true north vs magnetic north (and tilt compensated versions of those). The similarity being that in both those cases and this one we are dealing with a different 'basis' from which to compute the channel (there it's a corrective rotation from magnetic north for the true north variants, here it's rotation change from an initial rotation). Hopefully that makes my thinking a little clearer. Deriving new ABI is about trying to build something consistent across many channel types. Not always easy to do. Jonathan > > Thanks, > JB > > > From: Jonathan Cameron <jic23@kernel.org> > Sent: Thursday, December 21, 2023 11:44 > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > On Wed, 20 Dec 2023 13: 29: 49 +0000 Jean-Baptiste Maneyrol <Jean-Baptiste. Maneyrol@ tdk. com> wrote: > Hi Jonhathan, Hiten, > > I can be of some help to explain the feature. > > This Tilt feature is the implementation of the > ZjQcmQRYFpfptBannerStart > This Message Is From an External Sender > This message came from outside your organization. > > ZjQcmQRYFpfptBannerEnd > On Wed, 20 Dec 2023 13:29:49 +0000 > Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > > > Hi Jonhathan, Hiten, > > > > I can be of some help to explain the feature. > > > > This Tilt feature is the implementation of the TILT_DETECTOR sensor from Android system. The angle computed is between gravity vectors. The 1st one is computed as a reference when turning the feature on, and the 2nd is continuously evaluated on a time frame. When the angle between these 2 gravity vectors is large enough, a tilt event is generated. > > > > Here are the TILT_DETECTOR specs from Android system: > > /** > > * WAKE_UP_TILT_DETECTOR > > * reporting-mode: special (setDelay has no impact) > > * > > * A sensor of this type generates an event each time a tilt event is > > * detected. A tilt event must be generated if the direction of the > > * 2-seconds window average gravity changed by at least 35 degrees since the > > * activation or the last trigger of the sensor. > > * > > * reference_estimated_gravity = average of accelerometer measurements over > > * the first 1 second after activation or the estimated gravity at the last > > * trigger. > > * > > * current_estimated_gravity = average of accelerometer measurements over > > * the last 2 seconds. > > * > > * trigger when > > * angle(reference_estimated_gravity, current_estimated_gravity) > > * > 35 degrees > > * > > * Large accelerations without a change in phone orientation must not > > * trigger a tilt event. > > * For example, a sharp turn or strong acceleration while driving a car > > * must not trigger a tilt event, even though the angle of the average > > * acceleration might vary by more than 35 degrees. > > * > > * Typically, this sensor is implemented with the help of only an > > * accelerometer. Other sensors can be used as well if they do not increase > > * the power consumption significantly. This is a low power sensor that > > * must allow the AP to go into suspend mode. Do not emulate this sensor > > * in the HAL. > > * Like other wake up sensors, the driver is expected to a hold a wake_lock > > * with a timeout of 200 ms while reporting this event. The only allowed > > * return value is 1.0. > > * > > * Implement only the wake-up version of this sensor. > > */ > > > > > > Hope it helps to better understand. > > Thanks - that is indeed very useful. A reference to this in the driver > would be a good idea. > > I'm open to suggestions on how to describe this. Maybe we just need to > use the gesture route? (I'm not keen on that unless it's the last resort > as it provides no real grouping of channel types or expectations on scaling > etc). Or we could use a new rotation channel modifier for 'tilt' and > just document that as relative to a 'reference' value. Effectively treat > it like we treat quaternions or Euler angles for rotation. If the channel > was exposed, we'd then support explicitly resetting it but here it is event > only so I suppose just using the event enable is enough. > > in_rot_tilt_* > > The averaging makes this even messier. I guess that's there to effectively > remove the non gravity component and we should probably just not bother > describing it. If it is useful to do so we could add it as a form of event > signal filter. > > Jonathan > > > > > Thanks, > > JB > > > > > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Wednesday, December 20, 2023 13:27 > > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io> > > Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > > > On Mon, 18 Dec 2023 06: 36: 28 +0000 Hiten Chauhan <hiten. chauhan@ siliconsignals. io> wrote: > Hi Jonathan, > > 1. As per the datasheet for tilt interrupt, there is no direction, angle, or axis. Tilt only gets reported beyond 35* > > ZjQcmQRYFpfptBannerStart > > This Message Is From an External Sender > > This message came from outside your organization. > > > > ZjQcmQRYFpfptBannerEnd > > On Mon, 18 Dec 2023 06:36:28 +0000 > > Hiten Chauhan <hiten.chauhan@siliconsignals.io> wrote: > > > > > Hi Jonathan, > > > > > > 1. As per the datasheet for tilt interrupt, there is no direction, angle, or axis. Tilt only gets reported beyond 35* angle. > > > > This can still be exposed by providing the threshold parameter but making it > > read only. This seems to be axis free tilt which is unusual and a bit tricky > > to define as we normally define rotations around a particular axis. > > So whatever we do is going to be something that userspace won't really know > > how to deal with. One option is to do what we allow for single axis > > rotation sensors and have in_rot channel (no modifier) then defines events > > for that. > > > > Given how poorly defined this is on the datasheet I think that's the best we can do. > > > > Tilt has to be relative to something, but there is no information as to what. Is it > > relative to orientation when the feature is enabled? Is it relative to some > > reference plane? I can't find any indication of which. > > > > Jonathan > > > > > > > > > > > 2. For the userspace application what I can suggest is, that when tilt gets detected we can share accelerometer x,y,z as per theory(Please correct me here if I am wrong) > > > > > > > 3. In such a case, what is the recommended design? > > > > > > Thanks &Regards, > > > Hiten Chauhan > > > ________________________________ > > > From: Jonathan Cameron <jic23@kernel.org> > > > Sent: Sunday, December 10, 2023 4:50 PM > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > > > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > > > > > On Fri, 8 Dec 2023 15:37:32 +0000 > > > Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote: > > > > > > > Hi Hiten, > > > > > > > > you can define property that are shared between channels by defining them inside info_mask_shared_by_type or info_mask_shared_by_all filed. As done for scale or for sampling_frequency. > > > > > > > > Thanks, > > > > JB > > > > > > > > > > > > From: Hiten Chauhan <hiten.chauhan@siliconsignals.io> > > > > Sent: Friday, December 8, 2023 15:14 > > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > > Cc: kernel test robot <lkp@intel.com> > > > > Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600 > > > > > > > > Hi Jean, > > > > > > > > As per your suggestions (thanks for this), I have looked into creating channels instead of custom sysfs entries. > > > > > > > > I have looked into an example of "mma9551.c". > > > > > > > > Now, by comparing the datasheet of mma9551 and inv_icm42600, in mma9551 they provide X, Y, and Z tilt interrupt registers. > > > > But in the case of inv_icm42600, there is only one register to receive interrupt for tilt. > > > > > > Just to check. Can you tell which axis the tilt event was on, or is it simply 'tilted somehow?' From a look > > > at the datasheet its 'tilted by 35 degrees in some direction' > > > There are a couple of ways we can report this case if that's what you have. There is a modifier for > > > X_OR_Y_OR_Z and we have several users already. With hindsight that's a bad design option and we should have > > > just reported it on each axis but there is precedence for doing it with this modifier so we are stuck with that. > > > Maybe we should consider adding an explicit X_OR_Y_OR_Z Channel that just has events. The risk is that > > > existing userspace software won't expect that so might not know what to do. > > > > > > For event controls, it's fine if one control affects multiple events. So enabling tilt_x may well enable tilt_y > > > and tilt_z as well. User space is meant to cope with getting events it didn't explicitly request. > > > > > > Jonathan > > > > > > > > > > > It appears hardware limitation in our case, so can you please suggest here how we can overcome this? > > > > > > > > Appreciate your feedback. > > > > > > > > -- > > > > Thanks and Regards, > > > > Hiten Chauhan > > > > > > > > > > > > > > > > From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> > > > > Sent: Monday, November 27, 2023 4:08 PM > > > > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > > > > Sent: Saturday, November 25, 2023 08:05 > > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > > Cc: kernel test robot <lkp@intel.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@tdk.com> > > > > Sent: Monday, November 20, 2023 7:48 PM > > > > To: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > > Cc: kernel test robot <lkp@intel.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@siliconsignals.io> > > > > Sent: Friday, November 17, 2023 16:14 > > > > To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > > > > Cc: Hiten Chauhan <hiten.chauhan@siliconsignals.io>; kernel test robot <lkp@intel.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]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@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] > >>> --- > > > > 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 > > > > > >
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; +}
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://lore.kernel.org/oe-kbuild-all/202311170235.HaVJnmWa-lkp@intel.com/ --- drivers/iio/imu/inv_icm42600/inv_icm42600.h | 24 ++++ .../iio/imu/inv_icm42600/inv_icm42600_accel.c | 129 ++++++++++++++++++ 2 files changed, 153 insertions(+) base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86