Re: [PATCH] staging: media: lirc: lirc_zilog.c: fix quoted strings split across lines
On Wed, 2014-11-26 at 15:42 +, Luis de Bethencourt wrote: On 26 November 2014 at 01:49, Joe Perches j...@perches.com wrote: [] There is a script I posted a while back that groups various checkpatch types together and makes it a bit easier to do cleanup style patches. https://lkml.org/lkml/2014/7/11/794 That is useful! I just run it on staging/octeon/ and it wrote two patches. Will submit them in a minute. Please make sure and write better commit messages than the script produces. Using checkpatch to get familiar with kernel development is fine and all, but fixing actual defects and submitting new code is way more useful. [] I agree. I was just using checkpatch to learn about the development process. How to create patches, submit patches, follow review, and such. Better to do it with small changes like this first. That's a good way to start. Which makes me wonder. Is my patch accepted? Will it be merged? I can do the proposed logging macro additions in a few days. Not sure yet how the final step of the process when patches get accepted and merged works. You will generally get an email from a maintainer when patches are accepted/rejected or you get feedback asking for various changes. Greg KH does that for drivers/staging but not for drivers/staging/media. Mauro Carvalho Chehab does. These emails are not immediate. It can take 2 or 3 weeks for a response. Sometimes longer, sometimes shorter, sometimes no response ever comes. After a month or so, if you get no response, maybe the maintainer never saw it. You should maybe expand the cc: list for the email. When the patch is more than a trivial style cleanup, Andrew Morton generally picks up orphan patches. For some subsystems, there are tracking mechanisms like patchwork: For instance, netdev (net/ and drivers/net/) uses: http://patchwork.ozlabs.org/project/netdev/list/ and David Miller, the primary networking maintainer is very prompt about updating it. There's this list of patchwork entries, but maintainer activity of these lists vary: https://patchwork.kernel.org/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: media: lirc: lirc_zilog.c: fix quoted strings split across lines
On Wed, Nov 26, 2014 at 08:05:55AM -0800, Joe Perches wrote: On Wed, 2014-11-26 at 15:42 +, Luis de Bethencourt wrote: On 26 November 2014 at 01:49, Joe Perches j...@perches.com wrote: [] There is a script I posted a while back that groups various checkpatch types together and makes it a bit easier to do cleanup style patches. https://lkml.org/lkml/2014/7/11/794 That is useful! I just run it on staging/octeon/ and it wrote two patches. Will submit them in a minute. Please make sure and write better commit messages than the script produces. Will do :) Using checkpatch to get familiar with kernel development is fine and all, but fixing actual defects and submitting new code is way more useful. [] I agree. I was just using checkpatch to learn about the development process. How to create patches, submit patches, follow review, and such. Better to do it with small changes like this first. That's a good way to start. Which makes me wonder. Is my patch accepted? Will it be merged? I can do the proposed logging macro additions in a few days. Not sure yet how the final step of the process when patches get accepted and merged works. You will generally get an email from a maintainer when patches are accepted/rejected or you get feedback asking for various changes. Greg KH does that for drivers/staging but not for drivers/staging/media. Mauro Carvalho Chehab does. These emails are not immediate. It can take 2 or 3 weeks for a response. Sometimes longer, sometimes shorter, sometimes no response ever comes. I understand. Busy people. After a month or so, if you get no response, maybe the maintainer never saw it. You should maybe expand the cc: list for the email. When the patch is more than a trivial style cleanup, Andrew Morton generally picks up orphan patches. For some subsystems, there are tracking mechanisms like patchwork: For instance, netdev (net/ and drivers/net/) uses: http://patchwork.ozlabs.org/project/netdev/list/ and David Miller, the primary networking maintainer is very prompt about updating it. There's this list of patchwork entries, but maintainer activity of these lists vary: https://patchwork.kernel.org/ Very interesting. I will follow the process through and learn on the way. Thanks Joe! -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: media: lirc: lirc_zilog.c: fix quoted strings split across lines
On Tue, 2014-11-25 at 20:19 +, Luis de Bethencourt wrote:
checkpatch makes an exception to the 80-colum rule for quotes strings, and
Documentation/CodingStyle recommends not splitting quotes strings across lines
because it breaks the ability to grep for the string. Fixing these.
[]
diff --git a/drivers/staging/media/lirc/lirc_zilog.c
b/drivers/staging/media/lirc/lirc_zilog.c
[]
@@ -794,8 +792,7 @@ static int fw_load(struct IR_tx *tx)
if (!read_uint8(data, tx_data-endp, version))
goto corrupt;
if (version != 1) {
- dev_err(tx-ir-l.dev, unsupported code set file version (%u,
expected
- 1) -- please upgrade to a newer driver,
+ dev_err(tx-ir-l.dev, unsupported code set file version (%u,
expected1) -- please upgrade to a newer driver,
version);
Hello Luis.
Please look at the strings being coalesced before
submitting patches.
It's a fairly common defect to have either a missing
space between the coalesced fragments or too many
spaces.
It's almost certain that there should be a space
between the expected and 1 here.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: media: lirc: lirc_zilog.c: fix quoted strings split across lines
On Tue, Nov 25, 2014 at 12:27:24PM -0800, Joe Perches wrote:
On Tue, 2014-11-25 at 20:19 +, Luis de Bethencourt wrote:
checkpatch makes an exception to the 80-colum rule for quotes strings, and
Documentation/CodingStyle recommends not splitting quotes strings across
lines
because it breaks the ability to grep for the string. Fixing these.
[]
diff --git a/drivers/staging/media/lirc/lirc_zilog.c
b/drivers/staging/media/lirc/lirc_zilog.c
[]
@@ -794,8 +792,7 @@ static int fw_load(struct IR_tx *tx)
if (!read_uint8(data, tx_data-endp, version))
goto corrupt;
if (version != 1) {
- dev_err(tx-ir-l.dev, unsupported code set file version (%u,
expected
- 1) -- please upgrade to a newer driver,
+ dev_err(tx-ir-l.dev, unsupported code set file version (%u,
expected1) -- please upgrade to a newer driver,
version);
Hello Luis.
Please look at the strings being coalesced before
submitting patches.
It's a fairly common defect to have either a missing
space between the coalesced fragments or too many
spaces.
It's almost certain that there should be a space
between the expected and 1 here.
Hello Joe,
Thanks for taking the time to review this. I sent a new
version fixing the missing space.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: media: lirc: lirc_zilog.c: fix quoted strings split across lines
On Tue, 2014-11-25 at 20:40 +, Luis de Bethencourt wrote:
On Tue, Nov 25, 2014 at 12:27:24PM -0800, Joe Perches wrote:
On Tue, 2014-11-25 at 20:19 +, Luis de Bethencourt wrote:
checkpatch makes an exception to the 80-colum rule for quotes strings, and
Documentation/CodingStyle recommends not splitting quotes strings across
lines
because it breaks the ability to grep for the string. Fixing these.
[]
diff --git a/drivers/staging/media/lirc/lirc_zilog.c
b/drivers/staging/media/lirc/lirc_zilog.c
[]
@@ -794,8 +792,7 @@ static int fw_load(struct IR_tx *tx)
if (!read_uint8(data, tx_data-endp, version))
goto corrupt;
if (version != 1) {
- dev_err(tx-ir-l.dev, unsupported code set file version (%u,
expected
- 1) -- please upgrade to a newer driver,
+ dev_err(tx-ir-l.dev, unsupported code set file version (%u,
expected1) -- please upgrade to a newer driver,
version);
Hello Luis.
Please look at the strings being coalesced before
submitting patches.
It's a fairly common defect to have either a missing
space between the coalesced fragments or too mano
spaces.
It's almost certain that there should be a space
between the expected and 1 here.
Hello Joe,
Thanks for taking the time to review this. I sent a new
version fixing the missing space.
Thanks.
In the future, you might consider being more
comprehensive with your patches.
This code could be neatened a bit by:
o using another set of logging macros
o removing the unnecessary ftrace like logging
o realigning arguments
Something like:
---
drivers/staging/media/lirc/lirc_zilog.c | 151 +++-
1 file changed, 73 insertions(+), 78 deletions(-)
diff --git a/drivers/staging/media/lirc/lirc_zilog.c
b/drivers/staging/media/lirc/lirc_zilog.c
index bebb9f1..523af12 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -158,6 +158,17 @@ static bool debug; /* debug output */
static bool tx_only; /* only handle the IR Tx function */
static int minor = -1; /* minor number */
+/* logging macros */
+#define ir_err(ir, fmt, ...) \
+ dev_err((ir)-l.dev, fmt, ##__VA_ARGS__)
+#define ir_warn(ir, fmt, ...) \
+ dev_warn((ir)-l.dev, fmt, ##__VA_ARGS__)
+#define ir_notice(ir, fmt, ...)\
+ dev_notice((ir)-l.dev, fmt, ##__VA_ARGS__)
+#define ir_info(ir, fmt, ...) \
+ dev_info((ir)-l.dev, fmt, ##__VA_ARGS__)
+#define ir_dbg(ir, fmt, ...) \
+ dev_dbg((ir)-l.dev, fmt, ##__VA_ARGS__)
/* struct IR reference counting */
static struct IR *get_ir_device(struct IR *ir, bool ir_devices_lock_held)
@@ -322,7 +333,7 @@ static int add_to_buf(struct IR *ir)
struct IR_tx *tx;
if (lirc_buffer_full(rbuf)) {
- dev_dbg(ir-l.dev, buffer overflow\n);
+ ir_dbg(ir, buffer overflow\n);
return -EOVERFLOW;
}
@@ -368,18 +379,15 @@ static int add_to_buf(struct IR *ir)
*/
ret = i2c_master_send(rx-c, sendbuf, 1);
if (ret != 1) {
- dev_err(ir-l.dev, i2c_master_send failed with %d\n,
- ret);
+ ir_err(ir, i2c_master_send failed with %d\n, ret);
if (failures = 3) {
mutex_unlock(ir-ir_lock);
- dev_err(ir-l.dev, unable to read from the IR
chip
- after 3 resets, giving up\n);
+ ir_err(ir, unable to read from the IR chip
after 3 resets, giving up\n);
break;
}
/* Looks like the chip crashed, reset it */
- dev_err(ir-l.dev, polling the IR receiver chip
failed,
- trying reset\n);
+ ir_err(ir, polling the IR receiver chip failed, trying
reset\n);
set_current_state(TASK_UNINTERRUPTIBLE);
if (kthread_should_stop()) {
@@ -405,14 +413,13 @@ static int add_to_buf(struct IR *ir)
ret = i2c_master_recv(rx-c, keybuf, sizeof(keybuf));
mutex_unlock(ir-ir_lock);
if (ret != sizeof(keybuf)) {
- dev_err(ir-l.dev, i2c_master_recv failed with %d --
- keeping last read buffer\n, ret);
+ ir_err(ir, i2c_master_recv failed with %d -- keeping
last read buffer\n,
+ ret);
} else {
rx-b[0] = keybuf[3];
rx-b[1] = keybuf[4];
Re: [PATCH] staging: media: lirc: lirc_zilog.c: fix quoted strings split across lines
On Tue, Nov 25, 2014 at 01:00:07PM -0800, Joe Perches wrote:
On Tue, 2014-11-25 at 20:40 +, Luis de Bethencourt wrote:
On Tue, Nov 25, 2014 at 12:27:24PM -0800, Joe Perches wrote:
On Tue, 2014-11-25 at 20:19 +, Luis de Bethencourt wrote:
checkpatch makes an exception to the 80-colum rule for quotes strings,
and
Documentation/CodingStyle recommends not splitting quotes strings
across lines
because it breaks the ability to grep for the string. Fixing these.
[]
diff --git a/drivers/staging/media/lirc/lirc_zilog.c
b/drivers/staging/media/lirc/lirc_zilog.c
[]
@@ -794,8 +792,7 @@ static int fw_load(struct IR_tx *tx)
if (!read_uint8(data, tx_data-endp, version))
goto corrupt;
if (version != 1) {
- dev_err(tx-ir-l.dev, unsupported code set file
version (%u, expected
- 1) -- please upgrade to a newer driver,
+ dev_err(tx-ir-l.dev, unsupported code set file
version (%u, expected1) -- please upgrade to a newer driver,
version);
Hello Luis.
Please look at the strings being coalesced before
submitting patches.
It's a fairly common defect to have either a missing
space between the coalesced fragments or too mano
spaces.
It's almost certain that there should be a space
between the expected and 1 here.
Hello Joe,
Thanks for taking the time to review this. I sent a new
version fixing the missing space.
Thanks.
In the future, you might consider being more
comprehensive with your patches.
Wasn't sure about the scope of the style fixing
patches. I've been reading Kernel Newbies and
this looked like a good way to start
contributing. Good to know more exhaustive
changes are welcome.
This code could be neatened a bit by:
o using another set of logging macros
o removing the unnecessary ftrace like logging
o realigning arguments
Great ideas.
Should this have been all included in one patch,
or each as part of a series with the previous
one?
Want to take the opportunity to learn about the
process.
Something like:
---
drivers/staging/media/lirc/lirc_zilog.c | 151
+++-
1 file changed, 73 insertions(+), 78 deletions(-)
diff --git a/drivers/staging/media/lirc/lirc_zilog.c
b/drivers/staging/media/lirc/lirc_zilog.c
index bebb9f1..523af12 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -158,6 +158,17 @@ static bool debug; /* debug output */
static bool tx_only; /* only handle the IR Tx function */
static int minor = -1; /* minor number */
+/* logging macros */
+#define ir_err(ir, fmt, ...) \
+ dev_err((ir)-l.dev, fmt, ##__VA_ARGS__)
+#define ir_warn(ir, fmt, ...)\
+ dev_warn((ir)-l.dev, fmt, ##__VA_ARGS__)
+#define ir_notice(ir, fmt, ...) \
+ dev_notice((ir)-l.dev, fmt, ##__VA_ARGS__)
+#define ir_info(ir, fmt, ...)\
+ dev_info((ir)-l.dev, fmt, ##__VA_ARGS__)
+#define ir_dbg(ir, fmt, ...) \
+ dev_dbg((ir)-l.dev, fmt, ##__VA_ARGS__)
/* struct IR reference counting */
static struct IR *get_ir_device(struct IR *ir, bool ir_devices_lock_held)
@@ -322,7 +333,7 @@ static int add_to_buf(struct IR *ir)
struct IR_tx *tx;
if (lirc_buffer_full(rbuf)) {
- dev_dbg(ir-l.dev, buffer overflow\n);
+ ir_dbg(ir, buffer overflow\n);
return -EOVERFLOW;
}
@@ -368,18 +379,15 @@ static int add_to_buf(struct IR *ir)
*/
ret = i2c_master_send(rx-c, sendbuf, 1);
if (ret != 1) {
- dev_err(ir-l.dev, i2c_master_send failed with %d\n,
-ret);
+ ir_err(ir, i2c_master_send failed with %d\n, ret);
if (failures = 3) {
mutex_unlock(ir-ir_lock);
- dev_err(ir-l.dev, unable to read from the IR
chip
- after 3 resets, giving up\n);
+ ir_err(ir, unable to read from the IR chip
after 3 resets, giving up\n);
break;
}
/* Looks like the chip crashed, reset it */
- dev_err(ir-l.dev, polling the IR receiver chip
failed,
- trying reset\n);
+ ir_err(ir, polling the IR receiver chip failed, trying
reset\n);
set_current_state(TASK_UNINTERRUPTIBLE);
if (kthread_should_stop()) {
@@ -405,14 +413,13 @@ static int add_to_buf(struct IR *ir)
ret =
Re: [PATCH] staging: media: lirc: lirc_zilog.c: fix quoted strings split across lines
On Tue, 2014-11-25 at 21:14 +, Luis de Bethencourt wrote: On Tue, Nov 25, 2014 at 01:00:07PM -0800, Joe Perches wrote: In the future, you might consider being more comprehensive with your patches. Wasn't sure about the scope of the style fixing patches. I've been reading Kernel Newbies and this looked like a good way to start contributing. Good to know more exhaustive changes are welcome. This code could be neatened a bit by: o using another set of logging macros o removing the unnecessary ftrace like logging o realigning arguments Great ideas. Should this have been all included in one patch, or each as part of a series with the previous one? Want to take the opportunity to learn about the process. Hello again Luis. I think the suggestion I posted here is suitable for a single change. Ideally, you'd make individual patches each with a single type of change. There is a script I posted a while back that groups various checkpatch types together and makes it a bit easier to do cleanup style patches. https://lkml.org/lkml/2014/7/11/794 But don't just use checkpatch as the sole decider of what's appropriate to fix or neaten. checkpatch is a stupid, brainless little script. So is the automation script that uses checkpatch. For instance, checkpatch would not have suggested creating and using another logging macro. Please use your own taste to best figure out what to fix and how. Using checkpatch to get familiar with kernel development is fine and all, but fixing actual defects and submitting new code is way more useful. cheers, welcome, Joe -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
