linux-kernel - Re: [PATCH v5 3/8] iio: accel: adxl313: add buffered FIFO watermark with interrupt handling

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: <CAFXKEHan_NnmMUfuy9cyjgBsdFK+C6V7eVxq_V3--afzQYGDDQ@mail.gmail.com>
Date: Tue, 17 Jun 2025 11:27:50 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com, 
	andy@...nel.org, corbet@....net, lucas.p.stankus@...il.com, lars@...afoo.de, 
	Michael.Hennerich@...log.com, bagasdotme@...il.com, linux-iio@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/8] iio: accel: adxl313: add buffered FIFO watermark
 with interrupt handling

Hi Andy,

On Mon, Jun 16, 2025 at 11:33 AM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> On Mon, Jun 16, 2025 at 1:23 AM Lothar Rubusch <l.rubusch@...il.com> wrote:
> >
> > Cover the following tasks:
> > – Add scan_mask and scan_index to the IIO channel configuration. The
> > scan_index sets up buffer usage. According to the datasheet, the ADXL313
> > uses a 13-bit wide data field in full-resolution mode. Set the
> > signedness, number of storage bits, and endianness accordingly.
> >
> > – Parse the devicetree for an optional interrupt line and configure the
> > interrupt mapping based on its presence. If no interrupt line is
> > specified, keep the FIFO in bypass mode as currently implemented.
> >
> > – Set up the interrupt handler. Add register access to detect and
> > evaluate interrupts. Implement functions to clear status registers and
> > reset the FIFO.
> >
> > – Implement FIFO watermark configuration and handling. Allow the
> > watermark level to be set, evaluate the corresponding interrupt, read
> > the FIFO contents, and push the data to the IIO channel.
>
> ...
>
> > +       int_line = ADXL313_INT1;
> > +       irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> > +       if (irq < 0) {
> > +               int_line = ADXL313_INT2;
> > +               irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> > +               if (irq < 0)
> > +                       int_line = ADXL313_INT_NONE;
> > +       }
> > +
> > +       if (int_line != ADXL313_INT_NONE) {
>
> > +       } else {
>
> > +       }
>
> What I meant is something like this:
>
>
>        int_line = ADXL313_INT_NONE;
>        irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
>        if (irq > 0) {
>               int_line = ADXL313_INT1;
>        } else {
>                irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
>                if (irq > 0)
>                       int_line = ADXL313_INT2;
>        }
>
>        if (int_line == ADXL313_INT_NONE) {
>    ...
>        } else {
>    ...
>        }
>

I probably got this wrong. I interpreted Jonathans review [PATCH v3
06/12] in the above way. Anyway, I did not read his second phrase. I
agree, flipping if / else case and going by '==' instead of '!='
simplifies it.
   ...
    > +
    > + if (int_line == ADXL313_INT1 || int_line == ADXL313_INT2) {

    Why not int_line != ADXL313_INT_NONE ?
    Or flip the logic so that you do that case first.
    ...
https://marc.info/?l=linux-iio&m=174817641830144&w=2


> Obviously with a helper you can unnest the if-else-if above.
>
> static unsigned int _get_int_type(...)
> {
>     if (irq > 0)
>         return ...
>     return _NONE;
> }
>

Well, indeed. That's definitely the obvious simplification needed
here. Thanks for pointing out.

> --
> With Best Regards,
> Andy Shevchenko