linux-mips.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] i2c: RTL9300 support
@ 2024-09-23 23:02 Chris Packham
  2024-09-23 23:02 ` [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Chris Packham @ 2024-09-23 23:02 UTC (permalink / raw)
  To: andi.shyti, robh, krzk+dt, conor+dt, tsbogend
  Cc: linux-i2c, devicetree, linux-kernel, linux-mips, 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. From v2 of this series I've taken
the approach suggested by Rob and represented the SDA lines as child nodes. I
expect there will be a bit of discussion around the naming of the controller
nodes (in the Realtek documentation they are referred to as I2C_MST1 and
I2C_MST2).

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

--
2.46.1

Chris Packham (3):
  dt-bindings: i2c: Add RTL9300 I2C controller
  i2c: Add driver for the RTL9300 I2C controller
  mips: dts: realtek: Add I2C controllers

 .../bindings/i2c/realtek,rtl9300-i2c.yaml     |  80 ++++
 MAINTAINERS                                   |   7 +
 arch/mips/boot/dts/realtek/rtl930x.dtsi       |  18 +
 drivers/i2c/busses/Kconfig                    |  10 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-rtl9300.c              | 421 ++++++++++++++++++
 6 files changed, 537 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-rtl9300.c

-- 
2.46.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller
  2024-09-23 23:02 [PATCH v3 0/3] i2c: RTL9300 support Chris Packham
@ 2024-09-23 23:02 ` Chris Packham
  2024-09-24  8:46   ` Krzysztof Kozlowski
  2024-09-24  8:55   ` Krzysztof Kozlowski
  2024-09-23 23:02 ` [PATCH v3 2/3] i2c: Add driver for the " Chris Packham
  2024-09-23 23:02 ` [PATCH v3 3/3] mips: dts: realtek: Add I2C controllers Chris Packham
  2 siblings, 2 replies; 11+ messages in thread
From: Chris Packham @ 2024-09-23 23:02 UTC (permalink / raw)
  To: andi.shyti, robh, krzk+dt, conor+dt, tsbogend
  Cc: linux-i2c, devicetree, linux-kernel, linux-mips, 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:
    Changes in v3:
    - Remove parent node in example
    - put unevaluatedProperties after required
    - Add #address-cells and #size-cells
    
    Changes in v2:
    - Use reg property for controller registers
    - Remove global-control-offset (will be hard coded in driver)
    - Integrated the multiplexing function. Child nodes now represent the
      available SDA lines

 .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 80 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 86 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..979ec22e81f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
@@ -0,0 +1,80 @@
+# 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
+
+  reg:
+    description: Register offset and size this I2C controller.
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  '^i2c@[0-7]$':
+    $ref: /schemas/i2c/i2c-controller.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        description: The SDA pin associated with the I2C bus.
+        maxItems: 1
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c@36c {
+      compatible = "realtek,rtl9300-i2c";
+      reg = <0x36c 0x14>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      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 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/3] i2c: Add driver for the RTL9300 I2C controller
  2024-09-23 23:02 [PATCH v3 0/3] i2c: RTL9300 support Chris Packham
  2024-09-23 23:02 ` [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
@ 2024-09-23 23:02 ` Chris Packham
  2024-09-25  9:35   ` Andi Shyti
  2024-09-23 23:02 ` [PATCH v3 3/3] mips: dts: realtek: Add I2C controllers Chris Packham
  2 siblings, 1 reply; 11+ messages in thread
From: Chris Packham @ 2024-09-23 23:02 UTC (permalink / raw)
  To: andi.shyti, robh, krzk+dt, conor+dt, tsbogend
  Cc: linux-i2c, devicetree, linux-kernel, linux-mips, Chris Packham

Add support for the I2C controller on the RTL9300 SoC. There are two I2C
controllers in the RTL9300 that are part of the Ethernet switch register
block. Each of these controllers owns a SCL pin (GPIO8 for the fiorst
I2C controller, GPIO17 for the second). There are 8 possible SDA pins
(GPIO9-16) that can be assigned to either I2C controller. This
relationship is represented in the device tree with a child node for
each SDA line in use.

This is based on the openwrt implementation[1] but has been
significantly modified

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

Notes:
    Changes in v2:
    - Replace a number of return 0; with tail calls
    - Add enum rtl9300_bus_freq
    - Use RTL9300_ prefix on new defines
    - Use reg property for register offset
    - Hard code RTL9300_I2C_MST_GLB_CTRL address as this does not need to
      come from DT binding
    - Use GENMASK() where appropriate
    - Propagate read/write errors through to rtl9300_i2c_smbus_xfer()
    - Don't error out on bad clock-frequency
    - Use devm_i2c_add_adapter()
    - Put more information in the commit message
    - Integrated multiplexing function, an adapter is created per SDA line

 MAINTAINERS                      |   1 +
 drivers/i2c/busses/Kconfig       |  10 +
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-rtl9300.c | 421 +++++++++++++++++++++++++++++++
 4 files changed, 433 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..ed2d888e8a9e
--- /dev/null
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -0,0 +1,421 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/bits.h>
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+enum rtl9300_bus_freq {
+	RTL9300_I2C_STD_FREQ,
+	RTL9300_I2C_FAST_FREQ,
+};
+
+struct rtl9300_i2c;
+
+struct rtl9300_i2c_chan {
+	struct i2c_adapter adap;
+	struct rtl9300_i2c *i2c;
+	enum rtl9300_bus_freq bus_freq;
+	u8 sda_pin;
+};
+
+#define RTL9300_I2C_MUX_NCHAN	8
+
+struct rtl9300_i2c {
+	struct regmap *regmap;
+	struct device *dev;
+	struct rtl9300_i2c_chan chans[RTL9300_I2C_MUX_NCHAN];
+	u32 reg_base;
+	u8 sda_pin;
+};
+
+#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
+
+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 << 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);
+	if (ret)
+		return ret;
+
+	val = reg << RTL9300_I2C_MST_CTRL1_MEM_ADDR_OFS;
+	mask = RTL9300_I2C_MST_CTRL1_MEM_ADDR_MASK;
+
+	return regmap_update_bits(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL1, mask, val);
+}
+
+static int rtl9300_i2c_config_io(struct rtl9300_i2c *i2c, u8 sda_pin)
+{
+	int ret;
+	u32 val, mask;
+
+	ret = regmap_update_bits(i2c->regmap, RTL9300_I2C_MST_GLB_CTRL, 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;
+
+	return regmap_update_bits(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL1, mask, val);
+}
+
+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;
+
+	val |= addr << RTL9300_I2C_MST_CTRL2_DEV_ADDR_OFS;
+	mask |= RTL9300_I2C_MST_CTRL2_DEV_ADDR_MASK;
+
+	val |= ((len - 1) & 0xf) << RTL9300_I2C_MST_CTRL2_DATA_WIDTH_OFS;
+	mask |= RTL9300_I2C_MST_CTRL2_DATA_WIDTH_MASK;
+
+	mask |= RTL9300_I2C_MST_CTRL2_RD_MODE;
+
+	return regmap_update_bits(i2c->regmap, i2c->reg_base + RTL9300_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->reg_base + RTL9300_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 0;
+}
+
+static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len)
+{
+	u32 vals[4] = {};
+	int i;
+
+	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];
+	}
+
+	return regmap_bulk_write(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0,
+				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);
+}
+
+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;
+
+	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_update_bits(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL1, mask, val);
+	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);
+	if (ret)
+		return ret;
+
+	if (val & RTL9300_I2C_MST_CTRL1_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->reg_base + RTL9300_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->reg_base + RTL9300_I2C_MST_DATA_WORD0, &val);
+			if (ret)
+				return ret;
+			data->word = val & 0xffff;
+		} else {
+			ret = rtl9300_i2c_read(i2c, &data->block[0], len);
+			if (ret)
+				return ret;
+		}
+	}
+
+	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_chan *chan = i2c_get_adapdata(adap);
+	struct rtl9300_i2c *i2c = chan->i2c;
+	int len = 0, ret;
+
+	mutex_lock(&i2c_lock);
+	if (chan->sda_pin != i2c->sda_pin) {
+		ret = rtl9300_i2c_config_io(i2c, chan->sda_pin);
+		if (ret)
+			goto out_unlock;
+		i2c->sda_pin = chan->sda_pin;
+	}
+
+	switch (size) {
+	case I2C_SMBUS_QUICK:
+		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, 0);
+		if (ret)
+			goto out_unlock;
+		ret = rtl9300_i2c_reg_addr_set(i2c, 0, 0);
+		if (ret)
+			goto out_unlock;
+		break;
+
+	case I2C_SMBUS_BYTE:
+		if (read_write == I2C_SMBUS_WRITE) {
+			ret = rtl9300_i2c_config_xfer(i2c, chan, addr, 0);
+			if (ret)
+				goto out_unlock;
+			ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
+			if (ret)
+				goto out_unlock;
+		} else {
+			ret = rtl9300_i2c_config_xfer(i2c, chan, addr, 1);
+			if (ret)
+				goto out_unlock;
+			ret = rtl9300_i2c_reg_addr_set(i2c, 0, 0);
+			if (ret)
+				goto out_unlock;
+		}
+		break;
+
+	case I2C_SMBUS_BYTE_DATA:
+		ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
+		if (ret)
+			goto out_unlock;
+		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, 1);
+		if (ret)
+			goto out_unlock;
+		if (read_write == I2C_SMBUS_WRITE) {
+			ret = rtl9300_i2c_writel(i2c, data->byte);
+			if (ret)
+				goto out_unlock;
+		}
+		break;
+
+	case I2C_SMBUS_WORD_DATA:
+		ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
+		if (ret)
+			goto out_unlock;
+		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, 2);
+		if (ret)
+			goto out_unlock;
+		if (read_write == I2C_SMBUS_WRITE) {
+			ret = rtl9300_i2c_writel(i2c, data->word);
+			if (ret)
+				goto out_unlock;
+		}
+		break;
+
+	case I2C_SMBUS_BLOCK_DATA:
+		ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
+		if (ret)
+			goto out_unlock;
+		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0]);
+		if (ret)
+			goto out_unlock;
+		if (read_write == I2C_SMBUS_WRITE) {
+			ret = rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
+			if (ret)
+				goto out_unlock;
+		}
+		len = data->block[0];
+		break;
+
+	default:
+		dev_err(&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_chan *chan;
+	struct rtl9300_i2c *i2c;
+	struct i2c_adapter *adap;
+	u32 clock_freq, sda_pin;
+	int ret, i = 0;
+	struct fwnode_handle *child;
+
+	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, "reg", &i2c->reg_base);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, i2c);
+
+	if (device_get_child_node_count(dev) >= RTL9300_I2C_MUX_NCHAN)
+		return dev_err_probe(dev, -EINVAL, "Too many channels\n");
+
+	device_for_each_child_node(dev, child) {
+		chan = &i2c->chans[i];
+		adap = &chan->adap;
+
+		ret = fwnode_property_read_u32(child, "reg", &sda_pin);
+		if (ret)
+			return ret;
+
+		ret = fwnode_property_read_u32(child, "clock-frequency", &clock_freq);
+		if (ret)
+			clock_freq = I2C_MAX_STANDARD_MODE_FREQ;
+
+		switch (clock_freq) {
+		case I2C_MAX_STANDARD_MODE_FREQ:
+			chan->bus_freq = RTL9300_I2C_STD_FREQ;
+			break;
+
+		case I2C_MAX_FAST_MODE_FREQ:
+			chan->bus_freq = RTL9300_I2C_FAST_FREQ;
+			break;
+		default:
+			dev_warn(i2c->dev, "SDA%d clock-frequency %d not supported using default\n",
+				 sda_pin, clock_freq);
+			break;
+		}
+
+		chan->sda_pin = sda_pin;
+		chan->i2c = i2c;
+		adap = &i2c->chans[i].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, chan);
+		adap->dev.of_node = to_of_node(child);
+		snprintf(adap->name, sizeof(adap->name), "%s SDA%d\n", dev_name(dev), sda_pin);
+		i++;
+
+		ret = devm_i2c_add_adapter(dev, adap);
+		if (ret)
+			return ret;
+	}
+	i2c->sda_pin = 0xff;
+
+	return 0;
+}
+
+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,
+	.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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 3/3] mips: dts: realtek: Add I2C controllers
  2024-09-23 23:02 [PATCH v3 0/3] i2c: RTL9300 support Chris Packham
  2024-09-23 23:02 ` [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
  2024-09-23 23:02 ` [PATCH v3 2/3] i2c: Add driver for the " Chris Packham
@ 2024-09-23 23:02 ` Chris Packham
  2024-09-24  8:50   ` Krzysztof Kozlowski
  2 siblings, 1 reply; 11+ messages in thread
From: Chris Packham @ 2024-09-23 23:02 UTC (permalink / raw)
  To: andi.shyti, robh, krzk+dt, conor+dt, tsbogend
  Cc: linux-i2c, devicetree, linux-kernel, linux-mips, Chris Packham

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

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

Notes:
    Changes in v2:
    - Use reg property

 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..cc43025cd46c 100644
--- a/arch/mips/boot/dts/realtek/rtl930x.dtsi
+++ b/arch/mips/boot/dts/realtek/rtl930x.dtsi
@@ -33,12 +33,30 @@ lx_clk: clock-175mhz {
 	switch0: switch@1b000000 {
 		compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
 		reg = <0x1b000000 0x10000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
 
 		reboot {
 			compatible = "syscon-reboot";
 			offset = <0x0c>;
 			value = <0x01>;
 		};
+
+		i2c0: i2c@36c {
+			compatible = "realtek,rtl9300-i2c";
+			reg = <0x36c 0x14>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c1: i2c@388 {
+			compatible = "realtek,rtl9300-i2c";
+			reg = <0x388 0x14>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
 	};
 };
 
-- 
2.46.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller
  2024-09-23 23:02 ` [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
@ 2024-09-24  8:46   ` Krzysztof Kozlowski
  2024-09-24  8:55   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-24  8:46 UTC (permalink / raw)
  To: Chris Packham
  Cc: andi.shyti, robh, krzk+dt, conor+dt, tsbogend, linux-i2c,
	devicetree, linux-kernel, linux-mips

On Tue, Sep 24, 2024 at 11:02: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:
>     Changes in v3:
>     - Remove parent node in example
>     - put unevaluatedProperties after required
>     - Add #address-cells and #size-cells
>     
>     Changes in v2:
>     - Use reg property for controller registers
>     - Remove global-control-offset (will be hard coded in driver)
>     - Integrated the multiplexing function. Child nodes now represent the
>       available SDA lines
> 
>  .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 80 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 86 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..979ec22e81f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> @@ -0,0 +1,80 @@
> +# 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

You need SoC-specific compatibles in the front followed by this
fallback. For each SoC.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/3] mips: dts: realtek: Add I2C controllers
  2024-09-23 23:02 ` [PATCH v3 3/3] mips: dts: realtek: Add I2C controllers Chris Packham
@ 2024-09-24  8:50   ` Krzysztof Kozlowski
  2024-09-24 21:23     ` Chris Packham
  0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-24  8:50 UTC (permalink / raw)
  To: Chris Packham
  Cc: andi.shyti, robh, krzk+dt, conor+dt, tsbogend, linux-i2c,
	devicetree, linux-kernel, linux-mips

On Tue, Sep 24, 2024 at 11:02:30AM +1200, Chris Packham wrote:
> Add the I2C controllers that are part of the RTL9300 SoC.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Use reg property
> 
>  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..cc43025cd46c 100644
> --- a/arch/mips/boot/dts/realtek/rtl930x.dtsi
> +++ b/arch/mips/boot/dts/realtek/rtl930x.dtsi
> @@ -33,12 +33,30 @@ lx_clk: clock-175mhz {
>  	switch0: switch@1b000000 {
>  		compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
>  		reg = <0x1b000000 0x10000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
>  
>  		reboot {
>  			compatible = "syscon-reboot";
>  			offset = <0x0c>;
>  			value = <0x01>;
>  		};
> +
> +		i2c0: i2c@36c {
> +			compatible = "realtek,rtl9300-i2c";
> +			reg = <0x36c 0x14>;
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};

This should be sent as one series... You have dependency otherwise. Also
this points to issue of mixing nodes with and without unit address.

I think i2c children should be under some sort of "i2c" bus node.

Please propose entire realtek,rtl9302c-switch binding with the I2C. It's
very confusing to see it partial.


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller
  2024-09-23 23:02 ` [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
  2024-09-24  8:46   ` Krzysztof Kozlowski
@ 2024-09-24  8:55   ` Krzysztof Kozlowski
  2024-09-24 21:11     ` Chris Packham
  1 sibling, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-24  8:55 UTC (permalink / raw)
  To: Chris Packham
  Cc: andi.shyti, robh, krzk+dt, conor+dt, tsbogend, linux-i2c,
	devicetree, linux-kernel, linux-mips

On Tue, Sep 24, 2024 at 11:02: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:
>     Changes in v3:
>     - Remove parent node in example
>     - put unevaluatedProperties after required
>     - Add #address-cells and #size-cells
>     
>     Changes in v2:
>     - Use reg property for controller registers
>     - Remove global-control-offset (will be hard coded in driver)
>     - Integrated the multiplexing function. Child nodes now represent the
>       available SDA lines
> 
>  .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 80 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 86 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..979ec22e81f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> @@ -0,0 +1,80 @@
> +# 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

This is confusing. It took me some minutes to understand that two I2Cs
in the example do not match what you wrote here. I got there only because
of your DTS. Your patchsets - sent separately and describing problem
incompletely - do not help.

This is the binding for I2C, not for RTL9300 SoC.

> +  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
> +
> +  reg:
> +    description: Register offset and size this I2C controller.

Nope, your reboot mode does not have reg. Either fix reboot mode driver
or this. Preferably reboot mode.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller
  2024-09-24  8:55   ` Krzysztof Kozlowski
@ 2024-09-24 21:11     ` Chris Packham
  2024-09-25  7:19       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Chris Packham @ 2024-09-24 21:11 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: andi.shyti@kernel.org, robh@kernel.org, krzk+dt@kernel.org,
	conor+dt@kernel.org, tsbogend@alpha.franken.de,
	linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org


On 24/09/24 20:55, Krzysztof Kozlowski wrote:
> On Tue, Sep 24, 2024 at 11:02: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:
>>      Changes in v3:
>>      - Remove parent node in example
>>      - put unevaluatedProperties after required
>>      - Add #address-cells and #size-cells
>>      
>>      Changes in v2:
>>      - Use reg property for controller registers
>>      - Remove global-control-offset (will be hard coded in driver)
>>      - Integrated the multiplexing function. Child nodes now represent the
>>        available SDA lines
>>
>>   .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 80 +++++++++++++++++++
>>   MAINTAINERS                                   |  6 ++
>>   2 files changed, 86 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..979ec22e81f1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
>> @@ -0,0 +1,80 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://scanmail.trustwave.com/?c=20988&d=gf7y5rgy9ER44HZptkkovLbVGtkYd7ByAz3K6PNPAw&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fi2c%2frealtek%2crtl9300-i2c%2eyaml%23
>> +$schema: http://scanmail.trustwave.com/?c=20988&d=gf7y5rgy9ER44HZptkkovLbVGtkYd7ByA2-a7KJKBg&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>> +
>> +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
> This is confusing. It took me some minutes to understand that two I2Cs
> in the example do not match what you wrote here. I got there only because
> of your DTS. Your patchsets - sent separately and describing problem
> incompletely - do not help.
>
> This is the binding for I2C, not for RTL9300 SoC.
>
>> +  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
>> +
>> +  reg:
>> +    description: Register offset and size this I2C controller.
> Nope, your reboot mode does not have reg. Either fix reboot mode driver
> or this. Preferably reboot mode.

I'm not sure what you mean by this. The syscon-reboot binding doesn't 
require a reg property and I can only find one in-tree dts 
(turris1x.dts) that actually gives it a reg property.

>
> Best regards,
> Krzysztof
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/3] mips: dts: realtek: Add I2C controllers
  2024-09-24  8:50   ` Krzysztof Kozlowski
@ 2024-09-24 21:23     ` Chris Packham
  0 siblings, 0 replies; 11+ messages in thread
From: Chris Packham @ 2024-09-24 21:23 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: andi.shyti@kernel.org, robh@kernel.org, krzk+dt@kernel.org,
	conor+dt@kernel.org, tsbogend@alpha.franken.de,
	linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org


On 24/09/24 20:50, Krzysztof Kozlowski wrote:
> On Tue, Sep 24, 2024 at 11:02:30AM +1200, Chris Packham wrote:
>> Add the I2C controllers that are part of the RTL9300 SoC.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      Changes in v2:
>>      - Use reg property
>>
>>   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..cc43025cd46c 100644
>> --- a/arch/mips/boot/dts/realtek/rtl930x.dtsi
>> +++ b/arch/mips/boot/dts/realtek/rtl930x.dtsi
>> @@ -33,12 +33,30 @@ lx_clk: clock-175mhz {
>>   	switch0: switch@1b000000 {
>>   		compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
>>   		reg = <0x1b000000 0x10000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>>   
>>   		reboot {
>>   			compatible = "syscon-reboot";
>>   			offset = <0x0c>;
>>   			value = <0x01>;
>>   		};
>> +
>> +		i2c0: i2c@36c {
>> +			compatible = "realtek,rtl9300-i2c";
>> +			reg = <0x36c 0x14>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +		};
> This should be sent as one series... You have dependency otherwise. Also
> this points to issue of mixing nodes with and without unit address.
>
> I think i2c children should be under some sort of "i2c" bus node.

something like this?

switch@1b000000 {
    i2c-controller {
      i2c-mst1 {
          status = "okay";
          i2c@0 {
             reg = <0>;
             gpio@20 {
                 reg = <0x20>;
             };
          };
          i2c@2 {
             reg = <2>;
             gpio@20 {
                 reg = <0x20>;
             };
          };
      };
      i2c-mst2 {
          status = "disabled";
      };
   };
};

> Please propose entire realtek,rtl9302c-switch binding with the I2C. It's
> very confusing to see it partial.
Yep will combine these series.
>
>
> Best regards,
> Krzysztof
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller
  2024-09-24 21:11     ` Chris Packham
@ 2024-09-25  7:19       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-25  7:19 UTC (permalink / raw)
  To: Chris Packham
  Cc: andi.shyti@kernel.org, robh@kernel.org, krzk+dt@kernel.org,
	conor+dt@kernel.org, tsbogend@alpha.franken.de,
	linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org

On 24/09/2024 23:11, Chris Packham wrote:
> 
> On 24/09/24 20:55, Krzysztof Kozlowski wrote:
>> On Tue, Sep 24, 2024 at 11:02: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:
>>>      Changes in v3:
>>>      - Remove parent node in example
>>>      - put unevaluatedProperties after required
>>>      - Add #address-cells and #size-cells
>>>      
>>>      Changes in v2:
>>>      - Use reg property for controller registers
>>>      - Remove global-control-offset (will be hard coded in driver)
>>>      - Integrated the multiplexing function. Child nodes now represent the
>>>        available SDA lines
>>>
>>>   .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 80 +++++++++++++++++++
>>>   MAINTAINERS                                   |  6 ++
>>>   2 files changed, 86 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..979ec22e81f1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
>>> @@ -0,0 +1,80 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://scanmail.trustwave.com/?c=20988&d=gf7y5rgy9ER44HZptkkovLbVGtkYd7ByAz3K6PNPAw&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fi2c%2frealtek%2crtl9300-i2c%2eyaml%23
>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=gf7y5rgy9ER44HZptkkovLbVGtkYd7ByA2-a7KJKBg&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>> +
>>> +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
>> This is confusing. It took me some minutes to understand that two I2Cs
>> in the example do not match what you wrote here. I got there only because
>> of your DTS. Your patchsets - sent separately and describing problem
>> incompletely - do not help.
>>
>> This is the binding for I2C, not for RTL9300 SoC.
>>
>>> +  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
>>> +
>>> +  reg:
>>> +    description: Register offset and size this I2C controller.
>> Nope, your reboot mode does not have reg. Either fix reboot mode driver
>> or this. Preferably reboot mode.
> 
> I'm not sure what you mean by this. The syscon-reboot binding doesn't 
> require a reg property and I can only find one in-tree dts 
> (turris1x.dts) that actually gives it a reg property.

But it could. It uses offset instead of reg for historical reasons, when
it was outside of syscon node. I propose to add there reg and oneOf
requiring either reg or offset. This way you will have only MMIO children.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] i2c: Add driver for the RTL9300 I2C controller
  2024-09-23 23:02 ` [PATCH v3 2/3] i2c: Add driver for the " Chris Packham
@ 2024-09-25  9:35   ` Andi Shyti
  0 siblings, 0 replies; 11+ messages in thread
From: Andi Shyti @ 2024-09-25  9:35 UTC (permalink / raw)
  To: Chris Packham
  Cc: robh, krzk+dt, conor+dt, tsbogend, linux-i2c, devicetree,
	linux-kernel, linux-mips

Hi Chris,

...

> +enum rtl9300_bus_freq {
> +	RTL9300_I2C_STD_FREQ,
> +	RTL9300_I2C_FAST_FREQ,
> +};
> +
> +struct rtl9300_i2c;
> +
> +struct rtl9300_i2c_chan {
> +	struct i2c_adapter adap;
> +	struct rtl9300_i2c *i2c;
> +	enum rtl9300_bus_freq bus_freq;
> +	u8 sda_pin;
> +};
> +
> +#define RTL9300_I2C_MUX_NCHAN	8
> +
> +struct rtl9300_i2c {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct rtl9300_i2c_chan chans[RTL9300_I2C_MUX_NCHAN];
> +	u32 reg_base;
> +	u8 sda_pin;
> +};

...

> +DEFINE_MUTEX(i2c_lock);

why is this lock defined here and not inside rtl9300_i2c?

...

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

...

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

your alignments are a bit off.

A part from these two little notes, I don't see any issue here.

Thanks,
Andi

> +{
> +	struct rtl9300_i2c_chan *chan = i2c_get_adapdata(adap);
> +	struct rtl9300_i2c *i2c = chan->i2c;
> +	int len = 0, ret;

...

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-09-25  9:35 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-23 23:02 [PATCH v3 0/3] i2c: RTL9300 support Chris Packham
2024-09-23 23:02 ` [PATCH v3 1/3] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
2024-09-24  8:46   ` Krzysztof Kozlowski
2024-09-24  8:55   ` Krzysztof Kozlowski
2024-09-24 21:11     ` Chris Packham
2024-09-25  7:19       ` Krzysztof Kozlowski
2024-09-23 23:02 ` [PATCH v3 2/3] i2c: Add driver for the " Chris Packham
2024-09-25  9:35   ` Andi Shyti
2024-09-23 23:02 ` [PATCH v3 3/3] mips: dts: realtek: Add I2C controllers Chris Packham
2024-09-24  8:50   ` Krzysztof Kozlowski
2024-09-24 21:23     ` Chris Packham

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).