[PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
Carlos Song (OSS)
carlos.song at oss.nxp.com
Wed Aug 12 02:20:25 PDT 2026
> -----Original Message-----
> From: Vincent Jardin <vjardin at free.fr>
> Sent: Wednesday, August 12, 2026 1:47 PM
> To: Carlos Song (OSS) <carlos.song at oss.nxp.com>
> Cc: Oleksij Rempel <o.rempel at pengutronix.de>; Pengutronix Kernel Team
> <kernel at pengutronix.de>; Andi Shyti <andi.shyti at kernel.org>; Frank Li
> <frank.li at nxp.com>; Sascha Hauer <s.hauer at pengutronix.de>; Fabio
> Estevam <festevam at gmail.com>; linux-i2c at vger.kernel.org;
> imx at lists.linux.dev; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org
> 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 at 0x7c 0x00 0x13 r4 at 0x7c
> 0xff 0xff 0x00 0x00
> # i2ctransfer -y -f -a 1 r6 at 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
More information about the linux-arm-kernel
mailing list