From: Maxime Rossi Bellom mrossibellom@gmail.com
Split line before funtion argument.
Error was reported by checkpatch.pl as WARNING: line over 80 characters
Signed-off-by: Maxime Rossi Bellom mrossibellom@gmail.com --- drivers/staging/greybus/fw-management.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c index 3cd6cf0..d88117e 100644 --- a/drivers/staging/greybus/fw-management.c +++ b/drivers/staging/greybus/fw-management.c @@ -516,7 +516,8 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd, */ fw_mgmt->mode_switch_started = true;
- ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf); + ret = gb_interface_request_mode_switch( + fw_mgmt->connection->intf); if (ret) { dev_err(fw_mgmt->parent, "Mode-switch failed: %d\n", ret);
On Fri, Jan 27, 2017 at 11:11:21AM +0100, mrossibellom@gmail.com wrote:
From: Maxime Rossi Bellom mrossibellom@gmail.com
Split line before funtion argument.
Error was reported by checkpatch.pl as WARNING: line over 80 characters
Signed-off-by: Maxime Rossi Bellom mrossibellom@gmail.com
drivers/staging/greybus/fw-management.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c index 3cd6cf0..d88117e 100644 --- a/drivers/staging/greybus/fw-management.c +++ b/drivers/staging/greybus/fw-management.c @@ -516,7 +516,8 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd, */ fw_mgmt->mode_switch_started = true;
ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);
ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);
That's horrid looking, please don't make code look worse, this is fine as-is.
thanks,
greg k-h
On Fri, Jan 27, 2017 at 11:11:21AM +0100, mrossibellom@gmail.com wrote:
From: Maxime Rossi Bellom mrossibellom@gmail.com
Split line before funtion argument.
Error was reported by checkpatch.pl as WARNING: line over 80 characters
Signed-off-by: Maxime Rossi Bellom mrossibellom@gmail.com
drivers/staging/greybus/fw-management.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c index 3cd6cf0..d88117e 100644 --- a/drivers/staging/greybus/fw-management.c +++ b/drivers/staging/greybus/fw-management.c @@ -516,7 +516,8 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd, */ fw_mgmt->mode_switch_started = true;
ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);
ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);
Another option would be to change the name from gb_interface_request_mode_switch() to gb_get_mode_switch() or something along those lines. It requires understanding the code to say what a good name is.
regards, dan carpenter
On Fri, Jan 27, 2017 at 03:29:30PM +0300, Dan Carpenter wrote:
On Fri, Jan 27, 2017 at 11:11:21AM +0100, mrossibellom@gmail.com wrote:
From: Maxime Rossi Bellom mrossibellom@gmail.com
Split line before funtion argument.
Error was reported by checkpatch.pl as WARNING: line over 80 characters
Signed-off-by: Maxime Rossi Bellom mrossibellom@gmail.com
drivers/staging/greybus/fw-management.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c index 3cd6cf0..d88117e 100644 --- a/drivers/staging/greybus/fw-management.c +++ b/drivers/staging/greybus/fw-management.c @@ -516,7 +516,8 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd, */ fw_mgmt->mode_switch_started = true;
ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);
ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);Another option would be to change the name from gb_interface_request_mode_switch() to gb_get_mode_switch() or something along those lines. It requires understanding the code to say what a good name is.
Indeed, and please do not rename this function.
Adding a temporary variable for the interface pointer would be a better way to deal with this if anyone is really bothered by this long line.
In fact, it may even be possible to just use connection->intf here in this case.
But I'd just leave it as is.
Thanks, Johan
The name sucks is what I'm saying. Please fix it eventually.
regards, dan carpenter
On Fri, Jan 27, 2017 at 03:47:21PM +0300, Dan Carpenter wrote:
The name sucks is what I'm saying. Please fix it eventually.
I disagree. The name uses a common prefix that reflects the object it is working on. This should not be changed. As it is currently named, the function is also self-documenting. Trying to abbreviate the name just to meet a 80 col limit (when there are alternatives) is just silly.
Johan
On Fri, Jan 27, 2017 at 02:08:06PM +0100, Johan Hovold wrote:
On Fri, Jan 27, 2017 at 03:47:21PM +0300, Dan Carpenter wrote:
The name sucks is what I'm saying. Please fix it eventually.
I disagree. The name uses a common prefix that reflects the object it is working on. This should not be changed. As it is currently named, the function is also self-documenting. Trying to abbreviate the name just to meet a 80 col limit (when there are alternatives) is just silly.
gb_interface_request_mode_switch()
It's not self documenting because there are no verbs in that sentence. I guess in JAVA it would be considered good style.
This is "Enterprise Quality" in the derogatory sense of the term. The prefix makes everything too long and doesn't add any value. We don't keep the 80 character limit because of small screens, we keep it to discourage this type of code.
regards, dan carpenter
On Fri, Jan 27, 2017 at 04:24:47PM +0300, Dan Carpenter wrote:
On Fri, Jan 27, 2017 at 02:08:06PM +0100, Johan Hovold wrote:
On Fri, Jan 27, 2017 at 03:47:21PM +0300, Dan Carpenter wrote:
The name sucks is what I'm saying. Please fix it eventually.
I disagree. The name uses a common prefix that reflects the object it is working on. This should not be changed. As it is currently named, the function is also self-documenting. Trying to abbreviate the name just to meet a 80 col limit (when there are alternatives) is just silly.
gb_interface_request_mode_switch()
It's not self documenting because there are no verbs in that sentence.
"request" is a verb.
We're requesting that the remote interface initiates a mode switch (something which it can refuse).
It's a function that is only a called a couple of times. No need to rewrite it as gb_intf_rms() or such (even if gb_intf would be a reasonable prefix).
You comments about missing verbs etc make me wonder how you parse the name:
gb_interface - a common prefix, indicating object being acted on request_mode_switch - describes what is done
we're not fetching anything as your "gb_get_mode_switch()" suggestion indicates you read it as.
I guess in JAVA it would be considered good style.
This is "Enterprise Quality" in the derogatory sense of the term. The prefix makes everything too long and doesn't add any value. We don't keep the 80 character limit because of small screens, we keep it to discourage this type of code.
And to keep indentation down.
The kernel already has similar function names such as "usb_driver_set_configuration" or "pm_runtime_mark_last_busy" which follow the same pattern (prefix - verb - object).
Johan
I guess I'm not a fan of really long prefixes... I've complained before to other people so don't feel special. Prefixes are something that people add without thinking as if they always make the code better and I do blame JAVA influence for this...
The "_interface" really doesn't add anything because that's always the first parameter that we pass. It's just filler.
I think "try" is better than "request" probably. Request is regularly used as a noun and try never is.
But I don't care at this point.
regards, dan carpenter
On Fri, Jan 27, 2017 at 05:13:01PM +0300, Dan Carpenter wrote:
I guess I'm not a fan of really long prefixes... I've complained before to other people so don't feel special. Prefixes are something that people add without thinking as if they always make the code better and I do blame JAVA influence for this...
The "_interface" really doesn't add anything because that's always the first parameter that we pass. It's just filler.
I think "try" is better than "request" probably. Request is regularly used as a noun and try never is.
But I don't care at this point.
regards, dan carpenter
I don't feel confortable removing a word in the function name as I don't fully understand the code.
What about shorting interface into intf as it is used in connection->intf ?
Johan, what do you mean by using connection->intf in this case ? Are you talking about a global variable usable in this context ?
Thanks, Maxime
Heh. I would just leave this one alone for now.
Everyone hates all the potential changes more than we hate going over 80 characters.
regards, dan carpenter
-
Dan Carpenter -
Greg KH -
Johan Hovold -
Maxime Rossi Bellom -
mrossibellom＠gmail.com