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

[Lothar Rubusch]

Hi Andy,

On Mon, Jun 16, 2025 at 11:33 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Mon, Jun 16, 2025 at 1:23 AM Lothar Rubusch <l.rubusch@xxxxxxxxx> 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