RE: [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART

[Carlos Song (OSS)]

> -----Original Message-----
> From: Vincent Jardin <vjardin@xxxxxxx>
> Sent: Wednesday, August 12, 2026 1:47 PM
> To: Carlos Song (OSS) <carlos.song@xxxxxxxxxxx>
> Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Andi Shyti <andi.shyti@xxxxxxxxxx>; Frank Li
> <frank.li@xxxxxxx>; Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; Fabio
> Estevam <festevam@xxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx;
> imx@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and
> I2C_M_NOSTART
> 
> Hi Carlos,
> 
> Thanks a lot for the review, and no problem for the delay.
> 
> > This is a such rare i2c frame design in the Realtek RTL8366SE SMI read
> > frame, right?
> 
> It is rare, but I have been inspired for I2C_M_REV_DIR_ADDR by the source of
> two client drivers that use it.
> 
> Note that I2C_M_NOSTART is not rare at all. It is what regmap uses for a real
> gather write. regmap_i2c_gather_write() opens with
> 
>         if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_NOSTART))
>                 return -ENOTSUPP;
> 
> FYI, some clients that need REV_DIR_ADDR and NOSTART,
> 
>   drivers/input/joystick/as5011.c
>   drivers/video/fbdev/matrox/matroxfb_maven.c
> 
> both build the identical 2-message pattern,
> 
>   i2c_check_functionality(adapter,
>                                 I2C_FUNC_NOSTART |
>                                 I2C_FUNC_PROTOCOL_MANGLING)
> 
> I do not have those devices, so I did not check it beside code readings.
> 
> Some clients that need NOSTART alone,
> 
>   drivers/base/regmap/regmap-i2c.c
>   drivers/infiniband/hw/hfi1/qsfp.c
>   drivers/gpu/drm/i915/display/dvo_ivch.c
> 
> About the i2c masters, that use I2C_M_REV_DIR_ADDR in code,
> 
>   drivers/i2c/algos/i2c-algo-bit.c
>   drivers/i2c/algos/i2c-algo-pcf.c
>   drivers/i2c/busses/i2c-s3c2410.c <- the model I did investigate
>   drivers/i2c/busses/i2c-tegra-bpmp.c
>   drivers/media/pci/cobalt/cobalt-i2c.c
> 
> So a taxnonomy can be,
> 
>   adapter            REV_DIR impl   MANGLING adv   NOSTART adv
> usable
>   i2c-algo-bit           yes            yes            yes
> yes
>   i2c-algo-pcf           yes            yes            no
> no
>   i2c-s3c2410            yes            yes            yes
> yes
>   i2c-tegra-bpmp         yes            yes            yes
> yes
>   cobalt-i2c             yes         (private adapter, not exposed)
>   i2c-brcmstb            no             yes            yes
> no
>   i2c-pxa                no             yes            yes
> no
>   i2c-tegra              no             yes         yes (cond)
> no
>   i2c-imx (this patch)   yes            yes            yes
> yes
> 
> > Is Realtek RTL8366SE SMI driver upstream? Can I found the driver?
> 
> Not yet, and I cannot point you at a tree today. It is Realtek's "Unmanaged
> Switch" DSA driver, which is not public yet.
> 
> What I can share is the message construction, which is the part you asked
> about and is not Realtek-specific.
> 
>         static int as5011_i2c_read(struct i2c_client *client,
>                                    uint8_t aregaddr, signed char
> *value)
>         {
>                 uint8_t data[2] = { aregaddr };
>                 struct i2c_msg msg_set[2] = {
>                         {
>                                 .addr = client->addr,
>                                 .flags = I2C_M_REV_DIR_ADDR,
>                                 .len = 1,
>                                 .buf = (uint8_t *)data
>                         },
>                         {
>                                 .addr = client->addr,
>                                 .flags = I2C_M_RD |
> I2C_M_NOSTART,
>                                 .len = 1,
>                                 .buf = (uint8_t *)data
>                         }
>                 };
>                 int error;
> 
>                 error = i2c_transfer(client->adapter, msg_set, 2);
>                 if (error < 0)
>                         return error;
> 
>                 *value = data[0] & 0x80 ? -1 * (1 + ~data[0]) : data[0];
>                 return 0;
>         }
> 
> And the Realtek accessor, which is the same two messages with a 2-byte
> register and 2-byte data instead of 1 and 1:
> 
>         u8 ra[2]   = { reg & 0xff, (reg >> 8) & 0xff };
>         u8 data[2] = { 0xff, 0xff };
>         struct i2c_msg msgs[2] = {
>                 {
>                         .addr  = client->addr,
>                         .flags = I2C_M_REV_DIR_ADDR,
>                         .len   = sizeof(ra),
>                         .buf   = ra,
>                 }, {
>                         .addr  = client->addr,
>                         .flags = I2C_M_RD | I2C_M_NOSTART,
>                         .len   = sizeof(data),
>                         .buf   = data,
>                 },
>         };
> 
>         ret = i2c_transfer(client->adapter, msgs, 2);
>         if (ret != 2)
>                 return ret < 0 ? ret : -EIO;
> 
>         *val = data[0] | (data[1] << 8);
> 
> msgs[0] is a write message carrying I2C_M_REV_DIR_ADDR, so the address
> byte goes out with the read bit set while the master keeps transmitting the
> two register-address bytes. msgs[1] is the read half with I2C_M_NOSTART, so
> no repeated start is emitted and the controller simply turns the bus around.
> Writes are an ordinary unflagged 4-byte write and need nothing from this
> patch.
> 
> > ... Have you test this i2c-imx feature in your LS board with RTL8366SE?
> 
> Yes, on an LX2160A board carrying four RTL8366SE-CG. Two are strapped to
> the chip's 2-wire "EEPROM SMI" mode and hang off hardware i2c-imx
> controllers; the other two are on MDIO, for both the same switch registers are
> reachable both ways and should answer the same values.
> 
> First, the failure on an unpatched kernel, which is reproducible with nothing
> but i2ctools and is I think the clearest way to see the problem.
> These two commands are byte-identical:
> 
>         # i2ctransfer -y -f -a 1 w2@0x7c 0x00 0x13 r4@0x7c
>         0xff 0xff 0x00 0x00
>         # i2ctransfer -y -f -a 1 r6@0x7c        # no preceding write at all
>         0xff 0xff 0x00 0x00 0x00 0x00
> 
> Then, with this patch both chips answer correctly. For testing this patch, I did
> use the following that I have just pushed to help, for the record:
> 
>         https://github.com/vjardin/smi-probe
> 
>         # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c id
>         chip_num    0x6980  CHIP_RTL8367E (inside RTL8366SE-CG)
>         chip_ver    0x0030
>         svlan_tpid  0x88a8  reset default, as expected
> 
>         # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c rd 0x1300
>         reg 0x1300 = 0x6980
>         # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c rd 0x1202
>         reg 0x1202 = 0x88a8
> 
>         # smi-probe -t i2c -b /dev/i2c-2 -a 0x5c rd 0x1202 # U19, IIC3
>         reg 0x1202 = 0x88a8
>         # smi-probe -t mdio -b 0x8b97000 -a 0x1a rd 0x1202 # MDIO
> control
>         reg 0x1202 = 0x88a8
> 
> Then it binds as a DSA switch and enumerates its four user ports:
> 
>         realtek-US-switch-dsa-i2c 2-005c: RTK DSA unit 0 (EEPROM SMI,
> addr 0x5c)
> 
> I have many other i2c devices on this board, no regression.
> 
> Thanks again for your review,
>   Vincent


Hi, Vincent

Thank you very much for such clear instructions. That helps lot to understand what happened.
Follow this, I also spend some time to learn and understand this case.

1. Why need I2C_M_REV_DIR_ADDR but not directly read?
The msg with I2C_M_REV_DIR_ADDR is a write msg in fact, it is a write message, master write data and target ack,
only w bit is toggled to r bit.
Because the device don't follow the standard I2C spec, it need this special frame to write:
[1] S + addr + r + [ACK] + data +[ACK] + data...
Yes, not wrong, a read bit on bus but it is a write msg for this device.

But the standard spec write msg is
[2] S + addr + w +[ACK] +data + [ACK] +data
So add this I2C_M_REV_DIR_ADDR flag to make this write msg w bit is changed to r bit, msg [2] become msg [1] so the device worked.
Only change the r/w bit, other data flow keep write logic.

2. What I2C_M_NOSTART is doing?
It help skip the msg repeat start header, next msg start from data. If the first msg setting this flag, it should be unsupported(so you add a loop check to make sure the first msg without I2C_M_NOSTART flag).

3. The msg with I2C_M_NOSTART should keep the same direction with pre msg?
No, not need. For example, RTL8366SE frame:

(1){S Addr Rd [A] reg[7:0] [A] reg[15:8] [A]} (2){ [data[7:0]] A [data[15:8]] NA} P

For (1):
{S Addr Wr [A] reg[7:0] [A] reg[15:8] [A] is a write msg and "change the Wr bit to Rd bit" by I2C_M_REV_DIR_ADDR.
So i2c bus signal is
{S Addr Rd [A] reg[7:0] [A] reg[15:8] [A]}

For (2):
{Rs Addr Rd [A] [data[7:0]] A [data[15:8]] NA} is a read msg and "skip the {Rs Addr Rd [A]}" by I2C_M_NOSTART.
So i2c bus signal is [data[7:0]] A [data[15:8]] NA.

This is a write msg then a read msg. So you add the I2C_M_NOSTART flag handle in these 4 path: atomic read/irq read/atomic write/irq write.
Do I understand right?


About code, Could this part of the code be improved? Of course, that's a matter of personal preference.

@@ -1583,6 +1622,27 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
        struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
        int use_dma = 0;
 
+       for (i = 0; i < num; i++) {
+               /*
+                * I2C_FUNC_PROTOCOL_MANGLING is advertised for
+                * I2C_M_REV_DIR_ADDR only; reject the flags it also covers but
+                * that this driver does not implement, rather than silently
+                * transferring something the caller did not ask for.
+                */
+               if (msgs[i].flags & (I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK |
+                                    I2C_M_STOP))
+                       return -EOPNOTSUPP;
+
+               if (!(msgs[i].flags & I2C_M_NOSTART))
+                       continue;
+
+               /* Nothing to continue from, and a zero-length continuation
+                * would leave the controller with no byte to clock.
+                */
+               if (i == 0 || msgs[i].len == 0)
+                       return -EOPNOTSUPP;
+       }
+
        /* Start I2C transfer */
        result = i2c_imx_start(i2c_imx, atomic);
        if (result) {


I prefer this:
1. Do a single function for this check.
static int i2c_imx_check_msgs()
2. Add macro definition.
#define I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS \
	(I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK | I2C_M_STOP)
3. drop continue and use:
		/* NOSTART-specific sanity checks. */
		if (msgs[i].flags & I2C_M_NOSTART) {
			/*
			 * NOSTART continues an already open frame. The first
			 * message has nothing to continue from, and a
			 * zero-length continuation leaves the controller with
			 * no byte to clock.
			 */
			if (i == 0 || msgs[i].len == 0)
				return -EOPNOTSUPP;
		}

It is up to you. All in all, I think this patch is nice for me and thank you again for the improvements.

Carlos Song