[PATCH v2] i2c: mediatek: fix potential incorrect use of I2C_MASTER_WRRD

[Chen-Yu Tsai]

On Tue, Sep 9, 2025 at 6:17 AM Andi Shyti <andi.shyti at kernel.org> wrote:
>
> Hi Leilk,
>
> > 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.

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.


ChenYu

> Andi
>
> > +                     left_num--;
> >               }
> >
> >               /* always use DMA mode. */
> > @@ -1293,7 +1293,10 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> >               if (ret < 0)
> >                       goto err_exit;
> >
> > -             msgs++;
> > +             if (i2c->op == I2C_MASTER_WRRD)
> > +                     msgs += 2;
> > +             else
> > +                     msgs++;
> >       }
> >       /* the return value is number of executed messages */
> >       ret = num;
> > --
> > 2.46.0
> >