[PATCH v3 0/6] Add Actions Semiconductor Owl S900 I2C support

[PATCH v3 0/6] Add Actions Semiconductor Owl S900 I2C support

[Manivannan Sadhasivam]

This patchset adds I2C controller support for Actions Semiconductor S900 SoC.
This driver has been structured in a way such that there will be only
one controller driver for the whole Owl family series (S500, S700 and
S900 SoCs).

There are 6 I2C controllers with separate memory mapped register space.
The I2C controller can handle atmost two messages concatenated by a
repeated start via its internal address feature. Hence the driver
uses this feature for messages of length greater than 1. In those cases,
the first message of the combined message should be a `write` with maximum
message length 6 and the second message's maximum length should be 240 bytes.

As far as the bus speed is concerned, this driver only supports
Standard (100KHz) and High speed (400KHz) for now.

The pinctrl definitions are only available for I2C0, I2C1 and I2C2.
With the mux option available only for I2C0.

For Bubblegum-96 board utilizing the S900 SoC, only I2C1 and I2C2 which
are exposed on the Low speed expansion connector are enabled.

This series depends on the pinctrl patch [1] is still not merged by the
platform maintainer Andreas but it has been reviewed by the pinctrl maintainer
Linus Walleij. For the reference, I have queued up all reviewed dts patches
in my tree [2] from which Andreas is picking them for 4.19.

For this series, since the dts patches will go through the ARM SoC tree,
Andreas will pick it up once it is reviewed.

Thanks,
Mani

[1] https://patchwork.kernel.org/patch/10322937/
[2] https://git.linaro.org/people/manivannan.sadhasivam/linux.git/log/?h=s900-for-next

Changes in v3:

As per Peter, Andy and Andreas review:

* Removed owl_i2c_reset function from interrupt handler since is
  involves the use of delays.
* Fixed the return path in owl_i2c_master_xfer and added a note
* Changed "base" parameter to "reg" in owl_i2c_update_reg for better
  understandability
* Ordered the includes in alphabetical order
* Small changes to defines and added comma at the end of struct members
* Removed rendundant 0 assignment in owl_i2c_master_xfer
* Changed all OWL naming convention to Owl and also changed Actions Semi
  Actions Semiconductor

Changes in v2:

As per Andy's review:
* Modified infinite loops to fixed number of retries
* Used i2c_8bit_addr_from_msg for constructing the slave address
* Removed unnecessary parenthesis around defines
* Modified certain dev_warn to dev_dbg
* Modified the error handling to more generic pattern

* Fixed the return value in owl_i2c_master_xfer
* Added MAINTAINERS patch for I2C driver and its binding

Manivannan Sadhasivam (6):
  dt-bindings: i2c: Add binding for Actions Semiconductor Owl I2C
    controller
  arm64: dts: actions: Add Actions Semiconductor S900 I2C controller
    nodes
  arm64: dts: actions: Add pinctrl definition for S900 I2C controller
  arm64: dts: actions: Enable I2C1 and I2C2 in Bubblegum-96 board
  i2c: Add Actions Semiconductor Owl family S900 I2C driver
  MAINTAINERS: Add entry for Actions Semiconductor Owl I2C driver

 .../devicetree/bindings/i2c/i2c-owl.txt       |  27 +
 MAINTAINERS                                   |   2 +
 .../dts/actions/s900-bubblegum-96-pins.dtsi   |  29 ++
 .../boot/dts/actions/s900-bubblegum-96.dts    |  11 +
 arch/arm64/boot/dts/actions/s900.dtsi         |  60 +++
 drivers/i2c/busses/Kconfig                    |   7 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-owl.c                  | 477 ++++++++++++++++++
 8 files changed, 614 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-owl.txt
 create mode 100644 arch/arm64/boot/dts/actions/s900-bubblegum-96-pins.dtsi
 create mode 100644 drivers/i2c/busses/i2c-owl.c

-- 
2.17.1

[PATCH v3 1/6] dt-bindings: i2c: Add binding for Actions Semiconductor Owl I2C controller

[Manivannan Sadhasivam]

Add devicetree binding for Actions Semiconductor Owl I2C controller

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../devicetree/bindings/i2c/i2c-owl.txt       | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-owl.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-owl.txt b/Documentation/devicetree/bindings/i2c/i2c-owl.txt
new file mode 100644
index 000000000000..b743fe444e9f
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-owl.txt
@@ -0,0 +1,27 @@
+Actions Semiconductor Owl I2C controller
+
+Required properties:
+
+- compatible        : Should be "actions,s900-i2c".
+- reg               : Offset and length of the register set for the device.
+- #address-cells    : Should be 1.
+- #size-cells       : Should be 0.
+- interrupts        : A single interrupt specifier.
+- clocks            : Phandle of the clock feeding the I2C controller.
+
+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.
+Examples:
+
+        i2c0: i2c@e0170000 {
+                compatible = "actions,s900-i2c";
+                reg = <0 0xe0170000 0 0x1000>;
+                #address-cells = <1>;
+                #size-cells = <0>;
+                interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
+                clocks = <&clock CLK_I2C0>;
+                clock-frequency = <100000>;
+        };
-- 
2.17.1

[PATCH v3 2/6] arm64: dts: actions: Add Actions Semiconductor S900 I2C controller nodes

[Manivannan Sadhasivam]

Add I2C controller nodes for Actions Semiconductor S900 SoC.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/actions/s900.dtsi | 60 +++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/arch/arm64/boot/dts/actions/s900.dtsi b/arch/arm64/boot/dts/actions/s900.dtsi
index 7ae8b931f000..6f7b89edbe4d 100644
--- a/arch/arm64/boot/dts/actions/s900.dtsi
+++ b/arch/arm64/boot/dts/actions/s900.dtsi
@@ -174,6 +174,66 @@
 			#clock-cells = <1>;
 		};
 
+		i2c0: i2c@e0170000 {
+			compatible = "actions,s900-i2c";
+			reg = <0 0xe0170000 0 0x1000>;
+			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c0_default>;
+		};
+
+		i2c1: i2c@e0172000 {
+			compatible = "actions,s900-i2c";
+			reg = <0 0xe0172000 0 0x1000>;
+			interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c1_default>;
+		};
+
+		i2c2: i2c@e0174000 {
+			compatible = "actions,s900-i2c";
+			reg = <0 0xe0174000 0 0x1000>;
+			interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c2_default>;
+		};
+
+		i2c3: i2c@e0176000 {
+			compatible = "actions,s900-i2c";
+			reg = <0 0xe0176000 0 0x1000>;
+			interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
+		i2c4: i2c@e0178000 {
+			compatible = "actions,s900-i2c";
+			reg = <0 0xe0178000 0 0x1000>;
+			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
+		i2c5: i2c@e017a000 {
+			compatible = "actions,s900-i2c";
+			reg = <0 0xe017a000 0 0x1000>;
+			interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		pinctrl: pinctrl@e01b0000 {
 			compatible = "actions,s900-pinctrl";
 			reg = <0x0 0xe01b0000 0x0 0x1000>;
-- 
2.17.1

[PATCH v3 3/6] arm64: dts: actions: Add pinctrl definition for S900 I2C controller

[Manivannan Sadhasivam]

Add pinctrl definition for Actions Semiconductor S900 I2C controller.
Pinctrl definitions are only available for I2C0, I2C1, and I2C2.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../dts/actions/s900-bubblegum-96-pins.dtsi   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 arch/arm64/boot/dts/actions/s900-bubblegum-96-pins.dtsi

diff --git a/arch/arm64/boot/dts/actions/s900-bubblegum-96-pins.dtsi b/arch/arm64/boot/dts/actions/s900-bubblegum-96-pins.dtsi
new file mode 100644
index 000000000000..95e8b31071f9
--- /dev/null
+++ b/arch/arm64/boot/dts/actions/s900-bubblegum-96-pins.dtsi
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+&pinctrl {
+
+	i2c0_default: i2c0_default {
+		pinmux {
+			groups = "i2c0_mfp";
+			function = "i2c0";
+		};
+		pinconf {
+			pins = "i2c0_sclk", "i2c0_sdata";
+			bias-pull-up;
+		};
+	};
+
+	i2c1_default: i2c1_default {
+		pinconf {
+			pins = "i2c1_sclk", "i2c1_sdata";
+			bias-pull-up;
+		};
+	};
+
+	i2c2_default: i2c2_default {
+		pinconf {
+			pins = "i2c2_sclk", "i2c2_sdata";
+			bias-pull-up;
+		};
+	};
+};
-- 
2.17.1

[PATCH v3 4/6] arm64: dts: actions: Enable I2C1 and I2C2 in Bubblegum-96 board

[Manivannan Sadhasivam]

Enable I2C1 and I2C2 exposed on the low speed expansion connector in
Bubblegum-96 board.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/actions/s900-bubblegum-96.dts | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/actions/s900-bubblegum-96.dts b/arch/arm64/boot/dts/actions/s900-bubblegum-96.dts
index d0ba35df9015..57ae374cfb5a 100644
--- a/arch/arm64/boot/dts/actions/s900-bubblegum-96.dts
+++ b/arch/arm64/boot/dts/actions/s900-bubblegum-96.dts
@@ -7,6 +7,7 @@
 /dts-v1/;
 
 #include "s900.dtsi"
+#include "s900-bubblegum-96-pins.dtsi"
 
 / {
 	compatible = "ucrobotics,bubblegum-96", "actions,s900";
@@ -35,6 +36,16 @@
 	clocks = <&cmu CLK_UART5>;
 };
 
+&i2c1 {
+	status = "okay";
+	clocks = <&cmu CLK_I2C1>;
+};
+
+&i2c2 {
+	status = "okay";
+	clocks = <&cmu CLK_I2C2>;
+};
+
 /*
  * GPIO name legend: proper name = the GPIO line is used as GPIO
  *         NC = not connected (pin out but not routed from the chip to
-- 
2.17.1

[PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver

[Manivannan Sadhasivam]

Add Actions Semiconductor Owl family S900 I2C driver.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/i2c/busses/Kconfig   |   7 +
 drivers/i2c/busses/Makefile  |   1 +
 drivers/i2c/busses/i2c-owl.c | 477 +++++++++++++++++++++++++++++++++++
 3 files changed, 485 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-owl.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 4f8df2ec87b1..8c8025f87ce4 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -762,6 +762,13 @@ config I2C_OMAP
 	  Like OMAP1510/1610/1710/5912 and OMAP242x.
 	  For details see http://www.ti.com/omap.
 
+config I2C_OWL
+	tristate "Actions Semiconductor Owl I2C Controller"
+	depends on ARCH_ACTIONS || COMPILE_TEST
+	help
+	  Say Y here if you want to use the I2C bus controller on
+	  the Actions Semiconductor Owl SoC's.
+
 config I2C_PASEMI
 	tristate "PA Semi SMBus interface"
 	depends on PPC_PASEMI && PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 5a869144a0c5..b71618f77880 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
+obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
 obj-$(CONFIG_I2C_PASEMI)	+= i2c-pasemi.o
 obj-$(CONFIG_I2C_PCA_PLATFORM)	+= i2c-pca-platform.o
 obj-$(CONFIG_I2C_PMCMSP)	+= i2c-pmcmsp.o
diff --git a/drivers/i2c/busses/i2c-owl.c b/drivers/i2c/busses/i2c-owl.c
new file mode 100644
index 000000000000..144a249bf994
--- /dev/null
+++ b/drivers/i2c/busses/i2c-owl.c
@@ -0,0 +1,477 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Actions Semiconductor Owl SoC's I2C driver
+ *
+ * Copyright (c) 2014 Actions Semi Inc.
+ * Author: David Liu <liuwei@actions-semi.com>
+ *
+ * Copyright (c) 2018 Linaro Ltd.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+/* I2C registers */
+#define OWL_I2C_REG_CTL		0x0000
+#define OWL_I2C_REG_CLKDIV	0x0004
+#define OWL_I2C_REG_STAT	0x0008
+#define OWL_I2C_REG_ADDR	0x000C
+#define OWL_I2C_REG_TXDAT	0x0010
+#define OWL_I2C_REG_RXDAT	0x0014
+#define OWL_I2C_REG_CMD		0x0018
+#define OWL_I2C_REG_FIFOCTL	0x001C
+#define OWL_I2C_REG_FIFOSTAT	0x0020
+#define OWL_I2C_REG_DATCNT	0x0024
+#define OWL_I2C_REG_RCNT	0x0028
+
+/* I2Cx_CTL Bit Mask */
+#define OWL_I2C_CTL_RB		BIT(1)
+#define OWL_I2C_CTL_GBCC(x)	(((x) & 0x3) << 2)
+#define	OWL_I2C_CTL_GBCC_NONE	OWL_I2C_CTL_GBCC(0)
+#define	OWL_I2C_CTL_GBCC_START	OWL_I2C_CTL_GBCC(1)
+#define	OWL_I2C_CTL_GBCC_STOP	OWL_I2C_CTL_GBCC(2)
+#define	OWL_I2C_CTL_GBCC_RSTART	OWL_I2C_CTL_GBCC(3)
+#define OWL_I2C_CTL_IRQE	BIT(5)
+#define OWL_I2C_CTL_EN		BIT(7)
+#define OWL_I2C_CTL_AE		BIT(8)
+#define OWL_I2C_CTL_SHSM	BIT(10)
+
+#define OWL_I2C_DIV_FACTOR(x)	((x) & 0xff)
+
+/* I2Cx_STAT Bit Mask */
+#define OWL_I2C_STAT_RACK	BIT(0)
+#define OWL_I2C_STAT_BEB	BIT(1)
+#define OWL_I2C_STAT_IRQP	BIT(2)
+#define OWL_I2C_STAT_LAB	BIT(3)
+#define OWL_I2C_STAT_STPD	BIT(4)
+#define OWL_I2C_STAT_STAD	BIT(5)
+#define OWL_I2C_STAT_BBB	BIT(6)
+#define OWL_I2C_STAT_TCB	BIT(7)
+#define OWL_I2C_STAT_LBST	BIT(8)
+#define OWL_I2C_STAT_SAMB	BIT(9)
+#define OWL_I2C_STAT_SRGC	BIT(10)
+
+/* I2Cx_CMD Bit Mask */
+#define OWL_I2C_CMD_SBE		BIT(0)
+#define OWL_I2C_CMD_RBE		BIT(4)
+#define OWL_I2C_CMD_DE		BIT(8)
+#define OWL_I2C_CMD_NS		BIT(9)
+#define OWL_I2C_CMD_SE		BIT(10)
+#define OWL_I2C_CMD_MSS		BIT(11)
+#define OWL_I2C_CMD_WRS		BIT(12)
+#define OWL_I2C_CMD_SECL	BIT(15)
+
+#define OWL_I2C_CMD_AS(x)	(((x) & 0x7) << 1)
+#define OWL_I2C_CMD_SAS(x)	(((x) & 0x7) << 5)
+
+/* I2Cx_FIFOCTL Bit Mask */
+#define OWL_I2C_FIFOCTL_NIB	BIT(0)
+#define OWL_I2C_FIFOCTL_RFR	BIT(1)
+#define OWL_I2C_FIFOCTL_TFR	BIT(2)
+
+/* I2Cc_FIFOSTAT Bit Mask */
+#define OWL_I2C_FIFOSTAT_RNB	BIT(1)
+#define OWL_I2C_FIFOSTAT_RFE	BIT(2)
+#define OWL_I2C_FIFOSTAT_TFF	BIT(5)
+#define OWL_I2C_FIFOSTAT_TFD	GENMASK(23, 16)
+#define OWL_I2C_FIFOSTAT_RFD	GENMASK(15, 8)
+
+/* I2C bus timeout */
+#define OWL_I2C_TIMEOUT		msecs_to_jiffies(4 * 1000)
+
+#define OWL_I2C_MAX_RETRIES	50
+
+#define OWL_I2C_DEF_SPEED_HZ	100000
+#define OWL_I2C_MAX_SPEED_HZ	400000
+
+struct owl_i2c_dev {
+	struct i2c_adapter	adap;
+	struct i2c_msg		*msg;
+	struct completion	msg_complete;
+	struct clk		*clk;
+	void __iomem		*base;
+	unsigned long		clk_rate;
+	u32			bus_freq;
+	u32			msg_ptr;
+};
+
+static void owl_i2c_update_reg(void __iomem *reg, unsigned int val, bool state)
+{
+	unsigned int regval;
+
+	regval = readl(reg);
+
+	if (state)
+		regval |= val;
+	else
+		regval &= ~val;
+
+	writel(regval, reg);
+}
+
+static int owl_i2c_reset(struct owl_i2c_dev *i2c_dev)
+{
+	unsigned int val, timeout = 0;
+
+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
+				OWL_I2C_CTL_EN, false);
+	mdelay(1);
+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
+				OWL_I2C_CTL_EN, true);
+
+	/* Reset FIFO */
+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_FIFOCTL,
+				OWL_I2C_FIFOCTL_RFR | OWL_I2C_FIFOCTL_TFR,
+				true);
+
+	/* Wait 50ms for FIFO reset complete */
+	do {
+		val = readl(i2c_dev->base + OWL_I2C_REG_FIFOCTL);
+		if (!(val & (OWL_I2C_FIFOCTL_RFR | OWL_I2C_FIFOCTL_TFR)))
+			break;
+		mdelay(1);
+	} while (timeout++ < OWL_I2C_MAX_RETRIES);
+
+	if (timeout > OWL_I2C_MAX_RETRIES) {
+		dev_err(&i2c_dev->adap.dev, "FIFO reset timeout");
+		return -ETIMEDOUT;
+	}
+
+	/* Clear status registers */
+	writel(0, i2c_dev->base + OWL_I2C_REG_STAT);
+
+	return 0;
+}
+
+static int owl_i2c_set_freq(struct owl_i2c_dev *i2c_dev)
+{
+	unsigned int val;
+
+	val = (i2c_dev->clk_rate + i2c_dev->bus_freq * 16 - 1) /
+				(i2c_dev->bus_freq * 16);
+
+	/* Set clock divider factor */
+	writel(OWL_I2C_DIV_FACTOR(val), i2c_dev->base + OWL_I2C_REG_CLKDIV);
+
+	return 0;
+}
+
+static int owl_i2c_hw_init(struct owl_i2c_dev *i2c_dev)
+{
+	int ret;
+
+	/* Reset I2C controller */
+	ret = owl_i2c_reset(i2c_dev);
+	if (ret)
+		return ret;
+
+	/* Set bus frequency */
+	return owl_i2c_set_freq(i2c_dev);
+}
+
+static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
+{
+	struct owl_i2c_dev *i2c_dev = _dev;
+	struct i2c_msg *msg = i2c_dev->msg;
+	unsigned int stat, fifostat;
+
+	fifostat = readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT);
+	if (fifostat & OWL_I2C_FIFOSTAT_RNB) {
+		dev_dbg(&i2c_dev->adap.dev, "received NACK from device");
+		goto stop;
+	}
+
+	stat = readl(i2c_dev->base + OWL_I2C_REG_STAT);
+	if (stat & OWL_I2C_STAT_BEB) {
+		dev_dbg(&i2c_dev->adap.dev, "bus error");
+		goto stop;
+	}
+
+	/* Handle FIFO read */
+	if (msg->flags & I2C_M_RD) {
+		while ((readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
+				OWL_I2C_FIFOSTAT_RFE) &&
+				(i2c_dev->msg_ptr < msg->len)) {
+			msg->buf[i2c_dev->msg_ptr++] = readl(i2c_dev->base +
+							OWL_I2C_REG_RXDAT);
+		}
+	} else {
+		/* Handle the remaining bytes which were not sent */
+		while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
+				OWL_I2C_FIFOSTAT_TFF) &&
+				i2c_dev->msg_ptr < msg->len) {
+			writel(msg->buf[i2c_dev->msg_ptr++], i2c_dev->base +
+							OWL_I2C_REG_TXDAT);
+		}
+	}
+
+stop:
+	/* Clear pending interrupts */
+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_STAT,
+				OWL_I2C_STAT_IRQP, true);
+
+	complete_all(&i2c_dev->msg_complete);
+
+	return IRQ_HANDLED;
+}
+
+static u32 owl_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static int owl_i2c_check_bus_busy(struct i2c_adapter *adap)
+{
+	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
+	unsigned long timeout;
+	unsigned int val;
+
+	/* Check for Arbitration lost */
+	val = readl(i2c_dev->base + OWL_I2C_REG_STAT);
+	if (val & OWL_I2C_STAT_LAB) {
+		val &= ~OWL_I2C_STAT_LAB;
+		writel(val, i2c_dev->base + OWL_I2C_REG_STAT);
+		return -EAGAIN;
+	}
+
+	/* Check for Bus busy */
+	timeout = jiffies + OWL_I2C_TIMEOUT;
+	while (readl(i2c_dev->base + OWL_I2C_REG_STAT) & OWL_I2C_STAT_BBB) {
+		if (time_after(jiffies, timeout)) {
+			dev_err(&adap->dev, "Bus busy timeout");
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
+}
+
+static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+				int num)
+{
+	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
+	struct i2c_msg *msg;
+	unsigned long time_left;
+	unsigned int i2c_cmd;
+	unsigned int addr;
+	int ret, idx;
+
+	ret = owl_i2c_hw_init(i2c_dev);
+	if (ret)
+		return ret;
+
+	ret = owl_i2c_check_bus_busy(adap);
+	if (ret)
+		return ret;
+
+	reinit_completion(&i2c_dev->msg_complete);
+
+	/* Enable I2C controller interrupt */
+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
+				OWL_I2C_CTL_IRQE, true);
+
+	/*
+	 * Select: FIFO enable, Master mode, Stop enable, Data count enable,
+	 * Send start bit
+	 */
+	i2c_cmd = (OWL_I2C_CMD_SECL | OWL_I2C_CMD_MSS | OWL_I2C_CMD_SE
+			| OWL_I2C_CMD_NS | OWL_I2C_CMD_DE | OWL_I2C_CMD_SBE);
+
+	/* Handle repeated start condition */
+	if (num > 1) {
+		/* Set internal address length and enable repeated start */
+		i2c_cmd |= (OWL_I2C_CMD_AS(msgs[0].len + 1)
+				| OWL_I2C_CMD_SAS(1) | OWL_I2C_CMD_RBE);
+
+		/* Write slave address */
+		addr = i2c_8bit_addr_from_msg(&msgs[0]);
+		writel(addr, i2c_dev->base + OWL_I2C_REG_TXDAT);
+
+		/* Write internal register address */
+		for (idx = 0; idx < msgs[0].len; idx++)
+			writel(msgs[0].buf[idx], i2c_dev->base +
+						OWL_I2C_REG_TXDAT);
+
+		msg = &msgs[1];
+	} else {
+		/* Set address length */
+		i2c_cmd |= OWL_I2C_CMD_AS(1);
+		msg = &msgs[0];
+	}
+
+	i2c_dev->msg = msg;
+	i2c_dev->msg_ptr = 0;
+
+	/* Set data count for the message */
+	writel(msg->len, i2c_dev->base + OWL_I2C_REG_DATCNT);
+
+	addr = i2c_8bit_addr_from_msg(msg);
+	writel(addr, i2c_dev->base + OWL_I2C_REG_TXDAT);
+
+	if (!(msg->flags & I2C_M_RD)) {
+		/* Write data to FIFO */
+		for (idx = 0; idx < msg->len; idx++) {
+			/* Check for FIFO full */
+			if (readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT)
+					& OWL_I2C_FIFOSTAT_TFF)
+				break;
+
+			writel(msg->buf[idx],
+					i2c_dev->base + OWL_I2C_REG_TXDAT);
+		}
+
+		i2c_dev->msg_ptr = idx;
+	}
+
+	/* Ignore the NACK if needed */
+	if (msg->flags & I2C_M_IGNORE_NAK)
+		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_FIFOCTL,
+				OWL_I2C_FIFOCTL_NIB, true);
+	else
+		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_FIFOCTL,
+				OWL_I2C_FIFOCTL_NIB, false);
+
+	/* Start the transfer */
+	writel(i2c_cmd, i2c_dev->base + OWL_I2C_REG_CMD);
+
+	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
+						adap->timeout);
+	if (time_left == 0) {
+		dev_err(&adap->dev, "Transaction timed out");
+		/* Send stop condition and release the bus */
+		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
+			OWL_I2C_CTL_GBCC_STOP | OWL_I2C_CTL_RB, true);
+		ret = -ETIMEDOUT;
+	}
+
+	/*
+	 * By default, 0 will be returned if interrupt occurred but no
+	 * read or write happened. Else if msg_ptr equals to message length,
+	 * message count will be returned.
+	 */
+	if (i2c_dev->msg_ptr == msg->len)
+		ret = num;
+
+	/* Disable I2C controller */
+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
+				OWL_I2C_CTL_EN, false);
+
+	return ret;
+}
+
+static const struct i2c_algorithm owl_i2c_algorithm = {
+	.master_xfer    = owl_i2c_master_xfer,
+	.functionality  = owl_i2c_func,
+};
+
+static const struct i2c_adapter_quirks owl_i2c_quirks = {
+	.flags		= I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST,
+	.max_read_len   = 240,
+	.max_write_len  = 240,
+	.max_comb_1st_msg_len = 6,
+	.max_comb_2nd_msg_len = 240,
+};
+
+static int owl_i2c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct owl_i2c_dev *i2c_dev;
+	struct resource *res;
+	int ret, irq;
+
+	i2c_dev = devm_kzalloc(dev, sizeof(*i2c_dev), GFP_KERNEL);
+	if (!i2c_dev)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	i2c_dev->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(i2c_dev->base))
+		return PTR_ERR(i2c_dev->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "failed to get IRQ number\n");
+		return irq;
+	}
+
+	if (of_property_read_u32(dev->of_node, "clock-frequency",
+					&i2c_dev->bus_freq))
+		i2c_dev->bus_freq = OWL_I2C_DEF_SPEED_HZ;
+
+	/* We support only frequencies of 100k and 400k for now */
+	if (i2c_dev->bus_freq != OWL_I2C_DEF_SPEED_HZ &&
+			i2c_dev->bus_freq > OWL_I2C_MAX_SPEED_HZ) {
+		dev_err(dev, "invalid clock-frequency %d\n", i2c_dev->bus_freq);
+		return -EINVAL;
+	}
+
+	i2c_dev->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(i2c_dev->clk)) {
+		dev_err(dev, "failed to get clock\n");
+		return PTR_ERR(i2c_dev->clk);
+	}
+
+	ret = clk_prepare_enable(i2c_dev->clk);
+	if (ret)
+		return ret;
+
+	i2c_dev->clk_rate = clk_get_rate(i2c_dev->clk);
+	if (!i2c_dev->clk_rate) {
+		dev_err(dev, "input clock rate should not be zero\n");
+		ret = -EINVAL;
+		goto disable_clk;
+	}
+
+	init_completion(&i2c_dev->msg_complete);
+	i2c_dev->adap.owner = THIS_MODULE;
+	i2c_dev->adap.algo = &owl_i2c_algorithm;
+	i2c_dev->adap.timeout = OWL_I2C_TIMEOUT;
+	i2c_dev->adap.quirks = &owl_i2c_quirks;
+	i2c_dev->adap.dev.parent = dev;
+	i2c_dev->adap.dev.of_node = dev->of_node;
+	snprintf(i2c_dev->adap.name, sizeof(i2c_dev->adap.name),
+			"%s", "OWL I2C adapter");
+	i2c_set_adapdata(&i2c_dev->adap, i2c_dev);
+
+	platform_set_drvdata(pdev, i2c_dev);
+
+	ret = devm_request_irq(dev, irq, owl_i2c_interrupt, 0, pdev->name,
+				i2c_dev);
+	if (ret) {
+		dev_err(dev, "failed to request irq %d\n", irq);
+		goto disable_clk;
+	}
+
+	return i2c_add_adapter(&i2c_dev->adap);
+
+disable_clk:
+	clk_disable_unprepare(i2c_dev->clk);
+
+	return ret;
+}
+
+static const struct of_device_id owl_i2c_of_match[] = {
+	{.compatible = "actions,s900-i2c"},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, owl_i2c_of_match);
+
+static struct platform_driver owl_i2c_driver = {
+	.probe		= owl_i2c_probe,
+	.driver		= {
+		.name	= "owl-i2c",
+		.of_match_table = of_match_ptr(owl_i2c_of_match),
+	},
+};
+module_platform_driver(owl_i2c_driver);
+
+MODULE_AUTHOR("David Liu <liuwei@actions-semi.com>");
+MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
+MODULE_DESCRIPTION("Actions Semiconductor Owl SoC's I2C driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

[PATCH v3 6/6] MAINTAINERS: Add entry for Actions Semiconductor Owl I2C driver

[Manivannan Sadhasivam]

Add entry for Actions Semiconductor Owl I2C driver under ARM/ACTIONS

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 09b54e9ebc6f..5084c62712fa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1145,12 +1145,14 @@ F:	arch/arm/boot/dts/owl-*
 F:	arch/arm64/boot/dts/actions/
 F:	drivers/clk/actions/
 F:	drivers/clocksource/owl-*
+F:	drivers/i2c/busses/i2c-owl.c
 F:	drivers/pinctrl/actions/*
 F:	drivers/soc/actions/
 F:	include/dt-bindings/power/owl-*
 F:	include/linux/soc/actions/
 F:	Documentation/devicetree/bindings/arm/actions.txt
 F:	Documentation/devicetree/bindings/clock/actions,s900-cmu.txt
+F:	Documentation/devicetree/bindings/i2c/i2c-owl.txt
 F:	Documentation/devicetree/bindings/pinctrl/actions,s900-pinctrl.txt
 F:	Documentation/devicetree/bindings/power/actions,owl-sps.txt
 F:	Documentation/devicetree/bindings/timer/actions,owl-timer.txt
-- 
2.17.1

Re: [PATCH v3 2/6] arm64: dts: actions: Add Actions Semiconductor S900 I2C controller nodes

[Peter Rosin]

On June 30, 2018 3:33:26 PM GMT+02:00, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
>Add I2C controller nodes for Actions Semiconductor S900 SoC.
>
>Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>---
> arch/arm64/boot/dts/actions/s900.dtsi | 60 +++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
>diff --git a/arch/arm64/boot/dts/actions/s900.dtsi
>b/arch/arm64/boot/dts/actions/s900.dtsi
>index 7ae8b931f000..6f7b89edbe4d 100644
>--- a/arch/arm64/boot/dts/actions/s900.dtsi
>+++ b/arch/arm64/boot/dts/actions/s900.dtsi
>@@ -174,6 +174,66 @@
> 			#clock-cells = <1>;
> 		};
> 
>+		i2c0: i2c@e0170000 {
>+			compatible = "actions,s900-i2c";
>+			reg = <0 0xe0170000 0 0x1000>;
>+			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
>+			#address-cells = <1>;
>+			#size-cells = <0>;
>+			status = "disabled";
>+			pinctrl-names = "default";
>+			pinctrl-0 = <&i2c0_default>;
>+		};
>+
>+		i2c1: i2c@e0172000 {
>+			compatible = "actions,s900-i2c";
>+			reg = <0 0xe0172000 0 0x1000>;
>+			interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>+			#address-cells = <1>;
>+			#size-cells = <0>;
>+			status = "disabled";
>+			pinctrl-names = "default";
>+			pinctrl-0 = <&i2c1_default>;
>+		};
>+
>+		i2c2: i2c@e0174000 {
>+			compatible = "actions,s900-i2c";
>+			reg = <0 0xe0174000 0 0x1000>;
>+			interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>+			#address-cells = <1>;
>+			#size-cells = <0>;
>+			status = "disabled";
>+			pinctrl-names = "default";
>+			pinctrl-0 = <&i2c2_default>;
>+		};
>+
>+		i2c3: i2c@e0176000 {
>+			compatible = "actions,s900-i2c";
>+			reg = <0 0xe0176000 0 0x1000>;
>+			interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
>+			#address-cells = <1>;
>+			#size-cells = <0>;
>+			status = "disabled";
>+		};
>+
>+		i2c4: i2c@e0178000 {
>+			compatible = "actions,s900-i2c";
>+			reg = <0 0xe0178000 0 0x1000>;
>+			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
>+			#address-cells = <1>;
>+			#size-cells = <0>;
>+			status = "disabled";
>+		};
>+
>+		i2c5: i2c@e017a000 {
>+			compatible = "actions,s900-i2c";
>+			reg = <0 0xe017a000 0 0x1000>;
>+			interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
>+			#address-cells = <1>;
>+			#size-cells = <0>;
>+			status = "disabled";
>+		};
>+
> 		pinctrl: pinctrl@e01b0000 {
> 			compatible = "actions,s900-pinctrl";
> 			reg = <0x0 0xe01b0000 0x0 0x1000>;

This patch *still* depends on patch 3/6. You need to reorder the series so that it is properly bisectable.

Cheers,
Peter

Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver

[Andy Shevchenko]

On Sat, Jun 30, 2018 at 4:33 PM, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
> Add Actions Semiconductor Owl family S900 I2C driver.

Thanks for an update. Few left comments and it would LGTM.

> +static int owl_i2c_reset(struct owl_i2c_dev *i2c_dev)
> +{

> +       mdelay(1);

But now, since it's not used in atomic context, we may switch to
usleep_range() / msleep() instead.

> +       owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> +                               OWL_I2C_CTL_EN, true);
> +

> +       /* Wait 50ms for FIFO reset complete */
> +       do {

> +               mdelay(1);

Especially in this case it's very important.

> +       } while (timeout++ < OWL_I2C_MAX_RETRIES);

> +}

> +       val = (i2c_dev->clk_rate + i2c_dev->bus_freq * 16 - 1) /
> +                               (i2c_dev->bus_freq * 16);

This is effectively DIV_ROUND_UP(->clk_rate, ->bus_freq * 16).

> +       /*
> +        * By default, 0 will be returned if interrupt occurred but no
> +        * read or write happened. Else if msg_ptr equals to message length,
> +        * message count will be returned.
> +        */

> +       if (i2c_dev->msg_ptr == msg->len)
> +               ret = num;

I dunno if

ret = ->msg_ptr == len ? num : 0;

would be slightly more explicit (yes, I aware about ret == 0).

Up to you to choose.

> +       /* We support only frequencies of 100k and 400k for now */
> +       if (i2c_dev->bus_freq != OWL_I2C_DEF_SPEED_HZ &&
> +                       i2c_dev->bus_freq > OWL_I2C_MAX_SPEED_HZ) {

I think it should be != in the second case as well.

> +               dev_err(dev, "invalid clock-frequency %d\n", i2c_dev->bus_freq);
> +               return -EINVAL;
> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver

[Peter Rosin]

On June 30, 2018 3:33:29 PM GMT+02:00, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
>Add Actions Semiconductor Owl family S900 I2C driver.
>
>Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>---
> drivers/i2c/busses/Kconfig   |   7 +
> drivers/i2c/busses/Makefile  |   1 +
> drivers/i2c/busses/i2c-owl.c | 477 +++++++++++++++++++++++++++++++++++
> 3 files changed, 485 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-owl.c
>
>diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>index 4f8df2ec87b1..8c8025f87ce4 100644
>--- a/drivers/i2c/busses/Kconfig
>+++ b/drivers/i2c/busses/Kconfig
>@@ -762,6 +762,13 @@ config I2C_OMAP
> 	  Like OMAP1510/1610/1710/5912 and OMAP242x.
> 	  For details see http://www.ti.com/omap.
> 
>+config I2C_OWL
>+	tristate "Actions Semiconductor Owl I2C Controller"
>+	depends on ARCH_ACTIONS || COMPILE_TEST
>+	help
>+	  Say Y here if you want to use the I2C bus controller on
>+	  the Actions Semiconductor Owl SoC's.
>+
> config I2C_PASEMI
> 	tristate "PA Semi SMBus interface"
> 	depends on PPC_PASEMI && PCI
>diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>index 5a869144a0c5..b71618f77880 100644
>--- a/drivers/i2c/busses/Makefile
>+++ b/drivers/i2c/busses/Makefile
>@@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
> obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
> obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
> obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
>+obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
> obj-$(CONFIG_I2C_PASEMI)	+= i2c-pasemi.o
> obj-$(CONFIG_I2C_PCA_PLATFORM)	+= i2c-pca-platform.o
> obj-$(CONFIG_I2C_PMCMSP)	+= i2c-pmcmsp.o
>diff --git a/drivers/i2c/busses/i2c-owl.c
>b/drivers/i2c/busses/i2c-owl.c
>new file mode 100644
>index 000000000000..144a249bf994
>--- /dev/null
>+++ b/drivers/i2c/busses/i2c-owl.c
>@@ -0,0 +1,477 @@
>+// SPDX-License-Identifier: GPL-2.0+
>+/*
>+ * Actions Semiconductor Owl SoC's I2C driver
>+ *
>+ * Copyright (c) 2014 Actions Semi Inc.
>+ * Author: David Liu <liuwei@actions-semi.com>
>+ *
>+ * Copyright (c) 2018 Linaro Ltd.
>+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>+ */
>+
>+#include <linux/clk.h>
>+#include <linux/delay.h>
>+#include <linux/i2c.h>
>+#include <linux/interrupt.h>
>+#include <linux/io.h>
>+#include <linux/module.h>
>+#include <linux/of_device.h>
>+
>+/* I2C registers */
>+#define OWL_I2C_REG_CTL		0x0000
>+#define OWL_I2C_REG_CLKDIV	0x0004
>+#define OWL_I2C_REG_STAT	0x0008
>+#define OWL_I2C_REG_ADDR	0x000C
>+#define OWL_I2C_REG_TXDAT	0x0010
>+#define OWL_I2C_REG_RXDAT	0x0014
>+#define OWL_I2C_REG_CMD		0x0018
>+#define OWL_I2C_REG_FIFOCTL	0x001C
>+#define OWL_I2C_REG_FIFOSTAT	0x0020
>+#define OWL_I2C_REG_DATCNT	0x0024
>+#define OWL_I2C_REG_RCNT	0x0028
>+
>+/* I2Cx_CTL Bit Mask */
>+#define OWL_I2C_CTL_RB		BIT(1)
>+#define OWL_I2C_CTL_GBCC(x)	(((x) & 0x3) << 2)
>+#define	OWL_I2C_CTL_GBCC_NONE	OWL_I2C_CTL_GBCC(0)
>+#define	OWL_I2C_CTL_GBCC_START	OWL_I2C_CTL_GBCC(1)
>+#define	OWL_I2C_CTL_GBCC_STOP	OWL_I2C_CTL_GBCC(2)
>+#define	OWL_I2C_CTL_GBCC_RSTART	OWL_I2C_CTL_GBCC(3)
>+#define OWL_I2C_CTL_IRQE	BIT(5)
>+#define OWL_I2C_CTL_EN		BIT(7)
>+#define OWL_I2C_CTL_AE		BIT(8)
>+#define OWL_I2C_CTL_SHSM	BIT(10)
>+
>+#define OWL_I2C_DIV_FACTOR(x)	((x) & 0xff)
>+
>+/* I2Cx_STAT Bit Mask */
>+#define OWL_I2C_STAT_RACK	BIT(0)
>+#define OWL_I2C_STAT_BEB	BIT(1)
>+#define OWL_I2C_STAT_IRQP	BIT(2)
>+#define OWL_I2C_STAT_LAB	BIT(3)
>+#define OWL_I2C_STAT_STPD	BIT(4)
>+#define OWL_I2C_STAT_STAD	BIT(5)
>+#define OWL_I2C_STAT_BBB	BIT(6)
>+#define OWL_I2C_STAT_TCB	BIT(7)
>+#define OWL_I2C_STAT_LBST	BIT(8)
>+#define OWL_I2C_STAT_SAMB	BIT(9)
>+#define OWL_I2C_STAT_SRGC	BIT(10)
>+
>+/* I2Cx_CMD Bit Mask */
>+#define OWL_I2C_CMD_SBE		BIT(0)
>+#define OWL_I2C_CMD_RBE		BIT(4)
>+#define OWL_I2C_CMD_DE		BIT(8)
>+#define OWL_I2C_CMD_NS		BIT(9)
>+#define OWL_I2C_CMD_SE		BIT(10)
>+#define OWL_I2C_CMD_MSS		BIT(11)
>+#define OWL_I2C_CMD_WRS		BIT(12)
>+#define OWL_I2C_CMD_SECL	BIT(15)
>+
>+#define OWL_I2C_CMD_AS(x)	(((x) & 0x7) << 1)
>+#define OWL_I2C_CMD_SAS(x)	(((x) & 0x7) << 5)
>+
>+/* I2Cx_FIFOCTL Bit Mask */
>+#define OWL_I2C_FIFOCTL_NIB	BIT(0)
>+#define OWL_I2C_FIFOCTL_RFR	BIT(1)
>+#define OWL_I2C_FIFOCTL_TFR	BIT(2)
>+
>+/* I2Cc_FIFOSTAT Bit Mask */
>+#define OWL_I2C_FIFOSTAT_RNB	BIT(1)
>+#define OWL_I2C_FIFOSTAT_RFE	BIT(2)
>+#define OWL_I2C_FIFOSTAT_TFF	BIT(5)
>+#define OWL_I2C_FIFOSTAT_TFD	GENMASK(23, 16)
>+#define OWL_I2C_FIFOSTAT_RFD	GENMASK(15, 8)
>+
>+/* I2C bus timeout */
>+#define OWL_I2C_TIMEOUT		msecs_to_jiffies(4 * 1000)
>+
>+#define OWL_I2C_MAX_RETRIES	50
>+
>+#define OWL_I2C_DEF_SPEED_HZ	100000
>+#define OWL_I2C_MAX_SPEED_HZ	400000
>+
>+struct owl_i2c_dev {
>+	struct i2c_adapter	adap;
>+	struct i2c_msg		*msg;
>+	struct completion	msg_complete;
>+	struct clk		*clk;
>+	void __iomem		*base;
>+	unsigned long		clk_rate;
>+	u32			bus_freq;
>+	u32			msg_ptr;
>+};
>+
>+static void owl_i2c_update_reg(void __iomem *reg, unsigned int val,
>bool state)
>+{
>+	unsigned int regval;
>+
>+	regval = readl(reg);
>+
>+	if (state)
>+		regval |= val;
>+	else
>+		regval &= ~val;
>+
>+	writel(regval, reg);
>+}
>+
>+static int owl_i2c_reset(struct owl_i2c_dev *i2c_dev)
>+{
>+	unsigned int val, timeout = 0;
>+
>+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
>+				OWL_I2C_CTL_EN, false);
>+	mdelay(1);
>+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
>+				OWL_I2C_CTL_EN, true);
>+
>+	/* Reset FIFO */
>+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_FIFOCTL,
>+				OWL_I2C_FIFOCTL_RFR | OWL_I2C_FIFOCTL_TFR,
>+				true);
>+
>+	/* Wait 50ms for FIFO reset complete */
>+	do {
>+		val = readl(i2c_dev->base + OWL_I2C_REG_FIFOCTL);
>+		if (!(val & (OWL_I2C_FIFOCTL_RFR | OWL_I2C_FIFOCTL_TFR)))
>+			break;
>+		mdelay(1);
>+	} while (timeout++ < OWL_I2C_MAX_RETRIES);
>+
>+	if (timeout > OWL_I2C_MAX_RETRIES) {
>+		dev_err(&i2c_dev->adap.dev, "FIFO reset timeout");
>+		return -ETIMEDOUT;
>+	}
>+
>+	/* Clear status registers */
>+	writel(0, i2c_dev->base + OWL_I2C_REG_STAT);
>+
>+	return 0;
>+}
>+
>+static int owl_i2c_set_freq(struct owl_i2c_dev *i2c_dev)
>+{
>+	unsigned int val;
>+
>+	val = (i2c_dev->clk_rate + i2c_dev->bus_freq * 16 - 1) /
>+				(i2c_dev->bus_freq * 16);
>+
>+	/* Set clock divider factor */
>+	writel(OWL_I2C_DIV_FACTOR(val), i2c_dev->base + OWL_I2C_REG_CLKDIV);
>+
>+	return 0;
>+}
>+
>+static int owl_i2c_hw_init(struct owl_i2c_dev *i2c_dev)
>+{
>+	int ret;
>+
>+	/* Reset I2C controller */
>+	ret = owl_i2c_reset(i2c_dev);
>+	if (ret)
>+		return ret;
>+
>+	/* Set bus frequency */
>+	return owl_i2c_set_freq(i2c_dev);
>+}
>+
>+static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
>+{
>+	struct owl_i2c_dev *i2c_dev = _dev;
>+	struct i2c_msg *msg = i2c_dev->msg;
>+	unsigned int stat, fifostat;
>+
>+	fifostat = readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT);
>+	if (fifostat & OWL_I2C_FIFOSTAT_RNB) {
>+		dev_dbg(&i2c_dev->adap.dev, "received NACK from device");
>+		goto stop;
>+	}
>+
>+	stat = readl(i2c_dev->base + OWL_I2C_REG_STAT);
>+	if (stat & OWL_I2C_STAT_BEB) {
>+		dev_dbg(&i2c_dev->adap.dev, "bus error");
>+		goto stop;
>+	}
>+
>+	/* Handle FIFO read */
>+	if (msg->flags & I2C_M_RD) {
>+		while ((readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
>+				OWL_I2C_FIFOSTAT_RFE) &&
>+				(i2c_dev->msg_ptr < msg->len)) {
>+			msg->buf[i2c_dev->msg_ptr++] = readl(i2c_dev->base +
>+							OWL_I2C_REG_RXDAT);
>+		}
>+	} else {
>+		/* Handle the remaining bytes which were not sent */
>+		while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
>+				OWL_I2C_FIFOSTAT_TFF) &&
>+				i2c_dev->msg_ptr < msg->len) {
>+			writel(msg->buf[i2c_dev->msg_ptr++], i2c_dev->base +
>+							OWL_I2C_REG_TXDAT);
>+		}
>+	}
>+
>+stop:
>+	/* Clear pending interrupts */
>+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_STAT,
>+				OWL_I2C_STAT_IRQP, true);
>+

Here is a read-modify-write on ..._REG_STAT from interrupt context...

>+	complete_all(&i2c_dev->msg_complete);
>+
>+	return IRQ_HANDLED;
>+}
>+
>+static u32 owl_i2c_func(struct i2c_adapter *adap)
>+{
>+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>+}
>+
>+static int owl_i2c_check_bus_busy(struct i2c_adapter *adap)
>+{
>+	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
>+	unsigned long timeout;
>+	unsigned int val;
>+
>+	/* Check for Arbitration lost */
>+	val = readl(i2c_dev->base + OWL_I2C_REG_STAT);
>+	if (val & OWL_I2C_STAT_LAB) {
>+		val &= ~OWL_I2C_STAT_LAB;
>+		writel(val, i2c_dev->base + OWL_I2C_REG_STAT);

...and here is one from process context.

Can they cross? If not, why? Either use some locking, or add a comment explaining why this is safe.

>+		return -EAGAIN;
>+	}
>+
>+	/* Check for Bus busy */
>+	timeout = jiffies + OWL_I2C_TIMEOUT;
>+	while (readl(i2c_dev->base + OWL_I2C_REG_STAT) & OWL_I2C_STAT_BBB) {
>+		if (time_after(jiffies, timeout)) {
>+			dev_err(&adap->dev, "Bus busy timeout");
>+			return -ETIMEDOUT;
>+		}
>+	}
>+
>+	return 0;
>+}
>+
>+static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct
>i2c_msg *msgs,
>+				int num)
>+{
>+	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
>+	struct i2c_msg *msg;
>+	unsigned long time_left;
>+	unsigned int i2c_cmd;
>+	unsigned int addr;
>+	int ret, idx;
>+
>+	ret = owl_i2c_hw_init(i2c_dev);
>+	if (ret)
>+		return ret;
>+
>+	ret = owl_i2c_check_bus_busy(adap);
>+	if (ret)
>+		return ret;
>+
>+	reinit_completion(&i2c_dev->msg_complete);
>+
>+	/* Enable I2C controller interrupt */
>+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
>+				OWL_I2C_CTL_IRQE, true);
>+
>+	/*
>+	 * Select: FIFO enable, Master mode, Stop enable, Data count enable,
>+	 * Send start bit
>+	 */
>+	i2c_cmd = (OWL_I2C_CMD_SECL | OWL_I2C_CMD_MSS | OWL_I2C_CMD_SE
>+			| OWL_I2C_CMD_NS | OWL_I2C_CMD_DE | OWL_I2C_CMD_SBE);
>+
>+	/* Handle repeated start condition */
>+	if (num > 1) {
>+		/* Set internal address length and enable repeated start */
>+		i2c_cmd |= (OWL_I2C_CMD_AS(msgs[0].len + 1)
>+				| OWL_I2C_CMD_SAS(1) | OWL_I2C_CMD_RBE);
>+
>+		/* Write slave address */
>+		addr = i2c_8bit_addr_from_msg(&msgs[0]);
>+		writel(addr, i2c_dev->base + OWL_I2C_REG_TXDAT);
>+
>+		/* Write internal register address */
>+		for (idx = 0; idx < msgs[0].len; idx++)
>+			writel(msgs[0].buf[idx], i2c_dev->base +
>+						OWL_I2C_REG_TXDAT);
>+
>+		msg = &msgs[1];
>+	} else {
>+		/* Set address length */
>+		i2c_cmd |= OWL_I2C_CMD_AS(1);
>+		msg = &msgs[0];
>+	}
>+
>+	i2c_dev->msg = msg;
>+	i2c_dev->msg_ptr = 0;
>+
>+	/* Set data count for the message */
>+	writel(msg->len, i2c_dev->base + OWL_I2C_REG_DATCNT);
>+
>+	addr = i2c_8bit_addr_from_msg(msg);
>+	writel(addr, i2c_dev->base + OWL_I2C_REG_TXDAT);
>+
>+	if (!(msg->flags & I2C_M_RD)) {
>+		/* Write data to FIFO */
>+		for (idx = 0; idx < msg->len; idx++) {
>+			/* Check for FIFO full */
>+			if (readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT)
>+					& OWL_I2C_FIFOSTAT_TFF)
>+				break;
>+
>+			writel(msg->buf[idx],
>+					i2c_dev->base + OWL_I2C_REG_TXDAT);
>+		}
>+
>+		i2c_dev->msg_ptr = idx;
>+	}
>+
>+	/* Ignore the NACK if needed */
>+	if (msg->flags & I2C_M_IGNORE_NAK)
>+		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_FIFOCTL,
>+				OWL_I2C_FIFOCTL_NIB, true);
>+	else
>+		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_FIFOCTL,
>+				OWL_I2C_FIFOCTL_NIB, false);
>+
>+	/* Start the transfer */
>+	writel(i2c_cmd, i2c_dev->base + OWL_I2C_REG_CMD);
>+
>+	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
>+						adap->timeout);
>+	if (time_left == 0) {
>+		dev_err(&adap->dev, "Transaction timed out");
>+		/* Send stop condition and release the bus */
>+		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
>+			OWL_I2C_CTL_GBCC_STOP | OWL_I2C_CTL_RB, true);
>+		ret = -ETIMEDOUT;
>+	}
>+
>+	/*
>+	 * By default, 0 will be returned if interrupt occurred but no
>+	 * read or write happened. Else if msg_ptr equals to message length,
>+	 * message count will be returned.
>+	 */

Zero is never a valid return value for this function. It has to be either num or a negative error. So, this comment is bad, which makes me assume the code is too. I didn't bother to check any further...

Cheers,
Peter

>+	if (i2c_dev->msg_ptr == msg->len)
>+		ret = num;
>+
>+	/* Disable I2C controller */
>+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
>+				OWL_I2C_CTL_EN, false);
>+
>+	return ret;
>+}
>+
>+static const struct i2c_algorithm owl_i2c_algorithm = {
>+	.master_xfer    = owl_i2c_master_xfer,
>+	.functionality  = owl_i2c_func,
>+};
>+
>+static const struct i2c_adapter_quirks owl_i2c_quirks = {
>+	.flags		= I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST,
>+	.max_read_len   = 240,
>+	.max_write_len  = 240,
>+	.max_comb_1st_msg_len = 6,
>+	.max_comb_2nd_msg_len = 240,
>+};
>+
>+static int owl_i2c_probe(struct platform_device *pdev)
>+{
>+	struct device *dev = &pdev->dev;
>+	struct owl_i2c_dev *i2c_dev;
>+	struct resource *res;
>+	int ret, irq;
>+
>+	i2c_dev = devm_kzalloc(dev, sizeof(*i2c_dev), GFP_KERNEL);
>+	if (!i2c_dev)
>+		return -ENOMEM;
>+
>+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>+	i2c_dev->base = devm_ioremap_resource(dev, res);
>+	if (IS_ERR(i2c_dev->base))
>+		return PTR_ERR(i2c_dev->base);
>+
>+	irq = platform_get_irq(pdev, 0);
>+	if (irq < 0) {
>+		dev_err(dev, "failed to get IRQ number\n");
>+		return irq;
>+	}
>+
>+	if (of_property_read_u32(dev->of_node, "clock-frequency",
>+					&i2c_dev->bus_freq))
>+		i2c_dev->bus_freq = OWL_I2C_DEF_SPEED_HZ;
>+
>+	/* We support only frequencies of 100k and 400k for now */
>+	if (i2c_dev->bus_freq != OWL_I2C_DEF_SPEED_HZ &&
>+			i2c_dev->bus_freq > OWL_I2C_MAX_SPEED_HZ) {
>+		dev_err(dev, "invalid clock-frequency %d\n", i2c_dev->bus_freq);
>+		return -EINVAL;
>+	}
>+
>+	i2c_dev->clk = devm_clk_get(dev, NULL);
>+	if (IS_ERR(i2c_dev->clk)) {
>+		dev_err(dev, "failed to get clock\n");
>+		return PTR_ERR(i2c_dev->clk);
>+	}
>+
>+	ret = clk_prepare_enable(i2c_dev->clk);
>+	if (ret)
>+		return ret;
>+
>+	i2c_dev->clk_rate = clk_get_rate(i2c_dev->clk);
>+	if (!i2c_dev->clk_rate) {
>+		dev_err(dev, "input clock rate should not be zero\n");
>+		ret = -EINVAL;
>+		goto disable_clk;
>+	}
>+
>+	init_completion(&i2c_dev->msg_complete);
>+	i2c_dev->adap.owner = THIS_MODULE;
>+	i2c_dev->adap.algo = &owl_i2c_algorithm;
>+	i2c_dev->adap.timeout = OWL_I2C_TIMEOUT;
>+	i2c_dev->adap.quirks = &owl_i2c_quirks;
>+	i2c_dev->adap.dev.parent = dev;
>+	i2c_dev->adap.dev.of_node = dev->of_node;
>+	snprintf(i2c_dev->adap.name, sizeof(i2c_dev->adap.name),
>+			"%s", "OWL I2C adapter");
>+	i2c_set_adapdata(&i2c_dev->adap, i2c_dev);
>+
>+	platform_set_drvdata(pdev, i2c_dev);
>+
>+	ret = devm_request_irq(dev, irq, owl_i2c_interrupt, 0, pdev->name,
>+				i2c_dev);
>+	if (ret) {
>+		dev_err(dev, "failed to request irq %d\n", irq);
>+		goto disable_clk;
>+	}
>+
>+	return i2c_add_adapter(&i2c_dev->adap);
>+
>+disable_clk:
>+	clk_disable_unprepare(i2c_dev->clk);
>+
>+	return ret;
>+}
>+
>+static const struct of_device_id owl_i2c_of_match[] = {
>+	{.compatible = "actions,s900-i2c"},
>+	{ /* sentinel */ }
>+};
>+MODULE_DEVICE_TABLE(of, owl_i2c_of_match);
>+
>+static struct platform_driver owl_i2c_driver = {
>+	.probe		= owl_i2c_probe,
>+	.driver		= {
>+		.name	= "owl-i2c",
>+		.of_match_table = of_match_ptr(owl_i2c_of_match),
>+	},
>+};
>+module_platform_driver(owl_i2c_driver);
>+
>+MODULE_AUTHOR("David Liu <liuwei@actions-semi.com>");
>+MODULE_AUTHOR("Manivannan Sadhasivam
><manivannan.sadhasivam@linaro.org>");
>+MODULE_DESCRIPTION("Actions Semiconductor Owl SoC's I2C driver");
>+MODULE_LICENSE("GPL");

Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver

[Manivannan Sadhasivam]

Hi Peter,

On Sun, Jul 01, 2018 at 06:57:54AM +0200, Peter Rosin wrote:
> On June 30, 2018 3:33:29 PM GMT+02:00, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> >Add Actions Semiconductor Owl family S900 I2C driver.
> >
> >Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >---
> > drivers/i2c/busses/Kconfig   |   7 +
> > drivers/i2c/busses/Makefile  |   1 +
> > drivers/i2c/busses/i2c-owl.c | 477 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 485 insertions(+)
> > create mode 100644 drivers/i2c/busses/i2c-owl.c
> >
> >diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> >index 4f8df2ec87b1..8c8025f87ce4 100644
> >--- a/drivers/i2c/busses/Kconfig
> >+++ b/drivers/i2c/busses/Kconfig
> >@@ -762,6 +762,13 @@ config I2C_OMAP
> > 	  Like OMAP1510/1610/1710/5912 and OMAP242x.
> > 	  For details see http://www.ti.com/omap.
> > 
> >+config I2C_OWL
> >+	tristate "Actions Semiconductor Owl I2C Controller"
> >+	depends on ARCH_ACTIONS || COMPILE_TEST
> >+	help
> >+	  Say Y here if you want to use the I2C bus controller on
> >+	  the Actions Semiconductor Owl SoC's.
> >+
> > config I2C_PASEMI
> > 	tristate "PA Semi SMBus interface"
> > 	depends on PPC_PASEMI && PCI
> >diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> >index 5a869144a0c5..b71618f77880 100644
> >--- a/drivers/i2c/busses/Makefile
> >+++ b/drivers/i2c/busses/Makefile
> >@@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
> > obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
> > obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
> > obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
> >+obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
> > obj-$(CONFIG_I2C_PASEMI)	+= i2c-pasemi.o
> > obj-$(CONFIG_I2C_PCA_PLATFORM)	+= i2c-pca-platform.o
> > obj-$(CONFIG_I2C_PMCMSP)	+= i2c-pmcmsp.o
> >diff --git a/drivers/i2c/busses/i2c-owl.c
> >b/drivers/i2c/busses/i2c-owl.c
> >new file mode 100644
> >index 000000000000..144a249bf994
> >--- /dev/null
> >+++ b/drivers/i2c/busses/i2c-owl.c
> >@@ -0,0 +1,477 @@
> >+// SPDX-License-Identifier: GPL-2.0+
> >+/*
> >+ * Actions Semiconductor Owl SoC's I2C driver
> >+ *
> >+ * Copyright (c) 2014 Actions Semi Inc.
> >+ * Author: David Liu <liuwei@actions-semi.com>
> >+ *
> >+ * Copyright (c) 2018 Linaro Ltd.
> >+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >+ */
> >+
> >+#include <linux/clk.h>
> >+#include <linux/delay.h>
> >+#include <linux/i2c.h>
> >+#include <linux/interrupt.h>
> >+#include <linux/io.h>
> >+#include <linux/module.h>
> >+#include <linux/of_device.h>
> >+
> >+/* I2C registers */
> >+#define OWL_I2C_REG_CTL		0x0000
> >+#define OWL_I2C_REG_CLKDIV	0x0004
> >+#define OWL_I2C_REG_STAT	0x0008
> >+#define OWL_I2C_REG_ADDR	0x000C
> >+#define OWL_I2C_REG_TXDAT	0x0010
> >+#define OWL_I2C_REG_RXDAT	0x0014
> >+#define OWL_I2C_REG_CMD		0x0018
> >+#define OWL_I2C_REG_FIFOCTL	0x001C
> >+#define OWL_I2C_REG_FIFOSTAT	0x0020
> >+#define OWL_I2C_REG_DATCNT	0x0024
> >+#define OWL_I2C_REG_RCNT	0x0028
> >+
> >+/* I2Cx_CTL Bit Mask */
> >+#define OWL_I2C_CTL_RB		BIT(1)
> >+#define OWL_I2C_CTL_GBCC(x)	(((x) & 0x3) << 2)
> >+#define	OWL_I2C_CTL_GBCC_NONE	OWL_I2C_CTL_GBCC(0)
> >+#define	OWL_I2C_CTL_GBCC_START	OWL_I2C_CTL_GBCC(1)
> >+#define	OWL_I2C_CTL_GBCC_STOP	OWL_I2C_CTL_GBCC(2)
> >+#define	OWL_I2C_CTL_GBCC_RSTART	OWL_I2C_CTL_GBCC(3)
> >+#define OWL_I2C_CTL_IRQE	BIT(5)
> >+#define OWL_I2C_CTL_EN		BIT(7)
> >+#define OWL_I2C_CTL_AE		BIT(8)
> >+#define OWL_I2C_CTL_SHSM	BIT(10)
> >+
> >+#define OWL_I2C_DIV_FACTOR(x)	((x) & 0xff)
> >+
> >+/* I2Cx_STAT Bit Mask */
> >+#define OWL_I2C_STAT_RACK	BIT(0)
> >+#define OWL_I2C_STAT_BEB	BIT(1)
> >+#define OWL_I2C_STAT_IRQP	BIT(2)
> >+#define OWL_I2C_STAT_LAB	BIT(3)
> >+#define OWL_I2C_STAT_STPD	BIT(4)
> >+#define OWL_I2C_STAT_STAD	BIT(5)
> >+#define OWL_I2C_STAT_BBB	BIT(6)
> >+#define OWL_I2C_STAT_TCB	BIT(7)
> >+#define OWL_I2C_STAT_LBST	BIT(8)
> >+#define OWL_I2C_STAT_SAMB	BIT(9)
> >+#define OWL_I2C_STAT_SRGC	BIT(10)
> >+
> >+/* I2Cx_CMD Bit Mask */
> >+#define OWL_I2C_CMD_SBE		BIT(0)
> >+#define OWL_I2C_CMD_RBE		BIT(4)
> >+#define OWL_I2C_CMD_DE		BIT(8)
> >+#define OWL_I2C_CMD_NS		BIT(9)
> >+#define OWL_I2C_CMD_SE		BIT(10)
> >+#define OWL_I2C_CMD_MSS		BIT(11)
> >+#define OWL_I2C_CMD_WRS		BIT(12)
> >+#define OWL_I2C_CMD_SECL	BIT(15)
> >+
> >+#define OWL_I2C_CMD_AS(x)	(((x) & 0x7) << 1)
> >+#define OWL_I2C_CMD_SAS(x)	(((x) & 0x7) << 5)
> >+
> >+/* I2Cx_FIFOCTL Bit Mask */
> >+#define OWL_I2C_FIFOCTL_NIB	BIT(0)
> >+#define OWL_I2C_FIFOCTL_RFR	BIT(1)
> >+#define OWL_I2C_FIFOCTL_TFR	BIT(2)
> >+
> >+/* I2Cc_FIFOSTAT Bit Mask */
> >+#define OWL_I2C_FIFOSTAT_RNB	BIT(1)
> >+#define OWL_I2C_FIFOSTAT_RFE	BIT(2)
> >+#define OWL_I2C_FIFOSTAT_TFF	BIT(5)
> >+#define OWL_I2C_FIFOSTAT_TFD	GENMASK(23, 16)
> >+#define OWL_I2C_FIFOSTAT_RFD	GENMASK(15, 8)
> >+
> >+/* I2C bus timeout */
> >+#define OWL_I2C_TIMEOUT		msecs_to_jiffies(4 * 1000)
> >+
> >+#define OWL_I2C_MAX_RETRIES	50
> >+
> >+#define OWL_I2C_DEF_SPEED_HZ	100000
> >+#define OWL_I2C_MAX_SPEED_HZ	400000
> >+
> >+struct owl_i2c_dev {
> >+	struct i2c_adapter	adap;
> >+	struct i2c_msg		*msg;
> >+	struct completion	msg_complete;
> >+	struct clk		*clk;
> >+	void __iomem		*base;
> >+	unsigned long		clk_rate;
> >+	u32			bus_freq;
> >+	u32			msg_ptr;
> >+};
> >+
> >+static void owl_i2c_update_reg(void __iomem *reg, unsigned int val,
> >bool state)
> >+{
> >+	unsigned int regval;
> >+
> >+	regval = readl(reg);
> >+
> >+	if (state)
> >+		regval |= val;
> >+	else
> >+		regval &= ~val;
> >+
> >+	writel(regval, reg);
> >+}
> >+
> >+static int owl_i2c_reset(struct owl_i2c_dev *i2c_dev)
> >+{
> >+	unsigned int val, timeout = 0;
> >+
> >+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> >+				OWL_I2C_CTL_EN, false);
> >+	mdelay(1);
> >+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> >+				OWL_I2C_CTL_EN, true);
> >+
> >+	/* Reset FIFO */
> >+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_FIFOCTL,
> >+				OWL_I2C_FIFOCTL_RFR | OWL_I2C_FIFOCTL_TFR,
> >+				true);
> >+
> >+	/* Wait 50ms for FIFO reset complete */
> >+	do {
> >+		val = readl(i2c_dev->base + OWL_I2C_REG_FIFOCTL);
> >+		if (!(val & (OWL_I2C_FIFOCTL_RFR | OWL_I2C_FIFOCTL_TFR)))
> >+			break;
> >+		mdelay(1);
> >+	} while (timeout++ < OWL_I2C_MAX_RETRIES);
> >+
> >+	if (timeout > OWL_I2C_MAX_RETRIES) {
> >+		dev_err(&i2c_dev->adap.dev, "FIFO reset timeout");
> >+		return -ETIMEDOUT;
> >+	}
> >+
> >+	/* Clear status registers */
> >+	writel(0, i2c_dev->base + OWL_I2C_REG_STAT);
> >+
> >+	return 0;
> >+}
> >+
> >+static int owl_i2c_set_freq(struct owl_i2c_dev *i2c_dev)
> >+{
> >+	unsigned int val;
> >+
> >+	val = (i2c_dev->clk_rate + i2c_dev->bus_freq * 16 - 1) /
> >+				(i2c_dev->bus_freq * 16);
> >+
> >+	/* Set clock divider factor */
> >+	writel(OWL_I2C_DIV_FACTOR(val), i2c_dev->base + OWL_I2C_REG_CLKDIV);
> >+
> >+	return 0;
> >+}
> >+
> >+static int owl_i2c_hw_init(struct owl_i2c_dev *i2c_dev)
> >+{
> >+	int ret;
> >+
> >+	/* Reset I2C controller */
> >+	ret = owl_i2c_reset(i2c_dev);
> >+	if (ret)
> >+		return ret;
> >+
> >+	/* Set bus frequency */
> >+	return owl_i2c_set_freq(i2c_dev);
> >+}
> >+
> >+static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> >+{
> >+	struct owl_i2c_dev *i2c_dev = _dev;
> >+	struct i2c_msg *msg = i2c_dev->msg;
> >+	unsigned int stat, fifostat;
> >+
> >+	fifostat = readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT);
> >+	if (fifostat & OWL_I2C_FIFOSTAT_RNB) {
> >+		dev_dbg(&i2c_dev->adap.dev, "received NACK from device");
> >+		goto stop;
> >+	}
> >+
> >+	stat = readl(i2c_dev->base + OWL_I2C_REG_STAT);
> >+	if (stat & OWL_I2C_STAT_BEB) {
> >+		dev_dbg(&i2c_dev->adap.dev, "bus error");
> >+		goto stop;
> >+	}
> >+
> >+	/* Handle FIFO read */
> >+	if (msg->flags & I2C_M_RD) {
> >+		while ((readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> >+				OWL_I2C_FIFOSTAT_RFE) &&
> >+				(i2c_dev->msg_ptr < msg->len)) {
> >+			msg->buf[i2c_dev->msg_ptr++] = readl(i2c_dev->base +
> >+							OWL_I2C_REG_RXDAT);
> >+		}
> >+	} else {
> >+		/* Handle the remaining bytes which were not sent */
> >+		while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> >+				OWL_I2C_FIFOSTAT_TFF) &&
> >+				i2c_dev->msg_ptr < msg->len) {
> >+			writel(msg->buf[i2c_dev->msg_ptr++], i2c_dev->base +
> >+							OWL_I2C_REG_TXDAT);
> >+		}
> >+	}
> >+
> >+stop:
> >+	/* Clear pending interrupts */
> >+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_STAT,
> >+				OWL_I2C_STAT_IRQP, true);
> >+
> 
> Here is a read-modify-write on ..._REG_STAT from interrupt context...
> 

Will use spinlock for the entire handler.

> >+	complete_all(&i2c_dev->msg_complete);
> >+
> >+	return IRQ_HANDLED;
> >+}
> >+
> >+static u32 owl_i2c_func(struct i2c_adapter *adap)
> >+{
> >+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> >+}
> >+
> >+static int owl_i2c_check_bus_busy(struct i2c_adapter *adap)
> >+{
> >+	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> >+	unsigned long timeout;
> >+	unsigned int val;
> >+
> >+	/* Check for Arbitration lost */
> >+	val = readl(i2c_dev->base + OWL_I2C_REG_STAT);
> >+	if (val & OWL_I2C_STAT_LAB) {
> >+		val &= ~OWL_I2C_STAT_LAB;
> >+		writel(val, i2c_dev->base + OWL_I2C_REG_STAT);
> 
> ...and here is one from process context.
> 
> Can they cross? If not, why? Either use some locking, or add a comment explaining why this is safe.
> 

As said above, this function will be protected by spinlock.

> >+		return -EAGAIN;
> >+	}
> >+
> >+	/* Check for Bus busy */
> >+	timeout = jiffies + OWL_I2C_TIMEOUT;
> >+	while (readl(i2c_dev->base + OWL_I2C_REG_STAT) & OWL_I2C_STAT_BBB) {
> >+		if (time_after(jiffies, timeout)) {
> >+			dev_err(&adap->dev, "Bus busy timeout");
> >+			return -ETIMEDOUT;
> >+		}
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> >+static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct
> >i2c_msg *msgs,
> >+				int num)
> >+{
> >+	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> >+	struct i2c_msg *msg;
> >+	unsigned long time_left;
> >+	unsigned int i2c_cmd;
> >+	unsigned int addr;
> >+	int ret, idx;
> >+
> >+	ret = owl_i2c_hw_init(i2c_dev);
> >+	if (ret)
> >+		return ret;
> >+
> >+	ret = owl_i2c_check_bus_busy(adap);
> >+	if (ret)
> >+		return ret;
> >+
> >+	reinit_completion(&i2c_dev->msg_complete);
> >+
> >+	/* Enable I2C controller interrupt */
> >+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> >+				OWL_I2C_CTL_IRQE, true);
> >+
> >+	/*
> >+	 * Select: FIFO enable, Master mode, Stop enable, Data count enable,
> >+	 * Send start bit
> >+	 */
> >+	i2c_cmd = (OWL_I2C_CMD_SECL | OWL_I2C_CMD_MSS | OWL_I2C_CMD_SE
> >+			| OWL_I2C_CMD_NS | OWL_I2C_CMD_DE | OWL_I2C_CMD_SBE);
> >+
> >+	/* Handle repeated start condition */
> >+	if (num > 1) {
> >+		/* Set internal address length and enable repeated start */
> >+		i2c_cmd |= (OWL_I2C_CMD_AS(msgs[0].len + 1)
> >+				| OWL_I2C_CMD_SAS(1) | OWL_I2C_CMD_RBE);
> >+
> >+		/* Write slave address */
> >+		addr = i2c_8bit_addr_from_msg(&msgs[0]);
> >+		writel(addr, i2c_dev->base + OWL_I2C_REG_TXDAT);
> >+
> >+		/* Write internal register address */
> >+		for (idx = 0; idx < msgs[0].len; idx++)
> >+			writel(msgs[0].buf[idx], i2c_dev->base +
> >+						OWL_I2C_REG_TXDAT);
> >+
> >+		msg = &msgs[1];
> >+	} else {
> >+		/* Set address length */
> >+		i2c_cmd |= OWL_I2C_CMD_AS(1);
> >+		msg = &msgs[0];
> >+	}
> >+
> >+	i2c_dev->msg = msg;
> >+	i2c_dev->msg_ptr = 0;
> >+
> >+	/* Set data count for the message */
> >+	writel(msg->len, i2c_dev->base + OWL_I2C_REG_DATCNT);
> >+
> >+	addr = i2c_8bit_addr_from_msg(msg);
> >+	writel(addr, i2c_dev->base + OWL_I2C_REG_TXDAT);
> >+
> >+	if (!(msg->flags & I2C_M_RD)) {
> >+		/* Write data to FIFO */
> >+		for (idx = 0; idx < msg->len; idx++) {
> >+			/* Check for FIFO full */
> >+			if (readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT)
> >+					& OWL_I2C_FIFOSTAT_TFF)
> >+				break;
> >+
> >+			writel(msg->buf[idx],
> >+					i2c_dev->base + OWL_I2C_REG_TXDAT);
> >+		}
> >+
> >+		i2c_dev->msg_ptr = idx;
> >+	}
> >+
> >+	/* Ignore the NACK if needed */
> >+	if (msg->flags & I2C_M_IGNORE_NAK)
> >+		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_FIFOCTL,
> >+				OWL_I2C_FIFOCTL_NIB, true);
> >+	else
> >+		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_FIFOCTL,
> >+				OWL_I2C_FIFOCTL_NIB, false);
> >+
> >+	/* Start the transfer */
> >+	writel(i2c_cmd, i2c_dev->base + OWL_I2C_REG_CMD);
> >+
> >+	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> >+						adap->timeout);
> >+	if (time_left == 0) {
> >+		dev_err(&adap->dev, "Transaction timed out");
> >+		/* Send stop condition and release the bus */
> >+		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> >+			OWL_I2C_CTL_GBCC_STOP | OWL_I2C_CTL_RB, true);
> >+		ret = -ETIMEDOUT;
> >+	}
> >+
> >+	/*
> >+	 * By default, 0 will be returned if interrupt occurred but no
> >+	 * read or write happened. Else if msg_ptr equals to message length,
> >+	 * message count will be returned.
> >+	 */
> 
> Zero is never a valid return value for this function. It has to be either num or a negative error. So, this comment is bad, which makes me assume the code is too. I didn't bother to check any further...
> 

hmm, okay. Can you please suggest an error value for this case? I mean
when the handler got triggered but no read/write happened.

Thanks,
Mani

> Cheers,
> Peter
> 
> >+	if (i2c_dev->msg_ptr == msg->len)
> >+		ret = num;
> >+
> >+	/* Disable I2C controller */
> >+	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> >+				OWL_I2C_CTL_EN, false);
> >+
> >+	return ret;
> >+}
> >+
> >+static const struct i2c_algorithm owl_i2c_algorithm = {
> >+	.master_xfer    = owl_i2c_master_xfer,
> >+	.functionality  = owl_i2c_func,
> >+};
> >+
> >+static const struct i2c_adapter_quirks owl_i2c_quirks = {
> >+	.flags		= I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST,
> >+	.max_read_len   = 240,
> >+	.max_write_len  = 240,
> >+	.max_comb_1st_msg_len = 6,
> >+	.max_comb_2nd_msg_len = 240,
> >+};
> >+
> >+static int owl_i2c_probe(struct platform_device *pdev)
> >+{
> >+	struct device *dev = &pdev->dev;
> >+	struct owl_i2c_dev *i2c_dev;
> >+	struct resource *res;
> >+	int ret, irq;
> >+
> >+	i2c_dev = devm_kzalloc(dev, sizeof(*i2c_dev), GFP_KERNEL);
> >+	if (!i2c_dev)
> >+		return -ENOMEM;
> >+
> >+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+	i2c_dev->base = devm_ioremap_resource(dev, res);
> >+	if (IS_ERR(i2c_dev->base))
> >+		return PTR_ERR(i2c_dev->base);
> >+
> >+	irq = platform_get_irq(pdev, 0);
> >+	if (irq < 0) {
> >+		dev_err(dev, "failed to get IRQ number\n");
> >+		return irq;
> >+	}
> >+
> >+	if (of_property_read_u32(dev->of_node, "clock-frequency",
> >+					&i2c_dev->bus_freq))
> >+		i2c_dev->bus_freq = OWL_I2C_DEF_SPEED_HZ;
> >+
> >+	/* We support only frequencies of 100k and 400k for now */
> >+	if (i2c_dev->bus_freq != OWL_I2C_DEF_SPEED_HZ &&
> >+			i2c_dev->bus_freq > OWL_I2C_MAX_SPEED_HZ) {
> >+		dev_err(dev, "invalid clock-frequency %d\n", i2c_dev->bus_freq);
> >+		return -EINVAL;
> >+	}
> >+
> >+	i2c_dev->clk = devm_clk_get(dev, NULL);
> >+	if (IS_ERR(i2c_dev->clk)) {
> >+		dev_err(dev, "failed to get clock\n");
> >+		return PTR_ERR(i2c_dev->clk);
> >+	}
> >+
> >+	ret = clk_prepare_enable(i2c_dev->clk);
> >+	if (ret)
> >+		return ret;
> >+
> >+	i2c_dev->clk_rate = clk_get_rate(i2c_dev->clk);
> >+	if (!i2c_dev->clk_rate) {
> >+		dev_err(dev, "input clock rate should not be zero\n");
> >+		ret = -EINVAL;
> >+		goto disable_clk;
> >+	}
> >+
> >+	init_completion(&i2c_dev->msg_complete);
> >+	i2c_dev->adap.owner = THIS_MODULE;
> >+	i2c_dev->adap.algo = &owl_i2c_algorithm;
> >+	i2c_dev->adap.timeout = OWL_I2C_TIMEOUT;
> >+	i2c_dev->adap.quirks = &owl_i2c_quirks;
> >+	i2c_dev->adap.dev.parent = dev;
> >+	i2c_dev->adap.dev.of_node = dev->of_node;
> >+	snprintf(i2c_dev->adap.name, sizeof(i2c_dev->adap.name),
> >+			"%s", "OWL I2C adapter");
> >+	i2c_set_adapdata(&i2c_dev->adap, i2c_dev);
> >+
> >+	platform_set_drvdata(pdev, i2c_dev);
> >+
> >+	ret = devm_request_irq(dev, irq, owl_i2c_interrupt, 0, pdev->name,
> >+				i2c_dev);
> >+	if (ret) {
> >+		dev_err(dev, "failed to request irq %d\n", irq);
> >+		goto disable_clk;
> >+	}
> >+
> >+	return i2c_add_adapter(&i2c_dev->adap);
> >+
> >+disable_clk:
> >+	clk_disable_unprepare(i2c_dev->clk);
> >+
> >+	return ret;
> >+}
> >+
> >+static const struct of_device_id owl_i2c_of_match[] = {
> >+	{.compatible = "actions,s900-i2c"},
> >+	{ /* sentinel */ }
> >+};
> >+MODULE_DEVICE_TABLE(of, owl_i2c_of_match);
> >+
> >+static struct platform_driver owl_i2c_driver = {
> >+	.probe		= owl_i2c_probe,
> >+	.driver		= {
> >+		.name	= "owl-i2c",
> >+		.of_match_table = of_match_ptr(owl_i2c_of_match),
> >+	},
> >+};
> >+module_platform_driver(owl_i2c_driver);
> >+
> >+MODULE_AUTHOR("David Liu <liuwei@actions-semi.com>");
> >+MODULE_AUTHOR("Manivannan Sadhasivam
> ><manivannan.sadhasivam@linaro.org>");
> >+MODULE_DESCRIPTION("Actions Semiconductor Owl SoC's I2C driver");
> >+MODULE_LICENSE("GPL");
> 

Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver

[Manivannan Sadhasivam]

Hi Andy,

On Sun, Jul 01, 2018 at 12:11:00AM +0300, Andy Shevchenko wrote:
> On Sat, Jun 30, 2018 at 4:33 PM, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> > Add Actions Semiconductor Owl family S900 I2C driver.
> 
> Thanks for an update. Few left comments and it would LGTM.
> 

Thanks :)

> > +static int owl_i2c_reset(struct owl_i2c_dev *i2c_dev)
> > +{
> 
> > +       mdelay(1);
> 
> But now, since it's not used in atomic context, we may switch to
> usleep_range() / msleep() instead.
> 

okay, will use msleep()

> > +       owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> > +                               OWL_I2C_CTL_EN, true);
> > +
> 
> > +       /* Wait 50ms for FIFO reset complete */
> > +       do {
> 
> > +               mdelay(1);
> 
> Especially in this case it's very important.
> 

Okay.

> > +       } while (timeout++ < OWL_I2C_MAX_RETRIES);
> 
> > +}
> 
> > +       val = (i2c_dev->clk_rate + i2c_dev->bus_freq * 16 - 1) /
> > +                               (i2c_dev->bus_freq * 16);
> 
> This is effectively DIV_ROUND_UP(->clk_rate, ->bus_freq * 16).
> 

Ack.

> > +       /*
> > +        * By default, 0 will be returned if interrupt occurred but no
> > +        * read or write happened. Else if msg_ptr equals to message length,
> > +        * message count will be returned.
> > +        */
> 
> > +       if (i2c_dev->msg_ptr == msg->len)
> > +               ret = num;
> 
> I dunno if
> 
> ret = ->msg_ptr == len ? num : 0;
> 
> would be slightly more explicit (yes, I aware about ret == 0).
> 
> Up to you to choose.
> 

As per Peter's comment, returning 0 will get changed to an error value.
Will use this pattern once we settle with a proper error value.

> > +       /* We support only frequencies of 100k and 400k for now */
> > +       if (i2c_dev->bus_freq != OWL_I2C_DEF_SPEED_HZ &&
> > +                       i2c_dev->bus_freq > OWL_I2C_MAX_SPEED_HZ) {
> 
> I think it should be != in the second case as well.
> 

yeah, agree. We don't support any other frequencies now.

Thanks,
Mani

> > +               dev_err(dev, "invalid clock-frequency %d\n", i2c_dev->bus_freq);
> > +               return -EINVAL;
> > +       }
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver

[Manivannan Sadhasivam]

On Sun, Jul 01, 2018 at 01:43:33PM +0530, Manivannan Sadhasivam wrote:
> Hi Andy,
> 
> On Sun, Jul 01, 2018 at 12:11:00AM +0300, Andy Shevchenko wrote:
> > On Sat, Jun 30, 2018 at 4:33 PM, Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > > Add Actions Semiconductor Owl family S900 I2C driver.
> > 
> > Thanks for an update. Few left comments and it would LGTM.
> > 
> 
> Thanks :)
> 
> > > +static int owl_i2c_reset(struct owl_i2c_dev *i2c_dev)
> > > +{
> > 
> > > +       mdelay(1);
> > 
> > But now, since it's not used in atomic context, we may switch to
> > usleep_range() / msleep() instead.
> > 
> 
> okay, will use msleep()
>

Just realised, I have to use spinlock for the entire owl_i2c_master_xfer
function, so can't use sleep* for delay.

> > > +       owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> > > +                               OWL_I2C_CTL_EN, true);
> > > +
> > 
> > > +       /* Wait 50ms for FIFO reset complete */
> > > +       do {
> > 
> > > +               mdelay(1);
> > 
> > Especially in this case it's very important.
> > 
> 
> Okay.

Same here, but I'm not sure about the latency. What is your suggestion?

Thanks,
Mani

> 
> > > +       } while (timeout++ < OWL_I2C_MAX_RETRIES);
> > 
> > > +}
> > 
> > > +       val = (i2c_dev->clk_rate + i2c_dev->bus_freq * 16 - 1) /
> > > +                               (i2c_dev->bus_freq * 16);
> > 
> > This is effectively DIV_ROUND_UP(->clk_rate, ->bus_freq * 16).
> > 
> 
> Ack.
> 
> > > +       /*
> > > +        * By default, 0 will be returned if interrupt occurred but no
> > > +        * read or write happened. Else if msg_ptr equals to message length,
> > > +        * message count will be returned.
> > > +        */
> > 
> > > +       if (i2c_dev->msg_ptr == msg->len)
> > > +               ret = num;
> > 
> > I dunno if
> > 
> > ret = ->msg_ptr == len ? num : 0;
> > 
> > would be slightly more explicit (yes, I aware about ret == 0).
> > 
> > Up to you to choose.
> > 
> 
> As per Peter's comment, returning 0 will get changed to an error value.
> Will use this pattern once we settle with a proper error value.
> 
> > > +       /* We support only frequencies of 100k and 400k for now */
> > > +       if (i2c_dev->bus_freq != OWL_I2C_DEF_SPEED_HZ &&
> > > +                       i2c_dev->bus_freq > OWL_I2C_MAX_SPEED_HZ) {
> > 
> > I think it should be != in the second case as well.
> > 
> 
> yeah, agree. We don't support any other frequencies now.
> 
> Thanks,
> Mani
> 
> > > +               dev_err(dev, "invalid clock-frequency %d\n", i2c_dev->bus_freq);
> > > +               return -EINVAL;
> > > +       }
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko

Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver

[Andy Shevchenko]

On Sun, Jul 1, 2018 at 11:41 AM, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
> On Sun, Jul 01, 2018 at 01:43:33PM +0530, Manivannan Sadhasivam wrote:
>> On Sun, Jul 01, 2018 at 12:11:00AM +0300, Andy Shevchenko wrote:
>> > On Sat, Jun 30, 2018 at 4:33 PM, Manivannan Sadhasivam
>> > <manivannan.sadhasivam@linaro.org> wrote:
>> > > Add Actions Semiconductor Owl family S900 I2C driver.

>> > > +static int owl_i2c_reset(struct owl_i2c_dev *i2c_dev)
>> > > +{
>> >
>> > > +       mdelay(1);
>> >
>> > But now, since it's not used in atomic context, we may switch to
>> > usleep_range() / msleep() instead.
>> >
>>
>> okay, will use msleep()
>>
>
> Just realised, I have to use spinlock for the entire owl_i2c_master_xfer
> function, so can't use sleep* for delay.
>
>> > > +       owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
>> > > +                               OWL_I2C_CTL_EN, true);
>> > > +
>> >
>> > > +       /* Wait 50ms for FIFO reset complete */
>> > > +       do {
>> >
>> > > +               mdelay(1);
>> >
>> > Especially in this case it's very important.
>> >
>>
>> Okay.
>
> Same here, but I'm not sure about the latency. What is your suggestion?

Look at other i2c bus drivers, check with data sheet you have, try to
refactor / redesign code that wouldn't need to sleep so long in atomic
context.


-- 
With Best Regards,
Andy Shevchenko