devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V4 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller
@ 2022-11-30  5:55 Binbin Zhou
  2022-11-30  5:55 ` [PATCH V4 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids' Binbin Zhou
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Binbin Zhou @ 2022-11-30  5:55 UTC (permalink / raw)
  To: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c
  Cc: loongarch, devicetree, Huacai Chen, WANG Xuerui, Andy Shevchenko,
	Arnd Bergmann, Rob Herring, Krzysztof Kozlowski, Jianmin Lv,
	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 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, such as dropping quotes and checking with
    dt_binding_check (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 (5):
  i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids'
  i2c: gpio: Add support on ACPI-based system
  dt-bindings: i2c: add bindings for Loongson LS2X I2C
  i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  LoongArch: Enable LS2X I2C in loongson3_defconfig

 .../bindings/i2c/loongson,ls2x-i2c.yaml       |  49 +++
 arch/loongarch/configs/loongson3_defconfig    |   1 +
 drivers/i2c/busses/Kconfig                    |  11 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-gpio.c                 |  30 +-
 drivers/i2c/busses/i2c-ls2x.c                 | 402 ++++++++++++++++++
 6 files changed, 482 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

-- 
2.31.1


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

* [PATCH V4 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids'
  2022-11-30  5:55 [PATCH V4 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller Binbin Zhou
@ 2022-11-30  5:55 ` Binbin Zhou
  2022-11-30 20:29   ` Andy Shevchenko
  2022-12-05  9:29   ` Wolfram Sang
  2022-11-30  5:55 ` [PATCH V4 2/5] i2c: gpio: Add support on ACPI-based system Binbin Zhou
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 18+ messages in thread
From: Binbin Zhou @ 2022-11-30  5:55 UTC (permalink / raw)
  To: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c
  Cc: loongarch, devicetree, Huacai Chen, WANG Xuerui, Andy Shevchenko,
	Arnd Bergmann, Rob Herring, Krzysztof Kozlowski, Jianmin Lv,
	Binbin Zhou

Dropping a matching #ifdef check along with dropping of_match_ptr()
is just a cleanup, while dropping of_match_ptr() that has no
corresponding #ifdef fixes an actual warning.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/i2c-gpio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index b1985c1667e1..0e4385a9bcf7 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -482,19 +482,17 @@ static int i2c_gpio_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#if defined(CONFIG_OF)
 static const struct of_device_id i2c_gpio_dt_ids[] = {
 	{ .compatible = "i2c-gpio", },
 	{ /* sentinel */ }
 };
 
 MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
-#endif
 
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
-		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
+		.of_match_table	= i2c_gpio_dt_ids,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= i2c_gpio_remove,
-- 
2.31.1


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

* [PATCH V4 2/5] i2c: gpio: Add support on ACPI-based system
  2022-11-30  5:55 [PATCH V4 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller Binbin Zhou
  2022-11-30  5:55 ` [PATCH V4 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids' Binbin Zhou
@ 2022-11-30  5:55 ` Binbin Zhou
  2022-11-30 20:32   ` Andy Shevchenko
  2022-11-30  5:55 ` [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C Binbin Zhou
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Binbin Zhou @ 2022-11-30  5:55 UTC (permalink / raw)
  To: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c
  Cc: loongarch, devicetree, Huacai Chen, WANG Xuerui, Andy Shevchenko,
	Arnd Bergmann, Rob Herring, Krzysztof Kozlowski, Jianmin Lv,
	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 | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 0e4385a9bcf7..33634d5e4c35 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -300,22 +300,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 props from DT or ACPI table */
+static void i2c_gpio_get_props(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 +374,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_props(dev, pdata);
 	} else {
 		/*
 		 * If all platform data settings are zero it is OK
@@ -489,10 +490,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


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

* [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C
  2022-11-30  5:55 [PATCH V4 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller Binbin Zhou
  2022-11-30  5:55 ` [PATCH V4 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids' Binbin Zhou
  2022-11-30  5:55 ` [PATCH V4 2/5] i2c: gpio: Add support on ACPI-based system Binbin Zhou
@ 2022-11-30  5:55 ` Binbin Zhou
  2022-12-02  8:57   ` Krzysztof Kozlowski
  2022-11-30  5:56 ` [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller Binbin Zhou
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Binbin Zhou @ 2022-11-30  5:55 UTC (permalink / raw)
  To: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c
  Cc: loongarch, devicetree, Huacai Chen, WANG Xuerui, Andy Shevchenko,
	Arnd Bergmann, Rob Herring, Krzysztof Kozlowski, Jianmin Lv,
	Binbin Zhou

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

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/i2c/loongson,ls2x-i2c.yaml       | 49 +++++++++++++++++++
 1 file changed, 49 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..5f36871e272f
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
@@ -0,0 +1,49 @@
+# 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:
+  - |
+    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


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

* [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  2022-11-30  5:55 [PATCH V4 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller Binbin Zhou
                   ` (2 preceding siblings ...)
  2022-11-30  5:55 ` [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C Binbin Zhou
@ 2022-11-30  5:56 ` Binbin Zhou
  2022-11-30 20:41   ` Andy Shevchenko
  2022-12-05  8:38   ` Wolfram Sang
  2022-11-30  5:56 ` [PATCH V4 5/5] LoongArch: Enable LS2X I2C in loongson3_defconfig Binbin Zhou
  2022-11-30 13:47 ` [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C Rob Herring
  5 siblings, 2 replies; 18+ messages in thread
From: Binbin Zhou @ 2022-11-30  5:56 UTC (permalink / raw)
  To: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c
  Cc: loongarch, devicetree, Huacai Chen, WANG Xuerui, Andy Shevchenko,
	Arnd Bergmann, Rob Herring, Krzysztof Kozlowski, Jianmin Lv,
	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 | 402 ++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e50f9603d189..0e342cd5b079 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..b532e672dba5
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ls2x.c
@@ -0,0 +1,402 @@
+// 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/delay.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/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)
+#define LS2X_SR_BUSY		BIT(6)
+#define LS2X_SR_AL		BIT(5)
+#define LS2X_SR_TIP		BIT(1)
+#define LS2X_SR_IF		BIT(0)
+
+#define LS2X_I2C_MAX_RETRIES	5
+
+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 (unlikely(!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_doxfer(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 int ls2x_i2c_xfer(struct i2c_adapter *adap,
+			struct i2c_msg *msgs, int num)
+{
+	int ret, retry;
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	for (retry = 0; retry < adap->retries; retry++) {
+		ret = ls2x_i2c_doxfer(adap, msgs, num);
+		if (ret != -EAGAIN)
+			return ret;
+
+		dev_dbg(priv->dev, "Retrying transmission (%d)\n", retry);
+		udelay(100);
+	}
+
+	return -EREMOTEIO;
+}
+
+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_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 & ~0x80, 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 | 0xe0, 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;
+
+	platform_set_drvdata(pdev, priv);
+	init_completion(&priv->cmd_complete);
+	priv->dev = dev;
+
+	/* 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);
+
+	ls2x_i2c_reginit(priv);
+
+	/* Add the i2c adapter */
+	adap = &priv->adapter;
+	i2c_set_adapdata(adap, priv);
+	adap->nr = pdev->id;
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON;
+	adap->retries = LS2X_I2C_MAX_RETRIES;
+	adap->algo = &ls2x_i2c_algo;
+	adap->dev.parent = dev;
+	adap->dev.of_node = pdev->dev.of_node;
+	device_set_node(&adap->dev, dev_fwnode(dev));
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+
+	/* i2c device drivers may be active on return from add_adapter() */
+	r = i2c_add_adapter(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 platform_device *pdev = to_platform_device(dev);
+	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
+
+	priv->suspended = 1;
+
+	return 0;
+}
+
+static int ls2x_i2c_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
+
+	priv->suspended = 0;
+	ls2x_i2c_reginit(priv);
+
+	return 0;
+}
+
+static const struct dev_pm_ops ls2x_i2c_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend, ls2x_i2c_resume)
+};
+
+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


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

* [PATCH V4 5/5] LoongArch: Enable LS2X I2C in loongson3_defconfig
  2022-11-30  5:55 [PATCH V4 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller Binbin Zhou
                   ` (3 preceding siblings ...)
  2022-11-30  5:56 ` [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller Binbin Zhou
@ 2022-11-30  5:56 ` Binbin Zhou
  2022-11-30 13:47 ` [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C Rob Herring
  5 siblings, 0 replies; 18+ messages in thread
From: Binbin Zhou @ 2022-11-30  5:56 UTC (permalink / raw)
  To: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c
  Cc: loongarch, devicetree, Huacai Chen, WANG Xuerui, Andy Shevchenko,
	Arnd Bergmann, Rob Herring, Krzysztof Kozlowski, Jianmin Lv,
	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


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

* Re: [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C
  2022-11-30  5:55 [PATCH V4 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller Binbin Zhou
                   ` (4 preceding siblings ...)
  2022-11-30  5:56 ` [PATCH V4 5/5] LoongArch: Enable LS2X I2C in loongson3_defconfig Binbin Zhou
@ 2022-11-30 13:47 ` Rob Herring
  5 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2022-11-30 13:47 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: WANG Xuerui, Wolfram Sang, Mika Westerberg, Arnd Bergmann,
	Jianmin Lv, linux-i2c, Andy Shevchenko, Huacai Chen, loongarch,
	Rob Herring, Wolfram Sang, devicetree, Krzysztof Kozlowski


On Wed, 30 Nov 2022 13:55:53 +0800, Binbin Zhou wrote:
> Add Loongson LS2X I2C controller binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  .../bindings/i2c/loongson,ls2x-i2c.yaml       | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.example.dts:25.30-31 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/8503ed054d5b4984b5d6e18891767cace2d36a31.1669777792.git.zhoubinbin@loongson.cn

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


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

* Re: [PATCH V4 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids'
  2022-11-30  5:55 ` [PATCH V4 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids' Binbin Zhou
@ 2022-11-30 20:29   ` Andy Shevchenko
  2022-12-05  9:29   ` Wolfram Sang
  1 sibling, 0 replies; 18+ messages in thread
From: Andy Shevchenko @ 2022-11-30 20:29 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c, loongarch,
	devicetree, Huacai Chen, WANG Xuerui, Arnd Bergmann, Rob Herring,
	Krzysztof Kozlowski, Jianmin Lv

On Wed, Nov 30, 2022 at 01:55:51PM +0800, Binbin Zhou wrote:
> Dropping a matching #ifdef check along with dropping of_match_ptr()
> is just a cleanup, while dropping of_match_ptr() that has no
> corresponding #ifdef fixes an actual warning.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  drivers/i2c/busses/i2c-gpio.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index b1985c1667e1..0e4385a9bcf7 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -482,19 +482,17 @@ static int i2c_gpio_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#if defined(CONFIG_OF)
>  static const struct of_device_id i2c_gpio_dt_ids[] = {
>  	{ .compatible = "i2c-gpio", },
>  	{ /* sentinel */ }
>  };
>  
>  MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
> -#endif
>  
>  static struct platform_driver i2c_gpio_driver = {
>  	.driver		= {
>  		.name	= "i2c-gpio",
> -		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
> +		.of_match_table	= i2c_gpio_dt_ids,
>  	},
>  	.probe		= i2c_gpio_probe,
>  	.remove		= i2c_gpio_remove,
> -- 
> 2.31.1
> 

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH V4 2/5] i2c: gpio: Add support on ACPI-based system
  2022-11-30  5:55 ` [PATCH V4 2/5] i2c: gpio: Add support on ACPI-based system Binbin Zhou
@ 2022-11-30 20:32   ` Andy Shevchenko
  0 siblings, 0 replies; 18+ messages in thread
From: Andy Shevchenko @ 2022-11-30 20:32 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c, loongarch,
	devicetree, Huacai Chen, WANG Xuerui, Arnd Bergmann, Rob Herring,
	Krzysztof Kozlowski, Jianmin Lv

On Wed, Nov 30, 2022 at 01:55:52PM +0800, Binbin Zhou wrote:
> Add support for the ACPI-based device registration, so that the driver
> can be also enabled through ACPI table.

...

The of.h is left with this patch. Is it still needed?

...

> +/* Get i2c-gpio props from DT or ACPI table */

properties

> +static void i2c_gpio_get_props(struct device *dev,

_properties

> +				struct i2c_gpio_platform_data *pdata)

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  2022-11-30  5:56 ` [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller Binbin Zhou
@ 2022-11-30 20:41   ` Andy Shevchenko
  2022-12-02  3:22     ` Binbin Zhou
  2022-12-05  8:38   ` Wolfram Sang
  1 sibling, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2022-11-30 20:41 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c, loongarch,
	devicetree, Huacai Chen, WANG Xuerui, Arnd Bergmann, Rob Herring,
	Krzysztof Kozlowski, Jianmin Lv

On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.

Looks like some of my comments were not addressed. Are you sure
you have read the previous reviews carefully?

...

> +static int ls2x_i2c_xfer(struct i2c_adapter *adap,
> +			struct i2c_msg *msgs, int num)
> +{
> +	int ret, retry;
> +	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
> +
> +	for (retry = 0; retry < adap->retries; retry++) {
> +		ret = ls2x_i2c_doxfer(adap, msgs, num);
> +		if (ret != -EAGAIN)
> +			return ret;
> +
> +		dev_dbg(priv->dev, "Retrying transmission (%d)\n", retry);

> +		udelay(100);

Why atomic? This long (esp. atomic) delay must be explained.

> +	}
> +
> +	return -EREMOTEIO;
> +}

...

> +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 & ~0x80, priv->base + I2C_LS2X_CTR);

Magic number.

> +	ls2x_i2c_adjust_bus_speed(priv);
> +
> +	/* Set to normal I2C operating mode and enable interrupts */
> +	data = readb(priv->base + I2C_LS2X_CTR);
> +	writeb(data | 0xe0, priv->base + I2C_LS2X_CTR);

Ditto.

> +}

...

> +	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
> +				IRQF_SHARED, "ls2x-i2c", priv);

Indentation.

> +	if (r < 0)
> +		return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);

...

> +	adap->dev.of_node = pdev->dev.of_node;

Why is this needed?

> +	device_set_node(&adap->dev, dev_fwnode(dev));

...

> +	/* i2c device drivers may be active on return from add_adapter() */
> +	r = i2c_add_adapter(adap);

Why not use devm_ variant of this?

> +	if (r)
> +		return dev_err_probe(dev, r, "Failure adding adapter\n");

...

> +static int ls2x_i2c_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);

Can't you use dev_get_drvdata() directly? Why?

> +
> +	priv->suspended = 1;
> +
> +	return 0;
> +}
> +
> +static int ls2x_i2c_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);

Ditto.

> +	priv->suspended = 0;
> +	ls2x_i2c_reginit(priv);
> +
> +	return 0;
> +}

...

> +static const struct dev_pm_ops ls2x_i2c_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend, ls2x_i2c_resume)
> +};

Use corresponding DEFINE_ macro.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  2022-11-30 20:41   ` Andy Shevchenko
@ 2022-12-02  3:22     ` Binbin Zhou
  2022-12-02  8:29       ` Andy Shevchenko
  0 siblings, 1 reply; 18+ messages in thread
From: Binbin Zhou @ 2022-12-02  3:22 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c, loongarch,
	devicetree, Huacai Chen, WANG Xuerui, Arnd Bergmann, Rob Herring,
	Krzysztof Kozlowski, Jianmin Lv

Hi Andy:

在 2022/12/1 04:41, Andy Shevchenko 写道:
> On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote:
>> This I2C module is integrated into the Loongson-2K SoCs and Loongson
>> LS7A bridge chip.
> Looks like some of my comments were not addressed. Are you sure
> you have read the previous reviews carefully?
>
> ...
>
>> +static int ls2x_i2c_xfer(struct i2c_adapter *adap,
>> +			struct i2c_msg *msgs, int num)
>> +{
>> +	int ret, retry;
>> +	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
>> +
>> +	for (retry = 0; retry < adap->retries; retry++) {
>> +		ret = ls2x_i2c_doxfer(adap, msgs, num);
>> +		if (ret != -EAGAIN)
>> +			return ret;
>> +
>> +		dev_dbg(priv->dev, "Retrying transmission (%d)\n", retry);
>> +		udelay(100);
> Why atomic? This long (esp. atomic) delay must be explained.

The modification records for this part of the source code are no longer 
traceable.

Communicating with colleagues offline, I learned that this part of the 
code first appeared on Linux 2.6.36, which was done to circumvent the 
problem of probable failure to scan the device for i2c devices on some 
boards.

How about I add a comment here to explain the reason for this?


>> +	}
>> +
>> +	return -EREMOTEIO;
>> +}
> ...
>
>> +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 & ~0x80, priv->base + I2C_LS2X_CTR);
> Magic number.
>
>> +	ls2x_i2c_adjust_bus_speed(priv);
>> +
>> +	/* Set to normal I2C operating mode and enable interrupts */
>> +	data = readb(priv->base + I2C_LS2X_CTR);
>> +	writeb(data | 0xe0, priv->base + I2C_LS2X_CTR);
> Ditto.

OK.. I will use the macro to define the corresponding bit.

@@ -48,6 +48,11 @@
  #define LS2X_SR_TIP            BIT(1)
  #define LS2X_SR_IF             BIT(0)

+/* Control Register Bit */
+#define LS2X_CTR_EN             BIT(7) /* 0: Frequency setting operation */
+#define LS2X_CTR_IEN            BIT(6) /* Enable i2c interrupt */
+#define LS2X_CTR_MST            BIT(5) /* 0 = Slave 1 = Master */
+
  #define LS2X_I2C_MAX_RETRIES   5

  struct ls2x_i2c_priv {
@@ -266,13 +271,14 @@ static void ls2x_i2c_reginit(struct ls2x_i2c_priv 
*priv)

         /* Enable operations about frequency divider register */
         data = readb(priv->base + I2C_LS2X_CTR);
-       writeb(data & ~0x80, 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 | 0xe0, priv->base + I2C_LS2X_CTR);
+       writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST,
+                       priv->base + I2C_LS2X_CTR);


>> +}
> ...
>
>> +	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
>> +				IRQF_SHARED, "ls2x-i2c", priv);
> Indentation.

Do you mean  "IRQF_SHARE"  should be aligned to "dev"  ?


>> +	if (r < 0)
>> +		return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
> ...
>
>> +	adap->dev.of_node = pdev->dev.of_node;
> Why is this needed?
>
>> +	device_set_node(&adap->dev, dev_fwnode(dev));
> ...
>
>> +	/* i2c device drivers may be active on return from add_adapter() */
>> +	r = i2c_add_adapter(adap);
> Why not use devm_ variant of this?
>
>> +	if (r)
>> +		return dev_err_probe(dev, r, "Failure adding adapter\n");
> ...
>
>> +static int ls2x_i2c_suspend(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
> Can't you use dev_get_drvdata() directly? Why?
>
>> +
>> +	priv->suspended = 1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ls2x_i2c_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
> Ditto.
>
>> +	priv->suspended = 0;
>> +	ls2x_i2c_reginit(priv);
>> +
>> +	return 0;
>> +}
> ...
>
>> +static const struct dev_pm_ops ls2x_i2c_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend, ls2x_i2c_resume)
>> +};
> Use corresponding DEFINE_ macro.

ok.

I will use

"static DEFINE_SIMPLE_DEV_PM_OPS(ls2x_i2c_pm_ops, ls2x_i2c_suspend, 
ls2x_i2c_resume);"  corresponding to  ".pm     = pm_ptr(&ls2x_i2c_pm_ops),"


Thanks.

Binbin


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

* Re: [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  2022-12-02  3:22     ` Binbin Zhou
@ 2022-12-02  8:29       ` Andy Shevchenko
  2022-12-05  2:23         ` Binbin Zhou
  0 siblings, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2022-12-02  8:29 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c, loongarch,
	devicetree, Huacai Chen, WANG Xuerui, Arnd Bergmann, Rob Herring,
	Krzysztof Kozlowski, Jianmin Lv

On Fri, Dec 02, 2022 at 11:22:19AM +0800, Binbin Zhou wrote:
> 在 2022/12/1 04:41, Andy Shevchenko 写道:
> > On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote:

...

> > > +	for (retry = 0; retry < adap->retries; retry++) {
> > > +		ret = ls2x_i2c_doxfer(adap, msgs, num);
> > > +		if (ret != -EAGAIN)
> > > +			return ret;
> > > +
> > > +		dev_dbg(priv->dev, "Retrying transmission (%d)\n", retry);
> > > +		udelay(100);
> > Why atomic? This long (esp. atomic) delay must be explained.
> 
> The modification records for this part of the source code are no longer
> traceable.
> 
> Communicating with colleagues offline, I learned that this part of the code
> first appeared on Linux 2.6.36, which was done to circumvent the problem of
> probable failure to scan the device for i2c devices on some boards.
> 
> How about I add a comment here to explain the reason for this?

Yes, that's what we want, and not what you said above. I.o.w. the comment like
"reason is unknown" is not accepted.

Can you be more specific about the boards and why do you still need this delay?

And also why is it atomic?

> > > +	}

...

> > > +	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
> > > +				IRQF_SHARED, "ls2x-i2c", priv);
> > Indentation.
> 
> Do you mean  "IRQF_SHARE"  should be aligned to "dev"  ?

Yes.

...

> > > +static const struct dev_pm_ops ls2x_i2c_pm_ops = {
> > > +	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend, ls2x_i2c_resume)
> > > +};
> > Use corresponding DEFINE_ macro.
> 
> ok.
> 
> I will use
> 
> "static DEFINE_SIMPLE_DEV_PM_OPS(ls2x_i2c_pm_ops, ls2x_i2c_suspend,
> ls2x_i2c_resume);"  corresponding to  ".pm     = pm_ptr(&ls2x_i2c_pm_ops),"

Shouldn't be pm_sleep_ptr()?


-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C
  2022-11-30  5:55 ` [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C Binbin Zhou
@ 2022-12-02  8:57   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 18+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-02  8:57 UTC (permalink / raw)
  To: Binbin Zhou, Wolfram Sang, Wolfram Sang, Mika Westerberg,
	linux-i2c
  Cc: loongarch, devicetree, Huacai Chen, WANG Xuerui, Andy Shevchenko,
	Arnd Bergmann, Rob Herring, Krzysztof Kozlowski, Jianmin Lv

On 30/11/2022 06:55, Binbin Zhou wrote:
> Add Loongson LS2X I2C controller binding with DT schema format using
> json-schema.

Subject: drop second, redundant "bindings".

> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---

With above:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof


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

* Re: [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  2022-12-02  8:29       ` Andy Shevchenko
@ 2022-12-05  2:23         ` Binbin Zhou
  0 siblings, 0 replies; 18+ messages in thread
From: Binbin Zhou @ 2022-12-05  2:23 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Wolfram Sang, Mika Westerberg, linux-i2c, loongarch,
	devicetree, Huacai Chen, WANG Xuerui, Arnd Bergmann, Rob Herring,
	Krzysztof Kozlowski, Jianmin Lv


在 2022/12/2 16:29, Andy Shevchenko 写道:
> On Fri, Dec 02, 2022 at 11:22:19AM +0800, Binbin Zhou wrote:
>> 在 2022/12/1 04:41, Andy Shevchenko 写道:
>>> On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote:
> ...
>
>>>> +	for (retry = 0; retry < adap->retries; retry++) {
>>>> +		ret = ls2x_i2c_doxfer(adap, msgs, num);
>>>> +		if (ret != -EAGAIN)
>>>> +			return ret;
>>>> +
>>>> +		dev_dbg(priv->dev, "Retrying transmission (%d)\n", retry);
>>>> +		udelay(100);
>>> Why atomic? This long (esp. atomic) delay must be explained.
>> The modification records for this part of the source code are no longer
>> traceable.
>>
>> Communicating with colleagues offline, I learned that this part of the code
>> first appeared on Linux 2.6.36, which was done to circumvent the problem of
>> probable failure to scan the device for i2c devices on some boards.
>>
>> How about I add a comment here to explain the reason for this?
> Yes, that's what we want, and not what you said above. I.o.w. the comment like
> "reason is unknown" is not accepted.
>
> Can you be more specific about the boards and why do you still need this delay?
>
> And also why is it atomic?


As we expected, the driver is geared towards Loongosn-2K and Loongson 
LS7A bridge chips, some of the older hardware still in use has scan 
failure issues and we expect to keep that part as compatible.

"Atomic" may not be necessary, It is expected to try a few more times 
when the scan device fails. Also, I think the corresponding part of the 
s3c2410 may have been referenced in the beginning.

I will try to drop the loop part to do the relevant i2c read/write tests.


Thanks.

Binbin

>>>> +	}
> ...
>
>>>> +	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
>>>> +				IRQF_SHARED, "ls2x-i2c", priv);
>>> Indentation.
>> Do you mean  "IRQF_SHARE"  should be aligned to "dev"  ?
> Yes.
>
> ...
>
>>>> +static const struct dev_pm_ops ls2x_i2c_pm_ops = {
>>>> +	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend, ls2x_i2c_resume)
>>>> +};
>>> Use corresponding DEFINE_ macro.
>> ok.
>>
>> I will use
>>
>> "static DEFINE_SIMPLE_DEV_PM_OPS(ls2x_i2c_pm_ops, ls2x_i2c_suspend,
>> ls2x_i2c_resume);"  corresponding to  ".pm     = pm_ptr(&ls2x_i2c_pm_ops),"
> Shouldn't be pm_sleep_ptr()?
>
>


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

* Re: [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  2022-11-30  5:56 ` [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller Binbin Zhou
  2022-11-30 20:41   ` Andy Shevchenko
@ 2022-12-05  8:38   ` Wolfram Sang
  2022-12-05  8:54     ` Binbin Zhou
  1 sibling, 1 reply; 18+ messages in thread
From: Wolfram Sang @ 2022-12-05  8:38 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Mika Westerberg, linux-i2c, loongarch, devicetree, Huacai Chen,
	WANG Xuerui, Andy Shevchenko, Arnd Bergmann, Rob Herring,
	Krzysztof Kozlowski, Jianmin Lv

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

On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>

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

https://lore.kernel.org/all/20221128130025.23184-1-zhuyinbo@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 --]

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

* Re: [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  2022-12-05  8:38   ` Wolfram Sang
@ 2022-12-05  8:54     ` Binbin Zhou
  2022-12-05  8:58       ` Wolfram Sang
  0 siblings, 1 reply; 18+ messages in thread
From: Binbin Zhou @ 2022-12-05  8:54 UTC (permalink / raw)
  To: Wolfram Sang, Mika Westerberg, linux-i2c, loongarch, devicetree,
	Huacai Chen, WANG Xuerui, Andy Shevchenko, Arnd Bergmann,
	Rob Herring, Krzysztof Kozlowski, Jianmin Lv


在 2022/12/5 16:38, Wolfram Sang 写道:
> On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote:
>> This I2C module is integrated into the Loongson-2K SoCs and Loongson
>> LS7A bridge chip.
>>
>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> There are currently two people submitting a driver for this hardware.
> This is the other driver:
>
> https://lore.kernel.org/all/20221128130025.23184-1-zhuyinbo@loongson.cn/
>
> Can you guys please decide which one is "better" or combine the two to
> make a better one?

Hi Wolfram:

When Krzysztof found out about this, Yinbo and I had negotiated offline 
that I would follow up on this series of patchset submissions,

and this was Yinbo's previous response:

https://lore.kernel.org/all/2e10ae64-3c91-ccf8-a970-eb6e3371b948@loongson.cn/ 
<https://lore.kernel.org/all/2e10ae64-3c91-ccf8-a970-eb6e3371b948@loongson.cn/>

Here is my previous response:

https://lore.kernel.org/all/57496a84-52b3-db97-f0b9-1477de84eb1e@loongson.cn/

Thanks.

Binbin


>
> Thanks,
>
>     Wolfram
>


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

* Re: [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller
  2022-12-05  8:54     ` Binbin Zhou
@ 2022-12-05  8:58       ` Wolfram Sang
  0 siblings, 0 replies; 18+ messages in thread
From: Wolfram Sang @ 2022-12-05  8:58 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Mika Westerberg, linux-i2c, loongarch, devicetree, Huacai Chen,
	WANG Xuerui, Andy Shevchenko, Arnd Bergmann, Rob Herring,
	Krzysztof Kozlowski, Jianmin Lv

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


> When Krzysztof found out about this, Yinbo and I had negotiated offline that
> I would follow up on this series of patchset submissions,
> 
> and this was Yinbo's previous response:
> 
> https://lore.kernel.org/all/2e10ae64-3c91-ccf8-a970-eb6e3371b948@loongson.cn/ <https://lore.kernel.org/all/2e10ae64-3c91-ccf8-a970-eb6e3371b948@loongson.cn/>

OK, I see. Thank you for the quick response!


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

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

* Re: [PATCH V4 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids'
  2022-11-30  5:55 ` [PATCH V4 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids' Binbin Zhou
  2022-11-30 20:29   ` Andy Shevchenko
@ 2022-12-05  9:29   ` Wolfram Sang
  1 sibling, 0 replies; 18+ messages in thread
From: Wolfram Sang @ 2022-12-05  9:29 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Mika Westerberg, linux-i2c, loongarch, devicetree, Huacai Chen,
	WANG Xuerui, Andy Shevchenko, Arnd Bergmann, Rob Herring,
	Krzysztof Kozlowski, Jianmin Lv

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

On Wed, Nov 30, 2022 at 01:55:51PM +0800, Binbin Zhou wrote:
> Dropping a matching #ifdef check along with dropping of_match_ptr()
> is just a cleanup, while dropping of_match_ptr() that has no
> corresponding #ifdef fixes an actual warning.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>

Applied to for-next, thanks!


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

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

end of thread, other threads:[~2022-12-05  9:29 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-30  5:55 [PATCH V4 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller Binbin Zhou
2022-11-30  5:55 ` [PATCH V4 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids' Binbin Zhou
2022-11-30 20:29   ` Andy Shevchenko
2022-12-05  9:29   ` Wolfram Sang
2022-11-30  5:55 ` [PATCH V4 2/5] i2c: gpio: Add support on ACPI-based system Binbin Zhou
2022-11-30 20:32   ` Andy Shevchenko
2022-11-30  5:55 ` [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C Binbin Zhou
2022-12-02  8:57   ` Krzysztof Kozlowski
2022-11-30  5:56 ` [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller Binbin Zhou
2022-11-30 20:41   ` Andy Shevchenko
2022-12-02  3:22     ` Binbin Zhou
2022-12-02  8:29       ` Andy Shevchenko
2022-12-05  2:23         ` Binbin Zhou
2022-12-05  8:38   ` Wolfram Sang
2022-12-05  8:54     ` Binbin Zhou
2022-12-05  8:58       ` Wolfram Sang
2022-11-30  5:56 ` [PATCH V4 5/5] LoongArch: Enable LS2X I2C in loongson3_defconfig Binbin Zhou
2022-11-30 13:47 ` [PATCH V4 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C Rob Herring

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