| Message ID | 1499900458-2339-5-git-send-email-jasmin@anw.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/13/2017 02:00 AM, Jasmin J. wrote: > From: Jasmin Jessich <jasmin@anw.at> > > Fixed all: > WARNING: Block comments use * on subsequent lines Also multiline comments should be written like this: /* * Comment. */ Quickly looking this patch serie I noticed few other coding style mistakes. You should read kernel coding style documentation first, and then make changes according to doc. regards Antti
Hello Antti! > Quickly looking this patch serie I noticed few other coding style mistakes. > You should read kernel coding style documentation first, and then make > changes according to doc. In fact I used checkpatch.pl to find the issues and fixed them. All the patches are 100% checkpatch.pl tested and did not have one single error or warning. So please can you point me to those issues you mean. BR, Jasmin
On 07/13/2017 02:23 AM, Jasmin J. wrote: > Hello Antti! > >> Quickly looking this patch serie I noticed few other coding style mistakes. >> You should read kernel coding style documentation first, and then make >> changes according to doc. > In fact I used checkpatch.pl to find the issues and fixed them. All the patches > are 100% checkpatch.pl tested and did not have one single error or warning. > > So please can you point me to those issues you mean. Have you ever looked that coding style doc? Maybe better to start reading it first. Checkpatch is only a tool, it is nothing which makes 100% decision which is correct or not. Multi-line comment style is explained on section 8 on kernel coding style doc. Antti
Hello Antti!
> Have you ever looked that coding style doc?
Yes I read it several times already and used it in my daily work in my
previous company.
Beside the Multi-line comment style, which I will fix in a follow up,
you mentioned other issues.
Please can you tell me which one you mean, so that I can check the series
for those things.
BR,
Jasmin
On 07/13/2017 02:45 AM, Jasmin J. wrote: > Hello Antti! > >> Have you ever looked that coding style doc? > Yes I read it several times already and used it in my daily work in my > previous company. > > Beside the Multi-line comment style, which I will fix in a follow up, > you mentioned other issues. > Please can you tell me which one you mean, so that I can check the series > for those things. eh, OK, here short list from my head: * you fixed comments, but left //-comments * many cases where if (ret != 0), which generally should be written as if (ret). If you expect it is just error ret value, then prefer if (ret), but if ret has some other meaning like it returns number of bytes then if you expect 0-bytes returned (ret != 0) is also valid. * unnecessary looking line split like that: if (a & b) * logical continuous line split wrong (I think I have seen checkpatch reported that kind of mistakes, dunno why not now) if (a && b) == > if (a && b) Antti
On 07/13/2017 03:04 AM, Antti Palosaari wrote: > On 07/13/2017 02:45 AM, Jasmin J. wrote: >> Hello Antti! >> >>> Have you ever looked that coding style doc? >> Yes I read it several times already and used it in my daily work in my >> previous company. >> >> Beside the Multi-line comment style, which I will fix in a follow up, >> you mentioned other issues. >> Please can you tell me which one you mean, so that I can check the series >> for those things. > > eh, OK, here short list from my head: > * you fixed comments, but left //-comments > > * many cases where if (ret != 0), which generally should be written as > if (ret). If you expect it is just error ret value, then prefer if > (ret), but if ret has some other meaning like it returns number of bytes > then if you expect 0-bytes returned (ret != 0) is also valid. > > * unnecessary looking line split like that: > if (a > & b) > > * logical continuous line split wrong (I think I have seen checkpatch > reported that kind of mistakes, dunno why not now) > if (a > && b) > == > > if (a && > b) actually it reports, when run --strict mode: + if (a + && b) { + foo(a); + foo(b); + } + CHECK: Logical continuations should be on the previous line #11: FILE: drivers/media/usb/dvb-usb-v2/af9035.c:2135: + if (a + && b) { Antti
Hello Antti! > actually it reports, when run --strict mode I checked this once, but I thought this would be too aggressive changes. >> * many cases where if (ret != 0), which generally should be written as if >> (ret). If you expect it is just error ret value, then prefer if (ret), but >> if> ret has some other meaning like it returns number of bytes then if you >> expect 0-bytes returned (ret != 0) is also valid. In fact I did no real code changes to keep the impact as little as possible. But I agree fully with you and in my drivers I used always (ret) or (!ret). Although this has been changed in my new company when it comes to certified software ... . I will try also to compile with GCC 7.1.1, if I get one for my system. >> * unnecessary looking line split like that: >> if (a >> & b) I am sure I did this because of the 80 col limit, but I will look again. THX for your review and the valuable input. I will add you the receiver list next time. BR, Jasmin
diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb-core/dvb_ca_en50221.c index c0fd63a..317968b 100644 --- a/drivers/media/dvb-core/dvb_ca_en50221.c +++ b/drivers/media/dvb-core/dvb_ca_en50221.c @@ -343,7 +343,8 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private *ca, int slot) ca->slot_info[slot].da_irq_supported = 0; /* set the host link buffer size temporarily. it will be overwritten with the - * real negotiated size later. */ + * real negotiated size later. + */ ca->slot_info[slot].link_buf_size = 2; /* read the buffer size from the CAM */ @@ -797,9 +798,10 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, return ca->pub->write_data(ca->pub, slot, buf, bytes_write); /* it is possible we are dealing with a single buffer implementation, - thus if there is data available for read or if there is even a read - already in progress, we do nothing but awake the kernel thread to - process the data if necessary. */ + * thus if there is data available for read or if there is even a read + * already in progress, we do nothing but awake the kernel thread to + * process the data if necessary. + */ if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) goto exitnowrite; if (status & (STATUSREG_DA | STATUSREG_RE)) { @@ -899,8 +901,9 @@ static int dvb_ca_en50221_slot_shutdown(struct dvb_ca_private *ca, int slot) ca->pub->slot_shutdown(ca->pub, slot); ca->slot_info[slot].slot_state = DVB_CA_SLOTSTATE_NONE; - /* need to wake up all processes to check if they're now - trying to write to a defunct CAM */ + /* need to wake up all processes to check if they're now trying to + * write to a defunct CAM + */ wake_up_interruptible(&ca->wait_queue); dprintk("Slot %i shutdown\n", slot); @@ -1681,8 +1684,10 @@ static int dvb_ca_en50221_io_open(struct inode *inode, struct file *file) if (ca->slot_info[i].slot_state == DVB_CA_SLOTSTATE_RUNNING) { if (ca->slot_info[i].rx_buffer.data != NULL) { - /* it is safe to call this here without locks because - * ca->open == 0. Data is not read in this case */ + /* it is safe to call this here without locks + * because ca->open == 0. Data is not read in + * this case + */ dvb_ringbuffer_flush(&ca->slot_info[i].rx_buffer); } }