Re: [PATCH v6 00/24] iio: core,buffer: add support for multiple IIO buffers per IIO device
On Mon, 15 Feb 2021 16:10:45 +0200 Alexandru Ardelean wrote: > On Mon, Feb 15, 2021 at 3:59 PM Jonathan Cameron wrote: > > > > On Mon, 15 Feb 2021 12:40:19 +0200 > > Alexandru Ardelean wrote: > > > > Hi Alex, > > > > One thought on this that came up whilst reading through it again. > > There are several uses for multiple buffers. > > 1) input vs output buffers > > 2) A device with separately clocked sampling of different channels. > > So, ADI typically has use-cases for 1). > Mostly RF transceivers. > Traditionally, these were written (inside the ADI tree) as 2-3 IIO > devices (1 IIO device for each path,1 for TX and 1-2 for RX). > With multi-buffer, they can be a single IIO device with 2-3 IIO buffers. > > There are some ADI devices that have channels that can run at different > speeds. > But this hasn't been an important use-cases to deal with. > Typically, a common-rate would be picked [for all channels] and that's that. > I think some people find this per-channel rates interesting; but let's see. The nasty ones for multiple speeds tend to be sensor hubs and similar units where there is heavy processing going on. Often they are taking in a bunch of physical sensors and 1) Shoving the data straight out unprocessed whenever it shows up. The raw sensors aren't running in sync, so they drift across each other and other fun things like that. 2) Doing sensor fusion in batches and kicking out an quaternion every now and then. They work on the basis of tagging the samples in a fifo with what they actually are. Often there is very little control of the flow so we've previously ended up with the same multi IIO device solution, with a common core redirecting the flows to the right devices (basically acting as an MFD). Thankfully those cases don't play well with triggers anyway so we mostly don't expose them. >From memory, cros_ec and st_lsm6dsx are like this. I think there are a few >more. > > > > > For case 2, there is at least sometimes a trigger involved > > (often data ready) so we probably also need to think long term about how > > to support multiple current_triggers in one driver. > > > > I guess that's something for us to figure out when we need it however. > > > > Thanks for you hard work on finally getting this upstream. > > + Lars (and maybe others) for earlier work on this. > > Thank you as well. > > > > > Whilst I've applied all these, they are only exposed in the testing branch > > of iio.git on kernel.org so far and as such any additional review anyone > > else wants to give would be welcome! > > I feel there may be other users stumbling over this multibuffer > support, and try it out. > Often people don't seem to care about multibuffer, until they really need it. > A bit of chicken-n-egg problem. > But after being added, people may see it and start to use it. Agreed. I'm fairly confident that the ABI is fine, so I'm not particularly worried if we need to make further adjustments as other users show up. Jonathan > > > > > Jonathan > > > > > Changelog v5 -> v6: > > > * > > > https://lore.kernel.org/linux-iio/20210211122452.78106-1-alexandru.ardel...@analog.com/T/#t > > > * merged series 'iio: kfifo: define a devm_iio_kfifo_buffer_setup helper' > > > into this > > > > > > https://lore.kernel.org/linux-iio/20210214143313.67202-4-alexandru.ardel...@analog.com/T/#u > > > * merged patch 'iio: buffer-dma,adi-axi-adc: introduce > > > devm_iio_dmaengine_buffer_setup()' into this > > > > > > https://lore.kernel.org/linux-iio/20210214155817.68678-1-alexandru.ardel...@analog.com/T/#u > > > * patch 'iio: core: wrap iio device & buffer into struct for character > > > devices' > > >- moved kfree(ib) to be first in iio_chrdev_release() to make the > > > cleanup > > > order be reversed as the open() order > > > * patch 'iio: buffer: group attr count and attr alloc' > > >- sizeof(struct attribute *) -> sizeof(*attr) in kcalloc() > > > * patch 'iio: core: merge buffer/ & scan_elements/ attributes' > > >- minor rework in iio_buffer_register_legacy_sysfs_groups() to > > > use more 'sizeof(*attrs)' in allocations > > >- fixed typo 'legacy_buffer_el_group' -> 'legacy_buffer_group' > > >- updated comment about > > > '/* we only need to link the legacy buffer groups for the first > > > buffer */' > > > to > > > '/* we only need to register the legacy groups for the first buffer > > > */' > > > * patch 'iio: buffer: add ioctl() to support opening extra buffers for > > > IIO device' > > >- removed 'buf_name' from iio_device_buffer_getfd(); > > > passing "iio:buffer" name; since it should be a class-name > > >- removed extra-line in drivers/iio/industrial-core.c > > >- changed init order in iio_device_buffer_getfd() and matched it with > > > reverse in > > > iio_buffer_chrdev_release() > > >- calling put_unused_fd() in iio_device_buffer_getfd() if > > > copy_to_user() fails > > > * added
Re: [PATCH v6 00/24] iio: core,buffer: add support for multiple IIO buffers per IIO device
On Mon, Feb 15, 2021 at 3:59 PM Jonathan Cameron wrote: > > On Mon, 15 Feb 2021 12:40:19 +0200 > Alexandru Ardelean wrote: > > Hi Alex, > > One thought on this that came up whilst reading through it again. > There are several uses for multiple buffers. > 1) input vs output buffers > 2) A device with separately clocked sampling of different channels. So, ADI typically has use-cases for 1). Mostly RF transceivers. Traditionally, these were written (inside the ADI tree) as 2-3 IIO devices (1 IIO device for each path,1 for TX and 1-2 for RX). With multi-buffer, they can be a single IIO device with 2-3 IIO buffers. There are some ADI devices that have channels that can run at different speeds. But this hasn't been an important use-cases to deal with. Typically, a common-rate would be picked [for all channels] and that's that. I think some people find this per-channel rates interesting; but let's see. > > For case 2, there is at least sometimes a trigger involved > (often data ready) so we probably also need to think long term about how > to support multiple current_triggers in one driver. > > I guess that's something for us to figure out when we need it however. > > Thanks for you hard work on finally getting this upstream. > + Lars (and maybe others) for earlier work on this. Thank you as well. > > Whilst I've applied all these, they are only exposed in the testing branch > of iio.git on kernel.org so far and as such any additional review anyone > else wants to give would be welcome! I feel there may be other users stumbling over this multibuffer support, and try it out. Often people don't seem to care about multibuffer, until they really need it. A bit of chicken-n-egg problem. But after being added, people may see it and start to use it. > > Jonathan > > > Changelog v5 -> v6: > > * > > https://lore.kernel.org/linux-iio/20210211122452.78106-1-alexandru.ardel...@analog.com/T/#t > > * merged series 'iio: kfifo: define a devm_iio_kfifo_buffer_setup helper' > > into this > > > > https://lore.kernel.org/linux-iio/20210214143313.67202-4-alexandru.ardel...@analog.com/T/#u > > * merged patch 'iio: buffer-dma,adi-axi-adc: introduce > > devm_iio_dmaengine_buffer_setup()' into this > > > > https://lore.kernel.org/linux-iio/20210214155817.68678-1-alexandru.ardel...@analog.com/T/#u > > * patch 'iio: core: wrap iio device & buffer into struct for character > > devices' > >- moved kfree(ib) to be first in iio_chrdev_release() to make the cleanup > > order be reversed as the open() order > > * patch 'iio: buffer: group attr count and attr alloc' > >- sizeof(struct attribute *) -> sizeof(*attr) in kcalloc() > > * patch 'iio: core: merge buffer/ & scan_elements/ attributes' > >- minor rework in iio_buffer_register_legacy_sysfs_groups() to > > use more 'sizeof(*attrs)' in allocations > >- fixed typo 'legacy_buffer_el_group' -> 'legacy_buffer_group' > >- updated comment about > > '/* we only need to link the legacy buffer groups for the first buffer > > */' > > to > > '/* we only need to register the legacy groups for the first buffer */' > > * patch 'iio: buffer: add ioctl() to support opening extra buffers for IIO > > device' > >- removed 'buf_name' from iio_device_buffer_getfd(); > > passing "iio:buffer" name; since it should be a class-name > >- removed extra-line in drivers/iio/industrial-core.c > >- changed init order in iio_device_buffer_getfd() and matched it with > > reverse in > > iio_buffer_chrdev_release() > >- calling put_unused_fd() in iio_device_buffer_getfd() if copy_to_user() > > fails > > * added 'iio: dummy: iio_simple_dummy_buffer: use triggered buffer core > > calls' > > * patch ' iio: buffer: introduce support for attaching more IIO buffers' > >- now handling iio_device_attach_buffer() error returns > >- rename iio_buffer_free_sysfs_and_mask() -> > > iio_buffers_free_sysfs_and_mask() > >- rename iio_buffer_alloc_sysfs_and_mask() -> > > iio_buffer_alloc_sysfs_and_mask() > > * patch 'tools: iio: convert iio_generic_buffer to use new IIO buffer API' > > - removed extra close() if getfd() ioctl() errors out > > - added comment that the sanity check for buffer0 should not be done > > under normal operation > > - update message from > >"This device does not support buffers" > >to > >"Device does not have this many buffers" > > > > Changelog v4 -> v5: > > * > > https://lore.kernel.org/linux-iio/20210210100823.46780-1-alexandru.ardel...@analog.com/T/#t > > * patch 'iio: buffer: add ioctl() to support opening extra buffers for IIO > > device' > > don't return -EBUSY in iio_buffer_poll_wrapper(); return 0 > > __poll_t is unsigned, so returning 0 is the best we can do > > Reported-by: kernel test robot > > * patch 'iio: buffer: dmaengine: obtain buffer object from attribute' > > removed unused 'indio_dev' variable; seems i missed this initially > > * patch 'iio:
Re: [PATCH v6 00/24] iio: core,buffer: add support for multiple IIO buffers per IIO device
On Mon, 15 Feb 2021 12:40:19 +0200 Alexandru Ardelean wrote: Hi Alex, One thought on this that came up whilst reading through it again. There are several uses for multiple buffers. 1) input vs output buffers 2) A device with separately clocked sampling of different channels. For case 2, there is at least sometimes a trigger involved (often data ready) so we probably also need to think long term about how to support multiple current_triggers in one driver. I guess that's something for us to figure out when we need it however. Thanks for you hard work on finally getting this upstream. + Lars (and maybe others) for earlier work on this. Whilst I've applied all these, they are only exposed in the testing branch of iio.git on kernel.org so far and as such any additional review anyone else wants to give would be welcome! Jonathan > Changelog v5 -> v6: > * > https://lore.kernel.org/linux-iio/20210211122452.78106-1-alexandru.ardel...@analog.com/T/#t > * merged series 'iio: kfifo: define a devm_iio_kfifo_buffer_setup helper' > into this > > https://lore.kernel.org/linux-iio/20210214143313.67202-4-alexandru.ardel...@analog.com/T/#u > * merged patch 'iio: buffer-dma,adi-axi-adc: introduce > devm_iio_dmaengine_buffer_setup()' into this > > https://lore.kernel.org/linux-iio/20210214155817.68678-1-alexandru.ardel...@analog.com/T/#u > * patch 'iio: core: wrap iio device & buffer into struct for character > devices' >- moved kfree(ib) to be first in iio_chrdev_release() to make the cleanup > order be reversed as the open() order > * patch 'iio: buffer: group attr count and attr alloc' >- sizeof(struct attribute *) -> sizeof(*attr) in kcalloc() > * patch 'iio: core: merge buffer/ & scan_elements/ attributes' >- minor rework in iio_buffer_register_legacy_sysfs_groups() to > use more 'sizeof(*attrs)' in allocations >- fixed typo 'legacy_buffer_el_group' -> 'legacy_buffer_group' >- updated comment about > '/* we only need to link the legacy buffer groups for the first buffer > */' > to > '/* we only need to register the legacy groups for the first buffer */' > * patch 'iio: buffer: add ioctl() to support opening extra buffers for IIO > device' >- removed 'buf_name' from iio_device_buffer_getfd(); > passing "iio:buffer" name; since it should be a class-name >- removed extra-line in drivers/iio/industrial-core.c >- changed init order in iio_device_buffer_getfd() and matched it with > reverse in > iio_buffer_chrdev_release() >- calling put_unused_fd() in iio_device_buffer_getfd() if copy_to_user() > fails > * added 'iio: dummy: iio_simple_dummy_buffer: use triggered buffer core calls' > * patch ' iio: buffer: introduce support for attaching more IIO buffers' >- now handling iio_device_attach_buffer() error returns >- rename iio_buffer_free_sysfs_and_mask() -> > iio_buffers_free_sysfs_and_mask() >- rename iio_buffer_alloc_sysfs_and_mask() -> > iio_buffer_alloc_sysfs_and_mask() > * patch 'tools: iio: convert iio_generic_buffer to use new IIO buffer API' > - removed extra close() if getfd() ioctl() errors out > - added comment that the sanity check for buffer0 should not be done > under normal operation > - update message from >"This device does not support buffers" >to >"Device does not have this many buffers" > > Changelog v4 -> v5: > * > https://lore.kernel.org/linux-iio/20210210100823.46780-1-alexandru.ardel...@analog.com/T/#t > * patch 'iio: buffer: add ioctl() to support opening extra buffers for IIO > device' > don't return -EBUSY in iio_buffer_poll_wrapper(); return 0 > __poll_t is unsigned, so returning 0 is the best we can do > Reported-by: kernel test robot > * patch 'iio: buffer: dmaengine: obtain buffer object from attribute' > removed unused 'indio_dev' variable; seems i missed this initially > * patch 'iio: buffer: add ioctl() to support opening extra buffers for IIO > device' > call 'wake_up(buffer->pollq)' in iio_buffer_chrdev_release() > > Changelog v3 -> v4: > * > https://lore.kernel.org/linux-iio/20210201145105.20459-1-alexandru.ardel...@analog.com/ > * patch 'docs: ioctl-number.rst: reserve IIO subsystem ioctl() space' >remove 'uapi/' from `uapi/linux/iio/*.h` > * patch 'iio: core: register chardev only if needed' >add commit comment about potentially breaking userspace ABI with chardev > removal > * patch 'iio: core: rework iio device group creation' >remove NULL re-init in iio_device_unregister_sysfs() ; memory is being > free'd > * patch 'iio: buffer: group attr count and attr alloc' > extend commit comment about the 2 or 1 buffer directores > * patch 'iio: core: merge buffer/ & scan_elements/ attributes' >fixed static checker complaints > - removed unused global > - initialize omitted 'ret = -ENOMEM' on error path > - made iio_buffer_unregister_legacy_sysfs_groups() static > * patch 'iio: buffer: wrap all buffer
[PATCH v6 00/24] iio: core,buffer: add support for multiple IIO buffers per IIO device
Changelog v5 -> v6: * https://lore.kernel.org/linux-iio/20210211122452.78106-1-alexandru.ardel...@analog.com/T/#t * merged series 'iio: kfifo: define a devm_iio_kfifo_buffer_setup helper' into this https://lore.kernel.org/linux-iio/20210214143313.67202-4-alexandru.ardel...@analog.com/T/#u * merged patch 'iio: buffer-dma,adi-axi-adc: introduce devm_iio_dmaengine_buffer_setup()' into this https://lore.kernel.org/linux-iio/20210214155817.68678-1-alexandru.ardel...@analog.com/T/#u * patch 'iio: core: wrap iio device & buffer into struct for character devices' - moved kfree(ib) to be first in iio_chrdev_release() to make the cleanup order be reversed as the open() order * patch 'iio: buffer: group attr count and attr alloc' - sizeof(struct attribute *) -> sizeof(*attr) in kcalloc() * patch 'iio: core: merge buffer/ & scan_elements/ attributes' - minor rework in iio_buffer_register_legacy_sysfs_groups() to use more 'sizeof(*attrs)' in allocations - fixed typo 'legacy_buffer_el_group' -> 'legacy_buffer_group' - updated comment about '/* we only need to link the legacy buffer groups for the first buffer */' to '/* we only need to register the legacy groups for the first buffer */' * patch 'iio: buffer: add ioctl() to support opening extra buffers for IIO device' - removed 'buf_name' from iio_device_buffer_getfd(); passing "iio:buffer" name; since it should be a class-name - removed extra-line in drivers/iio/industrial-core.c - changed init order in iio_device_buffer_getfd() and matched it with reverse in iio_buffer_chrdev_release() - calling put_unused_fd() in iio_device_buffer_getfd() if copy_to_user() fails * added 'iio: dummy: iio_simple_dummy_buffer: use triggered buffer core calls' * patch ' iio: buffer: introduce support for attaching more IIO buffers' - now handling iio_device_attach_buffer() error returns - rename iio_buffer_free_sysfs_and_mask() -> iio_buffers_free_sysfs_and_mask() - rename iio_buffer_alloc_sysfs_and_mask() -> iio_buffer_alloc_sysfs_and_mask() * patch 'tools: iio: convert iio_generic_buffer to use new IIO buffer API' - removed extra close() if getfd() ioctl() errors out - added comment that the sanity check for buffer0 should not be done under normal operation - update message from "This device does not support buffers" to "Device does not have this many buffers" Changelog v4 -> v5: * https://lore.kernel.org/linux-iio/20210210100823.46780-1-alexandru.ardel...@analog.com/T/#t * patch 'iio: buffer: add ioctl() to support opening extra buffers for IIO device' don't return -EBUSY in iio_buffer_poll_wrapper(); return 0 __poll_t is unsigned, so returning 0 is the best we can do Reported-by: kernel test robot * patch 'iio: buffer: dmaengine: obtain buffer object from attribute' removed unused 'indio_dev' variable; seems i missed this initially * patch 'iio: buffer: add ioctl() to support opening extra buffers for IIO device' call 'wake_up(buffer->pollq)' in iio_buffer_chrdev_release() Changelog v3 -> v4: * https://lore.kernel.org/linux-iio/20210201145105.20459-1-alexandru.ardel...@analog.com/ * patch 'docs: ioctl-number.rst: reserve IIO subsystem ioctl() space' remove 'uapi/' from `uapi/linux/iio/*.h` * patch 'iio: core: register chardev only if needed' add commit comment about potentially breaking userspace ABI with chardev removal * patch 'iio: core: rework iio device group creation' remove NULL re-init in iio_device_unregister_sysfs() ; memory is being free'd * patch 'iio: buffer: group attr count and attr alloc' extend commit comment about the 2 or 1 buffer directores * patch 'iio: core: merge buffer/ & scan_elements/ attributes' fixed static checker complaints - removed unused global - initialize omitted 'ret = -ENOMEM' on error path - made iio_buffer_unregister_legacy_sysfs_groups() static * patch 'iio: buffer: wrap all buffer attributes into iio_dev_attr' - update some omitted unwindings; seems i forgot a few originally this was showing up when trying to read from buffer1 * add patch 'iio: buffer: move __iio_buffer_free_sysfs_and_mask() before alloc func' * patch 'iio: buffer: introduce support for attaching more IIO buffers' - removed 'iio_dev_opaque->attached_buffers = NULL' after kfree() - using 'iio_dev_opaque->attached_buffers_cnt' to check that we have buffers instead of checking 'indio_dev->buffer' * patch 'iio: buffer: add ioctl() to support opening extra buffers for IIO device' - replaced -ENOENT with -ENODEV when buffer index is out of range * add 'iio: core: rename 'dev' -> 'indio_dev' in iio_device_alloc()' * add 'iio: buffer: dmaengine: obtain buffer object from attribute' * add tools/iio patches for new multibuffer logic tools: iio: make iioutils_get_type() private in iio_utils tools: iio: privatize globals and functions in iio_generic_buffer.c file tools: iio: convert
