| Message ID | 20240216-iio-backend-axi-dds-v1-0-22aed9fb07a1@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: dac: support IIO backends on the output direction | expand |
On Fri, 16 Feb 2024 15:10:49 +0100 Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > Hi all, > My first comment here isn't don't worry too much about getting it perfectly correct from the start. Until you have half a dozen or more drivers using this stuff you won't have seen enough to get generalizations right. As this stuff is all in kernel and not exposed to userspace, that isn't a problem. We can evolve and improve this as we go along. Userspace ABI design is much worse as you are stuck with your errors for ever (so never finalize that until you have a good number of drivers to try it with!) > This RFC is mainly because I'm not getting extremely happy with the > direction of the API in the series. So I think it's better to have the > discussion now so the actual series will look better. Also note that patch > 2 to 5 are brought from Paul's series to bring output buffer support to > DMA buffers [1]. > > So, the main API I'm speaking about is: > - iio_backend_read_phase() > - iio_backend_write_phase() > - iio_backend_read_scale() > - iio_backend_write_scale() > - iio_backend_read_frequency() > - iio_backend_write_frequency() > > All the above is basically ABI/attributes that belong and are supported > by the AXI_DAC IP core. The main things I dislike are: > * The sample_freq and tone_id in iio_backend_read|write_frequency(). > The API (like the others) should resemble the IIO read|write_raw() > API and even though multiple tone waves is not something unusual, it > would be better to keep it local to the core. The sample_freq is not > that bad as we can eliminate it by having a new op for setting the > sample_frquency. One of the gaps in the IIO core has always been handling multiples of the same attribute of a channel. Sounds like this is one of those. We never really came up with a good solution for it there so I'm not surprised it's causing you pain as well. I'm not certain what tone ID actually is though. Some mode in which the FPGA is generating a fixed tone, or modulating between several rather than streaming from the DMA buffer? Or something entirely different? (I had a google and found an ADI AXI core where it seems this is a DDS feature, so fsk modulation?) I know a little on highspeed ADCs, my DAC knowledge is much more limited so you may need to provide more background or some reference links. > * Code duplication. Any DAC using the AXI_DAC backend will have to define > that extended_info. Can't it stub it out cleanly? > > One idea that I had was to allow to get IIO channels from the backend > but I then quickly started to dislike it because it would open a lot of > complexity. I mean, if we allow backends to define whatever they > want, that might quickly get nasty. > > I guess the above comes from (maybe naive) this idea that we should be > capable to replace a backend and the IIO frontend should still work > without any change to the code. But given how the backends are tightly > coupled to the frontend (at least on ADI usecases) I think that > changing the backend is a very unlikely usecase. And if it happens it > definitely means different HW and userspace ABI so devicetree might make > sense (maybe even a new compatible). It's a dream we should strive for, but until we actually have multiple backend implementations for a given front end it's going to be tricky. + I'd still expect some info transfer along the lines of 'use options a, b, c' to be passed to the frontend. > > So yes, I think it's definitely possible to have something generic where > the backend could completely define a channel (even had some ideas) but > I think the complexity it would bring is just not worth it > (at least right now). Agreed. > > However, another idea that started to grow (that maybe is not that bad) > is that the IIO frontend would still define how many channels, the > channel type, which channel is buffered, etc... but allowing the backends > to extend a certain channel (the backend would be given the channel type > and it could then decide if it can or cannot extend it). We should be > careful with what we allow to extend though... For instance, in this case, > allowing to extend extended_info is likely not that bad because it's > a fairly self contained thing. An example that might make sense is filtering. More than possible the backend would do controllable digital filtering, or indeed the related option of oversampling and averaging. This is definitely plausible. I'd not worry about limiting what the backend can in theory modify that much, in practice I'd expect it to remain sane and if it becomes a problem the front end can take a look at the result and reject things it doesn't like. > > Another thing that we could consider is the info masks. Mentioning this > because (it's not part of the RFC but it should be in the real series) > the AXI_DAC also has CALIBSCALE and CALIBBIAS (I think) that can be > applied to the buffered channel. But in here, it's not that much of code > duplication to set a couple of bits in the mask and then we can just > forward the read/write to the backend... Still, maybe worth considering it > at least.. This feels like something not to do 'yet', but maybe in the future. Have way for the two to negotiate ownership of an interface call (hopefully only one handles it) and pass them on through. > > So, the above two paragraphs are kind of an intermediate approach which > does not look that crazy (or complex to implement). > > Thoughts? > > [1]: https://lore.kernel.org/linux-iio/20230807112113.47157-1-paul@crapouillou.net/ > > --- > Nuno Sa (4): > iio: buffer: add function to set the buffer direction > iio: backend: add new backend ops > iio: dac: add support for the AD97339A RF DAC > iio: dac: adi-axi-dac: add support for AXI DAC IP core > > Paul Cercueil (4): > iio: buffer-dma: Rename iio_dma_buffer_data_available() > iio: buffer-dma: Enable buffer write support > iio: buffer-dmaengine: Support specifying buffer direction > iio: buffer-dmaengine: Enable write support > > drivers/iio/buffer/industrialio-buffer-dma.c | 100 +++- > drivers/iio/buffer/industrialio-buffer-dmaengine.c | 28 +- > drivers/iio/dac/Kconfig | 37 ++ > drivers/iio/dac/Makefile | 2 + > drivers/iio/dac/ad9739a.c | 503 ++++++++++++++++++++ > drivers/iio/dac/adi-axi-dac.c | 510 +++++++++++++++++++++ > drivers/iio/industrialio-backend.c | 65 +++ > drivers/iio/industrialio-buffer.c | 12 + > include/linux/iio/backend.h | 53 ++- > include/linux/iio/buffer-dma.h | 4 +- > include/linux/iio/buffer-dmaengine.h | 5 +- > include/linux/iio/buffer.h | 3 + > 12 files changed, 1292 insertions(+), 30 deletions(-) > --- > base-commit: 7d50ed99f4d40b6fa672be971dda91a8cc8ebae4 > change-id: 20240216-iio-backend-axi-dds-1772fb20604f > -- > > Thanks! > - Nuno Sá >
On Sun, 2024-03-03 at 18:25 +0000, Jonathan Cameron wrote: > On Fri, 16 Feb 2024 15:10:49 +0100 > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > > > Hi all, > > > My first comment here isn't don't worry too much about getting it > perfectly correct from the start. Until you have half a dozen or more > drivers using this stuff you won't have seen enough to > get generalizations right. As this stuff is all in kernel and not > exposed to userspace, that isn't a problem. We can evolve and > improve this as we go along. Userspace ABI design is much worse > as you are stuck with your errors for ever (so never finalize that until > you have a good number of drivers to try it with!) > Agreed. I just felt this is something that I would like some more opinions on as then it is slightly more likely to get it better (not right :)) from the beginning. > > This RFC is mainly because I'm not getting extremely happy with the > > direction of the API in the series. So I think it's better to have the > > discussion now so the actual series will look better. Also note that patch > > 2 to 5 are brought from Paul's series to bring output buffer support to > > DMA buffers [1]. > > > > So, the main API I'm speaking about is: > > - iio_backend_read_phase() > > - iio_backend_write_phase() > > - iio_backend_read_scale() > > - iio_backend_write_scale() > > - iio_backend_read_frequency() > > - iio_backend_write_frequency() > > > > All the above is basically ABI/attributes that belong and are supported > > by the AXI_DAC IP core. The main things I dislike are: > > * The sample_freq and tone_id in iio_backend_read|write_frequency(). > > The API (like the others) should resemble the IIO read|write_raw() > > API and even though multiple tone waves is not something unusual, it > > would be better to keep it local to the core. The sample_freq is not > > that bad as we can eliminate it by having a new op for setting the > > sample_frquency. > > One of the gaps in the IIO core has always been handling multiples of the > same attribute of a channel. Sounds like this is one of those. > We never really came up with a good solution for it there so I'm not > surprised it's causing you pain as well. I did briefly thought on supporting it at the chan_spec level. > > I'm not certain what tone ID actually is though. Some mode in which the > FPGA is generating a fixed tone, or modulating between several rather > than streaming from the DMA buffer? Or something entirely different? > (I had a google and found an ADI AXI core where it seems this is a DDS > feature, so fsk modulation?) Maybe it's poor naming (at least the ID part). This basically enables you to have two frequencies (in theory you can have more but the core only supports two) in your sine wave (two tones if you think FFT). So the ABI I have (a bit inspired on the ltc2688 toggle stuff where we have raw0 and raw1) is: out_altvoltage0_frequency0 out_altvoltage0_frequency1 out_altvoltage0_scale0 out_altvoltage0_scale1 out_altvoltage0_phase0 out_altvoltage0_phase1 > > > I know a little on highspeed ADCs, my DAC knowledge is much more limited > so you may need to provide more background or some reference links. > > > * Code duplication. Any DAC using the AXI_DAC backend will have to define > > that extended_info. > > Can't it stub it out cleanly? Maybe. But I definitely want to avoid to have backend headers in frontend code and would also like to not "pollute" much the generic backend.h header with specific bits from different backends. The last part, I guess we'll have something eventually but the less the better :). But maybe we can something without any of the above... > > > > > One idea that I had was to allow to get IIO channels from the backend > > but I then quickly started to dislike it because it would open a lot of > > complexity. I mean, if we allow backends to define whatever they > > want, that might quickly get nasty. > > > > I guess the above comes from (maybe naive) this idea that we should be > > capable to replace a backend and the IIO frontend should still work > > without any change to the code. But given how the backends are tightly > > coupled to the frontend (at least on ADI usecases) I think that > > changing the backend is a very unlikely usecase. And if it happens it > > definitely means different HW and userspace ABI so devicetree might make > > sense (maybe even a new compatible). > > It's a dream we should strive for, but until we actually have multiple > backend implementations for a given front end it's going to be tricky. > + I'd still expect some info transfer along the lines of 'use options > a, b, c' to be passed to the frontend. > Agreed! > > > > So yes, I think it's definitely possible to have something generic where > > the backend could completely define a channel (even had some ideas) but > > I think the complexity it would bring is just not worth it > > (at least right now). > > Agreed. > > > > > However, another idea that started to grow (that maybe is not that bad) > > is that the IIO frontend would still define how many channels, the > > channel type, which channel is buffered, etc... but allowing the backends > > to extend a certain channel (the backend would be given the channel type > > and it could then decide if it can or cannot extend it). We should be > > careful with what we allow to extend though... For instance, in this case, > > allowing to extend extended_info is likely not that bad because it's > > a fairly self contained thing. > > An example that might make sense is filtering. More than possible the > backend would do controllable digital filtering, or indeed the related > option of oversampling and averaging. We do have a use case for interpolation. And that one will be a funny one to bring upstream since it will effectively turn the axi_dac both a frontend and a backend (the interpolation core sits being the DAC core). But that's a story for another day :) > > This is definitely plausible. I'd not worry about limiting what the backend > can in theory modify that much, in practice I'd expect it to remain sane > and if it becomes a problem the front end can take a look at the result > and reject things it doesn't like. I did had an implementation where the extended_info was gotten from the backend so I may give that a try in the actual series. So we can compare the outcome to this RFC and make a decision. Regarding what the backend can modify, I agree with what you just said and if we do notice some common rejection pattern in the future, we can always move it into the core code. - Nuno Sá
Hi all, This RFC is mainly because I'm not getting extremely happy with the direction of the API in the series. So I think it's better to have the discussion now so the actual series will look better. Also note that patch 2 to 5 are brought from Paul's series to bring output buffer support to DMA buffers [1]. So, the main API I'm speaking about is: - iio_backend_read_phase() - iio_backend_write_phase() - iio_backend_read_scale() - iio_backend_write_scale() - iio_backend_read_frequency() - iio_backend_write_frequency() All the above is basically ABI/attributes that belong and are supported by the AXI_DAC IP core. The main things I dislike are: * The sample_freq and tone_id in iio_backend_read|write_frequency(). The API (like the others) should resemble the IIO read|write_raw() API and even though multiple tone waves is not something unusual, it would be better to keep it local to the core. The sample_freq is not that bad as we can eliminate it by having a new op for setting the sample_frquency. * Code duplication. Any DAC using the AXI_DAC backend will have to define that extended_info. One idea that I had was to allow to get IIO channels from the backend but I then quickly started to dislike it because it would open a lot of complexity. I mean, if we allow backends to define whatever they want, that might quickly get nasty. I guess the above comes from (maybe naive) this idea that we should be capable to replace a backend and the IIO frontend should still work without any change to the code. But given how the backends are tightly coupled to the frontend (at least on ADI usecases) I think that changing the backend is a very unlikely usecase. And if it happens it definitely means different HW and userspace ABI so devicetree might make sense (maybe even a new compatible). So yes, I think it's definitely possible to have something generic where the backend could completely define a channel (even had some ideas) but I think the complexity it would bring is just not worth it (at least right now). However, another idea that started to grow (that maybe is not that bad) is that the IIO frontend would still define how many channels, the channel type, which channel is buffered, etc... but allowing the backends to extend a certain channel (the backend would be given the channel type and it could then decide if it can or cannot extend it). We should be careful with what we allow to extend though... For instance, in this case, allowing to extend extended_info is likely not that bad because it's a fairly self contained thing. Another thing that we could consider is the info masks. Mentioning this because (it's not part of the RFC but it should be in the real series) the AXI_DAC also has CALIBSCALE and CALIBBIAS (I think) that can be applied to the buffered channel. But in here, it's not that much of code duplication to set a couple of bits in the mask and then we can just forward the read/write to the backend... Still, maybe worth considering it at least.. So, the above two paragraphs are kind of an intermediate approach which does not look that crazy (or complex to implement). Thoughts? [1]: https://lore.kernel.org/linux-iio/20230807112113.47157-1-paul@crapouillou.net/ --- Nuno Sa (4): iio: buffer: add function to set the buffer direction iio: backend: add new backend ops iio: dac: add support for the AD97339A RF DAC iio: dac: adi-axi-dac: add support for AXI DAC IP core Paul Cercueil (4): iio: buffer-dma: Rename iio_dma_buffer_data_available() iio: buffer-dma: Enable buffer write support iio: buffer-dmaengine: Support specifying buffer direction iio: buffer-dmaengine: Enable write support drivers/iio/buffer/industrialio-buffer-dma.c | 100 +++- drivers/iio/buffer/industrialio-buffer-dmaengine.c | 28 +- drivers/iio/dac/Kconfig | 37 ++ drivers/iio/dac/Makefile | 2 + drivers/iio/dac/ad9739a.c | 503 ++++++++++++++++++++ drivers/iio/dac/adi-axi-dac.c | 510 +++++++++++++++++++++ drivers/iio/industrialio-backend.c | 65 +++ drivers/iio/industrialio-buffer.c | 12 + include/linux/iio/backend.h | 53 ++- include/linux/iio/buffer-dma.h | 4 +- include/linux/iio/buffer-dmaengine.h | 5 +- include/linux/iio/buffer.h | 3 + 12 files changed, 1292 insertions(+), 30 deletions(-) --- base-commit: 7d50ed99f4d40b6fa672be971dda91a8cc8ebae4 change-id: 20240216-iio-backend-axi-dds-1772fb20604f -- Thanks! - Nuno Sá