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

[Andi Shyti]

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 <andi.shyti at kernel.org> wrote:
> > > 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