Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Jasmin J.]

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

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Antti Palosaari]

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
--
http://palosaari.fi/

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Antti Palosaari]

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

--
http://palosaari.fi/

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Jasmin J.]

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

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Antti Palosaari]

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


--
http://palosaari.fi/

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Jasmin J.]

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

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Antti Palosaari]

On 07/13/2017 02:00 AM, Jasmin J. wrote:

From: Jasmin Jessich 

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

--
http://palosaari.fi/

[PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Jasmin J.]

From: Jasmin Jessich 

Fixed all:
  WARNING: Block comments use * on subsequent lines

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

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(>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(>slot_info[i].rx_buffer);
}
}
-- 
2.7.4