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. */ > > >