On 5/7/22 19:35, Jonathan Cameron wrote:
+static int ad4130_set_fifo_watermark(struct iio_dev *indio_dev, unsigned int val)
+{
+ struct ad4130_state *st = iio_priv(indio_dev);
+ unsigned int eff;
+ int ret;
+
+ if (val > AD4130_FIFO_SIZE)
+ return -EINVAL;
+
+ /*
+ * Always set watermark to a multiple of the number of enabled channels
+ * to avoid making the FIFO unaligned.
+ */
+ eff = rounddown(val, st->num_enabled_channels);
+
+ mutex_lock(&st->lock);
+
+ ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
+ AD4130_WATERMARK_MASK,
+ FIELD_PREP(AD4130_WATERMARK_MASK,
+ ad4130_watermark_reg_val(eff)));
+ if (ret)
+ goto out;
+
+ st->effective_watermark = eff;
+ st->watermark = val;
Hmm this is a potential inconsistency in the IIO ABI.
ABI docs describes watermark as being number of 'scan elements' which is
not the clearest text we could have gone with...
Now I may well have made a mistake in the following as it's rather a long time
since I last looked at the core handling for this...
The core treats it as number datum (which is same as a scan) when using
it for the main watermark attribute and also when using watermarks with the
kfifo (the IIO fifo is made up of objects each of which is a scan. So kfifo_len()
returns the number of scans.
Looking very quickly at a few other drivers
adxl367 seems to use number of samples.
adxl372 is using number of scans.
bmc150 hardware seems to work on basis of frame count which I 'think' is probably scans.
fxls8962 uses 'samples count' which is not clearly defined in the datasheet but there
is an example showing that it's scans (I think)...
lsm6dsx - some of the fifos used with this are based on tagged data so the connection to
what hits the front end buffers is non obvious.
So, not great for consistency :(
Going forwards i think we should standardize the hardware fifo watermark on what is being
used for the software watermark which I believe is number of scans.
Not necessary much we can do about old drivers though due to risk of breaking ABI...
We should make the documentation clearer though.
I was confused too, but this seemed more logical to me at the time, and
since you didn't say anything regarding it on ADXL367, I did it the same
way here. I guess we can't go back and change it now on ADXL367, I'm
sorry for this. I'll fix it.
I missed it. Review is never perfect (mine definitely aren't!)
Thinking more on the adxl367. We still have a window to fix that as
the driver isn't yet in a release kernel. Would you mind spinning a
patch to fix that one? Even if we miss the rc cycle (it's a bit tight
timing wise) we can sneak it in as an early fix in stable without
significant risk of breaking anyone's userspace.
I hope Monday is not too late to do it?
I can also try to do the changes tomorrow but I don't have the hardware
anymore so I won't be able to test until I get it back, which is only
next week.
There might be other drivers that have that interpretation we can't
fix but if we can reduce the scope of the problem by changing the adxl367
that would be great.
We should also definitely improve the docs and perhaps add a note to say
that due to need to maintain ABI, a few drivers use scans * number of channels
rather than scans.
I guess I could also do that at the same time.
