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