| Message ID | 20200326101647.1756-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC] i2c: refactor parsing of timings | expand |
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
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 >
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); > > +}
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
> 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?
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
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!
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?
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);
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(-)