[PATCH] i2c: add missing KERN_* constants to printks

[PATCH] i2c: add missing KERN_* constants to printks

[Frank Seidel]

From: Frank Seidel <frank@f-seidel.de>

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 <frank@f-seidel.de>
---
 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(-)

--- a/drivers/i2c/busses/i2c-pca-isa.c
+++ b/drivers/i2c/busses/i2c-pca-isa.c
@@ -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;
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -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;
-	printk("i2c: error: %s\n", why);
-	printk("i2c: msg_num: %d msg_idx: %d msg_ptr: %d\n",
+	printk(KERN_ERR "i2c: error: %s\n", why);
+	printk(KERN_ERR "i2c: msg_num: %d msg_idx: %d msg_ptr: %d\n",
 		i2c->msg_num, i2c->msg_idx, i2c->msg_ptr);
-	printk("i2c: ICR: %08x ISR: %08x\n"
+	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]);
--- a/drivers/i2c/busses/i2c-powermac.c
+++ b/drivers/i2c/busses/i2c-powermac.c
@@ -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);
--- a/drivers/i2c/algos/i2c-algo-pca.c
+++ b/drivers/i2c/algos/i2c-algo-pca.c
@@ -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");
+			DEB2(KERN_WARNING "BUS ERROR - SCL Stuck low\n");
 			pca_reset(adap);
 			goto out;
 		case 0x00: /* Bus error during master or slave mode due to illegal START or STOP condition */
-			DEB2("BUS ERROR - Illegal START or STOP\n");
+			DEB2(KERN_WARNING
+			     "BUS ERROR - Illegal START or STOP\n");
 			pca_reset(adap);
 			goto out;
 		default:
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -61,7 +61,7 @@ static int i2c_debug;
 
 static void i2c_start(struct i2c_algo_pcf_data *adap) 
 {
-	DEBPROTO(printk("S "));
+	DEBPROTO(printk(KERN_WARNING "S "));
 	set_pcf(adap, 1, I2C_PCF_START);
 }
 

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Uwe Kleine-König]

Hello,

On Fri, Feb 06, 2009 at 02:45:14PM +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> 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.
OK in principle.  Still *I* prefer the pr_debug, pr_emerg etc. macros.
Looks a bit nicer ...

> --- a/drivers/i2c/algos/i2c-algo-pca.c
> +++ b/drivers/i2c/algos/i2c-algo-pca.c
> @@ -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");
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.

-#define DEB2(fmt, args...) do { if (i2c_debug>=2) printk(fmt, ## args); } while(0)
+#define DEB2(fmt, args...) do { if (i2c_debug>=2) pr_warning(fmt, ## args); } while(0)

Best regards,
Uwe

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Frank Seidel]

Uwe Kleine-König schrieb:
> 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.

Thanks,
Frank

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Uwe Kleine-König]

Hallo Frank,

On Fri, Feb 06, 2009 at 08:59:44PM +0100, Frank Seidel wrote:
> Uwe Kleine-König schrieb:
> > 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.
Ah, OK.  Then you have my Acked-by.

Best regards
Uwe

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Jean Delvare]

On Fri, 06 Feb 2009 20:59:44 +0100, Frank Seidel wrote:
> 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?

-- 
Jean Delvare

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Frank Seidel]

Jean Delvare wrote:
> 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.

Thanks,
Frank

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Uwe Kleine-König]

On Sat, Feb 07, 2009 at 11:37:48AM +0100, Frank Seidel wrote:
> Jean Delvare wrote:
> > 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.
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.

Best regards
Uwe

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Jean Delvare]

On Sat, 7 Feb 2009 11:52:19 +0100, Uwe Kleine-König wrote:
> On Sat, Feb 07, 2009 at 11:37:48AM +0100, Frank Seidel wrote:
> > Jean Delvare wrote:
> > > 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.
> one little note:  This is changable writing to /proc/sys/kernel/printk.

OK, thanks for the information. I admit I am surprised that there is
such a default log level and that it is considered important enough to
be changeable through /proc/sys, and that on the other hand it is
considered bad to not have a log level for every message. That's
somewhat contradictory.

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

The aim of Frank's patch shouldn't be to preserve the behavior, as
there is no guarantee that the behavior is correct. In fact, for some
of the messages it is very obvious that it is not. The aim should be to
pick the appropriate log level for every message. If not then I don't
see any point in fixing the messages.

-- 
Jean Delvare

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Jean Delvare]

Hi Frank,

On Fri, 06 Feb 2009 14:45:14 +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> 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 <frank@f-seidel.de>
> ---
>  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(-)
> 
> --- a/drivers/i2c/busses/i2c-pca-isa.c
> +++ b/drivers/i2c/busses/i2c-pca-isa.c
> @@ -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;
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -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;
> -	printk("i2c: error: %s\n", why);
> -	printk("i2c: msg_num: %d msg_idx: %d msg_ptr: %d\n",
> +	printk(KERN_ERR "i2c: error: %s\n", why);
> +	printk(KERN_ERR "i2c: msg_num: %d msg_idx: %d msg_ptr: %d\n",
>  		i2c->msg_num, i2c->msg_idx, i2c->msg_ptr);
> -	printk("i2c: ICR: %08x ISR: %08x\n"
> +	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.

> --- a/drivers/i2c/busses/i2c-powermac.c
> +++ b/drivers/i2c/busses/i2c-powermac.c
> @@ -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);
> --- a/drivers/i2c/algos/i2c-algo-pca.c
> +++ b/drivers/i2c/algos/i2c-algo-pca.c
> @@ -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");
> +			DEB2(KERN_WARNING "BUS ERROR - SCL Stuck low\n");
>  			pca_reset(adap);
>  			goto out;
>  		case 0x00: /* Bus error during master or slave mode due to illegal START or STOP condition */
> -			DEB2("BUS ERROR - Illegal START or STOP\n");
> +			DEB2(KERN_WARNING
> +			     "BUS ERROR - Illegal START or STOP\n");
>  			pca_reset(adap);
>  			goto out;
>  		default:

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.

> --- a/drivers/i2c/algos/i2c-algo-pcf.c
> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
> @@ -61,7 +61,7 @@ static int i2c_debug;
>  
>  static void i2c_start(struct i2c_algo_pcf_data *adap) 
>  {
> -	DEBPROTO(printk("S "));
> +	DEBPROTO(printk(KERN_WARNING "S "));
>  	set_pcf(adap, 1, I2C_PCF_START);
>  }

This one is a clear KERN_DEBUG.

-- 
Jean Delvare

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Frank Seidel]

Hi,

Jean Delvare wrote:
>> +	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.

> 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.

Ok, i fully agree with you. So, i'll redo the patch asap.

Thanks for your feedback,
Frank

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Jean Delvare]

On Sun, 08 Feb 2009 14:11:52 +0100, Frank Seidel wrote:
> Hi,
> 
> Jean Delvare wrote:
> >> +	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.

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

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

-- 
Jean Delvare

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Frank Seidel]

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

I changed it a bit to have the loglevel at the real beginning of the
line.

Thanks,
Frank

[PATCHv2 1/2] i2c: add missing KERN_* constants to printks

[Frank Seidel]

From: Frank Seidel <frank@f-seidel.de>

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 <frank@f-seidel.de>
---
 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(-)

--- linux-2.6.orig/drivers/i2c/algos/i2c-algo-pcf.c
+++ linux-2.6/drivers/i2c/algos/i2c-algo-pcf.c
@@ -61,7 +61,7 @@ static int i2c_debug;
 
 static void i2c_start(struct i2c_algo_pcf_data *adap) 
 {
-	DEBPROTO(printk("S "));
+	DEBPROTO(printk(KERN_DEBUG "S "));
 	set_pcf(adap, 1, I2C_PCF_START);
 }
 
--- linux-2.6.orig/drivers/i2c/busses/i2c-pca-isa.c
+++ linux-2.6/drivers/i2c/busses/i2c-pca-isa.c
@@ -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;
--- linux-2.6.orig/drivers/i2c/busses/i2c-powermac.c
+++ linux-2.6/drivers/i2c/busses/i2c-powermac.c
@@ -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);
--- linux-2.6.orig/drivers/i2c/busses/i2c-pxa.c
+++ linux-2.6/drivers/i2c/busses/i2c-pxa.c
@@ -210,11 +210,12 @@ 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;
-	printk("i2c: error: %s\n", why);
-	printk("i2c: msg_num: %d msg_idx: %d msg_ptr: %d\n",
+	printk(KERN_ERR "i2c: error: %s\n", why);
+	printk(KERN_ERR "i2c: msg_num: %d msg_idx: %d msg_ptr: %d\n",
 		i2c->msg_num, i2c->msg_idx, i2c->msg_ptr);
-	printk("i2c: ICR: %08x ISR: %08x\n"
-	       "i2c: log: ", readl(_ICR(i2c)), readl(_ISR(i2c)));
+	printk(KERN_ERR "i2c: ICR: %08x ISR: %08x\n"
+	       readl(_ICR(i2c)), readl(_ISR(i2c)));
+	printk(KERN_DEBUG "i2c: log: ");
 	for (i = 0; i < i2c->irqlogidx; i++)
 		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
 	printk("\n");

[PATCHv2 2/2] i2c: adapt debug macros for KERN_* constants

[Frank Seidel]

From: Frank Seidel <frank@f-seidel.de>

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 <frank@f-seidel.de>
---
 drivers/i2c/algos/i2c-algo-pca.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

--- a/drivers/i2c/algos/i2c-algo-pca.c
+++ b/drivers/i2c/algos/i2c-algo-pca.c
@@ -27,9 +27,12 @@
 #include <linux/i2c.h>
 #include <linux/i2c-algo-pca.h>
 
-#define DEB1(fmt, args...) do { if (i2c_debug>=1) printk(fmt, ## args); } while(0)
-#define DEB2(fmt, args...) do { if (i2c_debug>=2) printk(fmt, ## args); } while(0)
-#define DEB3(fmt, args...) do { if (i2c_debug>=3) printk(fmt, ## args); } while(0)
+#define DEB1(fmt, args...) do { if (i2c_debug >= 1)			\
+				 printk(KERN_DEBUG fmt, ## args); } while (0)
+#define DEB2(fmt, args...) do { if (i2c_debug >= 2)			\
+				 printk(KERN_DEBUG fmt, ## args); } while (0)
+#define DEB3(fmt, args...) do { if (i2c_debug >= 3)			\
+				 printk(KERN_DEBUG fmt, ## args); } while (0)
 
 static int i2c_debug;
 
@@ -313,7 +316,7 @@ static int pca_xfer(struct i2c_adapter *
 
 	ret = curmsg;
  out:
-	DEB1(KERN_CRIT "}}} transfered %d/%d messages. "
+	DEB1("}}} transfered %d/%d messages. "
 	     "status is %#04x. control is %#04x\n",
 	     curmsg, num, pca_status(adap),
 	     pca_get_con(adap));
@@ -347,7 +350,8 @@ static int pca_init(struct i2c_adapter *
 	pca_reset(pca_data);
 
 	clock = pca_clock(pca_data);
-	DEB1(KERN_INFO "%s: Clock frequency is %dkHz\n", adap->name, freqs[clock]);
+	printk(KERN_INFO "%s: Clock frequency is %dkHz\n", adap->name,
+	       freqs[clock]);
 
 	pca_set_con(pca_data, I2C_PCA_CON_ENSIO | clock);
 	udelay(500); /* 500 us for oscilator to stabilise */

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

> I mean the (first) printk in the loop, not the last printk in this
> function. So what I am suggesting is:
> 
> +	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.

Bye,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] i2c: add missing KERN_* constants to printks

[Jean Delvare]

On Mon, 9 Feb 2009 09:35:37 +0100, Wolfram Sang wrote:
> > I mean the (first) printk in the loop, not the last printk in this
> > function. So what I am suggesting is:
> > 
> > +	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.

Correct, however the code was already wrong before Frank's patch, and
his patch is fixing a different problem. So I don't have any problem
with this, this can be fixed in a later patch if anybody cares.

-- 
Jean Delvare

Re: [PATCHv2 1/2] i2c: add missing KERN_* constants to printks

[Jean Delvare]

On Sun, 08 Feb 2009 15:18:49 +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> 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 <frank@f-seidel.de>
> ---
>  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(-)

Applied, thanks.

-- 
Jean Delvare

Re: [PATCHv2 2/2] i2c: adapt debug macros for KERN_* constants

[Jean Delvare]

On Sun, 08 Feb 2009 15:20:31 +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> 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 <frank@f-seidel.de>
> ---
>  drivers/i2c/algos/i2c-algo-pca.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Applied, thanks.

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

Thanks,
-- 
Jean Delvare

Re: [PATCHv2 2/2] i2c: adapt debug macros for KERN_* constants

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

> 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

Will do, but may take until the end of the week. Patch looks good,
though.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCHv2 2/2] i2c: adapt debug macros for KERN_* constants

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

> 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

Better late than never: Works for me.

Tested-by: Wolfram Sang <w.sang@pengutronix.de>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]