* [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style @ 2018-10-16 21:09 Giuliano Belinassi 2018-10-16 21:59 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Giuliano Belinassi @ 2018-10-16 21:09 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman Cc: linux-iio, devel, linux-kernel, kernel-usp 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style 2018-10-16 21:09 [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style Giuliano Belinassi @ 2018-10-16 21:59 ` Joe Perches 2018-10-16 22:48 ` Giuliano Belinassi 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2018-10-16 21:59 UTC (permalink / raw) To: Giuliano Belinassi, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman Cc: linux-iio, devel, linux-kernel, kernel-usp 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: ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style 2018-10-16 21:59 ` Joe Perches @ 2018-10-16 22:48 ` Giuliano Belinassi 2018-10-16 23:07 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Giuliano Belinassi @ 2018-10-16 22:48 UTC (permalink / raw) To: joe Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio, devel, linux-kernel, kernel-usp [-- Attachment #1: Type: text/plain, Size: 5545 bytes --] 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? On Tue, Oct 16, 2018 at 6:59 PM Joe Perches <joe@perches.com> wrote: > 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: > > > [-- Attachment #2: Type: text/html, Size: 7995 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style 2018-10-16 22:48 ` Giuliano Belinassi @ 2018-10-16 23:07 ` Joe Perches 2018-10-16 23:29 ` Giuliano Augusto Faulin Belinassi 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2018-10-16 23:07 UTC (permalink / raw) To: Giuliano Belinassi Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio, devel, linux-kernel (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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style 2018-10-16 23:07 ` Joe Perches @ 2018-10-16 23:29 ` Giuliano Augusto Faulin Belinassi 2018-10-16 23:57 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Giuliano Augusto Faulin Belinassi @ 2018-10-16 23:29 UTC (permalink / raw) To: joe Cc: giuliano.belinassi, lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel >(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? > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style 2018-10-16 23:29 ` Giuliano Augusto Faulin Belinassi @ 2018-10-16 23:57 ` Joe Perches 0 siblings, 0 replies; 7+ messages in thread From: Joe Perches @ 2018-10-16 23:57 UTC (permalink / raw) To: Giuliano Augusto Faulin Belinassi Cc: giuliano.belinassi, lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style @ 2018-10-16 23:57 ` Joe Perches 0 siblings, 0 replies; 7+ messages in thread From: Joe Perches @ 2018-10-16 23:57 UTC (permalink / raw) To: Giuliano Augusto Faulin Belinassi Cc: giuliano.belinassi, lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-16 23:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-16 21:09 [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style Giuliano Belinassi 2018-10-16 21:59 ` Joe Perches 2018-10-16 22:48 ` Giuliano Belinassi 2018-10-16 23:07 ` Joe Perches 2018-10-16 23:29 ` Giuliano Augusto Faulin Belinassi 2018-10-16 23:57 ` Joe Perches 2018-10-16 23:57 ` Joe Perches
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.