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

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

[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 only tested the Loongson-3A5000+LS7A1000/LS7A2000 under LoongArch
architecture.

Thanks.

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

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

 .../bindings/i2c/loongson,ls2x-i2c.yaml       |  48 +++
 arch/loongarch/configs/loongson3_defconfig    |   1 +
 drivers/i2c/busses/Kconfig                    |   7 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-gpio.c                 |  30 ++
 drivers/i2c/busses/i2c-ls2x.c                 | 365 ++++++++++++++++++
 6 files changed, 452 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

-- 
2.31.1

[PATCH V2 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: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/i2c-gpio.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index b1985c1667e1..417eb31e0971 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/platform_data/i2c-gpio.h>
 #include <linux/platform_device.h>
@@ -318,6 +319,24 @@ static void of_i2c_gpio_get_props(struct device_node *np,
 		of_property_read_bool(np, "i2c-gpio,scl-output-only");
 }
 
+static void acpi_i2c_gpio_get_props(struct device *dev,
+				  struct i2c_gpio_platform_data *pdata)
+{
+	u32 reg;
+
+	device_property_read_u32(dev, "delay-us", &pdata->udelay);
+
+	if (!device_property_read_u32(dev, "timeout-ms", &reg))
+		pdata->timeout = msecs_to_jiffies(reg);
+
+	pdata->sda_is_open_drain =
+		device_property_read_bool(dev, "sda-open-drain");
+	pdata->scl_is_open_drain =
+		device_property_read_bool(dev, "scl-open-drain");
+	pdata->scl_is_output_only =
+		device_property_read_bool(dev, "scl-output-only");
+}
+
 static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
 					   const char *con_id,
 					   unsigned int index,
@@ -375,6 +394,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
 
 	if (np) {
 		of_i2c_gpio_get_props(np, pdata);
+	} else if (ACPI_COMPANION(dev)) {
+		acpi_i2c_gpio_get_props(dev, pdata);
 	} else {
 		/*
 		 * If all platform data settings are zero it is OK
@@ -491,10 +512,19 @@ static const struct of_device_id i2c_gpio_dt_ids[] = {
 MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
 #endif
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id i2c_gpio_acpi_match[] = {
+	{"LOON0005"}, /*LoongArch*/
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, i2c_gpio_acpi_match);
+#endif
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
+		.acpi_match_table = ACPI_PTR(i2c_gpio_acpi_match),
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= i2c_gpio_remove,
-- 
2.31.1

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

[Mika Westerberg]

[+Rafael and Andy]

Hi,

On Mon, Sep 26, 2022 at 09:00:04PM +0800, Binbin Zhou wrote:
> Add support for the ACPI-based device registration so that the driver
> can be also enabled through ACPI table.
> 
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  drivers/i2c/busses/i2c-gpio.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index b1985c1667e1..417eb31e0971 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -13,6 +13,7 @@
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/acpi.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/i2c-gpio.h>
>  #include <linux/platform_device.h>
> @@ -318,6 +319,24 @@ static void of_i2c_gpio_get_props(struct device_node *np,
>  		of_property_read_bool(np, "i2c-gpio,scl-output-only");
>  }
>  
> +static void acpi_i2c_gpio_get_props(struct device *dev,
> +				  struct i2c_gpio_platform_data *pdata)
> +{
> +	u32 reg;
> +
> +	device_property_read_u32(dev, "delay-us", &pdata->udelay);
> +
> +	if (!device_property_read_u32(dev, "timeout-ms", &reg))
> +		pdata->timeout = msecs_to_jiffies(reg);
> +
> +	pdata->sda_is_open_drain =
> +		device_property_read_bool(dev, "sda-open-drain");
> +	pdata->scl_is_open_drain =
> +		device_property_read_bool(dev, "scl-open-drain");
> +	pdata->scl_is_output_only =
> +		device_property_read_bool(dev, "scl-output-only");
> +}

Otherwise this patch looks good but I'm concerned because we have two
kinds of bindings now. The DT one above uses "i2c-gpio,..." and this
ACPI one uses just "..." so the question is where did these come from?
Is there already some existing system out there with these bindings or
they are documented somewhere?

Ideally we would be able to just do:

	pdata->sda_is_open_drain =
		device_property_read_bool(dev, "i2c-gpio,sda-open-drain");

for any firmware description.

> +
>  static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
>  					   const char *con_id,
>  					   unsigned int index,
> @@ -375,6 +394,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>  
>  	if (np) {
>  		of_i2c_gpio_get_props(np, pdata);
> +	} else if (ACPI_COMPANION(dev)) {
> +		acpi_i2c_gpio_get_props(dev, pdata);
>  	} else {
>  		/*
>  		 * If all platform data settings are zero it is OK
> @@ -491,10 +512,19 @@ static const struct of_device_id i2c_gpio_dt_ids[] = {
>  MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
>  #endif
>  
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id i2c_gpio_acpi_match[] = {
> +	{"LOON0005"}, /*LoongArch*/
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, i2c_gpio_acpi_match);
> +#endif
> +
>  static struct platform_driver i2c_gpio_driver = {
>  	.driver		= {
>  		.name	= "i2c-gpio",
>  		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
> +		.acpi_match_table = ACPI_PTR(i2c_gpio_acpi_match),
>  	},
>  	.probe		= i2c_gpio_probe,
>  	.remove		= i2c_gpio_remove,
> -- 
> 2.31.1

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

[Arnd Bergmann]

On Mon, Sep 26, 2022, at 4:59 PM, Mika Westerberg wrote:

>> +static void acpi_i2c_gpio_get_props(struct device *dev,
>> +				  struct i2c_gpio_platform_data *pdata)
>> +{
>> +	u32 reg;
>> +
>> +	device_property_read_u32(dev, "delay-us", &pdata->udelay);
>> +
>> +	if (!device_property_read_u32(dev, "timeout-ms", &reg))
>> +		pdata->timeout = msecs_to_jiffies(reg);
>> +
>> +	pdata->sda_is_open_drain =
>> +		device_property_read_bool(dev, "sda-open-drain");
>> +	pdata->scl_is_open_drain =
>> +		device_property_read_bool(dev, "scl-open-drain");
>> +	pdata->scl_is_output_only =
>> +		device_property_read_bool(dev, "scl-output-only");
>> +}
>
> Otherwise this patch looks good but I'm concerned because we have two
> kinds of bindings now. The DT one above uses "i2c-gpio,..." and this
> ACPI one uses just "..." so the question is where did these come from?
> Is there already some existing system out there with these bindings or
> they are documented somewhere?

I'm fairly sure it's just a mistake and it should use the regular
binding. As far as I understand, there are still other incompatible
changes being made to the firmware on these machines, so it's just
a matter of updating this part as well.

     Arnd

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

[Andy Shevchenko]

On Mon, Sep 26, 2022 at 09:00:04PM +0800, Binbin Zhou wrote:
> Add support for the ACPI-based device registration so that the driver
> can be also enabled through ACPI table.
> 
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>

Who is this and why this SoB in the chain?

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

...

>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/acpi.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/i2c-gpio.h>
>  #include <linux/platform_device.h>

Seems you misinterpret ordering.

Besides that I don't see the needs of acpi.h. The header missed the
mod_devicetable.h (even without this patch), and your code needs property.h.

...

> +static void acpi_i2c_gpio_get_props(struct device *dev,
> +				  struct i2c_gpio_platform_data *pdata)
> +{
> +	u32 reg;
> +
> +	device_property_read_u32(dev, "delay-us", &pdata->udelay);
> +
> +	if (!device_property_read_u32(dev, "timeout-ms", &reg))
> +		pdata->timeout = msecs_to_jiffies(reg);
> +
> +	pdata->sda_is_open_drain =
> +		device_property_read_bool(dev, "sda-open-drain");
> +	pdata->scl_is_open_drain =
> +		device_property_read_bool(dev, "scl-open-drain");
> +	pdata->scl_is_output_only =
> +		device_property_read_bool(dev, "scl-output-only");
> +}

+1 to Mika's objection. Instead, make the common bindings and convert
the driver from OF to be agnostic.

...

>  MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
>  #endif
>  
> +#ifdef CONFIG_ACPI

Please, drop these ifdefferies (including OF one), it's more harmful
than useful.

> +#endif

...

>  		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
> +		.acpi_match_table = ACPI_PTR(i2c_gpio_acpi_match),

No ACPI_PTR(), and accordingly no of_match_ptr(). See above.

-- 
With Best Regards,
Andy Shevchenko

[PATCH V2 2/4] dt-bindings: i2c: add bindings for Loongson LS2X I2C

[Binbin Zhou]

Add device tree bindings for the i2c controller on the Loongson-2K Soc
or Loongosn LS7A bridge.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/i2c/loongson,ls2x-i2c.yaml       | 48 +++++++++++++++++++
 1 file changed, 48 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..8c785f329d2f
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
@@ -0,0 +1,48 @@
+# 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-2K SoCs
+      - loongson,ls7a-i2c # Loongson LS7A Bridge
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c@1fe21000 {
+        compatible = "loongson,ls2k-i2c";
+        reg = <0 0x1fe21000 0 0x8>;
+        interrupts = <22>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        eeprom@57{
+            compatible = "atmel,24c16";
+            reg = <0x57>;
+            pagesize = <16>;
+        };
+    };
-- 
2.31.1

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

[Binbin Zhou]

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

Initialize the i2c controller early. This is required in order to ensure
that core system devices such as the display controller(DC) attached via
I2C are available early in boot.

Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/Kconfig    |   7 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-ls2x.c | 365 ++++++++++++++++++++++++++++++++++
 3 files changed, 373 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 7284206b278b..99b21935dc71 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -887,6 +887,13 @@ 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's LS2K/LS7A Platform-Bridge.
+
 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 c5cac15f075c..721841361e34 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..ab4c1b45d3d6
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ls2x.c
@@ -0,0 +1,365 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Loongson-2K/7A I2C master mode driver
+ *
+ * Copyright (C) 2013 Loongson Technology Corporation Limited
+ * Copyright (C) 2014-2017 Lemote, Inc.
+ *
+ * Originally written by liushaozong
+ */
+
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/completion.h>
+#include <linux/platform_device.h>
+
+#define LS2X_I2C_PRER_LO_REG	0x0
+#define LS2X_I2C_PRER_HI_REG	0x1
+#define LS2X_I2C_CTR_REG	0x2
+#define LS2X_I2C_TXR_REG	0x3
+#define LS2X_I2C_RXR_REG	0x3
+#define LS2X_I2C_CR_REG		0x4
+#define LS2X_I2C_SR_REG		0x4
+
+#define LS2X_I2C_CMD_START	BIT(7)
+#define LS2X_I2C_CMD_STOP	BIT(6)
+#define LS2X_I2C_CMD_READ	BIT(5)
+#define LS2X_I2C_CMD_WRITE	BIT(4)
+#define LS2X_I2C_CMD_ACK	BIT(3)
+#define LS2X_I2C_CMD_IACK	BIT(0)
+
+#define LS2X_I2C_SR_NOACK	BIT(7)
+#define LS2X_I2C_SR_BUSY	BIT(6)
+#define LS2X_I2C_SR_AL		BIT(5)
+#define LS2X_I2C_SR_TIP		BIT(1)
+#define LS2X_I2C_SR_IF		BIT(0)
+
+#define I2C_MAX_RETRIES		5
+
+/* I2C clock frequency 50M */
+#define I2C_CLK_RATE_50M	(50 * 1000000)
+
+#define i2c_readb(addr)		readb(dev->base + addr)
+#define i2c_writeb(val, addr)	writeb(val, dev->base + addr)
+
+struct ls2x_i2c_dev {
+	unsigned int		suspended:1;
+	struct device		*dev;
+	void __iomem		*base;
+	int			irq;
+	u32			bus_clk_rate;
+	struct completion	cmd_complete;
+	struct i2c_adapter	adapter;
+};
+
+static void i2c_stop(struct ls2x_i2c_dev *dev)
+{
+again:
+	i2c_writeb(LS2X_I2C_CMD_STOP, LS2X_I2C_CR_REG);
+	wait_for_completion(&dev->cmd_complete);
+
+	i2c_readb(LS2X_I2C_SR_REG);
+
+	while (i2c_readb(LS2X_I2C_SR_REG) & LS2X_I2C_SR_BUSY)
+		goto again;
+}
+
+static int ls2x_i2c_start(struct ls2x_i2c_dev *dev,
+		     int dev_addr, int flags)
+{
+	int retry = I2C_MAX_RETRIES;
+	unsigned char addr = (dev_addr & 0x7f) << 1;
+
+	addr |= (flags & I2C_M_RD) ? 1 : 0;
+start:
+	mdelay(1);
+	i2c_writeb(addr, LS2X_I2C_TXR_REG);
+	dev_dbg(dev->dev, "%s <line%d>: i2c device address: 0x%x\n",
+			__func__, __LINE__, addr);
+
+	i2c_writeb((LS2X_I2C_CMD_START | LS2X_I2C_CMD_WRITE),
+			LS2X_I2C_CR_REG);
+	wait_for_completion(&dev->cmd_complete);
+
+	if (i2c_readb(LS2X_I2C_SR_REG) & LS2X_I2C_SR_NOACK) {
+		i2c_stop(dev);
+		while (retry--)
+			goto start;
+		dev_info(dev->dev, "There is no i2c device ack\n");
+		return 0;
+	}
+
+	return 1;
+}
+
+static void ls2x_i2c_reginit(struct ls2x_i2c_dev *dev)
+{
+	u16 val = 0x12c;
+
+	if (dev->bus_clk_rate)
+		val = I2C_CLK_RATE_50M / (5 * dev->bus_clk_rate) - 1;
+
+	i2c_writeb(0, LS2X_I2C_CTR_REG);
+	i2c_writeb(val & 0xff, LS2X_I2C_PRER_LO_REG);
+	i2c_writeb((val & 0xff00) >> 8, LS2X_I2C_PRER_HI_REG);
+	i2c_writeb(0xc0, LS2X_I2C_CTR_REG);
+}
+
+static int ls2x_i2c_read(struct ls2x_i2c_dev *dev,
+			unsigned char *buf, int count)
+{
+	int i;
+	int cmd = LS2X_I2C_CMD_READ;
+
+	for (i = 0; i < count; i++) {
+		if (i == count - 1)
+			cmd |= LS2X_I2C_CMD_ACK;
+
+		i2c_writeb(cmd, LS2X_I2C_CR_REG);
+		wait_for_completion(&dev->cmd_complete);
+
+		buf[i] = i2c_readb(LS2X_I2C_RXR_REG);
+		dev_dbg(dev->dev, "%s <line%d>: read buf[%d] <= %02x\n",
+				__func__, __LINE__, i, buf[i]);
+	}
+
+	return i;
+}
+
+static int ls2x_i2c_write(struct ls2x_i2c_dev *dev,
+			unsigned char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		i2c_writeb(buf[i], LS2X_I2C_TXR_REG);
+		dev_dbg(dev->dev, "%s <line%d>: write buf[%d] => %02x\n",
+				__func__, __LINE__, i, buf[i]);
+
+		i2c_writeb(LS2X_I2C_CMD_WRITE, LS2X_I2C_CR_REG);
+		wait_for_completion(&dev->cmd_complete);
+
+		if (i2c_readb(LS2X_I2C_SR_REG) & LS2X_I2C_SR_NOACK) {
+			dev_dbg(dev->dev, "%s <line%d>: device no ack\n",
+					__func__, __LINE__);
+			i2c_stop(dev);
+			return 0;
+		}
+	}
+
+	return i;
+}
+
+static int ls2x_i2c_doxfer(struct ls2x_i2c_dev *dev,
+			struct i2c_msg *msgs, int num)
+{
+	int i;
+	struct i2c_msg *m = msgs;
+
+	for (i = 0; i < num; i++) {
+		reinit_completion(&dev->cmd_complete);
+		if (!ls2x_i2c_start(dev, m->addr, m->flags))
+			return 0;
+
+		if (m->flags & I2C_M_RD)
+			ls2x_i2c_read(dev, m->buf, m->len);
+		else
+			ls2x_i2c_write(dev, m->buf, m->len);
+		++m;
+	}
+
+	i2c_stop(dev);
+
+	return i;
+}
+
+static int ls2x_i2c_xfer(struct i2c_adapter *adap,
+			struct i2c_msg *msgs, int num)
+{
+	int ret, retry;
+	struct ls2x_i2c_dev *dev;
+
+	dev = i2c_get_adapdata(adap);
+	for (retry = 0; retry < adap->retries; retry++) {
+		ret = ls2x_i2c_doxfer(dev, msgs, num);
+		if (ret != -EAGAIN)
+			return ret;
+
+		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_isr(int this_irq, void *dev_id)
+{
+	unsigned char iflag;
+	struct ls2x_i2c_dev *dev = dev_id;
+
+	iflag = i2c_readb(LS2X_I2C_SR_REG);
+
+	if (iflag & LS2X_I2C_SR_IF) {
+		i2c_writeb(LS2X_I2C_CMD_IACK, LS2X_I2C_CR_REG);
+		complete(&dev->cmd_complete);
+	} else
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static int ls2x_i2c_probe(struct platform_device *pdev)
+{
+	int r;
+	struct ls2x_i2c_dev *dev;
+	struct i2c_adapter *adap;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(struct ls2x_i2c_dev), GFP_KERNEL);
+	if (unlikely(!dev))
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, dev);
+	init_completion(&dev->cmd_complete);
+	dev->dev = &pdev->dev;
+
+	/* Map hardware registers */
+	dev->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(dev->base))
+		return PTR_ERR(dev->base);
+
+	dev->irq = platform_get_irq(pdev, 0);
+	if (unlikely(dev->irq <= 0))
+		return -ENODEV;
+
+	r = devm_request_irq(&pdev->dev, dev->irq, ls2x_i2c_isr,
+			      IRQF_SHARED, "ls2x-i2c", dev);
+	if (unlikely(r)) {
+		dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
+		return r;
+	}
+
+	dev->bus_clk_rate = i2c_acpi_find_bus_speed(&pdev->dev);
+	if (!dev->bus_clk_rate)
+		device_property_read_u32(&pdev->dev, "clock-frequency",
+					&dev->bus_clk_rate);
+
+	ls2x_i2c_reginit(dev);
+
+	/* Add the i2c adapter */
+	adap = &dev->adapter;
+	i2c_set_adapdata(adap, dev);
+	adap->nr = pdev->id;
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON;
+	adap->retries = I2C_MAX_RETRIES;
+	adap->algo = &ls2x_i2c_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
+
+	/* i2c device drivers may be active on return from add_adapter() */
+	r = i2c_add_adapter(adap);
+	if (r) {
+		dev_err(dev->dev, "failure adding adapter\n");
+		return r;
+	}
+
+	return 0;
+}
+
+static int ls2x_i2c_remove(struct platform_device *pdev)
+{
+	struct ls2x_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dev->adapter);
+	return 0;
+}
+
+static int __maybe_unused ls2x_i2c_suspend_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_dev->suspended = 1;
+
+	return 0;
+}
+
+static int __maybe_unused ls2x_i2c_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_dev->suspended = 0;
+	ls2x_i2c_reginit(i2c_dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops ls2x_i2c_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend_noirq, ls2x_i2c_resume)
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id ls2x_i2c_id_table[] = {
+	{.compatible = "loongson,ls2k-i2c"},
+	{.compatible = "loongson,ls7a-i2c"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ls2x_i2c_id_table);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id ls2x_i2c_acpi_match[] = {
+	{"LOON0004"},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, ls2x_i2c_acpi_match);
+#endif
+
+static struct platform_driver ls2x_i2c_driver = {
+	.probe		= ls2x_i2c_probe,
+	.remove		= ls2x_i2c_remove,
+	.driver		= {
+		.name	= "ls2x-i2c",
+		.owner	= THIS_MODULE,
+		.pm	= &ls2x_i2c_dev_pm_ops,
+		.of_match_table = of_match_ptr(ls2x_i2c_id_table),
+		.acpi_match_table = ACPI_PTR(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_AUTHOR("Loongson Technology Corporation Limited");
+MODULE_DESCRIPTION("Loongson LS2X I2C bus adapter");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ls2x-i2c");
-- 
2.31.1

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

[Andy Shevchenko]

On Mon, Sep 26, 2022 at 09:00:06PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoC and the Loongson
> LS7A bridge chip.
> 
> Initialize the i2c controller early. This is required in order to ensure
> that core system devices such as the display controller(DC) attached via
> I2C are available early in boot.

...

> +	help
> +	  If you say yes to this option, support will be included for the
> +	  I2C interface on the Loongson's LS2K/LS7A Platform-Bridge.

What will be module name?

...

> + * Copyright (C) 2013 Loongson Technology Corporation Limited
> + * Copyright (C) 2014-2017 Lemote, Inc.

It's 2022 out of the window, are you sure this code wasn't changed
for 5 years?!

...

> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/completion.h>
> +#include <linux/platform_device.h>

Keep it sorted.

Also check the headers, the rule of thumb is to include headers you are direct
user of, excluding the ones that are guaranteed to be included by already
mentioned.

...

> +#define I2C_MAX_RETRIES		5

No namespace?

...

> +#define I2C_CLK_RATE_50M	(50 * 1000000)

HZ_PER_MHZ

...

> +#define i2c_readb(addr)		readb(dev->base + addr)
> +#define i2c_writeb(val, addr)	writeb(val, dev->base + addr)

No namespace? What is the usefulness of these macros taking into consideration
that:
 - they are macros and not inliners
 - they missed the used parameter

...

> +struct ls2x_i2c_dev {
> +	unsigned int		suspended:1;
> +	struct device		*dev;
> +	void __iomem		*base;
> +	int			irq;
> +	u32			bus_clk_rate;
> +	struct completion	cmd_complete;
> +	struct i2c_adapter	adapter;

You may save a few bytes of code if you put the first member the one that is
used a lot in the pointer arithmetics or performance-wise. You may check the
result with bloat-o-meter.

> +};

> +static void i2c_stop(struct ls2x_i2c_dev *dev)
> +{
> +again:
> +	i2c_writeb(LS2X_I2C_CMD_STOP, LS2X_I2C_CR_REG);
> +	wait_for_completion(&dev->cmd_complete);
> +
> +	i2c_readb(LS2X_I2C_SR_REG);
> +
> +	while (i2c_readb(LS2X_I2C_SR_REG) & LS2X_I2C_SR_BUSY)
> +		goto again;
> +}

Can't you refactor to avoid label?

...

> +static int ls2x_i2c_start(struct ls2x_i2c_dev *dev,
> +		     int dev_addr, int flags)
> +{
> +	int retry = I2C_MAX_RETRIES;
> +	unsigned char addr = (dev_addr & 0x7f) << 1;

> +	addr |= (flags & I2C_M_RD) ? 1 : 0;

NIH: i2c_8bit_addr_from_msg() ?

> +start:
> +	mdelay(1);
> +	i2c_writeb(addr, LS2X_I2C_TXR_REG);
> +	dev_dbg(dev->dev, "%s <line%d>: i2c device address: 0x%x\n",
> +			__func__, __LINE__, addr);

No need to have __func__, __LINE__, etc. First of all, these are available via
Dynamic Debug. Second, using those mean the lack of uniqueness of the message
test, make it more unique instead.

> +
> +	i2c_writeb((LS2X_I2C_CMD_START | LS2X_I2C_CMD_WRITE),
> +			LS2X_I2C_CR_REG);
> +	wait_for_completion(&dev->cmd_complete);
> +
> +	if (i2c_readb(LS2X_I2C_SR_REG) & LS2X_I2C_SR_NOACK) {
> +		i2c_stop(dev);
> +		while (retry--)

> +			goto start;

Try to refactor your code to avoid using too many labels here and there.

> +		dev_info(dev->dev, "There is no i2c device ack\n");
> +		return 0;
> +	}
> +
> +	return 1;
> +}

...

> +	u16 val = 0x12c;

Magic!

...

> +	i2c_writeb(val & 0xff, LS2X_I2C_PRER_LO_REG);
> +	i2c_writeb((val & 0xff00) >> 8, LS2X_I2C_PRER_HI_REG);

Redundant '& 0xff...' parts. Besides that, is there any HW limitation of using
16-bit writes?

...

> +	i2c_writeb(0xc0, LS2X_I2C_CTR_REG);

Magic!

It's enough for now, this code needs much more work, please take your time.


-- 
With Best Regards,
Andy Shevchenko

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

[Binbin Zhou]

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

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 4083d3051109..7910adf20887 100644
--- a/arch/loongarch/configs/loongson3_defconfig
+++ b/arch/loongarch/configs/loongson3_defconfig
@@ -558,6 +558,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