i2c: scx200_acb - debug log cleanup

PATCH New Ben Gardner 2 participants 7 messages Dec 19, 2005 – Dec 20, 2005

Subsystems (2)

I2C subsystem host drivers SCx200 CPU support

lore

Share & tools

lore

Ben Gardner

author

2005-12-19 18:51 UTC #0 lore raw

PATCH 2/7 i2c: scx200_acb - debug log cleanup

scx200_acb debug log cleanup.

Signed-off-by: Ben Gardner <bgardner at wabtec.com> -------------- next part --------------

 drivers/i2c/busses/scx200_acb.c |   34 ++++++++++++++++------------------
 1 files changed, 16 insertions(+), 18 deletions(-)

drivers/i2c/busses/scx200_acb.c +16 ~~-18~~

@@ -45,12 +45,6 @@ static int base[MAX_DEVICES] = { 0x820, 
 module_param_array(base, int, NULL, 0);
 MODULE_PARM_DESC(base, "Base addresses for the ACCESS.bus controllers");
 
-#ifdef DEBUG
-#define DBG(x...) printk(KERN_DEBUG NAME ": " x)
-#else
-#define DBG(x...)
-#endif
-
 /* The hardware supports interrupt driven mode too, but I haven't
    implemented that. */
 #define POLLED_MODE	1

@@ -120,8 +114,8 @@ static void scx200_acb_machine(struct sc
 {
 	const char *errmsg;
 
-	DBG("state %s, status = 0x%02x\n",
-	    scx200_acb_state_name[iface->state], status);
+	dev_dbg(&iface->adapter.dev, "state %s, status = 0x%02x\n",
+		scx200_acb_state_name[iface->state], status);
 
 	if (status & ACBST_BER) {
 		errmsg = "bus error";

@@ -210,8 +204,8 @@ static void scx200_acb_machine(struct sc
 	return;
 
  negack:
-	DBG("negative acknowledge in state %s\n",
-	    scx200_acb_state_name[iface->state]);
+	dev_dbg(&iface->adapter.dev, "negative acknowledge in state %s\n",
+		scx200_acb_state_name[iface->state]);
 
 	iface->state  = state_idle;
 	iface->result = -ENXIO;

@@ -327,16 +321,17 @@ static s32 scx200_acb_smbus_xfer(struct 
 		return -EINVAL;
 	}
 
-	DBG("size=%d, address=0x%x, command=0x%x, len=%d, read=%d\n",
-	    size, address, command, len, rw = I2C_SMBUS_READ);
+	dev_dbg(&adapter->dev,
+		"size=%d, address=0x%x, command=0x%x, len=%d, read=%d\n",
+		size, address, command, len, rw);
 
 	if (!len && rw = I2C_SMBUS_READ) {
-		dev_warn(&adapter->dev, "zero length read\n");
+		dev_dbg(&adapter->dev, "zero length read\n");
 		return -EINVAL;
 	}
 
 	if (len && !buffer) {
-		dev_warn(&adapter->dev, "nonzero length but no buffer\n");
+		dev_dbg(&adapter->dev, "nonzero length but no buffer\n");
 		return -EFAULT;
 	}

@@ -376,7 +371,7 @@ static s32 scx200_acb_smbus_xfer(struct 
 		data->word = le16_to_cpu(cur_word);
 
 #ifdef DEBUG
-	DBG(": transfer done, result: %d", rc);
+	dev_dbg(&adapter->dev, "transfer done, result: %d", rc);
 	if (buffer) {
 		int i;
 		printk(" data:");

@@ -413,7 +408,7 @@ static int scx200_acb_probe(struct scx20
 	outb(0x70, ACBCTL2);
 
 	if (inb(ACBCTL2) != 0x70) {
-		DBG("ACBCTL2 readback failed\n");
+		dev_dbg(&iface->adapter.dev, "ACBCTL2 readback failed\n");
 		return -ENXIO;
 	}

@@ -421,7 +416,8 @@ static int scx200_acb_probe(struct scx20
 
 	val = inb(ACBCTL1);
 	if (val) {
-		DBG("disabled, but ACBCTL1=0x%02x\n", val);
+		dev_dbg(&iface->adapter.dev, "disabled, but ACBCTL1=0x%02x\n",
+			val);
 		return -ENXIO;
 	}

@@ -431,7 +427,9 @@ static int scx200_acb_probe(struct scx20
 
 	val = inb(ACBCTL1);
 	if ((val & ACBCTL1_NMINTE) != ACBCTL1_NMINTE) {
-		DBG("enabled, but NMINTE won't be set, ACBCTL1=0x%02x\n", val);
+		dev_dbg(&iface->adapter.dev,
+			"enabled, but NMINTE won't be set, ACBCTL1=0x%02x\n",
+			val);
 		return -ENXIO;
 	}

Jean Delvare

2005-12-19 20:42 UTC #1 lore raw

PATCH 2/7 i2c: scx200_acb - debug log cleanup

Hi Ben,

drivers/i2c/busses/scx200_acb.c static s32 scx200_acb_smbus_xfer(struct · line 331

Quoted 7 lines

scx200_acb debug log cleanup.

drivers/i2c/busses/scx200_acb.c +1 -1

 	if (!len && rw = I2C_SMBUS_READ) {
-		dev_warn(&adapter->dev, "zero length read\n");
+		dev_dbg(&adapter->dev, "zero length read\n");
 		return -EINVAL;
 	}

It just happened to me that this test is bogus. len = 0 && rw = I2C_SMBUS_READ is perfectly valid according to the SMBus specification. It's a "quick command" with data (1 bit) = 1. I never saw it used in practice, and everyone seems to agree that this is a weirdness in SMBus' specification (using the read/write bit as data in this specific case) but it's still valid.

drivers/i2c/busses/scx200_acb.c static s32 scx200_acb_smbus_xfer(struct · line 336

Quoted 5 lines

drivers/i2c/busses/scx200_acb.c +1 -1

 	if (len && !buffer) {
-		dev_warn(&adapter->dev, "nonzero length but no buffer\n");
+		dev_dbg(&adapter->dev, "nonzero length but no buffer\n");
 		return -EFAULT;
 	}

I think that the whole block should be enclosed by #ifdef DEBUG/#endif, as this error condition is simply not supposed to happen.

Signature · 1 line

Jean Delvare

Jean Delvare

2005-12-19 21:35 UTC #2 lore raw

PATCH 2/7 i2c: scx200_acb - debug log cleanup

In reply to Ben Gardner

Hi Ben,

Quoted 22 lines

scx200_acb debug log cleanup.

drivers/i2c/busses/scx200_acb.c +3 -2

@@ -413,7 +408,7 @@ static int scx200_acb_probe(struct scx20
 	outb(0x70, ACBCTL2);
 
 	if (inb(ACBCTL2) != 0x70) {
-		DBG("ACBCTL2 readback failed\n");
+		dev_dbg(&iface->adapter.dev, "ACBCTL2 readback failed\n");
 		return -ENXIO;
 	}

@@ -421,7 +416,8 @@ static int scx200_acb_probe(struct scx20
 
 	val = inb(ACBCTL1);
 	if (val) {
-		DBG("disabled, but ACBCTL1=0x%02x\n", val);
+		dev_dbg(&iface->adapter.dev, "disabled, but ACBCTL1=0x%02x\n",
+			val);
 		return -ENXIO;
 	}

drivers/i2c/busses/scx200_acb.c static int scx200_acb_probe(struct scx20 · line 434

Quoted 9 lines

drivers/i2c/busses/scx200_acb.c +3 -1

@@ -431,7 +427,9 @@ static int scx200_acb_probe(struct scx20
 
 	val = inb(ACBCTL1);
 	if ((val & ACBCTL1_NMINTE) != ACBCTL1_NMINTE) {
-		DBG("enabled, but NMINTE won't be set, ACBCTL1=0x%02x\n", val);
+		dev_dbg(&iface->adapter.dev,
+			"enabled, but NMINTE won't be set, ACBCTL1=0x%02x\n",
+			val);
 		return -ENXIO;
 	}

On second thought, these ones are probably not correct:
scx200_acb_probe is called before i2c_add_adapter, so the adapter's device isn't properly setup at this point. You have to use pr_debug instead of dev_dbg.

This also means that the uses of dev_err and dev_warn in scx200_acb_create are not correct either, and should be changed to boring printk calls.

Signature · 1 line

Jean Delvare

Ben Gardner

author

2005-12-19 22:05 UTC #3 lore raw

PATCH 2/7 i2c: scx200_acb - debug log cleanup

In reply to Ben Gardner

Hi Jean,

Quoted 10 lines · Jean Delvare

Hi Ben,
drivers/i2c/busses/scx200_acb.c static s32 scx200_acb_smbus_xfer(struct · line 331
Quoted 7 lines
scx200_acb debug log cleanup.
      if (!len && rw = I2C_SMBUS_READ) {
drivers/i2c/busses/scx200_acb.c +1 -1
-             dev_warn(&adapter->dev, "zero length read\n");
+             dev_dbg(&adapter->dev, "zero length read\n");
              return -EINVAL;
      }

Quoted 6 lines

It just happened to me that this test is bogus. len = 0 && rw = I2C_SMBUS_READ is perfectly valid according to the SMBus specification. It's a "quick command" with data (1 bit) = 1. I never saw it used in practice, and everyone seems to agree that this is a weirdness in SMBus' specification (using the read/write bit as data in this specific case) but it's still valid.

It looks like the case for state_quick won't work for 0-len reads. It should unconditionally jump to state_write for a 0-len state_quick operation. I'll write up a separate patch to fix that and remove the check tomorrow.

Quoted 8 lines

drivers/i2c/busses/scx200_acb.c static s32 scx200_acb_smbus_xfer(struct · line 336
Quoted 5 lines
      if (len && !buffer) {
drivers/i2c/busses/scx200_acb.c +1 -1
-             dev_warn(&adapter->dev, "nonzero length but no buffer\n");
+             dev_dbg(&adapter->dev, "nonzero length but no buffer\n");
              return -EFAULT;
      }
I think that the whole block should be enclosed by #ifdef DEBUG/#endif, as this error condition is simply not supposed to happen.

I assume other drivers don't check for this... why not just delete it?

Thanks,
Ben

Jean Delvare

2005-12-20 20:17 UTC #4 lore raw

PATCH 2/7 i2c: scx200_acb - debug log cleanup

In reply to Ben Gardner

Hi Ben,

Quoted 10 lines

Quoted 6 lines
It just happened to me that this test is bogus. len = 0 && rw = I2C_SMBUS_READ is perfectly valid according to the SMBus specification. It's a "quick command" with data (1 bit) = 1. I never saw it used in practice, and everyone seems to agree that this is a weirdness in SMBus' specification (using the read/write bit as data in this specific case) but it's still valid.
It looks like the case for state_quick won't work for 0-len reads. It should unconditionally jump to state_write for a 0-len state_quick operation. I'll write up a separate patch to fix that and remove the check tomorrow.

Oops, you may be right. My comment was solely based on my knowledge of the SMBus specification. I did not actually check if the code would handle it properly.

This state machine looks rather strange to me, I don't think I ever saw an i2c bus driver implemented that way.

Quoted 10 lines

Quoted 8 lines
drivers/i2c/busses/scx200_acb.c static s32 scx200_acb_smbus_xfer(struct · line 336
Quoted 5 lines
      if (len && !buffer) {
drivers/i2c/busses/scx200_acb.c +1 -1
-             dev_warn(&adapter->dev, "nonzero length but no buffer\n");
+             dev_dbg(&adapter->dev, "nonzero length but no buffer\n");
              return -EFAULT;
      }
I think that the whole block should be enclosed by #ifdef DEBUG/#endif, as this error condition is simply not supposed to happen.
I assume other drivers don't check for this... why not just delete it?

Sure, that's fine with me too.

Signature · 1 line

Jean Delvare

Ben Gardner

author

2005-12-20 22:37 UTC #5 lore raw

PATCH 2/7 i2c: scx200_acb - debug log cleanup

In reply to Ben Gardner

Hi Jean,

Quoted 3 lines · Jean Delvare

(...)
This state machine looks rather strange to me, I don't think I ever saw an i2c bus driver implemented that way.

Yeah, I think the driver may have been better implemented using the 'emulated' (i2c_transfer) method. The hardware supplies all the
basics: start, stop, stall, write address, write & read byte. All the SMBus stuff has to be emulated anyway.

Ben

Ben Gardner

author

2005-12-20 22:56 UTC #6 lore raw

PATCH 2/7 i2c: scx200_acb - debug log cleanup

In reply to Ben Gardner

scx200_acb: debug log cleanup

Signed-off-by Ben Gardner <bgardner at wabtec.com>

---
This is the second version of the series.
-------------- next part --------------

 drivers/i2c/busses/scx200_acb.c |   42 ++++++++++++++++------------------------
 1 files changed, 17 insertions(+), 25 deletions(-)

drivers/i2c/busses/scx200_acb.c +17 ~~-25~~

@@ -45,12 +45,6 @@ static int base[MAX_DEVICES] = { 0x820, 
 module_param_array(base, int, NULL, 0);
 MODULE_PARM_DESC(base, "Base addresses for the ACCESS.bus controllers");
 
-#ifdef DEBUG
-#define DBG(x...) printk(KERN_DEBUG NAME ": " x)
-#else
-#define DBG(x...)
-#endif
-
 /* The hardware supports interrupt driven mode too, but I haven't
    implemented that. */
 #define POLLED_MODE 1

@@ -119,8 +113,8 @@ static void scx200_acb_machine(struct sc
 {
 	const char *errmsg;
 
-	DBG("state %s, status = 0x%02x\n",
-	    scx200_acb_state_name[iface->state], status);
+	dev_dbg(&iface->adapter.dev, "state %s, status = 0x%02x\n",
+		scx200_acb_state_name[iface->state], status);
 
 	if (status & ACBST_BER) {
 		errmsg = "bus error";

@@ -209,8 +203,8 @@ static void scx200_acb_machine(struct sc
 	return;
 
  negack:
-	DBG("negative acknowledge in state %s\n",
-	    scx200_acb_state_name[iface->state]);
+	dev_dbg(&iface->adapter.dev, "negative ack in state %s\n",
+		scx200_acb_state_name[iface->state]);
 
 	iface->state = state_idle;
 	iface->result = -ENXIO;

@@ -326,19 +320,15 @@ static s32 scx200_acb_smbus_xfer(struct 
 		return -EINVAL;
 	}
 
-	DBG("size=%d, address=0x%x, command=0x%x, len=%d, read=%d\n",
-	    size, address, command, len, rw = I2C_SMBUS_READ);
+	dev_dbg(&adapter->dev,
+		"size=%d, address=0x%x, command=0x%x, len=%d, read=%d\n",
+		size, address, command, len, rw);
 
 	if (!len && rw = I2C_SMBUS_READ) {
-		dev_warn(&adapter->dev, "zero length read\n");
+		dev_dbg(&adapter->dev, "zero length read\n");
 		return -EINVAL;
 	}
 
-	if (len && !buffer) {
-		dev_warn(&adapter->dev, "nonzero length but no buffer\n");
-		return -EFAULT;
-	}
-
 	down(&iface->sem);
 
 	iface->address_byte = address<<1;

@@ -375,7 +365,7 @@ static s32 scx200_acb_smbus_xfer(struct 
 		data->word = le16_to_cpu(cur_word);
 
 #ifdef DEBUG
-	DBG(": transfer done, result: %d", rc);
+	dev_dbg(&adapter->dev, "transfer done, result: %d", rc);
 	if (buffer) {
 		int i;
 		printk(" data:");

@@ -412,7 +402,7 @@ static int scx200_acb_probe(struct scx20
 	outb(0x70, ACBCTL2);
 
 	if (inb(ACBCTL2) != 0x70) {
-		DBG("ACBCTL2 readback failed\n");
+		pr_debug(NAME ": ACBCTL2 readback failed\n");
 		return -ENXIO;
 	}

@@ -420,7 +410,8 @@ static int scx200_acb_probe(struct scx20
 
 	val = inb(ACBCTL1);
 	if (val) {
-		DBG("disabled, but ACBCTL1=0x%02x\n", val);
+		pr_debug(NAME ": disabled, but ACBCTL1=0x%02x\n",
+			val);
 		return -ENXIO;
 	}

@@ -430,7 +421,8 @@ static int scx200_acb_probe(struct scx20
 
 	val = inb(ACBCTL1);
 	if ((val & ACBCTL1_NMINTE) != ACBCTL1_NMINTE) {
-		DBG("enabled, but NMINTE won't be set, ACBCTL1=0x%02x\n", val);
+		pr_debug(NAME ": enabled, but NMINTE won't be set, "
+			 "ACBCTL1=0x%02x\n", val);
 		return -ENXIO;
 	}

@@ -464,7 +456,7 @@ static int  __init scx200_acb_create(int
 	snprintf(description, sizeof(description),
 		 "NatSemi SCx200 ACCESS.bus [%s]", adapter->name);
 	if (request_region(base, 8, description) = 0) {
-		dev_err(&adapter->dev, "can't allocate io 0x%x-0x%x\n",
+		printk(KERN_ERR NAME ": can't allocate io 0x%x-0x%x\n",
 			base, base + 8-1);
 		rc = -EBUSY;
 		goto errout;

@@ -473,14 +465,14 @@ static int  __init scx200_acb_create(int
 
 	rc = scx200_acb_probe(iface);
 	if (rc) {
-		dev_warn(&adapter->dev, "probe failed\n");
+		printk(KERN_WARNING NAME ": probe failed\n");
 		goto errout;
 	}
 
 	scx200_acb_reset(iface);
 
 	if (i2c_add_adapter(adapter) < 0) {
-		dev_err(&adapter->dev, "failed to register\n");
+		printk(KERN_ERR NAME ": failed to register\n");
 		rc = -ENODEV;
 		goto errout;
 	}