*** Support ASPEED AST2600 I2C ***

AST2600 support the new register set of I2C controller Add bindings document to support the new driver of I2C

Signed-off-by Jamin Lin <[email protected]>

---
 .../bindings/i2c/aspeed,new-i2c.yaml          | 107 ++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,new-i2c.yaml

Documentation/devicetree/bindings/i2c/aspeed,new-i2c.yaml new file +17 -0

new file mode 100644
index 000000000000..2c264596b138
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/aspeed,new-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED I2C on the AST26XX SoCs Device Tree Bindings
+
+description: |
+  ASPEED I2C controller support the new register set since AST26XX
+  The i2c-global-regs device is used to enable new register set
+
+maintainers:
+  - Ryan Chen <[email protected]>
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#

Documentation/devicetree/bindings/i2c/aspeed,new-i2c.yaml new file +75 -0

+    items:
+      - description: address offset and range of bus
+      - description: address offset and range of bus buffer
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description:
+      root clock of bus, should reference the APB
+      clock in the second cell
+
+  resets:
+    maxItems: 1
+
+  bus-frequency:
+    minimum: 100
+    maximum: 5000000
+    default: 100000
+    description: frequency of the bus clock in Hz defaults to 100 kHz when not
+      specified
+
+  multi-master:
+    type: boolean
+    description:
+      states that there is another master active on this bus
+
+  buff-mode:
+    type: boolean
+    description:
+      buffer mode data transfer
+
+  byte-mode:
+    type: boolean
+    description:
+      byte mode tata transfer
+
+  smbus-alert:
+    type: boolean
+    description:
+      smbus alert protocol
+
+required:
+  - reg
+  - compatible
+  - clocks
+  - resets
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/aspeed-scu-ic.h>
+    #include <dt-bindings/clock/ast2600-clock.h>
+
+    i2c_gr: i2c-global-regs at 0 {
+      compatible = "aspeed,ast2600-i2c-global", "syscon";
+      reg = <0x0 0x20>;
+      clocks = <&syscon ASPEED_CLK_APB2>;
+      resets = <&syscon ASPEED_RESET_I2C>;
+    };
+
+    i2c0: i2c-bus at 80 {
+      compatible = "aspeed,ast2600-i2c-bus";
+      #address-cells = <1>;
+      #size-cells = <0>;
+      #interrupt-cells = <1>;
+      reg = <0x80 0x80>, <0xC00 0x20>;
+      clocks = <&syscon ASPEED_CLK_APB2>;
+      resets = <&syscon ASPEED_RESET_I2C>;
+      interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+      bus-frequency = <100000>;
+    };

Signature · 1 line

2.17.1

Add i2c new driver to support new register set for AST2600. AST2600 support three modes for data transfer which are byte mode, buffer mode and dma mode, respectively.

The global driver of i2c is used to set the new register mode and define the base clock frequency
of baseclk_1~baseclk_4.

drivers/i2c/busses/Kconfig +1 -0

+	select I2C_SMBUS

+ */

drivers/i2c/busses/ast2600-i2c-global.c new file +10 -0

+#include <linux/clk.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>

+};

drivers/i2c/busses/ast2600-i2c-global.c new file +6 -0

+		for (i = 0; i < 0xff; i++) {
+			/*
+			 * i maps to div:
+			 * 0x00: div 1
+			 * 0x01: div 1.5
+			 * 0x02: div 2

drivers/i2c/busses/ast2600-i2c-global.c new file +8 -0

+	}
+
+	err = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, onecell);
+	if (err)
+		pr_err("failed to add i2c base clk provider: %d\n", err);
+
+	return clk_divider;
+}

drivers/i2c/busses/ast2600-i2c-global.c new file +30 -0

+	} else {
+		/* SCU I2C Reset */
+		reset_control_assert(i2c_ic->rst);
+		udelay(3);
+		reset_control_deassert(i2c_ic->rst);
+	}
+
+	/* ast2600 init */
+	writel(ASPEED_I2CG_SLAVE_PKT_NAK |
+	       ASPEED_I2CG_CTRL_NEW_REG |
+	       ASPEED_I2CG_CTRL_NEW_CLK_DIV,
+	       i2c_ic->base + ASPEED_I2CG_CTRL);
+	parent_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(parent_clk))
+		return PTR_ERR(parent_clk);
+	parent_clk_frequency = clk_get_rate(parent_clk);
+	pr_debug("parent_clk_frequency %ld\n", parent_clk_frequency);
+	clk_divider = aspeed_i2c_ic_get_new_clk_divider(parent_clk_frequency, node);
+	writel(clk_divider, i2c_ic->base + ASPEED_I2CG_CLK_DIV_CTRL);
+
+	pr_info("i2c global registered\n");
+
+	return 0;
+
+err_iounmap:
+	iounmap(i2c_ic->base);
+err_free_ic:
+	kfree(i2c_ic);
+	return ret;
+}

+#endif /* AST2600_I2C_GLOBAL_H */

+/*
+ * i2c-new-aspeed.c - I2C driver for the Aspeed SoC

+ *
+ * Copyright (C) ASPEED Technology Inc.
+ * Ryan Chen <[email protected]>
+ *
+ */

+#define AST_I2CC_TX_DIR_MASK (0x7 << 29)

+static struct ast_i2c_timing_table aspeed_old_i2c_timing_table[] = {

drivers/i2c/busses/i2c-new-aspeed.c new file +141 -0

+	/* Divisor : Base Clock : tCKHighMin : tCK High : tCK Low  */
+	/* Divisor :	  [3:0] : [23: 20]   :   [19:16]:   [15:12] */
+	{ 6, 0x00000300 | (0x0) | (0x2 << 20) | (0x2 << 16) | (0x2 << 12) },
+	{ 7, 0x00000300 | (0x0) | (0x3 << 20) | (0x3 << 16) | (0x2 << 12) },
+	{ 8, 0x00000300 | (0x0) | (0x3 << 20) | (0x3 << 16) | (0x3 << 12) },
+	{ 9, 0x00000300 | (0x0) | (0x4 << 20) | (0x4 << 16) | (0x3 << 12) },
+	{ 10, 0x00000300 | (0x0) | (0x4 << 20) | (0x4 << 16) | (0x4 << 12) },
+	{ 11, 0x00000300 | (0x0) | (0x5 << 20) | (0x5 << 16) | (0x4 << 12) },
+	{ 12, 0x00000300 | (0x0) | (0x5 << 20) | (0x5 << 16) | (0x5 << 12) },
+	{ 13, 0x00000300 | (0x0) | (0x6 << 20) | (0x6 << 16) | (0x5 << 12) },
+	{ 14, 0x00000300 | (0x0) | (0x6 << 20) | (0x6 << 16) | (0x6 << 12) },
+	{ 15, 0x00000300 | (0x0) | (0x7 << 20) | (0x7 << 16) | (0x6 << 12) },
+	{ 16, 0x00000300 | (0x0) | (0x7 << 20) | (0x7 << 16) | (0x7 << 12) },
+	{ 17, 0x00000300 | (0x0) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 18, 0x00000300 | (0x0) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 19, 0x00000300 | (0x0) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 20, 0x00000300 | (0x0) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 21, 0x00000300 | (0x0) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 22, 0x00000300 | (0x0) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 23, 0x00000300 | (0x0) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 24, 0x00000300 | (0x0) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 25, 0x00000300 | (0x0) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 26, 0x00000300 | (0x0) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 27, 0x00000300 | (0x0) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 28, 0x00000300 | (0x0) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 29, 0x00000300 | (0x0) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 30, 0x00000300 | (0x0) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 31, 0x00000300 | (0x0) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 32, 0x00000300 | (0x0) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 34, 0x00000300 | (0x1) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 36, 0x00000300 | (0x1) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 38, 0x00000300 | (0x1) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 40, 0x00000300 | (0x1) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 42, 0x00000300 | (0x1) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 44, 0x00000300 | (0x1) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 46, 0x00000300 | (0x1) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 48, 0x00000300 | (0x1) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 50, 0x00000300 | (0x1) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 52, 0x00000300 | (0x1) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 54, 0x00000300 | (0x1) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 56, 0x00000300 | (0x1) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 58, 0x00000300 | (0x1) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 60, 0x00000300 | (0x1) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 62, 0x00000300 | (0x1) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 64, 0x00000300 | (0x1) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 68, 0x00000300 | (0x2) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 72, 0x00000300 | (0x2) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 76, 0x00000300 | (0x2) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 80, 0x00000300 | (0x2) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 84, 0x00000300 | (0x2) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 88, 0x00000300 | (0x2) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 92, 0x00000300 | (0x2) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 96, 0x00000300 | (0x2) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 100, 0x00000300 | (0x2) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 104, 0x00000300 | (0x2) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 108, 0x00000300 | (0x2) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 112, 0x00000300 | (0x2) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 116, 0x00000300 | (0x2) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 120, 0x00000300 | (0x2) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 124, 0x00000300 | (0x2) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 128, 0x00000300 | (0x2) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 136, 0x00000300 | (0x3) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 144, 0x00000300 | (0x3) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 152, 0x00000300 | (0x3) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 160, 0x00000300 | (0x3) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 168, 0x00000300 | (0x3) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 176, 0x00000300 | (0x3) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 184, 0x00000300 | (0x3) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 192, 0x00000300 | (0x3) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 200, 0x00000300 | (0x3) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 208, 0x00000300 | (0x3) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 216, 0x00000300 | (0x3) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 224, 0x00000300 | (0x3) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 232, 0x00000300 | (0x3) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 240, 0x00000300 | (0x3) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 248, 0x00000300 | (0x3) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 256, 0x00000300 | (0x3) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 272, 0x00000300 | (0x4) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 288, 0x00000300 | (0x4) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 304, 0x00000300 | (0x4) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 320, 0x00000300 | (0x4) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 336, 0x00000300 | (0x4) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 352, 0x00000300 | (0x4) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 368, 0x00000300 | (0x4) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 384, 0x00000300 | (0x4) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 400, 0x00000300 | (0x4) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 416, 0x00000300 | (0x4) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 432, 0x00000300 | (0x4) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 448, 0x00000300 | (0x4) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 464, 0x00000300 | (0x4) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 480, 0x00000300 | (0x4) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 496, 0x00000300 | (0x4) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 512, 0x00000300 | (0x4) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 544, 0x00000300 | (0x5) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 576, 0x00000300 | (0x5) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 608, 0x00000300 | (0x5) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 640, 0x00000300 | (0x5) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 672, 0x00000300 | (0x5) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 704, 0x00000300 | (0x5) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 736, 0x00000300 | (0x5) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 768, 0x00000300 | (0x5) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 800, 0x00000300 | (0x5) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 832, 0x00000300 | (0x5) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 864, 0x00000300 | (0x5) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 896, 0x00000300 | (0x5) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 928, 0x00000300 | (0x5) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 960, 0x00000300 | (0x5) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 992, 0x00000300 | (0x5) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 1024, 0x00000300 | (0x5) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 1088, 0x00000300 | (0x6) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 1152, 0x00000300 | (0x6) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 1216, 0x00000300 | (0x6) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 1280, 0x00000300 | (0x6) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 1344, 0x00000300 | (0x6) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 1408, 0x00000300 | (0x6) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 1472, 0x00000300 | (0x6) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 1536, 0x00000300 | (0x6) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 1600, 0x00000300 | (0x6) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 1664, 0x00000300 | (0x6) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 1728, 0x00000300 | (0x6) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 1792, 0x00000300 | (0x6) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 1856, 0x00000300 | (0x6) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 1920, 0x00000300 | (0x6) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 1984, 0x00000300 | (0x6) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 2048, 0x00000300 | (0x6) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 2176, 0x00000300 | (0x7) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 2304, 0x00000300 | (0x7) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 2432, 0x00000300 | (0x7) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 2560, 0x00000300 | (0x7) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 2688, 0x00000300 | (0x7) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 2816, 0x00000300 | (0x7) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 2944, 0x00000300 | (0x7) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 3072, 0x00000300 | (0x7) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+};

+};

Quoted 3 lines · Jamin Lin

Add i2c new driver to support new register set for AST2600. AST2600 support three modes for data transfer which are byte mode, buffer mode and dma mode, respectively.

DMA

Quoted 4 lines

The global driver of i2c is used to set the new register mode and define the base clock frequency
of baseclk_1~baseclk_4.

...

drivers/i2c/busses/Kconfig config I2C_ALTERA · line 382

Quoted 2 lines

drivers/i2c/busses/Kconfig +2 -0

+config I2C_NEW_ASPEED
+	bool "Aspeed New I2C Controller"

Why can't be module?

drivers/i2c/busses/Kconfig config I2C_ALTERA · line 383

Quoted 1 line

drivers/i2c/busses/Kconfig +1 -0

+	depends on ARCH_ASPEED || MACH_ASPEED_G6

COMPILE_TEST?

Quoted 1 line

drivers/i2c/busses/Kconfig +1 -0

+	select I2C_SMBUS

...

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +6 -0

+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  Aspeed I2C Interrupt Controller.
+ *
+ *	Copyright (C) ASPEED Technology Inc.
+ *	Ryan Chen <[email protected]>

Quoted 4 lines

+ *

+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.

SPDX covers this.

...

Quoted 15 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +15 -0

+#include <linux/clk.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/reset.h>
+#include <linux/delay.h>
+#include <linux/clk-provider.h>
+#include "ast2600-i2c-global.h"

drivers/i2c/busses/ast2600-i2c-global.c line 39

Quoted 4 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +3 -0

+static const struct of_device_id aspeed_i2c_ic_of_match[] = {
+	{ .compatible = "aspeed,ast2600-i2c-global", .data = (void *)0},
 
+	{ },

No need to have comma for terminator line.

Quoted 1 line

+};

No MODULE_DEVICE_TABLE()?

...

drivers/i2c/busses/ast2600-i2c-global.c line 61

Quoted 7 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +7 -0

+static const struct aspeed_i2c_base_clk i2c_base_clk[BASE_CLK_COUNT] = {
+	/* name	target_freq */
+	{  "base_clk0",	1000000 },	/* 1M */
+	{  "base_clk1",	4000000 },	/* 4M */
+	{  "base_clk2",	10000000 },	/* 10M */
+	{  "base_clk3",	40000000 },	/* 40M */
+};

Why it's not provided as the clock provider(s)?

...

drivers/i2c/busses/ast2600-i2c-global.c line 70

Quoted 9 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +8 -0

+static u32 aspeed_i2c_ic_get_new_clk_divider(unsigned long	base_clk, struct device_node *node)
+{
+	struct clk_hw_onecell_data *onecell;
+	struct clk_hw *hw;
+	int err;
+	u32 clk_divider = 0;
+	int i, j;
+	unsigned long base_freq;

Use reversed xmas tree order for all these blocks.

The rule of thumb, btw, that any comment applies to all similar places by default (independently on which line it was given against).

drivers/i2c/busses/ast2600-i2c-global.c line 73

Quoted 2 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +2 -0

+	onecell = kzalloc(sizeof(*onecell) +
+			  (BASE_CLK_COUNT * sizeof(struct clk_hw *)),

Check overflow.h for useful macros.

drivers/i2c/busses/ast2600-i2c-global.c line 75

Quoted 3 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +2 -0

+			  GFP_KERNEL);
 
+

Redundant blank line.

drivers/i2c/busses/ast2600-i2c-global.c line 77

Quoted 3 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +2 -0

+	if (!onecell) {
 
+		pr_err("allocate clk_hw\n");

Drop all these noisy useless messages.

drivers/i2c/busses/ast2600-i2c-global.c line 83

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +6 -0

+		return 0;
+	}
+
+	onecell->num = BASE_CLK_COUNT;
+
+	pr_debug("base_clk %ld\n", base_clk);

This...

drivers/i2c/busses/ast2600-i2c-global.c line 85

Quoted 2 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +2 -0

+	for (j = 0; j < BASE_CLK_COUNT; j++) {
+		pr_debug("target clk : %ld\n", i2c_base_clk[j].base_freq);

...and this and so on...

Quoted 11 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +11 -0

+		for (i = 0; i < 0xff; i++) {
+			/*
+			 * i maps to div:
+			 * 0x00: div 1
+			 * 0x01: div 1.5
+			 * 0x02: div 2
+			 * 0x03: div 2.5
+			 * 0x04: div 3
+			 * ...
+			 * 0xFE: div 128
+			 * 0xFF: div 128.5

drivers/i2c/busses/ast2600-i2c-global.c line 101

Quoted 5 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +5 -0

+			 */
+			base_freq = base_clk * 2 / (2 + i);
+			if (base_freq <= i2c_base_clk[j].base_freq)
+				break;
+		}

oAre yuo sure you can't eliminate the entire for-loop? Think about it a bit, please.

drivers/i2c/busses/ast2600-i2c-global.c line 109

Quoted 9 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +8 -0

+		pr_info("i2cg - %s : %ld\n", i2c_base_clk[j].name, base_freq);
+		hw = clk_hw_register_fixed_rate(NULL, i2c_base_clk[j].name, NULL, 0, base_freq);
+		if (IS_ERR(hw)) {
+			pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
+			break;
+		}
+		onecell->hws[j] = hw;
 
+		clk_divider |= (i << (8 * j));

See above.

Quoted 8 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +8 -0

+	}
+
+	err = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, onecell);
+	if (err)
+		pr_err("failed to add i2c base clk provider: %d\n", err);
+
+	return clk_divider;
+}

...

Quoted 9 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +9 -0

+static int aspeed_i2c_global_probe(struct platform_device *pdev)
+{
+	struct aspeed_i2c_ic *i2c_ic;
+	struct device_node *node = pdev->dev.of_node;
+	const struct of_device_id *match;
+	struct clk *parent_clk;
+	unsigned long	parent_clk_frequency;
+	u32 clk_divider;
+	int ret = 0;

drivers/i2c/busses/ast2600-i2c-global.c line 131

Quoted 3 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +3 -0

+	match = of_match_node(aspeed_i2c_ic_of_match, node);
+	if (!match)
+		return -ENOMEM;

device_get_match_data()

drivers/i2c/busses/ast2600-i2c-global.c line 133

Quoted 1 line

drivers/i2c/busses/ast2600-i2c-global.c new file +1 -0

+	i2c_ic = kzalloc(sizeof(*i2c_ic), GFP_KERNEL);

devm_kzalloc()

drivers/i2c/busses/ast2600-i2c-global.c line 137

Quoted 4 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +4 -0

+	if (!i2c_ic)
+		return -ENOMEM;
+
+	i2c_ic->base = of_iomap(node, 0);

platform_get_resource

drivers/i2c/busses/ast2600-i2c-global.c line 143

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +5 -0

+	if (!i2c_ic->base) {
+		ret = -ENOMEM;
+		goto err_free_ic;
+	}
 
+	i2c_ic->bus_num = (int)match->data;

Why not auto?

drivers/i2c/busses/ast2600-i2c-global.c line 146

Quoted 2 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +2 -0

+	if (i2c_ic->bus_num) {
+		i2c_ic->parent_irq = irq_of_parse_and_map(node, 0);

platform_get_irq()

drivers/i2c/busses/ast2600-i2c-global.c line 154

Quoted 9 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +7 -0

+		if (i2c_ic->parent_irq < 0) {
+			ret = i2c_ic->parent_irq;
+			goto err_iounmap;
+		}
+	}
 
+	i2c_ic->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 
+

Redundant.

drivers/i2c/busses/ast2600-i2c-global.c line 157

Quoted 4 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +3 -0

+	if (IS_ERR(i2c_ic->rst)) {
 
+		dev_dbg(&pdev->dev,
+			"missing or invalid reset controller device tree entry");

Make it optional.

Quoted 30 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +30 -0

+	} else {
+		/* SCU I2C Reset */
+		reset_control_assert(i2c_ic->rst);
+		udelay(3);
+		reset_control_deassert(i2c_ic->rst);
+	}
+
+	/* ast2600 init */
+	writel(ASPEED_I2CG_SLAVE_PKT_NAK |
+	       ASPEED_I2CG_CTRL_NEW_REG |
+	       ASPEED_I2CG_CTRL_NEW_CLK_DIV,
+	       i2c_ic->base + ASPEED_I2CG_CTRL);
+	parent_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(parent_clk))
+		return PTR_ERR(parent_clk);
+	parent_clk_frequency = clk_get_rate(parent_clk);
+	pr_debug("parent_clk_frequency %ld\n", parent_clk_frequency);
+	clk_divider = aspeed_i2c_ic_get_new_clk_divider(parent_clk_frequency, node);
+	writel(clk_divider, i2c_ic->base + ASPEED_I2CG_CLK_DIV_CTRL);
+
+	pr_info("i2c global registered\n");
+
+	return 0;
+
+err_iounmap:
+	iounmap(i2c_ic->base);
+err_free_ic:
+	kfree(i2c_ic);
+	return ret;
+}

...

drivers/i2c/busses/ast2600-i2c-global.h line 8

Quoted 2 lines

drivers/i2c/busses/ast2600-i2c-global.h new file +2 -0

+#ifndef AST2600_I2C_GLOBAL_H
+#define AST2600_I2C_GLOBAL_H 1

Missed bits.h

Quoted 1 line

+#endif /* AST2600_I2C_GLOBAL_H */

...

Quoted 2 lines

+/*
+ * i2c-new-aspeed.c - I2C driver for the Aspeed SoC

It's a bad idea to put file name inside the file.

Quoted 5 lines

+ *
+ * Copyright (C) ASPEED Technology Inc.
+ * Ryan Chen <[email protected]>
+ *
+ */

...

Quoted 1 line

+#define AST_I2CC_TX_DIR_MASK (0x7 << 29)

GENMASK()

...

drivers/i2c/busses/i2c-new-aspeed.c line 75

Quoted 4 lines

drivers/i2c/busses/i2c-new-aspeed.c new file +4 -0

+#define AST_I2CC_GET_RX_BUF_LEN(x)		(((x) >> 24) & 0x3f)
+#define AST_I2CC_SET_RX_BUF_LEN(x)		((((x) - 1) & 0x1f) << 16)
+#define AST_I2CC_SET_TX_BUF_LEN(x)		((((x) - 1) & 0x1f) << 8)
+#define AST_I2CC_GET_TX_BUF_LEN(x)		((((x) >> 8) & 0x1f) + 1)

Ditto.

...

Quoted 1 line

+static struct ast_i2c_timing_table aspeed_old_i2c_timing_table[] = {

What the ... is this for?!

Quoted 142 lines

drivers/i2c/busses/i2c-new-aspeed.c new file +141 -0

+	/* Divisor : Base Clock : tCKHighMin : tCK High : tCK Low  */
+	/* Divisor :	  [3:0] : [23: 20]   :   [19:16]:   [15:12] */
+	{ 6, 0x00000300 | (0x0) | (0x2 << 20) | (0x2 << 16) | (0x2 << 12) },
+	{ 7, 0x00000300 | (0x0) | (0x3 << 20) | (0x3 << 16) | (0x2 << 12) },
+	{ 8, 0x00000300 | (0x0) | (0x3 << 20) | (0x3 << 16) | (0x3 << 12) },
+	{ 9, 0x00000300 | (0x0) | (0x4 << 20) | (0x4 << 16) | (0x3 << 12) },
+	{ 10, 0x00000300 | (0x0) | (0x4 << 20) | (0x4 << 16) | (0x4 << 12) },
+	{ 11, 0x00000300 | (0x0) | (0x5 << 20) | (0x5 << 16) | (0x4 << 12) },
+	{ 12, 0x00000300 | (0x0) | (0x5 << 20) | (0x5 << 16) | (0x5 << 12) },
+	{ 13, 0x00000300 | (0x0) | (0x6 << 20) | (0x6 << 16) | (0x5 << 12) },
+	{ 14, 0x00000300 | (0x0) | (0x6 << 20) | (0x6 << 16) | (0x6 << 12) },
+	{ 15, 0x00000300 | (0x0) | (0x7 << 20) | (0x7 << 16) | (0x6 << 12) },
+	{ 16, 0x00000300 | (0x0) | (0x7 << 20) | (0x7 << 16) | (0x7 << 12) },
+	{ 17, 0x00000300 | (0x0) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 18, 0x00000300 | (0x0) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 19, 0x00000300 | (0x0) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 20, 0x00000300 | (0x0) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 21, 0x00000300 | (0x0) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 22, 0x00000300 | (0x0) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 23, 0x00000300 | (0x0) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 24, 0x00000300 | (0x0) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 25, 0x00000300 | (0x0) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 26, 0x00000300 | (0x0) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 27, 0x00000300 | (0x0) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 28, 0x00000300 | (0x0) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 29, 0x00000300 | (0x0) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 30, 0x00000300 | (0x0) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 31, 0x00000300 | (0x0) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 32, 0x00000300 | (0x0) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 34, 0x00000300 | (0x1) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 36, 0x00000300 | (0x1) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 38, 0x00000300 | (0x1) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 40, 0x00000300 | (0x1) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 42, 0x00000300 | (0x1) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 44, 0x00000300 | (0x1) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 46, 0x00000300 | (0x1) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 48, 0x00000300 | (0x1) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 50, 0x00000300 | (0x1) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 52, 0x00000300 | (0x1) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 54, 0x00000300 | (0x1) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 56, 0x00000300 | (0x1) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 58, 0x00000300 | (0x1) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 60, 0x00000300 | (0x1) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 62, 0x00000300 | (0x1) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 64, 0x00000300 | (0x1) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 68, 0x00000300 | (0x2) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 72, 0x00000300 | (0x2) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 76, 0x00000300 | (0x2) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 80, 0x00000300 | (0x2) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 84, 0x00000300 | (0x2) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 88, 0x00000300 | (0x2) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 92, 0x00000300 | (0x2) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 96, 0x00000300 | (0x2) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 100, 0x00000300 | (0x2) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 104, 0x00000300 | (0x2) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 108, 0x00000300 | (0x2) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 112, 0x00000300 | (0x2) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 116, 0x00000300 | (0x2) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 120, 0x00000300 | (0x2) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 124, 0x00000300 | (0x2) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 128, 0x00000300 | (0x2) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 136, 0x00000300 | (0x3) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 144, 0x00000300 | (0x3) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 152, 0x00000300 | (0x3) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 160, 0x00000300 | (0x3) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 168, 0x00000300 | (0x3) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 176, 0x00000300 | (0x3) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 184, 0x00000300 | (0x3) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 192, 0x00000300 | (0x3) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 200, 0x00000300 | (0x3) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 208, 0x00000300 | (0x3) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 216, 0x00000300 | (0x3) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 224, 0x00000300 | (0x3) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 232, 0x00000300 | (0x3) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 240, 0x00000300 | (0x3) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 248, 0x00000300 | (0x3) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 256, 0x00000300 | (0x3) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 272, 0x00000300 | (0x4) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 288, 0x00000300 | (0x4) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 304, 0x00000300 | (0x4) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 320, 0x00000300 | (0x4) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 336, 0x00000300 | (0x4) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 352, 0x00000300 | (0x4) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 368, 0x00000300 | (0x4) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 384, 0x00000300 | (0x4) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 400, 0x00000300 | (0x4) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 416, 0x00000300 | (0x4) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 432, 0x00000300 | (0x4) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 448, 0x00000300 | (0x4) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 464, 0x00000300 | (0x4) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 480, 0x00000300 | (0x4) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 496, 0x00000300 | (0x4) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 512, 0x00000300 | (0x4) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 544, 0x00000300 | (0x5) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 576, 0x00000300 | (0x5) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 608, 0x00000300 | (0x5) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 640, 0x00000300 | (0x5) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 672, 0x00000300 | (0x5) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 704, 0x00000300 | (0x5) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 736, 0x00000300 | (0x5) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 768, 0x00000300 | (0x5) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 800, 0x00000300 | (0x5) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 832, 0x00000300 | (0x5) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 864, 0x00000300 | (0x5) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 896, 0x00000300 | (0x5) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 928, 0x00000300 | (0x5) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 960, 0x00000300 | (0x5) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 992, 0x00000300 | (0x5) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 1024, 0x00000300 | (0x5) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 1088, 0x00000300 | (0x6) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 1152, 0x00000300 | (0x6) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 1216, 0x00000300 | (0x6) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 1280, 0x00000300 | (0x6) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 1344, 0x00000300 | (0x6) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 1408, 0x00000300 | (0x6) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 1472, 0x00000300 | (0x6) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 1536, 0x00000300 | (0x6) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+	{ 1600, 0x00000300 | (0x6) | (0xc << 20) | (0xc << 16) | (0xb << 12) },
+	{ 1664, 0x00000300 | (0x6) | (0xc << 20) | (0xc << 16) | (0xc << 12) },
+	{ 1728, 0x00000300 | (0x6) | (0xd << 20) | (0xd << 16) | (0xc << 12) },
+	{ 1792, 0x00000300 | (0x6) | (0xd << 20) | (0xd << 16) | (0xd << 12) },
+	{ 1856, 0x00000300 | (0x6) | (0xe << 20) | (0xe << 16) | (0xd << 12) },
+	{ 1920, 0x00000300 | (0x6) | (0xe << 20) | (0xe << 16) | (0xe << 12) },
+	{ 1984, 0x00000300 | (0x6) | (0xf << 20) | (0xf << 16) | (0xe << 12) },
+	{ 2048, 0x00000300 | (0x6) | (0xf << 20) | (0xf << 16) | (0xf << 12) },
+
+	{ 2176, 0x00000300 | (0x7) | (0x8 << 20) | (0x8 << 16) | (0x7 << 12) },
+	{ 2304, 0x00000300 | (0x7) | (0x8 << 20) | (0x8 << 16) | (0x8 << 12) },
+	{ 2432, 0x00000300 | (0x7) | (0x9 << 20) | (0x9 << 16) | (0x8 << 12) },
+	{ 2560, 0x00000300 | (0x7) | (0x9 << 20) | (0x9 << 16) | (0x9 << 12) },
+	{ 2688, 0x00000300 | (0x7) | (0xa << 20) | (0xa << 16) | (0x9 << 12) },
+	{ 2816, 0x00000300 | (0x7) | (0xa << 20) | (0xa << 16) | (0xa << 12) },
+	{ 2944, 0x00000300 | (0x7) | (0xb << 20) | (0xb << 16) | (0xa << 12) },
+	{ 3072, 0x00000300 | (0x7) | (0xb << 20) | (0xb << 16) | (0xb << 12) },
+};

...

drivers/i2c/busses/i2c-new-aspeed.c line 373

Quoted 6 lines

drivers/i2c/busses/i2c-new-aspeed.c new file +5 -0

+enum xfer_mode {
+	BYTE_MODE = 0,
+	BUFF_MODE,
+	DMA_MODE,
 
+

Redundant blank line.

Quoted 1 line

+};

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 1 line · Andy Shevchenko

On Thu, Jun 17, 2021 at 05:43:40PM +0800, Jamin Lin wrote:

Quoted 7 lines

Quoted 4 lines

+ *

+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.

SPDX covers this.

Will change as following what do you think?
// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Aspeed I2C Interrupt Controller.
* Copyright (C) ASPEED Technology Inc.
* Ryan Chen <[email protected]>
*/

Quoted 9 lines

drivers/i2c/busses/ast2600-i2c-global.c line 61

Quoted 7 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +7 -0

+static const struct aspeed_i2c_base_clk i2c_base_clk[BASE_CLK_COUNT] = {
+	/* name	target_freq */
+	{  "base_clk0",	1000000 },	/* 1M */
+	{  "base_clk1",	4000000 },	/* 4M */
+	{  "base_clk2",	10000000 },	/* 10M */
+	{  "base_clk3",	40000000 },	/* 40M */
+};

Why it's not provided as the clock provider(s)?

According to the design of ASPEED AST2600 SOC, the I2C bus is connected to PHB bus. The clock driver provides PHB clock and its default frequency is 100MHZ. AST2600 support the bus frequency of I2C from 0.1kbps to 5Mbps if PHB clock is 50MHZ. To meet the different bus frequency of I2C, we use this programmer to calculate the divider to change the base clock.
For example,
It calculates divider to change base_clock 1 to 1M to support I2C bus frequency 100KHZ It calculates divider to change base_clock 2 to 4M to support I2C bus frequency 400KHZ

Quoted 9 lines

drivers/i2c/busses/ast2600-i2c-global.c line 70

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +6 -0

+	struct clk_hw_onecell_data *onecell;
+	struct clk_hw *hw;
+	int err;
+	u32 clk_divider = 0;
+	int i, j;
+	unsigned long base_freq;

Use reversed xmas tree order for all these blocks.

Quoted 3 lines

The rule of thumb, btw, that any comment applies to all similar places by default (independently on which line it was given against).

Do you mena change as following?
struct clk_hw_onecell_data *onecell;
unsigned long base_freq;
u32 clk_divider = 0;
struct clk_hw *hw;
int err;
int i;
int j;

Quoted 11 lines

Quoted 11 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +11 -0

+		for (i = 0; i < 0xff; i++) {
+			/*
+			 * i maps to div:
+			 * 0x00: div 1
+			 * 0x01: div 1.5
+			 * 0x02: div 2
+			 * 0x03: div 2.5
+			 * 0x04: div 3
+			 * ...
+			 * 0xFE: div 128
+			 * 0xFF: div 128.5

Quoted 9 lines

drivers/i2c/busses/ast2600-i2c-global.c line 101

Quoted 5 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +5 -0

+			 */
+			base_freq = base_clk * 2 / (2 + i);
+			if (base_freq <= i2c_base_clk[j].base_freq)
+				break;
+		}

oAre yuo sure you can't eliminate the entire for-loop? Think about it a bit, please.

What do you think if we use "lookup table" instaed of above for loop?

Quoted 8 lines

drivers/i2c/busses/ast2600-i2c-global.c line 157

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +4 -0

+	i2c_ic->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 
+	if (IS_ERR(i2c_ic->rst)) {
 
+		dev_dbg(&pdev->dev,
+			"missing or invalid reset controller device tree entry");

Make it optional.

Can I change to use "dev_err"?

Quoted 6 lines

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +6 -0

+	} else {
+		/* SCU I2C Reset */
+		reset_control_assert(i2c_ic->rst);
+		udelay(3);
+		reset_control_deassert(i2c_ic->rst);
+	}

Quoted 3 lines

Quoted 1 line

+static struct ast_i2c_timing_table aspeed_old_i2c_timing_table[] = {

What the ... is this for?!

AST2600 support old/new register set for I2C controller.
This lookup table is used to find the divisor for the specific I2C bus frequency for AST2600 I2C controller with old register set.
For example
If I2C bus frequency is 100KHZ and PHB clock is 100MHZ, it will find this table because 100000000/1024 < 100000
"{ 1024, 0x00000300 | (0x5) | (0xf << 20) | (0xf << 16) | (0xf << 12) }"

Quoted 4 lines

Quoted 4 lines

drivers/i2c/busses/i2c-new-aspeed.c new file +3 -0

+	/* Divisor : Base Clock : tCKHighMin : tCK High : tCK Low  */
+	/* Divisor :	  [3:0] : [23: 20]   :   [19:16]:   [15:12] */
 
+};

Quoted 2 lines · Jamin Lin

Quoted 1 line · Andy Shevchenko

On Thu, Jun 17, 2021 at 05:43:40PM +0800, Jamin Lin wrote:

...

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

+ *

+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.

SPDX covers this.

Quoted 7 lines

Will change as following what do you think?
// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Aspeed I2C Interrupt Controller.
* Copyright (C) ASPEED Technology Inc.
* Ryan Chen <[email protected]>
*/

Yes, something like this.

...

Quoted 9 lines

Quoted 9 lines

drivers/i2c/busses/ast2600-i2c-global.c line 61

Quoted 7 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +7 -0

+static const struct aspeed_i2c_base_clk i2c_base_clk[BASE_CLK_COUNT] = {
+	/* name	target_freq */
+	{  "base_clk0",	1000000 },	/* 1M */
+	{  "base_clk1",	4000000 },	/* 4M */
+	{  "base_clk2",	10000000 },	/* 10M */
+	{  "base_clk3",	40000000 },	/* 40M */
+};

Why it's not provided as the clock provider(s)?

Quoted 8 lines

According to the design of ASPEED AST2600 SOC, the I2C bus is connected to PHB bus. The clock driver provides PHB clock and its default frequency is 100MHZ. AST2600 support the bus frequency of I2C from 0.1kbps to 5Mbps if PHB clock is 50MHZ. To meet the different bus frequency of I2C, we use this programmer to calculate the divider to change the base clock.
For example,
It calculates divider to change base_clock 1 to 1M to support I2C bus frequency 100KHZ It calculates divider to change base_clock 2 to 4M to support I2C bus frequency 400KHZ

My question is, why don't you provide a clock provider (under drivers/clk) for this platform and use it instead?

...

Quoted 12 lines

Quoted 9 lines

drivers/i2c/busses/ast2600-i2c-global.c line 70

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +6 -0

+	struct clk_hw_onecell_data *onecell;
+	struct clk_hw *hw;
+	int err;
+	u32 clk_divider = 0;
+	int i, j;
+	unsigned long base_freq;

Use reversed xmas tree order for all these blocks.

Quoted 3 lines

The rule of thumb, btw, that any comment applies to all similar places by default (independently on which line it was given against).

Quoted 8 lines

Do you mena change as following?
struct clk_hw_onecell_data *onecell;
unsigned long base_freq;
u32 clk_divider = 0;
struct clk_hw *hw;
int err;
int i;
int j;

Yes.

...

Quoted 20 lines

Quoted 20 lines

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +6 -0

+		for (i = 0; i < 0xff; i++) {
+			/*
+			 * i maps to div:
+			 * 0x00: div 1
+			 * 0x01: div 1.5
+			 * 0x02: div 2

drivers/i2c/busses/ast2600-i2c-global.c line 101

Quoted 10 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +10 -0

+			 * 0x03: div 2.5
+			 * 0x04: div 3
+			 * ...
+			 * 0xFE: div 128
+			 * 0xFF: div 128.5
+			 */
+			base_freq = base_clk * 2 / (2 + i);
+			if (base_freq <= i2c_base_clk[j].base_freq)
+				break;
+		}

oAre yuo sure you can't eliminate the entire for-loop? Think about it a bit, please.

Quoted 1 line

What do you think if we use "lookup table" instaed of above for loop?

Besides that it should be a part of clock provider, no, you may use a formula (bit operations and so on).

...

Quoted 9 lines

Quoted 8 lines

drivers/i2c/busses/ast2600-i2c-global.c line 157

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +4 -0

+	i2c_ic->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 
+	if (IS_ERR(i2c_ic->rst)) {
 
+		dev_dbg(&pdev->dev,
+			"missing or invalid reset controller device tree entry");

Make it optional.

Can I change to use "dev_err"?

What I meant here is to make the reset optional (there is even specific API for that) and return an error in that case.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/i2c/busses/ast2600-i2c-global.c new file +6 -0

+	} else {
+		/* SCU I2C Reset */
+		reset_control_assert(i2c_ic->rst);
+		udelay(3);
+		reset_control_deassert(i2c_ic->rst);
+	}

...

Quoted 10 lines

Quoted 3 lines

Quoted 1 line

+static struct ast_i2c_timing_table aspeed_old_i2c_timing_table[] = {

What the ... is this for?!

AST2600 support old/new register set for I2C controller.
This lookup table is used to find the divisor for the specific I2C bus frequency for AST2600 I2C controller with old register set.
For example
If I2C bus frequency is 100KHZ and PHB clock is 100MHZ, it will find this table because 100000000/1024 < 100000
"{ 1024, 0x00000300 | (0x5) | (0xf << 20) | (0xf << 16) | (0xf << 12) }"

Can't you derive it by formula(s)?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/i2c/busses/i2c-new-aspeed.c new file +3 -0

+	/* Divisor : Base Clock : tCKHighMin : tCK High : tCK Low  */
+	/* Divisor :	  [3:0] : [23: 20]   :   [19:16]:   [15:12] */
 
+};

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Add i2c new driver to support new register set for AST2600. AST2600 support three modes for data transfer which are byte mode, buffer mode and dma mode, respectively. The global driver of i2c is used to set the new register mode and define the base clock frequency
of baseclk_1~baseclk_4.

Signed-off-by Jamin Lin <[email protected]>

---
 drivers/i2c/busses/Kconfig              |   11 +
 drivers/i2c/busses/Makefile             |    1 +
 drivers/i2c/busses/ast2600-i2c-global.c |  205 +++
 drivers/i2c/busses/ast2600-i2c-global.h |   25 +
 drivers/i2c/busses/i2c-new-aspeed.c     | 1796 +++++++++++++++++++++++

Quoted 7 lines · Jamin Lin

Add i2c new driver to support new register set for AST2600. AST2600 support three modes for data transfer which are byte mode, buffer mode and dma mode, respectively. The global driver of i2c is used to set the new register mode and define the base clock frequency
of baseclk_1~baseclk_4.

Quoted 7 lines

Signed-off-by Jamin Lin <[email protected]>

---
 drivers/i2c/busses/Kconfig              |   11 +
 drivers/i2c/busses/Makefile             |    1 +
 drivers/i2c/busses/ast2600-i2c-global.c |  205 +++
 drivers/i2c/busses/ast2600-i2c-global.h |   25 +
 drivers/i2c/busses/i2c-new-aspeed.c     | 1796 +++++++++++++++++++++++

I commented _something_ (but read comments carefully, they will cover much more). The overall it seems you have to:
- shrink the code base by at least ~15% (it's possible), i.e. -200 LOCs

- rethink how you do calculations and bit operations
- better code style

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 7 lines · Andy Shevchenko

Quoted 6 lines · Jamin Lin

Add i2c new driver to support new register set for AST2600. AST2600 support three modes for data transfer which are byte mode, buffer mode and dma mode, respectively. The global driver of i2c is used to set the new register mode and define the base clock frequency
of baseclk_1~baseclk_4.

Quoted 1 line

- shrink the code base by at least ~15% (it's possible), i.e. -200 LOCs

Can you describe it more detail?
Do you mean I should separate the patch file to fix size limitation?

Quoted 3 lines

- rethink how you do calculations and bit operations
- better code style

Thanks for your review and very good suggestion
I will update them and sent patch again.
By the way, I received test failed email from Robot due to compiling warning. I will fix them, too.

Quoted 8 lines · Jamin Lin

Quoted 7 lines · Andy Shevchenko

Quoted 6 lines · Jamin Lin

Add i2c new driver to support new register set for AST2600. AST2600 support three modes for data transfer which are byte mode, buffer mode and dma mode, respectively. The global driver of i2c is used to set the new register mode and define the base clock frequency
of baseclk_1~baseclk_4.

Quoted 3 lines

Quoted 1 line

- shrink the code base by at least ~15% (it's possible), i.e. -200 LOCs

Can you describe it more detail?
Do you mean I should separate the patch file to fix size limitation?

No. Based on my brief review you introduce to many redundant LOCs (lines of code). Remove them, refactor the code, make it small and neat and easy to read and understand. It is possible to achieve!

Quoted 7 lines

Quoted 3 lines

- rethink how you do calculations and bit operations
- better code style

Thanks for your review and very good suggestion
I will update them and sent patch again.
By the way, I received test failed email from Robot due to compiling warning. I will fix them, too.

Signature · 2 lines

With Best Regards,
Andy Shevchenko