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

[PATCH 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 commit reworks the driver to use more of the regmap API.
Instead of using macros, all registers are defined as reg_field and most
operations on these registers are performed using regmap_field and the
corresponding API. This allows to add support for further chips quite
easily by providing the correct registers and driver data. Moreover, it
avoids to add new macros and a lot of chip-specific function for every
additional chip support.

The second commit makes use of this by adding support for the RTL9310
series, providing the slightly different register layout and a few
specifics. This also introduces a new device tree property 'scl-num' to
explicitly specify the hardware instance in the device tree. This is
needed because RTL9310 does SCL selection/activation in a global
register. In contrast, on RTL9300 this is done in a master-specific
register and thus is already handled by the reg_base of the controller.
At this point I'm still open to any suggestions if and how this could be
done better to avoid a new dt property.
Both has been tested successfully on RTL9302B-based Zyxel XGS1210-12
and RTL9313-based Netgear MS510TXM.

The third commit adds proper documentation to the device tree bindings
for the RTL9310 support.

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

In case I messed something up, please give a hint and I'll fix it.

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

 .../bindings/i2c/realtek,rtl9301-i2c.yaml     |  33 ++-
 drivers/i2c/busses/i2c-rtl9300.c              | 227 +++++++++++++-----
 2 files changed, 190 insertions(+), 70 deletions(-)

-- 
2.48.1

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

[Jonas Jelonek]

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

Since this patch is done in preparation to add support for RTL9310, one
operation is handled using separate chip-specific functions and
references to them. This operation cannot be handled with the reg_field
description only due to different register layout.

Overall, this makes 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.

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 2/3] i2c: add RTL9310 support to RTL9300 I2C controller driver

[Jonas Jelonek]

This adds support for the internal I2C controllers of RTL9310 series
based chip to the driver for RTL9300. Adds driver data with register
definitions, a chip-specific function and a generic compatible string
for the whole RTL9310 series because as of now there are no differences
between the different variants (RTL9311, RTL9312, RTL9313).

Also adds a new device tree property 'scl-num' which needs to be
specified in case both or only the second master is used. This is
required due to the register layout 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 | 39 +++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index f8e81102ee74..fae6eb39adfd 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,
@@ -366,6 +378,10 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = device_property_read_u8(dev, "scl-num", &i2c->scl_num);
+	if (ret || (i2c->scl_num != 0 && i2c->scl_num != 1))
+		i2c->scl_num = 0;
+
 	platform_set_drvdata(pdev, i2c);
 
 	drv_data = (struct rtl9300_i2c_drv_data *)device_get_match_data(i2c->dev);
@@ -457,12 +473,33 @@ 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 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
-- 
2.48.1

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

[Jonas Jelonek]

This extends the dt-bindings for the I2C driver for RTL9300 to account
for the added support for RTL9310 series.

A new property is added to explicitly set the SCL num/hardware instance
of the controller that is used. In contrast to RTL9300 the driver needs
to know that explicitly for RTL9310 because the SCL selection is now in
a global register instead of a master-specific register.

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

A single generic compatible "realtek,rtl9310-i2c" is added. To best
knowledge, all existing SoCs of RTL9310 series (RTL9311, RTL9312,
RTL9313) have equal I2C capabilities thus don't need special treatment.
However, in the unlikely case of future differences with specific
SoCs within this series, more can be added as needed.

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

diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
index eddfd329c67b..3b32da3de2af 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:
@@ -24,6 +26,7 @@ properties:
               - realtek,rtl9303-i2c
           - const: realtek,rtl9301-i2c
       - const: realtek,rtl9301-i2c
+      - const: realtek,rtl9310-i2c
 
   reg:
     description: Register offset and size this I2C controller.
@@ -34,8 +37,18 @@ properties:
   "#size-cells":
     const: 0
 
+  scl-num:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      SCL number (0 or 1) to select I2C hardware instance. SCL0
+      is hardwired to master 1 and SCL1 to master 2.
+      Required only on RTL9310 if both or only second master
+      is used.
+    minimum: 0
+    maximum: 1
+
 patternProperties:
-  '^i2c@[0-7]$':
+  '^i2c@([0-9]|1[0-1])$':
     $ref: /schemas/i2c/i2c-controller.yaml
     unevaluatedProperties: false
 
@@ -67,3 +80,15 @@ examples:
         #size-cells = <0>;
       };
     };
+  - |
+    i2c@100c {
+      compatible = "realtek,rtl9310-i2c";
+      reg = <0x100c 0x18>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+      scl-num = <0>;
+
+      i2c@0 {
+        reg = <0>;
+      };
+    };
-- 
2.48.1

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

[Krzysztof Kozlowski]

On 01/07/2025 11:17, Jonas Jelonek wrote:
> This extends the dt-bindings for the I2C driver for RTL9300 to account

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> for the added support for RTL9310 series.
> 
> A new property is added to explicitly set the SCL num/hardware instance
> of the controller that is used. In contrast to RTL9300 the driver needs
> to know that explicitly for RTL9310 because the SCL selection is now in
> a global register instead of a master-specific register.
> 
> The regex for child-node address is adjusted to account for the fact
> that RTL9310 supports 12 instead of 8 SDA lines.
> 
> A single generic compatible "realtek,rtl9310-i2c" is added. To best
> knowledge, all existing SoCs of RTL9310 series (RTL9311, RTL9312,
> RTL9313) have equal I2C capabilities thus don't need special treatment.


You always need specific front compatible (and fallback if applicable).



> However, in the unlikely case of future differences with specific
> SoCs within this series, more can be added as needed.
> 
> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> ---
>  .../bindings/i2c/realtek,rtl9301-i2c.yaml     | 33 ++++++++++++++++---
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
> index eddfd329c67b..3b32da3de2af 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:
> @@ -24,6 +26,7 @@ properties:
>                - realtek,rtl9303-i2c
>            - const: realtek,rtl9301-i2c
>        - const: realtek,rtl9301-i2c
> +      - const: realtek,rtl9310-i2c
>  
>    reg:
>      description: Register offset and size this I2C controller.
> @@ -34,8 +37,18 @@ properties:
>    "#size-cells":
>      const: 0
>  
> +  scl-num:


No, you do not get own instance IDs.

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 01/07/2025 11:17, Jonas Jelonek wrote:
> This extends the dt-bindings for the I2C driver for RTL9300 to account
> for the added support for RTL9310 series.

There is no driver here in the patch, so message should not reference it
at all. Describe the hardware instead.

Also, bindings come before the user (see submitting patches in DT dir).

Best regards,
Krzysztof

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

[Jonas Jelonek]

Hi Krzysztof,


On 01.07.2025 13:33, Krzysztof Kozlowski wrote:
> On 01/07/2025 11:17, Jonas Jelonek wrote:
>> This extends the dt-bindings for the I2C driver for RTL9300 to account
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
I'll fix this in v2.
>> for the added support for RTL9310 series.
>>
>> A new property is added to explicitly set the SCL num/hardware instance
>> of the controller that is used. In contrast to RTL9300 the driver needs
>> to know that explicitly for RTL9310 because the SCL selection is now in
>> a global register instead of a master-specific register.
>>
>> The regex for child-node address is adjusted to account for the fact
>> that RTL9310 supports 12 instead of 8 SDA lines.
>>
>> A single generic compatible "realtek,rtl9310-i2c" is added. To best
>> knowledge, all existing SoCs of RTL9310 series (RTL9311, RTL9312,
>> RTL9313) have equal I2C capabilities thus don't need special treatment.
>
> You always need specific front compatible (and fallback if applicable).
>
Since I only have RTL9313 variant in my device, I'd be able to add
'realtek,rtl9313-i2c' as a verified compatible. For others, I do not 
have a list
of which variants actually exist, if there are more variants than just 
RTL9311,
RTL9312 and RTL9313. Should I add compatibles for those anyway or just for
that variant I have?

'realtek,rtl9301-i2c' seems to be such a fallback for RTL9300, should 
the one
for RTL9310 be 'realtek,rtl9311-i2c' or is 'realtek,rtl9310-i2c' fine?
Just asking because this isn't obvious to me right now.
>
>> However, in the unlikely case of future differences with specific
>> SoCs within this series, more can be added as needed.
>>
>> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
>> ---
>>   .../bindings/i2c/realtek,rtl9301-i2c.yaml     | 33 ++++++++++++++++---
>>   1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
>> index eddfd329c67b..3b32da3de2af 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:
>> @@ -24,6 +26,7 @@ properties:
>>                 - realtek,rtl9303-i2c
>>             - const: realtek,rtl9301-i2c
>>         - const: realtek,rtl9301-i2c
>> +      - const: realtek,rtl9310-i2c
>>   
>>     reg:
>>       description: Register offset and size this I2C controller.
>> @@ -34,8 +37,18 @@ properties:
>>     "#size-cells":
>>       const: 0
>>   
>> +  scl-num:
>
> No, you do not get own instance IDs.
Is that meant for the wording/naming of the property and/or its
description or for the general idea of this property?
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.
>
> Please kindly resend and include all necessary To/Cc entries.
> </form letter>
>
> Best regards,
> Krzysztof

Best regards,
Jonas

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

[Jonas Jelonek]

On 01.07.2025 13:35, Krzysztof Kozlowski wrote:
> On 01/07/2025 11:17, Jonas Jelonek wrote:
>> This extends the dt-bindings for the I2C driver for RTL9300 to account
>> for the added support for RTL9310 series.
> There is no driver here in the patch, so message should not reference it
> at all. Describe the hardware instead.
>
> Also, bindings come before the user (see submitting patches in DT dir).
>
> Best regards,
> Krzysztof
I'll fix this too in v2.

Best regards,
Jonas

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

[Krzysztof Kozlowski]

On 01/07/2025 14:34, Jonas Jelonek wrote:
> Hi Krzysztof,
> 
> 
> On 01.07.2025 13:33, Krzysztof Kozlowski wrote:
>> On 01/07/2025 11:17, Jonas Jelonek wrote:
>>> This extends the dt-bindings for the I2C driver for RTL9300 to account
>> Please do not use "This commit/patch/change", but imperative mood. See
>> longer explanation here:
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> I'll fix this in v2.
>>> for the added support for RTL9310 series.
>>>
>>> A new property is added to explicitly set the SCL num/hardware instance
>>> of the controller that is used. In contrast to RTL9300 the driver needs
>>> to know that explicitly for RTL9310 because the SCL selection is now in
>>> a global register instead of a master-specific register.
>>>
>>> The regex for child-node address is adjusted to account for the fact
>>> that RTL9310 supports 12 instead of 8 SDA lines.
>>>
>>> A single generic compatible "realtek,rtl9310-i2c" is added. To best
>>> knowledge, all existing SoCs of RTL9310 series (RTL9311, RTL9312,
>>> RTL9313) have equal I2C capabilities thus don't need special treatment.
>>
>> You always need specific front compatible (and fallback if applicable).
>>
> Since I only have RTL9313 variant in my device, I'd be able to add
> 'realtek,rtl9313-i2c' as a verified compatible. For others, I do not 
> have a list
> of which variants actually exist, if there are more variants than just 
> RTL9311,
> RTL9312 and RTL9313. Should I add compatibles for those anyway or just for
> that variant I have?


You have some very odd wrapping of emails.

Anyway, you keep mentioning in multiple places rtl9311-9313, so that's
confusing. If you mention them, I would expect compatibles. They cannot
use rtl9310 compatible alone.

I don't mind skipping them, but then just don't mention any sort of
treatment for other devices. You add this and only this hardware, if you
do not want to follow the make-binding-complete principle (see writing
bindings).

> 

> 'realtek,rtl9301-i2c' seems to be such a fallback for RTL9300, should 
> the one
> for RTL9310 be 'realtek,rtl9311-i2c' or is 'realtek,rtl9310-i2c' fine?
> Just asking because this isn't obvious to me right now.
>>
>>> However, in the unlikely case of future differences with specific
>>> SoCs within this series, more can be added as needed.
>>>
>>> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
>>> ---
>>>   .../bindings/i2c/realtek,rtl9301-i2c.yaml     | 33 ++++++++++++++++---
>>>   1 file changed, 29 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9301-i2c.yaml
>>> index eddfd329c67b..3b32da3de2af 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:
>>> @@ -24,6 +26,7 @@ properties:
>>>                 - realtek,rtl9303-i2c
>>>             - const: realtek,rtl9301-i2c
>>>         - const: realtek,rtl9301-i2c
>>> +      - const: realtek,rtl9310-i2c
>>>   
>>>     reg:
>>>       description: Register offset and size this I2C controller.
>>> @@ -34,8 +37,18 @@ properties:
>>>     "#size-cells":
>>>       const: 0
>>>   
>>> +  scl-num:
>>
>> No, you do not get own instance IDs.
> Is that meant for the wording/naming of the property and/or its
> description or for the general idea of this property?

You do not get such property. We don't accept it, it's generic rule.
Nowhere in the kernel... unless this is a standard, generic property
(there is no vendor prefix), but I could not find it. If it is standard
property, where is it defined in dtschema or common bindings?

I don't get the need for this property and description does not help, so
just drop it.


Best regards,
Krzysztof

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

[Jonas Jelonek]

On 01.07.2025 15:17, Krzysztof Kozlowski wrote:
> You have some very odd wrapping of emails.

Sorry for that, I'm not used to this workflow and obviously haven't configured
my editor properly. I'm working on that.

> Anyway, you keep mentioning in multiple places rtl9311-9313, so that's
> confusing. If you mention them, I would expect compatibles. They cannot
> use rtl9310 compatible alone.
>
> I don't mind skipping them, but then just don't mention any sort of
> treatment for other devices. You add this and only this hardware, if you
> do not want to follow the make-binding-complete principle (see writing
> bindings).
>

I think I got. I'll adjust this in v2.
>>>> +  scl-num:
>>> No, you do not get own instance IDs.
>> Is that meant for the wording/naming of the property and/or its
>> description or for the general idea of this property?
> You do not get such property. We don't accept it, it's generic rule.
> Nowhere in the kernel... unless this is a standard, generic property
> (there is no vendor prefix), but I could not find it. If it is standard
> property, where is it defined in dtschema or common bindings?
>
> I don't get the need for this property and description does not help, so
> just drop it.

Ok, I missed that this is seen as a generic rule then. Sorry for that.

For the purpose:
RTL9310 changed the register layout compared to RTL9300. Activating
the SCL line is done by setting bit 12 for master 1 and bit 13 for master 2
in a global register which is located before the master-specific registers.
Thus, the driver needs to know which master (1 or 2) it is currently doing
something for. That is what this property is intended to be used, naming
to-be-discussed.

Alternatively, this could be derived from the reg property if that is
fail-safe enough. The value of the reg property has to match anyway
to make the whole driver work properly.

>
> Best regards,
> Krzysztof

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

[markus.stockhausen]

Hi Jonas,

> +	ret = device_property_read_u8(dev, "scl-num", &i2c->scl_num);
> +	if (ret || (i2c->scl_num != 0 && i2c->scl_num != 1))
> +		i2c->scl_num = 0;
> +

if (ret || i2c->scl_num != 1)
    i2c->scl_num = 0;

should be enough.

Markus

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

[Chris Packham]

Hi Jonas,

On 01/07/2025 21:17, Jonas Jelonek wrote:
> This reworks 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.
>
> Since this patch is done in preparation to add support for RTL9310, one
> operation is handled using separate chip-specific functions and
> references to them. This operation cannot be handled with the reg_field
> description only due to different register layout.
>
> Overall, this makes 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.
>
> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz> # On RTL9302c

> ---
>   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);

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

[Krzysztof Kozlowski]

On 01/07/2025 16:31, Jonas Jelonek wrote:
> 
> 
> On 01.07.2025 15:17, Krzysztof Kozlowski wrote:
>> You have some very odd wrapping of emails.
> 
> Sorry for that, I'm not used to this workflow and obviously haven't configured
> my editor properly. I'm working on that.
> 
>> Anyway, you keep mentioning in multiple places rtl9311-9313, so that's
>> confusing. If you mention them, I would expect compatibles. They cannot
>> use rtl9310 compatible alone.
>>
>> I don't mind skipping them, but then just don't mention any sort of
>> treatment for other devices. You add this and only this hardware, if you
>> do not want to follow the make-binding-complete principle (see writing
>> bindings).
>>
> 
> I think I got. I'll adjust this in v2.
>>>>> +  scl-num:
>>>> No, you do not get own instance IDs.
>>> Is that meant for the wording/naming of the property and/or its
>>> description or for the general idea of this property?
>> You do not get such property. We don't accept it, it's generic rule.
>> Nowhere in the kernel... unless this is a standard, generic property
>> (there is no vendor prefix), but I could not find it. If it is standard
>> property, where is it defined in dtschema or common bindings?
>>
>> I don't get the need for this property and description does not help, so
>> just drop it.
> 
> Ok, I missed that this is seen as a generic rule then. Sorry for that.
> 
> For the purpose:
> RTL9310 changed the register layout compared to RTL9300. Activating
> the SCL line is done by setting bit 12 for master 1 and bit 13 for master 2
> in a global register which is located before the master-specific registers.
> Thus, the driver needs to know which master (1 or 2) it is currently doing
> something for. That is what this property is intended to be used, naming
> to-be-discussed.

There is no global register space here and no syscon, so I don't
understand how can you access it, especially when they are located
BEFORE your address space.

Best regards,
Krzysztof

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

[Jonas Jelonek]

Hi Krzysztof,

thanks for taking the time to deal with this.

On 02.07.2025 08:11, Krzysztof Kozlowski wrote:
> On 01/07/2025 16:31, Jonas Jelonek wrote:
>> For the purpose:
>> RTL9310 changed the register layout compared to RTL9300. Activating
>> the SCL line is done by setting bit 12 for master 1 and bit 13 for master 2
>> in a global register which is located before the master-specific registers.
>> Thus, the driver needs to know which master (1 or 2) it is currently doing
>> something for. That is what this property is intended to be used, naming
>> to-be-discussed.
> There is no global register space here and no syscon, so I don't
> understand how can you access it, especially when they are located
> BEFORE your address space.

Probably this explanation is still missing some background and 'global'
was misleading here. The I2C controllers are part of Realtek switchcore.
This is defined as a syscon in DTS and the I2C controller has to be
defined as a child-node of it. (see 
https://elixir.bootlin.com/linux/v6.15.1/source/arch/mips/boot/dts/realtek/rtl930x.dtsi#L45-L72)
The driver takes its regmap from this syscon node as the I2C registers
are within that switchcore address space.

Address layout in RTL9310 is (addresses relative to parent syscon node):
0x1004 - 0x100b    I2C 'global' registers
0x100c - 0x1023    I2C master 1 registers
0x1024 - 0x103b    I2C master 2 registers

The driver has to access both the registers for the master that it is
configuring AND the 'global' ones (for SCL + SDA activation).

For upstream RTL9300 it's similar with the difference of having SCL
selection in the master-specific registers and the register layout
having the order:
- I2C master 1 registers
- I2C 'global' register
- I2C master 2 registers

> Best regards,
> Krzysztof

Just an idea to discuss regarding the DT compatibles:
I was proposed out of this conversation to use something like
'i2c-otto-mango' ('otto' being the codename for their platform,
'mango' being the codename for the RTL9310 series). Based on, that
technically(!) it doesn't make sense to distinguish between the
variants of the series.

Has this any chance of acceptance, even when I do not mention
specific SoC variants anywhere in my patchset?

Best regards,
Jonas

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

[Krzysztof Kozlowski]

On 02/07/2025 09:34, Jonas Jelonek wrote:
> Hi Krzysztof,
> 
> thanks for taking the time to deal with this.
> 
> On 02.07.2025 08:11, Krzysztof Kozlowski wrote:
>> On 01/07/2025 16:31, Jonas Jelonek wrote:
>>> For the purpose:
>>> RTL9310 changed the register layout compared to RTL9300. Activating
>>> the SCL line is done by setting bit 12 for master 1 and bit 13 for master 2
>>> in a global register which is located before the master-specific registers.
>>> Thus, the driver needs to know which master (1 or 2) it is currently doing
>>> something for. That is what this property is intended to be used, naming
>>> to-be-discussed.
>> There is no global register space here and no syscon, so I don't
>> understand how can you access it, especially when they are located
>> BEFORE your address space.
> 
> Probably this explanation is still missing some background and 'global'
> was misleading here. The I2C controllers are part of Realtek switchcore.
> This is defined as a syscon in DTS and the I2C controller has to be
> defined as a child-node of it. (see 
> https://elixir.bootlin.com/linux/v6.15.1/source/arch/mips/boot/dts/realtek/rtl930x.dtsi#L45-L72)
> The driver takes its regmap from this syscon node as the I2C registers
> are within that switchcore address space.
> 
> Address layout in RTL9310 is (addresses relative to parent syscon node):
> 0x1004 - 0x100b    I2C 'global' registers
> 0x100c - 0x1023    I2C master 1 registers
> 0x1024 - 0x103b    I2C master 2 registers
> 
> The driver has to access both the registers for the master that it is
> configuring AND the 'global' ones (for SCL + SDA activation).
> 
> For upstream RTL9300 it's similar with the difference of having SCL
> selection in the master-specific registers and the register layout
> having the order:
> - I2C master 1 registers
> - I2C 'global' register
> - I2C master 2 registers

I think I will keep bookmark this talk because this is perfect example
of writing bindings rule: they supposed to be complete.

If people sent complete bindings, they would see that you are now in
tricky spot and this maybe has to be redone to standard approach - I2C
is not a child of syscon block, but separate device. When it is a
separate device we solve it (plenty of examples) with phandle to syscon
with offset or value argument.

But no! Some incomplete hardware description was sent, stuffing
everything into syscon and claiming that everything is child of syscon,
and now you are stuck with this:

system controller
        |
        |
   i2c-controller-for-multiple-SDA
           |
           |
       i2c-controllers-for-each-SDA

This is not only just confusing but maybe even not correct.

I understand that either i2c controller can take any SCL line. If so,
that could be a pinctrl, but again this is child of that device, so
pinctrl to parent would be odd.

Vendor (not generic) property seems the only solution, but then this
should not be part of the existing binding or you should clearly narrow
this per variant. It was made very clear that rtl9301-family has only
one SCL per controller and you cannot choose.

> 
>> Best regards,
>> Krzysztof
> 
> Just an idea to discuss regarding the DT compatibles:
> I was proposed out of this conversation to use something like
> 'i2c-otto-mango' ('otto' being the codename for their platform,
> 'mango' being the codename for the RTL9310 series). Based on, that
> technically(!) it doesn't make sense to distinguish between the
> variants of the series.

I don't understand what does it refer to, but anyway, soc compatibles
are the only preferred way.


Best regards,
Krzysztof

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

[Jonas Jelonek]

(sorry for double sending, previous one had HTML)

Hi Krzysztof,

On 02.07.2025 09:49, Krzysztof Kozlowski wrote:
> If people sent complete bindings, they would see that you are now in
> tricky spot and this maybe has to be redone to standard approach - I2C
> is not a child of syscon block, but separate device. When it is a
> separate device we solve it (plenty of examples) with phandle to syscon
> with offset or value argument.

Was this assumption probably made based on that the I2C peripherals are
controlled via mapped registers within the address space that is
designated to that 'switchcore'/syscon node?

If using a phandle instead solves this, I'd be fine. Moving the I2C
controller out of the syscon shouldn't be an issue. Is changing this
behavior fine with changing the dt-bindings then? IIRC there's a rule
that dt-bindings must not be changed once they are published.

>
> But no! Some incomplete hardware description was sent, stuffing
> everything into syscon and claiming that everything is child of syscon,
> and now you are stuck with this:
>
> system controller
>         |
>         |
>    i2c-controller-for-multiple-SDA
>            |
>            |
>        i2c-controllers-for-each-SDA
>
> This is not only just confusing but maybe even not correct.

At least I'm confused already.

To make sure we're talking about the same hardware architecture:
RTL93xx have two I2C controllers each having a hardwired SCL and being
able to use any of the 8/12 SDA lines.

As far as I understood, the 'i2c-controller-for-each-SDA' you
mention is meant to handle the muxing behavior (being able to use each
SDA on either controller). Being more like a channel than a dedicated
controller.

In comparison, downstream in OpenWrt there's currently a separate
driver i2c-rtl9300-mux. The child nodes belong to the Mux node there
instead of to the I2C controller. The mux node has a reference to the
I2C controller it uses.

> I understand that either i2c controller can take any SCL line. If so,
> that could be a pinctrl, but again this is child of that device, so
> pinctrl to parent would be odd.

Either I2C controller can take any SDA line, SCL line is fixed.
I see that adding the property implies the opposite, so I agree
if this is not acceptable.

>
> Vendor (not generic) property seems the only solution, but then this
> should not be part of the existing binding or you should clearly narrow
> this per variant. 

Based on the previous comment, I could completely omit this property
and somehow hardcode it in the driver based on e.g. the address of the
I2C controller. But I need advice in this case. Either use a property
to explicitly mention this in the device tree or infer it from an
already used property. 

> It was made very clear that rtl9301-family has only
> one SCL per controller and you cannot choose.

This hasn't changed in RTL9310 family.

And to clarify since this is already confusing for me in how the
compatibles are named: there is no rtl9301-family, RTL9301X SoCs (among
RTL9302X and RTL9303X) are part of the RTL9300 / longan family.Similarly, RTL9311X, RTL9312X and RTL9313X SoCs are part of the RTL9310 / mango family.
> Best regards,
> Krzysztof

Best regards,
Jonas