| Message ID | 20230804145342.1600136-1-nuno.sa@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add converter framework | expand |
On Fri, 4 Aug 2023 16:53:38 +0200 Nuno Sa <nuno.sa@analog.com> wrote: > This is the initial RFC following the discussion in [1]. I'm aware this is > by no means ready for inclusion and it's not even compilable since in > the RFC I did not included the patch to add component_compare_fwnode() > and component_release_fwnode(). Whilst I haven't read this through yet, I suspect Olivier will be able to offer some insight on some of this and likewise you may be able to point out pitfalls etc in his series (I see you did some review already :) https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/ Both are about multiple interacting components of an overall datapath. Whether there is commonality isn't yet clear to me. > > The goal is to have a first feel on the > direction of the framework so that if I need to drastically change it, > better do it now. The RFC also brings the ad9647 and the axi_adc core to > the same functionality we have now upstream with one extra fundamental > feature that is calibrating the digital interface. This would be very > difficult to do with the current design. Note that I don't expect any > review on those drivers (rather than things related to the framework). > > I also want to bring up a couple of things that I've > been thinking that I'm yet not sure about (so some feedback might make > mind in one direction or another). > > 1) Im yet not sure if I should have different compatibles in the > axi-adc-core driver. Note this soft core is a generic core and for every > design (where the frontend device changes or has subtle changes like > different number of data paths) there are subtle changes. So, the number > of channels might be different, the available test patterns might be > different, some ops might be available for some designs but not for > others, etc... I don't suppose there is any chance Analog can make at least some of this discoverable from the hardware? Capability registers etc in the long run. Can't fix what is already out there. > With a different compatible we could fine tune > those differences (with a chip_info like structure) and pass some const > converter_config to the framework that would allow it to do more safety > checks and potentially reduce the number of converter_ops. > OTOH, starting to add all of these compatibles might become messy in the > long run and will likely mean that we'll always have to change both > drivers in order to support a new frontend. And the frontend devices > should really be the ones having all the "knowledge" to configure the > soft core even if it means more converter_ops (though devicetree might > help as some features are really HW dependent). I more inclined to just > leave things as-is in the RFC. I'm fine with putting this stuff in DT where possible. > > 2) There are some IIO attributes (like scale, frequency, etc) that might > be implemented in the soft cores. I still didn't made my mind if I should just > have a catch all read_raw() and write_raw() converter_ops or more fine > tuned ops. Having the catch all reduces the number of ops but also makes > it more easier to add stuff that ends up being not used anymore and then > forgotten. There are also cases (eg: setting sampling frequency) where > we might need to apply settings in both the frontend and the backend > devices which means having the catch all write_raw() would be more > awkward in these case. I'm a bit more inclined to the more specific ops. It's the kernel - we can always change the internal API later as long as we don't touch the user space part. Go with your gut feeling today and if it changes this sort of refactor usually isn't that bad. > > 3) I also placed this in addac as this is mostly used in high speed DACs > and ADCs but maybe we should just have it in the top level directory > just in case this is started to be used in different type of devices? Easy to change later so right now I don't care where it is. > > 4) Some function and data names are also starting to become very big so > if there are no objections I will move all to use conv instead of full > converter. Or maybe something a bit more generic (converter is a bit specific > I know)? Abrv. fine as long as consistenty used. > > I would love to hear some ideas about the above... > > Anyways, I should also mention that the only visible ABI breakage is in > the IIO device name. Before it was named "adi-axi-adc" and now it's > "ad9647" which is what makes sense actually. With the current approach > we would not be able to actually distinguish between designs. Given that will probably only result in support calls to ADI I'm fine with that breakage. :) > > So my plan for the actual series will be to just add the framework and migrate > the current drivers to it with the same functionality as they have now (not > sure if it will be viable to migrate the drivers in a way each commit is > functional - unless we convert both drivers in one commit). Make sure they build. It's fine to end up with some non functional stubs during such a migration. > After that > point, I will start adding all the missing features (and devices) to the > ad9467 driver. To note that I also plan to include the axi-dac driver in > the first series and that will require IIO DMA output buffer support > so we might need to cherry-pick those patches from Paul's DMABUF series. As mentioned in reply to that, I'm fine with you carrying Paul's miniseries in your patch set to make this all easy to manage. Jonathan > > Thanks! > - Nuno Sá > > [1]: https://lore.kernel.org/linux-iio/dac3967805d7ddbd4653ead6d50e614844e0b70b.camel@gmail.com/ > > Nuno Sa (3): > iio: addac: add new converter framework > iio: adc: ad9647: add based on converter framework > iio: adc: adi-axi-adc: add based on new converter framework > > drivers/iio/adc/ad9467_new.c | 830 ++++++++++++++++++++++++++++ > drivers/iio/adc/adi-axi-adc-new.c | 405 ++++++++++++++ > drivers/iio/addac/converter.c | 547 ++++++++++++++++++ > include/linux/iio/addac/converter.h | 485 ++++++++++++++++ > 4 files changed, 2267 insertions(+) > create mode 100644 drivers/iio/adc/ad9467_new.c > create mode 100644 drivers/iio/adc/adi-axi-adc-new.c > create mode 100644 drivers/iio/addac/converter.c > create mode 100644 include/linux/iio/addac/converter.h >
On Wed, 30 Aug 2023 17:29:03 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Fri, 4 Aug 2023 16:53:38 +0200 > Nuno Sa <nuno.sa@analog.com> wrote: > > > This is the initial RFC following the discussion in [1]. I'm aware this is > > by no means ready for inclusion and it's not even compilable since in > > the RFC I did not included the patch to add component_compare_fwnode() > > and component_release_fwnode(). > > Whilst I haven't read this through yet, I suspect Olivier will be able to > offer some insight on some of this and likewise you may be able to > point out pitfalls etc in his series (I see you did some review already :) > > https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/ > > Both are about multiple interacting components of an overall datapath. > Whether there is commonality isn't yet clear to me. Works better if I actually remember to CC Olivier. +CC Olivier! > > > > > The goal is to have a first feel on the > > direction of the framework so that if I need to drastically change it, > > better do it now. The RFC also brings the ad9647 and the axi_adc core to > > the same functionality we have now upstream with one extra fundamental > > feature that is calibrating the digital interface. This would be very > > difficult to do with the current design. Note that I don't expect any > > review on those drivers (rather than things related to the framework). > > > > I also want to bring up a couple of things that I've > > been thinking that I'm yet not sure about (so some feedback might make > > mind in one direction or another). > > > > 1) Im yet not sure if I should have different compatibles in the > > axi-adc-core driver. Note this soft core is a generic core and for every > > design (where the frontend device changes or has subtle changes like > > different number of data paths) there are subtle changes. So, the number > > of channels might be different, the available test patterns might be > > different, some ops might be available for some designs but not for > > others, etc... > > I don't suppose there is any chance Analog can make at least some of this > discoverable from the hardware? Capability registers etc in the long > run. Can't fix what is already out there. > > > With a different compatible we could fine tune > > those differences (with a chip_info like structure) and pass some const > > converter_config to the framework that would allow it to do more safety > > checks and potentially reduce the number of converter_ops. > > OTOH, starting to add all of these compatibles might become messy in the > > long run and will likely mean that we'll always have to change both > > drivers in order to support a new frontend. And the frontend devices > > should really be the ones having all the "knowledge" to configure the > > soft core even if it means more converter_ops (though devicetree might > > help as some features are really HW dependent). I more inclined to just > > leave things as-is in the RFC. > > I'm fine with putting this stuff in DT where possible. > > > > > 2) There are some IIO attributes (like scale, frequency, etc) that might > > be implemented in the soft cores. I still didn't made my mind if I should just > > have a catch all read_raw() and write_raw() converter_ops or more fine > > tuned ops. Having the catch all reduces the number of ops but also makes > > it more easier to add stuff that ends up being not used anymore and then > > forgotten. There are also cases (eg: setting sampling frequency) where > > we might need to apply settings in both the frontend and the backend > > devices which means having the catch all write_raw() would be more > > awkward in these case. I'm a bit more inclined to the more specific ops. > > It's the kernel - we can always change the internal API later as long as we > don't touch the user space part. Go with your gut feeling today and > if it changes this sort of refactor usually isn't that bad. > > > > > 3) I also placed this in addac as this is mostly used in high speed DACs > > and ADCs but maybe we should just have it in the top level directory > > just in case this is started to be used in different type of devices? > > Easy to change later so right now I don't care where it is. > > > > > 4) Some function and data names are also starting to become very big so > > if there are no objections I will move all to use conv instead of full > > converter. Or maybe something a bit more generic (converter is a bit specific > > I know)? > > Abrv. fine as long as consistenty used. > > > > > I would love to hear some ideas about the above... > > > > Anyways, I should also mention that the only visible ABI breakage is in > > the IIO device name. Before it was named "adi-axi-adc" and now it's > > "ad9647" which is what makes sense actually. With the current approach > > we would not be able to actually distinguish between designs. > > Given that will probably only result in support calls to ADI I'm fine with > that breakage. :) > > > > > So my plan for the actual series will be to just add the framework and migrate > > the current drivers to it with the same functionality as they have now (not > > sure if it will be viable to migrate the drivers in a way each commit is > > functional - unless we convert both drivers in one commit). > Make sure they build. It's fine to end up with some non functional stubs > during such a migration. > > > After that > > point, I will start adding all the missing features (and devices) to the > > ad9467 driver. To note that I also plan to include the axi-dac driver in > > the first series and that will require IIO DMA output buffer support > > so we might need to cherry-pick those patches from Paul's DMABUF series. > As mentioned in reply to that, I'm fine with you carrying Paul's miniseries > in your patch set to make this all easy to manage. > > Jonathan > > > > > Thanks! > > - Nuno Sá > > > > [1]: https://lore.kernel.org/linux-iio/dac3967805d7ddbd4653ead6d50e614844e0b70b.camel@gmail.com/ > > > > Nuno Sa (3): > > iio: addac: add new converter framework > > iio: adc: ad9647: add based on converter framework > > iio: adc: adi-axi-adc: add based on new converter framework > > > > drivers/iio/adc/ad9467_new.c | 830 ++++++++++++++++++++++++++++ > > drivers/iio/adc/adi-axi-adc-new.c | 405 ++++++++++++++ > > drivers/iio/addac/converter.c | 547 ++++++++++++++++++ > > include/linux/iio/addac/converter.h | 485 ++++++++++++++++ > > 4 files changed, 2267 insertions(+) > > create mode 100644 drivers/iio/adc/ad9467_new.c > > create mode 100644 drivers/iio/adc/adi-axi-adc-new.c > > create mode 100644 drivers/iio/addac/converter.c > > create mode 100644 include/linux/iio/addac/converter.h > > >
On Wed, 2023-08-30 at 17:29 +0100, Jonathan Cameron wrote: > On Fri, 4 Aug 2023 16:53:38 +0200 > Nuno Sa <nuno.sa@analog.com> wrote: > > > This is the initial RFC following the discussion in [1]. I'm aware this is > > by no means ready for inclusion and it's not even compilable since in > > the RFC I did not included the patch to add component_compare_fwnode() > > and component_release_fwnode(). > > Whilst I haven't read this through yet, I suspect Olivier will be able to > offer some insight on some of this and likewise you may be able to > point out pitfalls etc in his series (I see you did some review already :) > > https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/ > > Both are about multiple interacting components of an overall datapath. > Whether there is commonality isn't yet clear to me. > I made a very general comment in that series but I need to look better at it. Not sure if we can merge them together but let's see... > > > > The goal is to have a first feel on the > > direction of the framework so that if I need to drastically change it, > > better do it now. The RFC also brings the ad9647 and the axi_adc core to > > the same functionality we have now upstream with one extra fundamental > > feature that is calibrating the digital interface. This would be very > > difficult to do with the current design. Note that I don't expect any > > review on those drivers (rather than things related to the framework). > > > > I also want to bring up a couple of things that I've > > been thinking that I'm yet not sure about (so some feedback might make > > mind in one direction or another). > > > > 1) Im yet not sure if I should have different compatibles in the > > axi-adc-core driver. Note this soft core is a generic core and for every > > design (where the frontend device changes or has subtle changes like > > different number of data paths) there are subtle changes. So, the number > > of channels might be different, the available test patterns might be > > different, some ops might be available for some designs but not for > > others, etc... > > I don't suppose there is any chance Analog can make at least some of this > discoverable from the hardware? Capability registers etc in the long > run. Can't fix what is already out there. > Well, it is a soft core so my naive assumption is that it's doable if some HDL guy is willing to implement it. But yes, it might get supported only for new designs. > > With a different compatible we could fine tune > > those differences (with a chip_info like structure) and pass some const > > converter_config to the framework that would allow it to do more safety > > checks and potentially reduce the number of converter_ops. > > OTOH, starting to add all of these compatibles might become messy in the > > long run and will likely mean that we'll always have to change both > > drivers in order to support a new frontend. And the frontend devices > > should really be the ones having all the "knowledge" to configure the > > soft core even if it means more converter_ops (though devicetree might > > help as some features are really HW dependent). I more inclined to just > > leave things as-is in the RFC. > > I'm fine with putting this stuff in DT where possible. > > > > > 2) There are some IIO attributes (like scale, frequency, etc) that might > > be implemented in the soft cores. I still didn't made my mind if I should just > > have a catch all read_raw() and write_raw() converter_ops or more fine > > tuned ops. Having the catch all reduces the number of ops but also makes > > it more easier to add stuff that ends up being not used anymore and then > > forgotten. There are also cases (eg: setting sampling frequency) where > > we might need to apply settings in both the frontend and the backend > > devices which means having the catch all write_raw() would be more > > awkward in these case. I'm a bit more inclined to the more specific ops. > > It's the kernel - we can always change the internal API later as long as we > don't touch the user space part. Go with your gut feeling today and > if it changes this sort of refactor usually isn't that bad. > Agreed... > - Nuno Sá
On Thu, 31 Aug 2023 10:20:20 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Wed, 2023-08-30 at 17:29 +0100, Jonathan Cameron wrote: > > On Fri, 4 Aug 2023 16:53:38 +0200 > > Nuno Sa <nuno.sa@analog.com> wrote: > > > > > This is the initial RFC following the discussion in [1]. I'm aware this is > > > by no means ready for inclusion and it's not even compilable since in > > > the RFC I did not included the patch to add component_compare_fwnode() > > > and component_release_fwnode(). > > > > Whilst I haven't read this through yet, I suspect Olivier will be able to > > offer some insight on some of this and likewise you may be able to > > point out pitfalls etc in his series (I see you did some review already :) > > > > https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/ > > > > Both are about multiple interacting components of an overall datapath. > > Whether there is commonality isn't yet clear to me. > > > > I made a very general comment in that series but I need to look better at it. Not > sure if we can merge them together but let's see... Great. I wasn't sure either! If nothing else more cross review is always good even if we decide we need two frameworks. My one takeaway from looking at this is I need to understand the component framework better and do some messing around with simple cases before I'll be confident on how this works. Maybe we can get some input from developers of that framework on future versions? Jonathan
On Thu, 2023-08-31 at 10:28 +0100, Jonathan Cameron wrote: > On Thu, 31 Aug 2023 10:20:20 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Wed, 2023-08-30 at 17:29 +0100, Jonathan Cameron wrote: > > > On Fri, 4 Aug 2023 16:53:38 +0200 > > > Nuno Sa <nuno.sa@analog.com> wrote: > > > > > > > This is the initial RFC following the discussion in [1]. I'm aware this is > > > > by no means ready for inclusion and it's not even compilable since in > > > > the RFC I did not included the patch to add component_compare_fwnode() > > > > and component_release_fwnode(). > > > > > > Whilst I haven't read this through yet, I suspect Olivier will be able to > > > offer some insight on some of this and likewise you may be able to > > > point out pitfalls etc in his series (I see you did some review already :) > > > > > > https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/ > > > > > > Both are about multiple interacting components of an overall datapath. > > > Whether there is commonality isn't yet clear to me. > > > > > > > I made a very general comment in that series but I need to look better at it. Not > > sure if we can merge them together but let's see... > > Great. I wasn't sure either! If nothing else more cross review is always > good even if we decide we need two frameworks. > Indeed... > My one takeaway from looking at this is I need to understand the component > framework better and do some messing around with simple cases before I'll Fair enough... It's a fairly straight framework and I like the idea of having the whole thing (multiple devices) coming up/down together since it simplifies some things. But I don't feel to strong about it so in the end, if we decide to go with typical OF/FW lookup, also fine with me. > be confident on how this works. Maybe we can get some input from developers > of that framework on future versions? > Sure, they might have some thoughts on the hacks I'm doing to use devres. But I fear they'll be a bit biased for what they "enforce" in the component framework (my idea was to build on top of it). - Nuno Sá