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

[Andy Shevchenko]

On Sun, Nov 30, 2025 at 5:38 PM Shrikant Raskar
<raskar.shree97@xxxxxxxxx> 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>

> -

Strat blank line removal.

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

These should be integrated to the block above the (removed) blank line.

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

...

>         struct i2c_client *client;
>         /* Serialize reads from the sensor */
>         struct mutex lock;
> +       /* Completion for interrupt-driven measurements */
> +       struct completion completion;
> +       /* Flag to indicate if interrupt is available */
> +       bool irq_en;

Instead of commenting each field separately, just convert existing to
a kernel-doc format (can be done in a separate change).

...

> +static int rfd77402_wait_for_irq(struct rfd77402_data *data)
> +{
> +       int ret;
> +
> +       ret = wait_for_completion_timeout(&data->completion,
> +                                         msecs_to_jiffies(200));

Perhaps a comment on the chosen value for timeout?

> +       if (ret == 0)
> +               return -ETIMEDOUT;
> +
> +       return 0;
> +}

...

> -       ret = rfd77402_result_polling(client);
> +       if (data->irq_en)
> +               ret = rfd77402_wait_for_irq(data);
> +       else
> +               ret = rfd77402_result_polling(data->client);

> +

Stray blank line addition. The check below is tightened to the upper if-else.

>         if (ret < 0)
>                 goto err;

...

> -       /* configure INT pad as push-pull, active low */
> -       ret = i2c_smbus_write_byte_data(client, RFD77402_ICSR,
> -                                       RFD77402_ICSR_INT_MODE);
> +       if (data->irq_en) {
> +       /*
> +        * Enable interrupt mode:
> +        * - Configure ICSR for auto-clear on read, push-pull output and falling edge
> +        * - Enable "result ready" interrupt in IER
> +        */

This should be indented to the ret = ... below.

> +               ret = rfd77402_config_irq(client,
> +                                         RFD77402_ICSR_CLR_CFG |
> +                                         RFD77402_ICSR_INT_MODE,
> +                                         RFD77402_IER_RESULT);
> +       } else {
> +       /*
> +        * Disable all interrupts:
> +        * - Clear ICSR configuration
> +        * - Disable all interrupt in IER
> +        */

Ditto.

> +               ret = rfd77402_config_irq(client, 0, 0);
> +       }

> +

Stray blank line addition.

>         if (ret < 0)
>                 return ret;

...

>         mutex_init(&data->lock);

This should be  converted to devm_mutex_init(). Otherwise the below
can be called on a (potentially) destroyed mutex.

...

> +       data->irq_en = false;
> +       if (client->irq > 0) {
> +               /* interrupt mode */
> +               ret = devm_request_threaded_irq(&client->dev, client->irq,
> +                                               NULL, rfd77402_interrupt_handler,

> +                                               IRQF_TRIGGER_FALLING | IRQF_ONESHOT,

What's wrong with the FW description for the IRQ line flags?! Why
can't it be fixed / written correctly to begin with?

> +                                               "rfd77402", data);
> +               if (ret < 0) {

> +                       dev_err(&client->dev,
> +                               "Failed to request IRQ %d: %d\n",
> +                               client->irq, ret);

Drop the dup messages, it will be printed by the above call.

> +                       return ret;
> +               }
> +
> +               data->irq_en = true;
> +               dev_info(&client->dev, "Using interrupt mode\n");

> +

Stray blank line addition.

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

-- 
With Best Regards,
Andy Shevchenko