Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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






[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux