| Message ID | 20240913101445.16513-1-kfting@nuvoton.com |
|---|---|
| Headers | show |
| Series | i2c: npcm: Bug fixes read/write operation, checkpatch | expand |
On Fri, Sep 13, 2024 at 06:14:40PM +0800, warp5tw@gmail.com wrote: > From: Tyrone Ting <kfting@nuvoton.com> > > This patchset includes the following fixes: > > - Enable the target functionality in the interrupt handling routine > when the i2c transfer is about to finish. > - Correct the read/write operation procedure. > - Introduce a software flag to handle the bus error (BER) condition > which is not caused by the i2c transfer. > - Modify timeout calculation. > - Assign the client address earlier logically. > - Use an i2c frequency table for the frequency parameters assignment. > - Coding style fix. > > The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards. Somehow your 6th patch becomes independent from the email thread. (Initially I thought it was a separate fix) Please, check what's going on with email settings on your side.
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);
Hi Andy: Thank you for your feedback. Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年9月13日 週五 下午6:31寫道： > > On Fri, Sep 13, 2024 at 06:14:40PM +0800, warp5tw@gmail.com wrote: > > From: Tyrone Ting <kfting@nuvoton.com> > > > > This patchset includes the following fixes: > > > > - Enable the target functionality in the interrupt handling routine > > when the i2c transfer is about to finish. > > - Correct the read/write operation procedure. > > - Introduce a software flag to handle the bus error (BER) condition > > which is not caused by the i2c transfer. > > - Modify timeout calculation. > > - Assign the client address earlier logically. > > - Use an i2c frequency table for the frequency parameters assignment. > > - Coding style fix. > > > > The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards. > > The series titled as "Bug fixes...", however I haven't noticed many > Fixes tags in it. Understood. I'll remove the "Bug fixes" in the title in the next patch set. > > -- > With Best Regards, > Andy Shevchenko > > Thank you. Regards, Tyrone
From: Tyrone Ting <kfting@nuvoton.com> This patchset includes the following fixes: - Enable the target functionality in the interrupt handling routine when the i2c transfer is about to finish. - Correct the read/write operation procedure. - Introduce a software flag to handle the bus error (BER) condition which is not caused by the i2c transfer. - Modify timeout calculation. - Assign the client address earlier logically. - Use an i2c frequency table for the frequency parameters assignment. - Coding style fix. The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards. Addressed comments from: - Andi Shyti : https://lore.kernel.org/lkml/ cfdfldh5tuhb4r5pdpgolcr2roeewsobedet2uvmpbnqlw5yh4@c4a2szsbs2r2/ - Andi Shyti : https://lore.kernel.org/lkml/ stnyjmnqdobzq2f2ntq32tu4kq6ohsxyevjn5rgz3uu2qncuzl@nt4ifscgokgj/ - Andy Shevchenko : https://lore.kernel.org/lkml/ZtWnd8bmiu-M4fQg @smile.fi.intel.com/ - Andy Shevchenko : https://lore.kernel.org/lkml/Zt7Nn9uJSeHFUZZF @smile.fi.intel.com/ - Andi Shyti : https://lore.kernel.org/lkml/ 2kqhf2ad3omx3dsjucrqhtnonnox7ghtp7vkogrwrdfh3dgg2o@4cpa4gfg6c3f/ Changes since version 2: - Add more explanations in the commit message and code modification. - Use lower character names for declarations. - Remove Fixes tags in commits which are not to fix bugs. Changes since version 1: - Restore the npcm_i2caddr array length to fix the smatch warning. - Remove unused variables. - Handle the condition where scl_table_cnt reaches to the maximum value. - Fix the checkpatch warning. Charles Boyer (1): i2c: npcm: Enable slave in eob interrupt Tyrone Ting (5): i2c: npcm: correct the read/write operation procedure i2c: npcm: use a software flag to indicate a BER condition i2c: npcm: Modify timeout evaluation mechanism i2c: npcm: Modify the client address assignment i2c: npcm: use i2c frequency table drivers/i2c/busses/i2c-npcm7xx.c | 281 ++++++++++++++++++++----------- 1 file changed, 179 insertions(+), 102 deletions(-) base-commit: 48b83f5f68edb4d19771d5ecc54bbbc37166f753