On Sun, 2020-05-24 at 14:38 +0100, Jonathan Cameron wrote:
> [External]
>
> On Fri, 22 May 2020 13:46:32 +0300
> Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
>
> > From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> >
> > This patch should be squashed into the first one, as the first one is
> > breaking the build (intentionally) to make the IIO core files easier to
> > review.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
>
> Yeah! Didn't realise you'd finally gotten to the end of your mammoth rework
> leading to this.
>
> A few really minor things inline to tidy up.
Will fix these and send a V2.
>
> Thanks,
>
> Jonathan
>
>
> > diff --git a/drivers/iio/accel/st_accel_buffer.c
> > b/drivers/iio/accel/st_accel_buffer.c
> > index b5c814ef1637..c87f9a7d2453 100644
> > --- a/drivers/iio/accel/st_accel_buffer.c
> > +++ b/drivers/iio/accel/st_accel_buffer.c
> > @@ -33,13 +33,9 @@ static int st_accel_buffer_postenable(struct iio_dev
> > *indio_dev)
> > {
> > int err;
> >
> > - err = iio_triggered_buffer_postenable(indio_dev);
> > - if (err < 0)
> > - return err;
> > -
> > err = st_sensors_set_axis_enable(indio_dev, indio_dev-
> > >active_scan_mask[0]);
> > if (err < 0)
> > - goto st_accel_buffer_predisable;
> > + return err;
> >
> > err = st_sensors_set_enable(indio_dev, true);
> > if (err < 0)
> > @@ -49,8 +45,6 @@ static int st_accel_buffer_postenable(struct iio_dev
> > *indio_dev)
> >
> > st_accel_buffer_enable_all_axis:
> > st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
> > -st_accel_buffer_predisable:
> > - iio_triggered_buffer_predisable(indio_dev);
> > return err;
> > }
> >
> > @@ -60,12 +54,10 @@ static int st_accel_buffer_predisable(struct iio_dev
> > *indio_dev)
> >
> > err = st_sensors_set_enable(indio_dev, false);
> > if (err < 0)
> > - goto st_accel_buffer_predisable;
> > -
> > - err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
> > + return err;
> >
> > -st_accel_buffer_predisable:
> > - err2 = iio_triggered_buffer_predisable(indio_dev);
> > + err2 = st_sensors_set_axis_enable(indio_dev,
> > + ST_SENSORS_ENABLE_ALL_AXIS);
> > if (!err)
> I don't think you can get here with err set.
> > err = err2;
> >
>
> ...
>
> > diff --git a/drivers/iio/gyro/st_gyro_buffer.c
> > b/drivers/iio/gyro/st_gyro_buffer.c
> > index 9c92ff7a82be..7b86502d5da3 100644
> > --- a/drivers/iio/gyro/st_gyro_buffer.c
> > +++ b/drivers/iio/gyro/st_gyro_buffer.c
> > @@ -33,13 +33,9 @@ static int st_gyro_buffer_postenable(struct iio_dev
> > *indio_dev)
> > {
> > int err;
> >
> > - err = iio_triggered_buffer_postenable(indio_dev);
> > - if (err < 0)
> > - return err;
> > -
> > err = st_sensors_set_axis_enable(indio_dev, indio_dev-
> > >active_scan_mask[0]);
> > if (err < 0)
> > - goto st_gyro_buffer_predisable;
> > + return err;
> >
> > err = st_sensors_set_enable(indio_dev, true);
> > if (err < 0)
> > @@ -49,8 +45,6 @@ static int st_gyro_buffer_postenable(struct iio_dev
> > *indio_dev)
> >
> > st_gyro_buffer_enable_all_axis:
> > st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
> > -st_gyro_buffer_predisable:
> > - iio_triggered_buffer_predisable(indio_dev);
> > return err;
> > }
> >
> > @@ -59,13 +53,8 @@ static int st_gyro_buffer_predisable(struct iio_dev
> > *indio_dev)
> > int err, err2;
> >
> > err = st_sensors_set_enable(indio_dev, false);
> > - if (err < 0)
> > - goto st_gyro_buffer_predisable;
>
> Previously we didn't bother trying to carry on if this failed. I don't think
> we
> should start doing so now.
>
> > -
> > - err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
> >
> > -st_gyro_buffer_predisable:
> > - err2 = iio_triggered_buffer_predisable(indio_dev);
> > + err2 = st_sensors_set_axis_enable(indio_dev,
> > ST_SENSORS_ENABLE_ALL_AXIS);
> > if (!err)
> > err = err2;
> >
>
> ...
>
> > diff --git a/drivers/iio/light/gp2ap020a00f.c
> > b/drivers/iio/light/gp2ap020a00f.c
> > index 070d4cd0cf54..29d7af33efa1 100644
> > --- a/drivers/iio/light/gp2ap020a00f.c
> > +++ b/drivers/iio/light/gp2ap020a00f.c
> > @@ -1390,12 +1390,6 @@ static int gp2ap020a00f_buffer_postenable(struct
> > iio_dev *indio_dev)
> >
> > mutex_lock(&data->lock);
> I guess it doesn't matter, but no idea why this was ever under the local lock!
>
> >
> > - err = iio_triggered_buffer_postenable(indio_dev);
> > - if (err < 0) {
> > - mutex_unlock(&data->lock);
> > - return err;
> > - }
> > -
> > /*
> > * Enable triggers according to the scan_mask. Enabling either
> > * LIGHT_CLEAR or LIGHT_IR scan mode results in enabling ALS
> > @@ -1430,8 +1424,6 @@ static int gp2ap020a00f_buffer_postenable(struct
> > iio_dev *indio_dev)
> > err = -ENOMEM;
> >
> > error_unlock:
> > - if (err < 0)
> > - iio_triggered_buffer_predisable(indio_dev);
> > mutex_unlock(&data->lock);
> >
> > return err;
> > @@ -1465,8 +1457,6 @@ static int gp2ap020a00f_buffer_predisable(struct
> > iio_dev *indio_dev)
> > if (err == 0)
> > kfree(data->buffer);
> >
> > - iio_triggered_buffer_predisable(indio_dev);
> > -
> > mutex_unlock(&data->lock);
> >
> > return err;
>
> ...
>
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 2a4b3d331055..0fee767af026 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -957,29 +957,20 @@ static int vcnl4010_buffer_postenable(struct iio_dev
> > *indio_dev)
> > int ret;
> > int cmd;
> >
> > - ret = iio_triggered_buffer_postenable(indio_dev);
> > - if (ret)
> > - return ret;
> > -
> > /* Do not enable the buffer if we are already capturing events. */
> > - if (vcnl4010_is_in_periodic_mode(data)) {
> > - ret = -EBUSY;
> > - goto end;
> > - }
> > + if (vcnl4010_is_in_periodic_mode(data))
> > + return -EBUSY;
> >
> > ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL,
> > VCNL4010_INT_PROX_EN);
> > if (ret < 0)
> > - goto end;
> > + return ret;
> >
> > cmd = VCNL4000_SELF_TIMED_EN | VCNL4000_PROX_EN;
> > +
> > ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, cmd);
> > if (ret < 0)
> > - goto end;
> > -
> > - return 0;
> > -end:
> > - iio_triggered_buffer_predisable(indio_dev);
> > + i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL, 0);
> >
> > return ret;
> > }
> > @@ -987,18 +978,14 @@ static int vcnl4010_buffer_postenable(struct iio_dev
> > *indio_dev)
> > static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
> > {
> > struct vcnl4000_data *data = iio_priv(indio_dev);
> > - int ret, ret_disable;
> > + int ret, ret2;
> >
> > ret = i2c_smbus_write_byte_data(data->client, VCNL4010_INT_CTRL, 0);
> > - if (ret < 0)
> > - goto end;
> >
> > - ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
> > + ret2 = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
>
> hmm. This does change the flow a tiny bit. I wonder if we really
> care about carrying on if we get an error on the first write?
> We are device not responding territory at that point. Maybe just return
> immediately and avoid the dance with the two ret variables?
>
> >
> > -end:
> > - ret_disable = iio_triggered_buffer_predisable(indio_dev);
> > if (ret == 0)
> > - ret = ret_disable;
> > + ret = ret2;
> >
> > return ret;
> > }
>
> ...
>
> > static const struct iio_buffer_setup_ops st_press_buffer_setup_ops = {
> > diff --git a/drivers/iio/pressure/zpa2326.c b/drivers/iio/pressure/zpa2326.c
> > index 37fe851f89af..e082ad007b22 100644
> > --- a/drivers/iio/pressure/zpa2326.c
> > +++ b/drivers/iio/pressure/zpa2326.c
> > @@ -1240,12 +1240,7 @@ static int zpa2326_preenable_buffer(struct iio_dev
> > *indio_dev)
> > static int zpa2326_postenable_buffer(struct iio_dev *indio_dev)
> > {
> > const struct zpa2326_private *priv = iio_priv(indio_dev);
> > - int err;
> > -
> > - /* Plug our own trigger event handler. */
> > - err = iio_triggered_buffer_postenable(indio_dev);
> > - if (err)
> > - goto err;
> > + int err = 0;
> >
> > if (!priv->waken) {
> > /*
> > @@ -1254,7 +1249,7 @@ static int zpa2326_postenable_buffer(struct iio_dev
> > *indio_dev)
> > */
> > err = zpa2326_clear_fifo(indio_dev, 0);
> > if (err)
> > - goto err_buffer_predisable;
> > + goto out;
> > }
> >
> > if (!iio_trigger_using_own(indio_dev) && priv->waken) {
> > @@ -1264,14 +1259,10 @@ static int zpa2326_postenable_buffer(struct iio_dev
> > *indio_dev)
> > */
> > err = zpa2326_config_oneshot(indio_dev, priv->irq);
> > if (err)
> > - goto err_buffer_predisable;
> > + goto out;
> > }
> >
> > - return 0;
> > -
> > -err_buffer_predisable:
> > - iio_triggered_buffer_predisable(indio_dev);
> > -err:
> > +out:
> > zpa2326_err(indio_dev, "failed to enable buffering (%d)", err);
>
> Doesn't this now print the error in the good path?
>
> Probably still want the return 0. It's a bit messier but I'd
> just move the prints into the error paths and return directly from
> each. Will be cleaner code that this.
>
>
> >
> > return err;
> > @@ -1287,7 +1278,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
> > };
> >
