Message-ID: <20231220122708.740c9ff9@jic23-huawei>
Date: Wed, 20 Dec 2023 12:27:08 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Hiten Chauhan <hiten.chauhan@...iconsignals.io>
Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>,
 "lars@...afoo.de" <lars@...afoo.de>, "linux-iio@...r.kernel.org"
 <linux-iio@...r.kernel.org>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] Added tilt interrupt support in inv_icm42600

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

Powered by blists - more mailing lists