On Sat, Dec 14, 2024 at 1:36 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri, 13 Dec 2024 21:19:08 +0000
> Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
>
> > Add a basic setup for FIFO with configurable watermark. Add a handler
> > for watermark interrupt events and extend the channel for the
> > scan_index needed for the iio channel. The sensor is configurable to use
> > a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now
> > a watermark can be configured, or disabled by setting 0. Further features
> > require a working FIFO setup.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
>
> I missed a theoretical bug around the dma buffer in previous reviews.
> A few other minor things inline.
>
> Thanks,
>
> Jonathan
>
> > /*
> > * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index fc4f89f22..e31a7cb3f 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -15,9 +15,17 @@
> >
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/kfifo_buf.h>
> >
> > #include "adxl345.h"
> >
> > +#define ADXL345_FIFO_BYPASS 0
> > +#define ADXL345_FIFO_FIFO 1
> > +#define ADXL345_FIFO_STREAM 2
> > +
> > +#define ADXL345_DIRS 3
> > +
> > #define ADXL345_INT_NONE 0xff
> > #define ADXL345_INT1 0
> > #define ADXL345_INT2 1
> > @@ -26,27 +34,68 @@ struct adxl345_state {
> > int irq;
> > const struct adxl345_chip_info *info;
> > struct regmap *regmap;
> > + __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1];
>
> Ah. The old corner case (sorry I missed this in earlier reviews!)
>
> In theory at least regmap makes no additional guarantees on how it uses buffers
> on top of those provided by the underlying busses.
> So we need to treat bulk reads the same way we do for those buses.
> That means we need to allow for direct DMA writes into the buffers
> we pass to bulk read. For that to be safe on all architectures (and not
> accidentally overwrite stuff in the same cacheline) we need to use
> what is known as a DMA safe buffer.
> Long ago we contrived the data layout in IIO to make that easy to
> do. Just move it down to the end of this structure as...
>
>
> > bool fifo_delay; /* delay: delay is needed for SPI */
> > u8 intio;
> > + u8 int_map;
> > + u8 watermark;
> > + u8 fifo_mode;
> this
> __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
>
> This structure already has the appropriate alignment (there is padding inserted
> in the allocation to make that true) so by doing this for the element we
> ask the compiler to make sure it adds sufficient padding before this element
> to ensure nothing else is in the same cacheline (if required by a particular
> architecture). C also guarantees that the structure itself is large enough
> to ensure padding is added after this element such that the overall structure
> size is a multiple of it's most aligned element (this one).
> Thus we end up with the buffer in it's own cacheline and none of the mess
> of non coherent DMA can cause us problems.
>
> If interested in learning more look for Wolfram's talk at ELCE a number
> of years back on trying to do DMA into the buffers passed to I2C calls.
>
> The 'in theory' bit is because last time I checked regmap is always bounce
> buffering the data but there are obvious optimizations that could be made
> so it didn't bounce. A long back we asked the maintainer about this and he
> said definitely don't assume it won't use the buffers directly.
>
> Note on arm64 for instance there is magic code that bounces all small
> DMA transfers, but that is not enabled on all architectures yet.
Initially, I just copied the buffer definition for the fifo_buf from
other drivers, inclusively the DMA alignment. In the hope 'probably
works similarly' adn without further understanding how the makro
works. Surely w/o knowing how to further verify, that it actually was
working.
When you remarked that my usage of the DMA alignment never could work
that way. I noticed that it was (probably) actually not even needed to
explicitely declare an IIO_DMA_MINALIGN, at least for my setup. So, to
not make it overly complicated, I simply dropped it.
I probably should have asked more questions, you now answered in
detail. I appreciate.
> > };
>
> >
> > +static const unsigned long adxl345_scan_masks[] = {
> > + BIT(chan_x) | BIT(chan_y) | BIT(chan_z),
> > + 0,
>
> Trivial but drop that trailing comma. It's a terminator so we don't want to make it
> easy for anyone to accidentally put anything after it!
>
> > +};
>
> > static int adxl345_read_raw(struct iio_dev *indio_dev,
> > struct iio_chan_spec const *chan,
> > int *val, int *val2, long mask)
> > @@ -132,6 +181,25 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> > return -EINVAL;
> > }
> >
> > +static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > +{
> > + struct adxl345_state *st = iio_priv(indio_dev);
> > + unsigned int fifo_mask = 0x1F;
> > + int ret;
> > +
> > + if (value > ADXL345_FIFO_SIZE)
> > + value = ADXL345_FIFO_SIZE - 1;
>
> value = min(value, AD345_FIFO_SIZE - 1);
>
> Shorter and maybe a tiny bit more readable. (trivial comment!)
>
> > +
> > + ret = regmap_update_bits(st->regmap, ADXL345_REG_FIFO_CTL, fifo_mask, value);
> > + if (ret)
> > + return ret;
> > +
> > + st->watermark = value;
> > + st->int_map |= ADXL345_INT_WATERMARK;
> > +
> > + return 0;
> > +}
>
>
>
> > /**
> > @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > ADXL345_DATA_FORMAT_JUSTIFY |
> > ADXL345_DATA_FORMAT_FULL_RES |
> > ADXL345_DATA_FORMAT_SELF_TEST);
> > + u8 fifo_ctl;
>
> Might as well make this declaration local to where it's used.
>
> > int ret;
> >
> > indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > @@ -242,6 +520,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > indio_dev->modes = INDIO_DIRECT_MODE;
> > indio_dev->channels = adxl345_channels;
> > indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
> > + indio_dev->available_scan_masks = adxl345_scan_masks;
> >
> > if (setup) {
> > /* Perform optional initial bus specific configuration */
> > @@ -292,6 +571,25 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > st->intio = ADXL345_INT_NONE;
> > }
> >
> > + if (st->intio != ADXL345_INT_NONE) {
> > + /* FIFO_STREAM mode is going to be activated later */
> > + 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_irq_handler,
> > + IRQF_SHARED | IRQF_ONESHOT,
> > + indio_dev->name, indio_dev);
> ret = devm_request_threaded_irq(dev, st->irq, NULL,
> &adxl345_irq_handler,
> IRQF_SHARED | IRQF_ONESHOT,
> indio_dev->name, indio_dev);
>
> All under 80 chars (still the preference when no reason to do otherwise) and
> aligned with opening bracket which is preferred kernel style when there
> is no reason to do otherwise.
>
> If you weren't going to be doing a v8 anyway I'd just tweak this whilst applying
> but seeing as you are... :)
>
>
> > + if (ret)
> > + return ret;
> > + } else {
> > + /* FIFO_BYPASS mode */
> > + 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);
>
