| Message ID | 20240209-iio-backend-v10-0-3ed842064318@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: add new backend framework | expand |
On Fri, Feb 9, 2024 at 5:26 PM Nuno Sa <nuno.sa@analog.com> wrote: > > Move to the IIO backend framework. Devices supported by adi-axi-adc now > register themselves as backend devices. ... > -#include <linux/iio/iio.h> > -#include <linux/iio/sysfs.h> > -#include <linux/iio/buffer.h> > -#include <linux/iio/buffer-dmaengine.h> > - > #include <linux/fpga/adi-axi-common.h> > -#include <linux/iio/adc/adi-axi-adc.h> + Blank line? > +#include <linux/iio/backend.h> > +#include <linux/iio/buffer-dmaengine.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/iio.h> ... > +static int axi_adc_enable(struct iio_backend *back) > { > + struct adi_axi_adc_state *st = iio_backend_get_priv(back); > int ret; > > + ret = regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN, > + ADI_AXI_REG_RSTN_MMCM_RSTN); > + if (ret) > + return ret; > + fsleep(10); Would be nice to have a comment that probably the datasheet defines the minimum timeout for reset. Ah and you decreased it 1000x times, why? > + return regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN, > + ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN); > } ... > + expected_ver = (unsigned int *)device_get_match_data(&pdev->dev); expected_ver should have const and you can drop the casting IIUC. > + if (!expected_ver) > + return -ENODEV;
On Fri, Feb 9, 2024 at 5:25 PM Nuno Sa <nuno.sa@analog.com> wrote: > Changes in v10: > - Patch 5 > * Removed meaningless @ in function names; > * Added ascii diagram for the typicaly HW setup (Andy request); > * Add missing period; > * Use of dev_err_probe() in APIs meant to be called during probe(). > - Patch 6 > * Removed unneeded blank line; > * Fixed some English in the commit message. Thanks for the update! I found it nice, only one (kinda important) finding is the 1000x reset timeout decrease. It might justify v11. If so, consider other comments as well.
On Fri, 2024-02-09 at 18:45 +0200, Andy Shevchenko wrote: > On Fri, Feb 9, 2024 at 5:26 PM Nuno Sa <nuno.sa@analog.com> wrote: > > > > Move to the IIO backend framework. Devices supported by adi-axi-adc now > > register themselves as backend devices. > > ... > > > -#include <linux/iio/iio.h> > > -#include <linux/iio/sysfs.h> > > -#include <linux/iio/buffer.h> > > -#include <linux/iio/buffer-dmaengine.h> > > - > > #include <linux/fpga/adi-axi-common.h> > > -#include <linux/iio/adc/adi-axi-adc.h> > > + Blank line? > can be... > > +#include <linux/iio/backend.h> > > +#include <linux/iio/buffer-dmaengine.h> > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/iio.h> > > ... > > > +static int axi_adc_enable(struct iio_backend *back) > > { > > + struct adi_axi_adc_state *st = iio_backend_get_priv(back); > > int ret; > > > > + ret = regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN, > > + ADI_AXI_REG_RSTN_MMCM_RSTN); > > + if (ret) > > + return ret; > > > + fsleep(10); > > Would be nice to have a comment that probably the datasheet defines > the minimum timeout for reset. Ah and you decreased it 1000x times, > why? > Arghh, always forget that fsleep() is us... That said, there's no minimum timeout specified in the docs. I guess the original developer has the sleeps out of habit or some "hidden" knowledge. In my testing I did not noticed anything weird with the 10us. I'll move back to the original timeout though. I can then check and make sure 10us is enough and send a follow up patch. > > + return regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN, > > + ADI_AXI_REG_RSTN_RSTN | > > ADI_AXI_REG_RSTN_MMCM_RSTN); > > } > > ... > > > + expected_ver = (unsigned int *)device_get_match_data(&pdev->dev); > > expected_ver should have const and you can drop the casting IIUC. Right... > > > + if (!expected_ver) > > + return -ENODEV; >
v1: https://lore.kernel.org/linux-iio/20231204144925.4fe9922f@jic23-huawei/T/#m222f5175273b81dbfe40b7f0daffcdc67d6cb8ff v2: https://lore.kernel.org/r/20231208-dev-iio-backend-v2-0-5450951895e1@analog.com v3: https://lore.kernel.org/linux-iio/20231213-dev-iio-backend-v3-0-bb9f12a5c6dc@analog.com/ v4: https://lore.kernel.org/r/20231220-iio-backend-v4-0-998e9148b692@analog.com v5: https://lore.kernel.org/r/20240112-iio-backend-v5-0-bdecad041ab4@analog.com v6: https://lore.kernel.org/r/20240119-iio-backend-v6-0-189536c35a05@analog.com v7 https://lore.kernel.org/r/20240123-iio-backend-v7-0-1bff236b8693@analog.com v8: https://lore.kernel.org/r/20240202-iio-backend-v8-0-f65ee8c8203d@analog.com v9: https://lore.kernel.org/r/20240206-iio-backend-v9-0-df66d159c000@analog.com Changes in v10: - Patch 5 * Removed meaningless @ in function names; * Added ascii diagram for the typicaly HW setup (Andy request); * Add missing period; * Use of dev_err_probe() in APIs meant to be called during probe(). - Patch 6 * Removed unneeded blank line; * Fixed some English in the commit message. Jonathan, the series is based on next-20240207 since it already includes the io-channels fix Rob applied in his tree. I guess it should land in rc3 so after you rebase, all patches should apply cleanly (if applying them of course :)). Let me know if anything fails... Keeping the block diagram so we don't have to follow links to check one of the typical setups. ------------------------------------------------------- ------------------ | ----------- ------------ ------- FPGA | | ADC |------------------------| | AXI ADC |---------| DMA CORE |------| RAM | | | (Frontend/IIO) | Serial Data (eg: LVDS) | |(backend)|---------| |------| | | | |------------------------| ----------- ------------ ------- | ------------------ ------------------------------------------------------- --- Nuno Sa (6): dt-bindings: adc: ad9467: add new io-backend property dt-bindings: adc: axi-adc: update bindings for backend framework iio: buffer-dmaengine: export buffer alloc and free functions iio: add the IIO backend framework iio: adc: ad9467: convert to backend framework iio: adc: adi-axi-adc: move to backend framework Olivier Moysan (1): of: property: add device link support for io-backends .../devicetree/bindings/iio/adc/adi,ad9467.yaml | 4 + .../devicetree/bindings/iio/adc/adi,axi-adc.yaml | 8 +- MAINTAINERS | 8 + drivers/iio/Kconfig | 9 + drivers/iio/Makefile | 1 + drivers/iio/adc/Kconfig | 4 +- drivers/iio/adc/ad9467.c | 267 ++++++++----- drivers/iio/adc/adi-axi-adc.c | 379 +++++-------------- drivers/iio/buffer/industrialio-buffer-dmaengine.c | 8 +- drivers/iio/industrialio-backend.c | 418 +++++++++++++++++++++ drivers/of/property.c | 2 + include/linux/iio/adc/adi-axi-adc.h | 68 ---- include/linux/iio/backend.h | 72 ++++ include/linux/iio/buffer-dmaengine.h | 3 + 14 files changed, 798 insertions(+), 453 deletions(-) --- base-commit: 2ae0a045e6814c8c1d676d6153c605a65746aa29 change-id: 20231219-iio-backend-a3dc1a6a7a58 -- Thanks! - Nuno Sá