Re: [PATCH] iio: adc: ad4000: Add iio_device_claim_direct() to protect buffered captures

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

Hi, Fernando, Octávio, Lucas,

I think you forgot to run get_maintainer.pl to get all the expected recipients
for this patch. Also, even though the code seems to be syntactically correct,
the proposed changes are not suitable for the IIO driver. See comments inline.

On 05/07, Octávio Carneiro wrote:
> Add iio_device_claim_direct() to protect buffered captures. In
> ad4000_write_raw_get_fmt, data reads are protected by the
> function call to avoid possible errors caused by concurrent
> access.
> 
> Signed-off-by: Fernando Lima <fernandolimabusiness@gmail.com>
> Co-developed-by: Octávio Carneiro <ocarneiro1@gmail.com>
> Signed-off-by: Octávio Carneiro <ocarneiro1@gmail.com>
> Co-developed-by: Lucas Eiji <eijiuchyama@usp.br>
> Signed-off-by: Lucas Eiji <eijiuchyama@usp.br>
> ---
>  drivers/iio/adc/ad4000.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> index 4fe8dee48..55ceee6e9 100644
> --- a/drivers/iio/adc/ad4000.c
> +++ b/drivers/iio/adc/ad4000.c
> @@ -583,6 +583,9 @@ static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
>  {
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +		iio_device_release_direct(indio_dev);
In the ad4000 driver, the main purpose of claiming IIO direct mode is to keep
the integrity of buffered data capture. Basically, we want to avoid changes of
device state properties and the issue of concurrent readings.
Note that under the hood, iio_device_claim_direct() acquires a mutex lock.
Also, it is good practice to protect only the critical paths and to not keep 
locks of access control mechanisms for longer than what's really needed.
Thus, we claim IIO direct mode at ad4000_read_raw() and at ad4000_write_raw(),
right before running ADC data or register access transfers.

So, the proposed addition is not needed and doesn't really follow best
programming practices.

>  		return IIO_VAL_INT_PLUS_NANO;
>  	default:
>  		return IIO_VAL_INT_PLUS_MICRO;

With best regards,
Marcelo