Re: [PATCH 3/4] iio: proximity: vncl3020: remove mutex from vcnl3020_data

[Jonathan Cameron]

On Fri, 30 Apr 2021 18:24:18 +0300
Ivan Mikhaylov <i.mikhaylov@xxxxxxxxx> wrote:

> Remove the mutex from vcnl3020_data structure and change it on
> iio_device_claim_direct_mode/iio_device_release_direct_mode.
> 
> Signed-off-by: Ivan Mikhaylov <i.mikhaylov@xxxxxxxxx>
I'm not keen on doing this.  The reason is that
iio_device_claim_direct_mode() is today implemented via a mutex which could
be used as you have it here, but... It might not be in the future.
I can see we might for example optimize it to allow multiple concurrent
accesses that assume direct mode.

So don't make assumptions about what it does.  It should be used if
you are protecting against the device entering (or already being in) a buffered
mode, but nothing else.

If other scope is needed, then stick to a local lock.  It's perfectly
fine to claim direct mode and take a lock of course if you are protecting
against different things.

Jonathan

> ---
>  drivers/iio/proximity/vcnl3020.c | 40 ++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
> index 0dfa6a0b5eec..bff59c7af966 100644
> --- a/drivers/iio/proximity/vcnl3020.c
> +++ b/drivers/iio/proximity/vcnl3020.c
> @@ -71,13 +71,11 @@ static const int vcnl3020_prox_sampling_frequency[][2] = {
>   * @regmap:	device register map.
>   * @dev:	vcnl3020 device.
>   * @rev:	revision id.
> - * @lock:	lock for protecting access to device hardware registers.
>   */
>  struct vcnl3020_data {
>  	struct regmap *regmap;
>  	struct device *dev;
>  	u8 rev;
> -	struct mutex lock;
>  };
>  
>  /**
> @@ -149,7 +147,6 @@ static int vcnl3020_init(struct vcnl3020_data *data)
>  	}
>  
>  	data->rev = reg;
> -	mutex_init(&data->lock);
>  
>  	return vcnl3020_get_and_apply_property(data,
>  					       vcnl3020_led_current_property);
> @@ -161,11 +158,9 @@ static int vcnl3020_measure_proximity(struct vcnl3020_data *data, int *val)
>  	unsigned int reg;
>  	__be16 res;
>  
> -	mutex_lock(&data->lock);
> -
>  	rc = regmap_write(data->regmap, VCNL_COMMAND, VCNL_PS_OD);
>  	if (rc)
> -		goto err_unlock;
> +		return rc;
>  
>  	/* wait for data to become ready */
>  	rc = regmap_read_poll_timeout(data->regmap, VCNL_COMMAND, reg,
> @@ -174,20 +169,17 @@ static int vcnl3020_measure_proximity(struct vcnl3020_data *data, int *val)
>  	if (rc) {
>  		dev_err(data->dev,
>  			"Error (%d) reading vcnl3020 command register\n", rc);
> -		goto err_unlock;
> +		return rc;
>  	}
>  
>  	/* high & low result bytes read */
>  	rc = regmap_bulk_read(data->regmap, VCNL_PS_RESULT_HI, &res,
>  			      sizeof(res));
>  	if (rc)
> -		goto err_unlock;
> +		return rc;
>  
>  	*val = be16_to_cpu(res);
>  
> -err_unlock:
> -	mutex_unlock(&data->lock);
> -
>  	return rc;
>  }
>  
> @@ -450,25 +442,37 @@ static int vcnl3020_read_raw(struct iio_dev *indio_dev,
>  	int rc;
>  	struct vcnl3020_data *data = iio_priv(indio_dev);
>  
> +	rc = iio_device_claim_direct_mode(indio_dev);
> +	if (rc)
> +		return rc;
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  
>  		/* Protect against event capture. */
> -		if (vcnl3020_is_in_periodic_mode(data))
> -			return -EBUSY;
> +		if (vcnl3020_is_in_periodic_mode(data)) {
> +			rc = -EBUSY;
> +			goto end;
> +		}
>  
>  		rc = vcnl3020_measure_proximity(data, val);
>  		if (rc)
> -			return rc;
> -		return IIO_VAL_INT;
> +			goto end;
> +		rc = IIO_VAL_INT;
> +		goto end;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		rc = vcnl3020_read_proxy_samp_freq(data, val, val2);
>  		if (rc < 0)
> -			return rc;
> -		return IIO_VAL_INT_PLUS_MICRO;
> +			goto end;
> +		rc = IIO_VAL_INT_PLUS_MICRO;
> +		goto end;
>  	default:
> -		return -EINVAL;
> +		rc = -EINVAL;
>  	}
> +
> +end:
> +	iio_device_release_direct_mode(indio_dev);
> +	return rc;
>  }
>  
>  static int vcnl3020_write_raw(struct iio_dev *indio_dev,