I2C: busses: Do not print error message in syslog if no ACK received

PATCH Superseded Andreas Werner 3 participants 8 messages Nov 12, 2013 – Nov 14, 2013 I2C subsystem host drivers

Share & tools

Andreas Werner

author

2013-11-12 17:45 UTC #0 lore raw

PATCH 1/1 I2C: busses: Do not print error message in syslog if no ACK received

Using the i2c-eg20t driver and call i2cdetect or probe on the bus, the driver will print a lot of error messages if there was no ACK received.

i2cdetect normally print a table with all the available devices. If there is no device on the address, the table will be empty. Currently with the i2c-eg20t driver, the table is not visible because the error messages destroy the table.

Error message: pch_i2c_getack return -71

This patch prevent the driver to print the messages to syslog if debug is not set.

Tested on Intel Atom E6xx and Eg20t Chipset.

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/i2c/busses/i2c-eg20t.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index 0f37529..b10c651 100644
@@ -322,7 +322,7 @@ static s32 pch_i2c_getack(struct i2c_algo_pch_data *adap)
 	reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
 
 	if (reg_val != 0) {
-		pch_err(adap, "return%d\n", -EPROTO);
+		pch_dbg(adap, "return%d\n", -EPROTO);
 		return -EPROTO;
 	}

Signature · 1 line

1.8.4

Marek Vasut

2013-11-12 18:00 UTC #1 lore raw

Dear Andreas Werner,

Quoted 26 lines

Using the i2c-eg20t driver and call i2cdetect or probe on the bus, the driver will print a lot of error messages if there was no ACK received.
i2cdetect normally print a table with all the available devices. If there is no device on the address, the table will be empty. Currently with the i2c-eg20t driver, the table is not visible because the error messages destroy the table.
Error message: pch_i2c_getack return -71
This patch prevent the driver to print the messages to syslog if debug is not set.
Tested on Intel Atom E6xx and Eg20t Chipset.
Signed-off-by Andreas Werner <[email protected]>
---
 drivers/i2c/busses/i2c-eg20t.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644

drivers/i2c/busses/i2c-eg20t.c static s32 pch_i2c_getack(struct i2c_algo_pch_data · line 327

Quoted 7 lines

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

@@ -322,7 +322,7 @@ static s32 pch_i2c_getack(struct i2c_algo_pch_data
 *adap) reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
 
 	if (reg_val != 0) {
-		pch_err(adap, "return%d\n", -EPROTO);
+		pch_dbg(adap, "return%d\n", -EPROTO);
 		return -EPROTO;
 	}

I'd just kill the pch_err() line altogether:

if (reg_val)

	return -EPROTO;

That is because if you look at the only caller of this function, which is pch_i2c_wait_for_check_xfer(), you will see that at the only place where pch_i2c_getack() is called there is already pch_dbg():

369 if (pch_i2c_getack(adap)) {
370 pch_dbg(adap, "Receive NACK for slave address"
371 "setting\n");
372 return -EIO;
373 }

btw. (idea for subsequent patch) this pch_dbg() should be fixed to be a one- liner /wrt the text so we can grep for the error messages. Such text arg should never be split in multiple lines in kernel.

Best regards,
Marek Vasut

Andreas Werner

author

2013-11-12 18:23 UTC #2 lore raw

Quoted 30 lines · Marek Vasut

Dear Andreas Werner,
Quoted 28 lines
Using the i2c-eg20t driver and call i2cdetect or probe on the bus, the driver will print a lot of error messages if there was no ACK received.
i2cdetect normally print a table with all the available devices. If there is no device on the address, the table will be empty. Currently with the i2c-eg20t driver, the table is not visible because the error messages destroy the table.
Error message: pch_i2c_getack return -71
This patch prevent the driver to print the messages to syslog if debug is not set.
Tested on Intel Atom E6xx and Eg20t Chipset.
Signed-off-by Andreas Werner <[email protected]>
---
 drivers/i2c/busses/i2c-eg20t.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644
drivers/i2c/busses/i2c-eg20t.c +0 -0
@@ -322,7 +322,7 @@ static s32 pch_i2c_getack(struct i2c_algo_pch_data
 *adap) reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
 

Quoted 7 lines

drivers/i2c/busses/i2c-eg20t.c static s32 pch_i2c_getack(struct i2c_algo_pch_data *adap) · line 327
Quoted 5 lines
drivers/i2c/busses/i2c-eg20t.c +1 -1
 	if (reg_val != 0) {
-		pch_err(adap, "return%d\n", -EPROTO);
+		pch_dbg(adap, "return%d\n", -EPROTO);
 		return -EPROTO;
 	}
I'd just kill the pch_err() line altogether:

But why? I think there a few pch_err message which are interesting to have in syslog, without building the dbg driver.

Quoted 14 lines

if (reg_val)
	return -EPROTO;
That is because if you look at the only caller of this function, which is pch_i2c_wait_for_check_xfer(), you will see that at the only place where pch_i2c_getack() is called there is already pch_dbg():
369 if (pch_i2c_getack(adap)) {
370 pch_dbg(adap, "Receive NACK for slave address"
371 "setting\n");
372 return -EIO;
373 }

Quoted 3 lines

btw. (idea for subsequent patch) this pch_dbg() should be fixed to be a one- liner /wrt the text so we can grep for the error messages. Such text arg should never be split in multiple lines in kernel.

Ok i will wait a few days for further comments to this patch and then i will resend the patch with a one liner message.

Quoted 3 lines

Best regards,
Marek Vasut

Marek Vasut

2013-11-12 18:33 UTC #3 lore raw

Dear Andreas Werner,

Quoted 37 lines · Marek Vasut

Quoted 36 lines
Dear Andreas Werner,
Quoted 34 lines
Using the i2c-eg20t driver and call i2cdetect or probe on the bus, the driver will print a lot of error messages if there was no ACK received.
i2cdetect normally print a table with all the available devices. If there is no device on the address, the table will be empty. Currently with the i2c-eg20t driver, the table is not visible because the error messages destroy the table.
Error message: pch_i2c_getack return -71
This patch prevent the driver to print the messages to syslog if debug is not set.
Tested on Intel Atom E6xx and Eg20t Chipset.
Signed-off-by Andreas Werner <[email protected]>
---
 drivers/i2c/busses/i2c-eg20t.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644
drivers/i2c/busses/i2c-eg20t.c +1 -1
@@ -322,7 +322,7 @@ static s32 pch_i2c_getack(struct i2c_algo_pch_data
 *adap) reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
 
 	if (reg_val != 0) {
 
-		pch_err(adap, "return%d\n", -EPROTO);
+		pch_dbg(adap, "return%d\n", -EPROTO);
 

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
drivers/i2c/busses/i2c-eg20t.c +0 -0
 		return -EPROTO;
}
I'd just kill the pch_err() line altogether:
But why? I think there a few pch_err message which are interesting to
have in syslog, without building the dbg driver.

See below for explanation why this one particular pch_err() line can go. Sorry if I was not clear enough, I meant only this one particular pch_err() line, NOT all of them.

Quoted 15 lines

Quoted 15 lines
if (reg_val)
	return -EPROTO;
That is because if you look at the only caller of this function, which is pch_i2c_wait_for_check_xfer(), you will see that at the only place where pch_i2c_getack() is called there is already pch_dbg():
369 if (pch_i2c_getack(adap)) {
370 pch_dbg(adap, "Receive NACK for slave address"
371 "setting\n");
372 return -EIO;
373 }

Quoted 6 lines

Quoted 3 lines
btw. (idea for subsequent patch) this pch_dbg() should be fixed to be a one- liner /wrt the text so we can grep for the error messages. Such text arg should never be split in multiple lines in kernel.
Ok i will wait a few days for further comments to this patch and then i will resend the patch with a one liner message.

Sure, wait at least for Wolfram's consent on this please ;-)

Best regards,
Marek Vasut

Andreas Werner

author

2013-11-13 18:24 UTC #4 lore raw

Quoted 44 lines · Marek Vasut

Dear Andreas Werner,
Quoted 42 lines · Marek Vasut
Quoted 41 lines
Dear Andreas Werner,
Quoted 36 lines
Using the i2c-eg20t driver and call i2cdetect or probe on the bus, the driver will print a lot of error messages if there was no ACK received.
i2cdetect normally print a table with all the available devices. If there is no device on the address, the table will be empty. Currently with the i2c-eg20t driver, the table is not visible because the error messages destroy the table.
Error message: pch_i2c_getack return -71
This patch prevent the driver to print the messages to syslog if debug is not set.
Tested on Intel Atom E6xx and Eg20t Chipset.
Signed-off-by Andreas Werner <[email protected]>
---
 drivers/i2c/busses/i2c-eg20t.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644
drivers/i2c/busses/i2c-eg20t.c +1 -1
@@ -322,7 +322,7 @@ static s32 pch_i2c_getack(struct i2c_algo_pch_data
 *adap) reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
 
 	if (reg_val != 0) {
 
-		pch_err(adap, "return%d\n", -EPROTO);
+		pch_dbg(adap, "return%d\n", -EPROTO);
 
 		return -EPROTO;
Quoted 1 line
}
I'd just kill the pch_err() line altogether:

Quoted 22 lines

Quoted 2 lines
But why? I think there a few pch_err message which are interesting to
have in syslog, without building the dbg driver.
See below for explanation why this one particular pch_err() line can go. Sorry if I was not clear enough, I meant only this one particular pch_err() line, NOT all of them.
Quoted 15 lines
Quoted 15 lines
if (reg_val)
	return -EPROTO;
That is because if you look at the only caller of this function, which is pch_i2c_wait_for_check_xfer(), you will see that at the only place where pch_i2c_getack() is called there is already pch_dbg():
369 if (pch_i2c_getack(adap)) {
370 pch_dbg(adap, "Receive NACK for slave address"
371 "setting\n");
372 return -EIO;
373 }

Sorry i misunderstood that. You are absolutly right, thats the best solution for that. Remove the pch_err at getack so that only the pch_dbg get printed where getack is called. This should be enough information.

Quoted 11 lines

Quoted 6 lines
Quoted 3 lines
btw. (idea for subsequent patch) this pch_dbg() should be fixed to be a one- liner /wrt the text so we can grep for the error messages. Such text arg should never be split in multiple lines in kernel.
Ok i will wait a few days for further comments to this patch and then i will resend the patch with a one liner message.
Sure, wait at least for Wolfram's consent on this please ;-)
Best regards,
Marek Vasut

Marek Vasut

2013-11-13 19:24 UTC #5 lore raw

Dear Andreas Werner,

Quoted 69 lines · Marek Vasut

Quoted 45 lines
Dear Andreas Werner,
Quoted 43 lines · Marek Vasut
Quoted 42 lines
Dear Andreas Werner,
Quoted 30 lines
Using the i2c-eg20t driver and call i2cdetect or probe on the bus, the driver will print a lot of error messages if there was no ACK received.
i2cdetect normally print a table with all the available devices. If there is no device on the address, the table will be empty. Currently with the i2c-eg20t driver, the table is not visible because the error messages destroy the table.
Error message: pch_i2c_getack return -71
This patch prevent the driver to print the messages to syslog if debug is not set.
Tested on Intel Atom E6xx and Eg20t Chipset.
Signed-off-by Andreas Werner <[email protected]>
---
 drivers/i2c/busses/i2c-eg20t.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644
drivers/i2c/busses/i2c-eg20t.c +0 -0
@@ -322,7 +322,7 @@ static s32 pch_i2c_getack(struct
i2c_algo_pch_data *adap) reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
drivers/i2c/busses/i2c-eg20t.c static s32 pch_i2c_getack(struct i2c_algo_pch_data *adap)
Quoted 8 lines
drivers/i2c/busses/i2c-eg20t.c +1 -1
 	if (reg_val != 0) {
 
-		pch_err(adap, "return%d\n", -EPROTO);
+		pch_dbg(adap, "return%d\n", -EPROTO);
 
 		return -EPROTO;
 
 	}
I'd just kill the pch_err() line altogether:
Quoted 22 lines
Quoted 3 lines
But why? I think there a few pch_err message which are interesting to
have in syslog, without building the dbg driver.
See below for explanation why this one particular pch_err() line can go. Sorry if I was not clear enough, I meant only this one particular pch_err() line, NOT all of them.
Quoted 14 lines
Quoted 14 lines
if (reg_val)
	return -EPROTO;
That is because if you look at the only caller of this function, which is pch_i2c_wait_for_check_xfer(), you will see that at the only place where pch_i2c_getack() is called there is already pch_dbg():
369 if (pch_i2c_getack(adap)) {
370 pch_dbg(adap, "Receive NACK for slave address"
371 "setting\n");
372 return -EIO;
373 }

Quoted 3 lines

Sorry i misunderstood that. You are absolutly right, thats the best solution for that. Remove the pch_err at getack so that only the pch_dbg get printed where getack is called. This should be enough information.

No problem ;-)

Joe Perches

2013-11-14 13:39 UTC #6 lore raw

Quoted 12 lines · Marek Vasut

Quoted 12 lines · Marek Vasut
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
That is because if you look at the only caller of this function, which is pch_i2c_wait_for_check_xfer(), you will see that at the only place where pch_i2c_getack() is called there is already pch_dbg():
369 if (pch_i2c_getack(adap)) {
370 pch_dbg(adap, "Receive NACK for slave address"
371 "setting\n");
372 return -EIO;
373 }

Quoted 5 lines

Quoted 3 lines
Sorry i misunderstood that. You are absolutly right, thats the best solution for that. Remove the pch_err at getack so that only the pch_dbg get printed where getack is called. This should be enough information.
No problem ;-)

If this is the approach taken, please coalesce the format. As is there's a missing space between address and setting. This should be:

		pch_dbg(adap, "Received NACK for slave address setting\n");

Andreas Werner

author

2013-11-14 18:26 UTC #7 lore raw

Quoted 17 lines · Joe Perches

Quoted 16 lines · Marek Vasut
Quoted 15 lines · Marek Vasut
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
That is because if you look at the only caller of this function, which is pch_i2c_wait_for_check_xfer(), you will see that at the only place where pch_i2c_getack() is called there is already pch_dbg():
369 if (pch_i2c_getack(adap)) {
370 pch_dbg(adap, "Receive NACK for slave address"
371 "setting\n");
372 return -EIO;
373 }
Sorry i misunderstood that. You are absolutly right, thats the best solution for that. Remove the pch_err at getack so that only the pch_dbg get printed where getack is called. This should be enough information.

Quoted 9 lines

Quoted 1 line
No problem ;-)
If this is the approach taken, please coalesce the format. As is there's a missing space between address and setting. This should be:
		pch_dbg(adap, "Received NACK for slave address setting\n");

Hi,
yes you are right, i will also add it and resand the patch in a few days.

regards
Andy