Re: [RFC PATCH 0/3] iio: buffer: add output buffer support for chrdev

Ardelean, Alexandru
2020-03-30T14:58:18Z

On Mon, 2020-03-30 at 17:57 +0300, Alexandru Ardelean wrote:

I goofed Michael's email on the first run.
Sorry for the spam.

> [This description is also present on patch 3/3, there have been some
> cosmetic stuff since that one that I did not remove. These patches
> probably won't make it in a final version, but they're there because
> I changed my mind a few times and got to this RFC]
>
> There have been some offline discussions about how to go about this.
> Since I wasn't involved in any of those discussions, this kind of tries to
> re-build things from various bits.
> It's incomplete work, since I don't have a clear idea of what the
> accepted/acceptable
> approach would be.
>
> 1. First approach is: we keep 1 buffer per device, and we make it either
> in/out, which means that for devices that for devices that have both in/out
> 2 iio_dev instances are required, or an ADC needs to be connected on the in
> path and a DAC on out-path. This is predominantly done in the ADI tree.
>
> 2. One discussion/proposal was to have multiple buffers per-device. But the
> details are vague since they were relayed to me.
> One detail was, to have indexes for devices that have more than 1
> buffer:
>
> Iio_deviceX:
> buffer
> scan_elements
>
> Iio_deviceX:
> BufferY
> scan_elementsY
> BufferZ
> scan_elementsZ
>
> I am not sure how feasible this is for a single chrdev, as when you look at
> the fileops that get assigned to a chrdev, it looks like it can have at
> most two buffers (one for input, out for output).
>
> Multiplexing input buffers can work (from ADCs), but demultiplexing output
> buffers into a DAC, not so well. Especially on a single chrdev.
>
> Question 1: do we want to support more than 2 buffers per chrdev?
>
> This is what ADI currently has in it's tree (and works).
>
> Example, ADC:
> # ls iio\:device3/buffer/
> data_available enable length length_align_bytes watermark
> # ls iio\:device3/scan_elements/
> in_voltage0_en in_voltage0_index in_voltage0_type in_voltage1_en in_volta
> ge1_index in_voltage1_type
>
> Example, DAC:
> # ls iio\:device4/buffer/
> data_available enable length length_align_bytes watermark
> # ls iio\:device4/scan_elements/
> out_voltage0_en out_voltage0_type out_voltage1_index out_voltage2_en
> out_voltage2_type out_voltage3_index
> out_voltage0_index out_voltage1_en out_voltage1_type out_voltage2_index
> out_voltage3_en out_voltage3_type
>
> The direction of each element is encoded into the filename of each channel.
>
> Another question is:
> Does it make sense to have more than 1 'scan_elements' folder?
> That is, for devices that would have both in & out channels.
>
> For 'buffer' folders I was thinking that it may make sense to have,
> 'buffer_in' && 'buffer_out'.
>
> So, one idea is:
>
> Iio_deviceX:
> buffer_in
> buffer_out
> scan_elements
>
> Currently, this patch kind of implements 2 buffers per iio_dev/chrdev.
> But the format is:
>
> Iio_deviceX:
> buffer_in
> buffer_out
> scan_elements_in
> scan_elements_out
>
> Obviously it shouldn't work as-is [as it wasn't tested], but at least gives
> some glimpse of where this could go.
>
> 3. A side question is about the 'iio_buffer -> pollq' field. I was
> wondering if it would make sense to move that on to 'iio_dev pollq' if
> adding multiple buffers are added per-device. It almost makes sense to
> unify the 'pollq' on indio_dev.
> But, it looks a bit difficult, and would require some more change [which is
> doable] if it makes sense for whatever reason.
> The only reason to do it, is because the iio_buffer_fileops has a .poll =
> iio_buffer_poll() function attached to it. Adding multiple buffers for an
> IIO device may require some consideration on the iio_buffer_poll() function
> as well.
>
> Alexandru Ardelean (3):
> iio: core: rename 'indio_dev->buffer_list' ->
> 'indio_dev->active_buffers'
> iio: buffer: extend short-hand use for 'indio_dev->buffer'
> iio: buffer: add output buffer support for chrdev
>
> .../buffer/industrialio-buffer-dmaengine.c | 3 +-
> drivers/iio/industrialio-buffer.c | 267 +++++++++++++-----
> drivers/iio/industrialio-core.c | 5 +-
> include/linux/iio/buffer.h | 9 +
> include/linux/iio/buffer_impl.h | 7 +
> include/linux/iio/iio.h | 12 +-
> 6 files changed, 225 insertions(+), 78 deletions(-)
>