Re: [PATCH v3] Added tilt interrupt support in inv_icm42600

[Jonathan Cameron]

On Sat, 25 Nov 2023 07:05:50 +0000
Hiten Chauhan <hiten.chauhan@xxxxxxxxxxxxxxxxx> 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@xxxxxxx>
> Sent: Monday, November 20, 2023 7:48 PM
> To: Hiten Chauhan <hiten.chauhan@xxxxxxxxxxxxxxxxx>; jic23@xxxxxxxxxx <jic23@xxxxxxxxxx>; lars@xxxxxxxxxx <lars@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
> Cc: kernel test robot <lkp@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxx>
> Sent: Friday, November 17, 2023 16:14
> To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx>; jic23@xxxxxxxxxx <jic23@xxxxxxxxxx>; lars@xxxxxxxxxx <lars@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
> Cc: Hiten Chauhan <hiten.chauhan@xxxxxxxxxxxxxxxxx>; kernel test robot <lkp@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxx>
> 
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/202311170235.HaVJnmWa-lkp@xxxxxxxxx/__;!!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
>