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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aSatqG9UEqkH0Glw@smile.fi.intel.com>
Date: Wed, 26 Nov 2025 09:35:04 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Shrikant Raskar <raskar.shree97@...il.com>
Cc: jic23@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
	andy@...nel.org, heiko@...ech.de, neil.armstrong@...aro.org,
	skhan@...uxfoundation.org, david.hunter.linux@...il.com,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: proximity: rfd77402: Add interrupt handling
 support

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