[PATCH v6 0/5] Add support for the STM32F4 I2C

[PATCH v6 0/5] Add support for the STM32F4 I2C

[M'boumba Cedric Madianga]

This patchset adds support for the I2C controller embedded in STM32F4xx SoC.
It enables I2C transfer in interrupt mode with Standard-mode and Fast-mode bus
speed.

Changes since v5:
- Change commit header from "ARM: dts:" to "ARM: dts: stm32:" (Alex)
- Change commit header from "ARM: configs:" to "ARM: configs: stm32:" (Alex)
- Fix warnings due to variable set but unused (Wolfram)
- Remove double space in Kconfig (Wolfram)
- Fix warning due to bad type parameter when using clamp() function
  (build-bot)

M'boumba Cedric Madianga (5):
  dt-bindings: Document the STM32 I2C bindings
  i2c: Add STM32F4 I2C driver
  ARM: dts: stm32: Add I2C1 support for STM32F429 SoC
  ARM: dts: stm32: Add I2C1 support for STM32429 eval board
  ARM: configs: stm32: Add I2C support for STM32 defconfig

 .../devicetree/bindings/i2c/i2c-stm32.txt          |  33 +
 arch/arm/boot/dts/stm32429i-eval.dts               |   6 +
 arch/arm/boot/dts/stm32f429.dtsi                   |  23 +
 arch/arm/configs/stm32_defconfig                   |   3 +
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-stm32f4.c                   | 849 +++++++++++++++++++++
 7 files changed, 925 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-stm32.txt
 create mode 100644 drivers/i2c/busses/i2c-stm32f4.c

-- 
1.9.1

[PATCH v6 1/5] dt-bindings: Document the STM32 I2C bindings

[M'boumba Cedric Madianga]

This patch adds documentation of device tree bindings for the STM32 I2C
controller.

Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/i2c/i2c-stm32.txt          | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-stm32.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-stm32.txt b/Documentation/devicetree/bindings/i2c/i2c-stm32.txt
new file mode 100644
index 0000000..78eaf7b
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-stm32.txt
@@ -0,0 +1,33 @@
+* I2C controller embedded in STMicroelectronics STM32 I2C platform
+
+Required properties :
+- compatible : Must be "st,stm32f4-i2c"
+- reg : Offset and length of the register set for the device
+- interrupts : Must contain the interrupt id for I2C event and then the
+  interrupt id for I2C error.
+- resets: Must contain the phandle to the reset controller.
+- clocks: Must contain the input clock of the I2C instance.
+- A pinctrl state named "default" must be defined to set pins in mode of
+  operation for I2C transfer
+- #address-cells = <1>;
+- #size-cells = <0>;
+
+Optional properties :
+- clock-frequency : Desired I2C bus clock frequency in Hz. If not specified,
+  the default 100 kHz frequency will be used. As only Normal and Fast modes
+  are supported, possible values are 100000 and 400000.
+
+Example :
+
+	i2c@40005400 {
+		compatible = "st,stm32f4-i2c";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x40005400 0x400>;
+		interrupts = <31>,
+			     <32>;
+		resets = <&rcc 277>;
+		clocks = <&rcc 0 149>;
+		pinctrl-0 = <&i2c1_sda_pin>, <&i2c1_scl_pin>;
+		pinctrl-names = "default";
+	};
-- 
1.9.1

[PATCH v6 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

This patch adds support for the STM32F4 I2C controller.

Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
---
 drivers/i2c/busses/Kconfig       |  10 +
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-stm32f4.c | 849 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 860 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-stm32f4.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 0cdc844..2719208 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -886,6 +886,16 @@ config I2C_ST
 	  This driver can also be built as module. If so, the module
 	  will be called i2c-st.
 
+config I2C_STM32F4
+	tristate "STMicroelectronics STM32F4 I2C support"
+	depends on ARCH_STM32 || COMPILE_TEST
+	help
+	  Enable this option to add support for STM32 I2C controller embedded
+	  in STM32F4 SoCs.
+
+	  This driver can also be built as module. If so, the module
+	  will be called i2c-stm32f4.
+
 config I2C_STU300
 	tristate "ST Microelectronics DDC I2C interface"
 	depends on MACH_U300
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 1c1bac8..a2c6ff5 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
 obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
 obj-$(CONFIG_I2C_SIRF)		+= i2c-sirf.o
 obj-$(CONFIG_I2C_ST)		+= i2c-st.o
+obj-$(CONFIG_I2C_STM32F4)	+= i2c-stm32f4.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
 obj-$(CONFIG_I2C_SUN6I_P2WI)	+= i2c-sun6i-p2wi.o
 obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
diff --git a/drivers/i2c/busses/i2c-stm32f4.c b/drivers/i2c/busses/i2c-stm32f4.c
new file mode 100644
index 0000000..89ad579
--- /dev/null
+++ b/drivers/i2c/busses/i2c-stm32f4.c
@@ -0,0 +1,849 @@
+/*
+ * Driver for STMicroelectronics STM32 I2C controller
+ *
+ * Copyright (C) M'boumba Cedric Madianga 2015
+ * Author: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
+ *
+ * This driver is based on i2c-st.c
+ *
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+/* STM32F4 I2C offset registers */
+#define STM32F4_I2C_CR1			0x00
+#define STM32F4_I2C_CR2			0x04
+#define STM32F4_I2C_DR			0x10
+#define STM32F4_I2C_SR1			0x14
+#define STM32F4_I2C_SR2			0x18
+#define STM32F4_I2C_CCR			0x1C
+#define STM32F4_I2C_TRISE		0x20
+#define STM32F4_I2C_FLTR		0x24
+
+/* STM32F4 I2C control 1*/
+#define STM32F4_I2C_CR1_SWRST		BIT(15)
+#define STM32F4_I2C_CR1_POS		BIT(11)
+#define STM32F4_I2C_CR1_ACK		BIT(10)
+#define STM32F4_I2C_CR1_STOP		BIT(9)
+#define STM32F4_I2C_CR1_START		BIT(8)
+#define STM32F4_I2C_CR1_PE		BIT(0)
+
+/* STM32F4 I2C control 2 */
+#define STM32F4_I2C_CR2_FREQ_MASK	GENMASK(5, 0)
+#define STM32F4_I2C_CR2_FREQ(n)		((n & STM32F4_I2C_CR2_FREQ_MASK))
+#define STM32F4_I2C_CR2_ITBUFEN		BIT(10)
+#define STM32F4_I2C_CR2_ITEVTEN		BIT(9)
+#define STM32F4_I2C_CR2_ITERREN		BIT(8)
+#define STM32F4_I2C_CR2_IRQ_MASK	(STM32F4_I2C_CR2_ITBUFEN \
+					| STM32F4_I2C_CR2_ITEVTEN \
+					| STM32F4_I2C_CR2_ITERREN)
+
+/* STM32F4 I2C Status 1 */
+#define STM32F4_I2C_SR1_AF		BIT(10)
+#define STM32F4_I2C_SR1_ARLO		BIT(9)
+#define STM32F4_I2C_SR1_BERR		BIT(8)
+#define STM32F4_I2C_SR1_TXE		BIT(7)
+#define STM32F4_I2C_SR1_RXNE		BIT(6)
+#define STM32F4_I2C_SR1_BTF		BIT(2)
+#define STM32F4_I2C_SR1_ADDR		BIT(1)
+#define STM32F4_I2C_SR1_SB		BIT(0)
+#define STM32F4_I2C_SR1_ITEVTEN_MASK	(STM32F4_I2C_SR1_BTF \
+					| STM32F4_I2C_SR1_ADDR \
+					| STM32F4_I2C_SR1_SB)
+#define STM32F4_I2C_SR1_ITBUFEN_MASK	(STM32F4_I2C_SR1_TXE \
+					| STM32F4_I2C_SR1_RXNE)
+#define STM32F4_I2C_SR1_ITERREN_MASK	(STM32F4_I2C_SR1_AF \
+					| STM32F4_I2C_SR1_ARLO \
+					| STM32F4_I2C_SR1_BERR)
+
+/* STM32F4 I2C Status 2 */
+#define STM32F4_I2C_SR2_BUSY		BIT(1)
+
+/* STM32F4 I2C Control Clock */
+#define STM32F4_I2C_CCR_CCR_MASK	GENMASK(11, 0)
+#define STM32F4_I2C_CCR_CCR(n)		((n & STM32F4_I2C_CCR_CCR_MASK))
+#define STM32F4_I2C_CCR_FS		BIT(15)
+#define STM32F4_I2C_CCR_DUTY		BIT(14)
+
+/* STM32F4 I2C Trise */
+#define STM32F4_I2C_TRISE_VALUE_MASK	GENMASK(5, 0)
+#define STM32F4_I2C_TRISE_VALUE(n)	((n & STM32F4_I2C_TRISE_VALUE_MASK))
+
+/* STM32F4 I2C Filter */
+#define STM32F4_I2C_FLTR_DNF_MASK	GENMASK(3, 0)
+#define STM32F4_I2C_FLTR_DNF(n)		((n & STM32F4_I2C_FLTR_DNF_MASK))
+#define STM32F4_I2C_FLTR_ANOFF		BIT(4)
+
+#define STM32F4_I2C_MIN_FREQ		2U
+#define STM32F4_I2C_MAX_FREQ		42U
+#define FAST_MODE_MAX_RISE_TIME		1000
+#define STD_MODE_MAX_RISE_TIME		300
+#define MHZ_TO_HZ			1000000
+
+enum stm32f4_i2c_speed {
+	STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
+	STM32F4_I2C_SPEED_FAST, /* 400 kHz */
+	STM32F4_I2C_SPEED_END,
+};
+
+/**
+ * struct stm32f4_i2c_timings - per-Mode tuning parameters
+ * @duty: Fast mode duty cycle
+ * @mul_ccr: Value to be multiplied to CCR to reach 100Khz/400Khz SCL frequency
+ * @min_ccr: Minimum clock ctrl reg value to reach 100Khz/400Khz SCL frequency
+ */
+struct stm32f4_i2c_timings {
+	u32 rate;
+	u32 duty;
+	u32 mul_ccr;
+	u32 min_ccr;
+};
+
+/**
+ * struct stm32f4_i2c_msg - client specific data
+ * @addr: 8-bit slave addr, including r/w bit
+ * @count: number of bytes to be transferred
+ * @buf: data buffer
+ * @result: result of the transfer
+ * @stop: last I2C msg to be sent, i.e. STOP to be generated
+ */
+struct stm32f4_i2c_msg {
+	u8	addr;
+	u32	count;
+	u8	*buf;
+	int	result;
+	bool	stop;
+};
+
+/**
+ * struct stm32f4_i2c_dev - private data of the controller
+ * @adap: I2C adapter for this controller
+ * @dev: device for this controller
+ * @base: virtual memory area
+ * @complete: completion of I2C message
+ * @irq_event: interrupt event line for the controller
+ * @irq_error: interrupt error line for the controller
+ * @clk: hw i2c clock
+ * speed: I2C clock frequency of the controller. Standard or Fast only supported
+ * @msg: I2C transfer information
+ */
+struct stm32f4_i2c_dev {
+	struct i2c_adapter		adap;
+	struct device			*dev;
+	void __iomem			*base;
+	struct completion		complete;
+	int				irq_event;
+	int				irq_error;
+	struct clk			*clk;
+	int				speed;
+	struct stm32f4_i2c_msg		msg;
+};
+
+static struct stm32f4_i2c_timings i2c_timings[] = {
+	[STM32F4_I2C_SPEED_STANDARD] = {
+		.mul_ccr		= 1,
+		.min_ccr		= 4,
+		.duty			= 0,
+	},
+	[STM32F4_I2C_SPEED_FAST] = {
+		.mul_ccr		= 16,
+		.min_ccr		= 1,
+		.duty			= 1,
+	},
+};
+
+static inline void stm32f4_i2c_set_bits(void __iomem *reg, u32 mask)
+{
+	writel_relaxed(readl_relaxed(reg) | mask, reg);
+}
+
+static inline void stm32f4_i2c_clr_bits(void __iomem *reg, u32 mask)
+{
+	writel_relaxed(readl_relaxed(reg) & ~mask, reg);
+}
+
+static void stm32f4_i2c_soft_reset(struct stm32f4_i2c_dev *i2c_dev)
+{
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
+
+	stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
+	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);
+}
+
+static void stm32f4_i2c_disable_it(struct stm32f4_i2c_dev *i2c_dev)
+{
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
+
+	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_IRQ_MASK);
+}
+
+static void stm32f4_i2c_set_periph_clk_freq(struct stm32f4_i2c_dev *i2c_dev)
+{
+	u32 clk_rate, cr2, freq;
+
+	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
+	clk_rate = clk_get_rate(i2c_dev->clk);
+	freq = clk_rate / MHZ_TO_HZ;
+	freq = clamp(freq, STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ);
+	cr2 |= STM32F4_I2C_CR2_FREQ(freq);
+	writel_relaxed(cr2, i2c_dev->base + STM32F4_I2C_CR2);
+}
+
+static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
+{
+	u32 trise, freq, cr2, val;
+
+	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
+
+	trise = readl_relaxed(i2c_dev->base + STM32F4_I2C_TRISE);
+	trise &= ~STM32F4_I2C_TRISE_VALUE_MASK;
+
+	/* Maximum rise time computation */
+	if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD) {
+		trise |= STM32F4_I2C_TRISE_VALUE((freq + 1));
+	} else {
+		val = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
+		trise |= STM32F4_I2C_TRISE_VALUE((val + 1));
+	}
+
+	writel_relaxed(trise, i2c_dev->base + STM32F4_I2C_TRISE);
+}
+
+static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
+	u32 ccr, clk_rate;
+	int val;
+
+	ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
+	ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
+		 STM32F4_I2C_CCR_CCR_MASK);
+
+	clk_rate = clk_get_rate(i2c_dev->clk);
+	val = clk_rate / MHZ_TO_HZ * t->mul_ccr;
+	if (val < t->min_ccr)
+		val = t->min_ccr;
+	ccr |= STM32F4_I2C_CCR_CCR(val);
+
+	if (t->duty)
+		ccr |= STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY;
+
+	writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
+}
+
+static void stm32f4_i2c_set_filter(struct stm32f4_i2c_dev *i2c_dev)
+{
+	u32 filter;
+
+	/* Enable analog noise filter and disable digital noise filter */
+	filter = readl_relaxed(i2c_dev->base + STM32F4_I2C_FLTR);
+	filter &= ~(STM32F4_I2C_FLTR_ANOFF | STM32F4_I2C_FLTR_DNF_MASK);
+	writel_relaxed(filter, i2c_dev->base + STM32F4_I2C_FLTR);
+}
+
+/**
+ * stm32f4_i2c_hw_config() - Prepare I2C block
+ * @i2c_dev: Controller's private data
+ */
+static void stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
+{
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
+
+	/* Disable I2C */
+	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
+
+	stm32f4_i2c_set_periph_clk_freq(i2c_dev);
+
+	stm32f4_i2c_set_rise_time(i2c_dev);
+
+	stm32f4_i2c_set_speed_mode(i2c_dev);
+
+	stm32f4_i2c_set_filter(i2c_dev);
+
+	/* Enable I2C */
+	stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
+}
+
+static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
+{
+	u32 status;
+	int ret;
+
+	ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
+					 status,
+					 !(status & STM32F4_I2C_SR2_BUSY),
+					 10, 1000);
+	if (ret) {
+		dev_err(i2c_dev->dev, "bus not free\n");
+		ret = -EBUSY;
+	}
+
+	return ret;
+}
+
+/**
+ * stm32f4_i2c_write_ byte() - Write a byte in the data register
+ * @i2c_dev: Controller's private data
+ * @byte: Data to write in the register
+ */
+static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
+{
+	writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
+}
+
+/**
+ * stm32f4_i2c_write_msg() - Fill the data register in write mode
+ * @i2c_dev: Controller's private data
+ *
+ * This function fills the data register with I2C transfer buffer
+ */
+static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+
+	stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
+	msg->count--;
+}
+
+static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	u32 rbuf;
+
+	rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
+	*msg->buf++ = (u8)rbuf & 0xff;
+	msg->count--;
+}
+
+static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
+
+	stm32f4_i2c_disable_it(i2c_dev);
+
+	reg = i2c_dev->base + STM32F4_I2C_CR1;
+	if (msg->stop)
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
+	else
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
+
+	complete(&i2c_dev->complete);
+}
+
+/**
+ * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
+ * @i2c_dev: Controller's private data
+ */
+static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
+
+	if (msg->count) {
+		stm32f4_i2c_write_msg(i2c_dev);
+		if (!msg->count) {
+			/* Disable BUF interrupt */
+			stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
+		}
+	} else {
+		stm32f4_i2c_terminate_xfer(i2c_dev);
+	}
+}
+
+/**
+ * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
+ * @i2c_dev: Controller's private data
+ */
+static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
+
+	switch (msg->count) {
+	case 1:
+		stm32f4_i2c_disable_it(i2c_dev);
+		stm32f4_i2c_read_msg(i2c_dev);
+		complete(&i2c_dev->complete);
+		break;
+	case 2:
+	case 3:
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
+		break;
+	default:
+		stm32f4_i2c_read_msg(i2c_dev);
+	}
+}
+
+/**
+ * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
+ * in case of read
+ * @i2c_dev: Controller's private data
+ */
+static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg;
+	u32 mask;
+	int i;
+
+	switch (msg->count) {
+	case 2:
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		/* Generate STOP or REPSTART */
+		if (msg->stop)
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
+		else
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
+
+		/* Read two last data bytes */
+		for (i = 2; i > 0; i--)
+			stm32f4_i2c_read_msg(i2c_dev);
+
+		/* Disable EVT and ERR interrupt */
+		reg = i2c_dev->base + STM32F4_I2C_CR2;
+		mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
+		stm32f4_i2c_clr_bits(reg, mask);
+
+		complete(&i2c_dev->complete);
+		break;
+	case 3:
+		/* Enable ACK and read data */
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
+		stm32f4_i2c_read_msg(i2c_dev);
+		break;
+	default:
+		stm32f4_i2c_read_msg(i2c_dev);
+	}
+}
+
+/**
+ * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
+ * master receiver
+ * @i2c_dev: Controller's private data
+ */
+static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg;
+
+	switch (msg->count) {
+	case 0:
+		stm32f4_i2c_terminate_xfer(i2c_dev);
+		/* Clear ADDR flag */
+		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+		break;
+	case 1:
+		/*
+		 * Single byte reception:
+		 * Enable NACK, clear ADDR flag and generate STOP or RepSTART
+		 */
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
+		if (msg->stop)
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
+		else
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
+		break;
+	case 2:
+		/*
+		 * 2-byte reception:
+		 * Enable NACK and PEC Position Ack and clear ADDR flag
+		 */
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
+		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+		break;
+
+	default:
+		/* N-byte reception: Enable ACK and clear ADDR flag */
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
+		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+		break;
+	}
+}
+
+/**
+ * stm32f4_i2c_isr_event() - Interrupt routine for I2C bus event
+ * @irq: interrupt number
+ * @data: Controller's private data
+ */
+static irqreturn_t stm32f4_i2c_isr_event(int irq, void *data)
+{
+	struct stm32f4_i2c_dev *i2c_dev = data;
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg;
+	u32 real_status, possible_status, ien;
+	int flag;
+
+	ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	ien &= STM32F4_I2C_CR2_IRQ_MASK;
+	possible_status = 0;
+
+	/* Check possible status combinations */
+	if (ien & STM32F4_I2C_CR2_ITEVTEN) {
+		possible_status = STM32F4_I2C_SR1_ITEVTEN_MASK;
+		if (ien & STM32F4_I2C_CR2_ITBUFEN)
+			possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
+	}
+
+	real_status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
+
+	if (!(real_status & possible_status)) {
+		dev_dbg(i2c_dev->dev,
+			"spurious evt it (status=0x%08x, ien=0x%08x)\n",
+			real_status, ien);
+		return IRQ_NONE;
+	}
+
+	/* Use __fls() to check error bits first */
+	flag = __fls(real_status & possible_status);
+
+	switch (1 << flag) {
+	case STM32F4_I2C_SR1_SB:
+		stm32f4_i2c_write_byte(i2c_dev, msg->addr);
+		break;
+
+	case STM32F4_I2C_SR1_ADDR:
+		if (msg->addr & I2C_M_RD)
+			stm32f4_i2c_handle_rx_addr(i2c_dev);
+		else
+			readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+
+		/* Enable ITBUF interrupts */
+		reg = i2c_dev->base + STM32F4_I2C_CR2;
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
+		break;
+
+	case STM32F4_I2C_SR1_BTF:
+		if (msg->addr & I2C_M_RD)
+			stm32f4_i2c_handle_rx_btf(i2c_dev);
+		else
+			stm32f4_i2c_handle_write(i2c_dev);
+		break;
+
+	case STM32F4_I2C_SR1_TXE:
+		stm32f4_i2c_handle_write(i2c_dev);
+		break;
+
+	case STM32F4_I2C_SR1_RXNE:
+		stm32f4_i2c_handle_read(i2c_dev);
+		break;
+
+	default:
+		dev_err(i2c_dev->dev,
+			"evt it unhandled: status=0x%08x)\n", real_status);
+		return IRQ_NONE;
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * stm32f4_i2c_isr_error() - Interrupt routine for I2C bus error
+ * @irq: interrupt number
+ * @data: Controller's private data
+ */
+static irqreturn_t stm32f4_i2c_isr_error(int irq, void *data)
+{
+	struct stm32f4_i2c_dev *i2c_dev = data;
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg;
+	u32 real_status, possible_status, ien;
+	int flag;
+
+	ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	ien &= STM32F4_I2C_CR2_IRQ_MASK;
+	possible_status = 0;
+
+	/* Check possible status combinations */
+	if (ien & STM32F4_I2C_CR2_ITERREN)
+		possible_status = STM32F4_I2C_SR1_ITERREN_MASK;
+
+	real_status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
+
+	if (!(real_status & possible_status)) {
+		dev_dbg(i2c_dev->dev,
+			"spurious err it (status=0x%08x, ien=0x%08x)\n",
+			real_status, ien);
+		return IRQ_NONE;
+	}
+
+	/* Use __fls() to check error bits first */
+	flag = __fls(real_status & possible_status);
+
+	switch (1 << flag) {
+	case STM32F4_I2C_SR1_BERR:
+		reg = i2c_dev->base + STM32F4_I2C_SR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_BERR);
+		msg->result = -EIO;
+		break;
+
+	case STM32F4_I2C_SR1_ARLO:
+		reg = i2c_dev->base + STM32F4_I2C_SR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_ARLO);
+		msg->result = -EAGAIN;
+		break;
+
+	case STM32F4_I2C_SR1_AF:
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
+		msg->result = -EIO;
+		break;
+
+	default:
+		dev_err(i2c_dev->dev,
+			"err it unhandled: status=0x%08x)\n", real_status);
+		return IRQ_NONE;
+	}
+
+	stm32f4_i2c_soft_reset(i2c_dev);
+	stm32f4_i2c_disable_it(i2c_dev);
+	complete(&i2c_dev->complete);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * stm32f4_i2c_xfer_msg() - Transfer a single I2C message
+ * @i2c_dev: Controller's private data
+ * @msg: I2C message to transfer
+ * @is_first: first message of the sequence
+ * @is_last: last message of the sequence
+ */
+static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
+				struct i2c_msg *msg, bool is_first,
+				bool is_last)
+{
+	struct stm32f4_i2c_msg *f4_msg = &i2c_dev->msg;
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
+	unsigned long timeout;
+	u32 mask;
+	int ret;
+
+	f4_msg->addr = i2c_8bit_addr_from_msg(msg);
+	f4_msg->buf = msg->buf;
+	f4_msg->count = msg->len;
+	f4_msg->result = 0;
+	f4_msg->stop = is_last;
+
+	reinit_completion(&i2c_dev->complete);
+
+	/* Enable ITEVT and ITERR interrupts */
+	mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
+	stm32f4_i2c_set_bits(i2c_dev->base + STM32F4_I2C_CR2, mask);
+
+	if (is_first) {
+		ret = stm32f4_i2c_wait_free_bus(i2c_dev);
+		if (ret)
+			return ret;
+
+		/* START generation */
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
+	}
+
+	timeout = wait_for_completion_timeout(&i2c_dev->complete,
+					      i2c_dev->adap.timeout);
+	ret = f4_msg->result;
+
+	/* Disable PEC position Ack */
+	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_POS);
+
+	if (!timeout)
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * stm32f4_i2c_xfer() - Transfer combined I2C message
+ * @i2c_adap: Adapter pointer to the controller
+ * @msgs: Pointer to data to be written.
+ * @num: Number of messages to be executed
+ */
+static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
+			    int num)
+{
+	struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+	int ret, i;
+
+	ret = clk_enable(i2c_dev->clk);
+	if (ret) {
+		dev_err(i2c_dev->dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	stm32f4_i2c_hw_config(i2c_dev);
+
+	for (i = 0; i < num && !ret; i++)
+		ret = stm32f4_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0,
+					   i == num - 1);
+
+	clk_disable(i2c_dev->clk);
+
+	return (ret < 0) ? ret : i;
+}
+
+static u32 stm32f4_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm stm32f4_i2c_algo = {
+	.master_xfer = stm32f4_i2c_xfer,
+	.functionality = stm32f4_i2c_func,
+};
+
+static int stm32f4_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct stm32f4_i2c_dev *i2c_dev;
+	struct resource *res;
+	u32 clk_rate;
+	struct i2c_adapter *adap;
+	struct reset_control *rst;
+	int ret;
+
+	i2c_dev = devm_kzalloc(&pdev->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(&pdev->dev, res);
+	if (IS_ERR(i2c_dev->base))
+		return PTR_ERR(i2c_dev->base);
+
+	i2c_dev->irq_event = irq_of_parse_and_map(np, 0);
+	if (!i2c_dev->irq_event) {
+		dev_err(&pdev->dev, "IRQ missing or invalid\n");
+		return -EINVAL;
+	}
+
+	i2c_dev->irq_error = irq_of_parse_and_map(np, 1);
+	if (!i2c_dev->irq_error) {
+		dev_err(&pdev->dev, "IRQ missing or invalid\n");
+		return -EINVAL;
+	}
+
+	i2c_dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(i2c_dev->clk)) {
+		dev_err(&pdev->dev, "Error: Missing controller clock\n");
+		return PTR_ERR(i2c_dev->clk);
+	}
+	ret = clk_prepare(i2c_dev->clk);
+	if (ret) {
+		dev_err(i2c_dev->dev, "Failed to prepare clock\n");
+		return ret;
+	}
+
+	rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(rst)) {
+		dev_err(&pdev->dev, "Error: Missing controller reset\n");
+		ret = PTR_ERR(rst);
+		goto clk_free;
+	}
+	reset_control_assert(rst);
+	udelay(2);
+	reset_control_deassert(rst);
+
+	i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
+	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
+	if ((!ret) && (clk_rate == 400000))
+		i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
+
+	i2c_dev->dev = &pdev->dev;
+
+	ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_event,
+					NULL, stm32f4_i2c_isr_event,
+					IRQF_ONESHOT, pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq %i\n",
+			i2c_dev->irq_error);
+		goto clk_free;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_error,
+					NULL, stm32f4_i2c_isr_error,
+					IRQF_ONESHOT, pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq %i\n",
+			i2c_dev->irq_error);
+		goto clk_free;
+	}
+
+	adap = &i2c_dev->adap;
+	i2c_set_adapdata(adap, i2c_dev);
+	snprintf(adap->name, sizeof(adap->name), "STM32 I2C(%pa)", &res->start);
+	adap->owner = THIS_MODULE;
+	adap->timeout = 2 * HZ;
+	adap->retries = 0;
+	adap->algo = &stm32f4_i2c_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+
+	init_completion(&i2c_dev->complete);
+
+	ret = i2c_add_adapter(adap);
+	if (ret)
+		goto clk_free;
+
+	platform_set_drvdata(pdev, i2c_dev);
+
+	dev_info(i2c_dev->dev, "STM32F4 I2C driver initialized\n");
+
+	return 0;
+
+clk_free:
+	clk_unprepare(i2c_dev->clk);
+	return ret;
+}
+
+static int stm32f4_i2c_remove(struct platform_device *pdev)
+{
+	struct stm32f4_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c_dev->adap);
+
+	clk_unprepare(i2c_dev->clk);
+
+	return 0;
+}
+
+static const struct of_device_id stm32f4_i2c_match[] = {
+	{ .compatible = "st,stm32f4-i2c", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stm32f4_i2c_match);
+
+static struct platform_driver stm32f4_i2c_driver = {
+	.driver = {
+		.name = "stm32f4-i2c",
+		.of_match_table = stm32f4_i2c_match,
+	},
+	.probe = stm32f4_i2c_probe,
+	.remove = stm32f4_i2c_remove,
+};
+
+module_platform_driver(stm32f4_i2c_driver);
+
+MODULE_AUTHOR("M'boumba Cedric Madianga <cedric.madianga@gmail.com>");
+MODULE_DESCRIPTION("STMicroelectronics STM32F4 I2C driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v6 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello,

On Mon, Dec 12, 2016 at 05:15:39PM +0100, M'boumba Cedric Madianga wrote:
> This patch adds support for the STM32F4 I2C controller.
> 
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
> ---
>  drivers/i2c/busses/Kconfig       |  10 +
>  drivers/i2c/busses/Makefile      |   1 +
>  drivers/i2c/busses/i2c-stm32f4.c | 849 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 860 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-stm32f4.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 0cdc844..2719208 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -886,6 +886,16 @@ config I2C_ST
>  	  This driver can also be built as module. If so, the module
>  	  will be called i2c-st.
>  
> +config I2C_STM32F4
> +	tristate "STMicroelectronics STM32F4 I2C support"
> +	depends on ARCH_STM32 || COMPILE_TEST
> +	help
> +	  Enable this option to add support for STM32 I2C controller embedded
> +	  in STM32F4 SoCs.
> +
> +	  This driver can also be built as module. If so, the module
> +	  will be called i2c-stm32f4.
> +
>  config I2C_STU300
>  	tristate "ST Microelectronics DDC I2C interface"
>  	depends on MACH_U300
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 1c1bac8..a2c6ff5 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
>  obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
>  obj-$(CONFIG_I2C_SIRF)		+= i2c-sirf.o
>  obj-$(CONFIG_I2C_ST)		+= i2c-st.o
> +obj-$(CONFIG_I2C_STM32F4)	+= i2c-stm32f4.o
>  obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
>  obj-$(CONFIG_I2C_SUN6I_P2WI)	+= i2c-sun6i-p2wi.o
>  obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
> diff --git a/drivers/i2c/busses/i2c-stm32f4.c b/drivers/i2c/busses/i2c-stm32f4.c
> new file mode 100644
> index 0000000..89ad579
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-stm32f4.c
> @@ -0,0 +1,849 @@
> +/*
> + * Driver for STMicroelectronics STM32 I2C controller
> + *
> + * Copyright (C) M'boumba Cedric Madianga 2015
> + * Author: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
> + *
> + * This driver is based on i2c-st.c
> + *
> + * License terms:  GNU General Public License (GPL), version 2
> + */

If there is a public description available for the device, a link here
would be great.

> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +
> +/* STM32F4 I2C offset registers */
> +#define STM32F4_I2C_CR1			0x00
> +#define STM32F4_I2C_CR2			0x04
> +#define STM32F4_I2C_DR			0x10
> +#define STM32F4_I2C_SR1			0x14
> +#define STM32F4_I2C_SR2			0x18
> +#define STM32F4_I2C_CCR			0x1C
> +#define STM32F4_I2C_TRISE		0x20
> +#define STM32F4_I2C_FLTR		0x24
> +
> +/* STM32F4 I2C control 1*/
> +#define STM32F4_I2C_CR1_SWRST		BIT(15)
> +#define STM32F4_I2C_CR1_POS		BIT(11)
> +#define STM32F4_I2C_CR1_ACK		BIT(10)
> +#define STM32F4_I2C_CR1_STOP		BIT(9)
> +#define STM32F4_I2C_CR1_START		BIT(8)
> +#define STM32F4_I2C_CR1_PE		BIT(0)
> +
> +/* STM32F4 I2C control 2 */
> +#define STM32F4_I2C_CR2_FREQ_MASK	GENMASK(5, 0)
> +#define STM32F4_I2C_CR2_FREQ(n)		((n & STM32F4_I2C_CR2_FREQ_MASK))

This should better be ((n) & STM32F4_I2C_CR2_FREQ_MASK). There a few
more constants that need the same fix.

> +#define STM32F4_I2C_CR2_ITBUFEN		BIT(10)
> +#define STM32F4_I2C_CR2_ITEVTEN		BIT(9)
> +#define STM32F4_I2C_CR2_ITERREN		BIT(8)
> +#define STM32F4_I2C_CR2_IRQ_MASK	(STM32F4_I2C_CR2_ITBUFEN \
> +					| STM32F4_I2C_CR2_ITEVTEN \
> +					| STM32F4_I2C_CR2_ITERREN)

I'd layout this like:

	#define STM32F4_I2C_CR2_IRQ_MASK	(STM32F4_I2C_CR2_ITBUFEN | \
						 STM32F4_I2C_CR2_ITEVTEN | \
						 STM32F4_I2C_CR2_ITERREN)
		
which is more usual I think.

> +/* STM32F4 I2C Status 1 */
> +#define STM32F4_I2C_SR1_AF		BIT(10)
> +#define STM32F4_I2C_SR1_ARLO		BIT(9)
> +#define STM32F4_I2C_SR1_BERR		BIT(8)
> +#define STM32F4_I2C_SR1_TXE		BIT(7)
> +#define STM32F4_I2C_SR1_RXNE		BIT(6)
> +#define STM32F4_I2C_SR1_BTF		BIT(2)
> +#define STM32F4_I2C_SR1_ADDR		BIT(1)
> +#define STM32F4_I2C_SR1_SB		BIT(0)
> +#define STM32F4_I2C_SR1_ITEVTEN_MASK	(STM32F4_I2C_SR1_BTF \
> +					| STM32F4_I2C_SR1_ADDR \
> +					| STM32F4_I2C_SR1_SB)
> +#define STM32F4_I2C_SR1_ITBUFEN_MASK	(STM32F4_I2C_SR1_TXE \
> +					| STM32F4_I2C_SR1_RXNE)
> +#define STM32F4_I2C_SR1_ITERREN_MASK	(STM32F4_I2C_SR1_AF \
> +					| STM32F4_I2C_SR1_ARLO \
> +					| STM32F4_I2C_SR1_BERR)
> +
> +/* STM32F4 I2C Status 2 */
> +#define STM32F4_I2C_SR2_BUSY		BIT(1)
> +
> +/* STM32F4 I2C Control Clock */
> +#define STM32F4_I2C_CCR_CCR_MASK	GENMASK(11, 0)
> +#define STM32F4_I2C_CCR_CCR(n)		((n & STM32F4_I2C_CCR_CCR_MASK))
> +#define STM32F4_I2C_CCR_FS		BIT(15)
> +#define STM32F4_I2C_CCR_DUTY		BIT(14)
> +
> +/* STM32F4 I2C Trise */
> +#define STM32F4_I2C_TRISE_VALUE_MASK	GENMASK(5, 0)
> +#define STM32F4_I2C_TRISE_VALUE(n)	((n & STM32F4_I2C_TRISE_VALUE_MASK))
> +
> +/* STM32F4 I2C Filter */
> +#define STM32F4_I2C_FLTR_DNF_MASK	GENMASK(3, 0)
> +#define STM32F4_I2C_FLTR_DNF(n)		((n & STM32F4_I2C_FLTR_DNF_MASK))
> +#define STM32F4_I2C_FLTR_ANOFF		BIT(4)
> +
> +#define STM32F4_I2C_MIN_FREQ		2U
> +#define STM32F4_I2C_MAX_FREQ		42U
> +#define FAST_MODE_MAX_RISE_TIME		1000
> +#define STD_MODE_MAX_RISE_TIME		300

Are these supposed to be the values "rise time of both SDA and SCL
signals" from the i2c specification? If so, you got it wrong, fast mode
has the smaller value.
Maybe these constants could get a home in a more central place?
Also I'd add /* ns */ to the definition.

> +#define MHZ_TO_HZ			1000000
> +
> +enum stm32f4_i2c_speed {
> +	STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
> +	STM32F4_I2C_SPEED_FAST, /* 400 kHz */
> +	STM32F4_I2C_SPEED_END,
> +};
> +
> +/**
> + * struct stm32f4_i2c_timings - per-Mode tuning parameters
> + * @duty: Fast mode duty cycle
> + * @mul_ccr: Value to be multiplied to CCR to reach 100Khz/400Khz SCL frequency
> + * @min_ccr: Minimum clock ctrl reg value to reach 100Khz/400Khz SCL frequency
> + */
> +struct stm32f4_i2c_timings {
> +	u32 rate;

rate is undocumented and unused.

> +	u32 duty;
> +	u32 mul_ccr;
> +	u32 min_ccr;
> +};
> +
> +/**
> + * struct stm32f4_i2c_msg - client specific data
> + * @addr: 8-bit slave addr, including r/w bit
> + * @count: number of bytes to be transferred
> + * @buf: data buffer
> + * @result: result of the transfer
> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> + */
> +struct stm32f4_i2c_msg {
> +	u8	addr;
> +	u32	count;
> +	u8	*buf;
> +	int	result;
> +	bool	stop;
> +};
> +
> +/**
> + * struct stm32f4_i2c_dev - private data of the controller
> + * @adap: I2C adapter for this controller
> + * @dev: device for this controller
> + * @base: virtual memory area
> + * @complete: completion of I2C message
> + * @irq_event: interrupt event line for the controller
> + * @irq_error: interrupt error line for the controller
> + * @clk: hw i2c clock
> + * speed: I2C clock frequency of the controller. Standard or Fast only supported
> + * @msg: I2C transfer information
> + */
> +struct stm32f4_i2c_dev {
> +	struct i2c_adapter		adap;
> +	struct device			*dev;
> +	void __iomem			*base;
> +	struct completion		complete;
> +	int				irq_event;
> +	int				irq_error;

You only use irq_event in the probe function. So there is no need to
remember this one and you could use a local variable instead.

> +	struct clk			*clk;
> +	int				speed;
> +	struct stm32f4_i2c_msg		msg;
> +};
> +
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> +	[STM32F4_I2C_SPEED_STANDARD] = {
> +		.mul_ccr		= 1,
> +		.min_ccr		= 4,
> +		.duty			= 0,
> +	},
> +	[STM32F4_I2C_SPEED_FAST] = {
> +		.mul_ccr		= 16,
> +		.min_ccr		= 1,
> +		.duty			= 1,
> +	},

Are these values from the datasheet?

> +};
> +
> +static inline void stm32f4_i2c_set_bits(void __iomem *reg, u32 mask)
> +{
> +	writel_relaxed(readl_relaxed(reg) | mask, reg);
> +}
> +
> +static inline void stm32f4_i2c_clr_bits(void __iomem *reg, u32 mask)
> +{
> +	writel_relaxed(readl_relaxed(reg) & ~mask, reg);
> +}
> +
> +static void stm32f4_i2c_soft_reset(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> +
> +	stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
> +	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);

Not very critical, but you're doing an unneeded register access here
because the register is read twice.

Also I think readability would improve if you dropped
stm32f4_i2c_{set,clr}_bits and do their logic explicitly in the callers. 

	stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);

vs

	val = readl_relaxed(reg);
	writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
	writel_relaxed(val, reg);

> +}
> +
> +static void stm32f4_i2c_disable_it(struct stm32f4_i2c_dev *i2c_dev)

What is "it"? If it stands for "interrupt" the more usual abbrev is
"irq".

> +{
> +	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> +	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_IRQ_MASK);
> +}
> +
> +static void stm32f4_i2c_set_periph_clk_freq(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	u32 clk_rate, cr2, freq;
> +
> +	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> +	cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
> +	clk_rate = clk_get_rate(i2c_dev->clk);
> +	freq = clk_rate / MHZ_TO_HZ;
> +	freq = clamp(freq, STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ);
> +	cr2 |= STM32F4_I2C_CR2_FREQ(freq);
> +	writel_relaxed(cr2, i2c_dev->base + STM32F4_I2C_CR2);

Can you quote the data sheet enough in a comment here to make it obvious
that your calculation is right?

Would it be more sensible to error out if clk_rate / MHZ_TO_HZ isn't in
the interval [STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ]?

Usually I would expect that you need to use
DIV_ROUND_UP(clk_rate, MHZ_TO_HZ) instead of a plain division.

> +}
> +
> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	u32 trise, freq, cr2, val;
> +
> +	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> +	freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> +
> +	trise = readl_relaxed(i2c_dev->base + STM32F4_I2C_TRISE);
> +	trise &= ~STM32F4_I2C_TRISE_VALUE_MASK;

Are you required to use rmw for STM32F4_I2C_TRISE? I'd prefer

	writel_relaxed(STM32F4_I2C_TRISE_VALUE(..), i2c_dev->base + STM32F4_I2C_TRISE);

unless the datasheet requires rmw.

> +	/* Maximum rise time computation */
> +	if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD) {
> +		trise |= STM32F4_I2C_TRISE_VALUE((freq + 1));

A single pair of parenthesis is enough when you fix
STM32F4_I2C_TRISE_VALUE as suggested above.

> +	} else {
> +		val = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
> +		trise |= STM32F4_I2C_TRISE_VALUE((val + 1));

val could be local to this branch.

Or make it shorter using:

	freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
	if (i2c_dev->speed == STM32F4_I2C_SPEED_FAST)
		freq = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;

	writel_relaxed(STM32F4_I2C_TRISE_VALUE(freq + 1), ...);

A quote from the data sheet about the algorithm would be good here, too.

> +	}
> +
> +	writel_relaxed(trise, i2c_dev->base + STM32F4_I2C_TRISE);
> +}
> +
> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
> +	u32 ccr, clk_rate;
> +	int val;
> +
> +	ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
> +	ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
> +		 STM32F4_I2C_CCR_CCR_MASK);
> +
> +	clk_rate = clk_get_rate(i2c_dev->clk);
> +	val = clk_rate / MHZ_TO_HZ * t->mul_ccr;

Is the rounding done right? Again please describe the hardware in a
comment.

> +	if (val < t->min_ccr)
> +		val = t->min_ccr;
> +	ccr |= STM32F4_I2C_CCR_CCR(val);
> +
> +	if (t->duty)
> +		ccr |= STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY;
> +
> +	writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
> +}
> +[...]
> +
> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	u32 status;
> +	int ret;
> +
> +	ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> +					 status,
> +					 !(status & STM32F4_I2C_SR2_BUSY),
> +					 10, 1000);
> +	if (ret) {
> +		dev_err(i2c_dev->dev, "bus not free\n");
> +		ret = -EBUSY;

I'm not sure if "bus not free" deserves an error message. Wolfram?

> +	}
> +
> +	return ret;
> +}
> +
> +[...]
> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	u32 rbuf;
> +
> +	rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> +	*msg->buf++ = (u8)rbuf & 0xff;

unneeded cast (or unneeded & 0xff).

> +	msg->count--;
> +}
> +
> +[...]
> +/**
> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> +	switch (msg->count) {
> +	case 1:
> +		stm32f4_i2c_disable_it(i2c_dev);
> +		stm32f4_i2c_read_msg(i2c_dev);
> +		complete(&i2c_dev->complete);
> +		break;
> +	case 2:
> +	case 3:
> +		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> +		break;
> +	default:
> +		stm32f4_i2c_read_msg(i2c_dev);
> +	}

It looks wrong that you don't call stm32f4_i2c_read_msg if msg->count is
2 or 3. I guess that's because these cases are handled in
stm32f4_i2c_handle_rx_btf? Maybe you can simplify the logic a bit?

> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
> + * in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	void __iomem *reg;
> +	u32 mask;
> +	int i;
> +
> +	switch (msg->count) {

I don't understand why the handling depends on the number of messages.

> +	case 2:
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		/* Generate STOP or REPSTART */

I stumbled about "REPSTART" and would spell it out as "repeated Start".

> +		if (msg->stop)
> +			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> +		else
> +			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +
> +		/* Read two last data bytes */
> +		for (i = 2; i > 0; i--)
> +			stm32f4_i2c_read_msg(i2c_dev);
> +
> +		/* Disable EVT and ERR interrupt */
> +		reg = i2c_dev->base + STM32F4_I2C_CR2;
> +		mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
> +		stm32f4_i2c_clr_bits(reg, mask);
> +
> +		complete(&i2c_dev->complete);
> +		break;
> +	case 3:
> +		/* Enable ACK and read data */
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> +		stm32f4_i2c_read_msg(i2c_dev);
> +		break;
> +	default:
> +		stm32f4_i2c_read_msg(i2c_dev);
> +	}
> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
> + * master receiver
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	void __iomem *reg;
> +
> +	switch (msg->count) {
> +	case 0:
> +		stm32f4_i2c_terminate_xfer(i2c_dev);
> +		/* Clear ADDR flag */
> +		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +		break;
> +	case 1:
> +		/*
> +		 * Single byte reception:
> +		 * Enable NACK, clear ADDR flag and generate STOP or RepSTART
> +		 */
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> +		if (msg->stop)
> +			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> +		else
> +			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +		break;
> +	case 2:
> +		/*
> +		 * 2-byte reception:
> +		 * Enable NACK and PEC Position Ack and clear ADDR flag

What is PEC?

> +		 */
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> +		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
> +		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +		break;
> +
> +	default:
> +		/* N-byte reception: Enable ACK and clear ADDR flag */
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
> +		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +		break;
> +	}
> +}
> +
> +/**
> + * stm32f4_i2c_isr_event() - Interrupt routine for I2C bus event
> + * @irq: interrupt number
> + * @data: Controller's private data
> + */
> +static irqreturn_t stm32f4_i2c_isr_event(int irq, void *data)
> +{
> +[...]
> +	real_status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);

s/real_status/status/ ?

> +
> +	if (!(real_status & possible_status)) {
> +		dev_dbg(i2c_dev->dev,
> +			"spurious evt it (status=0x%08x, ien=0x%08x)\n",
> +			real_status, ien);

s/it/irq/

> +		return IRQ_NONE;
> +	}
> +
> +	/* Use __fls() to check error bits first */
> +	flag = __fls(real_status & possible_status);

If you get several events reported you only handle a single one. Is this
effective?

> +	switch (1 << flag) {
> +	case STM32F4_I2C_SR1_SB:
> +		stm32f4_i2c_write_byte(i2c_dev, msg->addr);
> +		break;
> +
> +	case STM32F4_I2C_SR1_ADDR:
> +		if (msg->addr & I2C_M_RD)
> +			stm32f4_i2c_handle_rx_addr(i2c_dev);
> +		else
> +			readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +
> +		/* Enable ITBUF interrupts */

What is ITBUF?

> +		reg = i2c_dev->base + STM32F4_I2C_CR2;
> +		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> +		break;
> +
> +	case STM32F4_I2C_SR1_BTF:
> +		if (msg->addr & I2C_M_RD)
> +			stm32f4_i2c_handle_rx_btf(i2c_dev);
> +		else
> +			stm32f4_i2c_handle_write(i2c_dev);
> +		break;
> +
> +	case STM32F4_I2C_SR1_TXE:
> +		stm32f4_i2c_handle_write(i2c_dev);
> +		break;
> +
> +	case STM32F4_I2C_SR1_RXNE:
> +		stm32f4_i2c_handle_read(i2c_dev);
> +		break;
> +
> +	default:
> +		dev_err(i2c_dev->dev,
> +			"evt it unhandled: status=0x%08x)\n", real_status);

s/it/irq/

> +		return IRQ_NONE;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +[...]
> +static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
> +				struct i2c_msg *msg, bool is_first,
> +				bool is_last)
> +{
> +[...]
> +	/* Enable ITEVT and ITERR interrupts */

This comment isn't helpful. Mentioning their meaning would be great
instead.

> +[...]
> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
> +			    int num)
> +{
> +	struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> +	int ret, i;
> +
> +	ret = clk_enable(i2c_dev->clk);
> +	if (ret) {
> +		dev_err(i2c_dev->dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	stm32f4_i2c_hw_config(i2c_dev);
> +
> +	for (i = 0; i < num && !ret; i++)
> +		ret = stm32f4_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0,
> +					   i == num - 1);
> +
> +	clk_disable(i2c_dev->clk);
> +
> +	return (ret < 0) ? ret : i;

using num instead of i would be a bit more obvious.

> +static int stm32f4_i2c_probe(struct platform_device *pdev)
> +{
> +[...]
> +	i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
> +	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
> +	if ((!ret) && (clk_rate == 400000))
> +		i2c_dev->speed = STM32F4_I2C_SPEED_FAST;

I'd use

	if (!ret && clk_rate >= 400000)
		i2c_dev->speed = STM32F4_I2C_SPEED_FAST;

. That's less parenthesis and a more robust selection of the bus
frequency.

> +
> +	i2c_dev->dev = &pdev->dev;
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_event,
> +					NULL, stm32f4_i2c_isr_event,
> +					IRQF_ONESHOT, pdev->name, i2c_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq %i\n",
> +			i2c_dev->irq_error);

That's wrong. Requesting irq_event failed.

> +		goto clk_free;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_error,
> +					NULL, stm32f4_i2c_isr_error,
> +					IRQF_ONESHOT, pdev->name, i2c_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq %i\n",
> +			i2c_dev->irq_error);
> +		goto clk_free;

It would also be nice to know for which type of irq this failed. I.e.
please point out if this is the error irq or the event irq in the
message. Ditto for checking the return type of irq_of_parse_and_map.

> +	}
> +
> +	adap = &i2c_dev->adap;
> +	i2c_set_adapdata(adap, i2c_dev);
> +	snprintf(adap->name, sizeof(adap->name), "STM32 I2C(%pa)", &res->start);
> +	adap->owner = THIS_MODULE;
> +	adap->timeout = 2 * HZ;
> +	adap->retries = 0;
> +	adap->algo = &stm32f4_i2c_algo;
> +	adap->dev.parent = &pdev->dev;
> +	adap->dev.of_node = pdev->dev.of_node;
> +
> +	init_completion(&i2c_dev->complete);
> +
> +	ret = i2c_add_adapter(adap);
> +	if (ret)
> +		goto clk_free;
> +
> +	platform_set_drvdata(pdev, i2c_dev);
> +
> +	dev_info(i2c_dev->dev, "STM32F4 I2C driver initialized\n");

This is wrong. The driver is bound now to a device, not initialized.

> +static const struct of_device_id stm32f4_i2c_match[] = {
> +	{ .compatible = "st,stm32f4-i2c", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, stm32f4_i2c_match);
> +
> +static struct platform_driver stm32f4_i2c_driver = {
> +	.driver = {
> +		.name = "stm32f4-i2c",
> +		.of_match_table = stm32f4_i2c_match,

Is this needed?

> +	},
> +	.probe = stm32f4_i2c_probe,
> +	.remove = stm32f4_i2c_remove,
> +};

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v6 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Hello,

Thanks for this review, it will help.

2016-12-13 10:20 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Mon, Dec 12, 2016 at 05:15:39PM +0100, M'boumba Cedric Madianga wrote:
>> This patch adds support for the STM32F4 I2C controller.
>>
>> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
>> ---
>>  drivers/i2c/busses/Kconfig       |  10 +
>>  drivers/i2c/busses/Makefile      |   1 +
>>  drivers/i2c/busses/i2c-stm32f4.c | 849 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 860 insertions(+)
>>  create mode 100644 drivers/i2c/busses/i2c-stm32f4.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 0cdc844..2719208 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -886,6 +886,16 @@ config I2C_ST
>>         This driver can also be built as module. If so, the module
>>         will be called i2c-st.
>>
>> +config I2C_STM32F4
>> +     tristate "STMicroelectronics STM32F4 I2C support"
>> +     depends on ARCH_STM32 || COMPILE_TEST
>> +     help
>> +       Enable this option to add support for STM32 I2C controller embedded
>> +       in STM32F4 SoCs.
>> +
>> +       This driver can also be built as module. If so, the module
>> +       will be called i2c-stm32f4.
>> +
>>  config I2C_STU300
>>       tristate "ST Microelectronics DDC I2C interface"
>>       depends on MACH_U300
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 1c1bac8..a2c6ff5 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_I2C_SH_MOBILE) += i2c-sh_mobile.o
>>  obj-$(CONFIG_I2C_SIMTEC)     += i2c-simtec.o
>>  obj-$(CONFIG_I2C_SIRF)               += i2c-sirf.o
>>  obj-$(CONFIG_I2C_ST)         += i2c-st.o
>> +obj-$(CONFIG_I2C_STM32F4)    += i2c-stm32f4.o
>>  obj-$(CONFIG_I2C_STU300)     += i2c-stu300.o
>>  obj-$(CONFIG_I2C_SUN6I_P2WI) += i2c-sun6i-p2wi.o
>>  obj-$(CONFIG_I2C_TEGRA)              += i2c-tegra.o
>> diff --git a/drivers/i2c/busses/i2c-stm32f4.c b/drivers/i2c/busses/i2c-stm32f4.c
>> new file mode 100644
>> index 0000000..89ad579
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-stm32f4.c
>> @@ -0,0 +1,849 @@
>> +/*
>> + * Driver for STMicroelectronics STM32 I2C controller
>> + *
>> + * Copyright (C) M'boumba Cedric Madianga 2015
>> + * Author: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
>> + *
>> + * This driver is based on i2c-st.c
>> + *
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>
> If there is a public description available for the device, a link here
> would be great.
The device is described in the reference manual of the STM32F429/439 SoC.
As this reference manual is public, I will add it at the beginning of
the driver as requested.

>
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +
>> +/* STM32F4 I2C offset registers */
>> +#define STM32F4_I2C_CR1                      0x00
>> +#define STM32F4_I2C_CR2                      0x04
>> +#define STM32F4_I2C_DR                       0x10
>> +#define STM32F4_I2C_SR1                      0x14
>> +#define STM32F4_I2C_SR2                      0x18
>> +#define STM32F4_I2C_CCR                      0x1C
>> +#define STM32F4_I2C_TRISE            0x20
>> +#define STM32F4_I2C_FLTR             0x24
>> +
>> +/* STM32F4 I2C control 1*/
>> +#define STM32F4_I2C_CR1_SWRST                BIT(15)
>> +#define STM32F4_I2C_CR1_POS          BIT(11)
>> +#define STM32F4_I2C_CR1_ACK          BIT(10)
>> +#define STM32F4_I2C_CR1_STOP         BIT(9)
>> +#define STM32F4_I2C_CR1_START                BIT(8)
>> +#define STM32F4_I2C_CR1_PE           BIT(0)
>> +
>> +/* STM32F4 I2C control 2 */
>> +#define STM32F4_I2C_CR2_FREQ_MASK    GENMASK(5, 0)
>> +#define STM32F4_I2C_CR2_FREQ(n)              ((n & STM32F4_I2C_CR2_FREQ_MASK))
>
> This should better be ((n) & STM32F4_I2C_CR2_FREQ_MASK). There a few
> more constants that need the same fix.
OK I will fix it in the V7. Thanks.

>
>> +#define STM32F4_I2C_CR2_ITBUFEN              BIT(10)
>> +#define STM32F4_I2C_CR2_ITEVTEN              BIT(9)
>> +#define STM32F4_I2C_CR2_ITERREN              BIT(8)
>> +#define STM32F4_I2C_CR2_IRQ_MASK     (STM32F4_I2C_CR2_ITBUFEN \
>> +                                     | STM32F4_I2C_CR2_ITEVTEN \
>> +                                     | STM32F4_I2C_CR2_ITERREN)
>
> I'd layout this like:
>
>         #define STM32F4_I2C_CR2_IRQ_MASK        (STM32F4_I2C_CR2_ITBUFEN | \
>                                                  STM32F4_I2C_CR2_ITEVTEN | \
>                                                  STM32F4_I2C_CR2_ITERREN)
>
> which is more usual I think.
OK I will fix it in the V7. Thanks.

>
>> +/* STM32F4 I2C Status 1 */
>> +#define STM32F4_I2C_SR1_AF           BIT(10)
>> +#define STM32F4_I2C_SR1_ARLO         BIT(9)
>> +#define STM32F4_I2C_SR1_BERR         BIT(8)
>> +#define STM32F4_I2C_SR1_TXE          BIT(7)
>> +#define STM32F4_I2C_SR1_RXNE         BIT(6)
>> +#define STM32F4_I2C_SR1_BTF          BIT(2)
>> +#define STM32F4_I2C_SR1_ADDR         BIT(1)
>> +#define STM32F4_I2C_SR1_SB           BIT(0)
>> +#define STM32F4_I2C_SR1_ITEVTEN_MASK (STM32F4_I2C_SR1_BTF \
>> +                                     | STM32F4_I2C_SR1_ADDR \
>> +                                     | STM32F4_I2C_SR1_SB)
>> +#define STM32F4_I2C_SR1_ITBUFEN_MASK (STM32F4_I2C_SR1_TXE \
>> +                                     | STM32F4_I2C_SR1_RXNE)
>> +#define STM32F4_I2C_SR1_ITERREN_MASK (STM32F4_I2C_SR1_AF \
>> +                                     | STM32F4_I2C_SR1_ARLO \
>> +                                     | STM32F4_I2C_SR1_BERR)
>> +
>> +/* STM32F4 I2C Status 2 */
>> +#define STM32F4_I2C_SR2_BUSY         BIT(1)
>> +
>> +/* STM32F4 I2C Control Clock */
>> +#define STM32F4_I2C_CCR_CCR_MASK     GENMASK(11, 0)
>> +#define STM32F4_I2C_CCR_CCR(n)               ((n & STM32F4_I2C_CCR_CCR_MASK))
>> +#define STM32F4_I2C_CCR_FS           BIT(15)
>> +#define STM32F4_I2C_CCR_DUTY         BIT(14)
>> +
>> +/* STM32F4 I2C Trise */
>> +#define STM32F4_I2C_TRISE_VALUE_MASK GENMASK(5, 0)
>> +#define STM32F4_I2C_TRISE_VALUE(n)   ((n & STM32F4_I2C_TRISE_VALUE_MASK))
>> +
>> +/* STM32F4 I2C Filter */
>> +#define STM32F4_I2C_FLTR_DNF_MASK    GENMASK(3, 0)
>> +#define STM32F4_I2C_FLTR_DNF(n)              ((n & STM32F4_I2C_FLTR_DNF_MASK))
>> +#define STM32F4_I2C_FLTR_ANOFF               BIT(4)
>> +
>> +#define STM32F4_I2C_MIN_FREQ         2U
>> +#define STM32F4_I2C_MAX_FREQ         42U
>> +#define FAST_MODE_MAX_RISE_TIME              1000
>> +#define STD_MODE_MAX_RISE_TIME               300
>
> Are these supposed to be the values "rise time of both SDA and SCL
> signals" from the i2c specification? If so, you got it wrong, fast mode
> has the smaller value.
Yes you are right. My mistake. Thanks

> Maybe these constants could get a home in a more central place?
Yes we probably could add these constants in a include file like i2c.h
or someting like that.
Wolfram, what is your opinion regarding this proposal ?

> Also I'd add /* ns */ to the definition.
Ok

>
>> +#define MHZ_TO_HZ                    1000000
>> +
>> +enum stm32f4_i2c_speed {
>> +     STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
>> +     STM32F4_I2C_SPEED_FAST, /* 400 kHz */
>> +     STM32F4_I2C_SPEED_END,
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_timings - per-Mode tuning parameters
>> + * @duty: Fast mode duty cycle
>> + * @mul_ccr: Value to be multiplied to CCR to reach 100Khz/400Khz SCL frequency
>> + * @min_ccr: Minimum clock ctrl reg value to reach 100Khz/400Khz SCL frequency
>> + */
>> +struct stm32f4_i2c_timings {
>> +     u32 rate;
>
> rate is undocumented and unused.
Good point. I will remove it. Thanks.

>
>> +     u32 duty;
>> +     u32 mul_ccr;
>> +     u32 min_ccr;
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_msg - client specific data
>> + * @addr: 8-bit slave addr, including r/w bit
>> + * @count: number of bytes to be transferred
>> + * @buf: data buffer
>> + * @result: result of the transfer
>> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
>> + */
>> +struct stm32f4_i2c_msg {
>> +     u8      addr;
>> +     u32     count;
>> +     u8      *buf;
>> +     int     result;
>> +     bool    stop;
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_dev - private data of the controller
>> + * @adap: I2C adapter for this controller
>> + * @dev: device for this controller
>> + * @base: virtual memory area
>> + * @complete: completion of I2C message
>> + * @irq_event: interrupt event line for the controller
>> + * @irq_error: interrupt error line for the controller
>> + * @clk: hw i2c clock
>> + * speed: I2C clock frequency of the controller. Standard or Fast only supported
>> + * @msg: I2C transfer information
>> + */
>> +struct stm32f4_i2c_dev {
>> +     struct i2c_adapter              adap;
>> +     struct device                   *dev;
>> +     void __iomem                    *base;
>> +     struct completion               complete;
>> +     int                             irq_event;
>> +     int                             irq_error;
>
> You only use irq_event in the probe function. So there is no need to
> remember this one and you could use a local variable instead.
Ok, I will fix it in the V7. Thanks

>
>> +     struct clk                      *clk;
>> +     int                             speed;
>> +     struct stm32f4_i2c_msg          msg;
>> +};
>> +
>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> +     [STM32F4_I2C_SPEED_STANDARD] = {
>> +             .mul_ccr                = 1,
>> +             .min_ccr                = 4,
>> +             .duty                   = 0,
>> +     },
>> +     [STM32F4_I2C_SPEED_FAST] = {
>> +             .mul_ccr                = 16,
>> +             .min_ccr                = 1,
>> +             .duty                   = 1,
>> +     },
>
> Are these values from the datasheet?
Yes, these values come from I2C IP datasheet.

>
>> +};
>> +
>> +static inline void stm32f4_i2c_set_bits(void __iomem *reg, u32 mask)
>> +{
>> +     writel_relaxed(readl_relaxed(reg) | mask, reg);
>> +}
>> +
>> +static inline void stm32f4_i2c_clr_bits(void __iomem *reg, u32 mask)
>> +{
>> +     writel_relaxed(readl_relaxed(reg) & ~mask, reg);
>> +}
>> +
>> +static void stm32f4_i2c_soft_reset(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +
>> +     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
>> +     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);
>
> Not very critical, but you're doing an unneeded register access here
> because the register is read twice.
>
> Also I think readability would improve if you dropped
> stm32f4_i2c_{set,clr}_bits and do their logic explicitly in the callers.
>
>         stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
>         stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);
>
> vs
>
>         val = readl_relaxed(reg);
>         writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
>         writel_relaxed(val, reg);
>
If it is just for this function, I don't have any objection to use
your proposal.
But if your request, it is to drop all calls of
stm32f4_i2c_{set,clr}_bits, I am wondering if it is something really
critical ?
Indeed, this is a big impact in this driver and I would prefer to avoid it.

>> +}
>> +
>> +static void stm32f4_i2c_disable_it(struct stm32f4_i2c_dev *i2c_dev)
>
> What is "it"? If it stands for "interrupt" the more usual abbrev is
> "irq".
Yes it stands for interrupt.
So, I will replace it by irq in the next version.

>
>> +{
>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> +     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_IRQ_MASK);
>> +}
>> +
>> +static void stm32f4_i2c_set_periph_clk_freq(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     u32 clk_rate, cr2, freq;
>> +
>> +     cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> +     cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
>> +     clk_rate = clk_get_rate(i2c_dev->clk);
>> +     freq = clk_rate / MHZ_TO_HZ;
>> +     freq = clamp(freq, STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ);
>> +     cr2 |= STM32F4_I2C_CR2_FREQ(freq);
>> +     writel_relaxed(cr2, i2c_dev->base + STM32F4_I2C_CR2);
>
> Can you quote the data sheet enough in a comment here to make it obvious
> that your calculation is right?
Ok I will add it.

>
> Would it be more sensible to error out if clk_rate / MHZ_TO_HZ isn't in
> the interval [STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ]?
Yes, input clock must be between 2 MHz and 42 Mhz to achieve standard
or fast mode mode I²C frequencies.
If it is not the case, the I2C signal integrity is not guarantee and
could lead to communication issue between devices on the I2C bus.

>
> Usually I would expect that you need to use
> DIV_ROUND_UP(clk_rate, MHZ_TO_HZ) instead of a plain division.
Ok, I will use this macro in the next version

>
>> +}
>> +
>> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     u32 trise, freq, cr2, val;
>> +
>> +     cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> +     freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> +
>> +     trise = readl_relaxed(i2c_dev->base + STM32F4_I2C_TRISE);
>> +     trise &= ~STM32F4_I2C_TRISE_VALUE_MASK;
>
> Are you required to use rmw for STM32F4_I2C_TRISE? I'd prefer
>
>         writel_relaxed(STM32F4_I2C_TRISE_VALUE(..), i2c_dev->base + STM32F4_I2C_TRISE);
>
> unless the datasheet requires rmw.
>
>> +     /* Maximum rise time computation */
>> +     if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD) {
>> +             trise |= STM32F4_I2C_TRISE_VALUE((freq + 1));
>
> A single pair of parenthesis is enough when you fix
> STM32F4_I2C_TRISE_VALUE as suggested above.
The datasheet does not required to use rmw so I will use your proposal
in the next version. Thanks.

>
>> +     } else {
>> +             val = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
>> +             trise |= STM32F4_I2C_TRISE_VALUE((val + 1));
>
> val could be local to this branch.
>
> Or make it shorter using:
>
>         freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>         if (i2c_dev->speed == STM32F4_I2C_SPEED_FAST)
>                 freq = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
>
>         writel_relaxed(STM32F4_I2C_TRISE_VALUE(freq + 1), ...);
>
> A quote from the data sheet about the algorithm would be good here, too.
Ok, I will use a shorter way to compute freq and add a quote from the datasheet

>
>> +     }
>> +
>> +     writel_relaxed(trise, i2c_dev->base + STM32F4_I2C_TRISE);
>> +}
>> +
>> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
>> +     u32 ccr, clk_rate;
>> +     int val;
>> +
>> +     ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
>> +     ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
>> +              STM32F4_I2C_CCR_CCR_MASK);
>> +
>> +     clk_rate = clk_get_rate(i2c_dev->clk);
>> +     val = clk_rate / MHZ_TO_HZ * t->mul_ccr;
>
> Is the rounding done right? Again please describe the hardware in a
> comment.
Ok I will use DIV_ROUND_UP here and add a comment from datasheet

>
>> +     if (val < t->min_ccr)
>> +             val = t->min_ccr;
>> +     ccr |= STM32F4_I2C_CCR_CCR(val);
>> +
>> +     if (t->duty)
>> +             ccr |= STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY;
>> +
>> +     writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
>> +}
>> +[...]
>> +
>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     u32 status;
>> +     int ret;
>> +
>> +     ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> +                                      status,
>> +                                      !(status & STM32F4_I2C_SR2_BUSY),
>> +                                      10, 1000);
>> +     if (ret) {
>> +             dev_err(i2c_dev->dev, "bus not free\n");
>> +             ret = -EBUSY;
>
> I'm not sure if "bus not free" deserves an error message. Wolfram?
>
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +[...]
>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     u32 rbuf;
>> +
>> +     rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> +     *msg->buf++ = (u8)rbuf & 0xff;
>
> unneeded cast (or unneeded & 0xff).
Ok thanks

>
>> +     msg->count--;
>> +}
>> +
>> +[...]
>> +/**
>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> +     switch (msg->count) {
>> +     case 1:
>> +             stm32f4_i2c_disable_it(i2c_dev);
>> +             stm32f4_i2c_read_msg(i2c_dev);
>> +             complete(&i2c_dev->complete);
>> +             break;
>> +     case 2:
>> +     case 3:
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> +             break;
>> +     default:
>> +             stm32f4_i2c_read_msg(i2c_dev);
>> +     }
>
> It looks wrong that you don't call stm32f4_i2c_read_msg if msg->count is
> 2 or 3. I guess that's because these cases are handled in
> stm32f4_i2c_handle_rx_btf? Maybe you can simplify the logic a bit?
stm32f4_i2c_handle_read is called when RXNE bit is set due to new data
present in DR register.
stm32f4_i2c_handle_rx_btf is called when BTF bit is set.
This bit is set when there is new data in shift register whereas data
in DR register has not been read yet.
The datasheet requires to wait for BTF bit for 2 byte reception and
for the 3 last bytes to be read for N bytes reception (with N > 2)
That's why, in these cases (2 and 3), I clear the ITBUF interrupt in
order to not be preempted again for RXNE event as I know that I have
to wait for BTF event.
So, this is not a wrong case but I will add a comment to explain that.

>
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>> + * in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     void __iomem *reg;
>> +     u32 mask;
>> +     int i;
>> +
>> +     switch (msg->count) {
>
> I don't understand why the handling depends on the number of messages.
Please see my above comment

>
>> +     case 2:
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             /* Generate STOP or REPSTART */
>
> I stumbled about "REPSTART" and would spell it out as "repeated Start".
Ok

>
>> +             if (msg->stop)
>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> +             else
>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> +             /* Read two last data bytes */
>> +             for (i = 2; i > 0; i--)
>> +                     stm32f4_i2c_read_msg(i2c_dev);
>> +
>> +             /* Disable EVT and ERR interrupt */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +             mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> +             stm32f4_i2c_clr_bits(reg, mask);
>> +
>> +             complete(&i2c_dev->complete);
>> +             break;
>> +     case 3:
>> +             /* Enable ACK and read data */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> +             stm32f4_i2c_read_msg(i2c_dev);
>> +             break;
>> +     default:
>> +             stm32f4_i2c_read_msg(i2c_dev);
>> +     }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>> + * master receiver
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     void __iomem *reg;
>> +
>> +     switch (msg->count) {
>> +     case 0:
>> +             stm32f4_i2c_terminate_xfer(i2c_dev);
>> +             /* Clear ADDR flag */
>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +             break;
>> +     case 1:
>> +             /*
>> +              * Single byte reception:
>> +              * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>> +              */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> +             if (msg->stop)
>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> +             else
>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +             break;
>> +     case 2:
>> +             /*
>> +              * 2-byte reception:
>> +              * Enable NACK and PEC Position Ack and clear ADDR flag
>
> What is PEC?
PEC stands for Packet Error Checking.
This feature is used is SMbus mode in order to guarantee data
integrity during an I2C transaction thanks to packet error code
comparaison.
The POS bit is used in reception to indicate that the next byte
received in the shift register will be ACK by hardware.
So, this is a wrong comment I would say POS ACK and I will fix it in
the next version.

>
>> +              */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +             break;
>> +
>> +     default:
>> +             /* N-byte reception: Enable ACK and clear ADDR flag */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +             break;
>> +     }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_isr_event() - Interrupt routine for I2C bus event
>> + * @irq: interrupt number
>> + * @data: Controller's private data
>> + */
>> +static irqreturn_t stm32f4_i2c_isr_event(int irq, void *data)
>> +{
>> +[...]
>> +     real_status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
>
> s/real_status/status/ ?
Ok. Thanks

>
>> +
>> +     if (!(real_status & possible_status)) {
>> +             dev_dbg(i2c_dev->dev,
>> +                     "spurious evt it (status=0x%08x, ien=0x%08x)\n",
>> +                     real_status, ien);
>
> s/it/irq/
Ok. Thanks

>
>> +             return IRQ_NONE;
>> +     }
>> +
>> +     /* Use __fls() to check error bits first */
>> +     flag = __fls(real_status & possible_status);
>
> If you get several events reported you only handle a single one. Is this
> effective?
You are right, if several events occur, I will execute this irq
routines for each event.
I will rework this in the next version. Thanks


>
>> +     switch (1 << flag) {
>> +     case STM32F4_I2C_SR1_SB:
>> +             stm32f4_i2c_write_byte(i2c_dev, msg->addr);
>> +             break;
>> +
>> +     case STM32F4_I2C_SR1_ADDR:
>> +             if (msg->addr & I2C_M_RD)
>> +                     stm32f4_i2c_handle_rx_addr(i2c_dev);
>> +             else
>> +                     readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +
>> +             /* Enable ITBUF interrupts */
>
> What is ITBUF?
ITBUF is an interrupt generated when RxNE or TxE flag is set

>
>> +             reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> +             break;
>> +
>> +     case STM32F4_I2C_SR1_BTF:
>> +             if (msg->addr & I2C_M_RD)
>> +                     stm32f4_i2c_handle_rx_btf(i2c_dev);
>> +             else
>> +                     stm32f4_i2c_handle_write(i2c_dev);
>> +             break;
>> +
>> +     case STM32F4_I2C_SR1_TXE:
>> +             stm32f4_i2c_handle_write(i2c_dev);
>> +             break;
>> +
>> +     case STM32F4_I2C_SR1_RXNE:
>> +             stm32f4_i2c_handle_read(i2c_dev);
>> +             break;
>> +
>> +     default:
>> +             dev_err(i2c_dev->dev,
>> +                     "evt it unhandled: status=0x%08x)\n", real_status);
>
> s/it/irq/
ok

>
>> +             return IRQ_NONE;
>> +     }
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +[...]
>> +static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
>> +                             struct i2c_msg *msg, bool is_first,
>> +                             bool is_last)
>> +{
>> +[...]
>> +     /* Enable ITEVT and ITERR interrupts */
>
> This comment isn't helpful. Mentioning their meaning would be great
> instead.
ok I will add it (ITEVT = event interrupt and ITERR = error interrupt)

>
>> +[...]
>> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
>> +                         int num)
>> +{
>> +     struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>> +     int ret, i;
>> +
>> +     ret = clk_enable(i2c_dev->clk);
>> +     if (ret) {
>> +             dev_err(i2c_dev->dev, "Failed to enable clock\n");
>> +             return ret;
>> +     }
>> +
>> +     stm32f4_i2c_hw_config(i2c_dev);
>> +
>> +     for (i = 0; i < num && !ret; i++)
>> +             ret = stm32f4_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0,
>> +                                        i == num - 1);
>> +
>> +     clk_disable(i2c_dev->clk);
>> +
>> +     return (ret < 0) ? ret : i;
>
> using num instead of i would be a bit more obvious.
ok

>
>> +static int stm32f4_i2c_probe(struct platform_device *pdev)
>> +{
>> +[...]
>> +     i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
>> +     ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
>> +     if ((!ret) && (clk_rate == 400000))
>> +             i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
>
> I'd use
>
>         if (!ret && clk_rate >= 400000)
>                 i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
>
> . That's less parenthesis and a more robust selection of the bus
> frequency.
ok

>
>> +
>> +     i2c_dev->dev = &pdev->dev;
>> +
>> +     ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_event,
>> +                                     NULL, stm32f4_i2c_isr_event,
>> +                                     IRQF_ONESHOT, pdev->name, i2c_dev);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to request irq %i\n",
>> +                     i2c_dev->irq_error);
>
> That's wrong. Requesting irq_event failed.
Ok Thanks.

>
>> +             goto clk_free;
>> +     }
>> +
>> +     ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_error,
>> +                                     NULL, stm32f4_i2c_isr_error,
>> +                                     IRQF_ONESHOT, pdev->name, i2c_dev);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to request irq %i\n",
>> +                     i2c_dev->irq_error);
>> +             goto clk_free;
>
> It would also be nice to know for which type of irq this failed. I.e.
> please point out if this is the error irq or the event irq in the
> message. Ditto for checking the return type of irq_of_parse_and_map.
Ok I will fix it in the next version.

>
>> +     }
>> +
>> +     adap = &i2c_dev->adap;
>> +     i2c_set_adapdata(adap, i2c_dev);
>> +     snprintf(adap->name, sizeof(adap->name), "STM32 I2C(%pa)", &res->start);
>> +     adap->owner = THIS_MODULE;
>> +     adap->timeout = 2 * HZ;
>> +     adap->retries = 0;
>> +     adap->algo = &stm32f4_i2c_algo;
>> +     adap->dev.parent = &pdev->dev;
>> +     adap->dev.of_node = pdev->dev.of_node;
>> +
>> +     init_completion(&i2c_dev->complete);
>> +
>> +     ret = i2c_add_adapter(adap);
>> +     if (ret)
>> +             goto clk_free;
>> +
>> +     platform_set_drvdata(pdev, i2c_dev);
>> +
>> +     dev_info(i2c_dev->dev, "STM32F4 I2C driver initialized\n");
>
> This is wrong. The driver is bound now to a device, not initialized.
Ok I will replaced initialized by registered. Thanks.

>
>> +static const struct of_device_id stm32f4_i2c_match[] = {
>> +     { .compatible = "st,stm32f4-i2c", },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32f4_i2c_match);
>> +
>> +static struct platform_driver stm32f4_i2c_driver = {
>> +     .driver = {
>> +             .name = "stm32f4-i2c",
>> +             .of_match_table = stm32f4_i2c_match,
>
> Is this needed?
Without of_match_table, I could not match an I2C device instance from
DT with this driver.
So maybe, there is a misunderstanding.
Could you please clarify your question ?

>
>> +     },
>> +     .probe = stm32f4_i2c_probe,
>> +     .remove = stm32f4_i2c_remove,
>> +};
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Thanks again
Best regards,

Cedric

Re: [PATCH v6 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello Cedric,

On Thu, Dec 15, 2016 at 04:26:25PM +0100, M'boumba Cedric Madianga wrote:
> >> +static struct platform_driver stm32f4_i2c_driver = {
> >> +     .driver = {
> >> +             .name = "stm32f4-i2c",
> >> +             .of_match_table = stm32f4_i2c_match,
> >
> > Is this needed?
> Without of_match_table, I could not match an I2C device instance from
> DT with this driver.
> So maybe, there is a misunderstanding.
> Could you please clarify your question ?

I thought (but I could be wrong) that if the driver is called
"stm32f4-i2c", it is used to match "st,stm32f4-i2c" even without
.of_match_table. If this doesn't work, please disregard my question.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v6 2/5] i2c: Add STM32F4 I2C driver

[Wolfram Sang]

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


> I thought (but I could be wrong) that if the driver is called
> "stm32f4-i2c", it is used to match "st,stm32f4-i2c" even without
> .of_match_table. If this doesn't work, please disregard my question.

This currently works only for *i2c client drivers*, it never did for i2c
adapters. And even for clients, this behaviour is going to be deprecated
somewhen. There are people working on it...


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

Re: [PATCH v6 2/5] i2c: Add STM32F4 I2C driver

[Wolfram Sang]

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


> > +	if (ret) {
> > +		dev_err(i2c_dev->dev, "bus not free\n");
> > +		ret = -EBUSY;
> 
> I'm not sure if "bus not free" deserves an error message. Wolfram?

I tend to agree. But I never enforced it up to now, never found the time
to double check if I could/should enforce it.


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

[PATCH v6 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC

[M'boumba Cedric Madianga]

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
---
 arch/arm/boot/dts/stm32f429.dtsi | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index 7de52ee..cbdece7 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -48,6 +48,7 @@
 #include "skeleton.dtsi"
 #include "armv7-m.dtsi"
 #include <dt-bindings/pinctrl/stm32f429-pinfunc.h>
+#include <dt-bindings/mfd/stm32f4-rcc.h>
 
 / {
 	clocks {
@@ -337,6 +338,16 @@
 					slew-rate = <2>;
 				};
 			};
+
+			i2c1_pins_b: i2c1@0 {
+				pins1 {
+					pinmux = <STM32F429_PB9_FUNC_I2C1_SDA>;
+					drive-open-drain;
+				};
+				pins2 {
+					pinmux = <STM32F429_PB6_FUNC_I2C1_SCL>;
+				};
+			};
 		};
 
 		rcc: rcc@40023810 {
@@ -409,6 +420,18 @@
 			interrupts = <80>;
 			clocks = <&rcc 0 38>;
 		};
+
+		i2c1: i2c@40005400 {
+			compatible = "st,stm32f4-i2c";
+			reg = <0x40005400 0x400>;
+			interrupts = <31>,
+				     <32>;
+			resets = <&rcc STM32F4_APB1_RESET(I2C1)>;
+			clocks = <&rcc 0 STM32F4_APB1_CLOCK(I2C1)>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
 	};
 };
 
-- 
1.9.1

Re: [PATCH v6 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC

[Alexandre Torgue]

Hi Cedric,

On 12/12/2016 05:15 PM, M'boumba Cedric Madianga wrote:
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>

Please Add a commit message.

> ---
>  arch/arm/boot/dts/stm32f429.dtsi | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
> index 7de52ee..cbdece7 100644
> --- a/arch/arm/boot/dts/stm32f429.dtsi
> +++ b/arch/arm/boot/dts/stm32f429.dtsi
> @@ -48,6 +48,7 @@
>  #include "skeleton.dtsi"
>  #include "armv7-m.dtsi"
>  #include <dt-bindings/pinctrl/stm32f429-pinfunc.h>
> +#include <dt-bindings/mfd/stm32f4-rcc.h>
>
>  / {
>  	clocks {
> @@ -337,6 +338,16 @@
>  					slew-rate = <2>;
>  				};
>  			};
> +
> +			i2c1_pins_b: i2c1@0 {
> +				pins1 {
> +					pinmux = <STM32F429_PB9_FUNC_I2C1_SDA>;
> +					drive-open-drain;
> +				};
> +				pins2 {
> +					pinmux = <STM32F429_PB6_FUNC_I2C1_SCL>;
> +				};
> +			};
>  		};
>
>  		rcc: rcc@40023810 {
> @@ -409,6 +420,18 @@
>  			interrupts = <80>;
>  			clocks = <&rcc 0 38>;
>  		};
> +
> +		i2c1: i2c@40005400 {

Can you check the order on device node please ? (should follow base@)

> +			compatible = "st,stm32f4-i2c";
> +			reg = <0x40005400 0x400>;
> +			interrupts = <31>,
> +				     <32>;
> +			resets = <&rcc STM32F4_APB1_RESET(I2C1)>;
> +			clocks = <&rcc 0 STM32F4_APB1_CLOCK(I2C1)>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +		};
>  	};
>  };
>
>

[PATCH v6 4/5] ARM: dts: stm32: Add I2C1 support for STM32429 eval board

[M'boumba Cedric Madianga]

Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
---
 arch/arm/boot/dts/stm32429i-eval.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/stm32429i-eval.dts b/arch/arm/boot/dts/stm32429i-eval.dts
index afb90bc..74e0045 100644
--- a/arch/arm/boot/dts/stm32429i-eval.dts
+++ b/arch/arm/boot/dts/stm32429i-eval.dts
@@ -141,3 +141,9 @@
 	pinctrl-names = "default";
 	status = "okay";
 };
+
+&i2c1 {
+	pinctrl-0 = <&i2c1_pins_b>;
+	pinctrl-names = "default";
+	status = "okay";
+};
-- 
1.9.1

Re: [PATCH v6 4/5] ARM: dts: stm32: Add I2C1 support for STM32429 eval board

[Alexandre Torgue]

Hi Cedric,

On 12/12/2016 05:15 PM, M'boumba Cedric Madianga wrote:
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>

Can you please add a commit message.

> ---
>  arch/arm/boot/dts/stm32429i-eval.dts | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32429i-eval.dts b/arch/arm/boot/dts/stm32429i-eval.dts
> index afb90bc..74e0045 100644
> --- a/arch/arm/boot/dts/stm32429i-eval.dts
> +++ b/arch/arm/boot/dts/stm32429i-eval.dts
> @@ -141,3 +141,9 @@
>  	pinctrl-names = "default";
>  	status = "okay";
>  };
> +
> +&i2c1 {
> +	pinctrl-0 = <&i2c1_pins_b>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
>

[PATCH v6 5/5] ARM: configs: stm32: Add I2C support for STM32 defconfig

[M'boumba Cedric Madianga]

Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
---
 arch/arm/configs/stm32_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
index e7b56d4..9494eaf 100644
--- a/arch/arm/configs/stm32_defconfig
+++ b/arch/arm/configs/stm32_defconfig
@@ -52,6 +52,9 @@ CONFIG_SERIAL_NONSTANDARD=y
 CONFIG_SERIAL_STM32=y
 CONFIG_SERIAL_STM32_CONSOLE=y
 # CONFIG_HW_RANDOM is not set
+CONFIG_I2C=y
+CONFIG_I2C_CHARDEV=y
+CONFIG_I2C_STM32F4=y
 # CONFIG_HWMON is not set
 # CONFIG_USB_SUPPORT is not set
 CONFIG_NEW_LEDS=y
-- 
1.9.1

Re: [PATCH v6 5/5] ARM: configs: stm32: Add I2C support for STM32 defconfig

[Alexandre Torgue]

Hi Cedric,

On 12/12/2016 05:15 PM, M'boumba Cedric Madianga wrote:
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>

Can you please add a commit message.

Thx in advance
Alex

> ---
>  arch/arm/configs/stm32_defconfig | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
> index e7b56d4..9494eaf 100644
> --- a/arch/arm/configs/stm32_defconfig
> +++ b/arch/arm/configs/stm32_defconfig
> @@ -52,6 +52,9 @@ CONFIG_SERIAL_NONSTANDARD=y
>  CONFIG_SERIAL_STM32=y
>  CONFIG_SERIAL_STM32_CONSOLE=y
>  # CONFIG_HW_RANDOM is not set
> +CONFIG_I2C=y
> +CONFIG_I2C_CHARDEV=y
> +CONFIG_I2C_STM32F4=y
>  # CONFIG_HWMON is not set
>  # CONFIG_USB_SUPPORT is not set
>  CONFIG_NEW_LEDS=y
>