On Sat, 14 May 2022 16:32:25 +0300
Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
> On 5/7/22 20:04, Jonathan Cameron wrote:
> > On Sat, 7 May 2022 19:49:17 +0300
> > Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
> >
> >> 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?
> >
> > Any time next week should be fine. If it ends up later that's fine as well.
> > We have at least a few weeks until the 5.18 release and then if we were to land
> > this during the first few weeks of the next cycle that would be fine as well.
> > No one should be insane enough to not pick up at least the first few stable
> > releases of a new kernel!
> >
> >> 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.
> >
> > Perfect :)
> >
> > Thanks for sorting this out.
> >
> > Jonathan
> >
> >
>
> I've just had another good look at ADXL367. It seems that I'm quick at
> forgetting stuff about the code I write.
>
> In adxl367_set_fifo_samples(), fifo_watermark is multiplied by
> fifo_set_size, then, if it is larger than the maximum watermark,
> it is capped at the maximum watermark, and then, it is divided
> by fifo_set_size again.
>
> In the end, fifo_watermark actually seems to mean number of scans
> in that driver too.
>
> So this was a huge confusion. The different thing about AD4130 is that
> the register actually means number of samples, and not number of scans,
> so that's where that confussion stemmed from.
>
> Sorry for wasting your time on this.
No problem and thanks for looking into this in more depth!
Jonathan
