| Message ID | 20200330145705.29447-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: buffer: add output buffer support for chrdev | expand |
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(-) >
[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_voltage1_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(-)