Re: [PATCH v4 06/11] iio: accel: adxl313: add basic interrupt handling for FIFO watermark

[Jonathan Cameron]

On Sun, 1 Jun 2025 22:26:42 +0300
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> >
> > Prepare the interrupt handler. Add register entries to evaluate the
> > incoming interrupt. Add functions to clear status registers and reset the
> > FIFO.
> >
> > Add FIFO watermark configuration and evaluation. Let a watermark to be
> > configured. Evaluate the interrupt accordingly. Read out the FIFO content
> > and push the values to the IIO channel.  
> 
> ...
> 
> > +static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > +{
> > +       struct adxl313_data *data = iio_priv(indio_dev);
> > +       const unsigned int fifo_mask = 0x1f, interrupt_mask = 0x02;  
> 
> GENMASK()
> BIT()
> 
> > +       int ret;
> > +
> > +       value = min(value, ADXL313_FIFO_SIZE - 1);
> > +
> > +       ret = regmap_update_bits(data->regmap, ADXL313_REG_FIFO_CTL,
> > +                                fifo_mask, value);
> > +       if (ret)
> > +               return ret;
> > +
> > +       data->watermark = value;
> > +
> > +       return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> > +                                 interrupt_mask, ADXL313_INT_WATERMARK);
> > +}  
> 
> ...
> 
> > +static int adxl313_get_samples(struct adxl313_data *data)
> > +{
> > +       unsigned int regval = 0;  
> 
> Useless assignment.
> 
> > +       int ret;
> > +
> > +       ret = regmap_read(data->regmap, ADXL313_REG_FIFO_STATUS, &regval);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return FIELD_GET(ADXL313_REG_FIFO_STATUS_ENTRIES_MSK, regval);
> > +}  
> 
> ...
> 
> > +               ret = devm_request_threaded_irq(dev, irq, NULL,
> > +                                               &adxl313_irq_handler,
> > +                                               IRQF_SHARED | IRQF_ONESHOT,
> > +                                               indio_dev->name, indio_dev);
> > +               if (ret)
> > +                       return ret;  
> 
> Now I see the first user of 'irq'. Logically these two patches may not
> be split. Or split should be made differently, let's say IRQ type
> holding variable + switch case can go in the first preparatory patch
> (however it will make a little sense without real users, as it is/will
> be a dead code).
> 

I'd just combine these two patches and patch 2 (which is also dead code
until this one is in place).

Jonathan