[PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style

[PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style

[Giuliano Belinassi]

A number of macro calls cause a checkpatch issue:

    "Lines should not end with a '('"

This was fixed by moving the first '(' token to the line of the first
argument.

Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
 drivers/staging/iio/adc/ad7280a.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index 58420dcb406d..f7df987412d7 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -725,8 +725,8 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 		} else {
 			if (((channels[i] >> 11) & 0xFFF) >= st->aux_threshhigh)
 				iio_push_event(indio_dev,
-					       IIO_UNMOD_EVENT_CODE(
-							IIO_TEMP,
+					       IIO_UNMOD_EVENT_CODE
+							(IIO_TEMP,
 							0,
 							IIO_EV_TYPE_THRESH,
 							IIO_EV_DIR_RISING),
@@ -734,8 +734,8 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 			else if (((channels[i] >> 11) & 0xFFF) <=
 				st->aux_threshlow)
 				iio_push_event(indio_dev,
-					       IIO_UNMOD_EVENT_CODE(
-							IIO_TEMP,
+					       IIO_UNMOD_EVENT_CODE
+							(IIO_TEMP,
 							0,
 							IIO_EV_TYPE_THRESH,
 							IIO_EV_DIR_FALLING),
-- 
2.19.1

Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style

[Joe Perches]

On Tue, 2018-10-16 at 18:09 -0300, Giuliano Belinassi wrote:
> A number of macro calls cause a checkpatch issue:
> 
>     "Lines should not end with a '('"
> 
> This was fixed by moving the first '(' token to the line of the first
> argument.

Please try to make the code more readable instead of
following mindless checkpatch messages.

For instance, this could be shorter, simpler, smaller
object code size, and easier to humans to read as:
---
 drivers/staging/iio/adc/ad7280a.c | 68 +++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 38 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index 58420dcb406d..a69ae76b5616 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -701,46 +701,38 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 		goto out;
 
 	for (i = 0; i < st->scan_cnt; i++) {
-		if (((channels[i] >> 23) & 0xF) <= AD7280A_CELL_VOLTAGE_6) {
-			if (((channels[i] >> 11) & 0xFFF) >=
-				st->cell_threshhigh)
-				iio_push_event(indio_dev,
-					       IIO_EVENT_CODE(IIO_VOLTAGE,
-							1,
-							0,
-							IIO_EV_DIR_RISING,
-							IIO_EV_TYPE_THRESH,
-							0, 0, 0),
-					       iio_get_time_ns(indio_dev));
-			else if (((channels[i] >> 11) & 0xFFF) <=
-				st->cell_threshlow)
-				iio_push_event(indio_dev,
-					       IIO_EVENT_CODE(IIO_VOLTAGE,
-							1,
-							0,
-							IIO_EV_DIR_FALLING,
-							IIO_EV_TYPE_THRESH,
-							0, 0, 0),
-					       iio_get_time_ns(indio_dev));
+		unsigned int voltage = (channels[i] >> 23) & 0xF;
+		unsigned int val = (channels[i] >> 11) & 0xFFF;
+		u64 code = 0;
+
+		if (voltage <= AD7280A_CELL_VOLTAGE_6) {
+			if (val >= st->cell_threshhigh) {
+				code = IIO_EVENT_CODE(IIO_VOLTAGE, 1, 0,
+						      IIO_EV_DIR_RISING,
+						      IIO_EV_TYPE_THRESH,
+						      0, 0, 0);
+			} else if (val <= st->cell_threshlow) {
+				code = IIO_EVENT_CODE(IIO_VOLTAGE, 1, 0,
+						      IIO_EV_DIR_FALLING,
+						      IIO_EV_TYPE_THRESH,
+						      0, 0, 0);
+			} else {
+				continue;
+			}
 		} else {
-			if (((channels[i] >> 11) & 0xFFF) >= st->aux_threshhigh)
-				iio_push_event(indio_dev,
-					       IIO_UNMOD_EVENT_CODE(
-							IIO_TEMP,
-							0,
-							IIO_EV_TYPE_THRESH,
-							IIO_EV_DIR_RISING),
-					       iio_get_time_ns(indio_dev));
-			else if (((channels[i] >> 11) & 0xFFF) <=
-				st->aux_threshlow)
-				iio_push_event(indio_dev,
-					       IIO_UNMOD_EVENT_CODE(
-							IIO_TEMP,
-							0,
-							IIO_EV_TYPE_THRESH,
-							IIO_EV_DIR_FALLING),
-					       iio_get_time_ns(indio_dev));
+			if (val >= st->aux_threshhigh) {
+				code = IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
+							    IIO_EV_TYPE_THRESH,
+							    IIO_EV_DIR_RISING);
+			} else if (val <= st->aux_threshlow) {
+				code = IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
+							    IIO_EV_TYPE_THRESH,
+							    IIO_EV_DIR_FALLING);
+			} else {
+				continue;
+			}
 		}
+		iio_push_event(indio_dev, code, iio_get_time_ns(indio_dev));
 	}
 
 out:

Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style

[Joe Perches]

(There is a linux-usp@googlegroups.com mailing list
that bounces when I reply, so I removed it from the
cc list)

On Tue, 2018-10-16 at 19:48 -0300, Giuliano Belinassi wrote:
> Hello,
>     Thank you for your review :-).
>     Sorry, but I am a newbie on this, and now I am confused about my next
> step. Should I make a v2 based on your changes, or do you want to submit
> your changes?

I wrote that to encourage you to do more than
what checkpatch says.

I just moved code around and reduced duplication.

There are many similar opportunities for code
refactoring in staging.

You could test what I wrote, add a good commit
message with a subject like:

[PATCH V2] staging: iio: ad7280a: Refactor <functionname>

with a commit message that describes the changes and
perhaps shows the object size difference using

$ size <old> <new>

Maybe add a Suggested-by: tag if it pleases you, but
what I did is trivial and I think it's unnecessary.

Are you doing this for a class assignment?

Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style

[Giuliano Augusto Faulin Belinassi]

>(There is a linux-usp@googlegroups.com mailing list
>that bounces when I reply, so I removed it from the
>cc list)

Sorry. I think this may be because my HTML mode in gmail was enabled.

> I wrote that to encourage you to do more than
> what checkpatch says.

> I just moved code around and reduced duplication.

> There are many similar opportunities for code
> refactoring in staging.

Thank you :-) I will put effort to improve these points.

> You could test what I wrote, add a good commit
> message with a subject like:
>
> [PATCH V2] staging: iio: ad7280a: Refactor <functionname>
>
> with a commit message that describes the changes and
> perhaps shows the object size difference using
>
> $ size <old> <new>

I will do that. :-)

> Are you doing this for a class assignment?
Yes, I am. I am into a group that aims to contribute to opensource
projects and we chose the Linux kernel. Our mentor suggested us to
contribute to the linux-iio

Thank you
On Tue, Oct 16, 2018 at 8:08 PM Joe Perches <joe@perches.com> wrote:
>
> (There is a linux-usp@googlegroups.com mailing list
> that bounces when I reply, so I removed it from the
> cc list)
>
> On Tue, 2018-10-16 at 19:48 -0300, Giuliano Belinassi wrote:
> > Hello,
> >     Thank you for your review :-).
> >     Sorry, but I am a newbie on this, and now I am confused about my next
> > step. Should I make a v2 based on your changes, or do you want to submit
> > your changes?
>
> I wrote that to encourage you to do more than
> what checkpatch says.
>
> I just moved code around and reduced duplication.
>
> There are many similar opportunities for code
> refactoring in staging.
>
> You could test what I wrote, add a good commit
> message with a subject like:
>
> [PATCH V2] staging: iio: ad7280a: Refactor <functionname>
>
> with a commit message that describes the changes and
> perhaps shows the object size difference using
>
> $ size <old> <new>
>
> Maybe add a Suggested-by: tag if it pleases you, but
> what I did is trivial and I think it's unnecessary.
>
> Are you doing this for a class assignment?
>
>

Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style

[Joe Perches]

On Tue, 2018-10-16 at 20:29 -0300, Giuliano Augusto Faulin Belinassi
wrote:
> > (There is a linux-usp@googlegroups.com mailing list
> > that bounces when I reply, so I removed it from the
> > cc list)
> 
> Sorry. I think this may be because my HTML mode in gmail was enabled.

No, it is because that group address is private
and rejects posts from non-members.

> > Are you doing this for a class assignment?
> Yes, I am. I am into a group that aims to contribute to opensource
> projects and we chose the Linux kernel. Our mentor suggested us to
> contribute to the linux-iio

That's fine but please tell your mentor to try
to vet the proposed patches within your internal
groups before sending them on to lkml.

Perhaps add the kernel-newbies list to the vetting.