On Tue, 14 Jul 2020 14:57:36 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> wrote:
> On Fri, 2020-05-22 at 13:46 +0300, Alexandru Ardelean wrote:
> > From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> >
> > All devices using a triggered buffer need to attach and detach the
> > trigger
> > to the device in order to properly work. Instead of doing this in each
> > and
> > every driver by hand move this into the core.
> >
> > At this point in time, all drivers should have been resolved to
> > attach/detach the poll-function in the same order.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > ---
> > .../buffer/industrialio-triggered-buffer.c | 10 +--------
> > drivers/iio/iio_core_trigger.h | 17 ++++++++++++++
> > drivers/iio/industrialio-buffer.c | 13 +++++++++++
> > drivers/iio/industrialio-trigger.c | 22 ++++---------------
> > include/linux/iio/trigger_consumer.h | 7 ------
> > 5 files changed, 35 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c
> > b/drivers/iio/buffer/industrialio-triggered-buffer.c
> > index e8046c1ecd6b..6c20a83f887e 100644
> > --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
> > +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
> > @@ -13,11 +13,6 @@
> > #include <linux/iio/triggered_buffer.h>
> > #include <linux/iio/trigger_consumer.h>
> >
> > -static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops
> > = {
> > - .postenable = &iio_triggered_buffer_postenable,
> > - .predisable = &iio_triggered_buffer_predisable,
> > -};
> > -
> > /**
> > * iio_triggered_buffer_setup() - Setup triggered buffer and pollfunc
> > * @indio_dev: IIO device structure
> > @@ -67,10 +62,7 @@ int iio_triggered_buffer_setup(struct iio_dev
> > *indio_dev,
> > }
> >
> > /* Ring buffer functions - here trigger setup related */
> > - if (setup_ops)
> > - indio_dev->setup_ops = setup_ops;
> > - else
> > - indio_dev->setup_ops = &iio_triggered_buffer_setup_ops;
> > + indio_dev->setup_ops = setup_ops;
> >
> > /* Flag that polled ring buffering is possible */
> > indio_dev->modes |= INDIO_BUFFER_TRIGGERED;
> > diff --git a/drivers/iio/iio_core_trigger.h
> > b/drivers/iio/iio_core_trigger.h
> > index e59fe2f36bbb..9d1a92cc6480 100644
> > --- a/drivers/iio/iio_core_trigger.h
> > +++ b/drivers/iio/iio_core_trigger.h
> > @@ -18,6 +18,12 @@ void iio_device_register_trigger_consumer(struct
> > iio_dev *indio_dev);
> > **/
> > void iio_device_unregister_trigger_consumer(struct iio_dev *indio_dev);
> >
> > +
> > +int iio_trigger_attach_poll_func(struct iio_trigger *trig,
> > + struct iio_poll_func *pf);
> > +int iio_trigger_detach_poll_func(struct iio_trigger *trig,
> > + struct iio_poll_func *pf);
> > +
> > #else
> >
> > /**
> > @@ -37,4 +43,15 @@ static void
> > iio_device_unregister_trigger_consumer(struct iio_dev *indio_dev)
> > {
> > }
> >
> > +static inline int iio_trigger_attach_poll_func(struct iio_trigger *trig,
> > + struct iio_poll_func *pf)
> > +{
> > + return 0;
> > +}
> > +static inline int iio_trigger_detach_poll_func(struct iio_trigger *trig,
> > + struct iio_poll_func *pf)
> > +{
> > + return 0;
> > +}
> > +
> > #endif /* CONFIG_TRIGGER_CONSUMER */
> > diff --git a/drivers/iio/industrialio-buffer.c
> > b/drivers/iio/industrialio-buffer.c
> > index ec4f531994fa..88d756107fb2 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -20,6 +20,7 @@
> >
> > #include <linux/iio/iio.h>
> > #include "iio_core.h"
> > +#include "iio_core_trigger.h"
> > #include <linux/iio/sysfs.h>
> > #include <linux/iio/buffer.h>
> > #include <linux/iio/buffer_impl.h>
> > @@ -972,6 +973,13 @@ static int iio_enable_buffers(struct iio_dev
> > *indio_dev,
> > }
> > }
> >
> > + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > + ret = iio_trigger_attach_poll_func(indio_dev->trig,
> > + indio_dev->pollfunc);
> > + if (ret)
> > + goto err_disable_buffers;
> > + }
> > +
>
> I'm wondering what happened here, or whether I was on some other planet, or
> some other universe where this was correct, but this part looks wrong.
> This should be called before the "indio_dev->setup_ops->postenable" call
>
> And similarly iio_trigger_detach_poll_func() should be called after the
> "indio_dev->setup_ops->predisable" call.
>
> This looks clearly like my fault.
> And it looks like I sent it like this from the start....
>
> At this point, what's the way to fix this?
> Re-send or send a fix?
Gah. I missed it as well. At this stage, a fix definitely. It will all
be in the same pull request, but I'd really rather not rebase the tree
again.
I guess this happened because of that debate a long time back on which
order made sense. We concluded the one that you morphed all the drivers
into, but I guess we forgot the final patch.
I'll hold that pull request a bit longer.
Jonathan
>
> I noticed this while sync-ing the ADI tree with upstream and comparing and
> spent a bit looking at this.
>
>
> > return 0;
> >
> > err_disable_buffers:
> > @@ -998,6 +1006,11 @@ static int iio_disable_buffers(struct iio_dev
> > *indio_dev)
> > if (list_empty(&indio_dev->buffer_list))
> > return 0;
> >
> > + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > + iio_trigger_detach_poll_func(indio_dev->trig,
> > + indio_dev->pollfunc);
> > + }
> > +
> > /*
> > * If things go wrong at some step in disable we still need to
> > continue
> > * to perform the other steps, otherwise we leave the device in a
> > diff --git a/drivers/iio/industrialio-trigger.c
> > b/drivers/iio/industrialio-trigger.c
> > index 53d1931f6be8..6f16357fd732 100644
> > --- a/drivers/iio/industrialio-trigger.c
> > +++ b/drivers/iio/industrialio-trigger.c
> > @@ -239,8 +239,8 @@ static void iio_trigger_put_irq(struct iio_trigger
> > *trig, int irq)
> > * the relevant function is in there may be the best option.
> > */
> > /* Worth protecting against double additions? */
> > -static int iio_trigger_attach_poll_func(struct iio_trigger *trig,
> > - struct iio_poll_func *pf)
> > +int iio_trigger_attach_poll_func(struct iio_trigger *trig,
> > + struct iio_poll_func *pf)
> > {
> > int ret = 0;
> > bool notinuse
> > @@ -290,8 +290,8 @@ static int iio_trigger_attach_poll_func(struct
> > iio_trigger *trig,
> > return ret;
> > }
> >
> > -static int iio_trigger_detach_poll_func(struct iio_trigger *trig,
> > - struct iio_poll_func *pf)
> > +int iio_trigger_detach_poll_func(struct iio_trigger *trig,
> > + struct iio_poll_func *pf)
> > {
> > int ret = 0;
> > bool no_other_users
> > @@ -705,17 +705,3 @@ void iio_device_unregister_trigger_consumer(struct
> > iio_dev *indio_dev)
> > if (indio_dev->trig)
> > iio_trigger_put(indio_dev->trig);
> > }
> > -
> > -int iio_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > -{
> > - return iio_trigger_attach_poll_func(indio_dev->trig,
> > - indio_dev->pollfunc);
> > -}
> > -EXPORT_SYMBOL(iio_triggered_buffer_postenable);
> > -
> > -int iio_triggered_buffer_predisable(struct iio_dev *indio_dev)
> > -{
> > - return iio_trigger_detach_poll_func(indio_dev->trig,
> > - indio_dev->pollfunc);
> > -}
> > -EXPORT_SYMBOL(iio_triggered_buffer_predisable);
> > diff --git a/include/linux/iio/trigger_consumer.h
> > b/include/linux/iio/trigger_consumer.h
> > index c3c6ba5ec423..3aa2f132dd67 100644
> > --- a/include/linux/iio/trigger_consumer.h
> > +++ b/include/linux/iio/trigger_consumer.h
> > @@ -50,11 +50,4 @@ irqreturn_t iio_pollfunc_store_time(int irq, void *p);
> >
> > void iio_trigger_notify_done(struct iio_trigger *trig);
> >
> > -/*
> > - * Two functions for common case where all that happens is a pollfunc
> > - * is attached and detached from a trigger
> > - */
> > -int iio_triggered_buffer_postenable(struct iio_dev *indio_dev);
> > -int iio_triggered_buffer_predisable(struct iio_dev *indio_dev);
> > -
> > #endif
