Re: [PATCH][V2] iio: Move attach/detach of the poll func to the core

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 6 May 2019 08:45:20 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2019-05-05 at 18:37 +0100, Jonathan Cameron wrote:
> > 
> > 
> > On Wed, 19 Dec 2018 16:09:12 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >   
> > > 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.
> > > 
> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > 
> > Hi Alex,
> > 
> > Firstly apologies that this is always at the back of my queue.
> > 
> > Anyhow, my one question is around where the new call to
> > iio_detach_pollfunc
> > is.  To reverse the ordering of the setup path it should be before the
> > individual buffer_disables, but you have it after.
> > 
> > I'll review the rest on the assumption we'll resolve that.
> > 
> > So the only difference intended here is to drop the attach out of
> > post_enable and have it just before it, and the detach out of predisable
> > and have it just after that.
> > 
> > There are a few drivers in here where the fundamental flow changes.
> > To my mind, the rules are:
> > 
> > 1) Anything used in the pollfunc should be setup before it is attached.
> > 2) Anything related to setup fo the flow that leads to the pollfunc being
> >    called isn't so critical as long as the final switch to enable that
> >    is after.
> > 
> > To do this, I think we need to first propose a series of patches
> > changing the ordering in those drivers so that the final patch just
> > becomes a change with no functional effects.  Any modifications
> > to the flow should occur in earlier patches.
> > 
> > So based on a not particularly exhaustive look we need to
> > 'fix' up 16 drivers before we look at the big cleanup.  
> 
> I'll take a look and see about these drivers.
> It will take me some time. I'll have to balance other stuff and come back
> to this.
Absolutely.  This is a nice bit of cleanup work but no need to rush.

Thanks

Jonathan

> 
> > 
> > I would really like to chip away at this over time even if you decide
> > you don't want to take it forwards because the current flows around
> > this are a mess (my fault as I've never been super clear to anyone
> > in review on how it 'should' work).
> > 
> > It would be nice to clear that up once and for all.
> > 
> > One thought is that we might want to change the update_scan_mode
> > function to take a flag that says 'no specific scan mode' as some
> > devices aren't using it because they want to enable 'all' channels
> > on leaving buffered mode.  If we do that we can probably call it
> > in the disable path as well and that will resolve a few drivers.
> > 
> > Thanks for your hard work on this.
> > 
> > *cross fingers you still want to take it forwards!*
> > 
> > Jonathan
> > 
> >   
> > > ---
> > > 
> > > Changelog v1 -> v2:
> > > * changed order of execution for attach poll func: first attach
> > > poll_func,
> > >   then call call post_enable() hook
> > > * updated drivers with change since first patch sent ; particularly
> > >   stm32-adc.c has a rather big update with V2
> > > 
> > > 
> > >  drivers/iio/accel/adxl372.c                   |  6 +-
> > >  drivers/iio/accel/bmc150-accel-core.c         |  4 +-
> > >  drivers/iio/accel/kxcjk-1013.c                |  2 -
> > >  drivers/iio/accel/kxsd9.c                     |  2 -
> > >  drivers/iio/accel/st_accel_buffer.c           |  8 ---
> > >  drivers/iio/accel/stk8312.c                   |  2 -
> > >  drivers/iio/accel/stk8ba50.c                  |  2 -
> > >  drivers/iio/adc/ad7266.c                      |  2 -
> > >  drivers/iio/adc/ad7766.c                      |  2 -
> > >  drivers/iio/adc/ad7887.c                      |  2 -
> > >  drivers/iio/adc/ad_sigma_delta.c              |  5 --
> > >  drivers/iio/adc/at91-sama5d2_adc.c            |  7 +--
> > >  drivers/iio/adc/dln2-adc.c                    |  4 +-
> > >  drivers/iio/adc/mxs-lradc-adc.c               |  2 -
> > >  drivers/iio/adc/stm32-adc.c                   | 36 ++----------
> > >  drivers/iio/adc/ti-adc084s021.c               |  2 -
> > >  drivers/iio/adc/ti-ads1015.c                  |  2 -
> > >  drivers/iio/adc/vf610_adc.c                   |  6 +-
> > >  drivers/iio/adc/xilinx-xadc-core.c            |  2 -
> > >  .../buffer/industrialio-triggered-buffer.c    | 10 +---
> > >  drivers/iio/chemical/atlas-ph-sensor.c        |  8 ---
> > >  drivers/iio/dummy/iio_simple_dummy_buffer.c   | 14 -----
> > >  drivers/iio/gyro/bmg160_core.c                |  2 -
> > >  drivers/iio/gyro/mpu3050-core.c               |  2 -
> > >  drivers/iio/gyro/st_gyro_buffer.c             |  8 ---
> > >  drivers/iio/humidity/hdc100x.c                |  7 +--
> > >  drivers/iio/humidity/hts221_buffer.c          |  2 -
> > >  drivers/iio/iio_core_trigger.h                | 20 ++++++-
> > >  drivers/iio/industrialio-buffer.c             | 16 ++++--
> > >  drivers/iio/industrialio-trigger.c            | 55 +++++++++----------
> > >  drivers/iio/light/gp2ap020a00f.c              | 11 +---
> > >  drivers/iio/light/isl29125.c                  |  5 --
> > >  drivers/iio/light/rpr0521.c                   |  2 -
> > >  drivers/iio/light/si1145.c                    |  2 -
> > >  drivers/iio/light/st_uvis25_core.c            |  2 -
> > >  drivers/iio/light/tcs3414.c                   |  5 --
> > >  drivers/iio/magnetometer/bmc150_magn.c        |  2 -
> > >  drivers/iio/magnetometer/rm3100-core.c        |  2 -
> > >  drivers/iio/magnetometer/st_magn_buffer.c     | 21 +------
> > >  drivers/iio/potentiostat/lmp91000.c           |  1 -
> > >  drivers/iio/pressure/st_pressure_buffer.c     | 23 +-------
> > >  drivers/iio/pressure/zpa2326.c                |  6 --
> > >  drivers/iio/proximity/sx9500.c                |  3 -
> > >  include/linux/iio/trigger_consumer.h          |  7 ---
> > >  44 files changed, 75 insertions(+), 259 deletions(-)
> > > 
> > > diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> > > index 3b84cb243a87..e3efdc0f23df 100644
> > > --- a/drivers/iio/accel/adxl372.c
> > > +++ b/drivers/iio/accel/adxl372.c
> > > @@ -817,7 +817,7 @@ static int adxl372_buffer_postenable(struct iio_dev
> > > *indio_dev)
> > >               return ret;
> > >       }
> > > 
> > > -     return iio_triggered_buffer_postenable(indio_dev);
> > > +     return 0;  
> > 
> > This is a reorder as now we'll call the rest of this function before the
> > attach.
> > Conceptually that means the pollfunc 'might be called'. In reality it
> > shouldn't
> > be as the upstream interrupt isn't enabled yet, but let us keep to the
> > flow.
> > 
> > Question is does that matter.
> > 
> > There is a lot of code in here, that definitely looks likely to be
> > relevant to
> > that so I would say yes it does...
> > 
> > It is 'probably' the case that we'd expect the code in here to be
> > conceptually
> > in preenable, rather than postenable, but as it stands this change looks
> > very
> > dubious an that change isn't trivial as there is a setwatermark callback
> > to deal as well.
> > 
> > Not too hard to fix, but I'd want a test before I'd be sure we hadn't
> > messed it
> > up.
> >   
> > >  }
> > > 
> > >  static int adxl372_buffer_predisable(struct iio_dev *indio_dev)
> > > @@ -825,10 +825,6 @@ static int adxl372_buffer_predisable(struct
> > > iio_dev *indio_dev)
> > >       struct adxl372_state *st = iio_priv(indio_dev);
> > >       int ret;
> > > 
> > > -     ret = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (ret < 0)
> > > -             return ret;
> > > -
> > >       adxl372_set_interrupts(st, 0, 0);
> > >       st->fifo_mode = ADXL372_FIFO_BYPASSED;
> > >       adxl372_configure_fifo(st);  
> > 
> > ...
> >   
> > > diff --git a/drivers/iio/accel/st_accel_buffer.c
> > > b/drivers/iio/accel/st_accel_buffer.c
> > > index 7fddc137e91e..cfa189774a1a 100644
> > > --- a/drivers/iio/accel/st_accel_buffer.c
> > > +++ b/drivers/iio/accel/st_accel_buffer.c
> > > @@ -51,10 +51,6 @@ static int st_accel_buffer_postenable(struct iio_dev
> > > *indio_dev)  
> > 
> > This driver is a bit miss-balanced already...
> >   
> > >       if (err < 0)
> > >               goto st_accel_buffer_postenable_error;
> > > 
> > > -     err = iio_triggered_buffer_postenable(indio_dev);
> > > -     if (err < 0)
> > > -             goto st_accel_buffer_postenable_error;
> > > -  
> > 
> > And another one where the postenable contains a lot of setup that we
> > would now
> > not have here.  This needs unwinding.  I would guess that having a static
> > buffer of sufficient size would solve most of the issue.
> > Interesting question of whether the axis enable should be before or after
> > the pollfunc is attached.
> > 
> >   
> > >       return err;
> > > 
> > >  st_accel_buffer_postenable_error:
> > > @@ -68,10 +64,6 @@ static int st_accel_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > >       int err;
> > >       struct st_sensor_data *adata = iio_priv(indio_dev);
> > > 
> > > -     err = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (err < 0)
> > > -             goto st_accel_buffer_predisable_error;
> > > -
> > >       err = st_sensors_set_axis_enable(indio_dev,
> > > ST_SENSORS_ENABLE_ALL_AXIS);
> > >       if (err < 0)
> > >               goto st_accel_buffer_predisable_error;  
> > 
> > ...  
> > > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > > b/drivers/iio/adc/ad_sigma_delta.c
> > > index ff5f2da2e1b1..815344387541 100644
> > > --- a/drivers/iio/adc/ad_sigma_delta.c
> > > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > > @@ -345,10 +345,6 @@ static int ad_sd_buffer_postenable(struct iio_dev
> > > *indio_dev)
> > >       unsigned int channel;
> > >       int ret;
> > > 
> > > -     ret = iio_triggered_buffer_postenable(indio_dev);
> > > -     if (ret < 0)
> > > -             return ret;
> > > -  
> > 
> > I think this one is fine as all the following is around getting the
> > device
> > into a state where the pollfunc 'will be called' rather than a state
> > where it is safe to call it. The boundary is a bit blurred though..
> >   
> > >       channel = find_first_bit(indio_dev->active_scan_mask,
> > >                                indio_dev->masklength);
> > >       ret = ad_sigma_delta_set_channel(sigma_delta,
> > > @@ -439,7 +435,6 @@ static irqreturn_t ad_sd_trigger_handler(int irq,
> > > void *p)
> > > 
> > >  static const struct iio_buffer_setup_ops ad_sd_buffer_setup_ops = {
> > >       .postenable = &ad_sd_buffer_postenable,
> > > -     .predisable = &iio_triggered_buffer_predisable,
> > >       .postdisable = &ad_sd_buffer_postdisable,
> > >       .validate_scan_mask = &iio_validate_scan_mask_onehot,
> > >  };
> > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-
> > > sama5d2_adc.c
> > > index d5ea84cf6460..90121aed2210 100644
> > > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > > @@ -904,7 +904,7 @@ static int at91_adc_buffer_postenable(struct
> > > iio_dev *indio_dev)
> > >               return ret;
> > >       }
> > > 
> > > -     return iio_triggered_buffer_postenable(indio_dev);  
> > 
> > Another one with major changes in resulting order.. I'm not totally sure
> > on
> > this one.
> > It looks like stuff that 'causes data' rather than is involved in
> > handling so
> > probably fine.  
> > > +     return 0;
> > >  }
> > > 
> > >  static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> > > @@ -924,11 +924,6 @@ static int at91_adc_buffer_predisable(struct
> > > iio_dev *indio_dev)
> > >       if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> > >               return -EINVAL;
> > > 
> > > -     /* continue with the triggered buffer */
> > > -     ret = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (ret < 0)
> > > -             dev_err(&indio_dev->dev, "buffer predisable failed\n");
> > > -
> > >       if (!st->dma_st.dma_chan)
> > >               return ret;
> > > 
> > > diff --git a/drivers/iio/adc/dln2-adc.c b/drivers/iio/adc/dln2-adc.c
> > > index c64c6675cae6..51135e7c0d4f 100644
> > > --- a/drivers/iio/adc/dln2-adc.c
> > > +++ b/drivers/iio/adc/dln2-adc.c
> > > @@ -560,7 +560,7 @@ static int
> > > dln2_adc_triggered_buffer_postenable(struct iio_dev *indio_dev)  
> > 
> > Flow has changed here as well..  
> > >               mutex_unlock(&dln2->mutex);
> > >       }
> > > 
> > > -     return iio_triggered_buffer_postenable(indio_dev);
> > > +     return 0;
> > >  }
> > > 
> > >  static int dln2_adc_triggered_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > > @@ -585,7 +585,7 @@ static int
> > > dln2_adc_triggered_buffer_predisable(struct iio_dev *indio_dev)
> > >               return ret;
> > >       }
> > > 
> > > -     return iio_triggered_buffer_predisable(indio_dev);
> > > +     return 0;
> > >  }
> > > 
> > >  static const struct iio_buffer_setup_ops dln2_adc_buffer_setup_ops = {  
> > 
> > ...
> > 
> >   
> > > diff --git a/drivers/iio/chemical/atlas-ph-sensor.c
> > > b/drivers/iio/chemical/atlas-ph-sensor.c
> > > index a406ad31b096..8fed75f9e95d 100644
> > > --- a/drivers/iio/chemical/atlas-ph-sensor.c
> > > +++ b/drivers/iio/chemical/atlas-ph-sensor.c
> > > @@ -305,10 +305,6 @@ static int atlas_buffer_postenable(struct iio_dev
> > > *indio_dev)
> > >       struct atlas_data *data = iio_priv(indio_dev);
> > >       int ret;
> > > 
> > > -     ret = iio_triggered_buffer_postenable(indio_dev);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > >       ret = pm_runtime_get_sync(&data->client->dev);
> > >       if (ret < 0) {
> > >               pm_runtime_put_noidle(&data->client->dev);
> > > @@ -323,10 +319,6 @@ static int atlas_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > >       struct atlas_data *data = iio_priv(indio_dev);
> > >       int ret;
> > >   
> > 
> > Hmm. This one is doing mix and match. I'll guess it's safe to fix that
> > up.
> >   
> > > -     ret = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > >       ret = atlas_set_interrupt(data, false);
> > >       if (ret)
> > >               return ret;
> > > diff --git a/drivers/iio/gyro/st_gyro_buffer.c
> > > b/drivers/iio/gyro/st_gyro_buffer.c
> > > index a5377044e42f..21501ffd879e 100644
> > > --- a/drivers/iio/gyro/st_gyro_buffer.c
> > > +++ b/drivers/iio/gyro/st_gyro_buffer.c
> > > @@ -51,10 +51,6 @@ static int st_gyro_buffer_postenable(struct iio_dev
> > > *indio_dev)
> > >       if (err < 0)
> > >               goto st_gyro_buffer_postenable_error;
> > >   
> > 
> > Same as the accel version.
> >   
> > > -     err = iio_triggered_buffer_postenable(indio_dev);
> > > -     if (err < 0)
> > > -             goto st_gyro_buffer_postenable_error;
> > > -
> > >       return err;
> > > 
> > >  st_gyro_buffer_postenable_error:
> > > @@ -68,10 +64,6 @@ static int st_gyro_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > >       int err;
> > >       struct st_sensor_data *gdata = iio_priv(indio_dev);
> > > 
> > > -     err = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (err < 0)
> > > -             goto st_gyro_buffer_predisable_error;
> > > -
> > >       err = st_sensors_set_axis_enable(indio_dev,
> > > ST_SENSORS_ENABLE_ALL_AXIS);
> > >       if (err < 0)
> > >               goto st_gyro_buffer_predisable_error;
> > > diff --git a/drivers/iio/humidity/hdc100x.c
> > > b/drivers/iio/humidity/hdc100x.c
> > > index 066e05f92081..959d7e17d471 100644
> > > --- a/drivers/iio/humidity/hdc100x.c
> > > +++ b/drivers/iio/humidity/hdc100x.c
> > > @@ -286,7 +286,7 @@ static int hdc100x_buffer_postenable(struct iio_dev
> > > *indio_dev)
> > >       if (ret)
> > >               return ret;  
> > 
> > I think the earlier part of this should be in preenable, but haven't
> > checked
> > properly.  Definitely can't just change it like this given there
> > are comments about ordering requirements ;)
> >   
> > > 
> > > -     return iio_triggered_buffer_postenable(indio_dev);
> > > +     return 0;
> > >  }
> > > 
> > >  static int hdc100x_buffer_predisable(struct iio_dev *indio_dev)
> > > @@ -294,11 +294,6 @@ static int hdc100x_buffer_predisable(struct
> > > iio_dev *indio_dev)
> > >       struct hdc100x_data *data = iio_priv(indio_dev);
> > >       int ret;
> > > 
> > > -     /* First detach poll func, then reset ACQ mode. OK to disable
> > > buffer */
> > > -     ret = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > >       mutex_lock(&data->lock);
> > >       ret = hdc100x_update_config(data, HDC100X_REG_CONFIG_ACQ_MODE,
> > > 0);
> > >       mutex_unlock(&data->lock);
> > > diff --git a/drivers/iio/industrialio-buffer.c
> > > b/drivers/iio/industrialio-buffer.c
> > > index cd5bfe39591b..d09a9223a5b0 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -24,8 +24,10 @@
> > > 
> > >  #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/trigger.h>
> > >  #include <linux/iio/buffer_impl.h>
> > > 
> > >  static const char * const iio_endian_prefix[] = {
> > > @@ -915,6 +917,7 @@ static int iio_enable_buffers(struct iio_dev
> > > *indio_dev,
> > >       indio_dev->active_scan_mask = config->scan_mask;
> > >       indio_dev->scan_timestamp = config->scan_timestamp;
> > >       indio_dev->scan_bytes = config->scan_bytes;
> > > +     indio_dev->currentmode = config->mode;
> > > 
> > >       iio_update_demux(indio_dev);
> > > 
> > > @@ -950,28 +953,32 @@ static int iio_enable_buffers(struct iio_dev
> > > *indio_dev,
> > >                       goto err_disable_buffers;
> > >       }
> > > 
> > > -     indio_dev->currentmode = config->mode;
> > > +     ret = iio_trigger_attach_poll_func(indio_dev);
> > > +     if (ret)
> > > +             goto err_disable_buffers;
> > > 
> > >       if (indio_dev->setup_ops->postenable) {
> > >               ret = indio_dev->setup_ops->postenable(indio_dev);
> > >               if (ret) {
> > >                       dev_dbg(&indio_dev->dev,
> > >                              "Buffer not started: postenable failed
> > > (%d)\n", ret);
> > > -                     goto err_disable_buffers;
> > > +                     goto err_detach_pollfunc;
> > >               }
> > >       }
> > > 
> > >       return 0;
> > > 
> > > +err_detach_pollfunc:
> > > +     iio_trigger_detach_poll_func(indio_dev);
> > >  err_disable_buffers:
> > >       list_for_each_entry_continue_reverse(buffer, &indio_dev-  
> > > >buffer_list,  
> > >                                            buffer_list)
> > >               iio_buffer_disable(buffer, indio_dev);
> > >  err_run_postdisable:
> > > -     indio_dev->currentmode = INDIO_DIRECT_MODE;
> > >       if (indio_dev->setup_ops->postdisable)
> > >               indio_dev->setup_ops->postdisable(indio_dev);
> > >  err_undo_config:
> > > +     indio_dev->currentmode = INDIO_DIRECT_MODE;
> > >       indio_dev->active_scan_mask = NULL;
> > > 
> > >       return ret;
> > > @@ -1006,7 +1013,7 @@ static int iio_disable_buffers(struct iio_dev
> > > *indio_dev)
> > >                       ret = ret2;
> > >       }
> > > 
> > > -     indio_dev->currentmode = INDIO_DIRECT_MODE;
> > > +     iio_trigger_detach_poll_func(indio_dev);  
> > 
> > This doesn't seem right. In the setup path
> > iio_trigger_detach_poll_func is called just before post_enable, so I'd
> > expect to
> > see this just after pre_disable for it to be balanced.
> > 
> > This has crossed the individual buffer disables.
> >   
> > > 
> > >       if (indio_dev->setup_ops->postdisable) {
> > >               ret2 = indio_dev->setup_ops->postdisable(indio_dev);
> > > @@ -1016,6 +1023,7 @@ static int iio_disable_buffers(struct iio_dev
> > > *indio_dev)
> > > 
> > >       iio_free_scan_mask(indio_dev, indio_dev->active_scan_mask);
> > >       indio_dev->active_scan_mask = NULL;
> > > +     indio_dev->currentmode = INDIO_DIRECT_MODE;
> > > 
> > >       return ret;
> > >  }
> > > diff --git a/drivers/iio/industrialio-trigger.c
> > > b/drivers/iio/industrialio-trigger.c
> > > index ce66699c7fcc..d50761717dbe 100644
> > > --- a/drivers/iio/industrialio-trigger.c
> > > +++ b/drivers/iio/industrialio-trigger.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/slab.h>
> > > 
> > >  #include <linux/iio/iio.h>
> > > +#include <linux/iio/buffer.h>
> > >  #include <linux/iio/trigger.h>
> > >  #include "iio_core.h"
> > >  #include "iio_core_trigger.h"
> > > @@ -242,12 +243,17 @@ 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_dev *indio_dev)
> > >  {
> > > +     struct iio_trigger *trig = indio_dev->trig;
> > > +     struct iio_poll_func *pf = indio_dev->pollfunc;
> > > +     bool notinuse;
> > >       int ret = 0;
> > > -     bool notinuse
> > > -             = bitmap_empty(trig->pool,
> > > CONFIG_IIO_CONSUMERS_PER_TRIGGER);
> > > +
> > > +     if (indio_dev->currentmode != INDIO_BUFFER_TRIGGERED)
> > > +             return 0;
> > > +
> > > +     notinuse = bitmap_empty(trig->pool,
> > > CONFIG_IIO_CONSUMERS_PER_TRIGGER);
> > > 
> > >       /* Prevent the module from being removed whilst attached to a
> > > trigger */
> > >       __module_get(pf->indio_dev->driver_module);
> > > @@ -290,14 +296,19 @@ 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_dev *indio_dev)
> > >  {
> > > +     struct iio_trigger *trig = indio_dev->trig;
> > > +     struct iio_poll_func *pf = indio_dev->pollfunc;
> > > +     bool no_other_users = false;
> > >       int ret = 0;
> > > -     bool no_other_users
> > > -             = (bitmap_weight(trig->pool,
> > > -                              CONFIG_IIO_CONSUMERS_PER_TRIGGER)
> > > -                == 1);
> > > +
> > > +     if (indio_dev->currentmode != INDIO_BUFFER_TRIGGERED)
> > > +             return 0;
> > > +
> > > +     if (bitmap_weight(trig->pool, CONFIG_IIO_CONSUMERS_PER_TRIGGER)
> > > == 1)
> > > +             no_other_users = true;
> > > +
> > >       if (trig->ops && trig->ops->set_trigger_state && no_other_users)
> > > {
> > >               ret = trig->ops->set_trigger_state(trig, false);
> > >               if (ret)
> > > @@ -434,18 +445,16 @@ static ssize_t iio_trigger_write_current(struct
> > > device *dev,
> > >                       goto out_trigger_put;
> > >       }
> > > 
> > > -     indio_dev->trig = trig;
> > > 
> > > -     if (oldtrig) {
> > > +     if (indio_dev->trig) {
> > >               if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
> > > -                     iio_trigger_detach_poll_func(oldtrig,
> > > -                                                  indio_dev-  
> > > >pollfunc_event);  
> > > +                     iio_trigger_detach_poll_func(indio_dev);
> > >               iio_trigger_put(oldtrig);
> > >       }
> > > +     indio_dev->trig = trig;
> > >       if (indio_dev->trig) {
> > >               if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
> > > -                     iio_trigger_attach_poll_func(indio_dev->trig,
> > > -                                                  indio_dev-  
> > > >pollfunc_event);  
> > > +                     iio_trigger_attach_poll_func(indio_dev);
> > >       }
> > > 
> > >       return len;
> > > @@ -758,17 +767,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/drivers/iio/light/gp2ap020a00f.c
> > > b/drivers/iio/light/gp2ap020a00f.c
> > > index 44b13fbcd093..24d4a57b41be 100644
> > > --- a/drivers/iio/light/gp2ap020a00f.c
> > > +++ b/drivers/iio/light/gp2ap020a00f.c
> > > @@ -1423,12 +1423,8 @@ static int gp2ap020a00f_buffer_postenable(struct
> > > iio_dev *indio_dev)
> > >               goto error_unlock;
> > > 
> > >       data->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> > > -     if (!data->buffer) {
> > > +     if (!data->buffer)
> > >               err = -ENOMEM;
> > > -             goto error_unlock;
> > > -     }
> > > -
> > > -     err = iio_triggered_buffer_postenable(indio_dev);  
> > 
> > Again, a fairly definite no.  The buffer is probably better off being
> > allocated
> > as part of data anyway. Not sure on the rest.  
> > > 
> > >  error_unlock:
> > >       mutex_unlock(&data->lock);
> > > @@ -1443,10 +1439,6 @@ static int gp2ap020a00f_buffer_predisable(struct
> > > iio_dev *indio_dev)
> > > 
> > >       mutex_lock(&data->lock);
> > > 
> > > -     err = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (err < 0)
> > > -             goto error_unlock;
> > > -
> > >       for_each_set_bit(i, indio_dev->active_scan_mask,
> > >               indio_dev->masklength) {
> > >               switch (i) {
> > > @@ -1468,7 +1460,6 @@ static int gp2ap020a00f_buffer_predisable(struct
> > > iio_dev *indio_dev)
> > >       if (err == 0)
> > >               kfree(data->buffer);
> > > 
> > > -error_unlock:
> > >       mutex_unlock(&data->lock);
> > > 
> > >       return err;
> > > diff --git a/drivers/iio/light/isl29125.c
> > > b/drivers/iio/light/isl29125.c
> > > index ed38edcd5efe..5fdfce92019b 100644
> > > --- a/drivers/iio/light/isl29125.c
> > > +++ b/drivers/iio/light/isl29125.c
> > > @@ -230,10 +230,6 @@ static int isl29125_buffer_predisable(struct
> > > iio_dev *indio_dev)
> > >       struct isl29125_data *data = iio_priv(indio_dev);
> > >       int ret;
> > > 
> > > -     ret = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (ret < 0)
> > > -             return ret;
> > > -  
> > 
> > Another to check.  
> > >       data->conf1 &= ~ISL29125_MODE_MASK;
> > >       data->conf1 |= ISL29125_MODE_PD;
> > >       return i2c_smbus_write_byte_data(data->client, ISL29125_CONF1,
> > > @@ -242,7 +238,6 @@ static int isl29125_buffer_predisable(struct
> > > iio_dev *indio_dev)
> > > 
> > >  static const struct iio_buffer_setup_ops isl29125_buffer_setup_ops = {
> > >       .preenable = isl29125_buffer_preenable,
> > > -     .postenable = &iio_triggered_buffer_postenable,
> > >       .predisable = isl29125_buffer_predisable,
> > >  };
> > > 
> > > diff --git a/drivers/iio/light/tcs3414.c b/drivers/iio/light/tcs3414.c
> > > index 205e5659ce6b..c31ed0c640e9 100644
> > > --- a/drivers/iio/light/tcs3414.c
> > > +++ b/drivers/iio/light/tcs3414.c
> > > @@ -257,10 +257,6 @@ static int tcs3414_buffer_predisable(struct
> > > iio_dev *indio_dev)
> > >       struct tcs3414_data *data = iio_priv(indio_dev);
> > >       int ret;
> > > 
> > > -     ret = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (ret < 0)
> > > -             return ret;  
> > 
> > Another one that needs checking but 'probably' fine.  
> > > -
> > >       data->control &= ~TCS3414_CONTROL_ADC_EN;
> > >       return i2c_smbus_write_byte_data(data->client, TCS3414_CONTROL,
> > >               data->control);
> > > @@ -268,7 +264,6 @@ static int tcs3414_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > > 
> > >  static const struct iio_buffer_setup_ops tcs3414_buffer_setup_ops = {
> > >       .preenable = tcs3414_buffer_preenable,
> > > -     .postenable = &iio_triggered_buffer_postenable,
> > >       .predisable = tcs3414_buffer_predisable,
> > >  };
> > > 
> > > diff --git a/drivers/iio/magnetometer/st_magn_buffer.c
> > > b/drivers/iio/magnetometer/st_magn_buffer.c
> > > index 37ab30566464..570975200955 100644
> > > --- a/drivers/iio/magnetometer/st_magn_buffer.c
> > > +++ b/drivers/iio/magnetometer/st_magn_buffer.c
> > > @@ -32,25 +32,13 @@ int st_magn_trig_set_state(struct iio_trigger
> > > *trig, bool state)
> > >   
> > 
> > I'll guess the flow here is the same as the accel driver.  
> > >  static int st_magn_buffer_postenable(struct iio_dev *indio_dev)
> > >  {
> > > -     int err;
> > >       struct st_sensor_data *mdata = iio_priv(indio_dev);
> > > 
> > >       mdata->buffer_data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> > > -     if (mdata->buffer_data == NULL) {
> > > -             err = -ENOMEM;
> > > -             goto allocate_memory_error;
> > > -     }
> > > -
> > > -     err = iio_triggered_buffer_postenable(indio_dev);
> > > -     if (err < 0)
> > > -             goto st_magn_buffer_postenable_error;
> > > +     if (mdata->buffer_data == NULL)
> > > +             return -ENOMEM;
> > > 
> > >       return st_sensors_set_enable(indio_dev, true);
> > > -
> > > -st_magn_buffer_postenable_error:
> > > -     kfree(mdata->buffer_data);
> > > -allocate_memory_error:
> > > -     return err;
> > >  }
> > > 
> > >  static int st_magn_buffer_predisable(struct iio_dev *indio_dev)
> > > @@ -59,12 +47,7 @@ static int st_magn_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > >       struct st_sensor_data *mdata = iio_priv(indio_dev);
> > > 
> > >       err = st_sensors_set_enable(indio_dev, false);
> > > -     if (err < 0)
> > > -             goto st_magn_buffer_predisable_error;
> > > -
> > > -     err = iio_triggered_buffer_predisable(indio_dev);
> > > 
> > > -st_magn_buffer_predisable_error:
> > >       kfree(mdata->buffer_data);
> > >       return err;
> > >  }
> > > diff --git a/drivers/iio/potentiostat/lmp91000.c
> > > b/drivers/iio/potentiostat/lmp91000.c
> > > index 90e895adf997..268da7992ff5 100644
> > > --- a/drivers/iio/potentiostat/lmp91000.c
> > > +++ b/drivers/iio/potentiostat/lmp91000.c
> > > @@ -295,7 +295,6 @@ static int lmp91000_buffer_predisable(struct
> > > iio_dev *indio_dev)
> > > 
> > >  static const struct iio_buffer_setup_ops lmp91000_buffer_setup_ops = {
> > >       .preenable = lmp91000_buffer_preenable,
> > > -     .postenable = iio_triggered_buffer_postenable,  
> > 
> > huh?  We attach but never detach?  That one needs following up as it
> > is probably broken.
> >   
> > >       .predisable = lmp91000_buffer_predisable,
> > >  };
> > > 
> > > diff --git a/drivers/iio/pressure/st_pressure_buffer.c
> > > b/drivers/iio/pressure/st_pressure_buffer.c
> > > index 99468d0a64e7..4cb8239f2599 100644
> > > --- a/drivers/iio/pressure/st_pressure_buffer.c
> > > +++ b/drivers/iio/pressure/st_pressure_buffer.c
> > > @@ -37,25 +37,13 @@ static int st_press_buffer_preenable(struct iio_dev
> > > *indio_dev)
> > > 
> > >  static int st_press_buffer_postenable(struct iio_dev *indio_dev)
> > >  {
> > > -     int err;
> > >       struct st_sensor_data *press_data = iio_priv(indio_dev);
> > > 
> > >       press_data->buffer_data = kmalloc(indio_dev->scan_bytes,
> > > GFP_KERNEL);
> > > -     if (press_data->buffer_data == NULL) {
> > > -             err = -ENOMEM;
> > > -             goto allocate_memory_error;
> > > -     }
> > > -
> > > -     err = iio_triggered_buffer_postenable(indio_dev);
> > > -     if (err < 0)
> > > -             goto st_press_buffer_postenable_error;
> > > -
> > > -     return err;
> > > +     if (press_data->buffer_data == NULL)
> > > +             return -ENOMEM;
> > > 
> > > -st_press_buffer_postenable_error:
> > > -     kfree(press_data->buffer_data);
> > > -allocate_memory_error:
> > > -     return err;
> > > +     return 0;
> > >  }
> > > 
> > >  static int st_press_buffer_predisable(struct iio_dev *indio_dev)
> > > @@ -63,13 +51,8 @@ static int st_press_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > >       int err;
> > >       struct st_sensor_data *press_data = iio_priv(indio_dev);
> > > 
> > > -     err = iio_triggered_buffer_predisable(indio_dev);
> > > -     if (err < 0)
> > > -             goto st_press_buffer_predisable_error;
> > > -
> > >       err = st_sensors_set_enable(indio_dev, false);
> > > 
> > > -st_press_buffer_predisable_error:
> > >       kfree(press_data->buffer_data);
> > >       return err;
> > >  }
> > > diff --git a/drivers/iio/pressure/zpa2326.c
> > > b/drivers/iio/pressure/zpa2326.c
> > > index 81d8f24eaeb4..2c4295087a36 100644
> > > --- a/drivers/iio/pressure/zpa2326.c
> > > +++ b/drivers/iio/pressure/zpa2326.c  
> > 
> > Now I'm just leaving this here as needs more investigation...
> >   
> > > @@ -1271,11 +1271,6 @@ static int zpa2326_postenable_buffer(struct
> > > iio_dev *indio_dev)
> > >                       goto err;
> > >       }
> > > 
> > > -     /* Plug our own trigger event handler. */
> > > -     err = iio_triggered_buffer_postenable(indio_dev);
> > > -     if (err)
> > > -             goto err;
> > > -
> > >       return 0;
> > > 
> > >  err:
> > > @@ -1294,7 +1289,6 @@ static int zpa2326_postdisable_buffer(struct
> > > iio_dev *indio_dev)
> > >  static const struct iio_buffer_setup_ops zpa2326_buffer_setup_ops = {
> > >       .preenable   = zpa2326_preenable_buffer,
> > >       .postenable  = zpa2326_postenable_buffer,
> > > -     .predisable  = iio_triggered_buffer_predisable,
> > >       .postdisable = zpa2326_postdisable_buffer
> > >  };
> > > 
> > > diff --git a/drivers/iio/proximity/sx9500.c
> > > b/drivers/iio/proximity/sx9500.c
> > > index ff80409e0c44..73335e61b350 100644
> > > --- a/drivers/iio/proximity/sx9500.c
> > > +++ b/drivers/iio/proximity/sx9500.c  
> > 
> > Fairly sure this is safe, but separate patch to move it first is needed
> > before
> > this change.
> >   
> > > @@ -707,8 +707,6 @@ static int sx9500_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > >       struct sx9500_data *data = iio_priv(indio_dev);
> > >       int ret = 0, i;
> > > 
> > > -     iio_triggered_buffer_predisable(indio_dev);
> > > -
> > >       mutex_lock(&data->mutex);
> > > 
> > >       for (i = 0; i < SX9500_NUM_CHANNELS; i++)
> > > @@ -730,7 +728,6 @@ static int sx9500_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > > 
> > >  static const struct iio_buffer_setup_ops sx9500_buffer_setup_ops = {
> > >       .preenable = sx9500_buffer_preenable,
> > > -     .postenable = iio_triggered_buffer_postenable,
> > >       .predisable = sx9500_buffer_predisable,
> > >  };
> > >