Re: [PATCH v7 3/4] i2c: npcm: use i2c frequency table

[Tali Perry]

Hi Andi,

>
> > > > -     /* 100KHz and below: */
> > > > -     if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
> > > > -             sclfrq = src_clk_khz / (bus_freq_khz * 4);
> > > > -
> > > > -             if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
> > > > -                     return -EDOM;
> > > > -
> > > > -             if (src_clk_khz >= 40000)
> > > > -                     hldt = 17;
> > > > -             else if (src_clk_khz >= 12500)
> > > > -                     hldt = 15;
> > > > -             else
> > > > -                     hldt = 7;
> > > > -     }
> > > > -
> > > > -     /* 400KHz: */
> > > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
> > > > -             sclfrq = 0;
> > > > +     switch (bus_freq_hz) {
> > > > +     case I2C_MAX_STANDARD_MODE_FREQ:
> > > > +             smb_timing = smb_timing_100khz;
> > > > +             table_size = ARRAY_SIZE(smb_timing_100khz);
> > > > +             break;
> > > > +     case I2C_MAX_FAST_MODE_FREQ:
> > > > +             smb_timing = smb_timing_400khz;
> > > > +             table_size = ARRAY_SIZE(smb_timing_400khz);
> > > >               fast_mode = I2CCTL3_400K_MODE;
> > > > -
> > > > -             if (src_clk_khz < 7500)
> > > > -                     /* 400KHZ cannot be supported for core clock < 7.5MHz */
> > > > -                     return -EDOM;
> > > > -
> > > > -             else if (src_clk_khz >= 50000) {
> > > > -                     k1 = 80;
> > > > -                     k2 = 48;
> > > > -                     hldt = 12;
> > > > -                     dbnct = 7;
> > > > -             }
> > > > -
> > > > -             /* Master or Slave with frequency > 25MHz */
> > > > -             else if (src_clk_khz > 25000) {
> > > > -                     hldt = clk_coef(src_clk_khz, 300) + 7;
> > > > -                     k1 = clk_coef(src_clk_khz, 1600);
> > > > -                     k2 = clk_coef(src_clk_khz, 900);
> > > > -             }
> > > > -     }
> > > > -
> > > > -     /* 1MHz: */
> > > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
> > > > -             sclfrq = 0;
> > > > +             break;
> > > > +     case I2C_MAX_FAST_MODE_PLUS_FREQ:
> > > > +             smb_timing = smb_timing_1000khz;
> > > > +             table_size = ARRAY_SIZE(smb_timing_1000khz);
> > > >               fast_mode = I2CCTL3_400K_MODE;
> > > > -
> > > > -             /* 1MHZ cannot be supported for core clock < 24 MHz */
> > > > -             if (src_clk_khz < 24000)
> > > > -                     return -EDOM;
> > > > -
> > > > -             k1 = clk_coef(src_clk_khz, 620);
> > > > -             k2 = clk_coef(src_clk_khz, 380);
> > > > -
> > > > -             /* Core clk > 40 MHz */
> > > > -             if (src_clk_khz > 40000) {
> > > > -                     /*
> > > > -                      * Set HLDT:
> > > > -                      * SDA hold time:  (HLDT-7) * T(CLK) >= 120
> > > > -                      * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
> > > > -                      */
> > > > -                     hldt = clk_coef(src_clk_khz, 120) + 7;
> > > > -             } else {
> > > > -                     hldt = 7;
> > > > -                     dbnct = 2;
> > > > -             }
> > > > +             break;
> > > > +     default:
> > > > +             return -EINVAL;
> > >
> > > There is here a slight change of behaiour which is not mentioned
> > > in the commit log. Before the user could set a bus_freq_hz which
> > > had to be <= I2C_MAX_..._MODE_FREQ, while now it has to be
> > > precisely that.
> > >
> > > Do we want to check what the user has set in the DTS?
> >
> > The driver checks the bus frequency the user sets in the DTS.
>
> yes, but before it was checking the value within a range, while
> now it's checking the exact value.
>
> The difference is that now if you don't set the exact value you
> get EINVAL, not before.
>
> Andi

Previously the driver was rounding numbers down.
The driver has settings for 100, 400, 1000 KHz.
but what happens if the user asks for 200KHz?
Some of the coefficients were calculated according to the equations,
and some were hard-coded values per setting.
We don't want to support this mix.
We prefer the users to ask for numbers that are one of the three
supported values and block unknown input values.

Thanks ,
Tali