On Tue, Sep 09, 2025 at 11:50:15AM +0800, Chen-Yu Tsai wrote:

On Tue, Sep 9, 2025 at 6:17 AM Andi Shyti [off-list ref] wrote:

quoted

quoted

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index ab456c3717db..dee40704825c 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c

@@ -1243,6 +1243,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 {
      int ret;
      int left_num = num;
+     bool write_then_read_en = false;
      struct mtk_i2c *i2c = i2c_get_adapdata(adap);

      ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);

@@ -1256,6 +1257,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
              if (!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
                  msgs[0].addr == msgs[1].addr) {
                      i2c->auto_restart = 0;
+                     write_then_read_en = true;
              }
      }

@@ -1280,12 +1282,10 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
              else
                      i2c->op = I2C_MASTER_WR;

-             if (!i2c->auto_restart) {
-                     if (num > 1) {
-                             /* combined two messages into one transaction */
-                             i2c->op = I2C_MASTER_WRRD;
-                             left_num--;
-                     }
+             if (write_then_read_en) {
+                     /* combined two messages into one transaction */
+                     i2c->op = I2C_MASTER_WRRD;

i2c doesn't change for the whole loop so that it can be set only
once outside the loop instead of setting it everytime.

Something like this:

        if (i2c->op == I2C_MASTER_WRRD)
                left_num--;
        else if (msgs->flags & I2C_M_RD)
                ...
        else

looks cleaner to me and we save the extra flag. Am I missing
anything?

It looks correct to me, though I think it requires a comment explaining
that "in the WRRD case there are only two messages that get processed
together, and the while loop doesn't actually iterate", and reference
the block where the WRRD op is set.

Otherwise someone is going to look at this snippet and think there's
some corner case where all messages (# of messages > 2) get handled
using the WRRD op.

Agree, indeed I wanted to write it somewhere that a comment would
have been nice.

I'd appreciate a comment even with the boolean flag. I think
boolean flags are often a forced solution and we always need to
describe their need.

So maybe it looks cleaner, but it requires more context to understand.
Whereas in the original patch, the extra variable sort of gives that
context. In this case I prefer the context being more visible, since
the original corner case this issue fixes is also from missing context
and assumptions.

I think both solutions are clear in a different way. Anyway, I'm
not very strong on this comment, I just see that from a code
perspective looks nicer. If you guys insist, then I will let this
go as it is.

Andi