Re: [PATCH v3 5/6] i2c: npcm: use i2c frequency table

[Tyrone Ting <warp5tw@gmail.com>]

Hi Andy:

Your comments will be addressed.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年9月13日 週五 下午6:39寫道：
>
> On Fri, Sep 13, 2024 at 06:14:45PM +0800, warp5tw@gmail.com wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > Modify i2c frequency from table parameters
> > for NPCM i2c modules.
> >
> > Supported frequencies are:
> >
> > 1. 100KHz
> > 2. 400KHz
> > 3. 1MHz
> >
> > The original equations were tested on a variety of chips and base clocks.
> > Since we added devices that use higher frequencies of the module we
> > saw that there is a mismatch between the equation and the actual
> > results on the bus itself, measured on scope.
> >
> > Meanwhile, the equations were not accurate to begin with.
> > They are an approximation of the ideal value. The ideal value is
> > calculated per frequency of the core module.
> >
> > So instead of using the equations we did an optimization per module
> > frequency, verified on a device.
> >
> > Most of the work was focused on the rise time of the SCL and SDA,
> > which depends on external load of the bus and PU.
> >
> > Different PCB designs, or specifically to this case: the number
> > and type of targets on the bus, impact the required values for
> > the timing registers.
> >
> > Users can recalculate the numbers for each bus and get an even better
> > optimization, but our users chose not to.
> >
> > We manually picked values per frequency that match the entire valid
> > range of targets (from 1 to max number). Then we check against the
> > AMR described in SMB spec and make sure that none of the values
> > is exceeding.
> >
> > This process was led by the chip architect and included a lot of testing.
>
> ...
>
> > +     {.core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x4, .sclfrq = 0xFB, .scllt = 0x0,
> > +     .sclht  = 0x0, .fast_mode = false },
>
> It seems you have no leading space, while having trailing one. Also the split
> seems a bit illogical to me, I would rather do like
>
>         {
>                 .core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x4,
>                 .sclfrq = 0xFB, .scllt = 0x0, .sclht  = 0x0,
>                 .fast_mode = false,
>         },
>
> Yes, this takes more lines, but also more flexible to the updates in the
> future. Also mind the trailing comma at the last member assignment.
>
> ...
>
> > +     for (scl_table_cnt = 0 ; scl_table_cnt < table_size ; scl_table_cnt++)
>
> Extra spaces...
>
> > +             if (bus->apb_clk >= smb_timing[scl_table_cnt].core_clk)
> > +                     break;
>
> ...
>
> >       /* bits [8:7] are in I2CCTL3 reg */
> > -     iowrite8(fast_mode | FIELD_PREP(I2CCTL3_SCLFRQ8_7, (sclfrq >> 7) & 0x3),
> > +     iowrite8(fast_mode | FIELD_PREP(I2CCTL3_SCLFRQ8_7, (smb_timing[scl_table_cnt].sclfrq >> 7)
> > +              & 0x3),
> >                bus->reg + NPCM_I2CCTL3);
>
> This has broken (illogical) indentation. Consider
>
>         iowrite8(FIELD_PREP(I2CCTL3_SCLFRQ8_7, (smb_timing[scl_table_cnt].sclfrq >> 7) & 0x3) |
>                  fast_mode,
>                  bus->reg + NPCM_I2CCTL3);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thank you.

Regards,
Tyrone