Re: [PATCH 08/13] iio: buffer: add new hardware triggered buffer driver

[Nuno Sá]

On Wed, 2024-01-10 at 13:49 -0600, David Lechner wrote:
> This adds a new hardware triggered buffer driver for the IIO subsystem.
> This driver is intended to be used by IIO device drivers that have
> a hardware buffer that is triggered by a hardware signal.
> 
> It is expected that components such as those providing a backend via the
> IIO backend framework will provide the actual implementation of this
> functionality by registering a matching device on the auxiliary bus.
> The auxiliary bus was chosen since it allows us to make use of existing
> kernel infrastructure instead of implementing our own registration and
> lookup system.
> 
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---
>  Documentation/driver-api/driver-model/devres.rst   |   1 +
>  drivers/iio/buffer/Kconfig                         |   7 ++
>  drivers/iio/buffer/Makefile                        |   1 +
>  .../iio/buffer/industrialio-hw-triggered-buffer.c  | 104
> +++++++++++++++++++++
>  include/linux/iio/hw_triggered_buffer.h            |  14 +++
>  include/linux/iio/hw_triggered_buffer_impl.h       |  16 ++++
>  6 files changed, 143 insertions(+)
> 
> diff --git a/Documentation/driver-api/driver-model/devres.rst
> b/Documentation/driver-api/driver-model/devres.rst
> index c5f99d834ec5..b23d4a2b68a6 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -296,6 +296,7 @@ IIO
>    devm_iio_channel_get()
>    devm_iio_channel_get_all()
>    devm_iio_hw_consumer_alloc()
> +  devm_iio_hw_triggered_buffer_setup()
>    devm_fwnode_iio_channel_get_by_name()
>  
>  INPUT
> diff --git a/drivers/iio/buffer/Kconfig b/drivers/iio/buffer/Kconfig
> index 047b931591a9..925c5bf074bc 100644
> --- a/drivers/iio/buffer/Kconfig
> +++ b/drivers/iio/buffer/Kconfig
> @@ -53,3 +53,10 @@ config IIO_TRIGGERED_BUFFER
>  	select IIO_KFIFO_BUF
>  	help
>  	  Provides helper functions for setting up triggered buffers.
> +
> +config IIO_HW_TRIGGERED_BUFFER
> +	tristate "Industrial I/O hardware triggered buffer support"
> +	select AUXILIARY_BUS
> +	select IIO_TRIGGER
> +	help
> +	  Provides helper functions for setting up hardware triggered
> buffers.
> diff --git a/drivers/iio/buffer/Makefile b/drivers/iio/buffer/Makefile
> index 1403eb2f9409..d1142bb20f61 100644
> --- a/drivers/iio/buffer/Makefile
> +++ b/drivers/iio/buffer/Makefile
> @@ -9,4 +9,5 @@ obj-$(CONFIG_IIO_BUFFER_DMA) += industrialio-buffer-dmao
>  obj-$(CONFIG_IIO_BUFFER_DMAENGINE) += industrialio-buffer-dmaengine.o
>  obj-$(CONFIG_IIO_BUFFER_HW_CONSUMER) += industrialio-hw-consumer.o
>  obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
> +obj-$(CONFIG_IIO_HW_TRIGGERED_BUFFER) += industrialio-hw-triggered-buffer.o
>  obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
> diff --git a/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> b/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> new file mode 100644
> index 000000000000..7a8a71066b0e
> --- /dev/null
> +++ b/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024 Analog Devices, Inc.
> + * Copyright (c) 2024 BayLibre, SAS
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/container_of.h>
> +#include <linux/export.h>
> +#include <linux/module.h>
> +#include <linux/iio/hw_triggered_buffer_impl.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +
> +static int iio_hw_triggered_buffer_match(struct device *dev, const void
> *match)
> +{
> +	return dev->parent == match;
> +}
> +
> +static struct iio_hw_triggered_buffer_device
> +*iio_hw_trigger_buffer_get(struct device *match)
> +{
> +	struct auxiliary_device *adev;
> +
> +	adev = auxiliary_find_device(NULL, match,
> iio_hw_triggered_buffer_match);
> +	if (!adev)
> +		return ERR_PTR(-ENOENT);
> +
> +	return container_of(adev, struct iio_hw_triggered_buffer_device,
> adev);
> +}
> +
> +static void iio_hw_trigger_buffer_put(void *dev)
> +{
> +	put_device(dev);
> +}
> +
> +/**
> + * devm_iio_hw_triggered_buffer_setup - Setup a hardware triggered buffer
> + * @dev:	Device for devm management
> + * @indio_dev:	An unconfigured/partially configured IIO device struct
> + * @match:	Device for matching the auxiliary bus device that provides
> the
> + *		interface to the hardware triggered buffer
> + * @ops:	Buffer setup functions to use for this IIO device
> + *
> + * Return: 0 on success, negative error code on failure.
> + *
> + * This function will search all registered hardware triggered buffers for
> one
> + * that matches the given indio_dev. If found, it will be used to setup both
> + * the trigger and the buffer on the indio_dev.
> + */
> +int devm_iio_hw_triggered_buffer_setup(struct device *dev,
> +				       struct iio_dev *indio_dev,
> +				       struct device *match,
> +				       const struct iio_buffer_setup_ops
> *ops)
> +{
> +	struct iio_hw_triggered_buffer_device *hw;
> +	int ret;
> +
> +	hw = iio_hw_trigger_buffer_get(match);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	ret = devm_add_action_or_reset(dev, iio_hw_trigger_buffer_put, &hw-
> >adev.dev);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->modes |= INDIO_HW_BUFFER_TRIGGERED;
> +	indio_dev->trig = iio_trigger_get(hw->trig);
> +	indio_dev->setup_ops = ops;
> +
> +	return iio_device_attach_buffer(indio_dev, hw->buffer);
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_hw_triggered_buffer_setup);
> +
> +static int iio_hw_trigger_buffer_probe(struct auxiliary_device *adev,
> +				       const struct auxiliary_device_id *id)
> +{
> +	struct iio_hw_triggered_buffer_device *hw =
> +		container_of(adev, struct iio_hw_triggered_buffer_device,
> adev);
> +
> +	if (!hw->buffer || !hw->trig)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static const struct auxiliary_device_id iio_hw_trigger_buffer_id_table[] = {
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, iio_hw_trigger_buffer_id_table);
> +
> +static struct auxiliary_driver iio_hw_trigger_buffer_driver = {
> +	.driver = {
> +		.name = "iio-hw-triggered-buffer",
> +	},
> +	.probe = iio_hw_trigger_buffer_probe,
> +	.id_table = iio_hw_trigger_buffer_id_table,
> +};
> +module_auxiliary_driver(iio_hw_trigger_buffer_driver);

This is one more example why I think the whole thing is overly complicated. If
I'm understanding things right, we have the spi controller creating platform
devices that will be probed by the new IIO offload driver that then creates an
auxiliary device that is probed by this driver. Even by looking at the probe
function, it already feels to me that something is architecturally wrong (as we
are essentially doing error handling in there).

One idea that just occurred to me now is to perhaps extend the IIO inkernel
interface so that we can split the logic a bit and have IIO devices consuming or
taking ownership of buffers created by other devices. Hmm, maybe not that good
of an idea :(

- Nuno Sá