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

[Cosmin Tanislav <demonsingur@gmail.com>]

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.