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

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

[Sven Eckelmann]

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,
-- 
Sven Eckelmann <sven@narfation.org>

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

[Jonas Jelonek]

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

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

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

On Friday, 26 September 2025 10:16:19 CEST Jonas Jelonek wrote:
> 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.

Yes, sounds good. Will submit it [1] tomorrow after the ~24h dwell time (if no 
one else requests anything different).

Kind regards,
	Sven

[1] https://git.open-mesh.org/linux-merge.git/log/?h=b4/i2c-rtl9300-multi-byte

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

On Fri, Sep 26, 2025 at 10:34:40AM +0200, Sven Eckelmann wrote:
> On Friday, 26 September 2025 10:16:19 CEST Jonas Jelonek wrote:
> > 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.
> 
> Yes, sounds good. Will submit it [1] tomorrow after the ~24h dwell time (if no 
> one else requests anything different).

I am getting a little confused already, so please make sure I know which
patches to apply where in what order.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

On Friday, 26 September 2025 10:36:46 CEST Wolfram Sang wrote:
> On Fri, Sep 26, 2025 at 10:34:40AM +0200, Sven Eckelmann wrote:
> > On Friday, 26 September 2025 10:16:19 CEST Jonas Jelonek wrote:
> > > 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.
> > 
> > Yes, sounds good. Will submit it [1] tomorrow after the ~24h dwell time (if no 
> > one else requests anything different).
> 
> I am getting a little confused already, so please make sure I know which
> patches to apply where in what order.

Understandable, there was a lot going on at the same time for this niche i2c 
host driver. Please check the "Order" in 
https://lore.kernel.org/r/20250927-i2c-rtl9300-multi-byte-v7-0-c0fd0e78b818@narfation.org

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Jonas Jelonek]

On 27.09.25 11:55, Sven Eckelmann wrote:
> Understandable, there was a lot going on at the same time for this niche i2c 
> host driver. Please check the "Order" in 
> https://lore.kernel.org/r/20250927-i2c-rtl9300-multi-byte-v7-0-c0fd0e78b818@narfation.org

Sorry for bringing back some confusion but I had to fix one final thing in my patch series.
The order Sven mentioned is fine but please use v8 of my series [2].

To wrap it up:
[1] needs to come first
[2] then this can be merged

> Kind regards,
> 	Sven

Best regards,
Jonas Jelonek

[1] https://lore.kernel.org/linux-i2c/20250927-i2c-rtl9300-multi-byte-v7-0-c0fd0e78b818@narfation.org/
[2] https://lore.kernel.org/linux-i2c/20250927101931.71575-1-jelonek.jonas@gmail.com/

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

[Wolfram Sang]

> To wrap it up:
> [1] needs to come first
> [2] then this can be merged

Thanks for the heads up here! I will do the following:

1) apply the patch for host-fixes now and send it to Linus tomorrow for
   6.17
2) during the 6.18 mergewindow, send my first PR without your patches
3) when both of the above are in Linus tree, I will prepare a second PR
   with (not only) your patches
4) send this some days later in the same merge window