linux-kernel - Re: [PATCH v2 4/4] 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: <CAHp75VdE6LMb=7V1iECwQ_GTyjM+2Ffh_3ifa3dsjpgbPHp1Lw@mail.gmail.com>
Date: Sun, 30 Nov 2025 19:01:58 +0200
From: Andy Shevchenko <andy.shevchenko@...il.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 v2 4/4] iio: proximity: rfd77402: Add interrupt handling support

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