Re: [PATCH i2c-host v6] i2c: rtl9300: Implement I2C block read and write

[Jonas Jelonek <jelonek.jonas@gmail.com>]

Hi Sven,

On 26.09.25 09:52, Sven Eckelmann wrote:
> From: Harshal Gohel <hg@simonwunderlich.de>
>
> It was noticed that the original implementation of SMBus Block Write in the
> driver was actually an I2C Block Write. Both differ only in the Count byte
> before the actual data:
>
>   S Addr Wr [A] Comm [A] Count [A] Data [A] Data [A] ... [A] Data [A] P
>
> The I2C Block Write is just skipping this Count byte and starts directly
> with the data:
>
>   S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P
>
> The I2C controller of RTL93xx doesn't handle this Count byte special and it
> is simply another one of (16 possible) data bytes. Adding support for the
> I2C Block Write therefore only requires skipping the count byte (0) in
> data->block.
>
> It is similar for reads. The SMBUS Block read is having a Count byte before
> the data:
>
>   S Addr Wr [A] Comm [A]
>             Sr Addr Rd [A] [Count] A [Data] A [Data] A ... A [Data] NA P
>
> And the I2C Block Read is directly starting with the actual data:
>
>   S Addr Wr [A] Comm [A]
>             Sr Addr Rd [A] [Data] A [Data] A ... A [Data] NA P
>
> The I2C controller is also not handling this byte in a special way. It
> simply provides every byte after the Rd marker + Ack as part of the 16 byte
> receive buffer (registers). The content of this buffer just has to be
> copied to the right position in the receive data->block.
>
> Signed-off-by: Harshal Gohel <hg@simonwunderlich.de>
> Co-developed-by: Sven Eckelmann <sven@narfation.org>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> Tested-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> ---
> This patch was already applied [1] but then removed. Instead, only the
> chunk
>
>   @@ -314,7 +343,7 @@ static u32 rtl9300_i2c_func(struct i2c_adapter *a)
>    {
>           return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>                  I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
>   -              I2C_FUNC_SMBUS_BLOCK_DATA;
>   +              I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_I2C_BLOCK;
>    }
>
> was added as part of a patch which has nothing to do with
> I2C_FUNC_SMBUS_I2C_BLOCK [2] and was never submitted like this [3].
>
> I am therefore resubmitting this patch again without the rtl9300_i2c_func
> change.
>
> [1] https://lore.kernel.org/r/a422shurtl3xrvnh2ieynqq2kw5awqnmall2wjdpozx336m26i@54ekftmkwvrv
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git/commit/?h=i2c/i2c-host&id=ede965fd555ac2536cf651893a998dbfd8e57b86
> [3] https://lore.kernel.org/r/20250831100457.3114-4-jelonek.jonas@gmail.com
> ---
> Changes in v6:
> - drop all fixes patches (which were already applied)
> - drop rtl9300_i2c_func chunk which was incorrectly added by another commit
>   [2] (but was not intended to be in there by the original patch [3]
> - Link to v5: https://lore.kernel.org/r/20250810-i2c-rtl9300-multi-byte-v5-0-cd9dca0db722@narfation.org
>
> Changes in v5:
> - Simplify function/capability registration by using
>   I2C_FUNC_SMBUS_I2C_BLOCK, thanks Jonas Jelonek
> - Link to v4: https://lore.kernel.org/r/20250809-i2c-rtl9300-multi-byte-v4-0-d71dd5eb6121@narfation.org
>
> Changes in v4:
> - Provide only "write" examples for "i2c: rtl9300: Fix multi-byte I2C write"
> - drop the second initialization of vals in rtl9300_i2c_write() directly in
>   the "Fix multi-byte I2C write" fix
> - indicate in target branch for each patch in PATCH prefix
> - minor commit message cleanups
> - Link to v3: https://lore.kernel.org/r/20250804-i2c-rtl9300-multi-byte-v3-0-e20607e1b28c@narfation.org
>
> Changes in v3:
> - integrated patch
>   https://lore.kernel.org/r/20250615235248.529019-1-alexguo1023@gmail.com
>   to avoid conflicts in the I2C_SMBUS_BLOCK_DATA code
> - added Fixes and stable@vger.kernel.org to Alex Guo's patch
> - added Chris Packham's Reviewed-by/Acked-by
> - Link to v2: https://lore.kernel.org/r/20250803-i2c-rtl9300-multi-byte-v2-0-9b7b759fe2b6@narfation.org
>
> Changes in v2:
> - add the missing transfer width and read length increase for the SMBus
>   Write/Read
> - Link to v1: https://lore.kernel.org/r/20250802-i2c-rtl9300-multi-byte-v1-0-5f687e0098e2@narfation.org
> ---
>  drivers/i2c/busses/i2c-rtl9300.c | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
> index 9e1f71fed0feac41e1534709de2406c7a63fa9cd..9e623207513718970dc1af82aa8756144a771819 100644
> --- a/drivers/i2c/busses/i2c-rtl9300.c
> +++ b/drivers/i2c/busses/i2c-rtl9300.c
> @@ -186,22 +186,32 @@ static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,
>  		return -EIO;
>  
>  	if (read_write == I2C_SMBUS_READ) {
> -		if (size == I2C_SMBUS_BYTE || size == I2C_SMBUS_BYTE_DATA) {
> +		switch (size) {
> +		case I2C_SMBUS_BYTE:
> +		case I2C_SMBUS_BYTE_DATA:
>  			ret = regmap_read(i2c->regmap,
>  					  i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0, &val);
>  			if (ret)
>  				return ret;
>  			data->byte = val & 0xff;
> -		} else if (size == I2C_SMBUS_WORD_DATA) {
> +			break;
> +		case I2C_SMBUS_WORD_DATA:
>  			ret = regmap_read(i2c->regmap,
>  					  i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0, &val);
>  			if (ret)
>  				return ret;
>  			data->word = val & 0xffff;
> -		} else {
> +			break;
> +		case I2C_SMBUS_I2C_BLOCK_DATA:
> +			ret = rtl9300_i2c_read(i2c, &data->block[1], len);
> +			if (ret)
> +				return ret;
> +			break;
> +		default:
>  			ret = rtl9300_i2c_read(i2c, &data->block[0], len);
>  			if (ret)
>  				return ret;
> +			break;
>  		}
>  	}
>  
> @@ -290,6 +300,25 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
>  		len = data->block[0] + 1;
>  		break;
>  
> +	case I2C_SMBUS_I2C_BLOCK_DATA:
> +		ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
> +		if (ret)
> +			goto out_unlock;
> +		if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) {
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
> +		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0]);
> +		if (ret)
> +			goto out_unlock;
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			ret = rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
> +			if (ret)
> +				goto out_unlock;
> +		}
> +		len = data->block[0];
> +		break;
> +
>  	default:
>  		dev_err(&adap->dev, "Unsupported transaction %d\n", size);
>  		ret = -EOPNOTSUPP;
>
> ---
> base-commit: 217f92d91c9faeb6b78bd6205b3585944cbcb433
> change-id: 20250802-i2c-rtl9300-multi-byte-edaa1fb0872c
>
> Best regards,

Thanks for taking care of this quickly.
Maybe we should include another patch here which fixes the committed version
of my patch, i.e. removing I2C_FUNC_SMBUS_I2C_BLOCK, with CC to stable. Since
the patch was also merged to stable, it is somewhat broken there now.

Best,
Jonas Jelonek