| Message ID | 1384678731-10399-1-git-send-email-wernerandy@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, Nov 17, 2013 at 09:58:51AM +0100, Andreas Werner wrote: > 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. > > diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c > index 0f37529..5c39f90 100644 > --- a/drivers/i2c/busses/i2c-eg20t.c > +++ b/drivers/i2c/busses/i2c-eg20t.c > @@ -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. > @@ -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
> Is there another reason why pch_i2c_getack returned EPROTO? > May be ENXIO was introduced later? Imperfect review :) > 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.
On Sun, Nov 17, 2013 at 12:08:46PM +0100, Wolfram Sang wrote: > On Sun, Nov 17, 2013 at 09:58:51AM +0100, Andreas Werner wrote: > > 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. > > > > > diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c > > index 0f37529..5c39f90 100644 > > --- a/drivers/i2c/busses/i2c-eg20t.c > > +++ b/drivers/i2c/busses/i2c-eg20t.c > > @@ -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. 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? > > > @@ -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? > 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 > Regards, > > Wolfram > -- 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 Sun, Nov 17, 2013 at 01:18:09PM +0100, Wolfram Sang wrote: > > > Is there another reason why pch_i2c_getack returned EPROTO? > > May be ENXIO was introduced later? > > Imperfect review :) > > > 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 -- 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 Sun, Nov 17, 2013 at 05:53:29PM +0100, Andreas Werner wrote: > On Sun, Nov 17, 2013 at 01:18:09PM +0100, Wolfram Sang wrote: > > > > > Is there another reason why pch_i2c_getack returned EPROTO? > > > May be ENXIO was introduced later? > > > > Imperfect review :) > > > > > 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; 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...
On Sun, Nov 17, 2013 at 06:08:38PM +0100, Wolfram Sang wrote: > On Sun, Nov 17, 2013 at 05:53:29PM +0100, Andreas Werner wrote: > > On Sun, Nov 17, 2013 at 01:18:09PM +0100, Wolfram Sang wrote: > > > > > > > Is there another reason why pch_i2c_getack returned EPROTO? > > > > May be ENXIO was introduced later? > > > > > > Imperfect review :) > > > > > > > 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; > > 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... > 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 -- 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
> 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.
diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c index 0f37529..5c39f90 100644 --- a/drivers/i2c/busses/i2c-eg20t.c +++ b/drivers/i2c/busses/i2c-eg20t.c @@ -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; }
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 <wernerandy@gmx.de> --- drivers/i2c/busses/i2c-eg20t.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)