Re: [PATCH v2 3/5] i2c: i2c-stm32f7: add driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hello

On 06/27/2017 10:51 PM, Uwe Kleine-König wrote:
> Hello,
> 
> just a quick review as last act in the night. I don't claim to catch
> everything at this time of the day. So don't be surprised if I still
> find something to criticize after you fixed everything. But keeping this
> until I find time to look again seems wrong to me.
> 
> On Tue, Jun 20, 2017 at 01:41:28PM +0200, Pierre-Yves MORDRET wrote:
>> This patch adds initial support for the STM32F7 I2C controller.
>>
>> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@xxxxxxxxx>
>> Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mordret@xxxxxx>
>> ---
>> Changes in V2:
>>    * Remove st,i2c-timing binding usage
>>    * Implement an I2C timings computation algorithm instead of static
>>      values(bindings). Algorithm uses generic I2C SCL Falling/Rising
>>      bindings and System clock to compute its timings.
>> ---
>>   drivers/i2c/busses/Kconfig       |  10 +
>>   drivers/i2c/busses/Makefile      |   1 +
>>   drivers/i2c/busses/i2c-stm32f7.c | 968 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 979 insertions(+)
>>   create mode 100644 drivers/i2c/busses/i2c-stm32f7.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index ed7106e..a97b987 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -899,6 +899,16 @@ config I2C_STM32F4
>>   	  This driver can also be built as module. If so, the module
>>   	  will be called i2c-stm32f4.
>>   
>> +config I2C_STM32F7
>> +	tristate "STMicroelectronics STM32F7 I2C support"
>> +	depends on ARCH_STM32 || COMPILE_TEST
>> +	help
>> +	  Enable this option to add support for STM32 I2C controller embedded
>> +	  in STM32F7 SoCs.
>> +
>> +	  This driver can also be built as module. If so, the module
>> +	  will be called i2c-stm32f7.
>> +
> 
> I wonder how similar I2C_STM32F4 and I2C_STM32F7 are. There is hardly
> any code sharing between them. Can this be improved?
>

Unfortunatly not. Factorization was tried but IP are rather different to 
be put in a single source file.

>>   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 c89a431..8efa3dd 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -87,6 +87,7 @@ 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_STM32F7)	+= i2c-stm32f7.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-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
>> new file mode 100644
>> index 0000000..9947a9d
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-stm32f7.c
>> @@ -0,0 +1,968 @@
>> +/*
>> + * Driver for STMicroelectronics STM32F7 I2C controller
>> + *
>> + * This I2C controller is described in the STM32F75xxx and STM32F74xxx Soc
>> + * reference manual.
>> + * Please see below a link to the documentation:
>> + * http://www.st.com/resource/en/reference_manual/dm00124865.pdf
>> + *
>> + * Copyright (C) M'boumba Cedric Madianga 2017
>> + * Author: M'boumba Cedric Madianga <cedric.madianga@xxxxxxxxx>
>> + *
>> + * This driver is based on i2c-stm32f4.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.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +#include "i2c-stm32.h"
>> +
>> +/* STM32F7 I2C registers */
>> +#define STM32F7_I2C_CR1				0x00
>> +#define STM32F7_I2C_CR2				0x04
>> +#define STM32F7_I2C_TIMINGR			0x10
>> +#define STM32F7_I2C_ISR				0x18
>> +#define STM32F7_I2C_ICR				0x1C
>> +#define STM32F7_I2C_RXDR			0x24
>> +#define STM32F7_I2C_TXDR			0x28
> 
> puh, this is really different than the f4 variant. Yet another hw
> engineer that likes keeping the sw guys busy :-|
> 

I'm busy

>> +/* STM32F7 I2C control 1 */
>> +#define STM32F7_I2C_CR1_ANFOFF			BIT(12)
>> +#define STM32F7_I2C_CR1_ERRIE			BIT(7)
>> +#define STM32F7_I2C_CR1_TCIE			BIT(6)
>> +#define STM32F7_I2C_CR1_STOPIE			BIT(5)
>> +#define STM32F7_I2C_CR1_NACKIE			BIT(4)
>> +#define STM32F7_I2C_CR1_ADDRIE			BIT(3)
>> +#define STM32F7_I2C_CR1_RXIE			BIT(2)
>> +#define STM32F7_I2C_CR1_TXIE			BIT(1)
>> +#define STM32F7_I2C_CR1_PE			BIT(0)
>> +#define STM32F7_I2C_ALL_IRQ_MASK		(STM32F7_I2C_CR1_ERRIE \
>> +						| STM32F7_I2C_CR1_TCIE \
>> +						| STM32F7_I2C_CR1_STOPIE \
>> +						| STM32F7_I2C_CR1_NACKIE \
>> +						| STM32F7_I2C_CR1_RXIE \
>> +						| STM32F7_I2C_CR1_TXIE)
>> +
>> +/* STM32F7 I2C control 2 */
>> +#define STM32F7_I2C_CR2_RELOAD			BIT(24)
>> +#define STM32F7_I2C_CR2_NBYTES_MASK		GENMASK(23, 16)
>> +#define STM32F7_I2C_CR2_NBYTES(n)		(((n) & 0xff) << 16)
>> +#define STM32F7_I2C_CR2_NACK			BIT(15)
>> +#define STM32F7_I2C_CR2_STOP			BIT(14)
>> +#define STM32F7_I2C_CR2_START			BIT(13)
>> +#define STM32F7_I2C_CR2_RD_WRN			BIT(10)
>> +#define STM32F7_I2C_CR2_SADD7_MASK		GENMASK(7, 1)
>> +#define STM32F7_I2C_CR2_SADD7(n)		(((n) & 0x7f) << 1)
>> +
>> +/* STM32F7 I2C Interrupt Status */
>> +#define STM32F7_I2C_ISR_BUSY			BIT(15)
>> +#define STM32F7_I2C_ISR_ARLO			BIT(9)
>> +#define STM32F7_I2C_ISR_BERR			BIT(8)
>> +#define STM32F7_I2C_ISR_TCR			BIT(7)
>> +#define STM32F7_I2C_ISR_TC			BIT(6)
>> +#define STM32F7_I2C_ISR_STOPF			BIT(5)
>> +#define STM32F7_I2C_ISR_NACKF			BIT(4)
>> +#define STM32F7_I2C_ISR_RXNE			BIT(2)
>> +#define STM32F7_I2C_ISR_TXIS			BIT(1)
>> +
>> +/* STM32F7 I2C Interrupt Clear */
>> +#define STM32F7_I2C_ICR_ARLOCF			BIT(9)
>> +#define STM32F7_I2C_ICR_BERRCF			BIT(8)
>> +#define STM32F7_I2C_ICR_STOPCF			BIT(5)
>> +#define STM32F7_I2C_ICR_NACKCF			BIT(4)
>> +
>> +/* STM32F7 I2C Timing */
>> +#define STM32F7_I2C_TIMINGR_PRESC(n)		(((n) & 0xf) << 28)
>> +#define STM32F7_I2C_TIMINGR_SCLDEL(n)		(((n) & 0xf) << 20)
>> +#define STM32F7_I2C_TIMINGR_SDADEL(n)		(((n) & 0xf) << 16)
>> +#define STM32F7_I2C_TIMINGR_SCLH(n)		(((n) & 0xff) << 8)
>> +#define STM32F7_I2C_TIMINGR_SCLL(n)		((n) & 0xff)
>> +
>> +#define STM32F7_I2C_MAX_LEN			0xff
>> +
>> +#define STM32F7_I2C_DNF_DEFAULT			0
>> +#define STM32F7_I2C_DNF_MAX			16
>> +
>> +#define STM32F7_I2C_ANALOG_FILTER_ENABLE	1
>> +#define STM32F7_I2C_ANALOG_FILTER_DELAY_MIN	50	/* ns */
>> +#define STM32F7_I2C_ANALOG_FILTER_DELAY_MAX	260	/* ns */
>> +
>> +#define STM32F7_I2C_RISE_TIME_DEFAULT		25	/* ns */
>> +#define STM32F7_I2C_FALL_TIME_DEFAULT		10	/* ns */
>> +
>> +#define STM32F7_PRESC_MAX			BIT(4)
>> +#define STM32F7_SCLDEL_MAX			BIT(4)
>> +#define STM32F7_SDADEL_MAX			BIT(4)
>> +#define STM32F7_SCLH_MAX			BIT(8)
>> +#define STM32F7_SCLL_MAX			BIT(8)
>> +
>> +/**
>> + * struct stm32f7_i2c_spec - private i2c specification timing
>> + * @rate: I2C bus speed (Hz)
>> + * @rate_min: 80% of I2C bus speed (Hz)
>> + * @rate_max: 120% of I2C bus speed (Hz)
>> + * @fall_max: Max fall time of both SDA and SCL signals (ns)
>> + * @rise_max: Max rise time of both SDA and SCL signals (ns)
>> + * @hddat_min: Min data hold time (ns)
>> + * @vddat_max: Max data valid time (ns)
>> + * @sudat_min: Min data setup time (ns)
>> + * @l_min: Min low period of the SCL clock (ns)
>> + * @h_min: Min high period of the SCL clock (ns)
>> + */
>> +
> 
> drop this empty line please.
> 

ok

>> +struct stm32f7_i2c_spec {
>> +	u32			rate;
>> +	u32			rate_min;
>> +	u32			rate_max;
>> +	u32			fall_max;
>> +	u32			rise_max;
>> +	u32			hddat_min;
>> +	u32			vddat_max;
>> +	u32			sudat_min;
>> +	u32			l_min;
>> +	u32			h_min;
>> +};
>> +
>> +/**
>> + * struct stm32f7_i2c_setup - private I2C timing setup parameters
>> + * @speed: I2C speed mode (standard, Fast Plus)
>> + * @speed_freq: I2C speed frequency  (Hz)
>> + * @clock_src: I2C clock source frequency (Hz)
>> + * @rise_time: Rise time (ns)
>> + * @fall_time: Fall time (ns)
>> + * @dnf: Digital filter coefficient (0-16)
>> + * @analog_filter: Analog filter delay (On/Off)
>> + */
>> +struct stm32f7_i2c_setup {
>> +	enum stm32_i2c_speed	speed;
>> +	u32			speed_freq;
>> +	u32			clock_src;
>> +	u32			rise_time;
>> +	u32			fall_time;
>> +	u8			dnf;
>> +	bool			analog_filter;
>> +};
>> +
>> +/**
>> + * struct stm32f7_i2c_timings - private I2C output parameters
>> + * @prec: Prescaler value
>> + * @scldel: Data setup time
>> + * @sdadel: Data hold time
>> + * @sclh: SCL high period (master mode)
>> + * @sclh: SCL low period (master mode)
>> + */
>> +struct stm32f7_i2c_timings {
>> +	struct list_head	node;
>> +	u8			presc;
>> +	u8			scldel;
>> +	u8			sdadel;
>> +	u8			sclh;
>> +	u8			scll;
>> +};
>> +
>> +/**
>> + * struct stm32f7_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 stm32f7_i2c_msg {
>> +	u8 addr;
>> +	u32 count;
>> +	u8 *buf;
>> +	int result;
>> +	bool stop;
>> +};
>> +
>> +/**
>> + * struct stm32f7_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, Fast or Fast+
>> + * @msg: Pointer to data to be written
>> + * @msg_num: number of I2C messages to be executed
>> + * @msg_id: message identifiant
>> + * @f7_msg: customized i2c msg for driver usage
>> + * @setup: I2C timing input setup
>> + * @timing: I2C computed timings
>> + */
>> +struct stm32f7_i2c_dev {
>> +	struct i2c_adapter adap;
>> +	struct device *dev;
>> +	void __iomem *base;
>> +	struct completion complete;
>> +	struct clk *clk;
>> +	int speed;
>> +	struct i2c_msg *msg;
>> +	unsigned int msg_num;
>> +	unsigned int msg_id;
>> +	struct stm32f7_i2c_msg f7_msg;
>> +	struct stm32f7_i2c_setup *setup;
>> +	struct stm32f7_i2c_timings timing;
>> +};
>> +
>> +static struct stm32f7_i2c_spec i2c_specs[] = {
>> +	[STM32_I2C_SPEED_STANDARD] = {
>> +		.rate		= 100000,
>> +		.rate_min	= 8000,
>> +		.rate_max	= 120000,
>> +		.fall_max	= 300,
>> +		.rise_max	= 1000,
>> +		.hddat_min	= 0,
>> +		.vddat_max	= 3450,
>> +		.sudat_min	= 250,
>> +		.l_min		= 4700,
>> +		.h_min		= 4000,
>> +	},
>> +	[STM32_I2C_SPEED_FAST] = {
>> +		.rate		= 400000,
>> +		.rate_min	= 320000,
>> +		.rate_max	= 480000,
>> +		.fall_max	= 300,
>> +		.rise_max	= 300,
>> +		.hddat_min	= 0,
>> +		.vddat_max	= 900,
>> +		.sudat_min	= 100,
>> +		.l_min		= 1300,
>> +		.h_min		= 600,
>> +	},
>> +	[STM32_I2C_SPEED_FAST_PLUS] = {
>> +		.rate		= 1000000,
>> +		.rate_min	= 800000,
>> +		.rate_max	= 1200000,
>> +		.fall_max	= 100,
>> +		.rise_max	= 120,
>> +		.hddat_min	= 0,
>> +		.vddat_max	= 450,
>> +		.sudat_min	= 50,
>> +		.l_min		= 500,
>> +		.h_min		= 260,
>> +	},
>> +};
> 
> What is the source of the above table? Maybe mention it in a comment?
> I don't like adding white space to align the =. The next person touching
> this either breaks the alignment or has to modify more than
> syntactically necessary. >

All values come directly out of I2C Specifications. I can mention it 
nonetheless.
Agree for spaces. I will solve this in next revision.

>> +struct stm32f7_i2c_setup stm32f7_setup = {
>> +	.rise_time		= STM32F7_I2C_RISE_TIME_DEFAULT,
>> +	.fall_time		= STM32F7_I2C_FALL_TIME_DEFAULT,
>> +	.dnf			= STM32F7_I2C_DNF_DEFAULT,
>> +	.analog_filter		= STM32F7_I2C_ANALOG_FILTER_ENABLE,
>> +};
>> +
>> +static inline void stm32f7_i2c_set_bits(void __iomem *reg, u32 mask)
>> +{
>> +	writel_relaxed(readl_relaxed(reg) | mask, reg);
>> +}
> 
> this is used three times only. Among these two times
> stm32f7_i2c_hw_config(). I wonder if it would be more robust to assign a
> complete value to the registers there and then drop this function and
> open code it for the last user.

Well this is true with this initial driver. But new features coming 
afterwards are using much more consuming such APIs. This means they will 
come back later on. Moreover as inlined functions I don't see the 
benefice of it.

>> +
>> +static inline void stm32f7_i2c_clr_bits(void __iomem *reg, u32 mask)
>> +{
>> +	writel_relaxed(readl_relaxed(reg) & ~mask, reg);
>> +}
> 
> Two users only, same thoughts as for stm32f7_i2c_set_bit >
>> +
>> +static int stm32f7_i2c_compute_timing(struct stm32f7_i2c_dev *i2c_dev,
>> +				      struct stm32f7_i2c_setup *setup,
>> +				      struct stm32f7_i2c_timings *output)
>> +{
>> +	u32 p_prev = STM32F7_PRESC_MAX;
>> +	u32 i2cclk = DIV_ROUND_CLOSEST(NSEC_PER_SEC,
>> +				       setup->clock_src);
>> +	u32 i2cbus = DIV_ROUND_CLOSEST(NSEC_PER_SEC,
>> +				       setup->speed_freq);
>> +	u32 clk_error_prev = i2cbus;
>> +	u32 tsync;
>> +	u32 af_delay_min, af_delay_max;
>> +	u32 dnf_delay;
>> +	u32 clk_min, clk_max;
>> +	int sdadel_min, sdadel_max;
>> +	int scldel_min;
>> +	struct stm32f7_i2c_timings *v, *_v, *s;
>> +	struct list_head solutions;
>> +	u16 p, l, a, h;
>> +	int ret = 0;
>> +
>> +	if (setup->speed >= STM32_I2C_SPEED_END) {
>> +		dev_err(i2c_dev->dev, "speed out of bound {%d/%d}\n",
>> +			setup->speed, STM32_I2C_SPEED_END - 1);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if ((setup->rise_time > i2c_specs[setup->speed].rise_max) ||
>> +	    (setup->fall_time > i2c_specs[setup->speed].fall_max)) {
>> +		dev_err(i2c_dev->dev,
>> +			"timings out of bound Rise{%d>%d}/Fall{%d>%d}\n",
>> +			setup->rise_time, i2c_specs[setup->speed].rise_max,
>> +			setup->fall_time, i2c_specs[setup->speed].fall_max);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (setup->dnf > STM32F7_I2C_DNF_MAX) {
>> +		dev_err(i2c_dev->dev,
>> +			"DNF out of bound %d/%d\n",
>> +			setup->dnf, STM32F7_I2C_DNF_MAX);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (setup->speed_freq > i2c_specs[setup->speed].rate) {
>> +		dev_err(i2c_dev->dev, "ERROR: Freq {%d/%d}\n",
>> +			setup->speed_freq, i2c_specs[setup->speed].rate);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*  Analog and Digital Filters */
>> +	af_delay_min =
>> +		(setup->analog_filter ?
>> +		 STM32F7_I2C_ANALOG_FILTER_DELAY_MIN : 0);
>> +	af_delay_max =
>> +		(setup->analog_filter ?
>> +		 STM32F7_I2C_ANALOG_FILTER_DELAY_MAX : 0);
>> +	dnf_delay = setup->dnf * i2cclk;
>> +
>> +	sdadel_min = setup->fall_time - i2c_specs[setup->speed].hddat_min -
>> +		af_delay_min - (setup->dnf + 3) * i2cclk;
>> +
>> +	sdadel_max = i2c_specs[setup->speed].vddat_max - setup->rise_time -
>> +		af_delay_max - (setup->dnf + 4) * i2cclk;
>> +
>> +	scldel_min = setup->rise_time + i2c_specs[setup->speed].sudat_min;
>> +
>> +	if (sdadel_min < 0)
>> +		sdadel_min = 0;
>> +	if (sdadel_max < 0)
>> +		sdadel_max = 0;
>> +
>> +	dev_dbg(i2c_dev->dev, "SDADEL(min/max): %i/%i, SCLDEL(Min): %i\n",
>> +		sdadel_min, sdadel_max, scldel_min);
>> +
>> +	INIT_LIST_HEAD(&solutions);
>> +	/* Compute possible values for PRESC, SCLDEL and SDADEL */
>> +	for (p = 0; p < STM32F7_PRESC_MAX; p++) {
>> +		for (l = 0; l < STM32F7_SCLDEL_MAX; l++) {
>> +			u32 scldel = (l + 1) * (p + 1) * i2cclk;
>> +
>> +			if (scldel < scldel_min)
>> +				continue;
>> +
>> +			for (a = 0; a < STM32F7_SDADEL_MAX; a++) {
>> +				u32 sdadel = (a * (p + 1) + 1) * i2cclk;
>> +
>> +				if (((sdadel >= sdadel_min) &&
>> +				     (sdadel <= sdadel_max)) &&
>> +				    (p != p_prev)) {
>> +					v = kmalloc(sizeof(*v), GFP_KERNEL);
>> +					if (!v) {
>> +						ret = -ENOMEM;
>> +						goto exit;
>> +					}
>> +
>> +					v->presc = p;
>> +					v->scldel = l;
>> +					v->sdadel = a;
>> +					p_prev = p;
>> +
>> +					list_add_tail(&v->node,
>> +						      &solutions);
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	if (list_empty(&solutions)) {
>> +		dev_err(i2c_dev->dev, "no Prescaler solution\n");
>> +		ret = -EPERM;
>> +		goto exit;
>> +	}
>> +
>> +	tsync = af_delay_min + dnf_delay + (2 * i2cclk);
>> +	s = NULL;
>> +	clk_max = NSEC_PER_SEC / i2c_specs[setup->speed].rate_min;
>> +	clk_min = NSEC_PER_SEC / i2c_specs[setup->speed].rate_max;
>> +
>> +	/*
>> +	 * Among Prescaler possibilities discovered above figures out SCL Low
>> +	 * and High Period. Provided:
>> +	 * - SCL Low Period has to be higher than Low Period of tehs SCL Clock
>> +	 *   defined by I2C Specification. I2C Clock has to be lower than
>> +	 *   (SCL Low Period - Analog/Digital filters) / 4.
>> +	 * - SCL High Period has to be lower than High Period of the SCL Clock
>> +	 *   defined by I2C Specification
>> +	 * - I2C Clock has to be lower than SCL High Period
>> +	 */
>> +	list_for_each_entry(v, &solutions, node) {
>> +		u32 prescaler = (v->presc + 1) * i2cclk;
>> +
>> +		for (l = 0; l < STM32F7_SCLL_MAX; l++) {
>> +			u32 tscl_l = (l + 1) * prescaler + tsync;
>> +
>> +			if ((tscl_l < i2c_specs[setup->speed].l_min) ||
>> +			    (i2cclk >=
>> +			     ((tscl_l - af_delay_min - dnf_delay) / 4))) {
>> +				continue;
>> +			}
>> +
>> +			for (h = 0; h < STM32F7_SCLH_MAX; h++) {
>> +				u32 tscl_h = (h + 1) * prescaler + tsync;
>> +				u32 tscl = tscl_l + tscl_h +
>> +					setup->rise_time + setup->fall_time;
>> +
>> +				if ((tscl >= clk_min) && (tscl <= clk_max) &&
>> +				    (tscl_h >= i2c_specs[setup->speed].h_min) &&
>> +				    (i2cclk < tscl_h)) {
>> +					int clk_error = tscl - i2cbus;
>> +
>> +					if (clk_error < 0)
>> +						clk_error = -clk_error;
>> +
>> +					if (clk_error < clk_error_prev) {
>> +						clk_error_prev = clk_error;
>> +						v->scll = l;
>> +						v->sclh = h;
>> +						s = v;
>> +					}
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	if (!s) {
>> +		dev_err(i2c_dev->dev, "no solution at all\n");
>> +		ret = -EPERM;
>> +		goto exit;
>> +	}
>> +
>> +	output->presc = s->presc;
>> +	output->scldel = s->scldel;
>> +	output->sdadel = s->sdadel;
>> +	output->scll = s->scll;
>> +	output->sclh = s->sclh;
>> +
>> +	dev_dbg(i2c_dev->dev,
>> +		"Presc: %i, scldel: %i, sdadel: %i, scll: %i, sclh: %i\n",
>> +		output->presc,
>> +		output->scldel, output->sdadel,
>> +		output->scll, output->sclh);
>> +
>> +exit:
>> +	/* Release list and memory */
>> +	list_for_each_entry_safe(v, _v, &solutions, node) {
>> +		list_del(&v->node);
>> +		kfree(v);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32f7_i2c_setup_timing(struct stm32f7_i2c_dev *i2c_dev,
>> +				    struct stm32f7_i2c_setup *setup)
>> +{
>> +	int ret = 0;
>> +
>> +	setup->speed = i2c_dev->speed;
>> +	setup->speed_freq = i2c_specs[setup->speed].rate;
>> +	setup->clock_src = clk_get_rate(i2c_dev->clk);
>> +
>> +	if (!setup->clock_src) {
>> +		dev_err(i2c_dev->dev, "clock rate is 0\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	do {
>> +		ret = stm32f7_i2c_compute_timing(i2c_dev, setup,
>> +						 &i2c_dev->timing);
>> +		if (ret) {
>> +			dev_err(i2c_dev->dev,
>> +				"failed to compute I2C timings.\n");
>> +			if (i2c_dev->speed > STM32_I2C_SPEED_STANDARD) {
>> +				i2c_dev->speed--;
>> +				setup->speed = i2c_dev->speed;
>> +				setup->speed_freq =
>> +					i2c_specs[setup->speed].rate;
>> +				dev_warn(i2c_dev->dev,
>> +					 "downgrade I2C Speed Freq to (%i)\n",
>> +					 i2c_specs[setup->speed].rate);
>> +			} else {
>> +				break;
>> +			}
>> +		}
>> +	} while (ret);
>> +
>> +	if (ret) {
>> +		dev_err(i2c_dev->dev, "Impossible to compute I2C timings.\n");
>> +		return ret;
>> +	}
>> +
>> +	dev_dbg(i2c_dev->dev, "I2C Speed(%i), Freq(%i), Clk Source(%i)\n",
>> +		setup->speed, setup->speed_freq, setup->clock_src);
>> +	dev_dbg(i2c_dev->dev, "I2C Rise(%i) and Fall(%i) Time\n",
>> +		setup->rise_time, setup->fall_time);
>> +	dev_dbg(i2c_dev->dev, "I2C Analog Filter(%s), DNF(%i)\n",
>> +		(setup->analog_filter ? "On" : "Off"), setup->dnf);
>> +
>> +	i2c_dev->setup = setup;
>> +
>> +	return 0;
>> +}
>> +
>> +static void stm32f7_i2c_hw_config(struct stm32f7_i2c_dev *i2c_dev)
>> +{
>> +	struct stm32f7_i2c_timings *t = &i2c_dev->timing;
>> +	u32 timing = 0;
>> +
>> +	/* Timing settings */
>> +	timing |= STM32F7_I2C_TIMINGR_PRESC(t->presc);
>> +	timing |= STM32F7_I2C_TIMINGR_SCLDEL(t->scldel);
>> +	timing |= STM32F7_I2C_TIMINGR_SDADEL(t->sdadel);
>> +	timing |= STM32F7_I2C_TIMINGR_SCLH(t->sclh);
>> +	timing |= STM32F7_I2C_TIMINGR_SCLL(t->scll);
>> +	writel_relaxed(timing, i2c_dev->base + STM32F7_I2C_TIMINGR);
>> +
>> +	/* Enable I2C */
>> +	if (i2c_dev->setup->analog_filter)
>> +		stm32f7_i2c_clr_bits(i2c_dev->base + STM32F7_I2C_CR1,
>> +				     STM32F7_I2C_CR1_ANFOFF);
>> +	else
>> +		stm32f7_i2c_set_bits(i2c_dev->base + STM32F7_I2C_CR1,
>> +				     STM32F7_I2C_CR1_ANFOFF);
>> +	stm32f7_i2c_set_bits(i2c_dev->base + STM32F7_I2C_CR1,
>> +			     STM32F7_I2C_CR1_PE);
>> +}
>> +
>> +static void stm32f7_i2c_write_tx_data(struct stm32f7_i2c_dev *i2c_dev)
>> +{
>> +	struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
>> +	void __iomem *base = i2c_dev->base;
>> +
>> +	if (f7_msg->count) {
>> +		writeb_relaxed(*f7_msg->buf++, base + STM32F7_I2C_TXDR);
>> +		f7_msg->count--;
>> +	}
>> +}
>> +
>> +static void stm32f7_i2c_read_rx_data(struct stm32f7_i2c_dev *i2c_dev)
>> +{
>> +	struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
>> +	void __iomem *base = i2c_dev->base;
>> +
>> +	if (f7_msg->count) {
>> +		*f7_msg->buf++ = readb_relaxed(base + STM32F7_I2C_RXDR);
>> +		f7_msg->count--;
>> +	}
>> +}
>> +
>> +static void stm32f7_i2c_reload(struct stm32f7_i2c_dev *i2c_dev)
>> +{
>> +	struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
>> +	u32 cr2;
>> +
>> +	cr2 = readl_relaxed(i2c_dev->base + STM32F7_I2C_CR2);
>> +
>> +	cr2 &= ~STM32F7_I2C_CR2_NBYTES_MASK;
>> +	if (f7_msg->count > STM32F7_I2C_MAX_LEN) {
>> +		cr2 |= STM32F7_I2C_CR2_NBYTES(STM32F7_I2C_MAX_LEN);
>> +	} else {
>> +		cr2 &= ~STM32F7_I2C_CR2_RELOAD;
>> +		cr2 |= STM32F7_I2C_CR2_NBYTES(f7_msg->count);
>> +	}
>> +
>> +	writel_relaxed(cr2, i2c_dev->base + STM32F7_I2C_CR2);
>> +}
>> +
>> +static int stm32f7_i2c_wait_free_bus(struct stm32f7_i2c_dev *i2c_dev)
>> +{
>> +	u32 status;
>> +	int ret;
>> +
>> +	ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F7_I2C_ISR,
>> +					 status,
>> +					 !(status & STM32F7_I2C_ISR_BUSY),
>> +					 10, 1000);
>> +	if (ret) {
>> +		dev_dbg(i2c_dev->dev, "bus busy\n");
>> +		ret = -EBUSY;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
>> +				 struct i2c_msg *msg)
>> +{
>> +	struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
>> +	void __iomem *base = i2c_dev->base;
>> +	u32 cr1, cr2;
>> +
>> +	f7_msg->addr = msg->addr;
>> +	f7_msg->buf = msg->buf;
>> +	f7_msg->count = msg->len;
>> +	f7_msg->result = 0;
>> +	f7_msg->stop = (i2c_dev->msg_id >= i2c_dev->msg_num - 1);
>> +
>> +	reinit_completion(&i2c_dev->complete);
>> +
>> +	cr1 = readl_relaxed(base + STM32F7_I2C_CR1);
>> +	cr2 = readl_relaxed(base + STM32F7_I2C_CR2);
> 
> Can you know the content of these registers from the context?
> 

Not entirely some infos are stored within the context but some others 
like IT masking aren't. We can't guess the content if this is the question.

>> +	/* Set transfer direction */
>> +	cr2 &= ~STM32F7_I2C_CR2_RD_WRN;
>> +	if (msg->flags & I2C_M_RD)
>> +		cr2 |= STM32F7_I2C_CR2_RD_WRN;
>> +
>> +	/* Set slave address */
>> +	cr2 &= ~STM32F7_I2C_CR2_SADD7_MASK;
>> +	cr2 |= STM32F7_I2C_CR2_SADD7(f7_msg->addr);
>> +
>> +	/* Set nb bytes to transfer and reload if needed */
>> +	cr2 &= ~(STM32F7_I2C_CR2_NBYTES_MASK | STM32F7_I2C_CR2_RELOAD);
>> +	if (f7_msg->count > STM32F7_I2C_MAX_LEN) {
>> +		cr2 |= STM32F7_I2C_CR2_NBYTES(STM32F7_I2C_MAX_LEN);
>> +		cr2 |= STM32F7_I2C_CR2_RELOAD;
>> +	} else {
>> +		cr2 |= STM32F7_I2C_CR2_NBYTES(f7_msg->count);
>> +	}
>> +
>> +	/* Enable NACK, STOP, error and transfer complete interrupts */
>> +	cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
>> +		STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
>> +
>> +	/* Clear TX/RX interrupt */
>> +	cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE);
>> +
>> +	/* Enable RX/TX interrupt according to msg direction */
>> +	if (msg->flags & I2C_M_RD)
>> +		cr1 |= STM32F7_I2C_CR1_RXIE;
>> +	else
>> +		cr1 |= STM32F7_I2C_CR1_TXIE;
>> +
>> +	/* Configure Start/Repeated Start */
>> +	cr2 |= STM32F7_I2C_CR2_START;
>> +
>> +	/* Write configurations registers */
>> +	writel_relaxed(cr1, base + STM32F7_I2C_CR1);
>> +	writel_relaxed(cr2, base + STM32F7_I2C_CR2);
>> +}
>> +
>> +static void stm32f7_i2c_disable_irq(struct stm32f7_i2c_dev *i2c_dev, u32 mask)
>> +{
>> +	stm32f7_i2c_clr_bits(i2c_dev->base + STM32F7_I2C_CR1, mask);
>> +}
> 
> mask is always STM32F7_I2C_ALL_IRQ_MASK. Maybe use this to simplify?
> 

Yes right. However same answer as set/clr_bits APIs: Some features will 
require different masking and I will come back with the same APIs later on.

>> +static irqreturn_t stm32f7_i2c_isr_event(int irq, void *data)
>> +{
>> +	struct stm32f7_i2c_dev *i2c_dev = data;
>> +	struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
>> +	void __iomem *base = i2c_dev->base;
>> +	u32 status, mask;
>> +
>> +	status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
>> +
>> +	/* Tx empty */
>> +	if (status & STM32F7_I2C_ISR_TXIS)
>> +		stm32f7_i2c_write_tx_data(i2c_dev);
>> +
>> +	/* RX not empty */
>> +	if (status & STM32F7_I2C_ISR_RXNE)
>> +		stm32f7_i2c_read_rx_data(i2c_dev);
>> +
>> +	/* NACK received */
>> +	if (status & STM32F7_I2C_ISR_NACKF) {
>> +		dev_dbg(i2c_dev->dev, "<%s>: Receive NACK\n", __func__);
>> +		writel_relaxed(STM32F7_I2C_ICR_NACKCF, base + STM32F7_I2C_ICR);
>> +		f7_msg->result = -EBADE;
>> +	}
>> +
>> +	/* STOP detection flag */
>> +	if (status & STM32F7_I2C_ISR_STOPF) {
>> +		/* Disable interrupts */
>> +		stm32f7_i2c_disable_irq(i2c_dev, STM32F7_I2C_ALL_IRQ_MASK);
>> +
>> +		/* Clear STOP flag */
>> +		writel_relaxed(STM32F7_I2C_ICR_STOPCF, base + STM32F7_I2C_ICR);
>> +
>> +		complete(&i2c_dev->complete);
>> +	}
>> +
>> +	/* Transfer complete */
>> +	if (status & STM32F7_I2C_ISR_TC) {
>> +		if (f7_msg->stop) {
>> +			mask = STM32F7_I2C_CR2_STOP;
>> +			stm32f7_i2c_set_bits(base + STM32F7_I2C_CR2, mask);
>> +		} else {
>> +			i2c_dev->msg_id++;
>> +			i2c_dev->msg++;
>> +			stm32f7_i2c_xfer_msg(i2c_dev, i2c_dev->msg);
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * Transfer Complete Reload: 255 data bytes have been transferred
>> +	 * We have to prepare the I2C controller to transfer the remaining
>> +	 * data
> 
> Missing punctuation.
> 

Ok

>> +	 */
>> +	if (status & STM32F7_I2C_ISR_TCR)
>> +		stm32f7_i2c_reload(i2c_dev);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
>> +{
>> +	struct stm32f7_i2c_dev *i2c_dev = data;
>> +	struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
>> +	void __iomem *base = i2c_dev->base;
>> +	struct device *dev = i2c_dev->dev;
>> +	u32 status;
>> +
>> +	status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
>> +
>> +	/* Bus error */
>> +	if (status & STM32F7_I2C_ISR_BERR) {
>> +		dev_err(dev, "<%s>: Bus error\n", __func__);
>> +		writel_relaxed(STM32F7_I2C_ICR_BERRCF, base + STM32F7_I2C_ICR);
>> +		f7_msg->result = -EIO;
>> +	}
>> +
>> +	/* Arbitration loss */
>> +	if (status & STM32F7_I2C_ISR_ARLO) {
>> +		dev_err(dev, "<%s>: Arbitration loss\n", __func__);
> 
> Drop this. Arbitration loss is not an error and it shouldn't pollute the
> kernel log.
> 

I would like to keep it as indication though.
Agree with dev_warn ?

>> +		writel_relaxed(STM32F7_I2C_ICR_ARLOCF, base + STM32F7_I2C_ICR);
>> +		f7_msg->result = -EAGAIN;
>> +	}
>> +
>> +	stm32f7_i2c_disable_irq(i2c_dev, STM32F7_I2C_ALL_IRQ_MASK);
>> +
>> +	complete(&i2c_dev->complete);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
>> +			    struct i2c_msg msgs[], int num)
>> +{
>> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>> +	struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
>> +	unsigned long timeout;
>> +	int ret;
>> +
>> +	i2c_dev->msg = msgs;
>> +	i2c_dev->msg_num = num;
>> +	i2c_dev->msg_id = 0;
>> +
>> +	ret = clk_enable(i2c_dev->clk);
>> +	if (ret) {
>> +		dev_err(i2c_dev->dev, "Failed to enable clock\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = stm32f7_i2c_wait_free_bus(i2c_dev);
>> +	if (ret)
>> +		goto clk_free;
>> +
>> +	stm32f7_i2c_xfer_msg(i2c_dev, msgs);
>> +
>> +	timeout = wait_for_completion_timeout(&i2c_dev->complete,
>> +					      i2c_dev->adap.timeout);
>> +	ret = f7_msg->result;
>> +
>> +	if (!timeout) {
>> +		dev_dbg(i2c_dev->dev, "Access to slave 0x%x timed out\n",
>> +			i2c_dev->msg->addr);
>> +		ret = -ETIMEDOUT;
>> +	}
>> +
>> +clk_free:
>> +	clk_disable(i2c_dev->clk);
>> +
>> +	return (ret < 0) ? ret : num;
>> +}
>> +
>> +static u32 stm32f7_i2c_func(struct i2c_adapter *adap)
>> +{
>> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>> +}
>> +
>> +static struct i2c_algorithm stm32f7_i2c_algo = {
>> +	.master_xfer = stm32f7_i2c_xfer,
>> +	.functionality = stm32f7_i2c_func,
>> +};
>> +
>> +static const struct of_device_id stm32f7_i2c_match[] = {
>> +	{ .compatible = "st,stm32f7-i2c", .data = &stm32f7_setup},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32f7_i2c_match);
> 
> I thought the naming to be uncommon and wanted to suggest _ids as
> suffix. But grep proves me wrong here. I'd like to see this further
> down, just above stm32f7_i2c_driver.
> 

ok

>> +static int stm32f7_i2c_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	const struct of_device_id *match;
>> +	struct stm32f7_i2c_dev *i2c_dev;
>> +	struct stm32f7_i2c_setup *setup;
>> +	struct resource *res;
>> +	u32 irq_error, irq_event, clk_rate, rise_time, fall_time;
>> +	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_enable(i2c_dev->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to prepare_enable 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 = STM32_I2C_SPEED_STANDARD;
>> +	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
>> +	if ((!ret) && (clk_rate == 400000))
>> +		i2c_dev->speed = STM32_I2C_SPEED_FAST;
>> +	else if ((!ret) && (clk_rate == 1000000))
>> +		i2c_dev->speed = STM32_I2C_SPEED_FAST_PLUS;
> 
> Did you use an early patch of stm32f4 as cut-n-paste source? Better use:
> 
> 	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
> 	if (!ret && clk_rate >= 1000000) {
> 		i2c_dev->speed = STM32_I2C_SPEED_FAST_PLUS;
> 	} else if (!ret && clk_rate >= 400000) {
> 		i2c_dev->speed = STM32_I2C_SPEED_FAST;
> 	} else if (!ret && clk_rate >= 100000)
> 		i2c_dev->speed = STM32_I2C_SPEED_STANDARD;
> 	else {
> 		dev_err(..., "Unsupported clock speed\n");
> 		return -EINVAL;
> 	}
> 
> and I suggest to do this before touching the hardware, i.e. (at least)
> above reset_control_assert().
> 

Agree.

>> +	i2c_dev->dev = &pdev->dev;
>> +
>> +	ret = devm_request_irq(&pdev->dev, irq_event, stm32f7_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, stm32f7_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;
>> +	}
>> +
>> +	match = of_match_device(stm32f7_i2c_match, &pdev->dev);
>> +	if (match && match->data)
>> +		setup = (struct stm32f7_i2c_setup *)match->data;
>> +	else
>> +		goto clk_free;
> 
> Why do you use this construct if there is only a single compatible that
> is supported? This is open coding of_device_get_match_data, an
> additional benefit of using this one is that you don't need
> stm32f7_i2c_match here which is a precondition to move this down as
> suggested above.
> 

Ok. I agree.

>> +
>> +	ret = device_property_read_u32(i2c_dev->dev, "i2c-scl-rising-time-ns",
>> +				       &rise_time);
>> +	if (!ret)
>> +		setup->rise_time = rise_time;
>> +
>> +	ret = device_property_read_u32(i2c_dev->dev, "i2c-scl-falling-time-ns",
>> +				       &fall_time);
>> +	if (!ret)
>> +		setup->fall_time = fall_time;
>> +
>> +	ret = stm32f7_i2c_setup_timing(i2c_dev, setup);
>> +	if (ret)
>> +		goto clk_free;
>> +
>> +	stm32f7_i2c_hw_config(i2c_dev);
>> +
>> +	adap = &i2c_dev->adap;
>> +	i2c_set_adapdata(adap, i2c_dev);
>> +	snprintf(adap->name, sizeof(adap->name), "STM32F7 I2C(%pa)",
>> +		 &res->start);
>> +	adap->owner = THIS_MODULE;
>> +	adap->timeout = 2 * HZ;
>> +	adap->retries = 0;
>> +	adap->algo = &stm32f7_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) {
>> +		dev_err(&pdev->dev, "Failed to add adapter\n");
>> +		goto clk_free;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, i2c_dev);
> 
> I'd do this earlier. There is no real problem that is fixed, as the only
> user is stm32f7_i2c_remove that is only called when stm32f7_i2c_probe
> returns success, but it still feels better. Might be subjective, so
> ignore at will.
> 

ok

>> +	clk_disable(i2c_dev->clk);
>> +
>> +	dev_info(i2c_dev->dev, "STM32F7 I2C-%d driver registered\n", adap->nr);
> 
> This is wrong. The driver is already registered via
> 
> 	module_platform_driver(stm32f7_i2c_driver);
> 
> below. What you want to claim here is, that the driver bound a device.
> 

I will rephrase then.

>> +
>> +	return 0;
>> +
>> +clk_free:
>> +	clk_disable_unprepare(i2c_dev->clk);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32f7_i2c_remove(struct platform_device *pdev)
>> +{
>> +	struct stm32f7_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
>> +
>> +	i2c_del_adapter(&i2c_dev->adap);
>> +
>> +	clk_unprepare(i2c_dev->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver stm32f7_i2c_driver = {
>> +	.driver = {
>> +		.name = "stm32f7-i2c",
>> +		.of_match_table = stm32f7_i2c_match,
>> +	},
>> +	.probe = stm32f7_i2c_probe,
>> +	.remove = stm32f7_i2c_remove,
>> +};
>> +
>> +module_platform_driver(stm32f7_i2c_driver);
>> +
>> +MODULE_AUTHOR("M'boumba Cedric Madianga <cedric.madianga@xxxxxxxxx>");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32F7 I2C driver");
>> +MODULE_LICENSE("GPL v2");
> 

Eager to read your next remarks :)��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux