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

[PATCH v7 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 v6:
- Add commit message for the patches in defconfig, .dtsi and .dts files (Alex)
- Order I2C instance base address in .dtsi file (Alex)
- Add commit message for the patch in stm32429i-eval.dts  (Alex)
- Add link to the STM32F4 Soc ref manual where I2C device is described (Uwe)
- Use more usal way to define constants with several lines (Uwe)
- Remove rate variable from stm32f4_i2c_timings as it is not used (Uwe)
- Remove irq variable from stm32f4_i2c_dev struct are they are only needed
  during probe (Uwe)
- Add comment from datasheet to explain stm32f4_i2c_timings values (Uwe)
- Rework i2c soft_reset implementation (Uwe)
- Replace "it" by "irq" as it is a more usual abbreviation for interrupt (Uwe)
- Add comment from datasheet to explain periph clk freq calculation (Uwe)
- Use DIV_ROUND_UP instead of plain division when required (Uwe)
- Add comment from datasheet to explain timing rise calculation (Uwe)
- Rework timing rise calculation by using shorter computation (Uwe)
- Remove (u8) cast when reading I2C data register (Uwe)
- Rework isr_event routine to handle several events during one call of the
  routine (Uwe)
- Precise which type of irq is failed when a irq request error occurs (Uwe)
- Use devm_request_irq() instead of devm_request_threaded_irq() to avoid
  spurious evt irq when clearing status registers in threaded context

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                   | 896 +++++++++++++++++++++
 7 files changed, 972 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-stm32.txt
 create mode 100644 drivers/i2c/busses/i2c-stm32f4.c

-- 
1.9.1

[PATCH v7 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 v7 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 | 896 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 907 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..ca11dee
--- /dev/null
+++ b/drivers/i2c/busses/i2c-stm32f4.c
@@ -0,0 +1,896 @@
+/*
+ * Driver for STMicroelectronics STM32 I2C controller
+ *
+ * This I2C controller is described in the STM32F429/439 Soc reference manual.
+ * Please see below a link to the documentation:
+ * http://www.st.com/resource/en/reference_manual/DM00031020.pdf
+ *
+ * Copyright (C) M'boumba Cedric Madianga 2016
+ * 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 HZ_TO_MHZ			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
+ * @scl_period: SCL low/high period in microsecond
+ * @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 duty;
+	u32 scl_period;
+	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
+ * @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;
+	struct clk			*clk;
+	int				speed;
+	struct stm32f4_i2c_msg		msg;
+};
+
+/*
+ * In standard mode:
+ * SCL high period = SCL low period = CCR * I2C CLK period
+ * So, CCR = SCL period * I2C CLK frequency
+ *
+ * In fast mode:
+ * DUTY = 0: Fast mode tlow/thigh = 2
+ * DUTY = 1: Fast mode tlow/thigh = 16/9
+ * If Duty = 0; SCL high period = 1  * CCR * I2C CLK period
+ *		SCL low period  = 2  * CCR * I2C CLK period
+ * If Duty = 1; SCL high period = 9  * CCR * I2C CLK period
+ *		SCL low period  = 16 * CCR * I2C CLK period
+ *
+ * Note that Duty has to bet set to reach 400khz in Fast mode
+ * So, in order to cover both SCL high/low with Duty = 1,
+ * CCR = 16 * SCL period * I2C CLK frequency
+ *
+ * Please note that the minimum allowed value is 0x04, except in FAST DUTY mode
+ * where the minimum allowed value is 0x01
+ */
+static struct stm32f4_i2c_timings i2c_timings[] = {
+	[STM32F4_I2C_SPEED_STANDARD] = {
+		.mul_ccr		= 1,
+		.min_ccr		= 4,
+		.duty			= 0,
+		.scl_period		= 5,
+	},
+	[STM32F4_I2C_SPEED_FAST] = {
+		.mul_ccr		= 16,
+		.min_ccr		= 1,
+		.duty			= 1,
+		.scl_period		= 2,
+	},
+};
+
+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;
+	u32 val;
+
+	val = readl_relaxed(reg);
+	writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
+	writel_relaxed(val, reg);
+}
+
+static void stm32f4_i2c_disable_irq(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;
+
+	/*
+	 * The minimum allowed frequency is 2 MHz, the maximum frequency is
+	 * limited by the maximum APB frequency 42 MHz
+	 */
+	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
+	clk_rate = clk_get_rate(i2c_dev->clk);
+	freq = DIV_ROUND_UP(clk_rate, HZ_TO_MHZ);
+	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;
+
+	/*
+	 * These bits must be programmed with the maximum SCL rise time given in
+	 * the I2C bus specification, incremented by 1.
+	 *
+	 * In standard mode, the maximum allowed SCL rise time is 1000 ns.
+	 * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
+	 * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
+	 * programmed with 09h.(1000 ns / 125 ns = 8 + 1)
+	 * So, for I2C standard mode TRISE = FREQ[5:0] + 1
+	 *
+	 * In fast mode, the maximum allowed SCL rise time is 300 ns.
+	 * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
+	 * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
+	 * programmed with 03h.(300 ns / 125 ns = 2 + 1)
+	 * So, for I2C fast mode TRISE = FREQ[5:0] * 300 / 1000 + 1
+	 */
+
+	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
+
+	if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
+		trise = freq + 1;
+	else
+		trise = freq * 300 / 1000 + 1;
+
+	writel_relaxed(STM32F4_I2C_TRISE_VALUE(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 cr2, ccr, freq, val;
+
+	ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
+	ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
+		 STM32F4_I2C_CCR_CCR_MASK);
+
+	/*
+	 * Please see the comments above regarding i2c_timings[] declaration
+	 * to understand the below calculation
+	 */
+	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
+	val = freq * t->scl_period * 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++ = 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_irq(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 buffer interrupts for RXNE/TXE events */
+			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_irq(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 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 events and error interrupts */
+		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);
+		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+		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 set POS
+		 */
+		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 */
+		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 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;
+	}
+
+	status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
+
+	if (!(status & possible_status)) {
+		dev_dbg(i2c_dev->dev,
+			"spurious evt irq (status=0x%08x, ien=0x%08x)\n",
+			status, ien);
+		return IRQ_NONE;
+	}
+
+	while (status & possible_status) {
+		/* Use __fls() to check error bits first */
+		flag = __fls(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 buffer interrupts for RXNE/TXE events */
+			reg = i2c_dev->base + STM32F4_I2C_CR2;
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
+			possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
+			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 irq unhandled: status=0x%08x)\n",
+				status);
+			return IRQ_NONE;
+		}
+		status &= ~(1 << flag);
+	}
+
+	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 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;
+
+	status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
+
+	if (!(status & possible_status)) {
+		dev_dbg(i2c_dev->dev,
+			"spurious err it (status=0x%08x, ien=0x%08x)\n",
+			status, ien);
+		return IRQ_NONE;
+	}
+
+	/* Use __fls() to check error bits first */
+	flag = __fls(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", status);
+		return IRQ_NONE;
+	}
+
+	stm32f4_i2c_soft_reset(i2c_dev);
+	stm32f4_i2c_disable_irq(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 events and errors 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 : num;
+}
+
+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 irq_event, irq_error, 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);
+
+	irq_event = irq_of_parse_and_map(np, 0);
+	if (!irq_event) {
+		dev_err(&pdev->dev, "IRQ event missing or invalid\n");
+		return -EINVAL;
+	}
+
+	irq_error = irq_of_parse_and_map(np, 1);
+	if (!irq_error) {
+		dev_err(&pdev->dev, "IRQ error 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 >= 40000)
+		i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
+
+	i2c_dev->dev = &pdev->dev;
+
+	ret = devm_request_irq(&pdev->dev, irq_event, stm32f4_i2c_isr_event, 0,
+			       pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq event %i\n",
+			irq_event);
+		goto clk_free;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq_error, stm32f4_i2c_isr_error, 0,
+			       pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq error %i\n",
+			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 registered\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 v7 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello,

On Thu, Dec 22, 2016 at 02:35:01PM +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 | 896 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 907 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..ca11dee
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-stm32f4.c
> @@ -0,0 +1,896 @@
> +/*
> + * Driver for STMicroelectronics STM32 I2C controller
> + *
> + * This I2C controller is described in the STM32F429/439 Soc reference manual.
> + * Please see below a link to the documentation:
> + * http://www.st.com/resource/en/reference_manual/DM00031020.pdf
> + *
> + * Copyright (C) M'boumba Cedric Madianga 2016
> + * 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))

		((n) & STM32F4_I2C_CR2_FREQ_MASK)

should be enough.

> +#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))

ditto

> +#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 HZ_TO_MHZ			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
> + * @scl_period: SCL low/high period in microsecond
> + * @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

s/Khz/ kHz/

> + */
> +struct stm32f4_i2c_timings {
> +	u32 duty;
> +	u32 scl_period;
> +	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;

You bought the argument about alignment of = in stm32f4_i2c_driver. The
same logic applies here.

> +};
> +
> +/**
> + * 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
> + * @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;
> +	struct clk			*clk;
> +	int				speed;
> +	struct stm32f4_i2c_msg		msg;
> +};

ditto

> +
> +/*
> + * In standard mode:
> + * SCL high period = SCL low period = CCR * I2C CLK period
> + * So, CCR = SCL period * I2C CLK frequency

is "SCL period" the same as "SCL low period"? I2C CLK frequency is the
input clk? If so, it's confusing that it has I2C in its name. The
reference manual calls it PCLK1 (parent clock?).

> + * In fast mode:
> + * DUTY = 0: Fast mode tlow/thigh = 2
> + * DUTY = 1: Fast mode tlow/thigh = 16/9
> + * If Duty = 0; SCL high period = 1  * CCR * I2C CLK period
> + *		SCL low period  = 2  * CCR * I2C CLK period
> + * If Duty = 1; SCL high period = 9  * CCR * I2C CLK period
> + *		SCL low period  = 16 * CCR * I2C CLK period

I'd drop the first two lines about the proportions.

> + *
> + * Note that Duty has to bet set to reach 400khz in Fast mode

s/khz/ kHz/

I don't understand why DUTY is required to reach 400 kHz. Given a parent
freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:

	t_high = 25 * 33.333 ns = 833.333 ns
	t_low = 2 * 25 * 33.333 ns = 1666.667 ns

then t_high and t_low satisfy the i2c bus specification
(t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
= 1 / 400 kHz.

Where is the error?

> + * So, in order to cover both SCL high/low with Duty = 1,
> + * CCR = 16 * SCL period * I2C CLK frequency

I don't get that. Actually you need to use low + high, so
CCR = parentrate / (25 * 400 kHz), right?

> + *
> + * Please note that the minimum allowed value is 0x04, except in FAST DUTY mode
> + * where the minimum allowed value is 0x01
> + */
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> +	[STM32F4_I2C_SPEED_STANDARD] = {
> +		.mul_ccr		= 1,
> +		.min_ccr		= 4,
> +		.duty			= 0,
> +		.scl_period		= 5,
> +	},
> +	[STM32F4_I2C_SPEED_FAST] = {
> +		.mul_ccr		= 16,
> +		.min_ccr		= 1,
> +		.duty			= 1,
> +		.scl_period		= 2,
> +	},
> +};
> +
> +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;
> +	u32 val;
> +
> +	val = readl_relaxed(reg);
> +	writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
> +	writel_relaxed(val, reg);
> +}
> +
> +static void stm32f4_i2c_disable_irq(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;
> +
> +	/*
> +	 * The minimum allowed frequency is 2 MHz, the maximum frequency is
> +	 * limited by the maximum APB frequency 42 MHz
> +	 */
> +	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> +	cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
> +	clk_rate = clk_get_rate(i2c_dev->clk);
> +	freq = DIV_ROUND_UP(clk_rate, HZ_TO_MHZ);
> +	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);

Last round I suggested error checking here instead of silent clamping.

> +}
> +
> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	u32 trise, freq, cr2;
> +
> +	/*
> +	 * These bits must be programmed with the maximum SCL rise time given in
> +	 * the I2C bus specification, incremented by 1.
> +	 *
> +	 * In standard mode, the maximum allowed SCL rise time is 1000 ns.
> +	 * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
> +	 * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
> +	 * programmed with 09h.(1000 ns / 125 ns = 8 + 1)
> +	 * So, for I2C standard mode TRISE = FREQ[5:0] + 1
> +	 *
> +	 * In fast mode, the maximum allowed SCL rise time is 300 ns.
> +	 * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
> +	 * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
> +	 * programmed with 03h.(300 ns / 125 ns = 2 + 1)
> +	 * So, for I2C fast mode TRISE = FREQ[5:0] * 300 / 1000 + 1
> +	 */
> +
> +	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> +	freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> +
> +	if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
> +		trise = freq + 1;
> +	else
> +		trise = freq * 300 / 1000 + 1;

if freq is big such that freq * 300 overflows does this result in a
wrong result, or does the compiler optimize correctly?

> +	writel_relaxed(STM32F4_I2C_TRISE_VALUE(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 cr2, ccr, freq, val;
> +
> +	ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
> +	ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
> +		 STM32F4_I2C_CCR_CCR_MASK);
> +
> +	/*
> +	 * Please see the comments above regarding i2c_timings[] declaration
> +	 * to understand the below calculation
> +	 */
> +	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> +	freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> +	val = freq * t->scl_period * 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);

This is the first write to STM32F4_I2C_CR1, right? So the state from the
bootloader leaks here. This probably works most of the time, but if it
makes problems later, that's hard to debug. Also, what if the bootloader
already did some i2c transfers and kept the PE bit 1? I read in the
manual that PE must be 0 for some things. So this only works most of the
time.

> +}
> +
> +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");

drop error message please or degrade to dev_debug

> +		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++ = 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_irq(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 buffer interrupts for RXNE/TXE events */
> +			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_irq(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 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 events and error interrupts */
> +		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);
> +		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +		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 set POS
> +		 */
> +		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 */
> +		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;
> +	}
> +}

This is still not really understandable.

> +
> +/**
> + * 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 status, possible_status, ien;
> +	int flag;
> +
> +	ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> +	ien &= STM32F4_I2C_CR2_IRQ_MASK;
> +	possible_status = 0;

This can already be done when declaring possible_status.

> +
> +	/* 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;
> +	}
> +
> +	status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
> +
> +	if (!(status & possible_status)) {
> +		dev_dbg(i2c_dev->dev,
> +			"spurious evt irq (status=0x%08x, ien=0x%08x)\n",
> +			status, ien);
> +		return IRQ_NONE;
> +	}
> +
> +	while (status & possible_status) {
> +		/* Use __fls() to check error bits first */
> +		flag = __fls(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 buffer interrupts for RXNE/TXE events */
> +			reg = i2c_dev->base + STM32F4_I2C_CR2;
> +			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> +			possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
> +			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 irq unhandled: status=0x%08x)\n",
> +				status);
> +			return IRQ_NONE;
> +		}
> +		status &= ~(1 << flag);
> +	}

I wouldn't do this in a loop. Just do:

	if (status & STM32F4_I2C_SR1_SB) {
		...
	}

	if (status & ...) {

	}

Then it's obvious by reading the code in which order they are handled
without the need to check the definitions. Do you really need to jugle
with possible_status?

> +
> +	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 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;
> +
> +	status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
> +
> +	if (!(status & possible_status)) {
> +		dev_dbg(i2c_dev->dev,
> +			"spurious err it (status=0x%08x, ien=0x%08x)\n",
> +			status, ien);
> +		return IRQ_NONE;
> +	}
> +
> +	/* Use __fls() to check error bits first */
> +	flag = __fls(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", status);
> +		return IRQ_NONE;
> +	}

You only check a single irq flag here.

> +
> +	stm32f4_i2c_soft_reset(i2c_dev);
> +	stm32f4_i2c_disable_irq(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 events and errors 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);

This is the only place mentioning PEC. Should this be about POS instead?

> +
> +	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 : num;
> +}
> +
> +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 irq_event, irq_error, 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);
> +
> +	irq_event = irq_of_parse_and_map(np, 0);
> +	if (!irq_event) {
> +		dev_err(&pdev->dev, "IRQ event missing or invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	irq_error = irq_of_parse_and_map(np, 1);
> +	if (!irq_error) {
> +		dev_err(&pdev->dev, "IRQ error 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 >= 40000)
> +		i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
> +
> +	i2c_dev->dev = &pdev->dev;
> +
> +	ret = devm_request_irq(&pdev->dev, irq_event, stm32f4_i2c_isr_event, 0,
> +			       pdev->name, i2c_dev);

Starting here this irq might trigger. Can this happen? If so,
stm32f4_i2c_isr_event is called without the adapter being registered.
Probably not an issue as the controller was just reset.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq event %i\n",
> +			irq_event);
> +		goto clk_free;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq_error, stm32f4_i2c_isr_error, 0,
> +			       pdev->name, i2c_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq error %i\n",
> +			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 registered\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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

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

[M'boumba Cedric Madianga]

Hi Uwe,

Thanks for your comments.
Please see below my answers and one question regarding duty cycle:

2016-12-23 10:00 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Thu, Dec 22, 2016 at 02:35:01PM +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 | 896 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 907 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..ca11dee
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-stm32f4.c
>> @@ -0,0 +1,896 @@
>> +/*
>> + * Driver for STMicroelectronics STM32 I2C controller
>> + *
>> + * This I2C controller is described in the STM32F429/439 Soc reference manual.
>> + * Please see below a link to the documentation:
>> + * http://www.st.com/resource/en/reference_manual/DM00031020.pdf
>> + *
>> + * Copyright (C) M'boumba Cedric Madianga 2016
>> + * 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))
>
>                 ((n) & STM32F4_I2C_CR2_FREQ_MASK)
>
> should be enough.
You are right. I will fix it in the V8.

>
>> +#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))
>
> ditto
ok

>
>> +#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 HZ_TO_MHZ                    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
>> + * @scl_period: SCL low/high period in microsecond
>> + * @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
>
> s/Khz/ kHz/
Good point. Thanks

>
>> + */
>> +struct stm32f4_i2c_timings {
>> +     u32 duty;
>> +     u32 scl_period;
>> +     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;
>
> You bought the argument about alignment of = in stm32f4_i2c_driver. The
> same logic applies here.
ok

>
>> +};
>> +
>> +/**
>> + * 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
>> + * @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;
>> +     struct clk                      *clk;
>> +     int                             speed;
>> +     struct stm32f4_i2c_msg          msg;
>> +};
>
> ditto
ok

>
>> +
>> +/*
>> + * In standard mode:
>> + * SCL high period = SCL low period = CCR * I2C CLK period
>> + * So, CCR = SCL period * I2C CLK frequency
>
> is "SCL period" the same as "SCL low period"? I2C CLK frequency is the
> input clk? If so, it's confusing that it has I2C in its name. The
> reference manual calls it PCLK1 (parent clock?).
SCL high period = SCL low period
I2C CLK frequency = I2C parent clock frequency so I will add this
precision in the V8

>
>> + * In fast mode:
>> + * DUTY = 0: Fast mode tlow/thigh = 2
>> + * DUTY = 1: Fast mode tlow/thigh = 16/9
>> + * If Duty = 0; SCL high period = 1  * CCR * I2C CLK period
>> + *           SCL low period  = 2  * CCR * I2C CLK period
>> + * If Duty = 1; SCL high period = 9  * CCR * I2C CLK period
>> + *           SCL low period  = 16 * CCR * I2C CLK period
>
> I'd drop the first two lines about the proportions
>
>> + *
>> + * Note that Duty has to bet set to reach 400khz in Fast mode
>
> s/khz/ kHz/
Ok. Thanks.

>
> I don't understand why DUTY is required to reach 400 kHz. Given a parent
> freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:
>
>         t_high = 25 * 33.333 ns = 833.333 ns
>         t_low = 2 * 25 * 33.333 ns = 1666.667 ns
>
> then t_high and t_low satisfy the i2c bus specification
> (t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
> = 1 / 400 kHz.
>
> Where is the error?
Hum ok you are right. I was a bad interpretation of the datasheet.
So now it is clearer. Thanks for that.
I will correct and improve my comments in the V8.

>
>> + * So, in order to cover both SCL high/low with Duty = 1,
>> + * CCR = 16 * SCL period * I2C CLK frequency
>
> I don't get that. Actually you need to use low + high, so
> CCR = parentrate / (25 * 400 kHz), right?
With your new inputs above, I think I could use a simpler implementation:
CCR = scl_high_period * parent_rate
with scl_high_period = 5 µs in standard mode to reach 100khz
and scl_high_period = 1 µs in fast mode to reach 400khz with 1/2 or
16/9 duty cycle.
So, I am wondering if I have to let the customer setting the duty
cycle in the DT for example with "st,duty=0" or "st,duty=1" property
(0 for 1/2 and 1 for 16/9).
Or perhaps the best option it to use a default value. What is your
feeling regarding this point ?

>
>> + *
>> + * Please note that the minimum allowed value is 0x04, except in FAST DUTY mode
>> + * where the minimum allowed value is 0x01
>> + */
>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> +     [STM32F4_I2C_SPEED_STANDARD] = {
>> +             .mul_ccr                = 1,
>> +             .min_ccr                = 4,
>> +             .duty                   = 0,
>> +             .scl_period             = 5,
>> +     },
>> +     [STM32F4_I2C_SPEED_FAST] = {
>> +             .mul_ccr                = 16,
>> +             .min_ccr                = 1,
>> +             .duty                   = 1,
>> +             .scl_period             = 2,
>> +     },
>> +};
>> +
>> +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;
>> +     u32 val;
>> +
>> +     val = readl_relaxed(reg);
>> +     writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
>> +     writel_relaxed(val, reg);
>> +}
>> +
>> +static void stm32f4_i2c_disable_irq(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;
>> +
>> +     /*
>> +      * The minimum allowed frequency is 2 MHz, the maximum frequency is
>> +      * limited by the maximum APB frequency 42 MHz
>> +      */
>> +     cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> +     cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
>> +     clk_rate = clk_get_rate(i2c_dev->clk);
>> +     freq = DIV_ROUND_UP(clk_rate, HZ_TO_MHZ);
>> +     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);
>
> Last round I suggested error checking here instead of silent clamping.
Ok

>
>> +}
>> +
>> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     u32 trise, freq, cr2;
>> +
>> +     /*
>> +      * These bits must be programmed with the maximum SCL rise time given in
>> +      * the I2C bus specification, incremented by 1.
>> +      *
>> +      * In standard mode, the maximum allowed SCL rise time is 1000 ns.
>> +      * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
>> +      * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
>> +      * programmed with 09h.(1000 ns / 125 ns = 8 + 1)
>> +      * So, for I2C standard mode TRISE = FREQ[5:0] + 1
>> +      *
>> +      * In fast mode, the maximum allowed SCL rise time is 300 ns.
>> +      * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
>> +      * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
>> +      * programmed with 03h.(300 ns / 125 ns = 2 + 1)
>> +      * So, for I2C fast mode TRISE = FREQ[5:0] * 300 / 1000 + 1
>> +      */
>> +
>> +     cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> +     freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> +
>> +     if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
>> +             trise = freq + 1;
>> +     else
>> +             trise = freq * 300 / 1000 + 1;
>
> if freq is big such that freq * 300 overflows does this result in a
> wrong result, or does the compiler optimize correctly?
For sure the compiler will never exceeds u32 max value

>
>> +     writel_relaxed(STM32F4_I2C_TRISE_VALUE(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 cr2, ccr, freq, val;
>> +
>> +     ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
>> +     ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
>> +              STM32F4_I2C_CCR_CCR_MASK);
>> +
>> +     /*
>> +      * Please see the comments above regarding i2c_timings[] declaration
>> +      * to understand the below calculation
>> +      */
>> +     cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> +     freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> +     val = freq * t->scl_period * 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);
>
> This is the first write to STM32F4_I2C_CR1, right? So the state from the
> bootloader leaks here. This probably works most of the time, but if it
> makes problems later, that's hard to debug. Also, what if the bootloader
> already did some i2c transfers and kept the PE bit 1? I read in the
> manual that PE must be 0 for some things. So this only works most of the
> time.
The first thing I do before configuring I2C device is to clear PE bit
in order to disable I2C.
In that way, all previous config will be erased by the new one.

>
>> +}
>> +
>> +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");
>
> drop error message please or degrade to dev_debug
ok I will use a dev_dbg message

>
>> +             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++ = 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_irq(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 buffer interrupts for RXNE/TXE events */
>> +                     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_irq(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 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 events and error interrupts */
>> +             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);
>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +             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 set POS
>> +              */
>> +             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 */
>> +             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;
>> +     }
>> +}
>
> This is still not really understandable.
I have already added some comments from datasheet to explain the
different cases.
I don't see how I could be more understandable as it is clearly the
hardware way of working...

>
>> +
>> +/**
>> + * 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 status, possible_status, ien;
>> +     int flag;
>> +
>> +     ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> +     ien &= STM32F4_I2C_CR2_IRQ_MASK;
>> +     possible_status = 0;
>
> This can already be done when declaring possible_status.
Ok.

>
>> +
>> +     /* 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;
>> +     }
>> +
>> +     status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
>> +
>> +     if (!(status & possible_status)) {
>> +             dev_dbg(i2c_dev->dev,
>> +                     "spurious evt irq (status=0x%08x, ien=0x%08x)\n",
>> +                     status, ien);
>> +             return IRQ_NONE;
>> +     }
>> +
>> +     while (status & possible_status) {
>> +             /* Use __fls() to check error bits first */
>> +             flag = __fls(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 buffer interrupts for RXNE/TXE events */
>> +                     reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> +                     possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
>> +                     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 irq unhandled: status=0x%08x)\n",
>> +                             status);
>> +                     return IRQ_NONE;
>> +             }
>> +             status &= ~(1 << flag);
>> +     }
>
> I wouldn't do this in a loop. Just do:
>
>         if (status & STM32F4_I2C_SR1_SB) {
>                 ...
>         }
>
>         if (status & ...) {
>
>         }
ok but I would prefer something like that:
flag = status & possible_status
if (flag & STM32F4_I2C_SR1_SB) {
...
}

if (flag & ...) {
}

>
> Then it's obvious by reading the code in which order they are handled
> without the need to check the definitions. Do you really need to jugle
> with possible_status?
I think I have to use possible_status as some events could occur
whereas the corresponding interrupt is disabled.
For example, for a 2 byte-reception, we don't have to take into accout
RXNE event so the corresponding interrupt is disabled.
If I don't check possible_status, I will call
stm32f4_i2c_handle_read() for nothing and generates unneeded registers
accesses.

>
>> +
>> +     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 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;
>> +
>> +     status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
>> +
>> +     if (!(status & possible_status)) {
>> +             dev_dbg(i2c_dev->dev,
>> +                     "spurious err it (status=0x%08x, ien=0x%08x)\n",
>> +                     status, ien);
>> +             return IRQ_NONE;
>> +     }
>> +
>> +     /* Use __fls() to check error bits first */
>> +     flag = __fls(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", status);
>> +             return IRQ_NONE;
>> +     }
>
> You only check a single irq flag here.
Yes only the first error could be reported to the i2c clients via
msg->result that's why I don't check all errors.
Moreover, as soon as an error occurs, the I2C device is reset.

>
>> +
>> +     stm32f4_i2c_soft_reset(i2c_dev);
>> +     stm32f4_i2c_disable_irq(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 events and errors 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);
>
> This is the only place mentioning PEC. Should this be about POS instead?
Yes you are right. Thanks

>
>> +
>> +     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 : num;
>> +}
>> +
>> +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 irq_event, irq_error, 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);
>> +
>> +     irq_event = irq_of_parse_and_map(np, 0);
>> +     if (!irq_event) {
>> +             dev_err(&pdev->dev, "IRQ event missing or invalid\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     irq_error = irq_of_parse_and_map(np, 1);
>> +     if (!irq_error) {
>> +             dev_err(&pdev->dev, "IRQ error 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 >= 40000)
>> +             i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
>> +
>> +     i2c_dev->dev = &pdev->dev;
>> +
>> +     ret = devm_request_irq(&pdev->dev, irq_event, stm32f4_i2c_isr_event, 0,
>> +                            pdev->name, i2c_dev);
>
> Starting here this irq might trigger. Can this happen? If so,
> stm32f4_i2c_isr_event is called without the adapter being registered.
> Probably not an issue as the controller was just reset.
No irq could be triggered as after resett, the I2C is not enable as PE bit = 0.

>
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to request irq event %i\n",
>> +                     irq_event);
>> +             goto clk_free;
>> +     }
>> +
>> +     ret = devm_request_irq(&pdev->dev, irq_error, stm32f4_i2c_isr_error, 0,
>> +                            pdev->name, i2c_dev);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to request irq error %i\n",
>> +                     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 registered\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
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Uwe Kleine-König]

Hello Cedric,

On Fri, Dec 23, 2016 at 01:41:15PM +0100, M'boumba Cedric Madianga wrote:
> > I don't understand why DUTY is required to reach 400 kHz. Given a parent
> > freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:
> >
> >         t_high = 25 * 33.333 ns = 833.333 ns
> >         t_low = 2 * 25 * 33.333 ns = 1666.667 ns
> >
> > then t_high and t_low satisfy the i2c bus specification
> > (t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
> > = 1 / 400 kHz.
> >
> > Where is the error?
> Hum ok you are right. I was a bad interpretation of the datasheet.
> So now it is clearer. Thanks for that.
> I will correct and improve my comments in the V8.

The benefit of DUTY=1 is (I think) that you can reach 400 kHz also with
lower parent frequencies. And so it't probably sensible to make use of
it unconditionally for fast mode.

> >> + * So, in order to cover both SCL high/low with Duty = 1,
> >> + * CCR = 16 * SCL period * I2C CLK frequency
> >
> > I don't get that. Actually you need to use low + high, so
> > CCR = parentrate / (25 * 400 kHz), right?
> With your new inputs above, I think I could use a simpler implementation:
> CCR = scl_high_period * parent_rate
> with scl_high_period = 5 µs in standard mode to reach 100khz
> and scl_high_period = 1 µs in fast mode to reach 400khz with 1/2 or
> 16/9 duty cycle.
> So, I am wondering if I have to let the customer setting the duty
> cycle in the DT for example with "st,duty=0" or "st,duty=1" property
> (0 for 1/2 and 1 for 16/9).
> Or perhaps the best option it to use a default value. What is your
> feeling regarding this point ?

No, don't add a property to the device tree. Just take pencil and paper
and think a while about the right algorithm to choose the right register
settings.


> >> +     cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> >> +     freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> >> +
> >> +     if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
> >> +             trise = freq + 1;
> >> +     else
> >> +             trise = freq * 300 / 1000 + 1;
> >
> > if freq is big such that freq * 300 overflows does this result in a
> > wrong result, or does the compiler optimize correctly?
> For sure the compiler will never exceeds u32 max value

If I compile
-------->8--------
#include <stdio.h>

void myfunc(unsigned freq)
{
	unsigned trise = freq * 300 / 1000 + 1;
	printf("trise = %u", trise);
}

-------->8--------

for arm with -O3 I get:

00000000 <myfunc>:
   0:	e3a01f4b 	mov	r1, #300	; 0x12c
   4:	e0010190 	mul	r1, r0, r1
   8:	e59f3010 	ldr	r3, [pc, #16]	; 20 <myfunc+0x20>
   c:	e59f0010 	ldr	r0, [pc, #16]	; 24 <myfunc+0x24>
  10:	e0812193 	umull	r2, r1, r3, r1
  14:	e1a01321 	lsr	r1, r1, #6
  18:	e2811001 	add	r1, r1, #1
  1c:	eafffffe 	b	0 <printf>
  20:	10624dd3 	.word	0x10624dd3
  24:	00000000 	.word	0x00000000

The mul instruction at offset 4 writes the least significant 32 bits of
300 * r0 to r1 and so doesn't handle overflow correctly.

> > This is still not really understandable.
> I have already added some comments from datasheet to explain the
> different cases.
> I don't see how I could be more understandable as it is clearly the
> hardware way of working...

You need to comment the check for the length, something like:

	/*
	 * To end the transfer correctly the foo bit has to be cleared
	 * already on the last but one byte to be transferred.
	 */

and bonus points if you can shrink the number of functions that check
for this stuff.

> > Just do:
> >
> >         if (status & STM32F4_I2C_SR1_SB) {
> >                 ...
> >         }
> >
> >         if (status & ...) {
> >
> >         }
> ok but I would prefer something like that:
> flag = status & possible_status
> if (flag & STM32F4_I2C_SR1_SB) {
> ...
> }
> 
> if (flag & ...) {
> }

Ok, if you really need possible_status.

> > Then it's obvious by reading the code in which order they are handled
> > without the need to check the definitions. Do you really need to jugle
> > with possible_status?
> I think I have to use possible_status as some events could occur
> whereas the corresponding interrupt is disabled.
> For example, for a 2 byte-reception, we don't have to take into accout
> RXNE event so the corresponding interrupt is disabled.

Is it possible to make it more obvious by doing:

	status = read_from_status_register() & read_from_interrupt_enable_register();

at a single place?

> >> +     /* Use __fls() to check error bits first */
> >> +     flag = __fls(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", status);
> >> +             return IRQ_NONE;
> >> +     }
> >
> > You only check a single irq flag here.
> Yes only the first error could be reported to the i2c clients via
> msg->result that's why I don't check all errors.
> Moreover, as soon as an error occurs, the I2C device is reset.

The the "first" in the comment for __fls is misleading. Also do:

	if (status & MOST_IMPORTANT_ERROR) {
		...
	}

	if (status & SECOND_MOST_IMPORTANT_ERROR) {
		...
	}

(if you need including possible_status stuff).

Best regards
Uwe

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

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

[M'boumba Cedric Madianga]

Hello Uwe,

2016-12-28 22:21 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Cedric,
>
> On Fri, Dec 23, 2016 at 01:41:15PM +0100, M'boumba Cedric Madianga wrote:
>> > I don't understand why DUTY is required to reach 400 kHz. Given a parent
>> > freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:
>> >
>> >         t_high = 25 * 33.333 ns = 833.333 ns
>> >         t_low = 2 * 25 * 33.333 ns = 1666.667 ns
>> >
>> > then t_high and t_low satisfy the i2c bus specification
>> > (t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
>> > = 1 / 400 kHz.
>> >
>> > Where is the error?
>> Hum ok you are right. I was a bad interpretation of the datasheet.
>> So now it is clearer. Thanks for that.
>> I will correct and improve my comments in the V8.
>
> The benefit of DUTY=1 is (I think) that you can reach 400 kHz also with
> lower parent frequencies. And so it't probably sensible to make use of
> it unconditionally for fast mode.
Ok I agree.

>
>> >> + * So, in order to cover both SCL high/low with Duty = 1,
>> >> + * CCR = 16 * SCL period * I2C CLK frequency
>> >
>> > I don't get that. Actually you need to use low + high, so
>> > CCR = parentrate / (25 * 400 kHz), right?
>> With your new inputs above, I think I could use a simpler implementation:
>> CCR = scl_high_period * parent_rate
>> with scl_high_period = 5 µs in standard mode to reach 100khz
>> and scl_high_period = 1 µs in fast mode to reach 400khz with 1/2 or
>> 16/9 duty cycle.
>> So, I am wondering if I have to let the customer setting the duty
>> cycle in the DT for example with "st,duty=0" or "st,duty=1" property
>> (0 for 1/2 and 1 for 16/9).
>> Or perhaps the best option it to use a default value. What is your
>> feeling regarding this point ?
>
> No, don't add a property to the device tree. Just take pencil and paper
> and think a while about the right algorithm to choose the right register
> settings.
Ok thanks

>
>
>> >> +     cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> >> +     freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> >> +
>> >> +     if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
>> >> +             trise = freq + 1;
>> >> +     else
>> >> +             trise = freq * 300 / 1000 + 1;
>> >
>> > if freq is big such that freq * 300 overflows does this result in a
>> > wrong result, or does the compiler optimize correctly?
>> For sure the compiler will never exceeds u32 max value
>
> If I compile
> -------->8--------
> #include <stdio.h>
>
> void myfunc(unsigned freq)
> {
>         unsigned trise = freq * 300 / 1000 + 1;
>         printf("trise = %u", trise);
> }
>
> -------->8--------
>
> for arm with -O3 I get:
>
> 00000000 <myfunc>:
>    0:   e3a01f4b        mov     r1, #300        ; 0x12c
>    4:   e0010190        mul     r1, r0, r1
>    8:   e59f3010        ldr     r3, [pc, #16]   ; 20 <myfunc+0x20>
>    c:   e59f0010        ldr     r0, [pc, #16]   ; 24 <myfunc+0x24>
>   10:   e0812193        umull   r2, r1, r3, r1
>   14:   e1a01321        lsr     r1, r1, #6
>   18:   e2811001        add     r1, r1, #1
>   1c:   eafffffe        b       0 <printf>
>   20:   10624dd3        .word   0x10624dd3
>   24:   00000000        .word   0x00000000
>
> The mul instruction at offset 4 writes the least significant 32 bits of
> 300 * r0 to r1 and so doesn't handle overflow correctly.
In case of overflow, the parent frequency has to be at least at 1431657 Mhz.
The STM32F4 SoC will never reach this value for any parent clock.
So, I think that this point is not really critical and you can
probably keep these simple lines of code without adding u32 overflow
checking for big frequency.

>
>> > This is still not really understandable.
>> I have already added some comments from datasheet to explain the
>> different cases.
>> I don't see how I could be more understandable as it is clearly the
>> hardware way of working...
>
> You need to comment the check for the length, something like:
>
>         /*
>          * To end the transfer correctly the foo bit has to be cleared
>          * already on the last but one byte to be transferred.
>          */
>
OK I will add more comments.

> and bonus points if you can shrink the number of functions that check
> for this stuff.
There are only 2 functions to handle this stuff:  handle_read() for
RXNE event and handle_rx_btf() for BTF event
I would prefer to keep 2 seperate functions to handle each event
according to buffer length as I don't have to do the same thing.
For example:
- for RXNE event with count = 2, I have to disable buffer interrupts
- for BTF event with msg count = 2, I have to .generate stop or
repeated start and disable all remaining interrupts.
I think that if I gather this stuff in one function, the code will be
less understandable.

>
>> > Just do:
>> >
>> >         if (status & STM32F4_I2C_SR1_SB) {
>> >                 ...
>> >         }
>> >
>> >         if (status & ...) {
>> >
>> >         }
>> ok but I would prefer something like that:
>> flag = status & possible_status
>> if (flag & STM32F4_I2C_SR1_SB) {
>> ...
>> }
>>
>> if (flag & ...) {
>> }
>
> Ok, if you really need possible_status.
>
>> > Then it's obvious by reading the code in which order they are handled
>> > without the need to check the definitions. Do you really need to jugle
>> > with possible_status?
>> I think I have to use possible_status as some events could occur
>> whereas the corresponding interrupt is disabled.
>> For example, for a 2 byte-reception, we don't have to take into accout
>> RXNE event so the corresponding interrupt is disabled.
>
> Is it possible to make it more obvious by doing:
>
>         status = read_from_status_register() & read_from_interrupt_enable_register();
>
> at a single place?
Ok I will add these 2 functions in order to only use one status variable.

>
>> >> +     /* Use __fls() to check error bits first */
>> >> +     flag = __fls(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", status);
>> >> +             return IRQ_NONE;
>> >> +     }
>> >
>> > You only check a single irq flag here.
>> Yes only the first error could be reported to the i2c clients via
>> msg->result that's why I don't check all errors.
>> Moreover, as soon as an error occurs, the I2C device is reset.
>
> The the "first" in the comment for __fls is misleading. Also do:
>
>         if (status & MOST_IMPORTANT_ERROR) {
>                 ...
>         }
>
>         if (status & SECOND_MOST_IMPORTANT_ERROR) {
>                 ...
>         }
>
> (if you need including possible_status stuff).
OK

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

Best regards,
Cedric

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

[M'boumba Cedric Madianga]

Hello Uwe,

>> Is it possible to make it more obvious by doing:
>>
>>         status = read_from_status_register() & read_from_interrupt_enable_register();
>>
>> at a single place?
Contrary to what I said previously I have to keep possible_status
variable as for one irq enabled we allow several events.
For example, ITBUFEN allows to generate an irq for RXNE and for TXE events.
So, using status = read_from_status_register() &
read_from_interrupt_enable_register(); is not possible.

Best regards,

Cedric

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

[M'boumba Cedric Madianga]

This patch adds I2C1 support for STM32F429 SoC

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..2277a2d 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 {
@@ -140,6 +141,18 @@
 			status = "disabled";
 		};
 
+		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";
+		};
+
 		usart7: serial@40007800 {
 			compatible = "st,stm32-usart", "st,stm32-uart";
 			reg = <0x40007800 0x400>;
@@ -337,6 +350,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 {
-- 
1.9.1

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

[Uwe Kleine-König]

Hello,

On Thu, Dec 22, 2016 at 02:35:02PM +0100, M'boumba Cedric Madianga wrote:
> @@ -337,6 +350,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>;
> +				};

the second doesn't need the open-drain property? Why?

> +			};
>  		};

Best regards
Uwe

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

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

[M'boumba Cedric Madianga]

Hi,


2016-12-22 20:11 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Thu, Dec 22, 2016 at 02:35:02PM +0100, M'boumba Cedric Madianga wrote:
>> @@ -337,6 +350,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>;
>> +                             };
>
> the second doesn't need the open-drain property? Why?
I thought that open-drain was only needed for SDA line.
But after double-checking I2C specification, it seems that SDA and SCL
lines need open-drain or open-collector to perform the wired-AND
function.
I will do some trials with this config and will fix it in the V8.
Thanks

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

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

[Linus Walleij]

On Fri, Dec 23, 2016 at 2:09 PM, M'boumba Cedric Madianga
<cedric.madianga@gmail.com> wrote:
> 2016-12-22 20:11 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> On Thu, Dec 22, 2016 at 02:35:02PM +0100, M'boumba Cedric Madianga wrote:
>>> @@ -337,6 +350,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>;
>>> +                             };
>>
>> the second doesn't need the open-drain property? Why?
>
> I thought that open-drain was only needed for SDA line.
> But after double-checking I2C specification, it seems that SDA and SCL
> lines need open-drain or open-collector to perform the wired-AND
> function.

I think I2C SDA/SCL must be open drain by definition.

It also requires pull-up resistors, I guess you have these mounted on the board
so you do not need pull-up from the pin controller?

Yours,
Linus Walleij

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

[M'boumba Cedric Madianga]

 Hi Linus,

2016-12-30 10:07 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
> On Fri, Dec 23, 2016 at 2:09 PM, M'boumba Cedric Madianga
> <cedric.madianga@gmail.com> wrote:
>> 2016-12-22 20:11 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>>> On Thu, Dec 22, 2016 at 02:35:02PM +0100, M'boumba Cedric Madianga wrote:
>>>> @@ -337,6 +350,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>;
>>>> +                             };
>>>
>>> the second doesn't need the open-drain property? Why?
>>
>> I thought that open-drain was only needed for SDA line.
>> But after double-checking I2C specification, it seems that SDA and SCL
>> lines need open-drain or open-collector to perform the wired-AND
>> function.
>
> I think I2C SDA/SCL must be open drain by definition.
>
> It also requires pull-up resistors, I guess you have these mounted on the board
> so you do not need pull-up from the pin controller?
Yes, I have 1 pull-up resistor of 1,5K ohm for each line (SDA & SCL)
on the board

>
> Yours,
> Linus Walleij

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

[M'boumba Cedric Madianga]

This patch adds I2C1 instance support for STM32x9I-Eval board.

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

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

[M'boumba Cedric Madianga]

This patch adds I2C support for STM32 default configuration

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