Re: [PATCH 3/3] iio: proximity: rfd77402: Add interrupt handling support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Nov 26, 2025 at 08:44:40AM +0530, Shrikant Raskar wrote:
> Add interrupt handling support to enable event-driven data acquisition
> instead of continuous polling. This improves responsiveness, reduces
> CPU overhead, and supports low-power operation by allowing the system
> to remain idle until an interrupt occurs.

...

>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
> -

Stray removal of blank line.

> +#include <linux/interrupt.h>
> +#include <linux/completion.h>

> +#include <linux/of.h>

Please, avoid using of.h in a new code.

>  #include <linux/iio/iio.h>

...

> +static irqreturn_t rfd77402_interrupt_handler(int irq, void *dev_id)
> +{
> +	struct rfd77402_data *data = dev_id;
> +	int ret;

> +	/* Check if the interrupt is from our device */

This comment only for the second part and I would split the condition to make
it clearer.

> +	ret = i2c_smbus_read_byte_data(data->client, RFD77402_ICSR);
> +	if (ret < 0 || !(ret & RFD77402_ICSR_RESULT))
> +		return IRQ_NONE;

	ret = i2c_smbus_read_byte_data(data->client, RFD77402_ICSR);
	if (ret < 0)
		return IRQ_NONE;

	/* Check if the interrupt is from our device */
	if (!(ret & RFD77402_ICSR_RESULT))
		return IRQ_NONE;

> +	/* Signal completion of measurement */
> +	complete(&data->completion);
> +	return IRQ_HANDLED;
> +}

...

> -	while (tries-- > 0) {
> -		ret = i2c_smbus_read_byte_data(client, RFD77402_ICSR);
> -		if (ret < 0)
> +	if (data->irq_en) {
> +		/* Wait for interrupt-driven completion */
> +		ret = wait_for_completion_timeout(&data->completion,
> +						  msecs_to_jiffies(200));
> +		if (ret == 0) {
> +			ret = -ETIMEDOUT;
>  			goto err;
> -		if (ret & RFD77402_ICSR_RESULT)
> -			break;
> -		msleep(20);
> -	}
> -
> -	if (tries < 0) {
> -		ret = -ETIMEDOUT;
> -		goto err;
> +		}
> +	} else {
> +		/* Fallback to polling mode */
> +		while (tries-- > 0) {
> +			ret = i2c_smbus_read_byte_data(client, RFD77402_ICSR);
> +			if (ret < 0)
> +				goto err;
> +			if (ret & RFD77402_ICSR_RESULT)
> +				break;
> +			msleep(20);
> +		}
> +
> +		if (tries < 0) {
> +			ret = -ETIMEDOUT;
> +			goto err;
> +		}
>  	}

Instead, move the current code into a helper (in a separate patch) and alter it
here with new conditional. So in the result it will be something like

	if (...)
		ret = call_new_helper_for_irq();
	else
		ret = call_old_helper_for_polling();

...

> +	if (data->irq_en) {
> +		/* Configure ICSR: auto-clear on read, push-pull, falling edge */
> +		ret = i2c_smbus_write_byte_data(client, RFD77402_ICSR,
> +						RFD77402_ICSR_CLR_CFG |
> +						RFD77402_ICSR_INT_MODE);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Enable 'new data available' interrupt in IER */
> +		ret = i2c_smbus_write_byte_data(client, RFD77402_IER,
> +						RFD77402_IER_RESULT);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		/* Disable interrupts */
> +		ret = i2c_smbus_write_byte_data(client, RFD77402_ICSR, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = i2c_smbus_write_byte_data(client, RFD77402_IER, 0);
> +		if (ret < 0)
> +			return ret;
> +	}

This can be also factored out to a helper(s). Something like this, perhaps

	if (irq_en)
		ret = call_a_helper(client, $CSR, $ER);
	else
		ret = call_a_helper(client, 0, 0);

...

> +	/* Check if interrupt is mentioned in device tree */
> +	data->irq_en = false;
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, rfd77402_interrupt_handler,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +						"rfd77402", data);
> +		if (ret == 0) {
> +			data->irq_en = true;
> +			dev_info(&client->dev, "Using interrupt mode\n");
> +		} else {
> +			dev_warn(&client->dev,
> +				 "Failed to request IRQ %d, using polling mode: %d\n",
> +				 client->irq, ret);

If we asked for interrupt and didn't get it due to "linux" errors, we should
not fallback. No need to work around bugs in the DT, the DT description must
be fixed instead.

> +		}
> +	} else {
> +		dev_info(&client->dev, "No interrupt specified, using polling mode\n");
> +	}

-- 
With Best Regards,
Andy Shevchenko