| Message ID | 1384278336-3694-1-git-send-email-wernerandy@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
Dear Andreas Werner, > 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 <wernerandy@gmx.de> > --- > drivers/i2c/busses/i2c-eg20t.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-eg20t.c > b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644 > --- a/drivers/i2c/busses/i2c-eg20t.c > +++ b/drivers/i2c/busses/i2c-eg20t.c > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 12, 2013 at 07:00:59PM +0100, Marek Vasut wrote: > Dear Andreas Werner, > > > 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 <wernerandy@gmx.de> > > --- > > drivers/i2c/busses/i2c-eg20t.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/i2c-eg20t.c > > b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644 > > --- a/drivers/i2c/busses/i2c-eg20t.c > > +++ b/drivers/i2c/busses/i2c-eg20t.c > > @@ -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: But why? I think there a few pch_err message which are interesting to have in syslog, without building the dbg driver. > > 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. 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. > > Best regards, > Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Andreas Werner, > On Tue, Nov 12, 2013 at 07:00:59PM +0100, Marek Vasut wrote: > > Dear Andreas Werner, > > > > > 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 <wernerandy@gmx.de> > > > --- > > > > > > drivers/i2c/busses/i2c-eg20t.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-eg20t.c > > > b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644 > > > --- a/drivers/i2c/busses/i2c-eg20t.c > > > +++ b/drivers/i2c/busses/i2c-eg20t.c > > > @@ -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: > 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. > > > 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 12, 2013 at 07:33:40PM +0100, Marek Vasut wrote: > Dear Andreas Werner, > > > On Tue, Nov 12, 2013 at 07:00:59PM +0100, Marek Vasut wrote: > > > Dear Andreas Werner, > > > > > > > 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 <wernerandy@gmx.de> > > > > --- > > > > > > > > drivers/i2c/busses/i2c-eg20t.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/i2c/busses/i2c-eg20t.c > > > > b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644 > > > > --- a/drivers/i2c/busses/i2c-eg20t.c > > > > +++ b/drivers/i2c/busses/i2c-eg20t.c > > > > @@ -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: > > 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. > > > > > > 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. > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Andreas Werner, > On Tue, Nov 12, 2013 at 07:33:40PM +0100, Marek Vasut wrote: > > Dear Andreas Werner, > > > > > On Tue, Nov 12, 2013 at 07:00:59PM +0100, Marek Vasut wrote: > > > > Dear Andreas Werner, > > > > > > > > > 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 <wernerandy@gmx.de> > > > > > --- > > > > > > > > > > drivers/i2c/busses/i2c-eg20t.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/i2c/busses/i2c-eg20t.c > > > > > b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644 > > > > > --- a/drivers/i2c/busses/i2c-eg20t.c > > > > > +++ b/drivers/i2c/busses/i2c-eg20t.c > > > > > @@ -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: > > > 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. > > > > > > 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. No problem ;-) -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-11-13 at 20:24 +0100, Marek Vasut wrote: > > On Tue, Nov 12, 2013 at 07:33:40PM +0100, Marek Vasut wrote: > > > > > 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. > > 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"); -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 14, 2013 at 05:39:52AM -0800, Joe Perches wrote: > On Wed, 2013-11-13 at 20:24 +0100, Marek Vasut wrote: > > > On Tue, Nov 12, 2013 at 07:33:40PM +0100, Marek Vasut wrote: > > > > > > 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. > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..b10c651 100644 --- a/drivers/i2c/busses/i2c-eg20t.c +++ b/drivers/i2c/busses/i2c-eg20t.c @@ -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; }
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 <wernerandy@gmx.de> --- drivers/i2c/busses/i2c-eg20t.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)