[v1] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()

[PATCH] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()

Posted by Alexandru Ardelean 1 year, 5 months ago

The ad7606_scan_direct() function returns 'int', which is fine for 16-bit
samples.
But when going to 18-bit samples, these need to be implemented as 32-bit
(or int) type.

In that case when getting samples (which can be negative), we'd get random
error codes.
So, the easiest thing is to, just move the 'val' pointer to
'ad7606_scan_direct()'. This doesn't qualify as a fix, it's just a
preparation for 18-bit ADCs (of the AD7606 family).

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index dba1f28782e4..35b2862c3928 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -138,7 +138,8 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
-static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
+static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
+			      int *val)
 {
 	struct ad7606_state *st = iio_priv(indio_dev);
 	int ret;
@@ -153,7 +154,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 
 	ret = ad7606_read_samples(st);
 	if (ret == 0)
-		ret = st->data[ch];
+		ret = (short)st->data[ch];
 
 error_ret:
 	gpiod_set_value(st->gpio_convst, 0);
@@ -173,10 +174,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
 		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			ret = ad7606_scan_direct(indio_dev, chan->address);
+			ret = ad7606_scan_direct(indio_dev, chan->address, val);
 			if (ret < 0)
 				return ret;
-			*val = (short) ret;
 			return IIO_VAL_INT;
 		}
 		unreachable();
-- 
2.46.0

Re: [PATCH] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()

Posted by Alexandru Ardelean 1 year, 5 months ago

On Sat, Aug 17, 2024 at 5:23 PM Alexandru Ardelean
<aardelean@baylibre.com> wrote:
>
> The ad7606_scan_direct() function returns 'int', which is fine for 16-bit
> samples.
> But when going to 18-bit samples, these need to be implemented as 32-bit
> (or int) type.
>
> In that case when getting samples (which can be negative), we'd get random
> error codes.
> So, the easiest thing is to, just move the 'val' pointer to
> 'ad7606_scan_direct()'. This doesn't qualify as a fix, it's just a
> preparation for 18-bit ADCs (of the AD7606 family).
>
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---

Baaah.
Please disregard this patch.

It seems I picked it up from an older working branch by mistake.

I still need to learn to use this new b4 tool.

Apologies.

>  drivers/iio/adc/ad7606.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index dba1f28782e4..35b2862c3928 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -138,7 +138,8 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>         return IRQ_HANDLED;
>  }
>
> -static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
> +static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> +                             int *val)
>  {
>         struct ad7606_state *st = iio_priv(indio_dev);
>         int ret;
> @@ -153,7 +154,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>
>         ret = ad7606_read_samples(st);
>         if (ret == 0)
> -               ret = st->data[ch];
> +               ret = (short)st->data[ch];
>
>  error_ret:
>         gpiod_set_value(st->gpio_convst, 0);
> @@ -173,10 +174,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>         switch (m) {
>         case IIO_CHAN_INFO_RAW:
>                 iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -                       ret = ad7606_scan_direct(indio_dev, chan->address);
> +                       ret = ad7606_scan_direct(indio_dev, chan->address, val);
>                         if (ret < 0)
>                                 return ret;
> -                       *val = (short) ret;
>                         return IIO_VAL_INT;
>                 }
>                 unreachable();
> --
> 2.46.0
>

[PATCH v2] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()

Posted by Alexandru Ardelean 1 year, 5 months ago

The ad7606_scan_direct() function returns 'int', which is fine for 16-bit
samples.
But when going to 18-bit samples, these need to be implemented as 32-bit
(or int) type.

In that case when getting samples (which can be negative), we'd get random
error codes.
So, the easiest thing is to just move the 'val' pointer to
'ad7606_scan_direct()'. This doesn't qualify as a fix, it's just a
preparation for 18-bit ADCs (of the AD7606 family).

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index dba1f28782e4..68481e97e50a 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -138,7 +138,8 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
-static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
+static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
+			      int *val)
 {
 	struct ad7606_state *st = iio_priv(indio_dev);
 	int ret;
@@ -153,7 +154,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 
 	ret = ad7606_read_samples(st);
 	if (ret == 0)
-		ret = st->data[ch];
+		*val = (short)st->data[ch];
 
 error_ret:
 	gpiod_set_value(st->gpio_convst, 0);
@@ -173,10 +174,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
 		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			ret = ad7606_scan_direct(indio_dev, chan->address);
+			ret = ad7606_scan_direct(indio_dev, chan->address, val);
 			if (ret < 0)
 				return ret;
-			*val = (short) ret;
 			return IIO_VAL_INT;
 		}
 		unreachable();
-- 
2.46.0

Re: [PATCH v2] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()

Posted by Jonathan Cameron 1 year, 5 months ago

On Sat, 17 Aug 2024 17:36:47 +0300
Alexandru Ardelean <aardelean@baylibre.com> wrote:

> The ad7606_scan_direct() function returns 'int', which is fine for 16-bit
> samples.
> But when going to 18-bit samples, these need to be implemented as 32-bit
> (or int) type.
> 
> In that case when getting samples (which can be negative), we'd get random
> error codes.
> So, the easiest thing is to just move the 'val' pointer to
> 'ad7606_scan_direct()'. This doesn't qualify as a fix, it's just a
> preparation for 18-bit ADCs (of the AD7606 family).

Send it at the start of that series then so as not to confuse me :)

> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---
>  drivers/iio/adc/ad7606.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index dba1f28782e4..68481e97e50a 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -138,7 +138,8 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> -static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
> +static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> +			      int *val)
>  {
>  	struct ad7606_state *st = iio_priv(indio_dev);
>  	int ret;
> @@ -153,7 +154,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  
>  	ret = ad7606_read_samples(st);
>  	if (ret == 0)
> -		ret = st->data[ch];
> +		*val = (short)st->data[ch];
I'd rather see a sign_extend32() for that seeing as you are touching the code.
obviously it's the same thing but nicer to read!
>  
>  error_ret:
>  	gpiod_set_value(st->gpio_convst, 0);
> @@ -173,10 +174,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
>  		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			ret = ad7606_scan_direct(indio_dev, chan->address);
> +			ret = ad7606_scan_direct(indio_dev, chan->address, val);
>  			if (ret < 0)
>  				return ret;
> -			*val = (short) ret;
>  			return IIO_VAL_INT;
>  		}
>  		unreachable();