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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> 
> 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.

> 
> 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,
> >  };
> > 




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux