Re: [PATCH 3/3] iio: remove iio_triggered_buffer_postenable()/iio_triggered_buffer_predisable()

From: Jonathan Cameron
Date: Sun May 24 2020 - 09:39:04 EST

Next message: Jonathan Cameron: "Re: [PATCH 1/3] iio: Move attach/detach of the poll func to the core"
Previous message: GAURAV PATHAK: "Re: [PATCH] Removing ununsed variable int lo, hi, int data and int i from comedi/drivers/dt2814.c."
Next in thread: Ardelean, Alexandru: "Re: [PATCH 3/3] iio: remove iio_triggered_buffer_postenable()/iio_triggered_buffer_predisable()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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

Next message: Jonathan Cameron: "Re: [PATCH 1/3] iio: Move attach/detach of the poll func to the core"
Previous message: GAURAV PATHAK: "Re: [PATCH] Removing ununsed variable int lo, hi, int data and int i from comedi/drivers/dt2814.c."
Next in thread: Ardelean, Alexandru: "Re: [PATCH 3/3] iio: remove iio_triggered_buffer_postenable()/iio_triggered_buffer_predisable()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]