[PATCH V5 0/4] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller

[PATCH V5 0/4] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller

[Binbin Zhou]

Hi all:

This patch series adds support for the I2C module found on various
Loongson systems with the Loongson-2K SoC or the Loongson LS7A bridge chip.

For now, the I2C driver is suitable for DT-based or ACPI-based systems.

I have tested on Loongson-3A5000LA+LS7A1000/LS7A2000, Loongson-2K1000LA
and Loongson-2K0500.

Thanks.

Changes since V4:
- patch (1/4)
  - Drop unneeded headers: of.h;
  - xxx_props -> xxx_properties.
- patch (2/4)
  - Add interrupt headers to fix syntax error found by Rob.
- patch (3/4)
  - Drop atmoic loop in ls2x_i2c_master_xfer(), The continuous I2C
    read/write test passes on the corresponding machine;
  - Define the corresponding bits in I2C_LS2X_CTR to avoid magic
    numbers;
  - dev_get_drvdata() is used to get ls2x_i2c_priv() in
    ls2x_i2c_suspend();
  - i2c_add_adapter() -> devm_i2c_add_adapter();
  - SET_SYSTEM_SLEEP_PM_OPS() -> DEFINE_RUNTIME_DEV_PM_OPS();
  - Code formatting, such as alignment.

    Details: https://lore.kernel.org/all/Y4e%2F6KewuHjAluSZ@smile.fi.intel.com/

Changes since V3:
- Addressed all review comments from v3
  - Change the changelog text to make it clearer (1/5);
  - Fix some minor bugs, such as formatting issues (2/5);
  - Fix some formatting issues (3/5);
  - Deep refactoring of code for clarity (4/5).
     Details: https://lore.kernel.org/all/Y4S2cnlAm3YYvZ8E@smile.fi.intel.com/

Thanks to all for their suggestions.

Changes since V2:
- Addressed all review comments from v2
  - Drop of_match_ptr() in i2c-gpio to avoid potential unused warnings
    (1/5);
  - Introduce i2c_gpio_get_props() function as the generic interface
    to get i2c-gpio props from DT or ACPI table (2/5);
  - Refact ls2x i2c code, similar to removing excessive goto tags (4/5).

Thanks to Andy and Mika for their suggestions.

Changes since V1:
- Remove the function of getting the static i2c bus number from ACPI "_UID";
- Fix build warning from kernel test robot.

Binbin Zhou (4):
  i2c: gpio: Add support on ACPI-based system
  dt-bindings: i2c: add Loongson LS2X I2C controller
  i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  LoongArch: Enable LS2X I2C in loongson3_defconfig

 .../bindings/i2c/loongson,ls2x-i2c.yaml       |  51 +++
 arch/loongarch/configs/loongson3_defconfig    |   1 +
 drivers/i2c/busses/Kconfig                    |  11 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-gpio.c                 |  27 +-
 drivers/i2c/busses/i2c-ls2x.c                 | 379 ++++++++++++++++++
 6 files changed, 460 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

-- 
2.31.1

[PATCH V5 1/4] i2c: gpio: Add support on ACPI-based system

[Binbin Zhou]

Add support for the ACPI-based device registration, so that the driver
can be also enabled through ACPI table.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/i2c-gpio.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 0e4385a9bcf7..7bec473cd6b1 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -13,7 +13,6 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
-#include <linux/of.h>
 #include <linux/platform_data/i2c-gpio.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -300,22 +299,23 @@ static inline void i2c_gpio_fault_injector_init(struct platform_device *pdev) {}
 static inline void i2c_gpio_fault_injector_exit(struct platform_device *pdev) {}
 #endif /* CONFIG_I2C_GPIO_FAULT_INJECTOR*/
 
-static void of_i2c_gpio_get_props(struct device_node *np,
-				  struct i2c_gpio_platform_data *pdata)
+/* Get i2c-gpio properties from DT or ACPI table */
+static void i2c_gpio_get_properties(struct device *dev,
+				     struct i2c_gpio_platform_data *pdata)
 {
 	u32 reg;
 
-	of_property_read_u32(np, "i2c-gpio,delay-us", &pdata->udelay);
+	device_property_read_u32(dev, "i2c-gpio,delay-us", &pdata->udelay);
 
-	if (!of_property_read_u32(np, "i2c-gpio,timeout-ms", &reg))
+	if (!device_property_read_u32(dev, "i2c-gpio,timeout-ms", &reg))
 		pdata->timeout = msecs_to_jiffies(reg);
 
 	pdata->sda_is_open_drain =
-		of_property_read_bool(np, "i2c-gpio,sda-open-drain");
+		device_property_read_bool(dev, "i2c-gpio,sda-open-drain");
 	pdata->scl_is_open_drain =
-		of_property_read_bool(np, "i2c-gpio,scl-open-drain");
+		device_property_read_bool(dev, "i2c-gpio,scl-open-drain");
 	pdata->scl_is_output_only =
-		of_property_read_bool(np, "i2c-gpio,scl-output-only");
+		device_property_read_bool(dev, "i2c-gpio,scl-output-only");
 }
 
 static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
@@ -373,8 +373,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
 	bit_data = &priv->bit_data;
 	pdata = &priv->pdata;
 
-	if (np) {
-		of_i2c_gpio_get_props(np, pdata);
+	if (dev_fwnode(dev)) {
+		i2c_gpio_get_properties(dev, pdata);
 	} else {
 		/*
 		 * If all platform data settings are zero it is OK
@@ -489,10 +489,17 @@ static const struct of_device_id i2c_gpio_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
 
+static const struct acpi_device_id i2c_gpio_acpi_match[] = {
+	{ "LOON0005" }, /* LoongArch */
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, i2c_gpio_acpi_match);
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.of_match_table	= i2c_gpio_dt_ids,
+		.acpi_match_table = i2c_gpio_acpi_match,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= i2c_gpio_remove,
-- 
2.31.1

[PATCH V5 2/4] dt-bindings: i2c: add Loongson LS2X I2C controller

[Binbin Zhou]

Add Loongson LS2X I2C controller binding with DT schema format using
json-schema.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/i2c/loongson,ls2x-i2c.yaml       | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
new file mode 100644
index 000000000000..67882ec6e06a
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/loongson,ls2x-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson LS2X I2C Controller
+
+maintainers:
+  - Binbin Zhou <zhoubinbin@loongson.cn>
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - loongson,ls2k-i2c
+      - loongson,ls7a-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    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 = <&extioiic>;
+        interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        eeprom@57 {
+            compatible = "atmel,24c16";
+            reg = <0x57>;
+            pagesize = <16>;
+        };
+    };
-- 
2.31.1

[PATCH V5 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Binbin Zhou]

This I2C module is integrated into the Loongson-2K SoCs and Loongson
LS7A bridge chip.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/Kconfig    |  11 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-ls2x.c | 379 ++++++++++++++++++++++++++++++++++
 3 files changed, 391 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e50f9603d189..908096a762ca 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -888,6 +888,17 @@ config I2C_OWL
 	  Say Y here if you want to use the I2C bus controller on
 	  the Actions Semiconductor Owl SoC's.
 
+config I2C_LS2X
+	tristate "Loongson LS2X I2C adapter"
+	depends on MACH_LOONGSON64 || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C interface on the Loongson-2K SoCs and Loongson LS7A bridge
+	  chip.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called i2c-ls2x.
+
 config I2C_PASEMI
 	tristate "PA Semi SMBus interface"
 	depends on PPC_PASEMI && PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e73cdb1d2b5a..df332ec3c489 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
 obj-$(CONFIG_I2C_NPCM)		+= i2c-npcm7xx.o
+obj-$(CONFIG_I2C_LS2X)		+= i2c-ls2x.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
diff --git a/drivers/i2c/busses/i2c-ls2x.c b/drivers/i2c/busses/i2c-ls2x.c
new file mode 100644
index 000000000000..788296380118
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ls2x.c
@@ -0,0 +1,379 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Loongson-2K/Loongson LS7A I2C master mode driver
+ *
+ * Copyright (C) 2013 Loongson Technology Corporation Limited.
+ * Copyright (C) 2014-2017 Lemote, Inc.
+ * Copyright (C) 2018-2022 Loongson Technology Corporation Limited.
+ *
+ * Originally written by liushaozong
+ *
+ * Rewritten for mainline by Binbin Zhou <zhoubinbin@loongson.cn>
+ */
+
+#include <linux/bits.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
+#include <linux/units.h>
+
+/* I2C Registers */
+#define I2C_LS2X_PRER_LO	0x0 /* Freq Division Low Byte Register */
+#define I2C_LS2X_PRER_HI	0x1 /* Freq Division High Byte Register */
+#define I2C_LS2X_CTR		0x2 /* Control Register */
+#define I2C_LS2X_TXR		0x3 /* Transport Data Register */
+#define I2C_LS2X_RXR		0x3 /* Receive Data Register */
+#define I2C_LS2X_CR		0x4 /* Command Control Register */
+#define I2C_LS2X_SR		0x4 /* State Register */
+
+/* Command Control Register Bit */
+#define LS2X_CR_START		BIT(7) /* Start signal */
+#define LS2X_CR_STOP		BIT(6) /* Stop signal */
+#define LS2X_CR_READ		BIT(5) /* Read signal */
+#define LS2X_CR_WRITE		BIT(4) /* Write signal */
+#define LS2X_CR_ACK		BIT(3) /* Response signal */
+#define LS2X_CR_IACK		BIT(0) /* Interrupt response signal */
+
+/* State Register Bit */
+#define LS2X_SR_NOACK		BIT(7) /* Receive NACK */
+#define LS2X_SR_BUSY		BIT(6) /* Bus busy state */
+#define LS2X_SR_AL		BIT(5) /* Arbitration lost */
+#define LS2X_SR_TIP		BIT(1) /* Transmission state */
+#define LS2X_SR_IF		BIT(0) /* Interrupt flag */
+
+/* Control Register Bit */
+#define LS2X_CTR_EN		BIT(7) /* 0: I2c frequency setting 1: Normal */
+#define LS2X_CTR_IEN		BIT(6) /* Enable i2c interrupt */
+#define LS2X_CTR_MST		BIT(5) /* 0: Slave mode 1: Master mode */
+
+struct ls2x_i2c_priv {
+	struct i2c_adapter	adapter;
+	struct device		*dev;
+	void __iomem		*base;
+	struct i2c_timings	i2c_t;
+	struct completion	cmd_complete;
+	unsigned int		suspended:1;
+};
+
+static int ls2x_i2c_xfer_byte(struct i2c_adapter *adap, u8 txdata, u8 *rxdatap)
+{
+	u8 rxdata;
+	unsigned long time_left;
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	writeb(txdata, priv->base + I2C_LS2X_CR);
+
+	time_left = wait_for_completion_timeout(&priv->cmd_complete,
+						adap->timeout);
+	if (!time_left) {
+		dev_err(&adap->dev, "transaction timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	rxdata = readb(priv->base + I2C_LS2X_SR);
+	if (rxdatap)
+		*rxdatap = rxdata;
+
+	return 0;
+}
+
+static int ls2x_i2c_send_byte(struct i2c_adapter *adap, u8 txdata)
+{
+	int ret;
+	u8 rxdata;
+
+	ret = ls2x_i2c_xfer_byte(adap, txdata, &rxdata);
+	if (ret)
+		return ret;
+
+	if (rxdata & LS2X_SR_NOACK)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int ls2x_i2c_stop(struct i2c_adapter *adap)
+{
+	int ret = ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
+
+	return (ret == -ENXIO) ? 0 : ret;
+}
+
+static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
+{
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	reinit_completion(&priv->cmd_complete);
+
+	writeb(i2c_8bit_addr_from_msg(msgs), priv->base + I2C_LS2X_TXR);
+	return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
+}
+
+static int ls2x_i2c_rx(struct i2c_adapter *adap, u8 *buf, u16 len)
+{
+	unsigned long time_left;
+	int cmd = LS2X_CR_READ;
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	while (len--) {
+		writeb(cmd | (len ? 0 : LS2X_CR_ACK), priv->base + I2C_LS2X_CR);
+
+		time_left = wait_for_completion_timeout(&priv->cmd_complete,
+							adap->timeout);
+		if (unlikely(!time_left)) {
+			dev_err(priv->dev, "transaction timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		*buf++ = readb(priv->base + I2C_LS2X_RXR);
+	}
+
+	return 0;
+}
+
+static int ls2x_i2c_tx(struct i2c_adapter *adap, u8 *buf, u16 len)
+{
+	int ret;
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	while (len--) {
+		writeb(*buf++, priv->base + I2C_LS2X_TXR);
+
+		ret = ls2x_i2c_send_byte(adap, LS2X_CR_WRITE);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ls2x_i2c_xfer_one(struct i2c_adapter *adap,
+			     struct i2c_msg *msg, bool stop)
+{
+	int ret;
+	bool is_read = msg->flags & I2C_M_RD;
+
+	/* Contains steps to send start condition and address */
+	ret = ls2x_i2c_start(adap, msg);
+	if (ret)
+		return ret;
+
+	if (is_read)
+		ret = ls2x_i2c_rx(adap, msg->buf, msg->len);
+	else
+		ret = ls2x_i2c_tx(adap, msg->buf, msg->len);
+
+	/* could not acquire bus. bail out without STOP */
+	if (ret == -EAGAIN)
+		return ret;
+
+	if (stop)
+		ret = ls2x_i2c_stop(adap);
+
+	return ret;
+}
+
+static int ls2x_i2c_master_xfer(struct i2c_adapter *adap,
+				struct i2c_msg *msgs, int num)
+{
+	int ret;
+	struct i2c_msg *msg, *emsg = msgs + num;
+
+	for (msg = msgs; msg < emsg; msg++) {
+		/* Emit STOP if it is the last message or I2C_M_STOP is set */
+		bool stop = (msg + 1 == emsg) || (msg->flags & I2C_M_STOP);
+
+		ret = ls2x_i2c_xfer_one(adap, msg, stop);
+		if (ret)
+			return ret;
+	}
+
+	return num;
+}
+
+static unsigned int ls2x_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm ls2x_i2c_algo = {
+	.master_xfer	= ls2x_i2c_master_xfer,
+	.functionality	= ls2x_i2c_func,
+};
+
+/*
+ * Interrupt service routine.
+ * This gets called whenever an I2C interrupt occurs.
+ */
+static irqreturn_t ls2x_i2c_irq_handler(int this_irq, void *dev_id)
+{
+	struct ls2x_i2c_priv *priv = dev_id;
+
+	if (!(readb(priv->base + I2C_LS2X_SR) & LS2X_SR_IF))
+		return IRQ_NONE;
+
+	writeb(LS2X_CR_IACK, priv->base + I2C_LS2X_CR);
+	complete(&priv->cmd_complete);
+	return IRQ_HANDLED;
+}
+
+/*
+ * The I2C controller has a fixed I2C bus frequency by default, but to
+ * be compatible with more client devices, we can obtain the set I2C
+ * bus frequency from ACPI or FDT.
+ */
+static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
+{
+	u16 val = 0x12c; /* Default value of I2C divider latch register */
+	struct i2c_timings *t = &priv->i2c_t;
+	u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);
+
+	i2c_parse_fw_timings(priv->dev, t, false);
+
+	if (acpi_speed || t->bus_freq_hz)
+		val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
+
+	/* Set LS2X I2C frequency */
+	writel(val, priv->base + I2C_LS2X_PRER_LO);
+}
+
+static void ls2x_i2c_reginit(struct ls2x_i2c_priv *priv)
+{
+	u8 data;
+
+	/* Enable operations about frequency divider register */
+	data = readb(priv->base + I2C_LS2X_CTR);
+	writeb(data & ~LS2X_CTR_EN, priv->base + I2C_LS2X_CTR);
+
+	ls2x_i2c_adjust_bus_speed(priv);
+
+	/* Set to normal I2C operating mode and enable interrupts */
+	data = readb(priv->base + I2C_LS2X_CTR);
+	writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST,
+			priv->base + I2C_LS2X_CTR);
+}
+
+static int ls2x_i2c_probe(struct platform_device *pdev)
+{
+	int r, irq;
+	struct i2c_adapter *adap;
+	struct ls2x_i2c_priv *priv;
+	struct device *dev = &pdev->dev;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/* Map hardware registers */
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
+			     IRQF_SHARED, "ls2x-i2c", priv);
+	if (r < 0)
+		return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
+
+	/* Add the i2c adapter */
+	adap = &priv->adapter;
+	adap->nr = pdev->id;
+	adap->owner = THIS_MODULE;
+	adap->retries = 5;
+	adap->algo = &ls2x_i2c_algo;
+	adap->dev.parent = dev;
+	device_set_node(&adap->dev, dev_fwnode(dev));
+	i2c_set_adapdata(adap, priv);
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+	init_completion(&priv->cmd_complete);
+
+	priv->dev = dev;
+	platform_set_drvdata(pdev, priv);
+
+	ls2x_i2c_reginit(priv);
+
+	r = devm_i2c_add_adapter(dev, adap);
+	if (r)
+		return dev_err_probe(dev, r, "Failure adding adapter\n");
+
+	return 0;
+}
+
+static int ls2x_i2c_remove(struct platform_device *pdev)
+{
+	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&priv->adapter);
+	return 0;
+}
+
+static int ls2x_i2c_suspend(struct device *dev)
+{
+	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);
+
+	priv->suspended = 1;
+	return 0;
+}
+
+static int ls2x_i2c_resume(struct device *dev)
+{
+	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);
+
+	priv->suspended = 0;
+	ls2x_i2c_reginit(priv);
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ls2x_i2c_pm_ops,
+				 ls2x_i2c_suspend, ls2x_i2c_resume, NULL);
+
+static const struct of_device_id ls2x_i2c_id_table[] = {
+	{ .compatible = "loongson,ls2k-i2c" },
+	{ .compatible = "loongson,ls7a-i2c" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ls2x_i2c_id_table);
+
+static const struct acpi_device_id ls2x_i2c_acpi_match[] = {
+	{ "LOON0004" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, ls2x_i2c_acpi_match);
+
+static struct platform_driver ls2x_i2c_driver = {
+	.probe		= ls2x_i2c_probe,
+	.remove		= ls2x_i2c_remove,
+	.driver		= {
+		.name	= "ls2x-i2c",
+		.pm	= pm_sleep_ptr(&ls2x_i2c_pm_ops),
+		.of_match_table = ls2x_i2c_id_table,
+		.acpi_match_table = ls2x_i2c_acpi_match,
+	},
+};
+
+static int __init ls2x_i2c_init_driver(void)
+{
+	return platform_driver_register(&ls2x_i2c_driver);
+}
+subsys_initcall(ls2x_i2c_init_driver);
+
+static void __exit ls2x_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&ls2x_i2c_driver);
+}
+module_exit(ls2x_i2c_exit_driver);
+
+MODULE_DESCRIPTION("Loongson LS2X I2C Bus driver");
+MODULE_AUTHOR("Loongson Technology Corporation Limited");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ls2x-i2c");
-- 
2.31.1

[PATCH V5 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Binbin Zhou]

This I2C module is integrated into the Loongson-2K SoCs and Loongson
LS7A bridge chip.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/Kconfig    |  11 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-ls2x.c | 379 ++++++++++++++++++++++++++++++++++
 3 files changed, 391 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e50f9603d189..908096a762ca 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -888,6 +888,17 @@ config I2C_OWL
 	  Say Y here if you want to use the I2C bus controller on
 	  the Actions Semiconductor Owl SoC's.
 
+config I2C_LS2X
+	tristate "Loongson LS2X I2C adapter"
+	depends on MACH_LOONGSON64 || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C interface on the Loongson-2K SoCs and Loongson LS7A bridge
+	  chip.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called i2c-ls2x.
+
 config I2C_PASEMI
 	tristate "PA Semi SMBus interface"
 	depends on PPC_PASEMI && PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e73cdb1d2b5a..df332ec3c489 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
 obj-$(CONFIG_I2C_NPCM)		+= i2c-npcm7xx.o
+obj-$(CONFIG_I2C_LS2X)		+= i2c-ls2x.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
diff --git a/drivers/i2c/busses/i2c-ls2x.c b/drivers/i2c/busses/i2c-ls2x.c
new file mode 100644
index 000000000000..788296380118
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ls2x.c
@@ -0,0 +1,379 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Loongson-2K/Loongson LS7A I2C master mode driver
+ *
+ * Copyright (C) 2013 Loongson Technology Corporation Limited.
+ * Copyright (C) 2014-2017 Lemote, Inc.
+ * Copyright (C) 2018-2022 Loongson Technology Corporation Limited.
+ *
+ * Originally written by liushaozong
+ *
+ * Rewritten for mainline by Binbin Zhou <zhoubinbin@loongson.cn>
+ */
+
+#include <linux/bits.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
+#include <linux/units.h>
+
+/* I2C Registers */
+#define I2C_LS2X_PRER_LO	0x0 /* Freq Division Low Byte Register */
+#define I2C_LS2X_PRER_HI	0x1 /* Freq Division High Byte Register */
+#define I2C_LS2X_CTR		0x2 /* Control Register */
+#define I2C_LS2X_TXR		0x3 /* Transport Data Register */
+#define I2C_LS2X_RXR		0x3 /* Receive Data Register */
+#define I2C_LS2X_CR		0x4 /* Command Control Register */
+#define I2C_LS2X_SR		0x4 /* State Register */
+
+/* Command Control Register Bit */
+#define LS2X_CR_START		BIT(7) /* Start signal */
+#define LS2X_CR_STOP		BIT(6) /* Stop signal */
+#define LS2X_CR_READ		BIT(5) /* Read signal */
+#define LS2X_CR_WRITE		BIT(4) /* Write signal */
+#define LS2X_CR_ACK		BIT(3) /* Response signal */
+#define LS2X_CR_IACK		BIT(0) /* Interrupt response signal */
+
+/* State Register Bit */
+#define LS2X_SR_NOACK		BIT(7) /* Receive NACK */
+#define LS2X_SR_BUSY		BIT(6) /* Bus busy state */
+#define LS2X_SR_AL		BIT(5) /* Arbitration lost */
+#define LS2X_SR_TIP		BIT(1) /* Transmission state */
+#define LS2X_SR_IF		BIT(0) /* Interrupt flag */
+
+/* Control Register Bit */
+#define LS2X_CTR_EN		BIT(7) /* 0: I2c frequency setting 1: Normal */
+#define LS2X_CTR_IEN		BIT(6) /* Enable i2c interrupt */
+#define LS2X_CTR_MST		BIT(5) /* 0: Slave mode 1: Master mode */
+
+struct ls2x_i2c_priv {
+	struct i2c_adapter	adapter;
+	struct device		*dev;
+	void __iomem		*base;
+	struct i2c_timings	i2c_t;
+	struct completion	cmd_complete;
+	unsigned int		suspended:1;
+};
+
+static int ls2x_i2c_xfer_byte(struct i2c_adapter *adap, u8 txdata, u8 *rxdatap)
+{
+	u8 rxdata;
+	unsigned long time_left;
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	writeb(txdata, priv->base + I2C_LS2X_CR);
+
+	time_left = wait_for_completion_timeout(&priv->cmd_complete,
+						adap->timeout);
+	if (!time_left) {
+		dev_err(&adap->dev, "transaction timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	rxdata = readb(priv->base + I2C_LS2X_SR);
+	if (rxdatap)
+		*rxdatap = rxdata;
+
+	return 0;
+}
+
+static int ls2x_i2c_send_byte(struct i2c_adapter *adap, u8 txdata)
+{
+	int ret;
+	u8 rxdata;
+
+	ret = ls2x_i2c_xfer_byte(adap, txdata, &rxdata);
+	if (ret)
+		return ret;
+
+	if (rxdata & LS2X_SR_NOACK)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int ls2x_i2c_stop(struct i2c_adapter *adap)
+{
+	int ret = ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
+
+	return (ret == -ENXIO) ? 0 : ret;
+}
+
+static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
+{
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	reinit_completion(&priv->cmd_complete);
+
+	writeb(i2c_8bit_addr_from_msg(msgs), priv->base + I2C_LS2X_TXR);
+	return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
+}
+
+static int ls2x_i2c_rx(struct i2c_adapter *adap, u8 *buf, u16 len)
+{
+	unsigned long time_left;
+	int cmd = LS2X_CR_READ;
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	while (len--) {
+		writeb(cmd | (len ? 0 : LS2X_CR_ACK), priv->base + I2C_LS2X_CR);
+
+		time_left = wait_for_completion_timeout(&priv->cmd_complete,
+							adap->timeout);
+		if (unlikely(!time_left)) {
+			dev_err(priv->dev, "transaction timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		*buf++ = readb(priv->base + I2C_LS2X_RXR);
+	}
+
+	return 0;
+}
+
+static int ls2x_i2c_tx(struct i2c_adapter *adap, u8 *buf, u16 len)
+{
+	int ret;
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	while (len--) {
+		writeb(*buf++, priv->base + I2C_LS2X_TXR);
+
+		ret = ls2x_i2c_send_byte(adap, LS2X_CR_WRITE);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ls2x_i2c_xfer_one(struct i2c_adapter *adap,
+			     struct i2c_msg *msg, bool stop)
+{
+	int ret;
+	bool is_read = msg->flags & I2C_M_RD;
+
+	/* Contains steps to send start condition and address */
+	ret = ls2x_i2c_start(adap, msg);
+	if (ret)
+		return ret;
+
+	if (is_read)
+		ret = ls2x_i2c_rx(adap, msg->buf, msg->len);
+	else
+		ret = ls2x_i2c_tx(adap, msg->buf, msg->len);
+
+	/* could not acquire bus. bail out without STOP */
+	if (ret == -EAGAIN)
+		return ret;
+
+	if (stop)
+		ret = ls2x_i2c_stop(adap);
+
+	return ret;
+}
+
+static int ls2x_i2c_master_xfer(struct i2c_adapter *adap,
+				struct i2c_msg *msgs, int num)
+{
+	int ret;
+	struct i2c_msg *msg, *emsg = msgs + num;
+
+	for (msg = msgs; msg < emsg; msg++) {
+		/* Emit STOP if it is the last message or I2C_M_STOP is set */
+		bool stop = (msg + 1 == emsg) || (msg->flags & I2C_M_STOP);
+
+		ret = ls2x_i2c_xfer_one(adap, msg, stop);
+		if (ret)
+			return ret;
+	}
+
+	return num;
+}
+
+static unsigned int ls2x_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm ls2x_i2c_algo = {
+	.master_xfer	= ls2x_i2c_master_xfer,
+	.functionality	= ls2x_i2c_func,
+};
+
+/*
+ * Interrupt service routine.
+ * This gets called whenever an I2C interrupt occurs.
+ */
+static irqreturn_t ls2x_i2c_irq_handler(int this_irq, void *dev_id)
+{
+	struct ls2x_i2c_priv *priv = dev_id;
+
+	if (!(readb(priv->base + I2C_LS2X_SR) & LS2X_SR_IF))
+		return IRQ_NONE;
+
+	writeb(LS2X_CR_IACK, priv->base + I2C_LS2X_CR);
+	complete(&priv->cmd_complete);
+	return IRQ_HANDLED;
+}
+
+/*
+ * The I2C controller has a fixed I2C bus frequency by default, but to
+ * be compatible with more client devices, we can obtain the set I2C
+ * bus frequency from ACPI or FDT.
+ */
+static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
+{
+	u16 val = 0x12c; /* Default value of I2C divider latch register */
+	struct i2c_timings *t = &priv->i2c_t;
+	u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);
+
+	i2c_parse_fw_timings(priv->dev, t, false);
+
+	if (acpi_speed || t->bus_freq_hz)
+		val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
+
+	/* Set LS2X I2C frequency */
+	writel(val, priv->base + I2C_LS2X_PRER_LO);
+}
+
+static void ls2x_i2c_reginit(struct ls2x_i2c_priv *priv)
+{
+	u8 data;
+
+	/* Enable operations about frequency divider register */
+	data = readb(priv->base + I2C_LS2X_CTR);
+	writeb(data & ~LS2X_CTR_EN, priv->base + I2C_LS2X_CTR);
+
+	ls2x_i2c_adjust_bus_speed(priv);
+
+	/* Set to normal I2C operating mode and enable interrupts */
+	data = readb(priv->base + I2C_LS2X_CTR);
+	writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST,
+			priv->base + I2C_LS2X_CTR);
+}
+
+static int ls2x_i2c_probe(struct platform_device *pdev)
+{
+	int r, irq;
+	struct i2c_adapter *adap;
+	struct ls2x_i2c_priv *priv;
+	struct device *dev = &pdev->dev;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/* Map hardware registers */
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
+			     IRQF_SHARED, "ls2x-i2c", priv);
+	if (r < 0)
+		return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
+
+	/* Add the i2c adapter */
+	adap = &priv->adapter;
+	adap->nr = pdev->id;
+	adap->owner = THIS_MODULE;
+	adap->retries = 5;
+	adap->algo = &ls2x_i2c_algo;
+	adap->dev.parent = dev;
+	device_set_node(&adap->dev, dev_fwnode(dev));
+	i2c_set_adapdata(adap, priv);
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+	init_completion(&priv->cmd_complete);
+
+	priv->dev = dev;
+	platform_set_drvdata(pdev, priv);
+
+	ls2x_i2c_reginit(priv);
+
+	r = devm_i2c_add_adapter(dev, adap);
+	if (r)
+		return dev_err_probe(dev, r, "Failure adding adapter\n");
+
+	return 0;
+}
+
+static int ls2x_i2c_remove(struct platform_device *pdev)
+{
+	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&priv->adapter);
+	return 0;
+}
+
+static int ls2x_i2c_suspend(struct device *dev)
+{
+	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);
+
+	priv->suspended = 1;
+	return 0;
+}
+
+static int ls2x_i2c_resume(struct device *dev)
+{
+	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);
+
+	priv->suspended = 0;
+	ls2x_i2c_reginit(priv);
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ls2x_i2c_pm_ops,
+				 ls2x_i2c_suspend, ls2x_i2c_resume, NULL);
+
+static const struct of_device_id ls2x_i2c_id_table[] = {
+	{ .compatible = "loongson,ls2k-i2c" },
+	{ .compatible = "loongson,ls7a-i2c" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ls2x_i2c_id_table);
+
+static const struct acpi_device_id ls2x_i2c_acpi_match[] = {
+	{ "LOON0004" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, ls2x_i2c_acpi_match);
+
+static struct platform_driver ls2x_i2c_driver = {
+	.probe		= ls2x_i2c_probe,
+	.remove		= ls2x_i2c_remove,
+	.driver		= {
+		.name	= "ls2x-i2c",
+		.pm	= pm_sleep_ptr(&ls2x_i2c_pm_ops),
+		.of_match_table = ls2x_i2c_id_table,
+		.acpi_match_table = ls2x_i2c_acpi_match,
+	},
+};
+
+static int __init ls2x_i2c_init_driver(void)
+{
+	return platform_driver_register(&ls2x_i2c_driver);
+}
+subsys_initcall(ls2x_i2c_init_driver);
+
+static void __exit ls2x_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&ls2x_i2c_driver);
+}
+module_exit(ls2x_i2c_exit_driver);
+
+MODULE_DESCRIPTION("Loongson LS2X I2C Bus driver");
+MODULE_AUTHOR("Loongson Technology Corporation Limited");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ls2x-i2c");
-- 
2.31.1

[PATCH V5 4/4] LoongArch: Enable LS2X I2C in loongson3_defconfig

[Binbin Zhou]

This is now supported, enable for Loongson-3 systems.
Other systems are unaffected.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 arch/loongarch/configs/loongson3_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/loongarch/configs/loongson3_defconfig b/arch/loongarch/configs/loongson3_defconfig
index cd8e003d885e..017eb0e36738 100644
--- a/arch/loongarch/configs/loongson3_defconfig
+++ b/arch/loongarch/configs/loongson3_defconfig
@@ -600,6 +600,7 @@ CONFIG_HW_RANDOM_VIRTIO=m
 CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_PIIX4=y
 CONFIG_I2C_GPIO=y
+CONFIG_I2C_LS2X=y
 CONFIG_SPI=y
 CONFIG_GPIO_SYSFS=y
 CONFIG_GPIO_LOONGSON=y
-- 
2.31.1

Re: [PATCH V5 1/4] i2c: gpio: Add support on ACPI-based system

[Andy Shevchenko]

On Tue, Dec 06, 2022 at 11:16:54AM +0800, Binbin Zhou wrote:
> Add support for the ACPI-based device registration, so that the driver
> can be also enabled through ACPI table.

...

> -#include <linux/of.h>

Cool, but don't you need to add property.h?

Otherwise looks good to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V5 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Andy Shevchenko]

On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.

Much better, thanks!

...

> +/*
> + * The I2C controller has a fixed I2C bus frequency by default, but to
> + * be compatible with more client devices, we can obtain the set I2C
> + * bus frequency from ACPI or FDT.
> + */
> +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
> +{
> +	u16 val = 0x12c; /* Default value of I2C divider latch register */

Besides comment better to be placed on top of the commented line, the value
is better to have its own definition where you place the comment and elaborate
what it means in practice (The clock frequency is changed?  Bus speed is
different?)

> +	struct i2c_timings *t = &priv->i2c_t;
> +	u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);
> +
> +	i2c_parse_fw_timings(priv->dev, t, false);
> +
> +	if (acpi_speed || t->bus_freq_hz)
> +		val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
> +
> +	/* Set LS2X I2C frequency */
> +	writel(val, priv->base + I2C_LS2X_PRER_LO);
> +}

...

> +	writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST,
> +			priv->base + I2C_LS2X_CTR);

Wrong indentation.

...

> +	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
> +			     IRQF_SHARED, "ls2x-i2c", priv);
> +	if (r < 0)
> +		return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);

You requested IRQ without filling all data structures. Is it fine? Have you
checked that with CONFIG_DEBUG_SHIRQ being enabled?

...

> +	r = devm_i2c_add_adapter(dev, adap);
> +	if (r)
> +		return dev_err_probe(dev, r, "Failure adding adapter\n");
> +
> +	return 0;
> +}

Looking at the above...

> +static int ls2x_i2c_remove(struct platform_device *pdev)
> +{
> +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&priv->adapter);

...are you sure this is correct?

> +	return 0;
> +}

...

> +static int ls2x_i2c_suspend(struct device *dev)
> +{
> +	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);

> +	priv->suspended = 1;

No protection needed?

> +	return 0;
> +}
> +
> +static int ls2x_i2c_resume(struct device *dev)
> +{
> +	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);

> +	priv->suspended = 0;

Ditto.

> +	ls2x_i2c_reginit(priv);
> +	return 0;
> +}

...

> +MODULE_ALIAS("platform:ls2x-i2c");

Why is this required?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V5 1/4] i2c: gpio: Add support on ACPI-based system

[Binbin Zhou]

在 2022/12/6 23:13, Andy Shevchenko 写道:
> On Tue, Dec 06, 2022 at 11:16:54AM +0800, Binbin Zhou wrote:
>> Add support for the ACPI-based device registration, so that the driver
>> can be also enabled through ACPI table.
> ...
>
>> -#include <linux/of.h>
> Cool, but don't you need to add property.h?

Get it, I should have been more careful, maybe I'm still not very 
sensitive to header files.

Now I'm still basically judging based on whether I compile with errors 
or not.

Thanks.

Binbin

>
> Otherwise looks good to me.
>

Re: [PATCH V5 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Binbin Zhou]

Hi Andy:

在 2022/12/6 23:23, Andy Shevchenko 写道:
> On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote:
>> This I2C module is integrated into the Loongson-2K SoCs and Loongson
>> LS7A bridge chip.
> Much better, thanks!
>
> ...
>
>> +/*
>> + * The I2C controller has a fixed I2C bus frequency by default, but to
>> + * be compatible with more client devices, we can obtain the set I2C
>> + * bus frequency from ACPI or FDT.
>> + */
>> +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
>> +{
>> +	u16 val = 0x12c; /* Default value of I2C divider latch register */
> Besides comment better to be placed on top of the commented line, the value
> is better to have its own definition where you place the comment and elaborate
> what it means in practice (The clock frequency is changed?  Bus speed is
> different?)

Ok, I'll put this comment on a separate line.

The LS2X I2C supports STANDARD_MODE and FAST_MODE, so the maximum bus 
frequency is 400kHz.
"0x12c" is our empirical value after experimentation and represents 33KHz.

Also, I think the better way is:

@@ -53,6 +53,15 @@
  #define LS2X_CTR_IEN           BIT(6) /* Enable i2c interrupt */
  #define LS2X_CTR_MST           BIT(5) /* 0: Slave mode 1: Master mode */

+/* The PCLK clock frequency input from the LPB bus */
+#define LS2X_I2C_PCLK_FREQ     (50 * HZ_PER_MHZ)
+/*
+ * The LS2X I2C controller supports standard mode and fast mode,
+ * so the maximum bus frequency is 400kHz.
+ * The '33KHz' is our empirical value after experimentation.
+ */
+#define LS2X_I2C_FREQ_STD      (33 * HZ_PER_KHZ)
+
  struct ls2x_i2c_priv {
         struct i2c_adapter      adapter;
         struct device           *dev;
@@ -231,17 +240,19 @@ static irqreturn_t ls2x_i2c_irq_handler(int 
this_irq, void *dev_id)
   */
  static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
  {
-       u16 val = 0x12c; /* Default value of I2C divider latch register */
         struct i2c_timings *t = &priv->i2c_t;
         u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);

         i2c_parse_fw_timings(priv->dev, t, false);

         if (acpi_speed || t->bus_freq_hz)
-               val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
+               t->bus_freq_hz = max(t->bus_freq_hz, acpi_speed);
+       else
+               t->bus_freq_hz = LS2X_I2C_FREQ_STD;

-       /* Set LS2X I2C frequency */
-       writel(val, priv->base + I2C_LS2X_PRER_LO);
+       /* Calculate and set LS2X I2C frequency */
+       writel((LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1),
+              priv->base + I2C_LS2X_PRER_LO);
  }

>> +	struct i2c_timings *t = &priv->i2c_t;
>> +	u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);
>> +
>> +	i2c_parse_fw_timings(priv->dev, t, false);
>> +
>> +	if (acpi_speed || t->bus_freq_hz)
>> +		val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
>> +
>> +	/* Set LS2X I2C frequency */
>> +	writel(val, priv->base + I2C_LS2X_PRER_LO);
>> +}
> ...
>
>> +	writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST,
>> +			priv->base + I2C_LS2X_CTR);
> Wrong indentation.
>
> ...
>
>> +	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
>> +			     IRQF_SHARED, "ls2x-i2c", priv);
>> +	if (r < 0)
>> +		return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
> You requested IRQ without filling all data structures. Is it fine? Have you
> checked that with CONFIG_DEBUG_SHIRQ being enabled?

Sorry, I don't quite understand what you mean by "without filling all 
data structures", I need call ls2x_i2c_reginit(priv) before it ?

I see that other i2c drivers request interrupts at about the same time 
as I do.

I tested it with CONFIG_DEBUG_SHIRQ and no exceptions were reported.


> ...
>
>> +	r = devm_i2c_add_adapter(dev, adap);
>> +	if (r)
>> +		return dev_err_probe(dev, r, "Failure adding adapter\n");
>> +
>> +	return 0;
>> +}
> Looking at the above...
>
>> +static int ls2x_i2c_remove(struct platform_device *pdev)
>> +{
>> +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
>> +
>> +	i2c_del_adapter(&priv->adapter);
> ...are you sure this is correct?

When we use devm_i2c_add_adapter(), the adapter will be auto deleted on 
driver detach.

So I just drop the ls2x_i2c_remove() ?

>> +	return 0;
>> +}
> ...
>
>> +static int ls2x_i2c_suspend(struct device *dev)
>> +{
>> +	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);
>> +	priv->suspended = 1;
> No protection needed?

Actually this variable is not used elsewhere, maybe it is useless, I 
will try to remove it and add some necessary actions in the 
suspend/rusume callbacks, such as disable i2c interrupts, to ensure 
integrity.


>> +	return 0;
>> +}
>> +
>> +static int ls2x_i2c_resume(struct device *dev)
>> +{
>> +	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);
>> +	priv->suspended = 0;
> Ditto.
>
>> +	ls2x_i2c_reginit(priv);
>> +	return 0;
>> +}
> ...
>
>> +MODULE_ALIAS("platform:ls2x-i2c");
> Why is this required?

I just referred to other drivers before, and now the MODULE_DEVICE_TABLE 
already creates proper alias for platform driver.

I will drop it.

Thanks.

Binbin

Re: [PATCH V5 1/4] i2c: gpio: Add support on ACPI-based system

[Andy Shevchenko]

On Wed, Dec 07, 2022 at 03:10:43PM +0800, Binbin Zhou wrote:
> 在 2022/12/6 23:13, Andy Shevchenko 写道:
> > On Tue, Dec 06, 2022 at 11:16:54AM +0800, Binbin Zhou wrote:

...

> > > -#include <linux/of.h>
> > Cool, but don't you need to add property.h?
> 
> Get it, I should have been more careful, maybe I'm still not very sensitive
> to header files.
> 
> Now I'm still basically judging based on whether I compile with errors or
> not.

You can generate the include tree by running `make includecheck`

The rule of thumb is to include what you are the direct user of.
With a few exceptions where we have the guarantees that one header
is always included by another (e.g. bits.h included by bitops.h).

> > Otherwise looks good to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V5 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Andy Shevchenko]

On Thu, Dec 08, 2022 at 04:55:39PM +0800, Binbin Zhou wrote:
> 在 2022/12/6 23:23, Andy Shevchenko 写道:
> > On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote:

...

> > > +/*
> > > + * The I2C controller has a fixed I2C bus frequency by default, but to
> > > + * be compatible with more client devices, we can obtain the set I2C
> > > + * bus frequency from ACPI or FDT.
> > > + */
> > > +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
> > > +{
> > > +	u16 val = 0x12c; /* Default value of I2C divider latch register */
> > Besides comment better to be placed on top of the commented line, the value
> > is better to have its own definition where you place the comment and elaborate
> > what it means in practice (The clock frequency is changed?  Bus speed is
> > different?)
> 
> Ok, I'll put this comment on a separate line.
> 
> The LS2X I2C supports STANDARD_MODE and FAST_MODE, so the maximum bus
> frequency is 400kHz.
> "0x12c" is our empirical value after experimentation and represents 33KHz.
> 
> Also, I think the better way is:
> 
> @@ -53,6 +53,15 @@
>  #define LS2X_CTR_IEN           BIT(6) /* Enable i2c interrupt */
>  #define LS2X_CTR_MST           BIT(5) /* 0: Slave mode 1: Master mode */
> 
> +/* The PCLK clock frequency input from the LPB bus */
> +#define LS2X_I2C_PCLK_FREQ     (50 * HZ_PER_MHZ)
> +/*
> + * The LS2X I2C controller supports standard mode and fast mode,
> + * so the maximum bus frequency is 400kHz.
> + * The '33KHz' is our empirical value after experimentation.
> + */
> +#define LS2X_I2C_FREQ_STD      (33 * HZ_PER_KHZ)
> +
>  struct ls2x_i2c_priv {
>         struct i2c_adapter      adapter;
>         struct device           *dev;
> @@ -231,17 +240,19 @@ static irqreturn_t ls2x_i2c_irq_handler(int this_irq,
> void *dev_id)
>   */
>  static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
>  {
> -       u16 val = 0x12c; /* Default value of I2C divider latch register */
>         struct i2c_timings *t = &priv->i2c_t;
>         u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);
> 
>         i2c_parse_fw_timings(priv->dev, t, false);
> 
>         if (acpi_speed || t->bus_freq_hz)
> -               val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
> +               t->bus_freq_hz = max(t->bus_freq_hz, acpi_speed);
> +       else
> +               t->bus_freq_hz = LS2X_I2C_FREQ_STD;
> 
> -       /* Set LS2X I2C frequency */
> -       writel(val, priv->base + I2C_LS2X_PRER_LO);
> +       /* Calculate and set LS2X I2C frequency */

> +       writel((LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1),

Fine with me, but drop unneeded parentheses.

> +              priv->base + I2C_LS2X_PRER_LO);
>  }

> > > +	struct i2c_timings *t = &priv->i2c_t;
> > > +	u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);
> > > +
> > > +	i2c_parse_fw_timings(priv->dev, t, false);
> > > +
> > > +	if (acpi_speed || t->bus_freq_hz)
> > > +		val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
> > > +
> > > +	/* Set LS2X I2C frequency */
> > > +	writel(val, priv->base + I2C_LS2X_PRER_LO);
> > > +}

...

> > > +	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
> > > +			     IRQF_SHARED, "ls2x-i2c", priv);
> > > +	if (r < 0)
> > > +		return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
> > You requested IRQ without filling all data structures. Is it fine? Have you
> > checked that with CONFIG_DEBUG_SHIRQ being enabled?
> 
> Sorry, I don't quite understand what you mean by "without filling all data
> structures", I need call ls2x_i2c_reginit(priv) before it ?

When you register an IRQ handler (which is that call) it needs to be prepared
to handle interrupt immediately. Which means that your data structures has to
be filled properly. If you can guarantee that with the current code, fine then.

> I see that other i2c drivers request interrupts at about the same time as I
> do.
> 
> I tested it with CONFIG_DEBUG_SHIRQ and no exceptions were reported.

Good. And you removed and reinserted module?

At least this helps to detect some of the potential issues.

...

> > > +	r = devm_i2c_add_adapter(dev, adap);
> > > +	if (r)
> > > +		return dev_err_probe(dev, r, "Failure adding adapter\n");
> > > +
> > > +	return 0;
> > > +}
> > Looking at the above...
> > 
> > > +static int ls2x_i2c_remove(struct platform_device *pdev)
> > > +{
> > > +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
> > > +
> > > +	i2c_del_adapter(&priv->adapter);
> > ...are you sure this is correct?
> 
> When we use devm_i2c_add_adapter(), the adapter will be auto deleted on
> driver detach.
> 
> So I just drop the ls2x_i2c_remove() ?

Correct.

> > > +	return 0;
> > > +}

...

> > > +static int ls2x_i2c_suspend(struct device *dev)
> > > +{
> > > +	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);
> > > +	priv->suspended = 1;
> > No protection needed?
> 
> Actually this variable is not used elsewhere, maybe it is useless, I will
> try to remove it and add some necessary actions in the suspend/rusume
> callbacks, such as disable i2c interrupts, to ensure integrity.

Is your interrupt a wake source? It might be that you will need a special
handling of it.

> > > +	return 0;
> > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V5 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Binbin Zhou]

Hi Andy:

On Mon, Dec 12, 2022 at 5:47 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 08, 2022 at 04:55:39PM +0800, Binbin Zhou wrote:
> > 在 2022/12/6 23:23, Andy Shevchenko 写道:
> > > On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote:
>
> ...
>
> > > > +/*
> > > > + * The I2C controller has a fixed I2C bus frequency by default, but to
> > > > + * be compatible with more client devices, we can obtain the set I2C
> > > > + * bus frequency from ACPI or FDT.
> > > > + */
> > > > +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
> > > > +{
> > > > + u16 val = 0x12c; /* Default value of I2C divider latch register */
> > > Besides comment better to be placed on top of the commented line, the value
> > > is better to have its own definition where you place the comment and elaborate
> > > what it means in practice (The clock frequency is changed?  Bus speed is
> > > different?)
> >
> > Ok, I'll put this comment on a separate line.
> >
> > The LS2X I2C supports STANDARD_MODE and FAST_MODE, so the maximum bus
> > frequency is 400kHz.
> > "0x12c" is our empirical value after experimentation and represents 33KHz.
> >
> > Also, I think the better way is:
> >
> > @@ -53,6 +53,15 @@
> >  #define LS2X_CTR_IEN           BIT(6) /* Enable i2c interrupt */
> >  #define LS2X_CTR_MST           BIT(5) /* 0: Slave mode 1: Master mode */
> >
> > +/* The PCLK clock frequency input from the LPB bus */
> > +#define LS2X_I2C_PCLK_FREQ     (50 * HZ_PER_MHZ)
> > +/*
> > + * The LS2X I2C controller supports standard mode and fast mode,
> > + * so the maximum bus frequency is 400kHz.
> > + * The '33KHz' is our empirical value after experimentation.
> > + */
> > +#define LS2X_I2C_FREQ_STD      (33 * HZ_PER_KHZ)
> > +
> >  struct ls2x_i2c_priv {
> >         struct i2c_adapter      adapter;
> >         struct device           *dev;
> > @@ -231,17 +240,19 @@ static irqreturn_t ls2x_i2c_irq_handler(int this_irq,
> > void *dev_id)
> >   */
> >  static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
> >  {
> > -       u16 val = 0x12c; /* Default value of I2C divider latch register */
> >         struct i2c_timings *t = &priv->i2c_t;
> >         u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);
> >
> >         i2c_parse_fw_timings(priv->dev, t, false);
> >
> >         if (acpi_speed || t->bus_freq_hz)
> > -               val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
> > +               t->bus_freq_hz = max(t->bus_freq_hz, acpi_speed);
> > +       else
> > +               t->bus_freq_hz = LS2X_I2C_FREQ_STD;
> >
> > -       /* Set LS2X I2C frequency */
> > -       writel(val, priv->base + I2C_LS2X_PRER_LO);
> > +       /* Calculate and set LS2X I2C frequency */
>
> > +       writel((LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1),
>
> Fine with me, but drop unneeded parentheses.
>
> > +              priv->base + I2C_LS2X_PRER_LO);
> >  }
>
> > > > + struct i2c_timings *t = &priv->i2c_t;
> > > > + u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);
> > > > +
> > > > + i2c_parse_fw_timings(priv->dev, t, false);
> > > > +
> > > > + if (acpi_speed || t->bus_freq_hz)
> > > > +         val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
> > > > +
> > > > + /* Set LS2X I2C frequency */
> > > > + writel(val, priv->base + I2C_LS2X_PRER_LO);
> > > > +}
>
> ...
>
> > > > + r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
> > > > +                      IRQF_SHARED, "ls2x-i2c", priv);
> > > > + if (r < 0)
> > > > +         return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
> > > You requested IRQ without filling all data structures. Is it fine? Have you
> > > checked that with CONFIG_DEBUG_SHIRQ being enabled?
> >
> > Sorry, I don't quite understand what you mean by "without filling all data
> > structures", I need call ls2x_i2c_reginit(priv) before it ?
>
> When you register an IRQ handler (which is that call) it needs to be prepared
> to handle interrupt immediately. Which means that your data structures has to
> be filled properly. If you can guarantee that with the current code, fine then.

Emm. If so, I think it might be safer to put ls2x_i2c_reginit() in
front of it, since the bus frequency needs to be set correctly.

>
> > I see that other i2c drivers request interrupts at about the same time as I
> > do.
> >
> > I tested it with CONFIG_DEBUG_SHIRQ and no exceptions were reported.
>
> Good. And you removed and reinserted module?
>
> At least this helps to detect some of the potential issues.

Yes, all of these seem normal.

>
> ...
>
> > > > + r = devm_i2c_add_adapter(dev, adap);
> > > > + if (r)
> > > > +         return dev_err_probe(dev, r, "Failure adding adapter\n");
> > > > +
> > > > + return 0;
> > > > +}
> > > Looking at the above...
> > >
> > > > +static int ls2x_i2c_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
> > > > +
> > > > + i2c_del_adapter(&priv->adapter);
> > > ...are you sure this is correct?
> >
> > When we use devm_i2c_add_adapter(), the adapter will be auto deleted on
> > driver detach.
> >
> > So I just drop the ls2x_i2c_remove() ?
>
> Correct.
>
> > > > + return 0;
> > > > +}
>
> ...
>
> > > > +static int ls2x_i2c_suspend(struct device *dev)
> > > > +{
> > > > + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);
> > > > + priv->suspended = 1;
> > > No protection needed?
.> >
> > Actually this variable is not used elsewhere, maybe it is useless, I will
> > try to remove it and add some necessary actions in the suspend/rusume
> > callbacks, such as disable i2c interrupts, to ensure integrity.
>
> Is your interrupt a wake source? It might be that you will need a special
> handling of it.

No. It isn't a wake source.
I think it is enough to clear the interrupt enable bit in the control register.
As follows:
writeb(readb(priv->base + I2C_LS2X_CTR) & ~LS2X_CTR_IEN,
               priv->base + I2C_LS2X_CTR);

Thanks.
Binbin


>
> > > > + return 0;
> > > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>