Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue
On Wednesday 01 July 2015 01:32 PM, Sudip Mukherjee wrote: On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote: snip No, now this will introduce a new checkpatch warning that else is not required after return. why did you introduce this else? I did this so that the code is more readable and understandable, I checked and checkpatch didn't call this out , so its clean. Otherwise the above code looks like this if(i) return 1; return 0; you should update your tree. virtpci folder has been deleted from unisys driver. As you are using an old tree, maybe that explains why checkpatch is not giving the error. This is from linux-stable branch and I updated it just yesterday, so looks like the folders still there regards sudip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue
On Wed, Jul 01, 2015 at 02:04:48PM +0530, Sohny Thomas wrote: On Wednesday 01 July 2015 01:32 PM, Sudip Mukherjee wrote: On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote: snip This is from linux-stable branch and I updated it just yesterday, so looks like the folders still there if you are sending patches for staging then you should work with staging-testing tree. regards sudip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue
i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE,
scsi.wwnn, NULL);
- if (i) {
+ if (i)
return 1;
- }
- return 0;
+ else
+ return 0;
No, now this will introduce a new checkpatch warning that else is not
required after return. why did you introduce this else?
I did this so that the code is more readable and understandable, I checked
and
checkpatch didn't call this out , so its clean.
Otherwise the above code looks like this
if(i)
return 1;
return 0;
That looks fine.
I haven't looked at the code in detail. Is it normal that the return
values seem to be 0 1 and -1? Which values represent success and which
represent an error? It is nicer to have the errors under if and success
as a direct return at the end.
Here in this driver directory, return 1 means SUCCESS and return 0 means FAILURE
So you mean my code change is fine?
julia
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue
On Wednesday 01 July 2015 02:45 PM, Sudip Mukherjee wrote: On Wed, Jul 01, 2015 at 02:04:48PM +0530, Sohny Thomas wrote: On Wednesday 01 July 2015 01:32 PM, Sudip Mukherjee wrote: On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote: snip This is from linux-stable branch and I updated it just yesterday, so looks like the folders still there if you are sending patches for staging then you should work with staging-testing tree. Oh Ok, its not there in linux-next tree , so Ignore this patch . Thanks for the review regards sudip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue
On Wed, 1 Jul 2015, Sohny Thomas wrote:
i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE,
scsi.wwnn, NULL);
-if (i) {
+if (i)
return 1;
-}
-return 0;
+else
+return 0;
No, now this will introduce a new checkpatch warning that else is not
required after return. why did you introduce this else?
I did this so that the code is more readable and understandable, I checked
and
checkpatch didn't call this out , so its clean.
Otherwise the above code looks like this
if(i)
return 1;
return 0;
That looks fine.
I haven't looked at the code in detail. Is it normal that the return
values seem to be 0 1 and -1? Which values represent success and which
represent an error? It is nicer to have the errors under if and success
as a direct return at the end.
Here in this driver directory, return 1 means SUCCESS and return 0 means
FAILURE
What is -1?
So you mean my code change is fine?
I think it would be best to have the success case that is not under an if.
So if (!i)
return 0;
return 1;
I guess some day the driver would need more normal error codes?
julia
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue
On Wed, Jul 01, 2015 at 03:05:45AM +0530, Sohny Thomas wrote:
FIX 2 unnecessary braces found by checkpatch.pl
Signed-off-by: Sohny Thomas sohnytho...@zoho.com
---
drivers/staging/unisys/virtpci/virtpci.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/unisys/virtpci/virtpci.c
b/drivers/staging/unisys/virtpci/virtpci.c
index d5ad017..f3674de 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -190,9 +190,10 @@ static int write_vbus_chp_info(struct
spar_vbus_channel_protocol *chan,
return -1;
off = sizeof(struct channel_header) + chan-hdr_info.chp_info_offset;
- if (chan-hdr_info.chp_info_offset == 0) {
+
+ if (chan-hdr_info.chp_info_offset == 0)
return -1;
- }
+
why you are inserting new line here?
memcpy(((u8 *)(chan)) + off, info, sizeof(*info));
return 0;
}
@@ -484,10 +485,10 @@ static int delete_vhba(struct del_virt_guestpart
*delparams)
i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE,
scsi.wwnn, NULL);
- if (i) {
+ if (i)
return 1;
- }
- return 0;
+ else
+ return 0;
No, now this will introduce a new checkpatch warning that else is not
required after return. why did you introduce this else?
regards
sudip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue
On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote: snip No, now this will introduce a new checkpatch warning that else is not required after return. why did you introduce this else? I did this so that the code is more readable and understandable, I checked and checkpatch didn't call this out , so its clean. Otherwise the above code looks like this if(i) return 1; return 0; you should update your tree. virtpci folder has been deleted from unisys driver. As you are using an old tree, maybe that explains why checkpatch is not giving the error. regards sudip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue
On Wed, 1 Jul 2015, Sohny Thomas wrote:
Thanks for review, my answers inline
On 01-07-2015 12:27, Sudip Mukherjee wrote:
On Wed, Jul 01, 2015 at 03:05:45AM +0530, Sohny Thomas wrote:
FIX 2 unnecessary braces found by checkpatch.pl
Signed-off-by: Sohny Thomas sohnytho...@zoho.com
---
drivers/staging/unisys/virtpci/virtpci.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/unisys/virtpci/virtpci.c
b/drivers/staging/unisys/virtpci/virtpci.c
index d5ad017..f3674de 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -190,9 +190,10 @@ static int write_vbus_chp_info(struct
spar_vbus_channel_protocol *chan,
return -1;
off = sizeof(struct channel_header) +
chan-hdr_info.chp_info_offset;
- if (chan-hdr_info.chp_info_offset == 0) {
+
+ if (chan-hdr_info.chp_info_offset == 0)
return -1;
- }
+
why you are inserting new line here?
I did it so that its readable, will remove it if not required
memcpy(((u8 *)(chan)) + off, info, sizeof(*info));
return 0;
}
@@ -484,10 +485,10 @@ static int delete_vhba(struct del_virt_guestpart
*delparams)
i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE,
scsi.wwnn, NULL);
- if (i) {
+ if (i)
return 1;
- }
- return 0;
+ else
+ return 0;
No, now this will introduce a new checkpatch warning that else is not
required after return. why did you introduce this else?
I did this so that the code is more readable and understandable, I checked and
checkpatch didn't call this out , so its clean.
Otherwise the above code looks like this
if(i)
return 1;
return 0;
That looks fine.
I haven't looked at the code in detail. Is it normal that the return
values seem to be 0 1 and -1? Which values represent success and which
represent an error? It is nicer to have the errors under if and success
as a direct return at the end.
julia
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue
Thanks for review, my answers inline
On 01-07-2015 12:27, Sudip Mukherjee wrote:
On Wed, Jul 01, 2015 at 03:05:45AM +0530, Sohny Thomas wrote:
FIX 2 unnecessary braces found by checkpatch.pl
Signed-off-by: Sohny Thomas sohnytho...@zoho.com
---
drivers/staging/unisys/virtpci/virtpci.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/unisys/virtpci/virtpci.c
b/drivers/staging/unisys/virtpci/virtpci.c
index d5ad017..f3674de 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -190,9 +190,10 @@ static int write_vbus_chp_info(struct
spar_vbus_channel_protocol *chan,
return -1;
off = sizeof(struct channel_header) + chan-hdr_info.chp_info_offset;
- if (chan-hdr_info.chp_info_offset == 0) {
+
+ if (chan-hdr_info.chp_info_offset == 0)
return -1;
- }
+
why you are inserting new line here?
I did it so that its readable, will remove it if not required
memcpy(((u8 *)(chan)) + off, info, sizeof(*info));
return 0;
}
@@ -484,10 +485,10 @@ static int delete_vhba(struct del_virt_guestpart
*delparams)
i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE,
scsi.wwnn, NULL);
- if (i) {
+ if (i)
return 1;
- }
- return 0;
+ else
+ return 0;
No, now this will introduce a new checkpatch warning that else is not
required after return. why did you introduce this else?
I did this so that the code is more readable and understandable, I
checked and checkpatch didn't call this out , so its clean.
Otherwise the above code looks like this
if(i)
return 1;
return 0;
regards
sudip
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
