Re: [PATCH v4 05/11] iio: accel: adxl313: prepare interrupt handling

[Lothar Rubusch]

Hi Andy,

On Sun, Jun 1, 2025 at 9:21 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> >
> > Evaluate the devicetree property for an optional interrupt line, and
> > configure the interrupt mapping accordingly. When no interrupt line
> > is defined in the devicetree, keep the FIFO in bypass mode as before.
>
> ...
>
> >         struct adxl313_data *data;
> >         struct iio_dev *indio_dev;
> > -       int ret;
> > +       u8 int_line;
> > +       u8 int_map_msk;
> > +       int irq, ret;
>
> Why do you need a specific irq variable?
>
> ...
>
> > +       int_line = ADXL313_INT1;
>
> Assign this when we are sure that the INT1 is defined. Current
> approach is not robust.
>
> > +       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;
> > +       }
>
> So, the below code does not use the returned vIRQ, moreover, the above
> code actually does the IRQ mapping. Why do you need that if the code
> doesn't use it?
>
> > +       if (int_line != ADXL313_INT_NONE) {
>
> Why not positive conditional? But see below...
>
> > +               /* FIFO_STREAM mode */
> > +               int_map_msk = ADXL313_INT_DREADY | ADXL313_INT_ACTIVITY |
> > +                       ADXL313_INT_INACTIVITY | ADXL313_INT_WATERMARK |
> > +                       ADXL313_INT_OVERRUN;
> > +               ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,
> > +                                        int_map_msk, int_line == ADXL313_INT2);
>
> This is fragile. It heavily relies on the existence of exactly three
> IRQ variants. Instead of defining special case (NONE) simply use
> whatever is undefined as the default case
>
>   switch (IRQ type) {
>   case 'INT1':
>     ...
>     break;
>   case 'INT2':
>     ...
>     break;
>   default:
>     // FIFO bypass mode
>     ...
>     break;
>   }

The idea here is actually to conditionally try to read if optional
interrupt lines are configured in the DT. First I check if INT1 is
configured. If not, I try INT2. Else, no interrupt line was setup. The
interrupt lines just need to be configured in the mapping register.
So, there is actually nothing more to a case INT1 or case INT2.

With this explanation and from how I also interprete your and
Jonathans commit, I'll go to merge some of the patches for a next
version. I won't change to switch/case here. IMHO it is not the
approach for the above idea (might be wrong).

I appreciate your feedback and have taken note of it. Thank you.

>
> But still, the main question and confusion here is the absence of the
> users of 'irq'.
>
> > +               if (ret)
> > +                       return ret;
> > +       } else {
> > +               /*
> > +                * FIFO_BYPASSED mode
> > +                *
> > +                * When no interrupt lines are specified, the driver falls back
> > +                * to use the sensor in FIFO_BYPASS mode. This means turning off
> > +                * internal FIFO and interrupt generation (since there is no
> > +                * line specified). Unmaskable interrupts such as overrun or
> > +                * data ready won't interfere. Even that a FIFO_STREAM mode w/o
> > +                * connected interrupt line might allow for obtaining raw
> > +                * measurements, a fallback to disable interrupts when no
> > +                * interrupt lines are connected seems to be the cleaner
> > +                * solution.
> > +                */
> > +               ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> > +                                  FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
> > +                                             ADXL313_FIFO_BYPASS));
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> >         return devm_iio_device_register(dev, indio_dev);
> >  }
>
> --
> With Best Regards,
> Andy Shevchenko