| Message ID | 20240209-iio-backend-v10-0-3ed842064318@analog.com |
|---|---|
| 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: > > This is a Framework to handle complex IIO aggregate devices. > > The typical architecture is to have one device as the frontend device which > can be "linked" against one or multiple backend devices. All the IIO and > userspace interface is expected to be registers/managed by the frontend > device which will callback into the backends when needed (to get/set > some configuration that it does not directly control). > > The basic framework interface is pretty simple: > - Backends should register themselves with @devm_iio_backend_register() > - Frontend devices should get backends with @devm_iio_backend_get() ... > + fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index); > + if (IS_ERR(fwnode)) { > + dev_err_probe(dev, PTR_ERR(fwnode), > + "Cannot get Firmware reference\n"); > + return ERR_CAST(fwnode); You can combine them in one line (yeah, a bit ugly, I know, we discussed with Andi at some point that it would be nice to have dev_err_probe*() family of helpers for this and other not yet covered cases). Whatever Jonathan likes (as two or a single line), I'm fine with this, just a side note. > + }
On Fri, Feb 9, 2024 at 5:26 PM Nuno Sa <nuno.sa@analog.com> wrote: ... > +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name) > +{ > + struct fwnode_handle *fwnode; > + struct iio_backend *back; > + unsigned int index; > + int ret; > + > + if (name) { > + ret = device_property_match_string(dev, "io-backend-names", > + name); > + if (ret < 0) > + return ERR_PTR(ret); > + index = ret; > + } else { > + index = 0; > + } > + > + fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index); > + if (IS_ERR(fwnode)) { > + dev_err_probe(dev, PTR_ERR(fwnode), > + "Cannot get Firmware reference\n"); > + return ERR_CAST(fwnode); > + } > + > + guard(mutex)(&iio_back_lock); > + list_for_each_entry(back, &iio_back_list, entry) { > + if (!device_match_fwnode(back->dev, fwnode)) > + continue; > + fwnode_handle_put(fwnode); > + ret = __devm_iio_backend_get(dev, back); This order makes me think about the reference counting. So, fwnode is the one of the backend devices to which the property points to. Another piece is the local (to this framework) list that keeps backend devices. So, fwnode reference can be dropped earlier, while the usual pattern to interleave gets and puts in a chain. Dunno if above needs a comment, reordering or nothing. > + if (ret) > + return ERR_PTR(ret); > + > + return back; > + } > + > + fwnode_handle_put(fwnode); > + return ERR_PTR(-EPROBE_DEFER); While thinking about the above, I noticed the room to refactor. list_for_each_entry(...) { if (...) break; } fwnode_handle_put(...); // Yes, we may use the below macro as the (global) pointers are protected by a mutex. if (list_entry_is_head(...)) return ERR_PTR(...); ret = __devm_iio_backend_get(...); ... > +}
On Fri, Feb 9, 2024 at 5:26 PM Nuno Sa <nuno.sa@analog.com> wrote: > > Convert the driver to use the new IIO backend framework. The device > functionality is expected to be the same (meaning no added or removed > features). > > Also note this patch effectively breaks ABI and that's needed so we can > properly support this device and add needed features making use of the > new IIO framework. > > Given the lack of features (and devices supported) in the ad9467 driver > compared with the ADI out of tree version, we don't expect any user of > the upstream driver so no one should notice the ABI breakage. However, > if someone is affected by this, ADI will happily support transitioning > to the backend framework. ... > struct ad9467_chip_info { > - struct adi_axi_adc_chip_info axi_adc_info; > - unsigned int default_output_mode; > - unsigned int vref_mask; > + const char *name; > + unsigned int id; > + const struct iio_chan_spec *channels; > + unsigned int num_channels; > + const unsigned int (*scale_table)[2]; > + int num_scales; > + unsigned long max_rate; > + unsigned int default_output_mode; > + unsigned int vref_mask; > }; Seems like you haven't checked this layout with `pahole`. ... > +static int ad9467_iio_backend_get(struct ad9467_state *st) > +{ > + struct device *dev = &st->spi->dev; > + struct device_node *__back; > + > + st->back = devm_iio_backend_get(&st->spi->dev, NULL); Simply 'dev' as the first parameter? ... > + /* If not found, don't error out as we might have legacy DT property */ This seems related to ENOENT, correct? > + if (!IS_ERR(st->back)) > + return 0; And the above is about something else (found?) case, right? > + if (PTR_ERR(st->back) != -ENOENT) > + return PTR_ERR(st->back); -- With Best Regards, Andy Shevchenko
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, 9 Feb 2024 18:30:53 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Feb 9, 2024 at 5:26 PM Nuno Sa <nuno.sa@analog.com> wrote: > > ... > > > +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name) > > +{ > > + struct fwnode_handle *fwnode; > > + struct iio_backend *back; > > + unsigned int index; > > + int ret; > > + > > + if (name) { > > + ret = device_property_match_string(dev, "io-backend-names", > > + name); > > + if (ret < 0) > > + return ERR_PTR(ret); > > + index = ret; > > + } else { > > + index = 0; > > + } > > + > > + fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index); > > + if (IS_ERR(fwnode)) { > > + dev_err_probe(dev, PTR_ERR(fwnode), > > + "Cannot get Firmware reference\n"); > > + return ERR_CAST(fwnode); > > + } > > + > > + guard(mutex)(&iio_back_lock); > > + list_for_each_entry(back, &iio_back_list, entry) { > > + if (!device_match_fwnode(back->dev, fwnode)) > > + continue; > > > + fwnode_handle_put(fwnode); > > + ret = __devm_iio_backend_get(dev, back); > > This order makes me think about the reference counting. So, fwnode is > the one of the backend devices to which the property points to. > Another piece is the local (to this framework) list that keeps backend > devices. So, fwnode reference can be dropped earlier, while the usual > pattern to interleave gets and puts in a chain. Dunno if above needs a > comment, reordering or nothing. > I'm lost. Why don't we need to hold fwnode reference for the device_match_fwnode() just before here? Or do you mean that we are safe here with the fwnode_handle_put() being before the __devm_iio_backend_get()? I think you are correct that the lifetimes are fine as we switched from the fwnode to the iio_backend from the list at this point. > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return back; > > + } > > + > > + fwnode_handle_put(fwnode); > > + return ERR_PTR(-EPROBE_DEFER); > > While thinking about the above, I noticed the room to refactor. > > list_for_each_entry(...) { > if (...) > break; > } > fwnode_handle_put(...); > // Yes, we may use the below macro as the (global) pointers are > protected by a mutex. > if (list_entry_is_head(...)) Knowing that means we failed to match is a bit obscure. > return ERR_PTR(...); > > ret = __devm_iio_backend_get(...); > ... Maybe - it's a little ugly either way. I don't think we care about potentially holding the fwnode handle too long, so flipping over to the cleanup.h handling (I need to get back to that sometime this week) will make this all simpler. > > > +} >
On Sat, Feb 10, 2024 at 6:42 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 9 Feb 2024 18:30:53 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Feb 9, 2024 at 5:26 PM Nuno Sa <nuno.sa@analog.com> wrote: ... > > > +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name) > > > +{ > > > + struct fwnode_handle *fwnode; > > > + struct iio_backend *back; > > > + unsigned int index; > > > + int ret; > > > + > > > + if (name) { > > > + ret = device_property_match_string(dev, "io-backend-names", > > > + name); > > > + if (ret < 0) > > > + return ERR_PTR(ret); > > > + index = ret; > > > + } else { > > > + index = 0; > > > + } > > > + > > > + fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index); > > > + if (IS_ERR(fwnode)) { > > > + dev_err_probe(dev, PTR_ERR(fwnode), > > > + "Cannot get Firmware reference\n"); > > > + return ERR_CAST(fwnode); > > > + } > > > + > > > + guard(mutex)(&iio_back_lock); > > > + list_for_each_entry(back, &iio_back_list, entry) { > > > + if (!device_match_fwnode(back->dev, fwnode)) > > > + continue; > > > > > + fwnode_handle_put(fwnode); > > > + ret = __devm_iio_backend_get(dev, back); > > > > This order makes me think about the reference counting. So, fwnode is > > the one of the backend devices to which the property points to. > > Another piece is the local (to this framework) list that keeps backend > > devices. So, fwnode reference can be dropped earlier, while the usual > > pattern to interleave gets and puts in a chain. Dunno if above needs a > > comment, reordering or nothing. > > > I'm lost. Why don't we need to hold fwnode reference for the > device_match_fwnode() just before here? > Or do you mean that we are safe here with the fwnode_handle_put() being > before the __devm_iio_backend_get()? This one. > I think you are correct that the > lifetimes are fine as we switched from the fwnode to the > iio_backend from the list at this point. > > > > + if (ret) > > > + return ERR_PTR(ret); > > > + > > > + return back; > > > + } > > > + > > > + fwnode_handle_put(fwnode); > > > + return ERR_PTR(-EPROBE_DEFER); > > > > While thinking about the above, I noticed the room to refactor. > > > > list_for_each_entry(...) { > > if (...) > > break; > > } > > fwnode_handle_put(...); > > // Yes, we may use the below macro as the (global) pointers are > > protected by a mutex. > > if (list_entry_is_head(...)) > > Knowing that means we failed to match is a bit obscure. > > > return ERR_PTR(...); > > > > ret = __devm_iio_backend_get(...); > > ... > > Maybe - it's a little ugly either way. I don't think we care about > potentially holding the fwnode handle too long, so flipping over to > the cleanup.h handling (I need to get back to that sometime this week) > will make this all simpler. Yes, I agree with your point of view. That's why I'm not insisting on this change. > > > +}
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; >
On Fri, 2024-02-09 at 18:37 +0200, Andy Shevchenko wrote: > On Fri, Feb 9, 2024 at 5:26 PM Nuno Sa <nuno.sa@analog.com> wrote: > > > > Convert the driver to use the new IIO backend framework. The device > > functionality is expected to be the same (meaning no added or removed > > features). > > > > Also note this patch effectively breaks ABI and that's needed so we can > > properly support this device and add needed features making use of the > > new IIO framework. > > > > Given the lack of features (and devices supported) in the ad9467 driver > > compared with the ADI out of tree version, we don't expect any user of > > the upstream driver so no one should notice the ABI breakage. However, > > if someone is affected by this, ADI will happily support transitioning > > to the backend framework. > > ... > > > struct ad9467_chip_info { > > - struct adi_axi_adc_chip_info axi_adc_info; > > - unsigned int default_output_mode; > > - unsigned int vref_mask; > > + const char *name; > > + unsigned int id; > > + const struct iio_chan_spec *channels; > > + unsigned int num_channels; > > + const unsigned int (*scale_table)[2]; > > + int num_scales; > > + unsigned long max_rate; > > + unsigned int default_output_mode; > > + unsigned int vref_mask; > > }; > > Seems like you haven't checked this layout with `pahole`. > Not really... IIRC, I just copied the members as-is from the previous struct. Can be done later I guess... > ... > > > +static int ad9467_iio_backend_get(struct ad9467_state *st) > > +{ > > + struct device *dev = &st->spi->dev; > > + struct device_node *__back; > > + > > + st->back = devm_iio_backend_get(&st->spi->dev, NULL); > > Simply 'dev' as the first parameter? > Makes sense... > ... > > > + /* If not found, don't error out as we might have legacy DT property */ > > This seems related to ENOENT, correct? Yeah, the comments are in line with the original version of the code where I was first checking for errors. - Nuno Sá
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á