Macros starting with 'if' should be enclosed by do - while loop.
Revealed by checkpatch.pl.
Change is necessary because by enclosure possible if/else logic
defects are avoided . Also fixed inconsistent macro usage ending
';', which caused build error with the macro defines
enclosure in some cases.
Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
---
drivers/i2c/algos/i2c-algo-pcf.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index fd563e845d4b..3fc4b5080a32 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -23,10 +23,19 @@
#include "i2c-algo-pcf.h"
-#define DEB2(x) if (i2c_debug >= 2) x
-#define DEB3(x) if (i2c_debug >= 3) x /* print several statistical values */
-#define DEBPROTO(x) if (i2c_debug >= 9) x;
- /* debug the protocol by showing transferred bits */
+#define DEB2(x) do { \
+ if (i2c_debug >= 2) \
+ x; \
+ } while (0)
+#define DEB3(x) do { \
+ if (i2c_debug >= 3) \
+ x; /* print several statistical values */ \
+ } while (0)
+#define DEBPROTO(x) do { \
+ if (i2c_debug >= 9) \
+ x; \
+ /* debug the protocol by showing transferred bits */ \
+ } while (0)
#define DEF_TIMEOUT 16
/*
@@ -308,7 +317,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
timeout = wait_for_bb(adap);
if (timeout) {
DEB2(printk(KERN_ERR "i2c-algo-pcf.o: "
- "Timeout waiting for BB in pcf_xfer\n");)
+ "Timeout waiting for BB in pcf_xfer\n"));
i = -EIO;
goto out;
}
@@ -318,7 +327,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: Doing %s %d bytes to 0x%02x - %d of %d messages\n",
str_read_write(pmsg->flags & I2C_M_RD),
- pmsg->len, pmsg->addr, i + 1, num);)
+ pmsg->len, pmsg->addr, i + 1, num));
ret = pcf_doAddress(adap, pmsg);
@@ -336,7 +345,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
}
i2c_stop(adap);
DEB2(printk(KERN_ERR "i2c-algo-pcf.o: Timeout waiting "
- "for PIN(1) in pcf_xfer\n");)
+ "for PIN(1) in pcf_xfer\n"));
i = -EREMOTEIO;
goto out;
}
@@ -344,13 +353,13 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
/* Check LRB (last rcvd bit - slave ack) */
if (status & I2C_PCF_LRB) {
i2c_stop(adap);
- DEB2(printk(KERN_ERR "i2c-algo-pcf.o: No LRB(1) in pcf_xfer\n");)
+ DEB2(printk(KERN_ERR "i2c-algo-pcf.o: No LRB(1) in pcf_xfer\n"));
i = -EREMOTEIO;
goto out;
}
DEB3(printk(KERN_DEBUG "i2c-algo-pcf.o: Msg %d, addr=0x%x, flags=0x%x, len=%d\n",
- i, msgs[i].addr, msgs[i].flags, msgs[i].len);)
+ i, msgs[i].addr, msgs[i].flags, msgs[i].len));
if (pmsg->flags & I2C_M_RD) {
ret = pcf_readbytes(i2c_adap, pmsg->buf, pmsg->len,
--
2.43.0…> Change is necessary because … Does anything hinder you to choose imperative mood for change descriptions? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc5#n94 Would you occasionally care more also for word wrapping? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc5#n658 …> --- > drivers/i2c/algos/i2c-algo-pcf.c | 27 ++++++++++++++++++--------- … Some contributors would appreciate patch version descriptions. https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc5#n310 Regards, Markus
On Mon, Sep 08, 2025 at 04:46:47PM +0200, Markus Elfring wrote: Hello Markus, First I want to thank you for reviewing my patches and patchset and making comments for me to improve my patches. I am new to kernel hacking and this is the first patchset that I submit to linux kernel. I'm doing my best to improve my work so that it can be accepted into the kernel tree. > …> Change is necessary because … > > Does anything hinder you to choose imperative mood for change descriptions? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc5#n94 Sorry, I didn't understood the first time. I will try my best. > > Would you occasionally care more also for word wrapping? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc5#n658 > I will follow from now on the line wrapped at 75 columns. > > …> --- > > drivers/i2c/algos/i2c-algo-pcf.c | 27 ++++++++++++++++++--------- > … > > Some contributors would appreciate patch version descriptions. > https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc5#n310 > After I sent V2 of patchset I realized I didn't included changes after --- marker line. I will send V3 trying to apply your suggestions. Best regards, Cezar Chiru
> Sorry, I didn't understood the first time. I will try my best. Would you get further development inspirations from previously published information? Regards, Markus
Hello maintainers,
This patch series fixes 18 errors reported by checkpatch.pl on
drivers/i2c/algos/i2c-algo-pcf.c file.
The series v1->v2->v3 is a response to the discussion on the mailing
list with Markus Elfring that had comments on my earlier submisions. First
he suggested to split my initial submission in a patch series. Then he had
some valid points on imperative mood usage in commit messages, wrapping
commit message to 75 columns per line.
Here is a brief summary and Order of patches to be applied:
Patch 1/3: i2c: PCF8584: Fix debug macros defines of if statements
This patch encloses the debug macro defines of if statements in do-while
loops.
Patch 2/3: i2c: PCF8584: Fix do not use assignment in if conditional
This patch takes the assignement from if conditional and moves it by 1 line
up.
Patch 3/3: i2c: PCF8584: Fix space(s) required before or after different
operators
This patch adds space(s) around, before or after some operators
Testing:
*built kernel and modules with I2C_ALGOPCF=m and my 3 patches applied on
top.
*installed kernel and external modules generated by build on my laptop
*rebooted and loaded i2c-algo-pcf.ko with param i2c_debug=2/3/9.
*No success message related to i2c_algo_pcf was seen in dmesg but also no
errors.
*Module loading and unloading successfull.
*No PCF8584 Hardware was available.
Patches 1 and 3 report 4 and 6 warnings when running checkpatch on them.
But the warnings will be fixed in a new patchset that addresses fixing
warnings.
Cezar Chiru (3):
i2c: PCF8584: Fix debug macros defines of if statements
i2c: PCF8584: Fix do not use assignment in if conditional
i2c: PCF8584: Fix space(s) required before or after different operators
drivers/i2c/algos/i2c-algo-pcf.c | 62 ++++++++++++++++++++------------
1 file changed, 39 insertions(+), 23 deletions(-)
--
2.43.0
macros: Enclose 'if' statements debug macros defines in do - while loops.
Fix inconsistent macro usage ending ';', which caused build errors in some
cases.
Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
---
v1->v2: - Removed space between [subsystem] and ':' in subject line.
- Attempted to change commit message to imperative mood but I
didn't understood how.
v2->v3: - Formulated again the commit message to be at imperative mood.
- Wrapped commit message to 75 columns per line.
drivers/i2c/algos/i2c-algo-pcf.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index fd563e845d4b..3fc4b5080a32 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -23,10 +23,19 @@
#include "i2c-algo-pcf.h"
-#define DEB2(x) if (i2c_debug >= 2) x
-#define DEB3(x) if (i2c_debug >= 3) x /* print several statistical values */
-#define DEBPROTO(x) if (i2c_debug >= 9) x;
- /* debug the protocol by showing transferred bits */
+#define DEB2(x) do { \
+ if (i2c_debug >= 2) \
+ x; \
+ } while (0)
+#define DEB3(x) do { \
+ if (i2c_debug >= 3) \
+ x; /* print several statistical values */ \
+ } while (0)
+#define DEBPROTO(x) do { \
+ if (i2c_debug >= 9) \
+ x; \
+ /* debug the protocol by showing transferred bits */ \
+ } while (0)
#define DEF_TIMEOUT 16
/*
@@ -308,7 +317,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
timeout = wait_for_bb(adap);
if (timeout) {
DEB2(printk(KERN_ERR "i2c-algo-pcf.o: "
- "Timeout waiting for BB in pcf_xfer\n");)
+ "Timeout waiting for BB in pcf_xfer\n"));
i = -EIO;
goto out;
}
@@ -318,7 +327,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: Doing %s %d bytes to 0x%02x - %d of %d messages\n",
str_read_write(pmsg->flags & I2C_M_RD),
- pmsg->len, pmsg->addr, i + 1, num);)
+ pmsg->len, pmsg->addr, i + 1, num));
ret = pcf_doAddress(adap, pmsg);
@@ -336,7 +345,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
}
i2c_stop(adap);
DEB2(printk(KERN_ERR "i2c-algo-pcf.o: Timeout waiting "
- "for PIN(1) in pcf_xfer\n");)
+ "for PIN(1) in pcf_xfer\n"));
i = -EREMOTEIO;
goto out;
}
@@ -344,13 +353,13 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
/* Check LRB (last rcvd bit - slave ack) */
if (status & I2C_PCF_LRB) {
i2c_stop(adap);
- DEB2(printk(KERN_ERR "i2c-algo-pcf.o: No LRB(1) in pcf_xfer\n");)
+ DEB2(printk(KERN_ERR "i2c-algo-pcf.o: No LRB(1) in pcf_xfer\n"));
i = -EREMOTEIO;
goto out;
}
DEB3(printk(KERN_DEBUG "i2c-algo-pcf.o: Msg %d, addr=0x%x, flags=0x%x, len=%d\n",
- i, msgs[i].addr, msgs[i].flags, msgs[i].len);)
+ i, msgs[i].addr, msgs[i].flags, msgs[i].len));
if (pmsg->flags & I2C_M_RD) {
ret = pcf_readbytes(i2c_adap, pmsg->buf, pmsg->len,
--
2.43.0> macros: Enclose 'if' statements debug macros defines in do - while loops. Would it be helpful to indicate in each commit message how the change motivation was influenced by the script “checkpatch.pl”? Regards, Markus
if statement: Use of assignment inside of 'if' conditional is not allowed.
Move assignment from inside 'if' conditional, to one line before each 'if'
conditional statement that caused errors.
Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
---
v1->v2: - Removed space between [subsystem] and ':' in subject line.
- Attempted to change commit message to imperative mood but I
didn't understood how.
v2->v3: - Formulated again the commit message to imperative mood.
- Wrapped commit message to 75 columns per line.
- Removed '' from subject line of patch
drivers/i2c/algos/i2c-algo-pcf.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 3fc4b5080a32..598bf000bf4a 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -169,7 +169,8 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
* check to see S1 now used as R/W ctrl -
* PCF8584 does that when ESO is zero
*/
- if (((temp = get_pcf(adap, 1)) & 0x7f) != (0)) {
+ temp = get_pcf(adap, 1);
+ if ((temp & 0x7f) != (0)) {
DEB2(printk(KERN_ERR "i2c-algo-pcf.o: PCF detection failed -- can't select S0 (0x%02x).\n", temp));
return -ENXIO; /* definitely not PCF8584 */
}
@@ -177,7 +178,8 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
/* load own address in S0, effective address is (own << 1) */
i2c_outb(adap, get_own(adap));
/* check it's really written */
- if ((temp = i2c_inb(adap)) != get_own(adap)) {
+ temp = i2c_inb(adap);
+ if (temp != get_own(adap)) {
DEB2(printk(KERN_ERR "i2c-algo-pcf.o: PCF detection failed -- can't set S0 (0x%02x).\n", temp));
return -ENXIO;
}
@@ -185,7 +187,8 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
/* S1=0xA0, next byte in S2 */
set_pcf(adap, 1, I2C_PCF_PIN | I2C_PCF_ES1);
/* check to see S2 now selected */
- if (((temp = get_pcf(adap, 1)) & 0x7f) != I2C_PCF_ES1) {
+ temp = get_pcf(adap, 1);
+ if ((temp & 0x7f) != I2C_PCF_ES1) {
DEB2(printk(KERN_ERR "i2c-algo-pcf.o: PCF detection failed -- can't select S2 (0x%02x).\n", temp));
return -ENXIO;
}
@@ -193,7 +196,8 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
/* load clock register S2 */
i2c_outb(adap, get_clock(adap));
/* check it's really written, the only 5 lowest bits does matter */
- if (((temp = i2c_inb(adap)) & 0x1f) != get_clock(adap)) {
+ temp = i2c_inb(adap);
+ if ((temp & 0x1f) != get_clock(adap)) {
DEB2(printk(KERN_ERR "i2c-algo-pcf.o: PCF detection failed -- can't set S2 (0x%02x).\n", temp));
return -ENXIO;
}
@@ -202,7 +206,8 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
set_pcf(adap, 1, I2C_PCF_IDLE);
/* check to see PCF is really idled and we can access status register */
- if ((temp = get_pcf(adap, 1)) != (I2C_PCF_PIN | I2C_PCF_BB)) {
+ temp = get_pcf(adap, 1);
+ if (temp != (I2C_PCF_PIN | I2C_PCF_BB)) {
DEB2(printk(KERN_ERR "i2c-algo-pcf.o: PCF detection failed -- can't select S1` (0x%02x).\n", temp));
return -ENXIO;
}
@@ -255,7 +260,8 @@ static int pcf_readbytes(struct i2c_adapter *i2c_adap, char *buf,
/* increment number of bytes to read by one -- read dummy byte */
for (i = 0; i <= count; i++) {
- if ((wfp = wait_for_pin(adap, &status))) {
+ wfp = wait_for_pin(adap, &status);
+ if (wfp) {
if (wfp == -EINTR)
return -EINTR; /* arbitration lost */
@@ -415,7 +421,8 @@ int i2c_pcf_add_bus(struct i2c_adapter *adap)
/* register new adapter to i2c module... */
adap->algo = &pcf_algo;
- if ((rval = pcf_init_8584(pcf_adap)))
+ rval = pcf_init_8584(pcf_adap);
+ if (rval)
return rval;
rval = i2c_add_adapter(adap);
--
2.43.0operators: Require spaces around or before or after '=', ';', '<' and ','.
Add space(s) around or before or after different operators.
Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
---
v1->v2: - Removed space between [subsystem] and ':' in subject line.
- Attempted to change commit message to imperative mood but I
didn't understood how.
v2->v3: - Formulated again the commit message to imperative mood.
- Wrapped commit message to 75 columns per line.
- Changed subject line of patch
drivers/i2c/algos/i2c-algo-pcf.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 598bf000bf4a..3439b7387a54 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -223,7 +223,7 @@ static int pcf_sendbytes(struct i2c_adapter *i2c_adap, const char *buf,
struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
int wrcount, status, timeout;
- for (wrcount=0; wrcount<count; ++wrcount) {
+ for (wrcount = 0; wrcount < count; ++wrcount) {
DEB2(dev_dbg(&i2c_adap->dev, "i2c_write: writing %2.2X\n",
buf[wrcount] & 0xff));
i2c_outb(adap, buf[wrcount]);
@@ -314,7 +314,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
struct i2c_msg *pmsg;
int i;
- int ret=0, timeout, status;
+ int ret = 0, timeout, status;
if (adap->xfer_begin)
adap->xfer_begin(adap->data);
@@ -328,7 +328,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
goto out;
}
- for (i = 0;ret >= 0 && i < num; i++) {
+ for (i = 0; ret >= 0 && i < num; i++) {
pmsg = &msgs[i];
DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: Doing %s %d bytes to 0x%02x - %d of %d messages\n",
@@ -373,9 +373,9 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
if (ret != pmsg->len) {
DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: fail: "
- "only read %d bytes.\n",ret));
+ "only read %d bytes.\n", ret));
} else {
- DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: read %d bytes.\n",ret));
+ DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: read %d bytes.\n", ret));
}
} else {
ret = pcf_sendbytes(i2c_adap, pmsg->buf, pmsg->len,
@@ -383,9 +383,9 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
if (ret != pmsg->len) {
DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: fail: "
- "only wrote %d bytes.\n",ret));
+ "only wrote %d bytes.\n", ret));
} else {
- DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: wrote %d bytes.\n",ret));
+ DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: wrote %d bytes.\n", ret));
}
}
}
--
2.43.0> operators: Require spaces around or before or after '=', ';', '<' and ','. > Add space(s) around or before or after different operators. How do you think about to refine such a wording approach another bit? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.17-rc5#n274 https://elixir.bootlin.com/linux/v6.17-rc5/source/scripts/checkpatch.pl#L5090-L5399 Regards, Markus
Hi Markus, Thank you for the time to review my patch series. > > operators: Require spaces around or before or after '=', ';', '<' and ','. > > Add space(s) around or before or after different operators. > > How do you think about to refine such a wording approach another bit? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.17-rc5#n274 > https://elixir.bootlin.com/linux/v6.17-rc5/source/scripts/checkpatch.pl#L5090-L5399 I will refer to binary and ternary operators instead of listing them. Also I will say punctuation signs like: ';' , ','
© 2016 - 2025 Red Hat, Inc.