* [PATCH v2 0/2]add support for Socionext SynQuacer I2C controller
@ 2018-02-22 19:16 Ard Biesheuvel
2018-02-22 19:16 ` [PATCH v2 1/2] dt-bindings: i2c: add binding for Socionext SynQuacer I2C Ard Biesheuvel
2018-02-22 19:16 ` [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller Ard Biesheuvel
0 siblings, 2 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2018-02-22 19:16 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, linux-i2c, devicetree
Cc: linux-kernel, linux-arm-kernel, jassisinghbrar, andy.shevchenko,
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.
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 | 11 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-synquacer.c | 796 ++++++++++++++++++++
4 files changed, 837 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-synquacer.txt
create mode 100644 drivers/i2c/busses/i2c-synquacer.c
--
2.11.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/2] dt-bindings: i2c: add binding for Socionext SynQuacer I2C
2018-02-22 19:16 [PATCH v2 0/2]add support for Socionext SynQuacer I2C controller Ard Biesheuvel
@ 2018-02-22 19:16 ` Ard Biesheuvel
2018-02-22 19:16 ` [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller Ard Biesheuvel
1 sibling, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2018-02-22 19:16 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, linux-i2c, devicetree
Cc: linux-kernel, linux-arm-kernel, jassisinghbrar, andy.shevchenko,
Ard Biesheuvel
Add a binding for the I2C controller that can be found in the Socionext
SynQuacer SoC. Note that this is the exact same IP as the Fujitsu F_I2C.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.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..4d0bb6a71211
--- /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 : the 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-22 19:16 [PATCH v2 0/2]add support for Socionext SynQuacer I2C controller Ard Biesheuvel
2018-02-22 19:16 ` [PATCH v2 1/2] dt-bindings: i2c: add binding for Socionext SynQuacer I2C Ard Biesheuvel
@ 2018-02-22 19:16 ` Ard Biesheuvel
2018-02-23 12:27 ` Andy Shevchenko
1 sibling, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2018-02-22 19:16 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, linux-i2c, devicetree
Cc: linux-kernel, linux-arm-kernel, jassisinghbrar, andy.shevchenko,
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>
---
drivers/i2c/busses/Kconfig | 11 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-synquacer.c | 796 ++++++++++++++++++++
3 files changed, 808 insertions(+)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a9805c7cb305..9001dbaeb60a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -995,6 +995,17 @@ 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
+ depends on OF || ACPI
+ 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..25329f65f35f
--- /dev/null
+++ b/drivers/i2c/busses/i2c-synquacer.c
@@ -0,0 +1,796 @@
+// 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 adress 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 *clk;
+ u32 clkrate;
+ 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 + 10 * num) * 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->clkrate;
+
+ /* Set own Address */
+ writeb(0, i2c->base + SYNQUACER_I2C_REG_ADR);
+
+ /* Set PCLK frequency */
+ writeb(SYNQUACER_I2C_BUS_CLK_FR(i2c->clkrate),
+ i2c->base + SYNQUACER_I2C_REG_FSR);
+
+ switch (i2c->speed_khz) {
+ case SYNQUACER_I2C_SPEED_FM:
+ if (i2c->clkrate <= 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->clkrate <= 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->clkrate);
+
+ 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->clkrate);
+
+ /* get bsr&bcr register */
+ 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 = 0;
+ 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);
+
+ while (count <= 100) {
+ WAIT_PCLK(20, i2c->clkrate);
+ 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;
+ }
+ count++;
+ }
+
+ /* Force to make one clock pulse */
+ for (count = 1;; count++) {
+ /* SCL = L->H */
+ writeb(SYNQUACER_I2C_BC2R_SCLL,
+ i2c->base + SYNQUACER_I2C_REG_BC2R);
+ WAIT_PCLK(20, i2c->clkrate);
+ writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
+
+ WAIT_PCLK(10, i2c->clkrate);
+
+ bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
+
+ WAIT_PCLK(5, i2c->clkrate);
+
+ if (bc2r & SYNQUACER_I2C_BC2R_SDAS)
+ break;
+ WAIT_PCLK(10, i2c->clkrate);
+ if (count > 9) {
+ dev_err(i2c->dev, "count: %i, bc2r: 0x%x\n", count,
+ 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->clkrate);
+ /* SDA = H */
+ writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
+ WAIT_PCLK(10, i2c->clkrate);
+
+ /* 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);
+ } while ((bsr & SYNQUACER_I2C_BSR_BB) &&
+ time_before(jiffies, bb_timeout));
+
+ return ret;
+}
+
+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 = 0;
+
+ 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:
+ if (!(bsr & SYNQUACER_I2C_BSR_FBT)) { /* data */
+ byte = readb(i2c->base + SYNQUACER_I2C_REG_DAR);
+ i2c->msg->buf[i2c->msg_ptr++] = byte;
+ } else /* address */
+ dev_dbg(i2c->dev, "address:0x%02x. ignore it.\n",
+ readb(i2c->base + SYNQUACER_I2C_REG_DAR));
+
+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->clkrate);
+ 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 = 0;
+
+ 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;
+ int speed_khz;
+ int ret;
+
+ ret = device_property_read_u32(&pdev->dev, "clock-frequency",
+ &speed_khz);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Missing clock-frequency property\n");
+ return -EINVAL;
+ }
+ speed_khz /= 1000;
+
+ i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ if (dev_of_node(&pdev->dev)) {
+ i2c->clk = devm_clk_get(&pdev->dev, "pclk");
+ if (IS_ERR(i2c->clk)) {
+ dev_err(&pdev->dev, "cannot get clock\n");
+ return PTR_ERR(i2c->clk);
+ }
+ dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
+
+ i2c->clkrate = clk_get_rate(i2c->clk);
+ dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
+ clk_prepare_enable(i2c->clk);
+ } else {
+ ret = device_property_read_u32(&pdev->dev,
+ "socionext,pclk-rate",
+ &i2c->clkrate);
+ if (ret)
+ return ret;
+ }
+
+ if (i2c->clkrate < SYNQUACER_I2C_MIN_CLK_RATE ||
+ i2c->clkrate > SYNQUACER_I2C_MAX_CLK_RATE) {
+ dev_err(&pdev->dev, "PCLK rate out of range (%d)\n",
+ i2c->clkrate);
+ 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);
+
+ dev_dbg(&pdev->dev, "registers %p (%p)\n", i2c->base, r);
+
+ 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->speed_khz = SYNQUACER_I2C_SPEED_SM;
+ if (speed_khz == SYNQUACER_I2C_SPEED_FM)
+ i2c->speed_khz = SYNQUACER_I2C_SPEED_FM;
+
+ synquacer_i2c_hw_init(i2c);
+
+ i2c->adapter = synquacer_i2c_ops;
+ i2c_set_adapdata(&i2c->adapter, i2c);
+ i2c->adapter.dev.parent = &pdev->dev;
+ i2c->adapter.nr = pdev->id;
+
+ 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);
+
+ platform_set_drvdata(pdev, NULL);
+ i2c_del_adapter(&i2c->adapter);
+ clk_disable_unprepare(i2c->clk);
+
+ 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);
+
+ clk_disable_unprepare(i2c->clk);
+
+ return 0;
+}
+
+static int __maybe_unused synquacer_i2c_resume(struct device *dev)
+{
+ struct synquacer_i2c *i2c = dev_get_drvdata(dev);
+ int ret;
+
+ i2c_lock_adapter(&i2c->adapter);
+
+ ret = clk_prepare_enable(i2c->clk);
+ 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);
+
+#ifdef CONFIG_OF
+static const struct of_device_id synquacer_i2c_dt_ids[] = {
+ { .compatible = "socionext,synquacer-i2c" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, synquacer_i2c_dt_ids);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id synquacer_i2c_acpi_ids[] = {
+ { "SCX0003" },
+ { /* sentinel */ }
+};
+#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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-22 19:16 ` [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller Ard Biesheuvel
@ 2018-02-23 12:27 ` Andy Shevchenko
2018-02-23 12:40 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2018-02-23 12:27 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> 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.
>
Thanks for an update.
My comments below.
In case you address them, feel free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> drivers/i2c/busses/Kconfig | 11 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-synquacer.c | 796 ++++++++++++++++++++
> 3 files changed, 808 insertions(+)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index a9805c7cb305..9001dbaeb60a 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -995,6 +995,17 @@ 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
> + depends on OF || ACPI
Does it make any sense?
> + 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..25329f65f35f
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-synquacer.c
> @@ -0,0 +1,796 @@
> +// 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 adress 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 *clk;
> + u32 clkrate;
> + 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 + 10 * num) * 3, 200) + 10;
When I suggested to drop parens, I also suggested to swap second pair
arguments (b/c I was thinking that parens to prevent confusion + vs
*), like
9 * bit_count + 10 * num, or
bit_count * 9 + num * 10.
Though, it is up to you, I still consider that + vs. * operator
precedence is quite obvious.
> +}
> +
> +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->clkrate;
> +
> + /* Set own Address */
> + writeb(0, i2c->base + SYNQUACER_I2C_REG_ADR);
> +
> + /* Set PCLK frequency */
> + writeb(SYNQUACER_I2C_BUS_CLK_FR(i2c->clkrate),
> + i2c->base + SYNQUACER_I2C_REG_FSR);
> +
> + switch (i2c->speed_khz) {
> + case SYNQUACER_I2C_SPEED_FM:
> + if (i2c->clkrate <= 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->clkrate <= 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->clkrate);
> +
> + 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->clkrate);
> +
> + /* get bsr&bcr register */
> + 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 = 0;
> + 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);
> +
> + while (count <= 100) {
> + WAIT_PCLK(20, i2c->clkrate);
> + 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;
> + }
> + count++;
> + }
I still consider do {} while not only taking less LOCs, but more
readable and understanble
unsigned int count = 100;
do {
...
} while (--count);
> +
> + /* Force to make one clock pulse */
> + for (count = 1;; count++) {
> + /* SCL = L->H */
> + writeb(SYNQUACER_I2C_BC2R_SCLL,
> + i2c->base + SYNQUACER_I2C_REG_BC2R);
> + WAIT_PCLK(20, i2c->clkrate);
> + writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
> +
> + WAIT_PCLK(10, i2c->clkrate);
> +
> + bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
> +
> + WAIT_PCLK(5, i2c->clkrate);
> +
> + if (bc2r & SYNQUACER_I2C_BC2R_SDAS)
> + break;
> + WAIT_PCLK(10, i2c->clkrate);
> + if (count > 9) {
> + dev_err(i2c->dev, "count: %i, bc2r: 0x%x\n", count,
> + bc2r);
> + return -EIO;
> + }
(1)
> + }
Ditto. More over in case (1) it's an invariant to the loop, so, it
shouldn't be there in any case.
> +
> + /* force to make bus-error phase */
> + /* SDA = L */
> + writeb(SYNQUACER_I2C_BC2R_SDAL,
> + i2c->base + SYNQUACER_I2C_REG_BC2R);
> + WAIT_PCLK(10, i2c->clkrate);
> + /* SDA = H */
> + writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
> + WAIT_PCLK(10, i2c->clkrate);
> +
> + /* 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);
> + } while ((bsr & SYNQUACER_I2C_BSR_BB) &&
> + time_before(jiffies, bb_timeout));
I would rather do split timeout exit path with main condition, i.e.
bsr = ...
if (bsr & ...)
break; // return 0; ?
} while (...);
return 0; // return -ETIMEDOUT; ?
> +
> + return ret;
return 0; ?
> +}
> +
> +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 = 0;
> +
> + 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:
> + if (!(bsr & SYNQUACER_I2C_BSR_FBT)) { /* data */
> + byte = readb(i2c->base + SYNQUACER_I2C_REG_DAR);
> + i2c->msg->buf[i2c->msg_ptr++] = byte;
> + } else /* address */
> + dev_dbg(i2c->dev, "address:0x%02x. ignore it.\n",
> + readb(i2c->base + SYNQUACER_I2C_REG_DAR));
> +
> +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->clkrate);
> + 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 = 0;
Redundant assignment.
> +
> + 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;
> + int speed_khz;
> + int ret;
> +
> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
> + &speed_khz);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Missing clock-frequency property\n");
> + return -EINVAL;
> + }
> + speed_khz /= 1000;
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> + if (!i2c)
> + return -ENOMEM;
> +
> + if (dev_of_node(&pdev->dev)) {
> + i2c->clk = devm_clk_get(&pdev->dev, "pclk");
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "cannot get clock\n");
> + return PTR_ERR(i2c->clk);
> + }
> + dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
> +
> + i2c->clkrate = clk_get_rate(i2c->clk);
> + dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
> + clk_prepare_enable(i2c->clk);
> + } else {
> + ret = device_property_read_u32(&pdev->dev,
> + "socionext,pclk-rate",
> + &i2c->clkrate);
> + if (ret)
> + return ret;
> + }
Okay, I got this case. It's more likely the one in 8250_dw.c.
Can you do the similar way?
> +
> + if (i2c->clkrate < SYNQUACER_I2C_MIN_CLK_RATE ||
> + i2c->clkrate > SYNQUACER_I2C_MAX_CLK_RATE) {
> + dev_err(&pdev->dev, "PCLK rate out of range (%d)\n",
> + i2c->clkrate);
> + 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);
> +
> + dev_dbg(&pdev->dev, "registers %p (%p)\n", i2c->base, r);
This one is achievable via /proc/iomem, isn't it?
> +
> + i2c->irq = platform_get_irq(pdev, 0);
> + if (i2c->irq <= 0) {
< 0 ?
On some platforms IRQ == 0 might be valid.
> + 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->speed_khz = SYNQUACER_I2C_SPEED_SM;
> + if (speed_khz == SYNQUACER_I2C_SPEED_FM)
> + i2c->speed_khz = SYNQUACER_I2C_SPEED_FM;
> +
> + synquacer_i2c_hw_init(i2c);
> +
> + i2c->adapter = synquacer_i2c_ops;
> + i2c_set_adapdata(&i2c->adapter, i2c);
> + i2c->adapter.dev.parent = &pdev->dev;
> + i2c->adapter.nr = pdev->id;
> +
> + 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);
> +
> + platform_set_drvdata(pdev, NULL);
> + i2c_del_adapter(&i2c->adapter);
> + clk_disable_unprepare(i2c->clk);
> +
> + 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);
> +
> + clk_disable_unprepare(i2c->clk);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused synquacer_i2c_resume(struct device *dev)
> +{
> + struct synquacer_i2c *i2c = dev_get_drvdata(dev);
> + int ret;
> +
> + i2c_lock_adapter(&i2c->adapter);
> +
> + ret = clk_prepare_enable(i2c->clk);
> + 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);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id synquacer_i2c_dt_ids[] = {
> + { .compatible = "socionext,synquacer-i2c" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, synquacer_i2c_dt_ids);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id synquacer_i2c_acpi_ids[] = {
> + { "SCX0003" },
> + { /* sentinel */ }
> +};
> +#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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-23 12:27 ` Andy Shevchenko
@ 2018-02-23 12:40 ` Ard Biesheuvel
2018-02-23 13:12 ` Andy Shevchenko
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2018-02-23 12:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On 23 February 2018 at 12:27, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> 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.
>>
>
> Thanks for an update.
> My comments below.
>
> In case you address them, feel free to add
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> drivers/i2c/busses/Kconfig | 11 +
>> drivers/i2c/busses/Makefile | 1 +
>> drivers/i2c/busses/i2c-synquacer.c | 796 ++++++++++++++++++++
>> 3 files changed, 808 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index a9805c7cb305..9001dbaeb60a 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -995,6 +995,17 @@ 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
>
>> + depends on OF || ACPI
>
> Does it make any sense?
>
Not really. ARCH_SYNQUACER only exists on arm64, and it mandates
CONFIG_OF. I will remove it.
>> + 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
...
>> +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 + 10 * num) * 3, 200) + 10;
>
> When I suggested to drop parens, I also suggested to swap second pair
> arguments (b/c I was thinking that parens to prevent confusion + vs
> *), like
>
> 9 * bit_count + 10 * num, or
> bit_count * 9 + num * 10.
>
> Though, it is up to you, I still consider that + vs. * operator
> precedence is quite obvious.
>
I can change it if you like.
>> +}
>> +
>> +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->clkrate;
>> +
>> + /* Set own Address */
>> + writeb(0, i2c->base + SYNQUACER_I2C_REG_ADR);
>> +
>> + /* Set PCLK frequency */
>> + writeb(SYNQUACER_I2C_BUS_CLK_FR(i2c->clkrate),
>> + i2c->base + SYNQUACER_I2C_REG_FSR);
>> +
>> + switch (i2c->speed_khz) {
>> + case SYNQUACER_I2C_SPEED_FM:
>> + if (i2c->clkrate <= 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->clkrate <= 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->clkrate);
>> +
>> + 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->clkrate);
>> +
>> + /* get bsr&bcr register */
>> + 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 = 0;
>> + 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);
>> +
>
>> + while (count <= 100) {
>> + WAIT_PCLK(20, i2c->clkrate);
>> + 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;
>> + }
>> + count++;
>> + }
>
> I still consider do {} while not only taking less LOCs, but more
> readable and understanble
>
> unsigned int count = 100;
>
> do {
> ...
> } while (--count);
>
Sure. I'll change that.
>> +
>> + /* Force to make one clock pulse */
>
>> + for (count = 1;; count++) {
>> + /* SCL = L->H */
>> + writeb(SYNQUACER_I2C_BC2R_SCLL,
>> + i2c->base + SYNQUACER_I2C_REG_BC2R);
>> + WAIT_PCLK(20, i2c->clkrate);
>> + writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
>> +
>> + WAIT_PCLK(10, i2c->clkrate);
>> +
>> + bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
>> +
>> + WAIT_PCLK(5, i2c->clkrate);
>> +
>> + if (bc2r & SYNQUACER_I2C_BC2R_SDAS)
>> + break;
>> + WAIT_PCLK(10, i2c->clkrate);
>
>> + if (count > 9) {
>> + dev_err(i2c->dev, "count: %i, bc2r: 0x%x\n", count,
>> + bc2r);
>> + return -EIO;
>> + }
>
> (1)
>
>> + }
>
> Ditto. More over in case (1) it's an invariant to the loop, so, it
> shouldn't be there in any case.
>
Fair enough. I will move it out.
>> +
>> + /* force to make bus-error phase */
>> + /* SDA = L */
>> + writeb(SYNQUACER_I2C_BC2R_SDAL,
>> + i2c->base + SYNQUACER_I2C_REG_BC2R);
>> + WAIT_PCLK(10, i2c->clkrate);
>> + /* SDA = H */
>> + writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
>> + WAIT_PCLK(10, i2c->clkrate);
>> +
>> + /* 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);
>> + } while ((bsr & SYNQUACER_I2C_BSR_BB) &&
>> + time_before(jiffies, bb_timeout));
>
> I would rather do split timeout exit path with main condition, i.e.
>
> bsr = ...
> if (bsr & ...)
> break; // return 0; ?
> } while (...);
>
> return 0; // return -ETIMEDOUT; ?
>
OK
>> +
>
>> + return ret;
>
> return 0; ?
>
Yes.
>> +}
>> +
>> +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 = 0;
>> +
>> + 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:
>> + if (!(bsr & SYNQUACER_I2C_BSR_FBT)) { /* data */
>> + byte = readb(i2c->base + SYNQUACER_I2C_REG_DAR);
>> + i2c->msg->buf[i2c->msg_ptr++] = byte;
>> + } else /* address */
>> + dev_dbg(i2c->dev, "address:0x%02x. ignore it.\n",
>> + readb(i2c->base + SYNQUACER_I2C_REG_DAR));
>> +
>> +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->clkrate);
>> + 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 = 0;
>
> Redundant assignment.
>
OK will remove
>> +
>> + 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;
>> + int speed_khz;
>> + int ret;
>> +
>> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
>> + &speed_khz);
>> + if (ret) {
>> + dev_err(&pdev->dev,
>> + "Missing clock-frequency property\n");
>> + return -EINVAL;
>> + }
>> + speed_khz /= 1000;
>> +
>> + i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
>> + if (!i2c)
>> + return -ENOMEM;
>> +
>
>> + if (dev_of_node(&pdev->dev)) {
>> + i2c->clk = devm_clk_get(&pdev->dev, "pclk");
>> + if (IS_ERR(i2c->clk)) {
>> + dev_err(&pdev->dev, "cannot get clock\n");
>> + return PTR_ERR(i2c->clk);
>> + }
>> + dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
>> +
>> + i2c->clkrate = clk_get_rate(i2c->clk);
>> + dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
>> + clk_prepare_enable(i2c->clk);
>> + } else {
>> + ret = device_property_read_u32(&pdev->dev,
>> + "socionext,pclk-rate",
>> + &i2c->clkrate);
>> + if (ret)
>> + return ret;
>> + }
>
> Okay, I got this case. It's more likely the one in 8250_dw.c.
>
> Can you do the similar way?
>
Could you elaborate?
>> +
>> + if (i2c->clkrate < SYNQUACER_I2C_MIN_CLK_RATE ||
>> + i2c->clkrate > SYNQUACER_I2C_MAX_CLK_RATE) {
>> + dev_err(&pdev->dev, "PCLK rate out of range (%d)\n",
>> + i2c->clkrate);
>> + 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);
>> +
>
>> + dev_dbg(&pdev->dev, "registers %p (%p)\n", i2c->base, r);
>
> This one is achievable via /proc/iomem, isn't it?
>
Yes, I can drop that.
>> +
>> + i2c->irq = platform_get_irq(pdev, 0);
>> + if (i2c->irq <= 0) {
>
> < 0 ?
>
> On some platforms IRQ == 0 might be valid.
>
Are you sure about that?
http://yarchive.net/comp/linux/no_irq.html
>> + 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->speed_khz = SYNQUACER_I2C_SPEED_SM;
>> + if (speed_khz == SYNQUACER_I2C_SPEED_FM)
>> + i2c->speed_khz = SYNQUACER_I2C_SPEED_FM;
>> +
>> + synquacer_i2c_hw_init(i2c);
>> +
>> + i2c->adapter = synquacer_i2c_ops;
>> + i2c_set_adapdata(&i2c->adapter, i2c);
>> + i2c->adapter.dev.parent = &pdev->dev;
>> + i2c->adapter.nr = pdev->id;
>> +
>> + 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);
>> +
>> + platform_set_drvdata(pdev, NULL);
>> + i2c_del_adapter(&i2c->adapter);
>> + clk_disable_unprepare(i2c->clk);
>> +
>> + 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);
>> +
>> + clk_disable_unprepare(i2c->clk);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused synquacer_i2c_resume(struct device *dev)
>> +{
>> + struct synquacer_i2c *i2c = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + i2c_lock_adapter(&i2c->adapter);
>> +
>> + ret = clk_prepare_enable(i2c->clk);
>> + 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);
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id synquacer_i2c_dt_ids[] = {
>> + { .compatible = "socionext,synquacer-i2c" },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, synquacer_i2c_dt_ids);
>> +#endif
>> +
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id synquacer_i2c_acpi_ids[] = {
>> + { "SCX0003" },
>> + { /* sentinel */ }
>> +};
>> +#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
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-23 12:40 ` Ard Biesheuvel
@ 2018-02-23 13:12 ` Andy Shevchenko
2018-02-26 9:59 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2018-02-23 13:12 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 23 February 2018 at 12:27, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> 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.
>>> + return DIV_ROUND_UP((bit_count * 9 + 10 * num) * 3, 200) + 10;
>>
>> When I suggested to drop parens, I also suggested to swap second pair
>> arguments (b/c I was thinking that parens to prevent confusion + vs
>> *), like
>>
>> 9 * bit_count + 10 * num, or
>> bit_count * 9 + num * 10.
>>
>> Though, it is up to you, I still consider that + vs. * operator
>> precedence is quite obvious.
> I can change it if you like.
I guess slightly better to change, thanks.
>>> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
>>> + &speed_khz);
>>> + if (ret) {
>>> + dev_err(&pdev->dev,
>>> + "Missing clock-frequency property\n");
>>> + return -EINVAL;
>>> + }
>>> + speed_khz /= 1000;
>>> + if (dev_of_node(&pdev->dev)) {
>>> + i2c->clk = devm_clk_get(&pdev->dev, "pclk");
>>> + if (IS_ERR(i2c->clk)) {
>>> + dev_err(&pdev->dev, "cannot get clock\n");
>>> + return PTR_ERR(i2c->clk);
>>> + }
>>> + dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
>>> +
>>> + i2c->clkrate = clk_get_rate(i2c->clk);
>>> + dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
>>> + clk_prepare_enable(i2c->clk);
>>> + } else {
>>> + ret = device_property_read_u32(&pdev->dev,
>>> + "socionext,pclk-rate",
>>> + &i2c->clkrate);
>>> + if (ret)
>>> + return ret;
>>> + }
>>
>> Okay, I got this case. It's more likely the one in 8250_dw.c.
>>
>> Can you do the similar way?
> Could you elaborate?
--- 8< --- 8< --- 8< ---
device_property_read_u32(dev, "clock-frequency", &p->uartclk);
/* If there is separate baudclk, get the rate from it. */
data->clk = devm_clk_get(dev, "baudclk");
...
if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
if (!IS_ERR_OR_NULL(data->clk)) {
err = clk_prepare_enable(data->clk);
if (err)
dev_warn(dev, "could not enable optional baudclk: %d\n",
err);
else
p->uartclk = clk_get_rate(data->clk);
}
/* If no clock rate is defined, fail. */
if (!p->uartclk) {
dev_err(dev, "clock rate not defined\n");
err = -EINVAL;
goto err_clk;
--- 8< --- 8< --- 8< ---
Replace 'baudclk' with 'pclk' and p->uartclk with i2c->clkrate in
above and you are almost done.
>>> + i2c->irq = platform_get_irq(pdev, 0);
>>> + if (i2c->irq <= 0) {
>>
>> < 0 ?
>>
>> On some platforms IRQ == 0 might be valid.
> Are you sure about that?
Yes. I fixed some cases on one of a such.
> http://yarchive.net/comp/linux/no_irq.html
I agree with Linus from software IRQ (and nowadays luckily we are
using IRQ descriptors), but I disagree with him from hardware
prospective.
0 is totally valid HW IRQ line. In hardware there is no descriptor
(except, yes, MSI and alike cases), it's just a wire with an index.
So, while drivers are getting better in code prospective (though I
don't see many of them comparing this to 0), the IRQ framework is
changing itself as well.
At which circumstances we might get 0 in the first place?
Second question, doesn't request_irq() fail on irq==0 if it's not
supported as valid by platform?
>>> + dev_err(&pdev->dev, "no IRQ resource found\n");
>>> + return -ENODEV;
>>> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-23 13:12 ` Andy Shevchenko
@ 2018-02-26 9:59 ` Ard Biesheuvel
2018-02-26 11:35 ` Andy Shevchenko
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2018-02-26 9:59 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On 23 February 2018 at 13:12, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 23 February 2018 at 12:27, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
...
>>>> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
>>>> + &speed_khz);
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev,
>>>> + "Missing clock-frequency property\n");
>>>> + return -EINVAL;
>>>> + }
>>>> + speed_khz /= 1000;
>
>>>> + if (dev_of_node(&pdev->dev)) {
>>>> + i2c->clk = devm_clk_get(&pdev->dev, "pclk");
>>>> + if (IS_ERR(i2c->clk)) {
>>>> + dev_err(&pdev->dev, "cannot get clock\n");
>>>> + return PTR_ERR(i2c->clk);
>>>> + }
>>>> + dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
>>>> +
>>>> + i2c->clkrate = clk_get_rate(i2c->clk);
>>>> + dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
>>>> + clk_prepare_enable(i2c->clk);
>>>> + } else {
>>>> + ret = device_property_read_u32(&pdev->dev,
>>>> + "socionext,pclk-rate",
>>>> + &i2c->clkrate);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>
>>> Okay, I got this case. It's more likely the one in 8250_dw.c.
>>>
>>> Can you do the similar way?
>
>> Could you elaborate?
>
> --- 8< --- 8< --- 8< ---
> device_property_read_u32(dev, "clock-frequency", &p->uartclk);
>
> /* If there is separate baudclk, get the rate from it. */
> data->clk = devm_clk_get(dev, "baudclk");
> ...
> if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
> return -EPROBE_DEFER;
> if (!IS_ERR_OR_NULL(data->clk)) {
> err = clk_prepare_enable(data->clk);
> if (err)
> dev_warn(dev, "could not enable optional baudclk: %d\n",
> err);
> else
> p->uartclk = clk_get_rate(data->clk);
> }
>
> /* If no clock rate is defined, fail. */
> if (!p->uartclk) {
> dev_err(dev, "clock rate not defined\n");
> err = -EINVAL;
> goto err_clk;
> --- 8< --- 8< --- 8< ---
>
> Replace 'baudclk' with 'pclk' and p->uartclk with i2c->clkrate in
> above and you are almost done.
>
I don't think this is better. The generic DT I2C 'clock-frequency'
property denotes the bus clock rate not the rate of the clock that
feeds the IP block. This is rather different from the UART bindings.
Also, I don't want to support 'socionext,pclk-rate' for DT platforms,
only for ACPI platforms.
>>>> + i2c->irq = platform_get_irq(pdev, 0);
>>>> + if (i2c->irq <= 0) {
>>>
>>> < 0 ?
>>>
>>> On some platforms IRQ == 0 might be valid.
>
>> Are you sure about that?
>
> Yes. I fixed some cases on one of a such.
>
>> http://yarchive.net/comp/linux/no_irq.html
>
> I agree with Linus from software IRQ (and nowadays luckily we are
> using IRQ descriptors), but I disagree with him from hardware
> prospective.
> 0 is totally valid HW IRQ line. In hardware there is no descriptor
> (except, yes, MSI and alike cases), it's just a wire with an index.
>
> So, while drivers are getting better in code prospective (though I
> don't see many of them comparing this to 0), the IRQ framework is
> changing itself as well.
>
> At which circumstances we might get 0 in the first place?
>
> Second question, doesn't request_irq() fail on irq==0 if it's not
> supported as valid by platform?
>
Yes, I suppose it does.
I'll change it to '< 0'
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-26 9:59 ` Ard Biesheuvel
@ 2018-02-26 11:35 ` Andy Shevchenko
2018-02-26 14:58 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2018-02-26 11:35 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On Mon, Feb 26, 2018 at 11:59 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 23 February 2018 at 13:12, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 23 February 2018 at 12:27, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>> On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
> ...
>>>>> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
>>>>> + &speed_khz);
>>>>> + if (ret) {
>>>>> + dev_err(&pdev->dev,
>>>>> + "Missing clock-frequency property\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> + speed_khz /= 1000;
>>
>>>>> + if (dev_of_node(&pdev->dev)) {
>>>>> + i2c->clk = devm_clk_get(&pdev->dev, "pclk");
>>>>> + if (IS_ERR(i2c->clk)) {
>>>>> + dev_err(&pdev->dev, "cannot get clock\n");
>>>>> + return PTR_ERR(i2c->clk);
>>>>> + }
>>>>> + dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
>>>>> +
>>>>> + i2c->clkrate = clk_get_rate(i2c->clk);
>>>>> + dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
>>>>> + clk_prepare_enable(i2c->clk);
>>>>> + } else {
>>>>> + ret = device_property_read_u32(&pdev->dev,
>>>>> + "socionext,pclk-rate",
>>>>> + &i2c->clkrate);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> + }
>>>>
>>>> Okay, I got this case. It's more likely the one in 8250_dw.c.
>>>>
>>>> Can you do the similar way?
>>
>>> Could you elaborate?
>>
>> --- 8< --- 8< --- 8< ---
>> device_property_read_u32(dev, "clock-frequency", &p->uartclk);
>>
>> /* If there is separate baudclk, get the rate from it. */
>> data->clk = devm_clk_get(dev, "baudclk");
>> ...
>> if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
>> return -EPROBE_DEFER;
>> if (!IS_ERR_OR_NULL(data->clk)) {
>> err = clk_prepare_enable(data->clk);
>> if (err)
>> dev_warn(dev, "could not enable optional baudclk: %d\n",
>> err);
>> else
>> p->uartclk = clk_get_rate(data->clk);
>> }
>>
>> /* If no clock rate is defined, fail. */
>> if (!p->uartclk) {
>> dev_err(dev, "clock rate not defined\n");
>> err = -EINVAL;
>> goto err_clk;
>> --- 8< --- 8< --- 8< ---
>>
>> Replace 'baudclk' with 'pclk' and p->uartclk with i2c->clkrate in
>> above and you are almost done.
>>
>
> I don't think this is better.
It's a pattern over ACPI vs. clk cases at least for now.
But hold on. We have already an example of dealing with ACPI /
non-ACPI cases for I2C controllers — i2c-designware-platdrv.c.
Check how it's done there.
I actually totally forgot about ACPI slaves described in the table. We
need to take into account the ones with lowest bus speed.
> The generic DT I2C 'clock-frequency'
> property denotes the bus clock rate not the rate of the clock that
> feeds the IP block. This is rather different from the UART bindings.
> Also, I don't want to support 'socionext,pclk-rate' for DT platforms,
> only for ACPI platforms.
Unfortunately, we are strong against deviations between DT and ACPI in
case of device properties.
You may provide a clock and use devm_clk_get() without any concerns
from where this comes from.
You won't find any deviation in DW I2C driver since there is none,
while driver works for non-ACPI platforms as well.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-26 11:35 ` Andy Shevchenko
@ 2018-02-26 14:58 ` Ard Biesheuvel
2018-02-26 17:05 ` Andy Shevchenko
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2018-02-26 14:58 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On 26 February 2018 at 11:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 11:59 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 23 February 2018 at 13:12, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 23 February 2018 at 12:27, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>>> On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org> wrote:
>> ...
>>>>>> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
>>>>>> + &speed_khz);
>>>>>> + if (ret) {
>>>>>> + dev_err(&pdev->dev,
>>>>>> + "Missing clock-frequency property\n");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> + speed_khz /= 1000;
>>>
>>>>>> + if (dev_of_node(&pdev->dev)) {
>>>>>> + i2c->clk = devm_clk_get(&pdev->dev, "pclk");
>>>>>> + if (IS_ERR(i2c->clk)) {
>>>>>> + dev_err(&pdev->dev, "cannot get clock\n");
>>>>>> + return PTR_ERR(i2c->clk);
>>>>>> + }
>>>>>> + dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
>>>>>> +
>>>>>> + i2c->clkrate = clk_get_rate(i2c->clk);
>>>>>> + dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
>>>>>> + clk_prepare_enable(i2c->clk);
>>>>>> + } else {
>>>>>> + ret = device_property_read_u32(&pdev->dev,
>>>>>> + "socionext,pclk-rate",
>>>>>> + &i2c->clkrate);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + }
>>>>>
>>>>> Okay, I got this case. It's more likely the one in 8250_dw.c.
>>>>>
>>>>> Can you do the similar way?
>>>
>>>> Could you elaborate?
>>>
>>> --- 8< --- 8< --- 8< ---
>>> device_property_read_u32(dev, "clock-frequency", &p->uartclk);
>>>
>>> /* If there is separate baudclk, get the rate from it. */
>>> data->clk = devm_clk_get(dev, "baudclk");
>>> ...
>>> if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
>>> return -EPROBE_DEFER;
>>> if (!IS_ERR_OR_NULL(data->clk)) {
>>> err = clk_prepare_enable(data->clk);
>>> if (err)
>>> dev_warn(dev, "could not enable optional baudclk: %d\n",
>>> err);
>>> else
>>> p->uartclk = clk_get_rate(data->clk);
>>> }
>>>
>>> /* If no clock rate is defined, fail. */
>>> if (!p->uartclk) {
>>> dev_err(dev, "clock rate not defined\n");
>>> err = -EINVAL;
>>> goto err_clk;
>>> --- 8< --- 8< --- 8< ---
>>>
>>> Replace 'baudclk' with 'pclk' and p->uartclk with i2c->clkrate in
>>> above and you are almost done.
>>>
>>
>> I don't think this is better.
>
> It's a pattern over ACPI vs. clk cases at least for now.
> But hold on. We have already an example of dealing with ACPI /
> non-ACPI cases for I2C controllers — i2c-designware-platdrv.c.
> Check how it's done there.
>
> I actually totally forgot about ACPI slaves described in the table. We
> need to take into account the ones with lowest bus speed.
>
Wow, that code is absolutely terrible.
So even while _DSD device properties require vendor prefixes, which
are lacking in this case, and the fact that the ACPI flavor of the
Designware I2C controller now provides two different ways to get the
timing parameters (using device properties or using SSCN/FMCN/etc ACPI
methods), you think this is a shining example of how this should be
implemented?
Also, I still think implementing a clock device using rate X just to
interrogate it for its rate (returning X) is absolutely pointless.
So what I can do is invent an ACPI method that returns the PCLK rate.
Would that work for you?
As for the max I2C speed: I think that looks sane, so I can
incorporate that as well.
>> The generic DT I2C 'clock-frequency'
>> property denotes the bus clock rate not the rate of the clock that
>> feeds the IP block. This is rather different from the UART bindings.
>
>> Also, I don't want to support 'socionext,pclk-rate' for DT platforms,
>> only for ACPI platforms.
>
> Unfortunately, we are strong against deviations between DT and ACPI in
> case of device properties.
> You may provide a clock and use devm_clk_get() without any concerns
> from where this comes from.
>
> You won't find any deviation in DW I2C driver since there is none,
> while driver works for non-ACPI platforms as well.
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-26 14:58 ` Ard Biesheuvel
@ 2018-02-26 17:05 ` Andy Shevchenko
2018-02-26 17:16 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2018-02-26 17:05 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On Mon, Feb 26, 2018 at 4:58 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 26 February 2018 at 11:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Mon, Feb 26, 2018 at 11:59 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 23 February 2018 at 13:12, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>> On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>> Replace 'baudclk' with 'pclk' and p->uartclk with i2c->clkrate in
>>>> above and you are almost done.
>>> I don't think this is better.
>>
>> It's a pattern over ACPI vs. clk cases at least for now.
>> But hold on. We have already an example of dealing with ACPI /
>> non-ACPI cases for I2C controllers — i2c-designware-platdrv.c.
>> Check how it's done there.
>>
>> I actually totally forgot about ACPI slaves described in the table. We
>> need to take into account the ones with lowest bus speed.
>>
>
> Wow, that code is absolutely terrible.
To some degree I may say yes it is.
> So even while _DSD device properties require vendor prefixes, which
> are lacking in this case,
What kind? clock-frequency? Does it require prefix?
> and the fact that the ACPI flavor of the
> Designware I2C controller now provides two different ways to get the
> timing parameters (using device properties or using SSCN/FMCN/etc ACPI
> methods), you think this is a shining example of how this should be
> implemented?
No, those methods because of windows driver and existed ACPI tables at
that time.
You are not supposed to uglify your case.
> Also, I still think implementing a clock device using rate X just to
> interrogate it for its rate (returning X) is absolutely pointless.
OTOH the deviation in the driver is what I absolutely against of.
Driver must not know the resource provider (ideally at all).
> So what I can do is invent an ACPI method that returns the PCLK rate.
> Would that work for you?
Again, looking into existing examples (UART, I2C, etc) we better to
create a generic helper in clock framework that would provide us a
clock based on property value.
But doing different paths for different resource providers is not what
we are looking for.
P.S. To move this somehow forward I may propose to submit an OF
driver, and discuss ACPI part after.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-26 17:05 ` Andy Shevchenko
@ 2018-02-26 17:16 ` Ard Biesheuvel
2018-02-26 17:51 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2018-02-26 17:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On 26 February 2018 at 17:05, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 4:58 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 26 February 2018 at 11:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Mon, Feb 26, 2018 at 11:59 AM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 23 February 2018 at 13:12, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>>> On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org> wrote:
>
>>>>> Replace 'baudclk' with 'pclk' and p->uartclk with i2c->clkrate in
>>>>> above and you are almost done.
>
>>>> I don't think this is better.
>>>
>>> It's a pattern over ACPI vs. clk cases at least for now.
>>> But hold on. We have already an example of dealing with ACPI /
>>> non-ACPI cases for I2C controllers — i2c-designware-platdrv.c.
>>> Check how it's done there.
>>>
>>> I actually totally forgot about ACPI slaves described in the table. We
>>> need to take into account the ones with lowest bus speed.
>>>
>>
>> Wow, that code is absolutely terrible.
>
> To some degree I may say yes it is.
>
>> So even while _DSD device properties require vendor prefixes, which
>> are lacking in this case,
>
> What kind? clock-frequency? Does it require prefix?
>
What I remember from the _DSD discussions is that we should vendor
prefixes for per-device properties, and only use unprefixed names for
generic properties. However, looking more closely, I understand that
this undermines the idea of having parity between DT and ACPI, because
DT did not require vendor prefixes in the past (but it does now)
I guess 'clock-frequency' is one that would not require such a vendor prefix.
>> and the fact that the ACPI flavor of the
>> Designware I2C controller now provides two different ways to get the
>> timing parameters (using device properties or using SSCN/FMCN/etc ACPI
>> methods), you think this is a shining example of how this should be
>> implemented?
>
> No, those methods because of windows driver and existed ACPI tables at
> that time.
> You are not supposed to uglify your case.
>
OK, in that case, can you please spell out what you think is
mandatory? Because handwavy references to existing UART and I2C
drivers are not helping me here.
>> Also, I still think implementing a clock device using rate X just to
>> interrogate it for its rate (returning X) is absolutely pointless.
>
> OTOH the deviation in the driver is what I absolutely against of.
> Driver must not know the resource provider (ideally at all).
>
There is no 'resource provider'. There is only a single number, which
is the clock rate, and is only used to calculate some internal
dividers of the I2C IP block.
>> So what I can do is invent an ACPI method that returns the PCLK rate.
>> Would that work for you?
>
> Again, looking into existing examples (UART, I2C, etc) we better to
> create a generic helper in clock framework that would provide us a
> clock based on property value.
> But doing different paths for different resource providers is not what
> we are looking for.
>
> P.S. To move this somehow forward I may propose to submit an OF
> driver, and discuss ACPI part after.
>
Thanks, but that does not really work for me. What I can do is split
it into an initial DT only driver, and a followup ACPI patch.
Can you point me to an example of such a clock provider?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-26 17:16 ` Ard Biesheuvel
@ 2018-02-26 17:51 ` Ard Biesheuvel
2018-02-26 18:18 ` Andy Shevchenko
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2018-02-26 17:51 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On 26 February 2018 at 17:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 26 February 2018 at 17:05, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Mon, Feb 26, 2018 at 4:58 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 26 February 2018 at 11:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>> On Mon, Feb 26, 2018 at 11:59 AM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>> On 23 February 2018 at 13:12, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>>>> On Fri, Feb 23, 2018 at 2:40 PM, Ard Biesheuvel
>>>>>> <ard.biesheuvel@linaro.org> wrote:
>>
>>>>>> Replace 'baudclk' with 'pclk' and p->uartclk with i2c->clkrate in
>>>>>> above and you are almost done.
>>
>>>>> I don't think this is better.
>>>>
>>>> It's a pattern over ACPI vs. clk cases at least for now.
>>>> But hold on. We have already an example of dealing with ACPI /
>>>> non-ACPI cases for I2C controllers — i2c-designware-platdrv.c.
>>>> Check how it's done there.
>>>>
>>>> I actually totally forgot about ACPI slaves described in the table. We
>>>> need to take into account the ones with lowest bus speed.
>>>>
>>>
>>> Wow, that code is absolutely terrible.
>>
>> To some degree I may say yes it is.
>>
>>> So even while _DSD device properties require vendor prefixes, which
>>> are lacking in this case,
>>
>> What kind? clock-frequency? Does it require prefix?
>>
>
> What I remember from the _DSD discussions is that we should vendor
> prefixes for per-device properties, and only use unprefixed names for
> generic properties. However, looking more closely, I understand that
> this undermines the idea of having parity between DT and ACPI, because
> DT did not require vendor prefixes in the past (but it does now)
>
> I guess 'clock-frequency' is one that would not require such a vendor prefix.
>
>>> and the fact that the ACPI flavor of the
>>> Designware I2C controller now provides two different ways to get the
>>> timing parameters (using device properties or using SSCN/FMCN/etc ACPI
>>> methods), you think this is a shining example of how this should be
>>> implemented?
>>
>> No, those methods because of windows driver and existed ACPI tables at
>> that time.
>> You are not supposed to uglify your case.
>>
>
> OK, in that case, can you please spell out what you think is
> mandatory? Because handwavy references to existing UART and I2C
> drivers are not helping me here.
>
>>> Also, I still think implementing a clock device using rate X just to
>>> interrogate it for its rate (returning X) is absolutely pointless.
>>
>> OTOH the deviation in the driver is what I absolutely against of.
>> Driver must not know the resource provider (ideally at all).
>>
>
> There is no 'resource provider'. There is only a single number, which
> is the clock rate, and is only used to calculate some internal
> dividers of the I2C IP block.
>
>>> So what I can do is invent an ACPI method that returns the PCLK rate.
>>> Would that work for you?
>>
>> Again, looking into existing examples (UART, I2C, etc) we better to
>> create a generic helper in clock framework that would provide us a
>> clock based on property value.
>> But doing different paths for different resource providers is not what
>> we are looking for.
>>
>> P.S. To move this somehow forward I may propose to submit an OF
>> driver, and discuss ACPI part after.
>>
>
> Thanks, but that does not really work for me. What I can do is split
> it into an initial DT only driver, and a followup ACPI patch.
>
> Can you point me to an example of such a clock provider?
Perhaps we could agree on the binding first? Currently, I have the following
Device (I2C0) {
Name (_HID, "SCX0003")
Name (_UID, Zero)
Name (_CRS, ResourceTemplate () {
Memory32Fixed (ReadWrite, SYNQUACER_I2C1_BASE, SYNQUACER_I2C1_SIZE)
Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 197 }
})
Name (_DSD, Package () // _DSD: Device-Specific Data
{
ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) { "socionext,pclk-rate", 62500000 },
}
})
and I don't intend to add 'clock-frequency' here because it would be
redundant anyway.
Does this look sane?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
2018-02-26 17:51 ` Ard Biesheuvel
@ 2018-02-26 18:18 ` Andy Shevchenko
0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2018-02-26 18:18 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, linux-i2c, devicetree,
Linux Kernel Mailing List, linux-arm Mailing List, Jassi Brar
On Mon, Feb 26, 2018 at 7:51 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 26 February 2018 at 17:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Perhaps we could agree on the binding first? Currently, I have the following
>
> Device (I2C0) {
> Name (_HID, "SCX0003")
> Name (_UID, Zero)
> Name (_CRS, ResourceTemplate () {
> Memory32Fixed (ReadWrite, SYNQUACER_I2C1_BASE, SYNQUACER_I2C1_SIZE)
> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 197 }
> })
>
> Name (_DSD, Package () // _DSD: Device-Specific Data
> {
> ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) { "socionext,pclk-rate", 62500000 },
> }
> })
>
> and I don't intend to add 'clock-frequency' here because it would be
> redundant anyway.
Right.
> Does this look sane?
I can't say about property name, but otherwise yes.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-02-26 18:18 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-22 19:16 [PATCH v2 0/2]add support for Socionext SynQuacer I2C controller Ard Biesheuvel
2018-02-22 19:16 ` [PATCH v2 1/2] dt-bindings: i2c: add binding for Socionext SynQuacer I2C Ard Biesheuvel
2018-02-22 19:16 ` [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller Ard Biesheuvel
2018-02-23 12:27 ` Andy Shevchenko
2018-02-23 12:40 ` Ard Biesheuvel
2018-02-23 13:12 ` Andy Shevchenko
2018-02-26 9:59 ` Ard Biesheuvel
2018-02-26 11:35 ` Andy Shevchenko
2018-02-26 14:58 ` Ard Biesheuvel
2018-02-26 17:05 ` Andy Shevchenko
2018-02-26 17:16 ` Ard Biesheuvel
2018-02-26 17:51 ` Ard Biesheuvel
2018-02-26 18:18 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).