[PATCH v3 1/6] i2c: core: Provide generic definitions for bus frequencies

[PATCH v3 1/6] i2c: core: Provide generic definitions for bus frequencies

[Andy Shevchenko]

There are few maximum bus frequencies being used in the I²C core code.
Provide generic definitions for bus frequencies and use them in the core.

The drivers may use predefined constants where it is appropriate.
Some of them are already using these under slightly different names.
We will convert them later to use newly introduced defines.

Note, the name of modes are chosen to follow well established naming
scheme [1].

These definitions will also help to avoid typos in the numbers that
may lead to subtle errors.

[1]: https://en.wikipedia.org/wiki/I%C2%B2C#Differences_between_modes

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: added link to Wiki to point the mode naming scheme out
 drivers/i2c/i2c-core-acpi.c | 2 +-
 drivers/i2c/i2c-core-base.c | 8 ++++----
 include/linux/i2c.h         | 8 ++++++++
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 8b0ff780919b..c8f42f2037cb 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -318,7 +318,7 @@ static acpi_status i2c_acpi_lookup_speed(acpi_handle handle, u32 level,
 		lookup->min_speed = lookup->speed;
 
 	if (acpi_match_device_ids(adev, i2c_acpi_force_400khz_device_ids) == 0)
-		lookup->force_speed = 400000;
+		lookup->force_speed = I2C_MAX_FAST_MODE_FREQ;
 
 	return AE_OK;
 }
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index cefad0881942..9b2972c7faa2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1612,13 +1612,13 @@ void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_de
 
 	ret = device_property_read_u32(dev, "clock-frequency", &t->bus_freq_hz);
 	if (ret && use_defaults)
-		t->bus_freq_hz = 100000;
+		t->bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ;
 
 	ret = device_property_read_u32(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns);
 	if (ret && use_defaults) {
-		if (t->bus_freq_hz <= 100000)
+		if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
 			t->scl_rise_ns = 1000;
-		else if (t->bus_freq_hz <= 400000)
+		else if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
 			t->scl_rise_ns = 300;
 		else
 			t->scl_rise_ns = 120;
@@ -1626,7 +1626,7 @@ void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_de
 
 	ret = device_property_read_u32(dev, "i2c-scl-falling-time-ns", &t->scl_fall_ns);
 	if (ret && use_defaults) {
-		if (t->bus_freq_hz <= 400000)
+		if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
 			t->scl_fall_ns = 300;
 		else
 			t->scl_fall_ns = 120;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f834687989f7..72e759328cee 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -39,6 +39,14 @@ enum i2c_slave_event;
 typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
 			      enum i2c_slave_event event, u8 *val);
 
+/* I2C Frequency Modes */
+#define I2C_MAX_STANDARD_MODE_FREQ	100000
+#define I2C_MAX_FAST_MODE_FREQ		400000
+#define I2C_MAX_FAST_MODE_PLUS_FREQ	1000000
+#define I2C_MAX_TURBO_MODE_FREQ		1400000
+#define I2C_MAX_HIGH_SPEED_MODE_FREQ	3400000
+#define I2C_MAX_ULTRA_FAST_MODE_FREQ	5000000
+
 struct module;
 struct property_entry;
 
-- 
2.25.1

[PATCH v3 2/6] i2c: core: Allow override timing properties with 0

[Andy Shevchenko]

Some drivers may allow to override properties with 0 value when defaults
are not in use, thus, replace memset() with corresponding per property
update.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: new patch
 drivers/i2c/busses/i2c-rcar.c |  2 +-
 drivers/i2c/i2c-core-base.c   | 30 +++++++++++++++++-------------
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 879f0e61a496..f1f915bfdb9d 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -920,7 +920,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	struct rcar_i2c_priv *priv;
 	struct i2c_adapter *adap;
 	struct device *dev = &pdev->dev;
-	struct i2c_timings i2c_t;
+	struct i2c_timings i2c_t = {0};
 	int ret;
 
 	/* Otherwise logic will break because some bytes must always use PIO */
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9b2972c7faa2..5cc0b0ec5570 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1593,23 +1593,21 @@ EXPORT_SYMBOL(i2c_del_adapter);
  * @dev: The device to scan for I2C timing properties
  * @t: the i2c_timings struct to be filled with values
  * @use_defaults: bool to use sane defaults derived from the I2C specification
- *		  when properties are not found, otherwise use 0
+ *		  when properties are not found, otherwise don't update
  *
  * Scan the device for the generic I2C properties describing timing parameters
  * for the signal and fill the given struct with the results. If a property was
  * not found and use_defaults was true, then maximum timings are assumed which
  * are derived from the I2C specification. If use_defaults is not used, the
- * results will be 0, so drivers can apply their own defaults later. The latter
- * is mainly intended for avoiding regressions of existing drivers which want
- * to switch to this function. New drivers almost always should use the defaults.
+ * results will be as before, so drivers can apply their own defaults before
+ * calling this helper. The latter is mainly intended for avoiding regressions
+ * of existing drivers which want to switch to this function. New drivers
+ * almost always should use the defaults.
  */
-
 void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults)
 {
 	int ret;
 
-	memset(t, 0, sizeof(*t));
-
 	ret = device_property_read_u32(dev, "clock-frequency", &t->bus_freq_hz);
 	if (ret && use_defaults)
 		t->bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ;
@@ -1632,19 +1630,25 @@ void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_de
 			t->scl_fall_ns = 120;
 	}
 
-	device_property_read_u32(dev, "i2c-scl-internal-delay-ns", &t->scl_int_delay_ns);
+	ret = device_property_read_u32(dev, "i2c-scl-internal-delay-ns", &t->scl_int_delay_ns);
+	if (ret && use_defaults)
+		t->scl_int_delay_ns = 0;
 
 	ret = device_property_read_u32(dev, "i2c-sda-falling-time-ns", &t->sda_fall_ns);
 	if (ret && use_defaults)
 		t->sda_fall_ns = t->scl_fall_ns;
 
-	device_property_read_u32(dev, "i2c-sda-hold-time-ns", &t->sda_hold_ns);
+	ret = device_property_read_u32(dev, "i2c-sda-hold-time-ns", &t->sda_hold_ns);
+	if (ret && use_defaults)
+		t->sda_hold_ns = 0;
 
-	device_property_read_u32(dev, "i2c-digital-filter-width-ns",
-				 &t->digital_filter_width_ns);
+	ret = device_property_read_u32(dev, "i2c-digital-filter-width-ns", &t->digital_filter_width_ns);
+	if (ret && use_defaults)
+		t->digital_filter_width_ns = 0;
 
-	device_property_read_u32(dev, "i2c-analog-filter-cutoff-frequency",
-				 &t->analog_filter_cutoff_freq_hz);
+	ret = device_property_read_u32(dev, "i2c-analog-filter-cutoff-frequency", &t->analog_filter_cutoff_freq_hz);
+	if (ret && use_defaults)
+		t->analog_filter_cutoff_freq_hz = 0;
 }
 EXPORT_SYMBOL_GPL(i2c_parse_fw_timings);
 
-- 
2.25.1

Re: [PATCH v3 2/6] i2c: core: Allow override timing properties with 0

[Wolfram Sang]

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


> +	struct i2c_timings i2c_t = {0};

Simply '... = { }'?


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

Re: [PATCH v3 2/6] i2c: core: Allow override timing properties with 0

[Andy Shevchenko]

On Fri, Mar 20, 2020 at 03:43:57PM +0100, Wolfram Sang wrote:
> 
> > +	struct i2c_timings i2c_t = {0};
> 
> Simply '... = { }'?

I prefer C standard over GCC extension.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/6] i2c: core: Allow override timing properties with 0

[Wolfram Sang]

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

On Fri, Mar 20, 2020 at 06:23:26PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 20, 2020 at 03:43:57PM +0100, Wolfram Sang wrote:
> > 
> > > +	struct i2c_timings i2c_t = {0};
> > 
> > Simply '... = { }'?
> 
> I prefer C standard over GCC extension.

Okay. I don't care too much. But '{ 0 }' then, with kernel style spacing?


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

Re: [PATCH v3 2/6] i2c: core: Allow override timing properties with 0

[Andy Shevchenko]

On Fri, Mar 20, 2020 at 05:44:54PM +0100, Wolfram Sang wrote:
> On Fri, Mar 20, 2020 at 06:23:26PM +0200, Andy Shevchenko wrote:
> > On Fri, Mar 20, 2020 at 03:43:57PM +0100, Wolfram Sang wrote:
> > > 
> > > > +	struct i2c_timings i2c_t = {0};
> > > 
> > > Simply '... = { }'?
> > 
> > I prefer C standard over GCC extension.
> 
> Okay. I don't care too much. But '{ 0 }' then, with kernel style spacing?

Works for me!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 3/6] i2c: rcar: Consolidate timings calls in rcar_i2c_clock_calculate()

[Andy Shevchenko]

Move i2c_parse_fw_timings() to rcar_i2c_clock_calculate() to consolidate
timings calls in one place.

While here, replace hard coded values with standard bus frequency definitions.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: new patch
 drivers/i2c/busses/i2c-rcar.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index f1f915bfdb9d..a8a136e490fa 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -235,17 +235,20 @@ static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
 	return i2c_recover_bus(&priv->adap);
 }
 
-static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv, struct i2c_timings *t)
+static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 {
 	u32 scgd, cdf, round, ick, sum, scl, cdf_width;
 	unsigned long rate;
 	struct device *dev = rcar_i2c_priv_to_dev(priv);
+	struct i2c_timings i2c_t, *t = &i2c_t;
 
 	/* Fall back to previously used values if not supplied */
-	t->bus_freq_hz = t->bus_freq_hz ?: 100000;
-	t->scl_fall_ns = t->scl_fall_ns ?: 35;
-	t->scl_rise_ns = t->scl_rise_ns ?: 200;
-	t->scl_int_delay_ns = t->scl_int_delay_ns ?: 50;
+	t->bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ;
+	t->scl_fall_ns = 35;
+	t->scl_rise_ns = 200;
+	t->scl_int_delay_ns = 50;
+
+	i2c_parse_fw_timings(dev, &i2c_t, false);
 
 	switch (priv->devtype) {
 	case I2C_RCAR_GEN1:
@@ -920,7 +923,6 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	struct rcar_i2c_priv *priv;
 	struct i2c_adapter *adap;
 	struct device *dev = &pdev->dev;
-	struct i2c_timings i2c_t = {0};
 	int ret;
 
 	/* Otherwise logic will break because some bytes must always use PIO */
@@ -957,8 +959,6 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	i2c_set_adapdata(adap, priv);
 	strlcpy(adap->name, pdev->name, sizeof(adap->name));
 
-	i2c_parse_fw_timings(dev, &i2c_t, false);
-
 	/* Init DMA */
 	sg_init_table(&priv->sg, 1);
 	priv->dma_direction = DMA_NONE;
@@ -967,7 +967,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	/* Activate device for clock calculation */
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
-	ret = rcar_i2c_clock_calculate(priv, &i2c_t);
+	ret = rcar_i2c_clock_calculate(priv);
 	if (ret < 0)
 		goto out_pm_put;
 
-- 
2.25.1

Re: [PATCH v3 3/6] i2c: rcar: Consolidate timings calls in rcar_i2c_clock_calculate()

[Wolfram Sang]

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


> +	struct i2c_timings i2c_t, *t = &i2c_t;
>  
>  	/* Fall back to previously used values if not supplied */
> -	t->bus_freq_hz = t->bus_freq_hz ?: 100000;
> -	t->scl_fall_ns = t->scl_fall_ns ?: 35;
> -	t->scl_rise_ns = t->scl_rise_ns ?: 200;
> -	t->scl_int_delay_ns = t->scl_int_delay_ns ?: 50;
> +	t->bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ;
> +	t->scl_fall_ns = 35;
> +	t->scl_rise_ns = 200;
> +	t->scl_int_delay_ns = 50;

Here, the initialization to 0 is missing, so some values are broken.

Why don't we just drop the pointer and init the array directly?

	struct i2c_timings t = {
		.bus_freq_hz = ...
		...
	}


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

Re: [PATCH v3 3/6] i2c: rcar: Consolidate timings calls in rcar_i2c_clock_calculate()

[Andy Shevchenko]

On Mon, Mar 23, 2020 at 10:54:21PM +0100, Wolfram Sang wrote:
> 
> > +	struct i2c_timings i2c_t, *t = &i2c_t;
> >  
> >  	/* Fall back to previously used values if not supplied */
> > -	t->bus_freq_hz = t->bus_freq_hz ?: 100000;
> > -	t->scl_fall_ns = t->scl_fall_ns ?: 35;
> > -	t->scl_rise_ns = t->scl_rise_ns ?: 200;
> > -	t->scl_int_delay_ns = t->scl_int_delay_ns ?: 50;
> > +	t->bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ;
> > +	t->scl_fall_ns = 35;
> > +	t->scl_rise_ns = 200;
> > +	t->scl_int_delay_ns = 50;
> 
> Here, the initialization to 0 is missing, so some values are broken.

Yes, and this is fine. They are not being used. So, the idea is, whenever we
pass "false" as a parameter to the function we must take care of all fields we
are using.

> Why don't we just drop the pointer and init the array directly?
> 
> 	struct i2c_timings t = {
> 		.bus_freq_hz = ...
> 		...
> 	}

I can do it if you think it's better. I have no strong opinion here.
>From code prospective I guess it will be something similar anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/6] i2c: rcar: Consolidate timings calls in rcar_i2c_clock_calculate()

[Wolfram Sang]

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

Hi Andy,

> > Here, the initialization to 0 is missing, so some values are broken.
> 
> Yes, and this is fine. They are not being used. So, the idea is, whenever we
> pass "false" as a parameter to the function we must take care of all fields we
> are using.

Can be argued. Still, uninitialized values look a little sloppy IMO. I
had a patch on top of this series to print the generated values as debug
output, and '0' looks much more intentional there.

> > Why don't we just drop the pointer and init the array directly?
> > 
> > 	struct i2c_timings t = {
> > 		.bus_freq_hz = ...
> > 		...
> > 	}
> 
> I can do it if you think it's better. I have no strong opinion here.
> From code prospective I guess it will be something similar anyway.

I like it better. Easier to read in the code, no need for a seperate
pointer. I can fix it locally here, though.

Thanks!

   Wolfram


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

Re: [PATCH v3 3/6] i2c: rcar: Consolidate timings calls in rcar_i2c_clock_calculate()

[Andy Shevchenko]

On Tue, Mar 24, 2020 at 09:13:28AM +0100, Wolfram Sang wrote:
> Hi Andy,
> 
> > > Here, the initialization to 0 is missing, so some values are broken.
> > 
> > Yes, and this is fine. They are not being used. So, the idea is, whenever we
> > pass "false" as a parameter to the function we must take care of all fields we
> > are using.
> 
> Can be argued. Still, uninitialized values look a little sloppy IMO. I
> had a patch on top of this series to print the generated values as debug
> output, and '0' looks much more intentional there.
> 
> > > Why don't we just drop the pointer and init the array directly?
> > > 
> > > 	struct i2c_timings t = {
> > > 		.bus_freq_hz = ...
> > > 		...
> > > 	}
> > 
> > I can do it if you think it's better. I have no strong opinion here.
> > From code prospective I guess it will be something similar anyway.
> 
> I like it better. Easier to read in the code, no need for a seperate
> pointer. I can fix it locally here, though.

I already sent v4 the other day, but can update since I have got new tags to
pick up.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/6] i2c: rcar: Consolidate timings calls in rcar_i2c_clock_calculate()

[Wolfram Sang]

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

On Tue, Mar 24, 2020 at 11:02:00AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 24, 2020 at 09:13:28AM +0100, Wolfram Sang wrote:
> > Hi Andy,
> > 
> > > > Here, the initialization to 0 is missing, so some values are broken.
> > > 
> > > Yes, and this is fine. They are not being used. So, the idea is, whenever we
> > > pass "false" as a parameter to the function we must take care of all fields we
> > > are using.
> > 
> > Can be argued. Still, uninitialized values look a little sloppy IMO. I
> > had a patch on top of this series to print the generated values as debug
> > output, and '0' looks much more intentional there.
> > 
> > > > Why don't we just drop the pointer and init the array directly?
> > > > 
> > > > 	struct i2c_timings t = {
> > > > 		.bus_freq_hz = ...
> > > > 		...
> > > > 	}
> > > 
> > > I can do it if you think it's better. I have no strong opinion here.
> > > From code prospective I guess it will be something similar anyway.
> > 
> > I like it better. Easier to read in the code, no need for a seperate
> > pointer. I can fix it locally here, though.
> 
> I already sent v4 the other day, but can update since I have got new tags to
> pick up.

Okay, v5 is fine with me as well.


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

[PATCH v3 4/6] i2c: stm32f7: switch to I²C generic property parsing

[Andy Shevchenko]

Switch to the new generic functions: i2c_parse_fw_timings().

While here, replace hard coded values with standard bus frequency definitions.

Cc: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: new patch instead of simple frequency conversion
 drivers/i2c/busses/i2c-stm32f7.c | 57 +++++++++++++++-----------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 6418f5982894..18f231f631f5 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -344,9 +344,9 @@ struct stm32f7_i2c_dev {
  */
 static struct stm32f7_i2c_spec i2c_specs[] = {
 	[STM32_I2C_SPEED_STANDARD] = {
-		.rate = 100000,
-		.rate_min = 80000,
-		.rate_max = 100000,
+		.rate = I2C_MAX_STANDARD_MODE_FREQ,
+		.rate_min = I2C_MAX_STANDARD_MODE_FREQ * 8 / 10,	/* 80% */
+		.rate_max = I2C_MAX_STANDARD_MODE_FREQ,
 		.fall_max = 300,
 		.rise_max = 1000,
 		.hddat_min = 0,
@@ -356,9 +356,9 @@ static struct stm32f7_i2c_spec i2c_specs[] = {
 		.h_min = 4000,
 	},
 	[STM32_I2C_SPEED_FAST] = {
-		.rate = 400000,
-		.rate_min = 320000,
-		.rate_max = 400000,
+		.rate = I2C_MAX_FAST_MODE_FREQ,
+		.rate_min = I2C_MAX_FAST_MODE_FREQ * 8 / 10,		/* 80% */
+		.rate_max = I2C_MAX_FAST_MODE_FREQ,
 		.fall_max = 300,
 		.rise_max = 300,
 		.hddat_min = 0,
@@ -368,9 +368,9 @@ static struct stm32f7_i2c_spec i2c_specs[] = {
 		.h_min = 600,
 	},
 	[STM32_I2C_SPEED_FAST_PLUS] = {
-		.rate = 1000000,
-		.rate_min = 800000,
-		.rate_max = 1000000,
+		.rate = I2C_MAX_FAST_MODE_PLUS_FREQ,
+		.rate_min = I2C_MAX_FAST_MODE_PLUS_FREQ * 8 / 10,	/* 80% */
+		.rate_max = I2C_MAX_FAST_MODE_PLUS_FREQ,
 		.fall_max = 100,
 		.rise_max = 120,
 		.hddat_min = 0,
@@ -610,8 +610,25 @@ static int stm32f7_i2c_compute_timing(struct stm32f7_i2c_dev *i2c_dev,
 static int stm32f7_i2c_setup_timing(struct stm32f7_i2c_dev *i2c_dev,
 				    struct stm32f7_i2c_setup *setup)
 {
+	struct i2c_timings timings, *t = &timings;
 	int ret = 0;
 
+	t->bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ;
+	t->scl_rise_ns = i2c_dev->setup.rise_time;
+	t->scl_fall_ns = i2c_dev->setup.fall_time;
+
+	i2c_parse_fw_timings(&pdev->dev, t, false);
+
+	if (t->bus_freq_hz >= I2C_MAX_FAST_MODE_PLUS_FREQ)
+		i2c_dev->speed = STM32_I2C_SPEED_FAST_PLUS;
+	else if (t->bus_freq_hz >= I2C_MAX_FAST_MODE_FREQ)
+		i2c_dev->speed = STM32_I2C_SPEED_FAST;
+	else
+		i2c_dev->speed = STM32_I2C_SPEED_STANDARD;
+
+	i2c_dev->setup.rise_time = t->scl_rise_ns;
+	i2c_dev->setup.fall_time = t->scl_fall_ns;
+
 	setup->speed = i2c_dev->speed;
 	setup->speed_freq = i2c_specs[setup->speed].rate;
 	setup->clock_src = clk_get_rate(i2c_dev->clk);
@@ -1914,7 +1931,6 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
 	struct stm32f7_i2c_dev *i2c_dev;
 	const struct stm32f7_i2c_setup *setup;
 	struct resource *res;
-	u32 clk_rate, rise_time, fall_time;
 	struct i2c_adapter *adap;
 	struct reset_control *rst;
 	dma_addr_t phy_addr;
@@ -1961,17 +1977,6 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	i2c_dev->speed = STM32_I2C_SPEED_STANDARD;
-	ret = device_property_read_u32(&pdev->dev, "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;
-	}
-
 	rst = devm_reset_control_get(&pdev->dev, NULL);
 	if (IS_ERR(rst)) {
 		dev_err(&pdev->dev, "Error: Missing controller reset\n");
@@ -2011,16 +2016,6 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
 	}
 	i2c_dev->setup = *setup;
 
-	ret = device_property_read_u32(i2c_dev->dev, "i2c-scl-rising-time-ns",
-				       &rise_time);
-	if (!ret)
-		i2c_dev->setup.rise_time = rise_time;
-
-	ret = device_property_read_u32(i2c_dev->dev, "i2c-scl-falling-time-ns",
-				       &fall_time);
-	if (!ret)
-		i2c_dev->setup.fall_time = fall_time;
-
 	ret = stm32f7_i2c_setup_timing(i2c_dev, &i2c_dev->setup);
 	if (ret)
 		goto clk_free;
-- 
2.25.1

Re: [PATCH v3 4/6] i2c: stm32f7: switch to I²C generic property parsing

[Alain Volmat]

On Mon, Mar 16, 2020 at 05:49:27PM +0200, Andy Shevchenko wrote:
> Switch to the new generic functions: i2c_parse_fw_timings().
> 
> While here, replace hard coded values with standard bus frequency definitions.
> 
> Cc: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v3: new patch instead of simple frequency conversion
>  drivers/i2c/busses/i2c-stm32f7.c | 57 +++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> index 6418f5982894..18f231f631f5 100644
> --- a/drivers/i2c/busses/i2c-stm32f7.c
> +++ b/drivers/i2c/busses/i2c-stm32f7.c
> @@ -344,9 +344,9 @@ struct stm32f7_i2c_dev {
>   */
>  static struct stm32f7_i2c_spec i2c_specs[] = {
>  	[STM32_I2C_SPEED_STANDARD] = {
> -		.rate = 100000,
> -		.rate_min = 80000,
> -		.rate_max = 100000,
> +		.rate = I2C_MAX_STANDARD_MODE_FREQ,
> +		.rate_min = I2C_MAX_STANDARD_MODE_FREQ * 8 / 10,	/* 80% */
> +		.rate_max = I2C_MAX_STANDARD_MODE_FREQ,
>  		.fall_max = 300,
>  		.rise_max = 1000,
>  		.hddat_min = 0,
> @@ -356,9 +356,9 @@ static struct stm32f7_i2c_spec i2c_specs[] = {
>  		.h_min = 4000,
>  	},
>  	[STM32_I2C_SPEED_FAST] = {
> -		.rate = 400000,
> -		.rate_min = 320000,
> -		.rate_max = 400000,
> +		.rate = I2C_MAX_FAST_MODE_FREQ,
> +		.rate_min = I2C_MAX_FAST_MODE_FREQ * 8 / 10,		/* 80% */
> +		.rate_max = I2C_MAX_FAST_MODE_FREQ,
>  		.fall_max = 300,
>  		.rise_max = 300,
>  		.hddat_min = 0,
> @@ -368,9 +368,9 @@ static struct stm32f7_i2c_spec i2c_specs[] = {
>  		.h_min = 600,
>  	},
>  	[STM32_I2C_SPEED_FAST_PLUS] = {
> -		.rate = 1000000,
> -		.rate_min = 800000,
> -		.rate_max = 1000000,
> +		.rate = I2C_MAX_FAST_MODE_PLUS_FREQ,
> +		.rate_min = I2C_MAX_FAST_MODE_PLUS_FREQ * 8 / 10,	/* 80% */
> +		.rate_max = I2C_MAX_FAST_MODE_PLUS_FREQ,
>  		.fall_max = 100,
>  		.rise_max = 120,
>  		.hddat_min = 0,
> @@ -610,8 +610,25 @@ static int stm32f7_i2c_compute_timing(struct stm32f7_i2c_dev *i2c_dev,
>  static int stm32f7_i2c_setup_timing(struct stm32f7_i2c_dev *i2c_dev,
>  				    struct stm32f7_i2c_setup *setup)
>  {
> +	struct i2c_timings timings, *t = &timings;
>  	int ret = 0;
>  
> +	t->bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ;
> +	t->scl_rise_ns = i2c_dev->setup.rise_time;
> +	t->scl_fall_ns = i2c_dev->setup.fall_time;
> +
> +	i2c_parse_fw_timings(&pdev->dev, t, false);

Andy, thanks for the patch.
Looks fine overall, apart from the above line which should be

        i2c_parse_fw_timings(i2c_dev->dev, t, false);

> +
> +	if (t->bus_freq_hz >= I2C_MAX_FAST_MODE_PLUS_FREQ)
> +		i2c_dev->speed = STM32_I2C_SPEED_FAST_PLUS;
> +	else if (t->bus_freq_hz >= I2C_MAX_FAST_MODE_FREQ)
> +		i2c_dev->speed = STM32_I2C_SPEED_FAST;
> +	else
> +		i2c_dev->speed = STM32_I2C_SPEED_STANDARD;
> +
> +	i2c_dev->setup.rise_time = t->scl_rise_ns;
> +	i2c_dev->setup.fall_time = t->scl_fall_ns;
> +
>  	setup->speed = i2c_dev->speed;
>  	setup->speed_freq = i2c_specs[setup->speed].rate;
>  	setup->clock_src = clk_get_rate(i2c_dev->clk);
> @@ -1914,7 +1931,6 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
>  	struct stm32f7_i2c_dev *i2c_dev;
>  	const struct stm32f7_i2c_setup *setup;
>  	struct resource *res;
> -	u32 clk_rate, rise_time, fall_time;
>  	struct i2c_adapter *adap;
>  	struct reset_control *rst;
>  	dma_addr_t phy_addr;
> @@ -1961,17 +1977,6 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	i2c_dev->speed = STM32_I2C_SPEED_STANDARD;
> -	ret = device_property_read_u32(&pdev->dev, "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;
> -	}
> -
>  	rst = devm_reset_control_get(&pdev->dev, NULL);
>  	if (IS_ERR(rst)) {
>  		dev_err(&pdev->dev, "Error: Missing controller reset\n");
> @@ -2011,16 +2016,6 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
>  	}
>  	i2c_dev->setup = *setup;
>  
> -	ret = device_property_read_u32(i2c_dev->dev, "i2c-scl-rising-time-ns",
> -				       &rise_time);
> -	if (!ret)
> -		i2c_dev->setup.rise_time = rise_time;
> -
> -	ret = device_property_read_u32(i2c_dev->dev, "i2c-scl-falling-time-ns",
> -				       &fall_time);
> -	if (!ret)
> -		i2c_dev->setup.fall_time = fall_time;
> -
>  	ret = stm32f7_i2c_setup_timing(i2c_dev, &i2c_dev->setup);
>  	if (ret)
>  		goto clk_free;
> -- 
> 2.25.1
> 

Re: [PATCH v3 4/6] i2c: stm32f7: switch to I²C generic property parsing

[Andy Shevchenko]

On Fri, Mar 20, 2020 at 09:23:42AM +0100, Alain Volmat wrote:
> On Mon, Mar 16, 2020 at 05:49:27PM +0200, Andy Shevchenko wrote:
> > Switch to the new generic functions: i2c_parse_fw_timings().
> > 
> > While here, replace hard coded values with standard bus frequency definitions.

...

> >  static int stm32f7_i2c_setup_timing(struct stm32f7_i2c_dev *i2c_dev,
> >  				    struct stm32f7_i2c_setup *setup)
> >  {

> > +	i2c_parse_fw_timings(&pdev->dev, t, false);
> 
> Andy, thanks for the patch.
> Looks fine overall, apart from the above line which should be
> 
>         i2c_parse_fw_timings(i2c_dev->dev, t, false);

Oh, good catch!

If I read the code correctly it's an equivalent, but I see that we don't supply pdev.

I will fix it for v4.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 5/6] i2c: algo: Use generic definitions for bus frequencies

[Andy Shevchenko]

Since we have generic definitions for bus frequencies, let's use them.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: no changes
 drivers/i2c/algos/i2c-algo-pca.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
index 5ac93f41bfec..dff4e178c732 100644
--- a/drivers/i2c/algos/i2c-algo-pca.c
+++ b/drivers/i2c/algos/i2c-algo-pca.c
@@ -459,17 +459,17 @@ static int pca_init(struct i2c_adapter *adap)
 		/* To avoid integer overflow, use clock/100 for calculations */
 		clock = pca_clock(pca_data) / 100;
 
-		if (pca_data->i2c_clock > 1000000) {
+		if (pca_data->i2c_clock > I2C_MAX_FAST_MODE_PLUS_FREQ) {
 			mode = I2C_PCA_MODE_TURBO;
 			min_tlow = 14;
 			min_thi  = 5;
 			raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
-		} else if (pca_data->i2c_clock > 400000) {
+		} else if (pca_data->i2c_clock > I2C_MAX_FAST_MODE_FREQ) {
 			mode = I2C_PCA_MODE_FASTP;
 			min_tlow = 17;
 			min_thi  = 9;
 			raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
-		} else if (pca_data->i2c_clock > 100000) {
+		} else if (pca_data->i2c_clock > I2C_MAX_STANDARD_MODE_FREQ) {
 			mode = I2C_PCA_MODE_FAST;
 			min_tlow = 44;
 			min_thi  = 20;
-- 
2.25.1

Re: [PATCH v3 1/6] i2c: core: Provide generic definitions for bus frequencies

[Andy Shevchenko]

On Mon, Mar 16, 2020 at 05:49:24PM +0200, Andy Shevchenko wrote:
> There are few maximum bus frequencies being used in the I²C core code.
> Provide generic definitions for bus frequencies and use them in the core.
> 
> The drivers may use predefined constants where it is appropriate.
> Some of them are already using these under slightly different names.
> We will convert them later to use newly introduced defines.
> 
> Note, the name of modes are chosen to follow well established naming
> scheme [1].
> 
> These definitions will also help to avoid typos in the numbers that
> may lead to subtle errors.

Wolfram, is any chance to get applied at least 1, 5 and 6 from this series?

> 
> [1]: https://en.wikipedia.org/wiki/I%C2%B2C#Differences_between_modes
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v3: added link to Wiki to point the mode naming scheme out
>  drivers/i2c/i2c-core-acpi.c | 2 +-
>  drivers/i2c/i2c-core-base.c | 8 ++++----
>  include/linux/i2c.h         | 8 ++++++++
>  3 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 8b0ff780919b..c8f42f2037cb 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -318,7 +318,7 @@ static acpi_status i2c_acpi_lookup_speed(acpi_handle handle, u32 level,
>  		lookup->min_speed = lookup->speed;
>  
>  	if (acpi_match_device_ids(adev, i2c_acpi_force_400khz_device_ids) == 0)
> -		lookup->force_speed = 400000;
> +		lookup->force_speed = I2C_MAX_FAST_MODE_FREQ;
>  
>  	return AE_OK;
>  }
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index cefad0881942..9b2972c7faa2 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1612,13 +1612,13 @@ void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_de
>  
>  	ret = device_property_read_u32(dev, "clock-frequency", &t->bus_freq_hz);
>  	if (ret && use_defaults)
> -		t->bus_freq_hz = 100000;
> +		t->bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ;
>  
>  	ret = device_property_read_u32(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns);
>  	if (ret && use_defaults) {
> -		if (t->bus_freq_hz <= 100000)
> +		if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
>  			t->scl_rise_ns = 1000;
> -		else if (t->bus_freq_hz <= 400000)
> +		else if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
>  			t->scl_rise_ns = 300;
>  		else
>  			t->scl_rise_ns = 120;
> @@ -1626,7 +1626,7 @@ void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_de
>  
>  	ret = device_property_read_u32(dev, "i2c-scl-falling-time-ns", &t->scl_fall_ns);
>  	if (ret && use_defaults) {
> -		if (t->bus_freq_hz <= 400000)
> +		if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
>  			t->scl_fall_ns = 300;
>  		else
>  			t->scl_fall_ns = 120;
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index f834687989f7..72e759328cee 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -39,6 +39,14 @@ enum i2c_slave_event;
>  typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
>  			      enum i2c_slave_event event, u8 *val);
>  
> +/* I2C Frequency Modes */
> +#define I2C_MAX_STANDARD_MODE_FREQ	100000
> +#define I2C_MAX_FAST_MODE_FREQ		400000
> +#define I2C_MAX_FAST_MODE_PLUS_FREQ	1000000
> +#define I2C_MAX_TURBO_MODE_FREQ		1400000
> +#define I2C_MAX_HIGH_SPEED_MODE_FREQ	3400000
> +#define I2C_MAX_ULTRA_FAST_MODE_FREQ	5000000
> +
>  struct module;
>  struct property_entry;
>  
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/6] i2c: core: Provide generic definitions for bus frequencies

[Wolfram Sang]

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

On Thu, Mar 19, 2020 at 07:19:13PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 16, 2020 at 05:49:24PM +0200, Andy Shevchenko wrote:
> > There are few maximum bus frequencies being used in the I²C core code.
> > Provide generic definitions for bus frequencies and use them in the core.
> > 
> > The drivers may use predefined constants where it is appropriate.
> > Some of them are already using these under slightly different names.
> > We will convert them later to use newly introduced defines.
> > 
> > Note, the name of modes are chosen to follow well established naming
> > scheme [1].
> > 
> > These definitions will also help to avoid typos in the numbers that
> > may lead to subtle errors.
> 
> Wolfram, is any chance to get applied at least 1, 5 and 6 from this series?

Yes, just make sure v4 is based on i2c/for-next to minimize merge
conflicts.


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

Re: [PATCH v3 1/6] i2c: core: Provide generic definitions for bus frequencies

[Andy Shevchenko]

On Fri, Mar 20, 2020 at 03:45:42PM +0100, Wolfram Sang wrote:
> On Thu, Mar 19, 2020 at 07:19:13PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 16, 2020 at 05:49:24PM +0200, Andy Shevchenko wrote:
> > > There are few maximum bus frequencies being used in the I²C core code.
> > > Provide generic definitions for bus frequencies and use them in the core.
> > > 
> > > The drivers may use predefined constants where it is appropriate.
> > > Some of them are already using these under slightly different names.
> > > We will convert them later to use newly introduced defines.
> > > 
> > > Note, the name of modes are chosen to follow well established naming
> > > scheme [1].
> > > 
> > > These definitions will also help to avoid typos in the numbers that
> > > may lead to subtle errors.
> > 
> > Wolfram, is any chance to get applied at least 1, 5 and 6 from this series?
> 
> Yes, just make sure v4 is based on i2c/for-next to minimize merge
> conflicts.

Will do, thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/6] i2c: drivers: Use generic definitions for bus frequencies

[Dmitry Osipenko]

16.03.2020 18:49, Andy Shevchenko пишет:
> Since we have generic definitions for bus frequencies, let's use them.

...
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index cbc2ad49043e..4c4d17ddc96b 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -123,10 +123,6 @@
>  #define I2C_THIGH_SHIFT				8
>  #define I2C_INTERFACE_TIMING_1			0x98
>  
> -#define I2C_STANDARD_MODE			100000
> -#define I2C_FAST_MODE				400000
> -#define I2C_FAST_PLUS_MODE			1000000
...

For NVIDIA Tegra I2C:

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>