All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2 v3] dt-bindings: i2c: i2c-mt7621: Add bindings for MediaTek MT7621/28/88 I2C
@ 2019-06-04 11:34 Stefan Roese
  2019-06-04 11:34 ` [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver Stefan Roese
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Roese @ 2019-06-04 11:34 UTC (permalink / raw)
  To: linux-i2c; +Cc: devicetree, Rob Herring, Wolfram Sang

Add bindings for the I2C controller that can be found in the MediaTek
MT7621/7628/7688 SoCs.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Rob Herring <robh@kernel.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
v3:
- New patch

 .../devicetree/bindings/i2c/i2c-mt7621.txt    | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mt7621.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mt7621.txt b/Documentation/devicetree/bindings/i2c/i2c-mt7621.txt
new file mode 100644
index 000000000000..bc36f0eb94cd
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mt7621.txt
@@ -0,0 +1,25 @@
+MediaTek MT7621/MT7628 I2C master controller
+
+Required properties:
+
+- compatible: Should be one of the following:
+  - "mediatek,mt7621-i2c": for MT7621/MT7628/MT7688 platforms
+- #address-cells: should be 1.
+- #size-cells: should be 0.
+- reg: Address and length of the register set for the device
+- resets: phandle to the reset controller asserting this device in
+          reset
+  See ../reset/reset.txt for details.
+
+Optional properties :
+
+Example:
+
+i2c: i2c@900 {
+	compatible = "mediatek,mt7621-i2c";
+	reg = <0x900 0x100>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	resets = <&rstctrl 16>;
+	reset-names = "i2c";
+};
-- 
2.21.0

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

* [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver
  2019-06-04 11:34 [PATCH 1/2 v3] dt-bindings: i2c: i2c-mt7621: Add bindings for MediaTek MT7621/28/88 I2C Stefan Roese
@ 2019-06-04 11:34 ` Stefan Roese
  2019-06-04 13:00   ` Jan Breuer
                     ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Stefan Roese @ 2019-06-04 11:34 UTC (permalink / raw)
  To: linux-i2c
  Cc: devicetree, Jan Breuer, John Crispin, René van Dorst, Wolfram Sang

This patch adds a driver for the I2C controller found on the MediaTek
MT7621/7628/7688 SoC's. The base version of this driver was done by
Steven Liu (according to the copyright and MODULE_AUTHOR lines). It
can be found in the OpenWRT repositories (v4.14 at the time I looked).

The base driver had many issues, which are disccussed here:

https://en.forum.labs.mediatek.com/t/openwrt-15-05-loads-non-working-i2c-kernel-module-for-mt7688/1286/3

>From this link an enhanced driver version (complete rewrite, mayor
changes: support clock stretching, repeated start, ACK handling and
unlimited message length) from Jan Breuer can be found here:

https://gist.github.com/j123b567/9b555b635c2b4069d716b24198546954

This patch now adds this enhanced I2C driver to mainline.

Changes by Stefan Roese for upstreaming:
- Add devicetree bindings
- checkpatch clean
- Use module_platform_driver()
- Minor cosmetic enhancements
- Removed IO warpped functions
- Use readl_relaxed_poll_timeout() and drop poll_down_timeout()
- Removed superfluous barrier() in mtk_i2c_reset()
- Use i2c_8bit_addr_from_msg()
- Added I2C_FUNC_PROTOCOL_MANGLING
- Removed adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Jan Breuer <jan.breuer@jaybee.cz>
Cc: John Crispin <john@phrozen.org>
Cc: René van Dorst <opensource@vdorst.com>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
v3:
- Added myself as maintainer for this driver
- Added René van Dorst to Cc as he tested an earlier version of this
  driver. The tested-by tag is not added, as the driver has now
  undergone some notable changes. A new testing would be very welcome.
- Removed Steven Liu from Cc, as his email address is not valid any
  more
- Moved bindings documentation into separate patch
- Removed IO warpped functions
- Use readl_relaxed_poll_timeout() and drop poll_down_timeout()
- Removed superfluous barrier() in mtk_i2c_reset()
- Use i2c_8bit_addr_from_msg()
- Added I2C_FUNC_PROTOCOL_MANGLING
- Removed adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
- Misc minor mostly cosmitic enhancements suggested by Wolfram

v2:
- Configure I2C controller to open-drain instead of push-pull, as
  noticed and suggested by Jan (misleading bit description)

 MAINTAINERS                     |   7 +
 drivers/i2c/busses/Kconfig      |   8 +
 drivers/i2c/busses/Makefile     |   1 +
 drivers/i2c/busses/i2c-mt7621.c | 346 ++++++++++++++++++++++++++++++++
 4 files changed, 362 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-mt7621.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a6954776a37e..8472dfbffcfa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9957,6 +9957,13 @@ L:	linux-wireless@vger.kernel.org
 S:	Maintained
 F:	drivers/net/wireless/mediatek/mt7601u/
 
+MEDIATEK MT7621/28/88 I2C DRIVER
+M:	Stefan Roese <sr@denx.de>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	drivers/i2c/busses/i2c-mt7621.c
+F:	Documentation/devicetree/bindings/i2c/i2c-mt7621.txt
+
 MEDIATEK NAND CONTROLLER DRIVER
 M:	Xiaolei Li <xiaolei.li@mediatek.com>
 L:	linux-mtd@lists.infradead.org
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index ee5dfb5aee2a..f9ae8bcd4d8b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -746,6 +746,14 @@ config I2C_MT65XX
 	  If you want to use MediaTek(R) I2C interface, say Y or M here.
 	  If unsure, say N.
 
+config I2C_MT7621
+	tristate "MT7621/MT7628 I2C Controller"
+	depends on (RALINK && (SOC_MT7620 || SOC_MT7621)) || COMPILE_TEST
+	select OF_I2C
+	help
+	  Say Y here to include support for I2C controller in the
+	  MediaTek MT7621/MT7628 SoCs.
+
 config I2C_MV64XXX
 	tristate "Marvell mv64xxx I2C Controller"
 	depends on MV64X60 || PLAT_ORION || ARCH_SUNXI || ARCH_MVEBU
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index a3245231b0b7..80c23895eaaf 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_I2C_LPC2K)		+= i2c-lpc2k.o
 obj-$(CONFIG_I2C_MESON)		+= i2c-meson.o
 obj-$(CONFIG_I2C_MPC)		+= i2c-mpc.o
 obj-$(CONFIG_I2C_MT65XX)	+= i2c-mt65xx.o
+obj-$(CONFIG_I2C_MT7621)	+= i2c-mt7621.o
 obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
diff --git a/drivers/i2c/busses/i2c-mt7621.c b/drivers/i2c/busses/i2c-mt7621.c
new file mode 100644
index 000000000000..ff8f8c80aa27
--- /dev/null
+++ b/drivers/i2c/busses/i2c-mt7621.c
@@ -0,0 +1,346 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * drivers/i2c/busses/i2c-mt7621.c
+ *
+ * Copyright (C) 2013 Steven Liu <steven_liu@mediatek.com>
+ * Copyright (C) 2016 Michael Lee <igvtee@gmail.com>
+ * Copyright (C) 2018 Jan Breuer <jan.breuer@jaybee.cz>
+ *
+ * Improve driver for i2cdetect from i2c-tools to detect i2c devices on the bus.
+ * (C) 2014 Sittisak <sittisaks@hotmail.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/reset.h>
+
+#define REG_SM0CFG2_REG		0x28
+#define REG_SM0CTL0_REG		0x40
+#define REG_SM0CTL1_REG		0x44
+#define REG_SM0D0_REG		0x50
+#define REG_SM0D1_REG		0x54
+#define REG_PINTEN_REG		0x5c
+#define REG_PINTST_REG		0x60
+#define REG_PINTCL_REG		0x64
+
+/* REG_SM0CFG2_REG */
+#define SM0CFG2_IS_AUTOMODE	BIT(0)
+
+/* REG_SM0CTL0_REG */
+#define SM0CTL0_ODRAIN		BIT(31)
+#define SM0CTL0_CLK_DIV_MASK	(0x7ff << 16)
+#define SM0CTL0_CLK_DIV_MAX	0x7ff
+#define SM0CTL0_CS_STATUS       BIT(4)
+#define SM0CTL0_SCL_STATE       BIT(3)
+#define SM0CTL0_SDA_STATE       BIT(2)
+#define SM0CTL0_EN              BIT(1)
+#define SM0CTL0_SCL_STRETCH     BIT(0)
+
+/* REG_SM0CTL1_REG */
+#define SM0CTL1_ACK_MASK	(0xff << 16)
+#define SM0CTL1_PGLEN_MASK	(0x7 << 8)
+#define SM0CTL1_PGLEN(x)	((((x) - 1) << 8) & SM0CTL1_PGLEN_MASK)
+#define SM0CTL1_READ		(5 << 4)
+#define SM0CTL1_READ_LAST	(4 << 4)
+#define SM0CTL1_STOP		(3 << 4)
+#define SM0CTL1_WRITE		(2 << 4)
+#define SM0CTL1_START		(1 << 4)
+#define SM0CTL1_MODE_MASK	(0x7 << 4)
+#define SM0CTL1_TRI		BIT(0)
+
+/* timeout waiting for I2C devices to respond */
+#define TIMEOUT_MS		1000
+
+#define MT76XX_I2C_INPUT_CLOCK	40000000
+
+struct mtk_i2c {
+	void __iomem *base;
+	struct device *dev;
+	struct i2c_adapter adap;
+	u32 bus_freq;
+	u32 clk_div;
+	u32 flags;
+};
+
+static int mtk_i2c_wait_idle(struct mtk_i2c *i2c)
+{
+	int ret;
+	u32 val;
+
+	ret = readl_relaxed_poll_timeout(i2c->base + REG_SM0CTL1_REG,
+					 val, !(val & SM0CTL1_TRI),
+					 10, TIMEOUT_MS * 1000);
+	if (ret)
+		dev_dbg(i2c->dev, "idle err(%d)\n", ret);
+
+	return ret;
+}
+
+static void mtk_i2c_reset(struct mtk_i2c *i2c)
+{
+	int ret;
+
+	ret = device_reset(i2c->adap.dev.parent);
+	if (ret)
+		dev_err(i2c->dev, "I2C reset failed!\n");
+
+	/*
+	 * Don't set SM0CTL0_ODRAIN as its bit meaning is inverted. To
+	 * configure open-drain mode, this bit needs to be cleared.
+	 */
+	iowrite32(((i2c->clk_div << 16) & SM0CTL0_CLK_DIV_MASK) | SM0CTL0_EN |
+		  SM0CTL0_SCL_STRETCH, i2c->base + REG_SM0CTL0_REG);
+	iowrite32(0, i2c->base + REG_SM0CFG2_REG);
+}
+
+static void mtk_i2c_dump_reg(struct mtk_i2c *i2c)
+{
+	dev_dbg(i2c->dev,
+		"SM0CFG2 %08x, SM0CTL0 %08x, SM0CTL1 %08x, SM0D0 %08x, SM0D1 %08x\n",
+		ioread32(i2c->base + REG_SM0CFG2_REG),
+		ioread32(i2c->base + REG_SM0CTL0_REG),
+		ioread32(i2c->base + REG_SM0CTL1_REG),
+		ioread32(i2c->base + REG_SM0D0_REG),
+		ioread32(i2c->base + REG_SM0D1_REG));
+}
+
+static int mtk_i2c_check_ack(struct mtk_i2c *i2c, u32 expected)
+{
+	u32 ack = readl_relaxed(i2c->base + REG_SM0CTL1_REG);
+	u32 ack_expected = (expected << 16) & SM0CTL1_ACK_MASK;
+
+	return ((ack & ack_expected) == ack_expected) ? 0 : -ENXIO;
+}
+
+static int mtk_i2c_master_start(struct mtk_i2c *i2c)
+{
+	iowrite32(SM0CTL1_START | SM0CTL1_TRI, i2c->base + REG_SM0CTL1_REG);
+	return mtk_i2c_wait_idle(i2c);
+}
+
+static int mtk_i2c_master_stop(struct mtk_i2c *i2c)
+{
+	iowrite32(SM0CTL1_STOP | SM0CTL1_TRI, i2c->base + REG_SM0CTL1_REG);
+	return mtk_i2c_wait_idle(i2c);
+}
+
+static int mtk_i2c_master_cmd(struct mtk_i2c *i2c, u32 cmd, int page_len)
+{
+	iowrite32(cmd | SM0CTL1_TRI | SM0CTL1_PGLEN(page_len),
+		  i2c->base + REG_SM0CTL1_REG);
+	return mtk_i2c_wait_idle(i2c);
+}
+
+static int mtk_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			       int num)
+{
+	struct mtk_i2c *i2c;
+	struct i2c_msg *pmsg;
+	u16 addr;
+	int i, j, ret, len, page_len;
+	u32 cmd;
+	u32 data[2];
+
+	i2c = i2c_get_adapdata(adap);
+
+	for (i = 0; i < num; i++) {
+		pmsg = &msgs[i];
+
+		/* wait hardware idle */
+		ret = mtk_i2c_wait_idle(i2c);
+		if (ret)
+			goto err_timeout;
+
+		/* start sequence */
+		ret = mtk_i2c_master_start(i2c);
+		if (ret)
+			goto err_timeout;
+
+		/* write address */
+		if (pmsg->flags & I2C_M_TEN) {
+			/* 10 bits address */
+			addr = 0xf0 | ((pmsg->addr >> 7) & 0x06);
+			addr |= (pmsg->addr & 0xff) << 8;
+			if (pmsg->flags & I2C_M_RD)
+				addr |= 1;
+			iowrite32(addr, i2c->base + REG_SM0D0_REG);
+			ret = mtk_i2c_master_cmd(i2c, SM0CTL1_WRITE, 2);
+			if (ret)
+				goto err_timeout;
+		} else {
+			/* 7 bits address */
+			addr = i2c_8bit_addr_from_msg(pmsg);
+			iowrite32(addr, i2c->base + REG_SM0D0_REG);
+			ret = mtk_i2c_master_cmd(i2c, SM0CTL1_WRITE, 1);
+			if (ret)
+				goto err_timeout;
+		}
+
+		/* check address ACK */
+		if (!(pmsg->flags & I2C_M_IGNORE_NAK)) {
+			ret = mtk_i2c_check_ack(i2c, BIT(0));
+			if (ret)
+				goto err_ack;
+		}
+
+		/* transfer data */
+		for (len = pmsg->len, j = 0; len > 0; len -= 8, j += 8) {
+			page_len = (len >= 8) ? 8 : len;
+
+			if (pmsg->flags & I2C_M_RD) {
+				cmd = (len > 8) ?
+					SM0CTL1_READ : SM0CTL1_READ_LAST;
+			} else {
+				memcpy(data, &pmsg->buf[j], page_len);
+				iowrite32(data[0], i2c->base + REG_SM0D0_REG);
+				iowrite32(data[1], i2c->base + REG_SM0D1_REG);
+				cmd = SM0CTL1_WRITE;
+			}
+
+			ret = mtk_i2c_master_cmd(i2c, cmd, page_len);
+			if (ret)
+				goto err_timeout;
+
+			if (pmsg->flags & I2C_M_RD) {
+				data[0] = ioread32(i2c->base + REG_SM0D0_REG);
+				data[1] = ioread32(i2c->base + REG_SM0D1_REG);
+				memcpy(&pmsg->buf[j], data, page_len);
+			} else {
+				if (!(pmsg->flags & I2C_M_IGNORE_NAK)) {
+					ret = mtk_i2c_check_ack(i2c,
+								(1 << page_len)
+								- 1);
+					if (ret)
+						goto err_ack;
+				}
+			}
+		}
+	}
+
+	ret = mtk_i2c_master_stop(i2c);
+	if (ret)
+		goto err_timeout;
+
+	/* the return value is number of executed messages */
+	return i;
+
+err_ack:
+	ret = mtk_i2c_master_stop(i2c);
+	if (ret)
+		goto err_timeout;
+	return -ENXIO;
+
+err_timeout:
+	mtk_i2c_dump_reg(i2c);
+	mtk_i2c_reset(i2c);
+	return ret;
+}
+
+static u32 mtk_i2c_func(struct i2c_adapter *a)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING;
+}
+
+static const struct i2c_algorithm mtk_i2c_algo = {
+	.master_xfer	= mtk_i2c_master_xfer,
+	.functionality	= mtk_i2c_func,
+};
+
+static const struct of_device_id i2c_mtk_dt_ids[] = {
+	{ .compatible = "mediatek,mt7621-i2c" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, i2c_mtk_dt_ids);
+
+static void mtk_i2c_init(struct mtk_i2c *i2c)
+{
+	i2c->clk_div = MT76XX_I2C_INPUT_CLOCK / i2c->bus_freq - 1;
+	if (i2c->clk_div < 99)
+		i2c->clk_div = 99;
+	if (i2c->clk_div > SM0CTL0_CLK_DIV_MAX)
+		i2c->clk_div = SM0CTL0_CLK_DIV_MAX;
+
+	mtk_i2c_reset(i2c);
+}
+
+static int mtk_i2c_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct mtk_i2c *i2c;
+	struct i2c_adapter *adap;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	i2c = devm_kzalloc(&pdev->dev, sizeof(struct mtk_i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(i2c->base))
+		return PTR_ERR(i2c->base);
+
+	i2c->dev = &pdev->dev;
+
+	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				 &i2c->bus_freq))
+		i2c->bus_freq = 100000;
+
+	if (i2c->bus_freq == 0) {
+		dev_warn(i2c->dev,
+			 "clock-frequency 0 not supported, using 1\n");
+		i2c->bus_freq = 1;
+	}
+
+	adap = &i2c->adap;
+	adap->owner = THIS_MODULE;
+	adap->algo = &mtk_i2c_algo;
+	adap->retries = 3;
+	adap->dev.parent = &pdev->dev;
+	i2c_set_adapdata(adap, i2c);
+	adap->dev.of_node = pdev->dev.of_node;
+	strlcpy(adap->name, dev_name(&pdev->dev), sizeof(adap->name));
+
+	platform_set_drvdata(pdev, i2c);
+
+	mtk_i2c_init(i2c);
+
+	ret = i2c_add_adapter(adap);
+	if (ret < 0)
+		return ret;
+
+	dev_info(&pdev->dev, "clock %u kHz\n", i2c->bus_freq / 1000);
+
+	return ret;
+}
+
+static int mtk_i2c_remove(struct platform_device *pdev)
+{
+	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adap);
+
+	return 0;
+}
+
+static struct platform_driver mtk_i2c_driver = {
+	.probe		= mtk_i2c_probe,
+	.remove		= mtk_i2c_remove,
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "i2c-mt7621",
+		.of_match_table = i2c_mtk_dt_ids,
+	},
+};
+
+module_platform_driver(mtk_i2c_driver);
+
+MODULE_AUTHOR("Steven Liu <steven_liu@mediatek.com>");
+MODULE_DESCRIPTION("MT7621 I2C host driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:MT7621-I2C");
-- 
2.21.0

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

* Re: [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver
  2019-06-04 11:34 ` [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver Stefan Roese
@ 2019-06-04 13:00   ` Jan Breuer
  2019-06-04 14:03     ` Stefan Roese
  2019-06-04 19:52   ` René van Dorst
  2019-06-14 21:40   ` Wolfram Sang
  2 siblings, 1 reply; 7+ messages in thread
From: Jan Breuer @ 2019-06-04 13:00 UTC (permalink / raw)
  To: Stefan Roese
  Cc: linux-i2c, devicetree, Jan Breuer, John Crispin,
	René van Dorst, Wolfram Sang

Hi Stefan,

> +#define MT76XX_I2C_INPUT_CLOCK 40000000

This is peripheral clock and should be reachable by devm_clk_get() and
later clk_get_rate() should give value similar to
MT76XX_I2C_INPUT_CLOCK.
I don't have currently recent enough kernel to test it or prove it,
but I see this in openwrt I2C driver for this platform
https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/patches-4.14/0045-i2c-add-mt7621-driver.patch

Kind regards,
Jan

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

* Re: [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver
  2019-06-04 13:00   ` Jan Breuer
@ 2019-06-04 14:03     ` Stefan Roese
  2019-06-04 14:42       ` Jan Breuer
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Roese @ 2019-06-04 14:03 UTC (permalink / raw)
  To: Jan Breuer
  Cc: linux-i2c, devicetree, John Crispin, René van Dorst, Wolfram Sang

Hi Jan,

On 04.06.19 15:00, Jan Breuer wrote:
> Hi Stefan,
> 
>> +#define MT76XX_I2C_INPUT_CLOCK 40000000
> 
> This is peripheral clock and should be reachable by devm_clk_get() and
> later clk_get_rate() should give value similar to
> MT76XX_I2C_INPUT_CLOCK.

Unfortunately not (yet) in mainline Linux.

> I don't have currently recent enough kernel to test it or prove it,
> but I see this in openwrt I2C driver for this platform
> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/patches-4.14/0045-i2c-add-mt7621-driver.patch

Yes, its available in the OpenWRT v4.14 tree, but not in mainline
Linux (AFAICT).

I might try to make some time to add this clock driver for these
SoC's. But I would prefer to use the hardcoded define in this
driver for now, to not block its usage in mainline any longer.

Thanks,
Stefan

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

* Re: [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver
  2019-06-04 14:03     ` Stefan Roese
@ 2019-06-04 14:42       ` Jan Breuer
  0 siblings, 0 replies; 7+ messages in thread
From: Jan Breuer @ 2019-06-04 14:42 UTC (permalink / raw)
  To: Stefan Roese
  Cc: Jan Breuer, linux-i2c, devicetree, John Crispin,
	René van Dorst, Wolfram Sang

Hi Stefan,

On 04.06.19 16:03, Stefan Roese wrote:
>
> Hi Jan,
>
> On 04.06.19 15:00, Jan Breuer wrote:
> > Hi Stefan,
> >
> >> +#define MT76XX_I2C_INPUT_CLOCK 40000000
> >
> > This is peripheral clock and should be reachable by devm_clk_get() and
> > later clk_get_rate() should give value similar to
> > MT76XX_I2C_INPUT_CLOCK.
>
> Unfortunately not (yet) in mainline Linux.

Ou, I didn't notice.

> > I don't have currently recent enough kernel to test it or prove it,
> > but I see this in openwrt I2C driver for this platform
> > https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/patches-4.14/0045-i2c-add-mt7621-driver.patch
>
> Yes, its available in the OpenWRT v4.14 tree, but not in mainline
> Linux (AFAICT).
>
> I might try to make some time to add this clock driver for these
> SoC's. But I would prefer to use the hardcoded define in this
> driver for now, to not block its usage in mainline any longer.

So it definitely makes sense to hardcode it for now.

Best regards,
Jan Breuer

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

* Re: [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver
  2019-06-04 11:34 ` [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver Stefan Roese
  2019-06-04 13:00   ` Jan Breuer
@ 2019-06-04 19:52   ` René van Dorst
  2019-06-14 21:40   ` Wolfram Sang
  2 siblings, 0 replies; 7+ messages in thread
From: René van Dorst @ 2019-06-04 19:52 UTC (permalink / raw)
  To: Stefan Roese
  Cc: linux-i2c, devicetree, Jan Breuer, John Crispin, Wolfram Sang

Quoting Stefan Roese <sr@denx.de>:

> This patch adds a driver for the I2C controller found on the MediaTek
> MT7621/7628/7688 SoC's. The base version of this driver was done by
> Steven Liu (according to the copyright and MODULE_AUTHOR lines). It
> can be found in the OpenWRT repositories (v4.14 at the time I looked).
>
> The base driver had many issues, which are disccussed here:
>
> https://en.forum.labs.mediatek.com/t/openwrt-15-05-loads-non-working-i2c-kernel-module-for-mt7688/1286/3
>
> From this link an enhanced driver version (complete rewrite, mayor
> changes: support clock stretching, repeated start, ACK handling and
> unlimited message length) from Jan Breuer can be found here:
>
> https://gist.github.com/j123b567/9b555b635c2b4069d716b24198546954
>
> This patch now adds this enhanced I2C driver to mainline.
>
> Changes by Stefan Roese for upstreaming:
> - Add devicetree bindings
> - checkpatch clean
> - Use module_platform_driver()
> - Minor cosmetic enhancements
> - Removed IO warpped functions
> - Use readl_relaxed_poll_timeout() and drop poll_down_timeout()
> - Removed superfluous barrier() in mtk_i2c_reset()
> - Use i2c_8bit_addr_from_msg()
> - Added I2C_FUNC_PROTOCOL_MANGLING
> - Removed adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Jan Breuer <jan.breuer@jaybee.cz>
> Cc: John Crispin <john@phrozen.org>
> Cc: René van Dorst <opensource@vdorst.com>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> v3:
> - Added myself as maintainer for this driver
> - Added René van Dorst to Cc as he tested an earlier version of this
>   driver. The tested-by tag is not added, as the driver has now
>   undergone some notable changes. A new testing would be very welcome.
> - Removed Steven Liu from Cc, as his email address is not valid any
>   more
> - Moved bindings documentation into separate patch
> - Removed IO warpped functions
> - Use readl_relaxed_poll_timeout() and drop poll_down_timeout()
> - Removed superfluous barrier() in mtk_i2c_reset()
> - Use i2c_8bit_addr_from_msg()
> - Added I2C_FUNC_PROTOCOL_MANGLING
> - Removed adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> - Misc minor mostly cosmitic enhancements suggested by Wolfram
>
> v2:
> - Configure I2C controller to open-drain instead of push-pull, as
>   noticed and suggested by Jan (misleading bit description)
>
>  MAINTAINERS                     |   7 +
>  drivers/i2c/busses/Kconfig      |   8 +
>  drivers/i2c/busses/Makefile     |   1 +
>  drivers/i2c/busses/i2c-mt7621.c | 346 ++++++++++++++++++++++++++++++++
>  4 files changed, 362 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-mt7621.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a6954776a37e..8472dfbffcfa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9957,6 +9957,13 @@ L:	linux-wireless@vger.kernel.org
>  S:	Maintained
>  F:	drivers/net/wireless/mediatek/mt7601u/
>
> +MEDIATEK MT7621/28/88 I2C DRIVER
> +M:	Stefan Roese <sr@denx.de>
> +L:	linux-i2c@vger.kernel.org
> +S:	Maintained
> +F:	drivers/i2c/busses/i2c-mt7621.c
> +F:	Documentation/devicetree/bindings/i2c/i2c-mt7621.txt
> +
>  MEDIATEK NAND CONTROLLER DRIVER
>  M:	Xiaolei Li <xiaolei.li@mediatek.com>
>  L:	linux-mtd@lists.infradead.org
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index ee5dfb5aee2a..f9ae8bcd4d8b 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -746,6 +746,14 @@ config I2C_MT65XX
>  	  If you want to use MediaTek(R) I2C interface, say Y or M here.
>  	  If unsure, say N.
>
> +config I2C_MT7621
> +	tristate "MT7621/MT7628 I2C Controller"
> +	depends on (RALINK && (SOC_MT7620 || SOC_MT7621)) || COMPILE_TEST
> +	select OF_I2C
> +	help
> +	  Say Y here to include support for I2C controller in the
> +	  MediaTek MT7621/MT7628 SoCs.
> +
>  config I2C_MV64XXX
>  	tristate "Marvell mv64xxx I2C Controller"
>  	depends on MV64X60 || PLAT_ORION || ARCH_SUNXI || ARCH_MVEBU
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index a3245231b0b7..80c23895eaaf 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_I2C_LPC2K)		+= i2c-lpc2k.o
>  obj-$(CONFIG_I2C_MESON)		+= i2c-meson.o
>  obj-$(CONFIG_I2C_MPC)		+= i2c-mpc.o
>  obj-$(CONFIG_I2C_MT65XX)	+= i2c-mt65xx.o
> +obj-$(CONFIG_I2C_MT7621)	+= i2c-mt7621.o
>  obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
>  obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
>  obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
> diff --git a/drivers/i2c/busses/i2c-mt7621.c  
> b/drivers/i2c/busses/i2c-mt7621.c
> new file mode 100644
> index 000000000000..ff8f8c80aa27
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-mt7621.c
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * drivers/i2c/busses/i2c-mt7621.c
> + *
> + * Copyright (C) 2013 Steven Liu <steven_liu@mediatek.com>
> + * Copyright (C) 2016 Michael Lee <igvtee@gmail.com>
> + * Copyright (C) 2018 Jan Breuer <jan.breuer@jaybee.cz>
> + *
> + * Improve driver for i2cdetect from i2c-tools to detect i2c  
> devices on the bus.
> + * (C) 2014 Sittisak <sittisaks@hotmail.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/reset.h>
> +
> +#define REG_SM0CFG2_REG		0x28
> +#define REG_SM0CTL0_REG		0x40
> +#define REG_SM0CTL1_REG		0x44
> +#define REG_SM0D0_REG		0x50
> +#define REG_SM0D1_REG		0x54
> +#define REG_PINTEN_REG		0x5c
> +#define REG_PINTST_REG		0x60
> +#define REG_PINTCL_REG		0x64
> +
> +/* REG_SM0CFG2_REG */
> +#define SM0CFG2_IS_AUTOMODE	BIT(0)
> +
> +/* REG_SM0CTL0_REG */
> +#define SM0CTL0_ODRAIN		BIT(31)
> +#define SM0CTL0_CLK_DIV_MASK	(0x7ff << 16)
> +#define SM0CTL0_CLK_DIV_MAX	0x7ff
> +#define SM0CTL0_CS_STATUS       BIT(4)
> +#define SM0CTL0_SCL_STATE       BIT(3)
> +#define SM0CTL0_SDA_STATE       BIT(2)
> +#define SM0CTL0_EN              BIT(1)
> +#define SM0CTL0_SCL_STRETCH     BIT(0)
> +
> +/* REG_SM0CTL1_REG */
> +#define SM0CTL1_ACK_MASK	(0xff << 16)
> +#define SM0CTL1_PGLEN_MASK	(0x7 << 8)
> +#define SM0CTL1_PGLEN(x)	((((x) - 1) << 8) & SM0CTL1_PGLEN_MASK)
> +#define SM0CTL1_READ		(5 << 4)
> +#define SM0CTL1_READ_LAST	(4 << 4)
> +#define SM0CTL1_STOP		(3 << 4)
> +#define SM0CTL1_WRITE		(2 << 4)
> +#define SM0CTL1_START		(1 << 4)
> +#define SM0CTL1_MODE_MASK	(0x7 << 4)
> +#define SM0CTL1_TRI		BIT(0)
> +
> +/* timeout waiting for I2C devices to respond */
> +#define TIMEOUT_MS		1000
> +
> +#define MT76XX_I2C_INPUT_CLOCK	40000000
> +
> +struct mtk_i2c {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct i2c_adapter adap;
> +	u32 bus_freq;
> +	u32 clk_div;
> +	u32 flags;
> +};
> +
> +static int mtk_i2c_wait_idle(struct mtk_i2c *i2c)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = readl_relaxed_poll_timeout(i2c->base + REG_SM0CTL1_REG,
> +					 val, !(val & SM0CTL1_TRI),
> +					 10, TIMEOUT_MS * 1000);
> +	if (ret)
> +		dev_dbg(i2c->dev, "idle err(%d)\n", ret);
> +
> +	return ret;
> +}
> +
> +static void mtk_i2c_reset(struct mtk_i2c *i2c)
> +{
> +	int ret;
> +
> +	ret = device_reset(i2c->adap.dev.parent);
> +	if (ret)
> +		dev_err(i2c->dev, "I2C reset failed!\n");
> +
> +	/*
> +	 * Don't set SM0CTL0_ODRAIN as its bit meaning is inverted. To
> +	 * configure open-drain mode, this bit needs to be cleared.
> +	 */
> +	iowrite32(((i2c->clk_div << 16) & SM0CTL0_CLK_DIV_MASK) | SM0CTL0_EN |
> +		  SM0CTL0_SCL_STRETCH, i2c->base + REG_SM0CTL0_REG);
> +	iowrite32(0, i2c->base + REG_SM0CFG2_REG);
> +}
> +
> +static void mtk_i2c_dump_reg(struct mtk_i2c *i2c)
> +{
> +	dev_dbg(i2c->dev,
> +		"SM0CFG2 %08x, SM0CTL0 %08x, SM0CTL1 %08x, SM0D0 %08x, SM0D1 %08x\n",
> +		ioread32(i2c->base + REG_SM0CFG2_REG),
> +		ioread32(i2c->base + REG_SM0CTL0_REG),
> +		ioread32(i2c->base + REG_SM0CTL1_REG),
> +		ioread32(i2c->base + REG_SM0D0_REG),
> +		ioread32(i2c->base + REG_SM0D1_REG));
> +}
> +
> +static int mtk_i2c_check_ack(struct mtk_i2c *i2c, u32 expected)
> +{
> +	u32 ack = readl_relaxed(i2c->base + REG_SM0CTL1_REG);
> +	u32 ack_expected = (expected << 16) & SM0CTL1_ACK_MASK;
> +
> +	return ((ack & ack_expected) == ack_expected) ? 0 : -ENXIO;
> +}
> +
> +static int mtk_i2c_master_start(struct mtk_i2c *i2c)
> +{
> +	iowrite32(SM0CTL1_START | SM0CTL1_TRI, i2c->base + REG_SM0CTL1_REG);
> +	return mtk_i2c_wait_idle(i2c);
> +}
> +
> +static int mtk_i2c_master_stop(struct mtk_i2c *i2c)
> +{
> +	iowrite32(SM0CTL1_STOP | SM0CTL1_TRI, i2c->base + REG_SM0CTL1_REG);
> +	return mtk_i2c_wait_idle(i2c);
> +}
> +
> +static int mtk_i2c_master_cmd(struct mtk_i2c *i2c, u32 cmd, int page_len)
> +{
> +	iowrite32(cmd | SM0CTL1_TRI | SM0CTL1_PGLEN(page_len),
> +		  i2c->base + REG_SM0CTL1_REG);
> +	return mtk_i2c_wait_idle(i2c);
> +}
> +
> +static int mtk_i2c_master_xfer(struct i2c_adapter *adap, struct  
> i2c_msg *msgs,
> +			       int num)
> +{
> +	struct mtk_i2c *i2c;
> +	struct i2c_msg *pmsg;
> +	u16 addr;
> +	int i, j, ret, len, page_len;
> +	u32 cmd;
> +	u32 data[2];
> +
> +	i2c = i2c_get_adapdata(adap);
> +
> +	for (i = 0; i < num; i++) {
> +		pmsg = &msgs[i];
> +
> +		/* wait hardware idle */
> +		ret = mtk_i2c_wait_idle(i2c);
> +		if (ret)
> +			goto err_timeout;
> +
> +		/* start sequence */
> +		ret = mtk_i2c_master_start(i2c);
> +		if (ret)
> +			goto err_timeout;
> +
> +		/* write address */
> +		if (pmsg->flags & I2C_M_TEN) {
> +			/* 10 bits address */
> +			addr = 0xf0 | ((pmsg->addr >> 7) & 0x06);
> +			addr |= (pmsg->addr & 0xff) << 8;
> +			if (pmsg->flags & I2C_M_RD)
> +				addr |= 1;
> +			iowrite32(addr, i2c->base + REG_SM0D0_REG);
> +			ret = mtk_i2c_master_cmd(i2c, SM0CTL1_WRITE, 2);
> +			if (ret)
> +				goto err_timeout;
> +		} else {
> +			/* 7 bits address */
> +			addr = i2c_8bit_addr_from_msg(pmsg);
> +			iowrite32(addr, i2c->base + REG_SM0D0_REG);
> +			ret = mtk_i2c_master_cmd(i2c, SM0CTL1_WRITE, 1);
> +			if (ret)
> +				goto err_timeout;
> +		}
> +
> +		/* check address ACK */
> +		if (!(pmsg->flags & I2C_M_IGNORE_NAK)) {
> +			ret = mtk_i2c_check_ack(i2c, BIT(0));
> +			if (ret)
> +				goto err_ack;
> +		}
> +
> +		/* transfer data */
> +		for (len = pmsg->len, j = 0; len > 0; len -= 8, j += 8) {
> +			page_len = (len >= 8) ? 8 : len;
> +
> +			if (pmsg->flags & I2C_M_RD) {
> +				cmd = (len > 8) ?
> +					SM0CTL1_READ : SM0CTL1_READ_LAST;
> +			} else {
> +				memcpy(data, &pmsg->buf[j], page_len);
> +				iowrite32(data[0], i2c->base + REG_SM0D0_REG);
> +				iowrite32(data[1], i2c->base + REG_SM0D1_REG);
> +				cmd = SM0CTL1_WRITE;
> +			}
> +
> +			ret = mtk_i2c_master_cmd(i2c, cmd, page_len);
> +			if (ret)
> +				goto err_timeout;
> +
> +			if (pmsg->flags & I2C_M_RD) {
> +				data[0] = ioread32(i2c->base + REG_SM0D0_REG);
> +				data[1] = ioread32(i2c->base + REG_SM0D1_REG);
> +				memcpy(&pmsg->buf[j], data, page_len);
> +			} else {
> +				if (!(pmsg->flags & I2C_M_IGNORE_NAK)) {
> +					ret = mtk_i2c_check_ack(i2c,
> +								(1 << page_len)
> +								- 1);
> +					if (ret)
> +						goto err_ack;
> +				}
> +			}
> +		}
> +	}
> +
> +	ret = mtk_i2c_master_stop(i2c);
> +	if (ret)
> +		goto err_timeout;
> +
> +	/* the return value is number of executed messages */
> +	return i;
> +
> +err_ack:
> +	ret = mtk_i2c_master_stop(i2c);
> +	if (ret)
> +		goto err_timeout;
> +	return -ENXIO;
> +
> +err_timeout:
> +	mtk_i2c_dump_reg(i2c);
> +	mtk_i2c_reset(i2c);
> +	return ret;
> +}
> +
> +static u32 mtk_i2c_func(struct i2c_adapter *a)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING;
> +}
> +
> +static const struct i2c_algorithm mtk_i2c_algo = {
> +	.master_xfer	= mtk_i2c_master_xfer,
> +	.functionality	= mtk_i2c_func,
> +};
> +
> +static const struct of_device_id i2c_mtk_dt_ids[] = {
> +	{ .compatible = "mediatek,mt7621-i2c" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, i2c_mtk_dt_ids);
> +
> +static void mtk_i2c_init(struct mtk_i2c *i2c)
> +{
> +	i2c->clk_div = MT76XX_I2C_INPUT_CLOCK / i2c->bus_freq - 1;
> +	if (i2c->clk_div < 99)
> +		i2c->clk_div = 99;
> +	if (i2c->clk_div > SM0CTL0_CLK_DIV_MAX)
> +		i2c->clk_div = SM0CTL0_CLK_DIV_MAX;
> +
> +	mtk_i2c_reset(i2c);
> +}
> +
> +static int mtk_i2c_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct mtk_i2c *i2c;
> +	struct i2c_adapter *adap;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	i2c = devm_kzalloc(&pdev->dev, sizeof(struct mtk_i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;
> +
> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(i2c->base))
> +		return PTR_ERR(i2c->base);
> +
> +	i2c->dev = &pdev->dev;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +				 &i2c->bus_freq))
> +		i2c->bus_freq = 100000;
> +
> +	if (i2c->bus_freq == 0) {
> +		dev_warn(i2c->dev,
> +			 "clock-frequency 0 not supported, using 1\n");
> +		i2c->bus_freq = 1;
> +	}
> +
> +	adap = &i2c->adap;
> +	adap->owner = THIS_MODULE;
> +	adap->algo = &mtk_i2c_algo;
> +	adap->retries = 3;
> +	adap->dev.parent = &pdev->dev;
> +	i2c_set_adapdata(adap, i2c);
> +	adap->dev.of_node = pdev->dev.of_node;
> +	strlcpy(adap->name, dev_name(&pdev->dev), sizeof(adap->name));
> +
> +	platform_set_drvdata(pdev, i2c);
> +
> +	mtk_i2c_init(i2c);
> +
> +	ret = i2c_add_adapter(adap);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(&pdev->dev, "clock %u kHz\n", i2c->bus_freq / 1000);
> +
> +	return ret;
> +}
> +
> +static int mtk_i2c_remove(struct platform_device *pdev)
> +{
> +	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c->adap);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mtk_i2c_driver = {
> +	.probe		= mtk_i2c_probe,
> +	.remove		= mtk_i2c_remove,
> +	.driver		= {
> +		.owner	= THIS_MODULE,
> +		.name	= "i2c-mt7621",
> +		.of_match_table = i2c_mtk_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(mtk_i2c_driver);
> +
> +MODULE_AUTHOR("Steven Liu <steven_liu@mediatek.com>");
> +MODULE_DESCRIPTION("MT7621 I2C host driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:MT7621-I2C");
> --
> 2.21.0

I was using the openwrt version for a while on my Ubiquity ER-X-SFP
router on top of the net-next tree.
But I was unable to readout the SFP module because i2c message size
limit of 64 bytes.
Without additional patches my console was spamming me with
-EOPNOTSUPP because SFP code tries to read +90 bytes in one go.

With this driver the sfp code can readout the module successfully.

Both the SFP module and the GPIO expander on the bus are working.

Tested-by: René van Dorst <opensource@vdorst.com>

Greats,

René

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

* Re: [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver
  2019-06-04 11:34 ` [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver Stefan Roese
  2019-06-04 13:00   ` Jan Breuer
  2019-06-04 19:52   ` René van Dorst
@ 2019-06-14 21:40   ` Wolfram Sang
  2 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2019-06-14 21:40 UTC (permalink / raw)
  To: Stefan Roese
  Cc: linux-i2c, devicetree, Jan Breuer, John Crispin,
	René van Dorst, Wolfram Sang

[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]

Hi Stefan,

On Tue, Jun 04, 2019 at 01:34:07PM +0200, Stefan Roese wrote:
> This patch adds a driver for the I2C controller found on the MediaTek
> MT7621/7628/7688 SoC's. The base version of this driver was done by
> Steven Liu (according to the copyright and MODULE_AUTHOR lines). It
> can be found in the OpenWRT repositories (v4.14 at the time I looked).
> 
> The base driver had many issues, which are disccussed here:
> 
> https://en.forum.labs.mediatek.com/t/openwrt-15-05-loads-non-working-i2c-kernel-module-for-mt7688/1286/3
> 
> From this link an enhanced driver version (complete rewrite, mayor
> changes: support clock stretching, repeated start, ACK handling and
> unlimited message length) from Jan Breuer can be found here:
> 
> https://gist.github.com/j123b567/9b555b635c2b4069d716b24198546954
> 
> This patch now adds this enhanced I2C driver to mainline.
> 
> Changes by Stefan Roese for upstreaming:
> - Add devicetree bindings
> - checkpatch clean
> - Use module_platform_driver()
> - Minor cosmetic enhancements
> - Removed IO warpped functions
> - Use readl_relaxed_poll_timeout() and drop poll_down_timeout()
> - Removed superfluous barrier() in mtk_i2c_reset()
> - Use i2c_8bit_addr_from_msg()
> - Added I2C_FUNC_PROTOCOL_MANGLING
> - Removed adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> 
> Signed-off-by: Stefan Roese <sr@denx.de>

Mostly good, really minor nits left.

> +config I2C_MT7621
> +	tristate "MT7621/MT7628 I2C Controller"
> +	depends on (RALINK && (SOC_MT7620 || SOC_MT7621)) || COMPILE_TEST
> +	select OF_I2C

OF_I2C is gone since 2013. Didn't Kconfig complain?

> +
> +#define MT76XX_I2C_INPUT_CLOCK	40000000

Add a comment here explaining that this is a temporary solution until
proper clock support is added?

> +	ret = readl_relaxed_poll_timeout(i2c->base + REG_SM0CTL1_REG,
> +					 val, !(val & SM0CTL1_TRI),
> +					 10, TIMEOUT_MS * 1000);

Yay, much better.

> +	if (i2c->bus_freq == 0) {
> +		dev_warn(i2c->dev,
> +			 "clock-frequency 0 not supported, using 1\n");
> +		i2c->bus_freq = 1;
> +	}

A bus frequency of 1 Hz does neither make much sense. -EINVAL because
there is surely something broken then?

> +MODULE_AUTHOR("Steven Liu <steven_liu@mediatek.com>");

His address is known to be broken!

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

end of thread, other threads:[~2019-06-14 21:40 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-04 11:34 [PATCH 1/2 v3] dt-bindings: i2c: i2c-mt7621: Add bindings for MediaTek MT7621/28/88 I2C Stefan Roese
2019-06-04 11:34 ` [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver Stefan Roese
2019-06-04 13:00   ` Jan Breuer
2019-06-04 14:03     ` Stefan Roese
2019-06-04 14:42       ` Jan Breuer
2019-06-04 19:52   ` René van Dorst
2019-06-14 21:40   ` Wolfram Sang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.