Re: [PATCH V9 09/11] iio: imu: inv_icm42607: Add IRQ for icm42607

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 29 May 2026 22:17:36 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add IRQ support for the icm42607 driver.
Note I'm only calling out a few things sashiko commented on. Make sure
you verify any others are fixed or false positives.

https://sashiko.dev/#/patchset/20260530031739.109063-1-macroalpha82%40gmail.com

I didn't have anything non sashiko related to add to this patch.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>  .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 97 ++++++++++++++++++-
>  1 file changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 6b623fb679f3..3c91623dffb2 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

> +
> +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);
> +
> +	ret = regmap_read(st->map, INV_ICM42607_REG_INT_STATUS, &status);
> +	if (ret) {
> +		dev_err(dev, "Interrut status read error %d\n", 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) {
> +		mutex_unlock(&st->lock);
> +		ret = inv_icm42607_buffer_fifo_read(st, 0);
> +		if (ret) {
> +			dev_err(dev, "FIFO read error %d\n", ret);
> +			goto out_unlock;
Sashiko caught this one.

Lock isn't held.  This dance is horrible though.  Normally we avoid this
by having an unlocked variant of the inner function 
__inv_icm42607_buffer_fifo_read() and a locked wrapper without the underscores.
Or push the lock out of there in general and add a __must_hold() marking so
we can detect any paths that don't have the lock.

> +		}
> +
> +		mutex_lock(&st->lock);
> +		ret = inv_icm42607_buffer_fifo_parse(st);
> +		if (ret)
> +			dev_err(dev, "FIFO parsing error %d\n", ret);
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&st->lock);
> +	return IRQ_HANDLED;
> +}

> +
>  static int inv_icm42607_enable_vddio_reg(struct inv_icm42607_state *st)
>  {
>  	int ret;
> @@ -367,13 +452,18 @@ int inv_icm42607_core_probe(struct regmap *regmap,
>  {
>  	struct device *dev = regmap_get_device(regmap);
>  	struct inv_icm42607_state *st;
> -	int irq;
> +	int irq, irq_type;
> +	bool open_drain;
>  	int ret;
>  
>  	irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
>  	if (irq < 0)
>  		return dev_err_probe(dev, irq, "Unable to get INT1 interrupt\n");
>  
> +	irq_type = irq_get_trigger_type(irq);
> +
> +	open_drain = device_property_read_bool(dev, "drive-open-drain");
> +
>  	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>  	if (!st)
>  		return -ENOMEM;
> @@ -433,6 +523,11 @@ int inv_icm42607_core_probe(struct regmap *regmap,
>  	if (IS_ERR(st->indio_accel))
>  		return PTR_ERR(st->indio_accel);
>  
> +	/* Initialize interrupt handling */
> +	ret = inv_icm42607_irq_init(st, irq, irq_type, open_drain);

Sashiko asks some stuff about ordering wrt to this call. It is relatively
unusual to register an irq after the driver is exposed to userspace. Tends to
lead to potentially silly races.  Can we move it before device registration
and rely on presence checks to handle any interrupts that we see before those
iio devices are registered?


> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");