Skip to contentSkip to conversation

i2c: rtl9300: remove IO_MODE configuration

PATCH Superseded Michael Zavertkin 3 participants 7 messages Oct 10 – Oct 20

Subsystems (3)
Device tree bindings I2C subsystem host drivers RTL9300 I2C driver (rtl9300-i2c)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Michael Zavertkin

author
#0 lore raw

v2 cover i2c: rtl9300: remove IO_MODE configuration

i2c driver for RTL9607C used to configure IO_MODE register. The register is global and responsible for configuring SoC pins, therefore it should belong to pinctrl driver.

SoC-specific pinctrl driver is WIP (waiting for these patches in OpenWRT), but pinctrl-single could be used instead.

While added compatible realtek,rtl9607c-i2c relies on pinctrl driver, old realtek,rtl9607-i2c left for backward compatibility.

Signed-off-by Michael Zavertkin <[email protected]>

---
Changes in v2:
- Backward compatibility achieved by adding new compatible value. Old one is marked as deprecated
- Link to v1: https://patch.msgid.link/[email protected]
---
Michael Zavertkin (2):
i2c: rtl9300: add realtek,rtl9607c-i2c compatible
dt-bindings: i2c: rtl9300: add 'rtl9607c-i2c'

 .../bindings/i2c/realtek,rtl9301-i2c.yaml     | 20 ++++++++++++++++++-
 drivers/i2c/busses/i2c-rtl9300.c              | 15 ++++++++++----
 2 files changed, 30 insertions(+), 5 deletions(-)
Signature · 1 line
2.54.0
author
#1 lore raw

PATCH v2 1/2 i2c: rtl9300: add realtek,rtl9607c-i2c compatible

RTL9607C was already supported by realtek,rtl9607-i2c, which is confusing, because there are another SoCs such as RTL9607F, RTL9607 (without letter) and a few more. So more correct compatible was added.

New compatible doesn't use IO_MODE_EN register, leaving it to pinctrl driver. Old one left for backward compatibility.

Signed-off-by Michael Zavertkin <[email protected]>
---
 drivers/i2c/busses/i2c-rtl9300.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-rtl9300.c +11 -4
index 3a8225b0666c..22aa3b38458c 100644
@@ -200,10 +200,12 @@ static int rtl9607_i2c_config_chan(struct rtl9300_i2c *i2c, struct rtl9300_i2c_c
 	if (ret)
 		return ret;
 
-	drv_data = device_get_match_data(i2c->dev);
-	ret = drv_data->select_scl(i2c, i2c->scl_num);
-	if (ret)
-		return ret;
+	if (device_is_compatible(i2c->dev, "realtek,rtl9607-i2c")) {
+		drv_data = device_get_match_data(i2c->dev);
+		ret = drv_data->select_scl(i2c, i2c->scl_num);
+		if (ret)
+			return ret;
+	}
 
 	i2c->sda_num = chan->sda_num;
 	return 0;

@@ -494,6 +496,10 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
 		scl_num = 0;
 	i2c->scl_num = (u8)scl_num;
 
+	if (device_is_compatible(i2c->dev, "realtek,rtl9607-i2c"))
+		dev_warn(dev, "Compatible 'realtek,rtl9607-i2c' is deprecated. "
+			      "Use 'realtek,rtl9607c-i2c' instead\n");
+
 	platform_set_drvdata(pdev, i2c);
 
 	drv_data = device_get_match_data(i2c->dev);

@@ -652,6 +658,7 @@ static const struct of_device_id i2c_rtl9300_dt_ids[] = {
 	{ .compatible = "realtek,rtl9312-i2c", .data = (void *) &rtl9310_i2c_drv_data },
 	{ .compatible = "realtek,rtl9313-i2c", .data = (void *) &rtl9310_i2c_drv_data },
 	{ .compatible = "realtek,rtl9607-i2c", .data = (void *) &rtl9607_i2c_drv_data },
+	{ .compatible = "realtek,rtl9607c-i2c", .data = (void *) &rtl9607_i2c_drv_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
Signature · 1 line
2.54.0
AI review
#2 lore raw

Re: [PATCH v2 1/2] i2c: rtl9300: add realtek,rtl9607c-i2c compatible

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] i2c: rtl9300: device_is_compatible check breaks DT fallback patterns --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] i2c: rtl9300: device_is_compatible check breaks DT fallback patterns --

commit 96f11e2144ef16398bba64fc512f5b346ab89571
Author: Michael Zavertkin <[email protected]>

i2c: rtl9300: add realtek,rtl9607c-i2c compatible

This commit adds support for the realtek,rtl9607c-i2c compatible string to distinguish it from other RTL9607 variants. It avoids using the IO_MODE_EN register on the new compatible by restricting the select_scl call and emits a deprecation warning for the old compatible string.

Quoted 8 lines
drivers/i2c/busses/i2c-rtl9300.c +0 -0
index 3a8225b0666c0..22aa3b38458c0 100644
@@ -200,10 +200,12 @@ static int rtl9607_i2c_config_chan(struct rtl9300_i2c *i2c, struct rtl9300_i2c_c
 	if (ret)
 		return ret;

drivers/i2c/busses/i2c-rtl9300.c static int rtl9607_i2c_config_chan(struct rtl9300_i2c *i2c, struct rtl9300_i2c_c · line 208

Quoted 10 lines
drivers/i2c/busses/i2c-rtl9300.c +6 -4
-	drv_data = device_get_match_data(i2c->dev);
-	ret = drv_data->select_scl(i2c, i2c->scl_num);
-	if (ret)
-		return ret;
+	if (device_is_compatible(i2c->dev, "realtek,rtl9607-i2c")) {
+		drv_data = device_get_match_data(i2c->dev);
+		ret = drv_data->select_scl(i2c, i2c->scl_num);
+		if (ret)
+			return ret;
+	}

[Severity: Medium]
Will this check break device tree fallback patterns?

If a new device tree provides a fallback compatible list like compatible = "realtek,rtl9607c-i2c", "realtek,rtl9607-i2c"; device_is_compatible() will return true because the legacy string is still present in the node.

If this happens, the driver will incorrectly execute the legacy select_scl hardware configuration (modifying the IO_MODE_EN register) on RTL9607C hardware, instead of delegating it to the pinctrl driver as intended.

Would it be safer to differentiate hardware capabilities based on the matched driver data (for instance, pointing the new compatible to a struct without a select_scl function) instead of dynamically searching the OF node for deprecated strings?

drivers/i2c/busses/i2c-rtl9300.c static int rtl9300_i2c_probe(struct platform_device *pdev) · line 502

Quoted 10 lines
drivers/i2c/busses/i2c-rtl9300.c +4 -0
 	i2c->sda_num = chan->sda_num;
 	return 0;

@@ -494,6 +496,10 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
 		scl_num = 0;
 	i2c->scl_num = (u8)scl_num;
 
+	if (device_is_compatible(i2c->dev, "realtek,rtl9607-i2c"))
+		dev_warn(dev, "Compatible 'realtek,rtl9607-i2c' is deprecated. "
+			      "Use 'realtek,rtl9607c-i2c' instead\n");
+

[Severity: Medium]
Does this also trigger an incorrect deprecation warning when fallback compatibles are used?

Because device_is_compatible() matches any string in the list, providing a valid fallback to support older kernels will falsely trigger this warning even if "realtek,rtl9607c-i2c" is properly specified as the primary compatible string.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author
#3 lore raw

Re: [PATCH v2 1/2] i2c: rtl9300: add realtek,rtl9607c-i2c compatible

Quoted 65 lines · On Sat, Oct 10, 2026 at 06:45:14AM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] i2c: rtl9300: device_is_compatible check breaks DT fallback patterns --

commit 96f11e2144ef16398bba64fc512f5b346ab89571
Author: Michael Zavertkin <[email protected]>

i2c: rtl9300: add realtek,rtl9607c-i2c compatible

This commit adds support for the realtek,rtl9607c-i2c compatible string to distinguish it from other RTL9607 variants. It avoids using the IO_MODE_EN register on the new compatible by restricting the select_scl call and emits a deprecation warning for the old compatible string.

Quoted 8 lines
drivers/i2c/busses/i2c-rtl9300.c +0 -0
index 3a8225b0666c0..22aa3b38458c0 100644
@@ -200,10 +200,12 @@ static int rtl9607_i2c_config_chan(struct rtl9300_i2c *i2c, struct rtl9300_i2c_c
 	if (ret)
 		return ret;

drivers/i2c/busses/i2c-rtl9300.c static int rtl9607_i2c_config_chan(struct rtl9300_i2c *i2c, struct rtl9300_i2c_c · line 208

Quoted 10 lines
drivers/i2c/busses/i2c-rtl9300.c +6 -4
-	drv_data = device_get_match_data(i2c->dev);
-	ret = drv_data->select_scl(i2c, i2c->scl_num);
-	if (ret)
-		return ret;
+	if (device_is_compatible(i2c->dev, "realtek,rtl9607-i2c")) {
+		drv_data = device_get_match_data(i2c->dev);
+		ret = drv_data->select_scl(i2c, i2c->scl_num);
+		if (ret)
+			return ret;
+	}

[Severity: Medium]
Will this check break device tree fallback patterns?

If a new device tree provides a fallback compatible list like compatible = "realtek,rtl9607c-i2c", "realtek,rtl9607-i2c"; device_is_compatible() will return true because the legacy string is still present in the node.

If this happens, the driver will incorrectly execute the legacy select_scl hardware configuration (modifying the IO_MODE_EN register) on RTL9607C hardware, instead of delegating it to the pinctrl driver as intended.

Would it be safer to differentiate hardware capabilities based on the matched driver data (for instance, pointing the new compatible to a struct without a select_scl function) instead of dynamically searching the OF node for deprecated strings?

drivers/i2c/busses/i2c-rtl9300.c static int rtl9300_i2c_probe(struct platform_device *pdev) · line 502

Quoted 10 lines
drivers/i2c/busses/i2c-rtl9300.c +4 -0
 	i2c->sda_num = chan->sda_num;
 	return 0;

@@ -494,6 +496,10 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
 		scl_num = 0;
 	i2c->scl_num = (u8)scl_num;
 
+	if (device_is_compatible(i2c->dev, "realtek,rtl9607-i2c"))
+		dev_warn(dev, "Compatible 'realtek,rtl9607-i2c' is deprecated. "
+			      "Use 'realtek,rtl9607c-i2c' instead\n");
+

[Severity: Medium]
Does this also trigger an incorrect deprecation warning when fallback compatibles are used?

Quoted 7 lines

Because device_is_compatible() matches any string in the list, providing a valid fallback to support older kernels will falsely trigger this warning even if "realtek,rtl9607c-i2c" is properly specified as the primary compatible string.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

There is no device-embedded device tree files with this compatibles, only embedded in kernel during build (OpenWRT approach), so fallback compatibles isn't expected, and proper fallback support would look like overkill right now.

author
#4 lore raw

PATCH v2 2/2 dt-bindings: i2c: rtl9300: add 'rtl9607c-i2c'

In reply to Michael Zavertkin

Add realtek,rtl9607c-i2c compatible for RTL9607C SoC and mark realtek,rtl9607-i2c as deprecated.

New compatible relies on pinctrl driver for correct pin assignment instead of directly modifying IO_MODE register.

Signed-off-by Michael Zavertkin <[email protected]>
---
 .../bindings/i2c/realtek,rtl9301-i2c.yaml     | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml +19 -1
index 5873cfdc5b3e..7c00918f33c8 100644
@@ -36,7 +36,11 @@ properties:
       - enum:
           - realtek,rtl9301-i2c
           - realtek,rtl9310-i2c
-          - realtek,rtl9607-i2c
+          - realtek,rtl9607c-i2c
+      - const: realtek,rtl9607-i2c
+        deprecated: true
+        description: >
+          Use 'realtek,rtl9607c-i2c' instead
 
   reg:
     items:

@@ -57,6 +61,10 @@ properties:
   clocks:
     maxItems: 1
 
+  pinctrl-0: true
+
+  pinctrl-names: true
+
 patternProperties:
   '^i2c@[0-9ab]$':
     $ref: /schemas/i2c/i2c-controller.yaml

@@ -96,6 +104,16 @@ allOf:
       required:
         - realtek,scl
         - clocks
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: realtek,rtl9607c-i2c
+    then:
+      required:
+        - clocks
+        - pinctrl-0
+        - pinctrl-names
 
 required:
   - compatible
Signature · 1 line
2.54.0
AI review
#5 lore raw

Re: [PATCH v2 2/2] dt-bindings: i2c: rtl9300: add 'rtl9607c-i2c'

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] dt-bindings: i2c: missing patternProperties restriction for rtl9607c-i2c --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dt-bindings: i2c: missing patternProperties restriction for rtl9607c-i2c --

commit 9c59eaea467911b21ec4b5c1d3ef7e034bb19883
Author: Michael Zavertkin <[email protected]>

dt-bindings: i2c: rtl9300: add 'rtl9607c-i2c'

Add realtek,rtl9607c-i2c compatible for RTL9607C SoC, deprecate the old one, and rely on pinctrl driver for pin assignment.

Quoted 4 lines
Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml +0 -0
index 5873cfdc5b3e0..7c00918f33c8d 100644

[ ... ]

Quoted 4 lines
Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml +0 -0
@@ -96,6 +104,16 @@ allOf:
       required:
         - realtek,scl
         - clocks

Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml allOf: · line 116

Quoted 10 lines
Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml +10 -0
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: realtek,rtl9607c-i2c
+    then:
+      required:
+        - clocks
+        - pinctrl-0
+        - pinctrl-names

[Severity: Low]
Does this conditional block need to restrict the allowed child nodes?

The RTL9607C hardware only supports 1 channel, but the base schema permits any child node matching '^i2c@[0-9ab]$'.

Without adding a restriction such as patternProperties: '^i2c@[1-9ab]$': false to this block, dt_binding_check could incorrectly pass single child nodes with invalid reg indices, like i2c@1 or i2c@b.

Since the driver ignores the SDA index during configuration, it would accept these invalid reg values without error.

Could we restrict the pattern here to prevent invalid channel definitions?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2