| Message ID | 20240311032600.56244-2-hanshu-oc@zhaoxin.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] i2c: viai2c: Fix some minor style issues | expand |
On 3/11/2024 8:56 AM, Hans Hu wrote: > This is a bug that was accidentally introduced when > adjusting the wmt driver. Now fix it > what exactly is the bug which you are fixing here ? > Signed-off-by: Hans Hu <hanshu-oc@zhaoxin.com> > --- > drivers/i2c/busses/i2c-viai2c-common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-viai2c-common.c b/drivers/i2c/busses/i2c-viai2c-common.c > index 4c208b3a509e..894d24c6b4d3 100644 > --- a/drivers/i2c/busses/i2c-viai2c-common.c > +++ b/drivers/i2c/busses/i2c-viai2c-common.c > @@ -145,7 +145,7 @@ static int viai2c_irq_xfer(struct viai2c *i2c) > if (msg->len == 0) { > val = VIAI2C_CR_TX_END | VIAI2C_CR_CPU_RDY | VIAI2C_CR_ENABLE; > writew(val, base + VIAI2C_REG_CR); > - return 0; > + return 1; Question: Do you really need to do anything when no data is there to transfer ? I am not sure what's the strategy adopted here. > } > > if ((i2c->xfered_len + 1) == msg->len) {
Hi Mukesh, >>>> This is a bug that was accidentally introduced when >>>> adjusting the wmt driver. Now fix it >>>> >>> >>> what exactly is the bug which you are fixing here ? >>> >> >> This bug was introduced by myself in a recent commit, >> >> id: 4b0c0569f03261aa4c10c8f5b24df6c1ca27f889 >> >> https://patchwork.ozlabs.org/project/linux-i2c/patch/20240306212413.1850236-5-andi.shyti@kernel.org/ >> >> >> >> The function viai2c_irq_xfer() is working in the interrupt context, >> >> if it returns a non-0 value indicating that the current msg access >> >> has ended, otherwise it has not ended. > Should be otherway around ? zero indicates success as per general > practices. I understand your suggestion. The return value here indicates whether the transfer should end or not. 0 means not, and non-0 means yes(error or successful). > > Also i think accordingly your commit log should have the explanation. > Yes, I should give a more detailed commit log. >> >> For the access that msg->len is 0, when the interruption occurs, >> >> it means that the access has ended, it should return 1; >> >> Otherwise wait_for_completion_timeout() will timeout. >> >> > IIUC, msg->len = 0 it indicates your transfer is completed and then you > want to return 1 indicating current message transfer is successful ? > Please ammend the logs reflecting the scenario to match with the code > changes. > No, msg->len = 0 here means this is I2C_SMBUS_QUICK access. Yes, return 1 indicating current message transfer is successful. Hans, Thanks
diff --git a/drivers/i2c/busses/i2c-viai2c-common.c b/drivers/i2c/busses/i2c-viai2c-common.c index 4c208b3a509e..894d24c6b4d3 100644 --- a/drivers/i2c/busses/i2c-viai2c-common.c +++ b/drivers/i2c/busses/i2c-viai2c-common.c @@ -145,7 +145,7 @@ static int viai2c_irq_xfer(struct viai2c *i2c) if (msg->len == 0) { val = VIAI2C_CR_TX_END | VIAI2C_CR_CPU_RDY | VIAI2C_CR_ENABLE; writew(val, base + VIAI2C_REG_CR); - return 0; + return 1; } if ((i2c->xfered_len + 1) == msg->len) {
This is a bug that was accidentally introduced when adjusting the wmt driver. Now fix it Signed-off-by: Hans Hu <hanshu-oc@zhaoxin.com> --- drivers/i2c/busses/i2c-viai2c-common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)