[PATCH v2 0/3] i2c: rework and extend RTL9300 I2C driver

[PATCH v2 0/3] i2c: rework and extend RTL9300 I2C driver

[Jonas Jelonek]

This patch series reworks the current implementation of the driver for
I2C controller integrated into RTL9300 SoCs to simplify support
extension, and adds support for the RTL9310 series.
Goal of this is to have RTL9310 support upstream in a proper
implementation to be able to drop downstream versions of this driver.

The first patch reworks the driver to use more of the regmap API.
Instead of using macros, all registers are defined as reg_field and
operations on these registers are performed using regmap_field and the
corresponding API. This simplifies adding support for further chip
families and avoids potential redundant code by just providing
chip-specific functions for every chip family.

The second patch extends the existing dt-bindings of RTL9300 for RTL9310
support.

The third patch makes use of previous changes by adding support for the
RTL9310 series, providing the correct register definitions and a few
specifics. This also uses a new vendor dt-property which was added by
the second patch to properly manage the I2C controllers. Having this
property is necessary to properly describe the hardware and allow the
driver to correctly work with the I2C controllers.

Both has been tested successfully on RTL9302B-based Zyxel XGS1210-12
and RTL9313-based Netgear MS510TXM.

Compile-tested with Linux, run-tested as backport in OpenWrt on the
aforementioned devices.

--
Changelog

v2: - Patch 1: 
	- adjusted commit message
    	- retained Tested-By and Reviewed-By from Chris Packham
    - Patch 2:
    	- simplified check as suggested by Markus Stockhausen
	- fixed commit message
    - Patch 3 (all requested by Krzysztof):
    	- use vendor property instead of generic
	- add front compatibles to make binding complete
	- fix commit message
    - reordered patches, dt-bindings patch now comes before its 'user'
    - properly add device-tree list and relevant maintainers to To/Cc

--

Jonas Jelonek (3):
  i2c: rework RTL9300 I2C controller driver
  dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support
  i2c: add RTL9310 support to RTL9300 I2C controller driver

 .../bindings/i2c/realtek,rtl9301-i2c.yaml     |  38 ++-
 drivers/i2c/busses/i2c-rtl9300.c              | 231 +++++++++++++-----
 2 files changed, 199 insertions(+), 70 deletions(-)

-- 
2.48.1

[PATCH v2 1/3] i2c: rework RTL9300 I2C controller driver

[Jonas Jelonek]

Rework the RTL9300 I2C controller driver to use more of the regmap
API, especially make use of reg_field and regmap_field to represent
registers instead of macros. Most register operations are performed
through regmap_field_* API then.

Handle SCL selection using separate chip-specific functions since this
is already known to differ between the Realtek SoC families in such a
way that this cannot be properly handled using just a different
reg_field.

These changes make it a lot easier to add support for newer generations
or to handle differences between specific revisions within a series.
Support can be added by defining a separate driver data structure with
the corresponding register field definitions and linking it to a new
compatible string.

Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz> # On RTL9302c
Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
 drivers/i2c/busses/i2c-rtl9300.c | 190 ++++++++++++++++++++-----------
 1 file changed, 124 insertions(+), 66 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index e064e8a4a1f0..f8e81102ee74 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -23,94 +23,115 @@ struct rtl9300_i2c_chan {
 	u8 sda_pin;
 };
 
+enum rtl9300_i2c_reg_scope {
+	REG_SCOPE_GLOBAL,
+	REG_SCOPE_MASTER,
+};
+
+struct rtl9300_i2c_reg_field {
+	struct reg_field field;
+	enum rtl9300_i2c_reg_scope scope;
+};
+
+enum rtl9300_i2c_reg_fields {
+	F_DATA_WIDTH = 0,
+	F_DEV_ADDR,
+	F_I2C_FAIL,
+	F_I2C_TRIG,
+	F_MEM_ADDR,
+	F_MEM_ADDR_WIDTH,
+	F_RD_MODE,
+	F_RWOP,
+	F_SCL_FREQ,
+	F_SCL_SEL,
+	F_SDA_OUT_SEL,
+	F_SDA_SEL,
+
+	/* keep last */
+	F_NUM_FIELDS
+};
+
+struct rtl9300_i2c_drv_data {
+	struct rtl9300_i2c_reg_field field_desc[F_NUM_FIELDS];
+	int (*select_scl)(struct rtl9300_i2c *i2c, u8 scl);
+	u32 data_reg;
+	u8 max_nchan;
+};
+
 #define RTL9300_I2C_MUX_NCHAN	8
 
 struct rtl9300_i2c {
 	struct regmap *regmap;
 	struct device *dev;
 	struct rtl9300_i2c_chan chans[RTL9300_I2C_MUX_NCHAN];
+	struct regmap_field *fields[F_NUM_FIELDS];
 	u32 reg_base;
+	u32 data_reg;
 	u8 sda_pin;
 	struct mutex lock;
 };
 
 #define RTL9300_I2C_MST_CTRL1				0x0
-#define  RTL9300_I2C_MST_CTRL1_MEM_ADDR_OFS		8
-#define  RTL9300_I2C_MST_CTRL1_MEM_ADDR_MASK		GENMASK(31, 8)
-#define  RTL9300_I2C_MST_CTRL1_SDA_OUT_SEL_OFS		4
-#define  RTL9300_I2C_MST_CTRL1_SDA_OUT_SEL_MASK		GENMASK(6, 4)
-#define  RTL9300_I2C_MST_CTRL1_GPIO_SCL_SEL		BIT(3)
-#define  RTL9300_I2C_MST_CTRL1_RWOP			BIT(2)
-#define  RTL9300_I2C_MST_CTRL1_I2C_FAIL			BIT(1)
-#define  RTL9300_I2C_MST_CTRL1_I2C_TRIG			BIT(0)
 #define RTL9300_I2C_MST_CTRL2				0x4
-#define  RTL9300_I2C_MST_CTRL2_RD_MODE			BIT(15)
-#define  RTL9300_I2C_MST_CTRL2_DEV_ADDR_OFS		8
-#define  RTL9300_I2C_MST_CTRL2_DEV_ADDR_MASK		GENMASK(14, 8)
-#define  RTL9300_I2C_MST_CTRL2_DATA_WIDTH_OFS		4
-#define  RTL9300_I2C_MST_CTRL2_DATA_WIDTH_MASK		GENMASK(7, 4)
-#define  RTL9300_I2C_MST_CTRL2_MEM_ADDR_WIDTH_OFS	2
-#define  RTL9300_I2C_MST_CTRL2_MEM_ADDR_WIDTH_MASK	GENMASK(3, 2)
-#define  RTL9300_I2C_MST_CTRL2_SCL_FREQ_OFS		0
-#define  RTL9300_I2C_MST_CTRL2_SCL_FREQ_MASK		GENMASK(1, 0)
 #define RTL9300_I2C_MST_DATA_WORD0			0x8
 #define RTL9300_I2C_MST_DATA_WORD1			0xc
 #define RTL9300_I2C_MST_DATA_WORD2			0x10
 #define RTL9300_I2C_MST_DATA_WORD3			0x14
-
 #define RTL9300_I2C_MST_GLB_CTRL			0x384
 
+
 static int rtl9300_i2c_reg_addr_set(struct rtl9300_i2c *i2c, u32 reg, u16 len)
 {
-	u32 val, mask;
 	int ret;
 
-	val = len << RTL9300_I2C_MST_CTRL2_MEM_ADDR_WIDTH_OFS;
-	mask = RTL9300_I2C_MST_CTRL2_MEM_ADDR_WIDTH_MASK;
-
-	ret = regmap_update_bits(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL2, mask, val);
+	ret = regmap_field_write(i2c->fields[F_MEM_ADDR_WIDTH], len);
 	if (ret)
 		return ret;
 
-	val = reg << RTL9300_I2C_MST_CTRL1_MEM_ADDR_OFS;
-	mask = RTL9300_I2C_MST_CTRL1_MEM_ADDR_MASK;
+	return regmap_field_write(i2c->fields[F_MEM_ADDR], reg);
+}
 
-	return regmap_update_bits(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL1, mask, val);
+static int rtl9300_i2c_select_scl(struct rtl9300_i2c *i2c, u8 scl)
+{
+	return regmap_field_write(i2c->fields[F_SCL_SEL], 1);
 }
 
 static int rtl9300_i2c_config_io(struct rtl9300_i2c *i2c, u8 sda_pin)
 {
+	struct rtl9300_i2c_drv_data *drv_data;
 	int ret;
-	u32 val, mask;
 
-	ret = regmap_update_bits(i2c->regmap, RTL9300_I2C_MST_GLB_CTRL, BIT(sda_pin), BIT(sda_pin));
+	drv_data = (struct rtl9300_i2c_drv_data *)device_get_match_data(i2c->dev);
+
+	ret = regmap_field_update_bits(i2c->fields[F_SDA_SEL], BIT(sda_pin), BIT(sda_pin));
 	if (ret)
 		return ret;
 
-	val = (sda_pin << RTL9300_I2C_MST_CTRL1_SDA_OUT_SEL_OFS) |
-		RTL9300_I2C_MST_CTRL1_GPIO_SCL_SEL;
-	mask = RTL9300_I2C_MST_CTRL1_SDA_OUT_SEL_MASK | RTL9300_I2C_MST_CTRL1_GPIO_SCL_SEL;
+	ret = regmap_field_write(i2c->fields[F_SDA_OUT_SEL], sda_pin);
+	if (ret)
+		return ret;
 
-	return regmap_update_bits(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL1, mask, val);
+	return drv_data->select_scl(i2c, 0);
 }
 
 static int rtl9300_i2c_config_xfer(struct rtl9300_i2c *i2c, struct rtl9300_i2c_chan *chan,
 				   u16 addr, u16 len)
 {
-	u32 val, mask;
-
-	val = chan->bus_freq << RTL9300_I2C_MST_CTRL2_SCL_FREQ_OFS;
-	mask = RTL9300_I2C_MST_CTRL2_SCL_FREQ_MASK;
+	int ret;
 
-	val |= addr << RTL9300_I2C_MST_CTRL2_DEV_ADDR_OFS;
-	mask |= RTL9300_I2C_MST_CTRL2_DEV_ADDR_MASK;
+	ret = regmap_field_write(i2c->fields[F_SCL_FREQ], chan->bus_freq);
+	if (ret)
+		return ret;
 
-	val |= ((len - 1) & 0xf) << RTL9300_I2C_MST_CTRL2_DATA_WIDTH_OFS;
-	mask |= RTL9300_I2C_MST_CTRL2_DATA_WIDTH_MASK;
+	ret = regmap_field_write(i2c->fields[F_DEV_ADDR], addr);
+	if (ret)
+		return ret;
 
-	mask |= RTL9300_I2C_MST_CTRL2_RD_MODE;
+	ret = regmap_field_write(i2c->fields[F_DATA_WIDTH], (len - 1) & 0xf);
+	if (ret)
+		return ret;
 
-	return regmap_update_bits(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL2, mask, val);
+	return regmap_field_write(i2c->fields[F_RD_MODE], 0);
 }
 
 static int rtl9300_i2c_read(struct rtl9300_i2c *i2c, u8 *buf, int len)
@@ -121,8 +142,7 @@ static int rtl9300_i2c_read(struct rtl9300_i2c *i2c, u8 *buf, int len)
 	if (len > 16)
 		return -EIO;
 
-	ret = regmap_bulk_read(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0,
-			       vals, ARRAY_SIZE(vals));
+	ret = regmap_bulk_read(i2c->regmap, i2c->data_reg, vals, ARRAY_SIZE(vals));
 	if (ret)
 		return ret;
 
@@ -149,49 +169,46 @@ static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len)
 		vals[i/4] |= buf[i];
 	}
 
-	return regmap_bulk_write(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0,
-				vals, ARRAY_SIZE(vals));
+	return regmap_bulk_write(i2c->regmap, i2c->data_reg, vals, ARRAY_SIZE(vals));
 }
 
 static int rtl9300_i2c_writel(struct rtl9300_i2c *i2c, u32 data)
 {
-	return regmap_write(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0, data);
+	return regmap_write(i2c->regmap, i2c->data_reg, data);
 }
 
 static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,
 				    int size, union i2c_smbus_data *data, int len)
 {
-	u32 val, mask;
+	u32 val;
 	int ret;
 
-	val = read_write == I2C_SMBUS_WRITE ? RTL9300_I2C_MST_CTRL1_RWOP : 0;
-	mask = RTL9300_I2C_MST_CTRL1_RWOP;
-
-	val |= RTL9300_I2C_MST_CTRL1_I2C_TRIG;
-	mask |= RTL9300_I2C_MST_CTRL1_I2C_TRIG;
+	ret = regmap_field_write(i2c->fields[F_RWOP], read_write == I2C_SMBUS_WRITE);
+	if (ret)
+		return ret;
 
-	ret = regmap_update_bits(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL1, mask, val);
+	ret = regmap_field_write(i2c->fields[F_I2C_TRIG], 1);
 	if (ret)
 		return ret;
 
-	ret = regmap_read_poll_timeout(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL1,
-				       val, !(val & RTL9300_I2C_MST_CTRL1_I2C_TRIG), 100, 2000);
+	regmap_field_read_poll_timeout(i2c->fields[F_I2C_TRIG], val, !val, 100, 2000);
 	if (ret)
 		return ret;
 
-	if (val & RTL9300_I2C_MST_CTRL1_I2C_FAIL)
+	ret = regmap_field_read(i2c->fields[F_I2C_FAIL], &val);
+	if (ret)
+		return ret;
+	if (val)
 		return -EIO;
 
 	if (read_write == I2C_SMBUS_READ) {
 		if (size == I2C_SMBUS_BYTE || size == I2C_SMBUS_BYTE_DATA) {
-			ret = regmap_read(i2c->regmap,
-					  i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0, &val);
+			ret = regmap_read(i2c->regmap, i2c->data_reg, &val);
 			if (ret)
 				return ret;
 			data->byte = val & 0xff;
 		} else if (size == I2C_SMBUS_WORD_DATA) {
-			ret = regmap_read(i2c->regmap,
-					  i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0, &val);
+			ret = regmap_read(i2c->regmap, i2c->data_reg, &val);
 			if (ret)
 				return ret;
 			data->word = val & 0xffff;
@@ -331,6 +348,8 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
 	u32 clock_freq, sda_pin;
 	int ret, i = 0;
 	struct fwnode_handle *child;
+	struct rtl9300_i2c_drv_data *drv_data;
+	struct reg_field fields[F_NUM_FIELDS];
 
 	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
@@ -349,9 +368,22 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, i2c);
 
-	if (device_get_child_node_count(dev) >= RTL9300_I2C_MUX_NCHAN)
+	drv_data = (struct rtl9300_i2c_drv_data *)device_get_match_data(i2c->dev);
+	if (device_get_child_node_count(dev) >= drv_data->max_nchan)
 		return dev_err_probe(dev, -EINVAL, "Too many channels\n");
 
+	i2c->data_reg = i2c->reg_base + drv_data->data_reg;
+	for (i = 0; i < F_NUM_FIELDS; i++) {
+		fields[i] = drv_data->field_desc[i].field;
+		if (drv_data->field_desc[i].scope == REG_SCOPE_MASTER)
+			fields[i].reg += i2c->reg_base;
+	}
+	ret = devm_regmap_field_bulk_alloc(dev, i2c->regmap, i2c->fields,
+					   fields, F_NUM_FIELDS);
+	if (ret)
+		return ret;
+
+	i = 0;
 	device_for_each_child_node(dev, child) {
 		struct rtl9300_i2c_chan *chan = &i2c->chans[i];
 		struct i2c_adapter *adap = &chan->adap;
@@ -400,11 +432,37 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
 	return 0;
 }
 
+#define GLB_REG_FIELD(reg, msb, lsb)    \
+	{ .field = REG_FIELD(reg, msb, lsb), .scope = REG_SCOPE_GLOBAL }
+#define MST_REG_FIELD(reg, msb, lsb)    \
+	{ .field = REG_FIELD(reg, msb, lsb), .scope = REG_SCOPE_MASTER }
+
+static const struct rtl9300_i2c_drv_data rtl9300_i2c_drv_data = {
+	.field_desc = {
+		[F_MEM_ADDR]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL1, 8, 31),
+		[F_SDA_OUT_SEL]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL1, 4, 6),
+		[F_SCL_SEL]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL1, 3, 3),
+		[F_RWOP]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL1, 2, 2),
+		[F_I2C_FAIL]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL1, 1, 1),
+		[F_I2C_TRIG]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL1, 0, 0),
+		[F_RD_MODE]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL2, 15, 15),
+		[F_DEV_ADDR]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL2, 8, 14),
+		[F_DATA_WIDTH]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL2, 4, 7),
+		[F_MEM_ADDR_WIDTH]	= MST_REG_FIELD(RTL9300_I2C_MST_CTRL2, 2, 3),
+		[F_SCL_FREQ]		= MST_REG_FIELD(RTL9300_I2C_MST_CTRL2, 0, 1),
+		[F_SDA_SEL]		= GLB_REG_FIELD(RTL9300_I2C_MST_GLB_CTRL, 0, 7),
+	},
+	.select_scl = rtl9300_i2c_select_scl,
+	.data_reg = RTL9300_I2C_MST_DATA_WORD0,
+	.max_nchan = RTL9300_I2C_MUX_NCHAN,
+};
+
+
 static const struct of_device_id i2c_rtl9300_dt_ids[] = {
-	{ .compatible = "realtek,rtl9301-i2c" },
-	{ .compatible = "realtek,rtl9302b-i2c" },
-	{ .compatible = "realtek,rtl9302c-i2c" },
-	{ .compatible = "realtek,rtl9303-i2c" },
+	{ .compatible = "realtek,rtl9301-i2c", .data = (void *) &rtl9300_i2c_drv_data },
+	{ .compatible = "realtek,rtl9302b-i2c", .data = (void *) &rtl9300_i2c_drv_data },
+	{ .compatible = "realtek,rtl9302c-i2c", .data = (void *) &rtl9300_i2c_drv_data },
+	{ .compatible = "realtek,rtl9303-i2c", .data = (void *) &rtl9300_i2c_drv_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
-- 
2.48.1

[PATCH v2 2/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Jonas Jelonek]

Add dt-bindings for RTL9310 series I2C controller.

Adjust the regex for child-node address to account for the fact that
RTL9310 supports 12 instead of only 8 SDA lines.

Add a vendor-specific property to explicitly specify the
Realtek-internal ID of the defined I2C controller/master. This is
required, in particular for RTL9310, to describe the correct I2C
master.

Add compatibles for known SoC variants RTL9311, RTL9312 and RTL9313.

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
 .../bindings/i2c/realtek,rtl9301-i2c.yaml     | 37 +++++++++++++++++--
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
index 69ac5db8b914..54327ca09e14 100644
--- a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
@@ -10,9 +10,11 @@ maintainers:
   - Chris Packham <chris.packham@alliedtelesis.co.nz>
 
 description:
-  The RTL9300 SoC has two I2C controllers. Each of these has an SCL line (which
-  if not-used for SCL can be a GPIO). There are 8 common SDA lines that can be
-  assigned to either I2C controller.
+  The RTL9300 SoCs have two I2C controllers. Each of these has an SCL line
+  (which if not-used for SCL can be a GPIO). There are 8 common SDA lines
+  that can be assigned to either I2C controller.
+  The RTL9310 SoCs have equal capabilities but support 12 common SDA lines
+  which can be assigned to either I2C controller.
 
 properties:
   compatible:
@@ -23,7 +25,14 @@ properties:
               - realtek,rtl9302c-i2c
               - realtek,rtl9303-i2c
           - const: realtek,rtl9301-i2c
+      - items:
+          - enum:
+              - realtek,rtl9311-i2c
+              - realtek,rtl9312-i2c
+              - realtek,rtl9313-i2c
+          - const: realtek,rtl9310-i2c
       - const: realtek,rtl9301-i2c
+      - const: realtek,rtl9310-i2c
 
   reg:
     items:
@@ -35,8 +44,16 @@ properties:
   "#size-cells":
     const: 0
 
+  realtek,mst-id:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      Realtek-internal ID of the I2C controller/master. Only required
+      for RTL9310 series.
+    minimum: 1
+    maximum: 2
+
 patternProperties:
-  '^i2c@[0-7]$':
+  '^i2c@([0-9]|1[0-1])$':
     $ref: /schemas/i2c/i2c-controller.yaml
     unevaluatedProperties: false
 
@@ -68,3 +85,15 @@ examples:
         #size-cells = <0>;
       };
     };
+  - |
+    i2c@100c {
+      compatible = "realtek,rtl9310-i2c";
+      reg = <0x100c 0x18>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+      realtek,mst-id = <1>;
+
+      i2c@0 {
+        reg = <0>;
+      };
+    };
-- 
2.48.1

[PATCH v2 3/3] i2c: add RTL9310 support to RTL9300 I2C controller driver

[Jonas Jelonek]

Add support for the internal I2C controllers of RTL9310 series based
SoCs to the driver for RTL9300. Add register definitions, chip-specific
functions and compatible strings for known RTL9310-based SoCs RTL9311,
RTL9312 and RTL9313.

Make use of a new device tree property 'realtek,mst-id' which needs to
be specified in case both or only the second master is used. This is
required due how the register layout changed in contrast to RTL9300,
which has SCL selection in a global register instead of a
master-specific one.

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
 drivers/i2c/busses/i2c-rtl9300.c | 43 +++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index f8e81102ee74..9bd8a62a2ba0 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -59,6 +59,7 @@ struct rtl9300_i2c_drv_data {
 };
 
 #define RTL9300_I2C_MUX_NCHAN	8
+#define RTL9310_I2C_MUX_NCHAN	12
 
 struct rtl9300_i2c {
 	struct regmap *regmap;
@@ -67,6 +68,7 @@ struct rtl9300_i2c {
 	struct regmap_field *fields[F_NUM_FIELDS];
 	u32 reg_base;
 	u32 data_reg;
+	u8 scl_num;
 	u8 sda_pin;
 	struct mutex lock;
 };
@@ -79,6 +81,11 @@ struct rtl9300_i2c {
 #define RTL9300_I2C_MST_DATA_WORD3			0x14
 #define RTL9300_I2C_MST_GLB_CTRL			0x384
 
+#define RTL9310_I2C_MST_IF_CTRL				0x1004
+#define	RTL9310_I2C_MST_IF_SEL				0x1008
+#define	RTL9310_I2C_MST_CTRL				0x0
+#define	RTL9310_I2C_MST_MEMADDR_CTRL			0x4
+#define RTL9310_I2C_MST_DATA_CTRL			0x8
 
 static int rtl9300_i2c_reg_addr_set(struct rtl9300_i2c *i2c, u32 reg, u16 len)
 {
@@ -96,6 +103,11 @@ static int rtl9300_i2c_select_scl(struct rtl9300_i2c *i2c, u8 scl)
 	return regmap_field_write(i2c->fields[F_SCL_SEL], 1);
 }
 
+static int rtl9310_i2c_select_scl(struct rtl9300_i2c *i2c, u8 scl)
+{
+	return regmap_field_update_bits(i2c->fields[F_SCL_SEL], BIT(scl), BIT(scl));
+}
+
 static int rtl9300_i2c_config_io(struct rtl9300_i2c *i2c, u8 sda_pin)
 {
 	struct rtl9300_i2c_drv_data *drv_data;
@@ -111,7 +123,7 @@ static int rtl9300_i2c_config_io(struct rtl9300_i2c *i2c, u8 sda_pin)
 	if (ret)
 		return ret;
 
-	return drv_data->select_scl(i2c, 0);
+	return drv_data->select_scl(i2c, i2c->scl_num);
 }
 
 static int rtl9300_i2c_config_xfer(struct rtl9300_i2c *i2c, struct rtl9300_i2c_chan *chan,
@@ -346,6 +358,7 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rtl9300_i2c *i2c;
 	u32 clock_freq, sda_pin;
+	u8 mst_id;
 	int ret, i = 0;
 	struct fwnode_handle *child;
 	struct rtl9300_i2c_drv_data *drv_data;
@@ -366,6 +379,11 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = device_property_read_u8(dev, "realtek,mst-id", &mst_id);
+	if (ret || mst_id != 2)
+		mst_id = 1;
+	i2c->scl_num = mst_id - 1;
+
 	platform_set_drvdata(pdev, i2c);
 
 	drv_data = (struct rtl9300_i2c_drv_data *)device_get_match_data(i2c->dev);
@@ -457,12 +475,35 @@ static const struct rtl9300_i2c_drv_data rtl9300_i2c_drv_data = {
 	.max_nchan = RTL9300_I2C_MUX_NCHAN,
 };
 
+static const struct rtl9300_i2c_drv_data rtl9310_i2c_drv_data = {
+	.field_desc = {
+		[F_SCL_SEL]		= GLB_REG_FIELD(RTL9310_I2C_MST_IF_SEL, 12, 13),
+		[F_SDA_SEL]		= GLB_REG_FIELD(RTL9310_I2C_MST_IF_SEL, 0, 11),
+		[F_SCL_FREQ]		= MST_REG_FIELD(RTL9310_I2C_MST_CTRL, 30, 31),
+		[F_DEV_ADDR]		= MST_REG_FIELD(RTL9310_I2C_MST_CTRL, 11, 17),
+		[F_SDA_OUT_SEL]		= MST_REG_FIELD(RTL9310_I2C_MST_CTRL, 18, 21),
+		[F_MEM_ADDR_WIDTH]	= MST_REG_FIELD(RTL9310_I2C_MST_CTRL, 9, 10),
+		[F_DATA_WIDTH]		= MST_REG_FIELD(RTL9310_I2C_MST_CTRL, 5, 8),
+		[F_RD_MODE]		= MST_REG_FIELD(RTL9310_I2C_MST_CTRL, 4, 4),
+		[F_RWOP]		= MST_REG_FIELD(RTL9310_I2C_MST_CTRL, 2, 2),
+		[F_I2C_FAIL]		= MST_REG_FIELD(RTL9310_I2C_MST_CTRL, 1, 1),
+		[F_I2C_TRIG]		= MST_REG_FIELD(RTL9310_I2C_MST_CTRL, 0, 0),
+		[F_MEM_ADDR]		= MST_REG_FIELD(RTL9310_I2C_MST_MEMADDR_CTRL, 0, 23),
+	},
+	.select_scl = rtl9310_i2c_select_scl,
+	.data_reg = RTL9310_I2C_MST_DATA_CTRL,
+	.max_nchan = RTL9310_I2C_MUX_NCHAN,
+};
 
 static const struct of_device_id i2c_rtl9300_dt_ids[] = {
 	{ .compatible = "realtek,rtl9301-i2c", .data = (void *) &rtl9300_i2c_drv_data },
 	{ .compatible = "realtek,rtl9302b-i2c", .data = (void *) &rtl9300_i2c_drv_data },
 	{ .compatible = "realtek,rtl9302c-i2c", .data = (void *) &rtl9300_i2c_drv_data },
 	{ .compatible = "realtek,rtl9303-i2c", .data = (void *) &rtl9300_i2c_drv_data },
+	{ .compatible = "realtek,rtl9310-i2c", .data = (void *) &rtl9310_i2c_drv_data },
+	{ .compatible = "realtek,rtl9311-i2c", .data = (void *) &rtl9310_i2c_drv_data },
+	{ .compatible = "realtek,rtl9312-i2c", .data = (void *) &rtl9310_i2c_drv_data },
+	{ .compatible = "realtek,rtl9313-i2c", .data = (void *) &rtl9310_i2c_drv_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
-- 
2.48.1

Re: [PATCH v2 2/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Rob Herring (Arm)]

On Sat, 12 Jul 2025 19:42:54 +0000, Jonas Jelonek wrote:
> Add dt-bindings for RTL9310 series I2C controller.
> 
> Adjust the regex for child-node address to account for the fact that
> RTL9310 supports 12 instead of only 8 SDA lines.
> 
> Add a vendor-specific property to explicitly specify the
> Realtek-internal ID of the defined I2C controller/master. This is
> required, in particular for RTL9310, to describe the correct I2C
> master.
> 
> Add compatibles for known SoC variants RTL9311, RTL9312 and RTL9313.
> 
> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> ---
>  .../bindings/i2c/realtek,rtl9301-i2c.yaml     | 37 +++++++++++++++++--
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.example.dtb: i2c@100c (realtek,rtl9310-i2c): realtek,mst-id: [0, 0, 0, 1] is not of type 'integer'
	from schema $id: http://devicetree.org/schemas/i2c/realtek,rtl9301-i2c.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.example.dtb: i2c@100c (realtek,rtl9310-i2c): realtek,mst-id: size is 32, expected 8
	from schema $id: http://devicetree.org/schemas/i2c/realtek,rtl9301-i2c.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250712194255.7022-3-jelonek.jonas@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 2/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Krzysztof Kozlowski]

On Sat, Jul 12, 2025 at 07:42:54PM +0000, Jonas Jelonek wrote:
>  properties:
>    compatible:
> @@ -23,7 +25,14 @@ properties:
>                - realtek,rtl9302c-i2c
>                - realtek,rtl9303-i2c
>            - const: realtek,rtl9301-i2c
> +      - items:
> +          - enum:
> +              - realtek,rtl9311-i2c
> +              - realtek,rtl9312-i2c
> +              - realtek,rtl9313-i2c
> +          - const: realtek,rtl9310-i2c
>        - const: realtek,rtl9301-i2c
> +      - const: realtek,rtl9310-i2c

So these two are just enum.

>  
>    reg:
>      items:
> @@ -35,8 +44,16 @@ properties:
>    "#size-cells":
>      const: 0
>  
> +  realtek,mst-id:
> +    $ref: /schemas/types.yaml#/definitions/uint8

uint32

> +    description:
> +      Realtek-internal ID of the I2C controller/master. Only required
> +      for RTL9310 series.

Drop last sentence, redundant.

> +    minimum: 1
> +    maximum: 2
> +
>  patternProperties:
> -  '^i2c@[0-7]$':
> +  '^i2c@([0-9]|1[0-1])$':
>      $ref: /schemas/i2c/i2c-controller.yaml
>      unevaluatedProperties: false
>  

As mentioned last time, missing constraints.

How did you solve this:

"you should clearly narrow this per variant"?

See example schema. It has EXACTLY this case.

https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212

You also need to narrow the number of children.


Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Jonas Jelonek]

Hi Krzysztof,


On 14.07.2025 08:00, Krzysztof Kozlowski wrote:
> On Sat, Jul 12, 2025 at 07:42:54PM +0000, Jonas Jelonek wrote:
>>  properties:
>>    compatible:
>> @@ -23,7 +25,14 @@ properties:
>>                - realtek,rtl9302c-i2c
>>                - realtek,rtl9303-i2c
>>            - const: realtek,rtl9301-i2c
>> +      - items:
>> +          - enum:
>> +              - realtek,rtl9311-i2c
>> +              - realtek,rtl9312-i2c
>> +              - realtek,rtl9313-i2c
>> +          - const: realtek,rtl9310-i2c
>>        - const: realtek,rtl9301-i2c
>> +      - const: realtek,rtl9310-i2c
> So these two are just enum.

Could you be more precise on that please? Sadly, I don't get what you're trying
to tell me.
>> +    minimum: 1
>> +    maximum: 2
>> +
>>  patternProperties:
>> -  '^i2c@[0-7]$':
>> +  '^i2c@([0-9]|1[0-1])$':
>>      $ref: /schemas/i2c/i2c-controller.yaml
>>      unevaluatedProperties: false
>>  
> As mentioned last time, missing constraints.
>
> How did you solve this:
>
> "you should clearly narrow this per variant"?
>
> See example schema. It has EXACTLY this case.
>
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212
>
> You also need to narrow the number of children.

I missed that from your previous review by mistake, sorry for that.

I managed to narrow it per variant whether 'realtek,mst-id' is required or not.
But I'm not really able to do the same for the different regex patterns or the
number of children. Although I'm trying to follow various examples,
dt_binding_check just fails not taking the regex patterns into account.

Since you have a lot of expertise on that and I obviously fail to find
documentation that helps me to do that properly, could you give me some hints
on how that has to look? I'd really appreciate this.

>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 2/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Krzysztof Kozlowski]

On 20/07/2025 21:51, Jonas Jelonek wrote:
> Hi Krzysztof,
> 
> 
> On 14.07.2025 08:00, Krzysztof Kozlowski wrote:
>> On Sat, Jul 12, 2025 at 07:42:54PM +0000, Jonas Jelonek wrote:
>>>  properties:
>>>    compatible:
>>> @@ -23,7 +25,14 @@ properties:
>>>                - realtek,rtl9302c-i2c
>>>                - realtek,rtl9303-i2c
>>>            - const: realtek,rtl9301-i2c
>>> +      - items:
>>> +          - enum:
>>> +              - realtek,rtl9311-i2c
>>> +              - realtek,rtl9312-i2c
>>> +              - realtek,rtl9313-i2c
>>> +          - const: realtek,rtl9310-i2c
>>>        - const: realtek,rtl9301-i2c
>>> +      - const: realtek,rtl9310-i2c
>> So these two are just enum.
> 
> Could you be more precise on that please? Sadly, I don't get what you're trying
> to tell me.


These two last lines should be part of one enum.

>>> +    minimum: 1
>>> +    maximum: 2
>>> +
>>>  patternProperties:
>>> -  '^i2c@[0-7]$':
>>> +  '^i2c@([0-9]|1[0-1])$':
>>>      $ref: /schemas/i2c/i2c-controller.yaml
>>>      unevaluatedProperties: false
>>>  
>> As mentioned last time, missing constraints.
>>
>> How did you solve this:
>>
>> "you should clearly narrow this per variant"?
>>
>> See example schema. It has EXACTLY this case.
>>
>> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212
>>
>> You also need to narrow the number of children.
> 
> I missed that from your previous review by mistake, sorry for that.
> 
> I managed to narrow it per variant whether 'realtek,mst-id' is required or not.
> But I'm not really able to do the same for the different regex patterns or the
> number of children. Although I'm trying to follow various examples,
> dt_binding_check just fails not taking the regex patterns into account.
> 
> Since you have a lot of expertise on that and I obviously fail to find
> documentation that helps me to do that properly, could you give me some hints
> on how that has to look? I'd really appreciate this.


So in your if:then: block where you narrow mst-id, you add on same level
as properties:

patternProperties:
  YOUR_REGEX: false


Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Jonas Jelonek]

Hi Krzysztoof,

On 21.07.2025 09:02, Krzysztof Kozlowski wrote:
>>>> +    minimum: 1
>>>> +    maximum: 2
>>>> +
>>>>  patternProperties:
>>>> -  '^i2c@[0-7]$':
>>>> +  '^i2c@([0-9]|1[0-1])$':
>>>>      $ref: /schemas/i2c/i2c-controller.yaml
>>>>      unevaluatedProperties: false
>>>>  
>>> As mentioned last time, missing constraints.
>>>
>>> How did you solve this:
>>>
>>> "you should clearly narrow this per variant"?
>>>
>>> See example schema. It has EXACTLY this case.
>>>
>>> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212
>>>
>>> You also need to narrow the number of children.
>> I missed that from your previous review by mistake, sorry for that.
>>
>> I managed to narrow it per variant whether 'realtek,mst-id' is required or not.
>> But I'm not really able to do the same for the different regex patterns or the
>> number of children. Although I'm trying to follow various examples,
>> dt_binding_check just fails not taking the regex patterns into account.
>>
>> Since you have a lot of expertise on that and I obviously fail to find
>> documentation that helps me to do that properly, could you give me some hints
>> on how that has to look? I'd really appreciate this.
>
> So in your if:then: block where you narrow mst-id, you add on same level
> as properties:
>
> patternProperties:
>   YOUR_REGEX: false

How I thought of narrowing that in the first place was to make mst-id required
for RTL9310 but optional for RTL9300. In terms of describing the hardware, this
is valid for RTL9300 too (but there's no need for the driver or anything else to
know that).

But I don't mind if you'd rather have it only defined in the 'then' block, or
just disallowed for RTL9300, effectively forbidding the usage for RTL9300.

Either way, it seems I'm still doing it wrong with the regex. Adding as you
suggested:

if:
    properties:
        compatible:
            contains:
                const: realtek,rtl9301-i2c
then:
    patternProperties:
        '^i2c@([0-9]|1[0-1])$': false

breaks validation of the RTL9300 example. Probably I don't see how this
is expected to look like in a working state.

Doing it like the following, very verbose and redundant but at least obvious to
me what it should achieve:

if:
    ...
then:
    patternProperties:
        '^i2c@([0-9]|1[0-1])$':
            ...
else:
    patternProperties:
        '^i2c@[0-7]$':
            ....

breaks validation of both example. To be honest I don't know why.

>
> Best regards,
> Krzysztof

Best regards,
Jonas Jelonek

Re: [PATCH v2 2/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Krzysztof Kozlowski]

On 22/07/2025 20:25, Jonas Jelonek wrote:
>>> Since you have a lot of expertise on that and I obviously fail to find
>>> documentation that helps me to do that properly, could you give me some hints
>>> on how that has to look? I'd really appreciate this.
>>
>> So in your if:then: block where you narrow mst-id, you add on same level
>> as properties:
>>
>> patternProperties:
>>   YOUR_REGEX: false
> 
> How I thought of narrowing that in the first place was to make mst-id required
> for RTL9310 but optional for RTL9300. In terms of describing the hardware, this
> is valid for RTL9300 too (but there's no need for the driver or anything else to
> know that).
> 
> But I don't mind if you'd rather have it only defined in the 'then' block, or
> just disallowed for RTL9300, effectively forbidding the usage for RTL9300.
> 
> Either way, it seems I'm still doing it wrong with the regex. Adding as you
> suggested:
> 
> if:
>     properties:
>         compatible:
>             contains:
>                 const: realtek,rtl9301-i2c
> then:
>     patternProperties:
>         '^i2c@([0-9]|1[0-1])$': false
> 
> breaks validation of the RTL9300 example. Probably I don't see how this
> is expected to look like in a working state.

RTL9300 has 8 controllers, so why are you disallowing them? We talk here
only about new stuff. Why would you change EXISTING behavior when adding
something new?

You need pattern matching redundant children for existing device.


Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Jonas Jelonek]

Hi Kryzysztof,

On 23.07.2025 08:22, Krzysztof Kozlowski wrote:
> On 22/07/2025 20:25, Jonas Jelonek wrote:
>>>> Since you have a lot of expertise on that and I obviously fail to find
>>>> documentation that helps me to do that properly, could you give me some hints
>>>> on how that has to look? I'd really appreciate this.
>>> So in your if:then: block where you narrow mst-id, you add on same level
>>> as properties:
>>>
>>> patternProperties:
>>>   YOUR_REGEX: false
>> How I thought of narrowing that in the first place was to make mst-id required
>> for RTL9310 but optional for RTL9300. In terms of describing the hardware, this
>> is valid for RTL9300 too (but there's no need for the driver or anything else to
>> know that).
>>
>> But I don't mind if you'd rather have it only defined in the 'then' block, or
>> just disallowed for RTL9300, effectively forbidding the usage for RTL9300.
>>
>> Either way, it seems I'm still doing it wrong with the regex. Adding as you
>> suggested:
>>
>> if:
>>     properties:
>>         compatible:
>>             contains:
>>                 const: realtek,rtl9301-i2c
>> then:
>>     patternProperties:
>>         '^i2c@([0-9]|1[0-1])$': false
>>
>> breaks validation of the RTL9300 example. Probably I don't see how this
>> is expected to look like in a working state.
> RTL9300 has 8 controllers, so why are you disallowing them? We talk here
> only about new stuff. Why would you change EXISTING behavior when adding
> something new?
>
> You need pattern matching redundant children for existing device.
>

It wasn't my intention to disallow that, but I'm struggling with these specifics
of the dt-bindings to accomplish what is requested. Anyway, I think your comment
gave me a good hint to solve that now.

if:
    properties:
        compatible:
            contains:
                const: realtek,rtl9301-i2c
then:
    patternProperties:
        '^i2c@([8-9]|1[0-1])$': false

This seems to do the trick, dt_binding_check is fine with this. Is this how you
intended it to look like?


Talking about 'not changing existing behaviour', is it fine to allow the mst-id
property for the existing RTL9300 too or should that really only be allowed for
what I add here now?

> Best regards,
> Krzysztof

Best regards,
Jonas Jelonek