i2c: add missing KERN_* constants to printks

From: Frank Seidel <[email protected]>

According to kerneljanitors todo list all printk calls (beginning a new line) should have an according KERN_* constant. Those are the missing pieces here for the i2c subsystem.

Are you sure about KERN_WARNING? I havn't looked deeper, but DEB2 suggests KERN_DEBUG? What about fixing DEB[1-3] directly instead of each "call". e.g.

Quoted 3 lines · Uwe Kleine-König

Are you sure about KERN_WARNING? I havn't looked deeper, but DEB2 suggests KERN_DEBUG? What about fixing DEB[1-3] directly instead of each "call". e.g.

No, in this case the DEB* macro was already in many places used with KERN_* constants just like direct printk. I didn't wanna change that.
KERN_WARNING is just the standard/default loglevel so that this change doesn't introduce different behaviour.

KERN_WARNING is just the standard/default loglevel so that this change doesn't introduce different behaviour.

Default log level? That's news to me. Care to elaborate?

Quoted 1 line · Jean Delvare

Default log level? That's news to me. Care to elaborate?

printk (in kernel/printk.c) uses vprintk and vprintk:
asmlinkage int vprintk(const char *fmt, va_list args)

{
        int printed_len = 0;
        int current_log_level = default_message_loglevel;

...
and default_message_loglevel in include/linux/kernel.h:

#define default_message_loglevel (console_printk[1])

and console_printk[1] is (in kernel/printk.c)
DEFAULT_MESSAGE_LOGLEVEL

and in kernel/printk.c:
#define DEFAULT_MESSAGE_LOGLEVEL 4 /* KERN_WARNING */

=> so as far as i could see KERN_WARNING is the
default loglevel of messages.

Quoted 16 lines · Frank Seidel

Quoted 1 line · Jean Delvare

Default log level? That's news to me. Care to elaborate?

printk (in kernel/printk.c) uses vprintk and vprintk:
asmlinkage int vprintk(const char *fmt, va_list args)

{
        int printed_len = 0;
        int current_log_level = default_message_loglevel;

...
and default_message_loglevel in include/linux/kernel.h:

#define default_message_loglevel (console_printk[1])

and console_printk[1] is (in kernel/printk.c)
DEFAULT_MESSAGE_LOGLEVEL

Quoted 5 lines

and in kernel/printk.c:
#define DEFAULT_MESSAGE_LOGLEVEL 4 /* KERN_WARNING */

=> so as far as i could see KERN_WARNING is the
default loglevel of messages.

one little note: This is changable writing to /proc/sys/kernel/printk.

So the patch introduces at least a little change in behaviour. IMHO this is OK though.

From: Frank Seidel <[email protected]>

According to kerneljanitors todo list all printk calls (beginning a new line) should have an according KERN_* constant. Those are the missing pieces here for the i2c subsystem.

Signed-off-by Frank Seidel <[email protected]>

---
 drivers/i2c/algos/i2c-algo-pca.c  |   34 ++++++++++++++++++----------------
 drivers/i2c/algos/i2c-algo-pcf.c  |    2 +-
 drivers/i2c/busses/i2c-pca-isa.c  |    5 +++--
 drivers/i2c/busses/i2c-powermac.c |    3 ++-
 drivers/i2c/busses/i2c-pxa.c      |    6 +++---
 5 files changed, 27 insertions(+), 23 deletions(-)

drivers/i2c/busses/i2c-pca-isa.c +3 -2

@@ -49,7 +49,8 @@ static void pca_isa_writebyte(void *pd,
 {
 #ifdef DEBUG_IO
 	static char *names[] = { "T/O", "DAT", "ADR", "CON" };
-	printk("*** write %s at %#lx <= %#04x\n", names[reg], base+reg, val);
+	printk(KERN_DEBUG "*** write %s at %#lx <= %#04x\n", names[reg],
+	       base+reg, val);
 #endif
 	outb(val, base+reg);
 }

@@ -60,7 +61,7 @@ static int pca_isa_readbyte(void *pd, in
 #ifdef DEBUG_IO
 	{
 		static char *names[] = { "STA", "DAT", "ADR", "CON" };
-		printk("*** read  %s => %#04x\n", names[reg], res);
+		printk(KERN_DEBUG "*** read  %s => %#04x\n", names[reg], res);
 	}
 #endif
 	return res;

drivers/i2c/busses/i2c-pxa.c +0 -0

@@ -210,10 +210,10 @@ static irqreturn_t i2c_pxa_handler(int t
 static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
 {
 	unsigned int i;

drivers/i2c/busses/i2c-powermac.c +2 -1

@@ -191,7 +191,8 @@ static int __devexit i2c_powermac_remove
 	i2c_set_adapdata(adapter, NULL);
 	/* We aren't that prepared to deal with this... */
 	if (rc)
-		printk("i2c-powermac.c: Failed to remove bus %s !\n",
+		printk(KERN_WARNING
+		       "i2c-powermac.c: Failed to remove bus %s !\n",
 		       adapter->name);
 	platform_set_drvdata(dev, NULL);
 	kfree(adapter);

drivers/i2c/algos/i2c-algo-pca.c +15 -15

@@ -51,7 +51,7 @@ static int i2c_debug;
 static void pca_start(struct i2c_algo_pca_data *adap)
 {
 	int sta = pca_get_con(adap);
-	DEB2("=== START\n");
+	DEB2(KERN_WARNING "=== START\n");
 	sta |= I2C_PCA_CON_STA;
 	sta &= ~(I2C_PCA_CON_STO|I2C_PCA_CON_SI);
 	pca_set_con(adap, sta);

@@ -66,7 +66,7 @@ static void pca_start(struct i2c_algo_pc
 static void pca_repeated_start(struct i2c_algo_pca_data *adap)
 {
 	int sta = pca_get_con(adap);
-	DEB2("=== REPEATED START\n");
+	DEB2(KERN_WARNING "=== REPEATED START\n");
 	sta |= I2C_PCA_CON_STA;
 	sta &= ~(I2C_PCA_CON_STO|I2C_PCA_CON_SI);
 	pca_set_con(adap, sta);

@@ -85,7 +85,7 @@ static void pca_repeated_start(struct i2
 static void pca_stop(struct i2c_algo_pca_data *adap)
 {
 	int sta = pca_get_con(adap);
-	DEB2("=== STOP\n");
+	DEB2(KERN_WARNING "=== STOP\n");
 	sta |= I2C_PCA_CON_STO;
 	sta &= ~(I2C_PCA_CON_STA|I2C_PCA_CON_SI);
 	pca_set_con(adap, sta);

@@ -105,7 +105,7 @@ static void pca_address(struct i2c_algo_
 	addr = ( (0x7f & msg->addr) << 1 );
 	if (msg->flags & I2C_M_RD )
 		addr |= 1;
-	DEB2("=== SLAVE ADDRESS %#04x+%c=%#04x\n",
+	DEB2(KERN_WARNING "=== SLAVE ADDRESS %#04x+%c=%#04x\n",
 	     msg->addr, msg->flags & I2C_M_RD ? 'R' : 'W', addr);
 
 	pca_outw(adap, I2C_PCA_DAT, addr);

@@ -125,7 +125,7 @@ static void pca_tx_byte(struct i2c_algo_
 			__u8 b)
 {
 	int sta = pca_get_con(adap);
-	DEB2("=== WRITE %#04x\n", b);
+	DEB2(KERN_WARNING "=== WRITE %#04x\n", b);
 	pca_outw(adap, I2C_PCA_DAT, b);
 
 	sta &= ~(I2C_PCA_CON_STO|I2C_PCA_CON_STA|I2C_PCA_CON_SI);

@@ -143,7 +143,7 @@ static void pca_rx_byte(struct i2c_algo_
 			__u8 *b, int ack)
 {
 	*b = pca_inw(adap, I2C_PCA_DAT);
-	DEB2("=== READ %#04x %s\n", *b, ack ? "ACK" : "NACK");
+	DEB2(KERN_WARNING "=== READ %#04x %s\n", *b, ack ? "ACK" : "NACK");
 }
 
 /*

@@ -185,7 +185,7 @@ static int pca_xfer(struct i2c_adapter *
 		return -EAGAIN;
 	}
 
-	DEB1("{{{ XFER %d messages\n", num);
+	DEB1(KERN_WARNING "{{{ XFER %d messages\n", num);
 
 	if (i2c_debug>=2) {
 		for (curmsg = 0; curmsg < num; curmsg++) {

@@ -213,7 +213,7 @@ static int pca_xfer(struct i2c_adapter *
 	while (curmsg < num) {
 		state = pca_status(adap);
 
-		DEB3("STATE is 0x%02x\n", state);
+		DEB3(KERN_WARNING "STATE is 0x%02x\n", state);
 		msg = &msgs[curmsg];
 
 		switch (state) {

@@ -241,7 +241,7 @@ static int pca_xfer(struct i2c_adapter *
 			break;
 
 		case 0x20: /* SLA+W has been transmitted; NOT ACK has been received */
-			DEB2("NOT ACK received after SLA+W\n");
+			DEB2(KERN_WARNING "NOT ACK received after SLA+W\n");
 			pca_stop(adap);
 			goto out;
 

@@ -264,16 +264,16 @@ static int pca_xfer(struct i2c_adapter *
 			break;
 
 		case 0x48: /* SLA+R has been transmitted; NOT ACK has been received */
-			DEB2("NOT ACK received after SLA+R\n");
+			DEB2(KERN_WARNING "NOT ACK received after SLA+R\n");
 			pca_stop(adap);
 			goto out;
 
 		case 0x30: /* Data byte in I2CDAT has been transmitted; NOT ACK has been received */
-			DEB2("NOT ACK received after data byte\n");
+			DEB2(KERN_WARNING "NOT ACK received after data byte\n");
 			goto out;
 
 		case 0x38: /* Arbitration lost during SLA+W, SLA+R or data bytes */
-			DEB2("Arbitration lost\n");
+			DEB2(KERN_WARNING "Arbitration lost\n");
 			goto out;
 
 		case 0x58: /* Data byte has been received; NOT ACK has been returned */

@@ -285,7 +285,8 @@ static int pca_xfer(struct i2c_adapter *
 				else
 					pca_repeated_start(adap);
 			} else {
-				DEB2("NOT ACK sent after data byte received. "
+				DEB2(KERN_WARNING
+				     "NOT ACK sent after data byte received. "
 				     "Not final byte. numbytes %d. len %d\n",
 				     numbytes, msg->len);
 				pca_stop(adap);

@@ -293,15 +294,16 @@ static int pca_xfer(struct i2c_adapter *
 			}
 			break;
 		case 0x70: /* Bus error - SDA stuck low */
-			DEB2("BUS ERROR - SDA Stuck low\n");
+			DEB2(KERN_WARNING "BUS ERROR - SDA Stuck low\n");
 			pca_reset(adap);
 			goto out;
 		case 0x90: /* Bus error - SCL stuck low */
-			DEB2("BUS ERROR - SCL Stuck low\n");

drivers/i2c/busses/i2c-pxa.c static irqreturn_t i2c_pxa_handler(int t · line 219

Quoted 4 lines

drivers/i2c/busses/i2c-pxa.c +1 -0

+	printk(KERN_ERR "i2c: ICR: %08x ISR: %08x\n"
 	       "i2c: log: ", readl(_ICR(i2c)), readl(_ISR(i2c)));
 	for (i = 0; i < i2c->irqlogidx; i++)
 		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);

This last printk also lacks a log level. You can't add it in the loop, but you could add one printk before the loop, printing just the log level.

I second Uwe's comments: it would be much better to add the log level in the DEBx macros. Additionally, some discussion is needed as to what log level would be appropriate. You put KERN_WARNING for all but some of these messages are clearly debugging messages. As a matter of fact, DEBx macros are all about debugging.

So I think that a good patch would 1* add KERN_DEBUG to the DEBx macros and 2* check for all usage of these DEBx macros and turn all messages which aren't actual debug messages to raw printks. As this will require some more work, you may want to split these changes off your initial patch.

Hi,

Quoted 7 lines · Jean Delvare

drivers/i2c/busses/i2c-pxa.c static irqreturn_t i2c_pxa_handler(int t · line 219

Quoted 4 lines

drivers/i2c/busses/i2c-pxa.c +1 -0

+	printk(KERN_ERR "i2c: ICR: %08x ISR: %08x\n"
 	       "i2c: log: ", readl(_ICR(i2c)), readl(_ISR(i2c)));
 	for (i = 0; i < i2c->irqlogidx; i++)
 		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);

This last printk also lacks a log level. You can't add it in the loop, but you could add one printk before the loop, printing just the log level.

Isn't the last printk just a followup of the previous KERN_ERR which doesn't have a newline? And i'm told only on new line beginnings the loglevel should be set.

(quoted diff) +1 -0

+	printk(KERN_DEBUG);

	for (i = 0; i < i2c->irqlogidx; i++)
		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
	printk("\n");

I mean the (first) printk in the loop, not the last printk in this function. So what I am suggesting is:

(quoted diff) +1 -0

+	printk(KERN_DEBUG);

	for (i = 0; i < i2c->irqlogidx; i++)
		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
	printk("\n");

Quoted 7 lines

I mean the (first) printk in the loop, not the last printk in this function. So what I am suggesting is:

(quoted diff) +1 -0

+	printk(KERN_DEBUG);

	for (i = 0; i < i2c->irqlogidx; i++)
		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
	printk("\n");

Hmm, I was told (by Jean ;)) that every printk should end with newline, otherwise it might get interrupted with other output -> mess.

From: Frank Seidel <[email protected]>

According to kerneljanitors todo list all printk calls (beginning a new line) should have an according KERN_* constant. Those are the missing pieces here for the i2c subsystem.

Signed-off-by Frank Seidel <[email protected]>

---
 drivers/i2c/algos/i2c-algo-pcf.c  |    2 +-
 drivers/i2c/busses/i2c-pca-isa.c  |    5 +++--
 drivers/i2c/busses/i2c-powermac.c |    3 ++-
 drivers/i2c/busses/i2c-pxa.c      |    9 +++++----
 4 files changed, 11 insertions(+), 8 deletions(-)

From: Frank Seidel <[email protected]>

According to kerneljanitors todo list all printk calls (beginning a new line) should have an according KERN_* constant. Those are the changes to the debug macros in the i2c subsystem to meet this requirement. Also changing no-debug statements to raw printks again.

Signed-off-by Frank Seidel <[email protected]>

---
 drivers/i2c/algos/i2c-algo-pca.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Wolfram, I would appreciate if you could test the patch and confirm it
doesn't break anything:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-algo-pca-adapt-debug-macros-for-KERN-constants.patch

Wolfram, I would appreciate if you could test the patch and confirm it
doesn't break anything:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-algo-pca-adapt-debug-macros-for-KERN-constants.patch