[PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller

[PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller

[Yinbo Zhu]

This bus driver supports the Loongson i2c hardware controller in the
Loongson platforms and supports to use DTS and ACPI framework to
register i2c adapter device resources.

The Loongson i2c controller supports operating frequencty is 50MHZ
and supports the maximum transmission rate is 400kbps.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
Change in v2:
		1. Used BIT() function to replace some register bit.
		2. Used static inline helpers to replace bad macros for io.
		3. Make some adjustment about the first element in 
		   struct loongson_i2c_dev.
		4. Fixup the Broken indentation.
		5. Used specific macro or helper for i2c addr.	
		6. Remove some unsuitable parentheses, such as that replace
		   (&dev->adapter)->timeout) with dev->adapter.timeout.
		7. Used do-while to replace the goto sentence. 
		8. Fixup the unnormative error code.
		9. Used GENMASK to replace 0xff/0xff0.
		10. Add device_set_node() after ACPI_COMPANION_SET. 
		11. Used devm_* function to standardize the code.
		12. Remove the of_match_ptr() and ACPI_PTR(). 

 MAINTAINERS                       |   6 +
 drivers/i2c/busses/Kconfig        |  10 +
 drivers/i2c/busses/Makefile       |   1 +
 drivers/i2c/busses/i2c-loongson.c | 547 ++++++++++++++++++++++++++++++
 4 files changed, 564 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-loongson.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b8a02a60973d..95f26184e17c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12058,6 +12058,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml
 F:	drivers/gpio/gpio-loongson-64bit.c
 
+LOONGSON I2C DRIVER
+M:	Yinbo Zhu <zhuyinbo@loongson.cn>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	drivers/i2c/busses/i2c-loongson.c
+
 LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
 M:	Sathya Prakash <sathya.prakash@broadcom.com>
 M:	Sreekanth Reddy <sreekanth.reddy@broadcom.com>
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e50f9603d189..25b22b54504a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -751,6 +751,16 @@ config I2C_KEMPLD
 	  This driver can also be built as a module. If so, the module
 	  will be called i2c-kempld.
 
+config I2C_LOONGSON
+        tristate "Loongson I2C controller"
+        depends on LOONGARCH || COMPILE_TEST
+        help
+          If you say yes to this option, support will be included for the
+          built-in I2C interface on the Loongson series Platform.
+
+          This driver can also be built as a module. If so, the module
+          will be called i2c-loongson.
+
 config I2C_LPC2K
 	tristate "I2C bus support for NXP LPC2K/LPC178x/18xx/43xx"
 	depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e73cdb1d2b5a..28a3fd18ffaf 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_IMX_LPI2C)	+= i2c-imx-lpi2c.o
 obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
 obj-$(CONFIG_I2C_JZ4780)	+= i2c-jz4780.o
 obj-$(CONFIG_I2C_KEMPLD)	+= i2c-kempld.o
+obj-$(CONFIG_I2C_LOONGSON)	+= i2c-loongson.o
 obj-$(CONFIG_I2C_LPC2K)		+= i2c-lpc2k.o
 obj-$(CONFIG_I2C_MESON)		+= i2c-meson.o
 obj-$(CONFIG_I2C_MICROCHIP_CORE)	+= i2c-microchip-corei2c.o
diff --git a/drivers/i2c/busses/i2c-loongson.c b/drivers/i2c/busses/i2c-loongson.c
new file mode 100644
index 000000000000..14afb8cd627b
--- /dev/null
+++ b/drivers/i2c/busses/i2c-loongson.c
@@ -0,0 +1,547 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Author: Yinbo Zhu <zhuyinbo@loongson.cn>
+ * Copyright (C) 2022 Loongson Technology Corporation Limited
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define LOONGSON_I2C_ADDR_A7(x)		(x & 0x7f)
+
+#define LOONGSON_I2C_PRER_LO_REG	0x0
+#define LOONGSON_I2C_PRER_HI_REG	0x1
+#define LOONGSON_I2C_CTR_REG		0x2
+#define LOONGSON_I2C_TXR_REG		0x3
+#define LOONGSON_I2C_RXR_REG		0x3
+#define LOONGSON_I2C_CR_REG		0x4
+#define LOONGSON_I2C_SR_REG		0x4
+#define LOONGSON_I2C_BLTOP_REG		0x5
+#define LOONGSON_I2C_SADDR_REG		0x7
+
+#define CTR_EN				BIT(7)
+#define CTR_IEN				BIT(6)
+#define CTR_TXROK			BIT(4)
+#define CTR_RXROK			BIT(3)
+
+#define CR_START			BIT(7)
+#define CR_STOP				BIT(6)
+#define CR_READ				BIT(5)
+#define CR_WRITE			BIT(4)
+#define CR_ACK				BIT(3)
+#define CR_IACK				BIT(0)
+
+#define SR_NOACK			BIT(7)
+#define SR_BUSY				BIT(6)
+#define SR_AL				BIT(5)
+#define SR_SLAVE_ADDRESSED		BIT(4)
+#define SR_SLAVE_RW			BIT(3)
+#define SR_TIP				BIT(1)
+#define SR_IF				BIT(0)
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+enum loongson_i2c_slave_state {
+	LOONGSON_I2C_SLAVE_STOP,
+	LOONGSON_I2C_SLAVE_START,
+	LOONGSON_I2C_SLAVE_READ_REQUESTED,
+	LOONGSON_I2C_SLAVE_READ_PROCESSED,
+	LOONGSON_I2C_SLAVE_WRITE_REQUESTED,
+	LOONGSON_I2C_SLAVE_WRITE_RECEIVED,
+};
+#endif
+
+struct loongson_i2c_dev {
+	struct i2c_adapter	adapter;
+	unsigned int		suspended:1;
+	struct device		*dev;
+	void __iomem		*base;
+	int			irq;
+	u32			speed_hz;
+	struct completion	cmd_complete;
+	spinlock_t		lock;
+	struct resource		*ioarea;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	struct i2c_client	*slave;
+	enum loongson_i2c_slave_state	slave_state;
+#endif
+};
+
+static inline u8 i2c_readb(struct loongson_i2c_dev *dev, u8 offset)
+{
+	return readb(dev->base + offset);
+}
+
+static inline void i2c_writeb(struct loongson_i2c_dev *dev, u8 val,
+			      u8 offset)
+{
+	writeb(val, dev->base + offset);
+}
+
+static int loongson_i2c_stop(struct loongson_i2c_dev *dev)
+{
+	unsigned long time_left;
+
+	do {
+		i2c_writeb(dev, CR_STOP | CR_IACK, LOONGSON_I2C_CR_REG);
+		time_left = wait_for_completion_timeout(&dev->cmd_complete,
+							dev->adapter.timeout);
+		if (!time_left)
+			return -ETIMEDOUT;
+
+	} while (i2c_readb(dev, LOONGSON_I2C_SR_REG) & SR_BUSY);
+
+	return 0;
+}
+
+static int loongson_i2c_start(struct loongson_i2c_dev *dev, int dev_addr,
+			      int flags)
+{
+	int ret;
+	unsigned long time_left;
+	int retry = 5;
+	unsigned char addr = LOONGSON_I2C_ADDR_A7(dev_addr) << 1;
+
+	addr |= (flags & I2C_M_RD) ? 1 : 0;
+
+	do {
+		mdelay(1);
+		i2c_writeb(dev, addr, LOONGSON_I2C_TXR_REG);
+		i2c_writeb(dev, (CR_START | CR_WRITE | CR_IACK),
+			   LOONGSON_I2C_CR_REG);
+		time_left = wait_for_completion_timeout(&dev->cmd_complete,
+							dev->adapter.timeout);
+		if (!time_left)
+			return -ETIMEDOUT;
+
+		if (i2c_readb(dev, LOONGSON_I2C_SR_REG) & SR_NOACK) {
+			ret = loongson_i2c_stop(dev);
+			if (ret)
+				return ret;
+		} else
+			break;
+	} while (retry--);
+
+	return 0;
+}
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static void loongson_i2c_slave_init(struct loongson_i2c_dev *dev,
+				    u16 slave_addr)
+{
+	i2c_writeb(dev, LOONGSON_I2C_ADDR_A7(slave_addr),
+		   LOONGSON_I2C_SADDR_REG);
+	i2c_writeb(dev, 0xc0, LOONGSON_I2C_CTR_REG);
+}
+
+static int loongson_i2c_reg_slave(struct i2c_client *client)
+{
+	unsigned long flags;
+	struct loongson_i2c_dev *dev = i2c_get_adapdata(client->adapter);
+
+	if (dev->slave)
+		return -EINVAL;
+
+	loongson_i2c_slave_init(dev, client->addr);
+
+	dev->slave = client;
+	dev->slave_state = LOONGSON_I2C_SLAVE_STOP;
+
+	return 0;
+}
+
+static int loongson_i2c_unreg_slave(struct i2c_client *client)
+{
+	unsigned long flags;
+	struct loongson_i2c_dev *dev = i2c_get_adapdata(client->adapter);
+
+	if (!dev->slave)
+		return -EINVAL;
+
+	i2c_writeb(dev, 0xa0, LOONGSON_I2C_CTR_REG);
+
+	dev->slave = NULL;
+
+	return 0;
+}
+#endif
+
+static void loongson_i2c_reginit(struct loongson_i2c_dev *dev)
+{
+	u16 prer_val;
+	u32 pclk;
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (dev->slave) {
+		loongson_i2c_slave_init(dev, dev->slave->addr);
+		return;
+	}
+#endif
+	if (!dev->speed_hz) {
+		prer_val = 0x12c;
+	} else {
+		pclk = 50000000;
+		prer_val = pclk / (5 * dev->speed_hz) - 1;
+	}
+
+	i2c_writeb(dev, i2c_readb(dev, LOONGSON_I2C_CR_REG) |
+		   0x01, LOONGSON_I2C_CR_REG);
+	i2c_writeb(dev, i2c_readb(dev, LOONGSON_I2C_CTR_REG) & ~0x80,
+		   LOONGSON_I2C_CTR_REG);
+	i2c_writeb(dev, prer_val & GENMASK(7, 0), LOONGSON_I2C_PRER_LO_REG);
+	i2c_writeb(dev, (prer_val & GENMASK(15, 8)) >> 8,
+		   LOONGSON_I2C_PRER_HI_REG);
+	i2c_writeb(dev, i2c_readb(dev, LOONGSON_I2C_CTR_REG) |
+		   0xe0, LOONGSON_I2C_CTR_REG);
+}
+
+static int loongson_i2c_read(struct loongson_i2c_dev *dev, unsigned char *buf,
+			     int count)
+{
+	int i;
+	unsigned long time_left;
+
+	for (i = 0; i < count; i++) {
+		i2c_writeb(dev, (i == count - 1) ?
+			(CR_READ | CR_IACK | CR_ACK) : (CR_READ | CR_IACK),
+			LOONGSON_I2C_CR_REG);
+
+		time_left = wait_for_completion_timeout(&dev->cmd_complete,
+							dev->adapter.timeout);
+		if (!time_left)
+			return -ETIMEDOUT;
+
+		buf[i] = i2c_readb(dev, LOONGSON_I2C_RXR_REG);
+	}
+
+	return i;
+}
+
+static int loongson_i2c_write(struct loongson_i2c_dev *dev, unsigned char *buf,
+			      int count)
+{
+	int i;
+	int ret;
+	unsigned long time_left;
+
+	for (i = 0; i < count; i++) {
+		i2c_writeb(dev, buf[i], LOONGSON_I2C_TXR_REG);
+		i2c_writeb(dev, CR_WRITE | CR_IACK, LOONGSON_I2C_CR_REG);
+		time_left = wait_for_completion_timeout(&dev->cmd_complete,
+							dev->adapter.timeout);
+		if (!time_left)
+			return -ETIMEDOUT;
+
+		if (i2c_readb(dev, LOONGSON_I2C_SR_REG) & SR_NOACK) {
+			ret = loongson_i2c_stop(dev);
+			if (ret)
+				return ret;
+			return 0;
+		}
+	}
+
+	return i;
+}
+
+static int loongson_i2c_doxfer(struct loongson_i2c_dev *dev, struct i2c_msg *msgs,
+			       int num)
+{
+	int i, ret;
+	struct i2c_msg *m = msgs;
+
+	for (i = 0; i < num; i++) {
+		reinit_completion(&dev->cmd_complete);
+		ret = loongson_i2c_start(dev, m->addr, m->flags);
+		if (ret)
+			return ret;
+
+		if (m->flags & I2C_M_RD) {
+			ret = loongson_i2c_read(dev, m->buf, m->len);
+			if (ret)
+				return ret;
+		}
+
+		if (!(m->flags & I2C_M_RD)) {
+			ret = loongson_i2c_write(dev, m->buf, m->len);
+			if (ret)
+				return ret;
+		}
+
+		++m;
+	}
+
+	ret = loongson_i2c_stop(dev);
+	if (ret)
+		return ret;
+
+	return i;
+}
+
+static int loongson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			     int num)
+{
+	int ret;
+	int retry;
+	struct loongson_i2c_dev *dev;
+
+	dev = i2c_get_adapdata(adap);
+	for (retry = 0; retry < adap->retries; retry++) {
+		ret = loongson_i2c_doxfer(dev, msgs, num);
+		if (ret != -EAGAIN)
+			return ret;
+
+		udelay(100);
+	}
+
+	return -EREMOTEIO;
+}
+
+static unsigned int loongson_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm loongson_i2c_algo = {
+	.master_xfer	= loongson_i2c_xfer,
+	.functionality	= loongson_i2c_func,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	.reg_slave	= loongson_i2c_reg_slave,
+	.unreg_slave	= loongson_i2c_unreg_slave,
+#endif
+};
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static bool loongson_i2c_slave_irq(struct loongson_i2c_dev *dev)
+{
+	u32 stat;
+	struct i2c_client *slave = dev->slave;
+	u8 value;
+
+	stat = i2c_readb(dev, LOONGSON_I2C_SR_REG);
+
+	if (stat & SR_SLAVE_ADDRESSED) {
+		dev->slave_state = LOONGSON_I2C_SLAVE_START;
+		i2c_writeb(dev, CTR_RXROK | CTR_EN | CTR_IEN,
+			LOONGSON_I2C_CTR_REG);
+	}
+
+	if (dev->slave_state == LOONGSON_I2C_SLAVE_STOP)
+		return IRQ_NONE;
+
+	if (dev->slave_state == LOONGSON_I2C_SLAVE_START)
+		if (stat & SR_SLAVE_RW)
+			dev->slave_state =
+				LOONGSON_I2C_SLAVE_READ_REQUESTED;
+		else
+			dev->slave_state =
+				LOONGSON_I2C_SLAVE_WRITE_REQUESTED;
+
+	if (stat & SR_NOACK)
+		dev->slave_state = LOONGSON_I2C_SLAVE_STOP;
+
+	value = i2c_readb(dev, LOONGSON_I2C_RXR_REG);
+	switch (dev->slave_state) {
+	case LOONGSON_I2C_SLAVE_READ_REQUESTED:
+		dev->slave_state = LOONGSON_I2C_SLAVE_READ_PROCESSED;
+		i2c_slave_event(
+			slave, LOONGSON_I2C_SLAVE_READ_REQUESTED, &value);
+		i2c_writeb(dev, value, LOONGSON_I2C_TXR_REG);
+		i2c_writeb(dev, CTR_TXROK | CTR_EN | CTR_IEN, LOONGSON_I2C_CTR_REG);
+		break;
+	case LOONGSON_I2C_SLAVE_READ_PROCESSED:
+		i2c_slave_event(
+			slave, LOONGSON_I2C_SLAVE_READ_PROCESSED, &value);
+		i2c_writeb(dev, value, LOONGSON_I2C_TXR_REG);
+		i2c_writeb(dev, CTR_TXROK | CTR_EN | CTR_IEN, LOONGSON_I2C_CTR_REG);
+		break;
+	case LOONGSON_I2C_SLAVE_WRITE_REQUESTED:
+		dev->slave_state = LOONGSON_I2C_SLAVE_WRITE_RECEIVED;
+		i2c_slave_event(
+			slave, LOONGSON_I2C_SLAVE_WRITE_REQUESTED, &value);
+		break;
+	case LOONGSON_I2C_SLAVE_WRITE_RECEIVED:
+		i2c_slave_event(
+			slave, LOONGSON_I2C_SLAVE_WRITE_RECEIVED, &value);
+		i2c_writeb(dev, CTR_RXROK | CTR_EN | CTR_IEN, LOONGSON_I2C_CTR_REG);
+		break;
+	case LOONGSON_I2C_SLAVE_STOP:
+		i2c_slave_event(slave, LOONGSON_I2C_SLAVE_STOP, &value);
+		i2c_writeb(dev, 0, LOONGSON_I2C_TXR_REG);
+		i2c_writeb(dev, CTR_TXROK | CTR_EN | CTR_IEN, LOONGSON_I2C_CTR_REG);
+		break;
+	default:
+		dev_err(dev->dev,
+			"unhandled slave_state: %d\n", dev->slave_state);
+		break;
+	}
+
+out:
+	return IRQ_HANDLED;
+}
+#endif
+
+static irqreturn_t loongson_i2c_isr(int this_irq, void *dev_id)
+{
+	unsigned char iflag;
+	struct loongson_i2c_dev *dev = dev_id;
+
+	iflag = i2c_readb(dev, LOONGSON_I2C_SR_REG);
+
+	if (iflag & SR_IF) {
+		i2c_writeb(dev, CR_IACK, LOONGSON_I2C_CR_REG);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		if (dev->slave)
+			loongson_i2c_slave_irq(dev);
+#endif
+		if (!(iflag & SR_TIP))
+			complete(&dev->cmd_complete);
+	} else
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static int loongson_i2c_probe(struct platform_device *pdev)
+{
+	struct loongson_i2c_dev	*dev;
+	void __iomem		*reg_base;
+	struct i2c_adapter	*adap;
+	int			ret, irq;
+	u32			bus_speed;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(reg_base))
+		return PTR_ERR(reg_base);
+
+	bus_speed = i2c_acpi_find_bus_speed(&pdev->dev);
+	if (!bus_speed)
+		device_property_read_u32(&pdev->dev, "clock-frequency",
+					&bus_speed);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return dev_err_probe(&pdev->dev, irq, "no irq resource\n");
+
+	init_completion(&dev->cmd_complete);
+
+	dev->dev = &pdev->dev;
+	dev->irq = irq;
+	dev->speed_hz = bus_speed;
+	dev->base = reg_base;
+
+	platform_set_drvdata(pdev, dev);
+
+	loongson_i2c_reginit(dev);
+
+	ret = devm_request_irq(&pdev->dev, irq, loongson_i2c_isr, IRQF_SHARED,
+			       dev_name(&pdev->dev), dev);
+	if (ret)
+		return ret;
+
+	adap = &dev->adapter;
+	i2c_set_adapdata(adap, dev);
+	adap->nr = pdev->id;
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON;
+	adap->retries = 5;
+	adap->algo = &loongson_i2c_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
+	device_set_node(&adap->dev, dev_fwnode(&pdev->dev));
+	adap->timeout = msecs_to_jiffies(100);
+
+	ret = i2c_add_adapter(adap);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int loongson_i2c_remove(struct platform_device *pdev)
+{
+	struct loongson_i2c_dev	*dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dev->adapter);
+
+	return 0;
+}
+
+static int loongson_i2c_suspend_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct loongson_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_dev->suspended = 1;
+
+	return 0;
+}
+
+static int loongson_i2c_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct loongson_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_dev->suspended = 0;
+	loongson_i2c_reginit(i2c_dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops loongson_i2c_pm_ops = {
+	.suspend_noirq	= loongson_i2c_suspend_noirq,
+	.resume		= loongson_i2c_resume,
+};
+
+static const struct of_device_id loongson_i2c_id_table[] = {
+	{.compatible = "loongson,ls2k-i2c"},
+	{.compatible = "loongson,ls7a-i2c"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, loongson_i2c_id_table);
+
+static const struct acpi_device_id loongson_i2c_acpi_match[] = {
+	{"LOON0004"},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, loongson_i2c_acpi_match);
+
+static struct platform_driver loongson_i2c_driver = {
+	.driver	= {
+		.name	= "loongson-i2c",
+		.owner	= THIS_MODULE,
+		.pm	= &loongson_i2c_pm_ops,
+		.of_match_table = loongson_i2c_id_table,
+		.acpi_match_table = loongson_i2c_acpi_match,
+	},
+	.probe		= loongson_i2c_probe,
+	.remove		= loongson_i2c_remove,
+};
+
+static int __init loongson_i2c_init_driver(void)
+{
+	return platform_driver_register(&loongson_i2c_driver);
+}
+subsys_initcall(loongson_i2c_init_driver);
+
+static void __exit loongson_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&loongson_i2c_driver);
+}
+module_exit(loongson_i2c_exit_driver);
+
+MODULE_DESCRIPTION("Loongson i2c driver");
+MODULE_LICENSE("GPL");
-- 
2.31.1

[PATCH v2 2/2] dt-bindings: i2c: add loongson i2c

[Yinbo Zhu]

Add the Loongson platform i2c binding with DT schema format using
json-schema.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change in v2:
		1. Removed the "#address-cells" and "#size-cells" in requied.
		2. Add the reviewed-by information.

 .../bindings/i2c/loongson,ls-i2c.yaml         | 47 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml b/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
new file mode 100644
index 000000000000..0e4aee9146f3
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/loongson,ls-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson I2C controller
+
+maintainers:
+  - Yinbo Zhu <zhuyinbo@loongson.cn>
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - loongson,ls2k-i2c
+      - loongson,ls7a-i2c
+
+  interrupts:
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c0: i2c@1fe21000 {
+        compatible = "loongson,ls2k-i2c";
+        reg = <0x1fe21000 0x8>;
+        interrupt-parent = <&liointc0>;
+        interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&i2c0_pins_default>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 95f26184e17c..9f70f4997afc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12062,6 +12062,7 @@ LOONGSON I2C DRIVER
 M:	Yinbo Zhu <zhuyinbo@loongson.cn>
 L:	linux-i2c@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
 F:	drivers/i2c/busses/i2c-loongson.c
 
 LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
-- 
2.31.1

Re: [PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 09:00:24PM +0800, Yinbo Zhu wrote:
> This bus driver supports the Loongson i2c hardware controller in the
> Loongson platforms and supports to use DTS and ACPI framework to
> register i2c adapter device resources.
> 
> The Loongson i2c controller supports operating frequencty is 50MHZ
> and supports the maximum transmission rate is 400kbps.

...

> +static inline u8 i2c_readb(struct loongson_i2c_dev *dev, u8 offset)
> +{
> +	return readb(dev->base + offset);
> +}
> +
> +static inline void i2c_writeb(struct loongson_i2c_dev *dev, u8 val,
> +			      u8 offset)

For this you may turn parameters to be in more intuitive order, i.e.

static inline void i2c_writeb(struct loongson_i2c_dev *dev, u8 offset, u8 val)

(Also, why not on one line? Even with strict 80 it still fits).

> +{
> +	writeb(val, dev->base + offset);
> +}

...

> +static int loongson_i2c_start(struct loongson_i2c_dev *dev, int dev_addr,
> +			      int flags)
> +{
> +	int ret;
> +	unsigned long time_left;
> +	int retry = 5;

> +	unsigned char addr = LOONGSON_I2C_ADDR_A7(dev_addr) << 1;
> +
> +	addr |= (flags & I2C_M_RD) ? 1 : 0;

Why not i2c_8bit_addr_from_msg()?

> +	do {

> +		mdelay(1);

Needs an explanation why.

> +		i2c_writeb(dev, addr, LOONGSON_I2C_TXR_REG);
> +		i2c_writeb(dev, (CR_START | CR_WRITE | CR_IACK),
> +			   LOONGSON_I2C_CR_REG);
> +		time_left = wait_for_completion_timeout(&dev->cmd_complete,
> +							dev->adapter.timeout);
> +		if (!time_left)
> +			return -ETIMEDOUT;
> +
> +		if (i2c_readb(dev, LOONGSON_I2C_SR_REG) & SR_NOACK) {
> +			ret = loongson_i2c_stop(dev);
> +			if (ret)
> +				return ret;
> +		} else
> +			break;
> +	} while (retry--);
> +
> +	return 0;
> +}

...

> +	i2c_writeb(dev, 0xa0, LOONGSON_I2C_CTR_REG);

Magic number.

...

> +	if (!dev->speed_hz) {


Why not positive conditional?

> +		prer_val = 0x12c;
> +	} else {
> +		pclk = 50000000;

50 * HZ_PER_MHZ?

> +		prer_val = pclk / (5 * dev->speed_hz) - 1;
> +	}
> +
> +	i2c_writeb(dev, i2c_readb(dev, LOONGSON_I2C_CR_REG) |
> +		   0x01, LOONGSON_I2C_CR_REG);
> +	i2c_writeb(dev, i2c_readb(dev, LOONGSON_I2C_CTR_REG) & ~0x80,
> +		   LOONGSON_I2C_CTR_REG);

> +	i2c_writeb(dev, prer_val & GENMASK(7, 0), LOONGSON_I2C_PRER_LO_REG);
> +	i2c_writeb(dev, (prer_val & GENMASK(15, 8)) >> 8,

Why do you need GENMASK() parts?

> +		   LOONGSON_I2C_PRER_HI_REG);
> +	i2c_writeb(dev, i2c_readb(dev, LOONGSON_I2C_CTR_REG) |
> +		   0xe0, LOONGSON_I2C_CTR_REG);

A lot of magic numbers...

...

> +static int loongson_i2c_read(struct loongson_i2c_dev *dev, unsigned char *buf,
> +			     int count)
> +{
> +	int i;
> +	unsigned long time_left;
> +
> +	for (i = 0; i < count; i++) {

> +		i2c_writeb(dev, (i == count - 1) ?
> +			(CR_READ | CR_IACK | CR_ACK) : (CR_READ | CR_IACK),
> +			LOONGSON_I2C_CR_REG);

With temporary variable this will look better.

	u8 val = CR_READ | CR_IACK;
	...

		i2c_writeb(dev, (i == count - 1) ? val | CR_ACK : val,
			   LOONGSON_I2C_CR_REG);


Also fix wrong indentation.

> +		time_left = wait_for_completion_timeout(&dev->cmd_complete,
> +							dev->adapter.timeout);
> +		if (!time_left)
> +			return -ETIMEDOUT;
> +
> +		buf[i] = i2c_readb(dev, LOONGSON_I2C_RXR_REG);
> +	}
> +
> +	return i;
> +}

...

> +static int loongson_i2c_write(struct loongson_i2c_dev *dev, unsigned char *buf,
> +			      int count)
> +{
> +	int i;
> +	int ret;
> +	unsigned long time_left;
> +
> +	for (i = 0; i < count; i++) {
> +		i2c_writeb(dev, buf[i], LOONGSON_I2C_TXR_REG);
> +		i2c_writeb(dev, CR_WRITE | CR_IACK, LOONGSON_I2C_CR_REG);
> +		time_left = wait_for_completion_timeout(&dev->cmd_complete,
> +							dev->adapter.timeout);
> +		if (!time_left)
> +			return -ETIMEDOUT;
> +
> +		if (i2c_readb(dev, LOONGSON_I2C_SR_REG) & SR_NOACK) {
> +			ret = loongson_i2c_stop(dev);
> +			if (ret)
> +				return ret;
> +			return 0;
> +		}
> +	}
> +
> +	return i;

Can i be not equal to count here?

> +}

...

> +static int loongson_i2c_doxfer(struct loongson_i2c_dev *dev, struct i2c_msg *msgs,
> +			       int num)
> +{
> +	int i, ret;
> +	struct i2c_msg *m = msgs;
> +
> +	for (i = 0; i < num; i++) {
> +		reinit_completion(&dev->cmd_complete);
> +		ret = loongson_i2c_start(dev, m->addr, m->flags);
> +		if (ret)
> +			return ret;
> +
> +		if (m->flags & I2C_M_RD) {
> +			ret = loongson_i2c_read(dev, m->buf, m->len);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		if (!(m->flags & I2C_M_RD)) {
> +			ret = loongson_i2c_write(dev, m->buf, m->len);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		++m;
> +	}
> +
> +	ret = loongson_i2c_stop(dev);
> +	if (ret)
> +		return ret;
> +
> +	return i;

Can i be not equal to num here?

> +}

...

> +static int loongson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			     int num)
> +{
> +	int ret;
> +	int retry;
> +	struct loongson_i2c_dev *dev;
> +
> +	dev = i2c_get_adapdata(adap);
> +	for (retry = 0; retry < adap->retries; retry++) {
> +		ret = loongson_i2c_doxfer(dev, msgs, num);
> +		if (ret != -EAGAIN)
> +			return ret;
> +
> +		udelay(100);
> +	}

Why udelay() and not usleep_range() ?
Why so long?

All these at least have to be explained.

> +	return -EREMOTEIO;
> +}

Why not utilizing something from iopoll.h?

...

> +	if (dev->slave_state == LOONGSON_I2C_SLAVE_START)
> +		if (stat & SR_SLAVE_RW)
> +			dev->slave_state =
> +				LOONGSON_I2C_SLAVE_READ_REQUESTED;
> +		else
> +			dev->slave_state =
> +				LOONGSON_I2C_SLAVE_WRITE_REQUESTED;

Even with strict 80 rule, these are fine to be on one line.

I suggest you to go through the code and shrink it by 20-30 LoCs. It seems
feasible taking into account this kind of indentation.

...

> +static irqreturn_t loongson_i2c_isr(int this_irq, void *dev_id)
> +{
> +	unsigned char iflag;
> +	struct loongson_i2c_dev *dev = dev_id;
> +
> +	iflag = i2c_readb(dev, LOONGSON_I2C_SR_REG);

> +	if (iflag & SR_IF) {


Why not using the usual pattern, i.e.

	if (!(...))
		return IRQ_NONE;

?

It seems you ignored some of my comments...
I stopped here, please check what was given against v1 and try again.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] dt-bindings: i2c: add loongson i2c

[Krzysztof Kozlowski]

On 28/11/2022 14:00, Yinbo Zhu wrote:
> Add the Loongson platform i2c binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Change in v2:
> 		1. Removed the "#address-cells" and "#size-cells" in requied.
> 		2. Add the reviewed-by information.
> 
>  .../bindings/i2c/loongson,ls-i2c.yaml         | 47 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml b/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
> new file mode 100644
> index 000000000000..0e4aee9146f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/loongson,ls-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson I2C controller
> +
> +maintainers:
> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - loongson,ls2k-i2c
> +      - loongson,ls7a-i2c

Why do we have the same bindings twice, with different people and file
names?

https://lore.kernel.org/all/57339e73b6c0bfe446e19a7f55a48b7ca640b9ec.1669359515.git.zhoubinbin@loongson.cn/

Best regards,
Krzysztof

Re: [PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller

[Krzysztof Kozlowski]

On 28/11/2022 15:02, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 09:00:24PM +0800, Yinbo Zhu wrote:
>> This bus driver supports the Loongson i2c hardware controller in the
>> Loongson platforms and supports to use DTS and ACPI framework to
>> register i2c adapter device resources.
>>
>> The Loongson i2c controller supports operating frequencty is 50MHZ
>> and supports the maximum transmission rate is 400kbps.
> 
> ...


> Why not using the usual pattern, i.e.
> 
> 	if (!(...))
> 		return IRQ_NONE;
> 
> ?
> 
> It seems you ignored some of my comments...
> I stopped here, please check what was given against v1 and try again.

I propose to wait with wasting more time on reviews because you might do
the same work twice:

https://lore.kernel.org/all/822356908305580d601e5b3e424371ed7f220b85.1669359515.git.zhoubinbin@loongson.cn/


Best regards,
Krzysztof

Re: [PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 03:12:54PM +0100, Krzysztof Kozlowski wrote:
> On 28/11/2022 15:02, Andy Shevchenko wrote:
> > On Mon, Nov 28, 2022 at 09:00:24PM +0800, Yinbo Zhu wrote:

...

> > It seems you ignored some of my comments...
> > I stopped here, please check what was given against v1 and try again.
> 
> I propose to wait with wasting more time on reviews because you might do
> the same work twice:
> 
> https://lore.kernel.org/all/822356908305580d601e5b3e424371ed7f220b85.1669359515.git.zhoubinbin@loongson.cn/

I see, thank for the pointer!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] dt-bindings: i2c: add loongson i2c

[Yinbo Zhu]

在 2022/11/28 22:11, Krzysztof Kozlowski 写道:
> On 28/11/2022 14:00, Yinbo Zhu wrote:
>> Add the Loongson platform i2c binding with DT schema format using
>> json-schema.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>> Change in v2:
>> 		1. Removed the "#address-cells" and "#size-cells" in requied.
>> 		2. Add the reviewed-by information.
>>
>>   .../bindings/i2c/loongson,ls-i2c.yaml         | 47 +++++++++++++++++++
>>   MAINTAINERS                                   |  1 +
>>   2 files changed, 48 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml b/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
>> new file mode 100644
>> index 000000000000..0e4aee9146f3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
>> @@ -0,0 +1,47 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/i2c/loongson,ls-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Loongson I2C controller
>> +
>> +maintainers:
>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
>> +
>> +allOf:
>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - loongson,ls2k-i2c
>> +      - loongson,ls7a-i2c
> Why do we have the same bindings twice, with different people and file
> names?
>
> https://lore.kernel.org/all/57339e73b6c0bfe446e19a7f55a48b7ca640b9ec.1669359515.git.zhoubinbin@loongson.cn/
>
> Best regards,
> Krzysztof

Inthe previous internal discussion, I was assigned to go to upstream for 
i2c, but I

don't know why other people are also working on the patch. I will go to

internal communication.

Re: [PATCH v2 2/2] dt-bindings: i2c: add loongson i2c

[Yinbo Zhu]

在 2022/11/29 7:51, Yinbo Zhu 写道:
>
> 在 2022/11/28 22:11, Krzysztof Kozlowski 写道:
>> On 28/11/2022 14:00, Yinbo Zhu wrote:
>>> Add the Loongson platform i2c binding with DT schema format using
>>> json-schema.
>>>
>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>> ---
>>> Change in v2:
>>>         1. Removed the "#address-cells" and "#size-cells" in requied.
>>>         2. Add the reviewed-by information.
>>>
>>>   .../bindings/i2c/loongson,ls-i2c.yaml         | 47 
>>> +++++++++++++++++++
>>>   MAINTAINERS                                   |  1 +
>>>   2 files changed, 48 insertions(+)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml 
>>> b/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
>>> new file mode 100644
>>> index 000000000000..0e4aee9146f3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/loongson,ls-i2c.yaml
>>> @@ -0,0 +1,47 @@
>>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/i2c/loongson,ls-i2c.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Loongson I2C controller
>>> +
>>> +maintainers:
>>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - loongson,ls2k-i2c
>>> +      - loongson,ls7a-i2c
>> Why do we have the same bindings twice, with different people and file
>> names?
>>
>> https://lore.kernel.org/all/57339e73b6c0bfe446e19a7f55a48b7ca640b9ec.1669359515.git.zhoubinbin@loongson.cn/ 
>>
>>
>> Best regards,
>> Krzysztof
>
> Inthe previous internal discussion, I was assigned to go to upstream 
> for i2c, but I
>
> don't know why other people are also working on the patch. I will go to
>
> internal communication.

Hi

please follow zhoubinbin's loongson i2c series patch.

I will not cover it.


Thanks,

Yinbo.

Re: [PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller

[Yinbo Zhu]

在 2022/11/28 22:26, Andy Shevchenko 写道:
> On Mon, Nov 28, 2022 at 03:12:54PM +0100, Krzysztof Kozlowski wrote:
>> On 28/11/2022 15:02, Andy Shevchenko wrote:
>>> On Mon, Nov 28, 2022 at 09:00:24PM +0800, Yinbo Zhu wrote:
> ...
>
>>> It seems you ignored some of my comments...
>>> I stopped here, please check what was given against v1 and try again.
>> I propose to wait with wasting more time on reviews because you might do
>> the same work twice:
>>
>> https://lore.kernel.org/all/822356908305580d601e5b3e424371ed7f220b85.1669359515.git.zhoubinbin@loongson.cn/
> I see, thank for the pointer!

Hi

Please ignore my i2c patch and help continue review zhoubinbin's 
loongson i2c series patch.

I will not cover it.


Thanks,

Yinbo.

>

Re: [PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller

[Wolfram Sang]

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

On Mon, Nov 28, 2022 at 09:00:24PM +0800, Yinbo Zhu wrote:
> This bus driver supports the Loongson i2c hardware controller in the
> Loongson platforms and supports to use DTS and ACPI framework to
> register i2c adapter device resources.
> 
> The Loongson i2c controller supports operating frequencty is 50MHZ
> and supports the maximum transmission rate is 400kbps.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>

There are currently two people submitting a driver for this hardware.
This is the other driver:

https://lore.kernel.org/all/f6cc2dbe5cd190031ab4f772d1cf250934288546.1669777792.git.zhoubinbin@loongson.cn/

Can you guys please decide which one is "better" or combine the two to
make a better one?

Thanks,

   Wolfram


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

Re: [PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller

[Yinbo Zhu]

在 2022/12/5 16:40, Wolfram Sang 写道:
> On Mon, Nov 28, 2022 at 09:00:24PM +0800, Yinbo Zhu wrote:
>> This bus driver supports the Loongson i2c hardware controller in the
>> Loongson platforms and supports to use DTS and ACPI framework to
>> register i2c adapter device resources.
>>
>> The Loongson i2c controller supports operating frequencty is 50MHZ
>> and supports the maximum transmission rate is 400kbps.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> There are currently two people submitting a driver for this hardware.
> This is the other driver:
>
> https://lore.kernel.org/all/f6cc2dbe5cd190031ab4f772d1cf250934288546.1669777792.git.zhoubinbin@loongson.cn/
>
> Can you guys please decide which one is "better" or combine the two to
> make a better one?
>
> Thanks,
>
>     Wolfram

thanks your feedback,  please help review zhoubingbing's i2c patch.


Thanks,

Yinbo.

>