Skip to contentSkip to conversation

i2c: rtl9300: remove IO_MODE configuration

PATCH Superseded Michael Zavertkin 3 participants 9 messages Oct 4 – Oct 7

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

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Michael Zavertkin

author
#0 lore raw

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.

Remove scl configuration (used only to modify IO_MODE register) and reflect these changes in Documentation.

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

---
Michael Zavertkin (2):
i2c: rtl9300: remove IO_MODE configuration
dt-bindings: i2c: rtl9300: add pinctrl

 .../devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml |  7 ++++++-
 drivers/i2c/busses/i2c-rtl9300.c                     | 12 +++---------
 2 files changed, 9 insertions(+), 10 deletions(-)
Signature · 1 line
2.54.0
author
#1 lore raw

PATCH 1/2 i2c: rtl9300: remove IO_MODE configuration

IO_MODE is used to route peripheral controller to SoC pins. This is a job for pinctrl driver.

Since select_scl now have nothing to do, remove it.

Signed-off-by Michael Zavertkin <[email protected]>
---
 drivers/i2c/busses/i2c-rtl9300.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)
drivers/i2c/busses/i2c-rtl9300.c +3 -9
index 3a8225b0666c..c36c29e1833e 100644
@@ -130,7 +130,6 @@ struct rtl9300_i2c_xfer {
 #define RTL9310_I2C_MST_DATA_CTRL			0x8
 
 #define RTL9607_I2C_CONFIG				0x22f50
-#define RTL9607_IO_MODE_EN				0x23014
 #define RTL9607_I2C_IND_WD				0x0
 #define RTL9607_I2C_IND_ADR				0x8
 #define RTL9607_I2C_IND_CMD				0x10

@@ -190,7 +189,6 @@ static int rtl9300_i2c_config_chan(struct rtl9300_i2c *i2c, struct rtl9300_i2c_c
 
 static int rtl9607_i2c_config_chan(struct rtl9300_i2c *i2c, struct rtl9300_i2c_chan *chan)
 {
-	const struct rtl9300_i2c_drv_data *drv_data;
 	int ret;
 
 	if (i2c->sda_num == chan->sda_num)

@@ -200,11 +198,6 @@ 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;
-
 	i2c->sda_num = chan->sda_num;
 	return 0;
 }

@@ -619,7 +612,6 @@ static const struct rtl9300_i2c_drv_data rtl9310_i2c_drv_data = {
 
 static const struct rtl9300_i2c_drv_data rtl9607_i2c_drv_data = {
 	.field_desc = {
-		[F_SCL_SEL]		= GLB_REG_FIELD(RTL9607_IO_MODE_EN, 13, 14),
 		[F_EXT_SCK_5MS]		= MST_REG_FIELD(RTL9607_I2C_CONFIG, 26, 26),
 		[F_DEV_ADDR]		= MST_REG_FIELD(RTL9607_I2C_CONFIG, 14, 20),
 		[F_MEM_ADDR_WIDTH]	= MST_REG_FIELD(RTL9607_I2C_CONFIG, 12, 13),

@@ -631,7 +623,9 @@ static const struct rtl9300_i2c_drv_data rtl9607_i2c_drv_data = {
 		[F_I2C_TRIG]		= MST_REG_FIELD(RTL9607_I2C_IND_CMD, 0, 0),
 		[F_MEM_ADDR]		= MST_REG_FIELD(RTL9607_I2C_IND_ADR, 0, 31),
 	},
-	.select_scl = rtl9310_i2c_select_scl,
+	/*
+	 * No select_scl - single channel available
+	 */
 	.config_chan = rtl9607_i2c_config_chan,
 	.config_clock = rtl9607_i2c_config_clock,
 	.misc_init = rtl9607_i2c_init,
Signature · 1 line
2.54.0
author
#2 lore raw

PATCH 2/2 dt-bindings: i2c: rtl9300: add pinctrl

In reply to Michael Zavertkin

For RTL9607C driver used to configure IO_MODE register. This register is global and is a scope of pinctrl driver (either pinctrl-single or WIP driver for SoC).

Drop realtek,scl requirement for that SoC and require pinctrl instead.

Signed-off-by Michael Zavertkin <[email protected]>
---
 .../devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml       | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml +6 -1
index 5873cfdc5b3e..ad143509aa42 100644
@@ -57,6 +57,10 @@ properties:
   clocks:
     maxItems: 1
 
+  pinctrl-0: true
+
+  pinctrl-names: true
+
 patternProperties:
   '^i2c@[0-9ab]$':
     $ref: /schemas/i2c/i2c-controller.yaml

@@ -94,8 +98,9 @@ allOf:
             const: realtek,rtl9607-i2c
     then:
       required:
-        - realtek,scl
         - clocks
+        - pinctrl-0
+        - pinctrl-names
 
 required:
   - compatible
Signature · 1 line
2.54.0
AI review
#3 lore raw

Re: [PATCH 2/2] dt-bindings: i2c: rtl9300: add pinctrl

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] dt-bindings: ABI breakage by making pinctrl required for rtl9607-i2c --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] dt-bindings: ABI breakage by making pinctrl required for rtl9607-i2c --

commit b162c61d2fc7ab941d4ea49cc274452fca8d089d
Author: Michael Zavertkin <[email protected]>

dt-bindings: i2c: rtl9300: add pinctrl

For RTL9607C driver used to configure IO_MODE register. This register is global and is a scope of pinctrl driver (either pinctrl-single or WIP driver for SoC). Drop realtek,scl requirement for that SoC and require pinctrl instead.

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

[ ... ]

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

Quoted 8 lines
Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml +2 -1
@@ -94,8 +98,9 @@ allOf:
             const: realtek,rtl9607-i2c
     then:
       required:
-        - realtek,scl
         - clocks
+        - pinctrl-0
+        - pinctrl-names

[Severity: Critical]
Does making these properties strictly required introduce a regression for existing device trees?

Device tree bindings are considered stable ABI, and changes must be backward compatible. The realtek,rtl9607-i2c compatible string was already present in the bindings. Existing valid device trees for this SoC do not contain pinctrl-0 or pinctrl-names because the previous schema used
additionalProperties: false without defining them.

Adding pinctrl-0 and pinctrl-names to the required list in the allOf block for rtl9607-i2c means all existing, deployed device trees will now fail schema validation during dtbs_check.

Could these new properties be made optional to prevent ABI breakage?

Quoted 3 lines
Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml +0 -0
 required:
   - compatible
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#4 lore raw

Re: [PATCH 2/2] dt-bindings: i2c: rtl9300: add pinctrl

In reply to Michael Zavertkin
Quoted 5 lines · Michael Zavertkin

For RTL9607C driver used to configure IO_MODE register. This register is global and is a scope of pinctrl driver (either pinctrl-single or WIP driver for SoC).

Drop realtek,scl requirement for that SoC and require pinctrl instead.

This patch itself is probably fine, but the driver changes will cause regressions because all existing devicetrees are going to have realtek,scl and not the pinctrl properties. You need to retain the driver code and make the register setting optional depending on if the scl is there (or similar).
pw-bot: changes-requested

Thanks,
Conor.

Quoted 35 lines
Signed-off-by Michael Zavertkin <[email protected]>
---
 .../devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml       | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml +6 -1
index 5873cfdc5b3e..ad143509aa42 100644
@@ -57,6 +57,10 @@ properties:
   clocks:
     maxItems: 1
 
+  pinctrl-0: true
+
+  pinctrl-names: true
+
 patternProperties:
   '^i2c@[0-9ab]$':
     $ref: /schemas/i2c/i2c-controller.yaml

@@ -94,8 +98,9 @@ allOf:
             const: realtek,rtl9607-i2c
     then:
       required:
-        - realtek,scl
         - clocks
+        - pinctrl-0
+        - pinctrl-names
 
 required:
   - compatible
Signature · 1 line
2.54.0
author
#5 lore raw

Re: [PATCH 2/2] dt-bindings: i2c: rtl9300: add pinctrl

Quoted 14 lines · Conor Dooley
Quoted 5 lines · Michael Zavertkin

For RTL9607C driver used to configure IO_MODE register. This register is global and is a scope of pinctrl driver (either pinctrl-single or WIP driver for SoC).

Drop realtek,scl requirement for that SoC and require pinctrl instead.

This patch itself is probably fine, but the driver changes will cause regressions because all existing devicetrees are going to have realtek,scl and not the pinctrl properties. You need to retain the driver code and make the register setting optional depending on if the scl is there (or similar).
pw-bot: changes-requested

Quoted 2 lines

Thanks,
Conor.

I understand backward compatibilyti requirement. However all known users of this driver are here (me and Rustam Adilov). Does compatibility strictly required for such case?

If so, what would be better - print warning about realtek,scl (only for RTL9607C) and work as earlier, potentially interfering with pinctrl driver - or abort probe with error if there is no pinctrl property? Explicit error or message is better then silent regression, if I understand you correctly.

#6 lore raw

Re: [PATCH 2/2] dt-bindings: i2c: rtl9300: add pinctrl

Quoted 15 lines · Michael Zavertkin
Quoted 14 lines · Conor Dooley
Quoted 5 lines · Michael Zavertkin

For RTL9607C driver used to configure IO_MODE register. This register is global and is a scope of pinctrl driver (either pinctrl-single or WIP driver for SoC).

Drop realtek,scl requirement for that SoC and require pinctrl instead.

This patch itself is probably fine, but the driver changes will cause regressions because all existing devicetrees are going to have realtek,scl and not the pinctrl properties. You need to retain the driver code and make the register setting optional depending on if the scl is there (or similar).
pw-bot: changes-requested

Quoted 6 lines
Quoted 2 lines

Thanks,
Conor.

I understand backward compatibilyti requirement. However all known users of this driver are here (me and Rustam Adilov). Does compatibility strictly required for such case?

How do you know there's only the two of you?

Quoted 5 lines

If so, what would be better - print warning about realtek,scl (only for RTL9607C) and work as earlier, potentially interfering with pinctrl driver - or abort probe with error if there is no pinctrl property? Explicit error or message is better then silent regression, if I understand you correctly.

If there's genuinely only two users, then I suppose you can break it. But two known users doesn't meant two users. Otherwise, use pintrl if available and use the fallback to the existing property if not. Mark the property deprecated and probably emit a warning.

author
#7 lore raw

Re: [PATCH 2/2] dt-bindings: i2c: rtl9300: add pinctrl

Quoted 10 lines · Conor Dooley
Quoted 5 lines

If so, what would be better - print warning about realtek,scl (only for RTL9607C) and work as earlier, potentially interfering with pinctrl driver - or abort probe with error if there is no pinctrl property? Explicit error or message is better then silent regression, if I understand you correctly.

If there's genuinely only two users, then I suppose you can break it. But two known users doesn't meant two users. Otherwise, use pintrl if available and use the fallback to the existing property if not. Mark the property deprecated and probably emit a warning.

Okay, I'll prepare v2. Support for deprecated property should be removed somewhere in future, right?

#8 lore raw

Re: [PATCH 2/2] dt-bindings: i2c: rtl9300: add pinctrl

Quoted 7 lines · Michael Zavertkin
Quoted 6 lines · Conor Dooley
Quoted 5 lines

If so, what would be better - print warning about realtek,scl (only for RTL9607C) and work as earlier, potentially interfering with pinctrl driver - or abort probe with error if there is no pinctrl property? Explicit error or message is better then silent regression, if I understand you correctly.

Quoted 7 lines
Quoted 4 lines

If there's genuinely only two users, then I suppose you can break it. But two known users doesn't meant two users. Otherwise, use pintrl if available and use the fallback to the existing property if not. Mark the property deprecated and probably emit a warning.

Okay, I'll prepare v2. Support for deprecated property should be removed somewhere in future, right?

Sure