Skip to contentSkip to conversation

i2c: rtl9300: remove IO_MODE configuration

PATCH In review Michael Zavertkin 3 participants 5 messages Nov 15

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v3 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.

---
Changes in v3:
- Added Acked-by
- Link to v2: https://patch.msgid.link/[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.55.0
#1 lore raw

PATCH v3 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.

---
 drivers/i2c/busses/i2c-rtl9300.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-rtl9300.c +11 -4
index a9dd85a286d4..ae20fbdc307a 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.55.0
#2 lore raw

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

Quoted 3 lines · Michael Zavertkin

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.

Yeah, that makes sense. Yesterday i spent some time documenting the rtl960x tree [1] and rtl9607 falls into apollomp rather than rtl9607c family.

We should ideally check if RTL9607 and RTL9607C have similar enough i2c controllers to use same compatible but maybe for another time. Cause if it turns out they are different then the rtl9607-i2c should not be deprecated? I don't know what the rules are for this particular case where due to lack of knowledge at a time we have been using a wrong compatible all this time.

[1] https://github.com/Anime4000/RTL960x#rtl960x-families-and-their-subtypes

Quoted 48 lines

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

---
 drivers/i2c/busses/i2c-rtl9300.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-rtl9300.c +11 -4
index a9dd85a286d4..ae20fbdc307a 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);
#3 lore raw

PATCH v3 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.

---
 .../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.55.0
#4 lore raw

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

Thank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Low] dt-bindings: i2c: rtl9301: per-compatible conditional pile-up for rtl9607c --

Thank you for your contribution! Sashiko AI review found 1 potential issue to consider:
- [Low] dt-bindings: i2c: rtl9301: per-compatible conditional pile-up for rtl9607c
--

commit 43c34d3c3c011204c713fd4d1ed08c55685b830d
Author: Michael Zavertkin <[email protected]>

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

This commit adds the realtek,rtl9607c-i2c compatible string with required clocks and pinctrl properties, and deprecates realtek,rtl9607-i2c.

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

[ ... ]

Quoted 18 lines
Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml +7 -0
@@ -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:

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

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

[Severity: Low]
Would it be cleaner to split realtek,rtl9607c-i2c into a separate schema document rather than adding a fourth per-compatible if-then block to allOf?

Unlike RTL9300 and RTL9310, which multiplex 8 or 12 shared SDA lines across controllers and select the SCL line via realtek,scl, RTL9607C has a single SCL/SDA pair per controller and uses pinctrl instead of realtek,scl. Keeping realtek,rtl9607c-i2c in this shared schema also leaves the unused realtek,scl property and multi-channel i2c@[0-9ab] child nodes permitted for it.

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