[PATCH v2 0/2] i2c: rtl9300: remove IO_MODE configuration

[PATCH v2 0/2] i2c: rtl9300: remove IO_MODE configuration

[Michael Zavertkin]

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 <misha.zavertkin@mail.ru>
---
Changes in v2:
- Backward compatibility achieved by adding new compatible value. Old
  one is marked as deprecated
- Link to v1: https://patch.msgid.link/cover.1791127538.git.misha.zavertkin@mail.ru
---
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(-)

-- 
2.54.0

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

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

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 <misha.zavertkin@mail.ru>
---
 drivers/i2c/busses/i2c-rtl9300.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index 3a8225b0666c..22aa3b38458c 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -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);
-- 
2.54.0

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

[sashiko-bot]

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 <misha.zavertkin@mail.ru>

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.

> diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
> index 3a8225b0666c0..22aa3b38458c0 100644
> --- a/drivers/i2c/busses/i2c-rtl9300.c
> +++ b/drivers/i2c/busses/i2c-rtl9300.c
> @@ -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;
> +	}

[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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791611999.git.misha.zavertkin@mail.ru?part=1

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

[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 <misha.zavertkin@mail.ru>
---
 .../bindings/i2c/realtek,rtl9301-i2c.yaml     | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
index 5873cfdc5b3e..7c00918f33c8 100644
--- a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
@@ -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
-- 
2.54.0

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

[sashiko-bot]

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 <misha.zavertkin@mail.ru>

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.

> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
> index 5873cfdc5b3e0..7c00918f33c8d 100644
> --- a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.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

[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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791611999.git.misha.zavertkin@mail.ru?part=2