diff mbox

[v2,2/9] i2c: add generic routine to parse DT for timing information

Message ID 1449567473-2084-3-git-send-email-wsa@the-dreams.de (mailing list archive)
State New, archived
Headers show

Commit Message

Wolfram Sang Dec. 8, 2015, 9:37 a.m. UTC
From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Inspired from the i2c-rk3x driver (thanks guys!) but refactored and
extended. See built-in docs for further information.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    | 18 ++++++++++++++++++
 2 files changed, 65 insertions(+)

Comments

Andy Shevchenko Dec. 8, 2015, 10:54 a.m. UTC | #1
On Tue, 2015-12-08 at 10:37 +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Inspired from the i2c-rk3x driver (thanks guys!) but refactored and
> extended. See built-in docs for further information.

One style comment.

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core.c | 47
> +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h    | 18 ++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index ba8eb087f22465..e94d2ca2aab4aa 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -53,6 +53,7 @@
>  #include <linux/jump_label.h>
>  #include <asm/uaccess.h>
>  #include <linux/err.h>
> +#include <linux/property.h>
>  
>  #include "i2c-core.h"
>  
> @@ -1438,6 +1439,52 @@ static void of_i2c_register_devices(struct
> i2c_adapter *adap)
>  	}
>  }
>  
> +/**
> + * i2c_parse_fw_timings - get I2C related timing parameters from
> firmware
> + * @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
> + *
> + * 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.
> + */
> +
> +void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t,
> bool use_defaults)
> +{
> +	memset(t, 0, sizeof(*t));
> +
> +	if (device_property_read_u32(dev, "clock-frequency", &t-
> >bus_freq_hz) && use_defaults)
> +		t->bus_freq_hz = 100000;
> +
> +	if (device_property_read_u32(dev, "i2c-scl-rising-time-ns",
> &t->scl_rise_ns) && use_defaults) {
> +		if (t->bus_freq_hz <= 100000)
> +			t->scl_rise_ns = 1000;
> +		else if (t->bus_freq_hz <= 400000)
> +			t->scl_rise_ns = 300;
> +		else
> +			t->scl_rise_ns = 120;
> +	}
> +
> +	if (device_property_read_u32(dev, "i2c-scl-falling-time-ns", 
> &t->scl_fall_ns) && use_defaults) {
> +		if (t->bus_freq_hz <= 400000)
> +			t->scl_fall_ns = 300;
> +		else
> +			t->scl_fall_ns = 120;
> +	}
> +
> +	device_property_read_u32(dev, "i2c-scl-internal-delay-ns",
> &t->scl_int_delay_ns);
> +
> +	if (device_property_read_u32(dev, "i2c-sda-falling-time-ns", 
> &t->sda_fall_ns) && use_defaults)
> +		t->sda_fall_ns = t->scl_fall_ns;

Too many && use_defaults. What about

memset(t, 0, sizeof(*t));

device_property_read_u32(dev, "i2c-scl-internal-delay-ns", &t-
>scl_int_delay_ns);

if (!use_defaults)
 return;

...
Mika Westerberg Dec. 8, 2015, 11:09 a.m. UTC | #2
On Tue, Dec 08, 2015 at 10:37:46AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Inspired from the i2c-rk3x driver (thanks guys!) but refactored and
> extended. See built-in docs for further information.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Looks good. I think we can take advantage of this in the designware
driver as well.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I wonder if it makes sense to add "i2c-sda-hold-time-ns" (taken from the
designware driver DT binding) to the timings structure? It is tHD;DAT
parameter in the I2C bus specification.
Wolfram Sang Dec. 8, 2015, 12:53 p.m. UTC | #3
> I wonder if it makes sense to add "i2c-sda-hold-time-ns" (taken from the
> designware driver DT binding) to the timings structure? It is tHD;DAT
> parameter in the I2C bus specification.

It totally makes sense. I just didn't need it for the RCar driver and
didn't want to implement something which isn't actually used. So feel
free to add it, or ask me to do it, if you promise to use it in the
designware driver :)

Thanks for the review!
Wolfram Sang Dec. 8, 2015, 1:03 p.m. UTC | #4
> Too many && use_defaults. What about
> 
> memset(t, 0, sizeof(*t));
> 
> device_property_read_u32(dev, "i2c-scl-internal-delay-ns", &t-
> >scl_int_delay_ns);
> 
> if (!use_defaults)
>  return;

I like this! Thanks for the input.
Mika Westerberg Dec. 8, 2015, 1:03 p.m. UTC | #5
On Tue, Dec 08, 2015 at 01:53:07PM +0100, Wolfram Sang wrote:
> 
> > I wonder if it makes sense to add "i2c-sda-hold-time-ns" (taken from the
> > designware driver DT binding) to the timings structure? It is tHD;DAT
> > parameter in the I2C bus specification.
> 
> It totally makes sense. I just didn't need it for the RCar driver and
> didn't want to implement something which isn't actually used. So feel
> free to add it, or ask me to do it, if you promise to use it in the
> designware driver :)

I can add it in a separate patch later on. Thanks!
Wolfram Sang Dec. 8, 2015, 9:51 p.m. UTC | #6
On Tue, Dec 08, 2015 at 02:03:23PM +0100, Wolfram Sang wrote:
> 
> > Too many && use_defaults. What about
> > 
> > memset(t, 0, sizeof(*t));
> > 
> > device_property_read_u32(dev, "i2c-scl-internal-delay-ns", &t-
> > >scl_int_delay_ns);
> > 
> > if (!use_defaults)
> >  return;
> 
> I like this! Thanks for the input.

Oops, too enthusiastic. This skips parsing all the other properties...
The defaults should only be applied if reading the properties fails.
Andy Shevchenko Dec. 9, 2015, 12:12 p.m. UTC | #7
On Tue, 2015-12-08 at 22:51 +0100, Wolfram Sang wrote:
> On Tue, Dec 08, 2015 at 02:03:23PM +0100, Wolfram Sang wrote:
> > 
> > > Too many && use_defaults. What about
> > > 
> > > memset(t, 0, sizeof(*t));
> > > 
> > > device_property_read_u32(dev, "i2c-scl-internal-delay-ns", &t-
> > > > scl_int_delay_ns);
> > > 
> > > if (!use_defaults)
> > >  return;
> > 
> > I like this! Thanks for the input.
> 
> Oops, too enthusiastic. This skips parsing all the other
> properties...
> The defaults should only be applied if reading the properties fails.

Looks like property stuff is not destructive in case of error, so, what
about

paramX = use_defaults ? defaultX : 0;
device_property_read…(…, &paramX);

Or maybe just be a bit verbose like the original variant
int ret;

ret = device_property_read…(…);
if (ret && use_defaults) {
…
}

Among all variants I like the latter one here. What do you think?
Wolfram Sang Dec. 14, 2015, 9:49 a.m. UTC | #8
> Or maybe just be a bit verbose like the original variant
> int ret;
> 
> ret = device_property_read…(…);
> if (ret && use_defaults) {
> …
> }
> 
> Among all variants I like the latter one here. What do you think?

I agree and fixed it like this.
diff mbox

Patch

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index ba8eb087f22465..e94d2ca2aab4aa 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -53,6 +53,7 @@ 
 #include <linux/jump_label.h>
 #include <asm/uaccess.h>
 #include <linux/err.h>
+#include <linux/property.h>
 
 #include "i2c-core.h"
 
@@ -1438,6 +1439,52 @@  static void of_i2c_register_devices(struct i2c_adapter *adap)
 	}
 }
 
+/**
+ * i2c_parse_fw_timings - get I2C related timing parameters from firmware
+ * @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
+ *
+ * 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.
+ */
+
+void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults)
+{
+	memset(t, 0, sizeof(*t));
+
+	if (device_property_read_u32(dev, "clock-frequency", &t->bus_freq_hz) && use_defaults)
+		t->bus_freq_hz = 100000;
+
+	if (device_property_read_u32(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns) && use_defaults) {
+		if (t->bus_freq_hz <= 100000)
+			t->scl_rise_ns = 1000;
+		else if (t->bus_freq_hz <= 400000)
+			t->scl_rise_ns = 300;
+		else
+			t->scl_rise_ns = 120;
+	}
+
+	if (device_property_read_u32(dev, "i2c-scl-falling-time-ns", &t->scl_fall_ns) && use_defaults) {
+		if (t->bus_freq_hz <= 400000)
+			t->scl_fall_ns = 300;
+		else
+			t->scl_fall_ns = 120;
+	}
+
+	device_property_read_u32(dev, "i2c-scl-internal-delay-ns", &t->scl_int_delay_ns);
+
+	if (device_property_read_u32(dev, "i2c-sda-falling-time-ns", &t->sda_fall_ns) && use_defaults)
+		t->sda_fall_ns = t->scl_fall_ns;
+}
+EXPORT_SYMBOL_GPL(i2c_parse_fw_timings);
+
 static int of_dev_node_match(struct device *dev, void *data)
 {
 	return dev->of_node == data;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 768063baafbf5e..7c45181f41ab7d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -414,6 +414,22 @@  struct i2c_algorithm {
 };
 
 /**
+ * struct i2c_timings - I2C timing information
+ * @bus_freq_hz: the bus frequency in Hz
+ * @scl_rise_ns: time SCL signal takes to rise in ns; t(r) in the I2C specification
+ * @scl_fall_ns: time SCL signal takes to fall in ns; t(f) in the I2C specification
+ * @scl_int_delay_ns: time IP core additionally needs to setup SCL in ns
+ * @sda_fall_ns: time SDA signal takes to fall in ns; t(f) in the I2C specification
+ */
+struct i2c_timings {
+	u32 bus_freq_hz;
+	u32 scl_rise_ns;
+	u32 scl_fall_ns;
+	u32 scl_int_delay_ns;
+	u32 sda_fall_ns;
+};
+
+/**
  * struct i2c_bus_recovery_info - I2C bus recovery information
  * @recover_bus: Recover routine. Either pass driver's recover_bus() routine, or
  *	i2c_generic_scl_recovery() or i2c_generic_gpio_recovery().
@@ -602,6 +618,7 @@  extern void i2c_clients_command(struct i2c_adapter *adap,
 extern struct i2c_adapter *i2c_get_adapter(int nr);
 extern void i2c_put_adapter(struct i2c_adapter *adap);
 
+void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults);
 
 /* Return the functionality mask */
 static inline u32 i2c_get_functionality(struct i2c_adapter *adap)
@@ -644,6 +661,7 @@  extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 
 /* must call i2c_put_adapter() when done with returned i2c_adapter device */
 struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node);
+
 #else
 
 static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)