[PATCH v5 0/2] add support for Socionext SynQuacer I2C controller

[PATCH v5 0/2] add support for Socionext SynQuacer I2C controller

[Ard Biesheuvel]

Add a binding and a driver for the I2C IP found in the Socionext SynQuacer
SoC, which is essentially a rebranded version of the Fujitsu F_I2C controller.

I think this is good to go now. Wolfram?

v5:
- add Rob's ack to #1
- drop unnecessary 'platform_set_drvdata(pdev, NULL)' in remove path (#2)

v4:
- clarify binding that only a single interrupt specifier is expected (#1)
- check return value of clk_prepare_enable() on probe path (#2)
- add Andy's R-b to patch #2

v3:
- incorporate more of Andy's review comments (#2), especially regarding the
  bus speed and clock source handling for ACPI
- patch #1 unchanged.

v2:
- incorporate Andy's review comments (#2)
- patch #1 unchanged.

Ard Biesheuvel (2):
  dt-bindings: i2c: add binding for Socionext SynQuacer I2C
  i2c: add support for Socionext SynQuacer I2C controller

 Documentation/devicetree/bindings/i2c/i2c-synquacer.txt |  29 +
 drivers/i2c/busses/Kconfig                              |  10 +
 drivers/i2c/busses/Makefile                             |   1 +
 drivers/i2c/busses/i2c-synquacer.c                      | 791 ++++++++++++++++++++
 4 files changed, 831 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-synquacer.txt
 create mode 100644 drivers/i2c/busses/i2c-synquacer.c

-- 
2.11.0

[PATCH v5 1/2] dt-bindings: i2c: add binding for Socionext SynQuacer I2C

[Ard Biesheuvel]

Add a binding for the I2C controller that can be found in the Socionext
SynQuacer SoC.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/i2c/i2c-synquacer.txt | 29 ++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-synquacer.txt b/Documentation/devicetree/bindings/i2c/i2c-synquacer.txt
new file mode 100644
index 000000000000..72f4a2f0fedc
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-synquacer.txt
@@ -0,0 +1,29 @@
+Socionext SynQuacer I2C
+
+Required properties:
+- compatible      : Must be "socionext,synquacer-i2c"
+- reg             : Offset and length of the register set for the device
+- interrupts      : A single interrupt specifier
+- #address-cells  : Must be <1>;
+- #size-cells     : Must be <0>;
+- clock-names     : Must contain "pclk".
+- clocks          : Must contain an entry for each name in clock-names.
+                    (See the common clock bindings.)
+
+Optional properties:
+- clock-frequency : Desired I2C bus clock frequency in Hz. As only Normal and
+                    Fast modes are supported, possible values are 100000 and
+                    400000.
+
+Example :
+
+    i2c@51210000 {
+        compatible = "socionext,synquacer-i2c";
+        reg = <0x51210000 0x1000>;
+        interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        clock-names = "pclk";
+        clocks = <&clk_i2c>;
+        clock-frequency = <400000>;
+    };
-- 
2.11.0

[PATCH v5 2/2] i2c: add support for Socionext SynQuacer I2C controller

[Ard Biesheuvel]

This is a cleaned up version of the I2C controller driver for
the Fujitsu F_I2C IP, which was never supported upstream, and
has now been incorporated into the Socionext SynQuacer SoC.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/i2c/busses/Kconfig         |  10 +
 drivers/i2c/busses/Makefile        |   1 +
 drivers/i2c/busses/i2c-synquacer.c | 791 ++++++++++++++++++++
 3 files changed, 802 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e2954fb86d65..56903952d364 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -997,6 +997,16 @@ config I2C_SUN6I_P2WI
 	  This interface is used to connect to specific PMIC devices (like the
 	  AXP221).
 
+config I2C_SYNQUACER
+	tristate "Socionext SynQuacer I2C controller"
+	depends on ARCH_SYNQUACER || COMPILE_TEST
+	help
+	  Say Y here to include support for the I2C controller used in some
+	  Fujitsu and Socionext SoCs.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called i2c-synquacer.
+
 config I2C_TEGRA
 	tristate "NVIDIA Tegra internal I2C controller"
 	depends on ARCH_TEGRA
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 2ce8576540a2..40ab7bfc3458 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_I2C_STM32F4)	+= i2c-stm32f4.o
 obj-$(CONFIG_I2C_STM32F7)	+= i2c-stm32f7.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
 obj-$(CONFIG_I2C_SUN6I_P2WI)	+= i2c-sun6i-p2wi.o
+obj-$(CONFIG_I2C_SYNQUACER)	+= i2c-synquacer.o
 obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_TEGRA_BPMP)	+= i2c-tegra-bpmp.o
 obj-$(CONFIG_I2C_UNIPHIER)	+= i2c-uniphier.o
diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c
new file mode 100644
index 000000000000..22096f06f3cf
--- /dev/null
+++ b/drivers/i2c/busses/i2c-synquacer.c
@@ -0,0 +1,791 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2012 FUJITSU SEMICONDUCTOR LIMITED
+ */
+
+#include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#define WAIT_PCLK(n, rate)	\
+	ndelay(DIV_ROUND_UP(DIV_ROUND_UP(1000000000, rate), n) + 10)
+
+/* I2C register address definitions */
+#define SYNQUACER_I2C_REG_BSR		(0x00 << 2) // Bus Status
+#define SYNQUACER_I2C_REG_BCR		(0x01 << 2) // Bus Control
+#define SYNQUACER_I2C_REG_CCR		(0x02 << 2) // Clock Control
+#define SYNQUACER_I2C_REG_ADR		(0x03 << 2) // Address
+#define SYNQUACER_I2C_REG_DAR		(0x04 << 2) // Data
+#define SYNQUACER_I2C_REG_CSR		(0x05 << 2) // Expansion CS
+#define SYNQUACER_I2C_REG_FSR		(0x06 << 2) // Bus Clock Freq
+#define SYNQUACER_I2C_REG_BC2R		(0x07 << 2) // Bus Control 2
+
+/* I2C register bit definitions */
+#define SYNQUACER_I2C_BSR_FBT		BIT(0)	// First Byte Transfer
+#define SYNQUACER_I2C_BSR_GCA		BIT(1)	// General Call Address
+#define SYNQUACER_I2C_BSR_AAS		BIT(2)	// Address as Slave
+#define SYNQUACER_I2C_BSR_TRX		BIT(3)	// Transfer/Receive
+#define SYNQUACER_I2C_BSR_LRB		BIT(4)	// Last Received Bit
+#define SYNQUACER_I2C_BSR_AL		BIT(5)	// Arbitration Lost
+#define SYNQUACER_I2C_BSR_RSC		BIT(6)	// Repeated Start Cond.
+#define SYNQUACER_I2C_BSR_BB		BIT(7)	// Bus Busy
+
+#define SYNQUACER_I2C_BCR_INT		BIT(0)	// Interrupt
+#define SYNQUACER_I2C_BCR_INTE		BIT(1)	// Interrupt Enable
+#define SYNQUACER_I2C_BCR_GCAA		BIT(2)	// Gen. Call Access Ack.
+#define SYNQUACER_I2C_BCR_ACK		BIT(3)	// Acknowledge
+#define SYNQUACER_I2C_BCR_MSS		BIT(4)	// Master Slave Select
+#define SYNQUACER_I2C_BCR_SCC		BIT(5)	// Start Condition Cont.
+#define SYNQUACER_I2C_BCR_BEIE		BIT(6)	// Bus Error Int Enable
+#define SYNQUACER_I2C_BCR_BER		BIT(7)	// Bus Error
+
+#define SYNQUACER_I2C_CCR_CS_MASK	(0x1f)	// CCR Clock Period Sel.
+#define SYNQUACER_I2C_CCR_EN		BIT(5)	// Enable
+#define SYNQUACER_I2C_CCR_FM		BIT(6)	// Speed Mode Select
+
+#define SYNQUACER_I2C_CSR_CS_MASK	(0x3f)	// CSR Clock Period Sel.
+
+#define SYNQUACER_I2C_BC2R_SCLL		BIT(0)	// SCL Low Drive
+#define SYNQUACER_I2C_BC2R_SDAL		BIT(1)	// SDA Low Drive
+#define SYNQUACER_I2C_BC2R_SCLS		BIT(4)	// SCL Status
+#define SYNQUACER_I2C_BC2R_SDAS		BIT(5)	// SDA Status
+
+/* PCLK frequency */
+#define SYNQUACER_I2C_BUS_CLK_FR(rate)	(((rate) / 20000000) + 1)
+
+/* STANDARD MODE frequency */
+#define SYNQUACER_I2C_CLK_MASTER_STD(rate)			\
+	DIV_ROUND_UP(DIV_ROUND_UP((rate), 100000) - 2, 2)
+/* FAST MODE frequency */
+#define SYNQUACER_I2C_CLK_MASTER_FAST(rate)			\
+	DIV_ROUND_UP((DIV_ROUND_UP((rate), 400000) - 2) * 2, 3)
+
+/* (clkrate <= 18000000) */
+/* calculate the value of CS bits in CCR register on standard mode */
+#define SYNQUACER_I2C_CCR_CS_STD_MAX_18M(rate)			\
+	   ((SYNQUACER_I2C_CLK_MASTER_STD(rate) - 65)		\
+					& SYNQUACER_I2C_CCR_CS_MASK)
+
+/* calculate the value of CS bits in CSR register on standard mode */
+#define SYNQUACER_I2C_CSR_CS_STD_MAX_18M(rate)		0x00
+
+/* calculate the value of CS bits in CCR register on fast mode */
+#define SYNQUACER_I2C_CCR_CS_FAST_MAX_18M(rate)			\
+	   ((SYNQUACER_I2C_CLK_MASTER_FAST(rate) - 1)		\
+					& SYNQUACER_I2C_CCR_CS_MASK)
+
+/* calculate the value of CS bits in CSR register on fast mode */
+#define SYNQUACER_I2C_CSR_CS_FAST_MAX_18M(rate)		0x00
+
+/* (clkrate > 18000000) */
+/* calculate the value of CS bits in CCR register on standard mode */
+#define SYNQUACER_I2C_CCR_CS_STD_MIN_18M(rate)			\
+	   ((SYNQUACER_I2C_CLK_MASTER_STD(rate) - 1)		\
+					& SYNQUACER_I2C_CCR_CS_MASK)
+
+/* calculate the value of CS bits in CSR register on standard mode */
+#define SYNQUACER_I2C_CSR_CS_STD_MIN_18M(rate)			\
+	   (((SYNQUACER_I2C_CLK_MASTER_STD(rate) - 1) >> 5)	\
+					& SYNQUACER_I2C_CSR_CS_MASK)
+
+/* calculate the value of CS bits in CCR register on fast mode */
+#define SYNQUACER_I2C_CCR_CS_FAST_MIN_18M(rate)			\
+	   ((SYNQUACER_I2C_CLK_MASTER_FAST(rate) - 1)		\
+					& SYNQUACER_I2C_CCR_CS_MASK)
+
+/* calculate the value of CS bits in CSR register on fast mode */
+#define SYNQUACER_I2C_CSR_CS_FAST_MIN_18M(rate)			\
+	   (((SYNQUACER_I2C_CLK_MASTER_FAST(rate) - 1) >> 5)	\
+					& SYNQUACER_I2C_CSR_CS_MASK)
+
+/* min I2C clock frequency 14M */
+#define SYNQUACER_I2C_MIN_CLK_RATE	(14 * 1000000)
+/* max I2C clock frequency 200M */
+#define SYNQUACER_I2C_MAX_CLK_RATE	(200 * 1000000)
+/* I2C clock frequency 18M */
+#define SYNQUACER_I2C_CLK_RATE_18M	(18 * 1000000)
+
+#define SYNQUACER_I2C_SPEED_FM		400	// Fast Mode
+#define SYNQUACER_I2C_SPEED_SM		100	// Standard Mode
+
+enum i2c_state {
+	STATE_IDLE,
+	STATE_START,
+	STATE_READ,
+	STATE_WRITE
+};
+
+struct synquacer_i2c {
+	struct completion	completion;
+
+	struct i2c_msg		*msg;
+	u32			msg_num;
+	u32			msg_idx;
+	u32			msg_ptr;
+
+	u32			irq;
+	struct device		*dev;
+	void __iomem		*base;
+	struct clk		*pclk;
+	u32			pclkrate;
+	u32			speed_khz;
+	u32			timeout_ms;
+	enum i2c_state		state;
+	struct i2c_adapter	adapter;
+
+	bool			is_suspended;
+};
+
+static inline int is_lastmsg(struct synquacer_i2c *i2c)
+{
+	return i2c->msg_idx >= (i2c->msg_num - 1);
+}
+
+static inline int is_msglast(struct synquacer_i2c *i2c)
+{
+	return i2c->msg_ptr == (i2c->msg->len - 1);
+}
+
+static inline int is_msgend(struct synquacer_i2c *i2c)
+{
+	return i2c->msg_ptr >= i2c->msg->len;
+}
+
+static inline unsigned long calc_timeout_ms(struct synquacer_i2c *i2c,
+					    struct i2c_msg *msgs,
+					    int num)
+{
+	unsigned long bit_count = 0;
+	int i;
+
+	for (i = 0; i < num; i++, msgs++)
+		bit_count += msgs->len;
+
+	return DIV_ROUND_UP((bit_count * 9 + num * 10) * 3, 200) + 10;
+}
+
+static void synquacer_i2c_stop(struct synquacer_i2c *i2c, int ret)
+{
+	/*
+	 * clear IRQ (INT=0, BER=0)
+	 * set Stop Condition (MSS=0)
+	 * Interrupt Disable
+	 */
+	writeb(0, i2c->base + SYNQUACER_I2C_REG_BCR);
+
+	i2c->state = STATE_IDLE;
+
+	i2c->msg_ptr = 0;
+	i2c->msg = NULL;
+	i2c->msg_idx++;
+	i2c->msg_num = 0;
+	if (ret)
+		i2c->msg_idx = ret;
+
+	complete(&i2c->completion);
+}
+
+static void synquacer_i2c_hw_init(struct synquacer_i2c *i2c)
+{
+	unsigned char ccr_cs, csr_cs;
+	u32 rt = i2c->pclkrate;
+
+	/* Set own Address */
+	writeb(0, i2c->base + SYNQUACER_I2C_REG_ADR);
+
+	/* Set PCLK frequency */
+	writeb(SYNQUACER_I2C_BUS_CLK_FR(i2c->pclkrate),
+	       i2c->base + SYNQUACER_I2C_REG_FSR);
+
+	switch (i2c->speed_khz) {
+	case SYNQUACER_I2C_SPEED_FM:
+		if (i2c->pclkrate <= SYNQUACER_I2C_CLK_RATE_18M) {
+			ccr_cs = SYNQUACER_I2C_CCR_CS_FAST_MAX_18M(rt);
+			csr_cs = SYNQUACER_I2C_CSR_CS_FAST_MAX_18M(rt);
+		} else {
+			ccr_cs = SYNQUACER_I2C_CCR_CS_FAST_MIN_18M(rt);
+			csr_cs = SYNQUACER_I2C_CSR_CS_FAST_MIN_18M(rt);
+		}
+
+		/* Set Clock and enable, Set fast mode */
+		writeb(ccr_cs | SYNQUACER_I2C_CCR_FM |
+		       SYNQUACER_I2C_CCR_EN,
+		       i2c->base + SYNQUACER_I2C_REG_CCR);
+		writeb(csr_cs, i2c->base + SYNQUACER_I2C_REG_CSR);
+		break;
+	case SYNQUACER_I2C_SPEED_SM:
+		if (i2c->pclkrate <= SYNQUACER_I2C_CLK_RATE_18M) {
+			ccr_cs = SYNQUACER_I2C_CCR_CS_STD_MAX_18M(rt);
+			csr_cs = SYNQUACER_I2C_CSR_CS_STD_MAX_18M(rt);
+		} else {
+			ccr_cs = SYNQUACER_I2C_CCR_CS_STD_MIN_18M(rt);
+			csr_cs = SYNQUACER_I2C_CSR_CS_STD_MIN_18M(rt);
+		}
+
+		/* Set Clock and enable, Set standard mode */
+		writeb(ccr_cs | SYNQUACER_I2C_CCR_EN,
+		      i2c->base + SYNQUACER_I2C_REG_CCR);
+		writeb(csr_cs, i2c->base + SYNQUACER_I2C_REG_CSR);
+		break;
+	default:
+		WARN_ON(1);
+	}
+
+	/* clear IRQ (INT=0, BER=0), Interrupt Disable */
+	writeb(0, i2c->base + SYNQUACER_I2C_REG_BCR);
+	writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
+}
+
+static void synquacer_i2c_hw_reset(struct synquacer_i2c *i2c)
+{
+	/* Disable clock */
+	writeb(0, i2c->base + SYNQUACER_I2C_REG_CCR);
+	writeb(0, i2c->base + SYNQUACER_I2C_REG_CSR);
+
+	WAIT_PCLK(100, i2c->pclkrate);
+
+	synquacer_i2c_hw_init(i2c);
+}
+
+static int synquacer_i2c_master_start(struct synquacer_i2c *i2c,
+				      struct i2c_msg *pmsg)
+{
+	unsigned char bsr, bcr;
+
+	if (pmsg->flags & I2C_M_RD)
+		writeb((pmsg->addr << 1) | 1,
+		       i2c->base + SYNQUACER_I2C_REG_DAR);
+	else
+		writeb(pmsg->addr << 1,
+		       i2c->base + SYNQUACER_I2C_REG_DAR);
+
+	dev_dbg(i2c->dev, "slave:0x%02x\n", pmsg->addr);
+
+	/* Generate Start Condition */
+	bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
+	bcr = readb(i2c->base + SYNQUACER_I2C_REG_BCR);
+	dev_dbg(i2c->dev, "bsr:0x%02x, bcr:0x%02x\n", bsr, bcr);
+
+	if ((bsr & SYNQUACER_I2C_BSR_BB) &&
+	    !(bcr & SYNQUACER_I2C_BCR_MSS)) {
+		dev_dbg(i2c->dev, "bus is busy");
+		return -EBUSY;
+	}
+
+	if (bsr & SYNQUACER_I2C_BSR_BB) { /* Bus is busy */
+		dev_dbg(i2c->dev, "Continuous Start");
+		writeb(bcr | SYNQUACER_I2C_BCR_SCC,
+		       i2c->base + SYNQUACER_I2C_REG_BCR);
+	} else {
+		if (bcr & SYNQUACER_I2C_BCR_MSS) {
+			dev_dbg(i2c->dev, "not in master mode");
+			return -EAGAIN;
+		}
+		dev_dbg(i2c->dev, "Start Condition");
+		/* Start Condition + Enable Interrupts */
+		writeb(bcr | SYNQUACER_I2C_BCR_MSS |
+		       SYNQUACER_I2C_BCR_INTE | SYNQUACER_I2C_BCR_BEIE,
+		       i2c->base + SYNQUACER_I2C_REG_BCR);
+	}
+
+	WAIT_PCLK(10, i2c->pclkrate);
+
+	/* get BSR & BCR registers */
+	bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
+	bcr = readb(i2c->base + SYNQUACER_I2C_REG_BCR);
+	dev_dbg(i2c->dev, "bsr:0x%02x, bcr:0x%02x\n", bsr, bcr);
+
+	if ((bsr & SYNQUACER_I2C_BSR_AL) ||
+	    !(bcr & SYNQUACER_I2C_BCR_MSS)) {
+		dev_dbg(i2c->dev, "arbitration lost\n");
+		return -EAGAIN;
+	}
+
+	return 0;
+}
+
+static int synquacer_i2c_master_recover(struct synquacer_i2c *i2c)
+{
+	unsigned int count;
+	unsigned char bc2r;
+
+	/* Disable interrupts */
+	writeb(0, i2c->base + SYNQUACER_I2C_REG_BCR);
+
+	/* monitor SDA, SCL */
+	bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
+	dev_dbg(i2c->dev, "bc2r:0x%02x\n", bc2r);
+
+	count = 100;
+	do {
+		WAIT_PCLK(20, i2c->pclkrate);
+		bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
+
+		/* another master is running */
+		if ((bc2r & SYNQUACER_I2C_BC2R_SDAS) ||
+		    !(bc2r & SYNQUACER_I2C_BC2R_SCLS)) {
+			dev_dbg(i2c->dev, "another master is running?\n");
+			return -EAGAIN;
+		}
+	} while (--count);
+
+	/* Force to make one clock pulse */
+	count = 10;
+	do {
+		/* SCL = L->H */
+		writeb(SYNQUACER_I2C_BC2R_SCLL,
+		       i2c->base + SYNQUACER_I2C_REG_BC2R);
+		WAIT_PCLK(20, i2c->pclkrate);
+		writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
+
+		WAIT_PCLK(10, i2c->pclkrate);
+
+		bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
+
+		WAIT_PCLK(5, i2c->pclkrate);
+
+		if (bc2r & SYNQUACER_I2C_BC2R_SDAS)
+			break;
+		WAIT_PCLK(10, i2c->pclkrate);
+	} while (--count);
+
+	if (!count) {
+		dev_err(i2c->dev, "bc2r: 0x%x\n",  bc2r);
+		return -EIO;
+	}
+
+	/* force to make bus-error phase */
+	/* SDA = L */
+	writeb(SYNQUACER_I2C_BC2R_SDAL,
+	       i2c->base + SYNQUACER_I2C_REG_BC2R);
+	WAIT_PCLK(10, i2c->pclkrate);
+	/* SDA = H */
+	writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
+	WAIT_PCLK(10, i2c->pclkrate);
+
+	/* Both SDA & SDL should be H */
+	bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
+	if ((bc2r & (SYNQUACER_I2C_BC2R_SDAS | SYNQUACER_I2C_BC2R_SCLS)) !=
+	    (SYNQUACER_I2C_BC2R_SDAS | SYNQUACER_I2C_BC2R_SCLS)) {
+		dev_err(i2c->dev, "bc2r: 0x%x\n", bc2r);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
+				struct i2c_msg *msgs, int num)
+{
+	unsigned char bsr;
+	unsigned long timeout, bb_timeout;
+	int ret;
+
+	if (i2c->is_suspended)
+		return -EBUSY;
+
+	synquacer_i2c_hw_init(i2c);
+	bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
+	if (bsr & SYNQUACER_I2C_BSR_BB) {
+		dev_err(i2c->dev, "cannot get bus (bus busy)\n");
+		return -EBUSY;
+	}
+
+	init_completion(&i2c->completion);
+
+	i2c->msg = msgs;
+	i2c->msg_num = num;
+	i2c->msg_ptr = 0;
+	i2c->msg_idx = 0;
+	i2c->state = STATE_START;
+
+	ret = synquacer_i2c_master_start(i2c, i2c->msg);
+	if (ret < 0) {
+		dev_dbg(i2c->dev, "Address failed: (%d)\n", ret);
+		return ret;
+	}
+
+	timeout = wait_for_completion_timeout(&i2c->completion,
+					msecs_to_jiffies(i2c->timeout_ms));
+	if (timeout <= 0) {
+		dev_dbg(i2c->dev, "timeout\n");
+		return -EAGAIN;
+	}
+
+	ret = i2c->msg_idx;
+	if (ret != num) {
+		dev_dbg(i2c->dev, "incomplete xfer (%d)\n", ret);
+		return -EAGAIN;
+	}
+
+	/* ensure the stop has been through the bus */
+	bb_timeout = jiffies + HZ;
+	do {
+		bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
+		if (!(bsr & SYNQUACER_I2C_BSR_BB))
+			return 0;
+	} while (time_before(jiffies, bb_timeout));
+
+	return -ETIMEDOUT;
+}
+
+static irqreturn_t synquacer_i2c_isr(int irq, void *dev_id)
+{
+	struct synquacer_i2c *i2c = dev_id;
+
+	unsigned char byte;
+	unsigned char bsr, bcr;
+	int ret;
+
+	bcr = readb(i2c->base + SYNQUACER_I2C_REG_BCR);
+	bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
+	dev_dbg(i2c->dev, "bsr:0x%02x, bcr:0x%02x\n", bsr, bcr);
+
+	if (bcr & SYNQUACER_I2C_BCR_BER) {
+		dev_err(i2c->dev, "bus error\n");
+		synquacer_i2c_stop(i2c, -EAGAIN);
+		goto out;
+	}
+	if ((bsr & SYNQUACER_I2C_BSR_AL) ||
+	    !(bcr & SYNQUACER_I2C_BCR_MSS)) {
+		dev_dbg(i2c->dev, "arbitration lost\n");
+		synquacer_i2c_stop(i2c, -EAGAIN);
+		goto out;
+	}
+
+	switch (i2c->state) {
+
+	case STATE_START:
+		if (bsr & SYNQUACER_I2C_BSR_LRB) {
+			dev_dbg(i2c->dev, "ack was not received\n");
+			synquacer_i2c_stop(i2c, -EAGAIN);
+			goto out;
+		}
+
+		if (i2c->msg->flags & I2C_M_RD)
+			i2c->state = STATE_READ;
+		else
+			i2c->state = STATE_WRITE;
+
+		if (is_lastmsg(i2c) && i2c->msg->len == 0) {
+			synquacer_i2c_stop(i2c, 0);
+			goto out;
+		}
+
+		if (i2c->state == STATE_READ)
+			goto prepare_read;
+
+		/* fallthru */
+
+	case STATE_WRITE:
+		if (bsr & SYNQUACER_I2C_BSR_LRB) {
+			dev_dbg(i2c->dev, "WRITE: No Ack\n");
+			synquacer_i2c_stop(i2c, -EAGAIN);
+			goto out;
+		}
+
+		if (!is_msgend(i2c)) {
+			writeb(i2c->msg->buf[i2c->msg_ptr++],
+			       i2c->base + SYNQUACER_I2C_REG_DAR);
+
+			/* clear IRQ, and continue */
+			writeb(SYNQUACER_I2C_BCR_BEIE |
+			       SYNQUACER_I2C_BCR_MSS |
+			       SYNQUACER_I2C_BCR_INTE,
+			       i2c->base + SYNQUACER_I2C_REG_BCR);
+			break;
+		}
+		if (is_lastmsg(i2c)) {
+			synquacer_i2c_stop(i2c, 0);
+			break;
+		}
+		dev_dbg(i2c->dev, "WRITE: Next Message\n");
+
+		i2c->msg_ptr = 0;
+		i2c->msg_idx++;
+		i2c->msg++;
+
+		/* send the new start */
+		ret = synquacer_i2c_master_start(i2c, i2c->msg);
+		if (ret < 0) {
+			dev_dbg(i2c->dev, "restart error (%d)\n", ret);
+			synquacer_i2c_stop(i2c, -EAGAIN);
+			break;
+		}
+		i2c->state = STATE_START;
+		break;
+
+	case STATE_READ:
+		byte = readb(i2c->base + SYNQUACER_I2C_REG_DAR);
+		if (!(bsr & SYNQUACER_I2C_BSR_FBT)) /* data */
+			i2c->msg->buf[i2c->msg_ptr++] = byte;
+		else /* address */
+			dev_dbg(i2c->dev, "address:0x%02x. ignore it.\n", byte);
+
+prepare_read:
+		if (is_msglast(i2c)) {
+			writeb(SYNQUACER_I2C_BCR_MSS |
+			       SYNQUACER_I2C_BCR_BEIE |
+			       SYNQUACER_I2C_BCR_INTE,
+			       i2c->base + SYNQUACER_I2C_REG_BCR);
+			break;
+		}
+		if (!is_msgend(i2c)) {
+			writeb(SYNQUACER_I2C_BCR_MSS |
+			       SYNQUACER_I2C_BCR_BEIE |
+			       SYNQUACER_I2C_BCR_INTE |
+			       SYNQUACER_I2C_BCR_ACK,
+			       i2c->base + SYNQUACER_I2C_REG_BCR);
+			break;
+		}
+		if (is_lastmsg(i2c)) {
+			/* last message, send stop and complete */
+			dev_dbg(i2c->dev, "READ: Send Stop\n");
+			synquacer_i2c_stop(i2c, 0);
+			break;
+		}
+		dev_dbg(i2c->dev, "READ: Next Transfer\n");
+
+		i2c->msg_ptr = 0;
+		i2c->msg_idx++;
+		i2c->msg++;
+
+		ret = synquacer_i2c_master_start(i2c, i2c->msg);
+		if (ret < 0) {
+			dev_dbg(i2c->dev, "restart error (%d)\n", ret);
+			synquacer_i2c_stop(i2c, -EAGAIN);
+		} else {
+			i2c->state = STATE_START;
+		}
+		break;
+	default:
+		dev_err(i2c->dev, "called in err STATE (%d)\n", i2c->state);
+		break;
+	}
+
+out:
+	WAIT_PCLK(10, i2c->pclkrate);
+	return IRQ_HANDLED;
+}
+
+static int synquacer_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			      int num)
+{
+	struct synquacer_i2c *i2c;
+	int retry;
+	int ret;
+
+	if (!msgs)
+		return -EINVAL;
+	if (num <= 0)
+		return -EINVAL;
+
+	i2c = i2c_get_adapdata(adap);
+	i2c->timeout_ms = calc_timeout_ms(i2c, msgs, num);
+
+	dev_dbg(i2c->dev, "calculated timeout %d ms\n", i2c->timeout_ms);
+
+	for (retry = 0; retry < adap->retries; retry++) {
+
+		ret = synquacer_i2c_doxfer(i2c, msgs, num);
+		if (ret != -EAGAIN)
+			return ret;
+
+		dev_dbg(i2c->dev, "Retrying transmission (%d)\n", retry);
+
+		synquacer_i2c_master_recover(i2c);
+		synquacer_i2c_hw_reset(i2c);
+	}
+	return -EIO;
+}
+
+static u32 synquacer_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm synquacer_i2c_algo = {
+	.master_xfer	= synquacer_i2c_xfer,
+	.functionality	= synquacer_i2c_functionality,
+};
+
+static struct i2c_adapter synquacer_i2c_ops = {
+	.owner		= THIS_MODULE,
+	.name		= "synquacer_i2c-adapter",
+	.algo		= &synquacer_i2c_algo,
+	.retries	= 5,
+};
+
+static int synquacer_i2c_probe(struct platform_device *pdev)
+{
+	struct synquacer_i2c *i2c;
+	struct resource *r;
+	u32 bus_speed;
+	int ret;
+
+	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	bus_speed = i2c_acpi_find_bus_speed(&pdev->dev);
+	if (!bus_speed)
+		device_property_read_u32(&pdev->dev, "clock-frequency",
+					 &bus_speed);
+
+	device_property_read_u32(&pdev->dev, "socionext,pclk-rate",
+				 &i2c->pclkrate);
+
+	i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(i2c->pclk) && PTR_ERR(i2c->pclk) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (!IS_ERR_OR_NULL(i2c->pclk)) {
+		dev_dbg(&pdev->dev, "clock source %p\n", i2c->pclk);
+
+		ret = clk_prepare_enable(i2c->pclk);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to enable clock (%d)\n",
+				ret);
+			return ret;
+		}
+		i2c->pclkrate = clk_get_rate(i2c->pclk);
+	}
+
+	if (i2c->pclkrate < SYNQUACER_I2C_MIN_CLK_RATE ||
+	    i2c->pclkrate > SYNQUACER_I2C_MAX_CLK_RATE) {
+		dev_err(&pdev->dev, "PCLK missing or out of range (%d)\n",
+			i2c->pclkrate);
+		return -EINVAL;
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	i2c->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(i2c->base))
+		return PTR_ERR(i2c->base);
+
+	i2c->irq = platform_get_irq(pdev, 0);
+	if (i2c->irq < 0) {
+		dev_err(&pdev->dev, "no IRQ resource found\n");
+		return -ENODEV;
+	}
+
+	ret = devm_request_irq(&pdev->dev, i2c->irq, synquacer_i2c_isr,
+			       0, dev_name(&pdev->dev), i2c);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "cannot claim IRQ %d\n", i2c->irq);
+		return ret;
+	}
+
+	i2c->state = STATE_IDLE;
+	i2c->dev = &pdev->dev;
+	i2c->adapter = synquacer_i2c_ops;
+	i2c_set_adapdata(&i2c->adapter, i2c);
+	i2c->adapter.dev.parent = &pdev->dev;
+	i2c->adapter.nr = pdev->id;
+
+	if (bus_speed < 400000)
+		i2c->speed_khz = SYNQUACER_I2C_SPEED_SM;
+	else
+		i2c->speed_khz = SYNQUACER_I2C_SPEED_FM;
+
+	synquacer_i2c_hw_init(i2c);
+
+	ret = i2c_add_numbered_adapter(&i2c->adapter);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add bus to i2c core\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	dev_info(&pdev->dev, "%s: synquacer_i2c adapter\n",
+		 dev_name(&i2c->adapter.dev));
+
+	return 0;
+}
+
+static int synquacer_i2c_remove(struct platform_device *pdev)
+{
+	struct synquacer_i2c *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adapter);
+	if (!IS_ERR(i2c->pclk))
+		clk_disable_unprepare(i2c->pclk);
+
+	return 0;
+};
+
+static int __maybe_unused synquacer_i2c_suspend(struct device *dev)
+{
+	struct synquacer_i2c *i2c = dev_get_drvdata(dev);
+
+	i2c_lock_adapter(&i2c->adapter);
+	i2c->is_suspended = true;
+	i2c_unlock_adapter(&i2c->adapter);
+
+	if (!IS_ERR(i2c->pclk))
+		clk_disable_unprepare(i2c->pclk);
+
+	return 0;
+}
+
+static int __maybe_unused synquacer_i2c_resume(struct device *dev)
+{
+	struct synquacer_i2c *i2c = dev_get_drvdata(dev);
+	int ret = 0;
+
+	i2c_lock_adapter(&i2c->adapter);
+
+	if (!IS_ERR(i2c->pclk))
+		ret = clk_prepare_enable(i2c->pclk);
+	if (!ret)
+		i2c->is_suspended = false;
+
+	i2c_unlock_adapter(&i2c->adapter);
+
+	return ret;
+}
+
+static SIMPLE_DEV_PM_OPS(synquacer_i2c_pm, synquacer_i2c_suspend,
+			 synquacer_i2c_resume);
+
+static const struct of_device_id synquacer_i2c_dt_ids[] = {
+	{ .compatible = "socionext,synquacer-i2c" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, synquacer_i2c_dt_ids);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id synquacer_i2c_acpi_ids[] = {
+	{ "SCX0003" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(acpi, synquacer_i2c_acpi_ids);
+#endif
+
+static struct platform_driver synquacer_i2c_driver = {
+	.probe	= synquacer_i2c_probe,
+	.remove	= synquacer_i2c_remove,
+	.driver	= {
+		.name = "synquacer_i2c",
+		.of_match_table = of_match_ptr(synquacer_i2c_dt_ids),
+		.acpi_match_table = ACPI_PTR(synquacer_i2c_acpi_ids),
+		.pm = &synquacer_i2c_pm,
+	},
+};
+module_platform_driver(synquacer_i2c_driver);
+
+MODULE_AUTHOR("Fujitsu Semiconductor Ltd");
+MODULE_DESCRIPTION("Socionext SynQuacer I2C Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH v5 0/2] add support for Socionext SynQuacer I2C controller

[Ard Biesheuvel]

On 2 March 2018 at 08:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Add a binding and a driver for the I2C IP found in the Socionext SynQuacer
> SoC, which is essentially a rebranded version of the Fujitsu F_I2C controller.
>
> I think this is good to go now. Wolfram?
>

Ping?

> v5:
> - add Rob's ack to #1
> - drop unnecessary 'platform_set_drvdata(pdev, NULL)' in remove path (#2)
>
> v4:
> - clarify binding that only a single interrupt specifier is expected (#1)
> - check return value of clk_prepare_enable() on probe path (#2)
> - add Andy's R-b to patch #2
>
> v3:
> - incorporate more of Andy's review comments (#2), especially regarding the
>   bus speed and clock source handling for ACPI
> - patch #1 unchanged.
>
> v2:
> - incorporate Andy's review comments (#2)
> - patch #1 unchanged.
>
> Ard Biesheuvel (2):
>   dt-bindings: i2c: add binding for Socionext SynQuacer I2C
>   i2c: add support for Socionext SynQuacer I2C controller
>
>  Documentation/devicetree/bindings/i2c/i2c-synquacer.txt |  29 +
>  drivers/i2c/busses/Kconfig                              |  10 +
>  drivers/i2c/busses/Makefile                             |   1 +
>  drivers/i2c/busses/i2c-synquacer.c                      | 791 ++++++++++++++++++++
>  4 files changed, 831 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-synquacer.txt
>  create mode 100644 drivers/i2c/busses/i2c-synquacer.c
>
> --
> 2.11.0
>

Re: [PATCH v5 0/2] add support for Socionext SynQuacer I2C controller

[Ard Biesheuvel]

On 10 March 2018 at 15:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 2 March 2018 at 08:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> Add a binding and a driver for the I2C IP found in the Socionext SynQuacer
>> SoC, which is essentially a rebranded version of the Fujitsu F_I2C controller.
>>
>> I think this is good to go now. Wolfram?
>>
>
> Ping?
>

Ping?

>> v5:
>> - add Rob's ack to #1
>> - drop unnecessary 'platform_set_drvdata(pdev, NULL)' in remove path (#2)
>>
>> v4:
>> - clarify binding that only a single interrupt specifier is expected (#1)
>> - check return value of clk_prepare_enable() on probe path (#2)
>> - add Andy's R-b to patch #2
>>
>> v3:
>> - incorporate more of Andy's review comments (#2), especially regarding the
>>   bus speed and clock source handling for ACPI
>> - patch #1 unchanged.
>>
>> v2:
>> - incorporate Andy's review comments (#2)
>> - patch #1 unchanged.
>>
>> Ard Biesheuvel (2):
>>   dt-bindings: i2c: add binding for Socionext SynQuacer I2C
>>   i2c: add support for Socionext SynQuacer I2C controller
>>
>>  Documentation/devicetree/bindings/i2c/i2c-synquacer.txt |  29 +
>>  drivers/i2c/busses/Kconfig                              |  10 +
>>  drivers/i2c/busses/Makefile                             |   1 +
>>  drivers/i2c/busses/i2c-synquacer.c                      | 791 ++++++++++++++++++++
>>  4 files changed, 831 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-synquacer.txt
>>  create mode 100644 drivers/i2c/busses/i2c-synquacer.c
>>
>> --
>> 2.11.0
>>

Re: [PATCH v5 0/2] add support for Socionext SynQuacer I2C controller

[Ard Biesheuvel]

On 17 March 2018 at 00:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 10 March 2018 at 15:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 2 March 2018 at 08:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> Add a binding and a driver for the I2C IP found in the Socionext SynQuacer
>>> SoC, which is essentially a rebranded version of the Fujitsu F_I2C controller.
>>>
>>> I think this is good to go now. Wolfram?
>>>
>>
>> Ping?
>>
>
> Ping?
>

Hello Wolfram,

Do you intend to queue this for v4.17? Or are there any issues that
need to be addressed first?

Thanks,
Ard.


>>> v5:
>>> - add Rob's ack to #1
>>> - drop unnecessary 'platform_set_drvdata(pdev, NULL)' in remove path (#2)
>>>
>>> v4:
>>> - clarify binding that only a single interrupt specifier is expected (#1)
>>> - check return value of clk_prepare_enable() on probe path (#2)
>>> - add Andy's R-b to patch #2
>>>
>>> v3:
>>> - incorporate more of Andy's review comments (#2), especially regarding the
>>>   bus speed and clock source handling for ACPI
>>> - patch #1 unchanged.
>>>
>>> v2:
>>> - incorporate Andy's review comments (#2)
>>> - patch #1 unchanged.
>>>
>>> Ard Biesheuvel (2):
>>>   dt-bindings: i2c: add binding for Socionext SynQuacer I2C
>>>   i2c: add support for Socionext SynQuacer I2C controller
>>>
>>>  Documentation/devicetree/bindings/i2c/i2c-synquacer.txt |  29 +
>>>  drivers/i2c/busses/Kconfig                              |  10 +
>>>  drivers/i2c/busses/Makefile                             |   1 +
>>>  drivers/i2c/busses/i2c-synquacer.c                      | 791 ++++++++++++++++++++
>>>  4 files changed, 831 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-synquacer.txt
>>>  create mode 100644 drivers/i2c/busses/i2c-synquacer.c
>>>
>>> --
>>> 2.11.0
>>>

Re: [PATCH v5 2/2] i2c: add support for Socionext SynQuacer I2C controller

[Wolfram Sang]

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

Hi Ard,

> +static int synquacer_i2c_master_start(struct synquacer_i2c *i2c,
> +				      struct i2c_msg *pmsg)
> +{
> +	unsigned char bsr, bcr;
> +
> +	if (pmsg->flags & I2C_M_RD)
> +		writeb((pmsg->addr << 1) | 1,
> +		       i2c->base + SYNQUACER_I2C_REG_DAR);
> +	else
> +		writeb(pmsg->addr << 1,
> +		       i2c->base + SYNQUACER_I2C_REG_DAR);

writeb(i2c_8bit_addr_from_msg(pmsg), i2c->base + SYNQUACER_I2C_REG_DAR);

?

> +static int synquacer_i2c_master_recover(struct synquacer_i2c *i2c)
> +{

This is the bus recovery mechanism with toggling SCL pulses, right?
That should be implemented using a 'struct i2c_bus_recovery_info' and
the core helpers.

> +static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
> +				struct i2c_msg *msgs, int num)
> +{
> +	unsigned char bsr;
> +	unsigned long timeout, bb_timeout;
> +	int ret;
> +
> +	if (i2c->is_suspended)
> +		return -EBUSY;
> +
> +	synquacer_i2c_hw_init(i2c);
> +	bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
> +	if (bsr & SYNQUACER_I2C_BSR_BB) {
> +		dev_err(i2c->dev, "cannot get bus (bus busy)\n");
> +		return -EBUSY;
> +	}
> +
> +	init_completion(&i2c->completion);

reinit_completion? And init_completion() in probe()?

> +	/* ensure the stop has been through the bus */
> +	bb_timeout = jiffies + HZ;
> +	do {
> +		bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
> +		if (!(bsr & SYNQUACER_I2C_BSR_BB))
> +			return 0;
> +	} while (time_before(jiffies, bb_timeout));

Busy looping for one second? And won't the bus_free detection at the
beginning of a transfer do?

> +static int synquacer_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			      int num)
> +{
> +	struct synquacer_i2c *i2c;
> +	int retry;
> +	int ret;
> +
> +	if (!msgs)
> +		return -EINVAL;
> +	if (num <= 0)
> +		return -EINVAL;

Hmm, this should be done by the core. I am surprised it doesn't do that yet :/

> +
> +	i2c = i2c_get_adapdata(adap);
> +	i2c->timeout_ms = calc_timeout_ms(i2c, msgs, num);
> +
> +	dev_dbg(i2c->dev, "calculated timeout %d ms\n", i2c->timeout_ms);
> +
> +	for (retry = 0; retry < adap->retries; retry++) {
> +
> +		ret = synquacer_i2c_doxfer(i2c, msgs, num);
> +		if (ret != -EAGAIN)
> +			return ret;
> +
> +		dev_dbg(i2c->dev, "Retrying transmission (%d)\n", retry);
> +
> +		synquacer_i2c_master_recover(i2c);

Recovery is only when SDA is stuck low, held by a client. That is
nothing you should do just on any error.

If you want the driver in v4.17, I'd suggest to drop
synquacer_i2c_master_recover() now and add it incrementally later, using
the existing recovery infrastructure. The rest is only minor stuff and
needs not much further discussion IMO.

Regards,

   Wolfram


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

Re: [PATCH v5 2/2] i2c: add support for Socionext SynQuacer I2C controller

[Wolfram Sang]

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

My code checkers mention this:

    CPPCHECK
drivers/i2c/busses/i2c-synquacer.c:422: style: Checking if unsigned variable 'timeout' is less than zero.


  CC      drivers/i2c/busses/i2c-synquacer.o
drivers/i2c/busses/i2c-synquacer.c: In function ‘synquacer_i2c_probe’:
drivers/i2c/busses/i2c-synquacer.c:678:15: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
  if (i2c->irq < 0) {
               ^

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

Re: [PATCH v5 2/2] i2c: add support for Socionext SynQuacer I2C controller

[Ard Biesheuvel]

On 24 March 2018 at 23:19, Wolfram Sang <wsa@the-dreams.de> wrote:
> Hi Ard,
>
>> +static int synquacer_i2c_master_start(struct synquacer_i2c *i2c,
>> +                                   struct i2c_msg *pmsg)
>> +{
>> +     unsigned char bsr, bcr;
>> +
>> +     if (pmsg->flags & I2C_M_RD)
>> +             writeb((pmsg->addr << 1) | 1,
>> +                    i2c->base + SYNQUACER_I2C_REG_DAR);
>> +     else
>> +             writeb(pmsg->addr << 1,
>> +                    i2c->base + SYNQUACER_I2C_REG_DAR);
>
> writeb(i2c_8bit_addr_from_msg(pmsg), i2c->base + SYNQUACER_I2C_REG_DAR);
>
> ?
>
>> +static int synquacer_i2c_master_recover(struct synquacer_i2c *i2c)
>> +{
>
> This is the bus recovery mechanism with toggling SCL pulses, right?
> That should be implemented using a 'struct i2c_bus_recovery_info' and
> the core helpers.
>
>> +static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
>> +                             struct i2c_msg *msgs, int num)
>> +{
>> +     unsigned char bsr;
>> +     unsigned long timeout, bb_timeout;
>> +     int ret;
>> +
>> +     if (i2c->is_suspended)
>> +             return -EBUSY;
>> +
>> +     synquacer_i2c_hw_init(i2c);
>> +     bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
>> +     if (bsr & SYNQUACER_I2C_BSR_BB) {
>> +             dev_err(i2c->dev, "cannot get bus (bus busy)\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     init_completion(&i2c->completion);
>
> reinit_completion? And init_completion() in probe()?
>
>> +     /* ensure the stop has been through the bus */
>> +     bb_timeout = jiffies + HZ;
>> +     do {
>> +             bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
>> +             if (!(bsr & SYNQUACER_I2C_BSR_BB))
>> +                     return 0;
>> +     } while (time_before(jiffies, bb_timeout));
>
> Busy looping for one second? And won't the bus_free detection at the
> beginning of a transfer do?
>
>> +static int synquacer_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>> +                           int num)
>> +{
>> +     struct synquacer_i2c *i2c;
>> +     int retry;
>> +     int ret;
>> +
>> +     if (!msgs)
>> +             return -EINVAL;
>> +     if (num <= 0)
>> +             return -EINVAL;
>
> Hmm, this should be done by the core. I am surprised it doesn't do that yet :/
>
>> +
>> +     i2c = i2c_get_adapdata(adap);
>> +     i2c->timeout_ms = calc_timeout_ms(i2c, msgs, num);
>> +
>> +     dev_dbg(i2c->dev, "calculated timeout %d ms\n", i2c->timeout_ms);
>> +
>> +     for (retry = 0; retry < adap->retries; retry++) {
>> +
>> +             ret = synquacer_i2c_doxfer(i2c, msgs, num);
>> +             if (ret != -EAGAIN)
>> +                     return ret;
>> +
>> +             dev_dbg(i2c->dev, "Retrying transmission (%d)\n", retry);
>> +
>> +             synquacer_i2c_master_recover(i2c);
>
> Recovery is only when SDA is stuck low, held by a client. That is
> nothing you should do just on any error.
>
> If you want the driver in v4.17, I'd suggest to drop
> synquacer_i2c_master_recover() now and add it incrementally later, using
> the existing recovery infrastructure. The rest is only minor stuff and
> needs not much further discussion IMO.
>

Thanks Wolfram.

I have fixed all these up and am about to send out the v6.