Re: [PATCH v3 1/3] iio: proximity: sx9310: keep the IRQ balanced across suspend and resume

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 29 Oct 2026 03:34:37 +0530
Arnav Kapoor <kapoorarnav43@gmail.com> wrote:

> sx9310_suspend() disables the interrupt first and returns with it still
> disabled if a register access fails. Resume isn't called after a failed
> suspend, so the interrupt is never enabled again. sx9310_resume() also
> returns without enabling it on error. Without an interrupt line,
> client->irq is 0 and both callbacks act on IRQ 0, which the driver
> doesn't own.

irq 0 doesn't exist any more on any architectures I think..
It did once, long ago. Today it means no interrupt.

> 
> Disable the interrupt only once the device is asleep, always enable it
> in resume, and skip both when there is no interrupt.
> 
> Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver")
> Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Signed-off-by: Arnav Kapoor <kapoorarnav43@gmail.com>

I'm not keen on suspend and resume not being mirror images of each
other with respect to ordering.

> ---
>  drivers/iio/proximity/sx9310.c | 42 ++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index 79ba46d8a0aa..f824d00f1174 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -943,8 +943,6 @@ static int sx9310_suspend(struct device *dev)
>  	u8 ctrl0;
>  	int ret;
>  
> -	disable_irq_nosync(data->client->irq);
> -
>  	guard(mutex)(&data->mutex);
>  	ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0,
>  			  &data->suspend_ctrl);
> @@ -956,7 +954,29 @@ static int sx9310_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	return regmap_write(data->regmap, SX9310_REG_PAUSE, 0);
> +	ret = regmap_write(data->regmap, SX9310_REG_PAUSE, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Only once nothing can fail, as resume isn't called on failure. */
> +	if (data->client->irq)
> +		disable_irq_nosync(data->client->irq);
> +
> +	return 0;
> +}
> +
> +static int sx9310_wake_up(struct sx_common_data *data)
> +{
> +	struct regmap *map = data->regmap;
> +	int ret;
> +
> +	guard(mutex)(&data->mutex);
> +
> +	ret = regmap_write(map, SX9310_REG_PAUSE, 1);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(map, SX9310_REG_PROX_CTRL0, data->suspend_ctrl);
>  }
>  
>  static int sx9310_resume(struct device *dev)
> @@ -964,19 +984,13 @@ static int sx9310_resume(struct device *dev)
>  	struct sx_common_data *data = iio_priv(dev_get_drvdata(dev));
>  	int ret;
>  
> -	scoped_guard(mutex, &data->mutex) {
> -		ret = regmap_write(data->regmap, SX9310_REG_PAUSE, 1);
> -		if (ret)
> -			return ret;
> +	ret = sx9310_wake_up(data);
>  
> -		ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
> -				   data->suspend_ctrl);
> -		if (ret)
> -			return ret;
> -	}
> +	/* Balance the suspend even if the device couldn't be woken up. */
> +	if (data->client->irq)
> +		enable_irq(data->client->irq);
If we are going to reorder suspend, I'd expect this to also move
and be done before the wake up. I think that should be safe enough?
>  
> -	enable_irq(data->client->irq);
> -	return 0;
> +	return ret;
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(sx9310_pm_ops, sx9310_suspend, sx9310_resume);