Re: [PATCH 1/3] iio: kfifo: add iio_device_attach_kfifo_buffer() helper

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 1 Apr 2020 15:59:34 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change adds the iio_device_attach_kfifo_buffer() helper/short-hand,
> which groups the simple routine of allocating a kfifo buffers via
> devm_iio_kfifo_allocate() and calling iio_device_attach_buffer().
> 
> The mode_flags parameter is required. The setup_ops parameter is optional.
> 
> This function will be a bit more useful when needing to define multiple
> buffers per IIO device.
> 
> One requirement [that is more a recommendation] for this helper, is to call
> it after 'indio_dev' has been populated.
> 
> Also, one consequence related to using this helper is that the resource
> management of the buffer will be tied to 'indio_dev->dev'. Previously it
> was open-coded, and each driver does it slightly differently. Most of them
> tied it to the parent device, some of them to 'indio_dev->dev'.
> This shouldn't be a problem, and may be a good idea when adding more
> buffers per-device.

I'm glad you highlighted this subtlety.  I'm not sure it's safe in all cases
because the result is that the managed cleanup for this will occur once we
get to the cleanup for devm_iio_device_alloc and we release the indio_dev->dev

That would put it 'after' any other devm calls that are still hung off the parent
device.

Now the question is whether that ever causes us problems... See next patch.
It potentially does.  I think we need to provide the dev separately even
if it feels a bit silly to do so.  Scope management is complex so I don't
really want to force people to mix and match between different devices
and so get it wrong by accident.

The other issue is that it's not readily apparent from the naming that
this function is registering stuff that is cleaned up automatically or
that it even allocates anything that might need that..

devm_iio_device_attach_new_kfifo_buffer maybe?

I'm sort of wondering if we should do what dma did and have

iiom_device_attach_new_kfifo_buffer to indicate it's managed in the
scope of the iio device?

What do people think?

However, see patch 2 before commenting.  Reality is I'm not sure forcing
managed calls to hang off iio_dev->dev is a good idea (at this stage given
where we are).

Thanks

Jonathan


> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/buffer/kfifo_buf.c | 37 ++++++++++++++++++++++++++++++++++
>  include/linux/iio/kfifo_buf.h  |  4 ++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> index 3150f8ab984b..05b7c5fc6f1d 100644
> --- a/drivers/iio/buffer/kfifo_buf.c
> +++ b/drivers/iio/buffer/kfifo_buf.c
> @@ -228,4 +228,41 @@ void devm_iio_kfifo_free(struct device *dev, struct iio_buffer *r)
>  }
>  EXPORT_SYMBOL(devm_iio_kfifo_free);
>  
> +/**
> + * iio_device_attach_kfifo_buffer - Allocate a kfifo buffer & attach it to an IIO device
> + * @indio_dev: The device the buffer should be attached to
> + * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE and/or
> + *		INDIO_BUFFER_TRIGGERED).
> + * @setup_ops: The setup_ops required to configure the HW part of the buffer (optional)
> + *
> + * This function allocates a kfifo buffer via devm_iio_kfifo_allocate() and
> + * attaches it to the IIO device via iio_device_attach_buffer().
> + * This is meant to be a bit of a short-hand/helper function as many driver
> + * seem to do this.
> + */
> +int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
> +				   int mode_flags,
> +				   const struct iio_buffer_setup_ops *setup_ops)
> +{
> +	struct iio_buffer *buffer;
> +
> +	if (mode_flags)
> +		mode_flags &= kfifo_access_funcs.modes;
> +
> +	if (!mode_flags)
> +		return -EINVAL;
> +
> +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
> +	indio_dev->modes |= mode_flags;
> +	indio_dev->setup_ops = setup_ops;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_device_attach_kfifo_buffer);
> +
>  MODULE_LICENSE("GPL");
> diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
> index 764659e01b68..2363a931be14 100644
> --- a/include/linux/iio/kfifo_buf.h
> +++ b/include/linux/iio/kfifo_buf.h
> @@ -11,4 +11,8 @@ void iio_kfifo_free(struct iio_buffer *r);
>  struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev);
>  void devm_iio_kfifo_free(struct device *dev, struct iio_buffer *r);
>  
> +int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
> +				   int mode_flags,
> +				   const struct iio_buffer_setup_ops *setup_ops);
> +
>  #endif