staging: iio: adt7316: Add error handling to adt7316_spi_probe()

[PATCH] staging: iio: adt7316: Add error handling to adt7316_spi_probe()

Posted by Maxwell Doose 1 month, 3 weeks ago

Currently, the return values of the adt7316_spi_write() calls in
adt7316_spi_probe() are unchecked. Add error handling to return early
and pass on the error code if we receive an error from
adt7316_spi_write().

While at it, move all three adt7316_spi_write() calls inside a for loop
to condense the logic.

Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 drivers/staging/iio/addac/adt7316-spi.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/addac/adt7316-spi.c b/drivers/staging/iio/addac/adt7316-spi.c
index f91325d11394..ffd1dae8d1b1 100644
--- a/drivers/staging/iio/addac/adt7316-spi.c
+++ b/drivers/staging/iio/addac/adt7316-spi.c
@@ -98,6 +98,7 @@ static int adt7316_spi_probe(struct spi_device *spi_dev)
 		.multi_read = adt7316_spi_multi_read,
 		.multi_write = adt7316_spi_multi_write,
 	};
+	int i, ret;
 
 	/* don't exceed max specified SPI CLK frequency */
 	if (spi_dev->max_speed_hz > ADT7316_SPI_MAX_FREQ_HZ) {
@@ -107,9 +108,12 @@ static int adt7316_spi_probe(struct spi_device *spi_dev)
 	}
 
 	/* switch from default I2C protocol to SPI protocol */
-	adt7316_spi_write(spi_dev, 0, 0);
-	adt7316_spi_write(spi_dev, 0, 0);
-	adt7316_spi_write(spi_dev, 0, 0);
+	for (i = 0; i < 3; i++) {
+		ret = adt7316_spi_write(spi_dev, 0, 0);
+		/* Check for errors, if we get an error, return early */
+		if (ret < 0)
+			return ret;
+	}
 
 	return adt7316_probe(&spi_dev->dev, &bus, spi_dev->modalias);
 }
-- 
2.53.0

Re: [PATCH] staging: iio: adt7316: Add error handling to adt7316_spi_probe()

Posted by Jonathan Cameron 1 month, 2 weeks ago

On Sun, 26 Apr 2026 15:50:38 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> Currently, the return values of the adt7316_spi_write() calls in
> adt7316_spi_probe() are unchecked. Add error handling to return early
> and pass on the error code if we receive an error from
> adt7316_spi_write().
> 
> While at it, move all three adt7316_spi_write() calls inside a for loop
> to condense the logic.
> 
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
>  drivers/staging/iio/addac/adt7316-spi.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316-spi.c b/drivers/staging/iio/addac/adt7316-spi.c
> index f91325d11394..ffd1dae8d1b1 100644
> --- a/drivers/staging/iio/addac/adt7316-spi.c
> +++ b/drivers/staging/iio/addac/adt7316-spi.c
> @@ -98,6 +98,7 @@ static int adt7316_spi_probe(struct spi_device *spi_dev)
>  		.multi_read = adt7316_spi_multi_read,
>  		.multi_write = adt7316_spi_multi_write,
>  	};
> +	int i, ret;
>  
>  	/* don't exceed max specified SPI CLK frequency */
>  	if (spi_dev->max_speed_hz > ADT7316_SPI_MAX_FREQ_HZ) {
> @@ -107,9 +108,12 @@ static int adt7316_spi_probe(struct spi_device *spi_dev)
>  	}
>  
>  	/* switch from default I2C protocol to SPI protocol */
> -	adt7316_spi_write(spi_dev, 0, 0);
> -	adt7316_spi_write(spi_dev, 0, 0);
> -	adt7316_spi_write(spi_dev, 0, 0);
> +	for (i = 0; i < 3; i++) {
> +		ret = adt7316_spi_write(spi_dev, 0, 0);
> +		/* Check for errors, if we get an error, return early */

If this is tested (and I agree with the others that this is a special
bit of code, as it's hammering a device using one protocol with messages
using the other, then the comment is pointless given we can see the code.

> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	return adt7316_probe(&spi_dev->dev, &bus, spi_dev->modalias);
>  }

Re: [PATCH] staging: iio: adt7316: Add error handling to adt7316_spi_probe()

Posted by Andy Shevchenko 1 month, 2 weeks ago

On Sun, Apr 26, 2026 at 03:50:38PM -0500, Maxwell Doose wrote:
> Currently, the return values of the adt7316_spi_write() calls in
> adt7316_spi_probe() are unchecked. Add error handling to return early
> and pass on the error code if we receive an error from
> adt7316_spi_write().
> 
> While at it, move all three adt7316_spi_write() calls inside a for loop
> to condense the logic.

...

> -	adt7316_spi_write(spi_dev, 0, 0);
> -	adt7316_spi_write(spi_dev, 0, 0);
> -	adt7316_spi_write(spi_dev, 0, 0);
> +	for (i = 0; i < 3; i++) {

	for (unsigned int i = 0; i < 3; i++) {

The magic 3 has to be explained in the comment above.

> +		ret = adt7316_spi_write(spi_dev, 0, 0);
> +		/* Check for errors, if we get an error, return early */
> +		if (ret < 0)
> +			return ret;
> +	}

Are you sure that's fine to make those fatal? Have you read datasheet on this?
maybe it will return errors which we have to ignore?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: iio: adt7316: Add error handling to adt7316_spi_probe()

Posted by Dan Carpenter 1 month, 2 weeks ago

On Sun, Apr 26, 2026 at 03:50:38PM -0500, Maxwell Doose wrote:
> Currently, the return values of the adt7316_spi_write() calls in
> adt7316_spi_probe() are unchecked. Add error handling to return early
> and pass on the error code if we receive an error from
> adt7316_spi_write().
> 
> While at it, move all three adt7316_spi_write() calls inside a for loop
> to condense the logic.
> 
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---

A lot of the time, functions can't fail or if they do it means
you need to buy a new computer and there is nothing the
operating system can do to help you.  spi_write() feels like
one of those things.

Plus we wouldn't add new checks like this unless they had been
tested.

regards,
dan carpenter

Re: [PATCH] staging: iio: adt7316: Add error handling to adt7316_spi_probe()

Posted by Jonathan Cameron 1 month, 2 weeks ago

On Mon, 27 Apr 2026 11:18:51 +0300
Dan Carpenter <error27@gmail.com> wrote:

> On Sun, Apr 26, 2026 at 03:50:38PM -0500, Maxwell Doose wrote:
> > Currently, the return values of the adt7316_spi_write() calls in
> > adt7316_spi_probe() are unchecked. Add error handling to return early
> > and pass on the error code if we receive an error from
> > adt7316_spi_write().
> > 
> > While at it, move all three adt7316_spi_write() calls inside a for loop
> > to condense the logic.
> > 
> > Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> > ---  
> 
> A lot of the time, functions can't fail or if they do it means
> you need to buy a new computer and there is nothing the
> operating system can do to help you.  spi_write() feels like
> one of those things.

We do normally check spi and i2c writes.  Though given spi doesn't
have known markers like I2C does, it's not always clear we can detect
an error.  However, these are 'special' as the comment above makes
clear. Device is in i2c mode and a magic SPI hammering sequence is
used to make it switch. For this sort of stuff all bets are off.
If it were the other way around we'd definitely expect errors from
the i2c transfers - though as mentioned above it is really hard
to tell if SPI failed.

Anyhow as far as I'm concerned adding checks to normal calls is
fine, but not to things around reset, mode switches or power
management unless you can test them.

> 
> Plus we wouldn't add new checks like this unless they had been
> tested.
> 
> regards,
> dan carpenter
>

Re: [PATCH] staging: iio: adt7316: Add error handling to adt7316_spi_probe()

Posted by Maxwell Doose 1 month, 2 weeks ago

Hi everyone,

On Mon, Apr 27, 2026 at 5:02 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> We do normally check spi and i2c writes.

Knew that, I was looking for unchecked probing and writing in this
area and came across this.

> Anyhow as far as I'm concerned adding checks to normal calls is
> fine, but not to things around reset, mode switches or power
> management unless you can test them.

Sorry, I didn't realize this area was *very* special. Obviously I do
now, but anyways, I'm going to drop this patch and focus on some other
things.

best regards,
maxwell