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

PATCH Superseded Andreas Werner 2 participants 8 messages Nov 17, 2013 I2C subsystem host drivers

Share & tools

Andreas Werner

author

2013-11-17 08:58 UTC #0 lore raw

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

Revision 2:
- delete the pch_err completly instead of changing to pch_dbg because there is already a pch_dbg at the function who calls pch_i2c_getack.
- Fixed message line issue

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. The pch_i2c_wait_for_check_xfer function is the only one who is calling pch_i2c_getack, and there is already a dbg print if it fails, so we can delete the pch_err in pch_i2c_getack completly.

Fixed print message to be a one liner so we can grep for the error message.

Tested on Intel Atom E6xx and Eg20t Chipset.

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

---
 drivers/i2c/busses/i2c-eg20t.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/i2c/busses/i2c-eg20t.c +2 -5

index 0f37529..5c39f90 100644
@@ -321,10 +321,8 @@ static s32 pch_i2c_getack(struct i2c_algo_pch_data *adap)
 	void __iomem *p = adap->pch_base_address;
 	reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
 
-	if (reg_val != 0) {
-		pch_err(adap, "return%d\n", -EPROTO);
+	if (reg_val != 0)
 		return -EPROTO;
-	}
 
 	return 0;
 }

@@ -367,8 +365,7 @@ static int pch_i2c_wait_for_check_xfer(struct i2c_algo_pch_data *adap)
 	adap->pch_event_flag = 0;
 
 	if (pch_i2c_getack(adap)) {
-		pch_dbg(adap, "Receive NACK for slave address"
-			"setting\n");
+		pch_dbg(adap, "Receive NACK for slave address setting\n");
 		return -EIO;
 	}

Signature · 1 line

1.8.4

Wolfram Sang

2013-11-17 11:08 UTC #1 lore raw

Quoted 5 lines · Andreas Werner

Revision 2:
- delete the pch_err completly instead of changing to pch_dbg because there is already a pch_dbg at the function who calls pch_i2c_getack.
- Fixed message line issue

I prefer this below "---" after Signed-off.

drivers/i2c/busses/i2c-eg20t.c static s32 pch_i2c_getack(struct i2c_algo_pch_data *adap) · line 325

Quoted 13 lines

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

index 0f37529..5c39f90 100644
@@ -321,10 +321,8 @@ static s32 pch_i2c_getack(struct i2c_algo_pch_data *adap)
 	void __iomem *p = adap->pch_base_address;
 	reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
 
-	if (reg_val != 0) {
-		pch_err(adap, "return%d\n", -EPROTO);
+	if (reg_val != 0)
 		return -EPROTO;

That could be fixed to -ENXIO according to
Documentation/i2c/fault-codes.

drivers/i2c/busses/i2c-eg20t.c static int pch_i2c_wait_for_check_xfer(struct i2c_algo_pch_data *adap) · line 369

Quoted 8 lines

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

@@ -367,8 +365,7 @@ static int pch_i2c_wait_for_check_xfer(struct i2c_algo_pch_data *adap)
 	adap->pch_event_flag = 0;
 
 	if (pch_i2c_getack(adap)) {
-		pch_dbg(adap, "Receive NACK for slave address"
-			"setting\n");
+		pch_dbg(adap, "Receive NACK for slave address setting\n");
 		return -EIO;

What about returning the value we got from pch_i2c_getack?

Regards,

Wolfram

Andreas Werner

author

2013-11-17 11:39 UTC #2 lore raw

Quoted 8 lines · Wolfram Sang

Quoted 5 lines · Andreas Werner
Revision 2:
- delete the pch_err completly instead of changing to pch_dbg because there is already a pch_dbg at the function who calls pch_i2c_getack.
- Fixed message line issue
I prefer this below "---" after Signed-off.

Ok i will change it next time.

Quoted 10 lines

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

index 0f37529..5c39f90 100644
@@ -321,10 +321,8 @@ static s32 pch_i2c_getack(struct i2c_algo_pch_data *adap)
 	void __iomem *p = adap->pch_base_address;
 	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 325
Quoted 4 lines
drivers/i2c/busses/i2c-eg20t.c +1 -2
-	if (reg_val != 0) {
-		pch_err(adap, "return%d\n", -EPROTO);
+	if (reg_val != 0)
 		return -EPROTO;
That could be fixed to -ENXIO according to
Documentation/i2c/fault-codes.

Yes you are right -ENXIO should returned if no ACK was received. Is there another reason why pch_i2c_getack returned EPROTO? May be ENXIO was introduced later?

Quoted 4 lines

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

@@ -367,8 +365,7 @@ static int pch_i2c_wait_for_check_xfer(struct i2c_algo_pch_data *adap)
 	adap->pch_event_flag = 0;

Quoted 8 lines

drivers/i2c/busses/i2c-eg20t.c static int pch_i2c_wait_for_check_xfer(struct i2c_algo_pch_data *adap) · line 369
Quoted 5 lines
drivers/i2c/busses/i2c-eg20t.c +1 -2
 	if (pch_i2c_getack(adap)) {
-		pch_dbg(adap, "Receive NACK for slave address"
-			"setting\n");
+		pch_dbg(adap, "Receive NACK for slave address setting\n");
 		return -EIO;
What about returning the value we got from pch_i2c_getack?

EIO is almost ok, because EIO means something went wrong when performing an I/O operation, but yes we can return the value from pch_i2c_getack (ENXIO) to get more detailed informations whats going wrong exactly (no ACK reveiced).

I think we can just replace the -EIO with -ENXIO or do you want to pick up the return vale of pch_i2c_getack and return that ?

Regards

Andy

Quoted 4 lines

Regards,
Wolfram

Wolfram Sang

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

Quoted 3 lines

Is there another reason why pch_i2c_getack returned EPROTO? May be ENXIO was introduced later?

Imperfect review :)

Quoted 2 lines

I think we can just replace the -EIO with -ENXIO or do you want to pick up the return vale of pch_i2c_getack and return that ?

The latter. As a rule of thumb, it is usually more sustainable to pass through error codes. Overloading them should only be done when really necessary IMO.

Andreas Werner

author

2013-11-17 16:53 UTC #4 lore raw

Quoted 4 lines · Wolfram Sang

Quoted 3 lines
Is there another reason why pch_i2c_getack returned EPROTO? May be ENXIO was introduced later?

Quoted 9 lines

Imperfect review :)
Quoted 2 lines
I think we can just replace the -EIO with -ENXIO or do you want to pick up the return vale of pch_i2c_getack and return that ?
The latter. As a rule of thumb, it is usually more sustainable to pass through error codes. Overloading them should only be done when really necessary IMO.

Ok, if that will be ok in pch_i2c_wait_for_check_xfer i will resend the patch.

        ret = pch_i2c_getack(adap);

        if (ret)
                pch_dbg(adap, "Receive NACK for slave address setting\n");

        return (int)ret;

Regards
Andy

Wolfram Sang

2013-11-17 17:08 UTC #5 lore raw

Quoted 17 lines · Andreas Werner

Quoted 4 lines · Wolfram Sang
Quoted 3 lines
Is there another reason why pch_i2c_getack returned EPROTO? May be ENXIO was introduced later?
Quoted 9 lines
Imperfect review :)
Quoted 2 lines
I think we can just replace the -EIO with -ENXIO or do you want to pick up the return vale of pch_i2c_getack and return that ?
The latter. As a rule of thumb, it is usually more sustainable to pass through error codes. Overloading them should only be done when really necessary IMO.
Ok, if that will be ok in pch_i2c_wait_for_check_xfer i will resend the patch.

Quoted 6 lines

        ret = pch_i2c_getack(adap);

        if (ret)
                pch_dbg(adap, "Receive NACK for slave address setting\n");

        return (int)ret;

Hmm, the cast looks ugly. Looking at the driver more closely, my preferred solution would be to elimiate the getack function and just do that directly in wait_for_check_xfer:

	if (ioread32(adap->pch_base_address + PCH_I2CSR) & PCH_GETACK) {
		pch_dbg ...
		return -ENXIO;
	}

Something like that...

Andreas Werner

author

2013-11-17 17:16 UTC #6 lore raw

Quoted 29 lines · Wolfram Sang

Quoted 17 lines · Andreas Werner
Quoted 4 lines · Wolfram Sang
Quoted 3 lines
Is there another reason why pch_i2c_getack returned EPROTO? May be ENXIO was introduced later?
Quoted 9 lines
Imperfect review :)
Quoted 2 lines
I think we can just replace the -EIO with -ENXIO or do you want to pick up the return vale of pch_i2c_getack and return that ?
The latter. As a rule of thumb, it is usually more sustainable to pass through error codes. Overloading them should only be done when really necessary IMO.
Ok, if that will be ok in pch_i2c_wait_for_check_xfer i will resend the patch.
Quoted 6 lines
        ret = pch_i2c_getack(adap);

        if (ret)
                pch_dbg(adap, "Receive NACK for slave address setting\n");

        return (int)ret;
Hmm, the cast looks ugly. Looking at the driver more closely, my preferred solution would be to elimiate the getack function and just do that directly in wait_for_check_xfer:

Quoted 7 lines

	if (ioread32(adap->pch_base_address + PCH_I2CSR) & PCH_GETACK) {
		pch_dbg ...
		return -ENXIO;
	}

Something like that...

Sometimes its really usfull to look closely :-)
I agree you, because the function is just called one time, so we can really delete this function.

regards
Andy

Wolfram Sang

2013-11-17 17:31 UTC #7 lore raw

Quoted 2 lines

Sometimes its really usfull to look closely :-)

I can't do this for every patch right from the beginning since my time is limited. I usually start with letting people sort it out themselves.