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
