[PATCH 0/5] i2c: RTL9300 support

[PATCH 0/5] i2c: RTL9300 support

[Chris Packham]

This builds on top of my in-flight series that adds the syscon node for the
switch block[1]. The I2C controllers are part of that block of registers. The
controller driver is adapted from openwrt, the multiplexing support is added by
me and differs from the openwrt implementation.

[1] - https://lore.kernel.org/lkml/20240913024948.1317786-1-chris.packham@alliedtelesis.co.nz/

Chris Packham (5):
  dt-bindings: i2c: Add RTL9300 I2C controller
  i2c: Add driver for the RTL9300 I2C controller
  mips: dts: realtek: Add I2C controllers
  dt-bindings: i2c: Add RTL9300 I2C multiplexer
  i2c: rtl9300: Add multiplexing support

 .../bindings/i2c/realtek,rtl9300-i2c-mux.yaml |  82 +++
 .../bindings/i2c/realtek,rtl9300-i2c.yaml     |  73 +++
 MAINTAINERS                                   |   8 +
 arch/mips/boot/dts/realtek/rtl930x.dtsi       |  18 +
 drivers/i2c/busses/Kconfig                    |  10 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-rtl9300.c              | 543 ++++++++++++++++++
 7 files changed, 735 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
 create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-rtl9300.c

-- 
2.46.1

[PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller

[Chris Packham]

Add dtschema for the I2C controller on the RTL9300 SoC. The I2C
controllers on this SoC are part of the "switch" block which is
represented here as a syscon node. The SCL pins are dependent on the I2C
controller (GPIO8 for the first controller, GPIO 17 for the second). The
SDA pins can be assigned to either one of the I2C controllers (but not
both).

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    This does hit generate the following dt_binding_check warning
    
    realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
    
    Which is totally correct. I haven't given this thing a reg property
    because I'm using an offset from the parent syscon node. I'm also not
    calling the first offset "offset" but I don't think that'd help.
    
    I looked at a couple of other examples of devices that are children of
    syscon nodes (e.g. armada-ap806-thermal, ap806-cpu-clock) these do have
    a reg property in the dts but as far as I can see from the code it's not
    actually used, instead the register offsets are in the code looked up
    from the driver data (in at least one-case the reg offset is for a
    legacy usage).
    
    So I'm a little unsure what to do here. I can add a reg property and
    update the driver to use that to get the offset for the first set of
    registers (or just not use it). Or I could drop the @36c from the node
    names but then I coudn't distinguish the two controllers without failing
    the $nodename: requirement from i2c-controller.yaml.

 .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 73 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
new file mode 100644
index 000000000000..5b74a1986720
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek RTL I2C Controller
+
+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.
+
+properties:
+  compatible:
+    const: realtek,rtl9300-i2c
+
+  realtek,control-offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Offset of the registers for this I2C controller
+
+  realtek,global-control-offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Offset of the I2C global control register (common between
+      controllers).
+
+  clock-frequency:
+    enum: [ 100000, 400000 ]
+
+  realtek,sda-pin:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 7
+    description:
+      SDA pin associated with this I2C controller.
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - realtek,control-offset
+  - realtek,global-control-offset
+
+examples:
+  - |
+    switch@1b000000 {
+      compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
+      reg = <0x1b000000 0x10000>;
+
+      i2c@36c {
+        compatible = "realtek,rtl9300-i2c";
+        realtek,control-offset = <0x36c>;
+        realtek,global-control-offset = <0x384>;
+        clock-frequency = <100000>;
+        realtek,sda-pin = <2>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+      };
+
+      i2c@388 {
+        compatible = "realtek,rtl9300-i2c";
+        realtek,control-offset = <0x388>;
+        realtek,global-control-offset = <0x384>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index f328373463b0..ccb1125444f4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19887,6 +19887,12 @@ S:	Maintained
 T:	git https://github.com/pkshih/rtw.git
 F:	drivers/net/wireless/realtek/rtl8xxxu/
 
+RTL9300 I2C DRIVER (rtl9300-i2c)
+M:	Chris Packham <chris.packham@alliedtelesis.co.nz>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
+
 RTRS TRANSPORT DRIVERS
 M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
 M:	Jack Wang <jinpu.wang@ionos.com>
-- 
2.46.1

[PATCH 2/5] i2c: Add driver for the RTL9300 I2C controller

[Chris Packham]

Add support for the I2C controller on the RTL9300 SoC. This is based on
the openwrt implementation[1] but cleaned up to make use of the regmap
APIs.

[1] - https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-5.15/drivers/i2c/busses/i2c-rtl9300.c

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 MAINTAINERS                      |   1 +
 drivers/i2c/busses/Kconfig       |  10 +
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-rtl9300.c | 377 +++++++++++++++++++++++++++++++
 4 files changed, 389 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-rtl9300.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ccb1125444f4..9e123e9839a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19892,6 +19892,7 @@ M:	Chris Packham <chris.packham@alliedtelesis.co.nz>
 L:	linux-i2c@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
+F:	drivers/i2c/busses/i2c-rtl9300.c
 
 RTRS TRANSPORT DRIVERS
 M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a22f9125322a..927b583002c7 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1041,6 +1041,16 @@ config I2C_RK3X
 	  This driver can also be built as a module. If so, the module will
 	  be called i2c-rk3x.
 
+config I2C_RTL9300
+	tristate "Realtek RTL9300 I2C controller"
+	depends on MACH_REALTEK_RTL || COMPILE_TEST
+	help
+	  Say Y here to include support for the I2C controller in Realtek
+	  RTL9300 SoCs.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called i2c-rtl9300.
+
 config I2C_RZV2M
 	tristate "Renesas RZ/V2M adapter"
 	depends on ARCH_RENESAS || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 78d0561339e5..ac2f9f22803c 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -102,6 +102,7 @@ obj-$(CONFIG_I2C_QCOM_GENI)	+= i2c-qcom-geni.o
 obj-$(CONFIG_I2C_QUP)		+= i2c-qup.o
 obj-$(CONFIG_I2C_RIIC)		+= i2c-riic.o
 obj-$(CONFIG_I2C_RK3X)		+= i2c-rk3x.o
+obj-$(CONFIG_I2C_RTL9300)	+= i2c-rtl9300.o
 obj-$(CONFIG_I2C_RZV2M)		+= i2c-rzv2m.o
 obj-$(CONFIG_I2C_S3C2410)	+= i2c-s3c2410.o
 obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
new file mode 100644
index 000000000000..f16e9b6343bf
--- /dev/null
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -0,0 +1,377 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct rtl9300_i2c {
+	struct regmap *regmap;
+	struct device *dev;
+	struct i2c_adapter adap;
+	u32 i2c_mst_ofs;
+	u32 i2c_mst_glb_ofs;
+	u8 bus_freq;
+	u8 sda_pin;
+};
+
+#define I2C_MST_CTRL1		0x0
+#define  MEM_ADDR_OFS		8
+#define  MEM_ADDR_MASK		0xffffff
+#define  SDA_OUT_SEL_OFS	4
+#define  SDA_OUT_SEL_MASK	0x7
+#define  GPIO_SCL_SEL		BIT(3)
+#define  RWOP			BIT(2)
+#define  I2C_FAIL		BIT(1)
+#define  I2C_TRIG		BIT(0)
+#define I2C_MST_CTRL2		0x4
+#define  RD_MODE		BIT(15)
+#define  DEV_ADDR_OFS		8
+#define  DEV_ADDR_MASK		0x7f
+#define  DATA_WIDTH_OFS		4
+#define  DATA_WIDTH_MASK	0xf
+#define  MEM_ADDR_WIDTH_OFS	2
+#define  MEM_ADDR_WIDTH_MASK	0x3
+#define  SCL_FREQ_OFS		0
+#define  SCL_FREQ_MASK		0x3
+#define I2C_MST_DATA_WORD0	0x8
+#define I2C_MST_DATA_WORD1	0xc
+#define I2C_MST_DATA_WORD2	0x10
+#define I2C_MST_DATA_WORD3	0x14
+
+#define RTL9300_I2C_STD_FREQ		0
+#define RTL9300_I2C_FAST_FREQ		1
+
+DEFINE_MUTEX(i2c_lock);
+
+static int rtl9300_i2c_reg_addr_set(struct rtl9300_i2c *i2c, u32 reg, u16 len)
+{
+	u32 val, mask;
+	int ret;
+
+	val = len << MEM_ADDR_WIDTH_OFS;
+	mask = MEM_ADDR_WIDTH_MASK << MEM_ADDR_WIDTH_OFS;
+
+	ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL2, mask, val);
+	if (ret)
+		return ret;
+
+	val = reg << MEM_ADDR_OFS;
+	mask = MEM_ADDR_MASK << MEM_ADDR_OFS;
+
+	ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, mask, val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rtl9300_i2c_config_io(struct rtl9300_i2c *i2c, u8 sda_pin)
+{
+	int ret;
+
+	ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_glb_ofs, BIT(sda_pin), BIT(sda_pin));
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1,
+				 (SDA_OUT_SEL_MASK << SDA_OUT_SEL_OFS) | GPIO_SCL_SEL,
+				 (sda_pin << SDA_OUT_SEL_OFS) | GPIO_SCL_SEL);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rtl9300_i2c_config_xfer(struct rtl9300_i2c *i2c, u16 addr, u16 len)
+{
+	u32 val, mask;
+
+	val = i2c->bus_freq << SCL_FREQ_OFS;
+	mask = SCL_FREQ_MASK << SCL_FREQ_OFS;
+
+	val |= addr << DEV_ADDR_OFS;
+	mask |= DEV_ADDR_MASK << DEV_ADDR_OFS;
+
+	val |= ((len - 1) & 0xf) << DATA_WIDTH_OFS;
+	mask |= DATA_WIDTH_MASK << DATA_WIDTH_OFS;
+
+	mask |= RD_MODE;
+
+	return regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL2, mask, val);
+}
+
+static int rtl9300_i2c_read(struct rtl9300_i2c *i2c, u8 *buf, int len)
+{
+	u32 vals[4] = {};
+	int i, ret;
+
+	if (len > 16)
+		return -EIO;
+
+	ret = regmap_bulk_read(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0,
+			       vals, ARRAY_SIZE(vals));
+	if (ret)
+		return ret;
+
+	for (i = 0; i < len; i++) {
+		buf[i] = vals[i/4] & 0xff;
+		vals[i/4] >>= 8;
+	}
+
+	return len;
+}
+
+static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len)
+{
+	u32 vals[4] = {};
+	int i, ret;
+
+	if (len > 16)
+		return -EIO;
+
+	for (i = 0; i < len; i++) {
+		if (i % 4 == 0)
+			vals[i/4] = 0;
+		vals[i/4] <<= 8;
+		vals[i/4] |= buf[i];
+	}
+
+	ret = regmap_bulk_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0,
+				vals, ARRAY_SIZE(vals));
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static int rtl9300_i2c_writel(struct rtl9300_i2c *i2c, u32 data)
+{
+	int ret;
+
+	ret = regmap_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+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;
+	int ret;
+
+	if (read_write == I2C_SMBUS_READ)
+		val = 0;
+	else
+		val = RWOP;
+	mask = RWOP;
+
+	val |= I2C_TRIG;
+	mask |= I2C_TRIG;
+
+	ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, mask, val);
+	if (ret)
+		return ret;
+
+	ret = regmap_read_poll_timeout(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1,
+				       val, !(val & I2C_TRIG), 100, 2000);
+	if (ret)
+		return ret;
+
+	if (val & I2C_FAIL)
+		return -EIO;
+
+	if (read_write == I2C_SMBUS_READ) {
+		if (size == I2C_SMBUS_BYTE || size == I2C_SMBUS_BYTE_DATA) {
+			ret = regmap_read(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, &val);
+			if (ret)
+				return ret;
+			data->byte = val & 0xff;
+		} else if (size == I2C_SMBUS_WORD_DATA) {
+			ret = regmap_read(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, &val);
+			if (ret)
+				return ret;
+			data->word = val & 0xffff;
+		} else {
+			rtl9300_i2c_read(i2c, &data->block[0], len);
+		}
+	}
+
+	return 0;
+}
+
+static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+		  unsigned short flags, char read_write,
+		  u8 command, int size, union i2c_smbus_data *data)
+{
+	struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
+	int len = 0, ret;
+
+	mutex_lock(&i2c_lock);
+	switch (size) {
+	case I2C_SMBUS_QUICK:
+		rtl9300_i2c_config_xfer(i2c, addr, 0);
+		rtl9300_i2c_reg_addr_set(i2c, 0, 0);
+		break;
+
+	case I2C_SMBUS_BYTE:
+		if (read_write == I2C_SMBUS_WRITE) {
+			rtl9300_i2c_config_xfer(i2c, addr, 0);
+			rtl9300_i2c_reg_addr_set(i2c, command, 1);
+		} else {
+			rtl9300_i2c_config_xfer(i2c, addr, 1);
+			rtl9300_i2c_reg_addr_set(i2c, 0, 0);
+		}
+		break;
+
+	case I2C_SMBUS_BYTE_DATA:
+		rtl9300_i2c_reg_addr_set(i2c, command, 1);
+		rtl9300_i2c_config_xfer(i2c, addr, 1);
+		if (read_write == I2C_SMBUS_WRITE)
+			rtl9300_i2c_writel(i2c, data->byte);
+		break;
+
+	case I2C_SMBUS_WORD_DATA:
+		rtl9300_i2c_reg_addr_set(i2c, command, 1);
+		rtl9300_i2c_config_xfer(i2c, addr, 2);
+		if (read_write == I2C_SMBUS_WRITE)
+			rtl9300_i2c_writel(i2c, data->word);
+		break;
+
+	case I2C_SMBUS_BLOCK_DATA:
+		rtl9300_i2c_reg_addr_set(i2c, command, 1);
+		rtl9300_i2c_config_xfer(i2c, addr, data->block[0]);
+		if (read_write == I2C_SMBUS_WRITE)
+			rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
+		len = data->block[0];
+		break;
+
+	default:
+		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
+		ret = -EOPNOTSUPP;
+		goto out_unlock;
+	}
+
+	ret = rtl9300_i2c_execute_xfer(i2c, read_write, size, data, len);
+
+out_unlock:
+	mutex_unlock(&i2c_lock);
+
+	return ret;
+}
+
+static u32 rtl9300_i2c_func(struct i2c_adapter *a)
+{
+	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
+	       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
+	       I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm rtl9300_i2c_algo = {
+	.smbus_xfer	= rtl9300_i2c_smbus_xfer,
+	.functionality	= rtl9300_i2c_func,
+};
+
+struct i2c_adapter_quirks rtl9300_i2c_quirks = {
+	.flags		= I2C_AQ_NO_CLK_STRETCH,
+	.max_read_len	= 16,
+	.max_write_len	= 16,
+};
+
+static int rtl9300_i2c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rtl9300_i2c *i2c;
+	struct i2c_adapter *adap;
+	u32 clock_freq, sda_pin;
+	int ret;
+
+	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->regmap = syscon_node_to_regmap(dev->parent->of_node);
+	if (IS_ERR(i2c->regmap))
+		return PTR_ERR(i2c->regmap);
+	i2c->dev = dev;
+
+	ret = device_property_read_u32(dev, "realtek,control-offset", &i2c->i2c_mst_ofs);
+	if (ret)
+		return ret;
+
+	ret = device_property_read_u32(dev, "realtek,global-control-offset", &i2c->i2c_mst_glb_ofs);
+	if (ret)
+		return ret;
+
+	ret = device_property_read_u32(dev, "clock-frequency", &clock_freq);
+	if (ret)
+		clock_freq = I2C_MAX_STANDARD_MODE_FREQ;
+
+	switch (clock_freq) {
+	case I2C_MAX_STANDARD_MODE_FREQ:
+		i2c->bus_freq = RTL9300_I2C_STD_FREQ;
+		break;
+
+	case I2C_MAX_FAST_MODE_FREQ:
+		i2c->bus_freq = RTL9300_I2C_FAST_FREQ;
+		break;
+	default:
+		dev_warn(i2c->dev, "clock-frequency %d not supported\n", clock_freq);
+		return -EINVAL;
+	}
+
+	ret = device_property_read_u32(dev, "realtek,sda-pin", &sda_pin);
+	if (ret)
+		sda_pin = 0;
+	i2c->sda_pin = sda_pin;
+
+	adap = &i2c->adap;
+	adap->owner = THIS_MODULE;
+	adap->algo = &rtl9300_i2c_algo;
+	adap->quirks = &rtl9300_i2c_quirks;
+	adap->retries = 3;
+	adap->dev.parent = dev;
+	i2c_set_adapdata(adap, i2c);
+	adap->dev.of_node = dev->of_node;
+	strscpy(adap->name, dev_name(dev), sizeof(adap->name));
+
+	platform_set_drvdata(pdev, i2c);
+
+	ret = rtl9300_i2c_config_io(i2c, i2c->sda_pin);
+	if (ret)
+		return ret;
+
+	return i2c_add_adapter(adap);
+}
+
+static void rtl9300_i2c_remove(struct platform_device *pdev)
+{
+	struct rtl9300_i2c *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adap);
+}
+
+static const struct of_device_id i2c_rtl9300_dt_ids[] = {
+	{ .compatible = "realtek,rtl9300-i2c" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
+
+static struct platform_driver rtl9300_i2c_driver = {
+	.probe = rtl9300_i2c_probe,
+	.remove = rtl9300_i2c_remove,
+	.driver = {
+		.name = "i2c-rtl9300",
+		.of_match_table = i2c_rtl9300_dt_ids,
+	},
+};
+
+module_platform_driver(rtl9300_i2c_driver);
+
+MODULE_DESCRIPTION("RTL9300 I2C controller driver");
+MODULE_LICENSE("GPL");
+
-- 
2.46.1

[PATCH 3/5] mips: dts: realtek: Add I2C controllers

[Chris Packham]

Add the I2C controllers that are part of the RTL9300 SoC.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 arch/mips/boot/dts/realtek/rtl930x.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/mips/boot/dts/realtek/rtl930x.dtsi b/arch/mips/boot/dts/realtek/rtl930x.dtsi
index cf1b38b6c353..c579665df700 100644
--- a/arch/mips/boot/dts/realtek/rtl930x.dtsi
+++ b/arch/mips/boot/dts/realtek/rtl930x.dtsi
@@ -39,6 +39,24 @@ reboot {
 			offset = <0x0c>;
 			value = <0x01>;
 		};
+
+		i2c0: i2c@36c {
+			compatible = "realtek,rtl9300-i2c";
+			realtek,control-offset = <0x36c>;
+			realtek,global-control-offset = <0x384>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c1: i2c@388 {
+			compatible = "realtek,rtl9300-i2c";
+			realtek,control-offset = <0x388>;
+			realtek,global-control-offset = <0x384>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
 	};
 };
 
-- 
2.46.1

[PATCH 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer

[Chris Packham]

An extension of the RTL9300 SoC is to support multiplexing by selecting
the SDA pins that are being used dynamically. Add a binding that allows
us to describe hardware that makes use of this.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 .../bindings/i2c/realtek,rtl9300-i2c-mux.yaml | 82 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml

diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
new file mode 100644
index 000000000000..a64879d0fda7
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c-mux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek RTL I2C Multiplexer
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+description: |
+  The I2C controllers on the RTL9300 support a level of multiplexing. In the
+  simple case the rtl9300-i2c binding can provide a single SDA pin per
+  controller. This binding allows a more than one SDA line to be used per
+  controller providing a level of multiplexing.
+
+properties:
+  compatible:
+    const: realtek,rtl9300-i2c-mux
+
+  i2c-parent:
+    description: phandle of the I2C bus controller that this multiplexer
+      operates on.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+allOf:
+  - $ref: i2c-mux.yaml
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - i2c-parent
+
+examples:
+  - |
+    switch@1b000000 {
+      compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
+      reg = <0x1b000000 0x10000>;
+
+      i2c0: i2c@36c {
+        compatible = "realtek,rtl9300-i2c";
+        realtek,control-offset = <0x36c>;
+        realtek,global-control-offset = <0x384>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+      };
+    };
+
+    base {
+      i2c-mux {
+        compatible = "realtek,rtl9300-i2c-mux";
+        #address-cells = <1>;
+        #size-cells = <0>;
+        i2c-parent = <&i2c0>;
+
+        i2c@0 {
+          reg = <0>;
+          #address-cells = <1>;
+          #size-cells = <0>;
+          gpio@20 {
+              compatible = "nxp,pca9555";
+              gpio-controller;
+              #gpio-cells = <2>;
+              reg = <0x20>;
+          };
+        };
+
+        i2c@2 {
+          reg = <2>;
+          #address-cells = <1>;
+          #size-cells = <0>;
+          gpio@20 {
+              compatible = "nxp,pca9555";
+              gpio-controller;
+              #gpio-cells = <2>;
+              reg = <0x20>;
+          };
+        };
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 9e123e9839a5..178ac8a7e843 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19891,6 +19891,7 @@ RTL9300 I2C DRIVER (rtl9300-i2c)
 M:	Chris Packham <chris.packham@alliedtelesis.co.nz>
 L:	linux-i2c@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
 F:	Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
 F:	drivers/i2c/busses/i2c-rtl9300.c
 
-- 
2.46.1

[PATCH 5/5] i2c: rtl9300: Add multiplexing support

[Chris Packham]

The RTL9300 I2C controller can support multiplexing by choosing the SDA
pin to be used dynamically. Add mutliplexing support to the rtl9300
driver.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-rtl9300.c | 168 ++++++++++++++++++++++++++++++-
 1 file changed, 167 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index f16e9b6343bf..a934a2526c92 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #include <linux/i2c.h>
+#include <linux/i2c-mux.h>
 #include <linux/mod_devicetable.h>
 #include <linux/mfd/syscon.h>
 #include <linux/mutex.h>
@@ -44,6 +45,14 @@ struct rtl9300_i2c {
 #define RTL9300_I2C_STD_FREQ		0
 #define RTL9300_I2C_FAST_FREQ		1
 
+struct rtl9300_i2c_chan {
+	int parent;
+	const unsigned int *chans;
+	int n_chan;
+};
+
+#define RTL9300_I2C_MUX_NCHAN	8
+
 DEFINE_MUTEX(i2c_lock);
 
 static int rtl9300_i2c_reg_addr_set(struct rtl9300_i2c *i2c, u32 reg, u16 len)
@@ -370,7 +379,164 @@ static struct platform_driver rtl9300_i2c_driver = {
 	},
 };
 
-module_platform_driver(rtl9300_i2c_driver);
+static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 chan)
+{
+	struct i2c_adapter *adap = muxc->parent;
+	struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
+	int ret;
+
+	ret = rtl9300_i2c_config_io(i2c, chan);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rtl9300_i2c_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
+{
+	struct i2c_adapter *adap = muxc->parent;
+	struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
+	int ret;
+
+	ret = rtl9300_i2c_config_io(i2c, i2c->sda_pin);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	struct device_node *np = dev->of_node;
+	struct device_node *adap_np;
+	struct i2c_adapter *adap = NULL;
+	struct fwnode_handle *child;
+	unsigned int *chans;
+	int i = 0;
+
+	if (!is_of_node(fwnode))
+		return -EOPNOTSUPP;
+
+	if (!np)
+		return -ENODEV;
+
+	adap_np = of_parse_phandle(np, "i2c-parent", 0);
+	if (!adap_np) {
+		dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
+		return -ENODEV;
+	}
+	adap = of_find_i2c_adapter_by_node(adap_np);
+	of_node_put(adap_np);
+
+	if (!adap)
+		return -EPROBE_DEFER;
+
+	mux->parent = i2c_adapter_id(adap);
+	put_device(&adap->dev);
+
+	mux->n_chan = device_get_child_node_count(dev);
+	if (mux->n_chan >= RTL9300_I2C_MUX_NCHAN)
+		return -EINVAL;
+
+	chans = devm_kcalloc(dev, mux->n_chan, sizeof(*mux->chans), GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	device_for_each_child_node(dev, child) {
+		fwnode_property_read_u32(child, "reg", chans + i);
+		i++;
+	}
+	mux->chans = chans;
+
+	return 0;
+}
+
+static int rtl9300_i2c_mux_probe(struct platform_device *pdev)
+{
+	struct i2c_mux_core *muxc;
+	struct i2c_adapter *adap;
+	struct rtl9300_i2c_chan *mux;
+	int ret, i;
+
+	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return -ENOMEM;
+
+	ret = rtl9300_i2c_mux_probe_fw(mux, pdev);
+	if (ret)
+		return ret;
+
+	adap = i2c_get_adapter(mux->parent);
+	if (!adap)
+		return -EPROBE_DEFER;
+
+	muxc = i2c_mux_alloc(adap, &pdev->dev, mux->n_chan, 0, 0,
+			     rtl9300_i2c_select_chan, rtl9300_i2c_deselect_mux);
+	if (!muxc) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
+	muxc->priv = mux;
+
+	platform_set_drvdata(pdev, muxc);
+
+	for (i = 0; i < mux->n_chan; i++) {
+		ret = i2c_mux_add_adapter(muxc, 0, mux->chans[i]);
+		if (ret)
+			goto err_del_adapters;
+	}
+
+	return 0;
+
+err_del_adapters:
+	i2c_mux_del_adapters(muxc);
+err_alloc:
+	i2c_put_adapter(adap);
+
+	return ret;
+}
+
+static void rtl9300_i2c_mux_remove(struct platform_device *pdev)
+{
+	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
+
+	i2c_mux_del_adapters(muxc);
+	i2c_put_adapter(muxc->parent);
+}
+
+static const struct of_device_id i2c_rtl9300_mux_dt_ids[] = {
+	{ .compatible = "realtek,rtl9300-i2c-mux" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, i2c_rtl9300_mux_dt_ids);
+
+static struct platform_driver rtl9300_i2c_mux_driver = {
+	.probe = rtl9300_i2c_mux_probe,
+	.remove = rtl9300_i2c_mux_remove,
+	.driver = {
+		.name = "i2c-mux-rtl9300",
+		.of_match_table = i2c_rtl9300_mux_dt_ids,
+	},
+};
+
+static struct platform_driver * const drivers[] = {
+	&rtl9300_i2c_driver,
+	&rtl9300_i2c_mux_driver,
+};
+
+static int __init rtl9300_i2c_init(void)
+{
+	return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
+}
+module_init(rtl9300_i2c_init);
+
+static void __exit rtl9300_i2c_exit(void)
+{
+	platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
+}
+module_exit(rtl9300_i2c_exit);
 
 MODULE_DESCRIPTION("RTL9300 I2C controller driver");
 MODULE_LICENSE("GPL");
-- 
2.46.1

Re: [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller

[Rob Herring (Arm)]

On Wed, 18 Sep 2024 11:29:28 +1200, Chris Packham wrote:
> Add dtschema for the I2C controller on the RTL9300 SoC. The I2C
> controllers on this SoC are part of the "switch" block which is
> represented here as a syscon node. The SCL pins are dependent on the I2C
> controller (GPIO8 for the first controller, GPIO 17 for the second). The
> SDA pins can be assigned to either one of the I2C controllers (but not
> both).
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     This does hit generate the following dt_binding_check warning
> 
>     realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
> 
>     Which is totally correct. I haven't given this thing a reg property
>     because I'm using an offset from the parent syscon node. I'm also not
>     calling the first offset "offset" but I don't think that'd help.
> 
>     I looked at a couple of other examples of devices that are children of
>     syscon nodes (e.g. armada-ap806-thermal, ap806-cpu-clock) these do have
>     a reg property in the dts but as far as I can see from the code it's not
>     actually used, instead the register offsets are in the code looked up
>     from the driver data (in at least one-case the reg offset is for a
>     legacy usage).
> 
>     So I'm a little unsure what to do here. I can add a reg property and
>     update the driver to use that to get the offset for the first set of
>     registers (or just not use it). Or I could drop the @36c from the node
>     names but then I coudn't distinguish the two controllers without failing
>     the $nodename: requirement from i2c-controller.yaml.
> 
>  .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 73 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.example.dts:32.19-38.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@388: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.example.dtb: /example-0/switch@1b000000: failed to match any schema with compatible: ['realtek,rtl9302c-switch', 'syscon', 'simple-mfd']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240917232932.3641992-2-chris.packham@alliedtelesis.co.nz

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 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer

[Rob Herring (Arm)]

On Wed, 18 Sep 2024 11:29:31 +1200, Chris Packham wrote:
> An extension of the RTL9300 SoC is to support multiplexing by selecting
> the SDA pins that are being used dynamically. Add a binding that allows
> us to describe hardware that makes use of this.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  .../bindings/i2c/realtek,rtl9300-i2c-mux.yaml | 82 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.example.dts:22.25-28.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.example.dtb: /example-0/switch@1b000000: failed to match any schema with compatible: ['realtek,rtl9302c-switch', 'syscon', 'simple-mfd']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240917232932.3641992-5-chris.packham@alliedtelesis.co.nz

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 1/5] dt-bindings: i2c: Add RTL9300 I2C controller

[Rob Herring]

On Wed, Sep 18, 2024 at 11:29:28AM +1200, Chris Packham wrote:
> Add dtschema for the I2C controller on the RTL9300 SoC. The I2C
> controllers on this SoC are part of the "switch" block which is
> represented here as a syscon node. The SCL pins are dependent on the I2C
> controller (GPIO8 for the first controller, GPIO 17 for the second). The
> SDA pins can be assigned to either one of the I2C controllers (but not
> both).
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     This does hit generate the following dt_binding_check warning
>     
>     realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
>     
>     Which is totally correct. I haven't given this thing a reg property
>     because I'm using an offset from the parent syscon node. I'm also not
>     calling the first offset "offset" but I don't think that'd help.
>     
>     I looked at a couple of other examples of devices that are children of
>     syscon nodes (e.g. armada-ap806-thermal, ap806-cpu-clock) these do have
>     a reg property in the dts but as far as I can see from the code it's not
>     actually used, instead the register offsets are in the code looked up
>     from the driver data (in at least one-case the reg offset is for a
>     legacy usage).
>     
>     So I'm a little unsure what to do here. I can add a reg property and
>     update the driver to use that to get the offset for the first set of
>     registers (or just not use it). Or I could drop the @36c from the node
>     names but then I coudn't distinguish the two controllers without failing
>     the $nodename: requirement from i2c-controller.yaml.

Use 'reg'. Looks like you need 2 entries for it.

Whether a driver of some OS decides to use it or not is irrelevant to 
the binding.

> 
>  .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 73 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> new file mode 100644
> index 000000000000..5b74a1986720
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek RTL I2C Controller
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description: |

Don't need '|' if no formatting.

> +  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.
> +
> +properties:
> +  compatible:
> +    const: realtek,rtl9300-i2c
> +
> +  realtek,control-offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Offset of the registers for this I2C controller
> +
> +  realtek,global-control-offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Offset of the I2C global control register (common between
> +      controllers).
> +
> +  clock-frequency:
> +    enum: [ 100000, 400000 ]
> +
> +  realtek,sda-pin:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0

0 is already the minimum.

> +    maximum: 7
> +    description:
> +      SDA pin associated with this I2C controller.
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - realtek,control-offset
> +  - realtek,global-control-offset
> +
> +examples:
> +  - |
> +    switch@1b000000 {
> +      compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
> +      reg = <0x1b000000 0x10000>;
> +
> +      i2c@36c {
> +        compatible = "realtek,rtl9300-i2c";
> +        realtek,control-offset = <0x36c>;
> +        realtek,global-control-offset = <0x384>;
> +        clock-frequency = <100000>;
> +        realtek,sda-pin = <2>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +      };
> +
> +      i2c@388 {
> +        compatible = "realtek,rtl9300-i2c";
> +        realtek,control-offset = <0x388>;
> +        realtek,global-control-offset = <0x384>;

Humm, normally we don't want the same address in multiple 'reg' entries. 
Is this offset known to vary? It could just be hardcoded in the driver 
or implicit from the compatible (different compatible is the usual way 
to deal with differing register layouts).

Rob

Re: [PATCH 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer

[Rob Herring]

On Wed, Sep 18, 2024 at 11:29:31AM +1200, Chris Packham wrote:
> An extension of the RTL9300 SoC is to support multiplexing by selecting
> the SDA pins that are being used dynamically. Add a binding that allows
> us to describe hardware that makes use of this.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  .../bindings/i2c/realtek,rtl9300-i2c-mux.yaml | 82 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
> new file mode 100644
> index 000000000000..a64879d0fda7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c-mux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek RTL I2C Multiplexer
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description: |
> +  The I2C controllers on the RTL9300 support a level of multiplexing. In the
> +  simple case the rtl9300-i2c binding can provide a single SDA pin per
> +  controller. This binding allows a more than one SDA line to be used per
> +  controller providing a level of multiplexing.
> +
> +properties:
> +  compatible:
> +    const: realtek,rtl9300-i2c-mux
> +
> +  i2c-parent:
> +    description: phandle of the I2C bus controller that this multiplexer
> +      operates on.
> +    $ref: /schemas/types.yaml#/definitions/phandle

The mux isn't a separate device, so I think this should just be part of 
the i2c parent:

i2c-mux@36c {
  i2c@0 {
    ...
  };
  i2c@1 {
    ...
  };
  ...
};

And then you can get rid of the SDA pin property. If you only use 1 pin, 
then there is just 1 'i2c' child node with an address matching the SDA 
pin.

Rob

Re: [PATCH 2/5] i2c: Add driver for the RTL9300 I2C controller

[Andi Shyti]

Hi Chris,

On Wed, Sep 18, 2024 at 11:29:29AM GMT, Chris Packham wrote:
> Add support for the I2C controller on the RTL9300 SoC. This is based on
> the openwrt implementation[1] but cleaned up to make use of the regmap
> APIs.

Can you please add a few more words to describe the device?

> [1] - https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-5.15/drivers/i2c/busses/i2c-rtl9300.c
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

...

> +#define I2C_MST_CTRL1		0x0
> +#define  MEM_ADDR_OFS		8
> +#define  MEM_ADDR_MASK		0xffffff
> +#define  SDA_OUT_SEL_OFS	4
> +#define  SDA_OUT_SEL_MASK	0x7
> +#define  GPIO_SCL_SEL		BIT(3)
> +#define  RWOP			BIT(2)
> +#define  I2C_FAIL		BIT(1)
> +#define  I2C_TRIG		BIT(0)
> +#define I2C_MST_CTRL2		0x4
> +#define  RD_MODE		BIT(15)
> +#define  DEV_ADDR_OFS		8
> +#define  DEV_ADDR_MASK		0x7f
> +#define  DATA_WIDTH_OFS		4
> +#define  DATA_WIDTH_MASK	0xf
> +#define  MEM_ADDR_WIDTH_OFS	2
> +#define  MEM_ADDR_WIDTH_MASK	0x3

can we have these masked already shifted? You could use
GENMASK().

> +#define  SCL_FREQ_OFS		0
> +#define  SCL_FREQ_MASK		0x3
> +#define I2C_MST_DATA_WORD0	0x8
> +#define I2C_MST_DATA_WORD1	0xc
> +#define I2C_MST_DATA_WORD2	0x10
> +#define I2C_MST_DATA_WORD3	0x14

Can we use a prefix for all these defines?

> +
> +#define RTL9300_I2C_STD_FREQ		0
> +#define RTL9300_I2C_FAST_FREQ		1

This can also be an enum.

> +
> +DEFINE_MUTEX(i2c_lock);

...

> +static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len)
> +{
> +	u32 vals[4] = {};
> +	int i, ret;
> +
> +	if (len > 16)
> +		return -EIO;
> +
> +	for (i = 0; i < len; i++) {
> +		if (i % 4 == 0)
> +			vals[i/4] = 0;
> +		vals[i/4] <<= 8;
> +		vals[i/4] |= buf[i];
> +	}
> +
> +	ret = regmap_bulk_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0,
> +				vals, ARRAY_SIZE(vals));
> +	if (ret)
> +		return ret;
> +
> +	return len;

why returning "len"? And in any case this is ignored.

> +}
> +
> +static int rtl9300_i2c_writel(struct rtl9300_i2c *i2c, u32 data)
> +{
> +	int ret;
> +
> +	ret = regmap_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

return regmap_write(...) ?

In any case, the returned value of these functions is completely
ignored, not even printed. Should we either:

 - condier the return value in the _xfer() functions
 or
 - make all these functions void?

> +}
> +
> +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;
> +	int ret;
> +
> +	if (read_write == I2C_SMBUS_READ)
> +		val = 0;
> +	else
> +		val = RWOP;
> +	mask = RWOP;
> +
> +	val |= I2C_TRIG;
> +	mask |= I2C_TRIG;

how about "mask = RWOP | I2C_TRIG" to make it in one line?

Also val can be simplified as:

	val = I2C_TRIG;
	if (read_write == I2C_SMBUS_WRITE)
		val |= RWOP;

Not a binding commeent, as you wish.

> +
> +	ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read_poll_timeout(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1,
> +				       val, !(val & I2C_TRIG), 100, 2000);
> +	if (ret)
> +		return ret;
> +
> +	if (val & I2C_FAIL)

where is val taking taking this bit?

> +		return -EIO;
> +

...

> +	switch (size) {
> +	case I2C_SMBUS_QUICK:
...
> +	case I2C_SMBUS_BYTE:
...
> +	case I2C_SMBUS_BYTE_DATA:
...
> +	case I2C_SMBUS_WORD_DATA:
...
> +	case I2C_SMBUS_BLOCK_DATA:
...
> +	default:
> +		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);

dev_err() ?

> +		ret = -EOPNOTSUPP;
> +		goto out_unlock;
> +	}

...

> +	switch (clock_freq) {
> +	case I2C_MAX_STANDARD_MODE_FREQ:
...
> +	case I2C_MAX_FAST_MODE_FREQ:
...
> +	default:
> +		dev_warn(i2c->dev, "clock-frequency %d not supported\n", clock_freq);
> +		return -EINVAL;

If we are returning an error we should print an error, let's make
it a "return dev_err_probe()"

But, I was thinking that by default we can assign
I2C_MAX_STANDARD_MODE_FREQ and if the DTS defines a different
frequency we could just print an error and stick to the default
value. Makes sense?

> +	}

...

> +	return i2c_add_adapter(adap);

return devm_i2c_add_adapter(adap);

and the remove function is not needed.

> +}
> +
> +static void rtl9300_i2c_remove(struct platform_device *pdev)
> +{
> +	struct rtl9300_i2c *i2c = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c->adap);
> +}
> +
> +static const struct of_device_id i2c_rtl9300_dt_ids[] = {
> +	{ .compatible = "realtek,rtl9300-i2c" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
> +
> +static struct platform_driver rtl9300_i2c_driver = {
> +	.probe = rtl9300_i2c_probe,
> +	.remove = rtl9300_i2c_remove,
> +	.driver = {
> +		.name = "i2c-rtl9300",
> +		.of_match_table = i2c_rtl9300_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(rtl9300_i2c_driver);
> +
> +MODULE_DESCRIPTION("RTL9300 I2C controller driver");
> +MODULE_LICENSE("GPL");
> +

Just a trailing blank line here.

Thanks,
Andi

Re: [PATCH 5/5] i2c: rtl9300: Add multiplexing support

[Andi Shyti]

Hi Chris,

...

> -module_platform_driver(rtl9300_i2c_driver);
> +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 chan)
> +{
> +	struct i2c_adapter *adap = muxc->parent;
> +	struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
> +	int ret;
> +
> +	ret = rtl9300_i2c_config_io(i2c, chan);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

return "rtl9300_i2c_config_io()"?

> +}

...

> +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	struct device_node *np = dev->of_node;
> +	struct device_node *adap_np;
> +	struct i2c_adapter *adap = NULL;
> +	struct fwnode_handle *child;
> +	unsigned int *chans;
> +	int i = 0;
> +
> +	if (!is_of_node(fwnode))
> +		return -EOPNOTSUPP;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	adap_np = of_parse_phandle(np, "i2c-parent", 0);
> +	if (!adap_np) {
> +		dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
> +		return -ENODEV;

return dev_err_probe(...)?

> +	}
> +	adap = of_find_i2c_adapter_by_node(adap_np);
> +	of_node_put(adap_np);

...

> +static int __init rtl9300_i2c_init(void)
> +{
> +	return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
> +}
> +module_init(rtl9300_i2c_init);
> +
> +static void __exit rtl9300_i2c_exit(void)
> +{
> +	platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
> +}
> +module_exit(rtl9300_i2c_exit);

You could use module_platform_driver()

Thanks,
Andi

>  
>  MODULE_DESCRIPTION("RTL9300 I2C controller driver");
>  MODULE_LICENSE("GPL");
> -- 
> 2.46.1
> 

Re: [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller

[Chris Packham]

Hi Rob,

On 19/09/24 02:14, Rob Herring wrote:
> On Wed, Sep 18, 2024 at 11:29:28AM +1200, Chris Packham wrote:
>> Add dtschema for the I2C controller on the RTL9300 SoC. The I2C
>> controllers on this SoC are part of the "switch" block which is
>> represented here as a syscon node. The SCL pins are dependent on the I2C
>> controller (GPIO8 for the first controller, GPIO 17 for the second). The
>> SDA pins can be assigned to either one of the I2C controllers (but not
>> both).
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      This does hit generate the following dt_binding_check warning
>>      
>>      realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
>>      
>>      Which is totally correct. I haven't given this thing a reg property
>>      because I'm using an offset from the parent syscon node. I'm also not
>>      calling the first offset "offset" but I don't think that'd help.
>>      
>>      I looked at a couple of other examples of devices that are children of
>>      syscon nodes (e.g. armada-ap806-thermal, ap806-cpu-clock) these do have
>>      a reg property in the dts but as far as I can see from the code it's not
>>      actually used, instead the register offsets are in the code looked up
>>      from the driver data (in at least one-case the reg offset is for a
>>      legacy usage).
>>      
>>      So I'm a little unsure what to do here. I can add a reg property and
>>      update the driver to use that to get the offset for the first set of
>>      registers (or just not use it). Or I could drop the @36c from the node
>>      names but then I coudn't distinguish the two controllers without failing
>>      the $nodename: requirement from i2c-controller.yaml.
> Use 'reg'. Looks like you need 2 entries for it.
>
> Whether a driver of some OS decides to use it or not is irrelevant to
> the binding.

OK thanks for that clarification.

>>   .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 73 +++++++++++++++++++
>>   MAINTAINERS                                   |  6 ++
>>   2 files changed, 79 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
>> new file mode 100644
>> index 000000000000..5b74a1986720
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
>> @@ -0,0 +1,73 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Realtek RTL I2C Controller
>> +
>> +maintainers:
>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>> +
>> +description: |
> Don't need '|' if no formatting.

Ack

>> +  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.
>> +
>> +properties:
>> +  compatible:
>> +    const: realtek,rtl9300-i2c
>> +
>> +  realtek,control-offset:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Offset of the registers for this I2C controller
>> +
>> +  realtek,global-control-offset:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Offset of the I2C global control register (common between
>> +      controllers).
>> +
>> +  clock-frequency:
>> +    enum: [ 100000, 400000 ]
>> +
>> +  realtek,sda-pin:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
> 0 is already the minimum.
Ack
>> +    maximum: 7
>> +    description:
>> +      SDA pin associated with this I2C controller.
>> +
>> +allOf:
>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>> +
>> +unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - realtek,control-offset
>> +  - realtek,global-control-offset
>> +
>> +examples:
>> +  - |
>> +    switch@1b000000 {
>> +      compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
>> +      reg = <0x1b000000 0x10000>;
>> +
>> +      i2c@36c {
>> +        compatible = "realtek,rtl9300-i2c";
>> +        realtek,control-offset = <0x36c>;
>> +        realtek,global-control-offset = <0x384>;
>> +        clock-frequency = <100000>;
>> +        realtek,sda-pin = <2>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +      };
>> +
>> +      i2c@388 {
>> +        compatible = "realtek,rtl9300-i2c";
>> +        realtek,control-offset = <0x388>;
>> +        realtek,global-control-offset = <0x384>;
> Humm, normally we don't want the same address in multiple 'reg' entries.
> Is this offset known to vary? It could just be hardcoded in the driver
> or implicit from the compatible (different compatible is the usual way
> to deal with differing register layouts).

It's one of those annoying registers that is sandwiched between the two 
I2C controllers that affects both of them so you kind of need to use it 
whether your using I2C1 or I2C2. For the RTL9300 it'll always be 0x384. 
There is a RTL9310 that appears to have it at a different offset but I'm 
not planning on adding support for that chip any time soon.

If I switch to "reg" for the I2C controller registers as suggested above 
I can just go with a hard coded register value in the driver for the 
global control.

Re: [PATCH 2/5] i2c: Add driver for the RTL9300 I2C controller

[Chris Packham]

Hi Andy,

On 19/09/24 08:27, Andi Shyti wrote:
> Hi Chris,
>
> On Wed, Sep 18, 2024 at 11:29:29AM GMT, Chris Packham wrote:
>> Add support for the I2C controller on the RTL9300 SoC. This is based on
>> the openwrt implementation[1] but cleaned up to make use of the regmap
>> APIs.
> Can you please add a few more words to describe the device?

Sure will do.

>> [1] - https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-5.15/drivers/i2c/busses/i2c-rtl9300.c
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ...
>
>> +#define I2C_MST_CTRL1		0x0
>> +#define  MEM_ADDR_OFS		8
>> +#define  MEM_ADDR_MASK		0xffffff
>> +#define  SDA_OUT_SEL_OFS	4
>> +#define  SDA_OUT_SEL_MASK	0x7
>> +#define  GPIO_SCL_SEL		BIT(3)
>> +#define  RWOP			BIT(2)
>> +#define  I2C_FAIL		BIT(1)
>> +#define  I2C_TRIG		BIT(0)
>> +#define I2C_MST_CTRL2		0x4
>> +#define  RD_MODE		BIT(15)
>> +#define  DEV_ADDR_OFS		8
>> +#define  DEV_ADDR_MASK		0x7f
>> +#define  DATA_WIDTH_OFS		4
>> +#define  DATA_WIDTH_MASK	0xf
>> +#define  MEM_ADDR_WIDTH_OFS	2
>> +#define  MEM_ADDR_WIDTH_MASK	0x3
> can we have these masked already shifted? You could use
> GENMASK().

I'll take a look.

>> +#define  SCL_FREQ_OFS		0
>> +#define  SCL_FREQ_MASK		0x3
>> +#define I2C_MST_DATA_WORD0	0x8
>> +#define I2C_MST_DATA_WORD1	0xc
>> +#define I2C_MST_DATA_WORD2	0x10
>> +#define I2C_MST_DATA_WORD3	0x14
> Can we use a prefix for all these defines?

Yes will add "RTL9300_".

I assume for the bit values too? So something like "MEM_ADDR_OFS" 
becomes "RTL9300_I2C_MST_CTRL1_MEM_ADDR_OFS" is that OK or too verbose?

>> +
>> +#define RTL9300_I2C_STD_FREQ		0
>> +#define RTL9300_I2C_FAST_FREQ		1
> This can also be an enum.
Ack
>
>> +
>> +DEFINE_MUTEX(i2c_lock);
> ...
>
>> +static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len)
>> +{
>> +	u32 vals[4] = {};
>> +	int i, ret;
>> +
>> +	if (len > 16)
>> +		return -EIO;
>> +
>> +	for (i = 0; i < len; i++) {
>> +		if (i % 4 == 0)
>> +			vals[i/4] = 0;
>> +		vals[i/4] <<= 8;
>> +		vals[i/4] |= buf[i];
>> +	}
>> +
>> +	ret = regmap_bulk_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0,
>> +				vals, ARRAY_SIZE(vals));
>> +	if (ret)
>> +		return ret;
>> +
>> +	return len;
> why returning "len"? And in any case this is ignored.
I copied that behaviour from the openwrt driver. I think making it the 
same as the other functions would make more sense.
>
>> +}
>> +
>> +static int rtl9300_i2c_writel(struct rtl9300_i2c *i2c, u32 data)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> return regmap_write(...) ?
>
> In any case, the returned value of these functions is completely
> ignored, not even printed. Should we either:
>
>   - condier the return value in the _xfer() functions
>   or
>   - make all these functions void?

I suppose it's a bit academic. Under the hood it's mmio so it's not as 
if it can really fail (famous last words). That said, this switch chip 
can be run in a core disabled mode and you could then in theory be doing 
I2C over SPI from an external SoC. If someone were just naively updating 
a hardware design to add the external SoC they might neglect to move the 
I2C connections. It's also just good practice so I'll propagate the 
returns up to the _xfer().

>> +}
>> +
>> +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;
>> +	int ret;
>> +
>> +	if (read_write == I2C_SMBUS_READ)
>> +		val = 0;
>> +	else
>> +		val = RWOP;
>> +	mask = RWOP;
>> +
>> +	val |= I2C_TRIG;
>> +	mask |= I2C_TRIG;
> how about "mask = RWOP | I2C_TRIG" to make it in one line?
>
> Also val can be simplified as:
>
> 	val = I2C_TRIG;
> 	if (read_write == I2C_SMBUS_WRITE)
> 		val |= RWOP;
>
> Not a binding commeent, as you wish.

I'll take a look. I kind of did like the pairing of val and mask for 
each thing being set.

>> +
>> +	ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, mask, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_read_poll_timeout(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1,
>> +				       val, !(val & I2C_TRIG), 100, 2000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (val & I2C_FAIL)
> where is val taking taking this bit?
In the regmap_read_poll_timeout().
>
>> +		return -EIO;
>> +
> ...
>
>> +	switch (size) {
>> +	case I2C_SMBUS_QUICK:
> ...
>> +	case I2C_SMBUS_BYTE:
> ...
>> +	case I2C_SMBUS_BYTE_DATA:
> ...
>> +	case I2C_SMBUS_WORD_DATA:
> ...
>> +	case I2C_SMBUS_BLOCK_DATA:
> ...
>> +	default:
>> +		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
> dev_err() ?
Ack.
>> +		ret = -EOPNOTSUPP;
>> +		goto out_unlock;
>> +	}
> ...
>
>> +	switch (clock_freq) {
>> +	case I2C_MAX_STANDARD_MODE_FREQ:
> ...
>> +	case I2C_MAX_FAST_MODE_FREQ:
> ...
>> +	default:
>> +		dev_warn(i2c->dev, "clock-frequency %d not supported\n", clock_freq);
>> +		return -EINVAL;
> If we are returning an error we should print an error, let's make
> it a "return dev_err_probe()"
>
> But, I was thinking that by default we can assign
> I2C_MAX_STANDARD_MODE_FREQ and if the DTS defines a different
> frequency we could just print an error and stick to the default
> value. Makes sense?

I don't have a strong opinion. Failing the probe just because something 
in the dts is wrong where we can have a sane default does seem overly 
harsh. On the other hand I've had hardware QA folks complain when the 
I2C bus is running at 98khz instead of 100khz.

>
>> +	}
> ...
>
>> +	return i2c_add_adapter(adap);
> return devm_i2c_add_adapter(adap);
>
> and the remove function is not needed.

OK thanks. I did look for a devm variant but obviously not hard enough.

>> +}
>> +
>> +static void rtl9300_i2c_remove(struct platform_device *pdev)
>> +{
>> +	struct rtl9300_i2c *i2c = platform_get_drvdata(pdev);
>> +
>> +	i2c_del_adapter(&i2c->adap);
>> +}
>> +
>> +static const struct of_device_id i2c_rtl9300_dt_ids[] = {
>> +	{ .compatible = "realtek,rtl9300-i2c" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
>> +
>> +static struct platform_driver rtl9300_i2c_driver = {
>> +	.probe = rtl9300_i2c_probe,
>> +	.remove = rtl9300_i2c_remove,
>> +	.driver = {
>> +		.name = "i2c-rtl9300",
>> +		.of_match_table = i2c_rtl9300_dt_ids,
>> +	},
>> +};
>> +
>> +module_platform_driver(rtl9300_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("RTL9300 I2C controller driver");
>> +MODULE_LICENSE("GPL");
>> +
> Just a trailing blank line here.
Ack.
>
> Thanks,
> Andi
>

Re: [PATCH 5/5] i2c: rtl9300: Add multiplexing support

[Chris Packham]

Hi Andi, Rob,

On 19/09/24 08:36, Andi Shyti wrote:
> Hi Chris,
>
> ...
>
>> -module_platform_driver(rtl9300_i2c_driver);
>> +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 chan)
>> +{
>> +	struct i2c_adapter *adap = muxc->parent;
>> +	struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
>> +	int ret;
>> +
>> +	ret = rtl9300_i2c_config_io(i2c, chan);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> return "rtl9300_i2c_config_io()"?

Ack.

>> +}
> ...
>
>> +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *adap_np;
>> +	struct i2c_adapter *adap = NULL;
>> +	struct fwnode_handle *child;
>> +	unsigned int *chans;
>> +	int i = 0;
>> +
>> +	if (!is_of_node(fwnode))
>> +		return -EOPNOTSUPP;
>> +
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	adap_np = of_parse_phandle(np, "i2c-parent", 0);
>> +	if (!adap_np) {
>> +		dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
>> +		return -ENODEV;
> return dev_err_probe(...)?

Ack.

>> +	}
>> +	adap = of_find_i2c_adapter_by_node(adap_np);
>> +	of_node_put(adap_np);
> ...
>
>> +static int __init rtl9300_i2c_init(void)
>> +{
>> +	return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
>> +}
>> +module_init(rtl9300_i2c_init);
>> +
>> +static void __exit rtl9300_i2c_exit(void)
>> +{
>> +	platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
>> +}
>> +module_exit(rtl9300_i2c_exit);
> You could use module_platform_driver()

Can I though? I want to support both the simple I2C controller and the 
MUX mode with the same driver. Which is why I've ended up with two 
drivers to register.

On the binding patch, Rob made the suggestion that I just make the 
i2c-mux part of the parent. I did consider that but quickly got tied in 
knots because I couldn't figure out how to have a device that is both an 
adapter and a mux. The main problem was that any child nodes of an i2c 
adapter in the device tree are presumed to be I2C devices and get probed 
automatically by of_i2c_register_devices(). Equally I can't register a 
mux without having an adapter that the mux operates over.

>
> Thanks,
> Andi
>
>>   
>>   MODULE_DESCRIPTION("RTL9300 I2C controller driver");
>>   MODULE_LICENSE("GPL");
>> -- 
>> 2.46.1
>>

Re: [PATCH 5/5] i2c: rtl9300: Add multiplexing support

[Chris Packham]

On 19/09/24 09:44, Chris Packham wrote:
> Hi Andi, Rob,
>
> On 19/09/24 08:36, Andi Shyti wrote:
>> Hi Chris,
>>
>> ...
>>
>>> -module_platform_driver(rtl9300_i2c_driver);
>>> +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 
>>> chan)
>>> +{
>>> +    struct i2c_adapter *adap = muxc->parent;
>>> +    struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
>>> +    int ret;
>>> +
>>> +    ret = rtl9300_i2c_config_io(i2c, chan);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return 0;
>> return "rtl9300_i2c_config_io()"?
>
> Ack.
>
>>> +}
>> ...
>>
>>> +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, 
>>> struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct fwnode_handle *fwnode = dev_fwnode(dev);
>>> +    struct device_node *np = dev->of_node;
>>> +    struct device_node *adap_np;
>>> +    struct i2c_adapter *adap = NULL;
>>> +    struct fwnode_handle *child;
>>> +    unsigned int *chans;
>>> +    int i = 0;
>>> +
>>> +    if (!is_of_node(fwnode))
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    if (!np)
>>> +        return -ENODEV;
>>> +
>>> +    adap_np = of_parse_phandle(np, "i2c-parent", 0);
>>> +    if (!adap_np) {
>>> +        dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
>>> +        return -ENODEV;
>> return dev_err_probe(...)?
>
> Ack.
>
>>> +    }
>>> +    adap = of_find_i2c_adapter_by_node(adap_np);
>>> +    of_node_put(adap_np);
>> ...
>>
>>> +static int __init rtl9300_i2c_init(void)
>>> +{
>>> +    return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
>>> +}
>>> +module_init(rtl9300_i2c_init);
>>> +
>>> +static void __exit rtl9300_i2c_exit(void)
>>> +{
>>> +    platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
>>> +}
>>> +module_exit(rtl9300_i2c_exit);
>> You could use module_platform_driver()
>
> Can I though? I want to support both the simple I2C controller and the 
> MUX mode with the same driver. Which is why I've ended up with two 
> drivers to register.
>
> On the binding patch, Rob made the suggestion that I just make the 
> i2c-mux part of the parent. I did consider that but quickly got tied 
> in knots because I couldn't figure out how to have a device that is 
> both an adapter and a mux. The main problem was that any child nodes 
> of an i2c adapter in the device tree are presumed to be I2C devices 
> and get probed automatically by of_i2c_register_devices(). Equally I 
> can't register a mux without having an adapter that the mux operates 
> over.

OK I think I've got something working that has a dt binding like

                 i2c@36c {
                         compatible = "realtek,rtl9300-i2c";
                         reg = <0x36c 0x14>;
                         status = "okay";
                         #address-cells = <0x01>;
                         #size-cells = <0x00>;

                         i2c@0 {
                                 reg = <0x00>;
                                 #address-cells = <1>;
                                 #size-cells = <1>;
                                 gpio@20 {
                                     ...
                                 };
                         };

                         i2c@2 {
                                 reg = <0x02>;
                         };
                 };

In the probe() I can iterate over the child nodes and create an adapter 
for each. The code is a bit fiddly but I think it's a net win if I can 
do away with the rtl9300-i2c-mux part. It also happily means that I 
don't have an extra I2C bus that is the same as the first mux channel.

I'll try an tidy things up and get another iteration out before my weekend.


>
>>
>> Thanks,
>> Andi
>>
>>>     MODULE_DESCRIPTION("RTL9300 I2C controller driver");
>>>   MODULE_LICENSE("GPL");
>>> -- 
>>> 2.46.1
>>>