> > > +}
> > > +
> > > +/**
> > > + * adxl345_fifo_transfer() - Read samples number of elements.
> > > + * @st: The instance of the state object of this sensor.
> > > + * @samples: The number of lines in the FIFO referred to as fifo_entry,
> > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
> > > + *
> > > + * It is recommended that a multiple-byte read of all registers be performed to
> > > + * prevent a change in data between reads of sequential registers. That is to
> > > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once.
> >
> > Doesn't match the code which is reading just one register lots of times.
>
> This is one of my painpoints, regmap_noinc_read() worked here, for a
> linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1
> in one command. Also, I tried here regmap_bulk_read(). At all, I find
> this solution is working, but I'm not sure if there is not a total
> differnt way to do the read out.
A bulk read is defined as indexing through registers. Eg. ADDR, ADDR + 1, ADDR + 2
etc. regmap_noinc_read() just keeps reading the same register, so is typically
used for fifos.
I opened the datasheet. It seems to say you need to read 3 registers repeatedly
rather than one 3 times as often. There isn't a good way to do that sort of
sequenced read in one go. So you will need a loop like you have, but it
should need a bulk read. Curious it doesn't seem to...
Ah. Device auto increments for both SPI and I2C. So in that case
the noinc_read and normal bulk read will actually issue the same thing and
as these are volatile registers it doesn't matter (it would if you were
caching the result as the data would end up cached in different places).
It will do the wrong thing though if you have an i2c controller that
is less capable and can't do large reads. So you should definitely use
bulk_read not noinc.
Have you set available_scan_masks? If not you want to do so as
per comment I made in the cover letter.
> > > + * @irq: The irq being handled.
> > > + * @p: The struct iio_device pointer for the device.
> > > + *
> > > + * Return: The interrupt was handled.
> > > + */
> > > +static irqreturn_t adxl345_event_handler(int irq, void *p)
> > > +{
> > > + struct iio_dev *indio_dev = p;
> > > + struct adxl345_state *st = iio_priv(indio_dev);
> > > + u8 int_stat;
> > > + int samples;
> > > +
> > > + int_stat = adxl345_get_status(st);
> > > + if (int_stat < 0)
> > > + return IRQ_NONE;
> > > +
> > > + if (int_stat == 0x0)
> > Doesn't this correspond to 'not our interrupt'?
> > If that's the case return IRQ_NONE is the right way to go and not reset the
> > interrupt. You have registered it as maybe shared, and if it is, then this
> > is a common thing to happen as interrupt from another device.
> >
>
> Here I see actually
> + int_stat = adxl345_get_status(st);
> + if (int_stat < 0)
> + return IRQ_NONE; // a bus error, reading register not possible
> ...and then...
> + if (int_stat == 0x0)
> + // interrupt sources were 0, so IRQ not from our sensor
>
> I'm unsure if the first IRQ_NONE here is actually correct. I mean, if
> the bus is not working,
> actually any IRQ usage should be considered broken. Is there a way to
> break out of measuring?
>
It is a much debated thing on what you should return if you have no
idea if it is our interrupt or not. There isn't really a right
answer. If you get a lot of IRQ_NONE and no one else claims it eventually
the interrupt will be disabled (to break the interrupt storm freezing the
machine).
> > > + goto err;
> > > +
> > > + if (int_stat & ADXL345_INT_OVERRUN)
> > > + goto err;
> > > +
> > > + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
> >
> > I think you only ever enable the INT_WATERMARK? If so does it
> > make sense to check for DATA_READY as well?
> >
>
> Watermark comes usually with data ready or overrun. I guess for the
> FIFO watermark, just evaluating watermark is probably sufficient. For
> other events, it then might be notification w/ a data ready set.
> Probably better to introduce data ready when it's actually really
> needed?
Yes. That dataready is normally used when you are doing capture without
the fifo and want to read each sample - kind of same as a watermark depth
of 1, but less hardware turned on. As such, may be no need to ever support it.
return dev_err_probe(dev, ret, "Failed to add action or reset\n");
> > >
> > > + if (st->irq > 0) {
> > > + dev_dbg(dev, "initialize for FIFO_STREAM mode\n");
> > > +
> > > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler,
> > > + IRQF_SHARED | IRQF_ONESHOT,
> > > + indio_dev->name, indio_dev);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> > > +
> > > + } else {
> > > + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n");
> > > +
> > Given you haven't removed this code from elsewhere, was the driver relying
> > on the defaults after reset before this patch?
> >
> > I don't mind having this branch as a form of documentation even if that's
> > true but maybe add a note to the patch description.
> >
>
> I'm not sure if I get you correctly. The driver before only
> implemented "BYPASS" mode. This was the case w/o a defined
> interrupt-name. My intention now is to keep this behavior as fallback.
> If no IRQ is around, i.e. interrupt + interrupt-name, the sensor
> driver will operate the sensor in BYPASS mode.
>
> I was interpreting this also as the "default behavior", you mentioned
> in the dt-binding patch? Is this correct?
>
> What do you mean when the driver is relying on the defaults after reset?
The driver worked without irq before now. Which is same as this path.
So how was the register written here being configured correctly before
this patch? I'm guessing it wasn't and that the value written here
is the default on power up.
Either that or I'm miss understanding what this branch of the if / else is
about.
Jonathan
>
> > > + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS);
> > > +
> > > + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
> > > + if (ret < 0)
> > > + return ret;
> > > + }
> > > return devm_iio_device_register(dev, indio_dev);
> > > }
> > > EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
> >
>
