[PATCH v3 0/6] staging:iio:ad2s90: Add scale info and improve error handling

[PATCH v3 0/6] staging:iio:ad2s90: Add scale info and improve error handling

[Matheus Tavares]

This patch set adds scale info to ad2s90's single channel, improve
error handling in it's functions and fix a possible race condition
issue.

The goal with this patch set is to address the points discussed in the
mailing list in an effort to move ad2s90.c out of staging.

Changes in v3:
 - Removed unconnected change in patch 1 (whitespace tidying up).
 - Added comment to patch 2's description regarding a code block that
was moved in patch 4.
 - Corrected scale in patch 5, from 2Pi/(2^12 - 1) to 2Pi/2^12 and
using IIO_VAL_FRACTIONAL_LOG2.

Changes in v2:
 - Added my S-o-B in patch 5. 

Matheus Tavares (5):
  staging:iio:ad2s90: Make read_raw return spi_read's error code
  staging:iio:ad2s90: Make probe handle spi_setup failure
  staging:iio:ad2s90: Remove always overwritten assignment
  staging:iio:ad2s90: Move device registration to the end of probe
  staging:iio:ad2s90: Check channel type at read_raw

Victor Colombo (1):
  staging:iio:ad2s90: Add IIO_CHAN_INFO_SCALE to channel spec and
    read_raw

 drivers/staging/iio/resolver/ad2s90.c | 53 ++++++++++++++++++---------
 1 file changed, 35 insertions(+), 18 deletions(-)

-- 
2.18.0

[PATCH v3 1/6] staging:iio:ad2s90: Make read_raw return spi_read's error code

[Matheus Tavares]

Previously, when spi_read returned an error code inside ad2s90_read_raw,
the code was ignored and IIO_VAL_INT was returned. This patch makes the
function return the error code returned by spi_read when it fails.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 drivers/staging/iio/resolver/ad2s90.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 59586947a936..ba55de29ef36 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -36,11 +36,12 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
 
 	mutex_lock(&st->lock);
 	ret = spi_read(st->sdev, st->rx, 2);
-	if (ret)
-		goto error_ret;
+	if (ret < 0) {
+		mutex_unlock(&st->lock);
+		return ret;
+	}
 	*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
 
-error_ret:
 	mutex_unlock(&st->lock);
 
 	return IIO_VAL_INT;
-- 
2.18.0

Re: [PATCH v3 1/6] staging:iio:ad2s90: Make read_raw return spi_read's error code

[Jonathan Cameron]

On Sat,  3 Nov 2018 19:49:43 -0300
Matheus Tavares <matheus.bernardino@usp.br> wrote:

> Previously, when spi_read returned an error code inside ad2s90_read_raw,
> the code was ignored and IIO_VAL_INT was returned. This patch makes the
> function return the error code returned by spi_read when it fails.
> 
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/resolver/ad2s90.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index 59586947a936..ba55de29ef36 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -36,11 +36,12 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&st->lock);
>  	ret = spi_read(st->sdev, st->rx, 2);
> -	if (ret)
> -		goto error_ret;
> +	if (ret < 0) {
> +		mutex_unlock(&st->lock);
> +		return ret;
> +	}
>  	*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
>  
> -error_ret:
>  	mutex_unlock(&st->lock);
>  
>  	return IIO_VAL_INT;

[PATCH v3 2/6] staging:iio:ad2s90: Make probe handle spi_setup failure

[Matheus Tavares]

Previously, ad2s90_probe ignored the return code from spi_setup, not
handling its possible failure. This patch makes ad2s90_probe check if
the code is an error code and, if so, do the following:

- Call dev_err with an appropriate error message.
- Return the spi_setup's error code.

Note: The 'return ret' statement could be out of the 'if' block, but
this whole block will be moved up in the function in the patch:
'staging:iio:ad2s90: Move device registration to the end of probe'.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 drivers/staging/iio/resolver/ad2s90.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index ba55de29ef36..4908c8a95fad 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -86,7 +86,12 @@ static int ad2s90_probe(struct spi_device *spi)
 	/* need 600ns between CS and the first falling edge of SCLK */
 	spi->max_speed_hz = 830000;
 	spi->mode = SPI_MODE_3;
-	spi_setup(spi);
+	ret = spi_setup(spi);
+
+	if (ret < 0) {
+		dev_err(&spi->dev, "spi_setup failed!\n");
+		return ret;
+	}
 
 	return 0;
 }
-- 
2.18.0

Re: [PATCH v3 2/6] staging:iio:ad2s90: Make probe handle spi_setup failure

[Jonathan Cameron]

On Sat,  3 Nov 2018 19:49:44 -0300
Matheus Tavares <matheus.bernardino@usp.br> wrote:

> Previously, ad2s90_probe ignored the return code from spi_setup, not
> handling its possible failure. This patch makes ad2s90_probe check if
> the code is an error code and, if so, do the following:
> 
> - Call dev_err with an appropriate error message.
> - Return the spi_setup's error code.
> 
> Note: The 'return ret' statement could be out of the 'if' block, but
> this whole block will be moved up in the function in the patch:
> 'staging:iio:ad2s90: Move device registration to the end of probe'.
> 
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
Very nicely presented patch.  Thanks.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/resolver/ad2s90.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index ba55de29ef36..4908c8a95fad 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -86,7 +86,12 @@ static int ad2s90_probe(struct spi_device *spi)
>  	/* need 600ns between CS and the first falling edge of SCLK */
>  	spi->max_speed_hz = 830000;
>  	spi->mode = SPI_MODE_3;
> -	spi_setup(spi);
> +	ret = spi_setup(spi);
> +
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "spi_setup failed!\n");
> +		return ret;
> +	}
>  
>  	return 0;
>  }

[PATCH v3 3/6] staging:iio:ad2s90: Remove always overwritten assignment

[Matheus Tavares]

This patch removes an initial assignment to the variable ret at probe,
that was always overwritten.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 drivers/staging/iio/resolver/ad2s90.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 4908c8a95fad..54ad85bd9dc6 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -62,7 +62,7 @@ static int ad2s90_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
 	struct ad2s90_state *st;
-	int ret = 0;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
-- 
2.18.0

Re: [PATCH v3 3/6] staging:iio:ad2s90: Remove always overwritten assignment

[Jonathan Cameron]

On Sat,  3 Nov 2018 19:49:45 -0300
Matheus Tavares <matheus.bernardino@usp.br> wrote:

> This patch removes an initial assignment to the variable ret at probe,
> that was always overwritten.
> 
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
Applied to the togreg branch of iio.git and pushed out as testing to
see if we are both wrong and there is something wrong with it that
build tests and static analysers can find!

Thanks,

Jonathan

> ---
>  drivers/staging/iio/resolver/ad2s90.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index 4908c8a95fad..54ad85bd9dc6 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -62,7 +62,7 @@ static int ad2s90_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
>  	struct ad2s90_state *st;
> -	int ret = 0;
> +	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>  	if (!indio_dev)

[PATCH v3 4/6] staging:iio:ad2s90: Move device registration to the end of probe

[Matheus Tavares]

Previously, devm_iio_device_register was being called before the
spi_setup call and the spi_device's max_speed_hz and mode assignments.
This could lead to a race condition since the driver was still being
set up after it was already made ready to use. To fix it, this patch
moves the device registration to the end of ad2s90_probe.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 drivers/staging/iio/resolver/ad2s90.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 54ad85bd9dc6..8f79cccf4814 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -79,10 +79,6 @@ static int ad2s90_probe(struct spi_device *spi)
 	indio_dev->num_channels = 1;
 	indio_dev->name = spi_get_device_id(spi)->name;
 
-	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
-	if (ret)
-		return ret;
-
 	/* need 600ns between CS and the first falling edge of SCLK */
 	spi->max_speed_hz = 830000;
 	spi->mode = SPI_MODE_3;
@@ -93,7 +89,7 @@ static int ad2s90_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	return 0;
+	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
 }
 
 static const struct spi_device_id ad2s90_id[] = {
-- 
2.18.0

Re: [PATCH v3 4/6] staging:iio:ad2s90: Move device registration to the end of probe

[Jonathan Cameron]

On Sat,  3 Nov 2018 19:49:46 -0300
Matheus Tavares <matheus.bernardino@usp.br> wrote:

> Previously, devm_iio_device_register was being called before the
> spi_setup call and the spi_device's max_speed_hz and mode assignments.
> This could lead to a race condition since the driver was still being
> set up after it was already made ready to use. To fix it, this patch
> moves the device registration to the end of ad2s90_probe.
> 
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
Applied.  This is definitely looking more like a 'modern' IIO driver.
All sorts of less than sensible things like this used to be done
when all of us were new to drivers.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/resolver/ad2s90.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index 54ad85bd9dc6..8f79cccf4814 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -79,10 +79,6 @@ static int ad2s90_probe(struct spi_device *spi)
>  	indio_dev->num_channels = 1;
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  
> -	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> -	if (ret)
> -		return ret;
> -
>  	/* need 600ns between CS and the first falling edge of SCLK */
>  	spi->max_speed_hz = 830000;
>  	spi->mode = SPI_MODE_3;
> @@ -93,7 +89,7 @@ static int ad2s90_probe(struct spi_device *spi)
>  		return ret;
>  	}
>  
> -	return 0;
> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>  }
>  
>  static const struct spi_device_id ad2s90_id[] = {

[PATCH v3 5/6] staging:iio:ad2s90: Add IIO_CHAN_INFO_SCALE to channel spec and read_raw

[Matheus Tavares]

This patch adds the IIO_CHAN_INFO_SCALE mask to ad2s90_chan and
implements the relative read behavior at ad2s90_read_raw.

Signed-off-by: Victor Colombo <victorcolombo@gmail.com>
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 drivers/staging/iio/resolver/ad2s90.c | 30 +++++++++++++++++++--------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 8f79cccf4814..9c168b7410d0 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -34,17 +34,29 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
 	int ret;
 	struct ad2s90_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&st->lock);
-	ret = spi_read(st->sdev, st->rx, 2);
-	if (ret < 0) {
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		/* 2 * Pi / 2^12 */
+		*val = 6283; /* mV */
+		*val2 = 12;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&st->lock);
+		ret = spi_read(st->sdev, st->rx, 2);
+		if (ret < 0) {
+			mutex_unlock(&st->lock);
+			return ret;
+		}
+		*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
+
 		mutex_unlock(&st->lock);
-		return ret;
-	}
-	*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
 
-	mutex_unlock(&st->lock);
+		return IIO_VAL_INT;
+	default:
+		break;
+	}
 
-	return IIO_VAL_INT;
+	return -EINVAL;
 }
 
 static const struct iio_info ad2s90_info = {
@@ -55,7 +67,7 @@ static const struct iio_chan_spec ad2s90_chan = {
 	.type = IIO_ANGL,
 	.indexed = 1,
 	.channel = 0,
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
 };
 
 static int ad2s90_probe(struct spi_device *spi)
-- 
2.18.0

Re: [PATCH v3 5/6] staging:iio:ad2s90: Add IIO_CHAN_INFO_SCALE to channel spec and read_raw

[Jonathan Cameron]

On Sat,  3 Nov 2018 19:49:47 -0300
Matheus Tavares <matheus.bernardino@usp.br> wrote:

> This patch adds the IIO_CHAN_INFO_SCALE mask to ad2s90_chan and
> implements the relative read behavior at ad2s90_read_raw.
> 
> Signed-off-by: Victor Colombo <victorcolombo@gmail.com>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
Hi Matheus,

Somewhere in the process, the authorship of this patch changed from
Victor to you (From:).  Given the sign off order I've assumed this
was be accident and put it back to Victor.

For reference
git commit --amend --author="Victor Colombo <victorcolombo@gmail.com>"

Whilst the patch was modified a fair bit, the fact you have left Victor
as the first sign-off implies you think it is still substantially
Victor's patch (I agree with that).

Anyhow, shout if you disagree as still time to change it before
I push this tree out as non rebasing (probably later this week).

Whilst things are only visible in testing I can change anything,
but once I push out as togreg, I am committing to that being a stable
platform for others to base their code on so can't fix things like this.

Applied.
Thanks,
Jonathan

> ---
>  drivers/staging/iio/resolver/ad2s90.c | 30 +++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index 8f79cccf4814..9c168b7410d0 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -34,17 +34,29 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
>  	int ret;
>  	struct ad2s90_state *st = iio_priv(indio_dev);
>  
> -	mutex_lock(&st->lock);
> -	ret = spi_read(st->sdev, st->rx, 2);
> -	if (ret < 0) {
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		/* 2 * Pi / 2^12 */
> +		*val = 6283; /* mV */
> +		*val2 = 12;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&st->lock);
> +		ret = spi_read(st->sdev, st->rx, 2);
> +		if (ret < 0) {
> +			mutex_unlock(&st->lock);
> +			return ret;
> +		}
> +		*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
> +
>  		mutex_unlock(&st->lock);
> -		return ret;
> -	}
> -	*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
>  
> -	mutex_unlock(&st->lock);
> +		return IIO_VAL_INT;
> +	default:
> +		break;
> +	}
>  
> -	return IIO_VAL_INT;
> +	return -EINVAL;
>  }
>  
>  static const struct iio_info ad2s90_info = {
> @@ -55,7 +67,7 @@ static const struct iio_chan_spec ad2s90_chan = {
>  	.type = IIO_ANGL,
>  	.indexed = 1,
>  	.channel = 0,
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>  };
>  
>  static int ad2s90_probe(struct spi_device *spi)

[PATCH v3 6/6] staging:iio:ad2s90: Check channel type at read_raw

[Matheus Tavares]

This patch adds a channel type check at the beginning of the
ad2s90_read_raw function. Since ad2s90 has only one channel, it just
checks if the given channel is the expected one and if not, return
-EINVAL.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 drivers/staging/iio/resolver/ad2s90.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 9c168b7410d0..3e257ac46f7a 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -34,6 +34,9 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
 	int ret;
 	struct ad2s90_state *st = iio_priv(indio_dev);
 
+	if (chan->type != IIO_ANGL)
+		return -EINVAL;
+
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
 		/* 2 * Pi / 2^12 */
-- 
2.18.0

Re: [PATCH v3 6/6] staging:iio:ad2s90: Check channel type at read_raw

[Jonathan Cameron]

On Sat,  3 Nov 2018 19:49:48 -0300
Matheus Tavares <matheus.bernardino@usp.br> wrote:

> This patch adds a channel type check at the beginning of the
> ad2s90_read_raw function. Since ad2s90 has only one channel, it just
> checks if the given channel is the expected one and if not, return
> -EINVAL.
> 
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>

Given you can't actually get here with another channel type by any
valid means, this is more a form of code as documentation than anything
else.  Still it does no harm and arguably does make it easier to read.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

A nice little clean up series.  Thanks!

Jonathan
> ---
>  drivers/staging/iio/resolver/ad2s90.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index 9c168b7410d0..3e257ac46f7a 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -34,6 +34,9 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
>  	int ret;
>  	struct ad2s90_state *st = iio_priv(indio_dev);
>  
> +	if (chan->type != IIO_ANGL)
> +		return -EINVAL;
> +
>  	switch (m) {
>  	case IIO_CHAN_INFO_SCALE:
>  		/* 2 * Pi / 2^12 */