On Mon, 6 Apr 2020 08:12:42 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx> 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?
That would work and be consistent with the vast majority of current cases.
>
> Would it work to also attach the parent device in devm_iio_device_alloc() by
> default?
That would need a thorough audit to check nothing crazy is done by
a driver with an odd structure. Such a driver would (I think) be
buggy though as the child lifetime should be dependent on the parent
and not some other device.
> Or change 'iio_device_alloc()' to take a parent device as argument?
I think there are only a couple of users, so that would work.
> Which for devm_iio_device_alloc(dev,...) would implicitly mean that 'dev' is
> 'parent'?
I think that's a fair assumption (though needs a sanity check)
>
> These are just some thoughts.
>
>
> >
> > Thanks
> >
> > Jonathan
> >
> >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > > ---
> > > 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
