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

["Ardelean, Alexandru" <alexandru.Ardelean@analog.com>]

On Sun, 2020-04-05 at 11:46 +0100, Jonathan Cameron wrote:
> [External]
> 
> 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).

What I am really after with this patch is to hide away these:
     iio_kfifo_free(indio_dev->buffer);
     iio_buffer_set_attrs(indio_dev->buffer, xxxx_fifo_attributes); 
i.e. not have 'indio_dev->buffer' open-coded in drivers, and hide it in IIO core
somewhere.
Some ideas can go in parallel [like this one] to add support for multiple
buffers.

So, I will think of a better [less sloppy] V2 for this.

One intermediate alternative is to do 'iio_device_kfifo_free(indio_dev)', but
I'll still try to think of a better devm_ approach.
devm_iio_device_attach_new_kfifo_buffer() sounds a bit long but may work.
iiom_device_attach_new_kfifo_buffer() can also work.

What if we just default attaching to the parent device?

Would it work to also attach the parent device in devm_iio_device_alloc() by
default?
Or change 'iio_device_alloc()' to take a parent device as argument?
Which for devm_iio_device_alloc(dev,...) would implicitly mean that 'dev' is
'parent'?

These are just some thoughts.


> 
> 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