| Message ID | 20181016210950.cmx4r4pb2yg5ofpp@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: iio: ad7280a: Lines should not end with a '(' - style | expand |
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:
(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?
>(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? > >
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.
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),
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(-)