Re: [PATCH RFC 3/9] iio: consumer: Add new APIs of triggered_buffer_setup() family

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 19 May 2025 23:25:55 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> Add new versions of the `iio_triggered_buffer_setup_ext()` APIs.
> (API names are tentative)
> 	iio_triggered_buffer_setup_new
> 	iio_triggered_buffer_setup_ext_new
> 	devm_iio_triggered_buffer_setup_new
> 	devm_iio_triggered_buffer_setup_ext_new
> 	iio_alloc_pollfunc_new
> these APIs take a bool parameter named `timestamp_enabled`.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>

See if you can find a way (probably a common shared function) to make it
more obvious that these are very nearly the same as the existing code.

Right now it is harder than I'd like to spot the differences.


> ---
>  drivers/iio/buffer/industrialio-triggered-buffer.c | 82 ++++++++++++++++++++++
>  drivers/iio/industrialio-trigger.c                 | 33 +++++++++
>  include/linux/iio/trigger_consumer.h               |  7 ++
>  include/linux/iio/triggered_buffer.h               | 25 +++++++
>  4 files changed, 147 insertions(+)
> 
> diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
> index 9bf75dee7ff8..9b99bf884ccb 100644
> --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
> +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
> @@ -14,6 +14,68 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
>  
> +int iio_triggered_buffer_setup_ext_new(struct iio_dev *indio_dev,
> +					irqreturn_t (*thread)(int irq, void *p),
> +					bool timestamp_enabled,
> +					enum iio_buffer_direction direction,
> +					const struct iio_buffer_setup_ops *setup_ops,
> +					const struct iio_dev_attr **buffer_attrs)
> +{
> +	struct iio_buffer *buffer;
> +	int ret;
> +
> +	/*
> +	 * iio_triggered_buffer_cleanup() assumes that the buffer allocated here
> +	 * is assigned to indio_dev->buffer but this is only the case if this
> +	 * function is the first caller to iio_device_attach_buffer(). If
> +	 * indio_dev->buffer is already set then we can't proceed otherwise the
> +	 * cleanup function will try to free a buffer that was not allocated here.
> +	 */
> +	if (indio_dev->buffer)
> +		return -EADDRINUSE;
> +
> +	buffer = iio_kfifo_allocate();
> +	if (!buffer) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +
> +	indio_dev->pollfunc = iio_alloc_pollfunc_new(thread,
> +							timestamp_enabled,
> +							IRQF_ONESHOT,
> +							indio_dev,
> +							"%s_consumer%d",
> +							indio_dev->name,
> +							iio_device_id(indio_dev));
> +	if (indio_dev->pollfunc == NULL) {
> +		ret = -ENOMEM;
> +		goto error_kfifo_free;
> +	}
> +
> +	/* Ring buffer functions - here trigger setup related */
> +	indio_dev->setup_ops = setup_ops;
> +
> +	/* Flag that polled ring buffering is possible */
> +	indio_dev->modes |= INDIO_BUFFER_TRIGGERED;
> +
> +	buffer->direction = direction;
> +	buffer->attrs = buffer_attrs;
> +
> +	ret = iio_device_attach_buffer(indio_dev, buffer);
> +	if (ret < 0)
> +		goto error_dealloc_pollfunc;
> +
> +	return 0;
> +
> +error_dealloc_pollfunc:
> +	iio_dealloc_pollfunc(indio_dev->pollfunc);
> +error_kfifo_free:
> +	iio_kfifo_free(buffer);
> +error_ret:
> +	return ret;
> +}
> +EXPORT_SYMBOL(iio_triggered_buffer_setup_ext_new);
> +
>  /**
>   * iio_triggered_buffer_setup_ext() - Setup triggered buffer and pollfunc
>   * @indio_dev:		IIO device structure
> @@ -114,6 +176,26 @@ static void devm_iio_triggered_buffer_clean(void *indio_dev)
>  	iio_triggered_buffer_cleanup(indio_dev);
>  }
>  
> +int devm_iio_triggered_buffer_setup_ext_new(struct device *dev,
> +						struct iio_dev *indio_dev,
> +						irqreturn_t (*thread)(int irq, void *p),
> +						bool timestamp_enabled,
> +						enum iio_buffer_direction direction,
> +						const struct iio_buffer_setup_ops *ops,
> +						const struct iio_dev_attr **buffer_attrs)
> +{
> +	int ret;
> +
> +	ret = iio_triggered_buffer_setup_ext_new(indio_dev, thread, timestamp_enabled, direction,
> +						     ops, buffer_attrs);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, devm_iio_triggered_buffer_clean,
> +					indio_dev);
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_triggered_buffer_setup_ext_new);
> +
>  int devm_iio_triggered_buffer_setup_ext(struct device *dev,
>  					struct iio_dev *indio_dev,
>  					irqreturn_t (*h)(int irq, void *p),
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 54416a384232..527c3cf84be0 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -361,6 +361,39 @@ irqreturn_t iio_pollfunc_store_time(int irq, void *p)
>  }
>  EXPORT_SYMBOL(iio_pollfunc_store_time);
>  
> +struct iio_poll_func
> +*iio_alloc_pollfunc_new(irqreturn_t (*thread)(int irq, void *p),
> +			bool timestamp_enabled,
> +			int type,
> +			struct iio_dev *indio_dev,
> +			const char *fmt,
> +			...)
> +{
> +	va_list vargs;
> +	struct iio_poll_func *pf;
> +
> +	pf = kmalloc(sizeof(*pf), GFP_KERNEL);
> +	if (!pf)
> +		return NULL;
> +	va_start(vargs, fmt);
> +	pf->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> +	va_end(vargs);
> +	if (pf->name == NULL) {
> +		kfree(pf);
> +		return NULL;
> +	}
> +	pf->timestamp_enabled = timestamp_enabled;
> +	pf->h = NULL;
> +	pf->thread = thread;
> +	pf->type = type;
> +	pf->indio_dev = indio_dev;
> +
> +	pf->timestamp = 0;
> +	pf->timestamp_type = 0;
> +	return pf;
> +}
> +EXPORT_SYMBOL_GPL(iio_alloc_pollfunc_new);
> +
>  struct iio_poll_func
>  *iio_alloc_pollfunc(irqreturn_t (*h)(int irq, void *p),
>  		    irqreturn_t (*thread)(int irq, void *p),
> diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
> index 5e6ff8738386..213cd8560518 100644
> --- a/include/linux/iio/trigger_consumer.h
> +++ b/include/linux/iio/trigger_consumer.h
> @@ -50,6 +50,13 @@ struct iio_poll_func {
>  	bool timestamp_enabled;
>  };
>  
> +__printf(5, 6) struct iio_poll_func
> +*iio_alloc_pollfunc_new(irqreturn_t (*thread)(int irq, void *p),
> +			bool timestamp_enabled,
> +			int type,
> +			struct iio_dev *indio_dev,
> +			const char *fmt,
> +			...);
>  
>  __printf(5, 6) struct iio_poll_func
>  *iio_alloc_pollfunc(irqreturn_t (*h)(int irq, void *p),
> diff --git a/include/linux/iio/triggered_buffer.h b/include/linux/iio/triggered_buffer.h
> index 29e1fe146879..5648c382a506 100644
> --- a/include/linux/iio/triggered_buffer.h
> +++ b/include/linux/iio/triggered_buffer.h
> @@ -9,6 +9,13 @@ struct iio_dev;
>  struct iio_dev_attr;
>  struct iio_buffer_setup_ops;
>  
> +int iio_triggered_buffer_setup_ext_new(struct iio_dev *indio_dev,
> +	irqreturn_t (*thread)(int irq, void *p),
> +	bool timestamp_enabled,
> +	enum iio_buffer_direction direction,
> +	const struct iio_buffer_setup_ops *setup_ops,
> +	const struct iio_dev_attr **buffer_attrs);
> +
>  int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
>  	irqreturn_t (*h)(int irq, void *p),
>  	irqreturn_t (*thread)(int irq, void *p),
> @@ -17,11 +24,24 @@ int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
>  	const struct iio_dev_attr **buffer_attrs);
>  void iio_triggered_buffer_cleanup(struct iio_dev *indio_dev);
>  
> +#define iio_triggered_buffer_setup_new(indio_dev, h, timestamp_enabled, setup_ops)	\
> +	iio_triggered_buffer_setup_ext_new((indio_dev), (h), (timestamp_enabled),	\
> +					IIO_BUFFER_DIRECTION_IN, (setup_ops),		\
> +					NULL)
> +
>  #define iio_triggered_buffer_setup(indio_dev, h, thread, setup_ops)		\
>  	iio_triggered_buffer_setup_ext((indio_dev), (h), (thread),		\
>  					IIO_BUFFER_DIRECTION_IN, (setup_ops),	\
>  					NULL)
>  
> +int devm_iio_triggered_buffer_setup_ext_new(struct device *dev,
> +					struct iio_dev *indio_dev,
> +					irqreturn_t (*thread)(int irq, void *p),
> +					bool timestamp_enabled,
> +					enum iio_buffer_direction direction,
> +					const struct iio_buffer_setup_ops *ops,
> +					const struct iio_dev_attr **buffer_attrs);
> +
>  int devm_iio_triggered_buffer_setup_ext(struct device *dev,
>  					struct iio_dev *indio_dev,
>  					irqreturn_t (*h)(int irq, void *p),
> @@ -30,6 +50,11 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
>  					const struct iio_buffer_setup_ops *ops,
>  					const struct iio_dev_attr **buffer_attrs);
>  
> +#define devm_iio_triggered_buffer_setup_new(dev, indio_dev, thread, timestamp_enabled, setup_ops)	\
> +	devm_iio_triggered_buffer_setup_ext_new((dev), (indio_dev), (thread), (timestamp_enabled),	\
> +					    IIO_BUFFER_DIRECTION_IN,					\
> +					    (setup_ops), NULL)
> +
>  #define devm_iio_triggered_buffer_setup(dev, indio_dev, h, thread, setup_ops)	\
>  	devm_iio_triggered_buffer_setup_ext((dev), (indio_dev), (h), (thread),	\
>  					    IIO_BUFFER_DIRECTION_IN,		\
>