* [RFC PATCH] i2c: refactor parsing of timings
@ 2020-03-26 10:16 Wolfram Sang
2020-03-26 10:36 ` Geert Uytterhoeven
2020-03-26 10:47 ` Andy Shevchenko
0 siblings, 2 replies; 9+ messages in thread
From: Wolfram Sang @ 2020-03-26 10:16 UTC (permalink / raw)
To: linux-i2c; +Cc: linux-renesas-soc, Andy Shevchenko, Wolfram Sang
When I wanted to print the chosen values to debug output, I concluded
that a helper function to parse one timing would be helpful.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
I am not entirely happy because 'dev' and 'u' is the same for each call.
Then again, we can't use a for-loop over an array of parameters, because
some default values depend on previously obtained timings.
Looking for opinions here...
drivers/i2c/i2c-core-base.c | 78 ++++++++++++++++++-------------------
1 file changed, 38 insertions(+), 40 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 474baaf8c9e7..60b0aa246af2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1609,6 +1609,18 @@ void i2c_del_adapter(struct i2c_adapter *adap)
}
EXPORT_SYMBOL(i2c_del_adapter);
+static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p,
+ u32 def_val, bool use_def)
+{
+ int ret;
+
+ ret = device_property_read_u32(dev, prop_name, cur_val_p);
+ if (ret && use_def)
+ *cur_val_p = def_val;
+
+ dev_dbg(dev, "%s: %u\n", prop_name, *cur_val_p);
+}
+
/**
* i2c_parse_fw_timings - get I2C related timing parameters from firmware
* @dev: The device to scan for I2C timing properties
@@ -1627,49 +1639,35 @@ EXPORT_SYMBOL(i2c_del_adapter);
*/
void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults)
{
- int ret;
-
- 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;
-
- ret = device_property_read_u32(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns);
- if (ret && use_defaults) {
- if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
- t->scl_rise_ns = 1000;
- else if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
- t->scl_rise_ns = 300;
- else
- t->scl_rise_ns = 120;
- }
-
- ret = device_property_read_u32(dev, "i2c-scl-falling-time-ns", &t->scl_fall_ns);
- if (ret && use_defaults) {
- if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
- t->scl_fall_ns = 300;
- else
- t->scl_fall_ns = 120;
- }
+ bool u = use_defaults;
+ u32 d;
- 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;
+ i2c_parse_timing(dev, "clock-frequency", &t->bus_freq_hz,
+ I2C_MAX_STANDARD_MODE_FREQ, u);
- 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;
-
- ret = device_property_read_u32(dev, "i2c-sda-hold-time-ns", &t->sda_hold_ns);
- if (ret && use_defaults)
- t->sda_hold_ns = 0;
-
- 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;
+ if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
+ d = 1000;
+ else if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
+ d = 300;
+ else
+ d = 120;
+ i2c_parse_timing(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns, d, u);
- 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;
+ if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
+ d = 300;
+ else
+ d = 120;
+ i2c_parse_timing(dev, "i2c-scl-falling-time-ns", &t->scl_fall_ns, d, u);
+
+ i2c_parse_timing(dev, "i2c-scl-internal-delay-ns",
+ &t->scl_int_delay_ns, 0, u);
+ i2c_parse_timing(dev, "i2c-sda-falling-time-ns", &t->sda_fall_ns,
+ t->scl_fall_ns, u);
+ i2c_parse_timing(dev, "i2c-sda-hold-time-ns", &t->sda_hold_ns, 0, u);
+ i2c_parse_timing(dev, "i2c-digital-filter-width-ns",
+ &t->digital_filter_width_ns, 0, u);
+ i2c_parse_timing(dev, "i2c-analog-filter-cutoff-frequency",
+ &t->analog_filter_cutoff_freq_hz, 0, u);
}
EXPORT_SYMBOL_GPL(i2c_parse_fw_timings);
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] i2c: refactor parsing of timings
2020-03-26 10:16 [RFC PATCH] i2c: refactor parsing of timings Wolfram Sang
@ 2020-03-26 10:36 ` Geert Uytterhoeven
2020-03-26 10:50 ` Andy Shevchenko
2020-03-26 10:52 ` Wolfram Sang
2020-03-26 10:47 ` Andy Shevchenko
1 sibling, 2 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-03-26 10:36 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Linux I2C, Linux-Renesas, Andy Shevchenko
Hi Wolfram,
On Thu, Mar 26, 2020 at 11:17 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> When I wanted to print the chosen values to debug output, I concluded
> that a helper function to parse one timing would be helpful.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Thanks for your patch!
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1609,6 +1609,18 @@ void i2c_del_adapter(struct i2c_adapter *adap)
> }
> EXPORT_SYMBOL(i2c_del_adapter);
>
> +static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p,
> + u32 def_val, bool use_def)
> +{
> + int ret;
> +
> + ret = device_property_read_u32(dev, prop_name, cur_val_p);
> + if (ret && use_def)
> + *cur_val_p = def_val;
Alternatively, you could just preinitialize the value with the default value
before calling this function, and ignoring ret.
That would remove the need for both the def_val and use_def parameters.
> +
> + dev_dbg(dev, "%s: %u\n", prop_name, *cur_val_p);
> +}
> +
> /**
> * i2c_parse_fw_timings - get I2C related timing parameters from firmware
> * @dev: The device to scan for I2C timing properties
> @@ -1627,49 +1639,35 @@ EXPORT_SYMBOL(i2c_del_adapter);
> */
> void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults)
> {
> - int ret;
> -
> - 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;
> -
> - ret = device_property_read_u32(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns);
> - if (ret && use_defaults) {
> - if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
> - t->scl_rise_ns = 1000;
> - else if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> - t->scl_rise_ns = 300;
> - else
> - t->scl_rise_ns = 120;
> - }
> -
> - ret = device_property_read_u32(dev, "i2c-scl-falling-time-ns", &t->scl_fall_ns);
> - if (ret && use_defaults) {
> - if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> - t->scl_fall_ns = 300;
> - else
> - t->scl_fall_ns = 120;
> - }
> + bool u = use_defaults;
> + u32 d;
>
> - 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;
> + i2c_parse_timing(dev, "clock-frequency", &t->bus_freq_hz,
> + I2C_MAX_STANDARD_MODE_FREQ, u);
>
> - 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;
> -
> - ret = device_property_read_u32(dev, "i2c-sda-hold-time-ns", &t->sda_hold_ns);
> - if (ret && use_defaults)
> - t->sda_hold_ns = 0;
> -
> - 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;
> + if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
> + d = 1000;
> + else if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> + d = 300;
> + else
> + d = 120;
> + i2c_parse_timing(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns, d, u);
>
> - 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;
> + if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> + d = 300;
> + else
> + d = 120;
Is the difference with above intentional, or an oversight?
If the latter, you could skip reinitializing d to the value it already has.
Just though I'd better ask ;-)
if the former, I like the dreaded ternary operator (only) for cases like this:
d = t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ ? 300 : 120
> + i2c_parse_timing(dev, "i2c-scl-falling-time-ns", &t->scl_fall_ns, d, u);
> +
> + i2c_parse_timing(dev, "i2c-scl-internal-delay-ns",
> + &t->scl_int_delay_ns, 0, u);
> + i2c_parse_timing(dev, "i2c-sda-falling-time-ns", &t->sda_fall_ns,
> + t->scl_fall_ns, u);
> + i2c_parse_timing(dev, "i2c-sda-hold-time-ns", &t->sda_hold_ns, 0, u);
> + i2c_parse_timing(dev, "i2c-digital-filter-width-ns",
> + &t->digital_filter_width_ns, 0, u);
> + i2c_parse_timing(dev, "i2c-analog-filter-cutoff-frequency",
> + &t->analog_filter_cutoff_freq_hz, 0, u);
> }
> EXPORT_SYMBOL_GPL(i2c_parse_fw_timings);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] i2c: refactor parsing of timings
2020-03-26 10:16 [RFC PATCH] i2c: refactor parsing of timings Wolfram Sang
2020-03-26 10:36 ` Geert Uytterhoeven
@ 2020-03-26 10:47 ` Andy Shevchenko
2020-03-26 11:00 ` Wolfram Sang
1 sibling, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-03-26 10:47 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c, linux-renesas-soc
On Thu, Mar 26, 2020 at 11:16:47AM +0100, Wolfram Sang wrote:
> When I wanted to print the chosen values to debug output, I concluded
> that a helper function to parse one timing would be helpful.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> I am not entirely happy because 'dev' and 'u' is the same for each call.
> Then again, we can't use a for-loop over an array of parameters, because
> some default values depend on previously obtained timings.
>
> Looking for opinions here...
No objections. (We may bikeshed about namings, though)
However, looking into the code, I would go a bit further (perhaps as a separate
change) and export parsing of clock-frequency, because tons of drivers only
need one property, i.e. clock-frequency out of firmware.
>
> drivers/i2c/i2c-core-base.c | 78 ++++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 474baaf8c9e7..60b0aa246af2 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1609,6 +1609,18 @@ void i2c_del_adapter(struct i2c_adapter *adap)
> }
> EXPORT_SYMBOL(i2c_del_adapter);
>
> +static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p,
> + u32 def_val, bool use_def)
> +{
> + int ret;
> +
> + ret = device_property_read_u32(dev, prop_name, cur_val_p);
> + if (ret && use_def)
> + *cur_val_p = def_val;
> +
> + dev_dbg(dev, "%s: %u\n", prop_name, *cur_val_p);
> +}
> +
> /**
> * i2c_parse_fw_timings - get I2C related timing parameters from firmware
> * @dev: The device to scan for I2C timing properties
> @@ -1627,49 +1639,35 @@ EXPORT_SYMBOL(i2c_del_adapter);
> */
> void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults)
> {
> - int ret;
> -
> - 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;
> -
> - ret = device_property_read_u32(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns);
> - if (ret && use_defaults) {
> - if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
> - t->scl_rise_ns = 1000;
> - else if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> - t->scl_rise_ns = 300;
> - else
> - t->scl_rise_ns = 120;
> - }
> -
> - ret = device_property_read_u32(dev, "i2c-scl-falling-time-ns", &t->scl_fall_ns);
> - if (ret && use_defaults) {
> - if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> - t->scl_fall_ns = 300;
> - else
> - t->scl_fall_ns = 120;
> - }
> + bool u = use_defaults;
> + u32 d;
>
> - 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;
> + i2c_parse_timing(dev, "clock-frequency", &t->bus_freq_hz,
> + I2C_MAX_STANDARD_MODE_FREQ, u);
>
> - 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;
> -
> - ret = device_property_read_u32(dev, "i2c-sda-hold-time-ns", &t->sda_hold_ns);
> - if (ret && use_defaults)
> - t->sda_hold_ns = 0;
> -
> - 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;
> + if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
> + d = 1000;
> + else if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> + d = 300;
> + else
> + d = 120;
> + i2c_parse_timing(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns, d, u);
>
> - 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;
> + if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> + d = 300;
> + else
> + d = 120;
> + i2c_parse_timing(dev, "i2c-scl-falling-time-ns", &t->scl_fall_ns, d, u);
> +
> + i2c_parse_timing(dev, "i2c-scl-internal-delay-ns",
> + &t->scl_int_delay_ns, 0, u);
> + i2c_parse_timing(dev, "i2c-sda-falling-time-ns", &t->sda_fall_ns,
> + t->scl_fall_ns, u);
> + i2c_parse_timing(dev, "i2c-sda-hold-time-ns", &t->sda_hold_ns, 0, u);
> + i2c_parse_timing(dev, "i2c-digital-filter-width-ns",
> + &t->digital_filter_width_ns, 0, u);
> + i2c_parse_timing(dev, "i2c-analog-filter-cutoff-frequency",
> + &t->analog_filter_cutoff_freq_hz, 0, u);
> }
> EXPORT_SYMBOL_GPL(i2c_parse_fw_timings);
>
> --
> 2.20.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] i2c: refactor parsing of timings
2020-03-26 10:36 ` Geert Uytterhoeven
@ 2020-03-26 10:50 ` Andy Shevchenko
2020-03-26 10:52 ` Wolfram Sang
1 sibling, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-03-26 10:50 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Wolfram Sang, Linux I2C, Linux-Renesas
On Thu, Mar 26, 2020 at 11:36:44AM +0100, Geert Uytterhoeven wrote:
> On Thu, Mar 26, 2020 at 11:17 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > When I wanted to print the chosen values to debug output, I concluded
> > that a helper function to parse one timing would be helpful.
...
> > +static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p,
> > + u32 def_val, bool use_def)
> > +{
> > + int ret;
> > +
> > + ret = device_property_read_u32(dev, prop_name, cur_val_p);
> > + if (ret && use_def)
> > + *cur_val_p = def_val;
>
> Alternatively, you could just preinitialize the value with the default value
> before calling this function, and ignoring ret.
> That would remove the need for both the def_val and use_def parameters.
Some drivers are using false to use_defaults. How they will survive this change?
(See rcar case, for instance)
> > + dev_dbg(dev, "%s: %u\n", prop_name, *cur_val_p);
> > +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] i2c: refactor parsing of timings
2020-03-26 10:36 ` Geert Uytterhoeven
2020-03-26 10:50 ` Andy Shevchenko
@ 2020-03-26 10:52 ` Wolfram Sang
2020-03-26 11:15 ` Geert Uytterhoeven
1 sibling, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2020-03-26 10:52 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Wolfram Sang, Linux I2C, Linux-Renesas, Andy Shevchenko
[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]
Hi Geert,
> > +{
> > + int ret;
> > +
> > + ret = device_property_read_u32(dev, prop_name, cur_val_p);
> > + if (ret && use_def)
> > + *cur_val_p = def_val;
>
> Alternatively, you could just preinitialize the value with the default value
> before calling this function, and ignoring ret.
> That would remove the need for both the def_val and use_def parameters.
I can't do that because if !use_def and ret, then the value must not be
changed.
> > + if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
> > + d = 1000;
> > + else if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> > + d = 300;
> > + else
> > + d = 120;
> > + i2c_parse_timing(dev, "i2c-scl-rising-time-ns", &t->scl_rise_ns, d, u);
> >
> > - 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;
> > + if (t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
> > + d = 300;
> > + else
> > + d = 120;
>
> Is the difference with above intentional, or an oversight?
If this is an oversight, then it is also in the I2C specs ;)
> if the former, I like the dreaded ternary operator (only) for cases like this:
>
> d = t->bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ ? 300 : 120
Yup, that would be an improvement!
Thanks,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] i2c: refactor parsing of timings
2020-03-26 10:47 ` Andy Shevchenko
@ 2020-03-26 11:00 ` Wolfram Sang
2020-03-26 11:16 ` Andy Shevchenko
0 siblings, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2020-03-26 11:00 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Wolfram Sang, linux-i2c, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 487 bytes --]
> However, looking into the code, I would go a bit further (perhaps as a separate
> change) and export parsing of clock-frequency, because tons of drivers only
> need one property, i.e. clock-frequency out of firmware.
Cool idea. Could be easily something like this (typed from the top of my
head):
static inline u32 i2c_parse_fw_bus_speed(struct device *dev)
{
u32 speed;
i2c_parse_timing(dev, "clock-frequency", &speed, I2C_MAX_STANDARD_MODE_FREQ, true);
return speed;
}
Or?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] i2c: refactor parsing of timings
2020-03-26 10:52 ` Wolfram Sang
@ 2020-03-26 11:15 ` Geert Uytterhoeven
2020-03-26 12:05 ` Andy Shevchenko
0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-03-26 11:15 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Wolfram Sang, Linux I2C, Linux-Renesas, Andy Shevchenko
Hi Wolfram,
On Thu, Mar 26, 2020 at 11:52 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > +{
> > > + int ret;
> > > +
> > > + ret = device_property_read_u32(dev, prop_name, cur_val_p);
> > > + if (ret && use_def)
> > > + *cur_val_p = def_val;
> >
> > Alternatively, you could just preinitialize the value with the default value
> > before calling this function, and ignoring ret.
> > That would remove the need for both the def_val and use_def parameters.
>
> I can't do that because if !use_def and ret, then the value must not be
> changed.
Of course the preinitialization must still be done conditionally:
if (use_defaults)
t->foo = DEFAULT_FOO;
i2c_parse_timing(dev, "foo-name", &t->foo);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] i2c: refactor parsing of timings
2020-03-26 11:00 ` Wolfram Sang
@ 2020-03-26 11:16 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-03-26 11:16 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Wolfram Sang, linux-i2c, linux-renesas-soc
On Thu, Mar 26, 2020 at 12:00:42PM +0100, Wolfram Sang wrote:
>
> > However, looking into the code, I would go a bit further (perhaps as a separate
> > change) and export parsing of clock-frequency, because tons of drivers only
> > need one property, i.e. clock-frequency out of firmware.
>
> Cool idea. Could be easily something like this (typed from the top of my
> head):
>
> static inline u32 i2c_parse_fw_bus_speed(struct device *dev)
> {
> u32 speed;
>
> i2c_parse_timing(dev, "clock-frequency", &speed, I2C_MAX_STANDARD_MODE_FREQ, true);
>
> return speed;
> }
Yes, looks like this, thanks!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] i2c: refactor parsing of timings
2020-03-26 11:15 ` Geert Uytterhoeven
@ 2020-03-26 12:05 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-03-26 12:05 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Wolfram Sang, Wolfram Sang, Linux I2C, Linux-Renesas
On Thu, Mar 26, 2020 at 12:15:22PM +0100, Geert Uytterhoeven wrote:
> On Thu, Mar 26, 2020 at 11:52 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = device_property_read_u32(dev, prop_name, cur_val_p);
> > > > + if (ret && use_def)
> > > > + *cur_val_p = def_val;
> > >
> > > Alternatively, you could just preinitialize the value with the default value
> > > before calling this function, and ignoring ret.
> > > That would remove the need for both the def_val and use_def parameters.
> >
> > I can't do that because if !use_def and ret, then the value must not be
> > changed.
>
> Of course the preinitialization must still be done conditionally:
>
> if (use_defaults)
> t->foo = DEFAULT_FOO;
> i2c_parse_timing(dev, "foo-name", &t->foo);
If the default *is* coming from timings structure?
Care to look at rcar case?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-26 12:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26 10:16 [RFC PATCH] i2c: refactor parsing of timings Wolfram Sang
2020-03-26 10:36 ` Geert Uytterhoeven
2020-03-26 10:50 ` Andy Shevchenko
2020-03-26 10:52 ` Wolfram Sang
2020-03-26 11:15 ` Geert Uytterhoeven
2020-03-26 12:05 ` Andy Shevchenko
2020-03-26 10:47 ` Andy Shevchenko
2020-03-26 11:00 ` Wolfram Sang
2020-03-26 11:16 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).