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
