Messages in this thread | | | Date | Sat, 7 May 2022 19:49:17 +0300 | Subject | Re: [PATCH v3 2/2] iio: adc: ad4130: add AD4130 driver | From | Cosmin Tanislav <> |
| |
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.
| |