| Message ID | 20220717035027.2135106-2-bjorn.andersson@linaro.org |
|---|---|
| State | Accepted |
| Commit | b3f0ceb7c2037c6e3affd7d9c84ac5f97af7a5b5 |
| Headers | show |
| Series | i2c: qcom-geni: Make it possible to run i2cdetect | expand |
On 16-07-22, 20:50, Bjorn Andersson wrote: > The introduction of GPI support moved things around and instead of > returning the result from geni_i2c_xfer() the number of messages in the > request was returned, ignoring the actual result. Fix this. Thanks for the fix, looking at master_xfer() it does expect error return, so look good with one nit: > > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 6ac402ea58fb..3bec7c782824 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > pm_runtime_put_autosuspend(gi2c->se.dev); > gi2c->cur = NULL; > gi2c->err = 0; Unrelated, should gi2c->err be set to ret here..? > - return num; > + return ret; > } > > static u32 geni_i2c_func(struct i2c_adapter *adap) > -- > 2.35.1
On Mon 18 Jul 00:36 CDT 2022, Vinod Koul wrote: > On 16-07-22, 20:50, Bjorn Andersson wrote: > > The introduction of GPI support moved things around and instead of > > returning the result from geni_i2c_xfer() the number of messages in the > > request was returned, ignoring the actual result. Fix this. > > Thanks for the fix, looking at master_xfer() it does expect error > return, so look good with one nit: > > > > > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > drivers/i2c/busses/i2c-qcom-geni.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > > index 6ac402ea58fb..3bec7c782824 100644 > > --- a/drivers/i2c/busses/i2c-qcom-geni.c > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > > @@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > > pm_runtime_put_autosuspend(gi2c->se.dev); > > gi2c->cur = NULL; > > gi2c->err = 0; > > Unrelated, should gi2c->err be set to ret here..? > When we reach this point we have concluded the current transfer (successfully or not...), so I believe that the purpose of this line is to clear the "error state" that might have occurred during that transfer. I believe this line could be removed, as the first step in a transfer is to clear the error state again. But as you suggest this is separate to the proposed change. May I have a R-b? Regards, Bjorn > > - return num; > > + return ret; > > } > > > > static u32 geni_i2c_func(struct i2c_adapter *adap) > > -- > > 2.35.1 > > -- > ~Vinod
On Sat, Jul 16, 2022 at 08:50:25PM -0700, Bjorn Andersson wrote: > The introduction of GPI support moved things around and instead of > returning the result from geni_i2c_xfer() the number of messages in the > request was returned, ignoring the actual result. Fix this. > > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 6ac402ea58fb..3bec7c782824 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > pm_runtime_put_autosuspend(gi2c->se.dev); > gi2c->cur = NULL; > gi2c->err = 0; > - return num; > + return ret; > } > > static u32 geni_i2c_func(struct i2c_adapter *adap) > -- > 2.35.1 > Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
On 16-07-22, 20:50, Bjorn Andersson wrote: > The introduction of GPI support moved things around and instead of > returning the result from geni_i2c_xfer() the number of messages in the > request was returned, ignoring the actual result. Fix this. Reviewed-by: Vinod Koul <vkoul@kernel.org> > > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 6ac402ea58fb..3bec7c782824 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > pm_runtime_put_autosuspend(gi2c->se.dev); > gi2c->cur = NULL; > gi2c->err = 0; > - return num; > + return ret; > } > > static u32 geni_i2c_func(struct i2c_adapter *adap) > -- > 2.35.1
> - return num; > + return ret; Not quite. Kdoc describes the retval in i2c_transfer. 2136 * i2c_transfer - execute a single or combined I2C message 2137 * @adap: Handle to I2C bus 2138 * @msgs: One or more messages to execute before STOP is issued to 2139 * terminate the operation; each message begins with a START. 2140 * @num: Number of messages to be executed. 2141 * 2142 * Returns negative errno, else the number of messages executed. I agree this needs better documentation in i2c.h, I will fix it.
On Sun, Jul 24, 2022 at 07:04:12AM +0200, Wolfram Sang wrote: > > > - return num; > > + return ret; > > Not quite. Kdoc describes the retval in i2c_transfer. > > 2136 * i2c_transfer - execute a single or combined I2C message > 2137 * @adap: Handle to I2C bus > 2138 * @msgs: One or more messages to execute before STOP is issued to > 2139 * terminate the operation; each message begins with a START. > 2140 * @num: Number of messages to be executed. > 2141 * > 2142 * Returns negative errno, else the number of messages executed. And this is exactly what the driver is returning after this fix. Note that ret above is assigned based on the return value from two helper functions that return either a negative errno or the number messages executed. So I believe this patch is still needed to fix the error handling. Reviewed-by: Johan Hovold <johan+linaro@kernel.org> Johan
> Note that ret above is assigned based on the return value from two > helper functions that return either a negative errno or the number > messages executed. Ah, cool. I see now. Thanks for the heads up!
On Sun, Jul 24, 2022 at 07:04:12AM +0200, Wolfram Sang wrote: > > > - return num; > > + return ret; > > Not quite. Kdoc describes the retval in i2c_transfer. Reconsidered and applied to for-next, thanks! > I agree this needs better documentation in i2c.h, I will fix it. I'll still do this :)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 6ac402ea58fb..3bec7c782824 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, pm_runtime_put_autosuspend(gi2c->se.dev); gi2c->cur = NULL; gi2c->err = 0; - return num; + return ret; } static u32 geni_i2c_func(struct i2c_adapter *adap)
The introduction of GPI support moved things around and instead of returning the result from geni_i2c_xfer() the number of messages in the request was returned, ignoring the actual result. Fix this. Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/i2c/busses/i2c-qcom-geni.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)