Re: [PATCH V4 08/10] iio: imu: inv_icm42607: Add Wake on Movement for icm42607

[Jonathan Cameron <jic23@kernel.org>]

On Fri,  1 May 2026 17:11:47 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add support for wake on movement for the icm42607 driver.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
One general thing as a result of change in how runtime pm
calls the stuff to track when it was last busy that I probably
missed in earlier patches.

Otherwise minor stuff inline.

Jonathan

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> index d998d8c94eb9..d056c74dcbfe 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c

> +static int inv_icm42607_accel_disable_wom(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *pdev = regmap_get_device(st->map);
> +	int ret;
> +
> +	ret = _inv_icm42607_accel_disable_wom(indio_dev);
> +
> +	pm_runtime_mark_last_busy(pdev);
> +	pm_runtime_put_autosuspend(pdev);
Check for this throughout. The definition of pm_runtime_put_autosuspend() is
now:
static inline int pm_runtime_put_autosuspend(struct device *dev)
{
	pm_runtime_mark_last_busy(dev);
	return __pm_runtime_put_autosuspend(dev);
}
So drop all separate calls to mark_last_busy()
> +
> +	return ret;
> +}

>  static const int inv_icm42607_accel_scale_nano[][2] = {
>  	[INV_ICM42607_ACCEL_FS_16G] = { 0, 4788403 },
>  	[INV_ICM42607_ACCEL_FS_8G] = { 0, 2394202 },
> @@ -464,6 +650,9 @@ static int inv_icm42607_accel_write_odr(struct iio_dev *indio_dev,
>  		return ret;
>  
>  	ret = inv_icm42607_set_accel_conf(st, &conf, NULL);
> +	if (ret)

> +		return ret;
Blank line here as the two blocks aren't that closely related.
Helps readability (a tiny bit!)
> +	ret = inv_icm42607_accel_set_wom_threshold(st, st->apex.wom.value, val, val2);
>  	if (ret)
>  		return ret;
>  

> +
>  static const struct iio_info inv_icm42607_accel_info = {
>  	.read_raw = inv_icm42607_accel_read_raw,
>  	.write_raw = inv_icm42607_accel_write_raw,
> @@ -586,6 +885,10 @@ static const struct iio_info inv_icm42607_accel_info = {
>  	.update_scan_mode = inv_icm42607_accel_update_scan_mode,
>  	.hwfifo_set_watermark = inv_icm42607_accel_hwfifo_set_watermark,
>  	.hwfifo_flush_to_buffer = inv_icm42607_accel_hwfifo_flush,
> +	.read_event_config = inv_icm42607_accel_read_event_config,
> +	.write_event_config = inv_icm42607_accel_write_event_config,
> +	.read_event_value = inv_icm42607_accel_read_event_value,
> +	.write_event_value = inv_icm42607_accel_write_event_value,
>  };
>  
>  struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st)
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 5c2010d8256f..480fe1741a04 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

> +int inv_icm42607_disable_wom(struct inv_icm42607_state *st)
> +{
> +	int ret;
> +
> +	/* disable WoM interrupt */
> +	ret = regmap_clear_bits(st->map, INV_ICM42607_REG_INT_SOURCE1,
> +				INV_ICM42607_INT_SOURCE1_WOM_INT1_EN);
> +	if (ret)
> +		return ret;
> +
> +	/* disable WoM hardware */
> +	return regmap_clear_bits(st->map, INV_ICM42607_REG_WOM_CONFIG,
> +				 INV_ICM42607_WOM_CONFIG_EN);
> +}
> +
> +

Single line only between functions. 

>  int inv_icm42607_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,

> +
> +static irqreturn_t inv_icm42607_irq_handler(int irq, void *_data)
> +{
> +	struct inv_icm42607_state *st = _data;
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int status;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	if (st->apex.on) {
> +		unsigned int status2, status3;
> +
> +		/* read INT_STATUS2 and INT_STATUS3 in 1 operation */
> +		ret = regmap_bulk_read(st->map, INV_ICM42607_REG_INT_STATUS2, st->buffer, 2);
> +		if (ret)
> +			goto out_unlock;
> +		status2 = st->buffer[0];
> +		status3 = st->buffer[1];
> +		inv_icm42607_accel_handle_events(st->indio_accel, status2, status3,
> +						 st->timestamp.accel);
> +	}
> +
> +	ret = regmap_read(st->map, INV_ICM42607_REG_INT_STATUS, &status);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (status & INV_ICM42607_INT_STATUS_FIFO_FULL)
> +		dev_warn(dev, "FIFO full data lost!\n");
> +
> +	if (status & INV_ICM42607_INT_STATUS_FIFO_THS) {
> +		ret = inv_icm42607_buffer_fifo_read(st, 0);
> +		if (ret) {
> +			dev_err(dev, "FIFO read error %d\n", ret);
> +			goto out_unlock;
> +		}
> +		ret = inv_icm42607_buffer_fifo_parse(st);
> +		if (ret)
> +			dev_err(dev, "FIFO parsing error %d\n", ret);
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&st->lock);

Looks like suitable place for guard() and early returns.


> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * inv_icm42607_irq_init() - initialize int pin and interrupt handler
> + * @st:		driver internal state
> + * @irq:	irq number
> + * @irq_type:	irq trigger type
> + * @open_drain:	true if irq is open drain, false for push-pull
> + *
> + * Returns 0 on success, a negative error code otherwise.
> + */
> +static int inv_icm42607_irq_init(struct inv_icm42607_state *st, int irq,
> +				int irq_type, bool open_drain)
> +{
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int val = 0;
> +	int ret;
> +
> +	switch (irq_type) {
> +	case IRQF_TRIGGER_RISING:
> +	case IRQF_TRIGGER_HIGH:
> +		val = INV_ICM42607_INT_CONFIG_INT1_ACTIVE_HIGH;
> +		break;
> +	default:
> +		val = INV_ICM42607_INT_CONFIG_INT1_ACTIVE_LOW;
> +		break;
> +	}
> +
> +	switch (irq_type) {
> +	case IRQF_TRIGGER_LOW:
> +	case IRQF_TRIGGER_HIGH:
> +		val |= INV_ICM42607_INT_CONFIG_INT1_LATCHED;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (!open_drain)
> +		val |= INV_ICM42607_INT_CONFIG_INT1_PUSH_PULL;
> +
> +	ret = regmap_write(st->map, INV_ICM42607_REG_INT_CONFIG, val);
> +	if (ret)
> +		return ret;
> +
> +	irq_type |= IRQF_ONESHOT;
> +	return devm_request_threaded_irq(dev, irq, inv_icm42607_irq_timestamp,
> +					 inv_icm42607_irq_handler, irq_type,
> +					 st->name, st);
> +}
> +
>  static int inv_icm42607_enable_vddio_reg(struct inv_icm42607_state *st)
>  {
>  	int ret;
> @@ -387,7 +523,7 @@ int inv_icm42607_core_probe(struct regmap *regmap, int chip,
>  	struct device *dev = regmap_get_device(regmap);
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct inv_icm42607_state *st;
> -	int irq;
> +	int irq, irq_type;
>  	bool open_drain;
>  	int ret;
>  
> @@ -402,6 +538,9 @@ int inv_icm42607_core_probe(struct regmap *regmap, int chip,
>  	if (irq < 0)
>  		return dev_err_probe(dev, irq, "error missing INT1 interrupt\n");
>  
> +	irq_type = irq_get_trigger_type(irq);
> +	if (!irq_type)
> +		irq_type = IRQF_TRIGGER_FALLING;
This is cut and pasted from elsewhere. The only reason we ever do this
is to paper over a driver that set the irq type unconditionally and we should
never have done that.  The problem in those historical cases is we don't know
if boards are relying on that behaviour. Hence this hack.

For new code just let the firmware set it up without the driver doing
anything to modify it.  Fine to read it here and fail to use the interrupt
if nothing is set.

>  	open_drain = device_property_read_bool(dev, "drive-open-drain");