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

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

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
>