On Tue, 11 Mar 2025 15:31:44 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx> wrote:
> Hello Jonathan,
>
> still sorry for not being able to reply in-line.
>
> No problem for all changes, except handling the 2 interrupt lines. Currently our driver only supports INT1 and cannot work with INT2, and this is not related to Wake-on-Motion feature. This is something we could add in another series, and I prefer to have a dedicated series for that.
You should check it isn't INT2 that you are getting via spi->irq etc.
Absolutely fine to reject that in the driver but you need to be
sure you have what you think you do and the spi->irq etc don't
provide that surety - they just give you the first one in the
dt-binding.
Jonathan
>
> For the mutex rework, I will delete them. This is something we can also do in another dedicated patch not inside this series.
>
> Is that OK for you?
>
> Thanks,
> JB
>
> ________________________________________
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Saturday, February 22, 2025 17:22
> To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@xxxxxxxxxx>
> Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion
>
> This Message Is From an External Sender
> This message came from outside your organization.
>
> On Thu, 20 Feb 2025 21:52:07 +0100
> Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@xxxxxxxxxx> wrote:
>
> > From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
> >
> > When Wake-on-Motion is on, enable system wakeup and keep chip on for
> > waking up system with interrupt.
> >
> > Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
> Hi Jean-Baptiste,
>
> A few comments inline.
>
> > ---
> > drivers/iio/imu/inv_icm42600/inv_icm42600.h | 2 +
> > drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c | 3 +
> > drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 89 +++++++++++++++--------
> > 3 files changed, 63 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > index 8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465..baf1dcd714800e84ccd21dc1d1e486849c77a9ae 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > @@ -151,6 +151,7 @@ struct inv_icm42600_apex {
> > * @map: regmap pointer.
> > * @vdd_supply: VDD voltage regulator for the chip.
> > * @vddio_supply: I/O voltage regulator for the chip.
> > + * @irq: chip irq.
>
> Maybe say a little on what the variable is used for. It's not otherwise
> obvious why we need it. Also, does this chip really only have one irq line?
> Looks like there are two. So the drivers should be fixed to cope with the
> only one wired being irq2 unless I've found the wrong datasheet which is
> certainly possible.
>
>
> > * @orientation: sensor chip orientation relative to main hardware.
> > * @conf: chip sensors configurations.
> > * @suspended: suspended sensors configuration.
> > @@ -168,6 +169,7 @@ struct inv_icm42600_state {
> > struct regmap *map;
> > struct regulator *vdd_supply;
> > struct regulator *vddio_supply;
> > + int irq;
> > struct iio_mount_matrix orientation;
> > struct inv_icm42600_conf conf;
> > struct inv_icm42600_suspended suspended;
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > index 8ce2276b3edc61cc1ea26810198dd0057054ec48..4240e8c576f4d07af5434e9a91dfda532f87ffb9 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > @@ -1149,6 +1149,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
> > if (ret)
> > return ERR_PTR(ret);
> >
> > + /* accel events are wakeup capable */
> > + device_set_wakeup_capable(&indio_dev->dev, true);
> > +
> > return indio_dev;
> > }
> >
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > index c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2..f94bda5dc094d6cc85e3facbd480b830bfbaa3f9 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > @@ -751,6 +751,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
> > mutex_init(&st->lock);
> > st->chip = chip;
> > st->map = regmap;
> > + st->irq = irq;
> >
> > ret = iio_read_mount_matrix(dev, &st->orientation);
> > if (ret) {
> > @@ -829,44 +830,56 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
> > static int inv_icm42600_suspend(struct device *dev)
> > {
> > struct inv_icm42600_state *st = dev_get_drvdata(dev);
> > + struct device *accel_dev;
> > + bool wakeup;
> > + int accel_conf;
> > int ret;
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
>
> As below. Pull these guard changes out as a precursor patch. That will make
> it easier to spot the real changes here.
>
> >
> > st->suspended.gyro = st->conf.gyro.mode;
> > st->suspended.accel = st->conf.accel.mode;
> > st->suspended.temp = st->conf.temp_en;
> > - if (pm_runtime_suspended(dev)) {
> > - ret = 0;
> > - goto out_unlock;
> > - }
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> >
> > /* disable FIFO data streaming */
> > if (st->fifo.on) {
> > ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> > INV_ICM42600_FIFO_CONFIG_BYPASS);
> > if (ret)
> > - goto out_unlock;
> > + return ret;
> > }
> >
> > - /* disable APEX features */
> > - if (st->apex.wom.enable) {
> > - ret = inv_icm42600_set_wom(st, false);
> > - if (ret)
> > - goto out_unlock;
> > + /* keep chip on and wake-up capable if APEX and wakeup on */
> > + accel_dev = &st->indio_accel->dev;
> > + wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > +
> > + if (!wakeup) {
> > + /* disable APEX features and accel if wakeup disabled */
> > + if (st->apex.wom.enable) {
> > + ret = inv_icm42600_set_wom(st, false);
> > + if (ret)
> > + return ret;
> > + }
> > + accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
> > + } else {
> > + /* keep accel on and setup irq for wakeup */
> > + accel_conf = st->conf.accel.mode;
> > + enable_irq_wake(st->irq);
> > + disable_irq(st->irq);
> > }
> >
> > ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> > - INV_ICM42600_SENSOR_MODE_OFF, false,
> > - NULL);
> > + accel_conf, false, NULL);
> > if (ret)
> > - goto out_unlock;
> > + return ret;
> >
> > - regulator_disable(st->vddio_supply);
> > + /* disable vddio regulator if chip is sleeping */
> > + if (!wakeup)
> > + regulator_disable(st->vddio_supply);
> >
> > -out_unlock:
> > - mutex_unlock(&st->lock);
> > - return ret;
> > + return 0;
> > }
> >
> > /*
> > @@ -878,13 +891,25 @@ static int inv_icm42600_resume(struct device *dev)
> > struct inv_icm42600_state *st = dev_get_drvdata(dev);
> > struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
> > struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
> > + struct device *accel_dev;
> > + bool wakeup;
> > int ret;
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
>
> Good change. But separate patch as not related to most of what is going on here.
>
>
> >
> > - ret = inv_icm42600_enable_regulator_vddio(st);
> > - if (ret)
> > - goto out_unlock;
> > + /* check wakeup capability */
> > + accel_dev = &st->indio_accel->dev;
> > + wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > +
> > + /* restore vddio if cut off or irq state */
> > + if (!wakeup) {
> > + ret = inv_icm42600_enable_regulator_vddio(st);
> > + if (ret)
> > + return ret;
> > + } else {
> > + enable_irq(st->irq);
> > + disable_irq_wake(st->irq);
> > + }
> >
> > pm_runtime_disable(dev);
> > pm_runtime_set_active(dev);
> > @@ -895,13 +920,15 @@ static int inv_icm42600_resume(struct device *dev)
> > st->suspended.accel,
> > st->suspended.temp, NULL);
> > if (ret)
> > - goto out_unlock;
> > + return ret;
> >
> > - /* restore APEX features */
> > - if (st->apex.wom.enable) {
> > - ret = inv_icm42600_set_wom(st, true);
> > - if (ret)
> > - goto out_unlock;
> > + /* restore APEX features if disabled */
> > + if (!wakeup) {
> > + if (st->apex.wom.enable) {
> > + ret = inv_icm42600_set_wom(st, true);
> > + if (ret)
> > + return ret;
> > + }
> > }
> >
> > /* restore FIFO data streaming */
> > @@ -910,11 +937,11 @@ static int inv_icm42600_resume(struct device *dev)
> > inv_sensors_timestamp_reset(&accel_st->ts);
> > ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> > INV_ICM42600_FIFO_CONFIG_STREAM);
> > + if (ret)
> > + return ret;
> > }
> >
> > -out_unlock:
> > - mutex_unlock(&st->lock);
> > - return ret;
> > + return 0;
> > }
> >
> > /* Runtime suspend will turn off sensors that are enabled by iio devices. */
> >
>
