| Message ID | 20190521013350.8426-1-masahisa.kojima@linaro.org |
|---|---|
| State | Accepted |
| Commit | ff9378904d9d7a3fcb8406604e089e535e357b1d |
| Headers | show |
| Series | i2c: synquacer: fix synquacer_i2c_doxfer() return value | expand |
On Tue, 21 May 2019 at 03:35, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > master_xfer should return the number of messages successfully > processed. > > Fixes: 0d676a6c4390 ("i2c: add support for Socionext SynQuacer I2C controller") > Cc: <stable@vger.kernel.org> # v4.19+ > Signed-off-by: Okamoto Satoru <okamoto.satoru@socionext.com> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/i2c/busses/i2c-synquacer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c > index f14d4b3fab44..f724c8e6b360 100644 > --- a/drivers/i2c/busses/i2c-synquacer.c > +++ b/drivers/i2c/busses/i2c-synquacer.c > @@ -351,7 +351,7 @@ static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c, > /* wait 2 clock periods to ensure the stop has been through the bus */ > udelay(DIV_ROUND_UP(2 * 1000, i2c->speed_khz)); > > - return 0; > + return ret; > } > > static irqreturn_t synquacer_i2c_isr(int irq, void *dev_id) > -- > 2.14.2 >
On Tue, May 21, 2019 at 10:33:50AM +0900, Masahisa Kojima wrote: > master_xfer should return the number of messages successfully > processed. > > Fixes: 0d676a6c4390 ("i2c: add support for Socionext SynQuacer I2C controller") > Cc: <stable@vger.kernel.org> # v4.19+ > Signed-off-by: Okamoto Satoru <okamoto.satoru@socionext.com> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> Applied to for-current, thanks! I just noticed you have an open coded loop in synquacer_i2c_xfer() which should not be needed because the I2C core does that? Your code does a HW reset, though, but is it really needed for a lost arbitration?
On Tue, 28 May 2019 at 04:34, Wolfram Sang <wsa@the-dreams.de> wrote: > > On Tue, May 21, 2019 at 10:33:50AM +0900, Masahisa Kojima wrote: > > master_xfer should return the number of messages successfully > > processed. > > > > Fixes: 0d676a6c4390 ("i2c: add support for Socionext SynQuacer I2C controller") > > Cc: <stable@vger.kernel.org> # v4.19+ > > Signed-off-by: Okamoto Satoru <okamoto.satoru@socionext.com> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > Applied to for-current, thanks! > Thank you for merging and your feedback. > I just noticed you have an open coded loop in synquacer_i2c_xfer() which > should not be needed because the I2C core does that? I'm not sure I correctly understand the meaning of "open coded loop", but I2C controller status such as lost arbitration and bus busy is checked in the retry of synquacer_i2c_doxfer(). > Your code does a HW > reset, though, but is it really needed for a lost arbitration? Other than handling lost arbitration, this loop also handles following errors. - transfer fails(expected size and actual transferred size is not matched) - transfer timeout I think it is reasonable to reset the I2C controller before xfer() retries. Regards, Masahisa
Hi Masahisa, > > I just noticed you have an open coded loop in synquacer_i2c_xfer() which > > should not be needed because the I2C core does that? > > I'm not sure I correctly understand the meaning of "open coded loop", It means the driver has the same loop which is already present in the I2C core. > > Your code does a HW > > reset, though, but is it really needed for a lost arbitration? > > Other than handling lost arbitration, this loop also handles following errors. > - transfer fails(expected size and actual transferred size is not matched) > - transfer timeout > I think it is reasonable to reset the I2C controller before xfer() retries. Resetting may be OK, but retrying is not correct. Retrying is only for lost arbitration. For the above errors, we return the error without retrying and let the upper layers decide what to do. Kind regards, Wolfram
Hi Wolfram, On Tue, 28 May 2019 at 14:48, Wolfram Sang <wsa@the-dreams.de> wrote: > > Hi Masahisa, > > > > I just noticed you have an open coded loop in synquacer_i2c_xfer() which > > > should not be needed because the I2C core does that? > > > > I'm not sure I correctly understand the meaning of "open coded loop", > > It means the driver has the same loop which is already present in the > I2C core. > > > > Your code does a HW > > > reset, though, but is it really needed for a lost arbitration? > > > > Other than handling lost arbitration, this loop also handles following errors. > > - transfer fails(expected size and actual transferred size is not matched) > > - transfer timeout > > I think it is reasonable to reset the I2C controller before xfer() retries. > > Resetting may be OK, but retrying is not correct. Retrying is only for > lost arbitration. For the above errors, we return the error without > retrying and let the upper layers decide what to do. Thank you for your reply, I understand. I will consider to update the current xfer() function. > Kind regards, > > Wolfram
diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c index f14d4b3fab44..f724c8e6b360 100644 --- a/drivers/i2c/busses/i2c-synquacer.c +++ b/drivers/i2c/busses/i2c-synquacer.c @@ -351,7 +351,7 @@ static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c, /* wait 2 clock periods to ensure the stop has been through the bus */ udelay(DIV_ROUND_UP(2 * 1000, i2c->speed_khz)); - return 0; + return ret; } static irqreturn_t synquacer_i2c_isr(int irq, void *dev_id)
master_xfer should return the number of messages successfully processed. Fixes: 0d676a6c4390 ("i2c: add support for Socionext SynQuacer I2C controller") Cc: <stable@vger.kernel.org> # v4.19+ Signed-off-by: Okamoto Satoru <okamoto.satoru@socionext.com> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- drivers/i2c/busses/i2c-synquacer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.14.2