| 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
| ||
|
Message-ID: <CAHc1_P5pgBGiHpyNBGMf8yDKZttVG0XoC0Bb5mWCeGyKbc6q7Q@mail.gmail.com>
Date: Fri, 28 Nov 2025 21:36:43 +0530
From: Shrikant <raskar.shree97@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.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 1:05 PM Andy Shevchenko
<andriy.shevchenko@...el.com> wrote:
>
> 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");
> > + }
>
Thank you for the detailed feedback. I will update the code as per
feedback and will share the v2 of the patch.
Regards,
Shrikant
Powered by blists - more mailing lists