| Message ID | 20240311032600.56244-2-hanshu-oc@zhaoxin.com |
|---|---|
| State | Changes Requested |
| 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, > 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 ? > 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. 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. >> 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. This is to be consistent with former i2c-wmt.c: https://elixir.bootlin.com/linux/v6.8/source/drivers/i2c/busses/i2c-wmt.c#L175 Hans, Thanks
Hi Hans, On 3/11/2024 11:41 AM, Hans Hu wrote: > Hi Mukesh, > > >> 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 ? >> > > 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. Also i think accordingly your commit log should have the explanation. > > 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. I would be more clear if commit log explains what you are doing. >>> 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. > > > This is to be consistent with former i2c-wmt.c: > https://elixir.bootlin.com/linux/v6.8/source/drivers/i2c/busses/i2c-wmt.c#L175 > > > Hans, > > Thanks >
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(-)