* [PATCH] iio: adc: ti-ads124s08: Add direct mode protection and refactor read channel logic
@ 2025-04-27 22:22 Lucas Sobrinho
2025-05-05 15:32 ` Jonathan Cameron
0 siblings, 1 reply; 2+ messages in thread
From: Lucas Sobrinho @ 2025-04-27 22:22 UTC (permalink / raw)
To: jic23; +Cc: linux-iio, filipetressmann, davidbtadokoro, paulormm,
Lucas Sobrinho
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.
Also refactor the channel read logic into a separate function.
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);
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]));
j++;
}
--
2.49.0
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] iio: adc: ti-ads124s08: Add direct mode protection and refactor read channel logic
2025-04-27 22:22 [PATCH] iio: adc: ti-ads124s08: Add direct mode protection and refactor read channel logic Lucas Sobrinho
@ 2025-05-05 15:32 ` Jonathan Cameron
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Cameron @ 2025-05-05 15:32 UTC (permalink / raw)
To: Lucas Sobrinho; +Cc: linux-iio, filipetressmann, davidbtadokoro, paulormm
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++;
> }
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-05-05 15:33 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-27 22:22 [PATCH] iio: adc: ti-ads124s08: Add direct mode protection and refactor read channel logic Lucas Sobrinho
2025-05-05 15:32 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox