Re: [PATCH v3 2/2] iio: adc: ad4130: add AD4130 driver

Cosmin Tanislav
2022-05-07T16:50:35Z

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.