iio: mxs-lradc: Remove useless check in read_raw

PATCH Superseded Marek Vasut 5 participants 18 messages Jul 2, 2013 – Jul 3, 2013

Subsystems (4)

Arm i.MX and Layerscape Staging IIO Staging - industrial io

lore

Share & tools

lore

Marek Vasut

author

2013-07-02 00:08 UTC #0 lore raw

PATCH 1/1 iio: mxs-lradc: Remove useless check in read_raw

The removed check in the read_raw implementation was always true, therefore remove it.

Signed-off-by Marek Vasut <[email protected]>

Cc Jonathan Cameron <[email protected]>

Cc Fabio Estevam <[email protected]>

---
 drivers/staging/iio/adc/mxs-lradc.c |    7 -------
 1 file changed, 7 deletions(-)

drivers/staging/iio/adc/mxs-lradc.c +0 -7

index d92c97a..c318eb6 100644
@@ -234,7 +234,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
 {
 	struct mxs_lradc *lradc = iio_priv(iio_dev);
 	int ret;
-	unsigned long mask;
 
 	if (m != IIO_CHAN_INFO_RAW)
 		return -EINVAL;

@@ -243,12 +242,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
 	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
 		return -EINVAL;
 
-	/* Validate the channel if it doesn't intersect with reserved chans. */
-	bitmap_set(&mask, chan->channel, 1);
-	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
-	if (ret)
-		return -EINVAL;
-
 	/*
 	 * See if there is no buffered operation in progess. If there is, simply
 	 * bail out. This can be improved to support both buffered and raw IO at

Signature · 1 line

1.7.10.4

Otavio Salvador

2013-07-02 03:38 UTC #1 lore raw

Quoted 6 lines · Marek Vasut

The removed check in the read_raw implementation was always true, therefore remove it.
Signed-off-by Marek Vasut <[email protected]>
Cc Jonathan Cameron <[email protected]>
Cc Fabio Estevam <[email protected]>

This fixes the driver for MX23 but why this code were add at first?

--
Otavio Salvador O.S. Systems
http://www.ossystems.com.br http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854 Mobile: +1 (347) 903-9750

Marek Vasut

author

2013-07-02 17:58 UTC #2 lore raw

Dear Otavio Salvador,

Quoted 9 lines · Marek Vasut

Quoted 6 lines
The removed check in the read_raw implementation was always true, therefore remove it.
Signed-off-by Marek Vasut <[email protected]>
Cc Jonathan Cameron <[email protected]>
Cc Fabio Estevam <[email protected]>
This fixes the driver for MX23 but why this code were add at first?

Obviously because I wrote the code incorrectly. I appologize for the trouble.

Jonathan, can we get this one into 3.10.1 maybe ?

Best regards,
Marek Vasut

Hector Palacios

2013-07-02 09:25 UTC #3 lore raw

In reply to Marek Vasut

Dear Marek,

Quoted 26 lines · Marek Vasut

The removed check in the read_raw implementation was always true, therefore remove it.

Signed-off-by Marek Vasut <[email protected]>

Cc Jonathan Cameron <[email protected]>

Cc Fabio Estevam <[email protected]>

---
  drivers/staging/iio/adc/mxs-lradc.c |    7 -------
  1 file changed, 7 deletions(-)

drivers/staging/iio/adc/mxs-lradc.c +0 -1

index d92c97a..c318eb6 100644
@@ -234,7 +234,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
  {
  	struct mxs_lradc *lradc = iio_priv(iio_dev);
  	int ret;
-	unsigned long mask;
 
  	if (m != IIO_CHAN_INFO_RAW)
  		return -EINVAL;

@@ -243,12 +242,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
  	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
  		return -EINVAL;

drivers/staging/iio/adc/mxs-lradc.c static int mxs_lradc_read_raw(struct iio_dev *iio_dev, · line 247

Quoted 10 lines

drivers/staging/iio/adc/mxs-lradc.c +0 -6

 
-	/* Validate the channel if it doesn't intersect with reserved chans. */
-	bitmap_set(&mask, chan->channel, 1);
-	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
-	if (ret)
-		return -EINVAL;
-
  	/*
  	 * See if there is no buffered operation in progess. If there is, simply
  	 * bail out. This can be improved to support both buffered and raw IO at

By "always true" you mean that it always entered the if and exited with -EINVAL, right?
I just started to look at this driver on my MX28 platform and this check was always returning -EINVAL. With this patch I can now read the channels. Was this what was happening to you as well?

Thank you,

Signature · 1 line

Héctor Palacios

Alexandre Belloni

2013-07-02 11:43 UTC #4 lore raw

In reply to Marek Vasut

Dear Marek,

I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?

Regards,

Quoted 36 lines · Marek Vasut

The removed check in the read_raw implementation was always true, therefore remove it.

Signed-off-by Marek Vasut <[email protected]>

Cc Jonathan Cameron <[email protected]>

Cc Fabio Estevam <[email protected]>

---
 drivers/staging/iio/adc/mxs-lradc.c |    7 -------
 1 file changed, 7 deletions(-)

drivers/staging/iio/adc/mxs-lradc.c +0 -7

index d92c97a..c318eb6 100644
@@ -234,7 +234,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
 {
 	struct mxs_lradc *lradc = iio_priv(iio_dev);
 	int ret;
-	unsigned long mask;
 
 	if (m != IIO_CHAN_INFO_RAW)
 		return -EINVAL;

@@ -243,12 +242,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
 	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
 		return -EINVAL;
 
-	/* Validate the channel if it doesn't intersect with reserved chans. */
-	bitmap_set(&mask, chan->channel, 1);
-	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
-	if (ret)
-		return -EINVAL;
-
 	/*
 	 * See if there is no buffered operation in progess. If there is, simply
 	 * bail out. This can be improved to support both buffered and raw IO at

Signature · 3 lines

Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Otavio Salvador

2013-07-02 11:53 UTC #5 lore raw

Quoted 2 lines · Alexandre Belloni

I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?

I had this, but in MX23.

It seems like a race as if I put some prints in this code area it works sometimes.

--
Otavio Salvador O.S. Systems
http://www.ossystems.com.br http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854 Mobile: +1 (347) 903-9750

Marek Vasut

author

2013-07-02 12:23 UTC #6 lore raw

In reply to Alexandre Belloni

Dear Alexandre Belloni,

Quoted 4 lines

Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?

The check is just redundant, it's not a bug.

Quoted 46 lines

Regards,
Quoted 43 lines · Marek Vasut
The removed check in the read_raw implementation was always true, therefore remove it.
Signed-off-by Marek Vasut <[email protected]>
Cc Jonathan Cameron <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 drivers/staging/iio/adc/mxs-lradc.c |    7 -------
 1 file changed, 7 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/iio/adc/mxs-lradc.c index d92c97a..c318eb6 100644
drivers/staging/iio/adc/mxs-lradc.c +0 -7
@@ -234,7 +234,6 @@ static int mxs_lradc_read_raw(struct iio_dev
 *iio_dev,
 
 {
 
 	struct mxs_lradc *lradc = iio_priv(iio_dev);
 	int ret;
 
-	unsigned long mask;
 
 	if (m != IIO_CHAN_INFO_RAW)
 
 		return -EINVAL;
@@ -243,12 +242,6 @@ static int mxs_lradc_read_raw(struct iio_dev
 *iio_dev,
 
 	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
 
 		return -EINVAL;
 
-	/* Validate the channel if it doesn't intersect with reserved chans. */
-	bitmap_set(&mask, chan->channel, 1);
-	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
-	if (ret)
-		return -EINVAL;
-

Quoted 5 lines

drivers/staging/iio/adc/mxs-lradc.c
Quoted 5 lines
drivers/staging/iio/adc/mxs-lradc.c +0 -0
 	/*
 
 	 * See if there is no buffered operation in progess. If there is,
 	 simply * bail out. This can be improved to support both buffered and
 	 raw IO at

Best regards,
Marek Vasut

Alexandre Belloni

2013-07-02 12:49 UTC #7 lore raw

Quoted 9 lines · Marek Vasut

Dear Alexandre Belloni,
Quoted 4 lines
Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.

Ok, that's what I understood first but then got confused by reports of it solving a bug.

Thanks !

Signature · 3 lines

Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Marek Vasut

author

2013-07-02 16:13 UTC #8 lore raw

Dear Alexandre Belloni,

Quoted 5 lines · Marek Vasut

Quoted 4 lines
Dear Alexandre Belloni,
Quoted 2 lines
Dear Marek,

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.
Ok, that's what I understood first but then got confused by reports of it solving a bug.

It cannot solve a thing. If it does, then we have a problem.

What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?

Best regards,
Marek Vasut

Otavio Salvador

2013-07-02 16:37 UTC #9 lore raw

Quoted 12 lines · Marek Vasut

Dear Alexandre Belloni,
Quoted 10 lines · Marek Vasut
Quoted 8 lines
Dear Alexandre Belloni,
Quoted 4 lines
Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.

Quoted 7 lines

Quoted 2 lines
Ok, that's what I understood first but then got confused by reports of it solving a bug.
It cannot solve a thing. If it does, then we have a problem.
What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?

As I said this code sometimes work. If you put a printf before this call it sometimes work. So I think we have a race somewhere.

When I were debugging this I found that when it works we have 10 active channels, it seems.

--
Otavio Salvador O.S. Systems
http://www.ossystems.com.br http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854 Mobile: +1 (347) 903-9750

Marek Vasut

author

2013-07-02 17:37 UTC #10 lore raw

Dear Otavio Salvador,

Quoted 18 lines · Marek Vasut

Quoted 17 lines
Dear Alexandre Belloni,
Quoted 5 lines · Marek Vasut
Quoted 4 lines
Dear Alexandre Belloni,
Quoted 2 lines
Dear Marek,
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.
Ok, that's what I understood first but then got confused by reports of it solving a bug.
It cannot solve a thing. If it does, then we have a problem.

Quoted 8 lines

Quoted 2 lines
What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?
As I said this code sometimes work. If you put a printf before this call it sometimes work. So I think we have a race somewhere.
When I were debugging this I found that when it works we have 10 active channels, it seems.

Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload() will fail iff buffered operation is in progress. Or what do you mean by having "10 active channels"?

Best regards,
Marek Vasut

Otavio Salvador

2013-07-02 17:42 UTC #11 lore raw

Quoted 26 lines · Marek Vasut

Dear Otavio Salvador,
Quoted 24 lines · Marek Vasut
Quoted 12 lines
Dear Alexandre Belloni,
Quoted 10 lines · Marek Vasut
Quoted 8 lines
Dear Alexandre Belloni,
Quoted 4 lines
Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.
Quoted 7 lines
Quoted 2 lines
Ok, that's what I understood first but then got confused by reports of it solving a bug.
It cannot solve a thing. If it does, then we have a problem.
What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?
As I said this code sometimes work. If you put a printf before this call it sometimes work. So I think we have a race somewhere.

Quoted 6 lines

Quoted 2 lines
When I were debugging this I found that when it works we have 10 active channels, it seems.
Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload() will fail iff buffered operation is in progress. Or what do you mean by having "10 active channels"?

The read_raw returned invalid. The onehot always returns false and I have 10 as mask comparing return.

--
Otavio Salvador O.S. Systems
http://www.ossystems.com.br http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854 Mobile: +1 (347) 903-9750

Marek Vasut

author

2013-07-02 17:55 UTC #12 lore raw

Dear Otavio Salvador,

Quoted 30 lines · Marek Vasut

Quoted 29 lines
Dear Otavio Salvador,
Quoted 26 lines · Marek Vasut
Quoted 12 lines
Dear Alexandre Belloni,
Quoted 10 lines · Marek Vasut
Quoted 8 lines
Dear Alexandre Belloni,
Quoted 4 lines
Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.
Quoted 7 lines
Quoted 2 lines
Ok, that's what I understood first but then got confused by reports of it solving a bug.
It cannot solve a thing. If it does, then we have a problem.
What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?
As I said this code sometimes work. If you put a printf before this call it sometimes work. So I think we have a race somewhere.
When I were debugging this I found that when it works we have 10 active channels, it seems.

Quoted 6 lines

Quoted 3 lines
Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload() will fail iff buffered operation is in progress. Or what do you mean by having "10 active channels"?
The read_raw returned invalid. The onehot always returns false and I have 10 as mask comparing return.

Looking at the code.

the bitmap_set() will set (1 << chan->channel) into "mask" var.

The iio_validate_scan_mask_onehot is implemented like this:

688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
689 const unsigned long *mask)
690 {
691 return bitmap_weight(mask, indio_dev->masklength) == 1;
692 }

So if mask _always_ has exactly one bit set, the iio_validate_scan_mask_onehot will always return 1. Aaaand ... now I see why it fixes a bug for you.

if (ret)
return -EINVAL;

This stuff above will always be if (1) so will always return -EINVAL. Dang!

Best regards,
Marek Vasut

Hector Palacios

2013-07-03 09:25 UTC #13 lore raw

Dear Marek,

Quoted 51 lines · Marek Vasut

Dear Otavio Salvador,
Quoted 30 lines · Marek Vasut
Quoted 29 lines
Dear Otavio Salvador,
Quoted 26 lines · Marek Vasut
Quoted 12 lines
Dear Alexandre Belloni,
Quoted 10 lines · Marek Vasut
Quoted 8 lines
Dear Alexandre Belloni,
Quoted 4 lines
Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.
Quoted 7 lines
Quoted 2 lines
Ok, that's what I understood first but then got confused by reports of it solving a bug.
It cannot solve a thing. If it does, then we have a problem.
What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?
As I said this code sometimes work. If you put a printf before this call it sometimes work. So I think we have a race somewhere.
When I were debugging this I found that when it works we have 10 active channels, it seems.
Quoted 6 lines
Quoted 3 lines
Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload() will fail iff buffered operation is in progress. Or what do you mean by having "10 active channels"?
The read_raw returned invalid. The onehot always returns false and I have 10 as mask comparing return.
Looking at the code.
the bitmap_set() will set (1 << chan->channel) into "mask" var.
The iio_validate_scan_mask_onehot is implemented like this:
688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
689 const unsigned long *mask)
690 {
691 return bitmap_weight(mask, indio_dev->masklength) == 1;
692 }

Quoted 7 lines

So if mask _always_ has exactly one bit set, the iio_validate_scan_mask_onehot will always return 1. Aaaand ... now I see why it fixes a bug for you.
if (ret)
return -EINVAL;
This stuff above will always be if (1) so will always return -EINVAL. Dang!

Yes, this was exactly what was happening on my MX28 when trying to read one channel.
Now, after the different comments I'm confused, was this a bug that your patch solved or was I doing something wrong?

Best regards,
--
Hector Palacios

Marek Vasut

author

2013-07-03 11:38 UTC #14 lore raw

Dear Hector Palacios,

Quoted 6 lines

Dear Marek,
Quoted 3 lines · Marek Vasut
Dear Otavio Salvador,
Quoted 1 line
On Tue, Jul 2, 2013 at 2:36 PM, Marek Vasut <marex-

Quoted 3 lines · [email protected]> wrote:

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Dear Otavio Salvador,
Quoted 1 line
On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex-

Quoted 51 lines · [email protected]> wrote:

Quoted 51 lines
Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 12 lines
Dear Alexandre Belloni,
Quoted 10 lines · Marek Vasut
Quoted 8 lines
Dear Alexandre Belloni,
Quoted 4 lines
Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.
Quoted 7 lines
Quoted 2 lines
Ok, that's what I understood first but then got confused by reports of it solving a bug.
It cannot solve a thing. If it does, then we have a problem.
What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?
As I said this code sometimes work. If you put a printf before this call it sometimes work. So I think we have a race somewhere.
When I were debugging this I found that when it works we have 10 active channels, it seems.
Quoted 6 lines
Quoted 3 lines
Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload() will fail iff buffered operation is in progress. Or what do you mean by having "10 active channels"?
The read_raw returned invalid. The onehot always returns false and I have 10 as mask comparing return.
Looking at the code.
the bitmap_set() will set (1 << chan->channel) into "mask" var.
The iio_validate_scan_mask_onehot is implemented like this:
688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
689 const unsigned long *mask)
690 {
691 return bitmap_weight(mask, indio_dev->masklength) == 1;
692 }
So if mask _always_ has exactly one bit set, the iio_validate_scan_mask_onehot will always return 1. Aaaand ... now I see why it fixes a bug for you.
if (ret)

Quoted 8 lines

Quoted 4 lines
return -EINVAL;
This stuff above will always be if (1) so will always return -EINVAL. Dang!
Yes, this was exactly what was happening on my MX28 when trying to read one channel. Now, after the different comments I'm confused, was this a bug that your patch solved or was I doing something wrong?

This patch accidentally (in completely unintended way) solved a bug, yes. But the main idea was to just remove redundant code.

Best regards,
Marek Vasut

Hector Palacios

2013-07-03 13:59 UTC #15 lore raw

Dear Marek,

Quoted 69 lines · Marek Vasut

Dear Hector Palacios,
Quoted 6 lines
Dear Marek,
Quoted 3 lines · Marek Vasut
Dear Otavio Salvador,
Quoted 1 line
On Tue, Jul 2, 2013 at 2:36 PM, Marek Vasut <marex-
Quoted 3 lines · [email protected]> wrote:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Dear Otavio Salvador,
Quoted 1 line
On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex-
Quoted 56 lines · [email protected]> wrote:
Quoted 55 lines
Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 12 lines
Dear Alexandre Belloni,
Quoted 10 lines · Marek Vasut
Quoted 8 lines
Dear Alexandre Belloni,
Quoted 4 lines
Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.
Quoted 7 lines
Quoted 2 lines
Ok, that's what I understood first but then got confused by reports of it solving a bug.
It cannot solve a thing. If it does, then we have a problem.
What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?
As I said this code sometimes work. If you put a printf before this call it sometimes work. So I think we have a race somewhere.
When I were debugging this I found that when it works we have 10 active channels, it seems.
Quoted 6 lines
Quoted 3 lines
Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload() will fail iff buffered operation is in progress. Or what do you mean by having "10 active channels"?
The read_raw returned invalid. The onehot always returns false and I have 10 as mask comparing return.
Looking at the code.
the bitmap_set() will set (1 << chan->channel) into "mask" var.
The iio_validate_scan_mask_onehot is implemented like this:
688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
689 const unsigned long *mask)
690 {
691 return bitmap_weight(mask, indio_dev->masklength) == 1;
692 }
So if mask _always_ has exactly one bit set, the iio_validate_scan_mask_onehot will always return 1. Aaaand ... now I see why it fixes a bug for you.
if (ret)
   return -EINVAL;
This stuff above will always be if (1) so will always return -EINVAL. Dang!

Quoted 6 lines

Quoted 3 lines
Yes, this was exactly what was happening on my MX28 when trying to read one channel. Now, after the different comments I'm confused, was this a bug that your patch solved or was I doing something wrong?
This patch accidentally (in completely unintended way) solved a bug, yes. But the main idea was to just remove redundant code.

Maybe you should then amend the commit message to make clear that it solves a bug.

Best regards,
--
Hector Palacios

Jonathan Cameron

maintainer

2013-07-03 19:26 UTC #16 lore raw

Quoted 72 lines · Hector Palacios

Dear Marek,
Quoted 69 lines · Marek Vasut
Dear Hector Palacios,
Quoted 6 lines
Dear Marek,
Quoted 3 lines · Marek Vasut
Dear Otavio Salvador,
Quoted 1 line
On Tue, Jul 2, 2013 at 2:36 PM, Marek Vasut <marex-
Quoted 3 lines · [email protected]> wrote:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Dear Otavio Salvador,
Quoted 1 line
On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex-
Quoted 56 lines · [email protected]> wrote:
Quoted 55 lines
Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 12 lines
Dear Alexandre Belloni,
Quoted 10 lines · Marek Vasut
Quoted 8 lines
Dear Alexandre Belloni,
Quoted 4 lines
Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.
Quoted 7 lines
Quoted 2 lines
Ok, that's what I understood first but then got confused by reports of it solving a bug.
It cannot solve a thing. If it does, then we have a problem.
What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?
As I said this code sometimes work. If you put a printf before this call it sometimes work. So I think we have a race somewhere.
When I were debugging this I found that when it works we have 10 active channels, it seems.
Quoted 6 lines
Quoted 3 lines
Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload() will fail iff buffered operation is in progress. Or what do you mean by having "10 active channels"?
The read_raw returned invalid. The onehot always returns false and I have 10 as mask comparing return.
Looking at the code.
the bitmap_set() will set (1 << chan->channel) into "mask" var.
The iio_validate_scan_mask_onehot is implemented like this:
688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
689 const unsigned long *mask)
690 {
691 return bitmap_weight(mask, indio_dev->masklength) == 1;
692 }
So if mask _always_ has exactly one bit set, the iio_validate_scan_mask_onehot will always return 1. Aaaand ... now I see why it fixes a bug for you.
if (ret)
   return -EINVAL;
This stuff above will always be if (1) so will always return -EINVAL. Dang!

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Yes, this was exactly what was happening on my MX28 when trying to read one channel. Now, after the different comments I'm confused, was this a bug that your patch solved or was I doing something wrong?
This patch accidentally (in completely unintended way) solved a bug, yes. But the main idea was to just remove redundant code.
Maybe you should then amend the commit message to make clear that it solves a bug.

Definitely.

I'll apply a version that mentions the bug in the commit comment to a branch that will hopefully be applied just after rc1 then can be picked up by stable as appropriate.

Jonathan

Quoted 8 lines

Best regards,

Signature · 5 lines

Hector Palacios
-- 
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alexandre Belloni

2013-07-02 17:19 UTC #17 lore raw

In reply to Marek Vasut

Hi,

Quoted 12 lines · Marek Vasut

Dear Alexandre Belloni,
Quoted 10 lines · Marek Vasut
Quoted 8 lines
Dear Alexandre Belloni,
Quoted 4 lines
Dear Marek,
I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know how to reproduce it ?
The check is just redundant, it's not a bug.

Quoted 8 lines

Quoted 2 lines
Ok, that's what I understood first but then got confused by reports of it solving a bug.
It cannot solve a thing. If it does, then we have a problem.
What kind of bug do you see ? How can I replicate it ? Can you send me a testcase?

It doesn't solve anything on my side, I never saw any issue with that code. I was surprised by those reports though:

http://marc.info/?l=linux-iio&m=137275710622191&w=2
http://marc.info/?l=linux-iio&m=137273629716559&w=2

Hector and Otavio seem to be the ones with an issue ;)

Signature · 3 lines

Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com