Re: [RFC PATCH] i2c: refactor parsing of timings

[Wolfram Sang <wsa@the-dreams.de>]

[-- 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 --]