[PATCH 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.

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 | 55 ++++++++++++++++++---------
1 file changed, 37 insertions(+), 18 deletions(-)

--
2.18.0

[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.b...@usp.br>
---
drivers/staging/iio/resolver/ad2s90.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 59586947a936..11fac9f90148 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
struct ad2s90_state *st = iio_priv(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

[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, aborting the execution of the
rest of the function.

Signed-off-by: Matheus Tavares <matheus.b...@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 11fac9f90148..d6a42e3f1bd8 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -88,7 +88,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

[Matheus Tavares]

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

Signed-off-by: Matheus Tavares <matheus.b...@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 d6a42e3f1bd8..c20d37dc065a 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -64,7 +64,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

[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.b...@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 c20d37dc065a..b4a6a89c11b0 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -81,10 +81,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;

@@ -95,7 +91,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

[Matheus Tavares]

From: Victor Colombo <victor...@gmail.com>

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 <victor...@gmail.com>
---
drivers/staging/iio/resolver/ad2s90.c | 32 ++++++++++++++++++---------
1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index b4a6a89c11b0..52b656875ed1 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -34,19 +34,31 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
int ret;

struct ad2s90_state *st = iio_priv(indio_dev);

- mutex_lock(&st->lock);
+ switch (m) {
+ case IIO_CHAN_INFO_SCALE:
+ /* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
+ *val = 0;
+ *val2 = 1534355;
+ return IIO_VAL_INT_PLUS_NANO;
+ 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);

- 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 IIO_VAL_INT;
+ default:
+ break;
+ }

- return IIO_VAL_INT;
+ return -EINVAL;
}

static const struct iio_info ad2s90_info = {
@@ -57,7 +69,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

[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.b...@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 52b656875ed1..24002042a5c5 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 - 1) ~= 0.001534355 */

--
2.18.0

[Dan Carpenter]

On Thu, Oct 25, 2018 at 09:45:11PM -0300, Matheus Tavares wrote:
> From: Victor Colombo <victor...@gmail.com>
>
> 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 <victor...@gmail.com>
> ---

You should be adding your S-o-B here as well because the patch is
passing through your hands.

regards,
dan carpenter

[Matheus Tavares Bernardino]

Thanks for the review! I'll be sending a v2 with my S-o-B there.

> regards,
> dan carpenter
>
> --
> You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+...@googlegroups.com.
> To post to this group, send email to kerne...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20181026100422.lvz2avowd6ddix54%40mwanda.
> For more options, visit https://groups.google.com/d/optout.

[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 v2:
- Added my S-o-B in patch 5.

[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.b...@usp.br>
---

drivers/staging/iio/resolver/ad2s90.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 59586947a936..11fac9f90148 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,

struct ad2s90_state *st = iio_priv(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);

[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, aborting the execution of the
rest of the function.

Signed-off-by: Matheus Tavares <matheus.b...@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 11fac9f90148..d6a42e3f1bd8 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -88,7 +88,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) {

[Matheus Tavares]

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

Signed-off-by: Matheus Tavares <matheus.b...@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 d6a42e3f1bd8..c20d37dc065a 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c

[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.b...@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 c20d37dc065a..b4a6a89c11b0 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c

@@ -81,10 +81,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;

[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 <victor...@gmail.com>

Signed-off-by: Matheus Tavares <matheus.b...@usp.br>
---

drivers/staging/iio/resolver/ad2s90.c | 32 ++++++++++++++++++---------
1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index b4a6a89c11b0..52b656875ed1 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -34,19 +34,31 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
int ret;

struct ad2s90_state *st = iio_priv(indio_dev);

- mutex_lock(&st->lock);
+ switch (m) {
+ case IIO_CHAN_INFO_SCALE:
+ /* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
+ *val = 0;
+ *val2 = 1534355;
+ return IIO_VAL_INT_PLUS_NANO;
+ 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);

- 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 IIO_VAL_INT;
+ default:
+ break;
+ }

- return IIO_VAL_INT;
+ return -EINVAL;
}

static const struct iio_info ad2s90_info = {
@@ -57,7 +69,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

[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.b...@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 52b656875ed1..24002042a5c5 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 - 1) ~= 0.001534355 */

--
2.18.0

[Jonathan Cameron]

On Fri, 26 Oct 2018 23:00:00 -0300
Matheus Tavares <matheus.b...@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.b...@usp.br>

Hi Matheus,

One quick process note is that it takes people a while to get around to reviewing
a series, so whilst it's tempting to very quickly send out a fix the moment
someone points out something that needs fixing, it is perhaps better to wait
at least a few days to see if you can pick up a few more reviews before you
do a V2.

A few comments on this one inline. I think it can be done 'slightly'
(and I mean only slightly) nicer than the version you have. Result is the
same though.

Thanks,

Jonathan

> ---
> drivers/staging/iio/resolver/ad2s90.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index 59586947a936..11fac9f90148 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
> struct ad2s90_state *st = iio_priv(indio_dev);
>
> mutex_lock(&st->lock);
> +

Unconnected change. I'm not against the change in principle but please
group white space tidying up in it's own patch.

> ret = spi_read(st->sdev, st->rx, 2);
> - if (ret)
> - goto error_ret;
> + if (ret < 0) {
> + mutex_unlock(&st->lock);
> + return ret;

I'd actually prefer to keep the return path the same as before as then
it is easy (if the function gets more complex in future) to be sure
that all paths unlock the mutex.

> + }
> +
> *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
>
> -error_ret:
> mutex_unlock(&st->lock);
>
> return IIO_VAL_INT;

The 'standard' if slightly nasty way of doing this is:

return ret ? ret : IIO_VAL_INT;

[Jonathan Cameron]

I would have reordered this first to be before the iio_device_register call.
The reason being that it would avoid this comment.

Drop the return ret out of the block above and return ret unconditionally.

I don't mind too much as I know this is moving later, but I only know that
because of the earlier discussion ;) Few reviewers read the whole patch
set before responding to the early patches - it's just too much like hard
work. So if you can do things in an order that minimizes standard responses
then that's great.

Patch is fine though - could be solved by a comment in the intro that
says the code in question will move in patch X.

Jonathan
>
> return 0;
> }

[Jonathan Cameron]

On Fri, 26 Oct 2018 23:00:04 -0300
Matheus Tavares <matheus.b...@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 <victor...@gmail.com>
> Signed-off-by: Matheus Tavares <matheus.b...@usp.br>

Hi,

A suggestion inline. This is a common case that we have infrastructure
to simplify. + I think your scale factor is very slightly wrong.

Jonathan

> ---
> drivers/staging/iio/resolver/ad2s90.c | 32 ++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index b4a6a89c11b0..52b656875ed1 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -34,19 +34,31 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
> int ret;
> struct ad2s90_state *st = iio_priv(indio_dev);
>
> - mutex_lock(&st->lock);
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + /* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> + *val = 0;
> + *val2 = 1534355;

Definitely 2^12 - 1? That's a bit unusual if true as it would imply
that 2^12 - 1 and 0 were the same value.

Imagine a smaller version with on 2^2 bits so 0, 1, 2, 3
Values of each are

0, M_PI/2, M_PI, 3*M_PI/2

So the multiplier is 2*M_PI/(2^2) not 2*M_PI/(2^2 - 1)
1/2 vs 2/3 * M_PI

Now this is a very common case so we have the return type
IIO_VAL_FRACTIONAL_LOG2 to give a more obvious and potentially
more accurate representation.

[Jonathan Cameron]

On Fri, 26 Oct 2018 22:59:59 -0300
Matheus Tavares <matheus.b...@usp.br> wrote:

> 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.

Thanks,

A good series in general. A few suggested improvements.
If I haven't commented on a patch, usually it means I'm happy with it
and will pick it up with the rest of the series.

Jonathan

[Matheus Tavares Bernardino]

On Sun, Oct 28, 2018 at 1:52 PM Jonathan Cameron <ji...@kernel.org> wrote:
>
> On Fri, 26 Oct 2018 22:59:59 -0300
> Matheus Tavares <matheus.b...@usp.br> wrote:
>
> > 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.
> Thanks,
>
> A good series in general. A few suggested improvements.
> If I haven't commented on a patch, usually it means I'm happy with it
> and will pick it up with the rest of the series.
>
> Jonathan
>

Thanks for the review, Jonathan. We will address the necessary changes in v3!

Matheus

[Matheus Tavares]

Ok, got it! But then, in patch 5, when we add the switch for
IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW, should I keep the goto and
label inside the switch case? I mean, should it be something like this:


    switch (m) {
    case IIO_CHAN_INFO_SCALE:
        ... // Does not use mutex
    case IIO_CHAN_INFO_RAW:
        mutex_lock(&st->lock);

        ret = spi_read(st->sdev, st->rx, 2);

        if (ret)
            goto error_ret;

        *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);

error_ret:
        mutex_unlock(&st->lock);

        return ret ? ret : IIO_VAL_INT;

    default:
        break;
    }


Matheus

[Matheus Tavares]

Ok! As a newcomer I'm not sure about the right way to add this comment.
Should I add it to the patch message or after the --- ? Also, how do I
reference patch X? Just giving the number of the patch in this series?

An alternative to adding the comment would be to drop the return ret,
and reinsert it in the next patch. Dosen't seem so good, to me, but
would avoid the problem you presented. What would be better?


Matheus.


>
> Jonathan
>>
>> return 0;
>> }

[Jonathan Cameron]

On Fri, 2 Nov 2018 10:49:59 -0300

This is was of those ugly signs that perhaps a given block of code
should be factored out as a separate function. It does feel like
overkill here though given how short the ugly bit will be ;)

So now I get why you did the refactor in the first place (I
hadn't made the connection).

I think on balance your first answer was the best one given
this is going to be deeper nested. If the function gets
more complex then this block should factored out anyway once the switch
statement is there then we go back to the simple exit path.

Thanks,

Jonathan

[Jonathan Cameron]

On Fri, 2 Nov 2018 10:59:06 -0300

For this sort of comment I would put it in the main description. It wants
to go in the git log for anyone who is looking at this patch in isolation
sometime in the future.

The stuff below the --- is usually version change things and other stuff
we don't want in the log.

>
> An alternative to adding the comment would be to drop the return ret,
> and reinsert it in the next patch. Dosen't seem so good, to me, but
> would avoid the problem you presented. What would be better?

Comment would be nicer for this one I think rather than the noise in the
actual code. Both would be acceptable though so which ever you
prefer.

Jonathan

>
>
> Matheus.
>
>
> >
> > Jonathan
> >>
> >> return 0;
> >> }

[Matheus Tavares Bernardino]

Oh, that makes a lot of sense! We used 2^12 - 1 here based on driver
drivers/iio/resolver/ad2s1200.c, whose resolution is also 12 bits, as
the ad2s90.c. Do you think this section is, perhaps, wrong on
ad2s1200.c too, or maybe there is some difference between these two
drivers that I didn't catch regarding the resolution?

Matheus.

[Jonathan Cameron]

Certainly seems likely to be wrong. Difference is tiny so no one
probably ever noticed :(

Feel free to post a patch fixing that one as well!

Thanks,

Jonathan

[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

[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.b...@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

[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.b...@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

[Matheus Tavares]

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

Signed-off-by: Matheus Tavares <matheus.b...@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)

[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.b...@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)

[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 <victor...@gmail.com>

Signed-off-by: Matheus Tavares <matheus.b...@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

[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.b...@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

[Jonathan Cameron]

On Sat, 3 Nov 2018 19:49:43 -0300
Matheus Tavares <matheus.b...@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.b...@usp.br>

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

Thanks,

Jonathan

[Jonathan Cameron]

On Sat, 3 Nov 2018 19:49:44 -0300
Matheus Tavares <matheus.b...@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.b...@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

[Jonathan Cameron]

On Sat, 3 Nov 2018 19:49:45 -0300
Matheus Tavares <matheus.b...@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.b...@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

[Jonathan Cameron]

On Sat, 3 Nov 2018 19:49:46 -0300
Matheus Tavares <matheus.b...@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.b...@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

[Jonathan Cameron]

On Sat, 3 Nov 2018 19:49:47 -0300
Matheus Tavares <matheus.b...@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 <victor...@gmail.com>
> Signed-off-by: Matheus Tavares <matheus.b...@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 <victor...@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

[Jonathan Cameron]

On Sat, 3 Nov 2018 19:49:48 -0300
Matheus Tavares <matheus.b...@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.b...@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