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 (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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
This patch removes an initial assignment to the variable ret at probe,
that was always overwritten.
Signed-off-by: Matheus Tavares <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Matheus Tavares <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, 26 Oct 2018 23:00:00 -0300
Matheus Tavares <[email protected]> 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 <[email protected]>
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;
On Fri, 26 Oct 2018 23:00:01 -0300
Matheus Tavares <[email protected]> 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, aborting the execution of the
> rest of the function.
>
> Signed-off-by: Matheus Tavares <[email protected]>
> ---
> 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;
> + }
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;
> }
On Fri, 26 Oct 2018 22:59:59 -0300
Matheus Tavares <[email protected]> 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
>
> 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 | 55 ++++++++++++++++++---------
> 1 file changed, 37 insertions(+), 18 deletions(-)
>
On Fri, 26 Oct 2018 23:00:04 -0300
Matheus Tavares <[email protected]> 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 <[email protected]>
> Signed-off-by: Matheus Tavares <[email protected]>
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.
> + 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)
On Sun, Oct 28, 2018 at 1:52 PM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 26 Oct 2018 22:59:59 -0300
> Matheus Tavares <[email protected]> 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
> >
> > 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 | 55 ++++++++++++++++++---------
> > 1 file changed, 37 insertions(+), 18 deletions(-)
> >
>
On 10/28/18 1:40 PM, Jonathan Cameron wrote:
> On Fri, 26 Oct 2018 23:00:00 -0300
> Matheus Tavares <[email protected]> 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 <[email protected]>
> 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.
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
>> + }
>> +
>> *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;
>
On 10/28/18 1:43 PM, Jonathan Cameron wrote:
> On Fri, 26 Oct 2018 23:00:01 -0300
> Matheus Tavares <[email protected]> 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, aborting the execution of the
>> rest of the function.
>>
>> Signed-off-by: Matheus Tavares <[email protected]>
>> ---
>> 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;
>> + }
> 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.
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;
>> }
On Fri, 2 Nov 2018 10:49:59 -0300
Matheus Tavares <[email protected]> wrote:
> On 10/28/18 1:40 PM, Jonathan Cameron wrote:
> > On Fri, 26 Oct 2018 23:00:00 -0300
> > Matheus Tavares <[email protected]> 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 <[email protected]>
> > 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.
>
>
> 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;
> }
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
>
>
> Matheus
>
>
> >> + }
> >> +
> >> *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;
> >
On Fri, 2 Nov 2018 10:59:06 -0300
Matheus Tavares <[email protected]> wrote:
> On 10/28/18 1:43 PM, Jonathan Cameron wrote:
> > On Fri, 26 Oct 2018 23:00:01 -0300
> > Matheus Tavares <[email protected]> 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, aborting the execution of the
> >> rest of the function.
> >>
> >> Signed-off-by: Matheus Tavares <[email protected]>
> >> ---
> >> 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;
> >> + }
> > 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.
>
>
> 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?
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;
> >> }
On Sun, Oct 28, 2018 at 1:50 PM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 26 Oct 2018 23:00:04 -0300
> Matheus Tavares <[email protected]> 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 <[email protected]>
> > Signed-off-by: Matheus Tavares <[email protected]>
>
> 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
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.
> 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.
>
> > + 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)
>
On Sat, 3 Nov 2018 13:04:04 -0300
Matheus Tavares Bernardino <[email protected]> wrote:
> On Sun, Oct 28, 2018 at 1:50 PM Jonathan Cameron <[email protected]> wrote:
> >
> > On Fri, 26 Oct 2018 23:00:04 -0300
> > Matheus Tavares <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Matheus Tavares <[email protected]>
> >
> > 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
>
> 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?
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.
>
> > 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.
> >
> > > + 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)
> >