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

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

On Thu, Jan 01, 2026 at 09:47:41PM +0530, Shrikant Raskar via B4 Relay 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>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
>  #include <linux/iopoll.h>

Same comment as per previous patch. Do not add even more misordering, please.

...

> +/**
> + * struct rfd77402_data - device-specific data for the RFD77402 sensor
> + * @client: I2C client handle
> + * @lock: mutex to serialize sensor reads
> + * @completion: completion used for interrupt-driven measurements
> + * @irq_en: indicates whether interrupt mode is enabled
> + */
>  struct rfd77402_data {
>  	struct i2c_client *client;
> -	/* Serialize reads from the sensor */
>  	struct mutex lock;
> +	struct completion completion;
> +	bool irq_en;
>  };

The kernel-doc conversion can be a separate patch, but I'm not insisting.

...

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

> +	if (!data || !data->client)
> +		return IRQ_NONE;

How is this possible to be non-dead code?

> +	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;
> +}

...

> +static int rfd77402_wait_for_irq(struct rfd77402_data *data)
> +{
> +	int ret;

Missed blank line. Doesn't checkpatch complain?

> +	/* As per datasheet, single measurement flow takes 100ms */

Please, be more specific about datasheet, i.e. which Chapter/Section (with its
number and possible name) or Table specifies this.

> +	ret = wait_for_completion_timeout(&data->completion,
> +					  msecs_to_jiffies(100));
> +	if (ret == 0)
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}

...

> +static int rfd77402_measure(struct rfd77402_data *data)
>  {
> +	struct i2c_client *client = data->client;

This (conversion to data instead of client) can be split into a separate
precursor change.a but it seems not a big deal. Up to maintainers.

...

> -	/* Poll ICSR until RESULT bit is set */
> -	ret = read_poll_timeout(i2c_smbus_read_byte_data, ret,
> -				ret & RFD77402_ICSR_RESULT,
> -				10000,    /* sleep: 10ms */
> -				100000,   /* timeout: 100ms */
> -				false,
> -				client, RFD77402_ICSR);
> +	if (data->irq_en) {
> +		/* Re-initialize completion and wait for interrupt */
> +		reinit_completion(&data->completion);
> +		ret = rfd77402_wait_for_irq(data);
> +	} else {
> +		/* Poll ICSR until RESULT bit is set */
> +		ret = read_poll_timeout(i2c_smbus_read_byte_data, ret,
> +					ret & RFD77402_ICSR_RESULT,
> +					10000,      /* sleep 10ms */
> +					100000,     /* timeout 100ms */
> +					false,
> +					client, RFD77402_ICSR);
> +	}

This is ping-pong type of change. You just introduced it a patch ago. Make sure
you don't remove/modify (too much at least) the lines that were just added.

One of the possible technique to achieve this is to use a helper function.

...

>  static int rfd77402_init(struct i2c_client *client)
>  {
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct rfd77402_data *data = iio_priv(indio_dev);

Can't this now take the data as above modified functon?

...

> -	mutex_init(&data->lock);
> +	ret = devm_mutex_init(&client->dev, &data->lock);
> +	if (ret)
> +		return ret;

In my opinion this deserves a separate change.

-- 
With Best Regards,
Andy Shevchenko