Re: [PATCH] iio: adc: ti-ads124s08: Add direct mode protection and refactor read channel logic

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 27 Apr 2025 19:22:34 -0300
Lucas Sobrinho <lucasmsobrinho@gmail.com> wrote:

> Add a call to iio_device_claim_direct() in the ads124s_read_raw() function
> to ensure exclusive access to the device during direct mode reads.

Why?  This should talk about one of these reads in buffered mode might
cause to happen and why we want to prevent it.

I agree it will be destructive.  However, part of the problem here is
the trigger handler is not taking the mutex() which should be protecting
device specific state (i.e. the command state)

The direct mode claim is only meant to reflect whether transitioning
in or out of buffered mode is a problem during the read (which it is
but this isn't all that is going on!)
Hence I think we should be claiming direct mode and taking the mutex
with care needed to do that in the same order in all paths.


> 
> Also refactor the channel read logic into a separate function.
A patch with the word 'also' in it is a strong indication that it is
doing multiple things and should be multiple patches. 

The factoring out here is separate from dealing with the direct mode
claim + locking.  If it is easier to fix that stuff after factoring
out do it in that order. If it doesn't matter preference is fix first
then cleanup.

> 
> Signed-off-by: Lucas Sobrinho <lucasmsobrinho@gmail.com>
> Co-developed-by: Filipe Tressmann Velozo <filipetressmann@usp.br>
> Signed-off-by: Filipe Tressmann Velozo <filipetressmann@usp.br>
> ---
>  drivers/iio/adc/ti-ads124s08.c | 85 ++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads124s08.c b/drivers/iio/adc/ti-ads124s08.c
> index 8ea1269f7..e1efe8865 100644
> --- a/drivers/iio/adc/ti-ads124s08.c
> +++ b/drivers/iio/adc/ti-ads124s08.c
> @@ -219,6 +219,40 @@ static int ads124s_read(struct iio_dev *indio_dev)
>  	return get_unaligned_be24(&priv->data[2]);
>  }
>  
> +static int  ads124s_read_channel(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val)
> +{
> +	int ret = ads124s_write_reg(indio_dev, ADS124S08_INPUT_MUX, chan);
> +
> +	if (ret) {
> +		dev_err(&priv->spi->dev, "Set ADC CH failed\n");
> +		return ret;
> +	}
> +
> +	ret = ads124s_write_cmd(indio_dev, ADS124S08_START_CONV);
> +	if (ret) {
> +		dev_err(&priv->spi->dev, "Start ADC conversions failed\n");
> +		return ret;
> +	}
> +
> +	ret = ads124s_read(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&priv->spi->dev, "Read ADC failed\n");
> +		return ret;
> +	}
> +
> +	*val = ret;
> +
> +	ret = ads124s_write_cmd(indio_dev, ADS124S08_STOP_CONV);
> +	if (ret) {
> +		dev_err(&priv->spi->dev, "Stop conversions failed\n");
> +		return ret;
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
>  static int ads124s_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long m)
> @@ -226,44 +260,18 @@ static int ads124s_read_raw(struct iio_dev *indio_dev,
>  	struct ads124s_private *priv = iio_priv(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&priv->lock);

As above. This lock is there for a reason. You can't just drop it
and rely on the buffer state to give similar protection.
Whilst with the current implementation of claim_direct that will work
that is an implementation detail we cannot rely on in a driver.

>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		ret = ads124s_write_reg(indio_dev, ADS124S08_INPUT_MUX,
> -					chan->channel);
> -		if (ret) {
> -			dev_err(&priv->spi->dev, "Set ADC CH failed\n");
> -			goto out;
> -		}
> -
> -		ret = ads124s_write_cmd(indio_dev, ADS124S08_START_CONV);
> -		if (ret) {
> -			dev_err(&priv->spi->dev, "Start conversions failed\n");
> -			goto out;
> -		}
> -
> -		ret = ads124s_read(indio_dev);
> -		if (ret < 0) {
> -			dev_err(&priv->spi->dev, "Read ADC failed\n");
> -			goto out;
> -		}
> -
> -		*val = ret;
> -
> -		ret = ads124s_write_cmd(indio_dev, ADS124S08_STOP_CONV);
> -		if (ret) {
> -			dev_err(&priv->spi->dev, "Stop conversions failed\n");
> -			goto out;
> -		}
> -
> -		ret = IIO_VAL_INT;
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		ret = ads124s_read_channel(indio_dev, chan->channel, val);
> +		iio_device_release_direct(indio_dev);
>  		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
>  	}
> -out:
> -	mutex_unlock(&priv->lock);
>  	return ret;
>  }
>  
> @@ -280,20 +288,7 @@ static irqreturn_t ads124s_trigger_handler(int irq, void *p)
>  	int ret;
>  
>  	iio_for_each_active_channel(indio_dev, scan_index) {
> -		ret = ads124s_write_reg(indio_dev, ADS124S08_INPUT_MUX,
> -					scan_index);
> -		if (ret)
> -			dev_err(&priv->spi->dev, "Set ADC CH failed\n");
> -
> -		ret = ads124s_write_cmd(indio_dev, ADS124S08_START_CONV);
> -		if (ret)
> -			dev_err(&priv->spi->dev, "Start ADC conversions failed\n");
> -
> -		priv->buffer[j] = ads124s_read(indio_dev);
> -		ret = ads124s_write_cmd(indio_dev, ADS124S08_STOP_CONV);
> -		if (ret)
> -			dev_err(&priv->spi->dev, "Stop ADC conversions failed\n");
> -
> +		ads124s_read_channel(indio_dev, chan->channel, &(priv->buffer[j]));

You are passing a u32 pointer to something that takes an int pointer.  That's not
a valid thing to do so fix the types up to use something appropriate in both cases
u32.  Which makes made me curious.  How did this code work before?
Answer seems to be this isn't a 32 bit sensor.  The channel description is wrong.
It's only 24 bits.  So we should fix that as well whilst here.  Given it's only 24 bits
using an int *  for the parameter here is fine.

>  		j++;
>  	}
>