Staging:iio:adc:add space around '-'

PATCH In review Bhumika Goyal 5 participants 11 messages Jan 15, 2016 – Jan 30, 2016

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

Bhumika Goyal

author

2016-01-15 19:42 UTC #0 lore raw

cover Staging:iio:adc:add space around '-'

These patches adds space around '-' operator.Found using checkpatch.pl.

Bhumika Goyal (2):
staging:iio:adc:added space around '-'
staging:iio:adc:add space around '-'

 drivers/staging/iio/adc/ad7192.c  | 2 +-
 drivers/staging/iio/adc/ad7280a.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Signature · 1 line

1.9.1

Bhumika Goyal

author

2016-01-15 19:43 UTC #1 lore raw

PATCH 1/2 staging:iio:adc:added space around '-'

This patch adds apace around '-' operator.Found using checkpatch.pl

Signed-off-by Bhumika Goyal <[email protected]>

---
 drivers/staging/iio/adc/ad7280a.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/iio/adc/ad7280a.c +2 -2

index f45ebed..0c73bce 100644
@@ -744,14 +744,14 @@ out:
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
-		in_voltage-voltage_thresh_low_value,
+		in_voltage - voltage_thresh_low_value,
 		S_IRUGO | S_IWUSR,
 		ad7280_read_channel_config,
 		ad7280_write_channel_config,
 		AD7280A_CELL_UNDERVOLTAGE);
 
 static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
-		in_voltage-voltage_thresh_high_value,
+		in_voltage - voltage_thresh_high_value,
 		S_IRUGO | S_IWUSR,
 		ad7280_read_channel_config,
 		ad7280_write_channel_config,

Signature · 1 line

1.9.1

Thanks for patch. But when sending cleanup patches like this please make sure that you a) understand what the code does and how your change affects it and b) as a bare minimum of testing perform a compile test, if possible also do functional testing.

The patch as it is, is neither semantically nor syntactically correct. As an exercise please make sure you understand why.

Same for the second patch.

- Lars

Dan Carpenter

2016-01-20 14:22 UTC #3 lore raw

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

Quoted 21 lines · Lars-Peter Clausen

Quoted 12 lines · Bhumika Goyal
This patch adds apace around '-' operator.Found using checkpatch.pl
Signed-off-by Bhumika Goyal <[email protected]>
---
 drivers/staging/iio/adc/ad7280a.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/iio/adc/ad7280a.c +0 -0
index f45ebed..0c73bce 100644
drivers/staging/iio/adc/ad7280a.c out: · line 747
Quoted 5 lines
drivers/staging/iio/adc/ad7280a.c +1 -1
@@ -744,14 +744,14 @@ out:
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
-		in_voltage-voltage_thresh_low_value,
+		in_voltage - voltage_thresh_low_value,
Hi,

Quoted 7 lines

Thanks for patch. But when sending cleanup patches like this please make sure that you a) understand what the code does and how your change affects it and b) as a bare minimum of testing perform a compile test, if possible also do functional testing.
The patch as it is, is neither semantically nor syntactically correct. As an exercise please make sure you understand why.

Ugh!

It took me a long time to figure out the bug in this patch... Why does that filename have a mix of dashes and underscores? Too late to fix it now... :/

regards,
dan carpenter

Jonathan Cameron

maintainer

2016-01-24 16:36 UTC #4 lore raw

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

Quoted 27 lines · Dan Carpenter

Quoted 26 lines · Lars-Peter Clausen
Quoted 12 lines · Bhumika Goyal
This patch adds apace around '-' operator.Found using checkpatch.pl
Signed-off-by Bhumika Goyal <[email protected]>
---
 drivers/staging/iio/adc/ad7280a.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/iio/adc/ad7280a.c +0 -0
index f45ebed..0c73bce 100644
drivers/staging/iio/adc/ad7280a.c out: · line 747
Quoted 5 lines
drivers/staging/iio/adc/ad7280a.c +1 -1
@@ -744,14 +744,14 @@ out:
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
-		in_voltage-voltage_thresh_low_value,
+		in_voltage - voltage_thresh_low_value,
Hi,
Thanks for patch. But when sending cleanup patches like this please make sure that you a) understand what the code does and how your change affects it and b) as a bare minimum of testing perform a compile test, if possible also do functional testing.

Quoted 9 lines

Quoted 2 lines
The patch as it is, is neither semantically nor syntactically correct. As an exercise please make sure you understand why.
Ugh!
It took me a long time to figure out the bug in this patch... Why does that filename have a mix of dashes and underscores? Too late to fix it now... :/

Very deliberately. The - is indicating it is a differential channel! Literally A minus B.

It's an awfully compact representation for maths ;)
This is obscured partly in this case as it's specifying an attribute shared by a set of differential channels so it's the generalization of
in_voltage0-voltage1_thresh_low_value
which does begin to slightly stretch the argument that it is nice and clear ;(

Jonathan

Quoted 8 lines

regards,
dan carpenter
--
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

Lars-Peter Clausen

2016-01-24 17:51 UTC #5 lore raw

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

Quoted 40 lines · Jonathan Cameron

Quoted 27 lines · Dan Carpenter
Quoted 26 lines · Lars-Peter Clausen
Quoted 12 lines · Bhumika Goyal
This patch adds apace around '-' operator.Found using checkpatch.pl
Signed-off-by Bhumika Goyal <[email protected]>
---
 drivers/staging/iio/adc/ad7280a.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/iio/adc/ad7280a.c +0 -0
index f45ebed..0c73bce 100644
drivers/staging/iio/adc/ad7280a.c out: · line 747
Quoted 5 lines
drivers/staging/iio/adc/ad7280a.c +1 -1
@@ -744,14 +744,14 @@ out:
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
-		in_voltage-voltage_thresh_low_value,
+		in_voltage - voltage_thresh_low_value,
Hi,
Thanks for patch. But when sending cleanup patches like this please make sure that you a) understand what the code does and how your change affects it and b) as a bare minimum of testing perform a compile test, if possible also do functional testing.
Quoted 9 lines
Quoted 2 lines
The patch as it is, is neither semantically nor syntactically correct. As an exercise please make sure you understand why.
Ugh!
It took me a long time to figure out the bug in this patch... Why does that filename have a mix of dashes and underscores? Too late to fix it now... :/
Very deliberately. The - is indicating it is a differential channel! Literally A minus B.

Quoted 7 lines

It's an awfully compact representation for maths ;)
This is obscured partly in this case as it's specifying an attribute shared by a set of differential channels so it's the generalization of
in_voltage0-voltage1_thresh_low_value
which does begin to slightly stretch the argument that it is nice and clear ;(

One thing we should maybe take a look at is making it explicit that this is a string so it does not get picked up by checkpatch.

Jonathan Cameron

maintainer

2016-01-30 14:12 UTC #6 lore raw

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

Quoted 49 lines · Lars-Peter Clausen

Quoted 40 lines · Jonathan Cameron
Quoted 27 lines · Dan Carpenter
Quoted 26 lines · Lars-Peter Clausen
Quoted 12 lines · Bhumika Goyal
This patch adds apace around '-' operator.Found using checkpatch.pl
Signed-off-by Bhumika Goyal <[email protected]>
---
 drivers/staging/iio/adc/ad7280a.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/iio/adc/ad7280a.c +0 -0
index f45ebed..0c73bce 100644
drivers/staging/iio/adc/ad7280a.c out: · line 747
Quoted 5 lines
drivers/staging/iio/adc/ad7280a.c +1 -1
@@ -744,14 +744,14 @@ out:
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
-		in_voltage-voltage_thresh_low_value,
+		in_voltage - voltage_thresh_low_value,
Hi,
Thanks for patch. But when sending cleanup patches like this please make sure that you a) understand what the code does and how your change affects it and b) as a bare minimum of testing perform a compile test, if possible also do functional testing.
Quoted 9 lines
Quoted 2 lines
The patch as it is, is neither semantically nor syntactically correct. As an exercise please make sure you understand why.
Ugh!
It took me a long time to figure out the bug in this patch... Why does that filename have a mix of dashes and underscores? Too late to fix it now... :/
Very deliberately. The - is indicating it is a differential channel! Literally A minus B.
Quoted 7 lines
It's an awfully compact representation for maths ;)
This is obscured partly in this case as it's specifying an attribute shared by a set of differential channels so it's the generalization of
in_voltage0-voltage1_thresh_low_value
which does begin to slightly stretch the argument that it is nice and clear ;(

Quoted 2 lines

One thing we should maybe take a look at is making it explicit that this is a string so it does not get picked up by checkpatch.

Make sense. Patches welcome :)

Quoted 6 lines

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

Dan Carpenter

2016-01-30 15:12 UTC #7 lore raw

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

We could make checkpatch.pl not complain if the line says checkpatch: on it. It would look like this.

- in_voltage-voltage_thresh_low_value,
+ in_voltage-voltage_thresh_low_value, /* checkpatch: not math */

I suppose I could have made the explanation longer since the it won't complain about the 80 character limit... What do you guys think?

regards,
dan carpenter

Lars-Peter Clausen

2016-01-30 15:15 UTC #8 lore raw

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

Quoted 8 lines · Dan Carpenter

We could make checkpatch.pl not complain if the line says checkpatch: on it. It would look like this.
(quoted diff) +1 -1
-               in_voltage-voltage_thresh_low_value,
+               in_voltage-voltage_thresh_low_value, /* checkpatch: not math */
I suppose I could have made the explanation longer since the it won't complain about the 80 character limit... What do you guys think?

We could add it as a temporary way to silence the checker. But it feels a bit ugly, there is really no reason why this shouldn't be a string, other than that the current device attr macros don't support that.

- Lars

Joe Perches

2016-01-30 17:02 UTC #9 lore raw

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

In reply to Dan Carpenter

Quoted 10 lines · Dan Carpenter

We could make checkpatch.pl not complain if the line says checkpatch:
on
it.  It would look like this.
-               in_voltage-voltage_thresh_low_value,
+               in_voltage-voltage_thresh_low_value, /* checkpatch:
not math */
I suppose I could have made the explanation longer since the it won't complain about the 80 character limit...  What do yo/u guys think?

Maybe use a more generic thing like the checkpatch type

in_voltage-voltage_thresh_low_value, /* checkpatch-SPACING */

Even so, it might uglify checkpatch code a lot to check something like this per-line or per-block.

And that likely would have to be per line in the code as checkpatch couldn't see when a patch block addition occurs outside the scope of a comment.

I suppose inside checkpatch the "sub report {" function could be extended to look at the specific $rawline being tested for any "checkpatch" comment and if so, test if it's the specific $type.

Bhumika Goyal

author

2016-01-15 19:43 UTC #10 lore raw

PATCH 2/2 staging:iio:adc:add space around '-'

In reply to Bhumika Goyal

This patch adds space around '-' operator.Found using checkpatch.pl

Signed-off-by Bhumika Goyal <[email protected]>

---
 drivers/staging/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/iio/adc/ad7192.c +1 -1

index 9221103..54db0f0 100644
@@ -376,7 +376,7 @@ ad7192_show_scale_available(struct device *dev,
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
-			     in_voltage-voltage_scale_available,
+			     in_voltage - voltage_scale_available,
 			     S_IRUGO, ad7192_show_scale_available, NULL, 0);
 
 static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO,

Signature · 1 line

1.9.1