[PATCH] i2c: imx: fix emulated smbus block read

[Carlos Song]

> -----Original Message-----
> From: Lukasz Kucharczyk <lukasz.kucharczyk at leica-geosystems.com>
> Sent: Tuesday, May 20, 2025 8:23 PM
> To: Oleksij Rempel <o.rempel at pengutronix.de>;
> stefan.eichenberger at toradex.com; Pengutronix Kernel Team
> <kernel at pengutronix.de>; Andi Shyti <andi.shyti at kernel.org>; Shawn Guo
> <shawnguo at kernel.org>; Sascha Hauer <s.hauer at pengutronix.de>; Fabio
> Estevam <festevam at gmail.com>
> Cc: open list:FREESCALE IMX I2C DRIVER <linux-i2c at vger.kernel.org>; open
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE <imx at lists.linux.dev>;
> moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel at lists.infradead.org>; open list
> <linux-kernel at vger.kernel.org>; bsp-development.geo at leica-geosystems.com;
> customers.leicageo at pengutronix.de; Lukasz Kucharczyk
> <lukasz.kucharczyk at leica-geosystems.com>
> Subject: [EXT] [PATCH] i2c: imx: fix emulated smbus block read
> 
> [You don't often get email from lukasz.kucharczyk at leica-geosystems.com.
> Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> Acknowledge the byte count submitted by the target.
> When I2C_SMBUS_BLOCK_DATA read operation is executed by
> i2c_smbus_xfer_emulated(), the length of the second (read) message is set to 1.
> Length of the block is supposed to be obtained from the target by the
> underlying bus driver.
> The i2c_imx_isr_read() function should emit the acknowledge on i2c bus after
> reading the first byte (i.e., byte count) while processing such message (as
> defined in Section 6.5.7 of System Management Bus Specification [1]). Without
> this acknowledge, the target does not submit subsequent bytes and the
> controller only reads 0xff's.
> 
> In addition, store the length of block data obtained from the target in the
> buffer provided by i2c_smbus_xfer_emulated() - otherwise the first byte of
> actual data is erroneously interpreted as length of the data block.
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsmbus.
> org%2Fspecs%2FSMBus_3_3_20240512.pdf&data=05%7C02%7Ccarlos.song%
> 40nxp.com%7Cd05bf48873224466159808dd97991d7d%7C686ea1d3bc2b4c
> 6fa92cd99c5c301635%7C0%7C1%7C638833406066875925%7CUnknown%7C
> TWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXa
> W4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=dmTE
> obQKzUzw03sEd%2BDtUo3se7kJ9ZTS4atfs0lGLC8%3D&reserved=0
> 
> Fixes: 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non dma mode")
> Signed-off-by: Lukasz Kucharczyk <lukasz.kucharczyk at leica-geosystems.com>

Hi,

Looks this is a nice fix.

I2C SMBUS block read need first read one byte from data length offset then I2C will know how many bytes
need to continue read. For this issue you can meet " Error: Read failed " when using i2cget -f -y bus address offset s to test.

So you apply this change to make i2c-imx controller can behavior like this:

S Addr Wr [A] Comm [A] Sr Addr Rd [A] [Count] A [Data] A [Data] A ... A [Data] NA P

Do I understand this right?

Carlos
> ---
>  drivers/i2c/busses/i2c-imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index
> ee0d25b498cb..4bf550a3b98d 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1008,7 +1008,7 @@ static inline int i2c_imx_isr_read(struct
> imx_i2c_struct *i2c_imx)
>         /* setup bus to read data */
>         temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>         temp &= ~I2CR_MTX;
> -       if (i2c_imx->msg->len - 1)
> +       if ((i2c_imx->msg->len - 1) || (i2c_imx->msg->flags &
> + I2C_M_RECV_LEN))
>                 temp &= ~I2CR_TXAK;
> 
>         imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); @@ -1063,6
> +1063,7 @@ static inline void i2c_imx_isr_read_block_data_len(struct
> imx_i2c_struct *i2c_im
>                 wake_up(&i2c_imx->queue);
>         }
>         i2c_imx->msg->len += len;
> +       i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = len;
>  }
> 
>  static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx,
> unsigned int status)
> --
> 2.34.1
>